[net/auth] Reset AuthChallengeInfo before picking a new HttpAuthHandler.

[net/auth] Reset AuthChallengeInfo when credentials are provided.

HttpAuthController should be invoked in the following manner:

                    :
                    :
         +---------------------+
         | HandleAuthChallenge +----+
         +----------+----------+    |
Credentials needed  |               |
auth_info_ is valid |               | No credentials
                    v               | needed
         +----------+----------+    |
         |      ResetAuth      |    |
         +----------+----------+    |
                    |               |
                    |               |
                    v               |
        +-----------+------------+  |
        | MaybeGenerateAuthToken +<-+
        +------------------------+
	            :

Once HandleAuthChallenge is called, HttpAuthController indicates whether
it needs credentials by setting |auth_info_| to a valid
AuthChallengeInfo object.

Code paths existed where the AuthChallengeInfo used with the prior
handler may survive all the way through a ResetAuth /
MaybeGenerateAuthToken / HandleAuthChallenge cycle. This of course left
the HttpAuthController in an inconsistent state.

This change ensures that |auth_info_| is only valid between a
HandleAuthChallenge call that determines the need for external
credentials and the corresponding ResetAuth call. It asserts that
credentials have been provided if necessary prior to calling
MaybeGenerateAuthToken or a followup HandleAuthChallenge via DCHECKs.

BUG=none

Committed: https://crrev.com/be76fc926553fe0e270f0ee8e3baf7b00dd41619
Cr-Commit-Position: refs/heads/master@{#432981}

[asanka, 2016-11-16 22:13:24]

asanka@chromium.org changed reviewers:
+ mmenke@chromium.org

[asanka, 2016-11-16 22:21:18]

This is tested in the big test refactor. It doesn't affect functionality other
than the ability for that big test to tell whether the HttpAuthHandler is
waiting for credentials or not.

[asanka, 2016-11-16 23:17:09]

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

[commit-bot: I haz the power, 2016-11-16 23:18:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 01:45:47]

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

[commit-bot: I haz the power, 2016-11-17 01:45:48]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-11-17 16:07:20]

https://codereview.chromium.org/2505203002/diff/1/net/http/http_auth_controll...
File net/http/http_auth_controller.cc (right):

https://codereview.chromium.org/2505203002/diff/1/net/http/http_auth_controll...
net/http/http_auth_controller.cc:280: auth_info_ = nullptr;
Can this be put at the start of this method?  I don't think anything needs to
know about the previous auth challenge.

https://codereview.chromium.org/2505203002/diff/1/net/http/http_auth_controll...
net/http/http_auth_controller.cc:280: auth_info_ = nullptr;
Please add a comment along the lines:  "Clear information about the previous
auth challenge."  As-is, it's non-obvious that this is the old challenge, and
not the new one.

[asanka, 2016-11-17 17:57:07]

Description was changed from

==========
[net/auth] Reset AuthChallengeInfo before picking a new HttpAuthHandler.

There exists code paths where the AuthChallengeInfo used with the prior
handle may survive a HttpAuthHandler selection. The presence of
the AuthChallengeInfo is used as an externally visible indicator that
the current HttpAuthHandler is expecting new credentials. If the
HttpAuthController has an AuthChallengeInfo but no HttpAuthHandler that
goes with it, its state is nonsensical.

BUG=none
==========

to

==========
[net/auth] Reset AuthChallengeInfo when credentials are provided.

HttpAuthController should be invoked in the following manner:

                    :
                    :
         +---------------------+
         | HandleAuthChallenge +----+
         +----------+----------+    |
Credentials needed  |               |
auth_info_ is valid |               | No credentials
                    v               | needed
         +----------+----------+    |
         |      ResetAuth      |    |
         +----------+----------+    |
                    |               |
                    |               |
                    v               |
        +-----------+------------+  |
        | MaybeGenerateAuthToken +<-+
        +------------------------+
	            :

Once HandleAuthChallenge is called, HttpAuthController indicates whether
it needs credentials by setting |auth_info_| to a valid
AuthChallengeInfo object.

Code paths existed where the AuthChallengeInfo used with the prior
handler may survive all the way through a ResetAuth /
MaybeGenerateAuthToken / HandleAuthChallenge cycle. This of course left
the HttpAuthController in an inconsistent state.

This change ensures that |auth_info_| is only valid between a
HandleAuthChallenge call that determines the need for external
credentials and the corresponding ResetAuth call. It asserts that
credentials have been provided if necessary prior to calling
MaybeGenerateAuthToken or a followup HandleAuthChallenge via DCHECKs.

BUG=none
==========

[asanka, 2016-11-17 18:01:25]

https://codereview.chromium.org/2505203002/diff/1/net/http/http_auth_controll...
File net/http/http_auth_controller.cc (right):

https://codereview.chromium.org/2505203002/diff/1/net/http/http_auth_controll...
net/http/http_auth_controller.cc:280: auth_info_ = nullptr;
On 2016/11/17 at 16:07:19, mmenke wrote:
> Can this be put at the start of this method?  I don't think anything needs to
know about the previous auth challenge.

I moved it all the way up to ResetAuth and commented there. Reasoning for this
is in the CL description.

[mmenke, 2016-11-17 18:06:56]

LGTM, though come to think of it...do we even need auth_info_?  Seems like we
could remove it and add a pointer argument to HandleAuthChallenge, so it looks
like auth_info is only used by the auth_info() method, which is only called
right after HandleAuthChallenge?  Or am I missing something?

[asanka, 2016-11-17 18:16:40]

On 2016/11/17 at 18:06:56, mmenke wrote:
> LGTM, though come to think of it...do we even need auth_info_?  Seems like we
could remove it and add a pointer argument to HandleAuthChallenge, so it looks
like auth_info is only used by the auth_info() method, which is only called
right after HandleAuthChallenge?  Or am I missing something?


We could rework the auth_info_ ownership so that the caller of
HandleAuthChallenge is responsible for taking over the AuthChallengeInfo and
propagating it up the stack until someone prompts the user for credentials. But
currently callers of HandleAuthChallenge doesn't have to take up that
responsibility and can use the auth_info() accessor to check if credentials are
needed.

Ideally, we'd make HandleAuthChallenge and ResetAuth be asynchronous so that we
can fold in the work of GenerateAuthToken into both those operations. That way
we don't need to cycle through the network transaction to discover that the
credentials were invalid.

One or both of these would force us to revisit the role of AuthChallengeInfo
ownership. But for now, keeping it with HttpAuthController allows us to decouple
calling HandleAuthCHallenge and plumbing the auth challenge up the stack.

[asanka, 2016-11-17 18:20:26]

The CQ bit was checked by asanka@chromium.org

[commit-bot: I haz the power, 2016-11-17 18:21:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2016-11-17 18:28:28]

On 2016/11/17 18:16:40, asanka wrote:
> On 2016/11/17 at 18:06:56, mmenke wrote:
> > LGTM, though come to think of it...do we even need auth_info_?  Seems like
we
> could remove it and add a pointer argument to HandleAuthChallenge, so it looks
> like auth_info is only used by the auth_info() method, which is only called
> right after HandleAuthChallenge?  Or am I missing something?
> 
> 
> We could rework the auth_info_ ownership so that the caller of
> HandleAuthChallenge is responsible for taking over the AuthChallengeInfo and
> propagating it up the stack until someone prompts the user for credentials.
But
> currently callers of HandleAuthChallenge doesn't have to take up that
> responsibility and can use the auth_info() accessor to check if credentials
are
> needed.

The don't have to, but the only two calls do in fact do this, so I think it's a
moot point?  My real concern here is we have a ton of variables with unclear
lifetimes and semantics.  One way to clean that up is by getting rid of some of
them.

> Ideally, we'd make HandleAuthChallenge and ResetAuth be asynchronous so that
we
> can fold in the work of GenerateAuthToken into both those operations. That way
> we don't need to cycle through the network transaction to discover that the
> credentials were invalid.
> 
> One or both of these would force us to revisit the role of AuthChallengeInfo
> ownership. But for now, keeping it with HttpAuthController allows us to
decouple
> calling HandleAuthCHallenge and plumbing the auth challenge up the stack.

If it were async, we could just make it an argument to the async callback. 
Anyhow, not going to push on this, just trying to make the code easier to
understand.

[asanka, 2016-11-17 18:35:42]

On 2016/11/17 at 18:28:28, mmenke wrote:
> On 2016/11/17 18:16:40, asanka wrote:
> > On 2016/11/17 at 18:06:56, mmenke wrote:
> > > LGTM, though come to think of it...do we even need auth_info_?  Seems like
we
> > could remove it and add a pointer argument to HandleAuthChallenge, so it
looks
> > like auth_info is only used by the auth_info() method, which is only called
> > right after HandleAuthChallenge?  Or am I missing something?
> > 
> > 
> > We could rework the auth_info_ ownership so that the caller of
> > HandleAuthChallenge is responsible for taking over the AuthChallengeInfo and
> > propagating it up the stack until someone prompts the user for credentials.
But
> > currently callers of HandleAuthChallenge doesn't have to take up that
> > responsibility and can use the auth_info() accessor to check if credentials
are
> > needed.
> 
> The don't have to, but the only two calls do in fact do this, so I think it's
a moot point?  My real concern here is we have a ton of variables with unclear
lifetimes and semantics.  One way to clean that up is by getting rid of some of
them.


Ah yes. Now that I look at it again, we can do away with the decoupling since
both call sites immediately add a reference to the AuthChallengeInfo. Thanks for
pointing that out.

> > Ideally, we'd make HandleAuthChallenge and ResetAuth be asynchronous so that
we
> > can fold in the work of GenerateAuthToken into both those operations. That
way
> > we don't need to cycle through the network transaction to discover that the
> > credentials were invalid.
> > 
> > One or both of these would force us to revisit the role of AuthChallengeInfo
> > ownership. But for now, keeping it with HttpAuthController allows us to
decouple
> > calling HandleAuthCHallenge and plumbing the auth challenge up the stack.
> 
> If it were async, we could just make it an argument to the async callback. 
Anyhow, not going to push on this, just trying to make the code easier to
understand.

[commit-bot: I haz the power, 2016-11-17 21:51:59]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-17 21:54:50]

Description was changed from

==========
[net/auth] Reset AuthChallengeInfo when credentials are provided.

HttpAuthController should be invoked in the following manner:

                    :
                    :
         +---------------------+
         | HandleAuthChallenge +----+
         +----------+----------+    |
Credentials needed  |               |
auth_info_ is valid |               | No credentials
                    v               | needed
         +----------+----------+    |
         |      ResetAuth      |    |
         +----------+----------+    |
                    |               |
                    |               |
                    v               |
        +-----------+------------+  |
        | MaybeGenerateAuthToken +<-+
        +------------------------+
	            :

Once HandleAuthChallenge is called, HttpAuthController indicates whether
it needs credentials by setting |auth_info_| to a valid
AuthChallengeInfo object.

Code paths existed where the AuthChallengeInfo used with the prior
handler may survive all the way through a ResetAuth /
MaybeGenerateAuthToken / HandleAuthChallenge cycle. This of course left
the HttpAuthController in an inconsistent state.

This change ensures that |auth_info_| is only valid between a
HandleAuthChallenge call that determines the need for external
credentials and the corresponding ResetAuth call. It asserts that
credentials have been provided if necessary prior to calling
MaybeGenerateAuthToken or a followup HandleAuthChallenge via DCHECKs.

BUG=none
==========

to

==========
[net/auth] Reset AuthChallengeInfo when credentials are provided.

HttpAuthController should be invoked in the following manner:

                    :
                    :
         +---------------------+
         | HandleAuthChallenge +----+
         +----------+----------+    |
Credentials needed  |               |
auth_info_ is valid |               | No credentials
                    v               | needed
         +----------+----------+    |
         |      ResetAuth      |    |
         +----------+----------+    |
                    |               |
                    |               |
                    v               |
        +-----------+------------+  |
        | MaybeGenerateAuthToken +<-+
        +------------------------+
	            :

Once HandleAuthChallenge is called, HttpAuthController indicates whether
it needs credentials by setting |auth_info_| to a valid
AuthChallengeInfo object.

Code paths existed where the AuthChallengeInfo used with the prior
handler may survive all the way through a ResetAuth /
MaybeGenerateAuthToken / HandleAuthChallenge cycle. This of course left
the HttpAuthController in an inconsistent state.

This change ensures that |auth_info_| is only valid between a
HandleAuthChallenge call that determines the need for external
credentials and the corresponding ResetAuth call. It asserts that
credentials have been provided if necessary prior to calling
MaybeGenerateAuthToken or a followup HandleAuthChallenge via DCHECKs.

BUG=none

Committed: https://crrev.com/be76fc926553fe0e270f0ee8e3baf7b00dd41619
Cr-Commit-Position: refs/heads/master@{#432981}
==========

[commit-bot: I haz the power, 2016-11-17 21:54:51]

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