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

Add dynamic data/interactions to Password component #33

Merged
merged 10 commits into from
Jan 5, 2023

Conversation

iandunn
Copy link
Member

@iandunn iandunn commented Dec 29, 2022

Fixes #27

@iandunn iandunn requested a review from dd32 December 29, 2022 23:01
@iandunn iandunn self-assigned this Dec 29, 2022
@iandunn iandunn force-pushed the password-functionality branch from 8b48935 to d95cb3d Compare December 30, 2022 22:26
@xknown
Copy link
Member

xknown commented Jan 4, 2023

Out of curiosity, why are doing this on the client side? Do we have the same checks on the backend that block weak or compromised passwords?

@iandunn
Copy link
Member Author

iandunn commented Jan 4, 2023

Building a custom UI was one of the requirements of the project, since the design of the wp-admin UI in the upstream plugin isn't modern enough.

I did initially use the generate-password AJAX endpoint to generate the password, but we decided to switch to doing it in the browser for performance/UX.

The client does check check the strength, and blocks saving when it's too weak. It's checked with zxcvbn, and we
require a higher entropy score for folks with elevated privileges. zxcvbn is somewhat outdated, but it's what Core still uses. If you feel like that's not good enough, I'd be open to swapping in a different one.

The REST endpoint doesn't check the strength when saving, so somebody could intentionally set a weak password if they really wanted to. But, we do have some backend code in a separate plugin that monitors for weak/compromised passwords, and would force them to reset it.

@xknown
Copy link
Member

xknown commented Jan 5, 2023

The client does check check the strength, and blocks saving when it's too weak. It's checked with zxcvbn, and we
require a higher entropy score for folks with elevated privileges. zxcvbn is somewhat outdated, but it's what Core still uses. If you feel like that's not good enough, I'd be open to swapping in a different one.

The REST endpoint doesn't check the strength when saving, so somebody could intentionally set a weak password if they really wanted to. But, we do have some backend code in a separate plugin that monitors for weak/compromised passwords, and would force them to reset it.

While I understand the motivations of doing this only client side, I think it'd be good to also enforce this on the backend, especially if we have already existing code to check for compromised/weak passwords -- it wouldn't be a good UX to set a password via this endpoint, and then later be blocked because it was considered weak or compromised somewhere else. Also, doing this only client side will most likely be reported in our bounty program :)

@iandunn
Copy link
Member Author

iandunn commented Jan 5, 2023

👍🏻 , I added backend validation to the endpoint in r19805-dotorg.

@dd32
Copy link
Member

dd32 commented Jan 6, 2023

While I understand the motivations of doing this only client side, I think it'd be good to also enforce this on the backend,

I added backend validation to the endpoint in r19805-dotorg.

It's worth noting that WordPress core / wp-admin doesn't do this, you can set any password as all the validation is client-side. As a result, we'll have to disable the admin UIs, I've opened #37 to track that.

@iandunn iandunn added this to the MVP milestone Feb 22, 2023
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.

UI: Password
3 participants