diff --git a/CHANGELOG.md b/CHANGELOG.md index cfdfa6e9d..68a105039 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog Fab-manager +- Support for JSONPath syntax in OAuth2 SSO fields mapping +- Support for OAuth2 scopes +- Add debug logs for the SSO authentication process +- Fix a bug: SSO configuration interface has a misnamed field (Common URL) +- Fix a bug: unable to bind Profile.birthday and Profile.gender from an SSO + - Ability to cancel a payement schedule from the interface - Ability to create slots in the past - Ability to disable public account creation diff --git a/Gemfile b/Gemfile index f80408e12..04eb74ec0 100644 --- a/Gemfile +++ b/Gemfile @@ -14,6 +14,7 @@ gem 'webpacker', '~> 5.x' gem 'jbuilder', '~> 2.5' gem 'jbuilder_cache_multi' gem 'json', '>= 2.3.0' +gem 'jsonpath' gem 'forgery' gem 'responders', '~> 2.0' @@ -65,7 +66,10 @@ gem 'pg_search' # authentication gem 'devise', '>= 4.6.0' -gem 'omniauth', '~> 1.9.0' + +git 'https://github.com/sleede/omniauth.git', branch: 'v1.9' do + gem 'omniauth', '~> 1.9.0' +end gem 'omniauth-oauth2' gem 'omniauth-rails_csrf_protection', '~> 0.1' diff --git a/Gemfile.lock b/Gemfile.lock index 312eac6ad..5d6476de3 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,3 +1,12 @@ +GIT + remote: https://github.com/sleede/omniauth.git + revision: 832d5ac48fcc3b93eba8bf1444145d0c876314cf + branch: v1.9 + specs: + omniauth (1.9.1) + hashie (>= 3.4.6) + rack (>= 1.6.2, < 3) + GEM remote: https://rubygems.org/ specs: @@ -175,6 +184,8 @@ GEM jbuilder_cache_multi (0.1.0) jbuilder (>= 1.5.0, < 3) json (2.3.1) + jsonpath (1.1.0) + multi_json jwt (2.2.1) kaminari (1.2.1) activesupport (>= 4.1.0) @@ -234,9 +245,6 @@ GEM multi_xml (~> 0.5) rack (>= 1.2, < 3) oj (3.10.5) - omniauth (1.9.1) - hashie (>= 3.4.6) - rack (>= 1.6.2, < 3) omniauth-oauth2 (1.6.0) oauth2 (~> 1.1) omniauth (~> 1.9) @@ -456,6 +464,7 @@ DEPENDENCIES jbuilder (~> 2.5) jbuilder_cache_multi json (>= 2.3.0) + jsonpath kaminari listen (~> 3.0.5) message_format @@ -463,7 +472,7 @@ DEPENDENCIES minitest-reporters notify_with oj - omniauth (~> 1.9.0) + omniauth (~> 1.9.0)! omniauth-oauth2 omniauth-rails_csrf_protection (~> 0.1) openlab_ruby diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index f0a9ba7a5..6de4ca40c 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -2,15 +2,19 @@ class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController active_provider = AuthProvider.active define_method active_provider.strategy_name do + logger.debug "[Users::OmniauthCallbacksController##{active_provider.strategy_name}] initiated" if request.env['omniauth.params'].blank? + logger.debug 'the user has not provided any authentication token' @user = User.from_omniauth(request.env['omniauth.auth']) # Here we create the new user or update the existing one with values retrieved from the SSO. if @user.id.nil? # => new user (ie. not updating existing) + logger.debug 'trying to create a new user' # If the username is mapped, we just check its uniqueness as it would break the postgresql # unique constraint otherwise. If the name is not unique, another unique is generated if active_provider.sso_fields.include?('user.username') + logger.debug 'the username was already in use, generating a new one' @user.username = generate_unique_username(@user.username) end # If the email is mapped, we check its uniqueness. If the email is already in use, we mark it as duplicate with an @@ -18,17 +22,21 @@ class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController # - if it is the same user, his email will be filled from the SSO when he merge his accounts # - if it is not the same user, this will prevent the raise of PG::UniqueViolation if active_provider.sso_fields.include?('user.email') && email_exists?(@user.email) + logger.debug 'the email was already in use, marking it as duplicate' old_mail = @user.email @user.email = "<#{old_mail}>#{Devise.friendly_token}-duplicate" flash[:alert] = t('omniauth.email_already_linked_to_another_account_please_input_your_authentication_code', OLD_MAIL: old_mail) end else # => update of an existing user + logger.debug "an existing user was found (id=#{@user.id})" if username_exists?(@user.username, @user.id) + logger.debug 'the username was already in use, alerting user' flash[:alert] = t('omniauth.your_username_is_already_linked_to_another_account_unable_to_update_it', USERNAME: @user.username) @user.username = User.find(@user.id).username end if email_exists?(@user.email, @user.id) + logger.debug 'the email was already in use, alerting user' flash[:alert] = t('omniauth.your_email_address_is_already_linked_to_another_account_unable_to_update_it', EMAIL: @user.email) @user.email = User.find(@user.id).email end @@ -36,19 +44,32 @@ class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController # We BYPASS THE VALIDATION because, in case of a new user, we want to save him anyway, we'll ask him later to complete his profile (on first login). # In case of an existing user, we trust the SSO validation as we want the SSO to have authority on users management and policy. - @user.save(validate: false) + logger.debug 'saving the user' + unless @user.save(validate: false) + logger.error "unable to save the user, an error occurred : #{@user.errors.full_messages.join(', ')}" + end + + logger.debug 'signing-in the user and redirecting' sign_in_and_redirect @user, event: :authentication # this will throw if @user is not activated else + logger.debug 'the user has provided an authentication token' @user = User.find_by(auth_token: request.env['omniauth.params']['auth_token']) # Here the user already exists in the database and request to be linked with the SSO # so let's update its sso attributes and log him on + logger.debug "found user id=#{@user.id}" begin + logger.debug 'linking with the omniauth provider' @user.link_with_omniauth_provider(request.env['omniauth.auth']) + logger.debug 'signing-in the user and redirecting' sign_in_and_redirect @user, event: :authentication rescue DuplicateIndexError + logger.error 'user already linked' redirect_to root_url, alert: t('omniauth.this_account_is_already_linked_to_an_user_of_the_platform', NAME: active_provider.name) + rescue StandardError => e + logger.unknown "an expected error occurred: #{e}" + raise e end end @@ -58,17 +79,17 @@ class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController def username_exists?(username, exclude_id = nil) if exclude_id.nil? - User.where(username: username).size.positive? + User.where('lower(username) = ?', username&.downcase).size.positive? else - User.where(username: username).where.not(id: exclude_id).size.positive? + User.where('lower(username) = ?', username&.downcase).where.not(id: exclude_id).size.positive? end end def email_exists?(email, exclude_id = nil) if exclude_id.nil? - User.where(email: email).size.positive? + User.where('lower(email) = ?', email&.downcase).size.positive? else - User.where(email: email).where.not(id: exclude_id).size.positive? + User.where('lower(email) = ?', email&.downcase).where.not(id: exclude_id).size.positive? end end diff --git a/app/frontend/templates/admin/authentications/_oauth2_mapping.html b/app/frontend/templates/admin/authentications/_oauth2_mapping.html index a810ca5c7..d6d37811c 100644 --- a/app/frontend/templates/admin/authentications/_oauth2_mapping.html +++ b/app/frontend/templates/admin/authentications/_oauth2_mapping.html @@ -64,10 +64,11 @@ diff --git a/app/models/o_auth2_provider.rb b/app/models/o_auth2_provider.rb index 99b6edd86..75f847d7d 100644 --- a/app/models/o_auth2_provider.rb +++ b/app/models/o_auth2_provider.rb @@ -8,13 +8,13 @@ class OAuth2Provider < ApplicationRecord accepts_nested_attributes_for :o_auth2_mappings, allow_destroy: true def domain - URI(base_url).scheme+'://'+URI(base_url).host + URI(base_url).scheme + '://' + URI(base_url).host end def protected_fields fields = [] o_auth2_mappings.each do |mapping| - fields.push(mapping.local_model+'.'+mapping.local_field) + fields.push(mapping.local_model + '.' + mapping.local_field) end fields end diff --git a/app/models/profile.rb b/app/models/profile.rb index 1c5d3c0ed..ec5853dfd 100644 --- a/app/models/profile.rb +++ b/app/models/profile.rb @@ -27,7 +27,8 @@ class Profile < ApplicationRecord # we protect some fields as they are designed to be managed by the system and must not be updated externally blacklist = %w[id user_id created_at updated_at] # model-relationships must be added manually - additional = [%w[avatar string], %w[address string], %w[organization_name string], %w[organization_address string]] + additional = [%w[avatar string], %w[address string], %w[organization_name string], %w[organization_address string], + %w[gender boolean], %w[birthday date]] Profile.columns_hash .map { |k, v| [k, v.type.to_s] } .delete_if { |col| blacklist.include?(col[0]) } diff --git a/app/models/user.rb b/app/models/user.rb index f4ad820ec..6ef1985c3 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -201,16 +201,20 @@ class User < ApplicationRecord end def self.from_omniauth(auth) + logger.debug "[User::from_omniauth] initiated with parameter #{auth}" active_provider = AuthProvider.active raise SecurityError, 'The identity provider does not match the activated one' if active_provider.strategy_name != auth.provider where(provider: auth.provider, uid: auth.uid).first_or_create.tap do |user| # execute this regardless of whether record exists or not (-> User#tap) # this will init or update the user thanks to the information retrieved from the SSO + logger.debug user.id.nil? ? 'no user found, creating a new one' : "found user id=#{user.id}" user.profile ||= Profile.new auth.info.mapping.each do |key, value| + logger.debug "mapping info #{key} with value=#{value}" user.set_data_from_sso_mapping(key, value) end + logger.debug "generating a new password" user.password = Devise.friendly_token[0, 20] end end @@ -238,6 +242,10 @@ class User < ApplicationRecord 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 @@ -256,15 +264,24 @@ class User < ApplicationRecord profile.user_avatar ||= UserAvatar.new profile.user_avatar.remote_attachment_url = data when 'profile.address' + invoicing_profile ||= InvoicingProfile.new invoicing_profile.address ||= Address.new invoicing_profile.address.address = data when 'profile.organization_name' + invoicing_profile ||= InvoicingProfile.new invoicing_profile.organization ||= Organization.new invoicing_profile.organization.name = data when 'profile.organization_address' + invoicing_profile ||= InvoicingProfile.new invoicing_profile.organization ||= Organization.new invoicing_profile.organization.address ||= Address.new invoicing_profile.organization.address.address = data + when 'profile.gender' + statistic_profile ||= StatisticProfile.new + statistic_profile.gender = data + when 'profile.birthday' + statistic_profile ||= StatisticProfile.new + statistic_profile.birthday = data else profile[sso_mapping[8..-1].to_sym] = data unless data.nil? end @@ -292,6 +309,7 @@ class User < ApplicationRecord ## Merge the provided User's SSO details into the current user and drop the provided user to ensure the unity ## @param sso_user {User} the provided user will be DELETED after the merge was successful def merge_from_sso(sso_user) + logger.debug "[User::merge_from_sso] initiated with parameter #{sso_user}" # update the attributes to link the account to the sso account self.provider = sso_user.provider self.uid = sso_user.uid @@ -303,24 +321,34 @@ class User < ApplicationRecord # check that the email duplication was resolved if sso_user.email.end_with? '-duplicate' email_addr = sso_user.email.match(/^<([^>]+)>.{20}-duplicate$/)[1] + logger.error 'duplicate email was not resolved' raise(DuplicateIndexError, email_addr) unless email_addr == email end # 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| 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') end # run the account transfer in an SQL transaction to ensure data integrity - User.transaction do - # remove the temporary account - sso_user.destroy - # finally, save the new details - save! + begin + User.transaction do + # remove the temporary account + logger.debug 'removing the temporary user' + sso_user.destroy + # finally, save the new details + logger.debug 'saving the updated user' + save! + end + rescue ActiveRecord::RecordInvalid => e + logger.error "error while merging user #{sso_user.id} into #{id}: #{e.message}" + raise e end end diff --git a/app/views/api/auth_providers/mapping_fields.json.jbuilder b/app/views/api/auth_providers/mapping_fields.json.jbuilder index 7eb9615bf..6959341ef 100644 --- a/app/views/api/auth_providers/mapping_fields.json.jbuilder +++ b/app/views/api/auth_providers/mapping_fields.json.jbuilder @@ -1,4 +1,4 @@ +# frozen_string_literal: true json.user User.mapping - -json.profile Profile.mapping \ No newline at end of file +json.profile Profile.mapping diff --git a/config/locales/app.shared.en.yml b/config/locales/app.shared.en.yml index d0ff724ed..33d8d7cd4 100644 --- a/config/locales/app.shared.en.yml +++ b/config/locales/app.shared.en.yml @@ -252,7 +252,7 @@ en: mappings: "Mappings" #edition/creation form of an OAuth2 authentication provider oauth2: - common_url: "Common URL" + common_url: "Server root URL" common_url_is_required: "Common URL is required." provided_url_is_not_a_valid_url: "Provided URL is not a valid URL." authorization_endpoint: "Authorization endpoint" @@ -274,6 +274,7 @@ en: api_endpoint_url: "API endpoint URL" api_type: "API type" api_fields: "API fields" + api_field_help: "JsonPath syntax is supported.\n If many fields are selected, the first one will be used.\n Example: $.data[*].name" #machine/training slot modification modal confirm_modify_slot_modal: change_the_slot: "Change the slot" diff --git a/doc/sso_with_github.md b/doc/sso_with_github.md index 26b92fee2..8dd487f9a 100644 --- a/doc/sso_with_github.md +++ b/doc/sso_with_github.md @@ -26,9 +26,9 @@ For this guide, we will use [GitHub](https://developer.github.com/v3/oauth/) as 3. The slug of this name is used in the callback URL provided to the SSO server (eg. /users/auth/oauth2-**github**/callback) - Fulfill the form with the following parameters: - - **Common URL**: `https://github.com/login/oauth/` This is the common part in the URLs of the two following parameters. - - **Authorization endpoint**: `authorize` This URL can be found [here](https://developer.github.com/v3/oauth/). - - **Token Acquisition Endpoint**: `access_token` This URL can be found [here](https://developer.github.com/v3/oauth/). + - **Server root URL**: `https://github.com` This is the domain name of the where the SSO server is located. + - **Authorization endpoint**: `/login/oauth/authorize` This URL can be found [here](https://developer.github.com/v3/oauth/). + - **Token Acquisition Endpoint**: `/login/oauth/access_token` This URL can be found [here](https://developer.github.com/v3/oauth/). - **Profile edition URL**: `https://github.com/settings/profile` This is the URL where you are directed when you click on `Edit profile` in your GitHub dashboard. - **Client identifier**: Your Client ID, collected just before. - **Client secret**: Your Client Secret, collected just before. diff --git a/lib/omni_auth/strategies/sso_oauth2_provider.rb b/lib/omni_auth/strategies/sso_oauth2_provider.rb index 80a45d3d3..ed1798bcc 100644 --- a/lib/omni_auth/strategies/sso_oauth2_provider.rb +++ b/lib/omni_auth/strategies/sso_oauth2_provider.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require 'omniauth-oauth2' +require 'jsonpath' module OmniAuth::Strategies # Authentication strategy provided trough oAuth 2.0 @@ -24,6 +25,12 @@ module OmniAuth::Strategies authorize_url: active_provider.providable.authorization_endpoint, token_url: active_provider.providable.token_endpoint + def authorize_params + super.tap do |params| + params[:scope] = ENV['OAUTH2_SCOPE'] + end + end + def callback_url url = Rails.application.config.action_controller.default_url_options "#{url[:protocol]}://#{url[:host]}#{script_name}#{callback_path}" @@ -46,9 +53,15 @@ module OmniAuth::Strategies # retrieve data from various url, querying each only once def raw_info @raw_info ||= {} + puts "[raw_info] @raw_infos = #{@raw_info&.to_json}" unless @raw_info.size.positive? OmniAuth::Strategies::SsoOauth2Provider.active_provider.providable.o_auth2_mappings.each do |mapping| + puts "mapping = #{mapping&.to_json}" unless @raw_info.key?(mapping.api_endpoint.to_sym) + puts "api_endpoint = #{mapping.api_endpoint.to_sym}" + puts "access_token = #{access_token&.to_json}" + puts "token get = #{access_token.get(mapping.api_endpoint)}" + puts "parsed = #{access_token.get(mapping.api_endpoint).parsed}" @raw_info[mapping.api_endpoint.to_sym] = access_token.get(mapping.api_endpoint).parsed end end @@ -58,6 +71,7 @@ module OmniAuth::Strategies def parsed_info @parsed_info ||= {} + puts "[parsed_info] @parsed_info = #{@parsed_info.to_json}" unless @parsed_info.size.positive? OmniAuth::Strategies::SsoOauth2Provider.active_provider.providable.o_auth2_mappings.each do |mapping| @@ -88,7 +102,8 @@ module OmniAuth::Strategies ## NO TRANSFORMATION else - @parsed_info[local_sym(mapping)] = raw_info[mapping.api_endpoint.to_sym][mapping.api_field] + puts "@parsed_info[#{local_sym(mapping)}] found in #{raw_info[mapping.api_endpoint.to_sym]}" + @parsed_info[local_sym(mapping)] = ::JsonPath.new(mapping.api_field).on(raw_info[mapping.api_endpoint.to_sym]).first end end end