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

Change PasswordTextfield and PasswordInput to check for password reveal permission #7202

Closed

Conversation

wmontwe
Copy link
Member

@wmontwe wmontwe commented Sep 27, 2023

This adds a permission check to reveal the password to the PasswordInput and PasswordOutlinedTextField. It's default is no allowance, so consumers needed to adapt.

Also added missing colors to the catalog app.

This will conflict with #7195 and either this or the other needs to be patched after merge.

@wmontwe wmontwe force-pushed the change_password_textfield_to_check_password_reveal branch from 2160dd7 to f54557e Compare September 27, 2023 16:16
@wmontwe wmontwe requested a review from cketti September 27, 2023 16:19
@wmontwe wmontwe force-pushed the change_password_textfield_to_check_password_reveal branch from f54557e to 312cebc Compare September 27, 2023 16:22
@wmontwe wmontwe changed the title Change password textfield to check password reveal Change PasswordTextfield and PasswordInput to check for password reveal permission Sep 27, 2023
@cketti
Copy link
Member

cketti commented Sep 28, 2023

We want to re-implement an existing feature that only reveals a saved password when the user has authenticated. However, the existing code does quite a bit of work to avoid users having to authenticate when the original password has been removed or replaced completely. It also marks the window as "secure" when the password is unmasked, and removes the flag when the password is masked again. That's to prevent the unmasked password from showing up in screenshots.

It doesn't look like this change makes TextFieldOutlinedPassword flexible enough to implement all of the details of that feature. I think a better approach would be to copy TextFieldOutlinedPassword and make it do what we actually need directly (instead of adding optional callbacks until we can implement the feature in a higher layer). That way TextFieldOutlinedPassword doesn't get needlessly complex and more generic than we need it to be.

Side note: I was confused by the word permission at first. I think we should only use it when referring to Android system permissions (or other access control systems). I don't think it's the right word to use in this case.

@wmontwe
Copy link
Member Author

wmontwe commented Sep 29, 2023

Some of the existing functionality requires an Activity and I didn't like to add it to the password input. But if you think that's not an issue, I could look into it. The current implementation just adds a callback to let the consumer decide what kind of allowance is necessary to reveal the password. And I agree the naming is not ideal.

The addition of the warning message is also a little bit overkill and could be delegated to the consumer.

@wmontwe
Copy link
Member Author

wmontwe commented Oct 5, 2023

Any update on this or should I close it.

@wmontwe
Copy link
Member Author

wmontwe commented Oct 11, 2023

This is superseeded by #7227

@wmontwe wmontwe closed this Oct 11, 2023
@cketti cketti deleted the change_password_textfield_to_check_password_reveal branch September 30, 2024 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants