From 84948f220228eea115684c5ea7e2b8310f3008c5 Mon Sep 17 00:00:00 2001 From: Bryan Miller Date: Wed, 16 Dec 2020 21:36:39 +0000 Subject: [PATCH 01/13] Update ee/lib/ee/gitlab/auth/saml/user.rb --- ee/lib/ee/gitlab/auth/saml/user.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ee/lib/ee/gitlab/auth/saml/user.rb b/ee/lib/ee/gitlab/auth/saml/user.rb index 9a325d0c1e3c8d..0d4ba6c1e286f8 100644 --- a/ee/lib/ee/gitlab/auth/saml/user.rb +++ b/ee/lib/ee/gitlab/auth/saml/user.rb @@ -16,7 +16,9 @@ def find_user elsif user&.persisted? block_user(user, "not in required group") unless user.blocked? else + # Return early if user == nil, before calling build_user_synced_attributes_metadata with a nil user object. user = nil + return end if user -- GitLab From b3e518f20308cf768957a218cdb61acdc3fd9539 Mon Sep 17 00:00:00 2001 From: Bryan Miller Date: Fri, 8 Jan 2021 18:28:16 +0000 Subject: [PATCH 02/13] Revert previous commit with early return --- ee/lib/ee/gitlab/auth/saml/user.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/ee/lib/ee/gitlab/auth/saml/user.rb b/ee/lib/ee/gitlab/auth/saml/user.rb index 0d4ba6c1e286f8..9a325d0c1e3c8d 100644 --- a/ee/lib/ee/gitlab/auth/saml/user.rb +++ b/ee/lib/ee/gitlab/auth/saml/user.rb @@ -16,9 +16,7 @@ def find_user elsif user&.persisted? block_user(user, "not in required group") unless user.blocked? else - # Return early if user == nil, before calling build_user_synced_attributes_metadata with a nil user object. user = nil - return end if user -- GitLab From b30e81c867fdc19b5157ee59c489b7d0d82d93ba Mon Sep 17 00:00:00 2001 From: Bryan Miller Date: Fri, 8 Jan 2021 18:32:17 +0000 Subject: [PATCH 03/13] Check for gl_user object in update_profile() --- lib/gitlab/auth/o_auth/user.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/auth/o_auth/user.rb b/lib/gitlab/auth/o_auth/user.rb index f556a7f40e95a9..fe1bf730e76b9a 100644 --- a/lib/gitlab/auth/o_auth/user.rb +++ b/lib/gitlab/auth/o_auth/user.rb @@ -239,8 +239,9 @@ def sync_profile_from_provider? end def update_profile - clear_user_synced_attributes_metadata + return unless gl_user + clear_user_synced_attributes_metadata return unless sync_profile_from_provider? || creating_linked_ldap_user? metadata = gl_user.build_user_synced_attributes_metadata -- GitLab From f84016ee38906a13a8e265ec2fcabefca4a78fb6 Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Thu, 4 Feb 2021 14:18:54 -0600 Subject: [PATCH 04/13] Add spec for nil user --- spec/lib/gitlab/auth/o_auth/user_spec.rb | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/spec/lib/gitlab/auth/o_auth/user_spec.rb b/spec/lib/gitlab/auth/o_auth/user_spec.rb index 6c6cee9c273b88..2f2103dbb0e6bb 100644 --- a/spec/lib/gitlab/auth/o_auth/user_spec.rb +++ b/spec/lib/gitlab/auth/o_auth/user_spec.rb @@ -995,6 +995,19 @@ def result_identities(dn, uid) end end + context 'when gl_user is nil' do + before do + allow_next_instance_of(described_class) do |instance| + allow(instance).to receive(:gl_user).and_return(nil) + end + end + + it 'does not raise NME' do + expect(gl_user).to be nil + expect { oauth_user.save }.not_to raise_error NoMethodError + end + end + describe '._uid_and_provider' do let!(:existing_user) { create(:omniauth_user, extern_uid: 'my-uid', provider: 'my-provider') } -- GitLab From f68b0cdbcb9320009d2aba170952ec0873e9da8c Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Fri, 5 Feb 2021 16:22:45 -0600 Subject: [PATCH 05/13] Omniauth callback spec --- lib/gitlab/auth/o_auth/user.rb | 2 +- .../omniauth_callbacks_controller_spec.rb | 12 +++++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/auth/o_auth/user.rb b/lib/gitlab/auth/o_auth/user.rb index fe1bf730e76b9a..7455b6bc2df73d 100644 --- a/lib/gitlab/auth/o_auth/user.rb +++ b/lib/gitlab/auth/o_auth/user.rb @@ -239,7 +239,7 @@ def sync_profile_from_provider? end def update_profile - return unless gl_user + #return unless gl_user clear_user_synced_attributes_metadata return unless sync_profile_from_provider? || creating_linked_ldap_user? diff --git a/spec/controllers/omniauth_callbacks_controller_spec.rb b/spec/controllers/omniauth_callbacks_controller_spec.rb index edd587389cb5d9..3fca8aa19c1505 100644 --- a/spec/controllers/omniauth_callbacks_controller_spec.rb +++ b/spec/controllers/omniauth_callbacks_controller_spec.rb @@ -354,7 +354,7 @@ def stub_route_as(path) end end end - + describe '#saml' do let(:last_request_id) { 'ONELOGIN_4fee3b046395c4e751011e97f8900b5273d56685' } let(:user) { create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: 'saml') } @@ -405,6 +405,16 @@ def stub_last_request_id(id) expect(flash[:alert]).to start_with 'Signing in using your saml account without a pre-existing GitLab account is not allowed.' end + + context 'when user is nil' do + let(:user) { nil } + + it 'serena test' do + stub_omniauth_setting(allow_single_sign_on: false, block_auto_created_users: false) + + expect { post :saml, params: { SAMLResponse: mock_saml_response } }.to raise_error NoMethodError + end + end end context 'with GitLab initiated request' do -- GitLab From e148a73bd351c2a81387dd5e6562060fea148d04 Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Mon, 22 Feb 2021 19:24:20 -0600 Subject: [PATCH 06/13] Revert "Omniauth callback spec" This reverts commit f68b0cdbcb9320009d2aba170952ec0873e9da8c. --- lib/gitlab/auth/o_auth/user.rb | 2 +- .../omniauth_callbacks_controller_spec.rb | 12 +----------- 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/lib/gitlab/auth/o_auth/user.rb b/lib/gitlab/auth/o_auth/user.rb index 7455b6bc2df73d..fe1bf730e76b9a 100644 --- a/lib/gitlab/auth/o_auth/user.rb +++ b/lib/gitlab/auth/o_auth/user.rb @@ -239,7 +239,7 @@ def sync_profile_from_provider? end def update_profile - #return unless gl_user + return unless gl_user clear_user_synced_attributes_metadata return unless sync_profile_from_provider? || creating_linked_ldap_user? diff --git a/spec/controllers/omniauth_callbacks_controller_spec.rb b/spec/controllers/omniauth_callbacks_controller_spec.rb index 3fca8aa19c1505..edd587389cb5d9 100644 --- a/spec/controllers/omniauth_callbacks_controller_spec.rb +++ b/spec/controllers/omniauth_callbacks_controller_spec.rb @@ -354,7 +354,7 @@ def stub_route_as(path) end end end - + describe '#saml' do let(:last_request_id) { 'ONELOGIN_4fee3b046395c4e751011e97f8900b5273d56685' } let(:user) { create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: 'saml') } @@ -405,16 +405,6 @@ def stub_last_request_id(id) expect(flash[:alert]).to start_with 'Signing in using your saml account without a pre-existing GitLab account is not allowed.' end - - context 'when user is nil' do - let(:user) { nil } - - it 'serena test' do - stub_omniauth_setting(allow_single_sign_on: false, block_auto_created_users: false) - - expect { post :saml, params: { SAMLResponse: mock_saml_response } }.to raise_error NoMethodError - end - end end context 'with GitLab initiated request' do -- GitLab From 7b96d4773ec10a97c2d6f3b5d3122a9641742597 Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Tue, 23 Feb 2021 09:34:51 -0600 Subject: [PATCH 07/13] Use any instance of --- spec/lib/gitlab/auth/o_auth/user_spec.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/lib/gitlab/auth/o_auth/user_spec.rb b/spec/lib/gitlab/auth/o_auth/user_spec.rb index 2f2103dbb0e6bb..e722b63acf318a 100644 --- a/spec/lib/gitlab/auth/o_auth/user_spec.rb +++ b/spec/lib/gitlab/auth/o_auth/user_spec.rb @@ -996,15 +996,15 @@ def result_identities(dn, uid) end context 'when gl_user is nil' do + # rubocop:disable RSpec/AnyInstanceOf before do - allow_next_instance_of(described_class) do |instance| - allow(instance).to receive(:gl_user).and_return(nil) - end + allow_any_instance_of(described_class).to receive(:gl_user) { nil } + allow_any_instance_of(described_class).to receive(:sync_profile_from_provider?) { true } # to make the code flow proceed till gl_user.build_user_synced_attributes_metadata is called end + # rubocop:enable RSpec/AnyInstanceOf it 'does not raise NME' do - expect(gl_user).to be nil - expect { oauth_user.save }.not_to raise_error NoMethodError + expect { oauth_user }.not_to raise_error end end -- GitLab From 9fc415ff73b29a626ff163302dd62afe92e0dd22 Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Tue, 23 Feb 2021 10:22:27 -0600 Subject: [PATCH 08/13] Add comments explaining any instance of --- spec/lib/gitlab/auth/o_auth/user_spec.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/spec/lib/gitlab/auth/o_auth/user_spec.rb b/spec/lib/gitlab/auth/o_auth/user_spec.rb index e722b63acf318a..f537e08ba0d093 100644 --- a/spec/lib/gitlab/auth/o_auth/user_spec.rb +++ b/spec/lib/gitlab/auth/o_auth/user_spec.rb @@ -996,10 +996,13 @@ def result_identities(dn, uid) end context 'when gl_user is nil' do + # We can't use `allow_next_instance_of` here because the stubbed method, `update_profile`, + # is called inside `initialize`. When the class calls `gl_user` during `initialize`, + # the `nil` value is overwritten and we do not see expected results from the spec. # rubocop:disable RSpec/AnyInstanceOf before do allow_any_instance_of(described_class).to receive(:gl_user) { nil } - allow_any_instance_of(described_class).to receive(:sync_profile_from_provider?) { true } # to make the code flow proceed till gl_user.build_user_synced_attributes_metadata is called + allow_any_instance_of(described_class).to receive(:sync_profile_from_provider?) { true } # to make the code flow proceed until gl_user.build_user_synced_attributes_metadata is called end # rubocop:enable RSpec/AnyInstanceOf -- GitLab From 81d71a3b2f0d39b32633ceb3001faab5aa0cc29d Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Tue, 23 Feb 2021 10:25:01 -0600 Subject: [PATCH 09/13] Add to comment for clarity --- spec/lib/gitlab/auth/o_auth/user_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/lib/gitlab/auth/o_auth/user_spec.rb b/spec/lib/gitlab/auth/o_auth/user_spec.rb index f537e08ba0d093..a58d4a94f434aa 100644 --- a/spec/lib/gitlab/auth/o_auth/user_spec.rb +++ b/spec/lib/gitlab/auth/o_auth/user_spec.rb @@ -996,9 +996,9 @@ def result_identities(dn, uid) end context 'when gl_user is nil' do - # We can't use `allow_next_instance_of` here because the stubbed method, `update_profile`, - # is called inside `initialize`. When the class calls `gl_user` during `initialize`, - # the `nil` value is overwritten and we do not see expected results from the spec. + # We can't use `allow_next_instance_of` here because the stubbed method is called inside `initialize`. + # When the class calls `gl_user` during `initialize`, the `nil` value is overwritten and we do not see expected results from the spec. + # So we use `allow_any_instance_of` to preserve the `nil` value to test the behavior when `gl_user` is nil. # rubocop:disable RSpec/AnyInstanceOf before do allow_any_instance_of(described_class).to receive(:gl_user) { nil } -- GitLab From b2c7b1fd343c814e8cf6e0efd69b0eb824197f05 Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Tue, 23 Feb 2021 10:28:56 -0600 Subject: [PATCH 10/13] Change spec name --- spec/lib/gitlab/auth/o_auth/user_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/lib/gitlab/auth/o_auth/user_spec.rb b/spec/lib/gitlab/auth/o_auth/user_spec.rb index a58d4a94f434aa..567e539c1c55dd 100644 --- a/spec/lib/gitlab/auth/o_auth/user_spec.rb +++ b/spec/lib/gitlab/auth/o_auth/user_spec.rb @@ -1006,7 +1006,7 @@ def result_identities(dn, uid) end # rubocop:enable RSpec/AnyInstanceOf - it 'does not raise NME' do + it 'does not raise NoMethodError' do expect { oauth_user }.not_to raise_error end end -- GitLab From 2a01db5de32dc324bc28dbcd2237f011325ef74d Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Wed, 24 Feb 2021 15:40:31 -0600 Subject: [PATCH 11/13] Add line break between comments --- spec/lib/gitlab/auth/o_auth/user_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/lib/gitlab/auth/o_auth/user_spec.rb b/spec/lib/gitlab/auth/o_auth/user_spec.rb index 567e539c1c55dd..7a8e6e77d52daf 100644 --- a/spec/lib/gitlab/auth/o_auth/user_spec.rb +++ b/spec/lib/gitlab/auth/o_auth/user_spec.rb @@ -999,6 +999,7 @@ def result_identities(dn, uid) # We can't use `allow_next_instance_of` here because the stubbed method is called inside `initialize`. # When the class calls `gl_user` during `initialize`, the `nil` value is overwritten and we do not see expected results from the spec. # So we use `allow_any_instance_of` to preserve the `nil` value to test the behavior when `gl_user` is nil. + # rubocop:disable RSpec/AnyInstanceOf before do allow_any_instance_of(described_class).to receive(:gl_user) { nil } -- GitLab From 9622fbd2ab152afd23629f78703f55e0ef2e96b4 Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Thu, 25 Feb 2021 15:47:17 -0600 Subject: [PATCH 12/13] Add changelog entry --- .../user-auth-bmiller-return-early-if-user-is-nil.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/user-auth-bmiller-return-early-if-user-is-nil.yml diff --git a/changelogs/unreleased/user-auth-bmiller-return-early-if-user-is-nil.yml b/changelogs/unreleased/user-auth-bmiller-return-early-if-user-is-nil.yml new file mode 100644 index 00000000000000..429add97c4e055 --- /dev/null +++ b/changelogs/unreleased/user-auth-bmiller-return-early-if-user-is-nil.yml @@ -0,0 +1,5 @@ +--- +title: Return early if gl user is nil +merge_request: 50216 +author: +type: other -- GitLab From 29af1d380d89b9691d28022daeb7ab5b5aadfa2e Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Fri, 26 Feb 2021 08:41:49 +0000 Subject: [PATCH 13/13] Apply 1 suggestion(s) to 1 file(s) --- .../user-auth-bmiller-return-early-if-user-is-nil.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelogs/unreleased/user-auth-bmiller-return-early-if-user-is-nil.yml b/changelogs/unreleased/user-auth-bmiller-return-early-if-user-is-nil.yml index 429add97c4e055..b259fdecd56347 100644 --- a/changelogs/unreleased/user-auth-bmiller-return-early-if-user-is-nil.yml +++ b/changelogs/unreleased/user-auth-bmiller-return-early-if-user-is-nil.yml @@ -1,5 +1,5 @@ --- -title: Return early if gl user is nil +title: Fix a crash when logging in using SAML for the first time when sign-ups are disabled merge_request: 50216 author: type: other -- GitLab