Unpublished pages should return a 404, not a 403

Merge request add that installs the https://www.drupal.org/project/unpublished_404 → module to the drupal_cms_authentication recipe.

Since this adds a module, I'm not sure we should add it in a patch release.

Yes, it's possible with ECA, I've just tested it successfully. We only require a new feature that has already been in the makes: ✨ Plugin to throw AccessDeniedException Active . I'll finish that later today and get a review from @freelock who originally suggested this. Will then be released later this week together with a few other improvements that we've just built.

Implemented an ECA model which does the same thing as the unpublish 404 module. This requires the MR from ✨ Plugin to throw AccessDeniedException Active to work.

I love the ECA idea. Postponing on ✨ Plugin to throw AccessDeniedException Active

thejimbirch → closed merge request !479

I don't fully understand why only the 'view own unpublished content' permission is checked (this is in the module, but also reproduced in the ECA model), why not also 'view all unpublished content' or other similar permissions?

Also should this only apply to nodes, or could it potentially find any entity and check EntityPublishedInterface - then it could handle unpublished taxonomy terms and similar.

One question on the MR about user switching as well.

@catch you're right, I've just copied the functionality from the module. And the reason they may have used the "view own unpublished content" permission only seems to be that the "'view all unpublished content" doesn't exist - something that puzzled me many times.

We could extend this feature to all entity types, yes. But that requires some ECA enhancements first.

@catch, another thought about the permission check: I think we can remove that entirely because we already know that the user has no permission to view the page as we got triggered by a 403.

I've updated the model and made the following changes utilizing the latest ECA 2.1.x-dev:

• Remove the permission check for viewing unpublished nodes because we already know from the 403 response code that the user has no permission
• Made sure that the thrown exception is not logged as an error, as this is not what a site owner would expect

I'm going to tag new releases for ECA and bpmn_io now, so that's back to NR here then.

I've also cleaned-up dependencies so that the latest ECE and bpmn_io releases will be used and all the recipes modeller declaration, since we no longer rely on the fallback modeller.

One recurring question about changing the dependency constraints, and I think this definitely needs explicit test coverage.

I'm reclassifying this as a bug, since returning a 403 for unpublished pages doesn't prevent them from being indexed by search engines. I think, to our target audience, that would seem like a bug, even if core doesn't consider it a bug.

I've resolved all open threads except the discussion with @catch.

Also added three tests for an unpublished node, a non-existent node and a non-permitted page.

This change looks great to me. I tested manually on the Tugboat preview and it behaved exactly as promise. Let's ship this.

OK - one question. It could be done in a follow-up, but since this change is SEO-related, should we not move it to the seo_basic recipe?

Responses in slack to that:

jurgenhaas
3 minutes ago
Good question. I'd say SEO exposes the issue, but even without SEO, a 404 response for unpublished nodes seems to be more logcial, and it won't disclose internal information. So, for me it could be a generic bug fix, not just SEO.

thejimbirch
2 minutes ago
I agree, I'd rather it in the recipe its in. That is a great ECA foundation.

Acknowledged! Merged into 1.x and cherry-picked to 1.0.x. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.