Skip to content

CNTRLPLANE-3010: Add API-side validation to enforce prefixPolicy is not set when username expression is used #2771

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.

Sign up for GitHub

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

Open
ShazaAldawamneh wants to merge 3 commits into
base: master
Choose a base branch
from
Open

CNTRLPLANE-3010: Add API-side validation to enforce prefixPolicy is not set when username expression is used #2771

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9b96486
add API-side validation to enforce prefixPolicy is not set to Prefix …
ShazaAldawamneh Mar 18, 2026
c795e9e
add API-side validation to enforce prefixPolicy is not set to Prefix …
ShazaAldawamneh Mar 18, 2026
ef9ca32
add API-side validation to enforce prefixPolicy is not set to Prefix …
ShazaAldawamneh Mar 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 67 additions & 0 deletions config/v1/tests/authentications.config.openshift.io/ExternalOIDCWithUpstreamParity.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,6 +617,73 @@ tests:
claim: "preferred_username"
groups:
claim: ""
- name: Cannot set prefixPolicy to Prefix when using username expression
initial: |
apiVersion: config.openshift.io/v1
kind: Authentication
spec:
type: OIDC
oidcProviders:
- name: myoidc
issuer:
issuerURL: https://meh.tld
audiences: ['openshift-aud']
claimMappings:
username:
expression: "claims.sub"
prefixPolicy: Prefix
prefix:
prefixString: "myoidc:"
expectedError: "prefixPolicy must not be set to 'Prefix' when expression is set"

- name: Can set prefixPolicy to NoPrefix when using username expression
initial: |
apiVersion: config.openshift.io/v1
kind: Authentication
spec:
type: OIDC
oidcProviders:
- name: myoidc
issuer:
issuerURL: https://meh.tld
audiences: ['openshift-aud']
claimMappings:
username:
expression: "claims.sub"
prefixPolicy: NoPrefix
expected: |
apiVersion: config.openshift.io/v1
kind: Authentication
spec:
type: OIDC
oidcProviders:
- name: myoidc
issuer:
issuerURL: https://meh.tld
audiences: ['openshift-aud']
claimMappings:
username:
expression: "claims.sub"
prefixPolicy: NoPrefix

- name: Cannot set prefix when using groups expression
initial: |
apiVersion: config.openshift.io/v1
kind: Authentication
spec:
type: OIDC
oidcProviders:
- name: myoidc
issuer:
issuerURL: https://meh.tld
audiences: ['openshift-aud']
claimMappings:
username:
claim: "preferred_username"
groups:
expression: "claims.groups"
prefix: "oidc-group:"
expectedError: "prefix must not be set when expression is set"
onUpdate:
- name: Should allow updating other fields if existing username claim mapping is longer than 256 characters
initialCRDPatches:
Expand Down
2 changes: 2 additions & 0 deletions config/v1/types_authentication.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -618,6 +618,7 @@ type OIDCClientReference struct {
// +openshift:validation:FeatureGateAwareXValidation:featureGate=ExternalOIDC,rule="has(self.claim)",message="claim is required"
// +openshift:validation:FeatureGateAwareXValidation:featureGate=ExternalOIDCWithUIDAndExtraClaimMappings,rule="has(self.claim)",message="claim is required"
// +openshift:validation:FeatureGateAwareXValidation:featureGate=ExternalOIDCWithUpstreamParity,rule="has(self.claim) ? !has(self.expression) : has(self.expression)",message="precisely one of claim or expression must be set"
// +openshift:validation:FeatureGateAwareXValidation:featureGate=ExternalOIDCWithUpstreamParity,rule="has(self.expression) && size(self.expression) > 0 ? !has(self.prefixPolicy) || self.prefixPolicy != 'Prefix' : true",message="prefixPolicy must not be set to 'Prefix' when expression is set"
type UsernameClaimMapping struct {
// claim is an optional field that configures the JWT token claim whose value is assigned to the cluster identity field associated with this mapping.
// claim is required when the ExternalOIDCWithUpstreamParity feature gate is not enabled.
Expand Down Expand Up @@ -710,6 +711,7 @@ type UsernamePrefix struct {

// PrefixedClaimMapping configures a claim mapping
// that allows for an optional prefix.
// +openshift:validation:FeatureGateAwareXValidation:featureGate=ExternalOIDCWithUpstreamParity,rule="has(self.expression) && size(self.expression) > 0 ? self.prefix == \"\" : true",message="prefix must not be set when expression is set"
Copy link
Contributor

@ehearne-redhat ehearne-redhat Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this file in full, I think we have run into this type of problem before.

Authentication.config.openshift.io "test-vfppn" is invalid: spec.oidcProviders[0].claimMappings.groups: Invalid value: "object": no such key: prefix evaluating rule: prefix must not be set when expression is set

The issue is likely with prefix being an optional string.

So, we need to change this expression to include size(self.prefix) > 0 to acknowledge that we know it may not exist.

Maybe something like

has(self.expression) && size(self.expression) > 0 ? !size(self.prefix) > 0 : true

will fix the issue. :)

Copy link
Contributor

@JoelSpeed JoelSpeed Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use has not size to check for key existence on the prefix field

type PrefixedClaimMapping struct {
TokenClaimMapping `json:",inline"`

Expand Down
8 changes: 8 additions & 0 deletions ...nerated.crd-manifests/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,9 @@ spec:
type: string
type: object
x-kubernetes-validations:
- message: prefix must not be set when expression is set
rule: 'has(self.expression) && size(self.expression) >
0 ? self.prefix == "" : true'
- message: expression must not be set if claim is specified
and is not an empty string
rule: '(size(self.?claim.orValue("")) > 0) ? !has(self.expression)
Expand Down Expand Up @@ -341,6 +344,11 @@ spec:
- message: precisely one of claim or expression must be
set
rule: 'has(self.claim) ? !has(self.expression) : has(self.expression)'
- message: prefixPolicy must not be set to 'Prefix' when
expression is set
rule: 'has(self.expression) && size(self.expression) >
0 ? !has(self.prefixPolicy) || self.prefixPolicy !=
''Prefix'' : true'
- message: prefix must be set if prefixPolicy is 'Prefix',
but must remain unset otherwise
rule: 'has(self.prefixPolicy) && self.prefixPolicy ==
Expand Down
8 changes: 8 additions & 0 deletions ...ted.crd-manifests/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,9 @@ spec:
type: string
type: object
x-kubernetes-validations:
- message: prefix must not be set when expression is set
rule: 'has(self.expression) && size(self.expression) >
0 ? self.prefix == "" : true'
- message: expression must not be set if claim is specified
and is not an empty string
rule: '(size(self.?claim.orValue("")) > 0) ? !has(self.expression)
Expand Down Expand Up @@ -341,6 +344,11 @@ spec:
- message: precisely one of claim or expression must be
set
rule: 'has(self.claim) ? !has(self.expression) : has(self.expression)'
- message: prefixPolicy must not be set to 'Prefix' when
expression is set
rule: 'has(self.expression) && size(self.expression) >
0 ? !has(self.prefixPolicy) || self.prefixPolicy !=
''Prefix'' : true'
- message: prefix must be set if prefixPolicy is 'Prefix',
but must remain unset otherwise
rule: 'has(self.prefixPolicy) && self.prefixPolicy ==
Expand Down
8 changes: 8 additions & 0 deletions ...ed.crd-manifests/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,9 @@ spec:
type: string
type: object
x-kubernetes-validations:
- message: prefix must not be set when expression is set
rule: 'has(self.expression) && size(self.expression) >
0 ? self.prefix == "" : true'
- message: expression must not be set if claim is specified
and is not an empty string
rule: '(size(self.?claim.orValue("")) > 0) ? !has(self.expression)
Expand Down Expand Up @@ -341,6 +344,11 @@ spec:
- message: precisely one of claim or expression must be
set
rule: 'has(self.claim) ? !has(self.expression) : has(self.expression)'
- message: prefixPolicy must not be set to 'Prefix' when
expression is set
rule: 'has(self.expression) && size(self.expression) >
0 ? !has(self.prefixPolicy) || self.prefixPolicy !=
''Prefix'' : true'
- message: prefix must be set if prefixPolicy is 'Prefix',
but must remain unset otherwise
rule: 'has(self.prefixPolicy) && self.prefixPolicy ==
Expand Down
8 changes: 8 additions & 0 deletions ...ted-crd-manifests/authentications.config.openshift.io/ExternalOIDCWithUpstreamParity.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,9 @@ spec:
type: string
type: object
x-kubernetes-validations:
- message: prefix must not be set when expression is set
rule: 'has(self.expression) && size(self.expression) >
0 ? self.prefix == "" : true'
- message: expression must not be set if claim is specified
and is not an empty string
rule: '(size(self.?claim.orValue("")) > 0) ? !has(self.expression)
Expand Down Expand Up @@ -204,6 +207,11 @@ spec:
- message: precisely one of claim or expression must be
set
rule: 'has(self.claim) ? !has(self.expression) : has(self.expression)'
- message: prefixPolicy must not be set to 'Prefix' when
expression is set
rule: 'has(self.expression) && size(self.expression) >
0 ? !has(self.prefixPolicy) || self.prefixPolicy !=
''Prefix'' : true'
- message: prefix must be set if prefixPolicy is 'Prefix',
but must remain unset otherwise
rule: 'has(self.prefixPolicy) && self.prefixPolicy ==
Expand Down
Loading