Make private file access handling respect the full entity reference chain

https://security.drupal.org/node/162111

This support can be public with support of the security team.

Re-tagging.

Thanks @solide-echt for reporting this.

We should add an automated test for this. There's also a couple todos in the patch (plus minor coding standards things).

I potentially filed a dupe of this as #2937642: Access to files attached via media entities should be ultimately controlled by the published state of related content - Thanks @marcoscano for letting me know.

Could someone please confirm/deny?

I'd say your issue is a duplicate yes, but we have a considerable dependency chain here to resolve until we can get there.

+++ b/core/modules/media/src/MediaAccessControlHandler.php
@@ -23,10 +26,68 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
+            $query = db_select('file_usage', 'fu')
+              ->fields('fu', ['id'])
+              ->condition('fu.fid', $file->id());
+            $ids = $query->execute()->fetchCol();
+
+            /**
+             * by default set access forbidden for private files
+             */
+            $result = AccessResult::forbidden();
 
+            /**
+             * get all possible reference field for media
+             **/
+            $bundle_fields = [];
+            $contentTypes = \Drupal::service('entity.manager')->getStorage('node_type')->loadMultiple();
+            foreach ($contentTypes as $contentType) {
+              $bundle_fields[$contentType->id()] = \Drupal::getContainer()->get('entity_field.manager')->getFieldDefinitions('node', $contentType->id());
+            }
+            $media_fields = [];
+            foreach ($bundle_fields as $bundle => $field) {
+              foreach ($field as $config) {
+                if ( $config->getSetting('target_type') === 'media' ) {
+                  $media_fields[] = $config->getName();
+                }

That's not how it works.

File usage is the entity that has the file attached. That means the media entity (*this* entity), not the node.

To be able to solve this, we first need to understand who uses the media entity. That means that this is blocked on #2835840: Track media usage and present it to the site builder (in the media library, media view, on media deletion confirmation, etc.) which in turn is blocked on moving entity_usage into core.

Yes, sorry, I overlooked that. Would actually not be too complicated to make that more generic with \Drupal\Core\Entity\EntityFieldManager::getFieldMapByFieldType() but it doesn't scale IMHO. Would not be very efficient if each media access check would involve the file usage query, N entity queries and then an access check for each matching entity. Might work as a site specific solution that has limited fields/content, but not generically in core.

@Berdir: Yes that may be true. Then perhaps it should be handled as a submodule or could perhaps be enabled per media entity via checkbox to make it more granular? Or (which might be a nice idea) happen if the file field of the media entity is set to private? Because then it would make sense to inherit the permission behaviour?

I like the last idea...

IMHO the fact that there is no native entity usage mechanism in core, together with the inherent re-usable nature of media entities, make this problem space too complex to be solved directly in core in a generic way.

I've started exploring ideas to solve this in contrib first, where sites could decide how to handle access restrictions on a per media type basis, depending on what makes sense for each use case. Feedback on the general approach would be very welcome: https://github.com/marcoscano/media_private_access/pull/2/files

@marcoscano, thank you very much, I'll definitely have a look at that!

Did you read my comment #11:

Or (which might be a nice idea) happen if the file field of the media entity is set to private? Because then it would make sense to inherit the permission behaviour?

The "private" functionality simply doesn't make much sense without the permission handling. What do you think about basing the access behaivour on this?
A separate module as proof of concept is a great idea.

I think there are two separate things:
1) Detecting / configuring when media view access should be done differently from what we have by default
2) Performing the real access check in a way that makes sense for that asset.

Your suggestion would help in the identification of when it's necessary (1), but we would still need to decide how to handle the access check itself. Should it always inherit from its immediate parent? That would require some underlying system that Drupal core doesn't have nowadays (that's why this issue is postponed). But maybe that's not even necessary for some sites, where a permission-specific access control could be enough, or deciding the access based on the top-level route, for example.

I actually think as a good idea the possibility of showing a message to the user whenever a private file field is added to a media entity, indicating that they need to go somewhere and configure something else, in order for the private access to work as they expect it to.

Completely agree with you in #14. +1

I actually think core could directly have such warning.

This may end up being a little bit intrusive on sites that do have some alternative solutions in place, but I believe it's a worth annoyance to have, if it prevents site builders on simpler sites exposing their private files inadvertently.

As long as we don't have a solution for this, it would be helpful to at least show a message so users will know what to expect. So +1 from me.

1. +++ b/core/modules/media/media.module
   @@ -336,3 +336,27 @@ function media_preprocess_media_reference_help(&$variables) {
   +    \Drupal::messenger()->addWarning(t("You defined a private scheme on a media field. Note that this will inherit access permissions from the media entity itself. If that is not your expectation, make sure you have additional contrib or custom modules allowing you to have access control inherited from the media entity's parents as well."));
   

   I think the part pointing to contrib or custom solutions is not helpful at the moment and maybe more confusing. We should probably just state what the behaviour is (we don't have a good contrib or custom solution at the moment anyway). Also we should make more explicit that you don't inherit permissions from content using a media item. Maybe something like this:
   You defined a private scheme on a media field. Note that this will inherit access permissions from a media item, not from the content using the media item.

2. +++ b/core/modules/media/media.module
   @@ -336,3 +336,27 @@ function media_preprocess_media_reference_help(&$variables) {
   +  // any user that can access the media entity, not its parent.
   

   We are using 'media item' instead of 'media entity' in the module documentation.

Can you also add a test for this?

Thanks @seanB!

I agree we shouldn't point to something that is not core, and the simpler the better.

I agree we should have a test here, but I'll wait until we have some more consensus before writing it... :)

Thanks!

Briefly discussed this with @Gabor Hotjsy, writing down the summary of our conversation before I forget, will work on the patch later:

- It is a good idea to warn users that they might not be having what they expect just by setting the field to use a private scheme
- We should have a config flag somewhere that module developers could use to programmatically disable this warning
- We should show the warning on the status page as well
- We should have the warning message as part of the form UI, being shown every time there's a private field there (and the flag hasn't been disabled)
- We should point to a documentation page on drupal.org, where possible solutions and recommendations to make the assets private could be listed
- We need to re-word the text, not use "You.." etc.

OK, let's see what the opinions on this are.

- Created a placeholder documentation page for this at https://www.drupal.org/docs/8/core/modules/media/setting-up-private-acce... , we still need to figure out what to explain there, point to contrib solutions, etc.
- The warning is shown in both the status page and in the manage fields page, as shown in the screenshots below
- Devs can disable the warning with drush by executing drush cset media.settings show_private_scheme_warning 0, and re-enable it back the same way by setting the flag to 1
- Final wording might still need some tweaking
- We still need tests for this, but I was hoping to get a 👍 or 👎 on the approach before writing them
- I'd be happy to move this to a different issue if we consider this one should be kept for the real problem (postponed until core cannot provide an inherited access control for referenced entities)

Warning on the fields page:

Warning on the status page:

(Haven't read the issue yet; will go back and do that shortly.)

Reading the message in the screenshot, my first thought is: "Huh? Why? Why would media items not inherit access control from their referencing entity? That seems the most sensible default." Especially given that File fields and Image fields respect it..?

I have a feeling it's because of some architectural reason like the media can be referenced from multiple entities, which, ok. But then that seems like a thing to have a contrib workaround for, not "secure by default."

Because I believe to a content author, who uses the node entry form daily and has no understanding of Drupal's underpinnings, that's just a field on a data entry form like any other field. And if content entered in text, radio button, select, etc. fields is access controlled, the expectation is that content entered into file upload fields would be as well.

But in any case, they're certainly not going to have access to admin/reports/status to learn differently. So if we're going to handle this with a message instead of changing the behaviour (which I would seriously caution against doing; it is security for non-technical people we're talking about, after all), seems like we might need to do so in a different place(s). For example, at the point a site builder is choosing private files as a destination on the Media field, and again to content authors when uploading files to content that's access controlled.

Here's a free issue summary update while I'm reading...

Also, as a side-note... for a first core patch, this one is quite a doozy! Great job, @solide-echt! :D

Ok, I'm now caught up. I do think we should split this messaging proposal off to its own issue, because I anticipate it may be tricky to get right, and there was a proposed fix, even if not a 100% core-viable one, prior to this direction change. Else, we could also re-purpose this one for messaging and split off a second one for the fix-fix (maybe we could re-open #2937642: Access to files attached via media entities should be ultimately controlled by the published state of related content for this). But at any rate, this underlying problem is unfortunately not remotely "fixed" just because we added some extra messaging, so the issue title + discussion should reflect that.

And finally, (I swear this is my last comment and then you can all go back to Dev Days-ing :D) it would be really helpful to have steps to reproduce in order to try and discern what messaging would work best and where. I'm not sure if we can reasonably do that with a security issue, but nevertheless...

Thanks @webchick, that's great feedback!

Yes, I agree 100% that these are two separate things and the messaging doesn't replace the real problem in any way. I've opened #2984093: Inform users that media items don't inherit access control from parents to discuss only the message, and let's keep this issue for the real fix. Sorry for derailing the conversation, let's pretend comments from #16 to #20 never happened :)

Also adjusted the IS accordingly.

(@solide-echt, since it's been quite a while, I'm unassigning the issue, but please feel free to assign it back to you if you are working on it again.)

Thanks!

This is an interesting effort. I don't think anyone has brought this up yet, but there's a problem I see with an approach like this:

Media has been designed around the idea of it being an independent entity. Media entities can be created independently from anything that may immediately reference them. It does not need to be referenced by anything to be useful to a site builder. For example, someone could upload many PDFs into a "Document" media bundle, and simply link to those PDFs directly from an email. How would a site builder define access in that case?

I think any solution around tracking where a media entity is used will be too brittle and complex.

I can't offer a more elegant solution, but I just wanted to voice that concern.

I'm working on a custom access control solution for media entities based on presenting a list of user roles on the media entity form where a user selects what roles should have access.

I thought about this and I think we can learn from the PHPCR / JCR efforts here, where the document model is not relational, but tree shaped:

In that model you could never access a media item that is part of the tree if you don’t have access to the Root — unless it is also referenced somewhere else.

We can probably also learn from the service_container, which has the notion of private services — a private service can still be used by other services — it however can not be gotten from the container directly.

In essence - I think we need to first split things up properly before fixing this issue is easy:

- Public files are attached to public entities
- Private files are attached to private entities

Therefore I would propose to introduce the notion of private entities:

- They are not accessible directly unless you have a very high override permission
- They are not appearing in base table listings — unless a special tag is added (for joins it will be more complicated obviously but this is a 90% case solution)
- Access to the private entity can be “sponsored” by any other entity — editing would always be in the context (yes literal context as in URL context) of this other entity, eg by accessing the parent entity, it dynamically grants access to it’s children / references - this would work recursively.

There are two ways to implement private entities:

- As a trait that would add a ‘private’ field / property / entity key to the base table definition.

- As it’s own entity type that is always private.

I think it would be better to have private entities be dynamic and just add a policy that once private it cannot be made public again. (And maybe attaching a private file would make the entity private by default?).

In any case for entities supporting private, the access control would be changed to need this permission per entity type and would also remove those entities from queries and views by adding a condition that it is NOT private.

I think with this necessary abstraction added this would be simpler to implement.

The difference from a who-references-this approach is that the parent entity sponsors the access.

This would also fit naturally into something like an entity_access_grants (similar to node_access) scheme.

My 2c

#32: Yes this concept totally makes sense. We can bikeshed if "private" is the best term. For me something like "dependent" gets the idea better.
In the end, it's the same concept that paragraphs are to their host entities, isn't it?

Note that you cannot make a file field's storage schema dynamic in Drupal 8. When you add a file field to an entity, you must decide then and there if the file storage for the file is public or private. That means if we had the concept of a "private" toggle for a media entity, then the files would actually always need to be stored in private file system.

#34: Yes, sigh.

Also we must not only think of view access. We might have public view access but want edit access restricted by the reference chain.
(I tried to hack this for group access in groupmediaplus.module, but this turns out to be a PITA and will hopefully be obsoleted by this some day.)

I don't quite follow #32 yet to be honest. The whole reason that private file access is so hard is that we *do* need to resolve backwards. We get a request to system/files/private/filename.pdf, we need to match that first against a file entity, then figure out where that is used and based on that, if the user has access.

I guess what is being proposed could be done by changing that up completely and building a URl like system/files/private/node:12/filename.pdf, or a query argument, and then you already know the context. But that more or less just moves the problem to where we build the URL, which might make it slightly easier, but there are still plenty of fun things, e.g. old/forward revisions, also would private files inside paragraphs or blocks placed through layout builder reference their top-level parent or themself, and if parent, where would we get that information from (backwards reference problem again), we also can't just rely on the current entity from the URL, as you might be looking at a node, but display a bunch of private file links through views from different entities in the sidebar.

> Note that you cannot make a file field's storage schema dynamic in Drupal 8. When you add a file field to an entity, you must decide then and there if the file storage for the file is public or private. That means if we had the concept of a "private" toggle for a media entity, then the files would actually always need to be stored in private file system.

That should't be too hard to solve. For starters, that the scheme is an immutable field storage setting is completely unnecessary, it just affects new files uploaded through the widget, you can reference whatever file entity you want, for example through entity browser. And media entities have a status, having logic that moves files between private and public if their default revision status changes wouldn't be that hard to implement. Seems like the least of our problems in relation to private files :) It's then IMHO again much harder again to know whether a user can view an unpublished media entity or not in a generic way.

Hey folks, landed here by discovering far too late that my content model using [host node[host media[file]]] does not protect the actual files when permissions are set on the host node. This is quite problematic for us.

I actually have just a practical question for a workaround. It sounds like we could potentially just apply the same node access permissions to the Media entity based on the host entity's permissions and those permissions would then be inherited by the file. I think I would implement this in an entity_save hook. It's messy but we're in a pinch right now.

Hi, @brooke_heaton. If you're just looking for a quick fix in a pinch, https://www.drupal.org/project/media_private_access or https://github.com/amcgowanca/media_status_by_node might help.

RE: #37, I suppose in your use case, the media items are not referenced in multiple places? Because if they were it would not work.

@TravisCarden - I took media_private_access for a spin but it did not restrict access to filefields on media. Even if the media was protected and the node host of the media was protected, no settings would protect the file attached to media. The direct link to the file was always available. :(

@brooke_heaton please see media_private_access's issue queue for support with that project. :)

Added blocker info from #8 to issue summary (per d.o issue summary guidelines).

So here is a real scenario. I have a content type with a paragraph builder, and within that, I've created a paragraph for a media item. The content type is configured with permission by term module, which limits access the content based on what the users define.

If I'm following this correctly, even if I set all my media storage locations to be served through the private file system, access will still be granted to anon users and logged in users that are not within the role that has access to that particular parent node?

Correct. And doing that automatically is extremely hard. As your example shows, there are several layers of references involved that need to be looked up. I'd strongly suggest to try and handle access control for your medias on the media level. I don't know if the permission by term module only supports nodes, but most likely the easier and more performant solution would be to expand it to medias as well, so they have direct access control as well. Manually or automatically.

@Berdir So far so good! Enabling "Permissions by term", and it's submodule "Permissions by Entity" correctly handles our access needs.

Re #49

@Berdir So far so good! Enabling "Permissions by term", and it's submodule "Permissions by Entity" correctly handles our access needs.

that is a good trick until this is solved! Should we perhaps add this as a workaround or alternative solution to the issue summary? Perhaps that information could also be added to the module page of the proof of concept module?

This seems to be one of the few ways to explicitly set permissions on certain media entities to make the behaviour explicit?

Please also note and help to fix this issue, if you're using this workaround: #3222563: Permissions by Entity affected by cache - returning wrong access result until cache is cleared