[JENKINS-32797] Break the catch clause contents of Jenkins.getTarget(… #2652
Conversation
…) out into a separate, publicly accessible function. This will allow plugins (particularly authentication plugins that override the normal authentication process) to determine if authentication is not required for a particular path by calling isPathUnprotected(restOfPath).
| /** | ||
| * Test a path to see if it does not require authentication | ||
| * @param restOfPath the URI, excluding the Jenkins root URI and query string | ||
| * @return true if the path is unprotected |
| throw e; | ||
| } | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Test a path to see if it does not require authentication |
There was a problem hiding this comment.
UnprotectedRootActions can easily implement authentication requirements themselves e.g. based on parameters. This Javadoc and method name is misleading.
There was a problem hiding this comment.
Any recommendations on a better name? I could call it "isPathAccessibleAnonymously", "isAnonymousAccessAllowed", "isPathAccessibleWithoutAuthentication", or invert the return value and call it "doesPathRequireAuthentication".
The only issue I see (with my current knowledge) with the first two is that they are also misleading, since we are not checking if anonymous access is allowed - they are instead checks for paths where access is allowed to any user, and the check normally only occurs if the user does not have permissions to the path they are trying to access, and the secondary use I am seeing it for is for checking first to see if the user can access a path before any authentication attempts take place.
Should the comment instead say "Test a path to see if it is accessible regardless of authentication status"?
There was a problem hiding this comment.
I'm having a hard time coming up with a name that is simple and not potentially misleading...
The basic idea it needs to convey is that the paths this function checks for are outside of the normal security settings for Jenkins, and are generally accessible regardless of authentication status, but we are not checking anything that can be controlled by the normal security settings.
isPathAlwaysReadable seems to be pretty close to the truth for the paths I know of, but I'm not sure how true this may be for UnprotectedRootActions that implement authentication requirements themselves, and seems too close to ALWAYS_READABLE_PATHS for something that checks more than just that collection.
Even better: isPathAccessControlExempt, since that seems to capture the point of UnprotectedRootActions and ALWAYS_READABLE_PATHS, with low chance of confusion; a longer and more clear name: isPathExemptFromContainerAccessControl - infers that there may still be some access control on it, but that access control is independent from the containers' access control.
Not looking for someone else to do the work, instead looking for some guidance before I commit changes to this pull request.
There was a problem hiding this comment.
isSubjectToMandatoryReadPermissionCheck perhaps, trying to capture the difference between the Jenkins issued permission check, and what UnprotectedRootActions can opt in to?
There was a problem hiding this comment.
I may not be understanding everything in the code there, but it seems to me that you may have meant isNotSubjectToMandatoryReadPermissionCheck?
That would be clear and understandable, since they are paths that have read access allowed even when the Jenkins issued read permission check fails.
There was a problem hiding this comment.
I'd invert its meaning to match that name, of course.
There was a problem hiding this comment.
Okay, so I'm not understanding something properly then, and may need a better explanation.
isSubjectToMandatoryReadPermissionCheck (to me) implies that the function would return true for any path that should go through the standard Jenkins permissions checks, which would be the opposite of the logic currently in this pull request. That is why I thought you might have meant isNotSubjectToMandatoryReadPermissionCheck, since to the understanding I have had, that would match the current logic.
There was a problem hiding this comment.
For an example of something I may be misunderstanding, looking at the current function summary, it is completely and absolutely ambiguous as to what it returns, and therefore a horrible summary - looking at the @return true if the path is unprotected comment is necessary to clarify that it returns true if the container access control can be bypassed. However, you may have been reading the summary only, and therefore thinking "this returns true if we need to run the read permission check". I don't know if this is the case, because I don't know your perspective.
There was a problem hiding this comment.
No, I'm saying the behavior should be changed to match the proposed name, if accepted.
This way, true is unambiguous, and false includes the potential for non-mandatory permission checks.
This will prevent potential problems for readers of statements like !isNotWhatever -- better to have isWhatever.
There was a problem hiding this comment.
Yes, that works; I wasn't seeing the name and thinking "switch the logic to match that name" before, hence my confusion. Probably just because I have been looking at this late at night after long days.
isPathUnprotected is misleading, and the Javadoc was worse. isSubjectToMandatoryReadPermissionCheck is a much better name, and the return value is reversed to match the name,
oleg-nenashev
added
the
ready-for-merge
…) out into a separate, publicly accessible function.
This will allow plugins (particularly authentication plugins that override the normal authentication process) to determine if authentication is not required for a particular path by calling isPathUnprotected(restOfPath).