From 32d0bf3afdd25eaff1f3bd2e80696e39b5b69c35 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 16 Oct 2014 16:50:09 +0300 Subject: [PATCH 1/5] Fix snippets seeds Signed-off-by: Dmitriy Zaporozhets --- db/fixtures/development/12_snippets.rb | 34 ++++++++++++++++++++------ 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/db/fixtures/development/12_snippets.rb b/db/fixtures/development/12_snippets.rb index ff91e8430a..b3a6f39c7d 100644 --- a/db/fixtures/development/12_snippets.rb +++ b/db/fixtures/development/12_snippets.rb @@ -1,9 +1,26 @@ Gitlab::Seeder.quiet do - contents = [ - `curl https://gist.githubusercontent.com/randx/4275756/raw/da2f262920c96d1a970d48bf2e99147954b1f4bd/glus1204.sh`, - `curl https://gist.githubusercontent.com/randx/3754594/raw/11026a295e6ef3a151c635707a3e1e8e15fc4725/gitlab_setup.sh`, - `curl https://gist.githubusercontent.com/randx/3065552/raw/29fbd09f4605a5ea22a5a9095e35fd1938dea4d6/gistfile1.sh`, - ] + content =< { where(access_level: GUEST) } + scope :reporters, -> { where(access_level: REPORTER) } + scope :developers, -> { where(access_level: DEVELOPER) } + scope :masters, -> { where(access_level: MASTER) } + scope :owners, -> { where(access_level: OWNER) } + + delegate :name, :username, :email, to: :user, prefix: true +end +eos (1..50).each do |i| user = User.all.sample @@ -12,10 +29,11 @@ Gitlab::Seeder.quiet do id: i, author_id: user.id, title: Faker::Lorem.sentence(3), - file_name: Faker::Internet.domain_word + '.sh', - private: [true, false].sample, - content: contents.sample, + file_name: Faker::Internet.domain_word + '.rb', + visibility_level: Gitlab::VisibilityLevel.values.sample, + content: content, }]) + print('.') end end From fad588f2bee102bf4ab090874d041e227d4e2ee4 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Thu, 16 Oct 2014 17:18:40 +0200 Subject: [PATCH 2/5] Remove LDAP save test This is handled within the LDAP class --- spec/lib/gitlab/oauth/user_spec.rb | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/spec/lib/gitlab/oauth/user_spec.rb b/spec/lib/gitlab/oauth/user_spec.rb index e4e96fd9f4..7c7d6babbf 100644 --- a/spec/lib/gitlab/oauth/user_spec.rb +++ b/spec/lib/gitlab/oauth/user_spec.rb @@ -29,16 +29,16 @@ describe Gitlab::OAuth::User do end describe :save do - context "LDAP" do - let(:provider) { 'ldap' } - it "creates a user from LDAP" do - oauth_user.save + let(:provider) { 'twitter' } - expect(gl_user).to be_valid - expect(gl_user.extern_uid).to eql uid - expect(gl_user.provider).to eql 'ldap' - end + it "creates a user from Omniauth" do + oauth_user.save + + expect(gl_user).to be_valid + expect(gl_user.extern_uid).to eql uid + expect(gl_user.provider).to eql 'twitter' end + end context "twitter" do let(:provider) { 'twitter' } From d9bfebc0e87ef426aea7eb4fdd1338f04b106354 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Thu, 16 Oct 2014 20:08:30 +0200 Subject: [PATCH 3/5] Add regressiontest to verify allow_single_sign_on setting verification for #1677 Since testing omniauth_callback_controller.rb is very difficult, the logic is moved to the models --- .../omniauth_callbacks_controller.rb | 13 +++++-------- lib/gitlab/oauth/user.rb | 17 ++++++++++++++--- spec/lib/gitlab/oauth/user_spec.rb | 19 ++++++++----------- 3 files changed, 27 insertions(+), 22 deletions(-) diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index 589f8387b0..58d1e37f65 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -49,22 +49,19 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController redirect_to profile_path else @user = Gitlab::OAuth::User.new(oauth) - - if Gitlab.config.omniauth['allow_single_sign_on'] && @user.new? - @user.save - end + @user.save # Only allow properly saved users to login. if @user.persisted? && @user.valid? sign_in_and_redirect(@user.gl_user) - elsif @user.gl_user.errors.any? + else @user.gl_user.errors.any? error_message = @user.gl_user.errors.map{ |attribute, message| "#{attribute} #{message}" }.join(", ") redirect_to omniauth_error_path(oauth['provider'], error: error_message) and return - else - flash[:notice] = "There's no such user!" - redirect_to new_user_session_path end end + rescue StandardError + flash[:notice] = "There's no such user!" + redirect_to new_user_session_path end def oauth diff --git a/lib/gitlab/oauth/user.rb b/lib/gitlab/oauth/user.rb index 133445d3d0..18ec63a62a 100644 --- a/lib/gitlab/oauth/user.rb +++ b/lib/gitlab/oauth/user.rb @@ -13,7 +13,7 @@ module Gitlab end def persisted? - gl_user.persisted? + gl_user.try(:persisted?) end def new? @@ -21,10 +21,12 @@ module Gitlab end def valid? - gl_user.valid? + gl_user.try(:valid?) end def save + unauthorized_to_create unless gl_user + gl_user.save! log.info "(OAuth) saving user #{auth_hash.email} from login with extern_uid => #{auth_hash.uid}" gl_user.block if needs_blocking? @@ -36,7 +38,12 @@ module Gitlab end def gl_user - @user ||= find_by_uid_and_provider || build_new_user + @user ||= find_by_uid_and_provider + + if Gitlab.config.omniauth.allow_single_sign_on + @user ||= build_new_user + end + @user end protected @@ -77,6 +84,10 @@ module Gitlab def model ::User end + + def raise_unauthorized_to_create + raise StandardError.new("Unauthorized to create user, signup disabled for #{auth_hash.provider}") + end end end end diff --git a/spec/lib/gitlab/oauth/user_spec.rb b/spec/lib/gitlab/oauth/user_spec.rb index 7c7d6babbf..e004d6edfa 100644 --- a/spec/lib/gitlab/oauth/user_spec.rb +++ b/spec/lib/gitlab/oauth/user_spec.rb @@ -31,17 +31,8 @@ describe Gitlab::OAuth::User do describe :save do let(:provider) { 'twitter' } - it "creates a user from Omniauth" do - oauth_user.save - - expect(gl_user).to be_valid - expect(gl_user.extern_uid).to eql uid - expect(gl_user.provider).to eql 'twitter' - end - end - - context "twitter" do - let(:provider) { 'twitter' } + context "with allow_single_sign_on enabled" do + before { Gitlab.config.omniauth.stub allow_single_sign_on: true } it "creates a user from Omniauth" do oauth_user.save @@ -51,5 +42,11 @@ describe Gitlab::OAuth::User do expect(gl_user.provider).to eql 'twitter' end end + + context "with allow_single_sign_on disabled (Default)" do + it "throws an error" do + expect{ oauth_user.save }.to raise_error StandardError + end + end end end From 966f68b33e1f15f08e383ec68346ed1bd690b59b Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Fri, 17 Oct 2014 13:15:59 +0300 Subject: [PATCH 4/5] Refactor error message a bit Signed-off-by: Dmitriy Zaporozhets --- app/controllers/omniauth_callbacks_controller.rb | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index 58d1e37f65..bd4b310fcb 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -54,8 +54,16 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController # Only allow properly saved users to login. if @user.persisted? && @user.valid? sign_in_and_redirect(@user.gl_user) - else @user.gl_user.errors.any? - error_message = @user.gl_user.errors.map{ |attribute, message| "#{attribute} #{message}" }.join(", ") + else + error_message = + if @user.gl_user.errors.any? + @user.gl_user.errors.map do |attribute, message| + "#{attribute} #{message}" + end.join(", ") + else + '' + end + redirect_to omniauth_error_path(oauth['provider'], error: error_message) and return end end From f8cdd62e2269b6c8243b6d1bc9bf73dc7dd1b535 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Fri, 17 Oct 2014 14:08:02 +0300 Subject: [PATCH 5/5] Fix account existing blocking Signed-off-by: Dmitriy Zaporozhets --- lib/gitlab/oauth/user.rb | 32 +++++++++---- spec/lib/gitlab/oauth/user_spec.rb | 76 ++++++++++++++++++++++++++---- 2 files changed, 89 insertions(+), 19 deletions(-) diff --git a/lib/gitlab/oauth/user.rb b/lib/gitlab/oauth/user.rb index 18ec63a62a..47f62153a5 100644 --- a/lib/gitlab/oauth/user.rb +++ b/lib/gitlab/oauth/user.rb @@ -17,7 +17,7 @@ module Gitlab end def new? - !gl_user.persisted? + !persisted? end def valid? @@ -27,10 +27,14 @@ module Gitlab def save unauthorized_to_create unless gl_user - gl_user.save! - log.info "(OAuth) saving user #{auth_hash.email} from login with extern_uid => #{auth_hash.uid}" - gl_user.block if needs_blocking? + if needs_blocking? + gl_user.save! + gl_user.block + else + gl_user.save! + end + log.info "(OAuth) saving user #{auth_hash.email} from login with extern_uid => #{auth_hash.uid}" gl_user rescue ActiveRecord::RecordInvalid => e log.info "(OAuth) Error saving user: #{gl_user.errors.full_messages}" @@ -40,13 +44,27 @@ module Gitlab def gl_user @user ||= find_by_uid_and_provider - if Gitlab.config.omniauth.allow_single_sign_on + if signup_enabled? @user ||= build_new_user end + @user end protected + + def needs_blocking? + new? && block_after_signup? + end + + def signup_enabled? + Gitlab.config.omniauth.allow_single_sign_on + end + + def block_after_signup? + Gitlab.config.omniauth.block_auto_created_users + end + def auth_hash=(auth_hash) @auth_hash = AuthHash.new(auth_hash) end @@ -77,10 +95,6 @@ module Gitlab Gitlab::AppLogger end - def needs_blocking? - Gitlab.config.omniauth['block_auto_created_users'] - end - def model ::User end diff --git a/spec/lib/gitlab/oauth/user_spec.rb b/spec/lib/gitlab/oauth/user_spec.rb index e004d6edfa..8a83a1b258 100644 --- a/spec/lib/gitlab/oauth/user_spec.rb +++ b/spec/lib/gitlab/oauth/user_spec.rb @@ -31,21 +31,77 @@ describe Gitlab::OAuth::User do describe :save do let(:provider) { 'twitter' } - context "with allow_single_sign_on enabled" do - before { Gitlab.config.omniauth.stub allow_single_sign_on: true } + describe 'signup' do + context "with allow_single_sign_on enabled" do + before { Gitlab.config.omniauth.stub allow_single_sign_on: true } - it "creates a user from Omniauth" do - oauth_user.save + it "creates a user from Omniauth" do + oauth_user.save - expect(gl_user).to be_valid - expect(gl_user.extern_uid).to eql uid - expect(gl_user.provider).to eql 'twitter' + expect(gl_user).to be_valid + expect(gl_user.extern_uid).to eql uid + expect(gl_user.provider).to eql 'twitter' + end + end + + context "with allow_single_sign_on disabled (Default)" do + it "throws an error" do + expect{ oauth_user.save }.to raise_error StandardError + end end end - context "with allow_single_sign_on disabled (Default)" do - it "throws an error" do - expect{ oauth_user.save }.to raise_error StandardError + describe 'blocking' do + let(:provider) { 'twitter' } + before { Gitlab.config.omniauth.stub allow_single_sign_on: true } + + context 'signup' do + context 'dont block on create' do + before { Gitlab.config.omniauth.stub block_auto_created_users: false } + + it do + oauth_user.save + gl_user.should be_valid + gl_user.should_not be_blocked + end + end + + context 'block on create' do + before { Gitlab.config.omniauth.stub block_auto_created_users: true } + + it do + oauth_user.save + gl_user.should be_valid + gl_user.should be_blocked + end + end + end + + context 'sign-in' do + before do + oauth_user.save + oauth_user.gl_user.activate + end + + context 'dont block on create' do + before { Gitlab.config.omniauth.stub block_auto_created_users: false } + + it do + oauth_user.save + gl_user.should be_valid + gl_user.should_not be_blocked + end + end + + context 'block on create' do + before { Gitlab.config.omniauth.stub block_auto_created_users: true } + + it do + oauth_user.save + gl_user.should be_valid + gl_user.should_not be_blocked + end + end end end end