From a1edc2314d2687c9320d884f8a584d8b539eec96 Mon Sep 17 00:00:00 2001 From: Antonin Delpeuch Date: Sun, 19 Nov 2023 11:50:05 +0000 Subject: [PATCH] [GITEA] oauth2: use link_account page when email/username is missing (#1757) Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/1757 Co-authored-by: Antonin Delpeuch Co-committed-by: Antonin Delpeuch (cherry picked from commit 0f6e0f90359b4b669d297a533de18b41e3293df2) (cherry picked from commit 779168a572c521507a35ba624dbd032ec28f272e) (cherry picked from commit 29a2457321e4587f55b333d0c5698925e403f026) --- routers/web/auth/oauth.go | 20 ++++++++++++-------- tests/integration/oauth_test.go | 27 +++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 8 deletions(-) diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index 9968fbe6a6..60229bc873 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -951,10 +951,16 @@ func SignInOAuthCallback(ctx *context.Context) { return } else if !setting.Service.AllowOnlyInternalRegistration && setting.OAuth2Client.EnableAutoRegistration { // create new user with details from oauth2 provider - var missingFields []string if gothUser.UserID == "" { - missingFields = append(missingFields, "sub") + log.Error("OAuth2 Provider %s returned empty or missing field: UserID", authSource.Name) + if authSource.IsOAuth2() && authSource.Cfg.(*oauth2.Source).Provider == "openidConnect" { + log.Error("You may need to change the 'OPENID_CONNECT_SCOPES' setting to request all required fields") + } + err = fmt.Errorf("OAuth2 Provider %s returned empty or missing field: UserID", authSource.Name) + ctx.ServerError("CreateUser", err) + return } + var missingFields []string if gothUser.Email == "" { missingFields = append(missingFields, "email") } @@ -962,12 +968,10 @@ func SignInOAuthCallback(ctx *context.Context) { missingFields = append(missingFields, "nickname") } if len(missingFields) > 0 { - log.Error("OAuth2 Provider %s returned empty or missing fields: %s", authSource.Name, missingFields) - if authSource.IsOAuth2() && authSource.Cfg.(*oauth2.Source).Provider == "openidConnect" { - log.Error("You may need to change the 'OPENID_CONNECT_SCOPES' setting to request all required fields") - } - err = fmt.Errorf("OAuth2 Provider %s returned empty or missing fields: %s", authSource.Name, missingFields) - ctx.ServerError("CreateUser", err) + // we don't have enough information to create an account automatically, + // so we prompt the user for the remaining bits + log.Trace("OAuth2 Provider %s returned empty or missing fields: %s, prompting the user for them", authSource.Name, missingFields) + showLinkingLogin(ctx, gothUser) return } u = &user_model.User{ diff --git a/tests/integration/oauth_test.go b/tests/integration/oauth_test.go index 4a00d73a02..fed73696e3 100644 --- a/tests/integration/oauth_test.go +++ b/tests/integration/oauth_test.go @@ -469,3 +469,30 @@ func TestSignInOAuthCallbackSignIn(t *testing.T) { userAfterLogin := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: userGitLab.ID}) assert.Greater(t, userAfterLogin.LastLoginUnix, userGitLab.LastLoginUnix) } + +func TestSignUpViaOAuthWithMissingFields(t *testing.T) { + defer tests.PrepareTestEnv(t)() + // enable auto-creation of accounts via OAuth2 + enableAutoRegistration := setting.OAuth2Client.EnableAutoRegistration + setting.OAuth2Client.EnableAutoRegistration = true + defer func() { + setting.OAuth2Client.EnableAutoRegistration = enableAutoRegistration + }() + + // OAuth2 authentication source GitLab + gitlabName := "gitlab" + addAuthSource(t, authSourcePayloadGitLabCustom(gitlabName)) + userGitLabUserID := "5678" + + // The Goth User returned by the oauth2 integration is missing + // an email address, so we won't be able to automatically create a local account for it. + defer mockCompleteUserAuth(func(res http.ResponseWriter, req *http.Request) (goth.User, error) { + return goth.User{ + Provider: gitlabName, + UserID: userGitLabUserID, + }, nil + })() + req := NewRequest(t, "GET", fmt.Sprintf("/user/oauth2/%s/callback?code=XYZ&state=XYZ", gitlabName)) + resp := MakeRequest(t, req, http.StatusSeeOther) + assert.Equal(t, test.RedirectURL(resp), "/user/link_account") +}