Problem/Motivation

Users that can create media but not edit them are able to change authoring information.
This is not ideal if you want to allow anonymous users to create media.

Steps to reproduce

I've given "anonymous" and "authenticated" the following permissions:

  1. 'create news content' so that they can submit news articles (nodes) without logging in
  2. 'create image media' so that they can submit images to attach to their news articles

At /node/add/news, an anonymous user does not get access to change the authoring information.

At /media/add/image, an anonymous user does get access to change the authoring information, and can change the "authored by" from anonymous to anyone they want, and the same with "authored on" (see attached image).

Proposed resolution

We should allow editing these fields only the user has the permission to edit media.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-2973447

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

George Bills created an issue. See original summary.

George Bills’s picture

The following patch seems to work for me - it's a one liner to hide the authoring information behind the "administer media" permission.

cilefen’s picture

Status: Active » Needs review

Status: Needs review » Needs work
dhirendra.mishra’s picture

Status: Needs work » Needs review
StatusFileSize
new700 bytes

I am attaching patch below.kindly test and review it.

Status: Needs review » Needs work

The last submitted patch, 5: core_media_author-2973447-5.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

George Bills’s picture

Maybe I'm missing something dhirendra.mishra, but how does MediaForm get $currentUser set?

MediaForm extends ContentEntityForm, ContentEntityForm extends EntityForm, EntityForm extends FormBase, FormBase has a protected function currentUser() that just does a return \Drupal::currentUser();. So my first patch just calls to currentUser() and checks perms that way.

NodeForm uses a currentUser property, but that gets injected into its constructor, MediaForm doesn't have that property. So I don't think your patch can work, since your $this->currentUser will always be null.

The "proper" thing to do is probably to inject the current user in the constructor but I was trying to keep the diff as small as possible.

George Bills’s picture

Reattaching the patch without the trailing whitespace on the last (not applied) line, since your autotester thingy refuses to apply with that whitespace there.

This is the "old" way calling to $this->currentUser() function, coding standards might require using an injected $this->currentUser variable but as above I wanted to keep the diff small. ContentEntityForm that we inherit from calls out to $this->currentUser() a few times already anyway.

George Bills’s picture

George Bills’s picture

George Bills’s picture

Status: Needs work » Needs review
chr.fritsch’s picture

Status: Needs review » Needs work

I think that makes sense. Could we get a test for that?

George Bills’s picture

New patch with a test included. @chr.fritsch please give feedback on if the test meets standards - I tried to match what looked like the convention the original tests all use. Maybe it'd be nice to merge the "test an admin user" section with my new "test a normal user with the 'administer media' permission" section but I didn't want to change things too much.

George Bills’s picture

Status: Needs work » Needs review
chr.fritsch’s picture

Status: Needs review » Needs work

Thank you @George Bills. This looks great.

I only found one nitpick:

+++ b/core/modules/media/tests/src/Functional/MediaAccessTest.php
@@ -154,6 +155,15 @@ public function testMediaAccess() {
+    $role = Role::load(RoleInterface::AUTHENTICATED_ID);

This line is not needed.

Could you also please upload a fail patch (a patch that only contains the new test)? That will prove the fix.

George Bills’s picture

Thanks @chr.fritsch, new patches attached.

chr.fritsch’s picture

Patches should end on *.patch. *.txt will not be picked up by the testbot.

@George Bills, could you please re-upload your files with the correct file extensions.

nitebreed’s picture

chr.fritsch’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @Nitebreed.

This looks good to me now.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I'm not sure that this is the best fix. Looking at the node entity I think we should improve the field access to determine access and not only do this on the form level. See \Drupal\node\NodeAccessControlHandler::checkFieldAccess().

    // Only users with the administer nodes permission can edit administrative
    // fields.
    $administrative_fields = ['uid', 'status', 'created', 'promote', 'sticky'];
    if ($operation == 'edit' && in_array($field_definition->getName(), $administrative_fields, TRUE)) {
      return AccessResult::allowedIfHasPermission($account, 'administer nodes');
    }
chr.fritsch’s picture

Status: Needs work » Needs review
StatusFileSize
new2.92 KB
new1.82 KB

Here is a new patch that moves the access check to the field level.

seanb’s picture

+++ b/core/modules/media/src/MediaAccessControlHandler.php
@@ -83,4 +85,18 @@ protected function checkCreateAccess(AccountInterface $account, array $context,
+    $administrative_fields = ['uid', 'status', 'created'];

Do we really need users to have 'administer media' for them to publish/unpublish media? I know node is doing that, but just not sure if we should copy it.

Berdir pointed out that the default status could be 'Unpublished' and there could be some kind of workflow where you don't want users to publish the nodes, but I'm not sure how valid that argument is for media.

Removing the author and created field is probably fine for most sites, but removing the published checkbox for some users on existing sites is probably not.

phenaproxima’s picture

Priority: Normal » Major
Issue tags: +Media Initiative

Tagging and escalating.

seanb’s picture

Version: 8.5.3 » 8.7.x-dev
Status: Needs review » Needs work
Issue tags: +Needs tests

Back to NW for the status field. I think we are also missing an interface test where users unpublish media via the interface. I would have at least expected some tests to fail after making a change like this.

wim leers’s picture

Do we really need users to have 'administer media' for them to publish/unpublish media? I know node is doing that, but just not sure if we should copy it.

I'd also question this.

webchick’s picture

Looking at https://api.drupal.org/api/drupal/core%21modules%21media%21media.permiss... it looks like there's an "Update own media" permission, so maybe attach published/unpublished to that, if we're indeed talking only about personal media here? (You definitely don't want random weirdos unpublishing other peoples' media; though there's also an "Update any media" for that.)

seanb’s picture

Linking the publishing permissions to edit sounds very reasonable. If you can change the value of the source field and the name, most of the times you would expect to be able to publish / unpublish as well. As long as we don't have more granular permissions in core, this is probably the best we can do.

wim leers’s picture

If you can change the value of the source field and the name, most of the times you would expect to be able to publish / unpublish as well.

This sounds reasonable to me.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

vsujeetkumar’s picture

Status: Needs work » Needs review
StatusFileSize
new3.71 KB
new3.4 KB

@seanB I have added test according to #25, Can you please check and advise these tests are up to the mark.

samiullah’s picture

@vsujeetkumar, Looks good

@seanB This needs code review as well for tests before we move this one to RTBC

seanb’s picture

Status: Needs review » Needs work

The suggestion from #25 to use update media / update any media instead of administer media still needs to be implemented. Setting to needs for for that.

Also

+++ b/core/modules/media/tests/src/Functional/MediaAccessTest.php
@@ -190,6 +199,15 @@ public function testMediaAccess() {
+    // Test the 'administer media' permission on a non-admin account with unpublished media.

This line is longer than 80 chars.

vsujeetkumar’s picture

Status: Needs work » Needs review
StatusFileSize
new3.69 KB
new3.1 KB

@seanB I have done with the changes mentioned in #35, Please have a look and advise.

Status: Needs review » Needs work

The last submitted patch, 36: 2973447_36.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

samiullah’s picture

@vsjujeetkumar tests are failing, please recheck

seanb’s picture

+++ b/core/modules/media/src/MediaAccessControlHandler.php
@@ -100,4 +102,18 @@ protected function checkCreateAccess(AccountInterface $account, array $context,
+      return AccessResult::allowedIfHasPermission($account, 'update media');

The update media permission is only for media owners/authors, so we should also check if the user is the owner.

The update any media permission is still missing.

BTW, in #2862422: Add per-media type creation permissions for media we added per media type permissions and the generic permissions will be deprecated in #2925459: Deprecate generic media permissions.

You should probably take a look at MediaAccessControlHandler::checkAccess().

And then specifically this part:

        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);
        }
        // @todo Deprecate this permission in
        // https://www.drupal.org/project/drupal/issues/2925459.
        if ($account->hasPermission('update any media')) {
          return AccessResult::allowed()->cachePerPermissions();
        }
        if ($account->hasPermission('update media') && $is_owner) {
          return AccessResult::allowed()->cachePerPermissions()->cachePerUser()->addCacheableDependency($entity);
        }

Another thing is also that the caching is probably not correct for the code we currently have, so you also need to take a look at #2998824: MediaAccessControlHandler update/delete access caching is not correct which adds some changes to fix the caching.

phenaproxima’s picture

samiullah’s picture

Needs updated patch for retesting
Needs fix

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

sakthivel m’s picture

StatusFileSize
new3.7 KB

#44 Fixed whitespace errors and re created patch

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

prudloff made their first commit to this issue’s fork.

prudloff changed the visibility of the branch 2973447-anonymous-users-can to hidden.

prudloff’s picture

Instead of re-implementing half the logic from MediaAccessControlHandler::checkAccess(), I think we can simply call this method.

smustgrave’s picture

Status: Needs review » Needs work

Since this is altering the access (all be it seems correct) think it needs a CR as it is altering behavior.

smustgrave’s picture

Status: Needs work » Reviewed & tested by the community

Felt bad so I went ahead did the CR, super simple one

godotislate’s picture

Status: Reviewed & tested by the community » Needs work

Since the author field is hidden from the form to users who don't have edit access, I think there should be test assertions that the UID is still set to that creator on save of new media entity.