Skip to content
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

Merged
merged 20 commits into from
Jul 11, 2024

Conversation

EMaksy
Copy link
Member

@EMaksy EMaksy commented Jul 9, 2024

Description

This pr adds abilities/permissions for the settings view.
Main features:

  • Added ['all:api_key_settings']; [all:suma_settings]; ['all:activity_logs_settings']; permissions which can be assigned to a user.
  • Disabled the Generate Key button for API creation [all:api_key_settings];
  • Disabled Edit Settings and Clear Settings Button if the user does not have [all: all] or [all:suma_settings];
  • Disabled Edit Settings button for activity logs if the user does not have [all: all] or ['all:activity_logs_settings'];
  • Protecting backend API calls with policies
  • Added and updated stories for storybook
  • Added migration for new abilities

Demo

User creation with new permissions

Default user with disabled buttons

User with right permissions

Did you add the right label?

  • DONE

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.

  • DONE

Did you update the documentation?

Added moduledoc in new policies for explanation

  • DONE

@EMaksy EMaksy added enhancement New feature or request javascript Pull requests that update Javascript code elixir Pull requests that update Elixir code test env Create an ephimeral environment for the pr branch labels Jul 9, 2024
@EMaksy EMaksy self-assigned this Jul 9, 2024
@EMaksy EMaksy force-pushed the forbid_change_settings_without_permission branch from dba7e05 to abaf282 Compare July 9, 2024 09:11
Copy link
Member

@CDimonaco CDimonaco left a 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.

assets/js/common/ActivityLogsConfig/ActivityLogsConfig.jsx Outdated Show resolved Hide resolved
assets/js/common/SuseManagerConfig/SuseManagerConfig.jsx Outdated Show resolved Hide resolved
lib/trento/settings/policy.ex Outdated Show resolved Hide resolved
lib/trento/settings/policy.ex Outdated Show resolved Hide resolved
lib/trento_web/controllers/v1/settings_controller.ex Outdated Show resolved Hide resolved
lib/trento/software_updates/policy.ex Outdated Show resolved Hide resolved
lib/trento/software_updates/policy.ex Outdated Show resolved Hide resolved
lib/trento/settings/policy.ex Outdated Show resolved Hide resolved
@CDimonaco CDimonaco requested a review from arbulu89 July 9, 2024 12:13
Copy link
Contributor

@arbulu89 arbulu89 left a 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

lib/trento/settings/policy.ex Outdated Show resolved Hide resolved
lib/trento/software_updates.ex Outdated Show resolved Hide resolved
lib/trento_web/controllers/v1/settings_controller.ex Outdated Show resolved Hide resolved
lib/trento/settings/api_key_settings.ex Show resolved Hide resolved
assets/js/pages/SettingsPage/SettingsPage.jsx Outdated Show resolved Hide resolved
@EMaksy EMaksy force-pushed the forbid_change_settings_without_permission branch from abaf282 to 075c8a1 Compare July 10, 2024 15:22
@EMaksy EMaksy force-pushed the forbid_change_settings_without_permission branch from 075c8a1 to d346b0a Compare July 10, 2024 15:24
@EMaksy EMaksy force-pushed the forbid_change_settings_without_permission branch from d346b0a to ca98a4d Compare July 10, 2024 15:27
@EMaksy EMaksy force-pushed the forbid_change_settings_without_permission branch from f1f8c4b to e2653a8 Compare July 10, 2024 15:55
@EMaksy EMaksy marked this pull request as ready for review July 10, 2024 16:16
@EMaksy EMaksy requested review from CDimonaco and arbulu89 July 10, 2024 16:17
Copy link
Contributor

@arbulu89 arbulu89 left a 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!

lib/trento/settings/policy.ex Outdated Show resolved Hide resolved
lib/trento/settings/policy.ex Outdated Show resolved Hide resolved
lib/trento_web/controllers/v1/settings_controller.ex Outdated Show resolved Hide resolved
test/trento/settings/policy_test.exs Outdated Show resolved Hide resolved
assets/js/pages/SettingsPage/SettingsPage.jsx Outdated Show resolved Hide resolved
@EMaksy EMaksy force-pushed the forbid_change_settings_without_permission branch from 3222473 to 58cbff5 Compare July 11, 2024 08:41
@EMaksy EMaksy force-pushed the forbid_change_settings_without_permission branch from 58cbff5 to cf923a5 Compare July 11, 2024 08:47
Copy link
Member

@CDimonaco CDimonaco left a 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

@EMaksy EMaksy merged commit e231aa9 into main Jul 11, 2024
26 checks passed
@EMaksy EMaksy deleted the forbid_change_settings_without_permission branch July 11, 2024 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
elixir Pull requests that update Elixir code enhancement New feature or request env Create an ephimeral environment for the pr branch javascript Pull requests that update Javascript code test
Development

Successfully merging this pull request may close these issues.

3 participants