Refactor: More extensible NameID validation. #37
Conversation
| * 1. Is the nameID in a GUID pattern? | ||
| * | ||
| * You can add different methods of validation via SAMLHelper's `updateNameIDValidation` extension point. | ||
| * An example can be found in {@see EmailNameIDValidationExtension}. |
There was a problem hiding this comment.
This doesn't exist here.
Will fix later, moved this from project code to this module then decided to chuck it in documentation here and not as an actual class.
|
|
||
| // OK IF: One of the registered extensions returns true. | ||
| $validationResults = $this->extend('updateNameIDValidation', $nameID, $nameIDFormat); | ||
| if ($validationResults && is_array($validationResults)) { |
There was a problem hiding this comment.
I'm not sure this is within convention. Usually it takes just one extension to object for the result to fail, not for just one to pass.
This way we are most cautious and we encourage implementors to be cautious too. There's false for hard fail, null for no opinion and true for pass. We should aim to have no false and at least one true.
There was a problem hiding this comment.
Good point. I rarely call extend since in non-module code extend is a little overkill for design.
I think I just snatched a code-snippet from SS somewhere when I wrote this for project code.
My opinion is still that there should be no GUID validation at all (maybe apart from the varchar length). If I ever need to revisit our SAML implementation in project I'll try and finish this PR to upstream our changes. For now I think I'm going to close this due to time. Sorry!
In summary:
Member'sGUIDfield.SAMLConfiguration'svalidate_nameid.validate_nameidis enabled validation will now be done via extensions.validate_nameidto be set to true to use.I'm still of the opinion that GUID (or any) validation by default is a little out of place, When I tested the following with Azure AD it used user.userprinciplename with Email format by default. It might be a nice middle-ground to at least have something a little more generic and extensible. ADFS is too dinosaur and mirosofty for me to even think about 😁. More than happy to enable the validation by default if wanted.
Is there a way I can grab a DBField's size (arguments) without creating a singleton? Grabbing it from config or via DOschema sounds neater but I couldn't find the right API.