[MM-28125] einterface/oauthprovide: return parsing errors (#15426)

* einterface/oauthprovide: return parsing errors

* Update app/oauth.go

* reflect review comments

* fix var name error

Co-authored-by: Mattermod <mattermod@users.noreply.github.com>

app/oauth.go

@@ -574,19 +574,18 @@ func (a *App) LoginByOAuth(service string, userData io.Reader, teamId string) (*
 		return nil, model.NewAppError("LoginByOAuth", "api.user.login_by_oauth.parse.app_error",
 			map[string]interface{}{"Service": service}, "", http.StatusBadRequest)
 	}
-	authUser := provider.GetUserFromJson(bytes.NewReader(buf.Bytes()))
 
-	authData := ""
-	if authUser.AuthData != nil {
-		authData = *authUser.AuthData
+	authUser, err1 := provider.GetUserFromJson(bytes.NewReader(buf.Bytes()))
+	if err1 != nil {
+		return nil, model.NewAppError("LoginByOAuth", "api.user.login_by_oauth.parse.app_error",
+			map[string]interface{}{"Service": service}, err1.Error(), http.StatusBadRequest)
 	}
 
-	if len(authData) == 0 {
+	if *authUser.AuthData == "" {
 		return nil, model.NewAppError("LoginByOAuth", "api.user.login_by_oauth.parse.app_error",
 			map[string]interface{}{"Service": service}, "", http.StatusBadRequest)
 	}
-
-	user, err := a.GetUserByAuth(&authData, service)
+	user, err := a.GetUserByAuth(model.NewString(*authUser.AuthData), service)
 	if err != nil {
 		if err.Id == store.MISSING_AUTH_ACCOUNT_ERROR {
 			user, err = a.CreateOAuthUser(service, bytes.NewReader(buf.Bytes()), teamId)
@@ -622,23 +621,22 @@ func (a *App) CompleteSwitchWithOAuth(service string, userData io.Reader, email
 		return nil, model.NewAppError("CompleteSwitchWithOAuth", "api.user.complete_switch_with_oauth.unavailable.app_error",
 			map[string]interface{}{"Service": strings.Title(service)}, "", http.StatusNotImplemented)
 	}
-	ssoUser := provider.GetUserFromJson(userData)
-	ssoEmail := ssoUser.Email
 
-	authData := ""
-	if ssoUser.AuthData != nil {
-		authData = *ssoUser.AuthData
+	if email == "" {
+		return nil, model.NewAppError("CompleteSwitchWithOAuth", "api.user.complete_switch_with_oauth.blank_email.app_error", nil, "", http.StatusBadRequest)
 	}
 
-	if len(authData) == 0 {
+	ssoUser, err1 := provider.GetUserFromJson(userData)
+	if err1 != nil {
+		return nil, model.NewAppError("CompleteSwitchWithOAuth", "api.user.complete_switch_with_oauth.parse.app_error",
+			map[string]interface{}{"Service": service}, err1.Error(), http.StatusBadRequest)
+	}
+
+	if *ssoUser.AuthData == "" {
 		return nil, model.NewAppError("CompleteSwitchWithOAuth", "api.user.complete_switch_with_oauth.parse.app_error",
 			map[string]interface{}{"Service": service}, "", http.StatusBadRequest)
 	}
 
-	if len(email) == 0 {
-		return nil, model.NewAppError("CompleteSwitchWithOAuth", "api.user.complete_switch_with_oauth.blank_email.app_error", nil, "", http.StatusBadRequest)
-	}
-
 	user, err := a.Srv().Store.User().GetByEmail(email)
 	if err != nil {
 		return nil, err
@@ -648,7 +646,7 @@ func (a *App) CompleteSwitchWithOAuth(service string, userData io.Reader, email
 		return nil, err
 	}
 
-	if _, err = a.Srv().Store.User().UpdateAuthData(user.Id, service, &authData, ssoEmail, true); err != nil {
+	if _, err = a.Srv().Store.User().UpdateAuthData(user.Id, service, model.NewString(*ssoUser.AuthData), ssoUser.Email, true); err != nil {
 		return nil, err
 	}
 

app/user.go

@@ -328,10 +328,9 @@ func (a *App) CreateOAuthUser(service string, userData io.Reader, teamId string)
 	if provider == nil {
 		return nil, model.NewAppError("CreateOAuthUser", "api.user.create_oauth_user.not_available.app_error", map[string]interface{}{"Service": strings.Title(service)}, "", http.StatusNotImplemented)
 	}
-	user := provider.GetUserFromJson(userData)
-
-	if user == nil {
-		return nil, model.NewAppError("CreateOAuthUser", "api.user.create_oauth_user.create.app_error", map[string]interface{}{"Service": service}, "", http.StatusInternalServerError)
+	user, err1 := provider.GetUserFromJson(userData)
+	if err1 != nil {
+		return nil, model.NewAppError("CreateOAuthUser", "api.user.create_oauth_user.create.app_error", map[string]interface{}{"Service": service}, err1.Error(), http.StatusInternalServerError)
 	}
 
 	suchan := make(chan store.StoreResult, 1)
@@ -1857,9 +1856,9 @@ func (a *App) AutocompleteUsersInTeam(teamId string, term string, options *model
 }
 
 func (a *App) UpdateOAuthUserAttrs(userData io.Reader, user *model.User, provider einterfaces.OauthProvider, service string) *model.AppError {
-	oauthUser := provider.GetUserFromJson(userData)
-	if oauthUser == nil {
-		return model.NewAppError("UpdateOAuthUserAttrs", "api.user.update_oauth_user_attrs.get_user.app_error", map[string]interface{}{"Service": service}, "", http.StatusBadRequest)
+	oauthUser, err1 := provider.GetUserFromJson(userData)
+	if err1 != nil {
+		return model.NewAppError("UpdateOAuthUserAttrs", "api.user.update_oauth_user_attrs.get_user.app_error", map[string]interface{}{"Service": service}, err1.Error(), http.StatusBadRequest)
 	}
 
 	userAttrsChanged := false

einterfaces/oauthproviders.go

@@ -10,7 +10,7 @@ import (
 )
 
 type OauthProvider interface {
-	GetUserFromJson(data io.Reader) *model.User
+	GetUserFromJson(data io.Reader) (*model.User, error)
 }
 
 var oauthProviders = make(map[string]OauthProvider)

model/gitlab/gitlab.go

@@ -5,6 +5,7 @@ package oauthgitlab
 
 import (
 	"encoding/json"
+	"errors"
 	"io"
 	"strconv"
 	"strings"
@@ -55,15 +56,14 @@ func userFromGitLabUser(glu *GitLabUser) *model.User {
 	return user
 }
 
-func gitLabUserFromJson(data io.Reader) *GitLabUser {
+func gitLabUserFromJson(data io.Reader) (*GitLabUser, error) {
 	decoder := json.NewDecoder(data)
 	var glu GitLabUser
 	err := decoder.Decode(&glu)
-	if err == nil {
-		return &glu
-	} else {
-		return nil
+	if err != nil {
+		return nil, err
 	}
+	return &glu, nil
 }
 
 func (glu *GitLabUser) ToJson() string {
@@ -75,27 +75,30 @@ func (glu *GitLabUser) ToJson() string {
 	}
 }
 
-func (glu *GitLabUser) IsValid() bool {
+func (glu *GitLabUser) IsValid() error {
 	if glu.Id == 0 {
-		return false
+		return errors.New("user id can't be 0")
 	}
 
-	if len(glu.Email) == 0 {
-		return false
+	if glu.Email == "" {
+		return errors.New("user e-mail should not be empty")
 	}
 
-	return true
+	return nil
 }
 
 func (glu *GitLabUser) getAuthData() string {
 	return strconv.FormatInt(glu.Id, 10)
 }
 
-func (m *GitLabProvider) GetUserFromJson(data io.Reader) *model.User {
-	glu := gitLabUserFromJson(data)
-	if glu.IsValid() {
-		return userFromGitLabUser(glu)
+func (m *GitLabProvider) GetUserFromJson(data io.Reader) (*model.User, error) {
+	glu, err := gitLabUserFromJson(data)
+	if err != nil {
+		return nil, err
+	}
+	if err = glu.IsValid(); err != nil {
+		return nil, err
 	}
 
-	return &model.User{}
+	return userFromGitLabUser(glu), nil
 }

web/oauth_test.go

@@ -549,10 +549,10 @@ func closeBody(r *http.Response) {
 type MattermostTestProvider struct {
 }
 
-func (m *MattermostTestProvider) GetUserFromJson(data io.Reader) *model.User {
+func (m *MattermostTestProvider) GetUserFromJson(data io.Reader) (*model.User, error) {
 	user := model.UserFromJson(data)
 	user.AuthData = &user.Email
-	return user
+	return user, nil
 }
 
 func GenerateTestAppName() string {