mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-20 18:43:20 +00:00
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.
This commit is contained in:
@@ -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;
|
||||
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
Reference in New Issue
Block a user