Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
At present we use 'administer contact forms' to govern access to viewing/editing contact messages.
However this is too generic.
We should look into adding 'view', 'delete' and 'edit' permissions for contact messages.
Possibly on a per-form basis. See #2724503: Per form based access permissions for that.
Proposed resolution
Decide on best permission set.
Patch.
Review.
Remaining tasks
Everything.
User interface changes
None
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#64 | constact-storage-2023-08-25.png | 11.71 KB | adpo |
#62 | interdiff_60-61.diff | 1.83 KB | ieguskiza |
#62 | revisit_permissions-2708809-61.patch | 13.63 KB | ieguskiza |
| |||
#60 | interdiff_56-60.diff | 3.16 KB | ieguskiza |
#60 | revisit_permissions-2708809-60.patch | 13.49 KB | ieguskiza |
Comments
Comment #2
pashupathi nath gajawada CreditAttribution: pashupathi nath gajawada as a volunteer and at Melity commentedI am looking into this issue.
Comment #3
Berdirper form is tricky for the list, you need to start doing query alter stuff.
Another problem is the accessibility of the messages, you have to go through the contact forms and then use the local task to get there.
Comment #4
naveenvalechaComment #5
4aficiona2 CreditAttribution: 4aficiona2 commentedI my opinion it is not only necessary to distinguish between permissions viewing / editing / deleting contact form messages but also between doing this (viewing / editing / deleting contact form messages) and to actually configure (administer) the contact form. This should be like with nodes where you have permissions for "Administer content" (contact form messages) and "Administer content types" (contact form definition / settings).
Comment #6
4aficiona2 CreditAttribution: 4aficiona2 commentedI figured out that there are some more contact message permissions listed under "Field UI".
With the permission "Contact message: Administer fields", "Contact message: Administer display" and "Contact message: Administer form display" I could almost solve my permission related issues except that the "Forms" tab (edit form details) should be hidden too.
Comment #7
scott_euser CreditAttribution: scott_euser at Fat Beehive commentedI am not sure if Administer Fields / Administer Form Display is the right way to go if you intend to give partial control to non-admin site editors. How about this approach? It is enough to allow a non-admin site editor to view messages if you give them that permission.
Example use case:
As a site editor (but not admin) I want to view contact messages via the admin
Proposed:
Comment #8
scott_euser CreditAttribution: scott_euser at Fat Beehive commentedSeems this discussion has quieted down. Going to submit my suggested patch for review then.
Comment #9
scott_euser CreditAttribution: scott_euser at Fat Beehive commentedComment #11
scott_euser CreditAttribution: scott_euser at Fat Beehive commentedUpdated with new permission for test user to match new view permission.
Comment #12
DuneBLI am not sure I correctly understood how this patch is working, but if
Then, I cant not get access to "/admin/structure/contact/messages" with this role (after clear cache)
Is there something I misunderstood?
Comment #13
scott_euser CreditAttribution: scott_euser at Fat Beehive commentedThat path is controlled by a view: with the permission configured as you have described, you can view and delete messages, you need to change the permission on the view as desired (eg, to use the new view contact messages permission).
I don't think the patch should change permission on the view - particularly for the upgrade path.
Comment #14
DuneBLMany thanks for the tip and your hard work!!!
I confirm it is working well.
Comment #15
scott_euser CreditAttribution: scott_euser at Fat Beehive commentedPlease let me know if that doesn't work for you by the way, didn't double check but I'm pretty sure that's the reason :)
Comment #16
DuneBLOur messages cross each-other, I double confirm that your patch is working well!
Comment #17
scott_euser CreditAttribution: scott_euser at Fat Beehive commentedPerfect, good to hear!
Comment #18
damondt CreditAttribution: damondt as a volunteer commented#11 Works to allow the user to see the contact messages view if the module is reinstalled, but does not allow them to actually view or delete the messages.
Comment #19
scott_euser CreditAttribution: scott_euser at Fat Beehive commented@damondt thanks for reviewing. Yes that is correct, the change to permissions is in the config/install default config settings only. As the site builder may have subsequently changed permissions to something else so I felt a bit nervous about imposing the change on them.
Thinking about it further, I've now done the following:
Comment #20
damondt CreditAttribution: damondt as a volunteer commentedTo get the entity access hook to work I had to change the function name to
function contact_storage_entity_access(EntityInterface $entity, $operation, AccountInterface $account)
And add
Comment #21
scott_euser CreditAttribution: scott_euser at Fat Beehive commentedGood spot! This interdiff look okay to you?
Comment #22
damondt CreditAttribution: damondt as a volunteer commentedSpot on, super good response time too.
Comment #23
scott_euser CreditAttribution: scott_euser at Fat Beehive commentedGoing to switch this to reviewed and tested as two people have reviewed and confirmed now.
Comment #24
hsponner CreditAttribution: hsponner commentedThanks, #21 works for me.
Comment #25
hugovk CreditAttribution: hugovk at Digia commentedPlease could a maintainer have a look at this, it would be very useful to get merged. Thank you!
Comment #26
larowlanA few things need to be done here - including needing new tests that ensure the permissions work the way they're intended.
Thanks for your work so far - observations below
the english here is out
This needs to use the trusted data flag - see https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension...
Saving roles using the entity api in update hooks isn't safe, either needs to use config api direct or a post update hook
nit ===
operation can also be update and view label here? there are no corresponding permissions for that
Comment #27
jibran@Berdir what's your opinion on this patch? Given that we also have #2724503: Per form based access permissions almost ready.
Comment #28
DuneBLpatch #21 apply partially on last dev:
Comment #29
DuneBLAfter applying #21 (see previous post), I got the following error:
Comment #30
rajeshwari10 CreditAttribution: rajeshwari10 as a volunteer and at Valuebound commentedComment #31
rajeshwari10 CreditAttribution: rajeshwari10 as a volunteer and at Valuebound commentedI was not able to apply the patch #21, So rerolled the patch.
Also made changes as per said in #26.
Please Review.
Thanks!!
Comment #32
DuneBLI confirm #31 apply cleanly and that the permissions are working as expected
Many thanks!!!!
Comment #33
scott_euser CreditAttribution: scott_euser at Fat Beehive commentedThanks for your updating it. We still need the tests that are requested in #26. I'll try to review the changes you've made soon.
Comment #34
scott_euser CreditAttribution: scott_euser at Fat Beehive commentedCouple minor coding standards fixes to the last patch + fixing one of the outstanding issues from #26.
Tests under construction.
Comment #35
scott_euser CreditAttribution: scott_euser at Fat Beehive commentedAdded tests
Comment #36
scott_euser CreditAttribution: scott_euser at Fat Beehive commented...and the patch
Comment #37
larowlanThis is coming along nicely, thanks for the tests, a few minor things which I've already identified in #26
I don't think we can change existing update hooks? Perhaps this is a bad reroll hunk?
nit: No need for comma after apart.
The english here needs work, suggest 'We ensure that only those roles which have access to administer contact forms will be able to view contact messages'
nit: extra blank line here
As per https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Extension!module.... we need the call to
trustDate()
hereWe're checking non-existent permissions here. We define permissions for 'view' and 'delete' but we don't define any of the other possible operations as seen in https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21... - i.e. 'update' and 'view label'?
Can we assert that they can see the title of the message, i.e. we're asserting a 200, but there is no positive assert, only negatives. This page might indeed be empty.
So can this user edit messages? We don't test that. I suspect that because we don't have an 'update contact messages' permission, they cannot - unless we give them the administer contact forms permission?
Comment #38
larowlanComment #39
scott_euser CreditAttribution: scott_euser at Fat Beehive commentedThanks for the review!
I couldn't find the 'trustDate()' function anywhere (checked in web/core, vendor/, and for examples in contrib file - couldn't find in the docs either. Is it that you want $config->save(TRUE); to mark it $has_trusted_data as true?
Makes sense, I've added the update permission. The test is checking if there is an Edit link - should it check for an access denied at /admin/structure/contact/messages/1/edit as well perhaps?
Where I am having an issue: when a user cannot delete messages, they can still see the bulk action 'Delete message' at /admin/structure/contact/messages. When they try to delete, they get access denied, but I don't see how to stop it from showing in the first place. It seems to check the access for the action plugin here:
src/Plugin/Action/DeleteMessage.php
Do we need to modify the entity access further? Maybe to return an AccessResult::forbidden()? I would have thought that this would catch the $object->access():
Comment #40
scott_euser CreditAttribution: scott_euser at Fat Beehive commentedComment #41
scott_euser CreditAttribution: scott_euser at Fat Beehive commentedPutting back to needs review (though the above issue should be resolved, just need reviewer advice)
Comment #42
larowlanSorry, typo meant
I think that it lets them see the plugin, but not execute it, but I'm not familiar with the action API.
What happens if you give someone 'access the content overview' permission but not 'adminster nodes' - can they see the actions but not execute?
Lee
Comment #43
naveenvalechaShould we close it as duplicate? #2724503: Per form based access permissions
Comment #44
jibranI think we can have both.
Comment #45
scott_euser CreditAttribution: scott_euser at Fat Beehive commentedPer form based access is very different in my opinion. With that the site builder needs to intervene to control access. This aims to allow the site builder to let an editor make there own forms (by granting create update delete permission for contact forms) and view and edit and delete submissions to all those forms (this patch) but not change the administrative settings for how contact storage should behave. The site builder shouldn't have to login to restrict access every time the client builds a new form and similarly shouldn't have to edit every form if they create a new role that should, let's say, be able to view all form submissions.
Comment #46
scott_euser CreditAttribution: scott_euser as a volunteer and at Fat Beehive commentedYes, actually the above gif is with exactly that case. I've checked if this is an issue with the module or core by checking how it behaves if you change the /admin/people view to 'View user information' for a role but don't grant access to cancel user accounts. The same issue occurs - you can see the bulk action to cancel a user account but when you attempt to run the action, it correctly gives permission denied.
I then checked core/modules/system/src/Plugin/views/field/BulkForm.php and see that as suspected, the form itself just loads all actions for that entity and doesn't check action access or not. It appears to be happening after the access control was added here but wasn't updated to make the BulkForm use that as well.
Anyways, based on the above, that is the current behaviour of core and shouldn't be a blocker to finish this patch as the access isn't actually granted, only the user is visually shown an action they cannot carry out.
I did notice this missing in the patch while going through this:
Comment #47
larowlanAgree
Comment #48
DuneBL#46 is working as expected!
Many thanks
Comment #49
scott_euser CreditAttribution: scott_euser as a volunteer and at Fat Beehive commentedIssue created for core here https://www.drupal.org/node/2895262
Comment #50
mansspams CreditAttribution: mansspams at Wunder commentedThis works really well. Fixed an issue where private files were not accessible even with permission. That depended on permission on entity. Thanks everyone!
Comment #51
jcnventura CreditAttribution: jcnventura at Wunder commentedNo longer applies to latest dev.
Comment #52
jcnventura CreditAttribution: jcnventura at Wunder commentedNot sure why composer-patches failed to apply it. Only change is a 5-line offset in the contact_storage.module patch.
No interdiff because of that. And setting previous status back.
Comment #53
DuneBL#52 apply cleanly on 8.6
Maybe we should commit
Comment #54
olofbokedal CreditAttribution: olofbokedal at Odd Hill commentedBumped the update function to contact_storage_update_8203.
Comment #56
Andras_Szilagyi CreditAttribution: Andras_Szilagyi at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedUpdating patch to latest changes in module and drupal
Comment #57
jcnventura CreditAttribution: jcnventura commented@Andras_Szilagyi can you provide an interdiff between #56 and #52?
Comment #58
Andras_Szilagyi CreditAttribution: Andras_Szilagyi at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedHi @jcnventura,
unfortunately both patchutils and git where unable to create the interdiff, the code base is to far apart, so a linux text diff is the best I can do..
Comment #59
sinn CreditAttribution: sinn as a volunteer commented#56 works for me.
Comment #60
ieguskiza CreditAttribution: ieguskiza at NTT DATA for European Commission and European Union Institutions, Agencies and Bodies commentedI rerolled the patch to the new 1.3 release. Attached the interdiff (I used patchutils and the result is... odd?) as well.
Comment #62
ieguskiza CreditAttribution: ieguskiza at NTT DATA for European Commission and European Union Institutions, Agencies and Bodies commentedTurns out the new test class was outdated. Uploading the new patch with interdiff.
Comment #63
joevagyok CreditAttribution: joevagyok for European Commission and European Union Institutions, Agencies and Bodies commentedTested #62 and works as designed.
Comment #64
adpo CreditAttribution: adpo commentedAlso tested #62 but I don't see any permission setting I'm afraid. I have run update.php and cleared the cache.
Is there anything else I need to do?