Add input validation for Activation Code and Document Number #415
Conversation
There was a problem hiding this comment.
Hi Tim, I'm a bit late with reviewing this code. I'm about to start upgrading MW to SF6.4 so I wanted to clear as many PR's as possible before having at that.
I'm not comfortable at merging your PR at this stage. I/We need to do some thorough testing and impact analysis before we can go ahead with this.
For now I did leave you some code review pointers. And be aware that I'm going to upgrade the MW. This includes removing the develop branch and renaming master -> main. So be aware of that too.
| ) { | ||
| if (!is_string($registrationCode)) { | ||
| throw InvalidArgumentException::invalidType('string', 'registrationCode', $registrationCode); | ||
| if (!preg_match('/^[A-Z0-9]{8}$/i', $registrationCode)) { |
There was a problem hiding this comment.
I love to have a more specific input validation here. It might be a good idea to introduce a registration code value object that is tasked with validating the code? That's more in line with the way the domain is set up. For now we kept it primitive because we did not realy bother with elaborate validation.
There was a problem hiding this comment.
If you want to go the extra mile, then please be my guest. I'm not going to solve the world's problems here, just the ones reported by our auditor ;)
Also not sure what you mean with 'more specific'
There was a problem hiding this comment.
In this case I think it's just a first guard on "sane" input as a defense-in-depth: does this even look like a registration code? The actual validity of the given code will be verified later obviously. So I can imagine that it's just fine to do the basic check that Tim implemented.
| { | ||
| return new self('—'); | ||
| return new self(null); |
There was a problem hiding this comment.
Does this not cause problems when an identitie is forgotten (right to be forgotten) and the entry pops up in the audit log? The value that is set for an unknown document number is displayed then and there in the audit log.
There was a problem hiding this comment.
This is really beyond my understanding... If you can get the regexp to work with this 'funny' character, we can leave this 'as-is'...
| { | ||
| if (!is_string($documentNumber) || empty($documentNumber)) { | ||
| if ($documentNumber === null) { | ||
| // Created using the static ::unknown method |
There was a problem hiding this comment.
Some work remains to be done here, also see my previous comment about a forgotten Identity.
There was a problem hiding this comment.
This is a defense in depth measure. Since we do not know what the format is of the document numbers – these are whatever the issuer decided to put on the document – retroactively setting a limit in the middleware in this way can cause issues when there are existing document numbers in the event stream that do not validate anymore. Limiting what is allowed in is fine, but this should be done on entry. We could do this in the middleware, but as a defense in depth it is best placed in the RA when the form post is received, not in the middleware.
SInce we do generate the activation codes, we know exactly what they look like so we could validate them in the middleware without additional risk.
There was a problem hiding this comment.
I did that at first, but Peter didn't want it there :-/
This PR:
Debatable:
nullfor unknown documents.Related: OpenConext/Stepup-RA#310
P.S.: There is a warning about an undefined method in the tests that is unrelated to this PR, but may need fixing by the maintainers. Should the code say
RecoveryTokenStatus::activeinstead?