feat: deprecate autoincrement primary key in hydra_client (#2784)

Closes #2781

client/client.go

@@ -25,6 +25,7 @@ import (
 	"time"
 
 	"github.com/gobuffalo/pop/v6"
+	"github.com/gofrs/uuid"
 
 	jose "gopkg.in/square/go-jose.v2" // Naming the dependency jose is important for go-swagger to work, see https://github.com/go-swagger/go-swagger/issues/1587
 
@@ -37,7 +38,10 @@ import (
 //
 // swagger:model oAuth2Client
 type Client struct {
-	ID int64 `json:"-" db:"pk"`
+	ID uuid.UUID `json:"-" db:"pk"`
+
+	// This field is deprecated and will be removed
+	PKDeprecated int64 `json:"-" db:"pk_deprecated"`
 
 	// ID  is the id for this client.
 	OutfacingID string `json:"client_id" db:"id"`

go.mod

@@ -27,6 +27,7 @@ require (
 	github.com/gobuffalo/pop/v6 v6.0.1
 	github.com/gobuffalo/x v0.0.0-20181007152206-913e47c59ca7
 	github.com/gobwas/glob v0.2.3
+	github.com/gofrs/uuid v4.1.0+incompatible
 	github.com/golang/mock v1.6.0
 	github.com/google/uuid v1.3.0
 	github.com/gorilla/securecookie v1.1.1

persistence/sql/migratest/exptected_data.go

@@ -6,6 +6,8 @@ import (
 
 	"gopkg.in/square/go-jose.v2"
 
+	"github.com/gofrs/uuid"
+
 	"github.com/ory/hydra/client"
 	"github.com/ory/hydra/consent"
 	"github.com/ory/hydra/jwk"
@@ -15,9 +17,10 @@ import (
 	"github.com/ory/x/sqlxx"
 )
 
-func expectedClient(i int) *client.Client {
+func expectedClient(id uuid.UUID, i int) *client.Client {
 	c := &client.Client{
-		ID:                                int64(i),
+		ID:                                id,
+		PKDeprecated:                      int64(i),
 		OutfacingID:                       fmt.Sprintf("client-%04d", i),
 		Name:                              fmt.Sprintf("Client %04d", i),
 		Secret:                            fmt.Sprintf("secret-%04d", i),

persistence/sql/migratest/migration_test.go

@@ -7,6 +7,7 @@ import (
 	"testing"
 
 	"github.com/gobuffalo/pop/v6"
+	"github.com/gofrs/uuid"
 
 	"github.com/ory/x/configx"
 
@@ -28,6 +29,11 @@ import (
 	"github.com/ory/hydra/x"
 )
 
+func assertUUID(t *testing.T, id *uuid.UUID) {
+	require.Equal(t, id.Version(), uuid.V4)
+	require.Equal(t, id.Variant(), uuid.VariantRFC4122)
+}
+
 func TestMigrations(t *testing.T) {
 	if testing.Short() {
 		t.SkipNow()
@@ -74,9 +80,11 @@ func TestMigrations(t *testing.T) {
 					continue
 				}
 				t.Run(fmt.Sprintf("case=client migration %d", i), func(t *testing.T) {
-					expected := expectedClient(i)
 					actual := &client.Client{}
-					require.NoError(t, c.Where("id = ?", expected.OutfacingID).First(actual))
+					outfacingID := fmt.Sprintf("client-%04d", i)
+					require.NoError(t, c.Where("id = ?", outfacingID).First(actual))
+					assertUUID(t, &actual.ID)
+					expected := expectedClient(actual.ID, i)
 					assertEqualClients(t, expected, actual)
 					if i == 14 {
 						client14 = actual
@@ -140,6 +148,14 @@ func TestMigrations(t *testing.T) {
 				})
 			}
 
+			t.Run("case=client migration 20211004/description=new client ID should be valid UUIDv4 variant 1", func(t *testing.T) {
+				outfacingID := "2021100400"
+				require.NoError(t, d.Persister().CreateClient(context.Background(), &client.Client{OutfacingID: outfacingID}))
+				actual := &client.Client{}
+				require.NoError(t, c.Where("id = ?", outfacingID).First(actual))
+				assertUUID(t, &actual.ID)
+			})
+
 			// TODO https://github.com/ory/hydra/issues/1815
 			// this is very stupid and should be replaced as soon the manager uses pop
 			// necessary because the manager does not provide any way to access the data

persistence/sql/migratest/testdata/20211004110001_testdata.sql

@@ -0,0 +1,75 @@
+INSERT INTO hydra_client
+(
+  pk,
+  pk_deprecated,
+  id,
+  client_name,
+  client_secret,
+  redirect_uris,
+  grant_types,
+  response_types,
+  scope,
+  owner,
+  policy_uri,
+  tos_uri,
+  client_uri,
+  logo_uri,
+  contacts,
+  client_secret_expires_at,
+  sector_identifier_uri,
+  jwks,
+  jwks_uri,
+  request_uris,
+  token_endpoint_auth_method,
+  request_object_signing_alg,
+  userinfo_signed_response_alg,
+  subject_type,
+  allowed_cors_origins,
+  audience,
+  created_at,
+  updated_at,
+  frontchannel_logout_uri,
+  frontchannel_logout_session_required,
+  post_logout_redirect_uris,
+  backchannel_logout_uri,
+  backchannel_logout_session_required,
+  metadata,
+  token_endpoint_auth_signing_alg
+)
+VALUES
+('08f4a4b7-6601-4fd7-bb7f-29ec0681b86d',
+  0,
+  'client-20',
+  'Client 20',
+  'secret-20',
+  'http://redirect/20_1',
+  'grant-20_1',
+  'response-20_1',
+  'scope-20',
+  'owner-20',
+  'http://policy/20',
+  'http://tos/20',
+  'http://client/20',
+  'http://logo/20',
+  'contact-20_1',
+  0,
+  'http://sector_id/20',
+  '',
+  'http://jwks/20',
+  'http://request/20_1',
+  'token_auth-20',
+  'r_alg-20',
+  'u_alg-20',
+  'subject-20',
+  'http://cors/20_1',
+  'autdience-20_1',
+  now(),
+  now(),
+  'http://front_logout/20',
+  true,
+  'http://post_redirect/20_1',
+  'http://back_logout/20',
+  true,
+  '{"migration": "20"}',
+  ''
+);

persistence/sql/migrations/20211004110001000000_change_client_primary_key.cockroach.down.sql

@@ -0,0 +1 @@
+ALTER TABLE hydra_client DROP CONSTRAINT "primary", ADD CONSTRAINT "primary" PRIMARY KEY (pk_deprecated);

persistence/sql/migrations/20211004110001000000_change_client_primary_key.cockroach.up.sql

@@ -0,0 +1,2 @@
+ALTER TABLE hydra_client RENAME pk TO pk_deprecated;
+ALTER TABLE hydra_client ADD pk UUID NOT NULL DEFAULT gen_random_uuid();

persistence/sql/migrations/20211004110001000000_change_client_primary_key.mysql.down.sql

@@ -0,0 +1,4 @@
+ALTER TABLE hydra_client RENAME pk TO pk_tmp;
+ALTER TABLE hydra_client CHANGE COLUMN pk_deprecated pk INT UNSIGNED AUTO_INCREMENT;
+ALTER TABLE hydra_client DROP PRIMARY KEY, ADD PRIMARY KEY (pk);
+ALTER TABLE hydra_client DROP pk_tmp;

persistence/sql/migrations/20211004110001000000_change_client_primary_key.mysql.up.sql

@@ -0,0 +1,14 @@
+ALTER TABLE hydra_client CHANGE COLUMN pk pk_deprecated INT UNSIGNED;
+ALTER TABLE hydra_client ADD COLUMN pk CHAR(36);
+-- UUIDv4 generation based on https://stackoverflow.com/a/66868340/12723442
+UPDATE hydra_client SET pk = (SELECT LOWER(CONCAT(
+    HEX(RANDOM_BYTES(4)),
+    '-', HEX(RANDOM_BYTES(2)),
+    '-4', SUBSTR(HEX(RANDOM_BYTES(2)), 2, 3),
+    '-', CONCAT(HEX(FLOOR(ASCII(RANDOM_BYTES(1)) / 64)+8),SUBSTR(HEX(RANDOM_BYTES(2)), 2, 3)),
+    '-', HEX(RANDOM_BYTES(6))
+)));
+ALTER TABLE hydra_client ALTER pk DROP DEFAULT;
+ALTER TABLE hydra_client DROP PRIMARY KEY, ADD PRIMARY KEY (pk);
+ALTER TABLE hydra_client ADD KEY (pk_deprecated);
+ALTER TABLE hydra_client CHANGE COLUMN pk_deprecated pk_deprecated INT UNSIGNED AUTO_INCREMENT;

persistence/sql/migrations/20211004110001000000_change_client_primary_key.postgres.down.sql

@@ -0,0 +1,5 @@
+ALTER TABLE hydra_client RENAME pk TO pk_tmp;
+ALTER TABLE hydra_client RENAME pk_deprecated TO pk;
+ALTER TABLE hydra_client DROP CONSTRAINT hydra_client_pkey;
+ALTER TABLE hydra_client ADD PRIMARY KEY (pk);
+ALTER TABLE hydra_client DROP pk_tmp;

persistence/sql/migrations/20211004110001000000_change_client_primary_key.postgres.up.sql

@@ -0,0 +1,16 @@
+ALTER TABLE hydra_client RENAME pk TO pk_deprecated;
+-- UUID generation based on https://stackoverflow.com/a/21327318/12723442
+ALTER TABLE hydra_client ADD COLUMN pk UUID DEFAULT uuid_in(
+  overlay(
+    overlay(
+      md5(random()::text || ':' || clock_timestamp()::text)
+      placing '4'
+      from 13
+    )
+    placing to_hex(floor(random()*(11-8+1) + 8)::int)::text
+    from 17
+  )::cstring
+);
+ALTER TABLE hydra_client ALTER pk DROP DEFAULT;
+ALTER TABLE hydra_client DROP CONSTRAINT hydra_client_pkey;
+ALTER TABLE hydra_client ADD PRIMARY KEY (pk);

persistence/sql/migrations/20211004110001000000_change_client_primary_key.sqlite.down.sql

@@ -0,0 +1,115 @@
+CREATE TABLE "_hydra_client_tmp"
+(
+    id                                   VARCHAR(255) NOT NULL,
+    client_name                          TEXT         NOT NULL,
+    client_secret                        TEXT         NOT NULL,
+    redirect_uris                        TEXT         NOT NULL,
+    grant_types                          TEXT         NOT NULL,
+    response_types                       TEXT         NOT NULL,
+    scope                                TEXT         NOT NULL,
+    owner                                TEXT         NOT NULL,
+    policy_uri                           TEXT         NOT NULL,
+    tos_uri                              TEXT         NOT NULL,
+    client_uri                           TEXT         NOT NULL,
+    logo_uri                             TEXT         NOT NULL,
+    contacts                             TEXT         NOT NULL,
+    client_secret_expires_at             INTEGER      NOT NULL DEFAULT 0,
+    sector_identifier_uri                TEXT         NOT NULL,
+    jwks                                 TEXT         NOT NULL,
+    jwks_uri                             TEXT         NOT NULL,
+    request_uris                         TEXT         NOT NULL,
+    token_endpoint_auth_method           VARCHAR(25)  NOT NULL DEFAULT '',
+    request_object_signing_alg           VARCHAR(10)  NOT NULL DEFAULT '',
+    userinfo_signed_response_alg         VARCHAR(10)  NOT NULL DEFAULT '',
+    subject_type                         VARCHAR(15)  NOT NULL DEFAULT '',
+    allowed_cors_origins                 TEXT         NOT NULL,
+    pk                                   INTEGER      PRIMARY KEY,
+    audience                             TEXT         NOT NULL,
+    created_at                           TIMESTAMP    NOT NULL DEFAULT CURRENT_TIMESTAMP,
+    updated_at                           TIMESTAMP    NOT NULL DEFAULT CURRENT_TIMESTAMP,
+    frontchannel_logout_uri              TEXT         NOT NULL DEFAULT '',
+    frontchannel_logout_session_required INTEGER      NOT NULL DEFAULT false,
+    post_logout_redirect_uris            TEXT         NOT NULL DEFAULT '',
+    backchannel_logout_uri               TEXT         NOT NULL DEFAULT '',
+    backchannel_logout_session_required  INTEGER      NOT NULL DEFAULT false,
+    metadata                             TEXT         NOT NULL DEFAULT '{}',
+    token_endpoint_auth_signing_alg      VARCHAR(10)  NOT NULL DEFAULT ''
+);
+
+INSERT INTO "_hydra_client_tmp" (
+    id,
+    client_name,
+    client_secret,
+    redirect_uris,
+    grant_types,
+    response_types,
+    scope,
+    owner,
+    policy_uri,
+    tos_uri,
+    client_uri,
+    logo_uri,
+    contacts,
+    client_secret_expires_at,
+    sector_identifier_uri,
+    jwks,
+    jwks_uri,
+    request_uris,
+    token_endpoint_auth_method,
+    request_object_signing_alg,
+    userinfo_signed_response_alg,
+    subject_type,
+    allowed_cors_origins,
+    pk,
+    audience,
+    created_at,
+    updated_at,
+    frontchannel_logout_uri,
+    frontchannel_logout_session_required,
+    post_logout_redirect_uris,
+    backchannel_logout_uri,
+    backchannel_logout_session_required,
+    metadata,
+    token_endpoint_auth_signing_alg
+) SELECT
+    id,
+    client_name,
+    client_secret,
+    redirect_uris,
+    grant_types,
+    response_types,
+    scope,
+    owner,
+    policy_uri,
+    tos_uri,
+    client_uri,
+    logo_uri,
+    contacts,
+    client_secret_expires_at,
+    sector_identifier_uri,
+    jwks,
+    jwks_uri,
+    request_uris,
+    token_endpoint_auth_method,
+    request_object_signing_alg,
+    userinfo_signed_response_alg,
+    subject_type,
+    allowed_cors_origins,
+    pk_deprecated,
+    audience,
+    created_at,
+    updated_at,
+    frontchannel_logout_uri,
+    frontchannel_logout_session_required,
+    post_logout_redirect_uris,
+    backchannel_logout_uri,
+    backchannel_logout_session_required,
+    metadata,
+    token_endpoint_auth_signing_alg
+FROM "hydra_client";
+
+DROP INDEX hydra_client_id_idx;
+DROP TABLE "hydra_client";
+ALTER TABLE "_hydra_client_tmp" RENAME TO "hydra_client";
+
+CREATE UNIQUE INDEX hydra_client_id_idx ON hydra_client (id);