Skip to content

feat: deprecate autoincrement primary key in hydra_client #2784

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

Merged
aeneasr merged 7 commits into from
Oct 11, 2021
Merged

feat: deprecate autoincrement primary key in hydra_client #2784

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
26aa373
feat: deprecate autoincrement primary key in hydra_client
grantzvolsky Oct 7, 2021
4047a3f
test: refactor expectedClient template
grantzvolsky Oct 8, 2021
385de00
refactor: rename hydra_client.pk_new to hydra_client.pk
grantzvolsky Oct 8, 2021
dbc748d
fix: ensure migration generates UUIDv4, variant 1
grantzvolsky Oct 8, 2021
c4334b9
test: ensure CreateClient generates UUIDv4 variant 1
grantzvolsky Oct 8, 2021
94668d0
fix: avoid duplicate client ID in migration tests
grantzvolsky Oct 8, 2021
1d20c54
fix: add blank migrations, refactor UUID test
grantzvolsky Oct 11, 2021
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
6 changes: 5 additions & 1 deletion client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"time"

"github.com/gobuffalo/pop/v5"
"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

Expand All @@ -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"`
Expand Down
1 change: 1 addition & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ require (
github.com/gobuffalo/pop/v5 v5.3.4
github.com/gobuffalo/x v0.0.0-20181007152206-913e47c59ca7
github.com/gobwas/glob v0.2.3
github.com/gofrs/uuid v3.2.0+incompatible
github.com/golang/mock v1.6.0
github.com/google/uuid v1.2.0
github.com/gorilla/securecookie v1.1.1
Expand Down
7 changes: 5 additions & 2 deletions persistence/sql/migratest/exptected_data.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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),
Expand Down
21 changes: 19 additions & 2 deletions persistence/sql/migratest/migration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import (
"fmt"
"testing"

"github.com/gofrs/uuid"

"github.com/ory/x/configx"

"github.com/ory/hydra/persistence/sql"
Expand All @@ -26,6 +28,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()
Expand Down Expand Up @@ -72,9 +79,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)
lastClient = actual
})
Expand Down Expand Up @@ -136,6 +145,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
Expand Down
75 changes: 75 additions & 0 deletions persistence/sql/migratest/testdata/20211004110001_testdata.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"}',
''
);
1 change: 1 addition & 0 deletions persistence/sql/migrations/20211004110001000000_change_client_primary_key.cockroach.down.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER TABLE hydra_client DROP CONSTRAINT "primary", ADD CONSTRAINT "primary" PRIMARY KEY (pk_deprecated);
2 changes: 2 additions & 0 deletions persistence/sql/migrations/20211004110001000000_change_client_primary_key.cockroach.up.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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();
4 changes: 4 additions & 0 deletions persistence/sql/migrations/20211004110001000000_change_client_primary_key.mysql.down.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
14 changes: 14 additions & 0 deletions persistence/sql/migrations/20211004110001000000_change_client_primary_key.mysql.up.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
5 changes: 5 additions & 0 deletions persistence/sql/migrations/20211004110001000000_change_client_primary_key.postgres.down.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
16 changes: 16 additions & 0 deletions persistence/sql/migrations/20211004110001000000_change_client_primary_key.postgres.up.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);
115 changes: 115 additions & 0 deletions persistence/sql/migrations/20211004110001000000_change_client_primary_key.sqlite.down.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);
Loading