zorn-v / nextcloud-social-login

GNU Affero General Public License v3.0
198 stars 137 forks source link

Group claim with dots in it #338

Closed marcojarjour closed 2 years ago

marcojarjour commented 2 years ago

Hi at all

First of all, thanks for the great plugin.

Now to my issue, the group claim I receive from my identity provider contains a dot (https://example.com/roles) and is therefore misinterpreted as a nested claim and not working correctly, I refer to the following line: https://github.com/zorn-v/nextcloud-social-login/blob/8d91045dee6c6d327090b3c78f556837274693ad/lib/Provider/CustomOAuth2.php#L89

Is there a workaround that I am not aware of?

Thanks in advance for your effort.

zorn-v commented 2 years ago

Currently such claims is not supported

marcojarjour commented 2 years ago

Thanks for your feedback, we could solve it with our identity provider, but it seems to be a common standard, as you can see in the JWT Example from the JWT Standard which is refered by the OpenID Connect Standard Section Additonal Claims

Unless i've missed something it would make sense from my point of view to adapt the app to allow such claims, if you're interested i'd be happy to see if i can work out a solution for this, but would appreciate your feedback.

zorn-v commented 2 years ago

but it seems to be a common standard

Where it described in followed docs ? Only as "example" ? "Additonal Claims" section only says

it is RECOMMENDED that collision-resistant names be used for the Claim Names

and I did not find "you should use url-like claims". I did not remember that some popular provider use such claims. AFAIR keycloak used nested claims for groups.

if you're interested i'd be happy to see if i can work out a solution for this

I think that complicate things (like allow to escape dots and preg_split or something) for this is not needed yet.

marcojarjour commented 2 years ago

sorry for my bad english, that was a misunderstanding, I just meant that it is also used in an example in the standard. I thought I knew it also from Microsoft, but I could not find an example, but I know it from Auth0.

I think that complicate things (like allow to escape dots and preg_split or something) for this is not needed yet.

Yes, it makes sense to keep it as simple as possible. I would have made the nesting configurable but if you think this is unnecessary feel free to close the ticket.

marcojarjour commented 2 years ago

I assume you are not interessted in my suggestion, therefore i close this issue. Do not hesitate to come back to me if am wrong.