MultiValued attributes / implement proc filters

[DeltinoRosso]

What Happened ?

https://github.com/catalyst/moodle-auth_saml2/blob/04044eabec6bf0841d8f11f5a0da4abed53ae883/auth.php#L676

In the referenced line of the code, if multiple values are passed by the IdP are automatically discarded because always the first returned value is taken.

What you expected Would be nice to have support for multi valued attributes coming from the IdP.

[jtsafran]

+1 for this!

We have a client at eThink/Open LMS that would be interested in knowing the cost of development to have this feature added. Can you let me know the best way to get in touch?

Thanks, Jesse

[danmarsden]

@jtsafran - see link in readme to our website/contact forms: https://github.com/catalyst/moodle-auth_saml2#support

[brendanheywood]

I'm curious what the actual desired end result is: If an attribute has say 5 values, then do you want some config to concatenate them together with a delimiter? Are you trying to push data into a custom field like one of these:?

https://moodle.org/plugins/customfield_dynamic

https://moodle.org/plugins/customfield_multiselect

[jtsafran]

@brendanheywood The use case for this client is getting the list of groups from the AD memberOf attribute concatenated and added to a plaintext field so we can add the user to a Totara audience where the user is in a specific group (custom profilefield contains ). For example, the attribute passed from AzureAD/ADFS would be something like:

` ["memberOf"]=>

array(5) {

[0]=>

string(40) "CN=group1,OU=GroupsOU,DC=SOMEDOMAIN,DC=COM"

[1]=>

string(40) "CN=group2,OU=GroupsOU,DC=SOMEDOMAIN,DC=COM"

[2]=>

string(40) "CN=group3,OU=GroupsOU,DC=SOMEDOMAIN,DC=COM"

[3]=>

string(40) "CN=group4,OU=GroupsOU,DC=SOMEDOMAIN,DC=COM"

[4]=>

string(40) "CN=group5,OU=GroupsOU,DC=SOMEDOMAIN,DC=COM"

} `

and the resulting string/profile field would contain something like:

CN=group1,OU=GroupsOU,DC=SOMEDOMAIN,DC=COM|CN=group2,OU=GroupsOU,DC=SOMEDOMAIN,DC=COM|CN=group3,OU=GroupsOU,DC=SOMEDOMAIN,DC=COM|CN=group4,OU=GroupsOU,DC=SOMEDOMAIN,DC=COM|CN=group5,OU=GroupsOU,DC=SOMEDOMAIN,DC=COM

[brendanheywood]

Ok, well at its simplest there could be a new admin config item for a delimiter and it will concat all attributes instead of grabbing the first if it is set. Thats probably a dozen lines of code, and be safe and backwards compatible. Pull requests welcome for that.

The next level up in complexity would be if you needed different delimiters for different fields, and deciding how to escape the delimiters if needed.

[jtsafran]

@brendanheywood The first option would be fine here.

Which office do I need to reach out to on the contact page?

[brendanheywood]

@jtsafran the sydney one is good but any of them would have been fine:

https://catalyst-au.net/contact-us/sydney#email-us

[dmitriim]

I feel like this should be implemented in a way to be able to enable/disable concatenating per field. E.g. have an extra setting in mapping section for each field and one global concatenating option. Or have a dropdown in mapping section against each field where you can pick one of the predefined (hardcoded) concatenating options or leave it disabled.

[brendanheywood]

The problem is the field mappings is managed by core so we can't touch them.

A bunch of ideas in rough order of complexity:

1) a new admin setting for a delimiter which defaults to an empty. If present it concats all multi valued fields.

2) the above, plus a second optional admin setting which if present contains a list of fields that this logic should be applied to, one per line in a textarea.

So using the example above and assuming we only wanted that single field processed the it would just be

memberOf

3) A slightly more succinct and more flexible way, but a bit more obscure, might be to combine both fields into a textarea which them maps to the delimiter. This means it's only one field, it defaults off, and you could use different delimiters if you want. The only trick is using a good delimiter for the config item itself which is unlikely to ever be in an attribute name. I think the arrow notation is pretty unambiguous:

memberOf => |

4) We step right back and forget about delimiters for a second and think more abstractly about simply processing the attributes data in a variety of ways. simplesaml already has a good filter concept, and we already allow a local plugin to implement a hook to insert filters into the flow to mutate the data but this needs to be done in code:

https://github.com/catalyst/moodle-auth_saml2/blob/eff8f823e6b476aee39544e6ab7d31d8bf1b0546/classes/api.php#L72

Perhaps the best way is we also allow auth proc filters to be defined in config. This would be limited to only the core filters that ship with simplesamlphp, and it has a filter for exactly this purpose: core:CardinalitySingle

https://simplesamlphp.org/docs/latest/core/authproc_cardinalitysingle.html

So this would be super powerful and extremely flexible. The full list of core auth proc filters is here:

https://simplesamlphp.org/docs/latest/simplesamlphp-authproc.html

In this case the config would look like this as raw php SSP config:

51 => [
    'class' => 'core:CardinalitySingle',
    'flatten' => ['memberOf'],
    'flattenWith' => '|',
]

If possible it would be nice to keep the syntax so all the SSP docs and examples work verbatim. But the one huge caveat is that in simplesaml this is raw config in literal php code, so we'd either have to figure out to safely parse that without eval, or convert it to json instead:

{
    "51": {
        "class": "core:CardinalitySingle",
        "flatten": ["memberOf"],
        "flattenWith": "|"
    }
}

I'd like a lot of opinions one way or the other on the options above. Overall I'm more inclined to see either something very simple: 1, or something very flexible and powerful ie 4, over something in the middle, and I'm honestly leaning more towards 4. In the early days I was attempting to abstract away and not be tightly coupled to the SSP external api / config but that ship has already sailed long ago. The only other downside is just admin complexity and I think a few example recipes in the readme along with a good debugging tools by adding the pre filter and post filter states to the testing page and I think that's quite reasonable.

(Important note: we'd also have to disable this core proc filter as it would open up an eval based security hole: https://simplesamlphp.org/docs/latest/core/authproc_php.html )

[jtsafran]

@brendanheywood While option 1 would fit the use case we need, I have to agree option 4 sounds like the best option to make this robust as possible.