From 50fa5f0f3e75b3bf3929afecd8e61cdea2694da4 Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Tue, 28 Jul 2020 14:06:41 +0530 Subject: [PATCH] MM-27275: Log the correct error when GetUser fails in MFA Authentication (#15115) * MM-27275: Log the correct error when GetUser fails in MFA Authentication We were not logging the error returned from GetUser which makes debugging things very hard in case of logout issues like https://mattermost.atlassian.net/browse/MM-27270. We fix this by appending the error inside model.AppError and setting a proper error string which says exactly what has happened. * Update web/context.go Co-authored-by: Doug Lauder Co-authored-by: Doug Lauder Co-authored-by: Mattermod --- i18n/en.json | 4 ++++ web/context.go | 51 +++++++++++++++++++++++---------------------- web/context_test.go | 39 ++++++++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 25 deletions(-) diff --git a/i18n/en.json b/i18n/en.json index 2c82d22fe0..ccd7c81332 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -1130,6 +1130,10 @@ "id": "api.context.404.app_error", "translation": "Sorry, we could not find the page." }, + { + "id": "api.context.get_user.app_error", + "translation": "Unable to get user from session UserID." + }, { "id": "api.context.invalid_body_param.app_error", "translation": "Invalid or missing {{.Name}} in request body." diff --git a/web/context.go b/web/context.go index 9c25854d3c..32e1eed644 100644 --- a/web/context.go +++ b/web/context.go @@ -154,35 +154,36 @@ func (c *Context) MfaRequired() { return } - if user, err := c.App.GetUser(c.App.Session().UserId); err != nil { - c.Err = model.NewAppError("", "api.context.session_expired.app_error", nil, "MfaRequired", http.StatusUnauthorized) + user, err := c.App.GetUser(c.App.Session().UserId) + if err != nil { + c.Err = model.NewAppError("MfaRequired", "api.context.get_user.app_error", nil, err.Error(), http.StatusUnauthorized) return - } else { - if user.IsGuest() && !*c.App.Config().GuestAccountsSettings.EnforceMultifactorAuthentication { - return - } - // Only required for email and ldap accounts - if user.AuthService != "" && - user.AuthService != model.USER_AUTH_SERVICE_EMAIL && - user.AuthService != model.USER_AUTH_SERVICE_LDAP { - return - } + } - // Special case to let user get themself - subpath, _ := utils.GetSubpathFromConfig(c.App.Config()) - if c.App.Path() == path.Join(subpath, "/api/v4/users/me") { - return - } + if user.IsGuest() && !*c.App.Config().GuestAccountsSettings.EnforceMultifactorAuthentication { + return + } + // Only required for email and ldap accounts + if user.AuthService != "" && + user.AuthService != model.USER_AUTH_SERVICE_EMAIL && + user.AuthService != model.USER_AUTH_SERVICE_LDAP { + return + } - // Bots are exempt - if user.IsBot { - return - } + // Special case to let user get themself + subpath, _ := utils.GetSubpathFromConfig(c.App.Config()) + if c.App.Path() == path.Join(subpath, "/api/v4/users/me") { + return + } - if !user.MfaActive { - c.Err = model.NewAppError("", "api.context.mfa_required.app_error", nil, "MfaRequired", http.StatusForbidden) - return - } + // Bots are exempt + if user.IsBot { + return + } + + if !user.MfaActive { + c.Err = model.NewAppError("MfaRequired", "api.context.mfa_required.app_error", nil, "", http.StatusForbidden) + return } } diff --git a/web/context_test.go b/web/context_test.go index f6278af242..c196f34ae2 100644 --- a/web/context_test.go +++ b/web/context_test.go @@ -7,6 +7,10 @@ import ( "net/http" "testing" + "github.com/mattermost/mattermost-server/v5/model" + "github.com/mattermost/mattermost-server/v5/plugin/plugintest/mock" + "github.com/mattermost/mattermost-server/v5/store/storetest/mocks" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -27,3 +31,38 @@ func TestRequireHookId(t *testing.T) { require.Equal(t, http.StatusBadRequest, c.Err.StatusCode, "Should have set status as 400") }) } + +func TestMfaRequired(t *testing.T) { + th := SetupWithStoreMock(t) + defer th.TearDown() + + mockStore := th.App.Srv().Store.(*mocks.Store) + mockUserStore := mocks.UserStore{} + mockUserStore.On("Count", mock.Anything).Return(int64(10), nil) + mockUserStore.On("Get", "userid").Return(nil, model.NewAppError("Userstore.Get", "storeerror", nil, "store error", http.StatusInternalServerError)) + mockPostStore := mocks.PostStore{} + mockPostStore.On("GetMaxPostSize").Return(65535, nil) + mockSystemStore := mocks.SystemStore{} + mockSystemStore.On("GetByName", "InstallationDate").Return(&model.System{Name: "InstallationDate", Value: "10"}, nil) + + mockStore.On("User").Return(&mockUserStore) + mockStore.On("Post").Return(&mockPostStore) + mockStore.On("System").Return(&mockSystemStore) + + th.App.Srv().SetLicense(model.NewTestLicense("mfa")) + + th.App.SetSession(&model.Session{Id: "abc", UserId: "userid"}) + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.EnableMultifactorAuthentication = true + *cfg.ServiceSettings.EnforceMultifactorAuthentication = true + }) + + c := &Context{ + App: th.App, + } + + c.MfaRequired() + + assert.Equal(t, c.Err.Id, "api.context.get_user.app_error") +}