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 @@ -24,6 +24,7 @@
* |/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 @@ -574,6 +575,7 @@
if err != nil {
return err.Error()
}

if len(subs) == 0 {
txt = "Currently there are no subscriptions in this channel"
} else {
Expand Down Expand Up @@ -610,6 +612,12 @@
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."
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 @@ -783,7 +791,7 @@

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]+)*/`)

Check warning on line 794 in server/command.go

View check run for this annotation

Codecov / codecov/patch

server/command.go#L794

Added line #L794 was not covered by tests
subscriptions.AddCommand(subscriptionsAdd)

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

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

"github.com/golang/mock/gomock"
Expand All @@ -11,6 +12,7 @@ import (
"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,7 +25,9 @@ type subscribeCommandTest struct {
want string
webhookInfo []*gitlab.WebhookInfo
mattermostURL string
noAccess bool
projectHookErr error
getProjectErr error
mockGitlab bool
}

Expand All @@ -37,6 +41,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 @@ -78,9 +101,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 @@ -224,15 +249,34 @@ 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(), gomock.Any()).Return("group", "project", nil)
mockedClient.EXPECT().GetProjectHooks(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(hooks, projectHookErr)
if projectHookErr == nil {
mockedClient.EXPECT().GetGroupHooks(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(hooks, projectHookErr)
if getProjectErr != nil {
mockedClient.EXPECT().GetProject(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, getProjectErr)
} else {
mockedClient.EXPECT().GetProject(gomock.Any(), 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(), gomock.Any()).Return(hooks, projectHookErr)
if projectHookErr == nil {
mockedClient.EXPECT().GetGroupHooks(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(hooks, projectHookErr)
}
}
}

Expand All @@ -247,15 +291,32 @@ func getTestPlugin(t *testing.T, mockCtrl *gomock.Controller, hooks []*gitlab.We
EncryptionKey: testEncryptionKey,
}

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

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

var subVal []byte

api := &plugintest.API{}
api.On("GetConfig", mock.Anything).Return(conf)
api.On("KVGet", "_usertoken").Return([]byte(encryptedToken), nil)
api.On("KVGet", mock.Anything).Return(subVal, nil)
api.On("KVGet", "user_id_usertoken").Return([]byte(encryptedToken), nil)
api.On("KVGet", "user_id_userinfo").Return(subVal, nil).Once()
api.On("KVGet", "user_id_gitlabtoken").Return(jsonInfo, nil).Once()
api.On("KVGet", "subscriptions").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)
api.On("LogWarn",
mock.AnythingOfTypeArgument("string"),
mock.AnythingOfTypeArgument("string"),
mock.AnythingOfTypeArgument("string"),
mock.AnythingOfTypeArgument("string"),
mock.AnythingOfTypeArgument("string"))

p.SetAPI(api)
p.client = pluginapi.NewClient(api, p.Driver)
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 @@
"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 @@
return strings.Contains(s.Features, "issues")
}

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

Check warning on line 68 in server/subscription/subscription.go

View check run for this annotation

Codecov / codecov/patch

server/subscription/subscription.go#L67-L68

Added lines #L67 - L68 were not covered by tests
}

func (s *Subscription) Pushes() bool {
return strings.Contains(s.Features, "pushes")
}
Expand Down
13 changes: 10 additions & 3 deletions server/webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,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 @@ -99,7 +100,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 @@ -224,10 +225,16 @@ func (p *Plugin) permissionToProject(ctx context.Context, userID, namespace, pro
})
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
}

// Check for guest level permissions
if result.Permissions.ProjectAccess == nil || result.Permissions.ProjectAccess.AccessLevel == gitlabLib.GuestPermissions {
return false
}

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 @@
"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 @@
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 @@
continue
}

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

Check warning on line 102 in server/webhook/issue.go

View check run for this annotation

Codecov / codecov/patch

server/webhook/issue.go#L102

Added line #L102 was not covered by tests
}

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
8 changes: 4 additions & 4 deletions webapp/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading