Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GH-326] Added subscription flag for filtering out confidential issues, and disallow guests from creating subscriptions #376

Merged
merged 8 commits into from
Mar 8, 2024
10 changes: 9 additions & 1 deletion server/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ const commandHelp = `* |/gitlab connect| - Connect your Mattermost account to yo
* |/gitlab subscriptions add owner[/repo] [features]| - Subscribe the current channel to receive notifications about opened merge requests and issues for a group or repository
* |features| is a comma-delimited list of one or more the following:
* issues - includes new and closed issues
* confidential_issues - includes new and closed confidential issues
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to make this opt-in, so as to not break existing subscriptions. If this is the case, the flag should be exclude_confidential_issues. Thoughts on this? @jupenur @esarafianou

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mickmister Fixed all the review comments apart for this one. Waiting for replies.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hanzei Do you have an opinion on this?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mickmister Hadn't been notified about this ping and bumped into it today.

The ideal behavior is secure by default. That would mean excluding confidential issues by default.

Since this is a breaking change for existing subscriptions, we can consider a major release.

* jobs - includes jobs status updates
* merges - includes new and closed merge requests
* pushes - includes pushes
Expand Down Expand Up @@ -514,6 +515,7 @@ func (p *Plugin) subscriptionsListCommand(channelID string) string {
if err != nil {
return err.Error()
}

if len(subs) == 0 {
txt = "Currently there are no subscriptions in this channel"
} else {
Expand Down Expand Up @@ -544,6 +546,12 @@ func (p *Plugin) subscriptionsAddCommand(ctx context.Context, info *gitlab.UserI
return err.Error()
}

if hasPermission := p.permissionToProject(ctx, info.UserID, namespace, project); !hasPermission {
msg := "You don't have the permissions to create subscriptions for this project"
Kshitij-Katiyar marked this conversation as resolved.
Show resolved Hide resolved
p.client.Log.Warn(msg)
return msg
}

updatedSubscriptions, subscribeErr := p.Subscribe(info, namespace, project, channelID, features)
if subscribeErr != nil {
p.client.Log.Warn(
Expand Down Expand Up @@ -710,7 +718,7 @@ func getAutocompleteData(config *configuration) *model.AutocompleteData {

subscriptionsAdd := model.NewAutocompleteData(commandAdd, "owner[/repo] [features]", "Subscribe the current channel to receive notifications from a project")
subscriptionsAdd.AddTextArgument("Project path: includes user or group name with optional slash project name", "owner[/repo]", "")
subscriptionsAdd.AddTextArgument("comma-delimited list of features to subscribe to: issues, merges, pushes, issue_comments, merge_request_comments, pipeline, tag, pull_reviews, label:<labelName>", "[features] (optional)", `/[^,-\s]+(,[^,-\s]+)*/`)
subscriptionsAdd.AddTextArgument("comma-delimited list of features to subscribe to: issues, confidential_issues, merges, pushes, issue_comments, merge_request_comments, pipeline, tag, pull_reviews, label:<labelName>", "[features] (optional)", `/[^,-\s]+(,[^,-\s]+)*/`)
subscriptions.AddCommand(subscriptionsAdd)

subscriptionsDelete := model.NewAutocompleteData(commandDelete, "owner[/repo]", "Unsubscribe the current channel from a repository")
Expand Down
107 changes: 96 additions & 11 deletions server/command_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,20 @@ package main

import (
"context"
"encoding/json"
"testing"
"time"

"github.com/golang/mock/gomock"
pluginapi "github.com/mattermost/mattermost-plugin-api"
"github.com/mattermost/mattermost-server/v6/model"
"github.com/mattermost/mattermost-server/v6/plugin/plugintest"
"golang.org/x/oauth2"

"github.com/pkg/errors"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
gitLabAPI "github.com/xanzy/go-gitlab"

"github.com/mattermost/mattermost-plugin-gitlab/server/gitlab"
Expand All @@ -23,10 +28,21 @@ type subscribeCommandTest struct {
want string
webhookInfo []*gitlab.WebhookInfo
mattermostURL string
noAccess bool
projectHookErr error
getProjectErr error
mockGitlab bool
}

func getTestConfig() *configuration {
return &configuration{
GitlabURL: "https://example.com",
GitlabOAuthClientID: "client_id",
GitlabOAuthClientSecret: "secret",
EncryptionKey: "encryption___key",
}
}

const subscribeSuccessMessage = "Successfully subscribed to group/project.\nA Webhook is needed, run ```/gitlab webhook add group/project``` to create one now."

var subscribeCommandTests = []subscribeCommandTest{
Expand All @@ -35,6 +51,25 @@ var subscribeCommandTests = []subscribeCommandTest{
parameters: []string{"list"},
want: "Currently there are no subscriptions in this channel",
},
{
testName: "No Repository permissions",
parameters: []string{"add", "group/project"},
mockGitlab: true,
want: "You don't have the permissions to create subscriptions for this project",
webhookInfo: []*gitlab.WebhookInfo{{URL: "example.com/somewebhookURL"}},
noAccess: true,
mattermostURL: "example.com",
getProjectErr: errors.New("unable to get project"),
},
{
testName: "Guest permissions only",
parameters: []string{"add", "group/project"},
mockGitlab: true,
want: "You don't have the permissions to create subscriptions for this project",
webhookInfo: []*gitlab.WebhookInfo{{URL: "example.com/somewebhookURL"}},
noAccess: true,
mattermostURL: "example.com",
},
{
testName: "Hook Found",
parameters: []string{"add", "group/project"},
Expand Down Expand Up @@ -76,9 +111,11 @@ func TestSubscribeCommand(t *testing.T) {
mockCtrl := gomock.NewController(t)

channelID := "12345"
userInfo := &gitlab.UserInfo{}
userInfo := &gitlab.UserInfo{
UserID: "user_id",
}

p := getTestPlugin(t, mockCtrl, test.webhookInfo, test.mattermostURL, test.projectHookErr, test.mockGitlab)
p := getTestPlugin(t, mockCtrl, test.webhookInfo, test.mattermostURL, test.projectHookErr, test.getProjectErr, test.mockGitlab, test.noAccess)
subscribeMessage := p.subscribeCommand(context.Background(), test.parameters, channelID, &configuration{}, userInfo)

assert.Equal(t, test.want, subscribeMessage, "Subscribe command message should be the same.")
Expand Down Expand Up @@ -211,34 +248,82 @@ func TestListWebhookCommand(t *testing.T) {
}
}

func getTestPlugin(t *testing.T, mockCtrl *gomock.Controller, hooks []*gitlab.WebhookInfo, mattermostURL string, projectHookErr error, mockGitlab bool) *Plugin {
func getTestPlugin(t *testing.T, mockCtrl *gomock.Controller, hooks []*gitlab.WebhookInfo, mattermostURL string, projectHookErr error, getProjectErr error, mockGitlab, noAccess bool) *Plugin {
p := new(Plugin)

accessLevel := gitLabAPI.OwnerPermission
if noAccess {
accessLevel = gitLabAPI.GuestPermissions
}

mockedClient := mocks.NewMockGitlab(mockCtrl)
if mockGitlab {
mockedClient.EXPECT().ResolveNamespaceAndProject(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return("group", "project", nil)
mockedClient.EXPECT().GetProjectHooks(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(hooks, projectHookErr)
if projectHookErr == nil {
mockedClient.EXPECT().GetGroupHooks(gomock.Any(), gomock.Any(), gomock.Any()).Return(hooks, projectHookErr)
if getProjectErr != nil {
mockedClient.EXPECT().GetProject(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, getProjectErr)
} else {
mockedClient.EXPECT().GetProject(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(&gitLabAPI.Project{
Permissions: &gitLabAPI.Permissions{
ProjectAccess: &gitLabAPI.ProjectAccess{
AccessLevel: accessLevel,
},
},
}, nil)
}

if !noAccess {
mockedClient.EXPECT().GetProjectHooks(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(hooks, projectHookErr)
if projectHookErr == nil {
mockedClient.EXPECT().GetGroupHooks(gomock.Any(), gomock.Any(), gomock.Any()).Return(hooks, projectHookErr)
}
}
}

p.GitlabClient = mockedClient

api := &plugintest.API{}
p.SetAPI(api)
Kshitij-Katiyar marked this conversation as resolved.
Show resolved Hide resolved
p.client = pluginapi.NewClient(api, p.Driver)

conf := &model.Config{}
conf.ServiceSettings.SiteURL = &mattermostURL
api.On("GetConfig", mock.Anything).Return(conf)

var subVal []byte
api.On("KVGet", mock.Anything).Return(subVal, nil)
api.On("KVSet", mock.Anything, mock.Anything).Return(nil)
api.On("KVSetWithOptions", mock.AnythingOfType("string"), mock.Anything, mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil)
api.On("PublishWebSocketEvent", mock.Anything, mock.Anything, mock.Anything).Return(nil)

p.SetAPI(api)
p.client = pluginapi.NewClient(api, p.Driver)
config := getTestConfig()
p.configuration = config
p.initializeAPI()

token := oauth2.Token{
AccessToken: "access_token",
Expiry: time.Now().Add(1 * time.Hour),
}

info := gitlab.UserInfo{
UserID: "user_id",
Token: &token,
GitlabUsername: "gitlab_username",
}

encryptedToken, err := encrypt([]byte(config.EncryptionKey), info.Token.AccessToken)
require.NoError(t, err)

info.Token.AccessToken = encryptedToken

jsonInfo, err := json.Marshal(info)
require.NoError(t, err)

api.On("KVGet", "user_id_gitlabtoken").Return(jsonInfo, nil)
var subVal []byte
api.On("KVGet", "subscriptions").Return(subVal, nil)
api.On("LogWarn",
mock.AnythingOfTypeArgument("string"),
mock.AnythingOfTypeArgument("string"),
mock.AnythingOfTypeArgument("string"),
mock.AnythingOfTypeArgument("string"),
mock.AnythingOfTypeArgument("string"))

return p
}
Expand Down
7 changes: 6 additions & 1 deletion server/subscription/subscription.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ var allFeatures = map[string]bool{
"pipeline": true,
"tag": true,
"pull_reviews": true,
// "label:": true,//particular case for label:XXX
"confidential_issues": true,
// "label:": true,//particular case for label:XXX
}

type Subscription struct {
Expand Down Expand Up @@ -63,6 +64,10 @@ func (s *Subscription) Issues() bool {
return strings.Contains(s.Features, "issues")
}

func (s *Subscription) ConfidentialIssues() bool {
return strings.Contains(s.Features, "confidential_issues")
}

func (s *Subscription) Pushes() bool {
return strings.Contains(s.Features, "pushes")
}
Expand Down
20 changes: 16 additions & 4 deletions server/webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,8 @@ func (p *Plugin) handleWebhook(w http.ResponseWriter, r *http.Request) {
return
}

event, err := gitlabLib.ParseWebhook(gitlabLib.WebhookEventType(r), body)
eventType := gitlabLib.WebhookEventType(r)
event, err := gitlabLib.ParseWebhook(eventType, body)
if err != nil {
p.client.Log.Debug("Can't parse webhook", "err", err.Error(), "header", r.Header.Get("X-Gitlab-Event"), "event", string(body))
http.Error(w, "Unable to handle request", http.StatusBadRequest)
Expand All @@ -97,7 +98,7 @@ func (p *Plugin) handleWebhook(w http.ResponseWriter, r *http.Request) {
repoPrivate = event.Project.Visibility == gitlabLib.PrivateVisibility
pathWithNamespace = event.Project.PathWithNamespace
fromUser = event.User.Username
handlers, errHandler = p.WebhookHandler.HandleIssue(ctx, event)
handlers, errHandler = p.WebhookHandler.HandleIssue(ctx, event, eventType)
case *gitlabLib.IssueCommentEvent:
repoPrivate = event.Project.Visibility == gitlabLib.PrivateVisibility
pathWithNamespace = event.Project.PathWithNamespace
Expand Down Expand Up @@ -207,12 +208,23 @@ func (p *Plugin) permissionToProject(ctx context.Context, userID, namespace, pro
return false
}

if result, err := p.GitlabClient.GetProject(ctx, info, namespace, project); result == nil || err != nil {
result, err := p.GitlabClient.GetProject(ctx, info, namespace, project)
if result == nil || err != nil {
if err != nil {
p.client.Log.Warn("can't get project in webhook", "err", err.Error(), "project", namespace+"/"+project)
p.client.Log.Warn("Can't get project in webhook", "err", err.Error(), "project", namespace+"/"+project)
}
return false
}

if result.Permissions.ProjectAccess == nil {
return false
}

// Check for guest level permissions
if result.Permissions.ProjectAccess != nil && result.Permissions.ProjectAccess.AccessLevel == gitlabLib.GuestPermissions {
Kshitij-Katiyar marked this conversation as resolved.
Show resolved Hide resolved
return false
}
Kshitij-Katiyar marked this conversation as resolved.
Show resolved Hide resolved

return true
}

Expand Down
10 changes: 7 additions & 3 deletions server/webhook/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@ import (
"github.com/xanzy/go-gitlab"
)

func (w *webhook) HandleIssue(ctx context.Context, event *gitlab.IssueEvent) ([]*HandleWebhook, error) {
func (w *webhook) HandleIssue(ctx context.Context, event *gitlab.IssueEvent, eventType gitlab.EventType) ([]*HandleWebhook, error) {
handlers, err := w.handleDMIssue(event)
if err != nil {
return nil, err
}
handlers2, err := w.handleChannelIssue(ctx, event)
handlers2, err := w.handleChannelIssue(ctx, event, eventType)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -65,7 +65,7 @@ func (w *webhook) handleDMIssue(event *gitlab.IssueEvent) ([]*HandleWebhook, err
return []*HandleWebhook{}, nil
}

func (w *webhook) handleChannelIssue(ctx context.Context, event *gitlab.IssueEvent) ([]*HandleWebhook, error) {
func (w *webhook) handleChannelIssue(ctx context.Context, event *gitlab.IssueEvent, eventType gitlab.EventType) ([]*HandleWebhook, error) {
issue := event.ObjectAttributes
senderGitlabUsername := event.User.Username
repo := event.Project
Expand Down Expand Up @@ -98,6 +98,10 @@ func (w *webhook) handleChannelIssue(ctx context.Context, event *gitlab.IssueEve
continue
}

if eventType == gitlab.EventConfidentialIssue && !sub.ConfidentialIssues() {
continue
}

if sub.Label() != "" && !containsLabel(event.Labels, sub.Label()) {
continue
}
Expand Down
2 changes: 1 addition & 1 deletion server/webhook/issue_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ func TestIssueWebhook(t *testing.T) {
if err := json.Unmarshal([]byte(test.fixture), issueEvent); err != nil {
assert.Fail(t, "can't unmarshal fixture")
}
res, err := w.HandleIssue(context.Background(), issueEvent)
res, err := w.HandleIssue(context.Background(), issueEvent, gitlab.EventTypeIssue)
assert.Empty(t, err)
assert.Equal(t, len(test.res), len(res))
for index := range res {
Expand Down
2 changes: 1 addition & 1 deletion server/webhook/webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ type HandleWebhook struct {
}

type Webhook interface {
HandleIssue(ctx context.Context, event *gitlab.IssueEvent) ([]*HandleWebhook, error)
HandleIssue(ctx context.Context, event *gitlab.IssueEvent, eventType gitlab.EventType) ([]*HandleWebhook, error)
HandleMergeRequest(ctx context.Context, event *gitlab.MergeEvent) ([]*HandleWebhook, error)
HandleIssueComment(ctx context.Context, event *gitlab.IssueCommentEvent) ([]*HandleWebhook, error)
HandleMergeRequestComment(ctx context.Context, event *gitlab.MergeCommentEvent) ([]*HandleWebhook, error)
Expand Down
2 changes: 1 addition & 1 deletion server/webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import (

type fakeWebhookHandler struct{}

func (fakeWebhookHandler) HandleIssue(_ context.Context, _ *gitlabLib.IssueEvent) ([]*webhook.HandleWebhook, error) {
func (fakeWebhookHandler) HandleIssue(_ context.Context, _ *gitlabLib.IssueEvent, _ gitlabLib.EventType) ([]*webhook.HandleWebhook, error) {
return []*webhook.HandleWebhook{{
Message: "hello",
From: "test",
Expand Down