Skip to content

Commit

Permalink
SONAR-17291 better message for user login requirements
Browse files Browse the repository at this point in the history
  • Loading branch information
@benjamin-campomenosi-sonarsource
benjamin-campomenosi-sonarsource authored and sonartech committed Oct 10, 2022
1 parent 0c06cb9 commit db3e840
Show file tree
Hide file tree
Showing 4 changed files with 56 additions and 21 deletions.
12 changes: 9 additions & 3 deletions server/sonar-webserver-auth/src/main/java/org/sonar/server/user/UserUpdater.java
View file
Expand Up @@ -27,6 +27,7 @@
import java.util.List;
import java.util.Objects;
import java.util.function.Consumer;
import java.util.regex.Pattern;
import java.util.stream.Stream;
import javax.annotation.Nullable;
import javax.inject.Inject;
Expand Down Expand Up @@ -66,6 +67,8 @@ public class UserUpdater {
private static final String PASSWORD_PARAM = "Password";
private static final String NAME_PARAM = "Name";
private static final String EMAIL_PARAM = "Email";
private static final Pattern START_WITH_SPECIFIC_AUTHORIZED_CHARACTERS = Pattern.compile("^[\\.\\-_@].*$");
private static final Pattern CONTAINS_ONLY_AUTHORIZED_CHARACTERS = Pattern.compile("\\A\\w[\\w\\.\\-_@]+\\z");

public static final int LOGIN_MIN_LENGTH = 2;
public static final int LOGIN_MAX_LENGTH = 255;
Expand Down Expand Up @@ -313,15 +316,18 @@ private static boolean checkNotEmptyParam(@Nullable String value, String param,

private static boolean validateLoginFormat(@Nullable String login, List<String> messages) {
boolean isValid = checkNotEmptyParam(login, LOGIN_PARAM, messages);
if (!isNullOrEmpty(login)) {
if (isValid) {
if (login.length() < LOGIN_MIN_LENGTH) {
messages.add(format(Validation.IS_TOO_SHORT_MESSAGE, LOGIN_PARAM, LOGIN_MIN_LENGTH));
return false;
} else if (login.length() > LOGIN_MAX_LENGTH) {
messages.add(format(Validation.IS_TOO_LONG_MESSAGE, LOGIN_PARAM, LOGIN_MAX_LENGTH));
return false;
} else if (!login.matches("\\A\\w[\\w\\.\\-_@]+\\z")) {
messages.add("Use only letters, numbers, and .-_@ please.");
} else if (START_WITH_SPECIFIC_AUTHORIZED_CHARACTERS.matcher(login).matches()){
messages.add("Login should not start with .-_@");
return false;
} else if (!CONTAINS_ONLY_AUTHORIZED_CHARACTERS.matcher(login).matches()) {
messages.add("Login should contain only letters, numbers, and .-_@");
return false;
}
}
Expand Down
2 changes: 1 addition & 1 deletion ...ver-auth/src/test/java/org/sonar/server/authentication/HttpHeadersAuthenticationTest.java
View file
Expand Up @@ -344,7 +344,7 @@ public void throw_AuthenticationException_when_BadRequestException_is_generated(
setNotUserInToken();

assertThatThrownBy(() -> underTest.authenticate(createRequest("invalid login", DEFAULT_NAME, DEFAULT_EMAIL, GROUPS), response))
.hasMessage("Use only letters, numbers, and .-_@ please.")
.hasMessage("Login should contain only letters, numbers, and .-_@")
.isInstanceOf(AuthenticationException.class)
.hasFieldOrPropertyWithValue("source", Source.sso());

Expand Down
49 changes: 33 additions & 16 deletions server/sonar-webserver-auth/src/test/java/org/sonar/server/user/UserUpdaterCreateTest.java
View file
Expand Up @@ -87,7 +87,7 @@ public void create_user() {
.setPassword("PASSWORD")
.setScmAccounts(ImmutableList.of("u1", "u_1", "User 1"))
.build(), u -> {
});
});

assertThat(dto.getUuid()).isNotNull();
assertThat(dto.getLogin()).isEqualTo("user");
Expand Down Expand Up @@ -122,7 +122,7 @@ public void create_user_with_minimum_fields() {
.setLogin("us")
.setName("User")
.build(), u -> {
});
});

UserDto dto = dbClient.userDao().selectByLogin(session, "us");
assertThat(dto.getUuid()).isNotNull();
Expand All @@ -140,7 +140,7 @@ public void create_user_generates_unique_login_no_login_provided() {
UserDto user = underTest.createAndCommit(db.getSession(), NewUser.builder()
.setName("John Doe")
.build(), u -> {
});
});

UserDto dto = dbClient.userDao().selectByLogin(session, user.getLogin());
assertThat(dto.getLogin()).startsWith("john-doe");
Expand All @@ -155,7 +155,7 @@ public void create_user_generates_unique_login_when_login_is_empty() {
.setLogin("")
.setName("John Doe")
.build(), u -> {
});
});

UserDto dto = dbClient.userDao().selectByLogin(session, user.getLogin());
assertThat(dto.getLogin()).startsWith("john-doe");
Expand All @@ -171,7 +171,7 @@ public void create_user_with_sq_authority_when_no_authority_set() {
.setName("User")
.setPassword("password")
.build(), u -> {
});
});

UserDto dto = dbClient.userDao().selectByLogin(session, "user");
assertThat(dto.getExternalLogin()).isEqualTo("user");
Expand All @@ -188,7 +188,7 @@ public void create_user_with_identity_provider() {
.setName("User")
.setExternalIdentity(new ExternalIdentity("github", "github-user", "ABCD"))
.build(), u -> {
});
});

UserDto dto = dbClient.userDao().selectByLogin(session, "user");
assertThat(dto.isLocal()).isFalse();
Expand All @@ -208,7 +208,7 @@ public void create_user_with_sonarqube_external_identity() {
.setName("User")
.setExternalIdentity(new ExternalIdentity(SQ_AUTHORITY, "user", "user"))
.build(), u -> {
});
});

UserDto dto = dbClient.userDao().selectByLogin(session, "user");
assertThat(dto.isLocal()).isFalse();
Expand All @@ -229,7 +229,7 @@ public void create_user_with_scm_accounts_containing_blank_or_null_entries() {
.setPassword("password")
.setScmAccounts(asList("u1", "", null))
.build(), u -> {
});
});

assertThat(dbClient.userDao().selectByLogin(session, "user").getScmAccountsAsList()).containsOnly("u1");
}
Expand All @@ -244,7 +244,7 @@ public void create_user_with_scm_accounts_containing_one_blank_entry() {
.setPassword("password")
.setScmAccounts(asList(""))
.build(), u -> {
});
});

assertThat(dbClient.userDao().selectByLogin(session, "user").getScmAccounts()).isNull();
}
Expand All @@ -259,7 +259,7 @@ public void create_user_with_scm_accounts_containing_duplications() {
.setPassword("password")
.setScmAccounts(asList("u1", "u1"))
.build(), u -> {
});
});

assertThat(dbClient.userDao().selectByLogin(session, "user").getScmAccountsAsList()).containsOnly("u1");
}
Expand All @@ -275,7 +275,7 @@ public void create_user_and_index_other_user() {
.setEmail("user@mail.com")
.setPassword("PASSWORD")
.build(), u -> {
}, otherUser);
}, otherUser);

assertThat(es.getIds(UserIndexDefinition.TYPE_USER)).containsExactlyInAnyOrder(created.getUuid(), otherUser.getUuid());
}
Expand All @@ -292,7 +292,7 @@ public void fail_to_create_user_with_invalid_login() {
});
})
.isInstanceOf(BadRequestException.class)
.hasMessage("Use only letters, numbers, and .-_@ please.");
.hasMessage("Login should contain only letters, numbers, and .-_@");
}

@Test
Expand All @@ -307,7 +307,7 @@ public void fail_to_create_user_with_space_in_login() {
});
})
.isInstanceOf(BadRequestException.class)
.hasMessage("Use only letters, numbers, and .-_@ please.");
.hasMessage("Login should contain only letters, numbers, and .-_@");
}

@Test
Expand All @@ -325,6 +325,23 @@ public void fail_to_create_user_with_too_short_login() {
.hasMessage("Login is too short (minimum is 2 characters)");
}


@Test
public void fail_to_create_user_login_start_with_underscore() {
assertThatThrownBy(() -> {
underTest.createAndCommit(db.getSession(), NewUser.builder()
.setLogin("_marbalous")
.setName("Marius")
.setEmail("marius@mail.com")
.setPassword("password")
.build(), u -> {
});
})
.isInstanceOf(BadRequestException.class)
.hasMessage("Login should not start with .-_@");
}


@Test
public void fail_to_create_user_with_too_long_login() {
assertThatThrownBy(() -> {
Expand Down Expand Up @@ -394,7 +411,7 @@ public void fail_to_create_user_with_many_errors() {
.setEmail("marius@mail.com")
.setPassword("")
.build(), u -> {
});
});
fail();
} catch (BadRequestException e) {
assertThat(e.errors()).containsExactlyInAnyOrder("Name can't be empty", "Password can't be empty");
Expand Down Expand Up @@ -515,7 +532,7 @@ public void notify_new_user() {
.setPassword("password")
.setScmAccounts(asList("u1", "u_1"))
.build(), u -> {
});
});

verify(newUserNotifier).onNewUser(newUserHandler.capture());
assertThat(newUserHandler.getValue().getLogin()).isEqualTo("user");
Expand All @@ -533,7 +550,7 @@ public void associate_default_group_when_creating_user() {
.setEmail("user@mail.com")
.setPassword("password")
.build(), u -> {
});
});

Multimap<String, String> groups = dbClient.groupMembershipDao().selectGroupsByLogins(session, singletonList("user"));
assertThat(groups.get("user")).containsOnly(defaultGroup.getName());
Expand Down
14 changes: 13 additions & 1 deletion server/sonar-webserver-auth/src/test/java/org/sonar/server/user/UserUpdaterUpdateTest.java
View file
Expand Up @@ -628,7 +628,19 @@ public void fail_to_update_login_when_format_is_invalid() {

assertThatThrownBy(() -> underTest.updateAndCommit(session, user, updateUser, EMPTY_USER_CONSUMER))
.isInstanceOf(BadRequestException.class)
.hasMessage("Use only letters, numbers, and .-_@ please.");
.hasMessage("Login should contain only letters, numbers, and .-_@");
}

@Test
public void fail_to_update_login_when_login_start_with_unauthorized_characters() {
UserDto user = db.users().insertUser();
createDefaultGroup();

UpdateUser updateUser = new UpdateUser().setLogin("_StartWithUnderscore");

assertThatThrownBy(() -> underTest.updateAndCommit(session, user, updateUser, EMPTY_USER_CONSUMER))
.isInstanceOf(BadRequestException.class)
.hasMessage("Login should not start with .-_@");
}

@Test
Expand Down

0 comments on commit db3e840

Please sign in to comment.