From cc0940362f6b34005539f98621fc46206e053f5b Mon Sep 17 00:00:00 2001 From: Gustavo Bordoni Date: Sun, 18 Oct 2026 20:37:56 +0000 Subject: [PATCH 1/2] fix(mfa): harden MFA against bypasses found in a security review - Bearer tokens from other apps on the WorkOS environment no longer act for users with MFA; only sessions that signed in here (VerifiedSessions). - Admin MFA routes and tools never apply to your own account (edit_user is granted on yourself to every role). - Adding a second authenticator needs a current code from an existing one, on the account panel and the legacy TOTP/SMS routes. - Wrong codes capped per account (5/15 min, 30/day) under a lock (CodeGuard); a code must come from its challenge's factor. - Sign-in /challenge and /verify need an mfa_token; WorkOS mfa_challenge is parked too, so the pending token never reaches the browser. - Policy reads factors from WorkOS at every sign-in; the enabled flag is sticky, so vanished factors mean enroll, not password-only. - Users with MFA are never re-linked by email; app-password exemption is per user (XML-RPC multicall); guard follows an 'always' default profile. - Parked sign-ins: fixed expiry, encrypted response, one-time resume code in the hosted return URL. - Factor changes email the account and sign out other sessions; admin reset signs out everywhere and revokes the WorkOS session. - Headless refuses before storing tokens; admin panel applies after save; MFA errors drop WorkOS response bodies. --- CHANGELOG.md | 14 + docs/mfa.md | 164 ++++- docs/rate-limiting.md | 6 +- src/WorkOS/Auth/AuthKit/FrontendRoute.php | 10 +- src/WorkOS/Auth/AuthKit/LoginCompleter.php | 167 +++-- src/WorkOS/Auth/Login.php | 18 +- src/WorkOS/Mfa/AccountChanges.php | 161 +++++ src/WorkOS/Mfa/Admin/UserEditPanel.php | 85 ++- src/WorkOS/Mfa/CodeGuard.php | 214 ++++++ src/WorkOS/Mfa/Controller.php | 10 +- src/WorkOS/Mfa/FactorService.php | 255 +++++-- src/WorkOS/Mfa/PasswordGuard.php | 94 ++- src/WorkOS/Mfa/PendingLogin.php | 156 +++- src/WorkOS/Mfa/RestApi.php | 34 +- src/WorkOS/Mfa/VerifiedSessions.php | 155 ++++ src/WorkOS/REST/Auth/Mfa.php | 197 ++--- src/WorkOS/REST/TokenAuth.php | 21 + src/WorkOS/Sync/UserSync.php | 12 +- src/js/authkit/index.tsx | 7 + src/js/authkit/types.ts | 6 +- src/js/mfa-settings/index.tsx | 10 +- tests/wpunit/AuthKitLoginCompleterMfaTest.php | 12 +- .../AuthKitRestEmailVerificationTest.php | 4 +- tests/wpunit/AuthKitRestMfaTest.php | 96 ++- tests/wpunit/AuthKitRestPasswordTest.php | 12 +- tests/wpunit/MfaAccountRestTest.php | 4 +- tests/wpunit/MfaAdminTest.php | 7 +- tests/wpunit/MfaFactorServiceTest.php | 14 +- tests/wpunit/MfaLoginErrorsTest.php | 4 +- tests/wpunit/MfaPluginEnforcementTest.php | 4 +- tests/wpunit/MfaSecurityTest.php | 672 ++++++++++++++++++ 31 files changed, 2329 insertions(+), 296 deletions(-) create mode 100644 src/WorkOS/Mfa/AccountChanges.php create mode 100644 src/WorkOS/Mfa/CodeGuard.php create mode 100644 src/WorkOS/Mfa/VerifiedSessions.php create mode 100644 tests/wpunit/MfaSecurityTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index aaa3dd7..16dad32 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,20 @@ ### Security - **Password-only sign-ins can't skip MFA** — in `plugin` mode, `wp-login.php` with `?fallback=1`, `wp_signon()`, headless mode and the login button refuse (`403 workos_mfa_step_unsupported`) a user who has a factor or must use MFA, since they can't ask for a code. Application passwords are exempt. Filter `workos_mfa_allow_password_fallback`. +- **MFA hardening** — from a security review of the MFA work before release. In `plugin` mode: + - **Bearer tokens** from other apps on the same WorkOS environment no longer act for users who have or need MFA. REST accepts only tokens whose session signed in on this site (filters `workos_mfa_accept_bearer_token`, `workos_bearer_token_user`). + - **Your own account**: the admin routes (`PUT /users/{id}/mfa/required`, `POST …/mfa/reset`) and the user-edit tools no longer work on yourself. WordPress grants every user `edit_user` on themselves, so a hijacked session could reset its factors and clear the require flag without a code. + - **Adding a second authenticator** to an account that has one needs a current code from an existing one (`factor_id`, `code`; `403 workos_mfa_code_required`). This covers the account panel and the legacy `/auth/mfa/totp/enroll` and `/auth/mfa/sms/enroll` routes, which also follow `workos_mfa_can_manage` now. + - **Wrong codes are capped per account**, 5 per 15 minutes and 30 a day, across sign-ins and the account panel (`429 workos_mfa_too_many_codes`), under a per-account lock so parallel requests can't overspend it. + - **Sign-in codes**: `/auth/mfa/challenge` and `/auth/mfa/verify` need an `mfa_token`. WorkOS's own `mfa_challenge` is parked like the plugin's, so the browser never holds WorkOS's pending token and can't pair it with a challenge from another account. A code must come from the factor its challenge was made for. + - **Factors added outside WordPress** (the WorkOS dashboard, another app, before this version) are challenged: the policy reads the user's factors from WorkOS at every sign-in. Factors removed outside WordPress send the user to enroll instead of letting them in with a password. + - **Re-linking**: a user with MFA is never re-linked to a different WorkOS user by email, whatever the Re-link Protection setting (filter `workos_relink_protected`). + - **Application passwords** exempt only their own user from the password-only guard, so an XML-RPC `system.multicall` can't borrow one to sign in another user. The guard also follows a default Login Profile set to `always`, for users linked to WorkOS. + - **A parked sign-in** expires 10 minutes after the first factor however often it's touched, and its stored WorkOS response is encrypted with a key derived from the token. The hosted-AuthKit return URL carries a one-time resume code instead of the token. + - **Account changes**: adding or removing an authenticator emails the account and signs out its other sessions. An admin reset signs the user out everywhere and revokes their WorkOS session (filter `workos_mfa_notify_user`). + - **Headless mode** refuses an MFA user before storing their tokens or firing `workos_user_authenticated`. + - **The admin panel** applies its changes only after the profile saves, and shows a reset WorkOS refused. + - **Errors** from MFA routes no longer include WorkOS's response body. - **WordPress password could skip WorkOS MFA** — on the headless `wp-login.php` path, a WorkOS `mfa_challenge` or `mfa_enrollment` answer was treated like a wrong password, and WordPress's own password check then signed the user in without a second factor. That path now refuses with `403 workos_mfa_step_unsupported`, past WordPress's password handlers too. The login button's form gives the same message instead of WorkOS's raw error, and the AuthKit password route no longer re-syncs the password to WorkOS when only the second factor is missing. ### Fixed diff --git a/docs/mfa.md b/docs/mfa.md index ff21651..d2c3f18 100644 --- a/docs/mfa.md +++ b/docs/mfa.md @@ -101,23 +101,33 @@ None of its methods check capabilities, so callers do. | Method | Does | |---|---| -| `status( $user_id )` | `{ enabled, required, factors }`, reconciled with WorkOS | -| `list_factors( $user_id )` | Confirmed factors from WorkOS. Also fixes the mirrored flag when factors were added or removed outside WordPress | -| `start_enrollment( $user_id )` | Creates a TOTP factor and returns `{ factor_id, challenge_id, qr_code, secret, otpauth_uri }`. The factor stays unconfirmed, and doesn't count, until confirmed | +| `status( $user_id )` | `{ enabled, required, must_use, factors }`, read from WorkOS. `required` is the admin flag; `must_use` adds roles and filters | +| `list_factors( $user_id )` | Confirmed factors from WorkOS. Sets the mirrored flag when WorkOS has a factor it didn't know about; never clears it (see below) | +| `start_enrollment( $user_id )` | Creates a TOTP factor and returns `{ factor_id, challenge_id, qr_code, secret, otpauth_uri }`. The factor stays unconfirmed, and doesn't count, until confirmed. One at a time per account | | `confirm_enrollment( $user_id, $factor_id, $code, $challenge_id = '' )` | Checks the first code and turns MFA on | -| `verify_code( $factor_id, $code, $challenge_id = '' )` | Checks a code; no hooks | +| `check_code( $user_id, $factor_id, $code, $challenge_id = '' )` | Checks a code, counted against the account's wrong-code limit; no hooks | +| `require_current_factor( $user_id, $factor_id, $code )` | `true` when the user has no factor yet; otherwise needs a current code from one of theirs | +| `verify_code( $factor_id, $code, $challenge_id = '' )` | Checks a code with no limit. A challenge passed in must belong to the factor | | `delete_factor( $user_id, $factor_id )` | Removes one of the user's factors (404 for anyone else's) | | `reset( $user_id )` | Removes every factor, for a lost authenticator | | `set_required( $user_id, $required )` | Sets or clears the require flag | An unconfirmed factor is deleted from WorkOS when the user starts over, or -after a day. +after a day. If WorkOS can't delete it, it stays tracked as unconfirmed. + +`_workos_mfa_enabled` means "this user turned MFA on". It's set when a factor +is confirmed or found in WorkOS, and cleared only when the user removes their +last factor or an admin resets them. If factors disappear from WorkOS some +other way (the WorkOS dashboard, another app), the next sign-in asks the user +to enroll again rather than letting them in with a password alone. To turn a +user's MFA off, use the reset (admin screen, `wp workos mfa reset`, +`workos_mfa_reset()`), not the WorkOS dashboard. ### PHP helpers ```php workos_mfa_get_status( $user_id = 0 ); // array|WP_Error — { enabled, required, factors } -workos_mfa_is_enabled( $user_id = 0 ); // bool — mirrored flag, no WorkOS call +workos_mfa_is_enabled( $user_id = 0 ); // bool — the user turned MFA on (mirrored flag, no WorkOS call) workos_mfa_is_required( $user_id = 0 ); // bool — flag, role or filter workos_mfa_set_required( $user_id, $required ); // bool|WP_Error — whether it changed workos_mfa_list_factors( $user_id = 0 ); // array|WP_Error @@ -134,7 +144,10 @@ check capabilities. |---|---|---| | `workos_mfa_user_not_linked` | 409 | The account has no WorkOS user | | `workos_mfa_factor_not_found` | 404 | The factor isn't the user's, or isn't the one being enrolled | -| `workos_mfa_invalid_code` | 400 | Wrong, expired or malformed code | +| `workos_mfa_invalid_code` | 400 | Wrong, expired or malformed code, or a challenge for another factor | +| `workos_mfa_code_required` | 403 | Adding an authenticator to an account that has one, without a current code | +| `workos_mfa_too_many_codes` | 429 | Too many wrong codes for the account (5 in 15 minutes, 30 a day) | +| `workos_mfa_busy` | 429 | Another code check or enrollment is running for the account | | `workos_mfa_invalid_user` | 404 | `workos_mfa_set_required()` got an unknown user | | `workos_mfa_invalid_response` | 502 | WorkOS answered an enrollment without a factor | @@ -142,19 +155,25 @@ check capabilities. Every AuthKit sign-in (password, magic code, OAuth, the hosted-AuthKit callback) ends in `LoginCompleter::complete()`, which hands the browser one of -three MFA steps: +two MFA steps: | Who asked | Response | The browser then | |---|---|---| -| WorkOS (`mfa_challenge`) | `{ mfa_required, pending_authentication_token, factors }` | Challenges a factor and verifies with `pending_authentication_token` | -| The plugin's policy (`challenge`) | `{ mfa_required, mfa_token, enroll: false, factors }` | Challenges a factor and verifies with `mfa_token` | +| WorkOS (`mfa_challenge`), or the plugin's policy (`challenge`) | `{ mfa_required, mfa_token, enroll: false, factors }` | Challenges one of `factors` and verifies with `mfa_token` | | The plugin's policy (`enroll`), or WorkOS (`mfa_enrollment`) | `{ mfa_required, mfa_token, enroll: true, factors: [] }` | Enrolls, then verifies the first code with `mfa_token` | -With an `mfa_token`, nothing reaches the browser but that token. WorkOS's -response (tokens included) or its pending token waits server-side in -`WorkOS\Mfa\PendingLogin`: bound to the Login Profile, used once, capped at 5 -wrong codes, and gone after 10 minutes (filter `workos_mfa_pending_ttl`). No -WordPress session exists until the code checks out. +Nothing reaches the browser but the `mfa_token`. WorkOS's response (tokens +included) or its pending token waits server-side in `WorkOS\Mfa\PendingLogin`: +bound to the Login Profile, used once, capped at 5 wrong codes, and gone 10 +minutes after the first factor however often it's touched (filter +`workos_mfa_pending_ttl`). The parked response is encrypted with a key derived +from the token, which the server doesn't keep. No WordPress session exists +until the code checks out. + +In `plugin` mode the policy asks WorkOS for the user's factors at every +sign-in, so a factor added outside WordPress is challenged too. Only factors +the profile allows can answer; a user who turned MFA on but has none the +profile can use is asked to enroll. A profile the policy blocks answers `403 workos_mfa_blocked` (message filter `workos_mfa_block_message`, action `workos_mfa_blocked`). @@ -167,8 +186,11 @@ All three take the profile and its nonce (`X-WorkOS-Nonce`), like every | Route | Body | Returns | |---|---|---| | `POST /auth/mfa/enroll` | `mfa_token` | `201 { factor_id, challenge_id, qr_code, secret, otpauth_uri }`. Calling it again replaces the unfinished factor | -| `POST /auth/mfa/challenge` | `factor_id`, plus `mfa_token` when there is one | `{ challenge_id, expires_at }`. With `mfa_token`, only the sign-in's own factors | -| `POST /auth/mfa/verify` | `authentication_challenge_id`, `code`, and `mfa_token` or `pending_authentication_token` | `{ user, redirect_to }` with the session set. With `mfa_token`, only challenges this sign-in asked for | +| `POST /auth/mfa/challenge` | `mfa_token`, `factor_id` | `{ challenge_id, expires_at }`. Only the sign-in's own factors | +| `POST /auth/mfa/verify` | `mfa_token`, `authentication_challenge_id`, `code` | `{ user, redirect_to }` with the session set. Only challenges this sign-in asked for | + +Before 1.1.0, `/challenge` took any `factor_id` and `/verify` took WorkOS's +`pending_authentication_token`. Both now need an `mfa_token`. | Code | Status | Cause | |---|---|---| @@ -177,27 +199,35 @@ All three take the profile and its nonce (`X-WorkOS-Nonce`), like every | `workos_authkit_challenge_not_found` | 404 | The challenge wasn't issued for this sign-in | | `workos_mfa_invalid_code` | 400 | Wrong code | | `workos_authkit_mfa_attempts_exceeded` | 403 | Fifth wrong code; start over | +| `workos_mfa_too_many_codes` | 429 | Too many wrong codes for the account, across sign-ins | | `workos_mfa_blocked` | 403 | The profile can't satisfy the user's MFA | | `workos_authkit_mfa_enrollment_required` | 403 | WorkOS asked for an enrollment without saying which user | Rate limits: `mfa_enroll_ip` / `mfa_enroll_token`, `mfa_challenge_ip` / -`mfa_challenge_factor`, `mfa_verify_ip` / `mfa_verify_token` (see -[`rate-limiting.md`](rate-limiting.md)). +`mfa_challenge_factor`, `mfa_verify_ip` / `mfa_verify_token`, and +`mfa_code_account` across every surface (see [`rate-limiting.md`](rate-limiting.md)). ### Hosted AuthKit The hosted page runs WorkOS's own MFA, but not the plugin's policy. When the policy wants a code after the callback, the sign-in is parked and the visitor -lands on `/workos/login/{profile}/?workos_mfa=`, which renders only the -MFA step, even for a profile in redirect mode. +lands on `/workos/login/{profile}/?workos_mfa=`, which renders only the +MFA step, even for a profile in redirect mode. The code in the URL works once, +for a minute, and only to put the real `mfa_token` into the page; the page +drops it from the address bar and sends `Referrer-Policy: no-referrer`. ### Password-only sign-ins `wp-login.php` with `?fallback=1`, `wp_signon()`, headless mode and the login button take a username and password only, so they can't ask for a code. In `plugin` mode they refuse (`403 workos_mfa_step_unsupported`) a user who has a -factor or must use MFA, and an MFA error from WorkOS never falls back to the -WordPress password. Application passwords are exempt. To allow it anyway: +factor or must use MFA, and, for a user linked to WorkOS, one the default +Login Profile would ask a code of (`enforce: always`). An MFA error from WorkOS +never falls back to the WordPress password. Accounts not linked to WorkOS +aren't held to the profile setting, so turning on `always` can't lock out +local administrators. Application passwords are exempt, for the user they +belong to only (an XML-RPC `system.multicall` can't borrow one for another +user). To allow it anyway: ```php add_filter( 'workos_mfa_allow_password_fallback', fn( $allow, $user_id ) => user_can( $user_id, 'manage_options' ), 10, 2 ); @@ -217,8 +247,11 @@ Users turn two-step verification on and off themselves: `workos_mfa_can_manage` says no. Turning it on shows a QR code and setup key, and the factor only counts once -the first code checks out. Removing a factor needs a current code from it, so -a session someone else got hold of can't quietly turn MFA off. +the first code checks out. Removing a factor needs a current code from it, and +adding a second one needs a current code from an existing one, so a session +someone else got hold of can't turn MFA off or add an authenticator of its +own. Adding or removing an authenticator emails the account and signs out its +other sessions. ### Routes @@ -227,17 +260,20 @@ can use them. | Route | Who | Body | Returns | |---|---|---|---| -| `GET /users/{id}/mfa` | The owner, or `edit_user` (`workos_mfa_can_manage_user`) | — | `{ enabled, required, factors }` | -| `POST /users/{id}/mfa/enroll` | The owner (`workos_mfa_can_manage`) | — | `201 { factor_id, challenge_id, qr_code, secret, otpauth_uri }` | +| `GET /users/{id}/mfa` | The owner, or `edit_user` (`workos_mfa_can_manage_user`) | — | `{ enabled, required, must_use, factors }` | +| `POST /users/{id}/mfa/enroll` | The owner (`workos_mfa_can_manage`) | `factor_id` and `code` when the account already has a factor | `201 { factor_id, challenge_id, qr_code, secret, otpauth_uri }` | | `POST /users/{id}/mfa/confirm` | The owner | `factor_id`, `code`, `challenge_id` | Status | | `DELETE /users/{id}/mfa/factors/{factor_id}` | The owner | `code` | Status | -Rate limits per user: `mfa_enroll_user`, `mfa_confirm_user`, `mfa_delete_user`. -Errors add `workos_mfa_not_logged_in` (401) and `workos_mfa_forbidden` (403). +Rate limits per user: `mfa_enroll_user`, `mfa_confirm_user`, `mfa_delete_user`, +plus the account's wrong-code limit. Errors add `workos_mfa_not_logged_in` +(401) and `workos_mfa_forbidden` (403). The older profile-scoped routes (`GET /auth/mfa/factors`, -`POST /auth/mfa/totp/enroll`, `POST /auth/mfa/factor/delete`) go through the -same service: enrollment needs the confirm step, and delete needs `code`. +`POST /auth/mfa/totp/enroll`, `POST /auth/mfa/sms/enroll`, +`POST /auth/mfa/factor/delete`) go through the same service and the same +rules: `workos_mfa_can_manage`, a current code to add a second factor, the +confirm step for TOTP, and `code` to delete. ```js // Third-party UI: is MFA on for the current user? @@ -255,7 +291,13 @@ const { enabled, required } = await res.json(); - **User-edit screen:** for another linked user, shows whether MFA is on, a **Require** checkbox, and **Lost authenticator** (removes every factor on save). Shown to anyone with `edit_user` on the target, or whom - `workos_mfa_can_manage_user` allows, e.g. a Support role. + `workos_mfa_can_manage_user` allows, e.g. a Support role. Changes apply + once the profile saves, and a reset WorkOS refuses shows as a notice. + A reset emails the user, signs them out everywhere and revokes their stored + WorkOS session. +- **Not on yourself:** WordPress lets every user edit their own account, so + the admin tools and routes never apply to your own account. Your own + factors go through the account panel, which asks for a code. - **Routes** (same permission): | Route | Body | Returns | @@ -292,7 +334,11 @@ const { enabled, required } = await res.json(); | `workos_mfa_pending_ttl` | `int $ttl` | 600 seconds | | `workos_mfa_allow_password_fallback` | `bool $allow, int $user_id` | `false` | | `workos_mfa_can_manage` | `bool $can, int $user_id` | `true` for the account owner | -| `workos_mfa_can_manage_user` | `bool $can, int $user_id` | `edit_user` on the target | +| `workos_mfa_can_manage_user` | `bool $can, int $user_id` | `edit_user` on the target, never yourself | +| `workos_mfa_accept_bearer_token` | `bool $passed, int $user_id, array $payload` | Whether the token's session signed in on this site | +| `workos_mfa_notify_user` | `bool $send, int $user_id, string $event` | `true` (`enrolled`, `removed`, `reset`) | +| `workos_bearer_token_user` | `int $wp_user_id, array $payload` | The user the token's `sub` maps to; 0 refuses (not MFA-specific) | +| `workos_relink_protected` | `bool $protected, int $wp_user_id` | The Re-link Protection setting; MFA turns it on for users with MFA (not MFA-specific) | **Actions** @@ -310,19 +356,63 @@ const { enabled, required } = await res.json(); | Event | Metadata | |---|---| -| `mfa.enrolled` | `factor_id`, `factor_type` | -| `mfa.removed` | `factor_id` | +| `mfa.enrolled` | `factor_id`, `factor_type`, `actor_id` | +| `mfa.removed` | `factor_id`, `actor_id` | | `mfa.reset` | `removed`, `actor_id` | | `mfa.required_set`, `mfa.required_cleared` | `actor_id` | | `mfa.challenge_succeeded`, `mfa.challenge_failed` | `factor_type` | | `mfa.blocked` | `profile` | +## Security model + +What the plugin guarantees in `plugin` mode, for a user who has or must use +MFA: + +- **No session before the code.** Every sign-in that can set a session ends in + `LoginCompleter`, which parks it until the code checks out. Paths that can't + ask for a code (password-only forms, headless mode, the login button) + refuse instead. WordPress's own password check can't finish one either. +- **Bearer tokens from elsewhere don't count.** Any app on the same WorkOS + environment can get an access token for the user, and it verifies here. + REST only accepts a token whose session (`sid`) signed in on this site + (`WorkOS\Mfa\VerifiedSessions`, filter `workos_mfa_accept_bearer_token`). +- **Only the sign-in's own factors.** The browser never holds WorkOS's pending + token. It can challenge only the factors the server listed for its sign-in, + and verify only challenges it was given. A code must come from the factor + its challenge belongs to. +- **Guessing is bounded per account.** 5 wrong codes per sign-in, and 5 per 15 + minutes and 30 per day per account across sign-ins, the account panel and + the legacy routes (`WorkOS\Mfa\CodeGuard`). One code check per account runs + at a time, so parallel requests can't overspend it. WorkOS adds its own: + 3 wrong codes per challenge, one verification per challenge, and a used + code won't verify again. +- **A borrowed session can't change MFA.** Removing a factor, or adding one to + an account that has one, needs a current code. The admin tools never apply + to your own account. Changes email the account and sign out its other + sessions. +- **The account can't be moved to another identity.** A user with MFA is never + re-linked to a different WorkOS user by email; an admin can still with + `wp workos user link --force`. +- **Factors don't vanish quietly.** A factor removed outside WordPress leaves + the user enrolling again at the next sign-in, not signing in with a + password. + +Known limits: + +- Enrolling at sign-in trusts whoever passed the first factor. A user who must + use MFA and hasn't enrolled can have it set up by someone holding their + password. They get an email when it happens; enroll required users promptly. +- Codes are 6 digits and WorkOS accepts a few around the current one, so the + per-account daily cap is what keeps guessing out of reach. +- In `workos` mode WorkOS enforces MFA, which doesn't cover SSO users, and + Bearer tokens aren't gated. + ## Don't do this - Don't store TOTP secrets or QR codes in WordPress. WorkOS holds them; the plugin returns them once, at enrollment. -- Don't write `_workos_mfa_enabled` yourself. It mirrors WorkOS, and - `list_factors()` overwrites it. +- Don't write `_workos_mfa_enabled` yourself, and don't remove factors in the + WorkOS dashboard to turn MFA off. Use the reset. - Don't let a profile with `enforce: never` stand in for "MFA off for this user". It blocks users who have MFA. Use `workos_mfa_decision` for exceptions. diff --git a/docs/rate-limiting.md b/docs/rate-limiting.md index 4c498ef..5ab19ca 100644 --- a/docs/rate-limiting.md +++ b/docs/rate-limiting.md @@ -57,13 +57,14 @@ Why the send policies layer: a single fixed window is burstable across its bound Why the request policies do not: a daily cap on sign-in attempts locks out legitimate users who mistype a password, and the abuse it would prevent is already covered by the send limits. Adding tiers there is a one-line edit if that changes. -Two endpoints declare their own request policy instead of using the shared ones, each next to the endpoint that owns the reasoning: +Three places declare their own policy instead of using the shared ones, each next to the endpoint that owns the reasoning: | Constant | Tiers | Why | |---|---|---| | `Signup::RATE_LIMIT_CREATE_IP` / `_CREATE_EMAIL` | 5 / min, 3 / min | Tighter: signup creates an account for an address nobody has proven they own. | | `Signup::RATE_LIMIT_VERIFY_IP` | 10 / min | Verify is cheap and users retype codes. | | `OAuth::RATE_LIMIT_URL_IP` | 20 / min | Looser: building an authorize URL sends nothing and costs no WorkOS call. | +| `Mfa\CodeGuard::POLICY` | 5 / 15 min, 30 / day | Wrong MFA codes per account (`mfa_code_account`), across sign-ins and the account panel. Charged before WorkOS checks the code and refunded when it's right, under a per-account lock so parallel requests can't overspend it. Someone with the password can start unlimited sign-ins, so a per-sign-in cap alone doesn't bound guessing. | These are class constants, not settings. There is no admin UI and no filter. Retuning is a code change, deliberately: the numbers are a support contract, and the earlier per-feature options (`change_email_rate_limit_*`, removed) were four knobs nobody tuned, where a stored `0` silently meant "unlimited". @@ -84,7 +85,8 @@ Every public auth route is throttled by requester. A subset also counts as a sen | `POST /auth/signup/verify` | — | `Signup` constants (IP only) | no | | `POST /auth/mfa/enroll` | `mfa_token` (hashed) | shared | no | | `POST /auth/mfa/challenge` | factor ID | shared | no | -| `POST /auth/mfa/verify` | `mfa_token` (hashed) when given, else IP only | shared | no | +| `POST /auth/mfa/verify` | `mfa_token` (hashed), plus the account (`CodeGuard`) | shared | no | +| `POST`/`DELETE /users/{id}/mfa/…` | user ID, plus the account (`CodeGuard`) for code checks | shared | no | | `GET /auth/invitation/{token}` | — | shared (IP only) | no | | `POST /auth/invitation/accept` | — | shared (IP only) | no | | `GET /auth/oauth/authorize-url` | — | `OAuth` constant | no | diff --git a/src/WorkOS/Auth/AuthKit/FrontendRoute.php b/src/WorkOS/Auth/AuthKit/FrontendRoute.php index c0cd4e0..67b9cd6 100644 --- a/src/WorkOS/Auth/AuthKit/FrontendRoute.php +++ b/src/WorkOS/Auth/AuthKit/FrontendRoute.php @@ -182,20 +182,26 @@ public function maybe_render(): void { if ( null !== $mfa ) { $context['mfa'] = $mfa; + nocache_headers(); + header( 'Referrer-Policy: no-referrer' ); } $this->renderer->render_full_page( $profile, $context ); } /** - * The MFA step to resume, when `?workos_mfa=` names a live parked sign-in. + * The MFA step to resume, when `?workos_mfa=` carries a live resume code. + * + * The code works once (see PendingLogin::handoff()); the token it + * stands for goes into the page, never the URL. * * @param Profile $profile Profile being rendered. * * @return array|null `{ mfa_required, mfa_token, enroll, factors }`, or null. */ private function parked_mfa( Profile $profile ): ?array { - $token = sanitize_text_field( (string) ( SuperGlobals::get_get_var( 'workos_mfa' ) ?? '' ) ); + $code = sanitize_text_field( (string) ( SuperGlobals::get_get_var( 'workos_mfa' ) ?? '' ) ); + $token = PendingLogin::redeem( $code ); $pending = '' !== $token ? PendingLogin::get( $token, $profile->get_slug() ) : null; if ( null === $pending ) { return null; diff --git a/src/WorkOS/Auth/AuthKit/LoginCompleter.php b/src/WorkOS/Auth/AuthKit/LoginCompleter.php index 00cbc95..2892581 100644 --- a/src/WorkOS/Auth/AuthKit/LoginCompleter.php +++ b/src/WorkOS/Auth/AuthKit/LoginCompleter.php @@ -17,6 +17,7 @@ use WorkOS\Mfa\PendingLogin; use WorkOS\Mfa\Policy; use WorkOS\Mfa\Settings as MfaSettings; +use WorkOS\Mfa\VerifiedSessions; use WorkOS\Organization\EntitlementGate; use WorkOS\Sync\UserSync; use WP_Error; @@ -73,13 +74,8 @@ public function __construct() { * 'user' => { id, email, display_name }, * 'redirect_to' => string, * } - * When WorkOS asks for a code: { - * 'mfa_required' => true, - * 'pending_authentication_token' => string, - * 'factors' => array, - * } - * When the plugin's MFA policy asks for a code, or the user must - * enroll first (either mode): { + * When WorkOS or the plugin's MFA policy asks for a code, or the + * user must enroll first (either mode): { * 'mfa_required' => true, * 'mfa_token' => string, * 'enroll' => bool, @@ -103,11 +99,8 @@ public function complete( $workos_response, Profile $profile, string $redirect_t if ( is_wp_error( $mfa ) ) { return $mfa; } - if ( isset( $mfa['mfa_token'] ) ) { - return $mfa; - } if ( null !== $mfa ) { - $workos_response = $mfa; + return $mfa; } } @@ -125,35 +118,18 @@ public function complete( $workos_response, Profile $profile, string $redirect_t } // WorkOS signals a required second factor by returning a pending - // authentication token and/or an authentication_factor. Hand that - // state back to the React shell so it can run the MFA challenge - // step and then POST /auth/mfa/verify. + // authentication token and/or an authentication_factor. Park it and + // hand the React shell an `mfa_token`, so the challenge and verify + // routes only ever touch this sign-in's own factors. if ( ! empty( $workos_response['pending_authentication_token'] ) ) { - $factors = $this->extract_factors( $workos_response ); - - // Enforce the profile's factor allowlist. If WorkOS surfaces a - // challenge for a factor type the admin has disabled on this - // profile, reject rather than completing via an unauthorized - // factor. - $allowed_types = $profile->get_mfa()['factors'] ?? []; - if ( ! empty( $allowed_types ) ) { - foreach ( $factors as $factor ) { - $type = (string) ( $factor['type'] ?? '' ); - if ( '' !== $type && ! in_array( $type, $allowed_types, true ) ) { - return new WP_Error( - 'workos_authkit_factor_not_allowed', - __( 'The multi-factor method returned is not permitted for this login.', 'integration-workos' ), - [ 'status' => 403 ] - ); - } - } - } - - return [ - 'mfa_required' => true, - 'pending_authentication_token' => (string) $workos_response['pending_authentication_token'], - 'factors' => $factors, - ]; + return $this->workos_challenge( + (string) $workos_response['pending_authentication_token'], + $this->extract_factors( $workos_response ), + (string) ( $workos_response['user']['id'] ?? '' ), + $profile, + $redirect_to, + $honor_profile_redirect + ); } // When the profile insists MFA is always required and WorkOS did @@ -223,6 +199,7 @@ public function complete( $workos_response, Profile $profile, string $redirect_t */ public function finish( array $workos_response, WP_User $wp_user, Profile $profile, string $redirect_to = '', bool $honor_profile_redirect = true ): array { Login::store_tokens( $wp_user->ID, $workos_response ); + VerifiedSessions::remember( (int) $wp_user->ID, $workos_response ); wp_set_auth_cookie( $wp_user->ID, true ); wp_set_current_user( $wp_user->ID ); @@ -328,21 +305,32 @@ private function plugin_mfa_step( WP_User $wp_user, array $workos_response, Prof $decision = $policy->decide( $wp_user->ID, $profile ); $factors = []; - if ( Policy::CHALLENGE === $decision ) { + // Ask WorkOS rather than trust the mirrored flag: a factor added + // outside WordPress (or before this version) must still be + // challenged. list_factors() sets the flag when it finds one, so the + // policy is asked again. + if ( Policy::BLOCK !== $decision ) { $factors = App::container()->get( FactorService::class )->list_factors( $wp_user->ID ); if ( is_wp_error( $factors ) ) { return $factors; } - // The mirrored flag said "enrolled", but WorkOS has no factor - // (removed outside WordPress). list_factors() just cleared the - // flag, so ask again; a challenge with nothing to challenge - // becomes an enrollment. - if ( empty( $factors ) ) { - $decision = $policy->decide( $wp_user->ID, $profile ); - if ( Policy::CHALLENGE === $decision ) { - $decision = Policy::ENROLL; - } + $decision = $policy->decide( $wp_user->ID, $profile ); + + // Only factors this profile allows can answer its challenge. + $factors = array_values( + array_filter( + $factors, + static fn( array $factor ): bool => $profile->allows_factor( (string) $factor['type'] ) + ) + ); + + // The user turned MFA on, but WorkOS has no factor this profile + // can use (removed outside WordPress, or SMS only). The flag + // stays set, so a challenge with nothing to challenge becomes an + // enrollment, at this sign-in and every one after it. + if ( Policy::CHALLENGE === $decision && empty( $factors ) ) { + $decision = Policy::ENROLL; } } @@ -431,18 +419,17 @@ static function ( array $factor ): array { /** * The MFA step, when WorkOS asks for one. * - * `mfa_challenge` becomes the same pending-token shape a challenge has - * always used here, so the factor allowlist below applies to it. - * `mfa_enrollment` parks WorkOS's pending token and user, and hands the - * browser an `mfa_token` for the enrollment step. Without a WorkOS user - * in the error there's nothing to enroll, so it's refused. + * Both steps park WorkOS's pending token and hand the browser an + * `mfa_token`. `mfa_challenge` keeps the factors WorkOS named; + * `mfa_enrollment` keeps the WorkOS user, to enroll. Without a WorkOS + * user in the error there's nothing to enroll, so it's refused. * * @param WP_Error $error Error from the WorkOS authenticate call. * @param Profile $profile Active Login Profile. * @param string $redirect_to Client-requested redirect. * @param bool $honor_profile_redirect Whether the profile's `post_login_redirect` wins. * - * @return array|WP_Error|null Pending-token response, an enrollment step, a refusal, or null for any other error. + * @return array|WP_Error|null The MFA step, a refusal, or null for any other error. */ private function mfa_step( WP_Error $error, Profile $profile, string $redirect_to, bool $honor_profile_redirect ) { $code = MfaStep::code( $error ); @@ -487,9 +474,75 @@ private function mfa_step( WP_Error $error, Profile $profile, string $redirect_t ); } + return $this->workos_challenge( + $pending_token, + isset( $body['authentication_factors'] ) && is_array( $body['authentication_factors'] ) ? $body['authentication_factors'] : [], + (string) ( $workos_user['id'] ?? '' ), + $profile, + $redirect_to, + $honor_profile_redirect + ); + } + + /** + * Park a sign-in WorkOS wants a code for. + * + * The browser gets an `mfa_token`, never WorkOS's pending token, and + * may challenge only the factors WorkOS named for this sign-in. Factors + * of a type the profile doesn't allow refuse the sign-in rather than + * completing through a method the admin turned off. + * + * @param string $pending_token WorkOS pending authentication token. + * @param array $factors Factors WorkOS returned. + * @param string $workos_user_id WorkOS user, when WorkOS named one (for the per-account code limit). + * @param Profile $profile Active Login Profile. + * @param string $redirect_to Client-requested redirect. + * @param bool $honor_profile_redirect Whether the profile's `post_login_redirect` wins. + * + * @return array|WP_Error + */ + private function workos_challenge( string $pending_token, array $factors, string $workos_user_id, Profile $profile, string $redirect_to, bool $honor_profile_redirect ) { + $allowed = (array) ( $profile->get_mfa()['factors'] ?? [] ); + $listed = []; + foreach ( $factors as $factor ) { + if ( ! is_array( $factor ) || empty( $factor['id'] ) ) { + continue; + } + + $type = (string) ( $factor['type'] ?? '' ); + if ( ! empty( $allowed ) && '' !== $type && ! in_array( $type, $allowed, true ) ) { + return new WP_Error( + 'workos_authkit_factor_not_allowed', + __( 'The multi-factor method returned is not permitted for this login.', 'integration-workos' ), + [ 'status' => 403 ] + ); + } + + $listed[] = [ + 'id' => (string) $factor['id'], + 'type' => $type, + ]; + } + + $token = PendingLogin::park( + [ + 'kind' => PendingLogin::KIND_WORKOS, + 'profile' => $profile->get_slug(), + 'enroll' => false, + 'redirect_to' => $redirect_to, + 'honor_profile_redirect' => $honor_profile_redirect, + 'pending_authentication_token' => $pending_token, + 'workos_user_id' => $workos_user_id, + 'factor_ids' => array_column( $listed, 'id' ), + 'factors' => $listed, + ] + ); + return [ - 'pending_authentication_token' => $pending_token, - 'authentication_factors' => isset( $body['authentication_factors'] ) && is_array( $body['authentication_factors'] ) ? $body['authentication_factors'] : [], + 'mfa_required' => true, + 'mfa_token' => $token, + 'enroll' => false, + 'factors' => $listed, ]; } diff --git a/src/WorkOS/Auth/Login.php b/src/WorkOS/Auth/Login.php index 5e6f170..cf878dd 100644 --- a/src/WorkOS/Auth/Login.php +++ b/src/WorkOS/Auth/Login.php @@ -12,6 +12,8 @@ use WorkOS\Auth\AuthKit\MfaStep; use WorkOS\Auth\AuthKit\Profile; use WorkOS\Auth\AuthKit\ProfileRepository; +use WorkOS\Mfa\PasswordGuard; +use WorkOS\Mfa\PendingLogin; use WorkOS\Vendor\StellarWP\SuperGlobals\SuperGlobals; use WP_Error; @@ -179,6 +181,16 @@ public function authenticate_headless( $user, $username, $password ) { return $wp_user; } + // A user who has or needs the plugin's MFA can't finish here, and + // this attempt mustn't overwrite their stored WorkOS session or + // look like a login to `workos_user_authenticated` listeners. + // PasswordGuard would refuse at priority 100 anyway; refusing now + // keeps the tokens and the hook out of it. + if ( \WorkOS\App::container()->get( PasswordGuard::class )->must_use_mfa( (int) $wp_user->ID ) ) { + $this->refusal = MfaStep::unsupported_error(); + return $this->refusal; + } + // Store tokens. self::store_tokens( $wp_user->ID, $result ); @@ -318,10 +330,12 @@ public function handle_callback(): void { // The plugin's MFA policy wants a code (or an enrollment) the hosted // page didn't ask for. The sign-in is parked; finish it on the - // profile's AuthKit page, which renders just the MFA step. + // profile's AuthKit page, which renders just the MFA step. The URL + // carries a one-time resume code, not the token itself. if ( ! empty( $outcome['mfa_token'] ) ) { nocache_headers(); - wp_safe_redirect( FrontendRoute::url_for_profile( $profile, [ 'workos_mfa' => (string) $outcome['mfa_token'] ] ) ); + header( 'Referrer-Policy: no-referrer' ); + wp_safe_redirect( FrontendRoute::url_for_profile( $profile, [ 'workos_mfa' => PendingLogin::handoff( (string) $outcome['mfa_token'] ) ] ) ); exit; } diff --git a/src/WorkOS/Mfa/AccountChanges.php b/src/WorkOS/Mfa/AccountChanges.php new file mode 100644 index 0000000..0245412 --- /dev/null +++ b/src/WorkOS/Mfa/AccountChanges.php @@ -0,0 +1,161 @@ +sign_out_others( $user_id ); + $this->notify( + $user_id, + 'enrolled', + __( 'Two-step verification: authenticator added', 'integration-workos' ), + __( 'An authenticator app was added to your account, and it will be asked for a code when you sign in.', 'integration-workos' ) + ); + } + + /** + * A factor was removed by its owner. + * + * @param int $user_id WP user ID. + * + * @return void + */ + public function removed( $user_id ): void { + $user_id = (int) $user_id; + $this->sign_out_others( $user_id ); + $this->notify( + $user_id, + 'removed', + __( 'Two-step verification: authenticator removed', 'integration-workos' ), + __( 'An authenticator app was removed from your account.', 'integration-workos' ) + ); + } + + /** + * Every factor was removed by an admin. + * + * @param int $user_id WP user ID. + * + * @return void + */ + public function reset( $user_id ): void { + $user_id = (int) $user_id; + + if ( get_current_user_id() !== $user_id ) { + WP_Session_Tokens::get_instance( $user_id )->destroy_all(); + + $session_id = User::get_session_id( $user_id ); + delete_user_meta( $user_id, User::META_ACCESS_TOKEN ); + delete_user_meta( $user_id, User::META_REFRESH_TOKEN ); + delete_user_meta( $user_id, User::META_SESSION_ID ); + delete_user_meta( $user_id, VerifiedSessions::META ); + if ( '' !== $session_id ) { + workos()->api()->revoke_session( $session_id ); + } + } + + $this->notify( + $user_id, + 'reset', + __( 'Two-step verification: authenticators removed', 'integration-workos' ), + __( 'A site administrator removed every authenticator app from your account, and you were signed out everywhere. If your account requires two-step verification, you’ll set up a new app the next time you sign in.', 'integration-workos' ) + ); + } + + /** + * End the user's other sessions, when the change came from the user. + * + * @param int $user_id WP user ID. + * + * @return void + */ + private function sign_out_others( int $user_id ): void { + if ( $user_id <= 0 || get_current_user_id() !== $user_id ) { + return; + } + + WP_Session_Tokens::get_instance( $user_id )->destroy_others( wp_get_session_token() ); + } + + /** + * Email the account about the change. + * + * @param int $user_id WP user ID. + * @param string $event `enrolled`, `removed` or `reset`. + * @param string $subject Subject, without the site name. + * @param string $message What happened. + * + * @return void + */ + private function notify( int $user_id, string $event, string $subject, string $message ): void { + $user = get_userdata( $user_id ); + if ( ! $user || '' === (string) $user->user_email ) { + return; + } + + /** + * Filters whether to email a user about a change to their MFA. + * + * @param bool $send Default true. + * @param int $user_id WP user ID. + * @param string $event `enrolled`, `removed` or `reset`. + */ + if ( ! apply_filters( 'workos_mfa_notify_user', true, $user_id, $event ) ) { + return; + } + + $site = wp_specialchars_decode( (string) get_bloginfo( 'name' ), ENT_QUOTES ); + $body = sprintf( + /* translators: 1: user display name, 2: what changed, 3: site name, 4: site URL */ + __( "Hi %1\$s,\n\n%2\$s\n\nIf you didn’t do this, contact the administrator of %3\$s right away.\n\n%4\$s", 'integration-workos' ), + $user->display_name, + $message, + $site, + home_url( '/' ) + ); + + wp_mail( $user->user_email, sprintf( '[%s] %s', $site, $subject ), $body ); + } +} diff --git a/src/WorkOS/Mfa/Admin/UserEditPanel.php b/src/WorkOS/Mfa/Admin/UserEditPanel.php index e9ab7de..ba4cbf0 100644 --- a/src/WorkOS/Mfa/Admin/UserEditPanel.php +++ b/src/WorkOS/Mfa/Admin/UserEditPanel.php @@ -21,6 +21,9 @@ * the profile form is saved. * * The user's own screen (`profile.php`) shows the account panel instead. + * + * Changes apply only once core has saved the profile, so a form that fails + * validation changes nothing. A reset WorkOS refused shows as a notice. */ class UserEditPanel { @@ -38,6 +41,13 @@ class UserEditPanel { */ private Policy $policy; + /** + * Changes from the submitted form, applied once the profile saves. + * + * @var array + */ + private array $queued = []; + /** * Constructor. * @@ -57,6 +67,8 @@ public function __construct( FactorService $factors, Policy $policy ) { public function register(): void { add_action( 'edit_user_profile', [ $this, 'render' ], 22 ); add_action( 'edit_user_profile_update', [ $this, 'save' ] ); + add_action( 'profile_update', [ $this, 'apply' ] ); + add_action( 'admin_notices', [ $this, 'notice' ] ); } /** @@ -129,10 +141,11 @@ public function render( WP_User $user ): void { } /** - * Apply the panel on save. + * Read the panel from the submitted form. * * Core has already checked the `update-user_{id}` nonce and `edit_user` - * before this action fires. + * before this action fires, but hasn't validated the rest of the form + * yet; apply() runs once it has. * * @param int $user_id User being saved. * @@ -144,12 +157,70 @@ public function save( int $user_id ): void { return; } - $required = sanitize_text_field( wp_unslash( $_POST['workos_mfa_required'] ) ); - $this->factors->set_required( $user_id, '1' === $required ); + $this->queued[ $user_id ] = [ + 'required' => '1' === sanitize_text_field( wp_unslash( $_POST['workos_mfa_required'] ) ), + 'reset' => ! empty( $_POST['workos_mfa_reset'] ), + ]; + // phpcs:enable WordPress.Security.NonceVerification.Missing + } - if ( ! empty( $_POST['workos_mfa_reset'] ) ) { - $this->factors->reset( $user_id ); + /** + * `profile_update`: apply the queued changes once the profile saved. + * + * @param int $user_id User saved. + * + * @return void + */ + public function apply( $user_id ): void { + $user_id = (int) $user_id; + if ( ! isset( $this->queued[ $user_id ] ) ) { + return; } - // phpcs:enable WordPress.Security.NonceVerification.Missing + + $changes = $this->queued[ $user_id ]; + unset( $this->queued[ $user_id ] ); + + $this->factors->set_required( $user_id, $changes['required'] ); + + if ( $changes['reset'] ) { + $removed = $this->factors->reset( $user_id ); + if ( is_wp_error( $removed ) ) { + set_transient( $this->notice_key(), $removed->get_error_message(), MINUTE_IN_SECONDS ); + } + } + } + + /** + * Show a reset WorkOS refused, on the next screen. + * + * @return void + */ + public function notice(): void { + $message = get_transient( $this->notice_key() ); + if ( ! is_string( $message ) || '' === $message ) { + return; + } + + delete_transient( $this->notice_key() ); + + printf( + '

%s

', + esc_html( + sprintf( + /* translators: %s: error from WorkOS */ + __( 'Couldn’t remove the user’s authenticator apps: %s', 'integration-workos' ), + $message + ) + ) + ); + } + + /** + * Transient key for the current admin's notice. + * + * @return string + */ + private function notice_key(): string { + return 'workos_mfa_reset_error_' . get_current_user_id(); } } diff --git a/src/WorkOS/Mfa/CodeGuard.php b/src/WorkOS/Mfa/CodeGuard.php new file mode 100644 index 0000000..99678c1 --- /dev/null +++ b/src/WorkOS/Mfa/CodeGuard.php @@ -0,0 +1,214 @@ +rate_limiter = $rate_limiter; + } + + /** + * Subject for a WP user. + * + * @param int $user_id WP user ID. + * + * @return string + */ + public static function for_user( int $user_id ): string { + return 'wp:' . $user_id; + } + + /** + * Subject for a WorkOS user not matched to a WP user yet. + * + * @param string $workos_user_id WorkOS user ID. + * + * @return string + */ + public static function for_workos_user( string $workos_user_id ): string { + return 'workos:' . $workos_user_id; + } + + /** + * Start a code check: take the account's lock and charge one attempt. + * + * Every successful begin() must be followed by passed() or failed(). + * + * @param string $subject Account subject (for_user() / for_workos_user()). + * + * @return true|WP_Error + */ + public function begin( string $subject ) { + if ( ! $this->lock( $subject ) ) { + return new WP_Error( + 'workos_mfa_busy', + __( 'Another code is being checked for this account. Try again in a moment.', 'integration-workos' ), + [ 'status' => 429 ] + ); + } + + $charged = $this->rate_limiter->attempt( self::BUCKET, $subject, self::POLICY ); + if ( is_wp_error( $charged ) ) { + $this->unlock( $subject ); + return new WP_Error( + 'workos_mfa_too_many_codes', + __( 'Too many wrong codes for this account. Wait a while, then try again.', 'integration-workos' ), + [ 'status' => 429 ] + ); + } + + return true; + } + + /** + * The code was right: refund the attempt and release the lock. + * + * @param string $subject Account subject. + * + * @return void + */ + public function passed( string $subject ): void { + $this->rate_limiter->refund( self::BUCKET, $subject, self::POLICY ); + $this->unlock( $subject ); + } + + /** + * The code was wrong, or the check didn't finish: keep the attempt + * counted and release the lock. + * + * @param string $subject Account subject. + * + * @return void + */ + public function failed( string $subject ): void { + $this->unlock( $subject ); + } + + /** + * Take the account's lock. + * + * With a persistent object cache, `wp_cache_add()` is atomic. Without + * one, an `INSERT IGNORE` on the options table's unique key is. + * + * @param string $subject Account subject. + * + * @return bool + */ + public function lock( string $subject ): bool { + $key = $this->lock_key( $subject ); + + if ( wp_using_ext_object_cache() ) { + return wp_cache_add( $key, time(), 'workos_mfa', self::LOCK_TTL ); + } + + global $wpdb; + + // phpcs:disable WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching + $wpdb->query( + $wpdb->prepare( + 'DELETE FROM %i WHERE option_name = %s AND option_value < %d', + $wpdb->options, + $key, + time() - self::LOCK_TTL + ) + ); + $inserted = $wpdb->query( + $wpdb->prepare( + "INSERT IGNORE INTO %i (option_name, option_value, autoload) VALUES (%s, %s, 'no')", + $wpdb->options, + $key, + (string) time() + ) + ); + // phpcs:enable + + return 1 === (int) $inserted; + } + + /** + * Release the account's lock. + * + * @param string $subject Account subject. + * + * @return void + */ + public function unlock( string $subject ): void { + $key = $this->lock_key( $subject ); + + if ( wp_using_ext_object_cache() ) { + wp_cache_delete( $key, 'workos_mfa' ); + return; + } + + global $wpdb; + $wpdb->delete( $wpdb->options, [ 'option_name' => $key ] ); // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching + } + + /** + * Lock key for a subject. + * + * @param string $subject Account subject. + * + * @return string + */ + private function lock_key( string $subject ): string { + return '_workos_mfa_lock_' . md5( $subject ); + } +} diff --git a/src/WorkOS/Mfa/Controller.php b/src/WorkOS/Mfa/Controller.php index 4a8b7c5..e30e347 100644 --- a/src/WorkOS/Mfa/Controller.php +++ b/src/WorkOS/Mfa/Controller.php @@ -10,8 +10,9 @@ use WorkOS\Contracts\Controller as BaseController; /** - * Binds the MFA settings, policy and factor service, the guard that keeps - * password-only sign-ins from skipping MFA, and the account panel (REST, + * Binds the MFA settings, policy and factor service, the guards that keep + * password-only sign-ins and outside Bearer tokens from skipping MFA, the + * emails and sign-outs around a change, and the account panel (REST, * assets, `[workos:mfa]`, profile screen). * * Sign-in enforcement lives in `Auth\AuthKit\LoginCompleter`, and the @@ -27,8 +28,11 @@ class Controller extends BaseController { protected function doRegister(): void { $this->container->singleton( Settings::class ); $this->container->singleton( Policy::class ); + $this->container->singleton( CodeGuard::class ); $this->container->singleton( FactorService::class ); $this->container->singleton( PasswordGuard::class ); + $this->container->singleton( VerifiedSessions::class ); + $this->container->singleton( AccountChanges::class ); $this->container->singleton( RestApi::class ); $this->container->singleton( Assets::class ); $this->container->singleton( Shortcode::class ); @@ -36,6 +40,8 @@ protected function doRegister(): void { $this->container->singleton( Admin\UserEditPanel::class ); $this->container->get( PasswordGuard::class )->register(); + $this->container->get( VerifiedSessions::class )->register(); + $this->container->get( AccountChanges::class )->register(); $this->container->get( RestApi::class )->register(); $this->container->get( Assets::class )->register(); $this->container->get( Shortcode::class )->register(); diff --git a/src/WorkOS/Mfa/FactorService.php b/src/WorkOS/Mfa/FactorService.php index f0738dc..efe071c 100644 --- a/src/WorkOS/Mfa/FactorService.php +++ b/src/WorkOS/Mfa/FactorService.php @@ -8,6 +8,7 @@ namespace WorkOS\Mfa; use WorkOS\ActivityLog\EventLogger; +use WorkOS\App; use WorkOS\User; use WP_Error; @@ -22,7 +23,11 @@ * * WorkOS holds the factors and their secrets. WordPress mirrors only: * - * - `_workos_mfa_enabled`: the user has at least one confirmed factor. + * - `_workos_mfa_enabled`: the user turned MFA on. Set when a factor is + * confirmed, or found in WorkOS; cleared only when the user removes their + * last factor or an admin resets them. Factors that vanish from WorkOS + * some other way leave it set, so the next sign-in asks the user to + * enroll again instead of quietly dropping their second factor. * - `_workos_mfa_required`: an admin requires MFA for the user. * - `_workos_mfa_pending_factor`: a factor enrolled but not yet confirmed * with a code. It never counts as enabled, and is deleted from WorkOS @@ -37,15 +42,32 @@ class FactorService { */ public const PENDING_FACTOR_TTL = DAY_IN_SECONDS; + /** + * Per-account lock and code limit. + * + * @var CodeGuard + */ + private CodeGuard $guard; + + /** + * Constructor. + * + * @param CodeGuard|null $guard Per-account lock and code limit; the container's when omitted. + */ + public function __construct( ?CodeGuard $guard = null ) { + $this->guard = $guard ?? App::container()->get( CodeGuard::class ); + } + /** * The user's MFA status, reconciled with WorkOS. * * @param int $user_id WP user ID. * * @return array|WP_Error { - * @type bool $enabled Whether the user has a confirmed factor. - * @type bool $required Whether an admin set the require flag (roles and filters: see Policy::is_required()). - * @type array $factors Confirmed factors: `{ id, type, created }`. + * @type bool $enabled Whether the user has a confirmed factor. + * @type bool $required Whether an admin set the require flag. + * @type bool $must_use Whether the user must use MFA for any reason (flag, role, filter). + * @type array $factors Confirmed factors: `{ id, type, created }`. * } */ public function status( int $user_id ) { @@ -55,8 +77,9 @@ public function status( int $user_id ) { } return [ - 'enabled' => $this->is_enabled( $user_id ), + 'enabled' => ! empty( $factors ), 'required' => $this->is_required_flag( $user_id ), + 'must_use' => App::container()->get( Policy::class )->is_required( $user_id ), 'factors' => $factors, ]; } @@ -64,9 +87,9 @@ public function status( int $user_id ) { /** * Confirmed factors, read from WorkOS. * - * Also keeps `_workos_mfa_enabled` in step with WorkOS, since factors - * can be added or removed outside WordPress, and drops a stale - * unconfirmed factor. + * Sets `_workos_mfa_enabled` when WorkOS has a factor the flag didn't + * know about (added outside WordPress, or before this version). Never + * clears it; see the class docblock. Drops a stale unconfirmed factor. * * @param int $user_id WP user ID. * @@ -84,14 +107,12 @@ public function list_factors( int $user_id ) { } $pending = $this->pending_factor( $user_id ); - if ( $pending && ( time() - $pending['created'] ) > self::PENDING_FACTOR_TTL ) { - workos()->api()->delete_auth_factor( $pending['factor_id'] ); - delete_user_meta( $user_id, User::META_MFA_PENDING_FACTOR ); + if ( $pending && ( time() - $pending['created'] ) > self::PENDING_FACTOR_TTL && $this->drop_pending( $user_id, $pending ) ) { $pending = null; } $factors = []; - foreach ( (array) ( $result['data'] ?? $result ) as $factor ) { + foreach ( (array) ( $result['data'] ?? [] ) as $factor ) { if ( ! is_array( $factor ) || empty( $factor['id'] ) ) { continue; } @@ -105,7 +126,9 @@ public function list_factors( int $user_id ) { ]; } - $this->sync_enabled( $user_id, ! empty( $factors ) ); + if ( ! empty( $factors ) ) { + $this->sync_enabled( $user_id, true ); + } return $factors; } @@ -115,7 +138,12 @@ public function list_factors( int $user_id ) { * * Creates the factor in WorkOS and remembers it as unconfirmed. Any * earlier unconfirmed factor is deleted first, so restarting doesn't - * pile up factors the user never finished. + * pile up factors the user never finished. Only one enrollment per + * account runs at a time: two at once would leave a factor nobody + * tracks as unconfirmed, which would then count as confirmed. + * + * Doesn't check whether the user already has a factor: callers that + * shouldn't let a session add a second one without a code check first. * * @param int $user_id WP user ID. * @@ -127,28 +155,44 @@ public function start_enrollment( int $user_id ) { return $workos_id; } - $pending = $this->pending_factor( $user_id ); - if ( $pending ) { - workos()->api()->delete_auth_factor( $pending['factor_id'] ); - delete_user_meta( $user_id, User::META_MFA_PENDING_FACTOR ); + $subject = 'enroll:' . $user_id; + if ( ! $this->guard->lock( $subject ) ) { + return new WP_Error( + 'workos_mfa_busy', + __( 'Another setup is in progress for this account. Try again in a moment.', 'integration-workos' ), + [ 'status' => 429 ] + ); } - $user = get_userdata( $user_id ); - $enroll = $this->enroll_totp( $workos_id, $user ? (string) $user->user_email : '', $user_id ); - if ( is_wp_error( $enroll ) ) { - return $enroll; - } + try { + $pending = $this->pending_factor( $user_id ); + if ( $pending && ! $this->drop_pending( $user_id, $pending ) ) { + return new WP_Error( + 'workos_mfa_pending_cleanup_failed', + __( 'Couldn’t clear the earlier setup. Try again in a moment.', 'integration-workos' ), + [ 'status' => 502 ] + ); + } - update_user_meta( - $user_id, - User::META_MFA_PENDING_FACTOR, - [ - 'factor_id' => $enroll['factor_id'], - 'created' => time(), - ] - ); + $user = get_userdata( $user_id ); + $enroll = $this->enroll_totp( $workos_id, $user ? (string) $user->user_email : '', $user_id ); + if ( is_wp_error( $enroll ) ) { + return $enroll; + } - return $enroll; + update_user_meta( + $user_id, + User::META_MFA_PENDING_FACTOR, + [ + 'factor_id' => $enroll['factor_id'], + 'created' => time(), + ] + ); + + return $enroll; + } finally { + $this->guard->unlock( $subject ); + } } /** @@ -216,7 +260,7 @@ public function confirm_enrollment( int $user_id, string $factor_id, string $cod return $this->factor_not_found(); } - $valid = $this->verify_code( $factor_id, $code, $challenge_id ); + $valid = $this->check_code( $user_id, $factor_id, $code, $challenge_id ); if ( is_wp_error( $valid ) ) { return $valid; } @@ -235,10 +279,11 @@ public function confirm_enrollment( int $user_id, string $factor_id, string $cod * * @param int $user_id WP user ID. * @param string $factor_id Confirmed factor. + * @param string $type Factor type. * * @return void */ - public function note_enrolled( int $user_id, string $factor_id ): void { + public function note_enrolled( int $user_id, string $factor_id, string $type = 'totp' ): void { $this->sync_enabled( $user_id, true ); EventLogger::log( @@ -247,7 +292,8 @@ public function note_enrolled( int $user_id, string $factor_id ): void { 'user_id' => $user_id, 'metadata' => [ 'factor_id' => $factor_id, - 'factor_type' => 'totp', + 'factor_type' => $type, + 'actor_id' => get_current_user_id(), ], ] ); @@ -263,17 +309,50 @@ public function note_enrolled( int $user_id, string $factor_id ): void { $user_id, [ 'id' => $factor_id, - 'type' => 'totp', + 'type' => $type, ] ); } /** - * Check a code against one of the user's factors. + * Check a code for a user, counted against the account's code limit. + * + * The way every surface checks a code: the sign-in step, confirming an + * enrollment, removing a factor, adding a second one. A wrong code + * counts toward {@see CodeGuard::POLICY}; a right one doesn't. + * + * @param int $user_id WP user ID. + * @param string $factor_id Factor ID (the caller has checked it's the user's). + * @param string $code Code from the authenticator app. + * @param string $challenge_id Existing challenge; a new one is created when empty. + * + * @return true|WP_Error + */ + public function check_code( int $user_id, string $factor_id, string $code, string $challenge_id = '' ) { + $subject = CodeGuard::for_user( $user_id ); + $begun = $this->guard->begin( $subject ); + if ( is_wp_error( $begun ) ) { + return $begun; + } + + $valid = $this->verify_code( $factor_id, $code, $challenge_id ); + + if ( true === $valid ) { + $this->guard->passed( $subject ); + } else { + $this->guard->failed( $subject ); + } + + return $valid; + } + + /** + * Check a code against a factor, with no account limit. * - * Used to confirm an enrollment and, in plugin mode, to answer the - * sign-in challenge. Doesn't fire hooks: callers know which of the two - * it was. + * Callers outside sign-in and self-service should use check_code(). + * Doesn't fire hooks: callers know what the code was for. A challenge + * passed in must belong to the factor; WorkOS would accept a code for + * any factor's challenge. * * @param string $factor_id Factor ID. * @param string $code Code from the authenticator app. @@ -302,7 +381,72 @@ public function verify_code( string $factor_id, string $code, string $challenge_ return ( $status >= 400 && $status < 500 ) ? $this->invalid_code() : $result; } - return empty( $result['valid'] ) ? $this->invalid_code() : true; + if ( empty( $result['valid'] ) ) { + return $this->invalid_code(); + } + + $challenged = (string) ( $result['challenge']['authentication_factor_id'] ?? '' ); + + return $challenged === $factor_id ? true : $this->invalid_code(); + } + + /** + * Prove the caller holds one of the user's factors, if they have any. + * + * For actions a borrowed session shouldn't be able to take alone, like + * adding a second authenticator: with a factor already on the account, + * a current code from it is required. Counted against the account's + * code limit. + * + * @param int $user_id WP user ID. + * @param string $factor_id One of the user's factors. + * @param string $code Current code from it. + * + * @return true|WP_Error True when the user has no factor yet, or the code checks out. + */ + public function require_current_factor( int $user_id, string $factor_id, string $code ) { + $factors = $this->list_factors( $user_id ); + if ( is_wp_error( $factors ) ) { + return $factors; + } + + if ( empty( $factors ) ) { + return true; + } + + if ( '' === $factor_id || '' === $code ) { + return new WP_Error( + 'workos_mfa_code_required', + __( 'Enter a current code from your authenticator app to add another one.', 'integration-workos' ), + [ 'status' => 403 ] + ); + } + + if ( ! in_array( $factor_id, array_column( $factors, 'id' ), true ) ) { + return $this->factor_not_found(); + } + + return $this->check_code( $user_id, $factor_id, $code ); + } + + /** + * The same error without the WorkOS response body. + * + * `Api\Client` errors carry WorkOS's body (user and factor details), + * which REST would hand to the browser. + * + * @param WP_Error $error Error. + * + * @return WP_Error + */ + public static function without_body( WP_Error $error ): WP_Error { + $data = $error->get_error_data(); + + return new WP_Error( + $error->get_error_code(), + $error->get_error_message(), + [ 'status' => is_array( $data ) ? (int) ( $data['status'] ?? 500 ) : 500 ] + ); } /** @@ -337,7 +481,10 @@ public function delete_factor( int $user_id, string $factor_id ) { 'mfa.removed', [ 'user_id' => $user_id, - 'metadata' => [ 'factor_id' => $factor_id ], + 'metadata' => [ + 'factor_id' => $factor_id, + 'actor_id' => get_current_user_id(), + ], ] ); @@ -374,7 +521,7 @@ public function reset( int $user_id ) { } $removed = 0; - foreach ( (array) ( $result['data'] ?? $result ) as $factor ) { + foreach ( (array) ( $result['data'] ?? [] ) as $factor ) { if ( ! is_array( $factor ) || empty( $factor['id'] ) ) { continue; } @@ -489,6 +636,28 @@ public function pending_factor( int $user_id ): ?array { ]; } + /** + * Delete an unconfirmed factor from WorkOS and forget it. + * + * Kept when WorkOS couldn't delete it, so it's never mistaken for a + * confirmed factor; a later call tries again. + * + * @param int $user_id WP user ID. + * @param array $pending Unconfirmed factor from pending_factor(). + * + * @return bool Whether it's gone. + */ + private function drop_pending( int $user_id, array $pending ): bool { + $deleted = workos()->api()->delete_auth_factor( $pending['factor_id'] ); + if ( is_wp_error( $deleted ) && 404 !== (int) ( $deleted->get_error_data()['status'] ?? 0 ) ) { + return false; + } + + delete_user_meta( $user_id, User::META_MFA_PENDING_FACTOR ); + + return true; + } + /** * Linked WorkOS user ID, or an error. * diff --git a/src/WorkOS/Mfa/PasswordGuard.php b/src/WorkOS/Mfa/PasswordGuard.php index c54577f..f003abb 100644 --- a/src/WorkOS/Mfa/PasswordGuard.php +++ b/src/WorkOS/Mfa/PasswordGuard.php @@ -8,6 +8,9 @@ namespace WorkOS\Mfa; use WorkOS\Auth\AuthKit\MfaStep; +use WorkOS\Auth\AuthKit\Profile; +use WorkOS\Auth\AuthKit\ProfileRepository; +use WorkOS\User; use WP_Error; use WP_User; @@ -23,7 +26,10 @@ * be a way around it. They're refused instead. * * Application passwords are exempt: they're for API clients, can't answer - * a challenge, and are created by a user who is already signed in. + * a challenge, and are created by a user who is already signed in. The + * exemption covers only the user the application password belongs to, in + * the sign-in it authenticated: an XML-RPC `system.multicall` runs several + * sign-ins in one request. * * In `workos` mode WorkOS asks for the code, but a WordPress password * never reaches WorkOS. Sites in that mode should turn off the password @@ -45,15 +51,31 @@ class PasswordGuard { */ private Policy $policy; + /** + * Login Profiles. + * + * @var ProfileRepository + */ + private ProfileRepository $profiles; + + /** + * User the current sign-in authenticated with an application password. + * + * @var int + */ + private int $app_password_user = 0; + /** * Constructor. * - * @param Settings $settings MFA settings. - * @param Policy $policy MFA policy. + * @param Settings $settings MFA settings. + * @param Policy $policy MFA policy. + * @param ProfileRepository $profiles Login Profiles. */ - public function __construct( Settings $settings, Policy $policy ) { + public function __construct( Settings $settings, Policy $policy, ProfileRepository $profiles ) { $this->settings = $settings; $this->policy = $policy; + $this->profiles = $profiles; } /** @@ -62,8 +84,59 @@ public function __construct( Settings $settings, Policy $policy ) { * @return void */ public function register(): void { + // Before every authenticator: each sign-in starts with no exemption. + add_filter( 'authenticate', [ $this, 'reset' ], 0 ); // After every authenticator (core's run at 20, spam checks at 99). add_filter( 'authenticate', [ $this, 'guard' ], 100 ); + add_action( 'application_password_did_authenticate', [ $this, 'note_app_password' ] ); + add_filter( 'workos_relink_protected', [ $this, 'protect_link' ], 10, 2 ); + } + + /** + * `workos_relink_protected` filter: never re-link an account with MFA. + * + * A re-link points the account at another WorkOS user, whose factors + * are not the owner's. Whoever controls that identity would get past + * the second factor, or enroll their own. An administrator can still + * reassign it with `wp workos user link --force`. + * + * @param bool $is_protected Whether Re-link Protection applies. + * @param int $wp_user_id WP user ID. + * + * @return bool + */ + public function protect_link( $is_protected, $wp_user_id ): bool { + if ( $is_protected ) { + return true; + } + + $wp_user_id = (int) $wp_user_id; + + return $wp_user_id > 0 && ( $this->policy->is_enrolled( $wp_user_id ) || $this->policy->is_required( $wp_user_id ) ); + } + + /** + * `authenticate` filter (first): forget the previous sign-in's exemption. + * + * @param WP_User|WP_Error|null $user Result so far. + * + * @return WP_User|WP_Error|null + */ + public function reset( $user ) { + $this->app_password_user = 0; + + return $user; + } + + /** + * `application_password_did_authenticate`: remember whose it was. + * + * @param WP_User $user User the application password belongs to. + * + * @return void + */ + public function note_app_password( $user ): void { + $this->app_password_user = $user instanceof WP_User ? (int) $user->ID : 0; } /** @@ -74,7 +147,7 @@ public function register(): void { * @return WP_User|WP_Error|null */ public function guard( $user ) { - if ( ! $user instanceof WP_User || did_action( 'application_password_did_authenticate' ) ) { + if ( ! $user instanceof WP_User || ( $this->app_password_user > 0 && $this->app_password_user === (int) $user->ID ) ) { return $user; } @@ -84,6 +157,12 @@ public function guard( $user ) { /** * Whether a password-only sign-in would skip this user's MFA. * + * True when the user has a factor or must use MFA, and, for a user + * linked to WorkOS, when the default Login Profile would ask for a code + * or an enrollment (`enforce = always`). Accounts not linked to WorkOS + * aren't held to a profile setting, so turning on `always` can't lock + * out a site's local administrators. + * * @param int $user_id WP user ID. * * @return bool @@ -95,6 +174,11 @@ public function must_use_mfa( int $user_id ): bool { $must = $this->policy->is_enrolled( $user_id ) || $this->policy->is_required( $user_id ); + if ( ! $must && User::is_sso( $user_id ) ) { + $profile = $this->profiles->find_by_slug( Profile::DEFAULT_SLUG ); + $must = $profile && Policy::NONE !== $this->policy->decide( $user_id, $profile ); + } + /** * Filters whether a password-only sign-in may skip MFA for a user. * diff --git a/src/WorkOS/Mfa/PendingLogin.php b/src/WorkOS/Mfa/PendingLogin.php index 3468528..aef2554 100644 --- a/src/WorkOS/Mfa/PendingLogin.php +++ b/src/WorkOS/Mfa/PendingLogin.php @@ -23,7 +23,10 @@ * * The browser only holds the random `mfa_token`. The record is keyed on a * hash of it, bound to the Login Profile it started on, used once, capped - * at a few wrong codes, and gone after a few minutes. + * at a few wrong codes, and gone a few minutes after it was parked, however + * often it's touched. The parked WorkOS response is encrypted with a key + * derived from the token, which the server never stores, so the tokens in + * it can't be read from the database. */ class PendingLogin { @@ -47,6 +50,16 @@ class PendingLogin { */ private const TTL = 10 * MINUTE_IN_SECONDS; + /** + * Challenges remembered per sign-in (the newest ones). + */ + private const MAX_CHALLENGES = 5; + + /** + * Lifetime of a resume code, in seconds. + */ + private const RESUME_TTL = MINUTE_IN_SECONDS; + /** * Park a sign-in and return the token the browser carries. * @@ -87,7 +100,9 @@ public static function park( array $record ): string { $record ); - set_transient( self::key( $token ), $record, self::ttl() ); + $record['expires_at'] = time() + self::ttl(); + + set_transient( self::key( $token ), self::seal( $token, $record ), self::ttl() ); return $token; } @@ -110,14 +125,20 @@ public static function get( string $token, string $profile_slug ): ?array { return null; } - return $record; + if ( (int) ( $record['expires_at'] ?? 0 ) <= time() ) { + self::forget( $token ); + return null; + } + + return self::open( $token, $record ); } /** * Save changes to a parked sign-in (factors, challenges, attempts). * - * The lifetime restarts, which is fine: wrong codes are capped, and the - * record is deleted as soon as the sign-in finishes. + * Keeps the original expiry, so touching a sign-in never extends it. + * A record that's already gone (finished, expired, or forgotten by a + * request running alongside) isn't brought back. * * @param string $token Token from the browser. * @param array $record Updated record. @@ -125,7 +146,15 @@ public static function get( string $token, string $profile_slug ): ?array { * @return void */ public static function update( string $token, array $record ): void { - set_transient( self::key( $token ), $record, self::ttl() ); + $left = (int) ( $record['expires_at'] ?? 0 ) - time(); + if ( $left <= 0 || false === get_transient( self::key( $token ) ) ) { + return; + } + + $record['challenges'] = array_slice( array_values( (array) ( $record['challenges'] ?? [] ) ), -self::MAX_CHALLENGES ); + $record['challenge_factors'] = array_intersect_key( (array) ( $record['challenge_factors'] ?? [] ), array_flip( $record['challenges'] ) ); + + set_transient( self::key( $token ), self::seal( $token, $record ), $left ); } /** @@ -159,6 +188,44 @@ public static function forget( string $token ): void { delete_transient( self::key( $token ) ); } + /** + * A one-time code that stands in for the token in a URL. + * + * The hosted sign-in has to send the browser back to the login page, + * and a URL ends up in history, logs and Referer headers. The code in + * it works once, for a minute, and only to get the real token into the + * page that resumes the sign-in. + * + * @param string $token Token of a parked sign-in. + * + * @return string + */ + public static function handoff( string $token ): string { + $code = wp_generate_password( 32, false, false ); + + set_transient( self::resume_key( $code ), $token, self::RESUME_TTL ); + + return $code; + } + + /** + * Swap a resume code for the token. Works once. + * + * @param string $code Code from the URL. + * + * @return string Token, or '' when the code is unknown, used or expired. + */ + public static function redeem( string $code ): string { + if ( '' === $code ) { + return ''; + } + + $token = get_transient( self::resume_key( $code ) ); + delete_transient( self::resume_key( $code ) ); + + return is_string( $token ) ? $token : ''; + } + /** * Lifetime in seconds. * @@ -183,4 +250,81 @@ private static function ttl(): int { private static function key( string $token ): string { return 'workos_mfa_pending_' . substr( hash( 'sha256', $token ), 0, 40 ); } + + /** + * Transient key for a resume code. + * + * @param string $code Resume code. + * + * @return string + */ + private static function resume_key( string $code ): string { + return 'workos_mfa_resume_' . substr( hash( 'sha256', $code ), 0, 40 ); + } + + /** + * Encrypt the parked WorkOS response for storage. + * + * @param string $token Token from the browser. + * @param array $record Record with a plain `response`. + * + * @return array + */ + private static function seal( string $token, array $record ): array { + if ( ! isset( $record['response'] ) ) { + return $record; + } + + $nonce = random_bytes( SODIUM_CRYPTO_SECRETBOX_NONCEBYTES ); + $plain = (string) wp_json_encode( $record['response'] ); + $record['response'] = base64_encode( $nonce . sodium_crypto_secretbox( $plain, $nonce, self::secret( $token ) ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_encode + + return $record; + } + + /** + * Decrypt the parked WorkOS response. + * + * @param string $token Token from the browser. + * @param array $record Stored record. + * + * @return array|null Record with a plain `response`, or null when it can't be read. + */ + private static function open( string $token, array $record ): ?array { + if ( ! isset( $record['response'] ) ) { + return $record; + } + + $raw = base64_decode( (string) $record['response'], true ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_decode + if ( false === $raw || strlen( $raw ) <= SODIUM_CRYPTO_SECRETBOX_NONCEBYTES ) { + return null; + } + + $plain = sodium_crypto_secretbox_open( + substr( $raw, SODIUM_CRYPTO_SECRETBOX_NONCEBYTES ), + substr( $raw, 0, SODIUM_CRYPTO_SECRETBOX_NONCEBYTES ), + self::secret( $token ) + ); + if ( false === $plain ) { + return null; + } + + $record['response'] = json_decode( $plain, true ); + + return is_array( $record['response'] ) ? $record : null; + } + + /** + * Encryption key for a token's record. + * + * Derived from the token and the site's salt; the storage key is a + * different hash of the token, so it doesn't reveal this one. + * + * @param string $token Token from the browser. + * + * @return string 32 raw bytes. + */ + private static function secret( string $token ): string { + return hash_hmac( 'sha256', 'workos_mfa_response|' . $token, wp_salt( 'auth' ), true ); + } } diff --git a/src/WorkOS/Mfa/RestApi.php b/src/WorkOS/Mfa/RestApi.php index 183b3ab..774dafa 100644 --- a/src/WorkOS/Mfa/RestApi.php +++ b/src/WorkOS/Mfa/RestApi.php @@ -205,7 +205,7 @@ public function set_required( WP_REST_Request $request ) { public function reset( WP_REST_Request $request ) { $removed = $this->factors->reset( (int) $request->get_param( 'id' ) ); if ( is_wp_error( $removed ) ) { - return $removed; + return FactorService::without_body( $removed ); } return new WP_REST_Response( [ 'removed' => $removed ], 200 ); @@ -256,18 +256,23 @@ public function can_manage_own( WP_REST_Request $request ) { /** * Whether the current user may administer another user's MFA. * + * Never your own account: WordPress grants `edit_user` on yourself to + * every role, so a session someone else got hold of could reset the + * factors and clear the require flag without a code. Your own factors + * go through the account routes, which ask for one. + * * @param int $user_id Target WP user ID. * * @return true|WP_Error */ public static function can_manage_user( int $user_id ) { - $can = $user_id > 0 && current_user_can( 'edit_user', $user_id ); + $can = $user_id > 0 && get_current_user_id() !== $user_id && current_user_can( 'edit_user', $user_id ); /** * Filters whether the current user may administer a user's MFA * (read the status, reset factors, set the require flag). * - * @param bool $can Default: `edit_user` on the target. + * @param bool $can Default: `edit_user` on the target, and not yourself. * @param int $user_id Target WP user ID. */ $can = (bool) apply_filters( 'workos_mfa_can_manage_user', $can, $user_id ); @@ -289,12 +294,16 @@ public static function can_manage_user( int $user_id ) { public function status( WP_REST_Request $request ) { $status = $this->factors->status( (int) $request->get_param( 'id' ) ); - return is_wp_error( $status ) ? $status : new WP_REST_Response( $status, 200 ); + return is_wp_error( $status ) ? FactorService::without_body( $status ) : new WP_REST_Response( $status, 200 ); } /** * POST /users/{id}/mfa/enroll * + * With a factor already on the account, takes `factor_id` and a current + * `code` from it, so a session someone else got hold of can't add a + * second authenticator of their own. + * * @param WP_REST_Request $request REST request. * * @return WP_REST_Response|WP_Error @@ -307,9 +316,14 @@ public function enroll( WP_REST_Request $request ) { return $limited; } + $holder = $this->factors->require_current_factor( $user_id, (string) $request->get_param( 'factor_id' ), (string) $request->get_param( 'code' ) ); + if ( is_wp_error( $holder ) ) { + return FactorService::without_body( $holder ); + } + $result = $this->factors->start_enrollment( $user_id ); - return is_wp_error( $result ) ? $result : new WP_REST_Response( $result, 201 ); + return is_wp_error( $result ) ? FactorService::without_body( $result ) : new WP_REST_Response( $result, 201 ); } /** @@ -334,7 +348,7 @@ public function confirm( WP_REST_Request $request ) { (string) $request->get_param( 'challenge_id' ) ); - return is_wp_error( $result ) ? $result : new WP_REST_Response( $result, 200 ); + return is_wp_error( $result ) ? FactorService::without_body( $result ) : new WP_REST_Response( $result, 200 ); } /** @@ -358,20 +372,20 @@ public function remove( WP_REST_Request $request ) { $factors = $this->factors->list_factors( $user_id ); if ( is_wp_error( $factors ) ) { - return $factors; + return FactorService::without_body( $factors ); } if ( ! in_array( $factor_id, array_column( $factors, 'id' ), true ) ) { return $this->error( 'workos_mfa_factor_not_found', __( 'Factor not found.', 'integration-workos' ), 404 ); } - $valid = $this->factors->verify_code( $factor_id, (string) $request->get_param( 'code' ) ); + $valid = $this->factors->check_code( $user_id, $factor_id, (string) $request->get_param( 'code' ) ); if ( is_wp_error( $valid ) ) { - return $valid; + return FactorService::without_body( $valid ); } $result = $this->factors->delete_factor( $user_id, $factor_id ); if ( is_wp_error( $result ) ) { - return $result; + return FactorService::without_body( $result ); } $status = $this->factors->status( $user_id ); diff --git a/src/WorkOS/Mfa/VerifiedSessions.php b/src/WorkOS/Mfa/VerifiedSessions.php new file mode 100644 index 0000000..565ac61 --- /dev/null +++ b/src/WorkOS/Mfa/VerifiedSessions.php @@ -0,0 +1,155 @@ +settings = $settings; + $this->policy = $policy; + } + + /** + * Register hooks. + * + * @return void + */ + public function register(): void { + add_filter( 'workos_bearer_token_user', [ $this, 'gate' ], 10, 2 ); + } + + /** + * Remember the session in a WorkOS response that just signed in here. + * + * @param int $user_id WP user ID. + * @param array $response WorkOS authenticate response. + * + * @return void + */ + public static function remember( int $user_id, array $response ): void { + $sid = self::session_id( (string) ( $response['access_token'] ?? '' ) ); + if ( '' === $sid ) { + return; + } + + $sessions = array_values( array_diff( self::sessions( $user_id ), [ $sid ] ) ); + $sessions[] = $sid; + + update_user_meta( $user_id, self::META, array_slice( $sessions, -self::MAX ) ); + } + + /** + * `workos_bearer_token_user` filter: refuse a token that skipped MFA. + * + * @param int $wp_user_id WP user the token maps to (0 for none). + * @param array $payload Verified JWT claims. + * + * @return int + */ + public function gate( $wp_user_id, $payload ): int { + $wp_user_id = (int) $wp_user_id; + if ( $wp_user_id <= 0 || Settings::MODE_PLUGIN !== $this->settings->mode() ) { + return $wp_user_id; + } + + if ( ! $this->policy->is_enrolled( $wp_user_id ) && ! $this->policy->is_required( $wp_user_id ) ) { + return $wp_user_id; + } + + $sid = is_array( $payload ) ? (string) ( $payload['sid'] ?? '' ) : ''; + $passed = '' !== $sid && in_array( $sid, self::sessions( $wp_user_id ), true ); + + /** + * Filters whether a Bearer token may act for a user who has or needs MFA. + * + * By default only tokens from sessions that signed in through this + * site (and so passed its MFA step) are accepted. + * + * @param bool $passed Whether the token's session signed in here. + * @param int $wp_user_id WP user ID. + * @param array $payload Verified JWT claims. + */ + return apply_filters( 'workos_mfa_accept_bearer_token', $passed, $wp_user_id, $payload ) ? $wp_user_id : 0; + } + + /** + * Session IDs that signed in here. + * + * @param int $user_id WP user ID. + * + * @return string[] + */ + private static function sessions( int $user_id ): array { + $sessions = get_user_meta( $user_id, self::META, true ); + + return is_array( $sessions ) ? array_map( 'strval', $sessions ) : []; + } + + /** + * The `sid` claim of an access token WorkOS just issued. + * + * Not verified: the token came straight from WorkOS in this request. + * + * @param string $token Access token. + * + * @return string + */ + private static function session_id( string $token ): string { + $parts = explode( '.', $token ); + if ( count( $parts ) < 2 ) { + return ''; + } + + $payload = json_decode( (string) base64_decode( strtr( $parts[1], '-_', '+/' ) ), true ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_decode + + return is_array( $payload ) ? (string) ( $payload['sid'] ?? '' ) : ''; + } +} diff --git a/src/WorkOS/REST/Auth/Mfa.php b/src/WorkOS/REST/Auth/Mfa.php index e92cee5..43bf772 100644 --- a/src/WorkOS/REST/Auth/Mfa.php +++ b/src/WorkOS/REST/Auth/Mfa.php @@ -11,9 +11,11 @@ use WorkOS\App; use WorkOS\Auth\AuthKit\Profile; use WorkOS\Auth\Deactivation; +use WorkOS\Mfa\CodeGuard; use WorkOS\Mfa\FactorService; use WorkOS\Mfa\PendingLogin; use WorkOS\RateLimit\TieredRateLimiter; +use WorkOS\User; use WP_Error; use WP_REST_Request; use WP_REST_Response; @@ -23,13 +25,12 @@ /** * MFA (TOTP + SMS) endpoints used during login *and* account settings. * - * During login, the browser carries either WorkOS's - * `pending_authentication_token` (WorkOS asked for a code) or the plugin's - * `mfa_token` (the plugin's MFA policy asked for a code, or the user must - * enroll first; see `Mfa\PendingLogin`): + * During login, the browser carries the plugin's `mfa_token` (WorkOS or + * the plugin's MFA policy asked for a code, or the user must enroll first; + * see `Mfa\PendingLogin`). WorkOS's pending token never leaves the server: * - * - POST /auth/mfa/enroll — create the factor to enroll (`mfa_token` only). - * - POST /auth/mfa/challenge — start a challenge on the chosen factor. + * - POST /auth/mfa/enroll — create the factor to enroll. + * - POST /auth/mfa/challenge — start a challenge on one of the sign-in's factors. * - POST /auth/mfa/verify — verify the challenge and complete login. * * From account settings (user is already logged in): @@ -124,13 +125,24 @@ public function register_routes(): void { * @return true|WP_Error */ public function authenticated_permission() { - if ( get_current_user_id() <= 0 ) { + $user_id = get_current_user_id(); + if ( $user_id <= 0 ) { return new WP_Error( 'workos_authkit_not_logged_in', __( 'You must be signed in to manage MFA factors.', 'integration-workos' ), [ 'status' => 401 ] ); } + + /** This filter is documented in src/WorkOS/Mfa/RestApi.php */ + if ( ! apply_filters( 'workos_mfa_can_manage', true, $user_id ) ) { + return new WP_Error( + 'workos_mfa_forbidden', + __( 'You can’t manage two-step verification for this account.', 'integration-workos' ), + [ 'status' => 403 ] + ); + } + return true; } @@ -178,29 +190,27 @@ public function challenge( WP_REST_Request $request ) { return $rate_ok; } - // A parked sign-in may only challenge its own factors, and the - // challenge is remembered so /verify can refuse any other. + // Only a parked sign-in may challenge, and only its own factors. + // The challenge is remembered so /verify can refuse any other. $mfa_token = (string) $request->get_param( 'mfa_token' ); - $pending = null; - if ( '' !== $mfa_token ) { - $pending = PendingLogin::get( $mfa_token, $profile->get_slug() ); - if ( null === $pending ) { - return $this->expired(); - } - if ( ! in_array( $factor_id, (array) $pending['factor_ids'], true ) ) { - return $this->factor_not_found(); - } + $pending = PendingLogin::get( $mfa_token, $profile->get_slug() ); + if ( null === $pending ) { + return $this->expired(); + } + if ( ! in_array( $factor_id, (array) $pending['factor_ids'], true ) ) { + return $this->factor_not_found(); } $result = workos()->api()->challenge_auth_factor( $factor_id ); if ( is_wp_error( $result ) ) { - return $result; + return FactorService::without_body( $result ); } - if ( null !== $pending ) { - $pending['challenges'][] = (string) ( $result['id'] ?? '' ); - PendingLogin::update( $mfa_token, $pending ); - } + $challenge_id = (string) ( $result['id'] ?? '' ); + + $pending['challenges'][] = $challenge_id; + $pending['challenge_factors'][ $challenge_id ] = $factor_id; + PendingLogin::update( $mfa_token, $pending ); return new WP_REST_Response( [ @@ -231,56 +241,7 @@ public function verify( WP_REST_Request $request ) { return $nonce_ok; } - $mfa_token = (string) $request->get_param( 'mfa_token' ); - if ( '' !== $mfa_token ) { - return $this->verify_pending( $request, $profile, $mfa_token ); - } - - $pending_auth_token = (string) $request->get_param( 'pending_authentication_token' ); - $challenge_id = (string) $request->get_param( 'authentication_challenge_id' ); - $code = (string) $request->get_param( 'code' ); - - if ( '' === $pending_auth_token || '' === $challenge_id || '' === $code ) { - return new WP_Error( - 'workos_authkit_invalid_input', - __( 'A pending authentication token, challenge id, and code are required.', 'integration-workos' ), - [ 'status' => 400 ] - ); - } - - $rate_ok = $this->rate_limit( - [ - [ 'mfa_verify_ip', $this->client_ip->get(), TieredRateLimiter::REQUEST_IP ], - ] - ); - if ( is_wp_error( $rate_ok ) ) { - return $rate_ok; - } - - $workos_response = workos()->api()->authenticate_with_totp( - $pending_auth_token, - $challenge_id, - $code, - $this->get_radar_token( $request ) - ); - - // `complete()` handles the `organization_selection_required` error - // transparently when the profile has a pinned org. Pass the WP_Error - // through so it can decide. A wrong code comes back as an error, so - // any success reaching the profile's `always` check has passed MFA. - $result = $this->login_completer->complete( - $workos_response, - $profile, - $this->sanitize_redirect( (string) $request->get_param( 'redirect_to' ) ), - true, - true - ); - - if ( is_wp_error( $result ) ) { - return $result; - } - - return new WP_REST_Response( $result, 200 ); + return $this->verify_pending( $request, $profile, (string) $request->get_param( 'mfa_token' ) ); } /** @@ -345,11 +306,23 @@ public function enroll_pending( WP_REST_Request $request ) { } $result = $service->enroll_totp( (string) $pending['workos_user_id'], (string) ( $pending['email'] ?? '' ) ); } else { + // The policy said enroll, but a factor may have appeared since + // (another tab, the WorkOS dashboard). Enrolling now would add a + // second authenticator without a code from the first. + $existing = $service->list_factors( (int) $pending['user_id'] ); + if ( is_wp_error( $existing ) ) { + return FactorService::without_body( $existing ); + } + if ( ! empty( $existing ) ) { + PendingLogin::forget( $mfa_token ); + return $this->expired(); + } + $result = $service->start_enrollment( (int) $pending['user_id'] ); } if ( is_wp_error( $result ) ) { - return $result; + return FactorService::without_body( $result ); } $pending['factor_ids'] = [ $result['factor_id'] ]; @@ -410,6 +383,16 @@ private function verify_pending( WP_REST_Request $request, Profile $profile, str $factor_id = (string) ( ( (array) $pending['factor_ids'] )[0] ?? '' ); if ( PendingLogin::KIND_WORKOS === $pending['kind'] ) { + $guard = App::container()->get( CodeGuard::class ); + $subject = '' !== (string) ( $pending['workos_user_id'] ?? '' ) + ? CodeGuard::for_workos_user( (string) $pending['workos_user_id'] ) + : 'pending:' . hash( 'sha256', (string) $pending['pending_authentication_token'] ); + + $begun = $guard->begin( $subject ); + if ( is_wp_error( $begun ) ) { + return $begun; + } + $workos_response = workos()->api()->authenticate_with_totp( (string) $pending['pending_authentication_token'], $challenge_id, @@ -418,9 +401,11 @@ private function verify_pending( WP_REST_Request $request, Profile $profile, str ); if ( $this->is_wrong_code( $workos_response ) ) { + $guard->failed( $subject ); return $this->wrong_code( $mfa_token, $pending, 0 ); } + $guard->passed( $subject ); PendingLogin::forget( $mfa_token ); $result = $this->login_completer->complete( @@ -435,7 +420,7 @@ private function verify_pending( WP_REST_Request $request, Profile $profile, str return $result; } - if ( ! empty( $result['user']['id'] ) && '' !== $factor_id ) { + if ( ! empty( $pending['enroll'] ) && ! empty( $result['user']['id'] ) && '' !== $factor_id ) { $service->note_enrolled( (int) $result['user']['id'], $factor_id ); } @@ -445,7 +430,7 @@ private function verify_pending( WP_REST_Request $request, Profile $profile, str $user_id = (int) $pending['user_id']; $checked = ! empty( $pending['enroll'] ) ? $service->confirm_enrollment( $user_id, $factor_id, $code, $challenge_id ) - : $service->verify_code( $factor_id, $code, $challenge_id ); + : $service->check_code( $user_id, $this->challenged_factor( $pending, $challenge_id, $factor_id ), $code, $challenge_id ); if ( is_wp_error( $checked ) ) { return 'workos_mfa_invalid_code' === $checked->get_error_code() @@ -494,6 +479,21 @@ private function verify_pending( WP_REST_Request $request, Profile $profile, str ); } + /** + * The factor a remembered challenge was made for. + * + * @param array $pending Parked sign-in. + * @param string $challenge_id Challenge ID. + * @param string $fallback Factor to use for records without the map. + * + * @return string + */ + private function challenged_factor( array $pending, string $challenge_id, string $fallback ): string { + $map = (array) ( $pending['challenge_factors'] ?? [] ); + + return (string) ( $map[ $challenge_id ] ?? $fallback ); + } + /** * Whether WorkOS refused the TOTP grant because of the code. * @@ -596,7 +596,7 @@ private function factor_not_found(): WP_Error { public function list_factors( WP_REST_Request $request ) { $factors = App::container()->get( FactorService::class )->list_factors( get_current_user_id() ); if ( is_wp_error( $factors ) ) { - return $factors; + return FactorService::without_body( $factors ); } return new WP_REST_Response( [ 'factors' => $factors ], 200 ); @@ -634,11 +634,18 @@ public function enroll_totp( WP_REST_Request $request ) { return $rate_ok; } - // Same path as the account panel: the factor stays unconfirmed until - // `POST /users/{id}/mfa/confirm` checks its first code. - $result = App::container()->get( FactorService::class )->start_enrollment( $user_id ); + // Same path as the account panel: a current code first when the + // account already has a factor, and the new one stays unconfirmed + // until `POST /users/{id}/mfa/confirm` checks its first code. + $service = App::container()->get( FactorService::class ); + $holder = $service->require_current_factor( $user_id, (string) $request->get_param( 'factor_id' ), (string) $request->get_param( 'code' ) ); + if ( is_wp_error( $holder ) ) { + return FactorService::without_body( $holder ); + } + + $result = $service->start_enrollment( $user_id ); if ( is_wp_error( $result ) ) { - return $result; + return FactorService::without_body( $result ); } return new WP_REST_Response( $result, 201 ); @@ -679,7 +686,8 @@ public function enroll_sms( WP_REST_Request $request ) { ); } - $workos_user_id = (string) get_user_meta( get_current_user_id(), '_workos_user_id', true ); + $user_id = get_current_user_id(); + $workos_user_id = User::get_workos_id( $user_id ); if ( '' === $workos_user_id ) { return new WP_Error( 'workos_authkit_no_workos_user', @@ -688,11 +696,26 @@ public function enroll_sms( WP_REST_Request $request ) { ); } + $rate_ok = $this->rate_limit( [ [ 'mfa_enroll_user', (string) $user_id, TieredRateLimiter::REQUEST_SUBJECT ] ] ); + if ( is_wp_error( $rate_ok ) ) { + return $rate_ok; + } + + // A borrowed session mustn't add the attacker's phone as a second + // factor: with a factor already on the account, a current code first. + $service = App::container()->get( FactorService::class ); + $holder = $service->require_current_factor( $user_id, (string) $request->get_param( 'factor_id' ), (string) $request->get_param( 'code' ) ); + if ( is_wp_error( $holder ) ) { + return FactorService::without_body( $holder ); + } + $result = workos()->api()->enroll_sms_factor( $workos_user_id, $phone_number ); if ( is_wp_error( $result ) ) { - return $result; + return FactorService::without_body( $result ); } + $service->note_enrolled( $user_id, (string) ( $result['id'] ?? '' ), 'sms' ); + return new WP_REST_Response( [ 'factor_id' => (string) ( $result['id'] ?? '' ), @@ -746,7 +769,7 @@ public function delete_factor( WP_REST_Request $request ) { $service = App::container()->get( FactorService::class ); $factors = $service->list_factors( $user_id ); if ( is_wp_error( $factors ) ) { - return $factors; + return FactorService::without_body( $factors ); } // Return 404 rather than 403 so we don't leak the existence of @@ -757,14 +780,14 @@ public function delete_factor( WP_REST_Request $request ) { // A current code from the factor, so a borrowed session can't turn // MFA off. - $valid = $service->verify_code( $factor_id, (string) $request->get_param( 'code' ) ); + $valid = $service->check_code( $user_id, $factor_id, (string) $request->get_param( 'code' ) ); if ( is_wp_error( $valid ) ) { - return $valid; + return FactorService::without_body( $valid ); } $result = $service->delete_factor( $user_id, $factor_id ); if ( is_wp_error( $result ) ) { - return $result; + return FactorService::without_body( $result ); } return new WP_REST_Response( [ 'deleted' => true ], 200 ); diff --git a/src/WorkOS/REST/TokenAuth.php b/src/WorkOS/REST/TokenAuth.php index b5ce011..cc282f3 100644 --- a/src/WorkOS/REST/TokenAuth.php +++ b/src/WorkOS/REST/TokenAuth.php @@ -78,6 +78,27 @@ public function authenticate( $user_id ) { $wp_user_id = \WorkOS\Sync\UserSync::get_wp_user_id_by_workos_id( $workos_user_id ); + /** + * Filters the WP user a verified Bearer token acts as. + * + * Return 0 to refuse the token. The MFA module uses it so tokens + * from sessions that never passed this site's MFA step can't act + * for users who have or need MFA. + * + * @param int $wp_user_id WP user ID, or 0 when the token maps to none. + * @param array $payload Verified JWT claims. + */ + $mapped = (int) $wp_user_id; + $wp_user_id = (int) apply_filters( 'workos_bearer_token_user', $mapped, $payload ); + + if ( $mapped > 0 && 0 === $wp_user_id ) { + $this->auth_error = new WP_Error( + 'workos_token_refused', + __( 'This token can’t be used here. Sign in on this site to get one.', 'integration-workos' ), + [ 'status' => 401 ] + ); + } + return $wp_user_id ? $wp_user_id : $user_id; } diff --git a/src/WorkOS/Sync/UserSync.php b/src/WorkOS/Sync/UserSync.php index 32d6969..9babf89 100644 --- a/src/WorkOS/Sync/UserSync.php +++ b/src/WorkOS/Sync/UserSync.php @@ -177,7 +177,17 @@ public static function link_user( int $wp_user_id, array $workos_user, string $s $previous_id = (string) get_user_meta( $wp_user_id, '_workos_user_id', true ); $is_relink = '' !== $previous_id && $previous_id !== $new_id; - $protected = Config::is_relink_protection_enabled(); + /** + * Filters whether Re-link Protection applies to a WP user. + * + * The MFA module turns it on for users who have or need MFA, so a + * different WorkOS identity with the same email can't take over the + * account and skip its second factor. + * + * @param bool $protected Default: the Re-link Protection setting. + * @param int $wp_user_id WP user ID. + */ + $protected = (bool) apply_filters( 'workos_relink_protected', Config::is_relink_protection_enabled(), $wp_user_id ); if ( $is_relink && ! $force && $protected ) { self::refuse_link( $wp_user_id, $previous_id, $workos_user, $source ); diff --git a/src/js/authkit/index.tsx b/src/js/authkit/index.tsx index 9205a28..80a167f 100644 --- a/src/js/authkit/index.tsx +++ b/src/js/authkit/index.tsx @@ -59,6 +59,13 @@ function parseConfig( root: HTMLElement ): AppProps { ? JSON.parse( mfaJson ) : undefined; + // The resume code in the URL is spent; keep it out of history. + const url = new URL( window.location.href ); + if ( url.searchParams.has( 'workos_mfa' ) ) { + url.searchParams.delete( 'workos_mfa' ); + window.history.replaceState( window.history.state, '', url.toString() ); + } + return { profile, invitationToken: root.getAttribute( 'data-invitation-token' ) || '', diff --git a/src/js/authkit/types.ts b/src/js/authkit/types.ts index a761589..c8ab463 100644 --- a/src/js/authkit/types.ts +++ b/src/js/authkit/types.ts @@ -88,9 +88,9 @@ export interface AuthFactor { } /** - * The MFA step. WorkOS's own challenge carries - * `pending_authentication_token`; a step the plugin's MFA policy asked for, - * or any enrollment, carries `mfa_token` instead. + * The MFA step. Carries the plugin's `mfa_token`, whether WorkOS or the + * plugin's MFA policy asked for the code. `pending_authentication_token` + * is only sent by servers older than 1.1.0. */ export interface MfaRequired { mfa_required: true; diff --git a/src/js/mfa-settings/index.tsx b/src/js/mfa-settings/index.tsx index cb989c3..6ebf26c 100644 --- a/src/js/mfa-settings/index.tsx +++ b/src/js/mfa-settings/index.tsx @@ -27,10 +27,16 @@ interface Factor { interface Status { enabled: boolean; + /** Admin flag on this user. */ required: boolean; + /** Must use MFA for any reason: the flag, a required role, or a filter. */ + must_use?: boolean; factors: Factor[]; } +const mustUse = ( status: Status ): boolean => + status.must_use ?? status.required; + interface Enrollment { factor_id: string; challenge_id: string; @@ -227,7 +233,7 @@ function Panel( { userId }: { userId: number } ) { 'integration-workos' ) }

- { status.required && ( + { mustUse( status ) && (

{ __( 'Your administrator requires two-step verification for this account.', @@ -271,7 +277,7 @@ function Panel( { userId }: { userId: number } ) { { removing && (

- { status.required && status.factors.length === 1 + { mustUse( status ) && status.factors.length === 1 ? __( 'This is your only authenticator app. Because two-step verification is required, you’ll set up a new one the next time you sign in.', 'integration-workos' diff --git a/tests/wpunit/AuthKitLoginCompleterMfaTest.php b/tests/wpunit/AuthKitLoginCompleterMfaTest.php index b77b406..af2b00d 100644 --- a/tests/wpunit/AuthKitLoginCompleterMfaTest.php +++ b/tests/wpunit/AuthKitLoginCompleterMfaTest.php @@ -88,6 +88,12 @@ public function test_enforce_if_required_allows_single_step_success(): void { ] ); + // The policy asks WorkOS for the user's factors: none. + $no_factors = static fn( $pre, $args, $url ) => str_contains( $url, '/auth_factors' ) + ? [ 'response' => [ 'code' => 200 ], 'body' => wp_json_encode( [ 'data' => [] ] ), 'headers' => [] ] + : $pre; + add_filter( 'pre_http_request', $no_factors, 10, 3 ); + $result = $this->completer->complete( [ 'access_token' => 'eyJ.eyJzdWIiOiJ1c2VyXzIifQ.sig', @@ -101,6 +107,8 @@ public function test_enforce_if_required_allows_single_step_success(): void { $this->assertIsArray( $result ); $this->assertArrayHasKey( 'user', $result ); $this->assertSame( 'ok@example.com', $result['user']['email'] ); + + remove_filter( 'pre_http_request', $no_factors, 10 ); } /** @@ -156,6 +164,8 @@ public function test_pending_factor_in_allowlist_surfaces_mfa_step(): void { $this->assertIsArray( $result ); $this->assertTrue( $result['mfa_required'] ); - $this->assertSame( 'pat_2', $result['pending_authentication_token'] ); + // WorkOS's pending token stays on the server; the browser gets an mfa_token. + $this->assertArrayNotHasKey( 'pending_authentication_token', $result ); + $this->assertNotEmpty( $result['mfa_token'] ); } } diff --git a/tests/wpunit/AuthKitRestEmailVerificationTest.php b/tests/wpunit/AuthKitRestEmailVerificationTest.php index 13094c7..95166df 100644 --- a/tests/wpunit/AuthKitRestEmailVerificationTest.php +++ b/tests/wpunit/AuthKitRestEmailVerificationTest.php @@ -272,7 +272,9 @@ public function test_verify_surfaces_mfa(): void { $this->assertSame( 200, $response->get_status() ); $this->assertTrue( $response->get_data()['mfa_required'] ); - $this->assertSame( 'pat_mfa', $response->get_data()['pending_authentication_token'] ); + // WorkOS's pending token stays on the server; the browser gets an mfa_token. + $this->assertArrayNotHasKey( 'pending_authentication_token', $response->get_data() ); + $this->assertNotEmpty( $response->get_data()['mfa_token'] ); } /** diff --git a/tests/wpunit/AuthKitRestMfaTest.php b/tests/wpunit/AuthKitRestMfaTest.php index 021efe4..c1f9073 100644 --- a/tests/wpunit/AuthKitRestMfaTest.php +++ b/tests/wpunit/AuthKitRestMfaTest.php @@ -11,6 +11,7 @@ use WorkOS\Auth\AuthKit\LoginCompleter; use WorkOS\Auth\AuthKit\Nonce; use WorkOS\Auth\AuthKit\Profile; +use WorkOS\Mfa\PendingLogin; use WP_REST_Request; use WP_REST_Response; use WorkOS\Auth\AuthKit\ProfileRepository; @@ -161,6 +162,23 @@ private function dispatch_with_nonce( string $method, string $route, array $body return rest_get_server()->dispatch( $request ); } + /** + * Park a sign-in WorkOS asked a code for, as LoginCompleter does. + */ + private function park_workos( string $profile_slug, string $pending_token, array $challenges = [] ): string { + return PendingLogin::park( + [ + 'kind' => PendingLogin::KIND_WORKOS, + 'profile' => $profile_slug, + 'pending_authentication_token' => $pending_token, + 'workos_user_id' => 'user_' . $pending_token, + 'factor_ids' => [ 'factor_123' ], + 'factors' => [ [ 'id' => 'factor_123', 'type' => 'totp' ] ], + 'challenges' => $challenges, + ] + ); + } + // --------------------------- /auth/mfa/challenge --------------------------- public function test_challenge_requires_factor_id(): void { @@ -183,7 +201,11 @@ public function test_challenge_returns_challenge_id_on_success(): void { $response = $this->dispatch_with_nonce( 'POST', '/workos/v1/auth/mfa/challenge', - [ 'profile' => 'mfa-on', 'factor_id' => 'factor_123' ], + [ + 'profile' => 'mfa-on', + 'factor_id' => 'factor_123', + 'mfa_token' => $this->park_workos( 'mfa-on', 'pat_1' ), + ], 'mfa-on' ); @@ -191,19 +213,63 @@ public function test_challenge_returns_challenge_id_on_success(): void { $this->assertSame( 'challenge_abc', $response->get_data()['challenge_id'] ); } + /** + * Without a parked sign-in, nobody may challenge a factor: the route + * would otherwise send codes to (and rate-limit) any factor ID. + */ + public function test_challenge_requires_a_parked_sign_in(): void { + $response = $this->dispatch_with_nonce( + 'POST', + '/workos/v1/auth/mfa/challenge', + [ 'profile' => 'mfa-on', 'factor_id' => 'factor_123' ], + 'mfa-on' + ); + + $this->assertSame( 403, $response->get_status() ); + $this->assertSame( 'workos_authkit_mfa_expired', $response->get_data()['code'] ); + $this->assertEmpty( $this->captured ); + } + // --------------------------- /auth/mfa/verify --------------------------- public function test_verify_requires_all_inputs(): void { $response = $this->dispatch_with_nonce( 'POST', '/workos/v1/auth/mfa/verify', - [ 'profile' => 'mfa-on', 'code' => '111111' ], + [ + 'profile' => 'mfa-on', + 'code' => '111111', + 'mfa_token' => $this->park_workos( 'mfa-on', 'pat_1', [ 'chal_1' ] ), + ], 'mfa-on' ); $this->assertSame( 400, $response->get_status() ); } + /** + * WorkOS's pending token alone no longer finishes a sign-in: the + * browser never sees it, and a client-chosen challenge can't be paired + * with someone else's. + */ + public function test_verify_refuses_a_bare_pending_token(): void { + $response = $this->dispatch_with_nonce( + 'POST', + '/workos/v1/auth/mfa/verify', + [ + 'profile' => 'mfa-on', + 'pending_authentication_token' => 'pat_1', + 'authentication_challenge_id' => 'chal_1', + 'code' => '123456', + ], + 'mfa-on' + ); + + $this->assertSame( 403, $response->get_status() ); + $this->assertSame( 'workos_authkit_mfa_expired', $response->get_data()['code'] ); + $this->assertEmpty( $this->captured ); + } + public function test_verify_completes_login_on_success(): void { $this->queue_response( '/user_management/authenticate', @@ -218,10 +284,10 @@ public function test_verify_completes_login_on_success(): void { 'POST', '/workos/v1/auth/mfa/verify', [ - 'profile' => 'mfa-on', - 'pending_authentication_token' => 'pat_1', - 'authentication_challenge_id' => 'chal_1', - 'code' => '123456', + 'profile' => 'mfa-on', + 'mfa_token' => $this->park_workos( 'mfa-on', 'pat_1', [ 'chal_1' ] ), + 'authentication_challenge_id' => 'chal_1', + 'code' => '123456', ], 'mfa-on' ); @@ -261,10 +327,10 @@ public function test_verify_completes_login_on_enforce_always_profile(): void { 'POST', '/workos/v1/auth/mfa/verify', [ - 'profile' => 'mfa-always', - 'pending_authentication_token' => 'pat_always', - 'authentication_challenge_id' => 'chal_always', - 'code' => '123456', + 'profile' => 'mfa-always', + 'mfa_token' => $this->park_workos( 'mfa-always', 'pat_always', [ 'chal_always' ] ), + 'authentication_challenge_id' => 'chal_always', + 'code' => '123456', ], 'mfa-always' ); @@ -293,10 +359,10 @@ public function test_verify_wrong_code_on_enforce_always_profile_fails(): void { 'POST', '/workos/v1/auth/mfa/verify', [ - 'profile' => 'mfa-always', - 'pending_authentication_token' => 'pat_always', - 'authentication_challenge_id' => 'chal_always', - 'code' => '000000', + 'profile' => 'mfa-always', + 'mfa_token' => $this->park_workos( 'mfa-always', 'pat_always', [ 'chal_always' ] ), + 'authentication_challenge_id' => 'chal_always', + 'code' => '000000', ], 'mfa-always' ); @@ -467,7 +533,7 @@ public function test_delete_factor_removes_owned_factor(): void { ); // A current code from the factor proves it's the owner. $this->queue_response( '/auth/factors/factor_x/challenge', [ 'id' => 'challenge_del' ] ); - $this->queue_response( '/auth/challenges/challenge_del/verify', [ 'valid' => true ] ); + $this->queue_response( '/auth/challenges/challenge_del/verify', [ 'challenge' => [ 'id' => 'challenge_del', 'authentication_factor_id' => 'factor_x' ], 'valid' => true ] ); $this->queue_response( '/auth/factors/factor_x', [] ); $response = $this->dispatch_with_nonce( diff --git a/tests/wpunit/AuthKitRestPasswordTest.php b/tests/wpunit/AuthKitRestPasswordTest.php index ff02cd2..17c8712 100644 --- a/tests/wpunit/AuthKitRestPasswordTest.php +++ b/tests/wpunit/AuthKitRestPasswordTest.php @@ -353,7 +353,9 @@ public function test_authenticate_mfa_required_is_surfaced(): void { $this->assertSame( 200, $response->get_status() ); $data = $response->get_data(); $this->assertTrue( $data['mfa_required'] ); - $this->assertSame( 'pat_123', $data['pending_authentication_token'] ); + // WorkOS's pending token stays on the server; the browser gets an mfa_token. + $this->assertArrayNotHasKey( 'pending_authentication_token', $data ); + $this->assertNotEmpty( $data['mfa_token'] ); $this->assertCount( 1, $data['factors'] ); } @@ -428,7 +430,9 @@ public function test_authenticate_mfa_challenge_error_is_surfaced(): void { $this->assertSame( 200, $response->get_status() ); $data = $response->get_data(); $this->assertTrue( $data['mfa_required'] ); - $this->assertSame( 'pat_mfa', $data['pending_authentication_token'] ); + // WorkOS's pending token stays on the server; the browser gets an mfa_token. + $this->assertArrayNotHasKey( 'pending_authentication_token', $data ); + $this->assertNotEmpty( $data['mfa_token'] ); $this->assertSame( 'auth_factor_1', $data['factors'][0]['id'] ); } @@ -1318,7 +1322,9 @@ public function test_reset_confirm_surfaces_mfa_challenge(): void { $this->assertSame( 200, $response->get_status() ); $data = $response->get_data(); $this->assertTrue( $data['mfa_required'] ?? false ); - $this->assertSame( 'pending_xyz', $data['pending_authentication_token'] ?? '' ); + // WorkOS's pending token stays on the server; the browser gets an mfa_token. + $this->assertArrayNotHasKey( 'pending_authentication_token', $data ); + $this->assertNotEmpty( $data['mfa_token'] ); $this->assertFalse( $data['signed_in'] ?? null, 'signed_in must NOT be true on the MFA path.' ); // User is not yet signed in — MFA verify will complete the session. $this->assertSame( 0, get_current_user_id() ); diff --git a/tests/wpunit/MfaAccountRestTest.php b/tests/wpunit/MfaAccountRestTest.php index 244c037..6d6a1e1 100644 --- a/tests/wpunit/MfaAccountRestTest.php +++ b/tests/wpunit/MfaAccountRestTest.php @@ -194,7 +194,7 @@ public function test_enroll_then_confirm_turns_mfa_on(): void { $this->assertSame( 201, $enroll->get_status() ); $this->assertSame( 'SECRET', $enroll->get_data()['secret'] ); - $this->queue( 'POST /auth/challenges/auth_challenge_new/verify', [ 'valid' => true ] ); + $this->queue( 'POST /auth/challenges/auth_challenge_new/verify', [ 'challenge' => [ 'id' => 'auth_challenge_new', 'authentication_factor_id' => 'auth_factor_new' ], 'valid' => true ] ); $this->factors( [ [ 'id' => 'auth_factor_new', 'type' => 'totp' ] ] ); $confirm = $this->request( @@ -229,7 +229,7 @@ public function test_remove_with_code_turns_mfa_off(): void { update_user_meta( $this->user_id, User::META_MFA_ENABLED, '1' ); $this->factors( [ [ 'id' => 'auth_factor_1', 'type' => 'totp' ] ] ); $this->queue( 'POST /auth/factors/auth_factor_1/challenge', [ 'id' => 'auth_challenge_rm' ] ); - $this->queue( 'POST /auth/challenges/auth_challenge_rm/verify', [ 'valid' => true ] ); + $this->queue( 'POST /auth/challenges/auth_challenge_rm/verify', [ 'challenge' => [ 'id' => 'auth_challenge_rm', 'authentication_factor_id' => 'auth_factor_1' ], 'valid' => true ] ); $response = $this->request( 'DELETE', '/factors/auth_factor_1', [ 'code' => '123456' ] ); diff --git a/tests/wpunit/MfaAdminTest.php b/tests/wpunit/MfaAdminTest.php index 917f50f..85f5f9a 100644 --- a/tests/wpunit/MfaAdminTest.php +++ b/tests/wpunit/MfaAdminTest.php @@ -156,7 +156,12 @@ public function test_panel_save_sets_flag_and_resets(): void { $_POST['workos_mfa_required'] = '1'; $_POST['workos_mfa_reset'] = '1'; - ( new UserEditPanel( new FactorService(), new Policy( new Settings() ) ) )->save( $this->user_id ); + $panel = new UserEditPanel( new FactorService(), new Policy( new Settings() ) ); + $panel->save( $this->user_id ); + + $this->assertSame( '', (string) get_user_meta( $this->user_id, User::META_MFA_REQUIRED, true ), 'Nothing changes until core saved the profile.' ); + + $panel->apply( $this->user_id ); $this->assertSame( '1', get_user_meta( $this->user_id, User::META_MFA_REQUIRED, true ) ); $this->assertSame( '', (string) get_user_meta( $this->user_id, User::META_MFA_ENABLED, true ) ); diff --git a/tests/wpunit/MfaFactorServiceTest.php b/tests/wpunit/MfaFactorServiceTest.php index 49b3c5b..a76e068 100644 --- a/tests/wpunit/MfaFactorServiceTest.php +++ b/tests/wpunit/MfaFactorServiceTest.php @@ -190,7 +190,7 @@ public function test_unlinked_user_gets_409(): void { public function test_confirm_with_valid_code_enables_mfa_and_fires_hook(): void { $this->set_pending( 'auth_factor_new' ); - $this->queue( 'POST /auth/challenges/auth_challenge_new/verify', [ 'challenge' => [ 'id' => 'auth_challenge_new' ], 'valid' => true ] ); + $this->queue( 'POST /auth/challenges/auth_challenge_new/verify', [ 'challenge' => [ 'id' => 'auth_challenge_new', 'authentication_factor_id' => 'auth_factor_new' ], 'valid' => true ] ); $this->queue( 'GET /user_management/users/user_mfa/auth_factors', [ 'data' => [ [ 'id' => 'auth_factor_new', 'type' => 'totp' ] ] ] ); $result = $this->service->confirm_enrollment( $this->user_id, 'auth_factor_new', '123 456', 'auth_challenge_new' ); @@ -207,7 +207,7 @@ public function test_confirm_with_valid_code_enables_mfa_and_fires_hook(): void public function test_confirm_creates_challenge_when_none_given(): void { $this->set_pending( 'auth_factor_new' ); $this->queue( 'POST /auth/factors/auth_factor_new/challenge', [ 'id' => 'auth_challenge_fresh' ] ); - $this->queue( 'POST /auth/challenges/auth_challenge_fresh/verify', [ 'valid' => true ] ); + $this->queue( 'POST /auth/challenges/auth_challenge_fresh/verify', [ 'challenge' => [ 'id' => 'auth_challenge_fresh', 'authentication_factor_id' => 'auth_factor_new' ], 'valid' => true ] ); $this->queue( 'GET /user_management/users/user_mfa/auth_factors', [ 'data' => [ [ 'id' => 'auth_factor_new', 'type' => 'totp' ] ] ] ); $this->service->confirm_enrollment( $this->user_id, 'auth_factor_new', '123456' ); @@ -263,7 +263,7 @@ public function test_confirm_rejects_factor_that_is_not_pending(): void { // --------------------------------- Listing --------------------------------- - public function test_list_hides_pending_factor_and_reconciles_enabled_flag(): void { + public function test_list_hides_pending_factor_and_keeps_enabled_flag(): void { $this->set_pending( 'auth_factor_pending' ); update_user_meta( $this->user_id, User::META_MFA_ENABLED, '1' ); $this->queue( @@ -274,7 +274,13 @@ public function test_list_hides_pending_factor_and_reconciles_enabled_flag(): vo $factors = $this->service->list_factors( $this->user_id ); $this->assertSame( [], $factors ); - $this->assertFalse( $this->service->is_enabled( $this->user_id ), 'Only a pending factor in WorkOS: MFA is off.' ); + $this->assertTrue( $this->service->is_enabled( $this->user_id ), 'Factors gone from WorkOS don\'t turn MFA off; the next sign-in enrolls.' ); + + $this->queue( + 'GET /user_management/users/user_mfa/auth_factors', + [ 'data' => [ [ 'id' => 'auth_factor_pending', 'type' => 'totp' ] ] ] + ); + $this->assertFalse( $this->service->status( $this->user_id )['enabled'], 'The status reports what WorkOS has.' ); } public function test_list_turns_flag_on_for_factor_added_outside_wordpress(): void { diff --git a/tests/wpunit/MfaLoginErrorsTest.php b/tests/wpunit/MfaLoginErrorsTest.php index be4c9b6..86f5854 100644 --- a/tests/wpunit/MfaLoginErrorsTest.php +++ b/tests/wpunit/MfaLoginErrorsTest.php @@ -100,7 +100,9 @@ public function test_completer_turns_challenge_error_into_mfa_step(): void { $this->assertIsArray( $result ); $this->assertTrue( $result['mfa_required'] ); - $this->assertSame( 'pat_mfa', $result['pending_authentication_token'] ); + // WorkOS's pending token stays on the server; the browser gets an mfa_token. + $this->assertArrayNotHasKey( 'pending_authentication_token', $result ); + $this->assertNotEmpty( $result['mfa_token'] ); $this->assertSame( [ [ 'id' => 'auth_factor_1', 'type' => 'totp' ] ], $result['factors'] ); $this->assertSame( 0, get_current_user_id() ); } diff --git a/tests/wpunit/MfaPluginEnforcementTest.php b/tests/wpunit/MfaPluginEnforcementTest.php index 74e8dee..f59c8f6 100644 --- a/tests/wpunit/MfaPluginEnforcementTest.php +++ b/tests/wpunit/MfaPluginEnforcementTest.php @@ -184,7 +184,7 @@ public function test_challenge_and_verify_finish_the_sign_in(): void { $challenge = $this->dispatch( '/workos/v1/auth/mfa/challenge', [ 'mfa_token' => $token, 'factor_id' => 'auth_factor_1' ] ); $this->assertSame( 'auth_challenge_1', $challenge->get_data()['challenge_id'] ); - $this->queue( 'POST /auth/challenges/auth_challenge_1/verify', [ 'valid' => true ] ); + $this->queue( 'POST /auth/challenges/auth_challenge_1/verify', [ 'challenge' => [ 'id' => 'auth_challenge_1', 'authentication_factor_id' => 'auth_factor_1' ], 'valid' => true ] ); $done = $this->dispatch( '/workos/v1/auth/mfa/verify', [ @@ -276,7 +276,7 @@ public function test_required_user_enrolls_during_sign_in(): void { $this->assertSame( 201, $enroll->get_status() ); $this->assertSame( 'SECRET', $enroll->get_data()['secret'] ); - $this->queue( 'POST /auth/challenges/auth_challenge_enroll/verify', [ 'valid' => true ] ); + $this->queue( 'POST /auth/challenges/auth_challenge_enroll/verify', [ 'challenge' => [ 'id' => 'auth_challenge_enroll', 'authentication_factor_id' => 'auth_factor_new' ], 'valid' => true ] ); $this->queue( 'GET /user_management/users/user_mfa/auth_factors', [ 'data' => [ [ 'id' => 'auth_factor_new', 'type' => 'totp' ] ] ] ); $done = $this->dispatch( '/workos/v1/auth/mfa/verify', diff --git a/tests/wpunit/MfaSecurityTest.php b/tests/wpunit/MfaSecurityTest.php new file mode 100644 index 0000000..33dbf2d --- /dev/null +++ b/tests/wpunit/MfaSecurityTest.php @@ -0,0 +1,672 @@ + + */ + private array $responses = []; + + /** + * Mail captured through `pre_wp_mail`. + * + * @var array + */ + private array $mail = []; + + public function setUp(): void { + parent::setUp(); + + \WorkOS\Config::set_active_environment( 'production' ); + update_option( + 'workos_production', + [ + 'api_key' => 'sk_test_fake', + 'client_id' => 'client_fake', + 'environment_id' => 'environment_test', + 'allow_password_fallback' => true, + ] + ); + App::container()->get( \WorkOS\Options\Production::class )->reset(); + + $this->repository = new ProfileRepository(); + $this->repository->register_post_type(); + foreach ( $this->repository->all() as $profile ) { + wp_delete_post( $profile->get_id(), true ); + } + $this->save_profile( 'members', Profile::MFA_ENFORCE_IF_REQUIRED ); + + $this->nonce = new Nonce(); + $limiter = new TieredRateLimiter( new DatabaseRateLimiter() ); + $deps = [ + $this->repository, + $this->nonce, + new Radar(), + $limiter, + new ClientIp(), + new AddressCanonicalizer(), + new LoginCompleter(), + ]; + $password = new Password( ...$deps ); + $mfa = new Mfa( ...$deps ); + $account = new RestApi( new FactorService(), $limiter ); + add_action( 'rest_api_init', [ $password, 'register_routes' ] ); + add_action( 'rest_api_init', [ $mfa, 'register_routes' ] ); + add_action( 'rest_api_init', [ $account, 'register_routes' ] ); + do_action( 'rest_api_init', rest_get_server() ); + + add_filter( 'pre_http_request', [ $this, 'intercept_http' ], 10, 3 ); + add_filter( 'pre_wp_mail', [ $this, 'capture_mail' ], 10, 2 ); + + $this->user_id = (int) wp_create_user( 'mfa_member', 'wp_password_123', 'member@example.com' ); + UserSync::link_user( $this->user_id, [ 'id' => 'user_mfa', 'email' => 'member@example.com' ], 'test' ); + $this->admin_id = self::factory()->user->create( [ 'role' => 'administrator' ] ); + + $this->captured = []; + $this->mail = []; + wp_set_current_user( 0 ); + } + + public function tearDown(): void { + remove_filter( 'pre_http_request', [ $this, 'intercept_http' ], 10 ); + remove_filter( 'pre_wp_mail', [ $this, 'capture_mail' ], 10 ); + remove_all_filters( 'workos_mfa_mode' ); + remove_all_filters( 'workos_mfa_notify_user' ); + unset( $_POST['workos_mfa_required'], $_POST['workos_mfa_reset'], $_COOKIE[ LOGGED_IN_COOKIE ] ); + $this->captured = []; + $this->responses = []; + $this->mail = []; + + foreach ( $this->repository->all() as $profile ) { + wp_delete_post( $profile->get_id(), true ); + } + + wp_set_current_user( 0 ); + delete_option( 'workos_production' ); + \WorkOS\Config::set_active_environment( 'staging' ); + App::container()->get( \WorkOS\Options\Production::class )->reset(); + + parent::tearDown(); + } + + public function intercept_http( $preempt, array $args, string $url ) { + if ( ! str_contains( $url, 'api.workos.com' ) ) { + return $preempt; + } + + $method = $args['method'] ?? 'GET'; + $this->captured[] = [ + 'method' => $method, + 'url' => $url, + 'body' => $args['body'] ?? '', + ]; + + foreach ( $this->responses as $key => $resp ) { + [ $want_method, $needle ] = explode( ' ', $key, 2 ); + if ( $want_method === $method && str_contains( $url, $needle ) ) { + return $resp; + } + } + + return [ + 'response' => [ 'code' => 200, 'message' => 'OK' ], + 'body' => '{}', + ]; + } + + public function capture_mail( $short_circuit, array $atts ) { + $this->mail[] = $atts; + return true; + } + + // ------------------------- Admin routes on yourself ------------------------- + + public function test_subscriber_cannot_reset_or_unrequire_their_own_mfa(): void { + update_user_meta( $this->user_id, User::META_MFA_REQUIRED, '1' ); + update_user_meta( $this->user_id, User::META_MFA_ENABLED, '1' ); + wp_set_current_user( $this->user_id ); + + $this->assertContains( $this->account( 'PUT', $this->user_id, '/required', [ 'required' => false ] )->get_status(), [ 401, 403 ] ); + $this->assertContains( $this->account( 'POST', $this->user_id, '/reset' )->get_status(), [ 401, 403 ] ); + $this->assertSame( '1', get_user_meta( $this->user_id, User::META_MFA_REQUIRED, true ) ); + $this->assertEmpty( array_filter( $this->captured, static fn( $c ) => 'DELETE' === $c['method'] ) ); + + wp_set_current_user( $this->admin_id ); + $this->assertSame( 200, $this->account( 'PUT', $this->user_id, '/required', [ 'required' => false ] )->get_status() ); + } + + // ------------------------- Second authenticator ------------------------- + + public function test_hijacked_session_cannot_add_a_second_authenticator_without_a_code(): void { + wp_set_current_user( $this->user_id ); + $this->factors( [ [ 'id' => 'auth_factor_1', 'type' => 'totp' ] ] ); + $this->queue( 'POST /user_management/users/user_mfa/auth_factors', $this->enroll_response(), 201 ); + + $refused = $this->account( 'POST', $this->user_id, '/enroll' ); + $this->assertSame( 403, $refused->get_status() ); + $this->assertSame( 'workos_mfa_code_required', $refused->get_data()['code'] ); + $this->assertEmpty( array_filter( $this->captured, static fn( $c ) => 'POST' === $c['method'] && str_ends_with( $c['url'], '/auth_factors' ) ) ); + + $this->queue( 'POST /auth/factors/auth_factor_1/challenge', [ 'id' => 'auth_challenge_step' ] ); + $this->queue( 'POST /auth/challenges/auth_challenge_step/verify', $this->valid( 'auth_factor_1' ) ); + + $allowed = $this->account( + 'POST', + $this->user_id, + '/enroll', + [ + 'factor_id' => 'auth_factor_1', + 'code' => '123456', + ] + ); + $this->assertSame( 201, $allowed->get_status() ); + } + + public function test_confirm_with_another_factors_challenge_does_not_turn_mfa_on(): void { + wp_set_current_user( $this->user_id ); + update_user_meta( + $this->user_id, + User::META_MFA_PENDING_FACTOR, + [ + 'factor_id' => 'auth_factor_new', + 'created' => time(), + ] + ); + $this->queue( 'POST /auth/challenges/auth_challenge_other/verify', $this->valid( 'auth_factor_other' ) ); + + $response = $this->account( + 'POST', + $this->user_id, + '/confirm', + [ + 'factor_id' => 'auth_factor_new', + 'code' => '123456', + 'challenge_id' => 'auth_challenge_other', + ] + ); + + $this->assertSame( 'workos_mfa_invalid_code', $response->get_data()['code'] ); + $this->assertSame( '', (string) get_user_meta( $this->user_id, User::META_MFA_ENABLED, true ) ); + } + + // ------------------------- Per-account code limit ------------------------- + + public function test_new_sign_ins_do_not_reset_the_wrong_code_budget(): void { + $this->enroll_user(); + $this->queue( 'POST /auth/factors/auth_factor_1/challenge', [ 'id' => 'auth_challenge_1' ] ); + $this->queue( 'POST /auth/challenges/auth_challenge_1/verify', [ 'valid' => false ] ); + + $codes = []; + foreach ( [ 3, 3 ] as $tries ) { + $token = $this->sign_in()->get_data()['mfa_token']; + $this->dispatch( '/workos/v1/auth/mfa/challenge', [ 'mfa_token' => $token, 'factor_id' => 'auth_factor_1' ] ); + for ( $i = 0; $i < $tries; $i++ ) { + $codes[] = $this->dispatch( + '/workos/v1/auth/mfa/verify', + [ + 'mfa_token' => $token, + 'authentication_challenge_id' => 'auth_challenge_1', + 'code' => '000000', + ] + )->get_data()['code']; + } + } + + $this->assertSame( array_fill( 0, 5, 'workos_mfa_invalid_code' ), array_slice( $codes, 0, 5 ) ); + $this->assertSame( 'workos_mfa_too_many_codes', $codes[5] ); + $this->assertSame( 0, get_current_user_id() ); + } + + public function test_right_code_does_not_count_toward_the_limit(): void { + $service = new FactorService(); + $this->queue( 'POST /auth/factors/auth_factor_1/challenge', [ 'id' => 'auth_challenge_1' ] ); + $this->queue( 'POST /auth/challenges/auth_challenge_1/verify', [ 'valid' => false ] ); + + for ( $i = 0; $i < 4; $i++ ) { + $this->assertSame( 'workos_mfa_invalid_code', $service->check_code( $this->user_id, 'auth_factor_1', '000000' )->get_error_code() ); + } + + $this->queue( 'POST /auth/challenges/auth_challenge_1/verify', $this->valid( 'auth_factor_1' ) ); + $this->assertTrue( $service->check_code( $this->user_id, 'auth_factor_1', '123456' ) ); + + $this->queue( 'POST /auth/challenges/auth_challenge_1/verify', [ 'valid' => false ] ); + $this->assertSame( 'workos_mfa_invalid_code', $service->check_code( $this->user_id, 'auth_factor_1', '000000' )->get_error_code() ); + $this->assertSame( 'workos_mfa_too_many_codes', $service->check_code( $this->user_id, 'auth_factor_1', '000000' )->get_error_code() ); + } + + // ------------------------- Token required ------------------------- + + public function test_challenge_and_verify_refuse_requests_without_a_parked_sign_in(): void { + $challenge = $this->dispatch( '/workos/v1/auth/mfa/challenge', [ 'factor_id' => 'auth_factor_1' ] ); + $this->assertSame( 403, $challenge->get_status() ); + $this->assertSame( 'workos_authkit_mfa_expired', $challenge->get_data()['code'] ); + + $verify = $this->dispatch( + '/workos/v1/auth/mfa/verify', + [ + 'pending_authentication_token' => 'pat_stolen', + 'authentication_challenge_id' => 'auth_challenge_attacker', + 'code' => '123456', + ] + ); + $this->assertSame( 403, $verify->get_status() ); + $this->assertSame( 'workos_authkit_mfa_expired', $verify->get_data()['code'] ); + $this->assertEmpty( $this->captured, 'Nothing reaches WorkOS.' ); + } + + public function test_workos_challenge_is_parked_and_bound_to_its_factors(): void { + $this->queue( + 'POST /user_management/authenticate', + [ + 'code' => 'mfa_challenge', + 'message' => 'The user must complete an MFA challenge.', + 'pending_authentication_token' => 'pat_secret', + 'authentication_factors' => [ [ 'id' => 'auth_factor_w', 'type' => 'totp' ] ], + 'user' => [ 'id' => 'user_mfa', 'email' => 'member@example.com' ], + ], + 403 + ); + + $data = $this->sign_in()->get_data(); + + $this->assertTrue( $data['mfa_required'] ); + $this->assertNotEmpty( $data['mfa_token'] ); + $this->assertArrayNotHasKey( 'pending_authentication_token', $data ); + $this->assertStringNotContainsString( 'pat_secret', (string) wp_json_encode( $data ) ); + + $other = $this->dispatch( '/workos/v1/auth/mfa/challenge', [ 'mfa_token' => $data['mfa_token'], 'factor_id' => 'auth_factor_attacker' ] ); + $this->assertSame( 404, $other->get_status() ); + + $this->queue( 'POST /auth/factors/auth_factor_w/challenge', [ 'id' => 'auth_challenge_w' ] ); + $own = $this->dispatch( '/workos/v1/auth/mfa/challenge', [ 'mfa_token' => $data['mfa_token'], 'factor_id' => 'auth_factor_w' ] ); + $this->assertSame( 200, $own->get_status() ); + } + + // ------------------------- Mirror flag ------------------------- + + public function test_factor_added_outside_wordpress_is_still_challenged(): void { + $this->factors( [ [ 'id' => 'auth_factor_outside', 'type' => 'totp' ] ] ); + + $data = $this->sign_in()->get_data(); + + $this->assertTrue( $data['mfa_required'] ); + $this->assertFalse( $data['enroll'] ); + $this->assertSame( [ [ 'id' => 'auth_factor_outside', 'type' => 'totp' ] ], $data['factors'] ); + $this->assertSame( 0, get_current_user_id() ); + $this->assertSame( '1', get_user_meta( $this->user_id, User::META_MFA_ENABLED, true ) ); + } + + public function test_factors_vanishing_from_workos_keep_asking_for_enrollment(): void { + update_user_meta( $this->user_id, User::META_MFA_ENABLED, '1' ); + $this->factors( [] ); + + $first = $this->sign_in()->get_data(); + $this->assertTrue( $first['enroll'] ); + $this->assertSame( '1', get_user_meta( $this->user_id, User::META_MFA_ENABLED, true ) ); + + $second = $this->sign_in()->get_data(); + $this->assertTrue( $second['enroll'] ); + $this->assertSame( 0, get_current_user_id() ); + } + + // ------------------------- PendingLogin ------------------------- + + public function test_touching_a_parked_sign_in_never_extends_it(): void { + $token = PendingLogin::park( + [ + 'profile' => 'members', + 'user_id' => $this->user_id, + 'response' => [ 'access_token' => 'secret_at_123' ], + ] + ); + $key = 'workos_mfa_pending_' . substr( hash( 'sha256', $token ), 0, 40 ); + + $raw = get_transient( $key ); + $this->assertIsString( $raw['response'] ); + $this->assertStringNotContainsString( 'secret_at_123', (string) wp_json_encode( $raw ) ); + $this->assertSame( 'secret_at_123', PendingLogin::get( $token, 'members' )['response']['access_token'] ); + + $raw['expires_at'] = time() + 30; + set_transient( $key, $raw, 600 ); + $record = PendingLogin::get( $token, 'members' ); + PendingLogin::update( $token, $record ); + $this->assertLessThanOrEqual( time() + 30, PendingLogin::get( $token, 'members' )['expires_at'] ); + + $raw = get_transient( $key ); + $raw['expires_at'] = time() - 1; + set_transient( $key, $raw, 600 ); + $this->assertNull( PendingLogin::get( $token, 'members' ) ); + $this->assertFalse( get_transient( $key ) ); + } + + public function test_update_does_not_bring_back_a_finished_sign_in(): void { + $token = PendingLogin::park( + [ + 'profile' => 'members', + 'user_id' => $this->user_id, + ] + ); + $record = PendingLogin::get( $token, 'members' ); + + PendingLogin::forget( $token ); + PendingLogin::update( $token, $record ); + + $this->assertNull( PendingLogin::get( $token, 'members' ) ); + } + + public function test_resume_code_works_once(): void { + $code = PendingLogin::handoff( 'the_token' ); + + $this->assertNotSame( 'the_token', $code ); + $this->assertSame( 'the_token', PendingLogin::redeem( $code ) ); + $this->assertSame( '', PendingLogin::redeem( $code ) ); + } + + // ------------------------- Password guard ------------------------- + + public function test_xmlrpc_multicall_app_password_does_not_exempt_another_user(): void { + $a = self::factory()->user->create_and_get(); + $b = get_userdata( $this->user_id ); + update_user_meta( $a->ID, User::META_MFA_ENABLED, '1' ); + update_user_meta( $b->ID, User::META_MFA_ENABLED, '1' ); + + // Call 1 of the multicall authenticated A with an application password. + do_action( 'application_password_did_authenticate', $a, [] ); + + // Call 2: B with a password. + $this->assertWPError( apply_filters( 'authenticate', $b, 'mfa_member', 'wp_password_123' ) ); + + // The exemption only lasted for its own run, even for A. + $this->assertWPError( apply_filters( 'authenticate', $a, $a->user_login, 'x' ) ); + + // In the run where A's application password authenticated, A passes. + $fire = static function ( $user ) use ( $a ) { + do_action( 'application_password_did_authenticate', $a, [] ); + return $user; + }; + add_filter( 'authenticate', $fire, 50 ); + $this->assertSame( $a->ID, apply_filters( 'authenticate', $a, $a->user_login, 'x' )->ID ); + remove_filter( 'authenticate', $fire, 50 ); + } + + public function test_always_profile_blocks_password_only_sign_in_for_linked_users(): void { + $this->save_profile( Profile::DEFAULT_SLUG, Profile::MFA_ENFORCE_ALWAYS ); + $guard = App::container()->get( PasswordGuard::class ); + $unlinked = self::factory()->user->create(); + + $this->assertTrue( $guard->must_use_mfa( $this->user_id ) ); + $this->assertFalse( $guard->must_use_mfa( $unlinked ) ); + } + + // ------------------------- Bearer tokens ------------------------- + + public function test_bearer_token_from_another_app_cannot_act_for_an_mfa_user(): void { + update_user_meta( $this->user_id, User::META_MFA_ENABLED, '1' ); + $plain = self::factory()->user->create(); + + $this->assertSame( 0, apply_filters( 'workos_bearer_token_user', $this->user_id, [ 'sid' => 's1' ] ) ); + $this->assertSame( $plain, apply_filters( 'workos_bearer_token_user', $plain, [ 'sid' => 's1' ] ) ); + + VerifiedSessions::remember( $this->user_id, [ 'access_token' => $this->jwt( [ 'sid' => 's1' ] ) ] ); + + $this->assertSame( $this->user_id, apply_filters( 'workos_bearer_token_user', $this->user_id, [ 'sid' => 's1' ] ) ); + $this->assertSame( 0, apply_filters( 'workos_bearer_token_user', $this->user_id, [ 'sid' => 's2' ] ) ); + } + + // ------------------------- Re-link ------------------------- + + public function test_mfa_account_cannot_be_relinked_to_another_workos_identity(): void { + $plain = self::factory()->user->create(); + update_user_meta( $this->user_id, User::META_MFA_ENABLED, '1' ); + + $this->assertTrue( apply_filters( 'workos_relink_protected', false, $this->user_id ) ); + $this->assertFalse( apply_filters( 'workos_relink_protected', false, $plain ) ); + + $this->assertFalse( UserSync::link_user( $this->user_id, [ 'id' => 'user_attacker', 'email' => 'member@example.com' ], 'test' ) ); + $this->assertSame( 'user_mfa', User::get_workos_id( $this->user_id ) ); + } + + // ------------------------- Account changes ------------------------- + + public function test_adding_an_authenticator_emails_and_signs_out_other_sessions(): void { + $manager = WP_Session_Tokens::get_instance( $this->user_id ); + $mine = $manager->create( time() + HOUR_IN_SECONDS ); + $other = $manager->create( time() + HOUR_IN_SECONDS ); + wp_set_current_user( $this->user_id ); + $_COOKIE[ LOGGED_IN_COOKIE ] = wp_generate_auth_cookie( $this->user_id, time() + HOUR_IN_SECONDS, 'logged_in', $mine ); + + do_action( + 'workos_mfa_enrolled', + $this->user_id, + [ + 'id' => 'auth_factor_1', + 'type' => 'totp', + ] + ); + + $this->assertTrue( $manager->verify( $mine ) ); + $this->assertFalse( $manager->verify( $other ) ); + $this->assertCount( 1, $this->mail ); + $this->assertSame( 'member@example.com', $this->mail[0]['to'] ); + } + + public function test_admin_reset_signs_the_user_out_everywhere(): void { + $manager = WP_Session_Tokens::get_instance( $this->user_id ); + $manager->create( time() + HOUR_IN_SECONDS ); + update_user_meta( $this->user_id, User::META_ACCESS_TOKEN, 'at' ); + update_user_meta( $this->user_id, User::META_REFRESH_TOKEN, 'rt' ); + update_user_meta( $this->user_id, User::META_SESSION_ID, 'sess_1' ); + wp_set_current_user( $this->admin_id ); + + do_action( 'workos_mfa_reset', $this->user_id, 1 ); + + $this->assertSame( [], $manager->get_all() ); + $this->assertSame( '', (string) get_user_meta( $this->user_id, User::META_REFRESH_TOKEN, true ) ); + $this->assertSame( '', (string) get_user_meta( $this->user_id, User::META_SESSION_ID, true ) ); + $this->assertNotEmpty( array_filter( $this->captured, static fn( $c ) => str_contains( $c['url'], 'sess_1' ) ) ); + $this->assertCount( 1, $this->mail ); + } + + public function test_notify_filter_suppresses_the_email(): void { + add_filter( 'workos_mfa_notify_user', '__return_false' ); + + do_action( 'workos_mfa_factor_removed', $this->user_id, 'auth_factor_1' ); + + $this->assertSame( [], $this->mail ); + } + + // ------------------------- Admin panel ------------------------- + + public function test_panel_changes_wait_for_core_to_save_the_profile(): void { + wp_set_current_user( $this->admin_id ); + $_POST['workos_mfa_required'] = '1'; + $panel = new UserEditPanel( new FactorService(), new Policy( new Settings() ) ); + $panel->register(); + + $panel->save( $this->user_id ); + $this->assertSame( '', (string) get_user_meta( $this->user_id, User::META_MFA_REQUIRED, true ) ); + + do_action( 'profile_update', $this->user_id, get_userdata( $this->user_id ), [] ); + $this->assertSame( '1', get_user_meta( $this->user_id, User::META_MFA_REQUIRED, true ) ); + } + + // -------------------------------- Fixtures -------------------------------- + + private function enroll_user(): void { + update_user_meta( $this->user_id, User::META_MFA_ENABLED, '1' ); + $this->factors( [ [ 'id' => 'auth_factor_1', 'type' => 'totp' ] ] ); + } + + private function factors( array $factors ): void { + $this->queue( 'GET /user_management/users/user_mfa/auth_factors', [ 'data' => $factors ] ); + } + + private function valid( string $factor_id ): array { + return [ + 'valid' => true, + 'challenge' => [ 'authentication_factor_id' => $factor_id ], + ]; + } + + private function jwt( array $claims ): string { + return 'eyJhbGciOiJSUzI1NiJ9.' . rtrim( strtr( base64_encode( (string) wp_json_encode( $claims ) ), '+/', '-_' ), '=' ) . '.sig'; + } + + private function sign_in( string $profile = 'members' ): WP_REST_Response { + if ( ! isset( $this->responses['POST /user_management/authenticate'] ) ) { + $this->queue( + 'POST /user_management/authenticate', + [ + 'access_token' => $this->jwt( + [ + 'sub' => 'user_mfa', + 'sid' => 'sess_1', + ] + ), + 'refresh_token' => 'rt_mfa', + 'user' => [ 'id' => 'user_mfa', 'email' => 'member@example.com' ], + ] + ); + } + + return $this->dispatch( + '/workos/v1/auth/password/authenticate', + [ + 'email' => 'member@example.com', + 'password' => 'CorrectPassword!1', + ], + $profile + ); + } + + private function enroll_response(): array { + return [ + 'authentication_factor' => [ + 'id' => 'auth_factor_new', + 'type' => 'totp', + 'totp' => [ + 'qr_code' => 'data:image/png;base64,AAA', + 'secret' => 'SECRET', + 'uri' => 'otpauth://totp/Site:member@example.com?secret=SECRET', + ], + ], + 'authentication_challenge' => [ 'id' => 'auth_challenge_enroll' ], + ]; + } + + private function save_profile( string $slug, string $enforce ): void { + $this->repository->save( + Profile::from_array( + [ + 'slug' => $slug, + 'title' => $slug, + 'methods' => [ Profile::METHOD_PASSWORD ], + 'mfa' => [ + 'enforce' => $enforce, + 'factors' => [ Profile::FACTOR_TOTP ], + ], + ] + ) + ); + } + + private function queue( string $key, array $body, int $code = 200 ): void { + $this->responses[ $key ] = [ + 'response' => [ 'code' => $code, 'message' => 'OK' ], + 'body' => wp_json_encode( $body ), + ]; + } + + private function dispatch( string $route, array $body, string $profile = 'members' ): WP_REST_Response { + $request = new WP_REST_Request( 'POST', $route ); + $request->set_header( 'Content-Type', 'application/json' ); + $request->set_header( 'X-WorkOS-Nonce', $this->nonce->mint( $profile ) ); + $request->set_body( wp_json_encode( array_merge( [ 'profile' => $profile ], $body ) ) ); + + return rest_get_server()->dispatch( $request ); + } + + private function account( string $method, int $user_id, string $path, array $body = [] ): WP_REST_Response { + $request = new WP_REST_Request( $method, '/workos/v1/users/' . $user_id . '/mfa' . $path ); + $request->set_header( 'Content-Type', 'application/json' ); + if ( $body ) { + $request->set_body( wp_json_encode( $body ) ); + } + + return rest_get_server()->dispatch( $request ); + } +} From 7430ef1cafefdb4600445f216882566d0081c0ff Mon Sep 17 00:00:00 2001 From: Gustavo Bordoni Date: Sun, 18 Oct 2026 23:16:15 +0000 Subject: [PATCH 2/2] fix(mfa): follow factors removed in WorkOS WorkOS is the source of truth for a user's factors. When WorkOS answers with none, list_factors() clears _workos_mfa_enabled instead of keeping it, logs mfa.removed_in_workos, fires workos_mfa_removed_in_workos and emails the user. A failed WorkOS call never clears the flag, and the require flag and roles are untouched, so required users enroll again. --- CHANGELOG.md | 2 +- docs/mfa.md | 38 ++++++++++------- src/WorkOS/ActivityLog/AdminPage.php | 2 +- src/WorkOS/Auth/AuthKit/LoginCompleter.php | 11 +++-- src/WorkOS/Mfa/AccountChanges.php | 23 ++++++++++- src/WorkOS/Mfa/FactorService.php | 48 ++++++++++++++++++---- tests/wpunit/MfaFactorServiceTest.php | 4 +- tests/wpunit/MfaSecurityTest.php | 38 ++++++++++++++--- 8 files changed, 125 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 16dad32..bb240da 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,7 +24,7 @@ - **Adding a second authenticator** to an account that has one needs a current code from an existing one (`factor_id`, `code`; `403 workos_mfa_code_required`). This covers the account panel and the legacy `/auth/mfa/totp/enroll` and `/auth/mfa/sms/enroll` routes, which also follow `workos_mfa_can_manage` now. - **Wrong codes are capped per account**, 5 per 15 minutes and 30 a day, across sign-ins and the account panel (`429 workos_mfa_too_many_codes`), under a per-account lock so parallel requests can't overspend it. - **Sign-in codes**: `/auth/mfa/challenge` and `/auth/mfa/verify` need an `mfa_token`. WorkOS's own `mfa_challenge` is parked like the plugin's, so the browser never holds WorkOS's pending token and can't pair it with a challenge from another account. A code must come from the factor its challenge was made for. - - **Factors added outside WordPress** (the WorkOS dashboard, another app, before this version) are challenged: the policy reads the user's factors from WorkOS at every sign-in. Factors removed outside WordPress send the user to enroll instead of letting them in with a password. + - **Factors added outside WordPress** (the WorkOS dashboard, another app, before this version) are challenged: the policy reads the user's factors from WorkOS at every sign-in. Factors removed in WorkOS turn the user's MFA off in WordPress to match (activity-log event `mfa.removed_in_workos`, action `workos_mfa_removed_in_workos`, an email to the user); users who must use MFA enroll again. - **Re-linking**: a user with MFA is never re-linked to a different WorkOS user by email, whatever the Re-link Protection setting (filter `workos_relink_protected`). - **Application passwords** exempt only their own user from the password-only guard, so an XML-RPC `system.multicall` can't borrow one to sign in another user. The guard also follows a default Login Profile set to `always`, for users linked to WorkOS. - **A parked sign-in** expires 10 minutes after the first factor however often it's touched, and its stored WorkOS response is encrypted with a key derived from the token. The hosted-AuthKit return URL carries a one-time resume code instead of the token. diff --git a/docs/mfa.md b/docs/mfa.md index d2c3f18..6b3e017 100644 --- a/docs/mfa.md +++ b/docs/mfa.md @@ -102,7 +102,7 @@ None of its methods check capabilities, so callers do. | Method | Does | |---|---| | `status( $user_id )` | `{ enabled, required, must_use, factors }`, read from WorkOS. `required` is the admin flag; `must_use` adds roles and filters | -| `list_factors( $user_id )` | Confirmed factors from WorkOS. Sets the mirrored flag when WorkOS has a factor it didn't know about; never clears it (see below) | +| `list_factors( $user_id )` | Confirmed factors from WorkOS. Updates the mirrored flag to match: set when WorkOS has one, cleared when it has none (see below) | | `start_enrollment( $user_id )` | Creates a TOTP factor and returns `{ factor_id, challenge_id, qr_code, secret, otpauth_uri }`. The factor stays unconfirmed, and doesn't count, until confirmed. One at a time per account | | `confirm_enrollment( $user_id, $factor_id, $code, $challenge_id = '' )` | Checks the first code and turns MFA on | | `check_code( $user_id, $factor_id, $code, $challenge_id = '' )` | Checks a code, counted against the account's wrong-code limit; no hooks | @@ -115,13 +115,18 @@ None of its methods check capabilities, so callers do. An unconfirmed factor is deleted from WorkOS when the user starts over, or after a day. If WorkOS can't delete it, it stays tracked as unconfirmed. -`_workos_mfa_enabled` means "this user turned MFA on". It's set when a factor -is confirmed or found in WorkOS, and cleared only when the user removes their -last factor or an admin resets them. If factors disappear from WorkOS some -other way (the WorkOS dashboard, another app), the next sign-in asks the user -to enroll again rather than letting them in with a password alone. To turn a -user's MFA off, use the reset (admin screen, `wp workos mfa reset`, -`workos_mfa_reset()`), not the WorkOS dashboard. +`_workos_mfa_enabled` mirrors WorkOS, which is the source of truth. It's set +when a factor is confirmed or found in WorkOS, and cleared when the user +removes their last factor, an admin resets them, or WorkOS answers with no +factors. So removing a user's factors in the WorkOS dashboard (or another app) +turns their MFA off in WordPress at the next check: their next sign-in, or +their account panel. WordPress logs `mfa.removed_in_workos`, fires +`workos_mfa_removed_in_workos` and emails the user. A failed WorkOS call +never clears the flag. The require flag and required roles don't change, so a +user who must use MFA enrolls again at their next sign-in. + +The reset (admin screen, `wp workos mfa reset`, `workos_mfa_reset()`) does +the same and also signs the user out everywhere. ### PHP helpers @@ -171,7 +176,8 @@ from the token, which the server doesn't keep. No WordPress session exists until the code checks out. In `plugin` mode the policy asks WorkOS for the user's factors at every -sign-in, so a factor added outside WordPress is challenged too. Only factors +sign-in, so a factor added outside WordPress is challenged too, and one +removed there no longer is. Only factors the profile allows can answer; a user who turned MFA on but has none the profile can use is asked to enroll. @@ -336,7 +342,7 @@ const { enabled, required } = await res.json(); | `workos_mfa_can_manage` | `bool $can, int $user_id` | `true` for the account owner | | `workos_mfa_can_manage_user` | `bool $can, int $user_id` | `edit_user` on the target, never yourself | | `workos_mfa_accept_bearer_token` | `bool $passed, int $user_id, array $payload` | Whether the token's session signed in on this site | -| `workos_mfa_notify_user` | `bool $send, int $user_id, string $event` | `true` (`enrolled`, `removed`, `reset`) | +| `workos_mfa_notify_user` | `bool $send, int $user_id, string $event` | `true` (`enrolled`, `removed`, `reset`, `removed_in_workos`) | | `workos_bearer_token_user` | `int $wp_user_id, array $payload` | The user the token's `sub` maps to; 0 refuses (not MFA-specific) | | `workos_relink_protected` | `bool $protected, int $wp_user_id` | The Re-link Protection setting; MFA turns it on for users with MFA (not MFA-specific) | @@ -347,6 +353,7 @@ const { enabled, required } = await res.json(); | `workos_mfa_enrolled` | `int $user_id, array $factor` | A new factor is confirmed | | `workos_mfa_factor_removed` | `int $user_id, string $factor_id` | One factor is removed | | `workos_mfa_reset` | `int $user_id, int $removed` | Every factor is removed | +| `workos_mfa_removed_in_workos` | `int $user_id` | WorkOS has no factors left for a user WordPress had as enrolled, and WordPress turns MFA off to match | | `workos_mfa_required_changed` | `int $user_id, bool $required` | The require flag changes | | `workos_mfa_challenge_succeeded` | `int $user_id` | The plugin's sign-in challenge is answered | | `workos_mfa_challenge_failed` | `int $user_id, string $error_code` | A wrong code at the plugin's sign-in challenge | @@ -359,6 +366,7 @@ const { enabled, required } = await res.json(); | `mfa.enrolled` | `factor_id`, `factor_type`, `actor_id` | | `mfa.removed` | `factor_id`, `actor_id` | | `mfa.reset` | `removed`, `actor_id` | +| `mfa.removed_in_workos` | — | | `mfa.required_set`, `mfa.required_cleared` | `actor_id` | | `mfa.challenge_succeeded`, `mfa.challenge_failed` | `factor_type` | | `mfa.blocked` | `profile` | @@ -393,9 +401,9 @@ MFA: - **The account can't be moved to another identity.** A user with MFA is never re-linked to a different WorkOS user by email; an admin can still with `wp workos user link --force`. -- **Factors don't vanish quietly.** A factor removed outside WordPress leaves - the user enrolling again at the next sign-in, not signing in with a - password. +- **WorkOS stays the source of truth.** Factors added outside WordPress are + challenged; factors removed there turn MFA off in WordPress, with an email + to the user and an activity-log event. Users who must use MFA enroll again. Known limits: @@ -411,8 +419,8 @@ Known limits: - Don't store TOTP secrets or QR codes in WordPress. WorkOS holds them; the plugin returns them once, at enrollment. -- Don't write `_workos_mfa_enabled` yourself, and don't remove factors in the - WorkOS dashboard to turn MFA off. Use the reset. +- Don't write `_workos_mfa_enabled` yourself. Change factors in WorkOS or + through the plugin, and the flag follows. - Don't let a profile with `enforce: never` stand in for "MFA off for this user". It blocks users who have MFA. Use `workos_mfa_decision` for exceptions. diff --git a/src/WorkOS/ActivityLog/AdminPage.php b/src/WorkOS/ActivityLog/AdminPage.php index 3f9bf33..ed86d0b 100644 --- a/src/WorkOS/ActivityLog/AdminPage.php +++ b/src/WorkOS/ActivityLog/AdminPage.php @@ -91,7 +91,7 @@ public function render_page(): void { // phpcs:ignore WordPress.Security.NonceVerification.Recommended $cleared = ! empty( SuperGlobals::get_get_var( 'cleared' ) ); - $event_types = [ 'login', 'logout', 'login_failed', 'login_denied', 'user_suspended', 'onboarding_sync', 'bypass_activated', 'link_refused', 'mfa.enrolled', 'mfa.removed', 'mfa.reset', 'mfa.required_set', 'mfa.required_cleared', 'mfa.challenge_succeeded', 'mfa.challenge_failed', 'mfa.blocked' ]; + $event_types = [ 'login', 'logout', 'login_failed', 'login_denied', 'user_suspended', 'onboarding_sync', 'bypass_activated', 'link_refused', 'mfa.enrolled', 'mfa.removed', 'mfa.reset', 'mfa.removed_in_workos', 'mfa.required_set', 'mfa.required_cleared', 'mfa.challenge_succeeded', 'mfa.challenge_failed', 'mfa.blocked' ]; ?>

diff --git a/src/WorkOS/Auth/AuthKit/LoginCompleter.php b/src/WorkOS/Auth/AuthKit/LoginCompleter.php index 2892581..2ec1265 100644 --- a/src/WorkOS/Auth/AuthKit/LoginCompleter.php +++ b/src/WorkOS/Auth/AuthKit/LoginCompleter.php @@ -307,8 +307,8 @@ private function plugin_mfa_step( WP_User $wp_user, array $workos_response, Prof // Ask WorkOS rather than trust the mirrored flag: a factor added // outside WordPress (or before this version) must still be - // challenged. list_factors() sets the flag when it finds one, so the - // policy is asked again. + // challenged, and one removed there no longer is. list_factors() + // updates the flag to match, so the policy is asked again. if ( Policy::BLOCK !== $decision ) { $factors = App::container()->get( FactorService::class )->list_factors( $wp_user->ID ); if ( is_wp_error( $factors ) ) { @@ -325,10 +325,9 @@ private function plugin_mfa_step( WP_User $wp_user, array $workos_response, Prof ) ); - // The user turned MFA on, but WorkOS has no factor this profile - // can use (removed outside WordPress, or SMS only). The flag - // stays set, so a challenge with nothing to challenge becomes an - // enrollment, at this sign-in and every one after it. + // The user has factors, but none this profile can use (SMS + // only, say). A challenge with nothing to challenge becomes an + // enrollment. if ( Policy::CHALLENGE === $decision && empty( $factors ) ) { $decision = Policy::ENROLL; } diff --git a/src/WorkOS/Mfa/AccountChanges.php b/src/WorkOS/Mfa/AccountChanges.php index 0245412..7e6e4eb 100644 --- a/src/WorkOS/Mfa/AccountChanges.php +++ b/src/WorkOS/Mfa/AccountChanges.php @@ -20,6 +20,8 @@ * outlive the change. * - Factors reset by an admin (or WP-CLI): an email, every session signs * out, and the stored WorkOS session is revoked. + * - Factors removed in WorkOS (its dashboard, another app): an email. The + * sessions stay; whoever removed them there can revoke WorkOS sessions. * * An authenticator added at sign-in also sends the email, so an owner * whose password was used to enroll someone else's app hears about it. @@ -35,6 +37,7 @@ public function register(): void { add_action( 'workos_mfa_enrolled', [ $this, 'enrolled' ] ); add_action( 'workos_mfa_factor_removed', [ $this, 'removed' ] ); add_action( 'workos_mfa_reset', [ $this, 'reset' ] ); + add_action( 'workos_mfa_removed_in_workos', [ $this, 'removed_in_workos' ] ); } /** @@ -104,6 +107,22 @@ public function reset( $user_id ): void { ); } + /** + * Every factor was removed in WorkOS, and WordPress followed. + * + * @param int $user_id WP user ID. + * + * @return void + */ + public function removed_in_workos( $user_id ): void { + $this->notify( + (int) $user_id, + 'removed_in_workos', + __( 'Two-step verification: authenticators removed', 'integration-workos' ), + __( 'Every authenticator app on your account was removed, so you’ll sign in without a code. If your account requires two-step verification, you’ll set up a new app the next time you sign in.', 'integration-workos' ) + ); + } + /** * End the user's other sessions, when the change came from the user. * @@ -123,7 +142,7 @@ private function sign_out_others( int $user_id ): void { * Email the account about the change. * * @param int $user_id WP user ID. - * @param string $event `enrolled`, `removed` or `reset`. + * @param string $event `enrolled`, `removed`, `reset` or `removed_in_workos`. * @param string $subject Subject, without the site name. * @param string $message What happened. * @@ -140,7 +159,7 @@ private function notify( int $user_id, string $event, string $subject, string $m * * @param bool $send Default true. * @param int $user_id WP user ID. - * @param string $event `enrolled`, `removed` or `reset`. + * @param string $event `enrolled`, `removed`, `reset` or `removed_in_workos`. */ if ( ! apply_filters( 'workos_mfa_notify_user', true, $user_id, $event ) ) { return; diff --git a/src/WorkOS/Mfa/FactorService.php b/src/WorkOS/Mfa/FactorService.php index efe071c..231e625 100644 --- a/src/WorkOS/Mfa/FactorService.php +++ b/src/WorkOS/Mfa/FactorService.php @@ -23,11 +23,11 @@ * * WorkOS holds the factors and their secrets. WordPress mirrors only: * - * - `_workos_mfa_enabled`: the user turned MFA on. Set when a factor is - * confirmed, or found in WorkOS; cleared only when the user removes their - * last factor or an admin resets them. Factors that vanish from WorkOS - * some other way leave it set, so the next sign-in asks the user to - * enroll again instead of quietly dropping their second factor. + * - `_workos_mfa_enabled`: the user has a confirmed factor. WorkOS is the + * source of truth: the flag is set when a factor is confirmed or found in + * WorkOS, and cleared when the user removes their last factor, an admin + * resets them, or WorkOS answers with none (removed in the WorkOS + * dashboard or another app). A failed WorkOS call never clears it. * - `_workos_mfa_required`: an admin requires MFA for the user. * - `_workos_mfa_pending_factor`: a factor enrolled but not yet confirmed * with a code. It never counts as enabled, and is deleted from WorkOS @@ -87,9 +87,10 @@ public function status( int $user_id ) { /** * Confirmed factors, read from WorkOS. * - * Sets `_workos_mfa_enabled` when WorkOS has a factor the flag didn't - * know about (added outside WordPress, or before this version). Never - * clears it; see the class docblock. Drops a stale unconfirmed factor. + * Mirrors the answer into `_workos_mfa_enabled`: set when WorkOS has a + * factor the flag didn't know about (added outside WordPress, or before + * this version), cleared when WorkOS has none left. Drops a stale + * unconfirmed factor. * * @param int $user_id WP user ID. * @@ -128,11 +129,42 @@ public function list_factors( int $user_id ) { if ( ! empty( $factors ) ) { $this->sync_enabled( $user_id, true ); + } elseif ( $this->is_enabled( $user_id ) ) { + $this->removed_in_workos( $user_id ); } return $factors; } + /** + * Follow factors removed outside WordPress: turn the user's MFA off. + * + * The require flag and required roles are untouched, so a user who must + * use MFA enrolls again at their next sign-in. + * + * @param int $user_id WP user ID. + * + * @return void + */ + private function removed_in_workos( int $user_id ): void { + $this->sync_enabled( $user_id, false ); + + EventLogger::log( + 'mfa.removed_in_workos', + [ + 'user_id' => $user_id, + ] + ); + + /** + * Fires when WorkOS has no factors left for a user WordPress had as + * enrolled, and WordPress turns their MFA off to match. + * + * @param int $user_id WP user ID. + */ + do_action( 'workos_mfa_removed_in_workos', $user_id ); + } + /** * Start a TOTP enrollment. * diff --git a/tests/wpunit/MfaFactorServiceTest.php b/tests/wpunit/MfaFactorServiceTest.php index a76e068..a89faa3 100644 --- a/tests/wpunit/MfaFactorServiceTest.php +++ b/tests/wpunit/MfaFactorServiceTest.php @@ -263,7 +263,7 @@ public function test_confirm_rejects_factor_that_is_not_pending(): void { // --------------------------------- Listing --------------------------------- - public function test_list_hides_pending_factor_and_keeps_enabled_flag(): void { + public function test_list_hides_pending_factor_and_follows_workos_off(): void { $this->set_pending( 'auth_factor_pending' ); update_user_meta( $this->user_id, User::META_MFA_ENABLED, '1' ); $this->queue( @@ -274,7 +274,7 @@ public function test_list_hides_pending_factor_and_keeps_enabled_flag(): void { $factors = $this->service->list_factors( $this->user_id ); $this->assertSame( [], $factors ); - $this->assertTrue( $this->service->is_enabled( $this->user_id ), 'Factors gone from WorkOS don\'t turn MFA off; the next sign-in enrolls.' ); + $this->assertFalse( $this->service->is_enabled( $this->user_id ), 'Only an unconfirmed factor left in WorkOS turns MFA off.' ); $this->queue( 'GET /user_management/users/user_mfa/auth_factors', diff --git a/tests/wpunit/MfaSecurityTest.php b/tests/wpunit/MfaSecurityTest.php index 33dbf2d..d89c428 100644 --- a/tests/wpunit/MfaSecurityTest.php +++ b/tests/wpunit/MfaSecurityTest.php @@ -370,17 +370,43 @@ public function test_factor_added_outside_wordpress_is_still_challenged(): void $this->assertSame( '1', get_user_meta( $this->user_id, User::META_MFA_ENABLED, true ) ); } - public function test_factors_vanishing_from_workos_keep_asking_for_enrollment(): void { + public function test_factors_removed_in_workos_turn_mfa_off(): void { + $fired = 0; + add_action( + 'workos_mfa_removed_in_workos', + static function () use ( &$fired ) { + ++$fired; + } + ); update_user_meta( $this->user_id, User::META_MFA_ENABLED, '1' ); $this->factors( [] ); - $first = $this->sign_in()->get_data(); - $this->assertTrue( $first['enroll'] ); - $this->assertSame( '1', get_user_meta( $this->user_id, User::META_MFA_ENABLED, true ) ); + $data = $this->sign_in()->get_data(); - $second = $this->sign_in()->get_data(); - $this->assertTrue( $second['enroll'] ); + $this->assertArrayNotHasKey( 'mfa_required', $data ); + $this->assertSame( $this->user_id, get_current_user_id() ); + $this->assertSame( '', get_user_meta( $this->user_id, User::META_MFA_ENABLED, true ) ); + $this->assertSame( 1, $fired ); + } + + public function test_factors_removed_in_workos_still_enroll_a_required_user(): void { + update_user_meta( $this->user_id, User::META_MFA_ENABLED, '1' ); + update_user_meta( $this->user_id, User::META_MFA_REQUIRED, '1' ); + $this->factors( [] ); + + $data = $this->sign_in()->get_data(); + + $this->assertTrue( $data['enroll'] ); $this->assertSame( 0, get_current_user_id() ); + $this->assertSame( '', get_user_meta( $this->user_id, User::META_MFA_ENABLED, true ) ); + } + + public function test_failed_factor_lookup_keeps_mfa_on(): void { + update_user_meta( $this->user_id, User::META_MFA_ENABLED, '1' ); + $this->queue( 'GET /user_management/users/user_mfa/auth_factors', [ 'message' => 'down' ], 500 ); + + $this->assertWPError( ( new FactorService() )->list_factors( $this->user_id ) ); + $this->assertSame( '1', get_user_meta( $this->user_id, User::META_MFA_ENABLED, true ) ); } // ------------------------- PendingLogin -------------------------