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-7413] - Update existing user account endpoints and mocks for Parent/Child Account Switching #9942

Merged
merged 9 commits into from
Nov 29, 2023

Conversation

mjac0bs
Copy link
Contributor

@mjac0bs mjac0bs commented Nov 28, 2023

Description 📝

GET /account/users and GET /account/users/<username> requests will return a new user_type field to allow us to distinguish between users.

GET /account/users/<username>/grants will return a new grant type: child_account_access, which will be true for parent accounts with access to child account endpoints.

These additions to existing endpoints set us up to implement account switching and user permissions flows.

Changes 🔄

  • Updated the User interface and factory to include the new user_type field
  • Updated the Account interface and Grant factory to include the new child_account_access field
  • Updated the MSW account/users endpoint to include a child, parent, and proxy user
    • IRL, we'd never see all three parent, proxy, and child users on one account, but mocking each user type will allow us to implement the upcoming User & Grants and User Permissions tickets for every user.

Preview 📷

GET /account/users
Screenshot 2023-11-28 at 1 35 00 PM

GET /account/users/<username>
Screenshot 2023-11-28 at 1 45 34 PM

GET /account/users/<username>/grants
Screenshot 2023-11-28 at 1 40 04 PM

How to test 🧪

Prerequisites

(How to setup test environment)

  • Check out this branch and yarn up.
  • Turn the Mock Service Worker on.

Verification steps

(How to verify changes)

  • Open the browser dev tools and navigate to the Network tab.
  • Go to localhost:3000/accounts/users (the Users & Grants page).
  • See the internal ticket for a link to the API spec and verify that section 6.2 (existing endpoints) reflects the changes in this PR. Specifically:
    • Examine the payload of GET /account/users and verify the user_type field is present and accurate with parent/child/proxy accounts and null in all other cases.
    • Visit the ParentUser's permissions page (http://localhost:3000/account/users/ParentUser/permissions) and verify the payload of GET /account/users/<username>/grants returns child_account_access set to true. In all other cases, it should be false.
    • Navigate to various users' user/profile or user/permissions pages and verify that GET /account/users/<username> returns the expected value for user_type in the payload.

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

ssh_keys: string[];
tfa_enabled: boolean;
username: string;
user_type: UserType | null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the new user_type field. All of the other changes in the User interface were just reordering.

@@ -169,6 +172,7 @@ export type GlobalGrantTypes =
| 'add_longview'
| 'longview_subscription'
| 'account_access'
| 'child_account_access'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the new child_account_access grant that returns true for all admin parent accounts and any additional parent accounts that the admin parent grants permission to access the child accounts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not that I am very privy to this architecture, but I was expecting an array of child accounts the parent has access to instead of just a boolean. I know it's handled in another way but just putting this thought out there for posterity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The array of child accounts will be returned by a new API endpoint, GET /account/child-accounts! This boolean is a new global grant that allows an unrestricted parent account to decide whether or not any other users associated with that parent account (i.e. non-admin, potentially restricted parent account users) will also have access to the data returned in GET /account/child-accounts.

Comment on lines -1142 to +1174
return res(ctx.json(profileFactory.build()));
return res(ctx.json(accountUserFactory.build()));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was an existing bug... The mock request to*/account/users/:user was returning profile instead of account data.

Comment on lines +1197 to +1206
add_domains: false,
add_firewalls: false,
add_images: false,
add_linodes: false,
add_longview: false,
add_nodebalancers: false,
add_stackscripts: false,
add_volumes: false,
add_vpcs: false,
cancel_account: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Proxy accounts have read only permissions by default and cannot be canceled.

ctx.json(
grantsFactory.build({
global: {
cancel_account: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Child accounts cannot be canceled while they have a user_type of "child".

Comment on lines +1221 to +1222
cancel_account: false,
child_account_access: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Parent accounts cannot be canceled while they have a user_type of "parent". This "admin" parent account has default child_account_access granted.

@mjac0bs mjac0bs marked this pull request as ready for review November 28, 2023 21:47
@mjac0bs mjac0bs requested a review from a team as a code owner November 28, 2023 21:47
@mjac0bs mjac0bs requested review from jdamore-linode, dwiley-akamai and jaalah-akamai and removed request for a team November 28, 2023 21:47
Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Tested locally and confirmed scenari outlined in PR description

  • payload of GET /account/users and verify the user_type field is present and accurate with parent/child/proxy accounts and null in all other cases.
  • ParentUser's permissions page returns child_account_access set to true. In all other cases, it should be false.
  • other users verify that GET /account/users/ returns the expected value for user_type in the payload.

@@ -169,6 +172,7 @@ export type GlobalGrantTypes =
| 'add_longview'
| 'longview_subscription'
| 'account_access'
| 'child_account_access'
Copy link
Contributor

Choose a reason for hiding this comment

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

Not that I am very privy to this architecture, but I was expecting an array of child accounts the parent has access to instead of just a boolean. I know it's handled in another way but just putting this thought out there for posterity.

@mjac0bs mjac0bs added the @linode/api-v4 Changes are made to the @linode/api-v4 package label Nov 29, 2023
Copy link
Contributor

@jaalah-akamai jaalah-akamai left a comment

Choose a reason for hiding this comment

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

👪 ✅

@mjac0bs mjac0bs merged commit a02081f into linode:develop Nov 29, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@linode/api-v4 Changes are made to the @linode/api-v4 package Parent / Child Account
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants