Add API client functions for RecoveryAPI V2 and on demand migration for new configs. #5744

RushanNanayakkara · 2024-06-13T05:04:00Z

Proposed changes in this pull request

-Adds the API client functions for RecoveryAPI V2.

Add on demand migration implementation for newly introduced configs under "SMS OTP For Password Recovery" feature.
Other improvements required for the same feature.

Related Issues

Related PRs

When should this PR be merged

Should be merged along with all other related PRs for the feature since there are inter dependencies.

Follow up actions

Test if users who have had the password recovery option enabled have the feature enabled after the version upgrade.

Checklist (for reviewing)

General

Is this PR explained thoroughly? All code changes must be accounted for in the PR description.
Is the PR labeled correctly?

Functionality

Are all requirements met? Compare implemented functionality with the requirements specification.
Does the UI work as expected? There should be no Javascript errors in the console; all resources should load. There should be no unexpected errors. Deliberately try to break the feature to find out if there are corner cases that are not handled.

Code

Do you fully understand the introduced changes to the code? If not ask for clarification, it might uncover ways to solve a problem in a more elegant and efficient way.
Does the PR introduce any inefficient database requests? Use the debug server to check for duplicate requests.
Are all necessary strings marked for translation? All strings that are exposed to users via the UI must be marked for translation.

Tests

Are there sufficient test cases? Ensure that all components are tested individually; models, forms, and serializers should be tested in isolation even if a test for a view covers these components.
If this is a bug fix, are tests for the issue in place? There must be a test case for the bug to ensure the issue won’t regress. Make sure that the tests break without the new code to fix the issue.
If this is a new feature or a significant change to an existing feature? has the manual testing spreadsheet been updated with instructions for manual testing?

Security

Confirm this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets.
Are all UI and API inputs run through forms or serializers?
Are all external inputs validated and sanitized appropriately?
Does all branching logic have a default case?
Does this solution handle outliers and edge cases gracefully?
Are all external communications secured and restricted to SSL?

Documentation

Are changes to the UI documented in the platform docs? If this PR introduces new platform site functionality or changes existing ones, the changes should be documented.
Are changes to the API documented in the API docs? If this PR introduces new API functionality or changes existing ones, the changes must be documented.
Are reusable components documented? If this PR introduces components that are relevant to other developers (for instance a mixin for a view or a generic form) they should be documented in the Wiki.

… from the RecoveryAPI V2"" This reverts commit 6111d57.

jenkins-is-staging · 2024-06-17T12:18:19Z

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/9547516936

jenkins-is-staging · 2024-06-17T14:35:11Z

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/9547516936
Status: failure

jenkins-is-staging · 2024-06-18T02:14:27Z

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/9557845940

jenkins-is-staging · 2024-06-18T04:15:30Z

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/9557845940
Status: failure

bhagyasakalanka · 2024-06-18T19:56:42Z

....util/src/main/java/org/wso2/carbon/identity/mgt/endpoint/util/client/api/RecoveryApiV2.java

+        final String localVarAccept = apiClient.selectHeaderAccept(
+                new String[]{FrameworkConstants.ContentTypes.TYPE_APPLICATION_JSON});
+        final String localVarContentType = apiClient.selectHeaderContentType(


Do we really need to define two different variables for localVarAccept and localVarContentType?

Imo having these two in two variables makes the code clearer opposed to having this inside the parameter list of invokeAPI function call.

...t/org.wso2.carbon.idp.mgt/src/main/java/org/wso2/carbon/idp/mgt/IdentityProviderManager.java

bhagyasakalanka · 2024-06-18T20:06:23Z

...-mgt/org.wso2.carbon.idp.mgt/src/main/java/org/wso2/carbon/idp/mgt/dao/IdPManagementDAO.java

+
+        // Enable all recovery options when Recovery.Notification.Password.Enable value is set as enabled.
+        // This keeps functionality consistent with previous API versions for migrating customers.
+        idpProperties.stream().filter(idp -> idp.getName().equals(EMAIL_LINK_PASSWORD_RECOVERY_PROPERTY)).findFirst()


Shall we switch the attributes order in with equals method to prevent possible null pointers

Check other places as well

Addressed with f5e241a

bhagyasakalanka · 2024-06-18T20:09:53Z

...-mgt/org.wso2.carbon.idp.mgt/src/main/java/org/wso2/carbon/idp/mgt/dao/IdPManagementDAO.java

@@ -121,6 +121,14 @@ public class IdPManagementDAO {
    private static final Log log = LogFactory.getLog(IdPManagementDAO.class);

    private static final String OPENID_IDP_ENTITY_ID = "IdPEntityId";
+    private static final String NOTIFICATION_PASSWORD_ENABLE_PROPERTY


Can we move these variables to a constant class and refer every places these are used. I can see few places where we have defined these as private variables.

Moved to IdPManagementConstants with f5e241a

bhagyasakalanka · 2024-06-18T20:10:21Z

...gt/org.wso2.carbon.idp.mgt/src/main/java/org/wso2/carbon/idp/mgt/util/IdPManagementUtil.java

 import static org.wso2.carbon.idp.mgt.util.IdPManagementConstants.PRESERVE_LOCALLY_ADDED_CLAIMS;

 public class IdPManagementUtil {

    private static final Log log = LogFactory.getLog(IdPManagementUtil.class);

+    private static final String RECOVERY_NOTIFICATION_PASSWORD_PROPERTY = "Recovery.Notification.Password.Enable";


We should move these to a common constant class

Moved to IdPManagementConstants with f5e241a

bhagyasakalanka · 2024-06-18T20:14:29Z

...gt/org.wso2.carbon.idp.mgt/src/main/java/org/wso2/carbon/idp/mgt/util/IdPManagementUtil.java

+    private static final String RECOVERY_NOTIFICATION_PASSWORD_PROPERTY = "Recovery.Notification.Password.Enable";
+    private static final String EMAIL_LINK_PASSWORD_RECOVERY_PROPERTY
+            = "Recovery.Notification.Password.emailLink.Enable";
+    private static final String SMS_OTP_PASSWORD_RECOVERY_PROPERTY = "Recovery.Notification.Password.smsOtp.Enable";


Have we added these configs to identity.xml, identity.xml.j2 and default.json files?

No. These are governance configs which are stored in the database as resident IDP properties.

bhagyasakalanka · 2024-06-20T04:32:21Z

...ava/org/wso2/carbon/identity/mgt/endpoint/util/client/model/recovery/v2/ConfirmResponse.java

+    @Override
+    public String toString() {
+
+        return "class RecoveryResponse {\n" +


Can we use the approach use in other response classes to implement this toString method? using a StringBuilder

Addressed with 8132e60

bhagyasakalanka · 2024-06-20T04:33:52Z

features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2

@@ -4080,4 +4080,10 @@
    </CacheInvalidator>
    {% endif %}

+    <ConfigSwitching>


As a practice, we add these configs to identity.xml file as well. Shall we add this to identity.xml file?

Addressed with 8132e60

RushanNanayakkara added 6 commits June 13, 2024 05:33

Revert "Revert "Enhance RecoveryApiV2 Client to add missing functions…

2590202

… from the RecoveryAPI V2"" This reverts commit 6111d57.

update on demand migration

ced1e7a

Check password recovery with connector status

df47e2d

Move password recovery config validation to utils.

2ad1f73

Remove depricate tag from checkUsernameRecovery function

688e140

Bug fixes and improvements

4eab6ae

This was referenced Jun 18, 2024

Validate Password Recovery Connector Configuration Updates wso2/identity-api-server#621

Merged

Sms otp pw recovery wso2-extensions/identity-governance#824

Merged

bhagyasakalanka reviewed Jun 18, 2024

View reviewed changes

...t/org.wso2.carbon.idp.mgt/src/main/java/org/wso2/carbon/idp/mgt/IdentityProviderManager.java Outdated Show resolved Hide resolved

bhagyasakalanka reviewed Jun 18, 2024

View reviewed changes

Address comments

f5e241a

bhagyasakalanka reviewed Jun 20, 2024

View reviewed changes

RushanNanayakkara added 2 commits June 20, 2024 11:03

Address comments

8132e60

Address config name comments

3e8b0d1

bhagyasakalanka approved these changes Jun 20, 2024

View reviewed changes

RushanNanayakkara merged commit 0db8132 into wso2:master Jun 25, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add API client functions for RecoveryAPI V2 and on demand migration for new configs. #5744

Add API client functions for RecoveryAPI V2 and on demand migration for new configs. #5744

RushanNanayakkara commented Jun 13, 2024 •

edited

Loading

jenkins-is-staging commented Jun 17, 2024

jenkins-is-staging commented Jun 17, 2024

jenkins-is-staging commented Jun 18, 2024

jenkins-is-staging commented Jun 18, 2024

bhagyasakalanka Jun 18, 2024

RushanNanayakkara Jun 19, 2024

bhagyasakalanka Jun 18, 2024

bhagyasakalanka Jun 18, 2024

RushanNanayakkara Jun 19, 2024

bhagyasakalanka Jun 18, 2024

RushanNanayakkara Jun 19, 2024

bhagyasakalanka Jun 18, 2024

RushanNanayakkara Jun 19, 2024

bhagyasakalanka Jun 18, 2024

RushanNanayakkara Jun 19, 2024 •

edited

Loading

bhagyasakalanka Jun 20, 2024

RushanNanayakkara Jun 20, 2024

bhagyasakalanka Jun 20, 2024

RushanNanayakkara Jun 20, 2024

Add API client functions for RecoveryAPI V2 and on demand migration for new configs. #5744

Add API client functions for RecoveryAPI V2 and on demand migration for new configs. #5744

Conversation

RushanNanayakkara commented Jun 13, 2024 • edited Loading

Proposed changes in this pull request

Related Issues

Related PRs

When should this PR be merged

Follow up actions

Checklist (for reviewing)

General

Functionality

Code

Tests

Security

Documentation

jenkins-is-staging commented Jun 17, 2024

jenkins-is-staging commented Jun 17, 2024

jenkins-is-staging commented Jun 18, 2024

jenkins-is-staging commented Jun 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RushanNanayakkara Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RushanNanayakkara commented Jun 13, 2024 •

edited

Loading

RushanNanayakkara Jun 19, 2024 •

edited

Loading