Merge branch '439167-restrict-member-roles-ff' into 'master'

Allow creation of group-level roles on self-managed instances

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/142637



Merged-by: Jarka Košanová <jarka@gitlab.com>
Approved-by: Harsha Muralidhar <hmuralidhar@gitlab.com>
Approved-by: Jarka Košanová <jarka@gitlab.com>
Approved-by: Vijay Hawoldar <vhawoldar@gitlab.com>
Reviewed-by: Vijay Hawoldar <vhawoldar@gitlab.com>
Reviewed-by: Hinam Mehra <hmehra@gitlab.com>
Co-authored-by: Hinam Mehra <hmehra@gitlab.com>

ee/app/graphql/mutations/member_roles/create.rb

@@ -49,7 +49,7 @@ def authorize_admin_roles!(group)
       end
 
       def authorize_group_member_roles!(group)
-        raise_resource_not_available_error! unless saas?
+        raise_resource_not_available_error! if restrict_member_roles? && !saas?
         raise_resource_not_available_error! unless Ability.allowed?(current_user, :admin_member_role, group)
         raise_resource_not_available_error! unless group.custom_roles_enabled?
       end
@@ -75,6 +75,10 @@ def canonicalize(args)
           new_args[permission.downcase] = true
         end
       end
+
+      def restrict_member_roles?
+        Feature.enabled?(:restrict_member_roles, type: :beta)
+      end
     end
   end
 end

ee/config/feature_flags/beta/restrict_member_roles.yml

+---
+name: restrict_member_roles
+feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/439167
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/142637
+rollout_issue_url: https://gitlab.com/gitlab-com/gl-infra/production/-/issues/17458
+milestone: '16.9'
+group: group::authorization
+type: beta
+default_enabled: false
\ No newline at end of file

ee/spec/features/admin/member_roles_spec.rb

@@ -11,10 +11,6 @@
   let(:permission_name) { permission.to_s.humanize }
   let(:access_level) { 'Developer' }
 
-  before_all do
-    sign_in(admin)
-  end
-
   before do
     stub_licensed_features(custom_roles: true)
   end
@@ -37,21 +33,45 @@ def created_role(name, id, access_level, permissions)
     before do
       allow(Gitlab::CustomRoles::Definition).to receive(:all).and_return(permissions)
 
-      visit admin_application_settings_roles_and_permissions_path
+      gitlab_sign_in(admin)
+    end
+
+    shared_examples 'creates a new custom role' do
+      it 'and displays it' do
+        create_role(access_level, name, [permission_name])
+
+        created_member_role = MemberRole.find_by(
+          name: name,
+          base_access_level: Gitlab::Access.options[access_level],
+          permission => true)
+
+        expect(created_member_role).not_to be_nil
+
+        role = created_role(name, created_member_role.id, access_level, [permission_name])
+        expect(page).to have_content(role)
+      end
     end
 
-    it 'creates a new custom role' do
-      create_role(access_level, name, [permission_name])
+    context 'when on self-managed' do
+      before do
+        stub_saas_features(group_custom_roles: false)
+
+        visit admin_application_settings_roles_and_permissions_path
+      end
+
+      it_behaves_like 'creates a new custom role'
+    end
 
-      created_member_role = MemberRole.find_by(
-        name: name,
-        base_access_level: Gitlab::Access.options[access_level],
-        permission => true)
+    context 'when on SaaS', :saas do
+      before do
+        visit admin_application_settings_roles_and_permissions_path
+      end
 
-      expect(created_member_role).not_to be_nil
+      it 'shows an error message' do
+        create_role(access_level, name, [permission_name])
 
-      role = created_role(name, created_member_role.id, access_level, [permission_name])
-      expect(page).to have_content(role)
+        expect(page).to have_content('Failed to create role')
+      end
     end
   end
 end

ee/spec/features/groups/member_roles_spec.rb

@@ -39,46 +39,59 @@ def created_role(name, id, access_level, permissions)
       allow(Gitlab::CustomRoles::Definition).to receive(:all).and_return(permissions)
 
       sign_in(user)
-      visit group_settings_roles_and_permissions_path(group)
     end
 
-    it 'creates a new custom role' do
-      create_role(access_level, name, [permission_name])
+    shared_examples 'creates a new custom role' do
+      it 'and displays it' do
+        create_role(access_level, name, [permission_name])
 
-      created_member_role = MemberRole.find_by(
-        name: name,
-        base_access_level: Gitlab::Access.options[access_level],
-        permission => true)
+        created_member_role = MemberRole.find_by(
+          name: name,
+          base_access_level: Gitlab::Access.options[access_level],
+          permission => true)
 
-      expect(created_member_role).not_to be_nil
+        expect(created_member_role).not_to be_nil
 
-      role = created_role(name, created_member_role.id, access_level, [permission_name])
-      expect(page).to have_content(role)
+        role = created_role(name, created_member_role.id, access_level, [permission_name])
+        expect(page).to have_content(role)
+      end
     end
 
-    context 'when the permission has a requirement' do
-      let(:permissions) do
-        { admin_vulnerability: { name: 'admin_vulnerability', requirements: ['read_vulnerability'] },
-          read_vulnerability: { name: 'read_vulnerability' } }
+    context 'when on SaaS' do
+      before do
+        visit group_settings_roles_and_permissions_path(group)
       end
 
-      let(:permission) { :admin_vulnerability }
-      let(:requirement) { permissions[permission][:requirements].first }
-      let(:requirement_name) { requirement.to_s.humanize }
+      it_behaves_like 'creates a new custom role'
+    end
 
-      it 'creates the custom role' do
-        create_role(access_level, name, [permission_name])
+    context 'when on self-managed' do
+      before do
+        stub_saas_features(group_custom_roles: false)
+      end
 
-        created_member_role = MemberRole.find_by(
-          name: name,
-          base_access_level: Gitlab::Access.options[access_level],
-          permission => true,
-          requirement => true)
+      context 'when restrict_member_roles feature-flag is disabled' do
+        before do
+          stub_feature_flags(restrict_member_roles: false)
 
-        expect(created_member_role).not_to be_nil
+          visit group_settings_roles_and_permissions_path(group)
+        end
 
-        role = created_role(name, created_member_role.id, access_level, [permission_name, requirement_name])
-        expect(page).to have_content(role)
+        it_behaves_like 'creates a new custom role'
+      end
+
+      context 'when restrict_member_roles feature-flag is enabled' do
+        before do
+          stub_feature_flags(restrict_member_roles: true)
+
+          visit group_settings_roles_and_permissions_path(group)
+        end
+
+        it 'shows an error message' do
+          create_role(access_level, name, [permission_name])
+
+          expect(page).to have_content('Failed to create role')
+        end
       end
     end
   end

ee/spec/finders/member_roles/roles_finder_spec.rb

@@ -174,7 +174,7 @@
           let(:current_user) { admin }
 
           context 'when on self-managed' do
-            it 'returns instance member roles for instance admin' do
+            it 'returns instance member roles' do
               expect(find_member_roles).to eq([member_role_instance])
             end
           end

ee/spec/requests/api/graphql/mutations/member_role/create_member_role_spec.rb

@@ -38,6 +38,28 @@
 
   subject(:create_member_role) { graphql_mutation_response(:member_role_create) }
 
+  shared_examples 'a mutation that creates a member role' do
+    it 'returns success', :aggregate_failures do
+      post_graphql_mutation(mutation, current_user: current_user)
+
+      expect(graphql_errors).to be_nil
+
+      expect(create_member_role['memberRole']['enabledPermissions']['nodes'].flat_map(&:values))
+        .to match_array(permissions)
+    end
+
+    it 'creates the member role', :aggregate_failures do
+      expect { post_graphql_mutation(mutation, current_user: current_user) }
+        .to change { MemberRole.count }.by(1)
+
+      member_role = MemberRole.last
+
+      expect(member_role.read_vulnerability).to eq(true)
+
+      expect(member_role.namespace).to eq(group)
+    end
+  end
+
   context 'without the custom roles feature' do
     before do
       stub_licensed_features(custom_roles: false)
@@ -74,49 +96,32 @@
         end
 
         context 'when on self-managed' do
-          it_behaves_like 'a mutation that returns a top-level access error'
-        end
+          context 'when restrict_member_roles feature-flag is disabled' do
+            before do
+              stub_feature_flags(restrict_member_roles: false)
+            end
 
-        context 'when on Saas', :saas do
-          context 'with valid arguments' do
-            it 'returns success' do
-              post_graphql_mutation(mutation, current_user: current_user)
+            it_behaves_like 'a mutation that creates a member role'
+          end
 
-              expect(graphql_errors).to be_nil
-              expect(create_member_role['memberRole']['enabledPermissions']['nodes'].flat_map(&:values))
-                .to match_array(MemberRole.all_customizable_permissions.keys.map(&:to_s).map(&:upcase))
+          context 'when restrict_member_roles feature-flag is enabled' do
+            before do
+              stub_feature_flags(restrict_member_roles: true)
             end
 
-            it 'creates the member role' do
-              expect { post_graphql_mutation(mutation, current_user: current_user) }
-                .to change { MemberRole.count }.by(1)
-
-              member_role = MemberRole.last
+            it_behaves_like 'a mutation that returns a top-level access error'
+          end
+        end
 
-              expect(member_role.read_vulnerability).to eq(true)
-              expect(member_role.namespace).to eq(group)
-            end
+        context 'when on SaaS', :saas do
+          context 'with valid arguments' do
+            it_behaves_like 'a mutation that creates a member role'
           end
 
           context 'with an array of permissions' do
             let(:permissions) { ['READ_VULNERABILITY'] }
 
-            it 'returns success' do
-              post_graphql_mutation(mutation, current_user: current_user)
-
-              expect(graphql_errors).to be_nil
-              mutation_response = create_member_role['memberRole']
-              expect(mutation_response['enabledPermissions']['nodes'].flat_map(&:values)).to eq(['READ_VULNERABILITY'])
-            end
-
-            it 'creates a member role with the specified permissions' do
-              expect do
-                post_graphql_mutation(mutation, current_user: current_user)
-              end.to change { MemberRole.count }.by(1)
-
-              member_role = MemberRole.last
-              expect(member_role.read_vulnerability).to eq(true)
-            end
+            it_behaves_like 'a mutation that creates a member role'
           end
 
           context 'with an unknown permission' do
@@ -139,8 +144,11 @@
     end
 
     context 'when creating an instance level member role' do
-      before do
-        input.delete(:group_path)
+      let(:input) do
+        {
+          base_access_level: 'GUEST',
+          permissions: permissions
+        }
       end
 
       context 'with unauthorized user' do
@@ -152,30 +160,35 @@
           current_user.update!(admin: true)
         end
 
-        context 'when on SaaS', :saas do
-          it_behaves_like 'a mutation that returns top-level errors', errors: ['group_path argument is required.']
-        end
-
-        context 'when running on self-managed' do
-          it 'returns success' do
+        context 'when on self-managed' do
+          it 'returns success', :aggregate_failures do
             post_graphql_mutation(mutation, current_user: current_user)
 
             expect(graphql_errors).to be_nil
+
             expect(create_member_role['memberRole']['enabledPermissions']['nodes'].flat_map(&:values))
-              .to include('READ_VULNERABILITY')
-            expect(create_member_role['memberRole']['namespace']).to be_nil
+              .to match_array(permissions)
           end
 
-          it 'creates the member role' do
+          it 'creates the member role', :aggregate_failures do
             expect { post_graphql_mutation(mutation, current_user: current_user) }
               .to change { MemberRole.count }.by(1)
 
             member_role = MemberRole.last
 
             expect(member_role.read_vulnerability).to eq(true)
+
             expect(member_role.namespace).to be_nil
           end
         end
+
+        context 'when on SaaS', :saas do
+          before do
+            stub_feature_flags(restrict_member_roles: false)
+          end
+
+          it_behaves_like 'a mutation that returns top-level errors', errors: ['group_path argument is required.']
+        end
       end
     end
   end