From ce01300afe41e6c431821c317428d9d9875e9f77 Mon Sep 17 00:00:00 2001 From: Kevin Jerinsky Date: Thu, 11 Nov 2021 09:56:54 -0500 Subject: [PATCH 1/2] Add distinct to pg function for accessible_objects --- CHANGELOG.md | 3 +++ lib/migrations/accessible_objects_for_operations_function.rb | 2 +- lib/policy_machine/version.rb | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d191dab..f51b325 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,8 @@ # Changelog +## 4.0.1 +* Updated PostgreSQL function `pm_accessible_objects_for_operations` to ensure uniqueness of results + ## 4.0.0 * Added a PostgreSQL function for `#accessible_objects` and `#accessible_objects_for_operations` which are performance optimized. Only supported for a single `field`, `direct_only`, and `ignore_prohibitions`. diff --git a/lib/migrations/accessible_objects_for_operations_function.rb b/lib/migrations/accessible_objects_for_operations_function.rb index c47f935..7c81c17 100644 --- a/lib/migrations/accessible_objects_for_operations_function.rb +++ b/lib/migrations/accessible_objects_for_operations_function.rb @@ -117,7 +117,7 @@ def up RETURN QUERY EXECUTE format( - 'SELECT os.unique_identifier, array_agg(pe.%I) AS objects ' || + 'SELECT os.unique_identifier, array_agg(DISTINCT pe.%I) AS objects ' || 'FROM ' || ' t_operation_set_ids os_id ' || ' JOIN t_operation_sets os ON os.operation_set_id = os_id.operation_set_id ' || diff --git a/lib/policy_machine/version.rb b/lib/policy_machine/version.rb index c8e2c9f..1e2c9c0 100644 --- a/lib/policy_machine/version.rb +++ b/lib/policy_machine/version.rb @@ -1,3 +1,3 @@ class PolicyMachine - VERSION = "4.0.0" + VERSION = "4.0.1" end From 2d275e2ffa21262e26e85d2ee29f790e52e0ceb1 Mon Sep 17 00:00:00 2001 From: Kevin Jerinsky Date: Thu, 11 Nov 2021 10:17:53 -0500 Subject: [PATCH 2/2] add specs for uniqueness --- .../active_record_spec.rb | 150 ++++++++++-------- 1 file changed, 81 insertions(+), 69 deletions(-) diff --git a/spec/policy_machine_storage_adapters/active_record_spec.rb b/spec/policy_machine_storage_adapters/active_record_spec.rb index d3009f2..9e3a9e7 100644 --- a/spec/policy_machine_storage_adapters/active_record_spec.rb +++ b/spec/policy_machine_storage_adapters/active_record_spec.rb @@ -742,6 +742,11 @@ end describe 'accessible_objects_for_operations_function' do + before do + priv_pm.add_association(color_1, creator, object_6) + priv_pm.add_association(color_2, creator, object_7) + end + it 'uses a PostgreSQL function' do expect_any_instance_of(PolicyMachineStorageAdapter::ActiveRecord) .to receive(:accessible_objects_for_operations_function) @@ -755,94 +760,101 @@ ) end - context 'when there are directly accessible objects' do - before do - priv_pm.add_association(color_1, creator, object_6) - priv_pm.add_association(color_2, creator, object_7) - end + it 'returns objects accessible via each of multiple given operations' do + result = priv_pm.accessible_objects_for_operations( + user_1, + [create, paint], + direct_only: true, + ignore_prohibitions: true, + fields: [:unique_identifier] + ) - it 'returns objects accessible via each of multiple given operations' do - result = priv_pm.accessible_objects_for_operations( - user_1, - [create, paint], - direct_only: true, - ignore_prohibitions: true, - fields: [:unique_identifier] - ) + expect(result.keys).to contain_exactly(create.to_s, paint.to_s) + expect(result[create.to_s]).to contain_exactly('object_7', 'object_6') + expect(result[paint.to_s]).to contain_exactly('object_7', 'object_6') + end - expect(result.keys).to contain_exactly(create.to_s, paint.to_s) - expect(result[create.to_s]).to contain_exactly('object_7', 'object_6') - expect(result[paint.to_s]).to contain_exactly('object_7', 'object_6') - end + it 'can handle string operations' do + result = priv_pm.accessible_objects_for_operations( + user_1, + ['create', 'paint'], + direct_only: true, + ignore_prohibitions: true, + fields: [:unique_identifier] + ) - it 'can handle string operations' do + expect(result.keys).to contain_exactly('create', 'paint') + expect(result[create.to_s]).to contain_exactly('object_7', 'object_6') + expect(result[paint.to_s]).to contain_exactly('object_7', 'object_6') + end + + it 'returns an empty list of objects for non-existent operations' do + result = priv_pm.accessible_objects_for_operations( + user_1, + [create, 'zagnut'], + direct_only: true, + ignore_prohibitions: true, + fields: [:unique_identifier] + ) + + expect(result.keys).to contain_exactly(create.to_s, 'zagnut') + expect(result[create.to_s]).to contain_exactly('object_7', 'object_6') + expect(result['zagnut']).to eq([]) + end + + it 'returns a unique list of objects for each operation' do + result = priv_pm.accessible_objects_for_operations( + user_1, + [create, paint], + direct_only: true, + ignore_prohibitions: true, + fields: [:type] + ) + + expect(result.keys).to contain_exactly('create', 'paint') + expect(result[create.to_s]).to contain_exactly('PolicyMachineStorageAdapter::ActiveRecord::Object') + expect(result[paint.to_s]).to contain_exactly('PolicyMachineStorageAdapter::ActiveRecord::Object') + end + + context 'filters' do + it 'works for a single filter' do result = priv_pm.accessible_objects_for_operations( user_1, - ['create', 'paint'], + [create, paint], + filters: { + user_attributes: { color: color_1.color } + }, direct_only: true, ignore_prohibitions: true, fields: [:unique_identifier] ) - expect(result.keys).to contain_exactly('create', 'paint') - expect(result[create.to_s]).to contain_exactly('object_7', 'object_6') - expect(result[paint.to_s]).to contain_exactly('object_7', 'object_6') + expect(result).to eq({ + create.to_s => ['object_6'], + paint.to_s => ['object_6'], + }) end - it 'returns an empty list of objects for non-existent operations' do + it 'works for multiple filters' do result = priv_pm.accessible_objects_for_operations( user_1, - [create, 'zagnut'], + [create, paint], + filters: { + user_attributes: { + color: color_1.color, + unique_identifier: color_1.unique_identifier, + id: color_1.id + } + }, direct_only: true, ignore_prohibitions: true, fields: [:unique_identifier] ) - expect(result.keys).to contain_exactly(create.to_s, 'zagnut') - expect(result[create.to_s]).to contain_exactly('object_7', 'object_6') - expect(result['zagnut']).to eq([]) - end - - context 'filters' do - it 'works for a single filter' do - result = priv_pm.accessible_objects_for_operations( - user_1, - [create, paint], - filters: { - user_attributes: { color: color_1.color } - }, - direct_only: true, - ignore_prohibitions: true, - fields: [:unique_identifier] - ) - - expect(result).to eq({ - create.to_s => ['object_6'], - paint.to_s => ['object_6'], - }) - end - - it 'works for multiple filters' do - result = priv_pm.accessible_objects_for_operations( - user_1, - [create, paint], - filters: { - user_attributes: { - color: color_1.color, - unique_identifier: color_1.unique_identifier, - id: color_1.id - } - }, - direct_only: true, - ignore_prohibitions: true, - fields: [:unique_identifier] - ) - - expect(result).to eq({ - create.to_s => ['object_6'], - paint.to_s => ['object_6'], - }) - end + expect(result).to eq({ + create.to_s => ['object_6'], + paint.to_s => ['object_6'], + }) end end end