FEATURE: Allow multiple attributes for group sync and also using group full_name (#127)

This commit
- 1️⃣ extends the `saml_groups_attribute` to accept an array of attributes, and 
- 2️⃣ adds a new setting: `saml_groups_use_full_name`.
- 3️⃣ it ensures the group sync feature removes the groups from the old attributes if it exists

See more here: #127

Author: nattsw

README.md

@@ -42,10 +42,11 @@ Add the following settings to your `discourse.conf` file:
 ### Group sync
 
 - `DISCOURSE_SAML_SYNC_GROUPS`: Sync groups. Defaults to false.
-- `DISCOURSE_SAML_GROUPS_ATTRIBUTE`: SAML attribute to use for group sync. Defaults to `memberOf`
+- `DISCOURSE_SAML_GROUPS_ATTRIBUTE`: SAML attribute to use for group sync. Defaults to `memberOf`, and accepts '|' separated attributes `Country|Department`.
 - `DISCOURSE_SAML_GROUPS_FULLSYNC`: Should the assigned groups be completely synced including adding AND removing groups based on the IDP? Defaults to false. If set to true, `DISCOURSE_SAML_SYNC_GROUPS_LIST` and SAML attribute `groups_to_add`/`groups_to_remove` are not used.
 - `DISCOURSE_SAML_GROUPS_LDAP_LEAFCN`: If your IDP transmits `cn=groupname,cn=groups,dc=example,dc=com` you can set this to true to use only `groupname`. This is useful if you want to keep the standard group name length of Discourse (20 characters).
 - `DISCOURSE_SAML_SYNC_GROUPS_LIST`: Groups mentioned in this list are synced if they are referenced by the IDP (in `memberOf` SAML attribue). Any other groups will not be removed/updated.
+- `DISCOURSE_SAML_GROUPS_USE_FULL_NAME`: If set to true, will match groups based on Discourse Group `full_name` instead of the default `name`. This allows usage of group names with spaces in them, e.g. "North Africa" instead of "north_africa".
 
 ### Other Supported settings
 

config/settings.yml

@@ -59,7 +59,10 @@ saml:
 
   saml_sync_groups: false
   saml_groups_fullsync: false
-  saml_groups_attribute: "memberOf"
+  saml_groups_attribute:
+    type: list
+    default: "memberOf"
+  saml_groups_use_full_name: false
   saml_groups_ldap_leafcn: false
   saml_sync_groups_list:
     type: list

lib/saml_authenticator.rb

@@ -114,6 +114,8 @@ def after_authenticate(auth)
 
     auth[:uid] = attributes.single("uid") || auth[:uid] if setting(:use_attributes_uid)
     uid = auth[:uid]
+    previous_attributes =
+      UserAssociatedAccount.find_by(provider_name: name, provider_uid: uid)&.extra
 
     auth.info[:email] ||= uid if uid.to_s&.include?("@")
 
@@ -141,7 +143,7 @@ def after_authenticate(auth)
         result.email_valid
     else
       user = result.user
-      sync_groups(user, attributes, info)
+      sync_groups(user, attributes, info, previous_attributes)
       sync_custom_fields(user, attributes, info)
       sync_moderator(user, attributes)
       sync_admin(user, attributes)
@@ -205,51 +207,69 @@ def auto_create_account(result, uid)
     end
   end
 
-  def sync_groups(user, attributes, info)
+  def sync_groups(user, attributes, info, previous_attributes = nil)
     return if setting(:sync_groups).blank?
 
     groups_fullsync = setting(:groups_fullsync)
-    raw_group_list = attributes.multi(setting(:groups_attribute)) || []
-    user_group_list = raw_group_list.map { |g| g.downcase.split(",") }.flatten
+    groups_attributes = setting(:groups_attribute).split("|")
+    group_match_column = setting(:groups_use_full_name) ? "full_name" : "name"
+
+    groups_from_groups_attributes =
+      groups_attributes
+        .flat_map { |attr| attributes.multi(attr.strip) || [] }
+        .compact
+        .map { |g| g.downcase.split(",") }
+        .flatten
+        .uniq
 
     if setting(:groups_ldap_leafcn).present?
       # Change cn=groupname,cn=groups,dc=example,dc=com to groupname
-      user_group_list = user_group_list.map { |group| group.split("=", 2).last }
+      groups_from_groups_attributes =
+        groups_from_groups_attributes.map { |group| group.split("=", 2).last }
     end
 
     if groups_fullsync
-      user_has_groups = user.groups.where(automatic: false).pluck(:name).map(&:downcase)
-      groups_to_add = user_group_list - user_has_groups
-      groups_to_remove = user_has_groups - user_group_list if user_has_groups.present?
-    else
-      total_group_list = setting(:sync_groups_list).split("|").map(&:downcase)
+      user_has_groups =
+        user.groups.where(automatic: false).pluck(group_match_column).compact.map(&:downcase)
 
+      groups_to_add = groups_from_groups_attributes - user_has_groups
+      groups_to_remove = user_has_groups - groups_from_groups_attributes if user_has_groups.present?
+    else
       groups_to_add = info["groups_to_add"] || attributes.multi("groups_to_add")&.join(",") || ""
       groups_to_add = groups_to_add.downcase.split(",")
-      groups_to_add += user_group_list
+      groups_to_add += groups_from_groups_attributes
 
       groups_to_remove =
         info["groups_to_remove"] || attributes.multi("groups_to_remove")&.join(",") || ""
       groups_to_remove = groups_to_remove.downcase.split(",")
 
-      if total_group_list.present?
-        groups_to_add = total_group_list & groups_to_add
+      groups_from_attributes_before_sync =
+        get_groups_from_attributes(user:, groups_attributes:, previous_attributes:)
+      groups_to_remove += (groups_from_attributes_before_sync - groups_from_groups_attributes)
+
+      groups_allowlist = setting(:sync_groups_list).split("|").map(&:downcase)
+      if groups_allowlist.present?
+        groups_to_add = groups_allowlist & groups_to_add
 
         removable_groups = groups_to_remove.dup
-        groups_to_remove = total_group_list - groups_to_add
+        groups_to_remove = groups_allowlist - groups_to_add
         groups_to_remove &= removable_groups if removable_groups.present?
       end
     end
 
-    return if user_group_list.blank? && groups_to_add.blank? && groups_to_remove.blank?
+    return if groups_to_add.blank? && groups_to_remove.blank?
 
-    Group
-      .where("LOWER(name) IN (?) AND NOT automatic", groups_to_add)
-      .each { |group| group.add user }
+    if groups_to_add.present?
+      Group
+        .where("LOWER(#{group_match_column}) IN (?) AND NOT automatic", groups_to_add)
+        .each { |group| group.add user }
+    end
 
-    Group
-      .where("LOWER(name) IN (?) AND NOT automatic", groups_to_remove)
-      .each { |group| group.remove user }
+    if groups_to_remove.present?
+      Group
+        .where("LOWER(#{group_match_column}) IN (?) AND NOT automatic", groups_to_remove)
+        .each { |group| group.remove user }
+    end
   end
 
   def sync_custom_fields(user, attributes, info)
@@ -355,6 +375,16 @@ def self.saml_base_url
 
   private
 
+  def get_groups_from_attributes(user:, groups_attributes:, previous_attributes: nil)
+    return [] unless previous_attributes.is_a?(Hash)
+
+    groups_attributes
+      .flat_map { |attr| Array(previous_attributes.dig("raw_info", attr)) }
+      .compact
+      .uniq
+      .map(&:downcase)
+  end
+
   def idp_cert_multi
     return if setting(:cert_multi).blank?
 

spec/saml_authenticator_spec.rb

@@ -330,8 +330,10 @@ def auth_hash(attributes)
     describe "Group Syncing" do
       fab!(:group1) { Fabricate(:group, name: "uno", full_name: "Group One") }
       fab!(:group2) { Fabricate(:group, name: "dos", full_name: "Group Two") }
-      fab!(:group3) { Fabricate(:group, name: "tres", full_name: "Group Three") }
-      fab!(:original_group) { Fabricate(:group, name: "original_group").tap { |g| g.add(user) } }
+      fab!(:group_without_fullname) { Fabricate(:group, name: "tres") }
+      fab!(:original_group) do
+        Fabricate(:group, name: "original_group", full_name: "The Origin").tap { |g| g.add(user) }
+      end
 
       before { SiteSetting.saml_sync_groups = true }
 
@@ -340,38 +342,45 @@ def auth_hash(attributes)
           hash =
             auth_hash(
               "memberOf" => [group1.name, group2.name],
-              "groups_to_add" => [group3.name],
+              "groups_to_add" => [group_without_fullname.name],
               "groups_to_remove" => [original_group.name],
             )
 
           result = authenticator.after_authenticate(hash)
           expect(result.user.groups.pluck(:name)).to contain_exactly(
             group1.name,
             group2.name,
-            group3.name,
+            group_without_fullname.name,
           )
         end
 
         it "sync users to the given groups within scope" do
-          SiteSetting.saml_sync_groups_list = [group2.name, group3.name, original_group.name].join(
-            "|",
-          )
+          SiteSetting.saml_sync_groups_list = [
+            group2.name,
+            group_without_fullname.name,
+            original_group.name,
+          ].join("|")
           hash =
             auth_hash(
               "memberOf" => [group1.name, group2.name],
-              "groups_to_add" => [group3.name],
+              "groups_to_add" => [group_without_fullname.name],
               "groups_to_remove" => [original_group.name],
             )
 
           result = authenticator.after_authenticate(hash)
-          expect(result.user.groups.pluck(:name)).to contain_exactly(group2.name, group3.name)
+          expect(result.user.groups.pluck(:name)).to contain_exactly(
+            group2.name,
+            group_without_fullname.name,
+          )
         end
       end
 
       describe "sync_groups with LDAP leaf cn" do
         let!(:group1_ldap) { "cn=#{group1.name},cn=groups,dc=example,dc=com" }
         let!(:group2_ldap) { "cn=#{group2.name},cn=groups,dc=example,dc=com" }
-        let!(:group3_ldap) { "cn=#{group3.name},cn=groups,dc=example,dc=com" }
+        let!(:group_without_fullname_ldap) do
+          "cn=#{group_without_fullname.name},cn=groups,dc=example,dc=com"
+        end
         let!(:original_group_ldap) { "cn=#{original_group.name},cn=groups,dc=example,dc=com" }
 
         before { SiteSetting.saml_groups_ldap_leafcn = true }
@@ -380,7 +389,7 @@ def auth_hash(attributes)
           hash =
             auth_hash(
               "memberOf" => [group1_ldap, group2_ldap],
-              "groups_to_add" => [group3.name],
+              "groups_to_add" => [group_without_fullname.name],
               "groups_to_remove" => [original_group.name],
             )
 
@@ -390,24 +399,29 @@ def auth_hash(attributes)
           expect(result.user.groups.pluck(:name)).to contain_exactly(
             group1.name,
             group2.name,
-            group3.name,
+            group_without_fullname.name,
           )
         end
 
         it "sync users to the groups within scope" do
-          SiteSetting.saml_sync_groups_list = [group2.name, group3.name, original_group.name].join(
-            "|",
-          )
+          SiteSetting.saml_sync_groups_list = [
+            group2.name,
+            group_without_fullname.name,
+            original_group.name,
+          ].join("|")
 
           hash =
             auth_hash(
               "memberOf" => [group1_ldap, group2_ldap],
-              "groups_to_add" => [group3.name],
+              "groups_to_add" => [group_without_fullname.name],
               "groups_to_remove" => [original_group.name],
             )
 
           result = authenticator.after_authenticate(hash)
-          expect(result.user.groups.pluck(:name)).to contain_exactly(group2.name, group3.name)
+          expect(result.user.groups.pluck(:name)).to contain_exactly(
+            group2.name,
+            group_without_fullname.name,
+          )
         end
       end
 
@@ -425,20 +439,111 @@ def auth_hash(attributes)
         end
 
         it "full sync, ignoring values in group list and groups_to_add/groups_to_remove" do
-          SiteSetting.saml_sync_groups_list = [group2.name, group3.name].join("|")
+          SiteSetting.saml_sync_groups_list = [group2.name, group_without_fullname.name].join("|")
 
           hash =
             auth_hash(
               "memberOf" => [group1.name, group2.name],
               "groups_to_add" => [original_group.name],
-              "groups_to_remove" => [group3.name],
+              "groups_to_remove" => [group_without_fullname.name],
             )
 
           result = authenticator.after_authenticate(hash)
 
           expect(result.user.groups.pluck(:name)).to contain_exactly(group1.name, group2.name)
         end
       end
+
+      describe "saml_groups_attribute" do
+        it "syncs groups from the saml_groups_attribute setting" do
+          SiteSetting.saml_groups_attribute = "notTheDefault"
+          hash = auth_hash("notTheDefault" => [group1.name, group2.name])
+
+          result = authenticator.after_authenticate(hash)
+          expect(result.user.groups.pluck(:name)).to contain_exactly(
+            original_group.name,
+            group1.name,
+            group2.name,
+          )
+        end
+
+        it "removes groups from the previously saved saml_groups_attributes in raw_info" do
+          SiteSetting.saml_groups_attribute = "notTheDefault"
+
+          # user's existing group associations and user_associated_account
+          group1.add(user)
+          user.user_associated_accounts.create!(
+            provider_name: "saml",
+            provider_uid: uid,
+            user:,
+            extra: {
+              raw_info: {
+                "memberOf" => [group_without_fullname.name], # this is ignored as it is not the correct attribute
+                "notTheDefault" => [group1.name], # this is the correct attribute
+              },
+            },
+          )
+
+          # new auth hash with a different group
+          hash = auth_hash("notTheDefault" => [group2.name])
+
+          result = authenticator.after_authenticate(hash)
+          expect(result.user.groups.pluck(:name)).to contain_exactly(
+            # group1 should be removed
+            original_group.name,
+            group2.name,
+          )
+        end
+
+        it "allows the attribute to specify an array, and assigns groups from those attributes" do
+          SiteSetting.saml_groups_attribute = "Country|Hemisphere"
+          hash =
+            auth_hash(
+              "Country" => [group1.name, group2.name],
+              "Hemisphere" => [group_without_fullname.name],
+            )
+
+          result = authenticator.after_authenticate(hash)
+          expect(result.user.groups.pluck(:name)).to contain_exactly(
+            original_group.name,
+            group1.name,
+            group2.name,
+            group_without_fullname.name,
+          )
+        end
+      end
+
+      describe "saml_groups_use_full_name" do
+        before { SiteSetting.saml_groups_use_full_name = true }
+
+        it "adds users to groups based on group's case insensitive full_names" do
+          SiteSetting.saml_groups_attribute = "oneAttribute|twoAttribute" # ensure compat
+          SiteSetting.saml_sync_groups_list = [group1.full_name, group2.full_name].join("|") # ensure compat
+
+          hash =
+            auth_hash(
+              "oneAttribute" => [group1.full_name.upcase, "I don't exist"],
+              "twoAttribute" => [group2.full_name],
+            )
+
+          result = authenticator.after_authenticate(hash)
+          expect(result.user.groups.pluck(:name)).to contain_exactly(
+            group1.name,
+            group2.name,
+            original_group.name,
+          )
+        end
+
+        it "is compatible with full_sync" do
+          SiteSetting.saml_groups_use_full_name = true
+          SiteSetting.saml_groups_fullsync = true
+
+          hash = auth_hash("memberOf" => [group1.full_name])
+
+          result = authenticator.after_authenticate(hash)
+          expect(result.user.groups.pluck(:name)).to contain_exactly(group1.name)
+        end
+      end
     end
 
     describe "set moderator" do