Merge branch 'if-10137-ee_specific_lines_groups_api' into 'master'

Move EE specific lines in API::Groups

See merge request gitlab-org/gitlab-ee!12240

ee/lib/ee/api/helpers/groups_helpers.rb

+# frozen_string_literal: true
+
+module EE
+  module API
+    module Helpers
+      module GroupsHelpers
+        extend ActiveSupport::Concern
+
+        prepended do
+          included do
+            helpers do
+              params :optional_params_ee do
+                optional :membership_lock, type: ::Grape::API::Boolean, desc: 'Prevent adding new members to project membership within this group'
+                optional :ldap_cn, type: String, desc: 'LDAP Common Name'
+                optional :ldap_access, type: Integer, desc: 'A valid access level'
+                optional :shared_runners_minutes_limit, type: Integer, desc: '(admin-only) Pipeline minutes quota for this group'
+                optional :extra_shared_runners_minutes_limit, type: Integer, desc: '(admin-only) Extra pipeline minutes quota for this group'
+                all_or_none_of :ldap_cn, :ldap_access
+              end
+
+              params :optional_update_params_ee do
+                optional :file_template_project_id, type: Integer, desc: 'The ID of a project to use for custom templates in this group'
+              end
+            end
+          end
+        end
+      end
+    end
+  end
+end

ee/spec/requests/api/groups_spec.rb

@@ -2,20 +2,53 @@ require 'spec_helper'
 
 describe API::Groups do
   set(:group) { create(:group) }
+  set(:private_group) { create(:group, :private) }
   set(:project) { create(:project, group: group) }
   set(:user) { create(:user) }
+  set(:another_user) { create(:user) }
+  let(:admin) { create(:admin) }
 
-  describe 'PUT /groups/:id' do
   before do
     group.add_owner(user)
+    group.ldap_group_links.create cn: 'ldap-group', group_access: Gitlab::Access::MAINTAINER, provider: 'ldap'
+  end
+
+  describe "GET /groups" do
+    context "when authenticated as user" do
+      it "returns ldap details" do
+        get api("/groups", user)
+
+        expect(json_response).to(
+          satisfy_one { |group_json| group_json['ldap_cn'] == group.ldap_cn })
+        expect(json_response).to(
+          satisfy_one do |group_json|
+            group_json['ldap_access'] == group.ldap_access
+          end
+        )
+
+        expect(json_response).to(
+          satisfy_one do |group_json|
+            ldap_group_link = group_json['ldap_group_links'].first
+
+            ldap_group_link['cn'] == group.ldap_cn &&
+              ldap_group_link['group_access'] == group.ldap_access &&
+              ldap_group_link['provider'] == 'ldap'
+          end
+        )
+      end
     end
+  end
+
+  describe 'PUT /groups/:id' do
+    subject { put api("/groups/#{group.id}", user), params: params }
 
-    subject(:do_it) { put api("/groups/#{group.id}", user), params: { file_template_project_id: project.id } }
+    context 'file_template_project_id' do
+      let(:params) { { file_template_project_id: project.id } }
 
       it 'does not update file_template_project_id if unlicensed' do
         stub_licensed_features(custom_file_templates_for_namespace: false)
 
-      expect { do_it }.not_to change { group.reload.file_template_project_id }
+        expect { subject }.not_to change { group.reload.file_template_project_id }
         expect(response).to have_gitlab_http_status(200)
         expect(json_response).not_to have_key('file_template_project_id')
       end
@@ -23,9 +56,173 @@ describe API::Groups do
       it 'updates file_template_project_id if licensed' do
         stub_licensed_features(custom_file_templates_for_namespace: true)
 
-      expect { do_it }.to change { group.reload.file_template_project_id }.to(project.id)
+        expect { subject }.to change { group.reload.file_template_project_id }.to(project.id)
         expect(response).to have_gitlab_http_status(200)
         expect(json_response['file_template_project_id']).to eq(project.id)
       end
     end
+
+    context 'shared_runners_minutes_limit' do
+      let(:params) { { shared_runners_minutes_limit: 133 } }
+
+      context 'when authenticated as the group owner' do
+        it 'returns 200 if shared_runners_minutes_limit is not changing' do
+          group.update(shared_runners_minutes_limit: 133)
+
+          expect do
+            put api("/groups/#{group.id}", user), params: { shared_runners_minutes_limit: 133 }
+          end.not_to change { group.shared_runners_minutes_limit }
+
+          expect(response).to have_gitlab_http_status(200)
+        end
+      end
+
+      context 'when authenticated as the admin' do
+        let(:user) { create(:admin) }
+
+        it 'updates the group for shared_runners_minutes_limit' do
+          expect { subject }.to(
+            change { group.reload.shared_runners_minutes_limit }.from(nil).to(133))
+
+          expect(response).to have_gitlab_http_status(200)
+          expect(json_response['shared_runners_minutes_limit']).to eq(133)
+        end
+      end
+    end
+  end
+
+  describe "POST /groups" do
+    context "when authenticated as user with group permissions" do
+      it "creates an ldap_group_link if ldap_cn and ldap_access are supplied" do
+        group_attributes = attributes_for(:group, ldap_cn: 'ldap-group', ldap_access: Gitlab::Access::DEVELOPER)
+        expect { post api("/groups", admin), params: group_attributes }.to change { LdapGroupLink.count }.by(1)
+      end
+
+      context 'when shared_runners_minutes_limit is given' do
+        context 'when the current user is not an admin' do
+          it "does not create a group with shared_runners_minutes_limit" do
+            group = attributes_for(:group, { shared_runners_minutes_limit: 133 })
+
+            expect do
+              post api("/groups", another_user), params: group
+            end.not_to change { Group.count }
+
+            expect(response).to have_gitlab_http_status(403)
+          end
+        end
+
+        context 'when the current user is an admin' do
+          it "creates a group with shared_runners_minutes_limit" do
+            group = attributes_for(:group, { shared_runners_minutes_limit: 133 })
+
+            expect do
+              post api("/groups", admin), params: group
+            end.to change { Group.count }.by(1)
+
+            created_group = Group.find(json_response['id'])
+
+            expect(created_group.shared_runners_minutes_limit).to eq(133)
+            expect(response).to have_gitlab_http_status(201)
+            expect(json_response['shared_runners_minutes_limit']).to eq(133)
+          end
+        end
+      end
+    end
+  end
+
+  describe 'POST /groups/:id/ldap_sync' do
+    before do
+      allow(Gitlab::Auth::LDAP::Config).to receive(:enabled?).and_return(true)
+    end
+
+    context 'when the ldap_group_sync feature is available' do
+      before do
+        stub_licensed_features(ldap_group_sync: true)
+      end
+
+      context 'when authenticated as the group owner' do
+        context 'when the group is ready to sync' do
+          it 'returns 202 Accepted' do
+            ldap_sync(group.id, user, :disable!)
+            expect(response).to have_gitlab_http_status(202)
+          end
+
+          it 'queues a sync job' do
+            expect { ldap_sync(group.id, user, :fake!) }.to change(LdapGroupSyncWorker.jobs, :size).by(1)
+          end
+
+          it 'sets the ldap_sync state to pending' do
+            ldap_sync(group.id, user, :disable!)
+            expect(group.reload.ldap_sync_pending?).to be_truthy
+          end
+        end
+
+        context 'when the group is already pending a sync' do
+          before do
+            group.pending_ldap_sync!
+          end
+
+          it 'returns 202 Accepted' do
+            ldap_sync(group.id, user, :disable!)
+            expect(response).to have_gitlab_http_status(202)
+          end
+
+          it 'does not queue a sync job' do
+            expect { ldap_sync(group.id, user, :fake!) }.not_to change(LdapGroupSyncWorker.jobs, :size)
+          end
+
+          it 'does not change the ldap_sync state' do
+            expect do
+              ldap_sync(group.id, user, :disable!)
+            end.not_to change { group.reload.ldap_sync_status }
+          end
+        end
+
+        it 'returns 404 for a non existing group' do
+          ldap_sync(1328, user, :disable!)
+          expect(response).to have_gitlab_http_status(404)
+        end
+      end
+
+      context 'when authenticated as the admin' do
+        it 'returns 202 Accepted' do
+          ldap_sync(group.id, admin, :disable!)
+          expect(response).to have_gitlab_http_status(202)
+        end
+      end
+
+      context 'when authenticated as a non-owner user that can see the group' do
+        it 'returns 403' do
+          ldap_sync(group.id, another_user, :disable!)
+          expect(response).to have_gitlab_http_status(403)
+        end
+      end
+
+      context 'when authenticated as an user that cannot see the group' do
+        it 'returns 404' do
+          ldap_sync(private_group.id, user, :disable!)
+
+          expect(response).to have_gitlab_http_status(404)
+        end
+      end
+    end
+
+    context 'when the ldap_group_sync feature is not available' do
+      before do
+        stub_licensed_features(ldap_group_sync: false)
+      end
+
+      it 'returns 404 (same as CE would)' do
+        ldap_sync(group.id, admin, :disable!)
+
+        expect(response).to have_gitlab_http_status(404)
+      end
+    end
+  end
+
+  def ldap_sync(group_id, user, sidekiq_testing_method)
+    Sidekiq::Testing.send(sidekiq_testing_method) do
+      post api("/groups/#{group_id}/ldap_sync", user)
+    end
+  end
 end

lib/api/groups.rb

@@ -20,20 +20,19 @@ module API
         optional :share_with_group_lock, type: Boolean, desc: 'Prevent sharing a project with another group within this group'
       end
 
-      if Gitlab.ee?
       params :optional_params_ee do
-          optional :membership_lock, type: Boolean, desc: 'Prevent adding new members to project membership within this group'
-          optional :ldap_cn, type: String, desc: 'LDAP Common Name'
-          optional :ldap_access, type: Integer, desc: 'A valid access level'
-          optional :shared_runners_minutes_limit, type: Integer, desc: '(admin-only) Pipeline minutes quota for this group'
-          optional :extra_shared_runners_minutes_limit, type: Integer, desc: '(admin-only) Extra pipeline minutes quota for this group'
-          all_or_none_of :ldap_cn, :ldap_access
+      end
+
+      params :optional_update_params_ee do
       end
     end
 
+    include ::API::Helpers::GroupsHelpers
+
+    helpers do
       params :optional_params do
         use :optional_params_ce
-        use :optional_params_ee if Gitlab.ee?
+        use :optional_params_ee
       end
 
       params :statistics_params do
@@ -176,10 +175,7 @@ module API
         optional :name, type: String, desc: 'The name of the group'
         optional :path, type: String, desc: 'The path of the group'
         use :optional_params
-
-        if Gitlab.ee?
-          optional :file_template_project_id, type: Integer, desc: 'The ID of a project to use for custom templates in this group'
-        end
+        use :optional_update_params_ee
       end
       put ':id' do
         group = find_group!(params[:id])

lib/api/helpers/groups_helpers.rb

+# frozen_string_literal: true
+
+module API
+  module Helpers
+    module GroupsHelpers
+      extend ActiveSupport::Concern
+    end
+  end
+end
+
+API::Helpers::GroupsHelpers.prepend(EE::API::Helpers::GroupsHelpers)

spec/requests/api/groups_spec.rb

@@ -16,7 +16,6 @@ describe API::Groups do
   before do
     group1.add_owner(user1)
     group2.add_owner(user2)
-    group1.ldap_group_links.create cn: 'ldap-group', group_access: Gitlab::Access::MAINTAINER, provider: 'ldap'
   end
 
   describe "GET /groups" do
@@ -58,19 +57,6 @@ describe API::Groups do
         expect(json_response.length).to eq(1)
         expect(json_response)
           .to satisfy_one { |group| group['name'] == group1.name }
-
-        expect(json_response)
-          .to satisfy_one { |group| group['ldap_cn'] == group1.ldap_cn }
-        expect(json_response)
-          .to satisfy_one { |group| group['ldap_access'] == group1.ldap_access }
-
-        expect(json_response).to satisfy_one do |group|
-          ldap_group_link = group['ldap_group_links'].first
-
-          ldap_group_link['cn'] == group1.ldap_cn &&
-            ldap_group_link['group_access'] == group1.ldap_access &&
-            ldap_group_link['provider'] == 'ldap'
-        end
       end
 
       it "does not include statistics" do
@@ -466,16 +452,6 @@ describe API::Groups do
 
         expect(response).to have_gitlab_http_status(404)
       end
-
-      it 'returns 200 if shared_runners_minutes_limit is not changing' do
-        group1.update(shared_runners_minutes_limit: 133)
-
-        expect do
-          put api("/groups/#{group1.id}", user1), params: { shared_runners_minutes_limit: 133 }
-        end.not_to change { group1.shared_runners_minutes_limit }
-
-        expect(response).to have_gitlab_http_status(200)
-      end
     end
 
     context 'when authenticated as the admin' do
@@ -485,17 +461,6 @@ describe API::Groups do
         expect(response).to have_gitlab_http_status(200)
         expect(json_response['name']).to eq(new_group_name)
       end
-
-      # EE
-      it 'updates the group for shared_runners_minutes_limit' do
-        expect do
-          put api("/groups/#{group1.id}", admin), params: { shared_runners_minutes_limit: 133 }
-        end.to change { group1.reload.shared_runners_minutes_limit }
-          .from(nil).to(133)
-
-        expect(response).to have_gitlab_http_status(200)
-        expect(json_response['shared_runners_minutes_limit']).to eq(133)
-      end
     end
 
     context 'when authenticated as an user that can see the group' do
@@ -890,42 +855,6 @@ describe API::Groups do
 
         expect(response).to have_gitlab_http_status(400)
       end
-
-      it "creates an ldap_group_link if ldap_cn and ldap_access are supplied" do
-        group_attributes = attributes_for(:group, ldap_cn: 'ldap-group', ldap_access: Gitlab::Access::DEVELOPER)
-        expect { post api("/groups", admin), params: group_attributes }.to change { LdapGroupLink.count }.by(1)
-      end
-
-      # EE
-      context 'when shared_runners_minutes_limit is given' do
-        context 'when the current user is not an admin' do
-          it "does not create a group with shared_runners_minutes_limit" do
-            group = attributes_for(:group, { shared_runners_minutes_limit: 133 })
-
-            expect do
-              post api("/groups", user3), params: group
-            end.not_to change { Group.count }
-
-            expect(response).to have_gitlab_http_status(403)
-          end
-        end
-
-        context 'when the current user is an admin' do
-          it "creates a group with shared_runners_minutes_limit" do
-            group = attributes_for(:group, { shared_runners_minutes_limit: 133 })
-
-            expect do
-              post api("/groups", admin), params: group
-            end.to change { Group.count }.by(1)
-
-            created_group = Group.find(json_response['id'])
-
-            expect(created_group.shared_runners_minutes_limit).to eq(133)
-            expect(response).to have_gitlab_http_status(201)
-            expect(json_response['shared_runners_minutes_limit']).to eq(133)
-          end
-        end
-      end
     end
   end
 
@@ -1052,100 +981,4 @@ describe API::Groups do
       group2.add_owner(user1)
     end
   end
-
-  describe 'POST /groups/:id/ldap_sync' do
-    before do
-      allow(Gitlab::Auth::LDAP::Config).to receive(:enabled?).and_return(true)
-    end
-
-    context 'when the ldap_group_sync feature is available' do
-      before do
-        stub_licensed_features(ldap_group_sync: true)
-      end
-
-      context 'when authenticated as the group owner' do
-        context 'when the group is ready to sync' do
-          it 'returns 202 Accepted' do
-            ldap_sync(group1.id, user1, :disable!)
-            expect(response).to have_gitlab_http_status(202)
-          end
-
-          it 'queues a sync job' do
-            expect { ldap_sync(group1.id, user1, :fake!) }.to change(LdapGroupSyncWorker.jobs, :size).by(1)
-          end
-
-          it 'sets the ldap_sync state to pending' do
-            ldap_sync(group1.id, user1, :disable!)
-            expect(group1.reload.ldap_sync_pending?).to be_truthy
-          end
-        end
-
-        context 'when the group is already pending a sync' do
-          before do
-            group1.pending_ldap_sync!
-          end
-
-          it 'returns 202 Accepted' do
-            ldap_sync(group1.id, user1, :disable!)
-            expect(response).to have_gitlab_http_status(202)
-          end
-
-          it 'does not queue a sync job' do
-            expect { ldap_sync(group1.id, user1, :fake!) }.not_to change(LdapGroupSyncWorker.jobs, :size)
-          end
-
-          it 'does not change the ldap_sync state' do
-            expect do
-              ldap_sync(group1.id, user1, :disable!)
-            end.not_to change { group1.reload.ldap_sync_status }
-          end
-        end
-
-        it 'returns 404 for a non existing group' do
-          ldap_sync(1328, user1, :disable!)
-          expect(response).to have_gitlab_http_status(404)
-        end
-      end
-
-      context 'when authenticated as the admin' do
-        it 'returns 202 Accepted' do
-          ldap_sync(group1.id, admin, :disable!)
-          expect(response).to have_gitlab_http_status(202)
-        end
-      end
-
-      context 'when authenticated as a non-owner user that can see the group' do
-        it 'returns 403' do
-          ldap_sync(group1.id, user2, :disable!)
-          expect(response).to have_gitlab_http_status(403)
-        end
-      end
-
-      context 'when authenticated as an user that cannot see the group' do
-        it 'returns 404' do
-          ldap_sync(group2.id, user1, :disable!)
-
-          expect(response).to have_gitlab_http_status(404)
-        end
-      end
-    end
-
-    context 'when the ldap_group_sync feature is not available' do
-      before do
-        stub_licensed_features(ldap_group_sync: false)
-      end
-
-      it 'returns 404 (same as CE would)' do
-        ldap_sync(group1.id, admin, :disable!)
-
-        expect(response).to have_gitlab_http_status(404)
-      end
-    end
-  end
-
-  def ldap_sync(group_id, user, sidekiq_testing_method)
-    Sidekiq::Testing.send(sidekiq_testing_method) do
-      post api("/groups/#{group_id}/ldap_sync", user)
-    end
-  end
 end