-
Notifications
You must be signed in to change notification settings - Fork 220
feat(auth-server): Add new attached_oauth_clients endpoint #19941
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
| * Gets a unique list of refresh tokens for a given user. | ||
| * Groups by clientId, returning only the most recently used token for each client. | ||
| */ | ||
| const QUERY_LIST_UNIQUE_REFRESH_TOKENS_BY_UID = |
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.
I went this route to offload the compute of uniqueness and limiting to only the most recent lastUsedAt to mysql. It's something that mysql should be good at handling - though I'm open to suggestions if it's better to hoist this into the service layer so we're not making a new query where we don't need. If we go that route I can re-use the existing call, and then filter and constrain in the service
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.
I'd prefer this approach. Let the database do its thing.
| * @param {*} getTokens | ||
| * @returns | ||
| */ | ||
| async _processRefreshTokens(uid, getTokens) { |
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.
Getting all and unique refreshTokens should be processed the same (compared against refreshTokens in redis).
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.
The logic of these new functions should be identical to what list was doing before, but with a new listUnique I didn't want to functions that are just copy/paste of eachother. This just splits it out to be a bit easier to read and used by the old list and new listUnique
| return tokens; | ||
| } | ||
|
|
||
| async getRefreshTokensByUid(uid) { |
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.
I like that you added doc to function above. Do this here too? And on the next method.
Because: - Some partners that use the existing attached_clients enpoint don't need as much data as it currently provides - And the existing endpoint can be slow depending on other data being fetched This Commit: - Adds a new attached_oauth_clients endpoint - Adds support for a new query to handle the unique clientId constraint at the db level - Adds tests Closes: FXA-12616
Because:
This Commit:
Closes: FXA-12616
Checklist
Put an
xin the boxes that applyScreenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
This adds a new endpoint at
account/attached_oauth_clients. No parameters are needed since we use the uid off of the sessiontoken.It will return a unique list of authorized oauth clients, or empty if there are none. If there are multiple refreshTokens for the same clientId, then we prioritize the most recent
lastUsedAt. Responses are simple and look like this:[ { clientId: "3c49430b43dfba77", lastAccessTime: 1769311751000 }, { /** more unique clients...*/ } ] // or, for no authorized clients []