diff --git a/app/assets/stylesheets/pages/groups.scss b/app/assets/stylesheets/pages/groups.scss index ee4f74882a1f8799696fc21847ac7035b927838b..1647bf9c7ce74c6cf84c3ae62870a7712b6a10ed 100644 --- a/app/assets/stylesheets/pages/groups.scss +++ b/app/assets/stylesheets/pages/groups.scss @@ -43,12 +43,6 @@ } } -.ldap-group-links { - .form-actions { - margin-bottom: $gl-padding; - } -} - .save-group-loader { margin-top: $gl-padding-50; margin-bottom: $gl-padding-50; diff --git a/config/feature_flags/development/saml_group_links.yml b/config/feature_flags/development/saml_group_links.yml new file mode 100644 index 0000000000000000000000000000000000000000..84c3c73882f44b1c284887406f7990ef175dd56b --- /dev/null +++ b/config/feature_flags/development/saml_group_links.yml @@ -0,0 +1,7 @@ +--- +name: saml_group_links +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/45080 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/267020 +type: development +group: group::access +default_enabled: false diff --git a/ee/app/controllers/groups/saml_group_links_controller.rb b/ee/app/controllers/groups/saml_group_links_controller.rb new file mode 100644 index 0000000000000000000000000000000000000000..672f9d2789f4037b9a4b4532453a1a44f4265812 --- /dev/null +++ b/ee/app/controllers/groups/saml_group_links_controller.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +module Groups + class SamlGroupLinksController < Groups::ApplicationController + before_action :require_saml_group_links_enabled + before_action :authorize_admin_saml_group_links! + + layout 'group_settings' + + feature_category :authentication_and_authorization + + def create + group_link = group.saml_group_links.build(saml_group_link_params) + + if group_link.save + flash[:notice] = s_('GroupSAML|New SAML group link saved.') + else + flash[:alert] = alert(group_link.errors.full_messages.join(', ')) + end + + redirect_to group_saml_group_links_path(@group) + end + + def destroy + group.saml_group_links.find(params[:id]).destroy + + redirect_to group_saml_group_links_path(@group), status: :found, notice: s_('GroupSAML|SAML group link was successfully removed.') + end + + private + + def require_saml_group_links_enabled + render_404 unless ::Feature.enabled?(:saml_group_links, group) + end + + def authorize_admin_saml_group_links! + access_denied! unless can?(current_user, :admin_saml_group_links, group) + end + + def saml_group_link_params + params.require(:saml_group_link).permit(:saml_group_name, :access_level) + end + + def alert(error_message) + s_('GroupSAML|Could not create SAML group link: %{errors}.') % { errors: error_message } + end + end +end diff --git a/ee/app/helpers/ee/saml_providers_helper.rb b/ee/app/helpers/ee/saml_providers_helper.rb index 68e101b1a42712d8a88ab50405eacf2e2b1a40be..35fa75077078cd5581cdbd69f80c1f3045f223cc 100644 --- a/ee/app/helpers/ee/saml_providers_helper.rb +++ b/ee/app/helpers/ee/saml_providers_helper.rb @@ -7,7 +7,7 @@ def show_saml_in_sidebar?(group) end def show_saml_group_links_in_sidebar?(group) - can?(current_user, :admin_saml_group_links, group) + ::Feature.enabled?(:saml_group_links, group) && can?(current_user, :admin_saml_group_links, group) end def saml_link_for_provider(text, provider, **args) diff --git a/ee/app/views/groups/ee/_settings_nav.html.haml b/ee/app/views/groups/ee/_settings_nav.html.haml index 4fa138189f5c2806b715a89771f31dc2f8d58a99..e6f772ccb207ec7f6778805b3d874b290fb1cc8d 100644 --- a/ee/app/views/groups/ee/_settings_nav.html.haml +++ b/ee/app/views/groups/ee/_settings_nav.html.haml @@ -12,6 +12,12 @@ %span = _('SAML SSO') +- if show_saml_group_links_in_sidebar?(@group) + = nav_link(path: 'saml_group_links#index') do + = link_to group_saml_group_links_path(@group), title: s_('GroupSAML|SAML Group Links') do + %span + = s_('GroupSAML|SAML Group Links') + - if @group.feature_available?(:group_webhooks) || show_promotions? = nav_link(path: 'hooks#index') do = link_to group_hooks_path(@group), title: 'Webhooks' do diff --git a/ee/app/views/groups/saml_group_links/_form.html.haml b/ee/app/views/groups/saml_group_links/_form.html.haml new file mode 100644 index 0000000000000000000000000000000000000000..c1302affd9388028779b78fa5eb48cbb63d609c5 --- /dev/null +++ b/ee/app/views/groups/saml_group_links/_form.html.haml @@ -0,0 +1,21 @@ +%section.saml-group-links + = form_for [group, SamlGroupLink.new] do |f| + .form-holder + .form-group.row + .col-sm-2.col-form-label + = f.label :saml_group_name, s_('GroupSAML|SAML Group Name') + .col-sm-10 + = f.text_field :saml_group_name, class: 'form-control xxlarge input-mn-300' + .form-text.text-muted + = s_('GroupSAML|The case-sensitive group name that will be sent by the SAML identity provider.') + + .form-group.row + .col-sm-2.col-form-label + = f.label :access_level, "Access Level" + .col-sm-10 + = f.select :access_level, options_for_select(SamlGroupLink.access_levels.keys), {}, class: 'form-control' + .form-text.text-muted + = s_('GroupSAML|Role to assign members of this SAML group.') + + .form-actions.gl-mb-5 + = f.submit _('Save'), class: 'btn gl-button btn-success' diff --git a/ee/app/views/groups/saml_group_links/_saml_group_link.html.haml b/ee/app/views/groups/saml_group_links/_saml_group_link.html.haml new file mode 100644 index 0000000000000000000000000000000000000000..aec5338f52da541e217804a1bd50a50d7a9dbd0b --- /dev/null +++ b/ee/app/views/groups/saml_group_links/_saml_group_link.html.haml @@ -0,0 +1,10 @@ +%li + .float-right + = link_to group_saml_group_link_path(group, saml_group_link), method: :delete, class: 'btn gl-button btn-danger btn-sm', data: { confirm: s_('GroupSAML|Are you sure you want to remove the SAML group link?') } do + = sprite_icon('unlink', size: 12, css_class: 'gl-m-0!') + %span= _('Remove') + %strong= s_('GroupSAML|SAML Group Name: %{saml_group_name}') % { saml_group_name: saml_group_link.saml_group_name } + + .light + = s_('GroupSAML|as %{access_level}') % { access_level: saml_group_link.access_level } + diff --git a/ee/app/views/groups/saml_group_links/_saml_group_links.html.haml b/ee/app/views/groups/saml_group_links/_saml_group_links.html.haml new file mode 100644 index 0000000000000000000000000000000000000000..07f3c2d2269f5d305c10d28fad253f2fd8e7c79e --- /dev/null +++ b/ee/app/views/groups/saml_group_links/_saml_group_links.html.haml @@ -0,0 +1,11 @@ +- group_links = group.saml_group_links.load + +.card + .card-header= s_('GroupSAML|Active SAML Group Links (%{count})') % { count: group_links.size } + + - if group_links.any? + %ul.content-list + = render collection: group_links, partial: 'saml_group_link', locals: { group: group } + - else + .card-body + = s_('GroupSAML|No active SAML group links') diff --git a/ee/app/views/groups/saml_group_links/index.html.haml b/ee/app/views/groups/saml_group_links/index.html.haml new file mode 100644 index 0000000000000000000000000000000000000000..fba7400289fae7b1739e403aaf258cbe23db2ef5 --- /dev/null +++ b/ee/app/views/groups/saml_group_links/index.html.haml @@ -0,0 +1,5 @@ +- page_title s_('GroupSAML|SAML Group Links') +%h3.page-title= s_('GroupSAML|SAML Group Links') + += render 'form', group: @group += render 'saml_group_links', group: @group diff --git a/ee/app/views/ldap_group_links/_form.html.haml b/ee/app/views/ldap_group_links/_form.html.haml index 3db219776f8aceacefc121190123ae76360ee6da..65edef542e4dd15be6c2332ee007aa86f76390ea 100644 --- a/ee/app/views/ldap_group_links/_form.html.haml +++ b/ee/app/views/ldap_group_links/_form.html.haml @@ -53,5 +53,5 @@ %br You can manage permission levels for individual group members in the Members tab. - .form-actions + .form-actions.gl-mb-5 = f.submit 'Add synchronization', class: 'btn btn-success qa-add-sync-button' diff --git a/ee/config/routes/group.rb b/ee/config/routes/group.rb index e8a32fda039bbb08e7bac4e6fc05b8908e9bbcab..224f17218fc2bcd27c5bd52e76a21d3aa1f2c6ad 100644 --- a/ee/config/routes/group.rb +++ b/ee/config/routes/group.rb @@ -69,6 +69,7 @@ resource :notification_setting, only: [:update] resources :ldap_group_links, only: [:index, :create, :destroy] + resources :saml_group_links, only: [:index, :create, :destroy] resources :audit_events, only: [:index] resources :usage_quotas, only: [:index] diff --git a/ee/spec/controllers/groups/saml_group_links_controller_spec.rb b/ee/spec/controllers/groups/saml_group_links_controller_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..52b9d1d8597ef0ec991e3f489d8aab625d1fefc6 --- /dev/null +++ b/ee/spec/controllers/groups/saml_group_links_controller_spec.rb @@ -0,0 +1,136 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Groups::SamlGroupLinksController do + let_it_be(:group) { create(:group) } + let_it_be(:user) { create(:user) } + + before_all do + group.add_owner(user) + end + + before do + stub_licensed_features(group_saml: true) + stub_feature_flags(saml_group_links: true) + + sign_in(user) + end + + shared_examples 'checks authorization' do + let_it_be(:saml_provider) { create(:saml_provider, group: group, enabled: true) } + let_it_be(:params) { route_params } + + it 'renders 404 when the feature is disabled' do + stub_feature_flags(saml_group_links: false) + + call_action + + expect(response).to have_gitlab_http_status(:not_found) + end + + it 'renders 404 when the user is not authorized' do + allow(controller).to receive(:can?).and_call_original + allow(controller).to receive(:can?).with(user, :admin_saml_group_links, group).and_return(false) + + call_action + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + describe '#index' do + let_it_be(:route_params) { { group_id: group } } + + subject(:call_action) { get :index, params: params } + + it_behaves_like 'checks authorization' + + context 'when the SAML provider is enabled' do + let_it_be(:saml_provider) { create(:saml_provider, group: group, enabled: true) } + let_it_be(:params) { route_params } + + it 'responds with 200' do + call_action + + expect(response).to have_gitlab_http_status(:ok) + end + end + end + + describe '#create' do + let_it_be(:route_params) { { group_id: group } } + + subject(:call_action) { post :create, params: params } + + it_behaves_like 'checks authorization' + + context 'when the SAML provider is enabled' do + let_it_be(:saml_provider) { create(:saml_provider, group: group, enabled: true) } + + context 'with valid parameters' do + let_it_be(:params) { route_params.merge(saml_group_link: { access_level: 'Reporter', saml_group_name: generate(:saml_group_name) }) } + + it 'responds with success' do + call_action + + expect(response).to have_gitlab_http_status(:found) + expect(flash[:notice]).to include('New SAML group link saved.') + end + + it 'creates the group link' do + expect { call_action }.to change { group.saml_group_links.count }.by(1) + end + end + + context 'with missing parameters' do + let_it_be(:params) { route_params.merge(saml_group_link: { access_level: 'Maintainer' }) } + + it 'displays an error' do + call_action + + expect(response).to have_gitlab_http_status(:found) + expect(flash[:alert]).to include("Could not create SAML group link: Saml group name can't be blank.") + end + end + end + end + + describe '#destroy' do + let_it_be(:group_link) { create(:saml_group_link, group: group) } + let_it_be(:route_params) { { group_id: group, id: group_link } } + + subject(:call_action) { delete :destroy, params: params } + + it_behaves_like 'checks authorization' + + context 'when the SAML provider is enabled' do + let_it_be(:saml_provider) { create(:saml_provider, group: group, enabled: true) } + + context 'with an existent group link' do + let_it_be(:params) { route_params } + + it 'responds with success' do + call_action + + expect(response).to have_gitlab_http_status(:found) + expect(flash[:notice]).to include('SAML group link was successfully removed.') + end + + it 'removes the group link' do + expect { call_action }.to change { group.saml_group_links.count }.by(-1) + end + end + + context 'with a non-existent group link' do + let_it_be(:params) { { group_id: group, id: non_existing_record_id } } + + it 'renders 404' do + call_action + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end + end +end diff --git a/ee/spec/factories/saml_group_links.rb b/ee/spec/factories/saml_group_links.rb index b65ee68818b9c1f11b9022405dce2aba40c9c7f5..05d7c5f01b4f33f512ea165b8c5d8a3f967735c6 100644 --- a/ee/spec/factories/saml_group_links.rb +++ b/ee/spec/factories/saml_group_links.rb @@ -1,8 +1,10 @@ # frozen_string_literal: true FactoryBot.define do + sequence(:saml_group_name) { |n| "saml-group#{n}" } + factory :saml_group_link do - sequence(:saml_group_name) { |n| "saml-group#{n}" } + saml_group_name { generate(:saml_group_name) } access_level { Gitlab::Access::GUEST } group end diff --git a/ee/spec/features/groups/saml_group_links_spec.rb b/ee/spec/features/groups/saml_group_links_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..d54c0e5c43b89643f3ca2765a4e5f439d59f4cfb --- /dev/null +++ b/ee/spec/features/groups/saml_group_links_spec.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'SAML group links' do + let_it_be(:user) { create(:user) } + let_it_be(:group) { create(:group) } + + before do + group.add_owner(user) + sign_in(user) + end + + context 'when SAML group links is available' do + before do + stub_licensed_features(group_saml: true) + stub_feature_flags(saml_group_links: true) + + create(:saml_provider, group: group, enabled: true) + + visit group_saml_group_links_path(group) + end + + context 'with existing records' do + let_it_be(:group_link1) { create(:saml_group_link, group: group, saml_group_name: 'Web Developers') } + let_it_be(:group_link2) { create(:saml_group_link, group: group, saml_group_name: 'Web Managers') } + let_it_be(:other_group_link) { create(:saml_group_link, group: create(:group), saml_group_name: 'Other Group') } + + it 'lists active links' do + expect(page).to have_content('SAML Group Name: Web Developers') + expect(page).to have_content('SAML Group Name: Web Managers') + end + + it 'does not list links for other groups' do + expect(page).not_to have_content('SAML Group Name: Other Group') + end + end + + it 'adds new SAML group link' do + page.within('form#new_saml_group_link') do + fill_in 'SAML Group Name', with: 'Acme SAML Group' + select 'Developer', from: 'saml_group_link_access_level' + + click_button 'Save' + end + + expect(page).not_to have_content('No active SAML group links') + expect(page).to have_content('SAML Group Name: Acme SAML Group') + expect(page).to have_content('as Developer') + end + end +end diff --git a/ee/spec/helpers/ee/saml_providers_helper_spec.rb b/ee/spec/helpers/ee/saml_providers_helper_spec.rb index 5328a2925ccfe3eb2921ab51af466983d463276c..2b7b3ad89322ab35e1823ae3d3b211398bfa2a8f 100644 --- a/ee/spec/helpers/ee/saml_providers_helper_spec.rb +++ b/ee/spec/helpers/ee/saml_providers_helper_spec.rb @@ -37,20 +37,35 @@ def stub_can(permission, value) describe '#show_saml_group_links_in_sidebar?' do subject { helper.show_saml_group_links_in_sidebar?(group) } - context 'when the user can admin saml group links' do + context 'when the feature is disabled' do before do + stub_feature_flags(saml_group_links: false) stub_can(:admin_saml_group_links, true) end - it { is_expected.to eq(true) } + it { is_expected.to eq(false) } end - context 'when the user cannot admin saml group links' do + context 'when the feature is enabled' do before do - stub_can(:admin_saml_group_links, false) + stub_feature_flags(saml_group_links: true) end - it { is_expected.to eq(false) } + context 'when the user can admin saml group links' do + before do + stub_can(:admin_saml_group_links, true) + end + + it { is_expected.to eq(true) } + end + + context 'when the user cannot admin saml group links' do + before do + stub_can(:admin_saml_group_links, false) + end + + it { is_expected.to eq(false) } + end end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 0e61fbeb119b12c7d0d0b918be362c90db7ab50b..2cfaff3e16e3b309318ec0d54309c41833f34ff5 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -12909,6 +12909,12 @@ msgstr "" msgid "GroupRoadmap|To widen your search, change or remove filters; from %{startDate} to %{endDate}." msgstr "" +msgid "GroupSAML|Active SAML Group Links (%{count})" +msgstr "" + +msgid "GroupSAML|Are you sure you want to remove the SAML group link?" +msgstr "" + msgid "GroupSAML|Certificate fingerprint" msgstr "" @@ -12918,6 +12924,9 @@ msgstr "" msgid "GroupSAML|Copy SAML Response XML" msgstr "" +msgid "GroupSAML|Could not create SAML group link: %{errors}." +msgstr "" + msgid "GroupSAML|Default membership role" msgstr "" @@ -12963,12 +12972,30 @@ msgstr "" msgid "GroupSAML|NameID Format" msgstr "" +msgid "GroupSAML|New SAML group link saved." +msgstr "" + +msgid "GroupSAML|No active SAML group links" +msgstr "" + msgid "GroupSAML|Prohibit outer forks" msgstr "" msgid "GroupSAML|Prohibit outer forks for this group." msgstr "" +msgid "GroupSAML|Role to assign members of this SAML group." +msgstr "" + +msgid "GroupSAML|SAML Group Links" +msgstr "" + +msgid "GroupSAML|SAML Group Name" +msgstr "" + +msgid "GroupSAML|SAML Group Name: %{saml_group_name}" +msgstr "" + msgid "GroupSAML|SAML Response Output" msgstr "" @@ -12981,6 +13008,9 @@ msgstr "" msgid "GroupSAML|SAML Single Sign On Settings" msgstr "" +msgid "GroupSAML|SAML group link was successfully removed." +msgstr "" + msgid "GroupSAML|SCIM API endpoint URL" msgstr "" @@ -12993,6 +13023,9 @@ msgstr "" msgid "GroupSAML|The SCIM token is now hidden. To see the value of the token again, you need to " msgstr "" +msgid "GroupSAML|The case-sensitive group name that will be sent by the SAML identity provider." +msgstr "" + msgid "GroupSAML|This will be set as the access level of users added to the group." msgstr "" @@ -13017,6 +13050,9 @@ msgstr "" msgid "GroupSAML|Your SCIM token" msgstr "" +msgid "GroupSAML|as %{access_level}" +msgstr "" + msgid "GroupSAML|must match stored NameID of \"%{extern_uid}\" as we use this to identify users. If the NameID changes users will be unable to sign in." msgstr ""