[JENKINS-67981] handle any AuthenticationException #1322
Conversation
When processing the changelog, if a looked user is disabled in LDAP the
git plugin would throw an `UncheckedExecutionException` with the cause
being `DisabledException`. The unhandled exception would cause a build
failure whenever processing a commit that is determinated to be from a
disabled user.
Jenkins `UserDetailsCache.loadUserByUsername()` receives an
`ExecutionException` which wraps the `AuthenticationException`. The
method inspects the cause, handles instances of
`UsernameNotFoundException` which it throws but ignores any other
`AuthenticationException`.
This introduces a `User` getter to handle any `AuthenticationException`
and throws it instead of the `ExecutionException`. Callers can thus:
try {
user = getUser("someOne");
} catch (AuthenticationException e) {
return User.getUnknown();
}
// Do something with "user"
I have added a test to ensure a `DisabledException` does yield an
unknown user. It only test a single code path though.
Reference: https://phabricator.wikimedia.org/T315897
|
I'd like to have confirmation from users that the change resolves their issue. I can't test it. https://ci.jenkins.io/job/Plugins/job/git-plugin/job/PR-1322/lastSuccessfulBuild/ shows the most recent build. Copy the URL of the HPI file from that page and paste it into the "Advanced" tab of the Jenkins plugin manager.
Thanks! |
There was a problem hiding this comment.
This looks good to me. Thanks for the investigation and for the contribution.
The Spring security AuthenticationException shows it has subclasses like:
Those all seem very reasonable things that should result in User.getUnknown() for these types of cases where the result from the call is used to present informational content. None of these uses are for authentication, so returning "unkwown" is much better than failing with an exception.
Thank you for investigating and confirming we could return an I have build the plugin at 13a61a0 + this pull request and deployed it on our affected instance. That has fixed the issue and the affected job completed without errors. Hurrah! |
|
I'm proceeding with the merge in hopes that others will test it before release. Even if not, we have the report from @hashar that the changes resolves the issue for his case. |
I tried Build 1015 and was not having any issues. 👍 |
|
Thanks @SaschaSchwarz ! The change is now merged to the master branch. It will be included in the next release of the git plugin. |
|
Thank you @SaschaSchwarz for the verification and thank you @MarkEWaite for the review/merge! |
JENKINS-67981 - handle any AuthenticationException
When processing the changelog, if a looked user is disabled in LDAP the git plugin would throw an
UncheckedExecutionExceptionwith the cause beingDisabledException. The unhandled exception would cause a buildfailure whenever processing a commit that is determinated to be from a disabled user.
Jenkins
UserDetailsCache.loadUserByUsername()receives anExecutionExceptionwhich wraps theAuthenticationException. The method inspects the cause, handles instances ofUsernameNotFoundExceptionwhich it throws but ignores any otherAuthenticationException.This introduces a
Usergetter to handle anyAuthenticationExceptionand throws it instead of theExecutionException. Callers can thus:I have added a test to ensure a
DisabledExceptiondoes yield an unknown user. It only test a single code path though.This pull request supersedes #1233 on which I analyzed the
AuthenticationExceptionis wrapped in anExecutionException. Compared to that other pull request, I introduce a getter to avoid copy pasting try/catch statements.References:
Checklist
Types of changes