Group membership via google hd authentication (PR #14835)

Update on #12446.

@davidtaylorhq A draft for an update on the associated group 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 the Admin::GroupsController create controller. I’ve yet to deal with the edge case you pointed out, i.e. multiple UserAssociatedGroups which link to the same group.

GitHub

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

@davidtaylorhq this is ready for another review!

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

This is looking great - thanks @angusmcleod. I’ve made a handful of suggestions, but nothing too major. I’d still like to take it for a spin locally before merging, but I think we’re super close to having this done! :rocket:

  end
  
  private
  
  def with_mutex
    DistributedMutex.synchronize("group_associated_group_#{group_id}_#{associated_group_id}") do
      yield
    end
  end
    with_mutex do
    with_mutex do

I think this can be made a little more efficient by using EXISTS rather than count(), and also removing the nested subquery:

    Group.where("NOT EXISTS (
      SELECT 1
      FROM user_associated_groups uag
      JOIN group_associated_group gag
      ON gag.associated_group_id = uag.associated_group_id
      WHERE uag.user_id = :user_id
      AND gag.id != :gag_id
      AND gag.group_id = groups.id
    )", gag_id: id, user_id: user_id).each do |group|

(this is untested, but I think it should work :crossed_fingers: )

We could also extract this SQL into a constant (maybe AssociatedGroup::OTHER_USER_ASSOCIATIONS_WITH_GROUP_SQL) so that it can be shared with GroupAssociatedGroup#remove_from_associated_group. Then it would be something like:

    Group.where("NOT EXISTS (
      #{AssociatedGroup::OTHER_USER_ASSOCIATIONS_WITH_GROUP_SQL}
    )", gag_id: id, user_id: user_id).each do |group|

  def include_associated_group_ids
    scope.can_associate_groups?
  end

  def include_can_associate_groups
    scope.admin?
  end

I don’t think this is doing anything? (note the ==, not =) Discourse.system_user is the default acting_user anyway.

I don’t think this is doing anything? (note the ==, not =) Discourse.system_user is the default acting_user anyway.

We’ve removed secondary authentication for now, so I think these are unused?

    strategy_class = Auth::OmniAuthStrategies::DiscourseGoogleOauth2

Let’s move this within the Discourse namespace, so that the file path matches the class name

class ::Auth::OmniAuthStrategies::DiscourseGoogleOauth2 < ::OmniAuth::Strategies::GoogleOauth2

(this is similar to how the Discord OmniAuth strategy is handled)

        strategy.options[:request_groups] = provides_groups?

Could we make this one method?

  def provides_groups?
    SiteSetting.google_oauth2_hd.present? && SiteSetting.google_oauth2_hd_groups

Why would it not respond_to? :apply_associated_attributes!?

    auth.apply_associated_attributes!
          Rails.logger.error("[Discourse Google OAuth2] failed to retrieve groups for #{uid} - status #{response.status}")