Login: Keep the mTLS certificate when handling the OAuth redirect - #205
Open
paolostivanin wants to merge 1 commit into
Open
Login: Keep the mTLS certificate when handling the OAuth redirect#205paolostivanin wants to merge 1 commit into
paolostivanin wants to merge 1 commit into
Conversation
If Android kills the app process while the OAuth Custom Tab is in the foreground, the browser redirect is handled by a brand new LoginActivity (isTaskRoot == true). That intent carries no extras, so loginAction fell back to ACTION_CREATE and userAccount was null. restoreClientCertAlias() ran before restoreAuthState(), took the "fresh login" branch and reset clientManager.loginClientCertAlias to null, so the recovery /status.php request went out with no client certificate. On an mTLS-protected host (for instance behind Cloudflare) that comes back as an HTML 403. StatusRequester parsed the body as JSON before looking at the status code, so the JSONException was mapped to INSTANCE_NOT_CONFIGURED and the login screen reported "Malformed server configuration" instead of the real HTTP error. The screen was also left unrecoverable: the dead, single-use authorization code stayed armed, the auth state was never cleared and the url field stayed empty. Logging in fresh from there then wrote a null KEY_MTLS_CERT_ALIAS onto the account, breaking every later connection and leaving a reinstall as the only way out. - Restore the persisted auth state at the top of onCreate on the redirect leg, before anything downstream reads loginAction or userAccount. - Key restoreClientCertAlias() on userAccount instead of loginAction. - Only overwrite the stored alias on login when the user actually picked or removed a certificate on this screen. - On a failed server check during the redirect leg, drop the dead authorization code, clear the auth state and refill the url field. - Check the HTTP status before parsing the status body as JSON, and stop dereferencing the (success-only) data of a failed result. - Report a 403 during login as a possible client certificate problem rather than the generic "Permission error".
Contributor
|
Ouch, I need to have a look at this. FYI: detekt fail ^ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Re-authentication fails with "Malformed server configuration" on the login screen when the server requires a client certificate. Fresh login is unaffected, which is what makes this confusing. The account is then left unusable and reinstalling the app is the only way out.
Reproduced against a host behind Cloudflare mTLS, but it applies to any mTLS-protected instance.
Root cause
When Android kills the app process while the OAuth Custom Tab is in the foreground, the browser redirect is handled by a brand new
LoginActivity(isTaskRoot == true) rather than being forwarded to a live one viaonNewIntent.That redirect intent carries no extras, so
loginActionfell back toACTION_CREATEanduserAccountwas null.restoreClientCertAlias()ran beforerestoreAuthState(), so it took the "fresh login, no certificate" branch and setclientManager.loginClientCertAlias = null. The recovery/status.phprequest therefore went out with no client certificate and the server answered with an HTML 403.StatusRequester.handleRequestResultparsed the body as JSON before looking at the status code, so the resultingJSONExceptionwas mapped toINSTANCE_NOT_CONFIGURED, surfacing as "Malformed server configuration" instead of the real HTTP error. The status-code branch below it was effectively dead code for any non-JSON body (proxy error pages, captive portals, empty bodies).Why it needed a reinstall
getServerInfoIsErroronly printed the message. It leftpendingAuthorizationIntentarmed with a now-dead single-use authorization code (so retrying produced a misleading "Unsuccessful authorization"), never calledclearAuthState(), and left the url field empty. If the user then logged in fresh from that same screen,loginIsSuccesswroteKEY_MTLS_CERT_ALIAS = nullonto the account, breaking every subsequent connection.Changes
LoginActivityonCreateon the redirect leg, before anything downstream readsloginActionoruserAccount.restoreClientCertAlias()onuserAccountrather thanloginAction. Every launch that passesEXTRA_ACCOUNTalso passes a non-CREATEEXTRA_ACTION, so this is behaviour-preserving for the existing paths and only changes the recovered-redirect case.loginIsSuccesswhen the user actually picked or removed a certificate on this screen (newclientCertAliasChangedByUserflag, persisted across configuration changes).StatusRequester/GetRemoteStatusOperationresult.data.baseUrlunconditionally:datais only set on success, so every failed status check turned into an opaque NPE result instead of the actual HTTP error.The fix covers the whole recovery chain, not just
/status.php: OIDC discovery, the token exchange and client registration all go throughClientManager.getClientForAnonymousCredentials, which appliesloginClientCertAliason both the new-client and reuse branches.Tests
New
StatusRequesterHandleResultTest(5 cases) guards the regression:FORBIDDEN, httpCode 403UNHANDLED_HTTP_CODE, httpCode 502UNAUTHORIZEDinstalled:falseINSTANCE_NOT_CONFIGUREDinstalled:trueOK_SSL, base url preservedassembleOriginalDebugbuilds and the app, domain, data and library unit test suites pass.