From 2ebeef595ecae9db05413513c31807d366fe3f70 Mon Sep 17 00:00:00 2001
From: Michael Stenta <mike@mstenta.net>
Date: Fri, 1 Oct 2021 12:44:07 -0400
Subject: [PATCH] Require explicit use of accessCheck() on the result of
 farm.log_query service.

---
 docs/development/module/services.md         | 14 +++++++++++---
 modules/asset/group/src/GroupMembership.php |  3 ++-
 modules/core/location/src/AssetLocation.php |  3 ++-
 modules/core/log/src/LogQueryFactory.php    |  4 ----
 4 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/docs/development/module/services.md b/docs/development/module/services.md
index a01989b7..c94c6f5e 100644
--- a/docs/development/module/services.md
+++ b/docs/development/module/services.md
@@ -115,6 +115,16 @@ The log query service is a helper service for building a standard log database
 query. This is primarily used to query for the "latest" log of an asset.
 The asset location and group membership services use this.
 
+Note that you must set specify whether or not you want access checking to be
+performed on the queried logs by running the `accessCheck()` method on the
+query object that is returned. This will determine whether or not logs that
+the current user does not have access to will be filtered out. If you are
+trying to find the "latest" log of an asset for a particular purpose, filtering
+out logs can cause inconsistent results, so typically `accessCheck(FALSE)` is
+necessary. It is the responsibility of the code that uses this service to
+understand the security implications of the data this returns, and perform
+additional access checking if necessary.
+
 **Methods**:
 
 `getQuery($options)` - Builds a log database query.
@@ -129,9 +139,6 @@ It accepts a keyed array of options:
 - `status` (string) - Filter by log status.
 - `asset` (asset entity) - Filter to logs that reference a particular asset.
 - `limit` (int) - Only include this many results.
-- `access_check` (bool) - Whether or not to check user access to logs returned
-  by the query (defaults to `TRUE`, do not set to `FALSE` unless you understand
-  the security implications for your use-case).
 
 **Example usage**:
 
@@ -145,6 +152,7 @@ $options = [
 ];
 $query = \Drupal::service('farm.log_query')->getQuery($options);
 $query->condition('is_movement', TRUE);
+$query->accessCheck(FALSE);
 $log_ids = $query->execute();
 
 // Load the first log.
diff --git a/modules/asset/group/src/GroupMembership.php b/modules/asset/group/src/GroupMembership.php
index 208d610e..c5740c51 100644
--- a/modules/asset/group/src/GroupMembership.php
+++ b/modules/asset/group/src/GroupMembership.php
@@ -114,15 +114,16 @@ class GroupMembership implements GroupMembershipInterface {
     }
 
     // Query for group assignment logs that reference the asset.
+    // We do not check access on the logs to ensure that none are filtered out.
     $options = [
       'asset' => $asset,
       'timestamp' => $this->time->getRequestTime(),
       'status' => 'done',
       'limit' => 1,
-      'access_check' => FALSE,
     ];
     $query = $this->logQueryFactory->getQuery($options);
     $query->condition('is_group_assignment', TRUE);
+    $query->accessCheck(FALSE);
     $log_ids = $query->execute();
 
     // Bail if no logs are found.
diff --git a/modules/core/location/src/AssetLocation.php b/modules/core/location/src/AssetLocation.php
index 107becc9..d25bfa69 100644
--- a/modules/core/location/src/AssetLocation.php
+++ b/modules/core/location/src/AssetLocation.php
@@ -187,15 +187,16 @@ class AssetLocation implements AssetLocationInterface {
     }
 
     // Query for movement logs that reference the asset.
+    // We do not check access on the logs to ensure that none are filtered out.
     $options = [
       'asset' => $asset,
       'timestamp' => $this->time->getRequestTime(),
       'status' => 'done',
       'limit' => 1,
-      'access_check' => FALSE,
     ];
     $query = $this->logQueryFactory->getQuery($options);
     $query->condition('is_movement', TRUE);
+    $query->accessCheck(FALSE);
     $log_ids = $query->execute();
 
     // Bail if no logs are found.
diff --git a/modules/core/log/src/LogQueryFactory.php b/modules/core/log/src/LogQueryFactory.php
index 5c567734..517c1751 100644
--- a/modules/core/log/src/LogQueryFactory.php
+++ b/modules/core/log/src/LogQueryFactory.php
@@ -48,10 +48,6 @@ class LogQueryFactory implements LogQueryFactoryInterface {
     // Add a tag.
     $query->addTag('farm.log_query');
 
-    // Specify access check. By default, only limit to logs the user can view.
-    $access_check = $options['access_check'] ?? TRUE;
-    $query->accessCheck($access_check);
-
     // If a type is specified, only include logs of that type.
     if (isset($options['type'])) {
       $query->condition('type', $options['type']);