-
Notifications
You must be signed in to change notification settings - Fork 217
chore(script): Add Pocket oauth access token pruning script #19536
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
Conversation
| const clientIdBuffer = Buffer.from(clientId, 'hex'); | ||
|
|
||
| const countResult = await db('tokens') | ||
| .where('clientId', clientIdBuffer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a generic access token pruning script that may also work?
That script handles expiration, but in this case we are deleting all access tokens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, seems I potentially could have repurposed that one a bit instead. That one's deleting expired authorization codes while this one deletes the access tokens for specific client ids, but they hook into the same db. I guess since this one's already written I'll leave it but that's good to know.
vbudhram
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks right, gonna give it a run locally
f2e1d08 to
18aecfb
Compare
| nonPocketClientId = buf(randomString(8)); | ||
| userId = buf(randomString(16)); | ||
|
|
||
| // Pocket clients may already exist in CI, so wrap in try/catch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still squinting at this - without these checks all the tests pass locally for me but in CI, I get an ER_DUP_ENTRY, so this seemed fine... everything is passing now locally and in CI but now I'm wondering if I'm deleting something in this test DB and then not restoring it at the end heh, I could do a check and add it back at the end 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I modified this a bit to restore the original clients at the end of the test. Let's see if it passes too.
Because: * We've been storing long-lived access tokens for Pocket in our oauth db as a work around to not use refresh tokens, and these should be deleted This commit: * Adds a script intended for a one-time run once Pocket removal code has landed in production, to delete these tokens closes FXA-12490
Because:
This commit:
closes FXA-12490
Testing this locally
See this, where we've got special casing for Pocket:
} else if (POCKET_IDS.includes(hex(vals.clientId))) { ..., which will be removed in that linked PR.If that PR has been merged already then re-add the else/if and change it to this, or if it hasn't been merged then simply update the line to the 123done client ID:
After running
yarn mysql, you canUSE fxa_oauthandselect * from tokens;to see sign-ins from 123done add tokens to that table. If you revert the changes and sign in normally you won't see a new row added to the table.Next, change the POCKET_CLIENT_IDS array in the script to:
Then run the script with
yarn workspace fxa-auth-server prune-pocket-access-tokensto see the dry-run output in the console.yarn workspace fxa-auth-server prune-pocket-access-tokens --dry-run=falseruns it for real. (You might need to comment out,throw new Error(Config '${key}' must be set in production))