Redesign connection attribute configuration and add claim-driven user type resolution#3906
Open
sadilchamishka wants to merge 1 commit into
Open
Conversation
sadilchamishka
requested review from
DonOmalVindula,
ThaminduDilshan,
brionmario,
jeradrutnam,
rajithacharith and
thiva-k
as code owners
Contributor
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (22)
📝 WalkthroughWalkthroughThe PR adds claim-driven user-type resolution, grouped attribute mappings, account-linking configuration, validation and runtime support, and a redesigned frontend editor. Connection creation no longer uses a separate attribute-mapping wizard step. ChangesAttribute configuration and federated resolution
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant FederatedAuth
participant GetAttributeMappings
participant GetMappedUserType
participant Claims
FederatedAuth->>GetAttributeMappings: pass claims and IDP configuration
GetAttributeMappings->>GetMappedUserType: resolve mapped user type
GetMappedUserType->>Claims: read external or nested claim
Claims-->>GetMappedUserType: claim value
GetMappedUserType-->>GetAttributeMappings: mapped or default user type
GetAttributeMappings-->>FederatedAuth: resolved attribute mappings
Possibly related PRs
Suggested reviewers: 🚥 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR redesigns the Connections “Attributes” experience into an “Attribute Configuration” editor (user type resolution, per-user-type mappings, and account linking) and extends backend IDP attribute-configuration handling to support claim-driven user type resolution with additional validation.
Changes:
- Frontend: rebuilds
AttributeMappingSectioninto three cards (resolution, mappings by user type, account linking) and updates attribute-configuration mapping utilities + tests. - Frontend: removes the attribute-mapping step from both connection creation wizards and updates related wizard tests/i18n.
- Backend: adds
externalAttribute+valueMappingsupport to user type resolution, updates mapping resolution helpers, and adds validation + tests.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/packages/i18n/src/locales/en-US.ts | Updates labels and adds new copy for “Attribute Configuration” sections; removes wizard step copy. |
| frontend/apps/console/src/features/connections/utils/attributeConfiguration.ts | Refactors form-state ↔ API conversion to support resolution/groups/linking. |
| frontend/apps/console/src/features/connections/utils/tests/attributeConfiguration.test.ts | Updates/expands unit tests for new attribute-configuration mapping behavior. |
| frontend/apps/console/src/features/connections/pages/ConnectionDetailPage.tsx | Updates dirty-checking canonicalization and embeds the redesigned section directly. |
| frontend/apps/console/src/features/connections/pages/ConnectionCreateWizardPage.tsx | Removes attribute-mapping wizard step; creates connection at end of configure step. |
| frontend/apps/console/src/features/connections/pages/ConnectionConfigureWizardPage.tsx | Removes attribute-mapping wizard step; simplifies to single-step create flow. |
| frontend/apps/console/src/features/connections/pages/tests/ConnectionCreateWizardPage.test.tsx | Removes attribute-mapping step stubs aligned with the wizard simplification. |
| frontend/apps/console/src/features/connections/pages/tests/ConnectionConfigureWizardPage.test.tsx | Updates wizard test to create directly with no attribute configuration. |
| frontend/apps/console/src/features/connections/models/connection.ts | Extends attribute-configuration types with dynamic resolution + account linking. |
| frontend/apps/console/src/features/connections/components/create-connection/ConnectionAttributeMappingStep.tsx | Removes the now-obsolete wizard step component. |
| frontend/apps/console/src/features/connections/components/create-connection/tests/ConnectionAttributeMappingStep.test.tsx | Removes tests for the deleted wizard step component. |
| frontend/apps/console/src/features/connections/components/AttributeMappingSection.tsx | Major rewrite into three-section editor with per-user-type groups and linking. |
| frontend/apps/console/src/features/connections/components/tests/AttributeMappingSection.test.tsx | Updates component tests for new UI structure and behaviors. |
| backend/pkg/thunderidengine/providers/model.go | Adds ExternalAttribute + ValueMapping to user-type resolution model. |
| backend/internal/system/i18n/core/defaults.go | Adds backend validation error message keys for resolution validation. |
| backend/internal/oauth/oauth2/tokenservice/validator.go | Updates attribute-mapping retrieval call site for new function signature. |
| backend/internal/idp/utils.go | Updates user-type resolution and attribute-mapping selection to use claims. |
| backend/internal/idp/utils_test.go | Adds/updates unit tests for claim-driven resolution behavior. |
| backend/internal/idp/service.go | Adds validation for claim-driven user-type resolution configuration. |
| backend/internal/idp/service_test.go | Adds tests for new validation rules and error handling. |
| backend/internal/authn/oauth/service.go | Passes claims into attribute-mapping resolution in federated auth flow. |
Files not reviewed (1)
- backend/internal/system/i18n/core/defaults.go: Generated file
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/apps/console/src/features/connections/pages/ConnectionDetailPage.tsx (1)
143-150: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winClearing the whole attribute configuration is silently discarded on save.
editedAttrusesnull(untouched) vsundefined(intentionally cleared to nothing) as distinct sentinels —attrDirty(line 139) relies on that distinction. ButhandleSaveuseseditedAttr ?? baselineAttr, which treatsundefinedthe same asnulland falls back tobaselineAttr. If a user clears every group/default type/linking attribute for an existing connection,AttributeMappingSection'sonChange(wired here at 276-283) reportsconfig = undefined;attrDirtycorrectly shows unsaved changes, buthandleSavethen re-sends the oldbaselineAttr, so the "clear" is dropped without any error or feedback.🐛 Preserve the null/undefined distinction in the save payload
- attributeConfiguration: editedAttr ?? baselineAttr, + attributeConfiguration: editedAttr === null ? baselineAttr : editedAttr,Also applies to: 276-283
🤖 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 `@frontend/apps/console/src/features/connections/pages/ConnectionDetailPage.tsx` around lines 143 - 150, Preserve the null/undefined sentinel distinction when constructing the save payload in handleSave: use baselineAttr only when editedAttr is null, while passing undefined through to represent an intentionally cleared configuration. Keep the onChange wiring for AttributeMappingSection aligned with this behavior so attrDirty and the submitted attributeConfiguration remain consistent.
🧹 Nitpick comments (1)
frontend/apps/console/src/features/connections/components/AttributeMappingSection.tsx (1)
208-214: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded English
aria-labels bypass i18n.These three icon-only remove buttons use raw strings (
"remove attribute mapping","remove value mapping","remove account linking attribute") instead oft(...), unlike every other label in this file. Screen-reader users on non-English locales will hear English text here.♻️ Localize the remove-button aria-labels
- aria-label="remove attribute mapping" + aria-label={t('attributeMapping.mappings.removeRow')}- aria-label="remove value mapping" + aria-label={t('attributeMapping.resolution.valueMapping.remove')}- aria-label="remove account linking attribute" + aria-label={t('attributeMapping.linking.remove')}(add the corresponding keys to
en-US.ts)Also applies to: 501-507, 629-635
🤖 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 `@frontend/apps/console/src/features/connections/components/AttributeMappingSection.tsx` around lines 208 - 214, Replace the hardcoded aria-labels on the remove buttons in AttributeMappingSection, including the buttons near the mapping and account-linking sections, with translated t(...) calls. Add corresponding translation keys and English values to en-US.ts, preserving the existing label meanings and interpolation style.
🤖 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 `@backend/pkg/thunderidengine/providers/model.go`:
- Around line 728-729: Update the yaml tags on ExternalAttribute and
ValueMapping in the relevant model struct to use the camelCase names
externalAttribute and valueMapping, matching their JSON tags and the existing
AccountLinking convention.
In
`@frontend/apps/console/src/features/connections/components/AttributeMappingSection.tsx`:
- Around line 281-343: Guard the configuration-emitting useEffect until
useGetUserTypes() has resolved, so it cannot call onChange with incomplete
defaults while user types are still loading. Use the query’s loading/settled
state (for example, isLoading or isPending, as exposed by useGetUserTypes) in
the effect’s early-return condition and dependency array, while preserving
emission once the query resolves.
---
Outside diff comments:
In
`@frontend/apps/console/src/features/connections/pages/ConnectionDetailPage.tsx`:
- Around line 143-150: Preserve the null/undefined sentinel distinction when
constructing the save payload in handleSave: use baselineAttr only when
editedAttr is null, while passing undefined through to represent an
intentionally cleared configuration. Keep the onChange wiring for
AttributeMappingSection aligned with this behavior so attrDirty and the
submitted attributeConfiguration remain consistent.
---
Nitpick comments:
In
`@frontend/apps/console/src/features/connections/components/AttributeMappingSection.tsx`:
- Around line 208-214: Replace the hardcoded aria-labels on the remove buttons
in AttributeMappingSection, including the buttons near the mapping and
account-linking sections, with translated t(...) calls. Add corresponding
translation keys and English values to en-US.ts, preserving the existing label
meanings and interpolation style.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4417d157-e395-457c-9d67-4220c50288fc
📒 Files selected for processing (21)
backend/internal/authn/oauth/service.gobackend/internal/idp/service.gobackend/internal/idp/service_test.gobackend/internal/idp/utils.gobackend/internal/idp/utils_test.gobackend/internal/oauth/oauth2/tokenservice/validator.gobackend/internal/system/i18n/core/defaults.gobackend/pkg/thunderidengine/providers/model.gofrontend/apps/console/src/features/connections/components/AttributeMappingSection.tsxfrontend/apps/console/src/features/connections/components/__tests__/AttributeMappingSection.test.tsxfrontend/apps/console/src/features/connections/components/create-connection/ConnectionAttributeMappingStep.tsxfrontend/apps/console/src/features/connections/components/create-connection/__tests__/ConnectionAttributeMappingStep.test.tsxfrontend/apps/console/src/features/connections/models/connection.tsfrontend/apps/console/src/features/connections/pages/ConnectionConfigureWizardPage.tsxfrontend/apps/console/src/features/connections/pages/ConnectionCreateWizardPage.tsxfrontend/apps/console/src/features/connections/pages/ConnectionDetailPage.tsxfrontend/apps/console/src/features/connections/pages/__tests__/ConnectionConfigureWizardPage.test.tsxfrontend/apps/console/src/features/connections/pages/__tests__/ConnectionCreateWizardPage.test.tsxfrontend/apps/console/src/features/connections/utils/__tests__/attributeConfiguration.test.tsfrontend/apps/console/src/features/connections/utils/attributeConfiguration.tsfrontend/packages/i18n/src/locales/en-US.ts
💤 Files with no reviewable changes (2)
- frontend/apps/console/src/features/connections/components/create-connection/tests/ConnectionAttributeMappingStep.test.tsx
- frontend/apps/console/src/features/connections/components/create-connection/ConnectionAttributeMappingStep.tsx
sadilchamishka
force-pushed
the
feat/attribute-configuration-resolution
branch
2 times, most recently
from
c208f45 to
ae1cd0c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/packages/configure-connections/src/components/AttributeMappingSection.tsx (1)
281-294: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winConfig can be emitted with dynamic-resolution fields dropped while user types are still loading.
effectiveResolveDynamicis derived fromuserTypeList, which defaults to[]untiluseGetUserTypes()resolves (L282, L289-294). The mount effect (L330-343) firesonChangeunconditionally on every render, including before that query settles. During that windoweffectiveResolveDynamicis forcedfalseeven if the seededinitialConfighas dynamic resolution enabled, sotoAttributeConfigurationomitsexternalAttribute/valueMappingfrom the emitted config. If this transient payload gets persisted (e.g., admin opens the tab and saves quickly, orConnectionDetailPage'sattrDirtytoggles on this transient value), the connection's dynamic user-type resolution silently reverts to the static default. This is the same class of issue flagged in a prior review round on an earlier version of this component (different file path due to the package move) that was not resolved.🐛 Proposed fix: skip emitting until the user-types query settles
useEffect(() => { + if (userTypesQuery.isLoading) { + return; + } const config = toAttributeConfiguration({ defaultUserType, resolveDynamic: effectiveResolveDynamic, externalAttribute, valueMapping: valueMapping.map((entry) => ({value: entry.value, userType: entry.userType})), groups: groups.map((group) => ({ userType: group.userType, rows: group.rows.map((row) => ({externalAttribute: row.externalAttribute, localAttribute: row.localAttribute})), })), linking: linking.map((entry) => entry.value), }); onChange(config, valid); - }, [defaultUserType, effectiveResolveDynamic, externalAttribute, valueMapping, groups, linking, valid, onChange]); + }, [ + defaultUserType, + effectiveResolveDynamic, + externalAttribute, + valueMapping, + groups, + linking, + valid, + onChange, + userTypesQuery.isLoading, + ]);Also applies to: 330-343
🤖 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 `@frontend/packages/configure-connections/src/components/AttributeMappingSection.tsx` around lines 281 - 294, Prevent the mount/update effect from emitting configuration while useGetUserTypes is still loading. In the effect around the component’s onChange call, check the query’s loading/settled state and return without invoking onChange until userTypesQuery has resolved; then derive effectiveResolveDynamic and serialize the complete configuration, preserving seeded dynamic-resolution fields during loading.
🤖 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
`@frontend/packages/configure-connections/src/components/AttributeMappingSection.tsx`:
- Around line 319-328: Update the validation logic near groupHasContent and
valid to detect partially filled mapping rows and value-mapping entries,
including rows where exactly one of externalAttribute/localAttribute is
non-empty and entries where exactly one of value/userType is non-empty; include
these errors in valid so saving is blocked. Add corresponding error state and
helper text wiring in the row and value-mapping rendering components, using the
existing validation/error patterns, so each invalid partial entry clearly
identifies what must be completed.
In `@frontend/packages/configure-connections/src/models/connection.ts`:
- Around line 118-132: Complete the JSDoc for the AccountLinking interface by
replacing the truncated phrase “when its subject identifier does not” with a
grammatically complete description, such as “when its subject identifier does
not match an existing account.”
---
Duplicate comments:
In
`@frontend/packages/configure-connections/src/components/AttributeMappingSection.tsx`:
- Around line 281-294: Prevent the mount/update effect from emitting
configuration while useGetUserTypes is still loading. In the effect around the
component’s onChange call, check the query’s loading/settled state and return
without invoking onChange until userTypesQuery has resolved; then derive
effectiveResolveDynamic and serialize the complete configuration, preserving
seeded dynamic-resolution fields during loading.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 98f61453-2fdf-4f49-b94f-de8649483cf9
📒 Files selected for processing (20)
backend/internal/authn/oauth/service.gobackend/internal/idp/service.gobackend/internal/idp/service_test.gobackend/internal/idp/utils.gobackend/internal/idp/utils_test.gobackend/internal/oauth/oauth2/tokenservice/validator.gobackend/internal/system/i18n/core/defaults.gobackend/pkg/thunderidengine/providers/model.gofrontend/packages/configure-connections/src/components/AttributeMappingSection.tsxfrontend/packages/configure-connections/src/components/__tests__/AttributeMappingSection.test.tsxfrontend/packages/configure-connections/src/index.tsfrontend/packages/configure-connections/src/models/connection.tsfrontend/packages/configure-connections/src/pages/ConnectionConfigureWizardPage.tsxfrontend/packages/configure-connections/src/pages/ConnectionCreateWizardPage.tsxfrontend/packages/configure-connections/src/pages/ConnectionDetailPage.tsxfrontend/packages/configure-connections/src/pages/__tests__/ConnectionConfigureWizardPage.test.tsxfrontend/packages/configure-connections/src/pages/__tests__/ConnectionCreateWizardPage.test.tsxfrontend/packages/configure-connections/src/utils/__tests__/attributeConfiguration.test.tsfrontend/packages/configure-connections/src/utils/attributeConfiguration.tsfrontend/packages/i18n/src/locales/en-US.ts
✅ Files skipped from review due to trivial changes (1)
- backend/internal/system/i18n/core/defaults.go
🚧 Files skipped from review as they are similar to previous changes (8)
- backend/internal/authn/oauth/service.go
- frontend/packages/i18n/src/locales/en-US.ts
- backend/pkg/thunderidengine/providers/model.go
- backend/internal/oauth/oauth2/tokenservice/validator.go
- backend/internal/idp/utils.go
- backend/internal/idp/service_test.go
- backend/internal/idp/service.go
- backend/internal/idp/utils_test.go
sadilchamishka
force-pushed
the
feat/attribute-configuration-resolution
branch
from
ae1cd0c to
b0ec550
Compare
… type resolution Reworks the connection Attributes tab into three sections: user type resolution (static default, or claim-driven via an external attribute and value mapping), per-user-type attribute mappings, and account linking. Extends the backend UserTypeResolution model and validation to support the claim-driven case. Removes the attribute-mapping step from the create/configure wizards, since the redesigned section now only applies to editing an existing connection.
sadilchamishka
force-pushed
the
feat/attribute-configuration-resolution
branch
from
b0ec550 to
53c0239
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
Purpose
The connection Attributes tab only supported a single default user type and one flat list of attribute mappings, with no way to derive the user type from a claim and no UI for account linking. This redesigns the tab (per the "Attribute Configuration" design) into three sections and extends the backend to support claim-driven resolution.
Approach
Backend
UserTypeResolutiongainsexternalAttribute+valueMapping(external value → local user type), alongside the existingdefaultfallback. Non-breaking (omitempty).GetMappedUserType/GetAttributeMappings(internal/idp/utils.go) resolve the user type from the given claims when configured, falling back todefault.idpService.validateAttributeConfiguration: external attribute and value mapping must be set together, entries can't be empty, and every mapped user type must be a valid, existing user type.Frontend
AttributeMappingSectionrebuilt into three cards: User type resolution (static default, or a toggle revealing an external attribute + value mapping table), Attribute mappings (repeatable per-user-type mapping groups), Account linking (AND-combined external attributes).Related Issues
Related PRs
Checklist
Security checks
Summary by CodeRabbit
New Features
Bug Fixes
Improvements