Skip to content

Login: Keep the mTLS certificate when handling the OAuth redirect - #205

Open
paolostivanin wants to merge 1 commit into
opencloud-eu:mainfrom
paolostivanin:fix-oauth-mtls-relogin
Open

Login: Keep the mTLS certificate when handling the OAuth redirect#205
paolostivanin wants to merge 1 commit into
opencloud-eu:mainfrom
paolostivanin:fix-oauth-mtls-relogin

Conversation

@paolostivanin

Copy link
Copy Markdown
Contributor

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 via onNewIntent.

That redirect intent carries no extras, so loginAction fell back to ACTION_CREATE and userAccount was null. restoreClientCertAlias() ran before restoreAuthState(), so it took the "fresh login, no certificate" branch and set clientManager.loginClientCertAlias = null. The recovery /status.php request therefore went out with no client certificate and the server answered with an HTML 403.

StatusRequester.handleRequestResult parsed the body as JSON before looking at the status code, so the resulting JSONException was mapped to INSTANCE_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

getServerInfoIsError only printed the message. It left pendingAuthorizationIntent armed with a now-dead single-use authorization code (so retrying produced a misleading "Unsuccessful authorization"), never called clearAuthState(), and left the url field empty. If the user then logged in fresh from that same screen, loginIsSuccess wrote KEY_MTLS_CERT_ALIAS = null onto the account, breaking every subsequent connection.

Changes

LoginActivity

  • 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 rather than loginAction. Every launch that passes EXTRA_ACCOUNT also passes a non-CREATE EXTRA_ACTION, so this is behaviour-preserving for the existing paths and only changes the recovered-redirect case.
  • Only overwrite the stored alias in loginIsSuccess when the user actually picked or removed a certificate on this screen (new clientCertAliasChangedByUser flag, persisted across configuration changes).
  • On a failed server check during the redirect leg, drop the dead authorization code, clear the auth state and refill the url field so the screen stays recoverable.
  • Report a 403 during login as a possible client certificate problem instead of the generic "Permission error", which is useless in this context.

StatusRequester / GetRemoteStatusOperation

  • Check the HTTP status before parsing the body as JSON.
  • Stop dereferencing result.data.baseUrl unconditionally: data is 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 through ClientManager.getClientForAnonymousCredentials, which applies loginClientCertAlias on both the new-client and reuse branches.

Tests

New StatusRequesterHandleResultTest (5 cases) guards the regression:

Response Expected
403 + HTML mTLS rejection page FORBIDDEN, httpCode 403
502 + HTML UNHANDLED_HTTP_CODE, httpCode 502
401 + empty body UNAUTHORIZED
200 + installed:false INSTANCE_NOT_CONFIGURED
200 + installed:true OK_SSL, base url preserved

assembleOriginalDebug builds and the app, domain, data and library unit test suites pass.

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".
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