Re-design UI by timja · Pull Request #749 · jenkinsci/role-strategy-plugin

timja · 2026-04-03T22:17:31Z

TODO:

For later:

Item based selection rather than just regex
User / group autocompletion / validation on add - might look at it in this

role-strategy.mov

Testing done

Extensively manually tested including with different personas:

Admin
SystemRead
Item roles admin
Agent roles admin
Item and Agent roles admin

Verified behaviour is correct across all of these.
I intend to create some playwright tests as well as there's 0 UI coverage at all in this repo (or in ATH), but that will likely be for another PR although I may stack it on this to get it going

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

This reverts commit bbfc645.

timja · 2026-04-04T13:42:37Z

There's probably some changes needed but its ready for a look from a design PoV.

Code not ready for review yet
Documentation probably needs updating
Once we're happy with the design, data saving etc, I can look at ATH / PCT.

cc @mawinter69

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 37 out of 37 changed files in this pull request and generated 8 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-17T07:57:45Z

+      rbas.doAddTemplate(templateName, permIds, overwrite);
+    }
+
+    rsp.sendRedirect(redirectUrl);


handleTemplateSubmit calls RoleBasedAuthorizationStrategy#doAddTemplate(...), which is an HTTP handler that may write an error response (e.g. 400 on duplicate template name) via Stapler.getCurrentResponse2().sendError(...). This method then unconditionally redirects, which can mask the error or attempt to redirect after the response is committed.

Consider moving the template creation logic into RoleStrategyConfig (or a shared non-HTTP helper) so you can return a proper error to the dialog, and only redirect on success.

Suggested change

rsp.sendRedirect(redirectUrl);

if (!rsp.isCommitted()) {

rsp.sendRedirect(redirectUrl);

}

Copilot · 2026-04-17T07:57:45Z

+                        class="jenkins-button jenkins-button--tertiary rsp-card__action rsp-template-edit"
+                        tooltip="${%Edit template}"
+                        data-type="dialog-opener"
+                        data-dialog-url="${rootURL}/manage/role-strategy/edit-template-dialog?templateName=${h.escape(template.name)}">


The dialog URL embeds template.name in a query string using h.escape(...), which HTML-escapes but does not URL-encode. Template names containing spaces, &, ?, etc. will produce a broken URL (and can lead to confusing request parameters).

URL-encode the query parameter value (e.g., with a URL-encoding helper) rather than HTML-escaping it.

Suggested change

data-dialog-url="${rootURL}/manage/role-strategy/edit-template-dialog?templateName=${h.escape(template.name)}">

data-dialog-url="${rootURL}/manage/role-strategy/edit-template-dialog?templateName=${h.urlEncode(template.name)}">

Copilot · 2026-04-17T07:57:46Z

+                      <button type="button" class="jenkins-button jenkins-button--tertiary rsp-card__action rsp-card__edit"
+                              tooltip="${%Edit role}"
+                              data-type="dialog-opener"
+                              data-dialog-url="${rootURL}/manage/role-strategy/edit-role-dialog?roleName=${h.escape(role.name)}&amp;scope=${roleType}">


The dialog URL embeds role.name in the query string using h.escape(...), which HTML-escapes but does not URL-encode. Role names containing spaces or reserved URL characters will break the generated URL.

URL-encode query parameter values instead of HTML-escaping them.

Suggested change

data-dialog-url="${rootURL}/manage/role-strategy/edit-role-dialog?roleName=${h.escape(role.name)}&scope=${roleType}">

data-dialog-url="${rootURL}/manage/role-strategy/edit-role-dialog?roleName=${h.urlEncode(role.name)}&scope=${roleType}">

Copilot · 2026-04-17T07:57:46Z

+        card.remove();
+        rspUpdateCardBorders();
+        tplDelete(name)
+          .then(() => {
+            if (
+              document.querySelectorAll("#rsp-template-cards .rsp-card")
+                .length === 0
+            ) {
+              window.location.reload();
+            } else {
+              notificationBar.show(
+                `Template "${name}" deleted`,
+                notificationBar.SUCCESS,
+              );
+            }
+          })
+          .catch((err) => {
+            notificationBar.show(
+              "Failed to save: " + err.message,
+              notificationBar.ERROR,
+            );
+          });


The delete flow removes the template card from the DOM before the server-side delete succeeds. If the tplDelete(...) request fails, the UI stays out of sync (the template appears deleted but still exists server-side) and the user must reload to recover.

Consider only removing the card after a successful delete, or implement a rollback path on failure (re-insert the card / reload on error).

Copilot · 2026-04-17T07:57:46Z

+   * @param role The role to check
+   * @param sid The user or group name
+   * @param type "USER" or "GROUP"
+   * @return true if the sid is assigned to the role
+   */
+  public boolean isAssigned(@NonNull Role role, @NonNull String sid, @NonNull String type) {
+    Set<PermissionEntry> entries = this.grantedRoles.get(role);
+    if (entries == null) {
+      return false;
+    }
+    AuthorizationType authType = "GROUP".equals(type) ? AuthorizationType.GROUP : AuthorizationType.USER;
+    return entries.contains(new PermissionEntry(authType, sid))
+        || entries.contains(new PermissionEntry(AuthorizationType.EITHER, sid));


RoleMap#isAssigned Javadoc says type is only "USER" or "GROUP", but callers in this PR also pass "EITHER". The implementation treats any non-"GROUP" as USER and then checks USER or EITHER entries, so it works, but the documentation is now inaccurate.

Update the Javadoc to include "EITHER" (or clarify how it is handled).

Copilot · 2026-04-17T07:57:47Z

+  @CheckForNull
+  private static JSONObject getSubmittedFormOrRedirect(
+      StaplerRequest2 req, StaplerResponse2 rsp, String redirectUrl) throws IOException {
    req.setCharacterEncoding("UTF-8");
-    JSONObject json = req.getSubmittedForm();
-    JSONObject rolesMapping;
-    if (json.has("submit")) {
-      String rm = json.getString("rolesMapping");
-      rolesMapping = JSONObject.fromObject(rm);
-    } else {
-      rolesMapping = json.getJSONObject("rolesMapping");
+    try {
+      return req.getSubmittedForm();
+    } catch (Exception e) {
+      rsp.sendRedirect(redirectUrl);
+      return null;
    }


getSubmittedFormOrRedirect swallows all getSubmittedForm() failures and issues a redirect. For endpoints like doAddRoleSubmit / doAssignRoleSubmit / template submits, an invalid or empty POST will likely become a 302 (often followed to 200 by the client) rather than a clear 400 error. This also makes the submit endpoints behave more like navigational pages than form APIs.

Consider returning a 400 (sendError) with a useful message when the form JSON is missing/invalid (and only redirect for genuine navigation flows), so both dialogs and tests can reliably detect bad submissions.

Copilot · 2026-04-17T07:57:47Z

+var escapeHTML = function (unsafe) {
+  return unsafe.replace(/[&<>"']/g, function(m) {
+    switch (m) {
+      case "&":
+        return "&amp;";
+      case "<":
+        return "&lt;";
+      case '"':
+        return "&quot;";
+      default:
+        return "&#039;";
+    }


escapeHTML currently maps the > character to ' (apostrophe) because there is no explicit case '>'. This will corrupt output containing > and also fails to properly HTML-escape >.

Add an explicit case '>' -> '>' (and keep the apostrophe mapping only for ').

Copilot · 2026-04-17T07:57:47Z

+    if (permsJson != null) {
+      for (String rawKey : permsJson.keySet()) {
+        if (permsJson.optBoolean(rawKey, false)) {
+          Permission p = Permission.fromId(rawKey);


collectPermissionsFromFlat calls Permission.fromId(rawKey) directly, but the checkbox names in the edit-role dialog are of the form name="[${p.id}]". That means the JSON keys are likely bracketed, so Permission.fromId will return null and editing a role can unintentionally clear all permissions.

Strip the surrounding brackets (reuse stripBrackets) before calling Permission.fromId, consistent with collectPermissionsFromScoped/collectPermissionIds.

Suggested change

Permission p = Permission.fromId(rawKey);

String permId = stripBrackets(rawKey);

Permission p = Permission.fromId(permId);

Co-authored-by: Tim Jacomb <21194782+timja@users.noreply.github.com>

timja · 2026-04-21T07:56:17Z

@@ -0,0 +1,201 @@
+#!/bin/bash


Useful script for performance testing

mawinter69

Check boxes in the dialogs are touching each other.

Maybe add some gap

.rsp-assign-dialog__group {
  display: flex;
  flex-direction: column;
  gap: 0.25rem;
}

mawinter69 · 2026-04-25T19:19:33Z

+        </div>
+      </f:entry>
+
+      <f:bottomButtonBar>
