Skip to content

Conversation

@mrvanes
Copy link

@mrvanes mrvanes commented Feb 16, 2019

When consuming attributes from headers instead of env, multi-valued attributes are sometimes header name postfixed (_01, _02) or merged using a separator. The former can not easily be solved without extra logic, but the latter can be exploded using the separator character. The advantage is that everything will now be cast into an array, so that values can always be array_merged. Even multi separator-merged-multi-valued attributes will result in the correct multi-valued value this way.

This will solve #293

TODO: I lack the time and Nextcloud app development experience to introduce a separator setting in UI.
Please add one before merging this PR.
Just make sure to replace an empty separator "" with "\0" so that explode still works and strings aren't unexpectedly cut into smaller pieces. \0 should not naturally occur in strings.

When consuming attributes from headers instead of env, multi-valued attributes are sometimes header name postfixed (_01, _02) or merged using a separator. The prior can not easily be solved without extra logic, but the latter can be exploded using the separator character. The advantage is that everything will now be cast into an array, so that values can always be array_merged. Even multi separator-merged-multi-valued attributes will result in the correct multi-valued value this way.

This will solve nextcloud#293

TODO: I lack the time and Nextcloud app development experience to introduce a seperator setting in UI.
Please add one before merging this PR.
@tuxcrafter
Copy link

What lines can I add to the code to print some debug information.

My saml is passing an array of groups, but I only get one group in my user preferences?

[Tue Mar 12 23:45:11.871725 2019] [:error] [pid 3848] [12/Mar/2019:23:45:11]  DEBUG(ipsilon/providers/common.py:93 Redirect.debug()): saml2: jelledj's attributes: {'_groups': ['generalinformation', 'communications'], 'surname': 'de Jong', '_extras': {'sssd': {}}, 'groups': ['generalinformation', 'communications'], '_auth_type': 'password', 'givenname': 'Jelle', 'email': '[email protected]'}
[Tue Mar 12 23:45:11.873282 2019] [:error] [pid 3848] [12/Mar/2019:23:45:11]  DEBUG(ipsilon/providers/common.py:93 Redirect.debug()): saml2: value de Jong
[Tue Mar 12 23:45:11.874799 2019] [:error] [pid 3848] [12/Mar/2019:23:45:11]  DEBUG(ipsilon/providers/common.py:93 Redirect.debug()): saml2: value generalinformation
[Tue Mar 12 23:45:11.876352 2019] [:error] [pid 3848] [12/Mar/2019:23:45:11]  DEBUG(ipsilon/providers/common.py:93 Redirect.debug()): saml2: value communications
[Tue Mar 12 23:45:11.877856 2019] [:error] [pid 3848] [12/Mar/2019:23:45:11]  DEBUG(ipsilon/providers/common.py:93 Redirect.debug()): saml2: value Jelle
[Tue Mar 12 23:45:11.879394 2019] [:error] [pid 3848] [12/Mar/2019:23:45:11]  DEBUG(ipsilon/providers/common.py:93 Redirect.debug()): saml2: value [email protected]

Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going through the open PRs and a there is a bunch of dusty ones here 😅

Now the behaviour is quite different though and no callees of that method are adjusted. When dealing with different formats we should also have a set of unit tests for them.

@mrvanes
Copy link
Author

mrvanes commented May 6, 2025

This is so old I don't even have a requirement for it anymore, but to answer the question: the behaviour never changed so that any caller was confused. The change just resulted in a more consistent representation of multi-valued attributes. Back then, the change correctly let me supply multi-valued attributes as semi-column separated strings and then profit...

@blizzz
Copy link
Member

blizzz commented May 6, 2025

This is so old I don't even have a requirement for it anymore

Yeah, sorry about that.

Maybe I am missing some, but back then only a string was returned. But maybe that was the misbehaviour. Maybe the callees changed since; need to give it a more thorough look.

Nevertheless, thanks for the PR and for the responsivity.

@blizzz
Copy link
Member

blizzz commented May 13, 2025

Replacing this with a slightly different approach in #954. The logic was present elsewhere where built-in SAML mode was not affected.

@blizzz blizzz closed this May 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants