Though the patch does a few things, the primary motivation for this issue is that the Edit link on monitor page is visible to people even without access. So classifying this issue as a bug. Otherwise switching sensorlist to use standard entity operations, and standardise entity and route access.

The attached patch also does a few extra things as a result.

  • Switches SensorList (/admin/reports/monitoring) to use operations from listbuilder. Same strategy as other parts of core, including Views.
  • Switch column to use 'Operations' phrasing.
  • Switching to Operations also added Delete button as a side effect. Access is correctly deferred to SensorConfigAccessControlHandler
  • Implemented hook_entity_operation to retain existing SensorList functions where Details and Force Run are buttons, and located before Edit button.
  • Modified lines switches deprecated usage: urlInfo->toUrl, array() -> short syntax, etc.
  • Switch access control edit and delete routes for sensor config to use entity access instead of hard coded permission. (makes it easier to override)
  • Changed SensorConfigAccessControlHandler adding additional docs and removing deprecated usage.
  • Added 'force run' to SensorConfigAccessControlHandler retaining existing access by permission.
  • Changed 'view' in SensorConfigAccessControlHandler to return neutral instead of forbidden to allow overriding. Its not a true forbidden. Correctly cache by permission not by user.

To summarise: Existing functionality is mostly the same, apart from 'view' access op returning neutral instead of forbidden. Delete button visible on list.

CommentFileSizeAuthor
#10 interdiff-3040171-access-control-9-10.txt601 bytesdpi
#10 3040171-access-control-10.patch7.2 KBdpi
#9 3040171-access-control-9.patch7.13 KBdpi
#9 interdiff-3040171-access-control-8-9.txt710 bytesdpi
#8 3040171-8.patch7.07 KBjibran
#6 3040171-access-control-6.patch7.77 KBdpi
#6 interdiff-3040171-access-control-2-6.txt2.75 KBdpi
#2 3040171-access-control.patch6 KBdpi

Issue fork monitoring-3040171

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

dpi created an issue. See original summary.

dpi’s picture

dpi
Location Perth, Australia
commented
StatusFileSize
new 3040171-access-control.patch6 KB
dpi’s picture

dpi
Location Perth, Australia
commented
Status: Active » Needs review
dpi’s picture

dpi
Location Perth, Australia
commented
Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 2: 3040171-access-control.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

dpi’s picture

dpi
Location Perth, Australia
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-3040171-access-control-2-6.txt2.75 KB
new 3040171-access-control-6.patch7.77 KB

Tweaks to retain fallback to administer behaviour

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work 
  1. +++ b/monitoring.module
@@ -4,14 +4,45 @@
+/**
+ * Implements hook_entity_operation().
+ */
+function monitoring_entity_operation(EntityInterface $entity) {
+  if (!$entity instanceof SensorConfigInterface) {
+    return [];
+  }
+

    Lets add this to getDefaultOperations() in SensorListBuilder instead, we don't need to add a hook for own entity type. 

  2. +++ b/src/Controller/SensorList.php
@@ -107,30 +108,19 @@ class SensorList extends ControllerBase {
 
-        \Drupal::moduleHandler()->alter('monitoring_sensor_links', $links, $sensor_config);
+        // @todo deprecate, same can be achieved with \hook_entity_operation().
+        \Drupal::moduleHandler()->alter('monitoring_sensor_links', $operations, $sensor_config);

    Fine we deprecating that right now, we can use alterDeprecated for it. 

  3. +++ b/src/SensorConfigAccessControlHandler.php
@@ -26,17 +27,27 @@ class SensorConfigAccessControlHandler extends EntityAccessControlHandler {
+
+      // We're testing against permission, so add vary by permission.
+      $cacheability->addCacheContexts(['user.permissions']);
+      if ($account->hasPermission('monitoring reports')) {
+        return AccessResult::allowed();

    you don't actually add $cacheability to the returned access result here.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs work » Needs review
StatusFileSize
new 3040171-8.patch7.07 KB

Reroll + #7

dpi’s picture

dpi
Location Perth, Australia
commented
StatusFileSize
new interdiff-3040171-access-control-8-9.txt710 bytes
new 3040171-access-control-9.patch7.13 KB

Minor fix to prevent many errors emitted by misuse of alterDeprecated

dpi’s picture

dpi
Location Perth, Australia
commented
StatusFileSize
new 3040171-access-control-10.patch7.2 KB
new interdiff-3040171-access-control-9-10.txt601 bytes

Fixes missing doc and test failure.

dpi opened merge request !1

dpi’s picture

dpi
Location Perth, Australia
commented

Created MR :)

acbramley’s picture

acbramley commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

MR needs rebasing.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review
Issue tags: -Needs reroll

Rebased.