[JENKINS-62218] Disable Default View select for read-only folder config viewers - #747
Conversation
f:advanced collapses the Health metrics section behind a click-to-expand toggle regardless of permissions. A user with Item.READ/EXTENDED_READ but not Item.CONFIGURE has no way to interact with the form anyway, so hiding already-configured health metrics behind an extra click only makes the read-only view less informative, contrary to the read-only rendering convention described in JENKINS-12548 (every advanced section should be expanded out of the box for read-only browsing). When readOnlyMode is true, render the health metrics hetero-list directly instead of wrapping it in f:advanced. Behavior for users with CONFIGURE is unchanged.
…ring Covers both cases: a viewer with Item.READ/EXTENDED_READ but not Item.CONFIGURE sees the configured health metric directly without an Advanced toggle, while a user with CONFIGURE still gets the collapsible Advanced section as before.
The Appearance section's Default View <select name="primaryView"> was
hand-rolled HTML with no readOnlyMode awareness, unlike the rest of the
configure page. A user with Item.READ + Item.EXTENDED_READ but without
Item.CONFIGURE could interact with and change this dropdown client-side
even though they could not save, since it lacked a disabled attribute.
Add disabled="${readOnlyMode ? 'true' : null}" to the select, matching
the idiom used elsewhere in Jenkins core (e.g. lib/form/dropdownList.jelly)
and consistent with how the rest of this page already respects
readOnlyMode.
…escriptor fragments f:dropdownDescriptorSelector renders the config fragment of the currently selected Views Tab Bar / Icon descriptor inline, but every other candidate descriptor's fragment is rendered lazily via l:renderOnDemand. That closure only snapshots jelly variables named in the tag's capture attribute, so the outer readOnlyMode variable set in configure.jelly was invisible to those lazy fragments, leaving any field they render fully editable for a read-only viewer (Item.READ+Item.EXTENDED_READ without Item.CONFIGURE) if the outer, already-disabled <select> is bypassed client-side or the render-on-demand endpoint is hit directly. Add capture="readOnlyMode" to both dropdownDescriptorSelector call sites (viewsTabBar and icon). Add a regression test that registers a second fake FolderIcon descriptor via @TestExtension so the Icon dropdown has a non-selected entry to lazily render, fetches that lazy fragment through the same stapler-bound-proxy protocol the browser's own JavaScript uses, and asserts a read-only viewer sees the read-only placeholder instead of an editable <input>. Verified the test fails without the capture attribute and passes with it.
jtnord
left a comment
There was a problem hiding this comment.
Health metrics was wrapped in f:advanced, Jenkins core's collapsible "Advanced..." toggle. That toggle renders identically for every user regardless of permissions, so a read-only viewer has to click it just to discover whether any health metric is even configured.
Seems appropriate, not a bug. Otherwise a config page can be full of loads of things you need to scroll through.
If this is an issue (which I do not beleive it is) it should not be fixed here, but in Jenkins core so that the behaviour is the same for all consumers of f:advanced in all of the Jenkins UI.
Views Tab Bar / Icon (f:dropdownDescriptorSelector) render their currently selected descriptor's config fragment inline, where readOnlyMode is visible normally — but any other descriptor's fragment is deferred via Jenkins core's l:renderOnDemand, which only exposes variables explicitly named in the tag's capture attribute. Neither call declared capture="readOnlyMode", so an extra config field from a third-party Icon/TabBar descriptor would come back fully editable to a read-only viewer once loaded.
Would appear to be a generic bug in Jenkins core and would affect all users of dropdownDescriptorSelector across the project so should be fixed in Jenkins core if so?
| <j:if test="${it.folderViews.primaryModifiable and views.size()>1}"> | ||
| <f:entry title="${%Default View}" field="defaultView"> | ||
| <div class="jenkins-select"> | ||
| <select class="jenkins-select__input" name="primaryView"> |
There was a problem hiding this comment.
better to adapt this to f:select?
There was a problem hiding this comment.
Sooo... Choosing f:select would require refactoring the control according to the AJAX-fill model; I decided that this change was better, since it is a pattern already used in the core
| oneEach="true" hasHeader="true" honorOrder="true"/> | ||
| </f:block> | ||
| </f:advanced> | ||
| <j:choose> |
There was a problem hiding this comment.
if advanced blocks are not supposed to be expanded by default in read only mode then the change should go in Jenkins core so that all configuration items behave the same.
There was a problem hiding this comment.
The strategy followed to implement these changes in other Epic tasks has been to implement them internally within the plugins—I understand this is to keep the components as generic primitives without any kind of permission context (such as read-only). Implementing the change in the core would mean altering all the components that use it, with the problems that could entail...
I also understand your point—not including it in the core would lead to UX inconsistencies, but that’s more of a design decision that the project maintainers should make.
There was a problem hiding this comment.
I also understand your point—not including it in the core would lead to UX inconsistencies, but that’s more of a design decision that the project maintainers should make.
FTR I think the current behaviour in this regard is correct. To me a readonly view should display the same as a read/write view with the exception of the disabled state of components, and the lack of save/apply buttons.
There was a problem hiding this comment.
@kohsuke was the creator of JENKINS-12548 ; perhaps he can provide some more information. I'm just trying to fix the reported issue. If the decision is to leave it as is, perhaps the issue should be closed.
There was a problem hiding this comment.
Kohsuke is no longer active in this project. JEP-224 does not explicitly state that the layout should or should not change only
Many views have been modified to hide information that is not relevent to read only users.
Jelly tags in the Jenkins core have been modified to display a read only state, i.e. text instead of inputs, icons for checkboxes
However I maintain that nothing should be changing advanced configuration to standard configuration just because it becomes read only, it is either always advanced, or never advanced), but will summon @timja as the author of the JEP for a second opinion.
There was a problem hiding this comment.
Advanced configuration does not change anything here, if it was previously advanced then it should still be under that as part of read only change.
(Advanced can be evaluated as part of separate UX review, it should only be used for non standard or uncommon configurations, as it will hide options and users may not see them)
There was a problem hiding this comment.
This means, this statement in the epic : ''every "advanced" sections should be expanded out of the box.''
Is not longer valid? Just trying to confirm if we are only expecting the fix on the Default view select or there is something else to fix that im not noticing.
Once I get the confirmation I can push the revert of the advanced sections.
There was a problem hiding this comment.
This means, this statement in the epic : ''every "advanced" sections should be expanded out of the box.''
Is not longer valid?
Correct.
Just trying to confirm if we are only expecting the fix on the Default view select or there is something else to fix that im not noticing.
that should be all that is needed
There was a problem hiding this comment.
Revert applied.
It would also be good to clarify the incorrect statement in the epic. I'll continue working on the remaining tasks in that epic, so I'll keep that in mind, but if any other contributors join, they might run into the same problem.
| <j:choose> | ||
| <j:when test="${descriptor.tabBarConfigurable}"> | ||
| <f:dropdownDescriptorSelector title="${%Views Tab Bar}" field="viewsTabBar"/> | ||
| <f:dropdownDescriptorSelector title="${%Views Tab Bar}" field="viewsTabBar" capture="readOnlyMode"/> |
There was a problem hiding this comment.
this looks strange.
if this is needed why is this not more prevelant elsewhere. e.g. any project that defines an SCM -> https://github.com/jenkinsci/jenkins/blob/master/core/src/main/resources/lib/hudson/project/config-scm.jelly#L45-L48 would appear to have issues?
There was a problem hiding this comment.
It looks like so... I've reverted the change in the plugin. I'll open an issue in the core later to try to fix it there.
That was a good catch—thanks for letting me know.
There was a problem hiding this comment.
@jtnord I've oppened jenkinsci/jenkins#27167
…descriptors The underlying gap (l:renderOnDemand's RenderOnDemandClosure only exposes variables named in `capture`, so readOnlyMode set in an ancestor scope is invisible to lazily-rendered descriptor fragments) is not specific to this plugin: Jenkins core's own hudson/model/Job/configure.jelly sets readOnlyMode the same way and includes config-scm.jelly, whose dropdownDescriptorSelector for scmCheckoutStrategy has the identical gap. Per review feedback, fixing this locally in cloudbees-folder-plugin wouldn't address the same issue on every other Job configuration page, so this should be fixed in Jenkins core instead. Reverts the capture="readOnlyMode" attribute additions and the associated FakeCaptureFolderIcon test. This is a revert of e6eed4f and 88edadd; the Default View select fix (c9aed80) and Health metrics fix (0bc0fda) are unaffected.
…ly viewers Per review feedback from jtnord and timja, advanced configuration sections should render the same regardless of readOnlyMode: if a section was previously collapsed behind f:advanced, it should stay that way in read-only mode too, rather than auto-expanding just because the viewer lacks CONFIGURE. Only the Default View select's disabled attribute (c9aed80) remains from this PR's original scope. This is a revert of 0bc0fda and 67b5274.
[JENKINS-62218] Disable Default View select for read-only folder config viewers
Problem
AbstractFolder/configure.jellysets areadOnlyModevariable (<j:set var="readOnlyMode" value="${!it.hasPermission(it.CONFIGURE)}" />) and already hides the Save/Apply buttons for a user withoutItem/Configure. Most fields inheritreadOnlyModeautomatically through Jenkins core's form tags, but the Default View selector is a hand-rolled<select>(notf:select), so it never inheritedreadOnlyModeat all — a read-only viewer could change the selection client-side (it just couldn't be saved, since Save/Apply are already correctly hidden).This is part of the JENKINS-12548 epic (read-only system configuration browsing / JEP-224).
Note on prior scope: earlier revisions of this PR also touched Health metrics (auto-expanding it out of
f:advancedfor read-only viewers) and addedcapture="readOnlyMode"to the Views Tab Bar / Iconf:dropdownDescriptorSelectorcalls. Per review discussion:capture="readOnlyMode"gap is a generic Jenkins core bug (affects everydropdownDescriptorSelectorconsumer, e.g.config-scm.jelly'sscmCheckoutStrategy), so it was reverted here and pursued instead as jenkinsci/jenkins#27167.readOnlyMode— if a section was previously collapsed behindf:advanced, it should stay that way in read-only mode too. That change (and its test) has been reverted.This PR's scope is now just the Default View select fix.
Changes
In
src/main/resources/com/cloudbees/hudson/plugins/folder/AbstractFolder/configure.jelly:disabled="${readOnlyMode ? 'true' : null}"to the<select name="primaryView">, matching the idiom used by Jenkins core's ownf:dropdownList(and by core's ownGlobalDefaultViewConfiguration/config.jelly, which solves the identical "pick a default view" problem the same way).Screenshots
Default View — Viewer
Verified on a local
mvn hpi:runinstance with a Matrix-Authorization-Strategyviewer(Item.READ+Item.EXTENDED_READ+View.READ, noItem.CONFIGURE) against a folder with 2 views:Before the fix, the select had no
disabledattribute and its value could be changed via a normalchangeevent (confirmed via script: setting.valueand dispatchingchangesucceeded). After the fix,disabledistrueand the control no longer accepts input.Admin, for reference (no regression — still fully editable):
Testing
mvn hpi:runinstance, comparing an admin against a read-only viewer on the same folder's configure page (screenshots above), including live DOM/interactivity checks (disabledattribute state, attemptedchangeevents) for the Default View select.Checklist
Proposed changelog entries