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 a SVN Password functionality #280

Merged
merged 44 commits into from
Aug 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
902f846
Initial run at adding a SVN Password functionality.
dd32 Jul 4, 2024
414dbe4
Linting and alerts
dd32 Jul 4, 2024
f2135a6
Add some TODOs
dd32 Jul 8, 2024
4d9ecf7
Remove the custom code to check for plugin committer / theme author s…
dd32 Jul 19, 2024
e5c853c
Use WordPress.org provided functions to set / check for a password in…
dd32 Jul 19, 2024
e26e188
Merge branch 'trunk' into add/svn-password
dd32 Jul 19, 2024
6c22e27
typo
dd32 Jul 19, 2024
20a862a
Linting fixes.
dd32 Jul 19, 2024
b441556
Remove nested terninaries
dd32 Jul 19, 2024
fdd7689
Switch to a dedicated rest api endpoint to generate the SVN password.
dd32 Jul 19, 2024
9b27770
Require 2FA validation to enter the SVN password screen.
dd32 Jul 19, 2024
a1ea2a2
Linting.
dd32 Jul 19, 2024
b3e42cd
Switch to 'Generate Password' rather than Request.
dd32 Aug 7, 2024
fb5de98
JSX
dd32 Aug 7, 2024
c19e325
Simplify description of the Account Overview SVN password tab.
dd32 Aug 7, 2024
762a291
Remove "forget password", these passwords are random and cannot be re…
dd32 Aug 7, 2024
f9dc398
The SVN password is required to commit
dd32 Aug 7, 2024
c070587
Merge branch 'trunk' into add/svn-password
dd32 Aug 22, 2024
805e8ba
Switch to using the Copy to clipboard component.
dd32 Aug 22, 2024
21b873e
Temporarily limit access to the SVN Password interface to beta users …
dd32 Aug 22, 2024
f757d2d
Linting: remove unused function.
dd32 Aug 22, 2024
98fcc52
Add some verbose about subversion text.
dd32 Aug 22, 2024
412ebf5
Expand upon the text, combine paragraphs, list the case-sensitive use…
dd32 Aug 22, 2024
5041a59
When the SVN password functions aren't available, just return that th…
dd32 Aug 23, 2024
4f358fc
Updated interface design
dd32 Aug 23, 2024
ec1a77d
Add a case-sensitive remark when a username is not all lowercase.
dd32 Aug 23, 2024
41c06dd
Use a list, add spacing.
dd32 Aug 23, 2024
9cc14d2
Convert the Copy-to-clipboard button into a link.
dd32 Aug 23, 2024
c03940b
Add the Generation date
dd32 Aug 23, 2024
744e047
Fill in the creation date to avoid a lag in the UI updating.
dd32 Aug 23, 2024
c283b82
Update the account overview text.
dd32 Aug 23, 2024
e94d95e
Merge branch 'trunk' into add/svn-password
dd32 Aug 27, 2024
1509ca9
Merge branch 'trunk' into add/svn-password
dd32 Aug 28, 2024
748ed28
Add action container to the svn button to match other views.
StevenDufresne Aug 28, 2024
55727ac
Update copy for svn blurb.
StevenDufresne Aug 28, 2024
b923ff5
Add copy intro class to maintain same spacing with other views
StevenDufresne Aug 28, 2024
668f0dd
Harmonize security keys and svn password detail styles.
StevenDufresne Aug 29, 2024
1542311
Add more contextual language to submit button.
StevenDufresne Aug 29, 2024
56663ac
Merge trunk.
StevenDufresne Aug 29, 2024
5305a81
Update copy to SVN Credentials.
StevenDufresne Aug 29, 2024
e37ac17
Change props for CopyToClipboard button since it's reused here.
StevenDufresne Aug 29, 2024
ad8dfc1
Fix linter issue.
StevenDufresne Aug 29, 2024
82919f3
Lowercase status text. That's the new standard.
StevenDufresne Aug 29, 2024
8d9d684
Enable the UI for all SVN-related users.
dd32 Aug 29, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion settings/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
"@wordpress/core-data": "^6.4.0",
"@wordpress/data": "^8.4.0",
"@wordpress/element": "^5.4.0",
"@wordpress/icons": "^9.18.0",
"@wordpress/icons": "^9.49.0",
"@wordpress/scripts": "^27.6.0",
"lodash": "^4.17.21"
}
Expand Down
89 changes: 89 additions & 0 deletions settings/rest-api.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
use Two_Factor_Core, Two_Factor_Totp, Two_Factor_Backup_Codes;
use WildWolf\WordPress\TwoFactorWebAuthn\{ WebAuthn_Credential_Store };
use WP_REST_Server, WP_REST_Request, WP_Error, WP_User;
use function WordPressdotorg\Security\SVNPasswords\{ set_svn_password, get_svn_password_creation_date };

defined( 'WPINC' ) || die();

Expand Down Expand Up @@ -131,6 +132,39 @@ function register_rest_routes() : void {
),
),
);

register_rest_route(
'wporg-two-factor/1.0',
'/generate-svn-password',
array(
'methods' => WP_REST_Server::EDITABLE,
'callback' => function( $request ) {
$user = get_userdata( $request['user_id'] );

// Local environment doesn't have the SVN password system, just mock it.
if ( ! function_exists( 'WordPressdotorg\Security\SVNPasswords\set_svn_password' ) ) {
return 'Local Development: SVN Password system unavailable.';
}

return [
'svn_password' => set_svn_password( $user->ID )
];
},
'permission_callback' => function( $request ) {
return Two_Factor_Core::rest_api_can_edit_user_and_update_two_factor_options( $request['user_id'] );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
'args' => array(
'user_id' => array(
'required' => true,
'type' => 'number',
'sanitize_callback' => 'absint',
'validate_callback' => function( $user_id ) {
return get_userdata( $user_id ) instanceof WP_User;
},
),
Comment on lines +157 to +164
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we allow user impersonation in WP.org? If not, maybe we can just use the current user id?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

),
),
);
}

/**
Expand Down Expand Up @@ -323,6 +357,61 @@ function register_user_fields(): void {
]
);

register_rest_field(
'user',
'svn_password_required',
[
'get_callback' => function( $user ) {
global $wpdb;

$user = get_userdata( $user['id'] );
if ( ! $user ) {
return false;
}

// Committers, supes, etc. It's likely these users will need a SVN password.
if ( function_exists( 'is_special_user' ) && is_special_user( $user->ID ) ) {
return true;
}

// Plugin committers & Theme authors have this user meta set.
if ( $user->has_plugins || $user->has_themes ) {
return true;
}

return false;
},
'schema' => [
'type' => 'boolean',
'context' => [ 'edit' ],
]
]
);
dd32 marked this conversation as resolved.
Show resolved Hide resolved

register_rest_field(
'user',
'svn_password_created',
[
'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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

$svn_password_created_date = get_svn_password_creation_date( $user['id'] );
if ( ! $svn_password_created_date ) {
return false;
}

return $svn_password_created_date;
},
'schema' => [
'type' => [ 'boolean', 'string' ],
'context' => [ 'edit' ],
]
]
);

}

/**
Expand Down
24 changes: 23 additions & 1 deletion settings/src/components/account-status.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,12 @@ export default function AccountStatus() {
const {
user: {
userRecord: {
record: { email, pending_email: pendingEmail },
record: {
email,
pending_email: pendingEmail,
svn_password_created: svnPasswordSet,
svn_password_required: svnPasswordRequired,
},
},
hasPrimaryProvider,
primaryProvider,
Expand Down Expand Up @@ -89,6 +94,23 @@ export default function AccountStatus() {
bodyText={ backupBodyText }
disabled={ ! hasPrimaryProvider }
/>

{ svnPasswordRequired || svnPasswordSet ? (
<SettingStatusCard
screen="svn-password"
status={
! svnPasswordRequired && ! svnPasswordSet ? 'info' : !! svnPasswordSet
}
headerText="SVN credentials"
bodyText={
! svnPasswordSet
? 'You have not configured a SVN password for your account.'
: "You've got a SVN password configured for your account."
}
/>
) : (
''
) }
</div>
);
}
Expand Down
2 changes: 1 addition & 1 deletion settings/src/components/backup-codes.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,10 +102,10 @@
};

generateCodes();
}, [] );

Check warning on line 105 in settings/src/components/backup-codes.js

View workflow job for this annotation

GitHub Actions / All

React Hook useEffect has missing dependencies: 'setBackupCodesVerified', 'setError', and 'userRecord'. Either include them or remove the dependency array

// Finish the setup process.
const handleFinished = useCallback( async () => {

Check warning on line 108 in settings/src/components/backup-codes.js

View workflow job for this annotation

GitHub Actions / All

React Hook useCallback does nothing when called with only one argument. Did you forget to pass an array of dependencies?
// TODO: Add try catch here after https://github.com/WordPress/wporg-two-factor/pull/187/files is merged.
// The codes have already been saved to usermeta, see `generateCodes()` above.
setBackupCodesVerified( true );
Expand Down Expand Up @@ -143,7 +143,7 @@
<CodeList codes={ backupCodes } />

<ButtonGroup>
<CopyToClipboardButton codes={ backupCodes } />
<CopyToClipboardButton contents={ backupCodes } />
<PrintButton />
<DownloadButton codes={ backupCodes } />
</ButtonGroup>
Expand Down
8 changes: 4 additions & 4 deletions settings/src/components/copy-to-clipboard-button.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,18 @@
import { useCallback, useState } from '@wordpress/element';
import { Button } from '@wordpress/components';

export default function CopyToClipboardButton( { codes } ) {
export default function CopyToClipboardButton( { contents, variant = 'secondary' } ) {
const [ copied, setCopied ] = useState( false );

const onClick = useCallback( () => {
navigator.clipboard.writeText( codes ).then( () => {
navigator.clipboard.writeText( contents ).then( () => {
setCopied( true );
setTimeout( () => setCopied( false ), 2000 );
} );
}, [ codes ] );
}, [ contents ] );

return (
<Button variant="secondary" onClick={ onClick }>
<Button variant={ variant } onClick={ onClick }>
{ copied ? 'Copied!' : 'Copy' }
</Button>
);
Expand Down
5 changes: 5 additions & 0 deletions settings/src/components/settings.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import EmailAddress from './email-address';
import TOTP from './totp';
import WebAuthn from './webauthn/webauthn';
import BackupCodes from './backup-codes';
import SVNPassword from './svn-password';

import { GlobalContext } from '../script';

Expand Down Expand Up @@ -62,6 +63,10 @@ export default function Settings() {
/>
),
},
'svn-password': {
title: 'SVN credentials',
component: <SVNPassword />,
},
};

const currentScreenComponent =
Expand Down
127 changes: 127 additions & 0 deletions settings/src/components/svn-password.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
/**
* WordPress dependencies
*/
import apiFetch from '@wordpress/api-fetch';
import { Button } from '@wordpress/components';
import { useCallback, useContext, useMemo, useState } from '@wordpress/element';
import { refreshRecord } from '../utilities/common';
import CopyToClipboardButton from './copy-to-clipboard-button';

/**
* Internal dependencies
*/
import { GlobalContext } from '../script';

/**
* Render the Email setting.
*/
export default function SVNPassword() {
const {
user: { userRecord },
setError,
} = useContext( GlobalContext );

const [ isGenerating, setGenerating ] = useState( false );
const [ generatedPassword, setGeneratedPassword ] = useState( '' );

// Generate a new SVN Password.
const handleGenerate = useCallback( async () => {
try {
setGenerating( true );

const response = await apiFetch( {
path: '/wporg-two-factor/1.0/generate-svn-password',
method: 'POST',
data: {
user_id: userRecord.record.id,
},
} );

// Fill in the creation date in the user record, we'll refresh it for the actual data below.
userRecord.record.svn_password_created = new Date().toISOString();

setGeneratedPassword( response.svn_password );
setGenerating( false );

await refreshRecord( userRecord );
} catch ( apiFetchError ) {
setError( apiFetchError );
}
} );

const getButtonText = useMemo( () => {
if ( isGenerating ) {
return 'Generating...';
}

if ( ! userRecord.record.svn_password_created ) {
return 'Generate Password';
}

return 'Regenerate Password';
}, [ isGenerating, userRecord.record.svn_password_created ] );

return (
<>
<p>
WordPress.org uses Subversion (SVN) for version control, providing each hosted
plugin and theme with a repository that the author can commit to. For information on
using SVN, please see the{ ' ' }
<a href="https://developer.wordpress.org/plugins/wordpress-org/how-to-use-subversion/">
WordPress.org Plugin Developer Handbook
</a>
.
</p>

<p className="wporg-2fa__screen-intro">
For security, your WordPress.org account password should not be used to commit to
SVN, use a separate SVN password, which you can generate here.
</p>

<h4>Details</h4>
<ul>
<li>
Username: <code>{ userRecord.record.username }</code>{ ' ' }
{ userRecord.record.username.match( /[^a-z0-9]/ ) && <>(case-sensitive)</> }
</li>
<li>
Password:{ ' ' }
{ generatedPassword || userRecord.record.svn_password_created ? (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the generatedPassword check necessary? wouldn't userRecord.record.svn_password_created be set if a password was generated?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<>
<code>{ generatedPassword || 'svn_*****************' }</code>
&nbsp;
{ generatedPassword && (
<CopyToClipboardButton
variant="link"
contents={ generatedPassword }
/>
) }
{ userRecord.record.svn_password_created && (
<div className="wporg-2fa__svn-password_generated">
Generated on{ ' ' }
{ new Date(
userRecord.record.svn_password_created
).toLocaleDateString() }
</div>
) }
</>
) : (
<>
<em>Not configured</em>
</>
) }
</li>
</ul>
<div className="wporg-2fa__submit-actions">
<Button
variant="secondary"
onClick={ handleGenerate }
isBusy={ isGenerating }
disabled={ isGenerating }
>
{ getButtonText }
</Button>
</div>
</>
);
}
26 changes: 26 additions & 0 deletions settings/src/components/svn-password.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
.wporg-2fa__svn-password {
code {
display: inline-block;
padding: 0 3px;
background: var(--wp--preset--color--light-grey-2, #f6f6f6);
border-radius: 2px;
}

ul {
padding-top: 16px;

li:not(:last-child) {
margin-bottom: 8px;

.wporg-2fa__svn-password_generated {
color: var(--wp--preset--color--charcoal-4, #656a71);
}
}
}

.wporg-2fa__svn-password_generated {
padding-top: 4px;
font-size: 12px;
color: var(--wp--preset--color--charcoal-4, #656a71);
}
}
dd32 marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 1 addition & 1 deletion settings/src/components/webauthn/list-keys.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ export default function ListKeys() {

return (
<>
<h4 className="wporg-2fa__webauthn-keys-header">Security Keys</h4>
<h4>Security Keys</h4>
<ul className="wporg-2fa__webauthn-keys-list">
{ keys.map( ( key ) => (
<li key={ key.id }>
Expand Down
5 changes: 0 additions & 5 deletions settings/src/components/webauthn/webauthn.scss
Original file line number Diff line number Diff line change
@@ -1,10 +1,5 @@
.wporg-2fa__webauthn,
.bbp-single-user .wporg-2fa__webauthn {
.wporg-2fa__webauthn-keys-header {
margin-bottom: 0;
font-weight: 600;
}

.wporg-2fa__webauthn-keys-list {
li {
display: flex;
Expand Down
2 changes: 1 addition & 1 deletion settings/src/script.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ function Main( { userId, isOnboarding } ) {
const [ screen, setScreen ] = useState( initialScreen === null ? 'home' : initialScreen );

// The screens where a recent two factor challenge is required.
const twoFactorRequiredScreens = [ 'webauthn', 'totp', 'backup-codes' ];
const twoFactorRequiredScreens = [ 'webauthn', 'totp', 'backup-codes', 'svn-password' ];

// Listen for back/forward button clicks.
useEffect( () => {
Expand Down
Loading
Loading