-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Enhances authentication token handling with metadata #4896
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
base: main
Are you sure you want to change the base?
Conversation
Introduces `TokenInfo` and `TokenWithInfo` types to encapsulate authentication access tokens along with their relevant metadata, such as provider ID, cloud status, scopes, and expiration. This refactoring replaces direct `accessToken` strings with the new `TokenWithInfo` object across various API calls within integration providers. Benefits include: - Provides richer context in `AuthenticationError` messages for improved debugging. - Increases type safety and clarity by explicitly passing token metadata. (#4880, #4896)
c12af76 to
65aeae8
Compare
🤖 Augment PR SummarySummary: This PR refactors integration authentication to carry structured token metadata alongside access tokens, improving diagnostics and type-safety. Changes:
Technical Notes: Token metadata includes a short MD5-based micro-hash intended for log correlation without exposing the full token. 🤖 Was this summary useful? React with 👍 or 👎 |
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.
src/errors.ts
Outdated
| const typeStrPiece = type ? ` ${type}` : ''; | ||
| const expiresAtPiece = expiresAt ? `expires at ${expiresAt.toISOString()}` : ''; | ||
| const scopesStrPiece = scopes ? `[${scopes.join(',')}]` : ''; | ||
| const authInfo = `(token info: ${[cloudStrPiece, typeStrPiece, expiresAtPiece, scopesStrPiece].join(', ')})`; |
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.
In AuthenticationError, authInfo joins a fixed array that can include empty strings (e.g., missing type/expiresAt), which can yield confusing output like cloud, , , [] and reduces the value of the added diagnostics. Consider omitting empty segments (and avoiding the leading space in typeStrPiece) so the metadata is consistently readable.
🤖 Was this useful? React with 👍 or 👎
| ); | ||
| const token = tokenWithInfo.accessToken; | ||
|
|
||
| return ( |
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.
Unlike getCurrentUser/getCurrentUserForResource, getCurrentUserForInstance doesn’t wrap provider exceptions via handleProviderError, so 401/403s may bypass the new AuthenticationError(TokenInfo, ...) path and lose the extra metadata. Consider handling errors consistently here so callers get normalized auth/rate-limit errors.
🤖 Was this useful? React with 👍 or 👎
Introduces `TokenInfo` and `TokenWithInfo` types to encapsulate authentication access tokens along with their relevant metadata, such as provider ID, cloud status, scopes, and expiration. This refactoring replaces direct `accessToken` strings with the new `TokenWithInfo` object across various API calls within integration providers. Benefits include: - Provides richer context in `AuthenticationError` messages for improved debugging. - Increases type safety and clarity by explicitly passing token metadata. (#4880, #4896)
65aeae8 to
983fab9
Compare
Introduces `TokenInfo` and `TokenWithInfo` types to encapsulate authentication access tokens along with their relevant metadata, such as provider ID, cloud status, scopes, and expiration. This refactoring replaces direct `accessToken` strings with the new `TokenWithInfo` object across various API calls within integration providers. Benefits include: - Provides richer context in `AuthenticationError` messages for improved debugging. - Increases type safety and clarity by explicitly passing token metadata. (#4880, #4896)
983fab9 to
8646077
Compare
Introduces `TokenInfo` and `TokenWithInfo` types to encapsulate authentication access tokens along with their relevant metadata, such as provider ID, cloud status, scopes, and expiration. This refactoring replaces direct `accessToken` strings with the new `TokenWithInfo` object across various API calls within integration providers. Benefits include: - Provides richer context in `AuthenticationError` messages for improved debugging. - Increases type safety and clarity by explicitly passing token metadata. (#4880, #4896)
8646077 to
b08cdc0
Compare
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.
| export function toTokenInfo<T extends IntegrationIds | 'gitkraken'>( | ||
| providerId: T, | ||
| accessToken: string | undefined, | ||
| info: { cloud: boolean; scopes?: readonly string[]; expiresAt?: Date }, |
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.
TokenInfo includes type, but toTokenInfo (and ProviderAuthenticationSession) never populates it, so AuthenticationError “token details” will always omit the auth type.
Consider plumbing CloudIntegrationAuthenticationSession.type through to ProviderAuthenticationSession and including it in toTokenInfo so the new diagnostics are consistent.
🤖 Was this useful? React with 👍 or 👎
Description
Implements #4880
Introduces
TokenInfoandTokenWithInfotypes to encapsulate authentication access tokens along with their relevant metadata, such as provider ID, cloud status, scopes, and expiration.This refactoring replaces direct
accessTokenstrings with the newTokenWithInfoobject across various API calls within integration providers. Benefits include:AuthenticationErrormessages for improved debugging.Implementation Details
There was an option to catch AuthenticationError upper in the call-stack and enrich it with the session metadata. By doing this I could reduce amount of changes. But it's hard identify all the possible points where it should be done, because a thrown exception does not highlight its type: such a place where AuthenticationError caught and logged can be missed, it can be caught earlier before it reaches your point, new points can appear later.
By requiring AuthenticationError to receive all the meta information everything is checked at compile time. Yes, we have to do massive changes once, but then it's always correct and checked by the compiler.
Checklist
Fixes $XXX -orCloses #XXX -prefix to auto-close the issue that your PR addresses