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

Fix unauthorized url access #189

Merged
merged 7 commits into from
Jun 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 12 additions & 3 deletions settings/src/components/backup-codes.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,18 @@ import { refreshRecord } from '../utilities';
*/
export default function BackupCodes() {
const {
user: { backupCodesEnabled },
user: { backupCodesEnabled, totpEnabled },
navigateToScreen,
} = useContext( GlobalContext );
const [ regenerating, setRegenerating ] = useState( false );

// If TOTP hasn't been enabled, the user should not have access to BackupCodes component.
// This is primarily added to prevent users from accessing through the URL.
if ( ! totpEnabled ) {
navigateToScreen( 'account-status' );
return;
}

if ( backupCodesEnabled && ! regenerating ) {
return <Manage setRegenerating={ setRegenerating } />;
}
Expand Down Expand Up @@ -71,9 +79,10 @@ function Setup( { setRegenerating } ) {
}, [] );

// Finish the setup process.
const handleFinished = useCallback( () => {
const handleFinished = useCallback( async () => {
// 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.
refreshRecord( userRecord ); // This has the intended side-effect of redirecting to the Manage screen.
await refreshRecord( userRecord ); // This has the intended side-effect of redirecting to the Manage screen.
setGlobalNotice( 'Backup codes have been enabled.' );
setRegenerating( false );
} );
Expand Down
19 changes: 14 additions & 5 deletions settings/src/components/revalidate-modal.js
Original file line number Diff line number Diff line change
@@ -1,16 +1,19 @@
/**
* WordPress dependencies
*/
import { useContext, useEffect, useRef } from '@wordpress/element';
import { useCallback, useContext, useEffect, useRef } from '@wordpress/element';
import { GlobalContext } from '../script';
import { Modal } from '@wordpress/components';
import { useMergeRefs, useFocusableIframe } from '@wordpress/compose';
import { refreshRecord } from '../utilities';

export default function RevalidateModal() {
const { clickScreenLink } = useContext( GlobalContext );
const { navigateToScreen } = useContext( GlobalContext );

const goBack = ( event ) => clickScreenLink( event, 'account-status' );
const goBack = useCallback( ( event ) => {
event.preventDefault();
navigateToScreen( 'account-status' );
}, [] );

return (
<Modal
Expand All @@ -37,7 +40,7 @@ function RevalidateIframe() {
const ref = useRef();

useEffect( () => {
function maybeRefreshUser( { data: { type, message } = {} } ) {
async function maybeRefreshUser( { data: { type, message } = {} } ) {
if ( type !== 'reValidationComplete' ) {
return;
}
Expand All @@ -49,7 +52,13 @@ function RevalidateIframe() {
record[ '2fa_revalidation' ].expires_at = new Date().getTime() / 1000 + 3600;

// Refresh the user record, to fetch the correct 2fa_revalidation data.
refreshRecord( userRecord );
try {
await refreshRecord( userRecord );
} catch ( error ) {
// TODO: handle error more properly here, likely by showing a error notice
// eslint-disable-next-line no-console
console.error( 'Failed to refresh user record:', error );
}
}

window.addEventListener( 'message', maybeRefreshUser );
Expand Down
11 changes: 8 additions & 3 deletions settings/src/components/screen-link.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
/**
* WordPress dependencies
*/
import { useContext } from '@wordpress/element';
import { useCallback, useContext } from '@wordpress/element';

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

export default function ScreenLink( { screen, anchorText, buttonStyle = false, ariaLabel } ) {
const { clickScreenLink } = useContext( GlobalContext );
const { navigateToScreen } = useContext( GlobalContext );
const classes = [];
const screenUrl = new URL( document.location.href );

Expand All @@ -23,10 +23,15 @@ export default function ScreenLink( { screen, anchorText, buttonStyle = false, a
classes.push( 'is-secondary' );
}

const onClick = useCallback( ( event ) => {
event.preventDefault();
navigateToScreen( screen );
}, [] );

return (
<a
href={ screenUrl.href }
onClick={ ( event ) => clickScreenLink( event, screen ) }
onClick={ onClick }
className={ classes.join( ' ' ) }
aria-label={ ariaLabel }
>
Expand Down
4 changes: 2 additions & 2 deletions settings/src/components/tests/utlitites.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,8 @@ describe( 'refreshRecord', () => {
mockRecord.save.mockReset();
} );

it( 'should call edit and save methods on the record object', () => {
refreshRecord( mockRecord );
it( 'should call edit and save methods on the record object', async () => {
await refreshRecord( mockRecord );

expect( mockRecord.edit ).toHaveBeenCalledWith( {
refreshRecordFakeKey: '',
Expand Down
8 changes: 4 additions & 4 deletions settings/src/components/totp.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export default function TOTP() {
*/
function Setup() {
const {
clickScreenLink,
navigateToScreen,
setGlobalNotice,
user: { userRecord },
} = useContext( GlobalContext );
Expand Down Expand Up @@ -74,8 +74,8 @@ function Setup() {
},
} );

refreshRecord( userRecord );
clickScreenLink( event, 'backup-codes' );
await refreshRecord( userRecord );
navigateToScreen( 'backup-codes' );
setGlobalNotice( 'Successfully enabled One Time Passwords.' ); // Must be After `clickScreenEvent` clears it.
} catch ( handleEnableError ) {
setError( handleEnableError.message );
Expand Down Expand Up @@ -304,7 +304,7 @@ function Manage() {
data: { user_id: userRecord.record.id },
} );

refreshRecord( userRecord );
await refreshRecord( userRecord );
setGlobalNotice( 'Successfully disabled One Time Passwords.' );
} catch ( handleDisableError ) {
setError( handleDisableError.message );
Expand Down
16 changes: 6 additions & 10 deletions settings/src/script.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ function Main( { userId } ) {
const [ globalNotice, setGlobalNotice ] = useState( '' );

let currentUrl = new URL( document.location.href );
let initialScreen = currentUrl.searchParams.get( 'screen' );
const [ screen, setScreen ] = useState( initialScreen );

// The index is the URL slug and the value is the React component.
const components = {
Expand All @@ -81,17 +83,12 @@ function Main( { userId } ) {
// The screens where a recent two factor challenge is required.
const twoFactorRequiredScreens = [ 'webauthn', 'totp', 'backup-codes' ];

let initialScreen = currentUrl.searchParams.get( 'screen' );

if ( ! components[ initialScreen ] ) {
initialScreen = 'account-status';
currentUrl.searchParams.set( 'screen', initialScreen );
window.history.pushState( {}, '', currentUrl );
}

const [ screen, setScreen ] = useState( initialScreen );
const currentScreen = components[ screen ];

// Listen for back/forward button clicks.
useEffect( () => {
window.addEventListener( 'popstate', handlePopState );
Expand All @@ -117,10 +114,8 @@ function Main( { userId } ) {
* This is used in conjunction with real links in order to preserve deep linking and other foundational
* behaviors that are broken otherwise.
*/
const clickScreenLink = useCallback(
( event, nextScreen ) => {
event.preventDefault();

const navigateToScreen = useCallback(
( nextScreen ) => {
// Reset to initial after navigating away from a page.
// Note: password was initially not in record, this would prevent incomplete state
// from resetting when leaving the password setting page.
Expand All @@ -146,6 +141,7 @@ function Main( { userId } ) {
return <Spinner />;
}

const currentScreen = components[ screen ];
let screenContent = currentScreen;

if ( 'account-status' !== screen ) {
Expand Down Expand Up @@ -187,7 +183,7 @@ function Main( { userId } ) {
}

return (
<GlobalContext.Provider value={ { clickScreenLink, user, setGlobalNotice } }>
<GlobalContext.Provider value={ { navigateToScreen, user, setGlobalNotice } }>
<GlobalNotice notice={ globalNotice } setNotice={ setGlobalNotice } />
{ screenContent }
</GlobalContext.Provider>
Expand Down