fix(login): resume browser login after returning via the launcher icon - #6811

No actionable comments were generated in the recent review. 🎉

Priority: ➖ Normal

Merge Risk: 🔵 Low · up to 13b3a

A login response arriving just after the 20-minute pending-login window can still complete account login. This is a narrow timing issue; the empty-response handling is fixed, and the remaining expiry risk is bounded.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 57902

The normal resume path preserves account identity and keeps login tokens out of persistent storage. However, cancellation and completion are not tied to a particular pending login, so late or overlapping operations can revive an abandoned login or discard another login’s recovery state. No credential disclosure or account takeover was demonstrated.

Retained concerns

• Medium · security · inferred: The new authentication handoff lacks flow-scoped cancellation and cleanup. A late initial response can recreate pending state after cancellation while its ViewModel remains alive; with overlapping flows, an older cancellation or poll completion can clear a newer pending record. These conditional races weaken abandonment and recovery guarantees, rather than demonstrating credential theft or identity substitution.

• Fix all pre-merge checks with AI

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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 @coderabbitai help to get the list of available commands.}

Hi @ToteMeiSter thank you for your contributions.
We will review them soon

@coderabbitai Re the outside-diff comment "Ignore poll results after explicit cancellation" (review 5432917494): fixed in 3504130. handleWebBrowserLogin returns when isLoginCanceled is set before it calls parseAndLogin, so a poll answered after the cancel neither logs in nor publishes a post-login state. Covered by the test cancel ends the pending login, also when the poll answers after it.

Re the retained security concern about overlapping flows: a late initial response after the cancel is covered since 1ddb1f5, and a late poll result by the commit above. A per-flow generation is not added. Only one browser login runs at a time: the activity whose login is resumed was destroyed by the launcher return, which cancels its view model scope before any clear. So an older poll cannot clear the record of a newer one in the flows this PR handles. A generation counter would add state for a race these flows do not reach.

Drafted with AI assistance (Claude Code).

@coderabbitai Re the nitpick "Add an active regression test for launcher re-entry" (review 5433085011, MainActivity.kt 288-294): not added in this PR, on purpose.

• MainActivity has no unit test setup: it is Dagger-injected and decides the next screen inside a coroutine over UserManager. A Robolectric test would need a fake application component only for this branch.
• MainActivityTest is an instrumented test. I can not run instrumented tests locally, and an untested UI test is not something I want to hand to the maintainers.
• The resume logic itself is covered by BrowserLoginActivityViewModelTest (pending login saved, resumed once without a new login request, cleared after poll end, cancel, failure, and expiry). The MainActivity part is a three-way branch plus an intent with KEY_RESUME_BROWSER_LOGIN.

If a maintainer prefers it, I can extract the screen decision into a small function with a unit test in a follow-up.

Drafted with AI assistance (Claude Code).

This is a bug fix and all CodeRabbit review threads are resolved. Once it is approved and merged, would it be possible to backport it to stable-25.0.x (e.g. for 25.0.3)? The bug is present in v25.0.2.

Suggested command for a maintainer after merge: /backport to stable-25.0.x

This comment was drafted with AI assistance (Claude Code).

#!/bin/bash
fd '^(BrowserLoginActivityViewModel|BrowserLoginActivityViewModelTest|BrowserLoginActivity|MainActivity|PendingBrowserLoginStore)\.kt$'
ast-grep outline app/src/main/java/com/nextcloud/talk/account/viewmodels/BrowserLoginActivityViewModel.kt --items all
ast-grep outline app/src/main/java/com/nextcloud/talk/account/BrowserLoginActivity.kt --items all
ast-grep outline app/src/main/java/com/nextcloud/talk/activities/MainActivity.kt --match 'onCreate|onNewIntent|init' --items all
ast-grep outline app/src/main/java/com/nextcloud/talk/account/data/PendingBrowserLoginStore.kt --items all
ast-grep outline app/src/test/java/com/nextcloud/talk/account/viewmodels/BrowserLoginActivityViewModelTest.kt --items all

#!/bin/bash
sed -n '24,240p' app/src/main/java/com/nextcloud/talk/account/viewmodels/BrowserLoginActivityViewModel.kt
sed -n '39,205p' app/src/main/java/com/nextcloud/talk/account/BrowserLoginActivity.kt
sed -n '36,310p' app/src/test/java/com/nextcloud/talk/account/viewmodels/BrowserLoginActivityViewModelTest.kt
sed -n '14,90p' app/src/main/java/com/nextcloud/talk/account/data/PendingBrowserLoginStore.kt
ast-grep outline app/src/main/java/com/nextcloud/talk/activities/MainActivity.kt --items all
sed -n '235,330p' app/src/main/java/com/nextcloud/talk/activities/MainActivity.kt
rg -n -C3 'singleTask|BrowserLoginActivity|MainActivity' app/src/main/AndroidManifest.xml

@ToteMeiSter Thanks for the clarification. I checked the current code and test source.

• Cancellation finding addressed: handleWebBrowserLogin() checks isLoginCanceled before clearing pending state or calling parseAndLogin(). The regression test supplies a successful poll result after cancellation and checks that no login or post-login state follows. I did not run the tests.
• Overlapping-flow concern withdrawn for this PR: The launcher-return path destroys the previous activity and cancels its ViewModel scope. Polling rethrows CancellationException before clearing pending state. Together with the late-response guards, this supports your explanation. A per-flow generation counter is not required for the flows covered here.

Learnt from: ToteMeiSter
URL: https://github.com/nextcloud/talk-android/pull/6811

Timestamp: 2026-10-06T18:44:14.786Z
Learning: In nextcloud/talk-android, the browser-login launcher-return flow uses singleTask MainActivity to destroy the previous BrowserLoginActivity before resuming its pending login. BrowserLoginActivityViewModel runs polling in viewModelScope and rethrows CancellationException before clearing PendingBrowserLoginStore. Explicit cancellation also guards late initial responses and late poll results with isLoginCanceled. Do not require a per-flow generation counter solely for overlapping old and resumed login flows in this lifecycle path unless a reachable overlap is demonstrated.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

You are interacting with an AI system.

#!/bin/bash
fd '^(MainActivity|MainActivityTest|BrowserLoginActivityViewModelTest)\.kt$'
ast-grep outline app/src/main/java/com/nextcloud/talk/activities/MainActivity.kt --items all --match 'onCreate|onNewIntent|init'
ast-grep outline app/src/test/java/com/nextcloud/talk/account/viewmodels/BrowserLoginActivityViewModelTest.kt --items all
fd '^MainActivityTest\.kt$' --exec ast-grep outline {} --items all

#!/bin/bash
ast-grep outline app/src/main/java/com/nextcloud/talk/activities/MainActivity.kt --items all
sed -n '70,155p' app/src/main/java/com/nextcloud/talk/activities/MainActivity.kt
sed -n '235,335p' app/src/main/java/com/nextcloud/talk/activities/MainActivity.kt
sed -n '1,100p' app/src/androidTest/java/com/nextcloud/talk/activities/MainActivityTest.kt
sed -n '36,310p' app/src/test/java/com/nextcloud/talk/account/viewmodels/BrowserLoginActivityViewModelTest.kt
fd '^MainActivity.*Test\.kt$' app/src/test

@ToteMeiSter Thanks for the clarification. I withdraw the nitpick as a requirement for this PR.

I checked the source. MainActivity uses application-component injection and coroutine-based routing through UserManager. MainActivityTest is an instrumented test. BrowserLoginActivityViewModelTest covers the resume and pending-state cases you listed. I did not run the tests.

Those unit tests do not exercise launcher re-entry through MainActivity. Your reported device check covers that path manually. This is a reasonable test scope for this bug fix. Extracting the screen decision can remain an optional follow-up if a maintainer requests it.

Learnt from: ToteMeiSter
URL: https://github.com/nextcloud/talk-android/pull/6811

Timestamp: 2026-10-06T18:44:59.813Z
Learning: For nextcloud/talk-android PR #6811, MainActivity.kt uses application-component injection and coroutine-based routing through UserManager. MainActivityTest.kt is an instrumented test, and the contributor cannot run instrumented tests locally. BrowserLoginActivityViewModelTest.kt covers pending login persistence, resumption without a new login request, duplicate polling, completion, cancellation, failure, and expiry. The contributor reports checking launcher re-entry on a device. Do not require new activity test infrastructure solely for this PR's launcher-routing branch; extracting the screen decision for unit testing is an optional maintainer-requested follow-up.

Learnt from: ToteMeiSter
URL: https://github.com/nextcloud/talk-android/pull/6811

Timestamp: 2026-10-06T18:44:15.005Z
Learning: In nextcloud/talk-android, the browser-login launcher-return flow uses singleTask MainActivity to destroy the previous BrowserLoginActivity before resuming its pending login. BrowserLoginActivityViewModel runs polling in viewModelScope and rethrows CancellationException before clearing PendingBrowserLoginStore. Explicit cancellation also guards late initial responses and late poll results with isLoginCanceled. Do not require a per-flow generation counter solely for overlapping old and resumed login flows in this lifecycle path unless a reachable overlap is demonstrated.

You are interacting with an AI system.

#!/bin/bash
set -euo pipefail
fd '^(MainActivity|MainActivityTest|BrowserLoginActivityViewModelTest)\.kt$'
ast-grep outline app/src/main/java/com/nextcloud/talk/activities/MainActivity.kt --items all
sed -n '70,155p' app/src/main/java/com/nextcloud/talk/activities/MainActivity.kt
sed -n '235,335p' app/src/main/java/com/nextcloud/talk/activities/MainActivity.kt
fd '^MainActivityTest\.kt$' --exec ast-grep outline {} --items all
fd '^MainActivityTest\.kt$' --exec sed -n '1,110p' {}
ast-grep outline app/src/test/java/com/nextcloud/talk/account/viewmodels/BrowserLoginActivityViewModelTest.kt --items all
sed -n '36,310p' app/src/test/java/com/nextcloud/talk/account/viewmodels/BrowserLoginActivityViewModelTest.kt

@ToteMeiSter Thanks for the clarification. I withdraw the launcher re-entry test nitpick as a requirement for this PR.

I checked the source. MainActivity uses application-component injection and coroutine-based routing through UserManager. MainActivityTest is an instrumented test. BrowserLoginActivityViewModelTest covers the pending-login and resume cases you listed. I did not run the tests.

The ViewModel tests do not exercise launcher re-entry through MainActivity. Your previously reported device check covers that path manually. I used the existing learning about that check and your test constraints.

This test scope is reasonable for this bug fix. Extracting the screen decision can remain an optional follow-up if a maintainer requests it.

Learnt from: ToteMeiSter
URL: https://github.com/nextcloud/talk-android/pull/6811

Timestamp: 2026-10-06T18:44:59.916Z
Learning: For nextcloud/talk-android PR #6811, MainActivity.kt uses application-component injection and coroutine-based routing through UserManager. MainActivityTest.kt is an instrumented test, and the contributor cannot run instrumented tests locally. BrowserLoginActivityViewModelTest.kt covers pending login persistence, resumption without a new login request, duplicate polling, completion, cancellation, failure, and expiry. The contributor reports checking launcher re-entry on a device. Do not require new activity test infrastructure solely for this PR's launcher-routing branch; extracting the screen decision for unit testing is an optional maintainer-requested follow-up.

You are interacting with an AI system.

@coderabbitai Correction: the launcher re-entry path through MainActivity has not been checked on a device yet. The first version of my comment said so by mistake; I edited it. Please do not keep a learning that it was device-checked.

Drafted with AI assistance (Claude Code).

@ToteMeiSter Thanks for the correction. My earlier replies incorrectly stated that launcher re-entry through MainActivity was device-checked. That path has not been checked on a device yet. The ViewModel tests do not verify that activity path.

The tags below request replacement of the inaccurate learning. You can also delete specific learnings from the CodeRabbit UI Learnings page.

Learnt from: ToteMeiSter
URL: https://github.com/nextcloud/talk-android/pull/6811

Timestamp: 2026-10-06T18:47:01.814Z
Learning: For nextcloud/talk-android PR #6811, ToteMeiSter clarified that launcher re-entry through MainActivity has not been checked on a device yet; the earlier claim was a mistake. MainActivity uses application-component injection and coroutine-based routing through UserManager. MainActivityTest is an instrumented test, which the contributor cannot run locally. BrowserLoginActivityViewModelTest covers pending login persistence, resumption without a new login request, duplicate polling, completion, cancellation, failure, and expiry, but does not exercise launcher re-entry through MainActivity. Do not require new activity test infrastructure solely for this PR's launcher-routing branch; extracting the screen decision for unit testing is an optional maintainer-requested follow-up.

Learnt from: ToteMeiSter
URL: https://github.com/nextcloud/talk-android/pull/6811

Timestamp: 2026-10-06T18:44:59.916Z
Learning: For nextcloud/talk-android PR #6811, MainActivity.kt uses application-component injection and coroutine-based routing through UserManager. MainActivityTest.kt is an instrumented test, and the contributor cannot run instrumented tests locally. BrowserLoginActivityViewModelTest.kt covers pending login persistence, resumption without a new login request, duplicate polling, completion, cancellation, failure, and expiry. The contributor reports checking launcher re-entry on a device. Do not require new activity test infrastructure solely for this PR's launcher-routing branch; extracting the screen decision for unit testing is an optional maintainer-requested follow-up.

You are interacting with an AI system.

@coderabbitai Regarding the outside-diff comment on NetworkLoginDataSource.kt:147-155 (empty HTTP 200 answer): fixed in d2e9fc9. A 200 without a body now returns null and ends the poll as unusable; other statuses without a body still mean "not confirmed yet". Covered by testing performLoginFlowV2 empty 200 response in NetworkLoginDataSourceTest.

#!/bin/bash
set -euo pipefail
fd '^(NetworkLoginDataSource|NetworkLoginDataSourceTest|LoginRepository)\.kt$'
ast-grep outline app/src/main/java/com/nextcloud/talk/account/data/network/NetworkLoginDataSource.kt --items all --match 'performLoginFlowV2|HTTP'
sed -n '110,195p' app/src/main/java/com/nextcloud/talk/account/data/network/NetworkLoginDataSource.kt
ast-grep outline app/src/test/java/com/nextcloud/talk/login/data/network/NetworkLoginDataSourceTest.kt --items all --match 'empty|performLoginFlowV2'
rg -n -C14 'testing performLoginFlowV2 empty 200 response' app/src/test/java/com/nextcloud/talk/login/data/network/NetworkLoginDataSourceTest.kt
ast-grep outline app/src/main/java/com/nextcloud/talk/account/data/LoginRepository.kt --items all --match 'pollLogin'
rg -n -A65 -B3 'suspend fun pollLogin' app/src/main/java/com/nextcloud/talk/account/data/LoginRepository.kt

@ToteMeiSter Thanks for the fix. I checked the current source.

• performLoginFlowV2() returns null for an HTTP 200 response with an empty or missing body. pollLogin() then ends polling.
• Other statuses with an empty or missing body retain their status, so polling continues.
• testing performLoginFlowV2 empty 200 response asserts that an empty HTTP 200 response returns null.

This addresses the finding. I did not run the tests.

You are interacting with an AI system.

@coderabbitai review