Associated group memberships (PR #12446)

This is not yet complete. I’m raising it for further discussion here

GitHub

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

@davidtaylorhq can you review?

@davidtaylorhq Is there something holding up this review? I’m curious about it.

Nothing in particular holding it up - just time. It’s a very complex change from a security/performance/api point of view, so I want to go through it in a lot of detail. I’ll do my best to get to it early next week.

@davidtaylorhq Let me know if there are particular areas of concern. I’d be happy to invest a bit more time in this one as well.

Sorry for the huge delay here @angusmcleod - it should not have taken me this long to review this! I think the best approach here is for us to get these changes into a minimum viable state so that we can merge them, default disabled, and with the relevant site settings set as hidden: true. Then we can start experimenting with it in the real world, and expand on the performance/functionality in later PRs.

Secondary authorization API

As we discussed on Meta, I’m not particularly comfortable with the ‘secondary authorization url’ API. In general, our authentication system expects all the nitty-gritty of the authentication to take place in OmniAuth. As you found, trying to manipulate HTTP requests/responses inside an Auth::Authenticator is very tricky - the authenticator is not given direct access to the request or the session, so various hacks are required to access that info (e.g. auth_token[:session] and session.instance_variable_get). I don’t think we should encourage this by including it in core.

It is technically possible to create an ‘incremental authentication’ flow using OmniAuth, without needing any changes to Discourse APIs. So if a plugin truly needs it, they can do something like this (very rough, and mostly untested). I’m open to introducing some additional APIs to make this easier, but I think that should be a separate PR.

For the purposes of this PR, I think we should remove the incremental authentication, and add a google_oauth2_request_groups boolean site setting. This would only work when google_oauth2_hd is configured. I know it’s not as flexible, but I think it would be a good MVP, and allow us to focus on the core APIs.

Data model

As you suggested on Meta, let’s add a provider_id column to the associated_groups table. I’d also like to remove the domain column, and instead make it the provider’s responsibility to prefix/suffix their associated_group.provider_id and associated_group.name fields if needed. I think this will make the data model more flexible, easier to query, and much simpler to understand/query for developers.

Other than those column changes, I think the db/model structure is great, and I like the use of ActiveRecord after_create/after_destroy hooks!

There is one edge case I noticed. If a user has multiple UserAssociatedGroups which link to the same group, then there can be problems. In pseudocode (this won’t actually run):

group = Group.create!
user = User.create!

ag1 = AssociatedGroup.create!(name: "externalgroup1", group: group, users: [user])
ag2 = AssociatedGroup.create!(name: "externalgroup2", group: group, users: [user])

# User is now a member of `group` via two associated groups

ag1.destroy

# User has now been removed from `group`, even though they are still part of `ag2`

Group UI

Most Discourse sites won’t use this functionality, so we should hide the UI by default. I suggest we add a new method to Auth::Authenticator like provides_groups?.

class Auth::Authenticator
  def provides_groups?
    false
  end
end

Individual providers can then override that like

class Auth::GoogleOAuth2Authenticator < Auth::ManagedAuthenticator
  def provides_groups?
    SiteSetting.google_oauth2_request_groups
  end
end

Then we can add a boolean to the group_show_serializer like

class GroupShowSerializer < BasicGroupSerializer
  attributes :show_associated_groups
  def show_associated_groups
    Discourse.enabled_authenticators.any? { |a| a.provides_groups? }
  end
  def include_show_associated_groups?
    scope.admin?
  end
end

and use that boolean to show/hide the UI on the client side.


Hope that all makes sense - sorry it’s so long! Very happy to help out with these making changes if you like - just let me know.

@davidtaylorhq Thanks david, I’ll process this later this week and look to follow up with some changes next week.