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

Fix unauthorized url access #189

merged 7 commits into from
Jun 9, 2023

Conversation

renintw
Copy link
Contributor

@renintw renintw commented May 25, 2023

Fixes #188

This PR tidies up the code a bit for readability and DRYness and then makes sure users can't access the BackupCodes component via the URL bar when 2fa isn't enabled yet.

Screencast

Sandbox

fix.unauthorized.url.access.mov

@renintw renintw changed the title Fix/unauthorized url access Fix unauthorized url access May 25, 2023
@renintw renintw self-assigned this May 26, 2023
@renintw renintw added bug Something isn't working ui Related to user interface labels May 26, 2023
@renintw renintw added this to the Iteration 1 milestone May 26, 2023
@renintw renintw marked this pull request as ready for review May 26, 2023 03:44

const totpEnabled = record?.[ '2fa_available_providers' ].includes( 'Two_Factor_Totp' );
const backupCodesEnabled =
record?.[ '2fa_available_providers' ].includes( 'Two_Factor_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'm generally in favor of moving these up the component chain or abstracting them into a shared state/context. I prefer that to how we currently reference them in the components, especially since they are provided by a third party plugin. Changing them in one place is better in my opinion and would provide a better api for graceful error handling. We are also not consistently checking for whether an object is defined:

record[ '2fa_revalidation' ].expires_at = new Date().getTime() / 1000 + 3600;

With that being said, I do find it hard to follow which changes were need to fix the originating issue. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I also find it harder to review PRs that do unrelated things, or get too big. What do you think about splitting those clean-up commits into a different PR?

I like the idea of pulling all the {provider}Enabled, hasPrimaryProvider, etc things into the Context object 👍🏻

Copy link
Contributor Author

@renintw renintw May 29, 2023

Choose a reason for hiding this comment

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

Thanks for pointing it out. I've split the DRYness commits into a separate PR, only related stuff is left in this one.

I like the idea of pulling all the {provider}Enabled, hasPrimaryProvider, etc things into the Context object 👍🏻

IMHO, we might want to avoid using Context in this case as we're not dealing with prop drilling - passing data down through many levels of components, and also not dealing with passing down to many components. Overusing Context sometimes might make the code hard to maintain.

@renintw renintw force-pushed the fix/unauthorized-url-access branch from cae02ef to 50ea0ae Compare May 29, 2023 20:50
@renintw renintw force-pushed the fix/unauthorized-url-access branch from 50ea0ae to 436af8f Compare May 29, 2023 23:54
@renintw renintw changed the base branch from trunk to enhancement/extract-variables-for-DRY May 29, 2023 23:58
@renintw renintw force-pushed the enhancement/extract-variables-for-DRY branch from 91ef2d8 to a2fdfb7 Compare May 30, 2023 09:30
@renintw renintw force-pushed the fix/unauthorized-url-access branch from 436af8f to 6a70eca Compare May 30, 2023 09:30
Base automatically changed from enhancement/extract-variables-for-DRY to trunk May 31, 2023 06:13
@renintw renintw force-pushed the fix/unauthorized-url-access branch from 6a70eca to a940f64 Compare May 31, 2023 14:54
@renintw
Copy link
Contributor Author

renintw commented May 31, 2023

It has been rebased to trunk, and is ready for review again.

Comment on lines 30 to 33
const currentUrl = new URL( document.location.href );
currentUrl.searchParams.set( 'screen', 'account-status' );
window.history.pushState( {}, '', currentUrl );
setScreen( 'account-status' );
Copy link
Contributor

@adamwoodnz adamwoodnz Jun 1, 2023

Choose a reason for hiding this comment

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

I actually had a go at doing just this recently and my approach was to reuse the internals of the clickScreenLink function, as this is almost exactly that. I created a branch which changed that to a basic navigation handler: https://github.com/WordPress/wporg-two-factor/compare/fix/unauthorized-backup-codes?expand=1

Events are handled at the component level and then navigateToScreen is called.

See what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the enhancement 👍 Thanks for it.
I've cherry-picked the commits from your branch and tested the behavior on a sandbox, things worked as expected to me. Do you mind taking another look and reviewing again? Thanks.

@renintw renintw force-pushed the fix/unauthorized-url-access branch 2 times, most recently from 91b0f42 to 57dc960 Compare June 6, 2023 20:33
@iandunn
Copy link
Member

iandunn commented Jun 6, 2023

#200 introduced dd26ab3, so 26760b8 no longer needs to modify refreshRecord(), it can just update the callers.

@renintw renintw force-pushed the fix/unauthorized-url-access branch 2 times, most recently from 0b7b675 to fac049f Compare June 6, 2023 22:50
@renintw
Copy link
Contributor Author

renintw commented Jun 6, 2023

#200 introduced dd26ab3, so 26760b8 no longer needs to modify refreshRecord(), it can just update the callers.

Thanks 👍 The code had been updated and so had the commit message. See f8abd29.

renintw added 3 commits June 8, 2023 19:53
save() promise was introduced in dd26ab3 for the function refreshRecord, so an await should be added.
@renintw renintw force-pushed the fix/unauthorized-url-access branch from fac049f to 1e76b9c Compare June 8, 2023 11:53
@renintw renintw force-pushed the fix/unauthorized-url-access branch from 1e76b9c to 8011dd2 Compare June 8, 2023 19:47
Copy link
Contributor

@adamwoodnz adamwoodnz left a comment

Choose a reason for hiding this comment

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

This works for me 👍

A couple of comments inline though

@renintw renintw merged commit 07aecfa into trunk Jun 9, 2023
@renintw renintw deleted the fix/unauthorized-url-access branch June 9, 2023 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ui Related to user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Users can still access backup-codes page through URL bar even if it's disabled
4 participants