feat(auth): support nested OIDC claim paths - #6294
Conversation
🚨 Preview Deployment Blocked - Security ProtectionYour pull request was blocked from triggering preview deployments Why was this blocked?
How to resolve this:Option 1: Get Collaborator Access (Recommended) Option 2: Request Permission Override For Repository Administrators:To disable this security check ( This security measure protects against malicious code execution in preview deployments. Only trusted collaborators should have the ability to trigger deployments. 🛡️ Learn more about this security featureThis protection prevents unauthorized users from:
Preview deployments are powerful but require trust. Only users with repository write access can trigger them. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughOIDC profile helpers now resolve dotted claim paths for display names and external groups. Sign-in synchronization uses nested group claims, with unit and integration tests covering the behavior. OIDC documentation tables retain related settings and describe dot-separated name-attribute paths. ChangesOIDC profile claim support
Sequence Diagram(s)sequenceDiagram
participant OidcSignInHandler
participant getProfileValueByPath
participant synchronizeGroupsWithExternalForUserAsync
OidcSignInHandler->>getProfileValueByPath: Resolve configured groups claim path
getProfileValueByPath-->>OidcSignInHandler: Return nested profile groups
OidcSignInHandler->>synchronizeGroupsWithExternalForUserAsync: Synchronize array-valued groups
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/auth/providers/oidc/profile.ts (1)
15-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding an explicit return type annotation.
extractProfileNameinfersstring | undefinedbut doesn't declare it. An explicit: string | undefinedreturn type would prevent accidental type drift and improve call-site clarity, since bothoidc-provider.tsandevents.tsdepend on this contract.♻️ Optional refactor
-export const extractProfileName = (profile: Profile) => { +export const extractProfileName = (profile: Profile): string | undefined => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/auth/providers/oidc/profile.ts` around lines 15 - 23, Add an explicit string | undefined return type annotation to extractProfileName, preserving its existing return behavior and contract for oidc-provider.ts and events.ts.packages/auth/providers/oidc/test/profile.spec.ts (1)
39-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for the
extractProfileNamefallback branch.The two tests cover the
AUTH_OIDC_NAME_ATTRIBUTE_OVERWRITEset path, but no test exercises the fallback (unset overwrite →preferred_username/namelogic). Since this behavior was moved fromoidc-provider.ts, adding a test here keeps the helper fully covered in its new location.♻️ Suggested additional tests
test("uses preferred_username when overwrite is not set", () => { const profile = { sub: "user-id", preferred_username: "johndoe" } satisfies Profile; expect(extractProfileName(profile)).toBe("johndoe"); }); test("uses name when preferred_username is an email", () => { const profile = { sub: "user-id", preferred_username: "john@example.com", name: "John Doe" } satisfies Profile; expect(extractProfileName(profile)).toBe("John Doe"); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/auth/providers/oidc/test/profile.spec.ts` around lines 39 - 59, Add tests in the extractProfileName suite for the fallback path when AUTH_OIDC_NAME_ATTRIBUTE_OVERWRITE is unset: verify preferred_username is returned for a non-email value, and name is returned when preferred_username is an email. Use Profile-compatible fixtures and preserve the existing overwrite-path tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/auth/providers/oidc/profile.ts`:
- Around line 15-23: Add an explicit string | undefined return type annotation
to extractProfileName, preserving its existing return behavior and contract for
oidc-provider.ts and events.ts.
In `@packages/auth/providers/oidc/test/profile.spec.ts`:
- Around line 39-59: Add tests in the extractProfileName suite for the fallback
path when AUTH_OIDC_NAME_ATTRIBUTE_OVERWRITE is unset: verify preferred_username
is returned for a non-email value, and name is returned when preferred_username
is an email. Use Profile-compatible fixtures and preserve the existing
overwrite-path tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bc89ffc0-03b5-4714-aed1-7d5f6de09344
📒 Files selected for processing (7)
apps/docs/docs/advanced/single-sign-on/index.mdxapps/docs/docs/getting-started/installation/helm.mdpackages/auth/events.tspackages/auth/providers/oidc/oidc-provider.tspackages/auth/providers/oidc/profile.tspackages/auth/providers/oidc/test/profile.spec.tspackages/auth/test/events.spec.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/auth/providers/oidc/test/profile.spec.ts (1)
40-67: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding a test for missing
preferred_username.When no overwrite is set and
preferred_usernameisundefined,extractProfileNamereturnsundefined, which causes the downstream provider to throw. This is an important edge case worth covering alongside the existing fallback tests.💚 Suggested additional test
+ test("returns undefined when preferred_username is missing and no overwrite is set", () => { + const profile = { + sub: "user-id", + } satisfies Profile; + + expect(extractProfileName(profile)).toBeUndefined(); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/auth/providers/oidc/test/profile.spec.ts` around lines 40 - 67, Add a test alongside the existing extractProfileName fallback tests that omits preferred_username while no overwrite is configured, and assert the expected undefined result from extractProfileName. Use the existing Profile fixture style and preserve the current tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/auth/providers/oidc/test/profile.spec.ts`:
- Around line 40-67: Add a test alongside the existing extractProfileName
fallback tests that omits preferred_username while no overwrite is configured,
and assert the expected undefined result from extractProfileName. Use the
existing Profile fixture style and preserve the current tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5eb594df-557f-4859-9735-6ead578c9c24
📒 Files selected for processing (2)
packages/auth/providers/oidc/profile.tspackages/auth/providers/oidc/test/profile.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/auth/providers/oidc/profile.ts
|
I am not familiar with the OIDC feature at all so I won't be able to test it, if you're telling me it solves issues for you then I will trust it and merge it |
|
Thanks. To be precise, this implements the concrete nested-claim behavior requested in #2657; I have not tested it against a live external IdP, so I do not want to overstate that. The automated coverage exercises the same profile shapes and sign-in path: dot-separated lookup, nested |
2 participants
Summary
AUTH_OIDC_NAME_ATTRIBUTE_OVERWRITEFor example,
AUTH_OIDC_GROUPS_ATTRIBUTE=resource_access.homarr.rolesnow resolves this profile shape:{ "resource_access": { "homarr": { "roles": ["admins"] } } }Fixes #2657.
Verification
NODE_ENV=development CI=true pnpm exec vitest run packages/auth/test/events.spec.ts packages/auth/providers/oidc/test/profile.spec.ts --coverage.enabled=false— 18 tests passedpnpm --filter @homarr/auth typecheck— passedpnpm --filter @homarr/auth lint— passed with 0 errors (6 existing warnings in the package)pnpm exec oxfmt --checkon the changed TypeScript files — passedgit diff --check— passedChecklist
pnpm build, autofix withpnpm format:fix) — full build not run; targeted checks are listed abovedevbranchSummary by CodeRabbit
AUTH_OIDC_*environment variables.