fix: client update breaks primary key (#2150)

Closes #2148
ory · Oct 29, 2020 · 7662917 · 7662917
1 parent 0b1de34
commit 7662917
Show file tree

Hide file tree

Showing 29 changed files with 201 additions and 154 deletions.
diff --git a/client/client.go b/client/client.go
@@ -37,10 +37,10 @@ import (
 //
 // swagger:model oAuth2Client
 type Client struct {
-	PK int64 `json:"-" db:"pk"`
+	ID int64 `json:"-" db:"pk"`
 
 	// ID  is the id for this client.
-	ID string `json:"client_id" db:"id"`
+	OutfacingID string `json:"client_id" db:"id"`
 
 	// Name is the human-readable string name of the client to be presented to the
 	// end-user during authorization.
@@ -235,7 +235,7 @@ func (c *Client) BeforeSave(_ *pop.Connection) error {
 }
 
 func (c *Client) GetID() string {
-	return c.ID
+	return c.OutfacingID
 }
 
 func (c *Client) GetRedirectURIs() []string {

diff --git a/client/client_test.go b/client/client_test.go
@@ -33,7 +33,7 @@ var _ fosite.Client = new(Client)
 
 func TestClient(t *testing.T) {
 	c := &Client{
-		ID:                      "foo",
+		OutfacingID:             "foo",
 		RedirectURIs:            []string{"foo"},
 		Scope:                   "foo bar",
 		TokenEndpointAuthMethod: "none",

diff --git a/client/handler.go b/client/handler.go
@@ -149,7 +149,7 @@ func (h *Handler) Update(w http.ResponseWriter, r *http.Request, ps httprouter.P
 		secret = c.Secret
 	}
 
-	c.ID = ps.ByName("id")
+	c.OutfacingID = ps.ByName("id")
 	if err := h.r.ClientValidator().Validate(&c); err != nil {
 		h.r.Writer().WriteError(w, r, err)
 		return

diff --git a/client/manager_test_helpers.go b/client/manager_test_helpers.go
@@ -39,7 +39,7 @@ func TestHelperClientAutoGenerateKey(k string, m Storage) func(t *testing.T) {
 	return func(t *testing.T) {
 		ctx := context.TODO()
 		c := &Client{
-			ID:                "foo",
+			OutfacingID:       "foo",
 			Secret:            "secret",
 			RedirectURIs:      []string{"http://redirect"},
 			TermsOfServiceURI: "foo",
@@ -54,7 +54,7 @@ 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{
-			ID:           "1234321",
+			OutfacingID:  "1234321",
 			Secret:       "secret",
 			RedirectURIs: []string{"http://redirect"},
 		}))
@@ -68,14 +68,28 @@ func TestHelperClientAuthenticate(k string, m Manager) func(t *testing.T) {
 	}
 }
 
+func TestHelperUpdateTwoClients(_ string, m Manager) func(t *testing.T) {
+	return func(t *testing.T) {
+		c1, c2 := &Client{OutfacingID: "klojdfc", Name: "test client 1"}, &Client{OutfacingID: "jlsdfkj", Name: "test client 2"}
+
+		require.NoError(t, m.CreateClient(context.Background(), c1))
+		require.NoError(t, m.CreateClient(context.Background(), c2))
+
+		c1.Name, c2.Name = "updated klojdfc client 1", "updated klojdfc client 2"
+
+		assert.NoError(t, m.UpdateClient(context.Background(), c1))
+		assert.NoError(t, m.UpdateClient(context.Background(), c2))
+	}
+}
+
 func TestHelperCreateGetUpdateDeleteClient(k string, m Storage) func(t *testing.T) {
 	return func(t *testing.T) {
-		ctx := context.TODO()
-		_, err := m.GetClient(ctx, "4321")
-		assert.NotNil(t, err)
+		ctx := context.Background()
+		_, err := m.GetClient(ctx, "1234")
+		require.Error(t, err)
 
 		c := &Client{
-			ID:                                "1234",
+			OutfacingID:                       "1234",
 			Name:                              "name",
 			Secret:                            "secret",
 			RedirectURIs:                      []string{"http://redirect", "http://redirect1"},
@@ -107,14 +121,14 @@ func TestHelperCreateGetUpdateDeleteClient(k string, m Storage) func(t *testing.
 			BackChannelLogoutSessionRequired:  true,
 		}
 
-		assert.NoError(t, m.CreateClient(ctx, c))
+		require.NoError(t, m.CreateClient(ctx, c))
 		assert.Equal(t, c.GetID(), "1234")
 		if k != "http" {
 			assert.NotEmpty(t, c.GetHashedSecret())
 		}
 
 		assert.NoError(t, m.CreateClient(ctx, &Client{
-			ID:                "2-1234",
+			OutfacingID:       "2-1234",
 			Name:              "name",
 			Secret:            "secret",
 			RedirectURIs:      []string{"http://redirect"},
@@ -130,8 +144,8 @@ func TestHelperCreateGetUpdateDeleteClient(k string, m Storage) func(t *testing.
 		ds, err := m.GetClients(ctx, 100, 0)
 		assert.NoError(t, err)
 		assert.Len(t, ds, 2)
-		assert.NotEqual(t, ds[0].ID, ds[1].ID)
-		assert.NotEqual(t, ds[0].ID, ds[1].ID)
+		assert.NotEqual(t, ds[0].OutfacingID, ds[1].OutfacingID)
+		assert.NotEqual(t, ds[0].OutfacingID, ds[1].OutfacingID)
 		// test if SecretExpiresAt was set properly
 		assert.Equal(t, ds[0].SecretExpiresAt, 0)
 		assert.Equal(t, ds[1].SecretExpiresAt, 1)
@@ -144,7 +158,7 @@ func TestHelperCreateGetUpdateDeleteClient(k string, m Storage) func(t *testing.
 		assert.NoError(t, err)
 
 		err = m.UpdateClient(ctx, &Client{
-			ID:                "2-1234",
+			OutfacingID:       "2-1234",
 			Name:              "name-new",
 			Secret:            "secret-new",
 			RedirectURIs:      []string{"http://redirect/new"},

diff --git a/client/validator.go b/client/validator.go
@@ -70,7 +70,7 @@ func NewValidatorWithClient(conf Configuration, client *http.Client) *Validator
 
 func (v *Validator) Validate(c *Client) error {
 	id := uuid.New()
-	c.ID = stringsx.Coalesce(c.ID, id)
+	c.OutfacingID = stringsx.Coalesce(c.OutfacingID, id)
 
 	if c.TokenEndpointAuthMethod == "" {
 		c.TokenEndpointAuthMethod = "client_secret_basic"

diff --git a/client/validator_test.go b/client/validator_test.go
@@ -53,59 +53,59 @@ func TestValidate(t *testing.T) {
 		{
 			in: new(Client),
 			check: func(t *testing.T, c *Client) {
-				assert.NotEmpty(t, c.ID)
+				assert.NotEmpty(t, c.OutfacingID)
 				assert.NotEmpty(t, c.GetID())
-				assert.Equal(t, c.GetID(), c.ID)
+				assert.Equal(t, c.GetID(), c.OutfacingID)
 			},
 		},
 		{
-			in: &Client{ID: "foo"},
+			in: &Client{OutfacingID: "foo"},
 			check: func(t *testing.T, c *Client) {
-				assert.Equal(t, c.GetID(), c.ID)
+				assert.Equal(t, c.GetID(), c.OutfacingID)
 			},
 		},
 		{
-			in: &Client{ID: "foo"},
+			in: &Client{OutfacingID: "foo"},
 			check: func(t *testing.T, c *Client) {
-				assert.Equal(t, c.GetID(), c.ID)
+				assert.Equal(t, c.GetID(), c.OutfacingID)
 			},
 		},
 		{
-			in:        &Client{ID: "foo", UserinfoSignedResponseAlg: "foo"},
+			in:        &Client{OutfacingID: "foo", UserinfoSignedResponseAlg: "foo"},
 			expectErr: true,
 		},
 		{
-			in:        &Client{ID: "foo", TokenEndpointAuthMethod: "private_key_jwt"},
+			in:        &Client{OutfacingID: "foo", TokenEndpointAuthMethod: "private_key_jwt"},
 			expectErr: true,
 		},
 		{
-			in:        &Client{ID: "foo", JSONWebKeys: &x.JoseJSONWebKeySet{JSONWebKeySet: new(jose.JSONWebKeySet)}, JSONWebKeysURI: "asdf", TokenEndpointAuthMethod: "private_key_jwt"},
+			in:        &Client{OutfacingID: "foo", JSONWebKeys: &x.JoseJSONWebKeySet{JSONWebKeySet: new(jose.JSONWebKeySet)}, JSONWebKeysURI: "asdf", TokenEndpointAuthMethod: "private_key_jwt"},
 			expectErr: true,
 		},
 		{
-			in:        &Client{ID: "foo", JSONWebKeys: &x.JoseJSONWebKeySet{JSONWebKeySet: new(jose.JSONWebKeySet)}, TokenEndpointAuthMethod: "private_key_jwt", TokenEndpointAuthSigningAlgorithm: "HS256"},
+			in:        &Client{OutfacingID: "foo", JSONWebKeys: &x.JoseJSONWebKeySet{JSONWebKeySet: new(jose.JSONWebKeySet)}, TokenEndpointAuthMethod: "private_key_jwt", TokenEndpointAuthSigningAlgorithm: "HS256"},
 			expectErr: true,
 		},
 		{
-			in:        &Client{ID: "foo", PostLogoutRedirectURIs: []string{"https://bar/"}, RedirectURIs: []string{"https://foo/"}},
+			in:        &Client{OutfacingID: "foo", PostLogoutRedirectURIs: []string{"https://bar/"}, RedirectURIs: []string{"https://foo/"}},
 			expectErr: true,
 		},
 		{
-			in:        &Client{ID: "foo", PostLogoutRedirectURIs: []string{"http://foo/"}, RedirectURIs: []string{"https://foo/"}},
+			in:        &Client{OutfacingID: "foo", PostLogoutRedirectURIs: []string{"http://foo/"}, RedirectURIs: []string{"https://foo/"}},
 			expectErr: true,
 		},
 		{
-			in:        &Client{ID: "foo", PostLogoutRedirectURIs: []string{"https://foo:1234/"}, RedirectURIs: []string{"https://foo/"}},
+			in:        &Client{OutfacingID: "foo", PostLogoutRedirectURIs: []string{"https://foo:1234/"}, RedirectURIs: []string{"https://foo/"}},
 			expectErr: true,
 		},
 		{
-			in: &Client{ID: "foo", PostLogoutRedirectURIs: []string{"https://foo/"}, RedirectURIs: []string{"https://foo/"}},
+			in: &Client{OutfacingID: "foo", PostLogoutRedirectURIs: []string{"https://foo/"}, RedirectURIs: []string{"https://foo/"}},
 			check: func(t *testing.T, c *Client) {
 				assert.Equal(t, []string{"https://foo/"}, []string(c.PostLogoutRedirectURIs))
 			},
 		},
 		{
-			in: &Client{ID: "foo"},
+			in: &Client{OutfacingID: "foo"},
 			check: func(t *testing.T, c *Client) {
 				assert.Equal(t, "public", c.SubjectType)
 			},
@@ -115,19 +115,19 @@ func TestValidate(t *testing.T) {
 				viper.Set(configuration.ViperKeySubjectTypesSupported, []string{"pairwise"})
 				return NewValidator(c)
 			},
-			in: &Client{ID: "foo"},
+			in: &Client{OutfacingID: "foo"},
 			check: func(t *testing.T, c *Client) {
 				assert.Equal(t, "pairwise", c.SubjectType)
 			},
 		},
 		{
-			in: &Client{ID: "foo", SubjectType: "pairwise"},
+			in: &Client{OutfacingID: "foo", SubjectType: "pairwise"},
 			check: func(t *testing.T, c *Client) {
 				assert.Equal(t, "pairwise", c.SubjectType)
 			},
 		},
 		{
-			in:        &Client{ID: "foo", SubjectType: "foo"},
+			in:        &Client{OutfacingID: "foo", SubjectType: "foo"},
 			expectErr: true,
 		},
 	} {

diff --git a/consent/handler_test.go b/consent/handler_test.go
@@ -106,7 +106,7 @@ func TestGetLogoutRequest(t *testing.T) {
 			reg := internal.NewRegistryMemory(t, conf)
 
 			if tc.exists {
-				cl := &client.Client{ID: "client" + key}
+				cl := &client.Client{OutfacingID: "client" + key}
 				require.NoError(t, reg.ClientManager().CreateClient(context.Background(), cl))
 				require.NoError(t, reg.ConsentManager().CreateLogoutRequest(context.TODO(), &LogoutRequest{
 					Client:  cl,
@@ -147,7 +147,7 @@ func TestGetLoginRequest(t *testing.T) {
 			reg := internal.NewRegistryMemory(t, conf)
 
 			if tc.exists {
-				cl := &client.Client{ID: "client" + key}
+				cl := &client.Client{OutfacingID: "client" + key}
 				require.NoError(t, reg.ClientManager().CreateClient(context.Background(), cl))
 				require.NoError(t, reg.ConsentManager().CreateLoginRequest(context.Background(), &LoginRequest{
 					Client: cl,
@@ -192,7 +192,7 @@ func TestGetConsentRequest(t *testing.T) {
 			reg := internal.NewRegistryMemory(t, conf)
 
 			if tc.exists {
-				cl := &client.Client{ID: "client" + key}
+				cl := &client.Client{OutfacingID: "client" + key}
 				require.NoError(t, reg.ClientManager().CreateClient(context.Background(), cl))
 				require.NoError(t, reg.ConsentManager().CreateConsentRequest(context.Background(), &ConsentRequest{
 					Client: cl,

diff --git a/consent/manager_test_helpers.go b/consent/manager_test_helpers.go
@@ -51,7 +51,7 @@ func MockConsentRequest(key string, remember bool, rememberFor int, hasError boo
 			UILocales: []string{"fr" + key, "de" + key},
 			Display:   "popup" + key,
 		},
-		Client:                 &client.Client{ID: "fk-client-" + key},
+		Client:                 &client.Client{OutfacingID: "fk-client-" + key},
 		RequestURL:             "https://request-url/path" + key,
 		LoginChallenge:         sqlxx.NullString("fk-login-challenge-" + key),
 		LoginSessionID:         sqlxx.NullString("fk-login-session-" + key),
@@ -103,7 +103,7 @@ func MockLogoutRequest(key string, withClient bool) (c *LogoutRequest) {
 	var cl *client.Client
 	if withClient {
 		cl = &client.Client{
-			ID: "fk-client-" + key,
+			OutfacingID: "fk-client-" + key,
 		}
 	}
 	return &LogoutRequest{
@@ -128,7 +128,7 @@ func MockAuthRequest(key string, authAt bool) (c *LoginRequest, h *HandledLoginR
 			Display:   "popup" + key,
 		},
 		RequestedAt:    time.Now().UTC().Add(-time.Hour),
-		Client:         &client.Client{ID: "fk-client-" + key},
+		Client:         &client.Client{OutfacingID: "fk-client-" + key},
 		Subject:        "subject" + key,
 		RequestURL:     "https://request-url/path" + key,
 		Skip:           true,
@@ -262,7 +262,7 @@ func ManagerTests(m Manager, clientManager client.Manager, fositeManager x.Fosit
 	return func(t *testing.T) {
 		t.Run("case=init-fks", func(t *testing.T) {
 			for _, k := range []string{"1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "rv1", "rv2"} {
-				require.NoError(t, clientManager.CreateClient(context.Background(), &client.Client{ID: fmt.Sprintf("fk-client-%s", k)}))
+				require.NoError(t, clientManager.CreateClient(context.Background(), &client.Client{OutfacingID: fmt.Sprintf("fk-client-%s", k)}))
 
 				require.NoError(t, m.CreateLoginSession(context.Background(), &LoginSession{
 					ID:              fmt.Sprintf("fk-login-session-%s", k),
@@ -273,7 +273,7 @@ func ManagerTests(m Manager, clientManager client.Manager, fositeManager x.Fosit
 				require.NoError(t, m.CreateLoginRequest(context.Background(), &LoginRequest{
 					ID:              fmt.Sprintf("fk-login-challenge-%s", k),
 					Verifier:        fmt.Sprintf("fk-login-verifier-%s", k),
-					Client:          &client.Client{ID: fmt.Sprintf("fk-client-%s", k)},
+					Client:          &client.Client{OutfacingID: fmt.Sprintf("fk-client-%s", k)},
 					AuthenticatedAt: sqlxx.NullTime(time.Now()),
 					RequestedAt:     time.Now(),
 				}))
@@ -646,7 +646,7 @@ func ManagerTests(m Manager, clientManager client.Manager, fositeManager x.Fosit
 						require.NoError(t, err)
 						for _, consent := range consents {
 							assert.Contains(t, tc.challenges, consent.ID)
-							assert.Contains(t, tc.clients, consent.ConsentRequest.Client.ID)
+							assert.Contains(t, tc.clients, consent.ConsentRequest.Client.OutfacingID)
 						}
 					}
 
@@ -710,7 +710,7 @@ func ManagerTests(m Manager, clientManager client.Manager, fositeManager x.Fosit
 						}
 						require.NoError(t, m.CreateLoginSession(context.Background(), ls))
 
-						cl := &client.Client{ID: uuid.New().String()}
+						cl := &client.Client{OutfacingID: uuid.New().String()}
 						switch k % 4 {
 						case 0:
 							cl.FrontChannelLogoutURI = "http://some-url.com/"
@@ -746,10 +746,10 @@ func ManagerTests(m Manager, clientManager client.Manager, fositeManager x.Fosit
 						for _, e := range es {
 							var found bool
 							for _, a := range actual {
-								if e.ID == a.ID {
+								if e.OutfacingID == a.OutfacingID {
 									found = true
 								}
-								assert.Equal(t, e.ID, a.ID)
+								assert.Equal(t, e.OutfacingID, a.OutfacingID)
 								assert.Equal(t, e.FrontChannelLogoutURI, a.FrontChannelLogoutURI)
 								assert.Equal(t, e.BackChannelLogoutURI, a.BackChannelLogoutURI)
 							}

diff --git a/consent/sdk_test.go b/consent/sdk_test.go
@@ -135,7 +135,7 @@ func TestSDK(t *testing.T) {
 	_, err = sdk.Admin.RevokeConsentSessions(admin.NewRevokeConsentSessionsParams().WithSubject("subject1"))
 	require.Error(t, err)
 
-	_, err = sdk.Admin.RevokeConsentSessions(admin.NewRevokeConsentSessionsParams().WithSubject(cr4.Subject).WithClient(&cr4.Client.ID))
+	_, err = sdk.Admin.RevokeConsentSessions(admin.NewRevokeConsentSessionsParams().WithSubject(cr4.Subject).WithClient(&cr4.Client.OutfacingID))
 	require.NoError(t, err)
 
 	_, err = sdk.Admin.RevokeConsentSessions(admin.NewRevokeConsentSessionsParams().WithSubject("subject1").WithAll(pointerx.Bool(true)))

diff --git a/consent/strategy_default.go b/consent/strategy_default.go
@@ -332,7 +332,7 @@ func (s *DefaultStrategy) obfuscateSubjectIdentifier(cl fosite.Client, subject,
 	if c, ok := cl.(*client.Client); ok && c.SubjectType == "pairwise" {
 		algorithm, ok := s.r.SubjectIdentifierAlgorithm()[c.SubjectType]
 		if !ok {
-			return "", errors.WithStack(fosite.ErrInvalidRequest.WithHint(fmt.Sprintf(`Subject Identifier Algorithm "%s" was requested by OAuth 2.0 Client "%s", but is not configured.`, c.SubjectType, c.ID)))
+			return "", errors.WithStack(fosite.ErrInvalidRequest.WithHint(fmt.Sprintf(`Subject Identifier Algorithm "%s" was requested by OAuth 2.0 Client "%s", but is not configured.`, c.SubjectType, c.OutfacingID)))
 		}
 
 		if len(forcedIdentifier) > 0 {
@@ -683,7 +683,7 @@ func (s *DefaultStrategy) executeBackChannelLogout(ctx context.Context, r *http.
 
 		t, _, err := s.r.OpenIDJWTStrategy().Generate(ctx, jwtgo.MapClaims{
 			"iss":    s.c.IssuerURL().String(),
-			"aud":    []string{c.ID},
+			"aud":    []string{c.OutfacingID},
 			"iat":    time.Now().UTC().Unix(),
 			"jti":    uuid.New(),
 			"events": map[string]struct{}{"http://schemas.openid.net/event/backchannel-logout": {}},
@@ -695,7 +695,7 @@ func (s *DefaultStrategy) executeBackChannelLogout(ctx context.Context, r *http.
 			return err
 		}
 
-		tasks = append(tasks, task{url: c.BackChannelLogoutURI, clientID: c.ID, token: t})
+		tasks = append(tasks, task{url: c.BackChannelLogoutURI, clientID: c.OutfacingID, token: t})
 	}
 
 	var wg sync.WaitGroup