Make it so Features only touch permissions for roles explicitly managed in each feature

The += operator does not appear to add unique elements to the $managed_roles array. As a result, $managed_roles is stuck on its original value. Any roles that aren't listed in the first iteration of the foreach loop are ignored, which causes the feature to be stuck in permanent "Overridden" status until the user manually changes the permissions to match what are found in the feature.

The problem is the added line here:

     foreach ($defaults as $permission) {
+      $managed_roles += $permission['roles'];
       $perm = $permission['name'];

After playing around with this for a while and not having much finding a PHP function to replace the += operator, I came up with this:

      foreach ($permission['roles'] as $permission_role) {
        if (!in_array($permission_role, $managed_roles)) {
          $managed_roles[] = $permission_role;
        }
      }

All this may be moot, however, as I can't think of a use case in which any permissions should actually be dropped. Can you give an example of a role that isn't "explicitly managed in each feature"?

I've looked at this issue and am realizing that the current export format of the user permissions is broken. Currently, it produces an array like this:

  // Exported permission: create features_test content
  $permissions[] = array(
    'name' => 'create features_test content',
    'roles' => array(
      0 => 'admin',
      1 => 'anonymous user',
      2 => 'authenticated user',
    ),
  );

Which stores only positive occurrences of the role. From that, Features is "assuming" that the remaining roles on the site are negative occurrences of the permission, but that's clearly a bad assumption.

This patch switches the export format to explicitly state negative occurrences from the feature creating site, which also means we no longer need to assume which roles should have the permission revoked on the new site.

  // Exported permission: create features_test content
  $permissions[] = array(
    'name' => 'create features_test content',
    'roles' => array(
      'admin' => TRUE,
      'anonymous user' => TRUE,
      'authenticated user' => TRUE,
      'manager' => FALSE,
    ),
  );

Testing/review of patch would be appreciated (especially whether it meets the use case defined above.)

This is related to #553866: Move role creation to hook_user_default_roles(), so we should think about both these issues before coming up with a solution for either.

#553866: Move role creation to hook_user_default_roles()

With that issue solved, this issue still applies. In particular in 7.x I don't think the export should cover the admin role - as it may differ on the sites and is already handled by core. But without a patch like #3 the admin role wouldn't receive the permission.

#3 looks like a good approach to me, additionally we might want to exclude the admin role from the export too.

I think there should be a way to apply admin permissions - if nothing else takes care of it (and so it seems), features should include admin in the export.

subscribing

The feature should really only touch permissions that are associated to roles handled by that feature. In my case I create feature modules for content types and would like to handle all permissions associated to that specific content type plus some global ones (e.g. translate content) for editor roles. Unfortunately roles by all other content type feature modules are also listed for the global permissions in the export... and this results in a feature module which is not really stand-alone anymore.

Thanx in advance & cheers

hctom

Hi.
Refreshing the patch (for D6 only at the moment).

After applying the patch each feature can provide its own permissions per role without being marked as overriden if other features uses the same permissions for different roles.

Even though the following part of the patch looks somehow scary (at least for me ;)) I didn't notice any side-effects (yet):

<?php
    // Get the roles from component "user_role".
    $feature_info = features_get_features($module);
    $roles = isset($feature_info->info['features']['user_role']) ? $feature_info->info['features']['user_role'] : array();
    $roles = array_combine($roles, $roles);
?>

Updated patch for Drupal 6 and patch for Drupal 7.

With this patch is it completely impossible to export permissions for Drupal's built in roles, anonymous user and authenticated user?

Hm, maybe.
Unfortunately the path does not solve all the problems, exporting roles and permissions in different features gives.

Seems we should re-think the way Features exports permissions in general.

Setting it to needs work (and bumping it to 7.x) based on above two comments. Also, this would break my site's permissions management, where permissions and and roles are in separate modules completely, which is a valid use case that should be supported imo.

From a brief look, permissions export needs a relook, like stBorchert said.

Accidentally changed type o.O

Bumping to 7.x since (I assume) that's where active development is.

I think what we really need is a way to export permission/role/access tuples (where "access" is true/false i.e. grant/deny); for example:

administer nodes, authenticated user, grant
administer nodes, anonymous user, deny
post comments, authenticated user, grant

This way we can export permissions more intentionally. Technically I generally wouldn't bother to export the "deny" ones, I would think, but it seems like a logical way to do this.

The problem is, what is the UI for this? The thought of seeing the sea of permissions checkboxes within the Features interface pretty much terrifies me. This really warrants a JavaScript based "drill down" interface, which unfortunately would be a rather huge special case within the features UI (I'm not yet familiar with the features UI and API for components but I'm guessing this would be difficult to implement). Perhaps we should develop a way to do this through code first.

Another idea is to have a new export called "role-permissions" which exports ALL the permissions for a single role (again, explicitly as grant/deny). That'd be fairly easy to implement, I'm thinking, but it would be partly redundant with the current permissions exportables so it feels a bit kludgey. On the other hand, for building reusable features (and even site-specific ones) it would be VERY useful.

One way or another we need a better way to export permissions.

Thoughts?

For D7, this simple patch seems to work for me.

Patch in #17 for d7 worked for me too, though I am not sure it answers the problem for all users.

I would like a permission to be controlled by my feature where it affects specified roles, but also for my feature to stop other features to controlling that permission for other roles. I do not believe that this use case is covered by the simpler patch, which only ignores other roles settings for the permission is they do not have that permission.

In response to ui considerations: I elected to run a couple of snippets through devel/php to create a feature with all the permissions for specified roles. These snippets could be a response to a form submit outside of the features ui in order to create a feature which captures permissions only for specified roles. With features patched so that it will only look at the status of permissions for roles exported by the feature, I could monitor the override status etc and update/recreate using features UI, but initially capture with a seperate form/module.

This approach is obviously just a crude response to the problem, but maybe it could form the basis of a solution?

ubersimple devel snippets included in case of use

The patch in #11 did not apply for me so I have recreated it against Features-7.x-1.x(dev).

I also had an error when there were features present in the features ui that didn't have roles included, so put in fix for that.

I have not fully evaluated the method, but it seems to work for me.

It is a real pity that exporting permissions in the manner allowed by this patch does not allow them to be stored for the default drupal roles (anonymous/authenticated).

I would really like to see this method of exporting permissions made more complete/switchable and included in features if that were deemed appropriate by the maintainers (and if anyone has any ideas how it might be accomplished).

Haven't fully been paying attention to this, but my guess is that this patch would be better as a contrib module as alternate for those that want to manage their permissions like this vs. the current method.

I do not use roles/permissions like this so it'd break my current features.

hefox is correct. I don't see applying this to Features directly. There are many cases where the "roles" and "permissions" are exported to different features and the changes proposed here would break a lot of existing sites.

User Permissions are really borderline "content" and fall into that area where Features wasn't really designed. It wouldn't be hard to create a new module that has it's own user permission exportable component that handles the export/import in a different way.

So closing this for now.

I totally agree that this way of using roles and permissions with features should be optional, and that it should be a separate module that enables this.

I guess the separate module could just come out of a patched version of features.user.inc (as above) that exposes an additional permissions option in the features UI?