WordPress / wporg-two-factor

2FA for WordPress.org accounts
38 stars 8 forks source link

Add a SVN Password functionality #280

Closed dd32 closed 2 months ago

dd32 commented 4 months ago

This is a user_pass but specific for SVN, in it's own table.

This is in place of an application password, fixes #276.

The option only shows when the user requires SVN access (or has a password set), based on user access.

Overview, not set. Overview, SVN password set
Screenshot 2024-07-04 at 4 41 29 PM Screenshot 2024-07-04 at 4 41 53 PM
Screen After password is generated
Screenshot 2024-07-04 at 4 41 37 PM Screenshot 2024-07-04 at 4 41 46 PM

TODO:

StevenDufresne commented 3 months ago

Regarding terminology, "SVN Password" makes me think that as a plugin committer, my wp.org account and SVN commit passwords are different.

Provided this isn't the only way to commit in the future (which I'm pretty sure it isn't), have we considered other options like:

I think avoiding "password" altogether would make for simpler documentation.

dd32 commented 3 months ago

Regarding terminology, "SVN Password" makes me think that as a plugin committer, my wp.org account and SVN commit passwords are different.

That's the intention here, they will be.

Provided this isn't the only way to commit in the future (which I'm pretty sure it isn't), have we considered other options like:

This will be the only manner to authenticate against SVN. Their account password will cease to work.

This specifically does not call it a 'Application Password' as it cannot be used elsewhere in WordPress where an Application password can be used. I didn't call it a token or key because SVN asks for a Password (Since it only does Username + Password auth).

dd32 commented 3 months ago

I noticed that we're no longer showing the username on the profile page, which is problematic for SVN as it uses a case-sensitive username.. so I've shoehorned that into this text too.

Latest iteration:

Screenshot 2024-08-22 at 5 50 58 PM
dd32 commented 3 months ago

Also a note; If you test this UI and regenerate the password multiple times, the Copy button stays present in the notice when the password vanishes. Conditionally showing the copy button component based on a conditional caused react to react badly.. in my experience..

StevenDufresne commented 3 months ago

I noticed that we're no longer showing the username on the profile page, which is problematic for SVN as it uses a case-sensitive username.. so I've shoehorned that into this text too.

You are right. It's also mentioned here. Should we re-add it to the profile page?

dd32 commented 3 months ago

You are right. It's also mentioned here. Should we re-add it to the profile page?

I'm thinking since it's only relevant to SVN, we can probably keep it on the SVN page.

That's lead me to questioning the design and layout of the screen, how about something like this instead?

Screenshot 2024-08-23 at 12 08 29 PM Screenshot 2024-08-23 at 12 09 07 PM
StevenDufresne commented 3 months ago

I'm thinking since it's only relevant to SVN, we can probably keep it on the SVN page.

That's lead me to questioning the design and layout of the screen, how about something like this instead?

Yeah I think those are good additions, letting the user know that there was a password configured in this view which is currently missing. Here's what I came up with based on your designs using some of the existing UI patterns. I think we can remove some of the copy when a password is configured.

Screenshot 2024-08-23 at 11 49 25 AM Screenshot 2024-08-23 at 12 03 15 PM
dd32 commented 3 months ago

I don't personally see the harm in leaving some of the basic boilerplate text there, how's this?

Current status:

https://github.com/user-attachments/assets/4fe96a3e-5825-486d-957b-62776ce5dece

StevenDufresne commented 2 months ago

@dd32 I've added some commits, mostly style, language updates. Can you verify that they are ok.

The last thing on my mind is whether we should change the screen from "SVN Password" to "SVN Credentials".

dd32 commented 2 months ago

+1 for SVN Credentials.

The copy changes appear good to me. I haven't re-tested it yet.

Noting that the systems change has been made, so this can now be used in production.