-
Notifications
You must be signed in to change notification settings - Fork 151
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.
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
Support the MS Teams Tab App #1942
base: master
Are you sure you want to change the base?
Conversation
@@ -18,7 +18,7 @@ type ActionsService struct { | |||
// Create an action. Returns the id of the newly created action. | |||
func (s *ActionsService) Create(ctx context.Context, channelID string, opts ChannelActionCreateOptions) (string, error) { | |||
actionURL := fmt.Sprintf("actions/channels/%s", channelID) | |||
req, err := s.client.newRequest(http.MethodPost, actionURL, opts) | |||
req, err := s.client.newAPIRequest(http.MethodPost, actionURL, opts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the first in a series of s/newRequest/newAPIRequest/
to allow the client
package to be used with non-/api
endpoints.
} | ||
|
||
func (s *TabAppService) GetRuns(ctx context.Context, token string, options TabAppGetRunsOptions) (*TabAppResults, error) { | ||
url := fmt.Sprintf("plugins/%s/tabapp/runs", manifestID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use the /api
endpoint directly here.
go.mod
Outdated
@@ -1,6 +1,8 @@ | |||
module github.com/mattermost/mattermost-plugin-playbooks | |||
|
|||
go 1.19 | |||
go 1.21 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This came for free -- trusting to e2e tests for coverage here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above, but only if no additional changes are needed. otherwise bumping to 1.22 can happen independently.
"help_text": "Enable experimental features that come with in-progress UI, bugs, and cool stuff." | ||
} | ||
] | ||
"id": "playbooks", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noisy because of some whitespace cleanup.
@@ -41,8 +41,8 @@ func NewHandler(pluginAPI *pluginapi.Client, config config.Service) *Handler { | |||
} | |||
|
|||
root := mux.NewRouter() | |||
root.Use(LogRequest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving this to the root to log all requests, not just those on api/
.
@@ -0,0 +1,91 @@ | |||
package api |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copied verbatim from the MS Teams plugin to help with testing permutations -- will lift this to a common place soonish.
MicrosoftTeamsAppDomain = "https://playbooks.integrations.mattermost.com" | ||
ExpectedAudience = "api://playbooks.integrations.mattermost.com/8f7d5beb-ed24-4d95-aa31-c26298d5a982" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both hard-coded to match our production deploy.
) | ||
|
||
// TestValidateToken was inspired by https://github.com/MicahParks/keyfunc/blob/main/keyfunc_test.go. | ||
func TestValidateToken(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These unit tests focus on the validation of the token -- I've added top-level tests that focus on the tabapp results themselves.
assert.Equal(t, http.StatusUnauthorized, validationErr.StatusCode) | ||
}) | ||
|
||
t.Run("hmac key pretending to be rsa", func(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @esarafianou
|
||
t.Run("CORS headers, mis-matched Origin header, developer mode", func(t *testing.T) { | ||
setTabApp(t, true) | ||
setDeveloperMode(t, true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We basically only test with developer mode, as we can't spoof a token for the production configuration.
Swapping @sbishel for @crspeller given the dual overlap of Playbook and the "Fast Futures" nature of this initiative. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Jesse! One comment around Full name and two optional comments
server/api/tabapp.go
Outdated
FirstName: user.FirstName, | ||
LastName: user.LastName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need the same check for the Full name option as when we collect participants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gah, thank you @esarafianou -- this was the last change I addressed and I should have given this a bit more "brain soak" time. Will address!
.github/workflows/ci.yaml
Outdated
@@ -11,7 +11,7 @@ defaults: | |||
|
|||
env: | |||
TERM: xterm | |||
GO_VERSION: 1.19.5 | |||
GO_VERSION: 1.21.13 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're updating it here, can we have it match the mattermost
golang version: https://github.com/mattermost/mattermost/blob/master/server/go.mod#L3-L5 ?
go.mod
Outdated
@@ -1,6 +1,8 @@ | |||
module github.com/mattermost/mattermost-plugin-playbooks | |||
|
|||
go 1.19 | |||
go 1.21 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above, but only if no additional changes are needed. otherwise bumping to 1.22 can happen independently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking this as requested changes for the Full name one
if err != nil { | ||
return nil, err | ||
} | ||
resp.Body.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit because unsure: errcheck?
Username: "msteams", | ||
DisplayName: "MS Teams", | ||
OwnerId: "playbooks", | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the MSTeams plugin is not installed, and as a consequence we don't have the MSTeams bot, what's the advantage of creating a MSTeams bot here instead of using the Playbooks one?
I'm just thinking if the name of the MSTeams bot has to be changed for some reason or we want to update the avatar for the bot, we'll have to remember to do it everywhere 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry it took me a minute to review. Swapping my review into "Approved" as I don't have anything that is a blocker to move forward.
Summary
This is the "backend" for https://github.com/mattermost/mattermost-teams-tab, allowing colleagues in Teams to connect to a Mattermost server, proving their membership in a configured tenant via the Microsoft ID token, and getting read-only access to playbook runs that have explicitly invited the
@msteams
bot.Ticket Link
Fixes: https://mattermost.atlassian.net/browse/MM-60362
Fixes: https://mattermost.atlassian.net/browse/MM-60363
Fixes: https://mattermost.atlassian.net/browse/MM-60366
Fixes: https://mattermost.atlassian.net/browse/MM-60367
Fixes: https://mattermost.atlassian.net/browse/MM-60368
Checklist