From 3c597c717338db19f3c78f7d7b940939ffb8b965 Mon Sep 17 00:00:00 2001 From: Fahim Masud Choudhury Date: Thu, 30 Oct 2025 14:48:35 +0600 Subject: [PATCH 1/8] fix: synchronize token refresh to mitigate race condition synchronized{} is used to mitigate race conditions between multiple threads of Account Manager trying to refresh the token simultaneously in BearerAuthInterceptor and also Notes app (via AIDL) in OidcTokenRefresher . If there's a valid authState (and access token) available, it'll be used across all of the threads of Account Manager and Notes app. The fix is based on upstream's implementation. https://github.com/bitfireAT/davx5-ose/pull/1547 --- .../android/sso/OidcTokenRefresher.kt | 119 ++++++++++-------- .../davdroid/network/BearerAuthInterceptor.kt | 90 ++++++++----- .../at/bitfire/davdroid/network/HttpClient.kt | 10 +- 3 files changed, 137 insertions(+), 82 deletions(-) diff --git a/app/src/main/java/com/nextcloud/android/sso/OidcTokenRefresher.kt b/app/src/main/java/com/nextcloud/android/sso/OidcTokenRefresher.kt index 3b180bb91..c4f0fb4e1 100644 --- a/app/src/main/java/com/nextcloud/android/sso/OidcTokenRefresher.kt +++ b/app/src/main/java/com/nextcloud/android/sso/OidcTokenRefresher.kt @@ -19,21 +19,22 @@ package com.nextcloud.android.sso import android.accounts.Account +import android.accounts.AccountManager import android.content.Context +import at.bitfire.davdroid.BuildConfig import at.bitfire.davdroid.OpenIdUtils -import at.bitfire.davdroid.log.Logger import at.bitfire.davdroid.network.HttpClient.HttpClientEntryPoint import at.bitfire.davdroid.settings.AccountSettings +import at.bitfire.davdroid.settings.AccountSettings.Companion.KEY_AUTH_STATE +import at.bitfire.davdroid.util.setAndVerifyUserData import dagger.hilt.android.EntryPointAccessors import kotlinx.coroutines.runBlocking import net.openid.appauth.AuthState -import net.openid.appauth.AuthorizationService -import net.openid.appauth.ClientAuthentication import org.jetbrains.annotations.Blocking +import java.util.concurrent.CompletableFuture +import java.util.concurrent.CompletionException import java.util.logging.Level -import kotlin.coroutines.resume -import kotlin.coroutines.resumeWithException -import kotlin.coroutines.suspendCoroutine +import java.util.logging.Logger /** * Utility for refreshing OpenID Connect (OIDC) tokens in the Android AccountManager. @@ -44,8 +45,12 @@ import kotlin.coroutines.suspendCoroutine */ object OidcTokenRefresher { + val logger: Logger + get() = Logger.getGlobal() + /** - * Refreshes the OIDC token for the given [Account]. + * Refreshes the OIDC token for the given [Account]. Uses the current one if it's still valid, + * or requests a new one if necessary. * * It will: * 1. Invoke the authorization service to refresh tokens. @@ -54,65 +59,70 @@ object OidcTokenRefresher { * **Threading:** This method uses [runBlocking] and therefore must **not** be * called from the Main/UI thread. It is annotated with `@Blocking` to signal * blocking behavior. + * + * This method is synchronized / thread-safe so that it can be called for multiple HTTP requests + * at the same time. */ @JvmStatic @Blocking - fun refresh(context: Context, account: Account) { + fun refresh(context: Context, account: Account) = synchronized(javaClass) { runBlocking { val accountSettings = AccountSettings(context, account) val credentials = accountSettings.credentials() val authState = credentials.authState if (authState == null) { - Logger.log.log(Level.FINE, "Account: $account has null AuthState, refresh isn't possible.") + logger.log( + Level.SEVERE, "Account: $account has null AuthState, refresh isn't possible." + ) + return@runBlocking + } + + // Use cached authState if possible + if (authState.isAuthorized && authState.accessToken != null && !authState.needsTokenRefresh) { + if (BuildConfig.DEBUG) { + // log sensitive information (refresh/access token) only in debug builds + logger.log( + Level.FINEST, + "Account: $account is using cached AuthState: " + authState.jsonSerializeString() + ) + } return@runBlocking } + logger.fine("Account: $account is requesting fresh access token") + + val authStateFuture = CompletableFuture() + val authorizationService = EntryPointAccessors.fromApplication(context, HttpClientEntryPoint::class.java) .authorizationService() - val clientAuth = OpenIdUtils.getClientAuthentication(credentials.clientSecret) - val updatedAuthState = runCatching { - refreshAuthState(authorizationService, authState, clientAuth) - }.getOrNull() + val clientAuth = OpenIdUtils.getClientAuthentication(credentials.clientSecret) - if (updatedAuthState != null) { - updateAndroidAccountManagerAuthState(accountSettings, updatedAuthState) - } else { - Logger.log.warning("Couldn't update AuthState for account: $account") + val updatedAuthState = try { + authState.performActionWithFreshTokens( + authorizationService, clientAuth + ) { accessToken, _, authorizationException -> + when { + accessToken != null -> authStateFuture.complete(authState) + authorizationException != null -> authStateFuture.completeExceptionally( + authorizationException + ) + } + } + authStateFuture.join() + } catch (e: CompletionException) { + logger.log( + Level.SEVERE, "Couldn't obtain access token for account: $account", e.cause + ) + null + } finally { + authorizationService.dispose() } - } - } - /** - * Suspends until the authState has fresh tokens from AuthorizationService. - * - * Internally it bridges the callback-based `performActionWithFreshTokens` - * API into a coroutine suspension using [suspendCoroutine]. On success, it - * resumes with the same [AuthState] instance containing updated tokens. On - * failure, it throws the encountered [Throwable]. - * - * @param authService The [AuthorizationService] to use for token refresh. - * @param authState The current [AuthState] containing existing tokens. - * @param clientAuth [ClientAuthentication] mechanism (e.g., client secret). - * @return The same [AuthState] instance with refreshed tokens. - * @throws Exception if the refresh operation fails. - */ - private suspend fun refreshAuthState( - authService: AuthorizationService, authState: AuthState, clientAuth: ClientAuthentication - ): AuthState { - return suspendCoroutine { continuation -> - authState.performActionWithFreshTokens( - authService, - clientAuth - ) { accessToken, _, authorizationException -> - when { - accessToken != null -> continuation.resume(authState) - authorizationException != null -> continuation.resumeWithException( - authorizationException - ) - } + if (updatedAuthState != null) { + updateAndroidAccountManagerAuthState(context, accountSettings, updatedAuthState) } } } @@ -121,8 +131,15 @@ object OidcTokenRefresher { * Persists an updated [AuthState] back into the Android AccountManager. */ private fun updateAndroidAccountManagerAuthState( - accountSettings: AccountSettings, updatedAuthState: AuthState - ) = accountSettings.credentials( - accountSettings.credentials().copy(authState = updatedAuthState) - ) + context: Context, accountSettings: AccountSettings, authState: AuthState + ) { + accountSettings.credentials( + accountSettings.credentials().copy(authState = authState) + ) + + val accountManager = AccountManager.get(context) + accountManager.setAndVerifyUserData( + accountSettings.account, KEY_AUTH_STATE, authState.jsonSerializeString() + ) + } } diff --git a/app/src/main/kotlin/at/bitfire/davdroid/network/BearerAuthInterceptor.kt b/app/src/main/kotlin/at/bitfire/davdroid/network/BearerAuthInterceptor.kt index 40773e862..0018af571 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/network/BearerAuthInterceptor.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/network/BearerAuthInterceptor.kt @@ -4,9 +4,8 @@ package at.bitfire.davdroid.network -import at.bitfire.davdroid.log.Logger -import kotlinx.coroutines.CancellationException -import kotlinx.coroutines.CompletableDeferred +import android.accounts.Account +import at.bitfire.davdroid.BuildConfig import kotlinx.coroutines.runBlocking import net.openid.appauth.AuthState import net.openid.appauth.AuthorizationException @@ -14,49 +13,85 @@ import net.openid.appauth.AuthorizationService import net.openid.appauth.ClientAuthentication import okhttp3.Interceptor import okhttp3.Response +import java.util.concurrent.CompletableFuture +import java.util.concurrent.CompletionException import java.util.logging.Level +import java.util.logging.Logger /** * Sends an OAuth Bearer token authorization as described in RFC 6750. */ -class BearerAuthInterceptor( - private val accessToken: String -): Interceptor { - +class BearerAuthInterceptor(private val accessToken: String) : Interceptor { companion object { + val logger: Logger + get() = Logger.getGlobal() - fun fromAuthState(authService: AuthorizationService, authState: AuthState, clientAuth: ClientAuthentication, - callback: AuthStateUpdateCallback? = null): BearerAuthInterceptor? { - return runBlocking { - val accessTokenFuture = CompletableDeferred() + fun fromAuthState( + account: Account?, + authService: AuthorizationService, + authState: AuthState?, + clientAuth: ClientAuthentication, + callback: AuthStateUpdateCallback? = null + ): BearerAuthInterceptor? = synchronized(BearerAuthInterceptor::class.java) { + runBlocking { + if (authState == null) { + logger.severe("Account: $account has null AuthState, can't get access token") + return@runBlocking null + } - authState.performActionWithFreshTokens(authService, clientAuth) { accessToken: String?, _: String?, ex: AuthorizationException? -> - if (accessToken != null) { - // persist updated AuthState - callback?.onUpdate(authState) + // Use cached authState/accessToken if possible + val accessToken = authState.accessToken - // emit access token - accessTokenFuture.complete(accessToken) - } - else { - Logger.log.log(Level.WARNING, "Couldn't obtain access token", ex) - accessTokenFuture.cancel() + if (authState.isAuthorized && accessToken != null && !authState.needsTokenRefresh) { + if (BuildConfig.DEBUG) { + // log sensitive information (refresh/access token) only in debug builds + logger.log( + Level.FINEST, + "Account: $account is using cached access token, authState: ${authState.jsonSerializeString()}" + ) } + return@runBlocking BearerAuthInterceptor(accessToken) } - // return value + logger.fine("Account: $account is requesting fresh access token") + + val accessTokenFuture = CompletableFuture() try { - BearerAuthInterceptor(accessTokenFuture.await()) - } catch (ignored: CancellationException) { + authState.performActionWithFreshTokens( + authService, clientAuth + ) { token: String?, _: String?, ex: AuthorizationException? -> + if (token != null) { + if (BuildConfig.DEBUG) { + logger.log( + Level.FINEST, + "Got new AuthState for account: $account", + authState.jsonSerializeString() + ) + } + + // persist updated AuthState + callback?.onUpdate(authState) + + // emit access token + accessTokenFuture.complete(token) + } else { + accessTokenFuture.completeExceptionally(ex) + } + } + BearerAuthInterceptor(accessTokenFuture.join()) + } catch (e: CompletionException) { + logger.log( + Level.SEVERE, "Couldn't obtain access token for account: $account", e + ) null + } finally { + authService.dispose() } } } - } override fun intercept(chain: Interceptor.Chain): Response { - Logger.log.finer("Authenticating request with access token") val rq = chain.request().newBuilder() .header("Authorization", "Bearer $accessToken") .build() @@ -67,5 +102,4 @@ class BearerAuthInterceptor( fun interface AuthStateUpdateCallback { fun onUpdate(authState: AuthState) } - -} \ No newline at end of file +} diff --git a/app/src/main/kotlin/at/bitfire/davdroid/network/HttpClient.kt b/app/src/main/kotlin/at/bitfire/davdroid/network/HttpClient.kt index 114dc2527..5b2378c7c 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/network/HttpClient.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/network/HttpClient.kt @@ -223,9 +223,13 @@ class HttpClient private constructor( val newAuthService = EntryPointAccessors.fromApplication(context, HttpClientEntryPoint::class.java).authorizationService() authService = newAuthService val clientAuth = OpenIdUtils.getClientAuthentication(credentials.clientSecret) - BearerAuthInterceptor.fromAuthState(newAuthService, authState, clientAuth, authStateCallback)?.let { bearerAuthInterceptor -> - orig.addNetworkInterceptor(bearerAuthInterceptor) - } + BearerAuthInterceptor.fromAuthState( + account, + newAuthService, + authState, + clientAuth, + authStateCallback + )?.let { interceptor -> orig.addNetworkInterceptor(interceptor) } } val accountForCookie = account ?: AccountUtils.getAccount(context, credentials.userName, host) -- GitLab From 4db92634ec1bea9c863778cafed052baf95059cc Mon Sep 17 00:00:00 2001 From: Fahim Masud Choudhury Date: Fri, 31 Oct 2025 17:42:22 +0600 Subject: [PATCH 2/8] refactor: use OidcTokenRefresher as a single point of contact for refreshing OAuth tokens --- .../android/sso/InputStreamBinder.java | 15 +- .../android/sso/OidcTokenRefresher.kt | 178 +++++++++++------- .../davdroid/network/BearerAuthInterceptor.kt | 87 +++------ .../at/bitfire/davdroid/network/HttpClient.kt | 2 +- .../davdroid/token/MurenaTokenManager.kt | 96 +++++----- 5 files changed, 192 insertions(+), 186 deletions(-) diff --git a/app/src/main/java/com/nextcloud/android/sso/InputStreamBinder.java b/app/src/main/java/com/nextcloud/android/sso/InputStreamBinder.java index 5e7b80e3e..c75f9c3aa 100644 --- a/app/src/main/java/com/nextcloud/android/sso/InputStreamBinder.java +++ b/app/src/main/java/com/nextcloud/android/sso/InputStreamBinder.java @@ -47,8 +47,11 @@ import com.owncloud.android.lib.common.OwnCloudAccount; import com.owncloud.android.lib.common.OwnCloudClient; import com.owncloud.android.lib.common.OwnCloudClientManager; import com.owncloud.android.lib.common.OwnCloudClientManagerFactory; +import com.owncloud.android.lib.common.accounts.AccountUtils.AccountNotFoundException; import com.owncloud.android.lib.common.operations.RemoteOperation; +import net.openid.appauth.AuthorizationException; + import org.apache.commons.httpclient.HttpConnection; import org.apache.commons.httpclient.HttpMethodBase; import org.apache.commons.httpclient.HttpState; @@ -315,10 +318,9 @@ public class InputStreamBinder extends IInputStreamService.Stub { return method; } - private HttpMethodBase processRequest(final NextcloudRequest request, final InputStream requestBodyInputStream) - throws UnsupportedOperationException, - com.owncloud.android.lib.common.accounts.AccountUtils.AccountNotFoundException, - OperationCanceledException, AuthenticatorException, IOException { + private HttpMethodBase processRequest(final NextcloudRequest request, final InputStream requestBodyInputStream) throws + UnsupportedOperationException, AccountNotFoundException, OperationCanceledException, + AuthenticatorException, IOException, AuthorizationException { Account account = AccountManagerUtils.getAccountByName(context, request.getAccountName()); if (account == null) { throw new IllegalStateException(EXCEPTION_ACCOUNT_NOT_FOUND); @@ -409,9 +411,8 @@ public class InputStreamBinder extends IInputStreamService.Stub { } private Response processRequestV2(final NextcloudRequest request, final InputStream requestBodyInputStream) - throws UnsupportedOperationException, - com.owncloud.android.lib.common.accounts.AccountUtils.AccountNotFoundException, - OperationCanceledException, AuthenticatorException, IOException { + throws UnsupportedOperationException, AccountNotFoundException, OperationCanceledException, + AuthenticatorException, IOException, AuthorizationException { Account account = AccountManagerUtils.getAccountByName(context, request.getAccountName()); if (account == null) { throw new IllegalStateException(EXCEPTION_ACCOUNT_NOT_FOUND); diff --git a/app/src/main/java/com/nextcloud/android/sso/OidcTokenRefresher.kt b/app/src/main/java/com/nextcloud/android/sso/OidcTokenRefresher.kt index c4f0fb4e1..dda71dd20 100644 --- a/app/src/main/java/com/nextcloud/android/sso/OidcTokenRefresher.kt +++ b/app/src/main/java/com/nextcloud/android/sso/OidcTokenRefresher.kt @@ -19,80 +19,82 @@ package com.nextcloud.android.sso import android.accounts.Account -import android.accounts.AccountManager import android.content.Context import at.bitfire.davdroid.BuildConfig import at.bitfire.davdroid.OpenIdUtils +import at.bitfire.davdroid.db.Credentials import at.bitfire.davdroid.network.HttpClient.HttpClientEntryPoint import at.bitfire.davdroid.settings.AccountSettings -import at.bitfire.davdroid.settings.AccountSettings.Companion.KEY_AUTH_STATE -import at.bitfire.davdroid.util.setAndVerifyUserData import dagger.hilt.android.EntryPointAccessors import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.suspendCancellableCoroutine +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock import net.openid.appauth.AuthState +import net.openid.appauth.AuthorizationException +import net.openid.appauth.AuthorizationService +import net.openid.appauth.ClientAuthentication import org.jetbrains.annotations.Blocking -import java.util.concurrent.CompletableFuture -import java.util.concurrent.CompletionException import java.util.logging.Level import java.util.logging.Logger +import kotlin.coroutines.resume +import kotlin.coroutines.resumeWithException /** - * Utility for refreshing OpenID Connect (OIDC) tokens in the Android AccountManager. - * - * This object exposes a synchronous, blocking entry point for token refresh requests - * and internally uses coroutines to perform the refresh operation with proper - * callback-to-suspension conversion. + * Utility for refreshing OAuth tokens and updating them in the Android AccountManager. */ object OidcTokenRefresher { - - val logger: Logger - get() = Logger.getGlobal() + private val logger: Logger = Logger.getGlobal() + private val mutex = Mutex() /** - * Refreshes the OIDC token for the given [Account]. Uses the current one if it's still valid, + * Refreshes the OAuth tokens for the given [Account]. Uses the current one if it's still valid, * or requests a new one if necessary. * * It will: - * 1. Invoke the authorization service to refresh tokens. + * 1. Invoke the AppAuth library's authorization service to refresh tokens. * 2. Update AccountManager on successful refresh or log failures. * * **Threading:** This method uses [runBlocking] and therefore must **not** be * called from the Main/UI thread. It is annotated with `@Blocking` to signal * blocking behavior. * - * This method is synchronized / thread-safe so that it can be called for multiple HTTP requests + * This method is synchronized so that it can be called for multiple HTTP requests * at the same time. + * + * Returns an updated AuthState if token refresh is successful; + * Throws [AuthorizationException.TokenRequestErrors.INVALID_GRANT] for invalid grant or null otherwise. */ @JvmStatic @Blocking - fun refresh(context: Context, account: Account) = synchronized(javaClass) { - runBlocking { - val accountSettings = AccountSettings(context, account) - val credentials = accountSettings.credentials() - val authState = credentials.authState - - if (authState == null) { + @Throws(AuthorizationException::class) + fun refresh( + context: Context, + account: Account, + ): AuthState? { + return runBlocking { + // Check for Account Settings + val accountSettings = runCatching { + AccountSettings(context, account) + }.getOrElse { throwable -> logger.log( - Level.SEVERE, "Account: $account has null AuthState, refresh isn't possible." + Level.SEVERE, "Couldn't get AccountSettings for $account", throwable ) - return@runBlocking + return@runBlocking null } - // Use cached authState if possible - if (authState.isAuthorized && authState.accessToken != null && !authState.needsTokenRefresh) { - if (BuildConfig.DEBUG) { - // log sensitive information (refresh/access token) only in debug builds - logger.log( - Level.FINEST, - "Account: $account is using cached AuthState: " + authState.jsonSerializeString() - ) - } - return@runBlocking + // Check for AuthState + val credentials = accountSettings.credentials() + val authState = credentials.authState ?: run { + logger.severe("Missing AuthState for $account during token refresh.") + return@runBlocking null } - logger.fine("Account: $account is requesting fresh access token") - - val authStateFuture = CompletableFuture() + // Check for AuthorizationException + val authorizationException = authState.authorizationException + if (authorizationException != null && isInvalidGrant(authorizationException)) { + throw AuthorizationException.TokenRequestErrors.INVALID_GRANT + } val authorizationService = EntryPointAccessors.fromApplication(context, HttpClientEntryPoint::class.java) @@ -100,46 +102,84 @@ object OidcTokenRefresher { val clientAuth = OpenIdUtils.getClientAuthentication(credentials.clientSecret) - val updatedAuthState = try { - authState.performActionWithFreshTokens( - authorizationService, clientAuth - ) { accessToken, _, authorizationException -> - when { - accessToken != null -> authStateFuture.complete(authState) - authorizationException != null -> authStateFuture.completeExceptionally( - authorizationException - ) - } - } - authStateFuture.join() - } catch (e: CompletionException) { - logger.log( - Level.SEVERE, "Couldn't obtain access token for account: $account", e.cause - ) + // Refresh token using AppAuth library + val updatedAuthState: AuthState? = try { + refreshAuthState(account, authorizationService, authState, clientAuth) + } catch (e: Exception) { + logger.severe("Couldn't obtain access token, ${e.cause}") null } finally { authorizationService.dispose() } if (updatedAuthState != null) { - updateAndroidAccountManagerAuthState(context, accountSettings, updatedAuthState) + logger.info("Token refreshed for $account") + if (BuildConfig.DEBUG) { + logger.finest("Updated authState = ${updatedAuthState.jsonSerializeString()}") + } + writeAuthState(accountSettings, credentials, updatedAuthState) + updatedAuthState + } else { + null } } } - /** - * Persists an updated [AuthState] back into the Android AccountManager. - */ - private fun updateAndroidAccountManagerAuthState( - context: Context, accountSettings: AccountSettings, authState: AuthState - ) { - accountSettings.credentials( - accountSettings.credentials().copy(authState = authState) - ) - - val accountManager = AccountManager.get(context) - accountManager.setAndVerifyUserData( - accountSettings.account, KEY_AUTH_STATE, authState.jsonSerializeString() - ) + suspend fun refreshAuthState( + account: Account?, + authService: AuthorizationService, + authState: AuthState, + clientAuth: ClientAuthentication + ): AuthState = mutex.withLock { + // Use cached authState if possible + if (authState.isAuthorized && authState.accessToken != null && !authState.needsTokenRefresh) { + if (BuildConfig.DEBUG) { + logger.fine("$account is using cached AuthState: ${authState.jsonSerializeString()}") + } + return authState + } + + logger.fine("$account is requesting fresh access token") + + return suspendCancellableCoroutine { continuation -> + authState.performActionWithFreshTokens( + authService, clientAuth + ) { accessToken, _, authorizationException -> + when { + accessToken != null -> { + if (continuation.isActive) { + continuation.resume(authState) + } + } + + authorizationException != null -> { + if (continuation.isActive) { + continuation.resumeWithException(authorizationException) + } + } + + else -> { + if (continuation.isActive) { + continuation.resumeWithException(IllegalStateException("No token or exception")) + } + } + } + } + } } + + + // Checks whether the given AuthorizationException indicates an invalid grant (requires re-login). + fun isInvalidGrant(ex: AuthorizationException?): Boolean { + val invalidGrant = AuthorizationException.TokenRequestErrors.INVALID_GRANT + return ex?.code == invalidGrant.code && ex.error == invalidGrant.error + } + + private fun writeAuthState( + accountSettings: AccountSettings, + credentials: Credentials, + updatedAuthState: AuthState, + ) = + // Save updated authState to Android's Account Manager and SharedPreference + accountSettings.credentials(credentials.copy(authState = updatedAuthState)) } diff --git a/app/src/main/kotlin/at/bitfire/davdroid/network/BearerAuthInterceptor.kt b/app/src/main/kotlin/at/bitfire/davdroid/network/BearerAuthInterceptor.kt index 0018af571..05a310b7b 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/network/BearerAuthInterceptor.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/network/BearerAuthInterceptor.kt @@ -5,17 +5,14 @@ package at.bitfire.davdroid.network import android.accounts.Account -import at.bitfire.davdroid.BuildConfig +import com.nextcloud.android.sso.OidcTokenRefresher import kotlinx.coroutines.runBlocking import net.openid.appauth.AuthState -import net.openid.appauth.AuthorizationException import net.openid.appauth.AuthorizationService import net.openid.appauth.ClientAuthentication import okhttp3.Interceptor import okhttp3.Response -import java.util.concurrent.CompletableFuture -import java.util.concurrent.CompletionException -import java.util.logging.Level +import org.jetbrains.annotations.Blocking import java.util.logging.Logger /** @@ -26,67 +23,29 @@ class BearerAuthInterceptor(private val accessToken: String) : Interceptor { val logger: Logger get() = Logger.getGlobal() - fun fromAuthState( + @Blocking + fun createFromAuthState( account: Account?, - authService: AuthorizationService, - authState: AuthState?, - clientAuth: ClientAuthentication, + authorizationService: AuthorizationService, + authState: AuthState, + clientAuthentication: ClientAuthentication, callback: AuthStateUpdateCallback? = null - ): BearerAuthInterceptor? = synchronized(BearerAuthInterceptor::class.java) { - runBlocking { - if (authState == null) { - logger.severe("Account: $account has null AuthState, can't get access token") - return@runBlocking null - } - - // Use cached authState/accessToken if possible - val accessToken = authState.accessToken - - if (authState.isAuthorized && accessToken != null && !authState.needsTokenRefresh) { - if (BuildConfig.DEBUG) { - // log sensitive information (refresh/access token) only in debug builds - logger.log( - Level.FINEST, - "Account: $account is using cached access token, authState: ${authState.jsonSerializeString()}" - ) - } - return@runBlocking BearerAuthInterceptor(accessToken) - } - - logger.fine("Account: $account is requesting fresh access token") - - val accessTokenFuture = CompletableFuture() - try { - authState.performActionWithFreshTokens( - authService, clientAuth - ) { token: String?, _: String?, ex: AuthorizationException? -> - if (token != null) { - if (BuildConfig.DEBUG) { - logger.log( - Level.FINEST, - "Got new AuthState for account: $account", - authState.jsonSerializeString() - ) - } - - // persist updated AuthState - callback?.onUpdate(authState) - - // emit access token - accessTokenFuture.complete(token) - } else { - accessTokenFuture.completeExceptionally(ex) - } - } - BearerAuthInterceptor(accessTokenFuture.join()) - } catch (e: CompletionException) { - logger.log( - Level.SEVERE, "Couldn't obtain access token for account: $account", e - ) - null - } finally { - authService.dispose() - } + ): BearerAuthInterceptor? = runBlocking { + val updatedAuthState = runCatching { + OidcTokenRefresher.refreshAuthState( + account, + authorizationService, + authState, + clientAuthentication + ) + }.getOrNull() + val accessToken = updatedAuthState?.accessToken + if (updatedAuthState != null && accessToken != null) { + // Persist authState + callback?.onUpdate(updatedAuthState) + return@runBlocking BearerAuthInterceptor(accessToken) + } else { + return@runBlocking null } } } diff --git a/app/src/main/kotlin/at/bitfire/davdroid/network/HttpClient.kt b/app/src/main/kotlin/at/bitfire/davdroid/network/HttpClient.kt index 5b2378c7c..013118364 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/network/HttpClient.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/network/HttpClient.kt @@ -223,7 +223,7 @@ class HttpClient private constructor( val newAuthService = EntryPointAccessors.fromApplication(context, HttpClientEntryPoint::class.java).authorizationService() authService = newAuthService val clientAuth = OpenIdUtils.getClientAuthentication(credentials.clientSecret) - BearerAuthInterceptor.fromAuthState( + BearerAuthInterceptor.createFromAuthState( account, newAuthService, authState, diff --git a/app/src/main/kotlin/at/bitfire/davdroid/token/MurenaTokenManager.kt b/app/src/main/kotlin/at/bitfire/davdroid/token/MurenaTokenManager.kt index 301416f63..2751d7fe4 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/token/MurenaTokenManager.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/token/MurenaTokenManager.kt @@ -18,6 +18,7 @@ package at.bitfire.davdroid.token +import android.accounts.Account import android.accounts.AccountManager import android.annotation.SuppressLint import android.app.AlarmManager @@ -27,10 +28,9 @@ import android.content.Intent import at.bitfire.davdroid.BuildConfig import at.bitfire.davdroid.R import at.bitfire.davdroid.log.Logger -import at.bitfire.davdroid.network.HttpClient import at.bitfire.davdroid.settings.AccountSettings import at.bitfire.davdroid.ui.NetworkUtils -import dagger.hilt.android.EntryPointAccessors +import com.nextcloud.android.sso.OidcTokenRefresher import net.openid.appauth.AuthState import net.openid.appauth.AuthorizationException import java.text.SimpleDateFormat @@ -75,7 +75,7 @@ object MurenaTokenManager { // Schedules a refresh before the token expires, or refreshes immediately if already expired. fun handleTokenRefresh(context: Context, onComplete: ((AuthState?) -> Unit)? = null) { - val credentials = getAccountSettings(context)?.credentials() ?: run { + val credentials = getMurenaAccountSettings(context)?.credentials() ?: run { Logger.log.warning("No account credentials found, cannot schedule refresh.") return } @@ -134,12 +134,7 @@ object MurenaTokenManager { // Refreshes the authentication token and updates stored credentials if successful. private fun refreshAuthToken(context: Context, onComplete: ((AuthState?) -> Unit)? = null) { try { - val httpEntryPoint = EntryPointAccessors.fromApplication( - context, HttpClient.HttpClientEntryPoint::class.java - ) - - val authService = httpEntryPoint.authorizationService() - val accountSettings = getAccountSettings(context) ?: run { + val accountSettings = getMurenaAccountSettings(context) ?: run { Logger.log.warning("No account settings found during token refresh.") return } @@ -150,67 +145,78 @@ object MurenaTokenManager { return } - if (isInvalidGrant(authState.authorizationException)) { + if (OidcTokenRefresher.isInvalidGrant(authState.authorizationException)) { Logger.log.warning("Invalid grant detected, user re-auth required.") cancelTokenRefreshAlarm(context) return } - val tokenRequest = authState.createTokenRefreshRequest() - authService.performTokenRequest(tokenRequest) { response, exception -> - when { - response != null && exception == null -> { - authState.update(response, null) - accountSettings.credentials(credentials.copy(authState = authState)) - Logger.log.info("Token refreshed for ${accountSettings.account.name}") - - // Schedule at least 2 minutes early for the new token. - val refreshAt = authState.accessTokenExpirationTime?.minus(2.minutes.inWholeMilliseconds) - if (refreshAt != null) { - setTokenRefreshAlarm(context, refreshAt) - } - - onComplete?.invoke(authState) - } + val murenaAccount = getMurenaAccount(context) ?: run { + Logger.log.warning("No Murena account found for token refresh.") + return + } - isInvalidGrant(exception) -> { - Logger.log.log(Level.SEVERE, "Invalid grant: refresh cancelled, User must re-authenticate.", exception) + // Refresh token + val updatedAuthState = + runCatching { + OidcTokenRefresher.refresh( + context, + murenaAccount + ) + }.getOrElse { throwable -> + if (throwable is AuthorizationException && + OidcTokenRefresher.isInvalidGrant(throwable) + ) { + Logger.log.severe(throwable.message) cancelTokenRefreshAlarm(context) + } else { + Logger.log.log( + Level.SEVERE, + "Token refresh failed: $throwable, retrying in 5 minutes." + ) + setTokenRefreshAlarm( + context, System.currentTimeMillis() + 5.minutes.inWholeMilliseconds + ) } + null + } - else -> { - Logger.log.log(Level.SEVERE, "Token refresh failed: unknown error, retrying in 5 minutes.") - setTokenRefreshAlarm(context, System.currentTimeMillis() + 5.minutes.inWholeMilliseconds) - } + if (updatedAuthState != null) { + val refreshAt = + updatedAuthState.accessTokenExpirationTime?.minus(2.minutes.inWholeMilliseconds) + if (refreshAt != null) { + setTokenRefreshAlarm(context, refreshAt) } + onComplete?.invoke(updatedAuthState) + } else { + Logger.log.severe("Couldn't refresh token, authState update returned null.") + onComplete?.invoke(null) } } catch (e: Exception) { Logger.log.log(Level.SEVERE, "Token refresh failed due to unexpected exception.", e) - } finally { onComplete?.invoke(null) } } - // Checks whether the given AuthorizationException indicates an invalid grant (requires re-login). - private fun isInvalidGrant(ex: AuthorizationException?): Boolean { - val invalidGrant = AuthorizationException.TokenRequestErrors.INVALID_GRANT - return ex?.code == invalidGrant.code && ex.error == invalidGrant.error - } - // Retrieves the Murena account settings for the currently active account, if available. // We only allow one murena account. - fun getAccountSettings(context: Context): AccountSettings? { - val accountType = context.getString(R.string.eelo_account_type) - val account = AccountManager.get(context) - .getAccountsByType(accountType) - .firstOrNull() - + private fun getMurenaAccountSettings(context: Context): AccountSettings? { + val account = getMurenaAccount(context) return account?.let { AccountSettings(context, it) } ?: run { Logger.log.info("No Murena account found.") null } } + private fun getMurenaAccount(context: Context): Account? { + val accountType = context.getString(R.string.eelo_account_type) + val account = AccountManager.get(context) + .getAccountsByType(accountType) + .firstOrNull() + return account + } + + private fun Long.asDateString(): String = SimpleDateFormat("yyyy-MM-dd HH:mm:ss", Locale.getDefault()).format(Date(this)) } -- GitLab From e97404e6f37f9aaad02a6c4e2a53ae5f6d0e347c Mon Sep 17 00:00:00 2001 From: Fahim Masud Choudhury Date: Mon, 3 Nov 2025 15:09:22 +0600 Subject: [PATCH 3/8] refactor: revert MurenaTokenManager changes --- .../davdroid/token/MurenaTokenManager.kt | 96 +++++++++---------- 1 file changed, 45 insertions(+), 51 deletions(-) diff --git a/app/src/main/kotlin/at/bitfire/davdroid/token/MurenaTokenManager.kt b/app/src/main/kotlin/at/bitfire/davdroid/token/MurenaTokenManager.kt index 2751d7fe4..301416f63 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/token/MurenaTokenManager.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/token/MurenaTokenManager.kt @@ -18,7 +18,6 @@ package at.bitfire.davdroid.token -import android.accounts.Account import android.accounts.AccountManager import android.annotation.SuppressLint import android.app.AlarmManager @@ -28,9 +27,10 @@ import android.content.Intent import at.bitfire.davdroid.BuildConfig import at.bitfire.davdroid.R import at.bitfire.davdroid.log.Logger +import at.bitfire.davdroid.network.HttpClient import at.bitfire.davdroid.settings.AccountSettings import at.bitfire.davdroid.ui.NetworkUtils -import com.nextcloud.android.sso.OidcTokenRefresher +import dagger.hilt.android.EntryPointAccessors import net.openid.appauth.AuthState import net.openid.appauth.AuthorizationException import java.text.SimpleDateFormat @@ -75,7 +75,7 @@ object MurenaTokenManager { // Schedules a refresh before the token expires, or refreshes immediately if already expired. fun handleTokenRefresh(context: Context, onComplete: ((AuthState?) -> Unit)? = null) { - val credentials = getMurenaAccountSettings(context)?.credentials() ?: run { + val credentials = getAccountSettings(context)?.credentials() ?: run { Logger.log.warning("No account credentials found, cannot schedule refresh.") return } @@ -134,7 +134,12 @@ object MurenaTokenManager { // Refreshes the authentication token and updates stored credentials if successful. private fun refreshAuthToken(context: Context, onComplete: ((AuthState?) -> Unit)? = null) { try { - val accountSettings = getMurenaAccountSettings(context) ?: run { + val httpEntryPoint = EntryPointAccessors.fromApplication( + context, HttpClient.HttpClientEntryPoint::class.java + ) + + val authService = httpEntryPoint.authorizationService() + val accountSettings = getAccountSettings(context) ?: run { Logger.log.warning("No account settings found during token refresh.") return } @@ -145,77 +150,66 @@ object MurenaTokenManager { return } - if (OidcTokenRefresher.isInvalidGrant(authState.authorizationException)) { + if (isInvalidGrant(authState.authorizationException)) { Logger.log.warning("Invalid grant detected, user re-auth required.") cancelTokenRefreshAlarm(context) return } - val murenaAccount = getMurenaAccount(context) ?: run { - Logger.log.warning("No Murena account found for token refresh.") - return - } + val tokenRequest = authState.createTokenRefreshRequest() + authService.performTokenRequest(tokenRequest) { response, exception -> + when { + response != null && exception == null -> { + authState.update(response, null) + accountSettings.credentials(credentials.copy(authState = authState)) + Logger.log.info("Token refreshed for ${accountSettings.account.name}") + + // Schedule at least 2 minutes early for the new token. + val refreshAt = authState.accessTokenExpirationTime?.minus(2.minutes.inWholeMilliseconds) + if (refreshAt != null) { + setTokenRefreshAlarm(context, refreshAt) + } + + onComplete?.invoke(authState) + } - // Refresh token - val updatedAuthState = - runCatching { - OidcTokenRefresher.refresh( - context, - murenaAccount - ) - }.getOrElse { throwable -> - if (throwable is AuthorizationException && - OidcTokenRefresher.isInvalidGrant(throwable) - ) { - Logger.log.severe(throwable.message) + isInvalidGrant(exception) -> { + Logger.log.log(Level.SEVERE, "Invalid grant: refresh cancelled, User must re-authenticate.", exception) cancelTokenRefreshAlarm(context) - } else { - Logger.log.log( - Level.SEVERE, - "Token refresh failed: $throwable, retrying in 5 minutes." - ) - setTokenRefreshAlarm( - context, System.currentTimeMillis() + 5.minutes.inWholeMilliseconds - ) } - null - } - if (updatedAuthState != null) { - val refreshAt = - updatedAuthState.accessTokenExpirationTime?.minus(2.minutes.inWholeMilliseconds) - if (refreshAt != null) { - setTokenRefreshAlarm(context, refreshAt) + else -> { + Logger.log.log(Level.SEVERE, "Token refresh failed: unknown error, retrying in 5 minutes.") + setTokenRefreshAlarm(context, System.currentTimeMillis() + 5.minutes.inWholeMilliseconds) + } } - onComplete?.invoke(updatedAuthState) - } else { - Logger.log.severe("Couldn't refresh token, authState update returned null.") - onComplete?.invoke(null) } } catch (e: Exception) { Logger.log.log(Level.SEVERE, "Token refresh failed due to unexpected exception.", e) + } finally { onComplete?.invoke(null) } } - // Retrieves the Murena account settings for the currently active account, if available. - // We only allow one murena account. - private fun getMurenaAccountSettings(context: Context): AccountSettings? { - val account = getMurenaAccount(context) - return account?.let { AccountSettings(context, it) } ?: run { - Logger.log.info("No Murena account found.") - null - } + // Checks whether the given AuthorizationException indicates an invalid grant (requires re-login). + private fun isInvalidGrant(ex: AuthorizationException?): Boolean { + val invalidGrant = AuthorizationException.TokenRequestErrors.INVALID_GRANT + return ex?.code == invalidGrant.code && ex.error == invalidGrant.error } - private fun getMurenaAccount(context: Context): Account? { + // Retrieves the Murena account settings for the currently active account, if available. + // We only allow one murena account. + fun getAccountSettings(context: Context): AccountSettings? { val accountType = context.getString(R.string.eelo_account_type) val account = AccountManager.get(context) .getAccountsByType(accountType) .firstOrNull() - return account - } + return account?.let { AccountSettings(context, it) } ?: run { + Logger.log.info("No Murena account found.") + null + } + } private fun Long.asDateString(): String = SimpleDateFormat("yyyy-MM-dd HH:mm:ss", Locale.getDefault()).format(Date(this)) -- GitLab From 6dce43cd935979f0dae016c4d941089ab09ca6e3 Mon Sep 17 00:00:00 2001 From: Fahim Masud Choudhury Date: Mon, 3 Nov 2025 16:26:23 +0600 Subject: [PATCH 4/8] refactor: use synchronized instead of Mutex --- .../android/sso/OidcTokenRefresher.kt | 80 ++++++++++--------- .../davdroid/network/BearerAuthInterceptor.kt | 14 ++-- 2 files changed, 47 insertions(+), 47 deletions(-) diff --git a/app/src/main/java/com/nextcloud/android/sso/OidcTokenRefresher.kt b/app/src/main/java/com/nextcloud/android/sso/OidcTokenRefresher.kt index dda71dd20..3d016d8b3 100644 --- a/app/src/main/java/com/nextcloud/android/sso/OidcTokenRefresher.kt +++ b/app/src/main/java/com/nextcloud/android/sso/OidcTokenRefresher.kt @@ -27,18 +27,16 @@ import at.bitfire.davdroid.network.HttpClient.HttpClientEntryPoint import at.bitfire.davdroid.settings.AccountSettings import dagger.hilt.android.EntryPointAccessors import kotlinx.coroutines.runBlocking -import kotlinx.coroutines.suspendCancellableCoroutine import kotlinx.coroutines.sync.Mutex -import kotlinx.coroutines.sync.withLock import net.openid.appauth.AuthState import net.openid.appauth.AuthorizationException import net.openid.appauth.AuthorizationService import net.openid.appauth.ClientAuthentication import org.jetbrains.annotations.Blocking +import java.util.concurrent.CompletableFuture +import java.util.concurrent.CompletionException import java.util.logging.Level import java.util.logging.Logger -import kotlin.coroutines.resume -import kotlin.coroutines.resumeWithException /** * Utility for refreshing OAuth tokens and updating them in the Android AccountManager. @@ -59,9 +57,6 @@ object OidcTokenRefresher { * called from the Main/UI thread. It is annotated with `@Blocking` to signal * blocking behavior. * - * This method is synchronized so that it can be called for multiple HTTP requests - * at the same time. - * * Returns an updated AuthState if token refresh is successful; * Throws [AuthorizationException.TokenRequestErrors.INVALID_GRANT] for invalid grant or null otherwise. */ @@ -102,21 +97,10 @@ object OidcTokenRefresher { val clientAuth = OpenIdUtils.getClientAuthentication(credentials.clientSecret) - // Refresh token using AppAuth library - val updatedAuthState: AuthState? = try { + val updatedAuthState: AuthState? = refreshAuthState(account, authorizationService, authState, clientAuth) - } catch (e: Exception) { - logger.severe("Couldn't obtain access token, ${e.cause}") - null - } finally { - authorizationService.dispose() - } if (updatedAuthState != null) { - logger.info("Token refreshed for $account") - if (BuildConfig.DEBUG) { - logger.finest("Updated authState = ${updatedAuthState.jsonSerializeString()}") - } writeAuthState(accountSettings, credentials, updatedAuthState) updatedAuthState } else { @@ -125,52 +109,70 @@ object OidcTokenRefresher { } } - suspend fun refreshAuthState( + /** + * Refreshes the current AuthState and updates it. Uses the current one if it's still valid, + * or requests a new one if necessary. + * + * It will: + * 1. Invoke the AppAuth library's authorization service to refresh tokens. + * 2. Update AccountManager on successful refresh or log failures. + * + * **Threading:** This method uses [runBlocking] and therefore must **not** be + * called from the Main/UI thread. It is annotated with `@Blocking` to signal + * blocking behavior. + * + * Returns an updated AuthState if token refresh is successful; + * Throws [AuthorizationException.TokenRequestErrors.INVALID_GRANT] for invalid grant or null otherwise. + */ + fun refreshAuthState( account: Account?, authService: AuthorizationService, authState: AuthState, clientAuth: ClientAuthentication - ): AuthState = mutex.withLock { + ): AuthState? = synchronized(javaClass) { // Use cached authState if possible if (authState.isAuthorized && authState.accessToken != null && !authState.needsTokenRefresh) { if (BuildConfig.DEBUG) { - logger.fine("$account is using cached AuthState: ${authState.jsonSerializeString()}") + logger.finest("$account is using cached AuthState: ${authState.jsonSerializeString()}") } return authState } - logger.fine("$account is requesting fresh access token") + logger.info("$account is requesting fresh access token") + if (BuildConfig.DEBUG) { + logger.finest("AuthState before update = ${authState.jsonSerializeString()}") + } + val authStateFuture = CompletableFuture() - return suspendCancellableCoroutine { continuation -> + return@synchronized try { authState.performActionWithFreshTokens( authService, clientAuth - ) { accessToken, _, authorizationException -> + ) { accessToken, _, exception -> when { accessToken != null -> { - if (continuation.isActive) { - continuation.resume(authState) - } - } - - authorizationException != null -> { - if (continuation.isActive) { - continuation.resumeWithException(authorizationException) + logger.info("Token refreshed for $account") + if (BuildConfig.DEBUG) { + logger.finest("Updated authState = ${authState.jsonSerializeString()}") } + authStateFuture.complete(authState) } - else -> { - if (continuation.isActive) { - continuation.resumeWithException(IllegalStateException("No token or exception")) - } + exception != null -> { + authStateFuture.completeExceptionally(exception) } } } + authStateFuture.join() + } catch (e: CompletionException) { + logger.log(Level.SEVERE, "Couldn't obtain access token", e) + null + } finally { + authService.dispose() } } - // Checks whether the given AuthorizationException indicates an invalid grant (requires re-login). - fun isInvalidGrant(ex: AuthorizationException?): Boolean { + private fun isInvalidGrant(ex: AuthorizationException?): Boolean { val invalidGrant = AuthorizationException.TokenRequestErrors.INVALID_GRANT return ex?.code == invalidGrant.code && ex.error == invalidGrant.error } diff --git a/app/src/main/kotlin/at/bitfire/davdroid/network/BearerAuthInterceptor.kt b/app/src/main/kotlin/at/bitfire/davdroid/network/BearerAuthInterceptor.kt index 05a310b7b..22e511ac9 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/network/BearerAuthInterceptor.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/network/BearerAuthInterceptor.kt @@ -31,14 +31,12 @@ class BearerAuthInterceptor(private val accessToken: String) : Interceptor { clientAuthentication: ClientAuthentication, callback: AuthStateUpdateCallback? = null ): BearerAuthInterceptor? = runBlocking { - val updatedAuthState = runCatching { - OidcTokenRefresher.refreshAuthState( - account, - authorizationService, - authState, - clientAuthentication - ) - }.getOrNull() + val updatedAuthState = OidcTokenRefresher.refreshAuthState( + account, + authorizationService, + authState, + clientAuthentication + ) val accessToken = updatedAuthState?.accessToken if (updatedAuthState != null && accessToken != null) { // Persist authState -- GitLab From e614fe4870414badc909681a5a7e58b5fe66502a Mon Sep 17 00:00:00 2001 From: Fahim Masud Choudhury Date: Mon, 3 Nov 2025 17:48:51 +0600 Subject: [PATCH 5/8] refactor: use single API to update tokens --- .../android/sso/InputStreamBinder.java | 40 ++++++- .../android/sso/OidcTokenRefresher.kt | 100 ++++-------------- .../android/utils/AccountManagerUtils.java | 22 +++- .../kotlin/at/bitfire/davdroid/OpenIdUtils.kt | 1 + .../davdroid/network/BearerAuthInterceptor.kt | 11 +- .../at/bitfire/davdroid/network/HttpClient.kt | 2 +- .../davdroid/util/AuthStatePrefUtils.kt | 1 + 7 files changed, 87 insertions(+), 90 deletions(-) diff --git a/app/src/main/java/com/nextcloud/android/sso/InputStreamBinder.java b/app/src/main/java/com/nextcloud/android/sso/InputStreamBinder.java index c75f9c3aa..94a951136 100644 --- a/app/src/main/java/com/nextcloud/android/sso/InputStreamBinder.java +++ b/app/src/main/java/com/nextcloud/android/sso/InputStreamBinder.java @@ -24,6 +24,7 @@ import static com.nextcloud.android.sso.Constants.EXCEPTION_INVALID_REQUEST_URL; import static com.nextcloud.android.sso.Constants.EXCEPTION_INVALID_TOKEN; import static com.nextcloud.android.sso.Constants.EXCEPTION_UNSUPPORTED_METHOD; import static com.nextcloud.android.sso.Constants.SSO_SHARED_PREFERENCE; +import static at.bitfire.davdroid.settings.AccountSettings.KEY_AUTH_STATE; import android.accounts.Account; import android.accounts.AuthenticatorException; @@ -50,7 +51,9 @@ import com.owncloud.android.lib.common.OwnCloudClientManagerFactory; import com.owncloud.android.lib.common.accounts.AccountUtils.AccountNotFoundException; import com.owncloud.android.lib.common.operations.RemoteOperation; +import net.openid.appauth.AuthState; import net.openid.appauth.AuthorizationException; +import net.openid.appauth.ClientAuthentication; import org.apache.commons.httpclient.HttpConnection; import org.apache.commons.httpclient.HttpMethodBase; @@ -83,7 +86,14 @@ import java.util.List; import java.util.Map; import java.util.logging.Level; +import at.bitfire.davdroid.OpenIdUtils; +import at.bitfire.davdroid.db.Credentials; import at.bitfire.davdroid.log.Logger; +import at.bitfire.davdroid.settings.AccountSettings; +import at.bitfire.davdroid.util.AuthStatePrefUtils; +import kotlin.Unit; +import kotlin.jvm.functions.Function0; +import kotlin.jvm.functions.Function1; public class InputStreamBinder extends IInputStreamService.Stub { private static final String CONTENT_TYPE_APPLICATION_JSON = "application/json"; @@ -337,8 +347,7 @@ public class InputStreamBinder extends IInputStreamService.Stub { } if (AccountManagerUtils.isOidcAccount(context, account)) { - // Blocking call - OidcTokenRefresher.refresh(context, account); + refreshAuth(account); } final OwnCloudClientManager ownCloudClientManager = OwnCloudClientManagerFactory.getDefaultSingleton(); @@ -430,8 +439,7 @@ public class InputStreamBinder extends IInputStreamService.Stub { } if (AccountManagerUtils.isOidcAccount(context, account)) { - // Blocking call - OidcTokenRefresher.refresh(context, account); + refreshAuth(account); } final OwnCloudClientManager ownCloudClientManager = OwnCloudClientManagerFactory.getDefaultSingleton(); @@ -487,6 +495,30 @@ public class InputStreamBinder extends IInputStreamService.Stub { } } + private void refreshAuth(Account account) throws AuthorizationException { + AccountSettings accountSettings = new AccountSettings(context, account); + Credentials credentials = accountSettings.credentials(); + + Function0 getClientAuth = () -> { + try { + return OpenIdUtils.getClientAuthentication(credentials.getClientSecret()); + } catch (Exception e) { + return null; + } + }; + + Function0 readAuthState = credentials::getAuthState; + + Function1 writeAuthState = authState -> { + AccountManagerUtils.setAndVerifyUserData(context, account, KEY_AUTH_STATE, authState.jsonSerializeString()); + AuthStatePrefUtils.saveAuthState(context, account, authState.jsonSerializeString()); + return Unit.INSTANCE; + }; + + // Blocking call + OidcTokenRefresher.refreshAuthState(context, account, getClientAuth, readAuthState, writeAuthState); + } + private boolean isValid(NextcloudRequest request) { String callingPackageName = context.getPackageManager().getNameForUid(Binder.getCallingUid()); diff --git a/app/src/main/java/com/nextcloud/android/sso/OidcTokenRefresher.kt b/app/src/main/java/com/nextcloud/android/sso/OidcTokenRefresher.kt index 3d016d8b3..803733f5f 100644 --- a/app/src/main/java/com/nextcloud/android/sso/OidcTokenRefresher.kt +++ b/app/src/main/java/com/nextcloud/android/sso/OidcTokenRefresher.kt @@ -21,16 +21,11 @@ package com.nextcloud.android.sso import android.accounts.Account import android.content.Context import at.bitfire.davdroid.BuildConfig -import at.bitfire.davdroid.OpenIdUtils -import at.bitfire.davdroid.db.Credentials import at.bitfire.davdroid.network.HttpClient.HttpClientEntryPoint -import at.bitfire.davdroid.settings.AccountSettings import dagger.hilt.android.EntryPointAccessors import kotlinx.coroutines.runBlocking -import kotlinx.coroutines.sync.Mutex import net.openid.appauth.AuthState import net.openid.appauth.AuthorizationException -import net.openid.appauth.AuthorizationService import net.openid.appauth.ClientAuthentication import org.jetbrains.annotations.Blocking import java.util.concurrent.CompletableFuture @@ -43,10 +38,9 @@ import java.util.logging.Logger */ object OidcTokenRefresher { private val logger: Logger = Logger.getGlobal() - private val mutex = Mutex() /** - * Refreshes the OAuth tokens for the given [Account]. Uses the current one if it's still valid, + * Refreshes the current AuthState and updates it. Uses the current one if it's still valid, * or requests a new one if necessary. * * It will: @@ -57,79 +51,24 @@ object OidcTokenRefresher { * called from the Main/UI thread. It is annotated with `@Blocking` to signal * blocking behavior. * + * This method is synchronized / thread-safe so that it can be called for + * multiple HTTP requests at the same time. + * * Returns an updated AuthState if token refresh is successful; * Throws [AuthorizationException.TokenRequestErrors.INVALID_GRANT] for invalid grant or null otherwise. */ + @JvmStatic @Blocking @Throws(AuthorizationException::class) - fun refresh( - context: Context, - account: Account, - ): AuthState? { - return runBlocking { - // Check for Account Settings - val accountSettings = runCatching { - AccountSettings(context, account) - }.getOrElse { throwable -> - logger.log( - Level.SEVERE, "Couldn't get AccountSettings for $account", throwable - ) - return@runBlocking null - } - - // Check for AuthState - val credentials = accountSettings.credentials() - val authState = credentials.authState ?: run { - logger.severe("Missing AuthState for $account during token refresh.") - return@runBlocking null - } - - // Check for AuthorizationException - val authorizationException = authState.authorizationException - if (authorizationException != null && isInvalidGrant(authorizationException)) { - throw AuthorizationException.TokenRequestErrors.INVALID_GRANT - } - - val authorizationService = - EntryPointAccessors.fromApplication(context, HttpClientEntryPoint::class.java) - .authorizationService() - - val clientAuth = OpenIdUtils.getClientAuthentication(credentials.clientSecret) - - val updatedAuthState: AuthState? = - refreshAuthState(account, authorizationService, authState, clientAuth) - - if (updatedAuthState != null) { - writeAuthState(accountSettings, credentials, updatedAuthState) - updatedAuthState - } else { - null - } - } - } - - /** - * Refreshes the current AuthState and updates it. Uses the current one if it's still valid, - * or requests a new one if necessary. - * - * It will: - * 1. Invoke the AppAuth library's authorization service to refresh tokens. - * 2. Update AccountManager on successful refresh or log failures. - * - * **Threading:** This method uses [runBlocking] and therefore must **not** be - * called from the Main/UI thread. It is annotated with `@Blocking` to signal - * blocking behavior. - * - * Returns an updated AuthState if token refresh is successful; - * Throws [AuthorizationException.TokenRequestErrors.INVALID_GRANT] for invalid grant or null otherwise. - */ fun refreshAuthState( + context: Context, account: Account?, - authService: AuthorizationService, - authState: AuthState, - clientAuth: ClientAuthentication + getClientAuth: () -> ClientAuthentication?, + readAuthState: () -> AuthState?, + writeAuthState: (AuthState) -> Unit ): AuthState? = synchronized(javaClass) { + val authState = readAuthState() ?: return null // Use cached authState if possible if (authState.isAuthorized && authState.accessToken != null && !authState.needsTokenRefresh) { if (BuildConfig.DEBUG) { @@ -138,16 +77,27 @@ object OidcTokenRefresher { return authState } + // Check for AuthorizationException + val authorizationException = authState.authorizationException + if (authorizationException != null && isInvalidGrant(authorizationException)) { + throw AuthorizationException.TokenRequestErrors.INVALID_GRANT + } + logger.info("$account is requesting fresh access token") if (BuildConfig.DEBUG) { logger.finest("AuthState before update = ${authState.jsonSerializeString()}") } + val clientAuth = getClientAuth() ?: return null + val authService = + EntryPointAccessors.fromApplication(context, HttpClientEntryPoint::class.java) + .authorizationService() val authStateFuture = CompletableFuture() return@synchronized try { authState.performActionWithFreshTokens( authService, clientAuth ) { accessToken, _, exception -> + writeAuthState(authState) when { accessToken != null -> { logger.info("Token refreshed for $account") @@ -176,12 +126,4 @@ object OidcTokenRefresher { val invalidGrant = AuthorizationException.TokenRequestErrors.INVALID_GRANT return ex?.code == invalidGrant.code && ex.error == invalidGrant.error } - - private fun writeAuthState( - accountSettings: AccountSettings, - credentials: Credentials, - updatedAuthState: AuthState, - ) = - // Save updated authState to Android's Account Manager and SharedPreference - accountSettings.credentials(credentials.copy(authState = updatedAuthState)) } diff --git a/app/src/main/java/com/nextcloud/android/utils/AccountManagerUtils.java b/app/src/main/java/com/nextcloud/android/utils/AccountManagerUtils.java index faf5ff7a1..7d4ac7b90 100644 --- a/app/src/main/java/com/nextcloud/android/utils/AccountManagerUtils.java +++ b/app/src/main/java/com/nextcloud/android/utils/AccountManagerUtils.java @@ -23,7 +23,9 @@ import android.content.Context; import androidx.annotation.NonNull; import androidx.annotation.Nullable; -import at.bitfire.davdroid.Constants; +import java.util.Objects; +import java.util.logging.Logger; + import at.bitfire.davdroid.R; import at.bitfire.davdroid.settings.AccountSettings; @@ -62,4 +64,22 @@ public final class AccountManagerUtils { return context.getString(R.string.eelo_account_type); } + // Java equivalent of AccountManager.setAndVerifyUserData() extension function from CompatUtils.kt + public static void setAndVerifyUserData(@NonNull Context context, @NonNull Account account, + @NonNull String key, @Nullable String value) { + AccountManager accountManager = AccountManager.get(context); + for (int i = 1; i <= 10; i++) { + accountManager.setUserData(account, key, value); + if (Objects.equals(accountManager.getUserData(account, key), value)) { + return; + } + try { + Thread.sleep(100); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + } + Logger.getLogger("AccountManagerExtensions").warning("AccountManager failed to set " + account + " user data " + key + " := " + value); + } + } diff --git a/app/src/main/kotlin/at/bitfire/davdroid/OpenIdUtils.kt b/app/src/main/kotlin/at/bitfire/davdroid/OpenIdUtils.kt index f2f9e0de9..0225c08cb 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/OpenIdUtils.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/OpenIdUtils.kt @@ -22,6 +22,7 @@ import net.openid.appauth.NoClientAuthentication object OpenIdUtils { + @JvmStatic fun getClientAuthentication(secret: String?): ClientAuthentication { if (secret == null) { return NoClientAuthentication.INSTANCE diff --git a/app/src/main/kotlin/at/bitfire/davdroid/network/BearerAuthInterceptor.kt b/app/src/main/kotlin/at/bitfire/davdroid/network/BearerAuthInterceptor.kt index 22e511ac9..8bcaaea78 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/network/BearerAuthInterceptor.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/network/BearerAuthInterceptor.kt @@ -5,10 +5,10 @@ package at.bitfire.davdroid.network import android.accounts.Account +import android.content.Context import com.nextcloud.android.sso.OidcTokenRefresher import kotlinx.coroutines.runBlocking import net.openid.appauth.AuthState -import net.openid.appauth.AuthorizationService import net.openid.appauth.ClientAuthentication import okhttp3.Interceptor import okhttp3.Response @@ -25,17 +25,18 @@ class BearerAuthInterceptor(private val accessToken: String) : Interceptor { @Blocking fun createFromAuthState( + context: Context, account: Account?, - authorizationService: AuthorizationService, authState: AuthState, clientAuthentication: ClientAuthentication, callback: AuthStateUpdateCallback? = null ): BearerAuthInterceptor? = runBlocking { val updatedAuthState = OidcTokenRefresher.refreshAuthState( + context, account, - authorizationService, - authState, - clientAuthentication + getClientAuth = { clientAuthentication }, + readAuthState = { authState }, + writeAuthState = { callback?.onUpdate(it) } ) val accessToken = updatedAuthState?.accessToken if (updatedAuthState != null && accessToken != null) { diff --git a/app/src/main/kotlin/at/bitfire/davdroid/network/HttpClient.kt b/app/src/main/kotlin/at/bitfire/davdroid/network/HttpClient.kt index 013118364..521728805 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/network/HttpClient.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/network/HttpClient.kt @@ -224,8 +224,8 @@ class HttpClient private constructor( authService = newAuthService val clientAuth = OpenIdUtils.getClientAuthentication(credentials.clientSecret) BearerAuthInterceptor.createFromAuthState( + context, account, - newAuthService, authState, clientAuth, authStateCallback diff --git a/app/src/main/kotlin/at/bitfire/davdroid/util/AuthStatePrefUtils.kt b/app/src/main/kotlin/at/bitfire/davdroid/util/AuthStatePrefUtils.kt index 19ad813d7..f8c5f26ce 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/util/AuthStatePrefUtils.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/util/AuthStatePrefUtils.kt @@ -24,6 +24,7 @@ object AuthStatePrefUtils { private const val AUTH_STATE_SHARED_PREF = "authStateShared_Pref" + @JvmStatic fun saveAuthState(context: Context, account: Account, value: String?) { val preferences = getSharedPref(context) preferences.edit() -- GitLab From c3f5cf9aaab21075dc7356349e604dd5df75f205 Mon Sep 17 00:00:00 2001 From: Fahim Masud Choudhury Date: Mon, 3 Nov 2025 18:22:22 +0600 Subject: [PATCH 6/8] refactor: use setAndVerifyUserData() method from generated Java code --- .../android/sso/InputStreamBinder.java | 5 ++++- .../android/utils/AccountManagerUtils.java | 22 ------------------- 2 files changed, 4 insertions(+), 23 deletions(-) diff --git a/app/src/main/java/com/nextcloud/android/sso/InputStreamBinder.java b/app/src/main/java/com/nextcloud/android/sso/InputStreamBinder.java index 94a951136..1661e8ddd 100644 --- a/app/src/main/java/com/nextcloud/android/sso/InputStreamBinder.java +++ b/app/src/main/java/com/nextcloud/android/sso/InputStreamBinder.java @@ -27,6 +27,7 @@ import static com.nextcloud.android.sso.Constants.SSO_SHARED_PREFERENCE; import static at.bitfire.davdroid.settings.AccountSettings.KEY_AUTH_STATE; import android.accounts.Account; +import android.accounts.AccountManager; import android.accounts.AuthenticatorException; import android.accounts.OperationCanceledException; import android.content.Context; @@ -91,6 +92,7 @@ import at.bitfire.davdroid.db.Credentials; import at.bitfire.davdroid.log.Logger; import at.bitfire.davdroid.settings.AccountSettings; import at.bitfire.davdroid.util.AuthStatePrefUtils; +import at.bitfire.davdroid.util.CompatUtilsKt; import kotlin.Unit; import kotlin.jvm.functions.Function0; import kotlin.jvm.functions.Function1; @@ -510,7 +512,8 @@ public class InputStreamBinder extends IInputStreamService.Stub { Function0 readAuthState = credentials::getAuthState; Function1 writeAuthState = authState -> { - AccountManagerUtils.setAndVerifyUserData(context, account, KEY_AUTH_STATE, authState.jsonSerializeString()); + AccountManager accountManager = AccountManager.get(context); + CompatUtilsKt.setAndVerifyUserData(accountManager, account, KEY_AUTH_STATE, authState.jsonSerializeString()); AuthStatePrefUtils.saveAuthState(context, account, authState.jsonSerializeString()); return Unit.INSTANCE; }; diff --git a/app/src/main/java/com/nextcloud/android/utils/AccountManagerUtils.java b/app/src/main/java/com/nextcloud/android/utils/AccountManagerUtils.java index 7d4ac7b90..9c5c99b13 100644 --- a/app/src/main/java/com/nextcloud/android/utils/AccountManagerUtils.java +++ b/app/src/main/java/com/nextcloud/android/utils/AccountManagerUtils.java @@ -23,9 +23,6 @@ import android.content.Context; import androidx.annotation.NonNull; import androidx.annotation.Nullable; -import java.util.Objects; -import java.util.logging.Logger; - import at.bitfire.davdroid.R; import at.bitfire.davdroid.settings.AccountSettings; @@ -63,23 +60,4 @@ public final class AccountManagerUtils { public static String getAccountType(@NonNull Context context) { return context.getString(R.string.eelo_account_type); } - - // Java equivalent of AccountManager.setAndVerifyUserData() extension function from CompatUtils.kt - public static void setAndVerifyUserData(@NonNull Context context, @NonNull Account account, - @NonNull String key, @Nullable String value) { - AccountManager accountManager = AccountManager.get(context); - for (int i = 1; i <= 10; i++) { - accountManager.setUserData(account, key, value); - if (Objects.equals(accountManager.getUserData(account, key), value)) { - return; - } - try { - Thread.sleep(100); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - } - } - Logger.getLogger("AccountManagerExtensions").warning("AccountManager failed to set " + account + " user data " + key + " := " + value); - } - } -- GitLab From e9508e7be577be262e1ee4b3106b636195cb4469 Mon Sep 17 00:00:00 2001 From: Fahim Masud Choudhury Date: Mon, 3 Nov 2025 18:56:05 +0600 Subject: [PATCH 7/8] refactor: improve exception handling and null annotation --- .../com/nextcloud/android/sso/InputStreamBinder.java | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/com/nextcloud/android/sso/InputStreamBinder.java b/app/src/main/java/com/nextcloud/android/sso/InputStreamBinder.java index 1661e8ddd..ae53cff6a 100644 --- a/app/src/main/java/com/nextcloud/android/sso/InputStreamBinder.java +++ b/app/src/main/java/com/nextcloud/android/sso/InputStreamBinder.java @@ -497,8 +497,13 @@ public class InputStreamBinder extends IInputStreamService.Stub { } } - private void refreshAuth(Account account) throws AuthorizationException { - AccountSettings accountSettings = new AccountSettings(context, account); + private void refreshAuth(@NonNull Account account) throws AuthorizationException { + AccountSettings accountSettings; + try { + accountSettings = new AccountSettings(context, account); + } catch (IllegalArgumentException e) { + throw new IllegalStateException(e.getMessage()); + } Credentials credentials = accountSettings.credentials(); Function0 getClientAuth = () -> { -- GitLab From 97316afb062bb1821ab92691623132f9497fc926 Mon Sep 17 00:00:00 2001 From: Fahim Masud Choudhury Date: Mon, 3 Nov 2025 19:27:44 +0600 Subject: [PATCH 8/8] refactor: remove unnecessary exception handling --- .../com/nextcloud/android/sso/InputStreamBinder.java | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/app/src/main/java/com/nextcloud/android/sso/InputStreamBinder.java b/app/src/main/java/com/nextcloud/android/sso/InputStreamBinder.java index ae53cff6a..2c8bad6e9 100644 --- a/app/src/main/java/com/nextcloud/android/sso/InputStreamBinder.java +++ b/app/src/main/java/com/nextcloud/android/sso/InputStreamBinder.java @@ -504,15 +504,10 @@ public class InputStreamBinder extends IInputStreamService.Stub { } catch (IllegalArgumentException e) { throw new IllegalStateException(e.getMessage()); } - Credentials credentials = accountSettings.credentials(); - Function0 getClientAuth = () -> { - try { - return OpenIdUtils.getClientAuthentication(credentials.getClientSecret()); - } catch (Exception e) { - return null; - } - }; + Credentials credentials = accountSettings.credentials(); + Function0 getClientAuth = + () -> OpenIdUtils.getClientAuthentication(credentials.getClientSecret()); Function0 readAuthState = credentials::getAuthState; -- GitLab