From: Jean-Baptiste Lievremont Date: Mon, 4 May 2015 09:47:51 +0000 (+0200) Subject: SONAR-6467 Change behavior of user update WS X-Git-Tag: 5.2-RC1~2024 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=974af974d1dd3b2abec32d4287ed696cedd16a30;p=sonarqube.git SONAR-6467 Change behavior of user update WS --- diff --git a/server/sonar-server/src/main/java/org/sonar/server/user/ws/UpdateAction.java b/server/sonar-server/src/main/java/org/sonar/server/user/ws/UpdateAction.java index 565781e35d0..1ab8413d536 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/user/ws/UpdateAction.java +++ b/server/sonar-server/src/main/java/org/sonar/server/user/ws/UpdateAction.java @@ -34,8 +34,6 @@ import org.sonar.server.user.index.UserIndex; public class UpdateAction implements BaseUsersWsAction { private static final String PARAM_LOGIN = "login"; - private static final String PARAM_PASSWORD = "password"; - private static final String PARAM_PASSWORD_CONFIRMATION = "password_confirmation"; private static final String PARAM_NAME = "name"; private static final String PARAM_EMAIL = "email"; private static final String PARAM_SCM_ACCOUNTS = "scm_accounts"; @@ -51,29 +49,19 @@ public class UpdateAction implements BaseUsersWsAction { @Override public void define(WebService.NewController controller) { WebService.NewAction action = controller.createAction("update") - .setDescription("Update a user. Requires Administer System permission") + .setDescription("Update a user. Requires Administer System permission. Since 5.2, a user's password can only be changed using the 'change_password' action.") .setSince("3.7") .setPost(true) - .setHandler(this); + .setHandler(this) + .setResponseExample(getClass().getResource("example-update.json")); action.createParam(PARAM_LOGIN) .setDescription("User login") .setRequired(true) .setExampleValue("myuser"); - action.createParam(PARAM_PASSWORD) - .setDescription("User password") - .setRequired(true) - .setExampleValue("mypassword"); - - action.createParam(PARAM_PASSWORD_CONFIRMATION) - .setDescription("Must be the same value as \"password\"") - .setRequired(true) - .setExampleValue("mypassword"); - action.createParam(PARAM_NAME) .setDescription("User name") - .setRequired(true) .setExampleValue("My Name"); action.createParam(PARAM_EMAIL) @@ -100,12 +88,6 @@ public class UpdateAction implements BaseUsersWsAction { if (request.hasParam(PARAM_SCM_ACCOUNTS)) { updateUser.setScmAccounts(request.paramAsStrings(PARAM_SCM_ACCOUNTS)); } - if (request.hasParam(PARAM_PASSWORD)) { - updateUser.setPassword(request.mandatoryParam(PARAM_PASSWORD)); - } - if (request.hasParam(PARAM_PASSWORD_CONFIRMATION)) { - updateUser.setPasswordConfirmation(request.mandatoryParam(PARAM_PASSWORD_CONFIRMATION)); - } userUpdater.update(updateUser); writeResponse(response, login); diff --git a/server/sonar-server/src/main/resources/org/sonar/server/user/ws/example-update.json b/server/sonar-server/src/main/resources/org/sonar/server/user/ws/example-update.json new file mode 100644 index 00000000000..f423f9ee855 --- /dev/null +++ b/server/sonar-server/src/main/resources/org/sonar/server/user/ws/example-update.json @@ -0,0 +1,9 @@ +{ + "user": { + "login": "ada.lovelace", + "name": "Ada Lovelace", + "email": "ada.lovelace@noteg.com", + "scmAccounts": ["ada.lovelace"], + "active": true + } +} 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 4a6ed1ee953..450dd4e2371 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 @@ -20,182 +20,149 @@ package org.sonar.server.user.ws; +import org.junit.After; import org.junit.Before; +import org.junit.ClassRule; import org.junit.Test; -import org.junit.runner.RunWith; -import org.mockito.ArgumentCaptor; -import org.mockito.Captor; -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; import org.sonar.core.permission.GlobalPermissions; +import org.sonar.core.persistence.DbSession; +import org.sonar.core.persistence.DbTester; +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.ForbiddenException; import org.sonar.server.user.MockUserSession; -import org.sonar.server.user.UpdateUser; +import org.sonar.server.user.NewUserNotifier; import org.sonar.server.user.UserUpdater; -import org.sonar.server.user.index.UserDoc; +import org.sonar.server.user.db.GroupDao; +import org.sonar.server.user.db.UserDao; +import org.sonar.server.user.db.UserGroupDao; import org.sonar.server.user.index.UserIndex; +import org.sonar.server.user.index.UserIndexDefinition; +import org.sonar.server.user.index.UserIndexer; import org.sonar.server.ws.WsTester; -import java.util.Map; - import static com.google.common.collect.Lists.newArrayList; -import static com.google.common.collect.Maps.newHashMap; -import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.mock; -@RunWith(MockitoJUnitRunner.class) public class UpdateActionTest { + static final Settings settings = new Settings().setProperty("sonar.defaultGroup", "sonar-users"); + + @ClassRule + public static final DbTester dbTester = new DbTester(); + + @ClassRule + public static final EsTester esTester = new EsTester().addDefinitions(new UserIndexDefinition(settings)); + WebService.Controller controller; WsTester tester; - @Mock UserIndex index; - @Mock - UserUpdater updater; + DbClient dbClient; - @Captor - ArgumentCaptor userCaptor; + UserIndexer userIndexer; + + DbSession session; @Before public void setUp() throws Exception { - tester = new WsTester(new UsersWs(new UpdateAction(index, updater))); + dbTester.truncateTables(); + esTester.truncateIndices(); + + System2 system2 = new System2(); + UserDao userDao = new UserDao(dbTester.myBatis(), system2); + UserGroupDao userGroupDao = new UserGroupDao(); + GroupDao groupDao = new GroupDao(); + dbClient = new DbClient(dbTester.database(), dbTester.myBatis(), userDao, userGroupDao, groupDao); + session = dbClient.openSession(false); + groupDao.insert(session, new GroupDto().setName("sonar-users")); + session.commit(); + + 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)))); controller = tester.controller("api/users"); + MockUserSession.set().setLogin("admin").setGlobalPermissions(GlobalPermissions.SYSTEM_ADMIN); } + @After + public void tearDown() throws Exception { + session.close(); + } + + @Test(expected = ForbiddenException.class) + public void fail_on_missing_permission() throws Exception { + createUser(); + + MockUserSession.set().setLogin("polop"); + tester.newPostRequest("api/users", "update") + .setParam("login", "john") + .execute(); + } + @Test public void update_user() throws Exception { - Map userDocMap = newHashMap(); - userDocMap.put("login", "john"); - userDocMap.put("name", "John"); - userDocMap.put("email", "john@email.com"); - userDocMap.put("scmAccounts", newArrayList("jn")); - userDocMap.put("active", true); - userDocMap.put("createdAt", 15000L); - userDocMap.put("updatedAt", 15000L); - when(index.getByLogin("john")).thenReturn(new UserDoc(userDocMap)); + createUser(); tester.newPostRequest("api/users", "update") .setParam("login", "john") - .setParam("name", "John") - .setParam("email", "john@email.com") - .setParam("scm_accounts", "jn") - .setParam("password", "1234") - .setParam("password_confirmation", "1234") + .setParam("name", "Jon Snow") + .setParam("email", "jon.snow@thegreatw.all") + .setParam("scm_accounts", "jon.snow") .execute() .assertJson(getClass(), "update_user.json"); - - verify(updater).update(userCaptor.capture()); - assertThat(userCaptor.getValue().login()).isEqualTo("john"); - assertThat(userCaptor.getValue().name()).isEqualTo("John"); - assertThat(userCaptor.getValue().email()).isEqualTo("john@email.com"); - assertThat(userCaptor.getValue().scmAccounts()).containsOnly("jn"); - assertThat(userCaptor.getValue().password()).isEqualTo("1234"); - assertThat(userCaptor.getValue().passwordConfirmation()).isEqualTo("1234"); } @Test public void update_only_name() throws Exception { - Map userDocMap = newHashMap(); - userDocMap.put("login", "john"); - userDocMap.put("name", "John"); - userDocMap.put("email", "john@email.com"); - userDocMap.put("scmAccounts", newArrayList("jn")); - userDocMap.put("active", true); - userDocMap.put("createdAt", 15000L); - userDocMap.put("updatedAt", 15000L); - when(index.getByLogin("john")).thenReturn(new UserDoc(userDocMap)); + createUser(); tester.newPostRequest("api/users", "update") .setParam("login", "john") - .setParam("name", "John") - .execute(); - - verify(updater).update(userCaptor.capture()); - assertThat(userCaptor.getValue().isNameChanged()).isTrue(); - assertThat(userCaptor.getValue().isEmailChanged()).isFalse(); - assertThat(userCaptor.getValue().isScmAccountsChanged()).isFalse(); - assertThat(userCaptor.getValue().isPasswordChanged()).isFalse(); - assertThat(userCaptor.getValue().isPasswordChanged()).isFalse(); + .setParam("name", "Jon Snow") + .execute() + .assertJson(getClass(), "update_name.json"); } @Test public void update_only_email() throws Exception { - Map userDocMap = newHashMap(); - userDocMap.put("login", "john"); - userDocMap.put("name", "John"); - userDocMap.put("email", "john@email.com"); - userDocMap.put("scmAccounts", newArrayList("jn")); - userDocMap.put("active", true); - userDocMap.put("createdAt", 15000L); - userDocMap.put("updatedAt", 15000L); - when(index.getByLogin("john")).thenReturn(new UserDoc(userDocMap)); + createUser(); tester.newPostRequest("api/users", "update") .setParam("login", "john") - .setParam("email", "john@email.com") - .execute(); - - verify(updater).update(userCaptor.capture()); - assertThat(userCaptor.getValue().isNameChanged()).isFalse(); - assertThat(userCaptor.getValue().isEmailChanged()).isTrue(); - assertThat(userCaptor.getValue().isScmAccountsChanged()).isFalse(); - assertThat(userCaptor.getValue().isPasswordChanged()).isFalse(); - assertThat(userCaptor.getValue().isPasswordChanged()).isFalse(); + .setParam("email", "jon.snow@thegreatw.all") + .execute() + .assertJson(getClass(), "update_email.json"); } @Test public void update_only_scm_accounts() throws Exception { - Map userDocMap = newHashMap(); - userDocMap.put("login", "john"); - userDocMap.put("name", "John"); - userDocMap.put("email", "john@email.com"); - userDocMap.put("scmAccounts", newArrayList("jn")); - userDocMap.put("active", true); - userDocMap.put("createdAt", 15000L); - userDocMap.put("updatedAt", 15000L); - when(index.getByLogin("john")).thenReturn(new UserDoc(userDocMap)); + createUser(); tester.newPostRequest("api/users", "update") .setParam("login", "john") - .setParam("scm_accounts", "jn") - .execute(); - - verify(updater).update(userCaptor.capture()); - assertThat(userCaptor.getValue().isNameChanged()).isFalse(); - assertThat(userCaptor.getValue().isEmailChanged()).isFalse(); - assertThat(userCaptor.getValue().isScmAccountsChanged()).isTrue(); - assertThat(userCaptor.getValue().isPasswordChanged()).isFalse(); - assertThat(userCaptor.getValue().isPasswordChanged()).isFalse(); + .setParam("scm_accounts", "jon.snow") + .execute() + .assertJson(getClass(), "update_scm_accounts.json"); } - @Test - public void update_only_password() throws Exception { - Map userDocMap = newHashMap(); - userDocMap.put("login", "john"); - userDocMap.put("name", "John"); - userDocMap.put("email", "john@email.com"); - userDocMap.put("scmAccounts", newArrayList("jn")); - userDocMap.put("active", true); - userDocMap.put("createdAt", 15000L); - userDocMap.put("updatedAt", 15000L); - when(index.getByLogin("john")).thenReturn(new UserDoc(userDocMap)); - - tester.newPostRequest("api/users", "update") - .setParam("login", "john") - .setParam("password", "1234") - .setParam("password_confirmation", "1234") - .execute(); - - verify(updater).update(userCaptor.capture()); - assertThat(userCaptor.getValue().isNameChanged()).isFalse(); - assertThat(userCaptor.getValue().isEmailChanged()).isFalse(); - assertThat(userCaptor.getValue().isScmAccountsChanged()).isFalse(); - assertThat(userCaptor.getValue().isPasswordChanged()).isTrue(); - assertThat(userCaptor.getValue().isPasswordChanged()).isTrue(); + private void createUser() { + dbClient.userDao().insert(session, new UserDto() + .setEmail("john@email.com") + .setLogin("john") + .setName("John") + .setScmAccounts(newArrayList("jn")) + .setActive(true)); + session.commit(); + userIndexer.index(); } } diff --git a/server/sonar-server/src/test/java/org/sonar/server/user/ws/UsersWsTest.java b/server/sonar-server/src/test/java/org/sonar/server/user/ws/UsersWsTest.java index 75f3f8ffc2b..3ab08ea313e 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/user/ws/UsersWsTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/user/ws/UsersWsTest.java @@ -77,7 +77,7 @@ public class UsersWsTest { WebService.Action action = controller.action("update"); assertThat(action).isNotNull(); assertThat(action.isPost()).isTrue(); - assertThat(action.params()).hasSize(6); + assertThat(action.params()).hasSize(4); } @Test diff --git a/server/sonar-server/src/test/resources/org/sonar/server/user/ws/UpdateActionTest/update_email.json b/server/sonar-server/src/test/resources/org/sonar/server/user/ws/UpdateActionTest/update_email.json new file mode 100644 index 00000000000..32461ebcf9a --- /dev/null +++ b/server/sonar-server/src/test/resources/org/sonar/server/user/ws/UpdateActionTest/update_email.json @@ -0,0 +1,11 @@ +{ + "user": { + "login": "john", + "name": "John", + "email": "jon.snow@thegreatw.all", + "active": true, + "scmAccounts": [ + "jn" + ] + } +} diff --git a/server/sonar-server/src/test/resources/org/sonar/server/user/ws/UpdateActionTest/update_name.json b/server/sonar-server/src/test/resources/org/sonar/server/user/ws/UpdateActionTest/update_name.json new file mode 100644 index 00000000000..f0d7b9297ee --- /dev/null +++ b/server/sonar-server/src/test/resources/org/sonar/server/user/ws/UpdateActionTest/update_name.json @@ -0,0 +1,11 @@ +{ + "user": { + "login": "john", + "name": "Jon Snow", + "email": "john@email.com", + "active": true, + "scmAccounts": [ + "jn" + ] + } +} diff --git a/server/sonar-server/src/test/resources/org/sonar/server/user/ws/UpdateActionTest/update_scm_accounts.json b/server/sonar-server/src/test/resources/org/sonar/server/user/ws/UpdateActionTest/update_scm_accounts.json new file mode 100644 index 00000000000..311a8ca6319 --- /dev/null +++ b/server/sonar-server/src/test/resources/org/sonar/server/user/ws/UpdateActionTest/update_scm_accounts.json @@ -0,0 +1,11 @@ +{ + "user": { + "login": "john", + "name": "John", + "email": "john@email.com", + "active": true, + "scmAccounts": [ + "jon.snow" + ] + } +} diff --git a/server/sonar-server/src/test/resources/org/sonar/server/user/ws/UpdateActionTest/update_user.json b/server/sonar-server/src/test/resources/org/sonar/server/user/ws/UpdateActionTest/update_user.json index 0feda7c9c1c..72938943557 100644 --- a/server/sonar-server/src/test/resources/org/sonar/server/user/ws/UpdateActionTest/update_user.json +++ b/server/sonar-server/src/test/resources/org/sonar/server/user/ws/UpdateActionTest/update_user.json @@ -1,9 +1,9 @@ { "user": { "login": "john", - "name": "John", - "email": "john@email.com", - "scmAccounts": ["jn"], + "name": "Jon Snow", + "email": "jon.snow@thegreatw.all", + "scmAccounts": ["jon.snow"], "active": true } }