From 80bc86535da657fb1f6bdb6f5c97be261b742ee1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93scar=20Beiro?= Date: Wed, 26 Aug 2026 19:48:39 +0200 Subject: [PATCH 1/2] Fix activate/deactivate group membership always failing with CSRF denial GLPI 11's core CheckCsrfListener already validates and consumes the CSRF token for every POST request before a Symfony-routed controller runs. The manual Session::validateCSRF() call added in 2.0.2 always found the token already spent, so every activate/deactivate click was rejected. Removed the redundant check; core already enforces it. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01EsonuwXxLGnoCtfkviVBmC --- CHANGELOG.md | 2 + src/Controller/GroupActionController.php | 8 ++-- tests/GroupActionControllerTest.php | 59 ++---------------------- 3 files changed, 11 insertions(+), 58 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e31d26d..abec519 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,8 @@ # Changelog ## [Unreleased] +### Bugs +- Fix the activate/deactivate group membership action always failing with an access-denied error ## 2.0.2 - 26/08/2026 ### Bugs diff --git a/src/Controller/GroupActionController.php b/src/Controller/GroupActionController.php index 087c046..8fedf0c 100644 --- a/src/Controller/GroupActionController.php +++ b/src/Controller/GroupActionController.php @@ -49,9 +49,11 @@ public function __invoke(Request $request): Response { global $CFG_GLPI; - if (!Session::validateCSRF($request->request->all())) { - throw new AccessDeniedHttpException(); - } + // GLPI's core CheckCsrfListener (kernel.controller event) already + // validates and consumes the CSRF token for every non-safe-method + // request before a Symfony-routed controller runs. A second manual + // Session::validateCSRF() call here always fails: the token has + // already been unset from $_SESSION by the time we get here. $rowaction = $request->request->get('rowaction'); $rowid = $request->request->get('rowid'); diff --git a/tests/GroupActionControllerTest.php b/tests/GroupActionControllerTest.php index c42c839..f9ef963 100644 --- a/tests/GroupActionControllerTest.php +++ b/tests/GroupActionControllerTest.php @@ -64,61 +64,10 @@ public function testMissingParametersAreBadRequest(): void $controller($request); } - public function testMissingCsrfTokenIsDenied(): void - { - $this->login(); - - $group = $this->createItem('Group', [ - 'name' => 'moregroups-ctrl-test-' . uniqid(), - 'entities_id' => getItemByTypeName('Entity', '_test_root_entity', true), - ]); - $user_id = getItemByTypeName('User', TU_USER, true); - - $tracked = $this->createItem('PluginMoregroupsGroup', [ - 'groups_id' => $group->getID(), - 'users_id' => $user_id, - ]); - - $controller = new GroupActionController(); - // No `_glpi_csrf_token` at all - regression test for the missing - // CSRF validation found by the security audit. - $request = new Request([], ['rowaction' => 'activate', 'rowid' => (string) $tracked->getID()]); - - $this->expectException(AccessDeniedHttpException::class); - $controller($request); - - $leftover = new PluginMoregroupsGroup(); - $this->assertTrue( - $leftover->getFromDB($tracked->getID()), - 'A request without a valid CSRF token must not perform the activation' - ); - } - - public function testInvalidCsrfTokenIsDenied(): void - { - $this->login(); - - $group = $this->createItem('Group', [ - 'name' => 'moregroups-ctrl-test-' . uniqid(), - 'entities_id' => getItemByTypeName('Entity', '_test_root_entity', true), - ]); - $user_id = getItemByTypeName('User', TU_USER, true); - - $tracked = $this->createItem('PluginMoregroupsGroup', [ - 'groups_id' => $group->getID(), - 'users_id' => $user_id, - ]); - - $controller = new GroupActionController(); - $request = new Request([], [ - 'rowaction' => 'activate', - 'rowid' => (string) $tracked->getID(), - '_glpi_csrf_token' => 'not-a-real-token', - ]); - - $this->expectException(AccessDeniedHttpException::class); - $controller($request); - } + // CSRF token presence/validity for this route is enforced by GLPI core's + // CheckCsrfListener (kernel.controller event) before the controller is + // ever invoked - it isn't exercisable from a direct controller call, so + // it isn't re-tested here. See GroupActionController::__invoke(). public function testActivateWithoutRightsIsDenied(): void { From b048ed75dfd6ff3d1c0860a5f6ab80c7197c4ee9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93scar=20Beiro?= Date: Wed, 26 Aug 2026 22:06:13 +0200 Subject: [PATCH 2/2] Fix deactivate button never rendering on the group Users tab MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The JS selector that injects the "Deactivate" button next to each active member was malformed (input[name^='item[Group_User]' — missing the closing ]), so jQuery threw a syntax error and the whole $(document).ready callback aborted. No deactivate button ever appeared, making the plugin's core deactivate flow unusable from the UI. Also adds a run-moregroups Claude Code skill with a Playwright driver that exercises the full deactivate/reactivate flow — both the single-row buttons and both massive actions — against a live GLPI 11 instance, to catch this class of bug (invisible to php -l/phpcs/PHPStan) going forward. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_011w4D3KeE6Z3BCX6nZgy1CX --- .claude/skills/run-moregroups/SKILL.md | 142 +++++++++++++++++++++++ .claude/skills/run-moregroups/driver.mjs | 129 ++++++++++++++++++++ .gitignore | 4 +- CHANGELOG.md | 1 + inc/group.class.php | 2 +- 5 files changed, 276 insertions(+), 2 deletions(-) create mode 100644 .claude/skills/run-moregroups/SKILL.md create mode 100644 .claude/skills/run-moregroups/driver.mjs diff --git a/.claude/skills/run-moregroups/SKILL.md b/.claude/skills/run-moregroups/SKILL.md new file mode 100644 index 0000000..45aebf9 --- /dev/null +++ b/.claude/skills/run-moregroups/SKILL.md @@ -0,0 +1,142 @@ +--- +name: run-moregroups +description: Build, install, and drive the More Groups GLPI plugin in a real running GLPI 11 instance — deactivate/reactivate a group member both via the single-row buttons and via the massive actions, through the actual UI, with a screenshot after each step. Use when asked to run, test, smoke-test, or screenshot More Groups, or to confirm a code change actually works in the browser (not just phpunit/phpcs). +--- + +Paths below are relative to the repo root (`moregroups/`), not this skill directory. + +More Groups is a classic-style (`inc/`) + modern (`src/Controller`) GLPI 11 plugin. There is +no standalone dev server — "running" it means a live GLPI 11 container with the plugin files +mounted, installed and activated, driven with Playwright through the actual `/front/group.form.php` +UI. Don't just `php -l` the change — this skill proves it renders and functions. + +## Prerequisites (already verified in this container) + +- Podman, with a GLPI 11 container already up (see below). +- `node_modules/@playwright/test` at repo root: `npm install` (root `package.json` already + pins `"@playwright/test": "1.62.1"`). +- Chromium browser binary: `npx playwright install chromium` (works without `--with-deps` — + the OS libs this needs are already present on this host; `--with-deps` fails here because it + needs passwordless `sudo`, which isn't available). + +## Getting a running GLPI instance + +This repo doesn't ship a compose file for a throwaway instance. In this environment there is +already a long-running GLPI 11.0.8 test stack usable for this: `glpi-65000-web` / +`glpi-65000-db` (podman), published at `http://localhost:65000`, default creds `glpi`/`glpi`. +If it's not running, any GLPI 11 container with the plugin's files mounted at +`/var/www/glpi/plugins/moregroups` and activated (Setup → Plugins) works the same way — update +`BASE_URL` below accordingly. + +**Sync your working tree into the running container** (this container mounts plugin code from +a host path, not directly from this repo checkout): + +```bash +rsync -a --exclude=vendor --exclude=.git --exclude=node_modules --exclude=tests --exclude=tools \ + inc src hook.php setup.php locales \ + /home/oscar/containers/all-plugins/moregroups/ +``` + +Then, if you only changed PHP under `inc/`/`src/`, no reinstall is needed — GLPI reads plugin +classes on every request. If you added a new migration/table, reinstall from Setup → Plugins. + +**PHP-file edits can hit a stale opcache** even after `rsync` updates the file on disk +(confirmed pattern for this stack — see the `glpi-plugin-builder` skill's Trap 7). If a fix +"isn't working" right after editing PHP, restart the container before concluding the fix is +wrong: + +```bash +podman restart glpi-65000-web +``` + +## Run (agent path) — the driver + +```bash +cd moregroups # repo root +npm install # once +npx playwright install chromium # once (cached after first run) +node .claude/skills/run-moregroups/driver.mjs http://localhost:65000 1 +``` + +The `1` is a `Group` id that already has ≥1 active `Group_User` member on this test instance +(group "TEST", id 1). The driver: + +1. Logs in as `glpi`/`glpi`. +2. Opens `/front/group.form.php?id=` and the **Users** tab. +3. Waits for the **Deactivate user** button to render next to the first active member — this + is the exact assertion that catches the malformed-jQuery-selector regression fixed on + 2026-08-26 (`inc/group.class.php`'s `input[name^='item[Group_User]']` selector). +4. Clicks it, confirms the member now appears in the **Deactivated users** panel. +5. Clicks that row's **Activate user** button, confirms the member is back in the active list. +6. Selects that same member's checkbox in the active-members table, opens the **Actions** + modal, picks **Deactivate users** (the `PluginMoregroupsGroup:deactivate` massive action + wired via `plugin_moregroups_MassiveActions` in `hook.php`), submits, confirms the member + is now in the **Deactivated users** panel, and confirms the page reloaded back onto the + same `group.form.php?id=` (not some other URL). +7. Selects that member's checkbox in the **Deactivated users** panel, opens its own **Actions** + modal, picks **Activate users** (`getSpecificMassiveActions` in `inc/group.class.php`), + submits, confirms the member is back in the active list, and confirms the same reload check. +8. Screenshots after every step into `.claude/skills/run-moregroups/screenshots/` + (`01-logged-in.png` … `06-massive-activated.png`, or `NN-FAILURE.png` on a thrown step). + +Exit code is non-zero on failure; read stdout for which step failed and check the `FAILURE` +screenshot. + +**The DB state this driver leaves behind is idempotent** — deactivate/reactivate and the +massive-action round trip both net out to the same active/deactivated row counts — safe to +re-run without manual cleanup; verified by running the full 6-step sequence twice back to back. + +## Direct invocation (no browser) — checking PHP syntax/logic fast + +For a quick sanity pass on a PHP edit before spinning up the browser driver: + +```bash +php -l inc/group.class.php +php -l src/Controller/GroupActionController.php +``` + +This does **not** catch the JS-selector class of bug that broke the deactivate button — that +only shows up by actually rendering the page and clicking, i.e. the driver above. + +## Gotchas + +- **The malformed-selector bug is invisible to `php -l`, phpcs, and PHPStan.** It's embedded + PHP-heredoc JavaScript (`Html::scriptBlock($script)` in `showDeactivated()`). Nothing in the + PHP toolchain parses it. The only way to catch it is rendering the page and asserting the + button is actually present in the DOM — which is why the driver's `waitFor` on the + deactivate button (not just clicking it) is the load-bearing assertion. +- **`rsync --delete` would wipe files unique to the container copy** (it has no `.git`, + `.phpcs.xml`, etc. — those are dev-only files that were never meant to ship). Don't add + `--delete` to the sync command above. +- **`localhost` from inside a Playwright browser launched on the host reaches the container + fine** here because the container publishes `0.0.0.0:65000->80/tcp` — no + `host.containers.internal` dance needed, unlike the Playwright-inside-a-container setup used + by `tools/manual-generator/run.sh` (a separate, heavier pipeline for generating the published + user manual — see the `glpi-plugin-manual-generator` skill for that one; this skill is for + fast local smoke-testing, not documentation screenshots). +- **`npx playwright install chromium --with-deps` fails** in this container (`sudo: a + terminal is required`). Omit `--with-deps` — the browser launches fine without it here. + +## Troubleshooting + +- `DRIVER FAILED: locator.waitFor: Timeout ... button[title="Deactivate user"]` on step 3 → + the JS-selector regression is back (or a new one like it). Check + `Html::scriptBlock($script)` in `inc/group.class.php`'s `showDeactivated()` for a syntax + error in the embedded jQuery, and check browser console errors (the driver prints any + `pageerror`/console-error events it captured after `ALL STEPS COMPLETED` or right before a + failure). +- `no active Group_User rows found` → the seed group has no members left (e.g. previous run's + cleanup didn't happen). Reset via the DB directly: + ```bash + podman exec glpi-65000-db sh -c "mariadb -u65000_db_user -p65000_db_password 65000_db_name \ + -e \"INSERT IGNORE INTO glpi_groups_users (users_id, groups_id) VALUES (3,1);\"" + ``` +- `DRIVER FAILED: ... did not reload back onto the group form` on step 6 or 7 → the massive + action's redirect target regressed (compare against `MassiveAction`'s own redirect handling; + this plugin doesn't override it, so a failure here usually points at something upstream of + the plugin, e.g. a stale/incorrect `Referer`). Check the matching `NN-massive-*` screenshot + for what page it actually landed on. +- The massive-action steps select the option by its **visible label** (`selectOption({label: + ...})`), not its value — if the button label text changes (e.g. a locale/translation string + edit to `__('Deactivate users', 'moregroups')` / `__('Activate users', 'moregroups')` in + `inc/group.class.php`), update the driver's `selectOption` calls to match. diff --git a/.claude/skills/run-moregroups/driver.mjs b/.claude/skills/run-moregroups/driver.mjs new file mode 100644 index 0000000..cdca0ab --- /dev/null +++ b/.claude/skills/run-moregroups/driver.mjs @@ -0,0 +1,129 @@ +#!/usr/bin/env node +// driver.mjs — smoke-drives the More Groups plugin against a running GLPI 11 instance. +// +// Usage: +// node .claude/skills/run-moregroups/driver.mjs +// +// Example (against the glpi-65000-web test container used to build this skill): +// node .claude/skills/run-moregroups/driver.mjs http://localhost:65000 1 +// +// Logs in as glpi/glpi, opens the group's Users tab, deactivates the first active +// member found via the single-row button, verifies they land in the "Deactivated users" +// panel, reactivates them via the single-row button, verifies they're back in the active +// list, then repeats the same round trip through the massive-action UI (checkbox row + +// "Actions" modal + "Deactivate users"/"Activate users") and confirms each submit reloads +// back onto the same group's Users tab. Screenshots after each step land in +// .claude/skills/run-moregroups/screenshots/. + +import { chromium } from '@playwright/test'; +import { mkdirSync } from 'node:fs'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const here = dirname(fileURLToPath(import.meta.url)); +const shotsDir = join(here, 'screenshots'); +mkdirSync(shotsDir, { recursive: true }); + +const BASE_URL = process.argv[2] || 'http://localhost:65000'; +const GROUP_ID = process.argv[3] || '1'; + +let shotN = 0; +async function shot(page, name) { + shotN += 1; + const path = join(shotsDir, `${String(shotN).padStart(2, '0')}-${name}.png`); + await page.screenshot({ path, fullPage: true }); + console.log('screenshot:', path); +} + +async function main() { + const browser = await chromium.launch({ args: ['--no-sandbox'] }); + const page = await browser.newPage(); + const errors = []; + page.on('pageerror', (e) => errors.push(String(e))); + page.on('console', (m) => { if (m.type() === 'error') errors.push(m.text()); }); + + try { + console.log('==> login'); + await page.goto(`${BASE_URL}/`, { timeout: 60_000 }); + await page.locator('input[name="login_name"]').fill('glpi'); + await page.locator('input[name="login_password"]').fill('glpi'); + await page.locator('button[name="submit"], input[name="submit"]').first().click(); + await page.waitForLoadState('networkidle'); + await shot(page, 'logged-in'); + + console.log('==> open group Users tab'); + await page.goto(`${BASE_URL}/front/group.form.php?id=${GROUP_ID}`, { timeout: 30_000 }); + await page.getByRole('tab', { name: /^Users/ }).click(); + await page.getByText('Deactivated users').waitFor({ state: 'visible', timeout: 30_000 }); + await shot(page, 'users-tab'); + + console.log('==> single-row deactivate'); + const activeRows = page.locator('tr[data-itemtype="Group_User"]'); + const activeCountBefore = await activeRows.count(); + if (activeCountBefore === 0) throw new Error('no active Group_User rows found — seed the group with a member first'); + const targetRow = activeRows.first(); + const deactivateBtn = targetRow.locator('button[title="Deactivate user"]'); + await deactivateBtn.waitFor({ state: 'visible', timeout: 10_000 }); // <- fails if the JS selector bug regresses + const memberText = (await targetRow.locator('td').nth(1).innerText()).trim(); + await deactivateBtn.click(); + + const panel = page.locator('.card.m-n2', { has: page.locator('.card-title', { hasText: 'Deactivated users' }) }); + await panel.locator('tr', { hasText: memberText }).waitFor({ state: 'visible', timeout: 30_000 }); + await page.waitForLoadState('networkidle'); + console.log(' deactivated OK:', memberText.trim()); + await shot(page, 'deactivated'); + + console.log('==> single-row reactivate'); + const deactivatedRow = panel.locator('tr', { hasText: memberText }); + await deactivatedRow.locator('button[title="Activate user"]').click(); + await page.locator('tr[data-itemtype="Group_User"]', { hasText: memberText }).waitFor({ state: 'visible', timeout: 30_000 }); + await page.waitForLoadState('networkidle'); + console.log(' reactivated OK:', memberText.trim()); + await shot(page, 'reactivated'); + + console.log('==> massive action: deactivate'); + const panel2 = page.locator('.card.m-n2', { has: page.locator('.card-title', { hasText: 'Deactivated users' }) }); + const activeRow = page.locator('tr[data-itemtype="Group_User"]', { hasText: memberText }); + await activeRow.locator('input[type="checkbox"]').check(); + await page.locator('a[href*="modal_massaction_content"]').first().click(); + const maSelect = page.locator('.modal.show select[name="massiveaction"]'); + await maSelect.waitFor({ state: 'visible', timeout: 10_000 }); + await maSelect.selectOption({ label: 'Deactivate users' }); // <- fails if the massive "deactivate" action (hook.php's plugin_moregroups_MassiveActions) regresses + await page.locator('.modal.show button[name="massiveaction"], .modal.show input[name="massiveaction"]').first().click(); + await panel2.locator('tr', { hasText: memberText }).waitFor({ state: 'visible', timeout: 30_000 }); + if (!page.url().includes(`group.form.php?id=${GROUP_ID}`)) { + throw new Error(`massive deactivate did not reload back onto the group form (landed on ${page.url()})`); + } + console.log(' massive-deactivated OK, reloaded on', page.url()); + await shot(page, 'massive-deactivated'); + + console.log('==> massive action: activate'); + const deactivatedRow2 = panel2.locator('tr', { hasText: memberText }); + await deactivatedRow2.locator('input[type="checkbox"]').check(); + await panel2.locator('a[href*="modal_massaction_content"]').first().click(); + const maSelect2 = page.locator('.modal.show select[name="massiveaction"]'); + await maSelect2.waitFor({ state: 'visible', timeout: 10_000 }); + await maSelect2.selectOption({ label: 'Activate users' }); // <- fails if the specific massive "activate" action (getSpecificMassiveActions in inc/group.class.php) regresses + await page.locator('.modal.show button[name="massiveaction"], .modal.show input[name="massiveaction"]').first().click(); + await page.locator('tr[data-itemtype="Group_User"]', { hasText: memberText }).waitFor({ state: 'visible', timeout: 30_000 }); + if (!page.url().includes(`group.form.php?id=${GROUP_ID}`)) { + throw new Error(`massive activate did not reload back onto the group form (landed on ${page.url()})`); + } + console.log(' massive-activated OK, reloaded on', page.url()); + await shot(page, 'massive-activated'); + + console.log('ALL STEPS COMPLETED'); + if (errors.length) { + console.log('--- console/page errors seen during the run ---'); + errors.forEach((e) => console.log(e)); + } + } catch (err) { + await shot(page, 'FAILURE'); + console.error('DRIVER FAILED:', err.message); + process.exitCode = 1; + } finally { + await browser.close(); + } +} + +main(); diff --git a/.gitignore b/.gitignore index dfd6caa..1fe2553 100644 --- a/.gitignore +++ b/.gitignore @@ -1,2 +1,4 @@ /vendor -composer.lock \ No newline at end of file +composer.lock +/node_modules +/.claude/skills/run-moregroups/screenshots/ \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index abec519..3dcc2fe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## [Unreleased] ### Bugs - Fix the activate/deactivate group membership action always failing with an access-denied error +- Fix the "Deactivate" button never appearing next to active group members, which made it impossible to deactivate a membership from the Users tab ## 2.0.2 - 26/08/2026 ### Bugs diff --git a/inc/group.class.php b/inc/group.class.php index 57caef1..4362ba5 100644 --- a/inc/group.class.php +++ b/inc/group.class.php @@ -226,7 +226,7 @@ public static function showDeactivated($item) $script = <<