-
Notifications
You must be signed in to change notification settings - Fork 10
Fix: Critical login state and authentication issues #60
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: main
Are you sure you want to change the base?
Conversation
dc63785 to
6b37d75
Compare
|
Notice: I utilized Gemini 3 Pro to accelerate the development of this PR. Therefore, I consider a single-person review (2-eye principle) insufficient. I strictly prefer a 4-eye principle review by another contributor to ensure correctness. |
cf23ce7 to
3cfa0d8
Compare
|
Looks fine in general 👍 I will give it a test |
| SingleSessionManager.setUserAgent(userAgent) | ||
|
|
||
| // initialise thumbnails cache on background thread | ||
| ThumbnailsCacheManager.InitDiskCacheTask().execute() |
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.
@zerox80 Why is this line here?
If this is a glitch from something else, can you please re-base this PR so we see if it's still there?
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.
its not glitch from something else, i always rebase all PRs after u merge something. What do u mean ? im confused
|
New app installation, fails immediatly after initial launch (I can still accept that the app sends me notifications) Might also be fishy that I see |
|
yes i solved that super stuff in other PR xD |
|
will focus this PR soon, i am cooking stuff with conflict management while syncing. |
|
OK, yes, rebase because I think all the stuff in ic_action_open_shortcut.xml is also not relevant herew :-) |
…various service instances.
… prevent login failure after process death
…omTabsIntent for Firefox compatibility - Add setIntent() in onNewIntent to properly handle OAuth redirects - Add logging for OAuth redirect debugging
bfbce96 to
5586f7e
Compare
|
@zerox80 Thanks for fixes, it seems to work now. Tried adding an account and switching away to another app as soon as the browser opens. Then switch back to app. I wonnder, I see this one in debug output: But I do not see this message. Am I supposed to see it? Am I testing this the right way? Or does this just mean my activity was not taken out of memory? |
The message "OAuth redirect detected with code or error parameter" only appears when the Activity is launched/recreated via an OAuth redirect (i.e., with ?code= or ?error= parameter in the intent data). What you're seeing is completely correct - your Activity was still in memory and simply resumed when you switched back to the app. The debug output shows intent data: null, which confirms no OAuth redirect triggered the restart. The primary use case this fix addresses is exactly what you tested: switching to a Password Manager (or another app) while the browser is open. Before this fix, if Android decided to kill the app (which happens frequently on devices with aggressive memory management), users would return to an empty URL field and have to start over. The fact that you didn't see the specific log message just means your device had enough memory to keep the Activity alive this time. But the fix is now in place to handle the cases where it doesn't! If you want to specifically test the edge case, you can enable Developer Options -> "Don't keep activities" - but if login worked successfully for you, everything is working as intended! |
Fix: Critical Login State Loss & Authentication Issues
📋 Overview
This PR resolves critical issues in the login process, specifically regarding OAuth authentication (OIDC). The primary issue was that the
LoginActivitystate (and thus crucial authentication parameters) was lost when the user switched to the browser view (Custom Tabs) and returned. This resulted in login failures or app crashes/restarts, particularly on devices with aggressive memory management.🐛 The Problem
When the app opens the browser for OAuth login,
LoginActivityis pushed to the background. Android may terminate the app process at this point to reclaim memory.When the user returns after logging in:
LoginActivityis recreated (onCreate).codeVerifier,codeChallenge,serverBaseUrl) are gone.🛠 The Solution
The solution consists of three main components:
codeVerifier,codeChallenge,oidcState) are now explicitly saved toSharedPreferencesbefore the browser is opened. This is more robust thanonSaveInstanceStateas it survives complete process restarts.onCreateandonSaveInstanceState, the UI state (URL input, Auth method) is now correctly saved and restored.LoginActivityfrom being placed on the stack when the browser returns. Instead, the existing instance is reused.🔍 Detailed Code Analysis
1.
AuthenticationViewModel.kt- Mutability for RestoreWe need to make the OAuth parameters mutable so we can overwrite them when restoring the old state from storage.
2.
LoginActivity.kt- Intent & Task ManagementWhen the browser returns with the Auth Code, we don't want Android to layer a new
LoginActivityover the old one. We want to reactivate the old one.3.
LoginActivity.kt- Saving & Loading Auth StateHere lies the magic of persistence. We don't just rely on RAM.
Saving before Browser Launch:
Restoring on Start (
onCreate):Cleanup on Success:
4.
LoginActivity.kt- UI State RestorationWe ensure that the UI (input fields, button visibility) is correctly restored so the user isn't faced with an empty screen.
✅ Verification / Testing