Irmak: feel free to take it if you want.

Hi!

I can take care of this one. :)

Reminder: I am only able to work after 6pm Munich time (because of school).

Hopefully I will send a cl today.

Thanks a lot Irmak! :)

In the attachment I am adding a video for a possible implementation of timeout. This is a WIP and the video is just for demonstration purposes.

The behavior in the video takes the timeout limit as 3 seconds. Counter starts when the bubble is opened. After 3 seconds, eye icon disappears. If user has the password visible when the timeout happens, the password becomes masked again and then the eye icon disappears.

Timer restarts if user closes and reopens the bubble.

It would be very helpful to me if we can clarify some things for the next steps:
- Is the timer going to be started by submit action (automatically creates the bubble) or the bubble creation (may also be triggered manually by clicking on the key icon)? Do we start timer whichever happens first?
- Do we care if user closes and opens the bubble again? Is timer going to be started only once or will reopening the bubble change anything?

Thanks!

Deleted: 3sec_timeout.mkv 108 KB

3sec_timeout.mkv 108 KB Download

Hi Irmak!

Thanks for starting implementation.

We corrected the expected behavior.

NEW SPECIFICATION:
The timer should control whether the password can be revealed without re-authentication. 
Therefore, there will be two states: 
1. Unlocked: a user can reveal passwords without re-authentication. - The timer is running.
2. Locked: OS re-authentication is required to reveal passwords. - The timer is not running.
Timeout: 90 seconds. (it is subject to change)
The timer starts/restarts (switch to “Unlocked” state) once:
* Automatic bubble pops up.
* Manual fallback bubble is shown/updated (“update” means a user changed field value and they are propagated to the prompt).
* A user clicked to reveal passwords. (The timer may be running and may not. If not, OS re-authentication will be requested.)

The password is concealed when the timer expires, the user clicks the eye icon a second time, or if the dialog disappears (regardless of reason for disappearing).



Feedback to your questions and video:
* the eye icon shouldn't disappear. Password revealing may require OS re-authentication, but the icon is always there. It is always possible to see the password.
* Timer doesn't restart when user closed/reopens the bubble. Timer starts on bubble shown and restarts when user reveals passwords.
* Let's start a timer on bubble shown event (but not on submission event).
* The fallback bubble can survive 90 seconds only if user keeps the bubble open. Therefore, the timer should affect on fallback as well. I believe the timer should start in ManagePasswordsUIController::OnShowManualFallbackForSaving

Hey Irmak!

Please keep us updated about the progress. This issue is a part of the core functionality of your feature which we would like to land before M63's feature freeze. Sorry that I have to hurry you up :(
You can split the change into several CLs and implement only some of them, we will take care of the rest. Another option is that we finish a CL you started. 

We can split into the following changes, for example:
* hide passwords after 90 seconds.
* request authentication when needed
* hide passwords when a bubble is reopened. 

Best regards,
Maxim 

Hi!

Maybe it is better if we go with the second option for this bug. I am only able to work after school and that sometimes is a very short time. :( Since this is a blocking issue, it probably is better for me to take some other non-blocking issues.

Good news is; I will (hopefully) get a Mac tomorrow. This means that I can work on the Mac implementation of the password drop down. 

I don't know if you saw it but in case that you didn't, my WIP was here: https://chromium-review.googlesource.com/c/chromium/src/+/686820

Best,
Irmak

Hi Irmak!

I took that issue. Actually, it is even more complicated when I thought. 

My suggestions about your contribution:
* Rename |other_possible_passwords| in PasswordForm to |all_possible_passwords| (as follow up of your CL)
*  Issue 770055  or  Issue 770051  may be a candidate. It is polishing work (perhaps just a couple of lines) and should be done till branch point. But again... I am not  sure if we will not start hurrying you up :)
* I created Issue 770046. It is umbrella bug for UI issues including bubble issues. I believe you have serious experience with bubble code. Feel free to take any of bugs. Some of them may be already fixed. To double check, let me what bugs you took/closed.

Thanks again for your attitude. Sorry for revoking tasks assigned to you.

Regards,
Maxim

I have started with the renaming issue. I have sent it to review. Thank you for the other suggestions too, I will let you know once I take another bug. :)

Happy weekends! ^^

I object to the proposed solution. There are following cases
1. Password is saved. A stranger can navigate to the login page and append a symbol to the username. The manual fallback appears and the password is revealed. Alternatively one can use the DevTools. Both ways are trivial.
2. User left the page with a password typed. Same is above is applicable. No timer solves the problem.

My suggestion is to always reveal the password for the manual fallback case as it disappears after 90 seconds anyway.

3. Submission detected, the form is gone, user is offered to save the password. I don't want us to introduce any reauth because it renders the feature useless. The reauth dialog would close the bubble and not many users would reopen it again. I see two reasonable options instead
- always show the password. Justification: it's super easy to click 'Save' and then we are back to the state 1) where the password can be easily stolen.
- A middle ground. The password is viewable through the automatic password prompt. The password isn't viewable when the prompt was opened manually (except manual fallback case). Justification: the user will most likely interact with the page by clicking/typing. Those actions dismiss the bubble and the password can't be stolen after that. At the same time the UX in the automatic prompt stays convenient.

We discussed with the team and come to the following conclusions. There are 4 states:

- Manual fallback, a password was autofilled -> it's NOT OK to reveal the passwords in the bubble by default.
- Manual fallback, no password was autofilled -> OK to show the plain text (typed) passwords in the bubble.
- Submission detected, the bubble automatically popped up -> it's OK to reveal the passwords.
- Submission detected, the automatic bubble was dismissed and later manually brought up by clicking the icon -> it's NOT OK to reveal the passwords by default.

As a quick solution for "NOT OK" case we can just hide the eye icon. After the branch point we could introduce a reauth:
- The user clicks the icon.
- Reauth dialog pops up, the bubble is dismissed because the focus is gone.
- User finishes the reauth. Chrome pops up the bubble back iff the UI state is still "PENDING_PASSWORD". The password is revealed iff the reauth was successful.

As of now I see the only corner case not covered:
- The password was autofilled.
- The attacker edits the username and clicks "Login".
- The login is obviously not successful but the Chrome password manager may mistakenly prompts the user and the password is visible.
I don't consider it a serious flaw because mostly Chrome detects successful submission. Nevertheless, we can fix it by requiring the reauth iff
- Chrome has credentials saved for the page, or
- the bubble was brought up manually after successful login.

 Issue 769237  has been merged into this issue.

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

commit faf1e85261c09fe27c7168fb753d319ff7763d4b
Author: Maxim Kolosovskiy <kolos@chromium.org>
Date: Mon Oct 09 10:54:30 2017

[Password Manager] Add flag whether |PasswordForm.all_possible_passwords| include autofilled value or its part

If any password field has autofilled value or its part, Chrome should request reauth when a user clicks the eye icon to reveal password(s).

Bug:  769237 ,  768742 
Change-Id: If38f87a97dbe794f454996d58ce0371d61fd82e0
Reviewed-on: https://chromium-review.googlesource.com/700457
Commit-Queue: Maxim Kolosovskiy <kolos@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507349}
[modify] https://crrev.com/faf1e85261c09fe27c7168fb753d319ff7763d4b/components/autofill/content/common/autofill_param_traits_macros.h
[modify] https://crrev.com/faf1e85261c09fe27c7168fb753d319ff7763d4b/components/autofill/content/common/autofill_types.mojom
[modify] https://crrev.com/faf1e85261c09fe27c7168fb753d319ff7763d4b/components/autofill/content/common/autofill_types_struct_traits.cc
[modify] https://crrev.com/faf1e85261c09fe27c7168fb753d319ff7763d4b/components/autofill/content/common/autofill_types_struct_traits.h
[modify] https://crrev.com/faf1e85261c09fe27c7168fb753d319ff7763d4b/components/autofill/content/common/autofill_types_struct_traits_unittest.cc
[modify] https://crrev.com/faf1e85261c09fe27c7168fb753d319ff7763d4b/components/autofill/content/renderer/password_autofill_agent.cc
[modify] https://crrev.com/faf1e85261c09fe27c7168fb753d319ff7763d4b/components/autofill/content/renderer/password_form_conversion_utils.cc
[modify] https://crrev.com/faf1e85261c09fe27c7168fb753d319ff7763d4b/components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc
[modify] https://crrev.com/faf1e85261c09fe27c7168fb753d319ff7763d4b/components/autofill/core/common/password_form.cc
[modify] https://crrev.com/faf1e85261c09fe27c7168fb753d319ff7763d4b/components/autofill/core/common/password_form.h
[modify] https://crrev.com/faf1e85261c09fe27c7168fb753d319ff7763d4b/components/password_manager/core/browser/password_form_manager.cc

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

commit 8a6beb7ab5262f4566462d06db1dcaa00bb4522e
Author: Maxim Kolosovskiy <kolos@chromium.org>
Date: Mon Oct 09 20:14:46 2017

[Password Manager] Hide the eye icon in a prompt for privacy reasons in some cases

Based on https://bugs.chromium.org/p/chromium/issues/detail?id=768742#c14, 
* manual fallback should have the eye icon iff the form doesn't contain autofilled value or its part.
* automatic prompt should have the eye icon iff the bubble is shown first time (i.e. right after submission). If the bubble was closed and re-opened again, the eye shouldn't be available.

In further CL, re-authentication will be implemented instead of icon hiding. 

Bug:  769237 ,  768742 
Change-Id: I12ec0b3fa9bc3fa0c42fd47323dabdc04726ae91
Reviewed-on: https://chromium-review.googlesource.com/704642
Commit-Queue: Maxim Kolosovskiy <kolos@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507466}
[modify] https://crrev.com/8a6beb7ab5262f4566462d06db1dcaa00bb4522e/chrome/browser/ui/passwords/manage_passwords_bubble_model.cc
[modify] https://crrev.com/8a6beb7ab5262f4566462d06db1dcaa00bb4522e/chrome/browser/ui/passwords/manage_passwords_bubble_model.h
[modify] https://crrev.com/8a6beb7ab5262f4566462d06db1dcaa00bb4522e/chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc
[modify] https://crrev.com/8a6beb7ab5262f4566462d06db1dcaa00bb4522e/chrome/browser/ui/passwords/manage_passwords_ui_controller.cc
[modify] https://crrev.com/8a6beb7ab5262f4566462d06db1dcaa00bb4522e/chrome/browser/ui/passwords/manage_passwords_ui_controller.h
[modify] https://crrev.com/8a6beb7ab5262f4566462d06db1dcaa00bb4522e/chrome/browser/ui/passwords/passwords_model_delegate.h
[modify] https://crrev.com/8a6beb7ab5262f4566462d06db1dcaa00bb4522e/chrome/browser/ui/passwords/passwords_model_delegate_mock.h
[modify] https://crrev.com/8a6beb7ab5262f4566462d06db1dcaa00bb4522e/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc

For M-63, hiding of the eye icon was implemented. For M64, I will replace hiding with re-authentication. 

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

commit e2decb889622a1178c46f1b9f9029b969525414b
Author: Vasilii Sukhanov <vasilii@chromium.org>
Date: Thu Oct 12 11:59:05 2017

Hide the eye icon in the password bubble on Mac when required.

The algorithm is listed in https://bugs.chromium.org/p/chromium/issues/detail?id=768742#c14

Bug:  768306 , 768742 
Change-Id: I137a95262ed9a328959aba61438094083364da84
Reviewed-on: https://chromium-review.googlesource.com/715739
Reviewed-by: Tatiana Gornak <melandory@chromium.org>
Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508317}
[modify] https://crrev.com/e2decb889622a1178c46f1b9f9029b969525414b/chrome/browser/ui/cocoa/passwords/save_pending_password_view_controller.mm

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

commit 77e711eb50d673ad4060e36570d81085511b2656
Author: Maxim Kolosovskiy <kolos@chromium.org>
Date: Tue Jan 09 14:00:59 2018

[Password Manager] Implement re-auth for password viewing in a prompt

Bug:  768742 
Change-Id: I9d7168b6d7e71a046f40c0cb8309c631739c0c99
Reviewed-on: https://chromium-review.googlesource.com/720979
Commit-Queue: Maxim Kolosovskiy <kolos@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527982}
[modify] https://crrev.com/77e711eb50d673ad4060e36570d81085511b2656/chrome/browser/ui/cocoa/passwords/passwords_bubble_controller_unittest.mm
[modify] https://crrev.com/77e711eb50d673ad4060e36570d81085511b2656/chrome/browser/ui/cocoa/passwords/save_pending_password_view_controller.mm
[modify] https://crrev.com/77e711eb50d673ad4060e36570d81085511b2656/chrome/browser/ui/passwords/manage_passwords_bubble_model.cc
[modify] https://crrev.com/77e711eb50d673ad4060e36570d81085511b2656/chrome/browser/ui/passwords/manage_passwords_bubble_model.h
[modify] https://crrev.com/77e711eb50d673ad4060e36570d81085511b2656/chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc
[modify] https://crrev.com/77e711eb50d673ad4060e36570d81085511b2656/chrome/browser/ui/passwords/manage_passwords_ui_controller.cc
[modify] https://crrev.com/77e711eb50d673ad4060e36570d81085511b2656/chrome/browser/ui/passwords/manage_passwords_ui_controller.h
[modify] https://crrev.com/77e711eb50d673ad4060e36570d81085511b2656/chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc
[modify] https://crrev.com/77e711eb50d673ad4060e36570d81085511b2656/chrome/browser/ui/passwords/passwords_model_delegate.h
[modify] https://crrev.com/77e711eb50d673ad4060e36570d81085511b2656/chrome/browser/ui/passwords/passwords_model_delegate_mock.h
[modify] https://crrev.com/77e711eb50d673ad4060e36570d81085511b2656/chrome/browser/ui/views/passwords/manage_password_items_view.cc
[modify] https://crrev.com/77e711eb50d673ad4060e36570d81085511b2656/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc