SPDY proxy authentication support.

SPDY proxy authentication support.


BUG=147591
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=159342

[willchan no longer on Chromium, 2012-09-17 17:53:37]

Mostly seems fine to me. Most of this is auth related, and I'll gladly defer to
Chris here :)

Why are you using the CommandLinePrefStore? Is this because you expect device
policies to perhaps override this setting? Or to use about:flags to configure
this (but we only have enable/disable on about:flags, so I don't think this is
possible)? If not, I think it's more common just to read from the CommandLine
directly. The reason to use a PrefStore is to unify the command line with the
Preferences file and corporate device management policies. I'm not sure if
that's applicable here. Let me know if it actually is.

[cbentzel, 2012-09-18 15:04:12]

Sorry for delay.

I'd still like to add the change sketched at
(http://codereview.chromium.org/10916272/) which would require no changes to
net/ when a new authentication scheme is introduced.

However, that can come after this.

[cbentzel, 2012-09-18 15:30:51]

LGTM, with some nits. Feel free to make changes and continue without any more
feedback from me. You will need to get chrome/OWNERS approval for the new
chrome/browser subdirectory, and may need to add OWNERS to that as well.
Alternately, I could imagine putting this in chrome/browser/net

http://codereview.chromium.org/10913238/diff/1/chrome/browser/spdyproxy/http_...
File chrome/browser/spdyproxy/http_auth_handler_spdyproxy.cc (right):

http://codereview.chromium.org/10913238/diff/1/chrome/browser/spdyproxy/http_...
chrome/browser/spdyproxy/http_auth_handler_spdyproxy.cc:57:
origin_origin.SchemeIs("https") &&
The SchemeIs check here is not needed - it's contained within the GURL equality
comparison below.

http://codereview.chromium.org/10913238/diff/1/chrome/browser/spdyproxy/http_...
chrome/browser/spdyproxy/http_auth_handler_spdyproxy.cc:170: 
Nit: extra new line

http://codereview.chromium.org/10913238/diff/1/chrome/browser/spdyproxy/http_...
File chrome/browser/spdyproxy/http_auth_handler_spdyproxy.h (right):

http://codereview.chromium.org/10913238/diff/1/chrome/browser/spdyproxy/http_...
chrome/browser/spdyproxy/http_auth_handler_spdyproxy.h:52: const std::string&
ps_token() const { return ps_token_; }
It looks like this is only part of the public interface because of tests?

http://codereview.chromium.org/10913238/diff/1/chrome/browser/spdyproxy/http_...
File chrome/browser/spdyproxy/http_auth_handler_spdyproxy_unittest.cc (right):

http://codereview.chromium.org/10913238/diff/1/chrome/browser/spdyproxy/http_...
chrome/browser/spdyproxy/http_auth_handler_spdyproxy_unittest.cc:123:
TEST(HttpAuthHandlerSpdyProxyTest, ParseChallenge) {
This is mainly covered by the GenerateAuthToken test, but I'm fine with this as
well.

http://codereview.chromium.org/10913238/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/10913238/diff/1/chrome/common/pref_names.cc#ne...
chrome/common/pref_names.cc:1745: // String that specifies the origin allowed to
use SpdyProxy
Nit: there should probablyh be a newline before this (and there should have been
on the other auth options, my fault).

[Michael Piatek, 2012-09-18 15:31:23]

No reason -- I was pattern matching on existing code -- what needs to change
here? I'll rm the changes to command_line_pref_store -- anything else?

<snip>
> Why are you using the CommandLinePrefStore? Is this because you expect device
> policies to perhaps override this setting? Or to use about:flags to configure
> this (but we only have enable/disable on about:flags, so I don't think this is
> possible)? If not, I think it's more common just to read from the CommandLine
> directly. The reason to use a PrefStore is to unify the command line with the
> Preferences file and corporate device management policies. I'm not sure if
> that's applicable here. Let me know if it actually is.

[Michael Piatek, 2012-09-18 20:32:05]

https://chromiumcodereview.appspot.com/10913238/diff/1/chrome/browser/spdypro...
File chrome/browser/spdyproxy/http_auth_handler_spdyproxy.cc (right):

https://chromiumcodereview.appspot.com/10913238/diff/1/chrome/browser/spdypro...
chrome/browser/spdyproxy/http_auth_handler_spdyproxy.cc:57:
origin_origin.SchemeIs("https") &&
On 2012/09/18 15:30:52, cbentzel wrote:
> The SchemeIs check here is not needed - it's contained within the GURL
equality
> comparison below.

Done.

https://chromiumcodereview.appspot.com/10913238/diff/1/chrome/browser/spdypro...
chrome/browser/spdyproxy/http_auth_handler_spdyproxy.cc:170: 
On 2012/09/18 15:30:52, cbentzel wrote:
> Nit: extra new line

Done.

https://chromiumcodereview.appspot.com/10913238/diff/1/chrome/browser/spdypro...
File chrome/browser/spdyproxy/http_auth_handler_spdyproxy.h (right):

https://chromiumcodereview.appspot.com/10913238/diff/1/chrome/browser/spdypro...
chrome/browser/spdyproxy/http_auth_handler_spdyproxy.h:52: const std::string&
ps_token() const { return ps_token_; }
On 2012/09/18 15:30:52, cbentzel wrote:
> It looks like this is only part of the public interface because of tests?

Done.

https://chromiumcodereview.appspot.com/10913238/diff/1/chrome/browser/spdypro...
File chrome/browser/spdyproxy/http_auth_handler_spdyproxy_unittest.cc (right):

https://chromiumcodereview.appspot.com/10913238/diff/1/chrome/browser/spdypro...
chrome/browser/spdyproxy/http_auth_handler_spdyproxy_unittest.cc:123:
TEST(HttpAuthHandlerSpdyProxyTest, ParseChallenge) {
On 2012/09/18 15:30:52, cbentzel wrote:
> This is mainly covered by the GenerateAuthToken test, but I'm fine with this
as
> well.

Ack.

https://chromiumcodereview.appspot.com/10913238/diff/1/chrome/common/pref_nam...
File chrome/common/pref_names.cc (right):

https://chromiumcodereview.appspot.com/10913238/diff/1/chrome/common/pref_nam...
chrome/common/pref_names.cc:1745: // String that specifies the origin allowed to
use SpdyProxy
On 2012/09/18 15:30:52, cbentzel wrote:
> Nit: there should probablyh be a newline before this (and there should have
been
> on the other auth options, my fault).

Done.

[cbentzel, 2012-09-19 14:07:58]

LGTM, pending directory choice

On Tue, Sep 18, 2012 at 4:32 PM, <piatek@google.com> wrote:

>
> https://chromiumcodereview.**appspot.com/10913238/diff/1/**
>
chrome/browser/spdyproxy/http_**auth_handler_spdyproxy.cc<https://chromiumcodereview.appspot.com/10913238/diff/1/chrome/browser/spdyproxy/http_auth_handler_spdyproxy.cc>
> File chrome/browser/spdyproxy/http_**auth_handler_spdyproxy.cc (right):
>
> https://chromiumcodereview.**appspot.com/10913238/diff/1/**
>
chrome/browser/spdyproxy/http_**auth_handler_spdyproxy.cc#**newcode57<https://chromiumcodereview.appspot.com/10913238/diff/1/chrome/browser/spdyproxy/http_auth_handler_spdyproxy.cc#newcode57>
>
> chrome/browser/spdyproxy/http_**auth_handler_spdyproxy.cc:57:
> origin_origin.SchemeIs("https"**) &&
> On 2012/09/18 15:30:52, cbentzel wrote:
>
>> The SchemeIs check here is not needed - it's contained within the GURL
>>
> equality
>
>> comparison below.
>>
>
> Done.
>
> https://chromiumcodereview.**appspot.com/10913238/diff/1/**
>
chrome/browser/spdyproxy/http_**auth_handler_spdyproxy.cc#**newcode170<https://chromiumcodereview.appspot.com/10913238/diff/1/chrome/browser/spdyproxy/http_auth_handler_spdyproxy.cc#newcode170>
> chrome/browser/spdyproxy/http_**auth_handler_spdyproxy.cc:170:
> On 2012/09/18 15:30:52, cbentzel wrote:
>
>> Nit: extra new line
>>
>
> Done.
>
> https://chromiumcodereview.**appspot.com/10913238/diff/1/**
>
chrome/browser/spdyproxy/http_**auth_handler_spdyproxy.h<https://chromiumcodereview.appspot.com/10913238/diff/1/chrome/browser/spdyproxy/http_auth_handler_spdyproxy.h>
> File chrome/browser/spdyproxy/http_**auth_handler_spdyproxy.h (right):
>
> https://chromiumcodereview.**appspot.com/10913238/diff/1/**
>
chrome/browser/spdyproxy/http_**auth_handler_spdyproxy.h#**newcode52<https://chromiumcodereview.appspot.com/10913238/diff/1/chrome/browser/spdyproxy/http_auth_handler_spdyproxy.h#newcode52>
>
> chrome/browser/spdyproxy/http_**auth_handler_spdyproxy.h:52: const
> std::string& ps_token() const { return ps_token_; }
> On 2012/09/18 15:30:52, cbentzel wrote:
>
>> It looks like this is only part of the public interface because of
>>
> tests?
>
> Done.
>
> https://chromiumcodereview.**appspot.com/10913238/diff/1/**
>
chrome/browser/spdyproxy/http_**auth_handler_spdyproxy_**unittest.cc<https://chromiumcodereview.appspot.com/10913238/diff/1/chrome/browser/spdyproxy/http_auth_handler_spdyproxy_unittest.cc>
> File chrome/browser/spdyproxy/http_**auth_handler_spdyproxy_**unittest.cc
> (right):
>
> https://chromiumcodereview.**appspot.com/10913238/diff/1/**
> chrome/browser/spdyproxy/http_**auth_handler_spdyproxy_**
>
unittest.cc#newcode123<https://chromiumcodereview.appspot.com/10913238/diff/1/chrome/browser/spdyproxy/http_auth_handler_spdyproxy_unittest.cc#newcode123>
> chrome/browser/spdyproxy/http_**auth_handler_spdyproxy_**unittest.cc:123:
> TEST(**HttpAuthHandlerSpdyProxyTest, ParseChallenge) {
>
> On 2012/09/18 15:30:52, cbentzel wrote:
>
>> This is mainly covered by the GenerateAuthToken test, but I'm fine
>>
> with this as
>
>> well.
>>
>
> Ack.
>
> https://chromiumcodereview.**appspot.com/10913238/diff/1/**
>
chrome/common/pref_names.cc<https://chromiumcodereview.appspot.com/10913238/diff/1/chrome/common/pref_names.cc>
> File chrome/common/pref_names.cc (right):
>
> https://chromiumcodereview.**appspot.com/10913238/diff/1/**
>
chrome/common/pref_names.cc#**newcode1745<https://chromiumcodereview.appspot.com/10913238/diff/1/chrome/common/pref_names.cc#newcode1745>
>
> chrome/common/pref_names.cc:**1745: // String that specifies the origin
> allowed to use SpdyProxy
> On 2012/09/18 15:30:52, cbentzel wrote:
>
>> Nit: there should probablyh be a newline before this (and there should
>>
> have been
>
>> on the other auth options, my fault).
>>
>
> Done.
>
>
https://chromiumcodereview.**appspot.com/10913238/<https://chromiumcodereview...
>

[Michael Piatek, 2012-09-20 22:24:12]

Moved browser/spdyproxy to browser/net/spdyproxy.

[cbentzel, 2012-09-21 13:00:27]

On 2012/09/20 22:24:12, Michael Piatek wrote:
> Moved browser/spdyproxy to browser/net/spdyproxy.

LGTM

[Ilya Sherman, 2012-09-24 23:47:25]

https://chromiumcodereview.appspot.com/10913238/diff/18001/chrome/browser/pas...
File chrome/browser/password_manager/password_manager_delegate_impl.cc (right):

https://chromiumcodereview.appspot.com/10913238/diff/18001/chrome/browser/pas...
chrome/browser/password_manager/password_manager_delegate_impl.cc:154: //
SpdyProxy authentication, as indicated by the realm.
What is the user action that would reach this code?  It rather seems like this
flow should be pre-empted prior to when we're about to show an infobar...

https://chromiumcodereview.appspot.com/10913238/diff/18001/chrome/browser/pas...
chrome/browser/password_manager/password_manager_delegate_impl.cc:155: if
(form_to_save->realm().find("/SpdyProxy") != std::string::npos) {
nit: Please define a constant for this string.  Also, can we do a full string
match, rather than a substring search?

https://chromiumcodereview.appspot.com/10913238/diff/18001/chrome/browser/pas...
chrome/browser/password_manager/password_manager_delegate_impl.cc:157: }
nit: No need for curly braces.

[Michael Piatek, 2012-09-25 19:28:14]

https://chromiumcodereview.appspot.com/10913238/diff/18001/chrome/browser/pas...
File chrome/browser/password_manager/password_manager_delegate_impl.cc (right):

https://chromiumcodereview.appspot.com/10913238/diff/18001/chrome/browser/pas...
chrome/browser/password_manager/password_manager_delegate_impl.cc:154: //
SpdyProxy authentication, as indicated by the realm.
On 2012/09/24 23:47:25, Ilya Sherman wrote:
> What is the user action that would reach this code?  It rather seems like this
> flow should be pre-empted prior to when we're about to show an infobar...

This is designed to suppress the 'remember the password for this website?'
prompt, which is inappropriate for this authentication type.

https://chromiumcodereview.appspot.com/10913238/diff/18001/chrome/browser/pas...
chrome/browser/password_manager/password_manager_delegate_impl.cc:155: if
(form_to_save->realm().find("/SpdyProxy") != std::string::npos) {
On 2012/09/24 23:47:25, Ilya Sherman wrote:
> nit: Please define a constant for this string.  Also, can we do a full string
> match, rather than a substring search?

Done. (Checked for a suffix match, since the full string contains this preceded
by the origin.)

https://chromiumcodereview.appspot.com/10913238/diff/18001/chrome/browser/pas...
chrome/browser/password_manager/password_manager_delegate_impl.cc:157: }
On 2012/09/24 23:47:25, Ilya Sherman wrote:
> nit: No need for curly braces.

Done.

[Ilya Sherman, 2012-09-25 22:31:06]

https://chromiumcodereview.appspot.com/10913238/diff/18001/chrome/browser/pas...
File chrome/browser/password_manager/password_manager_delegate_impl.cc (right):

https://chromiumcodereview.appspot.com/10913238/diff/18001/chrome/browser/pas...
chrome/browser/password_manager/password_manager_delegate_impl.cc:154: //
SpdyProxy authentication, as indicated by the realm.
On 2012/09/25 19:28:14, Michael Piatek wrote:
> On 2012/09/24 23:47:25, Ilya Sherman wrote:
> > What is the user action that would reach this code?  It rather seems like
this
> > flow should be pre-empted prior to when we're about to show an infobar...
> 
> This is designed to suppress the 'remember the password for this website?'
> prompt, which is inappropriate for this authentication type.

Can you describe more what "this authentication type" is?  I still don't
understand what user action triggers this code...

https://chromiumcodereview.appspot.com/10913238/diff/27001/chrome/browser/pas...
File chrome/browser/password_manager/password_manager_delegate_impl.cc (right):

https://chromiumcodereview.appspot.com/10913238/diff/27001/chrome/browser/pas...
chrome/browser/password_manager/password_manager_delegate_impl.cc:162: if
(base::StringPiece(form_to_save->realm()).ends_with(kSpdyProxyRealm))
nit: Please use the EndsWith() function from base/string_util.cc.  Explicitly
constructing a StringPiece is almost never what you want.

[Michael Piatek, 2012-09-26 23:08:34]

https://chromiumcodereview.appspot.com/10913238/diff/18001/chrome/browser/pas...
File chrome/browser/password_manager/password_manager_delegate_impl.cc (right):

https://chromiumcodereview.appspot.com/10913238/diff/18001/chrome/browser/pas...
chrome/browser/password_manager/password_manager_delegate_impl.cc:154: //
SpdyProxy authentication, as indicated by the realm.
On 2012/09/25 22:31:07, Ilya Sherman wrote:
> On 2012/09/25 19:28:14, Michael Piatek wrote:
> > On 2012/09/24 23:47:25, Ilya Sherman wrote:
> > > What is the user action that would reach this code?  It rather seems like
> this
> > > flow should be pre-empted prior to when we're about to show an infobar...
> > 
> > This is designed to suppress the 'remember the password for this website?'
> > prompt, which is inappropriate for this authentication type.
> 
> Can you describe more what "this authentication type" is?  I still don't
> understand what user action triggers this code...

moved this to password_manager.cc per discussion

https://chromiumcodereview.appspot.com/10913238/diff/27001/chrome/browser/pas...
File chrome/browser/password_manager/password_manager_delegate_impl.cc (right):

https://chromiumcodereview.appspot.com/10913238/diff/27001/chrome/browser/pas...
chrome/browser/password_manager/password_manager_delegate_impl.cc:162: if
(base::StringPiece(form_to_save->realm()).ends_with(kSpdyProxyRealm))
On 2012/09/25 22:31:07, Ilya Sherman wrote:
> nit: Please use the EndsWith() function from base/string_util.cc.  Explicitly
> constructing a StringPiece is almost never what you want.

Done.

[Ilya Sherman, 2012-09-26 23:17:17]

LGTM, thanks :)

[sky, 2012-09-28 17:14:30]

LGTM

https://chromiumcodereview.appspot.com/10913238/diff/34001/chrome/browser/net...
File chrome/browser/net/spdyproxy/http_auth_handler_spdyproxy.cc (right):

https://chromiumcodereview.appspot.com/10913238/diff/34001/chrome/browser/net...
chrome/browser/net/spdyproxy/http_auth_handler_spdyproxy.cc:17: 
remove one of these lines.

https://chromiumcodereview.appspot.com/10913238/diff/34001/chrome/browser/net...
File chrome/browser/net/spdyproxy/http_auth_handler_spdyproxy.h (right):

https://chromiumcodereview.appspot.com/10913238/diff/34001/chrome/browser/net...
chrome/browser/net/spdyproxy/http_auth_handler_spdyproxy.h:45: virtual
net::HttpAuth::AuthorizationResult HandleAnotherChallenge(
Keep all your overrides grouped together with no newlines between them and
prefix the section with where they come from.

https://chromiumcodereview.appspot.com/10913238/diff/34001/chrome/browser/net...
chrome/browser/net/spdyproxy/http_auth_handler_spdyproxy.h:75: };
DISALLOW_CO..

https://chromiumcodereview.appspot.com/10913238/diff/34001/chrome/browser/net...
File chrome/browser/net/spdyproxy/http_auth_handler_spdyproxy_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10913238/diff/34001/chrome/browser/net...
chrome/browser/net/spdyproxy/http_auth_handler_spdyproxy_unittest.cc:11:
#include "chrome/browser/net/spdyproxy/http_auth_handler_spdyproxy.h"
This should be your first include (same structure as
http_auth_handler_spdyproxy.cc).

[Michael Piatek, 2012-09-28 17:51:29]

https://chromiumcodereview.appspot.com/10913238/diff/34001/chrome/browser/net...
File chrome/browser/net/spdyproxy/http_auth_handler_spdyproxy.cc (right):

https://chromiumcodereview.appspot.com/10913238/diff/34001/chrome/browser/net...
chrome/browser/net/spdyproxy/http_auth_handler_spdyproxy.cc:17: 
On 2012/09/28 17:14:30, sky wrote:
> remove one of these lines.

Done.

https://chromiumcodereview.appspot.com/10913238/diff/34001/chrome/browser/net...
File chrome/browser/net/spdyproxy/http_auth_handler_spdyproxy.h (right):

https://chromiumcodereview.appspot.com/10913238/diff/34001/chrome/browser/net...
chrome/browser/net/spdyproxy/http_auth_handler_spdyproxy.h:45: virtual
net::HttpAuth::AuthorizationResult HandleAnotherChallenge(
On 2012/09/28 17:14:30, sky wrote:
> Keep all your overrides grouped together with no newlines between them and
> prefix the section with where they come from.

Done.

https://chromiumcodereview.appspot.com/10913238/diff/34001/chrome/browser/net...
chrome/browser/net/spdyproxy/http_auth_handler_spdyproxy.h:75: };
On 2012/09/28 17:14:30, sky wrote:
> DISALLOW_CO..

Done.

https://chromiumcodereview.appspot.com/10913238/diff/34001/chrome/browser/net...
File chrome/browser/net/spdyproxy/http_auth_handler_spdyproxy_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10913238/diff/34001/chrome/browser/net...
chrome/browser/net/spdyproxy/http_auth_handler_spdyproxy_unittest.cc:11:
#include "chrome/browser/net/spdyproxy/http_auth_handler_spdyproxy.h"
On 2012/09/28 17:14:30, sky wrote:
> This should be your first include (same structure as
> http_auth_handler_spdyproxy.cc).

Done.

[commit-bot: I haz the power, 2012-09-28 18:16:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piatek@google.com/10913238/30004

[commit-bot: I haz the power, 2012-09-28 21:31:53]

Change committed as 159342