Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2097043002:
Clear HTTP auth data on clearing browsing data (Closed)
|
Created:
4 years, 6 months ago by Tomasz Moniuszko Modified:
4 years, 3 months ago CC:
chromium-reviews, gavinp+disk_chromium.org, kinuko+cache_chromium.org, ramant (doing other things) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClear HTTP auth data on clearing cache
BUG=108291
Committed: https://crrev.com/053298c3828bff48de45264c8263af6e82d5c354
Cr-Commit-Position: refs/heads/master@{#415919}
Patch Set 1 #Patch Set 2 : Delete within time period + delete with cookies and passwords #
Total comments: 10
Patch Set 3 : Clear auth data directly from BrowsingDataRemover #
Total comments: 4
Patch Set 4 : Fix review issues #
Total comments: 8
Patch Set 5 : Fix review issues #
Total comments: 2
Patch Set 6 : Add unit tests #
Total comments: 1
Patch Set 7 : Minor unit tests fixes #
Total comments: 6
Patch Set 8 : More unittest fixes #
Total comments: 1
Patch Set 9 : Fixup to HttpAuthCacheTest.ClearEntriesAddedWithin #
Total comments: 2
Patch Set 10 : Replace FRIEND_TEST_ALL_PREFIXES with set_creation_time_for_testing #
Messages
Total messages: 44 (11 generated)
tmoniuszko@opera.com changed reviewers: + markusheintz@chromium.org, msramek@chromium.org
@msramek: I know the bug is assigned to you but since I have the fix ready, I decided to upload it.
Yep, no worries, I'm happy if you take it :) I was actually looking into it in the past (https://codereview.chromium.org/1484143003/), but haven't had time to get back to it. I can pass you the feedback that I got: "I don't think it makes much sense to clear auth information without also flushing the socket pools. Otherwise, we may still have reusable connections around that actually use that information." (mmenke@) Furthermore, since HttpAuthCache does have a notion of origins and timestamps, we should only delete data corresponding to the given |delete_begin_|, |delete_end_|, and |url_predicate_|. Finally, these data should be also deleted with cookies (and possibly passwords, but I'll have to investigate whether password manager still works with http basic auth). So StoragePartitionHTTPCacheDataRemover might not be the best class to put it. But it's certainly better than nothing, just please change the title of your CL to "...on clearing cache".
Description was changed from ========== Clear HTTP auth data on clearing browsing data BUG=108291 ========== to ========== Clear HTTP auth data on clearing cache BUG=108291 ==========
tmoniuszko@opera.com changed reviewers: + rtenneti@chromium.org
> Furthermore, since HttpAuthCache does have a notion of origins and timestamps, > we should only delete data corresponding to the given |delete_begin_|, > |delete_end_|, and |url_predicate_|. Time period is now considered (Patch Set 2). > > Finally, these data should be also deleted with cookies (and possibly passwords, > but I'll have to investigate whether password manager still works with http > basic auth). So StoragePartitionHTTPCacheDataRemover might not be the best class > to put it. But it's certainly better than nothing, just please change the title > of your CL to "...on clearing cache". I modified BrowsingDataRemover::RemoveImpl() in Patch Set 2. It now calls StoragePartitionHTTPCacheDataRemover also in case of cookies and passwords. Auth data is being cleared properly but I'm not sure about this change as HTTP cache might be cleared too aggressively now. What do you think?
https://codereview.chromium.org/2097043002/diff/20001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/2097043002/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.cc:843: remove_mask & REMOVE_PASSWORDS) { This is not correct. Yes, you will delete the HTTP auth data when user chooses to REMOVE_CACHE, REMOVE_COOKIES, or REMOVE_PASSWORDS. But StoragePartitionHttpCacheDataRemover does a lot more than that, it wipes the entire HTTP and media caches. It makes no sense to do that if the user just wanted to delete passwords. We should not involve StoragePartitionHttpCacheDataRemover at all. Instead, we should call http_auth_cache()->Clear() directly from BrowsingDataRemover. It might require you jumping to the IO thread and back - in that case, check out ClearHostnameResolutionCacheOnIOThread in this file for an example how to do that. https://codereview.chromium.org/2097043002/diff/20001/net/http/http_auth_cach... File net/http/http_auth_cache.cc (right): https://codereview.chromium.org/2097043002/diff/20001/net/http/http_auth_cach... net/http/http_auth_cache.cc:259: : (delete_begin - base::Time::UnixEpoch()).InMilliseconds(); Can you use base::Time::ToTimeT() to do these conversions? https://codereview.chromium.org/2097043002/diff/20001/net/http/http_auth_cach... net/http/http_auth_cache.cc:271: return creation_millis >= begin_millis && creation_millis <= end_millis; BrowsingDataRemover always uses half-closed intervals for deletion (i.e. < instead of <=). https://codereview.chromium.org/2097043002/diff/20001/net/http/http_auth_cache.h File net/http/http_auth_cache.h (right): https://codereview.chromium.org/2097043002/diff/20001/net/http/http_auth_cach... net/http/http_auth_cache.h:165: // Clears the cache. Please improve the comment explain the parameters.
rtenneti@chromium.org changed reviewers: + mmenke@chromium.org - rtenneti@chromium.org
https://codereview.chromium.org/2097043002/diff/20001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/2097043002/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.cc:843: remove_mask & REMOVE_PASSWORDS) { On 2016/06/30 11:36:20, msramek wrote: > This is not correct. > > Yes, you will delete the HTTP auth data when user chooses to REMOVE_CACHE, > REMOVE_COOKIES, or REMOVE_PASSWORDS. > > But StoragePartitionHttpCacheDataRemover does a lot more than that, it wipes the > entire HTTP and media caches. It makes no sense to do that if the user just > wanted to delete passwords. > > We should not involve StoragePartitionHttpCacheDataRemover at all. Instead, we > should call http_auth_cache()->Clear() directly from BrowsingDataRemover. It > might require you jumping to the IO thread and back - in that case, check out > ClearHostnameResolutionCacheOnIOThread in this file for an example how to do > that. Done. https://codereview.chromium.org/2097043002/diff/20001/net/http/http_auth_cach... File net/http/http_auth_cache.cc (right): https://codereview.chromium.org/2097043002/diff/20001/net/http/http_auth_cach... net/http/http_auth_cache.cc:259: : (delete_begin - base::Time::UnixEpoch()).InMilliseconds(); On 2016/06/30 11:36:20, msramek wrote: > Can you use base::Time::ToTimeT() to do these conversions? There's a TODO saying base::Time::ToTimeT() should be removed. Should I use it anyway? https://codereview.chromium.org/2097043002/diff/20001/net/http/http_auth_cach... net/http/http_auth_cache.cc:271: return creation_millis >= begin_millis && creation_millis <= end_millis; On 2016/06/30 11:36:20, msramek wrote: > BrowsingDataRemover always uses half-closed intervals for deletion (i.e. < > instead of <=). Done. https://codereview.chromium.org/2097043002/diff/20001/net/http/http_auth_cache.h File net/http/http_auth_cache.h (right): https://codereview.chromium.org/2097043002/diff/20001/net/http/http_auth_cach... net/http/http_auth_cache.h:165: // Clears the cache. On 2016/06/30 11:36:20, msramek wrote: > Please improve the comment explain the parameters. Done.
Much better :) I left a few more comments. I'll wait for mmenke@ to comment on what else needs to be done to properly clean the auth cache. If there are no more structural changes to browsing_data/, I'll stamp it. https://codereview.chromium.org/2097043002/diff/20001/net/http/http_auth_cach... File net/http/http_auth_cache.cc (right): https://codereview.chromium.org/2097043002/diff/20001/net/http/http_auth_cach... net/http/http_auth_cache.cc:259: : (delete_begin - base::Time::UnixEpoch()).InMilliseconds(); On 2016/07/01 12:44:02, Tomasz Moniuszko wrote: > On 2016/06/30 11:36:20, msramek wrote: > > Can you use base::Time::ToTimeT() to do these conversions? > > There's a TODO saying base::Time::ToTimeT() should be removed. Should I use it > anyway? I would say yes. The TODO says that the function should be removed when no one uses time_t anymore, but if HttpAuthCache still does use it, I think that's fair. https://codereview.chromium.org/2097043002/diff/40001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/2097043002/diff/40001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.cc:919: remove_mask & REMOVE_PASSWORDS && g_browser_process->io_thread()) { The g_browser_process->io_thread() is not necessary - BrowserProcessImpl::io_thread() DCHECKs that it's not null. https://codereview.chromium.org/2097043002/diff/40001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.cc:920: scoped_refptr<net::URLRequestContextGetter> request_context = This is not really related to storage partition, so it would be cleaner to do just: profile_->GetRequestContext()->GetURLRequestContext()
https://codereview.chromium.org/2097043002/diff/20001/net/http/http_auth_cach... File net/http/http_auth_cache.cc (right): https://codereview.chromium.org/2097043002/diff/20001/net/http/http_auth_cach... net/http/http_auth_cache.cc:259: : (delete_begin - base::Time::UnixEpoch()).InMilliseconds(); On 2016/07/01 13:19:40, msramek wrote: > On 2016/07/01 12:44:02, Tomasz Moniuszko wrote: > > On 2016/06/30 11:36:20, msramek wrote: > > > Can you use base::Time::ToTimeT() to do these conversions? > > > > There's a TODO saying base::Time::ToTimeT() should be removed. Should I use it > > anyway? > > I would say yes. The TODO says that the function should be removed when no one > uses time_t anymore, but if HttpAuthCache still does use it, I think that's > fair. Done. https://codereview.chromium.org/2097043002/diff/40001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/2097043002/diff/40001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.cc:919: remove_mask & REMOVE_PASSWORDS && g_browser_process->io_thread()) { On 2016/07/01 13:19:40, msramek wrote: > The g_browser_process->io_thread() is not necessary - > BrowserProcessImpl::io_thread() DCHECKs that it's not null. Done. https://codereview.chromium.org/2097043002/diff/40001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.cc:920: scoped_refptr<net::URLRequestContextGetter> request_context = On 2016/07/01 13:19:40, msramek wrote: > This is not really related to storage partition, so it would be cleaner to do > just: > > profile_->GetRequestContext()->GetURLRequestContext() Done.
mmenke@chromium.org changed reviewers: + asanka@chromium.org
[+asanka]: Deferring to asanka on this one. I'm shocked we aren't already doing this, but doesn't look like we are. https://codereview.chromium.org/2097043002/diff/60001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/2097043002/diff/60001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.cc:198: http_session->http_auth_cache()->Clear(delete_begin, delete_end); 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. https://codereview.chromium.org/2097043002/diff/60001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.cc:918: if (remove_mask & REMOVE_CACHE || remove_mask & REMOVE_COOKIES || Hrm...Should we really clear this when clearing the cache?
mmenke@chromium.org changed reviewers: - mmenke@chromium.org
https://codereview.chromium.org/2097043002/diff/60001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/2097043002/diff/60001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.cc:918: if (remove_mask & REMOVE_CACHE || remove_mask & REMOVE_COOKIES || On 2016/07/12 16:27:11, mmenke wrote: > Hrm...Should we really clear this when clearing the cache? 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.
https://codereview.chromium.org/2097043002/diff/60001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/2097043002/diff/60001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.cc:918: if (remove_mask & REMOVE_CACHE || remove_mask & REMOVE_COOKIES || On 2016/07/12 at 16:52:14, msramek wrote: > On 2016/07/12 16:27:11, mmenke wrote: > > Hrm...Should we really clear this when clearing the cache? > > 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. 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. https://codereview.chromium.org/2097043002/diff/60001/net/http/http_auth_cach... File net/http/http_auth_cache.cc (right): https://codereview.chromium.org/2097043002/diff/60001/net/http/http_auth_cach... net/http/http_auth_cache.cc:255: void HttpAuthCache::Clear(base::Time delete_begin, base::Time delete_end) { How feasible would it be to have the API be: void Clear(base::TimeDelta duration) where any auth cache entry created within |duration| of base::TimeTicks::Now() would be removed? It seems such an API would be comparable in accuracy to what's being done here and would also be independent of whether times are store in TimeTicks or Time.
https://codereview.chromium.org/2097043002/diff/60001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/2097043002/diff/60001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.cc:198: http_session->http_auth_cache()->Clear(delete_begin, delete_end); On 2016/07/12 16:27:11, mmenke wrote: > 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. Done. https://codereview.chromium.org/2097043002/diff/60001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.cc:918: if (remove_mask & REMOVE_CACHE || remove_mask & REMOVE_COOKIES || On 2016/07/12 17:50:32, asanka wrote: > On 2016/07/12 at 16:52:14, msramek wrote: > > On 2016/07/12 16:27:11, mmenke wrote: > > > Hrm...Should we really clear this when clearing the cache? > > > > 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. > > 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. REMOVE_CACHE test removed in Patch Set 5. https://codereview.chromium.org/2097043002/diff/60001/net/http/http_auth_cach... File net/http/http_auth_cache.cc (right): https://codereview.chromium.org/2097043002/diff/60001/net/http/http_auth_cach... net/http/http_auth_cache.cc:255: void HttpAuthCache::Clear(base::Time delete_begin, base::Time delete_end) { On 2016/07/12 17:50:32, asanka wrote: > How feasible would it be to have the API be: void Clear(base::TimeDelta > duration) where any auth cache entry created within |duration| of > base::TimeTicks::Now() would be removed? It seems such an API would be > comparable in accuracy to what's being done here and would also be independent > of whether times are store in TimeTicks or Time. Done.
ping :) I think I've addressed all the issues raised in the review. Reviewers, does the patch look good to you?
LGTM I was actually wondering why we didn't hear from you so long, and now I see that you finished the CL two weeks ago. Not sure how I missed it, sorry for that :(
msramek@chromium.org changed reviewers: + mmenke@chromium.org
Readding mmenke@ for net/, since Asanka is OOO.
HttpAuthCache::Clear needs unit tests (It didn't have them before...but it also looks like it wasn't being used before). Seems like BrowsingDataRemover should have them, too. https://codereview.chromium.org/2097043002/diff/80001/net/http/http_auth_cache.h File net/http/http_auth_cache.h (right): https://codereview.chromium.org/2097043002/diff/80001/net/http/http_auth_cach... net/http/http_auth_cache.h:30: class NET_EXPORT_PRIVATE HttpAuthCache { This should presumably be NET_EXPORT, if we're using it externally. https://codereview.chromium.org/2097043002/diff/80001/net/http/http_auth_cach... net/http/http_auth_cache.h:166: void Clear(base::TimeDelta duration); This method name is unclear. Maybe make it take a base::TimeTicks, and call it Clear[Entries]Since, or Clear[Entries]AddedSince, or somesuch? Suppose could call it ClearEntriesAddedWithin
On 2016/07/29 14:08:48, mmenke wrote: > HttpAuthCache::Clear needs unit tests (It didn't have them before...but it also > looks like it wasn't being used before). Seems like BrowsingDataRemover should > have them, too. I added unit tests both to HttpAuthCache and BrowsingDataRemoved. There's one issue I've found while writing unit tests. Creation time is a property of HttpAuthCache::Entry. It means that if there's few days old entry and new path was added to this entry few minutes ago, it won't be removed when we clear entries added in the past 1 hour. I guess we need to store creation time for each path in the entry in order to handle this scenario properly. What do you think?
On 2016/08/01 14:26:25, Tomasz Moniuszko wrote: > On 2016/07/29 14:08:48, mmenke wrote: > > HttpAuthCache::Clear needs unit tests (It didn't have them before...but it > also > > looks like it wasn't being used before). Seems like BrowsingDataRemover > should > > have them, too. > > I added unit tests both to HttpAuthCache and BrowsingDataRemoved. > > There's one issue I've found while writing unit tests. Creation time is a > property of HttpAuthCache::Entry. It means that if there's few days old entry > and new path was added to this entry few minutes ago, it won't be removed when > we clear entries added in the past 1 hour. I guess we need to store creation > time for each path in the entry in order to handle this scenario properly. What > do you think? I'm not at all familiar with the class. I'll take a look so I can give an informed opinion, but may not get to it today (Bunch of reviews in my queue - sorry for the delay)
browsing_data/ still LGTM, thanks for adding the test. Regarding your question about HttpAuthCache::Entry, that's a good catch. The semantics that BrowsingDataRemover normally uses are "last modified timestamp", not "creation timestamp". This means that in your scenario it would be actually it would be also acceptable to delete the entire entry if a path was added within the last hour. That's of course a bit aggresive, but given that this is a privacy feature, it's acceptable. A better solution would be to keep timestamps with paths as you suggested. I'll leave it to mmenke@ to tell which change (last modification timestamp or per-path timestamps) is better from his perspective. But as for me, the CL can land as it is - deleting according to the creation timestamp is strictly better than what we currently have (i.e. nothing). You could add a TODO and improve it in a separate CL if you want to. https://codereview.chromium.org/2097043002/diff/100001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right): https://codereview.chromium.org/2097043002/diff/100001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_remover_unittest.cc:866: http_auth_cache_->Lookup(GURL("http://foo.com"), "Realm1", Nit: Can you please add the same lookup to the constructor? We want to assure that the lookup in this destructor fails because the entry was deleted and not because adding it failed.
Adding a path doesn't really seem like a user-visible modification, if I understand how it works correctly. A user enters a password for http://foo/bar, Realm1. We later see a challenge for http://foo/baz, Realm1, try the password again automatically (I think?), and it works. We update the path. None of that is user visible, so not sure that really counts as modification. However, if the user re-enters the password, we *should* consider that a change, and I think we currently use the same code path there (In terms of updating the HttpAuthCache) as in the cache-expansion case. I'm not sure if there are a significant number of cases where the user re-enters the same password, and it works. If not, we could just add a modified_time_, and update it on creation, and where the cache entry is updated with credentials that don't match the old ones. On 2016/08/01 14:42:38, msramek wrote: > browsing_data/ still LGTM, thanks for adding the test. > > Regarding your question about HttpAuthCache::Entry, that's a good catch. The > semantics that BrowsingDataRemover normally uses are "last modified timestamp", > not "creation timestamp". This means that in your scenario it would be actually > it would be also acceptable to delete the entire entry if a path was added > within the last hour. That's of course a bit aggresive, but given that this is a > privacy feature, it's acceptable. A better solution would be to keep timestamps > with paths as you suggested. > > I'll leave it to mmenke@ to tell which change (last modification timestamp or > per-path timestamps) is better from his perspective. > > But as for me, the CL can land as it is - deleting according to the creation > timestamp is strictly better than what we currently have (i.e. nothing). You > could add a TODO and improve it in a separate CL if you want to. > > https://codereview.chromium.org/2097043002/diff/100001/chrome/browser/browsin... > File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right): > > https://codereview.chromium.org/2097043002/diff/100001/chrome/browser/browsin... > chrome/browser/browsing_data/browsing_data_remover_unittest.cc:866: > http_auth_cache_->Lookup(GURL("http://foo.com"), "Realm1", > Nit: Can you please add the same lookup to the constructor? We want to assure > that the lookup in this destructor fails because the entry was deleted and not > because adding it failed.
On 2016/08/02 16:58:38, mmenke wrote: > Adding a path doesn't really seem like a user-visible modification, if I > understand how it works correctly. > > A user enters a password for http://foo/bar, Realm1. We later see a challenge > for http://foo/baz, Realm1, try the password again automatically (I think?), and > it works. We update the path. None of that is user visible, so not sure that > really counts as modification. > > However, if the user re-enters the password, we *should* consider that a change, > and I think we currently use the same code path there (In terms of updating the > HttpAuthCache) as in the cache-expansion case. I'm not sure if there are a > significant number of cases where the user re-enters the same password, and it > works. If not, we could just add a modified_time_, and update it on creation, > and where the cache entry is updated with credentials that don't match the old > ones. Same path as the path-expansion case, rather.
On 2016/08/02 16:58:38, mmenke wrote: > Adding a path doesn't really seem like a user-visible modification, if I > understand how it works correctly. > > A user enters a password for http://foo/bar, Realm1. We later see a challenge > for http://foo/baz, Realm1, try the password again automatically (I think?), and > it works. We update the path. None of that is user visible, so not sure that > really counts as modification. Yes, you are right. No UI is shown in this case. > > However, if the user re-enters the password, we *should* consider that a change, > and I think we currently use the same code path there (In terms of updating the > HttpAuthCache) as in the cache-expansion case. I'm not sure if there are a > significant number of cases where the user re-enters the same password, and it > works. If not, we could just add a modified_time_, and update it on creation, > and where the cache entry is updated with credentials that don't match the old > ones. I tested the case with different passwords set for two different directories on the server. HttpAuthCache::Entry is being removed from HttpAuthController::InvalidateRejectedAuthFromCache() when its AuthCredentials are rejected. New entry is then added after successful authentication. Additional modified_time_ is redundant then. I think current implementation works as expected: - No UI shown when path is added to existing entry. Entry isn't treated as modified for the purpose of clearing browsing data. - Entry is replaced with the new one when authentication UI is shown. New entry means new creation time for the purpose of clearing browsing data. I think net/ still needs lgtm from you. Do you think this CL is ready for landing?
https://codereview.chromium.org/2097043002/diff/120001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right): https://codereview.chromium.org/2097043002/diff/120001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_remover_unittest.cc:852: class ClearHttpAuthDataTester { Think this makes the tests needlessly obscure. Looking at the test bodies, it's not remotely clear that we check stat when this is torn down. Think it's better to either inline in the tests, or add two helper methods. https://codereview.chromium.org/2097043002/diff/120001/net/http/http_auth_cac... File net/http/http_auth_cache_unittest.cc (right): https://codereview.chromium.org/2097043002/diff/120001/net/http/http_auth_cac... net/http/http_auth_cache_unittest.cc:387: AuthCredentials(kRoot, kWileCoyote), "/"); I don't think we're guaranteed the time recorded here will be strictly less than start_time. So it's possible that ClearEntriesAddedWithin will delete these, too. https://codereview.chromium.org/2097043002/diff/120001/net/http/http_auth_cac... net/http/http_auth_cache_unittest.cc:399: cache.ClearEntriesAddedWithin(base::TimeTicks::Now() - start_time); I don't think we're guaranteed that no time will pass between when we call Now() here, and when the body of ClearEntriesAddedWithin() is invoked. So it's possible that ClearEntriesAddedWithin() won't remove anything. Could do something silly like spin the message loop enough cycles so those won't be problems, but then there's a question of how long is long enough, which varies by platform, and by whether or not this is an ASAN build.
https://codereview.chromium.org/2097043002/diff/120001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right): https://codereview.chromium.org/2097043002/diff/120001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_remover_unittest.cc:852: class ClearHttpAuthDataTester { On 2016/08/09 14:40:20, mmenke (busy) wrote: > Think this makes the tests needlessly obscure. Looking at the test bodies, it's > not remotely clear that we check stat when this is torn down. Think it's better > to either inline in the tests, or add two helper methods. Done. I inlined the code. https://codereview.chromium.org/2097043002/diff/120001/net/http/http_auth_cac... File net/http/http_auth_cache_unittest.cc (right): https://codereview.chromium.org/2097043002/diff/120001/net/http/http_auth_cac... net/http/http_auth_cache_unittest.cc:387: AuthCredentials(kRoot, kWileCoyote), "/"); On 2016/08/09 14:40:20, mmenke (busy) wrote: > I don't think we're guaranteed the time recorded here will be strictly less than > start_time. > > So it's possible that ClearEntriesAddedWithin will delete these, too. Done. I added EnsureTimeDifference() call. https://codereview.chromium.org/2097043002/diff/120001/net/http/http_auth_cac... net/http/http_auth_cache_unittest.cc:399: cache.ClearEntriesAddedWithin(base::TimeTicks::Now() - start_time); On 2016/08/09 14:40:20, mmenke (busy) wrote: > I don't think we're guaranteed that no time will pass between when we call Now() > here, and when the body of ClearEntriesAddedWithin() is invoked. > > So it's possible that ClearEntriesAddedWithin() won't remove anything. > > Could do something silly like spin the message loop enough cycles so those won't > be problems, but then there's a question of how long is long enough, which > varies by platform, and by whether or not this is an ASAN build. I added EnsureTimeDifference() function which ensures the difference between the two TimeTicks is non-zero.
https://codereview.chromium.org/2097043002/diff/140001/net/http/http_auth_cac... File net/http/http_auth_cache_unittest.cc (right): https://codereview.chromium.org/2097043002/diff/140001/net/http/http_auth_cac... net/http/http_auth_cache_unittest.cc:417: cache.ClearEntriesAddedWithin(base::TimeTicks::Now() - start_time); I still don't think this works - ClearEntriesAddedWithin calls base::TimeTicks::Now() again, which may be greater than the base::TimeTicks::Now() when it's called here, to calculate the Delta. I don't think we can reasonably expect to be able to ensure that a base::TimeTicks::Now() - delta when called by production code is between two times, without mocking out base::TimeTicks::Now().
On 2016/08/25 19:28:02, mmenke (busy) wrote: > https://codereview.chromium.org/2097043002/diff/140001/net/http/http_auth_cac... > File net/http/http_auth_cache_unittest.cc (right): > > https://codereview.chromium.org/2097043002/diff/140001/net/http/http_auth_cac... > net/http/http_auth_cache_unittest.cc:417: > cache.ClearEntriesAddedWithin(base::TimeTicks::Now() - start_time); > I still don't think this works - ClearEntriesAddedWithin calls > base::TimeTicks::Now() again, which may be greater than the > base::TimeTicks::Now() when it's called here, to calculate the Delta. I don't > think we can reasonably expect to be able to ensure that a > base::TimeTicks::Now() - delta when called by production code is between two > times, without mocking out base::TimeTicks::Now(). Maybe the solution would be to return to the implementation from Patch Set 4: https://codereview.chromium.org/2097043002/diff/60001/net/http/http_auth_cach... I can change Clear() parameter types to base::TimeTicks so HttpAuthCache::Entry and HttpAuthCache::Clear() uses the same time type. BrowsingDataRemover would be responsible for converting the type. What do you think?
On 2016/08/26 09:14:49, Tomasz Moniuszko wrote: > On 2016/08/25 19:28:02, mmenke (busy) wrote: > > > https://codereview.chromium.org/2097043002/diff/140001/net/http/http_auth_cac... > > File net/http/http_auth_cache_unittest.cc (right): > > > > > https://codereview.chromium.org/2097043002/diff/140001/net/http/http_auth_cac... > > net/http/http_auth_cache_unittest.cc:417: > > cache.ClearEntriesAddedWithin(base::TimeTicks::Now() - start_time); > > I still don't think this works - ClearEntriesAddedWithin calls > > base::TimeTicks::Now() again, which may be greater than the > > base::TimeTicks::Now() when it's called here, to calculate the Delta. I don't > > think we can reasonably expect to be able to ensure that a > > base::TimeTicks::Now() - delta when called by production code is between two > > times, without mocking out base::TimeTicks::Now(). > > Maybe the solution would be to return to the implementation from Patch Set 4: > https://codereview.chromium.org/2097043002/diff/60001/net/http/http_auth_cach... > I can change Clear() parameter types to base::TimeTicks so HttpAuthCache::Entry > and HttpAuthCache::Clear() uses the same time type. BrowsingDataRemover would be > responsible for converting the type. What do you think? That works.... Another option would be to split it into two tests. And yet another option would be to add en entry to the CookieStore with an insertion time in the past (Say, 2 minutes before the test runs), and then TimeDelta of 1 minute. Not sure if adding a cookie to the store updates its time or not.
On 2016/08/26 14:32:34, mmenke (busy) wrote: > On 2016/08/26 09:14:49, Tomasz Moniuszko wrote: > > On 2016/08/25 19:28:02, mmenke (busy) wrote: > > > > > > https://codereview.chromium.org/2097043002/diff/140001/net/http/http_auth_cac... > > > File net/http/http_auth_cache_unittest.cc (right): > > > > > > > > > https://codereview.chromium.org/2097043002/diff/140001/net/http/http_auth_cac... > > > net/http/http_auth_cache_unittest.cc:417: > > > cache.ClearEntriesAddedWithin(base::TimeTicks::Now() - start_time); > > > I still don't think this works - ClearEntriesAddedWithin calls > > > base::TimeTicks::Now() again, which may be greater than the > > > base::TimeTicks::Now() when it's called here, to calculate the Delta. I > don't > > > think we can reasonably expect to be able to ensure that a > > > base::TimeTicks::Now() - delta when called by production code is between two > > > times, without mocking out base::TimeTicks::Now(). > > > > Maybe the solution would be to return to the implementation from Patch Set 4: > > > https://codereview.chromium.org/2097043002/diff/60001/net/http/http_auth_cach... > > I can change Clear() parameter types to base::TimeTicks so > HttpAuthCache::Entry > > and HttpAuthCache::Clear() uses the same time type. BrowsingDataRemover would > be > > responsible for converting the type. What do you think? > > That works.... Another option would be to split it into two tests. And yet > another option would be to add en entry to the CookieStore with an insertion > time in the past (Say, 2 minutes before the test runs), and then TimeDelta of 1 > minute. Not sure if adding a cookie to the store updates its time or not. I'm fine with any of these solutions - I'm not trying to be an unreasonable perfectionist here, just don't want a flaky test.
On 2016/08/26 14:33:47, mmenke wrote: > I'm fine with any of these solutions - I'm not trying to be an unreasonable > perfectionist here, just don't want a flaky test. Sure, it's understandable. The test should be fine now. Thanks for the review!
LGTM! Thanks for fixing this! https://codereview.chromium.org/2097043002/diff/160001/net/http/http_auth_cac... File net/http/http_auth_cache.h (right): https://codereview.chromium.org/2097043002/diff/160001/net/http/http_auth_cac... net/http/http_auth_cache.h:67: FRIEND_TEST_ALL_PREFIXES(HttpAuthCacheTest, ClearEntriesAddedWithin); Suggest a public method set_creation_time_for_testing instead. We have presubmit checks about for_testing methods called in tests, and not-friending limits the API that tests can depend on to the public API + for_testing methods, as opposed all the guts of the class.
https://codereview.chromium.org/2097043002/diff/160001/net/http/http_auth_cac... File net/http/http_auth_cache.h (right): https://codereview.chromium.org/2097043002/diff/160001/net/http/http_auth_cac... net/http/http_auth_cache.h:67: FRIEND_TEST_ALL_PREFIXES(HttpAuthCacheTest, ClearEntriesAddedWithin); On 2016/08/31 14:48:32, mmenke wrote: > Suggest a public method set_creation_time_for_testing instead. We have > presubmit checks about for_testing methods called in tests, and not-friending > limits the API that tests can depend on to the public API + for_testing methods, > as opposed all the guts of the class. Done.
The CQ bit was checked by tmoniuszko@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2097043002/#ps180001 (title: "Replace FRIEND_TEST_ALL_PREFIXES with set_creation_time_for_testing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Clear HTTP auth data on clearing cache BUG=108291 ========== to ========== Clear HTTP auth data on clearing cache BUG=108291 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Clear HTTP auth data on clearing cache BUG=108291 ========== to ========== Clear HTTP auth data on clearing cache BUG=108291 Committed: https://crrev.com/053298c3828bff48de45264c8263af6e82d5c354 Cr-Commit-Position: refs/heads/master@{#415919} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/053298c3828bff48de45264c8263af6e82d5c354 Cr-Commit-Position: refs/heads/master@{#415919} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
