From db3e840d742f1e02193567c2ca3a80b0d1e04b22 Mon Sep 17 00:00:00 2001 From: Benjamin Campomenosi <109955405+benjamin-campomenosi-sonarsource@users.noreply.github.com> Date: Mon, 10 Oct 2022 11:34:40 +0200 Subject: [PATCH] SONAR-17291 better message for user login requirements --- .../org/sonar/server/user/UserUpdater.java | 12 +++-- .../HttpHeadersAuthenticationTest.java | 2 +- .../server/user/UserUpdaterCreateTest.java | 49 +++++++++++++------ .../server/user/UserUpdaterUpdateTest.java | 14 +++++- 4 files changed, 56 insertions(+), 21 deletions(-) diff --git a/server/sonar-webserver-auth/src/main/java/org/sonar/server/user/UserUpdater.java b/server/sonar-webserver-auth/src/main/java/org/sonar/server/user/UserUpdater.java index 7df1a44f9b3..594de62f0a2 100644 --- a/server/sonar-webserver-auth/src/main/java/org/sonar/server/user/UserUpdater.java +++ b/server/sonar-webserver-auth/src/main/java/org/sonar/server/user/UserUpdater.java @@ -27,6 +27,7 @@ import java.util.HashSet; 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; @@ -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; @@ -313,15 +316,18 @@ public class UserUpdater { private static boolean validateLoginFormat(@Nullable String login, List 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; } } diff --git a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/HttpHeadersAuthenticationTest.java b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/HttpHeadersAuthenticationTest.java index 6a8b42ab979..743e98b5099 100644 --- a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/HttpHeadersAuthenticationTest.java +++ b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/HttpHeadersAuthenticationTest.java @@ -344,7 +344,7 @@ public class HttpHeadersAuthenticationTest { 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()); diff --git a/server/sonar-webserver-auth/src/test/java/org/sonar/server/user/UserUpdaterCreateTest.java b/server/sonar-webserver-auth/src/test/java/org/sonar/server/user/UserUpdaterCreateTest.java index 5d799c5cc0b..bd9b077a727 100644 --- a/server/sonar-webserver-auth/src/test/java/org/sonar/server/user/UserUpdaterCreateTest.java +++ b/server/sonar-webserver-auth/src/test/java/org/sonar/server/user/UserUpdaterCreateTest.java @@ -87,7 +87,7 @@ public class UserUpdaterCreateTest { .setPassword("PASSWORD") .setScmAccounts(ImmutableList.of("u1", "u_1", "User 1")) .build(), u -> { - }); + }); assertThat(dto.getUuid()).isNotNull(); assertThat(dto.getLogin()).isEqualTo("user"); @@ -122,7 +122,7 @@ public class UserUpdaterCreateTest { .setLogin("us") .setName("User") .build(), u -> { - }); + }); UserDto dto = dbClient.userDao().selectByLogin(session, "us"); assertThat(dto.getUuid()).isNotNull(); @@ -140,7 +140,7 @@ public class UserUpdaterCreateTest { 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"); @@ -155,7 +155,7 @@ public class UserUpdaterCreateTest { .setLogin("") .setName("John Doe") .build(), u -> { - }); + }); UserDto dto = dbClient.userDao().selectByLogin(session, user.getLogin()); assertThat(dto.getLogin()).startsWith("john-doe"); @@ -171,7 +171,7 @@ public class UserUpdaterCreateTest { .setName("User") .setPassword("password") .build(), u -> { - }); + }); UserDto dto = dbClient.userDao().selectByLogin(session, "user"); assertThat(dto.getExternalLogin()).isEqualTo("user"); @@ -188,7 +188,7 @@ public class UserUpdaterCreateTest { .setName("User") .setExternalIdentity(new ExternalIdentity("github", "github-user", "ABCD")) .build(), u -> { - }); + }); UserDto dto = dbClient.userDao().selectByLogin(session, "user"); assertThat(dto.isLocal()).isFalse(); @@ -208,7 +208,7 @@ public class UserUpdaterCreateTest { .setName("User") .setExternalIdentity(new ExternalIdentity(SQ_AUTHORITY, "user", "user")) .build(), u -> { - }); + }); UserDto dto = dbClient.userDao().selectByLogin(session, "user"); assertThat(dto.isLocal()).isFalse(); @@ -229,7 +229,7 @@ public class UserUpdaterCreateTest { .setPassword("password") .setScmAccounts(asList("u1", "", null)) .build(), u -> { - }); + }); assertThat(dbClient.userDao().selectByLogin(session, "user").getScmAccountsAsList()).containsOnly("u1"); } @@ -244,7 +244,7 @@ public class UserUpdaterCreateTest { .setPassword("password") .setScmAccounts(asList("")) .build(), u -> { - }); + }); assertThat(dbClient.userDao().selectByLogin(session, "user").getScmAccounts()).isNull(); } @@ -259,7 +259,7 @@ public class UserUpdaterCreateTest { .setPassword("password") .setScmAccounts(asList("u1", "u1")) .build(), u -> { - }); + }); assertThat(dbClient.userDao().selectByLogin(session, "user").getScmAccountsAsList()).containsOnly("u1"); } @@ -275,7 +275,7 @@ public class UserUpdaterCreateTest { .setEmail("user@mail.com") .setPassword("PASSWORD") .build(), u -> { - }, otherUser); + }, otherUser); assertThat(es.getIds(UserIndexDefinition.TYPE_USER)).containsExactlyInAnyOrder(created.getUuid(), otherUser.getUuid()); } @@ -292,7 +292,7 @@ public class UserUpdaterCreateTest { }); }) .isInstanceOf(BadRequestException.class) - .hasMessage("Use only letters, numbers, and .-_@ please."); + .hasMessage("Login should contain only letters, numbers, and .-_@"); } @Test @@ -307,7 +307,7 @@ public class UserUpdaterCreateTest { }); }) .isInstanceOf(BadRequestException.class) - .hasMessage("Use only letters, numbers, and .-_@ please."); + .hasMessage("Login should contain only letters, numbers, and .-_@"); } @Test @@ -325,6 +325,23 @@ public class UserUpdaterCreateTest { .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(() -> { @@ -394,7 +411,7 @@ public class UserUpdaterCreateTest { .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"); @@ -515,7 +532,7 @@ public class UserUpdaterCreateTest { .setPassword("password") .setScmAccounts(asList("u1", "u_1")) .build(), u -> { - }); + }); verify(newUserNotifier).onNewUser(newUserHandler.capture()); assertThat(newUserHandler.getValue().getLogin()).isEqualTo("user"); @@ -533,7 +550,7 @@ public class UserUpdaterCreateTest { .setEmail("user@mail.com") .setPassword("password") .build(), u -> { - }); + }); Multimap groups = dbClient.groupMembershipDao().selectGroupsByLogins(session, singletonList("user")); assertThat(groups.get("user")).containsOnly(defaultGroup.getName()); diff --git a/server/sonar-webserver-auth/src/test/java/org/sonar/server/user/UserUpdaterUpdateTest.java b/server/sonar-webserver-auth/src/test/java/org/sonar/server/user/UserUpdaterUpdateTest.java index 999dccae463..e779665c0c2 100644 --- a/server/sonar-webserver-auth/src/test/java/org/sonar/server/user/UserUpdaterUpdateTest.java +++ b/server/sonar-webserver-auth/src/test/java/org/sonar/server/user/UserUpdaterUpdateTest.java @@ -628,7 +628,19 @@ public class UserUpdaterUpdateTest { 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 -- 2.39.5