Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-34426] Fix to handle SECURITY-243 #34

Merged
merged 1 commit into from
May 19, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,7 @@ private List<SocketInfo> obtainLDAPServers(String domainName) throws Authenticat
public UserDetails retrieveUser(final String username, final String password, final String domainName, final List<SocketInfo> ldapServers) {
UserDetails userDetails;
try {
final ActiveDirectoryUserDetail[] cacheMiss = new ActiveDirectoryUserDetail[1];
userDetails = userCache.get(username, new Callable<UserDetails>() {
public UserDetails call() throws AuthenticationException {
DirContext context;
Expand Down Expand Up @@ -312,11 +313,12 @@ public UserDetails call() throws AuthenticationException {
Set<GrantedAuthority> groups = resolveGroups(domainDN, dnFormatted, context);
groups.add(SecurityRealm.AUTHENTICATED_AUTHORITY);

return new ActiveDirectoryUserDetail(username, password, true, true, true, true, groups.toArray(new GrantedAuthority[groups.size()]),
cacheMiss[0] = new ActiveDirectoryUserDetail(username, password, true, true, true, true, groups.toArray(new GrantedAuthority[groups.size()]),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this does not look thread safe to me. cacheMiss is a class level field...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I asked for a method scoped field

getStringAttribute(user, "displayName"),
getStringAttribute(user, "mail"),
getStringAttribute(user, "telephoneNumber")
).updateUserInfo();
);
return cacheMiss[0];
} catch (NamingException e) {
if (anonymousBind && e.getMessage().contains("successful bind must be completed") && e.getMessage().contains("000004DC")) {
// sometimes (or always?) anonymous bind itself will succeed but the actual query will fail.
Expand All @@ -331,6 +333,9 @@ public UserDetails call() throws AuthenticationException {
}
}
});
if (cacheMiss[0] != null) {
cacheMiss[0].updateUserInfo();
}
} catch (UncheckedExecutionException e) {
Throwable t = e.getCause();
if (t instanceof AuthenticationException) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
package hudson.plugins.active_directory;

import java.io.IOException;
import java.lang.reflect.InvocationTargetException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashSet;
Expand Down Expand Up @@ -177,6 +178,19 @@ public static long getSerialVersionUID() {
* Gets the corresponding {@link hudson.model.User} object.
*/
public hudson.model.User getJenkinsUser() {
try { // TODO 1.651.2+ remove reflection
return (hudson.model.User) hudson.model.User.class.getMethod("getById", String.class, boolean.class).invoke(null, getUsername(), true);
} catch (InvocationTargetException e) {
if (e.getCause() instanceof RuntimeException) {
throw (RuntimeException)e.getCause();
}
// Only RuntimeException is expected
LOGGER.log(Level.WARNING, String.format("There was a problem obtaining the Jenkins user %s by Id", getUsername()), e);
} catch (NoSuchMethodException e) {
// fine, older baseline
} catch (Exception e) { // unexpected
LOGGER.log(Level.WARNING, String.format("There was a problem obtaining the Jenkins user %s by Id", getUsername()), e);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe InvocationTargetException should be caught to unwind any legitimate exception getById may throw.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andresrc Are you sure it is necessary? getById should not throw any exception. So if .invoke fails you will just go tot the catch of Exception to record it. Is it not fine?

return hudson.model.User.get(getUsername());
}

Expand Down