Add a SVN Password functionality #280
Conversation
…tatus, and use the user_meta instead.
| 'user_id' => array( | ||
| 'required' => true, | ||
| 'type' => 'number', | ||
| 'sanitize_callback' => 'absint', | ||
| 'validate_callback' => function( $user_id ) { | ||
| return get_userdata( $user_id ) instanceof WP_User; | ||
| }, | ||
| ), |
There was a problem hiding this comment.
Do we allow user impersonation in WP.org? If not, maybe we can just use the current user id?
There was a problem hiding this comment.
We do, although not how you may have invisioned it here.
Currently a super-admin can modify another users account data, that's why user_id is passed to the various 2FA endpoints.
I'm not a super-fan of it, but it's how the other endpoints in the plugin operate, so it made sense to duplicate that here.
An example of where this IS needed, is a case of accounts that cannot be logged into, system-level automation accounts that we need a SVN password for. However, we do have the CLI commands that can be used for that.
It also allows for super-admins to be able to quickly re-generate a new svn password for a user to lock them out (although, that's not something we'd likely make use of).
Happy to change this to throw a error along the lines of You can only re-generate a password for yourself if you feel strongly.
| ]; | ||
| }, | ||
| 'permission_callback' => function( $request ) { | ||
| return Two_Factor_Core::rest_api_can_edit_user_and_update_two_factor_options( $request['user_id'] ); |
There was a problem hiding this comment.
Should we restrict this endpoint to users only need a svn password? It seems like any authenticated .org user will be able to set their own SVN password here regardless if they need it or not.
There was a problem hiding this comment.
This was intentional, as WordPress is not aware of every user that may need a SVN password, such as those who are granted directly via svn configuration files (ie. develop.svn.wordpress.org).
Leaving this open allows for direct links to the svn password screen to be used, rather than having to set some user-meta to allow access.
| 'context' => [ 'edit' ], | ||
| ] | ||
| ] | ||
| ); |
There was a problem hiding this comment.
Is this information exposed for unauthenticated users via the /wp/v2/users/* endpoints? If so, should we expose them?
There was a problem hiding this comment.
No, this is only exposed in the edit context, which is limited to authenticated users who can edit that given user; which is limited to the actual user and administrators on the given site.
| if ( svnPasswordRequired || svnPasswordSet ) { | ||
| if ( svnPasswordRequired ) { | ||
| svnBodyText = svnPasswordSet | ||
| ? 'You have an active SVN password set. You can request a new one at any time.' |
There was a problem hiding this comment.
Only reviewing wording here:
- Do we need to explicitly define like "SVN (Subversion)"?
- Could be briefer and more direct, like "A SVN password is set" (or active)
- Makes me wonder if there's such a thing as an inactive SVN password?
| } else { | ||
| svnBodyText = svnPasswordSet | ||
| ? 'You have an active SVN password set. You can request a new one at any time.' | ||
| : 'You do not currently require WordPress.org SVN access.'; |
There was a problem hiding this comment.
"You not currently require.." seems like a slightly puzzsing inclusion. Should we just not show it at all? Or grey it out?
I assume it's here for users who don't have commit access to anything.
| @@ -32,7 +32,8 @@ const ScreenNavigation = ( { screen, children } ) => ( | |||
| { screen | |||
| .replace( '-', ' ' ) | |||
| .replace( 'totp', 'Two-Factor Authentication' ) | |||
| .replace( 'webauthn', 'Two-Factor Security Key' ) } | |||
| .replace( 'webauthn', 'Two-Factor Security Key' ) | |||
There was a problem hiding this comment.
Is 'Two-Factor Security Key' the usual term? Should it mention Hardware or Physical Key in the name?
| navigator.clipboard.writeText( generatedPassword ); | ||
| setGlobalNotice( 'Copied to clipboard' ); | ||
| } catch ( error ) { | ||
| setGlobalNotice( "Couldn't write to clipboard" ); |
There was a problem hiding this comment.
Is it possible to detect whether writing to the clipboard is possible before offering a button that won't work?
I can't come up with a neat way of phrasing it off the top of my head, but we could add words to the effect of "you're going to have to type it yourself" (ie offer a solution).
There was a problem hiding this comment.
On googling: seems like there's no reliable way to detect whether or not it will work.
| return ( | ||
| <> | ||
| <p> | ||
| Your SVN password can be used to commit to WordPress.org SVN repositories, such as |
There was a problem hiding this comment.
Rather than "can be used" (which implies other possibilities), what about "is used" or "is required".
| for a plugin or theme. | ||
| </p> | ||
| <p> | ||
| If you forget your SVN password, you can generate a new one here. All previous SVN |
There was a problem hiding this comment.
Simpler to say "..you can reset it here"? And warn about invalidating old ones as part of the process.
|
|
||
| const regenerateButtonText = userRecord.record.svn_password | ||
| ? 'Regenerate password' | ||
| : 'Request password'; |
There was a problem hiding this comment.
Should this be simply Generate password? Request password feels like there could be delay in me receiving the password or some type of application process to complete.
| { isGenerating ? ( | ||
| <> | ||
| <Spinner /> | ||
| Requesting.. |
There was a problem hiding this comment.
If we agree on "Generate password", this should be "Generating...".
|
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. |
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.
TODO: