WIP

ory · Sep 8, 2023 · 233c8df · 233c8df
1 parent c692b1d
commit 233c8df
Show file tree

Hide file tree

Showing 60 changed files with 680 additions and 359 deletions.
diff --git a/client/client.go b/client/client.go
@@ -10,12 +10,11 @@ import (
 
 	"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 +34,12 @@ 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.
@@ -409,7 +407,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 +419,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 {

diff --git a/client/client_test.go b/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",

diff --git a/client/handler.go b/client/handler.go
@@ -164,10 +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()
 	}
 
-	c.ID = uuidx.NewV4()
-
 	if len(c.Secret) == 0 {
 		secretb, err := x.GenerateSecret(26)
 		if err != nil {
@@ -254,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
@@ -367,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

diff --git a/client/handler_test.go b/client/handler_test.go
@@ -311,19 +311,19 @@ func TestHandler(t *testing.T) {
 				{
 					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.StatusCreated,
 				},
 				{
 					d: "setting client id as uuid works",
 					payload: &client.Client{
-						LegacyClientID: "98941dac-f963-4468-8a23-9483b1e04e3c",
-						Secret:         "not too 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.StatusCreated,
@@ -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)
 					}

diff --git a/client/manager_test_helpers.go b/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)