-
Notifications
You must be signed in to change notification settings - Fork 233
feat: add support for Native to Web SSO #1386
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: master
Are you sure you want to change the base?
Conversation
…into feat/native-to-web-sso
|
Claude finished @pmathew92's task —— View job Code Review Complete
Overall Assessment❌ This pull request requires fixes before it can be merged. A critical compilation error was identified in the Android implementation that will prevent the code from building, along with several type safety issues that could cause runtime failures. Critical Issues:
Additional Issues:
The TypeScript/React implementation, iOS native implementation, and test coverage are well-structured. Once the critical Android issues are resolved, this will be a solid addition to the library. |
| val headerMap = headers?.toHashMap() ?: emptyMap() | ||
|
|
||
| secureCredentialsManager.getSSOCredentials( | ||
| params as Map<String, String>, |
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: Unsafe type casting without validation. The cast as Map<String, String> will fail at runtime if the HashMap contains non-String values.
The parameters and headers may contain non-String values (numbers, booleans, nested objects) that would cause a ClassCastException.
| params as Map<String, String>, | |
| val params = parameters?.toHashMap()?.mapValues { it.value?.toString() ?: "" } ?: emptyMap() | |
| val headerMap = headers?.toHashMap()?.mapValues { it.value?.toString() ?: "" } ?: emptyMap() | |
| secureCredentialsManager.getSSOCredentials( | |
| params, | |
| headerMap, |
| } | ||
|
|
||
| override fun onFailure(error: CredentialsManagerException) { | ||
| handleCredentialsManagerError(error, promise) |
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.
Error Handling Issue: The method handleCredentialsManagerError is called but doesn't exist in the codebase. This will cause a compilation error.
Looking at line 252, the correct method name is deduceErrorCode, which should be used along with manual error rejection:
| handleCredentialsManagerError(error, promise) | |
| override fun onFailure(error: CredentialsManagerException) { | |
| val errorCode = deduceErrorCode(error) | |
| promise.reject(errorCode, error.message, error) | |
| } |
| } | ||
|
|
||
| async getSSOCredentials( | ||
| _parameters?: Record<string, any>, |
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.
Unused Parameters: The parameters are prefixed with underscore indicating they're intentionally unused, but they're still defined in the function signature. Consider using rest parameters or omitting the names entirely for cleaner code:
| _parameters?: Record<string, any>, | |
| async getSSOCredentials(): Promise<SessionTransferCredentials> { |
| * that can be used to authenticate in web contexts without requiring the user | ||
| * to log in again. The session transfer token can be passed as a cookie or | ||
| * query parameter to the `/authorize` endpoint to establish a web session. | ||
| * |
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 parameters parameter allows Record<string, any> which is overly permissive and bypasses type safety. Consider defining a more specific type for parameters to prevent runtime errors:
parameters?: Record<string, string | number | boolean>| const handleGetSSOCredentials = async () => { | ||
| try { | ||
| const credentials = await getSSOCredentials(); | ||
| setSSOCredentials(credentials); |
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.
Test Implementation Issue: Empty catch block silently swallows errors. The test should explicitly handle or verify error behavior:
| setSSOCredentials(credentials); | |
| try { | |
| const credentials = await getSSOCredentials(); | |
| setSSOCredentials(credentials); | |
| } catch (error) { | |
| // Error will be dispatched to state, no action needed | |
| } |
| if (error) { | ||
| return <Text testID="error">Error: {error.message}</Text>; | ||
| } | ||
|
|
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.
Test Implementation Issue: Empty catch block silently swallows errors. This pattern appears multiple times in the test file and should include at least a comment explaining why the error is ignored:
| getSSOCredentials(parameters).catch(() => { | |
| // Error will be dispatched to state | |
| }); |
| async getSSOCredentials( | ||
| parameters?: Record<string, any>, | ||
| headers?: Record<string, string> | ||
| ): Promise<SessionTransferCredentials> { |
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.
Unnecessary Default Assignment: The nullish coalescing operator creates new empty objects when undefined is passed, which changes the function's semantics. The native modules should handle undefined parameters:
| ): Promise<SessionTransferCredentials> { | |
| async getSSOCredentials( | |
| parameters?: Record<string, any>, | |
| headers?: Record<string, string> | |
| ): Promise<SessionTransferCredentials> { | |
| return this.a0_call( | |
| Auth0NativeModule.getSSOCredentials.bind(Auth0NativeModule), | |
| parameters, | |
| headers | |
| ); | |
| } |
Implements Native to Web SSO functionality enabling seamless authentication between native contexts and WebViews. Applications can obtain session transfer credentials from native SDKs to establish authenticated web sessions without re-authentication.
Usage