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.

Sign up for GitHub

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

Only create backup codes when user initiated #296

Merged
merged 4 commits into from
Sep 2, 2024
Merged

Only create backup codes when user initiated #296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
695fb09
Require explicit action to create backup codes
adamwoodnz Aug 14, 2024
fa3d768
Stop rendering code actions before codes are available
adamwoodnz Aug 14, 2024
0e1cea8
Make code generation button more descriptive of the action
adamwoodnz Aug 28, 2024
f5a9c25
Fix TOTP success navigation
adamwoodnz Aug 28, 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
115 changes: 56 additions & 59 deletions settings/src/components/backup-codes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,9 @@
*/
export default function BackupCodes( { onSuccess = () => {} } ) {
const {
user: { backupCodesEnabled, hasPrimaryProvider, backupCodesRemaining },
backupCodesVerified,
user: { hasPrimaryProvider },
} = useContext( GlobalContext );
const [ regenerating, setRegenerating ] = useState( false );
const [ generating, setGenerating ] = useState( false );

// Prevent users from accessing directly through the URL.
if ( ! hasPrimaryProvider ) {
Expand All @@ -41,26 +40,21 @@
);
}

if (
! backupCodesEnabled ||
backupCodesRemaining === 0 ||
regenerating ||
! backupCodesVerified
Comment on lines -45 to -48
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are the conditions which led to the codes being generated on load

Copy link
Contributor

Choose a reason for hiding this comment

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

We currently auto navigate to the backup code component when a user sets up a one time password. We'll also do that in the onboarding flow and add that support for Webauthn. Now that this doesn't auto trigger, do you think we should export the "Setup" component and handle that in those flows? Alternatively we could pass a prop into this component. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I wasn't aware that they need to be auto generated in those cases. Yeah passing a prop seems reasonable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I'm confused about this. We can still auto navigate to backup codes, but my understanding is that we still should not auto generate. Is it a requirement that codes are generated if one of the 2FA methods has been activated? If it is then auto generation still makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not a requirement per se, but we want to limit the number of accounts that don't have recovery codes as much as possible seeing that regaining access to a locked out account can be troublesome for users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I guess it's a balance between acting on their account without their action vs ensuring they don't get locked out. I'll add the auto-generation so we can see what it looks like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reading through #507, and in particular @dd32's comment, I don't think auto generating is the best path here. If we're going to auto generate it feels like we should only display the codes, and require a second action to save. I've updated the button text in this PR to be more explicit about the action taken if they do choose to generate. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's fair. I just tested this. It does help to establish context.

We can monitor whether users bail on this view and adjust as necessary.

) {
return <Setup setRegenerating={ setRegenerating } onSuccess={ onSuccess } />;
}

return <Manage setRegenerating={ setRegenerating } />;
return generating ? (
<Setup setGenerating={ setGenerating } onSuccess={ onSuccess } />
) : (
<Manage setGenerating={ setGenerating } />
);
}

/**
* Setup the Backup Codes provider.
*
* @param props
* @param props.setRegenerating
* @param props.setGenerating
* @param props.onSuccess
*/
function Setup( { setRegenerating, onSuccess } ) {
function Setup( { setGenerating, onSuccess } ) {
const {
setGlobalNotice,
user: { userRecord },
Expand Down Expand Up @@ -102,26 +96,22 @@
};

generateCodes();
}, [] );

Check warning on line 99 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 102 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 );
setGlobalNotice( 'Backup codes have been enabled.' );
setRegenerating( false );
setGenerating( false );
onSuccess();
} );

return (
<>
<div className="wporg-2fa__screen-intro">
<p>
Backup codes let you access your account if your primary two-factor
authentication method is unavailable, like if your phone is lost or stolen. Each
code can only be used once.
</p>
<IntroText />

<p>Please print the codes and keep them in a safe place.</p>

Expand All @@ -142,12 +132,6 @@
<>
<CodeList codes={ backupCodes } />

<ButtonGroup>
<CopyToClipboardButton contents={ backupCodes } />
<PrintButton />
<DownloadButton codes={ backupCodes } />
</ButtonGroup>

<CheckboxControl
label="I have printed or saved these codes"
checked={ hasPrinted }
Expand All @@ -172,63 +156,74 @@
}

/**
* Display a list of backup codes
* Display a list of backup codes and actions
*
* @param props
* @param props.codes
*/
function CodeList( { codes } ) {
return (
<div className="wporg-2fa__backup-codes-list">
{ ! codes.length && (
<p>
Generating backup codes...
<Spinner />
</p>
) }
const hasCodes = !! codes.length;

{ codes.length > 0 && (
<ol>
{ codes.map( ( code ) => {
return (
<li key={ code } className="wporg-2fa__token">
{ code.slice( 0, 4 ) + ' ' + code.slice( 4 ) }
</li>
);
} ) }
</ol>
return (
<>
<div className="wporg-2fa__backup-codes-list">
{ hasCodes ? (
<ol>
{ codes.map( ( code ) => {
return (
<li key={ code } className="wporg-2fa__token">
{ code.slice( 0, 4 ) + ' ' + code.slice( 4 ) }
</li>
);
} ) }
</ol>
) : (
<p>
<Spinner /> Generating backup codes...
</p>
) }
</div>
{ hasCodes && (
<ButtonGroup>
<CopyToClipboardButton contents={ codes } />
<PrintButton />
<DownloadButton codes={ codes } />
</ButtonGroup>
) }
</div>
</>
);
}

const IntroText = () => (
<p>
Backup codes let you access your account if your primary two-factor authentication method is
unavailable, like if your phone is lost or stolen. Each code can only be used once.
</p>
);

/**
* Render the screen where users can manage Backup Codes.
*
* @param props
* @param props.setRegenerating
* @param props.setGenerating
*/
function Manage( { setRegenerating } ) {
function Manage( { setGenerating } ) {
const {
user: { backupCodesRemaining },
user: { backupCodesEnabled, backupCodesRemaining },
} = useContext( GlobalContext );

return (
<>
<div className="wporg-2fa__screen-intro">
<p>
Backup codes let you access your account if your primary two-factor
authentication method is unavailable, like if your phone is lost or stolen. Each
code can only be used once.
</p>
<IntroText />

{ backupCodesRemaining > 5 && (
{ backupCodesEnabled && backupCodesRemaining > 5 && (
<p>
You have <strong>{ backupCodesRemaining }</strong> backup codes remaining.
</p>
) }

{ backupCodesRemaining <= 5 && (
{ backupCodesEnabled && backupCodesRemaining <= 5 && (
<Notice status="warning" isDismissible={ false }>
<Icon icon={ warning } />
<div>
Expand All @@ -241,8 +236,10 @@
) }
</div>

<Button isSecondary onClick={ () => setRegenerating( true ) }>
Generate new backup codes
<Button isSecondary onClick={ () => setGenerating( true ) }>
{ backupCodesEnabled
? 'Regenerate and save backup codes'
: 'Generate and save backup codes' }
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand why we want to add "Generate and save backup codes" because that's what technically happens but I don't know that first time users will understand what save means here. I could be wrong.

</Button>
</>
);
Expand Down
6 changes: 5 additions & 1 deletion settings/src/components/settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,11 @@ import { GlobalContext } from '../script';
* Render the correct component based on the URL.
*/
export default function Settings() {
const { backupCodesEnabled, navigateToScreen, screen } = useContext( GlobalContext );
const {
user: { backupCodesEnabled },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was undefined. Sometimes I wish we had TS for things like this.

navigateToScreen,
screen,
} = useContext( GlobalContext );

// The index is the URL slug and the value is the React component.
const components = {
Expand Down
Loading