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-7961] - Disable usePersonAccessTokensQuery in token revocation hook based on user_type #10358

Conversation

mjac0bs
Copy link
Contributor

@mjac0bs mjac0bs commented Apr 8, 2024

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
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

Copy link
Contributor Author

@mjac0bs mjac0bs Apr 8, 2024

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
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
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
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.

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
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