Skip to content

Commit

Permalink
feat: allow unverified email signins (#1301)
Browse files Browse the repository at this point in the history 
## 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.
  • Loading branch information
@kangmingtay
kangmingtay authored Nov 16, 2023
1 parent 69feebc commit 94293b7
Show file tree
Hide file tree
Showing 13 changed files with 349 additions and 202 deletions.
155 changes: 73 additions & 82 deletions internal/api/external.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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)
Expand All @@ -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
}
}
Expand All @@ -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
Expand All @@ -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)
Expand All @@ -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
}
Expand All @@ -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
}
Expand All @@ -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
}

Expand All @@ -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
}
Expand All @@ -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{}{
Expand Down
8 changes: 4 additions & 4 deletions internal/api/external_azure_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ func (ts *ExternalTestSuite) TestSignupExternalAzureDisableSignupSuccessWithPrim

ts.Config.DisableSignup = true

ts.createUser("azuretestid", "[email protected]", "Azure Test", "http://example.com/avatar", "")
ts.createUser("azuretestid", "[email protected]", "Azure Test", "", "")

tokenCount := 0
code := "authcode"
Expand All @@ -205,14 +205,14 @@ func (ts *ExternalTestSuite) TestSignupExternalAzureDisableSignupSuccessWithPrim

u := performAuthorization(ts, "azure", code, "")

assertAuthorizationSuccess(ts, u, tokenCount, -1, "[email protected]", "Azure Test", "azuretestid", "http://example.com/avatar")
assertAuthorizationSuccess(ts, u, tokenCount, -1, "[email protected]", "Azure Test", "azuretestid", "")
}

func (ts *ExternalTestSuite) TestInviteTokenExternalAzureSuccessWhenMatchingToken() {
setupAzureOverrideVerifiers()

// name should be populated from Azure API
ts.createUser("azuretestid", "[email protected]", "", "http://example.com/avatar", "invite_token")
ts.createUser("azuretestid", "[email protected]", "", "", "invite_token")

tokenCount := 0
code := "authcode"
Expand All @@ -221,7 +221,7 @@ func (ts *ExternalTestSuite) TestInviteTokenExternalAzureSuccessWhenMatchingToke

u := performAuthorization(ts, "azure", code, "invite_token")

assertAuthorizationSuccess(ts, u, tokenCount, -1, "[email protected]", "Azure Test", "azuretestid", "http://example.com/avatar")
assertAuthorizationSuccess(ts, u, tokenCount, -1, "[email protected]", "Azure Test", "azuretestid", "")
}

func (ts *ExternalTestSuite) TestInviteTokenExternalAzureErrorWhenNoMatchingToken() {
Expand Down
3 changes: 2 additions & 1 deletion internal/api/external_github_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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":"[email protected]", "primary": true, "verified": false}]`
Expand All @@ -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, "", "", "")
}

Expand Down
3 changes: 2 additions & 1 deletion internal/api/external_kakao_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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":"[email protected]", "primary": true, "verified": false}]`
Expand All @@ -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, "", "", "")
}

Expand Down
12 changes: 6 additions & 6 deletions internal/api/external_keycloak_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,15 +78,15 @@ func (ts *ExternalTestSuite) TestSignupExternalKeycloakWithoutURLSetup() {

func (ts *ExternalTestSuite) TestSignupExternalKeycloak_AuthorizationCode() {
ts.Config.DisableSignup = false
ts.createUser("keycloaktestid", "[email protected]", "Keycloak Test", "http://example.com/avatar", "")
ts.createUser("keycloaktestid", "[email protected]", "Keycloak Test", "", "")
tokenCount, userCount := 0, 0
code := "authcode"
server := KeycloakTestSignupSetup(ts, &tokenCount, &userCount, code, keycloakUser)
defer server.Close()

u := performAuthorization(ts, "keycloak", code, "")

assertAuthorizationSuccess(ts, u, tokenCount, userCount, "[email protected]", "Keycloak Test", "keycloaktestid", "http://example.com/avatar")
assertAuthorizationSuccess(ts, u, tokenCount, userCount, "[email protected]", "Keycloak Test", "keycloaktestid", "")
}

func (ts *ExternalTestSuite) TestSignupExternalKeycloakDisableSignupErrorWhenNoUser() {
Expand Down Expand Up @@ -117,7 +117,7 @@ func (ts *ExternalTestSuite) TestSignupExternalKeycloakDisableSignupErrorWhenNoE
func (ts *ExternalTestSuite) TestSignupExternalKeycloakDisableSignupSuccessWithPrimaryEmail() {
ts.Config.DisableSignup = true

ts.createUser("keycloaktestid", "[email protected]", "Keycloak Test", "http://example.com/avatar", "")
ts.createUser("keycloaktestid", "[email protected]", "Keycloak Test", "", "")

tokenCount, userCount := 0, 0
code := "authcode"
Expand All @@ -126,12 +126,12 @@ func (ts *ExternalTestSuite) TestSignupExternalKeycloakDisableSignupSuccessWithP

u := performAuthorization(ts, "keycloak", code, "")

assertAuthorizationSuccess(ts, u, tokenCount, userCount, "[email protected]", "Keycloak Test", "keycloaktestid", "http://example.com/avatar")
assertAuthorizationSuccess(ts, u, tokenCount, userCount, "[email protected]", "Keycloak Test", "keycloaktestid", "")
}

func (ts *ExternalTestSuite) TestInviteTokenExternalKeycloakSuccessWhenMatchingToken() {
// name and avatar should be populated from Keycloak API
ts.createUser("keycloaktestid", "[email protected]", "", "http://example.com/avatar", "invite_token")
ts.createUser("keycloaktestid", "[email protected]", "", "", "invite_token")

tokenCount, userCount := 0, 0
code := "authcode"
Expand All @@ -140,7 +140,7 @@ func (ts *ExternalTestSuite) TestInviteTokenExternalKeycloakSuccessWhenMatchingT

u := performAuthorization(ts, "keycloak", code, "invite_token")

assertAuthorizationSuccess(ts, u, tokenCount, userCount, "[email protected]", "Keycloak Test", "keycloaktestid", "http://example.com/avatar")
assertAuthorizationSuccess(ts, u, tokenCount, userCount, "[email protected]", "Keycloak Test", "keycloaktestid", "")
}

func (ts *ExternalTestSuite) TestInviteTokenExternalKeycloakErrorWhenNoMatchingToken() {
Expand Down
4 changes: 2 additions & 2 deletions internal/api/external_twitch_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ func (ts *ExternalTestSuite) TestSignupExternalTwitchDisableSignupErrorWhenEmpty
func (ts *ExternalTestSuite) TestSignupExternalTwitchDisableSignupSuccessWithPrimaryEmail() {
ts.Config.DisableSignup = true

ts.createUser("twitchTestId", "[email protected]", "Twitch Test", "https://s.gravatar.com/avatar/23463b99b62a72f26ed677cc556c44e8", "")
ts.createUser("twitchTestId", "[email protected]", "Twitch user", "https://s.gravatar.com/avatar/23463b99b62a72f26ed677cc556c44e8", "")

tokenCount, userCount := 0, 0
code := "authcode"
Expand All @@ -116,7 +116,7 @@ func (ts *ExternalTestSuite) TestSignupExternalTwitchDisableSignupSuccessWithPri

u := performAuthorization(ts, "twitch", code, "")

assertAuthorizationSuccess(ts, u, tokenCount, userCount, "[email protected]", "Twitch Test", "twitchTestId", "https://s.gravatar.com/avatar/23463b99b62a72f26ed677cc556c44e8")
assertAuthorizationSuccess(ts, u, tokenCount, userCount, "[email protected]", "Twitch user", "twitchTestId", "https://s.gravatar.com/avatar/23463b99b62a72f26ed677cc556c44e8")
}

func (ts *ExternalTestSuite) TestInviteTokenExternalTwitchSuccessWhenMatchingToken() {
Expand Down
Loading

0 comments on commit 94293b7

Please sign in to comment.