SONAR-8715 User builder for NewUser

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);

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<String> 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<String> scmAccounts() {
     return scmAccounts;
   }
 
-  public NewUser setScmAccounts(@Nullable List<String> scmAccounts) {
+  public NewUser setScmAccounts(List<String> 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<String> 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<String> 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);
+    }
+  }
 }

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);
   }
 

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();
+  }
 }

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<String, String> 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<String, String> groups = dbClient.groupMembershipDao().selectGroupsByLogins(session, asList(DEFAULT_LOGIN));

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());
   }
 }