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

Store the two-factor details in the user session at login time #528

Merged
merged 17 commits into from
Apr 19, 2023

Conversation

dd32
Copy link
Member

@dd32 dd32 commented Feb 20, 2023

This PR is part of some larger work for #484.

This PR causes the two-factor timestamp and provider to be stored in the user session, which can be later used to validate that the user session is two-factor authenticated.

Before After
Screenshot 2023-02-20 at 2 45 59 pm Screenshot 2023-02-20 at 2 45 46 pm

Code Notes:

  • The 'attach_session_information' hook is poorly designed for this case, requiring the usage of an anonymous function so as to retrieve the details from the current-scope of the class.
  • It's possible not to use the callback (See the commit history where I've gone back and forth) by creating the session token yourself and passing that into wp_set_auth_cookie(), but that requires the caller to duplicate significantly more code than this option.
  • Note: The screenshots were taken with 0749b4a which places the extra array fields at the end, with dbee826 the fields are placed at the start of the array. This has no meaningful effect.
  • Unit testing this should be possible, but has not been included at time of PR creation.

To test, you can use something like $session = WP_Session_Tokens::get_instance( $user->ID )->get( wp_get_session_token() ); on an admin page to inspect the current sessions session, this is what's used in the above screenshots.

@dd32 dd32 added enhancement PHP Pull requests that update Php code labels Feb 20, 2023
@dd32 dd32 self-assigned this Feb 20, 2023
@dd32
Copy link
Member Author

dd32 commented Feb 20, 2023

Unit testing this should be possible, but has not been included at time of PR creation.

Unfortunately full unit testing isn't possible unless #527 lands.

I've added a unit test to verify that this new getter returns false for a user session set outside of the two-factor auth flow though.

@jeffpaul jeffpaul added this to the Future Release milestone Mar 1, 2023
@dd32 dd32 force-pushed the add/mark-sessions-as-2fa branch 2 times, most recently from 181d319 to f2b74a8 Compare March 2, 2023 03:58
@dd32
Copy link
Member Author

dd32 commented Mar 2, 2023

Unit testing this should be possible, but has not been included at time of PR creation.

Unfortunately full unit testing isn't possible unless #527 lands.

I've rebased this PR after I merged #527, and tests are now included that covers the login flow - validating that a cookie set outside of the login flow doesn't get flagged as 2fa, and that a login via the 2fa flow sets the appropriate session flags.

This unfortunately did require changing how the login_form_validate_2fa handler works, splitting it in two - the part that does the login, and the part that does the exit. Not ideal, but does now allow for validate_2fa to be unit testable,

Copy link
Member

@iandunn iandunn left a comment

Choose a reason for hiding this comment

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

I ran out of time today, but wanted to send what I have so far. I'll pick it back up tomorrow.

class-two-factor-core.php Show resolved Hide resolved
tests/class-two-factor-core.php Outdated Show resolved Hide resolved
tests/class-two-factor-core.php Outdated Show resolved Hide resolved
class-two-factor-core.php Outdated Show resolved Hide resolved
tests/class-two-factor-core.php Show resolved Hide resolved
Copy link
Member

@iandunn iandunn left a comment

Choose a reason for hiding this comment

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

Looks good and works in my tests 👍🏻

@dd32 dd32 force-pushed the add/mark-sessions-as-2fa branch from d1668c4 to a799823 Compare April 13, 2023 04:50
class-two-factor-core.php Outdated Show resolved Hide resolved
@dd32 dd32 modified the milestones: Future Release, 0.9.0 Apr 19, 2023
@dd32 dd32 merged commit 836ef62 into WordPress:master Apr 19, 2023
@dd32 dd32 deleted the add/mark-sessions-as-2fa branch April 19, 2023 04:58
dd32 added a commit that referenced this pull request May 22, 2023
@jeffpaul jeffpaul mentioned this pull request May 24, 2023
14 tasks
@kasparsd kasparsd mentioned this pull request Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PHP Pull requests that update Php code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants