From 2b941be6239db1df8f5db17b8eebcb82930ddc1d Mon Sep 17 00:00:00 2001 From: Matt Date: Wed, 30 Sep 2026 10:42:58 +0200 Subject: [PATCH 1/2] fix(permissions): reload the users list on an unknown id or a denied access [PRD-1404] A user or service account created after the agent started was unknown to it until the users cache expired (15 min by default), since nothing reloaded the list on an unknown id. Without SSE (the default outside production), or when the refresh-users event is lost, every call it made got a 403 meanwhile. - get_user_data reloads the users list once when the id is absent. - can? reloads the users on a denial, not only the collections permissions, so a role change applies right away. - Both reloads share a throttle: at most one per minute per process. Co-Authored-By: Claude Opus 5.5 --- .../services/permissions.rb | 48 ++++++++++--- .../services/permissions_spec.rb | 71 +++++++++++++++++++ 2 files changed, 111 insertions(+), 8 deletions(-) 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..77be1f1c5 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. 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 From ef28c4d26e9a628105470c68423f528bc76f2756 Mon Sep 17 00:00:00 2001 From: Matt Date: Wed, 30 Sep 2026 12:13:16 +0200 Subject: [PATCH 2/2] fix(permissions): reload the caller when a read denial refetches the collections [PRD-1404] A user moved to a role that reads a related collection kept its fields redacted until the cache expired: the refetch reused the user loaded before it. Co-Authored-By: Claude Opus 5.5 --- .../lib/forest_admin_agent/services/permissions.rb | 1 + .../security/related_read_permissions_spec.rb | 13 +++++++++++++ 2 files changed, 14 insertions(+) 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 77be1f1c5..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 @@ -325,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([])