Merge branch 'jej/scim-pagination' into 'master'

SCIM pagination

Closes #14805

See merge request gitlab-org/gitlab!14892

doc/api/scim.md

@@ -24,6 +24,11 @@ Parameters:
 |:----------|:--------|:---------|:----------------------------------------------------------------------------------------------------------------------------------------|
 | `filter`   | string  | yes     | A [filter](#available-filters) expression. |
 | `group_path` | string | yes    | Full path to the group. |
+| `startIndex` | integer | no    | The 1-based index indicating where to start returning results from. A value of less than one will be interpreted as 1. |
+| `count` | integer | no    | Desired maximum number of query results. |
+
+NOTE: **Note:**
+Pagination follows the [SCIM spec](https://tools.ietf.org/html/rfc7644#section-3.4.2.4) rather than GitLab pagination as used elsewhere. If records change between requests it is possible for a page to either be missing records that have moved to a different page or repeat records from a previous request.
 
 Example request:
 

ee/app/finders/scim_finder.rb

+# frozen_string_literal: true
+
+class ScimFinder
+  attr_reader :saml_provider
+
+  def initialize(group)
+    @saml_provider = group&.saml_provider
+  end
+
+  def search(params)
+    return Identity.none unless saml_provider&.enabled?
+
+    parsed_hash = EE::Gitlab::Scim::ParamsParser.new(params).result
+    Identity.where_group_saml_uid(saml_provider, parsed_hash[:extern_uid])
+  end
+end

ee/app/models/concerns/scim_paginatable.rb

+# frozen_string_literal: true
+
+module ScimPaginatable
+  extend ActiveSupport::Concern
+
+  class_methods do
+    def scim_paginate(start_index:, count:)
+      one_based_index = [start_index.presence || 1, 1].max
+      zero_based_index = one_based_index - 1
+
+      offset(zero_based_index).limit(count)
+    end
+  end
+end

ee/app/models/ee/identity.rb

@@ -5,6 +5,8 @@ module EE
     extend ActiveSupport::Concern
 
     prepended do
+      include ScimPaginatable
+
       belongs_to :saml_provider
 
       validates :name_id, presence: true, if: :saml_provider
@@ -49,10 +51,14 @@ module EE
         with_extern_uid(provider, extern_uid).take
       end
 
-      def find_by_group_saml_uid(saml_provider, extern_uid)
+      def where_group_saml_uid(saml_provider, extern_uid)
         where(provider: :group_saml,
               saml_provider: saml_provider,
-              extern_uid: extern_uid).take
+              extern_uid: extern_uid)
+      end
+
+      def find_by_group_saml_uid(saml_provider, extern_uid)
+        where_group_saml_uid(saml_provider, extern_uid).take
       end
 
       def preload_saml_group

ee/changelogs/unreleased/jej-scim-pagination.yml

+---
+title: Paginate SCIM responses using count and startIndex
+merge_request: 14892
+author:
+type: added

ee/lib/api/scim.rb

@@ -12,6 +12,8 @@ module API
         requires :group, type: String
       end
 
+      helpers ::EE::API::Helpers::ScimPagination
+
       helpers do
         def logger
           API.logger
@@ -97,12 +99,14 @@ module API
           scim_error!(message: 'Missing filter params') unless params[:filter]
 
           group = find_and_authenticate_group!(params[:group])
-          parsed_hash = EE::Gitlab::Scim::ParamsParser.new(params).result
-          identity = GroupSamlIdentityFinder.find_by_group_and_uid(group: group, uid: parsed_hash[:extern_uid])
+
+          results = ScimFinder.new(group).search(params)
+          response_page = scim_paginate(results)
 
           status 200
 
-          present identity || {}, with: ::EE::Gitlab::Scim::Users
+          result_set = { resources: response_page, total_results: results.count, items_per_page: per_page(params[:count]), start_index: params[:startIndex] }
+          present result_set, with: ::EE::Gitlab::Scim::Users
         end
 
         desc 'Get a SAML user' do

ee/lib/ee/api/helpers/scim_pagination.rb

+# frozen_string_literal: true
+
+module EE
+  module API
+    module Helpers
+      module ScimPagination
+        def scim_paginate(relation)
+          relation.scim_paginate(start_index: params[:startIndex], count: per_page(params[:count]))
+        end
+
+        def per_page(requested_count)
+          requested_limit = requested_count.to_i
+
+          if requested_limit <= 0
+            Kaminari.config.default_per_page
+          else
+            [requested_limit, Kaminari.config.max_per_page].min
+          end
+        end
+      end
+    end
+  end
+end

ee/lib/ee/gitlab/scim/users.rb

@@ -14,28 +14,25 @@ module EE
         private
 
         DEFAULT_SCHEMA = 'urn:ietf:params:scim:api:messages:2.0:ListResponse'
-        ITEMS_PER_PAGE = 20
-        START_INDEX = 1
 
         def schemas
           [DEFAULT_SCHEMA]
         end
 
         def total_results
-          resources.count
+          object[:total_results] || resources.count
         end
 
         def items_per_page
-          ITEMS_PER_PAGE
+          object[:items_per_page] || Kaminari.config.default_per_page
         end
 
         def start_index
-          START_INDEX
+          object[:start_index].presence || 1
         end
 
-        # We only support a single resource at the moment
         def resources
-          [object].select(&:present?)
+          object[:resources] || []
         end
       end
     end

ee/spec/finders/scim_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ScimFinder do
+  let(:group) { create(:group) }
+  let(:unused_params) { double }
+
+  subject(:finder) { described_class.new(group) }
+
+  describe '#search' do
+    context 'without a SAML provider' do
+      it 'returns an empty relation when there is no saml provider' do
+        expect(finder.search(unused_params)).to eq Identity.none
+      end
+    end
+
+    context 'SCIM/SAML is not enabled' do
+      before do
+        create(:saml_provider, group: group, enabled: false)
+      end
+
+      it 'returns an empty relation when SCIM/SAML is not enabled' do
+        expect(finder.search(unused_params)).to eq Identity.none
+      end
+    end
+
+    context 'with SCIM enabled' do
+      let!(:saml_provider) { create(:saml_provider, group: group) }
+
+      context 'with an eq filter' do
+        let!(:identity) { create(:group_saml_identity, saml_provider: saml_provider) }
+
+        it 'allows identity lookup by id/externalId' do
+          expect(finder.search(filter: "id eq #{identity.extern_uid}")).to be_a ActiveRecord::Relation
+          expect(finder.search(filter: "id eq #{identity.extern_uid}").first).to eq identity
+          expect(finder.search(filter: "externalId eq #{identity.extern_uid}").first).to eq identity
+        end
+      end
+    end
+  end
+end

ee/spec/lib/ee/api/helpers/scim_pagination_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ::EE::API::Helpers::ScimPagination do
+  let(:query) { {} }
+  let(:pagination_class) { Struct.new(:params).include(described_class) }
+
+  subject(:paginatable) { pagination_class.new(query) }
+
+  describe '#per_page' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:count, :per_page) do
+      nil    | Kaminari.config.default_per_page
+      ''     | Kaminari.config.default_per_page
+      'abc'  | Kaminari.config.default_per_page
+      0      | Kaminari.config.default_per_page
+      999999 | Kaminari.config.max_per_page
+      4      | 4
+      '4'    | 4
+    end
+
+    with_them do
+      it { expect(subject.per_page(count)).to eq(per_page) }
+    end
+  end
+
+  describe '#scim_paginate' do
+    let(:resource) { Identity.all }
+
+    before do
+      create_list(:group_saml_identity, 3)
+    end
+
+    describe 'without pagination params' do
+      it 'returns all results' do
+        expect(subject.scim_paginate(resource).count).to eq resource.count
+      end
+    end
+
+    describe 'with :count param' do
+      let(:count) { 2 }
+      let(:query) { { count: count } }
+
+      it 'limits results to count' do
+        expect(subject.scim_paginate(resource).count).to eq count
+      end
+    end
+
+    describe 'with :startIndex param' do
+      it 'starts from an offset' do
+        query = { startIndex: Identity.count }
+
+        result = pagination_class.new(query).scim_paginate(resource)
+
+        expect(result.count).to eq(1)
+        expect(result).to eq [resource.last]
+      end
+
+      it 'uses a 1-based index' do
+        query = { startIndex: 1 }
+
+        result = pagination_class.new(query).scim_paginate(resource)
+
+        expect(result.count).to eq(Identity.count)
+      end
+
+      it 'uses 1 when provided an index less than 1' do
+        query = { startIndex: 0 }
+
+        result = pagination_class.new(query).scim_paginate(resource)
+
+        expect(result.count).to eq(Identity.count)
+      end
+    end
+  end
+end

ee/spec/lib/ee/gitlab/scim/users_spec.rb

@@ -7,7 +7,7 @@ describe ::EE::Gitlab::Scim::Users do
   let(:identity) { build(:group_saml_identity, user: user) }
 
   let(:entity) do
-    described_class.new(identity)
+    described_class.new(resources: [identity])
   end
 
   subject { entity.as_json }
@@ -16,15 +16,15 @@ describe ::EE::Gitlab::Scim::Users do
     expect(subject[:schemas]).to eq(['urn:ietf:params:scim:api:messages:2.0:ListResponse'])
   end
 
-  it 'contains the totalResults' do
+  it 'calculates the totalResults' do
     expect(subject[:totalResults]).to eq(1)
   end
 
-  it 'contains the itemsPerPage' do
+  it 'contains the default itemsPerPage' do
     expect(subject[:itemsPerPage]).to eq(20)
   end
 
-  it 'contains the startIndex' do
+  it 'contains the default startIndex' do
     expect(subject[:startIndex]).to eq(1)
   end
 
@@ -35,4 +35,22 @@ describe ::EE::Gitlab::Scim::Users do
   it 'contains the user ID' do
     expect(subject[:Resources].first[:id]).to eq(identity.extern_uid)
   end
+
+  context 'with configured values' do
+    let(:entity) do
+      described_class.new(resources: [identity], total_results: 31, items_per_page: 10, start_index: 30)
+    end
+
+    it 'contains the configured totalResults' do
+      expect(subject[:totalResults]).to eq(31)
+    end
+
+    it 'contains the configured itemsPerPage' do
+      expect(subject[:itemsPerPage]).to eq(10)
+    end
+
+    it 'contains the configured startIndex' do
+      expect(subject[:startIndex]).to eq(30)
+    end
+  end
 end

ee/spec/models/concerns/scim_paginatable_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ScimPaginatable do
+  let(:paginatable_class) { Identity }
+
+  describe 'scim_paginate' do
+    let(:start_index) { 1 }
+    let(:count) { 1 }
+
+    it 'paginates with offset and limit' do
+      expect(paginatable_class).to receive_message_chain(:offset, :limit)
+
+      paginatable_class.scim_paginate(start_index: start_index, count: count)
+    end
+
+    it 'translates a 1-based index to an offset of 0' do
+      expect(paginatable_class).to receive(:offset).with(0).and_return(double(limit: double))
+
+      paginatable_class.scim_paginate(start_index: start_index, count: count)
+    end
+  end
+end

ee/spec/requests/api/scim_spec.rb

@@ -37,6 +37,14 @@ describe API::Scim do
           expect(json_response['Resources']).not_to be_empty
           expect(json_response['totalResults']).to eq(1)
         end
+
+        it 'sets default values as required by the specification' do
+          get scim_api(%{scim/v2/groups/#{group.full_path}/Users?filter=id eq "#{identity.extern_uid}"})
+
+          expect(json_response['schemas']).to eq(['urn:ietf:params:scim:api:messages:2.0:ListResponse'])
+          expect(json_response['itemsPerPage']).to eq(20)
+          expect(json_response['startIndex']).to eq(1)
+        end
       end
 
       context 'no user' do