Skip to content

Commit

Permalink
Add empty authorities by default
Browse files Browse the repository at this point in the history 
Closes gh-12533
  • Loading branch information
@stillya @sjohnr
stillya authored and sjohnr committed Jan 30, 2023
1 parent 6abbdd3 commit 3229bfa
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 2 deletions.
7 changes: 5 additions & 2 deletions core/src/main/java/org/springframework/security/core/userdetails/User.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ public static final class UserBuilder {

private String password;

private List<GrantedAuthority> authorities;
private List<GrantedAuthority> authorities = new ArrayList<>();

private boolean accountExpired;

Expand Down Expand Up @@ -427,6 +427,7 @@ public UserBuilder roles(String... roles) {
* @see #roles(String...)
*/
public UserBuilder authorities(GrantedAuthority... authorities) {
Assert.notNull(authorities, "authorities cannot be null");
return authorities(Arrays.asList(authorities));
}

Expand All @@ -439,7 +440,8 @@ public UserBuilder authorities(GrantedAuthority... authorities) {
* @see #roles(String...)
*/
public UserBuilder authorities(Collection<? extends GrantedAuthority> authorities) {
this.authorities = new ArrayList<>(authorities);
Assert.notNull(authorities, "authorities cannot be null");

This comment has been minimized.

Sign in to view

Copy link
@dkorotych

dkorotych Jun 8, 2023

Contributor

Broken backwards compatibility. There is currently no way to reset the this.authorities list.
For example, the code will not work correctly,

final User.UserBuilder builder = User.withUserDetails(userDetails);
if (CollectionUtils.isEmpty(authorities)) {
  builder.authorities(Collections.emptyList());
} else {
  builder.authorities(authorities.toArray(String[]::new));
}
userDetailsManager.updateUser(builder.build());

it will always merge new and old authorities, but the previous version will overwrite them

To solve this problem and return the past behavior, it is enough to call
this.authorities.clear();

This comment has been minimized.

Sign in to view

Copy link
@sjohnr

sjohnr Jun 8, 2023

Member

Thanks for bringing this up, @dkorotych! Apologies, I missed that this was changed from overwrite to add. Would you mind opening a new issue for this?

In the meantime, you can of course create a new builder yourself to work around the problem. For example:

final User.UserBuilder builder = withUserDetails(userDetails);
if (CollectionUtils.isEmpty(authorities)) {
	builder.authorities(Collections.emptyList());
} else {
	builder.authorities(authorities.toArray(String[]::new));
}
userDetailsManager.updateUser(builder.build());

with a customized helper method:

private static User.UserBuilder withUserDetails(UserDetails userDetails) {
	return User.withUsername(userDetails.getUsername())
			.password(userDetails.getPassword())
			.accountExpired(!userDetails.isAccountNonExpired())
			.accountLocked(!userDetails.isAccountNonLocked())
			// .authorities(userDetails.getAuthorities())
			.credentialsExpired(!userDetails.isCredentialsNonExpired());
}
this.authorities.addAll(authorities);
return this;
}

Expand All @@ -452,6 +454,7 @@ public UserBuilder authorities(Collection<? extends GrantedAuthority> authoritie
* @see #roles(String...)
*/
public UserBuilder authorities(String... authorities) {
Assert.notNull(authorities, "authorities cannot be null");
return authorities(AuthorityUtils.createAuthorityList(authorities));
}

Expand Down
30 changes: 30 additions & 0 deletions core/src/test/java/org/springframework/security/core/userdetails/UserTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@

import java.io.ByteArrayOutputStream;
import java.io.ObjectOutputStream;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
Expand All @@ -37,6 +39,7 @@
* Tests {@link User}.
*
* @author Ben Alex
* @author Ilya Starchenko
*/
public class UserTests {

Expand Down Expand Up @@ -68,6 +71,33 @@ public void testNoArgConstructorDoesntExist() {
.isThrownBy(() -> User.class.getDeclaredConstructor((Class[]) null));
}

@Test
public void testBuildUserWithNoAuthorities() {
UserDetails user = User.builder().username("user").password("password").build();
assertThat(user.getAuthorities()).isEmpty();
}

@Test
public void testNullWithinUserAuthoritiesIsRejected() {
assertThatIllegalArgumentException().isThrownBy(() -> User.builder().username("user").password("password")
.authorities((Collection<? extends GrantedAuthority>) null).build());
List<GrantedAuthority> authorities = new ArrayList<>();
authorities.add(null);
authorities.add(null);
assertThatIllegalArgumentException().isThrownBy(
() -> User.builder().username("user").password("password").authorities(authorities).build());

assertThatIllegalArgumentException().isThrownBy(() -> User.builder().username("user").password("password")
.authorities((GrantedAuthority[]) null).build());
assertThatIllegalArgumentException().isThrownBy(() -> User.builder().username("user").password("password")
.authorities(new GrantedAuthority[] { null, null }).build());

assertThatIllegalArgumentException().isThrownBy(
() -> User.builder().username("user").password("password").authorities((String[]) null).build());
assertThatIllegalArgumentException().isThrownBy(() -> User.builder().username("user").password("password")
.authorities(new String[] { null, null }).build());
}

@Test
public void testNullValuesRejected() {
assertThatIllegalArgumentException().isThrownBy(() -> new User(null, "koala", true, true, true, true, ROLE_12));
Expand Down

0 comments on commit 3229bfa

Please sign in to comment.