Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle groups #7

Closed
wants to merge 5 commits into from
Closed

Handle groups #7

wants to merge 5 commits into from

Conversation

mattcg
Copy link

@mattcg mattcg commented Dec 17, 2020

As discussed in a comment on the plugin post and in a separate post asking for proposals. Would appreciate some help with tests as I am not an experienced Ruby developer.

Introduces three new settings:

  • openid_connect_handle_groups will activate the code path for handling groups in the token
  • openid_connect_create_groups will create groups that the IdP user is a member of but that are not present in Discourse
  • openid_connect_strict_groups will remove user from Discourse groups that are not present in the token

Thanks to @rysiekpl who originally wrote most of this code for a prototype plugin.

@eviltrout
Copy link

As you noted this is going to require some tests. I recommend you look at the existing tests and how they work, but you can also reference the rspec documentation.

@davidtaylorhq
Copy link
Member

If possible I'd like us to keep any modification of group membership out of this group plugin, and instead move it into this method of discourse core

https://github.com/discourse/discourse/blob/422f395042ef7f32d528a2c11e8da619fdbd5641/lib/auth/result.rb#L71-L89

That way, we have a clean API which all plugins can use.

The most difficult thing here is the data model. 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?

e.g.

  1. a user authenticates via OIDC with groups ['group1', 'group2']
  2. In the discourse UI, the user is added to group3
  3. 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:

  1. a user authenticates via OIDC with groups ['group1', 'group2']
  2. In the discourse UI, the user is removed from group1
  3. 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 🤔

If we can iron out the data model, then I think this would be a great addition to Discourse core. If you like, a topic on Discourse Meta would be welcome. Happy to help with implementation there once we plan things out.

@angusmcleod
Copy link

See further: https://meta.discourse.org/t/managing-group-membership-via-authentication/175950

Base automatically changed from master to main February 16, 2021 20:50
@pierreozoux
Copy link

pierreozoux commented May 5, 2022

It seems that the basic work was made upstream thanks to @angusmcleod
https://meta.discourse.org/t/managing-group-membership-via-authentication/175950/25?u=pierreozoux
discourse/discourse#14835

I guess then that this PR has to be updated according.

Thanks for your work @mattcg :)

@CvX CvX requested a review from davidtaylorhq June 7, 2022 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants