From fdac8168ef04b48667df435952df23de21e389bd Mon Sep 17 00:00:00 2001 From: Sylvain Date: Mon, 24 Oct 2022 11:42:47 +0200 Subject: [PATCH] (quality) refactored SingleSignOnConcern to match rubocop rules --- CHANGELOG.md | 1 + app/models/concerns/single_sign_on_concern.rb | 72 +++--------- app/services/user_getter_service.rb | 35 ++++++ app/services/user_service.rb | 106 +++++++++--------- app/services/user_setter_service.rb | 73 ++++++++++++ 5 files changed, 177 insertions(+), 110 deletions(-) create mode 100644 app/services/user_getter_service.rb create mode 100644 app/services/user_setter_service.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 9332e8557..048135e76 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/app/models/concerns/single_sign_on_concern.rb b/app/models/concerns/single_sign_on_concern.rb index ec19d12e2..d4642b7cb 100644 --- a/app/models/concerns/single_sign_on_concern.rb +++ b/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) - if parsed[1] == 'user' - 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 + service = UserSetterService.new(self) + service.read_attribute(sso_mapping) 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.' - self[sso_mapping[5..-1].to_sym] = 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 + service = UserSetterService.new(self) + service.assign_attibute(sso_mapping, data) 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}" - auth_provider.sso_fields.each do |field| + logger.debug "found auth_provider=#{auth_provider&.name}" + 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 - # 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?) + # 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. + # 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 diff --git a/app/services/user_getter_service.rb b/app/services/user_getter_service.rb new file mode 100644 index 000000000..4634dd753 --- /dev/null +++ b/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 diff --git a/app/services/user_service.rb b/app/services/user_service.rb index d117193fb..23973e80b 100644 --- a/app/services/user_service.rb +++ b/app/services/user_service.rb @@ -2,67 +2,69 @@ # helpers for managing users with special roles class UserService - def self.create_partner(params) - generated_password = SecurePassword.generate - group_id = Group.first.id - user = User.new( - email: params[:email], - username: "#{params[:first_name]}#{params[:last_name]}".parameterize, - password: generated_password, - password_confirmation: generated_password, - group_id: group_id - ) - user.build_profile( - first_name: params[:first_name], - last_name: params[:last_name], - phone: '0000000000' - ) - user.build_statistic_profile( - gender: true, - birthday: DateTime.current - ) + class << self + def create_partner(params) + generated_password = SecurePassword.generate + group_id = Group.first.id + user = User.new( + email: params[:email], + username: "#{params[:first_name]}#{params[:last_name]}".parameterize, + password: generated_password, + password_confirmation: generated_password, + group_id: group_id + ) + user.build_profile( + first_name: params[:first_name], + last_name: params[:last_name], + phone: '0000000000' + ) + user.build_statistic_profile( + gender: true, + birthday: DateTime.current + ) - saved = user.save - if saved - user.remove_role :member - user.add_role :partner + saved = user.save + if saved + user.remove_role :member + user.add_role :partner + end + { saved: saved, user: user } end - { saved: saved, user: user } - end - def self.create_admin(params) - generated_password = SecurePassword.generate - admin = User.new(params.merge(password: generated_password)) - admin.send :set_slug + def create_admin(params) + generated_password = SecurePassword.generate + admin = User.new(params.merge(password: generated_password)) + admin.send :set_slug - # we associate the admin group to prevent linking any other 'normal' group (which won't be deletable afterwards) - admin.group = Group.find_by(slug: 'admins') + # we associate the admin group to prevent linking any other 'normal' group (which won't be deletable afterwards) + admin.group = Group.find_by(slug: 'admins') - # if the authentication is made through an SSO, generate a migration token - admin.generate_auth_migration_token unless AuthProvider.active.providable_type == DatabaseProvider.name + # if the authentication is made through an SSO, generate a migration token + admin.generate_auth_migration_token unless AuthProvider.active.providable_type == DatabaseProvider.name - saved = admin.save - if saved - admin.send_confirmation_instructions - admin.add_role(:admin) - admin.remove_role(:member) - UsersMailer.notify_user_account_created(admin, generated_password).deliver_later + saved = admin.save + if saved + admin.send_confirmation_instructions + admin.add_role(:admin) + admin.remove_role(:member) + UsersMailer.notify_user_account_created(admin, generated_password).deliver_later + end + { saved: saved, user: admin } end - { saved: saved, user: admin } - end - def self.create_manager(params) - generated_password = SecurePassword.generate - manager = User.new(params.merge(password: generated_password)) - manager.send :set_slug + def create_manager(params) + generated_password = SecurePassword.generate + manager = User.new(params.merge(password: generated_password)) + manager.send :set_slug - saved = manager.save - if saved - manager.send_confirmation_instructions - manager.add_role(:manager) - manager.remove_role(:member) - UsersMailer.notify_user_account_created(manager, generated_password).deliver_later + saved = manager.save + if saved + manager.send_confirmation_instructions + manager.add_role(:manager) + manager.remove_role(:member) + UsersMailer.notify_user_account_created(manager, generated_password).deliver_later + end + { saved: saved, user: manager } end - { saved: saved, user: manager } end end diff --git a/app/services/user_setter_service.rb b/app/services/user_setter_service.rb new file mode 100644 index 000000000..be7ea1c50 --- /dev/null +++ b/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