Skip to content

fix: [M3-7961] - Disable usePersonAccessTokensQuery in token revocation hook based on user_type - #10358

Merged
mjac0bs merged 4 commits into
linode:developfrom
mjac0bs:M3-7961-make-usePersonalAccessTokensQuery-disableable
Apr 9, 2024
Merged

mjac0bs merged 4 commits into
linode:developfrom
mjac0bs:M3-7961-make-usePersonalAccessTokensQuery-disableable

Conversation

@mjac0bs

@mjac0bs mjac0bs commented Apr 8, 2024 •

Copy link
Copy Markdown
Contributor

Description 📝

Disable unnecessary runs of the usePersonalAccessTokensQuery, which was running for all accounts - regardless of user_type - from the UserMenu. This query is used within the usePendingRevocationToken hook, which allows us to revoke PATs for proxy accounts when account switching occurs (see #10313).

Changes 🔄

  • Updates usePersonalAccessTokensQuery to allow it to be enabled/disabled.

Target release date 🗓️

4/15/24

How to test 🧪

Reproduction steps

(How to reproduce the issue, if applicable)

  • Log into https://cloud.linode.com as a regular user and observe that there is an API call to /tokens made when Cloud is loaded, since the user menu will load on any page. The UI doesn't need access to tokens for this user at this point, so the call is unnecessary.

Verification steps

(How to verify changes)

  • Check out this branch and, as the same user, log into http://localhost:3000 and observe that there is NOT an API call made to /tokens when Cloud is loaded, since the user is not of proxy user_type.
  • Then log into a parent user (using our shared parent account credentials - ask if you don't know where to find these) and switch into to a proxy account via the user menu > Switch Account button. Confirm there is an API call to /tokens. Confirm that feat: [M3-7888] - Revoke proxy PAT when switching accounts #10313 works as expected.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is definitely an improvement, but I still have some general concerns with this hook.

  • The hook will cause a GET /v4/profile/tokens whenever at the app loads (on proxy and parent accounts with child_account_acces). Is this necessary? It would be great if we could write this in a way where the fetch is deferred until it is needed.
  • The reliability of the hook might be questionable. We are using usePersonalAccessTokensQuery. Are we confident that the token that needs to be revoked is on the first page of GET /v4/profile/tokens?

cc @jaalah-akamai

@mjac0bs mjac0bs Apr 8, 2024 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@bnussman-akamai I think this query actually only needs to run for proxy users, since those are the tokens we're revoking to keep them from piling up on account switch... and in that case, I think we are confident that the token would be on the first page of GET /v4/profile/tokens, since a proxy user cannot create their own PATs and the API generates one ephemeral PAT for them at a time (which we then revoke before the next switch) when an account switch is made. (Correct me if I'm wrong, @jaalah-akamai.)

It would be great if we could write this in a way where the fetch is deferred until it is needed.

getPendingRevocationToken() is called in handleAccountSwitch() to get the token we'll revoke before switching accounts if the user is a proxy. I'm not sure what we'd pass into the usePendingRevocationToken to further restrict the query from running until an account switch is happening, without capturing something in state. Did you have any thoughts there?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@bnussman-akamai @mjac0bs I did some refactoring over the weekend and we can actually remove this hook. I tossed up my draft this morning where I'm calling usePersonalAccessTokensQuery directly in SwitchAccountDrawer.tsx: https://github.com/linode/manager/pull/10361/files#diff-ef77afc013835684663f6da8ba90d1beda21ccc418965d2195fbec75c7b3bea3R50

The pagination question is still valid... I'll have to think about that.

So with this PR, @mjac0bs I think just the query change is necessary and we'll tackle the rest in the PR above.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, @jaalah-akamai. I see you deleted that hook in your draft PR, so I'll leave it and the change to disable here in case this is merged first.

@mjac0bs
mjac0bs marked this pull request as ready for review April 8, 2024 20:30
@mjac0bs
mjac0bs requested a review from a team as a code owner April 8, 2024 20:30
@mjac0bs
mjac0bs requested review from hana-akamai and jaalah-akamai and removed request for a team April 8, 2024 20:30

@bnussman-akamai bnussman-akamai left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm still not super happy with how this will fetch at all times when the app loads for proxy users. I think moving it into SwitchAccountDrawer and only enabling it when the drawer is open would much better. Approving assuming this is just a temporary solution

@mjac0bs mjac0bs added Approved Multiple approvals and ready to merge! and removed Ready for Review labels Apr 9, 2024
@mjac0bs
mjac0bs merged commit 7044496 into linode:develop Apr 9, 2024
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