From a10f0ee00d127abe14b3b829dea828b4f005575f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Rozlach?= Date: Mon, 23 Mar 2020 18:20:17 +0100 Subject: [PATCH] [GH-84] Add the concept of plugin admins (#93) * Add the concept of plugin admins * Review fixes #1 Co-Authored-By: Jason Frerich * Review fixes #2 Co-authored-by: Jason Frerich --- plugin.json | 6 + server/autolink/autolink_test.go | 2 +- server/autolinkplugin/command.go | 12 +- server/autolinkplugin/config.go | 58 +++++-- server/autolinkplugin/plugin.go | 25 +++- server/autolinkplugin/plugin_test.go | 216 ++++++++++++++++++++++++++- server/main.go | 2 +- 7 files changed, 295 insertions(+), 26 deletions(-) diff --git a/plugin.json b/plugin.json index a9e5163f..3a44fb28 100644 --- a/plugin.json +++ b/plugin.json @@ -27,6 +27,12 @@ "display_name": "Apply plugin to updated posts as well as new posts", "type": "bool", "default": false + }, + { + "key": "PluginAdmins", + "display_name": "Admin User IDs", + "type": "text", + "help_text": "Comma-separated list of userIDs authorized to administer the plugin in addition to the System Admins.\n \nUser IDs can be found by navigating to **System Console** > **Users**." } ] } diff --git a/server/autolink/autolink_test.go b/server/autolink/autolink_test.go index 8459c402..35e6a578 100644 --- a/server/autolink/autolink_test.go +++ b/server/autolink/autolink_test.go @@ -15,7 +15,7 @@ import ( ) func setupTestPlugin(t *testing.T, l autolink.Autolink) *autolinkplugin.Plugin { - p := &autolinkplugin.Plugin{} + p := autolinkplugin.New() api := &plugintest.API{} api.On("GetChannel", mock.AnythingOfType("string")).Run(func(args mock.Arguments) { diff --git a/server/autolinkplugin/command.go b/server/autolinkplugin/command.go index 386df5cc..8861c14d 100644 --- a/server/autolinkplugin/command.go +++ b/server/autolinkplugin/command.go @@ -66,17 +66,19 @@ func (ch CommandHandler) Handle(p *Plugin, c *plugin.Context, header *model.Comm } func (p *Plugin) ExecuteCommand(c *plugin.Context, commandArgs *model.CommandArgs) (*model.CommandResponse, *model.AppError) { - user, appErr := p.API.GetUser(commandArgs.UserId) - if appErr != nil { - return responsef("%v", appErr.Error()), nil + isAdmin, err := p.IsAuthorizedAdmin(commandArgs.UserId) + if err != nil { + return responsef("error occured while authorizing the command: %v", err), nil } - if !strings.Contains(user.Roles, "system_admin") { - return responsef("`/autolink` can only be executed by a system administrator."), nil + if !isAdmin { + return responsef("`/autolink` commands can only be executed by a system administrator or `autolink` plugin admins."), nil } + args := strings.Fields(commandArgs.Command) if len(args) == 0 || args[0] != "/autolink" { return responsef(helpText), nil } + return autolinkCommandHandler.Handle(p, c, commandArgs, args[1:]...), nil } diff --git a/server/autolinkplugin/config.go b/server/autolinkplugin/config.go index c548cea7..5353c456 100644 --- a/server/autolinkplugin/config.go +++ b/server/autolinkplugin/config.go @@ -14,24 +14,34 @@ import ( type Config struct { EnableAdminCommand bool EnableOnUpdate bool + PluginAdmins string Links []autolink.Autolink + + // AdminUserIds is a set of UserIds that are permitted to perform + // administrative operations on the plugin configuration (i.e. plugin + // admins). On each configuration change the contents of PluginAdmins + // config field is parsed into this field. + AdminUserIds map[string]struct{} } // OnConfigurationChange is invoked when configuration changes may have been made. func (p *Plugin) OnConfigurationChange() error { var c Config - err := p.API.LoadPluginConfiguration(&c) - if err != nil { - return err + if err := p.API.LoadPluginConfiguration(&c); err != nil { + return fmt.Errorf("failed to load configuration: %w", err) } for i := range c.Links { - err = c.Links[i].Compile() - if err != nil { + if err := c.Links[i].Compile(); err != nil { mlog.Error(fmt.Sprintf("Error creating autolinker: %+v: %v", c.Links[i], err)) } } + // Plugin admin UserId parsing and validation errors are + // not fatal, if everything fails only sysadmin will be able to manage the + // config which is still OK + c.parsePluginAdminList(p) + p.UpdateConfig(func(conf *Config) { *conf = c }) @@ -54,15 +64,17 @@ func (p *Plugin) OnConfigurationChange() error { return nil } -func (p *Plugin) getConfig() Config { +func (p *Plugin) getConfig() *Config { p.confLock.RLock() defer p.confLock.RUnlock() + return p.conf } func (p *Plugin) GetLinks() []autolink.Autolink { p.confLock.RLock() defer p.confLock.RUnlock() + return p.conf.Links } @@ -78,17 +90,16 @@ func (p *Plugin) SaveLinks(links []autolink.Autolink) error { return nil } -func (p *Plugin) UpdateConfig(f func(conf *Config)) Config { +func (p *Plugin) UpdateConfig(f func(conf *Config)) { p.confLock.Lock() defer p.confLock.Unlock() - f(&p.conf) - return p.conf + f(p.conf) } // ToConfig marshals Config into a tree of map[string]interface{} to pass down // to p.API.SavePluginConfig, otherwise RPC/gob barfs at the unknown type. -func (conf Config) ToConfig() map[string]interface{} { +func (conf *Config) ToConfig() map[string]interface{} { links := []interface{}{} for _, l := range conf.Links { links = append(links, l.ToConfig()) @@ -96,12 +107,13 @@ func (conf Config) ToConfig() map[string]interface{} { return map[string]interface{}{ "EnableAdminCommand": conf.EnableAdminCommand, "EnableOnUpdate": conf.EnableOnUpdate, + "PluginAdmins": conf.PluginAdmins, "Links": links, } } // Sorted returns a clone of the Config, with links sorted alphabetically -func (conf Config) Sorted() Config { +func (conf *Config) Sorted() *Config { sorted := conf sorted.Links = append([]autolink.Autolink{}, conf.Links...) sort.Slice(conf.Links, func(i, j int) bool { @@ -109,3 +121,27 @@ func (conf Config) Sorted() Config { }) return conf } + +// parsePluginAdminList parses the contents of PluginAdmins config field +func (conf *Config) parsePluginAdminList(p *Plugin) { + conf.AdminUserIds = make(map[string]struct{}) + + if len(conf.PluginAdmins) == 0 { + // There were no plugin admin users defined + return + } + + userIDs := strings.Split(conf.PluginAdmins, ",") + + for _, v := range userIDs { + userId := strings.TrimSpace(v) + // Let's verify that the given user really exists + _, appErr := p.API.GetUser(userId) + if appErr != nil { + mlog.Error(fmt.Sprintf( + "error occured while verifying userId %s: %v", v, appErr)) + } else { + conf.AdminUserIds[userId] = struct{}{} + } + } +} diff --git a/server/autolinkplugin/plugin.go b/server/autolinkplugin/plugin.go index 63d93463..a602c224 100644 --- a/server/autolinkplugin/plugin.go +++ b/server/autolinkplugin/plugin.go @@ -21,24 +21,41 @@ type Plugin struct { handler *api.Handler // configuration and a muttex to control concurrent access - conf Config + conf *Config confLock sync.RWMutex } +func New() *Plugin { + return &Plugin{ + conf: new(Config), + } +} + func (p *Plugin) OnActivate() error { p.handler = api.NewHandler(p, p) return nil } -func (p *Plugin) IsAuthorizedAdmin(mattermostID string) (bool, error) { - user, err := p.API.GetUser(mattermostID) +func (p *Plugin) IsAuthorizedAdmin(userId string) (bool, error) { + user, err := p.API.GetUser(userId) if err != nil { - return false, err + return false, fmt.Errorf( + "failed to obtain information about user `%s`: %w", userId, err) } if strings.Contains(user.Roles, "system_admin") { + mlog.Info( + fmt.Sprintf("UserId `%s` is authorized basing on the sysadmin role membership", userId)) + return true, nil + } + + conf := p.getConfig() + if _, ok := conf.AdminUserIds[userId]; ok { + mlog.Info( + fmt.Sprintf("UserId `%s` is authorized basing on the list of plugin admins list", userId)) return true, nil } + return false, nil } diff --git a/server/autolinkplugin/plugin_test.go b/server/autolinkplugin/plugin_test.go index 4c925d9f..0408706d 100644 --- a/server/autolinkplugin/plugin_test.go +++ b/server/autolinkplugin/plugin_test.go @@ -3,6 +3,7 @@ package autolinkplugin import ( "bytes" "encoding/json" + "fmt" "net/http" "net/http/httptest" "testing" @@ -14,6 +15,7 @@ import ( "github.com/mattermost/mattermost-server/v5/plugin/plugintest/mock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/stretchr/testify/suite" ) func TestPlugin(t *testing.T) { @@ -45,7 +47,7 @@ func TestPlugin(t *testing.T) { api.On("GetChannel", mock.AnythingOfType("string")).Return(&testChannel, nil) api.On("GetTeam", mock.AnythingOfType("string")).Return(&testTeam, nil) - p := Plugin{} + p := New() p.SetAPI(api) p.OnConfigurationChange() @@ -55,6 +57,212 @@ func TestPlugin(t *testing.T) { assert.Equal(t, "Welcome to [Mattermost](https://mattermost.com)!", rpost.Message) } +type SuiteAuthorization struct { + suite.Suite + + api *plugintest.API + + adminUsernames string + userInfo map[string]*model.User +} + +func (suite *SuiteAuthorization) SetupTest() { + suite.adminUsernames = "" + suite.userInfo = make(map[string]*model.User) + + suite.api = &plugintest.API{} + suite.api.On( + "LoadPluginConfiguration", + mock.AnythingOfType("*autolinkplugin.Config"), + ).Return( + func(dest interface{}) error { + *dest.(*Config) = Config{ + PluginAdmins: suite.adminUsernames, + } + return nil + }, + ) + suite.api.On( + "GetUser", + mock.AnythingOfType("string"), + ).Return( + func(userId string) *model.User { + return suite.userInfo[userId] + }, + func(userId string) *model.AppError { + if _, ok := suite.userInfo[userId]; ok { + return nil + } else { + return &model.AppError{ + Message: fmt.Sprintf("user %s not found", userId), + } + } + }, + ) + suite.api.On( + "UnregisterCommand", + mock.AnythingOfType("string"), + mock.AnythingOfType("string"), + ).Return( + (*model.AppError)(nil), + ) +} + +func (suite *SuiteAuthorization) TestSysadminIsAuthorized() { + suite.userInfo["marynaId"] = &model.User{ + Username: "maryna", + Id: "marynaId", + Roles: "smurf,system_admin,reaper", + } + + p := New() + p.SetAPI(suite.api) + + err := p.OnConfigurationChange() + require.NoError(suite.T(), err) + + allowed, err := p.IsAuthorizedAdmin("marynaId") + require.NoError(suite.T(), err) + assert.True(suite.T(), allowed) +} + +func (suite *SuiteAuthorization) TestPlainUserIsDenied() { + suite.userInfo["marynaId"] = &model.User{ + Username: "maryna", + Id: "marynaId", + Roles: "smurf,reaper", + } + + p := New() + p.SetAPI(suite.api) + + err := p.OnConfigurationChange() + require.NoError(suite.T(), err) + + allowed, err := p.IsAuthorizedAdmin("marynaId") + require.NoError(suite.T(), err) + assert.False(suite.T(), allowed) +} + +func (suite *SuiteAuthorization) TestAdminUserIsAuthorized() { + suite.userInfo["marynaId"] = &model.User{ + Username: "maryna", + Id: "marynaId", + Roles: "smurf,reaper", + } + suite.adminUsernames = "marynaId" + + p := New() + p.SetAPI(suite.api) + + err := p.OnConfigurationChange() + require.NoError(suite.T(), err) + + allowed, err := p.IsAuthorizedAdmin("marynaId") + require.NoError(suite.T(), err) + assert.True(suite.T(), allowed) +} + +func (suite *SuiteAuthorization) TestMultipleUsersAreAuthorized() { + suite.userInfo["marynaId"] = &model.User{ + Username: "maryna", + Id: "marynaId", + Roles: "smurf,reaper", + } + suite.userInfo["borynaId"] = &model.User{ + Username: "boryna", + Id: "borynaId", + Roles: "smurf", + } + suite.userInfo["karynaId"] = &model.User{ + Username: "karyna", + Id: "karynaId", + Roles: "screamer", + } + suite.adminUsernames = "marynaId,karynaId" + + p := New() + p.SetAPI(suite.api) + + err := p.OnConfigurationChange() + require.NoError(suite.T(), err) + + allowed, err := p.IsAuthorizedAdmin("marynaId") + require.NoError(suite.T(), err) + assert.True(suite.T(), allowed) + + allowed, err = p.IsAuthorizedAdmin("karynaId") + require.NoError(suite.T(), err) + assert.True(suite.T(), allowed) + + allowed, err = p.IsAuthorizedAdmin("borynaId") + require.NoError(suite.T(), err) + assert.False(suite.T(), allowed) +} + +func (suite *SuiteAuthorization) TestWhitespaceIsIgnored() { + suite.userInfo["marynaId"] = &model.User{ + Username: "maryna", + Id: "marynaId", + Roles: "smurf,reaper", + } + suite.userInfo["borynaId"] = &model.User{ + Username: "boryna", + Id: "borynaId", + Roles: "smurf", + } + suite.userInfo["karynaId"] = &model.User{ + Username: "karyna", + Id: "karynaId", + Roles: "screamer", + } + suite.adminUsernames = "marynaId , karynaId, borynaId " + + p := New() + p.SetAPI(suite.api) + + err := p.OnConfigurationChange() + require.NoError(suite.T(), err) + + allowed, err := p.IsAuthorizedAdmin("marynaId") + require.NoError(suite.T(), err) + assert.True(suite.T(), allowed) + + allowed, err = p.IsAuthorizedAdmin("karynaId") + require.NoError(suite.T(), err) + assert.True(suite.T(), allowed) + + allowed, err = p.IsAuthorizedAdmin("borynaId") + require.NoError(suite.T(), err) + assert.True(suite.T(), allowed) +} + +func (suite *SuiteAuthorization) TestNonExistantUsersAreIgnored() { + suite.userInfo["marynaId"] = &model.User{ + Username: "maryna", + Id: "marynaId", + Roles: "smurf,reaper", + } + suite.adminUsernames = "marynaId,karynaId" + + p := New() + p.SetAPI(suite.api) + + err := p.OnConfigurationChange() + require.NoError(suite.T(), err) + + allowed, err := p.IsAuthorizedAdmin("marynaId") + require.NoError(suite.T(), err) + assert.True(suite.T(), allowed) + + allowed, err = p.IsAuthorizedAdmin("karynaId") + require.Error(suite.T(), err) +} + +func TestSuiteAuthorization(t *testing.T) { + suite.Run(t, new(SuiteAuthorization)) +} + func TestSpecialCases(t *testing.T) { links := make([]autolink.Autolink, 0) links = append(links, autolink.Autolink{ @@ -106,7 +314,7 @@ func TestSpecialCases(t *testing.T) { api.On("GetChannel", mock.AnythingOfType("string")).Return(&testChannel, nil) api.On("GetTeam", mock.AnythingOfType("string")).Return(&testTeam, nil) - p := Plugin{} + p := New() p.SetAPI(api) p.OnConfigurationChange() @@ -312,7 +520,7 @@ func TestHashtags(t *testing.T) { api.On("GetChannel", mock.AnythingOfType("string")).Return(&testChannel, nil) api.On("GetTeam", mock.AnythingOfType("string")).Return(&testTeam, nil) - p := Plugin{} + p := New() p.SetAPI(api) p.OnConfigurationChange() @@ -356,7 +564,7 @@ func TestAPI(t *testing.T) { api.On("GetTeam", mock.AnythingOfType("string")).Return(&testTeam, nil) api.On("SavePluginConfig", mock.AnythingOfType("map[string]interface {}")).Return(nil) - p := Plugin{} + p := New() p.SetAPI(api) p.OnConfigurationChange() p.OnActivate() diff --git a/server/main.go b/server/main.go index e6b75df1..e155061e 100644 --- a/server/main.go +++ b/server/main.go @@ -6,5 +6,5 @@ import ( ) func main() { - plugin.ClientMain(&autolinkplugin.Plugin{}) + plugin.ClientMain(autolinkplugin.New()) }