Skip to content

MM-69161 Adding permission check model to allow/deny requests #678

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.

Sign up for GitHub

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

Merged
avasconcelos114 merged 5 commits into from
Jun 16, 2026
Merged

MM-69161 Adding permission check model to allow/deny requests #678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
24a987e
MM-69161 Adding permission check model to allow/deny requests
avasconcelos114 Jun 11, 2026
4e081a1
Revert "MM-69161 Adding permission check model to allow/deny requests"
avasconcelos114 Jun 15, 2026
d2c40ad
Adding audit logging to webhook command
avasconcelos114 Jun 15, 2026
fc8d93f
Allowing webhook autocomplete to be visible to standard users
avasconcelos114 Jun 15, 2026
a33ba22
Added more human-readable error responses to webhook cmd
avasconcelos114 Jun 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions server/audit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,32 @@ func (p AttachCommentToIssueAuditResult) Auditable() map[string]any {
return map[string]any{"note_id": p.NoteID}
}

// AddWebhookAuditParams holds request audit data for the webhook add command.
type AddWebhookAuditParams struct {
MattermostUserID string `json:"mattermost_user_id"`
GitlabUsername string `json:"gitlab_username"`
Namespace string `json:"namespace"`
URL string `json:"url"`
}

func (p AddWebhookAuditParams) Auditable() map[string]any {
return map[string]any{
"mattermost_user_id": p.MattermostUserID, "gitlab_username": p.GitlabUsername,
"namespace": p.Namespace, "url": p.URL,
}
}

// AddWebhookAuditResult holds GitLab response for the webhook add command (for AddEventResultState).
type AddWebhookAuditResult struct {
HookID int `json:"hook_id"`
URL string `json:"url"`
Scope string `json:"scope"`
}

func (p AddWebhookAuditResult) Auditable() map[string]any {
return map[string]any{"hook_id": p.HookID, "url": p.URL, "scope": p.Scope}
}

// ReEncryptUserDataAuditParams holds request audit data for the reEncryptUserData operation.
type ReEncryptUserDataAuditParams struct {
TotalUsers int `json:"total_users"`
Expand Down
66 changes: 52 additions & 14 deletions server/command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,8 @@ const (
)

const (
groupNotFoundError = "404 {message: 404 Group Not Found}"
groupNotFoundMessage = "Unable to find GitLab group: "

projectNotFoundError = "404 {message: 404 Project Not Found}"
projectNotFoundMessage = "Unable to find project with namespace: "

invalidSubscribeSubCommand = "Invalid subscribe command. Available commands are add, delete, and list"
Expand Down Expand Up @@ -513,15 +511,6 @@ func (p *Plugin) handleSettings(ctx context.Context, args *model.CommandArgs, pa
}

func (p *Plugin) handleWebhookHandler(ctx context.Context, args *model.CommandArgs, parameters []string, info *gitlab.UserInfo) (*model.CommandResponse, *model.AppError) {
isSysAdmin, err := p.isAuthorizedSysAdmin(args.UserId)
if err != nil {
p.client.Log.Warn("Failed to check if user is System Admin", "error", err.Error())
return p.getCommandResponse(args, "Error checking user's permissions", true), nil
}
if !isSysAdmin {
return p.getCommandResponse(args, "Only System Admins are allowed to manage webhooks.", true), nil
}

config := p.getConfiguration()
message := p.webhookCommand(ctx, parameters, info, config.EnablePrivateRepo)
return p.getCommandResponse(args, message, true), nil
Expand Down Expand Up @@ -549,6 +538,16 @@ func (p *Plugin) handleIssueHelper(_ *plugin.Context, args *model.CommandArgs, p
}
}

// webhookPermissionMessage builds a user-friendly message for GitLab permission
// failures when managing webhooks, naming the scope and the access required.
func webhookPermissionMessage(group, project string) string {
scope := "group"
if project != "" {
scope = "repository"
}
return fmt.Sprintf("You don't have permission to manage webhooks for the %s `%s`. You need Maintainer or Owner access in GitLab.", scope, namespaceFromGroupAndProject(group, project))
}

// webhookCommand processes the /gitlab webhook commands
func (p *Plugin) webhookCommand(ctx context.Context, parameters []string, info *gitlab.UserInfo, enablePrivateRepo bool) string {
if len(parameters) < 1 {
Expand Down Expand Up @@ -577,6 +576,10 @@ func (p *Plugin) webhookCommand(ctx context.Context, parameters []string, info *
return err.Error()
}

if err := p.isNamespaceAllowed(namespaceFromGroupAndProject(group, project)); err != nil {
return err.Error()
}

var webhookInfo []*gitlab.WebhookInfo
if project != "" {
err := p.useGitlabClient(info, func(info *gitlab.UserInfo, token *oauth2.Token) error {
Expand All @@ -588,7 +591,10 @@ func (p *Plugin) webhookCommand(ctx context.Context, parameters []string, info *
return nil
})
if err != nil {
if strings.Contains(err.Error(), projectNotFoundError) {
if errors.Is(err, gitlab.ErrForbidden) {
return webhookPermissionMessage(group, project)
}
if errors.Is(err, gitlab.ErrNotFound) {
return projectNotFoundMessage + namespace
}
return err.Error()
Expand All @@ -603,7 +609,10 @@ func (p *Plugin) webhookCommand(ctx context.Context, parameters []string, info *
return nil
})
if err != nil {
if strings.Contains(err.Error(), groupNotFoundError) {
if errors.Is(err, gitlab.ErrForbidden) {
return webhookPermissionMessage(group, project)
}
if errors.Is(err, gitlab.ErrNotFound) {
return groupNotFoundMessage + group
}
return err.Error()
Expand Down Expand Up @@ -662,10 +671,40 @@ func (p *Plugin) webhookCommand(ctx context.Context, parameters []string, info *
return namespaceErr.Error()
}

resolvedNamespace := namespaceFromGroupAndProject(group, project)

auditRec := plugin.MakeAuditRecord("addWebhook", model.AuditStatusFail)
defer p.API.LogAuditRec(auditRec)
auditRec.Actor.UserId = info.UserID

auditParams := AddWebhookAuditParams{
MattermostUserID: info.UserID,
GitlabUsername: info.GitlabUsername,
Namespace: resolvedNamespace,
URL: hookOptions.URL,
}
model.AddEventParameterAuditableToAuditRec(auditRec, "add_webhook", auditParams)

if err := p.isNamespaceAllowed(resolvedNamespace); err != nil {

@edgarbellot edgarbellot Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for this!

auditRec.AddErrorDesc(err.Error())
return err.Error()
}

newWebhook, err := p.createHook(ctx, p.GitlabClient, info, group, project, hookOptions)
if err != nil {
auditRec.AddErrorDesc(err.Error())
if errors.Is(err, gitlab.ErrForbidden) {
return webhookPermissionMessage(group, project)
}
return err.Error()
}

auditRec.Success()
auditRec.AddEventResultState(AddWebhookAuditResult{
HookID: newWebhook.ID,
URL: newWebhook.URL,
Scope: newWebhook.Scope.String(),
})
return fmt.Sprintf("Webhook Created:\n%s", newWebhook.String())

default:
Expand Down Expand Up @@ -1137,7 +1176,6 @@ func (p *Plugin) getAutocompleteData(config *configuration) *model.AutocompleteD
gitlab.AddCommand(settings)

webhook := model.NewAutocompleteData("webhook", "[command]", "Available Commands: list, add")
webhook.RoleID = model.SystemAdminRoleId
webhookList := model.NewAutocompleteData(commandList, "owner/[repo]", "List existing project or group webhooks")
webhookList.AddTextArgument("Project path: includes user or group name with optional slash project name", "owner[/repo]", "")
webhook.AddCommand(webhookList)
Expand Down
184 changes: 110 additions & 74 deletions server/command_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,31 @@ func TestListWebhookCommand(t *testing.T) {
}
}

func TestListWebhookCommandNamespaceNotAllowed(t *testing.T) {
p := new(Plugin)

mockCtrl := gomock.NewController(t)
mockedClient := mocks.NewMockGitlab(mockCtrl)
mockedClient.EXPECT().ResolveNamespaceAndProject(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), true).Return("other-group", "project", nil)
p.GitlabClient = mockedClient

encryptedToken, _ := encrypt([]byte(testEncryptionKey), testGitlabToken)

p.configuration = &configuration{
EncryptionKey: testEncryptionKey,
GitlabGroup: "allowed-group",
}

api := &plugintest.API{}
api.On("KVGet", "_usertoken").Return([]byte(encryptedToken), nil)
p.SetAPI(api)
p.client = pluginapi.NewClient(api, p.Driver)

got := p.webhookCommand(context.Background(), []string{"list", "other-group/project"}, &gitlab.UserInfo{}, true)

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 {
p := new(Plugin)

Expand Down Expand Up @@ -496,6 +521,7 @@ func TestAddWebhookCommand(t *testing.T) {
api := &plugintest.API{}
api.On("GetConfig", mock.Anything).Return(conf)
api.On("KVGet", "_usertoken").Return([]byte(encryptedToken), nil)
api.On("LogAuditRec", mock.Anything).Maybe()
p.SetAPI(api)
p.client = pluginapi.NewClient(api, p.Driver)

Expand All @@ -506,6 +532,90 @@ func TestAddWebhookCommand(t *testing.T) {
}
}

func TestAddWebhookCommandNamespaceNotAllowed(t *testing.T) {
p := new(Plugin)

mockCtrl := gomock.NewController(t)
mockedClient := mocks.NewMockGitlab(mockCtrl)
mockedClient.EXPECT().ResolveNamespaceAndProject(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), true).Return("other-group", "project", nil)
p.GitlabClient = mockedClient

conf := &model.Config{}
conf.ServiceSettings.SiteURL = model.NewPointer("https://example.com")

encryptedToken, _ := encrypt([]byte(testEncryptionKey), testGitlabToken)

p.configuration = &configuration{
EncryptionKey: testEncryptionKey,
GitlabGroup: "allowed-group",
}

api := &plugintest.API{}
api.On("GetConfig", mock.Anything).Return(conf)
api.On("KVGet", "_usertoken").Return([]byte(encryptedToken), nil)
api.On("LogAuditRec", mock.Anything).Maybe()
p.SetAPI(api)
p.client = pluginapi.NewClient(api, p.Driver)

got := p.webhookCommand(context.Background(), []string{"add", "other-group/project"}, &gitlab.UserInfo{}, true)

assert.Contains(t, got, "only repositories in the allowed-group namespace are allowed")
}

func TestAddWebhookCommandForbidden(t *testing.T) {
p := new(Plugin)

mockCtrl := gomock.NewController(t)
mockedClient := mocks.NewMockGitlab(mockCtrl)
mockedClient.EXPECT().ResolveNamespaceAndProject(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), true).Return("group", "project", nil)
mockedClient.EXPECT().GetProject(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(&gitLabAPI.Project{ID: 4}, nil)
mockedClient.EXPECT().NewProjectHook(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, gitlab.ErrForbidden)
p.GitlabClient = mockedClient

conf := &model.Config{}
conf.ServiceSettings.SiteURL = model.NewPointer("https://example.com")

encryptedToken, _ := encrypt([]byte(testEncryptionKey), testGitlabToken)

p.configuration = &configuration{EncryptionKey: testEncryptionKey}

api := &plugintest.API{}
api.On("GetConfig", mock.Anything).Return(conf)
api.On("KVGet", "_usertoken").Return([]byte(encryptedToken), nil)
api.On("LogAuditRec", mock.Anything).Maybe()
p.SetAPI(api)
p.client = pluginapi.NewClient(api, p.Driver)

got := p.webhookCommand(context.Background(), []string{"add", "group/project"}, &gitlab.UserInfo{}, true)

assert.Contains(t, got, "You don't have permission to manage webhooks for the repository `group/project`")
assert.Contains(t, got, "Maintainer or Owner access")
}

func TestListWebhookCommandForbidden(t *testing.T) {
p := new(Plugin)

mockCtrl := gomock.NewController(t)
mockedClient := mocks.NewMockGitlab(mockCtrl)
mockedClient.EXPECT().ResolveNamespaceAndProject(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), true).Return("group", "", nil)
mockedClient.EXPECT().GetGroupHooks(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, gitlab.ErrForbidden)
p.GitlabClient = mockedClient

encryptedToken, _ := encrypt([]byte(testEncryptionKey), testGitlabToken)

p.configuration = &configuration{EncryptionKey: testEncryptionKey}

api := &plugintest.API{}
api.On("KVGet", "_usertoken").Return([]byte(encryptedToken), nil)
p.SetAPI(api)
p.client = pluginapi.NewClient(api, p.Driver)

got := p.webhookCommand(context.Background(), []string{"list", "group"}, &gitlab.UserInfo{}, true)

assert.Contains(t, got, "You don't have permission to manage webhooks for the group `group`")
assert.Contains(t, got, "Maintainer or Owner access")
}

type instanceNameTestCase struct {
name string
parameters []string
Expand Down Expand Up @@ -554,80 +664,6 @@ func setupInstanceCommandTest(t *testing.T, instanceList []string, instanceConfi
}

func TestAdminProtectedCommands(t *testing.T) {
t.Run("webhook command requires admin", func(t *testing.T) {
p := new(Plugin)
p.configuration = &configuration{EncryptionKey: testEncryptionKey}

// Mock a non-admin user
nonAdminUser := &model.User{
Id: "user_id",
Roles: "system_user",
}

api := &plugintest.API{}
api.On("GetUser", "user_id").Return(nonAdminUser, nil)

var capturedMessage string
api.On("SendEphemeralPost", mock.Anything, mock.MatchedBy(func(post *model.Post) bool {
capturedMessage = post.Message
return true
})).Return(&model.Post{})

p.SetAPI(api)
p.client = pluginapi.NewClient(api, p.Driver)

args := &model.CommandArgs{UserId: "user_id", ChannelId: "channel_id"}
userInfo := &gitlab.UserInfo{UserID: "user_id"}

_, _ = p.handleWebhookHandler(context.Background(), args, []string{"list", "group/project"}, userInfo)

assert.Contains(t, capturedMessage, "Only System Admins are allowed to manage webhooks.")
})

t.Run("webhook command allowed for admin", func(t *testing.T) {
p := new(Plugin)

mockCtrl := gomock.NewController(t)
mockedClient := mocks.NewMockGitlab(mockCtrl)

// Mock admin user
adminUser := &model.User{
Id: "admin_id",
Roles: "system_admin system_user",
}

encryptedToken, _ := encrypt([]byte(testEncryptionKey), testGitlabToken)

p.configuration = &configuration{
EncryptionKey: testEncryptionKey,
EnablePrivateRepo: true,
}

mockedClient.EXPECT().ResolveNamespaceAndProject(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), true).Return("group", "project", nil)
mockedClient.EXPECT().GetProjectHooks(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return([]*gitlab.WebhookInfo{}, nil)
p.GitlabClient = mockedClient

api := &plugintest.API{}
api.On("GetUser", "admin_id").Return(adminUser, nil)
api.On("KVGet", "admin_id_usertoken").Return([]byte(encryptedToken), nil)

var capturedMessage string
api.On("SendEphemeralPost", mock.Anything, mock.MatchedBy(func(post *model.Post) bool {
capturedMessage = post.Message
return true
})).Return(&model.Post{})

p.SetAPI(api)
p.client = pluginapi.NewClient(api, p.Driver)

args := &model.CommandArgs{UserId: "admin_id", ChannelId: "channel_id"}
userInfo := &gitlab.UserInfo{UserID: "admin_id"}

_, _ = p.handleWebhookHandler(context.Background(), args, []string{"list", "group/project"}, userInfo)

assert.Contains(t, capturedMessage, "No webhooks found")
})

t.Run("instance commands require admin", func(t *testing.T) {
testCases := []struct {
name string
Expand Down
3 changes: 3 additions & 0 deletions server/gitlab/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,9 @@ func (g *gitlab) GetProjectHooks(ctx context.Context, user *UserInfo, token *oau
projectPath := fmt.Sprintf("%s/%s", owner, repo)
for {
hooks, resp, err = client.Projects.ListProjectHooks(projectPath, opt)
if respErr := checkResponse(resp); respErr != nil {
return nil, respErr
}
if err != nil {
return nil, err
}
Expand Down
7 changes: 7 additions & 0 deletions server/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,13 @@ func fullPathFromNamespaceAndProject(namespace, project string) string {
return fmt.Sprintf("%s/%s", namespace, project)
}

func namespaceFromGroupAndProject(group, project string) string {
if project == "" {
return group
}
return fullPathFromNamespaceAndProject(group, project)
}

// isValidURL checks if a given URL is a valid URL with a host and a http or http scheme.
func isValidURL(rawURL string) error {
u, err := url.ParseRequestURI(rawURL)
Expand Down
Loading