Integrate Media Library with Content Moderation

Thinking on a 80/20 mindset, I wonder how many sites really have media items "moderatable", instead of having their host entities' revisions pointing to different media items in their workflows.

I personally think most sites will have the host entity being moderated. If that's the case, and considering that this is just a view, maybe these sites can just remove the published field from the view manually?

Trying to solve this automatically could become tricky, once content moderation is set per bundle (media type), but in the view we are showing several types. So the published field could make sense for some media types but not for others, in some scenarios...

To clarify: per #2834729-169: [META] Roadmap to stabilize Media Library, release manager @xjm has indicated this is a must-have.

How is the 80/20 determined?

We are using Content Moderation for our Media Types as our Marketing and Comms department requires strict control of what images get published (danger of getting sued if someone just lifts a stock image off of google for example). This is done separately to the content as usually M&C doesn't have context on what a media item will be used on. They only need to vet and check the image.

Similarly Documents usually need to be moderated before they are approved to be added on some content or shown in the "Approved Media Library" for regulatory and compliance reasons.

One of the pain points right now is that Media are not handling revisions the same way as nodes do. (e.g missing the revisions tab).

#2987911: Make media compatible with content moderation points out some parts of the media system that don't seem to be ready for content-moderation-prime-time yet

Thanks @pavlosdan! Those do sound like issues we should identify here.

To add to #9: The must-have scope of this issue is to identify and file issues for whatever bugs, incompatibilities, etc. we have. Once we do that testing, then we'll go back through the issue list and sort whatever specific issues are identified into must/should/could blockers for the stable release. I imagine the issues in #10 are probably major bugs.

On a Media-related meeting just now @seanB and @phenaproxima said they aren't sure that something even needs to change as far as media_library.module is concerned (this issue is specifically about media_library.module, not about media.module).

@seanB indicated that the view used by the Media Library simply respects the published flag, and that that is what Content Moderation is using to ensure only authorized users are able to see the appropriate Media entities. Hence that should also work in the Media Library.

If this is true — and I sure hope it is! — then it should be easy to write test coverage proving this is the case:

1. Install standard + content_moderation. This also results in core/profiles/standard/config/optional/workflows.workflow.editorial.yml getting installed.
2. Configure the Editorial workflow to apply to image media.
3. Create 3 image media items. Put one in the archived content moderation state, one in draft, one in published.
4. User 1 should be able to see all three when viewing the media library.
5. User 2 (with admin permissions assigned) should be able to see all three when viewing the media library.
6. User 3 (with only View media) should be able to see only the published one when viewing the media library.
7. User 4 (with only View media + View own unpublished media) should be able to see only the published one when viewing the media library.
8. Repeat previous test case, but make all three media items owned by user 4, now they should be able to see all three.

Note: IDK exactly what the difference in effect is between draft and archived. I think that draft means a forward (non-default) revision is created, but Views lists only default revisions of entities. So that'd mean that the draft entity would be visible to anybody with the View media permission. Content Moderation expert should chime in.

If a Content Moderation expert (ideally maintainer) confirms the above testing scenarios are sufficient, then the tests will either show we have no work to do (like @seanB thinks 🌞) or will show exactly which scenarios are failing and hence where the bugs are hiding 🤓

Just wrote the tests for the scenario's of #14. Some remarks.

• The scenario's only apply to admin/content/media, since the widget view is configured to only show published media.
• The tests need #3063216: Inaccessible Media entities still have rows generated for them in the Media Library view, containing form wrapper markup *and* the media label to pass, since without it the bulk form adds the media titles to the page without checking permissions.
• As explained in #3063216-7: Inaccessible Media entities still have rows generated for them in the Media Library view, containing form wrapper markup *and* the media label, we don't have an actual query access API for entities until #777578: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter() is fixed. So even though users don't have permissions to view media items, the view still generates rows for those items.

Attached is a combined patch with #3063216: Inaccessible Media entities still have rows generated for them in the Media Library view, containing form wrapper markup *and* the media label to prove it works, and a patch with just the content moderation test.

For now this is blocked on #3063216: Inaccessible Media entities still have rows generated for them in the Media Library view, containing form wrapper markup *and* the media label.

Ahh, I should have used the latest patch in #3063216: Inaccessible Media entities still have rows generated for them in the Media Library view, containing form wrapper markup *and* the media label. Retry.

#2987911: Make media compatible with content moderation contains a few issues with media generally that should probably be addressed, but as far as I can see these don't really relate to the media library.

In general content moderation manages revisions on two axis: the publishing status and the default/non-default status of a revision. If the user facing views/lists/rendering of media items generally load and use the default revision, then in theory as long as you are respecting your own published/unpublished semantics, you shouldn't need any kind of special integration or consideration for content moderation. In all workflow states, you are either getting a published or unpublished revision, so for the purposes of display unless you are loading non-default revisions you don't need to be concerned by defaultness.

As far as updating revisions, I would assume updates are made via an entity form? Content moderation switches some of the routing upcasting to load the latest revision of an entity for entity forms, but it might be worth asserting that if you create a pending revision via a draft, that reloading or visiting an interface to update media items, the draft content is still loaded as the starting point. I can't really see any scenarios like that, but I'm not sure if that's actually within the scope of the media library?

(Note, 'draft' state content is only placed into a pending revision if there was a previously published revision, so it's not enough to simply create draft content when testing the above)

In terms of actually providing an experience for users of the CMS to view revisions and possibly see a list of in-flight moderated entities, nodes have the following:

• A revision UI, I don't know which other entity types (if any!) have a revisions UI, but there is work to make that generic here: #2350939: Implement a generic revision UI.
• Content moderation ships with a view specially for nodes being moderated, which shows the latest translation affected revision of content and a moderation state filter. Modules like blocks or media could also ship with a view to display moderated entities, but I believe this is effort individual sub-systems should evaluate and undertake with all the relevant steps for introducing new user interfaces.

So in short, #10 is correct, but this isn't necessarily a content moderation integration problem. Even without content moderation, media entities are missing a way to view past revisions.

I have reviewed the test and it looks great. I don't see any issues. If it were checking only visible rows, it would pass and prove that media library respects the accessibility of media based on user permissions. As it's checking using raw assertions, it's blocked on #3063216: Inaccessible Media entities still have rows generated for them in the Media Library view, containing form wrapper markup *and* the media label. I don't have anything new to add other than to saw I reviewed the test and it looks great.

#3063216: Inaccessible Media entities still have rows generated for them in the Media Library view, containing form wrapper markup *and* the media label is about handling media access in general, but for purposes of this issue (integration with Content Moderation), we only need to handle the special case of Published, right?

In which case, can't we solve it in exactly the same way that the /admin/content View does: by adding a filter for "Published status or admin user"? Grep HEAD for status_extra to see the different code pieces needed for that, including the Drupal\node\Plugin\views\filter\Status class. I think for the purposes of this issue, maybe we can just duplicate all that stuff to Media? As a follow-up, maybe we can genericize the code for other entity types to benefit from too?

In which case, can't we solve it in exactly the same way that the /admin/content View does: by adding a filter for "Published status or admin user"? Grep HEAD for status_extra to see the different code pieces needed for that, including the Drupal\node\Plugin\views\filter\Status class. I think for the purposes of this issue, maybe we can just duplicate all that stuff to Media? As a follow-up, maybe we can genericize the code for other entity types to benefit from too?

That would solve it for admin users, but saving an unpublished media item would then only be available for admin users. I guess that could be very confusing, since users could have permissions to view their own unpublished media?
Specially since media also doesn't have a canonical route by default, which means a view is the only way to find and edit existing media.

[non-admin] users could have permissions to view their own unpublished media

The label of the filter is misleading. Although it says "or admin user", what it really means is "or the user has permission to the unpublished media". The query in Drupal\node\Plugin\views\filter\Status includes checks for "view own unpublished content" and "view any unpublished content".

My apologies, that does seems to solve it. I guess this would remove the dependency on the other issues. Thanks!

This implements the 'Published status or admin user' filter for media. It makes the tests from #14 pass and is a much nicer solution compared to #3063216: Inaccessible Media entities still have rows generated for them in the Media Library view, containing form wrapper markup *and* the media label.

For now I skipped an upgrade path for existing media / media_library views. I feel adding this to existing sites should be a site builder decision and could potentially cause rows to be removed unexpectedly. Views are tricky and existing sites could be depending on the existing behaviour.

Ok, this should hopefully fix the tests.

Some minor doc improvements and also updated the content moderation test name and namespace.

Sorry, the last interdiff was not correct, here is the correct interdiff for #30.

My feeling is that we should open a separate issue to talk about the update path; basically, if the product managers think it should be done, we should do it in there.

+1

Assigning to @webchick, this feels like a thing she'd be most opinionated about. Plus, she's already reviewed this issue before.

So, just so I’m understanding… the question being posed is…

“We are making a change to the default view to make it so that you don’t see media you shouldn’t have access to (e.g. unpublished where you do not have perms to see unpublished), and we are wondering whether to retroactively apply this to all existing sites?”

I mean. I think the answer can only be yes? Isn’t it an information disclosure vulnerability otherwise?

And if people are truly depending on broken behaviour, as @seanB frets, they can just remove the filter, no? But we’d still leave all other sites “secure by default”?

So yes, I think I would recommend the spin-off issue, unless I'm missing something here.

+++ b/core/modules/media/src/Plugin/views/filter/Status.php
@@ -0,0 +1,52 @@
+    $snippet = "$table.status = 1 OR ($table.uid = ***CURRENT_USER*** AND ***CURRENT_USER*** <> 0 AND ***VIEW_OWN_UNPUBLISHED_MEDIA*** = 1) OR ***ADMINISTER_MEDIA*** = 1";

If Content Moderation module is enabled, we also need to add a condition for "view any unpublished content", just like the filter in the node namespace does. The reason is because content_moderation_entity_access() checks that permission for all EntityPublishedInterface types, not just nodes.

I also don't think this needs Product Manager review anymore, especially after #36. It's a stale tag from #8.

This isn't a meta issue anymore. #30 is almost a complete fix. Just needs to incorporate the reviews since then.

#32

1. Fixed. Not sure why it was there, works without it as well.
2. The non-admin user doesn't have permissions to view unpublished media. The default view was not using rendered entities, so the view was not preventing the user from seeing media they shouldn't.
3. Fixed.
4. Fixed. I indeed changed the order of the filters for more consistency with the content view, but that is not really necessary. Reverted the unrelated changes.
5. Fixed.
6. Fixed. Initially implemented the test as suggested in #14, but it is true we can also test without the standard profile.
7. Fixed.
8. Fixed.
9. Fixed.
10. :)

#37

Fixed and updated the content moderation test for this.

There are some coding standard warnings:

/var/lib/drupalci/workspace/jenkins-drupal_patches-8661/source/core/modules/media_library/tests/src/FunctionalJavascript/ContentModerationTest.php ✗ 4 more
line 13	Unused use statement
14	Unused use statement
76	Short array syntax must be used to define arrays
282	Functions must not contain multiple empty lines in a row; found 2 empty lines

Fixing the coding standard warnings and fixing the config test.

Same as the last comment. Trying this again, the last comment, the interdiff had the changes, but the patch lacked them. I must have been on the wrong branch when doing the diff against 8.8.x branch.

Don't think I can RTBC, but thanks @oknate! I think this is ready now.

1. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/ContentModerationTest.php
   @@ -0,0 +1,328 @@
   +class ContentModerationTest extends WebDriverTestBase {
   

   👍 This test looks excellent, and it does what I recommended in #14. It does more, actually, because I had not mentioned view any unpublished content in #14.

   @effulgentsia pointed out the need for this in #37. I think this is extremely misleading in the Content Moderation module and frankly downright dangerous. But that's how HEAD works, so … 🤷‍♂️

2. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/ContentModerationTest.php
   @@ -0,0 +1,328 @@
   +    // unpublished media', and a user the has 'view media' and 'view any
   

   🤓 Nit: s/the/that/. Can be fixed on commit.

3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/ContentModerationTest.php
   @@ -0,0 +1,328 @@
   +    // The media viewer user that can also view it's own unpublished media
   

   🤓 Übernit: s/it's/its/. Can be fixed on commit.

Fixed the nits to make it easier for committers :)

Updated the IS.

Crediting @phenaproxima and @Wim Leers for patch review.

I was torn on whether to also credit @marcoscano, @pavlosdan, and @Sam152. You all wrote very helpful comments earlier in this issue, but because they're more in the scope of #2987911: Make media compatible with content moderation, I added issue credit to you there instead of here.

Pushed to 8.8.x, but per #35, this needs a followup for a post_update function.

I'v reverted this for now. We need upgrade paths to be included in the same issue, not in followups. Thanks!

The first question is, do we really want an upgrade path at all and add this new filter to all media views on all existing sites?
This could mean users will all of a sudden not be able to see content they previously could, and I'm not sure if we want this. User could be relying on the existing behaviour.

do we really want an upgrade path at all and add this new filter to all media views on all existing sites?

No. Not all views. Just the 2 Views shipped with core and being changed by this patch. Views shipped by contrib/custom modules are the responsibility of those other modules. Views created by the end user via the UI already get a Status=1 default filter applied on them, and if the user removed it, it's their responsibility.

explaining what was changed, and how to restore the previous behavior

I'm fine with us returning text that explains what was changed. I don't think we need to explain how to restore the previous behavior. There's no legitimate reason to restore the previous behavior for these 2 Views. If someone really wants to do it for an illegitimate reason, they can, but we shouldn't communicate any text that encourages it.

Ok, added the update hooks and tests.

As a first response to #61:
The existing status filter is exposed and allows user to change it. The status extra filter is a hidden thing and does something similar, but different. So I think they should both be there.

I will take a look at #60 tonight.

New patch to address #60:

1. Fixed.
2. Fixed.
3. Fixed. I tried referring to the machine name of the view. But I guess the label would be better.
4. Fixed.
5. Just checked, removing the media library view, then uninstall/install the module will bring back the media library view as expected. So yeah, this seems to be correct. Not sure how hard the update should fail, the site is not at risk of being more/less broken by continuing the updates. I copied this from media_library_post_update_table_display() btw.

Fixed #64.

No worries, it's already end of the day here. Missed the quotes. Sorry!

The post_update functions and tests look great to me. Thanks! Pushed to 8.8.x.

Thanks!

Tests are failing in SQLite and Postgre: https://www.drupal.org/node/3060/qa

https://www.drupal.org/pift-ci-job/1417227
https://www.drupal.org/pift-ci-job/1417228

The order of the items differs. Since they are all programmatically created at the same time, the sort by 'changed' is brittle.

Mysql:

SQLite:

Here's a patch that addressed #71. It fixes the sorting by changed date, so the expected item is in the second position.

We recently had to do a similar thing in JSON:API: when no explicit sort is specified, the underlying DB's default sort is used. Which is obviously different between MySQL/MariaDB, PostgreSQL and SQLite.

Committed 2dfe9e9 and pushed to 8.8.x. Thanks!

We can mention this in the highlights as part of what a stabilized Media Library offers, e.g. "CKEditor support, Content Moderation integration..."

Actually this needs a change record for the API addition (also, isn't the codebase missing API documentation for it?).

I think this also may need a release note since it changes the result of the view to (by design) not include media the user can access. (It does that, correct?) In the past when we've had security issues that similarly revoke access to things, we've also mentioned it in the release notes for sites that might have been relying on this behavior rather than on correct access control handling. So, let's add a release note too (if I'm correct about that).

We should also describe the disruption from the un-access-bypassing: Before stuff you shouldn't have access to was listed; now it's not. If your permissions were configured incorrectly, you'll want to fix that.

Added change record draft and release notes snippet.

It may require some refining as I'm not sure what API documentation needs to be added, which was mentioned in #80. If there's something equivalent elsewhere in core I could be made aware of, that's all the reference I'll need to take care of it.

CR https://www.drupal.org/node/3088444 and release note added. Setting to Needs Review.

Created followup to update the API documentation in the codebase #3088632: Add the Content Moderation changes from 2969678 to Media API docs, and received confirmation that CR/RN are all set. Setting back to Fixed.

Perfect, thanks @bnjmnm!