From dd43b88fffbcf40f88042158077379c7bbef1afa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vi=CC=81ctor=20Falco=CC=81n?= Date: Mon, 6 Jul 2026 18:18:15 +0200 Subject: [PATCH] fix(spaces): make members first-class in shared spaces (review) Address the review findings that blocked collaboration and hardened tenancy: - Ownership validation and the view/budget/real-estate policies scope by the active space instead of user_id, so a member can open detail pages and create or edit data in a shared space (previously 422/403). - Authorize purely on live space membership, dropping the current_space_id shortcut, and reset a removed member's pointer to their personal space so no stale-pointer write window remains. - Apply automation rules per space, so a multi-space owner's personal-space rules never categorise a business-space transaction. - Flush the offline cursor before the rows on space switch to avoid a stale re-sync. Adds member-access tests. --- app/Http/Controllers/SpaceController.php | 8 ++- .../Concerns/ValidatesUserOwnedResources.php | 11 +++- app/Policies/AccountPolicy.php | 2 +- app/Policies/BudgetPolicy.php | 19 ++---- .../Concerns/HandlesUserOwnership.php | 5 +- app/Policies/RealEstateDetailPolicy.php | 18 +++--- app/Services/AutomationRuleService.php | 16 ++--- resources/js/lib/sync-manager.ts | 5 +- .../Feature/Spaces/SpaceMemberAccessTest.php | 58 +++++++++++++++++++ 9 files changed, 102 insertions(+), 40 deletions(-) create mode 100644 tests/Feature/Spaces/SpaceMemberAccessTest.php diff --git a/app/Http/Controllers/SpaceController.php b/app/Http/Controllers/SpaceController.php index e0517448..e5e3a37c 100644 --- a/app/Http/Controllers/SpaceController.php +++ b/app/Http/Controllers/SpaceController.php @@ -131,8 +131,8 @@ class SpaceController extends Controller } /** - * Remove a member from a space (owner only). Their pointer self-heals back to - * their personal space on their next request. + * Remove a member from a space (owner only), sending them back to their + * personal space so they never sit on a pointer to a space they've lost. */ public function removeMember(Request $request, Space $space, User $member): RedirectResponse { @@ -140,6 +140,10 @@ class SpaceController extends Controller $space->members()->detach($member->id); + if ($member->current_space_id === $space->id) { + $member->forceFill(['current_space_id' => $member->personalSpace->id])->save(); + } + return back()->with('success', __('Member removed.')); } diff --git a/app/Http/Requests/Concerns/ValidatesUserOwnedResources.php b/app/Http/Requests/Concerns/ValidatesUserOwnedResources.php index 16347000..2835e3f5 100644 --- a/app/Http/Requests/Concerns/ValidatesUserOwnedResources.php +++ b/app/Http/Requests/Concerns/ValidatesUserOwnedResources.php @@ -9,15 +9,20 @@ use Illuminate\Validation\Rules\Exists; trait ValidatesUserOwnedResources { /** - * Rule asserting the value references a record on the given table owned by the authenticated user. + * Rule asserting the value references a record on the given (space-owned) + * table in the user's active space. Scoping by space rather than user lets a + * member of a shared space reference that space's accounts, categories and + * labels — the whole point of collaboration — while still blocking any row + * outside the active space. */ protected function userOwned(string $table): Exists { - return Rule::exists($table, 'id')->where('user_id', $this->user()->id); + return Rule::exists($table, 'id')->where('space_id', $this->user()->activeSpace()->id); } /** - * Rule asserting the value references an account of the given type owned by the authenticated user. + * Rule asserting the value references an account of the given type in the + * user's active space. */ protected function userOwnedAccountOfType(AccountType $type): Exists { diff --git a/app/Policies/AccountPolicy.php b/app/Policies/AccountPolicy.php index 46fad420..7cc6cb1e 100644 --- a/app/Policies/AccountPolicy.php +++ b/app/Policies/AccountPolicy.php @@ -15,6 +15,6 @@ class AccountPolicy */ public function view(User $user, Account $account): bool { - return $user->id === $account->user_id; + return $this->userCanAccess($user, $account); } } diff --git a/app/Policies/BudgetPolicy.php b/app/Policies/BudgetPolicy.php index fb181589..fdb7098a 100644 --- a/app/Policies/BudgetPolicy.php +++ b/app/Policies/BudgetPolicy.php @@ -4,9 +4,12 @@ namespace App\Policies; use App\Models\Budget; use App\Models\User; +use App\Policies\Concerns\HandlesUserOwnership; class BudgetPolicy { + use HandlesUserOwnership; + public function viewAny(User $user): bool { return true; @@ -14,7 +17,7 @@ class BudgetPolicy public function view(User $user, Budget $budget): bool { - return $user->id === $budget->user_id; + return $this->userCanAccess($user, $budget); } public function create(User $user): bool @@ -22,23 +25,13 @@ class BudgetPolicy return true; } - public function update(User $user, Budget $budget): bool - { - return $user->id === $budget->user_id; - } - - public function delete(User $user, Budget $budget): bool - { - return $user->id === $budget->user_id; - } - public function restore(User $user, Budget $budget): bool { - return $user->id === $budget->user_id; + return $this->userCanAccess($user, $budget); } public function forceDelete(User $user, Budget $budget): bool { - return $user->id === $budget->user_id; + return $this->userCanAccess($user, $budget); } } diff --git a/app/Policies/Concerns/HandlesUserOwnership.php b/app/Policies/Concerns/HandlesUserOwnership.php index 805a32b6..ab2621cf 100644 --- a/app/Policies/Concerns/HandlesUserOwnership.php +++ b/app/Policies/Concerns/HandlesUserOwnership.php @@ -60,8 +60,9 @@ trait HandlesUserOwnership return $user->id === $model->getAttribute('user_id'); } - return $user->current_space_id === $spaceId - || $user->accessibleSpaces()->contains('id', $spaceId); + // Authorize purely on live membership — never on current_space_id, which + // can still point at a space the user was just removed from. + return $user->accessibleSpaces()->contains('id', $spaceId); } /** diff --git a/app/Policies/RealEstateDetailPolicy.php b/app/Policies/RealEstateDetailPolicy.php index 59af7a14..47aec5ec 100644 --- a/app/Policies/RealEstateDetailPolicy.php +++ b/app/Policies/RealEstateDetailPolicy.php @@ -4,30 +4,28 @@ namespace App\Policies; use App\Models\RealEstateDetail; use App\Models\User; +use App\Policies\Concerns\HandlesUserOwnership; class RealEstateDetailPolicy { + use HandlesUserOwnership; + /** - * Determine whether the user can view the model. + * A real-estate detail has no space of its own — it inherits its account's, + * so access follows membership of the account's space. */ public function view(User $user, RealEstateDetail $realEstateDetail): bool { - return $user->id === $realEstateDetail->account->user_id; + return $this->userCanAccess($user, $realEstateDetail->account); } - /** - * Determine whether the user can update the model. - */ public function update(User $user, RealEstateDetail $realEstateDetail): bool { - return $user->id === $realEstateDetail->account->user_id; + return $this->userCanAccess($user, $realEstateDetail->account); } - /** - * Determine whether the user can delete the model. - */ public function delete(User $user, RealEstateDetail $realEstateDetail): bool { - return $user->id === $realEstateDetail->account->user_id; + return $this->userCanAccess($user, $realEstateDetail->account); } } diff --git a/app/Services/AutomationRuleService.php b/app/Services/AutomationRuleService.php index 9a31d76e..4d403cbb 100644 --- a/app/Services/AutomationRuleService.php +++ b/app/Services/AutomationRuleService.php @@ -14,24 +14,24 @@ use JWadhams\JsonLogic; class AutomationRuleService { /** - * Per-user rule cache, memoized for the lifetime of this service instance. + * Per-space rule cache, memoized for the lifetime of this service instance. * * Bulk re-evaluation calls applyRules() once per transaction; without this - * every transaction re-queried the user's full rule set (an N+1). A service + * every transaction re-queried the space's full rule set (an N+1). A service * instance is short-lived (resolved fresh per job/request), so rules created * after it is built are never seen mid-run — which is the intended behaviour. * * @var array> */ - private array $rulesByUser = []; + private array $rulesBySpace = []; public function applyRules(Transaction $transaction): void { - if ($transaction->description_iv !== null) { + if ($transaction->description_iv !== null || $transaction->space_id === null) { return; } - $rules = $this->rulesForUser($transaction->user_id); + $rules = $this->rulesForSpace($transaction->space_id); if ($rules->isEmpty()) { return; @@ -250,10 +250,10 @@ class AutomationRuleService /** * @return EloquentCollection */ - private function rulesForUser(string $userId): EloquentCollection + private function rulesForSpace(string $spaceId): EloquentCollection { - return $this->rulesByUser[$userId] ??= AutomationRule::query() - ->where('user_id', $userId) + return $this->rulesBySpace[$spaceId] ??= AutomationRule::query() + ->where('space_id', $spaceId) ->with('labels') ->orderBy('priority') ->get(); diff --git a/resources/js/lib/sync-manager.ts b/resources/js/lib/sync-manager.ts index 15eb7d97..2d418f40 100644 --- a/resources/js/lib/sync-manager.ts +++ b/resources/js/lib/sync-manager.ts @@ -148,7 +148,10 @@ export class TransactionSyncManager { } async clearAll(): Promise { - await db.transactions.clear(); + // Drop the cursor before the rows so a sync racing this flush (e.g. right + // after a space switch) reads no cursor and re-pulls the full set, rather + // than resuming from a stale cursor and missing the new space's history. await db.sync_metadata.delete(LAST_SYNC_KEY); + await db.transactions.clear(); } } diff --git a/tests/Feature/Spaces/SpaceMemberAccessTest.php b/tests/Feature/Spaces/SpaceMemberAccessTest.php new file mode 100644 index 00000000..d8d03058 --- /dev/null +++ b/tests/Feature/Spaces/SpaceMemberAccessTest.php @@ -0,0 +1,58 @@ +onboarded()->create(); + $space = $owner->ownedSpaces()->create(['name' => 'Acme', 'personal' => false]); + + $member = User::factory()->onboarded()->create(); + $space->members()->attach($member->id, ['role' => 'member']); + $member->forceFill(['current_space_id' => $space->id])->save(); + + return [$owner, $space, $member]; +} + +it('lets a member open an owner-owned account detail page', function () { + [$owner, $space, $member] = memberOf(); + $account = Account::factory()->for($owner)->create(['space_id' => $space->id]); + + $this->actingAs($member)->get("/accounts/{$account->id}") + ->assertOk() + ->assertInertia(fn (AssertableInertia $page) => $page->component('Accounts/Show')); +}); + +it('lets a member recategorize a transaction using the space\'s category', function () { + [$owner, $space, $member] = memberOf(); + $account = Account::factory()->for($owner)->create(['space_id' => $space->id]); + $transaction = Transaction::factory()->for($account)->create(['user_id' => $owner->id]); + $category = Category::factory()->for($owner)->create(['space_id' => $space->id]); + + // Exercises both the space-membership policy and the space-scoped ownership + // validation on category_id (which used to be user_id-scoped → 422 here). + $this->actingAs($member) + ->patchJson("/transactions/{$transaction->id}", ['category_id' => $category->id]) + ->assertOk(); + + expect($transaction->fresh()->category_id)->toBe($category->id); +}); + +it('denies a removed member access to the space\'s data', function () { + [$owner, $space, $member] = memberOf(); + $account = Account::factory()->for($owner)->create(['space_id' => $space->id]); + + $space->members()->detach($member->id); + + $this->actingAs($member)->get("/accounts/{$account->id}") + ->assertForbidden(); +});