From aa5f425a236f8be72f709cc91175d3f65a26753d Mon Sep 17 00:00:00 2001 From: Teryk Bellahsene Date: Wed, 24 Aug 2016 19:34:36 +0200 Subject: [PATCH] SONAR-7970 WS settings/set validates data --- .../sonar/server/settings/ws/SetAction.java | 39 ++++++- .../server/settings/ws/SetActionTest.java | 103 +++++++++++++++++- .../org/sonar/db/property/PropertyDto.java | 5 + .../sonar/db/property/PropertyDtoTest.java | 16 +++ .../sonar/api/config/PropertyDefinitions.java | 2 + 5 files changed, 161 insertions(+), 4 deletions(-) diff --git a/server/sonar-server/src/main/java/org/sonar/server/settings/ws/SetAction.java b/server/sonar-server/src/main/java/org/sonar/server/settings/ws/SetAction.java index a5b7e060a47..c7fb7ac44e7 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/settings/ws/SetAction.java +++ b/server/sonar-server/src/main/java/org/sonar/server/settings/ws/SetAction.java @@ -20,7 +20,11 @@ package org.sonar.server.settings.ws; +import java.util.Locale; import java.util.Optional; +import org.sonar.api.config.PropertyDefinition; +import org.sonar.api.config.PropertyDefinitions; +import org.sonar.api.i18n.I18n; import org.sonar.api.server.ws.Request; import org.sonar.api.server.ws.Response; import org.sonar.api.server.ws.WebService; @@ -42,12 +46,18 @@ import static org.sonarqube.ws.client.setting.SettingsWsParameters.PARAM_COMPONE import static org.sonarqube.ws.client.setting.SettingsWsParameters.PARAM_KEY; import static org.sonarqube.ws.client.setting.SettingsWsParameters.PARAM_VALUE; +import static org.sonar.server.ws.WsUtils.checkRequest; + public class SetAction implements SettingsWsAction { + private final PropertyDefinitions propertyDefinitions; + private final I18n i18n; private final DbClient dbClient; private final ComponentFinder componentFinder; private final UserSession userSession; - public SetAction(DbClient dbClient, ComponentFinder componentFinder, UserSession userSession) { + public SetAction(PropertyDefinitions propertyDefinitions, I18n i18n, DbClient dbClient, ComponentFinder componentFinder, UserSession userSession) { + this.propertyDefinitions = propertyDefinitions; + this.i18n = i18n; this.dbClient = dbClient; this.componentFinder = componentFinder; this.userSession = userSession; @@ -94,6 +104,7 @@ public class SetAction implements SettingsWsAction { Optional component = searchComponent(dbSession, setRequest); checkPermissions(component); + validate(setRequest, component); dbClient.propertiesDao().insertProperty(dbSession, toProperty(setRequest, component)); dbSession.commit(); } finally { @@ -103,6 +114,25 @@ public class SetAction implements SettingsWsAction { response.noContent(); } + private void validate(SetRequest request, Optional component) { + PropertyDefinition definition = propertyDefinitions.get(request.getKey()); + if (definition == null) { + return; + } + + PropertyDefinition.Result result = definition.validate(request.getValue()); + + checkRequest(result.isValid(), + i18n.message(Locale.ENGLISH, "property.error." + result.getErrorKey(), "Error when validating setting with key '%s' and value '%s'"), + request.getKey(), request.getValue()); + + checkRequest(component.isPresent() || definition.global(), "Setting '%s' cannot be global", request.getKey()); + String qualifier = component.isPresent() ? component.get().qualifier() : ""; + checkRequest(!component.isPresent() + || definition.qualifiers().contains(component.get().qualifier()), + "Setting '%s' cannot be set on a %s", request.getKey(), i18n.message(Locale.ENGLISH, "qualifier." + qualifier, null)); + } + private void checkPermissions(Optional component) { if (component.isPresent()) { userSession.checkComponentUuidPermission(UserRole.ADMIN, component.get().uuid()); @@ -130,9 +160,12 @@ public class SetAction implements SettingsWsAction { return Optional.of(project); } - private static PropertyDto toProperty(SetRequest request, Optional component) { + private PropertyDto toProperty(SetRequest request, Optional component) { + PropertyDefinition definition = propertyDefinitions.get(request.getKey()); + // handles deprecated key but persist the new key + String key = definition == null ? request.getKey() : definition.key(); PropertyDto property = new PropertyDto() - .setKey(request.getKey()) + .setKey(key) .setValue(request.getValue()); if (component.isPresent()) { diff --git a/server/sonar-server/src/test/java/org/sonar/server/settings/ws/SetActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/settings/ws/SetActionTest.java index aa8cb63e548..6c1e01e85cd 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/settings/ws/SetActionTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/settings/ws/SetActionTest.java @@ -26,6 +26,10 @@ import javax.annotation.Nullable; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; +import org.sonar.api.PropertyType; +import org.sonar.api.config.PropertyDefinition; +import org.sonar.api.config.PropertyDefinitions; +import org.sonar.api.resources.Qualifiers; import org.sonar.api.server.ws.WebService; import org.sonar.api.server.ws.WebService.Param; import org.sonar.api.utils.System2; @@ -40,7 +44,9 @@ import org.sonar.db.property.PropertyDbTester; import org.sonar.db.property.PropertyDto; import org.sonar.db.property.PropertyQuery; import org.sonar.server.component.ComponentFinder; +import org.sonar.server.exceptions.BadRequestException; import org.sonar.server.exceptions.ForbiddenException; +import org.sonar.server.i18n.I18nRule; import org.sonar.server.tester.UserSessionRule; import org.sonar.server.ws.TestRequest; import org.sonar.server.ws.TestResponse; @@ -48,6 +54,7 @@ import org.sonar.server.ws.WsActionTester; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.groups.Tuple.tuple; +import static org.sonar.db.component.ComponentTesting.newView; import static org.sonar.db.property.PropertyTesting.newComponentPropertyDto; import static org.sonar.db.property.PropertyTesting.newGlobalPropertyDto; @@ -66,7 +73,10 @@ public class SetActionTest { DbSession dbSession = db.getSession(); ComponentFinder componentFinder = new ComponentFinder(dbClient); - SetAction underTest = new SetAction(dbClient, componentFinder, userSession); + I18nRule i18n = new I18nRule(); + PropertyDefinitions propertyDefinitions = new PropertyDefinitions(); + + SetAction underTest = new SetAction(propertyDefinitions, i18n, dbClient, componentFinder, userSession); WsActionTester ws = new WsActionTester(underTest); @@ -142,6 +152,24 @@ public class SetActionTest { assertUserProperty("my.key", "my user value", 42L); } + @Test + public void persist_global_property_with_deprecated_key() { + propertyDefinitions.addComponent(PropertyDefinition + .builder("my.key") + .deprecatedKey("my.old.key") + .name("foo") + .description("desc") + .category("cat") + .subCategory("subCat") + .type(PropertyType.STRING) + .defaultValue("default") + .build()); + + callForGlobalProperty("my.old.key", "My Value"); + + assertGlobalProperty("my.key", "My Value"); + } + @Test public void fail_when_no_key() { expectedException.expect(IllegalArgumentException.class); @@ -180,6 +208,79 @@ public class SetActionTest { callForGlobalProperty("my.key", "my value"); } + @Test + public void fail_when_data_and_type_do_not_match() { + propertyDefinitions.addComponent(PropertyDefinition + .builder("my.key") + .name("foo") + .description("desc") + .category("cat") + .subCategory("subCat") + .type(PropertyType.INTEGER) + .defaultValue("default") + .build()); + i18n.put("property.error.notInteger", "Not an integer error message"); + expectedException.expect(BadRequestException.class); + expectedException.expectMessage("Not an integer error message"); + + callForGlobalProperty("my.key", "My Value"); + } + + @Test + public void fail_when_data_and_type_do_not_match_with_unknown_error_key() { + propertyDefinitions.addComponent(PropertyDefinition + .builder("my.key") + .name("foo") + .description("desc") + .category("cat") + .subCategory("subCat") + .type(PropertyType.INTEGER) + .defaultValue("default") + .build()); + expectedException.expect(BadRequestException.class); + expectedException.expectMessage("Error when validating setting with key 'my.key' and value 'My Value'"); + + callForGlobalProperty("my.key", "My Value"); + } + + @Test + public void fail_when_global_with_property_only_on_projects() { + propertyDefinitions.addComponent(PropertyDefinition + .builder("my.key") + .name("foo") + .description("desc") + .category("cat") + .subCategory("subCat") + .type(PropertyType.INTEGER) + .defaultValue("default") + .onlyOnQualifiers(Qualifiers.PROJECT) + .build()); + expectedException.expect(BadRequestException.class); + expectedException.expectMessage("Setting 'my.key' cannot be global"); + + callForGlobalProperty("my.key", "42"); + } + + @Test + public void fail_when_view_property_when_on_projects_only() { + propertyDefinitions.addComponent(PropertyDefinition + .builder("my.key") + .name("foo") + .description("desc") + .category("cat") + .subCategory("subCat") + .type(PropertyType.STRING) + .defaultValue("default") + .onQualifiers(Qualifiers.PROJECT) + .build()); + ComponentDto view = componentDb.insertComponent(newView("view-uuid")); + i18n.put("qualifier." + Qualifiers.VIEW, "View"); + expectedException.expect(BadRequestException.class); + expectedException.expectMessage("Setting 'my.key' cannot be set on a View"); + + callForProjectPropertyByUuid("my.key", "My Value", view.uuid()); + } + @Test public void definition() { WebService.Action definition = ws.getDef(); diff --git a/sonar-db/src/main/java/org/sonar/db/property/PropertyDto.java b/sonar-db/src/main/java/org/sonar/db/property/PropertyDto.java index a9f9a5f118d..cffe1e84c02 100644 --- a/sonar-db/src/main/java/org/sonar/db/property/PropertyDto.java +++ b/sonar-db/src/main/java/org/sonar/db/property/PropertyDto.java @@ -22,7 +22,11 @@ package org.sonar.db.property; import com.google.common.base.MoreObjects; import java.util.Objects; +import static com.google.common.base.Preconditions.checkArgument; + public class PropertyDto { + private static final int MAX_KEY_LENGTH = 512; + private Long id; private String key; private String value; @@ -43,6 +47,7 @@ public class PropertyDto { } public PropertyDto setKey(String key) { + checkArgument(key.length() <= MAX_KEY_LENGTH, "Setting key length (%s) is longer than the maximum authorized (%s). '%s' was provided", key.length(), MAX_KEY_LENGTH, key); this.key = key; return this; } diff --git a/sonar-db/src/test/java/org/sonar/db/property/PropertyDtoTest.java b/sonar-db/src/test/java/org/sonar/db/property/PropertyDtoTest.java index a86c7f2e822..a1ff04f7a33 100644 --- a/sonar-db/src/test/java/org/sonar/db/property/PropertyDtoTest.java +++ b/sonar-db/src/test/java/org/sonar/db/property/PropertyDtoTest.java @@ -19,11 +19,18 @@ */ package org.sonar.db.property; +import com.google.common.base.Strings; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import static org.assertj.core.api.Assertions.assertThat; public class PropertyDtoTest { + @Rule + public ExpectedException expectedException = ExpectedException.none(); + + PropertyDto underTest = new PropertyDto(); @Test public void testEquals() { @@ -44,4 +51,13 @@ public class PropertyDtoTest { public void testToString() { assertThat(new PropertyDto().setKey("foo:bar").setValue("value").setResourceId(123L).setUserId(456L).toString()).isEqualTo("PropertyDto{foo:bar, value, 123, 456}"); } + + @Test + public void fail_if_key_longer_than_512_characters() { + String veryLongKey = Strings.repeat("a", 513); + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("Setting key length (513) is longer than the maximum authorized (512). '" + veryLongKey + "' was provided"); + + underTest.setKey(veryLongKey); + } } diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/config/PropertyDefinitions.java b/sonar-plugin-api/src/main/java/org/sonar/api/config/PropertyDefinitions.java index 4aa351ab910..2f62eb28f2c 100644 --- a/sonar-plugin-api/src/main/java/org/sonar/api/config/PropertyDefinitions.java +++ b/sonar-plugin-api/src/main/java/org/sonar/api/config/PropertyDefinitions.java @@ -25,6 +25,7 @@ import java.util.Arrays; import java.util.Collection; import java.util.HashMap; import java.util.Map; +import javax.annotation.CheckForNull; import javax.annotation.Nullable; import org.apache.commons.lang.StringUtils; import org.sonar.api.CoreProperties; @@ -119,6 +120,7 @@ public final class PropertyDefinitions { return this; } + @CheckForNull public PropertyDefinition get(String key) { return definitions.get(validKey(key)); } -- 2.39.5