Problem/Motivation

  1. The FilterFormat config entity currently does not provide config dependencies to its configured roles.
  2. Related to this, when exporting the config (admin/config/development/configuration/single/export) the dumped YAML contains no roles property as the default config provides.

Proposed resolution

The fix of the main issue is coupled with the fix of the 2nd because when calculating dependencies we want to rely on the $format->roles array.

Add them, like many other places in core has them already.

Remaining tasks

Fix the reverse flow: When deleting a format the roles granted with the permission to use that format should drop the stale permissions. This is fixed in #2571235: Roles should depend on objects that are building the granted permissions.

User interface changes

None.

API changes

None.

Data model changes

The FilterFormat object will store the roles granted with "use text format {format}" permission.

Comments

dawehner created an issue. See original summary.

claudiu.cristea’s picture

Assigned: Unassigned » claudiu.cristea

Let's see...

Wim Leers’s picture

Issue tags: +Configuration system
claudiu.cristea’s picture

Assigned: claudiu.cristea » Unassigned
Status: Active » Needs review
FileSize
5 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,153 pass(es), 91 fail(s), and 0 exception(s). View

Patch.

catch’s picture

Should it also do onDependencyRemoval()? I can see a site having 5 roles, three having access to the format, then a role gets deleted, the format shouldn't.

catch’s picture

Priority: Normal » Major
Issue tags: +rc target

Also missing config dependencies are at least major.

dawehner’s picture

+++ b/core/modules/filter/src/Entity/FilterFormat.php
@@ -427,6 +428,27 @@ public function onDependencyRemoval(array $dependencies) {
+    // The permission name is FALSE when this is a fallback text format and that
+    // doesn't depend on any user role.
+    if (!$permission = $this->getPermissionName()) {
+      return $this->dependencies;
+    }
+    $roles = user_roles(TRUE, $permission);

That seems to be the wrong way round. Permissions are configured by those roles, not the other way round, see Filterformat::postSave(), so you should not rely on the configuration of the permissions IMHO

Status: Needs review » Needs work

The last submitted patch, 4: 2569741-4.patch, failed testing.

claudiu.cristea’s picture

@dawehner,

Permissions are configured by those roles, not the other way round, see Filterformat::postSave(), so you should not rely on the configuration of the permissions IMHO

Not that simple. In fact what you see in ::postSave() is a hack to import permissions when the config object is first time loaded from the default config. After that permissions are not stored in the config anymore, but they are stored as any other permission, in the role. Looking more to this I see that this is buggy while when exporting the config (admin/config/development/configuration/single/export) the dumped YAML contains no roles property as the default config provides (see core/modules/filter/config/install/filter.format.plain_text.yml).

I propose to fix this hack (and the bug) in this issue:

  1. Replace roles property from the default config YAML with normal dependencies definition, including dependencies to 'anonymous' and 'authenticated'.
  2. On ::postSave(), for $update == FALSE, check if roles specified in dependencies have proper permissions for this text format. If they miss, add the permissions. In fact this happens only the first time when a text format config default is imported. This is replacing what now does ::postSave().
  3. Implement onDependencyRemoval() and return $changed = TRUE (see EDIT).
  4. ::calculateDependencies() should add also dependencies to 'anonymous' and 'authenticated'.

@catch:

Should it also do onDependencyRemoval()? I can see a site having 5 roles, three having access to the format, then a role gets deleted, the format shouldn't.

No, we don't need to update anything onDependencyRemoval() because the permissions are stored in roles. When a role gets deleted, its permissions to all text formats are deleted too. But, it's true, we need to implement onDependencyRemoval() and return $changed = TRUE in such cases to signal that the format needs to save and recalculate the dependencies (see EDIT).

EDIT: @catch, in fact we don't need onDependencyRemoval() because, if a role is deleted, on removal, the dependencies are recalculated and there will be no unresolved dependency that triggers the entire text format deletion.

claudiu.cristea’s picture

Thinking more on #9, I like the way now the .yml is written by explicitly defining the roles. This is more meaningful for module developers. But what about export? I think that at the time when this piece of code was written the config export was treated as "export the whole site config". And that assured the consistency because on an export also the roles & permissions would have been exported. But this, I think, is no more true. We need to re-add the roles array on export in ::toArray().

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
2.42 KB
4.87 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,260 pass(es). View

The export of "roles" property is not handled here. I will open a separate issue for that, if case. In #9 I responded to problems raised in #5 and #7.

We have to clarify if we store the "roles" value inside the text format and if we export that value. If yes, I will open a followup for that.

alexpott’s picture

Status: Needs review » Needs work
Issue tags: +missing config dependencies

Nice find.

I agree with previous comments that we need to implement onDependencyRemoval

+++ b/core/modules/filter/src/Tests/FilterSettingsTest.php
@@ -62,4 +63,78 @@ function testFilterDefaults() {
+    $roles[1]->delete();
+    $format->save();

At this point you need to reload the $format - because I think it would be deleted in the background.

claudiu.cristea’s picture

@alexpott,

At this point you need to reload the $format - because I think it would be deleted in the background.

No, its not I explained in #9 (see EDIT at the bottom) why. Also I'm checking this in test. But anyway, after discussing with @dawehner on IRC, we'll fix the "roles" property here too by storing the roles along with the text format and keep it in sync with role permissions. This would add more consistency to text format and would solve the export bug that I mentioned in #9.

alexpott’s picture

Status: Needs work » Needs review
FileSize
920 bytes
5.13 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,281 pass(es), 8 fail(s), and 1 exception(s). View

See the changes to the test. This will fail.

Status: Needs review » Needs work

The last submitted patch, 14: 2569741-14.patch, failed testing.

The last submitted patch, 14: 2569741-14.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
12.67 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,434 pass(es), 2 fail(s), and 0 exception(s). View
11.46 KB

This is solving also the bug explained in #9 by storing roles also inside the text format config entity.

But now I found that the reverse is also true: when a text format is deleted, the roles granted with permissions to use that format must update and remove the stale permissions.

EDIT/ADD: In HEAD, the sync of role permissions is done in the UI. I cleaned that, now everything is handled in the API.

Status: Needs review » Needs work

The last submitted patch, 17: 2569741-16.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
13.78 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Setup environment: Test cancelled by admin prior to completion. View
4.42 KB

Fixed the failing test.

Status: Needs review » Needs work

The last submitted patch, 19: 2569741-19.patch, failed testing.

The last submitted patch, 17: 2569741-16.patch, failed testing.

The last submitted patch, 19: 2569741-19.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
12.84 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,439 pass(es). View
10.18 KB
jibran’s picture

+++ b/core/modules/filter/filter.install
@@ -0,0 +1,19 @@
+  foreach ($factory->listAll('filter.format.') as $key) {

Do we have to update any filter format config file ?

claudiu.cristea’s picture

As you can see, yes.

jibran’s picture

Oh I meant static yml files in core.

claudiu.cristea’s picture

#2571235: Roles should depend on objects that are building the granted permissions must solve the reverse situation: when a text format is deleted, user_role entities involved must update.

claudiu.cristea’s picture

Issue summary: View changes
FileSize
9.86 KB
18.08 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,622 pass(es), 1 fail(s), and 0 exception(s). View

Added support for the reverse flow. The role is directly granted with the permission to use a text format. The text format 'roles' property and the dependency to that role should be updated accordingly.

Updated the issue summary.

Status: Needs review » Needs work

The last submitted patch, 28: 2569741-28.patch, failed testing.

The last submitted patch, 28: 2569741-28.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
18.53 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,775 pass(es). View
467 bytes

Fix the kernel test.

catch’s picture

Right I think missing configuration dependencies is something we should aim to fix during rc. This was previously release blocking since it leads to data integrity issues but it's clear there are some we didn't catch in the previous sweeps.

tim.plunkett’s picture

I need to dig up the original issues, but this was done very purposefully by @sun.

claudiu.cristea’s picture

@tim.plunkett, can you review also the reverse issue? When a FilterFormat (but can be any config providing dynamic permissions -- e.g. a node_type) is removed update the roles granted with permissions to interact with that config to clear those permissions: #2571235: Roles should depend on objects that are building the granted permissions.

catch’s picture

Just a note I tagged this rc target due to the missing config dependencies.

afaik this leads to config keys getting stale but no actual data loss or runtime errors at all.

jhedstrom’s picture

Patch from above no longer applied, this is just a reroll--I'm unclear on what remains to be done here, since the remaining task is in another issue.

Status: Needs review » Needs work

The last submitted patch, 36: 2569119-36.patch, failed testing.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
2.05 KB
20.2 KB

This should fix the fails.

alexpott’s picture

Hmmm... if we do this and #2571235: Roles should depend on objects that are building the granted permissions won't we end up with circular dependencies. Thinking about this some more... the filter format should not be dependent on a role. A role does not need to exist for the filter format to exist. If the role has a permission "use text format X" that that role is indeed dependent on the filter format - BUT the issue here is that all such dependencies are soft - deleting the filter format should not delete the role.

The export bug mentioned in the issue summary is not real - if you are exporting the filter format and you what the roles to have the permission you need to export the roles too.

xjm’s picture

Issue tags: -rc target

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.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.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.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.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.