Closed pradtke closed 8 years ago
Thanks for the PR, I think it will be useful feature. I made some tests, with extreme config parameters, and I have two remarks:
email
) into the attributesWithScopeSuffix
config array as well as to the attributesWithScope
, then the email
with subdomain scope will be filtered out before the part of the process which deals with the suffixes@
sign, it is not filtered out, but IMHO it should be.What do you think?
re: put attribute name in multiple config options.
I added some clarifications to the README about this likely being a user configuration error. I thought of doing a code check (and logging a warning) if an attribute name was in multiple options, but that seemed like unnecessary overhead on each invocation.
re: multiple '@'. I agree. I added test cases and adjusted the regex to handle. I split the regex into 2 since a combined one was too difficult for me to read :)
Lastly, should a scoped attribute value of @scope
pass or be rejected by the attributesWithScope
and attributesWithScopeSuffix
filters? Currently it makes it through both.
Lastly, should a scoped attribute value of
@scope
pass or be rejected by theattributesWithScope
andattributesWithScopeSuffix
filters? Currently it makes it through both.
These values should be rejected.
Other parts of the PR are clear, thanks.
I had some time to circle back to this. I updated the PR to filter scoped attributes that do not have data before the '@'.
Thanks, it is fine, I have done the merge.
We have a use case for checking attributes against a subdomain form of the scope. Example if the scope is 'example.com' then we want to accept (for a mail attribute)
user@example.com
,user@department.example.com
, etc. This might be generally useful so I'm creating a PR. If you think the use case is too specific to us (and don't want the PR) that is fine too :)