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} />