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
Comments
Comment #2
paul121 commentedComment #3
m.stentaI 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).
Should we do this in the
3.xbranch 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...
Comment #4
paul121 commentedGreat, the easy approach!
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 quantityhard-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
Comment #5
m.stentaMerged into my
3.xbranch. Thanks @paul121! Great CR too!Comment #6
m.stenta