Redesign UI

[timja]

TODO:

• Filter box would be nice if the border radius's were appropriate, right now they don't have when others around them get hidden
• ATH - passing now, PR needs merging after this is released
  • Adapt matrix-auth tests to its redesign acceptance-test-harness#2665
• PCT - passed
  • Test matrix-auth redesign bom#6563

Part of jenkinsci/sig-ux#10

Future enhancement: Once wizard functionality is in core we could look to improve the add user and group flow so that users don't get returned to the page and have to select permissions for a new entry they could do that in a flow which I think would be nicer.

Redesigns the plugin interface, see video:

matrix-auth.mov

Screenshots:

Simple

Expanded

Ambiguous

This was originally written by Gen AI with direction from myself. Jan and I tidied up the code and reviewed every line

Testing done

• Tested project and global auth strategies
  • Test at project, folder, system level
• Ambiguous entries
  • migrating them as well

Submitter checklist

• Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
• Ensure that the pull request title represents the desired changelog entry
• Please describe what you did
• Link to relevant issues in GitHub or Jira
• Link to relevant pull requests, esp. upstream and downstream changes
• Ensure you have provided tests that demonstrate the feature works or the issue is fixed

[timja]

cc @janfaracik,

fyi @mawinter69, this is what I meant about doing a redesign of the security plugins so the tables aren't a problem with the sidepanel.

[mawinter69]

In role-strategy that will work for the page where you define the roles. For the assign page, that might require some adjustments. When you have 100 or even 300 roles and want to assign roles to users that will make the opening area below really huge and lead to scrolling.
Currently role-strategy has 2 filters on the assign page so you can make the matrix small and manageable. So something like that is also needed in a new UI.
Another think to consider is that you can't send a checkbox for everything. When you have 300 roles and 300 users you get a form with almost 100k fields. That is over all default limits (10k number of form fields and 200k content size). So what role-strategy does is creating a json that contains only the checked fields as usually the matrix is quite sparse.

[mawinter69]

How does that look when you have selected like 10 different permissions, e.g. when you have more plugins that contribute additional permissions

[timja]

How does that look when you have selected like 10 different permissions, e.g. when you have more plugins that contribute additional permissions

The summary only shows what there is room for an then there is ellipses to show there's more:

[daniel-beck]

From a casual look, this seems great, but would be interesting to get an idea of the scale of some instances' configuration first, as this doesn't look particularly scalable to dozens of users/groups. My hunch is that there are some crazy configurations out there.

It also looks impossible to find who has a given permission, at least if I'm reading the JS right. Right now I highlight a table column and can easily see it, here it looks like I'd need to expand all sections and deal with a much more complex grid.

Re "grant everything" only giving Administer instead of everything, does that work with optional permissions not implied by anything else, like Run/Artifacts?

[mawinter69]

Re "grant everything" only giving Administer instead of everything, does that work with optional permissions not implied by anything else, like Run/Artifacts?

Seems not. Any reason why it was designed like that? Is that maybe a leftover from things like RunScripts? So maybe when you pass null as impliedBy then falling back to admin might make sense?

[daniel-beck]

Any reason why it was designed like that?

I would need to dig.

Is that maybe a leftover from things like RunScripts?

Those came later.

maybe when you pass null as impliedBy then falling back to admin might make sense

Maybe. There may be weird edge cases here as well. Maybe it's just time to clean up those permission declarations; meanwhile the button should do what it says IMO.

[timja]

Re "grant everything" only giving Administer instead of everything, does that work with optional permissions not implied by anything else, like Run/Artifacts?

It works fine from what I can tell - where are you seeing grant admin only? Its just implied don't get checked

matrix-auth-select-all.mov

[daniel-beck]

@timja Great! The existing UI did look different between implied not granted and implied granted, yours didn't seem to.

[timja]

It also looks impossible to find who has a given permission,

Should be possible now since 220a832

I tested it quite a lot and I think its setup well. I've made it so that if you search for a permission that they have as implied they'll come back, e.g search read and administer will show.

matrix-auth-filter.mov

as this doesn't look particularly scalable to dozens of users/groups

By scalable do you think as it takes up a bit more room? Or something else?

[daniel-beck]

By scalable do you think as it takes up a bit more room? Or something else?

Mostly when it comes to getting an "overview" of permissions -- right now, it's a not a great UI, but it's using space efficiently. Expanding all list entries provides the same information in the new UI, and that's just impractical. Filtering for permissions seems like a good addition though (untested), perhaps that's enough.

[timja]

Future enhancement: Once wizard functionality is in core we could look to improve the add user and group flow so that users don't get returned to the page and have to select permissions for a new entry they could do that in a flow which I think would be nicer.

Thinking: could probably just adding it to the dialog that comes up

[daniel-beck]

Overall this looks pretty solid, thanks! I identified a few minor issues so far, and found a few problems:

• Problem: As each (collapsed) row takes more space, and the expanded row takes much more space, it's weirder now than it was in the past that new rows appear at the botton of the list. It is fairly likely to need to scroll to configure a new entry (distance "Add user" button and "Metrics" permissions category in the new entry). Would it be better to add new entries on top? (Related, would then make more sense to sort authenticated and anonymous at the bottom?)
• Bug: Migrating an ambiguous entry does not clear the tooltip and text color of being ambiguous.
• Bug: Migrating an ambiguous "authenticated" entry (and probably "anonymous" too) while filters are live will end up hiding the migrated entry if migrating to a currently filtered out entry.

Other thoughts:

• Why is the maximum width fairly low? Wouldn't it make more sense to use the full width, as the rest of the form does? This may even allow putting permission group titles on the same line as individual permissions.
• The filter field is kinda weird because the text part is names, the button part is permissions
• Should filtering by permissions have "OR" (current behavior) or "AND" semantics?
• Would it make sense to make the "dropdown with checkboxes" a default UI element? Seems a little much custom CSS + JS for me to maintain 😬

The wording should be updated: The description for ambiguous permissions refers to a "table" when there's no table anymore. (Also, did I ever re-read this after writing? The text is terrible. Happy to take this one myself. The plugin uses long obsolete "User ID" terminology cf. recent PR by @janfaracik in core, so I'd need to follow up anyway.)

[mawinter69]

Suggested change

	<j:choose>
	<j:when test="${isChecked}">
	<input type="checkbox" name="[${p.id}]" checked="checked"/>
	</j:when>
	<j:otherwise>
	<input type="checkbox" name="[${p.id}]"/>
	</j:otherwise>
	</j:choose>
	<input type="checkbox" name="[${p.id}]" checked="${isChecked?'checked':null}"/>