1) Login as a user with "administer image styles" permission
2) Go to /admin/config/media/image-styles/manage/media_library/delete
3) Submit form
Expected. You should not be able to do this.
Actual: "The image style Media Library thumbnail (220x220) has been deleted."
It works when not using the UI, see MediaLibraryAccessTest::testMediaLibraryImageStyleAccess()
The problem is the superuser bypasses access, because image_style routes are using permission checks instead of entity access checks.
entity.image_style.delete_form:
path: '/admin/config/media/image-styles/manage/{image_style}/delete'
defaults:
_entity_form: 'image_style.delete'
_title: 'Delete'
requirements:
_permission: 'administer image styles'
This then skips the entity access hook in the media_library module.
/**
* Implements hook_ENTITY_TYPE_access().
*/
function media_library_image_style_access(EntityInterface $entity, $operation, AccountInterface $account) {
// Prevent the fallback 'media_library' image style from being deleted.
// @todo: Lock the image style instead of preventing delete access.
// https://www.drupal.org/project/drupal/issues/2247293
if ($operation === 'delete' && $entity->id() === 'media_library') {
return AccessResult::forbidden();
}
}
TBD
TBD
TBD
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | Screenshot 2023-07-11 at 11.21.31 AM.png | 81.2 KB | keshavv |
| #11 | 3079235-11.patch | 3.62 KB | acbramley |
| #11 | 3079235-11-failing-test.patch | 3.1 KB | acbramley |
| #9 | CleanShot 2023-07-10 at 20.27.48.png | 173.4 KB | amjad1233 |
| #9 | CleanShot 2023-07-10 at 20.28.12.png | 12.46 KB | amjad1233 |
Comments
Comment #2
oknateComment #9
amjad1233I couldn't find a button on the UI to delete the task. But when I navigated to `***/delete` manually, it did delete the task. However, two observations were made:
1. Even after deletion, it asked for a "Replacement Image Style". Why couldn't it be removed in the first place?
2. There's a very slim chance someone can delete the task outside of the UI.
- In any case, it does not sound like a bug. (in my opinion)
Comment #10
acbramley commentedI actually think this is a bug, and it's not just super users that can bypass this access check.
The problem is that all image style entity routes don't have
_entity_accessrequirements, rather they're just checking for a permission.Since user 1 has all permissions, it gets access, but also any role with
administer image styleswould have accessComment #11
acbramley commentedHere's a red/green combo (providing the fix doesn't break anything else) to prove it.
Moved the test from Kernel -> Functional so we have one place to do API based and UI based access checks.
Comment #13
keshavv commentedI have applied the #11 pass patch and confirmed that it is working perfectly.
Comment #15
lauriiiCommitted af3a8ef and pushed to 11.x. Thanks!
Comment #16
acbramley commentedOh wow didn't expect that, thanks! I wonder if we should do follow ups for the other image style routes? Edit and create both use the permission as well as image effects
Comment #17
lauriiiSeems fine to have a follow-up for that. I was thinking of asking for those changes here but thought it would be nice to land this for delete route first because it had some pre-existing tests in Media Library.
Comment #18
acbramley commentedDefinitely makes sense, opened #3374258: Image style routes use _permission requirement instead of _entity_access