From eb9afd1e445072d9f6dbed1d3c2fe5b21504e8b0 Mon Sep 17 00:00:00 2001 From: Markus Winter Date: Tue, 9 Apr 2024 20:26:09 +0200 Subject: [PATCH 1/3] [JENKINS-72988] validate displayname only against items in same ItemGroup The check if a displayname collides with an existing job name or displayname was always performing the check against the root. So if one wanted to use a displayname for a job inside a folder the check would wrongly fail if the name is used by a something at root level and not fail when it is used by another item in the folder --- .../java/hudson/model/AbstractProject.java | 2 +- core/src/main/java/jenkins/model/Jenkins.java | 40 ++++++++++++++++--- .../test/java/jenkins/model/JenkinsTest.java | 20 +++++----- 3 files changed, 45 insertions(+), 17 deletions(-) diff --git a/core/src/main/java/hudson/model/AbstractProject.java b/core/src/main/java/hudson/model/AbstractProject.java index c554ef813440..2681d5274378 100644 --- a/core/src/main/java/hudson/model/AbstractProject.java +++ b/core/src/main/java/hudson/model/AbstractProject.java @@ -1942,7 +1942,7 @@ public boolean isApplicable(Descriptor descriptor) { @Restricted(DoNotUse.class) public FormValidation doCheckDisplayNameOrNull(@AncestorInPath AbstractProject project, @QueryParameter String value) { - return Jenkins.get().doCheckDisplayName(value, project.getName()); + return Jenkins.get().doCheckDisplayName(value, project); } @Restricted(DoNotUse.class) diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java index 77e6f69ec875..d2c79cd3365f 100644 --- a/core/src/main/java/jenkins/model/Jenkins.java +++ b/core/src/main/java/jenkins/model/Jenkins.java @@ -5274,8 +5274,9 @@ public View getStaplerFallback() { * job that the user is configuring though to prevent a validation warning * if the user sets the displayName to what it currently is. */ - boolean isDisplayNameUnique(String displayName, String currentJobName) { - Collection itemCollection = items.values(); + boolean isDisplayNameUnique(ItemGroup itemGroup, String displayName, String currentJobName) { + + Collection itemCollection = itemGroup.getAllItems(TopLevelItem.class); // if there are a lot of projects, we'll have to store their // display names in a HashSet or something for a quick check @@ -5299,8 +5300,8 @@ else if (displayName.equals(item.getDisplayName())) { * @param name The name to test * @param currentJobName The name of the job that the user is configuring */ - boolean isNameUnique(String name, String currentJobName) { - Item item = getItem(name); + boolean isNameUnique(ItemGroup itemGroup, String name, String currentJobName) { + Item item = itemGroup.getItem(name); if (null == item) { // the candidate name didn't return any items so the name is unique @@ -5322,17 +5323,44 @@ else if (item.getName().equals(currentJobName)) { * existing display names or project names * @param displayName The display name to test * @param jobName The name of the job the user is configuring + * + * @deprecated use {@link Jenkins#doCheckDisplayName(String, Item)} */ + @Deprecated public FormValidation doCheckDisplayName(@QueryParameter String displayName, @QueryParameter String jobName) { displayName = displayName.trim(); LOGGER.fine(() -> "Current job name is " + jobName); - if (!isNameUnique(displayName, jobName)) { + if (!isNameUnique(this, displayName, jobName)) { + return FormValidation.warning(Messages.Jenkins_CheckDisplayName_NameNotUniqueWarning(displayName)); + } + else if (!isDisplayNameUnique(this, displayName, jobName)) { + return FormValidation.warning(Messages.Jenkins_CheckDisplayName_DisplayNameNotUniqueWarning(displayName)); + } + else { + return FormValidation.ok(); + } + } + + /** + * Checks to see if the candidate displayName collides with any + * existing display names or project names in the items parent group + * @param displayName The display name to test + * @param item The item to check for duplicates + */ + public FormValidation doCheckDisplayName(String displayName, + Item item) { + displayName = displayName.trim(); + String jobName = item.getName(); + + LOGGER.fine(() -> "Current job name is " + jobName); + + if (!isNameUnique(item.getParent(), displayName, jobName)) { return FormValidation.warning(Messages.Jenkins_CheckDisplayName_NameNotUniqueWarning(displayName)); } - else if (!isDisplayNameUnique(displayName, jobName)) { + else if (!isDisplayNameUnique(item.getParent(), displayName, jobName)) { return FormValidation.warning(Messages.Jenkins_CheckDisplayName_DisplayNameNotUniqueWarning(displayName)); } else { diff --git a/test/src/test/java/jenkins/model/JenkinsTest.java b/test/src/test/java/jenkins/model/JenkinsTest.java index a8d846c5d79b..873969205407 100644 --- a/test/src/test/java/jenkins/model/JenkinsTest.java +++ b/test/src/test/java/jenkins/model/JenkinsTest.java @@ -203,8 +203,8 @@ public void testIsDisplayNameUniqueTrue() throws Exception { p.setDisplayName("displayName"); Jenkins jenkins = Jenkins.get(); - assertTrue(jenkins.isDisplayNameUnique("displayName1", curJobName)); - assertTrue(jenkins.isDisplayNameUnique(jobName, curJobName)); + assertTrue(jenkins.isDisplayNameUnique(jenkins, "displayName1", curJobName)); + assertTrue(jenkins.isDisplayNameUnique(jenkins, jobName, curJobName)); } @Test @@ -220,7 +220,7 @@ public void testIsDisplayNameUniqueFalse() throws Exception { p.setDisplayName(displayName); Jenkins jenkins = Jenkins.get(); - assertFalse(jenkins.isDisplayNameUnique(displayName, curJobName)); + assertFalse(jenkins.isDisplayNameUnique(jenkins, displayName, curJobName)); } @Test @@ -233,7 +233,7 @@ public void testIsDisplayNameUniqueSameAsCurrentJob() throws Exception { Jenkins jenkins = Jenkins.get(); // should be true as we don't test against the current job - assertTrue(jenkins.isDisplayNameUnique(displayName, curJobName)); + assertTrue(jenkins.isDisplayNameUnique(jenkins, displayName, curJobName)); } @Test @@ -244,7 +244,7 @@ public void testIsNameUniqueTrue() throws Exception { j.createFreeStyleProject(jobName); Jenkins jenkins = Jenkins.get(); - assertTrue(jenkins.isNameUnique("jobName1", curJobName)); + assertTrue(jenkins.isNameUnique(jenkins, "jobName1", curJobName)); } @Test @@ -255,7 +255,7 @@ public void testIsNameUniqueFalse() throws Exception { j.createFreeStyleProject(jobName); Jenkins jenkins = Jenkins.get(); - assertFalse(jenkins.isNameUnique(jobName, curJobName)); + assertFalse(jenkins.isNameUnique(jenkins, jobName, curJobName)); } @Test @@ -267,7 +267,7 @@ public void testIsNameUniqueSameAsCurrentJob() throws Exception { Jenkins jenkins = Jenkins.get(); // true because we don't test against the current job - assertTrue(jenkins.isNameUnique(curJobName, curJobName)); + assertTrue(jenkins.isNameUnique(jenkins, curJobName, curJobName)); } @Test @@ -281,7 +281,7 @@ public void testDoCheckDisplayNameUnique() throws Exception { p.setDisplayName("displayName"); Jenkins jenkins = Jenkins.get(); - FormValidation v = jenkins.doCheckDisplayName("1displayName", curJobName); + FormValidation v = jenkins.doCheckDisplayName("1displayName", curProject); assertEquals(FormValidation.ok(), v); } @@ -297,7 +297,7 @@ public void testDoCheckDisplayNameSameAsDisplayName() throws Exception { p.setDisplayName(displayName); Jenkins jenkins = Jenkins.get(); - FormValidation v = jenkins.doCheckDisplayName(displayName, curJobName); + FormValidation v = jenkins.doCheckDisplayName(displayName, curProject); assertEquals(FormValidation.Kind.WARNING, v.kind); } @@ -313,7 +313,7 @@ public void testDoCheckDisplayNameSameAsJobName() throws Exception { p.setDisplayName(displayName); Jenkins jenkins = Jenkins.get(); - FormValidation v = jenkins.doCheckDisplayName(jobName, curJobName); + FormValidation v = jenkins.doCheckDisplayName(jobName, curProject); assertEquals(FormValidation.Kind.WARNING, v.kind); } From 0e06d65651c386f0bab4455f1aad22e7e649e4c8 Mon Sep 17 00:00:00 2001 From: Markus Winter Date: Tue, 9 Apr 2024 21:22:03 +0200 Subject: [PATCH 2/3] adjust method name to avoid conflict with old implementations --- core/src/main/java/hudson/model/AbstractProject.java | 5 ----- core/src/main/java/jenkins/model/Jenkins.java | 9 +++++---- test/src/test/java/jenkins/model/JenkinsTest.java | 6 +++--- 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/core/src/main/java/hudson/model/AbstractProject.java b/core/src/main/java/hudson/model/AbstractProject.java index 2681d5274378..cceff9c42b19 100644 --- a/core/src/main/java/hudson/model/AbstractProject.java +++ b/core/src/main/java/hudson/model/AbstractProject.java @@ -1940,11 +1940,6 @@ public boolean isApplicable(Descriptor descriptor) { return true; } - @Restricted(DoNotUse.class) - public FormValidation doCheckDisplayNameOrNull(@AncestorInPath AbstractProject project, @QueryParameter String value) { - return Jenkins.get().doCheckDisplayName(value, project); - } - @Restricted(DoNotUse.class) public FormValidation doCheckAssignedLabelString(@AncestorInPath AbstractProject project, @QueryParameter String value) { diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java index d2c79cd3365f..0808d2e7ce7f 100644 --- a/core/src/main/java/jenkins/model/Jenkins.java +++ b/core/src/main/java/jenkins/model/Jenkins.java @@ -5276,7 +5276,7 @@ public View getStaplerFallback() { */ boolean isDisplayNameUnique(ItemGroup itemGroup, String displayName, String currentJobName) { - Collection itemCollection = itemGroup.getAllItems(TopLevelItem.class); + Collection itemCollection = (Collection) itemGroup.getItems(t -> t instanceof TopLevelItem); // if there are a lot of projects, we'll have to store their // display names in a HashSet or something for a quick check @@ -5324,7 +5324,7 @@ else if (item.getName().equals(currentJobName)) { * @param displayName The display name to test * @param jobName The name of the job the user is configuring * - * @deprecated use {@link Jenkins#doCheckDisplayName(String, Item)} + * @deprecated use {@link TopLevelItemDescriptor#doCheckDisplayNameOrNull(TopLevelItem, String)} */ @Deprecated public FormValidation doCheckDisplayName(@QueryParameter String displayName, @@ -5350,8 +5350,9 @@ else if (!isDisplayNameUnique(this, displayName, jobName)) { * @param displayName The display name to test * @param item The item to check for duplicates */ - public FormValidation doCheckDisplayName(String displayName, - Item item) { + @Restricted(NoExternalUse.class) + public FormValidation checkDisplayName(String displayName, + TopLevelItem item) { displayName = displayName.trim(); String jobName = item.getName(); diff --git a/test/src/test/java/jenkins/model/JenkinsTest.java b/test/src/test/java/jenkins/model/JenkinsTest.java index 873969205407..c44b12cdd037 100644 --- a/test/src/test/java/jenkins/model/JenkinsTest.java +++ b/test/src/test/java/jenkins/model/JenkinsTest.java @@ -281,7 +281,7 @@ public void testDoCheckDisplayNameUnique() throws Exception { p.setDisplayName("displayName"); Jenkins jenkins = Jenkins.get(); - FormValidation v = jenkins.doCheckDisplayName("1displayName", curProject); + FormValidation v = jenkins.checkDisplayName("1displayName", curProject); assertEquals(FormValidation.ok(), v); } @@ -297,7 +297,7 @@ public void testDoCheckDisplayNameSameAsDisplayName() throws Exception { p.setDisplayName(displayName); Jenkins jenkins = Jenkins.get(); - FormValidation v = jenkins.doCheckDisplayName(displayName, curProject); + FormValidation v = jenkins.checkDisplayName(displayName, curProject); assertEquals(FormValidation.Kind.WARNING, v.kind); } @@ -313,7 +313,7 @@ public void testDoCheckDisplayNameSameAsJobName() throws Exception { p.setDisplayName(displayName); Jenkins jenkins = Jenkins.get(); - FormValidation v = jenkins.doCheckDisplayName(jobName, curProject); + FormValidation v = jenkins.checkDisplayName(jobName, curProject); assertEquals(FormValidation.Kind.WARNING, v.kind); } From ee50b59a727495c1ecaad427238c0024b3c801fd Mon Sep 17 00:00:00 2001 From: Markus Winter Date: Fri, 30 Aug 2024 15:37:55 +0200 Subject: [PATCH 3/3] call new method in TopLevelItemDescriptor --- core/src/main/java/hudson/model/TopLevelItemDescriptor.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/hudson/model/TopLevelItemDescriptor.java b/core/src/main/java/hudson/model/TopLevelItemDescriptor.java index d84e4a684f22..baf7436d3d48 100644 --- a/core/src/main/java/hudson/model/TopLevelItemDescriptor.java +++ b/core/src/main/java/hudson/model/TopLevelItemDescriptor.java @@ -293,6 +293,6 @@ public static ExtensionList all() { @Restricted(NoExternalUse.class) public FormValidation doCheckDisplayNameOrNull(@AncestorInPath TopLevelItem item, @QueryParameter String value) { - return Jenkins.get().doCheckDisplayName(value, item.getName()); + return Jenkins.get().checkDisplayName(value, item); } }