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.
| 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?
There was a problem hiding this comment.
I kind of hope that those who need SVN access would know what SVN is, but, it's easy so why not?
Makes me wonder if there's such a thing as an inactive SVN password?
There isn't, currently. An 'inactive SVN password' would be an account that doesn't have a svn password set, or that we expired it (some time in the future).
There was a problem hiding this comment.
Noting the text was simplified significantly.
Do we need to explicitly define like "SVN (Subversion)"?
I wonder if people know it more as SVN or Subversion.. I actually suspect it's more the former in the WordPress sphere.
looking at other WordPress documentation:
- https://codex.wordpress.org/Using_Subversion doesn't call it SVN, but refers to our SVN repo's.
- https://make.wordpress.org/core/handbook/contribute/svn/ refers to 'Subversion' twice, and SVN a bunch more.
I've experimented with <abbr> and Subversion (SVN), and have settled on using SVN everywhere but calling it Subversion (SVN) in the first sentence on the page. Additionally, linking to the plugins handbook, see 98fcc52
|
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. |
That's the intention here, they will be.
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). |
…only, until the systems side is finalised.
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. 
|
|
I don't personally see the harm in leaving some of the basic boilerplate text there, how's this? Current status: Screen.Recording.2024-08-23.at.4.44.31.PM.mov |
| return ( | ||
| <> | ||
| <p> | ||
| WordPress.org uses Subversion (SVN) for version control, every Plugin and Theme |
There was a problem hiding this comment.
Can we shorten this to avoid using WordPress.org and SVN twice? It makes it a bit hard on the eyes.
WordPress.org uses Subversion (SVN) for version control, providing each hosted plugin and theme with a repository that the author can commit to.
| Before | After |
|---|---|
 |
 |
| </li> | ||
| <li> | ||
| Password:{ ' ' } | ||
| { generatedPassword || userRecord.record.svn_password_created ? ( |
There was a problem hiding this comment.
Is the generatedPassword check necessary? wouldn't userRecord.record.svn_password_created be set if a password was generated?
There was a problem hiding this comment.
Technically you're probably right, it might not be required, since we set userRecord.record.svn_password_created above manually.
In practise, I found it to be required, as without it it caused the UI to "flash" as it re-rendered the branch.
| 'get_callback' => function( $user ) { | ||
| // Local environment doesn't have the SVN password system, just return false for that. | ||
| if ( ! function_exists( 'WordPressdotorg\Security\SVNPasswords\get_svn_password_creation_date' ) ) { | ||
| return false; |
There was a problem hiding this comment.
Since this returns false, it doesn't allow us to test locally because of this check. Can we return a hard-coded date for testing?
There was a problem hiding this comment.
Yep, but not really at the same time.
You can return Dummy data here, but if you hard-code dummy data you can't then test the branch where you've not set a password.
The best option would probably be to add a local test variant of the svn password functionality that uses user meta.
|
@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". |
|
+1 for 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. |
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: