[GH-84] Add the concept of plugin admins #93

vespian · 2020-02-16T16:32:37Z

Summary

This plugin adds the concept of plugin admin users. A new plugin configuratoin option was added which lists comma-separated usernames for which modifying the configuration of the plugin is allowed (no auto-compleate, see [3]). Please see below for the sample config window:

and sample interaction with the plugin as a user who first did not have admin privileges, and then was granted ones:

While working on this change I have also changed how the Config is handled/passed around. In Go, function arguments are passed by value [1], so if. e.g. the underlying array of the Config.Links changed (i.e. new entries were added, causing reallocation), the other copies would be still using the old underlying array which could lead to weird errors. Also, based on the plugin template[2], the thread-safety seems to be guaranteed by passing around the reference to the plugin's config, so that if one goroutine updates the config, the other still has the consistent view on the old configuration as it is holding the old reference. Happy to discuss!

Do we still need EnableAdminCommand config option in this plugin, now that we have plugin administrators? Do we want to allow also adding userIds in the new field?

[1] https://stackoverflow.com/a/47486724
[2] https://github.com/mattermost/mattermost-plugin-starter-template/blob/282a8cf227849a25e306caff11597a0e4ba4cd01/server/configuration.go
[3] #84 (comment)

Ticket Link

Fixes ~~mattermost/mattermost#84~~
Fixes #84 (jfrerich 2/28/20)

vespian · 2020-02-16T16:33:12Z

server/autolinkplugin/config.go

@@ -77,34 +89,61 @@ func (p *Plugin) SaveLinks(links []autolink.Autolink) error {
 	return nil
 }

-func (p *Plugin) UpdateConfig(f func(conf *Config)) Config {


The return value was never used.

vespian · 2020-02-16T16:34:20Z

server/autolinkplugin/plugin_test.go

@@ -55,6 +57,259 @@ func TestPlugin(t *testing.T) {
 	assert.Equal(t, "Welcome to [Mattermost](https://mattermost.com)!", rpost.Message)
 }

+type SuiteAuthorization struct {


I took the liberty of not being consistent with other tests in this file, as using testing suite approach IMO is cleaner and we are already using testing suites in e.g. mattermost-server repo.

plugin.json

vespian · 2020-02-17T20:45:29Z

@crspeller Done, ready for another pass.

jfrerich

Great work, @vespian! I really appreciate the additional tests and descriptive nature of your discussion. I've only a few nit-picking suggestions to address before approving

plugin.json

server/autolinkplugin/config.go

vespian · 2020-02-19T17:31:12Z

@jfrerich Done, changes in commit Review fixes #1.

jfrerich

Thanks for the changes, @vespian!

server/autolinkplugin/config.go

crspeller · 2020-03-02T16:12:17Z

@DHaussermann @aaronrothschild

vespian · 2020-03-07T09:28:28Z

@DHaussermann @aaronrothschild Ping

crspeller · 2020-03-09T15:34:48Z

@vespian I pinged them on the community server. Sorry for the delay!

aaronrothschild · 2020-03-09T17:03:32Z

Sorry for the delay on this @vespian , thanks for your contribution!

aaronrothschild

Sorry for the delay, LGTM @vespian

aaronrothschild · 2020-03-09T17:09:08Z

Sorry @vespian is this a general addition for "Admins" on "Plugins" or just for the Autolink plugin?

Just want to make sure I'm understanding correctly.

If we have a general purpose "Admin Users" for plugins....we need to think this through a bit more. It likely would need to be limited to Enterprise Edition licences (ie: Only EE licenced MM servers would have a centralized plce for admins of plugins).

vespian · 2020-03-09T20:06:39Z

Sorry @vespian is this a general addition for "Admins" on "Plugins" or just for the Autolink plugin?

@aaronrothschild This is only for autolink plugin :)

DHaussermann · 2020-03-10T04:30:02Z

@vespian I'm seeing an odd bug here. The list of authorized user is clearing itself out and it seem related maybe to when the functionality is used to change autolink settings. It seems like it only works once for a non-admin user and then the list empties.

Steps:

Login as sys admin
Pick a target non-system admin user and note the ID
Open autolink config and save the ID as an authorized user (optionally, can also add other data)
Login in a separate browser session as target user
Use commands like list and help repeatedly and note that it keeps working
Use a command that changes the data such as enable, disable or set WordMatch
Try using any other autolink command
Observed:
A. Users see's message about functionality being locked to sysadmin
B. Return to admin browser session and refresh the list is now empty

Can you please see if you can also repro this? Please reach out if there is anymore clarification or details I can provide.

vespian · 2020-03-10T18:21:51Z

@DHaussermann Thanks a lot for checking my work, and a detailed bug report! I will take a closer look into it during the weekend, maybe earlier but no promises.

codecov-io · 2020-03-22T16:35:26Z

Codecov Report

Merging #93 into master will increase coverage by 3.85%.
The diff coverage is 78.57%.

@@            Coverage Diff             @@
##           master      #93      +/-   ##
==========================================
+ Coverage   36.43%   40.28%   +3.85%     
==========================================
  Files           6        6              
  Lines         538      561      +23     
==========================================
+ Hits          196      226      +30     
+ Misses        320      313       -7     
  Partials       22       22

Impacted Files	Coverage Δ
server/autolinkplugin/command.go	`0.00% <0.00%> (ø)`
server/autolinkplugin/config.go	`68.49% <82.60%> (+6.19%)`	⬆️
server/autolinkplugin/plugin.go	`68.36% <100.00%> (+12.04%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e53e3f...daefa31. Read the comment docs.

Co-Authored-By: Jason Frerich <[email protected]>

vespian · 2020-03-22T16:36:38Z

@DHaussermann Fixed in commit daefa31

Let me know if you find anything more. Thanks!

DHaussermann

Thanks @vespian for this fix! This is now working as expected.

Tested that designated plugin admin could use slash commands including those that modify data
Ensured previously found issue was resolved
No other issues found
LGTM!
Added test for this to release testing.

…ost-community#93) * Add the concept of plugin admins * Review fixes mattermost-community#1 Co-Authored-By: Jason Frerich <[email protected]> * Review fixes mattermost-community#2 Co-authored-by: Jason Frerich <[email protected]>

vespian commented Feb 16, 2020

View reviewed changes

vespian mentioned this pull request Feb 16, 2020

Add the concept of plugin admin users #84

Closed

levb requested review from crspeller, aaronrothschild, DHaussermann and jfrerich February 17, 2020 15:25

levb added 1: PM Review Requires review by a product manager 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Feb 17, 2020

crspeller requested changes Feb 17, 2020

View reviewed changes

plugin.json Outdated Show resolved Hide resolved

vespian changed the title ~~Add the concept of plugin admins~~ [GH-84] Add the concept of plugin admins Feb 17, 2020

vespian force-pushed the prozlach/GH-84_plugin_admin_users branch from 74f6058 to 0149686 Compare February 17, 2020 20:43

jfrerich suggested changes Feb 19, 2020

View reviewed changes

plugin.json Outdated Show resolved Hide resolved

server/autolinkplugin/config.go Outdated Show resolved Hide resolved

server/autolinkplugin/config.go Outdated Show resolved Hide resolved

vespian force-pushed the prozlach/GH-84_plugin_admin_users branch from aaf42da to bb32075 Compare February 19, 2020 17:30

hanzei requested review from crspeller and jfrerich February 20, 2020 04:53

jfrerich approved these changes Feb 25, 2020

View reviewed changes

crspeller reviewed Feb 28, 2020

View reviewed changes

server/autolinkplugin/config.go Show resolved Hide resolved

crspeller approved these changes Mar 2, 2020

View reviewed changes

crspeller removed the 2: Dev Review Requires review by a core committer label Mar 2, 2020

aaronrothschild removed the 1: PM Review Requires review by a product manager label Mar 9, 2020

aaronrothschild approved these changes Mar 9, 2020

View reviewed changes

jfrerich mentioned this pull request Mar 19, 2020

Bump version to 1.2.0 #101

Merged

3 tasks

vespian and others added 3 commits March 22, 2020 17:35

Add the concept of plugin admins

f7e6179

Review fixes mattermost-community#1

09585e4

Co-Authored-By: Jason Frerich <[email protected]>

Review fixes mattermost-community#2

daefa31

vespian force-pushed the prozlach/GH-84_plugin_admin_users branch from 8991513 to daefa31 Compare March 22, 2020 16:35

DHaussermann requested a review from jfrerich March 23, 2020 05:30

DHaussermann approved these changes Mar 23, 2020

View reviewed changes

DHaussermann added 4: Reviews Complete All reviewers have approved the pull request QA Review Done PR has been approved by QA and removed 3: QA Review Requires review by a QA tester 4: Reviews Complete All reviewers have approved the pull request labels Mar 23, 2020

jfrerich merged commit a10f0ee into mattermost-community:master Mar 23, 2020

jfrerich added the 4: Reviews Complete All reviewers have approved the pull request label Mar 23, 2020

vespian deleted the prozlach/GH-84_plugin_admin_users branch March 23, 2020 17:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GH-84] Add the concept of plugin admins #93

[GH-84] Add the concept of plugin admins #93

vespian commented Feb 16, 2020 •

edited by jfrerich

Loading

vespian Feb 16, 2020

vespian Feb 16, 2020

vespian commented Feb 17, 2020

jfrerich left a comment

vespian commented Feb 19, 2020

jfrerich left a comment

crspeller commented Mar 2, 2020

vespian commented Mar 7, 2020

crspeller commented Mar 9, 2020

aaronrothschild commented Mar 9, 2020

aaronrothschild left a comment

aaronrothschild commented Mar 9, 2020

vespian commented Mar 9, 2020

DHaussermann commented Mar 10, 2020 •

edited

Loading

vespian commented Mar 10, 2020

codecov-io commented Mar 22, 2020 •

edited

Loading

vespian commented Mar 22, 2020

DHaussermann left a comment

[GH-84] Add the concept of plugin admins #93

[GH-84] Add the concept of plugin admins #93

Conversation

vespian commented Feb 16, 2020 • edited by jfrerich Loading

Summary

Ticket Link

vespian Feb 16, 2020

Choose a reason for hiding this comment

vespian Feb 16, 2020

Choose a reason for hiding this comment

vespian commented Feb 17, 2020

jfrerich left a comment

Choose a reason for hiding this comment

vespian commented Feb 19, 2020

jfrerich left a comment

Choose a reason for hiding this comment

crspeller commented Mar 2, 2020

vespian commented Mar 7, 2020

crspeller commented Mar 9, 2020

aaronrothschild commented Mar 9, 2020

aaronrothschild left a comment

Choose a reason for hiding this comment

aaronrothschild commented Mar 9, 2020

vespian commented Mar 9, 2020

DHaussermann commented Mar 10, 2020 • edited Loading

vespian commented Mar 10, 2020

codecov-io commented Mar 22, 2020 • edited Loading

Codecov Report

vespian commented Mar 22, 2020

DHaussermann left a comment

Choose a reason for hiding this comment

vespian commented Feb 16, 2020 •

edited by jfrerich

Loading

DHaussermann commented Mar 10, 2020 •

edited

Loading

codecov-io commented Mar 22, 2020 •

edited

Loading