zorn-v / nextcloud-social-login

GNU Affero General Public License v3.0
199 stars 138 forks source link

Adding an email claim for Custom OpenIDConnect #425

Open TBG-FR opened 1 year ago

TBG-FR commented 1 year ago

This plugin provides a lot of really useful features around SSO. However, it is possible to edit the claim to customize the displayname, but not the email. In some specific cases, we don't have / don't want to use the field email, but another one 😅

Would it be possible to add a new claim for this ? And if it's empty, still use email key I think it's quite simple, with something like this, but I may be missing some files, I don't know the codebase as wall as you 😉

ProviderService.php

  'displayname_claim' => 'displayNameClaim',
  +'email_claim' => 'emailClaim',

CustomOpenIDConnect.php

$displayNameClaim = $this->config->get('displayname_claim');
+$emailClaim = $this->config->get('email_claim');

$userProfile->photoURL    = $data->get('picture');
-$userProfile->email       = $data->get('email');
+$userProfile->email = $data->get($emailClaim) ?: $data->get('email');

if (!$userProfile->email) {
-  $userProfile->email = $profile->get('email');
+  $userProfile->email = $profile->get($emailClaim) ?: $profile->get('email')
 }

Many thanks in advance !

zorn-v commented 1 year ago

I think it's quite simple

Simple is not mean good idea :wink: Bloat interface for some unclear reason ? Just because I can ?

TBG-FR commented 1 year ago

Let me get this straight then :

The idea is only to provide as much customization as possible to people using the plugin, not to add things "because you can"

If you really think that's a bad idea, I would kindly ask you to tell me what would be missing to implement it, and we'll edit directly the plugin on our instance, leaving you in peace.

zorn-v commented 1 year ago

Why only email then ? I am not fan of "customizing everything" as interface becomes "spaceship dashboard"

TBG-FR commented 1 year ago

Because for now you can customize some other fields (roles for example), but not email... But you're right, it could also be a "mapping" thing to map any Provider field to its corresponding Nextcloud field

I understand. However, email is an important field (can be used to send important notifications or system messages) and for now we can't get it right on Nextcloud because of that "restriction", while on the other products the oidc plugins allow us to do so 😅

zorn-v commented 1 year ago

If you have only one provider, you can try other app https://github.com/pulsejet/nextcloud-oidc-login Maybe in the future I will come up with something about mapping fields.

TBG-FR commented 1 year ago

Your plugin is the only one that handles roles and groups mapping in a good way 😅 I think I tried it at first but it wasn't enough for what we needed

Alright, thanks for the answer. May I still ask you what would be missing in my code above to implement it on my side ?

zorn-v commented 1 year ago

May I still ask you what would be missing in my code above to implement it on my side ?

You should also add field on frontend side. And build it of course - npm i && npm run build