Closed kousu closed 10 months ago
Currently this only reads Discord role IDs, not their names, which makes them very verbose in the UI:
Obviously I would like it to read their names instead. For that, I believe it needs to call [GET/guilds/{guild.id}/roles, but for that I fear that it probably requires a. creating a Bot User, b. inviting that Bot User to the target guilds, c. giving the Bot User's token to this plugin d. using the Bot User's token to call that API, all of which is quite a bit more complicated than the status quo.
It might be good enough to just impose a group mapping like @pktiuk suggested; it'll be annoying for the admin to set it up the first time and every time they add a new role ((I have no idea how to find out the role IDs from the Discord UI, they just don't appear anywhere)), but it'll be stable at least.
Currently this reads ALL roles from ALL guilds someone is in. It should probably be restricted to just the guilds in the $allowedGuilds list.
Also, I'm not sure if roles have globally unique IDs across Discord or not? What happens if there's an ID collision between two guilds? Do we need to prefix the roles with the guild ID to make sure they're unique? That sounds...even worse.
I think, also, this patch belongs better in HybridAuth. But I tried patching the vendored copy of it under 3rdparty and couldn't get it to work, until I learned from https://github.com/zorn-v/nextcloud-social-login/commit/ef5e24c192929d7fa8d7260f1a8dab278d7d09d2 that lib/Service/ProviderService.php overrides a lot of its settings so that patching it wouldn't work anyway. I still think that's a good idea, but it will require some coordination and deeper thought -- and like, say, not everyone is going to want to use it, which means the guilds
and guilds.members.read
scopes need to be optional.
I've got group mapping protoyped. But I didn't take the time to build a whole UI for it, so so far all I have is this patch, which I'm not going to attach to the branch because it's too specific to my use case, but I want to record it here anyway because it shows what needs doing:
diff --git a/lib/Service/ProviderService.php b/lib/Service/ProviderService.php
index 5e737f3..011332e 100644
--- a/lib/Service/ProviderService.php
+++ b/lib/Service/ProviderService.php
@@ -423,6 +423,15 @@ class ProviderService
// TODO: /member returns roles as their ID; to get their name requires an extra API call
// (and perhaps extra permissions?)
}
+
+ // HACK: because there's no UI for mapped groups under the Discord connector like there is under OIDC or Discourse
+ // code in our mapping here
+ $profile->data['group_mapping'] = array(
+ '5916019825232292399' => "admin",
+ '3908310574390159257' => "Stars",
+ '1647820633687053577' => "Coordinators",
+
+ );
}
if (!empty($config['logout_url'])) {
This works! I can assign/unassign people roles in Discord, and the next time they log in (which I can force with NextCloud's "Wipe all devices" option in the /settings/users page) their groups will be updated. And with "Restrict login for users without mapped groups" turned on this means that only the roles listed there are allowed to log in, giving me some peace of mind.
Some important points:
the keys in "group_mapping" are the strings as given by Discord; they're not "discord-XXXXXXXX" which
Yep, mapping assumes "exact foreign group -> exact NC group".
nextcloud-social-login still creates groups named "discord-XXXXXXXX"; I wish I could turn that off
Just disable auto_create_groups
option
the values are the names of the NextCloud groups; and you have to be careful here, these are the original names of the NextCloud groups; if you "rename" a NextCloud group it doesn't actually rename it, it just adds a label overtop that masks the internal name
There is gid
and displayname
for groups. Long time displayname
just not used. Seems it is changed.
@kousu
Maybe I will wait with this testing until you will apply suggestions of zorn-v
@pktiuk I did not add any useful information (almost), soo )
Just disable
auto_create_groups
option
This is useful! And obvious in retrospect but I missed it. I assumed mapped groups would count as their target names.
@pktiuk
Maybe I will wait with this testing until you will apply suggestions of zorn-v
There's no code changes to make from this yet :face_with_spiral_eyes: I haven't added UI to set up the group mappings yet. For my team, I edited group_mapping
in directly into the code just to see if I understood how it worked, but that's not part of the branch you can use.
Looking ahead, I'm stumped on how to do the workflow for adding new mapped groups. With "Custom Discourse" the UI looks like this empty free-form box:
Which is maybe fine for Discourse where the group names are obvious, but with Discord the group names are ID numbers, and as far as I can, the only way to get those numbers are to use the API:
The former works with the plugin's current basic OAuth2 setup, but the latter, I think, requires additionally creating a bot user and inviting it to each guild you might want to use it with. Which just seems like it greatly complicates the setup process.
Currently I have been relying on auto_create_groups
and, tbh, tailing my logs, to find out what the Discord role IDs I should be looking for are. It's not been a good time.
When will the feature be integrated into the official version?
Yeah, I want this too
When will the feature be integrated into the official version?
Unfortunately there's no timeline :( . It has no UI yet and what I do have written needs a lot more testing. If you want to speed it along, cheer me on, help me test, and if you want to learn a bit of PHP or Angular, read https://nextcloud.com/developer/ and give me a hand getting the code over the finish line.
Obviously I would like it to read their names instead.
Note to self: I bet the giant username migration is going to either simplify or complexify this whole bit.
no, I confused myself; this PR is about groups and their names, not users and their names. And mapping role IDs to named groups ( https://github.com/zorn-v/nextcloud-social-login/pull/395#issuecomment-1668357856 ) is a 99% better solution than trying to put strings from the Discord API into NextCloud.
When will the feature be integrated into the official version?
Unfortunately there's no timeline :( . It has no UI yet and what I do have written needs a lot more testing. If you want to speed it along, cheer me on, help me test, and if you want to learn a bit of PHP or Angular, read https://nextcloud.com/developer/ and give me a hand getting the code over the finish line.
I would help test too if you need. I need this for my project
or Angular
angular sucks, learn vue :smile:
Currently I have been relying on
auto_create_groups
and, tbh, tailing my logs, to find out what the Discord role IDs I should be looking for are. It's not been a good time.
@FabiChan99 taught me something! Discord has a Developer Mode under User -> Settings -> Advanced
and with this enabled, you can get your Role IDs out by right clicking on them under Server -> Settings -> Roles
That also gives me a good idea to at least get this to a usable state: the "Custom OIDC" and "Custom OAuth2" backends have this "Add Group Mapping" button which expects you to know the OIDC/OAuth2 Group ID strings:
If I can teach people how to turn on Discord Developer Mode then I can copy this UI and have them paste in the Role IDs the same way. :tada: Once I get that working I'll undraft this and ask it to be merged.
I would really like to have a button that pulls the Discord roles from their API, but that requires bouncing through their OAuth flow so it's more complicated than I want to think about right now. That can be a follow up.
I've got it working! @SDS1234, @FabiChan99, @pktiuk, please test and let me know what you think. It shouldn't break anything -- it should be safe to switch between this one and the official release at any time.
To try this out (from scratch, to give you a complete picture):
sociallogin
from your NextCloud instance's app manager. You need to be a NextCloud admin to do this.cd path/to/nextcloud/
; to make sure you're in the right place, check that ls -l apps/sociallogin/
shows you files -- they should be the same as you can see at https://github.com/zorn-v/nextcloud-social-login/mv sociallogin sociallogin.official.disabled && git clone -b discord-roles https://github.com/kousu/nextcloud-social-login sociallogin
php occ upgrade
-- I'm not sure this is always necessary but it was for me when NextCloud noticed sociallogin
had been changed (this is to let the plugin run new migrations. But I haven't added any migrations so it shouldn't change anything.)Go to Discord, User -> Settings -> Advanced, and turn on Developer Mode to enable the next step
For each Discord role you want to map in
Create a group in your NextCloud server
Go to Discord -> Server -> Settings -> Roles -> find the role -> Copy Role ID
Go to NextCloud -> Settings -> Admin Settings -> Social Login -> Discord -> Add Group Mapping. Paste in the Role ID and pick the matching NextCloud group
Click Save so that you see
Settings for social login successfully saved
Test it out by
logging in with Discord
checking Settings -> Personal Settings to see if your groups got picked up. With the default settings only mapped groups will be saved; all others will be forgotten each time you log in.
To switch back to the official release:
mv sociallogin sociallogin.discord-roles && mv sociallogin.official.disabled sociallogin
php occ upgrade
I'll take a look later (and maybe make some refactoring). In vacations now :smiley:
I have one question linked with implementation of this functionality.
Are these groups assigned only after the first time login, or are they updated after every login?
Are these groups assigned only after the first time login, or are they updated after every login?
They're updated every login. There's a setting to tune it, but by default it does what you want:
I will update today and test it with my organisation
I updated my own today finally (replacing the one from my git branch with the one from the app store) and it's looking good!! Thanks for fixing the CSS on that button @zorn-v.
I also appreciate that the default providers are now all hidden by default unless in use!!
Fixes https://github.com/zorn-v/nextcloud-social-login/issues/390