Add multi-device support for push authentication#10431
Conversation
|
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:
📝 WalkthroughWalkthroughAdds push device management configuration end-to-end: deployment/runtime config now exposes a ChangesPush Device Management End-to-End
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10431 +/- ##
==========================================
+ Coverage 72.83% 72.90% +0.06%
==========================================
Files 470 470
Lines 71191 71365 +174
Branches 240 448 +208
==========================================
+ Hits 51855 52029 +174
+ Misses 19225 19040 -185
- Partials 111 296 +185
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/myaccount/src/components/multi-factor-authentication/authenticators/push-authenticator.tsx (1)
99-105:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid rendering an empty modal action bar during the QR step.
When
qrCodeis present, no action is returned, butModal.Actionsis still rendered. This leaves an empty footer area.Suggested fix
- { ( + { !qrCode && ( <Modal.Actions data-componentId={ `${ componentId }-view-modal-actions` } className ="actions" > { renderPushAuthenticatorWizardActions() } - </Modal.Actions>) - } + </Modal.Actions> + ) }Also applies to: 200-216
🤖 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 `@apps/myaccount/src/components/multi-factor-authentication/authenticators/push-authenticator.tsx` around lines 99 - 105, The Modal.Actions component is being rendered unconditionally even when renderPushAuthenticatorWizardActions() returns no content during the QR code step, resulting in an empty footer area. Modify the code to conditionally render the Modal.Actions wrapper only when renderPushAuthenticatorWizardActions() returns actual content. You can achieve this by storing the result of renderPushAuthenticatorWizardActions() in a variable first, then wrapping the entire Modal.Actions component in a conditional that checks if that result is not empty or null. Apply this fix wherever Modal.Actions is used with renderPushAuthenticatorWizardActions() throughout the file.
🧹 Nitpick comments (2)
apps/myaccount/src/components/multi-factor-authentication/authenticators/push-authenticator.tsx (1)
301-301: ⚡ Quick winUse
data-componentidfor new selector attributes.This newly-added selector uses
data-componentId; align it with the project’s selector convention to keep tests consistent.As per coding guidelines, "Use
data-componentidfor DOM element selection in tests ... Do not usedata-testid."🤖 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 `@apps/myaccount/src/components/multi-factor-authentication/authenticators/push-authenticator.tsx` at line 301, The selector attribute at line 301 uses `data-componentId` with camelCase, which does not align with the project's convention for test selector attributes. Change the attribute name from `data-componentId` to `data-componentid` (all lowercase) to maintain consistency with the project's coding guidelines for DOM element selection in tests.Source: Coding guidelines
apps/myaccount/src/hooks/use-push-authenticator.ts (1)
203-219: ⚡ Quick winAdd a named return interface for this custom hook.
The expanded return object is currently anonymous; a named return interface will keep call sites stable and self-documented.
As per coding guidelines, "Define a named return interface for custom hooks ... instead of using anonymous object types."
🤖 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 `@apps/myaccount/src/hooks/use-push-authenticator.ts` around lines 203 - 219, Create a named TypeScript interface for the return type of the usePushAuthenticator hook. Define an interface that includes all the properties currently being returned in the anonymous object: deleteRegisteredDevice, deviceLimit, handlePushAuthenticatorInitCancel, handlePushAuthenticatorSetupSubmit, initPushAuthenticatorRegFlow, isConfigPushAuthenticatorModalOpen, isDeviceLimitReached, isLoading, isPushDeviceMgtConfigLoading, isMultipleDeviceEnrollmentEnabled, isRegisteredDeviceListLoading, qrCode, registeredDeviceList, setIsConfigPushAuthenticatorModalOpen, and translateKey. Then apply this named interface as the explicit return type annotation for the usePushAuthenticator hook function.Source: Coding guidelines
🤖 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
`@apps/console/java/org.wso2.identity.apps.console.server.feature/resources/deployment.config.json.j2`:
- Around line 2847-2849: The `maxDeviceLimitUpperBound` property in the
`pushDeviceManagement` object is accessed without checking if the optional field
is defined, which can result in invalid JSON when the value is undefined. Wrap
the `maxDeviceLimitUpperBound` line with an `{% if
push_device_management.max_device_limit_upper_bound is defined %}` guard and
close it with `{% endif %}`, following the same conditional rendering pattern
used elsewhere in the template for optional fields like `routes`.
In `@apps/myaccount/src/hooks/use-push-authenticator.ts`:
- Around line 97-99: The maximumDeviceLimit parsing in the
PushAuthenticatorConstants.PROPERTY_MAXIMUM_DEVICE_LIMIT section uses parseInt
which can return NaN if the value is non-numeric or empty, causing
isDeviceLimitReached to behave unexpectedly. After parsing with parseInt,
validate that the result is a valid number using Number.isNaN() or isNaN(), and
if it is NaN, fall back to the default value of 1 instead of passing NaN
downstream. This ensures that invalid or missing preference values default to
the expected limit rather than bypassing device limit checks.
In `@apps/myaccount/src/models/push-authenticator.ts`:
- Around line 38-40: The interface ConfigPreferencePropertyInterface is exported
but unused, triggering a Knip linting error. Remove the export keyword from the
interface declaration so it becomes a local type only, preserving its use within
the file while eliminating the unused exported type warning.
In `@features/admin.identity-providers.v1/api/push-device-mgt-configs.ts`:
- Around line 57-63: The isLoading derivation in the return statement of the
hook at line 57 uses the formula !error && !data, which does not account for the
disabled fetch case. When shouldFetch is false, the requestConfig is null and
SWR does not run, leaving both data and error as undefined, causing isLoading to
evaluate to true indefinitely and block downstream logic in
push-authenticator-form.tsx. Fix the isLoading derivation to also check the
shouldFetch condition, so that when shouldFetch is false, isLoading is false to
allow the downstream useEffect to proceed properly.
In
`@features/admin.identity-providers.v1/components/forms/authenticators/push-authenticator-form.tsx`:
- Around line 241-242: Replace all instances of data-testid with
data-componentid throughout the push-authenticator-form component to comply with
project coding guidelines. Locate each occurrence where ["data-testid"] is used
(including the destructuring at line 241 and all other instances mentioned at
lines 676-677, 703-704, 787-788, 991-992, and 1016-1017) and change them to
["data-componentid"] to align with the IdentifiableComponentInterface pattern
required by the project.
In `@identity-apps-core/apps/authentication-portal/src/main/webapp/pushAuth.jsp`:
- Around line 334-339: The code retrieves and parses a stored timestamp from
sessionStorage using PUSH_AUTH_STORAGE_KEY, but does not validate that parseInt
returns a valid number. If the stored value is malformed, parseInt will return
NaN and cause invalid countdown logic. After parsing the stored value with
parseInt(stored, 10), add a validation check to ensure the result is not NaN. If
the parsed value is invalid, treat it the same as the else branch by setting
startedAt = Date.now() and updating sessionStorage with the fresh timestamp to
ensure valid state is always used for countdown calculations.
In
`@identity-apps-core/apps/authentication-portal/src/main/webapp/pushAuthError.jsp`:
- Around line 64-65: The i18n key error.push.max.device.limit.reached is defined
in the base English translation file Resources.properties but is missing from
all locale-specific variant files (de_DE, es_ES, fr_FR, ja_JP, nl_NL, pt_BR,
pt_PT, zh_CN). Add the error.push.max.device.limit.reached key with appropriate
translations to each of these locale-specific properties files to ensure
non-English users see translated error messages instead of the English fallback
when this error condition is triggered.
In `@modules/i18n/src/translations/pt-BR/portals/myaccount.ts`:
- Line 1030: The deviceLimitReachedHint translation string uses the plural form
"dispositivos" which reads awkwardly when limit equals 1. Update the string to
use pluralization-safe wording by either implementing proper i18n pluralization
rules if supported by the translation system (typically with separate singular
and plural variants), or by using neutral wording that works grammatically for
both singular and plural cases. Ensure the updated text reads naturally whether
the limit value is 1 or greater than 1.
In `@modules/i18n/src/translations/pt-PT/portals/myaccount.ts`:
- Line 1055: The deviceLimitReachedHint translation string uses plural form
"dispositivos" which reads awkwardly when limit equals 1 (e.g., "1 dispositivos"
is grammatically incorrect in Portuguese). Update the translation to use
pluralization-safe wording that works for both singular and plural cases, such
as using a neutral form like "dispositivo(s)" or restructuring the message to
avoid explicit plural agreement with the numeric variable, ensuring the text
reads naturally regardless of whether limit is 1 or greater than 1.
---
Outside diff comments:
In
`@apps/myaccount/src/components/multi-factor-authentication/authenticators/push-authenticator.tsx`:
- Around line 99-105: The Modal.Actions component is being rendered
unconditionally even when renderPushAuthenticatorWizardActions() returns no
content during the QR code step, resulting in an empty footer area. Modify the
code to conditionally render the Modal.Actions wrapper only when
renderPushAuthenticatorWizardActions() returns actual content. You can achieve
this by storing the result of renderPushAuthenticatorWizardActions() in a
variable first, then wrapping the entire Modal.Actions component in a
conditional that checks if that result is not empty or null. Apply this fix
wherever Modal.Actions is used with renderPushAuthenticatorWizardActions()
throughout the file.
---
Nitpick comments:
In
`@apps/myaccount/src/components/multi-factor-authentication/authenticators/push-authenticator.tsx`:
- Line 301: The selector attribute at line 301 uses `data-componentId` with
camelCase, which does not align with the project's convention for test selector
attributes. Change the attribute name from `data-componentId` to
`data-componentid` (all lowercase) to maintain consistency with the project's
coding guidelines for DOM element selection in tests.
In `@apps/myaccount/src/hooks/use-push-authenticator.ts`:
- Around line 203-219: Create a named TypeScript interface for the return type
of the usePushAuthenticator hook. Define an interface that includes all the
properties currently being returned in the anonymous object:
deleteRegisteredDevice, deviceLimit, handlePushAuthenticatorInitCancel,
handlePushAuthenticatorSetupSubmit, initPushAuthenticatorRegFlow,
isConfigPushAuthenticatorModalOpen, isDeviceLimitReached, isLoading,
isPushDeviceMgtConfigLoading, isMultipleDeviceEnrollmentEnabled,
isRegisteredDeviceListLoading, qrCode, registeredDeviceList,
setIsConfigPushAuthenticatorModalOpen, and translateKey. Then apply this named
interface as the explicit return type annotation for the usePushAuthenticator
hook function.
🪄 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: 19a97ea1-75da-4210-9bd8-e7ba8542a194
📒 Files selected for processing (38)
apps/console/java/org.wso2.identity.apps.console.server.feature/resources/deployment.config.json.j2apps/console/src/public/deployment.config.jsonapps/myaccount/src/api/multi-factor-push.tsapps/myaccount/src/components/multi-factor-authentication/authenticators/push-authenticator.tsxapps/myaccount/src/configs/app.tsapps/myaccount/src/constants/mfa-constants.tsapps/myaccount/src/hooks/use-push-authenticator.tsapps/myaccount/src/models/app-config.tsapps/myaccount/src/models/push-authenticator.tsfeatures/admin.connections.v1/configs/endpoints.tsfeatures/admin.connections.v1/constants/connection-ui-constants.tsfeatures/admin.connections.v1/models/endpoints.tsfeatures/admin.core.v1/configs/app.tsfeatures/admin.core.v1/models/config.tsfeatures/admin.core.v1/store/reducers/config.tsfeatures/admin.identity-providers.v1/api/index.tsfeatures/admin.identity-providers.v1/api/push-device-mgt-configs.tsfeatures/admin.identity-providers.v1/components/forms/authenticators/push-authenticator-form.scssfeatures/admin.identity-providers.v1/components/forms/authenticators/push-authenticator-form.tsxfeatures/admin.identity-providers.v1/models/identity-provider.tsidentity-apps-core/apps/authentication-portal/src/main/resources/org/wso2/carbon/identity/application/authentication/endpoint/i18n/Resources.propertiesidentity-apps-core/apps/authentication-portal/src/main/webapp/pushAuth.jspidentity-apps-core/apps/authentication-portal/src/main/webapp/pushAuthError.jspidentity-apps-core/apps/authentication-portal/src/main/webapp/pushEnroll.jspmodules/i18n/src/models/namespaces/authentication-provider-ns.tsmodules/i18n/src/models/namespaces/myaccount-ns.tsmodules/i18n/src/translations/de-DE/portals/myaccount.tsmodules/i18n/src/translations/en-US/portals/authentication-provider.tsmodules/i18n/src/translations/en-US/portals/myaccount.tsmodules/i18n/src/translations/es-ES/portals/myaccount.tsmodules/i18n/src/translations/fr-FR/portals/myaccount.tsmodules/i18n/src/translations/ja-JP/portals/myaccount.tsmodules/i18n/src/translations/nl-NL/portals/myaccount.tsmodules/i18n/src/translations/pt-BR/portals/myaccount.tsmodules/i18n/src/translations/pt-PT/portals/myaccount.tsmodules/i18n/src/translations/si-LK/portals/myaccount.tsmodules/i18n/src/translations/ta-IN/portals/myaccount.tsmodules/i18n/src/translations/zh-CN/portals/myaccount.ts
d1e72ad to
b8255eb
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
features/admin.identity-providers.v1/components/forms/authenticators/push-authenticator-form.tsx (1)
487-523: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKeep the push-device update and authenticator save consistent.
features/admin.identity-providers.v1/components/forms/authenticators/push-authenticator-form.tsx:487-523
updatePushDeviceMgtConfig()is committed first; if the authenticator save fails afterward, the push-specific settings are already persisted and the two endpoints drift apart. Add rollback/compensation or make the save path atomic.🤖 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.identity-providers.v1/components/forms/authenticators/push-authenticator-form.tsx` around lines 487 - 523, The push-device config update and authenticator save in push-authenticator-form are not atomic, so a successful updatePushDeviceMgtConfig can leave state committed even if the later onSubmit(authenticatorConfig) fails. Update the submit flow around updatePushDeviceMgtConfig, onSubmit, and the catch/finally chain to either perform both operations as one coordinated transaction or add compensation logic that rolls back the push-device config when the authenticator save fails. Keep the existing error handling and mutatePushDeviceMgtConfig refresh, but ensure the two endpoints cannot drift apart after a partial failure.
🧹 Nitpick comments (2)
features/admin.identity-providers.v1/components/forms/authenticators/push-authenticator-form.tsx (2)
278-283: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
useSelectorselector lacks an explicit return type, and thenumberannotation ignores theundefinedcase.The optional chain can resolve to
undefined, so annotatingconfiguredMaxDeviceLimitUpperBoundasnumberis inaccurate. The??fallback on Line 281 already handles this, but the selector return type should be explicit per project rules.♻️ Suggested typing
- const configuredMaxDeviceLimitUpperBound: number = useSelector( - (state: AppState) => state?.config?.ui?.pushDeviceManagement?.maxDeviceLimitUpperBound - ); + const configuredMaxDeviceLimitUpperBound: number = useSelector( + (state: AppState): number => state?.config?.ui?.pushDeviceManagement?.maxDeviceLimitUpperBound + );As per coding guidelines: "Always include explicit type annotations on
useSelectorhook with the selector function parameter type and return type".🤖 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.identity-providers.v1/components/forms/authenticators/push-authenticator-form.tsx` around lines 278 - 283, The `useSelector` call in `push-authenticator-form.tsx` should use an explicitly typed selector function return type, and the selected value should be allowed to be `undefined` instead of being annotated as `number`. Update the selector in the `configuredMaxDeviceLimitUpperBound` assignment to declare both the `state: AppState` parameter type and the selector return type, then keep the `??` fallback in `maxDeviceLimitUpperBound` to handle the `undefined` case cleanly.Source: Coding guidelines
617-619: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid
anyin theonSubmithandler.The values are cast to
PushAuthenticatorFormInitialValuesInterfaceon the next line, so type the parameter accordingly (orRecord<string, unknown>) instead ofRecord<string, any>.♻️ Suggested change
- onSubmit={ (values: Record<string, any>) =>{ - handleFormSubmit(values as PushAuthenticatorFormInitialValuesInterface); - } } + onSubmit={ (values: Record<string, unknown>): void => { + handleFormSubmit(values as PushAuthenticatorFormInitialValuesInterface); + } }As per coding guidelines: "Never use
anytype; use proper types orunknownwith type guards instead".🤖 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.identity-providers.v1/components/forms/authenticators/push-authenticator-form.tsx` around lines 617 - 619, The onSubmit handler in PushAuthenticatorForm is using Record<string, any> unnecessarily; replace that parameter type with PushAuthenticatorFormInitialValuesInterface (or Record<string, unknown> if needed) so the form callback avoids any and matches the existing cast in handleFormSubmit. Keep the change localized to the onSubmit prop where the values argument is declared.Source: Coding guidelines
🤖 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.
Outside diff comments:
In
`@features/admin.identity-providers.v1/components/forms/authenticators/push-authenticator-form.tsx`:
- Around line 487-523: The push-device config update and authenticator save in
push-authenticator-form are not atomic, so a successful
updatePushDeviceMgtConfig can leave state committed even if the later
onSubmit(authenticatorConfig) fails. Update the submit flow around
updatePushDeviceMgtConfig, onSubmit, and the catch/finally chain to either
perform both operations as one coordinated transaction or add compensation logic
that rolls back the push-device config when the authenticator save fails. Keep
the existing error handling and mutatePushDeviceMgtConfig refresh, but ensure
the two endpoints cannot drift apart after a partial failure.
---
Nitpick comments:
In
`@features/admin.identity-providers.v1/components/forms/authenticators/push-authenticator-form.tsx`:
- Around line 278-283: The `useSelector` call in `push-authenticator-form.tsx`
should use an explicitly typed selector function return type, and the selected
value should be allowed to be `undefined` instead of being annotated as
`number`. Update the selector in the `configuredMaxDeviceLimitUpperBound`
assignment to declare both the `state: AppState` parameter type and the selector
return type, then keep the `??` fallback in `maxDeviceLimitUpperBound` to handle
the `undefined` case cleanly.
- Around line 617-619: The onSubmit handler in PushAuthenticatorForm is using
Record<string, any> unnecessarily; replace that parameter type with
PushAuthenticatorFormInitialValuesInterface (or Record<string, unknown> if
needed) so the form callback avoids any and matches the existing cast in
handleFormSubmit. Keep the change localized to the onSubmit prop where the
values argument is declared.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: a9946e73-b575-4a23-a2b7-2b29d803b06f
📒 Files selected for processing (3)
features/admin.identity-providers.v1/components/forms/authenticators/push-authenticator-form.scssfeatures/admin.identity-providers.v1/components/forms/authenticators/push-authenticator-form.tsxidentity-apps-core/apps/authentication-portal/src/main/webapp/pushAuth.jsp
🚧 Files skipped from review as they are similar to previous changes (2)
- features/admin.identity-providers.v1/components/forms/authenticators/push-authenticator-form.scss
- identity-apps-core/apps/authentication-portal/src/main/webapp/pushAuth.jsp
44e3a8d to
1790139
Compare
ad8472d to
99c179e
Compare
Enable enrolling and managing multiple push devices per user, with device registration notifications and progressive enrollment. Feature visibility is gated via deployment config feature properties (console: identityProviders.properties, myaccount: security.properties) and a configurable maximum device limit.
225d44e to
7900686
Compare
This pull request introduces support for configurable device limits in the Push Authenticator feature, enabling dynamic control over how many devices a user can register for push-based multi-factor authentication. The changes span backend configuration, frontend API integration, UI logic, and new constants/models to support this feature.
Push Device Management Configuration Support
maxDeviceLimitUpperBound, to deployment configuration files (deployment.config.json.j2,deployment.config.json).pushDeviceMgtConfigsendpoint for fetching device management preferences.Frontend API and State Management Enhancements
Implemented a new API method
getConfigPreferencesto fetch push device management preferences from the backend, and introduced related TypeScript interfaces for request/response modeling.Added a React hook (
usePushAuthenticator) that loads device management config, exposes device limit state, and disables device enrollment when the limit is reached.UI/UX Improvements for Device Enrollment
Updated the Push Authenticator component to:
Constants and Validation
These changes collectively provide a robust, configurable, and user-friendly way to manage the number of devices a user can register for push-based MFA, with clear feedback and enforcement in the UI.
Related Issues