Skip to content

test(integration): bypass SSO browser UI with direct OIDC API login flow#2980

Open
9clg6 wants to merge 2 commits intomainfrom
feat/integration-test-api-login-flow
Open

test(integration): bypass SSO browser UI with direct OIDC API login flow#2980
9clg6 wants to merge 2 commits intomainfrom
feat/integration-test-api-login-flow

Conversation

@9clg6
Copy link
Copy Markdown
Collaborator

@9clg6 9clg6 commented Apr 1, 2026

Problem

The integration test login was brittle:

  • Depended on the device locale (button labels like "Continue", "Continuer"…)
  • Required navigating the LemonLDAP browser modal via native taps — fragile and slow
  • Difficult to debug when the SSO portal changed its response structure (missing cookie, consent page, etc.)

Solution

Replace the browser-based SSO flow with a headless HTTP OIDC flow that runs entirely via dart:io HttpClient, then feeds the resulting loginToken directly to the app.

Changes

  • core_robot.dart — Extract getLoginTokenViaOIDC(username, password): runs all 7 OIDC steps over HTTP including lemonldappdata cookie forwarding and consent-page fallback. Returns (loginToken, lemonldap).
  • login_robot.dart — Add loginViaApi(): calls the OIDC helper then navigates via TwakeApp.router.go('/onAuthRedirect?loginToken=…&homeserver=…'), bypassing app_links entirely.
  • login_scenario.dart — Reduce login() to a single loginViaApi() call; removes fragile UI steps.
  • on_auth_redirect.dart — Read GoRouterState params synchronously before any await (inherited widget anti-pattern fix), add mounted guards, support homeserver as query param.
  • init_config_mixin.dart — Ensure initConfigCompleter is always completed even on config fetch error.
  • pubspec.yaml — Add patrol_cli dev dependency.

Test example

Enregistrement.de.l.ecran.2026-04-02.a.09.16.16.mov

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced login redirect stability by adding lifecycle guards to prevent navigation and state mutations after widget disposal.
    • Improved app initialization by ensuring mobile configuration completion is properly tracked and homeserver resolution is correctly validated.
  • Tests

    • Refactored login test infrastructure with new API-based login option, enhanced OIDC flow validation, and improved error handling.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 1, 2026

Walkthrough

This pull request refactors the login authentication flow to support API-based OIDC login alongside UI-driven approaches. Changes include extracting OIDC token acquisition into a reusable getLoginTokenViaOIDC method, adding MCP server configuration, introducing a new loginViaApi method that bypasses Flutter UI steps by directly performing HTTP-based OIDC and navigating to the auth redirect page, and simplifying the login scenario to use this new path. Additionally, lifecycle guards are added to prevent state mutations post-disposal, homeserver resolution logic is improved in the auth redirect handler, and the mobile config initializer now completes its completer on both success and error paths.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely summarizes the main objective: replacing browser-based SSO login with a direct HTTP OIDC API login flow in integration tests.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check ✅ Passed PR description fully addresses the template sections with clear problem, solution, and changes, though some template sections like 'Test recommendations', 'Pre-merge', and 'Resolved' with media are not completed.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/integration-test-api-login-flow

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

codescene-delta-analysis[bot]

This comment was marked as outdated.

codescene-delta-analysis[bot]

This comment was marked as outdated.

codescene-delta-analysis[bot]

This comment was marked as outdated.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 1, 2026

This PR has been deployed to https://linagora.github.io/twake-on-matrix/2980

codescene-delta-analysis[bot]

This comment was marked as outdated.

codescene-delta-analysis[bot]

This comment was marked as outdated.

codescene-delta-analysis[bot]

This comment was marked as outdated.

codescene-delta-analysis[bot]

This comment was marked as outdated.

@9clg6 9clg6 force-pushed the feat/integration-test-api-login-flow branch from 3b76faa to 93d1198 Compare April 1, 2026 20:13
codescene-delta-analysis[bot]

This comment was marked as outdated.

@9clg6 9clg6 marked this pull request as ready for review April 1, 2026 20:15
codescene-delta-analysis[bot]

This comment was marked as outdated.

@9clg6 9clg6 force-pushed the feat/integration-test-api-login-flow branch from e13ab5e to 66f3f41 Compare April 1, 2026 20:17
codescene-delta-analysis[bot]

This comment was marked as outdated.

Replace the browser-based SSO login in integration tests with a
headless HTTP OIDC flow. The old approach was brittle: it depended
on the device locale (button labels), required navigating the LemonLDAP
browser modal, and was hard to debug when the SSO portal changed
its response structure.

Key changes:
- Extract getLoginTokenViaOIDC() in CoreRobot: runs all 7 OIDC steps
  (including lemonldappdata cookie forwarding and consent-page fallback)
  via raw HTTP, returning a one-time loginToken without any UI interaction.
- Add loginViaApi() in LoginRobot: calls the OIDC helper then feeds
  the loginToken directly to the app via the /onAuthRedirect deep link
  using TwakeApp.router.go(), bypassing the app_links listener.
- Simplify LoginScenario.login() to a single loginViaApi() call.
- Add patrol_cli dev dependency for running Patrol tests from CLI.
- Add .mcp.json MCP servers configuration.
On cold start via SSO deep link, AppConfig.homeserver may be empty when
OnAuthRedirect mounts. Reading it before await initConfigCompleter.future
caused a silent auth failure.

homeserverFromRoute is now String? — only the route param (available
synchronously) is captured before the await. The fallback to
AppConfig.homeserver is deferred to after initConfig resolves, where it
is guaranteed to be populated.

Also fix InitConfigMixin to ensure initConfigCompleter is always
completed even when the config fetch throws, and add mounted guards
to prevent use-after-dispose crashes.
Copy link
Copy Markdown

@codescene-delta-analysis codescene-delta-analysis bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gates Passed
3 Quality Gates Passed

See analysis details in CodeScene

Quality Gate Profile: The Bare Minimum
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.

@9clg6
Copy link
Copy Markdown
Collaborator Author

9clg6 commented Apr 2, 2026

@coderabbitai review please

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 2, 2026

@9clg6, I'll start reviewing the changes right away!


🧠 Learnings used
Learnt from: nqhhdev
Repo: linagora/twake-on-matrix PR: 0
File: :0-0
Timestamp: 2026-03-27T03:14:38.748Z
Learning: In `lib/pages/login/login.dart` (linagora/twake-on-matrix PR `#2911`), the regex-based identifier branching (`username.isEmail` → `AuthenticationThirdPartyIdentifier(medium: 'email', ...)`, `username.isPhoneNumber` → `AuthenticationThirdPartyIdentifier(medium: 'msisdn', ...)`, otherwise `AuthenticationUserIdentifier`) is intentional. It is kept to support homeservers that advertise 3PID (email/phone) password login flows. Do NOT suggest replacing it with a universal `AuthenticationUserIdentifier` in future reviews.
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
lib/presentation/mixins/init_config_mixin.dart (1)

35-43: ⚠️ Potential issue | 🟠 Major

Complete initConfigCompleter with the real success state.

The catch path still completes the mobile config future with true. OnAuthRedirect.tryLoggingUsingToken() now uses that boolean to decide whether config is usable, so a loadEnvironment() failure will be cached as a successful init and the flow will continue with unset config.

Suggested fix
  Future<void> initConfigMobile() async {
+    var isConfigured = true;
     try {
       AppConfig.loadEnvironment();
     } catch (e) {
+      isConfigured = false;
       Logs().e('[ConfigLoader] Config mobile error', e);
     }
     if (!AppConfig.initConfigCompleter.isCompleted) {
-      AppConfig.initConfigCompleter.complete(true);
+      AppConfig.initConfigCompleter.complete(isConfigured);
     }
   }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lib/presentation/mixins/init_config_mixin.dart` around lines 35 - 43, In
initConfigMobile, the catch block currently still completes
AppConfig.initConfigCompleter with true causing failures to be treated as
success; change the logic so AppConfig.initConfigCompleter.complete is called
with true only on successful AppConfig.loadEnvironment() and complete(false) in
the catch path (or completeError) so callers like
OnAuthRedirect.tryLoggingUsingToken() receive the real success state; update
references in this function (initConfigMobile), the load call
(AppConfig.loadEnvironment), the completer usage
(AppConfig.initConfigCompleter.complete) and the error log (Logs().e)
accordingly.
🧹 Nitpick comments (1)
.mcp.json (1)

3-6: Consider pinning chrome-devtools-mcp to a specific version.

Using @latest tag can lead to unexpected behavior if the package releases breaking changes. For development tooling stability, consider pinning to a specific version (e.g., chrome-devtools-mcp@1.2.3).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.mcp.json around lines 3 - 6, Update the chrome-devtools mcp invocation to
pin the package to a specific released version instead of using the `@latest` tag:
replace the "chrome-devtools-mcp@latest" entry inside the args array for the
"chrome-devtools" command (the npx invocation) with a concrete semver string
like "chrome-devtools-mcp@1.2.3" (pick the current tested release), and document
or add a comment noting to bump this version intentionally when upgrading.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.mcp.json:
- Around line 17-24: Create the missing executable script at .claude/run-patrol
which wraps the actual patrol startup command (e.g., invokes the test runner or
server used in this repo) and respects the PROJECT_ROOT and PATROL_FLAGS env
vars; ensure the script is added to the repo, marked executable, and contains
robust error handling and logging so failures surface. Replace the literal
placeholder "<YOUR_DEVICE_ID>" in the PATROL_FLAGS value with either a real
device identifier or a shell-friendly substitution (e.g., read from an
environment variable like $DEVICE_ID) so the command in .mcp.json becomes usable
at runtime, and verify the .claude/run-patrol script reads and uses PATROL_FLAGS
when launching the patrol process.

In `@integration_test/base/core_robot.dart`:
- Around line 208-215: getLoginTokenViaOIDC currently reads
MATRIX_URL/SSO_URL/CHAT_URL from environment while loginViaApi now accepts a
serverUrl, which can cause mismatched homeservers; update getLoginTokenViaOIDC
to accept a serverUrl (String serverUrl) and use it to derive or override
matrix/sso/chat URLs instead of String.fromEnvironment, and update all callers
to forward the selected serverUrl (or alternatively remove serverUrl from
loginViaApi if you prefer the env-only contract). Specifically, change the
getLoginTokenViaOIDC signature and references inside the function (use the
passed serverUrl when constructing endpoints for the OIDC flow), then update
call sites that invoke getLoginTokenViaOIDC to pass the same serverUrl used for
loginViaApi so both flows target the same homeserver.
- Around line 404-407: The substring calls pass responseBody.length.clamp(0,
300) (a num) to String.substring which requires an int; fix both occurrences by
converting the clamp result to an int (e.g., call .toInt() after clamp) or use
an int-aware alternative like min(responseBody.length, 300). Update the
substring invocations that use responseBody.length.clamp(...) so they provide an
int end index (e.g., responseBody.length.clamp(0,300).toInt() or
math.min(responseBody.length,300)).

In `@lib/pages/login/on_auth_redirect.dart`:
- Around line 90-96: OnAuthRedirect currently reads AppConfig.homeserver on web
before awaiting AppConfig.initConfigCompleter.future, which can snapshot an
empty/stale homeserver; modify the web branch so it does not read
AppConfig.homeserver until after awaiting AppConfig.initConfigCompleter.future
(or otherwise defer using AppConfig.homeserver as a fallback) and update the
later logic that prefers the earlier snapshot (the same logic affecting the
other web-read block) to instead prefer the post-init value; locate uses in
OnAuthRedirect and references to AppConfig.initConfigCompleter and
AppConfig.homeserver to make this change.

---

Outside diff comments:
In `@lib/presentation/mixins/init_config_mixin.dart`:
- Around line 35-43: In initConfigMobile, the catch block currently still
completes AppConfig.initConfigCompleter with true causing failures to be treated
as success; change the logic so AppConfig.initConfigCompleter.complete is called
with true only on successful AppConfig.loadEnvironment() and complete(false) in
the catch path (or completeError) so callers like
OnAuthRedirect.tryLoggingUsingToken() receive the real success state; update
references in this function (initConfigMobile), the load call
(AppConfig.loadEnvironment), the completer usage
(AppConfig.initConfigCompleter.complete) and the error log (Logs().e)
accordingly.

---

Nitpick comments:
In @.mcp.json:
- Around line 3-6: Update the chrome-devtools mcp invocation to pin the package
to a specific released version instead of using the `@latest` tag: replace the
"chrome-devtools-mcp@latest" entry inside the args array for the
"chrome-devtools" command (the npx invocation) with a concrete semver string
like "chrome-devtools-mcp@1.2.3" (pick the current tested release), and document
or add a comment noting to bump this version intentionally when upgrading.
🪄 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: f1b0fac2-1a21-4333-9e97-8acceced2d59

📥 Commits

Reviewing files that changed from the base of the PR and between e3621e0 and afac130.

⛔ Files ignored due to path filters (1)
  • pubspec.lock is excluded by !**/*.lock
📒 Files selected for processing (7)
  • .mcp.json
  • integration_test/base/core_robot.dart
  • integration_test/robots/login_robot.dart
  • integration_test/scenarios/login_scenario.dart
  • lib/pages/login/on_auth_redirect.dart
  • lib/presentation/mixins/init_config_mixin.dart
  • pubspec.yaml

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant