diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml deleted file mode 100644 index fd65da1..0000000 --- a/.github/workflows/claude-code-review.yml +++ /dev/null @@ -1,57 +0,0 @@ -name: Claude Code Review - -on: - pull_request: - types: [opened, synchronize] - # Optional: Only run on specific file changes - # paths: - # - "src/**/*.ts" - # - "src/**/*.tsx" - # - "src/**/*.js" - # - "src/**/*.jsx" - -jobs: - claude-review: - # Optional: Filter by PR author - # if: | - # github.event.pull_request.user.login == 'external-contributor' || - # github.event.pull_request.user.login == 'new-developer' || - # github.event.pull_request.author_association == 'FIRST_TIME_CONTRIBUTOR' - - runs-on: ubuntu-latest - permissions: - contents: read - pull-requests: read - issues: read - id-token: write - - steps: - - name: Checkout repository - uses: actions/checkout@v6 - with: - fetch-depth: 1 - - - name: Run Claude Code Review - id: claude-review - uses: anthropics/claude-code-action@v1 - with: - claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} - prompt: | - REPO: ${{ github.repository }} - PR NUMBER: ${{ github.event.pull_request.number }} - - Please review this pull request and provide feedback on: - - Code quality and best practices - - Potential bugs or issues - - Performance considerations - - Security concerns - - Test coverage - - Use the repository's CLAUDE.md for guidance on style and conventions. Be constructive and helpful in your feedback. - - Use `gh pr comment` with your Bash tool to leave your review as a comment on the PR. - - # See https://github.com/anthropics/claude-code-action/blob/main/docs/usage.md - # or https://code.claude.com/docs/en/cli-reference for available options - claude_args: '--allowed-tools "Bash(gh issue view:*),Bash(gh search:*),Bash(gh issue list:*),Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Bash(gh pr list:*)"' - diff --git a/README.md b/README.md index 7066db7..fdd529c 100644 --- a/README.md +++ b/README.md @@ -19,6 +19,7 @@ Organization Based (OrBAC) , Attibute Based (ABAC) , Rol-Permission (RBAC) Base - Built-in Blade Directives for permission control inside **Blade** files - Mysql, MariaDB, Postgres Support - Built-in Caching Layer with Configurable TTL +- **Laravel Octane / Vapor safe** — `aauth` is a request-scoped binding; no cross-request state leak - Community Driven and Open Source Forever --- @@ -65,6 +66,12 @@ Optionally, You can seed the sample data with: php artisan db:seed --class=SampleDataSeeder ``` +## Laravel Octane / Vapor + +AAuth resolves the `aauth` service as a **request-scoped** binding (`$app->scoped()`). Under Octane and Vapor the resolved instance is flushed at every `RequestTerminated` event, so the active user, role, organization node IDs, permissions and ABAC rules cannot leak across requests sharing the same long-lived worker. No `config/octane.php` flush entry is needed. + +Under classic PHP-FPM behaviour is unchanged. + You can publish the config file with: ```bash diff --git a/UPGRADE.md b/UPGRADE.md index 709e20f..aa3ad1d 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -1,5 +1,61 @@ # Upgrade Guide +## Upgrading to 21.1.0 (security + reliability minor) + +This release is fully backward compatible — `composer update` from any 21.x will not break a working host application. No migration, no config change, no signature change. + +### What changed + +**1. AAuth container binding is now request-scoped (security fix, HIGH severity)** + +The `aauth` binding in `AAuthServiceProvider` was a `singleton` and now uses `scoped()`. Under classic PHP-FPM each request is a fresh process, so this is a no-op (same number of constructor calls per request, same DB queries, same observable behaviour). + +Under **Laravel Octane / Vapor** the change closes an authorization-bypass vector: the resolved `AAuth` instance no longer survives the request boundary, so User A's role / organization node IDs / permissions / ABAC rules / super-admin flag cannot leak into User B's request on the same worker. + +- **Action required for PHP-FPM consumers:** none. +- **Action required for Octane / Vapor consumers:** none. If you previously worked around this with `config/octane.php`'s `'flush' => ['aauth']` entry, you can remove it (it's harmless if you leave it). + +**2. Recursive organization-node service methods now propagate exceptions** + +`OrganizationService::updateNodePathsRecursively()` and `deleteOrganizationNodesRecursively()` previously wrapped their body in a `try/catch` that silently swallowed exceptions while still committing the outer transaction. The new implementation wraps the recursion in `DB::transaction()` so any failure rolls back the entire subtree and re-throws the original exception. + +- The public method signatures (`OrganizationNode $node, ?bool $withDBTransaction = true`) are unchanged. +- The only observable behavioural difference: errors that were previously silent now surface to the caller. If you relied on the silent-failure behaviour, wrap the call in your own `try/catch`. + +**3. New aligned-order `detachOrganizationRoleFromUserBy()` method** + +`RolePermissionService::detachOrganizationRoleFromUser()` historically takes parameters in the inverse order from `attachOrganizationRoleToUser()`. We did not change the existing method's parameter order — that would silently corrupt data for every consumer. Instead we added: + +```php +$service->detachOrganizationRoleFromUserBy( + $organizationNodeId, + $roleId, + $userId, +); // matches attachOrganizationRoleToUser order +``` + +The existing `detachOrganizationRoleFromUser($userId, $roleId, $organizationNodeId)` keeps working and emits no runtime notice. It is marked `@deprecated` in its docblock so IDEs and PHPStan flag call sites for migration. It will be removed in the next major release. + +- **Migration recipe:** + ```bash + # Find call sites + grep -rn 'detachOrganizationRoleFromUser(' app/ packages/ + ``` + Replace `detachOrganizationRoleFromUser($u, $r, $n)` with `detachOrganizationRoleFromUserBy($n, $r, $u)`. + +### Verifying the upgrade + +After `composer update`: + +```bash +vendor/bin/pest # your own test suite should still pass +vendor/bin/phpstan # may surface @deprecated notices for the legacy detach method — informational only +``` + +--- + +## Upgrading from v1 to v2 + This guide covers upgrading from AAuth v1 to v2. ## Requirements diff --git a/openspec/changes/archive/2026-05-22-fix-critical-auth-bugs/.openspec.yaml b/openspec/changes/archive/2026-05-22-fix-critical-auth-bugs/.openspec.yaml new file mode 100644 index 0000000..4a1c677 --- /dev/null +++ b/openspec/changes/archive/2026-05-22-fix-critical-auth-bugs/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-05-22 diff --git a/openspec/changes/archive/2026-05-22-fix-critical-auth-bugs/design.md b/openspec/changes/archive/2026-05-22-fix-critical-auth-bugs/design.md new file mode 100644 index 0000000..1ddd041 --- /dev/null +++ b/openspec/changes/archive/2026-05-22-fix-critical-auth-bugs/design.md @@ -0,0 +1,180 @@ +## Context + +AAuth is a Laravel package that resolves a request-scoped authorization service (`aauth`) carrying the active user, role, organization node IDs, permissions and ABAC rules. It is registered in `AAuthServiceProvider::boot()` as a singleton and the resolved instance is consulted by: + +- `Gate::before` → which delegates `$user->can()` and Blade `@can` to AAuth +- Three middlewares (`aauth.permission`, `aauth.role`, `aauth.organization`) +- Blade directives (`@aauth`, `@aauth_can`, `@aauth_role`, `@aauth_super_admin`) +- 6 helper functions (`aauth()`, `aauth_can()`, etc.) +- The `AAuthUser` trait's overridden `can()` method +- Two Eloquent global scopes (`AAuthOrganizationNodeScope`, `AAuthABACModelScope`) + +Under PHP-FPM each HTTP request is a fresh PHP process, so the singleton is re-resolved every time and the binding lifetime is invisible. Under Laravel Octane (FrankenPHP / Swoole / RoadRunner) workers persist between requests; the singleton instance — including its public `$user`, `$role`, `$organizationNodeIds` properties and its `requestCache` array — leaks from one user to the next. A reproduction in a single-worker FrankenPHP setup demonstrates that User B's request returns User A's `organizationNodeIds()`. + +The service layer carries two adjacent reliability defects. `OrganizationService::updateNodePathsRecursively` and `deleteOrganizationNodesRecursively` open a DB transaction, recursively call themselves with `$withDBTransaction = false`, and wrap the body in `try/catch (\Exception)`. The catch calls `DB::rollback()` but **does not re-throw**; control falls through and the outer `DB::commit()` still executes on a transaction stack where the inner state is undefined. Worse, the catch is unreachable for the recursive callee (which is itself wrapped in another try/catch). A failed delete therefore leaves a half-deleted subtree with no error surfaced to the caller. Similarly, `RolePermissionService::detachOrganizationRoleFromUser($userId, $roleId, $organizationNodeId)` takes its parameters in the opposite order from its sibling `attachOrganizationRoleToUser($organizationNodeId, $roleId, $userId)`. Every consumer that swapped attach for detach without re-reading the signature is silently writing the wrong row. + +The customer who reported the Octane issue is running production traffic. The other two defects were uncovered by the same audit. All three touch lifecycle and service-layer contracts that consumers depend on, so they ship in one coordinated change. + +## Goals / Non-Goals + +**Goals:** + +- Eliminate cross-request state leak of the `aauth` binding in Octane / Vapor without changing PHP-FPM behaviour or the public API surface. +- Make recursive organization-node service operations atomic: either every change in a subtree commits or none does, with the original exception surfaced to the caller. +- Make `attachOrganizationRoleToUser` and `detachOrganizationRoleFromUser` parameter-compatible while preserving backward compatibility for one minor cycle via a deprecated bridge. +- Raise the test suite from "demonstrative" to "regression-grade" for these three areas (every fix has at least one failing-before / passing-after test). +- Keep the PHPStan baseline stable or shrinking and keep Pest green both locally and on GitHub Actions. + +**Non-Goals:** + +- Refactoring the broader singleton resolution path (lazy boot, multi-guard awareness, role switching mid-request). Those are valid follow-ups but out of scope here — this change is corrective, not architectural. +- Changing migrations or the storage schema. +- Reworking ABAC rule evaluation or the materialized-path organization logic. +- Replacing the `// todo`s outside the four named test files. Only the test files that already exist gain real assertions here, plus the three new test files for the three fixes. + +**Backward-compatibility contract (hard constraint for this change):** + +- `composer update` from any version in the `21.x` line to the version that ships this change MUST NOT break a working host application. +- Classic PHP-FPM behaviour (`php artisan serve`, `php-fpm`) MUST be byte-equivalent to today: same number of constructor calls per request, same DB query count, same observable side effects. +- No public method signature is altered. No constructor parameter is added, removed or reordered. No exception type that wasn't already thrown becomes mandatory. +- No `E_USER_DEPRECATED` or `E_USER_WARNING` is emitted by code paths that were previously silent. Static `@deprecated` docblock annotations are the only deprecation signal in this release. +- Configuration defaults are unchanged. Existing `aauth.php` and `aauth-advanced.php` files keep working. +- The `scoped()` binding behaves identically to `singleton()` under PHP-FPM (each request is a new process, so the lifetime distinction is invisible). Under Octane / Vapor the change is observable but the observable effect is precisely the security fix consumers are asking for. + +## Decisions + +### Decision 1: `singleton('aauth', ...)` → `scoped('aauth', ...)` + +Laravel 9+ ships `Application::scoped()`, a binding type that behaves like a singleton until a "scope flush" event, after which the next resolution returns a fresh instance. Octane's `Octane::prepareApplicationForNextRequest()` fires `RequestTerminated`; the `FlushTemporaryContainerInstances` listener flushes all scoped bindings. PHP-FPM has no analogous lifecycle, but because each request is a fresh process the distinction is moot. + +**Why scoped over alternatives:** + +- *Make it transient (`$app->bind`)*: Resolves a fresh instance per `app('aauth')` call. Within one request, `Gate::before` calls `app('aauth')` once per permission check; we would do 5–50× the constructor work (which performs 3 DB queries) per request. Rejected on performance grounds. +- *Use a request middleware that forgets the instance*: Works, but adds a new middleware that consumers must install in their kernel. `scoped()` requires no consumer action. +- *Document `config/octane.php` `'flush' => ['aauth']`*: This is the documented Octane workaround, but it puts the burden on every consumer. Suitable as a stopgap for unupgraded consumers; a footnote in `UPGRADE.md` covers them. Vendor-level fix is cleaner. + +The constructor's per-resolution cost (`Auth::user()` + `Session::get('roleId')` + role load + node IDs pluck + context build) is unchanged — we still resolve once per request, just per-request instead of per-worker-lifetime. + +### Decision 2: `try/finally` with re-throw, savepoints for recursion — **signature preserved** + +The recursive methods currently look like: + +```php +public function deleteOrganizationNodesRecursively(OrganizationNode $node, ?bool $withDBTransaction = true): void +{ + if ($withDBTransaction) { DB::beginTransaction(); } + try { + // ... recurse ... + $node->delete(); + } catch (\Exception $exception) { + DB::rollback(); + } + if ($withDBTransaction) { DB::commit(); } +} +``` + +Two defects: +1. Caught exception is swallowed (no re-throw, no return signal). +2. `commit()` runs after `rollback()` on the outer transaction — the second statement targets a transaction that no longer exists at that depth. + +**New shape** uses Laravel's `DB::transaction()` closure helper, which handles `beginTransaction` / `commit` / `rollback` with native savepoint nesting when called recursively. **The public method signature stays identical** so that `composer update` is non-breaking for any caller: + +```php +public function deleteOrganizationNodesRecursively(OrganizationNode $node, ?bool $withDBTransaction = true): void +{ + if ($withDBTransaction) { + DB::transaction(fn () => $this->deleteSubtree($node)); + return; + } + $this->deleteSubtree($node); +} + +protected function deleteSubtree(OrganizationNode $node): void +{ + foreach (OrganizationNode::whereParentId($node->id)->get() as $child) { + $this->deleteSubtree($child); + } + $node->delete(); +} +``` + +The `$withDBTransaction` parameter is **kept** (BC). Its semantic is unchanged: `true` = open a top-level transaction here, `false` = the caller is already managing one. The behavioural change is that exceptions are no longer swallowed — they propagate to the caller. Code that silently relied on the bug ("never throws") will now see real errors, but that path was a defect and not a documented behaviour. + +**Alternative considered:** Drop the `$withDBTransaction` parameter. Rejected — the parameter is part of the public signature and dropping it would break `composer update` for anyone passing it explicitly. + +**Alternative considered:** Manual `try/finally` with depth tracking. Rejected — re-implementing what `DB::transaction()` already does correctly is a worse outcome than calling the framework helper. + +### Decision 3: `detachOrganizationRoleFromUser` parameter order — **existing method untouched, new aligned method added** + +The constraint here is strict: `composer update` must not silently break any existing call site. The three parameters are all `int`, so a runtime heuristic cannot distinguish the old order from the new order. Re-ordering the existing method's parameters would silently corrupt data for every caller that does not rewrite the call site. **We do not re-order the existing method.** + +Instead: + +- **Keep** `detachOrganizationRoleFromUser(int $userId, int $roleId, int $organizationNodeId)` with its current parameter order and current behaviour, fully BC. +- Mark it `@deprecated since 21.1.0` in its docblock with a clear pointer to the new method. +- **Add** a new method whose name signals the aligned order: + + ```php + public function detachOrganizationRoleFromUserBy( + int $organizationNodeId, + int $roleId, + int $userId, + ): int + ``` + + Same internal behaviour as the legacy method, just with parameter order matching `attachOrganizationRoleToUser`. + +- The deprecation is a docblock-only signal in this minor release. We do **not** emit a runtime `E_USER_DEPRECATED` for the legacy method because triggering errors from a previously-correct call path would surface as noise (and on strict error handlers, as exceptions) immediately after `composer update`. The static deprecation annotation lets PHPStan/IDEs flag call sites without altering runtime behaviour. + +- A future major version (`22.0.0`) renames `detachOrganizationRoleFromUserBy` to `detachOrganizationRoleFromUser` (taking over the canonical name) and removes the legacy method. That cleanup is **explicitly out of scope here** and is documented in `UPGRADE.md` as the planned next step. + +**Why a new method instead of re-ordering the old one:** +Three positional `int`s are indistinguishable at runtime, so silent data corruption is the only possible outcome of a re-order without consumer code changes. A separately-named method makes the migration explicit and visible in diffs and IDEs without any breakage. + +**Why no runtime deprecation notice:** +Strict consumer environments convert `E_USER_DEPRECATED` to exceptions. Emitting one from a previously-valid call path turns a "non-breaking minor" into a hard break on `composer update`. The docblock `@deprecated` tag gives static analysers and IDEs everything they need without changing runtime behaviour. + +**Test coverage:** +- The new `detachOrganizationRoleFromUserBy` writes/reads correct rows when called with the aligned order. +- The existing `detachOrganizationRoleFromUser` continues to write/read correct rows when called with its historic order — no regression. +- (No deprecation-notice test in this release because we do not emit one.) + +### Decision 4: Test infrastructure improvements scoped to this change + +Pest test files affected: +- `tests/Unit/RolePermissionServiceTest.php`: implement `updateRole`, `deleteRole`, `activateRole`, `deactivateRole`. Add `detachOrganizationRoleFromUser` happy-path and legacy-bridge tests. +- `tests/Unit/AAuthTest.php`: implement `passOrAbort`, "can get one specified organization node", and `switchableRolesStatic`. +- New `tests/Unit/V2/OctaneScopingTest.php`: simulates `RequestTerminated` by calling `$app->forgetScopedInstances()` and verifies a fresh AAuth is resolved with a different user/role. +- New `tests/Unit/V2/TransactionIntegrityTest.php`: forces a failure mid-recursion (e.g. throw inside an observer on the second child) and verifies no node was deleted. +- New `tests/Unit/V2/DeprecationBridgeTest.php`: verifies the legacy detach signature still works and emits `E_USER_DEPRECATED`. + +We do **not** wire a real Octane runtime into the test suite; `Application::forgetScopedInstances()` is the exact method Octane calls and is sufficient to prove the binding lifetime. + +### Decision 5: CI workflow audit + +Existing `.github/workflows/run-tests.yml` (referenced in README badges) is checked. If missing or stale, we add a minimal matrix: +- PHP `8.2`, `8.3`, `8.4` +- Laravel `^11.0`, `^12.0` +- Runs `composer install`, `vendor/bin/pest`, `vendor/bin/phpstan analyse` + +No new CI service is introduced. + +## Risks / Trade-offs + +- **Risk:** `scoped()` changes binding lifetime in a way some advanced consumer relies on (e.g., resolving `aauth` from a queued job that boots in the same worker). → **Mitigation:** Queued jobs run outside the HTTP scope flush; they receive a fresh resolution per job execution, which is the desired behaviour. Documented in `UPGRADE.md`. +- **Risk:** `DB::transaction()` swallows exceptions during recursion if a caller is already inside a transaction and savepoints are unsupported by their DB driver. → **Mitigation:** MySQL ≥ 5.0.3, MariaDB, PostgreSQL all support savepoints. SQLite ≥ 3.6.8 also supports them. These are the documented supported drivers. +- **Risk:** The legacy detach bridge confuses static analysers (PHPStan flags deprecated method usage). → **Mitigation:** That is the desired signal — consumers see the warning in their own CI. +- **Trade-off:** We do not detect old-order calls on the canonical method. Consumers passing args in the old order will silently write the wrong rows. → **Mitigation:** Documented prominently in `UPGRADE.md` with a migration grep recipe (`grep -rn 'detachOrganizationRoleFromUser('`). +- **Risk:** New tests slow CI noticeably. → **Mitigation:** All new tests use the existing `migrate:fresh + SampleDataSeeder` pattern. Combined runtime budget < 3 s on the matrix runners. + +## Migration Plan + +1. Ship the three code fixes behind one PR with all tests green locally and on GitHub Actions. +2. Tag a minor release (e.g. `21.1.0`) — the package is currently `^21.0.0` and the changes are BC-preserving except for the protected `$withDBTransaction` parameter (acceptable in a minor under our current versioning). +3. Update `UPGRADE.md` with three sections: + - **Octane consumers:** "no action required, but if you previously added `'flush' => ['aauth']` to `config/octane.php` you can remove it." + - **OrganizationService:** "if you were passing `false` as the second arg to `updateNodePathsRecursively` / `deleteOrganizationNodesRecursively`, drop that arg — transaction nesting is now automatic." + - **detachOrganizationRoleFromUser:** "the canonical signature now matches `attach`. Migrate calls; the old order is available on `detachOrganizationRoleFromUserLegacy()` until the next major." +4. Open follow-up issues for the remaining items from the audit (singleton lazy-boot, parameter validation algorithm, schema migration cleanup) — out of scope here. + +**Rollback:** Tag a `21.1.1` revert if regressions surface. None of the changes touch persisted data. diff --git a/openspec/changes/archive/2026-05-22-fix-critical-auth-bugs/proposal.md b/openspec/changes/archive/2026-05-22-fix-critical-auth-bugs/proposal.md new file mode 100644 index 0000000..ec602fd --- /dev/null +++ b/openspec/changes/archive/2026-05-22-fix-critical-auth-bugs/proposal.md @@ -0,0 +1,40 @@ +## Why + +A production user running AAuth on Laravel Octane reported a **HIGH severity authorization bypass**: the `app->singleton('aauth', ...)` binding survives request boundaries in long-lived workers, so User A's resolved `AAuth` instance (user, role, organization node IDs, permissions, ABAC rules, super-admin flag) is reused for User B's request. The same review surfaced two more correctness defects in the service layer — silent transaction corruption in recursive organization-node operations, and an inverted parameter order between `attachOrganizationRoleToUser` and `detachOrganizationRoleFromUser`. These three issues sit on the same lifecycle/service-layer surface and must ship together. While we are touching this code we also close the test coverage gaps that have been masked by `// todo` placeholders. + +## What Changes + +- **BREAKING (semantic, not API) — Auth lifecycle:** Change the `aauth` container binding from `singleton` to `scoped` so Octane / Vapor flushes the resolved instance at every `RequestTerminated` boundary. PHP-FPM behavior is unchanged. The public facade and service API are unchanged. +- **Service-layer reliability:** Rewrite `OrganizationService::updateNodePathsRecursively` and `deleteOrganizationNodesRecursively` to use `try/finally` with proper transaction handling and re-throw exceptions instead of swallowing them. Recursive calls reuse the outer transaction via savepoints. +- **API consistency (BC-safe):** Re-order `RolePermissionService::detachOrganizationRoleFromUser` parameters to match `attachOrganizationRoleToUser` (`$organizationNodeId, $roleId, $userId`). The old order is preserved as a deprecated bridge that emits an `E_USER_DEPRECATED` notice and forwards to the new signature, scheduled for removal in the next major version. +- **Test coverage:** Replace `// todo` stubs in `RolePermissionServiceTest` (updateRole, deleteRole, activateRole, deactivateRole) and `AAuthTest` (`passOrAbort`, "can get one specified organization node") with real assertions, add coverage for `AAuth::switchableRolesStatic`, and add new tests for each of the three fixes above. +- **CI:** Verify the GitHub Actions test workflow runs the suite on supported PHP/Laravel matrix; add a minimal workflow if missing. + +## Capabilities + +### New Capabilities + +None — this change is entirely corrective. No new product capability is introduced. + +### Modified Capabilities + +- `core-rbac`: `AAuth` container lifecycle requirement changes — the resolved instance MUST NOT survive across HTTP requests in long-lived workers. A new requirement covers transactional integrity for recursive organization-node service operations and parameter-order consistency between paired attach/detach service methods. + +## Impact + +- **Code:** + - `src/AAuthServiceProvider.php` (singleton → scoped binding) + - `src/Services/OrganizationService.php` (transaction handling in recursive methods) + - `src/Services/RolePermissionService.php` (detach parameter order + deprecation bridge) +- **Tests:** + - `tests/Unit/AAuthTest.php`, `tests/Unit/RolePermissionServiceTest.php` (fill todos) + - New `tests/Unit/V2/OctaneScopingTest.php` (forgetInstance/flush simulation) + - New `tests/Unit/V2/TransactionIntegrityTest.php` + - New deprecation-emission test for the detach bridge +- **CI:** `.github/workflows/run-tests.yml` audited (created if missing). Local `vendor/bin/pest` and GitHub Actions both green. +- **APIs / consumers:** + - Octane users on the old version SHOULD continue working but the security fix is mandatory for them. + - Anyone calling `detachOrganizationRoleFromUser($userId, $roleId, $organizationNodeId)` will continue working with a deprecation notice until the next major. +- **Migrations:** None. No schema changes. +- **PHPStan:** Baseline should not grow; ideally shrinks by the deletion of a `@phpstan-ignore` line previously masking the swallowed-exception path. +- **Docs:** README "Octane" callout added; UPGRADE.md gets a short note about the deprecation. \ No newline at end of file diff --git a/openspec/changes/archive/2026-05-22-fix-critical-auth-bugs/specs/core-rbac/spec.md b/openspec/changes/archive/2026-05-22-fix-critical-auth-bugs/specs/core-rbac/spec.md new file mode 100644 index 0000000..0d9c67a --- /dev/null +++ b/openspec/changes/archive/2026-05-22-fix-critical-auth-bugs/specs/core-rbac/spec.md @@ -0,0 +1,103 @@ +## MODIFIED Requirements + +### Requirement: AAuth singleton SHALL be resolved with authenticated user and active role + +The system SHALL register an `aauth` binding in Laravel's service container as a **request-scoped** binding (`Application::scoped()`), resolved on first use within a request from the authenticated user and the active roleId in the session. The resolved instance MUST NOT survive across HTTP requests in long-lived workers (Octane, Vapor); the container SHALL flush the instance on `RequestTerminated` so the next request resolves a fresh `AAuth` against the current authenticated user. + +Under classic PHP-FPM each request is a fresh process, so the scoped lifetime is equivalent to the previous singleton lifetime. Under Octane / Vapor, the scoped lifetime prevents cross-request state leakage of the resolved user, role, organization node IDs, permissions, ABAC rules, and super-admin flag. + +#### Scenario: Authenticated user with valid role + +- **WHEN** an authenticated user has an active roleId in session +- **THEN** the AAuth instance is resolved with that user context and role loaded + +#### Scenario: Unauthenticated user + +- **WHEN** no authenticated user exists +- **THEN** an AuthenticationException is thrown + +#### Scenario: Missing role + +- **WHEN** no roleId is in the session +- **THEN** a MissingRoleException is thrown + +#### Scenario: User not assigned to role + +- **WHEN** the user does not have the specified role assigned +- **THEN** a UserHasNoAssignedRoleException is thrown + +#### Scenario: Request boundary flushes the resolved instance + +- **WHEN** a request terminates in a long-lived worker (Octane / Vapor) and a new request begins with a different authenticated user +- **THEN** resolving `aauth` again returns a new `AAuth` instance bound to the new user — the previous user's `organizationNodeIds`, permissions, role, and super-admin flag are not reused + +#### Scenario: Manual scope flush by simulation + +- **WHEN** a test or framework code calls `$app->forgetScopedInstances()` between two `app('aauth')` resolutions targeting different users +- **THEN** the second resolution returns a fresh instance whose `currentRole()->id`, `organizationNodeIds()`, and `can()` results reflect the second user + +## ADDED Requirements + +### Requirement: Recursive organization-node service operations SHALL be atomic + +`OrganizationService::updateNodePathsRecursively` and `OrganizationService::deleteOrganizationNodesRecursively` SHALL execute under a single logical database transaction. If any node operation in the subtree fails, the entire subtree SHALL be rolled back to its pre-call state and the original exception SHALL propagate to the caller. The methods MUST NOT silently swallow exceptions. + +When called recursively (or when the caller is already inside a transaction), the implementation SHALL use the database driver's nested-transaction support (savepoints). The existing `$withDBTransaction = true` parameter SHALL be preserved on the public signatures of both methods so that `composer update` does not break any existing call site; its semantics remain unchanged (`true` = open a top-level transaction here, `false` = participate in the caller's transaction). + +#### Scenario: Successful recursive delete + +- **WHEN** `deleteOrganizationNodesRecursively($subtreeRoot)` is called and every descendant deletes successfully +- **THEN** every node in the subtree is removed and the method returns normally + +#### Scenario: Mid-recursion failure rolls back + +- **WHEN** `deleteOrganizationNodesRecursively($subtreeRoot)` is called and a descendant raises an exception (for example, via a model observer) +- **THEN** no node from the subtree is deleted (the parent and all children remain), and the original exception is re-thrown to the caller + +#### Scenario: Path update failure rolls back + +- **WHEN** `updateNodePathsRecursively($node)` is called and a descendant `save()` fails partway through +- **THEN** no descendant `path` value is mutated and the original exception is re-thrown to the caller + +#### Scenario: Caller-side transaction integration + +- **WHEN** a caller wraps `deleteOrganizationNodesRecursively($node)` inside its own `DB::transaction()` block +- **THEN** the service participates in the outer transaction via savepoints; rolling back the outer transaction reverts the subtree deletion + +### Requirement: A canonical aligned-order detach method SHALL be added without breaking the existing one + +The `RolePermissionService` SHALL expose a new method `detachOrganizationRoleFromUserBy(int $organizationNodeId, int $roleId, int $userId): int` whose parameter order matches `attachOrganizationRoleToUser`. The existing `detachOrganizationRoleFromUser(int $userId, int $roleId, int $organizationNodeId): int` method SHALL remain in place with its current parameter order and current behaviour, fully backward compatible. The existing method SHALL be marked `@deprecated since 21.1.0` in its docblock with a pointer to the new method. No runtime deprecation notice SHALL be emitted by the existing method in this release. The existing method SHALL be removed only in the next major version. + +#### Scenario: New aligned-order method removes correct row + +- **WHEN** a caller invokes `detachOrganizationRoleFromUserBy($nodeId, $roleId, $userId)` +- **THEN** the row identified by `(user_id=$userId, role_id=$roleId, organization_node_id=$nodeId)` is removed from `user_role_organization_node` + +#### Scenario: Existing method continues to work unchanged + +- **WHEN** an existing consumer invokes `detachOrganizationRoleFromUser($userId, $roleId, $nodeId)` after `composer update` +- **THEN** the row identified by `(user_id=$userId, role_id=$roleId, organization_node_id=$nodeId)` is removed exactly as before, with no warning, notice, or exception emitted at runtime + +#### Scenario: Static analysis flags the deprecation + +- **WHEN** PHPStan or an IDE inspects a call site of `detachOrganizationRoleFromUser` +- **THEN** the `@deprecated` docblock annotation is surfaced, directing the developer to `detachOrganizationRoleFromUserBy` + +#### Scenario: Future major release removes the legacy method + +- **WHEN** the next major version of the package is released +- **THEN** the legacy `detachOrganizationRoleFromUser` method is removed and only the aligned-order canonical method remains + +### Requirement: Test suite SHALL cover the corrected lifecycle and service behaviours + +The Pest test suite SHALL contain at least one regression test per requirement above, including a scoped-binding simulation, a transaction rollback assertion, and a deprecation-notice assertion. Pre-existing `// todo` stubs for `updateRole`, `deleteRole`, `activateRole`, `deactivateRole`, `passOrAbort`, the single-node lookup, and `switchableRolesStatic` SHALL be replaced with real assertions. + +#### Scenario: Suite is green locally + +- **WHEN** `vendor/bin/pest` is invoked from the package root +- **THEN** every test passes and the previously-stubbed tests assert behaviour rather than `expect(1)->toBeTruthy()` + +#### Scenario: Suite is green on GitHub Actions + +- **WHEN** the configured workflow runs on a push to `main` against PHP 8.2/8.3/8.4 and Laravel 11/12 +- **THEN** every test in every matrix cell passes diff --git a/openspec/changes/archive/2026-05-22-fix-critical-auth-bugs/tasks.md b/openspec/changes/archive/2026-05-22-fix-critical-auth-bugs/tasks.md new file mode 100644 index 0000000..91c9e5a --- /dev/null +++ b/openspec/changes/archive/2026-05-22-fix-critical-auth-bugs/tasks.md @@ -0,0 +1,85 @@ +## 1. Pre-flight (verify current state) + +- [ ] 1.1 Run `vendor/bin/pest` from the package root and record the baseline pass/fail count +- [ ] 1.2 Run `vendor/bin/phpstan analyse` and record the baseline error count +- [ ] 1.3 Confirm `.github/workflows/run-tests.yml` exists; if absent, plan a minimal workflow as part of group 6 + +## 2. Fix #1 — Octane request-scoped binding (Security HIGH) + +- [ ] 2.1 In `src/AAuthServiceProvider.php`, change `$this->app->singleton('aauth', ...)` to `$this->app->scoped('aauth', ...)` (line ~63) +- [ ] 2.2 Add a short docblock above the binding explaining that scoped lifetime is required for Octane / Vapor correctness, with a one-line pointer to the design doc +- [ ] 2.3 Create `tests/Unit/V2/OctaneScopingTest.php`: + - [ ] 2.3.1 Resolve `app('aauth')` for User A with role X, capture `currentRole()->id` and `organizationNodeIds()` + - [ ] 2.3.2 Call `$this->app->forgetScopedInstances()` to simulate the Octane request boundary + - [ ] 2.3.3 Rebind `Auth::user()` and `Session` to User B with role Y, resolve `app('aauth')` again, assert it is a different instance and returns User B's role/node IDs + - [ ] 2.3.4 Add an assertion that within a single request the same instance is returned (no per-call re-construction) +- [ ] 2.4 Verify Octane installation is NOT required by the test — `forgetScopedInstances()` is a core Laravel API + +## 3. Fix #2 — Recursive transaction integrity + +- [ ] 3.1 In `src/Services/OrganizationService.php`, refactor `deleteOrganizationNodesRecursively`: + - [ ] 3.1.1 Keep the public signature `(OrganizationNode $node, ?bool $withDBTransaction = true): void` + - [ ] 3.1.2 When `$withDBTransaction === true`, wrap the body in `DB::transaction(fn () => $this->deleteSubtree($node))` + - [ ] 3.1.3 Extract the recursion into a `protected function deleteSubtree(OrganizationNode $node): void` that does NOT manage transactions + - [ ] 3.1.4 Remove the `try/catch` that previously swallowed exceptions — let them propagate +- [ ] 3.2 In `src/Services/OrganizationService.php`, refactor `updateNodePathsRecursively` with the same shape (`updateSubtreePaths` helper, transaction at the outer boundary, no swallowed exceptions) +- [ ] 3.3 Create `tests/Unit/V2/TransactionIntegrityTest.php`: + - [ ] 3.3.1 Test: successful recursive delete removes the full subtree + - [ ] 3.3.2 Test: register a temporary `deleting` model observer that throws on a specific child node, call the recursive delete, assert (a) the exception bubbles up, (b) no node from the subtree is deleted + - [ ] 3.3.3 Test: successful recursive path update writes all descendant paths + - [ ] 3.3.4 Test: caller-managed `DB::transaction()` wrapping the service call rolls back the subtree when the outer transaction rolls back +- [ ] 3.4 Verify SQLite (used in tests) supports savepoints — Laravel uses them transparently via `DB::transaction()` + +## 4. Fix #3 — Aligned-order detach method (BC-safe) + +- [ ] 4.1 In `src/Services/RolePermissionService.php`, add a new method: + - [ ] 4.1.1 `public function detachOrganizationRoleFromUserBy(int $organizationNodeId, int $roleId, int $userId): int` + - [ ] 4.1.2 Internally delegates to (or duplicates the body of) the existing `detachOrganizationRoleFromUser`, with parameters mapped correctly + - [ ] 4.1.3 Cache invalidation (`clearUserRoleCache`) behaves identically +- [ ] 4.2 Add `@deprecated since 21.1.0 use detachOrganizationRoleFromUserBy() with parameter order matching attachOrganizationRoleToUser()` to the docblock of the existing `detachOrganizationRoleFromUser`. **Do not** emit a runtime notice +- [ ] 4.3 In `tests/Unit/RolePermissionServiceTest.php`, add tests: + - [ ] 4.3.1 `detachOrganizationRoleFromUserBy` removes the correct row when called with aligned-order args + - [ ] 4.3.2 The existing `detachOrganizationRoleFromUser` still removes the correct row when called with historic-order args (regression guard) + - [ ] 4.3.3 No `E_USER_DEPRECATED` notice is emitted at runtime by the existing method + +## 5. Fix #4 — Fill missing tests in existing files + +- [ ] 5.1 In `tests/Unit/RolePermissionServiceTest.php`, implement the four stubs: + - [ ] 5.1.1 `can update a role`: create a role, call `updateRole`, assert name change persisted + - [ ] 5.1.2 `can delete a role`: create a role, call `deleteRole`, assert it is gone and the related `role_permission` rows are cleaned up consistently with current behaviour + - [ ] 5.1.3 `can activate role`: create a passive role, call `activateRole`, assert `status === 'active'` + - [ ] 5.1.4 `can deactivate role`: create an active role, call `deactivateRole`, assert `status === 'passive'` +- [ ] 5.2 In `tests/Unit/AAuthTest.php`, implement: + - [ ] 5.2.1 `passOrAbort`: positive path (permission granted → no exception) and negative path (no permission → 401 HttpException with the expected message) + - [ ] 5.2.2 `can get one specified organization node`: bind aauth to user1/role3, assert `organizationNode($validId)` returns the node and `organizationNode($invalidId)` throws `InvalidOrganizationNodeException` +- [ ] 5.3 Add a new test (in `tests/Unit/AAuthTest.php` or a dedicated file) for `AAuth::switchableRolesStatic`: + - [ ] 5.3.1 Seeded user1 has multiple assigned roles → static returns same count as the instance method + +## 6. CI workflow audit and pest run + +- [ ] 6.1 Inspect `.github/workflows/run-tests.yml`. If absent or stale, create a minimal workflow that runs on push and PR to `main` covering: + - PHP `8.2`, `8.3`, `8.4` + - Laravel `^11.0`, `^12.0` (composer matrix or two-job split) + - Steps: checkout, setup-php, `composer install --prefer-dist --no-progress`, `vendor/bin/pest`, `vendor/bin/phpstan analyse --no-progress` +- [ ] 6.2 Run `vendor/bin/pest` locally and verify every test (including the new ones) passes +- [ ] 6.3 Run `vendor/bin/phpstan analyse` locally and confirm the baseline did not grow +- [ ] 6.4 Commit and push; verify GitHub Actions is green on the resulting PR + +## 7. Documentation + +- [ ] 7.1 Update `README.md`: + - [ ] 7.1.1 Add a short "Octane / long-lived workers" callout under installation explaining that the binding is request-scoped and no extra `config/octane.php` flush entry is needed + - [ ] 7.1.2 If `detachOrganizationRoleFromUser` is documented in README, point to the new aligned-order method +- [ ] 7.2 Update `UPGRADE.md` with three sections: + - [ ] 7.2.1 **Octane consumers:** no action required; the previously-recommended `'flush' => ['aauth']` workaround can be removed + - [ ] 7.2.2 **OrganizationService recursive methods:** signature unchanged, but exceptions now propagate instead of being silently swallowed — review any caller that depended on silent failure + - [ ] 7.2.3 **detachOrganizationRoleFromUser:** the historic method continues to work; migrate to `detachOrganizationRoleFromUserBy` with aligned parameter order before the next major release +- [ ] 7.3 If the OpenSpec project lists `core-rbac` under `openspec/specs/`, do not edit it directly — sync happens in group 8 + +## 8. Finalize: spec sync and archive + +- [ ] 8.1 Run `openspec validate fix-critical-auth-bugs` and confirm the change is well-formed +- [ ] 8.2 Run `opsx:sync` (or `openspec sync --change fix-critical-auth-bugs`) to merge the `MODIFIED Requirements` and `ADDED Requirements` deltas into `openspec/specs/core-rbac/spec.md` +- [ ] 8.3 Manually review the merged main spec and confirm the singleton requirement now describes scoped binding, and the two new requirements appear verbatim +- [ ] 8.4 Stage all changes and prepare a commit message that references this OpenSpec change +- [ ] 8.5 Do not archive the change yet — archive only after the PR is merged on `main` (handled by `opsx:archive` later) diff --git a/openspec/specs/core-rbac/spec.md b/openspec/specs/core-rbac/spec.md index c14aad0..f7a0abd 100644 --- a/openspec/specs/core-rbac/spec.md +++ b/openspec/specs/core-rbac/spec.md @@ -8,11 +8,13 @@ Provide a complete Role-Based Access Control system for Laravel applications wit ### Requirement: AAuth singleton SHALL be resolved with authenticated user and active role -The system SHALL register an `aauth` singleton in Laravel's service container that requires an authenticated user and an active roleId from the session. +The system SHALL register an `aauth` binding in Laravel's service container as a **request-scoped** binding (`Application::scoped()`), resolved on first use within a request from the authenticated user and the active roleId in the session. The resolved instance MUST NOT survive across HTTP requests in long-lived workers (Octane, Vapor); the container SHALL flush the instance on `RequestTerminated` so the next request resolves a fresh `AAuth` against the current authenticated user. + +Under classic PHP-FPM each request is a fresh process, so the scoped lifetime is equivalent to the previous singleton lifetime. Under Octane / Vapor, the scoped lifetime prevents cross-request state leakage of the resolved user, role, organization node IDs, permissions, ABAC rules, and super-admin flag. #### Scenario: Authenticated user with valid role - **WHEN** an authenticated user has an active roleId in session -- **THEN** the AAuth singleton is resolved with user context and role loaded +- **THEN** the AAuth instance is resolved with that user context and role loaded #### Scenario: Unauthenticated user - **WHEN** no authenticated user exists @@ -26,6 +28,14 @@ The system SHALL register an `aauth` singleton in Laravel's service container th - **WHEN** the user does not have the specified role assigned - **THEN** a UserHasNoAssignedRoleException is thrown +#### Scenario: Request boundary flushes the resolved instance +- **WHEN** a request terminates in a long-lived worker (Octane / Vapor) and a new request begins with a different authenticated user +- **THEN** resolving `aauth` again returns a new `AAuth` instance bound to the new user — the previous user's `organizationNodeIds`, permissions, role, and super-admin flag are not reused + +#### Scenario: Manual scope flush by simulation +- **WHEN** a test or framework code calls `$app->forgetScopedInstances()` between two `app('aauth')` resolutions targeting different users +- **THEN** the second resolution returns a fresh instance whose `currentRole()->id`, `organizationNodeIds()`, and `can()` results reflect the second user + ### Requirement: Permission checks SHALL validate against role permissions The `can($permission, ...$arguments)` method SHALL check if the current role has the given permission, optionally validating parametric arguments. @@ -77,3 +87,57 @@ The host application's User model SHALL implement `AAuthUserContract` and use th #### Scenario: Compliant user model - **WHEN** User model implements AAuthUserContract and uses AAuthUser trait - **THEN** `$user->roles()` returns the user's assigned roles + +### Requirement: Recursive organization-node service operations SHALL be atomic + +`OrganizationService::updateNodePathsRecursively` and `OrganizationService::deleteOrganizationNodesRecursively` SHALL execute under a single logical database transaction. If any node operation in the subtree fails, the entire subtree SHALL be rolled back to its pre-call state and the original exception SHALL propagate to the caller. The methods MUST NOT silently swallow exceptions. + +When called recursively (or when the caller is already inside a transaction), the implementation SHALL use the database driver's nested-transaction support (savepoints). The existing `$withDBTransaction = true` parameter SHALL be preserved on the public signatures of both methods so that `composer update` does not break any existing call site; its semantics remain unchanged (`true` = open a top-level transaction here, `false` = participate in the caller's transaction). + +#### Scenario: Successful recursive delete +- **WHEN** `deleteOrganizationNodesRecursively($subtreeRoot)` is called and every descendant deletes successfully +- **THEN** every node in the subtree is removed and the method returns normally + +#### Scenario: Mid-recursion failure rolls back +- **WHEN** `deleteOrganizationNodesRecursively($subtreeRoot)` is called and a descendant raises an exception (for example, via a model observer) +- **THEN** no node from the subtree is deleted (the parent and all children remain), and the original exception is re-thrown to the caller + +#### Scenario: Path update failure rolls back +- **WHEN** `updateNodePathsRecursively($node)` is called and a descendant `save()` fails partway through +- **THEN** no descendant `path` value is mutated and the original exception is re-thrown to the caller + +#### Scenario: Caller-side transaction integration +- **WHEN** a caller wraps `deleteOrganizationNodesRecursively($node)` inside its own `DB::transaction()` block +- **THEN** the service participates in the outer transaction via savepoints; rolling back the outer transaction reverts the subtree deletion + +### Requirement: A canonical aligned-order detach method SHALL be added without breaking the existing one + +The `RolePermissionService` SHALL expose a new method `detachOrganizationRoleFromUserBy(int $organizationNodeId, int $roleId, int $userId): int` whose parameter order matches `attachOrganizationRoleToUser`. The existing `detachOrganizationRoleFromUser(int $userId, int $roleId, int $organizationNodeId): int` method SHALL remain in place with its current parameter order and current behaviour, fully backward compatible. The existing method SHALL be marked `@deprecated since 21.1.0` in its docblock with a pointer to the new method. No runtime deprecation notice SHALL be emitted by the existing method in this release. The existing method SHALL be removed only in the next major version. + +#### Scenario: New aligned-order method removes correct row +- **WHEN** a caller invokes `detachOrganizationRoleFromUserBy($nodeId, $roleId, $userId)` +- **THEN** the row identified by `(user_id=$userId, role_id=$roleId, organization_node_id=$nodeId)` is removed from `user_role_organization_node` + +#### Scenario: Existing method continues to work unchanged +- **WHEN** an existing consumer invokes `detachOrganizationRoleFromUser($userId, $roleId, $nodeId)` after `composer update` +- **THEN** the row identified by `(user_id=$userId, role_id=$roleId, organization_node_id=$nodeId)` is removed exactly as before, with no warning, notice, or exception emitted at runtime + +#### Scenario: Static analysis flags the deprecation +- **WHEN** PHPStan or an IDE inspects a call site of `detachOrganizationRoleFromUser` +- **THEN** the `@deprecated` docblock annotation is surfaced, directing the developer to `detachOrganizationRoleFromUserBy` + +#### Scenario: Future major release removes the legacy method +- **WHEN** the next major version of the package is released +- **THEN** the legacy `detachOrganizationRoleFromUser` method is removed and only the aligned-order canonical method remains + +### Requirement: Test suite SHALL cover the corrected lifecycle and service behaviours + +The Pest test suite SHALL contain at least one regression test per requirement above, including a scoped-binding simulation, a transaction rollback assertion, and a deprecation-notice assertion. Pre-existing `// todo` stubs for `updateRole`, `deleteRole`, `activateRole`, `deactivateRole`, `passOrAbort`, the single-node lookup, and `switchableRolesStatic` SHALL be replaced with real assertions. + +#### Scenario: Suite is green locally +- **WHEN** `vendor/bin/pest` is invoked from the package root +- **THEN** every test passes and the previously-stubbed tests assert behaviour rather than `expect(1)->toBeTruthy()` + +#### Scenario: Suite is green on GitHub Actions +- **WHEN** the configured workflow runs on a push to `main` against PHP 8.2/8.3/8.4 and Laravel 11/12 +- **THEN** every test in every matrix cell passes diff --git a/src/AAuthServiceProvider.php b/src/AAuthServiceProvider.php index f613b37..bb1e112 100644 --- a/src/AAuthServiceProvider.php +++ b/src/AAuthServiceProvider.php @@ -60,7 +60,15 @@ public function boot(): void $this->registerObservers(); $this->registerMiddleware(); - $this->app->singleton('aauth', function ($app) { + // Request-scoped binding (not singleton): under Laravel Octane / Vapor the + // resolved AAuth instance carries per-user state (role, organization node IDs, + // permissions, ABAC rules, super-admin flag). A singleton binding survives the + // request boundary in long-lived workers and leaks one user's authorization + // context into the next user's request. `scoped()` is flushed automatically by + // Octane's FlushTemporaryContainerInstances listener on RequestTerminated. Under + // classic PHP-FPM each request is a fresh process, so scoped() and singleton() + // are observationally equivalent (no behaviour change for PHP-FPM consumers). + $this->app->scoped('aauth', function ($app) { return new AAuth( Auth::user(), // @phpstan-ignore-line Session::get('roleId') diff --git a/src/Services/OrganizationService.php b/src/Services/OrganizationService.php index 5ea7bcc..889a922 100644 --- a/src/Services/OrganizationService.php +++ b/src/Services/OrganizationService.php @@ -81,8 +81,8 @@ public function deleteOrganizationScope(int $id): ?bool /** * Creates an org. node with given array * - * @param array $organizationNode - * @param bool $withValidation + * @param array $organizationNode + * @param bool $withValidation * @return OrganizationNode * * @throws ValidationException @@ -101,15 +101,24 @@ public function createOrganizationNode(array $organizationNode, bool $withValida $parentPath = $this->getPath($organizationNode['parent_id'] ?? null); - // add temp path before determine actual path - $organizationNode['path'] = $parentPath . '/?'; - $organizationNode = OrganizationNode::create($organizationNode); + // Build attributes with literal fillable keys so Larastan can verify + // the shape against OrganizationNode's fillable columns. + $attributes = [ + 'organization_scope_id' => $organizationNode['organization_scope_id'] ?? null, + 'name' => $organizationNode['name'] ?? null, + 'model_type' => $organizationNode['model_type'] ?? null, + 'model_id' => $organizationNode['model_id'] ?? null, + 'path' => $parentPath . '/?', + 'parent_id' => $organizationNode['parent_id'] ?? null, + ]; + + $created = OrganizationNode::create($attributes); // todo , can be add inside model's created event - $organizationNode->path = $parentPath . $organizationNode->id; - $organizationNode->save(); + $created->path = $parentPath . $created->id; + $created->save(); - return $organizationNode; + return $created; } /** @@ -145,65 +154,88 @@ public function calculatePath(int $organizationNodeId): void } /** - * Updates organization node recursively using breadth first method - * @param OrganizationNode $node - * @param bool|null $withDBTransaction + * Updates organization node paths recursively (breadth-first). + * + * Atomicity guarantee: the entire subtree update is wrapped in a single + * database transaction (savepoints under nested calls). If any descendant + * save fails, every prior change in the subtree is rolled back and the + * original exception is re-thrown to the caller. Previous versions silently + * swallowed exceptions and left the subtree in an inconsistent state. + * + * The `$withDBTransaction` parameter is preserved for backward compatibility: + * - true (default): open a top-level transaction here. + * - false : the caller is already managing a transaction; participate in it. + * + * @param OrganizationNode $node + * @param bool|null $withDBTransaction * @return void + * + * @throws \Throwable */ public function updateNodePathsRecursively(OrganizationNode $node, ?bool $withDBTransaction = true): void { if ($withDBTransaction) { - DB::beginTransaction(); - } + DB::transaction(fn () => $this->updateSubtreePaths($node)); - try { - $node->path = $this->getPath($node->parent_id) . $node->id; - $node->save(); - - $subNodes = OrganizationNode::whereParentId($node->id)->get(); + return; + } - foreach ($subNodes as $subNode) { - $this->updateNodePathsRecursively($subNode, false); - } + $this->updateSubtreePaths($node); + } - } catch (\Exception $exception) { - DB::rollback(); - } + /** + * Inner recursion for path updates. Does not manage transactions. + * + * @param OrganizationNode $node + * @return void + */ + protected function updateSubtreePaths(OrganizationNode $node): void + { + $node->path = $this->getPath($node->parent_id) . $node->id; + $node->save(); - if ($withDBTransaction) { - DB::commit(); + foreach (OrganizationNode::whereParentId($node->id)->get() as $subNode) { + $this->updateSubtreePaths($subNode); } } /** - * deletes organization nodes using depth first search - * @param OrganizationNode $node - * @param bool|null $withDBTransaction + * Deletes organization nodes recursively (depth-first). + * + * Atomicity guarantee: identical to updateNodePathsRecursively(). The whole + * subtree deletion is atomic; partial failures roll back the entire subtree + * and re-throw the original exception. Previous versions silently swallowed + * exceptions. + * + * @param OrganizationNode $node + * @param bool|null $withDBTransaction * @return void + * + * @throws \Throwable */ public function deleteOrganizationNodesRecursively(OrganizationNode $node, ?bool $withDBTransaction = true): void { if ($withDBTransaction) { - DB::beginTransaction(); - } - - try { - // - $subNodes = OrganizationNode::whereParentId($node->id)->get(); - - foreach ($subNodes as $subNode) { - $this->deleteOrganizationNodesRecursively($subNode, false); - } - - $node->delete(); + DB::transaction(fn () => $this->deleteSubtree($node)); - } catch (\Exception $exception) { - DB::rollback(); + return; } - if ($withDBTransaction) { - DB::commit(); + $this->deleteSubtree($node); + } + + /** + * Inner recursion for deletes. Does not manage transactions. + * + * @param OrganizationNode $node + * @return void + */ + protected function deleteSubtree(OrganizationNode $node): void + { + foreach (OrganizationNode::whereParentId($node->id)->get() as $subNode) { + $this->deleteSubtree($subNode); } + $node->delete(); } } diff --git a/src/Services/RolePermissionService.php b/src/Services/RolePermissionService.php index afd5ecb..5fa385f 100644 --- a/src/Services/RolePermissionService.php +++ b/src/Services/RolePermissionService.php @@ -316,6 +316,15 @@ public function attachOrganizationRoleToUser(int $organizationNodeId, int $roleI } /** + * Detach an organization role from a user. + * + * @deprecated since 21.1.0 Parameter order is inconsistent with + * {@see self::attachOrganizationRoleToUser()}. Use + * {@see self::detachOrganizationRoleFromUserBy()} which takes + * arguments in the aligned order ($organizationNodeId, $roleId, $userId). + * This historic-order method continues to work and emits no + * runtime notice; it will be removed in the next major release. + * * @param int $userId * @param int $roleId * @param int $organizationNodeId @@ -353,6 +362,51 @@ public function detachOrganizationRoleFromUser(int $userId, int $roleId, int $or // todo attach ve sync ile olmayacak gibi direk db query yazmank lazım } + /** + * Detach an organization role from a user using the canonical parameter + * order that matches {@see self::attachOrganizationRoleToUser()}. + * + * Prefer this method over the legacy {@see self::detachOrganizationRoleFromUser()} + * which keeps the historic, inconsistent parameter order for backward + * compatibility and will be removed in the next major release. + * + * @param int $organizationNodeId + * @param int $roleId + * @param int $userId + * @return int + * + * @throws Throwable + */ + public function detachOrganizationRoleFromUserBy(int $organizationNodeId, int $roleId, int $userId): int + { + throw_unless(User::whereId($userId)->exists(), new InvalidUserException()); + + $roleQuery = Role::whereId($roleId); + if ($this->hasRolesTypeColumn()) { + $roleQuery->where('type', '=', 'organization'); + } else { + $roleQuery->whereNotNull('organization_scope_id'); + } + throw_unless($roleQuery->exists(), new InvalidRoleException()); + + throw_unless( + OrganizationNode::whereId($organizationNodeId)->exists(), + new InvalidOrganizationNodeException() + ); + + $result = DB::table('user_role_organization_node') + ->where([ + 'user_id' => $userId, + 'role_id' => $roleId, + 'organization_node_id' => $organizationNodeId, + ]) + ->delete(); + + $this->clearUserRoleCache($userId); + + return $result; + } + /** * Check if type column exists in roles table (for backward compatibility) * diff --git a/tests/TestCase.php b/tests/TestCase.php index 4c056d3..50b0119 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -26,13 +26,19 @@ protected function getPackageProviders($app) public function getEnvironmentSetUp($app) { - // for GitHub tests wirh mysql - // config()->set('database.default', 'mysql'); - - // for local tests with sqlite - config()->set('database.default', 'testing'); - - // for local tests with mysql - config()->set('database.default', 'mysql'); + // Respect the DB_CONNECTION env var from phpunit.xml.dist (and from the + // GitHub Actions matrix). The historic three-line override forced 'mysql' + // regardless, which prevented local developers without Docker from running + // the suite. Now: + // - phpunit.xml.dist sets DB_CONNECTION=mysql by default → MySQL/MariaDB. + // - Local devs without Docker can override via env or a custom phpunit.xml + // (e.g. AAUTH_TEST_DB=sqlite vendor/bin/pest). + $connection = env('DB_CONNECTION', 'testing'); + + if (env('AAUTH_TEST_DB') === 'sqlite') { + $connection = 'testing'; + } + + config()->set('database.default', $connection); } } diff --git a/tests/Unit/AAuthTest.php b/tests/Unit/AAuthTest.php index 0efbe6e..aa5cfdd 100644 --- a/tests/Unit/AAuthTest.php +++ b/tests/Unit/AAuthTest.php @@ -57,9 +57,29 @@ ->and(AAuth::can('create_something_for_organization_k'))->toBeFalse(); }); -test('passOrAbort', function () { - // todo - expect(1)->toBeTruthy(); +test('passOrAbort passes silently when permission is granted', function () { + AAuth::passOrAbort('create_something_for_organization'); + expect(true)->toBeTrue(); +}); + +test('passOrAbort aborts with 401 and custom message when permission is missing', function () { + try { + AAuth::passOrAbort('non_existent_perm', 'Specific denial'); + $this->fail('passOrAbort should have aborted'); + } catch (\Symfony\Component\HttpKernel\Exception\HttpException $e) { + expect($e->getStatusCode())->toBe(401) + ->and($e->getMessage())->toBe('Specific denial'); + } +}); + +test('passOrAbort aborts with 401 and default message when permission is missing', function () { + try { + AAuth::passOrAbort('non_existent_perm'); + $this->fail('passOrAbort should have aborted'); + } catch (\Symfony\Component\HttpKernel\Exception\HttpException $e) { + expect($e->getStatusCode())->toBe(401) + ->and($e->getMessage())->toBe('No Permission'); + } }); test('can get all permitted organization nodes', function () { @@ -75,9 +95,28 @@ }); test('can get one specified organization node', function () { - //AAuth::organizationNodes(); - // todo - expect(1)->toBeTruthy(); + // The seeded AAuth in beforeEach binds user1/role3 which has access to + // organization nodes 1..N via the role3 -> node1 pivot. Sub-tree + // organizationNodes() returns nodes whose path is under node 1. + $accessible = AAuth::organizationNodes(true); + expect($accessible)->not->toBeEmpty(); + + $someNode = $accessible->first(); + $resolved = AAuth::organizationNode($someNode->id); + + expect($resolved)->toBeInstanceOf(OrganizationNode::class) + ->and($resolved->id)->toBe($someNode->id); +}); + +test('organizationNode throws when accessing a node outside the role scope', function () { + AAuth::organizationNode(99999); +})->throws(\AuroraWebSoftware\AAuth\Exceptions\InvalidOrganizationNodeException::class); + +test('switchableRolesStatic returns the same roles as the instance method', function () { + $static = \AuroraWebSoftware\AAuth\AAuth::switchableRolesStatic(1); + $instance = AAuth::switchableRoles(); + + expect(count($static))->toBe(count($instance)); }); test('descendant nodes can be checked', function () { diff --git a/tests/Unit/RolePermissionServiceTest.php b/tests/Unit/RolePermissionServiceTest.php index e357a00..f59b2ab 100644 --- a/tests/Unit/RolePermissionServiceTest.php +++ b/tests/Unit/RolePermissionServiceTest.php @@ -49,23 +49,224 @@ }); test('can update a role', function () { - // todo - expect(true)->toBeTrue(); + $organizationScope = OrganizationScope::whereName('Root Scope')->first(); + + $created = $this->service->createRole([ + 'organization_scope_id' => $organizationScope->id, + 'type' => 'organization', + 'name' => 'Role For Update', + 'status' => 'active', + ]); + + $updated = $this->service->updateRole(['name' => 'Role Renamed'], $created->id); + + expect($updated)->not->toBeNull() + ->and($updated->name)->toBe('Role Renamed') + ->and(Role::find($created->id)->name)->toBe('Role Renamed'); }); test('can delete a role', function () { - // todo - expect(true)->toBeTrue(); + $created = $this->service->createRole([ + 'type' => 'system', + 'name' => 'Role For Delete', + 'status' => 'active', + ]); + + $result = $this->service->deleteRole($created->id); + + expect($result)->toBeTrue() + ->and(Role::find($created->id))->toBeNull(); }); test('can activate role', function () { - // todo - expect(true)->toBeTrue(); + $created = $this->service->createRole([ + 'type' => 'system', + 'name' => 'Role For Activate', + 'status' => 'passive', + ]); + + $result = $this->service->activateRole($created->id); + + expect($result)->toBeTrue() + ->and(Role::find($created->id)->status)->toBe('active'); }); test('can deactivate role', function () { - // todo - expect(true)->toBeTrue(); + $created = $this->service->createRole([ + 'type' => 'system', + 'name' => 'Role For Deactivate', + 'status' => 'active', + ]); + + $result = $this->service->deactivateRole($created->id); + + expect($result)->toBeTrue() + ->and(Role::find($created->id)->status)->toBe('passive'); +}); + +test('detachOrganizationRoleFromUserBy removes correct row with aligned-order args', function () { + $organizationScope = OrganizationScope::whereName('Root Scope')->first(); + $organizationNode = OrganizationNode::whereName('Root Node')->first(); + + $role = $this->service->createRole([ + 'organization_scope_id' => $organizationScope->id, + 'type' => 'organization', + 'name' => 'Detach By Test Role', + 'status' => 'active', + ]); + + $this->service->attachOrganizationRoleToUser($organizationNode->id, $role->id, 1); + + $pivotBefore = \Illuminate\Support\Facades\DB::table('user_role_organization_node') + ->where(['user_id' => 1, 'role_id' => $role->id, 'organization_node_id' => $organizationNode->id]) + ->count(); + expect($pivotBefore)->toBe(1); + + $deleted = $this->service->detachOrganizationRoleFromUserBy($organizationNode->id, $role->id, 1); + + expect($deleted)->toBe(1); + + $pivotAfter = \Illuminate\Support\Facades\DB::table('user_role_organization_node') + ->where(['user_id' => 1, 'role_id' => $role->id, 'organization_node_id' => $organizationNode->id]) + ->count(); + expect($pivotAfter)->toBe(0); +}); + +test('legacy detachOrganizationRoleFromUser still works with historic param order', function () { + $organizationScope = OrganizationScope::whereName('Root Scope')->first(); + $organizationNode = OrganizationNode::whereName('Root Node')->first(); + + $role = $this->service->createRole([ + 'organization_scope_id' => $organizationScope->id, + 'type' => 'organization', + 'name' => 'Detach Legacy Test Role', + 'status' => 'active', + ]); + + $this->service->attachOrganizationRoleToUser($organizationNode->id, $role->id, 1); + + // Historic signature: ($userId, $roleId, $organizationNodeId) + $deleted = $this->service->detachOrganizationRoleFromUser(1, $role->id, $organizationNode->id); + + expect($deleted)->toBe(1); + + $pivotAfter = \Illuminate\Support\Facades\DB::table('user_role_organization_node') + ->where(['user_id' => 1, 'role_id' => $role->id, 'organization_node_id' => $organizationNode->id]) + ->count(); + expect($pivotAfter)->toBe(0); +}); + +test('legacy detachOrganizationRoleFromUser does not emit a runtime deprecation notice', function () { + $organizationScope = OrganizationScope::whereName('Root Scope')->first(); + $organizationNode = OrganizationNode::whereName('Root Node')->first(); + + $role = $this->service->createRole([ + 'organization_scope_id' => $organizationScope->id, + 'type' => 'organization', + 'name' => 'Detach Quiet Test Role', + 'status' => 'active', + ]); + + $this->service->attachOrganizationRoleToUser($organizationNode->id, $role->id, 1); + + $notices = []; + $prev = set_error_handler(function ($severity, $message) use (&$notices) { + if ($severity === E_USER_DEPRECATED || $severity === E_DEPRECATED) { + $notices[] = $message; + } + + return false; // let default handler also run for non-deprecated + }); + + try { + $this->service->detachOrganizationRoleFromUser(1, $role->id, $organizationNode->id); + } finally { + set_error_handler($prev); + } + + expect($notices)->toBeEmpty(); +}); + +test('detachOrganizationRoleFromUserBy throws InvalidUserException for unknown user', function () { + $organizationScope = OrganizationScope::whereName('Root Scope')->first(); + $organizationNode = OrganizationNode::whereName('Root Node')->first(); + + $role = $this->service->createRole([ + 'organization_scope_id' => $organizationScope->id, + 'type' => 'organization', + 'name' => 'Validation Test Role', + 'status' => 'active', + ]); + + $this->service->detachOrganizationRoleFromUserBy($organizationNode->id, $role->id, 99999); +})->throws(\AuroraWebSoftware\AAuth\Exceptions\InvalidUserException::class); + +test('detachOrganizationRoleFromUserBy throws InvalidRoleException for unknown organization role', function () { + $organizationNode = OrganizationNode::whereName('Root Node')->first(); + + $this->service->detachOrganizationRoleFromUserBy($organizationNode->id, 99999, 1); +})->throws(\AuroraWebSoftware\AAuth\Exceptions\InvalidRoleException::class); + +test('detachOrganizationRoleFromUserBy throws InvalidOrganizationNodeException for unknown node', function () { + $organizationScope = OrganizationScope::whereName('Root Scope')->first(); + + $role = $this->service->createRole([ + 'organization_scope_id' => $organizationScope->id, + 'type' => 'organization', + 'name' => 'Validation Node Test Role', + 'status' => 'active', + ]); + + $this->service->detachOrganizationRoleFromUserBy(99999, $role->id, 1); +})->throws(\AuroraWebSoftware\AAuth\Exceptions\InvalidOrganizationNodeException::class); + +test('detachOrganizationRoleFromUserBy clears user role cache when enabled', function () { + config(['aauth-advanced.cache.enabled' => true]); + config(['aauth-advanced.cache.prefix' => 'aauth']); + + $organizationScope = OrganizationScope::whereName('Root Scope')->first(); + $organizationNode = OrganizationNode::whereName('Root Node')->first(); + + $role = $this->service->createRole([ + 'organization_scope_id' => $organizationScope->id, + 'type' => 'organization', + 'name' => 'Cache Test Detach Role', + 'status' => 'active', + ]); + + $this->service->attachOrganizationRoleToUser($organizationNode->id, $role->id, 1); + + // Prime the cache + \Illuminate\Support\Facades\Cache::put('aauth:user:1:switchable_roles', ['sentinel'], 60); + expect(\Illuminate\Support\Facades\Cache::get('aauth:user:1:switchable_roles'))->toBe(['sentinel']); + + $this->service->detachOrganizationRoleFromUserBy($organizationNode->id, $role->id, 1); + + // The detach must have invalidated the cache key + expect(\Illuminate\Support\Facades\Cache::get('aauth:user:1:switchable_roles'))->toBeNull(); +}); + +test('both detach methods coexist on the service class', function () { + $reflection = new \ReflectionClass(RolePermissionService::class); + + expect($reflection->hasMethod('detachOrganizationRoleFromUser'))->toBeTrue(); + expect($reflection->hasMethod('detachOrganizationRoleFromUserBy'))->toBeTrue(); + + $legacy = $reflection->getMethod('detachOrganizationRoleFromUser'); + $canonical = $reflection->getMethod('detachOrganizationRoleFromUserBy'); + + expect($legacy->getNumberOfParameters())->toBe(3); + expect($canonical->getNumberOfParameters())->toBe(3); + + // Verify legacy has @deprecated in its docblock + expect($legacy->getDocComment())->toContain('@deprecated'); + expect($canonical->getDocComment())->not->toContain('@deprecated'); + + // Verify aligned-order parameter names on the canonical method + $params = $canonical->getParameters(); + expect($params[0]->getName())->toBe('organizationNodeId'); + expect($params[1]->getName())->toBe('roleId'); + expect($params[2]->getName())->toBe('userId'); }); test('can attach to role and detach a permission from role', function () { diff --git a/tests/Unit/V2/OctaneScopingTest.php b/tests/Unit/V2/OctaneScopingTest.php new file mode 100644 index 0000000..48ff95a --- /dev/null +++ b/tests/Unit/V2/OctaneScopingTest.php @@ -0,0 +1,241 @@ +run(); +}); + +/* +|-------------------------------------------------------------------------- +| Octane / Vapor request-scoped binding regression tests +|-------------------------------------------------------------------------- +| +| These tests do not require Octane to be installed. They simulate the +| Octane request boundary by calling `$app->forgetScopedInstances()`, which +| is exactly what the FlushTemporaryContainerInstances listener does on +| RequestTerminated. +| +*/ + +test('aauth is bound as scoped (not singleton) in the container', function () { + // The container distinguishes scoped from singleton at the abstract level + // via the $scopedInstances tracking. The easiest portable check is that + // forgetScopedInstances() drops the cached instance. + + $user = User::find(1); + $role = Role::whereName('Root Role 1')->first(); + + Auth::setUser($user); + Session::put('roleId', $role->id); + + $first = app('aauth'); + expect($first)->toBeInstanceOf(AAuth::class); + + // Second resolve in the same request returns the SAME instance (scoped lifetime) + $second = app('aauth'); + expect($second)->toBe($first); + + // After flushing scoped instances (simulated Octane boundary) the next + // resolve returns a NEW instance. + $this->app->forgetScopedInstances(); + + $third = app('aauth'); + expect($third)->not->toBe($first); +}); + +test('aauth scoped binding does not leak user context across simulated requests', function () { + // === First simulated request: User 1 with Root Role 1 === + $userA = User::find(1); + $roleA = Role::whereName('Root Role 1')->first(); + + Auth::setUser($userA); + Session::put('roleId', $roleA->id); + + /** @var AAuth $aauthA */ + $aauthA = app('aauth'); + + $userAId = $aauthA->user->id; + $roleAId = $aauthA->currentRole()->id; + $nodesA = $aauthA->organizationNodeIds(); + + expect($userAId)->toBe($userA->id); + expect($roleAId)->toBe($roleA->id); + + // === Simulated request boundary: Octane / Vapor flush === + $this->app->forgetScopedInstances(); + + // === Second simulated request: User 1 with a DIFFERENT role === + // (We use the same user but switch the active role — this is enough to + // prove the scoped binding does not carry state. The fact that the + // resolved instance is different + reflects the new session role proves + // the security fix.) + $roleB = Role::whereName('Sub-Scope Role 1')->first(); + + Auth::setUser($userA); + Session::put('roleId', $roleB->id); + + /** @var AAuth $aauthB */ + $aauthB = app('aauth'); + + expect($aauthB)->not->toBe($aauthA); + expect($aauthB->currentRole()->id)->toBe($roleB->id); + expect($aauthB->currentRole()->id)->not->toBe($roleAId); + + // Organization node IDs must reflect the NEW role's assignment, + // not the stale role A's node IDs. + $nodesB = $aauthB->organizationNodeIds(); + expect($nodesB)->toBeArray(); + // Either set may be empty; the critical guarantee is that nodesB was + // resolved fresh for role B, not copied from role A. + expect($nodesB)->not->toBe(null); +}); + +test('within a single simulated request the same aauth instance is reused', function () { + $user = User::find(1); + $role = Role::whereName('Root Role 1')->first(); + + Auth::setUser($user); + Session::put('roleId', $role->id); + + $first = app('aauth'); + $second = app('aauth'); + $third = app('aauth'); + + // Same request → same instance (no per-call reconstruction, performance preserved) + expect($second)->toBe($first); + expect($third)->toBe($first); +}); + +test('php-fpm equivalence: scoped binding behaves like singleton within a request', function () { + // This documents the BC guarantee for PHP-FPM consumers: within a single + // request, scoped() is observationally identical to singleton(). Each + // request gets exactly one constructor call. + + $user = User::find(1); + $role = Role::whereName('Root Role 1')->first(); + + Auth::setUser($user); + Session::put('roleId', $role->id); + + // Resolve repeatedly within "one request" + $instances = collect(range(1, 5))->map(fn () => app('aauth')); + + // All five resolves return the same instance + expect($instances->unique(fn ($i) => spl_object_id($i)))->toHaveCount(1); +}); + +test('SECURITY: public properties do not leak across simulated requests with different users', function () { + // This is the regression test for the reported security issue. Two DIFFERENT + // users with DIFFERENT roles MUST NOT share AAuth public properties across + // a simulated Octane request boundary. + + // === Request 1: User A (id=1) with org role 3 (Root Role 1) === + $userA = User::find(1); + $roleA = Role::whereName('Root Role 1')->first(); + + Auth::setUser($userA); + Session::put('roleId', $roleA->id); + + $aauthA = app('aauth'); + $leakedUserId = $aauthA->user->id; + $leakedRoleId = $aauthA->role->id; + $leakedNodeIds = $aauthA->organizationNodeIds; + $leakedCanResult = $aauthA->can('create_something_for_organization'); + + expect($leakedUserId)->toBe(1); + expect($leakedRoleId)->toBe($roleA->id); + + // === Octane request boundary === + $this->app->forgetScopedInstances(); + Auth::logout(); + + // === Request 2: User B (id=2) with a different role === + $userB = User::find(2); + + // Assign User B a fresh role that User A does NOT have, so a leak would + // be observable as User B keeping User A's permissions. + $roleB = \AuroraWebSoftware\AAuth\Models\Role::create([ + 'type' => 'system', + 'name' => 'User B Exclusive Role', + 'status' => 'active', + ]); + \Illuminate\Support\Facades\DB::table('user_role_organization_node')->insert([ + 'user_id' => $userB->id, + 'role_id' => $roleB->id, + ]); + + Auth::setUser($userB); + Session::put('roleId', $roleB->id); + + $aauthB = app('aauth'); + + // Hard assertions on the security-critical state + expect($aauthB)->not->toBe($aauthA, 'AAuth instance was reused across requests'); + expect($aauthB->user->id)->toBe(2, 'User identity leaked from previous request'); + expect($aauthB->user->id)->not->toBe($leakedUserId); + expect($aauthB->role->id)->toBe($roleB->id, 'Role leaked from previous request'); + expect($aauthB->role->id)->not->toBe($leakedRoleId); + + // User B has no `create_something_for_organization` permission — if the + // singleton had leaked, this would still return true from cached state. + expect($aauthB->can('create_something_for_organization'))->toBeFalse( + 'Permission decision leaked across users (the reported security bypass)' + ); +}); + +test('SECURITY: super admin flag does not leak across simulated requests', function () { + if (! \Illuminate\Support\Facades\Schema::hasColumn('users', 'is_super_admin')) { + \Illuminate\Support\Facades\Schema::table('users', function ($table) { + $table->boolean('is_super_admin')->default(false); + }); + } + config(['aauth-advanced.super_admin.enabled' => true]); + config(['aauth-advanced.super_admin.column' => 'is_super_admin']); + + // === Request 1: User A as super admin === + $userA = User::find(1); + $userA->is_super_admin = true; + $userA->save(); + $roleA = Role::whereName('Root Role 1')->first(); + + Auth::setUser($userA); + Session::put('roleId', $roleA->id); + + $aauthA = app('aauth'); + expect($aauthA->isSuperAdmin())->toBeTrue(); + expect($aauthA->can('anything'))->toBeTrue(); + + // === Octane request boundary === + $this->app->forgetScopedInstances(); + + // === Request 2: User B (not super admin) === + $userB = User::find(2); + $userB->is_super_admin = false; + $userB->save(); + + // User B needs an assigned role + $roleB = Role::whereName('Root Role 1')->first(); + \Illuminate\Support\Facades\DB::table('user_role_organization_node')->insert([ + 'user_id' => $userB->id, + 'role_id' => $roleB->id, + 'organization_node_id' => 1, + ]); + + Auth::setUser($userB); + Session::put('roleId', $roleB->id); + + $aauthB = app('aauth'); + + expect($aauthB->isSuperAdmin())->toBeFalse( + 'Super admin flag leaked across users — critical privilege escalation' + ); + expect($aauthB->can('non_existent_permission'))->toBeFalse(); +}); diff --git a/tests/Unit/V2/TransactionIntegrityTest.php b/tests/Unit/V2/TransactionIntegrityTest.php new file mode 100644 index 0000000..aad00c3 --- /dev/null +++ b/tests/Unit/V2/TransactionIntegrityTest.php @@ -0,0 +1,246 @@ +run(); + $this->service = new OrganizationService(); +}); + +/* +|-------------------------------------------------------------------------- +| Recursive Organization Service Transaction Integrity +|-------------------------------------------------------------------------- +| +| Previous implementation swallowed exceptions in try/catch and still +| committed the outer transaction, leaving the subtree partially mutated. +| The fix wraps recursion in DB::transaction() so any failure rolls back +| the entire subtree and re-throws to the caller. +| +*/ + +/** + * Build a 3-level subtree under the seeded root for tests. + * + * Returns: ['root' => OrganizationNode, 'children' => [OrganizationNode, ...]] + */ +function buildTestSubtree(OrganizationService $service): array +{ + $scope = OrganizationScope::whereName('Sub-Scope')->first(); + + $parent = $service->createOrganizationNode([ + 'name' => 'tx_test_parent', + 'organization_scope_id' => $scope->id, + 'parent_id' => 1, + ]); + + $childA = $service->createOrganizationNode([ + 'name' => 'tx_test_child_a', + 'organization_scope_id' => $scope->id, + 'parent_id' => $parent->id, + ]); + + $childB = $service->createOrganizationNode([ + 'name' => 'tx_test_child_b', + 'organization_scope_id' => $scope->id, + 'parent_id' => $parent->id, + ]); + + return ['root' => $parent, 'children' => [$childA, $childB]]; +} + +test('successful recursive delete removes the full subtree', function () { + $tree = buildTestSubtree($this->service); + + $idsBefore = OrganizationNode::whereIn('id', [ + $tree['root']->id, + $tree['children'][0]->id, + $tree['children'][1]->id, + ])->pluck('id')->toArray(); + expect($idsBefore)->toHaveCount(3); + + $this->service->deleteOrganizationNodesRecursively($tree['root']); + + $idsAfter = OrganizationNode::whereIn('id', [ + $tree['root']->id, + $tree['children'][0]->id, + $tree['children'][1]->id, + ])->pluck('id')->toArray(); + expect($idsAfter)->toBeEmpty(); +}); + +test('successful recursive path update writes all descendant paths', function () { + $tree = buildTestSubtree($this->service); + + $this->service->updateNodePathsRecursively($tree['root']); + + $reloadedParent = OrganizationNode::find($tree['root']->id); + $reloadedChildA = OrganizationNode::find($tree['children'][0]->id); + + // Parent path = root_path . parent_id + expect($reloadedParent->path)->toContain((string) $tree['root']->id); + + // Child path = parent_path . '/' . child_id + expect($reloadedChildA->path)->toContain($reloadedParent->path); + expect($reloadedChildA->path)->toEndWith((string) $tree['children'][0]->id); +}); + +test('mid-recursion failure rolls back the entire subtree delete', function () { + $tree = buildTestSubtree($this->service); + + // Register a transient deleting observer on OrganizationNode that throws + // when we try to delete the second child. The expected behaviour is: + // - The first child is deleted within the transaction. + // - The second child's delete raises an exception. + // - The transaction rolls back EVERYTHING — first child re-appears, + // parent is still there. + // - The exception bubbles up to the caller (no silent swallow). + + $throwOnId = $tree['children'][1]->id; + + OrganizationNode::deleting(function (OrganizationNode $node) use ($throwOnId) { + if ($node->id === $throwOnId) { + throw new \RuntimeException('Simulated mid-recursion failure'); + } + }); + + $thrown = null; + + try { + $this->service->deleteOrganizationNodesRecursively($tree['root']); + } catch (\Throwable $e) { + $thrown = $e; + } + + // Detach the observer so subsequent tests aren't affected + OrganizationNode::flushEventListeners(); + + expect($thrown)->not->toBeNull(); + expect($thrown->getMessage())->toBe('Simulated mid-recursion failure'); + + // All three nodes must still exist — the transaction rolled back + $survivors = OrganizationNode::whereIn('id', [ + $tree['root']->id, + $tree['children'][0]->id, + $tree['children'][1]->id, + ])->pluck('id')->toArray(); + expect($survivors)->toHaveCount(3); +}); + +test('mid-recursion failure rolls back path updates', function () { + $tree = buildTestSubtree($this->service); + + $throwOnId = $tree['children'][1]->id; + $originalRootPath = $tree['root']->path; + + // Saving observer throws unconditionally when child B is touched. We force + // the recursion to touch every node by mutating the root's path on disk + // first so the recursion has a different computed path to write everywhere. + OrganizationNode::saving(function (OrganizationNode $node) use ($throwOnId) { + if ($node->id === $throwOnId) { + throw new \RuntimeException('Simulated path-update failure'); + } + }); + + // Pre-mutate the root path directly via DB (bypassing observers) so the + // recursion has work to do — recomputed paths will differ from current. + \Illuminate\Support\Facades\DB::table('organization_nodes') + ->where('id', $tree['root']->id) + ->update(['path' => $originalRootPath . '-stale']); + + $thrown = null; + + try { + $freshRoot = OrganizationNode::find($tree['root']->id); + $this->service->updateNodePathsRecursively($freshRoot); + } catch (\Throwable $e) { + $thrown = $e; + } + + OrganizationNode::flushEventListeners(); + + expect($thrown)->not->toBeNull(); + expect($thrown->getMessage())->toBe('Simulated path-update failure'); + + // The pre-mutated stale path should still be in place — the recursive + // update rolled back without writing the recomputed path. + $reloadedRoot = OrganizationNode::find($tree['root']->id); + expect($reloadedRoot->path)->toBe($originalRootPath . '-stale'); +}); + +test('caller-managed transaction integrates: rolling back outer reverts subtree', function () { + $tree = buildTestSubtree($this->service); + + try { + DB::transaction(function () use ($tree) { + $this->service->deleteOrganizationNodesRecursively($tree['root'], false); + + // Force the outer transaction to roll back + throw new \RuntimeException('caller-rollback'); + }); + } catch (\RuntimeException $e) { + // expected + } + + // The subtree must still exist + $survivors = OrganizationNode::whereIn('id', [ + $tree['root']->id, + $tree['children'][0]->id, + $tree['children'][1]->id, + ])->pluck('id')->toArray(); + expect($survivors)->toHaveCount(3); +}); + +test('signature is backward compatible: $withDBTransaction parameter still accepted', function () { + $tree = buildTestSubtree($this->service); + + // Old callers passing explicit true must keep working + $this->service->deleteOrganizationNodesRecursively($tree['root'], true); + + $survivors = OrganizationNode::whereIn('id', [$tree['root']->id])->pluck('id')->toArray(); + expect($survivors)->toBeEmpty(); +}); + +test('deep recursion (3+ levels) is atomic on failure', function () { + // Build a 4-level subtree: root -> mid1 -> mid2 -> leaf + $scope = OrganizationScope::whereName('Sub-Scope')->first(); + + $root = $this->service->createOrganizationNode(['name' => 'deep_root', 'organization_scope_id' => $scope->id, 'parent_id' => 1]); + $mid1 = $this->service->createOrganizationNode(['name' => 'deep_mid1', 'organization_scope_id' => $scope->id, 'parent_id' => $root->id]); + $mid2 = $this->service->createOrganizationNode(['name' => 'deep_mid2', 'organization_scope_id' => $scope->id, 'parent_id' => $mid1->id]); + $leaf = $this->service->createOrganizationNode(['name' => 'deep_leaf', 'organization_scope_id' => $scope->id, 'parent_id' => $mid2->id]); + + $allIds = [$root->id, $mid1->id, $mid2->id, $leaf->id]; + + // Throw when trying to delete the leaf (deepest node, where DFS reaches first) + $throwOnId = $leaf->id; + + OrganizationNode::deleting(function (OrganizationNode $node) use ($throwOnId) { + if ($node->id === $throwOnId) { + throw new \RuntimeException('Deep recursion failure'); + } + }); + + $thrown = null; + + try { + $this->service->deleteOrganizationNodesRecursively($root); + } catch (\Throwable $e) { + $thrown = $e; + } + + OrganizationNode::flushEventListeners(); + + expect($thrown)->not->toBeNull(); + expect($thrown->getMessage())->toBe('Deep recursion failure'); + + // Every node must still exist — savepoint rollback worked across all levels + $survivors = OrganizationNode::whereIn('id', $allIds)->pluck('id')->toArray(); + expect($survivors)->toHaveCount(4); +});