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-7967] - Returning proper scope when selecting all perms #10359

Merged
merged 3 commits into from
Apr 8, 2024

Conversation

jaalah-akamai
Copy link
Contributor

Description 📝

The Cloud Manager isn't working right when making tokens that should have access to everything. Even when you choose 'all' for the token's access level, it's mistakenly giving limited access (even though the scopes are all included). Also, when making tokens using the API and setting access to * (which should mean everything), it should reply with * to show that. But, it's listing out each access area one by one instead of just using *, which is not what's supposed to happen.

Changes 🔄

  • No longer need to filter out child account access since API is in prod
  • We're still only visually showing the "child account access" perm for accounts that have access to it from a user experience point of view (even though the perm will be part of the token). This is ok since features are still blocked from a grants perspective.

Target release date 🗓️

4/8-9/2024 (Hotfix)

Preview 📷

Before:

{
  "id": 123,
  "scopes": "account:read_write child_account:read_write databases:read_write domains:read_write events:read_write firewall:read_write images:read_write ips:read_write linodes:read_write lke:read_write longview:read_write nodebalancers:read_write object_storage:read_write stackscripts:read_write vpc:read_write volumes: read_write",
  "created": "2024-04-08T13:26:24",
  "label": "all-scopes-but-not-star",
  "token": "abcd",
  "expiry": "2024-10-08T00:00:00"
}

After:

{
  "id": 123,
  "scopes": "*",
  "created": "2024-04-08T13:26:24",
  "label": "this-has-star-indicating-all-scopes",
  "token": "abcd",
  "expiry": "2024-10-08T00:00:00"
}

How to test 🧪

Prerequisites

  • Log into normal development account
  • Log into Parent account using developer credentials
    • Test as Parent User - Unrestricted Access
    • Test as Parent User - Restricted Access with no child account access grant

Reproduction steps

  • Currently, selected "All" for any new PAT in any account will generate an enumerated scope list, not scopes: "*"

Verification steps

  • In a normal development account (non-parent/child), create a PAT with all scopes, and observe response has scopes: "*"
    • Further, observe that child account access is NOT visible
  • In a parent user account (unrestricted access), create a PAT with all scopes, and observe response has scopes: "*"
    • Further, observe that child account access is visible.
  • In a parent user account (restricted access with no child account access grant), create a PAT with all scopes, and observe response has scopes: "*"
    • Further, observe that child account access is NOT visible.

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 Hotfix Hotfix: This is going to staging Parent / Child Account labels Apr 8, 2024
@jaalah-akamai jaalah-akamai self-assigned this Apr 8, 2024
@jaalah-akamai jaalah-akamai requested a review from a team as a code owner April 8, 2024 14:39
@jaalah-akamai jaalah-akamai requested review from mjac0bs and abailly-akamai and removed request for a team April 8, 2024 14:39
@jaalah-akamai jaalah-akamai added the Bug Fixes for regressions or bugs label Apr 8, 2024
}

const scopeIsForVPC = scopeTup[0] === 'vpc';
{allPermissions.map((scopeTup) => {
Copy link
Contributor

@abailly-akamai abailly-akamai Apr 8, 2024

Choose a reason for hiding this comment

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

scopeTup - was wondering what it meant at first EDIT: This is whole thing is a bit hard to read because of naming conventions. naming it a tuple is ok-ish wondering if we can come up with something better.

also:
const something = renamedScopeTup[0]
const somethingElse = renamedScopeTup[1]

or similar to improve readability?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe it means "scope tuple", but agreed that this section of the code base isn't the most readable

Copy link
Contributor

Choose a reason for hiding this comment

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

could be a follow up since this is a hotfix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted for followup ✏️

(scopeTup) => basePermNameMap[scopeTup[0]] !== 'Child Account Access'
);
isChildAccountAccessRestricted ||
!flags.parentChildAccountAccess;
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link

github-actions bot commented Apr 8, 2024

Coverage Report:
Base Coverage: 81.7%
Current Coverage: 81.7%

abailly-akamai
abailly-akamai previously approved these changes Apr 8, 2024
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.

Confirmed the steps outlined in the PR description and got the proper answer (I think) when using a parent account

Screenshot 2024-04-08 at 11 23 16

bnussman-akamai
bnussman-akamai previously approved these changes Apr 8, 2024
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.

Tested on my normal personal account, a parent account, and a restricted parent account. Everything worked as expected! ✅

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.

✅ In a normal development account (non-parent/child), create a PAT with all scopes, and observed response has scopes: "*"
✅ Observed that child account access is NOT visible

✅ In a parent user account (unrestricted access), create a PAT with all scopes, and observed response has scopes: "*"
✅ Observe that child account access is visible.

✅ In a parent user account (restricted access with no child account access grant), create a PAT with all scopes, and observe response has scopes: "*"
✅ Observe that child account access is NOT visible.

Confirmed that child_account_access is showing up in the View Account Access drawer as expected, as well.

@jaalah-akamai jaalah-akamai merged commit a5dc116 into linode:staging Apr 8, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fixes for regressions or bugs Hotfix Hotfix: This is going to staging Parent / Child Account
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants