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

upcoming: [M3-7463] - Disable Billing access user permission for child users #10045

Merged
merged 9 commits into from
Jan 10, 2024

Conversation

mjac0bs
Copy link
Contributor

@mjac0bs mjac0bs commented Jan 9, 2024

Description 📝

As a child user, the UI of the permissions page will be the same as global or non-parent/child, except for Billing Access. Billing Access will be set to "Read Only" with the option for unrestricted child users to switch it to "None" for other child users. "Read-Write" will always be disabled. In other words, child account users will have no permission greater than "Read Only" billing access.

Why are we enforcing this in the UI, rather than relying on the account_access grant?
For an unrestricted (admin) child account user's account_access (which determines Billing Access), the API will return read_write in order for the user to have access to the rest of the user permissions, including managing those of their proxy user. Therefore, we need to make an additional check of the user_type to ensure on the front-end that we actually restrict billing access for child users.

Changes 🔄

  • The Billing Access default setting is configured to "Read Only" for child users.
  • Child users have the option to switch Billing Access to "None."
  • The "Read-Write" option is consistently disabled and cannot be selected by child users.
  • Adds test coverage for the above to user-permissions.spec.ts.

Preview 📷

Screenshot 2024-01-08 at 4 19 02 PM

How to test 🧪

Prerequisites

(How to setup test environment)

  • Check out this PR and yarn dev.
  • In devtools, make sure the feature flag is on and enable the MSW.
  • Go to serverHandlers.ts and edit the account/users/:user request to the following in order to mock a child user:
  rest.get('*/account/users/:user', (req, res, ctx) => {
  // Parent/Child: switch the `user_type` depending on what account view you need to mock.
  return res(ctx.json(accountUserFactory.build({ user_type: 'child' })));
  }),

Verification steps

(How to verify changes)

  • Go to http://localhost:3000/account/users and navigate to the User Permissions for a mock restricted user.
  • Confirm the Changes described above.
  • Toggle off the feature flag and confirm that Read-Write billing access is not disabled and is the default selection.
  • Play around with the Billing Access options and make sure there are no regressions in the selected options (e.g. no more than one option can be selected at the same time).
  • Verify test passes:
yarn cy:run -s "cypress/e2e/core/account/user-permissions.spec.ts"

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@mjac0bs mjac0bs marked this pull request as ready for review January 9, 2024 20:52
@mjac0bs mjac0bs requested review from a team as code owners January 9, 2024 20:52
@mjac0bs mjac0bs requested review from cliu-akamai, jdamore-linode and bnussman-akamai and removed request for a team January 9, 2024 20:52
Copy link

github-actions bot commented Jan 9, 2024

Coverage Report:
Base Coverage: 79.86%
Current Coverage: 79.86%

Copy link
Contributor

@jdamore-linode jdamore-linode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @mjac0bs!

Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Do childAccountAccessEnabled and isAccountAccessRestricted need to be in state? Can we just derive them from existing data?

@mjac0bs
Copy link
Contributor Author

mjac0bs commented Jan 10, 2024

The CI e2e failures are irrelevant to this PR and I connected with Banks offline regarding the above comment - we can't derive since user data isn't already stored in state.

Going to go ahead and merge. 🚢

@mjac0bs mjac0bs added the Approved Multiple approvals and ready to merge! label Jan 10, 2024
@mjac0bs mjac0bs merged commit 81174ef into linode:develop Jan 10, 2024
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Parent / Child Account
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants