Feat/4311 category permission enforcement - #4553
Merged
Merged
Conversation
…sticky and listing
📝 WalkthroughWalkthroughThe PR adds category-scoped permission APIs for basic and medium modes. Administration controllers enforce these permissions for category and FAQ operations. FAQ administration views filter category trees. Tests and documentation cover restricted, unrestricted, and uncategorized content. ChangesCategory-scoped administration permissions
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AdminController
participant AbstractController
participant MediumPermission
participant CategoryTreeRestrictionFilter
participant FAQStore
AdminController->>AbstractController: validate category permission
AbstractController->>MediumPermission: check user right for categories
MediumPermission-->>AbstractController: return authorization result
AdminController->>CategoryTreeRestrictionFilter: filter allowed category tree
CategoryTreeRestrictionFilter-->>AdminController: return filtered tree
AdminController->>FAQStore: perform authorized FAQ operation
FAQStore-->>AdminController: return operation result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/CategoryController.php (1)
150-185: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject unauthorized categories before saving the submitted order.
updateOrderonly checks$categoryId, while$this->categoryOrder->setCategoryTree($categoryTree)persists order rows for every node in the submitted tree. Exclude or reject tree nodes outside the user’s granted category rights before setting the tree and before updating the parent.🤖 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 `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/CategoryController.php` around lines 150 - 185, Update updateOrder to validate every category node in categoryTree against the current user’s CATEGORY_EDIT rights before calling setCategoryTree or updateParentCategory. Reject unauthorized nodes or remove them from the submitted tree, while preserving authorized nodes and ensuring parent lookup and persistence use only the validated tree.phpmyfaq/src/phpMyFAQ/Controller/Administration/CategoryController.php (1)
69-118: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRestrict category overview/hierarchy tree data with category permissions.
index()passescategoryTree,categoryInfo, andcategoryTranslationsderived fromcategoryInfoto the template;getAllCategories()loads the full category catalog.hierarchy()passescategory->getCategoryTree()afterbuildCategoryTree()useswithPermission: false. Use a permission-scoped category list and filter the returned tree by the user’s allowed category IDs, as FAQ admin controllers do.🤖 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 `@phpmyfaq/src/phpMyFAQ/Controller/Administration/CategoryController.php` around lines 69 - 118, Restrict category data in both index() and hierarchy() to the current user’s permitted category IDs. Replace the unrestricted category loading around Category and buildCategoryTree() with the established permission-scoped category list used by FAQ admin controllers, then filter categoryTree and categoryInfo-derived categoryTranslations consistently before rendering.
🧹 Nitpick comments (3)
phpmyfaq/src/phpMyFAQ/Permission/MediumPermission.php (1)
588-627: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated right-resolution logic into a private helper.
The block that resolves
$rightto a right ID (numeric check,getRightId()for a name,getRightId()for aPermissionType) is duplicated inhasPermission,hasPermissionForCategory, and nowgetAllowedCategoriesForRight. Extract a private method, for exampleresolveRightId(mixed $right): int, and call it from all three places. This reduces the risk that a future fix to right resolution is applied in only one location.♻️ Proposed helper extraction
+ private function resolveRightId(mixed $right): int + { + if (!is_numeric($right) && is_string($right)) { + $right = $this->getRightId($right); + } + + if ($right instanceof PermissionType) { + $right = $this->getRightId($right->value); + } + + return (int) $right; + } + #[\Override] public function getAllowedCategoriesForRight(int $userId, mixed $right): ?array { $currentUser = new CurrentUser($this->configuration); $currentUser->getUserById($userId); if ($currentUser->isSuperAdmin()) { return null; } - if (!is_numeric($right) && is_string($right)) { - $right = $this->getRightId($right); - } - - if ($right instanceof PermissionType) { - $right = $this->getRightId($right->value); - } - - $rightId = (int) $right; + $rightId = $this->resolveRightId($right);🤖 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 `@phpmyfaq/src/phpMyFAQ/Permission/MediumPermission.php` around lines 588 - 627, Extract the duplicated right-resolution logic into a private resolveRightId(mixed $right): int helper, preserving the existing numeric, string, and PermissionType handling. Replace the corresponding resolution blocks in hasPermission, hasPermissionForCategory, and getAllowedCategoriesForRight with calls to this helper, then use its returned integer right ID.phpmyfaq/src/phpMyFAQ/Controller/Administration/FaqController.php (1)
100-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated "allowed categories + filter tree" pattern into a helper.
The same three-line pattern — call
getAllowedCategoriesForRight()for aPermissionType, then pass the result and$category->getCategoryTree()toCategoryTreeRestrictionFilter::filter()— repeats at lines 148-152/167-170, 228-232/247-250, 362-366/394-397, 452-456/474-477, 532-536/554-557, and 623-627/645-648. Extract a private helper, for examplegetFilteredCategoryTree(Category $category, PermissionType $permissionType): array, and call it from all seven sites. This removes duplication and keeps future fixes (such as the type-mismatch fix on line 305 above) in one place.As per path instructions for
**/src/phpMyFAQ/**/*.php: "Prefer guard clauses and early returns over deeply nested conditionals; keep functions small and single-purpose."♻️ Proposed helper extraction
+ private function getFilteredCategoryTree(Category $category, PermissionType $permissionType): array + { + $allowed = $this->currentUser->perm->getAllowedCategoriesForRight( + $this->currentUser->getUserId(), + $permissionType->value, + ); + + return CategoryTreeRestrictionFilter::filter($category->getCategoryTree(), $allowed); + } + #[Route(path: '/faqs', name: 'admin.faqs', methods: ['GET'])] public function index(Request $request): Response { ... - $allowedCategories = $this->currentUser->perm->getAllowedCategoriesForRight( - $this->currentUser->getUserId(), - PermissionType::FAQ_EDIT->value, - ); - return $this->render('`@admin/content/faq.overview.twig`', [ ... - 'categories' => CategoryTreeRestrictionFilter::filter($category->getCategoryTree(), $allowedCategories), + 'categories' => $this->getFilteredCategoryTree($category, PermissionType::FAQ_EDIT),🤖 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 `@phpmyfaq/src/phpMyFAQ/Controller/Administration/FaqController.php` around lines 100 - 110, Extract the repeated permission lookup and category-tree filtering into a private FaqController helper named getFilteredCategoryTree(Category $category, PermissionType $permissionType): array. Have it call getAllowedCategoriesForRight() with the current user ID and permission value, then pass those categories and $category->getCategoryTree() to CategoryTreeRestrictionFilter::filter(); replace all seven duplicated patterns with this helper while preserving each site’s permission type.Source: Path instructions
tests/phpMyFAQ/Controller/Administration/CategoryControllerTest.php (1)
646-652: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThis PR adds category-scoped permission enforcement to
Administration/CategoryController.php(addChild,create,edit,translate,update) and presumably toAdministration/FaqController.php. The corresponding admin test files only extend their permission mocks to satisfy thePermissionInterfacecontract (hasPermissionForCategory()alwaystrue,getAllowedCategoriesForRight()alwaysnull), unlike the sibling API test files, which added explicit sentinel-category (666) forbidden-path tests in this same PR.
tests/phpMyFAQ/Controller/Administration/CategoryControllerTest.php#L646-L652: Add a forbidden-path test for at least one ofaddChild,create,edit,translate, orupdate, following the same sentinel-category pattern used intests/phpMyFAQ/Controller/Administration/Api/CategoryControllerTest.php'stestDeleteReturnsForbiddenForRestrictedCategory.tests/phpMyFAQ/Controller/Administration/FaqControllerTest.php#L352-L358: Add an equivalent forbidden-path test for the correspondingAdministration/FaqController.phpmethods that now calluserHasPermissionForCategories(), following the pattern intests/phpMyFAQ/Controller/Administration/Api/FaqControllerTest.php.🤖 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 `@tests/phpMyFAQ/Controller/Administration/CategoryControllerTest.php` around lines 646 - 652, Add sentinel-category forbidden-path coverage for the administration category actions in tests/phpMyFAQ/Controller/Administration/CategoryControllerTest.php:646-652, using the existing permission mock and pattern from the API category test; the test should verify a restricted category returns forbidden for at least one of addChild, create, edit, translate, or update. Add equivalent coverage in tests/phpMyFAQ/Controller/Administration/FaqControllerTest.php:352-358 for an affected FAQ action, following the API FAQ test pattern and asserting forbidden access for category 666.
🤖 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 `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/FaqController.php`:
- Around line 360-364: Fix the inferred category ID type used by
userHasPermissionForCategories at all four call sites in FaqController,
including the paths around the current category relation and the other
getCategories calls. Prefer adding a precise return annotation to
Relation::getCategories so array_keys infers integer IDs consistently; otherwise
explicitly cast each array_keys result to int[] before passing or spreading it,
while preserving the existing permission checks.
In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/FaqController.php`:
- Around line 303-308: Update the call to userHasPermissionForCategories() in
the FAQ edit flow to convert every key from $categories to an integer before
passing the resulting list, satisfying the method’s array<int> value contract
while leaving the surrounding permission and logging behavior unchanged.
In `@phpmyfaq/src/phpMyFAQ/Permission/MediumPermission.php`:
- Around line 576-627: Update getAllowedCategoriesForRight to aggregate category
permissions using the same checkUserGroupRightForCategory semantics as
hasPermissionForCategory, including categories explicitly present in each
group-right restriction instead of omitting restricted groups. Preserve
unrestricted handling for groups without category restrictions and ensure the
returned set matches the categories that userHasPermissionForCategories can
allow.
---
Outside diff comments:
In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/CategoryController.php`:
- Around line 150-185: Update updateOrder to validate every category node in
categoryTree against the current user’s CATEGORY_EDIT rights before calling
setCategoryTree or updateParentCategory. Reject unauthorized nodes or remove
them from the submitted tree, while preserving authorized nodes and ensuring
parent lookup and persistence use only the validated tree.
In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/CategoryController.php`:
- Around line 69-118: Restrict category data in both index() and hierarchy() to
the current user’s permitted category IDs. Replace the unrestricted category
loading around Category and buildCategoryTree() with the established
permission-scoped category list used by FAQ admin controllers, then filter
categoryTree and categoryInfo-derived categoryTranslations consistently before
rendering.
---
Nitpick comments:
In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/FaqController.php`:
- Around line 100-110: Extract the repeated permission lookup and category-tree
filtering into a private FaqController helper named
getFilteredCategoryTree(Category $category, PermissionType $permissionType):
array. Have it call getAllowedCategoriesForRight() with the current user ID and
permission value, then pass those categories and $category->getCategoryTree() to
CategoryTreeRestrictionFilter::filter(); replace all seven duplicated patterns
with this helper while preserving each site’s permission type.
In `@phpmyfaq/src/phpMyFAQ/Permission/MediumPermission.php`:
- Around line 588-627: Extract the duplicated right-resolution logic into a
private resolveRightId(mixed $right): int helper, preserving the existing
numeric, string, and PermissionType handling. Replace the corresponding
resolution blocks in hasPermission, hasPermissionForCategory, and
getAllowedCategoriesForRight with calls to this helper, then use its returned
integer right ID.
In `@tests/phpMyFAQ/Controller/Administration/CategoryControllerTest.php`:
- Around line 646-652: Add sentinel-category forbidden-path coverage for the
administration category actions in
tests/phpMyFAQ/Controller/Administration/CategoryControllerTest.php:646-652,
using the existing permission mock and pattern from the API category test; the
test should verify a restricted category returns forbidden for at least one of
addChild, create, edit, translate, or update. Add equivalent coverage in
tests/phpMyFAQ/Controller/Administration/FaqControllerTest.php:352-358 for an
affected FAQ action, following the API FAQ test pattern and asserting forbidden
access for category 666.
🪄 Autofix
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 Plus
Run ID: 584d4ac0-40a1-4536-93a5-d431be2c0b14
📒 Files selected for processing (19)
docs/administration.mdphpmyfaq/src/phpMyFAQ/Category/CategoryTreeRestrictionFilter.phpphpmyfaq/src/phpMyFAQ/Controller/AbstractController.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/Api/CategoryController.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/Api/FaqController.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/CategoryController.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/FaqController.phpphpmyfaq/src/phpMyFAQ/Permission/BasicPermission.phpphpmyfaq/src/phpMyFAQ/Permission/MediumPermission.phpphpmyfaq/src/phpMyFAQ/Permission/PermissionInterface.phptests/phpMyFAQ/Administration/AdminMenuBuilderTest.phptests/phpMyFAQ/Attachment/AttachmentServiceTest.phptests/phpMyFAQ/Category/CategoryTreeRestrictionFilterTest.phptests/phpMyFAQ/Controller/Administration/Api/CategoryControllerTest.phptests/phpMyFAQ/Controller/Administration/Api/FaqControllerTest.phptests/phpMyFAQ/Controller/Administration/CategoryControllerTest.phptests/phpMyFAQ/Controller/Administration/FaqControllerTest.phptests/phpMyFAQ/Permission/BasicPermissionTest.phptests/phpMyFAQ/Permission/MediumPermissionTest.php
thorsten
force-pushed
the
feat/4311-category-permission-enforcement
branch
from
August 10, 2026 10:48
f04732a to
abc49af
Compare
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 `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/CategoryController.php`:
- Around line 171-174: Replace the hard-coded denial text in the category
reorder permission check with a new translation key, and retrieve the localized
message through Translation::get() before constructing ForbiddenException. Add
the key to the appropriate translation resources while preserving the permission
name interpolation and existing exception behavior.
🪄 Autofix
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 Plus
Run ID: fb379bb5-12e5-4408-90c9-bb4cbe7257de
📒 Files selected for processing (9)
phpmyfaq/src/phpMyFAQ/Category/CategoryTreeRestrictionFilter.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/Api/CategoryController.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/CategoryController.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/FaqController.phpphpmyfaq/src/phpMyFAQ/Permission/MediumPermission.phptests/phpMyFAQ/Category/CategoryTreeRestrictionFilterTest.phptests/phpMyFAQ/Controller/Administration/Api/CategoryControllerTest.phptests/phpMyFAQ/Controller/Administration/CategoryControllerTest.phptests/phpMyFAQ/Controller/Administration/FaqControllerTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- phpmyfaq/src/phpMyFAQ/Controller/Administration/FaqController.php
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 `@tests/phpMyFAQ/Permission/MediumPermissionTest.php`:
- Around line 713-714: Replace the raw UPDATE and DELETE statements in the test
setup with the existing database abstraction or fixture helper that configures
user 1 as a non-superadmin without right 1. Preserve the resulting permission
state while removing inline SQL from the test.
🪄 Autofix
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 Plus
Run ID: f2911d0e-caad-4dc5-9fb7-04197274a82d
📒 Files selected for processing (1)
tests/phpMyFAQ/Permission/MediumPermissionTest.php
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
1 participant
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
Documentation