feat: re-enable legacy client IDs (#3628)

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.

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)