From 95a20a4ac1ce7ea4deabb976cdaeeb7b1c9322b0 Mon Sep 17 00:00:00 2001 From: Lev <1187448+levb@users.noreply.github.com> Date: Tue, 26 Apr 2022 17:28:49 -0700 Subject: [PATCH] MM-43004: better logging (#322) cc @DHaussermann --- apps/call.go | 25 +++ apps/call_request.go | 25 +++ apps/call_response.go | 82 ++++++++- apps/context.go | 116 ++++++++++++ apps/expand.go | 18 ++ apps/log_string_test.go | 170 ++++++++++++++++++ assets/i18n/active.en.json | 3 +- assets/i18n/active.es.json | 4 - cmd/appsctl/aws.go | 2 +- cmd/appsctl/mattermost.go | 4 +- dev/short_names.md | 8 + examples/go/hello-lifecycle/hello.go | 8 +- .../go/hello-serverless/function/handler.go | 4 +- .../golang-middleware/function/handler.go | 8 +- server/appservices/kv.go | 12 +- server/appservices/oauth2.go | 14 +- server/appservices/service.go | 20 +-- server/appservices/subscriptions.go | 12 +- server/builtin/app.go | 5 +- server/builtin/debug_clean.go | 27 ++- server/builtin/debug_session_view.go | 2 +- server/config/config.go | 13 +- server/config/log_string_test.go | 54 ++++++ server/config/service.go | 28 +-- server/config/test_service.go | 4 +- server/httpin/gateway/gateway.go | 4 +- server/httpin/gateway/oauth2.go | 24 +-- server/httpin/gateway/static.go | 14 +- server/httpin/gateway/webhook.go | 36 ++-- server/httpin/handler.go | 63 ++++--- server/httpin/restapi/admin.go | 46 ++--- server/httpin/restapi/app_kv.go | 26 +-- server/httpin/restapi/app_kv_test.go | 11 +- server/httpin/restapi/app_oauth2_store.go | 16 +- server/httpin/restapi/app_subscribe.go | 28 +-- server/httpin/restapi/call.go | 34 ++-- server/httpin/restapi/call_test.go | 40 +---- server/httpin/restapi/marketplace.go | 8 +- server/httpin/restapi/ping.go | 2 +- server/httpin/restapi/restapi.go | 4 +- server/httpin/restapi/restapitestlib.go | 8 +- server/httpin/restapi/ux_bindings.go | 6 +- server/httpin/restapi/ux_get_ids.go | 8 +- server/httpin/service.go | 13 +- server/incoming/request.go | 7 +- server/mocks/mock_config/mock_config.go | 19 +- server/mocks/mock_store/mock_app.go | 16 +- server/mocks/mock_store/mock_appkv.go | 24 +-- server/mocks/mock_store/mock_session.go | 32 ++-- server/plugin.go | 47 +++-- server/plugin_test.go | 2 +- server/proxy/bindings.go | 12 +- server/proxy/bindings_test.go | 2 +- server/proxy/call.go | 9 +- server/proxy/enable.go | 4 +- server/proxy/install.go | 21 ++- server/proxy/list.go | 18 +- server/proxy/notify.go | 20 +-- server/proxy/notify_test.go | 4 +- server/proxy/oauth2.go | 8 +- server/proxy/service.go | 20 +-- server/proxy/service_test.go | 2 +- server/proxy/synchronize.go | 6 +- server/proxy/uninstall.go | 2 +- server/proxy/webhook.go | 20 +-- server/session/session.go | 28 +-- server/session/session_test.go | 18 +- server/store/appkv.go | 14 +- server/store/apps.go | 22 +-- server/store/manifest.go | 28 +-- server/store/oauth2.go | 17 +- server/store/oauth2_test.go | 20 +-- server/store/session.go | 30 ++-- server/store/subscriptions.go | 23 ++- server/store/subscriptions_test.go | 34 ++-- upstream/provision.go | 2 +- upstream/upaws/clean.go | 12 +- upstream/upaws/client.go | 4 +- upstream/upaws/initialize.go | 14 +- upstream/upaws/provision_app.go | 8 +- upstream/upaws/provision_data.go | 8 +- upstream/upplugin/httpclient.go | 4 +- utils/httputils/utils.go | 4 +- utils/{zap_logger.go => logger.go} | 133 +++++++------- utils/plugin_logger.go | 81 +++++++++ 85 files changed, 1199 insertions(+), 659 deletions(-) create mode 100644 apps/log_string_test.go create mode 100644 dev/short_names.md create mode 100644 server/config/log_string_test.go rename utils/{zap_logger.go => logger.go} (60%) create mode 100644 utils/plugin_logger.go diff --git a/apps/call.go b/apps/call.go index a775d393b..4cc075011 100644 --- a/apps/call.go +++ b/apps/call.go @@ -5,6 +5,9 @@ package apps import ( "encoding/json" + "fmt" + + "github.com/mattermost/mattermost-plugin-apps/utils" ) // Call defines a way to invoke an App's function. Calls are used to fetch App's @@ -128,3 +131,25 @@ func (c *Call) PartialCopy() *Call { } return &clone } + +func (c Call) String() string { + s := c.Path + if c.Expand != nil { + s += fmt.Sprintf(", expand: %v", c.Expand.String()) + } + if c.State != nil { + s += fmt.Sprintf(", state: %v", utils.LogDigest(c.State)) + } + return s +} + +func (c Call) Loggable() []interface{} { + props := []interface{}{"call_path", c.Path} + if c.Expand != nil { + props = append(props, "call_expand", c.Expand.String()) + } + if c.State != nil { + props = append(props, "call_state", utils.LogDigest(c.State)) + } + return props +} diff --git a/apps/call_request.go b/apps/call_request.go index f32715609..fe228b466 100644 --- a/apps/call_request.go +++ b/apps/call_request.go @@ -5,7 +5,10 @@ package apps import ( "encoding/json" + "fmt" "io" + + "github.com/mattermost/mattermost-plugin-apps/utils" ) // CallRequest envelops all requests sent to Apps. @@ -138,3 +141,25 @@ func (creq *CallRequest) BoolValue(name string) bool { return false } + +func (creq CallRequest) String() string { + s := fmt.Sprintf("call: %s, context: %s", creq.Call.String(), creq.Context.String()) + if len(creq.Values) > 0 { + s += ", values: " + utils.LogDigest(creq.Values) + } + if creq.Query != "" { + s += ", query: " + creq.Query + } + return s +} + +func (creq CallRequest) Loggable() []interface{} { + props := append([]interface{}{}, creq.Call, creq.Context) + if len(creq.Values) > 0 { + props = append(props, "values", utils.LogDigest(creq.Values)) + } + if creq.Query != "" { + props = append(props, "query", creq.Query) + } + return props +} diff --git a/apps/call_response.go b/apps/call_response.go index 837ad2a33..499d4bc04 100644 --- a/apps/call_response.go +++ b/apps/call_response.go @@ -5,6 +5,8 @@ package apps import ( "fmt" + + "github.com/mattermost/mattermost-plugin-apps/utils" ) type CallResponseType string @@ -103,9 +105,83 @@ func NewLookupResponse(opts []SelectOption) CallResponse { } // Error makes CallResponse a valid error, for convenience -func (cr CallResponse) Error() string { - if cr.Type == CallResponseTypeError { - return cr.Text +func (cresp CallResponse) Error() string { + if cresp.Type == CallResponseTypeError { + return cresp.Text } return "" } + +func (cresp CallResponse) String() string { + switch cresp.Type { + case CallResponseTypeError: + return fmt.Sprint("Error: ", cresp.Text) + + case "", CallResponseTypeOK: + switch { + case cresp.Text != "" && cresp.Data == nil: + return fmt.Sprint("OK: ", cresp.Text) + case cresp.Text != "" && cresp.Data != nil: + return fmt.Sprintf("OK: %s; + data type %T", cresp.Text, cresp.Data) + case cresp.Data != nil: + return fmt.Sprintf("OK: data type %T, value: %v", cresp.Data, cresp.Data) + default: + return "OK: (none)" + } + + case CallResponseTypeForm: + return fmt.Sprintf("Form: %v", utils.ToJSON(cresp.Form)) + + case CallResponseTypeCall: + return fmt.Sprintf("Call: %v", cresp.Call) + + case CallResponseTypeNavigate: + s := fmt.Sprintf("Navigate to: %q", cresp.NavigateToURL) + if cresp.UseExternalBrowser { + s += ", using external browser" + } + return s + + default: + return fmt.Sprintf("?? unknown response type %q", cresp.Type) + } +} + +func (cresp CallResponse) Loggable() []interface{} { + props := []interface{}{"response_type", string(cresp.Type)} + + switch cresp.Type { + case CallResponseTypeError: + props = append(props, "error", cresp.Text) + + case "", CallResponseTypeOK: + if cresp.Text != "" { + text := cresp.Text + if len(text) > 100 { + text = text[:100] + "...(truncated)" + } + props = append(props, "response_text", text) + } + if cresp.Data != nil { + props = append(props, "response_data", cresp.Data) + } + + case CallResponseTypeForm: + if cresp.Form != nil { + props = append(props, "response_form", *cresp.Form) + } + + case CallResponseTypeCall: + if cresp.Call != nil { + props = append(props, "response_call", cresp.Call.String()) + } + + case CallResponseTypeNavigate: + props = append(props, "response_url", cresp.NavigateToURL) + if cresp.UseExternalBrowser { + props = append(props, "use_external_browser", true) + } + } + + return props +} diff --git a/apps/context.go b/apps/context.go index cef21ab09..facac92d1 100644 --- a/apps/context.go +++ b/apps/context.go @@ -4,7 +4,13 @@ package apps import ( + "fmt" + "sort" + "strings" + "github.com/mattermost/mattermost-server/v6/model" + + "github.com/mattermost/mattermost-plugin-apps/utils" ) // Context is included in CallRequest and provides App with information about @@ -118,3 +124,113 @@ type OAuth2Context struct { User interface{} `json:"user,omitempty"` } + +func (c Context) String() string { + display, _ := c.loggable() + + keys := []string{} + for k := range display { + keys = append(keys, k) + } + sort.Strings(keys) + + ss := []string{} + for _, k := range keys { + ss = append(ss, fmt.Sprintf("%s: %s", k, display[k])) + } + return strings.Join(ss, ", ") +} + +func (c Context) Loggable() []interface{} { + _, props := c.loggable() + return props +} + +func (c Context) loggable() (map[string]string, []interface{}) { + display := map[string]string{} + props := []interface{}{} + add := func(f, v string) { + if v != "" { + display[f] = v + props = append(props, f, v) + } + } + + add("locale", c.Locale) + add("subject", string(c.Subject)) + add("ua", c.UserAgentContext.UserAgent) + add("ua_loc", string(c.UserAgentContext.Location)) + if !c.UserAgentContext.TrackAsSubmit { + add("is_not_submit", "true") + } + + if c.ExpandedContext.ActingUser != nil { + display["acting_user"] = c.ExpandedContext.ActingUser.GetDisplayName(model.ShowNicknameFullName) + props = append(props, "acting_user_id", c.ExpandedContext.ActingUser.Id) + } + if c.ExpandedContext.ActingUserAccessToken != "" { + display["acting_user_access_token"] = utils.LastN(c.ExpandedContext.ActingUserAccessToken, 4) + props = append(props, "acting_user_access_token", utils.LastN(c.ExpandedContext.ActingUserAccessToken, 4)) + } + + if c.ExpandedContext.Channel != nil { + display["channel"] = c.ExpandedContext.Channel.Name + props = append(props, "channel_id", c.ExpandedContext.Channel.Id) + } + if c.ExpandedContext.Team != nil { + display["team"] = c.ExpandedContext.Team.Name + props = append(props, "team_id", c.ExpandedContext.Channel.Id) + } + if c.ExpandedContext.Post != nil { + display["post"] = utils.LastN(c.ExpandedContext.Post.Message, 32) + props = append(props, "post_id", c.ExpandedContext.Post.Id) + } + if c.ExpandedContext.RootPost != nil { + display["root_post"] = utils.LastN(c.ExpandedContext.RootPost.Message, 32) + props = append(props, "root_post_id", c.ExpandedContext.RootPost.Id) + } + + if c.ExpandedContext.BotUserID != "" { + display["bot_user_id"] = c.ExpandedContext.BotUserID + props = append(props, "bot_user_id", c.ExpandedContext.BotUserID) + } + if c.ExpandedContext.BotAccessToken != "" { + display["bot_access_token"] = utils.LastN(c.ExpandedContext.BotAccessToken, 4) + props = append(props, "bot_access_token", utils.LastN(c.ExpandedContext.BotAccessToken, 4)) + } + if c.ExpandedContext.ChannelMember != nil { + display["channel_member_channel_id"] = c.ExpandedContext.ChannelMember.ChannelId + display["channel_member_user_id"] = c.ExpandedContext.ChannelMember.UserId + props = append(props, "channel_member_channel_id", c.ExpandedContext.ChannelMember.ChannelId) + props = append(props, "channel_member_user_id", c.ExpandedContext.ChannelMember.UserId) + } + if c.ExpandedContext.TeamMember != nil { + display["team_member_team_id"] = c.ExpandedContext.TeamMember.TeamId + display["team_member_user_id"] = c.ExpandedContext.TeamMember.UserId + props = append(props, "team_member_team_id", c.ExpandedContext.TeamMember.TeamId) + props = append(props, "team_member_user_id", c.ExpandedContext.TeamMember.UserId) + } + + if c.ExpandedContext.OAuth2.OAuth2App.RemoteRootURL != "" { + display["remote_url"] = c.ExpandedContext.OAuth2.OAuth2App.RemoteRootURL + props = append(props, "remote_url", c.ExpandedContext.OAuth2.OAuth2App.RemoteRootURL) + } + if c.ExpandedContext.OAuth2.OAuth2App.ClientID != "" { + display["remote_client_id"] = utils.LastN(c.ExpandedContext.OAuth2.OAuth2App.ClientID, 4) + props = append(props, "remote_client_id", utils.LastN(c.ExpandedContext.OAuth2.OAuth2App.ClientID, 4)) + } + if c.ExpandedContext.OAuth2.OAuth2App.ClientSecret != "" { + display["remote_client_secret"] = utils.LastN(c.ExpandedContext.OAuth2.OAuth2App.ClientSecret, 4) + props = append(props, "remote_client_secret", utils.LastN(c.ExpandedContext.OAuth2.OAuth2App.ClientSecret, 4)) + } + if c.ExpandedContext.OAuth2.OAuth2App.Data != nil { + display["app_data"] = "(private)" + props = append(props, "app_data", "(private)") + } + if c.ExpandedContext.OAuth2.User != nil { + display["user_data"] = "(private)" + props = append(props, "user_data", "(private)") + } + + return display, props +} diff --git a/apps/expand.go b/apps/expand.go index 3f42ae33a..c26f24194 100644 --- a/apps/expand.go +++ b/apps/expand.go @@ -3,6 +3,13 @@ package apps +import ( + "sort" + "strings" + + "github.com/mattermost/mattermost-plugin-apps/utils" +) + type ExpandLevel string const ( @@ -81,3 +88,14 @@ type Expand struct { // previously stored with appclient.StoreOAuthUser). OAuth2User ExpandLevel `json:"oauth2_user,omitempty"` } + +func (e Expand) String() string { + m := map[string]string{} + utils.Remarshal(&m, e) + ss := []string{} + for k, v := range m { + ss = append(ss, k+":"+v) + } + sort.Strings(ss) + return strings.Join(ss, ",") +} diff --git a/apps/log_string_test.go b/apps/log_string_test.go new file mode 100644 index 000000000..b93b385bb --- /dev/null +++ b/apps/log_string_test.go @@ -0,0 +1,170 @@ +// Copyright (c) 2020-present Mattermost, Inc. All Rights Reserved. +// See License for license information. + +package apps + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/mattermost/mattermost-plugin-apps/utils" +) + +func TestLoggable(t *testing.T) { + var _ utils.HasLoggable = Call{} + var _ utils.HasLoggable = CallRequest{} + var _ utils.HasLoggable = CallResponse{} + var _ utils.HasLoggable = Context{} + + var simpleContext = Context{ + ActingUserID: "id_of_acting_user", + ExpandedContext: ExpandedContext{ + BotUserID: "id_of_bot_user", + BotAccessToken: "bot_user_access_tokenXYZ", + }, + } + var simpleCall = Call{ + Path: "/some-path", + } + var fullCall = Call{ + Path: "/some-path", + State: map[string]interface{}{ + "key1": "confidential1", + "key2": "confidential2", + }, + Expand: &Expand{ + User: ExpandAll, + Channel: ExpandSummary, + ActingUserAccessToken: ExpandAll, + OAuth2App: ExpandAll, + }, + } + var simpleCallRequest = CallRequest{ + Call: simpleCall, + Context: simpleContext, + } + var fullCallRequest = CallRequest{ + Call: fullCall, + Context: simpleContext, + Values: map[string]interface{}{ + "vkey1": "confidential1", + "vkey2": "confidential2", + }, + } + var testData = map[string]interface{}{ + "A": "test", + "B": 99, + } + var testForm = Form{ + Title: "name", + Fields: []Field{ + { + Name: "f1", + }, + { + Name: "f2", + }, + }, + Submit: &simpleCall, + } + + for name, test := range map[string]struct { + In interface{} + ExpectedProps []interface{} + ExpectedString string + }{ + "Context": { + In: simpleContext, + ExpectedProps: []interface{}{ + "is_not_submit", "true", + "bot_user_id", "id_of_bot_user", + "bot_access_token", "***nXYZ", + }, + ExpectedString: "bot_access_token: ***nXYZ, bot_user_id: id_of_bot_user, is_not_submit: true", + }, + "Call simple": { + In: simpleCall, + ExpectedProps: []interface{}{ + "call_path", "/some-path", + }, + ExpectedString: "/some-path", + }, + "Call full": { + In: fullCall, + ExpectedProps: []interface{}{ + "call_path", "/some-path", + "call_expand", "acting_user_access_token:all,channel:summary,oauth2_app:all,user:all", + "call_state", "key1,key2", + }, + ExpectedString: "/some-path, expand: acting_user_access_token:all,channel:summary,oauth2_app:all,user:all, state: key1,key2", + }, + "CallRequest simple": { + In: simpleCallRequest, + ExpectedProps: []interface{}{simpleCall, simpleContext}, + ExpectedString: "call: /some-path, context: bot_access_token: ***nXYZ, bot_user_id: id_of_bot_user, is_not_submit: true", + }, + "CallRequest full": { + In: fullCallRequest, + ExpectedProps: []interface{}{fullCall, simpleContext, "values", "vkey1,vkey2"}, + ExpectedString: "call: /some-path, expand: acting_user_access_token:all,channel:summary,oauth2_app:all,user:all, state: key1,key2, context: bot_access_token: ***nXYZ, bot_user_id: id_of_bot_user, is_not_submit: true, values: vkey1,vkey2", + }, + "CallResponse text": { + In: NewTextResponse("test"), + ExpectedProps: []interface{}{"response_type", "ok", "response_text", "test"}, + ExpectedString: "OK: test", + }, + "CallResponse JSON data": { + In: NewDataResponse(testData), + ExpectedProps: []interface{}{"response_type", "ok", "response_data", testData}, + ExpectedString: "OK: data type map[string]interface {}, value: map[A:test B:99]", + }, + "CallResponse byte data": { + In: NewDataResponse([]byte("12345")), + ExpectedProps: []interface{}{"response_type", "ok", "response_data", []byte("12345")}, + ExpectedString: "OK: data type []uint8, value: [49 50 51 52 53]", + }, + "CallResponse text data": { + In: NewDataResponse("12345"), + ExpectedProps: []interface{}{"response_type", "ok", "response_data", "12345"}, + ExpectedString: "OK: data type string, value: 12345", + }, + "CallResponse form": { + In: NewFormResponse(testForm), + ExpectedProps: []interface{}{"response_type", "form", "response_form", testForm}, + ExpectedString: `Form: {"title":"name","submit":{"path":"/some-path"},"fields":[{"name":"f1","type":""},{"name":"f2","type":""}]}`, + }, + "CallResponse navigate": { + In: CallResponse{ + Type: CallResponseTypeNavigate, + NavigateToURL: "http://x.y.z", + UseExternalBrowser: true, + }, + ExpectedProps: []interface{}{"response_type", "navigate", "response_url", "http://x.y.z", "use_external_browser", true}, + ExpectedString: `Navigate to: "http://x.y.z", using external browser`, + }, + "CallResponse call": { + In: CallResponse{ + Type: CallResponseTypeCall, + Call: &fullCall, + }, + ExpectedProps: []interface{}{"response_type", "call", "response_call", "/some-path, expand: acting_user_access_token:all,channel:summary,oauth2_app:all,user:all, state: key1,key2"}, + ExpectedString: `Call: /some-path, expand: acting_user_access_token:all,channel:summary,oauth2_app:all,user:all, state: key1,key2`, + }, + } { + t.Run(name, func(t *testing.T) { + if test.ExpectedProps != nil { + lp, ok := test.In.(utils.HasLoggable) + require.True(t, ok) + require.EqualValues(t, test.ExpectedProps, lp.Loggable()) + } + + if test.ExpectedString != "" { + s, ok := test.In.(fmt.Stringer) + require.True(t, ok) + require.Equal(t, test.ExpectedString, s.String()) + } + }) + } +} diff --git a/assets/i18n/active.en.json b/assets/i18n/active.en.json index e3832b89a..94adaddde 100644 --- a/assets/i18n/active.en.json +++ b/assets/i18n/active.en.json @@ -4,7 +4,8 @@ "command.debug.bindings.label": "bindings", "command.debug.clean.description": "Remove all Apps and reset the persistent store", "command.debug.clean.label": "clean", - "command.debug.clean.submit": "Deleted all KV records and emptied the config.", + "command.debug.clean.submit.config": "Emptied the config.", + "command.debug.clean.submit.kv": "Deleted all KV records.", "command.debug.kv.clean.description": "Delete KV keys for an app, in a specific namespace.", "command.debug.kv.clean.hint": "[ App ID ]", "command.debug.kv.clean.label": "clean", diff --git a/assets/i18n/active.es.json b/assets/i18n/active.es.json index 0edf39412..5c7d81439 100644 --- a/assets/i18n/active.es.json +++ b/assets/i18n/active.es.json @@ -19,10 +19,6 @@ "hash": "sha1-6a1cec45eaf37b34e1b1d89130d7746fe4006346", "other": "limpiar" }, - "command.debug.clean.submit": { - "hash": "sha1-56290358b8e6c2d856b8668240035c92f47e220a", - "other": "Todos los registros KV han sido eliminados y la configuraciĆ³n vaciada." - }, "command.debug.label": { "hash": "sha1-32faaecac742100f7753f0c1d0aa0add01b4046b", "other": "depurar" diff --git a/cmd/appsctl/aws.go b/cmd/appsctl/aws.go index ca29c2ec9..b46ac68ab 100644 --- a/cmd/appsctl/aws.go +++ b/cmd/appsctl/aws.go @@ -200,7 +200,7 @@ var awsTestS3Cmd = &cobra.Command{ return err } r := string(data) - log.Debugf("Received: %s", string(data)) + log.Debugw("received data", "data", string(data)) if r != "static pong" { return errors.Errorf("expected 'static pong', got '%s'", r) diff --git a/cmd/appsctl/mattermost.go b/cmd/appsctl/mattermost.go index f916e743a..ffb7b0a47 100644 --- a/cmd/appsctl/mattermost.go +++ b/cmd/appsctl/mattermost.go @@ -38,14 +38,14 @@ func updateMattermost(m apps.Manifest, deployType apps.DeployType, installApp bo if err != nil { return errors.Wrap(err, "failed to add local manifest to Mattermost") } - log.Debugw("Updated local manifest", "app_id", m.AppID, "deploy_type", deployType) + log.Debugw("updated local manifest", "app_id", m.AppID, "deploy_type", deployType) if installApp { _, err = appClient.InstallApp(m.AppID, deployType) if err != nil { return errors.Wrap(err, "failed to install the app to Mattermost") } - log.Debugw("Installed app to Mattermost", "app_id", m.AppID) + log.Debugw("installed app to Mattermost", "app_id", m.AppID) } return nil diff --git a/dev/short_names.md b/dev/short_names.md new file mode 100644 index 000000000..55efa8d80 --- /dev/null +++ b/dev/short_names.md @@ -0,0 +1,8 @@ +##### Short Variable Name Convention + +| short| type | +| :--- | :--- | +| creq | [`apps.CallRequest`](../apps/call_request.go) | +| cresp | [`apps.CallResponse`](../apps/call_response.go) | +| r | [`*incoming.Request*`](../server/incoming/request.go) | +| req | `*http.Request` | diff --git a/examples/go/hello-lifecycle/hello.go b/examples/go/hello-lifecycle/hello.go index 8659aca41..4b6d8e20d 100644 --- a/examples/go/hello-lifecycle/hello.go +++ b/examples/go/hello-lifecycle/hello.go @@ -41,12 +41,12 @@ func main() { panic(http.ListenAndServe(addr, nil)) } -func respondWithMessage(message string) func(w http.ResponseWriter, r *http.Request) { +func respondWithMessage(message string) func(w http.ResponseWriter, req *http.Request) { return func(w http.ResponseWriter, req *http.Request) { - c := apps.CallRequest{} - json.NewDecoder(req.Body).Decode(&c) + creq := apps.CallRequest{} + json.NewDecoder(req.Body).Decode(&creq) - _, err := appclient.AsBot(c.Context).DM(c.Context.ActingUser.Id, message) + _, err := appclient.AsBot(creq.Context).DM(creq.Context.ActingUser.Id, message) if err != nil { json.NewEncoder(w).Encode(apps.NewErrorResponse(err)) return diff --git a/examples/go/hello-serverless/function/handler.go b/examples/go/hello-serverless/function/handler.go index a4cab47c3..f4a523612 100644 --- a/examples/go/hello-serverless/function/handler.go +++ b/examples/go/hello-serverless/function/handler.go @@ -21,9 +21,9 @@ var DeployType apps.DeployType // // `golang-middleware` template makes use of `http.DefaultServeMux`, so we just // need to add our handlers and serve, like we do in AWS or HTTP deployments. -func Handle(w http.ResponseWriter, r *http.Request) { +func Handle(w http.ResponseWriter, req *http.Request) { DeployType = apps.DeployOpenFAAS - http.DefaultServeMux.ServeHTTP(w, r) + http.DefaultServeMux.ServeHTTP(w, req) } // Init sets up the app's HTTp server, which is exactly the same for all of the diff --git a/examples/go/hello-serverless/openfaas/template/golang-middleware/function/handler.go b/examples/go/hello-serverless/openfaas/template/golang-middleware/function/handler.go index c52b94704..df202c1bb 100644 --- a/examples/go/hello-serverless/openfaas/template/golang-middleware/function/handler.go +++ b/examples/go/hello-serverless/openfaas/template/golang-middleware/function/handler.go @@ -6,13 +6,13 @@ import ( "net/http" ) -func Handle(w http.ResponseWriter, r *http.Request) { +func Handle(w http.ResponseWriter, req *http.Request) { var input []byte - if r.Body != nil { - defer r.Body.Close() + if req.Body != nil { + defer req.Body.Close() - body, _ := ioutil.ReadAll(r.Body) + body, _ := ioutil.ReadAll(req.Body) input = body } diff --git a/server/appservices/kv.go b/server/appservices/kv.go index 97755a2bb..ad7f9b2e8 100644 --- a/server/appservices/kv.go +++ b/server/appservices/kv.go @@ -10,18 +10,18 @@ import ( "github.com/mattermost/mattermost-plugin-apps/utils" ) -func (a *AppServices) KVSet(r *incoming.Request, appID apps.AppID, actingUserID, prefix, id string, data []byte) (bool, error) { +func (a *AppServices) KVSet(_ *incoming.Request, appID apps.AppID, actingUserID, prefix, id string, data []byte) (bool, error) { if !json.Valid(data) { return false, utils.NewInvalidError("payload is no valid json") } - return a.store.AppKV.Set(r, appID, actingUserID, prefix, id, data) + return a.store.AppKV.Set(appID, actingUserID, prefix, id, data) } // KVGet returns the stored KV data for a given user and app. // If err != nil, the returned data is always valid JSON. -func (a *AppServices) KVGet(r *incoming.Request, appID apps.AppID, actingUserID, prefix, id string) ([]byte, error) { - data, err := a.store.AppKV.Get(r, appID, actingUserID, prefix, id) +func (a *AppServices) KVGet(_ *incoming.Request, appID apps.AppID, actingUserID, prefix, id string) ([]byte, error) { + data, err := a.store.AppKV.Get(appID, actingUserID, prefix, id) if err != nil && !errors.Is(err, utils.ErrNotFound) { return nil, err } @@ -34,8 +34,8 @@ func (a *AppServices) KVGet(r *incoming.Request, appID apps.AppID, actingUserID, return data, nil } -func (a *AppServices) KVDelete(r *incoming.Request, appID apps.AppID, actingUserID, prefix, id string) error { - return a.store.AppKV.Delete(r, appID, actingUserID, prefix, id) +func (a *AppServices) KVDelete(_ *incoming.Request, appID apps.AppID, actingUserID, prefix, id string) error { + return a.store.AppKV.Delete(appID, actingUserID, prefix, id) } func (a *AppServices) KVList(r *incoming.Request, appID apps.AppID, actingUserID, prefix string, processf func(key string) error) error { diff --git a/server/appservices/oauth2.go b/server/appservices/oauth2.go index ac20a8947..d410de2ae 100644 --- a/server/appservices/oauth2.go +++ b/server/appservices/oauth2.go @@ -21,7 +21,7 @@ func (a *AppServices) StoreOAuth2App(r *incoming.Request, appID apps.AppID, acti return err } - app, err := a.store.App.Get(r, appID) + app, err := a.store.App.Get(appID) if err != nil { return err } @@ -50,7 +50,7 @@ func (a *AppServices) StoreOAuth2User(r *incoming.Request, appID apps.AppID, act return utils.NewInvalidError("payload is no valid json") } - app, err := a.store.App.Get(r, appID) + app, err := a.store.App.Get(appID) if err != nil { return err } @@ -62,7 +62,7 @@ func (a *AppServices) StoreOAuth2User(r *incoming.Request, appID apps.AppID, act return err } - oldData, err := a.store.OAuth2.GetUser(r, appID, actingUserID) + oldData, err := a.store.OAuth2.GetUser(appID, actingUserID) if err != nil { return err } @@ -70,7 +70,7 @@ func (a *AppServices) StoreOAuth2User(r *incoming.Request, appID apps.AppID, act return nil } - err = a.store.OAuth2.SaveUser(r, appID, actingUserID, data) + err = a.store.OAuth2.SaveUser(appID, actingUserID, data) if err != nil { return err } @@ -81,8 +81,8 @@ func (a *AppServices) StoreOAuth2User(r *incoming.Request, appID apps.AppID, act // GetOAuth2User returns the stored OAuth2 user data for a given user and app. // If err != nil, the returned data is always valid JSON. -func (a *AppServices) GetOAuth2User(r *incoming.Request, appID apps.AppID, actingUserID string) ([]byte, error) { - app, err := a.store.App.Get(r, appID) +func (a *AppServices) GetOAuth2User(_ *incoming.Request, appID apps.AppID, actingUserID string) ([]byte, error) { + app, err := a.store.App.Get(appID) if err != nil { return nil, err } @@ -94,7 +94,7 @@ func (a *AppServices) GetOAuth2User(r *incoming.Request, appID apps.AppID, actin return nil, err } - data, err := a.store.OAuth2.GetUser(r, appID, actingUserID) + data, err := a.store.OAuth2.GetUser(appID, actingUserID) if err != nil && !errors.Is(err, utils.ErrNotFound) { return nil, err } diff --git a/server/appservices/service.go b/server/appservices/service.go index 0e21408c2..86c5d074e 100644 --- a/server/appservices/service.go +++ b/server/appservices/service.go @@ -21,22 +21,22 @@ var ErrIsABot = errors.New("is a bot") type Service interface { // Subscriptions - Subscribe(r *incoming.Request, sub apps.Subscription) error - GetSubscriptions(r *incoming.Request, appID apps.AppID, actingUserID string) ([]apps.Subscription, error) - Unsubscribe(r *incoming.Request, sub apps.Subscription) error + Subscribe(*incoming.Request, apps.Subscription) error + GetSubscriptions(_ *incoming.Request, _ apps.AppID, actingUserID string) ([]apps.Subscription, error) + Unsubscribe(*incoming.Request, apps.Subscription) error // KV - KVSet(r *incoming.Request, appID apps.AppID, actingUserID, prefix, id string, data []byte) (bool, error) - KVGet(r *incoming.Request, appID apps.AppID, actingUserID, prefix, id string) ([]byte, error) - KVDelete(r *incoming.Request, appID apps.AppID, actingUserID, prefix, id string) error - KVList(r *incoming.Request, appID apps.AppID, actingUserID, namespace string, processf func(key string) error) error + KVSet(_ *incoming.Request, _ apps.AppID, actingUserID, prefix, id string, data []byte) (bool, error) + KVGet(_ *incoming.Request, _ apps.AppID, actingUserID, prefix, id string) ([]byte, error) + KVDelete(_ *incoming.Request, _ apps.AppID, actingUserID, prefix, id string) error + KVList(_ *incoming.Request, _ apps.AppID, actingUserID, namespace string, processf func(key string) error) error // Remote (3rd party) OAuth2 - StoreOAuth2App(r *incoming.Request, appID apps.AppID, actingUserID string, data []byte) error - StoreOAuth2User(r *incoming.Request, AppID apps.AppID, actingUserID string, data []byte) error - GetOAuth2User(r *incoming.Request, appID apps.AppID, actingUserID string) ([]byte, error) + StoreOAuth2App(_ *incoming.Request, _ apps.AppID, actingUserID string, data []byte) error + StoreOAuth2User(_ *incoming.Request, _ apps.AppID, actingUserID string, data []byte) error + GetOAuth2User(_ *incoming.Request, _ apps.AppID, actingUserID string) ([]byte, error) } type AppServices struct { diff --git a/server/appservices/subscriptions.go b/server/appservices/subscriptions.go index ef191fa6f..eb37ec949 100644 --- a/server/appservices/subscriptions.go +++ b/server/appservices/subscriptions.go @@ -66,7 +66,7 @@ func CheckSubscriptionPermission(checker PermissionChecker, sub apps.Subscriptio return nil } -func (a *AppServices) Subscribe(r *incoming.Request, sub apps.Subscription) error { +func (a *AppServices) Subscribe(_ *incoming.Request, sub apps.Subscription) error { if err := sub.Validate(); err != nil { return utils.NewInvalidError("invalid subscription") } @@ -75,17 +75,17 @@ func (a *AppServices) Subscribe(r *incoming.Request, sub apps.Subscription) erro return err } - return a.store.Subscription.Save(r, sub) + return a.store.Subscription.Save(sub) } -func (a *AppServices) GetSubscriptions(r *incoming.Request, appID apps.AppID, userID string) ([]apps.Subscription, error) { - return a.store.Subscription.ListByUserID(r, appID, userID) +func (a *AppServices) GetSubscriptions(_ *incoming.Request, appID apps.AppID, userID string) ([]apps.Subscription, error) { + return a.store.Subscription.ListByUserID(appID, userID) } -func (a *AppServices) Unsubscribe(r *incoming.Request, sub apps.Subscription) error { +func (a *AppServices) Unsubscribe(_ *incoming.Request, sub apps.Subscription) error { if err := sub.Validate(); err != nil { return utils.NewInvalidError("invalid subscription") } - return a.store.Subscription.Delete(r, sub) + return a.store.Subscription.Delete(sub) } diff --git a/server/builtin/app.go b/server/builtin/app.go index b9be0a4b2..df9425a1e 100644 --- a/server/builtin/app.go +++ b/server/builtin/app.go @@ -186,8 +186,7 @@ func (a *builtinApp) Roundtrip(ctx context.Context, _ apps.App, creq apps.CallRe stack := string(debug.Stack()) txt := "Call `" + creq.Path + "` panic-ed." log = log.With( - "path", creq.Path, - "values", creq.Values, + creq, "error", x, "stack", stack, ) @@ -195,7 +194,7 @@ func (a *builtinApp) Roundtrip(ctx context.Context, _ apps.App, creq apps.CallRe txt = "Command `" + creq.RawCommand + "` panic-ed." log.Errorw("Recovered from a panic in a command", "command", creq.RawCommand) } else { - log.Errorw("Recovered from a panic in a Call") + log.Errorf("Recovered from a panic in a Call") } if a.conf.Get().DeveloperMode { diff --git a/server/builtin/debug_clean.go b/server/builtin/debug_clean.go index 826ec8a92..b0bb66475 100644 --- a/server/builtin/debug_clean.go +++ b/server/builtin/debug_clean.go @@ -26,12 +26,25 @@ func (a *builtinApp) debugCleanCommandBinding(loc *i18n.Localizer) apps.Binding } } -func (a *builtinApp) debugClean(_ *incoming.Request, creq apps.CallRequest) apps.CallResponse { +func (a *builtinApp) debugClean(r *incoming.Request, creq apps.CallRequest) apps.CallResponse { loc := a.newLocalizer(creq) - _ = a.conf.MattermostAPI().KV.DeleteAll() - _ = a.conf.StoreConfig(config.StoredConfig{}) - return apps.NewTextResponse(a.conf.I18N().LocalizeDefaultMessage(loc, &i18n.Message{ - ID: "command.debug.clean.submit", - Other: "Deleted all KV records and emptied the config.", - })) + err := a.conf.MattermostAPI().KV.DeleteAll() + if err != nil { + return apps.NewErrorResponse(err) + } + done := "- " + a.conf.I18N().LocalizeDefaultMessage(loc, &i18n.Message{ + ID: "command.debug.clean.submit.kv", + Other: "Deleted all KV records.", + }) + "\n" + + err = a.conf.StoreConfig(config.StoredConfig{}, r.Log) + if err != nil { + return apps.NewErrorResponse(err) + } + done += "- " + a.conf.I18N().LocalizeDefaultMessage(loc, &i18n.Message{ + ID: "command.debug.clean.submit.config", + Other: "Emptied the config.", + }) + "\n" + + return apps.NewTextResponse(done) } diff --git a/server/builtin/debug_session_view.go b/server/builtin/debug_session_view.go index f50ebc6dd..3de979fab 100644 --- a/server/builtin/debug_session_view.go +++ b/server/builtin/debug_session_view.go @@ -48,7 +48,7 @@ func (a *builtinApp) debugSessionsViewBinding(loc *i18n.Localizer) apps.Binding } } -func (a *builtinApp) debugSessionsView(r *incoming.Request, creq apps.CallRequest) apps.CallResponse { +func (a *builtinApp) debugSessionsView(_ *incoming.Request, creq apps.CallRequest) apps.CallResponse { sessionID := creq.GetValue(fSessionID, "") session, err := a.conf.MattermostAPI().Session.Get(sessionID) if err != nil { diff --git a/server/config/config.go b/server/config/config.go index 885cfab6c..689bf8bde 100644 --- a/server/config/config.go +++ b/server/config/config.go @@ -93,7 +93,7 @@ var allowHTTPAppsDomains = regexp.MustCompile("^" + strings.Join([]string{ `community-[a-z]+\.mattermost\.com`, }, "|") + "$") -func (conf *Config) Update(stored StoredConfig, mmconf *model.Config, license *model.License, log utils.Logger) error { +func (conf *Config) update(stored StoredConfig, mmconf *model.Config, license *model.License, log utils.Logger) error { mattermostSiteURL := mmconf.ServiceSettings.SiteURL if mattermostSiteURL == nil { return errors.New("plugin requires Mattermost Site URL to be set") @@ -195,3 +195,14 @@ func (conf *Config) InfoTemplateData() map[string]string { "AllowHTTPApps": fmt.Sprintf("%t", conf.AllowHTTPApps), } } + +func (conf Config) Loggable() []interface{} { + return append([]interface{}{}, + "version", conf.PluginManifest.Version, + "commit", conf.BuildHashShort, + "build_date", conf.BuildDate, + "cloud_mode", fmt.Sprintf("%t", conf.MattermostCloudMode), + "developer_mode", fmt.Sprintf("%t", conf.DeveloperMode), + "allow_http_apps", fmt.Sprintf("%t", conf.AllowHTTPApps), + ) +} diff --git a/server/config/log_string_test.go b/server/config/log_string_test.go new file mode 100644 index 000000000..20cb88889 --- /dev/null +++ b/server/config/log_string_test.go @@ -0,0 +1,54 @@ +// Copyright (c) 2020-present Mattermost, Inc. All Rights Reserved. +// See License for license information. + +package config + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/mattermost/mattermost-server/v6/model" + + "github.com/mattermost/mattermost-plugin-apps/utils" +) + +func TestLoggable(t *testing.T) { + var _ utils.HasLoggable = Config{} + + var simpleConfig = Config{ + PluginManifest: model.Manifest{ + Version: "v1.2.3", + }, + BuildHashShort: "1234567", + BuildDate: "date-is-just-a-string", + MattermostCloudMode: true, + DeveloperMode: true, + AllowHTTPApps: true, + } + + for name, test := range map[string]struct { + In interface{} + ExpectedProps []interface{} + }{ + "simple Config": { + In: simpleConfig, + ExpectedProps: []interface{}{ + "version", "v1.2.3", + "commit", "1234567", + "build_date", "date-is-just-a-string", + "cloud_mode", "true", + "developer_mode", "true", + "allow_http_apps", "true", + }, + }, + } { + t.Run(name, func(t *testing.T) { + if test.ExpectedProps != nil { + lp, ok := test.In.(utils.HasLoggable) + require.True(t, ok) + require.EqualValues(t, test.ExpectedProps, lp.Loggable()) + } + }) + } +} diff --git a/server/config/service.go b/server/config/service.go index 8eb222bc2..e96fba230 100644 --- a/server/config/service.go +++ b/server/config/service.go @@ -27,8 +27,8 @@ type Service interface { I18N() *i18n.Bundle Telemetry() *telemetry.Telemetry - Reconfigure(StoredConfig, ...Configurable) error - StoreConfig(sc StoredConfig) error + Reconfigure(StoredConfig, utils.Logger, ...Configurable) error + StoreConfig(StoredConfig, utils.Logger) error } var _ Service = (*service)(nil) @@ -36,7 +36,6 @@ var _ Service = (*service)(nil) type service struct { pluginManifest model.Manifest botUserID string - log utils.Logger mm *pluginapi.Client i18n *i18n.Bundle telemetry *telemetry.Telemetry @@ -50,7 +49,6 @@ func NewService(mm *pluginapi.Client, pliginManifest model.Manifest, botUserID s return &service{ pluginManifest: pliginManifest, botUserID: botUserID, - log: utils.NewPluginLogger(mm), mm: mm, lock: &sync.RWMutex{}, i18n: i18nBundle, @@ -110,7 +108,7 @@ func (s *service) reloadMattermostConfig() *model.Config { return mmconf } -func (s *service) Reconfigure(stored StoredConfig, services ...Configurable) error { +func (s *service) Reconfigure(stored StoredConfig, log utils.Logger, services ...Configurable) error { mmconf := s.reloadMattermostConfig() newConfig := s.Get() @@ -121,11 +119,11 @@ func (s *service) Reconfigure(stored StoredConfig, services ...Configurable) err if license == nil { license = s.mm.System.GetLicense() if license == nil { - s.log.Infof("Failed to fetch license twice. May incorrectly default to on-prem mode.") + log.Debugf("failed to fetch license twice. May incorrectly default to on-prem mode") } } - err := newConfig.Update(stored, mmconf, license, s.log) + err := newConfig.update(stored, mmconf, license, log) if err != nil { return err } @@ -134,25 +132,27 @@ func (s *service) Reconfigure(stored StoredConfig, services ...Configurable) err s.conf = &newConfig s.lock.Unlock() - log := s.log - for _, s := range services { - err = s.Configure(newConfig, log) + for _, service := range services { + err = service.Configure(newConfig, log) if err != nil { - return errors.Wrapf(err, "error configuring %T", s) + return errors.Wrapf(err, "failed to configure service %T", service) } } return nil } -func (s *service) StoreConfig(sc StoredConfig) error { +func (s *service) StoreConfig(stored StoredConfig, log utils.Logger) error { + log.Debugf("Storing configuration, %v installed , %v listed apps", + len(stored.InstalledApps), len(stored.LocalManifests)) + // Refresh computed values immediately, do not wait for OnConfigurationChanged - err := s.Reconfigure(sc) + err := s.Reconfigure(stored, log) if err != nil { return err } - data, err := json.Marshal(sc) + data, err := json.Marshal(stored) if err != nil { return err } diff --git a/server/config/test_service.go b/server/config/test_service.go index 77079572b..4a744a021 100644 --- a/server/config/test_service.go +++ b/server/config/test_service.go @@ -82,11 +82,11 @@ func (s *TestService) MattermostConfig() configservice.ConfigService { return &mattermostConfigService{&s.mmconfig} } -func (s *TestService) Reconfigure(StoredConfig, ...Configurable) error { +func (s *TestService) Reconfigure(StoredConfig, utils.Logger, ...Configurable) error { return nil } -func (s *TestService) StoreConfig(sc StoredConfig) error { +func (s *TestService) StoreConfig(sc StoredConfig, _ utils.Logger) error { s.config.StoredConfig = sc return nil } diff --git a/server/httpin/gateway/gateway.go b/server/httpin/gateway/gateway.go index a557b981f..620a59904 100644 --- a/server/httpin/gateway/gateway.go +++ b/server/httpin/gateway/gateway.go @@ -43,8 +43,8 @@ func Init(h *httpin.Handler, config config.Service, p proxy.Service, _ appservic g.remoteOAuth2Complete, httpin.RequireUser).Methods(http.MethodGet) } -func appIDVar(r *http.Request) apps.AppID { - s, ok := mux.Vars(r)["appid"] +func appIDVar(req *http.Request) apps.AppID { + s, ok := mux.Vars(req)["appid"] if ok { return apps.AppID(s) } diff --git a/server/httpin/gateway/oauth2.go b/server/httpin/gateway/oauth2.go index d51bd8551..f2fd6ef05 100644 --- a/server/httpin/gateway/oauth2.go +++ b/server/httpin/gateway/oauth2.go @@ -8,41 +8,41 @@ import ( "github.com/mattermost/mattermost-plugin-apps/utils/httputils" ) -func (g *gateway) remoteOAuth2Connect(req *incoming.Request, w http.ResponseWriter, r *http.Request) { - appID := appIDVar(r) +func (g *gateway) remoteOAuth2Connect(r *incoming.Request, w http.ResponseWriter, req *http.Request) { + appID := appIDVar(req) if appID == "" { httputils.WriteError(w, utils.NewInvalidError("app_id not specified")) return } - req.SetAppID(appID) + r.SetAppID(appID) - connectURL, err := g.proxy.GetRemoteOAuth2ConnectURL(req, appID) + connectURL, err := g.proxy.GetRemoteOAuth2ConnectURL(r, appID) if err != nil { - req.Log.WithError(err).Warnf("Failed to get remote OAuth2 connect URL") + r.Log.WithError(err).Warnf("Failed to get remote OAuth2 connect URL") httputils.WriteError(w, err) return } - http.Redirect(w, r, connectURL, http.StatusTemporaryRedirect) + http.Redirect(w, req, connectURL, http.StatusTemporaryRedirect) } -func (g *gateway) remoteOAuth2Complete(req *incoming.Request, w http.ResponseWriter, r *http.Request) { - appID := appIDVar(r) +func (g *gateway) remoteOAuth2Complete(r *incoming.Request, w http.ResponseWriter, req *http.Request) { + appID := appIDVar(req) if appID == "" { httputils.WriteError(w, utils.NewInvalidError("app_id not specified")) return } - req.SetAppID(appID) + r.SetAppID(appID) - q := r.URL.Query() + q := req.URL.Query() urlValues := map[string]interface{}{} for key := range q { urlValues[key] = q.Get(key) } - err := g.proxy.CompleteRemoteOAuth2(req, appID, urlValues) + err := g.proxy.CompleteRemoteOAuth2(r, appID, urlValues) if err != nil { - req.Log.WithError(err).Warnf("Failed to complete remote OAuth2") + r.Log.WithError(err).Warnf("Failed to complete remote OAuth2") httputils.WriteError(w, err) return } diff --git a/server/httpin/gateway/static.go b/server/httpin/gateway/static.go index ef0cbd30e..72b914a94 100644 --- a/server/httpin/gateway/static.go +++ b/server/httpin/gateway/static.go @@ -11,15 +11,15 @@ import ( "github.com/mattermost/mattermost-plugin-apps/utils/httputils" ) -func (g *gateway) static(req *incoming.Request, w http.ResponseWriter, r *http.Request) { - appID := appIDVar(r) +func (g *gateway) static(r *incoming.Request, w http.ResponseWriter, req *http.Request) { + appID := appIDVar(req) if appID == "" { httputils.WriteError(w, utils.NewInvalidError("app_id not specified")) return } - req.SetAppID(appID) + r.SetAppID(appID) - vars := mux.Vars(r) + vars := mux.Vars(req) if len(vars) == 0 { httputils.WriteError(w, utils.NewInvalidError("invalid URL format")) return @@ -32,14 +32,14 @@ func (g *gateway) static(req *incoming.Request, w http.ResponseWriter, r *http.R // TODO verify that request is from the correct app - body, status, err := g.proxy.GetStatic(req, appID, assetName) + body, status, err := g.proxy.GetStatic(r, appID, assetName) if err != nil { - req.Log.WithError(err).Debugw("Failed to get asset", "asset_name", assetName) + r.Log.WithError(err).Debugw("failed to get asset", "asset_name", assetName) httputils.WriteError(w, err) return } - copyHeader(w.Header(), r.Header) + copyHeader(w.Header(), req.Header) w.WriteHeader(status) if _, err := io.Copy(w, body); err != nil { httputils.WriteError(w, err) diff --git a/server/httpin/gateway/webhook.go b/server/httpin/gateway/webhook.go index ff692a1f8..43f76d3b6 100644 --- a/server/httpin/gateway/webhook.go +++ b/server/httpin/gateway/webhook.go @@ -11,52 +11,52 @@ import ( "github.com/mattermost/mattermost-plugin-apps/utils/httputils" ) -func (g *gateway) handleWebhook(req *incoming.Request, w http.ResponseWriter, r *http.Request) { - err := g.doHandleWebhook(req, w, r) +func (g *gateway) handleWebhook(r *incoming.Request, w http.ResponseWriter, req *http.Request) { + err := g.doHandleWebhook(r, w, req) if err != nil { - req.Log.WithError(err).Warnw("failed to process remote webhook") + r.Log.WithError(err).Warnw("failed to process remote webhook") httputils.WriteError(w, err) } } -func (g *gateway) doHandleWebhook(req *incoming.Request, _ http.ResponseWriter, r *http.Request) error { - appID := appIDVar(r) +func (g *gateway) doHandleWebhook(r *incoming.Request, _ http.ResponseWriter, req *http.Request) error { + appID := appIDVar(req) if appID == "" { return utils.NewInvalidError("app_id not specified") } - req.SetAppID(appID) + r.SetAppID(appID) - sreq, err := newHTTPCallRequest(r, g.conf.Get().MaxWebhookSize) + sreq, err := newHTTPCallRequest(req, g.conf.Get().MaxWebhookSize) if err != nil { return err } - sreq.Path = mux.Vars(r)["path"] - req.Log = req.Log.With("call_path", sreq.Path) + sreq.Path = mux.Vars(req)["path"] + r.Log = r.Log.With("call_path", sreq.Path) - err = g.proxy.NotifyRemoteWebhook(req, appID, *sreq) + err = g.proxy.NotifyRemoteWebhook(r, appID, *sreq) if err != nil { return err } - req.Log.Debugf("processed remote webhook") + r.Log.Debugf("processed remote webhook") return nil } -func newHTTPCallRequest(r *http.Request, limit int64) (*apps.HTTPCallRequest, error) { - data, err := httputils.LimitReadAll(r.Body, limit) +func newHTTPCallRequest(req *http.Request, limit int64) (*apps.HTTPCallRequest, error) { + data, err := httputils.LimitReadAll(req.Body, limit) if err != nil { return nil, err } sreq := apps.HTTPCallRequest{ - HTTPMethod: r.Method, - Path: r.URL.Path, - RawQuery: r.URL.RawQuery, + HTTPMethod: req.Method, + Path: req.URL.Path, + RawQuery: req.URL.RawQuery, Body: string(data), Headers: map[string]string{}, } - for key := range r.Header { - sreq.Headers[key] = r.Header.Get(key) + for key := range req.Header { + sreq.Headers[key] = req.Header.Get(key) } return &sreq, nil diff --git a/server/httpin/handler.go b/server/httpin/handler.go index 4c6e85dc8..059b238e7 100644 --- a/server/httpin/handler.go +++ b/server/httpin/handler.go @@ -18,14 +18,14 @@ import ( "github.com/mattermost/mattermost-plugin-apps/utils/sessionutils" ) -type check func(req *incoming.Request, mm *pluginapi.Client, w http.ResponseWriter, r *http.Request) bool // check return true if the it was successful +type check func(*incoming.Request, *pluginapi.Client, http.ResponseWriter, *http.Request) bool // check return true if the it was successful -type handlerFunc func(req *incoming.Request, w http.ResponseWriter, r *http.Request) +type handlerFunc func(*incoming.Request, http.ResponseWriter, *http.Request) type Handler struct { mm *pluginapi.Client config config.Service - log utils.Logger + baseLog utils.Logger sessionService incoming.SessionService router *mux.Router @@ -33,11 +33,11 @@ type Handler struct { checks []check } -func NewHandler(mm *pluginapi.Client, config config.Service, log utils.Logger, session incoming.SessionService, router *mux.Router) *Handler { +func NewHandler(mm *pluginapi.Client, config config.Service, baseLog utils.Logger, session incoming.SessionService, router *mux.Router) *Handler { rh := &Handler{ mm: mm, config: config, - log: log, + baseLog: baseLog, sessionService: session, router: router, } @@ -50,7 +50,7 @@ func (h *Handler) clone() *Handler { return &Handler{ mm: h.mm, config: h.config, - log: h.log, + baseLog: h.baseLog, sessionService: h.sessionService, router: h.router, @@ -77,22 +77,23 @@ func (h *Handler) HandleFunc(path string, handlerFunc handlerFunc, checks ...che return clone.router.Handle(path, clone) } -func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { - ctx, cancel := context.WithTimeout(r.Context(), config.RequestTimeout) +func (h *Handler) ServeHTTP(w http.ResponseWriter, req *http.Request) { + ctx, cancel := context.WithTimeout(req.Context(), config.RequestTimeout) defer cancel() - req := incoming.NewRequest(h.mm, h.config, h.log, h.sessionService, incoming.WithCtx(ctx)) + r := incoming.NewRequest(h.mm, h.config, h.baseLog, h.sessionService, incoming.WithCtx(ctx)) - req.Log = req.Log.With( - "path", r.URL.Path, + // TODO: what else to add? 1/5 add clones of CallResponse to Request and log it. + r.Log = r.Log.With( + "path", req.URL.Path, ) defer func() { if x := recover(); x != nil { stack := string(debug.Stack()) - req.Log.Errorw( + r.Log.Errorw( "Recovered from a panic in an HTTP handler", - "url", r.URL.String(), + "url", req.URL.String(), "error", x, "stack", string(debug.Stack()), ) @@ -110,37 +111,41 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { }() for _, check := range h.checks { - succeeded := check(req, h.mm, w, r) + succeeded := check(r, h.mm, w, req) if !succeeded { return } } - h.handlerFunc(req, w, r) + h.handlerFunc(r, w, req) + + if h.config.Get().DeveloperMode { + r.Log.Debugf("HTTP") + } } -func getUserID(r *http.Request) string { - return r.Header.Get(config.MattermostUserIDHeader) +func getUserID(req *http.Request) string { + return req.Header.Get(config.MattermostUserIDHeader) } -func RequireUser(req *incoming.Request, mm *pluginapi.Client, w http.ResponseWriter, r *http.Request) bool { - actingUserID := getUserID(r) +func RequireUser(r *incoming.Request, mm *pluginapi.Client, w http.ResponseWriter, req *http.Request) bool { + actingUserID := getUserID(req) if actingUserID == "" { httputils.WriteError(w, utils.NewUnauthorizedError("user ID is required")) return false } - req.SetActingUserID(actingUserID) + r.SetActingUserID(actingUserID) return true } -func RequireSysadmin(req *incoming.Request, mm *pluginapi.Client, w http.ResponseWriter, r *http.Request) bool { - if successful := RequireUser(req, mm, w, r); !successful { +func RequireSysadmin(r *incoming.Request, mm *pluginapi.Client, w http.ResponseWriter, req *http.Request) bool { + if successful := RequireUser(r, mm, w, req); !successful { return false } - if !mm.User.HasPermissionTo(req.ActingUserID(), model.PermissionManageSystem) { + if !mm.User.HasPermissionTo(r.ActingUserID(), model.PermissionManageSystem) { httputils.WriteError(w, utils.NewUnauthorizedError("user is not a system admin")) return false } @@ -148,17 +153,17 @@ func RequireSysadmin(req *incoming.Request, mm *pluginapi.Client, w http.Respons return true } -func RequireSysadminOrPlugin(req *incoming.Request, mm *pluginapi.Client, w http.ResponseWriter, r *http.Request) bool { - pluginID := r.Header.Get(config.MattermostPluginIDHeader) +func RequireSysadminOrPlugin(r *incoming.Request, mm *pluginapi.Client, w http.ResponseWriter, req *http.Request) bool { + pluginID := req.Header.Get(config.MattermostPluginIDHeader) if pluginID != "" { return true } - return RequireSysadmin(req, mm, w, r) + return RequireSysadmin(r, mm, w, req) } -func RequireApp(req *incoming.Request, mm *pluginapi.Client, w http.ResponseWriter, r *http.Request) bool { - sessionID := r.Header.Get(config.MattermostSessionIDHeader) +func RequireApp(r *incoming.Request, mm *pluginapi.Client, w http.ResponseWriter, req *http.Request) bool { + sessionID := req.Header.Get(config.MattermostSessionIDHeader) if sessionID == "" { httputils.WriteError(w, utils.NewUnauthorizedError("a session is required")) return false @@ -176,7 +181,7 @@ func RequireApp(req *incoming.Request, mm *pluginapi.Client, w http.ResponseWrit return false } - req.SetAppID(appID) + r.SetAppID(appID) return true } diff --git a/server/httpin/restapi/admin.go b/server/httpin/restapi/admin.go index af47e904b..ab4e6e5e2 100644 --- a/server/httpin/restapi/admin.go +++ b/server/httpin/restapi/admin.go @@ -43,14 +43,14 @@ func (a *restapi) initAdmin(h *httpin.Handler) { // "add_deploys": []string e.g. ["aws_lambda","http"] // "remove_deploys": []string e.g. ["aws_lambda","http"] // Output: The updated listing manifest -func (a *restapi) UpdateAppListing(req *incoming.Request, w http.ResponseWriter, r *http.Request) { +func (a *restapi) UpdateAppListing(r *incoming.Request, w http.ResponseWriter, req *http.Request) { listReq := appclient.UpdateAppListingRequest{} - err := json.NewDecoder(r.Body).Decode(&listReq) + err := json.NewDecoder(req.Body).Decode(&listReq) if err != nil { httputils.WriteError(w, utils.NewInvalidError(err, "failed to unmarshal input")) return } - m, err := a.proxy.UpdateAppListing(req, listReq) + m, err := a.proxy.UpdateAppListing(r, listReq) if err != nil { httputils.WriteError(w, err) return @@ -65,17 +65,17 @@ func (a *restapi) UpdateAppListing(req *incoming.Request, w http.ResponseWriter, // Method: POST // Input: JSON {app_id, deploy_type} // Output: None -func (a *restapi) InstallApp(req *incoming.Request, w http.ResponseWriter, r *http.Request) { +func (a *restapi) InstallApp(r *incoming.Request, w http.ResponseWriter, req *http.Request) { var input apps.App - err := json.NewDecoder(r.Body).Decode(&input) + err := json.NewDecoder(req.Body).Decode(&input) if err != nil { httputils.WriteError(w, errors.Wrap(err, "failed to unmarshal input")) return } - req.SetAppID(input.AppID) + r.SetAppID(input.AppID) - _, _, err = a.proxy.InstallApp(req, apps.Context{}, input.AppID, input.DeployType, false, "") + _, _, err = a.proxy.InstallApp(r, apps.Context{}, input.AppID, input.DeployType, false, "") if err != nil { httputils.WriteError(w, err) return @@ -87,17 +87,17 @@ func (a *restapi) InstallApp(req *incoming.Request, w http.ResponseWriter, r *ht // Method: POST // Input: JSON {app_id} // Output: None -func (a *restapi) EnableApp(req *incoming.Request, w http.ResponseWriter, r *http.Request) { +func (a *restapi) EnableApp(r *incoming.Request, w http.ResponseWriter, req *http.Request) { var input apps.App - err := json.NewDecoder(r.Body).Decode(&input) + err := json.NewDecoder(req.Body).Decode(&input) if err != nil { httputils.WriteError(w, errors.Wrap(err, "failed to unmarshal input")) return } - req.SetAppID(input.AppID) + r.SetAppID(input.AppID) - _, err = a.proxy.EnableApp(req, apps.Context{}, input.AppID) + _, err = a.proxy.EnableApp(r, apps.Context{}, input.AppID) if err != nil { httputils.WriteError(w, err) return @@ -109,17 +109,17 @@ func (a *restapi) EnableApp(req *incoming.Request, w http.ResponseWriter, r *htt // Method: POST // Input: JSON {app_id} // Output: None -func (a *restapi) DisableApp(req *incoming.Request, w http.ResponseWriter, r *http.Request) { +func (a *restapi) DisableApp(r *incoming.Request, w http.ResponseWriter, req *http.Request) { var input apps.App - err := json.NewDecoder(r.Body).Decode(&input) + err := json.NewDecoder(req.Body).Decode(&input) if err != nil { httputils.WriteError(w, errors.Wrap(err, "failed to unmarshal input")) return } - req.SetAppID(input.AppID) + r.SetAppID(input.AppID) - _, err = a.proxy.DisableApp(req, apps.Context{}, input.AppID) + _, err = a.proxy.DisableApp(r, apps.Context{}, input.AppID) if err != nil { httputils.WriteError(w, err) return @@ -131,17 +131,17 @@ func (a *restapi) DisableApp(req *incoming.Request, w http.ResponseWriter, r *ht // Method: POST // Input: JSON {app_id} // Output: None -func (a *restapi) UninstallApp(req *incoming.Request, w http.ResponseWriter, r *http.Request) { +func (a *restapi) UninstallApp(r *incoming.Request, w http.ResponseWriter, req *http.Request) { var input apps.App - err := json.NewDecoder(r.Body).Decode(&input) + err := json.NewDecoder(req.Body).Decode(&input) if err != nil { httputils.WriteError(w, errors.Wrap(err, "failed to unmarshal input")) return } - req.SetAppID(input.AppID) + r.SetAppID(input.AppID) - _, err = a.proxy.UninstallApp(req, apps.Context{}, input.AppID) + _, err = a.proxy.UninstallApp(r, apps.Context{}, input.AppID) if err != nil { httputils.WriteError(w, err) return @@ -160,15 +160,15 @@ func (a *restapi) initGetApp(h *httpin.Handler) { // Method: GET // Input: none // Output: App -func (a *restapi) GetApp(req *incoming.Request, w http.ResponseWriter, r *http.Request) { - appID := appIDVar(r) +func (a *restapi) GetApp(r *incoming.Request, w http.ResponseWriter, req *http.Request) { + appID := appIDVar(req) if appID == "" { httputils.WriteError(w, utils.NewInvalidError("app is required")) return } - req.SetAppID(appID) + r.SetAppID(appID) - app, err := a.proxy.GetInstalledApp(req, appID) + app, err := a.proxy.GetInstalledApp(r, appID) if err != nil { httputils.WriteError(w, err) return diff --git a/server/httpin/restapi/app_kv.go b/server/httpin/restapi/app_kv.go index 69e746f02..155e6d44e 100644 --- a/server/httpin/restapi/app_kv.go +++ b/server/httpin/restapi/app_kv.go @@ -38,10 +38,10 @@ func (a *restapi) initKV(h *httpin.Handler) { // Method: GET // Input: none // Output: a JSON object -func (a *restapi) KVGet(req *incoming.Request, w http.ResponseWriter, r *http.Request) { - id := mux.Vars(r)["key"] - prefix := mux.Vars(r)["prefix"] - data, err := a.appServices.KVGet(req, req.AppID(), req.ActingUserID(), prefix, id) +func (a *restapi) KVGet(r *incoming.Request, w http.ResponseWriter, req *http.Request) { + id := mux.Vars(req)["key"] + prefix := mux.Vars(req)["prefix"] + data, err := a.appServices.KVGet(r, r.AppID(), r.ActingUserID(), prefix, id) if err != nil { httputils.WriteError(w, err) return @@ -56,17 +56,17 @@ func (a *restapi) KVGet(req *incoming.Request, w http.ResponseWriter, r *http.Re // Output: a JSON object // Output: // changed: set to true if the key value was changed. -func (a *restapi) KVPut(req *incoming.Request, w http.ResponseWriter, r *http.Request) { - id := mux.Vars(r)["key"] - prefix := mux.Vars(r)["prefix"] +func (a *restapi) KVPut(r *incoming.Request, w http.ResponseWriter, req *http.Request) { + id := mux.Vars(req)["key"] + prefix := mux.Vars(req)["prefix"] - data, err := httputils.LimitReadAll(r.Body, MaxKVStoreValueLength) + data, err := httputils.LimitReadAll(req.Body, MaxKVStoreValueLength) if err != nil { httputils.WriteError(w, err) return } - changed, err := a.appServices.KVSet(req, req.AppID(), req.ActingUserID(), prefix, id, data) + changed, err := a.appServices.KVSet(r, r.AppID(), r.ActingUserID(), prefix, id, data) if err != nil { httputils.WriteError(w, err) return @@ -81,11 +81,11 @@ func (a *restapi) KVPut(req *incoming.Request, w http.ResponseWriter, r *http.Re // Methods: DELETE // Input: none // Output: none -func (a *restapi) KVDelete(req *incoming.Request, w http.ResponseWriter, r *http.Request) { - id := mux.Vars(r)["key"] - prefix := mux.Vars(r)["prefix"] +func (a *restapi) KVDelete(r *incoming.Request, w http.ResponseWriter, req *http.Request) { + id := mux.Vars(req)["key"] + prefix := mux.Vars(req)["prefix"] - err := a.appServices.KVDelete(req, req.AppID(), req.ActingUserID(), prefix, id) + err := a.appServices.KVDelete(r, r.AppID(), r.ActingUserID(), prefix, id) if err != nil { httputils.WriteError(w, err) return diff --git a/server/httpin/restapi/app_kv_test.go b/server/httpin/restapi/app_kv_test.go index 688d342a9..8a1dd0475 100644 --- a/server/httpin/restapi/app_kv_test.go +++ b/server/httpin/restapi/app_kv_test.go @@ -20,7 +20,6 @@ import ( "github.com/mattermost/mattermost-plugin-apps/server/appservices" "github.com/mattermost/mattermost-plugin-apps/server/config" "github.com/mattermost/mattermost-plugin-apps/server/httpin" - "github.com/mattermost/mattermost-plugin-apps/server/incoming" "github.com/mattermost/mattermost-plugin-apps/server/mocks/mock_appservices" "github.com/mattermost/mattermost-plugin-apps/server/mocks/mock_proxy" "github.com/mattermost/mattermost-plugin-apps/server/mocks/mock_session" @@ -67,9 +66,8 @@ func TestKV(t *testing.T) { req.Header.Set(config.MattermostUserIDHeader, "01234567890123456789012345") req.Header.Add(config.MattermostSessionIDHeader, "some_session_id") require.NoError(t, err) - mocked.EXPECT().Set(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( - func(r *incoming.Request, appID apps.AppID, actingUserID, prefix, id string, ref []byte) (bool, error) { - assert.NotNil(t, r) + mocked.EXPECT().Set(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( + func(appID apps.AppID, actingUserID, prefix, id string, ref []byte) (bool, error) { assert.Equal(t, apps.AppID("some_app_id"), appID) assert.Equal(t, "01234567890123456789012345", actingUserID) assert.Equal(t, "", prefix) @@ -87,9 +85,8 @@ func TestKV(t *testing.T) { req.Header.Set(config.MattermostUserIDHeader, "01234567890123456789012345") req.Header.Add(config.MattermostSessionIDHeader, "some_session_id") require.NoError(t, err) - mocked.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( - func(r *incoming.Request, appID apps.AppID, botUserID, prefix, id string) ([]byte, error) { - require.NotNil(t, r) + mocked.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( + func(appID apps.AppID, botUserID, prefix, id string) ([]byte, error) { assert.Equal(t, apps.AppID("some_app_id"), appID) require.Equal(t, "01234567890123456789012345", botUserID) require.Equal(t, "", prefix) diff --git a/server/httpin/restapi/app_oauth2_store.go b/server/httpin/restapi/app_oauth2_store.go index e31efd647..445f8256a 100644 --- a/server/httpin/restapi/app_oauth2_store.go +++ b/server/httpin/restapi/app_oauth2_store.go @@ -18,36 +18,36 @@ func (a *restapi) initOAuth2Store(h *httpin.Handler) { a.OAuth2GetUser, httpin.RequireUser, httpin.RequireApp).Methods(http.MethodGet) } -func (a *restapi) OAuth2StoreApp(req *incoming.Request, w http.ResponseWriter, r *http.Request) { - data, err := httputils.LimitReadAll(r.Body, MaxKVStoreValueLength) +func (a *restapi) OAuth2StoreApp(r *incoming.Request, w http.ResponseWriter, req *http.Request) { + data, err := httputils.LimitReadAll(req.Body, MaxKVStoreValueLength) if err != nil { httputils.WriteError(w, err) return } - err = a.appServices.StoreOAuth2App(req, req.AppID(), req.ActingUserID(), data) + err = a.appServices.StoreOAuth2App(r, r.AppID(), r.ActingUserID(), data) if err != nil { httputils.WriteError(w, err) return } } -func (a *restapi) OAuth2StoreUser(req *incoming.Request, w http.ResponseWriter, r *http.Request) { - data, err := httputils.LimitReadAll(r.Body, MaxKVStoreValueLength) +func (a *restapi) OAuth2StoreUser(r *incoming.Request, w http.ResponseWriter, req *http.Request) { + data, err := httputils.LimitReadAll(req.Body, MaxKVStoreValueLength) if err != nil { httputils.WriteError(w, err) return } - err = a.appServices.StoreOAuth2User(req, req.AppID(), req.ActingUserID(), data) + err = a.appServices.StoreOAuth2User(r, r.AppID(), r.ActingUserID(), data) if err != nil { httputils.WriteError(w, err) return } } -func (a *restapi) OAuth2GetUser(req *incoming.Request, w http.ResponseWriter, r *http.Request) { - data, err := a.appServices.GetOAuth2User(req, req.AppID(), req.ActingUserID()) +func (a *restapi) OAuth2GetUser(r *incoming.Request, w http.ResponseWriter, req *http.Request) { + data, err := a.appServices.GetOAuth2User(r, r.AppID(), r.ActingUserID()) if err != nil { httputils.WriteError(w, err) return diff --git a/server/httpin/restapi/app_subscribe.go b/server/httpin/restapi/app_subscribe.go index 553d82e02..a5ed5fda3 100644 --- a/server/httpin/restapi/app_subscribe.go +++ b/server/httpin/restapi/app_subscribe.go @@ -28,8 +28,8 @@ func (a *restapi) initSubscriptions(h *httpin.Handler) { // Method: POST // Input: Subscription // Output: None -func (a *restapi) Subscribe(req *incoming.Request, w http.ResponseWriter, r *http.Request) { - a.handleSubscribeCore(req, w, r, true) +func (a *restapi) Subscribe(r *incoming.Request, w http.ResponseWriter, req *http.Request) { + a.handleSubscribeCore(r, w, req, true) } // GetSubscriptions returns a users current list of subscriptions. @@ -37,8 +37,8 @@ func (a *restapi) Subscribe(req *incoming.Request, w http.ResponseWriter, r *htt // Method: GET // Input: None // Output: []Subscription -func (a *restapi) GetSubscriptions(req *incoming.Request, w http.ResponseWriter, r *http.Request) { - subs, err := a.appServices.GetSubscriptions(req, req.AppID(), req.ActingUserID()) +func (a *restapi) GetSubscriptions(r *incoming.Request, w http.ResponseWriter, req *http.Request) { + subs, err := a.appServices.GetSubscriptions(r, r.AppID(), r.ActingUserID()) if err != nil { _, _ = w.Write([]byte(err.Error())) return @@ -46,7 +46,7 @@ func (a *restapi) GetSubscriptions(req *incoming.Request, w http.ResponseWriter, err = httputils.WriteJSON(w, subs) if err != nil { - req.Log.WithError(err).Errorf("Error marshaling subscriptions") + r.Log.WithError(err).Errorf("Error marshaling subscriptions") } } @@ -55,26 +55,26 @@ func (a *restapi) GetSubscriptions(req *incoming.Request, w http.ResponseWriter, // Method: POST // Input: Subscription // Output: None -func (a *restapi) Unsubscribe(req *incoming.Request, w http.ResponseWriter, r *http.Request) { - a.handleSubscribeCore(req, w, r, false) +func (a *restapi) Unsubscribe(r *incoming.Request, w http.ResponseWriter, req *http.Request) { + a.handleSubscribeCore(r, w, req, false) } -func (a *restapi) handleSubscribeCore(req *incoming.Request, w http.ResponseWriter, r *http.Request, isSubscribe bool) { +func (a *restapi) handleSubscribeCore(r *incoming.Request, w http.ResponseWriter, req *http.Request, isSubscribe bool) { status, logMessage, err := func() (int, string, error) { var sub apps.Subscription - if err := json.NewDecoder(r.Body).Decode(&sub); err != nil { + if err := json.NewDecoder(req.Body).Decode(&sub); err != nil { return http.StatusBadRequest, "Failed to parse Subscription", err } - sub.AppID = req.AppID() - sub.UserID = req.ActingUserID() + sub.AppID = r.AppID() + sub.UserID = r.ActingUserID() // TODO replace with an appropriate API-level call that would deduplicate, etc. var err error if isSubscribe { - err = a.appServices.Subscribe(req, sub) + err = a.appServices.Subscribe(r, sub) } else { - err = a.appServices.Unsubscribe(req, sub) + err = a.appServices.Unsubscribe(r, sub) } if err != nil { @@ -85,7 +85,7 @@ func (a *restapi) handleSubscribeCore(req *incoming.Request, w http.ResponseWrit }() if err != nil { - req.Log.WithError(err).Warnw(logMessage) + r.Log.WithError(err).Warnw(logMessage) http.Error(w, err.Error(), status) } } diff --git a/server/httpin/restapi/call.go b/server/httpin/restapi/call.go index 27dbf937d..a575163b7 100644 --- a/server/httpin/restapi/call.go +++ b/server/httpin/restapi/call.go @@ -2,7 +2,6 @@ package restapi import ( "net/http" - "strings" "github.com/pkg/errors" @@ -26,46 +25,37 @@ func (a *restapi) initCall(h *httpin.Handler) { // Method: POST // Input: CallRequest // Output: CallResponse -func (a *restapi) Call(req *incoming.Request, w http.ResponseWriter, r *http.Request) { - creq, err := apps.CallRequestFromJSONReader(r.Body) +func (a *restapi) Call(r *incoming.Request, w http.ResponseWriter, req *http.Request) { + creq, err := apps.CallRequestFromJSONReader(req.Body) if err != nil { httputils.WriteError(w, utils.NewInvalidError(errors.Wrap(err, "failed to unmarshal Call request"))) return } - - req.SetAppID(creq.Context.AppID) + r.SetAppID(creq.Context.AppID) // Clear out anythging in the incoming expanded context for security // reasons, it will be set by Expand before passing to the app. creq.Context.ExpandedContext = apps.ExpandedContext{} - creq.Context, err = a.cleanUserAgentContext(req, req.ActingUserID(), creq.Context) + creq.Context, err = a.cleanUserAgentContext(r.ActingUserID(), creq.Context) if err != nil { httputils.WriteError(w, utils.NewInvalidError(errors.Wrap(err, "invalid call context for user"))) return } - res := a.proxy.Call(req, *creq) + cresp := a.proxy.Call(r, *creq) - errorText := "" - if res.Type == apps.CallResponseTypeError { - errorText = res.Text - } - req.Log.Debugw( - "Received call response", - "error", errorText, - "type", res.Type, - "call_path", creq.Path, - ) - - // Only track submit calls - if strings.HasSuffix(creq.Path, "submit") { + // Add the request and response digests to the logger. + r.Log = r.Log.With(creq, cresp) + + // Only track submit calls. + if creq.Context.UserAgentContext.TrackAsSubmit { a.conf.Telemetry().TrackCall(string(creq.Context.AppID), string(creq.Context.Location), creq.Context.ActingUserID, "submit") } - _ = httputils.WriteJSON(w, res) + _ = httputils.WriteJSON(w, cresp) } -func (a *restapi) cleanUserAgentContext(_ *incoming.Request, userID string, orig apps.Context) (apps.Context, error) { +func (a *restapi) cleanUserAgentContext(userID string, orig apps.Context) (apps.Context, error) { mm := a.conf.MattermostAPI() var postID, channelID, teamID string cc := apps.Context{ diff --git a/server/httpin/restapi/call_test.go b/server/httpin/restapi/call_test.go index a98abd698..6e89f059d 100644 --- a/server/httpin/restapi/call_test.go +++ b/server/httpin/restapi/call_test.go @@ -17,7 +17,6 @@ import ( "github.com/mattermost/mattermost-plugin-apps/apps" "github.com/mattermost/mattermost-plugin-apps/server/config" "github.com/mattermost/mattermost-plugin-apps/server/httpin" - "github.com/mattermost/mattermost-plugin-apps/server/incoming" "github.com/mattermost/mattermost-plugin-apps/server/mocks/mock_appservices" "github.com/mattermost/mattermost-plugin-apps/server/mocks/mock_proxy" "github.com/mattermost/mattermost-plugin-apps/server/mocks/mock_session" @@ -27,10 +26,7 @@ import ( func TestCleanUserAgentContext(t *testing.T) { t.Run("no context params passed", func(t *testing.T) { - ctrl := gomock.NewController(t) conf := config.NewTestConfigService(nil) - sessionService := mock_session.NewMockService(ctrl) - a := &restapi{ conf: conf, } @@ -40,17 +36,13 @@ func TestCleanUserAgentContext(t *testing.T) { UserAgentContext: apps.UserAgentContext{}, } - _, err := a.cleanUserAgentContext(incoming.NewRequest(conf.MattermostAPI(), conf, utils.NewTestLogger(), sessionService), userID, cc) + _, err := a.cleanUserAgentContext(userID, cc) require.Error(t, err) }) t.Run("post id provided in context", func(t *testing.T) { t.Run("user is a member of the post's channel", func(t *testing.T) { conf, api := config.NewTestService(nil) - - ctrl := gomock.NewController(t) - sessionService := mock_session.NewMockService(ctrl) - a := &restapi{ conf: conf, } @@ -86,7 +78,7 @@ func TestCleanUserAgentContext(t *testing.T) { TeamId: teamID, }, nil) - cc, err := a.cleanUserAgentContext(incoming.NewRequest(conf.MattermostAPI(), conf, utils.NewTestLogger(), sessionService), userID, cc) + cc, err := a.cleanUserAgentContext(userID, cc) require.NoError(t, err) expected := apps.Context{ ActingUserID: "some_user_id", @@ -104,9 +96,6 @@ func TestCleanUserAgentContext(t *testing.T) { t.Run("user is not a member of the post's channel", func(t *testing.T) { conf, api := config.NewTestService(nil) - ctrl := gomock.NewController(t) - sessionService := mock_session.NewMockService(ctrl) - a := &restapi{ conf: conf, } @@ -132,7 +121,7 @@ func TestCleanUserAgentContext(t *testing.T) { Message: "user is not a member of the specified channel", }) - _, err := a.cleanUserAgentContext(incoming.NewRequest(conf.MattermostAPI(), conf, utils.NewTestLogger(), sessionService), userID, cc) + _, err := a.cleanUserAgentContext(userID, cc) require.Error(t, err) }) }) @@ -140,9 +129,6 @@ func TestCleanUserAgentContext(t *testing.T) { t.Run("channel id provided in context", func(t *testing.T) { t.Run("user is a member of the channel", func(t *testing.T) { conf, api := config.NewTestService(nil) - ctrl := gomock.NewController(t) - sessionService := mock_session.NewMockService(ctrl) - a := &restapi{ conf: conf, } @@ -168,7 +154,7 @@ func TestCleanUserAgentContext(t *testing.T) { TeamId: teamID, }, nil) - cc, err := a.cleanUserAgentContext(incoming.NewRequest(conf.MattermostAPI(), conf, utils.NewTestLogger(), sessionService), userID, cc) + cc, err := a.cleanUserAgentContext(userID, cc) require.NoError(t, err) expected := apps.Context{ ActingUserID: "some_user_id", @@ -182,8 +168,6 @@ func TestCleanUserAgentContext(t *testing.T) { t.Run("user is not a member of the channel", func(t *testing.T) { conf, api := config.NewTestService(nil) - ctrl := gomock.NewController(t) - sessionService := mock_session.NewMockService(ctrl) a := &restapi{ conf: conf, } @@ -202,7 +186,7 @@ func TestCleanUserAgentContext(t *testing.T) { Message: "user is not a member of the specified channel", }) - _, err := a.cleanUserAgentContext(incoming.NewRequest(conf.MattermostAPI(), conf, utils.NewTestLogger(), sessionService), userID, cc) + _, err := a.cleanUserAgentContext(userID, cc) require.Error(t, err) }) }) @@ -210,9 +194,6 @@ func TestCleanUserAgentContext(t *testing.T) { t.Run("team id provided in context", func(t *testing.T) { t.Run("user is a member of the team", func(t *testing.T) { conf, api := config.NewTestService(nil) - ctrl := gomock.NewController(t) - sessionService := mock_session.NewMockService(ctrl) - a := &restapi{ conf: conf, } @@ -231,7 +212,7 @@ func TestCleanUserAgentContext(t *testing.T) { UserId: userID, }, nil) - cc, err := a.cleanUserAgentContext(incoming.NewRequest(conf.MattermostAPI(), conf, utils.NewTestLogger(), sessionService), userID, cc) + cc, err := a.cleanUserAgentContext(userID, cc) require.NoError(t, err) expected := apps.Context{ ActingUserID: "some_user_id", @@ -244,9 +225,6 @@ func TestCleanUserAgentContext(t *testing.T) { t.Run("user is not a member of the team", func(t *testing.T) { conf, api := config.NewTestService(nil) - ctrl := gomock.NewController(t) - sessionService := mock_session.NewMockService(ctrl) - a := &restapi{ conf: conf, } @@ -264,7 +242,7 @@ func TestCleanUserAgentContext(t *testing.T) { Message: "user is not a member of the specified team", }) - _, err := a.cleanUserAgentContext(incoming.NewRequest(conf.MattermostAPI(), conf, utils.NewTestLogger(), sessionService), userID, cc) + _, err := a.cleanUserAgentContext(userID, cc) require.Error(t, err) }) }) @@ -272,8 +250,6 @@ func TestCleanUserAgentContext(t *testing.T) { func TestCleanUserAgentContextIgnoredValues(t *testing.T) { conf, api := config.NewTestService(nil) - ctrl := gomock.NewController(t) - sessionService := mock_session.NewMockService(ctrl) a := &restapi{ conf: conf, } @@ -325,7 +301,7 @@ func TestCleanUserAgentContextIgnoredValues(t *testing.T) { TeamId: teamID, }, nil) - cc, err := a.cleanUserAgentContext(incoming.NewRequest(conf.MattermostAPI(), conf, utils.NewTestLogger(), sessionService), userID, cc) + cc, err := a.cleanUserAgentContext(userID, cc) require.NoError(t, err) expected := apps.Context{ ActingUserID: "some_user_id", diff --git a/server/httpin/restapi/marketplace.go b/server/httpin/restapi/marketplace.go index 65493c220..a9170e87e 100644 --- a/server/httpin/restapi/marketplace.go +++ b/server/httpin/restapi/marketplace.go @@ -13,10 +13,10 @@ func (a *restapi) initMarketplace(h *httpin.Handler) { h.HandleFunc(path.Marketplace, a.GetMarketplace, httpin.RequireUser).Methods(http.MethodGet) } -func (a *restapi) GetMarketplace(req *incoming.Request, w http.ResponseWriter, r *http.Request) { - filter := r.URL.Query().Get("filter") - includePlugins := r.URL.Query().Get("include_plugins") != "" +func (a *restapi) GetMarketplace(r *incoming.Request, w http.ResponseWriter, req *http.Request) { + filter := req.URL.Query().Get("filter") + includePlugins := req.URL.Query().Get("include_plugins") != "" - result := a.proxy.GetListedApps(req, filter, includePlugins) + result := a.proxy.GetListedApps(r, filter, includePlugins) _ = httputils.WriteJSON(w, result) } diff --git a/server/httpin/restapi/ping.go b/server/httpin/restapi/ping.go index f7ad4ebb1..26b433a5a 100644 --- a/server/httpin/restapi/ping.go +++ b/server/httpin/restapi/ping.go @@ -18,7 +18,7 @@ func (a *restapi) initPing(h *httpin.Handler) { a.Ping, httpin.RequireUser).Methods(http.MethodPost) } -func (a *restapi) Ping(req *incoming.Request, w http.ResponseWriter, r *http.Request) { +func (a *restapi) Ping(r *incoming.Request, w http.ResponseWriter, req *http.Request) { info := a.conf.Get().GetPluginVersionInfo() _ = httputils.WriteJSON(w, info) } diff --git a/server/httpin/restapi/restapi.go b/server/httpin/restapi/restapi.go index 16395b116..d7e656516 100644 --- a/server/httpin/restapi/restapi.go +++ b/server/httpin/restapi/restapi.go @@ -51,8 +51,8 @@ func Init(h *httpin.Handler, conf config.Service, p proxy.Service, appServices a a.initMarketplace(h) } -func appIDVar(r *http.Request) apps.AppID { - s, ok := mux.Vars(r)["appid"] +func appIDVar(req *http.Request) apps.AppID { + s, ok := mux.Vars(req)["appid"] if ok { return apps.AppID(s) } diff --git a/server/httpin/restapi/restapitestlib.go b/server/httpin/restapi/restapitestlib.go index c0daa7a12..98d90ddb8 100644 --- a/server/httpin/restapi/restapitestlib.go +++ b/server/httpin/restapi/restapitestlib.go @@ -216,8 +216,8 @@ func (th *TestHelper) SetupApp(m apps.Manifest) TestApp { router := mux.NewRouter() router.HandleFunc(apps.DefaultPing.Path, httputils.DoHandleJSON(apps.NewDataResponse(nil))) - router.HandleFunc("/setup/user", func(w http.ResponseWriter, r *http.Request) { - creq, err := apps.CallRequestFromJSONReader(r.Body) + router.HandleFunc("/setup/user", func(w http.ResponseWriter, req *http.Request) { + creq, err := apps.CallRequestFromJSONReader(req.Body) require.NoError(err) require.NotNil(creq) @@ -229,8 +229,8 @@ func (th *TestHelper) SetupApp(m apps.Manifest) TestApp { httputils.WriteJSON(w, apps.NewDataResponse(nil)) }) - router.HandleFunc("/setup/user2", func(w http.ResponseWriter, r *http.Request) { - creq, err := apps.CallRequestFromJSONReader(r.Body) + router.HandleFunc("/setup/user2", func(w http.ResponseWriter, req *http.Request) { + creq, err := apps.CallRequestFromJSONReader(req.Body) require.NoError(err) require.NotNil(creq) diff --git a/server/httpin/restapi/ux_bindings.go b/server/httpin/restapi/ux_bindings.go index 675b05f01..3dbf9846d 100644 --- a/server/httpin/restapi/ux_bindings.go +++ b/server/httpin/restapi/ux_bindings.go @@ -21,10 +21,10 @@ func (a *restapi) initGetBindings(h *httpin.Handler) { // Method: GET // Input: none // Output: []Binding -func (a *restapi) GetBindings(req *incoming.Request, w http.ResponseWriter, r *http.Request) { - q := r.URL.Query() +func (a *restapi) GetBindings(r *incoming.Request, w http.ResponseWriter, req *http.Request) { + q := req.URL.Query() - bindings, err := a.proxy.GetBindings(req, apps.Context{ + bindings, err := a.proxy.GetBindings(r, apps.Context{ UserAgentContext: apps.UserAgentContext{ TeamID: q.Get(config.PropTeamID), ChannelID: q.Get(config.PropChannelID), diff --git a/server/httpin/restapi/ux_get_ids.go b/server/httpin/restapi/ux_get_ids.go index 04c1379ff..9aa547e98 100644 --- a/server/httpin/restapi/ux_get_ids.go +++ b/server/httpin/restapi/ux_get_ids.go @@ -24,8 +24,8 @@ func (a *restapi) initGetOAuthAppIDs(h *httpin.Handler) { // Method: GET // Input: none // Output: []string - the list of Bot user IDs for all installed Apps. -func (a *restapi) GetBotIDs(req *incoming.Request, w http.ResponseWriter, r *http.Request) { - apps := a.proxy.GetInstalledApps(req) +func (a *restapi) GetBotIDs(r *incoming.Request, w http.ResponseWriter, _ *http.Request) { + apps := a.proxy.GetInstalledApps(r) ids := []string{} for _, app := range apps { if app.BotUserID != "" { @@ -41,8 +41,8 @@ func (a *restapi) GetBotIDs(req *incoming.Request, w http.ResponseWriter, r *htt // Method: GET // Input: none // Output: []string - the list of OAuth ClientIDs for all installed Apps. -func (a *restapi) GetOAuthAppIDs(req *incoming.Request, w http.ResponseWriter, r *http.Request) { - apps := a.proxy.GetInstalledApps(req) +func (a *restapi) GetOAuthAppIDs(r *incoming.Request, w http.ResponseWriter, _ *http.Request) { + apps := a.proxy.GetInstalledApps(r) ids := []string{} for _, app := range apps { if app.MattermostOAuth2 != nil { diff --git a/server/httpin/service.go b/server/httpin/service.go index f2d213bf1..ce1ad8a1e 100644 --- a/server/httpin/service.go +++ b/server/httpin/service.go @@ -16,18 +16,19 @@ import ( ) type Service interface { - ServeHTTP(c *plugin.Context, w http.ResponseWriter, r *http.Request) + ServeHTTP(c *plugin.Context, w http.ResponseWriter, req *http.Request) } +// TODO <>/<>: 1/5: Can Handler be combined into Service? type service struct { h *Handler } var _ Service = (*service)(nil) -func NewService(mm *pluginapi.Client, router *mux.Router, config config.Service, log utils.Logger, session incoming.SessionService, proxy proxy.Service, appServices appservices.Service, +func NewService(mm *pluginapi.Client, router *mux.Router, config config.Service, baseLog utils.Logger, session incoming.SessionService, proxy proxy.Service, appServices appservices.Service, initf ...func(*Handler, config.Service, proxy.Service, appservices.Service)) Service { - rh := NewHandler(mm, config, log, session, router) + rh := NewHandler(mm, config, baseLog, session, router) for _, f := range initf { f(rh, config, proxy, appServices) @@ -41,7 +42,7 @@ func NewService(mm *pluginapi.Client, router *mux.Router, config config.Service, } // Handle should be called by the plugin when a command invocation is received from the Mattermost server. -func (s *service) ServeHTTP(c *plugin.Context, w http.ResponseWriter, r *http.Request) { - r.Header.Set(config.MattermostSessionIDHeader, c.SessionId) - s.h.router.ServeHTTP(w, r) +func (s *service) ServeHTTP(c *plugin.Context, w http.ResponseWriter, req *http.Request) { + req.Header.Set(config.MattermostSessionIDHeader, c.SessionId) + s.h.router.ServeHTTP(w, req) } diff --git a/server/incoming/request.go b/server/incoming/request.go index 62d36820b..9459caf3c 100644 --- a/server/incoming/request.go +++ b/server/incoming/request.go @@ -68,7 +68,9 @@ func NewRequest(mm *pluginapi.Client, config config.Service, log utils.Logger, s config: config, Log: log, sessionService: session, - requestID: model.NewId(), + + // TODO <>/<>: is the incoming Mattermost request ID available, and should it be used? + requestID: model.NewId(), } r.Log = r.Log.With( @@ -121,7 +123,6 @@ func (r *Request) AppID() apps.AppID { func (r *Request) SetAppID(appID apps.AppID) { r.Log = r.Log.With("app_id", appID) - r.appID = appID } @@ -130,7 +131,7 @@ func (r *Request) ActingUserID() string { } func (r *Request) SetActingUserID(userID string) { - r.Log = r.Log.With("user_id", userID) + r.Log = r.Log.With("acting_user_id", userID) r.actingUserID = userID } diff --git a/server/mocks/mock_config/mock_config.go b/server/mocks/mock_config/mock_config.go index 7bfd1b5ff..6ae40a612 100644 --- a/server/mocks/mock_config/mock_config.go +++ b/server/mocks/mock_config/mock_config.go @@ -12,6 +12,7 @@ import ( i18n "github.com/mattermost/mattermost-plugin-api/i18n" config "github.com/mattermost/mattermost-plugin-apps/server/config" telemetry "github.com/mattermost/mattermost-plugin-apps/server/telemetry" + utils "github.com/mattermost/mattermost-plugin-apps/utils" configservice "github.com/mattermost/mattermost-server/v6/services/configservice" ) @@ -95,10 +96,10 @@ func (mr *MockServiceMockRecorder) MattermostConfig() *gomock.Call { } // Reconfigure mocks base method. -func (m *MockService) Reconfigure(arg0 config.StoredConfig, arg1 ...config.Configurable) error { +func (m *MockService) Reconfigure(arg0 config.StoredConfig, arg1 utils.Logger, arg2 ...config.Configurable) error { m.ctrl.T.Helper() - varargs := []interface{}{arg0} - for _, a := range arg1 { + varargs := []interface{}{arg0, arg1} + for _, a := range arg2 { varargs = append(varargs, a) } ret := m.ctrl.Call(m, "Reconfigure", varargs...) @@ -107,24 +108,24 @@ func (m *MockService) Reconfigure(arg0 config.StoredConfig, arg1 ...config.Confi } // Reconfigure indicates an expected call of Reconfigure. -func (mr *MockServiceMockRecorder) Reconfigure(arg0 interface{}, arg1 ...interface{}) *gomock.Call { +func (mr *MockServiceMockRecorder) Reconfigure(arg0, arg1 interface{}, arg2 ...interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - varargs := append([]interface{}{arg0}, arg1...) + varargs := append([]interface{}{arg0, arg1}, arg2...) return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Reconfigure", reflect.TypeOf((*MockService)(nil).Reconfigure), varargs...) } // StoreConfig mocks base method. -func (m *MockService) StoreConfig(arg0 config.StoredConfig) error { +func (m *MockService) StoreConfig(arg0 config.StoredConfig, arg1 utils.Logger) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "StoreConfig", arg0) + ret := m.ctrl.Call(m, "StoreConfig", arg0, arg1) ret0, _ := ret[0].(error) return ret0 } // StoreConfig indicates an expected call of StoreConfig. -func (mr *MockServiceMockRecorder) StoreConfig(arg0 interface{}) *gomock.Call { +func (mr *MockServiceMockRecorder) StoreConfig(arg0, arg1 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "StoreConfig", reflect.TypeOf((*MockService)(nil).StoreConfig), arg0) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "StoreConfig", reflect.TypeOf((*MockService)(nil).StoreConfig), arg0, arg1) } // Telemetry mocks base method. diff --git a/server/mocks/mock_store/mock_app.go b/server/mocks/mock_store/mock_app.go index 478b27942..98a0e1126 100644 --- a/server/mocks/mock_store/mock_app.go +++ b/server/mocks/mock_store/mock_app.go @@ -38,17 +38,17 @@ func (m *MockAppStore) EXPECT() *MockAppStoreMockRecorder { } // AsMap mocks base method. -func (m *MockAppStore) AsMap(arg0 *incoming.Request) map[apps.AppID]apps.App { +func (m *MockAppStore) AsMap() map[apps.AppID]apps.App { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "AsMap", arg0) + ret := m.ctrl.Call(m, "AsMap") ret0, _ := ret[0].(map[apps.AppID]apps.App) return ret0 } // AsMap indicates an expected call of AsMap. -func (mr *MockAppStoreMockRecorder) AsMap(arg0 interface{}) *gomock.Call { +func (mr *MockAppStoreMockRecorder) AsMap() *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AsMap", reflect.TypeOf((*MockAppStore)(nil).AsMap), arg0) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AsMap", reflect.TypeOf((*MockAppStore)(nil).AsMap)) } // Configure mocks base method. @@ -80,18 +80,18 @@ func (mr *MockAppStoreMockRecorder) Delete(arg0, arg1 interface{}) *gomock.Call } // Get mocks base method. -func (m *MockAppStore) Get(arg0 *incoming.Request, arg1 apps.AppID) (*apps.App, error) { +func (m *MockAppStore) Get(arg0 apps.AppID) (*apps.App, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Get", arg0, arg1) + ret := m.ctrl.Call(m, "Get", arg0) ret0, _ := ret[0].(*apps.App) ret1, _ := ret[1].(error) return ret0, ret1 } // Get indicates an expected call of Get. -func (mr *MockAppStoreMockRecorder) Get(arg0, arg1 interface{}) *gomock.Call { +func (mr *MockAppStoreMockRecorder) Get(arg0 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockAppStore)(nil).Get), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockAppStore)(nil).Get), arg0) } // InitBuiltin mocks base method. diff --git a/server/mocks/mock_store/mock_appkv.go b/server/mocks/mock_store/mock_appkv.go index d5cd208e1..2c48bd744 100644 --- a/server/mocks/mock_store/mock_appkv.go +++ b/server/mocks/mock_store/mock_appkv.go @@ -36,32 +36,32 @@ func (m *MockAppKVStore) EXPECT() *MockAppKVStoreMockRecorder { } // Delete mocks base method. -func (m *MockAppKVStore) Delete(arg0 *incoming.Request, arg1 apps.AppID, arg2, arg3, arg4 string) error { +func (m *MockAppKVStore) Delete(arg0 apps.AppID, arg1, arg2, arg3 string) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Delete", arg0, arg1, arg2, arg3, arg4) + ret := m.ctrl.Call(m, "Delete", arg0, arg1, arg2, arg3) ret0, _ := ret[0].(error) return ret0 } // Delete indicates an expected call of Delete. -func (mr *MockAppKVStoreMockRecorder) Delete(arg0, arg1, arg2, arg3, arg4 interface{}) *gomock.Call { +func (mr *MockAppKVStoreMockRecorder) Delete(arg0, arg1, arg2, arg3 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Delete", reflect.TypeOf((*MockAppKVStore)(nil).Delete), arg0, arg1, arg2, arg3, arg4) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Delete", reflect.TypeOf((*MockAppKVStore)(nil).Delete), arg0, arg1, arg2, arg3) } // Get mocks base method. -func (m *MockAppKVStore) Get(arg0 *incoming.Request, arg1 apps.AppID, arg2, arg3, arg4 string) ([]byte, error) { +func (m *MockAppKVStore) Get(arg0 apps.AppID, arg1, arg2, arg3 string) ([]byte, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Get", arg0, arg1, arg2, arg3, arg4) + ret := m.ctrl.Call(m, "Get", arg0, arg1, arg2, arg3) ret0, _ := ret[0].([]byte) ret1, _ := ret[1].(error) return ret0, ret1 } // Get indicates an expected call of Get. -func (mr *MockAppKVStoreMockRecorder) Get(arg0, arg1, arg2, arg3, arg4 interface{}) *gomock.Call { +func (mr *MockAppKVStoreMockRecorder) Get(arg0, arg1, arg2, arg3 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockAppKVStore)(nil).Get), arg0, arg1, arg2, arg3, arg4) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockAppKVStore)(nil).Get), arg0, arg1, arg2, arg3) } // List mocks base method. @@ -79,16 +79,16 @@ func (mr *MockAppKVStoreMockRecorder) List(arg0, arg1, arg2, arg3, arg4 interfac } // Set mocks base method. -func (m *MockAppKVStore) Set(arg0 *incoming.Request, arg1 apps.AppID, arg2, arg3, arg4 string, arg5 []byte) (bool, error) { +func (m *MockAppKVStore) Set(arg0 apps.AppID, arg1, arg2, arg3 string, arg4 []byte) (bool, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Set", arg0, arg1, arg2, arg3, arg4, arg5) + ret := m.ctrl.Call(m, "Set", arg0, arg1, arg2, arg3, arg4) ret0, _ := ret[0].(bool) ret1, _ := ret[1].(error) return ret0, ret1 } // Set indicates an expected call of Set. -func (mr *MockAppKVStoreMockRecorder) Set(arg0, arg1, arg2, arg3, arg4, arg5 interface{}) *gomock.Call { +func (mr *MockAppKVStoreMockRecorder) Set(arg0, arg1, arg2, arg3, arg4 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Set", reflect.TypeOf((*MockAppKVStore)(nil).Set), arg0, arg1, arg2, arg3, arg4, arg5) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Set", reflect.TypeOf((*MockAppKVStore)(nil).Set), arg0, arg1, arg2, arg3, arg4) } diff --git a/server/mocks/mock_store/mock_session.go b/server/mocks/mock_store/mock_session.go index 90b30ccb0..4a455200a 100644 --- a/server/mocks/mock_store/mock_session.go +++ b/server/mocks/mock_store/mock_session.go @@ -37,17 +37,17 @@ func (m *MockSessionStore) EXPECT() *MockSessionStoreMockRecorder { } // Delete mocks base method. -func (m *MockSessionStore) Delete(arg0 *incoming.Request, arg1 apps.AppID, arg2 string) error { +func (m *MockSessionStore) Delete(arg0 apps.AppID, arg1 string) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Delete", arg0, arg1, arg2) + ret := m.ctrl.Call(m, "Delete", arg0, arg1) ret0, _ := ret[0].(error) return ret0 } // Delete indicates an expected call of Delete. -func (mr *MockSessionStoreMockRecorder) Delete(arg0, arg1, arg2 interface{}) *gomock.Call { +func (mr *MockSessionStoreMockRecorder) Delete(arg0, arg1 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Delete", reflect.TypeOf((*MockSessionStore)(nil).Delete), arg0, arg1, arg2) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Delete", reflect.TypeOf((*MockSessionStore)(nil).Delete), arg0, arg1) } // DeleteAllForApp mocks base method. @@ -79,33 +79,33 @@ func (mr *MockSessionStoreMockRecorder) DeleteAllForUser(arg0, arg1 interface{}) } // Get mocks base method. -func (m *MockSessionStore) Get(arg0 *incoming.Request, arg1 apps.AppID, arg2 string) (*model.Session, error) { +func (m *MockSessionStore) Get(arg0 apps.AppID, arg1 string) (*model.Session, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Get", arg0, arg1, arg2) + ret := m.ctrl.Call(m, "Get", arg0, arg1) ret0, _ := ret[0].(*model.Session) ret1, _ := ret[1].(error) return ret0, ret1 } // Get indicates an expected call of Get. -func (mr *MockSessionStoreMockRecorder) Get(arg0, arg1, arg2 interface{}) *gomock.Call { +func (mr *MockSessionStoreMockRecorder) Get(arg0, arg1 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockSessionStore)(nil).Get), arg0, arg1, arg2) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockSessionStore)(nil).Get), arg0, arg1) } // ListForApp mocks base method. -func (m *MockSessionStore) ListForApp(arg0 *incoming.Request, arg1 apps.AppID) ([]*model.Session, error) { +func (m *MockSessionStore) ListForApp(arg0 apps.AppID) ([]*model.Session, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ListForApp", arg0, arg1) + ret := m.ctrl.Call(m, "ListForApp", arg0) ret0, _ := ret[0].([]*model.Session) ret1, _ := ret[1].(error) return ret0, ret1 } // ListForApp indicates an expected call of ListForApp. -func (mr *MockSessionStoreMockRecorder) ListForApp(arg0, arg1 interface{}) *gomock.Call { +func (mr *MockSessionStoreMockRecorder) ListForApp(arg0 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListForApp", reflect.TypeOf((*MockSessionStore)(nil).ListForApp), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListForApp", reflect.TypeOf((*MockSessionStore)(nil).ListForApp), arg0) } // ListForUser mocks base method. @@ -124,15 +124,15 @@ func (mr *MockSessionStoreMockRecorder) ListForUser(arg0, arg1 interface{}) *gom } // Save mocks base method. -func (m *MockSessionStore) Save(arg0 *incoming.Request, arg1 apps.AppID, arg2 string, arg3 *model.Session) error { +func (m *MockSessionStore) Save(arg0 apps.AppID, arg1 string, arg2 *model.Session) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Save", arg0, arg1, arg2, arg3) + ret := m.ctrl.Call(m, "Save", arg0, arg1, arg2) ret0, _ := ret[0].(error) return ret0 } // Save indicates an expected call of Save. -func (mr *MockSessionStoreMockRecorder) Save(arg0, arg1, arg2, arg3 interface{}) *gomock.Call { +func (mr *MockSessionStoreMockRecorder) Save(arg0, arg1, arg2 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Save", reflect.TypeOf((*MockSessionStore)(nil).Save), arg0, arg1, arg2, arg3) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Save", reflect.TypeOf((*MockSessionStore)(nil).Save), arg0, arg1, arg2) } diff --git a/server/plugin.go b/server/plugin.go index 90cc26d37..044e6db20 100644 --- a/server/plugin.go +++ b/server/plugin.go @@ -4,10 +4,8 @@ package main import ( - "bytes" gohttp "net/http" "path/filepath" - "text/template" "github.com/gorilla/mux" "github.com/pkg/errors" @@ -59,13 +57,11 @@ func NewPlugin(pluginManifest model.Manifest) *Plugin { } } -var infoTemplate = template.Must(template.New("info").Parse( - "Version: {{.Version}}, {{.URL}}, built {{.BuildDate}}, Cloud Mode: {{.CloudMode}}, Developer Mode: {{.DeveloperMode}}, Allow install over HTTP: {{.AllowHTTPApps}}")) - func (p *Plugin) OnActivate() (err error) { mm := pluginapi.NewClient(p.API, p.Driver) p.log = utils.NewPluginLogger(mm) + // Make sure we have the Bot. botUserID, err := mm.Bot.EnsureBot(&model.Bot{ Username: config.BotUsername, DisplayName: config.BotDisplayName, @@ -74,7 +70,9 @@ func (p *Plugin) OnActivate() (err error) { if err != nil { return errors.Wrap(err, "failed to ensure bot account") } + p.log.Debugw("ensured bot", "id", botUserID, "username", config.BotUsername) + // Initialize internalization and telemetrty. i18nBundle, err := i18n.InitBundle(p.API, filepath.Join("assets", "i18n")) if err != nil { return err @@ -84,49 +82,49 @@ func (p *Plugin) OnActivate() (err error) { if err != nil { p.API.LogWarn("telemetry client not started", "error", err.Error()) } - p.tracker = telemetry.NewTelemetry(nil) + // Configure the plugin. p.conf = config.NewService(mm, p.manifest, botUserID, p.tracker, i18nBundle) stored := config.StoredConfig{} _ = mm.Configuration.LoadPluginConfiguration(&stored) - err = p.conf.Reconfigure(stored) + err = p.conf.Reconfigure(stored, p.log) if err != nil { + p.log.WithError(err).Infof("failed to configure") return errors.Wrap(err, "failed to load initial configuration") } conf := p.conf.Get() - infoBuf := &bytes.Buffer{} - _ = infoTemplate.Execute(infoBuf, conf.InfoTemplateData()) - p.log.Debugf("OnActivate: %s", infoBuf.String()) + p.log.With(conf).Debugf("configured") + // Initialize outgoing HTTP. p.httpOut = httpout.NewService(p.conf) + // Initialize persistent storage. Also initialize the app API and the + // session services, both need the persisitent store. p.store, err = store.MakeService(p.log, p.conf, p.httpOut) if err != nil { return errors.Wrap(err, "failed to initialize persistent store") } p.store.App.InitBuiltin(builtin.App(conf)) - p.log.Debugf("Initialized persistent store") - p.appservices = appservices.NewService(p.conf, p.store) - p.sessionService = session.NewService(mm, p.store) + p.log.Debugf("initialized API and persistent store") + // Initialize the app proxy. mutex, err := cluster.NewMutex(p.API, config.KVClusterMutexKey) if err != nil { return errors.Wrapf(err, "failed creating cluster mutex") } - p.proxy = proxy.NewService(p.conf, p.store, mutex, p.httpOut, p.sessionService, p.appservices) err = p.proxy.Configure(conf, p.log) if err != nil { - return errors.Wrapf(err, "failed to initialize app proxy service") + return errors.Wrapf(err, "failed to initialize app proxy") } p.proxy.AddBuiltinUpstream( builtin.AppID, builtin.NewBuiltinApp(p.conf, p.proxy, p.appservices, p.httpOut, p.sessionService), ) - p.log.Debugf("Initialized the app proxy") + p.log.Debugf("initialized the app proxy") p.httpIn = httpin.NewService(mm, mux.NewRouter(), p.conf, p.log, p.sessionService, p.proxy, p.appservices, restapi.Init, @@ -136,12 +134,12 @@ func (p *Plugin) OnActivate() (err error) { if conf.MattermostCloudMode { err = p.proxy.SynchronizeInstalledApps() if err != nil { - p.log.WithError(err).Errorf("Failed to synchronize apps metadata") + p.log.WithError(err).Errorf("failed to synchronize apps metadata") } else { p.log.Debugf("Synchronized the installed apps metadata") } } - p.log.Infof("Plugin activated") + p.log.Infof("activated") p.conf.MattermostAPI().Frontend.PublishWebSocketEvent(config.WebSocketEventPluginEnabled, conf.GetPluginVersionInfo(), &model.WebsocketBroadcast{}) @@ -163,12 +161,6 @@ func (p *Plugin) OnDeactivate() error { } func (p *Plugin) OnConfigurationChange() (err error) { - defer func() { - if err != nil { - p.log.WithError(err).Errorf("Failed to reconfigure") - } - }() - if p.conf == nil { // pre-activate, nothing to do. return nil @@ -187,7 +179,12 @@ func (p *Plugin) OnConfigurationChange() (err error) { stored := config.StoredConfig{} _ = mm.Configuration.LoadPluginConfiguration(&stored) - return p.conf.Reconfigure(stored, p.store.App, p.store.Manifest, p.proxy) + err = p.conf.Reconfigure(stored, p.log, p.store.App, p.store.Manifest, p.proxy) + if err != nil { + p.log.WithError(err).Infof("failed to reconfigure") + return err + } + return nil } func (p *Plugin) ServeHTTP(c *plugin.Context, w gohttp.ResponseWriter, req *gohttp.Request) { diff --git a/server/plugin_test.go b/server/plugin_test.go index bae4c4862..57c1068e3 100644 --- a/server/plugin_test.go +++ b/server/plugin_test.go @@ -54,7 +54,7 @@ func TestOnActivate(t *testing.T) { SkuShortName: "professional", }) - expectLog(testAPI, "LogDebug", 9) + expectLog(testAPI, "LogDebug", 13) expectLog(testAPI, "LogInfo", 5) expectLog(testAPI, "LogError", 3) diff --git a/server/proxy/bindings.go b/server/proxy/bindings.go index 23f3da1ee..f3eecba0e 100644 --- a/server/proxy/bindings.go +++ b/server/proxy/bindings.go @@ -45,19 +45,19 @@ func (p *Proxy) GetBindings(r *incoming.Request, cc apps.Context) ([]apps.Bindin all := make(chan []apps.Binding) defer close(all) - allApps := store.SortApps(p.store.App.AsMap(r)) + allApps := store.SortApps(p.store.App.AsMap()) for i := range allApps { app := allApps[i] copy := r.Clone() copy.SetAppID(app.AppID) - go func(r *incoming.Request, app apps.App) { - bb, err := p.GetAppBindings(r, cc, app) + go func(app apps.App) { + bb, err := p.GetAppBindings(copy, cc, app) if err != nil { - r.Log.WithError(err).Debugw("Binding errors") + copy.Log.WithError(err).Debugf("failed to fetch app bindings") } all <- bb - }(copy, app) + }(app) } ret := []apps.Binding{} @@ -71,7 +71,7 @@ func (p *Proxy) GetBindings(r *incoming.Request, cc apps.Context) ([]apps.Bindin // GetAppBindings fetches bindings for a specific apps. We should avoid // unnecessary logging here as this route is called very often. func (p *Proxy) GetAppBindings(r *incoming.Request, cc apps.Context, app apps.App) ([]apps.Binding, error) { - if !p.appIsEnabled(r, app) { + if !p.appIsEnabled(app) { return nil, nil } if len(app.GrantedLocations) == 0 { diff --git a/server/proxy/bindings_test.go b/server/proxy/bindings_test.go index 07d90a183..bafda7df7 100644 --- a/server/proxy/bindings_test.go +++ b/server/proxy/bindings_test.go @@ -699,7 +699,7 @@ func newTestProxyForBindings(tb testing.TB, testData []bindingTestData, ctrl *go upstreams[test.app.Manifest.AppID] = up } - appStore.EXPECT().AsMap(gomock.Any()).Return(appList) + appStore.EXPECT().AsMap().Return(appList) p := &Proxy{ store: s, diff --git a/server/proxy/call.go b/server/proxy/call.go index ecd635d59..6a9460756 100644 --- a/server/proxy/call.go +++ b/server/proxy/call.go @@ -51,7 +51,7 @@ func (p *Proxy) Call(r *incoming.Request, creq apps.CallRequest) CallResponse { utils.NewInvalidError("app_id is not set in Context, don't know what app to call"))) } - app, err := p.store.App.Get(r, creq.Context.AppID) + app, err := p.store.App.Get(creq.Context.AppID) if err != nil { return NewProxyCallResponse(apps.NewErrorResponse(err)) } @@ -97,7 +97,7 @@ func (p *Proxy) callApp(r *incoming.Request, app apps.App, creq apps.CallRequest conf := p.conf.Get() - if !p.appIsEnabled(r, app) { + if !p.appIsEnabled(app) { return respondErr(errors.Errorf("%s is disabled", app.AppID)) } @@ -133,7 +133,7 @@ func (p *Proxy) callApp(r *incoming.Request, app apps.App, creq apps.CallRequest if cresp.Form != nil { clean, err := cleanForm(*cresp.Form, conf, app.AppID) if err != nil { - r.Log.WithError(err).Debugw("invalid form") + r.Log.WithError(err).Debugf("invalid form") } cresp.Form = &clean } @@ -158,12 +158,13 @@ func normalizeStaticPath(conf config.Config, appID apps.AppID, icon string) (str } func (p *Proxy) GetStatic(r *incoming.Request, appID apps.AppID, path string) (io.ReadCloser, int, error) { - app, err := p.store.App.Get(r, appID) + app, err := p.store.App.Get(appID) if err != nil { status := http.StatusInternalServerError if errors.Is(err, utils.ErrNotFound) { status = http.StatusNotFound } + r.Log = r.Log.WithError(err) return nil, status, err } diff --git a/server/proxy/enable.go b/server/proxy/enable.go index d6e9f26f8..2889ebdd3 100644 --- a/server/proxy/enable.go +++ b/server/proxy/enable.go @@ -101,14 +101,14 @@ func (p *Proxy) DisableApp(r *incoming.Request, cc apps.Context, appID apps.AppI return message, nil } -func (p *Proxy) appIsEnabled(r *incoming.Request, app apps.App) bool { +func (p *Proxy) appIsEnabled(app apps.App) bool { if app.DeployType == apps.DeployBuiltin { return true } if app.Disabled { return false } - if m, _ := p.store.Manifest.Get(r, app.AppID); m == nil { + if m, _ := p.store.Manifest.Get(app.AppID); m == nil { return false } return true diff --git a/server/proxy/install.go b/server/proxy/install.go index 915614675..ff2d02517 100644 --- a/server/proxy/install.go +++ b/server/proxy/install.go @@ -25,7 +25,7 @@ import ( // - cc is the Context that will be passed down to the App's OnInstall callback. func (p *Proxy) InstallApp(r *incoming.Request, cc apps.Context, appID apps.AppID, deployType apps.DeployType, trusted bool, secret string) (*apps.App, string, error) { conf := p.conf.Get() - m, err := p.store.Manifest.Get(r, appID) + m, err := p.store.Manifest.Get(appID) if err != nil { return nil, "", errors.Wrap(err, "failed to find manifest to install app") } @@ -37,7 +37,7 @@ func (p *Proxy) InstallApp(r *incoming.Request, cc apps.Context, appID apps.AppI return nil, "", err } - app, err := p.store.App.Get(r, appID) + app, err := p.store.App.Get(appID) if err != nil { if !errors.Is(err, utils.ErrNotFound) { return nil, "", errors.Wrap(err, "failed looking for existing app") @@ -130,8 +130,7 @@ func (p *Proxy) InstallApp(r *incoming.Request, cc apps.Context, appID apps.AppI func (p *Proxy) ensureOAuthApp(r *incoming.Request, conf config.Config, app *apps.App, noUserConsent bool, actingUserID string) error { mm := p.conf.MattermostAPI() if app.MattermostOAuth2 != nil { - r.Log.Debugw("App install flow: Using existing OAuth2 App", "id", app.MattermostOAuth2.Id) - + r.Log.Debugw("app install flow: Using existing OAuth2 App", "id", app.MattermostOAuth2.Id) return nil } @@ -153,7 +152,7 @@ func (p *Proxy) ensureOAuthApp(r *incoming.Request, conf config.Config, app *app app.MattermostOAuth2 = oAuthApp - r.Log.Debugw("App install flow: Created OAuth2 App", "id", app.MattermostOAuth2.Id) + r.Log.Debugw("app install flow: created OAuth2 App", "id", app.MattermostOAuth2.Id) return nil } @@ -166,7 +165,7 @@ func (p *Proxy) createAndValidateBot(r *incoming.Request, bot *model.Bot) error return err } - r.Log.Debugw("App install flow: created Bot Account ", + r.Log.Debugw("app install flow: created Bot Account ", "username", bot.Username, "bot_user_id", bot.UserId) const tryCount = 5 @@ -175,16 +174,16 @@ func (p *Proxy) createAndValidateBot(r *incoming.Request, bot *model.Bot) error for { bb, ee := mm.Bot.Get(bot.UserId, false) if bb != nil { - r.Log.Debugw("App install flow: validated Bot Account", "bot_user_id", bb.UserId) + r.Log.Debugw("app install flow: validated Bot Account", "bot_user_id", bb.UserId) return nil } i++ if i >= tryCount { - r.Log.Debugw("App install flow: failed to validate Bot Account, try re-installing the App", "try", i+1, "error", ee) + r.Log.Debugw("app install flow: failed to validate Bot Account, try re-installing the App", "try", i+1, "error", ee) return ee } - r.Log.Debugw("App install flow: retrying Bot Account after delay", "try", i+1, "error", ee, "delay", delay) + r.Log.Debugw("app install flow: retrying Bot Account after delay", "try", i+1, "error", ee, "delay", delay) time.Sleep(delay) delay *= 2 } @@ -227,7 +226,7 @@ func (p *Proxy) ensureBot(r *incoming.Request, app *apps.App, icon io.Reader) er } else { bot.UserId = user.Id bot.Username = user.Username - r.Log.Debugw("App install flow: using existing Bot Account", "username", bot.Username, "id", bot.UserId) + r.Log.Debugw("app install flow: using existing Bot Account", "username", bot.Username, "id", bot.UserId) } } app.BotUserID = bot.UserId @@ -236,7 +235,7 @@ func (p *Proxy) ensureBot(r *incoming.Request, app *apps.App, icon io.Reader) er if icon != nil { err := mm.User.SetProfileImage(app.BotUserID, icon) if err != nil { - r.Log.WithError(err).Errorw("App install flow: failed to update Bot Account profile icon, try re-installing") + r.Log.WithError(err).Errorf("app install flow: failed to update Bot Account profile icon, try re-installing") } } diff --git a/server/proxy/list.go b/server/proxy/list.go index 8fce94409..701d44048 100644 --- a/server/proxy/list.go +++ b/server/proxy/list.go @@ -13,16 +13,16 @@ import ( "github.com/mattermost/mattermost-plugin-apps/server/incoming" ) -func (p *Proxy) GetManifest(r *incoming.Request, appID apps.AppID) (*apps.Manifest, error) { - return p.store.Manifest.Get(r, appID) +func (p *Proxy) GetManifest(_ *incoming.Request, appID apps.AppID) (*apps.Manifest, error) { + return p.store.Manifest.Get(appID) } -func (p *Proxy) GetInstalledApp(r *incoming.Request, appID apps.AppID) (*apps.App, error) { - return p.store.App.Get(r, appID) +func (p *Proxy) GetInstalledApp(_ *incoming.Request, appID apps.AppID) (*apps.App, error) { + return p.store.App.Get(appID) } -func (p *Proxy) GetInstalledApps(r *incoming.Request) []apps.App { - installed := p.store.App.AsMap(r) +func (p *Proxy) GetInstalledApps(_ *incoming.Request) []apps.App { + installed := p.store.App.AsMap() out := []apps.App{} for _, app := range installed { out = append(out, app) @@ -36,11 +36,11 @@ func (p *Proxy) GetInstalledApps(r *incoming.Request) []apps.App { return out } -func (p *Proxy) GetListedApps(r *incoming.Request, filter string, includePluginApps bool) []apps.ListedApp { +func (p *Proxy) GetListedApps(_ *incoming.Request, filter string, includePluginApps bool) []apps.ListedApp { conf := p.conf.Get() out := []apps.ListedApp{} - for _, m := range p.store.Manifest.AsMap(r) { + for _, m := range p.store.Manifest.AsMap() { if !appMatchesFilter(m, filter) { continue } @@ -57,7 +57,7 @@ func (p *Proxy) GetListedApps(r *incoming.Request, filter string, includePluginA marketApp.IconURL = conf.StaticURL(m.AppID, m.Icon) } - app, _ := p.store.App.Get(r, m.AppID) + app, _ := p.store.App.Get(m.AppID) if app != nil { marketApp.Installed = true marketApp.Enabled = !app.Disabled diff --git a/server/proxy/notify.go b/server/proxy/notify.go index e792cdd64..b5e2081ba 100644 --- a/server/proxy/notify.go +++ b/server/proxy/notify.go @@ -27,7 +27,7 @@ func (p *Proxy) Notify(base apps.Context, subj apps.Subject) error { mm := p.conf.MattermostAPI() r := incoming.NewRequest(mm, p.conf, utils.NewPluginLogger(mm), p.sessionService, incoming.WithCtx(ctx)) - subs, err := p.store.Subscription.Get(r, subj, base.TeamID, base.ChannelID) + subs, err := p.store.Subscription.Get(subj, base.TeamID, base.ChannelID) if err != nil { return err } @@ -50,7 +50,7 @@ func (p *Proxy) notify(r *incoming.Request, base apps.Context, subs []apps.Subsc err = p.notifyForSubscription(r, &base, sub) if err != nil { - r.Log.WithError(err).Debugw("Error sending subscription notification to app") + r.Log.WithError(err).Debugf("failed to send subscription notification to app") } } @@ -61,11 +61,11 @@ func (p *Proxy) notifyForSubscription(r *incoming.Request, base *apps.Context, s creq := apps.CallRequest{ Call: sub.Call, } - app, err := p.store.App.Get(r, sub.AppID) + app, err := p.store.App.Get(sub.AppID) if err != nil { return err } - if !p.appIsEnabled(r, *app) { + if !p.appIsEnabled(*app) { return errors.Errorf("%s is disabled", app.AppID) } @@ -89,7 +89,7 @@ func (p *Proxy) NotifyMessageHasBeenPosted(post *model.Post, cc apps.Context) er mm := p.conf.MattermostAPI() r := incoming.NewRequest(mm, p.conf, utils.NewPluginLogger(mm), p.sessionService, incoming.WithCtx(ctx)) - postSubs, err := p.store.Subscription.Get(r, apps.SubjectPostCreated, cc.TeamID, cc.ChannelID) + postSubs, err := p.store.Subscription.Get(apps.SubjectPostCreated, cc.TeamID, cc.ChannelID) if err != nil && err != utils.ErrNotFound { return errors.Wrap(err, "failed to get post_created subscriptions") } @@ -100,8 +100,8 @@ func (p *Proxy) NotifyMessageHasBeenPosted(post *model.Post, cc apps.Context) er mentions := possibleAtMentions(post.Message) if len(mentions) > 0 { - appsMap := p.store.App.AsMap(r) - mentionSubs, err := p.store.Subscription.Get(r, apps.SubjectBotMentioned, cc.TeamID, cc.ChannelID) + appsMap := p.store.App.AsMap() + mentionSubs, err := p.store.Subscription.Get(apps.SubjectBotMentioned, cc.TeamID, cc.ChannelID) if err != nil && err != utils.ErrNotFound { return errors.Wrap(err, "failed to get bot_mentioned subscriptions") } @@ -149,12 +149,12 @@ func (p *Proxy) notifyJoinLeave(cc apps.Context, subject, botSubject apps.Subjec mm := p.conf.MattermostAPI() r := incoming.NewRequest(mm, p.conf, utils.NewPluginLogger(mm), p.sessionService, incoming.WithCtx(ctx)) - userSubs, err := p.store.Subscription.Get(r, subject, cc.TeamID, cc.ChannelID) + userSubs, err := p.store.Subscription.Get(subject, cc.TeamID, cc.ChannelID) if err != nil && err != utils.ErrNotFound { return errors.Wrapf(err, "failed to get %s subscriptions", subject) } - botSubs, err := p.store.Subscription.Get(r, botSubject, cc.TeamID, cc.ChannelID) + botSubs, err := p.store.Subscription.Get(botSubject, cc.TeamID, cc.ChannelID) if err != nil && err != utils.ErrNotFound { return errors.Wrapf(err, "failed to get %s subscriptions", botSubject) } @@ -162,7 +162,7 @@ func (p *Proxy) notifyJoinLeave(cc apps.Context, subject, botSubject apps.Subjec subs := []apps.Subscription{} subs = append(subs, userSubs...) - appsMap := p.store.App.AsMap(r) + appsMap := p.store.App.AsMap() for _, sub := range botSubs { app, ok := appsMap[sub.AppID] if !ok { diff --git a/server/proxy/notify_test.go b/server/proxy/notify_test.go index e50b55959..9e69b2f93 100644 --- a/server/proxy/notify_test.go +++ b/server/proxy/notify_test.go @@ -614,14 +614,14 @@ func runNotifyTest(t *testing.T, allApps []apps.App, tc notifyTestcase) { for i := range allApps { app := allApps[i] appMap[app.AppID] = app - appStore.EXPECT().Get(gomock.Any(), app.AppID).Return(&app, nil).AnyTimes() + appStore.EXPECT().Get(app.AppID).Return(&app, nil).AnyTimes() up := mock_upstream.NewMockUpstream(ctrl) upMap[app.AppID] = up upMockMap[app.AppID] = up } - appStore.EXPECT().AsMap(gomock.Any()).Return(appMap).AnyTimes() + appStore.EXPECT().AsMap().Return(appMap).AnyTimes() p := &Proxy{ store: s, diff --git a/server/proxy/oauth2.go b/server/proxy/oauth2.go index 30ee04171..7faa11a78 100644 --- a/server/proxy/oauth2.go +++ b/server/proxy/oauth2.go @@ -9,7 +9,7 @@ import ( ) func (p *Proxy) GetRemoteOAuth2ConnectURL(r *incoming.Request, appID apps.AppID) (string, error) { - app, err := p.store.App.Get(r, appID) + app, err := p.store.App.Get(appID) if err != nil { return "", err } @@ -17,7 +17,7 @@ func (p *Proxy) GetRemoteOAuth2ConnectURL(r *incoming.Request, appID apps.AppID) return "", errors.Errorf("%s is not authorized to use OAuth2", appID) } - state, err := p.store.OAuth2.CreateState(r, r.ActingUserID()) + state, err := p.store.OAuth2.CreateState(r.ActingUserID()) if err != nil { return "", err } @@ -39,7 +39,7 @@ func (p *Proxy) GetRemoteOAuth2ConnectURL(r *incoming.Request, appID apps.AppID) } func (p *Proxy) CompleteRemoteOAuth2(r *incoming.Request, appID apps.AppID, urlValues map[string]interface{}) error { - app, err := p.store.App.Get(r, appID) + app, err := p.store.App.Get(appID) if err != nil { return err } @@ -51,7 +51,7 @@ func (p *Proxy) CompleteRemoteOAuth2(r *incoming.Request, appID apps.AppID, urlV if urlState == "" { return utils.NewUnauthorizedError("no state arg in the URL") } - err = p.store.OAuth2.ValidateStateOnce(r, urlState, r.ActingUserID()) + err = p.store.OAuth2.ValidateStateOnce(urlState, r.ActingUserID()) if err != nil { return err } diff --git a/server/proxy/service.go b/server/proxy/service.go index de27d9bec..33fe62a70 100644 --- a/server/proxy/service.go +++ b/server/proxy/service.go @@ -78,12 +78,12 @@ type Notifier interface { // Internal implements go API used by other packages. type Internal interface { AddBuiltinUpstream(apps.AppID, upstream.Upstream) - CanDeploy(deployType apps.DeployType) (allowed, usable bool) - GetAppBindings(r *incoming.Request, cc apps.Context, app apps.App) ([]apps.Binding, error) - GetInstalledApp(r *incoming.Request, appID apps.AppID) (*apps.App, error) - GetInstalledApps(r *incoming.Request) []apps.App - GetListedApps(r *incoming.Request, filter string, includePluginApps bool) []apps.ListedApp - GetManifest(r *incoming.Request, appID apps.AppID) (*apps.Manifest, error) + CanDeploy(apps.DeployType) (allowed, usable bool) + GetAppBindings(*incoming.Request, apps.Context, apps.App) ([]apps.Binding, error) + GetInstalledApp(*incoming.Request, apps.AppID) (*apps.App, error) + GetInstalledApps(*incoming.Request) []apps.App + GetListedApps(_ *incoming.Request, filter string, includePluginApps bool) []apps.ListedApp + GetManifest(*incoming.Request, apps.AppID) (*apps.Manifest, error) SynchronizeInstalledApps() error } @@ -212,15 +212,15 @@ func (p *Proxy) initUpstream(typ apps.DeployType, newConfig config.Config, log u up, err := makef() switch { case errors.Cause(err) == utils.ErrNotFound: - log.WithError(err).Debugf("Skipped %q upstream: not configured.", typ) + log.WithError(err).Debugf("Skipped upstream: %s: not configured.", typ) case err != nil: - log.WithError(err).Errorf("Failed to initialize %q upstream.", typ) + log.WithError(err).Errorf("Failed to initialize upstream %s.", typ) default: p.upstreams.Store(typ, up) - log.Debugf("Initialized %q upstream.", typ) + log.Debugw("available upstream", "type", typ) } } else { p.upstreams.Delete(typ) - log.Debugf("Deploy type %q is not configured on this Mattermost server", typ) + log.Debugf("Deploy type %s is not supported on this Mattermost server", typ) } } diff --git a/server/proxy/service_test.go b/server/proxy/service_test.go index efb728984..8f41f403e 100644 --- a/server/proxy/service_test.go +++ b/server/proxy/service_test.go @@ -44,7 +44,7 @@ func newTestProxy(tb testing.TB, testApps []apps.App, ctrl *gomock.Controller) * up.EXPECT().Roundtrip(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(reader, nil) upstreams[app.Manifest.AppID] = up - appStore.EXPECT().Get(gomock.Any(), app.AppID).Return(&app, nil) + appStore.EXPECT().Get(app.AppID).Return(&app, nil) } p := &Proxy{ diff --git a/server/proxy/synchronize.go b/server/proxy/synchronize.go index 4c82e7947..0aa0478ba 100644 --- a/server/proxy/synchronize.go +++ b/server/proxy/synchronize.go @@ -26,8 +26,8 @@ func (p *Proxy) SynchronizeInstalledApps() error { mm := p.conf.MattermostAPI() r := incoming.NewRequest(mm, p.conf, utils.NewPluginLogger(mm), p.sessionService, incoming.WithCtx(ctx)) - installed := p.store.App.AsMap(r) - listed := p.store.Manifest.AsMap(r) + installed := p.store.App.AsMap() + listed := p.store.Manifest.AsMap() diff := map[apps.AppID]apps.App{} for _, app := range installed { @@ -65,7 +65,7 @@ func (p *Proxy) SynchronizeInstalledApps() error { return nil }) if err != nil { - r.Log.WithError(err).Errorw("Failed in callOnce:OnVersionChanged", + r.Log.WithError(err).Errorw("failed in callOnce:OnVersionChanged", "app_id", app.AppID) } } diff --git a/server/proxy/uninstall.go b/server/proxy/uninstall.go index 78a5181a2..86db27435 100644 --- a/server/proxy/uninstall.go +++ b/server/proxy/uninstall.go @@ -14,7 +14,7 @@ import ( func (p *Proxy) UninstallApp(r *incoming.Request, cc apps.Context, appID apps.AppID) (string, error) { mm := p.conf.MattermostAPI() - app, err := p.store.App.Get(r, appID) + app, err := p.store.App.Get(appID) if err != nil { return "", errors.Wrapf(err, "failed to get app. appID: %s", appID) } diff --git a/server/proxy/webhook.go b/server/proxy/webhook.go index 004b8224f..7bfcc149b 100644 --- a/server/proxy/webhook.go +++ b/server/proxy/webhook.go @@ -17,12 +17,12 @@ import ( "github.com/mattermost/mattermost-plugin-apps/utils" ) -func (p *Proxy) NotifyRemoteWebhook(r *incoming.Request, appID apps.AppID, req apps.HTTPCallRequest) error { - app, err := p.store.App.Get(r, appID) +func (p *Proxy) NotifyRemoteWebhook(r *incoming.Request, appID apps.AppID, httpCallRequest apps.HTTPCallRequest) error { + app, err := p.store.App.Get(appID) if err != nil { return err } - if !p.appIsEnabled(r, *app) { + if !p.appIsEnabled(*app) { return errors.Errorf("%s is disabled", app.AppID) } if !app.GrantedPermissions.Contains(apps.PermissionRemoteWebhooks) { @@ -34,7 +34,7 @@ func (p *Proxy) NotifyRemoteWebhook(r *incoming.Request, appID apps.AppID, req a case "", apps.SecretAuth: var q url.Values - q, err = url.ParseQuery(req.RawQuery) + q, err = url.ParseQuery(httpCallRequest.RawQuery) if err != nil { return utils.NewForbiddenError(err) } @@ -56,14 +56,14 @@ func (p *Proxy) NotifyRemoteWebhook(r *incoming.Request, appID apps.AppID, req a } var datav interface{} - err = json.Unmarshal([]byte(req.Body), &datav) + err = json.Unmarshal([]byte(httpCallRequest.Body), &datav) if err != nil { // if the data can not be decoded as JSON, send it "as is", as a string. - datav = req.Body + datav = httpCallRequest.Body } call := app.OnRemoteWebhook.WithDefault(apps.DefaultOnRemoteWebhook) - call.Path = path.Join(call.Path, req.Path) + call.Path = path.Join(call.Path, httpCallRequest.Path) cc, err := p.expandContext(r, *app, nil, call.Expand) if err != nil { @@ -78,10 +78,10 @@ func (p *Proxy) NotifyRemoteWebhook(r *incoming.Request, appID apps.AppID, req a Call: call, Context: cc, Values: map[string]interface{}{ - "headers": req.Headers, + "headers": httpCallRequest.Headers, "data": datav, - "httpMethod": req.HTTPMethod, - "rawQuery": req.RawQuery, + "httpMethod": httpCallRequest.HTTPMethod, + "rawQuery": httpCallRequest.RawQuery, }, }) } diff --git a/server/session/session.go b/server/session/session.go index d2e291913..6f10aea7e 100644 --- a/server/session/session.go +++ b/server/session/session.go @@ -20,10 +20,10 @@ const ( ) type Service interface { - GetOrCreate(r *incoming.Request, appID apps.AppID, userID string) (*model.Session, error) - ListForUser(r *incoming.Request, userID string) ([]*model.Session, error) - RevokeSessionsForApp(r *incoming.Request, appID apps.AppID) error - RevokeSessionsForUser(r *incoming.Request, userID string) error + GetOrCreate(_ *incoming.Request, _ apps.AppID, userID string) (*model.Session, error) + ListForUser(_ *incoming.Request, userID string) ([]*model.Session, error) + RevokeSessionsForApp(*incoming.Request, apps.AppID) error + RevokeSessionsForUser(_ *incoming.Request, userID string) error } var _ Service = (*service)(nil) @@ -41,10 +41,10 @@ func NewService(mm *pluginapi.Client, store *store.Service) Service { } func (s *service) GetOrCreate(r *incoming.Request, appID apps.AppID, userID string) (*model.Session, error) { - session, err := s.store.Session.Get(r, appID, userID) + session, err := s.store.Session.Get(appID, userID) if err == nil && !session.IsExpired() { - err = s.extendSessionExpiryIfNeeded(r, appID, userID, session) + err = s.extendSessionExpiryIfNeeded(appID, userID, session) if err != nil { return nil, errors.Wrap(err, "failed to extend session length") } @@ -65,7 +65,7 @@ func (s *service) createSession(r *incoming.Request, appID apps.AppID, userID st return nil, errors.Wrap(err, "failed to fetch user for new session") } - app, err := s.store.App.Get(r, appID) + app, err := s.store.App.Get(appID) if err != nil { return nil, errors.Wrap(err, "failed to fetch app for new session") } @@ -99,17 +99,17 @@ func (s *service) createSession(r *incoming.Request, appID apps.AppID, userID st return nil, errors.Wrap(err, "failed to create new app session") } - err = s.store.Session.Save(r, appID, userID, session) + err = s.store.Session.Save(appID, userID, session) if err != nil { return nil, errors.Wrap(err, "failed to save new session in store") } - r.Log.Debugw("Created new access token", "app_id", appID, "user_id", userID, "session_id", session.Id, "token", utils.LastN(session.Token, 3)) + r.Log.Debugw("created new access token", "app_id", appID, "user_id", userID, "session_id", session.Id, "token", utils.LastN(session.Token, 3)) return session, nil } -func (s *service) extendSessionExpiryIfNeeded(r *incoming.Request, appID apps.AppID, userID string, session *model.Session) error { +func (s *service) extendSessionExpiryIfNeeded(appID apps.AppID, userID string, session *model.Session) error { remaining := time.Until(time.UnixMilli(session.ExpiresAt)) if remaining > MinSessionLength { return nil @@ -122,10 +122,10 @@ func (s *service) extendSessionExpiryIfNeeded(r *incoming.Request, appID apps.Ap return err } - // Update store + // Update the store. session.ExpiresAt = newExpiryTime.UnixMilli() - err = s.store.Session.Save(r, appID, userID, session) + err = s.store.Session.Save(appID, userID, session) if err != nil { return errors.Wrap(err, "failed to save new session in store") } @@ -147,7 +147,7 @@ func (s service) revokeSessions(r *incoming.Request, sessions []*model.Session) } } - err := s.store.Session.Delete(r, sessionutils.GetAppID(session), session.UserId) + err := s.store.Session.Delete(sessionutils.GetAppID(session), session.UserId) if err != nil { r.Log.WithError(err).Warnw("failed to delete revoked session from store") } @@ -157,7 +157,7 @@ func (s service) revokeSessions(r *incoming.Request, sessions []*model.Session) } func (s service) RevokeSessionsForApp(r *incoming.Request, appID apps.AppID) error { - sessions, err := s.store.Session.ListForApp(r, appID) + sessions, err := s.store.Session.ListForApp(appID) if err != nil { return errors.Wrap(err, "failed to list app sessions for revocation") } diff --git a/server/session/session_test.go b/server/session/session_test.go index ff2715cf2..3565b59c0 100644 --- a/server/session/session_test.go +++ b/server/session/session_test.go @@ -62,7 +62,7 @@ func TestGetOrCreate(t *testing.T) { } session.AddProp(model.SessionPropMattermostAppID, string(appID)) - sessionStore.EXPECT().Get(r, appID, userID).Times(1).Return(session, nil) + sessionStore.EXPECT().Get(appID, userID).Times(1).Return(session, nil) rSession, err := sessionService.GetOrCreate(r, appID, userID) assert.NoError(t, err) @@ -88,9 +88,9 @@ func TestGetOrCreate(t *testing.T) { api.On("ExtendSessionExpiry", s.Id, mock.Anything).Once().Return(nil) s.AddProp(model.SessionPropMattermostAppID, string(appID)) - sessionStore.EXPECT().Get(r, appID, userID).Times(1).Return(&s, nil) + sessionStore.EXPECT().Get(appID, userID).Times(1).Return(&s, nil) - sessionStore.EXPECT().Save(r, appID, userID, gomock.Any()).Times(1).Return(nil) + sessionStore.EXPECT().Save(appID, userID, gomock.Any()).Times(1).Return(nil) rSession, err := sessionService.GetOrCreate(r, appID, userID) assert.NoError(t, err) @@ -145,9 +145,9 @@ func TestGetOrCreate(t *testing.T) { assert.Equal(t, newSession, rSession) }).Return(newSession, nil) - sessionStore.EXPECT().Get(r, appID, userID).Times(1).Return(nil, utils.ErrNotFound) - sessionStore.EXPECT().Save(r, appID, userID, gomock.Any()).Times(1).Return(nil) - appStore.EXPECT().Get(r, appID).Times(1).Return(&apps.App{ + sessionStore.EXPECT().Get(appID, userID).Times(1).Return(nil, utils.ErrNotFound) + sessionStore.EXPECT().Save(appID, userID, gomock.Any()).Times(1).Return(nil) + appStore.EXPECT().Get(appID).Times(1).Return(&apps.App{ MattermostOAuth2: oAuthApp, }, nil) @@ -197,9 +197,9 @@ func TestRevokeSessionsForApp(t *testing.T) { session2.AddProp(model.SessionPropMattermostAppID, string(appID)) sessions := []*model.Session{session1, session2} - sessionStore.EXPECT().ListForApp(r, appID).Return(sessions, nil).Times(1) - sessionStore.EXPECT().Delete(r, appID, userID1).Return(nil).Times(1) - sessionStore.EXPECT().Delete(r, appID, userID2).Return(nil).Times(1) + sessionStore.EXPECT().ListForApp(appID).Return(sessions, nil).Times(1) + sessionStore.EXPECT().Delete(appID, userID1).Return(nil).Times(1) + sessionStore.EXPECT().Delete(appID, userID2).Return(nil).Times(1) api.On("RevokeSession", sessions[0].Id).Return(nil).Once() api.On("RevokeSession", sessions[1].Id).Return(nil).Once() diff --git a/server/store/appkv.go b/server/store/appkv.go index 64374d9a1..ffb08c4ee 100644 --- a/server/store/appkv.go +++ b/server/store/appkv.go @@ -16,10 +16,10 @@ const ( ) type AppKVStore interface { - Set(r *incoming.Request, appID apps.AppID, actingUserID, prefix, id string, data []byte) (bool, error) - Get(r *incoming.Request, appID apps.AppID, actingUserID, prefix, id string) ([]byte, error) - Delete(r *incoming.Request, appID apps.AppID, actingUserID, prefix, id string) error - List(r *incoming.Request, appID apps.AppID, actingUserID, namespace string, processf func(key string) error) error + Set(_ apps.AppID, actingUserID, prefix, id string, data []byte) (bool, error) + Get(_ apps.AppID, actingUserID, prefix, id string) ([]byte, error) + Delete(_ apps.AppID, actingUserID, prefix, id string) error + List(_ *incoming.Request, _ apps.AppID, actingUserID, namespace string, processf func(key string) error) error } type appKVStore struct { @@ -28,7 +28,7 @@ type appKVStore struct { var _ AppKVStore = (*appKVStore)(nil) -func (s *appKVStore) Set(r *incoming.Request, appID apps.AppID, actingUserID, prefix, id string, data []byte) (bool, error) { +func (s *appKVStore) Set(appID apps.AppID, actingUserID, prefix, id string, data []byte) (bool, error) { if appID == "" || actingUserID == "" { return false, utils.NewInvalidError("app and user IDs must be provided") } @@ -41,7 +41,7 @@ func (s *appKVStore) Set(r *incoming.Request, appID apps.AppID, actingUserID, pr return s.conf.MattermostAPI().KV.Set(key, data) } -func (s *appKVStore) Get(r *incoming.Request, appID apps.AppID, actingUserID, prefix, id string) ([]byte, error) { +func (s *appKVStore) Get(appID apps.AppID, actingUserID, prefix, id string) ([]byte, error) { key, err := Hashkey(config.KVAppPrefix, appID, actingUserID, prefix, id) if err != nil { return nil, err @@ -55,7 +55,7 @@ func (s *appKVStore) Get(r *incoming.Request, appID apps.AppID, actingUserID, pr return data, err } -func (s *appKVStore) Delete(r *incoming.Request, appID apps.AppID, actingUserID, prefix, id string) error { +func (s *appKVStore) Delete(appID apps.AppID, actingUserID, prefix, id string) error { key, err := Hashkey(config.KVAppPrefix, appID, actingUserID, prefix, id) if err != nil { return err diff --git a/server/store/apps.go b/server/store/apps.go index a934561c6..b52b9079e 100644 --- a/server/store/apps.go +++ b/server/store/apps.go @@ -23,10 +23,10 @@ type AppStore interface { InitBuiltin(...apps.App) - Get(r *incoming.Request, appID apps.AppID) (*apps.App, error) - AsMap(r *incoming.Request) map[apps.AppID]apps.App - Save(r *incoming.Request, app apps.App) error - Delete(r *incoming.Request, appID apps.AppID) error + Get(apps.AppID) (*apps.App, error) + AsMap() map[apps.AppID]apps.App + Save(*incoming.Request, apps.App) error + Delete(*incoming.Request, apps.AppID) error } // appStore combines installed and builtin Apps. The installed Apps are stored @@ -75,19 +75,19 @@ func (s *appStore) Configure(conf config.Config, log utils.Logger) error { var data []byte err := mm.KV.Get(config.KVInstalledAppPrefix+key, &data) if err != nil { - log.WithError(err).Errorw("Failed to load app") + log.WithError(err).Errorw("failed to load app") continue } if len(data) == 0 { err = utils.NewNotFoundError(config.KVInstalledAppPrefix + key) - log.WithError(err).Errorw("Failed to load app") + log.WithError(err).Errorw("failed to load app") continue } app, err := apps.DecodeCompatibleApp(data) if err != nil { - log.WithError(err).Errorw("Failed to decode app") + log.WithError(err).Errorw("failed to decode app") continue } newInstalled[apps.AppID(id)] = *app @@ -99,7 +99,7 @@ func (s *appStore) Configure(conf config.Config, log utils.Logger) error { return nil } -func (s *appStore) Get(r *incoming.Request, appID apps.AppID) (*apps.App, error) { +func (s *appStore) Get(appID apps.AppID) (*apps.App, error) { s.mutex.RLock() installed := s.installed builtin := s.builtinInstalled @@ -116,7 +116,7 @@ func (s *appStore) Get(r *incoming.Request, appID apps.AppID) (*apps.App, error) return nil, utils.NewNotFoundError("app %s is not installed", appID) } -func (s *appStore) AsMap(_ *incoming.Request) map[apps.AppID]apps.App { +func (s *appStore) AsMap() map[apps.AppID]apps.App { s.mutex.RLock() installed := s.installed builtin := s.builtinInstalled @@ -184,7 +184,7 @@ func (s *appStore) Save(r *incoming.Request, app apps.App) error { } updated[string(app.AppID)] = sha sc.InstalledApps = updated - err = s.conf.StoreConfig(sc) + err = s.conf.StoreConfig(sc, r.Log) if err != nil { return err } @@ -237,5 +237,5 @@ func (s *appStore) Delete(r *incoming.Request, appID apps.AppID) error { } delete(updated, string(appID)) sc.InstalledApps = updated - return s.conf.StoreConfig(sc) + return s.conf.StoreConfig(sc, r.Log) } diff --git a/server/store/manifest.go b/server/store/manifest.go index a34871535..b35836d1d 100644 --- a/server/store/manifest.go +++ b/server/store/manifest.go @@ -26,11 +26,11 @@ import ( type ManifestStore interface { config.Configurable - StoreLocal(r *incoming.Request, m apps.Manifest) error - Get(r *incoming.Request, appID apps.AppID) (*apps.Manifest, error) - GetFromS3(r *incoming.Request, appID apps.AppID, version apps.AppVersion) (*apps.Manifest, error) - AsMap(r *incoming.Request) map[apps.AppID]apps.Manifest - DeleteLocal(r *incoming.Request, appID apps.AppID) error + StoreLocal(*incoming.Request, apps.Manifest) error + Get(apps.AppID) (*apps.Manifest, error) + GetFromS3(appID apps.AppID, version apps.AppVersion) (*apps.Manifest, error) + AsMap() map[apps.AppID]apps.Manifest + DeleteLocal(*incoming.Request, apps.AppID) error } // manifestStore combines global (aka marketplace) manifests, and locally @@ -115,12 +115,12 @@ func (s *manifestStore) InitGlobal(httpOut httpout.Service, log utils.Logger) er case len(parts) == 2 && (parts[0] == "http" || parts[0] == "https"): data, err = httpOut.GetFromURL(loc, conf.DeveloperMode, apps.MaxManifestSize) default: - log.WithError(err).Errorw("Failed to load global manifest", + log.WithError(err).Errorw("failed to load global manifest", "app_id", appID) continue } if err != nil { - log.WithError(err).Errorw("Failed to load global manifest", + log.WithError(err).Errorw("failed to load global manifest", "app_id", appID, "loc", loc) continue @@ -128,14 +128,14 @@ func (s *manifestStore) InitGlobal(httpOut httpout.Service, log utils.Logger) er m, err := apps.DecodeCompatibleManifest(data) if err != nil { - log.WithError(err).Errorw("Failed to load global manifest", + log.WithError(err).Errorw("failed to load global manifest", "app_id", appID, "loc", loc) continue } if m.AppID != appID { err = errors.Errorf("mismatched app ids while getting manifest %s != %s", m.AppID, appID) - log.WithError(err).Errorw("Failed to load global manifest", + log.WithError(err).Errorw("failed to load global manifest", "app_id", appID, "loc", loc) continue @@ -184,7 +184,7 @@ func (s *manifestStore) Configure(conf config.Config, log utils.Logger) error { return nil } -func (s *manifestStore) Get(_ *incoming.Request, appID apps.AppID) (*apps.Manifest, error) { +func (s *manifestStore) Get(appID apps.AppID) (*apps.Manifest, error) { s.mutex.RLock() local := s.local global := s.global @@ -201,7 +201,7 @@ func (s *manifestStore) Get(_ *incoming.Request, appID apps.AppID) (*apps.Manife return nil, errors.Wrap(utils.ErrNotFound, string(appID)) } -func (s *manifestStore) AsMap(_ *incoming.Request) map[apps.AppID]apps.Manifest { +func (s *manifestStore) AsMap() map[apps.AppID]apps.Manifest { s.mutex.RLock() local := s.local global := s.global @@ -254,7 +254,7 @@ func (s *manifestStore) StoreLocal(r *incoming.Request, m apps.Manifest) error { updated[string(m.AppID)] = sha sc := conf.StoredConfig sc.LocalManifests = updated - err = s.conf.StoreConfig(sc) + err = s.conf.StoreConfig(sc, r.Log) if err != nil { return err } @@ -299,7 +299,7 @@ func (s *manifestStore) DeleteLocal(r *incoming.Request, appID apps.AppID) error sc := conf.StoredConfig sc.LocalManifests = updated - return s.conf.StoreConfig(sc) + return s.conf.StoreConfig(sc, r.Log) } // getFromS3 returns manifest data for an app from the S3 @@ -314,7 +314,7 @@ func (s *manifestStore) getDataFromS3(appID apps.AppID, version apps.AppVersion) } // GetFromS3 returns the manifest for an app from the S3 -func (s *manifestStore) GetFromS3(_ *incoming.Request, appID apps.AppID, version apps.AppVersion) (*apps.Manifest, error) { +func (s *manifestStore) GetFromS3(appID apps.AppID, version apps.AppVersion) (*apps.Manifest, error) { data, err := s.getDataFromS3(appID, version) if err != nil { return nil, errors.Wrap(err, "failed to get manifest data") diff --git a/server/store/oauth2.go b/server/store/oauth2.go index 0060cc451..3d6b43f0d 100644 --- a/server/store/oauth2.go +++ b/server/store/oauth2.go @@ -14,15 +14,14 @@ import ( "github.com/mattermost/mattermost-plugin-apps/apps" "github.com/mattermost/mattermost-plugin-apps/server/config" - "github.com/mattermost/mattermost-plugin-apps/server/incoming" "github.com/mattermost/mattermost-plugin-apps/utils" ) type OAuth2Store interface { - CreateState(r *incoming.Request, actingUserID string) (string, error) - ValidateStateOnce(r *incoming.Request, urlState, actingUserID string) error - SaveUser(r *incoming.Request, appID apps.AppID, actingUserID string, data []byte) error - GetUser(r *incoming.Request, appID apps.AppID, actingUserID string) ([]byte, error) + CreateState(actingUserID string) (string, error) + ValidateStateOnce(urlState, actingUserID string) error + SaveUser(appID apps.AppID, actingUserID string, data []byte) error + GetUser(appID apps.AppID, actingUserID string) ([]byte, error) } type oauth2Store struct { @@ -31,7 +30,7 @@ type oauth2Store struct { var _ OAuth2Store = (*oauth2Store)(nil) -func (s *oauth2Store) CreateState(r *incoming.Request, actingUserID string) (string, error) { +func (s *oauth2Store) CreateState(actingUserID string) (string, error) { // fit the max key size of ~50chars buf := make([]byte, 15) _, _ = rand.Read(buf) @@ -43,7 +42,7 @@ func (s *oauth2Store) CreateState(r *incoming.Request, actingUserID string) (str return state, nil } -func (s *oauth2Store) ValidateStateOnce(r *incoming.Request, urlState, actingUserID string) error { +func (s *oauth2Store) ValidateStateOnce(urlState, actingUserID string) error { ss := strings.Split(urlState, ".") if len(ss) != 2 || ss[1] != actingUserID { return utils.ErrForbidden @@ -63,7 +62,7 @@ func (s *oauth2Store) ValidateStateOnce(r *incoming.Request, urlState, actingUse return nil } -func (s *oauth2Store) SaveUser(r *incoming.Request, appID apps.AppID, actingUserID string, data []byte) error { +func (s *oauth2Store) SaveUser(appID apps.AppID, actingUserID string, data []byte) error { if appID == "" || actingUserID == "" { return utils.NewInvalidError("app and user IDs must be provided") } @@ -77,7 +76,7 @@ func (s *oauth2Store) SaveUser(r *incoming.Request, appID apps.AppID, actingUser return err } -func (s *oauth2Store) GetUser(r *incoming.Request, appID apps.AppID, actingUserID string) ([]byte, error) { +func (s *oauth2Store) GetUser(appID apps.AppID, actingUserID string) ([]byte, error) { if appID == "" || actingUserID == "" { return nil, utils.NewInvalidError("app and user IDs must be provided") } diff --git a/server/store/oauth2_test.go b/server/store/oauth2_test.go index dc7da1f3b..f04fd1dd5 100644 --- a/server/store/oauth2_test.go +++ b/server/store/oauth2_test.go @@ -12,15 +12,12 @@ import ( "github.com/mattermost/mattermost-server/v6/model" "github.com/mattermost/mattermost-plugin-apps/server/config" - "github.com/mattermost/mattermost-plugin-apps/server/incoming" - "github.com/mattermost/mattermost-plugin-apps/utils" ) func TestCreateOAuth2State(t *testing.T) { stateRE := `[A-Za-z0-9-_]+\.[A-Za-z0-9]` userID := `userid-test` conf, api := config.NewTestService(nil) - req := incoming.NewRequest(conf.MattermostAPI(), conf, utils.NewTestLogger(), nil) s := oauth2Store{ Service: &Service{ conf: conf, @@ -33,32 +30,32 @@ func TestCreateOAuth2State(t *testing.T) { data, _ := args.Get(1).([]byte) require.Regexp(t, stateRE, string(data)) }).Return(true, nil) - urlState, err := s.CreateState(req, userID) + urlState, err := s.CreateState(userID) require.NoError(t, err) key := config.KVOAuth2StatePrefix + urlState require.LessOrEqual(t, len(key), model.KeyValueKeyMaxRunes) require.Regexp(t, stateRE, urlState) // Validate errors - err = s.ValidateStateOnce(req, "invalidformat", userID) + err = s.ValidateStateOnce("invalidformat", userID) require.EqualError(t, err, "forbidden") - err = s.ValidateStateOnce(req, "nonexistent.value", userID) + err = s.ValidateStateOnce("nonexistent.value", userID) require.EqualError(t, err, "forbidden") - err = s.ValidateStateOnce(req, urlState, "idmismatch") + err = s.ValidateStateOnce(urlState, "idmismatch") require.EqualError(t, err, "forbidden") mismatchedState := "mismatched-random." + strings.Split(urlState, ".")[1] mismatchedKey := config.KVOAuth2StatePrefix + mismatchedState api.On("KVGet", mismatchedKey).Once().Return(nil, nil) // not found api.On("KVSetWithOptions", mismatchedKey, []byte(nil), mock.Anything).Once().Return(false, nil) // delete attempt - err = s.ValidateStateOnce(req, mismatchedState, userID) + err = s.ValidateStateOnce(mismatchedState, userID) require.EqualError(t, err, "state mismatch: forbidden") api.On("KVGet", key).Once().Return([]byte(`"`+urlState+`"`), nil) api.On("KVSetWithOptions", key, []byte(nil), mock.Anything).Once().Return(true, nil) // delete - err = s.ValidateStateOnce(req, urlState, userID) + err = s.ValidateStateOnce(urlState, userID) require.NoError(t, err) } @@ -79,13 +76,12 @@ func TestOAuth2User(t *testing.T) { data := []byte(`{"Test1":"test-1","Test2":"test-2"}`) // CreateState api.On("KVSetWithOptions", key, data, mock.Anything).Return(true, nil).Once() - req := incoming.NewRequest(conf.MattermostAPI(), conf, utils.NewTestLogger(), nil) - err := s.SaveUser(req, "some_app_id", userID, data) + err := s.SaveUser("some_app_id", userID, data) require.NoError(t, err) api.On("KVGet", key).Return(data, nil).Once() - rData, err := s.GetUser(req, "some_app_id", userID) + rData, err := s.GetUser("some_app_id", userID) assert.NoError(t, err) assert.NotNil(t, rData) var r Entity diff --git a/server/store/session.go b/server/store/session.go index 6d8191a13..08ce7d1df 100644 --- a/server/store/session.go +++ b/server/store/session.go @@ -14,13 +14,13 @@ import ( ) type SessionStore interface { - Get(r *incoming.Request, appID apps.AppID, userID string) (*model.Session, error) - ListForApp(r *incoming.Request, appID apps.AppID) ([]*model.Session, error) - ListForUser(r *incoming.Request, userID string) ([]*model.Session, error) - Save(r *incoming.Request, appID apps.AppID, userID string, session *model.Session) error - Delete(r *incoming.Request, appID apps.AppID, userID string) error - DeleteAllForApp(r *incoming.Request, appID apps.AppID) error - DeleteAllForUser(r *incoming.Request, userID string) error + Get(_ apps.AppID, userID string) (*model.Session, error) + ListForApp(apps.AppID) ([]*model.Session, error) + ListForUser(_ *incoming.Request, userID string) ([]*model.Session, error) + Save(_ apps.AppID, userID string, session *model.Session) error + Delete(_ apps.AppID, userID string) error + DeleteAllForApp(*incoming.Request, apps.AppID) error + DeleteAllForUser(_ *incoming.Request, userID string) error } type sessionStore struct { @@ -50,11 +50,11 @@ func parseKey(key string) (apps.AppID, string, error) { return apps.AppID(s[1]), s[2], nil } -func (s sessionStore) Get(r *incoming.Request, appID apps.AppID, userID string) (*model.Session, error) { - return s.get(r, sessionKey(appID, userID)) +func (s sessionStore) Get(appID apps.AppID, userID string) (*model.Session, error) { + return s.get(sessionKey(appID, userID)) } -func (s sessionStore) get(_ *incoming.Request, key string) (*model.Session, error) { +func (s sessionStore) get(key string) (*model.Session, error) { var session model.Session err := s.conf.MattermostAPI().KV.Get(key, &session) if err != nil { @@ -68,7 +68,7 @@ func (s sessionStore) get(_ *incoming.Request, key string) (*model.Session, erro return &session, nil } -func (s sessionStore) Save(r *incoming.Request, appID apps.AppID, userID string, session *model.Session) error { +func (s sessionStore) Save(appID apps.AppID, userID string, session *model.Session) error { _, err := s.conf.MattermostAPI().KV.Set(sessionKey(appID, userID), session) if err != nil { return err @@ -126,7 +126,7 @@ func (s sessionStore) listKeysForUser(userID string) ([]string, error) { return ret, nil } -func (s sessionStore) ListForApp(r *incoming.Request, appID apps.AppID) ([]*model.Session, error) { +func (s sessionStore) ListForApp(appID apps.AppID) ([]*model.Session, error) { keys, err := s.listKeysForApp(appID) if err != nil { return nil, err @@ -135,7 +135,7 @@ func (s sessionStore) ListForApp(r *incoming.Request, appID apps.AppID) ([]*mode ret := make([]*model.Session, 0) for _, key := range keys { - session, err := s.get(r, key) + session, err := s.get(key) if err != nil { return nil, errors.Wrapf(err, "failed get key, %s", key) } @@ -165,7 +165,7 @@ func (s sessionStore) ListForUser(r *incoming.Request, userID string) ([]*model. continue } - session, err := s.get(r, key) + session, err := s.get(key) if err != nil { r.Log.WithError(err).Debugf("failed get session for key, %s", key) continue @@ -182,7 +182,7 @@ func (s sessionStore) ListForUser(r *incoming.Request, userID string) ([]*model. return ret, nil } -func (s sessionStore) Delete(r *incoming.Request, appID apps.AppID, userID string) error { +func (s sessionStore) Delete(appID apps.AppID, userID string) error { return s.conf.MattermostAPI().KV.Delete(sessionKey(appID, userID)) } diff --git a/server/store/subscriptions.go b/server/store/subscriptions.go index e097ef30a..f5cde18a7 100644 --- a/server/store/subscriptions.go +++ b/server/store/subscriptions.go @@ -10,16 +10,15 @@ import ( "github.com/mattermost/mattermost-plugin-apps/apps" "github.com/mattermost/mattermost-plugin-apps/server/config" - "github.com/mattermost/mattermost-plugin-apps/server/incoming" "github.com/mattermost/mattermost-plugin-apps/utils" ) type SubscriptionStore interface { - Get(r *incoming.Request, subject apps.Subject, teamID, channelID string) ([]apps.Subscription, error) - List(r *incoming.Request) ([]apps.Subscription, error) - ListByUserID(r *incoming.Request, appID apps.AppID, userID string) ([]apps.Subscription, error) - Save(r *incoming.Request, sub apps.Subscription) error - Delete(r *incoming.Request, sub apps.Subscription) error + Get(_ apps.Subject, teamID, channelID string) ([]apps.Subscription, error) + List() ([]apps.Subscription, error) + ListByUserID(_ apps.AppID, userID string) ([]apps.Subscription, error) + Save(apps.Subscription) error + Delete(apps.Subscription) error } type subscriptionStore struct { @@ -52,7 +51,7 @@ func subsKey(subject apps.Subject, teamID, channelID string) (string, error) { return config.KVSubPrefix + string(subject) + idSuffix, nil } -func (s subscriptionStore) Get(r *incoming.Request, subject apps.Subject, teamID, channelID string) ([]apps.Subscription, error) { +func (s subscriptionStore) Get(subject apps.Subject, teamID, channelID string) ([]apps.Subscription, error) { key, err := subsKey(subject, teamID, channelID) if err != nil { return nil, err @@ -69,7 +68,7 @@ func (s subscriptionStore) Get(r *incoming.Request, subject apps.Subject, teamID return subs, nil } -func (s subscriptionStore) List(r *incoming.Request) ([]apps.Subscription, error) { +func (s subscriptionStore) List() ([]apps.Subscription, error) { keys, err := s.conf.MattermostAPI().KV.ListKeys(0, keysPerPage, pluginapi.WithPrefix(config.KVSubPrefix)) if err != nil { return nil, err @@ -88,8 +87,8 @@ func (s subscriptionStore) List(r *incoming.Request) ([]apps.Subscription, error return subs, nil } -func (s subscriptionStore) ListByUserID(r *incoming.Request, appID apps.AppID, userID string) ([]apps.Subscription, error) { - subs, err := s.List(r) +func (s subscriptionStore) ListByUserID(appID apps.AppID, userID string) ([]apps.Subscription, error) { + subs, err := s.List() if err != nil { return nil, err } @@ -104,7 +103,7 @@ func (s subscriptionStore) ListByUserID(r *incoming.Request, appID apps.AppID, u return rSubs, nil } -func (s subscriptionStore) Save(r *incoming.Request, sub apps.Subscription) error { +func (s subscriptionStore) Save(sub apps.Subscription) error { key, err := subsKey(sub.Subject, sub.TeamID, sub.ChannelID) if err != nil { return err @@ -135,7 +134,7 @@ func (s subscriptionStore) Save(r *incoming.Request, sub apps.Subscription) erro return nil } -func (s subscriptionStore) Delete(r *incoming.Request, sub apps.Subscription) error { +func (s subscriptionStore) Delete(sub apps.Subscription) error { key, err := subsKey(sub.Subject, sub.TeamID, sub.ChannelID) if err != nil { return err diff --git a/server/store/subscriptions_test.go b/server/store/subscriptions_test.go index f156798b8..69c5cf4ef 100644 --- a/server/store/subscriptions_test.go +++ b/server/store/subscriptions_test.go @@ -11,7 +11,6 @@ import ( "github.com/mattermost/mattermost-plugin-apps/apps" "github.com/mattermost/mattermost-plugin-apps/server/config" - "github.com/mattermost/mattermost-plugin-apps/server/incoming" "github.com/mattermost/mattermost-plugin-apps/utils" ) @@ -19,7 +18,6 @@ func TestDeleteSub(t *testing.T) { conf, api := config.NewTestService(nil) defer api.AssertExpectations(t) - req := incoming.NewRequest(conf.MattermostAPI(), conf, utils.NewTestLogger(), nil) s, err := MakeService(utils.NewTestLogger(), conf, nil) require.NoError(t, err) @@ -79,21 +77,21 @@ func TestDeleteSub(t *testing.T) { t.Run("error getting subscriptions", func(t *testing.T) { api.On("KVGet", subKey).Return(nil, model.NewAppError("KVGet", "test", map[string]interface{}{}, "test error", 0)).Times(1) - err := s.Subscription.Delete(req, toDelete) + err := s.Subscription.Delete(toDelete) require.Error(t, err) require.Equal(t, "KVGet: test, test error", err.Error()) }) t.Run("no value for subs key", func(t *testing.T) { api.On("KVGet", subKey).Return(nil, nil).Times(1) - err := s.Subscription.Delete(req, toDelete) + err := s.Subscription.Delete(toDelete) require.Error(t, err) require.Equal(t, utils.ErrNotFound.Error(), err.Error()) }) t.Run("empty list for subs key", func(t *testing.T) { api.On("KVGet", subKey).Return(emptySubsBytes, nil).Times(1) - err := s.Subscription.Delete(req, toDelete) + err := s.Subscription.Delete(toDelete) require.Error(t, err) require.Equal(t, utils.ErrNotFound.Error(), err.Error()) }) @@ -101,14 +99,14 @@ func TestDeleteSub(t *testing.T) { t.Run("error setting subscription", func(t *testing.T) { api.On("KVGet", subKey).Return(storedSubsWithToDeleteBytes, nil).Times(1) api.On("KVSetWithOptions", subKey, storedSubsBytes, mock.Anything).Return(false, model.NewAppError("KVSet", "test", map[string]interface{}{}, "test error", 0)).Times(1) - err := s.Subscription.Delete(req, toDelete) + err := s.Subscription.Delete(toDelete) require.Error(t, err) require.Equal(t, "failed to save subscriptions: KVSet: test, test error", err.Error()) }) t.Run("subscription not found", func(t *testing.T) { api.On("KVGet", subKey).Return(storedSubsBytes, nil).Times(1) - err := s.Subscription.Delete(req, toDelete) + err := s.Subscription.Delete(toDelete) require.Error(t, err) require.Equal(t, utils.ErrNotFound.Error(), err.Error()) }) @@ -116,7 +114,7 @@ func TestDeleteSub(t *testing.T) { t.Run("subscription deleted", func(t *testing.T) { api.On("KVGet", subKey).Return(storedSubsWithToDeleteBytes, nil).Times(1) api.On("KVSetWithOptions", subKey, storedSubsBytes, mock.Anything).Return(true, nil).Times(1) - err := s.Subscription.Delete(req, toDelete) + err := s.Subscription.Delete(toDelete) require.NoError(t, err) }) } @@ -124,7 +122,6 @@ func TestDeleteSub(t *testing.T) { func TestGetSubs(t *testing.T) { conf, api := config.NewTestService(nil) defer api.AssertExpectations(t) - req := incoming.NewRequest(conf.MattermostAPI(), conf, utils.NewTestLogger(), nil) s, err := MakeService(utils.NewTestLogger(), conf, nil) require.NoError(t, err) @@ -155,28 +152,28 @@ func TestGetSubs(t *testing.T) { t.Run("error getting subscriptions", func(t *testing.T) { api.On("KVGet", subKey).Return(nil, model.NewAppError("KVGet", "test", map[string]interface{}{}, "test error", 0)).Times(1) - _, err := s.Subscription.Get(req, "user_joined_channel", "team-id", "channel-id") + _, err := s.Subscription.Get("user_joined_channel", "team-id", "channel-id") require.Error(t, err) require.Equal(t, "KVGet: test, test error", err.Error()) }) t.Run("no value for subs key", func(t *testing.T) { api.On("KVGet", subKey).Return(nil, nil).Times(1) - _, err := s.Subscription.Get(req, "user_joined_channel", "team-id", "channel-id") + _, err := s.Subscription.Get("user_joined_channel", "team-id", "channel-id") require.Error(t, err) require.Equal(t, utils.ErrNotFound.Error(), err.Error()) }) t.Run("empty list for subs key", func(t *testing.T) { api.On("KVGet", subKey).Return(emptySubsBytes, nil).Times(1) - _, err := s.Subscription.Get(req, "user_joined_channel", "team-id", "channel-id") + _, err := s.Subscription.Get("user_joined_channel", "team-id", "channel-id") require.Error(t, err) require.Equal(t, utils.ErrNotFound.Error(), err.Error()) }) t.Run("subscription list got", func(t *testing.T) { api.On("KVGet", subKey).Return(storedSubsBytes, nil).Times(1) - subs, err := s.Subscription.Get(req, "user_joined_channel", "team-id", "channel-id") + subs, err := s.Subscription.Get("user_joined_channel", "team-id", "channel-id") require.NoError(t, err) require.Equal(t, storedSubs, subs) }) @@ -185,7 +182,6 @@ func TestGetSubs(t *testing.T) { func TestStoreSub(t *testing.T) { conf, api := config.NewTestService(nil) defer api.AssertExpectations(t) - req := incoming.NewRequest(conf.MattermostAPI(), conf, utils.NewTestLogger(), nil) s, err := MakeService(utils.NewTestLogger(), conf, nil) require.NoError(t, err) @@ -249,7 +245,7 @@ func TestStoreSub(t *testing.T) { t.Run("error getting subscriptions", func(t *testing.T) { api.On("KVGet", subKey).Return(nil, model.NewAppError("KVGet", "test", map[string]interface{}{}, "test error", 0)).Times(1) - err := s.Subscription.Save(req, toStore) + err := s.Subscription.Save(toStore) require.Error(t, err) require.Equal(t, "KVGet: test, test error", err.Error()) }) @@ -257,21 +253,21 @@ func TestStoreSub(t *testing.T) { t.Run("no value for subs key", func(t *testing.T) { api.On("KVGet", subKey).Return(nil, nil).Times(1) api.On("KVSetWithOptions", subKey, emptySubsWithToStoreBytes, mock.Anything).Return(true, nil).Times(1) - err := s.Subscription.Save(req, toStore) + err := s.Subscription.Save(toStore) require.NoError(t, err) }) t.Run("empty list for subs key", func(t *testing.T) { api.On("KVGet", subKey).Return(emptySubsBytes, nil).Times(1) api.On("KVSetWithOptions", subKey, emptySubsWithToStoreBytes, mock.Anything).Return(true, nil).Times(1) - err := s.Subscription.Save(req, toStore) + err := s.Subscription.Save(toStore) require.NoError(t, err) }) t.Run("error setting subscription", func(t *testing.T) { api.On("KVGet", subKey).Return(storedSubsBytes, nil).Times(1) api.On("KVSetWithOptions", subKey, storedSubsWithToStoreBytes, mock.Anything).Return(false, model.NewAppError("KVSet", "test", map[string]interface{}{}, "test error", 0)).Times(1) - err := s.Subscription.Save(req, toStore) + err := s.Subscription.Save(toStore) require.Error(t, err) require.Equal(t, "KVSet: test, test error", err.Error()) }) @@ -279,7 +275,7 @@ func TestStoreSub(t *testing.T) { t.Run("subscription stored", func(t *testing.T) { api.On("KVGet", subKey).Return(storedSubsBytes, nil).Times(1) api.On("KVSetWithOptions", subKey, storedSubsWithToStoreBytes, mock.Anything).Return(true, nil).Times(1) - err := s.Subscription.Save(req, toStore) + err := s.Subscription.Save(toStore) require.NoError(t, err) }) } diff --git a/upstream/provision.go b/upstream/provision.go index 92c812ae3..986c813c3 100644 --- a/upstream/provision.go +++ b/upstream/provision.go @@ -48,7 +48,7 @@ func GetAppBundle(bundlePath string, log utils.Logger) (*apps.Manifest, string, if err != nil { return nil, "", errors.Wrap(err, "invalid manifest.json") } - log.Debugw("Loaded App bundle", + log.Debugw("loaded App bundle", "bundle", bundlePath, "app_id", m.AppID) diff --git a/upstream/upaws/clean.go b/upstream/upaws/clean.go index aaa904d13..b5308d69d 100644 --- a/upstream/upaws/clean.go +++ b/upstream/upaws/clean.go @@ -16,9 +16,9 @@ func CleanAWS(asAdmin Client, accessKeyID string, log utils.Logger) error { if !errors.Is(err, utils.ErrNotFound) { return err } - log.Infow("Not found "+typ, "key", name) + log.Infow("not found "+typ, "key", name) } else { - log.Infow("Deleted "+typ, "key", name) + log.Infow("deleted "+typ, "key", name) } return nil } @@ -26,7 +26,7 @@ func CleanAWS(asAdmin Client, accessKeyID string, log utils.Logger) error { err := asAdmin.RemoveUserFromGroup(DefaultUserName, DefaultGroupName) switch { case err == nil: - log.Infow("Removed user from group", "user", DefaultUserName, "group", DefaultGroupName) + log.Infow("removed user from group", "user", DefaultUserName, "group", DefaultGroupName) case errors.Is(err, utils.ErrNotFound): // nothing to do default: @@ -38,7 +38,7 @@ func CleanAWS(asAdmin Client, accessKeyID string, log utils.Logger) error { err = asAdmin.DetachGroupPolicy(DefaultGroupName, ARN(*policy.Arn)) switch { case err == nil: - log.Infow("Detached policy from group", "policy", DefaultPolicyName, "group", DefaultGroupName) + log.Infow("detached policy from group", "policy", DefaultPolicyName, "group", DefaultGroupName) case errors.Is(err, utils.ErrNotFound): // nothing to do default: @@ -67,9 +67,9 @@ func CleanAWS(asAdmin Client, accessKeyID string, log utils.Logger) error { if !errors.Is(err, utils.ErrNotFound) { return err } - log.Infow("Not found policy", "ARN", *policy.Arn) + log.Infow("not found policy", "ARN", *policy.Arn) } else { - log.Infow("Deleted policy", "ARN", *policy.Arn) + log.Infow("deleted policy", "ARN", *policy.Arn) } } diff --git a/upstream/upaws/client.go b/upstream/upaws/client.go index ac1a54d41..d8819d8da 100644 --- a/upstream/upaws/client.go +++ b/upstream/upaws/client.go @@ -97,7 +97,7 @@ func MakeClient(awsAccessKeyID, awsSecretAccessKey, region string, log utils.Log awsSession.Handlers.Complete.PushFront(func(r *request.Request) { if r.HTTPResponse != nil && r.HTTPRequest != nil { - log.Debugw("AWS request", + log.Debugw("completed an AWS request", "method", r.HTTPRequest.Method, "url", r.HTTPRequest.URL.String(), "status", r.HTTPResponse.Status, @@ -116,7 +116,7 @@ func MakeClient(awsAccessKeyID, awsSecretAccessKey, region string, log utils.Log region: region, } - log.Debugw("New AWS client", + log.Debugw("created an AWS client", "region", region, "access", utils.LastN(awsAccessKeyID, 7), "secret", utils.LastN(awsSecretAccessKey, 4)) diff --git a/upstream/upaws/initialize.go b/upstream/upaws/initialize.go index 5cb2a7ece..ff85345cb 100644 --- a/upstream/upaws/initialize.go +++ b/upstream/upaws/initialize.go @@ -41,7 +41,7 @@ func InitializeAWS(asAdmin Client, log utils.Logger, params InitParams) (r *Init if !exists { return nil, errors.Errorf("S3 bucket %q is not configured", params.Bucket) } - log.Infow("Using existing S3 bucket", "name", params.Bucket) + log.Infow("using existing S3 bucket", "name", params.Bucket) r.Bucket = params.Bucket ensure := func(typ string, name Name, find, create func(Name) (ARN, error)) (ARN, error) { @@ -56,9 +56,9 @@ func InitializeAWS(asAdmin Client, log utils.Logger, params InitParams) (r *Init if err != nil { return "", errors.Wrapf(err, "failed to create %s %s", typ, name) } - log.Infow("Created "+typ, "ARN", arn) + log.Infow("created "+typ, "ARN", arn) } else { - log.Infow("Using existing "+typ, "ARN", arn) + log.Infow("using existing "+typ, "ARN", arn) } return arn, nil } @@ -73,7 +73,7 @@ func InitializeAWS(asAdmin Client, log utils.Logger, params InitParams) (r *Init if err != nil { return nil, err } - log.Infow("Created access key") + log.Infof("created access key") } r.GroupARN, err = ensure("group", params.Group, asAdmin.FindGroup, asAdmin.CreateGroup) if err != nil { @@ -104,12 +104,12 @@ func InitializeAWS(asAdmin Client, log utils.Logger, params InitParams) (r *Init if err != nil { return nil, errors.Wrapf(err, "failed to attach %s to %s", params.Policy, params.Group) } - log.Infow("Attached policy to group", "policyARN", r.PolicyARN, "groupName", params.Group) + log.Infow("attached policy to group", "policyARN", r.PolicyARN, "groupName", params.Group) err = asAdmin.AddUserToGroup(params.User, params.Group) if err != nil { return nil, errors.Wrapf(err, "failed to add user %s to %s", params.User, params.Group) } - log.Infow("Added user to group", "userName", params.User, "groupName", params.Group) + log.Infow("added user to group", "userName", params.User, "groupName", params.Group) // Create an execution role for the Apps' Lambdas. It uses // AWSLambdaBasicExecutionRole service execution policy. @@ -121,7 +121,7 @@ func InitializeAWS(asAdmin Client, log utils.Logger, params InitParams) (r *Init if err != nil { return nil, errors.Wrapf(err, "failed to attach %s to %s", LambdaExecutionPolicyARN, params.ExecuteRole) } - log.Infow("Attached AWSLambdaBasicExecutionRole policy to role", "roleName", params.ExecuteRole) + log.Infow("attached AWSLambdaBasicExecutionRole policy to role", "roleName", params.ExecuteRole) return r, nil } diff --git a/upstream/upaws/provision_app.go b/upstream/upaws/provision_app.go index a8d57f067..b8083a492 100644 --- a/upstream/upaws/provision_app.go +++ b/upstream/upaws/provision_app.go @@ -89,7 +89,7 @@ func deployS3StaticAssets(c Client, log utils.Logger, pd *DeployData, params Dep asset.File.Close() arn := ARN(fmt.Sprintf("arn:aws:s3:::%s/%s", params.Bucket, asset.Key)) arns = append(arns, arn) - log.Infow("Uploaded static asset to S3", "bucket", params.Bucket, "key", asset.Key) + log.Infow("uploaded static asset to S3", "bucket", params.Bucket, "key", asset.Key) } out.StaticARNs = arns @@ -101,7 +101,7 @@ func deployLambdaFunctions(c Client, log utils.Logger, pd *DeployData, params De if err != nil { return err } - log.Infow("Found execute role, deploying functions", "ARN", executeRoleARN) + log.Infow("found execute role, deploying functions", "ARN", executeRoleARN) createdARNs := []ARN{} for _, function := range pd.LambdaFunctions { @@ -126,7 +126,7 @@ func deployLambdaFunctions(c Client, log utils.Logger, pd *DeployData, params De return err } invokePolicyARN := ARN(*invokePolicy.Arn) - log.Infow("Found invoke policy, updating", "ARN", invokePolicyARN) + log.Infow("found invoke policy, updating", "ARN", invokePolicyARN) newDoc, err := c.AddResourcesToPolicyDocument(invokePolicy, createdARNs) if err != nil { @@ -157,6 +157,6 @@ func deployS3Manifest(c Client, log utils.Logger, pd *DeployData, params DeployA out.Manifest = *pd.Manifest out.ManifestURL = url - log.Infow("Uploaded manifest to S3 (public-read)", "bucket", params.Bucket, "key", pd.ManifestKey) + log.Infow("uploaded manifest to S3 (public-read)", "bucket", params.Bucket, "key", pd.ManifestKey) return nil } diff --git a/upstream/upaws/provision_data.go b/upstream/upaws/provision_data.go index ec043c705..d7333bca8 100644 --- a/upstream/upaws/provision_data.go +++ b/upstream/upaws/provision_data.go @@ -86,7 +86,7 @@ func getDeployData(b []byte, log utils.Logger) (*DeployData, error) { if err != nil { return nil, errors.Wrapf(err, "can't unmarshal manifest.json file %s", string(data)) } - log.Infow("Found manifest", "file", file.Name) + log.Infow("found manifest", "file", file.Name) case strings.HasSuffix(file.Name, ".zip"): lambdaFunctionFile, err := file.Open() @@ -98,7 +98,7 @@ func getDeployData(b []byte, log utils.Logger) (*DeployData, error) { Name: strings.TrimSuffix(file.Name, ".zip"), Bundle: lambdaFunctionFile, }) - log.Infow("Found lambda function bundle", "file", file.Name) + log.Infow("found lambda function bundle", "file", file.Name) case strings.HasPrefix(file.Name, path.StaticFolder+"/"): assetName := strings.TrimPrefix(file.Name, path.StaticFolder+"/") @@ -113,10 +113,10 @@ func getDeployData(b []byte, log utils.Logger) (*DeployData, error) { Key: assetName, File: assetFile, }) - log.Infow("Found static asset", "file", file.Name) + log.Infow("found static asset", "file", file.Name) default: - log.Infow("Ignored unknown file", "file", file.Name) + log.Infow("ignored unknown file", "file", file.Name) } } diff --git a/upstream/upplugin/httpclient.go b/upstream/upplugin/httpclient.go index 488c29e9c..f507363fb 100644 --- a/upstream/upplugin/httpclient.go +++ b/upstream/upplugin/httpclient.go @@ -14,8 +14,8 @@ type pluginAPIRoundTripper struct { client PluginHTTPAPI } -func (p *pluginAPIRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { - resp := p.client.HTTP(r) +func (p *pluginAPIRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { + resp := p.client.HTTP(req) if resp == nil { return nil, errors.Errorf("Failed to make interplugin request") } diff --git a/utils/httputils/utils.go b/utils/httputils/utils.go index 230f82ac6..8fff30856 100644 --- a/utils/httputils/utils.go +++ b/utils/httputils/utils.go @@ -94,7 +94,7 @@ func DoHandleJSONData(data []byte) http.HandlerFunc { // DoHandleData returns an http.HandleFunc that serves a data chunk with a // specified content-type. func DoHandleData(ct string, data []byte) http.HandlerFunc { - return func(w http.ResponseWriter, r *http.Request) { + return func(w http.ResponseWriter, req *http.Request) { w.Header().Set("Content-Type", ct) _, _ = w.Write(data) } @@ -103,7 +103,7 @@ func DoHandleData(ct string, data []byte) http.HandlerFunc { // DoHandleJSON returns an http.HandleFunc that serves a data chunk with a // specified content-type. func DoHandleJSON(v interface{}) http.HandlerFunc { - return func(w http.ResponseWriter, r *http.Request) { + return func(w http.ResponseWriter, req *http.Request) { _ = WriteJSON(w, v) } } diff --git a/utils/zap_logger.go b/utils/logger.go similarity index 60% rename from utils/zap_logger.go rename to utils/logger.go index 54274e71a..f1fbf7c9a 100644 --- a/utils/zap_logger.go +++ b/utils/logger.go @@ -4,10 +4,12 @@ package utils import ( + "fmt" + "sort" + "strings" + "go.uber.org/zap" "go.uber.org/zap/zapcore" - - pluginapi "github.com/mattermost/mattermost-plugin-api" ) const ErrorKey = "error" @@ -31,19 +33,52 @@ type Logger interface { With(args ...interface{}) Logger } -type plugin struct { - zapcore.LevelEnabler - plog pluginapi.LogService - fields map[string]zapcore.Field +type logger struct { + *zap.SugaredLogger } -func NewPluginLogger(client *pluginapi.Client) Logger { - pc := &plugin{ - LevelEnabler: zapcore.DebugLevel, - plog: client.Log, +func (l *logger) WithError(err error) Logger { + if err == nil { + return l } return &logger{ - SugaredLogger: zap.New(pc).Sugar(), + SugaredLogger: l.SugaredLogger.With(ErrorKey, err.Error()), + } +} + +type HasLoggable interface { + Loggable() []interface{} +} + +// expandWith expands anything that implements LogProps into name, value pairs. +func expandWith(args []interface{}) []interface{} { + var with []interface{} + + expectKeyOrProps := true + for _, v := range args { + lp, hasProps := v.(HasLoggable) + _, isString := v.(string) + switch { + case !expectKeyOrProps: + with = append(with, v) + expectKeyOrProps = true + case hasProps: + with = append(with, expandWith(lp.Loggable())...) + case !isString: + with = append(with, "log_error", fmt.Sprintf("expected a string key or hasLogProps, found %T", v)) + return with + default: + // a string key. + with = append(with, v) + expectKeyOrProps = false + } + } + return with +} + +func (l *logger) With(args ...interface{}) Logger { + return &logger{ + SugaredLogger: l.SugaredLogger.With(expandWith(args)...), } } @@ -82,73 +117,27 @@ func MustMakeCommandLogger(level zapcore.Level) Logger { } } -func (p *plugin) With(fields []zapcore.Field) zapcore.Core { - return p.with(fields) -} - -func (p *plugin) with(fields []zapcore.Field) *plugin { - ff := map[string]zapcore.Field{} - for k, v := range p.fields { - ff[k] = v - } - for _, f := range fields { - ff[f.Key] = f - } - c := *p - c.fields = ff - return &c -} - -func (p *plugin) Check(e zapcore.Entry, ce *zapcore.CheckedEntry) *zapcore.CheckedEntry { - return ce.AddCore(e, p) -} - -func (p *plugin) Sync() error { - return nil -} - -func (p *plugin) Write(e zapcore.Entry, fields []zapcore.Field) error { - p = p.with(fields) - w := p.plog.Error - switch e.Level { - case zapcore.DebugLevel: - w = p.plog.Debug - case zapcore.InfoLevel: - w = p.plog.Info - case zapcore.WarnLevel: - w = p.plog.Warn +func LogDigest(i interface{}) string { + if s, ok := i.(string); ok { + return s } - pairs := []interface{}{} - for k, f := range p.fields { - switch { - case f.Integer != 0: - pairs = append(pairs, k, f.Integer) - case f.String != "": - pairs = append(pairs, k, f.String) - default: - pairs = append(pairs, k, f.Interface) + var keys []string + if m, ok := i.(map[string]interface{}); ok { + for key := range m { + keys = append(keys, key) } } - w(e.Message, pairs...) - return nil -} - -type logger struct { - *zap.SugaredLogger -} -func (l *logger) WithError(err error) Logger { - if err == nil { - return l + if m, ok := i.(map[string]string); ok { + for key := range m { + keys = append(keys, key) + } } - return &logger{ - SugaredLogger: l.SugaredLogger.With(ErrorKey, err.Error()), + if len(keys) > 0 { + sort.Strings(keys) + return strings.Join(keys, ",") } -} -func (l *logger) With(args ...interface{}) Logger { - return &logger{ - SugaredLogger: l.SugaredLogger.With(args...), - } + return fmt.Sprintf("%v", i) } diff --git a/utils/plugin_logger.go b/utils/plugin_logger.go new file mode 100644 index 000000000..f9dcebf6b --- /dev/null +++ b/utils/plugin_logger.go @@ -0,0 +1,81 @@ +// Copyright (c) 2019-present Mattermost, Inc. All Rights Reserved. +// See License for license information. + +package utils + +import ( + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + + pluginapi "github.com/mattermost/mattermost-plugin-api" +) + +// plugin implements zapcore.Core interface to serve as a logging "backend" for +// SugaredLogger, using the plugin API log methods. +type plugin struct { + zapcore.LevelEnabler + plog pluginapi.LogService + fields map[string]zapcore.Field +} + +func NewPluginLogger(client *pluginapi.Client) Logger { + pc := &plugin{ + LevelEnabler: zapcore.DebugLevel, + plog: client.Log, + } + return &logger{ + SugaredLogger: zap.New(pc).Sugar(), + } +} + +func (p *plugin) With(fields []zapcore.Field) zapcore.Core { + return p.with(fields) +} + +func (p *plugin) with(fields []zapcore.Field) *plugin { + ff := map[string]zapcore.Field{} + for k, v := range p.fields { + ff[k] = v + } + for _, f := range fields { + ff[f.Key] = f + } + c := *p + c.fields = ff + return &c +} + +func (p *plugin) Check(e zapcore.Entry, ce *zapcore.CheckedEntry) *zapcore.CheckedEntry { + return ce.AddCore(e, p) +} + +func (p *plugin) Sync() error { + return nil +} + +func (p *plugin) Write(e zapcore.Entry, fields []zapcore.Field) error { + p = p.with(fields) + w := p.plog.Error + switch e.Level { + case zapcore.DebugLevel: + w = p.plog.Debug + case zapcore.InfoLevel: + w = p.plog.Info + case zapcore.WarnLevel: + w = p.plog.Warn + } + + pairs := []interface{}{} + for k, f := range p.fields { + switch { + case f.Integer != 0: + pairs = append(pairs, k, f.Integer) + case f.String != "": + pairs = append(pairs, k, f.String) + default: + pairs = append(pairs, k, f.Interface) + } + } + w(e.Message, pairs...) + return nil +}