-
Notifications
You must be signed in to change notification settings - Fork 0
feat: FGI-1573 add MRRT support #58
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
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…/auth0-server-python into FGI-1573_connected-account-support
…ith one matching the audience Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…/auth0-server-python into FGI-1573_connected-account-support
Co-authored-by: Adam Mcgrath <adamjmcgrath@gmail.com>
Co-authored-by: Adam Mcgrath <adamjmcgrath@gmail.com>
Co-authored-by: Adam Mcgrath <adamjmcgrath@gmail.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…-python into FGI-1573_mrrt_support
This comment has been minimized.
This comment has been minimized.
|
Claude finished @sam-muncke's task —— View job Code Review Complete
Overall Assessment❌ This PR requires fixes before merging. While the MRRT feature implementation is well-structured with comprehensive test coverage, there are several critical issues related to audience handling consistency, edge case validation, and a syntax error in documentation that must be addressed. Key Issues:
Strengths:
Job Run | Branch: |
…mum number of scopes rather than first match
| if merged_scope: | ||
| get_refresh_token_options["scope"] = merged_scope | ||
|
|
||
| token_endpoint_response = await self.get_token_by_refresh_token(get_refresh_token_options) |
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.
When multiple coroutines concurrently request a token for the same (audience, scope) that isn't cached, they all execute this line simultaneously, making redundant refresh token requests to Auth0.
We can build something referring to this Next.js Implementation
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.
This is the case regardless of whether MRRT is being used or not. Prior to this PR, the same issue exists. I think adding a caching layer to this SDK is beyond the scope of this implementation. Also even if we were to add this, it would likely best be as a framework specific implementation rather than the generic python lib. How effective a local memory request cache would be would be very dependent on what framework/infrastructure was using the library. Arguably this would best be handled as a caching StateStore implementation, given that as soon as we receive the token, we update the StateStore and return. Adding an additional abstraction for request cache seems unnecessary.
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 understand the scope concern, but I want to clarify the issue isn't about "caching" rather it's about request deduplication for in-flight requests.
StateStore caching helps with sequential requests (request 1 stores token -> request 2 reads from cache).
But for concurrent requests, all 5 coroutines check StateStore at the same time before any can write. This results in unnecessary duplicate API calls.
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 understand the point you are trying to make but the value this adds beyond a just a stateful StateStore is questionable and highly dependent on the framework using this SDK (and more practically the infrastructure setup of the specific application is being run on).
Regardless in as much as this is an issue, its an issue that exists independently of MRRT. Its no more or less of a problem before or after this PR. If its an optimisation that you think is worth while, I suggest it is done a standalone improvement to the SDK in a future piece of work.
| get_refresh_token_options["scope"] = merged_scope | ||
|
|
||
| token_endpoint_response = await self.get_token_by_refresh_token(get_refresh_token_options) | ||
|
|
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.
When requesting an access token for an audience not in the MRRT policy, the SDK returns a cached token for a different audience without raising an error or warning.
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.
This is the behaviour of MRRT and consistent with all the other SDKs. This concern has been raised with the Sessions team and they address this by returning the requested resource in the the response for oauth/token in order for consumers to validate it in the future but as of right no, that does not exist.
|
|
||
| token_endpoint_response = await self.get_token_by_refresh_token(get_refresh_token_options) | ||
|
|
||
| # Update state data with new token |
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 method returns or constructs a response that claims audience: X but the actual JWT has aud: Y
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.
Is this a reference to when MRRT is not configured or another scenario. If it is - see my other comments.
| if merged_scope: | ||
| get_refresh_token_options["scope"] = merged_scope | ||
|
|
||
| token_endpoint_response = await self.get_token_by_refresh_token(get_refresh_token_options) |
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.
When Auth0 rejects the audience, the SDK may be using DEFAULT_AUDIENCE_STATE_KEY without informing the caller.
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'm not sure what you mean by "rejects the audience" since Auth0 will not reject any audience you pass to oauth/token for the RT exchange - it will either use it and return the correct AT or ignore it and return an AT for the original audience used to obtain the RT. As mentioned above, I agree with you that this second behaviour is not ideal but that is the current behaviour of the MRRT feature, there is some complexity over this and it has been raised with the Sessions team. I discussed the possibility of erroring in the SDK with @frederikprijck and he advised not to error in this case and the handling here is consistent with the other SDKs.
As for the use of DEFAULT_AUDIENCE_STATE_KEY, this is only used when no audience is specified in the default auth params or the request auth params. This actually resolves an issue whereby default was always used as the state key due to the fact that the previous code was attempting to pull the audience from the /oauth/token response but would never find it because that endpoint doesnt return it. This was sort-of fine for single audience applications now we're introducing more.
Changes
Introduces ability to use Multi-resource Refresh Tokens (MRRT) to obtain access tokens from multiple audiences using a single refresh token.
audienceandscopeparameters toget_access_token()methodServerClientconfigurationReferences
https://auth0team.atlassian.net/browse/FGI-1573
Testing
See included docs for testing instructions.
Checklist