Unit tests for HttpAuthHandlerNegotiate

Adds unit tests for how HttpAuthHandlerNegotiate creates SPNs. 

BUG=None
TEST=net_unittests --gtest_filter="*HttpAuthHandlerNegotiate*"

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

[ahendrickson, 2010-04-20 19:37:06]

http://codereview.chromium.org/1705001/diff/8001/9004
File net/http/http_auth_handler_negotiate_unittest.cc (right):

http://codereview.chromium.org/1705001/diff/8001/9004#newcode24
net/http/http_auth_handler_negotiate_unittest.cc:24: MockSSPILibrary
mock_library;
Does mock_library need to persist after CreateHandler() returns?

[cbentzel, 2010-04-20 20:19:28]

http://codereview.chromium.org/1705001/diff/8001/9004
File net/http/http_auth_handler_negotiate_unittest.cc (right):

http://codereview.chromium.org/1705001/diff/8001/9004#newcode24
net/http/http_auth_handler_negotiate_unittest.cc:24: MockSSPILibrary
mock_library;
On 2010/04/20 19:37:06, ahendrickson wrote:
> Does mock_library need to persist after CreateHandler() returns?
> 

Good question. In this case, it was only needed for InitFromChallenge, so it
worked correctly.

However, that feels doomed to flakiness, so I changed this to pass an
SSPILibrary in and the tests ensure that the scope of the library exceeds that
of the handler.

[wtc, 2010-04-20 21:25:03]

LGTM, with the following comments.

eroman should review the changes to mock_host_resolver.{h,cc}.
I think the new kResolverTypeCanonical resolver type isn't
necessary.

http://codereview.chromium.org/1705001/diff/8001/9001
File net/base/mock_host_resolver.cc (right):

http://codereview.chromium.org/1705001/diff/8001/9001#newcode39
net/base/mock_host_resolver.cc:39: int CreateCanonicalAddressList(const
std::string& host,
Nit: add a comment to note that this function assumes
AF_INET (IPv4).

It seems to assume |host| is an IP address literal.

http://codereview.chromium.org/1705001/diff/8001/9001#newcode132
net/base/mock_host_resolver.cc:132: kResolverTypeCanonical,
Nit: kResolverTypeCanonical => kResolverTypeCanonicalName
or kResolverTypeCanonName

http://codereview.chromium.org/1705001/diff/8001/9001#newcode259
net/base/mock_host_resolver.cc:259: case Rule::kResolverTypeCanonical:
You don't seem to need a new resolver type for canonical name.
'Rule' has a canonical_name member now.  If the
canonical_name member is an empty string, set
ai.ai_canonname to NULL.

http://codereview.chromium.org/1705001/diff/8001/9002
File net/base/mock_host_resolver.h (right):

http://codereview.chromium.org/1705001/diff/8001/9002#newcode112
net/base/mock_host_resolver.h:112: void AddRuleForCanonicalName(const
std::string& host_pattern,
Nit: AddRuleWithCanonicalName seems more accurate...

http://codereview.chromium.org/1705001/diff/8001/9004
File net/http/http_auth_handler_negotiate_unittest.cc (right):

http://codereview.chromium.org/1705001/diff/8001/9004#newcode8
net/http/http_auth_handler_negotiate_unittest.cc:8: #include
"net/http/http_auth_handler_negotiate.h"
Include "net/http/http_auth_handler_negotiate.h" first, and
separate it from the other headers, because it is the header
that defines the class we're testing.

The style guide has an example that hints this convention:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

Search for "basictypes_unittest.cc".

http://codereview.chromium.org/1705001/diff/8001/9004#newcode11
net/http/http_auth_handler_negotiate_unittest.cc:11: #endif  // defined(OS_WIN)
Omit the "// defined(OS_WIN)" comment for a short ifdef
block.

http://codereview.chromium.org/1705001/diff/13001/8005#newcode21
net/http/http_auth_handler_negotiate_unittest.cc:21: void CreateHandler(bool
disable_cname, bool include_port,
Nit: "cname" should be changed to "cname_lookup" through out
this file, to match the current name of the parameter for the
HttpAuthHandlerNegotiate constructor.

This is the "CNAME" ambiguity issue that I raised before.
A DNS CNAME record is used to establish a DNS alias.  It
seems that to many people familiar with DNS, "CNAME" means
a DNS alias.  But to people unfamiliar with DNS, "CNAME"
appears to be the abbreviation of "canonical name", which is
the opposite of an alias.

[cbentzel, 2010-04-21 14:07:31]

I made the suggested changes, except for getting rid of the addition rule
resolver type.

I'll wait for eroman to give a vote before comitting this.

http://codereview.chromium.org/1705001/diff/8001/9001
File net/base/mock_host_resolver.cc (right):

http://codereview.chromium.org/1705001/diff/8001/9001#newcode39
net/base/mock_host_resolver.cc:39: int CreateCanonicalAddressList(const
std::string& host,
On 2010/04/20 21:25:03, wtc wrote:
> Nit: add a comment to note that this function assumes
> AF_INET (IPv4).
> 
> It seems to assume |host| is an IP address literal.

Done.

http://codereview.chromium.org/1705001/diff/8001/9001#newcode259
net/base/mock_host_resolver.cc:259: case Rule::kResolverTypeCanonical:
On 2010/04/20 21:25:03, wtc wrote:
> You don't seem to need a new resolver type for canonical name.
> 'Rule' has a canonical_name member now.  If the
> canonical_name member is an empty string, set
> ai.ai_canonname to NULL.

The problem is the default rule ends up calling SystemHostResolverProc so
there's not a great way to intercept that.

http://codereview.chromium.org/1705001/diff/8001/9004
File net/http/http_auth_handler_negotiate_unittest.cc (right):

http://codereview.chromium.org/1705001/diff/8001/9004#newcode8
net/http/http_auth_handler_negotiate_unittest.cc:8: #include
"net/http/http_auth_handler_negotiate.h"
On 2010/04/20 21:25:03, wtc wrote:
> Include "net/http/http_auth_handler_negotiate.h" first, and
> separate it from the other headers, because it is the header
> that defines the class we're testing.
> 
> The style guide has an example that hints this convention:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...
> 
> Search for "basictypes_unittest.cc".
> 

Thanks - the style guide was unclear about this for foo_unittest.cc files, but I
have used it for foo.cc files where it's a bit more explicit.

http://codereview.chromium.org/1705001/diff/13001/8005#newcode21
net/http/http_auth_handler_negotiate_unittest.cc:21: void CreateHandler(bool
disable_cname, bool include_port,
On 2010/04/20 21:25:03, wtc wrote:
> Nit: "cname" should be changed to "cname_lookup" through out
> this file, to match the current name of the parameter for the
> HttpAuthHandlerNegotiate constructor.
> 
> This is the "CNAME" ambiguity issue that I raised before.
> A DNS CNAME record is used to establish a DNS alias.  It
> seems that to many people familiar with DNS, "CNAME" means
> a DNS alias.  But to people unfamiliar with DNS, "CNAME"
> appears to be the abbreviation of "canonical name", which is
> the opposite of an alias.

Yes, I agree with the ambiguity/confusion issue. I frequently hear people say
"alias.example.com" is a CNAME for "canonical.example.com".

Saying disable_cname_lookup does make it clearer.

[eroman, 2010-04-23 19:59:45]

Comments on mock host resolver:

http://codereview.chromium.org/1705001/diff/19001/20001
File net/base/mock_host_resolver.cc (right):

http://codereview.chromium.org/1705001/diff/19001/20001#newcode50
net/base/mock_host_resolver.cc:50: reinterpret_cast<unsigned char*>(&address),
&num_components);
This isn't quite right... The first fields in sockaddr_in are the family and
port. really you want to be setting the in_addr field.

[cbentzelold, 2010-04-23 20:12:40]

On 2010/04/23 19:59:45, eroman wrote:
> Comments on mock host resolver:
> 
> http://codereview.chromium.org/1705001/diff/19001/20001
> File net/base/mock_host_resolver.cc (right):
> 
> http://codereview.chromium.org/1705001/diff/19001/20001#newcode50
> net/base/mock_host_resolver.cc:50: reinterpret_cast<unsigned char*>(&address),
> &num_components);
> This isn't quite right... The first fields in sockaddr_in are the family and
> port. really you want to be setting the in_addr field.

[cbentzel, 2010-04-27 16:50:46]

Eric - this is ready for re-review. Thanks.

[eroman, 2010-04-27 19:55:22]

LGTM

http://codereview.chromium.org/1705001/diff/5006/30004
File net/base/mock_host_resolver.cc (right):

http://codereview.chromium.org/1705001/diff/5006/30004#newcode52
net/base/mock_host_resolver.cc:52: host.c_str(), host_comp, ipv4_addr,
&num_components);
nit: my pet peeve for whatever reason, is using .data() instead of .c_str()
wherever possible.

http://codereview.chromium.org/1705001/diff/5006/30004#newcode239
net/base/mock_host_resolver.cc:239: bool matches_flags = (r->host_resolver_flags
& host_resolver_flags) ==
I am finding this hard to grok.

Would it be possible to write this in terms of boolean logic similar to
matches_address_family?

http://codereview.chromium.org/1705001/diff/5006/30005
File net/base/mock_host_resolver.h (right):

http://codereview.chromium.org/1705001/diff/5006/30005#newcode125
net/base/mock_host_resolver.h:125: // If |canonical-name| is non-empty, it is
used as the canonical DNS name
nit: I would reword this to be more clear that it is copied to the result (and
doesn't impact the resolution itself).

[cbentzel, 2010-04-28 14:34:33]

I uploaded the latest patchset which I'll commit.

http://codereview.chromium.org/1705001/diff/5006/30004
File net/base/mock_host_resolver.cc (right):

http://codereview.chromium.org/1705001/diff/5006/30004#newcode239
net/base/mock_host_resolver.cc:239: bool matches_flags = (r->host_resolver_flags
& host_resolver_flags) ==
On 2010/04/27 19:55:23, eroman wrote:
> I am finding this hard to grok.
> 
> Would it be possible to write this in terms of boolean logic similar to
> matches_address_family?

The logic here was essentially that r->host_resolver_flags might have multiple
additional bits that host_resolver_flags does not have, which is why I did the
bitwise-and followed by equality. I will document what this is doing.

If you'd still like me to simplify the logic (since there's only one effective
bit flag currently), let me know and I'll do it in a follow-up CL.