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.

Sign up for GitHub

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-70035] Use User.getById() instead of User.get() #194

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[JENKINS-70035] Use User.getById() instead of User.get() #194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/main/java/hudson/security/LDAPSecurityRealm.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -779,7 +779,7 @@ public UserDetails updateUserDetails(UserDetails userDetails, @CheckForNull Ldap
}

public DelegatedLdapUserDetails updateUserDetails(LdapUserDetails d, @CheckForNull LdapUserSearch ldapUserSearch) {
hudson.model.User u = hudson.model.User.get(fixUsername(d.getUsername()), true, Collections.emptyMap());
hudson.model.User u = hudson.model.User.getById(fixUsername(d.getUsername()), true);
LDAPConfiguration configuration = getConfigurationFor(d);
String displayNameAttributeName;
String mailAddressAttributeName;
Expand Down
15 changes: 15 additions & 0 deletions src/test/java/hudson/security/LDAPEmbeddedTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -655,4 +655,19 @@ public void userValidityAttributes() throws Exception {
assertThrows(AccountExpiredException.class, () -> User.getById("bender", true).impersonate2());
assertThrows(FailingHttpStatusCodeException.class, () -> r.createWebClient().withBasicApiToken("amy").goTo(""));
}

@Test
@Issue("JENKINS-70035")
@LDAPSchema(ldif = "planetexpress", id = "planetexpress", dn = "dc=planetexpress,dc=com")
public void do_not_create_two_users_when_userId_have_special_chars() throws Exception {
LDAPSecurityRealm realm =
new LDAPSecurityRealm(ads.getUrl(), "dc=planetexpress,dc=com", null, null, null, null, null,
"uid=admin,ou=system", Secret.fromString("pass"), false, false, null,
null, "cn", "mail", null, null);
r.jenkins.setSecurityRealm(realm);
r.configRoundtrip();
String content = r.createWebClient().login("bender<", "bender").goTo("asynchPeople").getBody().getTextContent();
assertThat(content, containsString("bender<"));
assertThat(content, not(containsString("bender_")));
}
}
4 changes: 2 additions & 2 deletions src/test/java/hudson/security/LdapMultiEmbeddedTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ public void when_first_is_wrong_and_ignorable_and_login_on_second_then_login() t
allOf(containsString(FAILED_COMMUNICATION_WITH_LDAP_SERVER),
containsString(WILL_TRY_NEXT_CONFIGURATION),
containsString(planetExpress.getUrl())),
CoreMatchers.<Throwable>instanceOf(UserMayOrMayNotExistException2.class)));
CoreMatchers.<Throwable>instanceOf(AuthenticationServiceException.class)));
Copy link
Member

@dwnusbaum dwnusbaum Nov 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, but I do not remember if the specific exception type here is important (I would assume it does not matter though, and I doubt that the multi-server mode is used very frequently anyways). What is the full stack trace in the logs before and after the change? Maybe the only difference is that there is a new outermost exception?

Copy link
Member

Choose a reason for hiding this comment

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

UserMayOrMayNotExistException2 seems important as it is used to say "I can not tell you that this users exists. or not" and as such can allow the use of a token (wheras another excpetion would be treated as the form - this user does not exist).

At the very least I think some manual testing with user impersonation needs to occur when the server is down (before and after this change)

}

@Test
Expand All @@ -351,7 +351,7 @@ public void when_first_is_unavailable_and_ignorable_and_login_on_second_then_log
allOf(containsString(FAILED_COMMUNICATION_WITH_LDAP_SERVER),
containsString(WILL_TRY_NEXT_CONFIGURATION),
containsString(INVALID_URL_PREFIX + planetExpress.getPort())),
CoreMatchers.<Throwable>instanceOf(UserMayOrMayNotExistException2.class)));
CoreMatchers.<Throwable>instanceOf(AuthenticationServiceException.class)));
}

@Test
Expand Down
Loading