On Thu, Apr 28, 2022 at 1:12 AM Tim Van Holder <tim.vanhol...@gmail.com> wrote:
> 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. > Just in case, please note that Manage is a lesser permission than Administer. Most stuff in Jenkins is (still) requiring Jenkins.ADMINISTER, and the UI needs to be adapted too for options to be available to users with "only" Manage. Some options are also unsafe to make available this way and it's not always obviou. See https://github.com/jenkinsci/jep/tree/master/jep/223 > 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)? > Yes, that's correct (well, I'd use Item.CONFIGURE but it's the same thing). Generic permissions can be *granted*, but should never be *checked*. A generic permission is any permission never shown in the matrix-auth table. Note that some permissions may be missing from there, like Item/Artifacts or Overall/Manage, which are optional and disabled-by-default permissions, and show up once enabled. > Am I right in using checkPermission, or should I be using > checkAnyPermission to check for multiples (say Permission.CONFIGURE, > FreeStyleProject,CONFIGURE, Jenkins.MANAGE)? > Probably not, because Overall/Manage is granted globally (i.e. you'd check it on Jenkins.get() ), while Item/Configure is granted on an Item, which you'd get from AncestorInPath in the usual Descriptor form validation methods. But there are cases where the check/hasAnyPermission methods make sense. JEP-223/224 define global permissions that make some previously admin-only stuff available to users with lesser permissions, and so core has some code to show UI to users that have any of Overall/Manage or Overall/SystemRead. > Or should I be using has(Any)Permission() and simply fail silently (i.e. > return OK validation, or filling no items)? > Yes, this is frequently the better UX especially for doCheck/doTest/doFill methods. One related complication for permission checks is for Pipeline-compatible steps (which you mentioned before). If someone opens the Snippet Generator, there may not be an @AncestorInPath Item, or have the permission you expect, but you'd still want the user to be able to generate *something*, because ultimately it ends up in the Jenkinsfile anyway. Fully JCasC configured instances come to mind here, where no actual user may be an admin or allowed to configure jobs. No need to return real data though if you don't have to. > 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. > This is a known limitation, tracked in https://github.com/jenkins-infra/jenkins-codeql/issues/4 Meanwhile, you can mark findings as invalid through the GitHub UI (unfortunately without an explanation). > Come to that, if a method doesn't really need @POST, is there any harm > (e.g. a perf hit) in adding it? > If the UI element sends POST anyway, there's no drawback to adding it (besides it being more annoying to trigger manually e.g. during development -- choose RequirePOST if that's a likely use case). > 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. > It's very difficult to distinguish between methods that need protections (permission checks or POST requirement) from those that don't. I attempted to exclude some obvious cases of harmless methods, but it can always be improved! See https://github.com/jenkins-infra/jenkins-codeql/blob/main/jenkins/java/ql/src/declarations/NonTrivialInvocation.qll and feel free to suggest additions (via PR or issues) -- 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 jenkinsci-dev+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAMo7PtL%3DRVP2%3DAeyu%3DK-HkTNZ2L5XRypMFFa6u9bKhuGVrMARA%40mail.gmail.com.
