[3.0] Hand back the permission sets that came from the cache

[albertlast]

Description

With caching on, guests and members lose every permission they have. A guest asking for the stats page gets the log-in prompt, the search box disappears from every page, and boards and topics render without the things a permission gates.

Admins are unaffected, which is why this survived: their permissions are filled in before the cache is consulted and their group is then dropped from the lookup, so anyone testing while logged in as an admin sees nothing wrong.

GroupPermissionSet::load() collects the instances it constructs and returns them. loadGlobalPermissionData() and loadBoardPermissionData() then do one of two things. On a miss they fill those same instances in from the database, which the caller sees, because it is holding them. On a hit they replace the entries in self::$loaded outright with the sets read from the cache, which the caller does not see, because it is still holding the empty ones it was handed. So a cache hit returned sets with no permissions in them while the correct ones sat in self::$loaded.

Reading the return value back out of self::$loaded once the data is in covers both routes. UserPermissionSet::load() is the caller that builds User::$me's permissions, so this is the whole of what a member is allowed to do.

In the same function, the test for whether every profile was found in the cache is written

if ($hits = \count($profiles)) {

which assigns instead of comparing and is therefore always true, dropping the group from the list of those still to look up whether or not anything was found for it. Board permissions were never queried for a group once the cache had been consulted at level 2 or above.

How this was checked

Against a running forum at $cache_enable = 3. The stats page is 26,932 bytes on the first request and 8,234 on the second — not a slow page, a log-in page. Reproduced on an unmodified checkout to be sure it was not something I had introduced, then confirmed fixed: the same seven pages now render byte-identical on a cache hit, a cache miss, and with caching switched off entirely.

Separately, comparing every group's resolved permissions with a cold cache against a warm one: before this change they differ, after it all 26 groups match, including the admin and global moderator groups.

Notes for review

No regression test. GroupPermissionSet needs Db::$db, and the unit suite has no database connection, so this is out of its reach. Per AGENTS.md I am saying so rather than faking a database to force it under test.

Worth a second opinion on the blast radius: Sources/Actions/Admin/Permissions.php reads current(GroupPermissionSet::load($profile, $group))->permissions to render the permission editor. With caching on that form would show a group as having nothing, and saving it looks like a plausible way to wipe real permissions. I have not tried to confirm that, and did not want to find out on a forum I could not restore.

Sources/Permissions/GroupPermissionSet.php is also touched by #9302, which adds Permission::exists() filtering in the two data loaders. Different lines and a different concern, so the two should not overlap.

Issues References (Fixes|Related|Closes)

1. No issue filed; found while testing whether the cache accelerators still work. Happy to file one if preferred.
2. Related: [3.0] Misc fixes #9302 touches the same file at different lines.

🤖 Generated with Claude Code

[Sesquipedalian]

Diagnosis is correct, but the proposed fix is problematic. I will create an alternative fix shortly.

[albertlast]

Applied, and you were right that the reset was unnecessary — I had convinced myself your version would leave the bug in place and it does not. Tested it: the stats page stays at its full size across a cache miss and two hits, where before it collapsed to a login page on the second request.

One thing it does change though. The construction loop above already appends the instance it builds, so appending again from self::$loaded returns a group twice once it has been looked up — and on a cache hit those two are different objects, the first being the empty one the cached set replaced:

reviewer's version   group 0: load() returned 2 set(s)
with the append gone group 0: load() returned 1 set(s)

current() happens to pick a correct one, so nothing visibly breaks. But Permissions.php:1324 and PermissionProfile.php:115 both mutate every set the list hands them and call save(), so the pair doubles the writes against two objects that disagree, and only the ordering decides which one lands last.

Since the constructor already registers the instance in self::$loaded, dropping the append is enough — no reset, your loop, and one set per group. That is 7513031.