Merge branch '214523-set-default-role-for-sso-in-gitlab-com' into 'master'

Resolve "Allow setting a default role for Group SSO"

See merge request gitlab-org/gitlab!37801

doc/user/group/saml_sso/index.md

@@ -63,10 +63,11 @@ Once you've set up your identity provider to work with GitLab, you'll need to co
 1. Navigate to the group's **Settings > SAML SSO**.
 1. Find the SSO URL from your Identity Provider and enter it the **Identity provider single sign-on URL** field.
 1. Find and enter the fingerprint for the SAML token signing certificate in the **Certificate** field.
+1. Select the access level to be applied to newly added users in the **Default membership role** field. The default access level is 'Guest'.
 1. Click the **Enable SAML authentication for this group** toggle switch.
 1. Click the **Save changes** button.
 
-![Group SAML Settings for GitLab.com](img/group_saml_settings.png)
+![Group SAML Settings for GitLab.com](img/group_saml_settings_v13_3.png)
 
 NOTE: **Note:**
 Please note that the certificate [fingerprint algorithm](#additional-providers-and-setup-options) must be in SHA1. When configuring the identity provider, use a secure signature algorithm.
@@ -216,7 +217,9 @@ On subsequent visits, you should be able to go [sign in to GitLab.com with SAML]
 
 ### Role
 
-The first time you sign in, GitLab adds you to the top-level parent group with the Guest role. Existing members with appropriate privileges can promote that new user.
+Starting from [GitLab 13.3](https://gitlab.com/gitlab-org/gitlab/-/issues/214523), group owners can set a 'Default membership role' other than 'Guest'. To do so, [configure the SAML SSO for the group](#configuring-gitlab). That role becomes the starting access level of all users added to the group.
+
+Existing members with appropriate privileges can promote or demote users, as needed.
 
 If a user is already a member of the group, linking the SAML identity does not change their role.
 

doc/user/group/saml_sso/scim_setup.md

@@ -40,7 +40,7 @@ Once [Group Single Sign-On](index.md) has been configured, we can:
 1. Click on the **Generate a SCIM token** button.
 1. Save the token and URL so they can be used in the next step.
 
-![SCIM token configuration](img/scim_token.png)
+![SCIM token configuration](img/scim_token_v13_3.png)
 
 ## Identity Provider configuration
 

ee/app/controllers/groups/saml_providers_controller.rb

@@ -45,7 +45,7 @@ class Groups::SamlProvidersController < Groups::ApplicationController
   end
 
   def saml_provider_params
-    allowed_params = %i[sso_url certificate_fingerprint enabled enforced_sso]
+    allowed_params = %i[sso_url certificate_fingerprint enabled enforced_sso default_membership_role]
 
     if Feature.enabled?(:group_managed_accounts, group)
       allowed_params += [:enforced_group_managed_accounts, :prohibited_outer_forks]

ee/app/models/saml_provider.rb

@@ -9,6 +9,7 @@ class SamlProvider < ApplicationRecord
   validates :group, presence: true, top_level_group: true
   validates :sso_url, presence: true, addressable_url: { schemes: %w(https), ascii_only: true }
   validates :certificate_fingerprint, presence: true, certificate_fingerprint: true
+  validates :default_membership_role, presence: true, inclusion: { in: Gitlab::Access.values }
 
   after_initialize :set_defaults, if: :new_record?
 

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

@@ -50,6 +50,13 @@
       .form-text.text-muted
         = s_('GroupSAML|SHA1 fingerprint of the SAML token signing certificate. Get this from your identity provider, where it can also be called "Thumbprint".')
 
+    .well-segment.borderless.gl-mb-3.col-12.col-lg-9.gl-p-0
+      = f.label :default_membership_role, class: 'label-bold' do
+        = s_('GroupSAML|Default membership role')
+      = f.select :default_membership_role, options_for_select(::Gitlab::Access.options, saml_provider.default_membership_role), {}, class: 'form-control'
+      .form-text.text-muted
+        = s_('GroupSAML|This will be set as the access level of users added to the group.')
+
     .mt-3
       = f.submit _("Save changes"), class: 'btn btn-success', data: { qa_selector: 'save_changes_button' }
       #js-saml-test-button.has-tooltip.float-right

ee/changelogs/unreleased/214523-set-default-role-for-sso-in-gitlab-com.yml

+---
+title: Allow setting a default role for Group SSO
+merge_request: 37801
+author:
+type: added

ee/lib/api/scim.rb

@@ -54,6 +54,7 @@ module API
           @group = find_group(group_path)
 
           scim_not_found!(message: "Group #{group_path} not found") unless @group
+          scim_not_found!(message: "Group #{group_path} does not have SAML SSO configured") unless @group.saml_provider
 
           check_access_to_group!(@group)
 

ee/lib/ee/gitlab/scim/provisioning_service.rb

@@ -8,7 +8,6 @@ module EE
 
         PASSWORD_AUTOMATICALLY_SET = true
         SKIP_EMAIL_CONFIRMATION = false
-        DEFAULT_ACCESS = :guest
 
         def initialize(group, parsed_hash)
           @group = group
@@ -126,7 +125,7 @@ module EE
         def user_params
           @parsed_hash.tap do |hash|
             hash[:skip_confirmation] = SKIP_EMAIL_CONFIRMATION
-            hash[:saml_provider_id] = @group.saml_provider&.id
+            hash[:saml_provider_id] = @group.saml_provider.id
             hash[:group_id] = @group.id
             hash[:provider] = identity_provider
             hash[:email_confirmation] = hash[:email]
@@ -154,10 +153,14 @@ module EE
           strong_memoize(:member) do
             next @group.group_member(user) if existing_member?(user)
 
-            @group.add_user(user, DEFAULT_ACCESS) if user.valid?
+            @group.add_user(user, default_membership_role) if user.valid?
           end
         end
 
+        def default_membership_role
+          @group.saml_provider.default_membership_role
+        end
+
         def create_identity_only?
           scim_identities_enabled? && existing_user? && existing_member?(user)
         end

ee/lib/ee/gitlab/scim/reprovision_service.rb

@@ -8,8 +8,6 @@ module EE
 
         delegate :user, :group, to: :identity
 
-        DEFAULT_ACCESS = :guest
-
         def initialize(identity)
           @identity = identity
         end
@@ -24,7 +22,11 @@ module EE
         private
 
         def add_member
-          group.add_user(user, DEFAULT_ACCESS)
+          group.add_user(user, default_membership_role)
+        end
+
+        def default_membership_role
+          group.saml_provider.default_membership_role
         end
 
         def existing_member?

ee/lib/gitlab/auth/group_saml/membership_updater.rb

@@ -6,7 +6,7 @@ module Gitlab
       class MembershipUpdater
         attr_reader :user, :saml_provider
 
-        delegate :group, to: :saml_provider
+        delegate :group, :default_membership_role, to: :saml_provider
 
         def initialize(user, saml_provider)
           @user = user
@@ -14,22 +14,18 @@ module Gitlab
         end
 
         def execute
-          return if group.member?(@user)
+          return if group.member?(user)
 
-          member = group.add_user(@user, default_membership_level)
+          member = group.add_user(user, default_membership_role)
 
           log_audit_event(member: member)
         end
 
         private
 
-        def default_membership_level
-          :guest
-        end
-
         def log_audit_event(member:)
           ::AuditEventService.new(
-            @user,
+            user,
             member.source,
             action: :create
           ).for_member(member).security_event

ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb

@@ -153,6 +153,19 @@ RSpec.describe Groups::OmniauthCallbacksController do
           expect(group).to be_member(user)
         end
 
+        context 'when a default access level is specified in the SAML provider' do
+          let!(:saml_provider) do
+            create(:saml_provider, group: group, default_membership_role: Gitlab::Access::DEVELOPER)
+          end
+
+          it 'sets the access level of the member as per the specified `default_membership_role`' do
+            post provider, params: { group_id: group }
+
+            created_member = group.members.find_by(user: user)
+            expect(created_member.access_level).to eq(Gitlab::Access::DEVELOPER)
+          end
+        end
+
         it_behaves_like "SAML session initiated"
 
         it "displays a flash indicating the account has been linked" do

ee/spec/controllers/groups/saml_providers_controller_spec.rb

@@ -116,17 +116,27 @@ RSpec.describe Groups::SamlProvidersController do
     end
 
     describe 'PUT #update' do
-      subject { put :update, params: { group_id: group, saml_provider: { enforced_sso: 'true' } } }
+      subject do
+        put :update, params:
+          {
+            group_id: group,
+            saml_provider: {
+              enforced_sso: 'true',
+              default_membership_role: Gitlab::Access::MAINTAINER
+            }
+          }
+      end
 
       before do
         group.add_owner(user)
       end
 
-      it 'updates the setting' do
+      it 'updates the settings' do
         expect do
           subject
           saml_provider.reload
         end.to change { saml_provider.enforced_sso? }.to(true)
+        .and change { saml_provider.default_membership_role }.to(Gitlab::Access::MAINTAINER)
       end
 
       context 'enabling group managed when owner has linked identity' do
@@ -170,7 +180,7 @@ RSpec.describe Groups::SamlProvidersController do
           stub_feature_flags(group_managed_accounts: true)
         end
 
-        it 'does not update update the flags' do
+        it 'does not update the flags' do
           expect do
             subject
             saml_provider.reload

ee/spec/lib/ee/gitlab/scim/provisioning_service_spec.rb

@@ -6,6 +6,7 @@ RSpec.describe ::EE::Gitlab::Scim::ProvisioningService do
   describe '#execute' do
     let(:group) { create(:group) }
     let(:service) { described_class.new(group, service_params) }
+    let!(:saml_provider) { create(:saml_provider, group: group, default_membership_role: Gitlab::Access::DEVELOPER) }
 
     before do
       stub_licensed_features(group_saml: true)
@@ -52,12 +53,18 @@ RSpec.describe ::EE::Gitlab::Scim::ProvisioningService do
           expect(user).to be_a(User)
         end
 
-        it 'creates the member with guest access level' do
-          service.execute
+        context 'access level of created group member' do
+          let!(:saml_provider) do
+            create(:saml_provider, group: group, default_membership_role: Gitlab::Access::DEVELOPER)
+          end
 
-          access_level = group.group_member(user).access_level
+          it 'sets the access level of the member as specified in saml_provider' do
+            service.execute
 
-          expect(access_level).to eq(Gitlab::Access::GUEST)
+            access_level = group.group_member(user).access_level
+
+            expect(access_level).to eq(Gitlab::Access::DEVELOPER)
+          end
         end
 
         it 'user record requires confirmation' do
@@ -116,7 +123,6 @@ RSpec.describe ::EE::Gitlab::Scim::ProvisioningService do
     context 'when scim_identities is disabled' do
       before do
         stub_feature_flags(scim_identities: false)
-        create(:saml_provider, group: group)
       end
 
       it_behaves_like 'scim provisioning'
@@ -156,7 +162,6 @@ RSpec.describe ::EE::Gitlab::Scim::ProvisioningService do
     context 'when scim_identities is enabled' do
       before do
         stub_feature_flags(scim_identities: true)
-        create(:saml_provider, group: group)
       end
 
       it_behaves_like 'scim provisioning'

ee/spec/lib/ee/gitlab/scim/reprovision_service_spec.rb

@@ -7,6 +7,9 @@ RSpec.describe ::EE::Gitlab::Scim::ReprovisionService do
     let_it_be(:identity) { create(:scim_identity, active: false) }
     let_it_be(:group) { identity.group }
     let_it_be(:user) { identity.user }
+    let_it_be(:saml_provider) do
+      create(:saml_provider, group: group, default_membership_role: Gitlab::Access::DEVELOPER)
+    end
 
     let(:service) { described_class.new(identity) }
 
@@ -22,12 +25,12 @@ RSpec.describe ::EE::Gitlab::Scim::ReprovisionService do
       expect(group.members.pluck(:user_id)).to include(user.id)
     end
 
-    it 'creates the member with guest access level' do
+    it 'creates the member with the access level as specified in saml_provider' do
       service.execute
 
       access_level = group.group_member(user).access_level
 
-      expect(access_level).to eq(Gitlab::Access::GUEST)
+      expect(access_level).to eq(Gitlab::Access::DEVELOPER)
     end
 
     it 'does not change group membership when the user is already a member' do

ee/spec/lib/gitlab/auth/group_saml/membership_updater_spec.rb

@@ -4,19 +4,28 @@ require 'spec_helper'
 
 RSpec.describe Gitlab::Auth::GroupSaml::MembershipUpdater do
   let(:user) { create(:user) }
-  let(:saml_provider) { create(:saml_provider) }
+  let(:saml_provider) { create(:saml_provider, default_membership_role: Gitlab::Access::DEVELOPER) }
   let(:group) { saml_provider.group }
 
-  it "adds the user to the group" do
-    described_class.new(user, saml_provider).execute
+  subject { described_class.new(user, saml_provider).execute }
+
+  it 'adds the user to the group' do
+    subject
 
     expect(group.users).to include(user)
   end
 
+  it 'adds the member with the specified `default_membership_role`' do
+    subject
+
+    created_member = group.members.find_by(user: user)
+    expect(created_member.access_level).to eq(Gitlab::Access::DEVELOPER)
+  end
+
   it "doesn't duplicate group membership" do
     group.add_guest(user)
 
-    described_class.new(user, saml_provider).execute
+    subject
 
     expect(group.members.count).to eq 1
   end
@@ -24,16 +33,16 @@ RSpec.describe Gitlab::Auth::GroupSaml::MembershipUpdater do
   it "doesn't overwrite existing membership level" do
     group.add_maintainer(user)
 
-    described_class.new(user, saml_provider).execute
+    subject
 
     expect(group.members.pluck(:access_level)).to eq([Gitlab::Access::MAINTAINER])
   end
 
   it "logs an audit event" do
     expect do
-      described_class.new(user, saml_provider).execute
+      subject
     end.to change { AuditEvent.by_entity('Group', group).count }.by(1)
 
-    expect(AuditEvent.last.details).to include(add: 'user_access', target_details: user.name, as: 'Guest')
+    expect(AuditEvent.last.details).to include(add: 'user_access', target_details: user.name, as: 'Developer')
   end
 end

ee/spec/models/saml_provider_spec.rb

@@ -20,6 +20,8 @@ RSpec.describe SamlProvider do
     it { is_expected.to validate_presence_of(:group) }
     it { is_expected.to validate_presence_of(:sso_url) }
     it { is_expected.to validate_presence_of(:certificate_fingerprint) }
+    it { is_expected.to validate_presence_of(:default_membership_role) }
+    it { is_expected.to validate_inclusion_of(:default_membership_role).in_array([10, 20, 30, 40]) }
 
     it 'expects sso_url to be an https URL' do
       expect(subject).to allow_value('https://example.com').for(:sso_url)

locale/gitlab.pot

@@ -11930,6 +11930,9 @@ msgstr ""
 msgid "GroupSAML|Copy SAML Response XML"
 msgstr ""
 
+msgid "GroupSAML|Default membership role"
+msgstr ""
+
 msgid "GroupSAML|Enable SAML authentication for this group."
 msgstr ""
 
@@ -12002,6 +12005,9 @@ msgstr ""
 msgid "GroupSAML|The SCIM token is now hidden. To see the value of the token again, you need to "
 msgstr ""
 
+msgid "GroupSAML|This will be set as the access level of users added to the group."
+msgstr ""
+
 msgid "GroupSAML|To be able to enable enforced SSO, you first need to enable SAML authentication."
 msgstr ""