diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb b/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb index e4c9ce5b9..04e249779 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb @@ -9,6 +9,10 @@ class Permissions include ForestAdminDatasourceToolkit::Exceptions include ForestAdminDatasourceToolkit::Components::Query::ConditionTree + USERS_RELOAD_INTERVAL_IN_SECONDS = 60 + + @users_reload_mutex = Mutex.new + attr_reader :caller, :forest_api, :cache def initialize(caller) @@ -28,13 +32,26 @@ def self.invalidate_cache(id_cache = nil) Facades::Container.config_from_cache[:permission_expiration] ) - cache.clear if id_cache.nil? + if id_cache.nil? + cache.clear + @users_reload_mutex.synchronize { @users_reloaded_at = nil } + end cache.delete(id_cache) unless cache.get(id_cache).nil? ForestAdminAgent::Facades::Container.logger.log('Info', "Invalidating #{id_cache} cache..") end + def self.users_reload_allowed? + @users_reload_mutex.synchronize do + now = Process.clock_gettime(Process::CLOCK_MONOTONIC) + return false if @users_reloaded_at && now - @users_reloaded_at < USERS_RELOAD_INTERVAL_IN_SECONDS + + @users_reloaded_at = now + true + end + end + def can?(action, collection, allow_fetch: false) return true unless permission_system? @@ -45,6 +62,7 @@ def can?(action, collection, allow_fetch: false) unless is_allowed collections_data = get_collections_permissions_data(force_fetch: true) + user_data = get_user_data(caller.id, reload: true) is_allowed = permission_allowed?(collections_data, collection, action, user_data) end @@ -246,7 +264,22 @@ def get_segments(collection, force_fetch: false) permissions[:segments][collection.name.to_sym] end - def get_user_data(user_id) + def get_user_data(user_id, reload: false) + users = cached_users + return users[user_id.to_s] if users.key?(user_id.to_s) && !reload + + reload_users[user_id.to_s] + end + + def get_team(rendering_id) + permissions = get_rendering_data(rendering_id) + + permissions[:team] + end + + private + + def cached_users cache.get_or_set('forest.users') do response = fetch('/liana/v4/permissions/users') users = {} @@ -258,17 +291,16 @@ def get_user_data(user_id) ForestAdminAgent::Facades::Container.logger.log('Debug', 'Refreshing user permissions cache') users - end[user_id.to_s] + end end - def get_team(rendering_id) - permissions = get_rendering_data(rendering_id) + def reload_users + return cached_users unless self.class.users_reload_allowed? - permissions[:team] + self.class.invalidate_cache('forest.users') + cached_users end - private - # An empty list of leaves resolves to no collection at all — a polymorphic relation declaring # no `foreign_collections`. `[].all?` would allow it unconditionally, which is the one answer # this guard must never give by default, so it counts as denied. @@ -293,6 +325,7 @@ def fetch_read_permissions(names) @read_permissions_refetched = true refetched = get_collections_permissions_data(force_fetch: true) + user_data = get_user_data(caller.id, reload: true) names.to_h { |name| [name, read_allowed?(refetched, name, user_data)] } end diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb index f65ae1d2f..ce94362f3 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb @@ -594,6 +594,19 @@ def with_instant_cache_refresh(enabled) expect(permissions).to have_received(:get_collections_permissions_data).with(force_fetch: true).once end + it 'reloads the caller on a denial so a role changed since the last load reads at once' do + with_instant_cache_refresh(false) + permissions = build_permissions([]) + allow(permissions).to receive(:get_collections_permissions_data).and_return( + { cards: { read: [7, 8] }, accounts: { read: [8] } } + ) + allow(permissions).to receive(:get_user_data).with(anything, reload: true).and_return({ id: 1, roleId: 8 }) + + expect(permissions.read_permissions('cards', %w[accounts])).to eq( + { 'cards' => true, 'accounts' => true } + ) + end + it 'refetches once for the whole request when the payload does not know a collection' do with_instant_cache_refresh(true) permissions = build_permissions([]) diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/services/permissions_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/services/permissions_spec.rb index c96778272..050bc7721 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/services/permissions_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/services/permissions_spec.rb @@ -210,6 +210,77 @@ module Services end end + context 'when the users list may be stale' do + let(:john) do + { 'id' => 1, 'firstName' => 'John', 'lastName' => 'Doe', 'fullName' => 'John Doe', + 'email' => 'john.doe@domain.com', 'tags' => [], 'roleId' => 1, 'permissionLevel' => 'user' } + end + let(:admin) do + { 'id' => 3, 'firstName' => 'Admin', 'lastName' => 'test', 'fullName' => 'Admin test', + 'email' => 'admin@forestadmin.com', 'tags' => [], 'roleId' => 13, 'permissionLevel' => 'admin' } + end + + def users_response(*users) + instance_double(Faraday::Response, status: 200, body: users.to_json) + end + + def stub_users(*responses) + allow(forest_api_requester).to receive(:get).with('/liana/v4/permissions/users').and_return(*responses) + end + + it 'reloads the users once when the id is unknown' do + stub_users(users_response(admin), users_response(admin, john)) + + expect(@permissions.get_user_data(1)).to include(id: 1, roleId: 1) + expect(forest_api_requester).to have_received(:get).with('/liana/v4/permissions/users').twice + end + + it 'does not reload the users when the id is known' do + stub_users(users_response(admin, john)) + + expect(@permissions.get_user_data(1)).to include(id: 1, roleId: 1) + expect(forest_api_requester).to have_received(:get).with('/liana/v4/permissions/users').once + end + + it 'reloads the users at most once per interval for an id that stays unknown' do + stub_users(users_response(admin)) + + 2.times { expect(@permissions.get_user_data(42)).to be_nil } + + expect(forest_api_requester).to have_received(:get).with('/liana/v4/permissions/users').twice + end + + it 'reloads the users again once the interval has elapsed' do + stub_users(users_response(admin)) + allow(Process).to receive(:clock_gettime).and_call_original + allow(Process).to receive(:clock_gettime).with(Process::CLOCK_MONOTONIC).and_return( + 1_000.0, + 1_000.0 + described_class::USERS_RELOAD_INTERVAL_IN_SECONDS + 1 + ) + + 2.times { @permissions.get_user_data(42) } + + expect(forest_api_requester).to have_received(:get).with('/liana/v4/permissions/users').exactly(3).times + end + + it 'reloads the users on a denial so a role granted since the last load applies' do + stub_users(users_response(john.merge('roleId' => 99)), users_response(john)) + + expect(@permissions.can?(:browse, @datasource.collections['Book'])).to be true + expect(forest_api_requester).to have_received(:get).with('/liana/v4/permissions/users').twice + end + + it 'denies without reloading the users when a reload already ran within the interval' do + stub_users(users_response(john.merge('roleId' => 99)), users_response(john)) + described_class.users_reload_allowed? + + expect do + @permissions.can?(:browse, @datasource.collections['Book']) + end.to raise_error(ForbiddenError) + expect(forest_api_requester).to have_received(:get).with('/liana/v4/permissions/users').once + end + end + context 'when can? is called' do it 'returns true when user is allowed' do expect(@permissions.can?(:browse, @datasource.collections['Book'])).to be true