From beef18e9a8efddd91407698818f45eed87eed94d Mon Sep 17 00:00:00 2001 From: Amp Date: Thu, 29 Oct 2026 17:19:17 +0000 Subject: [PATCH] feat: let only the first github claim make an organization admin Once an organization's page has an admin, the Claim button is hidden and both claim endpoints refuse with who already claimed it, including when the page was claimed while the user was on GitHub. Admins added another way, like a future admin panel, are unaffected. Co-authored-by: Matthieu Amp-Thread-ID: https://ampcode.com/threads/T-01a18855-a19e-7393-aabd-4e6c09ce4353 --- KNOWLEDGE_BASE.md | 2 +- app/Actions/CreateOrganizationAdmin.php | 20 +++++-- .../OrganizationAdminController.php | 31 +++++++++- resources/js/pages/organization/show.tsx | 60 ++++++++++--------- .../OrganizationAdminControllerTest.php | 48 +++++++++++++-- .../Actions/CreateOrganizationAdminTest.php | 16 +++++ 6 files changed, 139 insertions(+), 38 deletions(-) create mode 100644 tests/Unit/Actions/CreateOrganizationAdminTest.php diff --git a/KNOWLEDGE_BASE.md b/KNOWLEDGE_BASE.md index a73b4cc22..2ab04e212 100644 --- a/KNOWLEDGE_BASE.md +++ b/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. diff --git a/app/Actions/CreateOrganizationAdmin.php b/app/Actions/CreateOrganizationAdmin.php index 96102f357..8d178e0b2 100644 --- a/app/Actions/CreateOrganizationAdmin.php +++ b/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; + }); } } diff --git a/app/Http/Controllers/OrganizationAdminController.php b/app/Http/Controllers/OrganizationAdminController.php index fc4f06193..2531221fe 100644 --- a/app/Http/Controllers/OrganizationAdminController.php +++ b/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. */ diff --git a/resources/js/pages/organization/show.tsx b/resources/js/pages/organization/show.tsx index 03f756a1b..c86f70d04 100644 --- a/resources/js/pages/organization/show.tsx +++ b/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. */}

{claimFailed.message}

-
- {claimFailed.approvalUrl && ( - + )} + {claimable && ( + - )} - -
+ + Try again + + + )} + + )} )} diff --git a/tests/Feature/Controllers/OrganizationAdminControllerTest.php b/tests/Feature/Controllers/OrganizationAdminControllerTest.php index 9f83a5b1d..f556f61df 100644 --- a/tests/Feature/Controllers/OrganizationAdminControllerTest.php +++ b/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 { diff --git a/tests/Unit/Actions/CreateOrganizationAdminTest.php b/tests/Unit/Actions/CreateOrganizationAdminTest.php new file mode 100644 index 000000000..c70f63417 --- /dev/null +++ b/tests/Unit/Actions/CreateOrganizationAdminTest.php @@ -0,0 +1,16 @@ +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]); +});