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)),