-
Notifications
You must be signed in to change notification settings - Fork 288
Add support for connected accounts #912
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
af7a728 to
caffd4d
Compare
caffd4d to
e3b852c
Compare
| {}, | ||
| document.title, | ||
| appState?.returnTo ?? window.location.pathname | ||
| appState!.returnTo ?? window.location.pathname |
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.
There is always an appState now (with response_type) - but if I make it a required argument for defaultOnRedirectCallback it may cause TS compilation errors for people doing https://github.com/auth0/auth0-react/blob/main/examples/cra-react-router/src/index.tsx#L17
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 importing the actual Auth0Client into the tests (to get ResponseType) so we need the same as https://github.com/auth0/auth0-spa-js/blob/main/jest.environment.js#L17-L18
package.json
Outdated
| }, | ||
| "dependencies": { | ||
| "@auth0/auth0-spa-js": "^2.5.0" | ||
| "@auth0/auth0-spa-js": "file:auth0-auth0-spa-js-2.6.0-connected-accounts.tgz" |
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 will update this PR when auth0/auth0-spa-js#1422 gets shipped
|
Claude finished @gyaneshgouraw-okta's task —— View job Review Complete
Overall Assessment❌ This PR requires attention to several type safety issues and edge case handling before merging. The core implementation is sound, but there are concerns around nullable handling, type casting without validation, and potential breaking changes to the Key Issues:
|
| {}, | ||
| document.title, | ||
| appState?.returnTo ?? window.location.pathname | ||
| appState!.returnTo ?? window.location.pathname |
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 non-null assertion operator (!) is used here, but appState could still be undefined if handleRedirectCallback returns { appState: undefined }. This could cause runtime errors.
Consider using optional chaining instead:
appState?.returnTo ?? window.location.pathnameThis also addresses the author's concern about making appState required breaking existing 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.
{ appState: undefined }
This is not possible, because appState is always an object with at least response_type. I casted it here because I didn't want to write a test case for some unreachable code
| let user: TUser | undefined; | ||
| if (hasAuthParams() && !skipRedirectCallback) { | ||
| const { appState } = await client.handleRedirectCallback(); | ||
| const { appState = {}, response_type, ...result } = await client.handleRedirectCallback(); |
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.
Type safety issue: The destructuring assumes response_type exists, but if it's missing from the response, appState.response_type will be set to undefined. This could lead to unexpected behavior if code assumes this property always has a value.
Consider adding validation or handling the case where response_type might be missing from the response.
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 [email protected] response_type will always have a value
| user = await client.getUser(); | ||
| appState.response_type = response_type; | ||
| if (response_type === ResponseType.ConnectCode) { | ||
| appState.connectedAccount = result as ConnectedAccount; |
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.
Potential type safety issue: The cast result as ConnectedAccount assumes all remaining properties after destructuring match the ConnectedAccount type. If the response contains unexpected properties or is missing required properties, this could cause runtime issues.
Consider validating that result contains the expected properties (id, connection, access_type, created_at, expires_at) before casting, or using a type guard function.
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.
There's no benefit in runtime checks to confirm the my account api is returning what we expect, it would just make the client more brittle
| onRedirectCallback, | ||
| }); | ||
| renderHook(() => useContext(Auth0Context), { | ||
| wrapper, |
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.
Inconsistent test expectations: This test now expects response_type to be merged into the appState object, but this changes the API surface. Existing consumers of onRedirectCallback will receive a different object shape than before (with an additional response_type property).
While this might be intentional for the new feature, it's a breaking change that should be documented in the migration guide or release notes.
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.
Adding a new field to the return object of an api is not a breaking change, customers will only need to change their code if they're trying to implement a function that behaves the same as handleRedirectCallback - as we are doing here with the mock. (they may have to change their mocks if they're doing the same, but I don't believe this qualifies as a breaking change)
| expires_at: '2024-01-02T00:00:00.000Z', | ||
| } | ||
| clientMock.getUser.mockResolvedValue(user); | ||
| clientMock.handleRedirectCallback.mockResolvedValue({ |
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.
Missing test coverage: There's no test case for when response_type is ResponseType.ConnectCode but the result object is malformed or missing expected properties. This edge case should be tested to ensure the code handles invalid responses gracefully.
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.
We're not putting in any additional logic to handle this case (see #912 (comment)) - so there's nothing to test
| import { OAuthError } from './errors'; | ||
|
|
||
| const CODE_RE = /[?&]code=[^&]+/; | ||
| const CODE_RE = /[?&](?:connect_)?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.
Regex pattern could match unintended parameters: The pattern (?:connect_)?code will match both code and connect_code, but it will also match any parameter starting with code (e.g., code_verifier, code_challenge).
Consider making the pattern more specific with word boundaries:
const CODE_RE = /[?&](?:connect_)?code(?:=|&|$)/;Or more explicitly:
const CODE_RE = /[?&](?:connect_code|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.
**Added** - Add support for connected accounts [\#912](#912) ([adamjmcgrath](https://github.com/adamjmcgrath))

Changes
Add support for Connected Accounts to SPA JS:
connectAccountWithRedirectmethod that redirects the application to the/connectendpoint on the auth server (similar mechanics to the logging in with the/authorizeendpoint)References
https://auth0team.atlassian.net/browse/AGAI-157
Testing
Manual testing steps and demo video are in ESD-52475
(integration tests are not possible since oidc-provider does not support the proprietary Auth0 connect flow)
Checklist