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-7613] - Grants and Permissions for Placement Groups #10257

Merged
merged 9 commits into from
Mar 22, 2024

Conversation

carrillo-erik
Copy link
Contributor

@carrillo-erik carrillo-erik commented Mar 5, 2024

Description πŸ“

This PR sets up the Grants and Permissions for users to be able to create, delete, and update Placement Groups. More specifically, write access to create a Placement Group will be dictated by the add_linodes grant. Access to assign/unassign an individual Linode to a Placement Group will be based on having read_write access to the specific Linode.

Changes πŸ”„

  • The majority of the changes revolve around the useRestrictedGlobalGrantCheck hook. Once it's determined if the user has the add_linodes grant, certain components will be hidden from the user or disabled to the user; and in other cases, we display components to convey a message to the user.
  • The other change in this PR comes from the useIsResourceRestricted hook. This time, we are interested to know if a user has read_write access for specific Linodes in order to assign (add) them to a Placement Group.

Preview πŸ“·

Screenshot 2024-03-11 at 10 07 00 AM
Screenshot 2024-03-11 at 9 59 39 AM
Screenshot 2024-03-11 at 10 00 48 AM
Screenshot 2024-03-11 at 9 59 14 AM

How to test πŸ§ͺ

Prerequisites

(How to setup test environment)

  • Enable the following using the Cloud Manager Dev Tools:
    • Placement Groups feature flag
    • MSW
  • Read the comments in the serverHandlers.ts file on line 559 and line 574.

Reproduction steps

  • Given that we are still working with API mocks, the best way to verify the UI is to follow the instructions from the last bullet point in the How to test section above. Kudos to @mjac0bs for this solution!
    • In the alternative, one can negate the isLinodeReadOnly value in the files touched by this PR.
  • As a consequence of the previous point, some actions like deleting a Placement Group or unassigning a Linode will not take effect.
  • Testing the functionality of the useIsResourceRestricted hook in the current state proved to be difficult since it would require having a testing account which we can set the grants and permissions, then log into that account while using the MSW. After the API hits the alpha environment, this will be more thoroughly tested.
  • The scenario where a user may not have the necessary access to update Placement Groups but have knowledge of a url route was considered, and can be verified by negating the isLinodeReadOnly value on the Placement Groups landing page and then visiting http://localhost:3000/placement-groups/create or http://localhost:3000/placement-groups/1/linodes

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

@carrillo-erik carrillo-erik marked this pull request as ready for review March 11, 2024 17:12
@carrillo-erik carrillo-erik requested a review from a team as a code owner March 11, 2024 17:12
@carrillo-erik carrillo-erik requested review from jdamore-linode and mjac0bs and removed request for a team March 11, 2024 17:12
Copy link

github-actions bot commented Mar 11, 2024

Coverage Report: βœ…
Base Coverage: 81.64%
Current Coverage: 81.65%

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Nice job with pointing out the drawer can be reached by URL in the testing instructions.

Left some feedback in comments, and I also wanted to mention how I tested at least the read only linodes part with a couple updates to the mocks that allow us to mock add_linodes as false.

  rest.get('*/profile', (req, res, ctx) => {
    const profile = profileFactory.build({
      restricted: true, // This is important! The grants endpoint is only called for restricted users!
      user_type: 'default',
    });
    return res(ctx.json(profile));
  }),
  rest.get('*/profile/grants', (req, res, ctx) => {
    return res(
      ctx.json(grantsFactory.build({ global: { add_linodes: false }},))
    );
  }),

@abailly-akamai
Copy link
Contributor

@jaalah-akamai would love to have your eyes on this initial permission PR to confirm the overall direction before I dive into a review. thx!

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Addressed feedback looks good. Couple more things:

As far as URLs, it looks like a user with read only access could still potentially navigate to the edit drawer and delete modal via URL with the placement group id, so these UIs would need disabled state too.

e.g. http://localhost:3000/placement-groups/edit/1
Screenshot 2024-03-18 at 11 42 09 AM

e.g. http://localhost:3000/placement-groups/delete/1
Screenshot 2024-03-18 at 11 51 23 AM

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.

@carrillo-erik please rebase at your earliest convenience so we can rely on serverHandler to test the PR, thx.

@@ -19,6 +19,7 @@ export interface LandingHeaderProps {
buttonDataAttrs?: { [key: string]: boolean | string };
createButtonText?: string;
disabledCreateButton?: boolean;
disableEditButton?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

this should read disabledBreadcrumbEditButton for clarity since Landing header has other CTAs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. The recent code push reflects this change.

@@ -146,7 +159,7 @@ export const PlacementGroupsCreateDrawer = (
autoFocus: true,
}}
aria-label="Label for the Placement Group"
disabled={false}
disabled={disabledCreateButton || false}
Copy link
Contributor

Choose a reason for hiding this comment

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

same here - the naming convention could lead to confusion.disabledCreateButton implies only the button and disabling the form using this variable isn't optimal. The button can also be disabled for other reasons limits) so we could run into logic problems down the line.

How about renaming disabledCreateButton with disablePlacementGroupCreation (or similar) where it applies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as the other comment.

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

βœ… Confirmed that the edit drawer and delete modal, when accessed via URL, are disabled as expected when the user has read only grant access.

Thanks for the update to the error notice in the create drawer, too.

Cannot Edit Cannot Delete
Screenshot 2024-03-22 at 1 56 31 PM Screenshot 2024-03-22 at 1 56 18 PM

@mjac0bs mjac0bs added the Approved Multiple approvals and ready to merge! label Mar 22, 2024
@carrillo-erik carrillo-erik merged commit e95cfbc into linode:develop Mar 22, 2024
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! Placement Groups
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants