Use HttpAuthController in SocketStream

net/socket_stream/socket_stream.cc

Index: net/socket_stream/socket_stream.cc
diff --git a/net/socket_stream/socket_stream.cc b/net/socket_stream/socket_stream.cc
index f656cb8b94ab447a84114c8d2f0fe72b1d70ed19..c3020b499ba61ddda9c549e0849256c2dcef8baa 100644
--- a/net/socket_stream/socket_stream.cc
+++ b/net/socket_stream/socket_stream.cc
@@ -25,8 +25,10 @@
 #include "net/base/net_errors.h"
 #include "net/base/net_util.h"
 #include "net/base/ssl_cert_request_info.h"
+#include "net/http/http_auth_controller.h"
 #include "net/http/http_auth_handler_factory.h"
 #include "net/http/http_network_session.h"
+#include "net/http/http_request_headers.h"
 #include "net/http/http_request_info.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_stream_factory.h"
@@ -92,7 +94,6 @@ SocketStream::SocketStream(const GURL& url, Delegate* delegate)
       host_resolver_(NULL),
       cert_verifier_(NULL),
       server_bound_cert_service_(NULL),
-      http_auth_handler_factory_(NULL),
       factory_(ClientSocketFactory::GetDefaultFactory()),
       proxy_mode_(kDirectConnection),
       proxy_url_(url),
@@ -160,7 +161,6 @@ void SocketStream::set_context(const URLRequestContext* context) {
     host_resolver_ = context_->host_resolver();
     cert_verifier_ = context_->cert_verifier();
     server_bound_cert_service_ = context_->server_bound_cert_service();
-    http_auth_handler_factory_ = context_->http_auth_handler_factory();
   }
 }
 
@@ -244,18 +244,13 @@ void SocketStream::RestartWithAuth(const AuthCredentials& credentials) {
       "The current MessageLoop must exist";
   DCHECK_EQ(MessageLoop::TYPE_IO, MessageLoop::current()->type()) <<
       "The current MessageLoop must be TYPE_IO";
-  DCHECK(auth_handler_.get());
+  DCHECK(proxy_auth_controller_.get());
   if (!socket_.get()) {
     LOG(ERROR) << "Socket is closed before restarting with auth.";
     return;
   }
 
-  if (auth_identity_.invalid) {
-    // Update the credentials.
-    auth_identity_.source = HttpAuth::IDENT_SRC_EXTERNAL;
-    auth_identity_.invalid = false;
-    auth_identity_.credentials = credentials;
-  }
+  proxy_auth_controller_->ResetAuth(credentials);
 
   MessageLoop::current()->PostTask(
       FROM_HERE,
@@ -475,6 +470,12 @@ void SocketStream::DoLoop(int result) {
       case STATE_TCP_CONNECT_COMPLETE:
         result = DoTcpConnectComplete(result);
         break;
+      case STATE_GENERATE_PROXY_AUTH_TOKEN:
+        result = DoGenerateProxyAuthToken();
+        break;
+      case STATE_GENERATE_PROXY_AUTH_TOKEN_COMPLETE:
+        result = DoGenerateProxyAuthTokenComplete(result);
+        break;
       case STATE_WRITE_TUNNEL_HEADERS:
         DCHECK_EQ(OK, result);
         result = DoWriteTunnelHeaders();
@@ -708,6 +709,10 @@ int SocketStream::DoTcpConnect(int result) {
   return socket_->Connect(io_callback_);
 }
 
+bool SocketStream::ShouldApplyProxyAuth() const {
+  return !is_secure() && (proxy_info_.is_http() || proxy_info_.is_https());

[wtc, 2012/08/14 18:34:37]

IMPORTANT: can you explain why you test !is_secure() here?

[bashi, 2012/08/14 21:45:45]

Done. The logic is the same as HttpNetworkTransaction::ShouldApplyProxyAuth().

[wtc, 2012/08/14 23:20:35]

Thank you for the explanation.  I think this test may
be incorrect for this class.

Based on the code on lines 763-774 in
SocketStream::DoGenerateProxyAuthTokenComplete, it seems
that we need the proxy auth header if proxy_mode_ == kTunnelProxy,
so it seems that we should test that in this function.

HttpNetworkTransaction is different because it seems to
do tunnel connection in some other class.

[bashi, 2012/08/20 01:35:10]

You are right. Removed !is_secure() and added the check (proxy_mode_ ==
kTunnelProxy).

+}
+
 int SocketStream::DoTcpConnectComplete(int result) {
   // TODO(ukai): if error occured, reconsider proxy after error.
   if (result != OK) {
@@ -715,6 +720,46 @@ int SocketStream::DoTcpConnectComplete(int result) {
     return result;
   }
 
+  if (ShouldApplyProxyAuth())
+    next_state_ = STATE_GENERATE_PROXY_AUTH_TOKEN;
+  else
+    next_state_ = STATE_GENERATE_PROXY_AUTH_TOKEN_COMPLETE;
+  return result;
+}
+
+int SocketStream::DoGenerateProxyAuthToken() {
+  next_state_ = STATE_GENERATE_PROXY_AUTH_TOKEN_COMPLETE;
+  if (!proxy_auth_controller_.get()) {
+    DCHECK(context_);
+    DCHECK(context_->http_transaction_factory());
+    DCHECK(context_->http_transaction_factory()->GetSession());
+    HttpNetworkSession* session =
+        context_->http_transaction_factory()->GetSession();
+    const char* scheme = proxy_info_.is_https() ? "https://" : "http://";
+    GURL auth_url(scheme +
+                  proxy_info_.proxy_server().host_port_pair().ToString());
+    proxy_auth_controller_ =
+        new HttpAuthController(HttpAuth::AUTH_PROXY,
+                               auth_url,
+                               session->http_auth_cache(),
+                               session->http_auth_handler_factory());
+  }
+  HttpRequestInfo request_info;
+  request_info.url = url();

[wtc, 2012/08/14 18:34:37]

Nit: this request_info.url assignment should be unnecessary.

[bashi, 2012/08/14 21:45:45]

Removed.

+  request_info.method = "GET";
+  request_info.load_flags = 0;
+  request_info.priority = MEDIUM;
+  request_info.request_id = 0;
+  return proxy_auth_controller_->MaybeGenerateAuthToken(
+      &request_info, io_callback_, net_log_);
+}
+
+int SocketStream::DoGenerateProxyAuthTokenComplete(int result) {
+  if (result != OK) {
+    next_state_ = STATE_CLOSE;
+    return result;
+  }
+
   if (proxy_mode_ == kTunnelProxy) {
     if (proxy_info_.is_https())
       next_state_ = STATE_SECURE_PROXY_CONNECT;
@@ -741,58 +786,19 @@ int SocketStream::DoWriteTunnelHeaders() {
     tunnel_request_headers_bytes_sent_ = 0;
   }
   if (tunnel_request_headers_->headers_.empty()) {
-    std::string authorization_headers;
-
-    if (!auth_handler_.get()) {
-      // Do preemptive authentication.
-      HttpAuthCache::Entry* entry = auth_cache_.LookupByPath(
-          ProxyAuthOrigin(), std::string());
-      if (entry) {
-        scoped_ptr<HttpAuthHandler> handler_preemptive;
-        int rv_create = http_auth_handler_factory_->
-            CreatePreemptiveAuthHandlerFromString(
-                entry->auth_challenge(), HttpAuth::AUTH_PROXY,
-                ProxyAuthOrigin(), entry->IncrementNonceCount(),
-                net_log_, &handler_preemptive);
-        if (rv_create == OK) {
-          auth_identity_.source = HttpAuth::IDENT_SRC_PATH_LOOKUP;
-          auth_identity_.invalid = false;
-          auth_identity_.credentials = AuthCredentials();
-          auth_handler_.swap(handler_preemptive);
-        }
-      }
-    }
-
-    // Support basic authentication scheme only, because we don't have
-    // HttpRequestInfo.
-    // TODO(ukai): Add support other authentication scheme.
-    if (auth_handler_.get() &&
-        auth_handler_->auth_scheme() == HttpAuth::AUTH_SCHEME_BASIC) {
-      HttpRequestInfo request_info;
-      std::string auth_token;
-      int rv = auth_handler_->GenerateAuthToken(
-          &auth_identity_.credentials,
-          &request_info,
-          CompletionCallback(),
-          &auth_token);
-      // TODO(cbentzel): Support async auth handlers.
-      DCHECK_NE(ERR_IO_PENDING, rv);
-      if (rv != OK)
-        return rv;
-      authorization_headers.append(
-          HttpAuth::GetAuthorizationHeaderName(HttpAuth::AUTH_PROXY) +
-          ": " + auth_token + "\r\n");
-    }
-
+    HttpRequestHeaders authorization_headers;
+    if (proxy_auth_controller_.get() && proxy_auth_controller_->HaveAuth())
+      proxy_auth_controller_->AddAuthorizationHeader(&authorization_headers);
     tunnel_request_headers_->headers_ = base::StringPrintf(
         "CONNECT %s HTTP/1.1\r\n"
         "Host: %s\r\n"
         "Proxy-Connection: keep-alive\r\n",
         GetHostAndPort(url_).c_str(),
         GetHostAndOptionalPort(url_).c_str());
-    if (!authorization_headers.empty())
-      tunnel_request_headers_->headers_ += authorization_headers;
-    tunnel_request_headers_->headers_ += "\r\n";
+    if (authorization_headers.IsEmpty())
+      tunnel_request_headers_->headers_ += "\r\n";
+    else
+      tunnel_request_headers_->headers_ += authorization_headers.ToString();

[wtc, 2012/08/14 18:34:37]

IMPORTANT: this seems wrong.  This implies that
authorization_headers.ToString() ends with "\r\n\r\n".

Update: I see why this code is correct now.  Perhaps
it would be clearer to build tunnel_request_headers_
from a HttpRequestHeaders.  I am not suggesting that
you make that change.  Just making an observation.

[bashi, 2012/08/14 21:45:45]

I agree. Done.

[wtc, 2012/08/14 23:20:35]

Great!  This change exceeds my expectations :-)

   }
   tunnel_request_headers_->SetDataOffset(tunnel_request_headers_bytes_sent_);
   int buf_len = static_cast<int>(tunnel_request_headers_->headers_.size() -
@@ -899,16 +905,13 @@ int SocketStream::DoReadTunnelHeadersComplete(int result) {
       }
       return OK;
     case 407:  // Proxy Authentication Required.
-      result = HandleAuthChallenge(headers.get());
-      if (result == ERR_PROXY_AUTH_UNSUPPORTED &&
-          auth_handler_.get() && delegate_) {
+      if (!proxy_auth_controller_.get() || proxy_mode_ != kTunnelProxy ||
+          proxy_info_.is_direct())
+        return ERR_UNEXPECTED_PROXY_AUTH;

[wtc, 2012/08/14 18:34:37]

Nit: add curly braces because the conditional expression is
more than one line.

[bashi, 2012/08/14 21:45:45]

Done.

+      result = proxy_auth_controller_->HandleAuthChallenge(
+          headers, false, true, net_log_);
+      if (result == OK && delegate_) {
         DCHECK(!proxy_info_.is_empty());
-        auth_info_ = new AuthChallengeInfo;
-        auth_info_->is_proxy = true;
-        auth_info_->challenger = proxy_info_.proxy_server().host_port_pair();
-        auth_info_->scheme = HttpAuth::SchemeToString(
-            auth_handler_->auth_scheme());
-        auth_info_->realm = auth_handler_->realm();
         // Wait until RestartWithAuth or Close is called.
         MessageLoop::current()->PostTask(
             FROM_HERE,
@@ -1144,53 +1147,6 @@ GURL SocketStream::ProxyAuthOrigin() const {
               proxy_info_.proxy_server().host_port_pair().ToString());
 }
 
-int SocketStream::HandleAuthChallenge(const HttpResponseHeaders* headers) {
-  GURL auth_origin(ProxyAuthOrigin());
-
-  VLOG(1) << "The proxy " << auth_origin << " requested auth";
-
-  // TODO(cbentzel): Since SocketStream only suppports basic authentication
-  // right now, another challenge is always treated as a rejection.
-  // Ultimately this should be converted to use HttpAuthController like the
-  // HttpNetworkTransaction has.
-  if (auth_handler_.get() && !auth_identity_.invalid) {
-    if (auth_identity_.source != HttpAuth::IDENT_SRC_PATH_LOOKUP)
-      auth_cache_.Remove(auth_origin,
-                         auth_handler_->realm(),
-                         auth_handler_->auth_scheme(),
-                         auth_identity_.credentials);
-    auth_handler_.reset();
-    auth_identity_ = HttpAuth::Identity();
-  }
-
-  auth_identity_.invalid = true;
-  std::set<HttpAuth::Scheme> disabled_schemes;
-  HttpAuth::ChooseBestChallenge(http_auth_handler_factory_, headers,
-                                HttpAuth::AUTH_PROXY,
-                                auth_origin, disabled_schemes,
-                                net_log_, &auth_handler_);
-  if (!auth_handler_.get()) {
-    LOG(ERROR) << "Can't perform auth to the proxy " << auth_origin;
-    return ERR_TUNNEL_CONNECTION_FAILED;
-  }
-  if (auth_handler_->NeedsIdentity()) {
-    // We only support basic authentication scheme now.
-    // TODO(ukai): Support other authentication scheme.
-    HttpAuthCache::Entry* entry = auth_cache_.Lookup(
-        auth_origin, auth_handler_->realm(), HttpAuth::AUTH_SCHEME_BASIC);
-    if (entry) {
-      auth_identity_.source = HttpAuth::IDENT_SRC_REALM_LOOKUP;
-      auth_identity_.invalid = false;
-      auth_identity_.credentials = AuthCredentials();
-      // Restart with auth info.
-    }
-    return ERR_PROXY_AUTH_UNSUPPORTED;
-  } else {
-    auth_identity_.invalid = false;
-  }
-  return ERR_TUNNEL_CONNECTION_FAILED;
-}
-
 int SocketStream::HandleCertificateRequest(int result, SSLConfig* ssl_config) {
   if (ssl_config->send_client_cert)
     // We already have performed SSL client authentication once and failed.
@@ -1271,21 +1227,14 @@ int SocketStream::AllowCertErrorForReconnection(SSLConfig* ssl_config) {
 }
 
 void SocketStream::DoAuthRequired() {
-  if (delegate_ && auth_info_.get())
-    delegate_->OnAuthRequired(this, auth_info_.get());
+  if (delegate_ && proxy_auth_controller_.get())
+    delegate_->OnAuthRequired(this, proxy_auth_controller_->auth_info().get());
   else
     DoLoop(ERR_UNEXPECTED);
 }
 
 void SocketStream::DoRestartWithAuth() {
   DCHECK_EQ(next_state_, STATE_AUTH_REQUIRED);
-  auth_cache_.Add(ProxyAuthOrigin(),
-                  auth_handler_->realm(),
-                  auth_handler_->auth_scheme(),
-                  auth_handler_->challenge(),
-                  auth_identity_.credentials,
-                  std::string());
-
   tunnel_request_headers_ = NULL;
   tunnel_request_headers_bytes_sent_ = 0;
   tunnel_response_headers_ = NULL;