diff --git a/api4/bot.go b/api4/bot.go index 6809894a96..6d9db44193 100644 --- a/api4/bot.go +++ b/api4/bot.go @@ -244,13 +244,15 @@ func getBotIconImage(c *Context, w http.ResponseWriter, r *http.Request) { return } - user, err := c.App.GetUser(botUserId) + img, err := c.App.GetBotIconImage(botUserId) if err != nil { c.Err = err return } - if !user.IsBot { - c.Err = model.MakeBotNotFoundError(botUserId) + + user, err := c.App.GetUser(botUserId) + if err != nil { + c.Err = err return } @@ -259,19 +261,8 @@ func getBotIconImage(c *Context, w http.ResponseWriter, r *http.Request) { return } - img, readFailed, err := c.App.GetBotIconImage(user.Id) - if err != nil { - c.Err = err - return - } - - if readFailed { - w.Header().Set("Cache-Control", fmt.Sprintf("max-age=%v, public", 5*60)) // 5 mins - } else { - w.Header().Set("Cache-Control", fmt.Sprintf("max-age=%v, public", 24*60*60)) // 24 hrs - w.Header().Set(model.HEADER_ETAG_SERVER, etag) - } - + w.Header().Set("Cache-Control", fmt.Sprintf("max-age=%v, public", 24*60*60)) // 24 hrs + w.Header().Set(model.HEADER_ETAG_SERVER, etag) w.Header().Set("Content-Type", "image/svg+xml") w.Write(img) } @@ -290,11 +281,6 @@ func setBotIconImage(c *Context, w http.ResponseWriter, r *http.Request) { return } - if _, err := c.App.GetBot(botUserId, true); err != nil { - c.Err = model.MakeBotNotFoundError(botUserId) - return - } - if r.ContentLength > *c.App.Config().FileSettings.MaxFileSize { c.Err = model.NewAppError("setBotIconImage", "api.bot.set_bot_icon_image.too_large.app_error", nil, "", http.StatusRequestEntityTooLarge) return @@ -318,7 +304,7 @@ func setBotIconImage(c *Context, w http.ResponseWriter, r *http.Request) { } imageData := imageArray[0] - if err := c.App.SetBotIconImage(botUserId, imageData); err != nil { + if err := c.App.SetBotIconImageFromMultiPartFile(botUserId, imageData); err != nil { c.Err = err return } diff --git a/api4/bot_test.go b/api4/bot_test.go index 48d3532cd7..ce080c0284 100644 --- a/api4/bot_test.go +++ b/api4/bot_test.go @@ -1155,7 +1155,13 @@ func TestSetBotIconImage(t *testing.T) { _, resp = th.SystemAdminClient.SetBotIconImage(bot.UserId, goodData) CheckNoError(t, resp) - info := &model.FileInfo{Path: "/bots/" + bot.UserId + "/icon.svg"} + fpath := fmt.Sprintf("/bots/%v/icon.svg", bot.UserId) + actualData, err := th.App.ReadFile(fpath) + require.Nil(t, err) + require.NotNil(t, actualData) + require.Equal(t, goodData, actualData) + + info := &model.FileInfo{Path: fpath} err = th.cleanupTestFile(info) require.Nil(t, err) } diff --git a/app/bot.go b/app/bot.go index 646db68c0f..0691e127cc 100644 --- a/app/bot.go +++ b/app/bot.go @@ -5,6 +5,7 @@ package app import ( "fmt" + "io" "mime/multipart" "net/http" @@ -197,19 +198,25 @@ func (a *App) ConvertUserToBot(user *model.User) (*model.Bot, *model.AppError) { return a.Srv.Store.Bot().Save(model.BotFromUser(user)) } -// SetBotIconImage sets LHS icon for a bot. -func (a *App) SetBotIconImage(botUserId string, imageData *multipart.FileHeader) *model.AppError { - if len(*a.Config().FileSettings.DriverName) == 0 { - return model.NewAppError("SetBotIconImage", "api.bot.icon_image.storage.app_error", nil, "", http.StatusNotImplemented) - } - +// SetBotIconImageFromMultiPartFile sets LHS icon for a bot. +func (a *App) SetBotIconImageFromMultiPartFile(botUserId string, imageData *multipart.FileHeader) *model.AppError { file, err := imageData.Open() if err != nil { return model.NewAppError("SetBotIconImage", "api.bot.set_bot_icon_image.open.app_error", nil, err.Error(), http.StatusBadRequest) } defer file.Close() - if _, err = parseSVG(file); err != nil { + file.Seek(0, 0) + return a.SetBotIconImage(botUserId, file) +} + +// SetBotIconImage sets LHS icon for a bot. +func (a *App) SetBotIconImage(botUserId string, file io.ReadSeeker) *model.AppError { + if _, err := a.GetBot(botUserId, true); err != nil { + return err + } + + if _, err := parseSVG(file); err != nil { return model.NewAppError("SetBotIconImage", "api.bot.set_bot_icon_image.parse.app_error", nil, err.Error(), http.StatusBadRequest) } @@ -229,8 +236,8 @@ func (a *App) SetBotIconImage(botUserId string, imageData *multipart.FileHeader) // DeleteBotIconImage deletes LHS icon for a bot. func (a *App) DeleteBotIconImage(botUserId string) *model.AppError { - if len(*a.Config().FileSettings.DriverName) == 0 { - return model.NewAppError("DeleteBotIconImage", "api.bot.icon_image.storage.app_error", nil, "", http.StatusNotImplemented) + if _, err := a.GetBot(botUserId, true); err != nil { + return err } // Delete icon @@ -247,17 +254,17 @@ func (a *App) DeleteBotIconImage(botUserId string) *model.AppError { } // GetBotIconImage retrieves LHS icon for a bot. -func (a *App) GetBotIconImage(botUserId string) ([]byte, bool, *model.AppError) { - if len(*a.Config().FileSettings.DriverName) == 0 { - return nil, false, model.NewAppError("GetBotIconImage", "api.bot.icon_image.storage.app_error", nil, "", http.StatusNotImplemented) +func (a *App) GetBotIconImage(botUserId string) ([]byte, *model.AppError) { + if _, err := a.GetBot(botUserId, true); err != nil { + return nil, err } data, err := a.ReadFile(getBotIconPath(botUserId)) if err != nil { - return nil, false, model.NewAppError("GetBotIconImage", "api.bot.get_bot_icon_image.read.app_error", nil, err.Error(), http.StatusNotFound) + return nil, model.NewAppError("GetBotIconImage", "api.bot.get_bot_icon_image.read.app_error", nil, err.Error(), http.StatusNotFound) } - return data, false, nil + return data, nil } func getBotIconPath(botUserId string) string { diff --git a/app/bot_test.go b/app/bot_test.go index 3c955356b1..3d06a329eb 100644 --- a/app/bot_test.go +++ b/app/bot_test.go @@ -5,6 +5,9 @@ package app import ( "fmt" + "io/ioutil" + "os" + "path/filepath" "strings" "testing" @@ -12,6 +15,7 @@ import ( "github.com/stretchr/testify/require" "github.com/mattermost/mattermost-server/model" + "github.com/mattermost/mattermost-server/utils/fileutils" ) func TestCreateBot(t *testing.T) { @@ -596,6 +600,165 @@ func TestConvertUserToBot(t *testing.T) { }) } +func TestSetBotIconImage(t *testing.T) { + t.Run("invalid bot", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + path, _ := fileutils.FindDir("tests") + svgFile, fileErr := os.Open(filepath.Join(path, "test.svg")) + require.NoError(t, fileErr) + defer svgFile.Close() + + err := th.App.SetBotIconImage("invalid_bot_id", svgFile) + require.NotNil(t, err) + }) + + t.Run("valid bot", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + // Set an icon image + path, _ := fileutils.FindDir("tests") + svgFile, fileErr := os.Open(filepath.Join(path, "test.svg")) + require.NoError(t, fileErr) + defer svgFile.Close() + + expectedData, fileErr := ioutil.ReadAll(svgFile) + require.Nil(t, fileErr) + require.NotNil(t, expectedData) + + bot, err := th.App.ConvertUserToBot(&model.User{ + Username: "username", + Id: th.BasicUser.Id, + }) + defer th.App.PermanentDeleteBot(bot.UserId) + + fpath := fmt.Sprintf("/bots/%v/icon.svg", bot.UserId) + exists, err := th.App.FileExists(fpath) + require.Nil(t, err) + require.False(t, exists, "icon.svg shouldn't exist for the bot") + + svgFile.Seek(0, 0) + err = th.App.SetBotIconImage(bot.UserId, svgFile) + require.Nil(t, err) + + exists, err = th.App.FileExists(fpath) + require.Nil(t, err) + require.True(t, exists, "icon.svg should exist for the bot") + + actualData, err := th.App.ReadFile(fpath) + require.Nil(t, err) + require.NotNil(t, actualData) + + require.Equal(t, expectedData, actualData) + }) +} + +func TestGetBotIconImage(t *testing.T) { + t.Run("invalid bot", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + actualData, err := th.App.GetBotIconImage("invalid_bot_id") + require.NotNil(t, err) + require.Nil(t, actualData) + }) + + t.Run("valid bot", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + // Set an icon image + path, _ := fileutils.FindDir("tests") + svgFile, fileErr := os.Open(filepath.Join(path, "test.svg")) + require.NoError(t, fileErr) + defer svgFile.Close() + + expectedData, fileErr := ioutil.ReadAll(svgFile) + require.Nil(t, fileErr) + require.NotNil(t, expectedData) + + bot, err := th.App.ConvertUserToBot(&model.User{ + Username: "username", + Id: th.BasicUser.Id, + }) + defer th.App.PermanentDeleteBot(bot.UserId) + + svgFile.Seek(0, 0) + fpath := fmt.Sprintf("/bots/%v/icon.svg", bot.UserId) + _, err = th.App.WriteFile(svgFile, fpath) + require.Nil(t, err) + + actualBytes, err := th.App.GetBotIconImage(bot.UserId) + require.Nil(t, err) + require.NotNil(t, actualBytes) + + actualData, err := th.App.ReadFile(fpath) + require.Nil(t, err) + require.NotNil(t, actualData) + + require.Equal(t, expectedData, actualData) + }) +} + +func TestDeleteBotIconImage(t *testing.T) { + t.Run("invalid bot", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + err := th.App.DeleteBotIconImage("invalid_bot_id") + require.NotNil(t, err) + }) + + t.Run("valid bot", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + // Set an icon image + path, _ := fileutils.FindDir("tests") + svgFile, fileErr := os.Open(filepath.Join(path, "test.svg")) + require.NoError(t, fileErr) + defer svgFile.Close() + + expectedData, fileErr := ioutil.ReadAll(svgFile) + require.Nil(t, fileErr) + require.NotNil(t, expectedData) + + bot, err := th.App.ConvertUserToBot(&model.User{ + Username: "username", + Id: th.BasicUser.Id, + }) + defer th.App.PermanentDeleteBot(bot.UserId) + + // Set icon + svgFile.Seek(0, 0) + err = th.App.SetBotIconImage(bot.UserId, svgFile) + require.Nil(t, err) + + // Get icon + actualData, err := th.App.GetBotIconImage(bot.UserId) + require.Nil(t, err) + require.NotNil(t, actualData) + require.Equal(t, expectedData, actualData) + + // Bot icon should exist + fpath := fmt.Sprintf("/bots/%v/icon.svg", bot.UserId) + exists, err := th.App.FileExists(fpath) + require.Nil(t, err) + require.True(t, exists, "icon.svg should exist for the bot") + + // Delete icon + err = th.App.DeleteBotIconImage(bot.UserId) + require.Nil(t, err) + + // Bot icon should not exist + exists, err = th.App.FileExists(fpath) + require.Nil(t, err) + require.False(t, exists, "icon.svg should be deleted for the bot") + }) +} + func sToP(s string) *string { return &s } diff --git a/app/plugin_api.go b/app/plugin_api.go index fb24940a2c..d8c60abe77 100644 --- a/app/plugin_api.go +++ b/app/plugin_api.go @@ -500,12 +500,7 @@ func (api *PluginAPI) SetProfileImage(userId string, data []byte) *model.AppErro return err } - fileReader := bytes.NewReader(data) - err = api.app.SetProfileImageFromFile(userId, fileReader) - if err != nil { - return err - } - return nil + return api.app.SetProfileImageFromFile(userId, bytes.NewReader(data)) } func (api *PluginAPI) GetEmojiList(sortBy string, page, perPage int) ([]*model.Emoji, *model.AppError) { @@ -580,12 +575,7 @@ func (api *PluginAPI) SetTeamIcon(teamId string, data []byte) *model.AppError { return err } - fileReader := bytes.NewReader(data) - err = api.app.SetTeamIconFromFile(team, fileReader) - if err != nil { - return err - } - return nil + return api.app.SetTeamIconFromFile(team, bytes.NewReader(data)) } func (api *PluginAPI) OpenInteractiveDialog(dialog model.OpenDialogRequest) *model.AppError { @@ -756,3 +746,27 @@ func (api *PluginAPI) UpdateBotActive(userId string, active bool) (*model.Bot, * func (api *PluginAPI) PermanentDeleteBot(userId string) *model.AppError { return api.app.PermanentDeleteBot(userId) } + +func (api *PluginAPI) GetBotIconImage(userId string) ([]byte, *model.AppError) { + if _, err := api.app.GetBot(userId, true); err != nil { + return nil, err + } + + return api.app.GetBotIconImage(userId) +} + +func (api *PluginAPI) SetBotIconImage(userId string, data []byte) *model.AppError { + if _, err := api.app.GetBot(userId, true); err != nil { + return err + } + + return api.app.SetBotIconImage(userId, bytes.NewReader(data)) +} + +func (api *PluginAPI) DeleteBotIconImage(userId string) *model.AppError { + if _, err := api.app.GetBot(userId, true); err != nil { + return err + } + + return api.app.DeleteBotIconImage(userId) +} diff --git a/i18n/en.json b/i18n/en.json index d826ff26dd..76ddaba666 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -143,10 +143,6 @@ "id": "api.bot.get_bot_icon_image.read.app_error", "translation": "Unable to read icon image file" }, - { - "id": "api.bot.icon_image.storage.app_error", - "translation": "Image storage is not configured." - }, { "id": "api.bot.set_bot_icon_image.app_error", "translation": "Couldn't upload icon image" diff --git a/model/client4.go b/model/client4.go index 96841dde39..1f64135802 100644 --- a/model/client4.go +++ b/model/client4.go @@ -1497,7 +1497,7 @@ func (c *Client4) AssignBot(botUserId, newOwnerId string) (*Bot, *Response) { return BotFromJson(r.Body), BuildResponse(r) } -// SetBotIconImage sets icon image of the user. +// SetBotIconImage sets LHS bot icon image. func (c *Client4) SetBotIconImage(botUserId string, data []byte) (bool, *Response) { body := &bytes.Buffer{} writer := multipart.NewWriter(body) @@ -1538,7 +1538,7 @@ func (c *Client4) SetBotIconImage(botUserId string, data []byte) (bool, *Respons return CheckStatusOK(rp), BuildResponse(rp) } -// GetBotIconImage gets user's LHS icon image. Must be logged in. +// GetBotIconImage gets LHS bot icon image. Must be logged in. func (c *Client4) GetBotIconImage(botUserId string) ([]byte, *Response) { r, appErr := c.DoApiGet(c.GetBotRoute(botUserId)+"/icon", "") if appErr != nil { @@ -1553,7 +1553,7 @@ func (c *Client4) GetBotIconImage(botUserId string) ([]byte, *Response) { return data, BuildResponse(r) } -// DeleteBotIconImage deletes user's LHS icon image. Must be logged in. +// DeleteBotIconImage deletes LHS bot icon image. Must be logged in. func (c *Client4) DeleteBotIconImage(botUserId string) (bool, *Response) { r, appErr := c.DoApiDelete(c.GetBotRoute(botUserId) + "/icon") if appErr != nil { @@ -3586,7 +3586,7 @@ func (c *Client4) GetBrandImage() ([]byte, *Response) { return data, BuildResponse(r) } -// DeleteBrandImage delets the brand image for the system. +// DeleteBrandImage deletes the brand image for the system. func (c *Client4) DeleteBrandImage() *Response { r, err := c.DoApiDelete(c.GetBrandRoute() + "/image") if err != nil { diff --git a/plugin/api.go b/plugin/api.go index f9e29c7058..8b652d3684 100644 --- a/plugin/api.go +++ b/plugin/api.go @@ -569,6 +569,22 @@ type API interface { // // Minimum server version: 5.10 PermanentDeleteBot(botUserId string) *model.AppError + + // GetBotIconImage gets LHS bot icon image. + // + // Minimum server version: 5.14 + GetBotIconImage(botUserId string) ([]byte, *model.AppError) + + // SetBotIconImage sets LHS bot icon image. + // Icon image must be SVG format, all other formats are rejected. + // + // Minimum server version: 5.14 + SetBotIconImage(botUserId string, data []byte) *model.AppError + + // DeleteBotIconImage deletes LHS bot icon image. + // + // Minimum server version: 5.14 + DeleteBotIconImage(botUserId string) *model.AppError } var handshake = plugin.HandshakeConfig{ diff --git a/plugin/client_rpc_generated.go b/plugin/client_rpc_generated.go index 73d4cc37a7..3634e72bce 100644 --- a/plugin/client_rpc_generated.go +++ b/plugin/client_rpc_generated.go @@ -4100,3 +4100,89 @@ func (s *apiRPCServer) PermanentDeleteBot(args *Z_PermanentDeleteBotArgs, return } return nil } + +type Z_GetBotIconImageArgs struct { + A string +} + +type Z_GetBotIconImageReturns struct { + A []byte + B *model.AppError +} + +func (g *apiRPCClient) GetBotIconImage(botUserId string) ([]byte, *model.AppError) { + _args := &Z_GetBotIconImageArgs{botUserId} + _returns := &Z_GetBotIconImageReturns{} + if err := g.client.Call("Plugin.GetBotIconImage", _args, _returns); err != nil { + log.Printf("RPC call to GetBotIconImage API failed: %s", err.Error()) + } + return _returns.A, _returns.B +} + +func (s *apiRPCServer) GetBotIconImage(args *Z_GetBotIconImageArgs, returns *Z_GetBotIconImageReturns) error { + if hook, ok := s.impl.(interface { + GetBotIconImage(botUserId string) ([]byte, *model.AppError) + }); ok { + returns.A, returns.B = hook.GetBotIconImage(args.A) + } else { + return encodableError(fmt.Errorf("API GetBotIconImage called but not implemented.")) + } + return nil +} + +type Z_SetBotIconImageArgs struct { + A string + B []byte +} + +type Z_SetBotIconImageReturns struct { + A *model.AppError +} + +func (g *apiRPCClient) SetBotIconImage(botUserId string, data []byte) *model.AppError { + _args := &Z_SetBotIconImageArgs{botUserId, data} + _returns := &Z_SetBotIconImageReturns{} + if err := g.client.Call("Plugin.SetBotIconImage", _args, _returns); err != nil { + log.Printf("RPC call to SetBotIconImage API failed: %s", err.Error()) + } + return _returns.A +} + +func (s *apiRPCServer) SetBotIconImage(args *Z_SetBotIconImageArgs, returns *Z_SetBotIconImageReturns) error { + if hook, ok := s.impl.(interface { + SetBotIconImage(botUserId string, data []byte) *model.AppError + }); ok { + returns.A = hook.SetBotIconImage(args.A, args.B) + } else { + return encodableError(fmt.Errorf("API SetBotIconImage called but not implemented.")) + } + return nil +} + +type Z_DeleteBotIconImageArgs struct { + A string +} + +type Z_DeleteBotIconImageReturns struct { + A *model.AppError +} + +func (g *apiRPCClient) DeleteBotIconImage(botUserId string) *model.AppError { + _args := &Z_DeleteBotIconImageArgs{botUserId} + _returns := &Z_DeleteBotIconImageReturns{} + if err := g.client.Call("Plugin.DeleteBotIconImage", _args, _returns); err != nil { + log.Printf("RPC call to DeleteBotIconImage API failed: %s", err.Error()) + } + return _returns.A +} + +func (s *apiRPCServer) DeleteBotIconImage(args *Z_DeleteBotIconImageArgs, returns *Z_DeleteBotIconImageReturns) error { + if hook, ok := s.impl.(interface { + DeleteBotIconImage(botUserId string) *model.AppError + }); ok { + returns.A = hook.DeleteBotIconImage(args.A) + } else { + return encodableError(fmt.Errorf("API DeleteBotIconImage called but not implemented.")) + } + return nil +} diff --git a/plugin/plugintest/api.go b/plugin/plugintest/api.go index af04132442..768e7635f9 100644 --- a/plugin/plugintest/api.go +++ b/plugin/plugintest/api.go @@ -262,6 +262,22 @@ func (_m *API) CreateUser(user *model.User) (*model.User, *model.AppError) { return r0, r1 } +// DeleteBotIconImage provides a mock function with given fields: botUserId +func (_m *API) DeleteBotIconImage(botUserId string) *model.AppError { + ret := _m.Called(botUserId) + + var r0 *model.AppError + if rf, ok := ret.Get(0).(func(string) *model.AppError); ok { + r0 = rf(botUserId) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*model.AppError) + } + } + + return r0 +} + // DeleteChannel provides a mock function with given fields: channelId func (_m *API) DeleteChannel(channelId string) *model.AppError { ret := _m.Called(channelId) @@ -420,6 +436,31 @@ func (_m *API) GetBot(botUserId string, includeDeleted bool) (*model.Bot, *model return r0, r1 } +// GetBotIconImage provides a mock function with given fields: botUserId +func (_m *API) GetBotIconImage(botUserId string) ([]byte, *model.AppError) { + ret := _m.Called(botUserId) + + var r0 []byte + if rf, ok := ret.Get(0).(func(string) []byte); ok { + r0 = rf(botUserId) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]byte) + } + } + + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(string) *model.AppError); ok { + r1 = rf(botUserId) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 +} + // GetBots provides a mock function with given fields: options func (_m *API) GetBots(options *model.BotGetOptions) ([]*model.Bot, *model.AppError) { ret := _m.Called(options) @@ -2358,6 +2399,22 @@ func (_m *API) SendMail(to string, subject string, htmlBody string) *model.AppEr return r0 } +// SetBotIconImage provides a mock function with given fields: botUserId, data +func (_m *API) SetBotIconImage(botUserId string, data []byte) *model.AppError { + ret := _m.Called(botUserId, data) + + var r0 *model.AppError + if rf, ok := ret.Get(0).(func(string, []byte) *model.AppError); ok { + r0 = rf(botUserId, data) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*model.AppError) + } + } + + return r0 +} + // SetProfileImage provides a mock function with given fields: userId, data func (_m *API) SetProfileImage(userId string, data []byte) *model.AppError { ret := _m.Called(userId, data)