From 3152ceb05b2f773be2704d81be9a6c5b41b5ebed Mon Sep 17 00:00:00 2001 From: Simon Brandhof Date: Tue, 29 Nov 2016 10:05:44 +0100 Subject: [PATCH] Fix Quality flaws related to org.sonar.server.rule --- .../org/sonar/server/rule/RuleService.java | 5 +- .../sonar/server/rule/index/RuleIndex.java | 26 +++---- .../server/rule/RuleServiceMediumTest.java | 75 ++++++++++++------- 3 files changed, 65 insertions(+), 41 deletions(-) diff --git a/server/sonar-server/src/main/java/org/sonar/server/rule/RuleService.java b/server/sonar-server/src/main/java/org/sonar/server/rule/RuleService.java index 581045bf5a17..48e703743188 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/rule/RuleService.java +++ b/server/sonar-server/src/main/java/org/sonar/server/rule/RuleService.java @@ -77,7 +77,8 @@ public void delete(RuleKey ruleKey) { } private void checkPermission() { - userSession.checkLoggedIn(); - userSession.checkPermission(GlobalPermissions.QUALITY_PROFILE_ADMIN); + userSession + .checkLoggedIn() + .checkPermission(GlobalPermissions.QUALITY_PROFILE_ADMIN); } } diff --git a/server/sonar-server/src/main/java/org/sonar/server/rule/index/RuleIndex.java b/server/sonar-server/src/main/java/org/sonar/server/rule/index/RuleIndex.java index d7fe053024f3..626f0ef34993 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/rule/index/RuleIndex.java +++ b/server/sonar-server/src/main/java/org/sonar/server/rule/index/RuleIndex.java @@ -64,6 +64,7 @@ import org.sonar.server.es.StickyFacetBuilder; import static org.elasticsearch.index.query.QueryBuilders.boolQuery; +import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery; import static org.elasticsearch.index.query.QueryBuilders.matchQuery; import static org.elasticsearch.index.query.QueryBuilders.simpleQueryStringQuery; import static org.sonar.server.es.EsUtils.SCROLL_TIME_IN_MINUTES; @@ -168,7 +169,7 @@ private static QueryBuilder buildQuery(RuleQuery query) { // No contextual query case String queryText = query.getQueryText(); if (StringUtils.isEmpty(queryText)) { - return QueryBuilders.matchAllQuery(); + return matchAllQuery(); } // Build RuleBased contextual query @@ -295,7 +296,7 @@ private static Map buildFilters(RuleQuery query) { if (childrenFilter.hasClauses()) { childQuery = childrenFilter; } else { - childQuery = QueryBuilders.matchAllQuery(); + childQuery = matchAllQuery(); } /** Implementation of activation query */ @@ -469,31 +470,28 @@ public Set terms(String fields) { } public Set terms(String fields, @Nullable String query, int size) { - Set tags = new HashSet<>(); - String key = "_ref"; + String aggregationKey = "_ref"; - TermsBuilder terms = AggregationBuilders.terms(key) + TermsBuilder termsAggregation = AggregationBuilders.terms(aggregationKey) .field(fields) .size(size) .minDocCount(1); if (query != null) { - terms.include(".*" + query + ".*"); + termsAggregation.include(".*" + query + ".*"); } SearchRequestBuilder request = getClient() .prepareSearch(INDEX) - .setQuery(QueryBuilders.matchAllQuery()) - .addAggregation(terms); + .setQuery(matchAllQuery()) + .addAggregation(termsAggregation); SearchResponse esResponse = request.get(); - Terms aggregation = esResponse.getAggregations().get(key); - + Set terms = new HashSet<>(); + Terms aggregation = esResponse.getAggregations().get(aggregationKey); if (aggregation != null) { - for (Terms.Bucket value : aggregation.getBuckets()) { - tags.add(value.getKeyAsString()); - } + aggregation.getBuckets().forEach(value -> terms.add(value.getKeyAsString())); } - return tags; + return terms; } private enum ToRuleKey implements Function { diff --git a/server/sonar-server/src/test/java/org/sonar/server/rule/RuleServiceMediumTest.java b/server/sonar-server/src/test/java/org/sonar/server/rule/RuleServiceMediumTest.java index 79f86211988d..d56d5aff5b71 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/rule/RuleServiceMediumTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/rule/RuleServiceMediumTest.java @@ -19,26 +19,26 @@ */ package org.sonar.server.rule; -import com.google.common.collect.Sets; -import java.util.Collections; import java.util.Set; import org.junit.After; import org.junit.Before; import org.junit.ClassRule; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import org.sonar.api.rule.RuleKey; import org.sonar.core.permission.GlobalPermissions; import org.sonar.db.DbClient; import org.sonar.db.DbSession; import org.sonar.db.rule.RuleDao; import org.sonar.db.rule.RuleTesting; +import org.sonar.server.exceptions.ForbiddenException; import org.sonar.server.exceptions.UnauthorizedException; -import org.sonar.server.rule.index.RuleIndex; -import org.sonar.server.rule.index.RuleIndexDefinition; import org.sonar.server.rule.index.RuleIndexer; import org.sonar.server.tester.ServerTester; import org.sonar.server.tester.UserSessionRule; +import static com.google.common.collect.Sets.newHashSet; import static org.assertj.core.api.Assertions.assertThat; public class RuleServiceMediumTest { @@ -46,14 +46,16 @@ public class RuleServiceMediumTest { @ClassRule public static ServerTester tester = new ServerTester().withEsIndexes(); - @org.junit.Rule + @Rule public UserSessionRule userSessionRule = UserSessionRule.forServerTester(tester); - RuleDao dao = tester.get(RuleDao.class); - RuleIndex index = tester.get(RuleIndex.class); - RuleService service = tester.get(RuleService.class); - DbSession dbSession; - RuleIndexer ruleIndexer; + @Rule + public ExpectedException expectedException = ExpectedException.none(); + + private RuleDao dao = tester.get(RuleDao.class); + private RuleService service = tester.get(RuleService.class); + private DbSession dbSession; + private RuleIndexer ruleIndexer; @Before public void before() { @@ -68,30 +70,53 @@ public void after() { } @Test - public void list_tags() { + public void listTags_returns_all_tags() { // insert db - RuleKey key1 = RuleKey.of("javascript", "S001"); - RuleKey key2 = RuleKey.of("java", "S001"); - dao.insert(dbSession, - RuleTesting.newDto(key1).setTags(Sets.newHashSet("tag1")).setSystemTags(Sets.newHashSet("sys1", "sys2"))); - dao.insert(dbSession, - RuleTesting.newDto(key2).setTags(Sets.newHashSet("tag2")).setSystemTags(Collections.emptySet())); - dbSession.commit(); - ruleIndexer.index(); + insertRule(RuleKey.of("javascript", "S001"), newHashSet("tag1"), newHashSet("sys1", "sys2")); + insertRule(RuleKey.of("java", "S001"), newHashSet("tag2"), newHashSet()); // all tags, including system Set tags = service.listTags(); assertThat(tags).containsOnly("tag1", "tag2", "sys1", "sys2"); + } - // verify in es - tags = index.terms(RuleIndexDefinition.FIELD_RULE_ALL_TAGS); - assertThat(tags).containsOnly("tag1", "tag2", "sys1", "sys2"); + @Test + public void listTags_returns_tags_filtered_by_name() { + insertRule(RuleKey.of("javascript", "S001"), newHashSet("tag1"), newHashSet("sys1", "sys2")); + insertRule(RuleKey.of("java", "S001"), newHashSet("tag2"), newHashSet()); + + assertThat(service.listTags("missing", 10)).isEmpty(); + assertThat(service.listTags("tag", 10)).containsOnly("tag1", "tag2"); + assertThat(service.listTags("sys", 10)).containsOnly("sys1", "sys2"); + + // LIMITATION: case sensitive + assertThat(service.listTags("TAG", 10)).isEmpty(); + assertThat(service.listTags("TAg", 10)).isEmpty(); + assertThat(service.listTags("MISSing", 10)).isEmpty(); + } + + @Test + public void delete_throws_UnauthorizedException_if_not_logged_in() { + expectedException.expect(UnauthorizedException.class); + expectedException.expectMessage("Authentication is required"); + + service.delete(RuleKey.of("java", "S001")); } - @Test(expected = UnauthorizedException.class) - public void do_not_delete_if_not_granted() { - userSessionRule.setGlobalPermissions(GlobalPermissions.SCAN_EXECUTION); + @Test + public void delete_throws_ForbiddenException_if_not_administrator() { + userSessionRule.login().setGlobalPermissions(GlobalPermissions.SCAN_EXECUTION); + + expectedException.expect(ForbiddenException.class); + expectedException.expectMessage("Insufficient privileges"); service.delete(RuleKey.of("java", "S001")); } + + private void insertRule(RuleKey key, Set tags, Set systemTags) { + dao.insert(dbSession, + RuleTesting.newDto(key).setTags(tags).setSystemTags(systemTags)); + dbSession.commit(); + ruleIndexer.index(); + } }