feat: re-enable legacy client IDs

client/.snapshots/TestHandler-common-case=create_clients-case=4-description=non-uuid_works.json

@@ -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
+}

client/.snapshots/TestHandler-common-case=create_clients-case=5-description=setting_client_id_as_uuid_works.json

@@ -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
+}

client/client.go

@@ -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"
@@ -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.
@@ -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 {
@@ -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 {

client/client_test.go

@@ -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",

client/handler.go

@@ -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 {
@@ -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 {
@@ -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
@@ -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

client/handler_test.go

@@ -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",
@@ -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,
@@ -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)
  }

client/manager_test_helpers.go

@@ -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()))
  }
 }
@@ -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"))
@@ -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"},
@@ -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 {
@@ -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"},
@@ -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))
  }
@@ -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)