Issue 1128043007: Support Kerberos on Android

aberent

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

5 years, 7 months ago (2015-05-13 17:11:14 UTC) #1

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1128043007/1

5 years, 7 months ago (2015-05-13 17:12:20 UTC) #2

aberent

aberent@chromium.org changed reviewers: + bauerb@chromium.org

5 years, 7 months ago (2015-05-13 17:12:38 UTC) #3

aberent

On 2015/05/13 at 17:12:38, aberent wrote: > aberent@chromium.org changed reviewers: > + bauerb@chromium.org Preliminary version, ...

5 years, 7 months ago (2015-05-13 17:13:40 UTC) #4

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

5 years, 7 months ago (2015-05-13 17:27:41 UTC) #5

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_gn_chromeos_rel/builds/37231) linux_chromium_gn_dbg on ...

5 years, 7 months ago (2015-05-13 17:27:43 UTC) #6

aberent

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

5 years, 7 months ago (2015-05-14 12:07:49 UTC) #7

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1128043007/20001

5 years, 7 months ago (2015-05-14 12:08:33 UTC) #8

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

5 years, 7 months ago (2015-05-14 12:18:45 UTC) #9

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_gn_dbg/builds/72033)

5 years, 7 months ago (2015-05-14 12:18:46 UTC) #10

aberent

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

5 years, 7 months ago (2015-05-14 17:54:52 UTC) #11

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1128043007/40001

5 years, 7 months ago (2015-05-14 17:55:12 UTC) #12

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

5 years, 7 months ago (2015-05-14 18:36:17 UTC) #13

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win8_chromium_rel on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel/builds/83903)

5 years, 7 months ago (2015-05-14 18:36:20 UTC) #14

Bernhard Bauer

On 2015/05/13 17:13:40, aberent wrote: > On 2015/05/13 at 17:12:38, aberent wrote: > > mailto:aberent@chromium.org ...

5 years, 7 months ago (2015-05-15 00:15:46 UTC) #15

aberent

On 2015/05/15 at 00:15:46, bauerb wrote: > On 2015/05/13 17:13:40, aberent wrote: > > On ...

5 years, 7 months ago (2015-05-15 08:42:54 UTC) #16

aberent

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

5 years, 7 months ago (2015-05-15 10:45:30 UTC) #17

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1128043007/60001

5 years, 7 months ago (2015-05-15 10:45:48 UTC) #18

aberent

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

5 years, 7 months ago (2015-05-15 11:13:22 UTC) #20

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1128043007/80001

5 years, 7 months ago (2015-05-15 11:13:39 UTC) #21

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

5 years, 7 months ago (2015-05-15 12:56:22 UTC) #22

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

5 years, 7 months ago (2015-05-15 12:56:23 UTC) #23

Bernhard Bauer

https://codereview.chromium.org/1128043007/diff/1/net/android/http_android_auth_negotiate.cc File net/android/http_android_auth_negotiate.cc (right): https://codereview.chromium.org/1128043007/diff/1/net/android/http_android_auth_negotiate.cc#newcode13 net/android/http_android_auth_negotiate.cc:13: #include "jni/HttpNegotiateAuthenticator_jni.h" The generated JNI include usually goes at ...

5 years, 7 months ago (2015-05-15 15:06:16 UTC) #24

aberent

https://codereview.chromium.org/1128043007/diff/1/net/android/http_android_auth_negotiate.cc File net/android/http_android_auth_negotiate.cc (right): https://codereview.chromium.org/1128043007/diff/1/net/android/http_android_auth_negotiate.cc#newcode13 net/android/http_android_auth_negotiate.cc:13: #include "jni/HttpNegotiateAuthenticator_jni.h" On 2015/05/15 15:06:16, Bernhard Bauer wrote: > ...

5 years, 7 months ago (2015-05-18 13:45:28 UTC) #25

Bernhard Bauer

https://codereview.chromium.org/1128043007/diff/1/net/android/http_android_auth_negotiate.cc File net/android/http_android_auth_negotiate.cc (right): https://codereview.chromium.org/1128043007/diff/1/net/android/http_android_auth_negotiate.cc#newcode13 net/android/http_android_auth_negotiate.cc:13: #include "jni/HttpNegotiateAuthenticator_jni.h" On 2015/05/18 13:45:27, aberent wrote: > On ...

5 years, 7 months ago (2015-05-18 15:11:54 UTC) #26

aberent

https://codereview.chromium.org/1128043007/diff/1/net/android/http_android_auth_negotiate.cc File net/android/http_android_auth_negotiate.cc (right): https://codereview.chromium.org/1128043007/diff/1/net/android/http_android_auth_negotiate.cc#newcode13 net/android/http_android_auth_negotiate.cc:13: #include "jni/HttpNegotiateAuthenticator_jni.h" On 2015/05/18 at 15:11:53, Bernhard Bauer wrote: ...

5 years, 7 months ago (2015-05-18 17:07:31 UTC) #27

https://codereview.chromium.org/1128043007/diff/1/net/android/http_android_au...
File net/android/http_android_auth_negotiate.cc (right):

https://codereview.chromium.org/1128043007/diff/1/net/android/http_android_au...
net/android/http_android_auth_negotiate.cc:13: #include
"jni/HttpNegotiateAuthenticator_jni.h"
On 2015/05/18 at 15:11:53, Bernhard Bauer wrote:
> On 2015/05/18 13:45:27, aberent wrote:
> > On 2015/05/15 15:06:16, Bernhard Bauer wrote:
> > > The generated JNI include usually goes at the end.
> > 
> > The pre-submit checks complain if I move it.
> 
> Hm, in that case let's remove the empty lines. Either this is a regular
include and gets treated like any other include, or it's not.
Done.

https://codereview.chromium.org/1128043007/diff/80001/net/android/http_androi...
File net/android/http_android_auth_negotiate.h (right):

https://codereview.chromium.org/1128043007/diff/80001/net/android/http_androi...
net/android/http_android_auth_negotiate.h:40: // The function may return
|IO_PENDING|, and call |callback| on completion.
On 2015/05/18 at 15:11:53, Bernhard Bauer wrote:
> On 2015/05/18 13:45:28, aberent wrote:
> > On 2015/05/15 at 15:06:16, Bernhard Bauer wrote:
> > > Do we want to mention that the callback will not end up being called if
this
> > object is destroyed before?
> > 
> > Other similar examples typically don't. I would tend to assume this. See,
for
> > example, Resolver in
> >
https://cs.corp.google.com/#clankium/src/net/dns/host_resolver.h&q=host_resol....
> > 
> > Is there any particular reason for concern in this case?
> 
> Not in this particular case, but it's good to be able to guarantee a caller
that the callback they pass only has to be valid as long as the object lives
(which is necessary for using base::Unretained, for example).
> 
> I like the way the comment on HostResolver is phrased, BTW. Can we use that
here as well?

Done.
> 
> > > 
> > > Also, now that doing authentication might not lock up the IO thread
anymore,
> > do we have to deal with more than one request calling this method before the
> > initial one is finished?
> > 
> > Other stages of the authentication process are already potentially
asynchronous
> > (for example resolving the canonical name), The existing code already deals
with
> > this, and which stage failed to complete synchronously isn't visible outside
> > HttpAuthHandlerNegotiate. I don't think making this step (potentially)
> > asynchronous introduces any new problems.
> > 
> 
> OK, good. Can we add a DCHECK just to make sure?
No obvious way in AndroidAuthNegotiate. HttpAuthHandlerNegotiate (which is the
owner and only caller of AndroidAuthNegotiate) already contains DCHECKs that
will fail if callers attempt to start a second authentication with the same
object.

https://codereview.chromium.org/1128043007/diff/100001/net/android/http_andro...
File net/android/http_android_auth_negotiate.h (right):

https://codereview.chromium.org/1128043007/diff/100001/net/android/http_andro...
net/android/http_android_auth_negotiate.h:53: // to act as the user, such as an
IIS server retrieiving data from a
On 2015/05/18 at 15:11:53, Bernhard Bauer wrote:
> Nit: "retrieving"

Fixed here, and in the Windows and Posix versions of this file (I copied this
from the Posix version)

https://codereview.chromium.org/1128043007/diff/100001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java
(right):

https://codereview.chromium.org/1128043007/diff/100001/net/android/java/src/o...
net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:39:
private String mAccountType;
On 2015/05/18 at 15:11:53, Bernhard Bauer wrote:
> Make this final?

Done.

https://codereview.chromium.org/1128043007/diff/100001/net/android/java/src/o...
net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:47: *
object creator
On 2015/05/18 at 15:11:53, Bernhard Bauer wrote:
> Nit: Capitalize and add punctuation (or remove it).

Removed.

https://codereview.chromium.org/1128043007/diff/100001/net/android/java/src/o...
net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:65: assert
mAccountType != null;
On 2015/05/18 at 15:11:53, Bernhard Bauer wrote:
> Assert this in the constructor?
Yes, although actually I should be testing for empty as well as null. Done.

aberent

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

5 years, 7 months ago (2015-05-20 09:17:11 UTC) #28

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1128043007/120001

5 years, 7 months ago (2015-05-20 09:17:58 UTC) #29

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

5 years, 7 months ago (2015-05-20 10:19:30 UTC) #30

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

5 years, 7 months ago (2015-05-20 10:19:32 UTC) #31

Bernhard Bauer

https://codereview.chromium.org/1128043007/diff/80001/net/android/http_android_auth_negotiate.h File net/android/http_android_auth_negotiate.h (right): https://codereview.chromium.org/1128043007/diff/80001/net/android/http_android_auth_negotiate.h#newcode13 net/android/http_android_auth_negotiate.h:13: Remove empty line? https://codereview.chromium.org/1128043007/diff/80001/net/android/http_android_auth_negotiate.h#newcode40 net/android/http_android_auth_negotiate.h:40: // The function may ...

5 years, 6 months ago (2015-06-01 14:51:13 UTC) #32

https://codereview.chromium.org/1128043007/diff/80001/net/android/http_androi...
File net/android/http_android_auth_negotiate.h (right):

https://codereview.chromium.org/1128043007/diff/80001/net/android/http_androi...
net/android/http_android_auth_negotiate.h:13: 
Remove empty line?

https://codereview.chromium.org/1128043007/diff/80001/net/android/http_androi...
net/android/http_android_auth_negotiate.h:40: // The function may return
|IO_PENDING|, and call |callback| on completion.
On 2015/05/18 17:07:30, aberent wrote:
> On 2015/05/18 at 15:11:53, Bernhard Bauer wrote:
> > On 2015/05/18 13:45:28, aberent wrote:
> > > On 2015/05/15 at 15:06:16, Bernhard Bauer wrote:
> > > > Do we want to mention that the callback will not end up being called if
> this
> > > object is destroyed before?
> > > 
> > > Other similar examples typically don't. I would tend to assume this. See,
> for
> > > example, Resolver in
> > >
>
https://cs.corp.google.com/#clankium/src/net/dns/host_resolver.h&q=host_resol....
> > > 
> > > Is there any particular reason for concern in this case?
> > 
> > Not in this particular case, but it's good to be able to guarantee a caller
> that the callback they pass only has to be valid as long as the object lives
> (which is necessary for using base::Unretained, for example).
> > 
> > I like the way the comment on HostResolver is phrased, BTW. Can we use that
> here as well?
> 
> Done.
> > 
> > > > 
> > > > Also, now that doing authentication might not lock up the IO thread
> anymore,
> > > do we have to deal with more than one request calling this method before
the
> > > initial one is finished?
> > > 
> > > Other stages of the authentication process are already potentially
> asynchronous
> > > (for example resolving the canonical name), The existing code already
deals
> with
> > > this, and which stage failed to complete synchronously isn't visible
outside
> > > HttpAuthHandlerNegotiate. I don't think making this step (potentially)
> > > asynchronous introduces any new problems.
> > > 
> > 
> > OK, good. Can we add a DCHECK just to make sure?
> No obvious way in AndroidAuthNegotiate. HttpAuthHandlerNegotiate (which is the
> owner and only caller of AndroidAuthNegotiate) already contains DCHECKs that
> will fail if callers attempt to start a second authentication with the same
> object.

You could add a DCHECK(completion_callback_.is_null()) and reset the callback
after running it. But if you feel that the existing DCHECK in the caller is
sufficient, that's fine with me.

https://codereview.chromium.org/1128043007/diff/120001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java
(right):

https://codereview.chromium.org/1128043007/diff/120001/net/android/java/src/o...
net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:42: assert
accountType != null && !accountType.isEmpty();
You could combine these two checks with android.text.TextUtils.isEmpty().

aberent

https://codereview.chromium.org/1128043007/diff/80001/net/android/http_android_auth_negotiate.h File net/android/http_android_auth_negotiate.h (right): https://codereview.chromium.org/1128043007/diff/80001/net/android/http_android_auth_negotiate.h#newcode13 net/android/http_android_auth_negotiate.h:13: On 2015/06/01 14:51:13, Bernhard Bauer wrote: > Remove empty ...

5 years, 6 months ago (2015-06-10 15:48:56 UTC) #33

https://codereview.chromium.org/1128043007/diff/80001/net/android/http_androi...
File net/android/http_android_auth_negotiate.h (right):

https://codereview.chromium.org/1128043007/diff/80001/net/android/http_androi...
net/android/http_android_auth_negotiate.h:13: 
On 2015/06/01 14:51:13, Bernhard Bauer wrote:
> Remove empty line?

Done.

https://codereview.chromium.org/1128043007/diff/80001/net/android/http_androi...
net/android/http_android_auth_negotiate.h:40: // The function may return
|IO_PENDING|, and call |callback| on completion.
On 2015/06/01 14:51:13, Bernhard Bauer wrote:
> On 2015/05/18 17:07:30, aberent wrote:
> > On 2015/05/18 at 15:11:53, Bernhard Bauer wrote:
> > > On 2015/05/18 13:45:28, aberent wrote:
> > > > On 2015/05/15 at 15:06:16, Bernhard Bauer wrote:
> > > > > Do we want to mention that the callback will not end up being called
if
> > this
> > > > object is destroyed before?
> > > > 
> > > > Other similar examples typically don't. I would tend to assume this.
See,
> > for
> > > > example, Resolver in
> > > >
> >
>
https://cs.corp.google.com/#clankium/src/net/dns/host_resolver.h&q=host_resol....
> > > > 
> > > > Is there any particular reason for concern in this case?
> > > 
> > > Not in this particular case, but it's good to be able to guarantee a
caller
> > that the callback they pass only has to be valid as long as the object lives
> > (which is necessary for using base::Unretained, for example).
> > > 
> > > I like the way the comment on HostResolver is phrased, BTW. Can we use
that
> > here as well?
> > 
> > Done.
> > > 
> > > > > 
> > > > > Also, now that doing authentication might not lock up the IO thread
> > anymore,
> > > > do we have to deal with more than one request calling this method before
> the
> > > > initial one is finished?
> > > > 
> > > > Other stages of the authentication process are already potentially
> > asynchronous
> > > > (for example resolving the canonical name), The existing code already
> deals
> > with
> > > > this, and which stage failed to complete synchronously isn't visible
> outside
> > > > HttpAuthHandlerNegotiate. I don't think making this step (potentially)
> > > > asynchronous introduces any new problems.
> > > > 
> > > 
> > > OK, good. Can we add a DCHECK just to make sure?
> > No obvious way in AndroidAuthNegotiate. HttpAuthHandlerNegotiate (which is
the
> > owner and only caller of AndroidAuthNegotiate) already contains DCHECKs that
> > will fail if callers attempt to start a second authentication with the same
> > object.
> 
> You could add a DCHECK(completion_callback_.is_null()) and reset the callback
> after running it. But if you feel that the existing DCHECK in the caller is
> sufficient, that's fine with me.

Done, together with a couple of other checks.

https://codereview.chromium.org/1128043007/diff/120001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java
(right):

https://codereview.chromium.org/1128043007/diff/120001/net/android/java/src/o...
net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:42: assert
accountType != null && !accountType.isEmpty();
On 2015/06/01 14:51:13, Bernhard Bauer wrote:
> You could combine these two checks with android.text.TextUtils.isEmpty().

Done.

aberent

aberent@chromium.org changed reviewers: + cbentzel@chromium.org, zea@chromium.org

5 years, 6 months ago (2015-06-10 16:17:03 UTC) #34

aberent

cbentzel@chromium.org: Please review all, but in particular changes in net. zea@chromium.org: Please review change in ...

5 years, 6 months ago (2015-06-10 16:17:04 UTC) #35

aberent

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

5 years, 6 months ago (2015-06-10 16:25:58 UTC) #36

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1128043007/240001

5 years, 6 months ago (2015-06-10 16:26:33 UTC) #37

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

5 years, 6 months ago (2015-06-10 16:43:43 UTC) #38

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_dbg_recipe/builds/85395)

5 years, 6 months ago (2015-06-10 16:43:44 UTC) #39

Bernhard Bauer

+Ryan FYI https://codereview.chromium.org/1128043007/diff/240001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/1128043007/diff/240001/chrome/common/pref_names.cc#newcode1711 chrome/common/pref_names.cc:1711: // String that specifies the name of ...

5 years, 6 months ago (2015-06-10 17:11:03 UTC) #40

cbentzel

On 2015/06/10 18:42:36, Nicolas Zea wrote: > mcs probe LGTM I'll take a look at ...

5 years, 6 months ago (2015-06-11 02:54:08 UTC) #42

cbentzel

On 2015/06/10 18:42:36, Nicolas Zea wrote: > mcs probe LGTM I'll take a look at ...

5 years, 6 months ago (2015-06-11 02:54:12 UTC) #43

cbentzel

On 2015/06/11 02:54:12, cbentzel wrote: > On 2015/06/10 18:42:36, Nicolas Zea wrote: > > mcs ...

5 years, 6 months ago (2015-06-11 03:39:25 UTC) #44

cbentzel

On 2015/06/11 03:39:25, cbentzel wrote: > On 2015/06/11 02:54:12, cbentzel wrote: > > On 2015/06/10 ...

5 years, 6 months ago (2015-06-11 15:41:50 UTC) #45

aberent

On 2015/06/11 15:41:50, cbentzel wrote: > On 2015/06/11 03:39:25, cbentzel wrote: > > On 2015/06/11 ...

5 years, 6 months ago (2015-06-11 18:10:23 UTC) #46

On 2015/06/11 15:41:50, cbentzel wrote:
> On 2015/06/11 03:39:25, cbentzel wrote:
> > On 2015/06/11 02:54:12, cbentzel wrote:
> > > On 2015/06/10 18:42:36, Nicolas Zea wrote:
> > > > mcs probe LGTM
> > > 
> > > I'll take a look at net/ tomorrow morning but may need to delegate it out
> > (both
> > > due to time constraints as well as better technical folks to review)
> > 
> > Two behavior questions before getting into nitty gritty code:
> > 
> > - This looks like it uses the POSIX URLSecurityManager which restricts
> > password-less authentication to whitelisted-only servers as well as proxies.
> Is
> > that your intent?
> 
> It looks like this is covered in the design document (thanks!) and you plan to
> require policy to be set for which sites to whitelist, rather than try to do
> intranet detection based on hostname.

Yes, precisely.
> 
> > 
> > - Is there a way to discover that this feature exists without requiring
prefs
> to
> > be set?

I don't think so at present. The next CL will add policies that allow the prefs
to be set, and these will, of course, be visible.
> 
> Still interested in this. Looks like main thing is discovering accountType for
> AccountManager. Not quit sure what this is used for.

The account type is used by the Android AccountManager to find the Authenticator
app. This is a standard Android mechanism.
> 
> One last question - is there a possibility that a SPNEGO implementation may
end
> up displaying UI at some point - for example, to type in a PIN or something
> else? If so, how does that interact with Chrome's UI?

Yes, the SPNEGO implementations are independent apps, written by third parties,
so can do anything any other Android app can do. If it displays a UI, then this
will overlay Chrome's UI, but the user can get back to Chrome's UI through the
recent applications (or on Android L or M, recent documents) list. We might want
to recommend that, if a SPNEGO authenticator needs user interaction, it should
simply an Android notification that the user can open, but we have no way of
enforcing this.

cbentzel

On 2015/06/11 18:10:23, aberent wrote: > On 2015/06/11 15:41:50, cbentzel wrote: > > On 2015/06/11 ...

5 years, 6 months ago (2015-06-11 20:32:36 UTC) #47

On 2015/06/11 18:10:23, aberent wrote:
> On 2015/06/11 15:41:50, cbentzel wrote:
> > On 2015/06/11 03:39:25, cbentzel wrote:
> > > On 2015/06/11 02:54:12, cbentzel wrote:
> > > > On 2015/06/10 18:42:36, Nicolas Zea wrote:
> > > > > mcs probe LGTM
> > > > 
> > > > I'll take a look at net/ tomorrow morning but may need to delegate it
out
> > > (both
> > > > due to time constraints as well as better technical folks to review)
> > > 
> > > Two behavior questions before getting into nitty gritty code:
> > > 
> > > - This looks like it uses the POSIX URLSecurityManager which restricts
> > > password-less authentication to whitelisted-only servers as well as
proxies.
> > Is
> > > that your intent?
> > 
> > It looks like this is covered in the design document (thanks!) and you plan
to
> > require policy to be set for which sites to whitelist, rather than try to do
> > intranet detection based on hostname.
> 
> Yes, precisely.
> > 
> > > 
> > > - Is there a way to discover that this feature exists without requiring
> prefs
> > to
> > > be set?
> 
> I don't think so at present. The next CL will add policies that allow the
prefs
> to be set, and these will, of course, be visible.
> > 
> > Still interested in this. Looks like main thing is discovering accountType
for
> > AccountManager. Not quit sure what this is used for.
> 
> The account type is used by the Android AccountManager to find the
Authenticator
> app. This is a standard Android mechanism.
> > 
> > One last question - is there a possibility that a SPNEGO implementation may
> end
> > up displaying UI at some point - for example, to type in a PIN or something
> > else? If so, how does that interact with Chrome's UI?
> 
> Yes, the SPNEGO implementations are independent apps, written by third
parties,
> so can do anything any other Android app can do. If it displays a UI, then
this
> will overlay Chrome's UI, but the user can get back to Chrome's UI through the
> recent applications (or on Android L or M, recent documents) list. We might
want
> to recommend that, if a SPNEGO authenticator needs user interaction, it should
> simply an Android notification that the user can open, but we have no way of
> enforcing this.

Thanks for all the responses.

cbentzel

Overall structure looks good. I have not taken a pass over unit tests yet. Could ...

5 years, 6 months ago (2015-06-11 20:51:44 UTC) #48

aberent

https://codereview.chromium.org/1128043007/diff/240001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/1128043007/diff/240001/chrome/common/pref_names.cc#newcode1711 chrome/common/pref_names.cc:1711: // String that specifies the name of Android account ...

5 years, 6 months ago (2015-06-15 15:52:20 UTC) #49

aberent

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

5 years, 6 months ago (2015-06-15 15:53:01 UTC) #50

aberent

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org Link to the ...

5 years, 6 months ago (2015-06-15 15:53:02 UTC) #51

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1128043007/260001

5 years, 6 months ago (2015-06-15 15:53:47 UTC) #52

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

5 years, 6 months ago (2015-06-15 16:07:12 UTC) #53

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_gn_dbg/builds/82113)

5 years, 6 months ago (2015-06-15 16:07:14 UTC) #54

aberent

On 2015/06/11 20:51:44, cbentzel wrote: > Overall structure looks good. I have not taken a ...

5 years, 6 months ago (2015-06-15 16:15:24 UTC) #55

Bernhard Bauer

https://codereview.chromium.org/1128043007/diff/260001/net/http/http_auth_handler_negotiate.cc File net/http/http_auth_handler_negotiate.cc (right): https://codereview.chromium.org/1128043007/diff/260001/net/http/http_auth_handler_negotiate.cc#newcode91 net/http/http_auth_handler_negotiate.cc:91: // TODO(ahendrickHson): Move towards model of parsing in the ...

5 years, 6 months ago (2015-06-15 16:43:11 UTC) #56

aberent

https://codereview.chromium.org/1128043007/diff/240001/net/android/http_android_auth_negotiate.cc File net/android/http_android_auth_negotiate.cc (right): https://codereview.chromium.org/1128043007/diff/240001/net/android/http_android_auth_negotiate.cc#newcode60 net/android/http_android_auth_negotiate.cc:60: // Verify the challenge's auth-scheme. On 2015/06/15 15:52:19, aberent ...

5 years, 6 months ago (2015-06-15 16:57:47 UTC) #57

cbentzel

On 2015/06/15 16:15:24, aberent wrote: > On 2015/06/11 20:51:44, cbentzel wrote: > > Overall structure ...

5 years, 6 months ago (2015-06-15 22:18:02 UTC) #58

Ryan Sleevi

rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org

5 years, 6 months ago (2015-06-16 01:07:44 UTC) #59

Ryan Sleevi

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_negotiate_android.cc File net/android/http_auth_negotiate_android.cc (right): https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_negotiate_android.cc#newcode65 net/android/http_auth_negotiate_android.cc:65: auth_token_ = auth_token; nit: a newline between 64 & ...

5 years, 6 months ago (2015-06-16 01:07:47 UTC) #60

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
File net/android/http_auth_negotiate_android.cc (right):

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
net/android/http_auth_negotiate_android.cc:65: auth_token_ = auth_token;
nit: a newline between 64 & 65 can help readability

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
net/android/http_auth_negotiate_android.cc:67: callback_task_runner_ =
base::MessageLoop::current()->task_runner();
BUG: You should be using base::ThreadTaskRunnerHandle::Get() over
base::MessageLoop

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
net/android/http_auth_negotiate_android.cc:79: return result ? ERR_IO_PENDING :
ERR_INVALID_AUTH_CREDENTIALS;
Is this the right error code? If it synchronously fails, is this indicative that
the user authenticator failed, or there was some fatal JNI issue?

Just wanting to make sure a status code like
ERR_UNDOCUMENTED_SECURITY_LIBRARY_STATUS doesn't apply

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
net/android/http_auth_negotiate_android.cc:103: callback_task_runner_->PostTask(
DANGEROUS PATTERN: This is an object that lives on the IO thread, that you're
trying to ensure is thread-safe for use on the IO thread (hence line 68/69), but
which you're accessing a data member on the UI thread - this is a common source
of bugs.

I've seen a couple different patterns for JNI calls that involve asynchronicity,
and I'm not sure which is the 'canonical'/preferred route.

Option 1:
The C++ call dispatches the JNI call on a worker thread, the JNI call is a
blocking method, and the Java implementation is allowed to dispatch (e.g. to a
Runner) to implement any asynchronicity it needs. Example is Android cert
verification (cert_verify_proc_android)

Option 2:
Use an explicit Delegate interface for the Java side, implemented by some C++
helper class. The helper class lives on any thread (can be tough to get
ownership right) and then dispatches to the appropriate thread. The
NetworkChangeNotifierAndroid is an example of this.



What I'd particularly encourage here is that you use a thread-safe,
implementation-only container that expresses the "lives on any thread" semantics
(e.g. a "::Core" class, as used by SSLClientSocketNSS or TCPClientSocketWin),
with the "lives on any thread" data members, and then that marshals things back
to appropriate threads and objects here.

The goal of this, though more boiler plate, is to try to make explicit the
object lifetimes (e.g. the HttpAuthNegotiateAndroid could live and die on a
single thread, safely, and for anyone) for consumers, and the member lifetimes
for future maintainers of this code (for example, if someone botched things and
called GenerateAuthToken twice, you'd potentially have thread-unsafe data races
that may not show until release builds)

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
File net/android/http_auth_negotiate_android.h (right):

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
net/android/http_auth_negotiate_android.h:16: #include "base/task_runner.h"
You can forward declare this

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
net/android/http_auth_negotiate_android.h:26: class NET_EXPORT_PRIVATE
HttpAuthNegotiateAndroid {
Could you comment this class?

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
net/android/http_auth_negotiate_android.h:28: HttpAuthNegotiateAndroid(const
std::string& account_type);
Could you document this constructor?

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
net/android/http_auth_negotiate_android.h:30: bool Init();
Could you add a newline between 29/30

And document what these methods do?

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
net/android/http_auth_negotiate_android.h:67: void Delegate();
The naming of this is weird, especially when this mutates state. Should this be
SetDelegationAllowed()? Something more expressive?

And it's unclear why you inlined 70-73 but not this method, which is
substantially cheaper.

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
net/android/http_auth_negotiate_android.h:69: static bool Register(JNIEnv* env);
Most of the code in Android I've seen makes this the first publicly declared
member.

They've also split up registration from implementation / JNI callbacks.

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
File net/android/http_auth_negotiate_android_unittest.cc (right):

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
net/android/http_auth_negotiate_android_unittest.cc:29: std::string auth_token;
API DANGER: This sort of highlights the danger in your std::string* parameter
taking. The issue is that the string has a shorter lifetime than
HttpAuthNegotiateAndroid (line 24), except the API contract you've used for
asycnhronicity is that the std::string must live until the callback is satisfied
or until the HttpAuthNegotiateAndroid is destructed, whatever comes first.

If, in between lines 24 and 29, you had an object that on destruction pumped the
message loop (e.g. for idle tasks), you could end up crashing on line 40 (when
things go out of scope) if the EXPECT_EQ's weren't satisfied.

To be clear, this isn't a problem in the existing code, because you don't, but
it shows how easily it is to write dangerous code using this pattern.

You could reorder such that the string was declared before the
HttpAuthNegotiateAndroid, which would solve the problem, or possibly contemplate
a more explicit lifetime contract than the object itself (in //net, we use the
scoped_ptr<Request> pattern as the 'holder to an unsatisfied asynchronous
request', and which as long as its valid, so too must the pointer arguments to
the asynchronous request be valid)

This implicitly encourages safer programming, since you wouldn't allocate the
request until line 35 (when you're calling things), and by needing to allocate
it at the same time as this string, be forced to think about lifetimes.

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
net/android/http_auth_negotiate_android_unittest.cc:39: EXPECT_EQ("Negotiate
DummyToken", auth_token);
It'd be much better to use a net::TestCompletionCallback() here, which will
handle quitting the loop, but also avoid spinning the loop in the event of
synchronous returns.

For example, if the expectation on line 32 failed, this test wouldn't fail - it
would deadlock to crash. You could make line 32 an ASSERT_EQ (so that the test
failed sooner), but it seems 'cleaner' to just use the
net::TestCompletionCallback.

https://codereview.chromium.org/1128043007/diff/260001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java
(right):

https://codereview.chromium.org/1128043007/diff/260001/net/android/java/src/o...
net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:79:
mFuture = am.getAuthTokenByFeatures(mAccountType, authTokenType, features,
activity, null,
Just to make sure: Can exceptions be thrown by any of these other methods (line
68, 70, 75, 76, 77, 79)?

Juuuuust wanting to make sure; IIRC, the answer is "no", because the cmopiler
would have yelled at you for the absent throw spec on line 65, but I wanted to
confirm.

https://codereview.chromium.org/1128043007/diff/260001/net/http/http_auth_han...
File net/http/http_auth_handler_factory.h (right):

https://codereview.chromium.org/1128043007/diff/260001/net/http/http_auth_han...
net/http/http_auth_handler_factory.h:181: const std::string&
auth_android_negotiate_account_type,
This certainly feels like a layering smell - to expose Android-specific data
types in a platform-generic class. Then again, not unprecedented (e.g. line
171/172)

Certainly, it seems like it should be documented (line 173).

https://codereview.chromium.org/1128043007/diff/260001/net/http/http_auth_han...
File net/http/http_auth_handler_factory_unittest.cc (right):

https://codereview.chromium.org/1128043007/diff/260001/net/http/http_auth_han...
net/http/http_auth_handler_factory_unittest.cc:175: // Note the default factory
doesn't support KERBEROS on Android
s/KERBEROS/Kerberos/

https://codereview.chromium.org/1128043007/diff/260001/net/http/http_auth_han...
File net/http/http_auth_handler_negotiate.cc (right):

https://codereview.chromium.org/1128043007/diff/260001/net/http/http_auth_han...
net/http/http_auth_handler_negotiate.cc:75: //                     method and
only constructing when valid.
This looks like you just copy-pasted the TODO from 61/62 - seems like that
should still point to Chris.

https://codereview.chromium.org/1128043007/diff/260001/net/http/http_auth_han...
net/http/http_auth_handler_negotiate.cc:189: if
(!base::LowerCaseEqualsASCII(challenge->scheme(), "negotiate"))
DANGER: It's worth noting here that there isn't a guarantee that
challenge->scheme() contains ASCII, but in fact contains untrusted server input.

That is, it could contain something like "Negotiate\0Foo"

As it happens, LowerCaseEqualsASCII 'does the right thing', but I think it may
merit noting the danger

https://codereview.chromium.org/1128043007/diff/260001/net/http/http_auth_han...
net/http/http_auth_handler_negotiate.cc:195: }
Consistent with lines 189/190 and 199/200 (and really, the rest of //net),
remove the braces here

https://codereview.chromium.org/1128043007/diff/260001/net/http/http_auth_han...
net/http/http_auth_handler_negotiate.cc:199: if (!base64_rv)
Does not seem like there's any reason to use a temporary here.

if (!base::Base64Decode(...)))

that avoids the naming nit of base64_rv I was going to leave ;)

aberent

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

5 years, 6 months ago (2015-06-18 15:59:14 UTC) #61

aberent

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org Link to the ...

5 years, 6 months ago (2015-06-18 15:59:15 UTC) #62

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1128043007/280001

5 years, 6 months ago (2015-06-18 16:00:59 UTC) #63

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

5 years, 6 months ago (2015-06-18 16:17:13 UTC) #64

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_gn_dbg/builds/83568) linux_chromium_gn_rel on ...

5 years, 6 months ago (2015-06-18 16:17:15 UTC) #65

aberent

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

5 years, 6 months ago (2015-06-19 15:02:12 UTC) #66

aberent

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org Link to the ...

5 years, 6 months ago (2015-06-19 15:02:13 UTC) #67

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1128043007/300001

5 years, 6 months ago (2015-06-19 15:03:06 UTC) #68

aberent

aberent@chromium.org changed reviewers: + jbudorick@chromium.org

5 years, 6 months ago (2015-06-19 15:06:22 UTC) #69

aberent

jbudorick@chromium.org: Please review changes in build/android/pylib Other reviewers PTAL. Note that I have moved the ...

5 years, 6 months ago (2015-06-19 15:06:25 UTC) #70

jbudorick@chromium.org: Please review changes in build/android/pylib

Other reviewers PTAL. Note that I have moved the net_unittests to running on a
background thread on Android, so that the main (UI) thread is available for Java
callbacks.

Still TODO - add tests for http_auth_handler_negotiate_parse, and take another
look at whether I can now get the http_auth_handler_negotiate tests running on
Android.

https://codereview.chromium.org/1128043007/diff/240001/net/android/http_andro...
File net/android/http_android_auth_negotiate.cc (right):

https://codereview.chromium.org/1128043007/diff/240001/net/android/http_andro...
net/android/http_android_auth_negotiate.cc:60: // Verify the challenge's
auth-scheme.
On 2015/06/15 16:57:47, aberent wrote:
> On 2015/06/15 15:52:19, aberent wrote:
> > On 2015/06/11 20:51:43, cbentzel wrote:
> > > I wonder if this could be moved into a common function. Main concern is
the
> > > first_challenge_ state needing to be preserved. But we now have three very
> > close
> > > copies in HttpAuthGSSAPI and HttpAuthSSPI
> > 
> > Done, or more precisely, moved into http_auth_handler.cc and then refactored
> out
> > of existence. Please take a careful look at my changes to
http_auth_handler.cc
> > to see if I have missed anything.
> 
> Realized that there are problems with my changes here. Please wait for next
> patch set to review.

I have now made a second attempt at this. I have left the ParseChallenge
functions in the Authenticator classes, but have factored out the common code
into new, common functions. See http_auth_negotiate_parse...

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
File net/android/http_auth_negotiate_android.cc (right):

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
net/android/http_auth_negotiate_android.cc:65: auth_token_ = auth_token;
On 2015/06/16 01:07:46, Ryan Sleevi wrote:
> nit: a newline between 64 & 65 can help readability

Done.

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
net/android/http_auth_negotiate_android.cc:67: callback_task_runner_ =
base::MessageLoop::current()->task_runner();
On 2015/06/16 01:07:46, Ryan Sleevi wrote:
> BUG: You should be using base::ThreadTaskRunnerHandle::Get() over
> base::MessageLoop

Done.

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
net/android/http_auth_negotiate_android.cc:79: return result ? ERR_IO_PENDING :
ERR_INVALID_AUTH_CREDENTIALS;
On 2015/06/16 01:07:46, Ryan Sleevi wrote:
> Is this the right error code? If it synchronously fails, is this indicative
that
> the user authenticator failed, or there was some fatal JNI issue?
> 
> Just wanting to make sure a status code like
> ERR_UNDOCUMENTED_SECURITY_LIBRARY_STATUS doesn't apply

Actually the only way it could fail synchronously is if it is somehow called
when there is no current Chrome activity. Due to other changes even this failure
will now come back asynchronously and GenerateAuthToken will now always return
ERR_IO_PENDING.

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
net/android/http_auth_negotiate_android.cc:103: callback_task_runner_->PostTask(
On 2015/06/16 01:07:46, Ryan Sleevi wrote:
> DANGEROUS PATTERN: This is an object that lives on the IO thread, that you're
> trying to ensure is thread-safe for use on the IO thread (hence line 68/69),
but
> which you're accessing a data member on the UI thread - this is a common
source
> of bugs.
> 
> I've seen a couple different patterns for JNI calls that involve
asynchronicity,
> and I'm not sure which is the 'canonical'/preferred route.
> 
> Option 1:
> The C++ call dispatches the JNI call on a worker thread, the JNI call is a
> blocking method, and the Java implementation is allowed to dispatch (e.g. to a
> Runner) to implement any asynchronicity it needs. Example is Android cert
> verification (cert_verify_proc_android)
> 
> Option 2:
> Use an explicit Delegate interface for the Java side, implemented by some C++
> helper class. The helper class lives on any thread (can be tough to get
> ownership right) and then dispatches to the appropriate thread. The
> NetworkChangeNotifierAndroid is an example of this.
> 
> 
> 
> What I'd particularly encourage here is that you use a thread-safe,
> implementation-only container that expresses the "lives on any thread"
semantics
> (e.g. a "::Core" class, as used by SSLClientSocketNSS or TCPClientSocketWin),
> with the "lives on any thread" data members, and then that marshals things
back
> to appropriate threads and objects here.
> 
> The goal of this, though more boiler plate, is to try to make explicit the
> object lifetimes (e.g. the HttpAuthNegotiateAndroid could live and die on a
> single thread, safely, and for anyone) for consumers, and the member lifetimes
> for future maintainers of this code (for example, if someone botched things
and
> called GenerateAuthToken twice, you'd potentially have thread-unsafe data
races
> that may not show until release builds)

Fixed with a helper class, similar to the ::Core model, but slightly modified to
allow for calling and thread selection from Java.

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
File net/android/http_auth_negotiate_android.h (right):

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
net/android/http_auth_negotiate_android.h:16: #include "base/task_runner.h"
On 2015/06/16 01:07:46, Ryan Sleevi wrote:
> You can forward declare this

Done.

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
net/android/http_auth_negotiate_android.h:26: class NET_EXPORT_PRIVATE
HttpAuthNegotiateAndroid {
On 2015/06/16 01:07:46, Ryan Sleevi wrote:
> Could you comment this class?

Done.

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
net/android/http_auth_negotiate_android.h:28: HttpAuthNegotiateAndroid(const
std::string& account_type);
On 2015/06/16 01:07:46, Ryan Sleevi wrote:
> Could you document this constructor?

Done.

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
net/android/http_auth_negotiate_android.h:30: bool Init();
On 2015/06/16 01:07:46, Ryan Sleevi wrote:
> Could you add a newline between 29/30
> 
> And document what these methods do?

Done.

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
net/android/http_auth_negotiate_android.h:67: void Delegate();
On 2015/06/16 01:07:46, Ryan Sleevi wrote:
> The naming of this is weird, especially when this mutates state. Should this
be
> SetDelegationAllowed()? Something more expressive?
> 
> And it's unclear why you inlined 70-73 but not this method, which is
> substantially cheaper.

The naming of the members of this class has to match that of the existing
classes for the other OSs, e.g. HttpAuthGSSAPI, so that common code can use all
of these.

I also think the inlining of 70-73 was a mistake, and have removed it (none of
this is performance critical, so the code is cleaner without inlining).

Actually, for other reasons, SetServerAuthToken is no longer needed and has been
removed.

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
net/android/http_auth_negotiate_android.h:69: static bool Register(JNIEnv* env);
On 2015/06/16 01:07:46, Ryan Sleevi wrote:
> Most of the code in Android I've seen makes this the first publicly declared
> member.

Done.
> 
> They've also split up registration from implementation / JNI callbacks.

Not clear what you mean by this.

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
File net/android/http_auth_negotiate_android_unittest.cc (right):

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
net/android/http_auth_negotiate_android_unittest.cc:29: std::string auth_token;
On 2015/06/16 01:07:46, Ryan Sleevi wrote:
> API DANGER: This sort of highlights the danger in your std::string* parameter
> taking. The issue is that the string has a shorter lifetime than
> HttpAuthNegotiateAndroid (line 24), except the API contract you've used for
> asycnhronicity is that the std::string must live until the callback is
satisfied
> or until the HttpAuthNegotiateAndroid is destructed, whatever comes first.
> 
> If, in between lines 24 and 29, you had an object that on destruction pumped
the
> message loop (e.g. for idle tasks), you could end up crashing on line 40 (when
> things go out of scope) if the EXPECT_EQ's weren't satisfied.
> 
> To be clear, this isn't a problem in the existing code, because you don't, but
> it shows how easily it is to write dangerous code using this pattern.
> 
> You could reorder such that the string was declared before the
> HttpAuthNegotiateAndroid, which would solve the problem, or possibly
contemplate
> a more explicit lifetime contract than the object itself (in //net, we use the
> scoped_ptr<Request> pattern as the 'holder to an unsatisfied asynchronous
> request', and which as long as its valid, so too must the pointer arguments to
> the asynchronous request be valid)
> 
> This implicitly encourages safer programming, since you wouldn't allocate the
> request until line 35 (when you're calling things), and by needing to allocate
> it at the same time as this string, be forced to think about lifetimes.
I agree that the std::string* parameter is horrible and dangerous (and was aware
of this when I wrote the code), but it is built into all the Http authenticators
(see
https://cs.corp.google.com/#clankium/src/net/http/http_auth_handler.h&q=http_...)
and would require going considerably beyond the scope of the current CL to fix.
I have moved this declaration upward however.

https://codereview.chromium.org/1128043007/diff/260001/net/android/http_auth_...
net/android/http_auth_negotiate_android_unittest.cc:39: EXPECT_EQ("Negotiate
DummyToken", auth_token);
On 2015/06/16 01:07:46, Ryan Sleevi wrote:
> It'd be much better to use a net::TestCompletionCallback() here, which will
> handle quitting the loop, but also avoid spinning the loop in the event of
> synchronous returns.
> 
> For example, if the expectation on line 32 failed, this test wouldn't fail -
it
> would deadlock to crash. You could make line 32 an ASSERT_EQ (so that the test
> failed sooner), but it seems 'cleaner' to just use the
> net::TestCompletionCallback.

Done.

https://codereview.chromium.org/1128043007/diff/260001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java
(right):

https://codereview.chromium.org/1128043007/diff/260001/net/android/java/src/o...
net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:79:
mFuture = am.getAuthTokenByFeatures(mAccountType, authTokenType, features,
activity, null,
On 2015/06/16 01:07:46, Ryan Sleevi wrote:
> Just to make sure: Can exceptions be thrown by any of these other methods
(line
> 68, 70, 75, 76, 77, 79)?
> 
> Juuuuust wanting to make sure; IIRC, the answer is "no", because the cmopiler
> would have yelled at you for the absent throw spec on line 65, but I wanted to
> confirm.

The only things they can, in theory, throw are exceptions of unchecked exception
types, such as NullPointerException, and I think I have protected against these.
If not, then these are generally programming errors, and I should fix these.

https://codereview.chromium.org/1128043007/diff/260001/net/http/http_auth_han...
File net/http/http_auth_handler_factory.h (right):

https://codereview.chromium.org/1128043007/diff/260001/net/http/http_auth_han...
net/http/http_auth_handler_factory.h:181: const std::string&
auth_android_negotiate_account_type,
On 2015/06/16 01:07:46, Ryan Sleevi wrote:
> This certainly feels like a layering smell - to expose Android-specific data
> types in a platform-generic class. Then again, not unprecedented (e.g. line
> 171/172)
> 
> Certainly, it seems like it should be documented (line 173).
I don't see that it is very different from gssapi_library_name, which is Posix
specific, or, with a different meaning, Windows specific.

I did wonder about changing the gssapi_library_name to something more generic
instead, such as negotiate_provider_name, and using this for both the library
name on Posix and Windows and for the account name on Android, but this didn't
feel quite right.

I have updated the function documentation.

Note that the old comment on gssapi_library_name is wrong, it can be used on
Windows to set the name of the SPPI library.

https://codereview.chromium.org/1128043007/diff/260001/net/http/http_auth_han...
File net/http/http_auth_handler_factory_unittest.cc (right):

https://codereview.chromium.org/1128043007/diff/260001/net/http/http_auth_han...
net/http/http_auth_handler_factory_unittest.cc:175: // Note the default factory
doesn't support KERBEROS on Android
On 2015/06/16 01:07:46, Ryan Sleevi wrote:
> s/KERBEROS/Kerberos/

Done.

https://codereview.chromium.org/1128043007/diff/260001/net/http/http_auth_han...
File net/http/http_auth_handler_negotiate.cc (right):

https://codereview.chromium.org/1128043007/diff/260001/net/http/http_auth_han...
net/http/http_auth_handler_negotiate.cc:75: //                     method and
only constructing when valid.
On 2015/06/16 01:07:47, Ryan Sleevi wrote:
> This looks like you just copy-pasted the TODO from 61/62 - seems like that
> should still point to Chris.

Actually copied it from 91/92, but happy to switch this one Chris. Done.

https://codereview.chromium.org/1128043007/diff/260001/net/http/http_auth_han...
net/http/http_auth_handler_negotiate.cc:91: // TODO(ahendrickHson): Move towards
model of parsing in the factory
On 2015/06/15 16:43:11, Bernhard Bauer wrote:
> Nit: Capital H snuck in here somehow :)

Done.

https://codereview.chromium.org/1128043007/diff/260001/net/http/http_auth_han...
net/http/http_auth_handler_negotiate.cc:189: if
(!base::LowerCaseEqualsASCII(challenge->scheme(), "negotiate"))
On 2015/06/16 01:07:47, Ryan Sleevi wrote:
> DANGER: It's worth noting here that there isn't a guarantee that
> challenge->scheme() contains ASCII, but in fact contains untrusted server
input.
> 
> That is, it could contain something like "Negotiate\0Foo"
> 
> As it happens, LowerCaseEqualsASCII 'does the right thing', but I think it may
> merit noting the danger

Note that there is substantial refactoring of this in the next version. The body
of it has moved into http_auth_negotiate_parse.

Done.

https://codereview.chromium.org/1128043007/diff/260001/net/http/http_auth_han...
net/http/http_auth_handler_negotiate.cc:195: }
On 2015/06/16 01:07:47, Ryan Sleevi wrote:
> Consistent with lines 189/190 and 199/200 (and really, the rest of //net),
> remove the braces here

Done.

https://codereview.chromium.org/1128043007/diff/260001/net/http/http_auth_han...
net/http/http_auth_handler_negotiate.cc:199: if (!base64_rv)
On 2015/06/16 01:07:47, Ryan Sleevi wrote:
> Does not seem like there's any reason to use a temporary here.
> 
> if (!base::Base64Decode(...)))
> 
> that avoids the naming nit of base64_rv I was going to leave ;)

Done.

https://codereview.chromium.org/1128043007/diff/260001/net/http/http_auth_han...
File net/http/http_auth_handler_negotiate.h (right):

https://codereview.chromium.org/1128043007/diff/260001/net/http/http_auth_han...
net/http/http_auth_handler_negotiate.h:18: #elif defined(OS_WIN)
On 2015/06/15 16:43:11, Bernhard Bauer wrote:
> Change these into separate #if/#endif blocks as well?

I think this is a different case. These really are three alternatives,
corresponding to three AuthSystem definitions, and it makes no sense to have
more than one of them. The account_type_ and auth_library_ are however (as Chris
pointed out) logically independent, and it is effectively a coincidence that no
OS needs both of them.

jbudorick

Please add similar logic here: https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pylib/gtest/gtest_test_instance.py&l=153 https://codereview.chromium.org/1128043007/diff/300001/build/android/pylib/gtest/test_package_apk.py File build/android/pylib/gtest/test_package_apk.py (right): https://codereview.chromium.org/1128043007/diff/300001/build/android/pylib/gtest/test_package_apk.py#newcode39 build/android/pylib/gtest/test_package_apk.py:39: if suite_name == ...

5 years, 6 months ago (2015-06-19 15:15:19 UTC) #71

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

5 years, 6 months ago (2015-06-19 15:15:36 UTC) #72

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromium_gn_compile_dbg/builds/83373)

5 years, 6 months ago (2015-06-19 15:15:38 UTC) #73

aberent

https://codereview.chromium.org/1128043007/diff/300001/build/android/pylib/gtest/test_package_apk.py File build/android/pylib/gtest/test_package_apk.py (right): https://codereview.chromium.org/1128043007/diff/300001/build/android/pylib/gtest/test_package_apk.py#newcode39 build/android/pylib/gtest/test_package_apk.py:39: if suite_name == 'content_browsertests': On 2015/06/19 15:15:19, jbudorick wrote: ...

5 years, 6 months ago (2015-06-19 17:59:40 UTC) #74

aberent

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

5 years, 6 months ago (2015-06-19 17:59:54 UTC) #75

aberent

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org Link to the ...

5 years, 6 months ago (2015-06-19 17:59:55 UTC) #76

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1128043007/320001

5 years, 6 months ago (2015-06-19 18:01:32 UTC) #77

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

5 years, 6 months ago (2015-06-19 20:11:04 UTC) #78

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/71812) win_chromium_x64_rel_ng on ...

5 years, 6 months ago (2015-06-19 20:11:06 UTC) #79

aberent

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

5 years, 6 months ago (2015-06-22 09:25:48 UTC) #80

aberent

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org Link to the ...

5 years, 6 months ago (2015-06-22 09:25:48 UTC) #81

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1128043007/340001

5 years, 6 months ago (2015-06-22 09:26:09 UTC) #82

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

5 years, 6 months ago (2015-06-22 09:58:53 UTC) #83

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_rel_ng/builds/36357)

5 years, 6 months ago (2015-06-22 09:58:54 UTC) #84

aberent

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

5 years, 6 months ago (2015-06-24 15:50:02 UTC) #86

aberent

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org Link to the ...

5 years, 6 months ago (2015-06-24 15:50:02 UTC) #87

jbudorick

On 2015/06/19 at 15:15:19, jbudorick wrote: > Please add similar logic here: https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pylib/gtest/gtest_test_instance.py&l=153 ^ > ...

5 years, 6 months ago (2015-06-24 15:50:10 UTC) #88

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1128043007/380001

5 years, 6 months ago (2015-06-24 15:50:20 UTC) #89

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

5 years, 6 months ago (2015-06-24 16:48:18 UTC) #90

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_dbg_recipe/builds/90177)

5 years, 6 months ago (2015-06-24 16:48:21 UTC) #91

jbudorick

On 2015/06/24 at 15:50:10, jbudorick wrote: > On 2015/06/19 at 15:15:19, jbudorick wrote: > > ...

5 years, 6 months ago (2015-06-24 18:19:22 UTC) #92

aberent

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

5 years, 6 months ago (2015-06-24 18:31:40 UTC) #93

aberent

The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org, zea@chromium.org Link to ...

5 years, 6 months ago (2015-06-24 18:31:40 UTC) #94

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1128043007/400001

5 years, 6 months ago (2015-06-24 18:32:03 UTC) #95

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

5 years, 6 months ago (2015-06-24 19:11:53 UTC) #96

commit-bot: I haz the power

Dry run: Exceeded global retry quota

5 years, 6 months ago (2015-06-24 19:11:56 UTC) #97

aberent

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

5 years, 6 months ago (2015-06-25 13:31:49 UTC) #98

aberent

The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org, zea@chromium.org Link to ...

5 years, 6 months ago (2015-06-25 13:31:50 UTC) #99

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1128043007/420001

5 years, 6 months ago (2015-06-25 13:32:31 UTC) #100

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

5 years, 6 months ago (2015-06-25 13:48:45 UTC) #101

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromium_gn_compile_rel/builds/102092)

5 years, 6 months ago (2015-06-25 13:48:46 UTC) #102

aberent

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

5 years, 6 months ago (2015-06-25 14:16:49 UTC) #103

aberent

The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org, zea@chromium.org Link to ...

5 years, 6 months ago (2015-06-25 14:16:50 UTC) #104

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1128043007/440001

5 years, 6 months ago (2015-06-25 14:17:59 UTC) #105

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

5 years, 6 months ago (2015-06-25 15:29:28 UTC) #106

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

5 years, 6 months ago (2015-06-25 15:29:30 UTC) #107

Ryan Sleevi

https://codereview.chromium.org/1128043007/diff/440001/net/android/dummy_spnego_authenticator.cc File net/android/dummy_spnego_authenticator.cc (right): https://codereview.chromium.org/1128043007/diff/440001/net/android/dummy_spnego_authenticator.cc#newcode12 net/android/dummy_spnego_authenticator.cc:12: // iso.org.dod.internet.security.mechanism.snego (1.3.6.1.5.5.2) nit: Add a comment explaining where ...

5 years, 5 months ago (2015-06-29 13:56:46 UTC) #108

cbentzel

The biggest question for me is whether we can avoid GSSAPI knowledge in the DummySpnegoAuthenticator ...

5 years, 5 months ago (2015-06-30 12:53:56 UTC) #109

aberent

https://codereview.chromium.org/1128043007/diff/440001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/1128043007/diff/440001/net/BUILD.gn#newcode1404 net/BUILD.gn:1404: # TODO(aberent) fix and enable these tests on Android. ...

5 years, 5 months ago (2015-07-02 21:13:37 UTC) #110

Ryan Sleevi

https://codereview.chromium.org/1128043007/diff/440001/net/android/http_auth_negotiate_android.cc File net/android/http_auth_negotiate_android.cc (right): https://codereview.chromium.org/1128043007/diff/440001/net/android/http_auth_negotiate_android.cc#newcode104 net/android/http_auth_negotiate_android.cc:104: auto callback_task_runner = base::ThreadTaskRunnerHandle::Get(); On 2015/07/02 21:13:34, aberent wrote: ...

5 years, 5 months ago (2015-07-02 23:51:47 UTC) #111

https://codereview.chromium.org/1128043007/diff/440001/net/android/http_auth_...
File net/android/http_auth_negotiate_android.cc (right):

https://codereview.chromium.org/1128043007/diff/440001/net/android/http_auth_...
net/android/http_auth_negotiate_android.cc:104: auto callback_task_runner =
base::ThreadTaskRunnerHandle::Get();
On 2015/07/02 21:13:34, aberent wrote:
> On 2015/06/29 13:56:44, Ryan Sleevi (slow through 7-15 wrote:
> > STYLE: Not a valid use of auto
> 
> Please explain further. The Google style guide simply says:
> 
> "auto is permitted, for local variables only. Do not use auto for file-scope
or
> namespace-scope variables, or for class members. Never initialize an
auto-typed
> variable with a braced initializer list."
> 
> and the Chromium C++11 page simply references the Google style guide for auto.
> 
> These are all local variables, and the code seems considerably tidier using
auto
> here rather than explicit types.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#auto

"Use auto to avoid type names that are just clutter. ***Continue to use manifest
type declarations when it helps readability,*** and never use auto for anything
but local variables."

From https://chromium-cpp.appspot.com/ on auto, you will see two discussion
threads linked. You can see examples such as
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/OQyYSfH9m2M/3qxtL...
or
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/5-Bt3BJzAo0/lpau2...

In every single use of "auto" you've used here, it is wholly non-obvious what
the return type is, which is why it isn't a valid use. It occludes ownership
semantics (such as this line, which has a scoped_refptr), it occludes the
expected callback signature type (such as 105), resulting in possibly incorrect
conversions. 107 - 111 all hide ownership semantics, and line 112 explicitly
(and dangerously) so.

The goal of auto - and the style guide in general - is to develop consistency
that primarily optimizes for the reader.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Background lays
out these principles.

I realize it makes it convenient for typing, but if as a reviewer - or a future
maintainer - I have to look up each of these function calls to see what they
return to determine whether or not this code is correct, then that's a net-loss
for productivity, even if there's a short-term gain for writability.

Sorry that I wasn't clearer in the original message, by pointing to the times on
chromium-dev where this has come up in the past (and has been enshrined in our
policy on auto)

aberent

https://codereview.chromium.org/1128043007/diff/440001/net/android/http_auth_negotiate_android.cc File net/android/http_auth_negotiate_android.cc (right): https://codereview.chromium.org/1128043007/diff/440001/net/android/http_auth_negotiate_android.cc#newcode104 net/android/http_auth_negotiate_android.cc:104: auto callback_task_runner = base::ThreadTaskRunnerHandle::Get(); On 2015/07/02 23:51:47, Ryan Sleevi ...

5 years, 5 months ago (2015-07-03 10:43:49 UTC) #112

https://codereview.chromium.org/1128043007/diff/440001/net/android/http_auth_...
File net/android/http_auth_negotiate_android.cc (right):

https://codereview.chromium.org/1128043007/diff/440001/net/android/http_auth_...
net/android/http_auth_negotiate_android.cc:104: auto callback_task_runner =
base::ThreadTaskRunnerHandle::Get();
On 2015/07/02 23:51:47, Ryan Sleevi (slow through 7-15 wrote:
> On 2015/07/02 21:13:34, aberent wrote:
> > On 2015/06/29 13:56:44, Ryan Sleevi (slow through 7-15 wrote:
> > > STYLE: Not a valid use of auto
> > 
> > Please explain further. The Google style guide simply says:
> > 
> > "auto is permitted, for local variables only. Do not use auto for file-scope
> or
> > namespace-scope variables, or for class members. Never initialize an
> auto-typed
> > variable with a braced initializer list."
> > 
> > and the Chromium C++11 page simply references the Google style guide for
auto.
> > 
> > These are all local variables, and the code seems considerably tidier using
> auto
> > here rather than explicit types.
> 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#auto
> 
> "Use auto to avoid type names that are just clutter. ***Continue to use
manifest
> type declarations when it helps readability,*** and never use auto for
anything
> but local variables."
> 
> From https://chromium-cpp.appspot.com/ on auto, you will see two discussion
> threads linked. You can see examples such as
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/OQyYSfH9m2M/3qxtL...
> or
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/5-Bt3BJzAo0/lpau2...
> 
> In every single use of "auto" you've used here, it is wholly non-obvious what
> the return type is, which is why it isn't a valid use. It occludes ownership
> semantics (such as this line, which has a scoped_refptr), it occludes the
> expected callback signature type (such as 105), resulting in possibly
incorrect
> conversions. 107 - 111 all hide ownership semantics, and line 112 explicitly
> (and dangerously) so.
> 
> The goal of auto - and the style guide in general - is to develop consistency
> that primarily optimizes for the reader.
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Background
lays
> out these principles.
> 
> I realize it makes it convenient for typing, but if as a reviewer - or a
future
> maintainer - I have to look up each of these function calls to see what they
> return to determine whether or not this code is correct, then that's a
net-loss
> for productivity, even if there's a short-term gain for writability.
> 
> Sorry that I wasn't clearer in the original message, by pointing to the times
on
> chromium-dev where this has come up in the past (and has been enshrined in our
> policy on auto)

Done.

https://codereview.chromium.org/1128043007/diff/440001/net/android/http_auth_...
net/android/http_auth_negotiate_android.cc:105: auto thread_safe_callback =
base::Bind(
On 2015/06/29 13:56:44, Ryan Sleevi (slow through 7-15 wrote:
> STYLE: Not a valid use of auto

Done.

https://codereview.chromium.org/1128043007/diff/440001/net/android/http_auth_...
net/android/http_auth_negotiate_android.cc:107: auto env =
AttachCurrentThread();
On 2015/06/29 13:56:44, Ryan Sleevi (slow through 7-15 wrote:
> STYLE: Not a valid use of auto

Done.

https://codereview.chromium.org/1128043007/diff/440001/net/android/http_auth_...
net/android/http_auth_negotiate_android.cc:109: ConvertUTF8ToJavaString(env,
server_auth_token_);
On 2015/06/29 13:56:45, Ryan Sleevi (slow through 7-15 wrote:
> STYLE: Not a valid use of auto

Done.

https://codereview.chromium.org/1128043007/diff/440001/net/android/http_auth_...
net/android/http_auth_negotiate_android.cc:110: auto java_spn =
ConvertUTF8ToJavaString(env, spn);
On 2015/06/29 13:56:45, Ryan Sleevi (slow through 7-15 wrote:
> STYLE: Not a valid use of auto

Done.

https://codereview.chromium.org/1128043007/diff/440001/net/android/http_auth_...
net/android/http_auth_negotiate_android.cc:111: auto java_account_type =
ConvertUTF8ToJavaString(env, account_type_);
On 2015/06/29 13:56:44, Ryan Sleevi (slow through 7-15 wrote:
> STYLE: Not a valid use of auto

Done.

https://codereview.chromium.org/1128043007/diff/440001/net/android/http_auth_...
net/android/http_auth_negotiate_android.cc:112: auto callback_wrapper = new
JavaNegotiateResultWrapper(callback_task_runner,
On 2015/06/29 13:56:45, Ryan Sleevi (slow through 7-15 wrote:
> STYLE: Not a valid use of auto

Done.

aberent

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

5 years, 5 months ago (2015-07-03 10:44:33 UTC) #113

aberent

The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org, zea@chromium.org Link to ...

5 years, 5 months ago (2015-07-03 10:44:34 UTC) #114

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1128043007/480001

5 years, 5 months ago (2015-07-03 10:44:41 UTC) #115

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

5 years, 5 months ago (2015-07-03 11:25:09 UTC) #116

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_clobber_rel_ng/builds/42190) (exceeded global ...

5 years, 5 months ago (2015-07-03 11:25:11 UTC) #117

aberent

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

5 years, 5 months ago (2015-07-03 16:20:52 UTC) #118

aberent

The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org, zea@chromium.org Link to ...

5 years, 5 months ago (2015-07-03 16:20:53 UTC) #119

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1128043007/500001

5 years, 5 months ago (2015-07-03 16:21:12 UTC) #120

aberent

https://codereview.chromium.org/1128043007/diff/440001/net/http/http_auth_gssapi_posix.h File net/http/http_auth_gssapi_posix.h (right): https://codereview.chromium.org/1128043007/diff/440001/net/http/http_auth_gssapi_posix.h#newcode12 net/http/http_auth_gssapi_posix.h:12: #include "net/base/completion_callback.h" On 2015/07/02 21:13:36, aberent wrote: > On ...

5 years, 5 months ago (2015-07-03 16:21:15 UTC) #121

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

5 years, 5 months ago (2015-07-03 17:36:17 UTC) #122

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

5 years, 5 months ago (2015-07-03 17:36:20 UTC) #123

cbentzel

On 2015/07/03 17:36:20, commit-bot: I haz the power wrote: > Dry run: This issue passed ...

5 years, 5 months ago (2015-07-08 17:11:32 UTC) #124

cbentzel

LGTM! Please address the nits I list below, but you don't need to ping me ...

5 years, 5 months ago (2015-07-08 18:27:11 UTC) #125

aberent

The patchset sent to the CQ was uploaded after l-g-t-m from cbentzel@chromium.org, jbudorick@chromium.org, zea@chromium.org Link ...

5 years, 5 months ago (2015-07-09 13:37:58 UTC) #127

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1128043007/560001

5 years, 5 months ago (2015-07-09 13:38:16 UTC) #128

aberent

https://codereview.chromium.org/1128043007/diff/440001/net/http/http_auth_handler_negotiate.h File net/http/http_auth_handler_negotiate.h (right): https://codereview.chromium.org/1128043007/diff/440001/net/http/http_auth_handler_negotiate.h#newcode38 net/http/http_auth_handler_negotiate.h:38: typedef net::android::HttpAuthNegotiateAndroid AuthSystem; On 2015/07/08 18:27:11, cbentzel wrote: > ...

5 years, 5 months ago (2015-07-09 13:38:46 UTC) #129

commit-bot: I haz the power

Patchset 29 (id:??) landed as https://crrev.com/ec894a5d76eadbe3cb489faed6469ff78e00aec8 Cr-Commit-Position: refs/heads/master@{#338039}

5 years, 5 months ago (2015-07-09 14:46:59 UTC) #131

Fabrice (no longer in Chrome)

fdegans@chromium.org changed reviewers: + fdegans@chromium.org

5 years, 4 months ago (2015-07-31 13:20:27 UTC) #132

Fabrice (no longer in Chrome)

5 years, 4 months ago (2015-07-31 13:20:29 UTC) #133

Message was sent while issue was closed.

https://codereview.chromium.org/1128043007/diff/560001/build/android/pylib/gt...
File build/android/pylib/gtest/test_package_apk.py (right):

https://codereview.chromium.org/1128043007/diff/560001/build/android/pylib/gt...
build/android/pylib/gtest/test_package_apk.py:47: self._extras =
{'RunInSubThread': ''}
This has been making the Android/asan bot fail since it landed.
Please, take a look at crbug.com/515881

Issue 1128043007: Support Kerberos on Android (Closed)

Description

Patch Set 1 #

Patch Set 2 : Fix build problems found by bots #

Patch Set 3 : Fix more problems found by bots #

Patch Set 4 : Fix Windows builds and one more Android test #

Patch Set 5 : Fix mistake in previous Windows fixes #

Patch Set 6 : Changes for bauerb@'s comments, plus some other minor fixes. #

Patch Set 7 : Fix more nits from bauerb@ #

Patch Set 8 : Add Junit tests for Java class #

Patch Set 9 : Various minor fixes. #

Patch Set 10 : Add some unit tests, separate out new policy into a new CL (TBA). #

Patch Set 11 : Add parser tests, remove unused changes #

Patch Set 12 : Move histograms.xml to policy patch, rebase, minor logging fixes. #

Patch Set 13 : Fix a nit I had missed. #

Patch Set 14 : Handle review comments #

Patch Set 15 : Intermediate upload to fix problems with tests and with refactoring of parsing. #

Patch Set 16 : Deal with comments and build and test failures. #

Patch Set 17 : Respond to jbudorick@ and fix build and test problems #

Patch Set 18 : Fix Windows Negotiate parsing #

Patch Set 19 : Add tests for new parser #

Patch Set 20 : Fix tests and error handling #

Patch Set 21 : Fix compile (clang) issues. #

Patch Set 22 : Fix remaining clang issue (missing override). #

Patch Set 23 : Fix Android GN build #

Patch Set 24 : Many fixes in response to comments, including more work on tests #

Patch Set 25 : Remove possibly confusing "auto"s. Add comments on callback_wrapper lifetime. #

Patch Set 26 : Fix compile errors caused by trying to forward declare a typedef #

Patch Set 27 : Pure rebase - no other changes #

Patch Set 28 : Pure rebase -- attempt 2 #

Patch Set 29 : Fix cbentzel@'s nits #

Messages