Skip to content

feat(roles): projects.viewAll and roles.manage; new permissions reach custom roles - #3944

Merged
marcelfolaron merged 2 commits into
masterfrom
feature/custom-roles-phase2
Oct 18, 2026
Merged

marcelfolaron merged 2 commits into
masterfrom
feature/custom-roles-phase2

Conversation

@marcelfolaron

@marcelfolaron marcelfolaron commented Oct 18, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Phase 2 of custom roles, on top of #3940. Two abilities that were tied to the admin rank become permissions a role can hold, and custom roles keep up with permissions that later updates add.

  • projects.viewAll (global): see every project without being a member. Replaces the hard-coded "admin and above" rule.
  • roles.manage (global): create, change and delete roles. Core has no screens for it; the custom roles plugin checks it.
  • Both are granted to admins and owners, the roles that had these abilities, so nothing changes for existing installs.

Changes

  • projects.viewAll now decides "sees all projects" at all nine sites that made that decision:
    • the engine's membership bypass (PermissionService::canAccessAllProjects → new roleSeesAllProjects(), reads the grant map directly, so no recursion through currentUserCan)
    • the six SQL access filters (Tickets ×4, Projects, ReportEngine) via Roles::keysThatSeeAllProjects()
    • Projects repository isUserAssignedToProject, the project-list filter and search via Roles::seesAllProjects(); the All Projects page uses the filtered getAll() for admin-level roles and viewAll holders, the client view for others
    • With unseeded grant tables (or no database) it falls back to the admin rank, so project lists don't empty out. Every engine check already denies in that state.
  • RolesPermissions (roles.manage) in Domain/Auth/Permissions.
  • Seeder: syncDiscoveredPermissions() grants the permissions that are new in this sync to every role that gets them by default (DefaultRolePermissions): built-in roles their own defaults, custom roles their base role's. Existing grants are never touched, so permissions an admin removed stay removed. Snapshot, sync and grants run in one transaction (cache flushed after commit), so a failed sync can be retried.
  • Migration 30533 (dbVersion 3.5.33): flushes the provider cache (roles.manage comes from a new class) and syncs. No reseed of the built-in roles.
  • Other userIsAtLeast(admin) checks ("act on another user's data", API keys, access tokens) are a different rule and stay for Phase 3.

Testing

  • Unit suite: 1948 tests green. New: PermissionServiceViewAllTest (custom role with viewAll skips membership; admin-level role without it needs membership; unseeded fallback), PermissionSeederCustomRolesTest (on SQLite: new permissions reach custom roles by base role; removed grants stay removed after a reseed; built-in roles get new permissions without revoked grants coming back; a failed sync can be retried; base role derived from level), UpdateSql30533Test, and a matrix assertion that only admin/owner get the two new permissions. PermissionServiceModulesTest fixtures now give their admin projects.viewAll, as real installs get it from the migration.
  • PHPStan + Pint clean.
  • Checked on a copy of a dev database after db:migrate:
    • both permissions granted to admin and owner only; the editor-based custom qa_lead role got neither
    • QA Lead without projects.viewAll: Projects.getProject(4) (not a member) → -32001, 8 accessible projects; with it: project 4 readable, all 32 accessible
    • owner: 32 accessible projects; dashboard, All Projects, search, users, project, kanban and reports pages render

🤖 Generated with Claude Code

… custom roles

Phase 2 of custom roles.
- "Sees every project without being a member" is now the global permission
  projects.viewAll instead of "admin and above". The engine's membership
  bypass, the six SQL access filters, isUserAssignedToProject, the project
  list filter, search and the All Projects page all read it (via
  PermissionService::roleSeesAllProjects / Roles::seesAllProjects /
  Roles::keysThatSeeAllProjects). Unseeded grant tables fall back to the old
  admin rule so project lists don't empty out.
- roles.manage (global) for the custom roles plugin.
- Both are granted to admins and owners, the roles that had these abilities.
- syncDiscoveredPermissions() gives each custom role the NEW permissions its
  base role gets by default, so features added by an update work for custom
  roles; grants an admin removed are never added back.
- Migration 30533 (dbVersion 3.5.33) rediscovers providers, syncs, seeds.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DqVeL6FWt71mr3sFBw3eSQ
@marcelfolaron
marcelfolaron requested a review from a team as a code owner October 18, 2026 14:38
@marcelfolaron
marcelfolaron requested review from broskees and a balanced review from Copilot and removed request for a team October 18, 2026 14:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Access restrictions and grant synchronization need fixes to prevent private-project exposure, restored revocations, and incomplete grants after retries.

3 open issues
What changed in this PR

Extends custom roles with permission-based project visibility, role management, and automatic grants for newly introduced permissions.

Changes:

  • Adds projects.viewAll and roles.manage.
  • Updates project access checks to use permission grants.
  • Adds migration 30533 and regression coverage.
File Description
tests/​Unit/​app/​Domain/​Install/​UpdateSql30533Test.php Tests migration sequencing.
tests/​Unit/​app/​Core/​Auth/​Permissions/​PermissionServiceViewAllTest.php Tests permission-based project access.
tests/​Unit/​app/​Core/​Auth/​Permissions/​PermissionServiceModulesTest.php Updates admin grant fixtures.
tests/​Unit/​app/​Core/​Auth/​Permissions/​PermissionSeederCustomRolesTest.php Tests custom-role grant inheritance.
tests/​Unit/​app/​Core/​Auth/​Permissions/​DefaultRolePermissionsTest.php Tests new permission defaults.
app/​Domain/​Tickets/​Repositories/​Tickets.php Updates ticket access predicates.
app/​Domain/​Search/​Services/​Search.php Updates search visibility checks.
app/​Domain/​Reports/​Repositories/​ReportEngine.php Updates report access predicates.
app/​Domain/​Projects/​Services/​Projects.php Updates project-list filtering.
app/​Domain/​Projects/​Repositories/​Projects.php Updates project access checks.
app/​Domain/​Projects/​Permissions/​ProjectsPermissions.php Defines projects.viewAll.
app/​Domain/​Projects/​Controllers/​ShowAll.php Changes project-list selection.
app/​Domain/​Install/​Repositories/​Install.php Adds migration 30533.
app/​Domain/​Auth/​Permissions/​RolesPermissions.php Defines roles.manage.
app/​Domain/​Auth/​Models/​Roles.php Adds project-visibility helpers.
app/​Core/​Configuration/​AppSettings.php Advances database version.
app/​Core/​Auth/​Permissions/​PermissionService.php Uses grants for membership bypass.
app/​Core/​Auth/​Permissions/​PermissionSeeder.php Grants new permissions to custom roles.

Review effort: Balanced

Comment thread app/Domain/Install/Repositories/Install.php Outdated
Comment thread app/Domain/Projects/Controllers/ShowAll.php Outdated
Comment thread app/Core/Auth/Permissions/PermissionSeeder.php Outdated
Review follow-ups on #3944:
- syncDiscoveredPermissions() now gives NEW permissions to built-in roles too
  (their own defaults), so migration 30533 only syncs. It no longer reseeds
  the built-in roles, which would have restored grants an admin removed.
- Snapshot, sync and grants run in one transaction (new
  PermissionRepository::transaction), cache flushed after commit, so a failed
  sync can be retried without losing the new keys' grants.
- All Projects page: admin-level roles keep getAll() (already filtered by
  projects.viewAll), roles with projects.viewAll get it too; others keep the
  client view. Admin-level roles without viewAll no longer fall through to
  the unfiltered client-projects query.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DqVeL6FWt71mr3sFBw3eSQ
Copilot AI balanced review requested due to automatic review settings October 18, 2026 15:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

All Projects still blocks lower-ranked permission holders, and access-filter construction repeatedly fetches the full grant map.

1 open issue
1 previously missed issue

In code that hasn't changed since last review

Medium severity Avoid repeated permission map cache reads per role

app/​Domain/​Auth/​Models/​Roles.php:210

Each iteration calls PermissionService::map(), which fetches the entire grant map from the cache rather than retaining an in-memory copy. With populated grants, this means six cache reads for the built-in roles alone, plus one per custom role, each time a ticket, project, or report access predicate is built. On Redis this adds a round trip and full-map deserialization per role. Fetch the map once per invocation through a bulk lookup in PermissionService, then derive the matching role keys from that snapshot. Preserve the unseeded fallback without introducing a stale process-wide cache.

3 resolved since last review

Review effort: Balanced

Comment thread app/Domain/Projects/Controllers/ShowAll.php
@marcelfolaron
marcelfolaron merged commit 6dcd8be into master Oct 18, 2026
13 of 14 checks passed
@marcelfolaron
marcelfolaron deleted the feature/custom-roles-phase2 branch October 18, 2026 15:41
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