diff --git a/app/assets/javascripts/discourse/app/components/groups-form-membership-fields.js b/app/assets/javascripts/discourse/app/components/groups-form-membership-fields.js index 7603df532c5..5292efa5858 100644 --- a/app/assets/javascripts/discourse/app/components/groups-form-membership-fields.js +++ b/app/assets/javascripts/discourse/app/components/groups-form-membership-fields.js @@ -1,11 +1,13 @@ import Component from "@ember/component"; import I18n from "I18n"; import { computed } from "@ember/object"; -import { not } from "@ember/object/computed"; +import { not, readOnly } from "@ember/object/computed"; import discourseComputed from "discourse-common/utils/decorators"; +import AssociatedGroup from "discourse/models/associated-group"; export default Component.extend({ tokenSeparator: "|", + showAssociatedGroups: readOnly("site.can_associate_groups"), init() { this._super(...arguments); @@ -20,6 +22,10 @@ export default Component.extend({ { name: 3, value: 3 }, { name: 4, value: 4 }, ]; + + if (this.showAssociatedGroups) { + this.loadAssociatedGroups(); + } }, canEdit: not("model.automatic"), @@ -54,6 +60,10 @@ export default Component.extend({ return this.model.emailDomains.split(this.tokenSeparator).filter(Boolean); }), + loadAssociatedGroups() { + AssociatedGroup.list().then((ags) => this.set("associatedGroups", ags)); + }, + actions: { onChangeEmailDomainsSetting(value) { this.set( diff --git a/app/assets/javascripts/discourse/app/models/associated-group.js b/app/assets/javascripts/discourse/app/models/associated-group.js new file mode 100644 index 00000000000..e914aaf824e --- /dev/null +++ b/app/assets/javascripts/discourse/app/models/associated-group.js @@ -0,0 +1,17 @@ +import EmberObject from "@ember/object"; +import { ajax } from "discourse/lib/ajax"; +import { popupAjaxError } from "discourse/lib/ajax-error"; + +const AssociatedGroup = EmberObject.extend(); + +AssociatedGroup.reopenClass({ + list() { + return ajax("/associated_groups") + .then((result) => { + return result.associated_groups.map((ag) => AssociatedGroup.create(ag)); + }) + .catch(popupAjaxError); + }, +}); + +export default AssociatedGroup; diff --git a/app/assets/javascripts/discourse/app/models/group.js b/app/assets/javascripts/discourse/app/models/group.js index 2f597b4dbac..a360f8c3d89 100644 --- a/app/assets/javascripts/discourse/app/models/group.js +++ b/app/assets/javascripts/discourse/app/models/group.js @@ -29,6 +29,11 @@ const Group = RestModel.extend({ return isEmpty(value) ? "" : value; }, + @discourseComputed("associated_group_ids") + associatedGroupIds(value) { + return isEmpty(value) ? [] : value; + }, + @discourseComputed("automatic") type(automatic) { return automatic ? "automatic" : "custom"; @@ -277,6 +282,11 @@ const Group = RestModel.extend({ } ); + let agIds = this.associated_group_ids; + if (agIds) { + attrs["associated_group_ids"] = agIds.length ? agIds : [null]; + } + if (this.flair_type === "icon") { attrs["flair_icon"] = this.flair_icon; } else if (this.flair_type === "image") { diff --git a/app/assets/javascripts/discourse/app/templates/components/groups-form-membership-fields.hbs b/app/assets/javascripts/discourse/app/templates/components/groups-form-membership-fields.hbs index bce462add00..98e85f36a9f 100644 --- a/app/assets/javascripts/discourse/app/templates/components/groups-form-membership-fields.hbs +++ b/app/assets/javascripts/discourse/app/templates/components/groups-form-membership-fields.hbs @@ -59,6 +59,23 @@ onChange=(action "onChangeEmailDomainsSetting") options=(hash allowAny=true) }} + + {{#if showAssociatedGroups}} + + + {{list-setting + name="automatic_membership_associated_groups" + class="group-form-automatic-membership-associated-groups" + value=model.associatedGroupIds + choices=associatedGroups + settingName="name" + nameProperty="label" + valueProperty="id" + onChange=(action (mut model.associated_group_ids)) + }} + {{/if}} {{plugin-outlet name="groups-form-membership-below-automatic" diff --git a/app/assets/javascripts/discourse/tests/acceptance/group-manage-membership-test.js b/app/assets/javascripts/discourse/tests/acceptance/group-manage-membership-test.js index 15c5c0d56fe..ffe5b0bf251 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/group-manage-membership-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/group-manage-membership-test.js @@ -7,9 +7,24 @@ import { import { click, visit } from "@ember/test-helpers"; import selectKit from "discourse/tests/helpers/select-kit-helper"; import { test } from "qunit"; +import Site from "discourse/models/site"; acceptance("Managing Group Membership", function (needs) { needs.user(); + needs.pretender((server, helper) => { + server.get("/associated_groups", () => + helper.response({ + associated_groups: [ + { + id: 123, + name: "test-group", + provider_name: "google_oauth2", + label: "google_oauth2:test-group", + }, + ], + }) + ); + }); test("As an admin", async function (assert) { updateCurrentUser({ can_create_group: true }); @@ -94,6 +109,36 @@ acceptance("Managing Group Membership", function (needs) { assert.strictEqual(emailDomains.header().value(), "foo.com"); }); + test("As an admin on a site that can associate groups", async function (assert) { + let site = Site.current(); + site.set("can_associate_groups", true); + updateCurrentUser({ can_create_group: true }); + + await visit("/g/alternative-group/manage/membership"); + + const associatedGroups = selectKit( + ".group-form-automatic-membership-associated-groups" + ); + await associatedGroups.expand(); + await associatedGroups.selectRowByName("google_oauth2:test-group"); + await associatedGroups.keyboard("enter"); + + assert.equal(associatedGroups.header().name(), "google_oauth2:test-group"); + }); + + test("As an admin on a site that can't associate groups", async function (assert) { + let site = Site.current(); + site.set("can_associate_groups", false); + updateCurrentUser({ can_create_group: true }); + + await visit("/g/alternative-group/manage/membership"); + + assert.ok( + !exists('label[for="automatic_membership_associated_groups"]'), + "it should not display associated groups automatic membership label" + ); + }); + test("As a group owner", async function (assert) { updateCurrentUser({ moderator: false, admin: false }); @@ -104,6 +149,11 @@ acceptance("Managing Group Membership", function (needs) { "it should not display automatic membership label" ); + assert.ok( + !exists('label[for="automatic_membership_associated_groups"]'), + "it should not display associated groups automatic membership label" + ); + assert.ok( !exists(".groups-form-automatic-membership-retroactive"), "it should not display automatic membership retroactive checkbox" diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb index f80dfe27c5e..33944246e8e 100644 --- a/app/controllers/admin/groups_controller.rb +++ b/app/controllers/admin/groups_controller.rb @@ -180,6 +180,10 @@ class Admin::GroupsController < Admin::AdminController custom_fields = DiscoursePluginRegistry.editable_group_custom_fields permitted << { custom_fields: custom_fields } unless custom_fields.blank? + if guardian.can_associate_groups? + permitted << { associated_group_ids: [] } + end + permitted = permitted | DiscoursePluginRegistry.group_params params.require(:group).permit(permitted) diff --git a/app/controllers/associated_groups_controller.rb b/app/controllers/associated_groups_controller.rb new file mode 100644 index 00000000000..cf82ae20c7d --- /dev/null +++ b/app/controllers/associated_groups_controller.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +class AssociatedGroupsController < ApplicationController + requires_login + + def index + guardian.ensure_can_associate_groups! + render_serialized(AssociatedGroup.all, AssociatedGroupSerializer, root: 'associated_groups') + end +end diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 6fbb21044f6..6a2d6d37d11 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -736,6 +736,10 @@ class GroupsController < ApplicationController end end + if guardian.can_associate_groups? + permitted_params << { associated_group_ids: [] } + end + permitted_params = permitted_params | DiscoursePluginRegistry.group_params params.require(:group).permit(*permitted_params) diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 57c794f9cc2..1047d6f9ff6 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -171,6 +171,7 @@ class Users::OmniauthCallbacksController < ApplicationController elsif Guardian.new(user).can_access_forum? && user.active # log on any account that is active with forum access begin user.save! if @auth_result.apply_user_attributes! + @auth_result.apply_associated_attributes! rescue ActiveRecord::RecordInvalid => e @auth_result.failed = true @auth_result.failed_reason = e.record.errors.full_messages.join(", ") diff --git a/app/jobs/scheduled/clean_up_associated_groups.rb b/app/jobs/scheduled/clean_up_associated_groups.rb new file mode 100644 index 00000000000..9b3514798b8 --- /dev/null +++ b/app/jobs/scheduled/clean_up_associated_groups.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module Jobs + class CleanUpAssociatedGroups < ::Jobs::Scheduled + every 1.day + + def execute(args) + AssociatedGroup.cleanup! + end + end +end diff --git a/app/models/associated_group.rb b/app/models/associated_group.rb new file mode 100644 index 00000000000..9d6e9365dcd --- /dev/null +++ b/app/models/associated_group.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true +class AssociatedGroup < ActiveRecord::Base + has_many :user_associated_groups, dependent: :destroy + has_many :users, through: :user_associated_groups + has_many :group_associated_groups, dependent: :destroy + has_many :groups, through: :group_associated_groups + + def label + "#{provider_name}:#{name}" + end + + def self.has_provider? + Discourse.enabled_authenticators.any? { |a| a.provides_groups? } + end + + def self.cleanup! + AssociatedGroup.left_joins(:group_associated_groups, :user_associated_groups) + .where("group_associated_groups.id IS NULL AND user_associated_groups.id IS NULL") + .where("last_used < ?", 1.week.ago).delete_all + end +end + +# == Schema Information +# +# Table name: associated_groups +# +# id :bigint not null, primary key +# name :string not null +# provider_name :string not null +# provider_id :string not null +# last_used :datetime not null +# created_at :datetime not null +# updated_at :datetime not null +# +# Indexes +# +# associated_groups_provider_id (provider_name,provider_id) UNIQUE +# diff --git a/app/models/group.rb b/app/models/group.rb index 37e08a23db8..ebb703e88cd 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -21,6 +21,7 @@ class Group < ActiveRecord::Base has_many :group_users, dependent: :destroy has_many :group_requests, dependent: :destroy has_many :group_mentions, dependent: :destroy + has_many :group_associated_groups, dependent: :destroy has_many :group_archived_messages, dependent: :destroy @@ -32,6 +33,7 @@ class Group < ActiveRecord::Base has_many :reviewables, foreign_key: :reviewable_by_group_id, dependent: :nullify has_many :group_category_notification_defaults, dependent: :destroy has_many :group_tag_notification_defaults, dependent: :destroy + has_many :associated_groups, through: :group_associated_groups, dependent: :destroy belongs_to :flair_upload, class_name: 'Upload' belongs_to :smtp_updated_by, class_name: 'User' @@ -768,6 +770,20 @@ class Group < ActiveRecord::Base self end + def add_automatically(user, subject: nil) + if users.exclude?(user) && add(user) + logger = GroupActionLogger.new(Discourse.system_user, self) + logger.log_add_user_to_group(user, subject) + end + end + + def remove_automatically(user, subject: nil) + if users.include?(user) && remove(user) + logger = GroupActionLogger.new(Discourse.system_user, self) + logger.log_remove_user_from_group(user, subject) + end + end + def staff? STAFF_GROUPS.include?(self.name.to_sym) end diff --git a/app/models/group_associated_group.rb b/app/models/group_associated_group.rb new file mode 100644 index 00000000000..be71f09eb08 --- /dev/null +++ b/app/models/group_associated_group.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true +class GroupAssociatedGroup < ActiveRecord::Base + belongs_to :group + belongs_to :associated_group + + after_commit :add_associated_users, on: [:create, :update] + before_destroy :remove_associated_users + + def add_associated_users + with_mutex do + associated_group.users.in_batches do |users| + users.each do |user| + group.add_automatically(user, subject: associated_group.label) + end + end + end + end + + def remove_associated_users + with_mutex do + User.where("NOT EXISTS( + SELECT 1 + FROM user_associated_groups uag + JOIN group_associated_groups gag + ON gag.associated_group_id = uag.associated_group_id + WHERE uag.user_id = users.id + AND gag.id != :gag_id + AND gag.group_id = :group_id + )", gag_id: id, group_id: group_id).in_batches do |users| + users.each do |user| + group.remove_automatically(user, subject: associated_group.label) + end + end + end + end + + private + + def with_mutex + DistributedMutex.synchronize("group_associated_group_#{group_id}_#{associated_group_id}") do + yield + end + end +end + +# == Schema Information +# +# Table name: group_associated_groups +# +# id :bigint not null, primary key +# group_id :bigint not null +# associated_group_id :bigint not null +# created_at :datetime not null +# updated_at :datetime not null +# +# Indexes +# +# index_group_associated_groups (group_id,associated_group_id) UNIQUE +# index_group_associated_groups_on_associated_group_id (associated_group_id) +# index_group_associated_groups_on_group_id (group_id) +# diff --git a/app/models/user.rb b/app/models/user.rb index fc62d990479..d610cc524f7 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -38,6 +38,7 @@ class User < ActiveRecord::Base has_many :reviewable_scores, dependent: :destroy has_many :invites, foreign_key: :invited_by_id, dependent: :destroy has_many :user_custom_fields, dependent: :destroy + has_many :user_associated_groups, dependent: :destroy has_many :pending_posts, -> { merge(Reviewable.pending) }, class_name: 'ReviewableQueuedPost', foreign_key: :created_by_id has_one :user_option, dependent: :destroy @@ -83,6 +84,7 @@ class User < ActiveRecord::Base has_many :topics_allowed, through: :topic_allowed_users, source: :topic has_many :groups, through: :group_users has_many :secure_categories, through: :groups, source: :categories + has_many :associated_groups, through: :user_associated_groups, dependent: :destroy # deleted in user_second_factors relationship has_many :totps, -> { diff --git a/app/models/user_associated_group.rb b/app/models/user_associated_group.rb new file mode 100644 index 00000000000..1413efb7c67 --- /dev/null +++ b/app/models/user_associated_group.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +class UserAssociatedGroup < ActiveRecord::Base + belongs_to :user + belongs_to :associated_group + + after_commit :add_to_associated_groups, on: [:create, :update] + before_destroy :remove_from_associated_groups + + def add_to_associated_groups + associated_group.groups.each do |group| + group.add_automatically(user, subject: associated_group.label) + end + end + + def remove_from_associated_groups + Group.where("NOT EXISTS( + SELECT 1 + FROM user_associated_groups uag + JOIN group_associated_groups gag + ON gag.associated_group_id = uag.associated_group_id + WHERE uag.user_id = :user_id + AND uag.id != :uag_id + AND gag.group_id = groups.id + )", uag_id: id, user_id: user_id).each do |group| + group.remove_automatically(user, subject: associated_group.label) + end + end +end + +# == Schema Information +# +# Table name: user_associated_groups +# +# id :bigint not null, primary key +# user_id :bigint not null +# associated_group_id :bigint not null +# created_at :datetime not null +# updated_at :datetime not null +# +# Indexes +# +# index_user_associated_groups (user_id,associated_group_id) UNIQUE +# index_user_associated_groups_on_associated_group_id (associated_group_id) +# index_user_associated_groups_on_user_id (user_id) +# diff --git a/app/serializers/associated_group_serializer.rb b/app/serializers/associated_group_serializer.rb new file mode 100644 index 00000000000..db7dec87238 --- /dev/null +++ b/app/serializers/associated_group_serializer.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class AssociatedGroupSerializer < ApplicationSerializer + attributes :id, + :name, + :provider_name, + :label +end diff --git a/app/serializers/group_show_serializer.rb b/app/serializers/group_show_serializer.rb index 826b2c6533f..0b51aed01bf 100644 --- a/app/serializers/group_show_serializer.rb +++ b/app/serializers/group_show_serializer.rb @@ -36,7 +36,8 @@ class GroupShowSerializer < BasicGroupSerializer :imap_old_emails, :imap_new_emails, :message_count, - :allow_unknown_sender_topic_replies + :allow_unknown_sender_topic_replies, + :associated_group_ids def self.admin_or_owner_attributes(*attrs) attributes(*attrs) @@ -121,6 +122,14 @@ class GroupShowSerializer < BasicGroupSerializer end end + def associated_group_ids + object.associated_groups.map(&:id) + end + + def include_associated_group_ids? + scope.can_associate_groups? + end + private def authenticated? diff --git a/app/serializers/site_serializer.rb b/app/serializers/site_serializer.rb index 29972ae8f57..c65628748f3 100644 --- a/app/serializers/site_serializer.rb +++ b/app/serializers/site_serializer.rb @@ -21,6 +21,7 @@ class SiteSerializer < ApplicationSerializer :can_tag_pms, :tags_filter_regexp, :top_tags, + :can_associate_groups, :wizard_required, :topic_featured_link_allowed_category_ids, :user_themes, @@ -134,6 +135,14 @@ class SiteSerializer < ApplicationSerializer scope.can_tag_pms? end + def can_associate_groups + scope.can_associate_groups? + end + + def include_can_associate_groups? + scope.is_admin? + end + def include_tags_filter_regexp? SiteSetting.tagging_enabled end diff --git a/app/services/group_action_logger.rb b/app/services/group_action_logger.rb index 9fb65afbdec..d38d0e7c5a7 100644 --- a/app/services/group_action_logger.rb +++ b/app/services/group_action_logger.rb @@ -21,17 +21,19 @@ class GroupActionLogger )) end - def log_add_user_to_group(target_user) + def log_add_user_to_group(target_user, subject = nil) GroupHistory.create!(default_params.merge( action: GroupHistory.actions[:add_user_to_group], - target_user: target_user + target_user: target_user, + subject: subject )) end - def log_remove_user_from_group(target_user) + def log_remove_user_from_group(target_user, subject = nil) GroupHistory.create!(default_params.merge( action: GroupHistory.actions[:remove_user_from_group], - target_user: target_user + target_user: target_user, + subject: subject )) end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 4c5c10671e8..05a88bd7994 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -4124,6 +4124,7 @@ en: automatic_membership_user_count: one: "%{count} user has the new email domains and will be added to the group." other: "%{count} users have the new email domains and will be added to the group." + automatic_membership_associated_groups: "Users who are members of a group on a service listed here will be automatically added to this group when they log in with the service." primary_group: "Automatically set as primary group" name_placeholder: "Group name, no spaces, same as username rule" primary: "Primary Group" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index b46fb39daca..a85fe23ffbc 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1724,6 +1724,7 @@ en: google_oauth2_client_secret: "Client secret of your Google application." google_oauth2_prompt: "An optional space-delimited list of string values that specifies whether the authorization server prompts the user for reauthentication and consent. See https://developers.google.com/identity/protocols/OpenIDConnect#prompt for the possible values." google_oauth2_hd: "An optional Google Apps Hosted domain that the sign-in will be limited to. See https://developers.google.com/identity/protocols/OpenIDConnect#hd-param for more details." + google_oauth2_hd_groups: "(experimental) Retrieve users' Google groups on the hosted domain on authentication. Retrieved Google groups can be used to grant automatic Discourse group membership (see group settings)." enable_twitter_logins: "Enable Twitter authentication, requires twitter_consumer_key and twitter_consumer_secret. See Configuring Twitter login (and rich embeds) for Discourse." twitter_consumer_key: "Consumer key for Twitter authentication, registered at https://developer.twitter.com/apps" @@ -2390,6 +2391,7 @@ en: leading_trailing_slash: "The regular expression must not start and end with a slash." unicode_usernames_avatars: "The internal system avatars do not support Unicode usernames." list_value_count: "The list must contain exactly %{count} values." + google_oauth2_hd_groups: "You must first set 'google oauth2 hd' before enabling this setting." placeholder: discourse_connect_provider_secrets: diff --git a/config/routes.rb b/config/routes.rb index 041b713b7d2..1f75aaa30ce 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -634,6 +634,8 @@ Discourse::Application.routes.draw do end end + resources :associated_groups, only: %i[index], constraints: AdminConstraint.new + # aliases so old API code works delete "admin/groups/:id/members" => "groups#remove_member", constraints: AdminConstraint.new put "admin/groups/:id/members" => "groups#add_members", constraints: AdminConstraint.new diff --git a/config/site_settings.yml b/config/site_settings.yml index f2019ac4356..ff5e68b3597 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -411,6 +411,9 @@ login: - "select_account" google_oauth2_hd: default: "" + google_oauth2_hd_groups: + default: false + validator: GoogleOauth2HdGroupsValidator enable_twitter_logins: default: false twitter_consumer_key: diff --git a/db/migrate/20211106085344_create_associated_groups.rb b/db/migrate/20211106085344_create_associated_groups.rb new file mode 100644 index 00000000000..72c441fe5fa --- /dev/null +++ b/db/migrate/20211106085344_create_associated_groups.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true +class CreateAssociatedGroups < ActiveRecord::Migration[6.1] + def change + create_table :associated_groups do |t| + t.string :name, null: false + t.string :provider_name, null: false + t.string :provider_id, null: false + t.datetime :last_used, null: false, default: -> { "CURRENT_TIMESTAMP" } + + t.timestamps + end + + add_index :associated_groups, %i[provider_name provider_id], unique: true, name: 'associated_groups_provider_id' + end +end diff --git a/db/migrate/20211106085527_create_user_associated_groups.rb b/db/migrate/20211106085527_create_user_associated_groups.rb new file mode 100644 index 00000000000..73464b66f47 --- /dev/null +++ b/db/migrate/20211106085527_create_user_associated_groups.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true +class CreateUserAssociatedGroups < ActiveRecord::Migration[6.1] + def change + create_table :user_associated_groups do |t| + t.references :user, null: false + t.references :associated_group, null: false + + t.timestamps + end + + add_index :user_associated_groups, %i[user_id associated_group_id], unique: true, name: 'index_user_associated_groups' + end +end diff --git a/db/migrate/20211106085605_create_group_associated_groups.rb b/db/migrate/20211106085605_create_group_associated_groups.rb new file mode 100644 index 00000000000..281adb11b17 --- /dev/null +++ b/db/migrate/20211106085605_create_group_associated_groups.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true +class CreateGroupAssociatedGroups < ActiveRecord::Migration[6.1] + def change + create_table :group_associated_groups do |t| + t.references :group, null: false + t.references :associated_group, null: false + + t.timestamps + end + + add_index :group_associated_groups, %i[group_id associated_group_id], unique: true, name: 'index_group_associated_groups' + end +end diff --git a/lib/auth.rb b/lib/auth.rb index f501d901574..21e2716250f 100644 --- a/lib/auth.rb +++ b/lib/auth.rb @@ -6,6 +6,7 @@ require 'auth/auth_provider' require 'auth/result' require 'auth/authenticator' require 'auth/managed_authenticator' +require 'auth/omniauth_strategies/discourse_google_oauth2' require 'auth/facebook_authenticator' require 'auth/github_authenticator' require 'auth/twitter_authenticator' diff --git a/lib/auth/authenticator.rb b/lib/auth/authenticator.rb index 2302d3bce00..4333e87d631 100644 --- a/lib/auth/authenticator.rb +++ b/lib/auth/authenticator.rb @@ -65,4 +65,9 @@ class Auth::Authenticator def revoke(user, skip_remote: false) raise NotImplementedError end + + # provider has implemented user group membership (or equivalent) request + def provides_groups? + false + end end diff --git a/lib/auth/google_oauth2_authenticator.rb b/lib/auth/google_oauth2_authenticator.rb index 59b0caa2c78..02e015cd10c 100644 --- a/lib/auth/google_oauth2_authenticator.rb +++ b/lib/auth/google_oauth2_authenticator.rb @@ -16,6 +16,7 @@ class Auth::GoogleOAuth2Authenticator < Auth::ManagedAuthenticator end def register_middleware(omniauth) + strategy_class = Auth::OmniAuthStrategies::DiscourseGoogleOauth2 options = { setup: lambda { |env| strategy = env["omniauth.strategy"] @@ -35,8 +36,25 @@ class Auth::GoogleOAuth2Authenticator < Auth::ManagedAuthenticator # the JWT can fail due to clock skew, so let's skip it completely. # https://github.com/zquestz/omniauth-google-oauth2/pull/392 strategy.options[:skip_jwt] = true + strategy.options[:request_groups] = provides_groups? + + if provides_groups? + strategy.options[:scope] = "#{strategy_class::DEFAULT_SCOPE},#{strategy_class::GROUPS_SCOPE}" + end } } - omniauth.provider :google_oauth2, options + omniauth.provider strategy_class, options + end + + def after_authenticate(auth_token, existing_account: nil) + result = super + if provides_groups? && (groups = auth_token[:extra][:raw_groups]) + result.associated_groups = groups.map { |group| group.slice(:id, :name) } + end + result + end + + def provides_groups? + SiteSetting.google_oauth2_hd.present? && SiteSetting.google_oauth2_hd_groups end end diff --git a/lib/auth/managed_authenticator.rb b/lib/auth/managed_authenticator.rb index e82f1de96e1..a7753050b8c 100644 --- a/lib/auth/managed_authenticator.rb +++ b/lib/auth/managed_authenticator.rb @@ -113,14 +113,16 @@ class Auth::ManagedAuthenticator < Auth::Authenticator result end - def after_create_account(user, auth) - auth_token = auth[:extra_data] + def after_create_account(user, auth_result) + auth_token = auth_result[:extra_data] association = UserAssociatedAccount.find_or_initialize_by(provider_name: auth_token[:provider], provider_uid: auth_token[:uid]) association.user = user association.save! retrieve_avatar(user, association.info["image"]) retrieve_profile(user, association.info) + + auth_result.apply_associated_attributes! end def find_user_by_email(auth_token) diff --git a/lib/auth/omniauth_strategies/discourse_google_oauth2.rb b/lib/auth/omniauth_strategies/discourse_google_oauth2.rb new file mode 100644 index 00000000000..326e26d81be --- /dev/null +++ b/lib/auth/omniauth_strategies/discourse_google_oauth2.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +class Auth::OmniAuthStrategies + class DiscourseGoogleOauth2 < OmniAuth::Strategies::GoogleOauth2 + GROUPS_SCOPE ||= "admin.directory.group.readonly" + GROUPS_DOMAIN ||= "admin.googleapis.com" + GROUPS_PATH ||= "/admin/directory/v1/groups" + + def extra + hash = {} + hash[:raw_info] = raw_info + hash[:raw_groups] = raw_groups if options[:request_groups] + hash + end + + def raw_groups + @raw_groups ||= begin + groups = [] + page_token = nil + groups_url = "https://#{GROUPS_DOMAIN}#{GROUPS_PATH}" + + loop do + params = { + userKey: uid + } + params[:pageToken] = page_token if page_token + + response = access_token.get(groups_url, params: params, raise_errors: false) + + if response.status == 200 + response = response.parsed + groups.push(*response['groups']) + page_token = response['nextPageToken'] + break if page_token.nil? + else + Rails.logger.error("[Discourse Google OAuth2] failed to retrieve groups for #{uid} - status #{response.status}") + break + end + end + + groups + end + end + end +end diff --git a/lib/auth/result.rb b/lib/auth/result.rb index 2b0ba30c2f8..c63fb928696 100644 --- a/lib/auth/result.rb +++ b/lib/auth/result.rb @@ -21,7 +21,8 @@ class Auth::Result :omniauth_disallow_totp, :failed, :failed_reason, - :failed_code + :failed_code, + :associated_groups ] attr_accessor *ATTRIBUTES @@ -36,7 +37,8 @@ class Auth::Result :name, :authenticator_name, :extra_data, - :skip_email_validation + :skip_email_validation, + :associated_groups ] def [](key) @@ -94,6 +96,29 @@ class Auth::Result change_made end + def apply_associated_attributes! + if authenticator&.provides_groups? && !associated_groups.nil? + associated_group_ids = [] + + associated_groups.uniq.each do |associated_group| + begin + associated_group = AssociatedGroup.find_or_create_by( + name: associated_group[:name], + provider_id: associated_group[:id], + provider_name: extra_data[:provider] + ) + rescue ActiveRecord::RecordNotUnique + retry + end + + associated_group_ids.push(associated_group.id) + end + + user.update(associated_group_ids: associated_group_ids) + AssociatedGroup.where(id: associated_group_ids).update_all("last_used = CURRENT_TIMESTAMP") + end + end + def can_edit_name !SiteSetting.auth_overrides_name end @@ -167,6 +192,10 @@ class Auth::Result username || name || email end + def authenticator + @authenticator ||= Discourse.enabled_authenticators.find { |a| a.name == authenticator_name } + end + def resolve_username if staged_user if !username.present? || UserNameSuggester.fix_username(username) == staged_user.username diff --git a/lib/guardian/group_guardian.rb b/lib/guardian/group_guardian.rb index 36d4efc4cc4..1a869e4e7fe 100644 --- a/lib/guardian/group_guardian.rb +++ b/lib/guardian/group_guardian.rb @@ -36,4 +36,8 @@ module GroupGuardian SiteSetting.enable_personal_messages? && group.users.include?(user) end + + def can_associate_groups? + is_admin? && AssociatedGroup.has_provider? + end end diff --git a/lib/validators/google_oauth2_hd_groups_validator.rb b/lib/validators/google_oauth2_hd_groups_validator.rb new file mode 100644 index 00000000000..b4c3f91431e --- /dev/null +++ b/lib/validators/google_oauth2_hd_groups_validator.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class GoogleOauth2HdGroupsValidator + def initialize(opts = {}) + @opts = opts + end + + def valid_value?(value) + @valid = value == "f" || SiteSetting.google_oauth2_hd.present? + end + + def error_message + I18n.t("site_settings.errors.google_oauth2_hd_groups") if !@valid + end +end diff --git a/spec/components/auth/google_oauth2_authenticator_spec.rb b/spec/components/auth/google_oauth2_authenticator_spec.rb index 71bd5ccbc02..b82649070d8 100644 --- a/spec/components/auth/google_oauth2_authenticator_spec.rb +++ b/spec/components/auth/google_oauth2_authenticator_spec.rb @@ -3,7 +3,6 @@ require 'rails_helper' describe Auth::GoogleOAuth2Authenticator do - it 'does not look up user unless email is verified' do # note, emails that come back from google via omniauth are always valid # this protects against future regressions @@ -113,6 +112,61 @@ describe Auth::GoogleOAuth2Authenticator do expect(result.user).to eq(nil) expect(result.name).to eq("Jane Doe") end + + context "provides groups" do + before do + SiteSetting.google_oauth2_hd = "domain.com" + group1 = OmniAuth::AuthHash.new(id: "12345", name: "group1") + group2 = OmniAuth::AuthHash.new(id: "67890", name: "group2") + @groups = [group1, group2] + @groups_hash = OmniAuth::AuthHash.new( + provider: "google_oauth2", + uid: "123456789", + info: { + first_name: "Jane", + last_name: "Doe", + name: "Jane Doe", + email: "jane.doe@the.google.com" + }, + extra: { + raw_info: { + email: "jane.doe@the.google.com", + email_verified: true, + name: "Jane Doe" + }, + raw_groups: @groups + } + ) + end + + context "enabled" do + before do + SiteSetting.google_oauth2_hd_groups = true + end + + it "adds associated groups" do + result = described_class.new.after_authenticate(@groups_hash) + expect(result.associated_groups).to eq(@groups) + end + + it "handles a blank groups array" do + @groups_hash[:extra][:raw_groups] = [] + result = described_class.new.after_authenticate(@groups_hash) + expect(result.associated_groups).to eq([]) + end + end + + context "disabled" do + before do + SiteSetting.google_oauth2_hd_groups = false + end + + it "doesnt add associated groups" do + result = described_class.new.after_authenticate(@groups_hash) + expect(result.associated_groups).to eq(nil) + end + end + end end context 'revoke' do diff --git a/spec/components/auth/managed_authenticator_spec.rb b/spec/components/auth/managed_authenticator_spec.rb index f98419ac67b..2f6e371a0ec 100644 --- a/spec/components/auth/managed_authenticator_spec.rb +++ b/spec/components/auth/managed_authenticator_spec.rb @@ -38,6 +38,12 @@ describe Auth::ManagedAuthenticator do ) } + def create_auth_result(attrs) + auth_result = Auth::Result.new + attrs.each { |k, v| auth_result.send("#{k}=", v) } + auth_result + end + describe 'after_authenticate' do it 'can match account from an existing association' do user = Fabricate(:user) @@ -250,14 +256,14 @@ describe Auth::ManagedAuthenticator do let!(:association) { UserAssociatedAccount.create!(provider_name: 'myauth', provider_uid: "1234") } it "doesn't schedule with no image" do - expect { result = authenticator.after_create_account(user, extra_data: create_hash) } + expect { result = authenticator.after_create_account(user, create_auth_result(extra_data: create_hash)) } .to change { Jobs::DownloadAvatarFromUrl.jobs.count }.by(0) end it "schedules with image" do association.info["image"] = "https://some.domain/image.jpg" association.save! - expect { result = authenticator.after_create_account(user, extra_data: create_hash) } + expect { result = authenticator.after_create_account(user, create_auth_result(extra_data: create_hash)) } .to change { Jobs::DownloadAvatarFromUrl.jobs.count }.by(1) end end @@ -267,14 +273,14 @@ describe Auth::ManagedAuthenticator do let!(:association) { UserAssociatedAccount.create!(provider_name: 'myauth', provider_uid: "1234") } it "doesn't explode without profile" do - authenticator.after_create_account(user, extra_data: create_hash) + authenticator.after_create_account(user, create_auth_result(extra_data: create_hash)) end it "works with profile" do association.info["location"] = "DiscourseVille" association.info["description"] = "Online forum expert" association.save! - authenticator.after_create_account(user, extra_data: create_hash) + authenticator.after_create_account(user, create_auth_result(extra_data: create_hash)) expect(user.user_profile.bio_raw).to eq("Online forum expert") expect(user.user_profile.location).to eq("DiscourseVille") end diff --git a/spec/components/auth/omniauth_strategies/discourse_google_oauth2_spec.rb b/spec/components/auth/omniauth_strategies/discourse_google_oauth2_spec.rb new file mode 100644 index 00000000000..e5eda8ff6b0 --- /dev/null +++ b/spec/components/auth/omniauth_strategies/discourse_google_oauth2_spec.rb @@ -0,0 +1,126 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Auth::OmniAuthStrategies::DiscourseGoogleOauth2 do + let(:response_hash) do + { + email: 'user@domain.com', + email_verified: true + } + end + let(:groups) do + [ + { + id: "12345", + name: "group1" + }, + { + id: "67890", + name: "group2" + } + ] + end + let(:uid) { "12345" } + let(:domain) { "domain.com" } + + def build_response(body, code = 200) + [code, { 'Content-Type' => 'application/json' }, body.to_json] + end + + def build_client(groups_response) + OAuth2::Client.new('abc', 'def') do |builder| + builder.request :url_encoded + builder.adapter :test do |stub| + stub.get('/oauth2/v3/userinfo') { build_response(response_hash) } + stub.get(described_class::GROUPS_PATH) { groups_response } + end + end + end + + let(:successful_groups_client) do + build_client( + build_response( + groups: groups + ) + ) + end + + let(:unsuccessful_groups_client) do + build_client( + build_response( + error: { + code: 403, + message: "Not Authorized to access this resource/api" + } + ) + ) + end + + let(:successful_groups_token) do + OAuth2::AccessToken.from_hash(successful_groups_client, {}) + end + + let(:unsuccessful_groups_token) do + OAuth2::AccessToken.from_hash(unsuccessful_groups_client, {}) + end + + def app + lambda do |_env| + [200, {}, ["Hello."]] + end + end + + def build_strategy(access_token) + strategy = described_class.new(app, 'appid', 'secret', @options) + strategy.stubs(:uid).returns(uid) + strategy.stubs(:access_token).returns(access_token) + strategy + end + + before do + @options = {} + OmniAuth.config.test_mode = true + end + + after do + OmniAuth.config.test_mode = false + end + + context 'request_groups is true' do + before do + @options[:request_groups] = true + end + + context 'groups request successful' do + before do + @strategy = build_strategy(successful_groups_token) + end + + it 'should include users groups' do + expect(@strategy.extra[:raw_groups].map(&:symbolize_keys)).to eq(groups) + end + end + + context 'groups request unsuccessful' do + before do + @strategy = build_strategy(unsuccessful_groups_token) + end + + it 'users groups should be empty' do + expect(@strategy.extra[:raw_groups].empty?).to eq(true) + end + end + end + + context 'request_groups is not true' do + before do + @options[:request_groups] = false + @strategy = build_strategy(successful_groups_token) + end + + it 'should not include users groups' do + expect(@strategy.extra).not_to have_key(:raw_groups) + end + end +end diff --git a/spec/fabricators/associated_group_fabricator.rb b/spec/fabricators/associated_group_fabricator.rb new file mode 100644 index 00000000000..f7aff76893c --- /dev/null +++ b/spec/fabricators/associated_group_fabricator.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +Fabricator(:associated_group) do + name { sequence(:name) { |n| "group_#{n}" } } + provider_name 'google' + provider_id { SecureRandom.hex(20) } +end diff --git a/spec/models/associated_group_spec.rb b/spec/models/associated_group_spec.rb new file mode 100644 index 00000000000..3127b35e29d --- /dev/null +++ b/spec/models/associated_group_spec.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe AssociatedGroup do + let(:user) { Fabricate(:user) } + let(:associated_group) { Fabricate(:associated_group) } + let(:group) { Fabricate(:group) } + + it "generates a label" do + ag = described_class.new(name: "group1", provider_name: "google") + expect(ag.label).to eq("google:group1") + end + + it "detects whether any auth providers provide associated groups" do + SiteSetting.enable_google_oauth2_logins = true + SiteSetting.google_oauth2_hd = 'domain.com' + SiteSetting.google_oauth2_hd_groups = false + expect(described_class.has_provider?).to eq(false) + + SiteSetting.google_oauth2_hd_groups = true + expect(described_class.has_provider?).to eq(true) + end + + context "cleanup!" do + before do + associated_group.last_used = 8.days.ago + associated_group.save + end + + it "deletes associated groups not used in over a week" do + described_class.cleanup! + expect(described_class.exists?(associated_group.id)).to eq(false) + end + + it "doesnt delete associated groups associated with groups" do + GroupAssociatedGroup.create(group_id: group.id, associated_group_id: associated_group.id) + described_class.cleanup! + expect(described_class.exists?(associated_group.id)).to eq(true) + end + + it "doesnt delete associated groups associated with users" do + UserAssociatedGroup.create(user_id: user.id, associated_group_id: associated_group.id) + described_class.cleanup! + expect(described_class.exists?(associated_group.id)).to eq(true) + end + end +end diff --git a/spec/models/group_associated_group_spec.rb b/spec/models/group_associated_group_spec.rb new file mode 100644 index 00000000000..213a7afecfc --- /dev/null +++ b/spec/models/group_associated_group_spec.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe GroupAssociatedGroup do + let(:user) { Fabricate(:user) } + let(:group) { Fabricate(:group) } + let(:group2) { Fabricate(:group) } + let(:associated_group) { Fabricate(:associated_group) } + let(:associated_group2) { Fabricate(:associated_group) } + + before do + UserAssociatedGroup.create(user_id: user.id, associated_group_id: associated_group.id) + @gag = described_class.create(group_id: group.id, associated_group_id: associated_group.id) + end + + it "adds users to group when created" do + expect(group.users.include?(user)).to eq(true) + end + + it "removes users from group when destroyed" do + @gag.destroy! + expect(group.users.include?(user)).to eq(false) + end + + it "does not remove users with multiple associations to group when destroyed" do + UserAssociatedGroup.create(user_id: user.id, associated_group_id: associated_group2.id) + described_class.create(group_id: group.id, associated_group_id: associated_group2.id) + + @gag.destroy! + expect(group.users.include?(user)).to eq(true) + end + + it "removes users with multiple associations to other groups when destroyed" do + UserAssociatedGroup.create(user_id: user.id, associated_group_id: associated_group2.id) + described_class.create(group_id: group2.id, associated_group_id: associated_group2.id) + + @gag.destroy! + expect(group.users.include?(user)).to eq(false) + end +end diff --git a/spec/models/user_associated_group_spec.rb b/spec/models/user_associated_group_spec.rb new file mode 100644 index 00000000000..c686cea9386 --- /dev/null +++ b/spec/models/user_associated_group_spec.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe UserAssociatedGroup do + let(:user) { Fabricate(:user) } + let(:group) { Fabricate(:group) } + let(:group2) { Fabricate(:group) } + let(:associated_group) { Fabricate(:associated_group) } + let(:associated_group2) { Fabricate(:associated_group) } + + before do + GroupAssociatedGroup.create(group_id: group.id, associated_group_id: associated_group.id) + @uag = described_class.create(user_id: user.id, associated_group_id: associated_group.id) + end + + it "adds user to group when created" do + expect(group.users.include?(user)).to eq(true) + end + + it "removes user from group when destroyed" do + @uag.destroy! + expect(group.users.include?(user)).to eq(false) + end + + it "does not remove user with multiple associations from group when destroyed" do + GroupAssociatedGroup.create(group_id: group.id, associated_group_id: associated_group2.id) + described_class.create(user_id: user.id, associated_group_id: associated_group2.id) + + @uag.destroy! + expect(group.users.include?(user)).to eq(true) + end + + it "removes users with multiple associations to other groups when destroyed" do + GroupAssociatedGroup.create(group_id: group2.id, associated_group_id: associated_group2.id) + described_class.create(user_id: user.id, associated_group_id: associated_group2.id) + + @uag.destroy! + expect(group.users.include?(user)).to eq(false) + end +end diff --git a/spec/requests/api/schemas/json/group_response.json b/spec/requests/api/schemas/json/group_response.json index c4b31112f60..a3bf6d151c4 100644 --- a/spec/requests/api/schemas/json/group_response.json +++ b/spec/requests/api/schemas/json/group_response.json @@ -251,6 +251,12 @@ "allow_unknown_sender_topic_replies": { "type": "boolean" }, + "associated_group_ids": { + "type": "array", + "items": [ + + ] + }, "watching_category_ids": { "type": "array", "items": [ diff --git a/spec/requests/api/schemas/json/site_response.json b/spec/requests/api/schemas/json/site_response.json index 82ae9485d0c..ce4bb9c6f1c 100644 --- a/spec/requests/api/schemas/json/site_response.json +++ b/spec/requests/api/schemas/json/site_response.json @@ -358,6 +358,9 @@ "wizard_required": { "type": "boolean" }, + "can_associate_groups": { + "type": "boolean" + }, "topic_featured_link_allowed_category_ids": { "type": "array", "items": [ diff --git a/spec/requests/omniauth_callbacks_controller_spec.rb b/spec/requests/omniauth_callbacks_controller_spec.rb index 8c594d555b2..8d2a0c2a5ad 100644 --- a/spec/requests/omniauth_callbacks_controller_spec.rb +++ b/spec/requests/omniauth_callbacks_controller_spec.rb @@ -694,6 +694,91 @@ RSpec.describe Users::OmniauthCallbacksController do expect(data["username"]).to eq(fixed_username) end + + context "groups are enabled" do + let(:strategy_class) { Auth::OmniAuthStrategies::DiscourseGoogleOauth2 } + let(:groups_url) { "#{strategy_class::GROUPS_DOMAIN}#{strategy_class::GROUPS_PATH}" } + let(:groups_scope) { strategy_class::DEFAULT_SCOPE + strategy_class::GROUPS_SCOPE } + let(:group1) { { id: "12345", name: "group1" } } + let(:group2) { { id: "67890", name: "group2" } } + let(:uid) { "12345" } + let(:token) { "1245678" } + let(:domain) { "mydomain.com" } + + def mock_omniauth_for_groups(groups) + raw_groups = groups.map { |group| OmniAuth::AuthHash.new(group) } + mock_auth = OmniAuth.config.mock_auth[:google_oauth2] + mock_auth[:extra][:raw_groups] = raw_groups + OmniAuth.config.mock_auth[:google_oauth2] = mock_auth + Rails.application.env_config["omniauth.auth"] = mock_auth + end + + before do + SiteSetting.google_oauth2_hd = domain + SiteSetting.google_oauth2_hd_groups = true + end + + it "updates associated groups" do + mock_omniauth_for_groups([group1, group2]) + get "/auth/google_oauth2/callback.json", params: { + scope: groups_scope.split(' '), + code: 'abcde', + hd: domain + } + expect(response.status).to eq(302) + + associated_groups = AssociatedGroup.where(provider_name: 'google_oauth2') + expect(associated_groups.length).to eq(2) + expect(associated_groups.exists?(name: group1[:name])).to eq(true) + expect(associated_groups.exists?(name: group2[:name])).to eq(true) + + user_associated_groups = UserAssociatedGroup.where(user_id: user.id) + expect(user_associated_groups.length).to eq(2) + expect(user_associated_groups.exists?(associated_group_id: associated_groups.first.id)).to eq(true) + expect(user_associated_groups.exists?(associated_group_id: associated_groups.second.id)).to eq(true) + + mock_omniauth_for_groups([group1]) + get "/auth/google_oauth2/callback.json", params: { + scope: groups_scope.split(' '), + code: 'abcde', + hd: domain + } + expect(response.status).to eq(302) + + user_associated_groups = UserAssociatedGroup.where(user_id: user.id) + expect(user_associated_groups.length).to eq(1) + expect(user_associated_groups.exists?(associated_group_id: associated_groups.first.id)).to eq(true) + expect(user_associated_groups.exists?(associated_group_id: associated_groups.second.id)).to eq(false) + + mock_omniauth_for_groups([]) + get "/auth/google_oauth2/callback.json", params: { + scope: groups_scope.split(' '), + code: 'abcde', + hd: domain + } + expect(response.status).to eq(302) + + user_associated_groups = UserAssociatedGroup.where(user_id: user.id) + expect(user_associated_groups.length).to eq(0) + expect(user_associated_groups.exists?(associated_group_id: associated_groups.first.id)).to eq(false) + expect(user_associated_groups.exists?(associated_group_id: associated_groups.second.id)).to eq(false) + end + + it "handles failure to retrieve groups" do + mock_omniauth_for_groups([]) + + get "/auth/google_oauth2/callback.json", params: { + scope: groups_scope.split(' '), + code: 'abcde', + hd: domain + } + + expect(response.status).to eq(302) + + associated_groups = AssociatedGroup.where(provider_name: 'google_oauth2') + expect(associated_groups.exists?).to eq(false) + end + end end context 'when attempting reconnect' do