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

fix: [M3-7796] - Correct Default VPC Scope and Adjust PAT Creation Behavior #10226

Conversation

mjac0bs
Copy link
Contributor

@mjac0bs mjac0bs commented Feb 23, 2024

Description πŸ“

There is an issue with the current Personal Access Token (PAT) creation process. When using the "Select All" option, it incorrectly selects the VPC scope even when it is disabled. This behavior could lead to unintentional granting of access and needs to be corrected to improve the usability and security of the platform.

Changes πŸ”„

  • Adds a check and early return in handleSelectAllScopes function to explicitly handle the VPC case where Read Only is checked, which should instead be None for that scope.
  • Modifies allScopesAreTheSame util to optionally pass in excluded scopes, allowing the correct "Select All" radio button to be checked when some scopes have a different default.
  • Adds test coverage to CreateAPITokenDrawer.test.tsx and APIToken utils.test.ts.

Preview πŸ“·

Before After
Screenshot 2024-02-20 at 4 32 11 PM localhost_3000_profile_tokens (4)
Screenshot 2024-02-20 at 4 32 50 PM Screenshot 2024-02-23 at 9 17 41 AM

How to test πŸ§ͺ

Prerequisites

(How to setup test environment)

  • Check out this PR and yarn dev.

Reproduction steps

(How to reproduce the issue, if applicable)

  • Go to https://cloud.linode.com/profile/tokens.
  • Click the "Select All" read only option and observe that the disabled VPC read only radio button is selected.
  • Observe that when the PAT is created, its VPC scope will be Read Only. (This can be seen either in the request body or the View PAT drawer.)

Verification steps

(How to verify changes)

  • Go to https://localhost:3000/profile/tokens.
  • Click the "Select All" read only option and verify that radio button is checked even though not all the scopes in that column are read only.
  • Verify that the disabled VPC read only radio button is not selected. Instead, None remains selected.
  • Verify that when the PAT is created, its VPC scope will be None. This can be seen either in the request body (the VPC scope will not be included) or the View PAT drawer.

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 added the VPC Relating to VPC project label Feb 23, 2024
@mjac0bs mjac0bs self-assigned this Feb 23, 2024
@mjac0bs mjac0bs added the Bug Fixes for regressions or bugs label Feb 23, 2024
@mjac0bs mjac0bs marked this pull request as ready for review February 23, 2024 18:09
@mjac0bs mjac0bs requested a review from a team as a code owner February 23, 2024 18:09
@mjac0bs mjac0bs requested review from dwiley-akamai and abailly-akamai and removed request for a team February 23, 2024 18:09
@mjac0bs mjac0bs marked this pull request as draft February 23, 2024 18:29
Copy link

github-actions bot commented Feb 23, 2024

Coverage Report: βœ…
Base Coverage: 81.34%
Current Coverage: 81.34%

@mjac0bs mjac0bs marked this pull request as ready for review February 28, 2024 20:56
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice πŸš€

@mjac0bs mjac0bs added the Add'tl Approval Needed Waiting on another approval! label Feb 28, 2024
Comment on lines 175 to 178
if (scope[0] === 'vpc' && value === 1) {
return [scope[0], 0];
}
return [scope[0], value];
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: Not sure exactly how to word this or if it makes any sense for this, but it could be nice if this chunk of code and excludedScopesFromSelectAll could pull from one source of truth array/object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does b15337e make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think so!

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.

Thanks for the fix - confirmed using the testing instructions and verifying payload βœ…

❓ Question

Screenshot 2024-02-29 at 11 47 13

Copy link
Contributor

@hana-linode hana-linode 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 disabled VPC Read Only radio button is not selected when selecting all and defaults to None instead. View Scopes drawer is displaying None as expected βœ…

@hana-linode hana-linode added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Feb 29, 2024
@mjac0bs
Copy link
Contributor Author

mjac0bs commented Feb 29, 2024

@abailly-akamai - I went with yes because otherwise it was an odd experience for the user to click the Select All > Read Only and not see their action reflected in a checked radio button. All are "Read Only" except any that can't be and have other defaults.

@mjac0bs mjac0bs merged commit 04d2fed into linode:develop Feb 29, 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! Bug Fixes for regressions or bugs VPC Relating to VPC project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants