feat(dex): add option to modify userid claim, skip email_verified verification

[kengou]

Add option to organization oidc config to change userID claim.
At the moment login_name is hardcoded, and with this change it could be changed to email or any other claim to use as identifier.
Possibility to skip email_verified verification on oidc config. Keycloak turns the email_verified to off(false) by default if using SAML or other user federation.

Description

What type of PR is this? (check all applicable)

• 🍕 Feature
• 🐛 Bug Fix
• 📝 Documentation Update
• 🎨 Style
• 🧑‍💻 Code Refactor
• 🔥 Performance Improvements
• ✅ Test
• 🤖 Build
• 🔁 CI
• 📦 Chore (Release)
• ⏩ Revert

Related Tickets & Documents

Added tests?

• 👍 yes
• 🙅 no, because they aren't needed
• 🙋 no, because I need help
• Separate ticket for tests # (issue/pr)

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Added to documentation?

• 📜 README.md
• 🤝 Documentation pages updated
• 🙅 no documentation needed
• (if applicable) generated OpenAPI docs for CRD changes

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• My changes generate no new warnings
• New and existing unit tests pass locally with my changes

Summary by CodeRabbit

Release Notes

• New Features
  • Added new optional OIDC configuration options:
    • Email verification handling in ID tokens (insecureSkipEmailVerified, default: false)
    • Custom user ID claim specification (userIDClaim, default: login_name)

[github-actions]

This PR is stale because it has been open for 45 days with no activity.

[coderabbitai]

📝 Walkthrough

Walkthrough

The changes extend OIDC configuration capabilities by introducing a new OIDCExtraConfig type with fields for email verification skipping and user ID claim customization. Updates include Go type definitions, generated deep copy methods, Kubernetes CRD schema, controller logic, API documentation, configuration examples, and TypeScript type bindings.

Changes

Cohort / File(s)	Summary
Go Type Definitions api/v1alpha1/organization_types.go, api/v1alpha1/zz_generated.deepcopy.go	Introduces OIDCExtraConfig struct with InsecureSkipEmailVerified and UserIDClaim fields; adds ExtraConfig field to OIDCConfig; generates deepcopy methods for proper object cloning.
Kubernetes CRD Schema charts/manager/crds/greenhouse.sap_organizations.yaml	Adds extraConfig object definition under spec.authentication.oidc with insecureSkipEmailVerified (boolean, default false) and userIDClaim (string, default "login_name") properties.
Controller Implementation internal/controller/organization/dex.go	Populates OIDC configuration with dynamic values from ExtraConfig; adds InsecureSkipEmailVerified field; uses userIDClaim for both UserNameKey and UserIDKey fields when available.
API Documentation docs/reference/api/index.html, docs/reference/api/openapi.yaml	Documents new OIDCExtraConfig type and extraConfig field in OpenAPI and reference schemas; includes field descriptions and default values.
Configuration Example config/samples/organization/demo.yaml	Provides sample extraConfig usage with insecureSkipEmailVerified set to true and userIDClaim set to "email".
TypeScript Type Definitions types/typescript/schema.d.ts	Adds insecureSkipEmailVerified and userIDClaim fields to Organization.Spec.authentication.oidc type definition.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Hops with glee! The OIDC path now bends,
With extra configs, the scheme extends,
Email skips verified, user claims abound,
Our authentication strengthens all around! 🌟

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title accurately summarizes the main feature: adding OIDC configuration options to modify the userID claim and skip email_verified verification.
Description check	✅ Passed	The PR description provides a clear summary of changes and includes the feature type checkbox selection, but lacks related tickets, test confirmation, and documentation updates.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat/dex-orgconfig

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

types/typescript/schema.d.ts (1)

549-571: ⚠️ Potential issue | 🟠 Major

The generated TS contract no longer matches the CRD.

oidc now exposes insecureSkipEmailVerified and userIDClaim as required top-level properties here, but the actual API in charts/manager/crds/greenhouse.sap_organizations.yaml at Line 95 through Line 113 and docs/reference/api/openapi.yaml at Line 696 through Line 710 nests both under oidc.extraConfig and keeps them optional. Consumers using this type will compile code that submits invalid manifests.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@types/typescript/schema.d.ts` around lines 549 - 571, The TypeScript schema
currently declares insecureSkipEmailVerified and userIDClaim as required
top-level properties on oidc, but the CRD/OpenAPI expect these as optional
fields under oidc.extraConfig; update the generated types so
insecureSkipEmailVerified and userIDClaim are defined as optional (use ?:) and
located on the oidc.extraConfig type/interface (or make oidc.extraConfig an
explicit typed object that includes insecureSkipEmailVerified?: boolean and
userIDClaim?: string) so the TS contract matches the CRD/openapi shape
(referencing the symbols insecureSkipEmailVerified, userIDClaim,
oidc.extraConfig, and the oidc type in schema.d.ts).

🧹 Nitpick comments (2)

config/samples/organization/demo.yaml (1)

56-58: Keep the insecure flag out of the default sample.

This is likely the first manifest users will copy. Setting insecureSkipEmailVerified: true here makes the bypass look like the recommended/default setup even though it should only be enabled for specific IdPs. I'd omit it here or move it into a provider-specific sample.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@config/samples/organization/demo.yaml` around lines 56 - 58, The sample
manifest's extraConfig currently enables insecureSkipEmailVerified: true which
should not be in a default/copyable sample; remove the insecureSkipEmailVerified
key from the extraConfig block in the demo sample (or move it into a separate
provider-specific sample) so the top-level demo.yaml only contains safe defaults
(e.g., keep userIDClaim: email) and document that insecureSkipEmailVerified is
only for testing or specific IdP cases.

api/v1alpha1/organization_types.go (1)

97-99: Document that this override changes more than the user ID.

UserIDClaim is later applied to both UserNameKey and UserIDKey in internal/controller/organization/dex.go, at Line 128 and Line 129. Either rename this field before the API settles, or update the doc string so callers know it also changes the username claim.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@api/v1alpha1/organization_types.go` around lines 97 - 99, The doc comment for
UserIDClaim is misleading because the same field is later used to set both
UserNameKey and UserIDKey (see UserIDClaim, UserNameKey, UserIDKey), so update
the API comment on the UserIDClaim field to state that this override controls
both the user ID and the username claim (or alternatively rename the field to
something like UserIDAndNameClaim before the API stabilizes); change the comment
on the UserIDClaim declaration to explicitly mention it is applied to both
UserNameKey and UserIDKey so callers know it also affects the username claim.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/reference/api/index.html`:
- Around line 1994-2005: The docstring for OIDCConfig.ExtraConfig is incorrect:
update the source comment on the OIDCConfig.ExtraConfig field to replace
"ExtraClaims contains additional configuration for extra claims." with text that
documents both extra claims handling and the skip-email-verified behavior (e.g.,
mention it configures additional claim mapping and whether to skip setting
email_verified), then regenerate the API reference so the change appears in
docs/index.html; locate the field comment for OIDCConfig.ExtraConfig (symbol:
OIDCConfig.ExtraConfig) and edit it accordingly before running the doc
generation step.
- Around line 2027-2047: The docs for insecureSkipEmailVerified and userIDClaim
lack their defaults and security impact; update the API reference entries for
insecureSkipEmailVerified and userIDClaim to state that userIDClaim defaults to
"login_name" when unset and to explicitly document that
insecureSkipEmailVerified relaxes ID token validation (it skips checking the
email_verified claim) and therefore should only be enabled for providers known
to misreport email_verified, with a clear warning about the increased risk of
accepting unverified emails.

In `@internal/controller/organization/dex.go`:
- Around line 115-121: The guard around setting userNameKey is inverted: in the
dex handling code that checks org.Spec.Authentication.OIDCConfig.ExtraConfig you
must set userNameKey when ExtraConfig.UserIDClaim is non-empty (UserIDClaim !=
"") instead of when it equals ""; change the condition in that block accordingly
and remove the duplicated ExtraConfig nil-check (or consolidate the nil-check
once) so skipEmailVerified = ExtraConfig.InsecureSkipEmailVerified still
executes when ExtraConfig is present; update the branches around
org.Spec.Authentication.OIDCConfig.ExtraConfig, the UserIDClaim assignment
(userNameKey) and the InsecureSkipEmailVerified assignment to use the correct
non-empty check and a single nil guard.

---

Outside diff comments:
In `@types/typescript/schema.d.ts`:
- Around line 549-571: The TypeScript schema currently declares
insecureSkipEmailVerified and userIDClaim as required top-level properties on
oidc, but the CRD/OpenAPI expect these as optional fields under
oidc.extraConfig; update the generated types so insecureSkipEmailVerified and
userIDClaim are defined as optional (use ?:) and located on the oidc.extraConfig
type/interface (or make oidc.extraConfig an explicit typed object that includes
insecureSkipEmailVerified?: boolean and userIDClaim?: string) so the TS contract
matches the CRD/openapi shape (referencing the symbols
insecureSkipEmailVerified, userIDClaim, oidc.extraConfig, and the oidc type in
schema.d.ts).

---

Nitpick comments:
In `@api/v1alpha1/organization_types.go`:
- Around line 97-99: The doc comment for UserIDClaim is misleading because the
same field is later used to set both UserNameKey and UserIDKey (see UserIDClaim,
UserNameKey, UserIDKey), so update the API comment on the UserIDClaim field to
state that this override controls both the user ID and the username claim (or
alternatively rename the field to something like UserIDAndNameClaim before the
API stabilizes); change the comment on the UserIDClaim declaration to explicitly
mention it is applied to both UserNameKey and UserIDKey so callers know it also
affects the username claim.

In `@config/samples/organization/demo.yaml`:
- Around line 56-58: The sample manifest's extraConfig currently enables
insecureSkipEmailVerified: true which should not be in a default/copyable
sample; remove the insecureSkipEmailVerified key from the extraConfig block in
the demo sample (or move it into a separate provider-specific sample) so the
top-level demo.yaml only contains safe defaults (e.g., keep userIDClaim: email)
and document that insecureSkipEmailVerified is only for testing or specific IdP
cases.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e1af7d7a-4f14-41b9-a22a-af118ad6390f

📥 Commits

Reviewing files that changed from the base of the PR and between 76f0768 and 2d63648.

📒 Files selected for processing (8)

• api/v1alpha1/organization_types.go
• api/v1alpha1/zz_generated.deepcopy.go
• charts/manager/crds/greenhouse.sap_organizations.yaml
• config/samples/organization/demo.yaml
• docs/reference/api/index.html
• docs/reference/api/openapi.yaml
• internal/controller/organization/dex.go
• types/typescript/schema.d.ts