Implement field permissions per-bundle (field instance)

I took a shot at it. The current version doesn't make it optional. The configuration is always per-bundle. This might overload the Field Permissions overview page, but it should keep the Drupal permissions page relatively clean, since only the field instances with "custom" field permissions are listed.

The old version had the problem, that it saved the FieldConfig too early and this lead to an error when using certain contrib modules (e.g. geocoder), which needed the third party settings for communication. Now the FieldConfig is saved on form submit.

Tested and works very well in my current project :)

Attached a rerolled patch for 8.x-1.0-rc2

would an update path be possible for - e.g. set all permissions to bundles according to the pre-patched permissions?

The patch works well, but an upgrade path would be great. Atm all existing field permissions do not have an effect any more.

Was in need of this patch with updated permissions so I created an update to address #8. The update goes through field storages transferring settings to field configs and updating roles.

+++ b/src/FieldPermissionsService.php
@@ -135,8 +151,8 @@ class FieldPermissionsService implements FieldPermissionsServiceInterface, Conta
-  public function fieldGetPermissionType(FieldStorageConfigInterface $field) {
-    return $field->getThirdPartySetting('field_permissions', 'permission_type', FieldPermissionTypeInterface::ACCESS_PUBLIC);
+  public function fieldGetPermissionType(FieldConfigInterface $field_config) {
+    return $field_config->getThirdPartySetting('field_permissions', 'permission_type', FieldPermissionTypeInterface::ACCESS_PUBLIC);

This change results in the following error on some JsonAPI calls:

Argument 1 passed to Drupal\field_permissions\FieldPermissionsService::fieldGetPermissionType() must implement interface
Drupal\field\FieldConfigInterface, instance of Drupal\field\Entity\FieldStorageConfig given

There are still some calls present to this function that pass field storage definition instead of field instance definition.
One such example in FieldPermissionsService class:

  public function hasFieldViewAccessForEveryEntity(AccountInterface $account, FieldDefinitionInterface $field_definition) {
    $permission_type = $this->fieldGetPermissionType($field_definition->getFieldStorageDefinition());

Patch #12 needs reroll. So I attached it.

This is NOT the correct patch.

Actually patch #13 is the right one.

I did a little mess with the files, so I'm attaching it again.
Sorry...

#12 installed and worked for me on D8.9.13. I couldn't get #13 or #14 to install.

I noticed I was getting a similar error to the one reported in #11. Mine was the following:

Argument 3 passed to Drupal\field_permissions\Plugin\FieldPermissionType\Manager::createInstance() must implement interface Drupal\field\FieldConfigInterface or be null, instance of Drupal\field\Entity\FieldStorageConfig given, called in /mnt/www/html/vcsweb/docroot/modules/contrib/field_permissions/src/FieldPermissionsService.php on line 232 in Drupal\field_permissions\Plugin\FieldPermissionType\Manager->createInstance() (line 51 of /mnt/www/html/vcsweb/docroot/modules/contrib/field_permissions/src/Plugin/FieldPermissionType/Manager.php) #0 /mnt/www/html/vcsweb/docroot/modules/contrib/field_permissions/src/FieldPermissionsService.php(232): Drupal\field_permissions\Plugin\FieldPermissionType\Manager->createInstance('custom', Array, Object(Drupal\field\Entity\FieldStorageConfig))

I was also getting it when I would do a JSON API call (via Entity Share).

I've rerolled the patch and corrected the issue reported in #11. I also checked and permissions set at a field level before installing the patch appear to be preserved.

My site uses D8.9.13, and I created the patch using the 8.x-1.x-dev branch (hopefully I got this terminology right, I'm still learning!).

I ran into a few more errors where the issue mentioned in #11 wasn't fixed, so I've updated my version of the patch to correct those issues. I've double-checked to make sure that that all issues that existed in my use case have been fixed.

I'm strongly in favor of adding this to the module: at the very least, the entity-type-level distinction seems like a huge bug. Is there consideration of merging this in? Maybe it needs to be 2.x to reflect the significant change in data structure?

This is the current blocker for merging: https://www.drupal.org/project/field_permissions/issues/2881776#comment-... -- the goal is to make the per-bundle option a configuration, not something you'd have to do on all entity types for all field permissions. The current patch does not allow this, and forces all entities to use per-bundle field permissions, and based on the number of bundles per entity, that might make this page: /admin/people/permissions unloadable.

I don't think adding the make-this-per-bundle configuration option would be too hard -- just none of the patches so far have done it!

I do also really like the idea of putting this into a 2.x branch as well though.

Re-rolled patch from comment 18 for the 8.x-1.1.0 release.

Hi, I've installed and tested #18, (not using #21, since it does not patch the actual dev-HEAD.)
So far it is working as expected, after adjusting our implemented custom plugins that extend `Drupal\field_permissions\Plugin\FieldPermissionType\Base`

I do also really like the idea of putting this into a 2.x branch as well though.

Because of the interface changes, I strongly vote for leaving 1.x untouched and consider this as first new feature for the 2.x-branch

Patch from #18 works for me too. I have many instances of a field in different bundles (and in different entity types).

In first place I could have named the fields differently for each entity type, which would have solved my issue. But I was not aware of the issue and now it would mean additional work, as it would imply creating migrations from old to new field. Luckily it got already fixed.

Would be nice if the patch could applied to the module ASAP, as there is an update hook involved. If there is a future update adding the same function (field_permissions_update_8001()) updating the patched version may get complicated.

For me it would be fine to pump the mayor version in case there are still concerns about compatibility or "performance" of the permission form (#3284722: Permissions not being saved).

Reroll should apply to version 1.2

Install file went *poof* during reroll, let's put that back

Trying to create an interdiff I got the error:

interdiff: Whitespace damage detected in input

Diff between #18 and #28 https://github.com/osopolar/patchdiff/commit/26e4d7b6c6ce0506fcbb8691b95...

I fixed the whitespace issue in 2881776-18.patch, see 2881776-18.whitespaces-fixed.patch by adding a whitespace on each empty line and created interdiff between #18 and #28.

I modified the patch from #28 to work with version 8.x-1.2. Besides the patch I added the interdiff between #28 and #32. Interdiff shows no differences between #18 and #32.

Patch does not apply to current dev version, therefore I leave this issue on needs work.

I'm applying the patch from https://www.drupal.org/project/field_permissions/issues/2881776#comment-... in a new merge request open here:

https://git.drupalcode.org/project/field_permissions/-/merge_requests/10...

It does two things.

1. Applies to the latest version of the 8.x-1.x branch.
2. Updates the referenced class to be Drupal\Core\Field\FieldConfigInterface instead of Drupal\field\FieldConfigInterface. The first locks us from altering permissions on fields defined using \Drupal\Core\Field\Entity\BaseFieldOverride (like the promoted field) which was technically possible with the entity-wide version of the module (although to get it to work with the UI, you needed something like https://www.drupal.org/project/base_field_override_ui)

Tests are failing, so will keep reviewing.

Ok, at this point the only test failing is related to the d7 -> d8+ field migration, which checks to make sure that the third-party settings on the FieldStorage match between the d7 and d8+ config. Since this patch changes where the third-party settings for this module live, pushing them onto the field config (field.field) level, it's a valid failure.

That migration process would need to be rewritten to move the settings onto the bundle level, and then this test would need to be updated.

Alternatively, if this is going onto an 8.x-2.x branch, I suppose we could say that branch doesn't support migration from d7, and drop both the migration and drop the test. If we find ourselves beyond end of life for D7 and this hasn't been merged into a 2.x branch, that is the easy solution.

Can I suggest that if we're talking about a new 2.x branch that it might be worth rolling this and the following issue in together:

https://www.drupal.org/project/field_permissions/issues/3110867

The patch on that issue already uses patch #10 from this issue as a base.

I'm attaching a patch that is compatible with the latest dev release and contains a fix to this patch that fails if you're looking at the field in a view (which does not provide a bundle, core issue: https://www.drupal.org/project/drupal/issues/2898635). This should fix https://www.drupal.org/project/field_permissions/issues/3373500 if people are seeing it.

I'll try to get this merged into the new 2.x branch this week or next so we can stop with all the patching.

Patch reroll against 1.3.

And how about a reroll that doesn't cause whitescreens!

And another reroll that lets the field configuration page load.

Reroll diff.

I'm taking steps to move this issue onto the 8.x-2.x branch, which currently matches the 8.x-1.x branch. This per-bundle permissions feature will be the major difference between branches.

Attaching Snapshot PR of the latest version of the Merge Request by mariacha. works with the latest release and tested on D10.

Huh. The last patch doesn't apply in builds because it patches the info file and the info file has extra junk in it from automated D.O. packaging. Here's one usable for those cases.

I'm attaching the interdiff between 42 and 45 (ignoring the .info stuff). It's worth noting that the merge request is slightly different based on the need in https://www.drupal.org/project/field_permissions/issues/2881776#comment-....

It would be good to know if we are still trying to make the per-bundle option something a user can choose to configure. In the current patches all field permissions are per-bundle and none are per-entity.

Here's the interdiff.

Here's a link to this issue's previous comment with helpful d.o markup (so we know the comment #): #2881776-35: Implement field permissions per-bundle (field instance)

I first tried the patch: 2881776-packaged-45.patch to the latest stable version Field Permissions: 8.x-1.3 and it applied cleanly. However, I get the following error when trying to save a new field permission:

The website encountered an unexpected error. Try again later.

RuntimeException: Adding non-existent permissions to a role is not allowed. The incorrect permissions are "create field_sidebar_sections", "edit own field_sidebar_sections", "view own field_sidebar_sections". in Drupal\user\Entity\Role->calculateDependencies() (line 207 of core/modules/user/src/Entity/Role.php).
Drupal\Core\Config\Entity\ConfigEntityBase->preSave(Object) (Line: 179)
Drupal\user\Entity\Role->preSave(Object) (Line: 528)
Drupal\Core\Entity\EntityStorageBase->doPreSave(Object) (Line: 483)
Drupal\Core\Entity\EntityStorageBase->save(Object) (Line: 257)
Drupal\Core\Config\Entity\ConfigEntityStorage->save(Object) (Line: 352)
Drupal\Core\Entity\EntityBase->save() (Line: 609)
Drupal\Core\Config\Entity\ConfigEntityBase->save() (Line: 96)
Drupal\field_permissions\Plugin\FieldPermissionType\CustomAccess->submitAdminForm(Array, Object, Object) (Line: 154)
field_permission_field_config_edit_form_submit(Array, Object)
call_user_func_array('field_permission_field_config_edit_form_submit', Array) (Line: 129)
Drupal\Core\Form\FormSubmitter->executeSubmitHandlers(Array, Object) (Line: 67)
Drupal\Core\Form\FormSubmitter->doSubmitForm(Array, Object) (Line: 597)
Drupal\Core\Form\FormBuilder->processForm('field_config_edit_form', Array, Object) (Line: 325)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 73)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 627)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 181)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle(Object, 1, 1) (Line: 32)
Drupal\big_pipe\StackMiddleware\ContentLength->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 704)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

I then installed the 8.x-2.x-dev module version and tried to apply the patch: 2881776-packaged-45.patch. It couldn't apply.

@gcb is patch in #47 actually working for you? Looking at field_permissions_entity_field_access():

- if (!$field_definition->isDisplayConfigurable($context) || empty($items) || !is_a($field_definition->getFieldStorageDefinition(), '\Drupal\field\FieldStorageConfigInterface')) {
+ if (!$field_definition->isDisplayConfigurable($context) || empty($items) || !is_a($field_definition->getFieldStorageDefinition(), '\Drupal\Core\Field\FieldConfigInterface') || !$field_definition->getTargetBundle()) {


I don't understand how $field_definition->getFieldStorageDefinition() can return \Drupal\Core\Field\FieldConfigInterface.

I did a diff between Merge request !10 and patch from #47, which also indicates that \Drupal\field\FieldStorageConfigInterface is the class to use (xdebug also suggests that):

- if (!$field_definition->isDisplayConfigurable($context) || empty($items) || !is_a($field_definition->getFieldStorageDefinition(), '\Drupal\field\FieldStorageConfigInterface')) {
-+ if (!$field_definition->isDisplayConfigurable($context) || empty($items) || !is_a($field_definition->getFieldStorageDefinition(), '\Drupal\field\FieldStorageConfigInterface') || !$field_definition->getTargetBundle()) {
++ if (!$field_definition->isDisplayConfigurable($context) || empty($items) || !is_a($field_definition->getFieldStorageDefinition(), '\Drupal\Core\Field\FieldConfigInterface') || !$field_definition->getTargetBundle()) {

Besides that difference the info file related stuff and the migration tests, which where removed in MR!10, I did not see other differences.

This patch is the fixed version of patch from #47 (with \Drupal\field\FieldStorageConfigInterface instead of \Drupal\Core\Field\FieldConfigInterface)

Similar to @sassafrass in comment #52, I tried the patch 2881776-55--packaged-45-fixed.patch in the latest 1.3 version. It applied, but then when running the database updates, I'm getting:

  Module              Update ID   Type            Description                                                     
 ------------------- ----------- --------------- ---------------------------------------------------------------- 
  field_permissions   8001        hook_update_n   8001 - Migrates existing third party settings and permissions.  
 ------------------- ----------- --------------- ---------------------------------------------------------------- 
 // Do you wish to run the specified pending updates?: yes.                                                             

>  [notice] Update started: field_permissions_update_8001
>  [error]  Adding non-existent permissions to a role is not allowed. The incorrect permissions are "create field_1", "edit field_2", "edit field_3", "view field_4", ( ...)
>  [error]  Update failed: field_permissions_update_8001 
 [error]  Update aborted by: field_permissions_update_8001 
 [error]  Finished performing updates. 

I also tried with the the 8.x-2.x-dev module version and the patch didn't apply.

Moving back to 8.x-1.x for now, in hopes we can redo the 2.x branch correctly (semantic versioning)

Alright, I've attempted to merge 8.x-1.4 into the work on this branch. There were a LOT of conflicts. I believe my branch is functioning as desired, but I have failed to get the automated tests fixed. I'm not terribly familiar with how these tests work, so I'd love some help getting those running on my branch.

The reason why #55 worked for me and some others might be, that we ran the update hook under Drupal 9. In Drupal 10 and above all permissions in a user role must be defined in a module.permissions.yml file or a permissions callback, linking to the CR Permissions must exist.

Copy of patch from https://git.drupalcode.org/project/field_permissions/-/merge_requests/34... (Merge request !34) attached, to be used safely with composer".

The new MR34 did not merge all 8.x-1.x changes correctly. I updated the original MR10. The automated tests are green again.

here's the latest state of the MR as a patch for stable builds:

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

I removed these commits because I don't think I got it right. There's more going on than simple class changes. After brief initial testing, it seems like the patch from #68 works for us for now.

I merged the current 8.x-1.x version (1.5) into 2881776-consider-implementing-per-bundle: https://git.drupalcode.org/project/field_permissions/-/merge_requests/10/diffs?commit_id=e3c30a47aa8d4cf663d1db2e78be54bc037e7368

The conflicts were pretty straightforward. Tests are still good.

If you relied on a dynamic URL to patch your installation and don't want to update yet, you can download the previous diff here: https://git.drupalcode.org/project/field_permissions/-/merge_requests/10/diffs.diff?diff_id=1866781

I was testing patch #68 and ran into a funny issue. Notably I am only experiencing this issue with my admin permissions config, and it does not appear to have any functional consequences. It is just an issue with inconsistency in my config files.

I am only noticing this right now with 2 fields. One is called field_office_hours. The other is called field_order.

After I installed an configured field permissions with this issue patch (initially #68, but problem persists with latest on MR 10), I exported config and these are the changes I see in my admin config:

--- a/config/sync/user.role.administrator.yml
+++ b/config/sync/user.role.administrator.yml
@@ -1,11 +1,18 @@
uuid: ccb30dce-64b9-4253-b4d1-db37c7162ebc
langcode: en
status: true
-dependencies: {  }
+dependencies:
+  module:
+    - field_permissions
_core:
default_config_hash: deBO6rveioT0_Xi1uj--U3HrKSa0JjKdvdHRKy0rXNo
id: administrator
label: Administrator
weight: -5
is_admin: true
-permissions: {  }
+permissions:
+  - 'create field_order (node.office)'
+  - 'edit field_order (node.office)'
+  - 'edit own field_order (node.office)'
+  - 'view field_order (node.office)'
+  - 'view own field_order (node.office)'

Strange because usually admin permission changes do not get managed in code like this. I did not think anything of it at the time though.

Later, I made more updates to permissions and now see this diff:

diff --git a/config/sync/user.role.administrator.yml b/config/sync/user.role.administrator.yml
index 1515721..db77bc5 100644
--- a/config/sync/user.role.administrator.yml
+++ b/config/sync/user.role.administrator.yml
@@ -11,8 +11,8 @@ label: Administrator
weight: -5
is_admin: true
permissions:
-  - 'create field_order (node.office)'
-  - 'edit field_order (node.office)'
-  - 'edit own field_order (node.office)'
-  - 'view field_order (node.office)'
-  - 'view own field_order (node.office)'
+  - 'create field_office_hours (node.office)'
+  - 'edit field_office_hours (node.office)'
+  - 'edit own field_office_hours (node.office)'
+  - 'view field_office_hours (node.office)'
+  - 'view own field_office_hours (node.office)'

I found that if I go directly to the field_order field edit page and click save, this diff will revert and my git working tree will be clean. But if I do the same thing on the field_office_hours edit page, or click save on the overall permissions page, then this diff comes back. Like I said, I don't think it has any affect on anything functionally, but I do find this behavior a little bit concerning. Has anyone else seen something like this?

These are 2 of 9 fields on this content type. field_office_hours has no custom permissions set, where field_order is restricted to certain roles. There are multiple of both, but I have only seen these 2 permissions sneak their way into the admin permissions config file.

I see this in submitAdminForm (CustomAccess.php):

      if ($role->isAdmin()) {
        continue;
      }

Which makes it look like the issue I am reporting should not be able to occur

FWIW, I restored my admin permissions config file to the state it was before configuring field_permissions (no permissions or dependencies listed). After doing that, I have not been able to reproduce the issue I describe in #71.

Here's a patch for the changes in #69 #70 above

CORRECTED following helpful comment in #75 below.

Thanks for all your work. I was using the patch from #61 (based on MR !34) until 8.x-1.5 broke it, so I decided to switch to MR !10 and reviewed the changes.

The update hook was improved meanwhile, but as we already ran it we do not need to run it again. The rest of the changes are compatible with #61, so switching to MR !10 should be safe.

The patch in #74 seems to refer to #70 (not #69), as it matches diff under https://git.drupalcode.org/project/field_permissions/-/merge_requests/10/diffs.diff?diff_id=1939210 (also safe to be used with composer patch) — which I didn't know before — you get this URL from the MR overview page (https://git.drupalcode.org/project/field_permissions/-/merge_requests/10) by taking the latest "Compare with previous version" link and adding .diff before ?diff_id=1939210 and removing the start_sha parameter.