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

CommentFileSizeAuthor
#64 constact-storage-2023-08-25.png11.71 KBadpo
#62 interdiff_60-61.diff1.83 KBieguskiza
#62 revisit_permissions-2708809-61.patch13.63 KBieguskiza
#60 interdiff_56-60.diff3.16 KBieguskiza
#60 revisit_permissions-2708809-60.patch13.49 KBieguskiza
#58 interdiff_52-56.txt6.59 KBAndras_Szilagyi
#56 revisit_permissions-2708809-56.patch13.56 KBAndras_Szilagyi
#54 revisit_permissions-2708809-54.patch4.42 KBolofbokedal
#52 revisit_permissions-2708809-52.patch12.98 KBjcnventura
#46 interdiff-2708809-39-46.txt330 bytesscott_euser
#46 revisit_permissions-2708809-46.patch12.98 KBscott_euser
#40 ezgif-3-098850ea46.gif152.63 KBscott_euser
#39 revisit_permissions-2708809-39.patch12.59 KBscott_euser
#39 interdiff-2708809-35-39.txt7.65 KBscott_euser
#36 revisit_permissions-2708809-35.patch10.93 KBscott_euser
#35 interdiff-2708809-34-35.txt4.82 KBscott_euser
#34 interdiff-2708809-31-34.txt2.34 KBscott_euser
#34 revisit_permissions-2708809-34.patch5.98 KBscott_euser
#31 interdiff-2708809-31.txt4.05 KBrajeshwari10
#31 2708809-31.patch6.1 KBrajeshwari10
#21 interdiff-2708809-21.txt778 bytesscott_euser
#21 contact_storage-revisit-permissions-required-to-view-contact-messages-2708809-21-D8.patch5.3 KBscott_euser
#19 interdiff-2708809-19.txt2.08 KBscott_euser
#19 contact_storage-revisit-permissions-required-to-view-contact-messages-2708809-19-D8.patch5.01 KBscott_euser
#11 interdiff-2708809-11.txt1.06 KBscott_euser
#11 contact_storage-revisit-permissions-required-to-view-contact-messages-2708809-11-D8.patch2.87 KBscott_euser
#7 contact_storage-revisit-permissions-required-to-view-contact-messages-2708809-8.1.patch1.73 KBscott_euser
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan created an issue. See original summary.

pashupathi nath gajawada’s picture

Assigned: Unassigned » pashupathi nath gajawada

I am looking into this issue.

Berdir’s picture

per 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.

naveenvalecha’s picture

4aficiona2’s picture

I 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).

4aficiona2’s picture

I 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.

scott_euser’s picture

I 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

  1. Go to admin > structure > views and change view access to something editors can access
  2. Go to the view and click to view a contact message as an editor: access denied

Proposed:

  1. Set the default access on initial install for the view to be access granted when permission 'view contact messages'
  2. Go to admin > structure > views as an editor with 'view contact messages' and click a contact message: access granted
scott_euser’s picture

Status: Active » Needs review

Seems this discussion has quieted down. Going to submit my suggested patch for review then.

scott_euser’s picture

Assigned: pashupathi nath gajawada » Unassigned

Status: Needs review » Needs work
scott_euser’s picture

Updated with new permission for test user to match new view permission.

DuneBL’s picture

I am not sure I correctly understood how this patch is working, but if

  • I **do not** allow "Administer contact forms and contact form settings" for a role
  • I allow "Delete contact messages" and "View contact messages" for the same role

Then, I cant not get access to "/admin/structure/contact/messages" with this role (after clear cache)

Is there something I misunderstood?

scott_euser’s picture

That 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.

DuneBL’s picture

Many thanks for the tip and your hard work!!!
I confirm it is working well.

scott_euser’s picture

Please 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 :)

DuneBL’s picture

Our messages cross each-other, I double confirm that your patch is working well!

scott_euser’s picture

Perfect, good to hear!

damondt’s picture

#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.

scott_euser’s picture

@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:

  1. Get the current view permission
  2. Loop through the roles
  3. If a role has the current view permission, they can already view contact messages, so grant them the new permission
  4. Change the view permission
damondt’s picture

To 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

use Drupal\Core\Access\AccessResult;
use Drupal\Core\Session\AccountInterface;
scott_euser’s picture

damondt’s picture

Spot on, super good response time too.

scott_euser’s picture

Status: Needs review » Reviewed & tested by the community

Going to switch this to reviewed and tested as two people have reviewed and confirmed now.

hsponner’s picture

Thanks, #21 works for me.

hugovk’s picture

Please could a maintainer have a look at this, it would be very useful to get merged. Thank you!

larowlan’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

A 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

  1. +++ b/contact_storage.install
    @@ -120,3 +121,48 @@ function contact_storage_update_8200() {
    + * ability to view contact messages, but we maintain both the access to the
    + * view as well as the ensure the roles that have access to administer contact
    

    the english here is out

  2. +++ b/contact_storage.install
    @@ -120,3 +121,48 @@ function contact_storage_update_8200() {
    +  $config->save();
    

    This needs to use the trusted data flag - see https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension...

  3. +++ b/contact_storage.install
    @@ -120,3 +121,48 @@ function contact_storage_update_8200() {
    +        $role->save();
    

    Saving roles using the entity api in update hooks isn't safe, either needs to use config api direct or a post update hook

  4. +++ b/contact_storage.module
    @@ -495,3 +497,19 @@ function contact_storage_contact_form_delete(EntityInterface $entity) {
    +  if ($entity->getEntityTypeId() == 'contact_message') {
    

    nit ===

  5. +++ b/contact_storage.module
    @@ -495,3 +497,19 @@ function contact_storage_contact_form_delete(EntityInterface $entity) {
    +    if ($account->hasPermission($operation . ' contact messages')) {
    

    operation can also be update and view label here? there are no corresponding permissions for that

jibran’s picture

Issue summary: View changes

@Berdir what's your opinion on this patch? Given that we also have #2724503: Per form based access permissions almost ready.

DuneBL’s picture

patch #21 apply partially on last dev:

can't find file to patch at input line 124
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/src/Tests/ContactStorageTest.php b/src/Tests/ContactStorageTest.php
|index 2e565f1..957580f 100644
|--- a/src/Tests/ContactStorageTest.php
|+++ b/src/Tests/ContactStorageTest.php
--------------------------

DuneBL’s picture

After applying #21 (see previous post), I got the following error:

The website encountered an unexpected error. Please try again later.
Recoverable fatal error: Argument 3 passed to contact_storage_entity_access() must be an instance of AccountInterface, instance of Drupal\Core\Session\AccountProxy given in contact_storage_entity_access() (line 521 of modules/contrib/contact_storage/contact_storage.module).
contact_storage_entity_access(Object, 'view', Object)
call_user_func_array('contact_storage_entity_access', Array) (Line: 402)
Drupal\Core\Extension\ModuleHandler->invokeAll('entity_access', Array) (Line: 84)
Drupal\Core\Entity\EntityAccessControlHandler->access(Object, 'view', NULL, 1) (Line: 631)
Drupal\Core\Entity\ContentEntityBase->access('view', NULL, 1) (Line: 184)
Drupal\Core\Field\Plugin\Field\FieldFormatter\EntityReferenceFormatterBase->checkAccess(Object) (Line: 55)
Drupal\Core\Field\Plugin\Field\FieldFormatter\EntityReferenceFormatterBase->getEntitiesToView(Object, 'en') (Line: 131)
Drupal\entity_reference_revisions\Plugin\Field\FieldFormatter\EntityReferenceRevisionsEntityFormatter->viewElements(Object, 'en') (Line: 80)
rajeshwari10’s picture

Assigned: Unassigned » rajeshwari10
rajeshwari10’s picture

Assigned: rajeshwari10 » Unassigned
Status: Needs work » Needs review
FileSize
6.1 KB
4.05 KB

I was not able to apply the patch #21, So rerolled the patch.
Also made changes as per said in #26.

Please Review.

Thanks!!

DuneBL’s picture

I confirm #31 apply cleanly and that the permissions are working as expected
Many thanks!!!!

scott_euser’s picture

Thanks 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.

scott_euser’s picture

Status: Needs review » Needs work
FileSize
5.98 KB
2.34 KB

Couple minor coding standards fixes to the last patch + fixing one of the outstanding issues from #26.
Tests under construction.

scott_euser’s picture

Status: Needs work » Needs review
FileSize
4.82 KB

Added tests

scott_euser’s picture

...and the patch

larowlan’s picture

Status: Needs review » Needs work
Issue tags: -php-novice, -Novice, -Needs tests

This is coming along nicely, thanks for the tests, a few minor things which I've already identified in #26

  1. +++ b/contact_storage.install
    @@ -110,7 +110,7 @@ function contact_storage_update_8200() {
    +      $config->trustData()->save();
    

    I don't think we can change existing update hooks? Perhaps this is a bad reroll hunk?

  2. +++ b/contact_storage.install
    @@ -121,3 +121,23 @@ function contact_storage_update_8200() {
    + * We are splitting apart, the permission to administer contact forms and the
    

    nit: No need for comma after apart.

  3. +++ b/contact_storage.install
    @@ -121,3 +121,23 @@ function contact_storage_update_8200() {
    + * permission to view contact messages. We ensure that the only those roles
    + * which has the access to administer contact forms will be able to view contact
    + * messages.
    

    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'

  4. +++ b/contact_storage.install
    @@ -121,3 +121,23 @@ function contact_storage_update_8200() {
    +
    

    nit: extra blank line here

  5. +++ b/contact_storage.install
    @@ -121,3 +121,23 @@ function contact_storage_update_8200() {
    +  $config->save();
    

    As per https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Extension!module.... we need the call to trustDate() here

  6. +++ b/contact_storage.module
    @@ -514,3 +516,22 @@ function contact_storage_contact_form_delete(EntityInterface $entity) {
    +    if ($account->hasPermission($operation . ' contact messages')) {
    

    We'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'?

  7. +++ b/tests/src/Functional/ContactStorageAccessTest.php
    @@ -0,0 +1,156 @@
    +    $this->drupalGet('admin/structure/contact/messages');
    +    $this->assertResponse(200, 'Allowed to access the list of contact messages.');
    +
    +    // Check that the view only user cannot edit.
    +    $this->assertNoText(t('Edit'));
    +
    +    // Assert that there is no delete button.
    +    $this->assertNoText('Delete.', 'Not allowed to delete the contact message');
    

    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.

  8. +++ b/tests/src/Functional/ContactStorageAccessTest.php
    @@ -0,0 +1,156 @@
    +    // Check that the view and delete user can delete.
    +    $this->drupalGet('admin/structure/contact/messages');
    +    $this->clickLink(t('Delete'));
    +    $this->drupalPostForm(NULL, NULL, t('Delete'));
    +    $this->assertRaw(t('The @entity-type %label has been deleted.', [
    +      // See \Drupal\Core\Entity\EntityDeleteFormTrait::getDeletionMessage().
    +      '@entity-type' => 'contact message',
    +      '%label'       => 'Test_subject',
    +    ]));
    +
    +    // Make sure no messages are available.
    +    $this->assertText('There is no Contact message yet.');
    

    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?

larowlan’s picture

scott_euser’s picture

Thanks for the review!

As per https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Extension!module.... we need the call to trustDate() here

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?

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?

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

  /**
   * {@inheritdoc}
   */
  public function access($object, AccountInterface $account = NULL, $return_as_object = FALSE) {
    /** @var \Drupal\contact\MessageInterface $object */
    return $object->access('delete', $account, $return_as_object);
  }

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():

/**
 * Implements hook_entity_access().
 */
function contact_storage_entity_access(EntityInterface $entity, $operation, AccountInterface $account) {

  // Check access to the contact message entity.
  $available_operations = ['view', 'delete', 'update'];
  if ($entity->getEntityTypeId() === 'contact_message' && in_array($operation, $available_operations)) {
    if ($account->hasPermission($operation . ' contact messages')) {
      return AccessResult::allowed();
    }
  }

  // No opinion.
  return AccessResult::neutral();
}
scott_euser’s picture

Issue summary: View changes
FileSize
152.63 KB

scott_euser’s picture

Status: Needs work » Needs review

Putting back to needs review (though the above issue should be resolved, just need reviewer advice)

larowlan’s picture

Sorry, typo meant

trustData

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

naveenvalecha’s picture

Should we close it as duplicate? #2724503: Per form based access permissions

jibran’s picture

I think we can have both.

scott_euser’s picture

Per 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.

scott_euser’s picture

What happens if you give someone 'access the content overview' permission but not 'adminster nodes' - can they see the actions but not execute?

Yes, 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:

--- a/contact_storage.routing.yml
+++ b/contact_storage.routing.yml
@@ -27,4 +27,4 @@ entity.contact.multiple_delete_confirm:
   defaults:
     _form: '\Drupal\contact_storage\Form\DeleteMultiple'
   requirements:
-    _permission: 'administer contact forms'
+    _permission: 'delete contact messages'
larowlan’s picture

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.

Agree

DuneBL’s picture

#46 is working as expected!
Many thanks

scott_euser’s picture

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.

Issue created for core here https://www.drupal.org/node/2895262

mansspams’s picture

Status: Needs review » Reviewed & tested by the community

This works really well. Fixed an issue where private files were not accessible even with permission. That depended on permission on entity. Thanks everyone!

jcnventura’s picture

Status: Reviewed & tested by the community » Needs work

No longer applies to latest dev.

jcnventura’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
12.98 KB

Not 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.

DuneBL’s picture

#52 apply cleanly on 8.6
Maybe we should commit

olofbokedal’s picture

Bumped the update function to contact_storage_update_8203.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 54: revisit_permissions-2708809-54.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Andras_Szilagyi’s picture

Updating patch to latest changes in module and drupal

jcnventura’s picture

@Andras_Szilagyi can you provide an interdiff between #56 and #52?

Andras_Szilagyi’s picture

FileSize
6.59 KB

Hi @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..

sinn’s picture

Status: Needs review » Reviewed & tested by the community

#56 works for me.

ieguskiza’s picture

I rerolled the patch to the new 1.3 release. Attached the interdiff (I used patchutils and the result is... odd?) as well.

The last submitted patch, 60: revisit_permissions-2708809-60.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

ieguskiza’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
13.63 KB
1.83 KB

Turns out the new test class was outdated. Uploading the new patch with interdiff.

joevagyok’s picture

Status: Needs review » Reviewed & tested by the community

Tested #62 and works as designed.

adpo’s picture

Also tested #62 but I don't see any permission setting I'm afraid. I have run update.php and cleared the cache.

setting

Is there anything else I need to do?