New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user
Status: Assigned
Owner:
OOO
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Feature

Blocking:
issue 341477
issue 790993



Sign in to add a comment
Android implementation of password export
Project Member Reported by vabr@chromium.org, Nov 27 Back to list
Design: https://docs.google.com/a/chromium.org/document/d/1miKr2x0PTNIKgt3RQeur51ICQ66uszlYAmFXJONbG_0/edit?usp=sharing

Mocks: go chrome-pwd-export-mocks-android (Google-internal only)
 
Adding screenshots for https://crrev.com/c/776681
menu-closed.png
80.8 KB View Download
menu-open.png
84.9 KB View Download
Project Member Comment 2 by bugdroid1@chromium.org, Nov 28
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dcd1fe27c6a6d37766c55a0a26b6db7f655ecd39

commit dcd1fe27c6a6d37766c55a0a26b6db7f655ecd39
Author: Vaclav Brozek <vabr@chromium.org>
Date: Tue Nov 28 08:29:05 2017

[Android settings] Add a menu item to export passwords

This CL adds a menu item, behind an off-by-default feature, for
exporting passwords from Chrome's settings. The menu item
currently does not perform any action.

Design: https://docs.google.com/a/chromium.org/document/d/1miKr2x0PTNIKgt3RQeur51ICQ66uszlYAmFXJONbG_0/edit?usp=sharing

Mocks: go chrome-pwd-export-mocks-android (Google-internal only)

Screenshots: https://crbug.com/788701#c1

Bug: 788701
Change-Id: I7e8b515596db4be061d494019a34220b9c019caf
Reviewed-on: https://chromium-review.googlesource.com/776681
Reviewed-by: Theresa <twellington@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519607}
[add] https://crrev.com/dcd1fe27c6a6d37766c55a0a26b6db7f655ecd39/chrome/android/java/res/menu/save_password_preferences_action_bar_menu.xml
[modify] https://crrev.com/dcd1fe27c6a6d37766c55a0a26b6db7f655ecd39/chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java
[modify] https://crrev.com/dcd1fe27c6a6d37766c55a0a26b6db7f655ecd39/chrome/android/java/strings/android_chrome_strings.grd
[modify] https://crrev.com/dcd1fe27c6a6d37766c55a0a26b6db7f655ecd39/chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java
[modify] https://crrev.com/dcd1fe27c6a6d37766c55a0a26b6db7f655ecd39/chrome/browser/android/chrome_feature_list.cc

Adding screencasts for https://crrev.com/c/797453
no-reauth.mp4
658 KB View Download
with-reauth.mp4
1.3 MB View Download
Maybe we can explore at some point in the future how to send users to the screen where they can turn on their screen lock. I think that many users would not know what a screen lock is and where to configure it.
Thanks for the suggestion in #4. I filed bug 789912 to track that.
Project Member Comment 6 by bugdroid1@chromium.org, Nov 30
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c721c7cddaa61e28a360f5942682d7417fc7c5c9

commit c721c7cddaa61e28a360f5942682d7417fc7c5c9
Author: Vaclav Brozek <vabr@chromium.org>
Date: Thu Nov 30 14:30:29 2017

[Android password settings] Separate reauthentication helper

Currently the logic for reauthentication is scattered over these files:

PasswordEntryEditor.java
SavePasswordsPreferences.java

where SavePasswordsPreferences take care of resetting and own the
timer for reauthentication validity (because it should be tied to the
passwords settings lifetime), and PasswordEntryEditor contains logic
to decide about reauthentication steps based on that timer and on
other device state.

This alone is already a little disorganised. In a follow-up CL, one
more callsite for the reauth logic will be added, in
SavePasswordsPreferences.java, meaning that there would be
difficulties reusing the code currently in PasswordEntryEditor.java.

For that reason, this CL introduces a new class,
ReauthenticationManager.java, which encapsulates the logic. Because
the timer has been a static value, the new class is also composed of
static methods.

Bug: 788701
Change-Id: Iea2007fdd9c89bff77595b2cf5ea8ac4e1501792
Reviewed-on: https://chromium-review.googlesource.com/797510
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Maria Khomenko <mariakhomenko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520536}
[modify] https://crrev.com/c721c7cddaa61e28a360f5942682d7417fc7c5c9/chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java
[modify] https://crrev.com/c721c7cddaa61e28a360f5942682d7417fc7c5c9/chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragment.java
[add] https://crrev.com/c721c7cddaa61e28a360f5942682d7417fc7c5c9/chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ReauthenticationManager.java
[modify] https://crrev.com/c721c7cddaa61e28a360f5942682d7417fc7c5c9/chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java
[modify] https://crrev.com/c721c7cddaa61e28a360f5942682d7417fc7c5c9/chrome/android/java_sources.gni
[modify] https://crrev.com/c721c7cddaa61e28a360f5942682d7417fc7c5c9/chrome/android/junit/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragmentTest.java
[add] https://crrev.com/c721c7cddaa61e28a360f5942682d7417fc7c5c9/chrome/android/junit/src/org/chromium/chrome/browser/preferences/password/ReauthenticationManagerTest.java

Screenshot for https://crrev.com/c/803955.
export-warning.png
94.2 KB View Download
Project Member Comment 8 by bugdroid1@chromium.org, Dec 5
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/999f75ab54fb2cf89c57263e91260dce08aa6dd3

commit 999f75ab54fb2cf89c57263e91260dce08aa6dd3
Author: Vaclav Brozek <vabr@chromium.org>
Date: Tue Dec 05 11:15:14 2017

Extend password export/import flags to Android

The connected base::Features are also used on Android, but the
about:flags page is set up to only allow changing it on desktop.

This CL adds the possibility to control those two base::Features
through about:flags on Android as well.

Bug: 788701
Change-Id: Ib6ae924ed453c8ceda746746fb10659d718d18e0
Reviewed-on: https://chromium-review.googlesource.com/808105
Reviewed-by: Christos Froussios <cfroussios@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521662}
[modify] https://crrev.com/999f75ab54fb2cf89c57263e91260dce08aa6dd3/chrome/browser/about_flags.cc

Project Member Comment 9 by bugdroid1@chromium.org, Dec 5
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cf177bac5e2eb43f3520532c4588e9594dbe9159

commit cf177bac5e2eb43f3520532c4588e9594dbe9159
Author: Vaclav Brozek <vabr@chromium.org>
Date: Tue Dec 05 11:39:00 2017

Fix invalid cast in SavePasswordsPreferences#resetList

The resetList method tries to call PreferenceGroup#removeAll on
TextMessagePreference (which is not a PreferenceGroup) if an update of
the settings happens while the last state had no saved passwords to
display.

This is a rare situation because people who have no saved passwords
are not very often inspecting their passwords settings, but can happen
in reality as well (e.g., when somebody inspects the page during early
moments of syncing and Chrome sync populates the page as the user
views it).

This CL introduces a special method to do the job of resetList in the
case there are no lists, and adds a test which executed the invalid
cast before the fix.

Bug: 788701
Change-Id: I64ada510deb78cc7b8f22f085ef2f401e9bb76ba
Reviewed-on: https://chromium-review.googlesource.com/806165
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Finnur Thorarinsson <finnur@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521667}
[modify] https://crrev.com/cf177bac5e2eb43f3520532c4588e9594dbe9159/chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java
[modify] https://crrev.com/cf177bac5e2eb43f3520532c4588e9594dbe9159/chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java

Project Member Comment 10 by bugdroid1@chromium.org, Dec 5
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ef6138f4969956e92b9e64bb1a60645b67c42ddd

commit ef6138f4969956e92b9e64bb1a60645b67c42ddd
Author: Vaclav Brozek <vabr@chromium.org>
Date: Tue Dec 05 15:07:27 2017

Remove dead code SavePasswordsPreferences#onPreferenceChange

SavePasswordsPreferences never registers itself as an
OnPreferenceChangeListener so its onPreferenceChange method never gets
called. This CL removes that dead code.

Note that OnPreferenceChangeListener is still used in-line, so the
import is not removed.

Bug: 788701
Change-Id: I87ae4f6be3e2dc4322fad56551338b0a8355e062
Reviewed-on: https://chromium-review.googlesource.com/808265
Reviewed-by: Finnur Thorarinsson <finnur@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521691}
[modify] https://crrev.com/ef6138f4969956e92b9e64bb1a60645b67c42ddd/chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java

Project Member Comment 11 by bugdroid1@chromium.org, Dec 6
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0db267e941f1c227914d20cc272617188bd3fd04

commit 0db267e941f1c227914d20cc272617188bd3fd04
Author: Vaclav Brozek <vabr@chromium.org>
Date: Wed Dec 06 10:36:25 2017

Allow mocking PasswordUIView.java

Tests are needed for viewing passwords in settings. Those tests need
an easy way to mock PasswordUIView, which communicates with C++ in the
production version.

Therefore this CL does:
(1) Define an interface, PasswordManagerHandler, for the public methods of
    PasswordUIView to be mocked in the tests (coming in
    https://crrev.com/c/797453).
(2) Create PasswordManagerHandlerProvider which encapsulates the logic
    for managing observers, life-time and test-replacement of the
    PasswordManagerHandler implementations.
(3) Lift SavedPasswordEntry to top level for reasons explained in its
    class comment.

This CL should not change any functionality, so current test coverage
is assumed to be sufficient for the new code.
The new tests are postponed until https://crrev.com/c/797453, which
also introduces other test utilities to make writing them less of a
pain.

Bug: 788701
Change-Id: I2b90a613065df50cad85de85ffa620a88df61a6e
Reviewed-on: https://chromium-review.googlesource.com/806155
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522059}
[add] https://crrev.com/0db267e941f1c227914d20cc272617188bd3fd04/chrome/android/java/src/org/chromium/chrome/browser/PasswordManagerHandler.java
[modify] https://crrev.com/0db267e941f1c227914d20cc272617188bd3fd04/chrome/android/java/src/org/chromium/chrome/browser/PasswordUIView.java
[add] https://crrev.com/0db267e941f1c227914d20cc272617188bd3fd04/chrome/android/java/src/org/chromium/chrome/browser/SavedPasswordEntry.java
[modify] https://crrev.com/0db267e941f1c227914d20cc272617188bd3fd04/chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java
[add] https://crrev.com/0db267e941f1c227914d20cc272617188bd3fd04/chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordManagerHandlerProvider.java
[modify] https://crrev.com/0db267e941f1c227914d20cc272617188bd3fd04/chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java
[modify] https://crrev.com/0db267e941f1c227914d20cc272617188bd3fd04/chrome/android/java_sources.gni

Project Member Comment 12 by bugdroid1@chromium.org, Dec 6 (6 days ago)
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/06c20788cc532a7565c4004b17789ba409751de1

commit 06c20788cc532a7565c4004b17789ba409751de1
Author: Vaclav Brozek <vabr@chromium.org>
Date: Wed Dec 06 11:12:26 2017

[Android passwords settings] Add reauthentication for export

Mocks: go chrome-pwd-export-mocks-android (Google-internal only)

Screencast after the change: https://crbug.com/788701#c3

The CL adds a reauthentication step before the user can trigger
exporting passwords.

In addition to that, the CL also wraps the check for screen lock
availability in ReauthenticationManager.isScreenLockSetUp, to
make it possible to override for testing.

Also, this CL adds two tests for viewing passwords, because one
of earlier iterations of this CL broke something for the viewing
passwords feature and such tests would have caught it.

Bug: 788701
Change-Id: Ibfa37192e26bc5797236ba9378aa1eb0f17f79cc
Reviewed-on: https://chromium-review.googlesource.com/797453
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522061}
[modify] https://crrev.com/06c20788cc532a7565c4004b17789ba409751de1/chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java
[modify] https://crrev.com/06c20788cc532a7565c4004b17789ba409751de1/chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragment.java
[modify] https://crrev.com/06c20788cc532a7565c4004b17789ba409751de1/chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ReauthenticationManager.java
[modify] https://crrev.com/06c20788cc532a7565c4004b17789ba409751de1/chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java
[modify] https://crrev.com/06c20788cc532a7565c4004b17789ba409751de1/chrome/android/java/strings/android_chrome_strings.grd
[modify] https://crrev.com/06c20788cc532a7565c4004b17789ba409751de1/chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java

Project Member Comment 13 by bugdroid1@chromium.org, Dec 6 (6 days ago)
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/05edf9ed8accdd886adfe88ee93b2a3bdf3a1a25

commit 05edf9ed8accdd886adfe88ee93b2a3bdf3a1a25
Author: Vaclav Brozek <vabr@chromium.org>
Date: Wed Dec 06 11:24:30 2017

[Android password settings] Display warning before export

This CL adds a warning to be displayed before the user can export
their passwords.

Mocks:
https://docs.google.com/presentation/d/1nIm5OmaOnb85ZAwMZPSVqHT0vkFbsRYZhOzbmznWU_c/edit#slide=id.g289b1efcd8_0_21

Note: the mock shows slightly different styling, but with the UX
designer we settled on using a standard dialog style already existing
in Chromium and checking separately whether that style needs fixing,
rather than introducing inconsistent styling among Chrome's dialogs.

Design: go chrome-pwd-export

Screenshot: https://crbug.com/788701#c7

Bug: 788701
Change-Id: I3a2dbdc24429c5662fdc2b178630a8e45ba603a1
Reviewed-on: https://chromium-review.googlesource.com/803955
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522062}
[add] https://crrev.com/05edf9ed8accdd886adfe88ee93b2a3bdf3a1a25/chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ExportWarningDialogFragment.java
[modify] https://crrev.com/05edf9ed8accdd886adfe88ee93b2a3bdf3a1a25/chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java
[modify] https://crrev.com/05edf9ed8accdd886adfe88ee93b2a3bdf3a1a25/chrome/android/java/strings/android_chrome_strings.grd
[modify] https://crrev.com/05edf9ed8accdd886adfe88ee93b2a3bdf3a1a25/chrome/android/java_sources.gni
[modify] https://crrev.com/05edf9ed8accdd886adfe88ee93b2a3bdf3a1a25/chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java

Comment 14 by vabr@chromium.org, Dec 6 (6 days ago)
Screenshots for https://crrev.com/c/811186.
export-disabled.png
66.6 KB View Download
export-enabled.png
85.6 KB View Download
Project Member Comment 15 by bugdroid1@chromium.org, Dec 7 (5 days ago)
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/065cd806af680a383ec1794f08e27702c1816980

commit 065cd806af680a383ec1794f08e27702c1816980
Author: Vaclav Brozek <vabr@chromium.org>
Date: Thu Dec 07 11:10:02 2017

[Android settings] Disable password export if there are no saved passwords

If the settings do not show any saved passwords, the export button in
the menu should still be present, but disabled.

Note that the existence of "blacklisted" entries, i.e., entries where
the user asked Chrome to never save a password, does not influence
the state of the menu item, because those entries are not exported.

Mocks: go chrome-pwd-export-mocks-android (currently slide 10,
Google-internal only)

General design:
https://docs.google.com/a/chromium.org/document/d/1miKr2x0PTNIKgt3RQeur51ICQ66uszlYAmFXJONbG_0/edit?usp=sharing

Bug: 788701
Change-Id: I1f5ad12eef9ef6ec1c9b0529c5b1ef16941c5298
Reviewed-on: https://chromium-review.googlesource.com/811186
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522400}
[modify] https://crrev.com/065cd806af680a383ec1794f08e27702c1816980/chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java
[modify] https://crrev.com/065cd806af680a383ec1794f08e27702c1816980/chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java

Comment 16 by vabr@chromium.org, Dec 7 (5 days ago)
Blockedon: -341477
Blocking: 341477 790993
Comment 17 by vabr@chromium.org, Dec 7 (5 days ago)
Status: Assigned
Status update:
One CL in review: https://crrev.com/c/809127
One CL in progress: https://crrev.com/c/793838
One more task pending -- adding the progress indicator before the export intent.

Will return in 2018 to complete this.
Sign in to add a comment