Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: re-enable legacy client IDs #3628

Merged
merged 13 commits into from
Sep 19, 2023
4 changes: 0 additions & 4 deletions client/.snapshots/TestClientSDK-case=id_can_not_be_set.json

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -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
}
Original file line number Diff line number Diff line change
@@ -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
}

This file was deleted.

30 changes: 9 additions & 21 deletions client/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -173,12 +166,7 @@ func (h *Handler) CreateClient(r *http.Request, validator func(context.Context,
}
}

if len(c.LegacyClientID) > 0 {
zepatrik marked this conversation as resolved.
Show resolved Hide resolved
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)
Expand Down
10 changes: 5 additions & 5 deletions client/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
},
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",
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",
Expand Down
34 changes: 15 additions & 19 deletions client/sdk_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ package client_test

import (
"context"
"encoding/json"
"io"
"net/http/httptest"
"strings"
"testing"
Expand All @@ -15,8 +13,6 @@ import (

"github.com/ory/x/ioutilx"

"github.com/ory/x/snapshotx"

"github.com/ory/x/uuidx"

"github.com/mohae/deepcopy"
Expand Down Expand Up @@ -112,7 +108,7 @@ func TestClientSDK(t *testing.T) {
assert.EqualValues(t, "bar", result.Metadata.(map[string]interface{})["foo"])

// secret is not returned on GetOAuth2Client
compareClient.ClientSecret = x.ToPointer("")
compareClient.ClientSecret = pointerx.Ptr("")
gresult, _, err := c.OAuth2Api.GetOAuth2Client(context.Background(), *createClient.ClientId).Execute()
require.NoError(t, err)
assertx.EqualAsJSONExcept(t, compareClient, gresult, append(defaultIgnoreFields, "client_secret"))
Expand Down Expand Up @@ -145,7 +141,7 @@ func TestClientSDK(t *testing.T) {

// again, test if secret is not returned on Get
compareClient = updateClient
compareClient.ClientSecret = x.ToPointer("")
compareClient.ClientSecret = pointerx.Ptr("")
gresult, _, err = c.OAuth2Api.GetOAuth2Client(context.Background(), *updateClient.ClientId).Execute()
require.NoError(t, err)
assertx.EqualAsJSONExcept(t, compareClient, gresult, append(defaultIgnoreFields, "client_secret"))
Expand All @@ -160,40 +156,40 @@ func TestClientSDK(t *testing.T) {

t.Run("case=public client is transmitted without secret", func(t *testing.T) {
result, _, err := c.OAuth2Api.CreateOAuth2Client(context.Background()).OAuth2Client(hydra.OAuth2Client{
TokenEndpointAuthMethod: x.ToPointer("none"),
TokenEndpointAuthMethod: pointerx.Ptr("none"),
}).Execute()
require.NoError(t, err)

assert.Equal(t, "", x.FromPointer[string](result.ClientSecret))
assert.Equal(t, "", pointerx.Deref(result.ClientSecret))

result, _, err = c.OAuth2Api.CreateOAuth2Client(context.Background()).OAuth2Client(createTestClient("")).Execute()
require.NoError(t, err)

assert.Equal(t, "secret", x.FromPointer[string](result.ClientSecret))
assert.Equal(t, "secret", pointerx.Deref(result.ClientSecret))
})

t.Run("case=id can not be set", func(t *testing.T) {
_, res, err := c.OAuth2Api.CreateOAuth2Client(context.Background()).OAuth2Client(hydra.OAuth2Client{ClientId: x.ToPointer(uuidx.NewV4().String())}).Execute()
require.Error(t, err)
body, err := io.ReadAll(res.Body)
t.Run("case=id can be set", func(t *testing.T) {
id := uuidx.NewV4().String()
result, _, err := c.OAuth2Api.CreateOAuth2Client(context.Background()).OAuth2Client(hydra.OAuth2Client{ClientId: pointerx.Ptr(id)}).Execute()
require.NoError(t, err)
snapshotx.SnapshotT(t, json.RawMessage(body))

assert.Equal(t, id, pointerx.Deref(result.ClientId))
})

t.Run("case=patch client legally", func(t *testing.T) {
op := "add"
path := "/redirect_uris/-"
value := "http://foo.bar"

client := createTestClient("")
created, _, err := c.OAuth2Api.CreateOAuth2Client(context.Background()).OAuth2Client(client).Execute()
cl := createTestClient("")
created, _, err := c.OAuth2Api.CreateOAuth2Client(context.Background()).OAuth2Client(cl).Execute()
require.NoError(t, err)
client.ClientId = created.ClientId
cl.ClientId = created.ClientId

expected := deepcopy.Copy(client).(hydra.OAuth2Client)
expected := deepcopy.Copy(cl).(hydra.OAuth2Client)
expected.RedirectUris = append(expected.RedirectUris, value)

result, _, err := c.OAuth2Api.PatchOAuth2Client(context.Background(), *client.ClientId).JsonPatch([]hydra.JsonPatch{{Op: op, Path: path, Value: value}}).Execute()
result, _, err := c.OAuth2Api.PatchOAuth2Client(context.Background(), *cl.ClientId).JsonPatch([]hydra.JsonPatch{{Op: op, Path: path, Value: value}}).Execute()
require.NoError(t, err)
expected.CreatedAt = result.CreatedAt
expected.UpdatedAt = result.UpdatedAt
Expand Down
17 changes: 0 additions & 17 deletions x/pointer.go

This file was deleted.

Loading