Check permissions of uploader when validating imports on submit We do validate code owner config files on upload. One validation that is performed for imports is that the project/branch of the imported code owner config is visible to the uploader. It's possible that this project/branch is not visible to everyone. By referring to another project/branch in a code owner config file the uploader reveals the existence of the project/branch to everyone who can see the code owner config file. If enabled, the validation of code owner config files is also done on submit. At this point it's intended to do the exact same validation as on upload. In particular this means that all visibility checks should be done from the perspective of the uploader (and not for the submitter) [1]. We already did the visibility checks for the code owners for the uploader, but wrongly checked the visibility of projects/branches from which code owner config files are imported for the submitter. This change fixes this so that visibility checks for projects/branches from which code owner config files are imported are also done for the uploader. [1] https://gerrit-review.googlesource.com/plugins/code-owners/Documentation/validation.html#:~:text=If%20enabled%2C%20on%20submit%20we%20repeat%20the%20exact%20same%20validation%20that%20was%20done%20on%20upload.%20This%20means%2C%20all%20visibility%20checks%20will%20be%20done%20from%20the%20perspective%20of%20the%20uploader. Bug: Google b/365550280 Change-Id: I6b1cc65f90f1fce1d9fb2930d40103390b6d10f0 Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java index 858b1a2..8411568 100644 --- a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java +++ b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
@@ -938,7 +938,10 @@ codeOwnerBackend.getFilePath(codeOwnerConfig.key()), codeOwnerConfig), validateImports( - branchNameKey, codeOwnerBackend.getFilePath(codeOwnerConfig.key()), codeOwnerConfig)); + branchNameKey, + user, + codeOwnerBackend.getFilePath(codeOwnerConfig.key()), + codeOwnerConfig)); } /** @@ -1015,6 +1018,8 @@ * Validates the imports of the given code owner config. * * @param branchNameKey the branch and the project + * @param user the user for which the visibility of the imported code owner config should be + * checked * @param codeOwnerConfigFilePath the path of the code owner config file which contains the code * owner config * @param codeOwnerConfig the code owner config for which the imports should be validated @@ -1022,7 +1027,10 @@ * if there are no issues */ private Stream<CommitValidationMessage> validateImports( - BranchNameKey branchNameKey, Path codeOwnerConfigFilePath, CodeOwnerConfig codeOwnerConfig) { + BranchNameKey branchNameKey, + IdentifiedUser user, + Path codeOwnerConfigFilePath, + CodeOwnerConfig codeOwnerConfig) { try { RevCommit codeOwnerConfigRevision; try (Repository repo = repoManager.openRepository(branchNameKey.project()); @@ -1035,6 +1043,7 @@ codeOwnerConfigReference -> validateCodeOwnerConfigReference( branchNameKey, + user, codeOwnerConfigFilePath, codeOwnerConfig.key(), codeOwnerConfigRevision, @@ -1046,6 +1055,7 @@ codeOwnerConfigReference -> validateCodeOwnerConfigReference( branchNameKey, + user, codeOwnerConfigFilePath, codeOwnerConfig.key(), codeOwnerConfigRevision, @@ -1063,6 +1073,8 @@ * Validates a code owner config reference. * * @param branchNameKey the branch and the project + * @param user the user for which the visibility of the imported code owner config should be + * checked * @param codeOwnerConfigFilePath the path of the code owner config file which contains the code * owner config reference * @param keyOfImportingCodeOwnerConfig key of the importing code owner config @@ -1075,6 +1087,7 @@ */ private Optional<CommitValidationMessage> validateCodeOwnerConfigReference( BranchNameKey branchNameKey, + IdentifiedUser user, Path codeOwnerConfigFilePath, CodeOwnerConfig.Key keyOfImportingCodeOwnerConfig, RevCommit codeOwnerConfigRevision, @@ -1093,7 +1106,7 @@ } Optional<ProjectState> projectState = projectCache.get(keyOfImportedCodeOwnerConfig.project()); - if (!projectState.isPresent() || !isProjectReadable(keyOfImportedCodeOwnerConfig)) { + if (!projectState.isPresent() || !isProjectReadable(keyOfImportedCodeOwnerConfig, user)) { // we intentionally use the same error message for non-existing and non-readable projects so // that uploaders cannot probe for the existence of projects (e.g. deduce from the error // message whether a project exists) @@ -1122,7 +1135,7 @@ Optional<ObjectId> revision = getRevision( keyOfImportingCodeOwnerConfig, codeOwnerConfigRevision, keyOfImportedCodeOwnerConfig); - if (!revision.isPresent() || !isBranchReadable(keyOfImportedCodeOwnerConfig)) { + if (!revision.isPresent() || !isBranchReadable(keyOfImportedCodeOwnerConfig, user)) { // we intentionally use the same error message for non-existing and non-readable branches so // that uploaders cannot probe for the existence of branches (e.g. deduce from the error // message whether a branch exists) @@ -1217,10 +1230,11 @@ .getFilePath(keyOfImportedCodeOwnerConfig)); } - private boolean isProjectReadable(CodeOwnerConfig.Key keyOfImportedCodeOwnerConfig) { + private boolean isProjectReadable( + CodeOwnerConfig.Key keyOfImportedCodeOwnerConfig, IdentifiedUser user) { try { return permissionBackend - .currentUser() + .user(user) .project(keyOfImportedCodeOwnerConfig.project()) .test(ProjectPermission.ACCESS); } catch (PermissionBackendException e) { @@ -1229,10 +1243,11 @@ } } - private boolean isBranchReadable(CodeOwnerConfig.Key keyOfImportedCodeOwnerConfig) { + private boolean isBranchReadable( + CodeOwnerConfig.Key keyOfImportedCodeOwnerConfig, IdentifiedUser user) { try { return permissionBackend - .currentUser() + .user(user) .project(keyOfImportedCodeOwnerConfig.project()) .ref(keyOfImportedCodeOwnerConfig.ref()) .test(RefPermission.READ);
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java index 0198cc4..4c1b823 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java +++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
@@ -32,8 +32,10 @@ import com.google.gerrit.acceptance.TestAccount; import com.google.gerrit.acceptance.TestProjectInput; import com.google.gerrit.acceptance.config.GerritConfig; +import com.google.gerrit.acceptance.testsuite.group.GroupOperations; import com.google.gerrit.acceptance.testsuite.project.ProjectOperations; import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations; +import com.google.gerrit.entities.AccountGroup; import com.google.gerrit.entities.BranchNameKey; import com.google.gerrit.entities.Permission; import com.google.gerrit.entities.Project; @@ -94,6 +96,7 @@ @Inject private RequestScopeOperations requestScopeOperations; @Inject private ProjectOperations projectOperations; + @Inject private GroupOperations groupOperations; @Inject private DynamicItem<UrlFormatter> urlFormatter; private FindOwnersCodeOwnerConfigParser findOwnersCodeOwnerConfigParser; @@ -3198,6 +3201,326 @@ assertThat(gApi.changes().id(r.getChangeId()).get().status).isEqualTo(ChangeStatus.MERGED); } + @Test + @GerritConfig(name = "plugin.code-owners.enableValidationOnCommitReceived", value = "false") + @GerritConfig(name = "plugin.code-owners.enableValidationOnSubmit", value = "true") + public void cannotSubmitConfigWithImportFromProjectThatIsNotVisibleToUploader() throws Exception { + skipTestIfImportsNotSupportedByCodeOwnersBackend(); + + setAsRootCodeOwners(admin); + TestAccount uploader = + accountCreator.create("uploader", "uploader@example.com", "Uploader", "Uploader"); + AccountGroup.UUID blockedGroup = groupOperations.newGroup().addMember(uploader.id()).create(); + + // create a project that is not visible to the uploader with a code owner config file that we + // try to import + Project.NameKey nonVisibleProject = + projectOperations.newProject().name(name("non-visible-project")).create(); + projectOperations + .project(nonVisibleProject) + .forUpdate() + .add(block(Permission.READ).ref("refs/*").group(blockedGroup)) + .update(); + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(nonVisibleProject) + .branch("master") + .folderPath("/") + .addCodeOwnerEmail(user.email()) + .create(); + + // create a code owner config that imports a code owner config from a project that is not + // visible to the uploader + CodeOwnerConfig.Key keyOfImportingCodeOwnerConfig = createCodeOwnerConfigKey("/"); + CodeOwnerConfigReference codeOwnerConfigReference = + CodeOwnerConfigReference.builder( + CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY, + codeOwnerConfigOperations + .codeOwnerConfig(CodeOwnerConfig.Key.create(nonVisibleProject, "master", "/")) + .getFilePath()) + .setProject(nonVisibleProject) + .build(); + CodeOwnerConfig codeOwnerConfig = + createCodeOwnerConfigWithImport( + keyOfImportingCodeOwnerConfig, + CodeOwnerConfigImportType.GLOBAL, + codeOwnerConfigReference); + + // upload works because the validation on commitReceived is disabled + PushOneCommit.Result r = + createChange( + uploader, + "Add code owners", + codeOwnerConfigOperations + .codeOwnerConfig(keyOfImportingCodeOwnerConfig) + .getJGitFilePath(), + format(codeOwnerConfig)); + assertOkWithHints( + r, + "skipping validation of code owner config files", + "code owners config validation is disabled"); + + // Try to submit as a user that is not blocked and can see the project from which the code + // owner config file is imported. This fails because the visibility of the import is checked for + // the uploader who is blocked and cannot see the project from which the code owner config file + // is imported. + requestScopeOperations.setApiUser(admin.id()); + approve(r.getChangeId()); + ResourceConflictException exception = + assertThrows( + ResourceConflictException.class, + () -> gApi.changes().id(r.getChangeId()).current().submit()); + assertThat(exception) + .hasMessageThat() + .isEqualTo( + String.format( + "Failed to submit 1 change due to the following problems:\n" + + "Change %s: [code-owners] invalid code owner config files (see %s for help):\n" + + " ERROR: invalid %s import in '%s': project '%s' not found", + r.getChange().getId().toString(), + getHelpPage(), + CodeOwnerConfigImportType.GLOBAL.getType(), + codeOwnerConfigOperations + .codeOwnerConfig(keyOfImportingCodeOwnerConfig) + .getFilePath(), + nonVisibleProject.get())); + } + + @Test + @GerritConfig(name = "plugin.code-owners.enableValidationOnCommitReceived", value = "false") + @GerritConfig(name = "plugin.code-owners.enableValidationOnSubmit", value = "true") + public void canSubmitConfigWithImportFromProjectThatIsNotVisibleToTheSubmitterButToTheUploader() + throws Exception { + skipTestIfImportsNotSupportedByCodeOwnersBackend(); + + setAsRootCodeOwners(admin); + + TestAccount submitter = + accountCreator.create("submitter", "submitter@example.com", "Submitter", "Submitter"); + AccountGroup.UUID blockedGroup = groupOperations.newGroup().addMember(submitter.id()).create(); + + // create a project that is not visible to the submitter with a code owner config file that we + // try to import + Project.NameKey nonVisibleProject = + projectOperations.newProject().name(name("non-visible-project")).create(); + projectOperations + .project(nonVisibleProject) + .forUpdate() + .add(block(Permission.READ).ref("refs/*").group(blockedGroup)) + .update(); + projectOperations + .project(project) + .forUpdate() + .add(allow(Permission.SUBMIT).ref("refs/*").group(REGISTERED_USERS)) + .update(); + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(nonVisibleProject) + .branch("master") + .folderPath("/") + .addCodeOwnerEmail(user.email()) + .create(); + + // create a code owner config that imports a code owner config from a project that is not + // visible to the submitter + CodeOwnerConfig.Key keyOfImportingCodeOwnerConfig = createCodeOwnerConfigKey("/"); + CodeOwnerConfigReference codeOwnerConfigReference = + CodeOwnerConfigReference.builder( + CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY, + codeOwnerConfigOperations + .codeOwnerConfig(CodeOwnerConfig.Key.create(nonVisibleProject, "master", "/")) + .getFilePath()) + .setProject(nonVisibleProject) + .build(); + CodeOwnerConfig codeOwnerConfig = + createCodeOwnerConfigWithImport( + keyOfImportingCodeOwnerConfig, + CodeOwnerConfigImportType.GLOBAL, + codeOwnerConfigReference); + + PushOneCommit.Result r = + createChange( + user, + "Add code owners", + codeOwnerConfigOperations + .codeOwnerConfig(keyOfImportingCodeOwnerConfig) + .getJGitFilePath(), + format(codeOwnerConfig)); + assertOkWithHints( + r, + "skipping validation of code owner config files", + "code owners config validation is disabled"); + + requestScopeOperations.setApiUser(admin.id()); + approve(r.getChangeId()); + + // Submit as a user that is blocked and cannot see the project from which the code owner config + // file is imported. This works because the visibility of the import is checked for the uploader + // who is not blocked and can see the project from which the code owner config file is imported. + requestScopeOperations.setApiUser(submitter.id()); + gApi.changes().id(r.getChangeId()).current().submit(); + } + + @Test + @GerritConfig(name = "plugin.code-owners.enableValidationOnCommitReceived", value = "false") + @GerritConfig(name = "plugin.code-owners.enableValidationOnSubmit", value = "true") + public void cannotSubmitConfigWithImportFromBranchThatIsNotVisibleToUploader() throws Exception { + skipTestIfImportsNotSupportedByCodeOwnersBackend(); + + setAsRootCodeOwners(admin); + TestAccount uploader = + accountCreator.create("uploader", "uploader@example.com", "Uploader", "Uploader"); + AccountGroup.UUID blockedGroup = groupOperations.newGroup().addMember(uploader.id()).create(); + + // create a project with a branch that is not visible to the uploader and that contains a code + // owner config file + Project.NameKey otherProject = + projectOperations.newProject().name(name("other-project")).create(); + projectOperations + .project(otherProject) + .forUpdate() + .add(block(Permission.READ).ref("refs/heads/master").group(blockedGroup)) + .update(); + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(otherProject) + .branch("master") + .folderPath("/") + .addCodeOwnerEmail(user.email()) + .create(); + + // create a code owner config that imports a code owner config from a branch that is not + // visible to the uploader + CodeOwnerConfig.Key keyOfImportingCodeOwnerConfig = createCodeOwnerConfigKey("/"); + CodeOwnerConfigReference codeOwnerConfigReference = + CodeOwnerConfigReference.builder( + CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY, + codeOwnerConfigOperations + .codeOwnerConfig(CodeOwnerConfig.Key.create(otherProject, "master", "/")) + .getFilePath()) + .setProject(otherProject) + .build(); + CodeOwnerConfig codeOwnerConfig = + createCodeOwnerConfigWithImport( + keyOfImportingCodeOwnerConfig, + CodeOwnerConfigImportType.GLOBAL, + codeOwnerConfigReference); + + // upload works because the validation on commitReceived is disabled + PushOneCommit.Result r = + createChange( + uploader, + "Add code owners", + codeOwnerConfigOperations + .codeOwnerConfig(keyOfImportingCodeOwnerConfig) + .getJGitFilePath(), + format(codeOwnerConfig)); + assertOkWithHints( + r, + "skipping validation of code owner config files", + "code owners config validation is disabled"); + + // Try to submit as a user that is not blocked and can see the branch from which the code + // owner config file is imported. This fails because the visibility of the import is checked for + // the uploader who is blocked and cannot see the branch from which the code owner config file + // is imported. + requestScopeOperations.setApiUser(admin.id()); + approve(r.getChangeId()); + ResourceConflictException exception = + assertThrows( + ResourceConflictException.class, + () -> gApi.changes().id(r.getChangeId()).current().submit()); + assertThat(exception) + .hasMessageThat() + .isEqualTo( + String.format( + "Failed to submit 1 change due to the following problems:\n" + + "Change %s: [code-owners] invalid code owner config files (see %s for help):\n" + + " ERROR: invalid %s import in '%s': branch 'master' not found in project '%s'", + r.getChange().getId().toString(), + getHelpPage(), + CodeOwnerConfigImportType.GLOBAL.getType(), + codeOwnerConfigOperations + .codeOwnerConfig(keyOfImportingCodeOwnerConfig) + .getFilePath(), + otherProject.get())); + } + + @Test + @GerritConfig(name = "plugin.code-owners.enableValidationOnCommitReceived", value = "false") + @GerritConfig(name = "plugin.code-owners.enableValidationOnSubmit", value = "true") + public void canSubmitConfigWithImportFromBranchThatIsNotVisibleToTheSubmitterButToTheUploader() + throws Exception { + skipTestIfImportsNotSupportedByCodeOwnersBackend(); + + setAsRootCodeOwners(admin); + + TestAccount submitter = + accountCreator.create("submitter", "submitter@example.com", "Submitter", "Submitter"); + AccountGroup.UUID blockedGroup = groupOperations.newGroup().addMember(submitter.id()).create(); + + // create a project with a branch that is not visible to the submitter and that contains a code + // owner config file + Project.NameKey otherProject = + projectOperations.newProject().name(name("pther-project")).create(); + projectOperations + .project(otherProject) + .forUpdate() + .add(block(Permission.READ).ref("refs/heads/master").group(blockedGroup)) + .update(); + projectOperations + .project(project) + .forUpdate() + .add(allow(Permission.SUBMIT).ref("refs/*").group(REGISTERED_USERS)) + .update(); + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(otherProject) + .branch("master") + .folderPath("/") + .addCodeOwnerEmail(user.email()) + .create(); + + // create a code owner config that imports a code owner config from a project that is not + // visible to the submitter + CodeOwnerConfig.Key keyOfImportingCodeOwnerConfig = createCodeOwnerConfigKey("/"); + CodeOwnerConfigReference codeOwnerConfigReference = + CodeOwnerConfigReference.builder( + CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY, + codeOwnerConfigOperations + .codeOwnerConfig(CodeOwnerConfig.Key.create(otherProject, "master", "/")) + .getFilePath()) + .setProject(otherProject) + .build(); + CodeOwnerConfig codeOwnerConfig = + createCodeOwnerConfigWithImport( + keyOfImportingCodeOwnerConfig, + CodeOwnerConfigImportType.GLOBAL, + codeOwnerConfigReference); + + PushOneCommit.Result r = + createChange( + user, + "Add code owners", + codeOwnerConfigOperations + .codeOwnerConfig(keyOfImportingCodeOwnerConfig) + .getJGitFilePath(), + format(codeOwnerConfig)); + assertOkWithHints( + r, + "skipping validation of code owner config files", + "code owners config validation is disabled"); + + requestScopeOperations.setApiUser(admin.id()); + approve(r.getChangeId()); + + // Submit as a user that is blocked and cannot see the branch from which the code owner config + // file is imported. This works because the visibility of the import is checked for the uploader + // who is not blocked and can see the branch from which the code owner config file is imported. + requestScopeOperations.setApiUser(submitter.id()); + gApi.changes().id(r.getChangeId()).current().submit(); + } + private CodeOwnerConfig createCodeOwnerConfigWithImport( CodeOwnerConfig.Key keyOfImportingCodeOwnerConfig, CodeOwnerConfigImportType importType,