Skip to content

Token refresh: Do not report transient failures as expired token #237

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.

Sign up for GitHub

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

Open
paolostivanin wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Token refresh: Do not report transient failures as expired token #237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 68 additions & 14 deletions .../src/main/java/eu/opencloud/android/presentation/authentication/AccountAuthenticator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@
import eu.opencloud.android.domain.authentication.oauth.model.OIDCServerConfiguration;
import eu.opencloud.android.domain.authentication.oauth.model.TokenRequest;
import eu.opencloud.android.domain.authentication.oauth.model.TokenResponse;
import eu.opencloud.android.domain.exceptions.OAuth2ErrorAccessDeniedException;
import eu.opencloud.android.domain.exceptions.OAuth2ErrorException;
import eu.opencloud.android.domain.exceptions.UnauthorizedException;
import eu.opencloud.android.lib.common.accounts.AccountTypeUtils;
import eu.opencloud.android.lib.common.accounts.AccountUtils;
import kotlin.Lazy;
Expand Down Expand Up @@ -74,6 +77,13 @@ public class AccountAuthenticator extends AbstractAccountAuthenticator {
private static final String KEY_REQUIRED_FEATURES = "requiredFeatures";
public static final String KEY_ACCOUNT = "account";

/**
* Serializes the refresh of access tokens. Refresh tokens can be single-use (rotation), so two refreshes
* running at once with the same refresh token make the second one fail with invalid_grant and may even
* revoke the whole session at the IdP.
*/
private static final Object REFRESH_TOKEN_LOCK = new Object();

private Context mContext;

AccountAuthenticator(Context context) {
Expand Down Expand Up @@ -152,7 +162,7 @@ public Bundle editProperties(AccountAuthenticatorResponse response,
*/
@Override
public Bundle getAuthToken(AccountAuthenticatorResponse accountAuthenticatorResponse,
Account account, String authTokenType, Bundle options) {
Account account, String authTokenType, Bundle options) throws NetworkErrorException {
/// validate parameters
try {
validateAccountType(account.type);
Expand All @@ -174,7 +184,15 @@ public Bundle getAuthToken(AccountAuthenticatorResponse accountAuthenticatorResp
// this account, null will be returned
accessToken = accountManager.peekAuthToken(account, authTokenType);
if (accessToken == null && canBeRefreshed(authTokenType) && clientSecretIsValid(accountManager, account)) {
accessToken = refreshToken(account, authTokenType, accountManager);
synchronized (REFRESH_TOKEN_LOCK) {
// Another thread may have refreshed the token while this one was waiting for the lock
accessToken = accountManager.peekAuthToken(account, authTokenType);
if (accessToken == null) {
// Throws NetworkErrorException if the failure is transient, so the caller does not mistake
// it for an expired session
accessToken = refreshToken(account, authTokenType, accountManager);
}
}
}
}

Expand Down Expand Up @@ -315,11 +333,19 @@ private boolean canBeRefreshed(String authTokenType) {
getAccountType())));
}

/**
* Exchanges the stored refresh token for a new access token.
*
* @return the new access token, or null if the session is definitely over (no refresh token stored, or the IdP
* rejected it) and the user needs to log in again.
* @throws NetworkErrorException if the refresh failed for a transient reason (no network, timeout, server
* error...). The stored tokens are untouched and a later attempt may succeed.
*/
private String refreshToken(
Account account,
String authTokenType,
AccountManager accountManager
) {
) throws NetworkErrorException {

// Prepare everything to perform the token request
String refreshToken = accountManager.getUserData(account, KEY_OAUTH2_REFRESH_TOKEN);
Expand All @@ -329,18 +355,21 @@ private String refreshToken(
return null;
}

Timber.d("Ready to exchange for new tokens. Account: [ %s ], Refresh token: [ %s ]", account.name,
refreshToken);
Timber.d("Ready to exchange for new tokens. Account: [ %s ]", account.name);

String baseUrl = accountManager.getUserData(account, AccountUtils.Constants.KEY_OC_BASE_URL);

// Servers requiring mTLS reject discovery and token requests that do not present the account's certificate
String clientCertAlias = AccountUtils.getClientCertAliasForAccount(mContext, account);

// OIDC Discovery: prefer the stored webfinger issuer (points to the real IDP),
// fall back to baseUrl for accounts created before webfinger support.
String oidcIssuer = accountManager.getUserData(account, KEY_OIDC_ISSUER);
String discoveryUrl = (oidcIssuer != null) ? oidcIssuer : baseUrl;

@NotNull Lazy<OIDCDiscoveryUseCase> oidcDiscoveryUseCase = inject(OIDCDiscoveryUseCase.class);
OIDCDiscoveryUseCase.Params oidcDiscoveryUseCaseParams = new OIDCDiscoveryUseCase.Params(discoveryUrl);
OIDCDiscoveryUseCase.Params oidcDiscoveryUseCaseParams =
new OIDCDiscoveryUseCase.Params(discoveryUrl, clientCertAlias);
UseCaseResult<OIDCServerConfiguration> oidcServerConfigurationUseCaseResult =
oidcDiscoveryUseCase.getValue().invoke(oidcDiscoveryUseCaseParams);

Expand Down Expand Up @@ -385,8 +414,14 @@ private String refreshToken(
clientAuth = OAuthUtils.Companion.getClientAuth(clientSecret, clientId);
}
} else {
Timber.d("OIDC Discovery failed. Server discovery info: [ %s ]",
oidcServerConfigurationUseCaseResult.getThrowableOrNull().toString());
Throwable discoveryError = oidcServerConfigurationUseCaseResult.getThrowableOrNull();
Timber.d("OIDC Discovery failed. Server discovery info: [ %s ]", String.valueOf(discoveryError));

if (oidcIssuer != null) {
// The IdP is known, so the legacy ownCloud endpoint below can not be the right one: the failure is
// just the IdP being unreachable for now
throw new NetworkErrorException("OIDC discovery failed for " + oidcIssuer, discoveryError);
}

tokenEndpoint = baseUrl + File.separator + mContext.getString(R.string.oauth2_url_endpoint_access);
clientAuth = OAuthUtils.Companion.getClientAuth(clientSecret, clientId);
Expand All @@ -404,7 +439,8 @@ private String refreshToken(
scope,
clientIdForRequest,
clientSecretForRequest,
refreshToken
refreshToken,
clientCertAlias
);

// Token exchange
Expand All @@ -416,10 +452,26 @@ private String refreshToken(
if (safeTokenResponse != null) {
return handleSuccessfulRefreshToken(safeTokenResponse,
account, authTokenType, accountManager, refreshToken);
} else {
Timber.e(tokenResponseResult.getThrowableOrNull(), "OAuth request to refresh access token failed. Preparing to access Login Activity");
}

Throwable error = tokenResponseResult.getThrowableOrNull();
if (isRefreshTokenRejected(error)) {
Timber.e(error, "OAuth refresh token was rejected. Preparing to access Login Activity");
return null;
}

Timber.w(error, "OAuth request to refresh access token failed, but the refresh token was not rejected");
throw new NetworkErrorException("Could not refresh the access token for " + account.name, error);
}

/**
* Tells whether a failed token request means the refresh token (or the client) is not valid anymore, as opposed
* to a transient problem like no connection, a timeout or a server error.
*/
static boolean isRefreshTokenRejected(Throwable error) {
return error instanceof OAuth2ErrorException
|| error instanceof OAuth2ErrorAccessDeniedException
|| error instanceof UnauthorizedException;
}

private String handleSuccessfulRefreshToken(
Expand All @@ -430,18 +482,20 @@ private String handleSuccessfulRefreshToken(
String oldRefreshToken
) {
String newAccessToken = tokenResponse.getAccessToken();
accountManager.setAuthToken(account, authTokenType, newAccessToken);

String refreshTokenToUseFromNowOn;
if (tokenResponse.getRefreshToken() != null) {
refreshTokenToUseFromNowOn = tokenResponse.getRefreshToken();
} else {
refreshTokenToUseFromNowOn = oldRefreshToken;
}

// The refresh token goes first: it may have been rotated, which makes the old one useless. If the process
// dies before the access token is stored, the next call finds no access token and refreshes with it.
accountManager.setUserData(account, KEY_OAUTH2_REFRESH_TOKEN, refreshTokenToUseFromNowOn);
accountManager.setAuthToken(account, authTokenType, newAccessToken);

Timber.d("Token refreshed successfully. New access token: [ %s ]. New refresh token: [ %s ]",
newAccessToken, refreshTokenToUseFromNowOn);
Timber.d("Token refreshed successfully for account [ %s ]", account.name);

return newAccessToken;
}
Expand Down
28 changes: 8 additions & 20 deletions opencloudApp/src/main/java/eu/opencloud/android/ui/activity/FileDisplayActivity.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ import eu.opencloud.android.extensions.SNACKBAR_DURATION_ERROR_MS
import eu.opencloud.android.extensions.showErrorInSnackbar
import eu.opencloud.android.extensions.showMessageInSnackbar
import eu.opencloud.android.lib.common.accounts.AccountUtils
import eu.opencloud.android.lib.common.authentication.OpenCloudBearerCredentials
import eu.opencloud.android.lib.common.network.CertificateCombinedException
import eu.opencloud.android.lib.common.operations.RemoteOperationResult
import eu.opencloud.android.lib.common.operations.RemoteOperationResult.ResultCode
Expand Down Expand Up @@ -1413,16 +1412,10 @@ class FileDisplayActivity : FileActivity(),
}

is UnauthorizedException -> {
launch(Dispatchers.IO) {
val credentials = AccountUtils.getCredentialsForAccount(MainApp.appContext, account)

launch(Dispatchers.Main) {
if (credentials is OpenCloudBearerCredentials) { // OAuth
showRequestRegainAccess()
} else {
showRequestAccountChangeNotice(getString(R.string.auth_failure_snackbar), false)
}
}
if (AccountUtils.isOAuth2Account(MainApp.appContext, account)) {
showRequestRegainAccess()
} else {
showRequestAccountChangeNotice(getString(R.string.auth_failure_snackbar), false)
}
}

Expand Down Expand Up @@ -1522,15 +1515,10 @@ class FileDisplayActivity : FileActivity(),
is UIResult.Error -> {
when (uiResult.error) {
is UnauthorizedException -> {
launch(Dispatchers.IO) {
val credentials = AccountUtils.getCredentialsForAccount(MainApp.appContext, account)
launch(Dispatchers.Main) {
if (credentials is OpenCloudBearerCredentials) { // OAuth
showRequestRegainAccess()
} else {
showRequestAccountChangeNotice(getString(R.string.auth_failure_snackbar), false)
}
}
if (AccountUtils.isOAuth2Account(MainApp.appContext, account)) {
showRequestRegainAccess()
} else {
showRequestAccountChangeNotice(getString(R.string.auth_failure_snackbar), false)
}
}

Expand Down
61 changes: 61 additions & 0 deletions ...rc/test/java/eu/opencloud/android/presentation/authentication/AccountAuthenticatorTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/**
* openCloud Android client application
*
* Copyright (C) 2026 OpenCloud GmbH.
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2,
* as published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

package eu.opencloud.android.presentation.authentication

import eu.opencloud.android.domain.exceptions.NoConnectionWithServerException
import eu.opencloud.android.domain.exceptions.OAuth2ErrorAccessDeniedException
import eu.opencloud.android.domain.exceptions.OAuth2ErrorException
import eu.opencloud.android.domain.exceptions.ServerConnectionTimeoutException
import eu.opencloud.android.domain.exceptions.ServerNotReachableException
import eu.opencloud.android.domain.exceptions.ServiceUnavailableException
import eu.opencloud.android.domain.exceptions.UnauthorizedException
import eu.opencloud.android.domain.exceptions.UnhandledHttpCodeException
import org.junit.Assert.assertFalse
import org.junit.Assert.assertTrue
import org.junit.Test

/**
* Whether a failed token refresh is "the session is over" or "try again later" decides if the user is sent to the
* login screen. Only a refusal by the IdP may do that.
*/
class AccountAuthenticatorTest {

@Test
fun `refresh token rejected by the IdP is a permanent failure`() {
assertTrue(AccountAuthenticator.isRefreshTokenRejected(OAuth2ErrorException()))
assertTrue(AccountAuthenticator.isRefreshTokenRejected(OAuth2ErrorAccessDeniedException()))
assertTrue(AccountAuthenticator.isRefreshTokenRejected(UnauthorizedException()))
}

@Test
fun `connection problems and server errors are transient failures`() {
assertFalse(AccountAuthenticator.isRefreshTokenRejected(NoConnectionWithServerException()))
assertFalse(AccountAuthenticator.isRefreshTokenRejected(ServerConnectionTimeoutException()))
assertFalse(AccountAuthenticator.isRefreshTokenRejected(ServerNotReachableException()))
assertFalse(AccountAuthenticator.isRefreshTokenRejected(ServiceUnavailableException()))
assertFalse(AccountAuthenticator.isRefreshTokenRejected(UnhandledHttpCodeException(500)))
assertFalse(AccountAuthenticator.isRefreshTokenRejected(UnhandledHttpCodeException(400)))
}

@Test
fun `unknown failure or no failure at all is not a rejection`() {
assertFalse(AccountAuthenticator.isRefreshTokenRejected(RuntimeException("boom")))
assertFalse(AccountAuthenticator.isRefreshTokenRejected(null))
}
}
Loading
Loading