Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
337013002:
Third Party Authentication for Android Part III - Android OAuth2 (Closed)
|
Created:
6 years, 6 months ago by kelvinp Modified:
6 years, 6 months ago CC:
chromium-reviews, chromoting-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionThird Party Authentication for Android Part III - Android OAuth2 implicit flow
This change implements the OAuth2 implicit flow to fetch a third party token from the user.
1. Introduces a class ThirdPartyTokenFetcher to pop up a third party login page located at |tokenurl| in the browser.
2. Introduces a class OAuthRedirectActivity, which has an intent filter declared in the manifest to intercept the redirect URI upon a successful login.
3. It then starts the chromoting activity, which uses the ThirdPartyTokenFetcher to extract the code and token from the URI and pass it into the native component using JNI.
BUG=329109
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278062
Patch Set 1 : #
Total comments: 12
Patch Set 2 : Address CL feedbacks #
Total comments: 33
Patch Set 3 : Address Lambros' feedbacks #Patch Set 4 : Fix indentation #
Messages
Total messages: 10 (0 generated)
lgtm The OAuth part lgtm once my comment is fixed. Please get a review from Lambros for the Android/Java part. https://codereview.chromium.org/337013002/diff/20001/remoting/android/java/An... File remoting/android/java/AndroidManifest.xml.jinja2 (right): https://codereview.chromium.org/337013002/diff/20001/remoting/android/java/An... remoting/android/java/AndroidManifest.xml.jinja2:30: <data android:scheme="chromoting"/> Nit: I've seen other apps use the full java package name as the scheme (org.chromium.chromoting). That seems like a good practice (this is a shared namespace, so it's better to avoid collisions). https://codereview.chromium.org/337013002/diff/20001/remoting/android/java/sr... File remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java (right): https://codereview.chromium.org/337013002/diff/20001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:142: return false; This would ignore explicit errors (where the error gets reported in a redirect param, but obviously no code or token). Rather than ignoring calls, you should explicitly fail the request (failFetchToken()) for any unexpected intent.
https://codereview.chromium.org/337013002/diff/20001/remoting/android/java/sr... File remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java (right): https://codereview.chromium.org/337013002/diff/20001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:90: Intent intent = new Intent(Intent.ACTION_VIEW, uri); Does this pop out external browser? If yes, then I'm not sure it's the right UX, it will be confusing. Also what if default browser doesn't handle client certs properly? Would it better to create a WebView and open the URL there? https://codereview.chromium.org/337013002/diff/20001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:93: // startActivity() may crash the application. Can we just catch exception thrown by startActivity()? https://codereview.chromium.org/337013002/diff/20001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:112: String host = data.getHost(); I think you want getAuthority here, to also verify username and port. Also don't need variables here. getHost() can be called inline. https://codereview.chromium.org/337013002/diff/20001/remoting/android/java/sr... File remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java (right): https://codereview.chromium.org/337013002/diff/20001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:439: * Third Party Authentication There are two javadoc comments in front of fetchThirdPartyToken() (they both start with /** ). I don't think you need this one.
PTAL https://codereview.chromium.org/337013002/diff/20001/remoting/android/java/An... File remoting/android/java/AndroidManifest.xml.jinja2 (right): https://codereview.chromium.org/337013002/diff/20001/remoting/android/java/An... remoting/android/java/AndroidManifest.xml.jinja2:30: <data android:scheme="chromoting"/> On 2014/06/16 22:13:15, rmsousa wrote: > Nit: I've seen other apps use the full java package name as the scheme > (org.chromium.chromoting). That seems like a good practice (this is a shared > namespace, so it's better to avoid collisions). Done. https://codereview.chromium.org/337013002/diff/20001/remoting/android/java/sr... File remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java (right): https://codereview.chromium.org/337013002/diff/20001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:90: Intent intent = new Intent(Intent.ACTION_VIEW, uri); On 2014/06/16 22:55:38, Sergey Ulanov wrote: > Does this pop out external browser? > If yes, then I'm not sure it's the right UX, it will be confusing. Also what if > default browser doesn't handle client certs properly? > Would it better to create a WebView and open the URL there? Spoke offline with Sergey and Renato. We are using the browser for two reasons: 1. The security team recommends us to use the browser for the OAuth flow. 2. Using the webview implies that our app will have access to the cookie jar, which will contain the all powerful cookie of the third party authentication server. https://codereview.chromium.org/337013002/diff/20001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:93: // startActivity() may crash the application. On 2014/06/16 22:55:38, Sergey Ulanov wrote: > Can we just catch exception thrown by startActivity()? Good point. https://codereview.chromium.org/337013002/diff/20001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:112: String host = data.getHost(); On 2014/06/16 22:55:38, Sergey Ulanov wrote: > I think you want getAuthority here, to also verify username and port. Also don't > need variables here. getHost() can be called inline. Done for the extra variables. I kept the call to getHost though. This redirectURL is specific to the chromoting app and agnostic to the third party server. It is hardcoded in this file and it currently doesn't have a port nor an user name. https://codereview.chromium.org/337013002/diff/20001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:142: return false; On 2014/06/16 22:13:15, rmsousa wrote: > This would ignore explicit errors (where the error gets reported in a redirect > param, but obviously no code or token). > > Rather than ignoring calls, you should explicitly fail the request > (failFetchToken()) for any unexpected intent. Done. https://codereview.chromium.org/337013002/diff/20001/remoting/android/java/sr... File remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java (right): https://codereview.chromium.org/337013002/diff/20001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:439: * Third Party Authentication On 2014/06/16 22:55:38, Sergey Ulanov wrote: > There are two javadoc comments in front of fetchThirdPartyToken() (they both > start with /** ). > I don't think you need this one. Done.
https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/An... File remoting/android/java/AndroidManifest.xml.jinja2 (right): https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/An... remoting/android/java/AndroidManifest.xml.jinja2:30: <data android:scheme="org.chromium.chromoting"/> I think it might be better to use the official package name "com.google.chromeremotedesktop" for the scheme, as that is specific to the official app - nobody else will use that name. So change this to "{{ APK_PACKAGE_NAME }}". And in Java code, use Context.getPackageName(). https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right): https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:468: // that the host can obtains the shared secret from the third party authorization s/obtains/obtain https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:470: String token = code; nit: Blank line before comment. https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... File remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java (right): https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:31: /** Redirect Uri. See http://tools.ietf.org/html/rfc6749#section-3.1.2. */ s/Uri/URI https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:32: private static final String REDIRECT_URI_SCHEME = "org.chromium.chromoting"; As mentioned before, use getPackageName() for this. https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:58: /** Url to pop the third party login page. */ s/Url/URL "Url" is correct for identifiers, but looks strange in English documentation. Also, not sure what you mean by "pop" here? https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:92: Log.i("ThirdPartyAuth", "fetchToken() url:" + uri); 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. https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:98: failFetchToken("No browser is installed to open " + uri); As before, don't log |uri|. https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:105: final String action = intent.getAction(); Why is |action| final but not |data| ? Probably don't need "final" here? https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:125: HashMap<String, String> params = getFragmentParameters(data); Use Uri.getQueryParameter() instead, then you don't need getFragmentParameters() method. https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:127: final String accessToken = params.get("access_token"); Is "final" useful here? https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:189: * Unfortunately, Most browsers on Android, e.g. chrome, reloads the URL when a browser s/Most/most s/reloads/reload https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:191: * chrome or closes other tabs that causes the |redirect URL| to become the topmost tab. Remove '|'s. These are for quoting Java identifiers, so they aren't mistaken for plain English text. https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:198: public static class OAuthRedirectActivity extends Activity { I think I'd prefer this as a top level class, though I don't feel too strongly. https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:217: state, Indentation. https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... File remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java (right): https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:442: /** Pops up a third party login page to fetch the token required for authentication.*/ Space after '.'
PTAL https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/An... File remoting/android/java/AndroidManifest.xml.jinja2 (right): https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/An... remoting/android/java/AndroidManifest.xml.jinja2:30: <data android:scheme="org.chromium.chromoting"/> On 2014/06/17 02:01:39, Lambros wrote: > I think it might be better to use the official package name > "com.google.chromeremotedesktop" for the scheme, as that is specific to the > official app - nobody else will use that name. > > So change this to "{{ APK_PACKAGE_NAME }}". And in Java code, use > Context.getPackageName(). Done. https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right): https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:468: // that the host can obtains the shared secret from the third party authorization On 2014/06/17 02:01:39, Lambros wrote: > s/obtains/obtain Done. https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:470: String token = code; On 2014/06/17 02:01:39, Lambros wrote: > nit: Blank line before comment. Done. https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... File remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java (right): https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:31: /** Redirect Uri. See http://tools.ietf.org/html/rfc6749#section-3.1.2. */ On 2014/06/17 02:01:39, Lambros wrote: > s/Uri/URI Done. https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:32: private static final String REDIRECT_URI_SCHEME = "org.chromium.chromoting"; On 2014/06/17 02:01:40, Lambros wrote: > As mentioned before, use getPackageName() for this. Done. https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:58: /** Url to pop the third party login page. */ On 2014/06/17 02:01:40, Lambros wrote: > s/Url/URL "Url" is correct for identifiers, but looks strange in English > documentation. > > Also, not sure what you mean by "pop" here? Fixed. Also fixed the pop, sorry bad English :( https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:92: Log.i("ThirdPartyAuth", "fetchToken() url:" + uri); On 2014/06/17 02:01:40, Lambros wrote: > 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. 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. https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:98: failFetchToken("No browser is installed to open " + uri); On 2014/06/17 02:01:40, Lambros wrote: > As before, don't log |uri|. Done. https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:105: final String action = intent.getAction(); On 2014/06/17 02:01:39, Lambros wrote: > Why is |action| final but not |data| ? Probably don't need "final" here? 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. https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:125: HashMap<String, String> params = getFragmentParameters(data); On 2014/06/17 02:01:39, Lambros wrote: > Use Uri.getQueryParameter() instead, then you don't need getFragmentParameters() > method. 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¶m2=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. https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:127: final String accessToken = params.get("access_token"); On 2014/06/17 02:01:39, Lambros wrote: > Is "final" useful here? Done. https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:189: * Unfortunately, Most browsers on Android, e.g. chrome, reloads the URL when a browser On 2014/06/17 02:01:39, Lambros wrote: > s/Most/most > s/reloads/reload Done. https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:191: * chrome or closes other tabs that causes the |redirect URL| to become the topmost tab. On 2014/06/17 02:01:39, Lambros wrote: > Remove '|'s. These are for quoting Java identifiers, so they aren't mistaken for > plain English text. Done. https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:198: public static class OAuthRedirectActivity extends Activity { On 2014/06/17 02:01:40, Lambros wrote: > I think I'd prefer this as a top level class, though I don't feel too strongly. I made this an inner class as this class is conceptually closely related to the ThirdPartyTokenFetcher. https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:217: state, On 2014/06/17 02:01:39, Lambros wrote: > Indentation. My Indentation is generated by clang format. Are you referring to the fact that wrap around line indents needs to be 8 spaces? https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... File remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java (right): https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:442: /** Pops up a third party login page to fetch the token required for authentication.*/ On 2014/06/17 02:01:40, Lambros wrote: > Space after '.' Done.
lgtm, see comment about indentation https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... File remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java (right): https://codereview.chromium.org/337013002/diff/40001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:217: state, On 2014/06/17 03:33:40, kelvinp wrote: > On 2014/06/17 02:01:39, Lambros wrote: > > Indentation. > > My Indentation is generated by clang format. Are you referring to the fact that > wrap around line indents needs to be 8 spaces? 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.
The CQ bit was checked by kelvinp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kelvinp@chromium.org/337013002/80001
Message was sent while issue was closed.
Change committed as 278062 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
