diff --git a/server/command.go b/server/command.go index 7cc16302..3a1e4195 100644 --- a/server/command.go +++ b/server/command.go @@ -904,13 +904,28 @@ func (p *Plugin) subscriptionsAddCommand(ctx context.Context, info *gitlab.UserI return err.Error() } + wantsConfidential := strings.Contains(features, "confidential_issues") + // Only check the permissions for a project if the project subscription is created (Not a group or a subgroup subscription) if project != "" { - if hasPermission := p.permissionToProject(ctx, info.UserID, namespace, project); !hasPermission { + if hasPermission := p.permissionToSubscribe(ctx, info.UserID, namespace, project, wantsConfidential); !hasPermission { msg := "You don't have the permissions to create subscriptions for this project." + if wantsConfidential { + msg = "You don't have the permissions to subscribe to confidential issues for this project." + } p.client.Log.Warn(msg) return msg } + } else if wantsConfidential { + groupAccessErr := p.useGitlabClient(info, func(info *gitlab.UserInfo, token *oauth2.Token) error { + _, groupErr := p.GitlabClient.GetGroup(ctx, info, token, namespace, "") + return groupErr + }) + if groupAccessErr != nil { + msg := "You don't have the permissions to subscribe to confidential issues for this group." + p.client.Log.Warn(msg, "err", groupAccessErr.Error()) + return msg + } } updatedSubscriptions, subscribeErr := p.Subscribe(info, namespace, project, channelID, features) diff --git a/server/command_test.go b/server/command_test.go index 4a79e6b7..57fd59f0 100644 --- a/server/command_test.go +++ b/server/command_test.go @@ -6,6 +6,7 @@ package main import ( "context" "encoding/json" + "strings" "testing" "github.com/mattermost/mattermost/server/public/model" @@ -32,6 +33,13 @@ type subscribeCommandTest struct { projectHookErr error getProjectErr error mockGitlab bool + // groupOnly resolves the namespace to a group rather than a project. + groupOnly bool + // getGroupErr is returned by GetGroup, simulating no access to the group. + getGroupErr error + // publicProjectNoMembership resolves to a public project with no membership, + // which is what GitLab reports for a non-member. + publicProjectNoMembership bool } const ( @@ -132,6 +140,50 @@ var subscribeCommandTests = []subscribeCommandTest{ parameters: []string{"unknown"}, want: invalidSubscribeSubCommand, }, + { + testName: "Group confidential_issues subscription denied without group access", + parameters: []string{"add", "group", "confidential_issues"}, + mockGitlab: true, + want: "You don't have the permissions to subscribe to confidential issues for this group.", + mattermostURL: "example.com", + groupOnly: true, + getGroupErr: errors.New("403 Forbidden"), + }, + { + testName: "Group confidential_issues subscription allowed with group access", + parameters: []string{"add", "group", "confidential_issues"}, + mockGitlab: true, + want: "Successfully subscribed to group.\nA Webhook is needed, run ```/gitlab webhook add group``` to create one now.", + mattermostURL: "example.com", + groupOnly: true, + }, + { + testName: "Group subscription without confidential_issues skips the group access check", + parameters: []string{"add", "group", "issues"}, + mockGitlab: true, + want: "Successfully subscribed to group.\nA Webhook is needed, run ```/gitlab webhook add group``` to create one now.", + mattermostURL: "example.com", + groupOnly: true, + // GetGroup is not mocked here, so gomock fails the test if it is called. + }, + { + testName: "Non-member can subscribe to a public project", + parameters: []string{"add", "group/project", "issues"}, + mockGitlab: true, + want: subscribeSuccessMessage, + webhookInfo: []*gitlab.WebhookInfo{{}}, + mattermostURL: "example.com", + publicProjectNoMembership: true, + }, + { + testName: "Non-member cannot subscribe to confidential issues on a public project", + parameters: []string{"add", "group/project", "confidential_issues"}, + mockGitlab: true, + want: "You don't have the permissions to subscribe to confidential issues for this project.", + mattermostURL: "example.com", + publicProjectNoMembership: true, + noAccess: true, + }, } func TestSubscribeCommand(t *testing.T) { @@ -144,7 +196,7 @@ func TestSubscribeCommand(t *testing.T) { UserID: "user_id", } - p := getTestPlugin(t, mockCtrl, test.webhookInfo, test.mattermostURL, test.projectHookErr, test.getProjectErr, test.mockGitlab, test.noAccess) + p := getTestPlugin(t, mockCtrl, test) subscribeMessage, _ := p.subscribeCommand(context.Background(), test.parameters, channelID, &configuration{}, userInfo) assert.Equal(t, test.want, subscribeMessage, "Subscribe command message should be the same.") @@ -314,20 +366,37 @@ func TestListWebhookCommandNamespaceNotAllowed(t *testing.T) { assert.Contains(t, got, "only repositories in the allowed-group namespace are allowed") } -func getTestPlugin(t *testing.T, mockCtrl *gomock.Controller, hooks []*gitlab.WebhookInfo, mattermostURL string, projectHookErr error, getProjectErr error, mockGitlab, noAccess bool) *Plugin { +func getTestPlugin(t *testing.T, mockCtrl *gomock.Controller, test subscribeCommandTest) *Plugin { p := new(Plugin) accessLevel := gitLabAPI.OwnerPermissions - if noAccess { + if test.noAccess { accessLevel = gitLabAPI.GuestPermissions } mockedClient := mocks.NewMockGitlab(mockCtrl) - if mockGitlab { + switch { + case !test.mockGitlab: + case test.groupOnly: + mockedClient.EXPECT().ResolveNamespaceAndProject(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return("group", "", nil) + if test.getGroupErr != nil { + mockedClient.EXPECT().GetGroup(gomock.Any(), gomock.Any(), gomock.Any(), "group", "").Return(nil, test.getGroupErr) + break + } + if strings.Contains(strings.Join(test.parameters, " "), "confidential_issues") { + mockedClient.EXPECT().GetGroup(gomock.Any(), gomock.Any(), gomock.Any(), "group", "").Return(&gitLabAPI.Group{}, nil) + } + mockedClient.EXPECT().GetGroupHooks(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(test.webhookInfo, test.projectHookErr) + default: mockedClient.EXPECT().ResolveNamespaceAndProject(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return("group", "project", nil) - if getProjectErr != nil { - mockedClient.EXPECT().GetProject(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, getProjectErr) - } else { + switch { + case test.getProjectErr != nil: + mockedClient.EXPECT().GetProject(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, test.getProjectErr) + case test.publicProjectNoMembership: + mockedClient.EXPECT().GetProject(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(&gitLabAPI.Project{ + Visibility: gitLabAPI.PublicVisibility, + }, nil) + default: mockedClient.EXPECT().GetProject(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(&gitLabAPI.Project{ Permissions: &gitLabAPI.Permissions{ ProjectAccess: &gitLabAPI.ProjectAccess{ @@ -337,10 +406,10 @@ func getTestPlugin(t *testing.T, mockCtrl *gomock.Controller, hooks []*gitlab.We }, 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) + if !test.noAccess { + mockedClient.EXPECT().GetProjectHooks(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(test.webhookInfo, test.projectHookErr) + if test.projectHookErr == nil { + mockedClient.EXPECT().GetGroupHooks(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(test.webhookInfo, test.projectHookErr) } } } @@ -348,7 +417,7 @@ func getTestPlugin(t *testing.T, mockCtrl *gomock.Controller, hooks []*gitlab.We p.GitlabClient = mockedClient conf := &model.Config{} - conf.ServiceSettings.SiteURL = &mattermostURL + conf.ServiceSettings.SiteURL = &test.mattermostURL encryptedToken, _ := encrypt([]byte(testEncryptionKey), testGitlabToken) diff --git a/server/subscriptions.go b/server/subscriptions.go index 4e7714c4..b542f0a7 100644 --- a/server/subscriptions.go +++ b/server/subscriptions.go @@ -118,6 +118,7 @@ func (p *Plugin) GetSubscribedChannelsForProject( namespace string, project string, isPublicVisibility bool, + isConfidential bool, ) []*subscription.Subscription { var subsForRepo []*subscription.Subscription @@ -145,7 +146,7 @@ func (p *Plugin) GetSubscribedChannelsForProject( subsToReturn := make([]*subscription.Subscription, 0, len(subsForRepo)) for _, sub := range subsForRepo { - if !isPublicVisibility && !p.permissionToProject(ctx, sub.CreatorID, namespace, project) { + if (!isPublicVisibility || isConfidential) && !p.permissionToProject(ctx, sub.CreatorID, namespace, project) { continue } subsToReturn = append(subsToReturn, sub) diff --git a/server/subscriptions_test.go b/server/subscriptions_test.go index f0c1fae1..129b9d1c 100644 --- a/server/subscriptions_test.go +++ b/server/subscriptions_test.go @@ -4,6 +4,7 @@ package main import ( + "context" "encoding/json" "testing" @@ -13,8 +14,11 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + gitLabAPI "github.com/xanzy/go-gitlab" + "go.uber.org/mock/gomock" "github.com/mattermost/mattermost-plugin-gitlab/server/gitlab" + mocks "github.com/mattermost/mattermost-plugin-gitlab/server/gitlab/mocks" "github.com/mattermost/mattermost-plugin-gitlab/server/subscription" ) @@ -236,3 +240,129 @@ func TestUnsubscribe(t *testing.T) { }) } } + +func TestGetSubscribedChannelsForProject(t *testing.T) { + t.Parallel() + + subscriptionsData := &Subscriptions{ + Repositories: map[string][]*subscription.Subscription{ + "group/project": { + {ChannelID: "channel1", CreatorID: "creator1", Features: "issues", Repository: "group/project"}, + }, + }, + } + subscriptionsJSON, err := json.Marshal(subscriptionsData) + require.NoError(t, err) + + userInfo := gitlab.UserInfo{ + UserID: "creator1", + GitlabUsername: "gitlab_user", + } + userInfoJSON, err := json.Marshal(userInfo) + require.NoError(t, err) + + encryptedToken, err := encrypt([]byte(testEncryptionKey), testGitlabToken) + require.NoError(t, err) + + testCases := []struct { + name string + namespace string + project string + isPublicVisibility bool + isConfidential bool + accessLevel gitLabAPI.AccessLevelValue + expectChannels []string + expectGetProject bool + }{ + { + name: "public non-confidential skips permission check", + namespace: "group", + project: "project", + isPublicVisibility: true, + isConfidential: false, + expectChannels: []string{"channel1"}, + expectGetProject: false, + }, + { + name: "public confidential enforces permission check with access", + namespace: "group", + project: "project", + isPublicVisibility: true, + isConfidential: true, + accessLevel: gitLabAPI.ReporterPermissions, + expectChannels: []string{"channel1"}, + expectGetProject: true, + }, + { + name: "public confidential excludes subscription without access", + namespace: "group", + project: "project", + isPublicVisibility: true, + isConfidential: true, + accessLevel: gitLabAPI.GuestPermissions, + expectChannels: []string{}, + expectGetProject: true, + }, + { + name: "private project always enforces permission check", + namespace: "group", + project: "project", + isPublicVisibility: false, + isConfidential: false, + accessLevel: gitLabAPI.ReporterPermissions, + expectChannels: []string{"channel1"}, + expectGetProject: true, + }, + } + + for _, test := range testCases { + t.Run(test.name, func(t *testing.T) { + mockCtrl := gomock.NewController(t) + + mockedClient := mocks.NewMockGitlab(mockCtrl) + if test.expectGetProject { + mockedClient.EXPECT().GetProject(gomock.Any(), gomock.Any(), gomock.Any(), test.namespace, test.project).Return(&gitLabAPI.Project{ + Permissions: &gitLabAPI.Permissions{ + ProjectAccess: &gitLabAPI.ProjectAccess{ + AccessLevel: test.accessLevel, + }, + }, + }, nil) + } + + api := &plugintest.API{} + api.On("KVGet", SubscriptionsKey).Return(subscriptionsJSON, nil).Once() + api.On("KVGet", "creator1"+GitlabUserInfoKey).Return(userInfoJSON, nil).Once() + api.On("KVGet", "creator1_usertoken").Return([]byte(encryptedToken), nil).Once() + api.On("LogWarn", + mock.AnythingOfType("string"), + mock.AnythingOfType("string"), + mock.AnythingOfType("string"), + mock.AnythingOfType("string"), + mock.AnythingOfType("string")) + + p := &Plugin{ + configuration: &configuration{ + EncryptionKey: testEncryptionKey, + }, + GitlabClient: mockedClient, + } + p.SetAPI(api) + p.client = pluginapi.NewClient(api, p.Driver) + + subs := p.GetSubscribedChannelsForProject( + context.Background(), + test.namespace, + test.project, + test.isPublicVisibility, + test.isConfidential, + ) + + channelIDs := make([]string, len(subs)) + for i, sub := range subs { + channelIDs[i] = sub.ChannelID + } + assert.ElementsMatch(t, test.expectChannels, channelIDs) + }) + } +} diff --git a/server/webhook.go b/server/webhook.go index c18e8f6f..92fff6a8 100644 --- a/server/webhook.go +++ b/server/webhook.go @@ -57,8 +57,9 @@ func (g *gitlabRetreiver) GetSubscribedChannelsForProject( namespace string, project string, isPublicVisibility bool, + isConfidential bool, ) []*subscription.Subscription { - return g.p.GetSubscribedChannelsForProject(ctx, namespace, project, isPublicVisibility) + return g.p.GetSubscribedChannelsForProject(ctx, namespace, project, isPublicVisibility, isConfidential) } func (p *Plugin) handleWebhook(w http.ResponseWriter, r *http.Request) { @@ -219,17 +220,47 @@ func (p *Plugin) sendRefreshIfNotAlreadySent(alreadySentRefresh map[string]bool, } func (p *Plugin) permissionToProject(ctx context.Context, userID, namespace, project string) bool { - if userID == "" { + result := p.fetchProjectAsUser(ctx, userID, namespace, project) + if result == nil { return false } - if err := p.isNamespaceAllowed(namespace); err != nil { + return effectiveAccess(result.Permissions) > gitlabLib.GuestPermissions +} + +// permissionToSubscribe reports whether the user may create a subscription for +// a project. Public projects only require membership when the subscription +// covers confidential content, mirroring the delivery-time check in +// GetSubscribedChannelsForProject so a subscription cannot be rejected here and +// then be considered deliverable later. +func (p *Plugin) permissionToSubscribe(ctx context.Context, userID, namespace, project string, wantsConfidential bool) bool { + result := p.fetchProjectAsUser(ctx, userID, namespace, project) + if result == nil { return false } + if result.Visibility == gitlabLib.PublicVisibility && !wantsConfidential { + return true + } + + return effectiveAccess(result.Permissions) > gitlabLib.GuestPermissions +} + +// fetchProjectAsUser loads a project using the given Mattermost user's GitLab +// token. It returns nil when the namespace is blocked, the user is unknown, or +// GitLab denies access. +func (p *Plugin) fetchProjectAsUser(ctx context.Context, userID, namespace, project string) *gitlabLib.Project { + if userID == "" { + return nil + } + + if err := p.isNamespaceAllowed(namespace); err != nil { + return nil + } + info, apiErr := p.getGitlabUserInfoByMattermostID(userID) if apiErr != nil { - return false + return nil } var result *gitlabLib.Project @@ -241,22 +272,31 @@ func (p *Plugin) permissionToProject(ctx context.Context, userID, namespace, pro result = resp return nil }) - if result == nil || err != nil { - if err != nil { - p.client.Log.Warn("Can't get project in webhook", "err", err.Error(), "project", namespace+"/"+project) - } - return false + if err != nil { + p.client.Log.Warn("Can't get project in webhook", "err", err.Error(), "project", namespace+"/"+project) + return nil } - // User permission for the project - userPermission := result.Permissions + return result +} - // Check if the user has guest permission or less for both project and group level - if (userPermission.ProjectAccess != nil && userPermission.ProjectAccess.AccessLevel <= gitlabLib.GuestPermissions) || (userPermission.GroupAccess != nil && userPermission.GroupAccess.AccessLevel <= gitlabLib.GuestPermissions) { - return false +// effectiveAccess returns the highest access level the user holds on a project, +// counting both direct project membership and access inherited from its group. +// Absent permissions mean no access, so callers deny rather than fall through. +func effectiveAccess(perms *gitlabLib.Permissions) gitlabLib.AccessLevelValue { + if perms == nil { + return gitlabLib.NoPermissions + } + + level := gitlabLib.NoPermissions + if perms.ProjectAccess != nil && perms.ProjectAccess.AccessLevel > level { + level = perms.ProjectAccess.AccessLevel + } + if perms.GroupAccess != nil && perms.GroupAccess.AccessLevel > level { + level = perms.GroupAccess.AccessLevel } - return true + return level } func (p *Plugin) createHook(ctx context.Context, gitlabClient gitlab.Gitlab, info *gitlab.UserInfo, group, project string, hookOptions *gitlab.AddWebhookOptions) (*gitlab.WebhookInfo, error) { diff --git a/server/webhook/constants.go b/server/webhook/constants.go index b20fcabc..2bbb234d 100644 --- a/server/webhook/constants.go +++ b/server/webhook/constants.go @@ -27,6 +27,11 @@ const ( statusUpdate = "update" statusDelete = "delete" + // eventTypeConfidentialNote is the payload's event_type value for internal + // notes. It differs from gitlab.EventConfidentialNote, which is the + // X-Gitlab-Event header value ("Confidential Note Hook"). + eventTypeConfidentialNote = "confidential_note" + PrivateVisibilityLevel = 0 PublicVisibilityLevel = 20 ) diff --git a/server/webhook/deployment.go b/server/webhook/deployment.go index df3dbe98..b5e73075 100644 --- a/server/webhook/deployment.go +++ b/server/webhook/deployment.go @@ -54,6 +54,7 @@ func (w *webhook) handleChannelDeployment(ctx context.Context, event *gitlab.Dep ctx, namespaceMetadata.Namespace, namespaceMetadata.Project, project.VisibilityLevel == PublicVisibilityLevel, + false, ) for _, sub := range subs { if !sub.Deployments() { diff --git a/server/webhook/issue.go b/server/webhook/issue.go index aa4d4479..16d64cbd 100644 --- a/server/webhook/issue.go +++ b/server/webhook/issue.go @@ -92,18 +92,23 @@ func (w *webhook) handleChannelIssue(ctx context.Context, event *gitlab.IssueEve } if len(message) > 0 { + // Trust the payload's confidential flag in addition to the event type, so a + // confidential issue delivered under the regular Issue Hook is still gated. + isConfidential := issue.Confidential || eventType == gitlab.EventConfidentialIssue + toChannels := make([]string, 0) namespace, project := normalizeNamespacedProject(repo.PathWithNamespace) subs := w.gitlabRetreiver.GetSubscribedChannelsForProject( ctx, namespace, project, repo.Visibility == gitlab.PublicVisibility, + isConfidential, ) for _, sub := range subs { if !sub.Issues() { continue } - if eventType == gitlab.EventConfidentialIssue && !sub.ConfidentialIssues() { + if isConfidential && !sub.ConfidentialIssues() { continue } diff --git a/server/webhook/issue_fixture_test.go b/server/webhook/issue_fixture_test.go index 57cfdc64..26df755b 100644 --- a/server/webhook/issue_fixture_test.go +++ b/server/webhook/issue_fixture_test.go @@ -125,6 +125,129 @@ const NewIssue = `{ }] }` +const NewConfidentialIssue = `{ + "object_kind":"issue", + "event_type":"confidential_issue", + "user":{ + "name":"Administrator", + "username":"root", + "avatar_url":"https://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80\\u0026d=identicon" + }, + "project":{ + "id":24, + "name":"webhook", + "description":"", + "web_url":"http://localhost:3000/manland/webhook", + "avatar_url":null, + "git_ssh_url":"ssh://rmaneschi@localhost:2222/manland/webhook.git", + "git_http_url":"http://localhost:3000/manland/webhook.git", + "namespace":"manland", + "visibility_level":20, + "visibility":"public", + "path_with_namespace":"manland/webhook", + "default_branch":"master", + "ci_config_path":null, + "homepage":"http://localhost:3000/manland/webhook", + "url":"ssh://rmaneschi@localhost:2222/manland/webhook.git", + "ssh_url":"ssh://rmaneschi@localhost:2222/manland/webhook.git", + "http_url":"http://localhost:3000/manland/webhook.git" + }, + "object_attributes":{ + "author_id":1, + "closed_at":null, + "confidential":true, + "created_at":"2019-04-06 21:03:04 UTC", + "description":"confidential details", + "due_date":null, + "id":181, + "iid":1, + "last_edited_at":null, + "last_edited_by_id":null, + "milestone_id":null, + "moved_to_id":null, + "project_id":24, + "relative_position":1073742323, + "state":"opened", + "time_estimate":0, + "title":"confidential issue", + "updated_at":"2019-04-06 21:03:04 UTC", + "updated_by_id":null, + "url":"http://localhost:3000/manland/webhook/issues/1", + "total_time_spent":0, + "human_total_time_spent":null, + "human_time_estimate":null, + "assignee_ids":[50], + "assignee_id":50, + "action":"open" + }, + "labels":[], + "changes":{ + "author_id":{ + "previous":null, + "current":1 + }, + "created_at":{ + "previous":null, + "current":"2019-04-06 21:03:04 UTC" + }, + "description":{ + "previous":null, + "current":"confidential details" + }, + "id":{ + "previous":null, + "current":181 + }, + "iid":{ + "previous":null, + "current":1 + }, + "project_id":{ + "previous":null, + "current":24 + }, + "relative_position":{ + "previous":null, + "current":1073742323 + }, + "state":{ + "previous":null, + "current":"opened" + }, + "title":{ + "previous":null, + "current":"confidential issue" + }, + "updated_at":{ + "previous":null, + "current":"2019-04-06 21:03:04 UTC" + }, + "assignees":{ + "previous":[], + "current":[{ + "name":"manland", + "username":"manland", + "avatar_url":"https://www.gravatar.com/avatar/c6b552a4cd47f7cf1701ea5b650cd2e3?s=80\\u0026d=identicon" + }] + }, + "total_time_spent":{ + "previous":null, + "current":0 + } + }, + "repository":{ + "name":"webhook", + "url":"ssh://rmaneschi@localhost:2222/manland/webhook.git", + "description":"", + "homepage":"http://localhost:3000/manland/webhook" + }, + "assignees":[{ + "name":"manland", + "username":"manland", + "avatar_url":"https://www.gravatar.com/avatar/c6b552a4cd47f7cf1701ea5b650cd2e3?s=80\\u0026d=identicon" + }] + }` + const NewIssueUnassigned = `{ "object_kind":"issue", "event_type":"issue", diff --git a/server/webhook/issue_test.go b/server/webhook/issue_test.go index 794f6166..95d0579c 100644 --- a/server/webhook/issue_test.go +++ b/server/webhook/issue_test.go @@ -10,6 +10,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/xanzy/go-gitlab" "github.com/mattermost/mattermost-plugin-gitlab/server/subscription" @@ -154,3 +155,121 @@ func TestIssueWebhook(t *testing.T) { }) } } + +func TestConfidentialIssueWebhook(t *testing.T) { + t.Parallel() + testCases := []testDataIssueStr{ + { + testTitle: "confidential issue on public project with confidential_issues subscription", + fixture: NewConfidentialIssue, + gitlabRetreiver: newFakeWebhook([]*subscription.Subscription{ + {ChannelID: "channel1", CreatorID: "1", Features: "issues,confidential_issues", Repository: "manland/webhook"}, + }), + res: []*HandleWebhook{{ + Message: "[root](http://my.gitlab.com/root) assigned you to issue [manland/webhook#1](http://localhost:3000/manland/webhook/issues/1)", + ToUsers: []string{"manland"}, + ToChannels: []string{}, + From: "root", + }, { + Message: "#### confidential issue\n##### [manland/webhook#1](http://localhost:3000/manland/webhook/issues/1)\n###### new issue by [root](http://my.gitlab.com/root) on [2019-04-06 21:03:04 UTC](http://localhost:3000/manland/webhook/issues/1)\n\nconfidential details", + ToUsers: []string{}, + ToChannels: []string{"channel1"}, + From: "root", + }}, + warnings: []string{}, + }, + { + testTitle: "confidential issue without confidential_issues feature does not notify channel", + fixture: NewConfidentialIssue, + gitlabRetreiver: newFakeWebhook([]*subscription.Subscription{ + {ChannelID: "channel1", CreatorID: "1", Features: "issues", Repository: "manland/webhook"}, + }), + res: []*HandleWebhook{{ + Message: "[root](http://my.gitlab.com/root) assigned you to issue [manland/webhook#1](http://localhost:3000/manland/webhook/issues/1)", + ToUsers: []string{"manland"}, + ToChannels: []string{}, + From: "root", + }}, + warnings: []string{}, + }, + } + + for _, test := range testCases { + t.Run(test.testTitle, func(t *testing.T) { + w := NewWebhook(test.gitlabRetreiver) + issueEvent := &gitlab.IssueEvent{} + if err := json.Unmarshal([]byte(test.fixture), issueEvent); err != nil { + assert.Fail(t, "can't unmarshal fixture") + } + res, warnings, err := w.HandleIssue(context.Background(), issueEvent, gitlab.EventConfidentialIssue) + assert.Empty(t, err) + assert.Equal(t, len(test.res), len(res)) + assert.ElementsMatch(t, test.warnings, warnings) + for index := range res { + assert.Equal(t, test.res[index].Message, res[index].Message) + assert.EqualValues(t, test.res[index].ToUsers, res[index].ToUsers) + assert.ElementsMatch(t, test.res[index].ToChannels, res[index].ToChannels) + assert.Equal(t, test.res[index].From, res[index].From) + } + + assert.True(t, test.gitlabRetreiver.gotIsConfidential, + "confidential issues must be looked up with the confidential flag so the permission check is enforced") + }) + } +} + +func TestIssueWebhookPassesConfidentialFlag(t *testing.T) { + t.Parallel() + testCases := []struct { + testTitle string + fixture string + expectedIsConf bool + }{ + { + testTitle: "non-confidential issue does not force the permission check", + fixture: NewIssue, + expectedIsConf: false, + }, + { + testTitle: "confidential issue forces the permission check", + fixture: NewConfidentialIssue, + expectedIsConf: true, + }, + } + + for _, test := range testCases { + t.Run(test.testTitle, func(t *testing.T) { + retreiver := newFakeWebhook([]*subscription.Subscription{ + {ChannelID: "channel1", CreatorID: "1", Features: "issues,confidential_issues", Repository: "manland/webhook"}, + }) + w := NewWebhook(retreiver) + issueEvent := &gitlab.IssueEvent{} + require.NoError(t, json.Unmarshal([]byte(test.fixture), issueEvent)) + + _, _, err := w.HandleIssue(context.Background(), issueEvent, gitlab.EventTypeIssue) + require.NoError(t, err) + + assert.Equal(t, test.expectedIsConf, retreiver.gotIsConfidential) + }) + } +} + +// A confidential issue delivered under the regular Issue Hook must still be +// gated on the confidential_issues feature, not just on the event type. +func TestConfidentialIssueUnderRegularEventTypeIsGated(t *testing.T) { + t.Parallel() + retreiver := newFakeWebhook([]*subscription.Subscription{ + {ChannelID: "channel1", CreatorID: "1", Features: "issues", Repository: "manland/webhook"}, + }) + w := NewWebhook(retreiver) + issueEvent := &gitlab.IssueEvent{} + require.NoError(t, json.Unmarshal([]byte(NewConfidentialIssue), issueEvent)) + + res, _, err := w.HandleIssue(context.Background(), issueEvent, gitlab.EventTypeIssue) + require.NoError(t, err) + + for _, handler := range res { + assert.Empty(t, handler.ToChannels, + "confidential issue must not reach a channel lacking the confidential_issues feature") + } +} diff --git a/server/webhook/jobs.go b/server/webhook/jobs.go index 6baf5170..d36d6903 100644 --- a/server/webhook/jobs.go +++ b/server/webhook/jobs.go @@ -53,6 +53,7 @@ func (w *webhook) handleChannelJob(ctx context.Context, event *gitlab.JobEvent) subs := w.gitlabRetreiver.GetSubscribedChannelsForProject( ctx, namespaceMetadata.Namespace, namespaceMetadata.Project, repo.Visibility == gitlab.PublicVisibility, + false, ) for _, sub := range subs { if !sub.Jobs() { diff --git a/server/webhook/merge_request.go b/server/webhook/merge_request.go index 10a563b4..edd201f4 100644 --- a/server/webhook/merge_request.go +++ b/server/webhook/merge_request.go @@ -192,6 +192,7 @@ func (w *webhook) handleChannelMergeRequest(ctx context.Context, event *gitlab.M subs := w.gitlabRetreiver.GetSubscribedChannelsForProject( ctx, namespace, project, repo.Visibility == gitlab.PublicVisibility, + false, ) if len(message) > 0 { diff --git a/server/webhook/note.go b/server/webhook/note.go index f7817943..bb949c66 100644 --- a/server/webhook/note.go +++ b/server/webhook/note.go @@ -62,11 +62,16 @@ func (w *webhook) handleChannelIssueComment(ctx context.Context, event *gitlab.I message := fmt.Sprintf("[%s](%s) New comment by [%s](%s) on [#%v %s](%s):\n\n%s", repo.PathWithNamespace, repo.WebURL, senderGitlabUsername, w.gitlabRetreiver.GetUserURL(senderGitlabUsername), event.Issue.IID, event.Issue.Title, event.ObjectAttributes.URL, body) + // An internal note carries a confidential event type even when the issue + // itself is public, so both signals gate delivery. + isConfidential := event.Issue.Confidential || event.EventType == eventTypeConfidentialNote + toChannels := make([]string, 0) namespace, project := normalizeNamespacedProject(repo.PathWithNamespace) subs := w.gitlabRetreiver.GetSubscribedChannelsForProject( ctx, namespace, project, repo.Visibility == gitlab.PublicVisibility, + isConfidential, ) var warnings []string for _, sub := range subs { @@ -74,6 +79,10 @@ func (w *webhook) handleChannelIssueComment(ctx context.Context, event *gitlab.I continue } + if isConfidential && !sub.ConfidentialIssues() { + continue + } + ok, warning := anyEventLabelInSubs(sub, event.Issue.Labels) if !ok { if len(warning) > 0 { @@ -137,18 +146,28 @@ func (w *webhook) handleChannelMergeRequestComment(ctx context.Context, event *g res := []*HandleWebhook{} message := fmt.Sprintf("[%s](%s) New comment by [%s](%s) on [#%v %s](%s):\n\n%s", repo.PathWithNamespace, repo.WebURL, senderGitlabUsername, w.gitlabRetreiver.GetUserURL(senderGitlabUsername), event.MergeRequest.IID, event.MergeRequest.Title, event.ObjectAttributes.URL, body) + + // Merge requests are never confidential themselves, so the event type is the + // only signal that a note is internal. + isConfidential := event.EventType == eventTypeConfidentialNote + var warnings []string toChannels := make([]string, 0) namespace, project := normalizeNamespacedProject(repo.PathWithNamespace) subs := w.gitlabRetreiver.GetSubscribedChannelsForProject( ctx, namespace, project, repo.Visibility == gitlab.PublicVisibility, + isConfidential, ) for _, sub := range subs { if !sub.MergeRequestComments() { continue } + if isConfidential && !sub.ConfidentialIssues() { + continue + } + ok, warning := anyEventLabelInSubs(sub, event.MergeRequest.Labels) if !ok { if len(warning) > 0 { diff --git a/server/webhook/note_test.go b/server/webhook/note_test.go index bde21ed0..54ab133f 100644 --- a/server/webhook/note_test.go +++ b/server/webhook/note_test.go @@ -12,6 +12,7 @@ import ( "github.com/mattermost/mattermost-plugin-gitlab/server/subscription" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/xanzy/go-gitlab" ) @@ -145,3 +146,166 @@ func TestNoteWebhook(t *testing.T) { }) } } + +// internalNoteOnPublicIssue is an internal (confidential) note on an issue that +// is not itself confidential, so only the event type marks it as private. +var internalNoteOnPublicIssue = strings.ReplaceAll(IssueComment, `"event_type":"note"`, `"event_type":"confidential_note"`) + +func TestIssueCommentWebhookPassesConfidentialFlag(t *testing.T) { + t.Parallel() + testCases := []struct { + testTitle string + fixture string + expectedIsConf bool + }{ + { + testTitle: "comment on regular issue does not force the permission check", + fixture: IssueComment, + expectedIsConf: false, + }, + { + testTitle: "comment on confidential issue forces the permission check", + fixture: strings.ReplaceAll(IssueComment, `"confidential":false`, `"confidential":true`), + expectedIsConf: true, + }, + { + testTitle: "internal note on a public issue forces the permission check", + fixture: internalNoteOnPublicIssue, + expectedIsConf: true, + }, + } + + for _, test := range testCases { + t.Run(test.testTitle, func(t *testing.T) { + retreiver := newFakeWebhook([]*subscription.Subscription{ + {ChannelID: "channel1", CreatorID: "1", Features: "issue_comments", Repository: "manland/webhook"}, + }) + w := NewWebhook(retreiver) + issueCommentEvent := &gitlab.IssueCommentEvent{} + require.NoError(t, json.Unmarshal([]byte(test.fixture), issueCommentEvent)) + + _, _, err := w.HandleIssueComment(context.Background(), issueCommentEvent) + require.NoError(t, err) + + assert.Equal(t, test.expectedIsConf, retreiver.gotIsConfidential) + }) + } +} + +// Comments on a confidential issue expose the issue title and comment body, so +// they must require the confidential_issues opt-in just like the issues themselves. +func TestConfidentialIssueCommentRequiresOptIn(t *testing.T) { + t.Parallel() + confidentialFixture := strings.ReplaceAll(IssueComment, `"confidential":false`, `"confidential":true`) + + testCases := []struct { + testTitle string + fixture string + features string + expectedToChannels []string + }{ + { + testTitle: "subscription without confidential_issues is skipped", + fixture: confidentialFixture, + features: "issue_comments", + expectedToChannels: nil, + }, + { + testTitle: "subscription with confidential_issues receives the comment", + fixture: confidentialFixture, + features: "issue_comments,confidential_issues", + expectedToChannels: []string{"channel1"}, + }, + { + testTitle: "internal note on a public issue is skipped without confidential_issues", + fixture: internalNoteOnPublicIssue, + features: "issue_comments", + expectedToChannels: nil, + }, + { + testTitle: "internal note on a public issue is delivered with confidential_issues", + fixture: internalNoteOnPublicIssue, + features: "issue_comments,confidential_issues", + expectedToChannels: []string{"channel1"}, + }, + } + + for _, test := range testCases { + t.Run(test.testTitle, func(t *testing.T) { + retreiver := newFakeWebhook([]*subscription.Subscription{ + {ChannelID: "channel1", CreatorID: "1", Features: test.features, Repository: "manland/webhook"}, + }) + w := NewWebhook(retreiver) + issueCommentEvent := &gitlab.IssueCommentEvent{} + require.NoError(t, json.Unmarshal([]byte(test.fixture), issueCommentEvent)) + + res, _, err := w.HandleIssueComment(context.Background(), issueCommentEvent) + require.NoError(t, err) + + var gotChannels []string + for _, handler := range res { + gotChannels = append(gotChannels, handler.ToChannels...) + } + assert.ElementsMatch(t, test.expectedToChannels, gotChannels) + }) + } +} + +// Internal notes on a merge request expose private review discussion, so they +// need the same confidential_issues opt-in as comments on confidential issues. +func TestInternalMergeRequestCommentRequiresOptIn(t *testing.T) { + t.Parallel() + internalNote := strings.ReplaceAll(MergeRequestComment, `"event_type":"note"`, `"event_type":"confidential_note"`) + + testCases := []struct { + testTitle string + fixture string + features string + expectedIsConf bool + expectedToChannels []string + }{ + { + testTitle: "regular comment does not force the permission check", + fixture: MergeRequestComment, + features: "merge_request_comments", + expectedIsConf: false, + expectedToChannels: []string{"channel1"}, + }, + { + testTitle: "internal note is skipped without confidential_issues", + fixture: internalNote, + features: "merge_request_comments", + expectedIsConf: true, + expectedToChannels: nil, + }, + { + testTitle: "internal note is delivered with confidential_issues", + fixture: internalNote, + features: "merge_request_comments,confidential_issues", + expectedIsConf: true, + expectedToChannels: []string{"channel1"}, + }, + } + + for _, test := range testCases { + t.Run(test.testTitle, func(t *testing.T) { + retreiver := newFakeWebhook([]*subscription.Subscription{ + {ChannelID: "channel1", CreatorID: "1", Features: test.features, Repository: "manland/webhook"}, + }) + w := NewWebhook(retreiver) + mergeCommentEvent := &gitlab.MergeCommentEvent{} + require.NoError(t, json.Unmarshal([]byte(test.fixture), mergeCommentEvent)) + + res, _, err := w.HandleMergeRequestComment(context.Background(), mergeCommentEvent) + require.NoError(t, err) + + assert.Equal(t, test.expectedIsConf, retreiver.gotIsConfidential) + + var gotChannels []string + for _, handler := range res { + gotChannels = append(gotChannels, handler.ToChannels...) + } + assert.ElementsMatch(t, test.expectedToChannels, gotChannels) + }) + } +} diff --git a/server/webhook/pipeline.go b/server/webhook/pipeline.go index 72bb129d..a31adc68 100644 --- a/server/webhook/pipeline.go +++ b/server/webhook/pipeline.go @@ -73,6 +73,7 @@ func (w *webhook) handleChannelPipeline(ctx context.Context, event *gitlab.Pipel subs := w.gitlabRetreiver.GetSubscribedChannelsForProject( ctx, namespace, project, repo.Visibility == gitlab.PublicVisibility, + false, ) for _, sub := range subs { if !sub.Pipeline() { diff --git a/server/webhook/push.go b/server/webhook/push.go index 3ac8a59e..2f1e9e56 100644 --- a/server/webhook/push.go +++ b/server/webhook/push.go @@ -70,6 +70,7 @@ func (w *webhook) handleChannelPush(ctx context.Context, event *gitlab.PushEvent subs := w.gitlabRetreiver.GetSubscribedChannelsForProject( ctx, namespace, project, repo.Visibility == gitlab.PublicVisibility, + false, ) for _, sub := range subs { if !sub.Pushes() { diff --git a/server/webhook/release.go b/server/webhook/release.go index 4e3206f9..7459d040 100644 --- a/server/webhook/release.go +++ b/server/webhook/release.go @@ -52,6 +52,7 @@ func (w *webhook) handleChannelRelease(ctx context.Context, event *gitlab.Releas ctx, namespaceMetadata.Namespace, namespaceMetadata.Project, project.VisibilityLevel == PublicVisibilityLevel, + false, ) for _, sub := range subs { if !sub.Releases() { diff --git a/server/webhook/tag.go b/server/webhook/tag.go index 6c3b05ce..b38ee7f4 100644 --- a/server/webhook/tag.go +++ b/server/webhook/tag.go @@ -67,6 +67,7 @@ func (w *webhook) handleChannelTag(ctx context.Context, event *gitlab.TagEvent) subs := w.gitlabRetreiver.GetSubscribedChannelsForProject( ctx, namespace, project, repo.Visibility == gitlab.PublicVisibility, + false, ) for _, sub := range subs { if !sub.Tag() { diff --git a/server/webhook/webhook.go b/server/webhook/webhook.go index de386ac5..3ff41155 100644 --- a/server/webhook/webhook.go +++ b/server/webhook/webhook.go @@ -29,7 +29,7 @@ type GitlabRetreiver interface { // ParseGitlabUsernamesFromText from a text return an array of username ParseGitlabUsernamesFromText(text string) []string // GetSubscribedChannelsForProject returns all subscriptions for given project. - GetSubscribedChannelsForProject(ctx context.Context, namespace, project string, isPublicVisibility bool) []*subscription.Subscription + GetSubscribedChannelsForProject(ctx context.Context, namespace, project string, isPublicVisibility, isConfidential bool) []*subscription.Subscription } type HandleWebhook struct { diff --git a/server/webhook/webhook_test.go b/server/webhook/webhook_test.go index d74d73ff..ed127724 100644 --- a/server/webhook/webhook_test.go +++ b/server/webhook/webhook_test.go @@ -15,6 +15,10 @@ import ( type fakeWebhook struct { subs []*subscription.Subscription + + // gotIsConfidential records the confidentiality flag of the last lookup so + // tests can assert that confidential events reach the authorization check. + gotIsConfidential bool } func newFakeWebhook(subs []*subscription.Subscription) *fakeWebhook { @@ -52,7 +56,8 @@ func (*fakeWebhook) ParseGitlabUsernamesFromText(body string) []string { return []string{} } -func (f *fakeWebhook) GetSubscribedChannelsForProject(ctx context.Context, namespace, project string, isPublicVisibility bool) []*subscription.Subscription { +func (f *fakeWebhook) GetSubscribedChannelsForProject(ctx context.Context, namespace, project string, isPublicVisibility, isConfidential bool) []*subscription.Subscription { + f.gotIsConfidential = isConfidential return f.subs } diff --git a/server/webhook_test.go b/server/webhook_test.go index a695d56a..04e78746 100644 --- a/server/webhook_test.go +++ b/server/webhook_test.go @@ -69,6 +69,81 @@ func (fakeWebhookHandler) HandleRelease(_ context.Context, _ *gitlabLib.ReleaseE return nil, nil } +func TestEffectiveAccess(t *testing.T) { + tests := []struct { + name string + perms *gitlabLib.Permissions + expectedLevel gitlabLib.AccessLevelValue + expectedAllowsMR bool + }{ + { + name: "nil permissions", + perms: nil, + }, + { + name: "no project or group membership", + perms: &gitlabLib.Permissions{}, + }, + { + name: "guest on project only", + perms: &gitlabLib.Permissions{ + ProjectAccess: &gitlabLib.ProjectAccess{AccessLevel: gitlabLib.GuestPermissions}, + }, + expectedLevel: gitlabLib.GuestPermissions, + }, + { + name: "reporter on project only", + perms: &gitlabLib.Permissions{ + ProjectAccess: &gitlabLib.ProjectAccess{AccessLevel: gitlabLib.ReporterPermissions}, + }, + expectedLevel: gitlabLib.ReporterPermissions, + expectedAllowsMR: true, + }, + { + name: "reporter inherited from group only", + perms: &gitlabLib.Permissions{ + GroupAccess: &gitlabLib.GroupAccess{AccessLevel: gitlabLib.ReporterPermissions}, + }, + expectedLevel: gitlabLib.ReporterPermissions, + expectedAllowsMR: true, + }, + { + name: "group access outranks guest project access", + perms: &gitlabLib.Permissions{ + ProjectAccess: &gitlabLib.ProjectAccess{AccessLevel: gitlabLib.GuestPermissions}, + GroupAccess: &gitlabLib.GroupAccess{AccessLevel: gitlabLib.MaintainerPermissions}, + }, + expectedLevel: gitlabLib.MaintainerPermissions, + expectedAllowsMR: true, + }, + { + name: "project access outranks guest group access", + perms: &gitlabLib.Permissions{ + ProjectAccess: &gitlabLib.ProjectAccess{AccessLevel: gitlabLib.OwnerPermissions}, + GroupAccess: &gitlabLib.GroupAccess{AccessLevel: gitlabLib.GuestPermissions}, + }, + expectedLevel: gitlabLib.OwnerPermissions, + expectedAllowsMR: true, + }, + { + name: "guest at both levels", + perms: &gitlabLib.Permissions{ + ProjectAccess: &gitlabLib.ProjectAccess{AccessLevel: gitlabLib.GuestPermissions}, + GroupAccess: &gitlabLib.GroupAccess{AccessLevel: gitlabLib.GuestPermissions}, + }, + expectedLevel: gitlabLib.GuestPermissions, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + level := effectiveAccess(test.perms) + assert.Equal(t, test.expectedLevel, level) + assert.Equal(t, test.expectedAllowsMR, level > gitlabLib.GuestPermissions) + }) + } +} + func TestHandleWebhookBadSecret(t *testing.T) { p := &Plugin{configuration: &configuration{WebhookSecret: "secret"}} req := httptest.NewRequest("POST", "http://example.com/foo", bytes.NewBufferString(""))