diff --git a/ee/app/finders/concerns/projects/filterable.rb b/ee/app/finders/concerns/projects/filterable.rb index 9a3a03ecbc5de8a449626b88ce1640285d2fc946..ffc501870c2254510fe14def6057e0da9022cf42 100644 --- a/ee/app/finders/concerns/projects/filterable.rb +++ b/ee/app/finders/concerns/projects/filterable.rb @@ -7,15 +7,16 @@ module Filterable private def by_saml_sso_session(projects) - return projects unless filter_expired_saml_session_projects? + return projects unless params.fetch(:filter_expired_saml_session_projects, false) + return projects unless current_user - projects.by_not_in_root_id(current_user.expired_sso_session_saml_providers.select(:group_id)) - end + saml_providers_to_exclude = current_user.expired_sso_session_saml_providers_with_access_restricted - def filter_expired_saml_session_projects? - return false if current_user.nil? || current_user.can_read_all_resources? + return projects if saml_providers_to_exclude.blank? - params.fetch(:filter_expired_saml_session_projects, false) + projects.by_not_in_root_id( + saml_providers_to_exclude.map(&:group_id) + ) end end end diff --git a/ee/app/graphql/ee/resolvers/projects/user_contributed_projects_resolver.rb b/ee/app/graphql/ee/resolvers/projects/user_contributed_projects_resolver.rb index 366c4ad3961bd1fb54765aecf81edec1fda6d00f..de51bb60fcbcf5f7aefca5a1fa49669cf1a2785f 100644 --- a/ee/app/graphql/ee/resolvers/projects/user_contributed_projects_resolver.rb +++ b/ee/app/graphql/ee/resolvers/projects/user_contributed_projects_resolver.rb @@ -8,9 +8,11 @@ module UserContributedProjectsResolver override :finder_params def finder_params(args) - # Expired SAML session filter disabled for now. - # Further investigation needed in https://gitlab.com/gitlab-org/gitlab/-/issues/514406 - super.merge(filter_expired_saml_session_projects: false) + super.merge( + filter_expired_saml_session_projects: ::Feature.enabled?( + :filter_saml_enforced_resources_from_graphql, current_user + ) + ) end end end diff --git a/ee/app/graphql/ee/resolvers/projects_resolver.rb b/ee/app/graphql/ee/resolvers/projects_resolver.rb index 9be80d4830b9a4eb59166f29c92b56d2222d0ccf..14d8a05e6da935f96e3f377bc73ac786e92f6875 100644 --- a/ee/app/graphql/ee/resolvers/projects_resolver.rb +++ b/ee/app/graphql/ee/resolvers/projects_resolver.rb @@ -32,9 +32,11 @@ module ProjectsResolver def finder_params(args) super(args) .merge(args.slice(:aimed_for_deletion, :include_hidden, :marked_for_deletion_on)) - # Expired SAML session filter disabled for now. - # Further investigation needed in https://gitlab.com/gitlab-org/gitlab/-/issues/514406 - .merge(filter_expired_saml_session_projects: false) + .merge( + filter_expired_saml_session_projects: ::Feature.enabled?( + :filter_saml_enforced_resources_from_graphql, current_user + ) + ) end end end diff --git a/ee/app/graphql/ee/resolvers/user_starred_projects_resolver.rb b/ee/app/graphql/ee/resolvers/user_starred_projects_resolver.rb index df057ed6cb129b05f82f944d5c5754ffbc72c8f8..ce27b1c506d8d6f07f23f043e4cbaefbcee5f291 100644 --- a/ee/app/graphql/ee/resolvers/user_starred_projects_resolver.rb +++ b/ee/app/graphql/ee/resolvers/user_starred_projects_resolver.rb @@ -7,9 +7,11 @@ module UserStarredProjectsResolver # rubocop:disable Gitlab/BoundedContexts -- n override :finder_params def finder_params(args) - # Expired SAML session filter disabled for now. - # Further investigation needed in https://gitlab.com/gitlab-org/gitlab/-/issues/514406 - super.merge(filter_expired_saml_session_projects: false) + super.merge( + filter_expired_saml_session_projects: ::Feature.enabled?( + :filter_saml_enforced_resources_from_graphql, current_user + ) + ) end end end diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb index f68e5f6a6c7b7934155ac82c658ff6520cdb8de9..fc2996f8b3c226b96773b531008ae05d8aeb3f97 100644 --- a/ee/app/models/ee/user.rb +++ b/ee/app/models/ee/user.rb @@ -312,6 +312,12 @@ def toggle_star(project) project.maintain_elasticsearch_update if self.active? && project.maintaining_elasticsearch? end + def expired_sso_session_saml_providers_with_access_restricted + expired_sso_session_saml_providers.select do |saml_provider| + ::Gitlab::Auth::GroupSaml::SsoEnforcer.new(saml_provider, user: self).access_restricted? + end + end + def expired_sso_session_saml_providers group_saml_providers.id_not_in(active_sso_sessions_saml_provider_ids) end diff --git a/ee/config/feature_flags/gitlab_com_derisk/filter_saml_enforced_resources_from_graphql.yml b/ee/config/feature_flags/gitlab_com_derisk/filter_saml_enforced_resources_from_graphql.yml new file mode 100644 index 0000000000000000000000000000000000000000..c58d026e9e873f20e29811214eaa19431ea1eeb0 --- /dev/null +++ b/ee/config/feature_flags/gitlab_com_derisk/filter_saml_enforced_resources_from_graphql.yml @@ -0,0 +1,9 @@ +--- +name: filter_saml_enforced_resources_from_graphql +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/514406 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/180998 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/517989 +milestone: '17.9' +group: group::organizations +type: gitlab_com_derisk +default_enabled: false diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb index 9fdc065f8754de3952052a0d7ca4c74e3707dc5d..d6efc4cdba53e71ca2f59b36d190c3d70486f3a7 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -409,6 +409,43 @@ end end + describe '.expired_sso_session_saml_providers_with_access_restricted' do + let_it_be(:user) { create(:user) } + let_it_be(:saml_provider) { create(:saml_provider) } + + before do + allow(user).to receive(:expired_sso_session_saml_providers).and_return([saml_provider]) + end + + subject(:expired_sso_session_saml_providers_with_access_restricted) do + user.expired_sso_session_saml_providers_with_access_restricted + end + + context 'when SAML provider has access restricted' do + before do + allow_next_instance_of(::Gitlab::Auth::GroupSaml::SsoEnforcer) do |sso_enforcer| + allow(sso_enforcer).to receive(:access_restricted?).and_return(true) + end + end + + it 'returns array including SAML provider' do + expect(expired_sso_session_saml_providers_with_access_restricted).to include(saml_provider) + end + end + + context 'when SAML provider does not have access restricted' do + before do + allow_next_instance_of(::Gitlab::Auth::GroupSaml::SsoEnforcer) do |sso_enforcer| + allow(sso_enforcer).to receive(:access_restricted?).and_return(false) + end + end + + it 'returns array excluding SAML provider' do + expect(expired_sso_session_saml_providers_with_access_restricted).not_to include(saml_provider) + end + end + end + describe '.expired_sso_session_saml_providers' do let_it_be(:user) { create(:user) } let_it_be(:saml_provider1) { create(:saml_provider) } diff --git a/ee/spec/requests/api/graphql/projects/projects_spec.rb b/ee/spec/requests/api/graphql/projects/projects_spec.rb index b6fbd98ade1c743dcc835b158aa96d061952b90e..93b0433abb038bbf35c215b219a82d4eebdc23b5 100644 --- a/ee/spec/requests/api/graphql/projects/projects_spec.rb +++ b/ee/spec/requests/api/graphql/projects/projects_spec.rb @@ -103,4 +103,6 @@ end.not_to exceed_query_limit(control) end end + + it_behaves_like 'projects graphql query with SAML session filtering' end diff --git a/ee/spec/requests/api/graphql/user/contributed_projects_query_spec.rb b/ee/spec/requests/api/graphql/user/contributed_projects_query_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..cfc8567ddbac85a5dbc5f902d8a4685ca7fe53d2 --- /dev/null +++ b/ee/spec/requests/api/graphql/user/contributed_projects_query_spec.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Getting contributedProjects of the user', feature_category: :groups_and_projects do + include GraphqlHelpers + + let_it_be(:current_user) { create(:user) } + let_it_be(:user_params) { { username: current_user.username } } + let_it_be(:user_fields) { 'contributedProjects { nodes { id } }' } + let_it_be(:query) { graphql_query_for(:user, user_params, user_fields) } + let_it_be(:path) { %i[user contributed_projects nodes] } + + it_behaves_like 'projects graphql query with SAML session filtering' do + before do + travel_to(4.hours.from_now) { create(:push_event, project: saml_project, author: current_user) } + end + end +end diff --git a/ee/spec/requests/api/graphql/user/starred_projects_query_spec.rb b/ee/spec/requests/api/graphql/user/starred_projects_query_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..6d74c8735fa70338f67a61be746e9a706a8eaacd --- /dev/null +++ b/ee/spec/requests/api/graphql/user/starred_projects_query_spec.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Getting starredProjects of the user', feature_category: :groups_and_projects do + include GraphqlHelpers + + let_it_be(:current_user) { create(:user) } + let_it_be(:path) { %i[user starred_projects nodes] } + let_it_be(:user_params) { { username: current_user.username } } + let_it_be(:user_fields) { 'starredProjects { nodes { id } }' } + let_it_be(:query) do + graphql_query_for(:user, user_params, user_fields) + end + + it_behaves_like 'projects graphql query with SAML session filtering' do + before do + current_user.toggle_star(saml_project) + end + end +end diff --git a/ee/spec/requests/ee/api/graphql/organizations/organization_query_spec.rb b/ee/spec/requests/ee/api/graphql/organizations/organization_query_spec.rb index 01fef0a0c4abe87c8cbdbb44287cee14e1ddca76..444280b6a2a078aa20ea6a385d31879abcb17ba4 100644 --- a/ee/spec/requests/ee/api/graphql/organizations/organization_query_spec.rb +++ b/ee/spec/requests/ee/api/graphql/organizations/organization_query_spec.rb @@ -15,6 +15,26 @@ subject(:request_organization) { post_graphql(query, current_user: current_user) } + context 'when requesting projects' do + let(:path) { %i[organization projects nodes] } + let(:organization_fields) do + <<~FIELDS + projects(first: 1) { + nodes { + id + } + } + FIELDS + end + + it_behaves_like 'projects graphql query with SAML session filtering' do + before do + saml_group.organization = organization + saml_project.organization = organization + end + end + end + context 'when requesting groups' do let_it_be(:saml_group) do create(:group, organization: organization, developers: current_user) do |group| diff --git a/ee/spec/support/shared_examples/finders/projects/saml_session_filtering_shared_examples.rb b/ee/spec/support/shared_examples/finders/projects/saml_session_filtering_shared_examples.rb index 658dd4ab07a009df15358052b3bf2260ebcc8661..0b01ce580b26aa105eb8dbb9ada46f998d6bea85 100644 --- a/ee/spec/support/shared_examples/finders/projects/saml_session_filtering_shared_examples.rb +++ b/ee/spec/support/shared_examples/finders/projects/saml_session_filtering_shared_examples.rb @@ -5,9 +5,9 @@ let_it_be(:current_user) { user } - let_it_be(:saml_provider1) { create(:saml_provider) } - let_it_be(:saml_provider2) { create(:saml_provider) } - let_it_be(:saml_provider3) { create(:saml_provider) } + let_it_be(:saml_provider1) { create(:saml_provider, enabled: true, enforced_sso: true) } + let_it_be(:saml_provider2) { create(:saml_provider, enabled: true, enforced_sso: true) } + let_it_be(:saml_provider3) { create(:saml_provider, enabled: true, enforced_sso: true) } let_it_be(:root_group1) do create(:group, saml_provider: saml_provider1, developers: current_user) do |group| @@ -30,6 +30,10 @@ let_it_be(:private_project) { create(:project, :private, group: private_root_group) } let_it_be(:all_projects) { [project1, project2, private_project] } + before do + stub_licensed_features(group_saml: true) + end + subject(:projects) { finder.execute.id_in(all_projects).to_a } context 'when the current user is nil' do @@ -53,8 +57,21 @@ end context 'when the current user has no active SAML sessions' do - it 'filters out the SAML member projects' do - expect(projects).to contain_exactly(project2) + context 'when in the context of web activity' do + around do |example| + session = { 'warden.user.user.key' => [[current_user.id], current_user.authenticatable_salt] } + Gitlab::Session.with_session(session) do + example.run + end + end + + it 'filters out the SAML member projects' do + expect(projects).to contain_exactly(project2) + end + end + + context 'when not in the context of web activity' do + it_behaves_like 'includes all SAML projects' end end diff --git a/ee/spec/support/shared_examples/requests/api/graphql/projects/saml_session_filtering_shared_examples.rb b/ee/spec/support/shared_examples/requests/api/graphql/projects/saml_session_filtering_shared_examples.rb index 552aa5ad9306e7974df212a4b2b9a23b6acffe72..4512eacae7d3f14fc0fa61f4a82475d8afa1d4b4 100644 --- a/ee/spec/support/shared_examples/requests/api/graphql/projects/saml_session_filtering_shared_examples.rb +++ b/ee/spec/support/shared_examples/requests/api/graphql/projects/saml_session_filtering_shared_examples.rb @@ -3,7 +3,7 @@ RSpec.shared_examples 'projects graphql query with SAML session filtering' do include GraphqlHelpers - let_it_be(:saml_provider) { create(:saml_provider) } + let_it_be(:saml_provider) { create(:saml_provider, enabled: true, enforced_sso: true) } let_it_be(:saml_group) do create(:group, saml_provider: saml_provider, developers: current_user) do |group| create(:group_saml_identity, saml_provider: group.saml_provider, user: current_user) @@ -12,12 +12,11 @@ let_it_be(:saml_project) { create(:project, group: saml_group, developers: current_user) } - context 'when current user has an active SAML session' do - before do - active_saml_sessions = { saml_group.saml_provider.id => Time.current } - allow(::Gitlab::Auth::GroupSaml::SsoState).to receive(:active_saml_sessions).and_return(active_saml_sessions) - end + before do + stub_licensed_features(group_saml: true) + end + shared_examples 'includes SAML project' do it 'includes SAML project' do post_graphql(query, current_user: current_user) @@ -25,11 +24,33 @@ end end + context 'when current user has an active SAML session' do + before do + active_saml_sessions = { saml_group.saml_provider.id => Time.current } + allow(::Gitlab::Auth::GroupSaml::SsoState).to receive(:active_saml_sessions).and_return(active_saml_sessions) + end + + it_behaves_like 'includes SAML project' + end + context 'when current user has no active SAML session' do - it 'excludes SAML project' do - post_graphql(query, current_user: current_user) + context 'when in the context of web activity' do + around do |example| + session = { 'warden.user.user.key' => [[current_user.id], current_user.authenticatable_salt] } + Gitlab::Session.with_session(session) do + example.run + end + end + + it 'excludes SAML project' do + post_graphql(query, current_user: current_user) + + expect(graphql_data_at(*path)).not_to include(a_graphql_entity_for(saml_project)) + end + end - expect(graphql_data_at(*path)).not_to include(a_graphql_entity_for(saml_project)) + context 'when not in the context of web activity' do + it_behaves_like 'includes SAML project' end end end