From 6b7b9598adec2f0c67951d50549fe38638101e0a Mon Sep 17 00:00:00 2001 From: Martin Pokorny <89339813+mPokornyETM@users.noreply.github.com> Date: Tue, 31 Jan 2023 01:40:01 +0100 Subject: [PATCH] Eliminate multiplicate code in LockableResourcesRootAction (#451) --- .../LockableResourcesManager.java | 15 ++++ .../actions/LockableResourcesRootAction.java | 90 +++++++++---------- 2 files changed, 58 insertions(+), 47 deletions(-) diff --git a/src/main/java/org/jenkins/plugins/lockableresources/LockableResourcesManager.java b/src/main/java/org/jenkins/plugins/lockableresources/LockableResourcesManager.java index bf6751d8b..d6e9bce7c 100644 --- a/src/main/java/org/jenkins/plugins/lockableresources/LockableResourcesManager.java +++ b/src/main/java/org/jenkins/plugins/lockableresources/LockableResourcesManager.java @@ -43,6 +43,8 @@ import org.jenkins.plugins.lockableresources.queue.QueuedContextStruct; import org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.SecureGroovyScript; import org.jenkinsci.plugins.workflow.steps.StepContext; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; import org.kohsuke.stapler.DataBoundSetter; import org.kohsuke.stapler.StaplerRequest; @@ -610,6 +612,19 @@ public synchronized void unlockNames( save(); } + /** Returns names (IDs) of given *resources*. + */ + @Restricted(NoExternalUse.class) + public static List getResourcesNames(List resources) { + List resourceNames = new ArrayList<>(); + if (resources != null) { + for (LockableResource resource : resources) { + resourceNames.add(resource.getName()); + } + } + return resourceNames; + } + /** @see #getNextQueuedContext(List, List, boolean, QueuedContextStruct) */ @CheckForNull private QueuedContextStruct getNextQueuedContext( diff --git a/src/main/java/org/jenkins/plugins/lockableresources/actions/LockableResourcesRootAction.java b/src/main/java/org/jenkins/plugins/lockableresources/actions/LockableResourcesRootAction.java index 4b88cf648..4510d480f 100644 --- a/src/main/java/org/jenkins/plugins/lockableresources/actions/LockableResourcesRootAction.java +++ b/src/main/java/org/jenkins/plugins/lockableresources/actions/LockableResourcesRootAction.java @@ -164,15 +164,11 @@ public void doUnlock(StaplerRequest req, StaplerResponse rsp) { Jenkins.get().checkPermission(UNLOCK); - String name = req.getParameter("resource"); - LockableResource r = LockableResourcesManager.get().fromName(name); - if (r == null) { - rsp.sendError(404, Messages.error_resourceDoesNotExist(name)); + List resources = this.getResourcesFromRequest(req, rsp); + if (resources == null) { return; } - List resources = new ArrayList<>(); - resources.add(r); LockableResourcesManager.get().unlock(resources, null); rsp.forwardToPreviousPage(req); @@ -184,19 +180,15 @@ public void doReserve(StaplerRequest req, StaplerResponse rsp) { Jenkins.get().checkPermission(RESERVE); - String name = req.getParameter("resource"); - LockableResource r = LockableResourcesManager.get().fromName(name); - if (r == null) { - rsp.sendError(404, Messages.error_resourceDoesNotExist(name)); + List resources = this.getResourcesFromRequest(req, rsp); + if (resources == null) { return; } - List resources = new ArrayList<>(); - resources.add(r); String userName = getUserName(); if (userName != null) { if (!LockableResourcesManager.get().reserve(resources, userName)) { - rsp.sendError(423, Messages.error_resourceAlreadyLocked(name)); + rsp.sendError(423, Messages.error_resourceAlreadyLocked(LockableResourcesManager.getResourcesNames(resources))); return; } } @@ -209,15 +201,11 @@ public void doSteal(StaplerRequest req, StaplerResponse rsp) { Jenkins.get().checkPermission(STEAL); - String name = req.getParameter("resource"); - LockableResource r = LockableResourcesManager.get().fromName(name); - if (r == null) { - rsp.sendError(404, Messages.error_resourceDoesNotExist(name)); + List resources = this.getResourcesFromRequest(req, rsp); + if (resources == null) { return; } - List resources = new ArrayList<>(); - resources.add(r); String userName = getUserName(); if (userName != null) { LockableResourcesManager.get().steal(resources, userName); @@ -239,23 +227,21 @@ public void doReassign(StaplerRequest req, StaplerResponse rsp) throw new AccessDeniedException3(Jenkins.getAuthentication2(), STEAL); } - String name = req.getParameter("resource"); - LockableResource r = LockableResourcesManager.get().fromName(name); - if (r == null) { - rsp.sendError(404, Messages.error_resourceDoesNotExist(name)); + List resources = this.getResourcesFromRequest(req, rsp); + if (resources == null) { return; } - if (userName.equals(r.getReservedBy())) { - // Can not achieve much by re-assigning the - // resource I already hold to myself again, - // that would just burn the compute resources. - // ...unless something catches the event? (TODO?) - return; + for (LockableResource resource : resources) { + if (userName.equals(resource.getReservedBy())) { + // Can not achieve much by re-assigning the + // resource I already hold to myself again, + // that would just burn the compute resources. + // ...unless something catches the event? (TODO?) + return; + } } - List resources = new ArrayList<>(); - resources.add(r); LockableResourcesManager.get().reassign(resources, userName); rsp.forwardToPreviousPage(req); @@ -267,22 +253,20 @@ public void doUnreserve(StaplerRequest req, StaplerResponse rsp) { Jenkins.get().checkPermission(RESERVE); - String name = req.getParameter("resource"); - LockableResource r = LockableResourcesManager.get().fromName(name); - if (r == null) { - rsp.sendError(404, Messages.error_resourceDoesNotExist(name)); + List resources = this.getResourcesFromRequest(req, rsp); + if (resources == null) { return; } String userName = getUserName(); - if ((userName == null || !userName.equals(r.getReservedBy())) - && !Jenkins.get().hasPermission(Jenkins.ADMINISTER) - ) { - throw new AccessDeniedException3(Jenkins.getAuthentication2(), RESERVE); + for (LockableResource resource : resources) { + if ((userName == null || !userName.equals(resource.getReservedBy())) + && !Jenkins.get().hasPermission(Jenkins.ADMINISTER) + ) { + throw new AccessDeniedException3(Jenkins.getAuthentication2(), RESERVE); + } } - List resources = new ArrayList<>(); - resources.add(r); LockableResourcesManager.get().unreserve(resources); rsp.forwardToPreviousPage(req); @@ -295,15 +279,11 @@ public void doReset(StaplerRequest req, StaplerResponse rsp) Jenkins.get().checkPermission(UNLOCK); // Should this also be permitted by "STEAL"?.. - String name = req.getParameter("resource"); - LockableResource r = LockableResourcesManager.get().fromName(name); - if (r == null) { - rsp.sendError(404, Messages.error_resourceDoesNotExist(name)); + List resources = this.getResourcesFromRequest(req, rsp); + if (resources == null) { return; } - List resources = new ArrayList<>(); - resources.add(r); LockableResourcesManager.get().reset(resources); rsp.forwardToPreviousPage(req); @@ -333,4 +313,20 @@ public void doSaveNote(final StaplerRequest req, final StaplerResponse rsp) rsp.forwardToPreviousPage(req); } } + + private List getResourcesFromRequest(final StaplerRequest req, final StaplerResponse rsp) throws IOException, ServletException { + // todo, when you try to improve the API to use multiple resources (a list instead of single one) + // this will be the best place to change it. Probably it will be enough to add a code piece here + // like req.getParameter("resources"); And split the content by some delimiter like ' ' (space) + String name = req.getParameter("resource"); + LockableResource r = LockableResourcesManager.get().fromName(name); + if (r == null) { + rsp.sendError(404, Messages.error_resourceDoesNotExist(name)); + return null; + } + + List resources = new ArrayList<>(); + resources.add(r); + return resources; + } }