As you probably can guess, I’m also keen to see a stable solution to this problem, which is not specific to the openid-connect plugin. Let’s see if we can figure out a way forward
To pick up on the point you’ve raised.
How do you decide which groups to add and remove the user from? And how does that affect manually adding/removing users from groups in Discourse itself?
a user authenticates via OIDC with groups ['group1', 'group2']
In the discourse UI, the user is added to group3
later, the same user authenticates via OIDC with groups ['group1']
Inspecting this as a human, we can see that the final state should be group1, group3 (group2 should be removed). But I don’t think there is enough state being tracked to make that decision programmatically. Similarly:
a user authenticates via OIDC with groups ['group1', 'group2']
In the discourse UI, the user is removed from group1
later, the same user authenticates via OIDC with groups ['group1', 'group2']
Now what should happen? An admin has explicitely removed the user from group1 , but OIDC just added them back. This is very confusing UX for the admin. We may need some way to identify groups as ‘externally managed’, and hide all the add/remove UI
I think there’s a few ways we could handle this issue (they’re non-exclusive)
Group and token attribute identification
In any implementation, explicit identification of
which groups can have their membership managed via authentication; and
which attributes in an auth token govern group membership
should be required, whether that be via site settings, group settings or otherwise, and default should be “off”.
Strict or permissive handling
Handling is “strict” if a user is removed when the group is absent from the identified token attribute. Handling is “permissive” if a user is not removed when the group is absent from the identified token attribute.
As you suggest, you could disable group membership addition / removal via the groups admin if the handling was “strict”.
Membership source identification
You could also possibly store the source of the group membership in the group_users table to allow for a “mixed” approach within a group, i.e. an admin can’t remove memberships created via auth token.
The more I think about this, the more I’m thinking this should be done via group settings.
Thanks @angus for getting this started! I know a lot of people are keen on this functionality!
Interesting! I had always imagined that groups would be auto-created during authentication, and that group name would be 1:1 with the group name on the identity provider. That’s how DiscourseConnect works at the moment.
But I actually like this more explicit option a lot! It means that people’s Discourse instances won’t be polluted by unneeded groups from their identity provider, and it means that admins can customize group names to their liking. It has a lot of parity with our existing email-domain-based membership
This sounds good from a technical point of view. My only worry is that it could be difficult to explain to users/admins. If we go with your idea of ‘explicit identification’ of auth-managed groups, I think we could just implement the ‘strict’ handling?
When a group is configured to be auth-managed, manually adding/removing is hidden behind a big warning, and it behaves like the ‘strict’ mode you described.
How does that sound?
Aside: we should also make sure that all automatic adding/removing of users is recorded in the group log. That will make it easier for everyone to understand what is happening and why.
Yeah, I feel it’s better to be explicit, for v1 of this feature set at least. I haven’t come across a use case yet where auto-creation has been really needed, i.e. the group couldn’t just be set up and configured by the site admin prior to any claims being handled.
Perhaps we could move to auto-creation as an option after we do the explicit version?
In terms of Group Settings, it would be something like
Settings Section: Membership (i.e. the existing section) Settings Group Title: Authentication Management Settings:
Service: List of authentication services. “All” would be an option. This setting would also function as an “enabled / disabled” state for this feature set. i.e. the default would be “None”.
Claim: text input to identify id token claim. The “description” for this feature (perhaps in a post here on meta) would explain the supported formats, e.g. boolean, comma-delineated string etc.
Mode: see below
Yes, if there was a strict / permissive setting we’d have to explain it concisely. I think the way to approach that would be to focus on “addition” and “removal”. You could perhaps describe it like this
Setting: “Mode”
Option 1 (permissive):
Label: “Add Members”
Description: “Allow members to be added to this group on authentication”
Option 2 (strict):
Label: “Add and Remove Members”
Description: “Allow members to be both added and removed on authentication. This will disable manual membership controls.”
The reason I’m keen to keep the “permissive” option is that when working with clients on this kind of thing before, that is the most common use case, i.e.
The main need is to allow for access to a group depending on a state in an external service
The need to remove access depending on the state in the external service is more marginal, i.e. people do lose access and should be removed, but this is relatively less important
There is often a desire to retain the ability to manually control membership in Discourse. When I’ve implemented the “strict” approach this has been “surprising” (i.e. “Why did person X lose membership?”) despite explaination(s) and it functioning (technically) as it should.
Yes this is important, as folks will often wonder “why” someone is added or removed, particularly in strict mode, and we (i.e. “Discourse”) won’t have control over the veracity of the claims the external auth service is making.
Perhaps a new type of “Remove User” and “Add User” action that includes the auth service responsible for the action, i.e. “Remove User ([service name])”.
One other “gotcha” here is that we’ll need to be clear that this isn’t a cure-all for the use case of “I want a group’s memberships to be based on external service X” as people don’t authenticate that frequently, or at least, according to the needs of the standard use case of that type.
This (auth management) is a necessary piece of handling that kind use case, but to properly serve it you also need to set up event-based integrations, e.g. a webhook receiver (we have a private webhook receiver plugin designed for group management that I hope to open source at some point soonish).
Just as a functionality comparison, thought I’d share what we do over at the Global Legal Empowerment Network, which uses wordpress and the discourse wp plugin. We have it set up so every time the user is updated in wordpress, it updates in discourse. This includes some special groups (e.g. core member, resource contributor, etc) as well as profile details. We added a hidden user field for “last updated” which helped with troubleshooting and making sure it was working properly.
We lock down those groups managed remotely so users cannot join or leave them when in discourse, but did not see the need to prevent group membership management by staff. I like the look of what you are trying above but it is a bit beyond me tbh.
We have some Discourse for Teams customers who use Okta actively for managing access to all their company apps. They set up roles there as well which are then supported for e.g. providing certain levels of accss to Microsoft Tableau, etc. Okta also is a directory so they manage user avatars, bio, location and other profile type info as well. They’d like to see this profile info updated in Teams externally from Okta as well.
We should try to keep the super-technical stuff out of the group settings if possible. Having to link to syntax documentation probably means it should be simplified, or relegated to the admin site settings. I think we should leave that part up to each auth plugin to implement, because it can vary so much.
Taking OIDC as an example, that plugin would add a new site setting openid_connect_roles_claim. If using Okta, the admin would configure that as groups. I think string array is a pretty standard format for OIDC, but we could explore more complex options here if really necessary.
To receive this information, Auth::Result would be given a new roles attribute, which accepts a simple array of strings. Core will then take this (in Auth::Result#apply_user_attributes!), and load it into a new UserAssociatedRoles table with columns (provider_name, user_id, role). This UserAssociatedRoles table gives us the ‘membership source identification’ you mentioned in the OP.
I imagine the group settings looking something like this:
Role names would be prefixed with the provider_name. Autocomplete would be based on all the existing values in the UserAssociatedRoles table, but we would also accept non-autocompleted values in case the role hasn’t been seen by Discourse yet.
The beauty of having a complete user_associated_roles table in the database is that admins can do whatever they like with Discourse groups, and memberships will be instantly updated without users having to log in again.
That’s fair. I think it would be best to keep this as simple as possible, at least for v1, so I’d rather not have multiple ‘modes’. How about we have this as the default behaviour:
Add members when they have a matching role from the auth provider
Remove members when that role disappears
Allow adding/removing in Discourse as well
If an admin tries to remove a user which was added via an auth provider, show a warning “this user may be added again next time they log in”
I think we should try to be secure-by-default here, and remove members when they lose the role on the identity provider. With the membership log improvements, I hope it should be less confusing than the current status-quo.
For sites that really don’t want that, we could have a site setting like remove_group_membership_when_auth_role_lost (default true).
Yeah for sure. We also have this problem with other user metadata like name, avatar, etc. I think at some point soon we need to look at making a sync_sso equivalent for other auth plugins. That could be passed all the info which is normally passed via OIDC / SAML / etc., including this new ‘role’ information’. Probably a separate project though.
How does that all sound @angus? It is a little less flexible than separate strict/permissive modes, but I think having just one mode will make troubleshooting / docs / support much easier.
With that plan, here’s a high-level view of what it looks like from an admin perspective:
In Discourse, set openid_connect_roles_claim to groups
Setting up a new group
Create a discourse group as normal. Configure name / full-name / flair / etc. to whatever you like
Go to the ‘membership’ preferences, put your cursor in the SSO roles dropdown, and choose a role from the dropdown. If Discourse hasn’t seen someone log in with the role yet, you’ll have to enter it manually
You can specify multiple roles for a single Discourse group. e.g. a ‘team’ group in Discourse could be a combination of oidc:employees and oidc:contractors
Press save, and group membership will be instantly updated with the role info Discourse has cached from previous logins.
On future logins, any changes to identity provider roles will be reflected in the Discourse group
You can still add / remove users to the group in the native Discourse UI
If you try to remove a user which was added via an identity provider, a warning will be shown to say that the user might be re-added on next login
If you later decide you do not want that IDP role to be associated with the group, you can remove it, and all users which were members via that role will be removed
hm yes, however there are cases in which supporting a boolean claim makes sense. But yes, I agree let’s keep it simple for now by just supporting a string array.
Doing it authenticator by authenticator, I guess we’d also have a group_sync_enabled? method in the Auth::Authenticator which would be overriden in individual authenticators, and in the generic authenticators upon the existence of a value in the relevant setting.
That actually raises an interesting question. This could also be used by the specific service authenticators, such as facebook (“groups”?), google (“groups”?) and discord (“roles”?). Not sure whether their id tokens would contain such info, but perhaps worth considering from a technical design perspective (i.e. the ability to add this down the track). Not a primary concern for v1 though I don’t think.
Those mechanics sound fine, however I’m curious why we’ve switched to using “roles” instead of “groups” here. Not a huge issue, but want to be sure there’s not something I’m missing.
From a UX perspective I can see confusion arising out of that use of nomeclature (i.e. “roles” and “groups” at the same time), even if it’s grounded in a useful technical distinction. Also possible developer confusion if coming to this without this background context.
I like the seperate table, however perhaps we should have a foreign key association with user_associated_accounts instead of provider_name? It could be useful in things like cleanup operations. I guess the answer to that question is partly how much we want the group/role association to be interlinked with the associated account from a product perspective. Would there be any downside to linking the two now?
** Edit, I guess we already have the association via the user_id.
That sounds fine, however I’m just thinking that we also have this information on a provider basis by virtue of the site setting you proposed, which would also cover the case of when the role hasn’t been seen. Perhaps there’s a way to use Discourse.authenticators here, i.e. an in-memory list of providers’ and their claims?
I think that’s a good compromise. Particularly if we also have the site setting mentioned below.
On a plugin (i.e. authenticator) basis? If so, I’m on board. That should service that need for v1.
Good summary of the user flow Pending the relatively minor suggestions I’ve made, I’m on board with the direction.
Yeah for sure. I was particularly thinking of Google, since people like to use that for their GSuite organisations, which I assume have some concept of groups.
I guess I was trying to avoid any confusion between “Identity Provider Groups” and “Discourse Groups”… but it’s possible I just made things more confusing by changing the name. Quite happy to stick to “groups” here.
My thinking was that we still support non-managed-authenticator auth providers, so there might not be a user_associated_account record. We can still join into it via the user_id as you said
I’m not sure the site setting would help here. If we’re talking about an array of strings representing “roles”, the site setting wouldn’t specify what the roles actually are. It would just specify how to obtain the array. Does that make sense?
Yes, you’re right. I was still thinking about a previous implementation where I had included specific groups in the setting itself, but we’re not doing that here, which is fine.
I think there’s enough info here to start on work on the PR, which I’ll start later this week, unless you have any remaining concerns? I’ll update here if anything arises on the way.
I’ve gone for the slightly tricker case of google apps hd groups for the initial implementation as I think it helps to think through the possible permutations of this, e.g. the need to account for domain-specific groups from a provider.
To implement that use case, I’ve also had to introduce a new concept of “secondary authorization” at the point of authentication, to allow for incremental authorization. I considered a few different ways to implement asking for specific user’s group permissions (i.e. if they were authenticating with a hd), and this seemed to be the most feasible. I appreciate this is perhaps a bigger change on that front than anticipated, but it’s perhaps worth discussing.
Note that to implement the google hd groups case you need to give the non-admin members of your google apps hd groups delegated admin authority in order to list their groups (via the admin directory API). There’s actually a “beta” pre-built administrator role called “Groups Reader” that works well for this. See Pre-built administrator roles - Google Workspace Admin Help
The Google implementation works. If you set it up and then authenticate with a hd your google hd groups will be available in the automatic group membership setting, you’ll be added to that discourse group if that hd group is selected, removed if it’s removed (with both actions logged with some specificity), and subsequent users in that google hd group who authenticate will be added immediately.
The details should be evident from the code and the specs. You’ll also notice that I’ve ended up adding three new tables. I attempted a few more "lightweight’ solutions, but they each ended up being more convoluted and inefficient when it came to handling updates to a user’s associated groups and a group’s associated groups. It’s hard to avoid just creating new tables for each. Open to ideas on the data modelling front though, and more generally.
Some technical todos left (aside from the conceptual/product questions raised above). Suggestions also welcome on this front:
Perhaps serialize the associated_groups label (instead of modelling on the client).
Add missing specs and qunits
Perhaps move user_associated_group / group_associated_group creation/destruction to a background job, as with large numbers this could be slow.
I am a little hesitant about the ‘secondary authorization’ thing, and also the provider_domain column. Can you elaborate some more on why they’re needed? Seems like they’re quite google-specific… is there a reason we can’t request the admin.directory.group.readonly scope during the first auth request? And maybe just prefix the group name with the domain? (or exclude the domain entirely, since I assume people will only be using this with a single google ‘hosted domain’?)
Yeah totally happy with 3 tables here - it keeps things cleaner
Agreed
We need to be careful here. Any group memberships need to be assigned before the user loads the site for the first time. Otherwise, they won’t see group-specific things the first time they log in (e.g. secure categories). So I think changes to user_associated_group records should be processed synchronously.
But for group_associated_group record changes, I think you’re right. Changes there could affect thousands of users, so they will need to be processed in_batches. I think I’d start by doing it synchronously, with a loading spinner in the UI. That way, admins will clearly be able to see when it’s running/done.
If we see it getting close to 30 seconds (the unicorn request timeout), then we might need to think about a background job.
We might also need to think about adding some DistributedMutex locking here. For example, if a user logs in while a group_associated_group change is being processed, what happens? Happy to discuss this kind of stuff on GitHub once we get the overall architecture finalised.
The reason you can’t request group permissions on the first request is that you don’t know who’s signing in, or what they’re comfortable with sharing. You could restrict google associated group mapping to people using the google_oauth2_hd setting, however this would limit the scope of the feature a fair bit. Having a team that uses google apps in addition to “public” users who also want to use google auth is relatively common.
*edit I should clarify, that if you ask for a groups scope and the user cannot give it (e.g. their HD has not delegated authority to non admin users as described above) then the auth will fail. You can’t request optional scopes alongside required scopes.
Moreover, that approach, i.e. asking for a groups scope up front as a standard practice in implementing this in the various authentication methods, would be arguably more google-specific than the alternative, as you don’t always have the equivalent of the hosted domain system to restrict login to. For example, I could be wrong, but I don’t think there’s a way to restrict Github OAuth2 login to a specific github organisation.
In other words in a number of cases you’d be left with the choice of asking everyone who uses that auth method to grant the relevant groups scope, or not using the feature. That approach may work in some contexts, but not in many. This incremental authorization approach gives different auth methods more flexibility in implementing the feature.
It’s true Google has been championing incremental authorization in the OAuth2 space, for example the working papers on the subject have all been written by a googler
However the concept is not Google-specific (other people don’t necessarily call it “incremental authorization”). It’s relatively common in different forms in mobile apps, and is being adopted in OAuth2 by other providers. For example, here’s Facebook’s docs on the same subject.
You’re probably familiar with the field already, but it is considered “good practice” to
tell people you’re about to ask for more permissions than the standard basic info;
and maybe why.
If the user clicked “Sign up with Facebook” in a Discourse login form, and then, in addition to their email, they were also asked for access to their facebook groups, they might drop off. Facebook puts it like this
As a general rule, the more permissions an app requests, the less likely it is that people will use Facebook to log into your app. In fact, our research shows that apps that ask for more than four permissions experience a significant drop off in the number of completed logins.
This raises the question of whether it’s a good idea to be asking for additional scopes at the point of authentication in the first place, and basically I concluded that we don’t really have any other good options. There’s the need to have immediate access to groups in some scenarios (as you alluded to), and there’s also just the reality that without it being asked up front many users would not take an additional step to authorize their groups on a service, in say their profile, or the groups page.
It has to be done at the point of authentication, which brings us back to the issue referenced above, and why I implemeted a “secondary authorizatiton” system. It is indeed intended to be a lightweight “system” insofar as it is relatively easy for another service, say Facebook or Github, to implement a secondary authorization request to obtain access to the user’s groups after they have authenticated and optionally passing a certain test relating to their basic info.
Each provider just needs to
Return a result with secondary_authorization_url
Use the state parameter to detect which authorization request the user is up to
Provide a omniauth_secondary_authorization_description for users/omniauth_callbacks/secondary_authorization.html.erb. e.g. this is the one for google, which the user sees before they confirm the secondary authorization redirect
As you've signed in with a %{domain} email, we need to ask for permission to view your %{domain} groups.
None of these parts are google-specific.
What I would like to do here is allow the user to say “no” to the secondary request, and still authenticate. In the Google Apps HD scenario this isn’t really an issue as if their account is part of a hosted domain, then they’re unlikely to want to, or be in a position to, say no. However it should be accomodated to allow for the full range of authentication secnarios here I think.
Finally, it should also be noted that secondary authorization is not required for associated_groups to work. An auth provider can just ask for the groups scope up front, and then add the groups to the auth result after receiving the first response. Indeed, we should probably build that in as an option to the basic oauth2 and openid connect plugins.
Provider Domain
I do think there needs to be some form of secondary identifier in the associated_groups table that’s readable by the site admin. There are a number of scenarios in which a group name by itself may not be enough. There’s the possibiity of name conflicts across each services’ equivalent concept, e.g.
multi-domain google group management (you can have multiple domains in one workspace too)
mutli-org github group management
multi-server discord role management
etc
We could change domain to namespace perhaps. We could include it in the group’s name, but would that give as any advantages? It may be useful to query by the “domain” or “namespace” at some point. Yes, perhaps namespace would be better than domain.
The reason it needs to be “admin readable” is that it’s used in the label seen by the admin in the groups UI, partly for disambiguation purposes.
I’m mulling whether it makes sense to attempt to also store a provider_id here (if one exists). Could be useful down the track perhaps.
Yup, agree with all this, and thanks for the tips. I’ll give this part a go and we can discuss it further on the PR.
@david I’ve just pushed a few updates on this one including
DistributedMutxes and in_batches in group_associated_group
Acceptance tests (already had rspec)
Will need some further work no doubt, but it’s currently working according to spec and all tests are passing. Take it for a test drive, let me know what you think and what changes you’d like.
Pending the relatively minor suggestions I’ve made, I’m on board with the direction.
