From 0118db9d2385d9244413478dbaa8123fdcee3a01 Mon Sep 17 00:00:00 2001 From: Rodrigo Villablanca Date: Fri, 26 Jun 2020 00:26:35 -0400 Subject: [PATCH] Migration of AuditStore to return plain errors (#14825) Automatic Merge --- app/audit.go | 27 ++++++++++++++++++++-- app/channel.go | 12 +++++----- app/terms_of_service.go | 4 ++-- app/user.go | 2 +- i18n/en.json | 36 +++++++++++++---------------- store/errors.go | 14 +++++++++++ store/opentracing_layer.go | 6 ++--- store/sqlstore/audit_store.go | 23 +++++++++--------- store/store.go | 6 ++--- store/storetest/mocks/AuditStore.go | 30 ++++++++++-------------- store/timer_layer.go | 6 ++--- web/context.go | 6 +++-- 12 files changed, 100 insertions(+), 72 deletions(-) diff --git a/app/audit.go b/app/audit.go index 9345b0781d..1b210caad8 100644 --- a/app/audit.go +++ b/app/audit.go @@ -4,12 +4,15 @@ package app import ( + "errors" "fmt" + "net/http" "os/user" "github.com/mattermost/mattermost-server/v5/audit" "github.com/mattermost/mattermost-server/v5/mlog" "github.com/mattermost/mattermost-server/v5/model" + "github.com/mattermost/mattermost-server/v5/store" ) const ( @@ -27,11 +30,31 @@ var ( ) func (a *App) GetAudits(userId string, limit int) (model.Audits, *model.AppError) { - return a.Srv().Store.Audit().Get(userId, 0, limit) + audits, err := a.Srv().Store.Audit().Get(userId, 0, limit) + if err != nil { + var outErr *store.ErrOutOfBounds + switch { + case errors.As(err, &outErr): + return nil, model.NewAppError("GetAudits", "app.audit.get.limit.app_error", nil, err.Error(), http.StatusBadRequest) + default: + return nil, model.NewAppError("GetAudits", "app.audit.get.finding.app_error", nil, err.Error(), http.StatusInternalServerError) + } + } + return audits, nil } func (a *App) GetAuditsPage(userId string, page int, perPage int) (model.Audits, *model.AppError) { - return a.Srv().Store.Audit().Get(userId, page*perPage, perPage) + audits, err := a.Srv().Store.Audit().Get(userId, page*perPage, perPage) + if err != nil { + var outErr *store.ErrOutOfBounds + switch { + case errors.As(err, &outErr): + return nil, model.NewAppError("GetAuditsPage", "app.audit.get.limit.app_error", nil, err.Error(), http.StatusBadRequest) + default: + return nil, model.NewAppError("GetAuditsPage", "app.audit.get.finding.app_error", nil, err.Error(), http.StatusInternalServerError) + } + } + return audits, nil } // LogAuditRec logs an audit record using default CLILevel. diff --git a/app/channel.go b/app/channel.go index b01a7092c0..cad4b44c71 100644 --- a/app/channel.go +++ b/app/channel.go @@ -536,10 +536,10 @@ func (a *App) UpdateChannel(channel *model.Channel) (*model.Channel, *model.AppE _, err := a.Srv().Store.Channel().Update(channel) if err != nil { var appErr *model.AppError - var iErr *store.ErrInvalidInput + var invErr *store.ErrInvalidInput switch { - case errors.As(err, &iErr): - return nil, model.NewAppError("UpdateChannel", "app.channel.update.bad_id", nil, iErr.Error(), http.StatusBadRequest) + case errors.As(err, &invErr): + return nil, model.NewAppError("UpdateChannel", "app.channel.update.bad_id", nil, invErr.Error(), http.StatusBadRequest) case errors.As(err, &appErr): return nil, appErr default: @@ -2382,10 +2382,10 @@ func (a *App) MoveChannel(team *model.Team, channel *model.Channel, user *model. channel.TeamId = team.Id if _, err := a.Srv().Store.Channel().Update(channel); err != nil { var appErr *model.AppError - var iErr *store.ErrInvalidInput + var invErr *store.ErrInvalidInput switch { - case errors.As(err, &iErr): - return model.NewAppError("MoveChannel", "app.channel.update.bad_id", nil, iErr.Error(), http.StatusBadRequest) + case errors.As(err, &invErr): + return model.NewAppError("MoveChannel", "app.channel.update.bad_id", nil, invErr.Error(), http.StatusBadRequest) case errors.As(err, &appErr): return appErr default: diff --git a/app/terms_of_service.go b/app/terms_of_service.go index d52086a02a..5df07124fc 100644 --- a/app/terms_of_service.go +++ b/app/terms_of_service.go @@ -23,10 +23,10 @@ func (a *App) CreateTermsOfService(text, userId string) (*model.TermsOfService, var err error if termsOfService, err = a.Srv().Store.TermsOfService().Save(termsOfService); err != nil { - var iErr *store.ErrInvalidInput + var invErr *store.ErrInvalidInput var appErr *model.AppError switch { - case errors.As(err, &iErr): + case errors.As(err, &invErr): return nil, model.NewAppError("CreateTermsOfService", "app.terms_of_service.create.existing.app_error", nil, "id="+termsOfService.Id, http.StatusBadRequest) case errors.As(err, &appErr): return nil, appErr diff --git a/app/user.go b/app/user.go index 1de9b27bcd..ddc8233266 100644 --- a/app/user.go +++ b/app/user.go @@ -1533,7 +1533,7 @@ func (a *App) PermanentDeleteUser(user *model.User) *model.AppError { } if err := a.Srv().Store.Audit().PermanentDeleteByUser(user.Id); err != nil { - return err + return model.NewAppError("PermanentDeleteUser", "app.audit.permanent_delete_by_user.app_error", nil, err.Error(), http.StatusInternalServerError) } if err := a.Srv().Store.Team().RemoveAllMembersByUser(user.Id); err != nil { diff --git a/i18n/en.json b/i18n/en.json index 9f8985ee8e..d12f7a6d59 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -2962,6 +2962,22 @@ "id": "app.admin.test_site_url.failure", "translation": "This is not a valid live URL" }, + { + "id": "app.audit.get.finding.app_error", + "translation": "We encountered an error finding the audits." + }, + { + "id": "app.audit.get.limit.app_error", + "translation": "Limit exceeded for paging." + }, + { + "id": "app.audit.permanent_delete_by_user.app_error", + "translation": "We encountered an error deleting the audits." + }, + { + "id": "app.audit.save.saving.app_error", + "translation": "We encountered an error saving the audit." + }, { "id": "app.bot.createbot.internal_error", "translation": "Unable to save the bot." @@ -6174,26 +6190,6 @@ "id": "store.sql.convert_string_map", "translation": "FromDb: Unable to convert StringMap to *string" }, - { - "id": "store.sql_audit.app_error", - "translation": "Failed to build query." - }, - { - "id": "store.sql_audit.get.finding.app_error", - "translation": "We encountered an error finding the audits." - }, - { - "id": "store.sql_audit.get.limit.app_error", - "translation": "Limit exceeded for paging." - }, - { - "id": "store.sql_audit.permanent_delete_by_user.app_error", - "translation": "We encountered an error deleting the audits." - }, - { - "id": "store.sql_audit.save.saving.app_error", - "translation": "We encountered an error saving the audit." - }, { "id": "store.sql_bot.get.missing.app_error", "translation": "Bot does not exist." diff --git a/store/errors.go b/store/errors.go index d2e66a5e64..ad2e27847b 100644 --- a/store/errors.go +++ b/store/errors.go @@ -88,3 +88,17 @@ func NewErrNotFound(resource, id string) *ErrNotFound { func (e *ErrNotFound) Error() string { return "resource: " + e.resource + " id: " + e.Id } + +// ErrOutOfBounds indicates that the requested total numbers of rows +// was greater than the allowed limit. +type ErrOutOfBounds struct { + value int +} + +func (e *ErrOutOfBounds) Error() string { + return fmt.Sprintf("invalid limit parameter: %d", e.value) +} + +func NewErrOutOfBounds(value int) *ErrOutOfBounds { + return &ErrOutOfBounds{value: value} +} diff --git a/store/opentracing_layer.go b/store/opentracing_layer.go index 1aabf62f26..02d67e2b3f 100644 --- a/store/opentracing_layer.go +++ b/store/opentracing_layer.go @@ -329,7 +329,7 @@ type OpenTracingLayerWebhookStore struct { Root *OpenTracingLayer } -func (s *OpenTracingLayerAuditStore) Get(user_id string, offset int, limit int) (model.Audits, *model.AppError) { +func (s *OpenTracingLayerAuditStore) Get(user_id string, offset int, limit int) (model.Audits, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "AuditStore.Get") s.Root.Store.SetContext(newCtx) @@ -347,7 +347,7 @@ func (s *OpenTracingLayerAuditStore) Get(user_id string, offset int, limit int) return resultVar0, resultVar1 } -func (s *OpenTracingLayerAuditStore) PermanentDeleteByUser(userId string) *model.AppError { +func (s *OpenTracingLayerAuditStore) PermanentDeleteByUser(userId string) error { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "AuditStore.PermanentDeleteByUser") s.Root.Store.SetContext(newCtx) @@ -365,7 +365,7 @@ func (s *OpenTracingLayerAuditStore) PermanentDeleteByUser(userId string) *model return resultVar0 } -func (s *OpenTracingLayerAuditStore) Save(audit *model.Audit) *model.AppError { +func (s *OpenTracingLayerAuditStore) Save(audit *model.Audit) error { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "AuditStore.Save") s.Root.Store.SetContext(newCtx) diff --git a/store/sqlstore/audit_store.go b/store/sqlstore/audit_store.go index 921732449f..b33e8698c2 100644 --- a/store/sqlstore/audit_store.go +++ b/store/sqlstore/audit_store.go @@ -4,9 +4,8 @@ package sqlstore import ( - "net/http" - sq "github.com/Masterminds/squirrel" + "github.com/pkg/errors" "github.com/mattermost/mattermost-server/v5/model" "github.com/mattermost/mattermost-server/v5/store" @@ -36,19 +35,19 @@ func (s SqlAuditStore) createIndexesIfNotExists() { s.CreateIndexIfNotExists("idx_audits_user_id", "Audits", "UserId") } -func (s SqlAuditStore) Save(audit *model.Audit) *model.AppError { +func (s SqlAuditStore) Save(audit *model.Audit) error { audit.Id = model.NewId() audit.CreateAt = model.GetMillis() if err := s.GetMaster().Insert(audit); err != nil { - return model.NewAppError("SqlAuditStore.Save", "store.sql_audit.save.saving.app_error", nil, "user_id="+audit.UserId+" action="+audit.Action, http.StatusInternalServerError) + return errors.Wrapf(err, "failed to save Audit with userId=%s and action=%s", audit.UserId, audit.Action) } return nil } -func (s SqlAuditStore) Get(user_id string, offset int, limit int) (model.Audits, *model.AppError) { +func (s SqlAuditStore) Get(userId string, offset int, limit int) (model.Audits, error) { if limit > 1000 { - return nil, model.NewAppError("SqlAuditStore.Get", "store.sql_audit.get.limit.app_error", nil, "user_id="+user_id, http.StatusBadRequest) + return nil, store.NewErrOutOfBounds(limit) } query := s.getQueryBuilder(). @@ -58,26 +57,26 @@ func (s SqlAuditStore) Get(user_id string, offset int, limit int) (model.Audits, Limit(uint64(limit)). Offset(uint64(offset)) - if len(user_id) != 0 { - query = query.Where(sq.Eq{"UserId": user_id}) + if len(userId) != 0 { + query = query.Where(sq.Eq{"UserId": userId}) } queryString, args, err := query.ToSql() if err != nil { - return nil, model.NewAppError("SqlAuditStore.Get", "store.sql_audit.app_error", nil, err.Error(), http.StatusInternalServerError) + return nil, errors.Wrap(err, "audits_tosql") } var audits model.Audits if _, err := s.GetReplica().Select(&audits, queryString, args...); err != nil { - return nil, model.NewAppError("SqlAuditStore.Get", "store.sql_audit.get.finding.app_error", nil, "user_id="+user_id, http.StatusInternalServerError) + return nil, errors.Wrapf(err, "failed to get Audit list for userId=%s", userId) } return audits, nil } -func (s SqlAuditStore) PermanentDeleteByUser(userId string) *model.AppError { +func (s SqlAuditStore) PermanentDeleteByUser(userId string) error { if _, err := s.GetMaster().Exec("DELETE FROM Audits WHERE UserId = :userId", map[string]interface{}{"userId": userId}); err != nil { - return model.NewAppError("SqlAuditStore.Delete", "store.sql_audit.permanent_delete_by_user.app_error", nil, "user_id="+userId, http.StatusInternalServerError) + return errors.Wrapf(err, "failed to delete Audit with userId=%s", userId) } return nil } diff --git a/store/store.go b/store/store.go index 169ced5d51..a1fd293add 100644 --- a/store/store.go +++ b/store/store.go @@ -374,9 +374,9 @@ type SessionStore interface { } type AuditStore interface { - Save(audit *model.Audit) *model.AppError - Get(user_id string, offset int, limit int) (model.Audits, *model.AppError) - PermanentDeleteByUser(userId string) *model.AppError + Save(audit *model.Audit) error + Get(user_id string, offset int, limit int) (model.Audits, error) + PermanentDeleteByUser(userId string) error } type ClusterDiscoveryStore interface { diff --git a/store/storetest/mocks/AuditStore.go b/store/storetest/mocks/AuditStore.go index a00df042de..b33fd5a6f6 100644 --- a/store/storetest/mocks/AuditStore.go +++ b/store/storetest/mocks/AuditStore.go @@ -15,7 +15,7 @@ type AuditStore struct { } // Get provides a mock function with given fields: user_id, offset, limit -func (_m *AuditStore) Get(user_id string, offset int, limit int) (model.Audits, *model.AppError) { +func (_m *AuditStore) Get(user_id string, offset int, limit int) (model.Audits, error) { ret := _m.Called(user_id, offset, limit) var r0 model.Audits @@ -27,45 +27,39 @@ func (_m *AuditStore) Get(user_id string, offset int, limit int) (model.Audits, } } - var r1 *model.AppError - if rf, ok := ret.Get(1).(func(string, int, int) *model.AppError); ok { + var r1 error + if rf, ok := ret.Get(1).(func(string, int, int) error); ok { r1 = rf(user_id, offset, limit) } else { - if ret.Get(1) != nil { - r1 = ret.Get(1).(*model.AppError) - } + r1 = ret.Error(1) } return r0, r1 } // PermanentDeleteByUser provides a mock function with given fields: userId -func (_m *AuditStore) PermanentDeleteByUser(userId string) *model.AppError { +func (_m *AuditStore) PermanentDeleteByUser(userId string) error { ret := _m.Called(userId) - var r0 *model.AppError - if rf, ok := ret.Get(0).(func(string) *model.AppError); ok { + var r0 error + if rf, ok := ret.Get(0).(func(string) error); ok { r0 = rf(userId) } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(*model.AppError) - } + r0 = ret.Error(0) } return r0 } // Save provides a mock function with given fields: audit -func (_m *AuditStore) Save(audit *model.Audit) *model.AppError { +func (_m *AuditStore) Save(audit *model.Audit) error { ret := _m.Called(audit) - var r0 *model.AppError - if rf, ok := ret.Get(0).(func(*model.Audit) *model.AppError); ok { + var r0 error + if rf, ok := ret.Get(0).(func(*model.Audit) error); ok { r0 = rf(audit) } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(*model.AppError) - } + r0 = ret.Error(0) } return r0 diff --git a/store/timer_layer.go b/store/timer_layer.go index be80666668..cca5afd016 100644 --- a/store/timer_layer.go +++ b/store/timer_layer.go @@ -329,7 +329,7 @@ type TimerLayerWebhookStore struct { Root *TimerLayer } -func (s *TimerLayerAuditStore) Get(user_id string, offset int, limit int) (model.Audits, *model.AppError) { +func (s *TimerLayerAuditStore) Get(user_id string, offset int, limit int) (model.Audits, error) { start := timemodule.Now() resultVar0, resultVar1 := s.AuditStore.Get(user_id, offset, limit) @@ -345,7 +345,7 @@ func (s *TimerLayerAuditStore) Get(user_id string, offset int, limit int) (model return resultVar0, resultVar1 } -func (s *TimerLayerAuditStore) PermanentDeleteByUser(userId string) *model.AppError { +func (s *TimerLayerAuditStore) PermanentDeleteByUser(userId string) error { start := timemodule.Now() resultVar0 := s.AuditStore.PermanentDeleteByUser(userId) @@ -361,7 +361,7 @@ func (s *TimerLayerAuditStore) PermanentDeleteByUser(userId string) *model.AppEr return resultVar0 } -func (s *TimerLayerAuditStore) Save(audit *model.Audit) *model.AppError { +func (s *TimerLayerAuditStore) Save(audit *model.Audit) error { start := timemodule.Now() resultVar0 := s.AuditStore.Save(audit) diff --git a/web/context.go b/web/context.go index fc66e7adfa..828120de93 100644 --- a/web/context.go +++ b/web/context.go @@ -65,7 +65,8 @@ func (c *Context) MakeAuditRecord(event string, initialStatus string) *audit.Rec func (c *Context) LogAudit(extraInfo string) { audit := &model.Audit{UserId: c.App.Session().UserId, IpAddress: c.App.IpAddress(), Action: c.App.Path(), ExtraInfo: extraInfo, SessionId: c.App.Session().Id} if err := c.App.Srv().Store.Audit().Save(audit); err != nil { - c.LogError(err) + appErr := model.NewAppError("LogAudit", "app.audit.save.saving.app_error", nil, err.Error(), http.StatusInternalServerError) + c.LogError(appErr) } } @@ -77,7 +78,8 @@ func (c *Context) LogAuditWithUserId(userId, extraInfo string) { audit := &model.Audit{UserId: userId, IpAddress: c.App.IpAddress(), Action: c.App.Path(), ExtraInfo: extraInfo, SessionId: c.App.Session().Id} if err := c.App.Srv().Store.Audit().Save(audit); err != nil { - c.LogError(err) + appErr := model.NewAppError("LogAuditWithUserId", "app.audit.save.saving.app_error", nil, err.Error(), http.StatusInternalServerError) + c.LogError(appErr) } }