From 2e505f40f90dd704c65f78075e7223211e7e1a7a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20M=C3=B6hlmann?= Date: Thu, 7 Dec 2023 11:43:45 +0200 Subject: [PATCH] fix(oidc): return clients without instance settings (#7036) --- internal/api/oidc/client.go | 6 +++ internal/api/oidc/client_converter.go | 4 +- internal/api/oidc/op.go | 28 +++++----- internal/api/oidc/server.go | 9 ++-- internal/query/embed/oidc_client_by_id.sql | 6 +-- internal/query/oidc_client.go | 3 +- internal/query/oidc_client_test.go | 53 ++++++++++++++++--- internal/query/oidc_settings.go | 8 +-- internal/query/testdata/oidc_client_jwt.json | 6 ++- .../testdata/oidc_client_no_settings.json | 30 +++++++++++ .../query/testdata/oidc_client_public.json | 6 ++- .../query/testdata/oidc_client_secret.json | 6 ++- 12 files changed, 126 insertions(+), 39 deletions(-) create mode 100644 internal/query/testdata/oidc_client_no_settings.json diff --git a/internal/api/oidc/client.go b/internal/api/oidc/client.go index 6514583564..246da37361 100644 --- a/internal/api/oidc/client.go +++ b/internal/api/oidc/client.go @@ -927,6 +927,12 @@ func (s *Server) VerifyClient(ctx context.Context, r *op.Request[op.ClientCreden if client.State != domain.AppStateActive { return nil, oidc.ErrInvalidClient().WithDescription("client is not active") } + if client.Settings == nil { + client.Settings = &query.OIDCSettings{ + AccessTokenLifetime: s.defaultAccessTokenLifetime, + IdTokenLifetime: s.defaultIdTokenLifetime, + } + } switch client.AuthMethodType { case domain.OIDCAuthMethodTypeBasic, domain.OIDCAuthMethodTypePost: diff --git a/internal/api/oidc/client_converter.go b/internal/api/oidc/client_converter.go index 1a6c5ed7c1..e6085926ae 100644 --- a/internal/api/oidc/client_converter.go +++ b/internal/api/oidc/client_converter.go @@ -92,11 +92,11 @@ func (c *Client) RestrictAdditionalAccessTokenScopes() func(scopes []string) []s } func (c *Client) AccessTokenLifetime() time.Duration { - return c.client.AccessTokenLifetime + return c.client.Settings.AccessTokenLifetime } func (c *Client) IDTokenLifetime() time.Duration { - return c.client.IDTokenLifetime + return c.client.Settings.IdTokenLifetime } func (c *Client) AccessTokenType() op.AccessTokenType { diff --git a/internal/api/oidc/op.go b/internal/api/oidc/op.go index 1f34f62fb0..f4020d3d87 100644 --- a/internal/api/oidc/op.go +++ b/internal/api/oidc/op.go @@ -122,19 +122,21 @@ func NewServer( } server := &Server{ - LegacyServer: op.NewLegacyServer(provider, endpoints(config.CustomEndpoints)), - features: config.Features, - repo: repo, - query: query, - command: command, - keySet: newKeySet(context.TODO(), time.Hour, query.GetActivePublicKeyByID), - defaultLoginURL: fmt.Sprintf("%s%s?%s=", login.HandlerPrefix, login.EndpointLogin, login.QueryAuthRequestID), - defaultLoginURLV2: config.DefaultLoginURLV2, - defaultLogoutURLV2: config.DefaultLogoutURLV2, - fallbackLogger: fallbackLogger, - hashAlg: crypto.NewBCrypt(10), // as we are only verifying in oidc, the cost is already part of the hash string and the config here is irrelevant. - signingKeyAlgorithm: config.SigningKeyAlgorithm, - assetAPIPrefix: assets.AssetAPI(externalSecure), + LegacyServer: op.NewLegacyServer(provider, endpoints(config.CustomEndpoints)), + features: config.Features, + repo: repo, + query: query, + command: command, + keySet: newKeySet(context.TODO(), time.Hour, query.GetActivePublicKeyByID), + defaultLoginURL: fmt.Sprintf("%s%s?%s=", login.HandlerPrefix, login.EndpointLogin, login.QueryAuthRequestID), + defaultLoginURLV2: config.DefaultLoginURLV2, + defaultLogoutURLV2: config.DefaultLogoutURLV2, + defaultAccessTokenLifetime: config.DefaultAccessTokenLifetime, + defaultIdTokenLifetime: config.DefaultIdTokenLifetime, + fallbackLogger: fallbackLogger, + hashAlg: crypto.NewBCrypt(10), // as we are only verifying in oidc, the cost is already part of the hash string and the config here is irrelevant. + signingKeyAlgorithm: config.SigningKeyAlgorithm, + assetAPIPrefix: assets.AssetAPI(externalSecure), } metricTypes := []metrics.MetricType{metrics.MetricTypeRequestCount, metrics.MetricTypeStatusCode, metrics.MetricTypeTotalCount} server.Handler = op.RegisterLegacyServer(server, op.WithHTTPMiddleware( diff --git a/internal/api/oidc/server.go b/internal/api/oidc/server.go index 1782966e9c..8ba186dd7b 100644 --- a/internal/api/oidc/server.go +++ b/internal/api/oidc/server.go @@ -3,6 +3,7 @@ package oidc import ( "context" "net/http" + "time" "github.com/zitadel/logging" "github.com/zitadel/oidc/v3/pkg/oidc" @@ -27,9 +28,11 @@ type Server struct { command *command.Commands keySet *keySetCache - defaultLoginURL string - defaultLoginURLV2 string - defaultLogoutURLV2 string + defaultLoginURL string + defaultLoginURLV2 string + defaultLogoutURLV2 string + defaultAccessTokenLifetime time.Duration + defaultIdTokenLifetime time.Duration fallbackLogger *slog.Logger hashAlg crypto.HashAlgorithm diff --git a/internal/query/embed/oidc_client_by_id.sql b/internal/query/embed/oidc_client_by_id.sql index 64986a42da..c2fa73c3e2 100644 --- a/internal/query/embed/oidc_client_by_id.sql +++ b/internal/query/embed/oidc_client_by_id.sql @@ -29,18 +29,18 @@ keys as ( group by identifier ), settings as ( - select instance_id, access_token_lifetime, id_token_lifetime + select instance_id, json_build_object('access_token_lifetime', access_token_lifetime, 'id_token_lifetime', id_token_lifetime) as settings from projections.oidc_settings2 where aggregate_id = $1 and instance_id = $1 ) select row_to_json(r) as client from ( - select c.*, r.project_role_keys, k.public_keys, s.access_token_lifetime, s.id_token_lifetime + select c.*, r.project_role_keys, k.public_keys, s.settings from client c left join roles r on r.project_id = c.project_id left join keys k on k.client_id = c.client_id - join settings s on s.instance_id = s.instance_id + left join settings s on s.instance_id = s.instance_id ) r; --execute q('230690539048009730', '236647088211951618@tests', true); \ No newline at end of file diff --git a/internal/query/oidc_client.go b/internal/query/oidc_client.go index 60ecf25174..ac685fab5d 100644 --- a/internal/query/oidc_client.go +++ b/internal/query/oidc_client.go @@ -37,8 +37,7 @@ type OIDCClient struct { PublicKeys map[string][]byte `json:"public_keys,omitempty"` ProjectID string `json:"project_id,omitempty"` ProjectRoleKeys []string `json:"project_role_keys,omitempty"` - AccessTokenLifetime time.Duration `json:"access_token_lifetime,omitempty"` - IDTokenLifetime time.Duration `json:"id_token_lifetime,omitempty"` + Settings *OIDCSettings `json:"settings,omitempty"` } //go:embed embed/oidc_client_by_id.sql diff --git a/internal/query/oidc_client_test.go b/internal/query/oidc_client_test.go index 0593e1ce03..d32e9208b4 100644 --- a/internal/query/oidc_client_test.go +++ b/internal/query/oidc_client_test.go @@ -24,6 +24,8 @@ var ( testdataOidcClientPublic string //go:embed testdata/oidc_client_secret.json testdataOidcClientSecret string + //go:embed testdata/oidc_client_no_settings.json + testdataOidcClientNoSettings string ) func TestQueries_GetOIDCClientByID(t *testing.T) { @@ -81,8 +83,10 @@ low2kyJov38V4Uk2I8kuXpLcnrpw5Tio2ooiUE27b0vHZqBKOei9Uo88qCrn3EKx ProjectID: "236645808328409090", PublicKeys: map[string][]byte{"236647201860747266": []byte(pubkey)}, ProjectRoleKeys: []string{"role1", "role2"}, - AccessTokenLifetime: 43200000000000, - IDTokenLifetime: 43200000000000, + Settings: &OIDCSettings{ + AccessTokenLifetime: 43200000000000, + IdTokenLifetime: 43200000000000, + }, }, }, { @@ -110,8 +114,10 @@ low2kyJov38V4Uk2I8kuXpLcnrpw5Tio2ooiUE27b0vHZqBKOei9Uo88qCrn3EKx PublicKeys: nil, ProjectID: "236645808328409090", ProjectRoleKeys: []string{"role1", "role2"}, - AccessTokenLifetime: 43200000000000, - IDTokenLifetime: 43200000000000, + Settings: &OIDCSettings{ + AccessTokenLifetime: 43200000000000, + IdTokenLifetime: 43200000000000, + }, }, }, { @@ -143,8 +149,43 @@ low2kyJov38V4Uk2I8kuXpLcnrpw5Tio2ooiUE27b0vHZqBKOei9Uo88qCrn3EKx PublicKeys: nil, ProjectID: "236645808328409090", ProjectRoleKeys: []string{"role1", "role2"}, - AccessTokenLifetime: 43200000000000, - IDTokenLifetime: 43200000000000, + Settings: &OIDCSettings{ + AccessTokenLifetime: 43200000000000, + IdTokenLifetime: 43200000000000, + }, + }, + }, + { + name: "no oidc settings", + mock: mockQuery(expQuery, cols, []driver.Value{testdataOidcClientNoSettings}, "instanceID", "clientID", true), + want: &OIDCClient{ + InstanceID: "239520764275982338", + AppID: "239520764276441090", + State: domain.AppStateActive, + ClientID: "239520764779364354@zitadel", + ClientSecret: nil, + RedirectURIs: []string{ + "http://test2-qucuh5.localhost:9000/ui/console/auth/callback", + "http://test.localhost.com:9000/ui/console/auth/callback"}, + ResponseTypes: []domain.OIDCResponseType{0}, + GrantTypes: []domain.OIDCGrantType{0}, + ApplicationType: domain.OIDCApplicationTypeUserAgent, + AuthMethodType: domain.OIDCAuthMethodTypeNone, + PostLogoutRedirectURIs: []string{ + "http://test2-qucuh5.localhost:9000/ui/console/signedout", + "http://test.localhost.com:9000/ui/console/signedout", + }, + IsDevMode: true, + AccessTokenType: domain.OIDCTokenTypeBearer, + AccessTokenRoleAssertion: false, + IDTokenRoleAssertion: false, + IDTokenUserinfoAssertion: false, + ClockSkew: 0, + AdditionalOrigins: nil, + PublicKeys: nil, + ProjectID: "239520764276178946", + ProjectRoleKeys: nil, + Settings: nil, }, }, } diff --git a/internal/query/oidc_settings.go b/internal/query/oidc_settings.go index 7e48ff43a7..a5d9052d4c 100644 --- a/internal/query/oidc_settings.go +++ b/internal/query/oidc_settings.go @@ -69,10 +69,10 @@ type OIDCSettings struct { ResourceOwner string Sequence uint64 - AccessTokenLifetime time.Duration - IdTokenLifetime time.Duration - RefreshTokenIdleExpiration time.Duration - RefreshTokenExpiration time.Duration + AccessTokenLifetime time.Duration `json:"access_token_lifetime,omitempty"` + IdTokenLifetime time.Duration `json:"id_token_lifetime,omitempty"` + RefreshTokenIdleExpiration time.Duration `json:"refresh_token_idle_expiration,omitempty"` + RefreshTokenExpiration time.Duration `json:"refresh_token_expiration,omitempty"` } func (q *Queries) OIDCSettingsByAggID(ctx context.Context, aggregateID string) (settings *OIDCSettings, err error) { diff --git a/internal/query/testdata/oidc_client_jwt.json b/internal/query/testdata/oidc_client_jwt.json index d32e5a5110..df871815dd 100644 --- a/internal/query/testdata/oidc_client_jwt.json +++ b/internal/query/testdata/oidc_client_jwt.json @@ -22,6 +22,8 @@ "public_keys": { "236647201860747266": "LS0tLS1CRUdJTiBSU0EgUFVCTElDIEtFWS0tLS0tCk1JSUJJakFOQmdrcWhraUc5dzBCQVFFRkFB\nT0NBUThBTUlJQkNnS0NBUUVBMnVmQUwxYjcyYkl5MWFyK1dzNmIKR29oSkpRRkI3ZGZSYXBEcWVx\nTThVa3A2Q1ZkUHpxL3BPejF2aUFxNTB5eldaSnJ5Risyd3NoRkFLR0Y5QTIvQgoyWWY5YkpYUFov\nS2JrRnJZVDNOVHZZRGt2bGFTVGw5bU1uenJVMjlzNDhGMVBUV0tmQitDM2FNc09FRzFCdWZWCnM2\nM3FGNG5yRVBqU2JobGpJY285RlpxNFhwcEl6aE1RMGZEZEEvK1h5Z0NKcXZ1YUwwTGliTTFLcmxV\nZG51NzEKWWVraFNKakVQbnZPaXNYSWs0SVh5d29HSU93dGp4a0R2Tkl0UXZhTVZsZHI0L2tiNnV2\nYmdkV3dxNUV3QlpYcQpsb3cya3lKb3YzOFY0VWsySThrdVhwTGNucnB3NVRpbzJvb2lVRTI3YjB2\nSFpxQktPZWk5VW84OHFDcm4zRUt4CjZRSURBUUFCCi0tLS0tRU5EIFJTQSBQVUJMSUMgS0VZLS0t\nLS0K" }, - "access_token_lifetime": 43200000000000, - "id_token_lifetime": 43200000000000 + "settings": { + "access_token_lifetime": 43200000000000, + "id_token_lifetime": 43200000000000 + } } diff --git a/internal/query/testdata/oidc_client_no_settings.json b/internal/query/testdata/oidc_client_no_settings.json new file mode 100644 index 0000000000..83d810d669 --- /dev/null +++ b/internal/query/testdata/oidc_client_no_settings.json @@ -0,0 +1,30 @@ +{ + "instance_id": "239520764275982338", + "app_id": "239520764276441090", + "client_id": "239520764779364354@zitadel", + "client_secret": null, + "redirect_uris": [ + "http://test2-qucuh5.localhost:9000/ui/console/auth/callback", + "http://test.localhost.com:9000/ui/console/auth/callback" + ], + "response_types": [0], + "grant_types": [0], + "application_type": 1, + "auth_method_type": 2, + "post_logout_redirect_uris": [ + "http://test2-qucuh5.localhost:9000/ui/console/signedout", + "http://test.localhost.com:9000/ui/console/signedout" + ], + "is_dev_mode": true, + "access_token_type": 0, + "access_token_role_assertion": false, + "id_token_role_assertion": false, + "id_token_userinfo_assertion": false, + "clock_skew": 0, + "additional_origins": null, + "project_id": "239520764276178946", + "state": 1, + "project_role_keys": null, + "public_keys": null, + "settings": null +} diff --git a/internal/query/testdata/oidc_client_public.json b/internal/query/testdata/oidc_client_public.json index ba23c95351..47cf750c8b 100644 --- a/internal/query/testdata/oidc_client_public.json +++ b/internal/query/testdata/oidc_client_public.json @@ -20,6 +20,8 @@ "state": 1, "project_role_keys": ["role1", "role2"], "public_keys": null, - "access_token_lifetime": 43200000000000, - "id_token_lifetime": 43200000000000 + "settings": { + "access_token_lifetime": 43200000000000, + "id_token_lifetime": 43200000000000 + } } diff --git a/internal/query/testdata/oidc_client_secret.json b/internal/query/testdata/oidc_client_secret.json index 43b3256bb6..e7d5926f7f 100644 --- a/internal/query/testdata/oidc_client_secret.json +++ b/internal/query/testdata/oidc_client_secret.json @@ -25,6 +25,8 @@ "state": 1, "project_role_keys": ["role1", "role2"], "public_keys": null, - "access_token_lifetime": 43200000000000, - "id_token_lifetime": 43200000000000 + "settings": { + "access_token_lifetime": 43200000000000, + "id_token_lifetime": 43200000000000 + } }