Repository: supabase/gotrue
Pull request #1311 (2023-11-17 22:43:46)

feat: update primary key for identities table
## What kind of change does this PR introduce?
* We need to be able to have a consistent identifier to fetch an identity from the database. The current approach for using a composite primary key isn't sufficient and not ideal for exposing through the API.
* Remove the composite primary key on (`auth.identities.id`, `auth.identities.provider`)
* Rename `auth.identities.id` to `auth.identities.provider_id`
* Add a new primary key called `auth.identities.id` 
* Add a unique constraint on (`auth.identities.provider_id`, `auth.identities.provider`)


Patch:
diff --git a/internal/api/token_refresh.go b/internal/api/token_refresh.go
index 2e91e2c9..c492ccbe 100644
--- a/internal/api/token_refresh.go
+++ b/internal/api/token_refresh.go
@@ -168,7 +168,7 @@ func (a *API) RefreshTokenGrant(ctx context.Context, w http.ResponseWriter, r *h
 
 			if token.Revoked {
 				activeRefreshToken, terr := session.FindCurrentlyActiveRefreshToken(tx)
-				if terr != nil {
+				if terr != nil && !models.IsNotFoundError(terr) {
 					return internalServerError(terr.Error())
 				}
 
@@ -199,7 +199,7 @@ func (a *API) RefreshTokenGrant(ctx context.Context, w http.ResponseWriter, r *h
 							}
 						}
 
-						return oauthError("invalid_grant", "Invalid Refresh Token: Already Used").WithInternalMessage("Possible abuse attempt: %v", token.ID)
+						return storage.NewCommitWithError(oauthError("invalid_grant", "Invalid Refresh Token: Already Used").WithInternalMessage("Possible abuse attempt: %v", token.ID))
 					}
 				}
 			}
diff --git a/internal/conf/configuration.go b/internal/conf/configuration.go
index cc5b7505..a3778a5c 100644
--- a/internal/conf/configuration.go
+++ b/internal/conf/configuration.go
@@ -250,7 +250,7 @@ func (c *SMTPConfiguration) Validate() error {
 
 type MailerConfiguration struct {
 	Autoconfirm                 bool `json:"autoconfirm"`
-	AllowUnverifiedEmailSignIns bool `json:"allow_unverified_email_sign_ins" split_words:"true" default:"true"`
+	AllowUnverifiedEmailSignIns bool `json:"allow_unverified_email_sign_ins" split_words:"true" default:"false"`
 
 	Subjects  EmailContentConfiguration `json:"subjects"`
 	Templates EmailContentConfiguration `json:"templates"`
diff --git a/internal/models/identity.go b/internal/models/identity.go
index 2e8f87a7..1ada9130 100644
--- a/internal/models/identity.go
+++ b/internal/models/identity.go
@@ -12,7 +12,12 @@ import (
 )
 
 type Identity struct {
-	ID           string     `json:"id" db:"id"`
+	// returned as identity_id in JSON for backward compatibility with the interface exposed by the client library
+	// see https://github.com/supabase/gotrue-js/blob/c9296bbc27a2f036af55c1f33fca5930704bd021/src/lib/types.ts#L230-L240
+	ID uuid.UUID `json:"identity_id" db:"id"`
+	// returned as id in JSON for backward compatibility with the interface exposed by the client library
+	// see https://github.com/supabase/gotrue-js/blob/c9296bbc27a2f036af55c1f33fca5930704bd021/src/lib/types.ts#L230-L240
+	ProviderID   string     `json:"id" db:"provider_id"`
 	UserID       uuid.UUID  `json:"user_id" db:"user_id"`
 	IdentityData JSONMap    `json:"identity_data,omitempty" db:"identity_data"`
 	Provider     string     `json:"provider" db:"provider"`
@@ -35,7 +40,7 @@ func NewIdentity(user *User, provider string, identityData map[string]interface{
 	now := time.Now()
 
 	identity := &Identity{
-		ID:           providerId.(string),
+		ProviderID:   providerId.(string),
 		UserID:       user.ID,
 		IdentityData: identityData,
 		Provider:     provider,
@@ -63,7 +68,7 @@ func (i *Identity) IsForSSOProvider() bool {
 // FindIdentityById searches for an identity with the matching id and provider given.
 func FindIdentityByIdAndProvider(tx *storage.Connection, providerId, provider string) (*Identity, error) {
 	identity := &Identity{}
-	if err := tx.Q().Where("id = ? AND provider = ?", providerId, provider).First(identity); err != nil {
+	if err := tx.Q().Where("provider_id = ? AND provider = ?", providerId, provider).First(identity); err != nil {
 		if errors.Cause(err) == sql.ErrNoRows {
 			return nil, IdentityNotFoundError{}
 		}
@@ -117,9 +122,8 @@ func (i *Identity) UpdateIdentityData(tx *storage.Connection, updates map[string
 	}
 	// pop doesn't support updates on tables with composite primary keys so we use a raw query here.
 	return tx.RawQuery(
-		"update "+(&pop.Model{Value: Identity{}}).TableName()+" set identity_data = ? where provider = ? and id = ?",
+		"update "+(&pop.Model{Value: Identity{}}).TableName()+" set identity_data = ? where id = ?",
 		i.IdentityData,
-		i.Provider,
 		i.ID,
 	).Exec()
 }
diff --git a/internal/models/refresh_token.go b/internal/models/refresh_token.go
index 4235c411..4d62ace7 100644
--- a/internal/models/refresh_token.go
+++ b/internal/models/refresh_token.go
@@ -94,6 +94,10 @@ func RevokeTokenFamily(tx *storage.Connection, token *RefreshToken) error {
 		update `+tablename+` r set revoked = true from token_family where token_family.id = r.id;`, token.Token).Exec()
 	}
 	if err != nil {
+		if errors.Cause(err) == sql.ErrNoRows || errors.Is(err, sql.ErrNoRows) {
+			return nil
+		}
+
 		return err
 	}
 	return nil
diff --git a/internal/models/sessions.go b/internal/models/sessions.go
index 0ec8a1c2..bce30c76 100644
--- a/internal/models/sessions.go
+++ b/internal/models/sessions.go
@@ -345,6 +345,10 @@ func (s *Session) FindCurrentlyActiveRefreshToken(tx *storage.Connection) (*Refr
 	var activeRefreshToken RefreshToken
 
 	if err := tx.Q().Where("session_id = ? and revoked is false", s.ID).Order("id desc").First(&activeRefreshToken); err != nil {
+		if errors.Cause(err) == sql.ErrNoRows || errors.Is(err, sql.ErrNoRows) {
+			return nil, RefreshTokenNotFoundError{}
+		}
+
 		return nil, err
 	}
 
diff --git a/internal/models/user.go b/internal/models/user.go
index b9debb1a..7e415f09 100644
--- a/internal/models/user.go
+++ b/internal/models/user.go
@@ -673,9 +673,7 @@ func (u *User) RemoveUnconfirmedIdentities(tx *storage.Connection, identity *Ide
 
 	// finally, remove all identities except the current identity being authenticated
 	for i := range u.Identities {
-		identityId := u.Identities[i].Provider + u.Identities[i].ID
-		identityIdToKeep := identity.Provider + identity.ID
-		if identityId != identityIdToKeep {
+		if u.Identities[i].ID != identity.ID {
 			if terr := tx.Destroy(&u.Identities[i]); terr != nil {
 				return terr
 			}
@@ -762,10 +760,9 @@ func (u *User) SoftDeleteUserIdentities(tx *storage.Connection) error {
 		if err := tx.RawQuery(
 			"update "+
 				(&pop.Model{Value: Identity{}}).TableName()+
-				" set id = ? where id = ? and provider = ?",
-			obfuscateIdentityId(identity),
+				" set provider_id = ? where id = ?",
+			obfuscateIdentityProviderId(identity),
 			identity.ID,
-			identity.Provider,
 		).Exec(); err != nil {
 			return err
 		}
@@ -787,6 +784,6 @@ func obfuscatePhone(u *User, phone string) string {
 	return obfuscateValue(u.ID, phone)[:15]
 }
 
-func obfuscateIdentityId(identity *Identity) string {
-	return obfuscateValue(identity.UserID, identity.Provider+":"+identity.ID)
+func obfuscateIdentityProviderId(identity *Identity) string {
+	return obfuscateValue(identity.UserID, identity.Provider+":"+identity.ProviderID)
 }
diff --git a/migrations/20231117164230_add_id_pkey_identities.up.sql b/migrations/20231117164230_add_id_pkey_identities.up.sql
new file mode 100644
index 00000000..31ed280d
--- /dev/null
+++ b/migrations/20231117164230_add_id_pkey_identities.up.sql
@@ -0,0 +1,29 @@
+do $$
+begin
+    if not exists(select * 
+        from information_schema.columns
+        where table_schema = '{{ index .Options "Namespace" }}' and table_name='identities' and column_name='provider_id')
+    then
+        alter table if exists {{ index .Options "Namespace" }}.identities 
+        rename column id to provider_id;
+    end if;
+end$$;
+
+alter table if exists {{ index .Options "Namespace" }}.identities 
+    drop constraint if exists identities_pkey,
+    add column if not exists id uuid default gen_random_uuid() primary key;
+
+do $$
+begin
+  if not exists
+     (select constraint_name
+      from information_schema.table_constraints
+      where table_schema = '{{ index .Options "Namespace" }}'
+      and table_name = 'identities'
+      and constraint_name = 'identities_provider_id_provider_unique')
+  then
+    alter table if exists {{ index .Options "Namespace" }}.identities 
+    add constraint identities_provider_id_provider_unique 
+    unique(provider_id, provider);
+  end if;
+end $$;


Test patch:
diff --git a/internal/api/admin_test.go b/internal/api/admin_test.go
index c3f5615f..204a14eb 100644
--- a/internal/api/admin_test.go
+++ b/internal/api/admin_test.go
@@ -438,7 +438,7 @@ func (ts *AdminTestSuite) TestAdminUserUpdate() {
 
 	for _, identity := range u.Identities {
 		// for email & phone identities, the providerId is the same as the userId
-		require.Equal(ts.T(), u.ID.String(), identity.ID)
+		require.Equal(ts.T(), u.ID.String(), identity.ProviderID)
 		require.Equal(ts.T(), u.ID, identity.UserID)
 		if identity.Provider == "email" {
 			require.Equal(ts.T(), newEmail, identity.IdentityData["email"])
diff --git a/internal/api/token_test.go b/internal/api/token_test.go
index dbb9b013..d2c19b50 100644
--- a/internal/api/token_test.go
+++ b/internal/api/token_test.go
@@ -362,84 +362,110 @@ func (ts *TokenTestSuite) TestTokenRefreshTokenGrantFailure() {
 	assert.Equal(ts.T(), http.StatusBadRequest, w.Code)
 }
 
-func (ts *TokenTestSuite) TestTokenRefreshTokenRotation() {
-	u, err := models.NewUser("", "foo@example.com", "password", ts.Config.JWT.Aud, nil)
-	require.NoError(ts.T(), err, "Error creating test user model")
-	t := time.Now()
-	u.EmailConfirmedAt = &t
-	require.NoError(ts.T(), ts.API.db.Create(u), "Error saving foo user")
+func (ts *TokenTestSuite) TestRefreshTokenReuseRevocation() {
+	originalSecurity := ts.API.config.Security
 
-	first, err := models.GrantAuthenticatedUser(ts.API.db, u, models.GrantParams{})
-	require.NoError(ts.T(), err)
-	second, err := models.GrantRefreshTokenSwap(&http.Request{}, ts.API.db, u, first)
-	require.NoError(ts.T(), err)
+	ts.API.config.Security.RefreshTokenRotationEnabled = true
+	ts.API.config.Security.RefreshTokenReuseInterval = 0
 
-	cases := []struct {
-		desc                        string
-		refreshTokenRotationEnabled bool
-		reuseInterval               int
-		refreshToken                string
-		expectedCode                int
-		expectedBody                map[string]interface{}
-	}{
-		{
-			desc:                        "Valid refresh within reuse interval",
-			refreshTokenRotationEnabled: true,
-			reuseInterval:               30,
-			refreshToken:                second.Token,
-			expectedCode:                http.StatusOK,
-			expectedBody: map[string]interface{}{
-				"refresh_token": "some-new-refresh-token",
-			},
-		},
-		{
-			desc:                        "Invalid refresh outside reuse interval",
-			refreshTokenRotationEnabled: true,
-			reuseInterval:               0,
-			refreshToken:                first.Token,
-			expectedCode:                http.StatusBadRequest,
-			expectedBody: map[string]interface{}{
-				"error":             "invalid_grant",
-				"error_description": "Invalid Refresh Token: Already Used",
-			},
-		},
-		{
-			desc:                        "Invalid refresh, revoke third token",
-			refreshTokenRotationEnabled: true,
-			reuseInterval:               0,
-			refreshToken:                first.Token,
-			expectedCode:                http.StatusBadRequest,
-			expectedBody: map[string]interface{}{
-				"error":             "invalid_grant",
-				"error_description": "Invalid Refresh Token: Already Used",
-			},
-		},
+	defer func() {
+		ts.API.config.Security = originalSecurity
+	}()
+
+	refreshTokens := []string{
+		ts.RefreshToken.Token,
 	}
 
-	for _, c := range cases {
-		ts.Run(c.desc, func() {
-			ts.Config.Security.RefreshTokenRotationEnabled = c.refreshTokenRotationEnabled
-			ts.Config.Security.RefreshTokenReuseInterval = c.reuseInterval
-			var buffer bytes.Buffer
-			require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{
-				"refresh_token": c.refreshToken,
-			}))
-			req := httptest.NewRequest(http.MethodPost, "http://localhost/token?grant_type=refresh_token", &buffer)
-			req.Header.Set("Content-Type", "application/json")
-			w := httptest.NewRecorder()
-			ts.API.handler.ServeHTTP(w, req)
-			assert.Equal(ts.T(), c.expectedCode, w.Code)
-
-			data := make(map[string]interface{})
-			require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&data))
-			for k, v := range c.expectedBody {
-				if k == "refresh_token" {
-					require.NotEmpty(ts.T(), v, data[k])
-				} else {
-					require.Equal(ts.T(), v, data[k])
-				}
-			}
-		})
+	for i := 0; i < 3; i += 1 {
+		var buffer bytes.Buffer
+		require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{
+			"refresh_token": refreshTokens[len(refreshTokens)-1],
+		}))
+
+		req := httptest.NewRequest(http.MethodPost, "http://localhost/token?grant_type=refresh_token", &buffer)
+		req.Header.Set("Content-Type", "application/json")
+
+		w := httptest.NewRecorder()
+		ts.API.handler.ServeHTTP(w, req)
+
+		assert.Equal(ts.T(), http.StatusOK, w.Code)
+
+		var response struct {
+			RefreshToken string `json:"refresh_token"`
+		}
+
+		require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&response))
+
+		refreshTokens = append(refreshTokens, response.RefreshToken)
+	}
+
+	// ensure that the 4 refresh tokens are setup correctly
+	for i, refreshToken := range refreshTokens {
+		_, token, _, err := models.FindUserWithRefreshToken(ts.API.db, refreshToken, false)
+		require.NoError(ts.T(), err)
+
+		if i == len(refreshTokens)-1 {
+			require.False(ts.T(), token.Revoked)
+		} else {
+			require.True(ts.T(), token.Revoked)
+		}
+	}
+
+	// try to reuse the first (earliest) refresh token which should trigger the family revocation logic
+	var buffer bytes.Buffer
+	require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{
+		"refresh_token": refreshTokens[0],
+	}))
+
+	req := httptest.NewRequest(http.MethodPost, "http://localhost/token?grant_type=refresh_token", &buffer)
+	req.Header.Set("Content-Type", "application/json")
+
+	w := httptest.NewRecorder()
+	ts.API.handler.ServeHTTP(w, req)
+
+	assert.Equal(ts.T(), http.StatusBadRequest, w.Code)
+
+	var response struct {
+		Error            string `json:"error"`
+		ErrorDescription string `json:"error_description"`
+	}
+
+	require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&response))
+	require.Equal(ts.T(), response.Error, "invalid_grant")
+	require.Equal(ts.T(), response.ErrorDescription, "Invalid Refresh Token: Already Used")
+
+	// ensure that the refresh tokens are marked as revoked in the database
+	for _, refreshToken := range refreshTokens {
+		_, token, _, err := models.FindUserWithRefreshToken(ts.API.db, refreshToken, false)
+		require.NoError(ts.T(), err)
+
+		require.True(ts.T(), token.Revoked)
+	}
+
+	// finally ensure that none of the refresh tokens can be reused any
+	// more, starting with the previously valid one
+	for i := len(refreshTokens) - 1; i >= 0; i -= 1 {
+		var buffer bytes.Buffer
+		require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{
+			"refresh_token": refreshTokens[i],
+		}))
+
+		req := httptest.NewRequest(http.MethodPost, "http://localhost/token?grant_type=refresh_token", &buffer)
+		req.Header.Set("Content-Type", "application/json")
+
+		w := httptest.NewRecorder()
+		ts.API.handler.ServeHTTP(w, req)
+
+		assert.Equal(ts.T(), http.StatusBadRequest, w.Code, "For refresh token %d", i)
+
+		var response struct {
+			Error            string `json:"error"`
+			ErrorDescription string `json:"error_description"`
+		}
+
+		require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&response))
+		require.Equal(ts.T(), response.Error, "invalid_grant", "For refresh token %d", i)
+		require.Equal(ts.T(), response.ErrorDescription, "Invalid Refresh Token: Already Used", "For refresh token %d", i)
 	}
 }