Contrib.social

Incorrect logic for 'edit-own' and 'delete-own' permissions

Open on Drupal.org →
Created on 28 July 2023, over 1 year ago
Updated 4 January 2024, 11 months ago

Problem/Motivation

In MicroContentAccessHandler.php, ‘own’ content is determined by $microcontent->getRevisionUserId()

case 'update':
        return AccessResult::allowedIfHasPermission($account, sprintf('update any %s microcontent', $microcontent->bundle()))
          ->orIf(AccessResult::allowedIf($account->hasPermission(sprintf('update own %s microcontent', $microcontent->bundle())) && $microcontent->getRevisionUserId() === $account->id())
            ->cachePerPermissions()
            ->cachePerUser());

Use of the revision owner user id here is wrong. Or, if intentional, it’s not how Drupal core and contrib modules generally work - they determine “own” content based on the author/owner value of the entity. Example for core media module in MediaAccessControlHandler.php

$is_owner = ($account->id() && $account->id() === $entity->getOwnerId());

// ... later ...

  case 'update':
    if ($account->hasPermission('edit any ' . $type . ' media')) {
      return AccessResult::allowed()->cachePerPermissions();
    }
    if ($account->hasPermission('edit own ' . $type . ' media') && $is_owner) {
      return AccessResult::allowed()->cachePerPermissions()->cachePerUser()->addCacheableDependency($entity);

Impact:

If a user’s edit and delete permissions are limited to edit-own and delete-own, they lose that access if an admin (etc) edits the item, because then the revision author would become that person.

Proposed resolution

Use $microcontent->getOwnerId() instead of $microcontent->getRevisionUserId()

Remaining tasks

I've built a patch, to be submitted shortly.

🐛 Bug report
Status

Fixed

Version

1.0

Component

Code

Created by

🇬🇧United Kingdom andy inman Gloucestershire, UK

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @andy inman
  • Merge request !4Issue #3377831: Incorrect logic for 'edit-own' and 'delete-own' permissions → (Merged) created by andy inman
  • Open on Drupal.org →
    Core: 10.0.7 + Environment: PHP 7.3 & MySQL 8
    last update over 1 year ago
    Not currently mergeable.
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.0.7 + Environment: PHP 7.3 & MySQL 8
    last update over 1 year ago
    Composer error. Unable to continue.
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.0.7 + Environment: PHP 7.3 & MySQL 8
    last update over 1 year ago
    Composer error. Unable to continue.
  • Comment over 1 year ago →
    🇬🇧United Kingdom andy inman Gloucestershire, UK

    I have learned: don't right-click the Open MR button hoping to check the URL - that will trigger the button action.

  • Open in Jenkins → Open on Drupal.org →
    Core: 10.1.x + Environment: PHP 8.1 & MariaDB 10.3.22
    last update over 1 year ago
    5 pass, 1 fail
  • Comment over 1 year ago →
    🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    This change looks reasonable to me.
    Thanks for the MR.

    Will merge if tests pass

  • Status changed to Needs work over 1 year ago
  • Comment over 1 year ago →
    🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Looks like there's a failing test, likely needs updating to match new logic

  • Open in Jenkins → Open on Drupal.org →
    Core: 10.1.x + Environment: PHP 8.1 & MariaDB 10.3.22
    last update over 1 year ago
    6 pass
  • Comment over 1 year ago →
    🇬🇧United Kingdom andy inman Gloucestershire, UK

    Looks like there's a failing test, likely needs updating to match new logic

    Confirmed - access tests were checking edit-own and delete-own permission based on revision uid rather than entity uid. I've changed that. I've also added a couple of tests cases to check update and delete when user has the edit/delete-own permission but is not the owner.

    Before marking as RTBC, I think it's worth at least considering the possibility that somebody out there may have an access control strategy that would be broken by these changes. That seems very unlikely to me, but I suppose it's not impossible. In the worst case, people could get edit/delete access to micro-content that they're not supposed to be able to change. There's no simple solution - I'm just suggesting a pause for thought - maybe wait for a bit more import from others.

  • Comment over 1 year ago →
    🇬🇧United Kingdom andy inman Gloucestershire, UK

    Looks like there's a failing test, likely needs updating to match new logic

    Confirmed - access tests were checking edit-own and delete-own permission based on revision uid rather than entity uid. I've changed that. I've also added a couple of tests cases to check update and delete when user has the edit/delete-own permission but is not the owner.

    Before marking as RTBC, I think it's worth at least considering the possibility that somebody out there may have an access control strategy that would be broken by these changes. That seems very unlikely to me, but I suppose it's not impossible. In the worst case, people could get edit/delete access to micro-content that they're not supposed to be able to change. There's no simple solution - I'm just suggesting a pause for thought - maybe wait for a bit more input from others.

  • Comment 11 months ago →
    🇦🇺Australia acbramley

    acbramley → changed the visibility of the branch 8.x-1.x to hidden.

  • Status changed to RTBC 11 months ago
  • Comment 11 months ago →
    🇦🇺Australia acbramley

    Thanks very much @Andy Inman for the thorough fix and test coverage. I agree this is wrong (and I wrote it originally).

    Your points in #7 are valid, but I think as long as we spell it out in the release notes we're good hopefully!

    Will merge and release on green.

  • Comment 11 months ago →
    🇦🇺Australia sime Melbourne

    Yeah the comparison to Media access checking is a good one - patch works for me.

  • Comment 11 months ago →
    System Message
  • Status changed to Fixed 11 months ago
  • Comment 11 months ago →
    🇦🇺Australia acbramley

    Available in 8.x-1.6

  • Comment 11 months ago →
    System Message

    Automatically closed - issue fixed for 2 weeks with no activity.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024