(quality) refactored SingleSignOnConcern to match rubocop rules

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 - Allow searching by username (#401)
 - Fix a bug: portuguese time formatting (#405)
+- Fix a bug: admin users groups being overriden by SSO group_id (#404)
 - Fix a security issue: updated nokogiri to 1.13.9 to fix [GHSA-2qc6-mcvw-92cw](https://github.com/advisories/GHSA-2qc6-mcvw-92cw)
 
 ## v5.4.25 2022 October 19

app/models/concerns/single_sign_on_concern.rb

@@ -13,27 +13,8 @@ module SingleSignOnConcern
     ## Retrieve the requested data in the User and user's Profile tables
     ## @param sso_mapping {String} must be of form 'user._field_' or 'profile._field_'. Eg. 'user.email'
     def get_data_from_sso_mapping(sso_mapping)
-      parsed = /^(user|profile)\.(.+)$/.match(sso_mapping)
+      service = UserSetterService.new(self)
-      if parsed[1] == 'user'
+      service.read_attribute(sso_mapping)
-        self[parsed[2].to_sym]
-      elsif parsed[1] == 'profile'
-        case sso_mapping
-        when 'profile.avatar'
-          profile.user_avatar.remote_attachment_url
-        when 'profile.address'
-          invoicing_profile.address&.address
-        when 'profile.organization_name'
-          invoicing_profile.organization&.name
-        when 'profile.organization_address'
-          invoicing_profile.organization&.address&.address
-        when 'profile.gender'
-          statistic_profile.gender
-        when 'profile.birthday'
-          statistic_profile.birthday
-        else
-          profile[parsed[2].to_sym]
-        end
-      end
     end
 
     ## Set some data on the current user, according to the sso_key given
@@ -42,36 +23,8 @@ module SingleSignOnConcern
     def set_data_from_sso_mapping(sso_mapping, data)
       return if data.nil? || data.blank?
 
-      if sso_mapping.to_s.start_with? 'user.'
+      service = UserSetterService.new(self)
-        self[sso_mapping[5..-1].to_sym] = data
+      service.assign_attibute(sso_mapping, data)
-      elsif sso_mapping.to_s.start_with? 'profile.'
-        case sso_mapping.to_s
-        when 'profile.avatar'
-          profile.user_avatar ||= UserAvatar.new
-          profile.user_avatar.remote_attachment_url = data
-        when 'profile.address'
-          self.invoicing_profile ||= InvoicingProfile.new
-          self.invoicing_profile.address ||= Address.new
-          self.invoicing_profile.address.address = data
-        when 'profile.organization_name'
-          self.invoicing_profile ||= InvoicingProfile.new
-          self.invoicing_profile.organization ||= Organization.new
-          self.invoicing_profile.organization.name = data
-        when 'profile.organization_address'
-          self.invoicing_profile ||= InvoicingProfile.new
-          self.invoicing_profile.organization ||= Organization.new
-          self.invoicing_profile.organization.address ||= Address.new
-          self.invoicing_profile.organization.address.address = data
-        when 'profile.gender'
-          self.statistic_profile ||= StatisticProfile.new
-          self.statistic_profile.gender = data
-        when 'profile.birthday'
-          self.statistic_profile ||= StatisticProfile.new
-          self.statistic_profile.birthday = data
-        else
-          profile[sso_mapping[8..-1].to_sym] = data
-        end
-      end
 
       return if mapped_from_sso&.include?(sso_mapping)
 
@@ -80,7 +33,7 @@ module SingleSignOnConcern
 
     ## used to allow the migration of existing users between authentication providers
     def generate_auth_migration_token
-      update_attributes(auth_token: Devise.friendly_token)
+      update(auth_token: Devise.friendly_token)
     end
 
     ## link the current user to the given provider (omniauth attributes hash)
@@ -93,7 +46,7 @@ module SingleSignOnConcern
         raise DuplicateIndexError, "This #{active_provider.name} account is already linked to an existing user"
       end
 
-      update_attributes(provider: auth.provider, uid: auth.uid, auth_token: nil)
+      update(provider: auth.provider, uid: auth.uid, auth_token: nil)
     end
 
     ## Merge the provided User's SSO details into the current user and drop the provided user to ensure the unity
@@ -118,13 +71,16 @@ module SingleSignOnConcern
 
       # update the user's profile to set the data managed by the SSO
       auth_provider = AuthProvider.from_strategy_name(sso_user.provider)
-      logger.debug "found auth_provider=#{auth_provider.name}"
+      logger.debug "found auth_provider=#{auth_provider&.name}"
-      auth_provider.sso_fields.each do |field|
+      auth_provider&.sso_fields&.each do |field|
         value = sso_user.get_data_from_sso_mapping(field)
         logger.debug "mapping sso field #{field} with value=#{value}"
-        # we do not merge the email field if its end with the special value '-duplicate' as this means
+        # We do not merge the email field if its end with the special value '-duplicate' as this means
-        # that the user is currently merging with the account that have the same email than the sso
+        # that the user is currently merging with the account that have the same email than the sso.
-        set_data_from_sso_mapping(field, value) unless (field == 'user.email' && value.end_with?('-duplicate')) || (field == 'user.group_id' && self.admin?)
+        # Moreover, if the user is an administrator, we must keep him in his group
+        unless (field == 'user.email' && value.end_with?('-duplicate')) || (field == 'user.group_id' && admin?)
+          set_data_from_sso_mapping(field, value)
+        end
       end
 
       # run the account transfer in an SQL transaction to ensure data integrity

app/services/user_getter_service.rb

@@ -0,0 +1,35 @@
+# frozen_string_literal: true
+
+# helpers to read data from a user
+class UserGetterService
+  def initialize(user)
+    @user = user
+  end
+
+  def read_attribute(attribute)
+    parsed = /^(user|profile)\.(.+)$/.match(attribute)
+    case parsed[1]
+    when 'user'
+      @user[parsed[2].to_sym]
+    when 'profile'
+      case attribute
+      when 'profile.avatar'
+        @user.profile.user_avatar.remote_attachment_url
+      when 'profile.address'
+        @user.invoicing_profile.address&.address
+      when 'profile.organization_name'
+        @user.invoicing_profile.organization&.name
+      when 'profile.organization_address'
+        @user.invoicing_profile.organization&.address&.address
+      when 'profile.gender'
+        @user.statistic_profile.gender
+      when 'profile.birthday'
+        @user.statistic_profile.birthday
+      else
+        @user.profile[parsed[2].to_sym]
+      end
+    else
+      nil
+    end
+  end
+end

app/services/user_service.rb

@@ -2,7 +2,8 @@
 
 # helpers for managing users with special roles
 class UserService
-  def self.create_partner(params)
+  class << self
+    def create_partner(params)
       generated_password = SecurePassword.generate
       group_id = Group.first.id
       user = User.new(
@@ -30,7 +31,7 @@ class UserService
       { saved: saved, user: user }
     end
 
-  def self.create_admin(params)
+    def create_admin(params)
       generated_password = SecurePassword.generate
       admin = User.new(params.merge(password: generated_password))
       admin.send :set_slug
@@ -51,7 +52,7 @@ class UserService
       { saved: saved, user: admin }
     end
 
-  def self.create_manager(params)
+    def create_manager(params)
       generated_password = SecurePassword.generate
       manager = User.new(params.merge(password: generated_password))
       manager.send :set_slug
@@ -65,4 +66,5 @@ class UserService
       end
       { saved: saved, user: manager }
     end
+  end
 end

app/services/user_setter_service.rb

@@ -0,0 +1,73 @@
+# frozen_string_literal: true
+
+# helpers to assign data to a user
+class UserSetterService
+  def initialize(user)
+    @user = user
+  end
+
+  def assign_avatar(data)
+    @user.profile.user_avatar ||= UserAvatar.new
+    @user.profile.user_avatar.remote_attachment_url = data
+  end
+
+  def assign_address(data)
+    @user.invoicing_profile ||= InvoicingProfile.new
+    @user.invoicing_profile.address ||= Address.new
+    @user.invoicing_profile.address.address = data
+  end
+
+  def assign_organization_name(data)
+    @user.invoicing_profile ||= InvoicingProfile.new
+    @user.invoicing_profile.organization ||= Organization.new
+    @user.invoicing_profile.organization.name = data
+  end
+
+  def assign_organization_address(data)
+    @user.invoicing_profile ||= InvoicingProfile.new
+    @user.invoicing_profile.organization ||= Organization.new
+    @user.invoicing_profile.organization.address ||= Address.new
+    @user.invoicing_profile.organization.address.address = data
+  end
+
+  def assign_gender(data)
+    @user.statistic_profile ||= StatisticProfile.new
+    @user.statistic_profile.gender = data
+  end
+
+  def assign_birthday(data)
+    @user.statistic_profile ||= StatisticProfile.new
+    @user.statistic_profile.birthday = data
+  end
+
+  def assign_profile_attribute(attribute, data)
+    @user.profile[attribute[8..].to_sym] = data
+  end
+
+  def assign_user_attribute(attribute, data)
+    @user[attribute[5..].to_sym] = data
+  end
+
+  def assign_attibute(attribute, data)
+    if attribute.to_s.start_with? 'user.'
+      assign_user_attribute(attribute, data)
+    elsif attribute.to_s.start_with? 'profile.'
+      case attribute.to_s
+      when 'profile.avatar'
+        assign_avatar(data)
+      when 'profile.address'
+        assign_address(data)
+      when 'profile.organization_name'
+        assign_organization_name(data)
+      when 'profile.organization_address'
+        assign_organization_address(data)
+      when 'profile.gender'
+        assign_gender(data)
+      when 'profile.birthday'
+        assign_birthday(data)
+      else
+        assign_profile_attribute(attribute, data)
+      end
+    end
+  end
+end