Minor updates to LDAP override membership API

Minor updates to `peterlloydcc` work to expose the ability to
override LDAP membership via the API. This along with `peterlloydcc`
original commit allow users to set the override flag on a member
to true or false, as needed.

doc/api/members.md

@@ -282,13 +282,14 @@ Example response:
 }
 ```
 
-### Override LDAP permissions for a member from a group
+### Set override flag for a member of a group
 
-Allows access level to be overriden for a LDAP group member
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/4875) in GitLab 12.10.
 
->**Note:** This API endpoint is only available on 11.x Starter and above.
+By default, the access level of LDAP group members is set to the value specified
+by LDAP through Group Sync. You can allow access level overrides by calling this endpoint.
 
-```
+```plaintext
 POST /groups/:id/members/:user_id/override
 ```
 
@@ -317,13 +318,14 @@ Example response:
 }
 ```
 
-### Un-override LDAP permissions for a member from a group
+### Remove override for a member of a group
 
-Resets access level for a LDAP group member back to be level determined by the LDAP group
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/4875) in GitLab 12.10.
 
->**Note:** This API endpoint is only available on 11.x Starter and above.
+Sets the override flag to false and allows LDAP Group Sync to reset the access
+level to the LDAP-prescribed value.
 
-```
+```plaintext
 DELETE /groups/:id/members/:user_id/override
 ```
 

ee/app/models/ee/group_member.rb

@@ -20,6 +20,7 @@ module EE
       end
 
       scope :non_owners, -> { where("members.access_level < ?", ::Gitlab::Access::OWNER) }
+      scope :by_user_id, ->(user_id) { where(user_id: user_id) }
     end
 
     class_methods do

ee/changelogs/unreleased-ee/pl-override-api.yml

 ---
 title: Add API methods to manipulate LDAP Override attribute
-merge_request:
+merge_request: 28674
 author: Peter Lloyd <peter.lloyd@cambridgeconsultants.com>
 type: added

ee/lib/ee/api/entities/member.rb

@@ -14,7 +14,7 @@ module EE
           expose :is_using_seat, if: -> (_, options) { options[:show_seat_info] }
 
           expose :override,
-                 if: ->(member, options) { member.source_type == 'Namespace' && member.ldap? }
+                 if: ->(member, _) { member.source_type == 'Namespace' && member.ldap? }
         end
       end
     end

ee/lib/ee/api/helpers/members_helpers.rb

@@ -45,6 +45,21 @@ module EE
           member
         end
 
+        def find_member(params)
+          source = find_source(:group, params.delete(:id))
+          authorize! :override_group_member, source
+
+          source.members.by_user_id(params[:user_id]).first
+        end
+
+        def present_member(updated_member)
+          if updated_member.valid?
+            present updated_member, with: ::API::Entities::Member
+          else
+            render_validation_error!(updated_member)
+          end
+        end
+
         def log_audit_event(member)
           ::AuditEventService.new(
             current_user,

ee/lib/ee/api/members.rb

@@ -10,55 +10,37 @@ module EE
           requires :id, type: String, desc: 'The ID of a group'
         end
         resource :groups, requirements: ::API::API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
-          desc 'Overrides a member of a group.' do
+          desc 'Overrides the access level of an LDAP group member.' do
             success Entities::Member
           end
           params do
             requires :user_id, type: Integer, desc: 'The user ID of the member'
           end
-          # rubocop: disable CodeReuse/ActiveRecord
           post ":id/members/:user_id/override" do
-            source = find_source(:group, params.delete(:id))
-            authorize_admin_source!(:group, source)
+            member = find_member(params)
 
-            member = source.members.find_by!(user_id: params[:user_id])
-            updated_member =
-              ::Members::UpdateService
-                .new(current_user, { override: true })
-                .execute(member, permission: :override)
+            updated_member = ::Members::UpdateService
+              .new(current_user, { override: true })
+              .execute(member, permission: :override)
 
-            if updated_member.valid?
-              present updated_member, with: ::API::Entities::Member
-            else
-              render_validation_error!(updated_member)
-            end
+            present_member(updated_member)
           end
-          # rubocop: enable CodeReuse/ActiveRecord
 
-          desc 'Remove an override of a member of a group.' do
+          desc 'Remove an LDAP group member access level override.' do
             success Entities::Member
           end
           params do
             requires :user_id, type: Integer, desc: 'The user ID of the member'
           end
-          # rubocop: disable CodeReuse/ActiveRecord
           delete ":id/members/:user_id/override" do
-            source = find_source(:group, params.delete(:id))
-            authorize_admin_source!(:group, source)
+            member = find_member(params)
 
-            member = source.members.find_by!(user_id: params[:user_id])
-            updated_member =
-              ::Members::UpdateService
-                .new(current_user, { override: false })
-                .execute(member, permission: :override)
+            updated_member = ::Members::UpdateService
+              .new(current_user, { override: false })
+              .execute(member, permission: :override)
 
-            if updated_member.valid?
-              present updated_member, with: ::API::Entities::Member
-            else
-              render_validation_error!(updated_member)
-            end
+            present_member(updated_member)
           end
-          # rubocop: enable CodeReuse/ActiveRecord
         end
       end
     end

ee/spec/requests/api/members_spec.rb

@@ -3,262 +3,249 @@
 require 'spec_helper'
 
 describe API::Members do
-  let(:group) { create(:group) }
-  let(:owner) { create(:user) }
-  let(:project) { create(:project, group: group) }
+  context 'without LDAP' do
+    let(:group) { create(:group) }
+    let(:owner) { create(:user) }
+    let(:project) { create(:project, group: group) }
 
-  before do
-    group.add_owner(owner)
-  end
+    before do
+      group.add_owner(owner)
+    end
 
-  describe 'POST /projects/:id/members' do
-    context 'group membership locked' do
-      let(:user) { create(:user) }
-      let(:group) { create(:group, membership_lock: true)}
-      let(:project) { create(:project, group: group) }
+    describe 'POST /projects/:id/members' do
+      context 'group membership locked' do
+        let(:user) { create(:user) }
+        let(:group) { create(:group, membership_lock: true)}
+        let(:project) { create(:project, group: group) }
 
-      context 'project in a group' do
-        it 'returns a 405 method not allowed error when group membership lock is enabled' do
-          post api("/projects/#{project.id}/members", owner),
-               params: { user_id: user.id, access_level: Member::MAINTAINER }
+        context 'project in a group' do
+          it 'returns a 405 method not allowed error when group membership lock is enabled' do
+            post api("/projects/#{project.id}/members", owner),
+                 params: { user_id: user.id, access_level: Member::MAINTAINER }
 
-          expect(response).to have_gitlab_http_status(:method_not_allowed)
+            expect(response).to have_gitlab_http_status(:method_not_allowed)
+          end
         end
       end
     end
-  end
 
-  describe 'GET /groups/:id/members' do
-    it 'matches json schema' do
-      get api("/groups/#{group.to_param}/members", owner)
+    describe 'GET /groups/:id/members' do
+      it 'matches json schema' do
+        get api("/groups/#{group.to_param}/members", owner)
 
-      expect(response).to have_gitlab_http_status(:ok)
-      expect(response).to match_response_schema('public_api/v4/members')
-    end
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(response).to match_response_schema('public_api/v4/members')
+      end
 
-    context 'when a group has SAML provider configured' do
-      let(:maintainer) { create(:user) }
+      context 'when a group has SAML provider configured' do
+        let(:maintainer) { create(:user) }
 
-      before do
-        saml_provider = create :saml_provider, group: group
-        create :group_saml_identity, user: owner, saml_provider: saml_provider
+        before do
+          saml_provider = create :saml_provider, group: group
+          create :group_saml_identity, user: owner, saml_provider: saml_provider
 
-        group.add_maintainer(maintainer)
-      end
+          group.add_maintainer(maintainer)
+        end
 
-      context 'and current_user is group owner' do
-        it 'returns a list of users with group SAML identities info' do
-          get api("/groups/#{group.to_param}/members", owner)
+        context 'and current_user is group owner' do
+          it 'returns a list of users with group SAML identities info' do
+            get api("/groups/#{group.to_param}/members", owner)
 
-          expect(response).to have_gitlab_http_status(:ok)
-          expect(json_response.size).to eq(2)
-          expect(json_response.first['group_saml_identity']).to match(kind_of(Hash))
-        end
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(json_response.size).to eq(2)
+            expect(json_response.first['group_saml_identity']).to match(kind_of(Hash))
+          end
 
-        it 'allows to filter by linked identity presence' do
-          get api("/groups/#{group.to_param}/members?with_saml_identity=true", owner)
+          it 'allows to filter by linked identity presence' do
+            get api("/groups/#{group.to_param}/members?with_saml_identity=true", owner)
 
-          expect(response).to have_gitlab_http_status(:ok)
-          expect(json_response.size).to eq(1)
-          expect(json_response.any? { |member| member['id'] == maintainer.id }).to be_falsey
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(json_response.size).to eq(1)
+            expect(json_response.any? { |member| member['id'] == maintainer.id }).to be_falsey
+          end
         end
-      end
 
-      context 'and current_user is not an owner' do
-        it 'returns a list of users without group SAML identities info' do
-          get api("/groups/#{group.to_param}/members", maintainer)
+        context 'and current_user is not an owner' do
+          it 'returns a list of users without group SAML identities info' do
+            get api("/groups/#{group.to_param}/members", maintainer)
 
-          expect(response).to have_gitlab_http_status(:ok)
-          expect(json_response.map(&:keys).flatten).not_to include('group_saml_identity')
-        end
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(json_response.map(&:keys).flatten).not_to include('group_saml_identity')
+          end
 
-        it 'ignores filter by linked identity presence' do
-          get api("/groups/#{group.to_param}/members?with_saml_identity=true", maintainer)
+          it 'ignores filter by linked identity presence' do
+            get api("/groups/#{group.to_param}/members?with_saml_identity=true", maintainer)
 
-          expect(response).to have_gitlab_http_status(:ok)
-          expect(json_response.size).to eq(2)
-          expect(json_response.any? { |member| member['id'] == maintainer.id }).to be_truthy
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(json_response.size).to eq(2)
+            expect(json_response.any? { |member| member['id'] == maintainer.id }).to be_truthy
+          end
         end
       end
-    end
 
-    context 'with is_using_seat' do
-      shared_examples 'seat information not included' do
-        it 'returns a list of users that does not contain the is_using_seat attribute' do
-          get api(api_url, owner)
+      context 'with is_using_seat' do
+        shared_examples 'seat information not included' do
+          it 'returns a list of users that does not contain the is_using_seat attribute' do
+            get api(api_url, owner)
 
-          expect(response).to have_gitlab_http_status(:ok)
-          expect(json_response.size).to eq(1)
-          expect(json_response.first.keys).not_to include('is_using_seat')
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(json_response.size).to eq(1)
+            expect(json_response.first.keys).not_to include('is_using_seat')
+          end
         end
-      end
 
-      context 'with show_seat_info set to true' do
-        it 'returns a list of users that contains the is_using_seat attribute' do
-          get api("/groups/#{group.to_param}/members?show_seat_info=true", owner)
+        context 'with show_seat_info set to true' do
+          it 'returns a list of users that contains the is_using_seat attribute' do
+            get api("/groups/#{group.to_param}/members?show_seat_info=true", owner)
 
-          expect(response).to have_gitlab_http_status(:ok)
-          expect(json_response.size).to eq(1)
-          expect(json_response.first['is_using_seat']).to be_truthy
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(json_response.size).to eq(1)
+            expect(json_response.first['is_using_seat']).to be_truthy
+          end
         end
-      end
 
-      context 'with show_seat_info set to false' do
-        let(:api_url) { "/groups/#{group.to_param}/members?show_seat_info=false" }
+        context 'with show_seat_info set to false' do
+          let(:api_url) { "/groups/#{group.to_param}/members?show_seat_info=false" }
 
-        it_behaves_like 'seat information not included'
-      end
+          it_behaves_like 'seat information not included'
+        end
 
-      context 'with no show_seat_info set' do
-        let(:api_url) { "/groups/#{group.to_param}/members" }
+        context 'with no show_seat_info set' do
+          let(:api_url) { "/groups/#{group.to_param}/members" }
 
-        it_behaves_like 'seat information not included'
+          it_behaves_like 'seat information not included'
+        end
       end
     end
-  end
 
-  shared_examples 'POST /:source_type/:id/members' do |source_type|
-    let(:stranger) { create(:user) }
-    let(:url) { "/#{source_type.pluralize}/#{source.id}/members" }
+    shared_examples 'POST /:source_type/:id/members' do |source_type|
+      let(:stranger) { create(:user) }
+      let(:url) { "/#{source_type.pluralize}/#{source.id}/members" }
 
-    context "with :source_type == #{source_type.pluralize}" do
-      it 'creates an audit event while creating a new member' do
-        params = { user_id: stranger.id, access_level: Member::DEVELOPER }
+      context "with :source_type == #{source_type.pluralize}" do
+        it 'creates an audit event while creating a new member' do
+          params = { user_id: stranger.id, access_level: Member::DEVELOPER }
 
-        expect do
-          post api(url, owner), params: params
+          expect do
+            post api(url, owner), params: params
 
-          expect(response).to have_gitlab_http_status(:created)
-        end.to change { AuditEvent.count }.by(1)
-      end
+            expect(response).to have_gitlab_http_status(:created)
+          end.to change { AuditEvent.count }.by(1)
+        end
 
-      it 'does not create audit event if creating a new member fails' do
-        params = { user_id: 0, access_level: Member::DEVELOPER }
+        it 'does not create audit event if creating a new member fails' do
+          params = { user_id: 0, access_level: Member::DEVELOPER }
 
-        expect do
-          post api(url, owner), params: params
+          expect do
+            post api(url, owner), params: params
 
-          expect(response).to have_gitlab_http_status(:not_found)
-        end.not_to change { AuditEvent.count }
+            expect(response).to have_gitlab_http_status(:not_found)
+          end.not_to change { AuditEvent.count }
+        end
       end
     end
-  end
 
-  it_behaves_like 'POST /:source_type/:id/members', 'project' do
-    let(:source) { project }
-  end
+    it_behaves_like 'POST /:source_type/:id/members', 'project' do
+      let(:source) { project }
+    end
 
-  it_behaves_like 'POST /:source_type/:id/members', 'group' do
-    let(:source) { group }
+    it_behaves_like 'POST /:source_type/:id/members', 'group' do
+      let(:source) { group }
+    end
   end
-end
 
-describe "API::Members with LDAP link" do
-  let(:owner) { create(:user, username: 'owner_user') }
-  let(:developer) { create(:user) }
-  let(:ldap_developer) { create(:user) }
-  let(:ldap_developer2) { create(:user) }
+  context 'group with LDAP group link' do
+    include LdapHelpers
 
-  let(:group) { create(:group_with_ldap_group_link, :public) }
+    let(:owner) { create(:user, username: 'owner_user') }
+    let(:developer) { create(:user) }
+    let(:ldap_developer) { create(:user) }
+    let(:ldap_developer2) { create(:user) }
 
-  let!(:owner_member) { create(:group_member, :owner, group: group, user: owner) }
-  let!(:ldap_member) { create(:group_member, :developer, group: group, user: ldap_developer, ldap: true) }
-  let!(:overridden_member) { create(:group_member, :developer, group: group, user: ldap_developer2, ldap: true, override: true) }
-  let!(:regular_member) { create(:group_member, :developer, group: group, user: developer, ldap: false) }
+    let(:group) { create(:group_with_ldap_group_link, :public) }
 
-  before do
-    # We need to actually activate the LDAP config otherwise `Group#ldap_synced?` will always be false!
-    allow(Gitlab.config.ldap).to receive_messages(enabled: true)
-  end
+    let!(:ldap_member) { create(:group_member, :developer, group: group, user: ldap_developer, ldap: true) }
+    let!(:overridden_member) { create(:group_member, :developer, group: group, user: ldap_developer2, ldap: true, override: true) }
+    let!(:regular_member) { create(:group_member, :developer, group: group, user: developer, ldap: false) }
+
+    before do
+      create(:group_member, :owner, group: group, user: owner)
+      stub_ldap_setting(enabled: true)
+    end
 
-  describe 'GET /groups/:id/members/:user_id has no override attribute' do
-    context 'project in a group' do
-      it 'succeeds not getting override on a non-LDAP user' do
+    describe 'GET /groups/:id/members/:user_id' do
+      it 'does not contain an override attribute for non-LDAP users in the response' do
         get api("/groups/#{group.id}/members/#{developer.id}", owner)
 
-        expect(response).to have_gitlab_http_status(200)
+        expect(response).to have_gitlab_http_status(:ok)
         expect(json_response['id']).to eq(developer.id)
         expect(json_response['access_level']).to eq(Member::DEVELOPER)
         expect(json_response['override']).to eq(nil)
       end
-    end
-  end
 
-  describe 'GET /groups/:id/members/:user_id has an override attribute' do
-    context 'project in a group' do
-      it 'succeeds getting override on an LDAP user' do
+      it 'contains an override attribute for ldap users in the response' do
         get api("/groups/#{group.id}/members/#{ldap_developer.id}", owner)
 
-        expect(response).to have_gitlab_http_status(200)
+        expect(response).to have_gitlab_http_status(:ok)
         expect(json_response['id']).to eq(ldap_developer.id)
-        expect(json_response['override']).to eq(false)
         expect(json_response['access_level']).to eq(Member::DEVELOPER)
+        expect(json_response['override']).to eq(false)
       end
     end
-  end
-
-  describe 'POST /groups/:id/members/:user_id/override with LDAP links' do
-    context 'project in a group' do
-      it 'succeeds when override is set on an LDAP user' do
-        post api("/groups/#{group.id}/members/#{ldap_developer.id}/override", owner)
-        expect(response).to have_gitlab_http_status(201)
-        expect(json_response['id']).to eq(ldap_developer.id)
-        expect(json_response['override']).to eq(true)
-        expect(json_response['access_level']).to eq(Member::DEVELOPER)
-      end
 
+    describe 'PUT /groups/:id/members/:user_id' do
       it 'succeeds when access_level is modified after override has been set' do
         post api("/groups/#{group.id}/members/#{ldap_developer.id}/override", owner)
-        expect(response).to have_gitlab_http_status(201)
+        expect(response).to have_gitlab_http_status(:created)
 
         put api("/groups/#{group.id}/members/#{ldap_developer.id}", owner),
-          params: { access_level: Member::MAINTAINER }
+            params: { access_level: Member::MAINTAINER }
 
-        expect(response).to have_gitlab_http_status(200)
+        expect(response).to have_gitlab_http_status(:ok)
         expect(json_response['id']).to eq(ldap_developer.id)
         expect(json_response['override']).to eq(true)
         expect(json_response['access_level']).to eq(Member::MAINTAINER)
       end
+
+      it 'fails when access level is modified without an override' do
+        put api("/groups/#{group.id}/members/#{ldap_developer.id}", owner),
+            params: { access_level: Member::MAINTAINER }
+
+        expect(response).to have_gitlab_http_status(:forbidden)
+      end
     end
-  end
 
-  describe 'DELETE /groups/:id/members/:user_id/override with LDAP links' do
-    context 'project in a group' do
+    describe 'POST /groups/:id/members/:user_id/override' do
       it 'succeeds when override is set on an LDAP user' do
-        delete api("/groups/#{group.id}/members/#{ldap_developer2.id}/override", owner)
+        post api("/groups/#{group.id}/members/#{ldap_developer.id}/override", owner)
 
-        expect(response).to have_gitlab_http_status(200)
-        expect(json_response['id']).to eq(ldap_developer2.id)
-        expect(json_response['override']).to eq(false)
+        expect(response).to have_gitlab_http_status(:created)
+        expect(json_response['id']).to eq(ldap_developer.id)
+        expect(json_response['override']).to eq(true)
+        expect(json_response['access_level']).to eq(Member::DEVELOPER)
       end
-    end
-  end
 
-  shared_examples 'POST /:source_type/:id/members/:user_id/override' do |source_type|
-    context "with :source_type == #{source_type.pluralize}" do
-      it 'returns 403  when override is set for a non-ldap user' do
-        post api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}/override", owner)
+      it 'fails when override is set for a non-ldap user' do
+        post api("/groups/#{group.id}/members/#{developer.id}/override", owner)
 
-        expect(response).to have_gitlab_http_status(403)
+        expect(response).to have_gitlab_http_status(:forbidden)
       end
     end
-  end
 
-  shared_examples 'DELETE /:source_type/:id/members/:user_id/override' do |source_type|
-    context "with :source_type == #{source_type.pluralize}" do
-      it 'returns 403  when override is set for a non-ldap user' do
-        delete api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}/override", owner)
+    describe 'DELETE /groups/:id/members/:user_id/override with LDAP links' do
+      it 'succeeds when override is already set on an LDAP user' do
+        delete api("/groups/#{group.id}/members/#{ldap_developer2.id}/override", owner)
 
-        expect(response).to have_gitlab_http_status(403)
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response['id']).to eq(ldap_developer2.id)
+        expect(json_response['override']).to eq(false)
       end
-    end
-  end
 
-  it_behaves_like 'POST /:source_type/:id/members/:user_id/override', 'group' do
-    let(:source) { group }
-  end
+      it 'returns 403 when override is set for a non-ldap user' do
+        delete api("/groups/#{group.id}/members/#{developer.id}/override", owner)
 
-  it_behaves_like 'DELETE /:source_type/:id/members/:user_id/override', 'group' do
-    let(:source) { group }
+        expect(response).to have_gitlab_http_status(:forbidden)
+      end
+    end
   end
 end