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 <wiggin77@warpmail.net>

Co-authored-by: Doug Lauder <wiggin77@warpmail.net>
Co-authored-by: Mattermod <mattermod@users.noreply.github.com>

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."

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
 	}
 }
 

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")
+}