Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes white-label with auto password #18453

Closed
wants to merge 5 commits into from
Closed

Conversation

peterhashair
Copy link
Contributor

@peterhashair peterhashair commented Dec 6, 2021

Description:

Fixes white-label, when modal open password manager autofill the field in the back, update to read-only fields on modal opens. Revert to normal when modal closed.
https://innocraft.atlassian.net/jira/software/projects/L3/boards/20?label=Core&selectedIssue=L3-161

Review

add modal background field readonly
@peterhashair peterhashair marked this pull request as ready for review December 9, 2021 21:15
@peterhashair peterhashair added the Needs Review PRs that need a code review label Dec 9, 2021
Comment on lines 69 to 77
$('.modal.open #currentUserPassword').focus();
$('.modal.open #currentUserPassword').off('keypress').keypress(onEnter);
$('.pluginSettings input').prop("readonly",true);
$('.confirm-password-modal input').prop("readonly",false);
},
onCloseStart: function () {
$('.pluginSettings input').prop('readonly', false);
},
}).modal('open');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if that will really solve the issue. I guess the readonly attributes are set to prevent password managers from filling in the field. On the one side, it might actually be too late to set that attribute when opening the modal. The HTML is already placed earlier and a password manager could already fill the field. On the other side I doubt that password managers will all respect the readonly attribute. There should also be a autocomplete=off, which is already ignore by certain password managers, so why should they respect other attributes?
I guess the cleanest solution would be to disconnect the password input field from angular and set the passwordConfirmation property to the input fields value once the modal is closed. That way the modal should always be shown, even if it is already filled.

@sgiehl sgiehl added this to the 4.7.0 milestone Dec 10, 2021
@peterhashair
Copy link
Contributor Author

peterhashair commented Dec 13, 2021

@sgiehl I did a screen capture on that one, it seems like autocomplete=off didn't prevent the autofill, but the read-only does, for Bitwarden and another third part password. See the screen capture.

screen-capture.mp4

@sgiehl
Copy link
Member

sgiehl commented Dec 13, 2021

Yes. It works for my password manager too. But I actually can only repeat my last comment. It works for the tested password managers. But as soon as any password manager is able to set a value for that field it will break again.
Btw. disconnecting the password input from angular would even solve another issue: When the modal is currently opened and you type something into the password field, but click No, the modal will be closed without clearing the password field. Due to this the next click on the save button, will simply use the password typed before, without opening the modal again.

@sgiehl sgiehl removed the Needs Review PRs that need a code review label Dec 14, 2021
Peter Zhang added 2 commits December 15, 2021 17:11
@peterhashair
Copy link
Contributor Author

@sgiehl did a simple approach, hopefully that disconnect the password input

@sgiehl
Copy link
Member

sgiehl commented Dec 15, 2021

@peterhashair That wasn't what I had in my mind, also it actually changes the behavior. Before the password confirm was only shown when saving some admin configuration. Now it is also shown for user configuration. What I had in mind, was removing the ng-model from the password field and instead fetch/set the value with javascript when it's needed.

disconnect password
@peterhashair
Copy link
Contributor Author

peterhashair commented Dec 16, 2021

@sgiehl update its discount password field. I notice this is in the process of converting to VUE on this PR #18432, did a VUE version as well which is here #18515, I probably can drop this PR.

@sgiehl
Copy link
Member

sgiehl commented Dec 17, 2021

Yep. closing this one in favor of the vue PR.

@sgiehl sgiehl closed this Dec 17, 2021
@sgiehl sgiehl added the duplicate For issues that already existed in our issue tracker and were reported previously. label Dec 17, 2021
@sgiehl sgiehl deleted the white-label branch December 17, 2021 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate For issues that already existed in our issue tracker and were reported previously.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants