From f572341e36f33ce894465e0839a4b8df4f62fee4 Mon Sep 17 00:00:00 2001 From: Paulo Castellano Date: Mon, 22 Jun 2026 14:41:11 -0300 Subject: [PATCH] fix(members): block self role-change/removal and lock role rules with tests The remove/role-change guards only protected the account owner, so a non-owner admin could change their own role or remove themselves via a crafted request (the UI hides it, but the backend didn't). Add an explicit self-guard to both updateRole and removeMember. Lock the whole role system with tests: accept assigns the exact invited role (viewer/admin/member), invite requires and persists a role, updateRole supports viewer and blocks self/owner/invalid, removeMember blocks self/owner, and a viewer is read-only (view yes; create post / manage team / invite no). --- .../App/WorkspaceInviteController.php | 10 +++++ .../Feature/WorkspaceInviteControllerTest.php | 42 +++++++++++++++++++ tests/Unit/Policies/WorkspacePolicyTest.php | 21 ++++++++++ 3 files changed, 73 insertions(+) diff --git a/app/Http/Controllers/App/WorkspaceInviteController.php b/app/Http/Controllers/App/WorkspaceInviteController.php index cf43fade..69cab4d2 100644 --- a/app/Http/Controllers/App/WorkspaceInviteController.php +++ b/app/Http/Controllers/App/WorkspaceInviteController.php @@ -120,6 +120,11 @@ public function removeMember(Request $request, string $userId): RedirectResponse $this->authorize('manageTeam', $workspace); + // You cannot remove yourself + if ($userId === $request->user()->id) { + return back()->withErrors(['member' => 'You cannot remove yourself.']); + } + // Account owner cannot be removed if ($userId === $workspace->account?->owner_id) { return back()->withErrors(['member' => 'Cannot remove the account owner.']); @@ -143,6 +148,11 @@ public function updateRole(Request $request, string $userId): RedirectResponse $this->authorize('manageTeam', $workspace); + // You cannot change your own role + if ($userId === $request->user()->id) { + return back()->withErrors(['role' => 'You cannot change your own role.']); + } + // Account owner's role cannot be changed if ($userId === $workspace->account?->owner_id) { return back()->withErrors(['role' => 'Cannot change the account owner role.']); diff --git a/tests/Feature/WorkspaceInviteControllerTest.php b/tests/Feature/WorkspaceInviteControllerTest.php index d54f7aa3..bff21193 100644 --- a/tests/Feature/WorkspaceInviteControllerTest.php +++ b/tests/Feature/WorkspaceInviteControllerTest.php @@ -227,6 +227,48 @@ expect($this->workspace->members()->where('user_id', $member->id)->first()->pivot->role)->toBe(WorkspaceRole::Admin->value); }); +test('update role changes member to viewer', function () { + $member = User::factory()->create([ + 'account_id' => $this->account->id, + ]); + $this->workspace->members()->attach($member->id, ['role' => WorkspaceRole::Member->value]); + + $response = $this->actingAs($this->user)->put(route('app.members.update-role', $member), [ + 'role' => WorkspaceRole::Viewer->value, + ]); + + $response->assertRedirect(); + expect($this->workspace->members()->where('user_id', $member->id)->first()->pivot->role)->toBe(WorkspaceRole::Viewer->value); +}); + +test('an admin cannot change their own role', function () { + $admin = User::factory()->create([ + 'account_id' => $this->account->id, + ]); + $this->workspace->members()->attach($admin->id, ['role' => WorkspaceRole::Admin->value]); + $admin->update(['current_workspace_id' => $this->workspace->id]); + + $response = $this->actingAs($admin)->put(route('app.members.update-role', $admin), [ + 'role' => WorkspaceRole::Member->value, + ]); + + $response->assertSessionHasErrors('role'); + expect($this->workspace->members()->where('user_id', $admin->id)->first()->pivot->role)->toBe(WorkspaceRole::Admin->value); +}); + +test('an admin cannot remove themselves', function () { + $admin = User::factory()->create([ + 'account_id' => $this->account->id, + ]); + $this->workspace->members()->attach($admin->id, ['role' => WorkspaceRole::Admin->value]); + $admin->update(['current_workspace_id' => $this->workspace->id]); + + $response = $this->actingAs($admin)->delete(route('app.members.remove', $admin)); + + $response->assertSessionHasErrors('member'); + expect($this->workspace->members()->where('user_id', $admin->id)->exists())->toBeTrue(); +}); + test('update role changes admin to member', function () { $member = User::factory()->create([ 'account_id' => $this->account->id, diff --git a/tests/Unit/Policies/WorkspacePolicyTest.php b/tests/Unit/Policies/WorkspacePolicyTest.php index 63f2c22a..7bc3bd45 100644 --- a/tests/Unit/Policies/WorkspacePolicyTest.php +++ b/tests/Unit/Policies/WorkspacePolicyTest.php @@ -256,6 +256,27 @@ expect($this->policy->createPost($otherUser, $workspace))->toBeFalse(); }); +test('a viewer can view but cannot create posts or manage the team', function () { + $account = Account::factory()->create(); + $owner = User::factory()->create([ + 'account_id' => $account->id, + ]); + $account->update(['owner_id' => $owner->id]); + $viewer = User::factory()->create([ + 'account_id' => $account->id, + ]); + $workspace = Workspace::factory()->create([ + 'account_id' => $account->id, + 'user_id' => $owner->id, + ]); + $workspace->members()->attach($viewer->id, ['role' => Role::Viewer->value]); + + expect($this->policy->view($viewer, $workspace))->toBeTrue(); + expect($this->policy->createPost($viewer, $workspace))->toBeFalse(); + expect($this->policy->manageTeam($viewer, $workspace))->toBeFalse(); + expect($this->policy->inviteMember($viewer, $workspace))->toBeFalse(); +}); + test('only account owner can manage billing', function () { $account = Account::factory()->create(); $owner = User::factory()->create([