Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow unverified email signins #1301

Merged
merged 13 commits into from
Nov 16, 2023
Merged

feat: allow unverified email signins #1301

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
a82826a
add allow_unverified_email_sign_ins config
kangmingtay Nov 7, 2023
59f42e1
refactor: update user metadata
kangmingtay Nov 8, 2023
51cf60f
feat: allow unverified email oauth identity to sign in
kangmingtay Nov 8, 2023
f93d98a
fix tests
kangmingtay Nov 8, 2023
53cce71
refactor: return error earlier
kangmingtay Nov 13, 2023
fcb44cb
refactor: simplify IsDuplicatedUser
kangmingtay Nov 13, 2023
659b46f
feat: update automatic linking algorithm
kangmingtay Nov 13, 2023
4b96048
fix tests
kangmingtay Nov 14, 2023
5a680e3
Merge branch 'master' into km/feat-allow-unverified-email-signins
kangmingtay Nov 14, 2023
ce08f7a
fix: repeated sign ins should use the existing user's email
kangmingtay Nov 14, 2023
8f4de31
refactor: rename import
kangmingtay Nov 14, 2023
ad52a15
fix: return nil if error is special
kangmingtay Nov 15, 2023
10edffe
refactor: add new lines to group sub config logically
kangmingtay Nov 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
153 changes: 69 additions & 84 deletions internal/api/external.go
View file
Open in desktop
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 @@ -196,10 +196,6 @@ 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) {
return nil
}

return terr
}
}
Expand All @@ -218,11 +214,30 @@ func (a *API) internalExternalProviderCallback(w http.ResponseWriter, r *http.Re
}
return nil
})

rurl := a.getExternalRedirectURL(r)
if err != nil {
msg := ""
if errors.Is(err, errEmailVerificationRequired) {
msg = fmt.Sprintf("Unverified email with %v. Verify the email with %v in order to sign in", providerType, providerType)
} else if errors.Is(err, errEmailConfirmationSent) {
msg = fmt.Sprintf("Unverified email with %v. A confirmation email has been sent to your %v email", providerType, providerType)
}

if errors.Is(err, errEmailVerificationRequired) || errors.Is(err, errEmailConfirmationSent) {
kangmingtay marked this conversation as resolved.
Show resolved Hide resolved
flowType := models.ImplicitFlow
if flowState != nil {
flowType = models.PKCEFlow
}
rurl, err = a.prepErrorRedirectURL(unauthorizedError(msg), w, r, rurl, flowType)
if err != nil {
return err
}
http.Redirect(w, r, rurl, http.StatusFound)
}
return err
}

rurl := a.getExternalRedirectURL(r)
if flowState != nil {
// This means that the callback is using PKCE
// Set the flowState.AuthCode to the query param here
Expand All @@ -244,12 +259,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)
J0 marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return err
}
}

http.Redirect(w, r, rurl, http.StatusFound)
Expand All @@ -261,26 +270,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 +286,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 +299,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 +327,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 +345,49 @@ 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
}
// email must be verified to issue a token
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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