New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 871140 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Last visit > 30 days ago
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 3
Type: Bug



Sign in to add a comment

Measure and Remove obsolete HTTP credentials

Project Member Reported by gemene@google.com, Aug 6

Issue description

This issue serves as a tracking bug for both measuring the occurrences and removal of obsolete HTTP credentials in the password store.
 
 
Cc: vabr@chromium.org jdoerrie@chromium.org vasi...@chromium.org
Components: UI>Browser>Passwords
Labels: OS-Chrome OS-Linux OS-Mac OS-Windows
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 16

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/23375b76c9d1a4a77e10a896b79af95c6904dd1b

commit 23375b76c9d1a4a77e10a896b79af95c6904dd1b
Author: Gemene Narcis <gemene@google.com>
Date: Thu Aug 16 15:36:17 2018

Recording metrics about HTTP migration process.

This CL adds six histograms. Three histograms are for HTTP credentials with HSTS
enabled and the other three are for HTTP credentials without HSTS enabled, and they
are supposed to count the next cases:
1) Number of HTTP credentials for which no HTTPS credential for the same username exists.
2) Number of HTTP credentials for which an equivalent (i.e. same host, username
   and password) HTTPS credential exists.
3) Number of HTTP credentials for which a conflicting (i.e. same host and
   username, but different password) HTTPS credential exists.

All six are recorded once for the profile on startup with a delay of 60 seconds.

Bug: 871140
Change-Id: I231ad82ab1e5d74520cd4c9be4e3cad62645c3d1
Reviewed-on: https://chromium-review.googlesource.com/1163243
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Brian White <bcwhite@chromium.org>
Commit-Queue: Narcis Gemene <gemene@google.com>
Cr-Commit-Position: refs/heads/master@{#583662}
[modify] https://crrev.com/23375b76c9d1a4a77e10a896b79af95c6904dd1b/chrome/browser/password_manager/password_store_factory.cc
[modify] https://crrev.com/23375b76c9d1a4a77e10a896b79af95c6904dd1b/components/password_manager/core/browser/password_manager_util.cc
[modify] https://crrev.com/23375b76c9d1a4a77e10a896b79af95c6904dd1b/components/password_manager/core/browser/password_manager_util.h
[modify] https://crrev.com/23375b76c9d1a4a77e10a896b79af95c6904dd1b/components/password_manager/core/browser/password_manager_util_unittest.cc
[modify] https://crrev.com/23375b76c9d1a4a77e10a896b79af95c6904dd1b/tools/metrics/histograms/histograms.xml

Project Member

Comment 4 by bugdroid1@chromium.org, Aug 30

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/da5278ff312bc30c50a3cb2f1300d45f03d13548

commit da5278ff312bc30c50a3cb2f1300d45f03d13548
Author: Gemene Narcis <gemene@google.com>
Date: Thu Aug 30 17:26:02 2018

Fix recording metrics about HTTP migration process

This CL fix the way that of considering that HTTP credentials have
no match, equivalent and conflicting HTTPS credentials added in
https://crrev.com/c/1163243/.

Starting with this CL we consider HTTP credential as no match,
HTTP credentials for which no HTTPS credential for the same
signon-realm (excluding the protocol), PasswordForm::Scheme
(i.e. HTML, BASIC etc.) and username exists in Password Store.

We consider HTTP credentials as having equivalent HTTPS credentials
if they have same signon-realm (excluding the protocol),
PasswordForm::Scheme, username and password.

We consider HTTP credentials as having conflicting HTTPS credentials
if they have same signon-realm (excluding the protocol),
PasswordForm::Scheme and username but different password.

Bug: 871140
Change-Id: If28c60f0a42f288c1f20a61f35c848978da39b49
Reviewed-on: https://chromium-review.googlesource.com/1196342
Commit-Queue: Narcis Gemene <gemene@google.com>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587640}
[modify] https://crrev.com/da5278ff312bc30c50a3cb2f1300d45f03d13548/components/password_manager/core/browser/password_manager_util.cc
[modify] https://crrev.com/da5278ff312bc30c50a3cb2f1300d45f03d13548/components/password_manager/core/browser/password_manager_util_unittest.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Sep 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/712d401798fa6897bdb418f3a436f3fd9dad6977

commit 712d401798fa6897bdb418f3a436f3fd9dad6977
Author: Gemene Narcis <gemene@google.com>
Date: Thu Sep 27 13:04:37 2018

Move HttpMetricsMigrationReporter into a separate class

This CL move the class that reports metrics about HTTP to HTTPS
migration in separate files. The class is renamed in
HttpCredentialCleaner since it will be extended in the next CLs to
remove obsolete HTTP credentials as well.

Bug: 871140
Change-Id: I52784893278a54876d68922cb2478a61974c049a
Reviewed-on: https://chromium-review.googlesource.com/1249122
Commit-Queue: Narcis Gemene <gemene@google.com>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594695}
[modify] https://crrev.com/712d401798fa6897bdb418f3a436f3fd9dad6977/components/password_manager/core/browser/BUILD.gn
[modify] https://crrev.com/712d401798fa6897bdb418f3a436f3fd9dad6977/components/password_manager/core/browser/DEPS
[add] https://crrev.com/712d401798fa6897bdb418f3a436f3fd9dad6977/components/password_manager/core/browser/http_credentials_cleaner.cc
[add] https://crrev.com/712d401798fa6897bdb418f3a436f3fd9dad6977/components/password_manager/core/browser/http_credentials_cleaner.h
[add] https://crrev.com/712d401798fa6897bdb418f3a436f3fd9dad6977/components/password_manager/core/browser/http_credentials_cleaner_unittest.cc
[modify] https://crrev.com/712d401798fa6897bdb418f3a436f3fd9dad6977/components/password_manager/core/browser/password_manager_util.cc
[modify] https://crrev.com/712d401798fa6897bdb418f3a436f3fd9dad6977/components/password_manager/core/browser/password_manager_util_unittest.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Sep 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/96d488a14c679084a46f3a3056d8aab869de555d

commit 96d488a14c679084a46f3a3056d8aab869de555d
Author: Gemene Narcis <gemene@google.com>
Date: Thu Sep 27 19:54:30 2018

Changes in HttpCredentialCleaner

This CL introduces a few improvements to HttpCredentialCleaner:
(1) A fix in removing the protocol from signon_realm,
(2) refactoring the unittest to use parameters,
(3) minor style fixes.

More details about (1):
HttpCredentialCleaner removes the protocol (HTTP or HTTPS) form
the signon_realm in order to compare the signon_realm of HTTP
credentials with the signon_realm of HTTPS credentials. Until now,
a GURL was created from the signon_realm of the form and then the
protocol was extracted from that GURL, resulting the signon_realm
excluding protocol. This can cause problems when the auth realm contains
characters that are forbidden in an url. This will lead in creating an
invalid url, and the resulting signon_realm with protocol exluded will
be an empty string.
This CL will avoid conversion from the signon_realm string to the GURL
and use other way to remove the protocol from the signon_realm.

More details about (2):
Unitests for this class were changed from one single test into a
bunch of parametrised tests in order to make debug easier in case
of failing test in the future.

Bug: 871140
Change-Id: Ic606d250f50806ae3a3fa07a480fcb01f5551c97
Reviewed-on: https://chromium-review.googlesource.com/1249104
Commit-Queue: Narcis Gemene <gemene@google.com>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594826}
[modify] https://crrev.com/96d488a14c679084a46f3a3056d8aab869de555d/components/password_manager/core/browser/http_credentials_cleaner.cc
[modify] https://crrev.com/96d488a14c679084a46f3a3056d8aab869de555d/components/password_manager/core/browser/http_credentials_cleaner_unittest.cc
[modify] https://crrev.com/96d488a14c679084a46f3a3056d8aab869de555d/components/password_manager/core/browser/invalid_realm_credential_cleaner.cc
[modify] https://crrev.com/96d488a14c679084a46f3a3056d8aab869de555d/components/password_manager/core/browser/password_manager_util.cc
[modify] https://crrev.com/96d488a14c679084a46f3a3056d8aab869de555d/components/password_manager/core/browser/password_manager_util.h
[modify] https://crrev.com/96d488a14c679084a46f3a3056d8aab869de555d/components/password_manager/core/browser/password_manager_util_unittest.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Sep 28

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c4d37b0d8ffd0eb121eae4a108470511ab1fc98c

commit c4d37b0d8ffd0eb121eae4a108470511ab1fc98c
Author: Gemene Narcis <gemene@google.com>
Date: Fri Sep 28 09:18:17 2018

Move HttpCredentialCleaner place of creation

This CL moves HttpCredentialCleaner instance inside
password_manager_utils.cc in order to be added in the clean-up
runner's queue.

Bug: 871140
Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs
Change-Id: Iaf321e3b136c235ea9eb0ab1489acbeefa8208d1
Reviewed-on: https://chromium-review.googlesource.com/1249071
Commit-Queue: Narcis Gemene <gemene@google.com>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595031}
[modify] https://crrev.com/c4d37b0d8ffd0eb121eae4a108470511ab1fc98c/chrome/browser/password_manager/password_store_factory.cc
[modify] https://crrev.com/c4d37b0d8ffd0eb121eae4a108470511ab1fc98c/components/password_manager/core/browser/blacklisted_duplicates_cleaner_unittest.cc
[modify] https://crrev.com/c4d37b0d8ffd0eb121eae4a108470511ab1fc98c/components/password_manager/core/browser/http_credentials_cleaner.cc
[modify] https://crrev.com/c4d37b0d8ffd0eb121eae4a108470511ab1fc98c/components/password_manager/core/browser/http_credentials_cleaner.h
[modify] https://crrev.com/c4d37b0d8ffd0eb121eae4a108470511ab1fc98c/components/password_manager/core/browser/http_credentials_cleaner_unittest.cc
[modify] https://crrev.com/c4d37b0d8ffd0eb121eae4a108470511ab1fc98c/components/password_manager/core/browser/invalid_realm_credential_cleaner_unittest.cc
[modify] https://crrev.com/c4d37b0d8ffd0eb121eae4a108470511ab1fc98c/components/password_manager/core/browser/password_manager_util.cc
[modify] https://crrev.com/c4d37b0d8ffd0eb121eae4a108470511ab1fc98c/components/password_manager/core/browser/password_manager_util.h
[modify] https://crrev.com/c4d37b0d8ffd0eb121eae4a108470511ab1fc98c/components/password_manager/core/browser/password_manager_util_unittest.cc
[modify] https://crrev.com/c4d37b0d8ffd0eb121eae4a108470511ab1fc98c/ios/chrome/browser/passwords/ios_chrome_password_store_factory.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Sep 28

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a6758f34c67c866c599cad37833f6162c9863b4d

commit a6758f34c67c866c599cad37833f6162c9863b4d
Author: Gemene Narcis <gemene@google.com>
Date: Fri Sep 28 14:17:55 2018

Clean Obsolete HTTP Credentials from the Password Store

This change is supposed to delete obsolete HTTP data from the password
store 60 seconds after start up.
This CL will delete HTTP credentials with HSTS enabled for that host and
for which an equivalent (i.e. same signon_realm, PasswordForm::Scheme,username
and password) HTTPS credential exists. Also it replaces HTTP credentials by an HTTPS
version of that form if site has HSTS enabled and no HTTPS credential
with same signon_realm, same PasswordForm::Scheme and username exists.

Bug: 871140, 879244
Change-Id: I605e39f25ce1052aa67d95cf84601a9db94c33a6
Reviewed-on: https://chromium-review.googlesource.com/1178282
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Commit-Queue: Narcis Gemene <gemene@google.com>
Cr-Commit-Position: refs/heads/master@{#595078}
[modify] https://crrev.com/a6758f34c67c866c599cad37833f6162c9863b4d/components/password_manager/core/browser/http_credentials_cleaner.cc
[modify] https://crrev.com/a6758f34c67c866c599cad37833f6162c9863b4d/components/password_manager/core/browser/http_credentials_cleaner.h
[modify] https://crrev.com/a6758f34c67c866c599cad37833f6162c9863b4d/components/password_manager/core/browser/http_credentials_cleaner_unittest.cc
[modify] https://crrev.com/a6758f34c67c866c599cad37833f6162c9863b4d/components/password_manager/core/browser/http_password_store_migrator.cc
[modify] https://crrev.com/a6758f34c67c866c599cad37833f6162c9863b4d/components/password_manager/core/browser/http_password_store_migrator.h
[modify] https://crrev.com/a6758f34c67c866c599cad37833f6162c9863b4d/components/password_manager/core/browser/http_password_store_migrator_unittest.cc
[modify] https://crrev.com/a6758f34c67c866c599cad37833f6162c9863b4d/components/password_manager/core/browser/password_manager.cc
[modify] https://crrev.com/a6758f34c67c866c599cad37833f6162c9863b4d/components/password_manager/core/browser/password_manager_util.cc
[modify] https://crrev.com/a6758f34c67c866c599cad37833f6162c9863b4d/components/password_manager/core/browser/password_manager_util.h
[modify] https://crrev.com/a6758f34c67c866c599cad37833f6162c9863b4d/components/password_manager/core/common/password_manager_pref_names.cc
[modify] https://crrev.com/a6758f34c67c866c599cad37833f6162c9863b4d/components/password_manager/core/common/password_manager_pref_names.h

Project Member

Comment 9 by bugdroid1@chromium.org, Nov 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/29b2fb6b0704d5da29f607175f80ee0e5e4f3021

commit 29b2fb6b0704d5da29f607175f80ee0e5e4f3021
Author: jdoerrie <jdoerrie@chromium.org>
Date: Wed Nov 21 19:10:55 2018

[Passwords] Fix DCHECK in GetSignonRealmWithProtocolExcluded

This change fixes a DCHECK in GetSignonRealmWithProtocolExcluded
triggered when operating on credentials where the form's origin contained
a port, but the signon_realm did not. For example. this can happen for
federated credentials.

Furthermore, this change improves the robustness of InvalidRealmCredentialCleaner
and HttpCredentialCleaner by performing the HTTP / HTTPS check on the form's
signon_realm instead of the origin.

Bug: 871140
Change-Id: If4c531e0c8741db05f5e1b7565c5fd6026836ae6
Reviewed-on: https://chromium-review.googlesource.com/c/1345512
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610120}
[modify] https://crrev.com/29b2fb6b0704d5da29f607175f80ee0e5e4f3021/components/password_manager/core/browser/BUILD.gn
[add] https://crrev.com/29b2fb6b0704d5da29f607175f80ee0e5e4f3021/components/password_manager/core/browser/credentials_cleaner.cc
[modify] https://crrev.com/29b2fb6b0704d5da29f607175f80ee0e5e4f3021/components/password_manager/core/browser/credentials_cleaner.h
[add] https://crrev.com/29b2fb6b0704d5da29f607175f80ee0e5e4f3021/components/password_manager/core/browser/credentials_cleaner_unittest.cc
[modify] https://crrev.com/29b2fb6b0704d5da29f607175f80ee0e5e4f3021/components/password_manager/core/browser/http_credentials_cleaner.cc
[modify] https://crrev.com/29b2fb6b0704d5da29f607175f80ee0e5e4f3021/components/password_manager/core/browser/invalid_realm_credential_cleaner.cc
[modify] https://crrev.com/29b2fb6b0704d5da29f607175f80ee0e5e4f3021/components/password_manager/core/browser/password_manager_util.cc
[modify] https://crrev.com/29b2fb6b0704d5da29f607175f80ee0e5e4f3021/components/password_manager/core/browser/password_manager_util.h
[modify] https://crrev.com/29b2fb6b0704d5da29f607175f80ee0e5e4f3021/components/password_manager/core/browser/password_manager_util_unittest.cc

Cc: -vabr@chromium.org
vabr going hobby only -> reducing involvement.
Please contact me directly in urgent matters.

Sign in to add a comment