-
Notifications
You must be signed in to change notification settings - Fork 17
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
Provide ability to update email with confirmation #188 #192
base: epic/cognito/develop
Are you sure you want to change the base?
Conversation
userdetails-plugin/grails-app/views/registration/createAccount.gsp
Outdated
Show resolved
Hide resolved
Everything looks fine. |
…re/update_email # Conflicts: # userdetails-plugin/grails-app/views/registration/createAccount.gsp
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's a lot of custom validation going on in jquery, feels like this could be done with the validation engine thing that's already in place as well?
application.a13=For any questions or technical assistance, please contact our dedicated support team at <a href="mailto:[email protected]">[email protected]</a>. | ||
update.email=Update your email | ||
update.email.desc.1=Please enter your new email to update the email. The new email will receive a code to which needs to be submitted to verify the new email address. |
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.
This should be moved into the userdetails-gorm/userdetails-cognito messages.properties with the same key, then remove the if branch in the createAccount.gsp below.
<div id="newEmailDiv"> | ||
<p id="newEmailMessage" hidden></p> | ||
<input id="newEmail" name="newEmail" type="text" class="form-control" data-validation-engine="validate[required]"/> | ||
<g:if test="${grailsApplication.config.getProperty('userdetails.cognito.auth', boolean, 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.
I would prefer this to be a feature based check rather than a cognito check - ie userdetails.activation.enabled
, userdetails.activation.manual: true
or similar, then remove userdetails.cognito.auth
from properties (applicationForm will need to change too, use userdetails.applications.insecure-urls-allowed
or similar)
<g:if test="${edit}"> | ||
<h2><g:message code="update.email" /></h2> | ||
<p> | ||
<g:if test="${grailsApplication.config.getProperty('userdetails.cognito.auth', boolean, 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.
Remove this as per above comment
} | ||
else { | ||
$.ajax({ | ||
url: "${createLink(uri:'/registration/update')}?email=" + newEmail, |
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.
newEmail probably needs to be encoded?
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.
Or use jquerys $.ajax({data: ...})
$("#requestCode").click(function(e) { | ||
var newEmail = $("#newEmail").val(); | ||
if(newEmail == null || newEmail === "") { | ||
document.getElementById("newEmailMessage").innerHTML = "Invalid email" |
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.
All these inner HTML messages should be i18n
window.location = "${createLink(uri:'/logout')}?url=/" | ||
} | ||
else{ | ||
document.getElementById("newEmailMessage").innerHTML = result.error |
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.
Potential XSS issue? If the userservice exception reflects the user input code or something then this is a problem.
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.
This is why I prefer to never touch innerHTML from jquery.
} | ||
else { | ||
$.ajax({ | ||
url: "${createLink(uri:'/registration/update')}?email=" + newEmail, |
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.
Encode newEmail
let error = "A user is already registered with the email address" | ||
if(result.success) { | ||
$.ajax({ | ||
url: "${createLink(uri:'/registration/deactivateAccountAndSendActivationEmail')}?email=" + newEmail, |
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.
Encode new email
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.
Also, sending 2 requests for this seems wrong - the user could update their email then block the deactivateAccountAndSendActivationEmail call? Surely the deactivateAccountAndSendUserEmail should happen atomically on the server side with the update if required?
@@ -370,7 +394,7 @@ | |||
|
|||
$("#setupMFA").click(function(e) { | |||
$.ajax({ | |||
url: "${createLink(action:'getSecretForMfa', controller: 'Registration')}", | |||
url: "${createLink(uri:'/registration/getSecretForMfa')}", |
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.
Why change from action: controller: which should be stable under refactoring or URL Mappings changes to uri?
url: "${createLink(uri:'/registration/deactivateAccountAndSendActivationEmail')}?email=" + newEmail, | ||
type: "GET", | ||
success: function(result){ | ||
if(result.success) { |
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.
Seems to be some overlap with the validation of #requestCode
…re/update_email # Conflicts: # userdetails-plugin/grails-app/i18n/messages.properties
Fixed all the comments. |
No description provided.