[DO NOT MERGE]feature: FGI-1574 add connected account support #67
Conversation
|
|
||
| # Extract the returnTo URL from the appState if available. | ||
| return_to = session_data.get("app_state", {}).get("returnTo") | ||
| return_to = app_state.get("returnTo") |
There was a problem hiding this comment.
Not the fault of this PR, but this SDK should have something like this https://github.com/auth0/nextjs-auth0/blob/main/EXAMPLES.md#oncallback-hook - otherwise the api response gets lost
There was a problem hiding this comment.
Yeh I struggled with what to do after completion. I thought this was the best option for expediency to get something functional for the GA date as there's not actually a lot thats useful in the response. This SDK is pretty bare feature wise though compared to the JS SDK. I can add something like this in in a follow up PR
|
Claude finished @sam-muncke's task —— View job Code Review Complete
Overall Assessment❌ This PR has critical security and correctness issues that must be addressed before merging. The most significant concern is an open redirect vulnerability in the callback handler where the |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/auth0_fastapi/server/routes.py
Outdated
| return Response(status_code=204) | ||
|
|
||
| if config.mount_connected_account_routes: | ||
| @router.get("/auth/connect-account") |
There was a problem hiding this comment.
For symmetry, this should be the same as the nextjs-auth0 implementation:
| @router.get("/auth/connect-account") | |
| @router.get("/auth/connect") |
This makes it easier for the front-end library to behave the same no matter what back-end framework+sdk is used.
There was a problem hiding this comment.
oh, I see that /auth/connect is already used for account linking 😬 I think that the account-linking approach should be discontinued (to be confirmed based on the deprecation plan). Is it intended to keep it as-is for now?
(if that's the case, I might need to override the url used by the front-end... We already have a way to do it, just a bit more complex setup for our customers)
There was a problem hiding this comment.
Yeh would have preferred /auth/connect but its taken. Would love to remove it but its a breaking change so think I had to avoid it. The other option would be to make mount_connect_routes and mount_connected_account_routes mutually exclusive, so the meaning of the /auth/connect route is dependent on which config setting you have enabled.
src/auth0_fastapi/config.py
Outdated
| audience: Optional[str] = Field(None, description="Target audience for tokens (if applicable)") | ||
| authorization_params: Optional[dict[str, Any]] = Field(None, description="Additional parameters to include in the authorization request") | ||
| pushed_authorization_requests: bool = Field(False, description="Whether to use pushed authorization requests") | ||
| use_mrrt: bool = Field(False, description="Whether to use Multi-Resource Refresh Tokens (MRRT)") |
There was a problem hiding this comment.
In the nextjs-auth0 SDK, we did not have to explicitly enable mrrt. Is there a reason why we have an explicit toggle here?
There was a problem hiding this comment.
I took inspiration from auth0-spa-js which does require it to be explicitly enabled https://github.com/auth0/auth0-spa-js/blob/main/src/global.ts#L285C19-L285C20
I'm fine either way - its not hard to remove - just depends which one we want to be consistent with 🤷
| # Route-mounting flags with desired defaults | ||
| mount_routes: bool = Field(True, description="Controls /auth/* routes: login, logout, callback, backchannel-logout") | ||
| mount_connect_routes: bool = Field(False, description="Controls /auth/connect routes (account-linking)") | ||
| mount_connected_account_routes: bool = Field(False, description="Controls /auth/connect-account routes (for connected accounts)") |
There was a problem hiding this comment.
just pointing out that this config is named enableConnectAccountEndpoint in nextjs-auth0. Any way to make them more similar?
One tiny nitpick here is that there's only 1 route, but we use the routes plural.
There was a problem hiding this comment.
Happy to but that makes in more different from the other mount_routes and mount_connect_routes in this SDK
As for routes vs route this does also extend the /auth/callback route functionality as well as adding the new route
… mutually exclusive with legacy connect behaviour
sam-muncke
changed the title
Changes
This change adds support for the new Connected Accounts flow.
--
use_mrrt- which indicates whether to use Multi-Resource Refresh Tokens (required for connected accounts)--
mount_connected_account_routes- Adds the/auth/connect-accountroute and allows/auth/callbackto handle the connnected account callback/auth/connect-accountroute to initiate connected account flow/auth/callbackto handle connected account callback to complete the flow if theconnect_codeparam is presentNote: This change depends on the functionality in [this PR]:auth0/auth0-server-python#57 and requires a version bump to
auth0-server-pythonlib.References
Testing
Please describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. If this library has unit and/or integration testing, tests should be added for new functionality and existing tests should complete without errors.
Checklist