diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a5bef3972..a7425c7f29 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,7 @@ This is a log of major user-visible changes in each phpMyFAQ release. - added experimental support for API key authentication via OAuth2 (Thorsten) - added experimental per-tenant quota enforcement and API request rate limits (Thorsten) - added SBOM (Software Bill of Materials) generation (Thorsten) +- added separate read, write and publish permissions for FAQs (Thorsten) - improved audit and activity log with comprehensive security event tracking (Thorsten) - improved API errors with formatted RFC 7807 Problem Details JSON responses (Thorsten) - improved support for PDO (Thorsten) diff --git a/docs/administration.md b/docs/administration.md index e3614570dd..6d21969b3d 100644 --- a/docs/administration.md +++ b/docs/administration.md @@ -43,8 +43,10 @@ Keep in mind that new users have no privileges at all; you will have to assign t A super admin can set users as super admins as well, then the given permissions aren't evaluated, so please be careful with setting this option. -A user without any permission in the admin section can still get read access to categories and records. You can set -the permissions on categories and records in the category and record administration frontend. +A user without any permission in the admin section can still get read access to categories and records. Reading FAQs +requires the `View FAQs` right, which new accounts receive automatically and which the update grants to every existing +user and group — see 5.1.5. Beyond that, you can set the permissions on categories and records in the category and +record administration frontend. If you enable LDAP or Microsoft Entra ID authentication, you can't edit the user's profile in the admin area. The users will be created automatically when they log in the first time. @@ -96,6 +98,35 @@ restrictions). The rules are deliberately different from category restrictions: - Category and language restrictions combine with **AND** semantics: an action must be permitted by both the category and the language rules to succeed. +### 5.1.5 Read, write and publish rights + +Reading FAQs, changing them, and deciding that a change goes public are three independently assignable rights: + +| Right | Name in the permission list | Grants | +|---|---|---| +| Read | `View FAQs` (`view_faqs`) | Seeing FAQs at all as a logged-in user | +| Write | `Add`/`Edit`/`Delete FAQs` | Creating and changing FAQ content | +| Publish | `Publish FAQs` (`faq_publish`) | Setting an FAQ live, on its own or in the FAQ overview | + +- **A user with write but not publish can edit a published FAQ without changing whether it is public.** The editor shows + the current publication state read-only instead of the visibility radio buttons, and saving leaves that state alone. + The activation checkbox in the FAQ overview is likewise shown read-only. +- `Publish FAQs` is separate from the older `Approve FAQs` (`approverec`) right, which continues to gate the list of + inactive FAQs. Upgrades grant `faq_publish` to everyone who already held `approverec`, carrying their category and + language restrictions across, so no one loses or gains an ability. +- **`View FAQs` is only enforced for logged-in users.** Anonymous visitors are unaffected by it; what they may see is + still governed by `security.enableLoginOnly` and by the per-FAQ user and group permissions set in the FAQ editor. + Revoking `view_faqs` from a user therefore hides FAQs from that account, not from the public site. +- New accounts receive `view_faqs` automatically, and the upgrade grants it to every existing user and group, so + enabling the gate changes nothing until you deliberately revoke it. +- Both rights honour the category and language restrictions described in 5.1.3 and 5.1.4, with the same **AND** + semantics. A user whose `view_faqs` is restricted to one category sees only that category's FAQs — in FAQ pages, + category listings, search results, tag listings, the sitemap and the "latest"/"most popular" lists alike. +- Super admins bypass all of this, as they do every other right. +- FAQs submitted through the frontend are still activated according to the `records.defaultActivation` configuration + option. That is a site-wide setting about anonymous submissions rather than a right exercised by the submitter, so it + is deliberately not gated on `faq_publish`. + ## 5.2 Content ### 5.2.1 Category Administration diff --git a/docs/update.md b/docs/update.md index c3600d9442..a40d7e5d17 100644 --- a/docs/update.md +++ b/docs/update.md @@ -182,6 +182,21 @@ Then go to the "phpMyFAQ Update" page in the configuration section and click thr Note: The online update feature is experimental and might not work in all environments. +### Permission changes in phpMyFAQ 4.2 + +The update separates reading, writing and publishing FAQs into independent rights, without changing what anybody can +currently do: + +- The existing `View FAQs` (`view_faqs`) right starts being enforced for logged-in users. The update grants it to every + existing user and group, and new accounts receive it automatically, so nothing becomes invisible. Anonymous visitors + are not affected by the right at all. +- A new `Publish FAQs` (`faq_publish`) right decides whether an FAQ may be set live. The update grants it to every user + and group that currently holds `Approve FAQs` (`approverec`), including their category and language restrictions, so + everyone who can publish today can still publish afterwards. + +Afterwards you can revoke `faq_publish` from editors who should be able to write but not publish. See section 5.1.5 of +the administration documentation for the details. + ## Modifying templates for phpMyFAQ 4.2 We recommend you take a look at the main [Bootstrap documentation](https://getbootstrap.com/). diff --git a/phpmyfaq/admin/assets/src/content/faqs.overview.test.ts b/phpmyfaq/admin/assets/src/content/faqs.overview.test.ts index 9b30cfff4f..722e601b5b 100644 --- a/phpmyfaq/admin/assets/src/content/faqs.overview.test.ts +++ b/phpmyfaq/admin/assets/src/content/faqs.overview.test.ts @@ -227,6 +227,7 @@ describe('FAQ Overview Functions', () => { category_id: 3, sticky: 'no', active: 'yes', + isAllowedToPublish: true, }, ], isAllowedToTranslate: false, @@ -271,6 +272,42 @@ describe('FAQ Overview Functions', () => { expect(fetchAllFaqsByCategory).toHaveBeenCalledWith('3', 'en', true, true); }); + it('should render the active state read-only without the publish permission', async () => { + document.body.innerHTML = ` +
+
+ `; + + (fetchAllFaqsByCategory as Mock).mockResolvedValue({ + faqs: [ + { + id: 7, + language: 'en', + solution_id: 1007, + question: 'Test FAQ', + created: '2026-03-01', + category_id: 3, + sticky: 'no', + active: 'yes', + isAllowedToPublish: false, + }, + ], + isAllowedToTranslate: false, + }); + + await handleFaqOverview(); + + const category = document.querySelector('.accordion-collapse') as HTMLElement; + category.dispatchEvent(new Event('shown.bs.collapse')); + + await flushPromises(); + + // No checkbox to click, but the state stays visible, and the sticky toggle is unaffected. + expect(document.querySelector('.pmf-admin-active-faq')).toBeNull(); + expect(document.querySelector('.bi-check-lg')).not.toBeNull(); + expect(document.querySelector('.pmf-admin-sticky-faq')).not.toBeNull(); + }); + it('should call saveStatus when sticky toggle changes', async () => { document.body.innerHTML = `
@@ -288,6 +325,7 @@ describe('FAQ Overview Functions', () => { category_id: 3, sticky: 'no', active: 'yes', + isAllowedToPublish: true, }, ], isAllowedToTranslate: false, @@ -342,6 +380,7 @@ describe('FAQ Overview Functions', () => { category_id: 3, sticky: 'no', active: 'yes', + isAllowedToPublish: true, }, ], isAllowedToTranslate: false, @@ -394,6 +433,7 @@ describe('FAQ Overview Functions', () => { category_id: 3, sticky: 'no', active: 'yes', + isAllowedToPublish: true, }, ], isAllowedToTranslate: false, @@ -439,6 +479,7 @@ describe('FAQ Overview Functions', () => { category_id: 3, sticky: 'no', active: 'yes', + isAllowedToPublish: true, }, ], isAllowedToTranslate: false, @@ -481,6 +522,7 @@ describe('FAQ Overview Functions', () => { category_id: 3, sticky: 'no', active: 'yes', + isAllowedToPublish: true, }, ], isAllowedToTranslate: false, diff --git a/phpmyfaq/admin/assets/src/content/faqs.overview.ts b/phpmyfaq/admin/assets/src/content/faqs.overview.ts index fbd9646a3b..5cd5216496 100644 --- a/phpmyfaq/admin/assets/src/content/faqs.overview.ts +++ b/phpmyfaq/admin/assets/src/content/faqs.overview.ts @@ -266,19 +266,28 @@ const populateCategoryTable = async (categoryId: string, faqs: Faq[], isAllowedT }), ]) ); + // Without the publish right for every category of this FAQ the checkbox would only ever + // produce a 403, so show the state read-only instead of offering an action the API rejects. row.append( - addElement('td', { classList: 'align-middle' }, [ - addElement('input', { - classList: 'form-check-input pmf-admin-active-faq', - type: 'checkbox', - 'data-pmf-category-id-active': faq.category_id.toString(), - 'data-pmf-faq-id': faq.id.toString(), - 'data-pmf-csrf': csrfToken, - lang: faq.language, - id: `active_record_${faq.category_id.toString()}_${faq.id.toString()}`, - checked: faq.active === 'yes', - }), - ]) + faq.isAllowedToPublish + ? addElement('td', { classList: 'align-middle' }, [ + addElement('input', { + classList: 'form-check-input pmf-admin-active-faq', + type: 'checkbox', + 'data-pmf-category-id-active': faq.category_id.toString(), + 'data-pmf-faq-id': faq.id.toString(), + 'data-pmf-csrf': csrfToken, + lang: faq.language, + id: `active_record_${faq.category_id.toString()}_${faq.id.toString()}`, + checked: faq.active === 'yes', + }), + ]) + : addElement('td', { classList: 'align-middle' }, [ + addElement('i', { + classList: faq.active === 'yes' ? 'bi bi-check-lg text-success' : 'bi bi-x-lg text-muted', + 'aria-hidden': 'true', + }), + ]) ); row.append( addElement('td', { classList: 'align-middle text-center' }, [ diff --git a/phpmyfaq/admin/assets/src/interfaces/FaqList.ts b/phpmyfaq/admin/assets/src/interfaces/FaqList.ts index 87dd419a13..3ea1f14802 100644 --- a/phpmyfaq/admin/assets/src/interfaces/FaqList.ts +++ b/phpmyfaq/admin/assets/src/interfaces/FaqList.ts @@ -9,6 +9,7 @@ export interface Faq { updated: string; // Format: YYYYMMDDHHMMSS visits: number; created: string; // Format: YYYY-MM-DD HH:MM:SS + isAllowedToPublish: boolean; } export interface FaqList { diff --git a/phpmyfaq/assets/templates/admin/content/faq.editor.twig b/phpmyfaq/assets/templates/admin/content/faq.editor.twig index bf5af34eef..3d8e8cb282 100644 --- a/phpmyfaq/assets/templates/admin/content/faq.editor.twig +++ b/phpmyfaq/assets/templates/admin/content/faq.editor.twig @@ -585,7 +585,7 @@ {{ ad_entry_status }} - {% if hasPermissionForApprove %} + {% if hasPermissionForPublish %}
{% else %} -
- - -
+ {# No "active" field is submitted, so saving preserves the stored publication state. #} +

+ {{ isActive ? ad_entry_visibility : ad_entry_not_visibility }} + {{ msgNoPublishPermission }} +

{% endif %} diff --git a/phpmyfaq/src/phpMyFAQ/Administration/Faq.php b/phpmyfaq/src/phpMyFAQ/Administration/Faq.php index 20b70c27eb..d12de2f148 100644 --- a/phpmyfaq/src/phpMyFAQ/Administration/Faq.php +++ b/phpmyfaq/src/phpMyFAQ/Administration/Faq.php @@ -34,6 +34,19 @@ public function __construct( /** * Get all FAQs by category + * + * @return list */ public function getAllFaqsByCategory(int $categoryId, bool $onlyInactive = false, bool $onlyNew = false): array { @@ -102,15 +115,15 @@ public function getAllFaqsByCategory(int $categoryId, bool $onlyInactive = false $faqData[] = [ 'id' => (int) $row->id, - 'language' => $row->lang, + 'language' => (string) $row->lang, 'solution_id' => (int) $row->solution_id, - 'active' => $row->active, + 'active' => (string) $row->active, 'sticky' => $row->sticky ? 'yes' : 'no', 'category_id' => (int) $row->category_id, - 'question' => $row->question, - 'updated' => $row->updated, - 'visits' => (int) $visits, - 'created' => $row->created, + 'question' => (string) $row->question, + 'updated' => (string) $row->updated, + 'visits' => $visits, + 'created' => (string) $row->created, ]; } } diff --git a/phpmyfaq/src/phpMyFAQ/Category/Language/CategoryLanguageService.php b/phpmyfaq/src/phpMyFAQ/Category/Language/CategoryLanguageService.php index cb061378e9..78463aedce 100644 --- a/phpmyfaq/src/phpMyFAQ/Category/Language/CategoryLanguageService.php +++ b/phpmyfaq/src/phpMyFAQ/Category/Language/CategoryLanguageService.php @@ -39,7 +39,7 @@ public function getLanguagesToTranslate(Configuration $configuration, int $categ $result = []; foreach (LanguageHelper::getAvailableLanguages() as $lang => $name) { - $langLower = strtolower((string) $lang); + $langLower = strtolower($lang); if (!in_array($langLower, $existingLower, strict: true)) { $result[$langLower] = $name; } diff --git a/phpmyfaq/src/phpMyFAQ/Category/Relation.php b/phpmyfaq/src/phpMyFAQ/Category/Relation.php index d51d235d11..9196c1ddc0 100644 --- a/phpmyfaq/src/phpMyFAQ/Category/Relation.php +++ b/phpmyfaq/src/phpMyFAQ/Category/Relation.php @@ -332,6 +332,45 @@ private function aggregateRecursively( return $total; } + /** + * Returns the category ids of many records at once, keyed by record id — listing pages + * would otherwise issue one getCategories() query per FAQ. + * + * @param int[] $faqIds + * @return array> + */ + public function getCategoryIdsForRecords(array $faqIds, string $faqLang): array + { + $faqIds = array_values(array_unique(array_filter($faqIds, static fn(int $faqId): bool => $faqId > 0))); + + if ($faqIds === []) { + return []; + } + + $query = sprintf( + 'SELECT record_id, category_id FROM %sfaqcategoryrelations WHERE record_id IN (%s)' + . ' AND record_lang = ?', + Database::getTablePrefix(), + implode(', ', array_fill(start_index: 0, count: count($faqIds), value: '?')), + ); + + $categoryIdsByFaq = []; + $result = $this->configuration->getDb()->queryPrepared($query, [...$faqIds, $faqLang]); + + if ($result) { + while (true) { + $row = $this->configuration->getDb()->fetchObject($result); + if (!$row instanceof \stdClass) { + break; + } + + $categoryIdsByFaq[(int) $row->record_id][] = (int) $row->category_id; + } + } + + return $categoryIdsByFaq; + } + /** * Returns the categories from a FAQ id and language. * diff --git a/phpmyfaq/src/phpMyFAQ/Controller/AbstractController.php b/phpmyfaq/src/phpMyFAQ/Controller/AbstractController.php index 9386655a61..b73c28aecf 100644 --- a/phpmyfaq/src/phpMyFAQ/Controller/AbstractController.php +++ b/phpmyfaq/src/phpMyFAQ/Controller/AbstractController.php @@ -447,6 +447,67 @@ protected function userHasPermissionForLanguage(PermissionType $permissionType, } } + /** + * Ensures the user may make FAQ content live in the given categories and language. + * + * Publishing is a right of its own: editing an FAQ and deciding that it goes public are + * separate decisions, so this is asserted only on requests that actually change the + * publication state, on top of the FAQ_ADD or FAQ_EDIT guard the endpoint already applies. + * + * @param int[] $categoryIds + * @throws UnauthorizedHttpException|ForbiddenException + */ + protected function userMayPublish(array $categoryIds, string $language): void + { + $this->userHasPermission(PermissionType::FAQ_PUBLISH); + $this->userHasPermissionForCategories(PermissionType::FAQ_PUBLISH, $categoryIds); + $this->userHasPermissionForLanguage(PermissionType::FAQ_PUBLISH, $language); + } + + /** + * Non-throwing companion to userMayPublish(), for the callers that need to decide between + * two valid outcomes rather than reject the request — a user without the publish right may + * still save an FAQ, their edit simply must not change its publication state. + * + * Mirrors the empty-list policy of userHasPermissionForCategories(): with no categories to + * check against, only a user whose right is unrestricted qualifies. + * + * @param int[] $categoryIds + * @throws \Exception + */ + protected function userMayPublishIn(array $categoryIds, string $language): bool + { + $currentUser = $this->currentUser; + if (!$currentUser->isLoggedIn()) { + return false; + } + + $userId = $currentUser->getUserId(); + $right = PermissionType::FAQ_PUBLISH->value; + + if (!$currentUser->perm->hasPermission($userId, $right)) { + return false; + } + + if (!$currentUser->perm->hasPermissionForLanguage($userId, $right, $language)) { + return false; + } + + $categoryIds = array_filter(array_unique($categoryIds), static fn(int $id): bool => $id > 0); + + if ($categoryIds === []) { + return $currentUser->perm->getAllowedCategoriesForRight($userId, $right) === null; + } + + foreach ($categoryIds as $categoryId) { + if (!$currentUser->perm->hasPermissionForCategory($userId, $right, $categoryId)) { + return false; + } + } + + return true; + } + /** * Grants access when the user owns at least one of the given permissions. * diff --git a/phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/FaqController.php b/phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/FaqController.php index b3e2fe1cd2..78bb7d3d24 100644 --- a/phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/FaqController.php +++ b/phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/FaqController.php @@ -29,6 +29,7 @@ use phpMyFAQ\Category; use phpMyFAQ\Category\Permission as CategoryPermission; use phpMyFAQ\Category\Relation; +use phpMyFAQ\Controller\Exception\ForbiddenException; use phpMyFAQ\Entity\FaqEntity; use phpMyFAQ\Entity\SeoEntity; use phpMyFAQ\Enums\AdminLogType; @@ -132,6 +133,12 @@ public function create(Request $request): JsonResponse $tags = Filter::filterVar($data->tags ?? '', FILTER_SANITIZE_SPECIAL_CHARS, ''); $active = Filter::filterVar($data->active ?? 'no', FILTER_SANITIZE_SPECIAL_CHARS, 'no'); + + // Creating an FAQ and making it public are separate rights. + if ($active === 'yes') { + $this->userMayPublish($categories, $language); + } + $sticky = Filter::filterVar($data->sticky ?? 'no', FILTER_SANITIZE_SPECIAL_CHARS, 'no'); $content = Filter::filterVar($data->answer ?? '', FILTER_SANITIZE_SPECIAL_CHARS, ''); $keywords = Filter::filterVar($data->keywords ?? '', FILTER_SANITIZE_SPECIAL_CHARS, ''); @@ -368,7 +375,7 @@ public function update(Request $request): JsonResponse $this->userHasPermissionForLanguage(PermissionType::FAQ_EDIT, $faqLang); $tags = Filter::filterVar($data->tags ?? '', FILTER_SANITIZE_SPECIAL_CHARS, ''); - $active = Filter::filterVar($data->active ?? 'no', FILTER_SANITIZE_SPECIAL_CHARS, 'no'); + $active = $this->resolvePublicationState($data, $faqId, $faqLang, [...$categories, ...$currentCategoryIds]); $sticky = Filter::filterVar($data->sticky ?? 'no', FILTER_SANITIZE_SPECIAL_CHARS, 'no'); $content = Filter::filterVar($data->answer ?? '', FILTER_SANITIZE_SPECIAL_CHARS, ''); $keywords = Filter::filterVar($data->keywords ?? '', FILTER_SANITIZE_SPECIAL_CHARS, ''); @@ -574,15 +581,17 @@ public function listByCategory(Request $request): JsonResponse $faq = new FaqAdministration($this->configuration); $faq->setLanguage($language); + $faqs = $faq->getAllFaqsByCategory($categoryId, $onlyInactive, $onlyNew); + return $this->json([ - 'faqs' => $faq->getAllFaqsByCategory($categoryId, $onlyInactive, $onlyNew), + 'faqs' => $this->withPublishCapability($faqs, $language), 'isAllowedToTranslate' => - $this->currentUser?->perm->hasPermissionForCategory( + $this->currentUser->perm->hasPermissionForCategory( $this->currentUser->getUserId(), PermissionType::FAQ_TRANSLATE->value, $categoryId, ) - && $this->currentUser?->perm->hasPermissionForLanguage( + && $this->currentUser->perm->hasPermissionForLanguage( $this->currentUser->getUserId(), PermissionType::FAQ_TRANSLATE->value, $language, @@ -596,7 +605,7 @@ public function listByCategory(Request $request): JsonResponse #[Route(path: 'faq/activate', name: 'admin.api.faq.activate', methods: ['POST'])] public function activate(Request $request): JsonResponse { - $this->userHasPermission(PermissionType::FAQ_APPROVE); + $this->userHasPermission(PermissionType::FAQ_PUBLISH); $data = $this->getJsonObject($request); @@ -613,13 +622,13 @@ public function activate(Request $request): JsonResponse } if ($faqIds !== []) { - $this->userHasPermissionForLanguage(PermissionType::FAQ_APPROVE, $faqLanguage); + $this->userHasPermissionForLanguage(PermissionType::FAQ_PUBLISH, $faqLanguage); $activateCategory = new Category($this->configuration, [], withPermission: false); $activateCategoryRelation = new Relation($this->configuration, $activateCategory); foreach ($faqIds as $faqId) { $this->userHasPermissionForCategories( - PermissionType::FAQ_APPROVE, + PermissionType::FAQ_PUBLISH, array_keys($activateCategoryRelation->getCategories($faqId, $faqLanguage)), ); } @@ -636,7 +645,10 @@ public function activate(Request $request): JsonResponse } if ($success) { - $this->adminLog->log($this->currentUser, AdminLogType::FAQ_EDIT->value); + $this->adminLog->log( + $this->currentUser, + ($checked ? AdminLogType::FAQ_PUBLISH : AdminLogType::FAQ_EDIT)->value, + ); return $this->json(['success' => Translation::get(key: 'ad_entry_savedsuc')], Response::HTTP_OK); } @@ -827,6 +839,7 @@ public function import(Request $request): JsonResponse { $this->userHasPermission(PermissionType::FAQ_ADD); + /** @var UploadedFile|null $file */ $file = $request->files->get(key: 'file'); if (!$file instanceof UploadedFile) { return $this->json(['error' => 'Bad request: There is no file submitted.'], Response::HTTP_BAD_REQUEST); @@ -925,7 +938,7 @@ private function findImportRowsOutOfScope(array $csvData): array ++$rowNumber; $categoryId = (int) Filter::filterVar($record[0] ?? null, FILTER_VALIDATE_INT, default: 0); - $language = (string) Filter::filterVar($record[4] ?? '', FILTER_SANITIZE_SPECIAL_CHARS, ''); + $language = Filter::filterVar($record[4] ?? '', FILTER_SANITIZE_SPECIAL_CHARS, ''); if (!$currentUser->perm->hasPermissionForCategory($userId, PermissionType::FAQ_ADD->value, $categoryId)) { $messages[] = sprintf( @@ -949,4 +962,98 @@ private function findImportRowsOutOfScope(array $csvData): array return $messages; } + + /** + * Adds isAllowedToPublish to every listed FAQ with a fixed number of lookups. + * + * activate() authorizes against every category an FAQ is attached to, so the active-state + * checkbox has to be offered on the same basis — judging only the requested category would + * offer an action the API then rejects with a 403 for multi-category FAQs. The publish + * scope is resolved once and the category memberships of all listed FAQs are loaded in one + * query, because a userMayPublishIn() call per FAQ issues several permission queries per + * row. + * + * @param list $faqs + * @return list + * @throws Exception + */ + private function withPublishCapability(array $faqs, string $language): array + { + $currentUser = $this->currentUser; + $publishRight = PermissionType::FAQ_PUBLISH->value; + + // Nobody may publish until the right, the language and the scope say otherwise. + $allowedCategoryIds = []; + + if ($currentUser->isLoggedIn()) { + $userId = $currentUser->getUserId(); + + if ( + $currentUser->perm->hasPermission($userId, $publishRight) + && $currentUser->perm->hasPermissionForLanguage($userId, $publishRight, $language) + ) { + // null means unrestricted — the same contract userMayPublishIn() evaluates + // per category via hasPermissionForCategory(). + $allowedCategoryIds = $currentUser->perm->getAllowedCategoriesForRight($userId, $publishRight); + } + } + + $categoryIdsByFaq = + $allowedCategoryIds === null || $allowedCategoryIds === [] + ? [] + : new Relation( + $this->configuration, + new Category($this->configuration, [], withPermission: false), + )->getCategoryIdsForRecords(array_column($faqs, 'id'), $language); + + foreach ($faqs as &$faqData) { + $faqCategoryIds = $categoryIdsByFaq[$faqData['id']] ?? []; + $faqData['isAllowedToPublish'] = + $allowedCategoryIds === null + || $faqCategoryIds !== [] && array_diff($faqCategoryIds, $allowedCategoryIds) === []; + } + + unset($faqData); + + return $faqs; + } + + /** + * Decides the publication state an update should persist. + * + * A user without the publish right may still edit a published FAQ; their save simply must + * not change whether it is live. The editor omits the field entirely for those users, and an + * absent field is deliberately distinct from an explicit "no" — treating the two alike would + * unpublish an FAQ on every ordinary save. A request that does try to change the state + * without the right is rejected rather than silently downgraded, so the caller never gets a + * success response for something that did not happen. + * + * @param int[] $categoryIds the FAQ's current and submitted categories + * @throws ForbiddenException|Exception + */ + private function resolvePublicationState( + stdClass $data, + int $faqId, + string $faqLanguage, + array $categoryIds, + ): string { + $requestedActive = property_exists($data, 'active') + ? Filter::filterVar($data->active, FILTER_SANITIZE_SPECIAL_CHARS, 'no') + : null; + + $currentActive = $this->faq->isActive($faqId, $faqLanguage) ? 'yes' : 'no'; + + if ($this->userMayPublishIn($categoryIds, $faqLanguage)) { + return $requestedActive ?? $currentActive; + } + + if ($requestedActive !== null && $requestedActive !== $currentActive) { + throw new ForbiddenException(message: sprintf( + 'User has no "%s" permission.', + PermissionType::FAQ_PUBLISH->name, + )); + } + + return $currentActive; + } } diff --git a/phpmyfaq/src/phpMyFAQ/Controller/Administration/FaqController.php b/phpmyfaq/src/phpMyFAQ/Controller/Administration/FaqController.php index fa95c35f90..ed057fe04c 100644 --- a/phpmyfaq/src/phpMyFAQ/Controller/Administration/FaqController.php +++ b/phpmyfaq/src/phpMyFAQ/Controller/Administration/FaqController.php @@ -83,10 +83,14 @@ public function __construct( #[Route(path: '/faqs', name: 'admin.faqs', methods: ['GET'])] public function index(Request $request): Response { - $this->userHasPermission(PermissionType::FAQ_ADD); - $this->userHasPermission(PermissionType::FAQ_APPROVE); - $this->userHasPermission(PermissionType::FAQ_EDIT); - $this->userHasPermission(PermissionType::FAQ_DELETE); + // Adding, editing, deleting and publishing are independently assignable, so holding any + // one of them is enough to reach the overview; each action enforces its own right. + $this->userHasAnyPermission( + PermissionType::FAQ_ADD, + PermissionType::FAQ_EDIT, + PermissionType::FAQ_DELETE, + PermissionType::FAQ_PUBLISH, + ); [$currentAdminUser, $currentAdminGroups] = CurrentUser::getCurrentUserGroupId($this->currentUser); @@ -168,9 +172,9 @@ public function add(Request $request): Response 'restrictedUsers' => false, 'userSelection' => $this->userHelper->getAllUsersForTemplate(-1, true), 'changelogs' => [], - 'hasPermissionForApprove' => $this->currentUser?->perm->hasPermission( + 'hasPermissionForPublish' => $this->currentUser?->perm->hasPermission( $this->currentUser->getUserId(), - PermissionType::FAQ_APPROVE->value, + PermissionType::FAQ_PUBLISH->value, ), 'isActive' => null, 'isInActive' => 'checked', @@ -240,9 +244,9 @@ public function addInCategory(Request $request): Response 'restrictedUsers' => false, 'userSelection' => $this->userHelper->getAllUsersForTemplate(-1, true), 'changelogs' => [], - 'hasPermissionForApprove' => $this->currentUser?->perm->hasPermission( + 'hasPermissionForPublish' => $this->currentUser?->perm->hasPermission( $this->currentUser->getUserId(), - PermissionType::FAQ_APPROVE->value, + PermissionType::FAQ_PUBLISH->value, ), 'isActive' => null, 'isInActive' => 'checked', @@ -380,10 +384,9 @@ public function edit(Request $request): Response 'restrictedUsers' => $restrictedUsers, 'userSelection' => $this->userHelper->getAllUsersForTemplate(-1, true), 'changelogs' => $this->changelog->getByFaqId($faqId), - 'hasPermissionForApprove' => $this->currentUser?->perm->hasPermission( - $this->currentUser->getUserId(), - PermissionType::FAQ_APPROVE->value, - ), + // Scoped to this FAQ, so a publisher restricted to other categories or languages is + // not offered a control that the API would reject. + 'hasPermissionForPublish' => $this->userMayPublishIn(array_keys($categories), $faqLanguage), 'isActive' => $faqData['active'] === 'yes' ? 'checked' : null, 'isInActive' => $faqData['active'] !== 'yes' ? 'checked' : null, 'nextSolutionId' => $this->faq->getNextSolutionId(), @@ -452,9 +455,9 @@ public function copy(Request $request): Response 'restrictedUsers' => false, 'userSelection' => $this->userHelper->getAllUsersForTemplate(-1, true), 'changelogs' => [], - 'hasPermissionForApprove' => $this->currentUser?->perm->hasPermission( + 'hasPermissionForPublish' => $this->currentUser?->perm->hasPermission( $this->currentUser->getUserId(), - PermissionType::FAQ_APPROVE->value, + PermissionType::FAQ_PUBLISH->value, ), 'isActive' => null, 'isInActive' => null, @@ -524,9 +527,9 @@ public function translate(Request $request): Response 'restrictedUsers' => false, 'userSelection' => $this->userHelper->getAllUsersForTemplate(-1, true), 'changelogs' => [], - 'hasPermissionForApprove' => $this->currentUser?->perm->hasPermission( + 'hasPermissionForPublish' => $this->currentUser?->perm->hasPermission( $this->currentUser->getUserId(), - PermissionType::FAQ_APPROVE->value, + PermissionType::FAQ_PUBLISH->value, ), 'isActive' => null, 'isInActive' => null, @@ -607,9 +610,9 @@ public function answer(Request $request): Response 'restrictedUsers' => false, 'userSelection' => $this->userHelper->getAllUsersForTemplate(-1, true), 'changelogs' => [], - 'hasPermissionForApprove' => $this->currentUser?->perm->hasPermission( + 'hasPermissionForPublish' => $this->currentUser?->perm->hasPermission( $this->currentUser->getUserId(), - PermissionType::FAQ_APPROVE->value, + PermissionType::FAQ_PUBLISH->value, ), 'isActive' => null, 'isInActive' => null, @@ -667,6 +670,7 @@ private function getBaseTemplateVars(): array 'ad_entry_status' => Translation::get(key: 'ad_entry_status'), 'ad_entry_visibility' => Translation::get(key: 'ad_entry_visibility'), 'ad_entry_not_visibility' => Translation::get(key: 'ad_entry_not_visibility'), + 'msgNoPublishPermission' => Translation::get(key: 'msgNoPublishPermission'), 'ad_entry_new_revision' => Translation::get(key: 'ad_entry_new_revision'), 'ad_gen_yes' => Translation::get(key: 'ad_gen_yes'), 'ad_gen_no' => Translation::get(key: 'ad_gen_no'), diff --git a/phpmyfaq/src/phpMyFAQ/Controller/Api/FaqController.php b/phpmyfaq/src/phpMyFAQ/Controller/Api/FaqController.php index d9021c2c3e..a59f34a754 100644 --- a/phpmyfaq/src/phpMyFAQ/Controller/Api/FaqController.php +++ b/phpmyfaq/src/phpMyFAQ/Controller/Api/FaqController.php @@ -23,6 +23,7 @@ use League\CommonMark\Exception\CommonMarkException; use OpenApi\Attributes as OA; use phpMyFAQ\Category; +use phpMyFAQ\Category\Relation as CategoryRelation; use phpMyFAQ\Entity\FaqEntity; use phpMyFAQ\Enums\PermissionType; use phpMyFAQ\Faq; @@ -758,8 +759,13 @@ public function create(Request $request): JsonResponse } $categories = [$categoryId]; - $isActive = !is_null($isActive); - $isSticky = !is_null($isSticky); + $isActive = $isActive === true; + $isSticky = $isSticky === true; + + // Creating an FAQ and making it public are separate rights. + if ($isActive) { + $this->userMayPublish([(int) $categoryId], $languageCode); + } $faqData = new FaqEntity(); $faqData @@ -909,8 +915,18 @@ public function update(Request $request): JsonResponse return $this->json($result, Response::HTTP_BAD_REQUEST); } - $isActive = !is_null($isActive); - $isSticky = !is_null($isSticky); + $isActive = $isActive === true; + $isSticky = $isSticky === true; + + // Editing an FAQ and deciding that it goes live are separate rights, so a change of + // the publication state needs FAQ_PUBLISH on top of the FAQ_EDIT guard above. + if ($isActive !== $this->faq->isActive($faqId, $languageCode)) { + $faqCategories = new CategoryRelation($this->configuration, $category)->getCategories( + $faqId, + $languageCode, + ); + $this->userMayPublish(array_keys($faqCategories), $languageCode); + } $faqEntity = new FaqEntity(); $faqEntity diff --git a/phpmyfaq/src/phpMyFAQ/Controller/Frontend/FaqController.php b/phpmyfaq/src/phpMyFAQ/Controller/Frontend/FaqController.php index 44d548e310..44da51f108 100644 --- a/phpmyfaq/src/phpMyFAQ/Controller/Frontend/FaqController.php +++ b/phpmyfaq/src/phpMyFAQ/Controller/Frontend/FaqController.php @@ -185,6 +185,8 @@ public function solution(Request $request): Response return new Response('', Response::HTTP_NOT_FOUND); } + $this->applyFaqAccessContext($this->faq); + $faqData = $this->faq->getIdFromSolutionId($solutionId); if ($faqData === []) { @@ -220,6 +222,8 @@ public function contentRedirect(Request $request): Response return new Response('', Response::HTTP_NOT_FOUND); } + $this->applyFaqAccessContext($this->faq); + // Query the FAQ data directly for the specified language $result = $this->faq->getFaqResult($faqId, $faqLang); @@ -286,7 +290,7 @@ public function show(Request $request): Response // Initialize core objects $faq = new Faq($this->configuration); - $currentGroups = $this->currentUser->perm->getUserGroups($this->currentUser->getUserId()); + $currentGroups = $this->applyFaqAccessContext($faq); // Handle bookmarks if ($bookmarkAction === 'add' && $faqId > 0) { @@ -522,6 +526,25 @@ public function show(Request $request): Response return $this->render('faq.twig', $templateVars); } + /** + * Hands the requester's identity to a Faq instance before it looks anything up. + * + * Without this every lookup evaluates permissions as an anonymous visitor: a logged-in + * user is denied FAQs that were shared with them specifically, and a scoped user can + * resolve titles of FAQs outside their read scope. + * + * @return int[] the resolved group ids of the current user + */ + private function applyFaqAccessContext(Faq $faq): array + { + $currentGroups = $this->currentUser->perm->getUserGroups($this->currentUser->getUserId()); + + $faq->setUser($this->currentUser->getUserId()); + $faq->setGroups($currentGroups); + + return $currentGroups; + } + /** * Prepares comment data for the Twig macro * diff --git a/phpmyfaq/src/phpMyFAQ/Enums/PermissionType.php b/phpmyfaq/src/phpMyFAQ/Enums/PermissionType.php index 20f4063831..fd98c9ae01 100644 --- a/phpmyfaq/src/phpMyFAQ/Enums/PermissionType.php +++ b/phpmyfaq/src/phpMyFAQ/Enums/PermissionType.php @@ -54,6 +54,8 @@ enum PermissionType: string case FAQ_EDIT = 'edit_faq'; + case FAQ_PUBLISH = 'faq_publish'; + case FAQ_TRANSLATE = 'translate_faq'; case FAQS_VIEW = 'view_faqs'; diff --git a/phpmyfaq/src/phpMyFAQ/Faq.php b/phpmyfaq/src/phpMyFAQ/Faq.php index d3e3a72419..8ae29a48d6 100755 --- a/phpmyfaq/src/phpMyFAQ/Faq.php +++ b/phpmyfaq/src/phpMyFAQ/Faq.php @@ -30,6 +30,7 @@ use phpMyFAQ\Entity\FaqEntity; use phpMyFAQ\Faq\FaqRepository; use phpMyFAQ\Faq\QueryHelper; +use phpMyFAQ\Faq\ReadScope; use phpMyFAQ\Faq\RecordVisibility; use phpMyFAQ\Helper\FaqHelper; use phpMyFAQ\Instance\Search\Elasticsearch; @@ -91,6 +92,8 @@ class Faq private bool $groupSupport = false; private ?TenantQuotaEnforcer $tenantQuotaEnforcer = null; + private ?ReadScope $readScope = null; + private readonly FaqRepository $faqRepository; /** @@ -110,6 +113,11 @@ public function __construct( public function setUser(int $userId = -1): Faq { $this->user = $userId; + // Resolved here, once, because this is where the requester becomes known. Every query + // below then reuses the same scope instead of re-deriving it. + $this->readScope = ReadScope::forUserId($this->configuration, $userId); + $this->faqRepository->setReadScope($this->readScope); + return $this; } @@ -973,7 +981,7 @@ public function get( ): array { $faqs = []; - $queryHelper = new QueryHelper($this->user, $this->groups); + $queryHelper = new QueryHelper($this->user, $this->groups, $this->readScope); $query = $queryHelper->getQuery($queryType, $categoryId, $downwards, $lang, $date); $result = $this->configuration->getDb()->query($query); diff --git a/phpmyfaq/src/phpMyFAQ/Faq/FaqRepository.php b/phpmyfaq/src/phpMyFAQ/Faq/FaqRepository.php index b2da35c49c..bb96867fb3 100644 --- a/phpmyfaq/src/phpMyFAQ/Faq/FaqRepository.php +++ b/phpmyfaq/src/phpMyFAQ/Faq/FaqRepository.php @@ -24,13 +24,25 @@ use phpMyFAQ\Database; use phpMyFAQ\Entity\FaqEntity; +/* @mago-expect lint:too-many-methods - one query method per FAQ lookup; a split is planned with the Faq facade rework */ final class FaqRepository implements FaqRepositoryInterface { + /** + * Resolved once by Faq::setUser(), rather than per query. Null means no requester was + * established, which leaves the read gate off and the record ACL at its anonymous default. + */ + private ?ReadScope $readScope = null; + public function __construct( private readonly Configuration $configuration, ) { } + public function setReadScope(?ReadScope $readScope): void + { + $this->readScope = $readScope; + } + public function getNextSolutionId(): int { $latestId = 0; @@ -105,7 +117,7 @@ public function isFaqVisibleForUser( array $groups, bool $groupSupport, ): bool { - $queryHelper = new QueryHelper($userId, $groups); + $queryHelper = new QueryHelper($userId, $groups, $this->readScope); $now = date(format: 'YmdHis'); $query = sprintf( @@ -140,7 +152,7 @@ public function isFaqVisibleForUser( $this->configuration->getDb()->escape($faqLang), $now, $now, - $queryHelper->queryPermission($groupSupport), + $queryHelper->queryPermission($groupSupport) . $queryHelper->queryReadScope(), ); $result = $this->configuration->getDb()->query($query); @@ -184,7 +196,7 @@ public function isActive(int $faqId, string $faqLang, string $commentType = 'faq public function getIdFromSolutionId(int $solutionId, int $userId, array $groups, bool $groupSupport): array { - $queryHelper = new QueryHelper($userId, $groups); + $queryHelper = new QueryHelper($userId, $groups, $this->readScope); $query = sprintf( ' SELECT @@ -217,7 +229,7 @@ public function getIdFromSolutionId(int $solutionId, int $userId, array $groups, Database::getTablePrefix(), Database::getTablePrefix(), $solutionId, - $queryHelper->queryPermission($groupSupport), + $queryHelper->queryPermission($groupSupport) . $queryHelper->queryReadScope('fd', 'fcr'), ); $result = $this->configuration->getDb()->query($query); @@ -292,7 +304,7 @@ public function getFaqResult( array $groups, bool $groupSupport, ): mixed { - $queryHelper = new QueryHelper($userId, $groups); + $queryHelper = new QueryHelper($userId, $groups, $this->readScope); $query = sprintf( "SELECT id, lang, solution_id, revision_id, active, sticky, keywords, @@ -321,7 +333,9 @@ public function getFaqResult( $faqId, $faqRevisionId !== null ? 'AND revision_id = ' . $faqRevisionId : '', $this->configuration->getDb()->escape($faqLanguage), - $isAdmin ? 'AND 1=1' : $queryHelper->queryPermission($groupSupport), + // An admin context skips the record ACL but not the read scope: a category-restricted + // editor is exactly who that scope exists for. + ($isAdmin ? 'AND 1=1' : $queryHelper->queryPermission($groupSupport)) . $queryHelper->queryReadScope(), ); return $this->configuration->getDb()->query($query); @@ -335,7 +349,7 @@ public function fetchFaqByIdAndCategoryId( array $groups, bool $groupSupport, ): ?object { - $queryHelper = new QueryHelper($userId, $groups); + $queryHelper = new QueryHelper($userId, $groups, $this->readScope); $now = date(format: 'YmdHis'); $query = sprintf( " @@ -391,7 +405,7 @@ public function fetchFaqByIdAndCategoryId( $onlyActive ? sprintf("AND fd.active = 'yes' AND fd.date_start <= '%s' AND fd.date_end >= '%s'", $now, $now) : '', - $queryHelper->queryPermission($groupSupport), + $queryHelper->queryPermission($groupSupport) . $queryHelper->queryReadScope('fd', 'fcr'), ); $result = $this->configuration->getDb()->query($query); @@ -402,7 +416,7 @@ public function fetchFaqByIdAndCategoryId( public function fetchRowBySolutionId(int $solutionId, int $userId, array $groups, bool $groupSupport): ?\stdClass { - $queryHelper = new QueryHelper($userId, $groups); + $queryHelper = new QueryHelper($userId, $groups, $this->readScope); $query = sprintf( 'SELECT fd.*, COALESCE(fdg.group_id, -1) AS group_id, fdu.user_id @@ -428,7 +442,7 @@ public function fetchRowBySolutionId(int $solutionId, int $userId, array $groups $solutionId, Database::getTablePrefix(), $solutionId, - $queryHelper->queryPermission($groupSupport), + $queryHelper->queryPermission($groupSupport) . $queryHelper->queryReadScope(), ); $result = $this->configuration->getDb()->query($query); @@ -450,10 +464,13 @@ public function fetchRowBySolutionId(int $solutionId, int $userId, array $groups && $this->configuration->getDb()->fetchObject($restrictionResult) instanceof \stdClass; if (!$hasRestriction) { + // The fallback deliberately ignores the record ACL for FAQs that carry none, but + // it must still honour the read scope, or it would become a way around it. $fallbackQuery = sprintf( - 'SELECT * FROM %sfaqdata fd WHERE fd.solution_id = %d LIMIT 1', + 'SELECT * FROM %sfaqdata fd WHERE fd.solution_id = %d %s LIMIT 1', Database::getTablePrefix(), $solutionId, + $queryHelper->queryReadScope(), ); $fallbackResult = $this->configuration->getDb()->query($fallbackQuery); $row = $this->configuration->getDb()->fetchObject($fallbackResult); @@ -476,7 +493,7 @@ public function fetchAvailableFaqsByCategoryId( bool $groupSupport, ): array { $now = date(format: 'YmdHis'); - $queryHelper = new QueryHelper($userId, $groups); + $queryHelper = new QueryHelper($userId, $groups, $this->readScope); $query = sprintf( " SELECT @@ -522,7 +539,7 @@ public function fetchAvailableFaqsByCategoryId( $now, $categoryId, $this->configuration->getDb()->escape($this->configuration->getLanguage()->getLanguage()), - $queryHelper->queryPermissionExistsAny($groupSupport), + $queryHelper->queryPermissionExistsAny($groupSupport) . $queryHelper->queryReadScope('fd', 'fcr'), $orderTable, $orderColumn, $sortDirection, @@ -542,7 +559,7 @@ public function fetchFaqsByIds( bool $groupSupport, ): array { $now = date(format: 'YmdHis'); - $queryHelper = new QueryHelper($userId, $groups); + $queryHelper = new QueryHelper($userId, $groups, $this->readScope); $query = sprintf( "SELECT fd.id AS id, @@ -591,7 +608,7 @@ public function fetchFaqsByIds( $onlyActive ? sprintf("AND fd.active = 'yes' AND fd.date_start <= '%s' AND fd.date_end >= '%s'", $now, $now) : '', - $queryHelper->queryPermission($groupSupport), + $queryHelper->queryPermission($groupSupport) . $queryHelper->queryReadScope('fd', 'fcr'), ); return $this->fetchAllRows($this->configuration->getDb()->query($query)); @@ -602,7 +619,7 @@ public function fetchFaqsByIds( */ public function fetchStickyFaqs(int $userId, array $groups, bool $groupSupport): array { - $queryHelper = new QueryHelper($userId, $groups); + $queryHelper = new QueryHelper($userId, $groups, $this->readScope); $query = sprintf( " SELECT @@ -651,7 +668,7 @@ public function fetchStickyFaqs(int $userId, array $groups, bool $groupSupport): Database::getTablePrefix(), Database::getTablePrefix(), $this->configuration->getDb()->escape($this->configuration->getLanguage()->getLanguage()), - $queryHelper->queryPermission($groupSupport), + $queryHelper->queryPermission($groupSupport) . $queryHelper->queryReadScope('fd', 'fcr'), ); return $this->fetchAllRows($this->configuration->getDb()->query($query)); @@ -674,7 +691,7 @@ public function fetchAllFaqs( ' group by fd.id, fcr.category_id,fd.solution_id,fd.revision_id,fd.active,fd.sticky,fd.keywords,' . 'fd.thema,fd.content,fd.author,fd.email,fd.comment,fd.updated,' . 'fd.date_start,fd.date_end,fd.sticky,fd.created,fd.notes,fd.lang '; - $queryHelper = new QueryHelper($userId, $groups); + $queryHelper = new QueryHelper($userId, $groups, $this->readScope); $query = sprintf( ' SELECT @@ -722,7 +739,7 @@ public function fetchAllFaqs( Database::getTablePrefix(), Database::getTablePrefix(), $where, - $queryHelper->queryPermission($groupSupport), + $queryHelper->queryPermission($groupSupport) . $queryHelper->queryReadScope('fd', 'fcr'), $groupBy, $orderBy, ); @@ -870,7 +887,7 @@ public function queryRenderableFaqsByCategoryId( int $rowcount = 0, ): mixed { $now = date(format: 'YmdHis'); - $queryHelper = new QueryHelper($userId, $groups); + $queryHelper = new QueryHelper($userId, $groups, $this->readScope); $query = sprintf( " SELECT @@ -914,7 +931,7 @@ public function queryRenderableFaqsByCategoryId( $now, $categoryId, $this->configuration->getDb()->escape($this->configuration->getLanguage()->getLanguage()), - $queryHelper->queryPermissionExistsAny($groupSupport), + $queryHelper->queryPermissionExistsAny($groupSupport) . $queryHelper->queryReadScope('fd', 'fcr'), $order, ); @@ -932,19 +949,22 @@ public function countRenderableFaqsByCategoryId( bool $groupSupport, ): int { $now = date(format: 'YmdHis'); - $queryHelper = new QueryHelper($userId, $groups); + $queryHelper = new QueryHelper($userId, $groups, $this->readScope); $query = sprintf( 'SELECT COUNT(*) AS total FROM %sfaqdata fd ' . "WHERE fd.date_start <= '%s' AND fd.date_end >= '%s' AND fd.active = 'yes' AND fd.lang = '%s' " + // The scope constraint lives inside the EXISTS so the count matches + // queryRenderableFaqsByCategoryId(), which constrains its fcr join the same way. . 'AND EXISTS (SELECT 1 FROM %sfaqcategoryrelations fcr ' - . 'WHERE fcr.record_id = fd.id AND fcr.record_lang = fd.lang AND fcr.category_id = %d) %s', + . 'WHERE fcr.record_id = fd.id AND fcr.record_lang = fd.lang AND fcr.category_id = %d%s) %s', Database::getTablePrefix(), $now, $now, $this->configuration->getDb()->escape($this->configuration->getLanguage()->getLanguage()), Database::getTablePrefix(), $categoryId, - $queryHelper->queryPermissionExistsAny($groupSupport), + $queryHelper->queryReadScopeCategoryRelation('fcr'), + $queryHelper->queryPermissionExistsAny($groupSupport) . $queryHelper->queryReadScope(), ); $result = $this->configuration->getDb()->query($query); @@ -962,7 +982,7 @@ public function queryRenderableFaqsByIds( bool $groupSupport, ): mixed { $now = date(format: 'YmdHis'); - $queryHelper = new QueryHelper($userId, $groups); + $queryHelper = new QueryHelper($userId, $groups, $this->readScope); $query = sprintf( " SELECT @@ -1006,7 +1026,7 @@ public function queryRenderableFaqsByIds( $now, $records, $this->configuration->getDb()->escape($this->configuration->getLanguage()->getLanguage()), - $queryHelper->queryPermissionExistsAny($groupSupport), + $queryHelper->queryPermissionExistsAny($groupSupport) . $queryHelper->queryReadScope(), $orderExpression, $sortDirection, ); diff --git a/phpmyfaq/src/phpMyFAQ/Faq/QueryHelper.php b/phpmyfaq/src/phpMyFAQ/Faq/QueryHelper.php index 515e2c50e9..7367284604 100644 --- a/phpmyfaq/src/phpMyFAQ/Faq/QueryHelper.php +++ b/phpmyfaq/src/phpMyFAQ/Faq/QueryHelper.php @@ -19,6 +19,7 @@ namespace phpMyFAQ\Faq; +use InvalidArgumentException; use phpMyFAQ\Category; use phpMyFAQ\Configuration; use phpMyFAQ\Database; @@ -38,14 +39,58 @@ private Configuration $configuration; + private ReadScope $readScope; + /** * @param int[] $groups */ public function __construct( private int $user, private array $groups, + ?ReadScope $readScope = null, ) { + // A known user must bring the scope resolved where the requester became known — + // silently defaulting them to an unrestricted read gate would disable the FAQS_VIEW + // enforcement for exactly the accounts it exists for. + if ($readScope === null && $user > 0) { + throw new InvalidArgumentException(sprintf( + 'QueryHelper requires a resolved ReadScope for user %d; only an anonymous or' + . ' unresolved requester may omit it.', + $user, + )); + } + $this->configuration = Configuration::getConfigurationInstance(); + // No scope handed in means the caller never established a requester, in which case + // $user is still -1 and queryPermission() already treats them as anonymous — the most + // restrictive record ACL there is. Resolving a scope here instead would make query + // building issue its own permission lookups. + $this->readScope = $readScope ?? ReadScope::unrestricted(); + } + + /** + * Restricts the result to the FAQs the requester may read at all. + * + * Appended next to queryPermission() / queryPermissionExistsAny(), which answer a different + * question: those check the record's own user and group ACL, this one checks whether the + * requester holds the FAQ read right and in which categories and languages. + * + * @param string $faqAlias the alias the query gives the faqdata table + * @param string|null $categoryRelationAlias the alias of a faqcategoryrelations join in the + * outer query, when it has one + */ + public function queryReadScope(string $faqAlias = 'fd', ?string $categoryRelationAlias = null): string + { + return $this->readScope->toSqlFragment($faqAlias, $categoryRelationAlias); + } + + /** + * Constrains a faqcategoryrelations alias to the scoped categories. For sub-queries that + * aggregate a category id on their own, where queryReadScope() cannot reach. + */ + public function queryReadScopeCategoryRelation(string $categoryRelationAlias): string + { + return $this->readScope->toCategoryRelationConstraint($categoryRelationAlias); } public function queryPermission(bool $hasGroupSupport = false): string @@ -214,6 +259,8 @@ public function getQuery( break; } + $query .= $this->queryReadScope('fd', 'fcr'); + match ($queryType) { self::FAQ_QUERY_TYPE_EXPORT_PDF, self::FAQ_QUERY_TYPE_EXPORT_JSON, diff --git a/phpmyfaq/src/phpMyFAQ/Faq/ReadScope.php b/phpmyfaq/src/phpMyFAQ/Faq/ReadScope.php new file mode 100644 index 0000000000..1f48063844 --- /dev/null +++ b/phpmyfaq/src/phpMyFAQ/Faq/ReadScope.php @@ -0,0 +1,218 @@ + + * @copyright 2026 phpMyFAQ Team + * @license https://www.mozilla.org/MPL/2.0/ Mozilla Public License Version 2.0 + * @link https://www.phpmyfaq.de + * @since 2026-08-12 + */ + +declare(strict_types=1); + +namespace phpMyFAQ\Faq; + +use phpMyFAQ\Configuration; +use phpMyFAQ\Database; +use phpMyFAQ\Enums\PermissionType; +use phpMyFAQ\Permission\PermissionInterface; +use phpMyFAQ\User\CurrentUser; + +/** + * Answers which FAQs a requester may read at all, as a reusable SQL fragment. + * + * This is the single place where PermissionType::FAQS_VIEW is evaluated, so read access stays + * one decision instead of a check repeated per query. It is deliberately orthogonal to the + * faqdata_user / faqdata_group grants that QueryHelper::queryPermission() renders: those answer + * "does this record's ACL admit me", this one answers "may I read FAQs, and which ones". + * + * Anonymous requesters are never gated. They hold no faqright rows at all, so enforcing the + * right against them would empty the public FAQ on every installation; guest access stays + * governed by security.enableLoginOnly and the per-record "-1 = everyone" grants. + */ +final readonly class ReadScope +{ + /** + * @param array|null $categoryIds null means every category + * @param array|null $languages null means every language + */ + private function __construct( + private bool $enforced, + private bool $granted, + private ?array $categoryIds, + private ?array $languages, + ) { + } + + public static function unrestricted(): self + { + return new self(false, true, null, null); + } + + /** + * The single evaluation point for the FAQ read right. + */ + public static function forUser(PermissionInterface $permission, int $userId): self + { + $right = PermissionType::FAQS_VIEW->value; + + if (!$permission->hasPermission($userId, $right)) { + return new self(true, false, [], []); + } + + return new self( + true, + true, + $permission->getAllowedCategoriesForRight($userId, $right), + $permission->getAllowedLanguagesForRight($userId, $right), + ); + } + + /** + * Resolve the scope for a user id. + * + * Call this once, where the requester becomes known — the setUser() of the class about to + * run the query — and hand the result to QueryHelper. Resolving it lazily while a query is + * being built would make every FAQ query issue its own user and permission lookups. + * + * @throws \Exception + */ + public static function forUserId(Configuration $configuration, int $userId): self + { + // -1 is the anonymous account, 0 means no user was ever resolved. + if ($userId <= 0) { + return self::unrestricted(); + } + + $currentUser = new CurrentUser($configuration); + $currentUser->getUserById($userId, allowBlockedUsers: true); + + return self::forUser($currentUser->perm, $userId); + } + + public function isUnrestricted(): bool + { + return !$this->enforced || $this->granted && $this->categoryIds === null && $this->languages === null; + } + + public function allowsCategory(int $categoryId): bool + { + if (!$this->enforced) { + return true; + } + + if (!$this->granted) { + return false; + } + + return $this->categoryIds === null || in_array($categoryId, $this->categoryIds, strict: true); + } + + public function allowsLanguage(string $language): bool + { + if (!$this->enforced) { + return true; + } + + if (!$this->granted) { + return false; + } + + return $this->languages === null || in_array($language, $this->languages, strict: true); + } + + /** + * Renders the scope as a WHERE fragment to append to an FAQ query. + * + * Returns an empty string whenever the requester is unrestricted, which is the case for + * anonymous visitors, superadmins and every user holding an unscoped grant — so the common + * query stays exactly as it was. + * + * @param string $faqAlias the alias the query gives the faqdata table + * @param string|null $categoryRelationAlias the alias of a faqcategoryrelations join in the + * outer query, when it has one + */ + public function toSqlFragment(string $faqAlias = 'fd', ?string $categoryRelationAlias = null): string + { + if (!$this->enforced) { + return ''; + } + + // No right at all, or a scope that resolves to nothing: match no rows rather than + // silently falling through to an unfiltered result. + if (!$this->granted || $this->categoryIds === [] || $this->languages === []) { + return ' AND 1 = 0'; + } + + $fragment = ''; + + if ($this->languages !== null) { + $fragment .= sprintf(' AND %s.lang IN (%s)', $faqAlias, $this->quotedLanguageList()); + } + + if ($this->categoryIds === null) { + return $fragment; + } + + // A query that joins faqcategoryrelations itself must constrain that alias directly: + // the EXISTS predicate admits the FAQ as soon as one permitted category exists, so the + // join could still project a denied category id next to it. + if ($categoryRelationAlias !== null) { + return $fragment . $this->toCategoryRelationConstraint($categoryRelationAlias); + } + + // An EXISTS sub-query rather than a join condition: not every FAQ query carries a + // faqcategoryrelations alias in its outer query, and a join would fan one FAQ out + // into several rows and break LIMIT and COUNT. + return sprintf( + '%s AND EXISTS (SELECT 1 FROM %sfaqcategoryrelations pmfrs' + . ' WHERE pmfrs.record_id = %s.id AND pmfrs.record_lang = %s.lang' + . ' AND pmfrs.category_id IN (%s))', + $fragment, + Database::getTablePrefix(), + $faqAlias, + $faqAlias, + $this->categoryIdList(), + ); + } + + /** + * Constrains a faqcategoryrelations alias to the scoped categories, for queries that + * project or aggregate a category id themselves — an outer join as well as a sub-query + * computing something like MIN(category_id) must not surface a denied category. + */ + public function toCategoryRelationConstraint(string $categoryRelationAlias): string + { + if (!$this->enforced || $this->categoryIds === null) { + return ''; + } + + if (!$this->granted || $this->categoryIds === []) { + return ' AND 1 = 0'; + } + + return sprintf(' AND %s.category_id IN (%s)', $categoryRelationAlias, $this->categoryIdList()); + } + + private function categoryIdList(): string + { + return implode(', ', array_map(intval(...), $this->categoryIds ?? [])); + } + + private function quotedLanguageList(): string + { + $database = Configuration::getConfigurationInstance()->getDb(); + + return implode(', ', array_map(static fn(string $language): string => sprintf( + "'%s'", + $database->escape($language), + ), $this->languages ?? [])); + } +} diff --git a/phpmyfaq/src/phpMyFAQ/Faq/Statistics.php b/phpmyfaq/src/phpMyFAQ/Faq/Statistics.php index 460efa03f5..25dfa0ed90 100644 --- a/phpmyfaq/src/phpMyFAQ/Faq/Statistics.php +++ b/phpmyfaq/src/phpMyFAQ/Faq/Statistics.php @@ -42,6 +42,9 @@ class Statistics /** Flag for Group support. */ private bool $groupSupport = false; + /** Resolved by setUser(); null leaves the read gate off, as for anonymous requesters. */ + private ?ReadScope $readScope = null; + /** Plural form support. */ private readonly Plurals $plurals; @@ -64,13 +67,15 @@ public function __construct( public function totalFaqs(?string $language = null): int { $now = date(format: 'YmdHis'); + $queryHelper = new QueryHelper($this->user, $this->groups, $this->readScope); $query = sprintf( - "SELECT id FROM %sfaqdata WHERE active = 'yes' %s AND date_start <= '%s' AND date_end >= '%s'", + "SELECT fd.id FROM %sfaqdata fd WHERE fd.active = 'yes' %s AND fd.date_start <= '%s' AND fd.date_end >= '%s'%s", Database::getTablePrefix(), - null === $language ? '' : "AND lang = '" . $this->configuration->getDb()->escape($language) . "'", + null === $language ? '' : "AND fd.lang = '" . $this->configuration->getDb()->escape($language) . "'", $now, $now, + $queryHelper->queryReadScope(), ); $num = $this->configuration->getDb()->numRows($this->configuration->getDb()->query($query)); @@ -431,10 +436,14 @@ private function buildStatisticsQuery( int $categoryId = 0, ): string { $now = date(format: 'YmdHis'); - $queryHelper = new QueryHelper($this->user, $this->groups); + $queryHelper = new QueryHelper($this->user, $this->groups, $this->readScope); $prefix = Database::getTablePrefix(); - $categoryFilter = $categoryId !== 0 ? sprintf(' AND fcr.category_id = %d', $categoryId) : ''; + // The scope constraint keeps the deterministic category out of the requester's + // denied categories — the outer WHERE only decides whether the FAQ shows at all. + $categoryFilter = + ($categoryId !== 0 ? sprintf(' AND fcr.category_id = %d', $categoryId) : '') + . $queryHelper->queryReadScopeCategoryRelation('fcr'); $query = sprintf( 'SELECT %s, (SELECT MIN(fcr.category_id) FROM %sfaqcategoryrelations fcr ' @@ -465,12 +474,21 @@ private function buildStatisticsQuery( $query .= sprintf(" AND fd.lang = '%s'", $this->configuration->getDb()->escape($language)); } - return $query . ' ' . $queryHelper->queryPermissionExistsAll($this->groupSupport) . ' ORDER BY ' . $orderBy; + return ( + $query + . ' ' + . $queryHelper->queryPermissionExistsAll($this->groupSupport) + . $queryHelper->queryReadScope() + . ' ORDER BY ' + . $orderBy + ); } public function setUser(int $userId = -1): Statistics { $this->user = $userId; + $this->readScope = ReadScope::forUserId($this->configuration, $userId); + return $this; } diff --git a/phpmyfaq/src/phpMyFAQ/Helper/LanguageHelper.php b/phpmyfaq/src/phpMyFAQ/Helper/LanguageHelper.php index 6b1e823b7f..1f3a1d6bf4 100644 --- a/phpmyfaq/src/phpMyFAQ/Helper/LanguageHelper.php +++ b/phpmyfaq/src/phpMyFAQ/Helper/LanguageHelper.php @@ -124,7 +124,7 @@ public static function renderLanguageOptions( ): string { $output = ''; foreach (LanguageHelper::getAvailableLanguages() as $key => $value) { - $normalizedKey = strtolower((string) $key); + $normalizedKey = strtolower($key); if ($onlyThisLang && $normalizedKey !== $lang) { continue; } diff --git a/phpmyfaq/src/phpMyFAQ/Permission/BasicPermissionRepository.php b/phpmyfaq/src/phpMyFAQ/Permission/BasicPermissionRepository.php index 2bbc0a6a02..8b627e36f6 100644 --- a/phpmyfaq/src/phpMyFAQ/Permission/BasicPermissionRepository.php +++ b/phpmyfaq/src/phpMyFAQ/Permission/BasicPermissionRepository.php @@ -200,6 +200,39 @@ public function getUserRights(int $userId): array return $result; } + /** + * Returns the IDs of all users that hold the given right directly. + * Group rights are not taken into account. + * + * @param int $rightId Right ID + * @return array + */ + public function getUserIdsWithRight(int $rightId): array + { + if ($rightId <= 0) { + return []; + } + + $select = sprintf( + 'SELECT user_id FROM %sfaquser_right WHERE right_id = %d ORDER BY user_id ASC', + Database::getTablePrefix(), + $rightId, + ); + + $res = $this->configuration->getDb()->query($select); + $result = []; + while (true) { + $row = $this->configuration->getDb()->fetchArray($res); + if ($row === false || $row === null || $row === []) { + break; + } + + $result[] = (int) $row['user_id']; + } + + return $result; + } + /** * Adds a new right into the database. Returns the ID of the new right. * diff --git a/phpmyfaq/src/phpMyFAQ/Permission/MediumPermissionRepository.php b/phpmyfaq/src/phpMyFAQ/Permission/MediumPermissionRepository.php index 915a1355f5..0898a649e0 100644 --- a/phpmyfaq/src/phpMyFAQ/Permission/MediumPermissionRepository.php +++ b/phpmyfaq/src/phpMyFAQ/Permission/MediumPermissionRepository.php @@ -29,6 +29,38 @@ public function __construct( ) { } + /** + * Returns the IDs of all groups that hold the given right. + * + * @param int $rightId Right ID + * @return array + */ + public function getGroupIdsWithRight(int $rightId): array + { + if ($rightId <= 0) { + return []; + } + + $select = sprintf( + 'SELECT group_id FROM %sfaqgroup_right WHERE right_id = %d ORDER BY group_id ASC', + Database::getTablePrefix(), + $rightId, + ); + + $res = $this->configuration->getDb()->query($select); + $result = []; + while (true) { + $row = $this->configuration->getDb()->fetchArray($res); + if ($row === false || $row === null || $row === []) { + break; + } + + $result[] = (int) $row['group_id']; + } + + return $result; + } + /** * Returns an array that contains the right-IDs of all * group-rights the group $groupId owns. diff --git a/phpmyfaq/src/phpMyFAQ/Search/SearchResultSet.php b/phpmyfaq/src/phpMyFAQ/Search/SearchResultSet.php index 0e0afca90d..cdc7411be1 100644 --- a/phpmyfaq/src/phpMyFAQ/Search/SearchResultSet.php +++ b/phpmyfaq/src/phpMyFAQ/Search/SearchResultSet.php @@ -20,7 +20,9 @@ namespace phpMyFAQ\Search; use phpMyFAQ\Configuration; +use phpMyFAQ\Database; use phpMyFAQ\Faq\Permission; +use phpMyFAQ\Faq\ReadScope; use phpMyFAQ\User\CurrentUser; use stdClass; @@ -82,6 +84,14 @@ public function reviewResultSet(array $resultSet): void $duplicateResults = []; $currentGroupIds = [-1]; + // Not every caller routes its hits through Faq::getFaqResult() first, so the read scope + // is applied here too — a hit a user may not open must not leak its question text. + $readScope = $this->resolveReadScope(); + + $storedRowsByFaq = $readScope->isUnrestricted() + ? [] + : $this->loadStoredRowsForIncompleteHits($this->rawResultSet); + if ( 'basic' !== $this->configuration->get(item: 'security.permLevel') && isset($this->currentUser->perm) // @mago-expect lint:no-isset - typed property may be uninitialized @@ -95,6 +105,10 @@ public function reviewResultSet(array $resultSet): void foreach ($this->rawResultSet as $result) { $permission = false; + if (!$this->isWithinReadScope($readScope, $result, $storedRowsByFaq)) { + continue; + } + // check permissions for groups if ('medium' === $this->configuration->get(item: 'security.permLevel')) { $groupPermissions = $this->faqPermission->get(Permission::GROUP, (int) $result->id); @@ -136,6 +150,135 @@ public function reviewResultSet(array $resultSet): void $this->setNumberOfResults($this->reviewedResultSet); } + /** + * The single place the requester's read scope is resolved, overridable so tests can review + * a result set under a chosen scope without a database-backed permission lookup. + */ + protected function resolveReadScope(): ReadScope + { + return ReadScope::forUserId($this->configuration, $this->currentUser->getUserId()); + } + + /** + * Search hits vary by backend, so a restricted scope must not take a hit at its word: a hit + * that does not carry a usable language or category is authorized against the stored FAQ + * data instead of being waved through — an absent field would otherwise become a way to + * leak the question text of an FAQ the requester may not open. + * + * @param array> $storedRowsByFaq + */ + private function isWithinReadScope(ReadScope $readScope, stdClass $result, array $storedRowsByFaq): bool + { + if ($readScope->isUnrestricted()) { + return true; + } + + $language = $this->hitLanguage($result); + $categoryId = $this->hitCategoryId($result); + + if ($language === null || $categoryId === null) { + return $this->isStoredFaqWithinReadScope( + $readScope, + $storedRowsByFaq[(int) ($result->id ?? 0)] ?? [], + $language, + $categoryId, + ); + } + + return $readScope->allowsLanguage($language) && $readScope->allowsCategory($categoryId); + } + + /** + * Loads the stored language and category rows of every hit that will need the storage + * fallback, in one query — resolving them per hit would issue one query for each + * incomplete hit in the raw result set. + * + * @param stdClass[] $results + * @return array> + */ + private function loadStoredRowsForIncompleteHits(array $results): array + { + $faqIds = []; + + foreach ($results as $result) { + if ($this->hitLanguage($result) !== null && $this->hitCategoryId($result) !== null) { + continue; + } + + $faqId = (int) ($result->id ?? 0); + if ($faqId > 0) { + $faqIds[$faqId] = true; + } + } + + if ($faqIds === []) { + return []; + } + + $database = $this->configuration->getDb(); + $query = sprintf( + 'SELECT fd.id AS id, fd.lang AS lang, fcr.category_id AS category_id FROM %sfaqdata fd ' + . 'LEFT JOIN %sfaqcategoryrelations fcr ON fcr.record_id = fd.id AND fcr.record_lang = fd.lang ' + . 'WHERE fd.id IN (%s)', + Database::getTablePrefix(), + Database::getTablePrefix(), + implode(', ', array_fill(start_index: 0, count: count($faqIds), value: '?')), + ); + + $storedRowsByFaq = []; + foreach ($database->fetchAll($database->queryPrepared($query, array_keys($faqIds))) ?? [] as $row) { + $storedRowsByFaq[(int) $row->id][] = $row; + } + + return $storedRowsByFaq; + } + + /** + * Authorizes an incomplete hit from its stored rows: the hit passes when at least one + * stored language and category combination lies within the scope, constrained to the + * language and category the hit itself carried — a hit claiming a denied category must + * not slip through on another membership of the same FAQ. A hit whose record resolved + * to no matching rows fails closed. + * + * @param list $storedRows + */ + private function isStoredFaqWithinReadScope( + ReadScope $readScope, + array $storedRows, + ?string $language, + ?int $categoryId, + ): bool { + foreach ($storedRows as $row) { + if ($language !== null && (string) $row->lang !== $language) { + continue; + } + + if ($categoryId !== null && (int) $row->category_id !== $categoryId) { + continue; + } + + if ( + $readScope->allowsLanguage((string) $row->lang) && $readScope->allowsCategory((int) $row->category_id) + ) { + return true; + } + } + + return false; + } + + private function hitLanguage(stdClass $result): ?string + { + return property_exists($result, 'lang') && (string) $result->lang !== '' ? (string) $result->lang : null; + } + + private function hitCategoryId(stdClass $result): ?int + { + return property_exists($result, 'category_id') && (int) $result->category_id > 0 + ? (int) $result->category_id + : null; + } + /** * Sets the "raw" search results. * diff --git a/phpmyfaq/src/phpMyFAQ/Setup/Installation/DefaultDataSeeder.php b/phpmyfaq/src/phpMyFAQ/Setup/Installation/DefaultDataSeeder.php index 2cdbcc11d4..59b9e7a61f 100644 --- a/phpmyfaq/src/phpMyFAQ/Setup/Installation/DefaultDataSeeder.php +++ b/phpmyfaq/src/phpMyFAQ/Setup/Installation/DefaultDataSeeder.php @@ -375,12 +375,12 @@ private static function buildDefaultRights(): array ['name' => PermissionType::GROUP_EDIT->value, 'description' => 'Right to edit group accounts'], ['name' => PermissionType::GROUP_DELETE->value, 'description' => 'Right to delete group accounts'], ['name' => PermissionType::FAQ_APPROVE->value, 'description' => 'Right to approve FAQs'], + ['name' => PermissionType::FAQ_PUBLISH->value, 'description' => 'Right to publish FAQs'], ['name' => PermissionType::ATTACHMENT_ADD->value, 'description' => 'Right to add attachments'], ['name' => PermissionType::ATTACHMENT_EDIT->value, 'description' => 'Right to edit attachments'], ['name' => PermissionType::ATTACHMENT_DELETE->value, 'description' => 'Right to delete attachments'], ['name' => PermissionType::ATTACHMENT_DOWNLOAD->value, 'description' => 'Right to download attachments'], ['name' => PermissionType::REPORTS->value, 'description' => 'Right to generate reports'], - ['name' => PermissionType::FAQ_ADD->value, 'description' => 'Right to add FAQs in frontend'], ['name' => PermissionType::QUESTION_ADD->value, 'description' => 'Right to add questions in frontend'], ['name' => PermissionType::COMMENT_ADD->value, 'description' => 'Right to add comments in frontend'], ['name' => PermissionType::INSTANCE_EDIT->value, 'description' => 'Right to edit multi-site instances'], diff --git a/phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/OperationRecorder.php b/phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/OperationRecorder.php index e55e598191..5150edf616 100644 --- a/phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/OperationRecorder.php +++ b/phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/OperationRecorder.php @@ -122,6 +122,33 @@ public function grantPermission(string $name, string $description, int $userId = return $this; } + /** + * Records a permission backfill operation. + * + * Unlike grantPermission(), this succeeds on an already existing right and reaches every + * matching user and group, so an existing-but-unenforced right can start being enforced + * without revoking access from anybody. + */ + public function backfillPermission( + string $name, + string $description, + bool $grantToAllUsers = false, + bool $grantToAllGroups = false, + ?string $mirrorFrom = null, + bool $mirrorRestrictions = false, + ): self { + $this->operations[] = new PermissionBackfillOperation( + $this->configuration, + $name, + $description, + $grantToAllUsers, + $grantToAllGroups, + $mirrorFrom, + $mirrorRestrictions, + ); + return $this; + } + /** * Records a user creation operation. */ diff --git a/phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/PermissionBackfillOperation.php b/phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/PermissionBackfillOperation.php new file mode 100644 index 0000000000..ad55088b9f --- /dev/null +++ b/phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/PermissionBackfillOperation.php @@ -0,0 +1,244 @@ + + * @copyright 2026 phpMyFAQ Team + * @license https://www.mozilla.org/MPL/2.0/ Mozilla Public License Version 2.0 + * @link https://www.phpmyfaq.de + * @since 2026-08-12 + */ + +declare(strict_types=1); + +namespace phpMyFAQ\Setup\Migration\Operations; + +use phpMyFAQ\Configuration; +use phpMyFAQ\Permission\BasicPermissionRepository; +use phpMyFAQ\Permission\GroupCategoryPermissionRepository; +use phpMyFAQ\Permission\LanguagePermissionRepository; +use phpMyFAQ\Permission\MediumPermission; +use phpMyFAQ\Permission\MediumPermissionRepository; +use phpMyFAQ\Permission\PermissionInterface; +use phpMyFAQ\User; +use Throwable; + +/** + * Grants an existing or newly created right to many users and groups at once. + * + * PermissionGrantOperation cannot do this: it grants to exactly one user, and it reports + * failure when the right already exists, because BasicPermission::addRight() returns 0 for a + * duplicate name. A backfill has to succeed on a right that is already present and has to reach + * every account, so that turning an existing-but-unenforced right into an enforced one does not + * revoke access from anybody. + * + * With $mirrorFrom set, the right is granted to exactly those users and groups that hold the + * source right, optionally carrying its category and language restrictions across, so a scoped + * grant stays scoped instead of silently widening. + */ +readonly class PermissionBackfillOperation implements OperationInterface +{ + public function __construct( + private Configuration $configuration, + private string $permissionName, + private string $permissionDescription, + private bool $grantToAllUsers = false, + private bool $grantToAllGroups = false, + private ?string $mirrorFrom = null, + private bool $mirrorRestrictions = false, + ) { + } + + public function getType(): string + { + return 'permission_backfill'; + } + + public function getDescription(): string + { + if ($this->mirrorFrom !== null) { + return sprintf('Backfill permission: %s for every holder of %s', $this->permissionName, $this->mirrorFrom); + } + + return sprintf('Backfill permission: %s for all users and groups', $this->permissionName); + } + + public function getPermissionName(): string + { + return $this->permissionName; + } + + public function getPermissionDescription(): string + { + return $this->permissionDescription; + } + + public function getMirrorFrom(): ?string + { + return $this->mirrorFrom; + } + + public function execute(): bool + { + try { + $user = new User($this->configuration); + $permission = $user->perm; + + $rightId = $this->resolveOrCreateRight($permission->getRightId($this->permissionName), $permission); + if ($rightId <= 0) { + return false; + } + + $sourceRightId = $this->mirrorFrom === null ? 0 : $permission->getRightId($this->mirrorFrom); + + return ( + $this->backfillUsers($user, $rightId, $sourceRightId) && $this->backfillGroups($rightId, $sourceRightId) + ); + } catch (Throwable) { + return false; + } + } + + public function toArray(): array + { + return [ + 'type' => $this->getType(), + 'description' => $this->getDescription(), + 'permissionName' => $this->permissionName, + 'permissionDescription' => $this->permissionDescription, + 'grantToAllUsers' => $this->grantToAllUsers, + 'grantToAllGroups' => $this->grantToAllGroups, + 'mirrorFrom' => $this->mirrorFrom, + 'mirrorRestrictions' => $this->mirrorRestrictions, + ]; + } + + /** + * An already existing right is a success, not a failure — that is the whole point of a + * backfill, which typically runs against a right that every installation already carries. + */ + private function resolveOrCreateRight(int $rightId, PermissionInterface $permission): int + { + if ($rightId > 0) { + return $rightId; + } + + return $permission->addRight([ + 'name' => $this->permissionName, + 'description' => $this->permissionDescription, + ]); + } + + /** + * Returns false as soon as one grant cannot be persisted: a backfill that silently skips + * an account would enforce a right that account never received, so the whole operation + * reports failure and stays retryable instead. + */ + private function backfillUsers(User $user, int $rightId, int $sourceRightId): bool + { + $userIds = $this->resolveUserIds($user, $sourceRightId); + + foreach ($userIds as $userId) { + // grantUserRight() is a plain INSERT, so a re-run after a partially failed update + // would hit a primary key violation without this guard. + if (!$user->perm->checkUserRight($userId, $rightId) && !$user->perm->grantUserRight($userId, $rightId)) { + return false; + } + + if ($this->mirrorRestrictions && $sourceRightId > 0) { + $this->mirrorUserLanguageRestrictions($userId, $rightId, $sourceRightId); + } + } + + return true; + } + + /** + * Same failure policy as backfillUsers(): the first grant that does not persist fails + * the operation. + */ + private function backfillGroups(int $rightId, int $sourceRightId): bool + { + if (!$this->grantToAllGroups && $this->mirrorFrom === null) { + return true; + } + + // The group tables exist in both permission levels, and an installation can be switched + // from basic to medium later on, so back-fill groups regardless of security.permLevel. + $groupRepository = new MediumPermissionRepository($this->configuration); + $groupIds = $this->mirrorFrom === null + ? $groupRepository->getAllGroups() + : $groupRepository->getGroupIdsWithRight($sourceRightId); + + if ($groupIds === []) { + return true; + } + + $permission = new MediumPermission($this->configuration); + + foreach ($groupIds as $groupId) { + if ( + !in_array($rightId, $groupRepository->getGroupRights($groupId), strict: true) + && !$permission->grantGroupRight($groupId, $rightId) + ) { + return false; + } + + if ($this->mirrorRestrictions && $sourceRightId > 0) { + $this->mirrorGroupRestrictions($groupId, $rightId, $sourceRightId); + } + } + + return true; + } + + /** + * @return array + */ + private function resolveUserIds(User $user, int $sourceRightId): array + { + if ($this->mirrorFrom !== null) { + return new BasicPermissionRepository($this->configuration)->getUserIdsWithRight($sourceRightId); + } + + if (!$this->grantToAllUsers) { + return []; + } + + return $user->getAllUsers(); + } + + private function mirrorUserLanguageRestrictions(int $userId, int $rightId, int $sourceRightId): void + { + $languageRepository = new LanguagePermissionRepository($this->configuration); + $languages = $languageRepository->getUserLanguageRestrictions($userId, $sourceRightId); + + // An empty restriction set means "unrestricted", so there is nothing to carry over. + if ($languages === []) { + return; + } + + $languageRepository->setUserLanguageRestrictions($userId, $rightId, $languages); + } + + private function mirrorGroupRestrictions(int $groupId, int $rightId, int $sourceRightId): void + { + $categoryRepository = new GroupCategoryPermissionRepository($this->configuration); + $categoryIds = $categoryRepository->getCategoryRestrictions($groupId, $sourceRightId); + if ($categoryIds !== []) { + $categoryRepository->setCategoryRestrictions($groupId, $rightId, $categoryIds); + } + + $languageRepository = new LanguagePermissionRepository($this->configuration); + $languages = $languageRepository->getLanguageRestrictions($groupId, $sourceRightId); + if ($languages !== []) { + $languageRepository->setLanguageRestrictions($groupId, $rightId, $languages); + } + } +} diff --git a/phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha2.php b/phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha2.php index 0dc44b3111..9b4bc05f04 100644 --- a/phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha2.php +++ b/phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha2.php @@ -19,6 +19,7 @@ namespace phpMyFAQ\Setup\Migration\Versions; +use phpMyFAQ\Enums\PermissionType; use phpMyFAQ\Setup\Migration\AbstractMigration; use phpMyFAQ\Setup\Migration\Operations\OperationRecorder; @@ -36,7 +37,10 @@ public function getDependencies(): array public function getDescription(): string { - return 'Add faquser_right_language and faqgroup_right_language tables for granular language-based permissions'; + return ( + 'Add faquser_right_language and faqgroup_right_language tables for granular ' + . 'language-based permissions, and separate the FAQ read and publish rights' + ); } public function up(OperationRecorder $recorder): void @@ -154,5 +158,36 @@ public function up(OperationRecorder $recorder): void 'Create faqgroup_right_language table (SQL Server)', ); } + + $this->separateReadAndPublishRights($recorder); + } + + /** + * Splits reading and publishing off from the edit/approve mix without changing anybody's + * effective permissions. + * + * view_faqs has existed since 3.0 but was never enforced, so it has to reach every existing + * user and group before the read gate goes live — otherwise an upgrade would blank the FAQ + * for everybody. faq_publish is new and mirrors approverec, carrying its category and + * language restrictions across so a scoped approver becomes an equally scoped publisher. + * + * The read backfill runs first: if the update dies between the two operations, everybody can + * still read, which is the more forgiving half-applied state. + */ + private function separateReadAndPublishRights(OperationRecorder $recorder): void + { + $recorder->backfillPermission( + PermissionType::FAQS_VIEW->value, + 'Right to view FAQs', + grantToAllUsers: true, + grantToAllGroups: true, + ); + + $recorder->backfillPermission( + PermissionType::FAQ_PUBLISH->value, + 'Right to publish FAQs', + mirrorFrom: PermissionType::FAQ_APPROVE->value, + mirrorRestrictions: true, + ); } } diff --git a/phpmyfaq/src/phpMyFAQ/Sitemap.php b/phpmyfaq/src/phpMyFAQ/Sitemap.php index 58ac62e92e..f32d75e17c 100644 --- a/phpmyfaq/src/phpMyFAQ/Sitemap.php +++ b/phpmyfaq/src/phpMyFAQ/Sitemap.php @@ -23,6 +23,7 @@ use League\CommonMark\CommonMarkConverter; use League\CommonMark\Exception\CommonMarkException; use phpMyFAQ\Database\Sqlite3; +use phpMyFAQ\Faq\ReadScope; use phpMyFAQ\Link\Util\TitleSlugifier; use stdClass; @@ -50,6 +51,11 @@ class Sitemap */ private bool $groupSupport = false; + /** + * Resolved by setUser(); null leaves the read gate off, as for anonymous requesters. + */ + private ?ReadScope $readScope = null; + /** * Constructor. */ @@ -64,6 +70,16 @@ public function __construct( public function setUser(int $userId = -1): void { $this->user = $userId; + $this->readScope = ReadScope::forUserId($this->configuration, $userId); + } + + /** + * The sitemap builds its own permission clause instead of going through QueryHelper, so the + * read scope has to be appended explicitly here. + */ + private function readScopeFragment(): string + { + return ($this->readScope ?? ReadScope::unrestricted())->toSqlFragment(); } /** @@ -123,7 +139,7 @@ public function getAllFirstLetters(): array Database::getTablePrefix(), Database::getTablePrefix(), $this->configuration->getDb()->escape($this->configuration->getLanguage()->getLanguage()), - $permPart, + $permPart . $this->readScopeFragment(), ); $result = $this->configuration->getDb()->query($query); @@ -214,7 +230,7 @@ public function getFaqsFromLetter(string $letter = 'A'): array Database::getTablePrefix(), $this->configuration->getDb()->escape($letter), $this->configuration->getDb()->escape($this->configuration->getLanguage()->getLanguage()), - $permPart, + $permPart . $this->readScopeFragment(), ); $result = $this->configuration->getDb()->query($query); diff --git a/phpmyfaq/src/phpMyFAQ/Tags.php b/phpmyfaq/src/phpMyFAQ/Tags.php index 61d7e8b106..09077afe8e 100644 --- a/phpmyfaq/src/phpMyFAQ/Tags.php +++ b/phpmyfaq/src/phpMyFAQ/Tags.php @@ -22,6 +22,7 @@ namespace phpMyFAQ; use phpMyFAQ\Entity\Tag; +use phpMyFAQ\Faq\ReadScope; /** * Class Tags @@ -41,6 +42,9 @@ class Tags private bool $bypassPermissionCheck = false; + /** Resolved by setUser(); null leaves the read gate off, as for anonymous requesters. */ + private ?ReadScope $readScope = null; + /** * Constructor. */ @@ -55,6 +59,8 @@ public function __construct( public function setUser(int $userId = -1): Tags { $this->user = $userId; + $this->readScope = ReadScope::forUserId($this->configuration, $userId); + return $this; } @@ -548,23 +554,21 @@ private function buildPermissionCheck(): string $groupSupport = $this->configuration->get(item: 'security.permLevel') !== 'basic'; $groupList = $this->normalizePermissionGroups(); - if ($groupSupport) { - if (-1 === $this->user) { - // Only group permissions apply (anonymous user) - return sprintf('AND fdg.group_id IN (%s)', $groupList); - } + // Tags alias faqdata as "d", so the read scope has to be rendered for that alias. + $readScope = ($this->readScope ?? ReadScope::unrestricted())->toSqlFragment('d'); + $recordAccess = match (true) { + // Only group permissions apply (anonymous user) + $groupSupport && -1 === $this->user => sprintf('AND fdg.group_id IN (%s)', $groupList), // Check both user and group permissions - return sprintf('AND ( fdu.user_id = %d OR fdg.group_id IN (%s) )', (int) $this->user, $groupList); - } - - // Basic permission level - only user permissions - if (-1 !== $this->user) { - return sprintf('AND ( fdu.user_id = %d OR fdu.user_id = -1 )', (int) $this->user); - } + $groupSupport => sprintf('AND ( fdu.user_id = %d OR fdg.group_id IN (%s) )', (int) $this->user, $groupList), + // Basic permission level - only user permissions + -1 !== $this->user => sprintf('AND ( fdu.user_id = %d OR fdu.user_id = -1 )', (int) $this->user), + // Anonymous user with basic permission level + default => 'AND fdu.user_id = -1', + }; - // Anonymous user with basic permission level - return 'AND fdu.user_id = -1'; + return $recordAccess . $readScope; } private function normalizePermissionGroups(): string diff --git a/phpmyfaq/src/phpMyFAQ/User.php b/phpmyfaq/src/phpMyFAQ/User.php index ed3da29a41..d2195660c3 100644 --- a/phpmyfaq/src/phpMyFAQ/User.php +++ b/phpmyfaq/src/phpMyFAQ/User.php @@ -23,6 +23,7 @@ use Exception; use phpMyFAQ\Auth\AuthDriverInterface; +use phpMyFAQ\Enums\PermissionType; use phpMyFAQ\Permission\MediumPermission; use phpMyFAQ\Permission\PermissionInterface; use phpMyFAQ\Tenant\TenantQuotaEnforcer; @@ -435,9 +436,47 @@ public function createUser(string $login, string $pass = '', string $domain = '' $this->perm->autoJoin($this->userId); } + $this->grantDefaultReadRight(); + return $this->getUserByLogin($login, false); } + /** + * Grants the FAQ read right to a freshly created account. + * + * Reading FAQs is gated on PermissionType::FAQS_VIEW for authenticated users, and new + * accounts start out with no rights at all. Without this grant a user created after the + * 4.2.0-alpha.2 upgrade would see an empty FAQ while an anonymous visitor sees everything. + * Hiding FAQs from a user therefore stays an explicit revoke instead of being an accident + * of account creation. + * + * @throws Exception when the grant cannot be persisted — reporting the account as created + * anyway would hand out a user that sees an empty FAQ + */ + private function grantDefaultReadRight(): void + { + // The anonymous account is never gated on the right, so granting it would only add a + // row that nothing reads — and it would let an unresolvable user id inherit read access + // through the anonymous fallback in hasPermission(). + if ($this->userId <= 0) { + return; + } + + $rightId = $this->perm->getRightId(PermissionType::FAQS_VIEW->value); + if ($rightId <= 0) { + return; + } + + if (!$this->perm->grantUserRight($this->userId, $rightId)) { + throw new Exception(sprintf( + '%sThe default "%s" right could not be granted to user %d.', + self::ERROR_USER_CANNOT_CREATE_USER, + PermissionType::FAQS_VIEW->value, + $this->userId, + )); + } + } + private function getTenantQuotaEnforcer(): TenantQuotaEnforcer { return $this->tenantQuotaEnforcer ??= TenantQuotaEnforcer::createFromDatabaseDriver( diff --git a/phpmyfaq/translations/language_en.php b/phpmyfaq/translations/language_en.php index f86d1d215d..d2ee7fbdd9 100644 --- a/phpmyfaq/translations/language_en.php +++ b/phpmyfaq/translations/language_en.php @@ -1869,4 +1869,8 @@ $PMF_LANG['msgImportRowNoCategoryPermission'] = 'Row %d: no "%s" permission for category %d.'; $PMF_LANG['msgImportRowNoLanguagePermission'] = 'Row %d: no "%s" permission for language "%s".'; +// added v4.2.0-alpha.2 - 2026-08-12 by Thorsten +$PMF_LANG['permission::faq_publish'] = 'Publish FAQs'; +$PMF_LANG['msgNoPublishPermission'] = 'You are not allowed to change the publication state of this FAQ.'; + return $PMF_LANG; diff --git a/tests/phpMyFAQ/Command/SeedTestDataCommandTest.php b/tests/phpMyFAQ/Command/SeedTestDataCommandTest.php index 3d9f791ca7..15b9910731 100644 --- a/tests/phpMyFAQ/Command/SeedTestDataCommandTest.php +++ b/tests/phpMyFAQ/Command/SeedTestDataCommandTest.php @@ -49,6 +49,7 @@ #[UsesClass(\phpMyFAQ\Faq::class)] #[UsesClass(\phpMyFAQ\Faq\FaqRepository::class)] #[UsesClass(\phpMyFAQ\Faq\Permission::class)] +#[UsesClass(\phpMyFAQ\Faq\ReadScope::class)] #[UsesClass(\phpMyFAQ\Filter::class)] #[UsesClass(\phpMyFAQ\Glossary::class)] #[UsesClass(\phpMyFAQ\Glossary\GlossaryRepository::class)] diff --git a/tests/phpMyFAQ/Controller/Administration/Api/FaqControllerTest.php b/tests/phpMyFAQ/Controller/Administration/Api/FaqControllerTest.php index 4f4f11525c..5445c96540 100644 --- a/tests/phpMyFAQ/Controller/Administration/Api/FaqControllerTest.php +++ b/tests/phpMyFAQ/Controller/Administration/Api/FaqControllerTest.php @@ -205,6 +205,7 @@ private function createAuthenticatedContainerWithAllowedCategories( PermissionType::FAQ_EDIT->value, PermissionType::FAQ_DELETE->value, PermissionType::FAQ_APPROVE->value, + PermissionType::FAQ_PUBLISH->value, PermissionType::FAQ_TRANSLATE->value, ], true, @@ -221,6 +222,7 @@ private function createAuthenticatedContainerWithAllowedCategories( PermissionType::FAQ_EDIT->value, PermissionType::FAQ_DELETE->value, PermissionType::FAQ_APPROVE->value, + PermissionType::FAQ_PUBLISH->value, PermissionType::FAQ_TRANSLATE->value, ], true, @@ -239,6 +241,7 @@ private function createAuthenticatedContainerWithAllowedCategories( PermissionType::FAQ_EDIT->value, PermissionType::FAQ_DELETE->value, PermissionType::FAQ_APPROVE->value, + PermissionType::FAQ_PUBLISH->value, PermissionType::FAQ_TRANSLATE->value, ], true, @@ -1766,7 +1769,7 @@ public function testActivateReturnsForbiddenWhenFaqIsInRestrictedCategory(): voi $controller->setContainer($this->createAuthenticatedContainer($session)); $this->expectException(ForbiddenException::class); - $this->expectExceptionMessage('User has no "FAQ_APPROVE" permission for category 666.'); + $this->expectExceptionMessage('User has no "FAQ_PUBLISH" permission for category 666.'); $controller->activate($request); } @@ -1792,7 +1795,7 @@ public function testActivateReturnsForbiddenWhenLanguageIsRestricted(): void $controller->setContainer($this->createAuthenticatedContainer($session)); $this->expectException(ForbiddenException::class); - $this->expectExceptionMessage('User has no "FAQ_APPROVE" permission for language "fr".'); + $this->expectExceptionMessage('User has no "FAQ_PUBLISH" permission for language "fr".'); $controller->activate($request); } @@ -2002,6 +2005,272 @@ public function testDeleteSucceedsForUnrestrictedUserWithOrphanedFaq(): void $this->removeCsrfCookie('pmf-csrf-token'); } + // ===================================================================== + // Read / write / publish matrix + // + // Writing and publishing are separate rights: holding edit_faq lets a user change an FAQ, + // but only faq_publish decides whether it goes live. + // ===================================================================== + + public function testCreateReturnsForbiddenForActiveFaqWithoutPublishPermission(): void + { + $session = new Session(new MockArraySessionStorage()); + $csrfToken = Token::getInstance($session)->getTokenString('pmf-csrf-token'); + $this->setCsrfCookie('pmf-csrf-token', $csrfToken); + + $request = $this->createRequestForNewFaq($csrfToken, active: 'yes'); + + $controller = $this->createController(); + $controller->setContainer($this->createContainerWithoutPublishPermission($session)); + + $this->expectException(ForbiddenException::class); + $this->expectExceptionMessage('User has no "FAQ_PUBLISH" permission.'); + + try { + $controller->create($request); + } finally { + $this->removeCsrfCookie('pmf-csrf-token'); + } + } + + public function testCreateSucceedsForInactiveFaqWithoutPublishPermission(): void + { + $session = new Session(new MockArraySessionStorage()); + $csrfToken = Token::getInstance($session)->getTokenString('pmf-csrf-token'); + $this->setCsrfCookie('pmf-csrf-token', $csrfToken); + + $request = $this->createRequestForNewFaq($csrfToken, active: 'no'); + + $faq = $this->createMock(Faq::class); + $faq->expects($this->once()) + ->method('create') + ->willReturnCallback(static fn(\phpMyFAQ\Entity\FaqEntity $faqEntity): \phpMyFAQ\Entity\FaqEntity => $faqEntity->setId(1)); + + $controller = $this->createControllerWithFaq($faq); + $controller->setContainer($this->createContainerWithoutPublishPermission($session)); + + $response = $controller->create($request); + + self::assertSame(Response::HTTP_OK, $response->getStatusCode()); + $this->removeCsrfCookie('pmf-csrf-token'); + } + + /** + * The editor omits the "active" field for a user without the publish right, and an absent + * field must not be read as "no" — that would unpublish a live FAQ on an ordinary save. + */ + public function testUpdateKeepsAPublishedFaqLiveWhenTheFieldIsOmitted(): void + { + $this->seedFaqRecord(question: 'Original FAQ'); + $this->configuration->getDb()->query("UPDATE faqdata SET active = 'yes' WHERE id = 1 AND lang = 'en'"); + + $session = new Session(new MockArraySessionStorage()); + $csrfToken = Token::getInstance($session)->getTokenString('pmf-csrf-token'); + $this->setCsrfCookie('pmf-csrf-token', $csrfToken); + + $request = $this->createRequestForFaqUpdate($csrfToken, active: null); + + $persisted = null; + $faq = $this->createMock(Faq::class); + $faq->method('hasTranslation')->willReturn(true); + $faq->method('isActive')->willReturn(true); + $faq->expects($this->once()) + ->method('update') + ->willReturnCallback(static function (\phpMyFAQ\Entity\FaqEntity $faqEntity) use (&$persisted): \phpMyFAQ\Entity\FaqEntity { + $persisted = $faqEntity; + + return $faqEntity; + }); + + $controller = $this->createControllerWithFaq($faq); + $controller->setContainer($this->createContainerWithoutPublishPermission($session)); + + $response = $controller->update($request); + + self::assertSame(Response::HTTP_OK, $response->getStatusCode()); + self::assertInstanceOf(\phpMyFAQ\Entity\FaqEntity::class, $persisted); + self::assertTrue($persisted->isActive(), 'A save without the publish right must not unpublish the FAQ.'); + $this->removeCsrfCookie('pmf-csrf-token'); + } + + public function testUpdateReturnsForbiddenWhenChangingTheStateWithoutPublishPermission(): void + { + $this->seedFaqRecord(question: 'Original FAQ'); + + $session = new Session(new MockArraySessionStorage()); + $csrfToken = Token::getInstance($session)->getTokenString('pmf-csrf-token'); + $this->setCsrfCookie('pmf-csrf-token', $csrfToken); + + $request = $this->createRequestForFaqUpdate($csrfToken, active: 'yes'); + + $faq = $this->createMock(Faq::class); + $faq->method('hasTranslation')->willReturn(true); + $faq->method('isActive')->willReturn(false); + $faq->expects($this->never())->method('update'); + + $controller = $this->createControllerWithFaq($faq); + $controller->setContainer($this->createContainerWithoutPublishPermission($session)); + + $this->expectException(ForbiddenException::class); + $this->expectExceptionMessage('User has no "FAQ_PUBLISH" permission.'); + + try { + $controller->update($request); + } finally { + $this->removeCsrfCookie('pmf-csrf-token'); + } + } + + public function testActivateReturnsForbiddenWithoutPublishPermission(): void + { + $session = new Session(new MockArraySessionStorage()); + $csrfToken = Token::getInstance($session)->getTokenString('pmf-csrf-token'); + $this->setCsrfCookie('pmf-csrf-token', $csrfToken); + + $request = new Request([], [], [], [], [], [], json_encode([ + 'csrf' => $csrfToken, + 'faqIds' => [1], + 'faqLanguage' => 'en', + 'checked' => true, + ], JSON_THROW_ON_ERROR)); + + $controller = $this->createController(); + $controller->setContainer($this->createContainerWithoutPublishPermission($session)); + + $this->expectException(ForbiddenException::class); + $this->expectExceptionMessage('User has no "FAQ_PUBLISH" permission.'); + + try { + $controller->activate($request); + } finally { + $this->removeCsrfCookie('pmf-csrf-token'); + } + } + + /** + * A user holding every FAQ right except faq_publish: everything that is not publishing has + * to keep working for them. + */ + private function createContainerWithoutPublishPermission(Session $session): ContainerInterface + { + $writeRights = [ + PermissionType::FAQ_ADD->value, + PermissionType::FAQ_EDIT->value, + PermissionType::FAQ_DELETE->value, + PermissionType::FAQ_APPROVE->value, + PermissionType::FAQ_TRANSLATE->value, + PermissionType::FAQS_VIEW->value, + ]; + + $permission = $this->createMock(PermissionInterface::class); + $permission + ->method('hasPermission') + ->willReturnCallback( + static fn(int $userId, mixed $right): bool => $userId === 42 + && in_array($right, $writeRights, true), + ); + $permission + ->method('hasPermissionForCategory') + ->willReturnCallback( + static fn(int $userId, mixed $right, int $categoryId): bool => $userId === 42 + && in_array($right, $writeRights, true), + ); + $permission + ->method('hasPermissionForLanguage') + ->willReturnCallback( + static fn(int $userId, mixed $right, string $language): bool => $userId === 42 + && in_array($right, $writeRights, true), + ); + $permission->method('getAllowedCategoriesForRight')->willReturn(null); + $permission->method('getAllowedLanguagesForRight')->willReturn(null); + + $currentUser = $this->createMock(CurrentUser::class); + $currentUser->perm = $permission; + $currentUser->method('isLoggedIn')->willReturn(true); + $currentUser->method('getUserId')->willReturn(42); + + $adminLog = $this->createStub(AdminLog::class); + + $container = $this->createStub(ContainerInterface::class); + $container + ->method('get') + ->willReturnCallback(function (string $id) use ($currentUser, $session, $adminLog) { + return match ($id) { + 'phpmyfaq.configuration' => $this->configuration, + 'phpmyfaq.user.current_user' => $currentUser, + 'session' => $session, + 'phpmyfaq.admin.admin-log' => $adminLog, + default => null, + }; + }); + + return $container; + } + + private function createRequestForNewFaq(string $csrfToken, string $active): Request + { + return new Request([], [], [], [], [], [], json_encode([ + 'data' => [ + 'pmf-csrf-token' => $csrfToken, + 'question' => 'New question', + 'categories[]' => 1, + 'lang' => 'en', + 'tags' => '', + 'active' => $active, + 'sticky' => 'no', + 'answer' => 'New answer', + 'keywords' => '', + 'author' => 'Author', + 'email' => 'author@example.com', + 'userpermission' => 'all', + 'restricted_users' => [], + 'grouppermission' => 'all', + 'restricted_groups' => [], + 'changed' => '', + 'notes' => '', + 'serpTitle' => '', + 'serpDescription' => '', + 'openQuestionId' => 0, + ], + ], JSON_THROW_ON_ERROR)); + } + + private function createRequestForFaqUpdate(string $csrfToken, ?string $active): Request + { + $data = [ + 'pmf-csrf-token' => $csrfToken, + 'faqId' => 1, + 'solutionId' => 1001, + 'revisionId' => 0, + 'question' => 'Updated FAQ', + 'categories[]' => 1, + 'lang' => 'en', + 'tags' => '', + 'sticky' => 'no', + 'answer' => 'Updated answer', + 'keywords' => 'updated', + 'author' => 'Author', + 'email' => 'author@example.com', + 'userpermission' => 'all', + 'restricted_users' => [], + 'grouppermission' => 'all', + 'restricted_groups' => [], + 'changed' => 'Updated', + 'notes' => 'Updated notes', + 'revision' => 'no', + 'serpTitle' => '', + 'serpDescription' => '', + ]; + + // A null $active omits the field entirely, which is what the editor sends for a user + // without the publish right. + if ($active !== null) { + $data['active'] = $active; + } + + return new Request([], [], [], [], [], [], json_encode(['data' => $data], JSON_THROW_ON_ERROR)); + } + private function setCsrfCookie(string $page, string $token): void { $_COOKIE['pmf-csrf-token-' . substr(md5($page), 0, 10)] = $token; diff --git a/tests/phpMyFAQ/Controller/Api/FaqControllerTest.php b/tests/phpMyFAQ/Controller/Api/FaqControllerTest.php index 5fdc99accb..f869302e45 100644 --- a/tests/phpMyFAQ/Controller/Api/FaqControllerTest.php +++ b/tests/phpMyFAQ/Controller/Api/FaqControllerTest.php @@ -111,6 +111,10 @@ private function authorizeCurrentUser(AbstractController $controller, int $userI { $permission = $this->createMock(BasicPermission::class); $permission->method('hasPermission')->willReturn(true); + $permission->method('hasPermissionForCategory')->willReturn(true); + $permission->method('hasPermissionForLanguage')->willReturn(true); + $permission->method('getAllowedCategoriesForRight')->willReturn(null); + $permission->method('getAllowedLanguagesForRight')->willReturn(null); $currentUser = $this->createMock(CurrentUser::class); $currentUser->perm = $permission; diff --git a/tests/phpMyFAQ/Faq/FaqRepositoryTest.php b/tests/phpMyFAQ/Faq/FaqRepositoryTest.php index 1dafd23bea..58b930d07d 100644 --- a/tests/phpMyFAQ/Faq/FaqRepositoryTest.php +++ b/tests/phpMyFAQ/Faq/FaqRepositoryTest.php @@ -52,6 +52,9 @@ protected function setUp(): void Language::$language = 'en'; $this->faqRepository = new FaqRepository($this->configuration); + // QueryHelper rejects a known user without a resolved scope; these tests exercise the + // record ACL, so the requester's read scope is explicitly unrestricted. + $this->faqRepository->setReadScope(ReadScope::unrestricted()); } protected function tearDown(): void diff --git a/tests/phpMyFAQ/Faq/QueryHelperTest.php b/tests/phpMyFAQ/Faq/QueryHelperTest.php index 346dce532a..a7da007f03 100644 --- a/tests/phpMyFAQ/Faq/QueryHelperTest.php +++ b/tests/phpMyFAQ/Faq/QueryHelperTest.php @@ -64,7 +64,7 @@ public function testQueryPermissionWithoutGroupSupport(): void { $user = 42; $groups = [1, 2, 3]; - $queryHelper = new QueryHelper($user, $groups); + $queryHelper = new QueryHelper($user, $groups, ReadScope::unrestricted()); $expectedQuery = 'AND ( fdu.user_id = 42 OR fdu.user_id = -1 )'; $this->assertEquals($expectedQuery, $queryHelper->queryPermission()); @@ -74,7 +74,7 @@ public function testQueryPermissionWithGroupSupport(): void { $user = 42; $groups = [1, 2, 3]; - $queryHelper = new QueryHelper($user, $groups); + $queryHelper = new QueryHelper($user, $groups, ReadScope::unrestricted()); $expectedQuery = 'AND ( fdu.user_id = 42 OR fdg.group_id IN (1, 2, 3) )'; $this->assertEquals($expectedQuery, $queryHelper->queryPermission(true)); @@ -84,7 +84,7 @@ public function testQueryPermissionWithNegativeUserAndGroupSupport(): void { $user = -1; $groups = [1, 2, 3]; - $queryHelper = new QueryHelper($user, $groups); + $queryHelper = new QueryHelper($user, $groups, ReadScope::unrestricted()); $expectedQuery = 'AND fdg.group_id IN (1, 2, 3)'; $this->assertEquals($expectedQuery, $queryHelper->queryPermission(true)); @@ -94,7 +94,7 @@ public function testQueryPermissionWithNegativeUserAndNoGroupSupport(): void { $user = -1; $groups = [1, 2, 3]; - $queryHelper = new QueryHelper($user, $groups); + $queryHelper = new QueryHelper($user, $groups, ReadScope::unrestricted()); $expectedQuery = 'AND fdu.user_id = -1'; $this->assertEquals($expectedQuery, $queryHelper->queryPermission()); @@ -104,7 +104,7 @@ public function testGetQuery(): void { $user = -1; $groups = [1, 2, 3]; - $queryHelper = new QueryHelper($user, $groups); + $queryHelper = new QueryHelper($user, $groups, ReadScope::unrestricted()); // Define the input values $queryType = 'export_pdf'; @@ -193,7 +193,7 @@ public function testQueryPermissionNormalizesGroupIds(): void public function testQueryPermissionExistsAllWithoutGroupSupport(): void { - $queryHelper = new QueryHelper(42, [1, 2, 3]); + $queryHelper = new QueryHelper(42, [1, 2, 3], ReadScope::unrestricted()); $this->assertSame( 'AND EXISTS (SELECT 1 FROM faqdata_user pfdu ' @@ -215,7 +215,7 @@ public function testQueryPermissionExistsAllForGuestWithoutGroupSupport(): void public function testQueryPermissionExistsAllWithGroupSupport(): void { - $queryHelper = new QueryHelper(42, [1, 2, 3]); + $queryHelper = new QueryHelper(42, [1, 2, 3], ReadScope::unrestricted()); $this->assertSame( 'AND EXISTS (SELECT 1 FROM faqdata_user pfdu ' @@ -241,7 +241,7 @@ public function testQueryPermissionExistsAllForGuestWithGroupSupport(): void public function testQueryPermissionExistsAnyWithoutGroupSupport(): void { - $queryHelper = new QueryHelper(42, [1, 2, 3]); + $queryHelper = new QueryHelper(42, [1, 2, 3], ReadScope::unrestricted()); $this->assertSame( 'AND EXISTS (SELECT 1 FROM faqdata_user pfdu ' @@ -263,7 +263,7 @@ public function testQueryPermissionExistsAnyForGuestWithoutGroupSupport(): void public function testQueryPermissionExistsAnyWithGroupSupportAllowsUserOrGroupMatch(): void { - $queryHelper = new QueryHelper(42, [1, 2, 3]); + $queryHelper = new QueryHelper(42, [1, 2, 3], ReadScope::unrestricted()); $this->assertSame( 'AND (EXISTS (SELECT 1 FROM faqdata_user pfdu ' @@ -291,4 +291,15 @@ public function testQueryPermissionExistsAllNormalizesGroupIds(): void $this->assertStringContainsString('pfdg.group_id IN (1, 2)', $queryHelper->queryPermissionExistsAll(true)); } + + /** + * A known user must never fall through to an unrestricted read gate by omission — the + * scope has to be resolved where the requester became known and passed in explicitly. + */ + public function testKnownUserWithoutReadScopeIsRejected(): void + { + $this->expectException(\InvalidArgumentException::class); + + new QueryHelper(42, [1, 2, 3]); + } } diff --git a/tests/phpMyFAQ/Faq/ReadScopeTest.php b/tests/phpMyFAQ/Faq/ReadScopeTest.php new file mode 100644 index 0000000000..018a8f8112 --- /dev/null +++ b/tests/phpMyFAQ/Faq/ReadScopeTest.php @@ -0,0 +1,194 @@ +isUnrestricted()); + self::assertSame('', $scope->toSqlFragment()); + self::assertTrue($scope->allowsCategory(42)); + self::assertTrue($scope->allowsLanguage('de')); + } + + /** + * The common case has to stay free: an unscoped grant must leave the FAQ queries byte for + * byte as they were, otherwise every category listing pays for a feature nobody enabled. + */ + public function testGrantWithoutRestrictionsAddsNothingToTheQuery(): void + { + $scope = ReadScope::forUser($this->permission(hasRight: true), 5); + + self::assertTrue($scope->isUnrestricted()); + self::assertSame('', $scope->toSqlFragment()); + } + + public function testMissingRightMatchesNoRows(): void + { + $scope = ReadScope::forUser($this->permission(hasRight: false), 5); + + self::assertFalse($scope->isUnrestricted()); + self::assertSame(' AND 1 = 0', $scope->toSqlFragment()); + self::assertFalse($scope->allowsCategory(1)); + self::assertFalse($scope->allowsLanguage('en')); + } + + /** + * An empty allowed list means "holds the right nowhere", which is not the same as the null + * that means "unrestricted". Rendering nothing for it would silently grant full access. + */ + public function testEmptyCategoryListMatchesNoRows(): void + { + $scope = ReadScope::forUser($this->permission(hasRight: true, categories: []), 5); + + self::assertSame(' AND 1 = 0', $scope->toSqlFragment()); + } + + public function testEmptyLanguageListMatchesNoRows(): void + { + $scope = ReadScope::forUser($this->permission(hasRight: true, languages: []), 5); + + self::assertSame(' AND 1 = 0', $scope->toSqlFragment()); + } + + public function testCategoryRestrictionRendersAnExistsSubQuery(): void + { + $scope = ReadScope::forUser($this->permission(hasRight: true, categories: [2, 3]), 5); + + $fragment = $scope->toSqlFragment(); + + self::assertStringContainsString('EXISTS (SELECT 1 FROM faqcategoryrelations pmfrs', $fragment); + self::assertStringContainsString('pmfrs.record_id = fd.id', $fragment); + self::assertStringContainsString('pmfrs.record_lang = fd.lang', $fragment); + self::assertStringContainsString('pmfrs.category_id IN (2, 3)', $fragment); + self::assertStringNotContainsString('1 = 0', $fragment); + } + + /** + * Tags alias faqdata as "d", so a hard-coded "fd" would produce invalid SQL there. + */ + public function testFragmentHonoursTheGivenTableAlias(): void + { + $scope = ReadScope::forUser($this->permission(hasRight: true, categories: [7]), 5); + + $fragment = $scope->toSqlFragment('d'); + + self::assertStringContainsString('pmfrs.record_id = d.id', $fragment); + self::assertStringNotContainsString('fd.id', $fragment); + } + + public function testCategoryAndLanguageRestrictionsAreCombined(): void + { + $scope = ReadScope::forUser( + $this->permission(hasRight: true, categories: [2], languages: ['de']), + 5, + ); + + $fragment = $scope->toSqlFragment(); + + self::assertStringContainsString("fd.lang IN ('de')", $fragment); + self::assertStringContainsString('pmfrs.category_id IN (2)', $fragment); + self::assertFalse($scope->isUnrestricted()); + self::assertTrue($scope->allowsCategory(2)); + self::assertFalse($scope->allowsCategory(3)); + self::assertTrue($scope->allowsLanguage('de')); + self::assertFalse($scope->allowsLanguage('en')); + } + + /** + * A query that joins faqcategoryrelations itself must constrain that alias instead of + * relying on EXISTS, or it could still project a denied category id for an FAQ that also + * belongs to an allowed category. + */ + public function testOuterRelationAliasReplacesTheExistsSubQuery(): void + { + $scope = ReadScope::forUser($this->permission(hasRight: true, categories: [2, 3]), 5); + + $fragment = $scope->toSqlFragment('fd', 'fcr'); + + self::assertStringContainsString('fcr.category_id IN (2, 3)', $fragment); + self::assertStringNotContainsString('EXISTS', $fragment); + } + + public function testCategoryRelationConstraintRendersNothingWithoutCategoryRestriction(): void + { + self::assertSame('', ReadScope::unrestricted()->toCategoryRelationConstraint('fcr')); + self::assertSame( + '', + ReadScope::forUser($this->permission(hasRight: true), 5)->toCategoryRelationConstraint('fcr'), + ); + } + + public function testCategoryRelationConstraintMatchesNoRowsWithoutTheRight(): void + { + $scope = ReadScope::forUser($this->permission(hasRight: false), 5); + + self::assertSame(' AND 1 = 0', $scope->toCategoryRelationConstraint('fcr')); + } + + public function testCategoryRelationConstraintHonoursTheGivenAlias(): void + { + $scope = ReadScope::forUser($this->permission(hasRight: true, categories: [7]), 5); + + self::assertSame(' AND rel.category_id IN (7)', $scope->toCategoryRelationConstraint('rel')); + } + + /** + * The read right is one decision, so it must be read once — not re-derived per query. + */ + public function testTheRightIsEvaluatedExactlyOnce(): void + { + $permission = $this->createMock(PermissionInterface::class); + $permission->expects($this->once()) + ->method('hasPermission') + ->with(5, PermissionType::FAQS_VIEW->value) + ->willReturn(true); + $permission->expects($this->once())->method('getAllowedCategoriesForRight')->willReturn(null); + $permission->expects($this->once())->method('getAllowedLanguagesForRight')->willReturn(null); + + $scope = ReadScope::forUser($permission, 5); + + // Rendering repeatedly must not go back to the permission layer. + $scope->toSqlFragment(); + $scope->toSqlFragment(); + $scope->allowsCategory(1); + } + + /** + * @param array|null $categories + * @param array|null $languages + */ + private function permission( + bool $hasRight, + ?array $categories = null, + ?array $languages = null, + ): PermissionInterface { + $permission = $this->createStub(PermissionInterface::class); + $permission->method('hasPermission')->willReturn($hasRight); + $permission->method('getAllowedCategoriesForRight')->willReturn($categories); + $permission->method('getAllowedLanguagesForRight')->willReturn($languages); + + return $permission; + } +} diff --git a/tests/phpMyFAQ/Faq/StatisticsIntegrationTest.php b/tests/phpMyFAQ/Faq/StatisticsIntegrationTest.php index deea60de04..cba284e0b7 100644 --- a/tests/phpMyFAQ/Faq/StatisticsIntegrationTest.php +++ b/tests/phpMyFAQ/Faq/StatisticsIntegrationTest.php @@ -70,6 +70,31 @@ protected function tearDown(): void parent::tearDown(); } + /** + * Creates a real account holding the FAQ read right. + * + * Reading is gated on that right for logged-in users, so a bare user id with only a + * faqdata_user row is denied — as it would be on a real installation, where the upgrade and + * User::createUser() are what hand the right out. + */ + private function createReaderAccount(int $userId): void + { + $database = $this->configuration->getDb(); + + $database->query(sprintf( + "INSERT INTO faquser (user_id, login, session_timestamp, member_since, account_status) + VALUES (%d, 'reader%d', 0, '20260101000000', 'active')", + $userId, + $userId, + )); + + $database->query(sprintf( + "INSERT INTO faquser_right (user_id, right_id) + SELECT %d, right_id FROM faqright WHERE name = 'view_faqs'", + $userId, + )); + } + /** * @param int[] $userIds * @param int[] $groupIds @@ -144,6 +169,7 @@ public function testGetLatestDataShowsUserRestrictedFaqsToTheirUser(): void { $this->seedFaq(9001, 'Public FAQ', '20260101000000', 1); $this->seedFaq(9002, 'Restricted FAQ', '20260102000000', 1, userIds: [42]); + $this->createReaderAccount(42); $latest = $this->createStatistics('basic')->setUser(42)->getLatestData(10, 'en'); @@ -165,6 +191,7 @@ public function testGetLatestDataFiltersByGroupWithGroupSupport(): void public function testGetLatestDataWithGroupSupportRequiresMatchingUser(): void { $this->seedFaq(9001, 'Owner-only FAQ', '20260101000000', 1, userIds: [42], groupIds: [10]); + $this->createReaderAccount(42); $statistics = $this->createStatistics('medium'); diff --git a/tests/phpMyFAQ/Faq/StatisticsTest.php b/tests/phpMyFAQ/Faq/StatisticsTest.php index 471119b4c7..0e77eee869 100644 --- a/tests/phpMyFAQ/Faq/StatisticsTest.php +++ b/tests/phpMyFAQ/Faq/StatisticsTest.php @@ -241,19 +241,12 @@ public function testTotalFaqsWithoutLanguage(): void $this->configurationMock->method('get')->willReturn('basic'); $statistics = new Statistics($this->configurationMock); - $expectedQuery = - "SELECT id FROM faqdata WHERE active = 'yes' AND date_start <= '" - . date(format: 'YmdHis') - . "' AND date_end >= '" - . date(format: 'YmdHis') - . "'"; - $resultMock = $this->createStub(\SQLite3Result::class); $this->dbMock ->expects($this->once()) ->method('query') - ->with($this->stringContains("SELECT id FROM faqdata WHERE active = 'yes'")) + ->with($this->stringContains("SELECT fd.id FROM faqdata fd WHERE fd.active = 'yes'")) ->willReturn($resultMock); $this->dbMock->expects($this->once())->method('numRows')->with($resultMock)->willReturn(5); @@ -275,7 +268,7 @@ public function testTotalFaqsWithSpecificLanguage(): void $this->dbMock ->expects($this->once()) ->method('query') - ->with($this->stringContains("AND lang = 'en'")) + ->with($this->stringContains("AND fd.lang = 'en'")) ->willReturn($resultMock); $this->dbMock->expects($this->once())->method('numRows')->with($resultMock)->willReturn(3); diff --git a/tests/phpMyFAQ/FaqTest.php b/tests/phpMyFAQ/FaqTest.php index 1b334e031d..72eae5c60a 100644 --- a/tests/phpMyFAQ/FaqTest.php +++ b/tests/phpMyFAQ/FaqTest.php @@ -8,6 +8,7 @@ use phpMyFAQ\Core\Exception; use phpMyFAQ\Database\Sqlite3; use phpMyFAQ\Entity\FaqEntity; +use phpMyFAQ\Enums\PermissionType; use phpMyFAQ\Tenant\QuotaExceededException; use PHPUnit\Framework\Attributes\AllowMockObjectsWithoutExpectations; use PHPUnit\Framework\Attributes\UsesClass; @@ -603,6 +604,7 @@ public function testGetRecordBySolutionIdDoesNotReturnFaqRestrictedToAnotherUser public function testGetRecordBySolutionIdReturnsFaqForAuthorizedUser(): void { $faqEntity = $this->createFaqWithSolutionId(126); + $this->createReaderAccount(23); $this->grantUserAccess($faqEntity, 23); $this->faq->setUser(23); @@ -668,6 +670,7 @@ public function testIsFaqAccessibleForUserReturnsTrueForAuthorizedUser(): void { $faqEntity = $this->getFaqEntity(); $faqEntity->setId($this->createTrackedFaq($faqEntity)->getId()); + $this->createReaderAccount(23); $this->grantUserAccess($faqEntity, 23); $this->faq->setUser(23); @@ -877,6 +880,32 @@ private function grantUserAccess(FaqEntity $faqEntity, int $userId): void )); } + /** + * Creates a real account holding the FAQ read right. + * + * Reading is gated on PermissionType::FAQS_VIEW for logged-in users, so a bare user id with + * only a faqdata_user row is denied — as it would be on a real installation, where the + * upgrade and User::createUser() are what hand out the right. + */ + private function createReaderAccount(int $userId): void + { + $database = $this->configuration->getDb(); + + $database->query(sprintf( + "INSERT INTO faquser (user_id, login, session_timestamp, member_since, account_status) + VALUES (%d, 'reader%d', 0, '20260101000000', 'active')", + $userId, + $userId, + )); + + $database->query(sprintf( + 'INSERT INTO faquser_right (user_id, right_id) + SELECT %d, right_id FROM faqright WHERE name = %s', + $userId, + "'" . $database->escape(PermissionType::FAQS_VIEW->value) . "'", + )); + } + private function grantGroupAccess(FaqEntity $faqEntity, int $groupId): void { $this->configuration diff --git a/tests/phpMyFAQ/Search/SearchResultSetTest.php b/tests/phpMyFAQ/Search/SearchResultSetTest.php index eab349f676..217da41371 100644 --- a/tests/phpMyFAQ/Search/SearchResultSetTest.php +++ b/tests/phpMyFAQ/Search/SearchResultSetTest.php @@ -5,6 +5,7 @@ use phpMyFAQ\Configuration; use phpMyFAQ\Database\Sqlite3; use phpMyFAQ\Faq\Permission; +use phpMyFAQ\Faq\ReadScope; use phpMyFAQ\Strings; use phpMyFAQ\User\CurrentUser; use PHPUnit\Framework\Attributes\AllowMockObjectsWithoutExpectations; @@ -35,9 +36,9 @@ protected function setUp(): void } /** - * Builds a SearchResultSet whose permission level, permission lookups and current - * user id are fully controlled, so reviewResultSet() can be exercised deterministically - * without touching the database. + * Builds a SearchResultSet whose permission level, permission lookups, current user id + * and read scope are fully controlled, so reviewResultSet() can be exercised + * deterministically without touching the database. * * @param int[] $permissionResult value returned by Permission::get() for every lookup */ @@ -45,17 +46,56 @@ private function makeReviewableResultSet( string $permLevel = 'basic', array $permissionResult = [-1], int $userId = 1, + ?ReadScope $readScope = null, + array $storedFaqRows = [], ): SearchResultSet { $configuration = $this->createMock(Configuration::class); $configuration->method('get')->willReturn($permLevel); + if ($storedFaqRows !== []) { + $database = $this->createMock(Sqlite3::class); + $database->method('queryPrepared')->willReturn(true); + $database->method('fetchAll')->willReturn($storedFaqRows); + $configuration->method('getDb')->willReturn($database); + } + $currentUser = $this->createMock(CurrentUser::class); $currentUser->method('getUserId')->willReturn($userId); $faqPermission = $this->createMock(Permission::class); $faqPermission->method('get')->willReturn($permissionResult); - return new SearchResultSet($currentUser, $faqPermission, $configuration); + return new class ($currentUser, $faqPermission, $configuration, $readScope) extends SearchResultSet { + public function __construct( + CurrentUser $currentUser, + Permission $faqPermission, + Configuration $configuration, + private readonly ?ReadScope $readScope, + ) { + parent::__construct($currentUser, $faqPermission, $configuration); + } + + protected function resolveReadScope(): ReadScope + { + return $this->readScope ?? ReadScope::unrestricted(); + } + }; + } + + /** + * A scope granting FAQS_VIEW restricted to the given categories and languages. + * + * @param int[]|null $categoryIds + * @param string[]|null $languages + */ + private function makeRestrictedScope(?array $categoryIds, ?array $languages): ReadScope + { + $permission = $this->createMock(\phpMyFAQ\Permission\PermissionInterface::class); + $permission->method('hasPermission')->willReturn(true); + $permission->method('getAllowedCategoriesForRight')->willReturn($categoryIds); + $permission->method('getAllowedLanguagesForRight')->willReturn($languages); + + return ReadScope::forUser($permission, 1); } private function makeResult(int $id, array $properties = []): stdClass @@ -236,4 +276,84 @@ public function testReviewResultSetWithEmptyInputYieldsNoResults(): void $this->assertSame([], $searchResultSet->getResultSet()); $this->assertSame(0, $searchResultSet->getNumberOfResults()); } + + public function testReviewResultSetKeepsScopedHitWithinScope(): void + { + $scope = $this->makeRestrictedScope([3], ['en']); + $searchResultSet = $this->makeReviewableResultSet('basic', [-1], 1, $scope); + + $searchResultSet->reviewResultSet([$this->makeResult(1, ['lang' => 'en', 'category_id' => 3])]); + + $this->assertCount(1, $searchResultSet->getResultSet()); + } + + public function testReviewResultSetDropsScopedHitOutsideScope(): void + { + $scope = $this->makeRestrictedScope([3], ['en']); + $searchResultSet = $this->makeReviewableResultSet('basic', [-1], 1, $scope); + + $searchResultSet->reviewResultSet([$this->makeResult(1, ['lang' => 'en', 'category_id' => 9])]); + + $this->assertSame([], $searchResultSet->getResultSet()); + } + + public function testReviewResultSetFailsClosedForScopedHitWithoutLanguageAndCategory(): void + { + // The hit carries neither language nor category, and the record cannot be resolved + // from storage either (the mocked driver returns nothing) — it must not pass. + $scope = $this->makeRestrictedScope([3], ['en']); + $searchResultSet = $this->makeReviewableResultSet('basic', [-1], 1, $scope); + + $searchResultSet->reviewResultSet([$this->makeResult(1)]); + + $this->assertSame([], $searchResultSet->getResultSet()); + $this->assertSame(0, $searchResultSet->getNumberOfResults()); + } + + public function testReviewResultSetAuthorizesIncompleteHitFromStoredRows(): void + { + $scope = $this->makeRestrictedScope([3], ['en']); + $storedRow = new stdClass(); + $storedRow->id = 1; + $storedRow->lang = 'en'; + $storedRow->category_id = 3; + + $searchResultSet = $this->makeReviewableResultSet('basic', [-1], 1, $scope, [$storedRow]); + + // The hit carries an allowed category but no language; the stored row supplies it. + $searchResultSet->reviewResultSet([$this->makeResult(1, ['category_id' => 3])]); + + $this->assertCount(1, $searchResultSet->getResultSet()); + } + + public function testReviewResultSetDropsIncompleteHitClaimingDeniedCategory(): void + { + $scope = $this->makeRestrictedScope([3], ['en']); + + // The FAQ also belongs to an allowed category and language — but the hit itself + // claims the denied category 9, so another membership must not admit it. + $storedRow = new stdClass(); + $storedRow->id = 1; + $storedRow->lang = 'en'; + $storedRow->category_id = 3; + + $searchResultSet = $this->makeReviewableResultSet('basic', [-1], 1, $scope, [$storedRow]); + + $searchResultSet->reviewResultSet([$this->makeResult(1, ['category_id' => 9])]); + + $this->assertSame([], $searchResultSet->getResultSet()); + } + + public function testReviewResultSetWithoutRightFailsClosedForIncompleteHit(): void + { + $permission = $this->createMock(\phpMyFAQ\Permission\PermissionInterface::class); + $permission->method('hasPermission')->willReturn(false); + $scope = ReadScope::forUser($permission, 1); + + $searchResultSet = $this->makeReviewableResultSet('basic', [-1], 1, $scope); + + $searchResultSet->reviewResultSet([$this->makeResult(1)]); + + $this->assertSame([], $searchResultSet->getResultSet()); + } } diff --git a/tests/phpMyFAQ/Setup/Migration/Operations/PermissionBackfillOperationTest.php b/tests/phpMyFAQ/Setup/Migration/Operations/PermissionBackfillOperationTest.php new file mode 100644 index 0000000000..7368d0fa57 --- /dev/null +++ b/tests/phpMyFAQ/Setup/Migration/Operations/PermissionBackfillOperationTest.php @@ -0,0 +1,367 @@ +getProperty('configuration'); + $this->previousConfiguration = $configurationProperty->getValue(); + + $databasePath = tempnam(sys_get_temp_dir(), 'pmf-perm-backfill-'); + self::assertNotFalse($databasePath); + self::assertTrue(copy(PMF_TEST_DIR . '/test.db', $databasePath)); + $this->databasePath = $databasePath; + + $this->dbHandle = new Sqlite3(); + $this->dbHandle->connect($this->databasePath, '', ''); + $this->initializeDatabaseStatics($this->dbHandle); + $this->configuration = new Configuration($this->dbHandle); + $configurationProperty->setValue(null, $this->configuration); + $this->configuration->set('security.permLevel', 'medium'); + + $this->dbHandle->query('DELETE FROM faqgroup_right_language'); + $this->dbHandle->query('DELETE FROM faqgroup_right_category'); + $this->dbHandle->query('DELETE FROM faquser_right_language'); + $this->dbHandle->query('DELETE FROM faqgroup_right'); + $this->dbHandle->query('DELETE FROM faqgroup'); + } + + protected function tearDown(): void + { + $configurationReflection = new ReflectionClass(Configuration::class); + $configurationReflection->getProperty('configuration')->setValue(null, $this->previousConfiguration); + + if (isset($this->dbHandle)) { + $this->dbHandle->close(); + } + + $databaseReflection = new ReflectionClass(Database::class); + $databaseReflection->getProperty('databaseDriver')->setValue(null, null); + $databaseReflection->getProperty('dbType')->setValue(null, ''); + + if (isset($this->databasePath) && is_file($this->databasePath)) { + unlink($this->databasePath); + } + + parent::tearDown(); + } + + public function testGetTypeAndDescriptionDescribeTheBackfill(): void + { + $operation = new PermissionBackfillOperation( + $this->configuration, + 'faq_publish', + 'Right to publish FAQs', + mirrorFrom: 'approverec', + ); + + self::assertSame('permission_backfill', $operation->getType()); + self::assertSame('faq_publish', $operation->getPermissionName()); + self::assertSame('Right to publish FAQs', $operation->getPermissionDescription()); + self::assertSame('approverec', $operation->getMirrorFrom()); + self::assertStringContainsString('faq_publish', $operation->getDescription()); + self::assertStringContainsString('approverec', $operation->getDescription()); + } + + public function testToArrayCarriesTheConfiguration(): void + { + $operation = new PermissionBackfillOperation( + $this->configuration, + 'view_faqs', + 'Right to view FAQs', + grantToAllUsers: true, + grantToAllGroups: true, + ); + + $array = $operation->toArray(); + + self::assertSame('permission_backfill', $array['type']); + self::assertSame('view_faqs', $array['permissionName']); + self::assertTrue($array['grantToAllUsers']); + self::assertTrue($array['grantToAllGroups']); + self::assertNull($array['mirrorFrom']); + } + + /** + * PermissionGrantOperation reports failure when the right already exists, because + * addRight() returns 0 for a duplicate name. A backfill exists precisely to enforce a right + * that every installation already carries, so it has to succeed instead. + */ + public function testSucceedsForARightThatAlreadyExists(): void + { + $this->dbHandle->query('DELETE FROM faquser_right'); + + $operation = new PermissionBackfillOperation( + $this->configuration, + PermissionType::FAQS_VIEW->value, + 'Right to view FAQs', + grantToAllUsers: true, + ); + + self::assertTrue($operation->execute()); + self::assertSame(1, $this->countRights(PermissionType::FAQS_VIEW->value)); + self::assertTrue($this->userHasRight(1, PermissionType::FAQS_VIEW->value)); + } + + public function testCreatesTheRightWhenItIsMissing(): void + { + $this->dbHandle->query("DELETE FROM faqright WHERE name = 'faq_publish'"); + self::assertSame(0, $this->countRights('faq_publish')); + + $operation = new PermissionBackfillOperation( + $this->configuration, + 'faq_publish', + 'Right to publish FAQs', + grantToAllUsers: true, + ); + + self::assertTrue($operation->execute()); + self::assertSame(1, $this->countRights('faq_publish')); + self::assertTrue($this->userHasRight(1, 'faq_publish')); + } + + /** + * grantUserRight() is a plain INSERT, so a second run after a partially failed update must + * not blow up on the primary key. + */ + public function testIsIdempotent(): void + { + $operation = new PermissionBackfillOperation( + $this->configuration, + PermissionType::FAQS_VIEW->value, + 'Right to view FAQs', + grantToAllUsers: true, + grantToAllGroups: true, + ); + + self::assertTrue($operation->execute()); + self::assertTrue($operation->execute()); + + self::assertSame(1, $this->countRights(PermissionType::FAQS_VIEW->value)); + self::assertSame(1, $this->countUserGrants(1, PermissionType::FAQS_VIEW->value)); + } + + public function testMirrorsGrantsFromTheSourceRight(): void + { + $this->dbHandle->query("DELETE FROM faqright WHERE name = 'faq_publish'"); + $this->dbHandle->query('DELETE FROM faquser_right'); + $approveRightId = $this->rightId(PermissionType::FAQ_APPROVE->value); + $this->dbHandle->query(sprintf( + 'INSERT INTO faquser_right (user_id, right_id) VALUES (1, %d)', + $approveRightId, + )); + $this->createGroup(7); + $this->dbHandle->query(sprintf( + 'INSERT INTO faqgroup_right (group_id, right_id) VALUES (7, %d)', + $approveRightId, + )); + + $operation = new PermissionBackfillOperation( + $this->configuration, + 'faq_publish', + 'Right to publish FAQs', + mirrorFrom: PermissionType::FAQ_APPROVE->value, + ); + + self::assertTrue($operation->execute()); + + self::assertTrue($this->userHasRight(1, 'faq_publish')); + self::assertTrue($this->groupHasRight(7, 'faq_publish')); + } + + /** + * Without carrying the restrictions across, a group allowed to approve German FAQs in one + * category would come out of the upgrade able to publish everything, everywhere. + */ + public function testMirrorsCategoryAndLanguageRestrictions(): void + { + $this->dbHandle->query("DELETE FROM faqright WHERE name = 'faq_publish'"); + $approveRightId = $this->rightId(PermissionType::FAQ_APPROVE->value); + $this->createGroup(7); + $this->dbHandle->query(sprintf( + 'INSERT INTO faqgroup_right (group_id, right_id) VALUES (7, %d)', + $approveRightId, + )); + + new GroupCategoryPermissionRepository($this->configuration) + ->setCategoryRestrictions(7, $approveRightId, [3, 4]); + new LanguagePermissionRepository($this->configuration) + ->setLanguageRestrictions(7, $approveRightId, ['de']); + + $operation = new PermissionBackfillOperation( + $this->configuration, + 'faq_publish', + 'Right to publish FAQs', + mirrorFrom: PermissionType::FAQ_APPROVE->value, + mirrorRestrictions: true, + ); + + self::assertTrue($operation->execute()); + + $publishRightId = $this->rightId('faq_publish'); + $categories = new GroupCategoryPermissionRepository($this->configuration) + ->getCategoryRestrictions(7, $publishRightId); + $languages = new LanguagePermissionRepository($this->configuration) + ->getLanguageRestrictions(7, $publishRightId); + + sort($categories); + self::assertSame([3, 4], $categories); + self::assertSame(['de'], $languages); + } + + /** + * An unrestricted source grant must stay unrestricted: an empty restriction set means + * "everywhere", so there is nothing to copy and nothing may be invented. + */ + public function testDoesNotInventRestrictionsForAnUnrestrictedGrant(): void + { + $this->dbHandle->query("DELETE FROM faqright WHERE name = 'faq_publish'"); + $approveRightId = $this->rightId(PermissionType::FAQ_APPROVE->value); + $this->createGroup(7); + $this->dbHandle->query(sprintf( + 'INSERT INTO faqgroup_right (group_id, right_id) VALUES (7, %d)', + $approveRightId, + )); + + $operation = new PermissionBackfillOperation( + $this->configuration, + 'faq_publish', + 'Right to publish FAQs', + mirrorFrom: PermissionType::FAQ_APPROVE->value, + mirrorRestrictions: true, + ); + + self::assertTrue($operation->execute()); + + $publishRightId = $this->rightId('faq_publish'); + self::assertSame( + [], + new GroupCategoryPermissionRepository($this->configuration) + ->getCategoryRestrictions(7, $publishRightId), + ); + } + + private function createGroup(int $groupId): void + { + $this->dbHandle->query(sprintf( + "INSERT INTO faqgroup (group_id, name, description, auto_join) VALUES (%d, 'group%d', '', 0)", + $groupId, + $groupId, + )); + } + + private function rightId(string $name): int + { + $result = $this->dbHandle->query(sprintf("SELECT right_id FROM faqright WHERE name = '%s'", $name)); + $row = $this->dbHandle->fetchArray($result); + + return is_array($row) ? (int) $row['right_id'] : 0; + } + + private function countRights(string $name): int + { + $result = $this->dbHandle->query(sprintf("SELECT COUNT(*) AS c FROM faqright WHERE name = '%s'", $name)); + $row = $this->dbHandle->fetchArray($result); + + return is_array($row) ? (int) $row['c'] : 0; + } + + private function countUserGrants(int $userId, string $rightName): int + { + $result = $this->dbHandle->query(sprintf( + 'SELECT COUNT(*) AS c FROM faquser_right WHERE user_id = %d AND right_id = %d', + $userId, + $this->rightId($rightName), + )); + $row = $this->dbHandle->fetchArray($result); + + return is_array($row) ? (int) $row['c'] : 0; + } + + private function userHasRight(int $userId, string $rightName): bool + { + return $this->countUserGrants($userId, $rightName) > 0; + } + + private function groupHasRight(int $groupId, string $rightName): bool + { + $result = $this->dbHandle->query(sprintf( + 'SELECT COUNT(*) AS c FROM faqgroup_right WHERE group_id = %d AND right_id = %d', + $groupId, + $this->rightId($rightName), + )); + $row = $this->dbHandle->fetchArray($result); + + return is_array($row) && (int) $row['c'] > 0; + } + + private function initializeDatabaseStatics(Sqlite3 $dbHandle): void + { + $databaseReflection = new ReflectionClass(Database::class); + $databaseReflection->getProperty('databaseDriver')->setValue(null, $dbHandle); + $databaseReflection->getProperty('dbType')->setValue(null, 'sqlite3'); + Database::setTablePrefix(''); + } +} diff --git a/tests/phpMyFAQ/SitemapTest.php b/tests/phpMyFAQ/SitemapTest.php index 8f541f56b7..2b4b9ae35f 100644 --- a/tests/phpMyFAQ/SitemapTest.php +++ b/tests/phpMyFAQ/SitemapTest.php @@ -32,8 +32,18 @@ use PHPUnit\Framework\TestCase; use stdClass; use Symfony\Component\HttpFoundation\Session\Session; +use phpMyFAQ\Auth\AuthDatabase; +use phpMyFAQ\Auth\PasswordHasher; use phpMyFAQ\Configuration\Storage\FilesystemConfigurationCache; +use phpMyFAQ\Faq\ReadScope; +use phpMyFAQ\Permission\BasicPermission; +use phpMyFAQ\Permission\BasicPermissionRepository; +use phpMyFAQ\Permission\LanguagePermissionRepository; use phpMyFAQ\Plugin\PluginDiscovery; +use phpMyFAQ\Session\SessionWrapper; +use phpMyFAQ\User\CurrentUser; +use phpMyFAQ\User\UserData; +use phpMyFAQ\User\UserSession; #[CoversClass(Sitemap::class)] #[UsesClass(Configuration::class)] @@ -72,12 +82,27 @@ #[UsesClass(Translation::class)] #[UsesClass(FilesystemConfigurationCache::class)] #[UsesClass(PluginDiscovery::class)] +#[UsesClass(Auth::class)] +#[UsesClass(AuthDatabase::class)] +#[UsesClass(PasswordHasher::class)] +#[UsesClass(Encryption::class)] +#[UsesClass(ReadScope::class)] +#[UsesClass(Permission::class)] +#[UsesClass(BasicPermission::class)] +#[UsesClass(BasicPermissionRepository::class)] +#[UsesClass(LanguagePermissionRepository::class)] +#[UsesClass(SessionWrapper::class)] +#[UsesClass(User::class)] +#[UsesClass(CurrentUser::class)] +#[UsesClass(UserData::class)] +#[UsesClass(UserSession::class)] class SitemapTest extends TestCase { private const int FAQ_ID = 9990; private const int FAQ_ID_2 = 9991; private const int CAT_ID = 9990; private const int CAT_ID_2 = 9991; + private const int READER_USER_ID = 42; private Configuration $configuration; @@ -190,8 +215,22 @@ public function testSetUser(): void $this->assertFalse($hasUser42, 'user42 question should not be visible to user -1'); + // Reading is gated on the FAQ read right for logged-in users, so user 42 has to be a + // real account holding it — as every account is after the upgrade. cleanupTestData() + // removes it again, because this suite shares the fixture database. + $this->db->query(sprintf( + "INSERT INTO faquser (user_id, login, session_timestamp, member_since, account_status) + VALUES (%d, 'user42', 0, '20260101000000', 'active')", + self::READER_USER_ID, + )); + $this->db->query(sprintf( + "INSERT INTO faquser_right (user_id, right_id) + SELECT %d, right_id FROM faqright WHERE name = 'view_faqs'", + self::READER_USER_ID, + )); + // After setUser(42), user42 question should be visible - $sitemap->setUser(42); + $sitemap->setUser(self::READER_USER_ID); $faqs = $sitemap->getFaqsFromLetter('U'); $hasUser42 = false; foreach ($faqs as $faq) { @@ -384,6 +423,12 @@ private function cleanupTestData(): void $this->db->query( "DELETE FROM faqcategoryrelations WHERE record_id IN ($idList) AND category_id IN ($catIdList)", ); + + // This suite works on the shared fixture database, so the reader account created by + // testSetUser() has to go too — leaving it behind would hand every later test a user + // that already holds the FAQ read right. + $this->db->query('DELETE FROM faquser_right WHERE user_id = ' . self::READER_USER_ID); + $this->db->query('DELETE FROM faquser WHERE user_id = ' . self::READER_USER_ID); } public function testGetAllFirstLettersNormalizesGroupIds(): void