From e466bac6b15336983340e8b2c552a88423a06264 Mon Sep 17 00:00:00 2001 From: "David A. Velasco" Date: Thu, 17 Aug 2017 09:22:30 +0200 Subject: [PATCH] Move responsibility for refreshing expired credentials from RemoteOperation to OwnCloudClient --- .../android/lib/common/OwnCloudAccount.java | 8 +- .../android/lib/common/OwnCloudClient.java | 169 ++++++++++++++++-- .../lib/common/SimpleFactoryManager.java | 2 + .../lib/common/SingleSessionManager.java | 4 +- .../lib/common/accounts/AccountUtils.java | 78 +------- .../common/operations/RemoteOperation.java | 102 +---------- 6 files changed, 170 insertions(+), 193 deletions(-) diff --git a/src/com/owncloud/android/lib/common/OwnCloudAccount.java b/src/com/owncloud/android/lib/common/OwnCloudAccount.java index d6534e96..533247df 100644 --- a/src/com/owncloud/android/lib/common/OwnCloudAccount.java +++ b/src/com/owncloud/android/lib/common/OwnCloudAccount.java @@ -124,10 +124,10 @@ public class OwnCloudAccount { throw new IllegalArgumentException("Parameter 'context' cannot be null"); } - if (mSavedAccount != null) { - mCredentials = AccountUtils.getCredentialsForAccount(context, mSavedAccount); - } - } + if (mSavedAccount != null) { + mCredentials = AccountUtils.getCredentialsForAccount(context, mSavedAccount); + } + } public Uri getBaseUri() { return mBaseUri; diff --git a/src/com/owncloud/android/lib/common/OwnCloudClient.java b/src/com/owncloud/android/lib/common/OwnCloudClient.java index 96bace13..99433d2c 100644 --- a/src/com/owncloud/android/lib/common/OwnCloudClient.java +++ b/src/com/owncloud/android/lib/common/OwnCloudClient.java @@ -43,6 +43,8 @@ import org.apache.commons.httpclient.cookie.CookiePolicy; import org.apache.commons.httpclient.params.HttpMethodParams; import org.apache.commons.httpclient.params.HttpParams; +import android.accounts.AccountManager; +import android.accounts.AccountsException; import android.content.Context; import android.net.Uri; @@ -57,7 +59,8 @@ import com.owncloud.android.lib.resources.status.OwnCloudVersion; public class OwnCloudClient extends HttpClient { private static final String TAG = OwnCloudClient.class.getSimpleName(); - public static final int MAX_REDIRECTIONS_COUNT = 3; + private static final int MAX_REDIRECTIONS_COUNT = 3; + private static final int MAX_REPEAT_COUNT_WITH_FRESH_CREDENTIALS = 1; private static final String PARAM_SINGLE_COOKIE_HEADER = "http.protocol.single-cookie-header"; private static final boolean PARAM_SINGLE_COOKIE_HEADER_VALUE = true; private static final String PARAM_PROTOCOL_VERSION = "http.protocol.version"; @@ -74,10 +77,24 @@ public class OwnCloudClient extends HttpClient { private OwnCloudVersion mVersion = null; /// next too attributes are a very ugly dependency, added to grant silent retry of OAuth token when needed ; - /// see {} for more details + /// see #shouldInvalidateCredentials and #invalidateCredentials for more details private Context mContext; private OwnCloudAccount mAccount; + /** + * {@link @OwnCloudClientManager} holding a reference to this object and delivering it to callers; might be + * NULL + */ + private OwnCloudClientManager mOwnCloudClientManager = null; + + /** + * When 'true', the method {@link #executeMethod(HttpMethod)} tries to silently refresh credentials + * if fails due to lack of authorization, if credentials support authorization refresh. + */ + private boolean mSilentRefreshOfAccountCredentials = true; + + + /** * Constructor */ @@ -190,25 +207,38 @@ public class OwnCloudClient extends HttpClient { */ @Override public int executeMethod(HttpMethod method) throws IOException { - // Update User Agent - HttpParams params = method.getParams(); - String userAgent = OwnCloudClientManagerFactory.getUserAgent(); - params.setParameter(HttpMethodParams.USER_AGENT, userAgent); - preventCrashDueToInvalidPort(method); + boolean repeatWithFreshCredentials; + int repeatCounter = 0; + int status; - Log_OC.d(TAG + " #" + mInstanceNumber, "REQUEST " + + do { + // Update User Agent + HttpParams params = method.getParams(); + String userAgent = OwnCloudClientManagerFactory.getUserAgent(); + params.setParameter(HttpMethodParams.USER_AGENT, userAgent); + + preventCrashDueToInvalidPort(method); + + Log_OC.d(TAG + " #" + mInstanceNumber, "REQUEST " + method.getName() + " " + method.getPath()); - //logCookiesAtRequest(method.getRequestHeaders(), "before"); - //logCookiesAtState("before"); - method.setFollowRedirects(false); + //logCookiesAtRequest(method.getRequestHeaders(), "before"); + //logCookiesAtState("before"); + method.setFollowRedirects(false); - int status = super.executeMethod(method); + status = super.executeMethod(method); - if (mFollowRedirects) { - status = followRedirection(method).getLastStatus(); - } + if (mFollowRedirects) { + status = followRedirection(method).getLastStatus(); + } + + repeatWithFreshCredentials = checkUnauthorizedAccess(status, repeatCounter); + if (repeatWithFreshCredentials) { + repeatCounter++; + } + + } while (repeatWithFreshCredentials); //logCookiesAtRequest(method.getRequestHeaders(), "after"); //logCookiesAtState("after"); @@ -217,7 +247,6 @@ public class OwnCloudClient extends HttpClient { return status; } - /** * Fix for https://github.com/owncloud/android/issues/1847#issuecomment-267558274 * @@ -468,4 +497,112 @@ public class OwnCloudClient extends HttpClient { return mAccount; } + /** + * Enables or disables silent refresh of credentials, if supported by credentials themselves. + */ + public void setSilentRefreshOfAccountCredentials(boolean silentRefreshOfAccountCredentials) { + mSilentRefreshOfAccountCredentials = silentRefreshOfAccountCredentials; + } + + public boolean getSilentRefreshOfAccountCredentials() { + return mSilentRefreshOfAccountCredentials; + } + + + /** + * Checks the status code of an execution and decides if should be repeated with fresh credentials. + * + * Invalidates current credentials if the request failed as anauthorized. + * + * Refresh current credentials if possible, and marks a retry. + * + * @param status + * @param repeatCounter + * @return + */ + private boolean checkUnauthorizedAccess(int status, int repeatCounter) { + boolean credentialsWereRefreshed = false; + + if (shouldInvalidateAccountCredentials(status)) { + boolean invalidated = invalidateAccountCredentials(); + + if (invalidated) { + if (getCredentials().authTokenCanBeRefreshed() && + repeatCounter < MAX_REPEAT_COUNT_WITH_FRESH_CREDENTIALS) { + + try { + mAccount.loadCredentials(mContext); + // if mAccount.getCredentials().length() == 0 --> refresh failed + setCredentials(mAccount.getCredentials()); + credentialsWereRefreshed = true; + + } catch (AccountsException | IOException e) { + Log_OC.e( + TAG, + "Error while trying to refresh auth token for " + mAccount.getSavedAccount().name, + e + ); + } + } + + if (!credentialsWereRefreshed && mOwnCloudClientManager != null) { + // if credentials are not refreshed, client must be removed + // from the OwnCloudClientManager to prevent it is reused once and again + mOwnCloudClientManager.removeClientFor(mAccount); + } + } + // else: execute will finish with status 401 + } + + return credentialsWereRefreshed; + } + + /** + * Determines if credentials should be invalidated according the to the HTTPS status + * of a network request just performed. + * + * @param httpStatusCode Result of the last request ran with the 'credentials' belows. + + * @return 'True' if credentials should and might be invalidated, 'false' if shouldn't or + * cannot be invalidated with the given arguments. + */ + private boolean shouldInvalidateAccountCredentials(int httpStatusCode) { + + boolean should = (httpStatusCode == HttpStatus.SC_UNAUTHORIZED); // invalid credentials + + should &= (mCredentials != null && // real credentials + !(mCredentials instanceof OwnCloudCredentialsFactory.OwnCloudAnonymousCredentials)); + + // test if have all the needed to effectively invalidate ... + should &= (mAccount != null && mAccount.getSavedAccount() != null && mContext != null); + + return should; + } + + /** + * Invalidates credentials stored for the given account in the system {@link AccountManager} and in + * current {@link OwnCloudClientManagerFactory#getDefaultSingleton()} instance. + * + * {@link #shouldInvalidateAccountCredentials(int)} should be called first. + * + * @return 'True' if invalidation was successful, 'false' otherwise. + */ + private boolean invalidateAccountCredentials() { + AccountManager am = AccountManager.get(mContext); + am.invalidateAuthToken( + mAccount.getSavedAccount().type, + mCredentials.getAuthToken() + ); + am.clearPassword(mAccount.getSavedAccount()); // being strict, only needed for Basic Auth credentials + return true; + } + + public OwnCloudClientManager getOwnCloudClientManager() { + return mOwnCloudClientManager; + } + + void setOwnCloudClientManager(OwnCloudClientManager clientManager) { + mOwnCloudClientManager = clientManager; + } + } diff --git a/src/com/owncloud/android/lib/common/SimpleFactoryManager.java b/src/com/owncloud/android/lib/common/SimpleFactoryManager.java index 81abbc78..b955c517 100644 --- a/src/com/owncloud/android/lib/common/SimpleFactoryManager.java +++ b/src/com/owncloud/android/lib/common/SimpleFactoryManager.java @@ -63,6 +63,8 @@ public class SimpleFactoryManager implements OwnCloudClientManager { } client.setCredentials(account.getCredentials()); client.setAccount(account); + client.setContext(context); + client.setOwnCloudClientManager(this); return client; } diff --git a/src/com/owncloud/android/lib/common/SingleSessionManager.java b/src/com/owncloud/android/lib/common/SingleSessionManager.java index f168837d..05396480 100644 --- a/src/com/owncloud/android/lib/common/SingleSessionManager.java +++ b/src/com/owncloud/android/lib/common/SingleSessionManager.java @@ -117,8 +117,10 @@ public class SingleSessionManager implements OwnCloudClientManager { true); // TODO remove dependency on OwnCloudClientFactory client.getParams().setCookiePolicy(CookiePolicy.BROWSER_COMPATIBILITY); client.setAccount(account); - // enable cookie tracking + client.setContext(context); + client.setOwnCloudClientManager(this); + // enable cookie tracking AccountUtils.restoreCookies(account.getSavedAccount(), client, context); account.loadCredentials(context); diff --git a/src/com/owncloud/android/lib/common/accounts/AccountUtils.java b/src/com/owncloud/android/lib/common/accounts/AccountUtils.java index 7f91207d..f59cfd59 100644 --- a/src/com/owncloud/android/lib/common/accounts/AccountUtils.java +++ b/src/com/owncloud/android/lib/common/accounts/AccountUtils.java @@ -28,6 +28,7 @@ package com.owncloud.android.lib.common.accounts; import java.io.IOException; import org.apache.commons.httpclient.Cookie; +import org.apache.commons.httpclient.HttpStatus; import android.accounts.Account; import android.accounts.AccountManager; @@ -262,83 +263,6 @@ public class AccountUtils { } } - /** - * Determines if credentials should be invalidated for the given account, according the to the result - * of an operation just run through the given client. - * - * @param result Result of the last remote operation ran through 'client' below. - * @param client {@link OwnCloudClient} that run last remote operation, resulting in 'result' above. - * @param account {@link Account} storing credentials used to run the last operation. - * @param context Android context, needed to access {@link AccountManager}; method will return false - * if NULL, since invalidation of credentials is just not possible if no context is - * available. - * @return 'True' if credentials should and might be invalidated, 'false' if shouldn't or - * cannot be invalidated with the given arguments. - */ - public static boolean shouldInvalidateAccountCredentials( - RemoteOperationResult result, - OwnCloudClient client, - Account account, - Context context) { - - boolean should = RemoteOperationResult.ResultCode.UNAUTHORIZED.equals(result.getCode()); - // invalid credentials - - should &= (client.getCredentials() != null && // real credentials - !(client.getCredentials() instanceof OwnCloudCredentialsFactory.OwnCloudAnonymousCredentials)); - - // test if have all the needed to effectively invalidate ... - should &= ((account != null && context != null) || // ... either directly, or ... - client.getAccount() != null && client.getContext() != null) ; // ... indirectly - - return should; - } - - /** - * Invalidates credentials stored for the given account in the system {@link AccountManager} and in - * current {@link OwnCloudClientManagerFactory#getDefaultSingleton()} instance. - * - * {@link AccountUtils#shouldInvalidateAccountCredentials(RemoteOperationResult, OwnCloudClient, Account, Context)} - * should be called first. - * - * @param client {@link OwnCloudClient} that run last remote operation. - * @param account {@link Account} storing credentials to invalidate. - * @param context Android context, needed to access {@link AccountManager}. - * - * @return 'True' if invalidation was successful, 'false' otherwise. - */ - public static boolean invalidateAccountCredentials( - OwnCloudClient client, - Account account, - Context context - ) { - Account checkedAccount = (account == null) ? client.getAccount().getSavedAccount() : account; - if (checkedAccount == null) { - throw new IllegalArgumentException("Account cannot be null both in parameter and in client"); - } - Context checkedContext = (context == null) ? client.getContext() : context; - if (checkedContext == null) { - throw new IllegalArgumentException("Context cannot be null both in parameter and in client"); - } - - try { - OwnCloudAccount ocAccount = new OwnCloudAccount(checkedAccount, checkedContext); - OwnCloudClientManagerFactory.getDefaultSingleton(). - removeClientFor(ocAccount); // to prevent nobody else is provided this client - AccountManager am = AccountManager.get(checkedContext); - am.invalidateAuthToken( - checkedAccount.type, - client.getCredentials().getAuthToken() - ); - am.clearPassword(checkedAccount); // being strict, only needed for Basic Auth credentials - return true; - - } catch (AccountUtils.AccountNotFoundException e) { - Log_OC.e(TAG, "Account was deleted from AccountManager, cannot invalidate its token", e); - return false; - } - } - public static class AccountNotFoundException extends AccountsException { /** diff --git a/src/com/owncloud/android/lib/common/operations/RemoteOperation.java b/src/com/owncloud/android/lib/common/operations/RemoteOperation.java index c2590adb..a8de3938 100644 --- a/src/com/owncloud/android/lib/common/operations/RemoteOperation.java +++ b/src/com/owncloud/android/lib/common/operations/RemoteOperation.java @@ -87,28 +87,6 @@ public abstract class RemoteOperation implements Runnable { */ private Handler mListenerHandler = null; - /** - * When 'true', the operation tries to silently refresh credentials if fails due to lack of authorization. - * - * Valid for 'execute methods' receiving an {@link Account} instance as parameter, out of the box: - * -- {@link RemoteOperation#execute(Account, Context)} - * -- {@link RemoteOperation#execute(Account, Context, OnRemoteOperationListener, Handler)} - * - * Valid for 'execute methods' receiving an {@link OwnCloudClient}, as long as it returns non null values - * to its methods {@link OwnCloudClient#getContext()} && {@link OwnCloudClient#getAccount()}: - * -- {@link RemoteOperation#execute(OwnCloudClient)} - * -- {@link RemoteOperation#execute(OwnCloudClient, OnRemoteOperationListener, Handler)} - */ - private boolean mSilentRefreshOfAccountCredentials = false; - - - /** - * Counter to establish the number of times a failed operation will be repeated due to - * an authorization error - */ - private int MAX_REPEAT_COUNTER = 1; - - /** * Abstract method to implement the operation in derived classes. */ @@ -137,14 +115,6 @@ public abstract class RemoteOperation implements Runnable { "Context"); mAccount = account; mContext = context.getApplicationContext(); - try { - OwnCloudAccount ocAccount = new OwnCloudAccount(mAccount, mContext); - mClient = OwnCloudClientManagerFactory.getDefaultSingleton(). - getClientFor(ocAccount, mContext); - } catch (Exception e) { - Log_OC.e(TAG, "Error while trying to access to " + mAccount.name, e); - return new RemoteOperationResult(e); - } return runOperation(); } @@ -290,51 +260,15 @@ public abstract class RemoteOperation implements Runnable { private RemoteOperationResult runOperation() { RemoteOperationResult result; - boolean repeat; - int repeatCounter = 0; - do { - repeat = false; + try { + grantOwnCloudClient(); + result = run(mClient); - try { - grantOwnCloudClient(); - result = run(mClient); - - } catch (AccountsException | IOException e) { - Log_OC.e(TAG, "Error while trying to access to " + mAccount.name, e); - result = new RemoteOperationResult(e); - } - - if (mSilentRefreshOfAccountCredentials && - AccountUtils.shouldInvalidateAccountCredentials( - result, - mClient, - mAccount, - mContext) - ) { - boolean invalidated = AccountUtils.invalidateAccountCredentials( - mClient, - mAccount, - mContext - ); - if (invalidated && - mClient.getCredentials().authTokenCanBeRefreshed() && - repeatCounter < MAX_REPEAT_COUNTER) { - - mClient = null; - repeat = true; - repeatCounter++; - - // this will result in a new loop, and grantOwnCloudClient() will - // create a new instance for mClient, getting a new fresh token in the - // way, in the AccountAuthenticator * ; - // this, unfortunately, is a hidden runtime dependency back to the app; - // we should fix it ASAP - } - // else: operation will finish with ResultCode.UNAUTHORIZED - } - - } while (repeat); + } catch (AccountsException | IOException e) { + Log_OC.e(TAG, "Error while trying to access to " + mAccount.name, e); + result = new RemoteOperationResult(e); + } return result; } @@ -364,26 +298,4 @@ public abstract class RemoteOperation implements Runnable { return mClient; } - - /** - * Enables or disables silent refresh of credentials, if supported by credentials itself. - * - * Will have effect if called before in all cases: - * -- {@link RemoteOperation#execute(Account, Context)} - * -- {@link RemoteOperation#execute(Account, Context, OnRemoteOperationListener, Handler)} - * - * Will have NO effect if called before: - * -- {@link RemoteOperation#execute(OwnCloudClient)} - * -- {@link RemoteOperation#execute(OwnCloudClient, OnRemoteOperationListener, Handler)} - * - * ... UNLESS the passed {@link OwnCloudClient} returns non-NULL values for - * {@link OwnCloudClient#getAccount()} && {@link OwnCloudClient#getContext()} - */ - public void setSilentRefreshOfAccountCredentials(boolean silentRefreshOfAccountCredentials) { - mSilentRefreshOfAccountCredentials = silentRefreshOfAccountCredentials; - } - - public boolean getSilentRefreshOfAccountCredentials() { - return mSilentRefreshOfAccountCredentials; - } } \ No newline at end of file