From 75f327a6a5fae38e2d1ccf1e5ee83555da7953dc Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Wed, 14 Jun 2017 10:08:58 +0200 Subject: [PATCH] SONAR-9422 Bad error message when creating a project with '%' in key --- .../server/component/ComponentUpdater.java | 14 +- .../component/ComponentUpdaterTest.java | 140 ++++++++++-------- .../server/project/ws/CreateActionTest.java | 83 +++++------ 3 files changed, 123 insertions(+), 114 deletions(-) diff --git a/server/sonar-server/src/main/java/org/sonar/server/component/ComponentUpdater.java b/server/sonar-server/src/main/java/org/sonar/server/component/ComponentUpdater.java index 632c646a888..759033e7f20 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/component/ComponentUpdater.java +++ b/server/sonar-server/src/main/java/org/sonar/server/component/ComponentUpdater.java @@ -82,7 +82,7 @@ public class ComponentUpdater { checkBranchFormat(newComponent.qualifier(), newComponent.branch()); String keyWithBranch = ComponentKeys.createKey(newComponent.key(), newComponent.branch()); checkRequest(!dbClient.componentDao().selectByKey(session, keyWithBranch).isPresent(), - formatMessage("Could not create %s, key already exists: %s", newComponent.qualifier(), keyWithBranch)); + "Could not create %s, key already exists: %s", getQualifierToDisplay(newComponent.qualifier()), keyWithBranch); String uuid = Uuids.create(); ComponentDto component = new ComponentDto() @@ -126,18 +126,18 @@ public class ComponentUpdater { } } - private void checkKeyFormat(String qualifier, String kee) { - checkRequest(isValidModuleKey(kee), formatMessage("Malformed key for %s: %s. Allowed characters are alphanumeric, '-', '_', '.' and ':', with at least one non-digit.", - qualifier, kee)); + private void checkKeyFormat(String qualifier, String key) { + checkRequest(isValidModuleKey(key), + "Malformed key for %s: %s. Allowed characters are alphanumeric, '-', '_', '.' and ':', with at least one non-digit.", getQualifierToDisplay(qualifier), key); } private void checkBranchFormat(String qualifier, @Nullable String branch) { checkRequest(branch == null || ComponentKeys.isValidBranch(branch), - formatMessage("Malformed branch for %s: %s. Allowed characters are alphanumeric, '-', '_', '.' and '/', with at least one non-digit.", qualifier, branch)); + "Malformed branch for %s: %s. Allowed characters are alphanumeric, '-', '_', '.' and '/', with at least one non-digit.", getQualifierToDisplay(qualifier), branch); } - private String formatMessage(String message, String qualifier, String key) { - return String.format(message, i18n.message(Locale.getDefault(), "qualifier." + qualifier, "Project"), key); + private String getQualifierToDisplay(String qualifier) { + return i18n.message(Locale.getDefault(), "qualifier." + qualifier, "Project"); } private void index(ComponentDto project) { diff --git a/server/sonar-server/src/test/java/org/sonar/server/component/ComponentUpdaterTest.java b/server/sonar-server/src/test/java/org/sonar/server/component/ComponentUpdaterTest.java index afacf907f68..fae662acb0a 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/component/ComponentUpdaterTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/component/ComponentUpdaterTest.java @@ -61,9 +61,9 @@ public class ComponentUpdaterTest { private PermissionTemplateService permissionTemplateService = mock(PermissionTemplateService.class); private ComponentUpdater underTest = new ComponentUpdater(db.getDbClient(), i18n, system2, - permissionTemplateService, - new FavoriteUpdater(db.getDbClient()), - projectIndexer); + permissionTemplateService, + new FavoriteUpdater(db.getDbClient()), + projectIndexer); @Test public void should_persist_and_index_when_creating_project() throws Exception { @@ -125,13 +125,13 @@ public class ComponentUpdaterTest { @Test public void create_project_with_branch() throws Exception { ComponentDto project = underTest.create(db.getSession(), - NewComponent.newComponentBuilder() - .setKey(DEFAULT_PROJECT_KEY) - .setName(DEFAULT_PROJECT_NAME) - .setBranch("origin/master") - .setOrganizationUuid(db.getDefaultOrganization().getUuid()) - .build(), - null); + NewComponent.newComponentBuilder() + .setKey(DEFAULT_PROJECT_KEY) + .setName(DEFAULT_PROJECT_NAME) + .setBranch("origin/master") + .setOrganizationUuid(db.getDefaultOrganization().getUuid()) + .build(), + null); assertThat(project.getKey()).isEqualTo("project-key:origin/master"); } @@ -140,10 +140,10 @@ public class ComponentUpdaterTest { public void should_apply_default_permission_template() throws Exception { int userId = 42; NewComponent project = NewComponent.newComponentBuilder() - .setKey(DEFAULT_PROJECT_KEY) - .setName(DEFAULT_PROJECT_NAME) - .setOrganizationUuid(db.getDefaultOrganization().getUuid()) - .build(); + .setKey(DEFAULT_PROJECT_KEY) + .setName(DEFAULT_PROJECT_NAME) + .setOrganizationUuid(db.getDefaultOrganization().getUuid()) + .build(); ComponentDto dto = underTest.create(db.getSession(), project, userId); verify(permissionTemplateService).applyDefault(db.getSession(), dto.getOrganizationUuid(), dto, userId); @@ -153,16 +153,16 @@ public class ComponentUpdaterTest { public void should_add_project_to_user_favorites_if_project_creator_is_defined_in_permission_template() throws Exception { UserDto userDto = db.users().insertUser(); NewComponent project = NewComponent.newComponentBuilder() - .setKey(DEFAULT_PROJECT_KEY) - .setName(DEFAULT_PROJECT_NAME) - .setOrganizationUuid(db.getDefaultOrganization().getUuid()) - .build(); + .setKey(DEFAULT_PROJECT_KEY) + .setName(DEFAULT_PROJECT_NAME) + .setOrganizationUuid(db.getDefaultOrganization().getUuid()) + .build(); when(permissionTemplateService.hasDefaultTemplateWithPermissionOnProjectCreator(eq(db.getSession()), eq(project.getOrganizationUuid()), any(ComponentDto.class))) - .thenReturn(true); + .thenReturn(true); ComponentDto dto = underTest.create(db.getSession(), - project, - userDto.getId()); + project, + userDto.getId()); assertThat(db.favorites().hasFavorite(dto, userDto.getId())).isTrue(); } @@ -170,12 +170,12 @@ public class ComponentUpdaterTest { @Test public void does_not_add_project_to_favorite_when_anonymously_created() throws Exception { ComponentDto project = underTest.create(db.getSession(), - NewComponent.newComponentBuilder() - .setKey(DEFAULT_PROJECT_KEY) - .setName(DEFAULT_PROJECT_NAME) - .setOrganizationUuid(db.getDefaultOrganization().getUuid()) - .build(), - null); + NewComponent.newComponentBuilder() + .setKey(DEFAULT_PROJECT_KEY) + .setName(DEFAULT_PROJECT_NAME) + .setOrganizationUuid(db.getDefaultOrganization().getUuid()) + .build(), + null); assertThat(db.favorites().hasNoFavorite(project)).isTrue(); } @@ -183,12 +183,12 @@ public class ComponentUpdaterTest { @Test public void does_not_add_project_to_favorite_when_project_has_no_permission_on_template() throws Exception { ComponentDto project = underTest.create(db.getSession(), - NewComponent.newComponentBuilder() - .setKey(DEFAULT_PROJECT_KEY) - .setName(DEFAULT_PROJECT_NAME) - .setOrganizationUuid(db.getDefaultOrganization().getUuid()) - .build(), - null); + NewComponent.newComponentBuilder() + .setKey(DEFAULT_PROJECT_KEY) + .setName(DEFAULT_PROJECT_NAME) + .setOrganizationUuid(db.getDefaultOrganization().getUuid()) + .build(), + null); assertThat(db.favorites().hasNoFavorite(project)).isTrue(); } @@ -201,12 +201,12 @@ public class ComponentUpdaterTest { expectedException.expectMessage("Could not create Project, key already exists: " + existing.key()); underTest.create(db.getSession(), - NewComponent.newComponentBuilder() - .setKey(existing.key()) - .setName(DEFAULT_PROJECT_NAME) - .setOrganizationUuid(existing.getOrganizationUuid()) - .build(), - null); + NewComponent.newComponentBuilder() + .setKey(existing.key()) + .setName(DEFAULT_PROJECT_NAME) + .setOrganizationUuid(existing.getOrganizationUuid()) + .build(), + null); } @Test @@ -217,12 +217,12 @@ public class ComponentUpdaterTest { expectedException.expectMessage("Could not create Project, key already exists: " + existing.key()); underTest.create(db.getSession(), - NewComponent.newComponentBuilder() - .setKey(existing.key()) - .setName(DEFAULT_PROJECT_NAME) - .setOrganizationUuid(existing.getOrganizationUuid()) - .build(), - null); + NewComponent.newComponentBuilder() + .setKey(existing.key()) + .setName(DEFAULT_PROJECT_NAME) + .setOrganizationUuid(existing.getOrganizationUuid()) + .build(), + null); } @Test @@ -231,12 +231,26 @@ public class ComponentUpdaterTest { expectedException.expectMessage("Malformed key for Project: 1234"); underTest.create(db.getSession(), - NewComponent.newComponentBuilder() - .setKey("1234") - .setName(DEFAULT_PROJECT_NAME) - .setOrganizationUuid(db.getDefaultOrganization().getUuid()) - .build(), - null); + NewComponent.newComponentBuilder() + .setKey("1234") + .setName(DEFAULT_PROJECT_NAME) + .setOrganizationUuid(db.getDefaultOrganization().getUuid()) + .build(), + null); + } + + @Test + public void properly_fail_when_key_contains_percent_character() { + expectedException.expect(BadRequestException.class); + expectedException.expectMessage("Malformed key for Project: project%Key"); + + underTest.create(db.getSession(), + NewComponent.newComponentBuilder() + .setKey("project%Key") + .setName(DEFAULT_PROJECT_NAME) + .setOrganizationUuid(db.getDefaultOrganization().getUuid()) + .build(), + null); } @Test @@ -245,23 +259,23 @@ public class ComponentUpdaterTest { expectedException.expectMessage("Malformed branch for Project: origin?branch. Allowed characters are alphanumeric, '-', '_', '.' and '/', with at least one non-digit."); underTest.create(db.getSession(), - NewComponent.newComponentBuilder() - .setKey(DEFAULT_PROJECT_KEY) - .setName(DEFAULT_PROJECT_NAME) - .setBranch("origin?branch") - .setOrganizationUuid(db.getDefaultOrganization().getUuid()) - .build(), - null); + NewComponent.newComponentBuilder() + .setKey(DEFAULT_PROJECT_KEY) + .setName(DEFAULT_PROJECT_NAME) + .setBranch("origin?branch") + .setOrganizationUuid(db.getDefaultOrganization().getUuid()) + .build(), + null); } @Test public void persist_and_index_when_creating_view() { NewComponent view = NewComponent.newComponentBuilder() - .setKey("view-key") - .setName("view-name") - .setQualifier(VIEW) - .setOrganizationUuid(db.getDefaultOrganization().getUuid()) - .build(); + .setKey("view-key") + .setName("view-name") + .setQualifier(VIEW) + .setOrganizationUuid(db.getDefaultOrganization().getUuid()) + .build(); ComponentDto returned = underTest.create(db.getSession(), view, null); diff --git a/server/sonar-server/src/test/java/org/sonar/server/project/ws/CreateActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/project/ws/CreateActionTest.java index 44b6226c6a8..ed742f09207 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/project/ws/CreateActionTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/project/ws/CreateActionTest.java @@ -24,37 +24,35 @@ import org.assertj.core.api.AssertionsForClassTypes; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; -import org.mockito.ArgumentCaptor; -import org.mockito.Mockito; import org.sonar.api.server.ws.WebService; import org.sonar.api.utils.System2; -import org.sonar.db.DbSession; import org.sonar.db.DbTester; import org.sonar.db.component.ComponentDto; import org.sonar.db.organization.OrganizationDto; import org.sonar.server.component.ComponentUpdater; -import org.sonar.server.component.NewComponent; +import org.sonar.server.es.ProjectIndexer; import org.sonar.server.exceptions.BadRequestException; import org.sonar.server.exceptions.ForbiddenException; +import org.sonar.server.favorite.FavoriteUpdater; +import org.sonar.server.i18n.I18nRule; import org.sonar.server.organization.BillingValidations; import org.sonar.server.organization.BillingValidations.BillingValidationsException; import org.sonar.server.organization.BillingValidationsProxy; import org.sonar.server.organization.DefaultOrganizationProvider; import org.sonar.server.organization.TestDefaultOrganizationProvider; +import org.sonar.server.permission.PermissionTemplateService; import org.sonar.server.tester.UserSessionRule; import org.sonar.server.ws.TestRequest; import org.sonar.server.ws.WsActionTester; import org.sonarqube.ws.WsProjects.CreateWsResponse; +import org.sonarqube.ws.WsProjects.CreateWsResponse.Project; import org.sonarqube.ws.client.project.CreateRequest; import static org.assertj.core.api.AssertionsForInterfaceTypes.assertThat; import static org.mockito.Matchers.any; -import static org.mockito.Matchers.anyInt; import static org.mockito.Matchers.eq; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; import static org.sonar.core.util.Protobuf.setNullable; import static org.sonar.db.permission.OrganizationPermission.PROVISION_PROJECTS; import static org.sonar.server.project.Visibility.PRIVATE; @@ -79,46 +77,50 @@ public class CreateActionTest { public DbTester db = DbTester.create(system2); @Rule public UserSessionRule userSession = UserSessionRule.standalone(); + @Rule + public I18nRule i18n = new I18nRule().put("qualifier.TRK", "Project"); private DefaultOrganizationProvider defaultOrganizationProvider = TestDefaultOrganizationProvider.from(db); - private ComponentUpdater componentUpdater = mock(ComponentUpdater.class, Mockito.RETURNS_MOCKS); private BillingValidationsProxy billingValidations = mock(BillingValidationsProxy.class); private WsActionTester ws = new WsActionTester( new CreateAction( new ProjectsWsSupport(db.getDbClient(), billingValidations), db.getDbClient(), userSession, - componentUpdater, + new ComponentUpdater(db.getDbClient(), i18n, system2, mock(PermissionTemplateService.class), new FavoriteUpdater(db.getDbClient()), + mock(ProjectIndexer.class)), defaultOrganizationProvider)); @Test public void create_project() throws Exception { userSession.addPermission(PROVISION_PROJECTS, db.getDefaultOrganization()); - expectSuccessfulCallToComponentUpdater(); CreateWsResponse response = call(CreateRequest.builder() .setKey(DEFAULT_PROJECT_KEY) .setName(DEFAULT_PROJECT_NAME) .build()); - assertThat(response.getProject().getKey()).isEqualTo(DEFAULT_PROJECT_KEY); - assertThat(response.getProject().getName()).isEqualTo(DEFAULT_PROJECT_NAME); - assertThat(response.getProject().getQualifier()).isEqualTo("TRK"); + assertThat(response.getProject()) + .extracting(Project::getKey, Project::getName, Project::getQualifier, Project::getVisibility) + .containsOnly(DEFAULT_PROJECT_KEY, DEFAULT_PROJECT_NAME, "TRK", "public"); + assertThat(db.getDbClient().componentDao().selectByKey(db.getSession(), DEFAULT_PROJECT_KEY).get()) + .extracting(ComponentDto::getKey, ComponentDto::name, ComponentDto::qualifier, ComponentDto::scope, ComponentDto::isPrivate) + .containsOnly(DEFAULT_PROJECT_KEY, DEFAULT_PROJECT_NAME, "TRK", "PRJ", false); } @Test public void create_project_with_branch() throws Exception { userSession.addPermission(PROVISION_PROJECTS, db.getDefaultOrganization()); - call(CreateRequest.builder() + CreateWsResponse response = call(CreateRequest.builder() .setKey(DEFAULT_PROJECT_KEY) .setName(DEFAULT_PROJECT_NAME) .setBranch("origin/master") .build()); - NewComponent called = verifyCallToComponentUpdater(); - assertThat(called.key()).isEqualTo(DEFAULT_PROJECT_KEY); - assertThat(called.branch()).isEqualTo("origin/master"); + assertThat(response.getProject()) + .extracting(Project::getKey, Project::getName, Project::getQualifier, Project::getVisibility) + .containsOnly(DEFAULT_PROJECT_KEY + ":origin/master", DEFAULT_PROJECT_NAME, "TRK", "public"); } @Test @@ -126,23 +128,22 @@ public class CreateActionTest { OrganizationDto organization = db.organizations().insert(); userSession.addPermission(PROVISION_PROJECTS, organization); - ws.newRequest() + CreateWsResponse response = ws.newRequest() .setMethod(POST.name()) .setParam("organization", organization.getKey()) .setParam("key", DEFAULT_PROJECT_KEY) .setParam(PARAM_NAME, DEFAULT_PROJECT_NAME) - .execute(); + .executeProtobuf(CreateWsResponse.class); - NewComponent called = verifyCallToComponentUpdater(); - assertThat(called.key()).isEqualTo(DEFAULT_PROJECT_KEY); - assertThat(called.branch()).isNull(); + assertThat(response.getProject()) + .extracting(Project::getKey, Project::getName, Project::getQualifier, Project::getVisibility) + .containsOnly(DEFAULT_PROJECT_KEY, DEFAULT_PROJECT_NAME, "TRK", "public"); } @Test public void apply_project_visibility_public() { OrganizationDto organization = db.organizations().insert(); userSession.addPermission(PROVISION_PROJECTS, organization); - expectSuccessfulCallToComponentUpdater(); CreateWsResponse result = ws.newRequest() .setParam("key", DEFAULT_PROJECT_KEY) @@ -158,7 +159,6 @@ public class CreateActionTest { public void apply_project_visibility_private() { OrganizationDto organization = db.organizations().insert(); userSession.addPermission(PROVISION_PROJECTS, organization); - expectSuccessfulCallToComponentUpdater(); CreateWsResponse result = ws.newRequest() .setParam("key", DEFAULT_PROJECT_KEY) @@ -175,7 +175,6 @@ public class CreateActionTest { OrganizationDto organization = db.organizations().insert(); db.organizations().setNewProjectPrivate(organization, false); userSession.addPermission(PROVISION_PROJECTS, organization); - expectSuccessfulCallToComponentUpdater(); CreateWsResponse result = ws.newRequest() .setParam("key", DEFAULT_PROJECT_KEY) @@ -191,7 +190,6 @@ public class CreateActionTest { OrganizationDto organization = db.organizations().insert(); db.organizations().setNewProjectPrivate(organization, true); userSession.addPermission(PROVISION_PROJECTS, organization); - expectSuccessfulCallToComponentUpdater(); CreateWsResponse result = ws.newRequest() .setParam("key", DEFAULT_PROJECT_KEY) @@ -206,7 +204,6 @@ public class CreateActionTest { public void does_not_fail_to_create_public_projects_when_organization_is_not_allowed_to_use_private_projects() { OrganizationDto organization = db.organizations().insert(); userSession.addPermission(PROVISION_PROJECTS, organization); - expectSuccessfulCallToComponentUpdater(); doThrow(new BillingValidationsException("This organization cannot use project private")).when(billingValidations) .checkCanUpdateProjectVisibility(any(BillingValidations.Organization.class), eq(true)); @@ -224,7 +221,6 @@ public class CreateActionTest { public void fail_to_create_private_projects_when_organization_is_not_allowed_to_use_private_projects() { OrganizationDto organization = db.organizations().insert(); userSession.addPermission(PROVISION_PROJECTS, organization); - expectSuccessfulCallToComponentUpdater(); doThrow(new BillingValidationsException("This organization cannot use project private")).when(billingValidations) .checkCanUpdateProjectVisibility(any(BillingValidations.Organization.class), eq(true)); @@ -242,7 +238,7 @@ public class CreateActionTest { @Test public void fail_when_project_already_exists() throws Exception { OrganizationDto organization = db.organizations().insert(); - when(componentUpdater.create(any(DbSession.class), any(NewComponent.class), anyInt())).thenThrow(BadRequestException.create("already exists")); + db.components().insertPublicProject(project -> project.setKey(DEFAULT_PROJECT_KEY)); userSession.addPermission(PROVISION_PROJECTS, organization); expectedException.expect(BadRequestException.class); @@ -254,6 +250,19 @@ public class CreateActionTest { .build()); } + @Test + public void properly_fail_when_project_key_contains_percent_character() { + userSession.addPermission(PROVISION_PROJECTS, db.getDefaultOrganization()); + + expectedException.expect(BadRequestException.class); + expectedException.expectMessage("Malformed key for Project: project%Key. Allowed characters are alphanumeric, '-', '_', '.' and ':', with at least one non-digit."); + + call(CreateRequest.builder() + .setKey("project%Key") + .setName(DEFAULT_PROJECT_NAME) + .build()); + } + @Test public void fail_when_missing_project_parameter() throws Exception { expectedException.expect(IllegalArgumentException.class); @@ -280,7 +289,6 @@ public class CreateActionTest { @Test public void test_example() { userSession.addPermission(PROVISION_PROJECTS, db.getDefaultOrganization()); - expectSuccessfulCallToComponentUpdater(); String result = ws.newRequest() .setParam("key", DEFAULT_PROJECT_KEY) @@ -304,8 +312,7 @@ public class CreateActionTest { PARAM_ORGANIZATION, PARAM_NAME, PARAM_PROJECT, - PARAM_BRANCH - ); + PARAM_BRANCH); WebService.Param organization = definition.param(PARAM_ORGANIZATION); Assertions.assertThat(organization.description()).isEqualTo("The key of the organization"); @@ -331,16 +338,4 @@ public class CreateActionTest { return httpRequest.executeProtobuf(CreateWsResponse.class); } - private NewComponent verifyCallToComponentUpdater() { - ArgumentCaptor argument = ArgumentCaptor.forClass(NewComponent.class); - verify(componentUpdater).create(any(DbSession.class), argument.capture(), anyInt()); - return argument.getValue(); - } - - private void expectSuccessfulCallToComponentUpdater() { - when(componentUpdater.create(any(DbSession.class), any(NewComponent.class), anyInt())).thenAnswer(invocation -> { - NewComponent newC = invocation.getArgumentAt(1, NewComponent.class); - return new ComponentDto().setKey(newC.key()).setQualifier(newC.qualifier()).setName(newC.name()).setPrivate(newC.isPrivate()); - }); - } } -- 2.39.5