fix(permissions): reload the users list on an unknown id or a denied access [PRD-1404]

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

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([])

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