Merge branch 'dblessing-gl-override-api' into 'master'

Expose LDAP Override in API, add set and clear methods

Closes #4875

See merge request gitlab-org/gitlab!28674

doc/api/members.md

@@ -282,6 +282,78 @@ Example response:
 }
 ```
 
+### Set override flag for a member of a group
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/4875) in GitLab 12.10.
+
+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
+```
+
+| Attribute | Type | Required | Description |
+| --------- | ---- | -------- | ----------- |
+| `id`      | integer/string | yes | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) owned by the authenticated user |
+| `user_id` | integer | yes   | The user ID of the member |
+
+```bash
+curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/groups/:id/members/:user_id/override
+```
+
+Example response:
+
+```json
+{
+  "id": 1,
+  "username": "raymond_smith",
+  "name": "Raymond Smith",
+  "state": "active",
+  "avatar_url": "https://www.gravatar.com/avatar/c2525a7f58ae3776070e44c106c48e15?s=80&d=identicon",
+  "web_url": "http://192.168.1.8:3000/root",
+  "expires_at": "2012-10-22T14:13:35Z",
+  "access_level": 40,
+  "override": true
+}
+```
+
+### Remove override for a member of a group
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/4875) in GitLab 12.10.
+
+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
+```
+
+| Attribute | Type | Required | Description |
+| --------- | ---- | -------- | ----------- |
+| `id`      | integer/string | yes | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) owned by the authenticated user |
+| `user_id` | integer | yes   | The user ID of the member |
+
+```bash
+curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/groups/:id/members/:user_id/override
+```
+
+Example response:
+
+```json
+{
+  "id": 1,
+  "username": "raymond_smith",
+  "name": "Raymond Smith",
+  "state": "active",
+  "avatar_url": "https://www.gravatar.com/avatar/c2525a7f58ae3776070e44c106c48e15?s=80&d=identicon",
+  "web_url": "http://192.168.1.8:3000/root",
+  "expires_at": "2012-10-22T14:13:35Z",
+  "access_level": 40,
+  "override": false
+}
+```
+
 ## Remove a member from a group or project
 
 Removes a user from a group or project.

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: 28674
+author: Peter Lloyd <peter.lloyd@cambridgeconsultants.com>
+type: added

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

@@ -10,7 +10,11 @@ module EE
           expose :group_saml_identity,
                  using: ::API::Entities::Identity,
                  if: -> (member, options) { Ability.allowed?(options[:current_user], :read_group_saml_identity, member.source) }
+
           expose :is_using_seat, if: -> (_, options) { options[:show_seat_info] }
+
+          expose :override,
+                 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

+# frozen_string_literal: true
+
+module EE
+  module API
+    module Members
+      extend ActiveSupport::Concern
+
+      prepended do
+        params do
+          requires :id, type: String, desc: 'The ID of a group'
+        end
+        resource :groups, requirements: ::API::API::NAMESPACE_OR_PROJECT_REQUIREMENTS 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
+          post ":id/members/:user_id/override" do
+            member = find_member(params)
+
+            updated_member = ::Members::UpdateService
+              .new(current_user, { override: true })
+              .execute(member, permission: :override)
+
+            present_member(updated_member)
+          end
+
+          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
+          delete ":id/members/:user_id/override" do
+            member = find_member(params)
+
+            updated_member = ::Members::UpdateService
+              .new(current_user, { override: false })
+              .execute(member, permission: :override)
+
+            present_member(updated_member)
+          end
+        end
+      end
+    end
+  end
+end

ee/spec/requests/api/members_spec.rb

@@ -3,152 +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)
+
+            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
 
-      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)
+          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(2)
-          expect(json_response.first['group_saml_identity']).to match(kind_of(Hash))
+            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
 
-        it 'allows to filter by linked identity presence' do
-          get api("/groups/#{group.to_param}/members?with_saml_identity=true", owner)
+        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.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.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)
+
+            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
 
-      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 '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.map(&:keys).flatten).not_to include('group_saml_identity')
+            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
 
-        it 'ignores filter by linked identity presence' do
-          get api("/groups/#{group.to_param}/members?with_saml_identity=true", maintainer)
+        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(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(1)
+            expect(json_response.first['is_using_seat']).to be_truthy
+          end
+        end
+
+        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
+
+        context 'with no show_seat_info set' do
+          let(:api_url) { "/groups/#{group.to_param}/members" }
+
+          it_behaves_like 'seat information not included'
         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)
+    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 }
 
-          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 do
+            post api(url, owner), params: params
+
+            expect(response).to have_gitlab_http_status(:created)
+          end.to change { AuditEvent.count }.by(1)
         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)
+        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(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(:not_found)
+          end.not_to change { AuditEvent.count }
         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', 'group' do
+      let(:source) { group }
+    end
+  end
+
+  context 'group with LDAP group link' do
+    include LdapHelpers
+
+    let(:owner) { create(:user, username: 'owner_user') }
+    let(:developer) { create(:user) }
+    let(:ldap_developer) { create(:user) }
+    let(:ldap_developer2) { create(:user) }
 
-      context 'with show_seat_info set to false' do
-        let(:api_url) { "/groups/#{group.to_param}/members?show_seat_info=false" }
+    let(:group) { create(:group_with_ldap_group_link, :public) }
 
-        it_behaves_like 'seat information not included'
+    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' 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(: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
 
-      context 'with no show_seat_info set' do
-        let(:api_url) { "/groups/#{group.to_param}/members" }
+      it 'contains an override attribute for ldap users in the response' do
+        get api("/groups/#{group.id}/members/#{ldap_developer.id}", owner)
 
-        it_behaves_like 'seat information not included'
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response['id']).to eq(ldap_developer.id)
+        expect(json_response['access_level']).to eq(Member::DEVELOPER)
+        expect(json_response['override']).to eq(false)
       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" }
+    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(:created)
 
-    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 }
+        put api("/groups/#{group.id}/members/#{ldap_developer.id}", owner),
+            params: { access_level: Member::MAINTAINER }
 
-        expect do
-          post api(url, owner), params: params
+        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
 
-          expect(response).to have_gitlab_http_status(:created)
-        end.to change { AuditEvent.count }.by(1)
+      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
+
+    describe 'POST /groups/:id/members/:user_id/override' do
+      it 'succeeds when override is set on an LDAP user' do
+        post api("/groups/#{group.id}/members/#{ldap_developer.id}/override", owner)
 
-      it 'does not create audit event if creating a new member fails' do
-        params = { user_id: 0, access_level: Member::DEVELOPER }
+        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
 
-        expect do
-          post api(url, owner), params: params
+      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(:not_found)
-        end.not_to change { AuditEvent.count }
+        expect(response).to have_gitlab_http_status(:forbidden)
       end
     end
-  end
 
-  it_behaves_like 'POST /:source_type/:id/members', 'project' do
-    let(:source) { project }
-  end
+    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)
 
-  it_behaves_like 'POST /:source_type/:id/members', 'group' do
-    let(:source) { group }
+        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
+
+      it 'returns 403 when override is set for a non-ldap user' do
+        delete api("/groups/#{group.id}/members/#{developer.id}/override", owner)
+
+        expect(response).to have_gitlab_http_status(:forbidden)
+      end
+    end
   end
 end

lib/api/members.rb

@@ -160,3 +160,5 @@ module API
     end
   end
 end
+
+API::Members.prepend_if_ee('EE::API::Members')