why do GitLab Guest see confidential issues ?

[genestouxguy]

Hello,

If the webhook authorize to push the Confidential issues events, the user right doesn't work. So if the user has a "Guest" role in GitLab, he can see all the issues with this plugin ? If yes, is it how the plugin was thought or is it a bug ?

Thanks!

[mickmister]

Hi @genestouxguy, thanks for creating this issue. If I understand correctly, the concern is that someone in Mattermost can see issues created from a subscription, no matter what their level of access control is in GitLab.

In the case of creating the webhook, the user creating it must have the appropriate access in GitLab. But the issue you're pointing out is that a given user that doesn't have that access can create subscriptions with this existing webhook? If so, we need to perform some user access validation upon an attempt to create a subscription, to make sure that user has permissions to view these issues. Please let me know your thoughts on this.

Note that the case of a valid user creating a subscription, and users not associated with that project being able to view the incoming messages about the project, is the intended functionality. When someone creates a subscription in a channel, they are essentially associating that channel with that GitLab project, so anyone with a membership to that channel should be assumed to be able to view the posts there associated with the project.

[genestouxguy]

Hi @mickmister,

Thanks for you answer. So to clarrify the question whit this plugin (mattermost-plugin-gitlab), with the workflow following:

1. the user connect to gitlab with /gitlab connect
2. the user subscribe at the project (owner|group/project) with the command /gitlab subscription add ...

In this case, can the user mattermost connect to all projects on GitLab which have configured the subscription webhook, whatever his role in GitLab, exact? It is more clear for you?

So the user mattermost can see all the confidential issues of all projects configured and subscribed?

I understand in Mattermost a user subscribe a channel or invited by other in private channel. But here, the user can subscribe to any confidential information and I didn't get this notion when I read the documentation of this plugin, so sorry to ask.

If I don't want the confidential information in the webhook I can disable this but I try to give the good usage of this plugin to the teams. And, I wanted my guest users to use this plugin to get a better notification system ;) But if this is impossible, it's ok I just give this plugin to the developer team ;)

Thanks!

Maybe my english is not the best so sorry if it's difficult to understand ...

[mickmister]

I think I understand now. There are a few fixes I see here:

• Subscriptions should support configuration to filter out confidential issues (not sure what the default should be)
• When a user creates a subscription, we should check their repo access level
  • If the user does not have access to the repo, we shouldn't allow them to create the subscription
  • If the user does not have access to confidential issues, we should not allow them to create a subscription that includes confidential issues. Maybe we should just automatically set the subscription to exclude the confidential issues.

cc @hanzei and @asatkinson on review for how UX is described here

[hanzei]

👍 for the first point.

But I don't understand why the second one is needed. Without a webhook no events will be shown in MM. And to create one, the user needs to have (admin?) access to the repo. Why do we also need to check for permissions?

[genestouxguy]

Hi @hanzei,

Indeed, if a webhook doesn't exists A Guest user of GitLab repository can't create one. But what if you created the webhook for your team and a Guest User use this webhook ? Then he access all confidential notes/issues.
I hope it's more clear.

Thanks for your work !

[ashutosh887]

Please let me work on this @mickmister @hanzei

[hanzei]

Sure, go for it, @ashutosh887 👍

[m1lt0n]

@ashutosh887 Are you still considering working on the specific issue or you don't have time to do so?

[ashutosh887]

@m1lt0n can you please guide me which section of code to focus on to resolve this?

[mickmister]

@ashutosh887 I'm thinking this should be applied on the API layer, which in this case is the slash command handler for the /gitlab subscriptions add command:

mattermost-plugin-gitlab/server/command.go

Line 529 in de004a9

func (p *Plugin) subscriptionsAddCommand(ctx context.Context, info *gitlab.UserInfo, config *configuration, fullPath, channelID, features string) string {

[Kshitij-Katiyar]

Hey @mickmister @hanzei I explored a bit on this issue. Have some small clarifications on this.

• When a user creates a subscription, we should check their repo access level

  • If the user does not have access to the repo, we shouldn't allow them to create the subscription

We can definitely add this check that before creating the subscription we can check whether that user has access to that repository or not.

• If the user does not have access to confidential issues, we should not allow them to create a subscription that includes confidential issues. Maybe we should just automatically set the subscription to exclude confidential issues.

When we are creating the subscription it is related to either a group or a repo. According to the GitLab documentation.
https://docs.gitlab.com/ee/user/project/issues/confidential_issues.html#permissions-and-access-to-confidential-issues

However, a user with the Guest role can create confidential issues, but can only view the ones that they created themselves.

Users with the Guest role or non-members can read the confidential issue if they are assigned to the issue. When a Guest user or non-member is unassigned from a confidential issue, they can no longer view it.

So as we can see just checking whether that user is a guest user or not does not make sense here as guest users can also see the confidential issues if they have created it or they are assigned to them.
I think what we can do here is filter out the notifications coming from GitLab. We are storing the id of the user who created the subscriptions, so at the time of the event coming from Gitlab, we can check whether the user who created the subscription has access to this or not, and depending upon that we can either show the notification or not.

• Subscriptions should support configuration to filter out confidential issues (not sure what the default should be).

Please explain what should I do for this part.

[mickmister]

So as we can see just checking whether that user is a guest user or not does not make sense here as guest users can also see the confidential issues if they have created it or they are assigned to them.

Right, but the subscription is not scoped to those issues. Let's assume they don't have access to the issues in general for the purpose of this discussion.

We are storing the id of the user who created the subscriptions, so at the time of the event coming from Gitlab, we can check whether the user who created the subscription has access to this or not, and depending upon that we can either show the notification or not.

I think this is beside the point. I don't think we should have logic based on the creator at the point of the webhook event coming in. We should instead defend against people creating subscriptions. Guests and non-members of a project should not be able to create subscriptions for that project.

Subscriptions should support configuration to filter out confidential issues (not sure what the default should be).

The /gitlab subscriptions add command can accept an additional flag for filtering out confidential issues. When a webhook event comes and it is a confidential issue, we will block the event if the subscription is configured to avoid these.

[mickmister]

cc @Kshitij-Katiyar on the comment above

[mickmister]

When a user creates a subscription, we should check their repo access level

• If the user does not have access to the repo, we shouldn't allow them to create the subscription
• If the user does not have access to confidential issues, we should not allow them to create a subscription that includes confidential issues. Maybe we should just automatically set the subscription to exclude the confidential issues.

But I don't understand why the second one is needed. Without a webhook no events will be shown in MM. And to create one, the user needs to have (admin?) access to the repo. Why do we also need to check for permissions?

@hanzei To be clear, I'm talking about "subscriptions" and not "webhooks", in case you thought I was talking about setting up a webhook.

Take this example:

User A - system admin that sets up a webhook for a private project
User B - doesn't have access to private project

User B creates a GitLab plugin subscription for the private project. No access permissions are checked here. Now User B can see everything going on in the private project.

The security model around this being acceptable is essentially "if an admin creates a webhook, any incoming events are permitted to be seen by anyone on the Mattermost server that has a connected GitLab account." If that is the model we want, then that's fine, though I think there should be a check of "does this user have access to the GitLab project they're subscribing to".

It's probably an uphill battle to try to mimic the access control system that's in GitLab itself, but if there are any places of low hanging fruit that customers request, I think we should consider implementing them. Maybe have it opt-out so admins can configure their system how they want to.

[hanzei]

Makes sense, thanks for the clarification @mickmister 👍

[raghavaggarwal2308]

Fixed via #376