From db65731c3ad28ac7f0d541c60cb65eb100958514 Mon Sep 17 00:00:00 2001 From: ignacionelson Date: Sat, 3 Oct 2026 23:05:23 -0300 Subject: [PATCH] Keep your own credentials behind your profile, and end tokens on a reset Your own email address, password and second factor are changed from your profile, which asks for your current password. The staff screen and the API changed the first two with no password at all, and the API removed the third on your own account without the confirmation the web asks for. StaffAccounts::ownCredentialChanges is the one rule both now ask: the staff screen refuses your own email or password with a validation error and points to the profile, and the API answers 403, as it does for removing your own second factor. Changing somebody else's password is unchanged: that is what edit_users and edit_clients mean, on the screen and over the API. It now also revokes that account's API tokens. Browser sessions already ended with the password hash; tokens did not. GHSA-j5cp-r8pr-m5cr --- .../Http/Controllers/Api/UsersController.php | 18 +++- .../Http/Controllers/UsersController.php | 17 ++++ app/Modules/Identity/StaffAccounts.php | 50 ++++++++++- resources/js/components/user-form.tsx | 83 +++++++++++++------ resources/js/pages/users/edit.tsx | 1 + 5 files changed, 140 insertions(+), 29 deletions(-) diff --git a/app/Modules/Identity/Http/Controllers/Api/UsersController.php b/app/Modules/Identity/Http/Controllers/Api/UsersController.php index 895fb4db..406a3724 100644 --- a/app/Modules/Identity/Http/Controllers/Api/UsersController.php +++ b/app/Modules/Identity/Http/Controllers/Api/UsersController.php @@ -182,6 +182,15 @@ class UsersController extends Controller 'assigned_clients.*' => ['integer', Rule::in($this->accounts->assignableClientIds($actor))], ]); + // Your own email address and password are changed from your + // profile, which asks for your current password. A token cannot be + // asked for one, so here the answer is no. + abort_if( + $this->accounts->ownCredentialChanges($actor, $user, $validated['email'] ?? null, $validated['password'] ?? null) !== [], + 403, + __('Change your own email address and password from your profile.'), + ); + // Read through Request::boolean() rather than off the validated // array, for the reason RolesController::guardScopeRemoval spells // out: the `boolean` rule accepts 0 and "0" as well as false but @@ -274,7 +283,14 @@ class UsersController extends Controller { abort_unless($user->isStaff(), 404); - $this->accounts->guardTarget($this->actor($request), $user); + $actor = $this->actor($request); + $this->accounts->guardTarget($actor, $user); + + // The web asks for your password before this (password.confirm), + // and a token cannot give one. On your own account it would let a + // token clear the second factor standing between it and a browser + // session as you. + abort_if($user->is($actor), 403, __('Remove your own two-factor authentication from your profile.')); $twoFactor->reset($user); diff --git a/app/Modules/Identity/Http/Controllers/UsersController.php b/app/Modules/Identity/Http/Controllers/UsersController.php index 174bf3c8..500fbac4 100644 --- a/app/Modules/Identity/Http/Controllers/UsersController.php +++ b/app/Modules/Identity/Http/Controllers/UsersController.php @@ -226,6 +226,23 @@ class UsersController extends Controller ]); } + // Your own email address and password are changed from your + // profile, which asks for your current password first; this screen + // does not, so it does not change them. + $ownCredentials = $this->accounts->ownCredentialChanges( + $this->actor(), + $user, + $validated['email'], + is_string($validated['password'] ?? null) ? $validated['password'] : null, + ); + + if ($ownCredentials !== []) { + throw ValidationException::withMessages(array_fill_keys( + $ownCredentials, + __('Change your own email address and password from your profile.'), + )); + } + $this->accounts->update($user, [ 'name' => $validated['name'], 'email' => $validated['email'], diff --git a/app/Modules/Identity/StaffAccounts.php b/app/Modules/Identity/StaffAccounts.php index f6191655..fc1239c4 100644 --- a/app/Modules/Identity/StaffAccounts.php +++ b/app/Modules/Identity/StaffAccounts.php @@ -161,6 +161,42 @@ class StaffAccounts abort_unless($role === null || $this->mayGrant($actor, $role), 403); } + /** + * Which of your own credentials this change would replace: a different + * email address, or a new password. Empty when the target is somebody + * else, or when nothing that signs the account in is changing. + * + * Your own are changed from your profile, which asks for your current + * password first (GHSA-f32x-fgmp-q353). The staff screen and the API + * must not be a second door to them. Over the API that door was wider + * still: a token limited to manage_users and edit_users could give its + * own owner a password it chose, and then sign in as the owner with + * every ability the token had been denied. + * + * The email address counts because it is how a password is recovered: + * an address you control is a password you can set. + * + * @return list<'email'|'password'> + */ + public function ownCredentialChanges(User $actor, User $target, ?string $email, ?string $password): array + { + if (! $target->is($actor)) { + return []; + } + + $fields = []; + + if ($email !== null && mb_strtolower($email) !== mb_strtolower($target->email)) { + $fields[] = 'email'; + } + + if ($password !== null && $password !== '') { + $fields[] = 'password'; + } + + return $fields; + } + /** * Refuse any change that would leave the installation without an * active administrator. @@ -297,12 +333,24 @@ class StaffAccounts $user->fill(array_intersect_key($attributes, array_flip(['name', 'email', 'role_id', 'active']))); - if (is_string($attributes['password'] ?? null) && $attributes['password'] !== '') { + $passwordReplaced = is_string($attributes['password'] ?? null) && $attributes['password'] !== ''; + + if ($passwordReplaced) { $user->password = $attributes['password']; } $user->save(); + // Somebody else gave this account a new password: whatever had been + // holding it, a person or a stolen credential, is ended with it. + // Browser sessions end on their own (AuthenticateSession reads the + // password hash), but API tokens do not, and a reset that left the + // previous holder's token working would not be a reset. Never your + // own password: both callers refuse that (ownCredentialChanges). + if ($passwordReplaced) { + $user->tokens()->delete(); + } + if ($assignedClients !== null) { $this->syncAssignedClients($user, (int) $user->role_id, $assignedClients); } elseif ($oldRoleId !== $user->role_id) { diff --git a/resources/js/components/user-form.tsx b/resources/js/components/user-form.tsx index c5d4f20c..de078b29 100644 --- a/resources/js/components/user-form.tsx +++ b/resources/js/components/user-form.tsx @@ -1,10 +1,11 @@ +import { Link } from '@inertiajs/react'; import { X } from 'lucide-react'; import InputError from '@/components/input-error'; +import { PasswordRequirements } from '@/components/password-requirements'; import { Input } from '@/components/ui/input'; import { Label } from '@/components/ui/label'; import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from '@/components/ui/select'; -import { PasswordRequirements } from '@/components/password-requirements'; import { useTranslation } from '@/hooks/use-translation'; export interface AssignableRole { @@ -31,6 +32,12 @@ interface UserFormProps { assignedClients: number[]; onAssignedClientsChange: (ids: number[]) => void; passwordOptional: boolean; + /** + * Editing your own account: the email address and password are changed + * from your profile, which asks for your current password first, so + * this form shows the address and leaves both alone. + */ + ownAccount?: boolean; errors: Partial>; } @@ -46,6 +53,7 @@ export function UserForm({ assignedClients, onAssignedClientsChange, passwordOptional, + ownAccount = false, errors, }: UserFormProps) { const { t } = useTranslation(); @@ -62,7 +70,23 @@ export function UserForm({
- onChange('email', e.target.value)} required autoComplete="off" /> + onChange('email', e.target.value)} + required + readOnly={ownAccount} + autoComplete="off" + /> + {ownAccount && ( +

+ {t('Change your own email address and password from your profile.')}{' '} + + {t('Go to your profile')} + +

+ )}
@@ -92,32 +116,37 @@ export function UserForm({ /> )} -
- - onChange('password', e.target.value)} - required={!passwordOptional} - autoComplete="new-password" - /> - - -
+ {!ownAccount && ( + <> +
+ + onChange('password', e.target.value)} + required={!passwordOptional} + autoComplete="new-password" + /> + + +
-
- - onChange('password_confirmation', e.target.value)} - required={!passwordOptional || password !== ''} - autoComplete="new-password" - /> - -
+
+ + onChange('password_confirmation', e.target.value)} + required={!passwordOptional || password !== ''} + autoComplete="new-password" + /> + +
+ + )} + {ownAccount && } ); } diff --git a/resources/js/pages/users/edit.tsx b/resources/js/pages/users/edit.tsx index 1be2e976..7afab247 100644 --- a/resources/js/pages/users/edit.tsx +++ b/resources/js/pages/users/edit.tsx @@ -128,6 +128,7 @@ export default function UsersEdit({ assignedClients={data.assigned_clients} onAssignedClientsChange={(ids) => setData('assigned_clients', ids)} passwordOptional + ownAccount={is_self} errors={errors} />