From 79a847ba689fd756929c818dca55a24f7b3d4852 Mon Sep 17 00:00:00 2001 From: Drew Blessing Date: Mon, 20 Mar 2023 15:01:09 -0500 Subject: [PATCH] Show alert if user todos are hidden due to expired SSO session If a user's SSO session is expired and SSO is enforced, they may not see all todos. This alert notifies the user that they need to reauthenticate to see additional todos. --- app/models/todo.rb | 2 +- app/views/dashboard/todos/index.html.haml | 1 + ee/app/helpers/ee/todos_helper.rb | 8 +++ .../todos/_saml_reauth_notice.html.haml | 11 ++++ .../dashboard_saml_reauth_support.yml | 8 +++ ee/lib/gitlab/auth/group_saml/sso_enforcer.rb | 13 +++++ ee/spec/features/dashboards/todos_spec.rb | 39 +++++++++++++ ee/spec/helpers/ee/todos_helper_spec.rb | 57 +++++++++++++++++++ .../auth/group_saml/sso_enforcer_spec.rb | 34 +++++++++++ locale/gitlab.pot | 3 + 10 files changed, 175 insertions(+), 1 deletion(-) create mode 100644 ee/app/views/dashboard/todos/_saml_reauth_notice.html.haml create mode 100644 ee/config/feature_flags/development/dashboard_saml_reauth_support.yml diff --git a/app/models/todo.rb b/app/models/todo.rb index 62252912c32b5a..ac41b5d0b2c3d1 100644 --- a/app/models/todo.rb +++ b/app/models/todo.rb @@ -76,7 +76,7 @@ class Todo < ApplicationRecord scope :for_target, -> (id) { where(target_id: id) } scope :for_commit, -> (id) { where(commit_id: id) } scope :with_entity_associations, -> do - preload(:target, :author, :note, group: :route, project: [:route, { namespace: [:route, :owner] }, :project_setting]) + preload(:target, :author, :note, group: :route, project: [:route, :group, { namespace: [:route, :owner] }, :project_setting]) end scope :joins_issue_and_assignees, -> { left_joins(issue: :assignees) } scope :for_internal_notes, -> { joins(:note).where(note: { confidential: true }) } diff --git a/app/views/dashboard/todos/index.html.haml b/app/views/dashboard/todos/index.html.haml index 10b1e257ac60aa..ca6b1071f03364 100644 --- a/app/views/dashboard/todos/index.html.haml +++ b/app/views/dashboard/todos/index.html.haml @@ -2,6 +2,7 @@ = render_two_factor_auth_recovery_settings_check = render_dashboard_ultimate_trial(current_user) += render_if_exists 'dashboard/todos/saml_reauth_notice' - add_page_specific_style 'page_bundles/todos' - add_page_specific_style 'page_bundles/issuable' diff --git a/ee/app/helpers/ee/todos_helper.rb b/ee/app/helpers/ee/todos_helper.rb index e02a10709c4544..5068a03041c34e 100644 --- a/ee/app/helpers/ee/todos_helper.rb +++ b/ee/app/helpers/ee/todos_helper.rb @@ -18,5 +18,13 @@ def todo_author_display?(todo) def show_todo_state?(todo) super || (todo.target.is_a?(Epic) && todo.target.state == 'closed') end + + def todo_groups_requiring_saml_reauth(todos) + return [] unless ::Feature.enabled?(:dashboard_saml_reauth_support, current_user) + + groups = todos.map { |todo| todo.group || todo.project.group } + + ::Gitlab::Auth::GroupSaml::SsoEnforcer.access_restricted_groups(groups, user: current_user) + end end end diff --git a/ee/app/views/dashboard/todos/_saml_reauth_notice.html.haml b/ee/app/views/dashboard/todos/_saml_reauth_notice.html.haml new file mode 100644 index 00000000000000..4297349f627652 --- /dev/null +++ b/ee/app/views/dashboard/todos/_saml_reauth_notice.html.haml @@ -0,0 +1,11 @@ +- groups_requiring_saml_reauth = todo_groups_requiring_saml_reauth(@todos) +- return unless groups_requiring_saml_reauth.any? + += render Pajamas::AlertComponent.new(variant: :warning, dismissible: false) do |c| + = c.body do + = s_('GroupSAML|Some todos may be hidden because your SAML session has expired. Click to reauthenticate with the following groups to view hidden todos:') + = c.actions do + .gl-display-flex.gl-flex-wrap-wrap + - groups_requiring_saml_reauth.each do |group| + = render Pajamas::ButtonComponent.new(href: sso_group_saml_providers_path(group, { token: group.saml_discovery_token, redirect: dashboard_todos_path }), button_options: { class: "gl-mr-3 gl-mb-3" }) do + = group.path diff --git a/ee/config/feature_flags/development/dashboard_saml_reauth_support.yml b/ee/config/feature_flags/development/dashboard_saml_reauth_support.yml new file mode 100644 index 00000000000000..28e35c8be5a02e --- /dev/null +++ b/ee/config/feature_flags/development/dashboard_saml_reauth_support.yml @@ -0,0 +1,8 @@ +--- +name: dashboard_saml_reauth_support +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/115254 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/396592 +milestone: '15.11' +type: development +group: group::authentication and authorization +default_enabled: false diff --git a/ee/lib/gitlab/auth/group_saml/sso_enforcer.rb b/ee/lib/gitlab/auth/group_saml/sso_enforcer.rb index 5db1ae77b27b5a..2ebd660ef6941e 100644 --- a/ee/lib/gitlab/auth/group_saml/sso_enforcer.rb +++ b/ee/lib/gitlab/auth/group_saml/sso_enforcer.rb @@ -37,6 +37,19 @@ def self.group_access_restricted?(group, user: nil, for_project: false) new(saml_provider, user: user).access_restricted? end + # Given an array of groups or subgroups, return an array + # of root groups that are access restricted for the user + def self.access_restricted_groups(groups, user: nil) + return [] unless groups.any? + + ::Preloaders::GroupRootAncestorPreloader.new(groups, [:saml_provider]).execute + root_ancestors = groups.map(&:root_ancestor).uniq + + root_ancestors.select do |root_ancestor| + group_access_restricted?(root_ancestor, user: user, for_project: true) + end + end + private def saml_enforced? diff --git a/ee/spec/features/dashboards/todos_spec.rb b/ee/spec/features/dashboards/todos_spec.rb index 1453724c52e23f..93973e59fd437f 100644 --- a/ee/spec/features/dashboards/todos_spec.rb +++ b/ee/spec/features/dashboards/todos_spec.rb @@ -37,4 +37,43 @@ expect(page).to have_selector('a', text: user.to_reference) end end + + context 'when the user has todos in an SSO enforced group' do + let_it_be(:saml_provider) { create(:saml_provider, enabled: true, enforced_sso: true) } + let_it_be(:restricted_group) { create(:group, saml_provider: saml_provider) } + let_it_be(:epic_todo) do + create(:todo, group: restricted_group, user: user, target: create(:epic, group: restricted_group)) + end + + before do + stub_licensed_features(group_saml: true) + create(:group_saml_identity, user: user, saml_provider: saml_provider) + + restricted_group.add_owner(user) + + sign_in(user) + end + + context 'and the session is not active' do + it 'shows the user an alert', :aggregate_failures do + visit page_path + + expect(page).to have_content(s_('GroupSAML|Some todos may be hidden because your SAML session has expired. Click to reauthenticate with the following groups to view hidden todos:')) # rubocop:disable Layout/LineLength + expect(page).to have_link(restricted_group.path, href: /#{sso_group_saml_providers_path(restricted_group)}/) + end + end + + context 'and the session is active' do + before do + dummy_session = { active_group_sso_sign_ins: { saml_provider.id => DateTime.now } } + allow(Gitlab::Session).to receive(:current).and_return(dummy_session) + end + + it 'does not show the user an alert', :aggregate_failures do + visit page_path + + expect(page).not_to have_content(s_('GroupSAML|Some todos may be hidden because your SAML session has expired. Click to reauthenticate with the following groups to view hidden todos:')) # rubocop:disable Layout/LineLength + end + end + end end diff --git a/ee/spec/helpers/ee/todos_helper_spec.rb b/ee/spec/helpers/ee/todos_helper_spec.rb index c021dc5353735d..0a09cadba581c0 100644 --- a/ee/spec/helpers/ee/todos_helper_spec.rb +++ b/ee/spec/helpers/ee/todos_helper_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe ::TodosHelper do + include Devise::Test::ControllerHelpers + describe '#todo_types_options' do it 'includes options for an epic todo' do expect(helper.todo_types_options).to include( @@ -82,4 +84,59 @@ expect(helper.show_todo_state?(todo)).to eq(true) end end + + describe '#todo_groups_requiring_saml_reauth' do + let_it_be(:restricted_group) do + create(:group, saml_provider: create(:saml_provider, enabled: true, enforced_sso: true)) + end + + let_it_be(:restricted_group2) do + create(:group, saml_provider: create(:saml_provider, enabled: true, enforced_sso: true)) + end + + let_it_be(:restricted_subgroup) { create(:group, parent: restricted_group) } + let_it_be(:unrestricted_group) { create(:group) } + + let_it_be(:epic_todo) { create(:todo, group: restricted_group, target: create(:epic, group: restricted_subgroup)) } + + let_it_be(:restricted_project) { create(:project, namespace: restricted_group2) } + + let_it_be(:issue_todo) do + create(:todo, project: restricted_project, target: create(:issue, project: restricted_project)) + end + + let_it_be(:unrestricted_project) { create(:project, namespace: unrestricted_group) } + + let_it_be(:mr_todo) do + create(:todo, project: unrestricted_project, target: create(:merge_request, source_project: unrestricted_project)) + end + + let_it_be(:todos) { [epic_todo, issue_todo, mr_todo] } + + let(:session) { {} } + + before do + stub_licensed_features(group_saml: true) + end + + around do |example| + Gitlab::Session.with_session(session) do + example.run + end + end + + context 'when the feature flag is disabled' do + before do + stub_feature_flags(dashboard_saml_reauth_support: false) + end + + it 'returns an empty array of groups' do + expect(helper.todo_groups_requiring_saml_reauth(todos)).to match_array([]) + end + end + + it 'returns root groups for todos with targets in SSO enforced groups' do + expect(helper.todo_groups_requiring_saml_reauth(todos)).to match_array([restricted_group, restricted_group2]) + end + end end diff --git a/ee/spec/lib/gitlab/auth/group_saml/sso_enforcer_spec.rb b/ee/spec/lib/gitlab/auth/group_saml/sso_enforcer_spec.rb index 966182f47d3d9d..811db10968f957 100644 --- a/ee/spec/lib/gitlab/auth/group_saml/sso_enforcer_spec.rb +++ b/ee/spec/lib/gitlab/auth/group_saml/sso_enforcer_spec.rb @@ -219,4 +219,38 @@ end end end + + describe '.access_restricted_groups' do + let!(:restricted_group) { create(:group, saml_provider: create(:saml_provider, enabled: true, enforced_sso: true)) } + let!(:restricted_subgroup) { create(:group, parent: restricted_group) } + let!(:restricted_group2) do + create(:group, saml_provider: create(:saml_provider, enabled: true, enforced_sso: true)) + end + + let!(:unrestricted_group) { create(:group) } + let!(:unrestricted_subgroup) { create(:group, parent: unrestricted_group) } + let!(:groups) { [restricted_subgroup, restricted_group2, unrestricted_group, unrestricted_subgroup] } + + it 'handles empty groups array' do + expect(described_class.access_restricted_groups([])).to eq([]) + end + + it 'returns a list of SSO enforced root groups' do + expect(described_class.access_restricted_groups(groups)) + .to match_array([restricted_group, restricted_group2]) + end + + it 'returns only unique root groups' do + expect(described_class.access_restricted_groups(groups.push(restricted_group))) + .to match_array([restricted_group, restricted_group2]) + end + + it 'avoids N+1 queries' do + control = ActiveRecord::QueryRecorder.new(skip_cached: false) do + described_class.access_restricted_groups([restricted_group]) + end + + expect { described_class.access_restricted_groups(groups) }.not_to exceed_all_query_limit(control) + end + end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 114fc34013294c..78a33e88f61408 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -20562,6 +20562,9 @@ msgstr "" msgid "GroupSAML|SHA1 fingerprint of the SAML token signing certificate. Get this from your identity provider, where it can also be called \"Thumbprint\"." msgstr "" +msgid "GroupSAML|Some todos may be hidden because your SAML session has expired. Click to reauthenticate with the following groups to view hidden todos:" +msgstr "" + msgid "GroupSAML|The SCIM token is now hidden. To see the value of the token again, you need to %{linkStart}reset it%{linkEnd}." msgstr "" -- GitLab