Skip to content

SSO: Bypass local Two-Factor prompt when WP.com 2FA was completed#47306

Open
kraftbj wants to merge 1 commit intotrunkfrom
kraftbj/sso-twofactor-bypass
Open

SSO: Bypass local Two-Factor prompt when WP.com 2FA was completed#47306
kraftbj wants to merge 1 commit intotrunkfrom
kraftbj/sso-twofactor-bypass

Conversation

@kraftbj
Copy link
Contributor

@kraftbj kraftbj commented Feb 24, 2026

See CONNECT-178

Proposed changes:

Two-Factor plugin 0.15.0+ changed how it hooks into WordPress authentication—both the authenticate filter and wp_login action are now registered unconditionally in add_hooks(). Since SSO's handle_login() fires do_action('wp_login', ...) directly, Two-Factor's handler destroys the session, clears the auth cookie SSO just set, and shows a local 2FA prompt. The user ends up double-prompted for 2FA even though they already completed it on WordPress.com.

This PR takes the approach of accepting WP.com 2FA as a valid second factor rather than fully restoring the old behavior. When WP.com confirms two_step_enabled, we remove Two-Factor's wp_login hook before it can intercept—SSO completes normally. When WP.com 2FA is NOT active, the hook stays in place and Two-Factor enforces local 2FA as usual. The idea is that if someone has Two-Factor installed, they want 2FA on login, and WP.com 2FA satisfies that intent.

This is safe across Two-Factor versions:

  • Pre-0.15.0: The wp_login hook wasn't registered during SSO (it was conditional inside filter_authenticate). remove_action is a no-op.
  • 0.15.0+: The hook is registered unconditionally. remove_action removes it, preventing the redundant prompt.
  • No Two-Factor: class_exists('Two_Factor_Core') is false, block is skipped entirely.

Other information:

  • Have you written new tests for your changes, if applicable?
  • Have you checked the E2E test CI results, and verified that your changes do not break them?
  • Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

What do you think about this approach, @bindlegirl? The alternative would be to fully restore the old behavior (always bypass Two-Factor during SSO regardless of WP.com 2FA status). This version felt more aligned with the intent of having Two-Factor installed—you want 2FA on login, and WP.com 2FA counts. But if you'd rather just fully restore the previous behavior, that's a one-line change (drop the two_step_enabled check).

See WordPress/two-factor#811

Does this pull request change what data or activity we track or use?

No.

Testing instructions:

  1. Install Two-Factor plugin (0.15.0+) alongside Jetpack with SSO enabled
  2. Configure a user with both WP.com 2FA and a local Two-Factor method (e.g. TOTP)
  3. Log out, click "Log in with WordPress.com", complete WP.com auth (including 2FA)
  4. Verify: User is logged in and redirected normally—no local 2FA prompt
  5. Disable WP.com 2FA on the account, repeat SSO login
  6. Verify: Two-Factor's local 2FA prompt appears (the safety net works)
  7. Test with Two-Factor plugin deactivated—SSO works normally
  8. Test with an older Two-Factor version (< 0.15.0)—SSO works normally

Changelog

  • Generate changelog entries for this PR (using AI).

Two-Factor plugin 0.15.0+ unconditionally hooks wp_login at PHP_INT_MAX,
which destroys the auth session and shows a local 2FA prompt — even for
SSO logins that already completed 2FA on WordPress.com.

When WP.com confirms two_step_enabled, remove Two-Factor's wp_login hook
so SSO can complete without a redundant prompt. When WP.com 2FA is NOT
active, the hook stays so Two-Factor can enforce local 2FA normally.

See CONNECT-178
@github-actions
Copy link
Contributor

github-actions bot commented Feb 24, 2026

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

  • To test on WoA, go to the Plugins menu on a WoA dev site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin (Jetpack or WordPress.com Site Helper), and enable the kraftbj/sso-twofactor-bypass branch.
  • To test on Simple, run the following command on your sandbox:
bin/jetpack-downloader test jetpack kraftbj/sso-twofactor-bypass
bin/jetpack-downloader test jetpack-mu-wpcom-plugin kraftbj/sso-twofactor-bypass

Interested in more tips and information?

  • In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
  • Read more about our development workflow here: PCYsg-eg0-p2
  • Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

@github-actions
Copy link
Contributor

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • 🔴 Add a "[Status]" label (In Progress, Needs Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖


Follow this PR Review Process:

  1. Ensure all required checks appearing at the bottom of this PR are passing.
  2. Make sure to test your changes on all platforms that it applies to. You're responsible for the quality of the code you ship.
  3. You can use GitHub's Reviewers functionality to request a review.
  4. When it's reviewed and merged, you will be pinged in Slack to deploy the changes to WordPress.com simple once the build is done.

If you have questions about anything, reach out in #jetpack-developers for guidance!

@github-actions github-actions bot added the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Feb 24, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a conflict between Jetpack SSO and the Two-Factor plugin version 0.15.0+, where users were being double-prompted for 2FA even after completing it on WordPress.com. The Two-Factor plugin now unconditionally hooks wp_login at PHP_INT_MAX priority, which destroys the auth session that SSO just created.

Changes:

  • Added conditional logic to remove Two-Factor's wp_login hook when the user has completed WP.com 2FA
  • Maintained the safety net behavior where local Two-Factor enforcement continues if WP.com 2FA is not active
  • Updated changelog to document the fix

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
projects/packages/connection/src/sso/class-sso.php Added logic to bypass Two-Factor plugin's wp_login hook when WP.com 2FA was completed, preventing redundant local 2FA prompts
projects/packages/connection/changelog/fix-sso-two-factor-bypass Added changelog entry documenting the fix for the Two-Factor 0.15.0+ conflict

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@jp-launch-control
Copy link

Code Coverage Summary

Coverage changed in 1 file.

File Coverage Δ% Δ Uncovered
projects/packages/connection/src/sso/class-sso.php 0/585 (0.00%) 0.00% 2 ❤️‍🩹

Full summary · PHP report · JS report

@bindlegirl
Copy link
Contributor

bindlegirl commented Feb 25, 2026

Thanks for working on this @kraftbj !
The proposed solution looks good to me and tests well. There are two concerns from WordPress/two-factor#811 I'm trying to consider.

  1. Opt-out for site admins. I agree with your approach of accepting WP.com 2FA as a valid second factor. But some admins may want to require the local Two-Factor prompt even when WP.com 2FA was completed, for whatever reasons. A filter that defaults to true (preserving your current behavior) would give them that option without us having to pick sides.
  2. Session not flagged as 2FA. As dd32 noted, bypassing Two-Factor's wp_login hook means the session won't have the two-factor-login metadata that Two-Factor sets on successful 2FA. The practical effect today is that is_current_user_session_two_factor() returns false, so users get re-prompted for 2FA when managing their Two-Factor settings. We can address this by hooking attach_session_information to set two-factor-login on the session.

Something like this, building on your existing block:

if (
    ! empty( $user_data->two_step_enabled )
    && class_exists( 'Two_Factor_Core' )
    /**
     * Filter whether to accept WordPress.com 2FA in place of a local
     * Two-Factor prompt during SSO login.
     *
     * Return false to always require the local Two-Factor prompt,
     * even when the user has completed 2FA on WordPress.com.
     *
     * @since $$next-version$$
     * @module sso
     *
     * @param bool    $accept    Whether to accept WP.com 2FA. Default true.
     * @param object  $user_data WordPress.com user data from SSO validation.
     * @param WP_User $user      The local WordPress user.
     */
    && apply_filters( 'jetpack_sso_accept_wpcom_2fa', true, $user_data, $user )
) {
    add_filter(
        'attach_session_information',
        function ( $session, $user_id ) use ( $user ) {
            if ( $user->ID === $user_id ) {
                $session['two-factor-login'] = time();
            }
            return $session;
        },
        10,
        2
    );

    remove_action( 'wp_login', array( 'Two_Factor_Core', 'wp_login' ), PHP_INT_MAX );
}

What do you think — worth the addition?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] SSO [Package] Connection [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants