From da2191afa0e1bf4e0d1f605df9528800eec91c61 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Tue, 20 Mar 2018 09:42:35 +0000 Subject: OmniauthInitializer created to improve devise.rb This should simplify refactoring and allow testing --- config/initializers/devise.rb | 46 +------------------- lib/gitlab/omniauth_initializer.rb | 65 ++++++++++++++++++++++++++++ spec/lib/gitlab/omniauth_initializer_spec.rb | 65 ++++++++++++++++++++++++++++ 3 files changed, 131 insertions(+), 45 deletions(-) create mode 100644 lib/gitlab/omniauth_initializer.rb create mode 100644 spec/lib/gitlab/omniauth_initializer_spec.rb diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index f642e6d47e0..362b9cc9a88 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -219,49 +219,5 @@ Devise.setup do |config| end end - Gitlab.config.omniauth.providers.each do |provider| - provider_arguments = [] - - %w[app_id app_secret].each do |argument| - provider_arguments << provider[argument] if provider[argument] - end - - case provider['args'] - when Array - # An Array from the configuration will be expanded. - provider_arguments.concat provider['args'] - when Hash - # Add procs for handling SLO - if provider['name'] == 'cas3' - provider['args'][:on_single_sign_out] = lambda do |request| - ticket = request.params[:session_index] - raise "Service Ticket not found." unless Gitlab::Auth::OAuth::Session.valid?(:cas3, ticket) - - Gitlab::Auth::OAuth::Session.destroy(:cas3, ticket) - true - end - end - - if provider['name'] == 'authentiq' - provider['args'][:remote_sign_out_handler] = lambda do |request| - authentiq_session = request.params['sid'] - if Gitlab::Auth::OAuth::Session.valid?(:authentiq, authentiq_session) - Gitlab::Auth::OAuth::Session.destroy(:authentiq, authentiq_session) - true - else - false - end - end - end - - if provider['name'] == 'shibboleth' - provider['args'][:fail_with_empty_uid] = true - end - - # A Hash from the configuration will be passed as is. - provider_arguments << provider['args'].symbolize_keys - end - - config.omniauth provider['name'].to_sym, *provider_arguments - end + Gitlab::OmniauthInitializer.new(config).execute(Gitlab.config.omniauth.providers) end diff --git a/lib/gitlab/omniauth_initializer.rb b/lib/gitlab/omniauth_initializer.rb new file mode 100644 index 00000000000..a2c37444730 --- /dev/null +++ b/lib/gitlab/omniauth_initializer.rb @@ -0,0 +1,65 @@ +module Gitlab + class OmniauthInitializer + def initialize(devise_config) + @devise_config = devise_config + end + + def config + @devise_config + end + + def execute(providers) + initialize_providers(providers) + end + + private + + def initialize_providers(providers) + providers.each do |provider| + provider_arguments = [] + + %w[app_id app_secret].each do |argument| + provider_arguments << provider[argument] if provider[argument] + end + + case provider['args'] + when Array + # An Array from the configuration will be expanded. + provider_arguments.concat provider['args'] + when Hash + # Add procs for handling SLO + if provider['name'] == 'cas3' + provider['args'][:on_single_sign_out] = lambda do |request| + ticket = request.params[:session_index] + raise "Service Ticket not found." unless Gitlab::Auth::OAuth::Session.valid?(:cas3, ticket) + + Gitlab::Auth::OAuth::Session.destroy(:cas3, ticket) + true + end + end + + if provider['name'] == 'authentiq' + provider['args'][:remote_sign_out_handler] = lambda do |request| + authentiq_session = request.params['sid'] + if Gitlab::Auth::OAuth::Session.valid?(:authentiq, authentiq_session) + Gitlab::Auth::OAuth::Session.destroy(:authentiq, authentiq_session) + true + else + false + end + end + end + + if provider['name'] == 'shibboleth' + provider['args'][:fail_with_empty_uid] = true + end + + # A Hash from the configuration will be passed as is. + provider_arguments << provider['args'].symbolize_keys + end + + config.omniauth provider['name'].to_sym, *provider_arguments + end + end + end +end diff --git a/spec/lib/gitlab/omniauth_initializer_spec.rb b/spec/lib/gitlab/omniauth_initializer_spec.rb new file mode 100644 index 00000000000..d808b4d49e0 --- /dev/null +++ b/spec/lib/gitlab/omniauth_initializer_spec.rb @@ -0,0 +1,65 @@ +require 'spec_helper' + +describe Gitlab::OmniauthInitializer do + let(:devise_config) { class_double(Devise) } + + subject { described_class.new(devise_config) } + + describe '#execute' do + it 'configures providers from array' do + generic_config = { 'name' => 'generic' } + + expect(devise_config).to receive(:omniauth).with(:generic) + + subject.execute([generic_config]) + end + + it 'allows "args" array for app_id and app_secret' do + legacy_config = { 'name' => 'legacy', 'args' => %w(123 abc) } + + expect(devise_config).to receive(:omniauth).with(:legacy, '123', 'abc') + + subject.execute([legacy_config]) + end + + it 'passes app_id and app_secret as additional arguments' do + twitter_config = { 'name' => 'twitter', 'app_id' => '123', 'app_secret' => 'abc' } + + expect(devise_config).to receive(:omniauth).with(:twitter, '123', 'abc') + + subject.execute([twitter_config]) + end + + it 'passes "args" hash as symbolized hash argument' do + hash_config = { 'name' => 'hash', 'args' => { 'custom' => 'format' } } + + expect(devise_config).to receive(:omniauth).with(:hash, custom: 'format') + + subject.execute([hash_config]) + end + + it 'configures fail_with_empty_uid for shibboleth' do + shibboleth_config = { 'name' => 'shibboleth', 'args' => {} } + + expect(devise_config).to receive(:omniauth).with(:shibboleth, fail_with_empty_uid: true) + + subject.execute([shibboleth_config]) + end + + it 'configures remote_sign_out_handler proc for authentiq' do + authentiq_config = { 'name' => 'authentiq', 'args' => {} } + + expect(devise_config).to receive(:omniauth).with(:authentiq, remote_sign_out_handler: an_instance_of(Proc)) + + subject.execute([authentiq_config]) + end + + it 'configures on_single_sign_out proc for cas3' do + cas3_config = { 'name' => 'cas3', 'args' => {} } + + expect(devise_config).to receive(:omniauth).with(:cas3, on_single_sign_out: an_instance_of(Proc)) + + subject.execute([cas3_config]) + end + end +end -- cgit v1.2.1 From 97cf5d737d05f841232f962db98ac600299668b0 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Tue, 20 Mar 2018 17:34:26 +0000 Subject: Omniauth callbacks moved to methods --- lib/gitlab/omniauth_initializer.rb | 64 ++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 26 deletions(-) diff --git a/lib/gitlab/omniauth_initializer.rb b/lib/gitlab/omniauth_initializer.rb index a2c37444730..1b8ffc8c096 100644 --- a/lib/gitlab/omniauth_initializer.rb +++ b/lib/gitlab/omniauth_initializer.rb @@ -27,32 +27,7 @@ module Gitlab # An Array from the configuration will be expanded. provider_arguments.concat provider['args'] when Hash - # Add procs for handling SLO - if provider['name'] == 'cas3' - provider['args'][:on_single_sign_out] = lambda do |request| - ticket = request.params[:session_index] - raise "Service Ticket not found." unless Gitlab::Auth::OAuth::Session.valid?(:cas3, ticket) - - Gitlab::Auth::OAuth::Session.destroy(:cas3, ticket) - true - end - end - - if provider['name'] == 'authentiq' - provider['args'][:remote_sign_out_handler] = lambda do |request| - authentiq_session = request.params['sid'] - if Gitlab::Auth::OAuth::Session.valid?(:authentiq, authentiq_session) - Gitlab::Auth::OAuth::Session.destroy(:authentiq, authentiq_session) - true - else - false - end - end - end - - if provider['name'] == 'shibboleth' - provider['args'][:fail_with_empty_uid] = true - end + set_provider_specific_defaults(provider) # A Hash from the configuration will be passed as is. provider_arguments << provider['args'].symbolize_keys @@ -61,5 +36,42 @@ module Gitlab config.omniauth provider['name'].to_sym, *provider_arguments end end + + def set_provider_specific_defaults(provider) + # Add procs for handling SLO + if provider['name'] == 'cas3' + provider['args'][:on_single_sign_out] = cas3_signout_handler + end + + if provider['name'] == 'authentiq' + provider['args'][:remote_sign_out_handler] = authentiq_signout_handler + end + + if provider['name'] == 'shibboleth' + provider['args'][:fail_with_empty_uid] = true + end + end + + def cas3_signout_handler + lambda do |request| + ticket = request.params[:session_index] + raise "Service Ticket not found." unless Gitlab::Auth::OAuth::Session.valid?(:cas3, ticket) + + Gitlab::Auth::OAuth::Session.destroy(:cas3, ticket) + true + end + end + + def authentiq_signout_handler + lambda do |request| + authentiq_session = request.params['sid'] + if Gitlab::Auth::OAuth::Session.valid?(:authentiq, authentiq_session) + Gitlab::Auth::OAuth::Session.destroy(:authentiq, authentiq_session) + true + else + false + end + end + end end end -- cgit v1.2.1 From 60b480fe814975b508f01bd1ae4f455f3ec454eb Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Wed, 21 Mar 2018 23:25:47 +0000 Subject: OmniauthInitializer refactoring --- lib/gitlab/omniauth_initializer.rb | 64 ++++++++++++++++++-------------------- 1 file changed, 31 insertions(+), 33 deletions(-) diff --git a/lib/gitlab/omniauth_initializer.rb b/lib/gitlab/omniauth_initializer.rb index 1b8ffc8c096..35ed3a5ac05 100644 --- a/lib/gitlab/omniauth_initializer.rb +++ b/lib/gitlab/omniauth_initializer.rb @@ -4,51 +4,49 @@ module Gitlab @devise_config = devise_config end - def config - @devise_config - end - def execute(providers) - initialize_providers(providers) + providers.each do |provider| + add_provider(provider['name'].to_sym, *arguments_for(provider)) + end end private - def initialize_providers(providers) - providers.each do |provider| - provider_arguments = [] - - %w[app_id app_secret].each do |argument| - provider_arguments << provider[argument] if provider[argument] - end - - case provider['args'] - when Array - # An Array from the configuration will be expanded. - provider_arguments.concat provider['args'] - when Hash - set_provider_specific_defaults(provider) + def add_provider(*args) + @devise_config.omniauth(*args) + end - # A Hash from the configuration will be passed as is. - provider_arguments << provider['args'].symbolize_keys - end + def arguments_for(provider) + provider_arguments = [] - config.omniauth provider['name'].to_sym, *provider_arguments + %w[app_id app_secret].each do |argument| + provider_arguments << provider[argument] if provider[argument] end - end - def set_provider_specific_defaults(provider) - # Add procs for handling SLO - if provider['name'] == 'cas3' - provider['args'][:on_single_sign_out] = cas3_signout_handler - end + case provider['args'] + when Array + # An Array from the configuration will be expanded. + provider_arguments.concat provider['args'] + when Hash + hash_arguments = provider['args'].merge(provider_defaults(provider)) - if provider['name'] == 'authentiq' - provider['args'][:remote_sign_out_handler] = authentiq_signout_handler + # A Hash from the configuration will be passed as is. + provider_arguments << hash_arguments.symbolize_keys end - if provider['name'] == 'shibboleth' - provider['args'][:fail_with_empty_uid] = true + provider_arguments + end + + def provider_defaults(provider) + case provider['name'] + when 'cas3' + { on_single_sign_out: cas3_signout_handler } + when 'authentiq' + { remote_sign_out_handler: authentiq_signout_handler } + when 'shibboleth' + { fail_with_empty_uid: true } + else + {} end end -- cgit v1.2.1