From 42c4ba04b0b4748f50420b3eedef2bbc6fe40c8f Mon Sep 17 00:00:00 2001 From: Simon Brandhof Date: Thu, 2 Feb 2017 15:55:18 +0100 Subject: [PATCH] SONAR-8716 fix check of permissions in api/settings --- .../setting/ws/GenerateSecretKeyAction.java | 3 +- .../sonar/server/setting/ws/ResetAction.java | 8 +- .../sonar/server/setting/ws/SetAction.java | 9 +-- .../ws/GenerateSecretKeyActionTest.java | 22 +++--- .../server/setting/ws/ResetActionTest.java | 77 +++++++++---------- .../server/setting/ws/SetActionTest.java | 13 ++-- 6 files changed, 59 insertions(+), 73 deletions(-) diff --git a/server/sonar-server/src/main/java/org/sonar/server/setting/ws/GenerateSecretKeyAction.java b/server/sonar-server/src/main/java/org/sonar/server/setting/ws/GenerateSecretKeyAction.java index 8c2999f6d7f..e897e57ed02 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/setting/ws/GenerateSecretKeyAction.java +++ b/server/sonar-server/src/main/java/org/sonar/server/setting/ws/GenerateSecretKeyAction.java @@ -27,7 +27,6 @@ import org.sonar.api.server.ws.WebService; import org.sonar.server.user.UserSession; import org.sonarqube.ws.Settings.GenerateSecretKeyWsResponse; -import static org.sonar.core.permission.GlobalPermissions.SYSTEM_ADMIN; import static org.sonar.server.ws.WsUtils.writeProtobuf; public class GenerateSecretKeyAction implements SettingsWsAction { @@ -52,7 +51,7 @@ public class GenerateSecretKeyAction implements SettingsWsAction { @Override public void handle(Request request, Response response) throws Exception { - userSession.checkPermission(SYSTEM_ADMIN); + userSession.checkIsRoot(); writeProtobuf(GenerateSecretKeyWsResponse.newBuilder().setSecretKey(settings.getEncryption().generateRandomSecretKey()).build(), request, response); } diff --git a/server/sonar-server/src/main/java/org/sonar/server/setting/ws/ResetAction.java b/server/sonar-server/src/main/java/org/sonar/server/setting/ws/ResetAction.java index 10e76bb1583..5c92dfc2322 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/setting/ws/ResetAction.java +++ b/server/sonar-server/src/main/java/org/sonar/server/setting/ws/ResetAction.java @@ -30,7 +30,6 @@ import org.sonar.api.server.ws.Request; import org.sonar.api.server.ws.Response; import org.sonar.api.server.ws.WebService; import org.sonar.api.web.UserRole; -import org.sonar.core.permission.GlobalPermissions; import org.sonar.core.util.stream.Collectors; import org.sonar.db.DbClient; import org.sonar.db.DbSession; @@ -90,8 +89,7 @@ public class ResetAction implements SettingsWsAction { @Override public void handle(Request request, Response response) throws Exception { - DbSession dbSession = dbClient.openSession(false); - try { + try (DbSession dbSession = dbClient.openSession(false)) { ResetRequest resetRequest = toWsRequest(request); Optional component = getComponent(dbSession, resetRequest); checkPermissions(component); @@ -109,8 +107,6 @@ public class ResetAction implements SettingsWsAction { } dbSession.commit(); response.noContent(); - } finally { - dbClient.closeSession(dbSession); } } @@ -142,7 +138,7 @@ public class ResetAction implements SettingsWsAction { if (component.isPresent()) { userSession.checkComponentPermission(UserRole.ADMIN, component.get()); } else { - userSession.checkPermission(GlobalPermissions.SYSTEM_ADMIN); + userSession.checkIsRoot(); } } } diff --git a/server/sonar-server/src/main/java/org/sonar/server/setting/ws/SetAction.java b/server/sonar-server/src/main/java/org/sonar/server/setting/ws/SetAction.java index 7ac07f678af..3f6161ffc46 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/setting/ws/SetAction.java +++ b/server/sonar-server/src/main/java/org/sonar/server/setting/ws/SetAction.java @@ -45,7 +45,6 @@ import org.sonar.api.server.ws.Request; import org.sonar.api.server.ws.Response; import org.sonar.api.server.ws.WebService; import org.sonar.api.web.UserRole; -import org.sonar.core.permission.GlobalPermissions; import org.sonar.db.DbClient; import org.sonar.db.DbSession; import org.sonar.db.component.ComponentDto; @@ -130,13 +129,9 @@ public class SetAction implements SettingsWsAction { @Override public void handle(Request request, Response response) throws Exception { - DbSession dbSession = dbClient.openSession(false); - try { + try (DbSession dbSession = dbClient.openSession(false)) { doHandle(dbSession, toWsRequest(request)); - } finally { - dbClient.closeSession(dbSession); } - response.noContent(); } @@ -273,7 +268,7 @@ public class SetAction implements SettingsWsAction { if (component.isPresent()) { userSession.checkComponentPermission(UserRole.ADMIN, component.get()); } else { - userSession.checkPermission(GlobalPermissions.SYSTEM_ADMIN); + userSession.checkIsRoot(); } } diff --git a/server/sonar-server/src/test/java/org/sonar/server/setting/ws/GenerateSecretKeyActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/setting/ws/GenerateSecretKeyActionTest.java index 1214adadbc8..27e8dce948a 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/setting/ws/GenerateSecretKeyActionTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/setting/ws/GenerateSecretKeyActionTest.java @@ -40,23 +40,19 @@ import org.sonarqube.ws.MediaTypes; import org.sonarqube.ws.Settings.GenerateSecretKeyWsResponse; import static org.assertj.core.api.Assertions.assertThat; -import static org.sonar.core.permission.GlobalPermissions.QUALITY_PROFILE_ADMIN; -import static org.sonar.core.permission.GlobalPermissions.SYSTEM_ADMIN; public class GenerateSecretKeyActionTest { @Rule public ExpectedException expectedException = ExpectedException.none(); @Rule - public UserSessionRule userSession = UserSessionRule.standalone().setGlobalPermissions(SYSTEM_ADMIN); + public UserSessionRule userSession = UserSessionRule.standalone().login().setRoot(); @Rule public TemporaryFolder temporaryFolder = new TemporaryFolder(); - Settings settings = new MapSettings(); - Encryption encryption = settings.getEncryption(); - - GenerateSecretKeyAction underTest = new GenerateSecretKeyAction(settings, userSession); - - WsActionTester ws = new WsActionTester(underTest); + private Settings settings = new MapSettings(); + private Encryption encryption = settings.getEncryption(); + private GenerateSecretKeyAction underTest = new GenerateSecretKeyAction(settings, userSession); + private WsActionTester ws = new WsActionTester(underTest); @Test public void generate_valid_secret_key() throws IOException { @@ -83,14 +79,16 @@ public class GenerateSecretKeyActionTest { } @Test - public void fail_if_insufficient_permissions() { - expectedException.expect(ForbiddenException.class); + public void throw_ForbiddenException_if_not_root() { + userSession.login(); - userSession.anonymous().setGlobalPermissions(QUALITY_PROFILE_ADMIN); + expectedException.expect(ForbiddenException.class); + expectedException.expectMessage("Insufficient privileges"); call(); } + private GenerateSecretKeyWsResponse call() { TestRequest request = ws.newRequest() .setMediaType(MediaTypes.PROTOBUF) diff --git a/server/sonar-server/src/test/java/org/sonar/server/setting/ws/ResetActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/setting/ws/ResetActionTest.java index 21a0910f947..b0d418329e6 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/setting/ws/ResetActionTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/setting/ws/ResetActionTest.java @@ -29,7 +29,6 @@ import org.sonar.api.config.PropertyDefinition; import org.sonar.api.config.PropertyDefinitions; import org.sonar.api.server.ws.WebService; import org.sonar.api.utils.System2; -import org.sonar.core.permission.GlobalPermissions; import org.sonar.db.DbClient; import org.sonar.db.DbSession; import org.sonar.db.DbTester; @@ -55,7 +54,6 @@ import static org.sonar.api.resources.Qualifiers.PROJECT; import static org.sonar.api.resources.Qualifiers.VIEW; import static org.sonar.api.web.UserRole.ADMIN; import static org.sonar.api.web.UserRole.USER; -import static org.sonar.core.permission.GlobalPermissions.SYSTEM_ADMIN; import static org.sonar.db.component.ComponentTesting.newProjectDto; import static org.sonar.db.property.PropertyTesting.newComponentPropertyDto; import static org.sonar.db.property.PropertyTesting.newGlobalPropertyDto; @@ -72,21 +70,18 @@ public class ResetActionTest { @Rule public DbTester db = DbTester.create(System2.INSTANCE); - I18nRule i18n = new I18nRule(); - - PropertyDbTester propertyDb = new PropertyDbTester(db); - ComponentDbTester componentDb = new ComponentDbTester(db); - DbClient dbClient = db.getDbClient(); - DbSession dbSession = db.getSession(); - ComponentFinder componentFinder = new ComponentFinder(dbClient); - PropertyDefinitions definitions = new PropertyDefinitions(); - SettingsUpdater settingsUpdater = new SettingsUpdater(dbClient, definitions); - SettingValidations settingValidations = new SettingValidations(definitions, dbClient, i18n); - - ComponentDto project; - - ResetAction underTest = new ResetAction(dbClient, componentFinder, settingsUpdater, userSession, definitions, settingValidations); - WsActionTester ws = new WsActionTester(underTest); + private I18nRule i18n = new I18nRule(); + private PropertyDbTester propertyDb = new PropertyDbTester(db); + private ComponentDbTester componentDb = new ComponentDbTester(db); + private DbClient dbClient = db.getDbClient(); + private DbSession dbSession = db.getSession(); + private ComponentFinder componentFinder = new ComponentFinder(dbClient); + private PropertyDefinitions definitions = new PropertyDefinitions(); + private SettingsUpdater settingsUpdater = new SettingsUpdater(dbClient, definitions); + private SettingValidations settingValidations = new SettingValidations(definitions, dbClient, i18n); + private ComponentDto project; + private ResetAction underTest = new ResetAction(dbClient, componentFinder, settingsUpdater, userSession, definitions, settingValidations); + private WsActionTester ws = new WsActionTester(underTest); @Before public void setUp() throws Exception { @@ -95,7 +90,7 @@ public class ResetActionTest { @Test public void remove_global_setting() throws Exception { - setUserAsSystemAdmin(); + logInAsRoot(); definitions.addComponent(PropertyDefinition.builder("foo").build()); propertyDb.insertProperties(newGlobalPropertyDto().setKey("foo").setValue("one")); @@ -105,7 +100,7 @@ public class ResetActionTest { @Test public void remove_global_setting_even_if_not_defined() throws Exception { - setUserAsSystemAdmin(); + logInAsRoot(); propertyDb.insertProperties(newGlobalPropertyDto().setKey("foo").setValue("one")); executeRequestOnGlobalSetting("foo"); @@ -114,7 +109,7 @@ public class ResetActionTest { @Test public void remove_component_setting() throws Exception { - setUserAsProjectAdmin(); + logInAsProjectAdmin(); definitions.addComponent(PropertyDefinition.builder("foo").onQualifiers(PROJECT).build()); propertyDb.insertProperties(newComponentPropertyDto(project).setKey("foo").setValue("value")); @@ -124,7 +119,7 @@ public class ResetActionTest { @Test public void remove_component_setting_even_if_not_defined() throws Exception { - setUserAsProjectAdmin(); + logInAsProjectAdmin(); propertyDb.insertProperties(newComponentPropertyDto(project).setKey("foo").setValue("value")); executeRequestOnProjectSetting("foo"); @@ -133,7 +128,7 @@ public class ResetActionTest { @Test public void remove_hidden_setting() throws Exception { - setUserAsSystemAdmin(); + logInAsRoot(); definitions.addComponent(PropertyDefinition.builder("foo").hidden().build()); propertyDb.insertProperties(newGlobalPropertyDto().setKey("foo").setValue("one")); @@ -143,7 +138,7 @@ public class ResetActionTest { @Test public void ignore_project_setting_when_removing_global_setting() throws Exception { - setUserAsSystemAdmin(); + logInAsRoot(); propertyDb.insertProperties(newGlobalPropertyDto().setKey("foo").setValue("one")); propertyDb.insertProperties(newComponentPropertyDto(project).setKey("foo").setValue("value")); @@ -155,7 +150,7 @@ public class ResetActionTest { @Test public void ignore_global_setting_when_removing_project_setting() throws Exception { - setUserAsProjectAdmin(); + logInAsProjectAdmin(); propertyDb.insertProperties(newGlobalPropertyDto().setKey("foo").setValue("one")); propertyDb.insertProperties(newComponentPropertyDto(project).setKey("foo").setValue("value")); @@ -167,7 +162,7 @@ public class ResetActionTest { @Test public void ignore_user_setting_when_removing_global_setting() throws Exception { - setUserAsSystemAdmin(); + logInAsRoot(); UserDto user = dbClient.userDao().insert(dbSession, UserTesting.newUserDto()); propertyDb.insertProperties(newUserPropertyDto("foo", "one", user)); @@ -177,7 +172,7 @@ public class ResetActionTest { @Test public void ignore_user_setting_when_removing_project_setting() throws Exception { - setUserAsProjectAdmin(); + logInAsProjectAdmin(); UserDto user = dbClient.userDao().insert(dbSession, UserTesting.newUserDto()); propertyDb.insertProperties(newUserPropertyDto("foo", "one", user)); @@ -187,14 +182,14 @@ public class ResetActionTest { @Test public void ignore_unknown_setting_key() throws Exception { - setUserAsSystemAdmin(); + logInAsRoot(); executeRequestOnGlobalSetting("unknown"); } @Test public void remove_setting_by_deprecated_key() throws Exception { - setUserAsSystemAdmin(); + logInAsRoot(); definitions.addComponent(PropertyDefinition.builder("foo").deprecatedKey("old").build()); propertyDb.insertProperties(newGlobalPropertyDto().setKey("foo").setValue("one")); @@ -204,7 +199,7 @@ public class ResetActionTest { @Test public void empty_204_response() { - setUserAsSystemAdmin(); + logInAsRoot(); TestResponse result = ws.newRequest() .setParam("keys", "my.key") .execute(); @@ -224,28 +219,30 @@ public class ResetActionTest { } @Test - public void fail_when_not_system_admin() throws Exception { - userSession.logIn("not-admin").setGlobalPermissions(GlobalPermissions.QUALITY_GATE_ADMIN); + public void throw_ForbiddenException_if_global_setting_and_not_root() throws Exception { + userSession.logIn(); definitions.addComponent(PropertyDefinition.builder("foo").build()); expectedException.expect(ForbiddenException.class); + expectedException.expectMessage("Insufficient privileges"); executeRequestOnGlobalSetting("foo"); } @Test - public void fail_when_not_project_admin() throws Exception { - userSession.logIn("project-admin").addProjectUuidPermissions(USER, project.uuid()); + public void throw_ForbiddenException_if_project_setting_and_not_project_administrator() throws Exception { + userSession.logIn().addProjectUuidPermissions(USER, project.uuid()); definitions.addComponent(PropertyDefinition.builder("foo").build()); expectedException.expect(ForbiddenException.class); + expectedException.expectMessage("Insufficient privileges"); executeRequestOnComponentSetting("foo", project); } @Test public void fail_when_not_global_and_no_component() { - setUserAsSystemAdmin(); + logInAsRoot(); definitions.addComponent(PropertyDefinition.builder("foo") .onlyOnQualifiers(VIEW) .build()); @@ -258,7 +255,7 @@ public class ResetActionTest { @Test public void fail_when_qualifier_not_included() { - setUserAsSystemAdmin(); + logInAsRoot(); definitions.addComponent(PropertyDefinition.builder("foo") .onQualifiers(VIEW) .build()); @@ -272,7 +269,7 @@ public class ResetActionTest { @Test public void fail_to_reset_setting_component_when_setting_is_global() { - setUserAsSystemAdmin(); + logInAsRoot(); definitions.addComponent(PropertyDefinition.builder("foo").build()); i18n.put("qualifier." + PROJECT, "project"); @@ -304,12 +301,12 @@ public class ResetActionTest { request.execute(); } - private void setUserAsSystemAdmin() { - userSession.logIn("admin").setGlobalPermissions(SYSTEM_ADMIN); + private void logInAsRoot() { + userSession.logIn().setRoot(); } - private void setUserAsProjectAdmin() { - userSession.logIn("project-admin").addProjectUuidPermissions(ADMIN, project.uuid()); + private void logInAsProjectAdmin() { + userSession.logIn().addProjectUuidPermissions(ADMIN, project.uuid()); } private void assertGlobalPropertyDoesNotExist(String key) { diff --git a/server/sonar-server/src/test/java/org/sonar/server/setting/ws/SetActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/setting/ws/SetActionTest.java index 8ab347473f9..38bc520db37 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/setting/ws/SetActionTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/setting/ws/SetActionTest.java @@ -37,7 +37,6 @@ import org.sonar.api.server.ws.WebService; import org.sonar.api.server.ws.WebService.Param; import org.sonar.api.utils.System2; import org.sonar.api.web.UserRole; -import org.sonar.core.permission.GlobalPermissions; import org.sonar.db.DbClient; import org.sonar.db.DbSession; import org.sonar.db.DbTester; @@ -73,8 +72,8 @@ public class SetActionTest { @Rule public ExpectedException expectedException = ExpectedException.none(); @Rule - public UserSessionRule userSession = UserSessionRule.standalone() - .setGlobalPermissions(GlobalPermissions.SYSTEM_ADMIN); + public UserSessionRule userSession = UserSessionRule.standalone().login().setRoot(); + @Rule public DbTester db = DbTester.create(System2.INSTANCE); private PropertyDbTester propertyDb = new PropertyDbTester(db); @@ -136,7 +135,7 @@ public class SetActionTest { @Test public void persist_project_property_with_project_admin_permission() { ComponentDto project = db.components().insertProject(); - userSession.anonymous().addProjectUuidPermissions(UserRole.ADMIN, project.uuid()); + userSession.login().addProjectUuidPermissions(UserRole.ADMIN, project.uuid()); callForProjectSettingByKey("my.key", "my value", project.key()); @@ -423,9 +422,11 @@ public class SetActionTest { } @Test - public void fail_when_insufficient_privileges() { - userSession.anonymous().setGlobalPermissions(GlobalPermissions.QUALITY_PROFILE_ADMIN); + public void throw_ForbiddenException_if_not_root() { + userSession.login(); + expectedException.expect(ForbiddenException.class); + expectedException.expectMessage("Insufficient privileges"); callForGlobalSetting("my.key", "my value"); } -- 2.39.5