The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Problem/Motivation
I have the following media bundles and they are checked in that order...:
- Custom 1 (forbidden)
- Custom 2 (forbidden)
- Custom 3 (forbidden)
- Image (Allowed)
- Video (Allowed)
- File (Allowed)
The generic button Add media should show up when the a user has access to create at least one of the bundles in the media. Because of how my bundles are ordered, as well as how the code in Drupal\Core\Entity\EntityCreateAnyAccessCheck::access() is implemented, the result in the end is forbidden and the button Add media does not show when it should.
Steps to reproduce
- Create a 2 bundles of a specific entity type (first_bundle & last_bundle)
- Implement
hook_ENTITY_TYPE_create_access()for the specific entity type and returnAccessResult::forbidden()for last_bundle - The /entity_type/add page is loaded and only allows to create "first_bundle" items
- Modify the hook above to return
AccessResult::forbidden()for first_bundle - The add page shows Access Denied, even if it is possible to create last_bundle entities and is possible to create them if you access directly to /entity_type/add/last_bundle
Proposed resolution
Tweaks the implementation to implement what is mentioned in the comments in the following piece of code:
// Check whether an entity of any bundle may be created.
foreach ($bundles as $bundle) {
$access = $access->orIf($access_control_handler->createAccess($bundle, $account, [], TRUE));
// In case there is a least one bundle user can create entities for,
// access is allowed.
if ($access->isAllowed()) {
break;
}
}
The first iteration get's a forbidden passed to the OR in here is causing the overall result to be Forbidden.
What I think is needed here is to aggregate the whole set of access permissions including their cache metadata. Breaking early might cause caheability metadata leaks, as the rest of the allowed access checks are fully ignored.
We can have the break changed, so it will build the whole set of access results and aggregate them (in terms of cache metadata).
Then have an Allow / Forbidden / Neutral based on tuning on the method...
Remaining tasks
Discussion, Patch...
User interface changes
None expected.
API changes
Small internal change to have correct access checking on entity level.
Data model changes
None.
Release notes snippet
TBD.
We will release a bit more actions on the system, as the bug is restricting the operations.
Possibly some more permissions will be alowed.
| Comment | File | Size | Author |
|---|---|---|---|
| #20 | 3048760-19-test-only.patch | 2.53 KB | seanb |
| #19 | 3048760-19.patch | 4.05 KB | upchuk |
| #16 | 3048760-16.patch | 3.58 KB | upchuk |
| #16 | 3048760-16-test-only.patch | 2.52 KB | upchuk |
| #9 | 3048760-9.patch | 1020 bytes | hchonov |
Comments
Comment #2
ndobromirov commentedThis implements the proposal form the description.
No tests are updated, but this is a good candidate for having test coverage added.
Needs review to trigger the CI builds on the patch...
Comment #3
ndobromirov commentedUpdated IS to reflect the problem better.
Comment #4
ndobromirov commentedI have trouble finding any test code for this file in Core. Even though it's used on I think all entity types :(.
Comment #5
ndobromirov commentedComment #6
valthebaldFrom the issue description, it looks like it should happen for all (content) entities, not only media. Have you checked, just in case?
Comment #7
ndobromirov commentedNo time to check my issue is only with media, but yes, it will likely fail on other entities as well.
I do not see how other types will differ if they are using this code.
You are free to update the IS if there is a reproducible case in other entity types.
Comment #8
berdirI think I saw another issue before on the forbidden problem.
However, I don't think we also need to change the return early, that is pretty important on sites with a lot of bundles (unless you're the unlucky guy to only have access to the last of many bundles). We might not have all the cacheablity metadata, but we have enough to know that we can allow access and cacheability metdata for render/dynamic page cache is self-learning over time.
We did add quite a bit of test coverage for that access check, so shouldn't be too hard to extend it to not just have neutral but also explicitly forbidden access.
Comment #9
hchonov@Berdir, I agree with this and here is a patch for that. We still need a test.
Comment #10
ndobromirov commentedLatest patch in #9 is better. Hiding mine from #2.
Comment #16
upchuk commentedI added test coverage for this and improved a bit on #10. Not sure why you ran still an orIf between the existing access and the bundle access. We just need to return allowed or neutral and inherit the cacheability from each iteration on whatever we return (essentially collect cache metadata).
Comment #19
upchuk commentedForgot about the access reason it seems.
Comment #20
seanbPatch seems to do the trick for me. Attached is a test-only patch from #19 that should prove the bug exists and that it is indeed fixed by the patch in #19.
Comment #21
seanbJust noticed #16 already contained the same test-only patch. Sorry about that. In that case I think this might be ready. Not sure I'm the best person to RTBC this, so I'll leave the status for now.
Also #3039629: Bug:"_entity_create_any_access" error using orIf method is probably a duplicate of this issue.
Comment #23
seanbComment #25
rp7 commentedI can confirm that the patch in #19 fixes this issue. Thank you!
Comment #27
smustgrave commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
I tried replicating this in D10.1.x by adding a custom media type
Giving the content editor role permission to create just image media types.
The admin media button appears fine.
Could use additional steps to show this issue.
Comment #28
plopesc@smustgrave
I have been able to reproduce the bug and can confirm that patch in #19 works
Steps to reproduce:
hook_ENTITY_TYPE_create_access()for the specific entity type and returnAccessResult::forbidden()for last_bundleAccessResult::forbidden()for first_bundleThe bundle name is an important point in this issue, because bundles are loaded alphabetically, and the bug only happens if the forbidden bundle comes first.
Comment #29
bkosborneSigh, ran into this myself after lots of time debugging. Glad there's some movement on this and others find it buggy. I think this affects anyone that is using hook_create_access to forbid access to specific bundles of an entity type.
Comment #30
plopescComment #31
bkosborneThis works well for me
Comment #37
catchAttempting a more descriptive title.
Committed/pushed to 11.x and cherry-picked back through to 9.5.x, thanks!