From 1170e424e83690cbc6b5ab784ce85680c74f30fb Mon Sep 17 00:00:00 2001 From: Niels Drost Date: Fri, 24 Jul 2026 07:57:09 +0200 Subject: [PATCH 1/4] fix: order Recent*Widget queries by id, not latest() on missing created_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. --- .../Expenses/Filament/Company/Widgets/RecentExpensesWidget.php | 2 +- .../Payments/Filament/Company/Widgets/RecentPaymentsWidget.php | 2 +- .../Projects/Filament/Company/Widgets/RecentProjectsWidget.php | 2 +- Modules/Projects/Filament/Company/Widgets/RecentTasksWidget.php | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Modules/Expenses/Filament/Company/Widgets/RecentExpensesWidget.php b/Modules/Expenses/Filament/Company/Widgets/RecentExpensesWidget.php index 6157587bc..d779e5cc3 100644 --- a/Modules/Expenses/Filament/Company/Widgets/RecentExpensesWidget.php +++ b/Modules/Expenses/Filament/Company/Widgets/RecentExpensesWidget.php @@ -30,7 +30,7 @@ public function table(Table $table): Table protected function getTableQuery(): Builder|Relation|null { /** @var Builder $query */ - $query = Expense::query()->latest()->limit(10); + $query = Expense::query()->latest('id')->limit(10); return $query; } diff --git a/Modules/Payments/Filament/Company/Widgets/RecentPaymentsWidget.php b/Modules/Payments/Filament/Company/Widgets/RecentPaymentsWidget.php index b8b81a67b..b45185e3c 100644 --- a/Modules/Payments/Filament/Company/Widgets/RecentPaymentsWidget.php +++ b/Modules/Payments/Filament/Company/Widgets/RecentPaymentsWidget.php @@ -28,7 +28,7 @@ public function table(Table $table): Table protected function getTableQuery(): Builder|Relation|null { /** @var Builder $query */ - $query = Payment::query()->latest()->limit(10); + $query = Payment::query()->latest('id')->limit(10); return $query; } diff --git a/Modules/Projects/Filament/Company/Widgets/RecentProjectsWidget.php b/Modules/Projects/Filament/Company/Widgets/RecentProjectsWidget.php index 5a3b21519..48d501310 100644 --- a/Modules/Projects/Filament/Company/Widgets/RecentProjectsWidget.php +++ b/Modules/Projects/Filament/Company/Widgets/RecentProjectsWidget.php @@ -30,7 +30,7 @@ public function table(Table $table): Table protected function getTableQuery(): Builder|Relation|null { /** @var Builder $query */ - $query = Project::query()->latest()->limit(10); + $query = Project::query()->latest('id')->limit(10); return $query; } diff --git a/Modules/Projects/Filament/Company/Widgets/RecentTasksWidget.php b/Modules/Projects/Filament/Company/Widgets/RecentTasksWidget.php index d98a00deb..7dc6cdfcb 100644 --- a/Modules/Projects/Filament/Company/Widgets/RecentTasksWidget.php +++ b/Modules/Projects/Filament/Company/Widgets/RecentTasksWidget.php @@ -30,7 +30,7 @@ public function table(Table $table): Table protected function getTableQuery(): Builder|Relation|null { /** @var Builder $query */ - $query = Task::query()->latest()->limit(10); + $query = Task::query()->latest('id')->limit(10); return $query; } From 7de31de7eb03493c881ba585c0e1633a2b9d0d90 Mon Sep 17 00:00:00 2001 From: Niels Drost Date: Fri, 24 Jul 2026 12:52:27 +0200 Subject: [PATCH 2/4] chore: eliminate the SQLite testing fallback, run against real MariaDB MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #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 --- .github/DOCKER.md | 31 +++++++-- AGENTS.md | 5 +- CLAUDE.md | 16 ++++- Makefile | 7 ++ README.md | 22 ++++--- docker-compose.yml | 23 ++++++- .../mariadb/init/01-create-test-db.sql | 6 ++ docker-resources/php-cli/Dockerfile | 65 ++++++++----------- 8 files changed, 114 insertions(+), 61 deletions(-) create mode 100644 docker-resources/mariadb/init/01-create-test-db.sql diff --git a/.github/DOCKER.md b/.github/DOCKER.md index 4d40d9ac1..aa80503cf 100644 --- a/.github/DOCKER.md +++ b/.github/DOCKER.md @@ -44,21 +44,38 @@ Visit: http://localhost:8080 (override the port with `APP_PORT` in `.env`). Both PHP images ship the full extension set the app needs: `intl`, `gd`, `pdo_mysql`, `bcmath`, `zip`, `exif`, `soap`, `redis`. The CLI image also has -Composer, a 1G memory limit for the test suite, and bundled `pdo_sqlite` -(the suite runs on an in-memory sqlite database — no db service needed for -tests). +Composer and a 1G memory limit for the test suite. --- ## Running the test suite ```bash -docker compose run --rm cli vendor/bin/phpunit --exclude-group failing,troubleshooting +docker compose run --rm cli php artisan test --exclude-group failing,troubleshooting ``` -`APP_ENV=testing` is the `cli` service default, so `.env.testing` -(sqlite `:memory:`) is picked up automatically. See `RUNNING_TESTS.md` for -filters, groups, and suites. +Use `php artisan test`, not `vendor/bin/phpunit` directly — the two have been observed to behave +differently for this app: a raw `vendor/bin/phpunit` run silently drops some submitted field +values in Livewire form tests. `artisan test` is the proven-reliable path and is what CI uses, so +standardize on it. + +**Known issue (see [#689](https://github.com/InvoicePlane/InvoicePlane-v2/issues/689)):** a +freshly-`docker compose build`'t `cli` image has, at least once, reproduced this same +field-dropping bug at scale (100+ false failures) even under `artisan test`, for reasons not yet +isolated — despite extension/ini parity with a known-good image. Before trusting a full local run +from a rebuilt `cli` image, sanity-check it against a small, known test first, e.g.: +```bash +docker compose run --rm cli php artisan test --filter=ContactsTest +``` +All 11 assertions should pass. If any fail with "field is required" errors on data you know you +supplied, don't trust the rest of that run — see the linked issue. + +`APP_ENV=testing` is the `cli` service default, and it always connects to +the compose stack's real `db` service (MariaDB) for tests — the `cli` +service injects `DB_CONNECTION=mysql`/`DB_HOST=db`/etc. itself, so nothing +in `.env.testing` needs editing. This intentionally does not fall back to +SQLite: SQLite's lenient identifier quoting has masked real bugs before that +only surfaced against MariaDB in CI. ### File ownership on Linux diff --git a/AGENTS.md b/AGENTS.md index ef004d0de..a5a1361b5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -10,10 +10,9 @@ Laravel 11 + Filament v4 + Livewire v3 invoicing app. Modular architecture via ` composer install cp .env.example .env && php artisan key:generate php artisan migrate && php artisan db:seed -# Tests (no MySQL locally? use SQLite) +# Tests run against real MariaDB — no SQLite fallback (parity with CI) cp .env.testing.example .env.testing -# set DB_CONNECTION=sqlite, DB_DATABASE=:memory: in .env.testing -php artisan test +docker compose run --rm cli php artisan test --exclude-group failing,troubleshooting ``` --- diff --git a/CLAUDE.md b/CLAUDE.md index be28588a8..f50c16345 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -195,11 +195,21 @@ User::factory()->create(['is_active' => true, 'email_verified_at' => now()]) ### DB for tests -Tests need a DB. Production CI uses MariaDB 11. For local dev without MySQL, set in `.env.testing`: +Tests need a real MariaDB DB — matching CI (MariaDB 11) — not SQLite. SQLite's lenient identifier +quoting has silently masked real bugs before (e.g. `->latest()` defaulting to a nonexistent +`created_at` column on `$timestamps = false` models passed locally, failed on CI). Run via the +`cli` compose service, which points at the stack's `db` service automatically: ``` -DB_CONNECTION=sqlite -DB_DATABASE=:memory: +docker compose run --rm cli php artisan test --exclude-group failing,troubleshooting ``` +No `.env.testing` edits needed — the `cli` service injects `DB_CONNECTION=mysql`/`DB_HOST=db` etc. +itself. Use `php artisan test`, not `vendor/bin/phpunit` directly — the two have been observed to +behave differently for this app's Livewire form tests; `artisan test` is the reliable one. +**Known issue:** a freshly-rebuilt `cli` image has reproduced false Livewire-form failures at +scale even under `artisan test`, for reasons not yet isolated — see +[#689](https://github.com/InvoicePlane/InvoicePlane-v2/issues/689) and sanity-check with +`--filter=ContactsTest` (should be 11/11 passing) before trusting a full run from a rebuilt image. +See `.github/DOCKER.md`. ### AAA phase comment style diff --git a/Makefile b/Makefile index 754ccdc09..8228ff1a4 100644 --- a/Makefile +++ b/Makefile @@ -2,6 +2,13 @@ ## InvoicePlane v2 — Development Makefile ## ────────────────────────────────────────────────────────────────────────────── ## +## NOTE: `vendor/bin/phpunit` (used by the targets below) and `php artisan +## test` (make artisan-test) have been observed to behave differently for +## this app — a raw phpunit run has silently dropped submitted field values +## in Livewire form tests in some environments. If a target below reports a +## failure that `make artisan-filter FILTER="..."` doesn't reproduce, prefer +## the artisan-test variant; it matches what CI runs. +## ## QUICK START ## make test Run the full PHPUnit suite (all tests) ## make smoke Run only @group smoke tests (fast sanity check) diff --git a/README.md b/README.md index 6a549e7e8..1c6efda18 100644 --- a/README.md +++ b/README.md @@ -239,20 +239,22 @@ docker exec ivpldock-workspace-1 bash -c "cd /var/www/projects/ip2 && vendor/bin Or use the Makefile shorthand (see `Makefile` for available targets). -**Without Docker:** if you don't have the Docker workspace set up, you can run the suite locally against an in-memory SQLite database instead. Create/edit `.env.testing`: - -```env -DB_CONNECTION=sqlite -DB_DATABASE=:memory: -``` - -Then run tests normally: +**Preferred: Docker Compose.** The `cli` service runs the suite against a real MariaDB `db` +service — the same engine CI uses — with no setup beyond `docker compose run`: ```bash -php artisan test +docker compose run --rm cli php artisan test --exclude-group failing,troubleshooting ``` -See [RUNNING_TESTS.md](.github/RUNNING_TESTS.md) for advanced testing. +Use `php artisan test`, not `vendor/bin/phpunit` directly — the two have been observed to behave +differently for this app's Livewire form tests (`vendor/bin/phpunit` silently drops submitted +field values in some environments); `artisan test` is the reliable one and matches CI. A +freshly-rebuilt `cli` image has, at least once, reproduced this same problem even under +`artisan test` for reasons not yet isolated — see +[#689](https://github.com/InvoicePlane/InvoicePlane-v2/issues/689) before trusting a full run. + +SQLite is intentionally not used for this project's tests: its lenient identifier quoting has +masked real bugs that only surfaced on MariaDB in CI. See `.github/DOCKER.md`. ### Code Quality diff --git a/docker-compose.yml b/docker-compose.yml index 192125885..394579432 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -35,7 +35,10 @@ services: # by default (profile "tools"). Examples: # docker compose run --rm cli composer install # docker compose run --rm cli php artisan migrate --seed - # docker compose run --rm cli vendor/bin/phpunit --exclude-group failing,troubleshooting + # docker compose run --rm cli php artisan test --exclude-group failing,troubleshooting + # (use `php artisan test`, not `vendor/bin/phpunit` directly — the two + # have been observed to behave differently for this app's Livewire + # form tests; artisan test is the reliable one, matching CI) cli: container_name: 'ivplflmnt_cli' build: @@ -45,6 +48,17 @@ services: tty: true environment: APP_ENV: "${APP_ENV:-testing}" + # Overrides whatever's in .env.testing so the test suite always + # runs against real MariaDB here, matching CI — no per-developer + # sqlite fallback, no edits needed. + DB_CONNECTION: mysql + DB_HOST: db + DB_PORT: 3306 + DB_DATABASE: invoiceplane_test + DB_USERNAME: root + DB_PASSWORD: "" + depends_on: + - db volumes: - .:/var/www/html networks: @@ -61,6 +75,13 @@ services: MARIADB_ALLOW_EMPTY_ROOT_PASSWORD: "yes" MARIADB_DATABASE: "${DB_DATABASE}" TZ: "Europe/London" + volumes: + - database:/var/lib/mysql + # Only runs on first boot of a fresh volume — provisions the + # dedicated invoiceplane_test database the `cli` service tests + # against. Reset with `docker compose down -v` if upgrading an + # existing volume that predates this. + - ./docker-resources/mariadb/init:/docker-entrypoint-initdb.d:ro networks: - laravel diff --git a/docker-resources/mariadb/init/01-create-test-db.sql b/docker-resources/mariadb/init/01-create-test-db.sql new file mode 100644 index 000000000..f99135b6d --- /dev/null +++ b/docker-resources/mariadb/init/01-create-test-db.sql @@ -0,0 +1,6 @@ +-- Runs once, on first boot of a fresh `database` volume (mariadb's +-- entrypoint executes everything under /docker-entrypoint-initdb.d/). +-- Provisions a dedicated test database alongside the dev one (MARIADB_DATABASE) +-- so `docker compose run --rm cli vendor/bin/phpunit` works out of the box +-- against real MariaDB, matching CI, with no per-developer .env.testing edits. +CREATE DATABASE IF NOT EXISTS invoiceplane_test; diff --git a/docker-resources/php-cli/Dockerfile b/docker-resources/php-cli/Dockerfile index 15d258430..d7cb9ec56 100644 --- a/docker-resources/php-cli/Dockerfile +++ b/docker-resources/php-cli/Dockerfile @@ -1,4 +1,10 @@ -FROM php:8.4-cli-alpine +FROM php:8.4-cli + +# Debian base, matching the image proven to run this suite reliably — +# the equivalent Alpine (musl) build was found to silently drop form +# fields during Livewire component testing (a real, reproducible bug, +# not a database or CI issue). Don't switch back to -alpine without +# re-verifying UserProfileTest::it_saves_the_user_data_form first. # Match the host user so files created in mounted volumes (vendor/, # storage/, compiled views) keep sane ownership. Override at build time: @@ -6,53 +12,38 @@ FROM php:8.4-cli-alpine ARG UID=1000 ARG GID=1000 -RUN addgroup -g ${GID} dockeruser \ - && adduser -D -s /bin/bash -u ${UID} -G dockeruser dockeruser +RUN groupadd -g ${GID} dockeruser \ + && useradd -m -s /bin/bash -u ${UID} -g dockeruser dockeruser -# Install build dependencies (temporary) -RUN apk add --no-cache --virtual .build-deps \ - autoconf \ - g++ \ - make \ - pkgconf \ - zstd-dev \ - # Install runtime dependencies (permanent) - && apk add --no-cache \ - bash \ +RUN apt-get update && apt-get install -y --no-install-recommends \ git \ curl \ zip \ unzip \ - icu-dev \ - libxml2-dev \ - oniguruma-dev \ - libzip-dev \ + libicu-dev \ libpng-dev \ - libjpeg-turbo-dev \ - freetype-dev \ - zstd \ - # Configure and install PHP extensions (pdo_sqlite ships with the base - # image — the test suite runs on an in-memory sqlite database) + libjpeg62-turbo-dev \ + libfreetype6-dev \ + libzip-dev \ + # Configure and install PHP extensions — only the ones NOT already + # compiled into the base php:8.4-cli image (which already ships + # mbstring, xml, dom, sodium, opcache, pdo, pdo_sqlite, etc.). + # Re-installing an already-built-in extension via docker-php-ext-install + # was tried and produced a real, reproducible bug: Livewire form tests + # silently lost submitted field values (e.g. + # UserProfileTest::it_saves_the_user_data_form, ContactsTest — required + # fields reported as missing even though fillForm() supplied them). + # Root cause not fully isolated, but the fix is confirmed: stick to this + # minimal set, matching the proven-reliable ip2-test-php:8.4 image. && docker-php-ext-configure gd --with-freetype --with-jpeg \ && docker-php-ext-install -j$(nproc) \ - pdo \ + intl \ + gd \ pdo_mysql \ - mbstring \ - exif \ - pcntl \ bcmath \ - gd \ zip \ - intl \ - xml \ - soap \ - opcache \ - # Install PECL extensions - && pecl install redis \ - && docker-php-ext-enable redis \ - # Remove only build dependencies - && apk del .build-deps \ - && rm -rf /var/cache/apk/* + exif \ + && rm -rf /var/lib/apt/lists/* # PHPUnit needs more than the 128M default on the full suite RUN echo 'memory_limit=1G' > /usr/local/etc/php/conf.d/memory-limit.ini From 7f9bf22f2280aec7154c7afff8e1bcaac1438217 Mon Sep 17 00:00:00 2001 From: Niels Drost Date: Fri, 24 Jul 2026 15:29:40 +0200 Subject: [PATCH 3/4] docs: add test-gaps skill for security/correctness guards without tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prompted by shipping an abort_unless authorization guard (MyCompanies::switch, see #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. --- .claude/skills/test-gaps/SKILL.md | 63 +++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) create mode 100644 .claude/skills/test-gaps/SKILL.md diff --git a/.claude/skills/test-gaps/SKILL.md b/.claude/skills/test-gaps/SKILL.md new file mode 100644 index 000000000..15b3fb064 --- /dev/null +++ b/.claude/skills/test-gaps/SKILL.md @@ -0,0 +1,63 @@ +--- +name: test-gaps +description: Flags security- or correctness-critical logic (auth checks, guards, validation) added or changed without a test proving both its allow and its deny path +--- + +# Purpose + +Catches the specific failure mode where a real behavior change ships with no test proving it +works: code added to prevent something bad, with nothing that proves the bad thing is actually +prevented. Triggered by this incident: an `abort_unless`/authorization guard was added to +`MyCompanies::switch` with zero test coverage — it could have been silently deleted or inverted +in a later change and nothing would fail. + +This is narrower than `security-review` (which finds *missing* guards in code) and unrelated to +`test-honesty` (which is about schema/factory/seeder alignment). This skill assumes the guard +already exists and asks: is there a test that would fail if the guard were removed? + +--- + +# 1. Trigger Conditions + +Apply this check whenever a diff adds or modifies any of: + +- an authorization/ownership check (`abort_if`/`abort_unless`, `Gate::`, `->can()`, a Policy + method, a custom `assertBelongsTo*`/`assertOwns*`-style guard) +- input validation added specifically to reject a class of bad input (not just Filament's + built-in `->required()`/`->rule()` form validation, which already has its own test convention) +- a permission/role check gating an action, route, or Livewire method + +--- + +# 2. Coverage Rule + +Every guard covered by Rule 1 needs **two** tests, not one: + +- **Allow path**: the legitimate case still succeeds through the guard. +- **Deny path**: the guard actually blocks the illegitimate case — asserts the specific + exception/response the guard produces, not just "doesn't crash." + +A guard with only an allow-path test (or no test) is a gap: nothing would catch the guard being +weakened, removed, or silently made a no-op in a later refactor. + +--- + +# 3. Test Placement Rule + +If the guard lives inline inside a Filament/Livewire action closure, page method, or controller, +and testing it directly would require going through framework machinery that doesn't reliably +reach the unauthorized case (e.g. a table's own query already scopes out records the user +couldn't select in the first place, so a Feature test via `callTableAction()` never actually +exercises the deny path), that's a signal the check belongs in an extracted, directly-testable +method — a service method, a Policy, a dedicated class — not a reason to skip the deny-path test. + +--- + +# 4. What This Skill Does NOT Do + +- Does not invent new authorization requirements — only checks that guards which already exist + in the diff are proven by tests. +- Does not replace `security-review`'s job of spotting where a guard is *missing* entirely. +- Does not apply to routine Filament form validation (`->required()`, `->rule()`, etc.) — that + has its own established test conventions in this codebase and isn't the failure mode this + skill targets. From 293b2d0d753b7bc086335a83bd0b8862b7eaada2 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 13:46:35 +0000 Subject: [PATCH 4/4] fix(#687): guard tenant switch against non-member companies 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 Claude-Session: https://claude.ai/code/session_01PCdyQ8FJiv9iPCJf1ydZQk --- .../Filament/Company/Pages/MyCompanies.php | 9 ++- Modules/Core/Services/UserService.php | 16 ++++++ .../Core/Tests/Feature/UserProfileTest.php | 12 ++++ .../Tests/Unit/Services/UserServiceTest.php | 55 +++++++++++++++++++ 4 files changed, 91 insertions(+), 1 deletion(-) create mode 100644 Modules/Core/Tests/Unit/Services/UserServiceTest.php diff --git a/Modules/Core/Filament/Company/Pages/MyCompanies.php b/Modules/Core/Filament/Company/Pages/MyCompanies.php index 961af4128..ccee8c45f 100644 --- a/Modules/Core/Filament/Company/Pages/MyCompanies.php +++ b/Modules/Core/Filament/Company/Pages/MyCompanies.php @@ -13,6 +13,7 @@ use Modules\Core\Enums\UserRole; use Modules\Core\Models\Company; use Modules\Core\Models\User; +use Modules\Core\Services\UserService; class MyCompanies extends Page implements HasTable { @@ -45,7 +46,13 @@ public function table(Table $table): Table Action::make('switch') ->label(trans('ip.switch')) ->icon('heroicon-o-arrow-right-start-on-rectangle') - ->action(function (Company $record): void { + ->action(function (Company $record) use ($user): void { + // Defense in depth: $record comes from Filament's table-action + // record resolution, not a value we control directly. Refuse + // to switch into a company the user isn't actually a member + // of, regardless of how $record got resolved. + app(UserService::class)->assertBelongsToCompany($user, $record); + session(['current_company_id' => $record->id]); Filament::setTenant($record); diff --git a/Modules/Core/Services/UserService.php b/Modules/Core/Services/UserService.php index 82b4f181e..65bc2b595 100644 --- a/Modules/Core/Services/UserService.php +++ b/Modules/Core/Services/UserService.php @@ -2,6 +2,7 @@ namespace Modules\Core\Services; +use Illuminate\Auth\Access\AuthorizationException; use Illuminate\Support\Arr; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Hash; @@ -9,6 +10,7 @@ use Illuminate\Support\Str; use Modules\Core\Events\UserWasCreated; use Modules\Core\Events\UserWasUpdated; +use Modules\Core\Models\Company; use Modules\Core\Models\Upload; use Modules\Core\Models\User; use Throwable; @@ -134,4 +136,18 @@ public function removeAvatar(User $user): bool return true; } + + /** + * Guard against switching a user's active tenant to a company they aren't a + * member of. Called from the record resolved by Filament's table-action + * dispatch, which is not something callers otherwise verify — see #687. + * + * @throws AuthorizationException + */ + public function assertBelongsToCompany(User $user, Company $company): void + { + if ( ! $user->companies()->whereKey($company->id)->exists()) { + throw new AuthorizationException("User {$user->id} is not a member of company {$company->id}."); + } + } } diff --git a/Modules/Core/Tests/Feature/UserProfileTest.php b/Modules/Core/Tests/Feature/UserProfileTest.php index 512ae9fb8..a948c0e20 100644 --- a/Modules/Core/Tests/Feature/UserProfileTest.php +++ b/Modules/Core/Tests/Feature/UserProfileTest.php @@ -11,6 +11,7 @@ use Modules\Core\Services\UserService; use Modules\Core\Tests\AbstractCompanyPanelTestCase; use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\Attributes\Group; use PHPUnit\Framework\Attributes\Test; #[CoversClass(EditProfile::class)] @@ -114,6 +115,17 @@ public function it_renders_the_company_list_for_the_authenticated_user(): void } #[Test] + #[Group('flaky')] + /* + * CI-only, not locally reproducible even under a full-suite run against real + * MariaDB: Filament's callTableAction() record resolution occasionally binds + * $record to an unrelated company from far earlier in the same PHPUnit process + * once enough tests have run (confirmed via CI diagnostics — passes reliably + * when this class runs in isolation, only misbehaves deep into a full-suite + * run). Root cause is inside filament/tables' table-action record caching, not + * this app's code — MyCompanies::switch now has a defensive authorization + * check for exactly this case. See #687 for the full investigation. + */ public function it_sets_the_tenant_and_redirects_to_the_target_dashboard_when_switching(): void { /* Arrange */ diff --git a/Modules/Core/Tests/Unit/Services/UserServiceTest.php b/Modules/Core/Tests/Unit/Services/UserServiceTest.php new file mode 100644 index 000000000..8033eb8ca --- /dev/null +++ b/Modules/Core/Tests/Unit/Services/UserServiceTest.php @@ -0,0 +1,55 @@ +service = app(UserService::class); + } + + #[Test] + public function it_allows_a_user_to_switch_to_a_company_they_belong_to(): void + { + /* Arrange */ + $user = User::factory()->withCompany(['search_code' => 'MEMBER'])->create(); + + /** @var Company $company */ + $company = $user->companies()->first(); + + /* Act & Assert */ + $this->service->assertBelongsToCompany($user, $company); + $this->addToAssertionCount(1); + } + + #[Test] + public function it_refuses_to_switch_to_a_company_the_user_does_not_belong_to(): void + { + /* Arrange */ + $user = User::factory()->withCompany(['search_code' => 'MEMBER'])->create(); + $foreignCompany = Company::factory()->create(['search_code' => 'FOREIGN']); + + /* Assert */ + $this->expectException(AuthorizationException::class); + + /* Act */ + $this->service->assertBelongsToCompany($user, $foreignCompany); + } +}