From 02cf38f74f10522b6c3c88ecb98a34c6a0bce154 Mon Sep 17 00:00:00 2001 From: Kang Ming Date: Wed, 15 Nov 2023 19:21:40 -0500 Subject: [PATCH] feat: allow unverified email signins (#1301) ## What kind of change does this PR introduce? * If `GOTRUE_ALLOW_UNVERIFIED_EMAIL_SIGN_INS` is enabled, it will allow a user with an unverified email address to sign in and obtain an access token JWT * This is particularly useful for OAuth in cases where the oauth provider doesn't return an email address / the oauth user didn't verify their email address with the OAuth provider. * Tests that broke and needed fixing were due to these reasons: * `RemoveUnconfirmedIdentities` was previously buggy and shouldn't be retaining the user metadata of a previously unconfirmed identity * `GOTRUE_ALLOW_UNVERIFIED_EMAIL_SIGN_INS` is enabled by default which caused some tests to return an access token instead of an error for a user with an unverified email ## Modifications made to automatic linking algorithm * If the candidate identity doesn't have a verified email, the decision should be to create a new account. * If the email belongs to a user already, then we opt to create a new user with no email. Previously, we would attempt to create a new user and the db will return an error due to the partial unique constraint on email violation. In order to add an email to the new user, they would have to call update user (`PUT /user`) to add a new email. --- internal/api/external.go | 155 +++++++++++------------- internal/api/external_azure_test.go | 8 +- internal/api/external_github_test.go | 3 +- internal/api/external_kakao_test.go | 3 +- internal/api/external_keycloak_test.go | 12 +- internal/api/external_twitch_test.go | 4 +- internal/api/external_workos_test.go | 8 +- internal/api/token_oidc.go | 2 +- internal/api/verify.go | 2 + internal/conf/configuration.go | 22 ++-- internal/models/linking.go | 142 +++++++++++++--------- internal/models/linking_test.go | 160 +++++++++++++++++++++---- internal/models/user.go | 30 +++-- 13 files changed, 349 insertions(+), 202 deletions(-) diff --git a/internal/api/external.go b/internal/api/external.go index 8b1a89ac6c..2afbd89a74 100644 --- a/internal/api/external.go +++ b/internal/api/external.go @@ -141,8 +141,8 @@ func (a *API) ExternalProviderCallback(w http.ResponseWriter, r *http.Request) e return nil } -// errReturnNil is a hack that signals internalExternalProviderCallback to return nil -var errReturnNil = errors.New("createAccountFromExternalIdentity: return nil in internalExternalProviderCallback") +var errEmailVerificationRequired = errors.New("email verification required") +var errEmailConfirmationSent = errors.New("email confirmation sent") func (a *API) internalExternalProviderCallback(w http.ResponseWriter, r *http.Request) error { ctx := r.Context() @@ -187,6 +187,7 @@ func (a *API) internalExternalProviderCallback(w http.ResponseWriter, r *http.Re var user *models.User var token *AccessTokenResponse + var errEmailVerificationRequiredOrConfirmationSent error err = db.Transaction(func(tx *storage.Connection) error { var terr error inviteToken := getInviteToken(ctx) @@ -196,10 +197,12 @@ func (a *API) internalExternalProviderCallback(w http.ResponseWriter, r *http.Re } } else { if user, terr = a.createAccountFromExternalIdentity(tx, r, userData, providerType); terr != nil { - if errors.Is(terr, errReturnNil) { + if errors.Is(terr, errEmailVerificationRequired) || errors.Is(terr, errEmailConfirmationSent) { + // we don't want the transaction to be rolled back because the user is created + // but not returned as further action is necessary + errEmailVerificationRequiredOrConfirmationSent = terr return nil } - return terr } } @@ -218,11 +221,30 @@ func (a *API) internalExternalProviderCallback(w http.ResponseWriter, r *http.Re } return nil }) + + rurl := a.getExternalRedirectURL(r) if err != nil { return err } - rurl := a.getExternalRedirectURL(r) + if errEmailVerificationRequiredOrConfirmationSent != nil { + flowType := models.ImplicitFlow + if flowState != nil { + flowType = models.PKCEFlow + } + msg := fmt.Sprintf("Unverified email with %v. ", providerType) + if errors.Is(errEmailVerificationRequiredOrConfirmationSent, errEmailVerificationRequired) { + msg += fmt.Sprintf("Verify the email with %v in order to sign in", providerType) + } else if errors.Is(errEmailVerificationRequiredOrConfirmationSent, errEmailConfirmationSent) { + msg += fmt.Sprintf("A confirmation email has been sent to your %v email", providerType) + } + rurl, err = a.prepErrorRedirectURL(unauthorizedError(msg), w, r, rurl, flowType) + if err != nil { + return err + } + http.Redirect(w, r, rurl, http.StatusFound) + } + if flowState != nil { // This means that the callback is using PKCE // Set the flowState.AuthCode to the query param here @@ -244,12 +266,6 @@ func (a *API) internalExternalProviderCallback(w http.ResponseWriter, r *http.Re if err := a.setCookieTokens(config, token, false, w); err != nil { return internalServerError("Failed to set JWT cookie. %s", err) } - } else { - // Left as hash fragment to comply with spec. Additionally, may override existing error query param if set to PKCE. - rurl, err = a.prepErrorRedirectURL(unauthorizedError("Unverified email with %v. A confirmation email has been sent to your %v email.", providerType, providerType), w, r, rurl, models.ImplicitFlow) - if err != nil { - return err - } } http.Redirect(w, r, rurl, http.StatusFound) @@ -261,26 +277,14 @@ func (a *API) createAccountFromExternalIdentity(tx *storage.Connection, r *http. aud := a.requestAud(ctx, r) config := a.config - var terr error - var user *models.User var identity *models.Identity - - var emailData provider.Email var identityData map[string]interface{} if userData.Metadata != nil { identityData = structs.Map(userData.Metadata) } - var emails []string - - for _, email := range userData.Emails { - if email.Verified || config.Mailer.Autoconfirm { - emails = append(emails, strings.ToLower(email.Email)) - } - } - - decision, terr := models.DetermineAccountLinking(tx, providerType, userData.Metadata.Subject, emails) + decision, terr := models.DetermineAccountLinking(tx, config, userData.Emails, aud, providerType, userData.Metadata.Subject) if terr != nil { return nil, terr } @@ -289,14 +293,6 @@ func (a *API) createAccountFromExternalIdentity(tx *storage.Connection, r *http. case models.LinkAccount: user = decision.User - emailData = userData.Emails[0] - for _, e := range userData.Emails { - if e.Primary || e.Verified { - emailData = e - break - } - } - if identity, terr = a.createNewIdentity(tx, user, providerType, identityData); terr != nil { return nil, terr } @@ -310,26 +306,19 @@ func (a *API) createAccountFromExternalIdentity(tx *storage.Connection, r *http. return nil, forbiddenError("Signups not allowed for this instance") } - // prefer primary email for new signups - emailData = userData.Emails[0] - for _, e := range userData.Emails { - if e.Primary { - emailData = e - break - } - } - params := &SignupParams{ Provider: providerType, - Email: emailData.Email, + Email: decision.CandidateEmail.Email, Aud: aud, Data: identityData, } - isSSOUser := strings.HasPrefix(providerType, "sso:") + isSSOUser := false + if decision.LinkingDomain == "sso" { + isSSOUser = true + } - user, terr = a.signupNewUser(ctx, tx, params, isSSOUser) - if terr != nil { + if user, terr = a.signupNewUser(ctx, tx, params, isSSOUser); terr != nil { return nil, terr } @@ -345,11 +334,6 @@ func (a *API) createAccountFromExternalIdentity(tx *storage.Connection, r *http. if terr = tx.UpdateOnly(identity, "identity_data", "last_sign_in_at"); terr != nil { return nil, terr } - // email & verified status might have changed if identity's email changed - emailData = provider.Email{ - Email: userData.Metadata.Email, - Verified: userData.Metadata.EmailVerified, - } if terr = user.UpdateUserMetaData(tx, identityData); terr != nil { return nil, terr } @@ -368,41 +352,48 @@ func (a *API) createAccountFromExternalIdentity(tx *storage.Connection, r *http. return nil, unauthorizedError("User is unauthorized") } - // An account with a previously unconfirmed email + password - // combination, phone or oauth identity may exist. These identities - // need to be removed when a new oauth identity is being added - // to prevent pre-account takeover attacks from happening. - if terr = user.RemoveUnconfirmedIdentities(tx, identity); terr != nil { - return nil, internalServerError("Error updating user").WithInternalError(terr) - } - if !user.IsConfirmed() { - if !emailData.Verified && !config.Mailer.Autoconfirm { - mailer := a.Mailer(ctx) - referrer := utilities.GetReferrer(r, config) - externalURL := getExternalHost(ctx) - if terr = sendConfirmation(tx, user, mailer, config.SMTP.MaxFrequency, referrer, externalURL, config.Mailer.OtpLength, models.ImplicitFlow); terr != nil { - if errors.Is(terr, MaxFrequencyLimitError) { - return nil, tooManyRequestsError("For security purposes, you can only request this once every minute") - } - return nil, internalServerError("Error sending confirmation mail").WithInternalError(terr) - } - // email must be verified to issue a token - return nil, errReturnNil - } - - if terr := models.NewAuditLogEntry(r, tx, user, models.UserSignedUpAction, "", map[string]interface{}{ - "provider": providerType, - }); terr != nil { - return nil, terr - } - if terr = triggerEventHooks(ctx, tx, SignupEvent, user, config); terr != nil { - return nil, terr + // The user may have other unconfirmed email + password + // combination, phone or oauth identities. These identities + // need to be removed when a new oauth identity is being added + // to prevent pre-account takeover attacks from happening. + if terr = user.RemoveUnconfirmedIdentities(tx, identity); terr != nil { + return nil, internalServerError("Error updating user").WithInternalError(terr) } + if decision.CandidateEmail.Verified || config.Mailer.Autoconfirm { + if terr := models.NewAuditLogEntry(r, tx, user, models.UserSignedUpAction, "", map[string]interface{}{ + "provider": providerType, + }); terr != nil { + return nil, terr + } + if terr = triggerEventHooks(ctx, tx, SignupEvent, user, config); terr != nil { + return nil, terr + } - // fall through to auto-confirm and issue token - if terr = user.Confirm(tx); terr != nil { - return nil, internalServerError("Error updating user").WithInternalError(terr) + // fall through to auto-confirm and issue token + if terr = user.Confirm(tx); terr != nil { + return nil, internalServerError("Error updating user").WithInternalError(terr) + } + } else { + emailConfirmationSent := false + if decision.CandidateEmail.Email != "" { + mailer := a.Mailer(ctx) + referrer := utilities.GetReferrer(r, config) + externalURL := getExternalHost(ctx) + if terr = sendConfirmation(tx, user, mailer, config.SMTP.MaxFrequency, referrer, externalURL, config.Mailer.OtpLength, models.ImplicitFlow); terr != nil { + if errors.Is(terr, MaxFrequencyLimitError) { + return nil, tooManyRequestsError("For security purposes, you can only request this once every minute") + } + return nil, internalServerError("Error sending confirmation mail").WithInternalError(terr) + } + emailConfirmationSent = true + } + if !config.Mailer.AllowUnverifiedEmailSignIns { + if emailConfirmationSent { + return nil, errEmailConfirmationSent + } + return nil, errEmailVerificationRequired + } } } else { if terr := models.NewAuditLogEntry(r, tx, user, models.LoginAction, "", map[string]interface{}{ diff --git a/internal/api/external_azure_test.go b/internal/api/external_azure_test.go index 0eedc08877..2983f59acd 100644 --- a/internal/api/external_azure_test.go +++ b/internal/api/external_azure_test.go @@ -196,7 +196,7 @@ func (ts *ExternalTestSuite) TestSignupExternalAzureDisableSignupSuccessWithPrim ts.Config.DisableSignup = true - ts.createUser("azuretestid", "azure@example.com", "Azure Test", "http://example.com/avatar", "") + ts.createUser("azuretestid", "azure@example.com", "Azure Test", "", "") tokenCount := 0 code := "authcode" @@ -205,14 +205,14 @@ func (ts *ExternalTestSuite) TestSignupExternalAzureDisableSignupSuccessWithPrim u := performAuthorization(ts, "azure", code, "") - assertAuthorizationSuccess(ts, u, tokenCount, -1, "azure@example.com", "Azure Test", "azuretestid", "http://example.com/avatar") + assertAuthorizationSuccess(ts, u, tokenCount, -1, "azure@example.com", "Azure Test", "azuretestid", "") } func (ts *ExternalTestSuite) TestInviteTokenExternalAzureSuccessWhenMatchingToken() { setupAzureOverrideVerifiers() // name should be populated from Azure API - ts.createUser("azuretestid", "azure@example.com", "", "http://example.com/avatar", "invite_token") + ts.createUser("azuretestid", "azure@example.com", "", "", "invite_token") tokenCount := 0 code := "authcode" @@ -221,7 +221,7 @@ func (ts *ExternalTestSuite) TestInviteTokenExternalAzureSuccessWhenMatchingToke u := performAuthorization(ts, "azure", code, "invite_token") - assertAuthorizationSuccess(ts, u, tokenCount, -1, "azure@example.com", "Azure Test", "azuretestid", "http://example.com/avatar") + assertAuthorizationSuccess(ts, u, tokenCount, -1, "azure@example.com", "Azure Test", "azuretestid", "") } func (ts *ExternalTestSuite) TestInviteTokenExternalAzureErrorWhenNoMatchingToken() { diff --git a/internal/api/external_github_test.go b/internal/api/external_github_test.go index d4f8a01474..55f579b0a8 100644 --- a/internal/api/external_github_test.go +++ b/internal/api/external_github_test.go @@ -267,6 +267,7 @@ func (ts *ExternalTestSuite) TestInviteTokenExternalGitHubErrorWhenEmailDoesntMa } func (ts *ExternalTestSuite) TestSignupExternalGitHubErrorWhenVerifiedFalse() { + ts.Config.Mailer.AllowUnverifiedEmailSignIns = false tokenCount, userCount := 0, 0 code := "authcode" emails := `[{"email":"github@example.com", "primary": true, "verified": false}]` @@ -279,7 +280,7 @@ func (ts *ExternalTestSuite) TestSignupExternalGitHubErrorWhenVerifiedFalse() { ts.Require().NoError(err) ts.Equal("unauthorized_client", v.Get("error")) ts.Equal("401", v.Get("error_code")) - ts.Equal("Unverified email with github. A confirmation email has been sent to your github email.", v.Get("error_description")) + ts.Equal("Unverified email with github. A confirmation email has been sent to your github email", v.Get("error_description")) assertAuthorizationFailure(ts, u, "", "", "") } diff --git a/internal/api/external_kakao_test.go b/internal/api/external_kakao_test.go index 6d4d3b217a..0552181cca 100644 --- a/internal/api/external_kakao_test.go +++ b/internal/api/external_kakao_test.go @@ -197,6 +197,7 @@ func (ts *ExternalTestSuite) TestInviteTokenExternalKakaoErrorWhenEmailDoesntMat } func (ts *ExternalTestSuite) TestSignupExternalKakaoErrorWhenVerifiedFalse() { + ts.Config.Mailer.AllowUnverifiedEmailSignIns = false tokenCount, userCount := 0, 0 code := "authcode" emails := `[{"email":"kakao@example.com", "primary": true, "verified": false}]` @@ -209,7 +210,7 @@ func (ts *ExternalTestSuite) TestSignupExternalKakaoErrorWhenVerifiedFalse() { ts.Require().NoError(err) ts.Equal("unauthorized_client", v.Get("error")) ts.Equal("401", v.Get("error_code")) - ts.Equal("Unverified email with kakao. A confirmation email has been sent to your kakao email.", v.Get("error_description")) + ts.Equal("Unverified email with kakao. A confirmation email has been sent to your kakao email", v.Get("error_description")) assertAuthorizationFailure(ts, u, "", "", "") } diff --git a/internal/api/external_keycloak_test.go b/internal/api/external_keycloak_test.go index 5cffc7df37..2562664a45 100644 --- a/internal/api/external_keycloak_test.go +++ b/internal/api/external_keycloak_test.go @@ -78,7 +78,7 @@ func (ts *ExternalTestSuite) TestSignupExternalKeycloakWithoutURLSetup() { func (ts *ExternalTestSuite) TestSignupExternalKeycloak_AuthorizationCode() { ts.Config.DisableSignup = false - ts.createUser("keycloaktestid", "keycloak@example.com", "Keycloak Test", "http://example.com/avatar", "") + ts.createUser("keycloaktestid", "keycloak@example.com", "Keycloak Test", "", "") tokenCount, userCount := 0, 0 code := "authcode" server := KeycloakTestSignupSetup(ts, &tokenCount, &userCount, code, keycloakUser) @@ -86,7 +86,7 @@ func (ts *ExternalTestSuite) TestSignupExternalKeycloak_AuthorizationCode() { u := performAuthorization(ts, "keycloak", code, "") - assertAuthorizationSuccess(ts, u, tokenCount, userCount, "keycloak@example.com", "Keycloak Test", "keycloaktestid", "http://example.com/avatar") + assertAuthorizationSuccess(ts, u, tokenCount, userCount, "keycloak@example.com", "Keycloak Test", "keycloaktestid", "") } func (ts *ExternalTestSuite) TestSignupExternalKeycloakDisableSignupErrorWhenNoUser() { @@ -117,7 +117,7 @@ func (ts *ExternalTestSuite) TestSignupExternalKeycloakDisableSignupErrorWhenNoE func (ts *ExternalTestSuite) TestSignupExternalKeycloakDisableSignupSuccessWithPrimaryEmail() { ts.Config.DisableSignup = true - ts.createUser("keycloaktestid", "keycloak@example.com", "Keycloak Test", "http://example.com/avatar", "") + ts.createUser("keycloaktestid", "keycloak@example.com", "Keycloak Test", "", "") tokenCount, userCount := 0, 0 code := "authcode" @@ -126,12 +126,12 @@ func (ts *ExternalTestSuite) TestSignupExternalKeycloakDisableSignupSuccessWithP u := performAuthorization(ts, "keycloak", code, "") - assertAuthorizationSuccess(ts, u, tokenCount, userCount, "keycloak@example.com", "Keycloak Test", "keycloaktestid", "http://example.com/avatar") + assertAuthorizationSuccess(ts, u, tokenCount, userCount, "keycloak@example.com", "Keycloak Test", "keycloaktestid", "") } func (ts *ExternalTestSuite) TestInviteTokenExternalKeycloakSuccessWhenMatchingToken() { // name and avatar should be populated from Keycloak API - ts.createUser("keycloaktestid", "keycloak@example.com", "", "http://example.com/avatar", "invite_token") + ts.createUser("keycloaktestid", "keycloak@example.com", "", "", "invite_token") tokenCount, userCount := 0, 0 code := "authcode" @@ -140,7 +140,7 @@ func (ts *ExternalTestSuite) TestInviteTokenExternalKeycloakSuccessWhenMatchingT u := performAuthorization(ts, "keycloak", code, "invite_token") - assertAuthorizationSuccess(ts, u, tokenCount, userCount, "keycloak@example.com", "Keycloak Test", "keycloaktestid", "http://example.com/avatar") + assertAuthorizationSuccess(ts, u, tokenCount, userCount, "keycloak@example.com", "Keycloak Test", "keycloaktestid", "") } func (ts *ExternalTestSuite) TestInviteTokenExternalKeycloakErrorWhenNoMatchingToken() { diff --git a/internal/api/external_twitch_test.go b/internal/api/external_twitch_test.go index 8c0b16ecfd..775c44ef83 100644 --- a/internal/api/external_twitch_test.go +++ b/internal/api/external_twitch_test.go @@ -107,7 +107,7 @@ func (ts *ExternalTestSuite) TestSignupExternalTwitchDisableSignupErrorWhenEmpty func (ts *ExternalTestSuite) TestSignupExternalTwitchDisableSignupSuccessWithPrimaryEmail() { ts.Config.DisableSignup = true - ts.createUser("twitchTestId", "twitch@example.com", "Twitch Test", "https://s.gravatar.com/avatar/23463b99b62a72f26ed677cc556c44e8", "") + ts.createUser("twitchTestId", "twitch@example.com", "Twitch user", "https://s.gravatar.com/avatar/23463b99b62a72f26ed677cc556c44e8", "") tokenCount, userCount := 0, 0 code := "authcode" @@ -116,7 +116,7 @@ func (ts *ExternalTestSuite) TestSignupExternalTwitchDisableSignupSuccessWithPri u := performAuthorization(ts, "twitch", code, "") - assertAuthorizationSuccess(ts, u, tokenCount, userCount, "twitch@example.com", "Twitch Test", "twitchTestId", "https://s.gravatar.com/avatar/23463b99b62a72f26ed677cc556c44e8") + assertAuthorizationSuccess(ts, u, tokenCount, userCount, "twitch@example.com", "Twitch user", "twitchTestId", "https://s.gravatar.com/avatar/23463b99b62a72f26ed677cc556c44e8") } func (ts *ExternalTestSuite) TestInviteTokenExternalTwitchSuccessWhenMatchingToken() { diff --git a/internal/api/external_workos_test.go b/internal/api/external_workos_test.go index 4ade89883d..05d3175cb7 100644 --- a/internal/api/external_workos_test.go +++ b/internal/api/external_workos_test.go @@ -160,7 +160,7 @@ func (ts *ExternalTestSuite) TestSignupExternalWorkosDisableSignupErrorWhenEmpty func (ts *ExternalTestSuite) TestSignupExternalWorkosDisableSignupSuccessWithPrimaryEmail() { ts.Config.DisableSignup = true - ts.createUser("test_prof_workos", "workos@example.com", "John Doe", "http://example.com/avatar", "") + ts.createUser("test_prof_workos", "workos@example.com", "John Doe", "", "") tokenCount, userCount := 0, 0 code := "authcode" @@ -169,11 +169,11 @@ func (ts *ExternalTestSuite) TestSignupExternalWorkosDisableSignupSuccessWithPri u := performAuthorization(ts, "workos", code, "") - assertAuthorizationSuccess(ts, u, tokenCount, userCount, "workos@example.com", "John Doe", "test_prof_workos", "http://example.com/avatar") + assertAuthorizationSuccess(ts, u, tokenCount, userCount, "workos@example.com", "John Doe", "test_prof_workos", "") } func (ts *ExternalTestSuite) TestInviteTokenExternalWorkosSuccessWhenMatchingToken() { - ts.createUser("test_prof_workos", "workos@example.com", "", "http://example.com/avatar", "invite_token") + ts.createUser("test_prof_workos", "workos@example.com", "", "", "invite_token") tokenCount, userCount := 0, 0 code := "authcode" @@ -182,7 +182,7 @@ func (ts *ExternalTestSuite) TestInviteTokenExternalWorkosSuccessWhenMatchingTok u := performAuthorization(ts, "workos", code, "invite_token") - assertAuthorizationSuccess(ts, u, tokenCount, userCount, "workos@example.com", "John Doe", "test_prof_workos", "http://example.com/avatar") + assertAuthorizationSuccess(ts, u, tokenCount, userCount, "workos@example.com", "John Doe", "test_prof_workos", "") } func (ts *ExternalTestSuite) TestInviteTokenExternalWorkosErrorWhenNoMatchingToken() { diff --git a/internal/api/token_oidc.go b/internal/api/token_oidc.go index dd00ea99bd..39692c6c32 100644 --- a/internal/api/token_oidc.go +++ b/internal/api/token_oidc.go @@ -201,7 +201,7 @@ func (a *API) IdTokenGrant(ctx context.Context, w http.ResponseWriter, r *http.R user, terr = a.createAccountFromExternalIdentity(tx, r, userData, providerType) if terr != nil { - if errors.Is(terr, errReturnNil) { + if errors.Is(terr, errEmailVerificationRequired) { return nil } diff --git a/internal/api/verify.go b/internal/api/verify.go index dc5a5e0d4b..8afe629391 100644 --- a/internal/api/verify.go +++ b/internal/api/verify.go @@ -415,8 +415,10 @@ func (a *API) prepErrorRedirectURL(err *HTTPError, w http.ResponseWriter, r *htt q.Set("error_code", strconv.Itoa(err.Code)) q.Set("error_description", err.Message) if flowType == models.PKCEFlow { + // Additionally, may override existing error query param if set to PKCE. u.RawQuery = q.Encode() } + // Left as hash fragment to comply with spec. u.Fragment = hq.Encode() return u.String(), nil } diff --git a/internal/conf/configuration.go b/internal/conf/configuration.go index 2686ba253c..cc5b7505df 100644 --- a/internal/conf/configuration.go +++ b/internal/conf/configuration.go @@ -249,13 +249,17 @@ func (c *SMTPConfiguration) Validate() error { } type MailerConfiguration struct { - Autoconfirm bool `json:"autoconfirm"` - Subjects EmailContentConfiguration `json:"subjects"` - Templates EmailContentConfiguration `json:"templates"` - URLPaths EmailContentConfiguration `json:"url_paths"` - SecureEmailChangeEnabled bool `json:"secure_email_change_enabled" split_words:"true" default:"true"` - OtpExp uint `json:"otp_exp" split_words:"true"` - OtpLength int `json:"otp_length" split_words:"true"` + Autoconfirm bool `json:"autoconfirm"` + AllowUnverifiedEmailSignIns bool `json:"allow_unverified_email_sign_ins" split_words:"true" default:"true"` + + Subjects EmailContentConfiguration `json:"subjects"` + Templates EmailContentConfiguration `json:"templates"` + URLPaths EmailContentConfiguration `json:"url_paths"` + + SecureEmailChangeEnabled bool `json:"secure_email_change_enabled" split_words:"true" default:"true"` + + OtpExp uint `json:"otp_exp" split_words:"true"` + OtpLength int `json:"otp_length" split_words:"true"` } type PhoneProviderConfiguration struct { @@ -438,6 +442,10 @@ func (config *GlobalConfiguration) ApplyDefaults() error { config.JWT.Exp = 3600 } + if config.Mailer.Autoconfirm && config.Mailer.AllowUnverifiedEmailSignIns { + return errors.New("cannot enable both GOTRUE_MAILER_AUTOCONFIRM and GOTRUE_MAILER_ALLOW_UNVERIFIED_EMAIL_SIGN_INS") + } + if config.Mailer.URLPaths.Invite == "" { config.Mailer.URLPaths.Invite = "/verify" } diff --git a/internal/models/linking.go b/internal/models/linking.go index 167da5bbca..52a9249bb2 100644 --- a/internal/models/linking.go +++ b/internal/models/linking.go @@ -3,6 +3,8 @@ package models import ( "strings" + "github.com/supabase/gotrue/internal/api/provider" + "github.com/supabase/gotrue/internal/conf" "github.com/supabase/gotrue/internal/storage" ) @@ -34,12 +36,11 @@ const ( ) type AccountLinkingResult struct { - Decision AccountLinkingDecision - - User *User - Identities []*Identity - - LinkingDomain string + Decision AccountLinkingDecision + User *User + Identities []*Identity + LinkingDomain string + CandidateEmail provider.Email } // DetermineAccountLinking uses the provided data and database state to compute a decision on whether: @@ -49,8 +50,20 @@ type AccountLinkingResult struct { // - It's not possible to decide due to data inconsistency (MultipleAccounts) and the caller should decide // // Errors signal failure in processing only, like database access errors. -func DetermineAccountLinking(tx *storage.Connection, provider, sub string, emails []string) (AccountLinkingResult, error) { - if identity, terr := FindIdentityByIdAndProvider(tx, sub, provider); terr == nil { +func DetermineAccountLinking(tx *storage.Connection, config *conf.GlobalConfiguration, emails []provider.Email, aud, providerName, sub string) (AccountLinkingResult, error) { + var verifiedEmails []string + var candidateEmail provider.Email + for _, email := range emails { + if email.Verified || config.Mailer.Autoconfirm { + verifiedEmails = append(verifiedEmails, strings.ToLower(email.Email)) + } + if email.Primary { + candidateEmail = email + candidateEmail.Email = strings.ToLower(email.Email) + } + } + + if identity, terr := FindIdentityByIdAndProvider(tx, sub, providerName); terr == nil { // account exists var user *User @@ -58,59 +71,65 @@ func DetermineAccountLinking(tx *storage.Connection, provider, sub string, email return AccountLinkingResult{}, terr } + // we overwrite the email with the existing user's email since the user + // could have an empty empty + candidateEmail.Email = user.GetEmail() return AccountLinkingResult{ - Decision: AccountExists, - User: user, - Identities: []*Identity{identity}, - LinkingDomain: GetAccountLinkingDomain(provider), + Decision: AccountExists, + User: user, + Identities: []*Identity{identity}, + LinkingDomain: GetAccountLinkingDomain(providerName), + CandidateEmail: candidateEmail, }, nil } else if !IsNotFoundError(terr) { return AccountLinkingResult{}, terr } - // account does not exist, identity and user not immediately - // identifiable, look for similar identities based on email - var similarIdentities []*Identity - var similarUsers []*User + // the identity does not exist, so we need to check if we should create a new account + // or link to an existing one - if len(emails) > 0 { - if terr := tx.Q().Eager().Where("email ilike any (?)", emails).All(&similarIdentities); terr != nil { + // this is the linking domain for the new identity + candidateLinkingDomain := GetAccountLinkingDomain(providerName) + if len(verifiedEmails) == 0 { + // if there are no verified emails, we always decide to create a new account + user, terr := IsDuplicatedEmail(tx, candidateEmail.Email, aud, nil) + if terr != nil { return AccountLinkingResult{}, terr } - - if !strings.HasPrefix(provider, "sso:") { - // there can be multiple user accounts with the same email when is_sso_user is true - // so we just do not consider those similar user accounts - if terr := tx.Q().Eager().Where("email ilike any (?) and is_sso_user is false", emails).All(&similarUsers); terr != nil { - return AccountLinkingResult{}, terr - } + if user != nil { + candidateEmail.Email = "" } - } - - // TODO: determine linking behavior over phone too - if len(similarIdentities) == 0 && len(similarUsers) == 0 { - // there are no similar identities, clearly we have to create a new account - return AccountLinkingResult{ - Decision: CreateAccount, - LinkingDomain: GetAccountLinkingDomain(provider), + Decision: CreateAccount, + LinkingDomain: candidateLinkingDomain, + CandidateEmail: candidateEmail, }, nil } - // there are some similar identities, we now need to proceed in - // identifying whether this supposed new identity should be assigned to - // an existing user or to create a new user, according to the automatic - // linking rules + var similarIdentities []*Identity + var similarUsers []*User + // look for similar identities and users based on email + if terr := tx.Q().Eager().Where("email ilike any (?)", verifiedEmails).All(&similarIdentities); terr != nil { + return AccountLinkingResult{}, terr + } - // this is the linking domain for the new identity - newAccountLinkingDomain := GetAccountLinkingDomain(provider) + if !strings.HasPrefix(providerName, "sso:") { + // there can be multiple user accounts with the same email when is_sso_user is true + // so we just do not consider those similar user accounts + if terr := tx.Q().Eager().Where("email ilike any (?) and is_sso_user is false", verifiedEmails).All(&similarUsers); terr != nil { + return AccountLinkingResult{}, terr + } + } + // Need to check if the new identity should be assigned to an + // existing user or to create a new user, according to the automatic + // linking rules var linkingIdentities []*Identity // now let's see if there are any existing and similar identities in // the same linking domain for _, identity := range similarIdentities { - if GetAccountLinkingDomain(identity.Provider) == newAccountLinkingDomain { + if GetAccountLinkingDomain(identity.Provider) == candidateLinkingDomain { linkingIdentities = append(linkingIdentities, identity) } } @@ -121,24 +140,27 @@ func DetermineAccountLinking(tx *storage.Connection, provider, sub string, email // so we link this new identity to the user // TODO: Backfill the missing identity for the user return AccountLinkingResult{ - Decision: LinkAccount, - User: similarUsers[0], - Identities: linkingIdentities, - LinkingDomain: newAccountLinkingDomain, + Decision: LinkAccount, + User: similarUsers[0], + Identities: linkingIdentities, + LinkingDomain: candidateLinkingDomain, + CandidateEmail: candidateEmail, }, nil } else if len(similarUsers) > 1 { // this shouldn't happen since there is a partial unique index on (email and is_sso_user = false) return AccountLinkingResult{ - Decision: MultipleAccounts, - Identities: linkingIdentities, - LinkingDomain: newAccountLinkingDomain, + Decision: MultipleAccounts, + Identities: linkingIdentities, + LinkingDomain: candidateLinkingDomain, + CandidateEmail: candidateEmail, }, nil } else { // there are no identities in the linking domain, we have to // create a new identity and new user return AccountLinkingResult{ - Decision: CreateAccount, - LinkingDomain: newAccountLinkingDomain, + Decision: CreateAccount, + LinkingDomain: candidateLinkingDomain, + CandidateEmail: candidateEmail, }, nil } } @@ -146,16 +168,17 @@ func DetermineAccountLinking(tx *storage.Connection, provider, sub string, email // there is at least one identity in the linking domain let's do a // sanity check to see if all of the identities in the domain share the // same user ID - + linkingUserId := linkingIdentities[0].UserID for _, identity := range linkingIdentities { - if identity.UserID != linkingIdentities[0].UserID { + if identity.UserID != linkingUserId { // ok this linking domain has more than one user account // caller should decide what to do return AccountLinkingResult{ - Decision: MultipleAccounts, - Identities: linkingIdentities, - LinkingDomain: newAccountLinkingDomain, + Decision: MultipleAccounts, + Identities: linkingIdentities, + LinkingDomain: candidateLinkingDomain, + CandidateEmail: candidateEmail, }, nil } } @@ -166,14 +189,15 @@ func DetermineAccountLinking(tx *storage.Connection, provider, sub string, email var user *User var terr error - if user, terr = FindUserByID(tx, linkingIdentities[0].UserID); terr != nil { + if user, terr = FindUserByID(tx, linkingUserId); terr != nil { return AccountLinkingResult{}, terr } return AccountLinkingResult{ - Decision: LinkAccount, - User: user, - Identities: linkingIdentities, - LinkingDomain: newAccountLinkingDomain, + Decision: LinkAccount, + User: user, + Identities: linkingIdentities, + LinkingDomain: candidateLinkingDomain, + CandidateEmail: candidateEmail, }, nil } diff --git a/internal/models/linking_test.go b/internal/models/linking_test.go index ecb80dc80e..d2e46944c6 100644 --- a/internal/models/linking_test.go +++ b/internal/models/linking_test.go @@ -5,6 +5,7 @@ import ( "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" + "github.com/supabase/gotrue/internal/api/provider" "github.com/supabase/gotrue/internal/conf" "github.com/supabase/gotrue/internal/storage" "github.com/supabase/gotrue/internal/storage/test" @@ -13,7 +14,8 @@ import ( type AccountLinkingTestSuite struct { suite.Suite - db *storage.Connection + config *conf.GlobalConfiguration + db *storage.Connection } func (ts *AccountLinkingTestSuite) SetupTest() { @@ -28,7 +30,8 @@ func TestAccountLinking(t *testing.T) { require.NoError(t, err) ts := &AccountLinkingTestSuite{ - db: conn, + config: globalConfig, + db: conn, } defer ts.db.Close() @@ -37,13 +40,18 @@ func TestAccountLinking(t *testing.T) { func (ts *AccountLinkingTestSuite) TestCreateAccountDecisionNoAccounts() { // when there are no accounts in the system -- conventional provider - decision, err := DetermineAccountLinking(ts.db, "provider", "abcdefgh", []string{"test@example.com"}) + testEmail := provider.Email{ + Email: "test@example.com", + Verified: true, + Primary: true, + } + decision, err := DetermineAccountLinking(ts.db, ts.config, []provider.Email{testEmail}, ts.config.JWT.Aud, "provider", "abcdefgh") require.NoError(ts.T(), err) require.Equal(ts.T(), decision.Decision, CreateAccount) // when there are no accounts in the system -- SSO provider - decision, err = DetermineAccountLinking(ts.db, "sso:f06f9e3d-ff92-4c47-a179-7acf1fda6387", "abcdefgh", []string{"test@example.com"}) + decision, err = DetermineAccountLinking(ts.db, ts.config, []provider.Email{testEmail}, ts.config.JWT.Aud, "sso:f06f9e3d-ff92-4c47-a179-7acf1fda6387", "abcdefgh") require.NoError(ts.T(), err) require.Equal(ts.T(), decision.Decision, CreateAccount) @@ -72,13 +80,25 @@ func (ts *AccountLinkingTestSuite) TestCreateAccountDecisionWithAccounts() { require.NoError(ts.T(), ts.db.Create(identityB)) // when the email doesn't exist in the system -- conventional provider - decision, err := DetermineAccountLinking(ts.db, "provider", "abcdefgh", []string{"other@example.com"}) + decision, err := DetermineAccountLinking(ts.db, ts.config, []provider.Email{ + provider.Email{ + Email: "other@example.com", + Verified: true, + Primary: true, + }, + }, ts.config.JWT.Aud, "provider", "abcdefgh") require.NoError(ts.T(), err) require.Equal(ts.T(), decision.Decision, CreateAccount) // when looking for an email that doesn't exist in the SSO linking domain - decision, err = DetermineAccountLinking(ts.db, "sso:f06f9e3d-ff92-4c47-a179-7acf1fda6387", "abcdefgh", []string{"other@samltest.id"}) + decision, err = DetermineAccountLinking(ts.db, ts.config, []provider.Email{ + provider.Email{ + Email: "other@samltest.id", + Verified: true, + Primary: true, + }, + }, ts.config.JWT.Aud, "sso:f06f9e3d-ff92-4c47-a179-7acf1fda6387", "abcdefgh") require.NoError(ts.T(), err) require.Equal(ts.T(), decision.Decision, CreateAccount) @@ -95,14 +115,20 @@ func (ts *AccountLinkingTestSuite) TestAccountExists() { require.NoError(ts.T(), err) require.NoError(ts.T(), ts.db.Create(identityA)) - decision, err := DetermineAccountLinking(ts.db, "provider", userA.ID.String(), []string{"test@example.com"}) + decision, err := DetermineAccountLinking(ts.db, ts.config, []provider.Email{ + provider.Email{ + Email: "test@example.com", + Verified: true, + Primary: true, + }, + }, ts.config.JWT.Aud, "provider", userA.ID.String()) require.NoError(ts.T(), err) require.Equal(ts.T(), decision.Decision, AccountExists) require.Equal(ts.T(), decision.User.ID, userA.ID) } -func (ts *AccountLinkingTestSuite) TestLinkAccountExists() { +func (ts *AccountLinkingTestSuite) TestLinkingScenarios() { userA, err := NewUser("", "test@example.com", "", "authenticated", nil) require.NoError(ts.T(), err) require.NoError(ts.T(), ts.db.Create(userA)) @@ -126,41 +152,123 @@ func (ts *AccountLinkingTestSuite) TestLinkAccountExists() { cases := []struct { desc string - email string + email provider.Email sub string provider string - decision AccountLinkingDecision + decision AccountLinkingResult }{ { // link decision because the below described identity is in the default linking domain but uses "other-provider" instead of "provder" - desc: "same email address", - email: "test@example.com", + desc: "same email address", + email: provider.Email{ + Email: "test@example.com", + Verified: true, + Primary: true, + }, sub: userA.ID.String(), provider: "other-provider", - decision: LinkAccount, + decision: AccountLinkingResult{ + Decision: LinkAccount, + User: userA, + LinkingDomain: "default", + CandidateEmail: provider.Email{ + Email: "test@example.com", + Verified: true, + Primary: true, + }, + }, }, { - desc: "same email address in uppercase", - email: "TEST@example.com", + desc: "same email address in uppercase", + email: provider.Email{ + Email: "TEST@example.com", + Verified: true, + Primary: true, + }, sub: userA.ID.String(), provider: "other-provider", - decision: LinkAccount, + decision: AccountLinkingResult{ + Decision: LinkAccount, + User: userA, + LinkingDomain: "default", + CandidateEmail: provider.Email{ + // expected email should be case insensitive + Email: "test@example.com", + Verified: true, + Primary: true, + }, + }, }, { - desc: "no link decision because the SSO linking domain is scoped to the provider unique ID", - email: "test@samltest.id", + desc: "no link decision because the SSO linking domain is scoped to the provider unique ID", + email: provider.Email{ + Email: "test@samltest.id", + Verified: true, + Primary: true, + }, sub: userB.ID.String(), provider: "sso:f06f9e3d-ff92-4c47-a179-7acf1fda6387", - decision: AccountExists, + // decision: AccountExists, + decision: AccountLinkingResult{ + Decision: AccountExists, + User: userB, + LinkingDomain: "sso:f06f9e3d-ff92-4c47-a179-7acf1fda6387", + CandidateEmail: provider.Email{ + Email: "test@samltest.id", + Verified: true, + Primary: true, + }, + }, + }, + { + desc: "create account with empty email because email is unverified and user exists", + email: provider.Email{ + Email: "test@example.com", + Verified: false, + Primary: true, + }, + sub: userA.ID.String(), + provider: "other-provider", + decision: AccountLinkingResult{ + Decision: CreateAccount, + LinkingDomain: "default", + CandidateEmail: provider.Email{ + Email: "", + Verified: false, + Primary: true, + }, + }, + }, + { + desc: "create account because email is unverified and user doesn't exist", + email: provider.Email{ + Email: "other@example.com", + Verified: false, + Primary: true, + }, + sub: "000000000", + provider: "other-provider", + decision: AccountLinkingResult{ + Decision: CreateAccount, + LinkingDomain: "default", + CandidateEmail: provider.Email{ + Email: "other@example.com", + Verified: false, + Primary: true, + }, + }, }, } for _, c := range cases { ts.Run(c.desc, func() { - decision, err := DetermineAccountLinking(ts.db, c.provider, c.sub, []string{c.email}) + decision, err := DetermineAccountLinking(ts.db, ts.config, []provider.Email{c.email}, ts.config.JWT.Aud, c.provider, c.sub) require.NoError(ts.T(), err) - - require.Equal(ts.T(), decision.Decision, c.decision) + require.Equal(ts.T(), c.decision.Decision, decision.Decision) + require.Equal(ts.T(), c.decision.LinkingDomain, decision.LinkingDomain) + require.Equal(ts.T(), c.decision.CandidateEmail.Email, decision.CandidateEmail.Email) + require.Equal(ts.T(), c.decision.CandidateEmail.Verified, decision.CandidateEmail.Verified) + require.Equal(ts.T(), c.decision.CandidateEmail.Primary, decision.CandidateEmail.Primary) }) } @@ -190,7 +298,13 @@ func (ts *AccountLinkingTestSuite) TestMultipleAccounts() { // decision is multiple accounts because there are two distinct // identities in the same "default" linking domain with the same email // address pointing to two different user accounts - decision, err := DetermineAccountLinking(ts.db, "provider", "abcdefgh", []string{"test@example.com"}) + decision, err := DetermineAccountLinking(ts.db, ts.config, []provider.Email{ + provider.Email{ + Email: "test@example.com", + Verified: true, + Primary: true, + }, + }, ts.config.JWT.Aud, "provider", "abcdefgh") require.NoError(ts.T(), err) require.Equal(ts.T(), decision.Decision, MultipleAccounts) diff --git a/internal/models/user.go b/internal/models/user.go index ed83c288c1..b9debb1afd 100644 --- a/internal/models/user.go +++ b/internal/models/user.go @@ -577,20 +577,27 @@ func IsDuplicatedEmail(tx *storage.Connection, email, aud string, currentUser *U userIDs := make(map[string]uuid.UUID) for _, identity := range identities { - if !identity.IsForSSOProvider() { - if (currentUser != nil && currentUser.ID != identity.UserID) || (currentUser == nil) { + if _, ok := userIDs[identity.UserID.String()]; !ok { + if !identity.IsForSSOProvider() { userIDs[identity.UserID.String()] = identity.UserID } } } + var currentUserId uuid.UUID + if currentUser != nil { + currentUserId = currentUser.ID + } + for _, userID := range userIDs { - user, err := FindUserByID(tx, userID) - if err != nil { - return nil, errors.Wrap(err, "unable to find user from email identity for duplicates") - } - if user.Aud == aud { - return user, nil + if userID != currentUserId { + user, err := FindUserByID(tx, userID) + if err != nil { + return nil, errors.Wrap(err, "unable to find user from email identity for duplicates") + } + if user.Aud == aud { + return user, nil + } } } @@ -641,10 +648,6 @@ func (u *User) UpdateBannedUntil(tx *storage.Connection) error { // RemoveUnconfirmedIdentities removes potentially malicious unconfirmed identities from a user (if any) func (u *User) RemoveUnconfirmedIdentities(tx *storage.Connection, identity *Identity) error { - if u.IsConfirmed() { - return nil - } - // user is unconfirmed so the password should be reset u.EncryptedPassword = "" if terr := tx.UpdateOnly(u, "encrypted_password"); terr != nil { @@ -654,6 +657,9 @@ func (u *User) RemoveUnconfirmedIdentities(tx *storage.Connection, identity *Ide // user is unconfirmed so existing user_metadata should be overwritten // to use the current identity metadata u.UserMetaData = identity.IdentityData + if terr := u.UpdateUserMetaData(tx, u.UserMetaData); terr != nil { + return terr + } // user is unconfirmed so none of the providers associated to it are verified yet // only the current provider should be kept