Contrib.social

Managed roles assume bundle permissions for quantities

Open on Drupal.org →
Created on 13 October 2023, about 1 year ago
Updated 30 October 2023, about 1 year ago

Problem/Motivation

Our current managed role permission logic assumes that quantities use bundle permissions, but in fact they do not. This does not cause any errors right now but it seems that it could cause issues in Drupal 10. When using a quantity bundle permission like this `create standard quantity` in a role that is used in tests I get this warning message:

1x: Adding non-existent permissions to a role is deprecated in drupal:9.3.0 and triggers a runtime exception before drupal:10.0.0. The incorrect permissions are "create standard quantity". Permissions should be defined in a permissions.yml file or a permission callback. See https://www.drupal.org/node/3193348 →

I discovered this when working with some custom permissions for rothamsted and using the following in a managed role config entity. This allows a user to see all entities, but only create observation logs and quantities:

third_party_settings:
  farm_role:
    access:
      config: false
      entity:
        view all: true
        create all: false
        update all: false
        delete all: false
        type:
          log:
            create:
              - observation
            update own:
              - all
            delete own:
              - all
          quantity:
            create:
              - all
            update own:
              - all
            delete own:
              - all

While debugging custom access control logic I was having issues getting quantities to work as expected. I ended up hard coding create standard quantity in the role I was testing with and discovered the above error message in tests.

The managed role permission manager is where the logic is hard-coded to assume bundle permissions for quantities: https://github.com/farmOS/farmOS/blob/2.x/modules/core/role/src/ManagedR... Interestingly this all is working fine because the access control handler for drupal\entity module *always* checks for either the bundle & non-bundle permission variants:


  // Drupal\entity\EntityAccessControlHandlerBase

  protected function checkEntityPermissions(EntityInterface $entity, $operation, AccountInterface $account) {
    $permissions = [
      "$operation {$entity->getEntityTypeId()}",
      "$operation {$entity->bundle()} {$entity->getEntityTypeId()}",
    ];

    return AccessResult::allowedIfHasPermissions($account, $permissions, 'OR');

In fact, because our managed roles never *actually* have permissions assigned to them (in the sense that Drupal core understands/expects), this might never be an issue in Drupal 10. But regardless I think it is something that we should correct.

If I remember correctly we did intentionally decide to not include bundle permissions for quantities, just for simplicity. If we do want bundle permissions we just need to add this permission_granularity = bundle key/value to the entity definition: https://github.com/farmOS/farmOS/blob/010a895092b30423b5b66ea06aa8f552ce...

Steps to reproduce

Use a quantity bundle permission in a role that is used in tests, observe error message. In Drupal 10 this might be worse.

Proposed resolution

Two options:
1. Fix the managed role permission manager so we do not generate/assign quantity bundle permissions. We may need to change how the third party settings/config works, too?
2. Use bundle permissions for quantities.

In some ways I kinda prefer option 2 because the "fix" is so much easier. But it would be a breaking change to modify the permissions that do currently exist eg: "create quantity" becomes "create standard quantity", etc.

Remaining tasks

Decide on solution, implement.

User interface changes

None

API changes

None

Data model changes

None

🐛 Bug report
Status

Fixed

Version

3.0

Component

Code

Created by

🇺🇸United States paul121 Spokane, WA

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

Comments & Activities

  • Issue created by @paul121
  • Comment about 1 year ago →
    🇺🇸United States paul121 Spokane, WA
  • Comment about 1 year ago →
    🇺🇸United States m.stenta

    2. Use bundle permissions for quantities.

    I can't think of any argument against this. I don't recall whether or not it was an intentional decision not to use bundle permissions when we made quantity types ( #3196825: Quantity types → ).

    But it would be a breaking change to modify the permissions that do currently exist

    Should we do this in the 3.x branch and make it an explicit breaking change with a changelog? See: #3347412: [META] farmOS 3.x Breaking Changes → )

    Are there any other potential ripple effects of changing these permissions? Do we explicitly check for them anywhere ourselves in code? Or will everything "just work"? I'm thinking of the Inline Entity Form widget in log edit forms... but I assume that would all "just work". I'm not sure if there are other places we need to think about...

  • Status changed to Needs review about 1 year ago
  • Comment about 1 year ago →
    🇺🇸United States paul121 Spokane, WA

    I can't think of any argument against this.

    Great, the easy approach!

    Are there any other potential ripple effects of changing these permissions? Do we explicitly check for them anywhere ourselves in code? Or will everything "just work"?

    I don't think there should be any issues in farmOS core. I did a search for the permissions that will be removed and did not see any. I know this will impact Rothamsted because they have a few custom roles that have create quantity hard-coded permissions. Are there any other projects we know like this?

    I made a PR to @m.stenta's 3.x branch here: https://github.com/mstenta/farmOS/pull/3

    I also started a change record for this: https://www.drupal.org/node/3395415 →

  • Status changed to Fixed about 1 year ago
  • Comment about 1 year ago →
    🇺🇸United States m.stenta

    Merged into my 3.x branch. Thanks @paul121! Great CR too!

  • Comment about 1 year ago →
    🇺🇸United States m.stenta
  • Comment about 1 year ago →
    System Message

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

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024