From eacba88db0a44e34156e9e31d315ef31d61e726a Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Tue, 18 Feb 2020 13:54:34 +0100 Subject: [PATCH] SONAR-13089 Fix permission template issue when lot of groups * Remove PermissionQuery#template * SONAR-13089 Fix api/templates/template_groups WS * Add UTs for api/templates/template_users WS --- .../sonar/db/permission/PermissionQuery.java | 14 ------ .../template/PermissionTemplateMapper.xml | 12 +++-- .../GroupWithPermissionTemplateDaoTest.java | 40 ++++++++++++++++- .../template/PermissionTemplateDaoTest.java | 15 +++---- .../UserWithPermissionTemplateDaoTest.java | 23 ++++++++++ .../ws/template/TemplateUsersAction.java | 3 +- .../ws/template/TemplateGroupsActionTest.java | 45 +++++++++++++------ .../ws/template/TemplateUsersActionTest.java | 31 ++++++++++++- 8 files changed, 140 insertions(+), 43 deletions(-) diff --git a/server/sonar-db-dao/src/main/java/org/sonar/db/permission/PermissionQuery.java b/server/sonar-db-dao/src/main/java/org/sonar/db/permission/PermissionQuery.java index 5fcc09aaa9b..e71aa775780 100644 --- a/server/sonar-db-dao/src/main/java/org/sonar/db/permission/PermissionQuery.java +++ b/server/sonar-db-dao/src/main/java/org/sonar/db/permission/PermissionQuery.java @@ -50,7 +50,6 @@ public class PermissionQuery { // filter on project, else filter org permissions private final String componentUuid; private final Long componentId; - private final String template; // filter on login, email or name of users or groups private final String searchQuery; @@ -70,7 +69,6 @@ public class PermissionQuery { this.withAtLeastOnePermission = builder.withAtLeastOnePermission; this.componentUuid = builder.componentUuid; this.componentId = builder.componentId; - this.template = builder.template; this.searchQuery = builder.searchQuery; this.searchQueryToSql = builder.searchQuery == null ? null : buildLikeValue(builder.searchQuery, WildcardPosition.BEFORE_AND_AFTER); this.searchQueryToSqlLowercase = searchQueryToSql == null ? null : searchQueryToSql.toLowerCase(Locale.ENGLISH); @@ -91,12 +89,6 @@ public class PermissionQuery { return withAtLeastOnePermission; } - // TODO remove it, it should not be in the query, but set as a separate parameter - @Deprecated - public String template() { - return template; - } - @CheckForNull public String getComponentUuid() { return componentUuid; @@ -139,7 +131,6 @@ public class PermissionQuery { private String organizationUuid; private String componentUuid; private Long componentId; - private String template; private String searchQuery; private boolean withAtLeastOnePermission; @@ -156,11 +147,6 @@ public class PermissionQuery { return this; } - public Builder setTemplate(@Nullable String template) { - this.template = template; - return this; - } - public Builder setComponent(ComponentDto component) { return setComponent(component.uuid(), component.getId()); } diff --git a/server/sonar-db-dao/src/main/resources/org/sonar/db/permission/template/PermissionTemplateMapper.xml b/server/sonar-db-dao/src/main/resources/org/sonar/db/permission/template/PermissionTemplateMapper.xml index 57ceee66ec0..a5cd5db918a 100644 --- a/server/sonar-db-dao/src/main/resources/org/sonar/db/permission/template/PermissionTemplateMapper.xml +++ b/server/sonar-db-dao/src/main/resources/org/sonar/db/permission/template/PermissionTemplateMapper.xml @@ -139,8 +139,10 @@ FROM users u - LEFT JOIN perm_templates_users ptu ON ptu.user_id=u.id AND ptu.template_id=#{templateId} - INNER JOIN organization_members om ON u.id=om.user_id AND om.organization_uuid=#{query.organizationUuid} + LEFT JOIN perm_templates_users ptu ON ptu.user_id=u.id + AND ptu.template_id=#{templateId} + INNER JOIN organization_members om ON u.id=om.user_id + AND om.organization_uuid=#{query.organizationUuid} u.active = ${_true} @@ -179,6 +181,7 @@ FROM groups g LEFT JOIN perm_templates_groups ptg ON ptg.group_id=g.id + AND ptg.template_id=#{templateId} where g.organization_uuid=#{query.organizationUuid,jdbcType=VARCHAR} UNION ALL @@ -187,8 +190,11 @@ 'Anyone' AS name, ptg.permission_reference AS permission, ptg.template_id AS templateId - FROM perm_templates_groups ptg + FROM groups g + LEFT JOIN perm_templates_groups ptg ON + ptg.template_id=#{templateId} + g.organization_uuid=#{query.organizationUuid,jdbcType=VARCHAR} AND ptg.group_id IS NULL diff --git a/server/sonar-db-dao/src/test/java/org/sonar/db/permission/template/GroupWithPermissionTemplateDaoTest.java b/server/sonar-db-dao/src/test/java/org/sonar/db/permission/template/GroupWithPermissionTemplateDaoTest.java index 7bb1bbb9457..29165c65c62 100644 --- a/server/sonar-db-dao/src/test/java/org/sonar/db/permission/template/GroupWithPermissionTemplateDaoTest.java +++ b/server/sonar-db-dao/src/test/java/org/sonar/db/permission/template/GroupWithPermissionTemplateDaoTest.java @@ -38,6 +38,7 @@ import static org.assertj.core.api.Assertions.tuple; import static org.sonar.api.web.UserRole.ADMIN; import static org.sonar.api.web.UserRole.USER; import static org.sonar.core.permission.GlobalPermissions.PROVISIONING; +import static org.sonar.db.permission.PermissionQuery.DEFAULT_PAGE_SIZE; import static org.sonar.db.permission.PermissionQuery.builder; import static org.sonar.db.user.GroupTesting.newGroupDto; @@ -91,7 +92,6 @@ public class GroupWithPermissionTemplateDaoTest { public void selectGroupNamesByQueryAndTemplate_is_ordering_results_by_groups_with_permission_then_by_name() { OrganizationDto organization = db.organizations().insert(); PermissionTemplateDto template = permissionTemplateDbTester.insertTemplate(organization); - GroupDto group1 = db.users().insertGroup(organization, "A"); GroupDto group2 = db.users().insertGroup(organization, "B"); GroupDto group3 = db.users().insertGroup(organization, "C"); @@ -103,6 +103,42 @@ public class GroupWithPermissionTemplateDaoTest { .containsExactly("Anyone", group3.getName(), group1.getName(), group2.getName()); } + @Test + public void selectGroupNamesByQueryAndTemplate_is_order_by_groups_with_permission_then_by_name_when_many_groups() { + OrganizationDto organization = db.organizations().insert(); + PermissionTemplateDto template = permissionTemplateDbTester.insertTemplate(organization); + IntStream.rangeClosed(1, DEFAULT_PAGE_SIZE + 1).forEach(i -> { + db.users().insertGroup(organization, "Group-" + i); + }); + + String lastGroupName = "Group-" + (DEFAULT_PAGE_SIZE + 1); + permissionTemplateDbTester.addGroupToTemplate(template, db.users().selectGroup(organization, lastGroupName).get(), UserRole.USER); + + PermissionQuery query = PermissionQuery.builder().setOrganizationUuid(organization.getUuid()).build(); + assertThat(underTest.selectGroupNamesByQueryAndTemplate(db.getSession(), query, template.getId())) + .hasSize(DEFAULT_PAGE_SIZE) + .startsWith("Anyone", lastGroupName, "Group-1"); + } + + @Test + public void selectGroupNamesByQueryAndTemplate_ignores_other_template_and_is_ordered_by_groups_with_permission_then_by_name_when_many_groups() { + OrganizationDto organization = db.organizations().insert(); + PermissionTemplateDto template = permissionTemplateDbTester.insertTemplate(organization); + PermissionTemplateDto otherTemplate = permissionTemplateDbTester.insertTemplate(organization); + IntStream.rangeClosed(1, DEFAULT_PAGE_SIZE + 1).forEach(i -> { + GroupDto group = db.users().insertGroup(organization, "Group-" + i); + permissionTemplateDbTester.addGroupToTemplate(otherTemplate, group, UserRole.USER); + }); + + String lastGroupName = "Group-" + (DEFAULT_PAGE_SIZE + 1); + permissionTemplateDbTester.addGroupToTemplate(template, db.users().selectGroup(organization, lastGroupName).get(), UserRole.USER); + + PermissionQuery query = PermissionQuery.builder().setOrganizationUuid(organization.getUuid()).build(); + assertThat(underTest.selectGroupNamesByQueryAndTemplate(db.getSession(), query, template.getId())) + .hasSize(DEFAULT_PAGE_SIZE) + .startsWith("Anyone", lastGroupName, "Group-1"); + } + @Test public void select_group_names_by_query_and_template_is_paginated() { OrganizationDto organization = db.organizations().insert(); @@ -121,7 +157,7 @@ public class GroupWithPermissionTemplateDaoTest { OrganizationDto organization = db.organizations().insert(); PermissionTemplateDto template = permissionTemplateDbTester.insertTemplate(organization); - GroupDto group = db.users().insertGroup(newGroupDto().setName("Group")); + GroupDto group = db.users().insertGroup(organization, "Group"); PermissionTemplateDto otherTemplate = permissionTemplateDbTester.insertTemplate(organization); permissionTemplateDbTester.addGroupToTemplate(otherTemplate.getId(), group.getId(), USER); diff --git a/server/sonar-db-dao/src/test/java/org/sonar/db/permission/template/PermissionTemplateDaoTest.java b/server/sonar-db-dao/src/test/java/org/sonar/db/permission/template/PermissionTemplateDaoTest.java index b9bcc3381e8..6839386155c 100644 --- a/server/sonar-db-dao/src/test/java/org/sonar/db/permission/template/PermissionTemplateDaoTest.java +++ b/server/sonar-db-dao/src/test/java/org/sonar/db/permission/template/PermissionTemplateDaoTest.java @@ -54,7 +54,6 @@ public class PermissionTemplateDaoTest { @Rule public ExpectedException expectedException = ExpectedException.none(); - @Rule public DbTester db = DbTester.create(); @@ -70,7 +69,7 @@ public class PermissionTemplateDaoTest { } @Test - public void should_create_permission_template() { + public void create_permission_template() { PermissionTemplateDto permissionTemplate = underTest.insert(db.getSession(), newPermissionTemplateDto() .setUuid("ABCD") .setName("my template") @@ -88,7 +87,7 @@ public class PermissionTemplateDaoTest { } @Test - public void should_select_permission_template_by_uuid() { + public void select_permission_template_by_uuid() { templateDb.insertTemplate(newPermissionTemplateDto() .setUuid("ABCD") .setName("my template") @@ -169,7 +168,7 @@ public class PermissionTemplateDaoTest { } @Test - public void should_delete_permission_template() { + public void delete_permission_template() { UserDto user1 = db.users().insertUser(); UserDto user2 = db.users().insertUser(); GroupDto group1 = db.users().insertGroup(); @@ -203,7 +202,7 @@ public class PermissionTemplateDaoTest { } @Test - public void should_add_user_permission_to_template() { + public void add_user_permission_to_template() { PermissionTemplateDto permissionTemplate = templateDb.insertTemplate(db.getDefaultOrganization()); UserDto user = db.users().insertUser(); @@ -216,7 +215,7 @@ public class PermissionTemplateDaoTest { } @Test - public void should_remove_user_permission_from_template() { + public void remove_user_permission_from_template() { PermissionTemplateDto permissionTemplate = templateDb.insertTemplate(db.getDefaultOrganization()); UserDto user1 = db.users().insertUser(); UserDto user2 = db.users().insertUser(); @@ -232,7 +231,7 @@ public class PermissionTemplateDaoTest { } @Test - public void should_add_group_permission_to_template() { + public void add_group_permission_to_template() { PermissionTemplateDto permissionTemplate = templateDb.insertTemplate(db.getDefaultOrganization()); GroupDto group = db.users().insertGroup(); @@ -264,7 +263,7 @@ public class PermissionTemplateDaoTest { } @Test - public void should_add_group_permission_to_anyone() { + public void add_group_permission_to_anyone() { PermissionTemplateDto permissionTemplate = templateDb.insertTemplate(db.getDefaultOrganization()); underTest.insertGroupPermission(dbSession, permissionTemplate.getId(), null, "user"); diff --git a/server/sonar-db-dao/src/test/java/org/sonar/db/permission/template/UserWithPermissionTemplateDaoTest.java b/server/sonar-db-dao/src/test/java/org/sonar/db/permission/template/UserWithPermissionTemplateDaoTest.java index 2944a8f507f..22e6cf453ba 100644 --- a/server/sonar-db-dao/src/test/java/org/sonar/db/permission/template/UserWithPermissionTemplateDaoTest.java +++ b/server/sonar-db-dao/src/test/java/org/sonar/db/permission/template/UserWithPermissionTemplateDaoTest.java @@ -20,6 +20,7 @@ package org.sonar.db.permission.template; import java.util.Collections; +import java.util.stream.IntStream; import org.junit.Rule; import org.junit.Test; import org.sonar.api.utils.System2; @@ -37,6 +38,7 @@ import static org.assertj.core.api.Assertions.tuple; import static org.sonar.api.web.UserRole.ADMIN; import static org.sonar.api.web.UserRole.CODEVIEWER; import static org.sonar.api.web.UserRole.USER; +import static org.sonar.db.permission.PermissionQuery.DEFAULT_PAGE_SIZE; import static org.sonar.db.permission.PermissionQuery.builder; public class UserWithPermissionTemplateDaoTest { @@ -159,6 +161,26 @@ public class UserWithPermissionTemplateDaoTest { .containsExactly(user3.getLogin(), user1.getLogin(), user2.getLogin()); } + @Test + public void selectUserLoginsByQueryAndTemplate_is_order_by_groups_with_permission_when_many_users() { + OrganizationDto organization = db.organizations().insert(); + PermissionTemplateDto template = db.permissionTemplates().insertTemplate(organization); + // Add another template having some users with permission to make sure it's correctly ignored + PermissionTemplateDto otherTemplate = db.permissionTemplates().insertTemplate(organization); + IntStream.rangeClosed(1, DEFAULT_PAGE_SIZE + 1).forEach(i -> { + UserDto user = db.users().insertUser("User-" + i); + db.organizations().addMember(organization, user); + db.permissionTemplates().addUserToTemplate(otherTemplate, user, UserRole.USER); + }); + String lastLogin = "User-" + (DEFAULT_PAGE_SIZE + 1); + db.permissionTemplates().addUserToTemplate(template, db.users().selectUserByLogin(lastLogin).get(), UserRole.USER); + + PermissionQuery query = PermissionQuery.builder().setOrganizationUuid(organization.getUuid()).build(); + assertThat(underTest.selectUserLoginsByQueryAndTemplate(db.getSession(), query, template.getId())) + .hasSize(DEFAULT_PAGE_SIZE) + .startsWith(lastLogin); + } + @Test public void should_be_paginated() { OrganizationDto organization = db.organizations().insert(); @@ -234,4 +256,5 @@ public class UserWithPermissionTemplateDaoTest { assertThat(underTest.selectUserPermissionsByTemplateIdAndUserLogins(dbSession, permissionTemplate.getId(), Collections.emptyList())).isEmpty(); assertThat(underTest.selectUserPermissionsByTemplateIdAndUserLogins(dbSession, 123L, singletonList(user1.getLogin()))).isEmpty(); } + } diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/permission/ws/template/TemplateUsersAction.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/permission/ws/template/TemplateUsersAction.java index 19268eb2505..76eba3e2b2f 100644 --- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/permission/ws/template/TemplateUsersAction.java +++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/permission/ws/template/TemplateUsersAction.java @@ -36,9 +36,9 @@ import org.sonar.db.permission.template.PermissionTemplateDto; import org.sonar.db.permission.template.PermissionTemplateUserDto; import org.sonar.db.user.UserDto; import org.sonar.server.issue.AvatarResolver; +import org.sonar.server.permission.RequestValidator; import org.sonar.server.permission.ws.PermissionWsSupport; import org.sonar.server.permission.ws.PermissionsWsAction; -import org.sonar.server.permission.RequestValidator; import org.sonar.server.permission.ws.WsParameters; import org.sonar.server.user.UserSession; import org.sonarqube.ws.Permissions; @@ -121,7 +121,6 @@ public class TemplateUsersAction implements PermissionsWsAction { String permission = wsRequest.param(PARAM_PERMISSION); PermissionQuery.Builder query = PermissionQuery.builder() .setOrganizationUuid(template.getOrganizationUuid()) - .setTemplate(template.getUuid()) .setPermission(permission != null ? requestValidator.validateProjectPermission(permission) : null) .setPageIndex(wsRequest.mandatoryParamAsInt(PAGE)) .setPageSize(wsRequest.mandatoryParamAsInt(PAGE_SIZE)) diff --git a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/permission/ws/template/TemplateGroupsActionTest.java b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/permission/ws/template/TemplateGroupsActionTest.java index 6b31529875f..4cbf3b0cba2 100644 --- a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/permission/ws/template/TemplateGroupsActionTest.java +++ b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/permission/ws/template/TemplateGroupsActionTest.java @@ -19,11 +19,13 @@ */ package org.sonar.server.permission.ws.template; +import java.util.stream.IntStream; import javax.annotation.Nullable; import org.junit.Test; import org.sonar.api.resources.Qualifiers; import org.sonar.api.resources.ResourceTypes; import org.sonar.api.server.ws.WebService; +import org.sonar.api.web.UserRole; import org.sonar.core.permission.GlobalPermissions; import org.sonar.db.component.ResourceTypesRule; import org.sonar.db.organization.OrganizationDto; @@ -36,8 +38,8 @@ import org.sonar.server.exceptions.NotFoundException; import org.sonar.server.exceptions.UnauthorizedException; import org.sonar.server.permission.PermissionService; import org.sonar.server.permission.PermissionServiceImpl; -import org.sonar.server.permission.ws.BasePermissionWsTest; import org.sonar.server.permission.RequestValidator; +import org.sonar.server.permission.ws.BasePermissionWsTest; import org.sonar.server.permission.ws.WsParameters; import org.sonarqube.ws.Permissions.WsGroupsResponse; @@ -49,6 +51,7 @@ import static org.sonar.api.web.UserRole.ADMIN; import static org.sonar.api.web.UserRole.CODEVIEWER; import static org.sonar.api.web.UserRole.ISSUE_ADMIN; import static org.sonar.api.web.UserRole.USER; +import static org.sonar.db.permission.PermissionQuery.DEFAULT_PAGE_SIZE; import static org.sonar.db.permission.template.PermissionTemplateTesting.newPermissionTemplateGroupDto; import static org.sonar.db.user.GroupTesting.newGroupDto; import static org.sonar.test.JsonAssert.assertJson; @@ -91,7 +94,6 @@ public class TemplateGroupsActionTest extends BasePermissionWsTest { + GroupDto group = db.users().insertGroup(defaultOrg, "Group-" + i); + db.permissionTemplates().addGroupToTemplate(otherTemplate, group, UserRole.USER); + }); + String lastGroupName = "Group-" + (DEFAULT_PAGE_SIZE + 1); + db.permissionTemplates().addGroupToTemplate(template, db.users().selectGroup(defaultOrg, lastGroupName).get(), UserRole.USER); + loginAsAdmin(db.getDefaultOrganization()); + + WsGroupsResponse response = newRequest() + .setParam(PARAM_TEMPLATE_ID, template.getUuid()) + .executeProtobuf(WsGroupsResponse.class); + + assertThat(response.getGroupsList()) + .extracting("name") + .hasSize(DEFAULT_PAGE_SIZE) + .startsWith("Anyone", lastGroupName, "Group-1"); + } + @Test public void fail_if_not_logged_in() { PermissionTemplateDto template1 = addTemplateToDefaultOrganization(); @@ -375,6 +396,7 @@ public class TemplateGroupsActionTest extends BasePermissionWsTest { + UserDto user = db.users().insertUser("User-" + i); + db.organizations().addMember(db.getDefaultOrganization(), user); + db.permissionTemplates().addUserToTemplate(otherTemplate, user, UserRole.USER); + }); + String lastLogin = "User-" + (DEFAULT_PAGE_SIZE + 1); + db.permissionTemplates().addUserToTemplate(template, db.users().selectUserByLogin(lastLogin).get(), UserRole.USER); + loginAsAdmin(defaultOrg); + + Permissions.UsersWsResponse response = newRequest(null, null) + .setParam(PARAM_TEMPLATE_NAME, template.getName()) + .executeProtobuf(Permissions.UsersWsResponse.class); + + assertThat(response.getUsersList()) + .extracting("login") + .hasSize(DEFAULT_PAGE_SIZE) + .startsWith(lastLogin); + } + @Test public void fail_if_not_a_project_permission() { PermissionTemplateDto template = addTemplateToDefaultOrganization(); -- 2.39.5