-
Notifications
You must be signed in to change notification settings - Fork 220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Email OTP based verification at registration #720
base: master
Are you sure you want to change the base?
Conversation
Latest version of identity governance
Gastrojune14
add unit test
|
...tity.recovery/src/main/java/org/wso2/carbon/identity/recovery/IdentityRecoveryConstants.java
Outdated
Show resolved
Hide resolved
...tity.recovery/src/main/java/org/wso2/carbon/identity/recovery/IdentityRecoveryConstants.java
Show resolved
Hide resolved
...ery/src/main/java/org/wso2/carbon/identity/recovery/handler/UserSelfRegistrationHandler.java
Outdated
Show resolved
Hide resolved
...ery/src/main/java/org/wso2/carbon/identity/recovery/handler/UserSelfRegistrationHandler.java
Outdated
Show resolved
Hide resolved
...ery/src/main/java/org/wso2/carbon/identity/recovery/handler/UserSelfRegistrationHandler.java
Outdated
Show resolved
Hide resolved
String eventName) throws IdentityRecoveryException { | ||
|
||
boolean emailOTPenabled = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take this logic out of triggernotification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we need to take this out of the TriggerNotification method then we need to pass this as an additional parameter to the method. If that is OK, I can take this out of the method and add an additional parameter for this method.
...ery/src/main/java/org/wso2/carbon/identity/recovery/handler/UserSelfRegistrationHandler.java
Outdated
Show resolved
Hide resolved
...ery/src/main/java/org/wso2/carbon/identity/recovery/handler/UserSelfRegistrationHandler.java
Show resolved
Hide resolved
properties.put(IdentityRecoveryConstants.CONFIRMATION_CODE, code); | ||
} | ||
properties.put(IdentityRecoveryConstants.TEMPLATE_TYPE, | ||
if (emailOTPenabled) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check for the canhandle method to which events it can handle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method is used only for the self user registration purpose. So we do not have to remove this logic out of TriggerNotification method.
...so2.carbon.identity.recovery/src/main/java/org/wso2/carbon/identity/recovery/util/Utils.java
Outdated
Show resolved
Hide resolved
...so2.carbon.identity.recovery/src/main/java/org/wso2/carbon/identity/recovery/util/Utils.java
Outdated
Show resolved
Hide resolved
...so2.carbon.identity.recovery/src/main/java/org/wso2/carbon/identity/recovery/util/Utils.java
Outdated
Show resolved
Hide resolved
...so2.carbon.identity.recovery/src/main/java/org/wso2/carbon/identity/recovery/util/Utils.java
Outdated
Show resolved
Hide resolved
...so2.carbon.identity.recovery/src/main/java/org/wso2/carbon/identity/recovery/util/Utils.java
Outdated
Show resolved
Hide resolved
.../src/test/java/org/wso2/carbon/identity/recovery/signup/UserSelfRegistrationManagerTest.java
Outdated
Show resolved
Hide resolved
...ry/src/main/java/org/wso2/carbon/identity/recovery/connector/SelfRegistrationConfigImpl.java
Outdated
Show resolved
Hide resolved
...tity.recovery/src/main/java/org/wso2/carbon/identity/recovery/IdentityRecoveryConstants.java
Outdated
Show resolved
Hide resolved
/* ArrayOrder: Username, Userstore, Tenant domain, Preferred channel, Error message, Manage notifications | ||
internally, excepted channel */ | ||
return new Object[][]{ | ||
{username, TEST_USERSTORE_DOMAIN, TEST_TENANT_DOMAIN_NAME, EMAIL, "User with EMAIL as Preferred " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we have the same username
, TEST_USERSTORE_DOMAIN
, TEST_TENANT_DOMAIN_NAME
, they are not required to be passed from the data provider. They can be defined in the test method itself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a similar implementation for a test in lines 212 - 221. I just followed the same implementation for this one as well.
Proposed changes in this pull request
This PR adds a new configuration to the IS management console to enable the Email OTP based verification. For that the following changes are made.
- Added a new configuration called "ENABLE_EMAIL_OTP_VERIFICATION"
- Seperated the secretKey generation method based on the preferred Channel.
- Added unit test for the new methods introduced.
- Made some changes in the TriggerNotification method to get the template according to configuration.
When should this PR be merged
[Please describe any preconditions that need to be addressed before we
can merge this pull request.]
Follow up actions
[List any possible follow-up actions here; for instance, testing data
migrations, software that we need to install on staging and production
environments.]
Checklist (for reviewing)
General
Functionality
Code
Tests
Security
Documentation