[JENKINS-46925] add support for custom folder types. #42

jtnord · 2017-09-18T10:14:04Z

It looks like the code was mostly working with AbstractFolder however the important Action was looking for types of Folder thus a user was not able to add configuration to those types of jobs. This widens the type to AbstractFolder.

This is not binary compatable with old code calling FolderConfigiFileAction, but I could not find any occurrences in the wild.

Any existing configuration should be re-read without issues.

and example of these are jenkins.branch.OrganizationFolder and org.jenkinsci.plugins.workflow.multibranch.WorkflowMultiBranchProject (whose parent is not an OrganisationFolder)

Generally I think this can be simplified to be any AbstractFolder that is not an descendant of a ComputedFolder, which in turn can be simplified to any AbstractFolder that can be configured, which is what is implemented in this PR (this is already taken care of in the WorkflowMultiBranchProject ACL.

Tested manually with an OrganizationFolder and a MultiBranch project that the config files are present in the UI and can be used in the jobs.

@reviewbybees

It looks like the code was mostly working with AbstractFolder however the important Action was looking for types of Folder. This widens the type to AbstractFolder. This is not binary compatable with old code calling FolderConfigiFileAction, but I could not find any occurences in the wild. Any existing configuration should be re-read without issues.

jtnord · 2017-09-18T10:15:43Z

src/main/java/org/jenkinsci/plugins/configfiles/folder/FolderConfigFileAction.java

@@ -96,7 +108,7 @@ public ContentType getContentTypeForProvider(String providerId) {

    @Override
    public HttpResponse doSaveConfig(StaplerRequest req) throws IOException, ServletException {
-        checkPermission(Job.CONFIGURE);
+        checkPermission(Item.CONFIGURE);


unrelated - but it is actually defined on Item not Job...

Ideally file this as a separate PR in general, this + the reorganized imports would have made a nice candidate for this, to keep only the gist here and make this more quickly reviewed/mergeable IMO.

Yeah - but I blame eclipse for sorting the stuff when the new import was added. (I did keep the removing unused imports in a different commit at least :-o )

Eclipse, contrary to IDEA, actually has a view for staging chunks selectively, which anyone should always do with Git :).
Or use git gui ugly but functional OOTB tool.

I [probably need to start using that again - but I tend to code and then build using command line and then commit in the command line cos well Jenkins extensions and eclipse just gives wired errors :-/

Will leave this to the maintaner as their choice if they want me to rewrite history or leave it as is.

jtnord · 2017-09-18T10:15:49Z

src/main/java/org/jenkinsci/plugins/configfiles/folder/FolderConfigFileAction.java

@@ -147,7 +159,7 @@ public void doEditConfig(StaplerRequest req, StaplerResponse rsp, @QueryParamete

    @Override
    public void doAddConfig(StaplerRequest req, StaplerResponse rsp, @QueryParameter("providerId") String providerId, @QueryParameter("configId") String configId) throws IOException, ServletException {
-        checkPermission(Job.CONFIGURE);
+        checkPermission(Item.CONFIGURE);


unrelated - but it is actually defined on Item not Job...

ghost · 2017-09-18T10:20:36Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

amuniz

LGTM

cyrille-leclerc

LGTM

daniel-beck · 2017-09-18T12:22:32Z

src/main/java/org/jenkinsci/plugins/configfiles/folder/FolderConfigFileAction.java

+                SecurityContextHolder.setContext(impersonate);
+            }
+        }
+
        return folder.hasPermission(Item.EXTENDED_READ) ? ConfigFilesManagement.ICON_PATH : null;


Looks like this line needs to make use of hasPerm.

batmat

🐝 LGTM

batmat · 2017-09-18T12:57:15Z

src/main/java/org/jenkinsci/plugins/configfiles/folder/FolderConfigFileAction.java

+import com.cloudbees.hudson.plugins.folder.AbstractFolder;
+
+import jenkins.model.TransientActionFactory;
+import net.sf.json.JSONObject;


Probably a lot of this could have been not staged, and possibly filed as a separate PR. It's hard to see what is really belonging to that PR and what is just like org.kohsuke.stapler.* expansion and so on.

batmat · 2017-09-18T13:00:04Z

src/main/java/org/jenkinsci/plugins/configfiles/folder/FolderConfigFileAction.java

@@ -96,7 +108,7 @@ public ContentType getContentTypeForProvider(String providerId) {

    @Override
    public HttpResponse doSaveConfig(StaplerRequest req) throws IOException, ServletException {
-        checkPermission(Job.CONFIGURE);
+        checkPermission(Item.CONFIGURE);


Ideally file this as a separate PR in general, this + the reorganized imports would have made a nice candidate for this, to keep only the gist here and make this more quickly reviewed/mergeable IMO.

The action was still showing up on projects that could never have configuration added. When you tried to add configuration you would get the User X is missing the Item.Configure permission, but its just not good UX, so hide the action in case System can not configure the thing (ie it can never have config files)

jtnord · 2017-09-18T13:48:35Z

src/main/java/org/jenkinsci/plugins/configfiles/folder/FolderConfigFileAction.java

        return folder;
    }

    @Override
    public String getIconFileName() {
-        return folder.hasPermission(Item.EXTENDED_READ) ? ConfigFilesManagement.ICON_PATH : null;
+        // whilst you may be able to read configuration of a ComputedFolder they are not configurable, so the link should never be shown.
+        boolean hasPerm = folder.hasPermission(Item.EXTENDED_READ);


This code does not work....
I wonder if it would be better to only show it when you have Item.CONFIGURE || (Item.EXTENDED_READ && hasatLeastOneManagedFile())

ref

varmenise · 2017-09-18T14:58:07Z

src/main/java/org/jenkinsci/plugins/configfiles/folder/FolderConfigFileAction.java

+            final SecurityContext impersonate = ACL.impersonate(ACL.SYSTEM);
+            try {
+                hasPerm = folder.hasPermission(Item.CONFIGURE);
+            }


I am not sure I follow this, will not this be always true since you are impersonating as system?

exactly see my above comment.

so why even bothering doing it, can you just check Item.EXTENDED_READ and then hasPerm is true and you show it. Why do you check configure if hasPerm is already true

because you can have extended_read for say a computed folder. but no user can ever have configure for that computed folder - so there can never be any managed files for it - so this was the case I want to not show the Action for. But does not seem to be easily possible so the use case needs to be adapted slightly to only show the action if you can "create new files, or there are some files and you have permission to see their configuration"

oleg-nenashev · 2017-09-18T15:29:28Z

src/main/java/org/jenkinsci/plugins/configfiles/folder/FolderConfigFileAction.java

        this.folder = folder;
    }

-    public Folder getFolder() {
+    public AbstractFolder<?> getFolder() {


formally it is a breaking change. But it is reasonable in this case, not the first time. Folders plugin just has confusing class naming

called out in the main PR description.

This is not binary compatible with old code calling FolderConfigiFileAction, but I could not find any occurrences in the wild.

The previous attempt did not work as SYSTEM always has configure. This approach uses a compromise, only show the action if you have configure permission or, there is one managed file AND you have extended read permission

jtnord · 2017-09-19T17:53:22Z

@daniel-beck issues resolved.

varmenise · 2017-09-20T09:05:24Z

🐝

jtnord · 2017-09-20T09:30:58Z

@reviewbybees done.

jtnord · 2017-09-20T09:32:24Z

@imod can you take a look, if you are short on time to make a release if the change looks ok I am happy to do the release for you.

imod · 2017-09-20T14:43:00Z

👍
@jtnord I'll have some time to do the release by end of this week

imod · 2017-09-22T06:44:28Z

@jtnord release done

jtnord · 2017-09-22T13:25:38Z

@imod thank you.

jtnord added 2 commits September 18, 2017 10:40

Remove unused imports.

f88df4c

jtnord commented Sep 18, 2017

View reviewed changes

alvarolobato approved these changes Sep 18, 2017

View reviewed changes

amuniz approved these changes Sep 18, 2017

View reviewed changes

cyrille-leclerc approved these changes Sep 18, 2017

View reviewed changes

daniel-beck reviewed Sep 18, 2017

View reviewed changes

batmat approved these changes Sep 18, 2017

View reviewed changes

jtnord force-pushed the JENKINS-46925 branch from b6420ee to 21fa629 Compare September 18, 2017 13:22

jtnord commented Sep 18, 2017

View reviewed changes

varmenise reviewed Sep 18, 2017

View reviewed changes

oleg-nenashev reviewed Sep 18, 2017

View reviewed changes

amuniz approved these changes Sep 20, 2017

View reviewed changes

imod requested review from imod and removed request for imod September 20, 2017 14:44

oleg-nenashev approved these changes Sep 20, 2017

View reviewed changes

imod merged commit f593d53 into jenkinsci:master Sep 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-46925] add support for custom folder types. #42

[JENKINS-46925] add support for custom folder types. #42

jtnord commented Sep 18, 2017 •

edited

jtnord Sep 18, 2017 •

edited

batmat Sep 18, 2017

jtnord Sep 18, 2017

batmat Sep 19, 2017

jtnord Sep 19, 2017

jtnord Sep 18, 2017 •

edited

ghost commented Sep 18, 2017

amuniz left a comment

cyrille-leclerc left a comment

daniel-beck Sep 18, 2017

batmat left a comment

batmat Sep 18, 2017

batmat Sep 18, 2017

jtnord Sep 18, 2017 •

edited

varmenise Sep 18, 2017

jtnord Sep 19, 2017

varmenise Sep 19, 2017

jtnord Sep 19, 2017

oleg-nenashev Sep 18, 2017

jtnord Sep 19, 2017 •

edited

jtnord commented Sep 19, 2017

varmenise commented Sep 20, 2017

jtnord commented Sep 20, 2017

jtnord commented Sep 20, 2017

imod commented Sep 20, 2017

imod commented Sep 22, 2017

jtnord commented Sep 22, 2017

Navigation Menu

[JENKINS-46925] add support for custom folder types. #42

[JENKINS-46925] add support for custom folder types. #42

Conversation

jtnord commented Sep 18, 2017 • edited

jtnord Sep 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtnord Sep 18, 2017 • edited

Choose a reason for hiding this comment

ghost commented Sep 18, 2017

amuniz left a comment

Choose a reason for hiding this comment

cyrille-leclerc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

batmat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtnord Sep 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtnord Sep 19, 2017 • edited

Choose a reason for hiding this comment

jtnord commented Sep 19, 2017

varmenise commented Sep 20, 2017

jtnord commented Sep 20, 2017

jtnord commented Sep 20, 2017

imod commented Sep 20, 2017

imod commented Sep 22, 2017

jtnord commented Sep 22, 2017

jtnord commented Sep 18, 2017 •

edited

jtnord Sep 18, 2017 •

edited

jtnord Sep 18, 2017 •

edited

jtnord Sep 18, 2017 •

edited

jtnord Sep 19, 2017 •

edited