From 8383142151eda15903f45acd69de5627bce5030c Mon Sep 17 00:00:00 2001 From: Luis Pedrosa <2365589+lpedrosa@users.noreply.github.com> Date: Wed, 24 Nov 2021 14:36:48 +0000 Subject: [PATCH 1/3] test: add failing secret patch test (#2869) --- client/sdk_test.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/client/sdk_test.go b/client/sdk_test.go index 944ff7880ac..2e5d0232070 100644 --- a/client/sdk_test.go +++ b/client/sdk_test.go @@ -266,4 +266,23 @@ func TestClientSDK(t *testing.T) { _, err = c.Admin.PatchOAuth2Client(admin.NewPatchOAuth2ClientParams().WithID(client.ClientID).WithBody(models.PatchRequest{{Op: &op, Path: &path, Value: value}})) require.Error(t, err) }) + + t.Run("case=patch should not alter secret if not requested", func(t *testing.T) { + op := "replace" + path := "/client_uri" + value := "http://foo.bar" + + client := createTestClient("") + client.ClientID = "patch3_client" + _, err := c.Admin.CreateOAuth2Client(admin.NewCreateOAuth2ClientParams().WithBody(client)) + require.NoError(t, err) + + result1, err := c.Admin.PatchOAuth2Client(admin.NewPatchOAuth2ClientParams().WithID(client.ClientID).WithBody(models.PatchRequest{{Op: &op, Path: &path, Value: value}})) + require.NoError(t, err) + result2, err := c.Admin.PatchOAuth2Client(admin.NewPatchOAuth2ClientParams().WithID(client.ClientID).WithBody(models.PatchRequest{{Op: &op, Path: &path, Value: value}})) + require.NoError(t, err) + + // secret hashes shouldn't change between these PUT calls + require.Equal(t, result1.Payload.ClientSecret, result2.Payload.ClientSecret) + }) } From b59aea10fd8a7de29e32dc0a741e7a8a146980ea Mon Sep 17 00:00:00 2001 From: Luis Pedrosa <2365589+lpedrosa@users.noreply.github.com> Date: Wed, 24 Nov 2021 15:49:16 +0000 Subject: [PATCH 2/3] fix: check if client patch changes the secret before updating (#2869) --- client/handler.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/client/handler.go b/client/handler.go index ac4226762ac..e3166eeac30 100644 --- a/client/handler.go +++ b/client/handler.go @@ -191,11 +191,21 @@ func (h *Handler) Patch(w http.ResponseWriter, r *http.Request, ps httprouter.Pa return } + oldSecret := c.Secret + if err := x.ApplyJSONPatch(patchJSON, c, "/id"); err != nil { h.r.Writer().WriteError(w, r, err) return } + // fix for #2869 + // GetConcreteClient returns a client with the hashed secret, however updateClient expects + // an empty secret if the secret hasn't changed. As such we need to check if the patch has + // updated the secret or not + if oldSecret == c.Secret { + c.Secret = "" + } + if err := h.updateClient(r.Context(), c); err != nil { h.r.Writer().WriteError(w, r, err) return From 766a2454f10d0a748009627150c3b41513d368ec Mon Sep 17 00:00:00 2001 From: Luis Pedrosa <2365589+lpedrosa@users.noreply.github.com> Date: Wed, 24 Nov 2021 16:31:03 +0000 Subject: [PATCH 3/3] chore: run goimports --- client/handler.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/handler.go b/client/handler.go index e3166eeac30..309f0c31195 100644 --- a/client/handler.go +++ b/client/handler.go @@ -199,7 +199,7 @@ func (h *Handler) Patch(w http.ResponseWriter, r *http.Request, ps httprouter.Pa } // fix for #2869 - // GetConcreteClient returns a client with the hashed secret, however updateClient expects + // GetConcreteClient returns a client with the hashed secret, however updateClient expects // an empty secret if the secret hasn't changed. As such we need to check if the patch has // updated the secret or not if oldSecret == c.Secret {