From 876789b79cfe92ba57ad32a03c083e65f6c383fb Mon Sep 17 00:00:00 2001 From: rakshit-gen Date: Wed, 16 Sep 2026 01:13:41 +0530 Subject: [PATCH] fix: recognize real Postgres unique violations in IsUniqueConstraintViolatedError IsUniqueConstraintViolatedError only matched the package's own hand raised UserEmailUniqueConflictError and never matched a real Postgres unique_violation (23505) coming back from a Create or Update call. This meant every caller that relied on it to turn a database race into a clean 4xx was silently broken. Extend the helper to also recognize a *pgconn.PgError with code 23505, and add the missing conflict check to createNewIdentity in external.go, which had no handling at all for a duplicate identity insert. Fixes #2810 --- internal/api/external.go | 3 +++ internal/api/identity_test.go | 29 +++++++++++++++++++++++++++++ internal/models/errors.go | 19 +++++++++++++++++-- internal/models/errors_test.go | 24 ++++++++++++++++++++++++ 4 files changed, 73 insertions(+), 2 deletions(-) create mode 100644 internal/models/errors_test.go diff --git a/internal/api/external.go b/internal/api/external.go index 41195d1dbc..b5b890bce5 100644 --- a/internal/api/external.go +++ b/internal/api/external.go @@ -910,6 +910,9 @@ func (a *API) createNewIdentity(tx *storage.Connection, user *models.User, provi } if terr := tx.Create(identity); terr != nil { + if models.IsUniqueConstraintViolatedError(terr) { + return nil, apierrors.NewUnprocessableEntityError(apierrors.ErrorCodeIdentityAlreadyExists, "Identity is already linked to another user").WithInternalError(terr) + } return nil, apierrors.NewInternalServerError("Error creating identity").WithInternalError(terr) } diff --git a/internal/api/identity_test.go b/internal/api/identity_test.go index d56f3714e1..e1b4a148bd 100644 --- a/internal/api/identity_test.go +++ b/internal/api/identity_test.go @@ -130,6 +130,35 @@ func (ts *IdentityTestSuite) TestLinkIdentityToUser() { require.Len(ts.T(), logs, 1, "an already-linked identity must not emit another audit log") } +// TestCreateNewIdentityRace covers what happens when two concurrent link +// requests for the same provider identity both pass linkIdentityToUser's +// FindIdentityByIdAndProvider check before either has committed. The loser +// of that race only finds out it lost when its own INSERT hits the +// identities_provider_id_provider_unique constraint. createNewIdentity must +// turn that into the same IdentityAlreadyExists conflict the pre-check +// returns, not a raw internal error. +func (ts *IdentityTestSuite) TestCreateNewIdentityRace() { + winner, err := models.FindUserByEmailAndAudience(ts.API.db, "one@example.com", ts.Config.JWT.Aud) + require.NoError(ts.T(), err) + + loser, err := models.FindUserByEmailAndAudience(ts.API.db, "two@example.com", ts.Config.JWT.Aud) + require.NoError(ts.T(), err) + + // the winner of the race already committed its identity by the time the + // loser's createNewIdentity call reaches the database + _, err = ts.API.createNewIdentity(ts.API.db, winner, "race_provider", map[string]interface{}{ + "sub": "race_subject", + }) + require.NoError(ts.T(), err) + + // the loser already passed the not-found check earlier and now tries to + // insert the same (provider, provider_id) pair + _, err = ts.API.createNewIdentity(ts.API.db, loser, "race_provider", map[string]interface{}{ + "sub": "race_subject", + }) + require.ErrorIs(ts.T(), err, apierrors.NewUnprocessableEntityError(apierrors.ErrorCodeIdentityAlreadyExists, "Identity is already linked to another user")) +} + func (ts *IdentityTestSuite) TestUnlinkIdentityError() { manualLinkingEnabled := ts.Config.Security.ManualLinkingEnabled ts.Config.Security.ManualLinkingEnabled = true diff --git a/internal/models/errors.go b/internal/models/errors.go index 149541d44f..7ffa68162a 100644 --- a/internal/models/errors.go +++ b/internal/models/errors.go @@ -1,6 +1,11 @@ package models -import "errors" +import ( + "errors" + + "github.com/jackc/pgconn" + "github.com/jackc/pgerrcode" +) // sentinel error for all not found errors. var errNotFound = errors.New("not found") @@ -136,8 +141,18 @@ func (e FlowStateNotFoundError) Is(target error) bool { return target == errNotFound } +// IsUniqueConstraintViolatedError returns whether an error represents a +// unique constraint violation. This covers this package's own hand-raised +// conflict errors (e.g. UserEmailUniqueConflictError) as well as a real +// Postgres unique_violation (SQLSTATE 23505) coming straight out of a +// Create or Update call, which is not wrapped into a sentinel anywhere else. func IsUniqueConstraintViolatedError(err error) bool { - return errors.Is(err, errUniqueConstraintViolated) + if errors.Is(err, errUniqueConstraintViolated) { + return true + } + + var pgErr *pgconn.PgError + return errors.As(err, &pgErr) && pgErr.Code == pgerrcode.UniqueViolation } type UserEmailUniqueConflictError struct{} diff --git a/internal/models/errors_test.go b/internal/models/errors_test.go new file mode 100644 index 0000000000..96e51f6ff8 --- /dev/null +++ b/internal/models/errors_test.go @@ -0,0 +1,24 @@ +package models + +import ( + "fmt" + "testing" + + "github.com/jackc/pgconn" + "github.com/pkg/errors" + "github.com/stretchr/testify/require" +) + +func TestIsUniqueConstraintViolatedError(t *testing.T) { + require.True(t, IsUniqueConstraintViolatedError(UserEmailUniqueConflictError{}), "should recognize the package's own conflict error") + + pgErr := &pgconn.PgError{Code: "23505", Message: "duplicate key value violates unique constraint"} + require.True(t, IsUniqueConstraintViolatedError(pgErr), "should recognize a raw Postgres unique_violation") + require.True(t, IsUniqueConstraintViolatedError(errors.Wrap(pgErr, "creating identity")), "should recognize a wrapped Postgres unique_violation") + + otherPgErr := &pgconn.PgError{Code: "23503", Message: "foreign key violation"} + require.False(t, IsUniqueConstraintViolatedError(otherPgErr), "a different Postgres error code must not be treated as a unique violation") + + require.False(t, IsUniqueConstraintViolatedError(fmt.Errorf("some other error")), "an unrelated error must not be treated as a unique violation") + require.False(t, IsUniqueConstraintViolatedError(nil), "a nil error must not be treated as a unique violation") +}