Fix the password reset logic and error handling

[Razorfang]

These changes aim to fix the following issues that were found regarding password reset logic:

• Users navigating to the password reset page after receiving a reset code from an admin would cause said code to be replaced by a new one.
• Phone numbers with dashes were not being sanitized at any point before being used for attempted SMS's.
• Attempted error handling during SMS send failures would fail due to multiple redirect attempts.
• Sometimes users would receive multiple SMS messages during a single password reset attempt due to one being sent every time obtainsubject is hit.

These changes have been tested by myself on a dev instance. I believe they should fix these issues.

[vladimir-mencl-eresearch]

Hi @Razorfang ,

Thanks, this is solid piece of work.

The generating of the reset code upfront is exactly as planned.

(1) Somewhat wondering about how the number sanitization now works.
I see the code now also removes '-' that we specifically ran into (and previous code removes spaces).

Wondering whether we should perhaps provide a more general solution.

Common usage (in some countries) also includes parentheses - like (03) 364 1234

This is another part of what should cover what gets automatically fixed - so perhaps remove any characters in -(). (I have seen a notation with dots used in phone numbers too).

We should then perhaps reject anything that is NOT a phone number. I've tested and the current UI allowed me to add letters to my phone number ... which sounds wrong. However, instead of automatically correcting, this should be rejected.

How would a rule to:

• strip the "known notation characters" as per above
• reject if the remaining string is not just numbers
  sound to you?

Btw, neat trick to use the validMobileNumber method from the domain class also to sanitise existing data before use - nicely deals with existing data not having been subject to the validation upon entering.

(2) Nice trick with allowedMethods to block the HEAD requests from Outlook.

Thinking whether we should go a step further and require a POST action to actually send a message (requiring an explicit SEND button to be clicked), but this is definitely an improvement and a neat fix. Let's leave this part for later - just me thinking aloud, ignore this one.

Otherwise, great work, well done!

Cheers,
Vlad

[vladimir-mencl-eresearch]

Suggested change

	controllers.aaf.vhr.lostpassword.mobile.invalid=You cannot be sent a reset code because of an issue with your configured mobile number. Please contact an administrator to fix this.
	controllers.aaf.vhr.lostpassword.mobile.invalid=A reset code could not be sent to you because of an issue with your configured mobile number. Please contact one of our administrator to address this.

[Razorfang]

Latest commit addresses this.

[vladimir-mencl-eresearch]

This regular expression might be doing something different than what you intended: \\-( might translate to range from \ to (.

The syntax for [] is that a - should come right after the opening [ to be unambigous (not mean a range).

[Razorfang]

I'll update the expression and test it out.

[vladimir-mencl-eresearch]

Btw, the Regexp implementation is used from Java - so https://docs.oracle.com/javase/8/docs/api/java/util/regex/Pattern.html

[vladimir-mencl-eresearch]

I may now be nit-picking, but wondering whether to make diff (and resulting code) smaller by avoiding a new variable and instead of defining newNumber only to be used in one test, you could do:

   if (checkedNumber.replaceAll("[^0-9+]", '') != checkedNumber)

(Or possibly finding a shorter way to check if it contains any characters other than 0-9+)

But this is at code-style level - and also OK to leave as it is.

[Razorfang]

newNumber is used later on to assign to the class variable. I think your suggestion means we'd need an extra call like checkedNumber = checkedNumber.replaceAll... later in the code.