Hello, Superficial read => I imagine you should use Item.CONFIGURE and not the generic Permission.CONFIGURE.
Wadeck On Thursday, April 28, 2022 at 1:12:47 AM UTC+2 [email protected] wrote: > Hi, > > I added the Jenkins security scan to my plugin on GitHub and resolved all > the issues.Or, so I thought. > > One of the main classes of reported issues (aside from adding @POST to > pretty much all Stapler doXXX methods) was the "no access check done". > > While most of the methods I have (mostly doCheck and DoFill...Items) are > very simple (no file/net/... access), I figured it would not hurt to add a > basic check either. > > For the bulk of the methods, typically involved in configuring a step, I > did the following: > - add an @AncestorInPath Item item parameter to the method > - add if (item != null) item.checkPermission(Permission.CONFIGURE); near > the top of the method. > > This seemed to make sense - you'd need to have access to the Configure > menu item to see the interface that calls those methods anyway > > For those bits related to the global tool setup, I simply use Jenkins.get > ().checkPermission(Jenkins.MANAGE); instead. Again, this seems to make > sense, given the tool setup lives among the Manage Jenkins options. > > However, when I set up users and activate the matrix strategy, then a user > who has the Job/Configure permission will get an Unauthorized error under > one of the drop-downs, saying they do not have the N/A/GenericConfigure > permission. > > Does this mean I should be testing for FreeStyleProject.CONFIGURE instead > (these Permission things seem rather hard to discover, I must say)? Or will > that prevent using the same UI/methods in other contexts (e.g. what > permissions make the pipeline syntax generator available? that uses the > same interface as the freestyle project configuration)? > > Am I right in using checkPermission, or should I be using > checkAnyPermission to check for multiples (say Permission.CONFIGURE, > FreeStyleProject,CONFIGURE, Jenkins.MANAGE)? > Or should I be using has(Any)Permission() and simply fail silently (i.e. > return OK validation, or filling no items)? > > If the suggestion is "just don't do any checking if there's no real need" > then I would really appreciate a way to be able to declare that in code so > that the security scan will not raise an issue for it. E.g. a @Unsecured > annotation or an empty Jenkins.noPermissionsNeeded() method that will > satisfy the "Stapler methods must check access rights" requirement. > Come to that, if a method doesn't really need @POST, is there any harm > (e.g. a perf hit) in adding it? If so, maybe a @GET/@NoPOST annotation to > say "this does not need @POST, stop bugging me" would be nice too. > Security scans become less useful if they mostly produce false positives. > > Thanks in advance for any guidance. > -- You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/9ef11edb-e1d3-4226-9086-34097166fa03n%40googlegroups.com.
