feat: let only the first github claim make an organization admin

KNOWLEDGE_BASE.md

@@ -113,7 +113,7 @@ GitHub organizations, like `@laravel`, have a page at `/@{handle}` too, with the
 
 The **Mentions** tab lists the commits and replies that mention the organization, newest first, a reply with the commit it answers. A commit stays listed after the organization is renamed, since it is linked to the organization rather than to the handle. It follows the usual visibility: guests see approved commits only, you also see your own pending ones, and commits across a block or by people you muted are left out.
 
-Members of an organization can **Claim** its page, which lists them as its admins. Claiming asks GitHub once for permission to read your organizations, then checks that you are an owner or a member there, not just invited. Organizations that restrict third-party apps only share their members once an owner approved /dev/null: the page then links to GitHub, where an owner grants access and a member requests it. GitHub is only the way in: leaving the organization on GitHub doesn't remove you as an admin on /dev/null. Claims are in your data export and go when you delete your account.
+A member of an organization can **Claim** its page, which lists them as its admin. Only the first member to claim it becomes its admin: once a page has one, it can't be claimed again, and the Claim button is gone. Claiming asks GitHub once for permission to read your organizations, then checks that you are an owner or a member there, not just invited. Organizations that restrict third-party apps only share their members once an owner approved /dev/null: the page then links to GitHub, where an owner grants access and a member requests it. GitHub is only the way in: leaving the organization on GitHub doesn't remove you as an admin on /dev/null. Claims are in your data export and go when you delete your account.
 
 Following organizations is planned, not shipped yet.
 

app/Actions/CreateOrganizationAdmin.php

@@ -6,15 +6,27 @@
 
 use App\Models\Profile;
 use App\Models\User;
+use Illuminate\Support\Facades\DB;
 
 final readonly class CreateOrganizationAdmin
 {
     /**
-     * Makes the user an admin of the organization's page, once GitHub confirmed they belong to it. Claiming again
-     * changes nothing.
+     * Makes the user the admin of the organization's page, once GitHub confirmed they belong to it, unless someone
+     * claimed it first. The organization is locked while it's checked, as two members coming back from GitHub at once
+     * can both pass the request's check: the second claim then just doesn't happen.
      */
-    public function handle(Profile $organization, User $user): void
+    public function handle(Profile $organization, User $user): bool
     {
-        $organization->admins()->syncWithoutDetaching([$user->id]);
+        return DB::transaction(function () use ($organization, $user): bool {
+            Profile::query()->whereKey($organization->id)->lockForUpdate()->first();
+
+            if ($organization->admins()->exists()) {
+                return false;
+            }
+
+            $organization->admins()->attach($user->id);
+
+            return true;
+        });
     }
 }

app/Http/Controllers/OrganizationAdminController.php

@@ -34,6 +34,12 @@
      */
     public function create(CreateOrganizationAdminRequest $request, Profile $organization): SymfonyResponse
     {
+        $claimed = $this->claimed($organization);
+
+        if ($claimed !== null) {
+            return $this->failed($claimed, to_route('user.show', $organization->handle));
+        }
+
         $request->session()->put(self::CLAIMING, $organization->id);
 
         return Inertia::location($this->github()->scopes(['read:org'])->redirect()->getTargetUrl());
@@ -56,6 +62,13 @@ public function store(Request $request, #[CurrentUser] User $user, GitHub $githu
 
         $page = to_route('user.show', $organization->handle);
 
+        // Claimed by someone else while the user was on GitHub...
+        $claimed = $this->claimed($organization);
+
+        if ($claimed !== null) {
+            return $this->failed($claimed, $page);
+        }
+
         if ($request->has('error')) {
             return $this->failed(__('GitHub was not asked about your organizations, so the page was not claimed.'), $page);
         }
@@ -96,7 +109,9 @@ public function store(Request $request, #[CurrentUser] User $user, GitHub $githu
             return $this->failed($failure[0], $page, $failure[1]);
         }
 
-        $action->handle($organization, $user);
+        if (! $action->handle($organization, $user)) {
+            return $this->failed($this->claimed($organization) ?? __('Unable to claim this organization. Please try again.'), $page);
+        }
 
         Inertia::flash('toast', ['type' => 'success', 'message' => __('You are now an admin of @:handle.', ['handle' => $organization->handle])]);
 
@@ -114,6 +129,20 @@ private function github(): GithubProvider
         return $provider->redirectUrl(route('organization-admin.store'));
     }
 
+    /**
+     * Only the first claim through GitHub makes an admin. Why another one fails, if the page was claimed.
+     */
+    private function claimed(Profile $organization): ?string
+    {
+        $admin = $organization->admins()->oldest('organization_admins.created_at')->first();
+
+        if ($admin === null) {
+            return null;
+        }
+
+        return __('@:handle was already claimed by @:admin.', ['handle' => $organization->handle, 'admin' => $admin->handle]);
+    }
+
     /**
      * The page says why the claim failed until the user leaves it, with where to approve /dev/null when that's why.
      */

resources/js/pages/organization/show.tsx

@@ -31,11 +31,9 @@ export default function Show({
     const { auth } = props;
     // Kept until the user leaves the page, as the background reloads bring no flash and would hide it.
     const [claimFailed] = useState(flash.claimFailed);
-    // While a claim failed, its alert offers to try again instead.
-    const canClaim =
-        auth.user !== null &&
-        claimFailed === undefined &&
-        !admins.some((admin) => admin.id === auth.user?.id);
+    // Only the first claim makes an admin. While a claim failed, its alert offers to try again instead.
+    const claimable = auth.user !== null && admins.length === 0;
+    const canClaim = claimable && claimFailed === undefined;
 
     return (
         <>
@@ -155,29 +153,37 @@ export default function Show({
                         {/* The reason in the text color, as red at this size is hard to read. */}
                         <AlertDescription className="text-foreground">
                             <p>{claimFailed.message}</p>
-                            <div className="mt-2 flex flex-wrap gap-2">
-                                {claimFailed.approvalUrl && (
-                                    <Button size="sm" asChild>
-                                        <a
-                                            href={claimFailed.approvalUrl}
-                                            target="_blank"
-                                            rel="noopener noreferrer"
+                            {(claimFailed.approvalUrl || claimable) && (
+                                <div className="mt-2 flex flex-wrap gap-2">
+                                    {claimFailed.approvalUrl && (
+                                        <Button size="sm" asChild>
+                                            <a
+                                                href={claimFailed.approvalUrl}
+                                                target="_blank"
+                                                rel="noopener noreferrer"
+                                            >
+                                                <GitHubIcon className="size-4" />
+                                                Open GitHub
+                                            </a>
+                                        </Button>
+                                    )}
+                                    {claimable && (
+                                        <Button
+                                            size="sm"
+                                            variant="outline"
+                                            asChild
                                         >
-                                            <GitHubIcon className="size-4" />
-                                            Open GitHub
-                                        </a>
-                                    </Button>
-                                )}
-                                <Button size="sm" variant="outline" asChild>
-                                    <Link
-                                        href={claimUrl}
-                                        method="post"
-                                        as="button"
-                                    >
-                                        Try again
-                                    </Link>
-                                </Button>
-                            </div>
+                                            <Link
+                                                href={claimUrl}
+                                                method="post"
+                                                as="button"
+                                            >
+                                                Try again
+                                            </Link>
+                                        </Button>
+                                    )}
+                                </div>
+                            )}
                         </AlertDescription>
                     </Alert>
                 )}

tests/Feature/Controllers/OrganizationAdminControllerTest.php

@@ -113,19 +113,57 @@ function claimingUserAndOrganization(): array
     Http::assertSent(fn (Request $request): bool => $request->hasHeader('Authorization', 'Bearer claim-token'));
 })->with(['admin', 'member']);
 
-it('keeps the first claim when the user claims again', function (): void {
+it('does not send anyone to github for a page someone already claimed', function (): void {
+    [$user, $organization] = claimingUserAndOrganization();
+    $admin = User::factory()->create();
+    $organization->admins()->attach($admin);
+
+    $this->actingAs($user)
+        ->post(route('organization-admin.create', $organization))
+        ->assertRedirect('/@brefphp')
+        ->assertInertiaFlash('claimFailed', ['message' => "@brefphp was already claimed by @{$admin->handle}.", 'approvalUrl' => null]);
+});
+
+it('refuses a claim when someone else claimed the page while the user was on github', function (): void {
     fakeClaimingGitHubAccount('720328');
     fakeGitHubMembership();
     [$user, $organization] = claimingUserAndOrganization();
-    $claimedAt = now()->subWeek()->startOfSecond();
-    DB::table('organization_admins')->insert(['organization_id' => $organization->id, 'user_id' => $user->id, 'created_at' => $claimedAt]);
+    $first = User::factory()->create();
+    $second = User::factory()->create();
+    DB::table('organization_admins')->insert([
+        ['organization_id' => $organization->id, 'user_id' => $second->id, 'created_at' => now()],
+        ['organization_id' => $organization->id, 'user_id' => $first->id, 'created_at' => now()->subDay()],
+    ]);
+
+    $this->actingAs($user)
+        ->withSession(['claiming-organization' => $organization->id])
+        ->get(route('organization-admin.store', ['code' => 'code', 'state' => 'state']))
+        ->assertRedirect('/@brefphp')
+        ->assertSessionMissing('claiming-organization')
+        ->assertInertiaFlash('claimFailed.message', "@brefphp was already claimed by @{$first->handle}.");
+
+    expect($organization->admins()->pluck('users.id')->sort()->values()->all())->toBe(collect([$first->id, $second->id])->sort()->values()->all());
+    Http::assertNothingSent();
+});
+
+it('refuses a member who loses the claim to another coming back from github at the same moment', function (): void {
+    fakeClaimingGitHubAccount('720328');
+    [$user, $organization] = claimingUserAndOrganization();
+    $other = User::factory()->create();
+    Http::fake(['api.github.com/user/memberships/orgs/brefphp' => function () use ($organization, $other) {
+        $organization->admins()->attach($other);
+
+        return Http::response(['state' => 'active', 'role' => 'member', 'organization' => ['id' => 45861341]]);
+    }]);
 
     $this->actingAs($user)
         ->withSession(['claiming-organization' => $organization->id])
         ->get(route('organization-admin.store'))
-        ->assertRedirect('/@brefphp');
+        ->assertRedirect('/@brefphp')
+        ->assertInertiaFlash('claimFailed.message', "@brefphp was already claimed by @{$other->handle}.")
+        ->assertInertiaFlashMissing('toast');
 
-    expect(DB::table('organization_admins')->sole()->created_at)->toBe($claimedAt->toDateTimeString());
+    expect($organization->admins()->pluck('users.id')->all())->toBe([$other->id]);
 });
 
 it('refuses users github does not say are members', function (array $membership, int $status): void {

tests/Unit/Actions/CreateOrganizationAdminTest.php

@@ -0,0 +1,16 @@
+<?php
+
+declare(strict_types=1);
+
+use App\Actions\CreateOrganizationAdmin;
+use App\Models\Profile;
+use App\Models\User;
+
+it('makes only the first of two members coming back from github at once the admin', function (): void {
+    $organization = Profile::factory()->organization()->create();
+    [$first, $second] = User::factory()->count(2)->create();
+
+    expect(resolve(CreateOrganizationAdmin::class)->handle($organization, $first))->toBeTrue()
+        ->and(resolve(CreateOrganizationAdmin::class)->handle($organization, $second))->toBeFalse()
+        ->and($organization->admins()->pluck('users.id')->all())->toBe([$first->id]);
+});