From c05ee81c9f777d07d38a28f742db1fa0ca22c092 Mon Sep 17 00:00:00 2001 From: Ibrahim Serdar Acikgoz Date: Mon, 19 Oct 2020 18:07:20 +0300 Subject: [PATCH] [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 --- app/oauth.go | 34 ++++++++++++++++------------------ app/user.go | 13 ++++++------- einterfaces/oauthproviders.go | 2 +- model/gitlab/gitlab.go | 33 ++++++++++++++++++--------------- web/oauth_test.go | 4 ++-- 5 files changed, 43 insertions(+), 43 deletions(-) diff --git a/app/oauth.go b/app/oauth.go index 1514ffe9ac..637a9371a5 100644 --- a/app/oauth.go +++ b/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 } diff --git a/app/user.go b/app/user.go index 3f3460273e..3532b31cec 100644 --- a/app/user.go +++ b/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 diff --git a/einterfaces/oauthproviders.go b/einterfaces/oauthproviders.go index b137d5e710..d33b7d2d37 100644 --- a/einterfaces/oauthproviders.go +++ b/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) diff --git a/model/gitlab/gitlab.go b/model/gitlab/gitlab.go index 9906a2ae4e..f100535497 100644 --- a/model/gitlab/gitlab.go +++ b/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 } diff --git a/web/oauth_test.go b/web/oauth_test.go index fef5cea1d3..2dcafe465d 100644 --- a/web/oauth_test.go +++ b/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 {