From f3996a09cb5f0305aff332cb7ab9e6f1d5d56a4c Mon Sep 17 00:00:00 2001 From: Sourov Biswas Date: Sun, 20 Sep 2026 10:54:25 +0600 Subject: [PATCH] Leave name and email to 3AG Accounts The profile page offered a name and an email address to edit, and neither was this app's to change. Single sign-on force-fills both from the Accounts claims on every login, so an edit here survived exactly until the user signed in again. The email field was worse than useless. Invitations are found by pendingFor($user->email) and accepted on a lowercased string comparison, so typing a colleague's address into this form and saving was enough to be offered their pending invitations -- no verification anywhere in the path. The page already said password, two-factor and passkeys lived in Accounts; name and email always belonged in that sentence too. So the form goes, along with the request and the validation rules that only it used, and the page becomes a read-only view of what Accounts holds with a link to go and change it there. Refuse an email_verified of false while here. Accounts will not issue a code for an unverified address now, so this should never fire -- but the address decides which invitations the signer can accept and which pre-existing account they adopt, which is more than should rest on the identity provider alone holding its end up. A missing claim still signs in: OIDC makes it optional, so its absence means the issuer did not say, and refusing on that would empty the app the day Accounts stopped sending it. Co-Authored-By: Claude Opus 5 --- app/Concerns/ProfileValidationRules.php | 51 ------- .../Auth/AccountsSsoController.php | 9 ++ .../Settings/ProfileController.php | 19 --- .../Settings/ProfileUpdateRequest.php | 22 --- app/Services/Auth/AccountsOidc.php | 21 +++ app/Services/Auth/AccountsUser.php | 6 + resources/js/pages/settings/profile.tsx | 127 +++++++----------- routes/settings.php | 1 - tests/Browser/Settings/ProfileTest.php | 30 +++++ tests/Feature/Auth/AccountsSsoTest.php | 37 +++++ tests/Feature/Settings/ProfileUpdateTest.php | 55 ++++---- tests/Pest.php | 4 +- 12 files changed, 183 insertions(+), 199 deletions(-) delete mode 100644 app/Concerns/ProfileValidationRules.php delete mode 100644 app/Http/Requests/Settings/ProfileUpdateRequest.php create mode 100644 tests/Browser/Settings/ProfileTest.php diff --git a/app/Concerns/ProfileValidationRules.php b/app/Concerns/ProfileValidationRules.php deleted file mode 100644 index a9c069b..0000000 --- a/app/Concerns/ProfileValidationRules.php +++ /dev/null @@ -1,51 +0,0 @@ -|string>> - */ - protected function profileRules(?int $userId = null): array - { - return [ - 'name' => $this->nameRules(), - 'email' => $this->emailRules($userId), - ]; - } - - /** - * Get the validation rules used to validate user names. - * - * @return array|string> - */ - protected function nameRules(): array - { - return ['required', 'string', 'max:255']; - } - - /** - * Get the validation rules used to validate user emails. - * - * @return array|string> - */ - protected function emailRules(?int $userId = null): array - { - return [ - 'required', - 'string', - 'email', - 'max:255', - $userId === null - ? Rule::unique(User::class) - : Rule::unique(User::class)->ignore($userId), - ]; - } -} diff --git a/app/Http/Controllers/Auth/AccountsSsoController.php b/app/Http/Controllers/Auth/AccountsSsoController.php index e457b00..19b3cbe 100644 --- a/app/Http/Controllers/Auth/AccountsSsoController.php +++ b/app/Http/Controllers/Auth/AccountsSsoController.php @@ -59,6 +59,15 @@ public function callback(Request $request): RedirectResponse return $this->failed(__('3AG Accounts did not return an email address.')); } + // Accounts will not issue a code for an unverified address, so this + // should never fire. It is here because the address is what decides + // which invitations the signer can accept and which pre-existing + // account they adopt below, and that is too much to rest on the + // identity provider alone holding its end up. + if ($accountsUser->emailVerified === false) { + return $this->failed(__('Your 3AG Accounts email address is not verified yet. Verify it and sign in again.')); + } + $user = $this->link($accountsUser, Str::lower($email)); Auth::login($user, remember: true); diff --git a/app/Http/Controllers/Settings/ProfileController.php b/app/Http/Controllers/Settings/ProfileController.php index 5bebe2a..95f57a5 100644 --- a/app/Http/Controllers/Settings/ProfileController.php +++ b/app/Http/Controllers/Settings/ProfileController.php @@ -5,7 +5,6 @@ use App\Actions\Organizations\HandOverOwnedOrganizations; use App\Http\Controllers\Controller; use App\Http\Requests\Settings\ProfileDeleteRequest; -use App\Http\Requests\Settings\ProfileUpdateRequest; use Illuminate\Http\RedirectResponse; use Illuminate\Http\Request; use Illuminate\Support\Facades\Auth; @@ -24,24 +23,6 @@ public function edit(Request $request): Response ]); } - /** - * Update the user's profile information. - */ - public function update(ProfileUpdateRequest $request): RedirectResponse - { - $request->user()->fill($request->validated()); - - if ($request->user()->isDirty('email')) { - $request->user()->email_verified_at = null; - } - - $request->user()->save(); - - Inertia::flash('toast', ['type' => 'success', 'message' => __('Profile updated.')]); - - return to_route('profile.edit'); - } - /** * Delete the user's profile. */ diff --git a/app/Http/Requests/Settings/ProfileUpdateRequest.php b/app/Http/Requests/Settings/ProfileUpdateRequest.php deleted file mode 100644 index e4eb8d8..0000000 --- a/app/Http/Requests/Settings/ProfileUpdateRequest.php +++ /dev/null @@ -1,22 +0,0 @@ -|string> - */ - public function rules(): array - { - return $this->profileRules($this->user()->id); - } -} diff --git a/app/Services/Auth/AccountsOidc.php b/app/Services/Auth/AccountsOidc.php index 14c17f6..50b6f87 100644 --- a/app/Services/Auth/AccountsOidc.php +++ b/app/Services/Auth/AccountsOidc.php @@ -144,9 +144,30 @@ private function claims(string $accessToken): AccountsUser id: $subject, email: is_string($email) ? $email : null, name: is_string($name) ? $name : null, + emailVerified: $this->emailVerified($claims), ); } + /** + * Read the email_verified claim, if the issuer sent one. + * + * The claim is optional in OIDC, so its absence is not a denial -- it + * means the issuer did not say, and null keeps that distinct from an + * explicit false the caller should refuse. Accounts sends a JSON bool; + * the filter also copes with the "true"/"false" strings some issuers + * send, and answers null for anything it cannot read either way. + * + * @param array $claims + */ + private function emailVerified(array $claims): ?bool + { + if (! array_key_exists('email_verified', $claims)) { + return null; + } + + return filter_var($claims['email_verified'], FILTER_VALIDATE_BOOLEAN, FILTER_NULL_ON_FAILURE); + } + /** * Get one endpoint from the issuer's discovery document. * diff --git a/app/Services/Auth/AccountsUser.php b/app/Services/Auth/AccountsUser.php index cf4c911..8ebd96b 100644 --- a/app/Services/Auth/AccountsUser.php +++ b/app/Services/Auth/AccountsUser.php @@ -7,9 +7,15 @@ */ class AccountsUser { + /** + * @param bool|null $emailVerified Null when the issuer sent no + * email_verified claim, which OIDC + * allows: not said, rather than no. + */ public function __construct( public readonly string $id, public readonly ?string $email, public readonly ?string $name, + public readonly ?bool $emailVerified = null, ) {} } diff --git a/resources/js/pages/settings/profile.tsx b/resources/js/pages/settings/profile.tsx index 63a0582..b5d952a 100644 --- a/resources/js/pages/settings/profile.tsx +++ b/resources/js/pages/settings/profile.tsx @@ -1,10 +1,8 @@ -import { Form, Head, usePage } from '@inertiajs/react'; -import ProfileController from '@/actions/App/Http/Controllers/Settings/ProfileController'; +import { Head, usePage } from '@inertiajs/react'; +import { ExternalLink } from 'lucide-react'; import DeleteUser from '@/components/delete-user'; import Heading from '@/components/heading'; -import InputError from '@/components/input-error'; import { Button } from '@/components/ui/button'; -import { Input } from '@/components/ui/input'; import { Label } from '@/components/ui/label'; import { edit } from '@/routes/profile'; import type { Auth } from '@/types'; @@ -14,6 +12,14 @@ type PageProps = { accountsUrl: string; }; +/** + * A read-only view of the identity 3AG Accounts holds. + * + * Nothing here is editable because nothing here was ever the local app's to + * change: single sign-on rewrites the name and email address from the + * Accounts claims on every login, so a local edit only ever lasted until the + * user signed in again. + */ export default function Profile() { const { auth, accountsUrl } = usePage().props; @@ -27,82 +33,53 @@ export default function Profile() { -
- {({ processing, errors }) => ( - <> -
- +
+
+ +
+ {auth.user.name} +
+
- +
+ +
+ {auth.user.email} +
+
+
- -
+

+ Your name, email address, password, two-factor + authentication, and passkeys are all managed in 3AG + Accounts. Changes there apply to every 3AG app the next time + you sign in. +

-
- - - - - -
- -

- Password, two-factor authentication, and - passkeys are managed in{' '} - - 3AG Accounts - - . -

- -
- -
- - )} -
+ diff --git a/routes/settings.php b/routes/settings.php index e929d51..ca4c27a 100644 --- a/routes/settings.php +++ b/routes/settings.php @@ -11,7 +11,6 @@ Route::redirect('settings', '/settings/profile'); Route::get('settings/profile', [ProfileController::class, 'edit'])->name('profile.edit'); - Route::patch('settings/profile', [ProfileController::class, 'update'])->name('profile.update'); }); Route::middleware(['auth', 'verified'])->group(function () { diff --git a/tests/Browser/Settings/ProfileTest.php b/tests/Browser/Settings/ProfileTest.php new file mode 100644 index 0000000..8b42f80 --- /dev/null +++ b/tests/Browser/Settings/ProfileTest.php @@ -0,0 +1,30 @@ +user = User::factory()->create([ + 'name' => 'Ada Lovelace', + 'email' => 'ada@3ag.local', + ]); + + $this->actingAs($this->user); +}); + +it('shows the identity Accounts holds without offering to edit it', function () { + $page = visit(route('profile.edit')); + + $page->assertSee('Ada Lovelace') + ->assertSee('ada@3ag.local') + ->assertNoJavaScriptErrors(); + + // Nothing to type into and nothing to submit: the only inputs left on the + // page belong to the delete-account dialog, which is still the user's own. + expect($page->script('document.querySelectorAll(\'input[name="email"], input[name="name"]\').length'))->toBe(0); +}); + +it('points the user at Accounts to change any of it', function () { + visit(route('profile.edit')) + ->assertAttribute('@accounts-link', 'href', rtrim((string) config('oidc.connections.accounts.base_url'), '/')) + ->assertNoJavaScriptErrors(); +}); diff --git a/tests/Feature/Auth/AccountsSsoTest.php b/tests/Feature/Auth/AccountsSsoTest.php index 3f75750..1bf2690 100644 --- a/tests/Feature/Auth/AccountsSsoTest.php +++ b/tests/Feature/Auth/AccountsSsoTest.php @@ -201,6 +201,43 @@ expect(User::query()->count())->toBe(0); }); +test('an address Accounts reports as unverified is refused', function () { + signInThroughAccounts(['email_verified' => false]); + + $this->get(route('auth.accounts.callback', ['code' => 'the-code', 'state' => 'state'])) + ->assertRedirect('/') + ->assertInertiaFlash('toast.type', 'error'); + + $this->assertGuest(); + + expect(User::query()->count())->toBe(0); +}); + +test('an unverified address cannot take over an account that already exists', function () { + $victim = User::factory()->create(['email' => 'signer@example.com', 'sso_id' => null]); + + signInThroughAccounts(['email_verified' => false]); + + $this->get(route('auth.accounts.callback', ['code' => 'the-code', 'state' => 'state'])); + + $this->assertGuest(); + + expect($victim->fresh()->sso_id)->toBeNull(); +}); + +test('an issuer that sends no email_verified claim is still trusted', function () { + // The claim is optional in OIDC, so a missing one means the issuer did + // not say -- not that it said no. Refusing here would lock every user + // out of the app the moment Accounts stopped sending it. + signInThroughAccounts(['email_verified' => null]); + + $this->get(route('auth.accounts.callback', ['code' => 'the-code', 'state' => 'state'])); + + expect(User::query()->where('email', 'signer@example.com')->exists())->toBeTrue(); + + $this->assertAuthenticated(); +}); + test('a nameless account falls back to the local part of the email', function () { signInThroughAccounts(['name' => null]); diff --git a/tests/Feature/Settings/ProfileUpdateTest.php b/tests/Feature/Settings/ProfileUpdateTest.php index e2e56a2..6350536 100644 --- a/tests/Feature/Settings/ProfileUpdateTest.php +++ b/tests/Feature/Settings/ProfileUpdateTest.php @@ -6,6 +6,8 @@ use App\Models\Shop; use App\Models\User; use Illuminate\Support\Facades\Queue; +use Illuminate\Support\Facades\Route; +use Inertia\Testing\AssertableInertia as Assert; test('profile page is displayed', function () { $user = User::factory()->create(); @@ -17,42 +19,35 @@ $response->assertOk(); }); -test('profile information can be updated', function () { - $user = User::factory()->create(); - - $response = $this - ->actingAs($user) - ->patch(route('profile.update'), [ - 'name' => 'Test User', - 'email' => 'test@example.com', - ]); - - $response - ->assertSessionHasNoErrors() - ->assertRedirect(route('profile.edit')); +test('the profile page shows the identity accounts holds, read only', function () { + $user = User::factory()->create(['name' => 'Ada Lovelace', 'email' => 'ada@3ag.local']); + + $this->actingAs($user) + ->get(route('profile.edit')) + ->assertOk() + ->assertInertia(fn (Assert $page) => $page + ->component('settings/profile') + ->where('auth.user.name', 'Ada Lovelace') + ->where('auth.user.email', 'ada@3ag.local'), + ); +}); - $user->refresh(); +test('there is no route left to edit the profile with', function () { + expect(Route::has('profile.update'))->toBeFalse(); - expect($user->name)->toBe('Test User'); - expect($user->email)->toBe('test@example.com'); - expect($user->email_verified_at)->toBeNull(); + $this->actingAs(User::factory()->create()) + ->patch('/settings/profile', ['name' => 'Someone Else', 'email' => 'someone-else@3ag.local']) + ->assertMethodNotAllowed(); }); -test('email verification status is unchanged when the email address is unchanged', function () { - $user = User::factory()->create(); +test('the email address a user signs in with cannot be changed from here', function () { + $user = User::factory()->create(['email' => 'ada@3ag.local']); - $response = $this - ->actingAs($user) - ->patch(route('profile.update'), [ - 'name' => 'Test User', - 'email' => $user->email, - ]); - - $response - ->assertSessionHasNoErrors() - ->assertRedirect(route('profile.edit')); + $this->actingAs($user) + ->post('/settings/profile', ['name' => 'Someone Else', 'email' => 'victim@3ag.local']) + ->assertMethodNotAllowed(); - expect($user->refresh()->email_verified_at)->not->toBeNull(); + expect($user->fresh()->email)->toBe('ada@3ag.local'); }); test('user can delete their account', function () { diff --git a/tests/Pest.php b/tests/Pest.php index eb194df..8f93827 100644 --- a/tests/Pest.php +++ b/tests/Pest.php @@ -48,7 +48,8 @@ /** * Answer the Accounts endpoints the OIDC client calls. * - * Pass a null claim to drop it from the userinfo response, or an 'endpoints' + * Pass a null claim to drop it from the userinfo response -- which is how to + * model an issuer that sends no email_verified at all -- or an 'endpoints' * key to reshape the discovery document. * * @param array $claims @@ -68,6 +69,7 @@ function fakeAccounts(array $claims = [], array $endpoints = []): void 'accounts.test/oauth/userinfo' => Http::response(array_filter([ 'sub' => 'oidc-sub-42', 'email' => 'signer@example.com', + 'email_verified' => true, 'name' => 'The Signer', ...$claims, ], fn (mixed $value) => $value !== null)),