Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 7 additions & 5 deletions server/flow.go
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,9 @@ func (fm *FlowManager) submitGitlabURL(f *flow.Flow, submitted map[string]any) (
return "", nil, errorList, nil
}

fm.gitlabURL = gitlabURL
if err := fm.setGitlabURL(gitlabURL); err != nil {

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.

submitDelegateSelection doesn't check the delegate is an admin so a non admin can reach this step. That was fine when it only set the in memory URL but now it writes the plugin setting and rebuilds p.GitlabClient so they could point the whole integration at their own host.

stepSetDefaultInstance does an isAuthorizedSysAdmin(f.UserID) check for the same reason. Can we add that here and on the gitlab.com button above?

return "", nil, nil, errors.Wrap(err, "failed to save GitLab URL")
}

return "", flow.State{
keyGitlabURL: gitlabURL,
Expand Down Expand Up @@ -804,10 +806,8 @@ func (fm *FlowManager) submitChannelAnnouncement(f *flow.Flow, submitted map[str
}

func (fm *FlowManager) setGitlabURL(gitlabURL string) error {
fm.gitlabURL = gitlabURL

// will need to get gitlab url from plugin config
config := fm.getConfiguration()
config := fm.getConfiguration().Clone()
config.GitlabURL = gitlabURL
Comment thread
coderabbitai[bot] marked this conversation as resolved.

configMap, err := config.ToMap()
if err != nil {
Expand All @@ -819,6 +819,8 @@ func (fm *FlowManager) setGitlabURL(gitlabURL string) error {
return errors.Wrap(err, "failed to save plugin config")
}

fm.gitlabURL = gitlabURL

return nil
}

Expand Down
66 changes: 66 additions & 0 deletions server/flow_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
// Copyright (c) 2019-present Mattermost, Inc. All Rights Reserved.
// See LICENSE.txt for license information.

package main

import (
"net/http"
"testing"

"github.com/mattermost/mattermost/server/public/model"
"github.com/mattermost/mattermost/server/public/plugin/plugintest"
"github.com/mattermost/mattermost/server/public/pluginapi"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
)

func TestSetGitlabURL(t *testing.T) {

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.

Both tests cover setGitlabURL, which already saved the config. Nothing covers submitGitlabURL, which is what actually changed. f is unused in the body so you can pass nil.

config := &configuration{
GitlabURL: "https://gitlab.com",
}

var savedConfig map[string]any
api := &plugintest.API{}
api.On("SavePluginConfig", mock.Anything).Run(func(args mock.Arguments) {
savedConfig = args.Get(0).(map[string]any)
}).Return(nil).Once()

fm := &FlowManager{
client: pluginapi.NewClient(api, nil),
getConfiguration: func() *configuration { return config },
}

err := fm.setGitlabURL("https://git.example.com")
require.NoError(t, err)

assert.Equal(t, "https://git.example.com", fm.gitlabURL)

require.NotNil(t, savedConfig)
assert.Equal(t, "https://git.example.com", savedConfig["gitlaburl"])

assert.Equal(t, "https://gitlab.com", config.GitlabURL)

api.AssertExpectations(t)
}

func TestSetGitlabURLSaveFailure(t *testing.T) {
config := &configuration{
GitlabURL: "https://gitlab.com",
}

api := &plugintest.API{}
api.On("SavePluginConfig", mock.Anything).Return(model.NewAppError("SavePluginConfig", "app.plugin.config.app_error", nil, "", http.StatusInternalServerError)).Once()

fm := &FlowManager{
client: pluginapi.NewClient(api, nil),
getConfiguration: func() *configuration { return config },
}

err := fm.setGitlabURL("https://git.example.com")
require.Error(t, err)

assert.Empty(t, fm.gitlabURL)

api.AssertExpectations(t)
}