diff --git a/CHANGELOG.md b/CHANGELOG.md index f05826f93d..41391e30b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,14 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - Nothing should go in this section, please add to the latest unreleased version (and update the corresponding date), or add a new version. +## [1.19.2] - 2022-01-13 + +### Fixed +- Previously, including `limit` or `offset` parameters to a resource list request + resulted in the returned list being unexpectedly sorted. Now, all resource list + request results are sorted by resource ID. + [cyberark/conjur#2702](https://github.com/cyberark/conjur/pull/2702) + ## [1.19.1] - 2022-12-08 ### Security diff --git a/app/models/resource.rb b/app/models/resource.rb index ed5cfa420a..f6b403266c 100644 --- a/app/models/resource.rb +++ b/app/models/resource.rb @@ -104,6 +104,9 @@ def search account: nil, kind: nil, owner: nil, offset: nil, limit: nil, search: # Filter by string search scope = scope.textsearch(search) if search + # Sort results alphabetically by resource ID + scope = scope.order(:resource_id) + if offset || limit # 'limit' must be an integer greater than 0 if given if limit && (!numeric?(limit) || limit.to_i <= 0) @@ -114,7 +117,7 @@ def search account: nil, kind: nil, owner: nil, offset: nil, limit: nil, search: raise ArgumentError, "'offset' contains an invalid value. 'offset' must be an integer greater than or equal to 0." end - scope = scope.order(:resource_id).limit( + scope = scope.limit( (limit || 10).to_i, (offset || 0).to_i ) diff --git a/spec/controllers/resources_controller_spec.rb b/spec/controllers/resources_controller_spec.rb index 78b77d4bdf..290870b853 100644 --- a/spec/controllers/resources_controller_spec.rb +++ b/spec/controllers/resources_controller_spec.rb @@ -34,59 +34,116 @@ { 'HTTP_AUTHORIZATION' => token_auth_str } end - def list_resources_without_limit() - get(resources_url, env: token_auth_header()) + def list_resources(limit: nil, offset: nil, count: false) + params = {} + params.merge!({ :limit => limit }) if limit + params.merge!({ :offset => offset }) if offset + params.merge!({ :count => count }) if count + get(resources_url, env: token_auth_header, params: params) end - def list_resources_with_limit(limit) - get(resources_url, env: token_auth_header(), params: {:limit => limit}) - end - - def count_resources_without_limit() - get(resources_url, env: token_auth_header(), params: {:count => "true"}) - end - - def count_resources_with_limit(limit) - get(resources_url, env: token_auth_header(), params: {:limit => limit, :count => "true"}) + def count_resources(limit: nil) + list_resources(limit: limit, count: true) end def load_variables() - payload = '[!variable preexisting\n!variable preexisting1]' + payload = '[!variable a, !variable b, !variable c, !variable d, !host a, !host b, !host c, !host d, !layer a, !layer b, !layer c]' put(policies_url, env: token_auth_header.merge({ 'RAW_POST_DATA' => payload })) end context 'with default configuration' do - it "should list all resources when limit not defined" do - list_resources_without_limit() - expect(response.code).to eq("200") - expect(JSON.parse(response.body).size).to eq(2) + context 'with no query params defined' do + before(:each) do + list_resources() + @resources = JSON.parse(response.body) + end + + it 'should return a 200 status code' do + expect(response.code).to eq("200") + end + + it 'should list all resources' do + expect(@resources.size).to eq(12) + end + + it 'should order resources alphabetically by resource id' do + @resources.each_with_index do |resource, idx| + next if idx == 0 + expect(resource["id"]).to be > @resources[idx-1]["id"] + end + end end - it "should list resources according to query param limit when limit defined" do - list_resources_with_limit(1) - expect(response.code).to eq("200") - expect(JSON.parse(response.body).size).to eq(1) + context 'with limit query param defined' do + before(:each) do + list_resources(limit: 5) + @resources = JSON.parse(response.body) + end + + it 'should return a 200 status code' do + expect(response.code).to eq("200") + end + + it 'should list resources according to the provided limit' do + expect(@resources.size).to eq(5) + end + + it 'should order resources alphabetically by resource id' do + @resources.each_with_index do |resource, idx| + next if idx == 0 + expect(resource["id"]).to be > @resources[idx-1]["id"] + end + end + end + + context 'with offset query param defined' do + before(:each) do + list_resources(offset: 1) + @resources = JSON.parse(response.body) + end + + it 'should return a 200 status code' do + expect(response.code).to eq("200") + end + + it 'should offset resources according to the provided offset' do + list_resources() + all_resources = JSON.parse(response.body) + + expect(@resources[0]).to eq(all_resources[1]) + end + + it 'should limit list to 10 resources when offset defined and limit not defined' do + expect(@resources.size).to eq(10) + end + + it 'should order resources alphabetically by resource id' do + @resources.each_with_index do |resource, idx| + next if idx == 0 + expect(resource["id"]).to be > @resources[idx-1]["id"] + end + end end end context 'with custom configuration' do it "should list resources according to custom configuration when limit not defined" do Rails.application.config.conjur_config.api_resource_list_limit_max = 1 - list_resources_without_limit() + list_resources() expect(response.code).to eq("200") expect(JSON.parse(response.body).size).to eq(1) end it "should list resources according to query param limit when custom configuration exceeds limit" do Rails.application.config.conjur_config.api_resource_list_limit_max = 2 - list_resources_with_limit(1) + list_resources(limit: 1) expect(response.code).to eq("200") expect(JSON.parse(response.body).size).to eq(1) end it "should throw error when limit exceeds custom configuration" do Rails.application.config.conjur_config.api_resource_list_limit_max = 1 - list_resources_with_limit(2) + list_resources(limit: 2) expect(response.code).to eq("422") end end @@ -94,22 +151,22 @@ def load_variables() context 'when validating count request' do it "should count all resources when custom configuration defined" do Rails.application.config.conjur_config.api_resource_list_limit_max = 1 - count_resources_without_limit() + count_resources() expect(response.code).to eq("200") - expect(response.body).to eq("{\"count\":2}") + expect(response.body).to eq("{\"count\":12}") end it "should count all resources when custom configuration not defined" do - count_resources_without_limit() + count_resources() expect(response.code).to eq("200") - expect(response.body).to eq("{\"count\":2}") + expect(response.body).to eq("{\"count\":12}") end # There is a currently a bug in the API when supplying both the `limit` # and `count` parameters. A count response shouldn't be affected by # the `limit` parameter. This should be changed when the bug is fixed (ONYX-22079) it "should count resources according to query param limit " do - count_resources_with_limit(1) + count_resources(limit: 1) expect(response.body).to eq("{\"count\":1}") end end