feat: add granular group-based permissions with category restrictions#4073
Open
feat: add granular group-based permissions with category restrictions#4073
Conversation
📝 WalkthroughWalkthroughAdd per-category restrictions for group permissions: DB table + migration, repository and MediumPermission extensions, API endpoints and CSRF support, frontend TypeScript APIs and UI, translations, and extensive unit tests across backend and frontend. Changes
Sequence DiagramsequenceDiagram
participant Admin as Admin User
participant UI as Browser/UI
participant API as GroupController API
participant Repo as GroupCategoryPermission<br/>Repository
participant DB as Database
Admin->>UI: Select group/right
UI->>API: GET /api/group/categories
API->>Repo: listCategories()
Repo->>DB: SELECT id,name,parent_id FROM faqcategories
DB-->>Repo: categories
Repo-->>API: categories JSON
API-->>UI: categories JSON
UI->>API: GET /api/group/category-restrictions/{groupId}
API->>Repo: getAllCategoryRestrictions(groupId)
Repo->>DB: SELECT right_id,category_id FROM faqgroup_right_category
DB-->>Repo: restrictions
Repo-->>API: restrictions JSON
API-->>UI: restrictions JSON
Admin->>UI: Modify selections and Save
UI->>API: POST /api/group/category-restrictions {groupId,rightId,categoryIds,csrf}
API->>Repo: setCategoryRestrictions(groupId,rightId,categoryIds)
Repo->>DB: DELETE/INSERT ... faqgroup_right_category
DB-->>Repo: success
Repo-->>API: {"success":true}
API-->>UI: {"success":true}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
tests/phpMyFAQ/Setup/Installation/SchemaInstallerTest.php (1)
53-53: Derive the expected table count from the schema instead of hard-coding53.
tests/phpMyFAQ/Setup/Installation/DatabaseSchemaTest.phpalready locks the total table count. Keeping another literal here means every schema change needs two manual updates, while this test really just needs to prove the installer emits oneCREATE TABLEper schema table.♻️ Suggested change
- $this->assertEquals(53, $createTableCount, 'Should generate CREATE TABLE for all 53 tables'); + $this->assertEquals( + count($installer->getSchema()->getAllTables()), + $createTableCount, + 'Should generate CREATE TABLE for every schema table', + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpMyFAQ/Setup/Installation/SchemaInstallerTest.php` at line 53, Replace the hard-coded literal 53 with a derived count from the canonical schema source: in SchemaInstallerTest use the same schema provider used by DatabaseSchemaTest (e.g. instantiate the DatabaseSchema or call its getTables()/getTableList() method) to compute $expectedCount = count($schemaTables) and assertEquals($expectedCount, $createTableCount, ...) so the test always compares one CREATE TABLE per schema table; reference the $createTableCount variable, SchemaInstallerTest class, and DatabaseSchemaTest (or DatabaseSchema::getTables()) to locate the schema provider.phpmyfaq/src/phpMyFAQ/Permission/MediumPermission.php (1)
494-501: Performance:CurrentUserinstantiated on every permission check.
hasPermissionForCategorycreates a newCurrentUserinstance and performs a database lookup for every call. This could be expensive when checking permissions for multiple categories in a loop.Consider accepting an optional
CurrentUserparameter or caching the superadmin check at a higher level.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Permission/MediumPermission.php` around lines 494 - 501, The method hasPermissionForCategory currently instantiates CurrentUser and calls getUserById on every invocation—move that expensive work out by adding an optional CurrentUser parameter to hasPermissionForCategory (or accept a user DTO) and use it when provided, or provide a short-circuit cached superadmin flag checked before creating a new CurrentUser; update the method signature (hasPermissionForCategory(int $userId, mixed $right, int $categoryId, ?CurrentUser $currentUser = null)) and the callers to pass an existing CurrentUser or check a cached isSuperAdmin value so you avoid repeated new CurrentUser(...) and getUserById calls.phpmyfaq/admin/assets/src/group/groups.ts (2)
401-412: Sequential saves without user feedback or error handling.The save loop iterates over all selects and calls
saveGroupCategoryRestrictionsfor each, but:
- No success/error feedback is shown to the user
- If one save fails, subsequent saves still proceed, leaving partial state
- The async calls are awaited one-by-one, which is correct, but the user has no indication of progress
Consider adding a toast/notification on success or error, or accumulating errors to report at the end.
Example improvement
export const handleCategoryRestrictionsSave = async (): Promise<void> => { // ... existing validation ... const selects = container.querySelectorAll<HTMLSelectElement>('select[data-right-id]'); + const errors: string[] = []; for (const select of selects) { const rightId = select.dataset.rightId; if (!rightId) { continue; } const selectedCategoryIds = [...select.options] .filter((option: HTMLOptionElement): boolean => option.selected) .map((option: HTMLOptionElement): number => parseInt(option.value)); - await saveGroupCategoryRestrictions(groupId, rightId, selectedCategoryIds); + const response = await saveGroupCategoryRestrictions(groupId, rightId, selectedCategoryIds); + if (!response.ok) { + errors.push(`Failed to save restrictions for right ${rightId}`); + } } + + // Show feedback (assuming a pushNotification or similar utility exists) + if (errors.length > 0) { + // show error notification + } else { + // show success notification + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/admin/assets/src/group/groups.ts` around lines 401 - 412, The loop over selects that calls saveGroupCategoryRestrictions(groupId, rightId, selectedCategoryIds) should provide user feedback and handle errors: collect any failures into an errors array (include rightId and error message), show a progress indicator or disable the UI while saving, await each save as now but on error push to errors and optionally stop further saves if you prefer atomicity; after the loop, show a toast/notification summarizing success or reporting accumulated errors (or a single error if you choose to abort on first failure) and re-enable the UI — reference select.dataset.rightId, selectedCategoryIds, saveGroupCategoryRestrictions, and groupId to locate and implement these changes.
317-318: Module-level mutable state may cause stale data issues.
cachedCategoriesandcurrentRestrictionsare module-level variables that persist across function calls. If categories are added/removed while the admin panel is open,cachedCategorieswill become stale. Consider adding a mechanism to invalidate the cache or refresh on demand.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/admin/assets/src/group/groups.ts` around lines 317 - 318, The module-level mutable variables cachedCategories and currentRestrictions can become stale; add a clear cache/refresh mechanism and use it where category changes occur: implement and export functions like invalidateCategoryCache() and refreshCachedCategories() (or integrate a TTL) and call them from mutation handlers (e.g., after add/remove/update category actions) or when the admin UI is focused, ensuring code paths that read cachedCategories use the refresh function to reload fresh data when invalidated.tests/phpMyFAQ/Permission/MediumPermissionTest.php (1)
523-548: Good test cleanup, but restoration SQL may fail silently.The test modifies
faquserandfaquser_righttables and restores them at the end. If the test fails mid-execution, the restoration code won't run, potentially leaving the test database in an inconsistent state for subsequent tests.Consider using a transaction rollback pattern or ensuring cleanup happens in a
finallyblock ortearDown.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpMyFAQ/Permission/MediumPermissionTest.php` around lines 523 - 548, The test mutates DB state directly (calls to $this->configuration->getDb()->query, and methods on $this->mediumPermission like addGroup, addToGroup, grantGroupRight, setCategoryRestrictions) but restores state only at the end of the test, so failures can leave the DB dirty; wrap the DB mutations in a transaction (use beginTransaction/rollBack/commit via $this->configuration->getDb()) or move the restoration into tearDown/ a try/finally block so deleteGroup(1) and the faquser/faquser_right restores always run even if assertions fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@phpmyfaq/admin/assets/src/api/group.ts`:
- Around line 91-106: Update the saveGroupCategoryRestrictions function to
accept a csrfToken parameter and include it in the POST body: modify the
signature of saveGroupCategoryRestrictions(groupId: string, rightId: string,
categoryIds: number[], csrfToken: string) and add csrfToken to the object passed
to JSON.stringify (alongside groupId, rightId, categoryIds) so the CSRF token is
sent with the request.
In `@phpmyfaq/assets/templates/admin/user/group.twig`:
- Around line 208-214: The injected helper/empty-state strings for the category
restrictions are hard-coded in the JS and bypass translation, causing
mixed-language UIs; modify the template around the `#categoryRestrictionsBody`
card to output translated strings (e.g., via data attributes like
data-empty-message and data-help-message or hidden elements with {{ 'key' |
translate }}) and update phpmyfaq/admin/assets/src/group/groups.ts to read those
attributes/elements (instead of embedding English text) when replacing
`#categoryRestrictionsBody` so all displayed text goes through the existing
translation keys.
In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/GroupController.php`:
- Around line 147-149: The branch in GroupController that checks if
(!$currentUser->perm instanceof MediumPermission) returns an empty JSON array,
but the API contract expects an object; change the response to return an empty
object instead (e.g. use (object)[] or new \stdClass()) in the $this->json(...)
call so the method (in GroupController) returns {} rather than [] to match
phpmyfaq/admin/assets/src/interfaces/Group.ts.
- Around line 168-177: Validate and sanitize the incoming payload before calling
setCategoryRestrictions: ensure $data from json_decode yields an array and that
'categoryIds' is an array; coerce each ID to an int and reject any non-positive
integers; load the canonical category IDs via loadCategories() and filter
$categoryIds so only IDs present in that set remain (or return a 400 JSON error
if any submitted IDs are invalid); keep existing groupId/rightId checks and only
call $currentUser->perm->setCategoryRestrictions($groupId, $rightId,
$categoryIds) with the validated, filtered IDs.
In `@phpmyfaq/src/phpMyFAQ/Permission/GroupCategoryPermissionRepository.php`:
- Around line 116-148: The delete-then-insert sequence in
setCategoryRestrictions can leave the DB in a partial state if an insert fails;
wrap the deletion and all subsequent INSERTs into a single DB transaction using
the database handle from $this->configuration->getDb(): begin a transaction
before calling deleteCategoryRestrictions, perform all INSERTs into
faqgroup_right_category, commit on success, and rollback on any failure
(returning false). Ensure you call rollback if any query fails and only return
true after a successful commit; reference setCategoryRestrictions,
deleteCategoryRestrictions, $this->configuration->getDb(), and the
faqgroup_right_category table when locating where to add
beginTransaction/commit/rollback.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha2.php`:
- Around line 25-30: Register the new migration by adding an entry for
'4.2.0-alpha.2' => Versions\Migration420Alpha2::class into the $migrationClasses
array inside the MigrationRegistry::registerDefaultMigrations() method so
Migration420Alpha2 (the readonly class Migration420Alpha2 extends
AbstractMigration) will be discovered and executed during upgrades.
---
Nitpick comments:
In `@phpmyfaq/admin/assets/src/group/groups.ts`:
- Around line 401-412: The loop over selects that calls
saveGroupCategoryRestrictions(groupId, rightId, selectedCategoryIds) should
provide user feedback and handle errors: collect any failures into an errors
array (include rightId and error message), show a progress indicator or disable
the UI while saving, await each save as now but on error push to errors and
optionally stop further saves if you prefer atomicity; after the loop, show a
toast/notification summarizing success or reporting accumulated errors (or a
single error if you choose to abort on first failure) and re-enable the UI —
reference select.dataset.rightId, selectedCategoryIds,
saveGroupCategoryRestrictions, and groupId to locate and implement these
changes.
- Around line 317-318: The module-level mutable variables cachedCategories and
currentRestrictions can become stale; add a clear cache/refresh mechanism and
use it where category changes occur: implement and export functions like
invalidateCategoryCache() and refreshCachedCategories() (or integrate a TTL) and
call them from mutation handlers (e.g., after add/remove/update category
actions) or when the admin UI is focused, ensuring code paths that read
cachedCategories use the refresh function to reload fresh data when invalidated.
In `@phpmyfaq/src/phpMyFAQ/Permission/MediumPermission.php`:
- Around line 494-501: The method hasPermissionForCategory currently
instantiates CurrentUser and calls getUserById on every invocation—move that
expensive work out by adding an optional CurrentUser parameter to
hasPermissionForCategory (or accept a user DTO) and use it when provided, or
provide a short-circuit cached superadmin flag checked before creating a new
CurrentUser; update the method signature (hasPermissionForCategory(int $userId,
mixed $right, int $categoryId, ?CurrentUser $currentUser = null)) and the
callers to pass an existing CurrentUser or check a cached isSuperAdmin value so
you avoid repeated new CurrentUser(...) and getUserById calls.
In `@tests/phpMyFAQ/Permission/MediumPermissionTest.php`:
- Around line 523-548: The test mutates DB state directly (calls to
$this->configuration->getDb()->query, and methods on $this->mediumPermission
like addGroup, addToGroup, grantGroupRight, setCategoryRestrictions) but
restores state only at the end of the test, so failures can leave the DB dirty;
wrap the DB mutations in a transaction (use beginTransaction/rollBack/commit via
$this->configuration->getDb()) or move the restoration into tearDown/ a
try/finally block so deleteGroup(1) and the faquser/faquser_right restores
always run even if assertions fail.
In `@tests/phpMyFAQ/Setup/Installation/SchemaInstallerTest.php`:
- Line 53: Replace the hard-coded literal 53 with a derived count from the
canonical schema source: in SchemaInstallerTest use the same schema provider
used by DatabaseSchemaTest (e.g. instantiate the DatabaseSchema or call its
getTables()/getTableList() method) to compute $expectedCount =
count($schemaTables) and assertEquals($expectedCount, $createTableCount, ...) so
the test always compares one CREATE TABLE per schema table; reference the
$createTableCount variable, SchemaInstallerTest class, and DatabaseSchemaTest
(or DatabaseSchema::getTables()) to locate the schema provider.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d7bdd86-267f-49e3-9d51-e86c3141c07d
📒 Files selected for processing (15)
phpmyfaq/admin/assets/src/api/group.test.tsphpmyfaq/admin/assets/src/api/group.tsphpmyfaq/admin/assets/src/group/groups.tsphpmyfaq/admin/assets/src/interfaces/Group.tsphpmyfaq/assets/templates/admin/user/group.twigphpmyfaq/src/phpMyFAQ/Controller/Administration/Api/GroupController.phpphpmyfaq/src/phpMyFAQ/Permission/GroupCategoryPermissionRepository.phpphpmyfaq/src/phpMyFAQ/Permission/MediumPermission.phpphpmyfaq/src/phpMyFAQ/Setup/Installation/DatabaseSchema.phpphpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha2.phpphpmyfaq/translations/language_en.phptests/phpMyFAQ/Permission/GroupCategoryPermissionRepositoryTest.phptests/phpMyFAQ/Permission/MediumPermissionTest.phptests/phpMyFAQ/Setup/Installation/DatabaseSchemaTest.phptests/phpMyFAQ/Setup/Installation/SchemaInstallerTest.php
Groups can now have their permissions scoped to specific categories. When no category restrictions are set, rights apply globally (backward compatible). This enables controlled external contributions where users can be restricted to specific categories via group membership. New table faqgroup_right_category links group rights to categories. Added API endpoints for managing category restrictions, admin UI panel for configuring restrictions per permission, and hasPermissionForCategory() for category-scoped permission checks. This feature implements and closes #3780.
The migration was not discoverable during upgrades because it was missing from the $migrationClasses array in registerDefaultMigrations().
The delete-then-insert sequence could leave partial state if an INSERT failed mid-loop. Now uses BEGIN/COMMIT/ROLLBACK to ensure atomicity.
Guard against invalid JSON, non-array categoryIds, and non-positive integer values before passing data to setCategoryRestrictions().
Empty PHP arrays serialize as JSON [] but the CategoryRestrictions
TypeScript interface expects an object ({[rightId: string]: number[]}).
Use stdClass to ensure {} is returned when there are no restrictions.
Hardcoded English strings in the JS bypassed the translation system. Added translation keys and data attributes on the template container so the TypeScript reads localized text instead of embedding English.
The POST endpoint lacked CSRF protection. Added token generation in the admin GroupController template vars, a data attribute on the template, passing the token through the TypeScript API call, and server-side verification via Token::verifyToken in the API controller.
Avoids instantiating a new CurrentUser and hitting the DB on every call when the caller already has one. The parameter is optional to maintain backward compatibility.
2c6a2c5 to
57c4bc2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
phpmyfaq/admin/assets/src/api/group.test.ts (1)
209-232: Consider adding an error handling test for consistency.The
saveGroupCategoryRestrictionstest suite only covers the success case. Other test suites in this file include error handling tests. Consider adding one for consistency:♻️ Suggested test addition
it('should throw an error if the network response is not ok', async () => { vi.spyOn(fetchWrapperModule, 'fetchWrapper').mockRejectedValue(new Error('Network response was not ok.')); await expect(saveGroupCategoryRestrictions('5', '1', [10, 20], 'csrf')).rejects.toThrow('Network response was not ok.'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/admin/assets/src/api/group.test.ts` around lines 209 - 232, Add a failing-network test for saveGroupCategoryRestrictions that mirrors other suites: spy on fetchWrapperModule.fetchWrapper to reject with new Error('Network response was not ok.'), then assert that calling saveGroupCategoryRestrictions('5','1',[10,20],'csrf') rejects with that error (use await expect(...).rejects.toThrow('Network response was not ok.')). Place this new it block alongside the existing success test in the saveGroupCategoryRestrictions describe so error handling behavior is verified.phpmyfaq/admin/assets/src/group/groups.ts (3)
335-384: innerHTML usage is safe here but consider DOM API for consistency.The static analysis flagged
innerHTMLusage, but the interpolatedemptyMsgcomes from adata-*attribute set by the server-rendered Twig template (seegroup.twig:213), not from user input. This is safe in practice.However, for consistency with the rest of the function which uses DOM API (
document.createElement), you could replace line 342 with DOM methods:♻️ Optional: Replace innerHTML with DOM API
if (checkedRights.length === 0) { const emptyMsg = container.dataset.msgEmpty || 'No permissions assigned to this group.'; - container.innerHTML = `<p class="text-muted">${emptyMsg}</p>`; + const p = document.createElement('p'); + p.className = 'text-muted'; + p.textContent = emptyMsg; + container.appendChild(p); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/admin/assets/src/group/groups.ts` around lines 335 - 384, The current renderCategoryRestrictions function uses container.innerHTML to inject a small static message (emptyMsg) which is safe but inconsistent with the rest of the function that uses DOM APIs; replace the innerHTML assignment in renderCategoryRestrictions (when checkedRights.length === 0) by creating a <p> element via document.createElement, set its className to "text-muted", assign textContent from container.dataset.msgEmpty (falling back to the literal), and append it to container to maintain DOM API consistency with the rest of the function.
386-416: Missing error handling and user feedback for save operation.The
handleCategoryRestrictionsSavefunction silently fails if anysaveGroupCategoryRestrictionscall fails. Users receive no indication of success or failure.Consider adding try/catch with user feedback:
♻️ Proposed improvement
export const handleCategoryRestrictionsSave = async (): Promise<void> => { const groupListSelect = document.getElementById('group_list_select') as HTMLSelectElement; if (!groupListSelect) { return; } const groupId = groupListSelect.value; if (!groupId) { return; } const container = document.getElementById('categoryRestrictionsBody'); if (!container) { return; } const csrfToken = container.dataset.csrfToken || ''; const selects = container.querySelectorAll<HTMLSelectElement>('select[data-right-id]'); + try { for (const select of selects) { const rightId = select.dataset.rightId; if (!rightId) { continue; } const selectedCategoryIds = [...select.options] .filter((option: HTMLOptionElement): boolean => option.selected) .map((option: HTMLOptionElement): number => parseInt(option.value, 10)); await saveGroupCategoryRestrictions(groupId, rightId, selectedCategoryIds, csrfToken); } + // Consider showing success toast/message + } catch (error) { + console.error('Failed to save category restrictions:', error); + // Consider showing error toast/message to user + } };Also,
parseInton line 413 should specify radix 10 explicitly:parseInt(option.value, 10).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/admin/assets/src/group/groups.ts` around lines 386 - 416, The save loop in handleCategoryRestrictionsSave silently ignores failures and uses parseInt without a radix; wrap each call to saveGroupCategoryRestrictions (inside the for...of over selects) in try/catch (or aggregate with Promise.allSettled if you prefer) to capture errors, surface them to the user (e.g., show a success/failure message in the UI or enable an error banner via the existing DOM with id/categoryRestrictionsBody), and stop or continue based on failure policy; also change parseInt(option.value) to parseInt(option.value, 10) when building selectedCategoryIds to avoid radix pitfalls and include csrfToken and groupId in any error/log context for easier debugging.
316-333: Module-level mutable state may cause stale data issues.
cachedCategoriesandcurrentRestrictionsare module-level variables that persist across group selections. WhilecurrentRestrictionsis refreshed on each group select,cachedCategoriesis only fetched once and never invalidated. If categories are added/removed in another tab or by another admin, this cache becomes stale until page reload.This is acceptable for the current use case, but consider adding a manual refresh mechanism or invalidation strategy if category management becomes more dynamic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/admin/assets/src/group/groups.ts` around lines 316 - 333, The module-level cachedCategories and currentRestrictions can become stale; update loadCategoryRestrictions to accept an optional forceRefresh boolean (or always refetch) and call fetchCategoriesForRestrictions when forceRefresh is true (or remove the one-time cache) so cachedCategories is refreshed when needed; additionally, ensure any category-modifying functions (e.g., the code paths that add/remove categories) clear or reset cachedCategories so subsequent calls to loadCategoryRestrictions will re-fetch, and keep currentRestrictions populated via fetchGroupCategoryRestrictions before calling renderCategoryRestrictions.phpmyfaq/src/phpMyFAQ/Permission/GroupCategoryPermissionRepository.php (1)
116-163: Transaction implementation is database-driver compatible.Raw SQL transaction commands (
BEGIN/COMMIT/ROLLBACK) are supported consistently across all configured database drivers (Mysqli, PDO MySQL, PostgreSQL, SQLite, SQL Server). The genericquery()method in each driver passes these commands directly to the underlying database library, which natively supports them. No compatibility issues exist.This is the first use of explicit transactions in the codebase. Consider adding a transaction abstraction layer in future refactoring for consistency, but this is not required for the current implementation to function correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Permission/GroupCategoryPermissionRepository.php` around lines 116 - 163, The current setCategoryRestrictions method uses raw SQL transaction commands via $db->query('BEGIN'/'COMMIT'/'ROLLBACK'); replace these with the database driver's transaction API where available (e.g. call $db->beginTransaction(), $db->commit(), $db->rollBack() or the driver's equivalent) to rely on the driver's abstraction and error handling; update the transaction calls in setCategoryRestrictions and ensure rollback is invoked via the driver's rollBack method when deletes or inserts fail, keeping the existing validation and casting logic for $groupId, $rightId and $categoryIds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@phpmyfaq/admin/assets/src/api/group.ts`:
- Around line 79-89: The frontend fetchGroupCategoryRestrictions constructs a
URL with a groupId path segment but the backend route for
listCategoryRestrictions is missing the {groupId} parameter; update the backend
route definition from path: 'group/category-restrictions' to path:
'group/category-restrictions/{groupId}' so $request->attributes->get('groupId')
inside listCategoryRestrictions will be populated and match the
fetchGroupCategoryRestrictions call.
---
Nitpick comments:
In `@phpmyfaq/admin/assets/src/api/group.test.ts`:
- Around line 209-232: Add a failing-network test for
saveGroupCategoryRestrictions that mirrors other suites: spy on
fetchWrapperModule.fetchWrapper to reject with new Error('Network response was
not ok.'), then assert that calling
saveGroupCategoryRestrictions('5','1',[10,20],'csrf') rejects with that error
(use await expect(...).rejects.toThrow('Network response was not ok.')). Place
this new it block alongside the existing success test in the
saveGroupCategoryRestrictions describe so error handling behavior is verified.
In `@phpmyfaq/admin/assets/src/group/groups.ts`:
- Around line 335-384: The current renderCategoryRestrictions function uses
container.innerHTML to inject a small static message (emptyMsg) which is safe
but inconsistent with the rest of the function that uses DOM APIs; replace the
innerHTML assignment in renderCategoryRestrictions (when checkedRights.length
=== 0) by creating a <p> element via document.createElement, set its className
to "text-muted", assign textContent from container.dataset.msgEmpty (falling
back to the literal), and append it to container to maintain DOM API consistency
with the rest of the function.
- Around line 386-416: The save loop in handleCategoryRestrictionsSave silently
ignores failures and uses parseInt without a radix; wrap each call to
saveGroupCategoryRestrictions (inside the for...of over selects) in try/catch
(or aggregate with Promise.allSettled if you prefer) to capture errors, surface
them to the user (e.g., show a success/failure message in the UI or enable an
error banner via the existing DOM with id/categoryRestrictionsBody), and stop or
continue based on failure policy; also change parseInt(option.value) to
parseInt(option.value, 10) when building selectedCategoryIds to avoid radix
pitfalls and include csrfToken and groupId in any error/log context for easier
debugging.
- Around line 316-333: The module-level cachedCategories and currentRestrictions
can become stale; update loadCategoryRestrictions to accept an optional
forceRefresh boolean (or always refetch) and call fetchCategoriesForRestrictions
when forceRefresh is true (or remove the one-time cache) so cachedCategories is
refreshed when needed; additionally, ensure any category-modifying functions
(e.g., the code paths that add/remove categories) clear or reset
cachedCategories so subsequent calls to loadCategoryRestrictions will re-fetch,
and keep currentRestrictions populated via fetchGroupCategoryRestrictions before
calling renderCategoryRestrictions.
In `@phpmyfaq/src/phpMyFAQ/Permission/GroupCategoryPermissionRepository.php`:
- Around line 116-163: The current setCategoryRestrictions method uses raw SQL
transaction commands via $db->query('BEGIN'/'COMMIT'/'ROLLBACK'); replace these
with the database driver's transaction API where available (e.g. call
$db->beginTransaction(), $db->commit(), $db->rollBack() or the driver's
equivalent) to rely on the driver's abstraction and error handling; update the
transaction calls in setCategoryRestrictions and ensure rollback is invoked via
the driver's rollBack method when deletes or inserts fail, keeping the existing
validation and casting logic for $groupId, $rightId and $categoryIds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 422ac9c3-00a8-41bc-889e-10e75cd63793
📒 Files selected for processing (18)
phpmyfaq/admin/assets/src/api/group.test.tsphpmyfaq/admin/assets/src/api/group.tsphpmyfaq/admin/assets/src/group/groups.tsphpmyfaq/admin/assets/src/interfaces/Group.tsphpmyfaq/assets/templates/admin/user/group.twigphpmyfaq/src/phpMyFAQ/Controller/Administration/Api/GroupController.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/GroupController.phpphpmyfaq/src/phpMyFAQ/Permission/GroupCategoryPermissionRepository.phpphpmyfaq/src/phpMyFAQ/Permission/MediumPermission.phpphpmyfaq/src/phpMyFAQ/Setup/Installation/DatabaseSchema.phpphpmyfaq/src/phpMyFAQ/Setup/Migration/MigrationRegistry.phpphpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha2.phpphpmyfaq/translations/language_en.phptests/phpMyFAQ/Permission/GroupCategoryPermissionRepositoryTest.phptests/phpMyFAQ/Permission/MediumPermissionTest.phptests/phpMyFAQ/Setup/Installation/DatabaseSchemaTest.phptests/phpMyFAQ/Setup/Installation/SchemaInstallerTest.phptests/phpMyFAQ/Setup/Migration/MigrationRegistryTest.php
🚧 Files skipped from review as they are similar to previous changes (6)
- phpmyfaq/assets/templates/admin/user/group.twig
- phpmyfaq/src/phpMyFAQ/Setup/Installation/DatabaseSchema.php
- phpmyfaq/src/phpMyFAQ/Permission/MediumPermission.php
- phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/GroupController.php
- phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha2.php
- tests/phpMyFAQ/Setup/Installation/DatabaseSchemaTest.php
The frontend appends the groupId as a path segment but the backend
route was missing the {groupId} placeholder, so
$request->attributes->get('groupId') was always null.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/GroupController.php (1)
192-202:⚠️ Potential issue | 🟠 MajorReject unknown category IDs instead of silently dropping invalid input.
The current coercion (
intval+> 0filter) accepts malformed payloads and can still persist stale/non-existent category IDs. Validate against canonical IDs fromCategory::loadCategories()before callingsetCategoryRestrictions().🛡️ Proposed change
$rawCategoryIds = $data['categoryIds'] ?? []; if (!is_array($rawCategoryIds)) { return $this->json(['error' => 'categoryIds must be an array.'], Response::HTTP_BAD_REQUEST); } - $categoryIds = array_values(array_filter( - array_map('intval', $rawCategoryIds), - static fn(int $id): bool => $id > 0, - )); + $knownCategoryIds = array_unique(array_map( + static fn(array $category): int => (int) $category['id'], + (new Category($this->configuration))->loadCategories(), + )); + + $categoryIds = []; + foreach ($rawCategoryIds as $categoryId) { + $categoryId = (int) $categoryId; + if ($categoryId <= 0 || !in_array($categoryId, $knownCategoryIds, true)) { + return $this->json(['error' => 'Invalid category ID.'], Response::HTTP_BAD_REQUEST); + } + $categoryIds[] = $categoryId; + } + $categoryIds = array_values(array_unique($categoryIds));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/GroupController.php` around lines 192 - 202, The controller currently coerces and silently drops invalid category IDs before calling perm->setCategoryRestrictions; instead load the canonical IDs via Category::loadCategories(), compare the incoming categoryIds (from $data['categoryIds']) against that canonical set, and if any requested IDs are unknown or invalid return a JSON error (HTTP_BAD_REQUEST) listing the bad IDs; only when all provided IDs are valid pass the validated array to $currentUser->perm->setCategoryRestrictions($groupId, $rightId, $categoryIds). Ensure you still enforce that categoryIds is an array and use Category::loadCategories() as the source of truth for validation.
🧹 Nitpick comments (1)
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/GroupController.php (1)
165-165: Rename POST route to match the Admin API naming convention.
admin.api.group.category-restrictions.saveintroduces an extra dot-segment and breaks theadmin.api.{resource}.{action}convention.🔧 Proposed change
- #[Route(path: 'group/category-restrictions', name: 'admin.api.group.category-restrictions.save', methods: ['POST'])] + #[Route(path: 'group/category-restrictions', name: 'admin.api.group.save-category-restrictions', methods: ['POST'])]As per coding guidelines: "Admin API routes should follow the naming convention: admin.api.{resource}.{action}".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/GroupController.php` at line 165, The route name on the Route attribute for the category-restrictions POST endpoint in GroupController currently uses an extra dot segment ("admin.api.group.category-restrictions.save"); change the name to follow the admin.api.{resource}.{action} convention by renaming it to something like "admin.api.group.saveCategoryRestrictions" (update the #[Route(path: 'group/category-restrictions', name: 'admin.api.group.category-restrictions.save', ...)] attribute to use the new name) so the resource is "group" and the action is a single token "saveCategoryRestrictions".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/GroupController.php`:
- Around line 192-202: The controller currently coerces and silently drops
invalid category IDs before calling perm->setCategoryRestrictions; instead load
the canonical IDs via Category::loadCategories(), compare the incoming
categoryIds (from $data['categoryIds']) against that canonical set, and if any
requested IDs are unknown or invalid return a JSON error (HTTP_BAD_REQUEST)
listing the bad IDs; only when all provided IDs are valid pass the validated
array to $currentUser->perm->setCategoryRestrictions($groupId, $rightId,
$categoryIds). Ensure you still enforce that categoryIds is an array and use
Category::loadCategories() as the source of truth for validation.
---
Nitpick comments:
In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/GroupController.php`:
- Line 165: The route name on the Route attribute for the category-restrictions
POST endpoint in GroupController currently uses an extra dot segment
("admin.api.group.category-restrictions.save"); change the name to follow the
admin.api.{resource}.{action} convention by renaming it to something like
"admin.api.group.saveCategoryRestrictions" (update the #[Route(path:
'group/category-restrictions', name:
'admin.api.group.category-restrictions.save', ...)] attribute to use the new
name) so the resource is "group" and the action is a single token
"saveCategoryRestrictions".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed515f57-1663-4af9-8d30-fa9911bdf6cb
📒 Files selected for processing (1)
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/GroupController.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.
Groups can now have their permissions scoped to specific categories. When no category restrictions are set, rights apply globally (backward compatible). This enables controlled external contributions where users can be restricted to specific categories via group membership.
New table faqgroup_right_category links group rights to categories. Added API endpoints for managing category restrictions, admin UI panel for configuring restrictions per permission, and hasPermissionForCategory() for category-scoped permission checks.
This feature implements and closes #3780.
Summary by CodeRabbit
New Features
API
Database
Permission
Tests
Localization