Problem/Motivation

Follow-up for #2843755-19: EntityResource: Provide comprehensive test coverage for Message entity
Message entities are not stored but access handler use permission access site-wide contact form to grant access to this operations

Proposed resolution

Forbid access for unused operations.

Remaining tasks

- Review merge request
- Write a change record draft
- Review change record and mark RTBC

User interface changes

no

API changes

Yes, Access denied may occur when using a module such as contact_storage, which will need to add its own access handler. However an entity's handlers and access check providers are considered internal per Drupal backwards-compatibility and internal API.

Data model changes

no

Issue fork drupal-2878513

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost created an issue. See original summary.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.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.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.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.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.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.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.

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.

larowlan’s picture

Category: Bug report » Task
Issue tags: +Novice, +Bug Smash Initiative

this feels like a nice novice task - rather than a bug, feel free to disagree

varshith’s picture

The patch forbids view/edit/update operations for Message entity type.

varshith’s picture

Status: Active » Needs review

Status: Needs review » Needs work
varshith’s picture

@larowlan
If the patch is good I can handle the failing tests as required. Pls advise.

lostcarpark’s picture

Hi,
I'm looking at this issue at DrupalCon Europe 2021. I'm going to investigate why the current patch fails.

lostcarpark’s picture

Reran tests and ran in DrupalPod. Test inherits MessageResourceTestBase, which asserts response, which is changed by this fix. Looking at overriding test to update expected text.

mradcliffe’s picture

Issue tags: +Europe2021

Adding the Europe2021 tag as we worked on this today.

Aside: We had a bit of trouble getting DrupalPod to run tests, but it looks like the issue was a typo in the namespace as we were working changes, which caused the autoloader not to load the test.

mradcliffe’s picture

We added the patch to the merge request so hiding the patch.

lostcarpark’s picture

I have been reworking the tests in MessageHalJsonAnonTest, overriding the functions that fail.
I have overridden the getExpectedUnauthorizedAccessMessage method to test for the updated error message.
I have overridden the testPost method to test for 403 forbidden error for all cases.
There are other classes requiring similar fixes, but I'll get this one reviewed before embarking on them.

lostcarpark’s picture

The above change includes a fix for the MessageHalJsonAnonTest test class.
I have overridden the getExpectedUnauthorizedAccessMessage to return the updated error message.
I've also overridden testPost to check that 403 forbidden is returned in all cases.
There are some other test classes that require similar changes. Could someone check my changes and let me know if this is the correct approach, please?

lostcarpark’s picture

Status: Needs work » Needs review
mradcliffe’s picture

Status: Needs review » Needs work

The testbot is failing quickly because of a typo in a comment.

larowlan’s picture

Drupal\Tests\jsonapi\Functional\MessageTest::getExpectedUnauthorizedAccessMessage is where the remaining issue is coming from.

I think we'll also need to add a change record for this, as it will break the contact storage module.

The contact storage module needs to add a new storage handler to modify how this works once this is committed. Both @andypost and I are maintainers there, but we'd welcome help with patches/an issue

andypost’s picture

Great catch @larowlan BC++

mradcliffe’s picture

Status: Needs work » Needs review
Issue tags: +Needs change record

It looks like we're in Needs review again. Also I added a Needs change record tag.

mradcliffe’s picture

Issue summary: View changes

I added a note about BC to the issue summary in the API changes section.

lostcarpark’s picture

I have opened #3243456: Add new access control handler on Contact Storage module's issue queue. I'm not sure how big a change is involved, but I think it would be disruptive to a lot of websites (26839 sites report using the module) if this change were implemented without updating it.

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.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 200, following Review a patch or merge require as a guide.

If the MR could please be updated for 10.1 or a new MR opened for that.

Also was tagged for a change record if we could get one started please.

Thanks.

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

rpayanm’s picture

Status: Needs work » Needs review
xjm’s picture

Status: Needs review » Needs work

Test failures look related:
https://www.drupal.org/pift-ci-job/2547101

lostcarpark’s picture

So the errors are:

Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'{"message":"No route found for \u0022POST \/entity\/contact_message\u0022"}'
+'{"message":"No route found for \u0022POST http:\/\/php-apache-jenkins-drupal-patches-158543\/subdirectory\/entity\/contact_message\u0022"}'

I believe they are stemming from this line:

 $this->assertResourceErrorResponse(404, 'No route found for "POST ' . str_replace($this->baseUrl, '', $this->getEntityResourcePostUrl()->setAbsolute()->toString()) . '"', $response);

In the following file:

core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php

As far as I can see, it is attempting to strip out the base URL of the site to get the relative URL. When I run the tests locally, it gives 2266 warnings of NULL passed into str_replace, so I wonder could $this->baseUrl be returning null?

Any thoughts on how to fix?

smustgrave’s picture

These are tricky but something with this test needs to be tweaked

$this->assertResourceErrorResponse(404, 'No route found for "POST ' . str_replace($this->baseUrl, '', $this->getEntityResourcePostUrl()->setAbsolute()->toString()) . '"', $response);

Was is the baseUrl being replace?

lostcarpark’s picture

Working on this as part of #ContributionWeekend2023.

lostcarpark’s picture

Any idea how I can run one specific test rather than a whole test group? That would hopefully make it easier to debug.

smustgrave’s picture

can probably setup your local to run tests and just run the one. Would have to google what you need to do to set that up though.

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.

lostcarpark’s picture

I think !3131 can be closed, but I didn't create it, so I don't have permission to close it.

lostcarpark’s picture

Tests are now passing on !5167.

Turned out trying to remove the base path from the URL when calling assertResourceErrorResponse was the wrong way to go. Compared to similar checks elsewhere in core and figured out I could remove the replace from this test. This also meant the setting of basePath could be removed from setUp.

lostcarpark’s picture

Status: Needs work » Needs review

Tests are now passing, so moving to needs review.

smustgrave’s picture

Status: Needs review » Needs work
1) Drupal\Tests\contact\Kernel\MessageEntityTest::testMessageMethods
Failed asserting that true is false.
/builds/issue/drupal-2878513/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/builds/issue/drupal-2878513/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/builds/issue/drupal-2878513/core/modules/contact/tests/src/Kernel/MessageEntityTest.php:70
/builds/issue/drupal-2878513/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
FAILURES!
Tests: 1, Assertions: 23, Failures: 1.

Rebased to run the test-only feature and got a failure as expected.

Seems most feedback has been addressed only moving to NW for the change record.

Good work!