RFR: 1903: Verify User's group membership when processing backport command

[Erik Joelsson]

On Tue, 9 May 2023 21:05:01 GMT, Zhao Song <zsong at openjdk.org> wrote:

> In GitLab, every project is under a group. If a user doesn't have access to the group, then the user will not be able to see any project under the group. 
> 
> However, when processing backport command, the bot will not verify user's group membership, so that it's possible for the bot to create a pull request that is invisible to the user. 
> 
> For example, if a user has access to "groupA" but not "groupB", then he can issue the "/backport groupB/repo2" command on one of the commits in "groupA/repo1". In this case, Skara bot would create a PR that is invisible to the user.
> 
> To fix this issue, we need to verify user's membership after we get the targetRepo. `GitLabRepository#canPush` is very helpful.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/BackportCommand.java line 393:

> 391:     }
> 392: 
> 393:     private boolean verifyGroupMembership(HostedRepository targetRepo, HostUser user, PrintWriter reply) {

The name of this method is misleading as we aren't actually checking group membership, in this patch we are checking write access, which may or may not be indicative of group membership. What actually matters is that the right access is available though.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/BackportCommand.java line 398:

> 396:         if (!targetRepo.forge().name().equals("GitHub") && !targetRepo.forge().name().equals("Test")) {
> 397:             hasGroupMembership = targetRepo.canPush(user);
> 398:         }

This is too much of a hack for me. There needs to be a method on the repository that each relevant implementation of the `HostedRepository` interface has a valid implementation for that answers the question we need to ask. I think that question is something like "is the user allowed to open pull requests against this repo"? For `GitHubRepository` that is probably always true, at least in our context. For `TestHostedRepository` I would assume it's always true, unless you wanted to make it false for testing purposes. In that case you could make it possible to toggle from a test. For `GitLabRepository` the method can probably delegate to `canPush`, but I would like to have that verified.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/BackportCommand.java line 400:

> 398:         }
> 399:         if (!hasGroupMembership) {
> 400:             reply.println("The backport can not be created because you don't have group membership in the target repository!");

Suggestion:

            reply.println("The backport can not be created because you don't have access to the target repository.");

-------------

PR Review Comment: https://git.openjdk.org/skara/pull/1516#discussion_r1189168147
PR Review Comment: https://git.openjdk.org/skara/pull/1516#discussion_r1189172246
PR Review Comment: https://git.openjdk.org/skara/pull/1516#discussion_r1189173084