From 02ecabc6dd80b6a700aeec569014642dcb30704f Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Fri, 26 Jul 2024 13:08:33 +0300 Subject: [PATCH] Remove block_password_auth_for_saml_users FF and the accompanying code Related to https://gitlab.com/gitlab-org/gitlab/-/issues/373718 Changelog: other EE: true --- app/models/user.rb | 5 - .../block_password_auth_for_saml_users.yml | 8 - .../concerns/ee/membership_actions.rb | 9 - ee/app/controllers/groups/sso_controller.rb | 6 +- ee/app/models/ee/user.rb | 15 -- ee/app/models/ee/user_detail.rb | 4 - .../ee/sessions_controller_spec.rb | 12 -- .../controllers/groups/sso_controller_spec.rb | 39 +--- .../groups/members/leave_group_spec.rb | 78 -------- ee/spec/models/ee/user_detail_spec.rb | 16 -- ee/spec/models/ee/user_spec.rb | 182 ------------------ ee/spec/requests/git_http_spec.rb | 98 ---------- lib/gitlab/auth/database/authentication.rb | 1 - lib/gitlab/auth/o_auth/user.rb | 2 - spec/support/rspec_order_todo.yml | 1 - 15 files changed, 6 insertions(+), 470 deletions(-) delete mode 100644 config/feature_flags/ops/block_password_auth_for_saml_users.yml delete mode 100644 ee/spec/features/groups/members/leave_group_spec.rb diff --git a/app/models/user.rb b/app/models/user.rb index 3aa9ba813da8b1..cfec491ed9651c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1462,11 +1462,6 @@ def allow_password_authentication_for_git? Gitlab::CurrentSettings.password_authentication_enabled_for_git? && !password_based_omniauth_user? end - # method overriden in EE - def password_based_login_forbidden? - false - end - def can_change_username? gitlab_config.username_changing_enabled end 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 deleted file mode 100644 index ffc04d14a19fba..00000000000000 --- a/config/feature_flags/ops/block_password_auth_for_saml_users.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: block_password_auth_for_saml_users -introduced_by_url: -rollout_issue_url: -milestone: '13.11' -type: ops -group: group::authentication -default_enabled: false diff --git a/ee/app/controllers/concerns/ee/membership_actions.rb b/ee/app/controllers/concerns/ee/membership_actions.rb index a0b889dbe5ee7f..1ceea4182b9a16 100644 --- a/ee/app/controllers/concerns/ee/membership_actions.rb +++ b/ee/app/controllers/concerns/ee/membership_actions.rb @@ -4,15 +4,6 @@ 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 - private def update_params diff --git a/ee/app/controllers/groups/sso_controller.rb b/ee/app/controllers/groups/sso_controller.rb index f2b2ddeeb49221..3d6195dd647974 100644 --- a/ee/app/controllers/groups/sso_controller.rb +++ b/ee/app/controllers/groups/sso_controller.rb @@ -36,11 +36,7 @@ def unlink GroupSaml::Identity::DestroyService.new(linked_identity).execute - if current_user.authorized_by_provisioning_group?(unauthenticated_group) - sign_out current_user - else - redirect_to profile_account_path - end + redirect_to profile_account_path end private diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb index 930bcf6e3f8d79..d68b94f118fcfb 100644 --- a/ee/app/models/ee/user.rb +++ b/ee/app/models/ee/user.rb @@ -521,7 +521,6 @@ def ldap_sync_time 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? return false if password_authentication_disabled_by_enterprise_group? super @@ -530,7 +529,6 @@ 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? return false if password_authentication_disabled_by_enterprise_group? super @@ -543,19 +541,6 @@ def password_authentication_disabled_by_enterprise_group? enterprise_group.saml_provider.enabled? && enterprise_group.saml_provider.disable_password_authentication_for_enterprise_users? end - override :password_based_login_forbidden? - def password_based_login_forbidden? - user_authorized_by_provisioning_group? || super - end - - def user_authorized_by_provisioning_group? - user_detail.provisioned_by_group? && ::Feature.enabled?(:block_password_auth_for_saml_users, user_detail.provisioned_by_group, type: :ops) - end - - def authorized_by_provisioning_group?(group) - user_authorized_by_provisioning_group? && provisioned_by_group == group - end - def enterprise_user_of_group?(group) enterprise_group_id == group.id end diff --git a/ee/app/models/ee/user_detail.rb b/ee/app/models/ee/user_detail.rb index 9c9fcafd80af06..7778ce08031be2 100644 --- a/ee/app/models/ee/user_detail.rb +++ b/ee/app/models/ee/user_detail.rb @@ -16,9 +16,5 @@ module UserDetail prefix: true ) end - - def provisioned_by_group? - !!provisioned_by_group - end end end diff --git a/ee/spec/controllers/ee/sessions_controller_spec.rb b/ee/spec/controllers/ee/sessions_controller_spec.rb index a445eb899f0562..f2c9b2660ab6bc 100644 --- a/ee/spec/controllers/ee/sessions_controller_spec.rb +++ b/ee/spec/controllers/ee/sessions_controller_spec.rb @@ -137,18 +137,6 @@ def authenticate_2fa(otp_user_id: user.id, **user_params) end end - context 'when user is not allowed to log in using password' do - let_it_be(:user) { create(:user, provisioned_by_group: build(:group)) } - - it 'does not authenticate the user' do - post(:create, params: { user: { login: user.username, password: user.password } }) - - expect(response).to have_gitlab_http_status(:ok) - expect(@request.env['warden']).not_to be_authenticated - expect(flash[:alert]).to include(I18n.t('devise.failure.invalid')) - end - end - context 'when password authentication disabled by enterprise group' do let_it_be(:enterprise_group) { create(:group) } let_it_be(:saml_provider) do diff --git a/ee/spec/controllers/groups/sso_controller_spec.rb b/ee/spec/controllers/groups/sso_controller_spec.rb index 278a7c8eeb6881..65647c76ad9440 100644 --- a/ee/spec/controllers/groups/sso_controller_spec.rb +++ b/ee/spec/controllers/groups/sso_controller_spec.rb @@ -34,41 +34,12 @@ expect(assigns[:group_name]).to eq 'our-group' end - 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 + it 'allows account unlinking' do + create(:group_saml_identity, saml_provider: saml_provider, user: user) - 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 + expect do + delete :unlink, params: { group_id: group } + end.to change { Identity.count }.by(-1) 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 deleted file mode 100644 index 7dc39da3ae4f17..00000000000000 --- a/ee/spec/features/groups/members/leave_group_spec.rb +++ /dev/null @@ -1,78 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe 'Groups > Members > Leave group', feature_category: :groups_and_projects do - include Features::MembersHelpers - include Spec::Support::Helpers::ModalHelpers - - let_it_be(:other_user) { create(:user) } - let_it_be(:group) { create(:group) } - - let(:user) { create(:user) } - let(:more_actions_dropdown) do - find('[data-testid="groups-projects-more-actions-dropdown"] .gl-new-dropdown-custom-toggle') - end - - 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 provisioned by this group leaves the group and is signed off', :js do - group.add_guest(user) - group.add_owner(other_user) - - visit group_path(group) - more_actions_dropdown.click - click_link 'Leave group' - accept_gl_confirm(button_text: 'Leave group') - - expect(page).to have_current_path(new_user_session_path, ignore_query: true) - expect(group).not_to have_user(user) - 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) - accept_gl_confirm(button_text: 'Leave group') - wait_for_all_requests - - expect(page).to have_current_path(new_user_session_path, ignore_query: true) - expect(group).not_to have_user(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) - accept_gl_confirm(button_text: 'Leave group') - wait_for_all_requests - - expect(page).to have_current_path(dashboard_groups_path, ignore_query: true) - expect(group).not_to have_user(user) - end - - it 'guest leaves the group as last member', :js do - group.add_guest(user) - - visit group_path(group) - more_actions_dropdown.click - click_link 'Leave group' - accept_gl_confirm(button_text: 'Leave group') - - expect(page).to have_current_path(dashboard_groups_path, ignore_query: true) - expect(group).not_to have_user(user) - end - end -end diff --git a/ee/spec/models/ee/user_detail_spec.rb b/ee/spec/models/ee/user_detail_spec.rb index 618068c8074929..9f3a203120cc16 100644 --- a/ee/spec/models/ee/user_detail_spec.rb +++ b/ee/spec/models/ee/user_detail_spec.rb @@ -32,22 +32,6 @@ end end - 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 provisioned by group' do - expect(subject).to eq(true) - end - - it 'returns true when user is provisioned by group' do - user.user_detail.update!(provisioned_by_group: nil) - - expect(subject).to eq(false) - end - end - context 'with loose foreign key on user_details.provisioned_by_group_id' do it_behaves_like 'cleanup by a loose foreign key' do let_it_be(:parent) { create(:group) } diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb index f56992bcbb0f49..4de5a0568c2c88 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -1574,26 +1574,6 @@ 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 - context 'when password authentication disabled by enterprise group' do let_it_be(:enterprise_group) { create(:group) } let_it_be(:saml_provider) { create(:saml_provider, group: enterprise_group, enabled: true, disable_password_authentication_for_enterprise_users: true) } @@ -1617,26 +1597,6 @@ 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 - context 'when password authentication disabled by enterprise group' do let_it_be(:enterprise_group) { create(:group) } let_it_be(:saml_provider) { create(:saml_provider, group: enterprise_group, enabled: true, disable_password_authentication_for_enterprise_users: true) } @@ -1704,110 +1664,6 @@ end end - describe '#user_authorized_by_provisioning_group?' do - context 'when user is provisioned by group' do - let(:group) { build(:group) } - - before do - user.user_detail.provisioned_by_group = 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 - - context 'with feature flag switched on for particular groups' do - before do - stub_feature_flags(block_password_auth_for_saml_users: false) - end - - it 'is false when provisioned by group without feature flag' do - stub_feature_flags(block_password_auth_for_saml_users: create(:group)) - - expect(user.user_authorized_by_provisioning_group?).to eq false - end - - it 'is true when provisioned by group with feature flag' do - stub_feature_flags(block_password_auth_for_saml_users: group) - - expect(user.user_authorized_by_provisioning_group?).to eq true - 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 - describe '#password_authentication_disabled_by_enterprise_group?' do subject(:password_authentication_disabled_by_enterprise_group?) { user.password_authentication_disabled_by_enterprise_group? } @@ -1915,44 +1771,6 @@ end end - describe '#password_based_login_forbidden?' 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.password_based_login_forbidden?).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.password_based_login_forbidden?).to eq false - end - end - end - - context 'when user is not provisioned by group' do - it 'is false' do - expect(user.password_based_login_forbidden?).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.password_based_login_forbidden?).to eq false - end - end - end - end - describe '#using_license_seat?' do let(:user) { create(:user) } diff --git a/ee/spec/requests/git_http_spec.rb b/ee/spec/requests/git_http_spec.rb index c17973bba6e6b9..907b7127d14b21 100644 --- a/ee/spec/requests/git_http_spec.rb +++ b/ee/spec/requests/git_http_spec.rb @@ -155,104 +155,6 @@ it_behaves_like 'pulls are allowed' end - describe 'when user cannot use password-based login' do - let_it_be(:group) { create(:group) } - let_it_be(:project) { create(:project, :repository, :private, group: group) } - let_it_be(:user) { create(:user, provisioned_by_group: group) } - - let(:env) { { user: user.username, password: user.password } } - let(:path) { "#{project.full_path}.git" } - - before do - project.add_developer(user) - end - - context 'with feature flag switched off' do - before do - stub_feature_flags(block_password_auth_for_saml_users: false) - end - - it_behaves_like 'pulls are allowed' - it_behaves_like 'pushes are allowed' - end - - context 'with feature flag switched on' do - it 'responds with status 401 Unauthorized for pull action' do - download(path, **env) do |response| - expect(response).to have_gitlab_http_status(:unauthorized) - end - end - - it 'responds with status 401 Unauthorized for push action' do - upload(path, **env) do |response| - expect(response).to have_gitlab_http_status(:unauthorized) - end - end - - context 'when username and personal access token are provided' do - let(:user) { create(:user, provisioned_by_group: group) } - let(:access_token) { create(:personal_access_token, user: user) } - let(:env) { { user: user.username, password: access_token.token } } - - it_behaves_like 'pulls are allowed' - it_behaves_like 'pushes are allowed' - end - - context 'when user has 2FA enabled' do - let(:user) { create(:user, :two_factor, provisioned_by_group: group) } - let(:access_token) { create(:personal_access_token, user: user) } - - context 'when username and personal access token are provided' do - let(:env) { { user: user.username, password: access_token.token } } - - it_behaves_like 'pulls are allowed' - it_behaves_like 'pushes are allowed' - - it 'rejects the push attempt for read_repository scope' do - read_access_token = create(:personal_access_token, user: user, scopes: [:read_repository]) - - upload(path, user: user.username, password: read_access_token.token) do |response| - expect(response).to have_gitlab_http_status(:forbidden) - expect(response.body).to include('You are not allowed to upload code') - end - end - - it 'accepts the push attempt for write_repository scope' do - write_access_token = create(:personal_access_token, user: user, scopes: [:write_repository]) - - upload(path, user: user.username, password: write_access_token.token) do |response| - expect(response).to have_gitlab_http_status(:ok) - end - end - - it 'accepts the pull attempt for read_repository scope' do - read_access_token = create(:personal_access_token, user: user, scopes: [:read_repository]) - - download(path, user: user.username, password: read_access_token.token) do |response| - expect(response).to have_gitlab_http_status(:ok) - end - end - - it 'accepts the pull attempt for api scope' do - read_access_token = create(:personal_access_token, user: user, scopes: [:api]) - - download(path, user: user.username, password: read_access_token.token) do |response| - expect(response).to have_gitlab_http_status(:ok) - end - end - - it 'accepts the push attempt for api scope' do - write_access_token = create(:personal_access_token, user: user, scopes: [:api]) - - upload(path, user: user.username, password: write_access_token.token) do |response| - expect(response).to have_gitlab_http_status(:ok) - end - end - end - end - end - end - context 'when password authentication disabled by enterprise group' do let_it_be(:enterprise_group) { create(:group) } let_it_be(:saml_provider) { create(:saml_provider, group: enterprise_group, enabled: true, disable_password_authentication_for_enterprise_users: true) } diff --git a/lib/gitlab/auth/database/authentication.rb b/lib/gitlab/auth/database/authentication.rb index bf35a9abe41dd1..c0dc2b0875fd27 100644 --- a/lib/gitlab/auth/database/authentication.rb +++ b/lib/gitlab/auth/database/authentication.rb @@ -9,7 +9,6 @@ module Database class Authentication < Gitlab::Auth::OAuth::Authentication def login(login, password) return false unless Gitlab::CurrentSettings.password_authentication_enabled_for_git? - return false if user.password_based_login_forbidden? return user if user&.valid_password?(password) end diff --git a/lib/gitlab/auth/o_auth/user.rb b/lib/gitlab/auth/o_auth/user.rb index 8a376d1b0a413b..283ba4fa8a200e 100644 --- a/lib/gitlab/auth/o_auth/user.rb +++ b/lib/gitlab/auth/o_auth/user.rb @@ -134,8 +134,6 @@ 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/support/rspec_order_todo.yml b/spec/support/rspec_order_todo.yml index e002cb7157ebd6..01caae30d738b6 100644 --- a/spec/support/rspec_order_todo.yml +++ b/spec/support/rspec_order_todo.yml @@ -91,7 +91,6 @@ - './ee/spec/features/groups/iterations/user_views_iteration_cadence_spec.rb' - './ee/spec/features/groups/iterations/user_views_iteration_spec.rb' - './ee/spec/features/groups/ldap_group_links_spec.rb' -- './ee/spec/features/groups/members/leave_group_spec.rb' - './ee/spec/features/groups/members/list_members_spec.rb' - './ee/spec/features/groups/members/manage_groups_spec.rb' - './ee/spec/features/groups/members/manage_members_spec.rb' -- GitLab