From 51ca401244b0e9da3fb32f5bfd9ef8cc61806221 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Lievremont Date: Wed, 20 May 2015 16:54:35 +0200 Subject: [PATCH] SONAR-6468 Restore unprivileged password change * Check if external authentication is in use (currently done on UI side) * Allow a user to change their own password --- .../server/user/SecurityRealmFactory.java | 6 +++++ .../org/sonar/server/user/UserUpdater.java | 27 ++++++++++++------- .../server/user/ws/ChangePasswordAction.java | 1 + .../server/user/SecurityRealmFactoryTest.java | 2 ++ .../sonar/server/user/UserUpdaterTest.java | 22 ++++++++++++++- .../user/ws/ChangePasswordActionTest.java | 25 ++++++++++++++++- .../server/user/ws/CreateActionTest.java | 6 ++--- .../server/user/ws/DeactivateActionTest.java | 3 ++- .../server/user/ws/UpdateActionTest.java | 3 ++- .../resources/org/sonar/l10n/core.properties | 1 + 10 files changed, 79 insertions(+), 17 deletions(-) diff --git a/server/sonar-server/src/main/java/org/sonar/server/user/SecurityRealmFactory.java b/server/sonar-server/src/main/java/org/sonar/server/user/SecurityRealmFactory.java index 78c4aa632a6..cf3a0ef3405 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/user/SecurityRealmFactory.java +++ b/server/sonar-server/src/main/java/org/sonar/server/user/SecurityRealmFactory.java @@ -19,6 +19,7 @@ */ package org.sonar.server.user; +import javax.annotation.Nullable; import org.apache.commons.lang.StringUtils; import org.picocontainer.Startable; import org.sonar.api.CoreProperties; @@ -97,10 +98,15 @@ public class SecurityRealmFactory implements Startable { // nothing } + @Nullable public SecurityRealm getRealm() { return realm; } + public boolean hasExternalAuthentication() { + return getRealm() != null; + } + private static SecurityRealm selectRealm(SecurityRealm[] realms, String realmName) { for (SecurityRealm realm : realms) { if (StringUtils.equals(realmName, realm.getName())) { diff --git a/server/sonar-server/src/main/java/org/sonar/server/user/UserUpdater.java b/server/sonar-server/src/main/java/org/sonar/server/user/UserUpdater.java index 9683ecc1485..f10432b3769 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/user/UserUpdater.java +++ b/server/sonar-server/src/main/java/org/sonar/server/user/UserUpdater.java @@ -24,12 +24,18 @@ import com.google.common.base.Joiner; import com.google.common.base.Predicate; import com.google.common.base.Strings; import com.google.common.collect.Iterables; +import java.security.SecureRandom; +import java.util.Arrays; +import java.util.List; +import java.util.Random; +import javax.annotation.CheckForNull; +import javax.annotation.Nullable; import org.apache.commons.codec.digest.DigestUtils; import org.apache.commons.lang.StringUtils; import org.sonar.api.CoreProperties; -import org.sonar.api.server.ServerSide; import org.sonar.api.config.Settings; import org.sonar.api.platform.NewUserHandler; +import org.sonar.api.server.ServerSide; import org.sonar.api.utils.System2; import org.sonar.core.persistence.DbSession; import org.sonar.core.user.GroupDto; @@ -41,14 +47,6 @@ import org.sonar.server.exceptions.Message; import org.sonar.server.user.index.UserIndexer; import org.sonar.server.util.Validation; -import javax.annotation.CheckForNull; -import javax.annotation.Nullable; - -import java.security.SecureRandom; -import java.util.Arrays; -import java.util.List; -import java.util.Random; - import static com.google.common.collect.Lists.newArrayList; @ServerSide @@ -65,13 +63,15 @@ public class UserUpdater { private final DbClient dbClient; private final UserIndexer userIndexer; private final System2 system2; + private final SecurityRealmFactory realmFactory; - public UserUpdater(NewUserNotifier newUserNotifier, Settings settings, DbClient dbClient, UserIndexer userIndexer, System2 system2) { + public UserUpdater(NewUserNotifier newUserNotifier, Settings settings, DbClient dbClient, UserIndexer userIndexer, System2 system2, SecurityRealmFactory realmFactory) { this.newUserNotifier = newUserNotifier; this.settings = settings; this.dbClient = dbClient; this.userIndexer = userIndexer; this.system2 = system2; + this.realmFactory = realmFactory; } /** @@ -182,6 +182,7 @@ public class UserUpdater { String password = updateUser.password(); String passwordConfirmation = updateUser.passwordConfirmation(); if (updateUser.isPasswordChanged()) { + checkPasswordChangeAllowed(messages); validatePasswords(password, passwordConfirmation, messages); setEncryptedPassWord(password, userDto); } @@ -233,6 +234,12 @@ public class UserUpdater { } } + private void checkPasswordChangeAllowed(List messages) { + if (realmFactory.hasExternalAuthentication()) { + messages.add(Message.of("user.password_cant_be_changed_on_external_auth")); + } + } + private static void validatePasswords(@Nullable String password, @Nullable String passwordConfirmation, List messages) { checkNotEmptyParam(password, PASSWORD_PARAM, messages); checkNotEmptyParam(passwordConfirmation, PASSWORD_CONFIRMATION_PARAM, messages); diff --git a/server/sonar-server/src/main/java/org/sonar/server/user/ws/ChangePasswordAction.java b/server/sonar-server/src/main/java/org/sonar/server/user/ws/ChangePasswordAction.java index 76d2d61596e..c59560a786b 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/user/ws/ChangePasswordAction.java +++ b/server/sonar-server/src/main/java/org/sonar/server/user/ws/ChangePasswordAction.java @@ -45,6 +45,7 @@ public class ChangePasswordAction implements UsersWsAction { public void define(WebService.NewController controller) { WebService.NewAction action = controller.createAction("change_password") .setDescription("Update a user's password. Authenticated users can change their own password, " + + "provided that the account is not linked to an external authentication system. " + "Administer System permission is required to change another user's password.") .setSince("5.2") .setPost(true) diff --git a/server/sonar-server/src/test/java/org/sonar/server/user/SecurityRealmFactoryTest.java b/server/sonar-server/src/test/java/org/sonar/server/user/SecurityRealmFactoryTest.java index d97ddbc314e..ab23323e5e2 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/user/SecurityRealmFactoryTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/user/SecurityRealmFactoryTest.java @@ -46,6 +46,7 @@ public class SecurityRealmFactoryTest { SecurityRealmFactory factory = new SecurityRealmFactory(settings, new SecurityRealm[]{realm}); factory.start(); assertThat(factory.getRealm()).isSameAs(realm); + assertThat(factory.hasExternalAuthentication()).isTrue(); verify(realm).init(); factory.stop(); @@ -56,6 +57,7 @@ public class SecurityRealmFactoryTest { SecurityRealmFactory factory = new SecurityRealmFactory(settings); factory.start(); assertThat(factory.getRealm()).isNull(); + assertThat(factory.hasExternalAuthentication()).isFalse(); } @Test 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 9f17e500e9e..1e9c7fa2b5d 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 @@ -78,6 +78,9 @@ public class UserUpdaterTest { @Mock NewUserNotifier newUserNotifier; + @Mock + SecurityRealmFactory realmFactory; + @Captor ArgumentCaptor newUserHandler; @@ -106,7 +109,7 @@ public class UserUpdaterTest { DbClient dbClient = new DbClient(db.database(), db.myBatis(), userDao, groupDao, userGroupDao); userIndexer = (UserIndexer) new UserIndexer(dbClient, es.client()).setEnabled(true); userUpdater = new UserUpdater(newUserNotifier, settings, dbClient, - userIndexer, system2); + userIndexer, system2, realmFactory); } @After @@ -763,6 +766,23 @@ public class UserUpdaterTest { assertThat(dto.getEmail()).isEqualTo("marius@lesbronzes.fr"); } + @Test + public void fail_to_update_password_when_external_auth_is_used() { + db.prepareDbUnit(getClass(), "update_user.xml"); + createDefaultGroup(); + + when(realmFactory.hasExternalAuthentication()).thenReturn(true); + + try { + userUpdater.update(UpdateUser.create("marius") + .setPassword("password2") + .setPasswordConfirmation("password2")); + } catch (BadRequestException e) { + assertThat(e.errors().messages()).containsOnly(Message.of("user.password_cant_be_changed_on_external_auth")); + } + } + + @Test public void associate_default_group_when_updating_user() { db.prepareDbUnit(getClass(), "associate_default_groups_when_updating_user.xml"); 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 092762c4e2d..38f507b27ba 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 @@ -25,6 +25,9 @@ import org.junit.Before; import org.junit.ClassRule; import org.junit.Rule; import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.runners.MockitoJUnitRunner; import org.sonar.api.config.Settings; import org.sonar.api.server.ws.WebService; import org.sonar.api.utils.System2; @@ -35,10 +38,12 @@ import org.sonar.core.user.GroupDto; import org.sonar.core.user.UserDto; import org.sonar.server.db.DbClient; import org.sonar.server.es.EsTester; +import org.sonar.server.exceptions.BadRequestException; import org.sonar.server.exceptions.ForbiddenException; import org.sonar.server.exceptions.NotFoundException; import org.sonar.server.tester.UserSessionRule; import org.sonar.server.user.NewUserNotifier; +import org.sonar.server.user.SecurityRealmFactory; import org.sonar.server.user.UserUpdater; import org.sonar.server.user.db.GroupDao; import org.sonar.server.user.db.UserDao; @@ -51,7 +56,9 @@ import org.sonar.server.ws.WsTester; import static com.google.common.collect.Lists.newArrayList; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +@RunWith(MockitoJUnitRunner.class) public class ChangePasswordActionTest { static final Settings settings = new Settings().setProperty("sonar.defaultGroup", "sonar-users"); @@ -76,6 +83,9 @@ public class ChangePasswordActionTest { DbSession session; + @Mock + SecurityRealmFactory realmFactory; + @Before public void setUp() { dbTester.truncateTables(); @@ -92,7 +102,8 @@ public class ChangePasswordActionTest { userIndexer = (UserIndexer) new UserIndexer(dbClient, esTester.client()).setEnabled(true); index = new UserIndex(esTester.client()); - tester = new WsTester(new UsersWs(new ChangePasswordAction(new UserUpdater(mock(NewUserNotifier.class), settings, dbClient, userIndexer, system2), userSessionRule))); + tester = new WsTester( + new UsersWs(new ChangePasswordAction(new UserUpdater(mock(NewUserNotifier.class), settings, dbClient, userIndexer, system2, realmFactory), userSessionRule))); controller = tester.controller("api/users"); } @@ -154,6 +165,18 @@ public class ChangePasswordActionTest { assertThat(newPassword).isNotEqualTo(originalPassword); } + @Test(expected = BadRequestException.class) + public void fail_to_update_password_on_external_auth() throws Exception { + createUser(); + session.clearCache(); + when(realmFactory.hasExternalAuthentication()).thenReturn(true); + + tester.newPostRequest("api/users", "change_password") + .setParam("login", "john") + .setParam("password", "Valar Morghulis") + .execute(); + } + private void createUser() { dbClient.userDao().insert(session, new UserDto() .setEmail("john@email.com") diff --git a/server/sonar-server/src/test/java/org/sonar/server/user/ws/CreateActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/user/ws/CreateActionTest.java index 02613a65adc..9c99614d6d8 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/user/ws/CreateActionTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/user/ws/CreateActionTest.java @@ -20,6 +20,7 @@ package org.sonar.server.user.ws; +import java.util.Locale; import org.junit.After; import org.junit.Before; import org.junit.ClassRule; @@ -42,6 +43,7 @@ import org.sonar.server.es.EsTester; import org.sonar.server.exceptions.ForbiddenException; import org.sonar.server.tester.UserSessionRule; import org.sonar.server.user.NewUserNotifier; +import org.sonar.server.user.SecurityRealmFactory; import org.sonar.server.user.UserUpdater; import org.sonar.server.user.db.GroupDao; import org.sonar.server.user.db.UserDao; @@ -52,8 +54,6 @@ import org.sonar.server.user.index.UserIndexDefinition; import org.sonar.server.user.index.UserIndexer; import org.sonar.server.ws.WsTester; -import java.util.Locale; - import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -103,7 +103,7 @@ public class CreateActionTest { userIndexer = (UserIndexer) new UserIndexer(dbClient, esTester.client()).setEnabled(true); index = new UserIndex(esTester.client()); tester = new WsTester(new UsersWs(new CreateAction(index, - new UserUpdater(mock(NewUserNotifier.class), settings, dbClient, userIndexer, system2), + new UserUpdater(mock(NewUserNotifier.class), settings, dbClient, userIndexer, system2, mock(SecurityRealmFactory.class)), i18n, userSessionRule))); controller = tester.controller("api/users"); diff --git a/server/sonar-server/src/test/java/org/sonar/server/user/ws/DeactivateActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/user/ws/DeactivateActionTest.java index c251ad45a03..871672759bb 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/user/ws/DeactivateActionTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/user/ws/DeactivateActionTest.java @@ -43,6 +43,7 @@ import org.sonar.server.exceptions.ForbiddenException; import org.sonar.server.exceptions.NotFoundException; import org.sonar.server.tester.UserSessionRule; import org.sonar.server.user.NewUserNotifier; +import org.sonar.server.user.SecurityRealmFactory; import org.sonar.server.user.UserUpdater; import org.sonar.server.user.db.UserDao; import org.sonar.server.user.index.UserDoc; @@ -95,7 +96,7 @@ public class DeactivateActionTest { userIndexer = (UserIndexer) new UserIndexer(dbClient, esTester.client()).setEnabled(true); index = new UserIndex(esTester.client()); tester = new WsTester(new UsersWs(new DeactivateAction(index, - new UserUpdater(mock(NewUserNotifier.class), settings, dbClient, userIndexer, system2), userSessionRule))); + new UserUpdater(mock(NewUserNotifier.class), settings, dbClient, userIndexer, system2, mock(SecurityRealmFactory.class)), userSessionRule))); controller = tester.controller("api/users"); } diff --git a/server/sonar-server/src/test/java/org/sonar/server/user/ws/UpdateActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/user/ws/UpdateActionTest.java index 5bd83a3cec6..2cce87b68eb 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/user/ws/UpdateActionTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/user/ws/UpdateActionTest.java @@ -38,6 +38,7 @@ import org.sonar.server.es.EsTester; import org.sonar.server.exceptions.ForbiddenException; import org.sonar.server.tester.UserSessionRule; import org.sonar.server.user.NewUserNotifier; +import org.sonar.server.user.SecurityRealmFactory; import org.sonar.server.user.UserUpdater; import org.sonar.server.user.db.GroupDao; import org.sonar.server.user.db.UserDao; @@ -91,7 +92,7 @@ public class UpdateActionTest { userIndexer = (UserIndexer) new UserIndexer(dbClient, esTester.client()).setEnabled(true); index = new UserIndex(esTester.client()); tester = new WsTester(new UsersWs(new UpdateAction(index, - new UserUpdater(mock(NewUserNotifier.class), settings, dbClient, userIndexer, system2), userSessionRule))); + new UserUpdater(mock(NewUserNotifier.class), settings, dbClient, userIndexer, system2, mock(SecurityRealmFactory.class)), userSessionRule))); controller = tester.controller("api/users"); } diff --git a/sonar-core/src/main/resources/org/sonar/l10n/core.properties b/sonar-core/src/main/resources/org/sonar/l10n/core.properties index 83ca78d5b1f..07880341d8b 100644 --- a/sonar-core/src/main/resources/org/sonar/l10n/core.properties +++ b/sonar-core/src/main/resources/org/sonar/l10n/core.properties @@ -2089,6 +2089,7 @@ user.reactivated=The user '{0}' has been reactivated. user.add_scm_account=Add SCM account user.scm_account_already_used=The scm account '{0}' is already used by user(s) : '{1}' user.login_or_email_used_as_scm_account=Login and email are automatically considered as SCM accounts +user.password_cant_be_changed_on_external_auth=Password cannot be changed when external authentication is used #------------------------------------------------------------------------------ -- 2.39.5