Repository navigation
feat: add OIDC workspace grant mapping - #2395
Merged
Merged
Conversation
yohamta0
marked this pull request as ready for review
July 20, 2026 05:23
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughOIDC role mappings now support workspace-scoped grants and default access policies. Login provisioning synchronizes roles and workspace access, password operations are restricted for OIDC users, setup gating is added, APIs report synchronization state, and the frontend displays provider-controlled authorization fields. ChangesOIDC workspace authorization
Managed user interface
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant OIDCProvider
participant RoleMapper
participant OIDCProvisioning
participant UserStore
participant UsersAPI
participant UsersPage
OIDCProvider->>RoleMapper: send role and group claims
RoleMapper->>OIDCProvisioning: return role and workspace access
OIDCProvisioning->>UserStore: synchronize user authorization
UsersPage->>UsersAPI: request users
UsersAPI-->>UsersPage: users and sync-enabled flag
UsersPage->>UsersPage: render managed OIDC state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 (3)
internal/cmn/config/loader.go (1)
1634-1634: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant env binding for
workspace_mappings.
auth.oidc.role_mapping.workspace_mappingsis also populated byloadOIDCWorkspaceMappingsEnv()vial.v.Set(...), which takes precedence over this binding, so line 1634 is effectively dead. It's harmless today, but if someone later removes the custom parser assuming this binding covers it, JSON-map parsing would silently break (the raw string wouldn't unmarshal intomap[string][]OIDCWorkspaceGrant). Consider dropping this binding or adding a comment cross-referencing the custom parser.🤖 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 `@internal/cmn/config/loader.go` at line 1634, Remove the redundant auth.oidc.role_mapping.workspace_mappings entry from the environment binding list, since loadOIDCWorkspaceMappingsEnv already populates this setting through l.v.Set. Keep the custom parser as the sole handling path for AUTH_OIDC_WORKSPACE_MAPPINGS.ui/src/pages/users/UserFormModal.tsx (2)
74-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMagic string
'oidc'used instead of the generatedUserAuthProviderenum. Both sites compareuser.authProvideragainst the literal'oidc'rather than the enum exported from@/api/v1/schema, losing compile-time safety if the schema value ever changes.
ui/src/pages/users/UserFormModal.tsx#L74-L85: replaceuser.authProvider === 'oidc'withuser.authProvider === UserAuthProvider.oidc(import the enum from@/api/v1/schema).ui/src/pages/users/index.tsx#L42-L56: replaceuser.authProvider !== 'oidc'withuser.authProvider !== UserAuthProvider.oidc; note the localUserAuthProviderfunction component must be renamed first to avoid a naming collision with the imported enum.🤖 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 `@ui/src/pages/users/UserFormModal.tsx` around lines 74 - 85, The auth-provider comparisons use a magic string instead of the generated enum. In ui/src/pages/users/UserFormModal.tsx:74-85, import UserAuthProvider from `@/api/v1/schema` and compare against UserAuthProvider.oidc; in ui/src/pages/users/index.tsx:42-56, rename the local UserAuthProvider component to avoid the collision, import the generated enum, and use UserAuthProvider.oidc for the comparison.
135-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated role-resolution ternary.
effectiveRole = workspaceAccess.all ? role : UserRole.viewer(line 138) duplicates the identical expression driving the RoleSelect'svalueat line 233. If either expression is edited independently in the future, the displayed role and the submitted role could silently diverge.♻️ Proposed fix: single source of truth
+ const effectiveRole = workspaceAccess.all ? role : UserRole.viewer; + const handleSubmit = async (e: React.FormEvent) => { ... - const effectiveRole = workspaceAccess.all ? role : UserRole.viewer; const payload = managedBySSO<Select - value={workspaceAccess.all ? role : UserRole.viewer} + value={effectiveRole}🤖 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 `@ui/src/pages/users/UserFormModal.tsx` around lines 135 - 158, Define the resolved role expression once in the UserFormModal component and reuse that value for both the Role Select's value and the request payload's effectiveRole, replacing the duplicate workspaceAccess.all ternary while preserving the existing viewer fallback.
🤖 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 `@internal/cmn/config/loader.go`:
- Line 1634: Remove the redundant auth.oidc.role_mapping.workspace_mappings
entry from the environment binding list, since loadOIDCWorkspaceMappingsEnv
already populates this setting through l.v.Set. Keep the custom parser as the
sole handling path for AUTH_OIDC_WORKSPACE_MAPPINGS.
In `@ui/src/pages/users/UserFormModal.tsx`:
- Around line 74-85: The auth-provider comparisons use a magic string instead of
the generated enum. In ui/src/pages/users/UserFormModal.tsx:74-85, import
UserAuthProvider from `@/api/v1/schema` and compare against UserAuthProvider.oidc;
in ui/src/pages/users/index.tsx:42-56, rename the local UserAuthProvider
component to avoid the collision, import the generated enum, and use
UserAuthProvider.oidc for the comparison.
- Around line 135-158: Define the resolved role expression once in the
UserFormModal component and reuse that value for both the Role Select's value
and the request payload's effectiveRole, replacing the duplicate
workspaceAccess.all ternary while preserving the existing viewer fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 11aad743-6071-4ce1-9d73-2cde13bfab10
📒 Files selected for processing (25)
api/v1/api.gen.goapi/v1/api.yamlinternal/cmn/config/config.gointernal/cmn/config/config_test.gointernal/cmn/config/definition.gointernal/cmn/config/key_hints.gointernal/cmn/config/loader.gointernal/cmn/config/loader_test.gointernal/cmn/schema/config.schema.jsoninternal/cmn/schema/config_schema_test.gointernal/service/auth/service.gointernal/service/auth/service_test.gointernal/service/frontend/api/v1/users.gointernal/service/frontend/api/v1/users_internal_test.gointernal/service/frontend/server.gointernal/service/oidcprovision/rolemapper.gointernal/service/oidcprovision/rolemapper_test.gointernal/service/oidcprovision/service.gointernal/service/oidcprovision/service_test.goui/src/api/v1/schema.tsui/src/components/WorkspaceAccessEditor.tsxui/src/components/__tests__/WorkspaceAccessEditor.test.tsxui/src/pages/users/UserFormModal.tsxui/src/pages/users/__tests__/index.test.tsxui/src/pages/users/index.tsx
Member
Author
|
Addressed the valid review feedback in
Validation after the changes: config tests, all 429 UI tests, TypeScript typecheck, Prettier, ESLint, and full Go lint (0 issues) pass. The generic docstring-coverage warning was not applied: this repository requires comments to describe caller-facing API contracts, and adding low-value comments to satisfy a percentage would conflict with that standard. |
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Member
Author
|
@copilot full review |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/cmn/schema/config.schema.json (1)
421-436: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMirror the reserved workspace-name check in the schema.
workspacecurrently accepts names likeall,default, andglobal, butworkspace.ValidateNamerejects them at runtime. Add the reserved-name exclusion here so schema-based tooling matchescfg.Validate().🤖 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 `@internal/cmn/schema/config.schema.json` around lines 421 - 436, The OIDCWorkspaceGrantDef workspace schema pattern does not exclude reserved names rejected by workspace.ValidateName. Update the workspace property’s validation pattern to reject all, default, and global while preserving the existing allowed-character constraints.
🤖 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.
Inline comments:
In `@ui/src/components/WorkspaceAccessEditor.tsx`:
- Around line 59-97: Update the styling in the WorkspaceAccessEditor render
paths: replace information-block backgrounds with bg-slate-200
dark:bg-slate-700, use text-slate-500 dark:text-slate-500 for muted labels and
availability text, and text-slate-800 dark:text-slate-200 for primary workspace
names. In the grants list, replace truncate with whitespace-normal break-words
so long workspace names wrap instead of being clipped.
---
Nitpick comments:
In `@internal/cmn/schema/config.schema.json`:
- Around line 421-436: The OIDCWorkspaceGrantDef workspace schema pattern does
not exclude reserved names rejected by workspace.ValidateName. Update the
workspace property’s validation pattern to reject all, default, and global while
preserving the existing allowed-character constraints.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7274809-966f-428e-b54d-1ac517e5650d
📒 Files selected for processing (32)
api/v1/api.gen.goapi/v1/api.yamlinternal/cmn/config/config.gointernal/cmn/config/config_test.gointernal/cmn/config/definition.gointernal/cmn/config/key_hints.gointernal/cmn/config/loader.gointernal/cmn/config/loader_test.gointernal/cmn/schema/config.schema.jsoninternal/cmn/schema/config_schema_test.gointernal/service/auth/service.gointernal/service/auth/service_test.gointernal/service/frontend/api/v1/auth.gointernal/service/frontend/api/v1/auth_internal_test.gointernal/service/frontend/api/v1/users.gointernal/service/frontend/api/v1/users_internal_test.gointernal/service/frontend/auth/oidc.gointernal/service/frontend/auth/oidc_test.gointernal/service/frontend/server.gointernal/service/frontend/server_test.gointernal/service/oidcprovision/rolemapper.gointernal/service/oidcprovision/rolemapper_test.gointernal/service/oidcprovision/service.gointernal/service/oidcprovision/service_test.goui/src/api/v1/schema.tsui/src/components/UserMenu.tsxui/src/components/WorkspaceAccessEditor.tsxui/src/components/__tests__/UserMenu.test.tsxui/src/components/__tests__/WorkspaceAccessEditor.test.tsxui/src/pages/users/UserFormModal.tsxui/src/pages/users/__tests__/index.test.tsxui/src/pages/users/index.tsx
Comment on lines
+59
to
+97
| <div className="rounded-md border border-border bg-muted/30 px-3 py-2 text-sm"> | ||
| All workspaces | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| if (access.grants.length === 0) { | ||
| return ( | ||
| <div className="rounded-md border border-border bg-muted/30 px-3 py-2 text-sm text-muted-foreground"> | ||
| No named workspace grants | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| const knownWorkspaces = new Set( | ||
| workspaces?.map((workspace) => workspace.name) | ||
| ); | ||
|
|
||
| return ( | ||
| <div className="max-h-56 overflow-auto rounded-md border border-border"> | ||
| {access.grants.map((grant) => { | ||
| const unavailable = | ||
| workspaces !== undefined && !knownWorkspaces.has(grant.workspace); | ||
| return ( | ||
| <div | ||
| key={grant.workspace} | ||
| className="flex items-center justify-between gap-3 border-b border-border px-3 py-2 text-sm last:border-b-0" | ||
| > | ||
| <div className="min-w-0"> | ||
| <div className="truncate">{grant.workspace}</div> | ||
| {unavailable && ( | ||
| <div className="text-xs text-muted-foreground"> | ||
| Workspace not currently available | ||
| </div> | ||
| )} | ||
| </div> | ||
| <span className="rounded bg-muted px-1.5 py-0.5 text-xs capitalize text-muted-foreground"> | ||
| {grant.role} | ||
| </span> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Update styling to comply with UI coding guidelines.
The current implementation uses bg-muted/30, text-muted-foreground, and truncate, which contradicts the explicit coding guidelines for the UI:
- Backgrounds: Use
bg-slate-200 dark:bg-slate-700for similar information blocks. - Text Hierarchy: Use
text-slate-500 dark:text-slate-500for muted text, andtext-slate-800 dark:text-slate-200for primary text. - Long Text: Always handle long text in lists with
whitespace-normal break-wordsinstead of clipping it withtruncate.
🎨 Proposed fixes for styling and text wrapping
- <div className="rounded-md border border-border bg-muted/30 px-3 py-2 text-sm">
+ <div className="rounded-md border border-border bg-slate-200 dark:bg-slate-700 px-3 py-2 text-sm text-slate-800 dark:text-slate-200">
All workspaces
</div>
);
}
if (access.grants.length === 0) {
return (
- <div className="rounded-md border border-border bg-muted/30 px-3 py-2 text-sm text-muted-foreground">
+ <div className="rounded-md border border-border bg-slate-200 dark:bg-slate-700 px-3 py-2 text-sm text-slate-500 dark:text-slate-500">
No named workspace grants
</div>
);
}
const knownWorkspaces = new Set(
workspaces?.map((workspace) => workspace.name)
);
return (
<div className="max-h-56 overflow-auto rounded-md border border-border">
{access.grants.map((grant) => {
const unavailable =
workspaces !== undefined && !knownWorkspaces.has(grant.workspace);
return (
<div
key={grant.workspace}
className="flex items-center justify-between gap-3 border-b border-border px-3 py-2 text-sm last:border-b-0"
>
<div className="min-w-0">
- <div className="truncate">{grant.workspace}</div>
+ <div className="whitespace-normal break-words text-slate-800 dark:text-slate-200">{grant.workspace}</div>
{unavailable && (
- <div className="text-xs text-muted-foreground">
+ <div className="text-xs text-slate-500 dark:text-slate-500">
Workspace not currently available
</div>
)}
</div>
- <span className="rounded bg-muted px-1.5 py-0.5 text-xs capitalize text-muted-foreground">
+ <span className="rounded bg-slate-200 dark:bg-slate-700 px-1.5 py-0.5 text-xs capitalize text-slate-500 dark:text-slate-500">
{grant.role}
</span>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <div className="rounded-md border border-border bg-muted/30 px-3 py-2 text-sm"> | |
| All workspaces | |
| </div> | |
| ); | |
| } | |
| if (access.grants.length === 0) { | |
| return ( | |
| <div className="rounded-md border border-border bg-muted/30 px-3 py-2 text-sm text-muted-foreground"> | |
| No named workspace grants | |
| </div> | |
| ); | |
| } | |
| const knownWorkspaces = new Set( | |
| workspaces?.map((workspace) => workspace.name) | |
| ); | |
| return ( | |
| <div className="max-h-56 overflow-auto rounded-md border border-border"> | |
| {access.grants.map((grant) => { | |
| const unavailable = | |
| workspaces !== undefined && !knownWorkspaces.has(grant.workspace); | |
| return ( | |
| <div | |
| key={grant.workspace} | |
| className="flex items-center justify-between gap-3 border-b border-border px-3 py-2 text-sm last:border-b-0" | |
| > | |
| <div className="min-w-0"> | |
| <div className="truncate">{grant.workspace}</div> | |
| {unavailable && ( | |
| <div className="text-xs text-muted-foreground"> | |
| Workspace not currently available | |
| </div> | |
| )} | |
| </div> | |
| <span className="rounded bg-muted px-1.5 py-0.5 text-xs capitalize text-muted-foreground"> | |
| {grant.role} | |
| </span> | |
| <div className="rounded-md border border-border bg-slate-200 dark:bg-slate-700 px-3 py-2 text-sm text-slate-800 dark:text-slate-200"> | |
| All workspaces | |
| </div> | |
| ); | |
| } | |
| if (access.grants.length === 0) { | |
| return ( | |
| <div className="rounded-md border border-border bg-slate-200 dark:bg-slate-700 px-3 py-2 text-sm text-slate-500 dark:text-slate-500"> | |
| No named workspace grants | |
| </div> | |
| ); | |
| } | |
| const knownWorkspaces = new Set( | |
| workspaces?.map((workspace) => workspace.name) | |
| ); | |
| return ( | |
| <div className="max-h-56 overflow-auto rounded-md border border-border"> | |
| {access.grants.map((grant) => { | |
| const unavailable = | |
| workspaces !== undefined && !knownWorkspaces.has(grant.workspace); | |
| return ( | |
| <div | |
| key={grant.workspace} | |
| className="flex items-center justify-between gap-3 border-b border-border px-3 py-2 text-sm last:border-b-0" | |
| > | |
| <div className="min-w-0"> | |
| <div className="whitespace-normal break-words text-slate-800 dark:text-slate-200">{grant.workspace}</div> | |
| {unavailable && ( | |
| <div className="text-xs text-slate-500 dark:text-slate-500"> | |
| Workspace not currently available | |
| </div> | |
| )} | |
| </div> | |
| <span className="rounded bg-slate-200 dark:bg-slate-700 px-1.5 py-0.5 text-xs capitalize text-slate-500 dark:text-slate-500"> | |
| {grant.role} | |
| </span> |
🤖 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 `@ui/src/components/WorkspaceAccessEditor.tsx` around lines 59 - 97, Update the
styling in the WorkspaceAccessEditor render paths: replace information-block
backgrounds with bg-slate-200 dark:bg-slate-700, use text-slate-500
dark:text-slate-500 for muted labels and availability text, and text-slate-800
dark:text-slate-200 for primary workspace names. In the grants list, replace
truncate with whitespace-normal break-words so long workspace names wrap instead
of being clipped.
Source: Coding guidelines
Contributor
Full review complete. Here's the summary: CI status: The only failing CI run ( All targeted tests pass locally: oidcprovision (115 tests), config (306), auth service, frontend API, OIDC auth handler, schema validation. Security
Correctness
Edge cases handled
No issues requiring changes were identified. |
5 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add identity-provider group mappings for workspace-scoped authorization while preserving safe administrative operations and current session behavior.
Changes
Why
Organizations need identity-provider-controlled workspace access without granting global workspace visibility.
Behavior
Related Issues
No linked issue.
Testing
make testpnpm testpnpm typecheckpnpm build.local/bin/golangci-lint run --timeout=10m ./...Checklist
Summary by CodeRabbit
New Features
all/none).Bug Fixes