From 7a5ee98096e1b351105d72de065cd996e999b0c8 Mon Sep 17 00:00:00 2001 From: romanetar Date: Fri, 6 Nov 2026 16:04:22 +0100 Subject: [PATCH 01/14] feat(2fa): add POST users/me/2fa/disable and expose the 2FA state through an admin user serializer - disableTwoFactor endpoint: csrf, current password (or the enrollment window for a user without one), 412 for a group-enforced user or when 2FA is off, expires the device trust cookie - new AdminUserSerializer (SerializerType_Admin) = private + two_factor_enabled, two_factor_method, two_factor_enforced (group-derived) and two_factor_enforced_at, used by the admin user CRUD, the admin edit page and the user's own profile - the private and public serializers keep their shape: OAuth2 clients receive the private one and must not get the 2FA state --- app/Http/Controllers/AdminController.php | 2 +- .../Controllers/Api/UserApiController.php | 54 +++- app/Http/Controllers/UserController.php | 2 +- app/ModelSerializers/Auth/UserSerializer.php | 42 ++- app/ModelSerializers/SerializerRegistry.php | 3 + routes/web.php | 1 + tests/DisableTwoFactorEndpointTest.php | 241 ++++++++++++++++++ tests/TwoFactorProfileRoutesCsrfTest.php | 1 + 8 files changed, 340 insertions(+), 6 deletions(-) create mode 100644 tests/DisableTwoFactorEndpointTest.php diff --git a/app/Http/Controllers/AdminController.php b/app/Http/Controllers/AdminController.php index c885633f2..efd55f158 100644 --- a/app/Http/Controllers/AdminController.php +++ b/app/Http/Controllers/AdminController.php @@ -661,7 +661,7 @@ public function editUser($user_id){ [ 'user_id' => $user_id, 'user' => json_encode(SerializerRegistry::getInstance() - ->getSerializer($user, SerializerRegistry::SerializerType_Private)->serialize('groups')), + ->getSerializer($user, SerializerRegistry::SerializerType_Admin)->serialize('groups')), 'countries' => CountryList::getCountries(), 'languages' => $lang2Code, ] diff --git a/app/Http/Controllers/Api/UserApiController.php b/app/Http/Controllers/Api/UserApiController.php index fa2274680..f2ca8e78a 100644 --- a/app/Http/Controllers/Api/UserApiController.php +++ b/app/Http/Controllers/Api/UserApiController.php @@ -23,6 +23,8 @@ use Exception; use Illuminate\Http\Request as LaravelRequest; use Illuminate\Support\Facades\Auth; +use Illuminate\Support\Facades\Config; +use Illuminate\Support\Facades\Cookie; use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Request; use Illuminate\Support\Facades\Validator; @@ -158,7 +160,7 @@ public function lock($id) protected function getAllSerializerType(): string { - return SerializerRegistry::SerializerType_Private; + return SerializerRegistry::SerializerType_Admin; } /** @@ -298,6 +300,54 @@ public function enableTwoFactor() }); } + /** + * Disables 2FA for the current user (SDS idp-mfa.md §5.6). Deletes their recovery + * codes and revokes every trusted device, so the device trust cookie is expired too. + * Requires the current password, or for a user without one a session inside the + * recovery enrollment window (SDS rev 2.6 §4.10.3). Refused for group-enforced users. + * + * @return \Illuminate\Http\JsonResponse|mixed + */ + public function disableTwoFactor() + { + if (!Auth::check()) + return $this->error403(); + + return $this->processRequest(function () { + $data = Request::all(); + $validator = Validator::make($data, [ + 'current_password' => 'sometimes|nullable|string', + ]); + + if (!$validator->passes()) { + return $this->error412($validator->getMessageBag()->getMessages()); + } + + $user = Auth::user(); + + if ($user->isTwoFactorEnforcedByGroup()) { + return $this->error412(['two_factor' => ['Two-factor authentication is required for members of an administrator group and cannot be turned off.']]); + } + + if (!$user->isTwoFactorEnabled()) { + return $this->error412(['two_factor' => ['Two-factor authentication is not enabled for this account.']]); + } + + // the service enforces current_password / the enrollment window for the + // self-service actor and throws ValidationException (-> 412) + $this->recovery_code_service->disableTwoFactor($user, $data['current_password'] ?? null, $user); + + // every trusted device was just revoked + Cookie::queue(Cookie::forget( + Config::get('two_factor.cookie_name', 'device_trust_token'), + Config::get('session.path'), + Config::get('session.domain') + )); + + return $this->ok(['two_factor_enabled' => false]); + }); + } + /** * Invalidates the current user's recovery codes and generates a fresh batch. * Plaintext codes are returned once in the response and never persisted. @@ -438,6 +488,6 @@ public function updatePic($id) protected function serializerType(): string { - return SerializerRegistry::SerializerType_Private; + return SerializerRegistry::SerializerType_Admin; } } \ No newline at end of file diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index f765b5c6d..be3ae6ab5 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -1208,7 +1208,7 @@ public function getProfile() return View::make("profile", array_merge([ 'user' => json_encode(SerializerRegistry::getInstance()->getSerializer( - $user, SerializerRegistry::SerializerType_Private)->serialize()), + $user, SerializerRegistry::SerializerType_Admin)->serialize()), "openid_url" => $this->server_configuration_service->getUserIdentityEndpointURL($user->getIdentifier()), "sites" => $sites, 'actions' => $actions, diff --git a/app/ModelSerializers/Auth/UserSerializer.php b/app/ModelSerializers/Auth/UserSerializer.php index dd4ef9354..bcf26ab48 100644 --- a/app/ModelSerializers/Auth/UserSerializer.php +++ b/app/ModelSerializers/Auth/UserSerializer.php @@ -43,7 +43,7 @@ final class PublicUserSerializer extends BaseUserSerializer * Class PrivateUserSerializer * @package App\ModelSerializers\Auth */ -final class PrivateUserSerializer extends BaseUserSerializer +class PrivateUserSerializer extends BaseUserSerializer { protected static $allowed_relations = [ @@ -130,4 +130,42 @@ public function serialize($expand = null, array $fields = [], array $relations = return $values; } -} \ No newline at end of file +} + +/** + * Class AdminUserSerializer + * The private user plus the 2FA state. For the user admin CRUD, the admin edit page and the + * user's own profile. Never for OAuth2 clients: they get the private serializer, whose + * shape does not change. + * @package App\ModelSerializers\Auth + */ +final class AdminUserSerializer extends PrivateUserSerializer +{ + protected static $array_mappings = [ + 'TwoFactorEnabled' => 'two_factor_enabled:json_boolean', + 'TwoFactorMethod' => 'two_factor_method:json_string', + 'TwoFactorEnforcedAt' => 'two_factor_enforced_at:datetime_epoch', + ]; + + /** + * @param null $expand + * @param array $fields + * @param array $relations + * @param array $params + * @return array + */ + public function serialize($expand = null, array $fields = [], array $relations = [], array $params = []) + { + $user = $this->object; + if (!$user instanceof User) return []; + + $values = parent::serialize($expand, $fields, $relations, $params); + + // group-derived, read only: not a stored column + if (empty($fields) || in_array('two_factor_enforced', $fields)) { + $values['two_factor_enforced'] = $user->isTwoFactorEnforcedByGroup(); + } + + return $values; + } +} diff --git a/app/ModelSerializers/SerializerRegistry.php b/app/ModelSerializers/SerializerRegistry.php index 12250286e..4ca269439 100644 --- a/app/ModelSerializers/SerializerRegistry.php +++ b/app/ModelSerializers/SerializerRegistry.php @@ -11,6 +11,7 @@ * See the License for the specific language governing permissions and * limitations under the License. **/ +use App\ModelSerializers\Auth\AdminUserSerializer; use App\ModelSerializers\Auth\PrivateUserSerializer; use App\ModelSerializers\Auth\PublicGroupSerializer; use App\ModelSerializers\Auth\PublicUserSerializer; @@ -43,6 +44,7 @@ final class SerializerRegistry const SerializerType_Public = 'PUBLIC'; const SerializerType_Private = 'PRIVATE'; + const SerializerType_Admin = 'ADMIN'; private function __clone(){} @@ -79,6 +81,7 @@ private function __construct() $this->registry["User"] = [ self::SerializerType_Public => PublicUserSerializer::class, self::SerializerType_Private => PrivateUserSerializer::class, + self::SerializerType_Admin => AdminUserSerializer::class, ]; $this->registry["UserAction"] = UserActionSerializer::class; diff --git a/routes/web.php b/routes/web.php index 281351c29..e77ea8107 100644 --- a/routes/web.php +++ b/routes/web.php @@ -202,6 +202,7 @@ Route::post('recovery-codes/enroll', ['middleware' => ['csrf', '2fa.rate:enroll'], 'uses' => "UserApiController@enrollRecoveryCodes"]); Route::post('recovery-codes/acknowledge', ['middleware' => ['csrf'], 'uses' => "UserApiController@acknowledgeRecoveryCodes"]); Route::post('2fa/enable', ['middleware' => ['csrf'], 'uses' => "UserApiController@enableTwoFactor"]); + Route::post('2fa/disable', ['middleware' => ['csrf'], 'uses' => "UserApiController@disableTwoFactor"]); }); Route::get('access-tokens', ['middleware' => ['openstackid.currentuser.serveradmin.json'], 'uses' => 'ClientApiController@getAllAccessTokens']); diff --git a/tests/DisableTwoFactorEndpointTest.php b/tests/DisableTwoFactorEndpointTest.php new file mode 100644 index 000000000..36de5807e --- /dev/null +++ b/tests/DisableTwoFactorEndpointTest.php @@ -0,0 +1,241 @@ +withoutMiddleware(); + Session::start(); + } + + private function enrolledUser(bool $withPassword = true): User + { + $payload = [ + 'first_name' => 'Disable', + 'last_name' => 'Endpoint', + 'email' => 'disable.' . uniqid() . '@test.invalid', + 'active' => true, + 'email_verified' => true, + 'identifier' => 'disable.' . uniqid(), + ]; + if ($withPassword) { + $payload['password'] = self::SEED_PASSWORD; + $payload['password_enc'] = AuthHelper::AlgSHA1_V2_4; + } + $user = UserFactory::build($payload); + EntityManager::persist($user); + EntityManager::flush(); + + $this->app->make(IRecoveryCodeService::class) + ->enableTwoFactorAndGenerateCodes($user, User::MFAMethod_OTP); + $this->app->make(IDeviceTrustService::class)->trustDevice($user, 'agent-a', '127.0.0.1'); + $this->be($user); + return $user; + } + + private function disable(?string $password) + { + return $this->action('POST', 'Api\\UserApiController@disableTwoFactor', + is_null($password) ? [] : ['current_password' => $password], [], [], []); + } + + private function row(User $user): object + { + return DB::table('users')->where('id', $user->getId())->first(); + } + + private function assertStillEnrolled(User $user): void + { + $this->assertSame(1, (int)$this->row($user)->two_factor_enabled); + $this->assertSame(10, DB::table('user_recovery_codes')->where('user_id', $user->getId())->count()); + $this->assertSame(0, DB::table('user_trusted_devices') + ->where('user_id', $user->getId())->where('is_revoked', 1)->count()); + } + + public function testDisableWithCorrectPasswordClearsEverything(): void + { + $user = $this->enrolledUser(); + + $response = $this->disable(self::SEED_PASSWORD); + + $this->assertResponseStatus(200); + $this->assertSame(['two_factor_enabled' => false], json_decode($response->getContent(), true)); + + $row = $this->row($user); + $this->assertSame(0, (int)$row->two_factor_enabled); + $this->assertNull($row->two_factor_enforced_at); + $this->assertSame(0, DB::table('user_recovery_codes')->where('user_id', $user->getId())->count()); + $devices = DB::table('user_trusted_devices')->where('user_id', $user->getId())->get(); + $this->assertNotEmpty($devices); + foreach ($devices as $device) { + $this->assertSame(1, (int)$device->is_revoked); + } + + $audit = DB::table('two_factor_audit_log') + ->where('user_id', $user->getId()) + ->where('event_type', TwoFactorAuditLog::EventEnrollmentChanged) + ->orderByDesc('id')->first(); + $metadata = json_decode($audit->metadata, true); + $this->assertFalse($metadata['enabled']); + $this->assertSame($user->getId(), $metadata['actor_id']); + $this->assertSame('self', $metadata['reason']); + } + + public function testDisableExpiresTheDeviceTrustCookie(): void + { + $this->enrolledUser(); + + $this->disable(self::SEED_PASSWORD); + + $this->assertResponseStatus(200); + $name = config('two_factor.cookie_name', 'device_trust_token'); + $queued = array_filter(Cookie::getQueuedCookies(), fn($c) => $c->getName() === $name); + $this->assertCount(1, $queued); + $this->assertLessThan(time(), array_values($queued)[0]->getExpiresTime()); + } + + public function testDisableWithWrongPasswordChangesNothing(): void + { + $user = $this->enrolledUser(); + + $this->disable('not-the-password'); + + $this->assertResponseStatus(412); + $this->assertStillEnrolled($user); + } + + public function testDisableWithoutPasswordIsRejectedForUserWithPassword(): void + { + $user = $this->enrolledUser(); + + $this->disable(null); + + $this->assertResponseStatus(412); + $this->assertStillEnrolled($user); + } + + public function testDisableUnauthenticatedReturns403(): void + { + $this->enrolledUser(); + Auth::logout(); + + $this->disable(self::SEED_PASSWORD); + + $this->assertResponseStatus(403); + } + + public function testDisableWhenTwoFactorIsNotEnabledReturns412(): void + { + $user = $this->enrolledUser(); + $this->disable(self::SEED_PASSWORD); + $this->assertResponseStatus(200); + + $this->disable(self::SEED_PASSWORD); + + $this->assertResponseStatus(412); + } + + public function testDisableForGroupEnforcedUserReturns412AndChangesNothing(): void + { + $admin = EntityManager::getRepository(User::class)->findOneBy(['identifier' => 'sebastian.marcet']); + $this->assertTrue($admin->isTwoFactorEnforcedByGroup(), 'seeded admin must be in an enforced group'); + $this->app->make(IRecoveryCodeService::class)->generateRecoveryCodes($admin); + $before = DB::table('user_recovery_codes')->where('user_id', $admin->getId())->count(); + $this->be($admin); + + $this->disable(self::SEED_PASSWORD); + + $this->assertResponseStatus(412); + $this->assertSame($before, DB::table('user_recovery_codes')->where('user_id', $admin->getId())->count()); + EntityManager::clear(); + $this->assertTrue(EntityManager::getRepository(User::class)->find($admin->getId())->shouldRequire2FA()); + } + + public function testDisableForUserWithoutPasswordInsideTheWindow(): void + { + $user = $this->enrolledUser(false); + Session::put(MFAConstants::SESSION_KEY_COMPLETED_AT, ['user_id' => $user->getId(), 'at' => time() - 30]); + + $this->disable(null); + + $this->assertResponseStatus(200); + $this->assertSame(0, (int)$this->row($user)->two_factor_enabled); + } + + public function testDisableForUserWithoutPasswordOutsideTheWindow(): void + { + $user = $this->enrolledUser(false); + $window = (int)config('two_factor.recovery_enrollment_window', 600); + Session::put(MFAConstants::SESSION_KEY_COMPLETED_AT, ['user_id' => $user->getId(), 'at' => time() - $window - 5]); + + $this->disable(null); + + $this->assertResponseStatus(412); + $this->assertStillEnrolled($user); + } + + public function testAdminSerializerExposesTheTwoFactorFieldsAndPrivateAndPublicDoNot(): void + { + $user = $this->enrolledUser(); + $keys = ['two_factor_enabled', 'two_factor_method', 'two_factor_enforced', 'two_factor_enforced_at']; + $registry = SerializerRegistry::getInstance(); + + $admin = $registry->getSerializer($user, SerializerRegistry::SerializerType_Admin)->serialize(); + foreach ($keys as $key) { + $this->assertArrayHasKey($key, $admin); + } + $this->assertTrue($admin['two_factor_enabled']); + $this->assertSame(User::MFAMethod_OTP, $admin['two_factor_method']); + $this->assertFalse($admin['two_factor_enforced']); + $this->assertNotNull($admin['two_factor_enforced_at']); + // the admin serializer is the private one plus the 2FA state + $this->assertSame([], array_diff( + array_keys($registry->getSerializer($user, SerializerRegistry::SerializerType_Private)->serialize()), + array_keys($admin) + )); + + // OAuth2 clients get the private serializer, public is public: neither has the 2FA state + foreach ([SerializerRegistry::SerializerType_Private, SerializerRegistry::SerializerType_Public] as $type) { + $payload = $registry->getSerializer($user, $type)->serialize(); + foreach ($keys as $key) { + $this->assertArrayNotHasKey($key, $payload, "$type must not expose $key"); + } + } + } +} diff --git a/tests/TwoFactorProfileRoutesCsrfTest.php b/tests/TwoFactorProfileRoutesCsrfTest.php index a11cd04a9..984365e01 100644 --- a/tests/TwoFactorProfileRoutesCsrfTest.php +++ b/tests/TwoFactorProfileRoutesCsrfTest.php @@ -43,6 +43,7 @@ public static function stateChangingRoutes(): array { return [ 'enable 2fa' => ['/admin/api/v1/users/me/2fa/enable'], + 'disable 2fa' => ['/admin/api/v1/users/me/2fa/disable'], 'regenerate recovery codes' => ['/admin/api/v1/users/me/recovery-codes/regenerate'], 'enroll recovery codes' => ['/admin/api/v1/users/me/recovery-codes/enroll'], 'acknowledge recovery codes' => ['/admin/api/v1/users/me/recovery-codes/acknowledge'], From 7e0116d7bb034b14cbfdecbac080ea621a64dfc0 Mon Sep 17 00:00:00 2001 From: romanetar Date: Fri, 6 Nov 2026 16:06:58 +0100 Subject: [PATCH 02/14] feat(2fa): pass the stored 2FA flag and the group enforcement separately to the profile - getProfile passes two_factor_enabled (stored flag) and two_factor_enforced (group) - profile.blade.php exposes twoFactorEnforced and DISABLE_TWO_FACTOR_ENDPOINT - add the disableTwoFactor action; TwoFactorSection treats an enforced user as enabled --- app/Http/Controllers/UserController.php | 5 +++- resources/js/components/two_factor_section.js | 3 ++- resources/js/profile/actions.js | 7 ++++++ resources/js/profile/profile.js | 2 ++ resources/views/profile.blade.php | 2 ++ .../js/components/two_factor_section.test.js | 9 +++++++ tests/js/profile/actions.test.js | 25 +++++++++++++++++++ 7 files changed, 51 insertions(+), 2 deletions(-) diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index be3ae6ab5..969a4cb67 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -1214,7 +1214,10 @@ public function getProfile() 'actions' => $actions, 'countries' => CountryList::getCountries(), 'languages' => $lang2Code, - 'two_factor_enabled' => $user->shouldRequire2FA(), + // the stored flag, not the effective requirement: the profile needs to tell a + // self-enrolled user (can turn it off) from one enforced by group (cannot) + 'two_factor_enabled' => $user->isTwoFactorEnabled(), + 'two_factor_enforced' => $user->isTwoFactorEnforcedByGroup(), // The session completed a 2FA challenge within the enrollment window: // a batch can be generated without the password. 'recovery_codes_window_open' => $window_open, diff --git a/resources/js/components/two_factor_section.js b/resources/js/components/two_factor_section.js index ce632a1bc..648bad627 100644 --- a/resources/js/components/two_factor_section.js +++ b/resources/js/components/two_factor_section.js @@ -9,6 +9,7 @@ const DEFAULT_METHOD = "email_otp"; const TwoFactorSection = ({ twoFactorEnabled, + twoFactorEnforced = false, recoveryCodesRemaining, recoveryCodesTotal, recoveryCodesLowThreshold, @@ -20,7 +21,7 @@ const TwoFactorSection = ({ email, appName }) => { - const [enabled, setEnabled] = useState(twoFactorEnabled); + const [enabled, setEnabled] = useState(twoFactorEnabled || twoFactorEnforced); const [loading, setLoading] = useState(false); // Standing and first batch returned by "Enable": the panel is mounted with them and // asks the user to confirm the batch before it counts as usable. diff --git a/resources/js/profile/actions.js b/resources/js/profile/actions.js index e853e7d8d..ccf458373 100644 --- a/resources/js/profile/actions.js +++ b/resources/js/profile/actions.js @@ -113,6 +113,13 @@ export const enableTwoFactor = async (method) => { return postRawRequestFull(window.ENABLE_TWO_FACTOR_ENDPOINT)(params, {'X-CSRF-TOKEN': window.CSFR_TOKEN}); } +// currentPassword is optional: a user without a password (social-only account) disables +// inside the recovery enrollment window instead of confirming one. +export const disableTwoFactor = async (currentPassword = null) => { + const params = currentPassword ? {current_password: currentPassword} : {}; + return postRawRequestFull(window.DISABLE_TWO_FACTOR_ENDPOINT)(params, jsonHeaders()); +} + const normalizeEntity = (entity) => { entity.public_profile_show_photo = entity.public_profile_show_photo ? 1 : 0; entity.public_profile_show_fullname = entity.public_profile_show_fullname ? 1 : 0; diff --git a/resources/js/profile/profile.js b/resources/js/profile/profile.js index 600bfbe43..4a46a4757 100644 --- a/resources/js/profile/profile.js +++ b/resources/js/profile/profile.js @@ -47,6 +47,7 @@ const ProfilePage = ({ passwordPolicy, redirectUri, twoFactorEnabled, + twoFactorEnforced, recoveryCodesRemaining, recoveryCodesTotal, recoveryCodesLowThreshold, @@ -792,6 +793,7 @@ const ProfilePage = ({ { expect(screen.getByTestId('recovery-codes-count')).toHaveTextContent('Recovery Codes: 7 of 10 remaining'); }); + it('treats a group-enforced user as enabled even when the stored flag is off', () => { + render( + + ); + expect(screen.queryByTestId('enable-two-factor-button')).not.toBeInTheDocument(); + expect(screen.getByTestId('recovery-codes-count')).toBeInTheDocument(); + }); + it('shows "not set up yet" instead of a count when the user requires enrollment', () => { render( ({ import request from "superagent"; import { enableTwoFactor, + disableTwoFactor, regenerateRecoveryCodes, enrollRecoveryCodes, acknowledgeRecoveryCodes, @@ -31,6 +32,7 @@ describe("profile/actions (unmocked request layer)", () => { beforeEach(() => { request.post.mockReset(); window.ENABLE_TWO_FACTOR_ENDPOINT = "https://idp.test/api/v2/users/me/2fa/enable"; + window.DISABLE_TWO_FACTOR_ENDPOINT = "https://idp.test/admin/api/v1/users/me/2fa/disable"; window.REGENERATE_RECOVERY_CODES_ENDPOINT = "https://idp.test/api/v2/users/me/2fa/recovery-codes"; window.ENROLL_RECOVERY_CODES_ENDPOINT = "https://idp.test/admin/api/v1/users/me/recovery-codes/enroll"; window.ACKNOWLEDGE_RECOVERY_CODES_ENDPOINT = "https://idp.test/admin/api/v1/users/me/recovery-codes/acknowledge"; @@ -49,6 +51,29 @@ describe("profile/actions (unmocked request layer)", () => { expect(response.recovery_codes).toEqual(["ABCD-1234"]); }); + it("disableTwoFactor sends current_password only in the request body, asking for JSON", async () => { + const req = makeChainableRequest({ body: { two_factor_enabled: false } }); + request.post.mockReturnValue(req); + + const { response } = await disableTwoFactor("super-secret-password"); + + const calledUrl = request.post.mock.calls[0][0]; + expect(calledUrl).toBe(window.DISABLE_TWO_FACTOR_ENDPOINT); + expect(calledUrl).not.toContain("super-secret-password"); + expect(req.send).toHaveBeenCalledWith({ current_password: "super-secret-password" }); + expect(req.set).toHaveBeenCalledWith({ "X-CSRF-TOKEN": "test-csrf-token", Accept: "application/json" }); + expect(response.two_factor_enabled).toBe(false); + }); + + it("disableTwoFactor without a password sends an empty body (passwordless disable inside the window)", async () => { + const req = makeChainableRequest({ body: { two_factor_enabled: false } }); + request.post.mockReturnValue(req); + + await disableTwoFactor(); + + expect(req.send).toHaveBeenCalledWith({}); + }); + it("regenerateRecoveryCodes sends current_password only in the request body, never in the URL", async () => { const req = makeChainableRequest({ body: { recovery_codes: ["WXYZ-5678"] } }); request.post.mockReturnValue(req); From 2c4294293c9680a371e0dc08d2da871f78508432 Mon Sep 17 00:00:00 2001 From: romanetar Date: Fri, 6 Nov 2026 16:08:44 +0100 Subject: [PATCH 03/14] feat(2fa): replace the enable button with a 2FA switch and a disable confirmation dialog - Switch labelled "Two-Factor Authentication (Email OTP)"; on -> off asks for the current password (none for a user without one) and calls the disable endpoint - group-enforced users see the switch checked and disabled with an explanation - Jest coverage for the switch, the dialog and the enforced state --- resources/js/components/two_factor_section.js | 138 ++++++++++++++++-- .../js/components/two_factor_section.test.js | 113 +++++++++++++- 2 files changed, 234 insertions(+), 17 deletions(-) diff --git a/resources/js/components/two_factor_section.js b/resources/js/components/two_factor_section.js index 648bad627..e87ebde2f 100644 --- a/resources/js/components/two_factor_section.js +++ b/resources/js/components/two_factor_section.js @@ -1,12 +1,23 @@ import React, {useState} from "react"; +import Box from "@material-ui/core/Box"; import Button from "@material-ui/core/Button"; +import Dialog from "@material-ui/core/Dialog"; +import DialogActions from "@material-ui/core/DialogActions"; +import DialogContent from "@material-ui/core/DialogContent"; +import DialogTitle from "@material-ui/core/DialogTitle"; +import FormControlLabel from "@material-ui/core/FormControlLabel"; +import Switch from "@material-ui/core/Switch"; +import TextField from "@material-ui/core/TextField"; import Typography from "@material-ui/core/Typography"; -import {enableTwoFactor} from "../profile/actions"; +import {enableTwoFactor, disableTwoFactor} from "../profile/actions"; import {handleErrorResponse} from "../utils"; import RecoveryCodesPanel from "./recovery_codes_panel"; const DEFAULT_METHOD = "email_otp"; +export const ENFORCED_MESSAGE = + "Two-factor authentication is required for members of an administrator group and cannot be turned off."; + const TwoFactorSection = ({ twoFactorEnabled, twoFactorEnforced = false, @@ -27,6 +38,9 @@ const TwoFactorSection = ({ // asks the user to confirm the batch before it counts as usable. const [enrollment, setEnrollment] = useState(null); + const [confirmingDisable, setConfirmingDisable] = useState(false); + const [currentPassword, setCurrentPassword] = useState(""); + const handleEnable = () => { setLoading(true); enableTwoFactor(DEFAULT_METHOD).then(({response}) => { @@ -39,27 +53,126 @@ const TwoFactorSection = ({ }); }; - if (!enabled) { - return ( - <> - - Two-factor authentication is not enabled for your account. + const closeDisableDialog = () => { + setConfirmingDisable(false); + setCurrentPassword(""); + }; + + // A user without a password sends none: the server accepts the request only inside + // the recovery enrollment window. + const handleDisable = () => { + setLoading(true); + disableTwoFactor(hasPassword ? currentPassword : null).then(() => { + setLoading(false); + closeDisableDialog(); + setEnrollment(null); + setEnabled(false); + }).catch((err) => { + setLoading(false); + // the dialog stays open so a mistyped password can be retried + handleErrorResponse(err); + }); + }; + + const handleToggle = (e) => { + if (twoFactorEnforced || loading) return; + if (e.target.checked) { + handleEnable(); + } else { + setConfirmingDisable(true); + } + }; + + const toggle = ( + + + } + label="Two-Factor Authentication (Email OTP)" + /> + { + twoFactorEnforced && + + {ENFORCED_MESSAGE} + + } + + ); + + // Rendered in a portal and driven by onClick only: the profile page wraps everything + // in one
, so this dialog must never submit it. + const disableDialog = ( + + Turn off two-factor authentication? + + + Your recovery codes will be deleted and every trusted device will be removed. + {hasPassword ? " Enter your current password to confirm." : ""} + { + hasPassword && + setCurrentPassword(e.target.value)} + onKeyDown={(e) => { + if (e.key === "Enter") { + e.preventDefault(); + if (currentPassword && !loading) handleDisable(); + } + }} + inputProps={{form: "disable-two-factor-detached-form", autoComplete: "off"}} + data-testid="disable-two-factor-current-password" + /> + } + + + + + + ); + + if (!enabled) { + return ( + <> + {toggle} + + Two-factor authentication is not enabled for your account. + ); } if (enrollment) { return ( + <> + {toggle} + {disableDialog} + ); } return ( + <> + {toggle} + {disableDialog} + ); }; diff --git a/tests/js/components/two_factor_section.test.js b/tests/js/components/two_factor_section.test.js index c9e74728a..018171338 100644 --- a/tests/js/components/two_factor_section.test.js +++ b/tests/js/components/two_factor_section.test.js @@ -1,10 +1,12 @@ import React from 'react'; -import {render, screen, fireEvent} from '@testing-library/react'; +import {render, screen, fireEvent, waitFor} from '@testing-library/react'; import TwoFactorSection from '../../../resources/js/components/two_factor_section'; -import {enableTwoFactor} from '../../../resources/js/profile/actions'; +import {enableTwoFactor, disableTwoFactor} from '../../../resources/js/profile/actions'; +import {handleErrorResponse} from '../../../resources/js/utils'; jest.mock('../../../resources/js/profile/actions'); jest.mock('sweetalert2', () => jest.fn()); +jest.mock('../../../resources/js/utils', () => ({handleErrorResponse: jest.fn()})); describe('TwoFactorSection', () => { beforeEach(() => { @@ -12,12 +14,13 @@ describe('TwoFactorSection', () => { jest.clearAllMocks(); }); - it('shows the enable button when 2FA is not enabled', () => { + it('shows the switch off when 2FA is not enabled', () => { render( ); - expect(screen.getByTestId('enable-two-factor-button')).toBeInTheDocument(); + expect(screen.getByTestId('two-factor-switch')).not.toBeChecked(); + expect(screen.getByTestId('two-factor-switch')).not.toBeDisabled(); expect(screen.queryByTestId('recovery-codes-count')).not.toBeInTheDocument(); }); @@ -26,7 +29,7 @@ describe('TwoFactorSection', () => { ); - expect(screen.queryByTestId('enable-two-factor-button')).not.toBeInTheDocument(); + expect(screen.getByTestId('two-factor-switch')).toBeChecked(); expect(screen.getByTestId('recovery-codes-count')).toHaveTextContent('Recovery Codes: 7 of 10 remaining'); }); @@ -35,7 +38,8 @@ describe('TwoFactorSection', () => { ); - expect(screen.queryByTestId('enable-two-factor-button')).not.toBeInTheDocument(); + expect(screen.getByTestId('two-factor-switch')).toBeChecked(); + expect(screen.getByTestId('two-factor-switch')).toBeDisabled(); expect(screen.getByTestId('recovery-codes-count')).toBeInTheDocument(); }); @@ -68,11 +72,106 @@ describe('TwoFactorSection', () => { email="user@test.com"/> ); - fireEvent.click(screen.getByTestId('enable-two-factor-button')); + fireEvent.click(screen.getByTestId('two-factor-switch')); expect(enableTwoFactor).toHaveBeenCalledWith('email_otp'); expect(await screen.findByText('AAAA-1111')).toBeInTheDocument(); // Not acknowledged yet: no count behind the modal. expect(screen.queryByTestId('recovery-codes-count')).not.toBeInTheDocument(); }); + + it('renders the switch checked and disabled with the explanation for a group-enforced user', () => { + render( + + ); + const toggle = screen.getByTestId('two-factor-switch'); + expect(toggle).toBeChecked(); + expect(toggle).toBeDisabled(); + expect(screen.getByTestId('two-factor-enforced-message')).toHaveTextContent( + 'Two-factor authentication is required for members of an administrator group and cannot be turned off.' + ); + fireEvent.click(toggle); + expect(screen.queryByTestId('disable-two-factor-dialog')).not.toBeInTheDocument(); + }); + + it('does not show the enforced explanation to a self-enrolled user', () => { + render( + + ); + expect(screen.queryByTestId('two-factor-enforced-message')).not.toBeInTheDocument(); + expect(screen.getByTestId('two-factor-switch')).not.toBeDisabled(); + }); + + it('turning the switch off asks for the password, disables 2FA and hides the recovery codes', async () => { + disableTwoFactor.mockResolvedValue({response: {two_factor_enabled: false}}); + render( + + ); + + fireEvent.click(screen.getByTestId('two-factor-switch')); + + expect(await screen.findByTestId('disable-two-factor-dialog')).toBeInTheDocument(); + expect(disableTwoFactor).not.toHaveBeenCalled(); + expect(screen.getByTestId('confirm-disable-two-factor-button')).toBeDisabled(); + + fireEvent.change(screen.getByTestId('disable-two-factor-current-password').querySelector('input'), + {target: {value: 'secret'}}); + fireEvent.click(screen.getByTestId('confirm-disable-two-factor-button')); + + await waitFor(() => expect(disableTwoFactor).toHaveBeenCalledWith('secret')); + await waitFor(() => expect(screen.queryByTestId('recovery-codes-count')).not.toBeInTheDocument()); + expect(screen.getByTestId('two-factor-switch')).not.toBeChecked(); + expect(screen.getByTestId('two-factor-disabled-message')).toBeInTheDocument(); + }); + + it('keeps the dialog open and 2FA on when the password is rejected', async () => { + disableTwoFactor.mockRejectedValue({status: 412}); + render( + + ); + + fireEvent.click(screen.getByTestId('two-factor-switch')); + fireEvent.change((await screen.findByTestId('disable-two-factor-current-password')).querySelector('input'), + {target: {value: 'wrong'}}); + fireEvent.click(screen.getByTestId('confirm-disable-two-factor-button')); + + await waitFor(() => expect(disableTwoFactor).toHaveBeenCalledWith('wrong')); + expect(handleErrorResponse).toHaveBeenCalled(); + expect(screen.getByTestId('disable-two-factor-dialog')).toBeInTheDocument(); + expect(screen.getByTestId('two-factor-switch')).toBeChecked(); + expect(screen.getByTestId('recovery-codes-count')).toBeInTheDocument(); + }); + + it('cancelling the dialog leaves 2FA on and sends nothing', async () => { + render( + + ); + + fireEvent.click(screen.getByTestId('two-factor-switch')); + fireEvent.click(await screen.findByTestId('cancel-disable-two-factor-button')); + + expect(disableTwoFactor).not.toHaveBeenCalled(); + expect(screen.getByTestId('two-factor-switch')).toBeChecked(); + }); + + it('a user without a password confirms without typing one', async () => { + disableTwoFactor.mockResolvedValue({response: {two_factor_enabled: false}}); + render( + + ); + + fireEvent.click(screen.getByTestId('two-factor-switch')); + + expect(await screen.findByTestId('disable-two-factor-dialog')).toBeInTheDocument(); + expect(screen.queryByTestId('disable-two-factor-current-password')).not.toBeInTheDocument(); + fireEvent.click(screen.getByTestId('confirm-disable-two-factor-button')); + + await waitFor(() => expect(disableTwoFactor).toHaveBeenCalledWith(null)); + }); }); From 52eddaaaf9d92343ef9aa22b6fa291a88632ba04 Mon Sep 17 00:00:00 2001 From: romanetar Date: Fri, 6 Nov 2026 17:06:14 +0100 Subject: [PATCH 04/14] feat(2fa): let a server admin enable or disable 2FA from the user CRUD - PUT /users/{id} accepts two_factor_enabled; UserService::update applies it only when a server admin edits another user and the value differs from the stored one - enabling goes through IRecoveryCodeService::enableTwoFactorForUser: no recovery codes, enrollment_changed audit with actor_id, notification email to the user - disabling reuses disableTwoFactor; a group-enforced user returns 412 and the whole update is rolled back - PUT /users/me and an admin editing their own account ignore the key --- .../Factories/UserValidationRulesFactory.php | 2 + app/Mail/TwoFactorEnabledByAdminMail.php | 80 ++++++++ app/Services/Auth/IRecoveryCodeService.php | 15 ++ app/Services/Auth/RecoveryCodeService.php | 37 ++++ app/Services/OpenId/UserService.php | 43 +++- .../two_factor_enabled_by_admin.blade.php | 28 +++ tests/AdminUserTwoFactorUpdateTest.php | 192 ++++++++++++++++++ 7 files changed, 395 insertions(+), 2 deletions(-) create mode 100644 app/Mail/TwoFactorEnabledByAdminMail.php create mode 100644 resources/views/emails/auth/two_factor_enabled_by_admin.blade.php create mode 100644 tests/AdminUserTwoFactorUpdateTest.php diff --git a/app/Http/Controllers/Factories/UserValidationRulesFactory.php b/app/Http/Controllers/Factories/UserValidationRulesFactory.php index 9e34ca7a3..663cb5cdd 100644 --- a/app/Http/Controllers/Factories/UserValidationRulesFactory.php +++ b/app/Http/Controllers/Factories/UserValidationRulesFactory.php @@ -58,6 +58,8 @@ public static function build(array $data, $update = false, ?User $currentUser = // admin fields 'email_verified' => 'nullable|boolean', 'active' => 'nullable|boolean', + // honoured only for a server admin editing ANOTHER user (UserService::update) + 'two_factor_enabled' => 'nullable|boolean', 'groups' => 'sometimes|int_array', 'public_profile_show_photo' => 'sometimes|boolean', 'public_profile_show_fullname' => 'sometimes|boolean', diff --git a/app/Mail/TwoFactorEnabledByAdminMail.php b/app/Mail/TwoFactorEnabledByAdminMail.php new file mode 100644 index 000000000..444640f71 --- /dev/null +++ b/app/Mail/TwoFactorEnabledByAdminMail.php @@ -0,0 +1,80 @@ +user_email = $user->getEmail(); + $this->user_fullname = $user->getFullName(); + $this->profile_url = URL::action('UserController@getProfile'); + } + + /** + * @return $this + */ + public function build() + { + $this->subject = sprintf('[%s] Two-factor authentication enabled', Config::get('app.app_name')); + Log::debug(sprintf("TwoFactorEnabledByAdminMail::build to %s", $this->user_email)); + return $this->from(Config::get("mail.from")) + ->to($this->user_email) + ->subject($this->subject) + ->view('emails.auth.two_factor_enabled_by_admin'); + } +} diff --git a/app/Services/Auth/IRecoveryCodeService.php b/app/Services/Auth/IRecoveryCodeService.php index 53bcaaf11..929ce603c 100644 --- a/app/Services/Auth/IRecoveryCodeService.php +++ b/app/Services/Auth/IRecoveryCodeService.php @@ -135,4 +135,19 @@ public function getStatus(User $user): RecoveryCodesStatus; * and the session is outside the enrollment window */ public function disableTwoFactor(User $user, ?string $currentPassword, ?User $actor): void; + + /** + * Enables 2FA for a user on behalf of an administrator (support path). Unlike + * enableTwoFactorAndGenerateCodes() no recovery codes are generated: codes are shown + * once and only to their owner, who is told by email to generate them from their + * profile. Audited as enrollment_changed with {enabled: true, actor_id, reason: admin}. + * The email and the audit row are best-effort once 2FA is enabled. + * + * @param User $user + * @param string $method one of User::ValidMFAMethods + * @param User $actor the administrator performing the change + * @return void + * @throws ValidationException if $method is not valid or not available + */ + public function enableTwoFactorForUser(User $user, string $method, User $actor): void; } diff --git a/app/Services/Auth/RecoveryCodeService.php b/app/Services/Auth/RecoveryCodeService.php index e9d9168b4..badd8cb96 100644 --- a/app/Services/Auth/RecoveryCodeService.php +++ b/app/Services/Auth/RecoveryCodeService.php @@ -14,6 +14,7 @@ **/ use App\libs\Auth\Models\TwoFactorAuditLog; +use App\Mail\TwoFactorEnabledByAdminMail; use App\libs\Auth\Models\UserRecoveryCode; use Auth\MFAConstants; use Auth\Repositories\IUserRecoveryCodeRepository; @@ -22,6 +23,7 @@ use Auth\User; use Illuminate\Support\Facades\Hash; use Illuminate\Support\Facades\Log; +use Illuminate\Support\Facades\Mail; use Illuminate\Support\Facades\Session; use Laminas\Math\Rand; use models\exceptions\ValidationException; @@ -294,6 +296,41 @@ public function disableTwoFactor(User $user, ?string $currentPassword, ?User $ac } } + /** + * @inheritDoc + */ + public function enableTwoFactorForUser(User $user, string $method, User $actor): void + { + $this->tx_service->transaction(function () use ($user, $method) { + // the lock re-hydrates $user, so it must precede enable2FA() + $this->repository->lockUserRow($user); + $user->enable2FA($method); + $this->user_repository->add($user, false); + }); + + try { + $this->audit_service->log( + $user, + TwoFactorAuditLog::EventEnrollmentChanged, + $method, + IPHelper::getUserIp(), + [ + 'enabled' => true, + 'actor_id' => $actor->getId(), + 'reason' => 'admin', + ] + ); + } catch (\Throwable $ex) { + Log::warning($ex); + } + + try { + Mail::queue(new TwoFactorEnabledByAdminMail($user)); + } catch (\Throwable $ex) { + Log::warning($ex); + } + } + /** * Invalidates every existing recovery code for the user and generates a * fresh batch, within the caller's already-open transaction. The caller diff --git a/app/Services/OpenId/UserService.php b/app/Services/OpenId/UserService.php index cb5f56545..ecaedfa90 100644 --- a/app/Services/OpenId/UserService.php +++ b/app/Services/OpenId/UserService.php @@ -23,6 +23,7 @@ use App\libs\Utils\FileSystem\FileNameSanitizer; use App\Mail\MonitoredSecurityGroupNotificationEmail; use App\Services\AbstractService; +use App\Services\Auth\IRecoveryCodeService; use App\Services\Auth\IUserIdentifierGeneratorService; use Auth\Group; use Auth\IUserNameGeneratorService; @@ -95,6 +96,11 @@ final class UserService extends AbstractService implements IUserService */ private $identifier_service; + /** + * @var IRecoveryCodeService + */ + private $recovery_code_service; + /** * UserService constructor. * @param IUserRepository $repository @@ -105,6 +111,8 @@ final class UserService extends AbstractService implements IUserService * @param ILogService $log_service * @param IResourceServerContext $server_ctx * @param IUserIdentifierGeneratorService $identifier_service + * @param IClientRepository $client_repository + * @param IRecoveryCodeService $recovery_code_service */ public function __construct ( @@ -116,10 +124,12 @@ public function __construct ILogService $log_service, IResourceServerContext $server_ctx, IUserIdentifierGeneratorService $identifier_service, - IClientRepository $client_repository + IClientRepository $client_repository, + IRecoveryCodeService $recovery_code_service ) { parent::__construct($tx_service); + $this->recovery_code_service = $recovery_code_service; $this->repository = $repository; $this->group_repository = $group_repository; $this->user_name_generator = $user_name_generator; @@ -295,8 +305,10 @@ public function create(array $payload): IEntity public function update(int $id, array $payload): IEntity { $password_changed = false; + $two_factor_change = null; + $acting_admin = null; - $user = $this->tx_service->transaction(function () use ($id, $payload, &$password_changed) { + $user = $this->tx_service->transaction(function () use ($id, $payload, &$password_changed, &$two_factor_change, &$acting_admin) { $user = $this->repository->getById($id); @@ -336,6 +348,25 @@ public function update(int $id, array $payload): IEntity throw new ValidationException(sprintf("identifier %s already belongs to another user", $payload["identifier"])); } + // 2FA is managed here and not in UserFactory::populate: it needs the acting user. + // Only a server admin editing ANOTHER user may change it, and only when the value + // differs from the stored one (the admin form submits the whole entity on every + // save). Anyone else, including PUT /users/me, has the key ignored: turning 2FA + // off needs the password and goes through POST /users/me/2fa/disable. + if (isset($payload['two_factor_enabled']) + && !is_null($current_user) + && $current_user->getId() != $user->getId() + && ($current_user->isAdmin() || $current_user->isSuperAdmin())) { + $requested = boolval($payload['two_factor_enabled']); + if ($requested !== $user->isTwoFactorEnabled()) { + // checked before anything is saved: the whole update is rolled back + if (!$requested && $user->isTwoFactorEnforcedByGroup()) + throw new ValidationException("Two-factor authentication is enforced by group membership and cannot be disabled."); + $two_factor_change = $requested; + $acting_admin = $current_user; + } + } + $user = UserFactory::populate($user, $payload); if (isset($payload['groups'])) { @@ -387,6 +418,14 @@ public function update(int $id, array $payload): IEntity Event::dispatch(new UserPasswordResetSuccessful($user->getId())); } + // after the update transaction: the recovery code service runs its own + if (!is_null($two_factor_change)) { + if ($two_factor_change) + $this->recovery_code_service->enableTwoFactorForUser($user, User::MFAMethod_OTP, $acting_admin); + else + $this->recovery_code_service->disableTwoFactor($user, null, $acting_admin); + } + try { if (Config::get("queue.enable_message_broker", false) == true) PublishUserUpdated::dispatch($user)->onConnection('message_broker'); diff --git a/resources/views/emails/auth/two_factor_enabled_by_admin.blade.php b/resources/views/emails/auth/two_factor_enabled_by_admin.blade.php new file mode 100644 index 000000000..fce10b102 --- /dev/null +++ b/resources/views/emails/auth/two_factor_enabled_by_admin.blade.php @@ -0,0 +1,28 @@ +@extends('emails.email_layout') + +@section('content') + + + + + + + + + + + + + + + +
+
Dear {{ $user_fullname }},
+
+
Two-factor authentication was enabled on your {{ Config::get('app.app_name') }} account by an administrator. From now on you will be asked for a verification code sent to {{ $user_email }} when you sign in.
+
+
Please generate your recovery codes from your profile page and keep them in a safe place: they let you sign in if you cannot receive the verification code.
+
+
If you did not expect this change please email {{ Config::get('app.help_email') }}.
+
+@stop diff --git a/tests/AdminUserTwoFactorUpdateTest.php b/tests/AdminUserTwoFactorUpdateTest.php new file mode 100644 index 000000000..74fd0e8b6 --- /dev/null +++ b/tests/AdminUserTwoFactorUpdateTest.php @@ -0,0 +1,192 @@ +withoutMiddleware(); + $this->admin = EntityManager::getRepository(User::class)->findOneBy(['identifier' => 'sebastian.marcet']); + $this->be($this->admin); + Session::start(); + } + + private function plainUser(bool $enrolled): User + { + $user = UserFactory::build([ + 'first_name' => 'Plain', + 'last_name' => 'Target', + 'email' => 'target.' . uniqid() . '@test.invalid', + 'active' => true, + 'email_verified' => true, + 'identifier' => 'target.' . uniqid(), + 'password' => '1Qaz2wsx!', + 'password_enc' => AuthHelper::AlgSHA1_V2_4, + ]); + EntityManager::persist($user); + EntityManager::flush(); + if ($enrolled) { + $this->app->make(IRecoveryCodeService::class)->enableTwoFactorAndGenerateCodes($user, User::MFAMethod_OTP); + $this->app->make(IDeviceTrustService::class)->trustDevice($user, 'agent-a', '127.0.0.1'); + } + return $user; + } + + private function updateUser(User $target, array $payload) + { + return $this->action('PUT', 'Api\\UserApiController@update', ['id' => $target->getId()], $payload, [], []); + } + + private function row(User $user): object + { + return DB::table('users')->where('id', $user->getId())->first(); + } + + private function lastAudit(User $user): ?array + { + $row = DB::table('two_factor_audit_log')->where('user_id', $user->getId()) + ->where('event_type', 'enrollment_changed')->orderByDesc('id')->first(); + return $row ? json_decode($row->metadata, true) : null; + } + + public function testAdminEnablesTwoFactorWithoutRecoveryCodesAndNotifiesTheUser(): void + { + $user = $this->plainUser(false); + + $this->updateUser($user, ['two_factor_enabled' => 1]); + + $this->assertResponseStatus(201); + $row = $this->row($user); + $this->assertSame(1, (int)$row->two_factor_enabled); + $this->assertNotNull($row->two_factor_enforced_at); + $this->assertSame(0, DB::table('user_recovery_codes')->where('user_id', $user->getId())->count()); + $audit = $this->lastAudit($user); + $this->assertTrue($audit['enabled']); + $this->assertSame($this->admin->getId(), $audit['actor_id']); + $this->assertSame('admin', $audit['reason']); + Mail::assertQueued(TwoFactorEnabledByAdminMail::class, 1); + } + + public function testAdminDisablesTwoFactorWithTheSameSideEffectsAsSelfService(): void + { + $user = $this->plainUser(true); + + $this->updateUser($user, ['two_factor_enabled' => 0]); + + $this->assertResponseStatus(201); + $row = $this->row($user); + $this->assertSame(0, (int)$row->two_factor_enabled); + $this->assertNull($row->two_factor_enforced_at); + $this->assertSame(0, DB::table('user_recovery_codes')->where('user_id', $user->getId())->count()); + $this->assertSame(0, DB::table('user_trusted_devices') + ->where('user_id', $user->getId())->where('is_revoked', 0)->count()); + $audit = $this->lastAudit($user); + $this->assertFalse($audit['enabled']); + $this->assertSame($this->admin->getId(), $audit['actor_id']); + $this->assertSame('admin', $audit['reason']); + Mail::assertNotQueued(TwoFactorEnabledByAdminMail::class); + } + + public function testDisablingAGroupEnforcedUserReturns412AndSavesNothing(): void + { + $user = $this->plainUser(true); + $group = $this->app->make(IGroupRepository::class)->getOneBySlug(IGroupSlugs::SponsorUsersGroup); + $user->addToGroup($group); + EntityManager::flush(); + Config::set('two_factor.enforced_groups', [IGroupSlugs::SponsorUsersGroup]); + + $this->updateUser($user, ['two_factor_enabled' => 0, 'first_name' => 'Renamed']); + + $this->assertResponseStatus(412); + EntityManager::clear(); + $this->assertSame('Plain', $this->row($user)->first_name, 'the rest of the update must be rolled back'); + $this->assertSame(1, (int)$this->row($user)->two_factor_enabled); + $this->assertSame(10, DB::table('user_recovery_codes')->where('user_id', $user->getId())->count()); + } + + public function testAPayloadWithoutTheKeyLeavesTwoFactorUntouched(): void + { + $user = $this->plainUser(true); + + $this->updateUser($user, ['first_name' => 'Renamed']); + + $this->assertResponseStatus(201); + $this->assertSame(1, (int)$this->row($user)->two_factor_enabled); + $this->assertSame(10, DB::table('user_recovery_codes')->where('user_id', $user->getId())->count()); + $this->assertSame(0, DB::table('user_trusted_devices') + ->where('user_id', $user->getId())->where('is_revoked', 1)->count()); + Mail::assertNotQueued(TwoFactorEnabledByAdminMail::class); + } + + public function testAnUnchangedValueIsIgnored(): void + { + $user = $this->plainUser(true); + + $this->updateUser($user, ['first_name' => 'Renamed', 'two_factor_enabled' => 1]); + + $this->assertResponseStatus(201); + $this->assertSame(10, DB::table('user_recovery_codes')->where('user_id', $user->getId())->count()); + $this->assertSame(1, DB::table('two_factor_audit_log') + ->where('user_id', $user->getId())->where('event_type', 'enrollment_changed')->count(), + 'only the enrollment of the fixture, no second row'); + Mail::assertNotQueued(TwoFactorEnabledByAdminMail::class); + } + + public function testAUserCannotTurnOffTheirOwnTwoFactorThroughTheProfileUpdate(): void + { + $user = $this->plainUser(true); + $this->be($user); + + $this->action('PUT', 'Api\\UserApiController@updateMe', [], ['two_factor_enabled' => 0], [], []); + + $this->assertSame(1, (int)$this->row($user)->two_factor_enabled); + $this->assertSame(10, DB::table('user_recovery_codes')->where('user_id', $user->getId())->count()); + } + + public function testAnAdminCannotChangeTheirOwnTwoFactorThroughTheUserCrud(): void + { + $admin = $this->plainUser(true); + $group = $this->app->make(IGroupRepository::class)->getOneBySlug(IGroupSlugs::SuperAdminGroup); + $admin->addToGroup($group); + EntityManager::flush(); + $this->be($admin); + + $this->updateUser($admin, ['two_factor_enabled' => 0]); + + $this->assertSame(1, (int)$this->row($admin)->two_factor_enabled); + } +} From 28e7637a960077df4e7af9af49a85fc57a43ad36 Mon Sep 17 00:00:00 2001 From: romanetar Date: Fri, 6 Nov 2026 17:09:44 +0100 Subject: [PATCH 05/14] feat(2fa): add the 2FA checkbox to the admin edit user form - checkbox bound to two_factor_enabled, normalised to 1/0 like active/email_verified - a group-enforced user shows it checked and disabled with "Enforced by group membership" - two_factor_enforced_at shown as read-only text; the read-only 2FA keys are not submitted --- resources/js/admin/edit_user/actions.js | 6 +++ .../components/two_factor_checkbox.js | 39 ++++++++++++++++ resources/js/admin/edit_user/edit_user.js | 6 +++ tests/js/admin/two_factor_checkbox.test.js | 44 +++++++++++++++++++ 4 files changed, 95 insertions(+) create mode 100644 resources/js/admin/edit_user/components/two_factor_checkbox.js create mode 100644 tests/js/admin/two_factor_checkbox.test.js diff --git a/resources/js/admin/edit_user/actions.js b/resources/js/admin/edit_user/actions.js index ca278dcd5..29cf39763 100644 --- a/resources/js/admin/edit_user/actions.js +++ b/resources/js/admin/edit_user/actions.js @@ -91,6 +91,12 @@ const normalizeEntity = (entity) => { entity.public_profile_allow_chat_with_me = entity.public_profile_allow_chat_with_me ? 1 : 0; entity.active = entity.active ? 1 : 0; entity.email_verified = entity.email_verified ? 1 : 0; + // the stored flag, 1/0 like the other checkboxes: the server acts only when it differs + // from the stored value. The rest of the 2FA state is read only. + entity.two_factor_enabled = entity.two_factor_enabled ? 1 : 0; + delete entity.two_factor_enforced; + delete entity.two_factor_enforced_at; + delete entity.two_factor_method; entity.post_code = entity.post_code.toString(); if (entity.birthday) { diff --git a/resources/js/admin/edit_user/components/two_factor_checkbox.js b/resources/js/admin/edit_user/components/two_factor_checkbox.js new file mode 100644 index 000000000..28b211b96 --- /dev/null +++ b/resources/js/admin/edit_user/components/two_factor_checkbox.js @@ -0,0 +1,39 @@ +import React from "react"; +import moment from "moment"; +import Checkbox from "@material-ui/core/Checkbox"; +import FormControlLabel from "@material-ui/core/FormControlLabel"; +import FormHelperText from "@material-ui/core/FormHelperText"; + +export const ENFORCED_BY_GROUP_TEXT = "Enforced by group membership"; + +/** + * 2FA checkbox of the admin edit user form. A user enforced by group membership is + * required to complete 2FA whatever the stored flag says, so the box shows checked and + * disabled: nobody can switch it off. The stored flag is what the form submits. + */ +const TwoFactorCheckbox = ({enabled, enforced = false, enforcedAt = null, onChange}) => ( + <> + { if (!enforced && onChange) onChange(e); }} + inputProps={{"data-testid": "two-factor-checkbox"}} + color="primary"/>} + label="Two-Factor Authentication (Email OTP)" + /> + { + enforced && + {ENFORCED_BY_GROUP_TEXT} + } + { + !!enforcedAt && + + Enforced since {moment.unix(Number(enforcedAt)).utc().format("YYYY-MM-DD HH:mm")} UTC + + } + +); + +export default TwoFactorCheckbox; diff --git a/resources/js/admin/edit_user/edit_user.js b/resources/js/admin/edit_user/edit_user.js index ca6adf34a..2f3db1f06 100644 --- a/resources/js/admin/edit_user/edit_user.js +++ b/resources/js/admin/edit_user/edit_user.js @@ -9,6 +9,7 @@ import CssBaseline from "@material-ui/core/CssBaseline"; import Checkbox from "@material-ui/core/Checkbox"; import Grid from "@material-ui/core/Grid"; import GroupsInput from "./components/groups_input"; +import TwoFactorCheckbox from "./components/two_factor_checkbox"; import MenuItem from "@material-ui/core/MenuItem"; import TextField from "@material-ui/core/TextField"; import Typography from "@material-ui/core/Typography"; @@ -733,6 +734,11 @@ const EditUserPage = ({ color="primary"/>} label="Email Verified?" /> + { + it('reflects the stored flag and can be toggled', () => { + const onChange = jest.fn(); + const {rerender} = render(); + const box = screen.getByTestId('two-factor-checkbox'); + expect(box).not.toBeChecked(); + expect(box).not.toBeDisabled(); + expect(screen.getByText('Two-Factor Authentication (Email OTP)')).toBeInTheDocument(); + + fireEvent.click(box); + expect(onChange).toHaveBeenCalledTimes(1); + + rerender(); + expect(screen.getByTestId('two-factor-checkbox')).toBeChecked(); + expect(screen.queryByTestId('two-factor-enforced-text')).not.toBeInTheDocument(); + }); + + it('is checked and disabled with the explanation for a group-enforced user, even with the stored flag off', () => { + const onChange = jest.fn(); + render(); + + const box = screen.getByTestId('two-factor-checkbox'); + expect(box).toBeChecked(); + expect(box).toBeDisabled(); + expect(screen.getByTestId('two-factor-enforced-text')).toHaveTextContent(ENFORCED_BY_GROUP_TEXT); + expect(ENFORCED_BY_GROUP_TEXT).toBe('Enforced by group membership'); + + fireEvent.click(box); + expect(onChange).not.toHaveBeenCalled(); + }); + + it('shows when 2FA was enforced as read only text, only when set', () => { + const {rerender} = render(); + expect(screen.queryByTestId('two-factor-enforced-at')).not.toBeInTheDocument(); + + // 2026-07-23 10:30:00 UTC + rerender(); + expect(screen.getByTestId('two-factor-enforced-at')).toHaveTextContent('Enforced since 2026-07-23 10:30 UTC'); + }); +}); From 87bb6965e409089757a55cec04c4bc363b8f4942 Mon Sep 17 00:00:00 2001 From: romanetar Date: Fri, 6 Nov 2026 17:11:33 +0100 Subject: [PATCH 06/14] test(2fa): cover the password login after disabling 2FA and for a group-enforced user - after the user disables 2FA the password alone logs in (Auth::check, no 2fa flow) - a group-enforced user cannot disable it and is still challenged --- tests/TwoFactorLoginFlowTest.php | 38 ++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/tests/TwoFactorLoginFlowTest.php b/tests/TwoFactorLoginFlowTest.php index 54294be05..d7db0efc7 100644 --- a/tests/TwoFactorLoginFlowTest.php +++ b/tests/TwoFactorLoginFlowTest.php @@ -191,6 +191,44 @@ public function testNonAdminWithoutMFALogsInNormally(): void $this->assertTrue(Auth::check(), 'a non-MFA user must get an authenticated session'); } + public function testPasswordLoginNoLongerHitsTheChallengeAfterTheUserDisabledTwoFactor(): void + { + $email = $this->createPlainUser(); + $user = $this->user($email); + $recovery = $this->app->make(IRecoveryCodeService::class); + $recovery->enableTwoFactorAndGenerateCodes($user, User::MFAMethod_OTP); + + // control: enrolled, the password alone does not log in + $this->postLogin($email, self::SEED_PASSWORD); + $this->assertFalse(Auth::check()); + $this->assertSame('2fa', Session::get('flow')); + $this->resetLogin(); + + $recovery->disableTwoFactor($user, self::SEED_PASSWORD, $user); + + $this->postLogin($email, self::SEED_PASSWORD); + + $this->assertResponseStatus(302); + $this->assertTrue(Auth::check(), 'with 2FA off the password alone must log the user in'); + $this->assertNotSame('2fa', Session::get('flow'), 'no 2FA screen must be pending'); + } + + public function testGroupEnforcedUserIsStillChallengedAfterARefusedDisable(): void + { + $admin = $this->user(self::ADMIN_EMAIL); + + try { + $this->app->make(IRecoveryCodeService::class)->disableTwoFactor($admin, self::SEED_PASSWORD, $admin); + $this->fail('a group-enforced user must not be able to disable 2FA'); + } catch (\models\exceptions\ValidationException $ex) { + } + + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + + $this->assertFalse(Auth::check(), 'enforcement does not depend on the stored flag'); + $this->assertSame('2fa', Session::get('flow')); + } + // ------------------------------------------------------------------------- // passwordless (flow=otp) login must not bypass the MFA gate // ------------------------------------------------------------------------- From 3b07300fcacce4ad9bfe659b108c80b89c792ec2 Mon Sep 17 00:00:00 2001 From: romanetar Date: Fri, 6 Nov 2026 17:48:16 +0100 Subject: [PATCH 07/14] fix(2fa): show the disable password error inside the confirmation dialog A SweetAlert opens below a MUI Dialog (z-index 1060 vs 1300), so a wrong password produced an error popup hidden behind the dialog. Resolve the message with handleErrorResponse and render it inside the dialog instead. --- resources/js/components/two_factor_section.js | 20 +++++++++++++++++-- .../js/components/two_factor_section.test.js | 9 +++++++-- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/resources/js/components/two_factor_section.js b/resources/js/components/two_factor_section.js index e87ebde2f..a52cf12a3 100644 --- a/resources/js/components/two_factor_section.js +++ b/resources/js/components/two_factor_section.js @@ -40,6 +40,9 @@ const TwoFactorSection = ({ const [confirmingDisable, setConfirmingDisable] = useState(false); const [currentPassword, setCurrentPassword] = useState(""); + // Shown inside the dialog: a SweetAlert opens below a MUI Dialog (z-index 1060 vs 1300), + // so the usual error popup would be hidden behind it. + const [disableError, setDisableError] = useState(null); const handleEnable = () => { setLoading(true); @@ -56,12 +59,14 @@ const TwoFactorSection = ({ const closeDisableDialog = () => { setConfirmingDisable(false); setCurrentPassword(""); + setDisableError(null); }; // A user without a password sends none: the server accepts the request only inside // the recovery enrollment window. const handleDisable = () => { setLoading(true); + setDisableError(null); disableTwoFactor(hasPassword ? currentPassword : null).then(() => { setLoading(false); closeDisableDialog(); @@ -70,7 +75,9 @@ const TwoFactorSection = ({ }).catch((err) => { setLoading(false); // the dialog stays open so a mistyped password can be retried - handleErrorResponse(err); + handleErrorResponse(err, (title, message) => { + setDisableError(Array.isArray(message) ? message.join(" ") : (message || title)); + }); }); }; @@ -128,7 +135,10 @@ const TwoFactorSection = ({ autoFocus label="Current Password" value={currentPassword} - onChange={(e) => setCurrentPassword(e.target.value)} + onChange={(e) => { + setCurrentPassword(e.target.value); + setDisableError(null); + }} onKeyDown={(e) => { if (e.key === "Enter") { e.preventDefault(); @@ -139,6 +149,12 @@ const TwoFactorSection = ({ data-testid="disable-two-factor-current-password" /> } + { + disableError && + + {disableError} + + }