fix: recognize real Postgres unique violations in IsUniqueConstraintViolatedError

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)
 	}
 

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

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{}

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")
+}