diff --git a/app/controllers/concerns/membership_actions.rb b/app/controllers/concerns/membership_actions.rb index 7bbee8ba79eb3b9d3d873d8b84013831aced2049..869661d95ba44c6e829f93730c9124021345f110 100644 --- a/app/controllers/concerns/membership_actions.rb +++ b/app/controllers/concerns/membership_actions.rb @@ -186,3 +186,5 @@ def requested_relations end end end + +MembershipActions.prepend_if_ee('EE::MembershipActions') diff --git a/config/feature_flags/ops/block_password_auth_for_saml_users.yml b/config/feature_flags/ops/block_password_auth_for_saml_users.yml new file mode 100644 index 0000000000000000000000000000000000000000..492c00f2dd5a5e2f9b3957dfcc7b2ff2c2062371 --- /dev/null +++ b/config/feature_flags/ops/block_password_auth_for_saml_users.yml @@ -0,0 +1,8 @@ +--- +name: block_password_auth_for_saml_users +introduced_by_url: +rollout_issue_url: +milestone: '13.11' +type: ops +group: group::access +default_enabled: false diff --git a/ee/app/controllers/concerns/ee/membership_actions.rb b/ee/app/controllers/concerns/ee/membership_actions.rb new file mode 100644 index 0000000000000000000000000000000000000000..165e426051015452dfbcffeba5922f1fa0562bbc --- /dev/null +++ b/ee/app/controllers/concerns/ee/membership_actions.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +module EE + module MembershipActions + extend ::Gitlab::Utils::Override + + override :leave + def leave + super + + if current_user.authorized_by_provisioning_group?(membershipable) + sign_out current_user + end + end + end +end diff --git a/ee/app/controllers/groups/sso_controller.rb b/ee/app/controllers/groups/sso_controller.rb index 004c61c81652bf90169040d2811cc5da28d21ce4..e177d8fd20c840c56ef60af30a2fba50c889d03e 100644 --- a/ee/app/controllers/groups/sso_controller.rb +++ b/ee/app/controllers/groups/sso_controller.rb @@ -33,7 +33,11 @@ def unlink GroupSaml::Identity::DestroyService.new(linked_identity).execute - redirect_to profile_account_path + if current_user.authorized_by_provisioning_group?(unauthenticated_group) + sign_out current_user + else + redirect_to profile_account_path + end end def sign_up_form diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb index b8a5a6b5d142b1d3ec94d149ceaae522d62cf6d7..26f3d3fa3109eadf5597b632586da45eab8d3770 100644 --- a/ee/app/models/ee/user.rb +++ b/ee/app/models/ee/user.rb @@ -314,6 +314,7 @@ def admin_unsubscribe! override :allow_password_authentication_for_web? def allow_password_authentication_for_web?(*) return false if group_managed_account? + return false if user_authorized_by_provisioning_group? super end @@ -321,10 +322,19 @@ def allow_password_authentication_for_web?(*) override :allow_password_authentication_for_git? def allow_password_authentication_for_git?(*) return false if group_managed_account? + return false if user_authorized_by_provisioning_group? super end + def user_authorized_by_provisioning_group? + ::Feature.enabled?(:block_password_auth_for_saml_users, type: :ops) && user_detail.provisioned_by_group? + end + + def authorized_by_provisioning_group?(group) + ::Feature.enabled?(:block_password_auth_for_saml_users, type: :ops) && provisioned_by_group == group + end + def gitlab_employee? strong_memoize(:gitlab_employee) do ::Gitlab.com? && ::Feature.enabled?(:gitlab_employee_badge) && gitlab_team_member? diff --git a/ee/app/models/ee/user_detail.rb b/ee/app/models/ee/user_detail.rb index 24d026ec6c94448c3ec93240621680153bf5ded5..2c041b71b1058a3760356be3776dbee5b1a6e4a1 100644 --- a/ee/app/models/ee/user_detail.rb +++ b/ee/app/models/ee/user_detail.rb @@ -7,5 +7,9 @@ module UserDetail prepended do belongs_to :provisioned_by_group, class_name: 'Group', optional: true, inverse_of: :provisioned_user_details end + + def provisioned_by_group? + !!provisioned_by_group + end end end diff --git a/ee/lib/gitlab/auth/group_saml/user.rb b/ee/lib/gitlab/auth/group_saml/user.rb index 1048b64cca0542d0ae64e7d4036f52892c693d8a..aeaaa6e9629cf445b178977b0b1a240ef1e118c9 100644 --- a/ee/lib/gitlab/auth/group_saml/user.rb +++ b/ee/lib/gitlab/auth/group_saml/user.rb @@ -17,8 +17,9 @@ def initialize(auth_hash) override :find_and_update! def find_and_update! - save("GroupSaml Provider ##{saml_provider.id}") + add_or_update_user_identities + save("GroupSaml Provider ##{@saml_provider.id}") # Do not return un-persisted user so user is prompted # to sign-in to existing account. return unless valid_sign_in? @@ -37,7 +38,7 @@ def bypass_two_factor? override :gl_user def gl_user strong_memoize(:gl_user) do - identity&.user || build_new_user + identity&.user || find_by_email || build_new_user end end @@ -47,6 +48,14 @@ def identity end end + override :find_by_email + def find_by_email + user = super + return unless user&.authorized_by_provisioning_group?(saml_provider.group) + + user + end + override :build_new_user def build_new_user(skip_confirmation: false) super.tap do |user| @@ -72,6 +81,13 @@ def user_attributes end end + override :add_or_update_user_identities + def add_or_update_user_identities + super.tap do |identity| + identity.saml_provider_id = @saml_provider.id + end + end + def update_group_membership MembershipUpdater.new(gl_user, saml_provider, auth_hash).execute end diff --git a/ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb b/ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb index a90496f5fd18fc70bedca76cb754d795cebc717a..ac23e6210bdb1fca798d84ce1f9dfcfec8f48be3 100644 --- a/ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb +++ b/ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb @@ -115,6 +115,48 @@ def stub_last_request_id(id) end end + context 'when user used to be a member of a group' do + before do + user.provisioned_by_group = group + user.save! + end + + it "displays a flash message verifying group sign in" do + post provider, params: { group_id: group } + + expect(flash[:notice]).to match(/Signed in with SAML/i) + end + + it 'adds linked identity' do + expect { post provider, params: { group_id: group } }.to change(linked_accounts, :count) + end + + it 'adds group membership' do + expect { post provider, params: { group_id: group } }.to change { group.members.count } + end + end + + context 'when user was provisioned by other group' do + before do + user.provisioned_by_group = create(:group) + user.save! + end + + it "displays a flash message verifying group sign in" do + post provider, params: { group_id: group } + + expect(flash[:notice]).to eq('Login to a GitLab account to link with your SAML identity') + end + + it 'does not add linked identity' do + expect { post provider, params: { group_id: group } }.not_to change(linked_accounts, :count) + end + + it 'does not add group membership' do + expect { post provider, params: { group_id: group } }.not_to change { group.members.count } + end + end + context "when signed in" do before do sign_in(user) diff --git a/ee/spec/controllers/groups/sso_controller_spec.rb b/ee/spec/controllers/groups/sso_controller_spec.rb index 8b9ef436e2bf1d07e3129eb4046398dd311ed568..4e2fe2940c6fa1deddd40244d1e706787df1f47c 100644 --- a/ee/spec/controllers/groups/sso_controller_spec.rb +++ b/ee/spec/controllers/groups/sso_controller_spec.rb @@ -34,12 +34,41 @@ expect(assigns[:group_name]).to eq 'our-group' end - it 'allows account unlinking' do - create(:group_saml_identity, saml_provider: saml_provider, user: user) + context 'unlinking user' do + before do + create(:group_saml_identity, saml_provider: saml_provider, user: user) + user.update!(provisioned_by_group: saml_provider.group) + end + + it 'allows account unlinking' do + expect do + delete :unlink, params: { group_id: group } + end.to change(Identity, :count).by(-1) + end + + context 'with block_password_auth_for_saml_users feature flag switched off' do + before do + stub_feature_flags(block_password_auth_for_saml_users: false) + end - expect do - delete :unlink, params: { group_id: group } - end.to change(Identity, :count).by(-1) + it 'does not sign out user provisioned by this group' do + delete :unlink, params: { group_id: group } + + expect(response).to redirect_to(profile_account_path) + end + end + + context 'with block_password_auth_for_saml_users feature flag switched on' do + before do + stub_feature_flags(block_password_auth_for_saml_users: true) + end + + it 'signs out user provisioned by this group' do + delete :unlink, params: { group_id: group } + + expect(subject.current_user).to eq(nil) + end + end end context 'when SAML is disabled for the group' do diff --git a/ee/spec/features/groups/members/leave_group_spec.rb b/ee/spec/features/groups/members/leave_group_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..4b2ccb0d2c5cf05bc35cf26426411799c27231c8 --- /dev/null +++ b/ee/spec/features/groups/members/leave_group_spec.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Groups > Members > Leave group' do + include Spec::Support::Helpers::Features::MembersHelpers + + let_it_be(:other_user) { create(:user) } + let_it_be(:group) { create(:group) } + + let(:user) { create(:user) } + + before do + user.update!(provisioned_by_group: group) + sign_in(user) + end + + context 'with block_password_auth_for_saml_users feature flag switched on' do + it 'guest provisoned by this group leaves the group and is signed off' do + group.add_guest(user) + group.add_owner(other_user) + + visit group_path(group) + click_link 'Leave group' + + expect(group.users).not_to include(user) + expect(current_path).to eq(new_user_session_path) + end + + it 'guest leaves the group by url param and is signed off', :js do + group.add_guest(user) + group.add_owner(other_user) + + visit group_path(group, leave: 1) + + page.accept_confirm + + wait_for_all_requests + expect(current_path).to eq(new_user_session_path) + expect(group.users).not_to include(user) + end + end + + context 'with block_password_auth_for_saml_users feature flag switched off' do + before do + stub_feature_flags(block_password_auth_for_saml_users: false) + end + + it 'guest leaves the group by url param', :js do + group.add_guest(user) + group.add_owner(other_user) + + visit group_path(group, leave: 1) + + page.accept_confirm + + wait_for_all_requests + expect(current_path).to eq(dashboard_groups_path) + expect(group.users).not_to include(user) + end + + it 'guest leaves the group as last member' do + group.add_guest(user) + + visit group_path(group) + click_link 'Leave group' + + expect(current_path).to eq(dashboard_groups_path) + expect(group.users).not_to include(user) + end + end +end diff --git a/ee/spec/lib/gitlab/auth/group_saml/user_spec.rb b/ee/spec/lib/gitlab/auth/group_saml/user_spec.rb index ce53ad54a6211ee01da3391878b68ece2a9d6fb5..7c3425da769e9d7a6f75546df6e802b3fcb1da1b 100644 --- a/ee/spec/lib/gitlab/auth/group_saml/user_spec.rb +++ b/ee/spec/lib/gitlab/auth/group_saml/user_spec.rb @@ -1,8 +1,6 @@ # frozen_string_literal: true require 'spec_helper' -require 'spec_helper' - RSpec.describe Gitlab::Auth::GroupSaml::User do let(:uid) { 1234 } let(:saml_provider) { create(:saml_provider) } @@ -107,9 +105,7 @@ def create_existing_identity end context 'when a conflicting user already exists' do - before do - create(:user, email: info_hash[:email]) - end + let!(:user) { create(:user, email: info_hash[:email]) } it 'does not update membership' do expect { find_and_update }.not_to change { group.members.count } @@ -118,6 +114,60 @@ def create_existing_identity it 'does not return a user' do expect(find_and_update).to eq nil end + + context 'when user was provisioned by this group' do + before do + user.update!(provisioned_by_group: group) + end + + it 'updates membership' do + expect { find_and_update }.to change { group.members.count }.by(1) + end + + it 'returns a user' do + expect(find_and_update).to eq user + end + + it 'updates idenitity' do + expect { find_and_update }.to change { user.group_saml_identities.count }.by(1) + end + + context 'without feature flag turned on' do + before do + stub_feature_flags(block_password_auth_for_saml_users: false) + end + + it 'does not update membership' do + expect { find_and_update }.not_to change { group.members.count } + end + + it 'does not return a user' do + expect(find_and_update).to eq nil + end + + it 'does not update identity' do + expect { find_and_update }.not_to change { user.group_saml_identities.count } + end + end + end + + context 'when user was provisioned by different group' do + before do + user.update!(provisioned_by_group: create(:group)) + end + + it 'does not update membership' do + expect { find_and_update }.not_to change { group.members.count } + end + + it 'does not return a user' do + expect(find_and_update).to eq nil + end + + it 'does not update identity' do + expect { find_and_update }.not_to change { user.group_saml_identities.count } + end + end end end end diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb index a94793dc5f0c4d61d16d09a99a5362d4bd5cf74a..0674c668006ccf7b32bba7613aed3ae228936c4f 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -753,24 +753,148 @@ describe '#allow_password_authentication_for_web?' do context 'when user has managing group linked' do before do - user.managing_group = Group.new + user.managing_group = build(:group) + end + + it 'is false' do + expect(user.allow_password_authentication_for_web?).to eq false + end + end + + context 'when user is provisioned by group' do + before do + user.user_detail.provisioned_by_group = build(:group) end it 'is false' do expect(user.allow_password_authentication_for_web?).to eq false end + + context 'with feature flag switched off' do + before do + stub_feature_flags(block_password_auth_for_saml_users: false) + end + + it 'is true' do + expect(user.allow_password_authentication_for_web?).to eq true + end + end end end describe '#allow_password_authentication_for_git?' do context 'when user has managing group linked' do before do - user.managing_group = Group.new + user.managing_group = build(:group) + end + + it 'is false' do + expect(user.allow_password_authentication_for_git?).to eq false + end + end + + context 'when user is provisioned by group' do + before do + user.user_detail.provisioned_by_group = build(:group) end it 'is false' do expect(user.allow_password_authentication_for_git?).to eq false end + + context 'with feature flag switched off' do + before do + stub_feature_flags(block_password_auth_for_saml_users: false) + end + + it 'is true' do + expect(user.allow_password_authentication_for_git?).to eq true + end + end + end + end + + describe '#user_authorized_by_provisioning_group?' do + context 'when user is provisioned by group' do + before do + user.user_detail.provisioned_by_group = build(:group) + end + + it 'is true' do + expect(user.user_authorized_by_provisioning_group?).to eq true + end + + context 'with feature flag switched off' do + before do + stub_feature_flags(block_password_auth_for_saml_users: false) + end + + it 'is false' do + expect(user.user_authorized_by_provisioning_group?).to eq false + end + end + end + + context 'when user is not provisioned by group' do + it 'is false' do + expect(user.user_authorized_by_provisioning_group?).to eq false + end + + context 'with feature flag switched off' do + before do + stub_feature_flags(block_password_auth_for_saml_users: false) + end + + it 'is false' do + expect(user.user_authorized_by_provisioning_group?).to eq false + end + end + end + end + + describe '#authorized_by_provisioning_group?' do + let_it_be(:group) { create(:group) } + + context 'when user is provisioned by group' do + before do + user.user_detail.provisioned_by_group = group + end + + it 'is true' do + expect(user.authorized_by_provisioning_group?(group)).to eq true + end + + context 'when other group is provided' do + it 'is false' do + expect(user.authorized_by_provisioning_group?(create(:group))).to eq false + end + end + + context 'with feature flag switched off' do + before do + stub_feature_flags(block_password_auth_for_saml_users: false) + end + + it 'is false' do + expect(user.authorized_by_provisioning_group?(group)).to eq false + end + end + end + + context 'when user is not provisioned by group' do + it 'is false' do + expect(user.authorized_by_provisioning_group?(group)).to eq false + end + + context 'with feature flag switched off' do + before do + stub_feature_flags(block_password_auth_for_saml_users: false) + end + + it 'is false' do + expect(user.authorized_by_provisioning_group?(group)).to eq false + end + end end end diff --git a/ee/spec/models/user_detail_spec.rb b/ee/spec/models/user_detail_spec.rb index 4b1c0e12b6834a9cc5c8d2c1e429544c99a63003..051735b10cda9c06777758cdcc9846d193d3ffa3 100644 --- a/ee/spec/models/user_detail_spec.rb +++ b/ee/spec/models/user_detail_spec.rb @@ -4,4 +4,20 @@ RSpec.describe UserDetail do it { is_expected.to belong_to(:provisioned_by_group) } + + describe '#provisioned_by_group?' do + let(:user) { create(:user, provisioned_by_group: build(:group)) } + + subject { user.user_detail.provisioned_by_group? } + + it 'returns true when user is provisoned by group' do + expect(subject).to eq(true) + end + + it 'returns true when user is provisoned by group' do + user.user_detail.update!(provisioned_by_group: nil) + + expect(subject).to eq(false) + end + end end diff --git a/lib/gitlab/auth/o_auth/user.rb b/lib/gitlab/auth/o_auth/user.rb index fe1bf730e76b9aa39e6ea13a3ae47ab843b30e40..5f29bb8f43383188161814d11a6c4784c10989e2 100644 --- a/lib/gitlab/auth/o_auth/user.rb +++ b/lib/gitlab/auth/o_auth/user.rb @@ -115,6 +115,8 @@ def add_or_update_user_identities log.info "Correct LDAP account has been found. identity to user: #{gl_user.username}." gl_user.identities.build(provider: ldap_person.provider, extern_uid: ldap_person.dn) end + + identity end def find_or_build_ldap_user diff --git a/spec/features/groups_spec.rb b/spec/features/groups_spec.rb index 33d2ac50628b72f0455eeed51467200ee9ff08ec..27b75d15d3b9c9069791775205c3b37aa4f75aa3 100644 --- a/spec/features/groups_spec.rb +++ b/spec/features/groups_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe 'Group' do - let_it_be(:user) { create(:user) } + let(:user) { create(:user) } before do sign_in(user)