[net/auth] Split HandleAnotherChallenge into a base and an Impl.

HttpAuthHandler is a base class that has a couple of pure virtual
methods for specialization by HTTP authentication scheme specific
handlers. There are three main entry points to an HttpAuthHandler:

  1. Construction and initialization, which is only invoked via a
     HttpAuthHandlerFactory.
  2. GenerateAuthToken().
  3. HandleAnotherChallenge().

Of these the initialization -- via InitFromChallenge() -- and
GenerateAuthToken() methods wrap pure virtual specializations via Init()
and GenerateAuthTokenImpl() respectively. This wrapping allows
consistent handling of logging around the specialization.

The odd one out is HandleAnotherChallenge() which was left as a pure
virtual method to be overridden by each specialization. This CL makes
HandleAnotherChallenge() also a wrapper in line with the other entry
points. This is done by making HandleAnotherChallenge() be a concrete
wrapper that invokes HandleAnotherChallengeImpl() which is a pure
virtual specialization point.

In addition, this CL introduces AUTH_GENERATE_TOKEN and
AUTH_HANDLE_CHALLENGE NetLog event types that wrap the events happening
within GenerateAuthToken() and HandleAnotherChallenge() respectively.
The handlers no longer log any target specific events like AUTH_SERVER
and AUTH_PROXY which are removed by this CL.

Bug: 884313
Change-Id: Ib7f97e61e0dcdc60dc15326604b7d3e383ab5c22
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1566490
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Asanka Herath <asanka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#662225}

net/http/http_auth.cc

@@ -9,6 +9,7 @@
 #include "base/stl_util.h"
 #include "base/strings/string_tokenizer.h"
 #include "base/strings/string_util.h"
+#include "base/values.h"
 #include "net/base/net_errors.h"
 #include "net/dns/host_resolver.h"
 #include "net/http/http_auth_challenge_tokenizer.h"
@@ -18,6 +19,7 @@
 #include "net/http/http_request_headers.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_util.h"
+#include "net/log/net_log.h"
 
 namespace net {
 
@@ -147,4 +149,31 @@ const char* HttpAuth::SchemeToString(Scheme scheme) {
   return kSchemeNames[scheme];
 }
 
+// static
+const char* HttpAuth::AuthorizationResultToString(
+    AuthorizationResult authorization_result) {
+  switch (authorization_result) {
+    case AUTHORIZATION_RESULT_ACCEPT:
+      return "accept";
+    case AUTHORIZATION_RESULT_REJECT:
+      return "reject";
+    case AUTHORIZATION_RESULT_STALE:
+      return "stale";
+    case AUTHORIZATION_RESULT_INVALID:
+      return "invalid";
+    case AUTHORIZATION_RESULT_DIFFERENT_REALM:
+      return "different_realm";
+  }
+  NOTREACHED();
+  return "(invalid result)";
+}
+
+// static
+NetLogParametersCallback HttpAuth::NetLogAuthorizationResultCallback(
+    const char* name,
+    AuthorizationResult authorization_result) {
+  return NetLog::StringCallback(
+      name, AuthorizationResultToString(authorization_result));
+}
+
 }  // namespace net

net/http/http_auth.h

@@ -12,6 +12,7 @@
 #include "net/base/auth.h"
 #include "net/base/net_export.h"
 #include "net/http/http_util.h"
+#include "net/log/net_log_parameters_callback.h"
 
 template <class T> class scoped_refptr;
 
@@ -140,6 +141,16 @@ class NET_EXPORT_PRIVATE HttpAuth {
   // Returns a string representation of an authentication Scheme.
   static const char* SchemeToString(Scheme scheme);
 
+  // Returns a string representation of an authorization result.
+  static const char* AuthorizationResultToString(
+      AuthorizationResult authorization_result);
+
+  // Use with BoundNetLog to log an authorization result. The returned callback
+  // is valid as long as |name| is valid.
+  static NetLogParametersCallback NetLogAuthorizationResultCallback(
+      const char* name,
+      AuthorizationResult authorization_result);
+
   // Iterate through |response_headers|, and pick the best one that we support.
   // Obtains the implementation class for handling the challenge, and passes it
   // back in |*handler|. If no supported challenge was found, |*handler| is set

net/http/http_auth_handler.cc

@@ -47,22 +47,6 @@ bool HttpAuthHandler::InitFromChallenge(HttpAuthChallengeTokenizer* challenge,
   return ok;
 }
 
-namespace {
-
-NetLogEventType EventTypeFromAuthTarget(HttpAuth::Target target) {
-  switch (target) {
-    case HttpAuth::AUTH_PROXY:
-      return NetLogEventType::AUTH_PROXY;
-    case HttpAuth::AUTH_SERVER:
-      return NetLogEventType::AUTH_SERVER;
-    default:
-      NOTREACHED();
-      return NetLogEventType::CANCELLED;
-  }
-}
-
-}  // namespace
-
 int HttpAuthHandler::GenerateAuthToken(const AuthCredentials* credentials,
                                        const HttpRequestInfo* request,
                                        CompletionOnceCallback callback,
@@ -73,14 +57,14 @@ int HttpAuthHandler::GenerateAuthToken(const AuthCredentials* credentials,
   DCHECK(auth_token != nullptr);
   DCHECK(callback_.is_null());
   callback_ = std::move(callback);
-  net_log_.BeginEvent(EventTypeFromAuthTarget(target_));
+  net_log_.BeginEvent(NetLogEventType::AUTH_GENERATE_TOKEN);
   int rv = GenerateAuthTokenImpl(
       credentials, request,
       base::BindOnce(&HttpAuthHandler::OnGenerateAuthTokenComplete,
                      base::Unretained(this)),
       auth_token);
   if (rv != ERR_IO_PENDING)
-    FinishGenerateAuthToken();
+    FinishGenerateAuthToken(rv);
   return rv;
 }
 
@@ -98,15 +82,24 @@ bool HttpAuthHandler::AllowsExplicitCredentials() {
 
 void HttpAuthHandler::OnGenerateAuthTokenComplete(int rv) {
   CompletionOnceCallback callback = std::move(callback_);
-  FinishGenerateAuthToken();
+  FinishGenerateAuthToken(rv);
   DCHECK(!callback.is_null());
   std::move(callback).Run(rv);
 }
 
-void HttpAuthHandler::FinishGenerateAuthToken() {
-  // TODO(cbentzel): Should this be done in OK case only?
-  net_log_.EndEvent(EventTypeFromAuthTarget(target_));
+void HttpAuthHandler::FinishGenerateAuthToken(int rv) {
+  DCHECK_NE(rv, ERR_IO_PENDING);
+  net_log_.EndEventWithNetErrorCode(NetLogEventType::AUTH_GENERATE_TOKEN, rv);
   callback_.Reset();
 }
 
+HttpAuth::AuthorizationResult HttpAuthHandler::HandleAnotherChallenge(
+    HttpAuthChallengeTokenizer* challenge) {
+  auto authorization_result = HandleAnotherChallengeImpl(challenge);
+  net_log_.AddEvent(NetLogEventType::AUTH_HANDLE_CHALLENGE,
+                    HttpAuth::NetLogAuthorizationResultCallback(
+                        "authorization_result", authorization_result));
+  return authorization_result;
+}
+
 }  // namespace net

net/http/http_auth_handler.h

@@ -63,8 +63,8 @@ class NET_EXPORT_PRIVATE HttpAuthHandler {
   // |challenge| must be non-NULL and have already tokenized the
   // authentication scheme, but none of the tokens occurring after the
   // authentication scheme.
-  virtual HttpAuth::AuthorizationResult HandleAnotherChallenge(
-      HttpAuthChallengeTokenizer* challenge) = 0;
+  HttpAuth::AuthorizationResult HandleAnotherChallenge(
+      HttpAuthChallengeTokenizer* challenge);
 
   // Generates an authentication token, potentially asynchronously.
   //
@@ -191,6 +191,12 @@ class NET_EXPORT_PRIVATE HttpAuthHandler {
                                     CompletionOnceCallback callback,
                                     std::string* auth_token) = 0;
 
+  // See HandleAnotherChallenge() above. HandleAuthChallengeImpl is the
+  // scheme-specific implementation. Callers should use HandleAnotherChallenge()
+  // instead.
+  virtual HttpAuth::AuthorizationResult HandleAnotherChallengeImpl(
+      HttpAuthChallengeTokenizer* challenge) = 0;
+
   // The auth-scheme as an enumerated value.
   HttpAuth::Scheme auth_scheme_;
 
@@ -216,7 +222,7 @@ class NET_EXPORT_PRIVATE HttpAuthHandler {
 
  private:
   void OnGenerateAuthTokenComplete(int rv);
-  void FinishGenerateAuthToken();
+  void FinishGenerateAuthToken(int rv);
 
   // NetLog that should be used for logging events generated by this
   // HttpAuthHandler.

net/http/http_auth_handler_basic.cc

@@ -77,19 +77,6 @@ bool HttpAuthHandlerBasic::ParseChallenge(
   return true;
 }
 
-HttpAuth::AuthorizationResult HttpAuthHandlerBasic::HandleAnotherChallenge(
-    HttpAuthChallengeTokenizer* challenge) {
-  // Basic authentication is always a single round, so any responses
-  // should be treated as a rejection.  However, if the new challenge
-  // is for a different realm, then indicate the realm change.
-  std::string realm;
-  if (!ParseRealm(*challenge, &realm))
-    return HttpAuth::AUTHORIZATION_RESULT_INVALID;
-  return (realm_ != realm)?
-      HttpAuth::AUTHORIZATION_RESULT_DIFFERENT_REALM:
-      HttpAuth::AUTHORIZATION_RESULT_REJECT;
-}
-
 int HttpAuthHandlerBasic::GenerateAuthTokenImpl(
     const AuthCredentials* credentials,
     const HttpRequestInfo*,
@@ -105,6 +92,18 @@ int HttpAuthHandlerBasic::GenerateAuthTokenImpl(
   return OK;
 }
 
+HttpAuth::AuthorizationResult HttpAuthHandlerBasic::HandleAnotherChallengeImpl(
+    HttpAuthChallengeTokenizer* challenge) {
+  // Basic authentication is always a single round, so any responses
+  // should be treated as a rejection.  However, if the new challenge
+  // is for a different realm, then indicate the realm change.
+  std::string realm;
+  if (!ParseRealm(*challenge, &realm))
+    return HttpAuth::AUTHORIZATION_RESULT_INVALID;
+  return (realm_ != realm) ? HttpAuth::AUTHORIZATION_RESULT_DIFFERENT_REALM
+                           : HttpAuth::AUTHORIZATION_RESULT_REJECT;
+}
+
 HttpAuthHandlerBasic::Factory::Factory() = default;
 
 HttpAuthHandlerBasic::Factory::~Factory() = default;

net/http/http_auth_handler_basic.h

@@ -34,17 +34,16 @@ class NET_EXPORT_PRIVATE HttpAuthHandlerBasic : public HttpAuthHandler {
                           std::unique_ptr<HttpAuthHandler>* handler) override;
   };
 
-  HttpAuth::AuthorizationResult HandleAnotherChallenge(
-      HttpAuthChallengeTokenizer* challenge) override;
-
- protected:
+ private:
+  // HttpAuthHandler
   bool Init(HttpAuthChallengeTokenizer* challenge,
             const SSLInfo& ssl_info) override;
-
   int GenerateAuthTokenImpl(const AuthCredentials* credentials,
                             const HttpRequestInfo* request,
                             CompletionOnceCallback callback,
                             std::string* auth_token) override;
+  HttpAuth::AuthorizationResult HandleAnotherChallengeImpl(
+      HttpAuthChallengeTokenizer* challenge) override;
 
  private:
   ~HttpAuthHandlerBasic() override {}

net/http/http_auth_handler_digest.cc

@@ -109,7 +109,31 @@ int HttpAuthHandlerDigest::Factory::CreateAuthHandler(
   return OK;
 }
 
-HttpAuth::AuthorizationResult HttpAuthHandlerDigest::HandleAnotherChallenge(
+bool HttpAuthHandlerDigest::Init(HttpAuthChallengeTokenizer* challenge,
+                                 const SSLInfo& ssl_info) {
+  return ParseChallenge(challenge);
+}
+
+int HttpAuthHandlerDigest::GenerateAuthTokenImpl(
+    const AuthCredentials* credentials,
+    const HttpRequestInfo* request,
+    CompletionOnceCallback callback,
+    std::string* auth_token) {
+  // Generate a random client nonce.
+  std::string cnonce = nonce_generator_->GenerateNonce();
+
+  // Extract the request method and path -- the meaning of 'path' is overloaded
+  // in certain cases, to be a hostname.
+  std::string method;
+  std::string path;
+  GetRequestMethodAndPath(request, &method, &path);
+
+  *auth_token =
+      AssembleCredentials(method, path, *credentials, cnonce, nonce_count_);
+  return OK;
+}
+
+HttpAuth::AuthorizationResult HttpAuthHandlerDigest::HandleAnotherChallengeImpl(
     HttpAuthChallengeTokenizer* challenge) {
   // Even though Digest is not connection based, a "second round" is parsed
   // to differentiate between stale and rejected responses.
@@ -136,30 +160,6 @@ HttpAuth::AuthorizationResult HttpAuthHandlerDigest::HandleAnotherChallenge(
       HttpAuth::AUTHORIZATION_RESULT_REJECT;
 }
 
-bool HttpAuthHandlerDigest::Init(HttpAuthChallengeTokenizer* challenge,
-                                 const SSLInfo& ssl_info) {
-  return ParseChallenge(challenge);
-}
-
-int HttpAuthHandlerDigest::GenerateAuthTokenImpl(
-    const AuthCredentials* credentials,
-    const HttpRequestInfo* request,
-    CompletionOnceCallback callback,
-    std::string* auth_token) {
-  // Generate a random client nonce.
-  std::string cnonce = nonce_generator_->GenerateNonce();
-
-  // Extract the request method and path -- the meaning of 'path' is overloaded
-  // in certain cases, to be a hostname.
-  std::string method;
-  std::string path;
-  GetRequestMethodAndPath(request, &method, &path);
-
-  *auth_token = AssembleCredentials(method, path, *credentials,
-                                    cnonce, nonce_count_);
-  return OK;
-}
-
 HttpAuthHandlerDigest::HttpAuthHandlerDigest(
     int nonce_count, const NonceGenerator* nonce_generator)
     : stale_(false),

net/http/http_auth_handler_digest.h

@@ -81,19 +81,17 @@ class NET_EXPORT_PRIVATE HttpAuthHandlerDigest : public HttpAuthHandler {
     std::unique_ptr<const NonceGenerator> nonce_generator_;
   };
 
-  HttpAuth::AuthorizationResult HandleAnotherChallenge(
-      HttpAuthChallengeTokenizer* challenge) override;
-
- protected:
+ private:
+  // HttpAuthHandler
   bool Init(HttpAuthChallengeTokenizer* challenge,
             const SSLInfo& ssl_info) override;
-
   int GenerateAuthTokenImpl(const AuthCredentials* credentials,
                             const HttpRequestInfo* request,
                             CompletionOnceCallback callback,
                             std::string* auth_token) override;
+  HttpAuth::AuthorizationResult HandleAnotherChallengeImpl(
+      HttpAuthChallengeTokenizer* challenge) override;
 
- private:
   FRIEND_TEST_ALL_PREFIXES(HttpAuthHandlerDigestTest, ParseChallenge);
   FRIEND_TEST_ALL_PREFIXES(HttpAuthHandlerDigestTest, AssembleCredentials);
   FRIEND_TEST_ALL_PREFIXES(HttpNetworkTransactionTest, DigestPreAuthNonceCount);

net/http/http_auth_handler_mock.cc

@@ -59,26 +59,6 @@ void HttpAuthHandlerMock::SetGenerateExpectation(bool async, int rv) {
   generate_rv_ = rv;
 }
 
-HttpAuth::AuthorizationResult HttpAuthHandlerMock::HandleAnotherChallenge(
-    HttpAuthChallengeTokenizer* challenge) {
-  EXPECT_THAT(state_, ::testing::AnyOf(State::WAIT_FOR_CHALLENGE,
-                                       State::WAIT_FOR_GENERATE_AUTH_TOKEN));
-  // If we receive an empty challenge for a connection based scheme, or a second
-  // challenge for a non connection based scheme, assume it's a rejection.
-  if (!is_connection_based() || challenge->base64_param().empty()) {
-    state_ = State::DONE;
-    return HttpAuth::AUTHORIZATION_RESULT_REJECT;
-  }
-
-  if (!base::LowerCaseEqualsASCII(challenge->scheme(), "mock")) {
-    state_ = State::DONE;
-    return HttpAuth::AUTHORIZATION_RESULT_INVALID;
-  }
-
-  state_ = State::WAIT_FOR_GENERATE_AUTH_TOKEN;
-  return HttpAuth::AUTHORIZATION_RESULT_ACCEPT;
-}
-
 bool HttpAuthHandlerMock::NeedsIdentity() {
   return first_round_;
 }
@@ -131,6 +111,26 @@ int HttpAuthHandlerMock::GenerateAuthTokenImpl(
   }
 }
 
+HttpAuth::AuthorizationResult HttpAuthHandlerMock::HandleAnotherChallengeImpl(
+    HttpAuthChallengeTokenizer* challenge) {
+  EXPECT_THAT(state_, ::testing::AnyOf(State::WAIT_FOR_CHALLENGE,
+                                       State::WAIT_FOR_GENERATE_AUTH_TOKEN));
+  // If we receive an empty challenge for a connection based scheme, or a second
+  // challenge for a non connection based scheme, assume it's a rejection.
+  if (!is_connection_based() || challenge->base64_param().empty()) {
+    state_ = State::DONE;
+    return HttpAuth::AUTHORIZATION_RESULT_REJECT;
+  }
+
+  if (!base::LowerCaseEqualsASCII(challenge->scheme(), "mock")) {
+    state_ = State::DONE;
+    return HttpAuth::AUTHORIZATION_RESULT_INVALID;
+  }
+
+  state_ = State::WAIT_FOR_GENERATE_AUTH_TOKEN;
+  return HttpAuth::AUTHORIZATION_RESULT_ACCEPT;
+}
+
 void HttpAuthHandlerMock::OnGenerateAuthToken() {
   EXPECT_TRUE(generate_async_);
   EXPECT_TRUE(!callback_.is_null());