Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2673053002:
Obsolete HTTP Cleaner (Closed)
DescriptionIntroduce Obsolete HTTP Cleaner
This change introduces a password store consumer that will delete obsolete
credentials, blacklisted hosts and site stats for sites that switched to HTTPS
and have HSTS enabled.
BUG=687968
R=vasilii@chromium.org
Review-Url: https://codereview.chromium.org/2673053002
Cr-Commit-Position: refs/heads/master@{#448967}
Committed: https://chromium.googlesource.com/chromium/src/+/8302e38b44617b92c635a8d0d8b80d9eb7c2b153
Patch Set 1 #
Total comments: 10
Patch Set 2 : Fix Tests and remove mock of GetSiteStats #
Total comments: 24
Patch Set 3 : Addressed comments. #Patch Set 4 : Fixed typo. #
Total comments: 16
Patch Set 5 : Addressed comments. #Patch Set 6 : Drop PKP Check #Messages
Total messages: 30 (20 generated)
The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run
Hi Vasilii, please review :) https://codereview.chromium.org/2673053002/diff/1/chrome/browser/password_man... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/2673053002/diff/1/chrome/browser/password_man... chrome/browser/password_manager/chrome_password_manager_client.cc:146: contents, autofill_client)); I'm just seeing these now, most likely they are caused by |git cl format|. Is it fine to keep them or should I remove these changes from the CL? https://codereview.chromium.org/2673053002/diff/1/chrome/browser/password_man... chrome/browser/password_manager/chrome_password_manager_client.cc:227: bool ChromePasswordManagerClient::IsHSTSActiveForOrigin( I'm not quite sure if naming is correct here. Maybe it should be |IsHSTSActiveForHost| instead? https://codereview.chromium.org/2673053002/diff/1/chrome/browser/password_man... chrome/browser/password_manager/chrome_password_manager_client.cc:245: security_state->GetDynamicPKPState(host, &pkp_state); This check is very similar to what is done in |TransportSecurityState::ShouldSSLErrorsBeFatal| (https://codesearch.chromium.org/chromium/src/net/http/transport_security_stat...) and |NetInternalsMessageHandler::IOThreadImpl::OnHSTSQuery| (https://codesearch.chromium.org/chromium/src/chrome/browser/ui/webui/net_inte...). Thus it leads to some code duplication, but I feel it is good to have this logic directly in PasswordManagerClient as well. Thoughts? https://codereview.chromium.org/2673053002/diff/1/components/password_manager... File components/password_manager/core/browser/mock_password_store.h (right): https://codereview.chromium.org/2673053002/diff/1/components/password_manager... components/password_manager/core/browser/mock_password_store.h:31: MOCK_METHOD1(RemoveSiteStats, void(const GURL&)); I failed to get the |RemoveSiteStatsImpl| mock to work, that's why I introduced this and made it virtual in password_store.h. I feel like there should be a better way, is there?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2673053002/diff/1/components/password_manager... File components/password_manager/core/browser/mock_password_store.h (right): https://codereview.chromium.org/2673053002/diff/1/components/password_manager... components/password_manager/core/browser/mock_password_store.h:31: MOCK_METHOD1(RemoveSiteStats, void(const GURL&)); On 2017/02/03 16:49:15, jdoerrie wrote: > I failed to get the |RemoveSiteStatsImpl| mock to work, that's why I introduced > this and made it virtual in password_store.h. I feel like there should be a > better way, is there? Figured it out, I had to use |base::RunLoop().RunUntilIdle()|
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2673053002/diff/1/chrome/browser/password_man... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/2673053002/diff/1/chrome/browser/password_man... chrome/browser/password_manager/chrome_password_manager_client.cc:146: contents, autofill_client)); On 2017/02/03 16:49:15, jdoerrie wrote: > I'm just seeing these now, most likely they are caused by |git cl format|. Is it > fine to keep them or should I remove these changes from the CL? Better remove. The previous formatting is nicer. https://codereview.chromium.org/2673053002/diff/1/chrome/browser/password_man... chrome/browser/password_manager/chrome_password_manager_client.cc:227: bool ChromePasswordManagerClient::IsHSTSActiveForOrigin( On 2017/02/03 16:49:15, jdoerrie wrote: > I'm not quite sure if naming is correct here. Maybe it should be > |IsHSTSActiveForHost| instead? Agree with host. https://codereview.chromium.org/2673053002/diff/1/chrome/browser/password_man... chrome/browser/password_manager/chrome_password_manager_client.cc:245: security_state->GetDynamicPKPState(host, &pkp_state); On 2017/02/03 16:49:15, jdoerrie wrote: > This check is very similar to what is done in > |TransportSecurityState::ShouldSSLErrorsBeFatal| > (https://codesearch.chromium.org/chromium/src/net/http/transport_security_stat...) > and |NetInternalsMessageHandler::IOThreadImpl::OnHSTSQuery| > (https://codesearch.chromium.org/chromium/src/chrome/browser/ui/webui/net_inte...). > Thus it leads to some code duplication, but I feel it is good to have this logic > directly in PasswordManagerClient as well. Thoughts? The code should be here. How is HPKP relevant here? https://codereview.chromium.org/2673053002/diff/20001/components/password_man... File components/password_manager/core/browser/hsts_deleter.cc (right): https://codereview.chromium.org/2673053002/diff/20001/components/password_man... components/password_manager/core/browser/hsts_deleter.cc:56: if (client_->IsHSTSActiveForOrigin(blacklisted_form->origin)) Does HSTS request for HTTP also work? https://codereview.chromium.org/2673053002/diff/20001/components/password_man... components/password_manager/core/browser/hsts_deleter.cc:58: }); Why can't you process and remove the blacklisted forms in one pass? https://codereview.chromium.org/2673053002/diff/20001/components/password_man... components/password_manager/core/browser/hsts_deleter.cc:65: // that we don't invalidate |begin_https| in case there are no HSTS forms. erase() doesn't ivalidate the iterators before |first| https://codereview.chromium.org/2673053002/diff/20001/components/password_man... components/password_manager/core/browser/hsts_deleter.cc:97: client_->IsHSTSActiveForOrigin(stat.origin_domain)) { How can HSTS be active for an HTTP origin? https://codereview.chromium.org/2673053002/diff/20001/components/password_man... File components/password_manager/core/browser/hsts_deleter.h (right): https://codereview.chromium.org/2673053002/diff/20001/components/password_man... components/password_manager/core/browser/hsts_deleter.h:24: class HstsDeleter : public PasswordStoreConsumer { A comment? The name isn't clear at all. The class definitely doesn't clear HSTS. So far the lifetime of the object is unclear to me. It can be a follow-up CL. https://codereview.chromium.org/2673053002/diff/20001/components/password_man... components/password_manager/core/browser/hsts_deleter.h:27: PasswordStore* password_store); You can get the password store from the client with GetPasswordStore() https://codereview.chromium.org/2673053002/diff/20001/components/password_man... components/password_manager/core/browser/hsts_deleter.h:28: A virtual destructor? https://codereview.chromium.org/2673053002/diff/20001/components/password_man... File components/password_manager/core/browser/hsts_deleter_unittest.cc (right): https://codereview.chromium.org/2673053002/diff/20001/components/password_man... components/password_manager/core/browser/hsts_deleter_unittest.cc:96: bool is_blacklisted; What exactly are you testing when |is_blacklisted| is false? (I'm not saying that it should be removed, just a question) https://codereview.chromium.org/2673053002/diff/20001/components/password_man... components/password_manager/core/browser/hsts_deleter_unittest.cc:109: for (const auto test_case : cases) { const auto& and below everywhere https://codereview.chromium.org/2673053002/diff/20001/components/password_man... components/password_manager/core/browser/hsts_deleter_unittest.cc:114: EXPECT_CALL(client(), IsHSTSActiveForOrigin(form.origin)) braces. https://codereview.chromium.org/2673053002/diff/20001/components/password_man... components/password_manager/core/browser/hsts_deleter_unittest.cc:186: EXPECT_CALL(client(), IsHSTSActiveForOrigin(stats.origin_domain)) braces https://codereview.chromium.org/2673053002/diff/20001/components/password_man... File components/password_manager/core/browser/password_manager_client.cc (right): https://codereview.chromium.org/2673053002/diff/20001/components/password_man... components/password_manager/core/browser/password_manager_client.cc:23: return false; What's on iOS? Separate CL?
The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Vasilii, please have another look :) https://codereview.chromium.org/2673053002/diff/1/chrome/browser/password_man... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/2673053002/diff/1/chrome/browser/password_man... chrome/browser/password_manager/chrome_password_manager_client.cc:227: bool ChromePasswordManagerClient::IsHSTSActiveForOrigin( On 2017/02/06 16:43:24, vasilii wrote: > On 2017/02/03 16:49:15, jdoerrie wrote: > > I'm not quite sure if naming is correct here. Maybe it should be > > |IsHSTSActiveForHost| instead? > > Agree with host. Done. https://codereview.chromium.org/2673053002/diff/1/chrome/browser/password_man... chrome/browser/password_manager/chrome_password_manager_client.cc:245: security_state->GetDynamicPKPState(host, &pkp_state); On 2017/02/06 16:43:24, vasilii wrote: > On 2017/02/03 16:49:15, jdoerrie wrote: > > This check is very similar to what is done in > > |TransportSecurityState::ShouldSSLErrorsBeFatal| > > > (https://codesearch.chromium.org/chromium/src/net/http/transport_security_stat...) > > and |NetInternalsMessageHandler::IOThreadImpl::OnHSTSQuery| > > > (https://codesearch.chromium.org/chromium/src/chrome/browser/ui/webui/net_inte...). > > Thus it leads to some code duplication, but I feel it is good to have this > logic > > directly in PasswordManagerClient as well. Thoughts? > > The code should be here. How is HPKP relevant here? Not sure, I reached out to security folks regarding this. https://codereview.chromium.org/2673053002/diff/20001/components/password_man... File components/password_manager/core/browser/hsts_deleter.cc (right): https://codereview.chromium.org/2673053002/diff/20001/components/password_man... components/password_manager/core/browser/hsts_deleter.cc:56: if (client_->IsHSTSActiveForOrigin(blacklisted_form->origin)) On 2017/02/06 16:43:24, vasilii wrote: > Does HSTS request for HTTP also work? Yes, it does. For HSTS queries only the host matters, the scheme is ignored. https://codereview.chromium.org/2673053002/diff/20001/components/password_man... components/password_manager/core/browser/hsts_deleter.cc:58: }); On 2017/02/06 16:43:24, vasilii wrote: > Why can't you process and remove the blacklisted forms in one pass? I could have done that, but then I also need to check for the blacklist flag when iterating over the container again in line 87. But this is also a moot point now. https://codereview.chromium.org/2673053002/diff/20001/components/password_man... components/password_manager/core/browser/hsts_deleter.cc:65: // that we don't invalidate |begin_https| in case there are no HSTS forms. On 2017/02/06 16:43:24, vasilii wrote: > erase() doesn't ivalidate the iterators before |first| As discussed offline this actually does leed to invalidation sometimes. But this is a moot point now, I changed it in the new patch set. https://codereview.chromium.org/2673053002/diff/20001/components/password_man... components/password_manager/core/browser/hsts_deleter.cc:97: client_->IsHSTSActiveForOrigin(stat.origin_domain)) { On 2017/02/06 16:43:24, vasilii wrote: > How can HSTS be active for an HTTP origin? See above (scheme is ignored, only host matters). https://codereview.chromium.org/2673053002/diff/20001/components/password_man... File components/password_manager/core/browser/hsts_deleter.h (right): https://codereview.chromium.org/2673053002/diff/20001/components/password_man... components/password_manager/core/browser/hsts_deleter.h:24: class HstsDeleter : public PasswordStoreConsumer { On 2017/02/06 16:43:24, vasilii wrote: > A comment? The name isn't clear at all. The class definitely doesn't clear HSTS. > > So far the lifetime of the object is unclear to me. It can be a follow-up CL. Done. https://codereview.chromium.org/2673053002/diff/20001/components/password_man... components/password_manager/core/browser/hsts_deleter.h:27: PasswordStore* password_store); On 2017/02/06 16:43:24, vasilii wrote: > You can get the password store from the client with GetPasswordStore() Done. https://codereview.chromium.org/2673053002/diff/20001/components/password_man... components/password_manager/core/browser/hsts_deleter.h:28: On 2017/02/06 16:43:24, vasilii wrote: > A virtual destructor? Done. https://codereview.chromium.org/2673053002/diff/20001/components/password_man... File components/password_manager/core/browser/hsts_deleter_unittest.cc (right): https://codereview.chromium.org/2673053002/diff/20001/components/password_man... components/password_manager/core/browser/hsts_deleter_unittest.cc:96: bool is_blacklisted; On 2017/02/06 16:43:24, vasilii wrote: > What exactly are you testing when |is_blacklisted| is false? (I'm not saying > that it should be removed, just a question) As explained offline, I wanted to test that non blacklisted forms are not removed in this test. https://codereview.chromium.org/2673053002/diff/20001/components/password_man... components/password_manager/core/browser/hsts_deleter_unittest.cc:109: for (const auto test_case : cases) { On 2017/02/06 16:43:24, vasilii wrote: > const auto& and below everywhere Done. https://codereview.chromium.org/2673053002/diff/20001/components/password_man... components/password_manager/core/browser/hsts_deleter_unittest.cc:114: EXPECT_CALL(client(), IsHSTSActiveForOrigin(form.origin)) On 2017/02/06 16:43:24, vasilii wrote: > braces. Done. https://codereview.chromium.org/2673053002/diff/20001/components/password_man... components/password_manager/core/browser/hsts_deleter_unittest.cc:186: EXPECT_CALL(client(), IsHSTSActiveForOrigin(stats.origin_domain)) On 2017/02/06 16:43:24, vasilii wrote: > braces Done. https://codereview.chromium.org/2673053002/diff/20001/components/password_man... File components/password_manager/core/browser/password_manager_client.cc (right): https://codereview.chromium.org/2673053002/diff/20001/components/password_man... components/password_manager/core/browser/password_manager_client.cc:23: return false; On 2017/02/06 16:43:24, vasilii wrote: > What's on iOS? Separate CL? Yep.
Description was changed from ========== Introduce HSTS Deleter This change introduces a password store consumer that will delete obsolete credentials, blacklisted hosts and site stats for sites that switched to HTTPS and have HSTS enabled. BUG=687968 R=vasilii@chromium.org ========== to ========== Introduce Obsolete HTTP Cleaner This change introduces a password store consumer that will delete obsolete credentials, blacklisted hosts and site stats for sites that switched to HTTPS and have HSTS enabled. BUG=687968 R=vasilii@chromium.org ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The only pending question is about PKP https://codereview.chromium.org/2673053002/diff/60001/components/password_man... File components/password_manager/core/browser/obsolete_http_cleaner.cc (right): https://codereview.chromium.org/2673053002/diff/60001/components/password_man... components/password_manager/core/browser/obsolete_http_cleaner.cc:26: std::vector<std::unique_ptr<PasswordForm>>::iterator from) { As this is an input parameter, it should be first. https://codereview.chromium.org/2673053002/diff/60001/components/password_man... File components/password_manager/core/browser/obsolete_http_cleaner.h (right): https://codereview.chromium.org/2673053002/diff/60001/components/password_man... components/password_manager/core/browser/obsolete_http_cleaner.h:20: struct InteractionsStats; I'm sure you don't need it :) https://codereview.chromium.org/2673053002/diff/60001/components/password_man... components/password_manager/core/browser/obsolete_http_cleaner.h:24: // obsolete, if the corresponding most migrated to HTTPS and has HSTS enabled. corresponding host https://codereview.chromium.org/2673053002/diff/60001/components/password_man... components/password_manager/core/browser/obsolete_http_cleaner.h:27: ObsoleteHttpCleaner(const PasswordManagerClient* client); explicit https://codereview.chromium.org/2673053002/diff/60001/components/password_man... File components/password_manager/core/browser/obsolete_http_cleaner_unittest.cc (right): https://codereview.chromium.org/2673053002/diff/60001/components/password_man... components/password_manager/core/browser/obsolete_http_cleaner_unittest.cc:69: } Blank line https://codereview.chromium.org/2673053002/diff/60001/components/password_man... components/password_manager/core/browser/obsolete_http_cleaner_unittest.cc:72: MockPasswordManagerClient(PasswordStore* store) : store_(store) {} explicit. https://codereview.chromium.org/2673053002/diff/60001/components/password_man... components/password_manager/core/browser/obsolete_http_cleaner_unittest.cc:78: PasswordStore* store_ = nullptr; nullptr is confusing because the only constructor inits it. https://codereview.chromium.org/2673053002/diff/60001/components/password_man... components/password_manager/core/browser/obsolete_http_cleaner_unittest.cc:119: for (const auto& test_case : cases) { Optionally: here and below SCOPED_TRACE
Another round. https://codereview.chromium.org/2673053002/diff/60001/components/password_man... File components/password_manager/core/browser/obsolete_http_cleaner.cc (right): https://codereview.chromium.org/2673053002/diff/60001/components/password_man... components/password_manager/core/browser/obsolete_http_cleaner.cc:26: std::vector<std::unique_ptr<PasswordForm>>::iterator from) { On 2017/02/07 15:31:26, vasilii wrote: > As this is an input parameter, it should be first. Done. https://codereview.chromium.org/2673053002/diff/60001/components/password_man... File components/password_manager/core/browser/obsolete_http_cleaner.h (right): https://codereview.chromium.org/2673053002/diff/60001/components/password_man... components/password_manager/core/browser/obsolete_http_cleaner.h:20: struct InteractionsStats; On 2017/02/07 15:31:26, vasilii wrote: > I'm sure you don't need it :) Done. https://codereview.chromium.org/2673053002/diff/60001/components/password_man... components/password_manager/core/browser/obsolete_http_cleaner.h:24: // obsolete, if the corresponding most migrated to HTTPS and has HSTS enabled. On 2017/02/07 15:31:26, vasilii wrote: > corresponding host Done. https://codereview.chromium.org/2673053002/diff/60001/components/password_man... components/password_manager/core/browser/obsolete_http_cleaner.h:27: ObsoleteHttpCleaner(const PasswordManagerClient* client); On 2017/02/07 15:31:26, vasilii wrote: > explicit Done. https://codereview.chromium.org/2673053002/diff/60001/components/password_man... File components/password_manager/core/browser/obsolete_http_cleaner_unittest.cc (right): https://codereview.chromium.org/2673053002/diff/60001/components/password_man... components/password_manager/core/browser/obsolete_http_cleaner_unittest.cc:69: } On 2017/02/07 15:31:26, vasilii wrote: > Blank line Done. https://codereview.chromium.org/2673053002/diff/60001/components/password_man... components/password_manager/core/browser/obsolete_http_cleaner_unittest.cc:72: MockPasswordManagerClient(PasswordStore* store) : store_(store) {} On 2017/02/07 15:31:26, vasilii wrote: > explicit. Done. https://codereview.chromium.org/2673053002/diff/60001/components/password_man... components/password_manager/core/browser/obsolete_http_cleaner_unittest.cc:78: PasswordStore* store_ = nullptr; On 2017/02/07 15:31:26, vasilii wrote: > nullptr is confusing because the only constructor inits it. Done. https://codereview.chromium.org/2673053002/diff/60001/components/password_man... components/password_manager/core/browser/obsolete_http_cleaner_unittest.cc:119: for (const auto& test_case : cases) { On 2017/02/07 15:31:26, vasilii wrote: > Optionally: here and below SCOPED_TRACE Done.
The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Last round?
lgtm
The CQ bit was checked by jdoerrie@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486552358786280,
"parent_rev": "983f3e3990d1048b85275d7b455ca6ca78bf5e23", "commit_rev":
"8302e38b44617b92c635a8d0d8b80d9eb7c2b153"}
Message was sent while issue was closed.
Description was changed from ========== Introduce Obsolete HTTP Cleaner This change introduces a password store consumer that will delete obsolete credentials, blacklisted hosts and site stats for sites that switched to HTTPS and have HSTS enabled. BUG=687968 R=vasilii@chromium.org ========== to ========== Introduce Obsolete HTTP Cleaner This change introduces a password store consumer that will delete obsolete credentials, blacklisted hosts and site stats for sites that switched to HTTPS and have HSTS enabled. BUG=687968 R=vasilii@chromium.org Review-Url: https://codereview.chromium.org/2673053002 Cr-Commit-Position: refs/heads/master@{#448967} Committed: https://chromium.googlesource.com/chromium/src/+/8302e38b44617b92c635a8d0d8b8... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/8302e38b44617b92c635a8d0d8b8... |
