Skip to content
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

Jump to bottom

feat: re-enable legacy client IDs #3628

Merged
merged 13 commits into from
Sep 19, 2023
Merged

feat: re-enable legacy client IDs #3628

merged 13 commits into from
Sep 19, 2023

Conversation

zepatrik
Copy link
Member

@zepatrik zepatrik commented Sep 6, 2023

closes #2911

@codecov
Copy link

codecov bot commented Sep 6, 2023 •
edited
Loading

Codecov Report

Merging #3628 (61ad8cf) into master (d1f9ba8) will decrease coverage by 0.13%.
Report is 2 commits behind head on master.
The diff coverage is 100.00%.

❗ Current head 61ad8cf differs from pull request most recent head 7a77c49. Consider uploading reports for the commit 7a77c49 to get more accurate results

@@            Coverage Diff             @@
##           master    #3628      +/-   ##
==========================================
- Coverage   76.18%   76.06%   -0.13%     
==========================================
  Files         133      132       -1     
  Lines       10043     9971      -72     
==========================================
- Hits         7651     7584      -67     
+ Misses       1868     1866       -2     
+ Partials      524      521       -3
Files Changed Coverage Δ
persistence/sql/persister_migration.go 22.22% <ø> (ø)
client/client.go 77.27% <100.00%> (ø)
client/handler.go 78.16% <100.00%> (-0.29%) ⬇️
client/manager_test_helpers.go 98.49% <100.00%> (-0.04%) ⬇️
consent/manager_test_helpers.go 98.12% <100.00%> (-0.01%) ⬇️
consent/strategy_default.go 68.67% <100.00%> (ø)
oauth2/fosite_store_helpers.go 100.00% <100.00%> (ø)
persistence/sql/migratest/assertion_helpers.go 100.00% <100.00%> (ø)
persistence/sql/persister.go 85.54% <100.00%> (ø)
persistence/sql/persister_client.go 84.44% <100.00%> (-0.51%) ⬇️
... and 1 more

... and 3 files with indirect coverage changes

@zepatrik zepatrik self-assigned this Sep 8, 2023
aeneasr
aeneasr reviewed Sep 8, 2023
View reviewed changes
persistence/sql/persister_client.go
Comment on lines -56 to -57
// set the internal primary key
cl.ID = o.ID
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can we remove this?

Copy link
Member Author

@zepatrik zepatrik Sep 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous UUID (pk) is basically not used. For cockroach, we can just use a hash-sharded index to avoid hot-spots. Mysql, postgres, and sqlite are fine with sequential keys (mysql actually prefers them).

@zepatrik zepatrik force-pushed the feat/re-enable-legacy-ids branch from bcd0f5c to 233c8df Compare September 8, 2023 15:21
@zepatrik zepatrik requested a review from aeneasr September 11, 2023 08:26
@zepatrik zepatrik requested a review from alnr September 12, 2023 10:00
alnr
alnr previously approved these changes Sep 13, 2023
View reviewed changes
Copy link
Contributor

@alnr alnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, lots of tedious work. Well done!

@alnr
Copy link
Contributor

alnr commented Sep 13, 2023

Couple more failing tests... LMK if you want help.

alnr
alnr previously approved these changes Sep 15, 2023
View reviewed changes
aeneasr
aeneasr reviewed Sep 18, 2023
View reviewed changes
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check all foreign key relations, and check that pk has no foreign key relations either.

persistence/sql/migrations/20230908104443000000_change_client_pk.cockroach.up.sql
@@ -0,0 +1 @@
ALTER TABLE hydra_client ALTER PRIMARY KEY USING COLUMNS (id, nid) USING HASH;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that id can be user-set, this can lead to ID collisions. We need to ensure that all foreign key lookups consider the full primary key.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked, and all foreign-keys on the client table use (id, nid):

select col.constraint_name, col.table_name, column_name, ordinal_position, referenced_table_name from information_schema.key_column_usage as col join information_schema.referential_constraints as constr on col.constraint_name = constr.constraint_name where referenced_table_name != 'networks';
                        constraint_name                       |                   table_name                   |   column_name    | ordinal_position |        referenced_table_name
--------------------------------------------------------------+------------------------------------------------+------------------+------------------+--------------------------------------
  hydra_oauth2_obfuscated_authentication_session_client_id_fk | hydra_oauth2_obfuscated_authentication_session | client_id        |                1 | hydra_client
  hydra_oauth2_obfuscated_authentication_session_client_id_fk | hydra_oauth2_obfuscated_authentication_session | nid              |                2 | hydra_client
  hydra_oauth2_logout_request_client_id_fk                    | hydra_oauth2_logout_request                    | client_id        |                1 | hydra_client
  hydra_oauth2_logout_request_client_id_fk                    | hydra_oauth2_logout_request                    | nid              |                2 | hydra_client
  hydra_oauth2_access_challenge_id_fk                         | hydra_oauth2_access                            | challenge_id     |                1 | hydra_oauth2_flow
  hydra_oauth2_access_client_id_fk                            | hydra_oauth2_access                            | client_id        |                1 | hydra_client
  hydra_oauth2_access_client_id_fk                            | hydra_oauth2_access                            | nid              |                2 | hydra_client
  hydra_oauth2_refresh_challenge_id_fk                        | hydra_oauth2_refresh                           | challenge_id     |                1 | hydra_oauth2_flow
  hydra_oauth2_refresh_client_id_fk                           | hydra_oauth2_refresh                           | client_id        |                1 | hydra_client
  hydra_oauth2_refresh_client_id_fk                           | hydra_oauth2_refresh                           | nid              |                2 | hydra_client
  hydra_oauth2_code_challenge_id_fk                           | hydra_oauth2_code                              | challenge_id     |                1 | hydra_oauth2_flow
  hydra_oauth2_code_client_id_fk                              | hydra_oauth2_code                              | client_id        |                1 | hydra_client
  hydra_oauth2_code_client_id_fk                              | hydra_oauth2_code                              | nid              |                2 | hydra_client
  hydra_oauth2_oidc_challenge_id_fk                           | hydra_oauth2_oidc                              | challenge_id     |                1 | hydra_oauth2_flow
  hydra_oauth2_oidc_client_id_fk                              | hydra_oauth2_oidc                              | client_id        |                1 | hydra_client
  hydra_oauth2_oidc_client_id_fk                              | hydra_oauth2_oidc                              | nid              |                2 | hydra_client
  hydra_oauth2_pkce_challenge_id_fk                           | hydra_oauth2_pkce                              | challenge_id     |                1 | hydra_oauth2_flow
  hydra_oauth2_pkce_client_id_fk                              | hydra_oauth2_pkce                              | client_id        |                1 | hydra_client
  hydra_oauth2_pkce_client_id_fk                              | hydra_oauth2_pkce                              | nid              |                2 | hydra_client
  hydra_oauth2_flow_client_id_fk                              | hydra_oauth2_flow                              | client_id        |                1 | hydra_client
  hydra_oauth2_flow_client_id_fk                              | hydra_oauth2_flow                              | nid              |                2 | hydra_client
  hydra_oauth2_flow_login_session_id_fk                       | hydra_oauth2_flow                              | login_session_id |                1 | hydra_oauth2_authentication_session
  fk_key_set_ref_hydra_jwk                                    | hydra_oauth2_trusted_jwt_bearer_issuer         | key_set          |                1 | hydra_jwk
  fk_key_set_ref_hydra_jwk                                    | hydra_oauth2_trusted_jwt_bearer_issuer         | key_id           |                2 | hydra_jwk
  fk_key_set_ref_hydra_jwk                                    | hydra_oauth2_trusted_jwt_bearer_issuer         | nid              |                3 | hydra_jwk

@zepatrik
Copy link
Member Author

Same way, pk is only used for the primary key:

select * from information_schema.key_column_usage where table_name = 'hydra_client';                                                                                                                                                                                                                 
  constraint_catalog | constraint_schema |     constraint_name     | table_catalog | table_schema |  table_name  | column_name | ordinal_position | position_in_unique_constraint
---------------------+-------------------+-------------------------+---------------+--------------+--------------+-------------+------------------+--------------------------------
  ory_hydra          | public            | hydra_client_pkey       | ory_hydra     | public       | hydra_client | pk          |                1 |                          NULL
  ory_hydra          | public            | hydra_client_id_key     | ory_hydra     | public       | hydra_client | id          |                1 |                          NULL
  ory_hydra          | public            | hydra_client_id_key     | ory_hydra     | public       | hydra_client | nid         |                2 |                          NULL
  ory_hydra          | public            | hydra_client_nid_fk_idx | ory_hydra     | public       | hydra_client | nid         |                1 |                             1

@aeneasr
Copy link
Member

aeneasr commented Sep 19, 2023

Nice! So I think we can drop this then

@aeneasr
Copy link
Member

aeneasr commented Sep 19, 2023

Do we have any indication how fast this will be on different systems? Some of our customers have rather large DBs already.

aeneasr
aeneasr previously approved these changes Sep 19, 2023
View reviewed changes
@zepatrik zepatrik dismissed stale reviews from aeneasr and alnr via 7a77c49 September 19, 2023 08:09
@aeneasr
Copy link
Member

aeneasr commented Sep 19, 2023

If you add the commit body to the PR description I can squash force merge (docker image scanner is failing)

@zepatrik zepatrik merged commit 5dd7d30 into master Sep 19, 2023
@zepatrik zepatrik deleted the feat/re-enable-legacy-ids branch September 19, 2023 09:20
@afreakk afreakk mentioned this pull request Feb 13, 2024
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aeneasr aeneasr aeneasr approved these changes

@alnr alnr alnr left review comments

@hperl hperl Awaiting requested review from hperl

Assignees

@zepatrik zepatrik

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

No longer allow users to set the client ID
3 participants
@zepatrik @alnr @aeneasr