Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2598003002:
Suppress save and update bubbles when storing a PSL matched credential (Closed)
DescriptionSuppress save and update bubbles when storing a PSL matched credential
The "PSL credentials for YOLO" design doc suggests suppressing the UI prompt
when the user selects a PSL match in the account chooser. This CL implements
this feature and introduces relevant tests.
BUG=666340
R=vabr@chromium.org,vasilii@chromium.org
Committed: https://crrev.com/a1e08bf72eed1cf156d0c549e846c899237dee61
Cr-Commit-Position: refs/heads/master@{#440611}
Patch Set 1 #Patch Set 2 : Cleanup includes and update doc #
Total comments: 29
Patch Set 3 : Addressed comments. #
Dependent Patchsets: Messages
Total messages: 21 (12 generated)
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...
Dear all, please review :) https://codereview.chromium.org/2598003002/diff/20001/chrome/browser/password... File chrome/browser/password_manager/credential_manager_browsertest.cc (right): https://codereview.chromium.org/2598003002/diff/20001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:170: mock_cert_verifier()->AddResultForCert(cert.get(), verify_result, net::OK); Even though there is a "--ignore-certificate-errors" flag (https://codesearch.chromium.org/chromium/src/content/public/common/content_sw...), we need to setup a mock certificate for the CM API. The reason for this is a missing check for "--ignore-certificate-errors" in |ChromePasswordManagerClient::DidLastPageLoadEncounterSSLErrors| (https://codesearch.chromium.org/chromium/src/chrome/browser/password_manager/...) which in turn is called by |ChromePasswordManagerClient::IsFillingEnabledForCurrentPage| and |CredentialManagerImpl::Get|. I am not sure whether this missing check is a bug or a feature, which is why I would like to hear your opinion here.
LGTM, very nice. I only have minor comments. Cheers, Vaclav https://codereview.chromium.org/2598003002/diff/20001/chrome/browser/password... File chrome/browser/password_manager/credential_manager_browsertest.cc (right): https://codereview.chromium.org/2598003002/diff/20001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:75: const std::string& relative_url = "/") { I don't see where this is only called with 2 arguments. Could we drop the default value for the third one? https://codereview.chromium.org/2598003002/diff/20001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:95: std::unique_ptr<net::MockCertVerifier> mock_cert_verifier_; This is created in the constructor and never deleted. Could it be just aggregated without being put into a unique_ptr? https://codereview.chromium.org/2598003002/diff/20001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:157: const base::FilePath::CharType kDocRoot[] = Please make this both static and constexpr, see https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pF7nAmUQ2uw/-2PSZ... https://codereview.chromium.org/2598003002/diff/20001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:170: mock_cert_verifier()->AddResultForCert(cert.get(), verify_result, net::OK); On 2016/12/22 12:33:53, jdoerrie wrote: > Even though there is a "--ignore-certificate-errors" flag > (https://codesearch.chromium.org/chromium/src/content/public/common/content_sw...), > we need to setup a mock certificate for the CM API. The reason for this is a > missing check for "--ignore-certificate-errors" in > |ChromePasswordManagerClient::DidLastPageLoadEncounterSSLErrors| > (https://codesearch.chromium.org/chromium/src/chrome/browser/password_manager/...) > which in turn is called by > |ChromePasswordManagerClient::IsFillingEnabledForCurrentPage| and > |CredentialManagerImpl::Get|. > > I am not sure whether this missing check is a bug or a feature, which is why I > would like to hear your opinion here. It depends on the purpose of the --ignore-certificate-errors. If it is only meant for testing, then it might be fine to stay as it is. Whoever introduced this flag might have an answer to that. In general, I prefer what you did here anyway -- setting the certificates properly ensures that the codepaths seen in this test are more similar to what the users generate than if it uses shortcuts through internal flags. https://codereview.chromium.org/2598003002/diff/20001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:218: std::unique_ptr<BubbleObserver> prompt_observer( auto prompt_observer = base::MakeUnique<BubbleObserver>(WebContents()); avoids duplicating the type name.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2598003002/diff/20001/chrome/browser/password... File chrome/browser/password_manager/credential_manager_browsertest.cc (right): https://codereview.chromium.org/2598003002/diff/20001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:95: std::unique_ptr<net::MockCertVerifier> mock_cert_verifier_; Why not just net::MockCertVerifier> mock_cert_verifier_? https://codereview.chromium.org/2598003002/diff/20001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:170: mock_cert_verifier()->AddResultForCert(cert.get(), verify_result, net::OK); On 2016/12/22 12:33:53, jdoerrie wrote: > Even though there is a "--ignore-certificate-errors" flag > (https://codesearch.chromium.org/chromium/src/content/public/common/content_sw...), > we need to setup a mock certificate for the CM API. The reason for this is a > missing check for "--ignore-certificate-errors" in > |ChromePasswordManagerClient::DidLastPageLoadEncounterSSLErrors| > (https://codesearch.chromium.org/chromium/src/chrome/browser/password_manager/...) > which in turn is called by > |ChromePasswordManagerClient::IsFillingEnabledForCurrentPage| and > |CredentialManagerImpl::Get|. > > I am not sure whether this missing check is a bug or a feature, which is why I > would like to hear your opinion here. Leave it as it is. I wonder how NoPromptForLoginFailedAndServerPushSeperateLoginForm_HttpsToHttp passes. Seems like the test is just broken. https://codereview.chromium.org/2598003002/diff/20001/components/password_man... File components/password_manager/content/browser/credential_manager_impl.cc (right): https://codereview.chromium.org/2598003002/diff/20001/components/password_man... components/password_manager/content/browser/credential_manager_impl.cc:89: if (form.is_public_suffix_match) { Use IsPendingCredentialsPublicSuffixMatch() for consistency with PasswordManager::ShouldPromptUserToSavePassword() https://codereview.chromium.org/2598003002/diff/20001/components/password_man... components/password_manager/content/browser/credential_manager_impl.cc:91: // an exactly matching origin. This is because the presence of the exact exactly matching origin and username. https://codereview.chromium.org/2598003002/diff/20001/components/password_man... components/password_manager/content/browser/credential_manager_impl.cc:92: // match would stop the PSL matches from appearing in the account chooser. The explanation is wrong. A site doesn't have to use the account chooser. 'store()' may be the only method called. https://codereview.chromium.org/2598003002/diff/20001/components/password_man... File components/password_manager/content/browser/credential_manager_impl_unittest.cc (right): https://codereview.chromium.org/2598003002/diff/20001/components/password_man... components/password_manager/content/browser/credential_manager_impl_unittest.cc:545: } Check that the credential was actually stored and not dropped. https://codereview.chromium.org/2598003002/diff/20001/components/password_man... components/password_manager/content/browser/credential_manager_impl_unittest.cc:548: CredentialManagerStoreNonPSLMatchWithDifferentUsernameTriggersBubble) { should it be CredentialManagerStorePSLMatchWithDifferentUsernameTriggersBubble https://codereview.chromium.org/2598003002/diff/20001/components/password_man... components/password_manager/content/browser/credential_manager_impl_unittest.cc:550: autofill::PasswordForm in_valid_psl_form = subdomain_form_; What is 'in_'? https://codereview.chromium.org/2598003002/diff/20001/components/password_man... components/password_manager/content/browser/credential_manager_impl_unittest.cc:565: // the form is not a (PSL) match for an existing form, and update the What kind of 'update' are you expecting? i think it's a typo. https://codereview.chromium.org/2598003002/diff/20001/components/password_man... components/password_manager/content/browser/credential_manager_impl_unittest.cc:569: EXPECT_TRUE(called); Check that client_->pending_manager()->pending_credentials() has a decent content.
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...
Thanks for your feedback, could you have another look? https://codereview.chromium.org/2598003002/diff/20001/chrome/browser/password... File chrome/browser/password_manager/credential_manager_browsertest.cc (right): https://codereview.chromium.org/2598003002/diff/20001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:75: const std::string& relative_url = "/") { On 2016/12/22 13:08:24, vabr (Chromium) wrote: > I don't see where this is only called with 2 arguments. Could we drop the > default value for the third one? Done. https://codereview.chromium.org/2598003002/diff/20001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:95: std::unique_ptr<net::MockCertVerifier> mock_cert_verifier_; On 2016/12/22 13:24:58, vasilii wrote: > Why not just net::MockCertVerifier> mock_cert_verifier_? Done. https://codereview.chromium.org/2598003002/diff/20001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:157: const base::FilePath::CharType kDocRoot[] = On 2016/12/22 13:08:24, vabr (Chromium) wrote: > Please make this both static and constexpr, see > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pF7nAmUQ2uw/-2PSZ... Done. https://codereview.chromium.org/2598003002/diff/20001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:170: mock_cert_verifier()->AddResultForCert(cert.get(), verify_result, net::OK); On 2016/12/22 13:24:58, vasilii wrote: > On 2016/12/22 12:33:53, jdoerrie wrote: > > Even though there is a "--ignore-certificate-errors" flag > > > (https://codesearch.chromium.org/chromium/src/content/public/common/content_sw...), > > we need to setup a mock certificate for the CM API. The reason for this is a > > missing check for "--ignore-certificate-errors" in > > |ChromePasswordManagerClient::DidLastPageLoadEncounterSSLErrors| > > > (https://codesearch.chromium.org/chromium/src/chrome/browser/password_manager/...) > > which in turn is called by > > |ChromePasswordManagerClient::IsFillingEnabledForCurrentPage| and > > |CredentialManagerImpl::Get|. > > > > I am not sure whether this missing check is a bug or a feature, which is why I > > would like to hear your opinion here. > > Leave it as it is. > > I wonder how NoPromptForLoginFailedAndServerPushSeperateLoginForm_HttpsToHttp > passes. Seems like the test is just broken. Acknowledged. https://codereview.chromium.org/2598003002/diff/20001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:218: std::unique_ptr<BubbleObserver> prompt_observer( On 2016/12/22 13:08:24, vabr (Chromium) wrote: > auto prompt_observer = base::MakeUnique<BubbleObserver>(WebContents()); > avoids duplicating the type name. Acknowledged. As discussed offline I prefer MakeUnique as well, but in this case I will use the raw new for consistency with other tests in this file. I will follow up with a cleanup CL soon. https://codereview.chromium.org/2598003002/diff/20001/components/password_man... File components/password_manager/content/browser/credential_manager_impl.cc (right): https://codereview.chromium.org/2598003002/diff/20001/components/password_man... components/password_manager/content/browser/credential_manager_impl.cc:89: if (form.is_public_suffix_match) { On 2016/12/22 13:24:59, vasilii wrote: > Use IsPendingCredentialsPublicSuffixMatch() for consistency with > PasswordManager::ShouldPromptUserToSavePassword() Done. https://codereview.chromium.org/2598003002/diff/20001/components/password_man... components/password_manager/content/browser/credential_manager_impl.cc:91: // an exactly matching origin. This is because the presence of the exact On 2016/12/22 13:24:58, vasilii wrote: > exactly matching origin and username. Done. https://codereview.chromium.org/2598003002/diff/20001/components/password_man... components/password_manager/content/browser/credential_manager_impl.cc:92: // match would stop the PSL matches from appearing in the account chooser. On 2016/12/22 13:24:59, vasilii wrote: > The explanation is wrong. A site doesn't have to use the account chooser. > 'store()' may be the only method called. Acknowledged. https://codereview.chromium.org/2598003002/diff/20001/components/password_man... File components/password_manager/content/browser/credential_manager_impl_unittest.cc (right): https://codereview.chromium.org/2598003002/diff/20001/components/password_man... components/password_manager/content/browser/credential_manager_impl_unittest.cc:545: } On 2016/12/22 13:24:59, vasilii wrote: > Check that the credential was actually stored and not dropped. Done. https://codereview.chromium.org/2598003002/diff/20001/components/password_man... components/password_manager/content/browser/credential_manager_impl_unittest.cc:548: CredentialManagerStoreNonPSLMatchWithDifferentUsernameTriggersBubble) { On 2016/12/22 13:24:59, vasilii wrote: > should it be CredentialManagerStorePSLMatchWithDifferentUsernameTriggersBubble Done. https://codereview.chromium.org/2598003002/diff/20001/components/password_man... components/password_manager/content/browser/credential_manager_impl_unittest.cc:550: autofill::PasswordForm in_valid_psl_form = subdomain_form_; On 2016/12/22 13:24:59, vasilii wrote: > What is 'in_'? Dropped it, meant to say invalid, but the PSL match in itself is correct. https://codereview.chromium.org/2598003002/diff/20001/components/password_man... components/password_manager/content/browser/credential_manager_impl_unittest.cc:565: // the form is not a (PSL) match for an existing form, and update the On 2016/12/22 13:24:59, vasilii wrote: > What kind of 'update' are you expecting? i think it's a typo. Done. https://codereview.chromium.org/2598003002/diff/20001/components/password_man... components/password_manager/content/browser/credential_manager_impl_unittest.cc:569: EXPECT_TRUE(called); On 2016/12/22 13:24:59, vasilii wrote: > Check that client_->pending_manager()->pending_credentials() has a decent > content. Done.
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm Ausgezeichnet!
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": 40001, "attempt_start_ts": 1482487546441770,
"parent_rev": "739cab52b2a44ac9fb1663ad2e2df9f8290b8c6d", "commit_rev":
"691100897e3f8f9812cea68aecd349e12eabebf3"}
Message was sent while issue was closed.
Description was changed from ========== Suppress save and update bubbles when storing a PSL matched credential The "PSL credentials for YOLO" design doc suggests suppressing the UI prompt when the user selects a PSL match in the account chooser. This CL implements this feature and introduces relevant tests. BUG=666340 R=vabr@chromium.org,vasilii@chromium.org ========== to ========== Suppress save and update bubbles when storing a PSL matched credential The "PSL credentials for YOLO" design doc suggests suppressing the UI prompt when the user selects a PSL match in the account chooser. This CL implements this feature and introduces relevant tests. BUG=666340 R=vabr@chromium.org,vasilii@chromium.org Review-Url: https://codereview.chromium.org/2598003002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Suppress save and update bubbles when storing a PSL matched credential The "PSL credentials for YOLO" design doc suggests suppressing the UI prompt when the user selects a PSL match in the account chooser. This CL implements this feature and introduces relevant tests. BUG=666340 R=vabr@chromium.org,vasilii@chromium.org Review-Url: https://codereview.chromium.org/2598003002 ========== to ========== Suppress save and update bubbles when storing a PSL matched credential The "PSL credentials for YOLO" design doc suggests suppressing the UI prompt when the user selects a PSL match in the account chooser. This CL implements this feature and introduces relevant tests. BUG=666340 R=vabr@chromium.org,vasilii@chromium.org Committed: https://crrev.com/a1e08bf72eed1cf156d0c549e846c899237dee61 Cr-Commit-Position: refs/heads/master@{#440611} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a1e08bf72eed1cf156d0c549e846c899237dee61 Cr-Commit-Position: refs/heads/master@{#440611} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
