From 5242169bb061855677bcb82c570aaa807afbb343 Mon Sep 17 00:00:00 2001
From: denkfabrik-li <274324701+denkfabrik-li@users.noreply.github.com>
Date: Wed, 26 Aug 2026 06:09:49 +0200
Subject: [PATCH] Deny a membership request once, as approve() already does

approve() refuses a request that is not pending:

    abort_unless($group !== null && $client !== null
        && $membershipRequest->status === MembershipRequest::STATUS_PENDING, 404);

deny(), one method below, checks nothing. Denying is not idempotent, so
repeating it is not a no-op:

  - denied_at is stamped again, and that is what the client's re-request
    cooldown counts from (MyGroupsController::inDenyCooldown). Repeating
    the request keeps one client out of one group for as long as somebody
    cares to keep asking, without a single new decision being made.
  - a second GroupMembershipDenied entry goes into the activity log, for
    a denial that did not happen.
  - a second "your request was declined" mail goes to the client.

The queue lists only pending requests, so nothing on the screen offers
this; it takes asking for the route directly. It needs
approve_groups_memberships_requests, so it is not a stranger's move.

The guard is the same one, answering the same 404, placed where deny()
can reach it. deny() keeps tolerating a vanished group or client -- that
tolerance is deliberate and separate: the denied row persists for the
cooldown even when the group it named is gone, and index() already
filters those rows out with whereHas.

Not in this change: deny() writes the status, the log entry and the
notification without a shared transaction. approve() has exactly the same
shape, so fixing one alone would replace a symmetry with a difference,
and doing both means also deciding where the mail sits relative to the
commit -- which is the question #1691 answers for file bytes, and worth
answering on its own rather than inside a state-machine fix.
---
 .../MembershipRequestsController.php          | 11 +++
 .../Groups/DenyMembershipRequestOnceTest.php  | 84 +++++++++++++++++++
 2 files changed, 95 insertions(+)
 create mode 100644 tests/Feature/Groups/DenyMembershipRequestOnceTest.php

diff --git a/app/Modules/Groups/Http/Controllers/MembershipRequestsController.php b/app/Modules/Groups/Http/Controllers/MembershipRequestsController.php
index f476c251..faa228e6 100644
--- a/app/Modules/Groups/Http/Controllers/MembershipRequestsController.php
+++ b/app/Modules/Groups/Http/Controllers/MembershipRequestsController.php
@@ -87,6 +87,17 @@ class MembershipRequestsController extends Controller
 
     public function deny(MembershipRequest $membershipRequest): RedirectResponse
     {
+        // The half of approve()'s guard that applies here. A request that
+        // has already been denied is not a decision left to make, and
+        // taking it again re-stamps denied_at -- which is what the
+        // client's re-request cooldown counts from, so the same request
+        // repeated keeps a client out of a group indefinitely -- while
+        // writing a second log entry and sending a second "your request
+        // was declined" mail for one decision. The queue only ever lists
+        // pending requests, so this is not reachable through the screen;
+        // it is reachable by asking for the route directly.
+        abort_unless($membershipRequest->status === MembershipRequest::STATUS_PENDING, 404);
+
         $group = $membershipRequest->group;
         $client = $membershipRequest->user;
 
diff --git a/tests/Feature/Groups/DenyMembershipRequestOnceTest.php b/tests/Feature/Groups/DenyMembershipRequestOnceTest.php
new file mode 100644
index 00000000..5177f774
--- /dev/null
+++ b/tests/Feature/Groups/DenyMembershipRequestOnceTest.php
@@ -0,0 +1,84 @@
+<?php
+
+declare(strict_types=1);
+
+use App\Models\User;
+use App\Modules\Audit\Action;
+use App\Modules\Audit\ActivityLog;
+use App\Modules\Groups\Models\Group;
+use App\Modules\Groups\Models\MembershipRequest;
+use App\Modules\Groups\Notifications\GroupMembershipDeniedNotification;
+use App\Modules\Platform\Settings\Setting;
+use App\Modules\Platform\Settings\Settings;
+use Illuminate\Support\Facades\Notification;
+
+/**
+ * approve() refuses a request that is not pending. deny() did not, and a
+ * denial is not idempotent: it re-stamps denied_at, which is what the
+ * client's re-request cooldown counts from.
+ */
+beforeEach(function () {
+    $this->admin = User::factory()->create();
+    $this->client = User::factory()->client()->create();
+    $this->group = Group::query()->create(['name' => 'Guarded', 'slug' => 'guarded', 'public' => true]);
+
+    $this->request = MembershipRequest::query()->create([
+        'group_id' => $this->group->id,
+        'user_id' => $this->client->id,
+        'status' => MembershipRequest::STATUS_PENDING,
+    ]);
+});
+
+test('denying a pending request works, once', function () {
+    $this->actingAs($this->admin)->delete("/membership-requests/{$this->request->id}")->assertRedirect();
+
+    expect($this->request->fresh()->status)->toBe(MembershipRequest::STATUS_DENIED);
+
+    $this->actingAs($this->admin)->delete("/membership-requests/{$this->request->id}")->assertNotFound();
+});
+
+test('a second denial does not move the stamp, or the log, or the mail', function () {
+    Notification::fake();
+    app(Settings::class)->set(Setting::EmailNotificationsEnabled, true);
+
+    $this->actingAs($this->admin)->delete("/membership-requests/{$this->request->id}");
+    $stamped = $this->request->fresh()->denied_at;
+
+    $this->travel(5)->days();
+    $this->actingAs($this->admin)->delete("/membership-requests/{$this->request->id}")->assertNotFound();
+
+    expect($this->request->fresh()->denied_at?->toIso8601String())->toBe($stamped?->toIso8601String())
+        ->and(ActivityLog::query()->where('action', Action::GroupMembershipDenied->value)->count())->toBe(1);
+
+    Notification::assertSentToTimes($this->client, GroupMembershipDeniedNotification::class, 1);
+});
+
+test('a replayed denial cannot hold a client out of a group past the cooldown', function () {
+    app(Settings::class)->set(Setting::ClientsCanSelectGroup, 'public');
+    app(Settings::class)->set(Setting::ClientsMembershipDenyCooldownDays, 30);
+
+    $this->actingAs($this->admin)->delete("/membership-requests/{$this->request->id}");
+
+    // Day 29: still inside the cooldown, and a denial arriving now used
+    // to restart it.
+    $this->travel(29)->days();
+    $this->actingAs($this->admin)->delete("/membership-requests/{$this->request->id}")->assertNotFound();
+
+    $this->travel(2)->days();
+
+    $this->actingAs($this->client)
+        ->post('/my-groups', ['group_id' => $this->group->id])
+        ->assertSessionHasNoErrors();
+
+    expect($this->request->fresh()->status)->toBe(MembershipRequest::STATUS_PENDING);
+});
+
+test('approve already refused a request that is not pending', function () {
+    $this->actingAs($this->admin)->delete("/membership-requests/{$this->request->id}");
+
+    $this->actingAs($this->admin)
+        ->post("/membership-requests/{$this->request->id}/approve")
+        ->assertNotFound();
+
+    expect($this->group->members()->count())->toBe(0);
+});