chore(rbac): actualize metadata.php descriptions and internal flags#80
chore(rbac): actualize metadata.php descriptions and internal flags#80SilverFire wants to merge 4 commits into
Conversation
Replace boilerplate "The role is generally assigned to..." descriptions with functional summaries derived from actual code usage. Fix incorrect internal flags for permissions accessible to resellers/clients. Fix typos. Key changes: - All role:* descriptions rewritten to describe what permissions they grant - Add internal=true: bill.import, bill.create-exchange, client.impersonate, client.set-roles, client.set-others-allowed-ips, config.*, domain.freeze, domain.unfreeze, domain.force-push, domain.force-set-nss, domain.force-send-foa, domain.approve-trasfer-out, domain.maintain, hub.sell, part.sell, model.create/update/delete, move.delete/update - Remove internal=true: consumption.update, consumption.delete (reseller-accessible) - Fix typos: "Maually"→"Manually", "maual"→"manual", "eraseing"→"erasing", "trasfer"→"transfer" in domain.approve-trasfer-out description - Improve descriptions for server.wizzard, server.sell, server.read-legend, mailing.prepare/send, see-no-mans, ref.view.not-used, and others Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR rewrites RBAC permission and role metadata, recomposes several composite roles, updates the role-to-permission tree, aligns unit tests with new ChangesRBAC metadata, role bundles, tree, tests, and dev deps
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)
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}
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/metadata.php`:
- Around line 142-143: The alias description for 'role:plan.master' is too
narrow compared to 'role:plan.manager'; update the 'description' value for
'role:plan.master' so it mirrors 'role:plan.manager' by including force-read
access and plan notes in addition to full CRUD access to tariff plans and prices
(i.e., state it as an alias for plan.manager and list all capabilities: full
CRUD, force-read, and plan notes access); edit the 'role:plan.master' entry in
metadata.php to match the manager description exactly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e6891bc8-ad0a-4850-9e52-13bb74ef6adb
📒 Files selected for processing (1)
src/files/source/metadata.php
| 'role:plan.master' => [ | ||
| 'description' => 'The role is generally assigned to staff who have exceptionally high permissions for the tariff plans management', | ||
| 'description' => 'Alias for plan.manager; grants full CRUD access to tariff plans and prices.', |
There was a problem hiding this comment.
Keep the alias summary in sync with role:plan.manager.
Line 143 narrows the role too much: role:plan.manager also includes force-read access and plan notes, so this alias description is currently incomplete.
Suggested fix
'role:plan.master' => [
- 'description' => 'Alias for plan.manager; grants full CRUD access to tariff plans and prices.',
+ 'description' => 'Alias for plan.manager; grants full CRUD access to tariff plans and prices, including force-read and plan notes.',
],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'role:plan.master' => [ | |
| 'description' => 'The role is generally assigned to staff who have exceptionally high permissions for the tariff plans management', | |
| 'description' => 'Alias for plan.manager; grants full CRUD access to tariff plans and prices.', | |
| 'role:plan.master' => [ | |
| 'description' => 'Alias for plan.manager; grants full CRUD access to tariff plans and prices, including force-read and plan notes.', |
🤖 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/metadata.php` around lines 142 - 143, The alias description
for 'role:plan.master' is too narrow compared to 'role:plan.manager'; update the
'description' value for 'role:plan.master' so it mirrors 'role:plan.manager' by
including force-read access and plan notes in addition to full CRUD access to
tariff plans and prices (i.e., state it as an alias for plan.manager and list
all capabilities: full CRUD, force-read, and plan notes access); edit the
'role:plan.master' entry in metadata.php to match the manager description
exactly.
role:client now includes role:manager, which includes role:admin, which includes role:support. All three roles are stripped of staff-only permissions that were never safe for external clients (mailing, client blocking, purse control, ticket deletion, blacklist management, etc.). Staff-only permissions moved to role:staff-admin and role:staff-manager. role:accounter now inherits role:staff-manager instead of role:manager. role:reseller gets back the management permissions it needs without picking up strictly-internal ones (mailing, blacklist). key changes: - role:ticket.user gains ticket.update (clients can edit own tickets) - server.wizzard/move-disks moved to role:server.staff-admin - role:support loses ticket.manager and blacklist.manager (both staff-only) - role:admin stripped of admin primitive, hub, stock, hosting.admin - role:staff-admin absorbs all the removed admin/support staff perms - role:staff-manager fully restructured around role:manager + staff adds - CheckAccessTrait tests rewritten for all affected roles - AuthManagerTest.testIsRoleInternal updated: role:admin/manager/support no longer internal; new internal flags for purse.update, contact.set-verified, client blocking, ticket deletion, etc. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- README.md: full rewrite covering architecture, role hierarchy, the internal flag, deny: pattern, client/staff boundary, and the add-permission workflow - CLAUDE.md: AI context file with key invariants, role compositions, gotchas, and the exact reinit+test workflow - docs/role-hierarchy.md: net permission sets for each top-level role - docs/client-vs-staff.md: decision log for every permission boundary established during the 2025-05 security audit - docs/internal-permissions.md: reference table of all staff-only permissions with reasons - docs/workflow.md: step-by-step guide for adding permissions and roles Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/files/items.php (1)
2918-2929:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate
access-subclientsdescription to clarify that it is granted torole:clientand beyond.The description at lines 2918-2929 states "Simple clients are NOT granted with this permission," but this contradicts the code:
role:client(line 879) →role:manager(line 973) →role:admin(line 905) →role:support(line 889) →access-subclients(line 893)testClient()intests/unit/CheckAccessTrait.phpexplicitly asserts thatrole:clienthasaccess-subclientsEither the description must be updated to reflect that
role:clientand all manager-level roles grant this permission, or therole:clientinheritance chain should be reconsidered. Given the existing test expectations and the breadth of permissions now flowing intorole:client, update the metadata description to accurately reflect the current permission model.🤖 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/items.php` around lines 2918 - 2929, The permission metadata for 'access-subclients' is incorrect: update the 'description' for 'access-subclients' to reflect that this permission is granted to role:client and any roles that inherit from it (e.g., role:manager, role:admin, role:support), removing the line "Simple clients are NOT granted with this permission" and adding a short note that clients and higher-level roles can access subclients; keep 'deny:access-subclients' as-is. Reference 'access-subclients', 'deny:access-subclients', and the role names (role:client, role:manager, role:admin, role:support) when editing the description so it matches tests like testClient().
🤖 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 `@composer.json`:
- Around line 52-53: Add a PHP version constraint and update the PHPUnit config:
in composer.json add "php": "^8.3" to the "require" section so phpunit/phpunit
(^12.5) and nikic/php-parser (^5.7) can be installed, and edit phpunit.xml.dist
to remove deprecated attributes (backupGlobals, backupStaticAttributes) and the
old <whitelist> element, replacing them with the new <coverage> structure (use
<coverage> with <include> rules) compatible with PHPUnit 12; target the
phpunit/phpunit and phpunit.xml.dist changes together to ensure tests run on PHP
≥8.3.
In `@src/files/items.php`:
- Around line 891-916: Descriptions in items.php no longer match the actual role
children after the restructure; update the source role metadata in
src/files/source/metadata.php (then regenerate items.php) so descriptions
reflect the current composition: remove references to removed children (e.g.,
delete “blacklist management” from role:support and stop calling
DNS/certificate/contact/server/hosting access “read-only” if their child roles
grant CRUD), remove claims in role:admin about hub/stock/hosting CRUD that
aren’t granted by its children (role:support, role:server.admin), add the newly
granted children to role:staff-admin’s description (role:hub.user,
role:ticket.manager, role:blacklist.manager, role:hosting.admin,
role:consumption.manager), correct role:accounter’s parent wording to “extends
role:staff-manager,” and update role:manager and role:reseller descriptions to
reflect only the permissions their actual children grant (remove mentions of
certificate manager/consumption/mailing/target/contact management where not
present, and add newly included children like role:certificate.manager,
role:ticket.manager, role:consumption.manager, role:target.manager where
applicable); after editing the textual descriptions for these specific role keys
(role:support, role:admin, role:staff-admin, role:accounter, role:manager,
role:reseller, role:server.admin) regenerate items.php so the generated artifact
matches the source.
- Around line 1666-1681: Add the missing 'internal' => true flag to the two
permission entries 'server.wizzard' and 'server.move-disks' so they match the
other staff-only permissions grouped under role:server.staff-admin; update the
permissions array entries for 'server.wizzard' and 'deny:server.move-disks' (if
applicable) to include 'internal' => true and then add 'server.wizzard' and
'server.move-disks' to the $expectedInternalItems array in
tests/unit/AuthManagerTest.php::testIsRoleInternal to keep the unit test in
sync.
---
Outside diff comments:
In `@src/files/items.php`:
- Around line 2918-2929: The permission metadata for 'access-subclients' is
incorrect: update the 'description' for 'access-subclients' to reflect that this
permission is granted to role:client and any roles that inherit from it (e.g.,
role:manager, role:admin, role:support), removing the line "Simple clients are
NOT granted with this permission" and adding a short note that clients and
higher-level roles can access subclients; keep 'deny:access-subclients' as-is.
Reference 'access-subclients', 'deny:access-subclients', and the role names
(role:client, role:manager, role:admin, role:support) when editing the
description so it matches tests like testClient().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 658fddf8-262f-45e2-9620-13d101998d3d
📒 Files selected for processing (5)
composer.jsonsrc/files/items.phpsrc/files/source/tree.phptests/unit/AuthManagerTest.phptests/unit/CheckAccessTrait.php
| "phpunit/phpunit": "^12.5", | ||
| "nikic/php-parser": "^5.7", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Surface PHPUnit/php-parser API usages that may be impacted.
rg -nP --type=php -C2 '@dataProvider|expectDeprecation|expectExceptionMessageRegExp|setMethods\(|withConsecutive\(|getMockBuilder\('
rg -nP --type=php -C2 'PhpParser\\(Node|NodeAbstract|PrettyPrinter|Parser|ParserFactory)\b'
fd -e php . tests | xargs rg -nP 'extends\s+(TestCase|\\\\PHPUnit\\\\Framework\\\\TestCase)'Repository: hiqdev/hipanel-rbac
Length of output: 1071
🌐 Web query:
PHPUnit 12 breaking changes from PHPUnit 9 upgrade guide
💡 Result:
PHPUnit 12 breaking changes from a PHPUnit 9 upgrade (what will likely break, and what to change) 1) PHPUnit 12 requires PHP 8.3+ PHPUnit 12 requires PHP 8.3 or later, so you must upgrade your runtime if you’re still on PHP 8.1/8.2. [1] 2) Test metadata via doc-comments/annotations is removed (must use PHP attributes) PHPUnit 11 deprecated the old “special PHP comments” approach for metadata, and PHPUnit 12 requires migrating to PHP 8 attributes. [1][2] 3) Test doubles for abstract classes and traits are removed The methods for creating mock objects for abstract classes and traits were removed in PHPUnit 12, and PHPUnit 12 no longer supports testing those in isolation. [1][2] 4) Expectations configured on objects created with createStub no longer work In PHPUnit 12, you can no longer configure expectations on objects created with createStub; this is explicitly called out as “no longer possible” in the PHPUnit 12 announcement. [1] How to upgrade (high-signal checklist to apply to your PHPUnit 9 suite) - First ensure your project runs on PHP 8.3+ (required for PHPUnit 12). [1] - Remove/replace any doc-comment/annotation-based PHPUnit metadata with PHP attributes. [1][2] - Refactor tests that create test doubles for abstract classes/traits “in isolation” (those mock/stub creation methods are removed). [1][2] - Search for createStub usage and remove expectation configuration on those stubs; use a mock creation approach that supports expectations instead. [1] - Don’t skip the “fix deprecations on PHPUnit 11.5 before upgrading to 12” guidance: PHPUnit explicitly says you should not even try to upgrade to PHPUnit 12 if you can’t run your test suite on PHPUnit 11.5 without deprecation warnings. [1] If you share your current PHPUnit 9 phpunit.xml and a quick grep list of the patterns above (createStub(, any “@Covers”, “@uses”, or other doc-comment metadata), I can map each item to the specific PHPUnit 12 failure mode and propose refactors.
Citations:
- 1: https://phpunit.de/announcements/phpunit-12.html
- 2: https://phpunit.de/announcements/phpunit-11.html
🏁 Script executed:
cat -n composer.jsonRepository: hiqdev/hipanel-rbac
Length of output: 3165
🏁 Script executed:
fd -e xml . | grep -i phpunitRepository: hiqdev/hipanel-rbac
Length of output: 45
🏁 Script executed:
fd -e php tests/ 2>/dev/null | head -20Repository: hiqdev/hipanel-rbac
Length of output: 45
🏁 Script executed:
# Check if phpunit is actually installed and what version
rg -i "require|require-dev" composer.json -A 50Repository: hiqdev/hipanel-rbac
Length of output: 1201
🏁 Script executed:
# Check if tests directory exists and list contents
ls -la tests/ 2>/dev/null || echo "tests directory not found"
find . -name "*.php" -path "*/tests/*" | head -10Repository: hiqdev/hipanel-rbac
Length of output: 709
🏁 Script executed:
# Look for any phpunit configuration files
find . \( -name "phpunit.xml" -o -name "phpunit.xml.dist" -o -name "phpunit.dist.xml" \)Repository: hiqdev/hipanel-rbac
Length of output: 684
🏁 Script executed:
# Check the main package requirement - what PHP version does it actually support?
cat -n composer.json | grep -A 20 '"require"'Repository: hiqdev/hipanel-rbac
Length of output: 799
🏁 Script executed:
# Examine the phpunit configuration
cat -n phpunit.xml.distRepository: hiqdev/hipanel-rbac
Length of output: 820
🏁 Script executed:
# Examine the actual test files for PHPUnit API usage
cat -n tests/unit/AuthManagerTest.phpRepository: hiqdev/hipanel-rbac
Length of output: 6334
🏁 Script executed:
# Check the bootstrap file
cat -n tests/_bootstrap.phpRepository: hiqdev/hipanel-rbac
Length of output: 921
🏁 Script executed:
# Look for any `@dataProvider` or attribute-based test annotations
rg -n '#\[|@dataProvider|@test|function test' tests/ --type=phpRepository: hiqdev/hipanel-rbac
Length of output: 1842
Update phpunit.xml.dist and add PHP version constraint to ensure PHPUnit 12 and php-parser 5 compatibility.
The test code uses modern syntax (protected function setUp(): void, test method naming) compatible with PHPUnit 12. However, the phpunit.xml.dist contains deprecated XML attributes (backupGlobals, backupStaticAttributes, <whitelist>) that are removed in PHPUnit 12 and must be removed or replaced with the new <coverage> structure. Additionally, add a PHP version constraint ("php": "^8.3") to the require section, since PHPUnit 12 mandates PHP ≥ 8.3; otherwise downstream consumers on PHP 8.0-8.2 will face installation failures. The php-parser v5 API usage (PhpParser\Node, PhpParser\Parser, PhpParser\ParserFactory, etc.) is compatible with your current imports.
🤖 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 `@composer.json` around lines 52 - 53, Add a PHP version constraint and update
the PHPUnit config: in composer.json add "php": "^8.3" to the "require" section
so phpunit/phpunit (^12.5) and nikic/php-parser (^5.7) can be installed, and
edit phpunit.xml.dist to remove deprecated attributes (backupGlobals,
backupStaticAttributes) and the old <whitelist> element, replacing them with the
new <coverage> structure (use <coverage> with <include> rules) compatible with
PHPUnit 12; target the phpunit/phpunit and phpunit.xml.dist changes together to
ensure tests run on PHP ≥8.3.
| 'description' => 'Grants ticket management access and read access to clients, domains, DNS, certificates, contacts, servers, and hosting, plus subclient visibility and blacklist management.', | ||
| 'children' => [ | ||
| 'access-subclients', | ||
| 'support', | ||
| 'role:ticket.manager', | ||
| 'role:ticket.user', | ||
| 'role:client.support', | ||
| 'role:domain.user', | ||
| 'role:dns.user', | ||
| 'role:certificate.user', | ||
| 'role:contact.user', | ||
| 'role:server.user', | ||
| 'role:hosting.user', | ||
| 'role:blacklist.manager', | ||
| ], | ||
| ], | ||
| 'role:admin' => [ | ||
| 'type' => 1, | ||
| 'description' => 'The role is generally assigned to reseller\'s clients who are in charge of the technical management of the resources', | ||
| 'description' => 'Extends support with hub read access, stock read access, server administration (including system info and wizard), and full hosting administration (IP and service CRUD).', | ||
| 'children' => [ | ||
| 'admin', | ||
| 'role:support', | ||
| 'role:hub.user', | ||
| 'role:stock.user', | ||
| 'role:server.admin', | ||
| 'role:hosting.admin', | ||
| ], | ||
| ], | ||
| 'role:staff-admin' => [ | ||
| 'type' => 1, | ||
| 'description' => 'The role is generally assigned to staff who are in charge for the technical management of the resources', | ||
| 'description' => 'Extends admin with stock admin permissions (move management, administrative part data), the ability to create/delete/update servers and assign hubs, create/delete hubs, and visibility of unsold objects.', | ||
| 'children' => [ |
There was a problem hiding this comment.
Several role descriptions still describe the pre-restructure children and contradict the new tree.
Since the stated goal of this PR is to “actualize metadata.php descriptions” based on actual code usage, these mismatches are worth correcting before merge. The descriptions appear to have been generated against the previous composition:
role:support(Line 891) – description still mentions “blacklist management”, butrole:blacklist.managerwas removed fromrole:support’s children (now:access-subclients, support, role:ticket.user, role:client.support, role:domain.user, role:dns.user, role:certificate.user, role:contact.user, role:server.user, role:hosting.user). It also describes “read access” to DNS/certificates/contacts/servers/hosting, but those sub-roles grant full CRUD, not read-only.role:admin(Line 907) – description claims “hub read access, stock read access … full hosting administration (IP and service CRUD)”, but per the new children (role:support, role:server.admin) none ofrole:hub.user,role:stock.*, orrole:hosting.adminare granted. This is the most divergent description in the file.role:staff-admin(Line 915) – description omits the newly-addedrole:hub.user,role:ticket.manager,role:blacklist.manager,role:hosting.admin, androle:consumption.managerfrom children.role:accounter(Line 966) – says “Extends manager”, but now extendsrole:staff-manager(per Line 968 children).role:manager(Line 975) – mentions “certificate management … consumption management, mailing, and billing targets” and “contact management”, none of which are granted directly here (onlycertificate.pay/pushand a few client permissions appear;role:certificate.manager,role:consumption.manager,mailing.*,role:target.manager,role:contact.userare not children).role:reseller(Line 1003) – description omits the newly-addedrole:certificate.manager,role:ticket.manager,role:consumption.manager, androle:target.manager.role:server.admin(Line 102) – mentions “disk moves”, butserver.move-diskswas moved out torole:server.staff-adminin this PR.
Since items.php is the generated artifact, the corrections likely need to be applied in src/files/source/metadata.php (then regenerated). Worth a second pass with the actual tree composition as the source of truth.
Also applies to: 966-967, 975-976, 1003-1004
🤖 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/items.php` around lines 891 - 916, Descriptions in items.php no
longer match the actual role children after the restructure; update the source
role metadata in src/files/source/metadata.php (then regenerate items.php) so
descriptions reflect the current composition: remove references to removed
children (e.g., delete “blacklist management” from role:support and stop calling
DNS/certificate/contact/server/hosting access “read-only” if their child roles
grant CRUD), remove claims in role:admin about hub/stock/hosting CRUD that
aren’t granted by its children (role:support, role:server.admin), add the newly
granted children to role:staff-admin’s description (role:hub.user,
role:ticket.manager, role:blacklist.manager, role:hosting.admin,
role:consumption.manager), correct role:accounter’s parent wording to “extends
role:staff-manager,” and update role:manager and role:reseller descriptions to
reflect only the permissions their actual children grant (remove mentions of
certificate manager/consumption/mailing/target/contact management where not
present, and add newly included children like role:certificate.manager,
role:ticket.manager, role:consumption.manager, role:target.manager where
applicable); after editing the textual descriptions for these specific role keys
(role:support, role:admin, role:staff-admin, role:accounter, role:manager,
role:reseller, role:server.admin) regenerate items.php so the generated artifact
matches the source.
| 'server.wizzard' => [ | ||
| 'type' => 2, | ||
| 'description' => 'Disable wizard mode on a server (wizard mode marks a server as having multiple active services; disabling removes this marker)', | ||
| ], | ||
| 'deny:server.wizzard' => [ | ||
| 'type' => 2, | ||
| 'description' => 'Prohibits disabling wizard mode on the server', | ||
| ], | ||
| 'server.move-disks' => [ | ||
| 'type' => 2, | ||
| 'description' => 'Exchange/swap disk hardware between servers', | ||
| ], | ||
| 'deny:server.move-disks' => [ | ||
| 'type' => 2, | ||
| 'description' => 'Deny move disks between servers', | ||
| ], |
There was a problem hiding this comment.
server.wizzard and server.move-disks are missing 'internal' => true.
Both permissions are newly extracted into role:server.staff-admin (Lines 131–143), alongside server.create/server.delete/server.update/server.assign-hub — all of which are marked 'internal' => true. Per the PR’s stated goal of correcting internal flags so they reflect staff-only accessibility, these two appear to be the same class of permission (staff-only, only granted via the internal role:server.staff-admin) and should likely carry the same flag.
This is also consistent with the expected-internal-items list in tests/unit/AuthManagerTest.php, which currently does not include server.wizzard or server.move-disks — if the omission is intentional, please add a short justification; otherwise the metadata and test should both be updated.
🔧 Proposed fix
'server.wizzard' => [
'type' => 2,
'description' => 'Disable wizard mode on a server (wizard mode marks a server as having multiple active services; disabling removes this marker)',
+ 'internal' => true,
],
'deny:server.wizzard' => [
'type' => 2,
'description' => 'Prohibits disabling wizard mode on the server',
],
'server.move-disks' => [
'type' => 2,
'description' => 'Exchange/swap disk hardware between servers',
+ 'internal' => true,
],
'deny:server.move-disks' => [
'type' => 2,
'description' => 'Deny move disks between servers',
],Don’t forget the corresponding additions to $expectedInternalItems in tests/unit/AuthManagerTest.php::testIsRoleInternal if you apply this.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'server.wizzard' => [ | |
| 'type' => 2, | |
| 'description' => 'Disable wizard mode on a server (wizard mode marks a server as having multiple active services; disabling removes this marker)', | |
| ], | |
| 'deny:server.wizzard' => [ | |
| 'type' => 2, | |
| 'description' => 'Prohibits disabling wizard mode on the server', | |
| ], | |
| 'server.move-disks' => [ | |
| 'type' => 2, | |
| 'description' => 'Exchange/swap disk hardware between servers', | |
| ], | |
| 'deny:server.move-disks' => [ | |
| 'type' => 2, | |
| 'description' => 'Deny move disks between servers', | |
| ], | |
| 'server.wizzard' => [ | |
| 'type' => 2, | |
| 'description' => 'Disable wizard mode on a server (wizard mode marks a server as having multiple active services; disabling removes this marker)', | |
| 'internal' => true, | |
| ], | |
| 'deny:server.wizzard' => [ | |
| 'type' => 2, | |
| 'description' => 'Prohibits disabling wizard mode on the server', | |
| ], | |
| 'server.move-disks' => [ | |
| 'type' => 2, | |
| 'description' => 'Exchange/swap disk hardware between servers', | |
| 'internal' => true, | |
| ], | |
| 'deny:server.move-disks' => [ | |
| 'type' => 2, | |
| 'description' => 'Deny move disks between servers', | |
| ], |
🤖 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/items.php` around lines 1666 - 1681, Add the missing 'internal' =>
true flag to the two permission entries 'server.wizzard' and 'server.move-disks'
so they match the other staff-only permissions grouped under
role:server.staff-admin; update the permissions array entries for
'server.wizzard' and 'deny:server.move-disks' (if applicable) to include
'internal' => true and then add 'server.wizzard' and 'server.move-disks' to the
$expectedInternalItems array in
tests/unit/AuthManagerTest.php::testIsRoleInternal to keep the unit test in
sync.
1 participant
Replace boilerplate "The role is generally assigned to..." descriptions with functional summaries derived from actual code usage. Fix incorrect internal flags for permissions accessible to resellers/clients. Fix typos.
Summary by CodeRabbit
Documentation
Chores
Tests