Skip to content

Commit

Permalink
feat: re-enable legacy client IDs (#3628)
Browse files Browse the repository at this point in the history
This patch changes the primary key of the `hydra_client` table. We do not expect issues, as that table is probably not overly huge in any deployment. We do however highly recommend to test the migration performance on a staging environment with a similar database setup.
  • Loading branch information
zepatrik authored Sep 19, 2023
1 parent 4b8c971 commit 5dd7d30
Show file tree
Hide file tree
Showing 69 changed files with 1,222 additions and 440 deletions.
4 changes: 0 additions & 4 deletions client/.snapshots/TestClientSDK-case=id_can_not_be_set.json

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
{
"client_id": "not-a-uuid",
"client_name": "",
"client_secret": "averylongsecret",
"redirect_uris": [
"http://localhost:3000/cb"
],
"grant_types": null,
"response_types": null,
"scope": "offline_access offline openid",
"audience": [],
"owner": "",
"policy_uri": "",
"allowed_cors_origins": [],
"tos_uri": "",
"client_uri": "",
"logo_uri": "",
"contacts": null,
"client_secret_expires_at": 0,
"subject_type": "public",
"jwks": {},
"token_endpoint_auth_method": "client_secret_basic",
"userinfo_signed_response_alg": "none",
"metadata": {},
"registration_client_uri": "http://localhost:4444/oauth2/register/not-a-uuid",
"skip_consent": false,
"authorization_code_grant_access_token_lifespan": null,
"authorization_code_grant_id_token_lifespan": null,
"authorization_code_grant_refresh_token_lifespan": null,
"client_credentials_grant_access_token_lifespan": null,
"implicit_grant_access_token_lifespan": null,
"implicit_grant_id_token_lifespan": null,
"jwt_bearer_grant_access_token_lifespan": null,
"refresh_token_grant_id_token_lifespan": null,
"refresh_token_grant_access_token_lifespan": null,
"refresh_token_grant_refresh_token_lifespan": null
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
{
"client_id": "98941dac-f963-4468-8a23-9483b1e04e3c",
"client_name": "",
"client_secret": "not too short",
"redirect_uris": [
"http://localhost:3000/cb"
],
"grant_types": null,
"response_types": null,
"scope": "offline_access offline openid",
"audience": [],
"owner": "",
"policy_uri": "",
"allowed_cors_origins": [],
"tos_uri": "",
"client_uri": "",
"logo_uri": "",
"contacts": null,
"client_secret_expires_at": 0,
"subject_type": "public",
"jwks": {},
"token_endpoint_auth_method": "client_secret_basic",
"userinfo_signed_response_alg": "none",
"metadata": {},
"registration_client_uri": "http://localhost:4444/oauth2/register/98941dac-f963-4468-8a23-9483b1e04e3c",
"skip_consent": false,
"authorization_code_grant_access_token_lifespan": null,
"authorization_code_grant_id_token_lifespan": null,
"authorization_code_grant_refresh_token_lifespan": null,
"client_credentials_grant_access_token_lifespan": null,
"implicit_grant_access_token_lifespan": null,
"implicit_grant_id_token_lifespan": null,
"jwt_bearer_grant_access_token_lifespan": null,
"refresh_token_grant_id_token_lifespan": null,
"refresh_token_grant_access_token_lifespan": null,
"refresh_token_grant_refresh_token_lifespan": null
}

This file was deleted.

19 changes: 11 additions & 8 deletions client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,18 @@
package client

import (
"database/sql"
"strconv"
"strings"
"time"

"github.com/twmb/murmur3"

"github.com/ory/hydra/v2/driver/config"
"github.com/ory/x/stringsx"

"github.com/gobuffalo/pop/v6"
"github.com/gofrs/uuid"

"github.com/ory/hydra/v2/driver/config"

"github.com/go-jose/go-jose/v3"

"github.com/ory/fosite"
Expand All @@ -35,13 +35,16 @@ var (
//
// swagger:model oAuth2Client
type Client struct {
ID uuid.UUID `json:"-" db:"pk"`
NID uuid.UUID `db:"nid" faker:"-" json:"-"`

// OAuth 2.0 Client ID
//
// The ID is autogenerated and immutable.
LegacyClientID string `json:"client_id" db:"id"`
// The ID is immutable. If no ID is provided, a UUID4 will be generated.
ID string `json:"client_id" db:"id"`

// DEPRECATED: This field is deprecated and will be removed. It serves
// no purpose except the database not complaining.
PK sql.NullString `json:"-" db:"pk" faker:"-"`

// DEPRECATED: This field is deprecated and will be removed. It serves
// no purpose except the database not complaining.
Expand Down Expand Up @@ -409,7 +412,7 @@ func (c *Client) BeforeSave(_ *pop.Connection) error {
}

func (c *Client) GetID() string {
return stringsx.Coalesce(c.LegacyClientID, c.ID.String())
return c.ID
}

func (c *Client) GetRedirectURIs() []string {
Expand All @@ -421,7 +424,7 @@ func (c *Client) GetHashedSecret() []byte {
}

func (c *Client) GetScopes() fosite.Arguments {
return fosite.Arguments(strings.Fields(c.Scope))
return strings.Fields(c.Scope)
}

func (c *Client) GetAudience() fosite.Arguments {
Expand Down
2 changes: 1 addition & 1 deletion client/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ var _ fosite.Client = new(Client)

func TestClient(t *testing.T) {
c := &Client{
LegacyClientID: "foo",
ID: "foo",
RedirectURIs: []string{"foo"},
Scope: "foo bar",
TokenEndpointAuthMethod: "none",
Expand Down
38 changes: 13 additions & 25 deletions client/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,26 +12,19 @@ import (
"strings"
"time"

"github.com/ory/x/pagination/tokenpagination"

"github.com/ory/x/httprouterx"

"github.com/ory/x/openapix"

"github.com/ory/x/uuidx"

"github.com/ory/x/jsonx"
"github.com/ory/x/urlx"
"github.com/julienschmidt/httprouter"
"github.com/pkg/errors"

"github.com/ory/fosite"

"github.com/ory/x/errorsx"

"github.com/ory/herodot"
"github.com/ory/hydra/v2/x"

"github.com/julienschmidt/httprouter"
"github.com/pkg/errors"
"github.com/ory/x/errorsx"
"github.com/ory/x/httprouterx"
"github.com/ory/x/jsonx"
"github.com/ory/x/openapix"
"github.com/ory/x/pagination/tokenpagination"
"github.com/ory/x/urlx"
"github.com/ory/x/uuidx"
)

type Handler struct {
Expand Down Expand Up @@ -171,15 +164,10 @@ func (h *Handler) CreateClient(r *http.Request, validator func(context.Context,
if c.Secret != "" {
return nil, errorsx.WithStack(herodot.ErrBadRequest.WithReasonf("It is not allowed to choose your own OAuth2 Client secret."))
}
// We do not allow to set the client ID for dynamic clients.
c.ID = uuidx.NewV4().String()
}

if len(c.LegacyClientID) > 0 {
return nil, errorsx.WithStack(herodot.ErrBadRequest.WithReason("It is no longer possible to set an OAuth2 Client ID as a user. The system will generate a unique ID for you."))
}

c.ID = uuidx.NewV4()
c.LegacyClientID = c.ID.String()

if len(c.Secret) == 0 {
secretb, err := x.GenerateSecret(26)
if err != nil {
Expand Down Expand Up @@ -266,7 +254,7 @@ func (h *Handler) setOAuth2Client(w http.ResponseWriter, r *http.Request, ps htt
return
}

c.LegacyClientID = ps.ByName("id")
c.ID = ps.ByName("id")
if err := h.updateClient(r.Context(), &c, h.r.ClientValidator().Validate); err != nil {
h.r.Writer().WriteError(w, r, err)
return
Expand Down Expand Up @@ -379,7 +367,7 @@ func (h *Handler) setOidcDynamicClient(w http.ResponseWriter, r *http.Request, p
c.RegistrationAccessToken = token
c.RegistrationAccessTokenSignature = signature

c.LegacyClientID = client.GetID()
c.ID = client.GetID()
if err := h.updateClient(r.Context(), &c, h.r.ClientValidator().ValidateDynamicRegistration); err != nil {
h.r.Writer().WriteError(w, r, err)
return
Expand Down
28 changes: 14 additions & 14 deletions client/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -309,24 +309,24 @@ func TestHandler(t *testing.T) {
statusCode: http.StatusBadRequest,
},
{
d: "non-uuid fails",
d: "non-uuid works",
payload: &client.Client{
LegacyClientID: "not-a-uuid",
Secret: "averylongsecret",
RedirectURIs: []string{"http://localhost:3000/cb"},
ID: "not-a-uuid",
Secret: "averylongsecret",
RedirectURIs: []string{"http://localhost:3000/cb"},
},
path: client.ClientsHandlerPath,
statusCode: http.StatusBadRequest,
statusCode: http.StatusCreated,
},
{
d: "setting client id fails",
d: "setting client id as uuid works",
payload: &client.Client{
LegacyClientID: "98941dac-f963-4468-8a23-9483b1e04e3c",
Secret: "short",
RedirectURIs: []string{"http://localhost:3000/cb"},
ID: "98941dac-f963-4468-8a23-9483b1e04e3c",
Secret: "not too short",
RedirectURIs: []string{"http://localhost:3000/cb"},
},
path: client.ClientsHandlerPath,
statusCode: http.StatusBadRequest,
statusCode: http.StatusCreated,
},
{
d: "setting access token strategy fails",
Expand Down Expand Up @@ -359,9 +359,9 @@ func TestHandler(t *testing.T) {
{
d: "basic dynamic client registration",
payload: &client.Client{
LegacyClientID: "ead800c5-a316-4d0c-bf00-d25666ba72cf",
Secret: "averylongsecret",
RedirectURIs: []string{"http://localhost:3000/cb"},
ID: "ead800c5-a316-4d0c-bf00-d25666ba72cf",
Secret: "averylongsecret",
RedirectURIs: []string{"http://localhost:3000/cb"},
},
path: client.DynClientsHandlerPath,
statusCode: http.StatusBadRequest,
Expand All @@ -383,7 +383,7 @@ func TestHandler(t *testing.T) {
if tc.path == client.DynClientsHandlerPath {
exclude = append(exclude, "client_id", "client_secret", "registration_client_uri")
}
if tc.payload.LegacyClientID == "" {
if tc.payload.ID == "" {
exclude = append(exclude, "client_id", "registration_client_uri")
assert.NotEqual(t, uuid.Nil.String(), gjson.Get(body, "client_id").String(), body)
}
Expand Down
34 changes: 14 additions & 20 deletions client/manager_test_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,12 @@ func TestHelperClientAutoGenerateKey(k string, m Storage) func(t *testing.T) {
RedirectURIs: []string{"http://redirect"},
TermsOfServiceURI: "foo",
}
assert.NoError(t, m.CreateClient(ctx, c))
require.NoError(t, m.CreateClient(ctx, c))
dbClient, err := m.GetClient(ctx, c.GetID())
assert.NoError(t, err)
require.NoError(t, err)
dbClientConcrete, ok := dbClient.(*Client)
assert.True(t, ok)
testhelpersuuid.AssertUUID(t, &dbClientConcrete.ID)
require.True(t, ok)
testhelpersuuid.AssertUUID(t, dbClientConcrete.ID)
assert.NoError(t, m.DeleteClient(ctx, c.GetID()))
}
}
Expand All @@ -47,9 +47,9 @@ func TestHelperClientAuthenticate(k string, m Manager) func(t *testing.T) {
return func(t *testing.T) {
ctx := context.TODO()
require.NoError(t, m.CreateClient(ctx, &Client{
LegacyClientID: "1234321",
Secret: "secret",
RedirectURIs: []string{"http://redirect"},
ID: "1234321",
Secret: "secret",
RedirectURIs: []string{"http://redirect"},
}))

c, err := m.Authenticate(ctx, "1234321", []byte("secret1"))
Expand Down Expand Up @@ -80,7 +80,7 @@ func testHelperUpdateClient(t *testing.T, ctx context.Context, network Storage,
d, err := network.GetClient(ctx, "1234")
assert.NoError(t, err)
err = network.UpdateClient(ctx, &Client{
LegacyClientID: "2-1234",
ID: "2-1234",
Name: "name-new",
Secret: "secret-new",
RedirectURIs: []string{"http://redirect/new"},
Expand Down Expand Up @@ -164,7 +164,7 @@ func TestHelperCreateGetUpdateDeleteClientNext(t *testing.T, m Storage, networks
for _, expected := range clients {
c, err := m.GetClient(ctx, expected.GetID())
if check != original {
t.Run(fmt.Sprintf("case=must not find client %s", expected.ID), func(t *testing.T) {
t.Run(fmt.Sprintf("case=must not find client %s", expected.GetID()), func(t *testing.T) {
require.ErrorIs(t, err, sqlcon.ErrNoRows)
})
} else {
Expand Down Expand Up @@ -206,8 +206,7 @@ func TestHelperCreateGetUpdateDeleteClient(k string, connection *pop.Connection,
require.Error(t, err)

t1c1 := &Client{
ID: uuid.FromStringOrNil("96bfe52e-af88-4cba-ab00-ae7a8b082228"),
LegacyClientID: "1234",
ID: "1234",
Name: "name",
Secret: "secret",
RedirectURIs: []string{"http://redirect", "http://redirect1"},
Expand Down Expand Up @@ -243,15 +242,12 @@ func TestHelperCreateGetUpdateDeleteClient(k string, connection *pop.Connection,
{
t2c1 := *t1c1
require.Error(t, connection.Create(&t2c1), "should not be able to create the same client in other manager/network; are they backed by the same database?")
t2c1.ID = uuid.Nil
require.NoError(t, t2.CreateClient(ctx, &t2c1), "we should be able to create a client with the same GetID() but different ID in other network")
require.NoError(t, t2.CreateClient(ctx, &t2c1), "we should be able to create a client with the same ID in other network")
}

t2c3 := *t1c1
{
pk, _ := uuid.NewV4()
t2c3.ID = pk
t2c3.LegacyClientID = "t2c2-1234"
t2c3.ID = "t2c2-1234"
require.NoError(t, t2.CreateClient(ctx, &t2c3))
require.Error(t, t2.CreateClient(ctx, &t2c3))
}
Expand All @@ -261,23 +257,21 @@ func TestHelperCreateGetUpdateDeleteClient(k string, connection *pop.Connection,
}

c2Template := &Client{
ID: uuid.FromStringOrNil("a6bfe52e-af88-4cba-ab00-ae7a8b082228"),
LegacyClientID: "2-1234",
ID: "2-1234",
Name: "name2",
Secret: "secret",
RedirectURIs: []string{"http://redirect"},
TermsOfServiceURI: "foo",
SecretExpiresAt: 1,
}
assert.NoError(t, t1.CreateClient(ctx, c2Template))
c2Template.ID = uuid.Nil
assert.NoError(t, t2.CreateClient(ctx, c2Template))

d, err := t1.GetClient(ctx, "1234")
require.NoError(t, err)

cc := d.(*Client)
testhelpersuuid.AssertUUID(t, &cc.NID)
testhelpersuuid.AssertUUID(t, cc.NID)

compare(t, t1c1, d, k)

Expand Down
Loading

0 comments on commit 5dd7d30

Please sign in to comment.