Repository navigation
fix(login): resume browser login after returning via the launcher icon - #6811
ToteMeiSter wants to merge 5 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughThe change stores an unfinished browser login and its reauthentication details in memory for up to 20 minutes. On startup, the app checks for an active pending login and opens the browser login screen in resume mode. The view model restores the saved login flow and polls without starting another login request. Poll completion and cancellation clear the pending state. Back navigation cancels the login before returning to MainActivity. Tests cover resumption, repeated resume calls, cancellation, and expiration. Priority: ➖ Normal Merge Risk: 🔵 Low · up to 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 ReviewSecurity architecture risk: 🟡 Moderate · up to 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
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
ffb3bdb0-0b2b-4d58-8040-7ee5dd6cc267
📒 Files selected for processing (7)
app/src/main/java/com/nextcloud/talk/account/BrowserLoginActivity.ktapp/src/main/java/com/nextcloud/talk/account/data/LoginRepository.ktapp/src/main/java/com/nextcloud/talk/account/data/PendingBrowserLoginStore.ktapp/src/main/java/com/nextcloud/talk/account/viewmodels/BrowserLoginActivityViewModel.ktapp/src/main/java/com/nextcloud/talk/activities/MainActivity.ktapp/src/main/java/com/nextcloud/talk/utils/bundle/BundleKeys.ktapp/src/test/java/com/nextcloud/talk/account/viewmodels/BrowserLoginActivityViewModelTest.kt
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.
|
Hi @ToteMeiSter thank you for your contributions. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Ignore poll results after explicit cancellation. · BrowserLoginActivityViewModel.kt:115-129
app/src/main/java/com/nextcloud/talk/account/viewmodels/BrowserLoginActivityViewModel.kt:115-129
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winIgnore poll results after explicit cancellation.
When the user cancels during polling, the in-flight request can still return HTTP OK before the activity clears its ViewModel scope.
pollLogin()then returns that result, andhandleWebBrowserLogin()callsparseAndLogin()and publishes a post-login state. Return whenisLoginCanceledis true before parsing the result.Suggested fix
} .getOrNull() + if (isLoginCanceled) return@launch // Only reached when the poll ended. If this view model is cleared first, the login stays pending. PendingBrowserLoginStore.clear()
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
d7167285-3f51-4328-81b6-c81f0283cbf6
📒 Files selected for processing (2)
app/src/main/java/com/nextcloud/talk/account/viewmodels/BrowserLoginActivityViewModel.ktapp/src/test/java/com/nextcloud/talk/account/viewmodels/BrowserLoginActivityViewModelTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/com/nextcloud/talk/account/viewmodels/BrowserLoginActivityViewModel.kt
- app/src/test/java/com/nextcloud/talk/account/viewmodels/BrowserLoginActivityViewModelTest.kt
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
|
@coderabbitai Re the outside-diff comment "Ignore poll results after explicit cancellation" (review 5432917494): fixed in 3504130. 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). |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/com/nextcloud/talk/activities/MainActivity.kt (1)
288-294: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an active regression test for launcher re-entry.
MainActivityTest.login()only creates an account. It does not populatePendingBrowserLoginStore, re-enterMainActivity, or assert theKEY_RESUME_BROWSER_LOGINextra.LoginITcontains the only broader login flow, but the class is marked@Ignore.A regression could therefore remove this branch or the resume flag while the active tests still pass. Add a focused active test that simulates launcher re-entry with a pending login and asserts that
BrowserLoginActivityreceivesKEY_RESUME_BROWSER_LOGIN=true.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
7cb1bf18-2157-47e3-b646-3e8f0b4e50d2
📒 Files selected for processing (2)
app/src/main/java/com/nextcloud/talk/account/viewmodels/BrowserLoginActivityViewModel.ktapp/src/test/java/com/nextcloud/talk/account/viewmodels/BrowserLoginActivityViewModelTest.kt
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
|
@coderabbitai Re the nitpick "Add an active regression test for launcher re-entry" (review 5433085011,
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 Suggested command for a maintainer after merge: This comment was drafted with AI assistance (Claude Code). |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/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 allLength of output: 6673 🏁 Script executed: #!/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.xmlLength of output: 29832
✏️ Learnings added
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. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/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 allLength of output: 3321 🏁 Script executed: #!/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/testLength of output: 18199
I checked the source. Those unit tests do not exercise launcher re-entry through ✏️ Learnings added
🧠 Learnings usedYou are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/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.ktLength of output: 21064
I checked the source. The ViewModel tests do not exercise launcher re-entry through This test scope is reasonable for this bug fix. Extracting the screen decision can remain an optional follow-up if a maintainer requests it. 🧠 Learnings usedYou are interacting with an AI system. |
|
@coderabbitai Correction: the launcher re-entry path through Drafted with AI assistance (Claude Code). |
|
Tip For best results, initiate chat on the files or code changes.
The tags below request replacement of the inaccurate learning. You can also delete specific learnings from the CodeRabbit UI Learnings page. ✏️ Learnings added
⛔ Learnings removedYou are interacting with an AI system. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Return null for an empty HTTP 200 response. · NetworkLoginDataSource.kt:147-155
app/src/main/java/com/nextcloud/talk/account/data/network/NetworkLoginDataSource.kt:147-155
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn null for an empty HTTP 200 response.
If the poll returns HTTP 200 without a body, this branch creates
LoginCompletion(200, "", "", "").LoginRepository.pollLogintreats it as success, so the view model attempts login with empty account details instead of reporting an unusable response. Return null for this case, as the new method contract specifies.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
d43601a8-3b38-4bcc-9c7f-bbd46aa96cb7
📒 Files selected for processing (7)
app/src/main/java/com/nextcloud/talk/account/BrowserLoginActivity.ktapp/src/main/java/com/nextcloud/talk/account/data/LoginRepository.ktapp/src/main/java/com/nextcloud/talk/account/data/network/NetworkLoginDataSource.ktapp/src/main/java/com/nextcloud/talk/account/viewmodels/BrowserLoginActivityViewModel.ktapp/src/test/java/com/nextcloud/talk/account/viewmodels/BrowserLoginActivityViewModelTest.ktapp/src/test/java/com/nextcloud/talk/login/data/LoginRepositoryTest.ktapp/src/test/java/com/nextcloud/talk/login/data/network/NetworkLoginDataSourceTest.kt
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
|
@coderabbitai Regarding the outside-diff comment on |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
44753126-30cb-4935-ade2-16553cabfa3e
📒 Files selected for processing (2)
app/src/main/java/com/nextcloud/talk/account/data/network/NetworkLoginDataSource.ktapp/src/test/java/com/nextcloud/talk/login/data/network/NetworkLoginDataSourceTest.kt
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/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.ktLength of output: 8092
This addresses the finding. I did not run the tests. You are interacting with an AI system. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
13b3acb to
3ff7b53
Compare
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
3ff7b53 to
9a79b23
Compare
mahibi
left a comment
mahibi
left a comment
There was a problem hiding this comment.
I approve this PR as it solves the issue for the login.
However the real cause isn't in the login code. MainActivity is both the launcher entry point and singleTask, so a launcher tap clears every screen above it.
That affects the whole app: an open chat is also lost and the conversation list is shown instead.
I will work on a separate PR that will address the launch mode. Once it's merged, the solution in the current PR may no longer be needed and might be removed.
MainActivity is singleTask, so opening the app by the launcher icon while the browser login is pending clears BrowserLoginActivity and routes to the server selection. The login confirmed in the browser was lost. Keep the unfinished login in memory (20 min, the login flow v2 lifetime) and let MainActivity reopen BrowserLoginActivity in resume mode, which polls the saved response without a new login request or browser launch. Back and the cancel button drop the pending login. Assisted-by: Claude-Code:claude-opus-5-5 Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru>
Codacy flags three returns as a ReturnCount issue. Behaviour is unchanged. Assisted-by: Claude-Code:claude-opus-5-5 Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru>
After process death the in-memory pending login is gone, so the recreated BrowserLoginActivity in resume mode only showed an error. The resume intent now also carries the base URL, reauthorize flag and account, and the login is started again from them when there is nothing to continue. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
9a79b23 to
e00d4da
Compare
|
Thanks for the approval and for digging into the root cause. I'm not sure the launcher behavior is really a bug. Yes, it differs from most messengers, which bring back the last open chat. But it is also a useful side effect: if you minimize a conversation, for example to hide it from someone looking at your screen, a tap on the app icon then opens the conversation list, not the chat with that particular person. Returning to the chat is still possible from the recent apps screen. If the launch mode is changed, maybe this could be kept as an option, or at least considered as a trade-off. |
The expected behavior should be to have the same screen open as when the app was left. I began to work on the launch mode behavior changes in I tend to not merge the current branch so #9640 will solve the login issue by the way. |
The server hands out the app password of a login flow v2 only once. It was lost in two ways when the login screen was destroyed by the launcher icon (singleTask clear-top) while the browser was in front: - The blocking poll request is not interrupted by the cancellation of the view model, but withContext then throws and drops the answer. The next poll of the used token gets 404 forever. - A poll that finished while the app was in the background cleared the pending login before any screen collected the result. pollLogin now reports the confirmed login through onConfirmed before the cancellation can discard it. PendingBrowserLoginStore keeps it (memory only) and the login stays pending until BrowserLoginActivity has shown the result. A resumed view model finishes with the saved login instead of polling. Cancel still erases everything. Assisted-by: Claude Code:claude-sonnet-5-5 Signed-off-by: Krainov Gleb <g18a08k@gmail.com>
…d only its own entry, honour cancel Port of the develop change: the confirmed login has its own lifetime, the store is synchronized, a screen that showed its result ends only its own login, and a login confirmed after cancel is not completed. Assisted-by: Claude Code:claude-sonnet-5-5 Signed-off-by: Krainov Gleb <g18a08k@gmail.com>
|
While testing a build with #6811 on a device (develop with the merged #6811), I saw a case that is still lost: the login result itself. Steps (Login Flow v2, fresh login):
Talk stays on "Continue the login in the browser" and does not add the account. The scenario was run twice, with the same result; in one of the runs the server entry screen was shown instead of the waiting screen. A continuous login without the icon works. The cause below was found by reading the code and is covered by unit tests. The fix was not checked on a device. Cause (from the code):
Fix (memory only, no new mechanism):
Tests (ViewModel): the login confirmed after the 20 minutes is still finished; a waiting poll uses the login that a poll in flight confirmed; a screen showing its result does not end a newer login; the login confirmed while the first view model is cancelled, and the login nobody showed, are both finished by the next view model; a login confirmed after cancel is not kept; cancel erases a confirmed login. The fix is pushed to this PR as two commits on top of the previous head: 5be5616 and b8d4742. |
Symptom
Enter the server address, log in in the browser, then return to Talk by tapping the launcher icon. Talk shows the server address screen again and no account is created. Returning via Recents or Back works.
Cause
MainActivityissingleTask. A launch from the icon clears the activities above it, includingServerSelectionActivityandBrowserLoginActivitywith its ViewModel.MainActivity.handleIntentfinds no accounts and routes to the server address screen.pollUrlstarts only inBrowserLoginActivity. That activity is gone, so the login already granted in the browser is lost.This matches the analysis of @hsudbrock in #1512 (Pixel 6 Pro, Android 16): with the launcher icon,
/login/v2/pollis never requested.Change
PendingBrowserLoginStore: keeps the started login flow v2 response and the reauth flags in process memory for 20 minutes. Nothing is written to preferences or the database, because the response contains the poll token.BrowserLoginActivityViewModelsaves the response after the POST.resumeWebBrowserLogin()polls the savedpollUrlwithout a new POST and without opening the browser again. The pending login is cleared on completion, error, cancel, Back and expiry.MainActivity.handleIntent: when a pending login exists, it reopensBrowserLoginActivityin resume mode (KEY_RESUME_BROWSER_LOGIN). Below it in the back stack is the conversation list if accounts exist, otherwise the server address screen.singleTaskare unchanged.Limitation: if the process is killed while the browser is open, the pending login is lost and the user logs in again, as before.
How to test
Unit tests:
BrowserLoginActivityViewModelTest(resume without a newstartLoginFlow, reset after completion and cancel, an expired login is not resumed, retry through network loss, cancel and expiry during retries),LoginRepositoryTestandNetworkLoginDataSourceTest(retry after a failed request).Tested by the reporter on a Huawei DEL-LX9 at fork commit
addf79662, except network loss while waiting: that case failed there and is fixed by10258697a(step 6), which is covered by unit tests only so far and not yet tested on a device.Related
Refs #1512 (closed; the maintainer asked for a new issue for the launcher icon case). Related to #3295 and #6716 by symptom (the server address screen comes back after login); their cause is not confirmed.
🖼️ Screenshots
Not applicable.
🚧 TODO
🏁 Checklist
/backport to stable-xx.x🤖 AI (if applicable)
This change was prepared with Claude Code (claude-opus-5-5). The author reviewed the code and tested it on a device at
addf79662; the network loss fix (step 6) is not yet tested on a device. The commits carry theAssisted-by: Claude-Code:claude-opus-5-5trailer.