Merge branch 'dblessing_saml_group_links_controller' into 'master'

SAML group link controller

See merge request gitlab-org/gitlab!45080

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;

config/feature_flags/development/saml_group_links.yml

+---
+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

ee/app/controllers/groups/saml_group_links_controller.rb

+# 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

ee/app/helpers/ee/saml_providers_helper.rb

@@ -7,7 +7,7 @@ module EE
     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)

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

ee/app/views/groups/saml_group_links/_form.html.haml

+%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'

ee/app/views/groups/saml_group_links/_saml_group_link.html.haml

+%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 }
+

ee/app/views/groups/saml_group_links/_saml_group_links.html.haml

+- 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')

ee/app/views/groups/saml_group_links/index.html.haml

+- 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

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'

ee/config/routes/group.rb

@@ -69,6 +69,7 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
     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]
 

ee/spec/controllers/groups/saml_group_links_controller_spec.rb

+# 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

ee/spec/factories/saml_group_links.rb

 # 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

ee/spec/features/groups/saml_group_links_spec.rb

+# 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

ee/spec/helpers/ee/saml_providers_helper_spec.rb

@@ -37,20 +37,35 @@ RSpec.describe EE::SamlProvidersHelper do
   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

locale/gitlab.pot

@@ -12912,6 +12912,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 ""
 
@@ -12921,6 +12927,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 ""
 
@@ -12966,12 +12975,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 ""
 
@@ -12984,6 +13011,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 ""
 
@@ -12996,6 +13026,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 ""
 
@@ -13020,6 +13053,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 ""