Skip to content

refactor: [M3-7687] - Linodes Restricted User Experience 2/2#10227

Merged
jaalah-akamai merged 10 commits intolinode:developfrom
jaalah-akamai:M3-7687-v2
Mar 7, 2024
Merged

refactor: [M3-7687] - Linodes Restricted User Experience 2/2#10227
jaalah-akamai merged 10 commits intolinode:developfrom
jaalah-akamai:M3-7687-v2

Conversation

@jaalah-akamai
Copy link
Contributor

@jaalah-akamai jaalah-akamai commented Feb 23, 2024

Description 📝

Overall improvement to the Linode restricted experience. This PR disables lot's of buttons and fields that a user doesn't have access to.

Changes 🔄

  • Removed the banners from beneath each tab on the Linodes detail page since we will display an overall banner at the top of the page.
  • If you don't have add_linodes grant, we disable create buttons. If you do have the grant, you will be able to initiate the creation of Linodes, as well as manage those you have personally established. However, restrictions may still apply for modifying of Linodes configured by the administrator.

Target release date 🗓️

03/04/2024

Preview 📷

BEFORE

Screenshot 2024-02-26 at 5 01 59 PM Screenshot 2024-02-26 at 5 02 24 PM
Screenshot 2024-02-26 at 5 02 40 PM Screenshot 2024-02-26 at 5 02 07 PM

AFTER

Screenshot 2024-02-26 at 5 09 06 PM Screenshot 2024-02-26 at 5 08 27 PM
Screenshot 2024-02-26 at 5 14 58 PM Screenshot 2024-02-26 at 5 15 33 PM
Screenshot 2024-02-26 at 5 20 57 PM

How to test 🧪

Prerequisites

(How to setup test environment)

  • Log into two accounts side by side:
    • An unrestricted admin user account: full access
    • A restricted user account (use Incognito for this)
      • Start with Read Only for everything

Reproduction steps

(How to reproduce the issue, if applicable)

  • Create a linode from an unrestricted admin account and give your restricted user view only access.
  • Observe that before changes, a user can attempt to edit/modify linodes that they don't have access to edit/modify and they will get denied by the API. This is what we're looking to prevent.

Verification steps

(How to verify changes)

  • Verify that restricted users can access only what they have permissions for.
  • Verify that navigating via URL still display correct banners

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

@jaalah-akamai jaalah-akamai added the Restricted User Access Improve UX surrounding restricted access to features label Feb 23, 2024
@jaalah-akamai jaalah-akamai self-assigned this Feb 23, 2024
children: string;
disabled?: boolean;
onClick: () => void;
tooltipText?: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are conditional props to disable the empty state landing page button

resourceType: 'Linodes',
})}
important
variant="warning"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New page level notice

}
footer={
<LinodeEntityDetailFooter
isLinodesReadOnly
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New prop... rest of this file is unchanged


return (
<>
{disabled && <LinodePermissionsError />}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer need notice here - it's at page level (same with similar deletions)


return (
<>
{readOnly && <LinodePermissionsError />}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed notice and fragment - otherwise unchanged - no additions in this file

>
<form onSubmit={formik.handleSubmit}>
{unauthorized && <LinodePermissionsError />}
{isLinodesReadOnly && <LinodePermissionsError />}
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 a drawer, so we're keeping the error notice.


const isLinodesReadOnly =
Boolean(profile.data?.restricted) &&
!grants.data?.global?.['add_linodes'];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Class component 😢 Otherwise I'd use our hook.

);

return linodeGrants ? linodeGrants.permissions : 'read_write';
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗑️ 🧹

@jaalah-akamai jaalah-akamai marked this pull request as ready for review February 26, 2024 22:34
@jaalah-akamai jaalah-akamai requested a review from a team as a code owner February 26, 2024 22:34
@jaalah-akamai jaalah-akamai requested review from hana-akamai and jdamore-linode and removed request for a team February 26, 2024 22:34
@mjac0bs mjac0bs self-requested a review February 27, 2024 17:46
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.

Most everything looks to be disabled correctly.

I'm seeing the unit test failures on ImageAndPassword.test.tsx and I'm a bit confused why they're happening. It seems like that component is only used in the Create Disk Drawer, which isn't accessible to a restricted user because the Add a Disk button is disabled and you can't reach it by URL. It's complaining that it's getting the wrong error message.

Screenshot 2024-02-28 at 10 38 20 AM

@jaalah-akamai
Copy link
Contributor Author

jaalah-akamai commented Feb 29, 2024

Screenshot 2024-02-29 at 1 30 41 PM
Screenshot 2024-02-29 at 1 31 38 PM
Other tag-related UX changes are being tackled by @hkhalil-akamai

@github-actions
Copy link

github-actions bot commented Feb 29, 2024

Coverage Report:
Base Coverage: 81.38%
Current Coverage: 81.39%

@mjac0bs mjac0bs self-requested a review March 1, 2024 16:23
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.

Thanks for doing this, I know disabling everything can be tedious. Tests look good now; the one CI e2e failure looks unrelated on resize-volume.spec, which passes locally. Approving pending a couple minor things are addressed.

Here are the last things I found:

  • Creating a linode from a backup allows you to select a linode and backup. The similiar clone flow disables the selections for the radio button and card.

Screenshot 2024-03-01 at 9 45 02 AM

  • In the Create flow, Firewall selection dropdown is not disabled, which differs from other selections in the form (e.g. VPC).

Screenshot 2024-02-28 at 10 13 21 AM

  • When a user has read/write privileges on Volumes but not linodes, they can attempt to attach or detach a linode from the volumes landing action menu and receive the following errors. Do we also want to disable those action menu items for those actions tied to another resource that is read only?
    Screenshot 2024-03-01 at 9 49 35 AM

Screenshot 2024-03-01 at 9 55 02 AM

isLoading: userTagsLoading,
} = useTagSuggestions(!profile?.restricted);

const isLinodesReadOnly = useIsResourceRestricted({
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we consider naming this const/prop isLinodesGrantReadOnly throughout all of these files to make it more clear that that's what we're also checking to disable everything?

@mjac0bs mjac0bs added the Add'tl Approval Needed Waiting on another approval! label Mar 1, 2024
@jaalah-akamai
Copy link
Contributor Author

I just have some tests to fix up, but thanks for those catches on this complex flow!

Screenshot 2024-03-01 at 2 57 13 PM
Screenshot 2024-03-01 at 3 03 51 PM
Screenshot 2024-03-01 at 3 23 39 PM

@jaalah-akamai
Copy link
Contributor Author

jaalah-akamai commented Mar 1, 2024

If you cannot add Linodes, it doesn't matter if you own the resource in this case, we'll just disable everything.
Screenshot 2024-03-01 at 3 56 18 PM
Screenshot 2024-03-01 at 4 00 19 PM

Copy link
Contributor

@hana-akamai hana-akamai left a comment

Choose a reason for hiding this comment

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

It seems that tags are incorrectly disabled for unrestricted accounts and r/w Linodes

image

@hana-akamai hana-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! Ready for Review labels Mar 7, 2024
@jaalah-akamai jaalah-akamai merged commit 491c106 into linode:develop Mar 7, 2024
vrajesh73 added a commit to vrajesh73/manager that referenced this pull request Mar 12, 2024
…eature/namespace-create

* 'develop' of https://github.com/vrajesh73/manager: (89 commits)
  fix: [M3-7269] - Display parent email in user menu when no company name is available for restricted parent user (linode#10248)
  fix: [M3-7817] - Show correct status of Child Account Enabled column for parent users (linode#10233)
  upcoming: [M3-7616] - Add Placement Groups Events and Notifications (linode#10221)
  upcoming: [M3-7816-v2] - Adjust logic for when to show Switch Account button (linode#10266)
  fix: [M3-7831] - Persisting error messages in ACLB delete dialogs (linode#10254)
  upcoming: [M3-7842] - Update Assign Linode Drawer and improve query skipping (linode#10263)
  upcoming: [M3-7704] - Disable Cloning, Private IP, Backups for edge regions (linode#10222)
  test: Fix test flake for Images landing page test (linode#10267)
  fix: [M3-7824] - ACLB TCP Rule Creation and other fixes (linode#10264)
  refactor: [M3-7687] - Linodes Restricted User Experience 2/2 (linode#10227)
  test: Resolve OBJ create and delete E2E test flake (linode#10245)
  upcoming: [M3-7723] - Placement Group feature flag as object (linode#10256)
  chore(deps): Bump sanitize-html from 2.11.0 to 2.12.1 (linode#10247)
  change: [M3-7813] - Allow the disabling of the TypeToConfirm input (linode#10251)
  upcoming: [M3-7839] - Change Business Partner to Parent User (linode#10259)
  upcoming: [M3-7835] - Adjust user table column count (linode#10252)
  upcoming: [M3 7738] - Update Placement Group Create & Edit Drawers (linode#10205)
  refactor: [M3-7437] - Use `@lukemorales/query-key-factory` for Profile Queries (linode#10241)
  fix: React Query `updateInPaginatedStore` helper function not working as expected (linode#10249)
  test: [M3-7497] - Add tests for child user verification banner (linode#10204)
  ...

# Conflicts:
#	packages/manager/src/MainContent.tsx
#	packages/manager/src/dev-tools/FeatureFlagTool.tsx
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! Restricted User Access Improve UX surrounding restricted access to features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants