Skip to content

fix: unlinking an identity should respect the autoconfirm behaviour #2682

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

Open
fadymak wants to merge 1 commit into
base: master
Choose a base branch
from
+55 −16
Open

fix: unlinking an identity should respect the autoconfirm behaviour #2682

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions internal/api/identity.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ func (a *API) DeleteIdentity(w http.ResponseWriter, r *http.Request) error {
return apierrors.NewInternalServerError("Database error updating user phone").WithInternalError(terr)
}
default:
if terr := user.UpdateUserEmailFromIdentities(tx); terr != nil {
if terr := user.UpdateUserEmailFromIdentities(tx, config.Mailer.Autoconfirm); terr != nil {
if models.IsUniqueConstraintViolatedError(terr) {
return apierrors.NewUnprocessableEntityError(apierrors.ErrorCodeEmailConflictIdentityNotDeletable, "Unable to unlink identity due to email conflict").WithInternalError(terr)
}
Expand Down Expand Up @@ -185,7 +185,7 @@ func (a *API) linkIdentityToUser(r *http.Request, ctx context.Context, tx *stora
}

if targetUser.GetEmail() == "" {
if terr := targetUser.UpdateUserEmailFromIdentities(tx); terr != nil {
if terr := targetUser.UpdateUserEmailFromIdentities(tx, a.config.Mailer.Autoconfirm); terr != nil {
if models.IsUniqueConstraintViolatedError(terr) {
return nil, apierrors.NewBadRequestError(apierrors.ErrorCodeEmailExists, DuplicateEmailMsg)
}
Expand Down
16 changes: 16 additions & 0 deletions internal/api/identity_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,13 +222,16 @@ func (ts *IdentityTestSuite) TestUnlinkIdentity() {

func (ts *IdentityTestSuite) TestUnlinkIdentityEmailVerification() {
ts.Config.Security.ManualLinkingEnabled = true
originalAutoconfirm := ts.Config.Mailer.Autoconfirm
defer func() { ts.Config.Mailer.Autoconfirm = originalAutoconfirm }()

boolPtr := func(b bool) *bool { return &b }
cases := []struct {
desc string
// value of email_verified on the remaining identity; nil means
// the key is absent from identity_data
emailVerified *bool
autoconfirm bool
expectedConfirmed bool
}{
{
Expand All @@ -246,11 +249,24 @@ func (ts *IdentityTestSuite) TestUnlinkIdentityEmailVerification() {
emailVerified: nil,
expectedConfirmed: false,
},
{
desc: "Autoconfirm keeps an unverified identity email confirmed",
emailVerified: boolPtr(false),
autoconfirm: true,
expectedConfirmed: true,
},
{
desc: "Autoconfirm keeps an identity without email_verified confirmed",
emailVerified: nil,
autoconfirm: true,
expectedConfirmed: true,
},
}

for _, c := range cases {
ts.Run(c.desc, func() {
ts.SetupTest()
ts.Config.Mailer.Autoconfirm = c.autoconfirm
u, err := models.NewUser("", "primary@example.com", "password", ts.Config.JWT.Aud, nil)
require.NoError(ts.T(), err)
require.NoError(ts.T(), ts.API.db.Create(u))
Expand Down
11 changes: 6 additions & 5 deletions internal/models/user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,8 +274,9 @@ func (u *User) UpdateAppMetaDataProviders(tx *storage.Connection) error {
}

// UpdateUserEmail updates the user's email to one of the identity's email
// if the current email used doesn't match any of the identities email
func (u *User) UpdateUserEmailFromIdentities(tx *storage.Connection) error {
// if the current email used doesn't match any of the identities email.
// Unverified identity emails remain confirmed when mailer autoconfirm is enabled.
func (u *User) UpdateUserEmailFromIdentities(tx *storage.Connection, mailerAutoconfirm bool) error {
identities, terr := FindIdentitiesByUserID(tx, u.ID)
if terr != nil {
return terr
Expand Down Expand Up @@ -337,9 +338,9 @@ func (u *User) UpdateUserEmailFromIdentities(tx *storage.Connection) error {
if terr := u.ClearAllPendingTokens(tx); terr != nil {
return terr
}
if primaryIdentity.GetEmail() == "" || !primaryIdentity.IsEmailVerified() {
// the promoted email was never verified by the IdP or ourselves,
// so the user's email can no longer be considered confirmed
if primaryIdentity.GetEmail() == "" || (!primaryIdentity.IsEmailVerified() && !mailerAutoconfirm) {
// the promoted email was neither verified by the IdP nor covered by
// the project's autoconfirm policy, so it can't remain confirmed
u.EmailConfirmedAt = nil
if terr := tx.UpdateOnly(u, "email_confirmed_at"); terr != nil {
return terr
Expand Down
40 changes: 31 additions & 9 deletions internal/models/user_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,7 +487,7 @@ func (ts *UserTestSuite) TestUpdateUserEmailSuccess() {
require.NoError(ts.T(), ts.db.Create(secondaryIdentity))

// UpdateUserEmail should not do anything and the user's email should still use the primaryIdentity
require.NoError(ts.T(), userA.UpdateUserEmailFromIdentities(ts.db))
require.NoError(ts.T(), userA.UpdateUserEmailFromIdentities(ts.db, false))
require.Equal(ts.T(), primaryIdentity.GetEmail(), userA.GetEmail())
require.NotNil(ts.T(), userA.EmailConfirmedAt)

Expand All @@ -496,7 +496,7 @@ func (ts *UserTestSuite) TestUpdateUserEmailSuccess() {

// UpdateUserEmail should update the user to use the secondary identity's email
// and clear the confirmation state since the promoted email was never verified
require.NoError(ts.T(), userA.UpdateUserEmailFromIdentities(ts.db))
require.NoError(ts.T(), userA.UpdateUserEmailFromIdentities(ts.db, false))
require.Equal(ts.T(), secondaryIdentity.GetEmail(), userA.GetEmail())
require.Nil(ts.T(), userA.EmailConfirmedAt)
require.Equal(ts.T(), false, userA.UserMetaData["email_verified"])
Expand All @@ -518,7 +518,7 @@ func (ts *UserTestSuite) TestUpdateUserEmailFromVerifiedIdentity() {

// the promoted email was verified by the identity provider so the
// user's confirmation state should be kept
require.NoError(ts.T(), userA.UpdateUserEmailFromIdentities(ts.db))
require.NoError(ts.T(), userA.UpdateUserEmailFromIdentities(ts.db, false))
require.Equal(ts.T(), secondaryIdentity.GetEmail(), userA.GetEmail())
require.NotNil(ts.T(), userA.EmailConfirmedAt)
}
Expand All @@ -539,12 +539,34 @@ func (ts *UserTestSuite) TestUpdateUserEmailFromUnverifiedIdentity() {

// the promoted email was never verified so the user should no longer
// be considered confirmed
require.NoError(ts.T(), userA.UpdateUserEmailFromIdentities(ts.db))
require.NoError(ts.T(), userA.UpdateUserEmailFromIdentities(ts.db, false))
require.Equal(ts.T(), secondaryIdentity.GetEmail(), userA.GetEmail())
require.Nil(ts.T(), userA.EmailConfirmedAt)
require.Equal(ts.T(), false, userA.UserMetaData["email_verified"])
}

func (ts *UserTestSuite) TestUpdateUserEmailFromUnverifiedIdentityWithAutoconfirm() {
userA, err := NewUser("", "foo@example.com", "", "authenticated", nil)
require.NoError(ts.T(), err)
require.NoError(ts.T(), ts.db.Create(userA))
require.NoError(ts.T(), userA.Confirm(ts.db))

secondaryIdentity, err := NewIdentity(userA, "google", map[string]any{
"sub": userA.ID.String(),
"email": "bar@example.com",
"email_verified": false,
})
require.NoError(ts.T(), err)
require.NoError(ts.T(), ts.db.Create(secondaryIdentity))

// autoconfirm keeps the promoted email confirmed even when the identity
// provider did not verify it
require.NoError(ts.T(), userA.UpdateUserEmailFromIdentities(ts.db, true))
require.Equal(ts.T(), secondaryIdentity.GetEmail(), userA.GetEmail())
require.NotNil(ts.T(), userA.EmailConfirmedAt)
require.Equal(ts.T(), true, userA.UserMetaData["email_verified"])
}

func (ts *UserTestSuite) TestUpdateUserEmailPrefersVerifiedIdentity() {
userA, err := NewUser("", "foo@example.com", "", "authenticated", nil)
require.NoError(ts.T(), err)
Expand All @@ -569,7 +591,7 @@ func (ts *UserTestSuite) TestUpdateUserEmailPrefersVerifiedIdentity() {

// the verified identity should be promoted even though the unverified
// identity was created first
require.NoError(ts.T(), userA.UpdateUserEmailFromIdentities(ts.db))
require.NoError(ts.T(), userA.UpdateUserEmailFromIdentities(ts.db, false))
require.Equal(ts.T(), verifiedIdentity.GetEmail(), userA.GetEmail())
require.NotNil(ts.T(), userA.EmailConfirmedAt)
}
Expand Down Expand Up @@ -602,7 +624,7 @@ func (ts *UserTestSuite) TestUpdateUserEmailVerifiedConflictFallsBack() {

// the verified identity's email is taken by userB so the unverified
// identity is promoted instead, which unconfirms the user
require.NoError(ts.T(), userA.UpdateUserEmailFromIdentities(ts.db))
require.NoError(ts.T(), userA.UpdateUserEmailFromIdentities(ts.db, false))
require.Equal(ts.T(), unverifiedIdentity.GetEmail(), userA.GetEmail())
require.Nil(ts.T(), userA.EmailConfirmedAt)
require.Equal(ts.T(), false, userA.UserMetaData["email_verified"])
Expand Down Expand Up @@ -659,7 +681,7 @@ func (ts *UserTestSuite) TestUpdateUserEmailClearsStaleTokens() {

// promoting another identity's email must revoke every outstanding
// token addressed to the previous email
require.NoError(ts.T(), userA.UpdateUserEmailFromIdentities(ts.db))
require.NoError(ts.T(), userA.UpdateUserEmailFromIdentities(ts.db, false))
require.Equal(ts.T(), secondaryIdentity.GetEmail(), userA.GetEmail())

userA, err = FindUserByID(ts.db, userA.ID)
Expand Down Expand Up @@ -713,7 +735,7 @@ func (ts *UserTestSuite) TestUpdateUserEmailFromEmptyClearsStaleTokens() {

// promoting an identity's email over an empty one is still a primary
// email transition, so outstanding tokens must be revoked
require.NoError(ts.T(), userA.UpdateUserEmailFromIdentities(ts.db))
require.NoError(ts.T(), userA.UpdateUserEmailFromIdentities(ts.db, false))
require.Equal(ts.T(), identity.GetEmail(), userA.GetEmail())

userA, err = FindUserByID(ts.db, userA.ID)
Expand Down Expand Up @@ -755,7 +777,7 @@ func (ts *UserTestSuite) TestUpdateUserEmailFailure() {

// UpdateUserEmail should fail with the email unique constraint violation error
// since userB is using the secondary identity's email
require.ErrorIs(ts.T(), userA.UpdateUserEmailFromIdentities(ts.db), UserEmailUniqueConflictError{})
require.ErrorIs(ts.T(), userA.UpdateUserEmailFromIdentities(ts.db, false), UserEmailUniqueConflictError{})
require.Equal(ts.T(), primaryIdentity.GetEmail(), userA.GetEmail())
}

Expand Down
Loading