Add LDAP User Filter to group link API

doc/api/groups.md

@@ -862,49 +862,71 @@ Lists LDAP group links.
 GET /groups/:id/ldap_group_links
 ```
 
-Parameters:
-
-- `id` (required) - The ID of a group
+| Attribute | Type           | Required | Description |
+| --------- | -------------- | -------- | ----------- |
+| `id`      | integer/string | yes      | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) |
 
-### Add LDAP group link **(STARTER)**
+### Add LDAP group link with CN or filter **(STARTER)**
 
-Adds an LDAP group link.
+Adds an LDAP group link using a CN or filter. Adding a group link by filter is only supported in the Premium tier and above.
 
 ```plaintext
 POST /groups/:id/ldap_group_links
 ```
 
-Parameters:
+| Attribute | Type           | Required | Description |
+| --------- | -------------- | -------- | ----------- |
+| `id`      | integer/string | yes      | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) |
+| `cn`      | string         | no       | The CN of an LDAP group |
+| `filter`  | string         | no       | The LDAP filter for the group |
+| `group_access` | integer   | yes      | Minimum access level for members of the LDAP group |
+| `provider` | string        | yes      | LDAP provider for the LDAP group link |
 
-- `id` (required) - The ID of a group
-- `cn` (required) - The CN of a LDAP group
-- `group_access` (required) - Minimum access level for members of the LDAP group
-- `provider` (required) - LDAP provider for the LDAP group
+NOTE: **Note:**
+To define the LDAP group link, provide either a `cn` or a `filter`, but not both.
 
 ### Delete LDAP group link **(STARTER)**
 
-Deletes an LDAP group link.
+Deletes an LDAP group link. Deprecated. Will be removed in a future release.
 
 ```plaintext
 DELETE /groups/:id/ldap_group_links/:cn
 ```
 
-Parameters:
-
-- `id` (required) - The ID of a group
-- `cn` (required) - The CN of a LDAP group
+| Attribute | Type           | Required | Description |
+| --------- | -------------- | -------- | ----------- |
+| `id`      | integer/string | yes      | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) |
+| `cn`      | string         | yes      | The CN of an LDAP group |
 
-Deletes a LDAP group link for a specific LDAP provider
+Deletes an LDAP group link for a specific LDAP provider. Deprecated. Will be removed in a future release.
 
 ```plaintext
 DELETE /groups/:id/ldap_group_links/:provider/:cn
 ```
 
-Parameters:
+| Attribute | Type           | Required | Description |
+| --------- | -------------- | -------- | ----------- |
+| `id`      | integer/string | yes      | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) |
+| `cn`      | string         | yes      | The CN of an LDAP group |
+| `provider` | string        | yes      | LDAP provider for the LDAP group link |
+
+### Delete LDAP group link with CN or filter **(STARTER)**
+
+Deletes an LDAP group link using a CN or filter. Deleting by filter is only supported in the Premium tier and above.
+
+```plaintext
+DELETE /groups/:id/ldap_group_links
+```
 
-- `id` (required) - The ID of a group
-- `cn` (required) - The CN of a LDAP group
-- `provider` (required) - Name of a LDAP provider
+| Attribute | Type           | Required | Description |
+| --------- | -------------- | -------- | ----------- |
+| `id`      | integer/string | yes      | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) |
+| `cn`      | string         | no       | The CN of an LDAP group |
+| `filter`  | string         | no       | The LDAP filter for the group |
+| `provider` | string        | yes       | LDAP provider for the LDAP group link |
+
+NOTE: **Note:**
+To delete the LDAP group link, provide either a `cn` or a `filter`, but not both.
 
 ## Namespaces in groups
 

ee/changelogs/unreleased/4435-add-ldap-user-filter-to-group-link-api.yml

+---
+title: Add LDAP user filter to group link API
+merge_request: 26202
+author:
+type: added

ee/lib/api/ldap_group_links.rb

@@ -16,7 +16,8 @@ module API
         authorize! :admin_group, group
 
         ldap_group_links = group.ldap_group_links
-        if ldap_group_links && ldap_group_links != []
+
+        if ldap_group_links.present?
           present ldap_group_links, with: EE::API::Entities::LdapGroupLink
         else
           render_api_error!('No linked LDAP groups found', 404)
@@ -27,16 +28,20 @@ module API
         success EE::API::Entities::LdapGroupLink
       end
       params do
-        requires 'cn', type: String, desc: 'The CN of a LDAP group'
+        optional 'cn', type: String, desc: 'The CN of a LDAP group'
+        optional 'filter', type: String, desc: 'The LDAP user filter'
         requires 'group_access', type: Integer, values: Gitlab::Access.all_values,
                                  desc: 'Level of permissions for the linked LDAP group'
         requires 'provider', type: String, desc: 'The LDAP provider for this LDAP group'
+        exactly_one_of :cn, :filter
       end
       post ":id/ldap_group_links" do
         group = find_group(params[:id])
         authorize! :admin_group, group
+        break not_found! if params[:filter] && !group.feature_available?(:ldap_group_sync_filter)
 
         ldap_group_link = group.ldap_group_links.new(declared_params(include_missing: false))
+
         if ldap_group_link.save
           present ldap_group_link, with: EE::API::Entities::LdapGroupLink
         else
@@ -44,7 +49,9 @@ module API
         end
       end
 
-      desc 'Remove a linked LDAP group from group'
+      desc 'Remove a linked LDAP group from group' do
+        detail 'Duplicate. DEPRECATED and will be removed in a later version'
+      end
       params do
         requires 'cn', type: String, desc: 'The CN of a LDAP group'
       end
@@ -54,6 +61,7 @@ module API
         authorize! :admin_group, group
 
         ldap_group_link = group.ldap_group_links.find_by(cn: params[:cn])
+
         if ldap_group_link
           ldap_group_link.destroy
           no_content!
@@ -63,7 +71,9 @@ module API
       end
       # rubocop: enable CodeReuse/ActiveRecord
 
-      desc 'Remove a linked LDAP group from group'
+      desc 'Remove a linked LDAP group from group' do
+        detail 'Duplicate. DEPRECATED and will be removed in a later version'
+      end
       params do
         requires 'cn', type: String, desc: 'The CN of a LDAP group'
         requires 'provider', type: String, desc: 'The LDAP provider for this LDAP group'
@@ -74,6 +84,7 @@ module API
         authorize! :admin_group, group
 
         ldap_group_link = group.ldap_group_links.find_by(cn: params[:cn], provider: params[:provider])
+
         if ldap_group_link
           ldap_group_link.destroy
           no_content!
@@ -82,6 +93,29 @@ module API
         end
       end
       # rubocop: enable CodeReuse/ActiveRecord
+
+      desc 'Remove a linked LDAP group from group'
+      params do
+        optional 'cn', type: String, desc: 'The CN of a LDAP group'
+        optional 'filter', type: String, desc: 'The LDAP user filter'
+        requires 'provider', type: String, desc: 'The LDAP provider for this LDAP group'
+        exactly_one_of :cn, :filter
+      end
+      # rubocop: disable CodeReuse/ActiveRecord
+      delete ":id/ldap_group_links" do
+        group = find_group(params[:id])
+        authorize! :admin_group, group
+        break not_found! if params[:filter] && !group.feature_available?(:ldap_group_sync_filter)
+
+        ldap_group_link = group.ldap_group_links.find_by(declared_params(include_missing: false))
+
+        if ldap_group_link
+          ldap_group_link.destroy
+          no_content!
+        else
+          render_api_error!('Linked LDAP group not found', 404)
+        end
+      end
     end
   end
 end

ee/lib/ee/api/entities/ldap_group_link.rb

@@ -5,6 +5,7 @@ module EE
     module Entities
       class LdapGroupLink < Grape::Entity
         expose :cn, :group_access, :provider
+        expose :filter, if: ->(_, _) { License.feature_available?(:ldap_group_sync_filter) }
       end
     end
   end

ee/spec/requests/api/ldap_group_links_spec.rb

@@ -13,7 +13,8 @@ describe API::LdapGroupLinks, api: true do
     group = create(:group)
     group.ldap_group_links.create cn: 'ldap-group1', group_access: Gitlab::Access::MAINTAINER, provider: 'ldap1'
     group.ldap_group_links.create cn: 'ldap-group2', group_access: Gitlab::Access::MAINTAINER, provider: 'ldap2'
-    group.ldap_group_links.create filter: '(uid=mary)', group_access: Gitlab::Access::DEVELOPER, provider: 'ldap3'
+    group.ldap_group_links.create cn: 'ldap-group3', group_access: Gitlab::Access::MAINTAINER, provider: 'ldap2'
+    group.ldap_group_links.create filter: '(uid=mary)', group_access: Gitlab::Access::DEVELOPER, provider: 'ldap2'
     group
   end
 
@@ -51,7 +52,8 @@ describe API::LdapGroupLinks, api: true do
           match([
             a_hash_including('cn' => 'ldap-group1', 'provider' => 'ldap1'),
             a_hash_including('cn' => 'ldap-group2', 'provider' => 'ldap2'),
-            a_hash_including('cn' => nil, 'provider' => 'ldap3')
+            a_hash_including('cn' => 'ldap-group3', 'provider' => 'ldap2'),
+            a_hash_including('cn' => nil, 'provider' => 'ldap2')
             ]))
       end
 
@@ -64,70 +66,99 @@ describe API::LdapGroupLinks, api: true do
   end
 
   describe "POST /groups/:id/ldap_group_links" do
-    context "when unauthenticated" do
-      it "returns authentication error" do
-        post api("/groups/#{group_with_ldap_links.id}/ldap_group_links")
-        expect(response).to have_gitlab_http_status(:unauthorized)
+    shared_examples 'creates LDAP group link' do
+      context "when unauthenticated" do
+        it "returns authentication error" do
+          params_test = params.merge( { group_access: GroupMember::GUEST, provider: 'ldap3' } )
+          post api("/groups/#{group_with_ldap_links.id}/ldap_group_links"), params: params_test
+
+          expect(response).to have_gitlab_http_status(:unauthorized)
+        end
       end
-    end
 
-    context "when a less priviledged user" do
-      it "does not allow less priviledged user to add LDAP group link" do
-        expect do
-          post api("/groups/#{group_with_ldap_links.id}/ldap_group_links", user),
-          params: { cn: 'ldap-group4', group_access: GroupMember::GUEST, provider: 'ldap3' }
-        end.not_to change { group_with_ldap_links.ldap_group_links.count }
+      context "when a less priviledged user" do
+        it "does not allow less priviledged user to add LDAP group link" do
+          params_test = params.merge( { group_access: GroupMember::GUEST, provider: 'ldap3' } )
+          expect do
+            post api("/groups/#{group_with_ldap_links.id}/ldap_group_links", user), params: params_test
+          end.not_to change { group_with_ldap_links.ldap_group_links.count }
 
-        expect(response).to have_gitlab_http_status(:forbidden)
+          expect(response).to have_gitlab_http_status(:forbidden)
+        end
       end
-    end
 
-    context "when owner of the group" do
-      it "returns ok and add ldap group link" do
-        expect do
-          post api("/groups/#{group_with_ldap_links.id}/ldap_group_links", owner),
-          params: { cn: 'ldap-group3', group_access: GroupMember::GUEST, provider: 'ldap3' }
-        end.to change { group_with_ldap_links.ldap_group_links.count }.by(1)
+      context "when owner of the group" do
+        it "returns ok and add ldap group link" do
+          params_test = params.merge( { group_access: GroupMember::GUEST, provider: 'ldap3' } )
+          expect do
+            post api("/groups/#{group_with_ldap_links.id}/ldap_group_links", owner), params: params_test
+          end.to change { group_with_ldap_links.ldap_group_links.count }.by(1)
 
-        expect(response).to have_gitlab_http_status(:created)
-        expect(json_response['cn']).to eq('ldap-group3')
-        expect(json_response['group_access']).to eq(GroupMember::GUEST)
-        expect(json_response['provider']).to eq('ldap3')
-      end
+          expect(response).to have_gitlab_http_status(:created)
+          expect(json_response['cn']).to eq(params_test[:cn])
+          expect(json_response['filter']).to eq(params_test[:filter])
+          expect(json_response['group_access']).to eq(params_test[:group_access])
+          expect(json_response['provider']).to eq(params_test[:provider])
+        end
 
-      # TODO: Correct and activate this test once issue #329 is fixed
-      xit "returns ok and add ldap group link even if no provider specified" do
-        expect do
-          post api("/groups/#{group_with_ldap_links.id}/ldap_group_links", owner),
-          params: { cn: 'ldap-group3', group_access: GroupMember::GUEST }
-        end.to change { group_with_ldap_links.ldap_group_links.count }.by(1)
+        it "returns error if LDAP group link already exists" do
+          params_test = params.merge( { group_access: GroupMember::GUEST, provider: 'ldap2' } )
+          post api("/groups/#{group_with_ldap_links.id}/ldap_group_links", owner), params: params_test
 
-        expect(response).to have_gitlab_http_status(:created)
-        expect(json_response['cn']).to eq('ldap-group3')
-        expect(json_response['group_access']).to eq(GroupMember::GUEST)
-        expect(json_response['provider']).to eq('ldapmain')
-      end
+          expect(response).to have_gitlab_http_status(:conflict)
+        end
+
+        it "returns a 400 error when CN or filter is not given" do
+          params_test = { group_access: GroupMember::GUEST, provider: 'ldap1' }
+          post api("/groups/#{group_with_ldap_links.id}/ldap_group_links", owner), params: params_test
+
+          expect(response).to have_gitlab_http_status(:bad_request)
+        end
+
+        it "returns a 400 error when group access is not given" do
+          params_test = params.merge( { provider: 'ldap1' } )
+          post api("/groups/#{group_with_ldap_links.id}/ldap_group_links", owner), params: params_test
 
-      it "returns error if LDAP group link already exists" do
-        post api("//groups/#{group_with_ldap_links.id}/ldap_group_links", owner), params: { provider: 'ldap1', cn: 'ldap-group1', group_access: GroupMember::GUEST }
-        expect(response).to have_gitlab_http_status(:conflict)
+          expect(response).to have_gitlab_http_status(:bad_request)
+        end
+
+        it "returns a 422 error when group access is not valid" do
+          params_test = params.merge( { group_access: 11, provider: 'ldap1' } )
+          post api("/groups/#{group_with_ldap_links.id}/ldap_group_links", owner), params: params_test
+
+          expect(response).to have_gitlab_http_status(:bad_request)
+          expect(json_response['error']).to eq('group_access does not have a valid value')
+        end
       end
+    end
 
-      it "returns a 400 error when cn is not given" do
-        post api("//groups/#{group_with_ldap_links.id}/ldap_group_links", owner), params: { group_access: GroupMember::GUEST }
-        expect(response).to have_gitlab_http_status(:bad_request)
+    context "adding a group link via CN" do
+      it_behaves_like 'creates LDAP group link' do
+        let(:params) { { cn: 'ldap-group3' } }
       end
+    end
+
+    context "adding a group link via filter" do
+      context "feature is available" do
+        before do
+          stub_licensed_features(ldap_group_sync_filter: true)
+        end
 
-      it "returns a 400 error when group access is not given" do
-        post api("//groups/#{group_with_ldap_links.id}/ldap_group_links", owner), params: { cn: 'ldap-group3' }
-        expect(response).to have_gitlab_http_status(:bad_request)
+        it_behaves_like 'creates LDAP group link' do
+          let(:params) { { filter: '(uid=mary)' } }
+        end
       end
 
-      it "returns a 422 error when group access is not known" do
-        post api("//groups/#{group_with_ldap_links.id}/ldap_group_links", owner), params: { cn: 'ldap-group3', group_access: 11, provider: 'ldap1' }
+      context 'feature is not available' do
+        before do
+          stub_licensed_features(ldap_group_sync_filter: false)
+        end
+
+        it 'returns 404' do
+          post api("/groups/#{group_with_ldap_links.id}/ldap_group_links", owner), params: { filter: '(uid=mary)', group_access: GroupMember::GUEST, provider: 'ldap3' }
 
-        expect(response).to have_gitlab_http_status(:bad_request)
-        expect(json_response['error']).to eq('group_access does not have a valid value')
+          expect(response).to have_gitlab_http_status(:not_found)
+        end
       end
     end
   end
@@ -205,4 +236,85 @@ describe API::LdapGroupLinks, api: true do
       end
     end
   end
+
+  describe 'DELETE /groups/:id/ldap_group_links' do
+    shared_examples 'deletes LDAP group link' do
+      context "when unauthenticated" do
+        it "returns authentication error" do
+          delete api("/groups/#{group_with_ldap_links.id}/ldap_group_links"), params: params
+          expect(response).to have_gitlab_http_status(:unauthorized)
+        end
+      end
+
+      context "when a less priviledged user" do
+        it "does not remove the LDAP group link" do
+          expect do
+            delete api("/groups/#{group_with_ldap_links.id}/ldap_group_links", user), params: params
+          end.not_to change { group_with_ldap_links.ldap_group_links.count }
+
+          expect(response).to have_gitlab_http_status(:forbidden)
+        end
+      end
+
+      context "when owner of the group" do
+        it "removes ldap group link" do
+          expect do
+            delete api("/groups/#{group_with_ldap_links.id}/ldap_group_links", owner), params: params
+
+            expect(response).to have_gitlab_http_status(:no_content)
+          end.to change { group_with_ldap_links.ldap_group_links.count }.by(-1)
+        end
+      end
+    end
+
+    shared_examples 'group link is not found' do
+      context "when owner of the group" do
+        it "returns 404 if LDAP input not used for a LDAP group link" do
+          expect do
+            delete api("/groups/#{group_with_ldap_links.id}/ldap_group_links", owner), params: params
+          end.not_to change { group_with_ldap_links.ldap_group_links.count }
+
+          expect(response).to have_gitlab_http_status(:not_found)
+        end
+      end
+    end
+
+    context "deleting a group link via CN and provider" do
+      it_behaves_like 'deletes LDAP group link' do
+        let(:params) { { cn: 'ldap-group3', provider: 'ldap2' } }
+      end
+
+      it_behaves_like 'group link is not found' do
+        let(:params) { { cn: 'ldap-group1356', provider: 'ldap2' } }
+      end
+    end
+
+    context "deleting a group link via filter and provider" do
+      context "feature is available" do
+        before do
+          stub_licensed_features(ldap_group_sync_filter: true)
+        end
+
+        it_behaves_like 'deletes LDAP group link' do
+          let(:params) { { filter: '(uid=mary)', provider: 'ldap2' } }
+        end
+
+        it_behaves_like 'group link is not found' do
+          let(:params) { { filter: '(uid=mary3)', provider: 'ldap1' } }
+        end
+      end
+
+      context 'feature is not available' do
+        before do
+          stub_licensed_features(ldap_group_sync_filter: false)
+        end
+
+        it 'returns 404' do
+          delete api("/groups/#{group_with_ldap_links.id}/ldap_group_links", owner), params: { filter: '(uid=mary)', provider: 'ldap1' }
+
+          expect(response).to have_gitlab_http_status(:not_found)
+        end
+      end
+    end
+  end
 end