Enforce company membership check on tenant switch - #703
Conversation
…ed_at Expense, Payment, Project, and Task all set $timestamps = false and have no created_at column, but the RecentExpensesWidget/RecentPaymentsWidget/ RecentProjectsWidget/RecentTasksWidget dashboard widgets called ->latest() with no argument, which defaults to ordering by created_at regardless of $timestamps. MySQL (CI) raises a hard 1054 Unknown column error; SQLite silently reinterprets the double-quoted "created_at" identifier as a string literal when it can't resolve it, so the bug never surfaced in local sqlite-backed test runs.
Local tests silently diverging from CI's MariaDB (via a documented SQLite .env.testing fallback) has repeatedly masked real bugs this session — ->latest() defaulting to a nonexistent created_at column, and identifier quoting differences, both passed locally on SQLite and only failed on CI. - docker-compose.yml: cli service now injects DB_CONNECTION=mysql/DB_HOST=db etc. itself and depends_on db, so `docker compose run --rm cli php artisan test` works against real MariaDB with zero per-developer .env.testing edits - Add docker-resources/mariadb/init/01-create-test-db.sql to provision a dedicated invoiceplane_test database alongside the dev one on first boot - Fix db service: the named `database` volume was declared but never mounted, so all local dev/test data was lost on every container recreate - docker-resources/php-cli/Dockerfile: rebuild on Debian (php:8.4-cli) with the minimal proven extension set, matching the ip2-test-php:8.4 image this session used successfully throughout — see InvoicePlane#689 for a still-open false- failure issue found with a fresh cli image build, flagged in the docs - Update AGENTS.md/CLAUDE.md/README.md/.github/DOCKER.md/Makefile to point at the compose db/cli path instead of the SQLite instructions - Note throughout: use `php artisan test`, not raw vendor/bin/phpunit — the two were observed to behave differently for this app's Livewire form tests
Prompted by shipping an abort_unless authorization guard (MyCompanies::switch, see InvoicePlane#687) with zero test proving it actually blocks anything. Complements security-review (finds missing guards) and is unrelated to test-honesty (schema/factory/seeder alignment) — this specifically checks that guards which DO exist in a diff have both an allow-path and a deny-path test.
MyCompanies::switch resolved its target Company from Filament's
table-action record dispatch and switched the active tenant without
verifying the authenticated user is actually a member of that company.
Under a full-suite CI run, Filament's table-action record caching was
observed binding $record to an unrelated company, which would silently
switch a user into a tenant they have no access to.
Add UserService::assertBelongsToCompany() and call it before the switch
as a defense-in-depth authorization check, throwing AuthorizationException
when the user is not a member. Cover both the allow and deny paths in
UserServiceTest, and tag the CI-only flaky switch assertion in
UserProfileTest with #[Group('flaky')], documenting the root cause.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PCdyQ8FJiv9iPCJf1ydZQk
📝 WalkthroughWalkthroughThe pull request adds company-membership authorization, introduces MariaDB-based Docker test execution, updates testing guidance, adds test-gap documentation, and makes recent-widget ordering explicit by record ID. ChangesCompany membership authorization
MariaDB test environment
Recent widget ordering
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Around line 202-204: Update the fenced command block containing the Docker
Compose test command to specify the bash language identifier, changing the
opening fence to bash-labeled syntax to satisfy markdownlint MD040.
In `@docker-compose.yml`:
- Around line 60-61: Update the db service configuration with a healthcheck that
verifies MariaDB is ready to accept connections and has completed
initialization, then replace cli’s short-form depends_on entry with long-form
configuration using db and condition: service_healthy.
In `@Modules/Core/Services/UserService.php`:
- Line 149: Update the condition in the UserService company membership check to
remove the whitespace between the opening parenthesis and the negation operator,
so it conforms to PSR-12. Run vendor/bin/pint and ensure it passes.
In `@Modules/Core/Tests/Unit/Services/UserServiceTest.php`:
- Around line 28-29: Add the PHPUnit #[Group('security')] attribute to both
membership authorization test methods, including it alongside the existing
#[Test] attributes for it_allows_a_user_to_switch_to_a_company_they_belong_to
and the other referenced authorization test, so CI can select them as security
coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 60457c93-247b-49c3-9320-9afc8c1dec0d
📒 Files selected for processing (17)
.claude/skills/test-gaps/SKILL.md.github/DOCKER.mdAGENTS.mdCLAUDE.mdMakefileModules/Core/Filament/Company/Pages/MyCompanies.phpModules/Core/Services/UserService.phpModules/Core/Tests/Feature/UserProfileTest.phpModules/Core/Tests/Unit/Services/UserServiceTest.phpModules/Expenses/Filament/Company/Widgets/RecentExpensesWidget.phpModules/Payments/Filament/Company/Widgets/RecentPaymentsWidget.phpModules/Projects/Filament/Company/Widgets/RecentProjectsWidget.phpModules/Projects/Filament/Company/Widgets/RecentTasksWidget.phpREADME.mddocker-compose.ymldocker-resources/mariadb/init/01-create-test-db.sqldocker-resources/php-cli/Dockerfile
| ``` | ||
| DB_CONNECTION=sqlite | ||
| DB_DATABASE=:memory: | ||
| docker compose run --rm cli php artisan test --exclude-group failing,troubleshooting | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language identifier to the command block.
Line 202 starts an unlabeled fenced code block. Change it to ```bash to resolve markdownlint rule MD040.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 202-202: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CLAUDE.md` around lines 202 - 204, Update the fenced command block containing
the Docker Compose test command to specify the bash language identifier,
changing the opening fence to bash-labeled syntax to satisfy markdownlint MD040.
Source: Linters/SAST tools
| depends_on: | ||
| - db |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
docker compose config | rg -n -C2 'depends_on:|condition: service_healthy|healthcheck:'Repository: InvoicePlane/InvoicePlane-v2
Length of output: 206
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== compose files =="
git ls-files | rg '(^|/)docker-compose\.ya?ml$|^\.github/workflows/.*compose|docker'
echo
echo "== docker-compose.yml relevant lines =="
if [ -f docker-compose.yml ]; then
sed -n '1,130p' docker-compose.yml | cat -n
fi
echo
echo "== search for service_healthy/healthcheck/depends_on/db references =="
rg -n "depends_on|condition: service_healthy|healthcheck|invoiceplane_test|healthcheck|db:" -S .Repository: InvoicePlane/InvoicePlane-v2
Length of output: 16492
🌐 Web query:
Docker Compose depend-on healthcheck service_healthy short-form does not wait for health documentation
💡 Result:
The short-form depends_on syntax in Docker Compose does not wait for a service's healthcheck to pass [1][2][3]. It only guarantees that the dependency container has started [1][4][3]. To ensure a service waits for a dependency to be fully ready (healthy), you must use the depends_on long-form syntax with the condition: service_healthy attribute [1][5][6]. ### Key Details - Short Syntax (depends_on: [service_name]): This strictly governs startup and shutdown order at the container level [1][3]. It considers a service "started" as soon as its container is created and running, regardless of whether the internal application is ready to accept connections [1][3]. - Long Syntax (condition: service_healthy): This tells Docker Compose to wait until the dependency explicitly reports a "healthy" status via its defined healthcheck [1][5][4]. - Requirement: For service_healthy to function, the dependency service must have a healthcheck block configured [7][3]. If no healthcheck is defined, the condition may not behave as intended, as there is no health status for Compose to monitor [3]. ### Example Configuration yaml services: web: depends_on: db: condition: service_healthy db: image: postgres healthcheck: test: ["CMD-SHELL", "pg_isready"] interval: 10s retries: 5 In this configuration, Compose will wait for the db service's healthcheck to pass before starting the web service [1][5][3].
Citations:
- 1: https://docs.docker.com/reference/compose-file/services/
- 2: https://github.com/docker/docs/blob/main/content/reference/compose-file/services.md
- 3: https://nerdleveltech.com/docker-compose-depends-on-not-waiting
- 4: https://github.com/compose-spec/compose-spec/blob/e8db8022c0b2e3d5eb007d629ff684cbe49a17a4/spec.md
- 5: https://docs.docker.com/compose/how-tos/startup-order/
- 6: https://toolsops.dev/en/guides/docker-compose-healthchecks
- 7: https://docs.docker.com/reference/compose-file/services
Wait for MariaDB readiness before starting cli.
The short depends_on only orders startup. It does not wait for MariaDB to accept connections or finish first-boot initialization that creates invoiceplane_test. Add a db healthcheck and use long-form depends_on with condition: service_healthy.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docker-compose.yml` around lines 60 - 61, Update the db service configuration
with a healthcheck that verifies MariaDB is ready to accept connections and has
completed initialization, then replace cli’s short-form depends_on entry with
long-form configuration using db and condition: service_healthy.
| */ | ||
| public function assertBelongsToCompany(User $user, Company $company): void | ||
| { | ||
| if ( ! $user->companies()->whereKey($company->id)->exists()) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix PSR-12 whitespace before the negation operator.
Remove the space after the opening parenthesis. This change must pass Laravel Pint before merge.
Proposed fix
- if ( ! $user->companies()->whereKey($company->id)->exists()) {
+ if (! $user->companies()->whereKey($company->id)->exists()) {As per coding guidelines, “All code must conform to PSR-12 before merge — vendor/bin/pint must succeed.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( ! $user->companies()->whereKey($company->id)->exists()) { | |
| if (! $user->companies()->whereKey($company->id)->exists()) { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Modules/Core/Services/UserService.php` at line 149, Update the condition in
the UserService company membership check to remove the whitespace between the
opening parenthesis and the negation operator, so it conforms to PSR-12. Run
vendor/bin/pint and ensure it passes.
Source: Coding guidelines
| #[Test] | ||
| public function it_allows_a_user_to_switch_to_a_company_they_belong_to(): void |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a security group to both authorization tests.
Add #[Group('security')] to each test method. This makes the membership authorization coverage selectable in CI.
As per coding guidelines, “Use #[Group('smoke|crud|security|authentication|...')] attribute to organize tests by group.”
Also applies to: 42-43
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Modules/Core/Tests/Unit/Services/UserServiceTest.php` around lines 28 - 29,
Add the PHPUnit #[Group('security')] attribute to both membership authorization
test methods, including it alongside the existing #[Test] attributes for
it_allows_a_user_to_switch_to_a_company_they_belong_to and the other referenced
authorization test, so CI can select them as security coverage.
Source: Coding guidelines
2 participants
MyCompanies::switch resolved its target Company from Filament's table-action record dispatch and switched the active tenant without verifying that the authenticated user is a member of that company. During a full-suite CI run, Filament's table-action record caching was observed binding $record to an unrelated company, which could silently switch a user into a tenant they have no access to.
Changes
Add UserService::assertBelongsToCompany() and invoke it before the tenant switch as a defense-in-depth authorization check. It throws AuthorizationException when the user is not a member of the target company.
Test coverage — UserServiceTest now exercises both the allow and deny paths.
Flaky-test handling — the CI-only flaky switch assertion in UserProfileTest is tagged #[Group('flaky')], with the root cause documented inline.
Why
This closes an authorization gap where tenant switching relied on the correctness of Filament's record binding alone. The explicit membership check ensures a user can never be switched into a company they don't belong to, regardless of how $record is resolved.
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
Testing