Third Party Authentication for Android Part III - Android OAuth2

remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java

Index: remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java
diff --git a/remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java b/remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java
new file mode 100644
index 0000000000000000000000000000000000000000..5a56a3868470c3fc7d579778dbf1777a5df35441
--- /dev/null
+++ b/remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java
@@ -0,0 +1,221 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chromoting;
+
+import android.app.Activity;
+import android.content.ActivityNotFoundException;
+import android.content.ComponentName;
+import android.content.Intent;
+import android.content.pm.PackageManager;
+import android.net.Uri;
+import android.util.Base64;
+import android.util.Log;
+
+import java.security.SecureRandom;
+import java.util.HashMap;
+
+/**
+ * This class is responsible for fetching a third party token from the user using the OAuth2
+ * implicit flow.  It pops up a third party login page located at |tokenurl|.  It relies on the
+ * |ThirdPartyTokenFetcher$OAuthRedirectActivity| to intercept the access token from the redirect at
+ * |REDIRECT_URI_SCHEME|://|REDIRECT_URI_HOST| upon successful login.
+ */
+public class ThirdPartyTokenFetcher {
+    /** Callback for receiving the token. */
+    public interface Callback {
+        void onTokenFetched(String code, String accessToken);
+    }
+
+    /** Redirect Uri. See http://tools.ietf.org/html/rfc6749#section-3.1.2. */

[Lambros, 2014/06/17 02:01:39]

s/Uri/URI

[kelvinp, 2014/06/17 03:33:40]

Done.

+    private static final String REDIRECT_URI_SCHEME = "org.chromium.chromoting";

[Lambros, 2014/06/17 02:01:40]

As mentioned before, use getPackageName() for this.

[kelvinp, 2014/06/17 03:33:40]

Done.

+
+    private static final String REDIRECT_URI_HOST = "oauthredirect";
+
+    private static final String REDIRECT_URI = REDIRECT_URI_SCHEME + "://" + REDIRECT_URI_HOST;
+
+    /**
+     * Request both the authorization code and access token from the server.  See
+     * http://tools.ietf.org/html/rfc6749#section-3.1.1.
+     */
+    private static final String RESPONSE_TYPE = "code token";
+
+    /** This is used to securely generate an opaque 128 bit for the |mState| variable. */
+    private static SecureRandom sSecureRandom = new SecureRandom();
+
+    /** This is used to launch the third party login page in the browser. */
+    private Activity mContext;
+
+    /**
+     * An opaque value used by the client to maintain state between the request and callback.  The
+     * authorization server includes this value when redirecting the user-agent back to the client.
+     * The parameter is used for preventing cross-site request forgery. See
+     * http://tools.ietf.org/html/rfc6749#section-10.12.
+     */
+    private final String mState;
+
+    /** Url to pop the third party login page. */

[Lambros, 2014/06/17 02:01:40]

s/Url/URL "Url" is correct for identifiers, but looks strange in English
documentation.

Also, not sure what you mean by "pop" here?

[kelvinp, 2014/06/17 03:33:40]

Fixed.  Also fixed the pop, sorry bad English :(

+    private final String mTokenUrl;
+
+    /** The client identifier. See http://tools.ietf.org/html/rfc6749#section-2.2. */
+    private final String mClientId;
+
+    /** The scope of access request. See http://tools.ietf.org/html/rfc6749#section-3.3. */
+    private final String mScope;
+
+    private final Callback mCallback;
+
+    public ThirdPartyTokenFetcher(Activity context,
+                                  String tokenUrl,
+                                  String clientId,
+                                  String scope,
+                                  Callback callback) {
+        this.mContext = context;
+        this.mTokenUrl = tokenUrl;
+        this.mClientId = clientId;
+        this.mState = generateXsrfToken();
+        this.mScope = scope;
+        this.mCallback = callback;
+    }
+
+    public void fetchToken() {
+        Uri.Builder uriBuilder = Uri.parse(mTokenUrl).buildUpon();
+        uriBuilder.appendQueryParameter("redirect_uri", REDIRECT_URI);
+        uriBuilder.appendQueryParameter("scope", mScope);
+        uriBuilder.appendQueryParameter("client_id", mClientId);
+        uriBuilder.appendQueryParameter("state", mState);
+        uriBuilder.appendQueryParameter("response_type", RESPONSE_TYPE);
+
+        Uri uri = uriBuilder.build();
+        Intent intent = new Intent(Intent.ACTION_VIEW, uri);
+        Log.i("ThirdPartyAuth", "fetchToken() url:" + uri);

[Lambros, 2014/06/17 02:01:40]

I'd prefer we don't log this. We should keep informational logging down to a
minimum. Also, in this case, the uri might contain sensitive info that it's
better not to log.

[kelvinp, 2014/06/17 03:33:40]

The user is going to see the url in the browser anyway, so I don't think it
contains any sensitive information.
But in the spirit of keeping the logging minimum, I can remove the uri in the
statement below.

+        OAuthRedirectActivity.setEnabled(mContext, true);
+
+        try {
+            mContext.startActivity(intent);
+        } catch (ActivityNotFoundException e) {
+            failFetchToken("No browser is installed to open " + uri);

[Lambros, 2014/06/17 02:01:40]

As before, don't log |uri|.

[kelvinp, 2014/06/17 03:33:40]

Done.

+        }
+    }
+
+    private boolean isValidIntent(Intent intent) {
+        assert intent != null;
+
+        final String action = intent.getAction();

[Lambros, 2014/06/17 02:01:39]

Why is |action| final but not |data| ? Probably don't need "final" here?

[kelvinp, 2014/06/17 03:33:40]

I generally put final on variable that I don't expect to change as a safeguard. 
I have removed them if they make the code unreadable.

+
+        Uri data = intent.getData();
+        if (data != null) {
+            return Intent.ACTION_VIEW.equals(action) &&
+                   REDIRECT_URI_SCHEME.equals(data.getScheme()) &&
+                   REDIRECT_URI_HOST.equals(data.getHost());
+        }
+        return false;
+    }
+
+    public boolean handleTokenFetched(Intent intent) {
+        assert intent != null;
+
+        if (!isValidIntent(intent)) {
+            Log.w("ThirdPartyAuth", "Ignoring unmatched intent.");
+            return false;
+        }
+
+        Uri data = intent.getData();
+        HashMap<String, String> params = getFragmentParameters(data);

[Lambros, 2014/06/17 02:01:39]

Use Uri.getQueryParameter() instead, then you don't need getFragmentParameters()
method.

[kelvinp, 2014/06/17 03:33:40]

According to the OAuth Standard
(http://openid.net/specs/oauth-v2-multiple-response-types-1_0.html#Combinations),
the response for the type "code token" is encoded in the fragment (e.g.
uri#param1=value1&param2=value2) instead of the querystring (e.g.
uri?param1=param2).  As a result, Uri.getQueryParameter() cannot be not used
here as it can only retrieve values from the query string.

+
+        final String accessToken = params.get("access_token");

[Lambros, 2014/06/17 02:01:39]

Is "final" useful here?

[kelvinp, 2014/06/17 03:33:40]

Done.

+        final String code = params.get("code");
+        final String state = params.get("state");
+
+        if (!mState.equals(state)) {
+            failFetchToken("Ignoring redirect with invalid state.");
+            return false;
+        }
+
+        if (code == null || accessToken == null) {
+            failFetchToken("Ignoring redirect with missing code or token.");
+            return false;
+        }
+
+        Log.i("ThirdPartyAuth", "handleTokenFetched().");
+        mCallback.onTokenFetched(code, accessToken);
+        OAuthRedirectActivity.setEnabled(mContext, false);
+        return true;
+    }
+
+    private void failFetchToken(String errorMessage) {
+        Log.e("ThirdPartyAuth", errorMessage);
+        mCallback.onTokenFetched("", "");
+        OAuthRedirectActivity.setEnabled(mContext, false);
+    }
+
+    /** Generate a 128 bit URL-safe opaque string to prevent cross site request forgery (XSRF).*/
+    private static String generateXsrfToken() {
+        byte[] bytes = new byte[16];
+        sSecureRandom.nextBytes(bytes);
+        // Uses a variant of Base64 to make sure the URL is URL safe:
+        // URL_SAFE replaces - with _ and + with /.
+        // NO_WRAP removes the trailing newline character.
+        // NO_PADDING removes any trailing =.
+        return Base64.encodeToString(bytes, Base64.URL_SAFE | Base64.NO_WRAP | Base64.NO_PADDING);
+    }
+
+    /** Parses the fragment string into a key value pair. */
+    private static HashMap<String, String> getFragmentParameters(Uri uri) {
+        assert uri != null;
+        HashMap<String, String> result = new HashMap<String, String>();
+
+        final String fragment = uri.getFragment();
+
+        if (fragment != null) {
+            String[] parts = fragment.split("&");
+
+            for (String part : parts) {
+                String keyValuePair[] = part.split("=", 2);
+                if (keyValuePair.length == 2) {
+                    result.put(keyValuePair[0], keyValuePair[1]);
+                }
+            }
+        }
+        return result;
+    };
+
+    /**
+     * In the OAuth2 implicit flow, the browser will be redirected to
+     * |REDIRECT_URI_SCHEME|://|REDIRECT_URI_HOST| upon a successful login. OAuthRedirectActivity
+     * uses an intent filter in the manifest to intercept the URL and launch the chromoting app.
+     *
+     * Unfortunately, Most browsers on Android, e.g. chrome, reloads the URL when a browser

[Lambros, 2014/06/17 02:01:39]

s/Most/most
s/reloads/reload

[kelvinp, 2014/06/17 03:33:40]

Done.

+     * tab is activated.  As a result, chromoting is launched unintentionally when the user restarts
+     * chrome or closes other tabs that causes the |redirect URL| to become the topmost tab.

[Lambros, 2014/06/17 02:01:39]

Remove '|'s. These are for quoting Java identifiers, so they aren't mistaken for
plain English text.

[kelvinp, 2014/06/17 03:33:40]

Done.

+     *
+     * To solve the problem, the redirect intent-filter is declared in a separate activity,
+     * |OAuthRedirectActivity| instead of the MainActivity.  In this way, we can disable it,
+     * together with its intent filter, by default. |OAuthRedirectActivity| is only enabled when
+     * there is a pending token fetch request.
+     */
+    public static class OAuthRedirectActivity extends Activity {

[Lambros, 2014/06/17 02:01:40]

I think I'd prefer this as a top level class, though I don't feel too strongly.

[kelvinp, 2014/06/17 03:33:40]

I made this an inner class as this class is conceptually closely related to the
ThirdPartyTokenFetcher.

+        @Override
+        public void onStart() {
+            super.onStart();
+            // |OAuthRedirectActivity| runs in its own task, it needs to route the intent back
+            // to Chromoting.java to access the state of the current request.
+            Intent intent = getIntent();
+            intent.setClass(this, Chromoting.class);
+            startActivity(intent);
+            finishActivity(0);
+        }
+
+        public static void setEnabled(Activity context, boolean enabled) {
+            int state = enabled ? PackageManager.COMPONENT_ENABLED_STATE_ENABLED
+                                : PackageManager.COMPONENT_ENABLED_STATE_DEFAULT;
+            context.getPackageManager().setComponentEnabledSetting(
+                new ComponentName(
+                    context.getApplicationContext(),
+                    ThirdPartyTokenFetcher.OAuthRedirectActivity.class),
+                state,

[Lambros, 2014/06/17 02:01:39]

Indentation.

[kelvinp, 2014/06/17 03:33:40]

My Indentation is generated by clang format.  Are you referring to the fact that
wrap around line indents needs to be 8 spaces?

[Lambros, 2014/06/17 23:13:48]

Actually I hadn't noticed that - I thought that |state| should be aligned with
ThirdParty above it.

But yes, I think we should use 8-space indent for continuation lines.

Your choice, but I think this would be more readable if you pull new
ComponentName(..) out to a local variable.

+                PackageManager.DONT_KILL_APP);
+        }
+    }
+}