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.
This commit is contained in:
parent
7df967c487
commit
dd43b88fff
|
|
@ -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.'));
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
{
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<string, EloquentCollection<int, AutomationRule>>
|
||||
*/
|
||||
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<int, AutomationRule>
|
||||
*/
|
||||
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();
|
||||
|
|
|
|||
|
|
@ -148,7 +148,10 @@ export class TransactionSyncManager {
|
|||
}
|
||||
|
||||
async clearAll(): Promise<void> {
|
||||
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();
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -0,0 +1,58 @@
|
|||
<?php
|
||||
|
||||
use App\Models\Account;
|
||||
use App\Models\Category;
|
||||
use App\Models\Transaction;
|
||||
use App\Models\User;
|
||||
use Inertia\Testing\AssertableInertia;
|
||||
|
||||
/**
|
||||
* A member of a shared space must be able to actually work in it — view detail
|
||||
* pages and edit data whose user_id is the owner's — and must lose that access
|
||||
* the moment they're removed.
|
||||
*/
|
||||
function memberOf(): array
|
||||
{
|
||||
$owner = User::factory()->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();
|
||||
});
|
||||
Loading…
Reference in New Issue