Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1422693002:
Make Android HttpNegotiateAuthenticator work without an activity (Closed)
DescriptionMake Android HttpNegotiateAuthenticator work without an activity
Fallback on the various alternatives to
AccountManager#getAuthTokenByFeatures that don't require an
activity when we don't have one. This will allow using this
authenticator in WebView.
BUG=533513
Committed: https://crrev.com/2a88ce48d75d70f24c210bcc0ae64b9314e65d78
Cr-Commit-Position: refs/heads/master@{#361871}
Patch Set 1 #Patch Set 2 : Add unit tests #Patch Set 3 : Add more tests #
Total comments: 18
Patch Set 4 : Add permission checks and better error code returns #Patch Set 5 : Rebase, comments #Patch Set 6 : Rebase without the AppContext patch #Patch Set 7 : Use ContextUtils to get the Application Context #Patch Set 8 : Rebase, fix ContextUtils issue with tests #Patch Set 9 : ApplicationStatus.destroyForTesting => ApplicationStatus.destroyForJUnitTests #
Total comments: 2
Patch Set 10 : Fix destroyForJUnitTests #
Dependent Patchsets: Messages
Total messages: 28 (8 generated)
dgn@chromium.org changed reviewers: + aberent@chromium.org
PTAL
dgn@chromium.org changed reviewers: + cbentzel@chromium.org, torne@chromium.org
PTAL. I added more tests and cleaned up. https://codereview.chromium.org/1422693002/diff/40001/net/android/junit/src/o... File net/android/junit/src/org/chromium/net/HttpNegotiateAuthenticatorTest.java (left): https://codereview.chromium.org/1422693002/diff/40001/net/android/junit/src/o... net/android/junit/src/org/chromium/net/HttpNegotiateAuthenticatorTest.java:53: private static class GetAuthTokenByFeaturesInvocation { Not needed anymore since we use Mockito mocks instead https://codereview.chromium.org/1422693002/diff/40001/net/android/junit/src/o... net/android/junit/src/org/chromium/net/HttpNegotiateAuthenticatorTest.java:229: public void testAccountManagerCallbackErrorReturns() { Split to reset the state of the mocks between tests. https://codereview.chromium.org/1422693002/diff/40001/net/android/junit/src/o... File net/android/junit/src/org/chromium/net/HttpNegotiateAuthenticatorTest.java (right): https://codereview.chromium.org/1422693002/diff/40001/net/android/junit/src/o... net/android/junit/src/org/chromium/net/HttpNegotiateAuthenticatorTest.java:89: private ArgumentCaptor<AccountManagerCallback<Bundle>> mBundleCallbackCaptor; Captors are done that way to avoid unchecked cast warnings because the generic parameter can't be specified in calls like `ArgumentCaptor.forClass(Foo<Bar>.class)`
https://codereview.chromium.org/1422693002/diff/40001/net/android/java/src/or... File net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java (right): https://codereview.chromium.org/1422693002/diff/40001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:117: final Context appContext = ApplicationStatus.getApplicationContext(); Note: torne@ is adding a replacement for this, I'll switch once the CL is live.
lgtm with very small nits. https://codereview.chromium.org/1422693002/diff/40001/net/android/java/src/or... File net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java (right): https://codereview.chromium.org/1422693002/diff/40001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:117: final Context appContext = ApplicationStatus.getApplicationContext(); On 2015/10/30 12:38:56, dgn wrote: > Note: torne@ is adding a replacement for this, I'll switch once the CL is live. nit: Maybe add a TODO if this is landing first. https://codereview.chromium.org/1422693002/diff/40001/net/android/junit/src/o... File net/android/junit/src/org/chromium/net/HttpNegotiateAuthenticatorTest.java (right): https://codereview.chromium.org/1422693002/diff/40001/net/android/junit/src/o... net/android/junit/src/org/chromium/net/HttpNegotiateAuthenticatorTest.java:89: private ArgumentCaptor<AccountManagerCallback<Bundle>> mBundleCallbackCaptor; On 2015/10/30 12:35:39, dgn wrote: > Captors are done that way to avoid unchecked cast warnings because the generic > parameter can't be specified in calls like > `ArgumentCaptor.forClass(Foo<Bar>.class)` nit: Comment to explain this?
https://codereview.chromium.org/1422693002/diff/40001/net/android/java/src/or... File net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java (right): https://codereview.chromium.org/1422693002/diff/40001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:117: final Context appContext = ApplicationStatus.getApplicationContext(); On 2015/10/30 16:38:10, aberent wrote: > On 2015/10/30 12:38:56, dgn wrote: > > Note: torne@ is adding a replacement for this, I'll switch once the CL is > live. > nit: Maybe add a TODO if this is landing first. I'll wait. It's broken otherwise, even though the code is unreachable until my webview patch lands https://codereview.chromium.org/1422693002/diff/40001/net/android/junit/src/o... File net/android/junit/src/org/chromium/net/HttpNegotiateAuthenticatorTest.java (right): https://codereview.chromium.org/1422693002/diff/40001/net/android/junit/src/o... net/android/junit/src/org/chromium/net/HttpNegotiateAuthenticatorTest.java:89: private ArgumentCaptor<AccountManagerCallback<Bundle>> mBundleCallbackCaptor; On 2015/10/30 16:38:10, aberent wrote: > On 2015/10/30 12:35:39, dgn wrote: > > Captors are done that way to avoid unchecked cast warnings because the generic > > parameter can't be specified in calls like > > `ArgumentCaptor.forClass(Foo<Bar>.class)` > > nit: Comment to explain this? It's a completely standard way to declare captors, and the documentation of @captor[1] even mentions the current issue with generics so it seems pretty normal. I commented here to give some context on the change from previous revisions of the patch, but I don't really want to make this look purely like a workaround, which I think explaining all that via a comment would do. [1]:http://docs.mockito.googlecode.com/hg/org/mockito/Captor.html
Description was changed from ========== Make Android HttpNegotiateAuthenticator work without an activity Fallback on the various alternatives to AccountManager#getAuthTokenByFeatures that don't require an activity when we don't have one. This will allow using this authenticator in WebView. BUG=533513 ========== to ========== Make Android HttpNegotiateAuthenticator work without an activity Fallback on the various alternatives to AccountManager#getAuthTokenByFeatures that don't require an activity when we don't have one. This will allow using this authenticator in WebView. BUG=533513 ==========
cbentzel@chromium.org changed reviewers: + asanka@chromium.org
Adding asanka as reviewer as he is looking at the other CL. I'll take a quick look now myself.
I can't speak Java well. But I do have a couple of more general questions and comments about what I think the code is doing. https://codereview.chromium.org/1422693002/diff/40001/net/android/java/src/or... File net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java (right): https://codereview.chromium.org/1422693002/diff/40001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:83: if (accounts.length != 1) { Is there no way to select an account in this case? Also, is there some UI to indicate that a particular identity is going to be used for authentication? (Not having one would still be consistent with how desktop works, but I'm curious). https://codereview.chromium.org/1422693002/diff/40001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:85: // instead of UNEXPECTED? Yeah. ERR_INVALID_AUTH_CREDENTIALS would be better. https://codereview.chromium.org/1422693002/diff/40001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:199: // If there is more than one account, it will show the disambig for each query expn disambig https://codereview.chromium.org/1422693002/diff/40001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:219: case HttpNegotiateConstants.OK: How permanent are the error codes in HttpNegotiateConstants.java? The error codes in //net are quite organic and were added as they were needed. For a more concrete API, we'd probably want to invest some time into figuring out what signals we want to get from the authenticator and have a few values for those rather than exposing the //net error codes.
PTAL https://codereview.chromium.org/1422693002/diff/40001/net/android/java/src/or... File net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java (right): https://codereview.chromium.org/1422693002/diff/40001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:83: if (accounts.length != 1) { On 2015/11/03 17:45:51, asanka wrote: > Is there no way to select an account in this case? > > Also, is there some UI to indicate that a particular identity is going to be > used for authentication? (Not having one would still be consistent with how > desktop works, but I'm curious). There isn't any. Showing any kind of UI requires having access to an activity, and in this case we don't have one. When we have one, we use the old code path that allows selecting an account. Going to settings > accounts shows a list of accounts and there should be one installed by the device/network administrator to authenticate users. Aside from that, we don't have anything to display it. https://codereview.chromium.org/1422693002/diff/40001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:85: // instead of UNEXPECTED? On 2015/11/03 17:45:51, asanka wrote: > Yeah. ERR_INVALID_AUTH_CREDENTIALS would be better. Done. https://codereview.chromium.org/1422693002/diff/40001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:219: case HttpNegotiateConstants.OK: On 2015/11/03 17:45:51, asanka wrote: > How permanent are the error codes in HttpNegotiateConstants.java? > > The error codes in //net are quite organic and were added as they were needed. > For a more concrete API, we'd probably want to invest some time into figuring > out what signals we want to get from the authenticator and have a few values for > those rather than exposing the //net error codes. They are supposed to not change as 3p devs will use these values in their authenticators. We explicitely direct them to the HttpNegotiateConstants source in https://www.chromium.org/developers/design-documents/http-authentication/writ... I'm not sure about changing them now. aberent@, what do you think?
https://codereview.chromium.org/1422693002/diff/40001/net/android/java/src/or... File net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java (right): https://codereview.chromium.org/1422693002/diff/40001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:219: case HttpNegotiateConstants.OK: On 2015/11/05 at 14:39:14, dgn wrote: > On 2015/11/03 17:45:51, asanka wrote: > > How permanent are the error codes in HttpNegotiateConstants.java? > > > > The error codes in //net are quite organic and were added as they were needed. > > For a more concrete API, we'd probably want to invest some time into figuring > > out what signals we want to get from the authenticator and have a few values for > > those rather than exposing the //net error codes. > > They are supposed to not change as 3p devs will use these values in their authenticators. We explicitely direct them to the HttpNegotiateConstants source in https://www.chromium.org/developers/design-documents/http-authentication/writ... > > I'm not sure about changing them now. aberent@, what do you think? To give some background, the net::Error values were constructed based on the fact that Chrome is responsible for logging and reporting error conditions in addition to handling them. //net explicitly invoked known and tested libraries (where applicable) or //net itself implemented support for the underlying protocols, and //chrome was responsible for prompting the user. These assumptions, I believe, no longer hold when we are invoking an unknown authenticator determined at runtime. So we kind of have to abstract out our HTTP auth state machine and figure out which high-level signals we want from this library. For example, the result of invoking the authenticator might be one of the following: * Success. Put this token in a Authorization/Proxy-Authorization header and send the request out again. * Failure: * Don't invoke me again for this origin. (w/ TTL or some other signal?) * Don't invoke me again for any origin (i.e. permanent error). (w/ TTL or some other signal?)
https://codereview.chromium.org/1422693002/diff/40001/net/android/java/src/or... File net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java (right): https://codereview.chromium.org/1422693002/diff/40001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:199: // If there is more than one account, it will show the disambig for each query On 2015/11/03 17:45:51, asanka wrote: > expn disambig Done. https://codereview.chromium.org/1422693002/diff/40001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:219: case HttpNegotiateConstants.OK: On 2015/11/05 17:41:32, asanka wrote: > On 2015/11/05 at 14:39:14, dgn wrote: > > On 2015/11/03 17:45:51, asanka wrote: > > > How permanent are the error codes in HttpNegotiateConstants.java? > > > > > > The error codes in //net are quite organic and were added as they were > needed. > > > For a more concrete API, we'd probably want to invest some time into > figuring > > > out what signals we want to get from the authenticator and have a few values > for > > > those rather than exposing the //net error codes. > > > > They are supposed to not change as 3p devs will use these values in their > authenticators. We explicitely direct them to the HttpNegotiateConstants source > in > https://www.chromium.org/developers/design-documents/http-authentication/writ... > > > > I'm not sure about changing them now. aberent@, what do you think? > > To give some background, the net::Error values were constructed based on the > fact that Chrome is responsible for logging and reporting error conditions in > addition to handling them. //net explicitly invoked known and tested libraries > (where applicable) or //net itself implemented support for the underlying > protocols, and //chrome was responsible for prompting the user. These > assumptions, I believe, no longer hold when we are invoking an unknown > authenticator determined at runtime. So we kind of have to abstract out our HTTP > auth state machine and figure out which high-level signals we want from this > library. > > For example, the result of invoking the authenticator might be one of the > following: > > * Success. Put this token in a Authorization/Proxy-Authorization header and send > the request out again. > * Failure: > * Don't invoke me again for this origin. (w/ TTL or some other signal?) > * Don't invoke me again for any origin (i.e. permanent error). (w/ TTL or some > other signal?) Moved this discussion to http://crbug.com/552414. I agree that simplifying the exposed codes seems to be the right thing to do. But if we make a change, it should be a different CL. I would prefer to keep the previously existing status codes in this one
PTAL. I opened https://codereview.chromium.org/1442053002 for changes related to the //net constants. I would like to land this without the ApplicationContext changes. So far, webview has nothing related to negotiate, so the code paths that would be affected can't be reached yet.
asanka@: PTAL. The changes needed to get an ApplicationContext now landed, so this patch could land too.
On 2015/11/24 at 17:04:07, dgn wrote: > asanka@: PTAL. The changes needed to get an ApplicationContext now landed, so this patch could land too. LGTM
Thanks! torne@: PTAL at the changes in ApplicationStatus
https://codereview.chromium.org/1422693002/diff/160001/base/android/java/src/... File base/android/java/src/org/chromium/base/ApplicationStatus.java (right): https://codereview.chromium.org/1422693002/diff/160001/base/android/java/src/... base/android/java/src/org/chromium/base/ApplicationStatus.java:404: sCachedApplicationState = 0; You should update this holding the lock (because *mumble* memory ordering), and it looks like "null" is the right initialisation for "this is unset" - it's an Integer, not an int.
https://codereview.chromium.org/1422693002/diff/160001/base/android/java/src/... File base/android/java/src/org/chromium/base/ApplicationStatus.java (right): https://codereview.chromium.org/1422693002/diff/160001/base/android/java/src/... base/android/java/src/org/chromium/base/ApplicationStatus.java:404: sCachedApplicationState = 0; On 2015/11/25 19:10:52, Torne wrote: > You should update this holding the lock (because *mumble* memory ordering), and > it looks like "null" is the right initialisation for "this is unset" - it's an > Integer, not an int. Thanks, that was bad :/ Done.
lgtm
The CQ bit was checked by dgn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aberent@chromium.org, asanka@chromium.org Link to the patchset: https://codereview.chromium.org/1422693002/#ps180001 (title: "Fix destroyForJUnitTests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422693002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422693002/180001
Message was sent while issue was closed.
Description was changed from ========== Make Android HttpNegotiateAuthenticator work without an activity Fallback on the various alternatives to AccountManager#getAuthTokenByFeatures that don't require an activity when we don't have one. This will allow using this authenticator in WebView. BUG=533513 ========== to ========== Make Android HttpNegotiateAuthenticator work without an activity Fallback on the various alternatives to AccountManager#getAuthTokenByFeatures that don't require an activity when we don't have one. This will allow using this authenticator in WebView. BUG=533513 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Make Android HttpNegotiateAuthenticator work without an activity Fallback on the various alternatives to AccountManager#getAuthTokenByFeatures that don't require an activity when we don't have one. This will allow using this authenticator in WebView. BUG=533513 ========== to ========== Make Android HttpNegotiateAuthenticator work without an activity Fallback on the various alternatives to AccountManager#getAuthTokenByFeatures that don't require an activity when we don't have one. This will allow using this authenticator in WebView. BUG=533513 Committed: https://crrev.com/2a88ce48d75d70f24c210bcc0ae64b9314e65d78 Cr-Commit-Position: refs/heads/master@{#361871} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/2a88ce48d75d70f24c210bcc0ae64b9314e65d78 Cr-Commit-Position: refs/heads/master@{#361871} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
