HQD-128: Make sure role client is a superset of admin, manager and support roles#79
Open
VadymHrechukha wants to merge 9 commits into
Conversation
…and `support` roles
Contributor
📝 WalkthroughWalkthroughReorganizes RBAC: expands client entitlements, reassigns blacklist and "see-no-mans" roles across staff/accounter/owner-staff, adds installment-plan permissions and internal flags, updates permission metadata/descriptions, and adjusts unit tests and a new contributor doc (CLAUDE.md). ChangesRBAC: permission metadata, role hierarchy, and tests
Sequence Diagram(s)(Skipped — changes are metadata/role/test updates without a new multi-component runtime sequence.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.54)Composer install failed: this project depends on private packages that require authentication (e.g. GitLab/GitHub, Laravel Nova, etc.). Instead, run PHPStan in a CI/CD pipeline where you can use custom packages — our pipeline remediation tool can use the PHPStan output from your CI/CD pipeline. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/files/source/tree.php`:
- Around line 391-393: The role hierarchy change made in tree.php now has
'role:client' inherit 'role:support', 'role:admin', and 'role:manager', so
update the testClient() expectations to reflect that role:client grants
admin/manager/support permissions (e.g., admin, manage, client.read,
client.list, ticket.update/delete) or alternatively change the role hierarchy if
that was unintended; locate the role array in tree.php and the testClient()
method in the tests and modify assertions to assert presence of those
permissions (or adjust role composition) so the test aligns with the new
hierarchy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
src/files/source/tree.php — role:client now includes role:support, role:admin, role:manager as children. tests/unit/CheckAccessTrait.php: - testClient() — expanded to the full union of all permissions from client + support + admin + manager (~80 permissions) - testLimited() — expanded to reflect the same full union minus the 13 explicitly denied permissions tests/unit/AuthManagerTest.php — added role:client to the expected internal items in testIsRoleInternal, because isItemInternal propagates recursively and role:client now inherits internal items (e.g. blacklist.* from role:support). This is consistent with role:owner, role:reseller, role:master which are already in that list for the same reason. src/files/source/metadata.php: - role:accounter — added 'internal' => true - domain.delete-agp — updated description to the one specified in the ticket TODOs
…s should not get blacklist permissions. Following the HP-2069 pattern, I need to move it to staff-specific roles. Let me check all affected tests first: The fix: remove role:blacklist.manager from role:support and add it explicitly to role:staff-admin, role:staff-manager, and role:accounter.
1. role:finance.reader — bill.read listed twice 2. role:stock.master — role:move.master listed twice 3. role:owner-staff — part.read-all-hierarchy listed twice
…because role:admin and role:manager already have role:support
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/files/source/tree.php (1)
543-556: 💤 Low value
role:owner-staffcomposition — verify duplication withrole:staff-admin/role:staff-manager
role:owner-staffandrole:staff-admin/role:staff-managernow both directly includerole:blacklist.managerandsee-no-mans. Ifrole:owner-staffis intended to inherit a broader staff scope, consider composing it fromrole:staff-admin(or similar) instead of re-listing overlapping entitlements; this reduces drift risk if staff role contents evolve. Skip if the current flat composition is deliberate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/files/source/tree.php` around lines 543 - 556, The ACL for 'role:owner-staff' duplicates entitlements already present on staff roles (e.g. 'role:blacklist.manager' and 'see-no-mans'), increasing drift risk; update the definition by composing/inheriting from the existing staff role(s) instead of repeating overlapping entries — replace the explicit duplicated rights in 'role:owner-staff' with a reference to 'role:staff-admin' or 'role:staff-manager' (or a merged include of those role symbols) so 'role:owner-staff' derives staff permissions centrally and only declares owner-specific entitlements.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/files/source/tree.php`:
- Around line 543-556: The ACL for 'role:owner-staff' duplicates entitlements
already present on staff roles (e.g. 'role:blacklist.manager' and
'see-no-mans'), increasing drift risk; update the definition by
composing/inheriting from the existing staff role(s) instead of repeating
overlapping entries — replace the explicit duplicated rights in
'role:owner-staff' with a reference to 'role:staff-admin' or
'role:staff-manager' (or a merged include of those role symbols) so
'role:owner-staff' derives staff permissions centrally and only declares
owner-specific entitlements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2cc0891-9d4e-4b0f-b3b6-0464b73250f6
📒 Files selected for processing (3)
src/files/items.phpsrc/files/source/metadata.phpsrc/files/source/tree.php
SilverFire
requested changes
May 12, 2026
| ], | ||
| 'document.acceptance' => [ | ||
| 'description' => 'Access acceptance documents', | ||
| 'description' => 'Sign and view service acceptance documents', |
Member
There was a problem hiding this comment.
Suggested change
| 'description' => 'Sign and view service acceptance documents', | |
| 'description' => 'View acceptance reports for employees', |
| 'document.read', 'document.create', 'document.invoice', | ||
| 'contact.read', 'contact.create', 'contact.update', 'contact.delete', | ||
| 'certificate.read', 'certificate.create', 'certificate.update', 'certificate.pay', 'certificate.push', 'certificate.delete', | ||
| 'document.read', 'document.create', 'document.invoice', 'document.update', 'document.delete', 'document.generate', 'document.acceptance', |
Member
There was a problem hiding this comment.
document.acceptance– should be only inrole:employeeandrole:employee-managerdocument.generate– only instaff-manageranddocument.masterdocument.invoice– consult with @bladeroot, seems to be unused and should be removeddocument.update,document.delete– only instaff-manageranddocument.mastercertificate.delete– consult with @bladeroot
| 'restore-password', 'deposit', 'have-goods', 'pay', | ||
| 'ticket.read', 'ticket.create', 'ticket.answer', 'ticket.close', | ||
| 'domain.read', 'domain.update', 'domain.pay', 'domain.push', 'domain.delete-agp', 'domain.set-nss', | ||
| 'access-subclients', 'support', 'admin', 'manage', 'access-reseller', 'client.notify', |
Member
There was a problem hiding this comment.
access-reseller– definitely not for client. Client should never access it's reseller!client.notify– See API, seems to be staff permission only
| 'ticket.read', 'ticket.create', 'ticket.answer', 'ticket.close', | ||
| 'domain.read', 'domain.update', 'domain.pay', 'domain.push', 'domain.delete-agp', 'domain.set-nss', | ||
| 'access-subclients', 'support', 'admin', 'manage', 'access-reseller', 'client.notify', | ||
| 'ticket.read', 'ticket.create', 'ticket.answer', 'ticket.close', 'ticket.update', 'ticket.delete', |
Member
There was a problem hiding this comment.
ticket.delete– I'd not allow this. Staff permission
| 'domain.read', 'domain.update', 'domain.pay', 'domain.push', 'domain.delete-agp', 'domain.set-nss', | ||
| 'access-subclients', 'support', 'admin', 'manage', 'access-reseller', 'client.notify', | ||
| 'ticket.read', 'ticket.create', 'ticket.answer', 'ticket.close', 'ticket.update', 'ticket.delete', | ||
| 'ticket.read-templates', 'ticket.read-statistics', 'ticket.set-private', 'ticket.set-recipient', 'ticket.set-time', |
Member
There was a problem hiding this comment.
Consult with @bladeroot on these permissions. I think they all are staff-only.
| 'certificate.read', 'certificate.create', 'certificate.update', 'certificate.pay', 'certificate.push', | ||
| 'document.read', 'document.create', 'document.invoice', | ||
| 'contact.read', 'contact.create', 'contact.update', 'contact.delete', | ||
| 'certificate.read', 'certificate.create', 'certificate.update', 'certificate.pay', 'certificate.push', 'certificate.delete', |
Member
There was a problem hiding this comment.
certificate.deleteis for staff only. @bladeroot, correct?
| 'contact.read', 'contact.create', 'contact.update', 'contact.delete', | ||
| 'certificate.read', 'certificate.create', 'certificate.update', 'certificate.pay', 'certificate.push', 'certificate.delete', | ||
| 'document.read', 'document.create', 'document.invoice', 'document.update', 'document.delete', 'document.generate', 'document.acceptance', | ||
| 'contact.read', 'contact.create', 'contact.update', 'contact.delete', 'contact.set-verified', 'contact.force-verify', |
Member
There was a problem hiding this comment.
contact.set-verified,contact.force-verify– staff only for sure. You've set description Force-mark a contact as verified, bypassing the normal verification process. It's a clear sign of an internal permission.
| 'document.read', 'document.create', 'document.invoice', 'document.update', 'document.delete', 'document.generate', 'document.acceptance', | ||
| 'contact.read', 'contact.create', 'contact.update', 'contact.delete', 'contact.set-verified', 'contact.force-verify', | ||
| 'server.read', 'server.pay', 'server.control-power', 'server.control-system', 'server.set-note', | ||
| 'server.wizzard', 'server.set-label', 'server.manage-settings', 'server.see-label', 'server.move-disks', |
Member
There was a problem hiding this comment.
All of these should be internal and fall into staff-roles.
server.see-labelandserver.set-labelare for internal server labels. This should be available for bothstaff-managerandstaff-admin.server.move-disksandserver.manage-settings– forstaff-admin.
| 'contact.read', 'contact.create', 'contact.update', 'contact.delete', 'contact.set-verified', 'contact.force-verify', | ||
| 'server.read', 'server.pay', 'server.control-power', 'server.control-system', 'server.set-note', | ||
| 'server.wizzard', 'server.set-label', 'server.manage-settings', 'server.see-label', 'server.move-disks', | ||
| 'server.read-wizzard', 'server.read-legend', 'server.read-system-info', 'server.read-financial-info', 'server.read-billing', |
| 'server.read', 'server.pay', 'server.control-power', 'server.control-system', 'server.set-note', | ||
| 'server.wizzard', 'server.set-label', 'server.manage-settings', 'server.see-label', 'server.move-disks', | ||
| 'server.read-wizzard', 'server.read-legend', 'server.read-system-info', 'server.read-financial-info', 'server.read-billing', | ||
| 'server.sell', 'server.enable-block', 'server.disable-block', |
Member
There was a problem hiding this comment.
server.enable-block,server.disable-block– staff onlyserver.sell–staff-manageronly
SilverFire
requested changes
May 13, 2026
| ], | ||
| 'access-reseller' => [ | ||
| 'description' => 'Allows access-reseller operation', | ||
| 'description' => 'Grants access to reseller-level functionality, allowing the user to manage clients within their reseller scope', |
Member
There was a problem hiding this comment.
Suggested change
| 'description' => 'Grants access to reseller-level functionality, allowing the user to manage clients within their reseller scope', | |
| 'description' => 'Grants access not only to the clients within their reseller scope, but to objects that belong the the reseller itself. Should be assigned to managers and admins of the resellers.', |
| ], | ||
| 'domain.force-push' => [ | ||
| 'description' => 'Force push domains', | ||
| 'description' => 'Force-push domains to another registrar account, bypassing normal restrictions', |
Member
There was a problem hiding this comment.
Suggested change
| 'description' => 'Force-push domains to another registrar account, bypassing normal restrictions', | |
| 'description' => 'Force-push domains to another client, bypassing normal restrictions', |
| ], | ||
| 'domain.push' => [ | ||
| 'description' => 'Push domains', | ||
| 'description' => 'Push (transfer) domains to another registrar account', |
Member
There was a problem hiding this comment.
Suggested change
| 'description' => 'Push (transfer) domains to another registrar account', | |
| 'description' => 'Push (transfer) domains to another client', |
| ], | ||
| 'part.read-administrative' => [ | ||
| 'description' => 'Reading parts administrative data', | ||
| 'description' => 'Read administrative data of hardware parts (e.g. purchase cost, supplier info)', |
Member
There was a problem hiding this comment.
Fun fact: not used, should be removed.
| ], | ||
| 'plan.force-read' => [ | ||
| 'description' => 'Read tariff plans additional data', | ||
| 'description' => 'Read restricted tariff plan details not visible to the plan owner (e.g. internal pricing data)', |
Member
There was a problem hiding this comment.
Fun fact: not used, should be removed.
| ], | ||
| 'purse.set-credit' => [ | ||
| 'description' => 'Set purse credit', | ||
| 'description' => 'Set a credit limit on a client\'s purse (wallet)', |
Member
There was a problem hiding this comment.
Suggested change
| 'description' => 'Set a credit limit on a client\'s purse (wallet)', | |
| 'description' => 'Set a credit limit on a client\'s purse', |
| ], | ||
| 'ref.view.not-used' => [ | ||
| 'description' => 'Read not used refs', | ||
| 'description' => 'View unused internal reference entries (internal technical permission)', |
Member
There was a problem hiding this comment.
Suggested change
| 'description' => 'View unused internal reference entries (internal technical permission)', | |
| 'description' => 'See the whole type tree not limited to types, that are used by client. E.g. see all bill types, including the internal ones.', |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Permissions
Documentation
Tests