Skip to content

Fix code audit findings: stale Gate memo in workers, model-backed writes, config validation - #1

Merged
pushpak1300 merged 3 commits into
mainfrom
claude/keen-fermat-qrpcpp
Sep 29, 2026
Merged

pushpak1300 merged 3 commits into
mainfrom
claude/keen-fermat-qrpcpp

Conversation

@pushpak1300

@pushpak1300 pushpak1300 commented Sep 29, 2026 •

Copy link
Copy Markdown
Member

What does this PR do?

This PR fixes 10 findings from a code-smell and architecture audit of the package. Each behaviour fix has a test that fails on main and passes here.

P0: Stale permissions in queue workers and Octane
The service provider resolved the scoped Grant once at boot and kept it inside every Gate ability, the super-admin hook and the saved/deleted listeners. Workers call forgetScopedInstances() between jobs and requests, so HasRoles got a fresh instance while the Gate kept reading the boot instance's memo for the life of the worker. After the first job, a revoked role still passed can() until the worker restarted. The closures now resolve Grant at call time.

P1: README invited unsupported usage
The README said HasRoles could go on any Eloquent model. Assignments are keyed only by user_id, so a second model would share roles with the user that has the same id. Both sentences now name auth.providers.users.model.

P2: Inconsistencies

  • #[Requires] denials now return the required permission's deniedMessage() instead of Laravel's generic message.
  • revoke() and syncRoles() now write through the assignment model, so a custom grant.model sees the same deleted events it already sees on grant(). syncRoles() no longer deletes and re-inserts rows that didn't change, so their ids stay stable.
  • grant.super_admin is validated like the other config keys. A value that isn't a case of the role enum now throws instead of silently turning the bypass off.
  • grant:show --on= prints global roles and scoped roles as separate rows. Before, a user with only global roles showed "Roles: None" next to a non-empty permission list.
  • Moving an assignment to another user now also clears the previous holder's cached roles.

P3 and tooling

  • Removed the always-true Gate::has filter from grant:list.
  • Updated the stale parts of plan.md (column storage mode, make:* commands, "no cache") and a README sentence that contradicted the Caching section.
  • Removed the Unit test suite from phpunit.xml.dist. It points at a tests/Unit directory that was never committed, which makes composer test:unit and vendor/bin/pest stop with "Test directory not found" on main too.

The bundled Boost skill now notes that #[Requires] denials carry the permission's message.

Type of change

  • Bug fix
  • New feature
  • Refactor / code cleanup
  • Documentation

Checklist

  • Tests added or updated (6 new tests, all failing against the old src/)
  • composer lint passes (Pint and Rector are clean under composer test:lint)
  • composer test passes: lint, PHPStan at max level, 100% type coverage, 26 tests / 101 assertions

🤖 Generated with Claude Code

https://claude.ai/code/session_01Li5c15yQmNGe1f2jZpMv5W

…g validation

- Resolve the scoped Grant inside Gate callbacks and model listeners so
  queue workers and Octane never read a stale boot-time memo.
- Return the required permission's denial response from #[Requires].
- Revoke and sync through the assignment model; sync keeps unchanged rows.
- Validate grant.super_admin like the other config keys.
- Flush the previous holder when an assignment changes user.
- Show global and scoped roles separately in grant:show.
- Drop the always-true Gate::has filter in grant:list.
- Correct README and plan.md claims, and remove the missing Unit suite.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Li5c15yQmNGe1f2jZpMv5W
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Li5c15yQmNGe1f2jZpMv5W
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Li5c15yQmNGe1f2jZpMv5W
@pushpak1300
pushpak1300 merged commit 5081529 into main Sep 29, 2026
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants