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

Issue 892260 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 12
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocking:
issue 890133



Sign in to add a comment

[Passwords][Settings] Do not assume (website, username) combination will be unique

Project Member Reported by jdoerrie@chromium.org, Oct 4

Issue description

In its current implementation, chrome://settings/passwords assumes the pair of (website, username) to be unique for each entry. While this is mostly true, this assumption breaks in corner cases, see e.g. https://crbug.com/890133. 

 https://crbug.com/865458#c2  contains some ideas how this could be achieved instead.

 
Blocking: 890133
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 8

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

commit 9e057b75d8e0ca01ecbeead1a4553815527aedc7
Author: jdoerrie <jdoerrie@chromium.org>
Date: Mon Oct 08 17:01:58 2018

[Password Manager] Get All Logins With Affiliation Info

This change adds GetAllLoginsWithAffiliationAndBrandingInformation() to
the PasswordStore. Prior to this change clients had to call both
GetAutofillableLoginsWithAffiliationAndBrandingInformation() and
GetBlacklistLoginsWithAffiliationAndBrandingInformation() to obtain the
same information. This was unnecessarily complex, in particular if
knowledge about which response corresponds to which request was
required. As a result of this change the password population logic in
PasswordManagerPresenter can be greatly simplified.

Bug:  892260 , 778146
Change-Id: Ic0773c4d0337bc3be1fded6f22bda8d8ac2927bb
Reviewed-on: https://chromium-review.googlesource.com/c/1268198
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597587}
[modify] https://crrev.com/9e057b75d8e0ca01ecbeead1a4553815527aedc7/chrome/browser/ui/passwords/password_manager_presenter.cc
[modify] https://crrev.com/9e057b75d8e0ca01ecbeead1a4553815527aedc7/chrome/browser/ui/passwords/password_manager_presenter.h
[modify] https://crrev.com/9e057b75d8e0ca01ecbeead1a4553815527aedc7/components/password_manager/core/browser/password_store.cc
[modify] https://crrev.com/9e057b75d8e0ca01ecbeead1a4553815527aedc7/components/password_manager/core/browser/password_store.h
[modify] https://crrev.com/9e057b75d8e0ca01ecbeead1a4553815527aedc7/components/password_manager/core/browser/password_store_unittest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Oct 9

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

commit 96cd9f3d507355ec61651fac0bd72efe313bb5cd
Author: jdoerrie <jdoerrie@chromium.org>
Date: Tue Oct 09 10:40:39 2018

[Password Manager] Clean-Up PasswordManagerPresenter

This change cleans-up the implementation of PasswordManagerPresenter by
introducing two helper methods, reducing code duplication. Furthermore,
it re-arranges the member function definitions, so that they match their
declaration order in the header file.

Bug:  892260 
Change-Id: I8311f21bd26d1f478418c11ad64896187247d55b
Reviewed-on: https://chromium-review.googlesource.com/c/1269734
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597871}
[modify] https://crrev.com/96cd9f3d507355ec61651fac0bd72efe313bb5cd/chrome/browser/ui/passwords/password_manager_presenter.cc
[modify] https://crrev.com/96cd9f3d507355ec61651fac0bd72efe313bb5cd/chrome/browser/ui/passwords/password_manager_presenter.h

Project Member

Comment 4 by bugdroid1@chromium.org, Oct 10

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

commit 1bd7fe4b761a8a2af6f7ba943fd25a42eb8289c9
Author: jdoerrie <jdoerrie@chromium.org>
Date: Wed Oct 10 15:36:16 2018

[Password Manager] Use Equivalence Classes in PasswordManagerPresenter

This change updates PasswordManagerPresenter to store the cached
PasswordForms in equivalence classes, joining the data structures
required to store regular entries and duplicates. Furthermore, this
change updates the corresponding unittest to rely on testing frameworks,
improving encapsulation.

Bug:  892260 
Change-Id: I3b0e1c1e9bbdd7f98c2e0291ae7128d626d7b251
Reviewed-on: https://chromium-review.googlesource.com/c/1270935
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598337}
[modify] https://crrev.com/1bd7fe4b761a8a2af6f7ba943fd25a42eb8289c9/chrome/browser/ui/passwords/password_manager_presenter.cc
[modify] https://crrev.com/1bd7fe4b761a8a2af6f7ba943fd25a42eb8289c9/chrome/browser/ui/passwords/password_manager_presenter.h
[modify] https://crrev.com/1bd7fe4b761a8a2af6f7ba943fd25a42eb8289c9/chrome/browser/ui/passwords/password_manager_presenter_unittest.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Oct 11

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

commit 3bdd96ee8ed4616345e69dc1e5deae117e639613
Author: Jan Wilken Doerrie <jdoerrie@chromium.org>
Date: Thu Oct 11 20:58:46 2018

[Password Manager] Use Sort Keys in PasswordPrivateDelegate

This change updates PasswordPrivateDelegate to use sort keys when
communicating with the PasswordManagerPresenter. Communication with the
JavaScript continues to be done via indices, which is why a utility
class to map between strings and integers is introduced. Furthermore,
PasswordManagerPresenter's API is updated with sort key based overloads.
This fixes the linked bug, which assumed the tuple (index, website,
username) will be a stable identifier for each entry present in the list
of passwords. This assumption can break, when there are multiple entries
with the same website and username. Replacing index with an identifier
that is derived from more information solves this issue.

TBR=tedchoc@chromium.org

Bug:  892260 
Change-Id: I4df39a8c7ebe33c1e56bb64c8a3a96262e246584
Reviewed-on: https://chromium-review.googlesource.com/c/1273717
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598929}
[modify] https://crrev.com/3bdd96ee8ed4616345e69dc1e5deae117e639613/chrome/browser/android/password_ui_view_android.cc
[modify] https://crrev.com/3bdd96ee8ed4616345e69dc1e5deae117e639613/chrome/browser/android/password_ui_view_android.h
[modify] https://crrev.com/3bdd96ee8ed4616345e69dc1e5deae117e639613/chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc
[modify] https://crrev.com/3bdd96ee8ed4616345e69dc1e5deae117e639613/chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.h
[modify] https://crrev.com/3bdd96ee8ed4616345e69dc1e5deae117e639613/chrome/browser/extensions/api/passwords_private/passwords_private_utils.cc
[modify] https://crrev.com/3bdd96ee8ed4616345e69dc1e5deae117e639613/chrome/browser/extensions/api/passwords_private/passwords_private_utils.h
[modify] https://crrev.com/3bdd96ee8ed4616345e69dc1e5deae117e639613/chrome/browser/extensions/api/passwords_private/passwords_private_utils_unittest.cc
[modify] https://crrev.com/3bdd96ee8ed4616345e69dc1e5deae117e639613/chrome/browser/ui/passwords/password_manager_presenter.cc
[modify] https://crrev.com/3bdd96ee8ed4616345e69dc1e5deae117e639613/chrome/browser/ui/passwords/password_manager_presenter.h
[modify] https://crrev.com/3bdd96ee8ed4616345e69dc1e5deae117e639613/chrome/browser/ui/passwords/password_ui_view.h
[modify] https://crrev.com/3bdd96ee8ed4616345e69dc1e5deae117e639613/chrome/browser/ui/passwords/password_ui_view_mock.h

Status: Fixed (was: Assigned)
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 15

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

commit a0801f6becc4f7c1d8037d37e1ebb55aa38ae381
Author: Jan Wilken Doerrie <jdoerrie@chromium.org>
Date: Mon Oct 15 20:04:13 2018

[Password][Settings] Rename Index to Id in Password Settings

r598929 changed the communication between the password settings
backend and frontend to identify password list elements by a unique
identifier instead of relying on the element's index in the list.
This change renames the appropriate members of the DisplayEntry structs
to reflect this change in semantics.

Bug:  892260 
Cq-Include-Trybots: luci.chromium.try:closure_compilation
Change-Id: Ic4ea4456ef88f8f0a94ffc6a0a824d2f9da6659a
Reviewed-on: https://chromium-review.googlesource.com/c/1278213
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599728}
[modify] https://crrev.com/a0801f6becc4f7c1d8037d37e1ebb55aa38ae381/chrome/browser/extensions/api/passwords_private/passwords_private_api.cc
[modify] https://crrev.com/a0801f6becc4f7c1d8037d37e1ebb55aa38ae381/chrome/browser/extensions/api/passwords_private/passwords_private_apitest.cc
[modify] https://crrev.com/a0801f6becc4f7c1d8037d37e1ebb55aa38ae381/chrome/browser/extensions/api/passwords_private/passwords_private_delegate.h
[modify] https://crrev.com/a0801f6becc4f7c1d8037d37e1ebb55aa38ae381/chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc
[modify] https://crrev.com/a0801f6becc4f7c1d8037d37e1ebb55aa38ae381/chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.h
[modify] https://crrev.com/a0801f6becc4f7c1d8037d37e1ebb55aa38ae381/chrome/browser/extensions/api/passwords_private/passwords_private_event_router.cc
[modify] https://crrev.com/a0801f6becc4f7c1d8037d37e1ebb55aa38ae381/chrome/browser/extensions/api/passwords_private/passwords_private_event_router.h
[modify] https://crrev.com/a0801f6becc4f7c1d8037d37e1ebb55aa38ae381/chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js
[modify] https://crrev.com/a0801f6becc4f7c1d8037d37e1ebb55aa38ae381/chrome/common/extensions/api/passwords_private.idl
[modify] https://crrev.com/a0801f6becc4f7c1d8037d37e1ebb55aa38ae381/chrome/test/data/extensions/api_test/passwords_private/test.js
[modify] https://crrev.com/a0801f6becc4f7c1d8037d37e1ebb55aa38ae381/chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js
[modify] https://crrev.com/a0801f6becc4f7c1d8037d37e1ebb55aa38ae381/chrome/test/data/webui/settings/settings_passwords_section_browsertest.js
[modify] https://crrev.com/a0801f6becc4f7c1d8037d37e1ebb55aa38ae381/third_party/closure_compiler/externs/passwords_private.js

Project Member

Comment 8 by bugdroid1@chromium.org, Oct 16

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

commit 1913a62d9bc634c9db0416103c0cc701aeda710e
Author: Jan Wilken Doerrie <jdoerrie@chromium.org>
Date: Tue Oct 16 17:09:02 2018

[Password][Settings] Rename index to id in password_manager_proxy.js

This change is a follow-up to r599728 that renamed index to id in
passwords_private.idl. That change missed to update
password_manager_proxy.js, which is fixed by this change.

Bug:  892260 ,  895785 
Change-Id: Id1ae5e6e46f8dd66c51bb01988b537ec99a1975c
Reviewed-on: https://chromium-review.googlesource.com/c/1283129
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600024}
[modify] https://crrev.com/1913a62d9bc634c9db0416103c0cc701aeda710e/chrome/browser/resources/settings/passwords_and_forms_page/password_manager_proxy.js

Project Member

Comment 9 by bugdroid1@chromium.org, Oct 17

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

commit e5b3b7b382787d750c0bee81feef48117b65caff
Author: Jan Wilken Doerrie <jdoerrie@chromium.org>
Date: Wed Oct 17 10:05:44 2018

[Passwords][Settings] Simplify Password List Update Logic

This change makes use of the updated semantics of PasswordUiEntry.id
to simplify the update list behavior of passwords_section.js.

Bug:  892260 
Change-Id: Icfde6048049edd6e418580660b34928db798764d
Reviewed-on: https://chromium-review.googlesource.com/c/1283013
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600335}
[modify] https://crrev.com/e5b3b7b382787d750c0bee81feef48117b65caff/chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js
[modify] https://crrev.com/e5b3b7b382787d750c0bee81feef48117b65caff/chrome/test/data/webui/settings/settings_passwords_section_browsertest.js

Cc: dpa...@chromium.org aee@chromium.org scottchen@chromium.org
 Issue 865458  has been merged into this issue.

Sign in to add a comment