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
Comment | File | Size | Author |
---|
Issue fork drupal-2878513
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
Comment #10
larowlanthis feels like a nice novice task - rather than a bug, feel free to disagree
Comment #11
varshith CreditAttribution: varshith as a volunteer and commentedThe patch forbids view/edit/update operations for Message entity type.
Comment #12
varshith CreditAttribution: varshith as a volunteer and commentedComment #14
varshith CreditAttribution: varshith as a volunteer and commented@larowlan
If the patch is good I can handle the failing tests as required. Pls advise.
Comment #15
lostcarpark CreditAttribution: lostcarpark as a volunteer commentedHi,
I'm looking at this issue at DrupalCon Europe 2021. I'm going to investigate why the current patch fails.
Comment #16
lostcarpark CreditAttribution: lostcarpark as a volunteer commentedReran 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.
Comment #17
mradcliffeAdding 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.
Comment #19
mradcliffeWe added the patch to the merge request so hiding the patch.
Comment #20
lostcarpark CreditAttribution: lostcarpark as a volunteer commentedI 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.
Comment #21
lostcarpark CreditAttribution: lostcarpark as a volunteer commentedThe 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?
Comment #22
lostcarpark CreditAttribution: lostcarpark as a volunteer commentedComment #23
mradcliffeThe testbot is failing quickly because of a typo in a comment.
Comment #24
larowlanDrupal\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
Comment #25
andypostGreat catch @larowlan BC++
Comment #26
mradcliffeIt looks like we're in Needs review again. Also I added a Needs change record tag.
Comment #27
mradcliffeI added a note about BC to the issue summary in the API changes section.
Comment #28
lostcarpark CreditAttribution: lostcarpark as a volunteer commentedI 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.
Comment #32
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis 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.
Comment #35
rpayanmComment #36
xjmTest failures look related:
https://www.drupal.org/pift-ci-job/2547101
Comment #38
lostcarpark CreditAttribution: lostcarpark as a volunteer commentedSo the errors are:
I believe they are stemming from this line:
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?
Comment #39
smustgrave CreditAttribution: smustgrave at Mobomo commentedThese 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?
Comment #40
lostcarpark CreditAttribution: lostcarpark as a volunteer commentedWorking on this as part of #ContributionWeekend2023.
Comment #41
lostcarpark CreditAttribution: lostcarpark as a volunteer commentedAny idea how I can run one specific test rather than a whole test group? That would hopefully make it easier to debug.
Comment #42
smustgrave CreditAttribution: smustgrave at Mobomo commentedcan 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.
Comment #45
lostcarpark CreditAttribution: lostcarpark as a volunteer commentedI think !3131 can be closed, but I didn't create it, so I don't have permission to close it.
Comment #46
lostcarpark CreditAttribution: lostcarpark as a volunteer commentedTests 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 ofbasePath
could be removed fromsetUp
.Comment #47
lostcarpark CreditAttribution: lostcarpark as a volunteer commentedTests are now passing, so moving to needs review.
Comment #48
smustgrave CreditAttribution: smustgrave at Mobomo commentedRebased 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!