Clear HTTP auth data on clearing browsing data

chrome/browser/browsing_data/browsing_data_remover.cc

Index: chrome/browser/browsing_data/browsing_data_remover.cc
diff --git a/chrome/browser/browsing_data/browsing_data_remover.cc b/chrome/browser/browsing_data/browsing_data_remover.cc
index c58ab51a4923239b8d8d0f3ea16a5e224ae75b50..8f456f7efedba372733ecb1d82bb7f0b1e24c849 100644
--- a/chrome/browser/browsing_data/browsing_data_remover.cc
+++ b/chrome/browser/browsing_data/browsing_data_remover.cc
@@ -72,6 +72,8 @@
 #include "content/public/browser/user_metrics.h"
 #include "net/base/net_errors.h"
 #include "net/cookies/cookie_store.h"
+#include "net/http/http_network_session.h"
+#include "net/http/http_transaction_factory.h"
 #include "net/http/transport_security_state.h"
 #include "net/ssl/channel_id_service.h"
 #include "net/ssl/channel_id_store.h"
@@ -183,6 +185,19 @@ void ClearHostnameResolutionCacheOnIOThread(IOThread* io_thread) {
   io_thread->ClearHostCache();
 }
 
+void ClearHttpAuthCacheOnIOThread(
+    scoped_refptr<net::URLRequestContextGetter> context_getter,
+    base::Time delete_begin,
+    base::Time delete_end) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  net::HttpNetworkSession* http_session = context_getter->GetURLRequestContext()
+                                              ->http_transaction_factory()
+                                              ->GetSession();
+  DCHECK(http_session);
+  http_session->http_auth_cache()->Clear(delete_begin, delete_end);

[mmenke, 2016/07/12 16:27:11]

It makes no sense to clear auth but not closing all connections, as some may
have negotiated a connection using those credendials.  CloseAllConnections is
also a method on HttpNetworkSesssion.

[Tomasz Moniuszko, 2016/07/15 11:41:34]

Done.

+}
+
 void ClearNetworkPredictorOnIOThread(chrome_browser_net::Predictor* predictor) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(predictor);
@@ -900,6 +915,19 @@ void BrowsingDataRemover::RemoveImpl(
                    weak_ptr_factory_.GetWeakPtr()));
   }
 
+  if (remove_mask & REMOVE_CACHE || remove_mask & REMOVE_COOKIES ||

[mmenke, 2016/07/12 16:27:11]

Hrm...Should we really clear this when clearing the cache?

[msramek, 2016/07/12 16:52:14]

Since this will log you out of the affected sites, it's an equivalent of cookies
- that part is the most important. I am leaning towards deleting it with cache
as well to avoid confusion, since it's called HTTP auth cache - but I'm not
feeling that strongly about it.

But I can't remember what was the reason for passwords (sorry, perhaps I lost
some context again). Clearing regular passwords (in the password manager) does
not log you out immediately (i.e. it doesn't delete cookies). The same should
probably apply here as well - if you deleted your saved (HTTP auth) passwords,
it shouldn't log you out immediately.

[asanka, 2016/07/12 17:50:32]

The semantics are related, but slightly different.

There should be no expectations that a user would be reliably "logged out" when
clearing the HTTP auth cache.

The auth cache is irrelevant for ambient authentication (when the user's OS
provided credentials are used for authentication).

Basic and Digest authentication is per-request, and an implementation is free to
tie an identity to a cookie after a successful authentication attempt. In such
cases, removing cached basic passwords don't affect the "logged in" state for a
user unless they also clear cookies.

NTLM and Negotiate authentication is per-connection. The server is free to
associate the identity of the user with any requests made over the same
connection (hence mmenke's suggestion to close active connections). Once again,
the server is free to associate the identity with a cookie.

Hence we'd need to clear cookies, the auth cache and then close all active
connections to approximate logging out. But none of that works with ambient
credentials.

That said:

The auth cache is a per-session password store with no persistence semantics.
Hence it makes sense to clear the auth cache when clearing saved passwords. The
observable behavior is that this would prevent Chrome from authenticating the
user using those stored passwords for new connections.

Clearing the auth cache is not consistent with what a user would expect if they
were clearing the cache but not the cookies.

Clearing the auth cache is consistent with clearing cookies insofar the user
expects any identity established during the relevant time period to be
dismantled.

[Tomasz Moniuszko, 2016/07/15 11:41:34]

REMOVE_CACHE test removed in Patch Set 5.

+      remove_mask & REMOVE_PASSWORDS) {
+    scoped_refptr<net::URLRequestContextGetter> request_context =
+        profile_->GetRequestContext();
+    waiting_for_clear_http_auth_cache_ = true;
+    BrowserThread::PostTaskAndReply(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&ClearHttpAuthCacheOnIOThread, std::move(request_context),
+                   delete_begin_, delete_end_),
+        base::Bind(&BrowsingDataRemover::OnClearedHttpAuthCache,
+                   weak_ptr_factory_.GetWeakPtr()));
+  }
+
   if (remove_mask & REMOVE_WEBRTC_IDENTITY) {
     storage_partition_remove_mask |=
         content::StoragePartition::REMOVE_DATA_MASK_WEBRTC_IDENTITY;
@@ -1108,6 +1136,7 @@ bool BrowsingDataRemover::AllDone() {
          !waiting_for_clear_domain_reliability_monitor_ &&
          !waiting_for_clear_form_ && !waiting_for_clear_history_ &&
          !waiting_for_clear_hostname_resolution_cache_ &&
+         !waiting_for_clear_http_auth_cache_ &&
          !waiting_for_clear_keyword_data_ && !waiting_for_clear_nacl_cache_ &&
          !waiting_for_clear_network_predictor_ &&
          !waiting_for_clear_networking_history_ &&
@@ -1180,6 +1209,12 @@ void BrowsingDataRemover::OnClearedHostnameResolutionCache() {
   NotifyIfDone();
 }
 
+void BrowsingDataRemover::OnClearedHttpAuthCache() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  waiting_for_clear_http_auth_cache_ = false;
+  NotifyIfDone();
+}
+
 void BrowsingDataRemover::OnClearedNetworkPredictor() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   waiting_for_clear_network_predictor_ = false;