fix(theme): drop the menu perm markers dispatch never reads - #6426
Open
e107help[bot] wants to merge 1 commit into
Open
fix(theme): drop the menu perm markers dispatch never reads#6426e107help[bot] wants to merge 1 commit into
e107help[bot] wants to merge 1 commit into
Conversation
theme_admin declared a perm on every $adminMenu entry and nothing has ever read one to decide who may reach a route. Both readers of the key only decide whether an entry is drawn: e_admin_dispatcher::restrictMenuAccess(), and e_navigation ::admin() by way of the keys processMenuItem() copies into the rendered nav item. e_admin_dispatcher::hasRouteAccess() consults $access and $perm, neither of which theme_admin declares. So the five markers set to 0 only hid menu entries whose routes still answered when their URL was typed, upload and the converter included. An administrator granted Theme preferences was shown a menu saying they could not upload a theme, and had no way to learn otherwise short of reading the dispatcher. Cameron added TMP in a950bf9 to grant "access to only theme preferences (but not others)", and the markers are where that intent was written. Entry to the page and the navigation link have read 1|TMP ever since, and the POST handling in themeHandler has read it since f3bf462. Deltik's call is that TMP reaches everything the Theme Manager offers, the converter and installContent included, so the markers come out rather than the enforced gates being narrowed to meet them. Who may work the Theme Manager does not change; an administrator holding 1 or TMP now sees the five entries they could already reach. The key is deleted rather than set to the page gate, because the two readers disagree about an empty marker: restrictMenuAccess() treats '' as unset and draws the entry, while e_navigation::admin() passes it to getperms(), which answers false for every administrator whose permissions are not exactly 0. The regression test reads theme.php with the tokenizer, an admin entry point being impossible to include from a test, and fails on any perm marker in the $adminMenu literal, at any nesting. A marker written into the menu at runtime, the way a handful of other admin pages write a conditional one, is outside what it can see; this page builds no menu in code. It pins the decision rather than modelling the dispatcher: a test that weighed each marker against the page gate would have to know what $modes and the two properties declare, and would tell the reader to delete a marker that was doing real work the day this page gains one.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Fixes #6274.
theme_admindeclares apermon every$adminMenuentry, five of the six set to'0', and nothing reads one to decide who may reach a route.e_admin_dispatcher::hasRouteAccess()consults$accessand$perm, andtheme_admindeclares neither, so every route answers the same way.The markers have two readers and both only decide whether an entry is drawn:
restrictMenuAccess(), ande_navigation::admin(), which re-checks the key becauseprocessMenuItem()copies every unrecognised key into the rendered nav item.An administrator granted Theme preferences therefore saw one menu entry out of six and could reach the other five, theme upload and the converter included, by typing the URL. The menu told them they could not; the dispatcher had nothing to check.
Deltik's call on #6274 is that
TMPreaches everything the Theme Manager offers, upload,installContentand the converter included, so the markers come out rather than the enforced gates being narrowed to meet them. Entry to the page, the POST handling and the navigation entry all read1|TMPand are untouched.What Changed
e107_admin/theme.php: the'perm'element comes off all six$adminMenuentries,main/mainincluded, since that one only restated the gate on entry to the page. The key is deleted rather than set to'', because the two readers disagree about an empty marker:restrictMenuAccess()treats''as unset and draws the entry, whilee_navigation::admin()passes it togetperms(), which answers false for every administrator whose stored permissions are not exactly0.e107_tests/tests/unit/themeAdminPageTest.php: new. An admin entry point cannot be included from a test, so it readstheme.phpwith the tokenizer and fails on anypermmarker in$adminMenu, with the reasoning in the failure message. It pins the decision rather than modelling the dispatcher: a test that weighed each marker against the page gate would have to know what$modesand the two properties declare, and would tell the reader to delete a marker that was doing real work the day this page gains one.How It Was Tested
The new test reds on all six markers and greens with them gone, proven by a revert run rather than by reading. The unit suite is green on PHP 8.5, and the file parses under PHP 5.6 for the release/v2.3.x twin.
Not run: an acceptance pass watching a
TMP-only administrator load the six routes. The reachability claim below rests on reading the dispatcher, not on a live session.Backwards Compatibility
No route changes hands. Nothing in the dispatch path ever read these markers, so every route that answered before answers now, to exactly the same administrators.
$adminMenuisprotected, so nothing outside the class could have read the markers either.What changes is the menu an administrator holding
1orTMP, and not0, is shown: six entries where there was one. Those five entries were already reachable by URL for that administrator, so the menu now describes what the page does. A main administrator sees no difference.AI Model (Optional)
Claude Opus 5 (claude-opus-5), as e107help.
Checklist