Add Admin defined constraints for key manager application configurations#1246
Conversation
manodyaSenevirathne
requested review from
Krishanx92,
lasanthaS,
piyumaldk and
tharikaGitHub
as code owners
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds application-configuration constraints end-to-end: validator implementation (RANGE_MIN, RANGE_MAX, RANGE, ENUM, REGEX), UI validation and error reporting in AppConfiguration/KeyConfiguration, localization key, and wiring of constraints into Add/Edit Key Manager load/save flows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AppConfig as AppConfiguration
participant Validator as constraintValidator
participant KeyConfig as KeyConfiguration
participant KMAdmin as AddEditKeyManager
User->>AppConfig: Enter/update config value
AppConfig->>Validator: validateConstraint(value, constraint)
Validator-->>AppConfig: { valid, message }
AppConfig->>AppConfig: Update local error state
AppConfig->>KeyConfig: onValidationError(configName, hasError)
KeyConfig->>KeyConfig: Aggregate config errors + callback errors
KeyConfig->>KMAdmin: Provide constraints for save (additionalProperties.constraints)
KMAdmin->>KMAdmin: Build/merge constraints and persist Key Manager
KMAdmin-->>User: Confirm save/update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/AppConfiguration.jsx (1)
248-262:⚠️ Potential issue | 🟡 Minor
useEffectdependency array is missingconfig.The effect reads
config.constraint(line 255) but the dependency array only includes[previousValue, settingsContext]. If the constraint on a config changes (e.g., switching key managers) whilepreviousValuestays the same, validation won't re-run. Addconfig(or at leastconfig.constraint) to the dependency array.- }, [previousValue, settingsContext]); + }, [previousValue, settingsContext, config]);portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/KeyConfiguration.jsx (1)
529-538:⚠️ Potential issue | 🟡 MinorMissing
keyprop on<AppConfiguration>inside.map().React requires a unique
keywhen rendering lists. This will produce a console warning and can cause subtle UI bugs with component state preservation during re-renders.Proposed fix
{applicationConfiguration.length > 0 && applicationConfiguration.map((config) => ( <AppConfiguration + key={config.name} config={config} previousValue={getPreviousValue(config)}
🤖 Fix all issues with AI agents
In
`@portals/admin/src/main/webapp/source/src/app/components/KeyManagers/AddEditKeyManager.jsx`:
- Around line 741-746: The code assigns constraints[key] by calling
parseConstraintValueToInput with the wrong property: it passes
constraintConfig.type (UI input type) instead of the constraint type; update the
call where constraints[key] is set to pass constraintConfig.constraintType so
parseConstraintValueToInput receives the actual constraint kind (e.g., 'RANGE',
'REGEX') and returns the correct value; locate the assignment using the symbols
constraints[key], constraintConfig, and parseConstraintValueToInput to make this
change.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/constraintValidator.js`:
- Around line 50-62: The numeric branches (cases for VALIDATOR_TYPES.RANGE_MIN,
RANGE_MAX, and RANGE) incorrectly treat empty/blank input as 0 because
Number('') === 0; update each numeric branch in constraintValidator.js to first
guard against empty/blank input (check inputValue === '' || inputValue == null
|| /^\s*$/.test(inputValue)) and return { valid: false, message: <required
message> } before converting to Number; then proceed to parse numericInput and
run the existing NaN/min/max checks so blank values no longer silently pass the
range validation.
- Around line 109-131: In the VALIDATOR_TYPES.ENUM branch, guard against allowed
being null/undefined by providing a safe default or an early validation check:
ensure the destructured symbol allowed from value defaults to an empty array (or
bail out with valid:false) before any call to allowed.includes, so the
Array.isArray(inputValue) checks and the includes calls in this case
(referencing allowed, inputValue, messages, intl, and messages.enumInvalid)
never throw a TypeError; update the ENUM case to treat missing allowed as [] (or
return a clear invalid response) and use allowed.join(', ') only when
allowed.length > 0.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/KeyConfiguration.jsx`:
- Around line 218-233: In callBackHasErrors, the expression updateHasError(true
|| anyConstraintError) short-circuits and renders anyConstraintError dead;
replace that call with updateHasError(true) to be explicit (or, if you truly
meant “either callback OR constraint errors”, compute a boolean like const
hasError = true || anyConstraintError and pass hasError), updating the call in
callBackHasErrors that currently uses updateHasError(true || anyConstraintError)
and keeping surrounding calls to setHasCallbackError and setCallbackHelper
unchanged.
🧹 Nitpick comments (6)
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/constraintValidator.js (1)
134-151: Invalid regex silently passes validation — consider warning the user.When the admin-provided regex pattern is itself invalid (caught at line 146), the validator returns
{ valid: true }, silently accepting any input. This could mask a misconfiguration. A log or console warning would help admins diagnose why their regex constraint appears to have no effect.portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/AppConfiguration.jsx (2)
180-209:defineMessagescalled inside the component body on every render.
defineMessagesis a compile-time helper for message extraction and should be called at module level. Placing it inside the component doesn't break functionality, but it unnecessarily recreates the object on every render and is non-idiomatic.
118-147: Same observation forAppConfigLabelsandAppConfigToolTips.These
defineMessagescalls are also inside the component body. Consider hoisting all three (AppConfigLabels,AppConfigToolTips,ConstraintErrorMessages) to module scope.portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/KeyConfiguration.jsx (1)
162-176:handleConfigValidationErrorcaptureshasCallbackErrorfrom the render closure — potential stale state.If
setHasCallbackErrorandhandleConfigValidationErrorare called in the same React batch (e.g., during initial mount effects),hasCallbackErrorin the closure may still reflect the previous render's value. Using a ref forhasCallbackError(similar toconfigErrorsRef) or a functional updater pattern would make this more robust.This is unlikely to cause issues in practice with the current flow, but worth noting for future maintainability.
portals/admin/src/main/webapp/source/src/app/components/KeyManagers/AddEditKeyManager.jsx (2)
545-561: RANGE parsing is fragile with-as both delimiter and negative sign.Splitting on
-means ranges with negative numbers (e.g.,-100-200) will be misinterpreted. This is likely acceptable for the current use case (token expiry times are non-negative), but if this utility is reused for other constraint types in the future, it will silently fail.Consider using a more unambiguous delimiter or documenting the non-negative assumption.
759-766:additionalPropertiesis read inside the effect but not listed as a dependency.
getAppConfigConstraints()readsadditionalPropertiesfrom the closure, but onlyavailableAppConfigConstraintsandidare listed as dependencies. This works in practice due to React 18 batching (bothdispatchandsetAvailableAppConfigConstraintsare called in the same.then()callback), but it's fragile and would be flagged by thereact-hooks/exhaustive-depslint rule.If the effect is intentionally a one-time initialization, consider adding a comment explaining why
additionalPropertiesis intentionally excluded, or use a ref to capture its latest value.
portals/admin/src/main/webapp/source/src/app/components/KeyManagers/AddEditKeyManager.jsx
Show resolved
Hide resolved
...evportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/constraintValidator.js
Show resolved
Hide resolved
...evportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/constraintValidator.js
Outdated
Show resolved
Hide resolved
.../devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/KeyConfiguration.jsx
Show resolved
Hide resolved
- Fix app config getter to use correct input - Treat empty numeric inputs as invalid - Add enum safety checks.
...evportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/constraintValidator.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/KeyConfiguration.jsx (1)
528-537:⚠️ Potential issue | 🟡 MinorMissing
keyprop onAppConfigurationin the.map()iterator.React requires a
keyprop for list-rendered components. Without it, reconciliation may behave incorrectly and React will emit a console warning.Proposed fix
{applicationConfiguration.length > 0 && applicationConfiguration.map((config) => ( <AppConfiguration + key={config.name} config={config} previousValue={getPreviousValue(config)} isUserOwner={isUserOwner} handleChange={handleChange} subscriptionScopes={subscriptionScopes} onValidationError={handleConfigValidationError} /> ))}portals/admin/src/main/webapp/source/src/app/components/KeyManagers/AddEditKeyManager.jsx (1)
316-324:⚠️ Potential issue | 🟡 MinorUse
.find()instead of.map()to avoid staleavailableAppConfigConstraintsstate.The iteration at line 298 uses
.map()but discards the return values. IfkeyManagerTypeis'default'and noWSO2-ISentry exists insettings.keyManagerConfiguration, neither condition at lines 302 nor 318 matches, andsetAvailableAppConfigConstraintsis never called. This leaves the state with a stale value from a previous type selection. Consider using.find()to short-circuit the search and add a fallback that explicitly sets constraints to an empty array if no match is found.
🤖 Fix all issues with AI agents
In
`@portals/admin/src/main/webapp/source/src/app/components/KeyManagers/AddEditKeyManager.jsx`:
- Around line 620-651: The code that builds additionalPropertiesWithConstraints
silently omits constraints when parseInputToConstraintValue(constraintType,
value) returns null (e.g., malformed RANGE); update the logic in the
constraint-building block (around additionalPropertiesWithConstraints and
parseInputToConstraintValue) to detect null returns and surface a validation
error instead of silently skipping: mark the form as invalid (e.g., set
formHasErrors or a new validation state for the specific key), add a user-facing
warning/message for that constraint key, and prevent the save until corrected
(mirror the existing formHasErrors handling used elsewhere), while still
preserving the ENUM-all-selected skip behavior for CONSTRAINT_TYPES.ENUM.
- Around line 525-543: parseConstraintValueToInput crashes when constraintValue
is null because branches access properties without a guard; update
parseConstraintValueToInput to first check if constraintValue is null/undefined
and return the appropriate empty default per CONSTRAINT_TYPES (e.g., '' for
RANGE/RANGE_MIN/RANGE_MAX/REGEX, [] for ENUM, or null for unknown types) before
accessing constraintValue.min/pattern/allowed; ensure the function still handles
valid objects as before and reference the parseConstraintValueToInput function
and CONSTRAINT_TYPES constants when applying the guard so
getAppConfigConstraints can safely pass null.
- Around line 717-749: The crash happens because getAppConfigConstraints calls
parseConstraintValueToInput(constraintConfig.constraintType, null) when there is
no saved value or default; update getAppConfigConstraints to avoid passing null
— e.g., compute const inputValue = savedConstraint?.value ??
(constraintConfig.default ?? undefined) and then call
parseConstraintValueToInput(constraintConfig.constraintType, inputValue)
(keeping the existing ENUM branch for CONSTRAINT_TYPES.ENUM); reference
getAppConfigConstraints, parseConstraintValueToInput,
availableAppConfigConstraints, additionalProperties, id and
CONSTRAINT_TYPES.ENUM when making the change.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/constraintValidator.js`:
- Around line 50-62: The RANGE_MIN/RANGE_MAX/RANGE branches in the validator
(switch on type in constraintValidator.js) call inputValue.trim() but inputValue
is typed any and may be number/null/undefined; coerce inputValue to a string
before trimming (e.g., const trimmed = String(inputValue || '').trim()) and then
use Number(trimmed) for numeric checks and comparisons in the
VALIDATOR_TYPES.RANGE_MIN, VALIDATOR_TYPES.RANGE_MAX and VALIDATOR_TYPES.RANGE
cases so the validator no longer throws when inputValue is not a string.
🧹 Nitpick comments (5)
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/constraintValidator.js (1)
137-154: Silently passing validation on an invalid admin-supplied regex may hide misconfiguration.When
new RegExp(pattern)throws (invalid regex), the catch block returns{ valid: true }, effectively disabling the constraint. Consider logging a warning so admins can detect the bad pattern, rather than silently skipping.Proposed enhancement
} catch (e) { // If the regex itself is invalid, skip validation + console.warn('Invalid constraint regex pattern:', pattern, e); return { valid: true, message: '' }; }portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/KeyConfiguration.jsx (1)
147-150: Remove unusedcallbackErrorfrom destructuring.
callbackErroris destructured on line 148 but never referenced in the component. The callback-URL<TextField>error handling has been migrated to usehasCallbackError(a local state variable defined on line 163, used on line 518). RemovecallbackErrorfrom the destructuring to clean up the code.portals/admin/src/main/webapp/source/src/app/components/KeyManagers/AddEditKeyManager.jsx (3)
545-561: RANGE parsing breaks for negative numbers or values containing hyphens.
String(value).split('-')at line 549 splits on every-, so inputs like"-100-200"(negative min) or even the more exotic"100--200"produce more than 2 parts and silently returnnull. While token expiry values are likely positive, the function doesn't document or enforce this assumption, and a user-entered negative range would be silently discarded during save with no error feedback.Consider a more robust split, e.g., splitting on the last
-preceded by a digit, or requiring the delimiter to be surrounded by digits.Proposed fix — use a regex-based split
case CONSTRAINT_TYPES.RANGE: { - // "min-max" → { min, max } - const parts = String(value).split('-'); - if (parts.length !== 2) { + // "min-max" → { min, max } (supports negative numbers) + const match = String(value).match(/^\s*(-?\d+(?:\.\d+)?)\s*-\s*(-?\d+(?:\.\d+)?)\s*$/); + if (!match) { // invalid format return null; } - const min = parseFloat(parts[0].trim()); - const max = parseFloat(parts[1].trim()); + const min = parseFloat(match[1]); + const max = parseFloat(match[2]); if (Number.isNaN(min) || Number.isNaN(max)) {
296-326:.map()used for side-effects only — should be.forEach().
settings.keyManagerConfiguration.map(...)at line 298 is used purely for side effects (setting state). The return values (true/false) are unused. Using.forEach()is semantically correct and avoids allocating a discarded array.
759-766: MissingadditionalPropertiesinuseEffectdependency array.
getAppConfigConstraints()readsadditionalProperties(line 719), but the effect only depends on[availableAppConfigConstraints, id]. During edit, the flow works because bothdispatch(which updatesadditionalProperties) andsetAvailableAppConfigConstraintsare called in the same tick (line 375–376), so the first render after both state updates will have the correct data when the effect fires.However, this will trigger a React Hooks exhaustive-deps lint warning. Since adding
additionalPropertieswould re-run the effect on every constraint edit (undesirable), consider either suppressing the lint rule with a justification comment, or extractingadditionalProperties.constraintsinto a ref to break the closure dependency.
portals/admin/src/main/webapp/source/src/app/components/KeyManagers/AddEditKeyManager.jsx
Outdated
Show resolved
Hide resolved
portals/admin/src/main/webapp/source/src/app/components/KeyManagers/AddEditKeyManager.jsx
Outdated
Show resolved
Hide resolved
portals/admin/src/main/webapp/source/src/app/components/KeyManagers/AddEditKeyManager.jsx
Outdated
Show resolved
Hide resolved
...evportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/constraintValidator.js
Show resolved
Hide resolved
… its properties in `parseConstraintValueToInput`.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@portals/admin/src/main/webapp/source/src/app/components/KeyManagers/AddEditKeyManager.jsx`:
- Around line 547-560: The RANGE branch currently uses String(value).split('-')
which fails for negative numbers; replace that parsing with a regex that
captures two signed numbers and optional whitespace so negatives parse
correctly. In the CONSTRAINT_TYPES.RANGE case, match value against a pattern
like /^\s*([+-]?\d+(\.\d+)?)\s*-\s*([+-]?\d+(\.\d+)?)\s*$/; if there is no match
return null, otherwise parseFloat(match[1]) and parseFloat(match[3]) to obtain
min and max and return { min, max } (preserving the existing Number.isNaN
checks).
- Around line 759-766: The effect using useEffect should include
additionalProperties in its dependency array because getAppConfigConstraints()
reads additionalProperties?.constraints and importKMConfig() can update
additionalProperties without changing availableAppConfigConstraints; update the
dependency array for the useEffect that calls getAppConfigConstraints()
(referenced by useEffect, getAppConfigConstraints, setAppConfigConstraints,
availableAppConfigConstraints, additionalProperties, importKMConfig) to include
additionalProperties (or alternatively derive additionalProperties.constraints
into a stable ref and depend on that) so the effect re-runs with the latest
additionalProperties when config is imported.
🧹 Nitpick comments (1)
portals/admin/src/main/webapp/source/src/app/components/KeyManagers/AddEditKeyManager.jsx (1)
316-324: WSO2-IS fallback fordefaultkey manager type — verify intent.When
keyManagerType === 'default', this iterates through all KM configs looking forWSO2-ISto borrow itsavailableAppConfigConstraints. Thereturn true/return falseinside.map()has no practical effect (map return value is discarded), but the side effect (setAvailableAppConfigConstraints) will trigger for the last matching entry if multiple iterations occur. Consider using.find()or.some()instead of.map()to make the intent clearer and avoid executing the callback for every element.
portals/admin/src/main/webapp/source/src/app/components/KeyManagers/AddEditKeyManager.jsx
Outdated
Show resolved
Hide resolved
portals/admin/src/main/webapp/source/src/app/components/KeyManagers/AddEditKeyManager.jsx
Outdated
Show resolved
Hide resolved
… but allowed values are present.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/constraintValidator.js`:
- Around line 137-154: The REGEX branch currently calls new RegExp(pattern) even
when pattern is undefined, which creates a regex that matches everything; add a
guard before constructing the RegExp in the VALIDATOR_TYPES.REGEX case to detect
a missing/empty pattern (e.g., when value is {} or pattern is falsy) and
short-circuit to the same "skip validation" response used in the catch branch
(return { valid: true, message: '' }) so the validator does not silently accept
all inputs; update the logic around pattern/value in the REGEX block that
references pattern, value, inputValue, intl, and messages.
🧹 Nitpick comments (1)
portals/devportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/constraintValidator.js (1)
115-125: Fallback error message is slightly misleading for empty multi-select.When
inputValueis[](empty selection) andallowed.length > 0, the condition on line 118 triggers, but the fallback message on line 123 reads:
Values must be from: web, native. Invalid:The "Invalid: " suffix is empty, which is confusing. The
intl.formatMessagepath likely produces a cleaner message, but the fallback could be improved.Proposed fix
return { valid: false, message: intl && messages ? intl.formatMessage(messages.enumInvalid, { allowed: allowed.join(', ') }) - : `Values must be from: ${allowed.join(', ')}. Invalid: ${invalidValues.join(', ')}`, + : invalidValues.length > 0 + ? `Values must be from: ${allowed.join(', ')}. Invalid: ${invalidValues.join(', ')}` + : `At least one value must be selected from: ${allowed.join(', ')}`, };
...evportal/src/main/webapp/source/src/app/components/Shared/AppsAndKeys/constraintValidator.js
Show resolved
Hide resolved
manodyaSenevirathne
force-pushed
the
feature/admin-defined-constraints-application-properties
branch
from
f4d6958 to
e2fab4a
Compare
…d `MAX` and enhance numeric input validation for constraint fields.
|
2 participants
Purpose
This PR introduces Frontend changes for configurable security guardrails in Key Manager Application configurations
Fixes : wso2/api-manager#4663
UI improvements
See below example scenario
Admin Portal (Create/Edit Key Manager View)
Dev Portal (Applications -> OAuth2 Token -> Key Configuration)
0210.mp4
[update on 16/02/2026]
Redid the UI after the suggestions from code review
Screen.Recording.2026-02-16.at.21.28.52.mov
Screen.Recording.2026-02-16.at.21.29.49.mov
Summary by CodeRabbit
New Features
Bug Fixes
Documentation