Move responsibility for refreshing expired credentials from RemoteOperation to OwnCloudClient

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) {
+        if (mSavedAccount != null) {
-			mCredentials = AccountUtils.getCredentialsForAccount(context, mSavedAccount);
+            mCredentials = AccountUtils.getCredentialsForAccount(context, mSavedAccount);
-		}
+        }
-	}
+    }
 
     public Uri getBaseUri() {
         return mBaseUri;

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");
+            //logCookiesAtRequest(method.getRequestHeaders(), "before");
-        //logCookiesAtState("before");
+            //logCookiesAtState("before");
-        method.setFollowRedirects(false);
+            method.setFollowRedirects(false);
 
-        int status = super.executeMethod(method);
+            status = super.executeMethod(method);
 
-        if (mFollowRedirects) {
+            if (mFollowRedirects) {
-            status = followRedirection(method).getLastStatus();
+                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;
+    }
+
 }

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;
     }
 

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);

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 {
 
         /**

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 {
+        try {
-            repeat = false;
+            grantOwnCloudClient();
+            result = run(mClient);
 
-            try {
+        } catch (AccountsException | IOException e) {
-                grantOwnCloudClient();
+            Log_OC.e(TAG, "Error while trying to access to " + mAccount.name, e);
-                result = run(mClient);
+            result = new RemoteOperationResult(e);
-
+        }
-            } 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);
 
         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;
-    }
 }