-
Notifications
You must be signed in to change notification settings - Fork 23
Add oauth_tokens to AuthkitAuthenticationResponse
#378
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
Add oauth_tokens to AuthkitAuthenticationResponse
#378
Conversation
oauth_tokens to AuthenticationResponse
10879ec to
b95dc7b
Compare
Created a new OAuthTokens model to represent OAuth credentials, including provider, access token, refresh token, expiration, and scopes. Integrated this new model into the AuthenticationResponse class, adding an optional field for OAuth tokens.
b95dc7b to
73bdedd
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.
Looks good, just some small feedback.
| access_token: str | ||
| refresh_token: str | ||
| expires_at: int | ||
| scopes: List[str] |
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.
For fields where we don't necessarily need mutability, we've been using the Sequence type instead of List. Example here.
| impersonator: Optional[Impersonator] = None | ||
| organization_id: Optional[str] = None | ||
| user: User | ||
| oauth_tokens: Optional[OAuthTokens] = None |
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.
Will OAuth credentials exist for all authentication methods or just authenticate with code?
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.
Just for authenticate_with_code - so I guess we should split this type apart?
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.
@mattgd I'm wondering if I change the response, would it be considered a breaking change?
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.
Never mind, I found that I had the new field to the AuthKitAuthenticationResponse
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 forgot about that type, nice!
oauth_tokens to AuthenticationResponseoauth_tokens to AuthkitAuthenticationResponse
Description
Created a new
OAuthTokensmodel to represent OAuth credentials, including provider, access token, refresh token, expiration, and scopes. Integrated this new model into theAuthenticationResponseclass, adding an optional field for OAuth tokens.Documentation
Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.
If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.