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-7481] - Disable creating and editing API tokens for proxy users #10109

Merged

Conversation

mjac0bs
Copy link
Contributor

@mjac0bs mjac0bs commented Jan 24, 2024

Description 📝

According to the API spec:

Proxy users may not generate additional tokens for themselves.
Proxy users will be able to view the tokens provisioned for them, but will not be able to modify them.
Proxy users should still be able to revoke their own PAT.

Changes 🔄

  • The "Create A Personal Access Token" button is disabled for proxy users.
  • The disabled button has a tooltip that reads: "You can only create API tokens for your own company."
  • Proxy users can view the tokens provisioned for them but cannot modify them.
  • The disabled Rename action has a tooltip that reads: "Only company users can edit API tokens."
  • Added Cypress test coverage for proxy user disabled PAT states.

Preview 📷

Description Screenshot
Disabled Edit action button Screenshot 2024-01-24 at 3 40 29 PM
Disabled Create button Screenshot 2024-01-24 at 3 40 09 PM

How to test 🧪

Prerequisites

(How to setup test environment)

  • Check out this PR.
  • Make sure the Parent/Child feature flag is on and change user_type to proxy in serverHandlers.ts.

Verification steps

(How to verify changes)

  • As a proxy user, go to http://localhost:3000/profile/tokens.
  • Confirm that the Create a Personal Access Token is disabled and hovering over the button displays a tooltip explaining the reason for disabling.
  • Confirm that the edit action (Rename) is disabled and hovering over the help tooltip displays a message explaining the reason for disabling.
  • Confirm that API tokens are editable and creatable for non proxy users (e.g. other user_types; feature flag off).
  • Confirm test passes:
yarn cy:run -s "cypress/e2e/core/account/personal-access-tokens.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 25, 2024 16:34
@mjac0bs mjac0bs requested review from a team as code owners January 25, 2024 16:34
@mjac0bs mjac0bs requested review from jdamore-linode, carrillo-erik and hkhalil-akamai and removed request for a team January 25, 2024 16:34
@mjac0bs mjac0bs requested a review from jaalah-akamai January 25, 2024 16:43
Copy link

github-actions bot commented Jan 25, 2024

Coverage Report:
Base Coverage: 80.33%
Current Coverage: 80.33%

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.

All the verification steps looked good. Thanks for including the test!

@bnussman-akamai bnussman-akamai added the Add'tl Approval Needed Waiting on another approval! label Jan 25, 2024
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.

This is great, thanks for the tests @mjac0bs! This made me realize I missed this flow when I wrote up the test tickets for this project, I appreciate you covering it here!

Comment on lines 275 to 281
cy.findByRole('tooltip')
.should('be.visible')
.within(() => {
cy.findByText(
'You can only create tokens for your own company.'
).should('be.visible');
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
cy.findByRole('tooltip')
.should('be.visible')
.within(() => {
cy.findByText(
'You can only create tokens for your own company.'
).should('be.visible');
});
ui.tooltip
.findByText('You can only create tokens for your own company.')
.should('be.visible');

Just pointing out that this exists for the future!

The helper is a little bit more ergonomic, and has the extra advantage of working from inside within(() => {}) callbacks even though the tooltip itself is often somewhere else in the DOM (not applicable to your situation of course)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, thanks! I appreciate the reminder that these UI helpers exist; made that quick change in 3a35491.

@jdamore-linode jdamore-linode added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Jan 25, 2024
@mjac0bs mjac0bs merged commit f0b774e into linode:develop Jan 25, 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! Parent / Child Account
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants