From a7937a81a7baeea47816555028798a55bb73b632 Mon Sep 17 00:00:00 2001 From: SamTV12345 <40429738+samtv12345@users.noreply.github.com> Date: Thu, 9 Jan 2025 18:09:08 +0100 Subject: [PATCH] fix: add feedback from review --- pkg/apis/middleware/session.go | 4 ++-- pkg/apis/options/legacy_options.go | 1 - pkg/apis/options/providers.go | 3 ++- pkg/apis/sessions/session_state.go | 4 ++-- providers/keycloak.go | 1 - providers/keycloak_oidc.go | 1 - providers/oidc.go | 1 + providers/provider_data.go | 2 +- providers/provider_default.go | 3 +-- providers/provider_default_test.go | 2 +- 10 files changed, 10 insertions(+), 12 deletions(-) diff --git a/pkg/apis/middleware/session.go b/pkg/apis/middleware/session.go index 4ef54b41..a7d32014 100644 --- a/pkg/apis/middleware/session.go +++ b/pkg/apis/middleware/session.go @@ -25,7 +25,7 @@ func CreateTokenToSessionFunc(verify VerifyFunc) TokenToSessionFunc { Verified *bool `json:"email_verified"` PreferredUsername string `json:"preferred_username"` Groups []string `json:"groups"` - Acr string `json:"acr"` + ACR string `json:"acr"` } idToken, err := verify(ctx, token) @@ -50,7 +50,7 @@ func CreateTokenToSessionFunc(verify VerifyFunc) TokenToSessionFunc { User: claims.Subject, Groups: claims.Groups, PreferredUsername: claims.PreferredUsername, - Acr: claims.Acr, + ACR: claims.ACR, AccessToken: token, IDToken: token, RefreshToken: "", diff --git a/pkg/apis/options/legacy_options.go b/pkg/apis/options/legacy_options.go index 8f539898..163aaa2c 100644 --- a/pkg/apis/options/legacy_options.go +++ b/pkg/apis/options/legacy_options.go @@ -727,7 +727,6 @@ func (l *LegacyProvider) convert() (Providers, error) { provider.KeycloakConfig = KeycloakOptions{ Groups: l.KeycloakGroups, Roles: l.AllowedRoles, - ACRs: l.AcrValues, } case "keycloak": provider.KeycloakConfig = KeycloakOptions{ diff --git a/pkg/apis/options/providers.go b/pkg/apis/options/providers.go index 11bbc2db..b1c8be2a 100644 --- a/pkg/apis/options/providers.go +++ b/pkg/apis/options/providers.go @@ -149,7 +149,6 @@ type KeycloakOptions struct { // Role enables to restrict login to users with role (only available when using the keycloak-oidc provider) Roles []string `json:"roles,omitempty"` - ACRs string `json:"acr,omitempty"` } type AzureOptions struct { @@ -261,6 +260,8 @@ type OIDCOptions struct { // ExtraAudiences is a list of additional audiences that are allowed // to pass verification in addition to the client id. ExtraAudiences []string `json:"extraAudiences,omitempty"` + // to pass acr values to the provider + ACRs string `json:"acr,omitempty"` } type LoginGovOptions struct { diff --git a/pkg/apis/sessions/session_state.go b/pkg/apis/sessions/session_state.go index 75c24a07..d3b5b0e3 100644 --- a/pkg/apis/sessions/session_state.go +++ b/pkg/apis/sessions/session_state.go @@ -28,7 +28,7 @@ type SessionState struct { User string `msgpack:"u,omitempty"` Groups []string `msgpack:"g,omitempty"` PreferredUsername string `msgpack:"pu,omitempty"` - Acr string `msgpack:"acr,omitempty"` + ACR string `msgpack:"acr,omitempty"` // Internal helpers, not serialized Clock clock.Clock `msgpack:"-"` @@ -150,7 +150,7 @@ func (s *SessionState) GetClaim(claim string) []string { case "preferred_username": return []string{s.PreferredUsername} case "acr": - return []string{s.Acr} + return []string{s.ACR} default: return []string{} } diff --git a/providers/keycloak.go b/providers/keycloak.go index cc2f1bc9..e36068c4 100644 --- a/providers/keycloak.go +++ b/providers/keycloak.go @@ -61,7 +61,6 @@ func NewKeycloakProvider(p *ProviderData, opts options.KeycloakOptions) *Keycloa provider := &KeycloakProvider{ProviderData: p} provider.setAllowedGroups(opts.Groups) - provider.setAllowedACR(opts.ACRs) return provider } diff --git a/providers/keycloak_oidc.go b/providers/keycloak_oidc.go index 353d0748..ab613752 100644 --- a/providers/keycloak_oidc.go +++ b/providers/keycloak_oidc.go @@ -26,7 +26,6 @@ func NewKeycloakOIDCProvider(p *ProviderData, opts options.Provider) *KeycloakOI } provider.addAllowedRoles(opts.KeycloakConfig.Roles) - provider.setAllowedACR(opts.KeycloakConfig.ACRs) return provider } diff --git a/providers/oidc.go b/providers/oidc.go index 43b5227e..103948a3 100644 --- a/providers/oidc.go +++ b/providers/oidc.go @@ -46,6 +46,7 @@ func NewOIDCProvider(p *ProviderData, opts options.OIDCOptions) *OIDCProvider { } p.setProviderDefaults(oidcProviderDefaults) + p.setAllowedACR(opts.ACRs) p.getAuthorizationHeaderFunc = makeOIDCHeader return &OIDCProvider{ diff --git a/providers/provider_data.go b/providers/provider_data.go index 304fbd47..9af504e6 100644 --- a/providers/provider_data.go +++ b/providers/provider_data.go @@ -268,7 +268,7 @@ func (p *ProviderData) buildSessionFromClaims(rawIDToken, accessToken string) (* {p.UserClaim, &ss.User}, {p.EmailClaim, &ss.Email}, {p.GroupsClaim, &ss.Groups}, - {oidcAcrClaim, &ss.Acr}, + {oidcAcrClaim, &ss.ACR}, // TODO (@NickMeves) Deprecate for dynamic claim to session mapping {"preferred_username", &ss.PreferredUsername}, } { diff --git a/providers/provider_default.go b/providers/provider_default.go index 841fbe24..96f47aa4 100644 --- a/providers/provider_default.go +++ b/providers/provider_default.go @@ -117,8 +117,7 @@ func (p *ProviderData) EnrichSession(_ context.Context, _ *sessions.SessionState // This is not used for fine-grained per route authorization rules. func (p *ProviderData) Authorize(_ context.Context, s *sessions.SessionState) (bool, error) { if len(p.AllowedACRs) > 0 { - var _, ok = p.AllowedACRs[s.Acr] - if !ok { + if _, ok := p.AllowedACRs[s.ACR]; !ok { return false, nil } } diff --git a/providers/provider_default_test.go b/providers/provider_default_test.go index 520c51d8..a818604f 100644 --- a/providers/provider_default_test.go +++ b/providers/provider_default_test.go @@ -128,7 +128,7 @@ func TestProviderDataAuthorize(t *testing.T) { session := &sessions.SessionState{ Groups: tc.groups, - Acr: tc.userAcr, + ACR: tc.userAcr, } p := &ProviderData{} p.setAllowedGroups(tc.allowedGroups)