Skip to content

fix(admin-ui): disable inline editing when the edit route is denied - #6454

Open
e107help[bot] wants to merge 1 commit into
masterfrom
e107help/6443
Open

e107help[bot] wants to merge 1 commit into
masterfrom
e107help/6443

Conversation

@e107help

@e107help e107help Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Why

e_admin_form_ui::getList() means to take two things away from a user the dispatcher refuses the edit route to: the edit button, and inline editing on the columns that offer it. It has only ever managed the first.

if(!$controller->getDispatcher()->hasRouteAccess($editRoute))
{
$fields['options']['readParms']['editClass'] = e_UC_NOBODY; // display the edit button.
foreach($options[$id]['fields'] as $k=>$v) // disable inline editing.
{
$fields[$k]['inline'] = false;
}
}

$options is a plain local, initialised empty at line 8244, and its only write is at line 8322, thirty-nine lines below the loop that reads it. Nothing in between touches it, and it is never a reference and never a global, so the loop body has never run on any request. On PHP 8 the dead read raises an undefined array key, an array offset on null and a foreach over a non-array, which is how it surfaced.

The visible cost is a list that offers inline-edit controls on the very route the screen's own permission map has just refused. The news screen says what it intends in as many words, at newspost.php lines 64-75: 'main/edit' => 'H|H1', // edit button and inline editing in list mode.

What Changed

The loop reads the variable the block means. $fields is the copy of the controller's declarations that line 8282 is already mutating one line above, and it is what ends up at $options[$id]['fields'] further down. One word.

treePrefix() moves with it. A tree list's child icon comes from one of two passes: the inline pass emits it as inlineParms['pre'] at form_handler.php line 6609 when the column is editable, and read mode draws it itself when the column is not. Read mode decided which by asking the controller whether the column was inline, at admin_ui.php line 8008, while the denial only ever reached getList()'s local copy. With the loop finally running, the two sources disagree and a denied user on a tree list would have lost the indentation icon along with the editor. Read mode now asks the question the renderer answers, noedit and the resolved editable parm together, so it draws the icon exactly when the inline pass is not going to.

Two limits worth stating rather than leaving to be discovered:

  • inline is an alias that sets the editable read parm, resolved at form_handler.php lines 5605-5607. A column that declares readParms editable=1 directly, as faqs/admin_config.php line 100 does, keeps its widget after this change, because clearing an alias cannot reach a declaration that never used it. That leaves the faqs list open, and it is the bundled screen with both halves: its dispatcher keys main/edit to an admin-set userclass at lines 72-80, and faq_question and faq_order spell editable as a read parm at line 220 and line 233. Closing it needs a decision this change does not take: blanking an editable key inside a readParms string means either normalising that string to an array, which changes the shape every other consumer of it sees, or a new way to say "not editable" that both spellings resolve through.
  • Removing a control is not by itself a refusal of anything. Whether the inline save route should carry a check of its own is a separate question with a different shape, and it is recorded for @Deltik rather than argued here.

How It Was Tested

New: e107_tests/tests/unit/adminUiInlineEditDenialTest.php with its fixture, six tests.

  • Three drive getList() through a stub dispatcher that refuses one named route and capture the field declarations it hands the renderer: inline goes false on every column when main/edit is refused, stays true when it is not, and the third builds its expectation from the permitted render, applies the only two changes a denial is allowed to make, and asserts identity. That last one is the over-reach guard, because the loop now walks columns that never declared inline, including the synthetic options column.
  • Three then call e_form::renderValue() on the field array getList() actually produced, so the alias the fix depends on is exercised rather than assumed: a denied tree column keeps its icon and loses its editor, a permitted one keeps both, and a column that declares inline editing and then sets noedit keeps its icon, which is the one declaration neither pass used to draw.
  • Red proof: the whole test file reds against the unfixed source and greens with it, both confirmed by reverting e107_handlers/admin_ui.php and restoring it.
  • The unit suite in full on PHP 8.5: 2735 tests, 25899 assertions, 6 skipped, green.

Backwards Compatibility

A permitted list is unchanged on every path. The loop runs only inside the denial branch, so nothing a permitted user sees moves.

For a denied user, the change is the point: controls the permission map already refused stop being drawn. Writing false over an absent inline key is inert, because every widget site tests the resolved parm with !empty() rather than distinguishing absent from false, so the columns that never offered inline editing render as before.

One rendering change is worth naming because it is not a denial. A tree column that spells editable as a read parm without noedit used to draw its child icon twice, once from read mode and once from the inline pass; it now draws it once. No bundled screen declares a tree prefix column that way, so this is a change in the contract treePrefix() offers third-party screens rather than anything visible in core.

The forum plugin carries the same read-mode shape in its own copy, with the icon code stranded under an unconditional return. That is #6453, filed separately because it touches a core plugin and needs a version bump.

One more copy worth knowing about, which this fix does not reach and does not need to yet: links.php lines 1030-1074 overrides getList() wholesale and carries none of the four route checks, so it hands the renderer the controller's fields untouched. links_admin declares no $access and no array $perm, and the adminMenu perms it does declare are menu-item perms that hasRouteAccess() never reads, so nothing there can be refused today. It is drift rather than a hole, but anyone adding an access map to that screen should not assume the base class covers them.

AI Model

Claude Opus 5 (claude-opus-5) as e107help.

Checklist

  • Tests added, red against the unfixed source
  • Unit suite green
  • No change to what a permitted user is served
  • Maintainer decides whether the readParms editable=1 spelling should be cleared too, and whether the inline save route wants a check of its own

Fixes #6443

e_admin_form_ui::getList() hides the edit button when the dispatcher refuses
main/edit, then walks $options[$id]['fields'] to clear the inline flag on every
column. That local is initialised empty thirty-nine lines above its only write,
so the loop has never had an element to iterate: a list went on offering
inline-edit widgets on the very route the screen's own permission map had just
refused. On PHP 8 the dead read also raised an undefined array key, an array
offset on null and a foreach over a non-array.

The variable the block means is $fields, the copy of the controller's field
declarations that the line above it is already mutating, and that is what ends
up at $options[$id]['fields'] further down.

Writing false over an absent inline key changes nothing, because every widget
site tests the resolved editable parm with !empty(), so the synthetic options
column is unharmed by being in the loop. A column that declares readParms
editable=1 directly keeps its widget: inline is an alias for that parm, and
clearing the alias cannot reach a declaration that never used it. That leaves
the faqs list open, which has both halves, keying main/edit to an admin-set
userclass and spelling its question and order columns as read parms. Closing it
needs a decision about how to blank an editable key inside a readParms string
without changing the shape every consumer of that string sees, and this change
does not take it.

treePrefix() moves with it. A tree list's child icon is emitted by the inline
pass as inlineParms['pre'] when the column is editable, and by treePrefix()
itself when it is not, but treePrefix() asked the controller whether the column
was inline while the denial only ever reached getList()'s local copy. With the
loop running the two disagree, and a denied user on a tree list would have lost
the indentation icon along with the editor. Read mode now asks the question the
renderer answers, noedit and the resolved editable parm together, so it draws
the icon exactly when the inline pass is not going to. A permitted list is
unchanged, and a column that spells editable as a readParm stops drawing the
icon twice.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The loop that disables inline editing for a denied user has never run

1 participant