From c33ccac8f6ada16178b90a905e6192980462a582 Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Thu, 2 Feb 2017 17:59:17 +0100 Subject: [PATCH] SONAR-8715 User builder for NewUser --- .../UserIdentityAuthenticator.java | 5 +- .../java/org/sonar/server/user/NewUser.java | 76 +++++++--- .../sonar/server/user/ws/CreateAction.java | 4 +- .../org/sonar/server/user/NewUserTest.java | 31 +++- .../sonar/server/user/UserUpdaterTest.java | 141 +++++++++++------- .../user/ws/ChangePasswordActionTest.java | 10 +- 6 files changed, 175 insertions(+), 92 deletions(-) diff --git a/server/sonar-server/src/main/java/org/sonar/server/authentication/UserIdentityAuthenticator.java b/server/sonar-server/src/main/java/org/sonar/server/authentication/UserIdentityAuthenticator.java index 8ad01f7f37a..17ae634cecf 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/authentication/UserIdentityAuthenticator.java +++ b/server/sonar-server/src/main/java/org/sonar/server/authentication/UserIdentityAuthenticator.java @@ -106,11 +106,12 @@ public class UserIdentityAuthenticator { } String userLogin = user.getLogin(); - userUpdater.create(dbSession, NewUser.create() + userUpdater.create(dbSession, NewUser.builder() .setLogin(userLogin) .setEmail(user.getEmail()) .setName(user.getName()) - .setExternalIdentity(new ExternalIdentity(provider.getKey(), user.getProviderLogin()))); + .setExternalIdentity(new ExternalIdentity(provider.getKey(), user.getProviderLogin())) + .build()); UserDto newUser = dbClient.userDao().selectOrFailByLogin(dbSession, userLogin); syncGroups(dbSession, user, newUser); updateRootFlag(dbSession, newUser); diff --git a/server/sonar-server/src/main/java/org/sonar/server/user/NewUser.java b/server/sonar-server/src/main/java/org/sonar/server/user/NewUser.java index 5036657213d..2e8b1e4e20f 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/user/NewUser.java +++ b/server/sonar-server/src/main/java/org/sonar/server/user/NewUser.java @@ -19,43 +19,39 @@ */ package org.sonar.server.user; +import java.util.ArrayList; import java.util.List; import javax.annotation.CheckForNull; import javax.annotation.Nullable; +import static com.google.common.base.Preconditions.checkState; + public class NewUser { private String login; + private String password; private String name; private String email; private List scmAccounts; - private String password; private ExternalIdentity externalIdentity; - private NewUser() { - // No direct call to this constructor + private NewUser(Builder builder) { + this.login = builder.login; + this.password = builder.password; + this.name = builder.name; + this.email = builder.email; + this.scmAccounts = builder.scmAccounts; + this.externalIdentity = builder.externalIdentity; } - public NewUser setLogin(@Nullable String login) { - this.login = login; - return this; - } - - @Nullable public String login() { return login; } - @Nullable public String name() { return name; } - public NewUser setName(@Nullable String name) { - this.name = name; - return this; - } - @CheckForNull public String email() { return email; @@ -66,12 +62,11 @@ public class NewUser { return this; } - @Nullable public List scmAccounts() { return scmAccounts; } - public NewUser setScmAccounts(@Nullable List scmAccounts) { + public NewUser setScmAccounts(List scmAccounts) { this.scmAccounts = scmAccounts; return this; } @@ -96,8 +91,51 @@ public class NewUser { return this; } - public static NewUser create() { - return new NewUser(); + public static Builder builder() { + return new Builder(); } + public static class Builder { + private String login; + private String name; + private String email; + private List scmAccounts = new ArrayList<>(); + private String password; + private ExternalIdentity externalIdentity; + + public Builder setLogin(String login) { + this.login = login; + return this; + } + + public Builder setName(String name) { + this.name = name; + return this; + } + + public Builder setEmail(@Nullable String email) { + this.email = email; + return this; + } + + public Builder setScmAccounts(List scmAccounts) { + this.scmAccounts = scmAccounts; + return this; + } + + public Builder setPassword(@Nullable String password) { + this.password = password; + return this; + } + + public Builder setExternalIdentity(@Nullable ExternalIdentity externalIdentity) { + this.externalIdentity = externalIdentity; + return this; + } + + public NewUser build() { + checkState(externalIdentity == null || password == null, "Password should not be set with an external identity"); + return new NewUser(this); + } + } } diff --git a/server/sonar-server/src/main/java/org/sonar/server/user/ws/CreateAction.java b/server/sonar-server/src/main/java/org/sonar/server/user/ws/CreateAction.java index 236fcc26f85..2ad3abf1dba 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/user/ws/CreateAction.java +++ b/server/sonar-server/src/main/java/org/sonar/server/user/ws/CreateAction.java @@ -102,13 +102,13 @@ public class CreateAction implements UsersWsAction { } private CreateWsResponse doHandle(CreateRequest request) { - NewUser newUser = NewUser.create() + NewUser.Builder newUser = NewUser.builder() .setLogin(request.getLogin()) .setName(request.getName()) .setEmail(request.getEmail()) .setScmAccounts(request.getScmAccounts()) .setPassword(request.getPassword()); - UserDto userDto = userUpdater.create(newUser); + UserDto userDto = userUpdater.create(newUser.build()); return buildResponse(userDto); } diff --git a/server/sonar-server/src/test/java/org/sonar/server/user/NewUserTest.java b/server/sonar-server/src/test/java/org/sonar/server/user/NewUserTest.java index 2c63c61567c..fbda28674e1 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/user/NewUserTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/user/NewUserTest.java @@ -29,16 +29,17 @@ import static org.assertj.core.api.Assertions.assertThat; public class NewUserTest { @Rule - public ExpectedException thrown = ExpectedException.none(); + public ExpectedException expectedException = ExpectedException.none(); @Test public void create_new_user() throws Exception { - NewUser newUser = NewUser.create() + NewUser newUser = NewUser.builder() .setLogin("login") .setName("name") .setEmail("email") .setPassword("password") - .setScmAccounts(asList("login1", "login2")); + .setScmAccounts(asList("login1", "login2")) + .build(); assertThat(newUser.login()).isEqualTo("login"); assertThat(newUser.name()).isEqualTo("name"); @@ -50,25 +51,39 @@ public class NewUserTest { @Test public void create_new_user_with_minimal_fields() throws Exception { - NewUser newUser = NewUser.create(); + NewUser newUser = NewUser.builder().build(); assertThat(newUser.login()).isNull(); assertThat(newUser.name()).isNull(); assertThat(newUser.email()).isNull(); assertThat(newUser.password()).isNull(); - assertThat(newUser.scmAccounts()).isNull(); + assertThat(newUser.scmAccounts()).isEmpty(); } @Test public void create_new_user_with_authority() throws Exception { - NewUser newUser = NewUser.create() + NewUser newUser = NewUser.builder() .setLogin("login") .setName("name") .setEmail("email") - .setPassword("password") - .setExternalIdentity(new ExternalIdentity("github", "github_login")); + .setExternalIdentity(new ExternalIdentity("github", "github_login")) + .build(); assertThat(newUser.externalIdentity().getProvider()).isEqualTo("github"); assertThat(newUser.externalIdentity().getId()).isEqualTo("github_login"); } + + @Test + public void fail_to_set_password_when_external_identity_is_set() throws Exception { + expectedException.expect(IllegalStateException.class); + expectedException.expectMessage("Password should not be set with an external identity"); + + NewUser.builder() + .setLogin("login") + .setName("name") + .setEmail("email") + .setPassword("password") + .setExternalIdentity(new ExternalIdentity("github", "github_login")) + .build(); + } } diff --git a/server/sonar-server/src/test/java/org/sonar/server/user/UserUpdaterTest.java b/server/sonar-server/src/test/java/org/sonar/server/user/UserUpdaterTest.java index cd51d205233..245260961c5 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/user/UserUpdaterTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/user/UserUpdaterTest.java @@ -92,12 +92,13 @@ public class UserUpdaterTest { public void create_user() { createDefaultGroup(); - UserDto dto = underTest.create(NewUser.create() + UserDto dto = underTest.create(NewUser.builder() .setLogin("user") .setName("User") .setEmail("user@mail.com") .setPassword("PASSWORD") - .setScmAccounts(ImmutableList.of("u1", "u_1", "User 1"))); + .setScmAccounts(ImmutableList.of("u1", "u_1", "User 1")) + .build()); assertThat(dto.getId()).isNotNull(); assertThat(dto.getLogin()).isEqualTo("user"); @@ -126,10 +127,11 @@ public class UserUpdaterTest { public void create_user_with_sq_authority_when_no_authority_set() throws Exception { createDefaultGroup(); - underTest.create(NewUser.create() + underTest.create(NewUser.builder() .setLogin("user") .setName("User") - .setPassword("password")); + .setPassword("password") + .build()); UserDto dto = dbClient.userDao().selectByLogin(session, "user"); assertThat(dto.getExternalIdentity()).isEqualTo("user"); @@ -142,9 +144,10 @@ public class UserUpdaterTest { when(system2.now()).thenReturn(1418215735482L); createDefaultGroup(); - underTest.create(NewUser.create() + underTest.create(NewUser.builder() .setLogin("us") - .setName("User")); + .setName("User") + .build()); UserDto dto = dbClient.userDao().selectByLogin(session, "us"); assertThat(dto.getId()).isNotNull(); @@ -160,11 +163,12 @@ public class UserUpdaterTest { when(system2.now()).thenReturn(1418215735482L); createDefaultGroup(); - underTest.create(NewUser.create() + underTest.create(NewUser.builder() .setLogin("user") .setName("User") .setPassword("password") - .setScmAccounts(newArrayList("u1", "", null))); + .setScmAccounts(newArrayList("u1", "", null)) + .build()); assertThat(dbClient.userDao().selectByLogin(session, "user").getScmAccountsAsList()).containsOnly("u1"); } @@ -174,11 +178,12 @@ public class UserUpdaterTest { when(system2.now()).thenReturn(1418215735482L); createDefaultGroup(); - underTest.create(NewUser.create() + underTest.create(NewUser.builder() .setLogin("user") .setName("User") .setPassword("password") - .setScmAccounts(newArrayList(""))); + .setScmAccounts(newArrayList("")) + .build()); assertThat(dbClient.userDao().selectByLogin(session, "user").getScmAccounts()).isNull(); } @@ -188,11 +193,12 @@ public class UserUpdaterTest { when(system2.now()).thenReturn(1418215735482L); createDefaultGroup(); - underTest.create(NewUser.create() + underTest.create(NewUser.builder() .setLogin("user") .setName("User") .setPassword("password") - .setScmAccounts(newArrayList("u1", "u1"))); + .setScmAccounts(newArrayList("u1", "u1")) + .build()); assertThat(dbClient.userDao().selectByLogin(session, "user").getScmAccountsAsList()).containsOnly("u1"); } @@ -202,11 +208,12 @@ public class UserUpdaterTest { expectedException.expect(BadRequestException.class); expectedException.expectMessage("Login can't be empty"); - underTest.create(NewUser.create() + underTest.create(NewUser.builder() .setLogin(null) .setName("Marius") .setEmail("marius@mail.com") - .setPassword("password")); + .setPassword("password") + .build()); } @Test @@ -214,11 +221,12 @@ public class UserUpdaterTest { expectedException.expect(BadRequestException.class); expectedException.expectMessage("Use only letters, numbers, and .-_@ please."); - underTest.create(NewUser.create() + underTest.create(NewUser.builder() .setLogin("/marius/") .setName("Marius") .setEmail("marius@mail.com") - .setPassword("password")); + .setPassword("password") + .build()); } @Test @@ -226,11 +234,12 @@ public class UserUpdaterTest { expectedException.expect(BadRequestException.class); expectedException.expectMessage("Use only letters, numbers, and .-_@ please."); - underTest.create(NewUser.create() + underTest.create(NewUser.builder() .setLogin("mari us") .setName("Marius") .setEmail("marius@mail.com") - .setPassword("password")); + .setPassword("password") + .build()); } @Test @@ -238,11 +247,12 @@ public class UserUpdaterTest { expectedException.expect(BadRequestException.class); expectedException.expectMessage("Login is too short (minimum is 2 characters)"); - underTest.create(NewUser.create() + underTest.create(NewUser.builder() .setLogin("m") .setName("Marius") .setEmail("marius@mail.com") - .setPassword("password")); + .setPassword("password") + .build()); } @Test @@ -250,11 +260,12 @@ public class UserUpdaterTest { expectedException.expect(BadRequestException.class); expectedException.expectMessage("Login is too long (maximum is 255 characters)"); - underTest.create(NewUser.create() + underTest.create(NewUser.builder() .setLogin(Strings.repeat("m", 256)) .setName("Marius") .setEmail("marius@mail.com") - .setPassword("password")); + .setPassword("password") + .build()); } @Test @@ -262,11 +273,12 @@ public class UserUpdaterTest { expectedException.expect(BadRequestException.class); expectedException.expectMessage("Name can't be empty"); - underTest.create(NewUser.create() + underTest.create(NewUser.builder() .setLogin(DEFAULT_LOGIN) .setName(null) .setEmail("marius@mail.com") - .setPassword("password")); + .setPassword("password") + .build()); } @Test @@ -274,11 +286,12 @@ public class UserUpdaterTest { expectedException.expect(BadRequestException.class); expectedException.expectMessage("Name is too long (maximum is 200 characters)"); - underTest.create(NewUser.create() + underTest.create(NewUser.builder() .setLogin(DEFAULT_LOGIN) .setName(Strings.repeat("m", 201)) .setEmail("marius@mail.com") - .setPassword("password")); + .setPassword("password") + .build()); } @Test @@ -286,21 +299,23 @@ public class UserUpdaterTest { expectedException.expect(BadRequestException.class); expectedException.expectMessage("Email is too long (maximum is 100 characters)"); - underTest.create(NewUser.create() + underTest.create(NewUser.builder() .setLogin(DEFAULT_LOGIN) .setName("Marius") .setEmail(Strings.repeat("m", 101)) - .setPassword("password")); + .setPassword("password") + .build()); } @Test public void fail_to_create_user_with_many_errors() { try { - underTest.create(NewUser.create() + underTest.create(NewUser.builder() .setLogin("") .setName("") .setEmail("marius@mail.com") - .setPassword("")); + .setPassword("") + .build()); fail(); } catch (BadRequestException e) { assertThat(e.errors().messages()).hasSize(3); @@ -313,12 +328,13 @@ public class UserUpdaterTest { expectedException.expect(BadRequestException.class); expectedException.expectMessage("The scm account 'jo' is already used by user(s) : 'John (john)'"); - underTest.create(NewUser.create() + underTest.create(NewUser.builder() .setLogin(DEFAULT_LOGIN) .setName("Marius") .setEmail("marius@mail.com") .setPassword("password") - .setScmAccounts(newArrayList("jo"))); + .setScmAccounts(newArrayList("jo")) + .build()); } @Test @@ -327,12 +343,13 @@ public class UserUpdaterTest { expectedException.expect(BadRequestException.class); expectedException.expectMessage("The scm account 'john@email.com' is already used by user(s) : 'John (john), Technical account (technical-account)'"); - underTest.create(NewUser.create() + underTest.create(NewUser.builder() .setLogin(DEFAULT_LOGIN) .setName("Marius") .setEmail("marius@mail.com") .setPassword("password") - .setScmAccounts(newArrayList("john@email.com"))); + .setScmAccounts(newArrayList("john@email.com")) + .build()); } @Test @@ -340,12 +357,13 @@ public class UserUpdaterTest { expectedException.expect(BadRequestException.class); expectedException.expectMessage("Login and email are automatically considered as SCM accounts"); - underTest.create(NewUser.create() + underTest.create(NewUser.builder() .setLogin(DEFAULT_LOGIN) .setName("Marius2") .setEmail("marius2@mail.com") .setPassword("password2") - .setScmAccounts(newArrayList(DEFAULT_LOGIN))); + .setScmAccounts(newArrayList(DEFAULT_LOGIN)) + .build()); } @Test @@ -353,24 +371,26 @@ public class UserUpdaterTest { expectedException.expect(BadRequestException.class); expectedException.expectMessage("Login and email are automatically considered as SCM accounts"); - underTest.create(NewUser.create() + underTest.create(NewUser.builder() .setLogin(DEFAULT_LOGIN) .setName("Marius2") .setEmail("marius2@mail.com") .setPassword("password2") - .setScmAccounts(newArrayList("marius2@mail.com"))); + .setScmAccounts(newArrayList("marius2@mail.com")) + .build()); } @Test public void notify_new_user() { createDefaultGroup(); - underTest.create(NewUser.create() + underTest.create(NewUser.builder() .setLogin("user") .setName("User") .setEmail("user@mail.com") .setPassword("password") - .setScmAccounts(newArrayList("u1", "u_1"))); + .setScmAccounts(newArrayList("u1", "u_1")) + .build()); verify(newUserNotifier).onNewUser(newUserHandler.capture()); assertThat(newUserHandler.getValue().getLogin()).isEqualTo("user"); @@ -382,12 +402,13 @@ public class UserUpdaterTest { public void associate_default_group_when_creating_user() { createDefaultGroup(); - underTest.create(NewUser.create() + underTest.create(NewUser.builder() .setLogin("user") .setName("User") .setEmail("user@mail.com") .setPassword("password") - .setScmAccounts(newArrayList("u1", "u_1"))); + .setScmAccounts(newArrayList("u1", "u_1")) + .build()); Multimap groups = dbClient.groupMembershipDao().selectGroupsByLogins(session, asList("user")); assertThat(groups.get("user")).containsOnly("sonar-users"); @@ -397,12 +418,13 @@ public class UserUpdaterTest { public void doest_not_fail_when_no_default_group() { settings.setProperty(CORE_DEFAULT_GROUP, (String) null); - underTest.create(NewUser.create() + underTest.create(NewUser.builder() .setLogin("user") .setName("User") .setEmail("user@mail.com") .setPassword("password") - .setScmAccounts(newArrayList("u1", "u_1"))); + .setScmAccounts(newArrayList("u1", "u_1")) + .build()); assertThat(dbClient.userDao().selectByLogin(session, "user")).isNotNull(); } @@ -413,12 +435,13 @@ public class UserUpdaterTest { expectedException.expect(ServerException.class); expectedException.expectMessage("The default group 'polop' for new users does not exist. Please update the general security settings to fix this issue."); - underTest.create(NewUser.create() + underTest.create(NewUser.builder() .setLogin("user") .setName("User") .setEmail("user@mail.com") .setPassword("password") - .setScmAccounts(newArrayList("u1", "u_1"))); + .setScmAccounts(newArrayList("u1", "u_1")) + .build()); } @Test @@ -429,11 +452,12 @@ public class UserUpdaterTest { .setUpdatedAt(PAST)); createDefaultGroup(); - UserDto dto = underTest.create(NewUser.create() + UserDto dto = underTest.create(NewUser.builder() .setLogin(DEFAULT_LOGIN) .setName("Marius2") .setEmail("marius2@mail.com") - .setPassword("password2")); + .setPassword("password2") + .build()); session.commit(); assertThat(dto.isActive()).isTrue(); @@ -456,10 +480,11 @@ public class UserUpdaterTest { when(system2.now()).thenReturn(1418215735486L); createDefaultGroup(); - UserDto dto = underTest.create(NewUser.create() + UserDto dto = underTest.create(NewUser.builder() .setLogin(DEFAULT_LOGIN) .setName("Marius2") - .setEmail("marius2@mail.com")); + .setEmail("marius2@mail.com") + .build()); session.commit(); assertThat(dto.isActive()).isTrue(); @@ -481,11 +506,11 @@ public class UserUpdaterTest { .setUpdatedAt(PAST)); createDefaultGroup(); - underTest.create(NewUser.create() + underTest.create(NewUser.builder() .setLogin(DEFAULT_LOGIN) .setName("Marius2") - .setPassword("password2") - .setExternalIdentity(new ExternalIdentity("github", "john"))); + .setExternalIdentity(new ExternalIdentity("github", "john")) + .build()); session.commit(); UserDto dto = dbClient.userDao().selectByLogin(session, DEFAULT_LOGIN); @@ -501,11 +526,12 @@ public class UserUpdaterTest { expectedException.expect(IllegalArgumentException.class); expectedException.expectMessage("An active user with login 'marius' already exists"); - underTest.create(NewUser.create() + underTest.create(NewUser.builder() .setLogin(DEFAULT_LOGIN) .setName("Marius2") .setEmail("marius2@mail.com") - .setPassword("password2")); + .setPassword("password2") + .build()); } @Test @@ -513,11 +539,12 @@ public class UserUpdaterTest { db.prepareDbUnit(getClass(), "associate_default_groups_when_reactivating_user.xml"); createDefaultGroup(); - underTest.create(NewUser.create() + underTest.create(NewUser.builder() .setLogin(DEFAULT_LOGIN) .setName("Marius2") .setEmail("marius2@mail.com") - .setPassword("password2")); + .setPassword("password2") + .build()); session.commit(); Multimap groups = dbClient.groupMembershipDao().selectGroupsByLogins(session, asList(DEFAULT_LOGIN)); diff --git a/server/sonar-server/src/test/java/org/sonar/server/user/ws/ChangePasswordActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/user/ws/ChangePasswordActionTest.java index 1cc924de603..42c6655315e 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/user/ws/ChangePasswordActionTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/user/ws/ChangePasswordActionTest.java @@ -178,12 +178,13 @@ public class ChangePasswordActionTest { @Test(expected = BadRequestException.class) public void fail_to_update_password_on_external_auth() throws Exception { - userUpdater.create(NewUser.create() + userUpdater.create(NewUser.builder() .setEmail("john@email.com") .setLogin("john") .setName("John") .setScmAccounts(newArrayList("jn")) - .setExternalIdentity(new ExternalIdentity("gihhub", "john"))); + .setExternalIdentity(new ExternalIdentity("gihhub", "john")) + .build()); session.clearCache(); when(realmFactory.hasExternalAuthentication()).thenReturn(true); @@ -194,11 +195,12 @@ public class ChangePasswordActionTest { } private void createUser() { - userUpdater.create(NewUser.create() + userUpdater.create(NewUser.builder() .setEmail("john@email.com") .setLogin("john") .setName("John") .setScmAccounts(newArrayList("jn")) - .setPassword("Valar Dohaeris")); + .setPassword("Valar Dohaeris") + .build()); } } -- 2.39.5