-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add permissions to the Settings View #2760
Conversation
dba7e05
to
abaf282
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall a good job,
Please refactor the policies, and in particular the settings controller, the third param of the authorize function, should contain the "context" on which the authorize function is acting, we can't have ApiKeySettings for something not related to the apikeysettings, in general in the controller you can match on actions and on whatever conn related and pass the params accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some few comments together with what @CDimonaco said
assets/js/common/ActivityLogsConfig/ActivityLogsConfig.stories.jsx
Outdated
Show resolved
Hide resolved
assets/js/common/ActivityLogsConfig/ActivityLogsConfig.stories.jsx
Outdated
Show resolved
Hide resolved
assets/js/common/ActivityLogsConfig/ActivityLogsConfig.test.jsx
Outdated
Show resolved
Hide resolved
assets/js/common/SuseManagerConfig/SuseManagerConfig.stories.jsx
Outdated
Show resolved
Hide resolved
abaf282
to
075c8a1
Compare
075c8a1
to
d346b0a
Compare
d346b0a
to
ca98a4d
Compare
f1f8c4b
to
e2653a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really good improvement @EMaksy
Nice!
Some few nitpick comments.
The unique one that I would really like you having a look is the passing of configEditPermittedFor
to SUMA configuration. It looks not neccesary, you can create the constant there and avoid one prop that we don't need. The same for activity log.
Once you change this, ready to merge!
assets/js/common/ActivityLogsConfig/ActivityLogsConfig.stories.jsx
Outdated
Show resolved
Hide resolved
3222473
to
58cbff5
Compare
58cbff5
to
cf923a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job!
LGTM 🚀
@arbulu89 I proposed Eugen to have also the get action in the get_policy_resource in the settings controller, to have more consistency, but it's true we don't use them, really up to you guys, no strong opinions :D
Description
This pr adds abilities/permissions for the settings view.
Main features:
['all:api_key_settings']; [all:suma_settings]; ['all:activity_logs_settings'];
permissions which can be assigned to a user.[all:api_key_settings];
[all: all]
or[all:suma_settings];
[all: all]
or['all:activity_logs_settings'];
Demo
Did you add the right label?
How was this tested?
Added frontend tests to check if buttons are disabled.
Added policy and controller test in backend.
E2E test will follow in a follow-up pr.
Describe the tests that have been added/changed for this new behavior.
Did you update the documentation?
Added moduledoc in new policies for explanation