Skip to content

MM-67398 Added checks for instance and webhooks commands #635

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

Open
avasconcelos114 wants to merge 3 commits into
base: master
Choose a base branch
from
+179 −16
Open

MM-67398 Added checks for instance and webhooks commands #635

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aa699c5
Added sysadmin permission checks for instance and webhooks commands
avasconcelos114 Feb 2, 2026
8be51f6
Added test coverage for commands
avasconcelos114 Feb 2, 2026
68cc02f
PR Feedback
avasconcelos114 Feb 3, 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
24 changes: 23 additions & 1 deletion server/command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,18 @@ func (p *Plugin) handleConfigError(args *model.CommandArgs, err error) (*model.C
}

func (p *Plugin) handleInstance(args *model.CommandArgs, parameters []string) (*model.CommandResponse, *model.AppError) {
userID := args.UserId
isSysAdmin, sysErr := p.isAuthorizedSysAdmin(userID)
if sysErr != nil {
p.client.Log.Warn("Failed to check if user is System Admin", "error", sysErr.Error())
p.postCommandResponse(args, "Error checking user's permissions", true)
return &model.CommandResponse{}, nil
}

if !isSysAdmin {
p.postCommandResponse(args, "Only System Admins are allowed to manage instances.", true)
return &model.CommandResponse{}, nil
Copy link

@nevyangelova nevyangelova Feb 3, 2026

Choose a reason for hiding this comment

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

Nit: Use return p.getCommandResponse(args, ...) here to match the pattern in handleWebhookHandler

}
if len(parameters) < 1 {
return p.getCommandResponse(args, "Please specify the instance command.", true), nil
}
Expand Down Expand Up @@ -272,7 +284,6 @@ func (p *Plugin) handleUnInstallInstance(args *model.CommandArgs, parameters []s
if len(parameters) < 1 {
return p.getCommandResponse(args, "Please specify the instance name.", true), nil
}

instanceName := strings.TrimSpace(strings.Join(parameters, " "))

err := p.uninstallInstance(instanceName)
Expand Down Expand Up @@ -524,6 +535,15 @@ 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 @@ -1067,6 +1087,7 @@ func (p *Plugin) getAutocompleteData(config *configuration) *model.AutocompleteD
gitlab.AddCommand(disconnect)

instance := model.NewAutocompleteData("instance", "[command]", "Install, Uninstall, List, Set-Default Instance")
instance.RoleID = model.SystemAdminRoleId

install := model.NewAutocompleteData("install", "", "Install GitLab Instance")
instance.AddCommand(install)
Expand Down Expand Up @@ -1140,6 +1161,7 @@ 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
186 changes: 186 additions & 0 deletions server/command_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -550,6 +550,192 @@ func setupInstanceCommandTest(t *testing.T, instanceList []string, instanceConfi
return p, &capturedMessage, api
}

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("install instance 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"}

_, _ = p.handleInstallInstance(args, []string{})

assert.Contains(t, capturedMessage, "Only System Admins are allowed to set up the plugin.")
Copy link

@nevyangelova nevyangelova Feb 3, 2026

Choose a reason for hiding this comment

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

this calls handleInstallInstance directly but the new permission check is in handleInstance. Is the existing check in handleInstallInstance now redundant?

Copy link
Contributor Author

@avasconcelos114 avasconcelos114 Feb 3, 2026

Choose a reason for hiding this comment

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

Great point, this PR moved the sysadmin check higher up the chain which renders the additional checks that exist solely in handleInstallInstance unnecessary, I'll remove that and update tests to reflect this

})

t.Run("instance commands require admin", func(t *testing.T) {
testCases := []struct {
name string
parameters []string
}{
{"list", []string{"list"}},
{"install", []string{"install"}},
{"uninstall", []string{"uninstall", "test-instance"}},
{"set-default", []string{"set-default", "test-instance"}},
}

for _, tc := range testCases {
t.Run(tc.name, 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"}

_, _ = p.handleInstance(args, tc.parameters)

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

t.Run("instance commands allowed for admin", func(t *testing.T) {
p := new(Plugin)
p.configuration = &configuration{EncryptionKey: testEncryptionKey}

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

instanceList := []string{"test-instance"}
instanceConfig := map[string]InstanceConfiguration{"test-instance": {GitlabURL: "https://gitlab.example.com"}}
instanceListJSON, _ := json.Marshal(instanceList)
instanceConfigJSON, _ := json.Marshal(instanceConfig)

api := &plugintest.API{}
api.On("GetUser", "admin_id").Return(adminUser, nil)
api.On("KVGet", instanceConfigNameListKey).Return(instanceListJSON, nil)
api.On("KVGet", instanceConfigMapKey).Return(instanceConfigJSON, 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"}

_, _ = p.handleInstance(args, []string{"list"})

// Should show instance list, not an error
assert.Contains(t, capturedMessage, "test-instance")
assert.NotContains(t, capturedMessage, "Only System Admins")
})
}

func TestInstanceCommands(t *testing.T) {
args := &model.CommandArgs{UserId: "user_id", ChannelId: "channel_id"}

Expand Down
Loading