Skip to content

fix(identity): update last_sign_in_at on subsequent external sign-ins (fixes #2563) #2714

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
deepshekhardas wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix(identity): update last_sign_in_at on subsequent external sign-ins (fixes #2563) #2714

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
37bccb0
fix(external): use email_address_not_provided instead of 500 when pro…
Aug 18, 2026
65ba314
fix(identity): update last_sign_in_at on subsequent external sign-ins…
Aug 18, 2026
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: 3 additions & 1 deletion internal/api/external.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ func (a *API) internalExternalProviderCallback(w http.ResponseWriter, r *http.Re
userData := data.userData

if len(userData.Emails) == 0 && !emailOptional {
return apierrors.NewInternalServerError("Error getting user email from external provider")
return apierrors.NewUnprocessableEntityError(apierrors.ErrorCodeEmailAddressNotProvided, "Error getting user email from external provider")
}

userData.Metadata.EmailVerified = false
Expand Down Expand Up @@ -384,7 +384,9 @@ func (a *API) createAccountFromExternalIdentity(tx *storage.Connection, r *http.
user = decision.User
identity = decision.Identities[0]

now := time.Now()
identity.IdentityData = identityData
identity.LastSignInAt = &now

@depthfirst-app depthfirst-app Bot Aug 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚪ Severity: LOW

An external IdP response can drive an existing, unconfirmed identity through this branch. When unverified provider email sign-ins are disallowed, the surrounding flow returns CommitWithError, which commits changes despite issuing no token; this new assignment therefore lets rejected attempts advance last_sign_in_at and poison the identity’s sign-in/audit signal.
Helpful? Add 👍 / 👎

💡 Fix Suggestion

Suggestion: The fix requires changes at two separate locations in createAccountFromExternalIdentity:

  1. In the AccountExists case (around lines 370–375): Remove the identity.LastSignInAt = &now assignment (line 372) and drop "last_sign_in_at" from the UpdateOnly call so only identity_data is eagerly persisted — the call should become tx.UpdateOnly(identity, "identity_data"). This prevents the timestamp from being written to the database in the same transaction that can be committed via CommitWithError.

  2. In the confirmed-user success path (the else branch around line 440): After the audit log entry, add the deferred last_sign_in_at update that was removed above:

now := time.Now()
identity.LastSignInAt = &now
if terr = tx.UpdateOnly(identity, "last_sign_in_at"); terr != nil {
    return 0, nil, terr
}

This ensures last_sign_in_at is only persisted when the sign-in actually succeeds (i.e., a token will be issued), rather than being committed eagerly for rejected attempts.

if terr = tx.UpdateOnly(identity, "identity_data", "last_sign_in_at"); terr != nil {
return 0, nil, terr
}
Expand Down