angus
November 8, 2021, 6:10am
22
Note that I’ve made an updated draft PR with a new approach (incorporating changes requested by @david on my last attempt). As mentioned in the comments to the PR, I’m looking to finish it off sometime this week.
discourse:main ← angusmcleod:group_membership_via_authentication
opened 06:06AM - 08 Nov 21 UTC
Update on #12446.
@davidtaylorhq A draft for an update on the associated grou… p membership work. It's been rebased, brought in line with the latest changes and updated in the ways explained below. Let me know if you're onboard with the approach so far. I'll finish it off next weekend.
### Authentication and group retrieval
I've removed the secondary auth stuff, added a google-hd-specific boolean setting, and moved the group retrieval into a new google oauth2 omniauth strategy. I've made a new folder and added a spec for the strategy (something new in the codebase) as I thought it deserved a spec (which is also included). If we add in secondary auth to this strategy that too will need a spec. Some of the existing discourse omniauth strategies, e.g. Discord, may benefit from being seperated out and tested too.
### Group Provision and UI
As suggested, I've added a ``provides_groups?`` to the ``Auth::Authenticator``. Using this approach led to a new groups guardian method to use at the various group CRUD points. Speaking of which, I've added in support for group_associated_group creation in group creation (i.e. ``/g/custom/new``). Supporting it in both ``new`` and ``manage`` (update) also led to adding a new site attribute ``can_associate_groups``. I attempted to make this more group-route specific, however this leads to various clumsy workarounds to determine whether an admin can associate groups in ``/g/custom/new``. The site attribute approach proved the simplest and most performant (i.e. fewer ajax calls).
### Data model
I've made one change to the data model so far, namely to use ``after_commit`` hooks rather than ``after_create``/``after_destroy`` as using those hooks leads to duplicate record creation in ``Admin::GroupsController`` ``create``. I've yet to deal with the edge case you pointed out, i.e. multiple UserAssociatedGroups which link to the same group.
3 Likes
mattdm
November 8, 2021, 2:57pm
23
Just so I’m not getting myself excited for no reason — this says “google”… will it also work with non-google oauth2 sso?
2 Likes
angus
November 8, 2021, 3:01pm
24
It’s a generic system, but the first supported use case will be groups in a google workspace. Once the system is in place adding support for additional providers will not be too difficult.
5 Likes
angus
November 22, 2021, 6:06am
25
Note that this PR was moved from draft to published over the weeknd (i.e. it’s ready for review again).
discourse:main ← angusmcleod:group_membership_via_authentication
opened 06:06AM - 08 Nov 21 UTC
Update on #12446.
@davidtaylorhq A draft for an update on the associated grou… p membership work. It's been rebased, brought in line with the latest changes and updated in the ways explained below. Let me know if you're onboard with the approach so far. I'll finish it off next weekend.
### Authentication and group retrieval
I've removed the secondary auth stuff, added a google-hd-specific boolean setting, and moved the group retrieval into a new google oauth2 omniauth strategy. I've made a new folder and added a spec for the strategy (something new in the codebase) as I thought it deserved a spec (which is also included). If we add in secondary auth to this strategy that too will need a spec. Some of the existing discourse omniauth strategies, e.g. Discord, may benefit from being seperated out and tested too.
### Group Provision and UI
As suggested, I've added a ``provides_groups?`` to the ``Auth::Authenticator``. Using this approach led to a new groups guardian method to use at the various group CRUD points. Speaking of which, I've added in support for group_associated_group creation in group creation (i.e. ``/g/custom/new``). Supporting it in both ``new`` and ``manage`` (update) also led to adding a new site attribute ``can_associate_groups``. I attempted to make this more group-route specific, however this leads to various clumsy workarounds to determine whether an admin can associate groups in ``/g/custom/new``. The site attribute approach proved the simplest and most performant (i.e. fewer ajax calls).
### Data model
I've made one change to the data model so far, namely to use ``after_commit`` hooks rather than ``after_create``/``after_destroy`` as using those hooks leads to duplicate record creation in ``Admin::GroupsController`` ``create``. I've yet to deal with the edge case you pointed out, i.e. multiple UserAssociatedGroups which link to the same group.
4 Likes
david
December 9, 2021, 12:32pm
26
I just merged this PR - huge thanks for all your work here @angus ! Excited to see how this gets used and extended going forward!
I’ve labelled the site setting “Experimental” for now, to give us some time to test it out and make sure everything is working smoothly. Once we’re confident, and we’ve added support in a few more authentication providers, I’ll be sure to make a #feature:announcements topic for the feature.
7 Likes
angus
December 9, 2021, 1:44pm
27
Awesome!
Thank you David. It wouldn’t have happened without your support. Happy to help with adding additional providers.
5 Likes
mattdm
December 9, 2021, 2:18pm
28
YES! Thank you everyone. We’re planning to use this extensively in Fedora once it works with oauth2.
4 Likes
jimkleiber
March 10, 2022, 11:57pm
29
I’m also excited for this to be available for non-Google oauth2/openID logins…any update on if/when that option might be available?
4 Likes
david
March 11, 2022, 11:49am
30
We don’t have a specific timeline, but it’s certainly #pr-welcome if anyone would like to submit a patch
3 Likes
agungor
May 19, 2022, 3:11pm
31
Looking forward to this as well! The use case for us is to pull group membership from Keycloak on authentication.
5 Likes
