From: Aurelien Poscia Date: Mon, 15 Jan 2024 14:09:44 +0000 (+0100) Subject: SONAR-21114 Allow updating externalProvider/externalLogin in PATCH /api/v2/users... X-Git-Tag: 10.4.0.87286~198 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=4242fb2533df340a520ad8c86912155ed8a53c20;p=sonarqube.git SONAR-21114 Allow updating externalProvider/externalLogin in PATCH /api/v2/users-management/users endpoint --- diff --git a/server/sonar-webserver-auth/src/it/java/org/sonar/server/user/UserUpdaterUpdateIT.java b/server/sonar-webserver-auth/src/it/java/org/sonar/server/user/UserUpdaterUpdateIT.java index a709dad773b..abd73df435c 100644 --- a/server/sonar-webserver-auth/src/it/java/org/sonar/server/user/UserUpdaterUpdateIT.java +++ b/server/sonar-webserver-auth/src/it/java/org/sonar/server/user/UserUpdaterUpdateIT.java @@ -69,7 +69,8 @@ public class UserUpdaterUpdateIT { private final NewUserNotifier newUserNotifier = mock(NewUserNotifier.class); private final DbSession session = db.getSession(); private final MapSettings settings = new MapSettings().setProperty("sonar.internal.pbkdf2.iterations", "1"); - private final CredentialsLocalAuthentication localAuthentication = new CredentialsLocalAuthentication(db.getDbClient(), settings.asConfig()); + private final CredentialsLocalAuthentication localAuthentication = new CredentialsLocalAuthentication(db.getDbClient(), + settings.asConfig()); private final AuditPersister auditPersister = mock(AuditPersister.class); private final UserUpdater underTest = new UserUpdater(newUserNotifier, dbClient, new DefaultGroupFinder(dbClient), settings.asConfig(), auditPersister, localAuthentication); @@ -105,7 +106,9 @@ public class UserUpdaterUpdateIT { underTest.updateAndCommit(session, user, new UpdateUser() .setName("Marius2") .setEmail("marius2@email.com") - .setExternalIdentity(new ExternalIdentity("github", "john", "ABCD")), u -> { + .setExternalIdentityProvider("github") + .setExternalIdentityProviderId("ABCD") + .setExternalIdentityProviderLogin("john"), u -> { }); UserDto dto = dbClient.userDao().selectByLogin(session, DEFAULT_LOGIN); @@ -123,7 +126,9 @@ public class UserUpdaterUpdateIT { underTest.updateAndCommit(session, user, new UpdateUser() .setName("Marius2") .setEmail("marius2@email.com") - .setExternalIdentity(new ExternalIdentity("github", "john", "ABCD")), u -> { + .setExternalIdentityProvider("github") + .setExternalIdentityProviderId("ABCD") + .setExternalIdentityProviderLogin("john"), u -> { }); UserDto dto = dbClient.userDao().selectByLogin(session, DEFAULT_LOGIN); @@ -220,7 +225,8 @@ public class UserUpdaterUpdateIT { .setLogin("new_login"), u -> { }); - assertThat(db.getDbClient().propertiesDao().selectByQuery(PropertyQuery.builder().setKey(DEFAULT_ISSUE_ASSIGNEE).build(), db.getSession())) + assertThat(db.getDbClient().propertiesDao().selectByQuery(PropertyQuery.builder().setKey(DEFAULT_ISSUE_ASSIGNEE).build(), + db.getSession())) .extracting(PropertyDto::getValue, PropertyDto::getEntityUuid) .containsOnly( tuple("new_login", null), @@ -376,9 +382,11 @@ public class UserUpdaterUpdateIT { .setExternalLogin("john.smith") .setExternalIdentityProvider("github")); createDefaultGroup(); - - underTest.updateAndCommit(session, user, new UpdateUser().setExternalIdentity(new ExternalIdentity("github", "john.smith", "ABCD")), u -> { - }); + UpdateUser updateUser = new UpdateUser() + .setExternalIdentityProvider("github") + .setExternalIdentityProviderId("ABCD") + .setExternalIdentityProviderLogin("john.smith"); + underTest.updateAndCommit(session, user, updateUser, u -> {}); assertThat(dbClient.userDao().selectByLogin(session, DEFAULT_LOGIN)) .extracting(UserDto::getExternalId) @@ -393,8 +401,8 @@ public class UserUpdaterUpdateIT { .setExternalIdentityProvider("github")); createDefaultGroup(); - underTest.updateAndCommit(session, user, new UpdateUser().setExternalIdentity(new ExternalIdentity("github", "john.smith", "ABCD")), u -> { - }); + UpdateUser updateUser = new UpdateUser().setExternalIdentityProviderLogin("john.smith"); + underTest.updateAndCommit(session, user, updateUser, u -> {}); assertThat(dbClient.userDao().selectByLogin(session, DEFAULT_LOGIN)) .extracting(UserDto::getExternalLogin, UserDto::getExternalIdentityProvider) @@ -409,7 +417,8 @@ public class UserUpdaterUpdateIT { .setExternalIdentityProvider("github")); createDefaultGroup(); - underTest.updateAndCommit(session, user, new UpdateUser().setExternalIdentity(new ExternalIdentity("bitbucket", "john", "ABCD")), u -> { + UpdateUser updateUser = new UpdateUser().setExternalIdentityProvider("bitbucket"); + underTest.updateAndCommit(session, user, updateUser, u -> { }); assertThat(dbClient.userDao().selectByLogin(session, DEFAULT_LOGIN)) @@ -425,11 +434,14 @@ public class UserUpdaterUpdateIT { createDefaultGroup(); underTest.updateAndCommit(session, user, new UpdateUser() - .setName(user.getName()) - .setEmail(user.getEmail()) - .setScmAccounts(user.getSortedScmAccounts()) - .setExternalIdentity(new ExternalIdentity(user.getExternalIdentityProvider(), user.getExternalLogin(), user.getExternalId())), u -> { - }); + .setName(user.getName()) + .setEmail(user.getEmail()) + .setScmAccounts(user.getSortedScmAccounts()) + .setExternalIdentityProvider(user.getExternalIdentityProvider()) + .setExternalIdentityProviderId(user.getExternalId()) + .setExternalIdentityProviderLogin(user.getExternalLogin()) + , u -> { + }); assertThat(dbClient.userDao().selectByLogin(session, DEFAULT_LOGIN).getUpdatedAt()).isEqualTo(user.getUpdatedAt()); } @@ -442,11 +454,14 @@ public class UserUpdaterUpdateIT { createDefaultGroup(); underTest.updateAndCommit(session, user, new UpdateUser() - .setName(user.getName()) - .setEmail(user.getEmail()) - .setScmAccounts(asList("ma2", "ma1")) - .setExternalIdentity(new ExternalIdentity(user.getExternalIdentityProvider(), user.getExternalLogin(), user.getExternalId())), u -> { - }); + .setName(user.getName()) + .setEmail(user.getEmail()) + .setScmAccounts(asList("ma2", "ma1")) + .setExternalIdentityProvider(user.getExternalIdentityProvider()) + .setExternalIdentityProviderId(user.getExternalId()) + .setExternalIdentityProviderLogin(user.getExternalLogin()) + , u -> { + }); assertThat(dbClient.userDao().selectByLogin(session, DEFAULT_LOGIN).getUpdatedAt()).isEqualTo(user.getUpdatedAt()); } @@ -500,7 +515,8 @@ public class UserUpdaterUpdateIT { // User is already associate to the default group Multimap groups = dbClient.groupMembershipDao().selectGroupsByLogins(session, asList(DEFAULT_LOGIN)); - assertThat(groups.get(DEFAULT_LOGIN).stream().anyMatch(g -> g.equals(defaultGroup.getName()))).as("Current user groups : %s", groups.get(defaultGroup.getName())).isTrue(); + assertThat(groups.get(DEFAULT_LOGIN).stream().anyMatch(g -> g.equals(defaultGroup.getName()))).as("Current user groups : %s", + groups.get(defaultGroup.getName())).isTrue(); underTest.updateAndCommit(session, user, new UpdateUser() .setName("Marius2") @@ -608,14 +624,13 @@ public class UserUpdaterUpdateIT { public void fail_to_update_user_when_external_id_and_external_provider_already_exists() { createDefaultGroup(); UserDto user = db.users().insertUser(u -> u.setActive(false)); - UserDto existingUser = db.users().insertUser(u -> u.setExternalId("existing_external_id").setExternalIdentityProvider("existing_external_provider")); + UserDto existingUser = db.users().insertUser(u -> u.setExternalId("existing_external_id").setExternalIdentityProvider( + "existing_external_provider")); UpdateUser updateUser = new UpdateUser() - .setExternalIdentity( - new ExternalIdentity( - existingUser.getExternalIdentityProvider(), - existingUser.getExternalLogin(), - existingUser.getExternalId())); + .setExternalIdentityProvider(existingUser.getExternalIdentityProvider()) + .setExternalIdentityProviderId(existingUser.getExternalId()) + .setExternalIdentityProviderLogin(existingUser.getExternalLogin()); assertThatThrownBy(() -> underTest.updateAndCommit(session, user, updateUser, EMPTY_USER_CONSUMER)) .isInstanceOf(IllegalArgumentException.class) diff --git a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/UserRegistrarImpl.java b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/UserRegistrarImpl.java index 24b5b6d9a0b..4e72bbb1498 100644 --- a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/UserRegistrarImpl.java +++ b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/UserRegistrarImpl.java @@ -188,10 +188,9 @@ public class UserRegistrarImpl implements UserRegistrar { UpdateUser update = new UpdateUser() .setEmail(authenticatorParameters.getUserIdentity().getEmail()) .setName(authenticatorParameters.getUserIdentity().getName()) - .setExternalIdentity(new ExternalIdentity( - authenticatorParameters.getProvider().getKey(), - authenticatorParameters.getUserIdentity().getProviderLogin(), - authenticatorParameters.getUserIdentity().getProviderId())); + .setExternalIdentityProvider(authenticatorParameters.getProvider().getKey()) + .setExternalIdentityProviderId(authenticatorParameters.getUserIdentity().getProviderId()) + .setExternalIdentityProviderLogin(authenticatorParameters.getUserIdentity().getProviderLogin()); Optional otherUserToIndex = detectEmailUpdate(dbSession, authenticatorParameters, userDto.getUuid()); userUpdater.updateAndCommit(dbSession, userDto, update, beforeCommit(dbSession, authenticatorParameters), toArray(otherUserToIndex)); return userDto; diff --git a/server/sonar-webserver-auth/src/main/java/org/sonar/server/user/UpdateUser.java b/server/sonar-webserver-auth/src/main/java/org/sonar/server/user/UpdateUser.java index 8123825fe0f..9df6f01f879 100644 --- a/server/sonar-webserver-auth/src/main/java/org/sonar/server/user/UpdateUser.java +++ b/server/sonar-webserver-auth/src/main/java/org/sonar/server/user/UpdateUser.java @@ -25,19 +25,22 @@ import javax.annotation.Nullable; public class UpdateUser { - private String login; - private String name; - private String email; - private List scmAccounts; - private String password; - private ExternalIdentity externalIdentity; - - private boolean loginChanged; - private boolean nameChanged; - private boolean emailChanged; - private boolean scmAccountsChanged; - private boolean passwordChanged; - private boolean externalIdentityChanged; + private String login = null; + private String name = null; + private String email = null; + private List scmAccounts = null; + private String password = null; + private String externalIdentityProvider = null; + private String externalIdentityProviderId = null; + private String externalIdentityProviderLogin = null; + private boolean loginChanged = false; + private boolean nameChanged = false; + private boolean emailChanged = false; + private boolean scmAccountsChanged = false; + private boolean passwordChanged = false; + private boolean externalIdentityProviderChanged = false; + private boolean externalIdentityProviderIdChanged = false; + private boolean externalIdentityProviderLoginChanged = false; @CheckForNull public String login() { @@ -94,17 +97,37 @@ public class UpdateUser { return this; } + + @CheckForNull + public String externalIdentityProvider() { + return externalIdentityProvider; + } + + public UpdateUser setExternalIdentityProvider(@Nullable String externalIdentityProvider) { + this.externalIdentityProvider = externalIdentityProvider; + externalIdentityProviderChanged = true; + return this; + } + + @CheckForNull + public String externalIdentityProviderId() { + return externalIdentityProviderId; + } + + public UpdateUser setExternalIdentityProviderId(@Nullable String externalIdentityProviderId) { + this.externalIdentityProviderId = externalIdentityProviderId; + externalIdentityProviderIdChanged = true; + return this; + } + @CheckForNull - public ExternalIdentity externalIdentity() { - return externalIdentity; + public String externalIdentityProviderLogin() { + return externalIdentityProviderLogin; } - /** - * This method should only be used when updating a none local user - */ - public UpdateUser setExternalIdentity(@Nullable ExternalIdentity externalIdentity) { - this.externalIdentity = externalIdentity; - externalIdentityChanged = true; + public UpdateUser setExternalIdentityProviderLogin(@Nullable String getExternalIdentityProviderLogin) { + this.externalIdentityProviderLogin = getExternalIdentityProviderLogin; + externalIdentityProviderLoginChanged = true; return this; } @@ -128,8 +151,15 @@ public class UpdateUser { return passwordChanged; } - public boolean isExternalIdentityChanged() { - return externalIdentityChanged; + public boolean isExternalIdentityProviderChanged() { + return externalIdentityProviderChanged; + } + + public boolean isExternalIdentityProviderIdChanged() { + return externalIdentityProviderIdChanged; } + public boolean isExternalIdentityProviderLoginChanged() { + return externalIdentityProviderLoginChanged; + } } 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 1175adf13b1..a2faa63fd9f 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 @@ -25,6 +25,7 @@ import java.util.ArrayList; import java.util.HashSet; import java.util.List; import java.util.Objects; +import java.util.Optional; import java.util.function.Consumer; import java.util.regex.Pattern; import javax.annotation.Nullable; @@ -101,8 +102,13 @@ public class UserUpdater { UpdateUser updateUser = new UpdateUser() .setName(newUser.name()) .setEmail(newUser.email()) - .setScmAccounts(newUser.scmAccounts()) - .setExternalIdentity(newUser.externalIdentity()); + .setScmAccounts(newUser.scmAccounts()); + + Optional externalIdentity = Optional.ofNullable(newUser.externalIdentity()); + updateUser.setExternalIdentityProvider(externalIdentity.map(ExternalIdentity::getProvider).orElse(null)); + updateUser.setExternalIdentityProviderId(externalIdentity.map(ExternalIdentity::getId).orElse(null)); + updateUser.setExternalIdentityProviderLogin(externalIdentity.map(ExternalIdentity::getLogin).orElse(null)); + String login = newUser.login(); if (login != null) { updateUser.setLogin(login); @@ -168,7 +174,7 @@ public class UserUpdater { userDto.setScmAccounts(scmAccounts); } - setExternalIdentity(dbSession, userDto, newUser.externalIdentity()); + setExternalIdentity(dbSession, userDto, ExternalIdentityLocal.fromExternalIdentity(newUser.externalIdentity())); checkRequest(messages.isEmpty(), messages); return userDto; @@ -233,14 +239,21 @@ public class UserUpdater { } private boolean updateExternalIdentity(DbSession dbSession, UpdateUser updateUser, UserDto userDto) { - ExternalIdentity externalIdentity = updateUser.externalIdentity(); - if (updateUser.isExternalIdentityChanged() && !isSameExternalIdentity(userDto, externalIdentity)) { - setExternalIdentity(dbSession, userDto, externalIdentity); - return true; + if (externalIdentityChanged(updateUser)) { + ExternalIdentityLocal externalIdentityLocal = ExternalIdentityLocal.fromUpdateUser(updateUser); + if (!externalIdentityLocal.isSameExternalIdentity(userDto)) { + setExternalIdentity(dbSession, userDto, externalIdentityLocal); + return true; + } } return false; } + private static boolean externalIdentityChanged(UpdateUser updateUser) { + return updateUser.isExternalIdentityProviderChanged() || updateUser.isExternalIdentityProviderIdChanged() || updateUser.isExternalIdentityProviderLoginChanged(); + } + + private boolean updatePassword(DbSession dbSession, UpdateUser updateUser, UserDto userDto, List messages) { String password = updateUser.password(); if (updateUser.isPasswordChanged() && validatePasswords(password, messages) && checkPasswordChangeAllowed(userDto, messages)) { @@ -270,29 +283,23 @@ public class UserUpdater { return false; } - private static boolean isSameExternalIdentity(UserDto dto, @Nullable ExternalIdentity externalIdentity) { - return externalIdentity != null - && !dto.isLocal() - && Objects.equals(dto.getExternalId(), externalIdentity.getId()) - && Objects.equals(dto.getExternalLogin(), externalIdentity.getLogin()) - && Objects.equals(dto.getExternalIdentityProvider(), externalIdentity.getProvider()); - } - private void setExternalIdentity(DbSession dbSession, UserDto dto, @Nullable ExternalIdentity externalIdentity) { - if (externalIdentity == null) { + private void setExternalIdentity(DbSession dbSession, UserDto dto, ExternalIdentityLocal externalIdentity) { + if (externalIdentity.isEmpty()) { dto.setExternalLogin(dto.getLogin()); dto.setExternalIdentityProvider(SQ_AUTHORITY); dto.setExternalId(dto.getLogin()); dto.setLocal(true); } else { - dto.setExternalLogin(externalIdentity.getLogin()); - dto.setExternalIdentityProvider(externalIdentity.getProvider()); - dto.setExternalId(externalIdentity.getId()); + dto.setExternalLogin(Optional.ofNullable(externalIdentity.login()).orElse(dto.getExternalLogin())); + dto.setExternalIdentityProvider(Optional.ofNullable(externalIdentity.provider()).orElse(dto.getExternalIdentityProvider())); + dto.setExternalId(Optional.ofNullable(externalIdentity.id()).orElse(dto.getExternalId())); dto.setLocal(false); dto.setSalt(null); dto.setCryptedPassword(null); } - UserDto existingUser = dbClient.userDao().selectByExternalIdAndIdentityProvider(dbSession, dto.getExternalId(), dto.getExternalIdentityProvider()); + UserDto existingUser = dbClient.userDao().selectByExternalIdAndIdentityProvider(dbSession, dto.getExternalId(), + dto.getExternalIdentityProvider()); checkArgument(existingUser == null || Objects.equals(dto.getUuid(), existingUser.getUuid()), "A user with provider id '%s' and identity provider '%s' already exists", dto.getExternalId(), dto.getExternalIdentityProvider()); } @@ -366,7 +373,8 @@ public class UserUpdater { return true; } - private boolean validateScmAccounts(DbSession dbSession, List scmAccounts, @Nullable String login, @Nullable String email, @Nullable UserDto existingUser, + private boolean validateScmAccounts(DbSession dbSession, List scmAccounts, @Nullable String login, @Nullable String email, + @Nullable UserDto existingUser, List messages) { boolean isValid = true; for (String scmAccount : scmAccounts) { @@ -383,7 +391,8 @@ public class UserUpdater { matchingUsersWithoutExistingUser.add(getNameOrLogin(matchingUser) + " (" + matchingUser.getLogin() + ")"); } if (!matchingUsersWithoutExistingUser.isEmpty()) { - messages.add(format("The scm account '%s' is already used by user(s) : '%s'", scmAccount, Joiner.on(", ").join(matchingUsersWithoutExistingUser))); + messages.add(format("The scm account '%s' is already used by user(s) : '%s'", scmAccount, + Joiner.on(", ").join(matchingUsersWithoutExistingUser))); isValid = false; } } @@ -449,4 +458,31 @@ public class UserUpdater { dbClient.userGroupDao().insert(dbSession, new UserGroupDto().setUserUuid(userDto.getUuid()).setGroupUuid(defaultGroup.getUuid()), defaultGroup.getName(), userDto.getLogin()); } + + private record ExternalIdentityLocal(@Nullable String provider, @Nullable String id, @Nullable String login) { + private static ExternalIdentityLocal fromUpdateUser(UpdateUser updateUser) { + return new ExternalIdentityLocal(updateUser.externalIdentityProvider(), updateUser.externalIdentityProviderId(), + updateUser.externalIdentityProviderLogin()); + } + + private static ExternalIdentityLocal fromExternalIdentity(@Nullable ExternalIdentity externalIdentity) { + if (externalIdentity == null) { + return new ExternalIdentityLocal(null, null, null); + } + return new ExternalIdentityLocal(externalIdentity.getProvider(), externalIdentity.getId(), externalIdentity.getLogin()); + } + + boolean isEmpty() { + return provider == null && id == null && login == null; + } + + private boolean isSameExternalIdentity(UserDto userDto) { + return !(provider == null && id == null && login == null) + && !userDto.isLocal() + && Objects.equals(userDto.getExternalIdentityProvider(), provider) + && Objects.equals(userDto.getExternalLogin(), login) + && Objects.equals(userDto.getExternalId(), id); + } + } + } diff --git a/server/sonar-webserver-common/src/it/java/org/sonar/server/common/user/service/UserServiceIT.java b/server/sonar-webserver-common/src/it/java/org/sonar/server/common/user/service/UserServiceIT.java index 64f3ff99397..58b649e1fa4 100644 --- a/server/sonar-webserver-common/src/it/java/org/sonar/server/common/user/service/UserServiceIT.java +++ b/server/sonar-webserver-common/src/it/java/org/sonar/server/common/user/service/UserServiceIT.java @@ -31,6 +31,7 @@ import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.sonar.api.config.internal.MapSettings; +import org.sonar.api.server.authentication.IdentityProvider; import org.sonar.api.utils.DateUtils; import org.sonar.api.web.UserRole; import org.sonar.core.util.UuidFactory; @@ -54,6 +55,7 @@ import org.sonar.db.user.SessionTokenDto; import org.sonar.db.user.UserDismissedMessageDto; import org.sonar.db.user.UserDto; import org.sonar.server.authentication.CredentialsLocalAuthentication; +import org.sonar.server.authentication.IdentityProviderRepository; import org.sonar.server.common.SearchResults; import org.sonar.server.common.avatar.AvatarResolverImpl; import org.sonar.server.common.management.ManagedInstanceChecker; @@ -105,7 +107,8 @@ public class UserServiceIT { private final UserUpdater userUpdater = new UserUpdater(mock(NewUserNotifier.class), db.getDbClient(), new DefaultGroupFinder(db.getDbClient()), settings.asConfig(), new NoOpAuditPersister(), localAuthentication); - private final UserService userService = new UserService(db.getDbClient(), new AvatarResolverImpl(), managedInstanceService, managedInstanceChecker, userDeactivator, userUpdater); + private final IdentityProviderRepository identityProviderRepository = mock(); + private final UserService userService = new UserService(db.getDbClient(), new AvatarResolverImpl(), managedInstanceService, managedInstanceChecker, userDeactivator, userUpdater, identityProviderRepository); @Before public void setUp() { @@ -804,6 +807,39 @@ public class UserServiceIT { assertThat(updatedUser.getSortedScmAccounts()).containsExactly("account1", "account2"); } + @Test + public void updateUser_whenUserExistsAndExternalProviderIllegal_shouldThrow() { + UpdateUser updateUser = new UpdateUser(); + updateUser.setExternalIdentityProvider("illegal"); + + assertThatThrownBy(() -> userService.updateUser("login", updateUser)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Value of 'externalProvider' (illegal) must be one of: [] or [LDAP, LDAP_{serverKey}]"); + + } + + @Test + public void updateUser_whenUserExistsAndExternalIdentityChanged_shouldChange() { + UserDto user = db.users().insertUser(); + IdentityProvider mockProvider1 = mock(); + IdentityProvider mockProvider2 = mock(); + when(identityProviderRepository.getAllEnabledAndSorted()).thenReturn(List.of(mockProvider1, mockProvider2)); + when(mockProvider2.getKey()).thenReturn("valid_provider"); + + UpdateUser updateUser = new UpdateUser(); + updateUser.setExternalIdentityProvider("valid_provider"); + updateUser.setExternalIdentityProviderId("prov_id"); + updateUser.setExternalIdentityProviderLogin("prov_login"); + + userService.updateUser(user.getUuid(), updateUser); + + UserDto updatedUser = db.users().selectUserByLogin(user.getLogin()).orElseThrow(); + + assertThat(updatedUser.getExternalIdentityProvider()).isEqualTo("valid_provider"); + assertThat(updatedUser.getExternalId()).isEqualTo("prov_id"); + assertThat(updatedUser.getExternalLogin()).isEqualTo("prov_login"); + } + private void assertUserWithFilter(Function query, String userLogin, boolean isExpectedToBeThere) { UsersSearchRequest.Builder builder = getBuilderWithDefaultsPageSize(); @@ -941,7 +977,7 @@ public class UserServiceIT { @Test public void createUser_whenDeactivatedUserExists_shouldReactivate() { - db.users().insertUser(newUserDto("john", "John", "john@email.com").setActive(false)); + db.users().insertUser(newUserDto("john", "John", "john@email.com").setActive(false).setLocal(true)); UserCreateRequest userCreateRequest = UserCreateRequest.builder() .setLogin("john") diff --git a/server/sonar-webserver-common/src/main/java/org/sonar/server/common/user/service/UserCreateRequest.java b/server/sonar-webserver-common/src/main/java/org/sonar/server/common/user/service/UserCreateRequest.java index 0f1da319d70..f0dbd5af138 100644 --- a/server/sonar-webserver-common/src/main/java/org/sonar/server/common/user/service/UserCreateRequest.java +++ b/server/sonar-webserver-common/src/main/java/org/sonar/server/common/user/service/UserCreateRequest.java @@ -21,6 +21,7 @@ package org.sonar.server.common.user.service; import java.util.List; import java.util.Optional; +import javax.annotation.Nullable; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Strings.isNullOrEmpty; @@ -82,7 +83,7 @@ public class UserCreateRequest { // enforce factory method use } - public Builder setEmail(String email) { + public Builder setEmail(@Nullable String email) { this.email = email; return this; } @@ -102,12 +103,12 @@ public class UserCreateRequest { return this; } - public Builder setPassword(String password) { + public Builder setPassword(@Nullable String password) { this.password = password; return this; } - public Builder setScmAccounts(List scmAccounts) { + public Builder setScmAccounts(@Nullable List scmAccounts) { this.scmAccounts = scmAccounts; return this; } diff --git a/server/sonar-webserver-common/src/main/java/org/sonar/server/common/user/service/UserService.java b/server/sonar-webserver-common/src/main/java/org/sonar/server/common/user/service/UserService.java index 27b8e739249..ed83c9673d0 100644 --- a/server/sonar-webserver-common/src/main/java/org/sonar/server/common/user/service/UserService.java +++ b/server/sonar-webserver-common/src/main/java/org/sonar/server/common/user/service/UserService.java @@ -28,10 +28,12 @@ import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; +import org.sonar.api.server.authentication.IdentityProvider; import org.sonar.db.DbClient; import org.sonar.db.DbSession; import org.sonar.db.user.UserDto; import org.sonar.db.user.UserQuery; +import org.sonar.server.authentication.IdentityProviderRepository; import org.sonar.server.common.SearchResults; import org.sonar.server.common.avatar.AvatarResolver; import org.sonar.server.common.management.ManagedInstanceChecker; @@ -47,6 +49,7 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Strings.emptyToNull; import static java.util.Comparator.comparing; import static java.util.Objects.requireNonNull; +import static org.sonar.auth.ldap.LdapRealm.LDAP_SECURITY_REALM; import static org.sonar.server.exceptions.NotFoundException.checkFound; import static org.sonar.server.user.ExternalIdentity.SQ_AUTHORITY; @@ -59,6 +62,7 @@ public class UserService { private final ManagedInstanceChecker managedInstanceChecker; private final UserDeactivator userDeactivator; private final UserUpdater userUpdater; + private final IdentityProviderRepository identityProviderRepository; public UserService( DbClient dbClient, @@ -66,13 +70,15 @@ public class UserService { ManagedInstanceService managedInstanceService, ManagedInstanceChecker managedInstanceChecker, UserDeactivator userDeactivator, - UserUpdater userUpdater) { + UserUpdater userUpdater, + IdentityProviderRepository identityProviderRepository) { this.dbClient = dbClient; this.avatarResolver = avatarResolver; this.managedInstanceService = managedInstanceService; this.managedInstanceChecker = managedInstanceChecker; this.userDeactivator = userDeactivator; this.userUpdater = userUpdater; + this.identityProviderRepository = identityProviderRepository; } public SearchResults findUsers(UsersSearchRequest request) { @@ -215,6 +221,7 @@ public class UserService { public UserInformation updateUser(String uuid, UpdateUser updateUser) { try (DbSession dbSession = dbClient.openSession(false)) { + throwIfInvalidChangeOfExternalProvider(updateUser); UserDto userDto = findUserOrThrow(uuid, dbSession); userUpdater.updateAndCommit(dbSession, userDto, updateUser, u -> { }); @@ -236,4 +243,27 @@ public class UserService { return checkFound(dbClient.userDao().selectByUuid(dbSession, uuid), USER_NOT_FOUND_MESSAGE, uuid); } + private void throwIfInvalidChangeOfExternalProvider(UpdateUser updateUser) { + Optional.ofNullable(updateUser.externalIdentityProvider()).ifPresent(this::assertProviderIsSupported); + } + + private void assertProviderIsSupported(String newExternalProvider) { + List allowedIdentityProviders = getAvailableIdentityProviders(); + + boolean isAllowedProvider = allowedIdentityProviders.contains(newExternalProvider) || isLdapIdentityProvider(newExternalProvider); + checkArgument(isAllowedProvider, "Value of 'externalProvider' (%s) must be one of: [%s] or [%s]", newExternalProvider, + String.join(", ", allowedIdentityProviders), String.join(", ", "LDAP", "LDAP_{serverKey}")); + } + + private List getAvailableIdentityProviders() { + return identityProviderRepository.getAllEnabledAndSorted() + .stream() + .map(IdentityProvider::getKey) + .toList(); + } + + private static boolean isLdapIdentityProvider(String identityProviderKey) { + return identityProviderKey.startsWith(LDAP_SECURITY_REALM); + } + } diff --git a/server/sonar-webserver-webapi-v2/src/main/java/org/sonar/server/v2/api/user/controller/DefaultUserController.java b/server/sonar-webserver-webapi-v2/src/main/java/org/sonar/server/v2/api/user/controller/DefaultUserController.java index 3c7a332adf6..c6da3fe0422 100644 --- a/server/sonar-webserver-webapi-v2/src/main/java/org/sonar/server/v2/api/user/controller/DefaultUserController.java +++ b/server/sonar-webserver-webapi-v2/src/main/java/org/sonar/server/v2/api/user/controller/DefaultUserController.java @@ -32,10 +32,10 @@ import org.sonar.server.user.UpdateUser; import org.sonar.server.user.UserSession; import org.sonar.server.v2.api.model.RestPage; import org.sonar.server.v2.api.user.converter.UsersSearchRestResponseGenerator; -import org.sonar.server.v2.api.user.response.UserRestResponse; import org.sonar.server.v2.api.user.request.UserCreateRestRequest; import org.sonar.server.v2.api.user.request.UserUpdateRestRequest; import org.sonar.server.v2.api.user.request.UsersSearchRestRequest; +import org.sonar.server.v2.api.user.response.UserRestResponse; import org.sonar.server.v2.api.user.response.UsersSearchRestResponse; import static org.sonar.server.common.PaginationInformation.forPageIndex; @@ -128,6 +128,8 @@ public class DefaultUserController implements UserController { updateRequest.getName().applyIfDefined(update::setName); updateRequest.getEmail().applyIfDefined(update::setEmail); updateRequest.getScmAccounts().applyIfDefined(update::setScmAccounts); + updateRequest.getExternalProvider().applyIfDefined(update::setExternalIdentityProvider); + updateRequest.getExternalLogin().applyIfDefined(update::setExternalIdentityProviderLogin); return update; } diff --git a/server/sonar-webserver-webapi-v2/src/main/java/org/sonar/server/v2/api/user/request/UserUpdateRestRequest.java b/server/sonar-webserver-webapi-v2/src/main/java/org/sonar/server/v2/api/user/request/UserUpdateRestRequest.java index b38aace4ce8..decc01eb601 100644 --- a/server/sonar-webserver-webapi-v2/src/main/java/org/sonar/server/v2/api/user/request/UserUpdateRestRequest.java +++ b/server/sonar-webserver-webapi-v2/src/main/java/org/sonar/server/v2/api/user/request/UserUpdateRestRequest.java @@ -22,6 +22,7 @@ package org.sonar.server.v2.api.user.request; import io.swagger.v3.oas.annotations.media.ArraySchema; import io.swagger.v3.oas.annotations.media.Schema; import java.util.List; +import javax.annotation.Nullable; import javax.validation.constraints.Email; import javax.validation.constraints.Size; import org.sonar.server.v2.common.model.UpdateField; @@ -32,6 +33,8 @@ public class UserUpdateRestRequest { private UpdateField name = UpdateField.undefined(); private UpdateField email = UpdateField.undefined(); private UpdateField> scmAccounts = UpdateField.undefined(); + private UpdateField externalProvider = UpdateField.undefined(); + private UpdateField externalLogin = UpdateField.undefined(); @Size(min = 2, max = 100) @Schema(description = "User login") @@ -72,4 +75,27 @@ public class UserUpdateRestRequest { public void setScmAccounts(List scmAccounts) { this.scmAccounts = UpdateField.withValue(scmAccounts); } + + @Schema(implementation = String.class, description = "New external provider. Only authentication system installed are available. " + + "Use 'LDAP' identity provider for single server LDAP setup. " + + "Use 'LDAP_{serverKey}' identity provider for multiple LDAP servers setup. " + + "Warning: when this information has been updated for a user, the user will only be able to authenticate via the new identity provider. " + + "It is not possible to migrate external user to local one.") + public UpdateField getExternalProvider() { + return externalProvider; + } + + public void setExternalProvider(@Nullable String externalProvider) { + this.externalProvider = UpdateField.withValue(externalProvider); + } + + @Size(min = 1, max = 255) + @Schema(implementation = String.class, description = "New external login, usually the login used in the authentication system. If not provided previous identity will be used.") + public UpdateField getExternalLogin() { + return externalLogin; + } + + public void setExternalLogin(@Nullable String externalLogin) { + this.externalLogin = UpdateField.withValue(externalLogin); + } } diff --git a/server/sonar-webserver-webapi-v2/src/test/java/org/sonar/server/v2/api/user/controller/DefaultUserControllerTest.java b/server/sonar-webserver-webapi-v2/src/test/java/org/sonar/server/v2/api/user/controller/DefaultUserControllerTest.java index 8ce64276939..d3d4b4a675f 100644 --- a/server/sonar-webserver-webapi-v2/src/test/java/org/sonar/server/v2/api/user/controller/DefaultUserControllerTest.java +++ b/server/sonar-webserver-webapi-v2/src/test/java/org/sonar/server/v2/api/user/controller/DefaultUserControllerTest.java @@ -502,6 +502,17 @@ public class DefaultUserControllerTest { assertThat(userUpdate.login()).isEqualTo("newLogin"); } + @Test + public void updateUser_whenExternalProviderIsProvided_shouldUpdate() throws Exception { + UpdateUser userUpdate = performPatchCallAndVerifyResponse("{\"externalProvider\":\"newExternalProvider\"}"); + assertThat(userUpdate.externalIdentityProvider()).isEqualTo("newExternalProvider"); + } + @Test + public void updateUser_whenExternalProviderLoginIsProvided_shouldUpdate() throws Exception { + UpdateUser userUpdate = performPatchCallAndVerifyResponse("{\"externalLogin\":\"newExternalProviderLogin\"}"); + assertThat(userUpdate.externalIdentityProviderLogin()).isEqualTo("newExternalProviderLogin"); + } + @Test public void updateUser_whenLoginIsEmpty_shouldReturnBadRequest() throws Exception { performPatchCallAndExpectBadRequest("{\"login\":\"\"}", "Value for field login was rejected. Error: size must be between 2 and 100."); @@ -523,6 +534,8 @@ public class DefaultUserControllerTest { private UpdateUser performPatchCallAndVerifyResponse(String payload) throws Exception { userSession.logIn().setSystemAdministrator(); + UserInformation mock = mock(); + when(userService.fetchUser("userUuid")).thenReturn(mock); UserInformation userInformation = generateUserSearchResult("1", true, true, false, 1, 2); when(userService.updateUser(eq("userUuid"), any())).thenReturn(userInformation); diff --git a/server/sonar-webserver-webapi/src/it/java/org/sonar/server/user/ws/CreateActionIT.java b/server/sonar-webserver-webapi/src/it/java/org/sonar/server/user/ws/CreateActionIT.java index 437e978f52f..88c1a1766c2 100644 --- a/server/sonar-webserver-webapi/src/it/java/org/sonar/server/user/ws/CreateActionIT.java +++ b/server/sonar-webserver-webapi/src/it/java/org/sonar/server/user/ws/CreateActionIT.java @@ -33,6 +33,7 @@ import org.sonar.db.audit.NoOpAuditPersister; import org.sonar.db.user.GroupDto; import org.sonar.db.user.UserDto; import org.sonar.server.authentication.CredentialsLocalAuthentication; +import org.sonar.server.authentication.IdentityProviderRepository; import org.sonar.server.common.avatar.AvatarResolverImpl; import org.sonar.server.common.management.ManagedInstanceChecker; import org.sonar.server.common.user.UserDeactivator; @@ -77,8 +78,9 @@ public class CreateActionIT { private final ManagedInstanceChecker managedInstanceChecker = mock(ManagedInstanceChecker.class); private final ManagedInstanceService managedInstanceService = mock(ManagedInstanceService.class); + private final IdentityProviderRepository identityProviderRepository = mock(); private final UserService userService = new UserService(db.getDbClient(), new AvatarResolverImpl(), managedInstanceService, managedInstanceChecker, mock(UserDeactivator.class), - new UserUpdater(mock(NewUserNotifier.class), db.getDbClient(), new DefaultGroupFinder(db.getDbClient()), settings.asConfig(), new NoOpAuditPersister(), localAuthentication)); + new UserUpdater(mock(NewUserNotifier.class), db.getDbClient(), new DefaultGroupFinder(db.getDbClient()), settings.asConfig(), new NoOpAuditPersister(), localAuthentication), identityProviderRepository); private final WsActionTester tester = new WsActionTester(new CreateAction(userSessionRule, managedInstanceChecker, userService)); @Before diff --git a/server/sonar-webserver-webapi/src/it/java/org/sonar/server/user/ws/DeactivateActionIT.java b/server/sonar-webserver-webapi/src/it/java/org/sonar/server/user/ws/DeactivateActionIT.java index 5c11094464e..08685bc9b62 100644 --- a/server/sonar-webserver-webapi/src/it/java/org/sonar/server/user/ws/DeactivateActionIT.java +++ b/server/sonar-webserver-webapi/src/it/java/org/sonar/server/user/ws/DeactivateActionIT.java @@ -44,6 +44,7 @@ import org.sonar.db.user.GroupDto; import org.sonar.db.user.SessionTokenDto; import org.sonar.db.user.UserDismissedMessageDto; import org.sonar.db.user.UserDto; +import org.sonar.server.authentication.IdentityProviderRepository; import org.sonar.server.common.avatar.AvatarResolver; import org.sonar.server.common.management.ManagedInstanceChecker; import org.sonar.server.common.user.UserAnonymizer; @@ -85,9 +86,9 @@ public class DeactivateActionIT { private final UserAnonymizer userAnonymizer = new UserAnonymizer(db.getDbClient(), () -> "anonymized"); private final UserDeactivator userDeactivator = new UserDeactivator(dbClient, userAnonymizer); private final ManagedInstanceChecker managedInstanceChecker = mock(ManagedInstanceChecker.class); - + private final IdentityProviderRepository identityProviderRepository = mock(); private final UserService userService = new UserService(dbClient, mock(AvatarResolver.class), mock(ManagedInstanceService.class), managedInstanceChecker, userDeactivator, - mock(UserUpdater.class)); + mock(UserUpdater.class), identityProviderRepository); private final WsActionTester ws = new WsActionTester(new DeactivateAction(dbClient, userSession, new UserJsonWriter(userSession), userService)); @Test diff --git a/server/sonar-webserver-webapi/src/it/java/org/sonar/server/user/ws/SearchActionIT.java b/server/sonar-webserver-webapi/src/it/java/org/sonar/server/user/ws/SearchActionIT.java index b077cbf5eda..972e08e521f 100644 --- a/server/sonar-webserver-webapi/src/it/java/org/sonar/server/user/ws/SearchActionIT.java +++ b/server/sonar-webserver-webapi/src/it/java/org/sonar/server/user/ws/SearchActionIT.java @@ -36,6 +36,7 @@ import org.sonar.db.DbTester; import org.sonar.db.scim.ScimUserDao; import org.sonar.db.user.GroupDto; import org.sonar.db.user.UserDto; +import org.sonar.server.authentication.IdentityProviderRepository; import org.sonar.server.common.avatar.AvatarResolverImpl; import org.sonar.server.common.management.ManagedInstanceChecker; import org.sonar.server.common.user.UserDeactivator; @@ -84,7 +85,8 @@ public class SearchActionIT { managedInstanceService, mock(ManagedInstanceChecker.class), mock(UserDeactivator.class), - mock(UserUpdater.class)); + mock(UserUpdater.class), + mock(IdentityProviderRepository.class)); private final SearchWsReponseGenerator searchWsReponseGenerator = new SearchWsReponseGenerator(userSession); diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/user/ws/UpdateIdentityProviderAction.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/user/ws/UpdateIdentityProviderAction.java index b5677e99868..cad8de36c43 100644 --- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/user/ws/UpdateIdentityProviderAction.java +++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/user/ws/UpdateIdentityProviderAction.java @@ -20,6 +20,7 @@ package org.sonar.server.user.ws; import java.util.List; +import java.util.Optional; import javax.annotation.Nullable; import org.sonar.api.server.authentication.IdentityProvider; import org.sonar.api.server.ws.Change; @@ -31,9 +32,8 @@ import org.sonar.db.DbClient; import org.sonar.db.DbSession; import org.sonar.db.user.UserDto; import org.sonar.server.authentication.IdentityProviderRepository; -import org.sonar.server.exceptions.NotFoundException; import org.sonar.server.common.management.ManagedInstanceChecker; -import org.sonar.server.user.ExternalIdentity; +import org.sonar.server.exceptions.NotFoundException; import org.sonar.server.user.UpdateUser; import org.sonar.server.user.UserSession; import org.sonar.server.user.UserUpdater; @@ -88,7 +88,7 @@ public class UpdateIdentityProviderAction implements UsersWsAction { .setRequired(true) .setDescription("New external provider. Only authentication system installed are available. " + "Use 'LDAP' identity provider for single server LDAP setup." + - "User 'LDAP_{serverKey}' identity provider for multiple LDAP server setup."); + "Use 'LDAP_{serverKey}' identity provider for multiple LDAP servers setup."); action.setChangelog( new Change("9.8", String.format("Use of 'sonarqube' for the value of '%s' is deprecated.", PARAM_NEW_EXTERNAL_PROVIDER))); @@ -111,8 +111,8 @@ public class UpdateIdentityProviderAction implements UsersWsAction { checkEnabledIdentityProviders(request.newExternalProvider); try (DbSession dbSession = dbClient.openSession(false)) { UserDto user = getUser(dbSession, request.login); - ExternalIdentity externalIdentity = getExternalIdentity(request, user); - userUpdater.updateAndCommit(dbSession, user, new UpdateUser().setExternalIdentity(externalIdentity), u -> { + UpdateUser updateUser = toUpdateUser(request, user); + userUpdater.updateAndCommit(dbSession, user, updateUser, u -> { }); } } @@ -133,7 +133,7 @@ public class UpdateIdentityProviderAction implements UsersWsAction { } private static boolean isLdapIdentityProvider(String identityProviderKey) { - return identityProviderKey.startsWith(LDAP_SECURITY_REALM + "_"); + return identityProviderKey.startsWith(LDAP_SECURITY_REALM); } private UserDto getUser(DbSession dbSession, String login) { @@ -144,11 +144,11 @@ public class UpdateIdentityProviderAction implements UsersWsAction { return user; } - private static ExternalIdentity getExternalIdentity(UpdateIdentityProviderRequest request, UserDto user) { - return new ExternalIdentity( - request.newExternalProvider, - request.newExternalIdentity != null ? request.newExternalIdentity : user.getExternalLogin(), - null); + private static UpdateUser toUpdateUser(UpdateIdentityProviderRequest request, UserDto user) { + return new UpdateUser() + .setExternalIdentityProvider(request.newExternalProvider) + .setExternalIdentityProviderLogin(Optional.ofNullable(request.newExternalIdentity).orElse(user.getExternalLogin()) + ); } private static UpdateIdentityProviderRequest toWsRequest(Request request) {