MM-69953 Fixing inconsistent behavior for issue webhook events

server/command.go

@@ -911,6 +911,16 @@ func (p *Plugin) subscriptionsAddCommand(ctx context.Context, info *gitlab.UserI
 			p.client.Log.Warn(msg)
 			return msg
 		}
+	} else if strings.Contains(features, "confidential_issues") {
+		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)

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,10 @@ 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
 }
 
 const (
@@ -132,6 +137,32 @@ 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.
+	},
 }
 
 func TestSubscribeCommand(t *testing.T) {
@@ -144,7 +175,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,19 +345,31 @@ 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)
+		if test.getProjectErr != nil {
+			mockedClient.EXPECT().GetProject(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, test.getProjectErr)
 		} else {
 			mockedClient.EXPECT().GetProject(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(&gitLabAPI.Project{
 				Permissions: &gitLabAPI.Permissions{
@@ -337,18 +380,18 @@ 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)
 			}
 		}
 	}
 
 	p.GitlabClient = mockedClient
 
 	conf := &model.Config{}
-	conf.ServiceSettings.SiteURL = &mattermostURL
+	conf.ServiceSettings.SiteURL = &test.mattermostURL
 
 	encryptedToken, _ := encrypt([]byte(testEncryptionKey), testGitlabToken)
 

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)

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)
+		})
+	}
+}

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) {

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() {

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
 			}