Add FAPI 2.0 coexistence support and per-application FAPI profile selection#10290
Open
VimukthiRajapaksha wants to merge 4 commits into
Open
Add FAPI 2.0 coexistence support and per-application FAPI profile selection#10290VimukthiRajapaksha wants to merge 4 commits into
VimukthiRajapaksha wants to merge 4 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a new FAPI Security Policy feature: types, hooks and update APIs, UI components and configuration page, application/wizard/form integrations, server endpoint wiring, feature flags, build/package artifacts, and i18n translations. ChangesFAPI Security Policy Feature
Suggested reviewers:
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ 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 |
VimukthiRajapaksha
force-pushed
the
master_fapi2_v2
branch
2 times, most recently
from
03bd204 to
541364e
Compare
![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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
features/admin.applications.v1/components/forms/inbound-oidc-form.tsx (2)
2862-2916:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear hybrid-flow state when FAPI disables the section.
Hiding the hybrid-flow controls here does not clear
selectedHybridFlowResponseTypes, which is still initialized frominitialValuesand later serialized inupdateConfiguration(). Editing an app that previously had hybrid flow enabled can therefore submitenable: falsetogether with a staleresponseTypestring.Possible fix
+useEffect((): void => { + if (fapiConstraints.isHybridFlowDisabled) { + setEnableHybridFlowResponseTypeField(false); + setSelectedHybridFlowResponseTypes([]); + } +}, [ fapiConstraints.isHybridFlowDisabled ]);🤖 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 `@features/admin.applications.v1/components/forms/inbound-oidc-form.tsx` around lines 2862 - 2916, The hybrid-flow inputs are hidden when fapiConstraints.isHybridFlowDisabled is true but selectedHybridFlowResponseTypes (initialized from initialValues and serialized in updateConfiguration()) is not cleared, allowing stale responseType data to be submitted; add a handler to clear/reset selectedHybridFlowResponseTypes (and the HYBRID_FLOW_ENABLE_CONFIG Field value if present) whenever fapiConstraints.isHybridFlowDisabled becomes true or when showHybridFlowEnableConfig toggles off—e.g., add a useEffect that watches fapiConstraints.isHybridFlowDisabled / showHybridFlowEnableConfig and sets selectedHybridFlowResponseTypes = [] (and updates the Field ref hybridFlowEnableConfig) so updateConfiguration() no longer serializes stale response types.
3940-3984:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset the renewal subtree when FAPI locks refresh-token rotation off.
When
isRefreshTokenRenewalDisabledis true, this parent toggle disappears, butisRenewRefreshTokenEnabledcan remaintruefrominitialValues. That leaves theextendRenewedRefreshTokenExpiryTimechild visible and submittable, producing an inconsistent payload withrenewRefreshToken: false.Possible fix
+useEffect((): void => { + if (fapiConstraints.isRefreshTokenRenewalDisabled) { + setIsRenewRefreshTokenEnabled(false); + } +}, [ fapiConstraints.isRefreshTokenRenewalDisabled ]);- { isRenewRefreshTokenEnabled && ( + { !fapiConstraints.isRefreshTokenRenewalDisabled && isRenewRefreshTokenEnabled && (🤖 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 `@features/admin.applications.v1/components/forms/inbound-oidc-form.tsx` around lines 3940 - 3984, When fapiConstraints.isRefreshTokenRenewalDisabled becomes true the UI hides the parent toggle but does not reset related state/fields, so isRenewRefreshTokenEnabled can remain true and leave child inputs like extendRenewedRefreshTokenExpiryTime set; update the component to clear/reset the renewal subtree whenever isRefreshTokenRenewalDisabled is true: call setIsRenewRefreshTokenEnabled(false) and clear the form values for the RefreshToken Field/ref (refreshToken) and the child field extendRenewedRefreshTokenExpiryTime (via the form API or field refs) so the payload reflects renewRefreshToken: false and the child input is not submitted. Ensure this reset runs when isRefreshTokenRenewalDisabled changes (e.g., in the same effect/handler that reacts to fapiConstraints) and guard read-only behavior.
🧹 Nitpick comments (3)
features/admin.fapi-security-policy.v1/constants/fapi-security-policy-constants.ts (1)
41-85: ⚡ Quick winMake
ErrorMessagesimmutable in the constants container.
ErrorMessagesis currently writable; accidental reassignment/mutation can alter shared error behavior at runtime.Proposed change
- public static ErrorMessages: { + public static readonly ErrorMessages: Readonly<{ FAPI_CONFIG_FETCH_ERROR_CODE: IdentityAppsError; FAPI_CONFIG_FETCH_INVALID_STATUS_CODE_ERROR_CODE: IdentityAppsError; FAPI_CONFIG_UPDATE_ERROR_CODE: IdentityAppsError; DCR_CONFIG_FETCH_ERROR_CODE: IdentityAppsError; DCR_CONFIG_FETCH_INVALID_STATUS_CODE_ERROR_CODE: IdentityAppsError; DCR_CONFIG_UPDATE_ERROR_CODE: IdentityAppsError; - } = { + }> = Object.freeze({ @@ - }; + });🤖 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 `@features/admin.fapi-security-policy.v1/constants/fapi-security-policy-constants.ts` around lines 41 - 85, ErrorMessages is mutable; make it immutable by changing the declaration to a static readonly property on FapiSecurityPolicyConstants (e.g., public static readonly ErrorMessages) and freeze the object (and optionally its nested values) after creation using Object.freeze so runtime code cannot reassign or mutate the error map or the IdentityAppsError instances.features/admin.fapi-security-policy.v1/hooks/use-fapi-profile-constraints.ts (1)
39-95: ⚡ Quick winPrevent shared mutable constraint state from leaking across consumers.
The hook returns shared constant objects containing mutable arrays. If any consumer mutates
disabledGrantTypesorallowedBindingTypes, the mutation persists globally and affects later renders/profiles.Proposed hardening
-const FAPI_DISABLED_GRANT_TYPES: string[] = ["implicit", "password"]; +const FAPI_DISABLED_GRANT_TYPES: readonly string[] = Object.freeze([ "implicit", "password" ]); export interface FapiProfileConstraintsInterface { - disabledGrantTypes: string[]; + readonly disabledGrantTypes: readonly string[]; @@ - allowedBindingTypes: string[] | null; + readonly allowedBindingTypes: readonly string[] | null; } const NO_CONSTRAINTS: FapiProfileConstraintsInterface = { allowedBindingTypes: null, - disabledGrantTypes: [], + disabledGrantTypes: Object.freeze([]), @@ const FAPI1_ADVANCED_CONSTRAINTS: FapiProfileConstraintsInterface = { - allowedBindingTypes: ["certificate"], + allowedBindingTypes: Object.freeze([ "certificate" ]), @@ const FAPI2_SECURITY_CONSTRAINTS: FapiProfileConstraintsInterface = { - allowedBindingTypes: ["certificate", "DPoP"], + allowedBindingTypes: Object.freeze([ "certificate", "DPoP" ]),Also applies to: 103-226, 306-317
🤖 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 `@features/admin.fapi-security-policy.v1/hooks/use-fapi-profile-constraints.ts` around lines 39 - 95, The module currently exposes shared constant constraint objects (e.g. NO_CONSTRAINTS) that contain mutable arrays (disabledGrantTypes, allowedBindingTypes) so a consumer mutating them will leak state globally; replace these shared constants with factory functions that return a fresh FapiProfileConstraintsInterface instance (e.g. createNoConstraints()) and ensure all other profile constraint objects are constructed via factories or by cloning arrays (use [...disabledGrantTypes] / allowedBindingTypes === null ? null : [...allowedBindingTypes]) so callers always get new arrays/objects instead of references to module-level mutable arrays; update any places referencing NO_CONSTRAINTS or other constant constraint objects to call the factory.features/admin.server-configurations.v1/components/governance-connector-grid.tsx (1)
257-265: ⚡ Quick winUse the access-control
isFeatureEnabledhelper for this new gate.Line 257 adds a new feature check with
isFeatureEnabled, but this component is still using the helper from@wso2is/core/helpers. Please switch to@wso2is/access-controlfor this check path (and keep the existing issuer check aligned in the same file).Suggested diff
-import { isFeatureEnabled } from "`@wso2is/core/helpers`"; +import { isFeatureEnabled } from "`@wso2is/access-control`";As per coding guidelines, use
isFeatureEnabledfrom@wso2is/access-controlto check feature enablement.🤖 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 `@features/admin.server-configurations.v1/components/governance-connector-grid.tsx` around lines 257 - 265, The feature-gate check for connector.id === ServerConfigurationsConstants.FAPI_SECURITY_POLICY is using isFeatureEnabled from `@wso2is/core/helpers`; change it to import and use isFeatureEnabled from `@wso2is/access-control` so this new gate aligns with the existing issuer check in the file—update the import statement to pull isFeatureEnabled from "`@wso2is/access-control`" and leave the conditional using isFeatureEnabled(organizationsFeatureConfig, OrganizationManagementConstants.FEATURE_DICTIONARY.get("ORGANIZATION_FAPI_SECURITY_POLICY")) intact.
🤖 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 `@features/admin.applications.v1/components/forms/inbound-oidc-form.tsx`:
- Around line 2245-2313: Replace the hardcoded FAPI profile array and raw
Semantic UI Radio usage by iterating over the runtime
serverSupportedFapiProfiles (and optionally appending the currently-saved
unsupported profile as a disabled warning row when
isSelectedFapiProfileUnsupported is true) and render each option with the form
library Field/Radio components from `@wso2is/forms` so the value participates in
the Forms lifecycle; update the mapping that currently uses selectedFapiProfile,
setSelectedFapiProfile, componentId and isSelectedFapiProfileUnsupported to
instead bind to the form field (name "fapiProfile") via the Forms Field wrapper,
show the same info/warning icons and Popup for the unsupported/disabled row, and
remove the old hardcoded array and raw Radio/Form.Field usage.
In
`@features/admin.applications.v1/components/wizard/minimal-application-create-wizard.tsx`:
- Around line 1217-1219: The data-componentid passed to FapiEnforcementSettings
is built from testId (unstable); change it to use componentId following the
hierarchical pattern `${componentId}-fapi-enforcement-settings`. Update the JSX
prop on FapiEnforcementSettings (the data-componentid attribute) to reference
componentId instead of testId so the component ID remains consistent and follows
the parentId-childName convention.
In `@features/admin.applications.v1/package.json`:
- Line 29: The workspace dependency "`@wso2is/admin.fapi-security-policy.v1`" was
added to features/admin.applications.v1/package.json but pnpm-lock.yaml wasn’t
updated; run pnpm install from the repository root (or pnpm -w install) to
regenerate/merge the lockfile so the new workspace entry is recorded, verify
pnpm-lock.yaml now contains `@wso2is/admin.fapi-security-policy.v1`@workspace:^,
and commit the updated pnpm-lock.yaml (and any other lockfile changes) alongside
your package.json change.
In
`@features/admin.fapi-security-policy.v1/components/fapi-enforcement-settings.tsx`:
- Around line 101-117: Add a proper accessible label for the profile Select by
inserting an InputLabel (e.g., label text from
t('fapiSecurityPolicy:form.profiles.label') with "(optional)" appended if not
required) and associate it with the Select via matching labelId (eg.
`${componentId}-profile-label`) and Select id (eg.
`${componentId}-profile-select`) and labelId prop; update the existing Select
(used with value={selectedProfile}, onChange={onProfileChange},
disabled={isReadOnly}, data-componentid={`${componentId}-profile-select`}) to
include labelId and id so the InputLabel is correctly bound to the control while
keeping the map rendering of supportedProfiles and use of
FapiSecurityPolicyConstants.PROFILE_TRANSLATION_SUFFIX_MAP unchanged.
- Around line 28-29: The component currently imports and uses Semantic UI's
Checkbox (imported as Checkbox, CheckboxProps) which must be replaced with
Oxygen UI's Checkbox from `@oxygen-ui/react` and styled via MUI's styled API;
update the import to use the Oxygen Checkbox, remove or replace any use of
CheckboxProps with the appropriate props type from `@oxygen-ui/react` (or React's
input props), and refactor the Checkbox usage inside the FapiEnforcementSettings
component (the checkbox instances referenced around the previous lines ~69-82)
to use the Oxygen Checkbox and any custom styling should be implemented using
MUI's styled() wrapper around the Oxygen Checkbox component.
In `@features/admin.fapi-security-policy.v1/package.json`:
- Line 19: Update the axios dependency in package.json from "axios": "^0.19.2"
to a supported, secure release (preferably a 1.x LTS, e.g. "axios": "^1.5.0";
alternatively at minimum "axios": "^0.32.0"), then run your package manager to
regenerate the lockfile and run tests; locate the axios entry in package.json to
make the change and ensure any axios usage in the codebase is compatible with
the newer major version (adjust call sites if needed).
In
`@features/admin.fapi-security-policy.v1/pages/fapi-security-policy-configuration.tsx`:
- Around line 217-224: The DCR enforcement UI remains interactive because the
read-only flag isn't forwarded; update the FapiEnforcementSettings usage to pass
the page's read-only state via the isReadOnly prop (e.g.,
<FapiEnforcementSettings ... isReadOnly={ isReadOnly } />) so the child
component can disable controls; determine isReadOnly from the featureConfig
permission check pattern (featureConfig?.server?.scopes?.update) used elsewhere
and ensure you pass that boolean to FapiEnforcementSettings (referenced by
component name FapiEnforcementSettings and prop isReadOnly) so the page truly
behaves read-only for users without update scope.
In `@modules/i18n/src/translations/en-US/portals/fapi-security-policy.ts`:
- Around line 55-60: Update the UI labels to use canonical profile names: change
the label value for the FAPI 1 entry (the object with label "FAPI 1 Advanced")
to "FAPI 1.0 Advanced" and change the label value for the fapi2Security entry
(key: fapi2Security) from "FAPI 2 Security" to "FAPI 2.0 Security" so labels
match the profile naming used in docs and features.
---
Outside diff comments:
In `@features/admin.applications.v1/components/forms/inbound-oidc-form.tsx`:
- Around line 2862-2916: The hybrid-flow inputs are hidden when
fapiConstraints.isHybridFlowDisabled is true but selectedHybridFlowResponseTypes
(initialized from initialValues and serialized in updateConfiguration()) is not
cleared, allowing stale responseType data to be submitted; add a handler to
clear/reset selectedHybridFlowResponseTypes (and the HYBRID_FLOW_ENABLE_CONFIG
Field value if present) whenever fapiConstraints.isHybridFlowDisabled becomes
true or when showHybridFlowEnableConfig toggles off—e.g., add a useEffect that
watches fapiConstraints.isHybridFlowDisabled / showHybridFlowEnableConfig and
sets selectedHybridFlowResponseTypes = [] (and updates the Field ref
hybridFlowEnableConfig) so updateConfiguration() no longer serializes stale
response types.
- Around line 3940-3984: When fapiConstraints.isRefreshTokenRenewalDisabled
becomes true the UI hides the parent toggle but does not reset related
state/fields, so isRenewRefreshTokenEnabled can remain true and leave child
inputs like extendRenewedRefreshTokenExpiryTime set; update the component to
clear/reset the renewal subtree whenever isRefreshTokenRenewalDisabled is true:
call setIsRenewRefreshTokenEnabled(false) and clear the form values for the
RefreshToken Field/ref (refreshToken) and the child field
extendRenewedRefreshTokenExpiryTime (via the form API or field refs) so the
payload reflects renewRefreshToken: false and the child input is not submitted.
Ensure this reset runs when isRefreshTokenRenewalDisabled changes (e.g., in the
same effect/handler that reacts to fapiConstraints) and guard read-only
behavior.
---
Nitpick comments:
In
`@features/admin.fapi-security-policy.v1/constants/fapi-security-policy-constants.ts`:
- Around line 41-85: ErrorMessages is mutable; make it immutable by changing the
declaration to a static readonly property on FapiSecurityPolicyConstants (e.g.,
public static readonly ErrorMessages) and freeze the object (and optionally its
nested values) after creation using Object.freeze so runtime code cannot
reassign or mutate the error map or the IdentityAppsError instances.
In
`@features/admin.fapi-security-policy.v1/hooks/use-fapi-profile-constraints.ts`:
- Around line 39-95: The module currently exposes shared constant constraint
objects (e.g. NO_CONSTRAINTS) that contain mutable arrays (disabledGrantTypes,
allowedBindingTypes) so a consumer mutating them will leak state globally;
replace these shared constants with factory functions that return a fresh
FapiProfileConstraintsInterface instance (e.g. createNoConstraints()) and ensure
all other profile constraint objects are constructed via factories or by cloning
arrays (use [...disabledGrantTypes] / allowedBindingTypes === null ? null :
[...allowedBindingTypes]) so callers always get new arrays/objects instead of
references to module-level mutable arrays; update any places referencing
NO_CONSTRAINTS or other constant constraint objects to call the factory.
In
`@features/admin.server-configurations.v1/components/governance-connector-grid.tsx`:
- Around line 257-265: The feature-gate check for connector.id ===
ServerConfigurationsConstants.FAPI_SECURITY_POLICY is using isFeatureEnabled
from `@wso2is/core/helpers`; change it to import and use isFeatureEnabled from
`@wso2is/access-control` so this new gate aligns with the existing issuer check in
the file—update the import statement to pull isFeatureEnabled from
"`@wso2is/access-control`" and leave the conditional using
isFeatureEnabled(organizationsFeatureConfig,
OrganizationManagementConstants.FEATURE_DICTIONARY.get("ORGANIZATION_FAPI_SECURITY_POLICY"))
intact.
🪄 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.yml
Review profile: CHILL
Plan: Pro
Run ID: 6a25e910-2d36-4b97-806a-c1c895626884
📒 Files selected for processing (44)
apps/console/package.jsonapps/console/src/configs/routes.tsxfeatures/admin.applications.v1/components/forms/inbound-oidc-form.tsxfeatures/admin.applications.v1/components/wizard/minimal-application-create-wizard.tsxfeatures/admin.applications.v1/models/application-inbound.tsfeatures/admin.applications.v1/models/applications-settings.tsfeatures/admin.applications.v1/package.jsonfeatures/admin.applications.v1/pages/applications-settings.tsxfeatures/admin.core.v1/configs/app.tsfeatures/admin.core.v1/constants/app-constants.tsfeatures/admin.core.v1/constants/i18n-constants.tsfeatures/admin.core.v1/store/reducers/config.tsfeatures/admin.fapi-security-policy.v1/CHANGELOG.mdfeatures/admin.fapi-security-policy.v1/api/update-dcr-fapi-config.tsfeatures/admin.fapi-security-policy.v1/api/update-fapi-config.tsfeatures/admin.fapi-security-policy.v1/api/use-get-dcr-fapi-config.tsfeatures/admin.fapi-security-policy.v1/api/use-get-fapi-config.tsfeatures/admin.fapi-security-policy.v1/components/fapi-enforcement-settings.tsxfeatures/admin.fapi-security-policy.v1/components/supported-fapi-profiles.tsxfeatures/admin.fapi-security-policy.v1/constants/fapi-security-policy-constants.tsfeatures/admin.fapi-security-policy.v1/hooks/use-fapi-profile-constraints.tsfeatures/admin.fapi-security-policy.v1/models/fapi-security-policy.tsfeatures/admin.fapi-security-policy.v1/package.jsonfeatures/admin.fapi-security-policy.v1/pages/fapi-security-policy-configuration.tsxfeatures/admin.fapi-security-policy.v1/public-api.tsfeatures/admin.fapi-security-policy.v1/rollup.config.cjsfeatures/admin.fapi-security-policy.v1/tsconfig.jsonfeatures/admin.feature-gate.v1/constants/feature-flag-constants.tsfeatures/admin.organizations.v1/constants/organization-constants.tsfeatures/admin.server-configurations.v1/components/governance-connector-grid.tsxfeatures/admin.server-configurations.v1/configs/endpoints.tsfeatures/admin.server-configurations.v1/constants/server-configurations-constants.tsfeatures/admin.server-configurations.v1/models/endpoints.tsfeatures/admin.server-configurations.v1/utils/governance-connector-utils.tsmodules/i18n/src/constants.tsmodules/i18n/src/models/namespaces/applications-ns.tsmodules/i18n/src/models/namespaces/fapi-security-policy-ns.tsmodules/i18n/src/models/namespaces/index.tsmodules/i18n/src/models/namespaces/pages-ns.tsmodules/i18n/src/translations/en-US/meta.tsmodules/i18n/src/translations/en-US/portals/applications.tsmodules/i18n/src/translations/en-US/portals/fapi-security-policy.tsmodules/i18n/src/translations/en-US/portals/index.tsmodules/i18n/src/translations/en-US/portals/pages.ts
Comment on lines
+217
to
+224
| <FapiEnforcementSettings | ||
| data-componentid={ `${ componentId }-dcr` } | ||
| enableFapiEnforcement={ enableFapiEnforcement } | ||
| selectedProfile={ dcrFapiProfile } | ||
| supportedProfiles={ supportedProfiles } | ||
| onEnforcementToggle={ setEnableFapiEnforcement } | ||
| onProfileChange={ setDcrFapiProfile } | ||
| /> |
Contributor
There was a problem hiding this comment.
Pass isReadOnly to FapiEnforcementSettings to keep the page truly read-only.
The page derives read-only state from update scopes, but the DCR enforcement controls are still interactive because isReadOnly is not forwarded to this component.
🔧 Suggested change
<FapiEnforcementSettings
data-componentid={ `${ componentId }-dcr` }
enableFapiEnforcement={ enableFapiEnforcement }
selectedProfile={ dcrFapiProfile }
supportedProfiles={ supportedProfiles }
onEnforcementToggle={ setEnableFapiEnforcement }
onProfileChange={ setDcrFapiProfile }
+ isReadOnly={ isReadOnly }
/>Based on learnings, "for any feature that modifies server configurations via the /api/server/v1/configs/ endpoint ... perform permission checks using featureConfig?.server?.scopes?.update".
📝 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
| <FapiEnforcementSettings | |
| data-componentid={ `${ componentId }-dcr` } | |
| enableFapiEnforcement={ enableFapiEnforcement } | |
| selectedProfile={ dcrFapiProfile } | |
| supportedProfiles={ supportedProfiles } | |
| onEnforcementToggle={ setEnableFapiEnforcement } | |
| onProfileChange={ setDcrFapiProfile } | |
| /> | |
| <FapiEnforcementSettings | |
| data-componentid={ `${ componentId }-dcr` } | |
| enableFapiEnforcement={ enableFapiEnforcement } | |
| selectedProfile={ dcrFapiProfile } | |
| supportedProfiles={ supportedProfiles } | |
| onEnforcementToggle={ setEnableFapiEnforcement } | |
| onProfileChange={ setDcrFapiProfile } | |
| isReadOnly={ isReadOnly } | |
| /> |
🤖 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
`@features/admin.fapi-security-policy.v1/pages/fapi-security-policy-configuration.tsx`
around lines 217 - 224, The DCR enforcement UI remains interactive because the
read-only flag isn't forwarded; update the FapiEnforcementSettings usage to pass
the page's read-only state via the isReadOnly prop (e.g.,
<FapiEnforcementSettings ... isReadOnly={ isReadOnly } />) so the child
component can disable controls; determine isReadOnly from the featureConfig
permission check pattern (featureConfig?.server?.scopes?.update) used elsewhere
and ensure you pass that boolean to FapiEnforcementSettings (referenced by
component name FapiEnforcementSettings and prop isReadOnly) so the page truly
behaves read-only for users without update scope.
VimukthiRajapaksha
force-pushed
the
master_fapi2_v2
branch
from
541364e to
0fc5f72
Compare
Contributor
There was a problem hiding this comment.
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 (2)
features/admin.applications.v1/components/forms/inbound-oidc-form.tsx (2)
3973-4020:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset renewal state when FAPI hides the parent refresh-token control.
If
initialValues.refreshToken.renewRefreshTokenis true,isRenewRefreshTokenEnabledstays true even after FAPI disables renewal, so the child “extend expiry” field can still render and submit alongsiderenewRefreshToken: false.🛠️ Minimal fix
+useEffect((): void => { + if (fapiConstraints.isRefreshTokenRenewalDisabled) { + setIsRenewRefreshTokenEnabled(false); + } +}, [ fapiConstraints.isRefreshTokenRenewalDisabled ]); + - { isRenewRefreshTokenEnabled && ( + { !fapiConstraints.isRefreshTokenRenewalDisabled && isRenewRefreshTokenEnabled && (🤖 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 `@features/admin.applications.v1/components/forms/inbound-oidc-form.tsx` around lines 3973 - 4020, The isRenewRefreshTokenEnabled state (used to show the child "extend expiry" field) is not reset when FAPI disables the parent RefreshToken control; update the component (e.g., the InboundOIDCForm component around the RefreshToken Field) to watch fapiConstraints.isRefreshTokenRenewalDisabled and when it becomes true setIsRenewRefreshTokenEnabled(false) (use a useEffect that depends on fapiConstraints.isRefreshTokenRenewalDisabled and optionally initialValues.refreshToken.renewRefreshToken) so the child field is hidden and not submitted when renewal is disabled.
2891-2949:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear hybrid-flow state when a FAPI profile disables the section.
This only hides the controls. If an app already has hybrid flow configured,
showHybridFlowEnableConfigstaystrueandselectedHybridFlowResponseTypeskeeps its old value, so the submit path can still send a stalehybridFlow.responseTypeafter the UI disappears.🛠️ Minimal fix
+useEffect((): void => { + if (fapiConstraints.isHybridFlowDisabled) { + setEnableHybridFlowResponseTypeField(false); + setSelectedHybridFlowResponseTypes([]); + } +}, [ fapiConstraints.isHybridFlowDisabled ]); + - if(showHybridFlowEnableConfig) { + if (showHybridFlowEnableConfig && !fapiConstraints.isHybridFlowDisabled) {🤖 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 `@features/admin.applications.v1/components/forms/inbound-oidc-form.tsx` around lines 2891 - 2949, When fapiConstraints.isHybridFlowDisabled becomes true the UI only hides controls but leaves selectedHybridFlowResponseTypes and the Field (hybridFlowEnableConfig) state intact, allowing stale hybridFlow.responseType to be submitted; add a useEffect that watches fapiConstraints.isHybridFlowDisabled (and/or showHybridFlowEnableConfig) and when it flips true clears selectedHybridFlowResponseTypes and resets the form value for hybridFlow.responseType (e.g. via formik/setFieldValue or the Field ref hybridFlowEnableConfig) and ensure hybridFlowConfigValuesChangeListener is notified of the cleared state so no stale value is sent on submit.
♻️ Duplicate comments (2)
features/admin.fapi-security-policy.v1/components/fapi-enforcement-settings.tsx (2)
98-101:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd an explicit form label association for the profile Select.
Line 101 renders a
Selectwithout a bound label (InputLabel+labelId/id), so accessibility and form semantics are incomplete.Proposed fix
+import InputLabel from "`@oxygen-ui/react/InputLabel`"; ... - <FormControl size="small"> + <FormControl size="small"> + <InputLabel id={ `${ componentId }-profile-label` }> + { `${ resolvedProfileLabel } (optional)` } + </InputLabel> <Select + id={ `${ componentId }-profile-select-input` } + labelId={ `${ componentId }-profile-label` } + label={ `${ resolvedProfileLabel } (optional)` } value={ selectedProfile ?? "" } displayEmpty onChange={ (event: SelectChangeEvent<FapiProfile>): void => onProfileChange(event.target.value as FapiProfile) } disabled={ isReadOnly } data-componentid={ `${ componentId }-profile-select` } >As per coding guidelines, "All form inputs must have labels; use '(optional)' suffix for non-required fields".
Also applies to: 101-117
🤖 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 `@features/admin.fapi-security-policy.v1/components/fapi-enforcement-settings.tsx` around lines 98 - 101, The profile Select currently has no associated label — add an explicit InputLabel and bind it to the Select by creating a label id (e.g., "profile-select-label") and setting InputLabel's id and the Select's labelId and id, and pass the same label text (using resolvedProfileLabel plus an "(optional)" suffix if the field is not required) to the Select’s label prop so FormControl, InputLabel and Select are properly associated for accessibility.
28-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace Semantic UI Checkbox with Oxygen UI Checkbox.
Line 28 and Line 69 use
semantic-ui-reactin a new component, which breaks the UI library standard for new code.Proposed fix
-import { Checkbox, CheckboxProps } from "semantic-ui-react"; +import Checkbox from "`@oxygen-ui/react/Checkbox`"; ... <Checkbox checked={ enableFapiEnforcement } - onChange={ (_: React.FormEvent<HTMLInputElement>, data: CheckboxProps): void => { - const checked: boolean = data.checked ?? false; + onChange={ (event: React.ChangeEvent<HTMLInputElement>): void => { + const checked: boolean = event.target.checked; onEnforcementToggle(checked); if (checked && supportedProfiles.length > 0) { onProfileChange(supportedProfiles[0]); } } }As per coding guidelines, "All new components must use Oxygen UI (
@oxygen-ui/react) with MUI'sstyledAPI for styling".Also applies to: 69-82
🤖 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 `@features/admin.fapi-security-policy.v1/components/fapi-enforcement-settings.tsx` around lines 28 - 29, The component currently imports and uses Semantic UI's Checkbox (import { Checkbox, CheckboxProps } from "semantic-ui-react") which must be replaced with Oxygen UI; update the import to use Checkbox from "`@oxygen-ui/react`" (remove CheckboxProps import), replace all <Checkbox ...> usages in this file (including the cluster around lines 69-82) with the Oxygen Checkbox API, map any Semantic-specific props to the Oxygen equivalents (checked, onChange signature, label handling), and apply styles via MUI's styled() (wrap the Oxygen Checkbox with styled(...) if custom styling is needed); keep references to FapiProfile and other identifiers intact.
🧹 Nitpick comments (1)
modules/i18n/src/models/namespaces/fapi-security-policy-ns.ts (1)
19-19: ⚡ Quick winRename the interface to follow the required
Interfacesuffix convention.
FapiSecurityPolicyNSshould use the mandated interface naming style (e.g.,FapiSecurityPolicyNSInterface) for consistency with the TS guideline set applied in this review scope.As per coding guidelines "
**/*.{ts,tsx}: Interface names must useInterfacesuffix".🤖 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 `@modules/i18n/src/models/namespaces/fapi-security-policy-ns.ts` at line 19, Rename the interface symbol FapiSecurityPolicyNS to FapiSecurityPolicyNSInterface to comply with the Interface suffix rule; update the exported declaration (export interface FapiSecurityPolicyNS -> export interface FapiSecurityPolicyNSInterface) and rename all references/usages (types, imports/exports, and any file-local mentions) to the new name so signatures and public API remain consistent.
🤖 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
`@features/admin.applications.v1/components/wizard/minimal-application-create-wizard.tsx`:
- Around line 219-221: The current code assigns a default of "FAPI1_ADVANCED" to
serverSupportedFapiProfiles which can produce an invalid profile when fapiConfig
is missing or when the server only supports FAPI2; change the assignment to
avoid defaulting to FAPI1_ADVANCED by using the actual fetched list or an empty
array instead (e.g., set serverSupportedFapiProfiles =
fapiConfig?.supportedProfiles ?? [] or use a conditional that returns [] when
supportedProfiles is empty), update any downstream logic that assumes a
non-empty array to handle an empty serverSupportedFapiProfiles, and remove the
hardcoded FAPI1_ADVANCED fallback (references: serverSupportedFapiProfiles,
fapiConfig.supportedProfiles, FAPI1_ADVANCED).
In `@features/admin.fapi-security-policy.v1/api/update-fapi-config.ts`:
- Around line 53-79: The catch block for the Axios call is wrapping every error
into a new IdentityAppsApiException which discards richer context when the
thrown error is already an IdentityAppsApiException; update the catch in
update-fapi-config.ts to check if (error instanceof IdentityAppsApiException)
and rethrow it unchanged, otherwise build and throw a new
IdentityAppsApiException as currently done (use existing variables like
errorMessage, error.stack, (error.response?.data as { code?: string })?.code,
error.request, error.response, error.config) so you preserve original
IdentityAppsApiException context from IdentityAppsApiException throws.
In
`@features/admin.fapi-security-policy.v1/hooks/use-fapi-profile-constraints.ts`:
- Around line 46-49: The docstring for isPKCEMandatory in
use-fapi-profile-constraints.ts claims FAPI 1.0 requires PKCE, but the
implementation sets isPKCEMandatory to false; update the implementation to match
the doc (or update the comment if the intent is to support FAPI 1.0 only
conditionally). Specifically, in the code that initializes or returns
isPKCEMandatory (symbol: isPKCEMandatory in the use-fapi-profile-constraints
hook), set the default/returned value to true for FAPI 1.0 profiles (or adjust
the docstring to explicitly state when it is false) so the comment and runtime
behavior are consistent.
---
Outside diff comments:
In `@features/admin.applications.v1/components/forms/inbound-oidc-form.tsx`:
- Around line 3973-4020: The isRenewRefreshTokenEnabled state (used to show the
child "extend expiry" field) is not reset when FAPI disables the parent
RefreshToken control; update the component (e.g., the InboundOIDCForm component
around the RefreshToken Field) to watch
fapiConstraints.isRefreshTokenRenewalDisabled and when it becomes true
setIsRenewRefreshTokenEnabled(false) (use a useEffect that depends on
fapiConstraints.isRefreshTokenRenewalDisabled and optionally
initialValues.refreshToken.renewRefreshToken) so the child field is hidden and
not submitted when renewal is disabled.
- Around line 2891-2949: When fapiConstraints.isHybridFlowDisabled becomes true
the UI only hides controls but leaves selectedHybridFlowResponseTypes and the
Field (hybridFlowEnableConfig) state intact, allowing stale
hybridFlow.responseType to be submitted; add a useEffect that watches
fapiConstraints.isHybridFlowDisabled (and/or showHybridFlowEnableConfig) and
when it flips true clears selectedHybridFlowResponseTypes and resets the form
value for hybridFlow.responseType (e.g. via formik/setFieldValue or the Field
ref hybridFlowEnableConfig) and ensure hybridFlowConfigValuesChangeListener is
notified of the cleared state so no stale value is sent on submit.
---
Duplicate comments:
In
`@features/admin.fapi-security-policy.v1/components/fapi-enforcement-settings.tsx`:
- Around line 98-101: The profile Select currently has no associated label — add
an explicit InputLabel and bind it to the Select by creating a label id (e.g.,
"profile-select-label") and setting InputLabel's id and the Select's labelId and
id, and pass the same label text (using resolvedProfileLabel plus an
"(optional)" suffix if the field is not required) to the Select’s label prop so
FormControl, InputLabel and Select are properly associated for accessibility.
- Around line 28-29: The component currently imports and uses Semantic UI's
Checkbox (import { Checkbox, CheckboxProps } from "semantic-ui-react") which
must be replaced with Oxygen UI; update the import to use Checkbox from
"`@oxygen-ui/react`" (remove CheckboxProps import), replace all <Checkbox ...>
usages in this file (including the cluster around lines 69-82) with the Oxygen
Checkbox API, map any Semantic-specific props to the Oxygen equivalents
(checked, onChange signature, label handling), and apply styles via MUI's
styled() (wrap the Oxygen Checkbox with styled(...) if custom styling is
needed); keep references to FapiProfile and other identifiers intact.
---
Nitpick comments:
In `@modules/i18n/src/models/namespaces/fapi-security-policy-ns.ts`:
- Line 19: Rename the interface symbol FapiSecurityPolicyNS to
FapiSecurityPolicyNSInterface to comply with the Interface suffix rule; update
the exported declaration (export interface FapiSecurityPolicyNS -> export
interface FapiSecurityPolicyNSInterface) and rename all references/usages
(types, imports/exports, and any file-local mentions) to the new name so
signatures and public API remain consistent.
🪄 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.yml
Review profile: CHILL
Plan: Pro
Run ID: 227d032e-b9d6-4112-8963-c4dffcfa095a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (45)
apps/console/package.jsonapps/console/src/configs/routes.tsxfeatures/admin.applications.v1/api/application.tsfeatures/admin.applications.v1/components/forms/inbound-oidc-form.tsxfeatures/admin.applications.v1/components/wizard/minimal-application-create-wizard.tsxfeatures/admin.applications.v1/models/application-inbound.tsfeatures/admin.applications.v1/models/applications-settings.tsfeatures/admin.applications.v1/package.jsonfeatures/admin.applications.v1/pages/applications-settings.tsxfeatures/admin.core.v1/configs/app.tsfeatures/admin.core.v1/constants/app-constants.tsfeatures/admin.core.v1/constants/i18n-constants.tsfeatures/admin.core.v1/store/reducers/config.tsfeatures/admin.fapi-security-policy.v1/CHANGELOG.mdfeatures/admin.fapi-security-policy.v1/api/update-dcr-fapi-config.tsfeatures/admin.fapi-security-policy.v1/api/update-fapi-config.tsfeatures/admin.fapi-security-policy.v1/api/use-get-dcr-fapi-config.tsfeatures/admin.fapi-security-policy.v1/api/use-get-fapi-config.tsfeatures/admin.fapi-security-policy.v1/components/fapi-enforcement-settings.tsxfeatures/admin.fapi-security-policy.v1/components/supported-fapi-profiles.tsxfeatures/admin.fapi-security-policy.v1/constants/fapi-security-policy-constants.tsfeatures/admin.fapi-security-policy.v1/hooks/use-fapi-profile-constraints.tsfeatures/admin.fapi-security-policy.v1/models/fapi-security-policy.tsfeatures/admin.fapi-security-policy.v1/package.jsonfeatures/admin.fapi-security-policy.v1/pages/fapi-security-policy-configuration.tsxfeatures/admin.fapi-security-policy.v1/public-api.tsfeatures/admin.fapi-security-policy.v1/rollup.config.cjsfeatures/admin.fapi-security-policy.v1/tsconfig.jsonfeatures/admin.feature-gate.v1/constants/feature-flag-constants.tsfeatures/admin.organizations.v1/constants/organization-constants.tsfeatures/admin.server-configurations.v1/components/governance-connector-grid.tsxfeatures/admin.server-configurations.v1/configs/endpoints.tsfeatures/admin.server-configurations.v1/constants/server-configurations-constants.tsfeatures/admin.server-configurations.v1/models/endpoints.tsfeatures/admin.server-configurations.v1/utils/governance-connector-utils.tsmodules/i18n/src/constants.tsmodules/i18n/src/models/namespaces/applications-ns.tsmodules/i18n/src/models/namespaces/fapi-security-policy-ns.tsmodules/i18n/src/models/namespaces/index.tsmodules/i18n/src/models/namespaces/pages-ns.tsmodules/i18n/src/translations/en-US/meta.tsmodules/i18n/src/translations/en-US/portals/applications.tsmodules/i18n/src/translations/en-US/portals/fapi-security-policy.tsmodules/i18n/src/translations/en-US/portals/index.tsmodules/i18n/src/translations/en-US/portals/pages.ts
✅ Files skipped from review due to trivial changes (7)
- features/admin.fapi-security-policy.v1/CHANGELOG.md
- apps/console/package.json
- features/admin.feature-gate.v1/constants/feature-flag-constants.ts
- features/admin.applications.v1/models/applications-settings.ts
- features/admin.core.v1/store/reducers/config.ts
- modules/i18n/src/translations/en-US/portals/applications.ts
- modules/i18n/src/models/namespaces/index.ts
Comment on lines
+53
to
+79
| .then((response: AxiosResponse) => { | ||
| if (response.status !== 200) { | ||
| throw new IdentityAppsApiException( | ||
| FapiSecurityPolicyConstants.ErrorMessages | ||
| .FAPI_CONFIG_UPDATE_ERROR_CODE.getErrorMessage(), | ||
| null, | ||
| response.status, | ||
| response.request, | ||
| response, | ||
| response.config | ||
| ); | ||
| } | ||
|
|
||
| return Promise.resolve(response.data as FapiConfigAPIResponseInterface); | ||
| }) | ||
| .catch((error: AxiosError) => { | ||
| const errorMessage: string = (error.response?.data as { message?: string })?.message | ||
| ?? FapiSecurityPolicyConstants.ErrorMessages.FAPI_CONFIG_UPDATE_ERROR_CODE.getErrorMessage(); | ||
|
|
||
| throw new IdentityAppsApiException( | ||
| errorMessage, | ||
| error.stack, | ||
| (error.response?.data as { code?: string })?.code, | ||
| error.request, | ||
| error.response, | ||
| error.config | ||
| ); |
Contributor
There was a problem hiding this comment.
Preserve already-built IdentityAppsApiException in catch path.
The exception thrown at Line 55 is re-caught at Line 68 and wrapped again, which drops the richer context from the first throw.
🔧 Suggested fix
- .catch((error: AxiosError) => {
- const errorMessage: string = (error.response?.data as { message?: string })?.message
+ .catch((error: unknown) => {
+ if (error instanceof IdentityAppsApiException) {
+ throw error;
+ }
+
+ const axiosError: AxiosError = error as AxiosError;
+ const errorMessage: string = (axiosError.response?.data as { message?: string })?.message
?? FapiSecurityPolicyConstants.ErrorMessages.FAPI_CONFIG_UPDATE_ERROR_CODE.getErrorMessage();
throw new IdentityAppsApiException(
errorMessage,
- error.stack,
- (error.response?.data as { code?: string })?.code,
- error.request,
- error.response,
- error.config
+ axiosError.stack,
+ (axiosError.response?.data as { code?: string })?.code,
+ axiosError.request,
+ axiosError.response,
+ axiosError.config
);
});📝 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
| .then((response: AxiosResponse) => { | |
| if (response.status !== 200) { | |
| throw new IdentityAppsApiException( | |
| FapiSecurityPolicyConstants.ErrorMessages | |
| .FAPI_CONFIG_UPDATE_ERROR_CODE.getErrorMessage(), | |
| null, | |
| response.status, | |
| response.request, | |
| response, | |
| response.config | |
| ); | |
| } | |
| return Promise.resolve(response.data as FapiConfigAPIResponseInterface); | |
| }) | |
| .catch((error: AxiosError) => { | |
| const errorMessage: string = (error.response?.data as { message?: string })?.message | |
| ?? FapiSecurityPolicyConstants.ErrorMessages.FAPI_CONFIG_UPDATE_ERROR_CODE.getErrorMessage(); | |
| throw new IdentityAppsApiException( | |
| errorMessage, | |
| error.stack, | |
| (error.response?.data as { code?: string })?.code, | |
| error.request, | |
| error.response, | |
| error.config | |
| ); | |
| .then((response: AxiosResponse) => { | |
| if (response.status !== 200) { | |
| throw new IdentityAppsApiException( | |
| FapiSecurityPolicyConstants.ErrorMessages | |
| .FAPI_CONFIG_UPDATE_ERROR_CODE.getErrorMessage(), | |
| null, | |
| response.status, | |
| response.request, | |
| response, | |
| response.config | |
| ); | |
| } | |
| return Promise.resolve(response.data as FapiConfigAPIResponseInterface); | |
| }) | |
| .catch((error: unknown) => { | |
| if (error instanceof IdentityAppsApiException) { | |
| throw error; | |
| } | |
| const axiosError: AxiosError = error as AxiosError; | |
| const errorMessage: string = (axiosError.response?.data as { message?: string })?.message | |
| ?? FapiSecurityPolicyConstants.ErrorMessages.FAPI_CONFIG_UPDATE_ERROR_CODE.getErrorMessage(); | |
| throw new IdentityAppsApiException( | |
| errorMessage, | |
| axiosError.stack, | |
| (axiosError.response?.data as { code?: string })?.code, | |
| axiosError.request, | |
| axiosError.response, | |
| axiosError.config | |
| ); | |
| }) |
🤖 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 `@features/admin.fapi-security-policy.v1/api/update-fapi-config.ts` around
lines 53 - 79, The catch block for the Axios call is wrapping every error into a
new IdentityAppsApiException which discards richer context when the thrown error
is already an IdentityAppsApiException; update the catch in
update-fapi-config.ts to check if (error instanceof IdentityAppsApiException)
and rethrow it unchanged, otherwise build and throw a new
IdentityAppsApiException as currently done (use existing variables like
errorMessage, error.stack, (error.response?.data as { code?: string })?.code,
error.request, error.response, error.config) so you preserve original
IdentityAppsApiException context from IdentityAppsApiException throws.
Comment on lines
+46
to
+49
| * Whether PKCE must be enforced as mandatory. | ||
| * FAPI 1.0 §5.2.2.2, FAPI 2.0 §5.3.2.2.5 — PKCE with S256 required. | ||
| */ | ||
| isPKCEMandatory: boolean; |
Contributor
There was a problem hiding this comment.
Align PKCE docstring with actual FAPI 1.0 constraint value.
The interface comment says FAPI 1.0 requires PKCE, but Line 153 sets isPKCEMandatory to false. Please reconcile the doc to avoid future mis-implementation.
🔧 Suggested fix
- * Whether PKCE must be enforced as mandatory.
- * FAPI 1.0 §5.2.2.2, FAPI 2.0 §5.3.2.2.5 — PKCE with S256 required.
+ * Whether PKCE must be enforced as mandatory.
+ * Mandatory for FAPI 2.0 (§5.3.2.2.5). For FAPI 1.0 Advanced, this hook keeps it optional.Also applies to: 153-153
🤖 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 `@features/admin.fapi-security-policy.v1/hooks/use-fapi-profile-constraints.ts`
around lines 46 - 49, The docstring for isPKCEMandatory in
use-fapi-profile-constraints.ts claims FAPI 1.0 requires PKCE, but the
implementation sets isPKCEMandatory to false; update the implementation to match
the doc (or update the comment if the intent is to support FAPI 1.0 only
conditionally). Specifically, in the code that initializes or returns
isPKCEMandatory (symbol: isPKCEMandatory in the use-fapi-profile-constraints
hook), set the default/returned value to true for FAPI 1.0 profiles (or adjust
the docstring to explicitly state when it is false) so the comment and runtime
behavior are consistent.
Contributor
Author
|
Attaching screenshots of the following changes below:
|
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
1 participant
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
This PR introduces FAPI 2.0 coexistence support alongside FAPI 1.0 Advanced, enabling organizations to run both FAPI profiles simultaneously and migrate applications at their own pace.
What's included:
New
@wso2is/admin.fapi-security-policy.v1feature package:GET/PATCH /api/server/v1/configs/fapiandGET/PATCH /api/server/v1/configs/dcruseFapiProfileConstraintshook:Per-application FAPI profile selection (OIDC form):
isFAPIApplication = true), the OIDC protocol form now renders a FAPI Profile radio selector showing all server-supported profilesuseFapiProfileConstraintsto automatically lock/constrain relevant protocol settings (grant types, PKCE, PAR, token binding, hybrid flow, etc.)Shared reusable components exported from the feature package:
SupportedFapiProfiles— supports both"multiple"(checkbox, server-level) and"single"(radio, per-app) selection modesFapiEnforcementSettings— reusable checkbox + profile dropdown for DCR enforcement, with overridable labelsSupporting infrastructure:
/login-and-registration/fapi-security-policyloginAndRegistration.organizationSettings.fapiSecurityPolicyfapiSecurityPolicywith full English translations (profiles, DCR settings, constraints hints, notifications)Related Issues
wso2-enterprise/iam-product-management#601
Related PRs
Checklist
Security checks
Developer Checklist (Mandatory)
product-isissue to track any behavioral change or migration impact.