feat: open browser login providers in a system auth session for WebAuthn support#7419
feat: open browser login providers in a system auth session for WebAuthn support#7419diegolmello wants to merge 5 commits into
Conversation
diegolmello
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
WalkthroughExtracts an inline ChangesOAuth System Browser Flow
Sequence Diagram(s)sequenceDiagram
actor User
participant providerHandler as Provider Handler
participant openOAuthSession
participant WebBrowser
participant parseDeepLinking
participant handleOAuth
User->>providerHandler: tap OAuth provider button
providerHandler->>openOAuthSession: authorization URL (response_type=code, encoded params)
openOAuthSession->>WebBrowser: openAuthSessionAsync(url, "rocketchat://auth")
WebBrowser-->>User: opens system browser for OAuth provider
User-->>WebBrowser: completes authentication
WebBrowser-->>openOAuthSession: result {type: "success", url: "rocketchat://auth?credentialToken=..."}
openOAuthSession->>parseDeepLinking: result.url
parseDeepLinking-->>openOAuthSession: parsed {type, credentialToken, credentialSecret}
openOAuthSession->>handleOAuth: dispatch(deepLinkingOpen(parsed))
handleOAuth->>handleOAuth: check consumedOAuthToken, deduplicate or call loginOAuthOrSso
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
diegolmello
had a problem deploying
to
android_build
GitHub Actions
Error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/containers/LoginServices/serviceLogin.ts (1)
101-101: ⚡ Quick winAdd explicit return types to the new TS functions.
The new function declarations rely on inferred return types. Please annotate them explicitly (
: void/: Promise<void>) to keep the login flow contract strict and readable.♻️ Suggested change
-export const onPressCustomOAuth = ({ loginService, server }: { loginService: IItemService; server: string }) => { +export const onPressCustomOAuth = ({ loginService, server }: { loginService: IItemService; server: string }): void => { @@ -const openOAuthSession = async (url: string) => { +const openOAuthSession = async (url: string): Promise<void> => {As per coding guidelines,
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types.Also applies to: 149-149
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/containers/LoginServices/serviceLogin.ts` at line 101, The functions onPressCustomOAuth (at line 101) and the other function referenced at line 149 are missing explicit return type annotations. Add explicit return types to both function declarations by specifying either `: void` or `: Promise<void>` depending on whether each function is asynchronous or not. This ensures the login flow contract is strict, readable, and follows the coding guidelines for TypeScript type safety.Source: Coding guidelines
app/lib/methods/helpers/parseDeepLinking.ts (1)
3-23: ⚡ Quick winDefine an explicit parser return contract (and nullable URL input).
The helper currently relies on inferred return typing, which weakens the deep-link payload contract across callers. Add an explicit return type and accept nullable input to match deep-link sources.
♻️ Suggested change
-const parseDeepLinking = (url: string) => { +const parseDeepLinking = (url: string | null | undefined): Record<string, string | undefined> | null => {As per coding guidelines,
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/methods/helpers/parseDeepLinking.ts` around lines 3 - 23, The parseDeepLinking function lacks explicit type annotations for both its parameter and return type, which weakens type safety. Add an explicit return type annotation to the parseDeepLinking function to define the deep-link payload contract, and make the url parameter accept nullable input (string | null or string | undefined) to properly handle cases where deep-link sources may provide null or undefined values.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/containers/LoginServices/serviceLogin.ts`:
- Around line 112-113: The URL composition logic in the serviceLogin function
uses includes(domain) to check if absolutePath is absolute, which is unreliable
and can generate invalid URLs. Replace the includes(domain) check with a proper
absolute-URL detection method (such as checking if absolutePath starts with a
protocol like http:// or https://, or using a URL parsing utility) to correctly
determine whether to prefix absolutePath with the domain before passing it to
openOAuthSession(url).
In `@app/sagas/deepLinking.js`:
- Around line 134-139: The variable consumedOAuthToken is being assigned the
credentialToken value before the loginOAuthOrSso function call completes. This
means if the login fails, the token is permanently marked as consumed for the
session. Move the assignment of consumedOAuthToken = credentialToken to after
the yield loginOAuthOrSso call succeeds, either by placing it after the yield
statement in the try block or in a success callback, so that only successfully
authenticated tokens are marked as consumed.
---
Nitpick comments:
In `@app/containers/LoginServices/serviceLogin.ts`:
- Line 101: The functions onPressCustomOAuth (at line 101) and the other
function referenced at line 149 are missing explicit return type annotations.
Add explicit return types to both function declarations by specifying either `:
void` or `: Promise<void>` depending on whether each function is asynchronous or
not. This ensures the login flow contract is strict, readable, and follows the
coding guidelines for TypeScript type safety.
In `@app/lib/methods/helpers/parseDeepLinking.ts`:
- Around line 3-23: The parseDeepLinking function lacks explicit type
annotations for both its parameter and return type, which weakens type safety.
Add an explicit return type annotation to the parseDeepLinking function to
define the deep-link payload contract, and make the url parameter accept
nullable input (string | null or string | undefined) to properly handle cases
where deep-link sources may provide null or undefined values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 20acd6e4-de2d-448c-89ab-3b1a28ad79cc
📒 Files selected for processing (5)
app/containers/LoginServices/serviceLogin.tsapp/index.tsxapp/lib/methods/helpers/__tests__/parseDeepLinking.test.tsapp/lib/methods/helpers/parseDeepLinking.tsapp/sagas/deepLinking.js
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/methods/helpers/__tests__/parseDeepLinking.test.tsapp/sagas/deepLinking.jsapp/lib/methods/helpers/parseDeepLinking.tsapp/index.tsxapp/containers/LoginServices/serviceLogin.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript with strict mode enabled
Files:
app/lib/methods/helpers/__tests__/parseDeepLinking.test.tsapp/lib/methods/helpers/parseDeepLinking.tsapp/index.tsxapp/containers/LoginServices/serviceLogin.ts
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses
Files:
app/lib/methods/helpers/__tests__/parseDeepLinking.test.tsapp/sagas/deepLinking.jsapp/lib/methods/helpers/parseDeepLinking.tsapp/index.tsxapp/containers/LoginServices/serviceLogin.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce ESLint rules from
@rocket.chat/eslint-configwith React, React Native, TypeScript, and Jest plugins
Files:
app/lib/methods/helpers/__tests__/parseDeepLinking.test.tsapp/sagas/deepLinking.jsapp/lib/methods/helpers/parseDeepLinking.tsapp/index.tsxapp/containers/LoginServices/serviceLogin.ts
app/containers/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place reusable UI components in 'app/containers/' directory
Files:
app/containers/LoginServices/serviceLogin.ts
🧠 Learnings (2)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/lib/methods/helpers/__tests__/parseDeepLinking.test.tsapp/lib/methods/helpers/parseDeepLinking.tsapp/index.tsxapp/containers/LoginServices/serviceLogin.ts
📚 Learning: 2026-05-07T13:19:52.152Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7304
File: app/sagas/deepLinking.js:237-243
Timestamp: 2026-05-07T13:19:52.152Z
Learning: In this codebase’s Redux-Saga usage, remember that `yield put(action)` dispatches through the Redux store synchronously, and any saga(s) that synchronously react via action listeners (and synchronous `put` chains) will run to completion before the calling saga resumes at its next `yield`. As a result, within a single saga there is no scheduler interleaving between a `yield select(...)` and a subsequent `yield take(...)` at the next `yield` point, so a check-then-take pattern like `const state = yield select(...); if (state !== TARGET) { yield take(a => a.type === TARGET); }` is safe from TOCTOU races under the synchronous `put`/take model described above.
Applied to files:
app/sagas/deepLinking.js
🔇 Additional comments (2)
app/lib/methods/helpers/__tests__/parseDeepLinking.test.ts (1)
3-38: LGTM!app/index.tsx (1)
25-25: LGTM!
The guard that stops Android's double-delivered OAuth redirect from consuming the single-use credentialToken twice had no coverage. Add cases asserting a fresh token logs in once, a repeat of the same token is a no-op, and a distinct token still logs in. Claude-Session: https://claude.ai/code/session_01GrrvSigJqJYa9CK4W1FiSy
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
diegolmello
had a problem deploying
to
android_build
GitHub Actions
Error
handleOAuth marks a credentialToken consumed before logging in; without a secret the login can only fail, needlessly burning the token. Bail out early when the secret is absent, alongside the empty-token and already-consumed checks. Claude-Session: https://claude.ai/code/session_01GrrvSigJqJYa9CK4W1FiSy
diegolmello
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
diegolmello
temporarily deployed
to
android_build
GitHub Actions
Inactive
|
iOS Build Available Rocket.Chat 4.74.0.109165 |
diegolmello
had a problem deploying
to
upload_android
GitHub Actions
Error
|
Android Build Available Rocket.Chat 4.74.0.109164 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNTk4HMV6Bd1HbnFwjI4A8n5GKE4wR8DNwCBqhpJ26Ym6-eNwrMR4mSEL6KRNI3BMcgSb1I7wa3UIu8uRlKq |
WebViews cannot run the browser WebAuthn/FIDO2 API, so providers that require passkeys could not complete login on mobile. Following the path already used by Custom OAuth and Google, every browser-based login now opens in a system auth session (ASWebAuthenticationSession on iOS, Chrome Custom Tabs on Android), which supports WebAuthn. - Facebook, Github, Gitlab, Google, Linkedin, Meteor, Twitter and Wordpress request the server's redirect login style and open in the auth session, completing through the deep-link pipeline. - SAML and CAS open in the auth session and complete by parsing the redirect result directly. The in-app WebView is retained for iframe authentication. Claude-Session: https://claude.ai/code/session_01GrrvSigJqJYa9CK4W1FiSy
diegolmello
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
diegolmello
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/containers/LoginServices/serviceLogin.ts`:
- Around line 124-129: In the onPressCas function, the `service` parameter is
constructed with an unencoded server URL value, which can cause parsing issues
on certain CAS servers. To fix this, apply URL encoding to the `server` variable
when building the URL string by wrapping it with encodeURIComponent() before
inserting it into the query string, so the full callback URL is properly escaped
as a query parameter value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ddbd5ede-5af7-4c28-b053-6122f0d322d8
📒 Files selected for processing (1)
app/containers/LoginServices/serviceLogin.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/containers/LoginServices/serviceLogin.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript with strict mode enabled
Files:
app/containers/LoginServices/serviceLogin.ts
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses
Files:
app/containers/LoginServices/serviceLogin.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce ESLint rules from
@rocket.chat/eslint-configwith React, React Native, TypeScript, and Jest plugins
Files:
app/containers/LoginServices/serviceLogin.ts
app/containers/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place reusable UI components in 'app/containers/' directory
Files:
app/containers/LoginServices/serviceLogin.ts
🧠 Learnings (1)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/containers/LoginServices/serviceLogin.ts
🔇 Additional comments (6)
app/containers/LoginServices/serviceLogin.ts (6)
5-12: LGTM!
16-70: LGTM!
72-98: LGTM!
115-122: LGTM!
162-177: LGTM!
179-191: LGTM!
| export const onPressCas = ({ casLoginUrl, server }: { casLoginUrl: string; server: string }) => { | ||
| logEvent(events.ENTER_WITH_CAS); | ||
| const ssoToken = random(17); | ||
| const url = `${casLoginUrl}?service=${server}/_cas/${ssoToken}`; | ||
| openOAuth({ url, ssoToken, authType: 'cas' }); | ||
| openSSOSession(url, 'cas', ssoToken); | ||
| }; |
There was a problem hiding this comment.
URL-encode the service parameter value.
The service parameter contains a full callback URL with protocol and slashes (e.g., https://server/_cas/token). Without encoding, certain CAS servers may misparse the query string. This could cause authentication failures.
🐛 Suggested fix
export const onPressCas = ({ casLoginUrl, server }: { casLoginUrl: string; server: string }) => {
logEvent(events.ENTER_WITH_CAS);
const ssoToken = random(17);
- const url = `${casLoginUrl}?service=${server}/_cas/${ssoToken}`;
+ const url = `${casLoginUrl}?service=${encodeURIComponent(`${server}/_cas/${ssoToken}`)}`;
openSSOSession(url, 'cas', ssoToken);
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/containers/LoginServices/serviceLogin.ts` around lines 124 - 129, In the
onPressCas function, the `service` parameter is constructed with an unencoded
server URL value, which can cause parsing issues on certain CAS servers. To fix
this, apply URL encoding to the `server` variable when building the URL string
by wrapping it with encodeURIComponent() before inserting it into the query
string, so the full callback URL is properly escaped as a query parameter value.
|
Android Build Available Rocket.Chat 4.74.0.109172 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNSOSmhEdjM-TdUouhq23CC26i-pP84kr41y0ifE6AUpFPGfDNSZ56fngyhbyKIABDQsu1xV3g3xrag72eyI |
|
iOS Build Available Rocket.Chat 4.74.0.109173 |
1 participant
Proposed changes
Browser-based login currently opens inside an in-app WebView. WebViews cannot run the browser WebAuthn/FIDO2 API, so providers that require passkeys (Custom OAuth with Keycloak, Authentik, AWS Cognito, etc.) cannot complete login on mobile.
This switches every browser-based login to a real system auth session via
WebBrowser.openAuthSessionAsync(ASWebAuthenticationSession on iOS, Chrome Custom Tabs on Android), both of which support WebAuthn. It follows the path Custom OAuth and Google login already used: the provider opens in the auth session, and the app completes login when the browser redirects back.Changes:
serviceLogin.ts— Facebook, Github, Gitlab, Google, Linkedin, Meteor, Twitter and Wordpress request the server'sredirectlogin style and open in the auth session. The server redirects torocketchat://auth?…&type=oauth&credentialToken=…&credentialSecret=…, and login completes through the existing deep-link pipeline (parseDeepLinking→deepLinkingOpen→handleOAuth). SAML and CAS open in the auth session and complete by parsing the redirect result directly withparseSamlOrCasRedirect. The now-unused WebView navigation helper is removed.parseDeepLinking.ts(new) — the deep-link parser is extracted fromapp/index.tsxinto a shared helper so both the OS deep-link listener and the new auth-session handler can use it. Behavior is unchanged.deepLinking.js—handleOAuthis guarded against re-using a single-usecredentialToken. On Android the redirect is delivered on two channels (the auth-session result and the OS deep-link listener, since therocketchatscheme is registeredBROWSABLE), which would otherwise consume the token twice and fail the login.The in-app WebView is retained for iframe authentication.
Issue(s)
How to test or reproduce
OAuth (incl. WebAuthn/passkey):
SAML and CAS (dedicated QA step — please test explicitly):
These two now complete by parsing the auth-session redirect result rather than inspecting WebView navigation, so they need explicit verification on both iOS and Android.
Screenshots
Types of changes
Checklist
Further comments
The server has supported redirect-style OAuth for all providers since 3.15.0, so no server change is required. A single deep-link parser is shared between the OS listener and the auth-session handler.
Summary by CodeRabbit