[MM-21688] Use dedicated bot account in Azure #32

mickmister · 2020-02-05T14:40:57Z

Summary

We are using the getSchedule Graph API endpoint to fetch user availabilities, and using application-level permissions to do so. The request requires us to supply a user ID, so we need a dedicated user in Azure to reference.

This PR allows the MM admin to connect the mscalendar bot to an account in Azure, using the /mscalendar connect_bot command.

Ticket Link

https://mattermost.atlassian.net/browse/MM-21688

Additional Info

Before running /mscalendar connect_bot, /mscalendar availability should fail, since we do not have a bot user to use for the user id.

Todo:

mscalendar/oauth2.go needs to have access to IsAuthorizedAdmin so we can ensure only admins can connect the bot account
- IsAuthorizedAdmin is now bot.IsUserAdmin and PluginAPI.IsSysAdmin. Shouldn't we want to union these two, since a sys admin should be treated as a plugin admin?
fix command/connect_test.go and command/disconnect_test.go once ^ has been resolved
oauth2_connect_test.go and oauth2_complete_test.go have been deleted, but there are some tests I have written to assert things pertaining to connecting the bot account
Write README instructions for creating dedicated account in Azure

Fixes #24

…s like IsAuthorizedAdmin

jfrerich · 2020-02-05T20:10:11Z

@mickmister, is this PR ready for review? I usually clone the repo to make checking some of the code easier to traverse and some of the methods don't exist.

server/command/connect.go

server/mscalendar/availability.go

server/remote/remote.go

mickmister · 2020-02-05T21:48:59Z

@jfrerich I should have marked the PR as draft since it is not properly building at the moment. IsAuthorizedAdmin in particular does not exist anymore after the project restructure, and needs to be exposed properly to some of the oauth related files. Is this the function you are referring to?

The connect.go, disconnect.go, and oauth* files have these issues. There are traces of GetRemoteUser as well, as @levb as pointed out.

@levb I need to discuss with you how to expose this admin-checking functionality to the oauth package, as only trusted admins should be able to connect/disconnect the bot from msgraph.

The oauth.App interface is more or less isolated from the rest of the packages at the moment (on purpose), but needs to have access to these admin-checking functions, as noted in a todo in the PR description. Or we need to restructure the flow to allow for checking before the oauth package receives the request.

mickmister · 2020-02-05T21:51:08Z

@jfrerich I can comment out certain things that need to be restructured/added so it will properly compile, though the tests will still fail without the added functions.

* implement IsAuthorizedAdmin, and expose through MSCalendar interface * ensure non-admins cannot connect the bot account

mickmister · 2020-02-06T17:53:45Z

@jfrerich This PR is ready for review now

mickmister · 2020-02-06T18:02:53Z

server/utils/pluginapi/api.go

-	return a.api.GetUserStatusesByIds(mattermostUserIDs)
+	st, err := a.api.GetUserStatusesByIds(mattermostUserIDs)
+	if err != nil {
+		return nil, err


@levb If this function is implemented as just return a.api.GetUserStatusesByIds(mattermostUserIDs), a nil error will be treated as a non-nil value to the caller.

You and I talked once about handling model.AppError and error, specifically that an interface pointer to a (nil pointer to a concrete type) is not a nil interface pointer, but I don't understand why this needs to be here in this case, and not the other functions in this file.

jfrerich · 2020-02-06T21:04:42Z

Thanks @mickmister! I'll get on the review soon!

levb

Just a couple nits/naming.

levb · 2020-02-07T14:48:24Z

server/mscalendar/availability.go

@@ -35,11 +35,25 @@ func (m *mscalendar) SyncStatusAll() (string, error) {
 		return "", err
 	}

-	mmIDs := userIndex.GetMattermostUserIDs()
+	index := userIndex.GetMattermostUserIDs()


1/5 s/index/allIDs/

levb · 2020-02-07T14:48:49Z

server/mscalendar/availability.go

@@ -35,11 +35,25 @@ func (m *mscalendar) SyncStatusAll() (string, error) {
 		return "", err
 	}

-	mmIDs := userIndex.GetMattermostUserIDs()
+	index := userIndex.GetMattermostUserIDs()
+	mmIDs := []string{}


1/5 s/mmIDs/filteredIDs/

jfrerich

Awesome work, @mickmister! I have one question and a single suggestion on response wording to the user.

server/command/connect.go

server/mscalendar/oauth2.go

* reword user error strings * rename vars

levb · 2020-02-07T20:27:17Z

server/mscalendar/availability.go

-	index := userIndex.GetMattermostUserIDs()
-	mmIDs := []string{}
-	for _, id := range index {
+	allIDs := userIndex.GetMattermostUserIDs()


only if you do another push anyway... a nit of a nit, but maybe inline userIndex.GetMattermostUserIDs() and just use ids for the return? :)

jfrerich

changes requests have been addressed and questions I had have been explained. Approving and thanks for the awesome work, @mickmister

DHaussermann

Spoke with @mickmister about this change. I will test post merge.

mickmister added 2 commits February 5, 2020 08:44

most things updated for bot account. need to resolve some dependencie…

89b0660

…s like IsAuthorizedAdmin

add connect and disconnect command tests

69e5e79

mickmister added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Feb 5, 2020

mickmister requested review from levb, jfrerich and DHaussermann February 5, 2020 14:40

levb reviewed Feb 5, 2020

View reviewed changes

server/command/connect.go Outdated Show resolved Hide resolved

server/mscalendar/availability.go Show resolved Hide resolved

server/remote/remote.go Outdated Show resolved Hide resolved

finish PR

11f2605

* implement IsAuthorizedAdmin, and expose through MSCalendar interface * ensure non-admins cannot connect the bot account

mickmister requested a review from levb February 6, 2020 17:48

lint

1f6b804

mickmister commented Feb 6, 2020

View reviewed changes

levb previously approved these changes Feb 7, 2020

View reviewed changes

jfrerich suggested changes Feb 7, 2020

View reviewed changes

server/command/connect.go Outdated Show resolved Hide resolved

server/command/connect.go Outdated Show resolved Hide resolved

server/mscalendar/oauth2.go Show resolved Hide resolved

server/mscalendar/oauth2.go Show resolved Hide resolved

mickmister commented Feb 7, 2020

View reviewed changes

server/mscalendar/oauth2.go Outdated Show resolved Hide resolved

PR feedback:

536d402

* reword user error strings * rename vars

mickmister dismissed levb’s stale review via 536d402 February 7, 2020 19:48

mickmister requested a review from jfrerich February 7, 2020 19:48

levb approved these changes Feb 7, 2020

View reviewed changes

jfrerich approved these changes Feb 8, 2020

View reviewed changes

mickmister removed the 2: Dev Review Requires review by a core committer label Feb 10, 2020

DHaussermann added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Feb 10, 2020

DHaussermann approved these changes Feb 10, 2020

View reviewed changes

mickmister merged commit 864a161 into master Feb 11, 2020

mickmister deleted the bot-account-manual-merge branch February 11, 2020 05:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM-21688] Use dedicated bot account in Azure #32

[MM-21688] Use dedicated bot account in Azure #32

mickmister commented Feb 5, 2020 •

edited

Loading

jfrerich commented Feb 5, 2020

mickmister commented Feb 5, 2020 •

edited

Loading

mickmister commented Feb 5, 2020

mickmister commented Feb 6, 2020

mickmister Feb 6, 2020 •

edited

Loading

jfrerich commented Feb 6, 2020

levb left a comment

levb Feb 7, 2020

levb Feb 7, 2020

jfrerich left a comment

levb Feb 7, 2020

jfrerich left a comment

DHaussermann left a comment

[MM-21688] Use dedicated bot account in Azure #32

[MM-21688] Use dedicated bot account in Azure #32

Conversation

mickmister commented Feb 5, 2020 • edited Loading

Summary

Ticket Link

Additional Info

jfrerich commented Feb 5, 2020

mickmister commented Feb 5, 2020 • edited Loading

mickmister commented Feb 5, 2020

mickmister commented Feb 6, 2020

mickmister Feb 6, 2020 • edited Loading

Choose a reason for hiding this comment

jfrerich commented Feb 6, 2020

levb left a comment

Choose a reason for hiding this comment

levb Feb 7, 2020

Choose a reason for hiding this comment

levb Feb 7, 2020

Choose a reason for hiding this comment

jfrerich left a comment

Choose a reason for hiding this comment

levb Feb 7, 2020

Choose a reason for hiding this comment

jfrerich left a comment

Choose a reason for hiding this comment

DHaussermann left a comment

Choose a reason for hiding this comment

mickmister commented Feb 5, 2020 •

edited

Loading

mickmister commented Feb 5, 2020 •

edited

Loading

mickmister Feb 6, 2020 •

edited

Loading