SONAR-4535 Add integer validation on active rule params

diff --git a/sonar-plugin-api/src/test/java/org/sonar/api/rules/AnnotationRuleParserTest.java b/sonar-plugin-api/src/test/java/org/sonar/api/rules/AnnotationRuleParserTest.java
index d056c3f3fac25e0c9476b3de477d89cdda5cd6e0..f844582db709651e48ac3fa9b8d86570a5b7f44f 100644 (file)
--- a/sonar-plugin-api/src/test/java/org/sonar/api/rules/AnnotationRuleParserTest.java
+++ b/sonar-plugin-api/src/test/java/org/sonar/api/rules/AnnotationRuleParserTest.java
@@ -75,7 +75,7 @@ public class AnnotationRuleParserTest {
   }
 
   @Test
-  public void should_reject_invalid_prroperty_types() {
+  public void should_reject_invalid_property_types() {
     exception.expect(SonarException.class);
     exception.expectMessage("Invalid property type [INVALID]");
 

diff --git a/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileOperations.java b/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileOperations.java
index 9db3e150f1e598592c86f40222089f14c6735a55..580f97e153e9bef4e827e3360610196a4fd22dbe 100644 (file)
--- a/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileOperations.java
+++ b/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileOperations.java
@@ -26,7 +26,9 @@ import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Multimap;
 import org.apache.commons.lang.StringUtils;
+import org.apache.commons.lang.math.NumberUtils;
 import org.apache.ibatis.session.SqlSession;
+import org.sonar.api.PropertyType;
 import org.sonar.api.ServerComponent;
 import org.sonar.api.profiles.ProfileImporter;
 import org.sonar.api.profiles.RulesProfile;
@@ -51,8 +53,6 @@ import org.sonar.server.rule.ProfileRules;
 import org.sonar.server.rule.RuleRegistry;
 import org.sonar.server.user.UserSession;
 
-import javax.annotation.CheckForNull;
-
 import java.io.StringReader;
 import java.util.Date;
 import java.util.List;
@@ -145,6 +145,7 @@ public class QProfileOperations implements ServerComponent {
 
   public ActiveRuleDto createActiveRule(QualityProfileDto qualityProfile, RuleDto rule, String severity, UserSession userSession) {
     checkPermission(userSession);
+    checkSeverity(severity);
     SqlSession session = myBatis.openSession();
     try {
       ActiveRuleDto activeRule = new ActiveRuleDto()
@@ -177,6 +178,7 @@ public class QProfileOperations implements ServerComponent {
 
   public void updateSeverity(QualityProfileDto qualityProfile, ActiveRuleDto activeRule, String newSeverity, UserSession userSession) {
     checkPermission(userSession);
+    checkSeverity(newSeverity);
     SqlSession session = myBatis.openSession();
     try {
       Integer oldSeverity = activeRule.getSeverity();
@@ -216,10 +218,8 @@ public class QProfileOperations implements ServerComponent {
 
     SqlSession session = myBatis.openSession();
     try {
-      RuleParamDto ruleParam = ruleDao.selectParamByRuleAndKey(activeRule.getRulId(), key, session);
-      if (ruleParam == null) {
-        throw new IllegalArgumentException("No rule param found");
-      }
+      RuleParamDto ruleParam = findRuleParamNotNull(activeRule.getRulId(), key, session);
+      validateParam(ruleParam.getType(), value);
       ActiveRuleParamDto activeRuleParam = new ActiveRuleParamDto().setActiveRuleId(activeRule.getId()).setKey(key).setValue(value).setRulesParameterId(ruleParam.getId());
       activeRuleDao.insert(activeRuleParam, session);
       session.commit();
@@ -252,6 +252,9 @@ public class QProfileOperations implements ServerComponent {
 
     SqlSession session = myBatis.openSession();
     try {
+      RuleParamDto ruleParam = findRuleParamNotNull(activeRule.getRulId(), activeRuleParam.getKey(), session);
+      validateParam(ruleParam.getType(), value);
+
       String sanitizedValue = Strings.emptyToNull(value);
       String oldValue = activeRuleParam.getValue();
       activeRuleParam.setValue(sanitizedValue);
@@ -434,11 +437,6 @@ public class QProfileOperations implements ServerComponent {
       .setValue(activeRuleParam.getValue());
   }
 
-  @CheckForNull
-  private ActiveRuleDto findActiveRule(QualityProfileDto qualityProfile, RuleDto rule) {
-    return activeRuleDao.selectByProfileAndRule(qualityProfile.getId(), rule.getId());
-  }
-
   private void checkPermission(UserSession userSession) {
     userSession.checkLoggedIn();
     userSession.checkGlobalPermission(GlobalPermissions.QUALITY_PROFILE_ADMIN);
@@ -452,4 +450,24 @@ public class QProfileOperations implements ServerComponent {
     return name;
   }
 
+  private void checkSeverity(String severity) {
+    if (!Severity.ALL.contains(severity)) {
+      throw new BadRequestException("The severity is not valid");
+    }
+  }
+
+  private RuleParamDto findRuleParamNotNull(Integer ruleId, String key, SqlSession session) {
+    RuleParamDto ruleParam = ruleDao.selectParamByRuleAndKey(ruleId, key, session);
+    if (ruleParam == null) {
+      throw new IllegalArgumentException("No rule param found");
+    }
+    return ruleParam;
+  }
+
+  private void validateParam(String type, String value) {
+    if (type.equals(PropertyType.INTEGER.name()) && !NumberUtils.isDigits(value)) {
+      throw new BadRequestException(String.format("Value '%s' must be an integer.", value));
+    }
+  }
+
 }

diff --git a/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfiles.java b/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfiles.java
index 2c8d1147b14e9888a6085f284c78ecd8e8b4f0fe..b7e93b3ea50e933895c267ba00b23441977995a9 100644 (file)
--- a/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfiles.java
+++ b/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfiles.java
@@ -90,6 +90,7 @@ public class QProfiles implements ServerComponent {
   // delete note on an active rule (only E/S indexing)
   // extends extension of a rule (only E/S indexing)
   // revert modification on active rule with inheritance
+  // active rule parameter validation (only Integer types are checked)
   //
   // TEMPLATE RULES
   // create template rule

diff --git a/sonar-server/src/main/webapp/WEB-INF/app/controllers/new_rules_configuration_controller.rb b/sonar-server/src/main/webapp/WEB-INF/app/controllers/new_rules_configuration_controller.rb
index 6b307c1b8ba2f0fa9453e870363c693b1d9f4643..3031eb2f1a5b14392ac450eb820f87d53f038850 100644 (file)
--- a/sonar-server/src/main/webapp/WEB-INF/app/controllers/new_rules_configuration_controller.rb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/controllers/new_rules_configuration_controller.rb
@@ -314,6 +314,7 @@ class NewRulesConfigurationController < ApplicationController
     call_backend do
       result = Internal.quality_profiles.updateActiveRuleParam(params[:profile_id].to_i, params[:active_rule_id].to_i, params[:param_id], params[:value])
     end
+
     profile = result.profile
     rule = result.rule
     render :partial => 'rule', :locals => {:profile => profile, :rule => rule}

diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/new_rules_configuration/_rule_param.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/new_rules_configuration/_rule_param.html.erb
index 6731a8d81069b2edc138b197d2746272e57b8ab6..4b09f1b9124dbb9ddaa2885873eb1f56784432f8 100644 (file)
--- a/sonar-server/src/main/webapp/WEB-INF/app/views/new_rules_configuration/_rule_param.html.erb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/views/new_rules_configuration/_rule_param.html.erb
@@ -1,5 +1,4 @@
 <% # locals = rule, profile, parameter, ancestor_active_rule
-   # Display default value only for inactive rules
    param_id = "#{rule.id}#{parameter.key}"
    # Display default value only for inactive rules
    param_value = parameter.default_value if !rule.activeRuleId
@@ -21,20 +20,13 @@
            name="form-<%=u parameter.key-%>" method="post"
            action="<%= ApplicationController.root_context -%>/new_rules_configuration/update_param/?active_rule_id=<%=active_rule_id-%>&param_id=<%=parameter.key-%>&profile_id=<%=profile.id-%>">
 
-  <div id="error_<%= active_rule_id -%><%= parameter.key -%>" class="error" style="display: none"></div>
+  <div id="error_<%= param_id -%>" class="error" style="display: none"></div>
 
-  <span id="text_<%= active_rule_id -%><%= parameter.key -%>"><%= param_value_input(rule, parameter, param_value, :disabled => read_only) -%></span>
+  <span id="text_<%= param_id -%>"><%= param_value_input(rule, parameter, param_value, :disabled => read_only) -%></span>
 
   <% unless read_only %>
     <%= submit_tag(message('update_verb'), :id => 'submit_' + param_id.to_s) %>
     <img src="<%= ApplicationController.root_context -%>/images/loading.gif" style="display:none;" id="param_loading_<%= param_id -%>" class="rule-param-loading">
-<%
-=begin %>
-    <% if active_parameter and active_parameter.errors.size>0 %>
-      <span class="error"><%= active_parameter.errors.on 'value' %></span>
-    <% end %>
-<%
-=end %>
   <% end %>
 
   <% if !rule.nil? && rule.overrides? && ancestor_active_rule

diff --git a/sonar-server/src/test/java/org/sonar/server/qualityprofile/QProfileOperationsTest.java b/sonar-server/src/test/java/org/sonar/server/qualityprofile/QProfileOperationsTest.java
index 1502e276afcdccec3aa4872147ffb96f8b12e108..8ee0f0bce0655476e3b38bdda0d3bc1936bcc167 100644 (file)
--- a/sonar-server/src/test/java/org/sonar/server/qualityprofile/QProfileOperationsTest.java
+++ b/sonar-server/src/test/java/org/sonar/server/qualityprofile/QProfileOperationsTest.java
@@ -32,6 +32,7 @@ import org.mockito.Mock;
 import org.mockito.invocation.InvocationOnMock;
 import org.mockito.runners.MockitoJUnitRunner;
 import org.mockito.stubbing.Answer;
+import org.sonar.api.PropertyType;
 import org.sonar.api.profiles.ProfileImporter;
 import org.sonar.api.profiles.RulesProfile;
 import org.sonar.api.rule.Severity;
@@ -265,8 +266,8 @@ public class QProfileOperationsTest {
       .addToIndex(idActiveRuleToUpdate)
       .addToDelete(idActiveRuleToDelete);
     when(profilesManager.activated(eq(1), anyInt(), eq("Nicolas"))).thenReturn(inheritanceActions);
-    when(activeRuleDao.selectByIds(anyList(), isA(SqlSession.class))).thenReturn(ImmutableList.<ActiveRuleDto> of(mock(ActiveRuleDto.class)));
-    when(activeRuleDao.selectParamsByActiveRuleIds(anyList(), isA(SqlSession.class))).thenReturn(ImmutableList.<ActiveRuleParamDto> of(mock(ActiveRuleParamDto.class)));
+    when(activeRuleDao.selectByIds(anyList(), isA(SqlSession.class))).thenReturn(ImmutableList.<ActiveRuleDto>of(mock(ActiveRuleDto.class)));
+    when(activeRuleDao.selectParamsByActiveRuleIds(anyList(), isA(SqlSession.class))).thenReturn(ImmutableList.<ActiveRuleParamDto>of(mock(ActiveRuleParamDto.class)));
 
     ActiveRuleDto result = operations.createActiveRule(qualityProfile, rule, Severity.CRITICAL, authorizedUserSession);
     assertThat(result).isNotNull();
@@ -303,6 +304,23 @@ public class QProfileOperationsTest {
     verify(ruleRegistry).bulkIndexActiveRules(anyList(), isA(Multimap.class));
   }
 
+  @Test
+  public void fail_to_update_severity_on_invalid_severity() throws Exception {
+    QualityProfileDto qualityProfile = new QualityProfileDto().setId(1).setName("My profile").setLanguage("java");
+    Rule rule = Rule.create().setRepositoryKey("squid").setKey("AvoidCycle");
+    rule.setId(10);
+    ActiveRuleDto activeRule = new ActiveRuleDto().setId(5).setProfileId(1).setRuleId(10).setSeverity(1);
+
+    try {
+      operations.updateSeverity(qualityProfile, activeRule, "Unknown", authorizedUserSession);
+      fail();
+    } catch (Exception e) {
+      assertThat(e).isInstanceOf(BadRequestException.class);
+    }
+    verify(activeRuleDao, never()).update(eq(activeRule), eq(session));
+    verifyZeroInteractions(profilesManager);
+  }
+
   @Test
   public void deactivate_rule() throws Exception {
     ActiveRuleDto activeRule = new ActiveRuleDto().setId(5).setProfileId(1).setRuleId(10).setSeverity(1);
@@ -323,6 +341,10 @@ public class QProfileOperationsTest {
   public void update_active_rule_param() throws Exception {
     ActiveRuleDto activeRule = new ActiveRuleDto().setId(5).setProfileId(1).setRuleId(10).setSeverity(1);
     ActiveRuleParamDto activeRuleParam = new ActiveRuleParamDto().setId(100).setActiveRuleId(5).setKey("max").setValue("20");
+
+    RuleParamDto ruleParam = new RuleParamDto().setRuleId(10).setName("max").setDefaultValue("20").setType(PropertyType.INTEGER.name());
+    when(ruleDao.selectParamByRuleAndKey(10, "max", session)).thenReturn(ruleParam);
+
     when(profilesManager.ruleParamChanged(eq(1), eq(5), eq("max"), eq("20"), eq("30"), eq("Nicolas"))).thenReturn(new RuleInheritanceActions());
 
     operations.updateActiveRuleParam(activeRule, activeRuleParam, "30", authorizedUserSession);
@@ -354,7 +376,7 @@ public class QProfileOperationsTest {
   @Test
   public void create_active_rule_param() throws Exception {
     ActiveRuleDto activeRule = new ActiveRuleDto().setId(5).setProfileId(1).setRuleId(10).setSeverity(1);
-    RuleParamDto ruleParam = new RuleParamDto().setRuleId(10).setName("max").setDefaultValue("20");
+    RuleParamDto ruleParam = new RuleParamDto().setRuleId(10).setName("max").setDefaultValue("20").setType(PropertyType.INTEGER.name());
     when(ruleDao.selectParamByRuleAndKey(10, "max", session)).thenReturn(ruleParam);
     when(profilesManager.ruleParamChanged(eq(1), eq(5), eq("max"), eq((String) null), eq("30"), eq("Nicolas"))).thenReturn(new RuleInheritanceActions());
 
@@ -385,6 +407,22 @@ public class QProfileOperationsTest {
     verifyZeroInteractions(profilesManager);
   }
 
+  @Test
+  public void fail_to_create_active_rule_if_type_is_invalid() throws Exception {
+    RuleParamDto ruleParam = new RuleParamDto().setRuleId(10).setName("max").setDefaultValue("20").setType(PropertyType.INTEGER.name());
+    when(ruleDao.selectParamByRuleAndKey(10, "max", session)).thenReturn(ruleParam);
+
+    ActiveRuleDto activeRule = new ActiveRuleDto().setId(5).setProfileId(1).setRuleId(10).setSeverity(1);
+    try {
+      operations.createActiveRuleParam(activeRule, "max", "invalid integer", authorizedUserSession);
+      fail();
+    } catch (Exception e) {
+      assertThat(e).isInstanceOf(BadRequestException.class);
+    }
+    verify(activeRuleDao, never()).insert(any(ActiveRuleParamDto.class), eq(session));
+    verifyZeroInteractions(profilesManager);
+  }
+
   @Test
   public void update_active_rule_note_when_no_existing_note() throws Exception {
     ActiveRuleDto activeRule = new ActiveRuleDto().setId(5).setProfileId(1).setRuleId(10).setSeverity(1).setNoteCreatedAt(null).setNoteData(null);