Problem/Motivation

Define permissions as unique to individual modules (a larger decision remains about whether this approach is the right one); see href="#comment-3239906">#58.

Proposed resolution

Small patch committed that matches current D8 schema; see href="#comment-3306614">#74. Several people propose closing
this issue and reporting unresolved concerns in new, more focused issues.

Remaining tasks

Add a new issue for each unresolved concern; then mark this issue
closed.

Original report by href="/user/41017">alexanderpas

found while updating #582478: testing admin/config/module for HEAD

Fatal error: Class name must be a valid object or a string in
/var/www/alexanderpas/drupal/includes/common.inc on line 5985

how to replicate:
- enable taxonomy
- disable taxonomy
- uninstall taxonomy

Call Stack:
0.0004 62204 1. {main}() index.php:0
0.7727 10559016 2. menu_execute_active_handler() index.php:22
0.7998 11336408 3. call_user_func_array() includes/menu.inc:454
0.7999 11336772 4. drupal_get_form() includes/menu.inc:0
0.7999 11337328 5. drupal_build_form() includes/form.inc:77
0.8031 11356524 6. drupal_process_form() includes/form.inc:198
0.8120 11465844 7. form_execute_handlers() includes/form.inc:560
0.8121 11466124 8. system_modules_uninstall_submit()
includes/form.inc:915
0.8252 11718336 9. drupal_uninstall_modules()
modules/system/system.admin.inc:1180
1.6985 12412940 10. module_invoke_all() includes/install.inc:711
1.7001 12413392 11. call_user_func_array() includes/module.inc:509
1.7001 12413392 12. user_modules_uninstalled() includes/module.inc:0
1.7001 12413392 13. module_invoke() modules/user/user.module:3205
1.7001 12413428 14. call_user_func_array() includes/module.inc:487
1.7001 12413428 15. taxonomy_permission() includes/module.inc:0
1.7002 12413428 16. taxonomy_get_vocabularies()
modules/taxonomy/taxonomy.module:19
1.7002 12413428 17. taxonomy_vocabulary_load_multiple()
modules/taxonomy/taxonomy.module:579
1.7002 12413428 18. entity_load() modules/taxonomy/taxonomy.module:857
1.7002 12413428 19. entity_get_controller() includes/common.inc:5974

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexanderpas’s picture

BTW: drupal is giving me:
Notice: Undefined index: taxonomy_vocabulary in entity_get_info() (line 5936 of includes/common.inc).
right afterwards!

hefox’s picture

Just started playing with d7 yesterday, so cannot give a solution to this, but it looks like the issue is that the entity taxonomy_vocabulary cannot be loaded anymore in user's hook_modules_uninstalled (user_modules_uninstalled)

So the issue as far as I can see is that drupal uninstall modules sets modules as uninstalled before hook_modules_uninstalled is called (http://api.drupal.org/api/function/drupal_uninstall_modules/7).

ie what happens
1) drupal_uninstall_modules
2) Module set as uninstalled
3) something is resetting the module_implements cache so taxonomy is removed from implementating hooks
4) hook_modules_uninstall module_invoked_all-ed
5) user_modules_uninstall call
6) Tries gathering taxonomy permissions.
7) taxonomy_get_vocabularies called
8) tries doing entity_load, and enitity_load_info and taxonomy no longer implements entity_load_info to it's knowledge so nothing is loaded // can't track down what is doing that exectly, my guess the cache wasn't there so it tried building it and module was already disabled
so on the lines

    $type_info = entity_get_info($entity_type);
    $class = $type_info['controller class'];
    $controllers[$entity_type] = new $class($entity_type);

$type_info is blank so
$controllers[$entity_type] = new null($entity_type);
fails and produces the fatal error.

So, probably not a taxonomy.module issue and a bit more complicated. Perhaps the 'this is uninstalled' should be set after the hook is implementated?

hefox’s picture

Er, I thought of something else that may be problamatic. if taxonomy_get_vocabularies is not cached and uses the database to get the vocabularies, wouldn't it be problamic for the tables to not be there when taxonomy_get_vocabularies is called?

hefox’s picture

additional to above; even if it's called before the the module's uninstall is called the module is disabled so it won't show up as implementating any hooks, which is part of the problem... right? @.@ so to get this combination to work, the module would have to be enabled to figure out what vocabularies exist? Mm i'm probably missing somethings.

Hm looking at http://api.drupal.org/api/function/module_list/7 there is a way to specifiy what modules to list, however that isn't saved and isn't used in module_implements.

hefox’s picture

Title: Fatal error: Class name must be a valid object or a string » user_modules_uninstalled doesn't remove/errors out for dynamically created permissions
Component: taxonomy.module » user.module

The fatal error is not really the issue here, other than there should probably be separate issue about entity load not making sure entity type exists.

The main issue here is that uninstall would not work any because the database tables do not exist.

We were discussing various ways:

Adding a pre-uninstall would not work because the module is not enabled; In order for hook_entity_load_info to be called the module would need to be enabled, which seems very very un-ideal. Tweeking module_list also seems a bit quirky.

One way could be have modules uninstall their own permissions if dynamic. That would involve them testing in their hook_permissions to make sure they won't error out if user_modules_uninstall still calls their hook_permissions, and for they need to implement their uninstall to remove the permissions.

Diffing the permissions stored with permissions of module invoke all permissions would end up removing any disabled permissions; again, not ideal.

Another option is to store the module provided with the permission ; but since permissions are repeated for role that seems ugly also.

Another option, similar, is to create a new permissions => module table used soley for uninstalling, which also seems unideal.

For the two above uninstall would be deleting from permissions where module = ones being uninstalled.

catch’s picture

Title: user_modules_uninstalled doesn't remove/errors out for dynamically created permissions » user_modules_uninstalled() calls hook_permission() for already uninstalled modules

This looks impossible to fix to me, at least not without huge refactoring, so I don't see an option other than rolling back user_modules_uninstalled().

One thing we could do is provide a helper function which takes the result of hook_permission(), so it'd look like

function taxonomy_uninstall() {
  user_uninstall_permissions(taxonomy_permission());
}

Then it would be up to taxonomy.module it ensure its hook_permission() worked when its disabled.

agentrickard’s picture

Title: user_modules_uninstalled() calls hook_permission() for already uninstalled modules » entity loading in hook_permission can break the uninstall process
Component: user.module » taxonomy.module

I think we're blaming the wrong system here, and have changed the issue accordingly.

The uninstalled hook went in before the entity loading, so that looks like a regression. When hook_modules_uninstalled() runs, we can be certain that .module and .install files have been loaded.

The fix would be to make sure that any module that does not encapsulate hook_permission() within its .module file makes sure to load its required file(s) and classes.

So I think this is a bug in taxonomy module, introduced by the entity loading system and its cache. How it is that a class contained within taxonomy.module is not available makes no sense to me.

We may also need to document this behavior in hook_permission().

agentrickard’s picture

We could also consider moving hook_uninstalled() invocation to before the actual uninstall command. I don't know why we didn't do that the first time...

hefox’s picture

if the taxonomy_permissions were changed to just use database queries, then moving hook_modules_uninstall before the hook_uninstall would help I believe. Anything that relies on the module being enabled would not; going to a burden on other modules, since there's likely going to be a few other modules that rely on dynamic information though (not sure if it's in d7, but cck's content permissions for example).

Or as catch says, having taxonomy uninstall it's own permissions; but then every module would have to uninstall their permissions, or else taxonomy permissions would be try to be uninstalled twice?

catch’s picture

Title: entity loading in hook_permission can break the uninstall process » user_modules_uninstalled() calls hook_permission() for already uninstalled modules
Component: taxonomy.module » user.module

This isn't about the entity loader, this is about user_modules_uninstalled() calling an API function for a module which isn't enabled and has no database tables - the same bug will appear for any module with it's own storage and dynamic permissions based on that. If you were to apply one of the patches on #375397: Make Node module optional then try to uninstall node module, I'm pretty sure the same error would arise for the content types table. So what you're saying is that modules can never have dynamically created permissions depending on their own APIs, and if they do, that's a regression? Moving this back to user.module.

We may be able to fix this by moving the hook invocation, and also making sure taxonomy module only runs direct queries, however that will only be a workaround for the larger issue, which requires extensive documentation in hook_permission() if that's what we end up doing, and really isn't an acceptable fix (although at this stage of the release cycle we might not have much choice).

agentrickard’s picture

Issue tags: +D7DX

@catch - Yes, I see that now.

As @hefox said, the other possibility is registering the module that assigned the permission. (This would alter the {role_permission} table or force the introduction of a {permission} registry table or even a variable.) Then you have the problem of multiple modules using the same permission.

But if one module is dependent on the permissions of another module (which I generally think is bad practice), then the parent module needs to be listed as a dependency in the child module's .info file. So I think we could get away with a registry.

The whole point, originally, was to be able to clean up permissions storage without adding another burden to module developers.

agentrickard’s picture

Status: Active » Needs review
FileSize
1 KB

Well, to keep moving forward, here's another approach for discussion. Here we just gather the existing permissions and delete any that don't exist.

Problem is, this would catch disabled modules as well as uninstalled modules. Now, I'm ok with that approach, but if we go that route, then this needs to move to user_modules_disabled().

hefox’s picture

Patch works, taxonomy uninstalls, permissions are removed from the role_permission table.

Moving it to disable will make it less usable then it currently is? I believe current setup permissions past disable stays until permissions are resaved? I think that would be quite annoying, so uninstall is likely the better place, but will effect disabled modules. :/

Dries’s picture

Mmm, the patch in #12 is cool. Maybe we should move that out of the user module and always execute that as part of system module or something? Could be a nice clean-up as it probably allows us to delete some code, and would also take care of 'broken' modules.

agentrickard’s picture

@Dries

The real question is: When do we clear permissions? When a module is disabled or when it is uninstalled?

Once we settle that, fixing this is easy.

@hefox

If we aren't going to destroy permissions when a module is disabled, then we need some means of storage to note the different state. A variable might be the easiest way, but I don't much like that approach.

We could also add an 'active' column to 'role_permission', but that also seems clunky.

moshe weitzman’s picture

To me, its pretty obvious that we remove perms on uninstall, and not on disable. Thats the whole point of distinguishing the two steps. Its not a big deal though.

catch’s picture

I also think uninstalled is the proper place, but it's nice to have the automatic removal and without adding a module or status column to role permission don't really see a way around it. If we make sure the admin role always gets reassigned permissions that might help the annoyance factor a bit. Or would it be a big deal to add the module column and delete based off that? Then we could keep it in uninstalled, inclined to say tough luck for bad namespacing or undeclared dependencies

agentrickard’s picture

@catch

If a module relies on permissions set by another module, then a dependency must be declared. Failure to do so is a bug in the contributed module.

@moshe

I suppose I agree, but that means we have to store the declaring module in {role_permission} or a parallel table, and the current hook_permission() implementation would need a rewrite, since module_invoke_all() is no longer sufficient.

catch’s picture

@agentrickard - sorry that was me agreeing - I meant tough luck to people who don't declare dependencies.

I think doing this in _disabled() is the best we can do - for D8 we should look at the 'module' column in role_permission. The code for adding permissions to the admin role will need to move into hook_modules_enabled() if it isn't already.

agentrickard’s picture

As it happens, I already have a D7 module for testing/maintaining that admin role case. http://drupal.org/project/secure_permissions

agentrickard’s picture

We could, of course, only _wipe_ unused permissions on uninstall (though it would wipe all inactive module permissions anyway). This might give site admins some control over not destroying permissions.

moshe weitzman’s picture

Changing from module_invoke_all to module_implements is about 5 lines of copy/paste. adding a module columnto users_role is another few lines in user_schema. I really don't think we are talking about hard work here. thats the way to go, IMO. these are not API changes. its all internal stuff.

Dries’s picture

I agree with all of the comments above. That said, if this is considered to be a critical bug, and API changes are required to fix a critical bug, then I have no problem making those changes.

agentrickard’s picture

@moshe

Not really disagreeing. I can roll a quick patch later. And I suppose it is critical, since its currently broken.

agentrickard’s picture

And a new patch.

This contains a new {role_permission}.module field and and update_N for user.module to track the needed data.

moshe weitzman’s picture

+++ modules/user/user.install	2 Feb 2010 22:59:55 -0000
@@ -65,6 +65,13 @@ function user_schema() {
+        'default' => '',

is there a use for NOT NULL with default''. i think the default line can go.

+++ modules/user/user.module	2 Feb 2010 23:00:00 -0000
@@ -2534,6 +2534,36 @@ function user_role_delete($role) {
+  $permissions = &drupal_static(__FUNCTION__);

how often is this function called on a page/ does it really need a static cache?

Powered by Dreditor.

catch’s picture

We should also add taxonomy to the install/uninstall tests if they already exist, or open an issue for that if they don't yet.

Dries’s picture

+++ modules/user/user.module	2 Feb 2010 23:00:00 -0000
@@ -2534,6 +2534,36 @@ function user_role_delete($role) {
 /**
+ * Determine the module that a permission belongs to.
+ *
+ * @param $permission
+ *   Optional permission name; returns the name of the module.
+ *   If NULL, all modules are returned.
+ * @return
+ *   An associative array in the format $permission => $module.
+ *   Or a string indicating the name of the module for a single permission.
+ */
+function user_get_permissions_by_module($permission = NULL) {
+  $permissions = &drupal_static(__FUNCTION__);
+  if (!isset($permissions)) {

First, it is not the end of the world, but this API is slightly confusing for me. When I read "user_get_permissions_by_module($name)" I was expecting $name to be the name of the module. Unfortunately, I can't think of a better name.

Second, we already have existing helper functions that use a different scheme. For example, user_role_permissions($roles = array()) returns the permissions for one or more roles. Maybe we should call it user_module_permissions()? Not sure that would be better but it would provide some extra consistency.

Thoughts?

agentrickard’s picture

@moshe

When using the permissions API calls (like http://api.drupal.org/api/function/user_role_grant_permissions/7), this function can be called multiple times, so a static seems prudent. Maybe not a drupal_static(), but a static.

The schema was copied from existing tables inside user.install. See {authmap}.module definition. (Which, btw is set to varchar 128 and is a bug, since other tables use varchar 255).

@Dries

The function name is a tough one. We could, in theory split into two functions. Other names:

user_permission_module()
user_permission_module_lookup()
user_permission_get_module()

I think I like the last one.

catch’s picture

user_permission_get_module() works for me.

moshe weitzman’s picture

Surely inserting permissions is an extreme edge case. I have no heard justification for static cache yet. I suggest removing it. Caches come with mental overhead and code overhead. One has to think about when they get stale which in turn requires a clearing API.

agentrickard’s picture

@moshe

1) User submits the permissions form after adding two roles and lots of permissions (100 per role, which is not outlandish).

2) user_role_grant_permission() runs the new callback 200 times and calls module_invoke_all('permission') 200 times.

3) Server cries. Hell is raised by users.

Since module_invoke_all() is not static caching, we need one here, too. Either in the lookup function or in user_role_grant_permission().

moshe weitzman’s picture

I doubt any server would cry about running this new function 1000 times in row. I would be shocked if that took more than .5 seconds. I think .05 is more likely. There's no DB access and I am now of the mind that you add caching when you have a demonstrated need, not "just in case".

Anyway, the only reason this has to call so many times is that it accepts only one permission at a time. If you accept multiple permissions and return an array back to the caller, the number of loops drastically reduces.

Sorry to be suck a pain here. I mean the best for drupal.

catch’s picture

taxonomy_permission() does query the database for vocabularies, I'm not sure if that caches internally somewhere though - it's likely the caching belongs in there though if it doesn't. Same for node types (which must be static cached so not an issue). Ironically these are why we have this issue in the first place...

agentrickard’s picture

Alright. /me bows to moshe.

moshes's approach means, to me, that you can only query for _all_ permissions, so the function now simply returns an array of all active permissions. It is now up to the calling function to behave appropriately.

This simplifies the code quite a bit.

moshe weitzman’s picture

Oh yeah, thats much better. I hadn't really thought of just letting the caller deal with it.

IMO, that function can now simply be user_get_permissions().

agentrickard’s picture

I like having modules in the function name somewhere. It tells me why we're using this instead of module_invoke_all().

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

OK

Dries’s picture

Status: Reviewed & tested by the community » Fixed

This looks great to me too. Committed to CVS HEAD.

catch’s picture

Status: Fixed » Reviewed & tested by the community
FileSize
717 bytes

This broke the upgrade path. One character patch (s/permissions/permission).

See http://drupal.org/node/563106#comment-2565854 for the bug report. ctmattice1 should get a commit credit for so diligently testing the upgrade path this past week

catch’s picture

Status: Reviewed & tested by the community » Needs review

While this is trivial I didn't actually mean to mark RTBC.

Dries’s picture

Status: Needs review » Fixed

Committed to CVS HEAD.

catch’s picture

Status: Fixed » Active

Also broke the installer, I get this with minimum profile:

Notice: Undefined index: use text format 1 in user_role_grant_permissions() (line 2610 of /home/catch/www/7/modules/user/user.module).
SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'module' cannot be null 
catch’s picture

Status: Active » Needs review
FileSize
901 bytes
catch’s picture

Status: Needs review » Needs work

No, not that at all.

catch’s picture

Status: Needs work » Fixed

Actual bug here, this patch just found it: #706558: Minimal profile is broken squared

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

David_Rothstein’s picture

This patch seemed to lead to a bug where user_role_grant_permissions() is now less flexible in terms of what you can call it with. See #737816: user_role_grant_permissions() throws PDOException when used with non-existent permissions (e.g. a permission for a disabled/uninstalled module)

salvis’s picture

Status: Closed (fixed) » Active

In #35, which was committed in #39 there's

 function user_role_grant_permissions($rid, array $permissions = array()) {
+  $modules = user_permission_get_modules();
   // Grant new permissions for the role.
   foreach ($permissions as $name) {
     db_merge('role_permission')
       ->key(array(
         'rid' => $rid,
         'permission' => $name,
+        'module' => $modules[$name],
       ))
       ->execute();
   }
 }

Shouldn't this have been

 function user_role_grant_permissions($rid, array $permissions = array()) {
+  $modules = user_permission_get_modules();
   // Grant new permissions for the role.
   foreach ($permissions as $name) {
     db_merge('role_permission')
       ->key(array(
         'rid' => $rid,
         'permission' => $name,
       ))
+      ->fields(array(
+        'module' => $modules[$name],
+      ))
       ->execute();
   }
 }

instead?

I just had a duplicate key error which was probably due to a record differing only in 'module'. The committed db_merge then tries to insert a new record, which causes a conflict, because the table's primary key is {'rid', 'permission'} only.

After I changed the function, it works fine now.

BenK’s picture

Subscribing... I'm experiencing the same duplicate key error described in #49.

salvis’s picture

Status: Active » Needs review
FileSize
530 bytes

Here's the patch.

BenK’s picture

+1 for salvis' patch in #51... I've tested it and everything works great.

Without the patch, I was getting a duplicate key error when I tried to save the site permissions page. As a result, I couldn't change permissions on the site. With the patch the error goes away and the permissions page functions properly.

Would love to see this committed as soon as possible...

Cheers,
Ben

lotyrin’s picture

Re #52: Is this RTBC then?

Stevel’s picture

This is probably caused by a permission being declared by two modules, which should not happen. You should probably identify the modules defining the duplicate permissions and file a bug against those modules.

Having duplicate permissions gives trouble when uninstalling the module listed in the database, removing the permission from all roles.

Apart from the above, this patch certainly does away with the error message on the site, but I'm not sure if this is the proper solution.

salvis’s picture

Unfortunately, I don't remember which permission was giving the trouble, and it's impossible to reconstruct. This happened on a test system which has only a handful of contrib modules installed. I've checked these and found no duplicate hook_permission() definitions. It might be due to updating the D7 core version on the fly without reinstalling from scratch (which is not supported, I know), but the current code of user_role_grant_permissions() is still buggy, IMO.

Are you suggesting to add diagnostic code to core to check for this condition and generate a specific error message, something like

"Permission 'xyz' was formerly assigned by module 'A' and is now being assigned by module 'B', which may be a fault of one of these modules."

I doubt this would be acceptable.

Or are you suggesting to keep the buggy code in core as it is?

BenK’s picture

I agree with salvis' comments in #55. I, too, experienced this problem on a test system with only a couple of contrib modules installed. There were no duplicate hook_permission() definitions.

--Ben

Stevel’s picture

Status: Needs review » Reviewed & tested by the community


"Permission 'xyz' was formerly assigned by module 'A' and is now being assigned by module 'B', which may be a fault of one of these modules."

This same wording applies to permissions that are not duplicate, but have moved between modules, which is something that should be possible, so marking this patch RTBC.

sun’s picture

*cough*

This is probably caused by a permission being declared by two modules, which should not happen.

That's a major API change, isn't it? There are many modules in contrib that use and implement the same permission name - some of them even have to, due to optionally supported interdependencies. The situation occurs whenever your module wants to re-use a permission that is normally provided by a "more common" module, but which is optional and may not be enabled. For instance, PHP module's "use PHP for settings" permission.

I doubt we are prepared for this 'module' column yet.

Stevel’s picture

Title: user_modules_uninstalled() calls hook_permission() for already uninstalled modules » Permissions are assumed to be unique among modules

So the issue is a bit bigger than this one symptom we're fixing here: looks like all the code assumes a permission belongs to a single module. As sun pointed out, we cannot assume this.

  • There is a single module listed with each permission in {role_permission}
  • user_permission_get_modules returns an associative array $permission => $module (mapping each permission to a single module)
  • user_modules_uninstalled revokes permissions of uninstalled modules based on the module column in role_permission, without checking that a permission isn't declared somewhere else
  • user_admin_permissions shows the permission for the last module processed (I think as I'm reading through the code)
  • user_role_grant_permissions assumes that the module that declares a permission never changes and that there is only one such module. (this is the original bug report)

To fix this, we need something more than changing a few db_merges here and there, we need to think about a bit more:

When more than one module declares a permission, under what module goes it at admin/people/permissions? What title/description do we show for it?
The {role_permission.module} field is no good in it's current form, as it stores only one of the modules that declares a permission.

Stevel’s picture

Status: Reviewed & tested by the community » Active

(The above is by no means claimed to be a complete listing of possible problems in the permission handling).
Marking as active as this needs some discussion and there's no rerolling of an earlier patch now.

David_Rothstein’s picture

See also: #737816: user_role_grant_permissions() throws PDOException when used with non-existent permissions (e.g. a permission for a disabled/uninstalled module)

Which in fact might also be part of the explanation for #49. I haven't tested this, but I assume that if you update a site from D6->D7 while one of your modules is disabled (and if that module already has permission info in the database), then http://api.drupal.org/api/function/user_update_7006/7 will just leave the 'module' column empty for those database rows. Then when you reenable that module in D7 and try to grant some permissions... boom. Seems like that's one way you could wind up with a merge error even without two modules defining the same permission?

agentrickard’s picture

Status: Active » Needs review
Issue tags: +Security

The original patch was introduced expressly because we _want_ to enforce a one-module per permission rule. Trying to revert that now is going backwards to a broken permission model.

Modules that piggyback on the permissions of other modules create potential security risks. Having two modules implement the same permission is a horrible idea. Having a contrib module bypass the PHP input warning from core, as sun's example shows, is a horrid practice.

This was put in long enough before freeze to get module developers aware of the new restrictions.

IMO this is a core improvement, not a bug.

Marking 'needs review' as in, needs a decision from the D7 maintainers.

Adding 'security' tag to get security team input.

Stevel’s picture

Title: Permissions are assumed to be unique among modules » Permissions are assumed to be unique among modules, but uniqueness is not enforced

Thinking aloud here:

Well, if we want the one-module-per-permission model (which is great from a security point), we need a way to enforce this (other than having a buggy site if you don't follow it).

One way to do so is make sure a module can't be installed when it declares a permission that is already declared in another installed module, but what if a module is updated and a "conflicting" permission is added to it?

This brings the responsibility to the site administrator, which is then responsible for not installing two modules with the same permission declared.

Another idea is be to automatically prefix the permission name with the name of the module declaring it (at the caller of hook_permission), but that means changing permission names literally everywhere, so that's not an attractive option because it would require changing permission names all over the place, and then there's a possibility of overlapping namespaces (e.g. modules node and node_access) for permissions.

edit: Also changed the title to more correctly show what the problem is.

tstoeckler’s picture

Can someone explain why it is so great to have one module per permission? I don't seem to get it.
For example, I find the generic 'Use PHP' permission provided by core to be a great way forward from D6, where CCK had 'Use PHP for field settings' and Views had 'Use PHP for display options' etc. This makes sense conceptually (PHP module is responsible for letting people enter PHP... into whatever module!) and, again, I fail to see how that is a loss in security.

moshe weitzman’s picture

Seems to me that re-using perms is useful and should be supported. This all started with us wanting to automatically remove perm assignments during uninstall. I think we should still do that, on a best effort basis. Its OK if it fails under certain conditions. Could we fix this by removing a unique index?

Damien Tournoud’s picture

Priority: Critical » Major

As far back as I can remember, permissions were *always* assumed to be globally unique. Remember that the permission page is sorted by module name?

As a consequence, I'm downgrading this to major.

tstoeckler’s picture

Correcting myself a little upon re-reading:
The question is not (or more specifically: not only) whether modules can/should implement the same permissions (user_access($perm)), but whether multiple modules need to define the same permission (hook_permissions).
In the case of PHP module, I would say no: If you don't have PHP module installed, you shouldn't be able to enter PHP into Views display options, IMO.
There may be other use-cases which might require that, though.

David_Rothstein’s picture

As outlined in #61, you don't need two modules to define the same permission to trigger this error (although that will certainly do it also). I've now gone and confirmed that the following steps will trigger the error too, even just using Drupal core:

1. Install Drupal 6 and enable the search module.
2. Go the permission page and assign some search module permissions.
3. Disable the search module.
4. Update the site to Drupal 7.
5. Now enable the search module again.
6. Try to save the permission page... boom.

David_Rothstein’s picture

Priority: Major » Critical
Issue tags: +D7 upgrade path
JacobSingh’s picture

Status: Needs review » Reviewed & tested by the community

Why don't we at least commit this patch since it matches with the actual DB schema. We can then debate until we get blue in the face if the schema makes sense, but we don't need to leave the code with a logical bug in it do we?

marcingy’s picture

Patch still applies without the need for a reroll.

Dries’s picture

Having read the issue, I'm actually inclined to commit the patch.

steinmb’s picture

Posted this duplicate then the issue title made it a bit hard to find ;) #875744: user_role_grant_permissions throws a primary key violation when it should do a merge
The small patch (#51) fixes this issue though I agree that there still is more to discuss regarding permissions pr. module etc but I suggest that we commit this patch and open a new issue where we move the remaining discussion.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Seems everyone wants to get the small patch committed first (including myself). Committed to CVS HEAD. Thanks.

sun’s picture

Status: Fixed » Needs review
FileSize
1.63 KB

Alright, putting this up for consideration. Should be properly fixed in this issue, IMHO.

tstoeckler’s picture

+++ modules/user/user.module	9 Aug 2010 20:18:48 -0000
@@ -3605,9 +3605,34 @@ function user_modules_installed($modules
+    // hook_modules_uninstalled() is not supposed to be invoked via
+    // module_invoke() in uninstalled modules.

I don't get this comment. Sorry if it's obvious, but reading it ~10 times didn't reveal any meaning to me =)

Otherwise looks good and actually easier than I would have thought.
Leaving at needs review for someone more knowledgeable to give this a spin.

Powered by Dreditor.

sun’s picture

heh, that was a bogus attempt to describe an edge-case, which actually applies to module_implements() only. Thus, simplified :)

sun’s picture

+++ modules/user/user.module	9 Aug 2010 21:54:30 -0000
@@ -3605,9 +3605,34 @@ function user_modules_installed($modules
 function user_modules_uninstalled($modules) {
...
+  foreach ($modules as $module) {
+    // Use module_invoke(), not module_implements(), which would not work for
+    // uninstalled modules in an implementation of hook_modules_uninstalled().
+    if ($module_permissions = module_invoke($module, 'permission')) {

Ok, perhaps it's still confusing and can simply be shortened to

"module_invoke() is currently supposed to work here. Though strictly speaking, the module system no longer knows of the uninstalled modules, which makes this usage of module_invoke() are bit hair splitting." :P

Anyway, any other feedback on the proposed implementation/resolution? :)

Powered by Dreditor.

marcingy’s picture

Priority: Critical » Major

Should this now be major as the fix atm resolves the upgrade path issue or seems too. Downgrading.

sun.core’s picture

deviantintegral’s picture

This issue has quite a few patches and issues discussed, so here's a summary of where it looks like we're at. Corrections welcome.

The issue currently is that Drupal tries to clean up assigned permissions in the {role_permission} when a module is uninstalled. This is a problem in that we don't enforce permission uniqueness, and permissions themselves aren't namespaced. So, a module could define something rather generic like "create signups", which could also be defined by another module such as when users create a content type. The way we should solve this should be that as soon as no module defines a permission, we can then clean up {role_permission}.

I've written a test for this situation, which looks to have exposed a bug in #77. It works fine if you uninstall modules in alphabetical order, but not otherwise. So, expect the attached pass to fail the testbot with one failure.

salvis’s picture

Disabling a module should not cause its permissions to be trashed. They must be preserved until the module is uninstalled! Or, pursuing your strategy, until all modules that define the permission are uninstalled.

deviantintegral’s picture

Yes, currently permissions are only removed on uninstall, not disable.

Though it doesn't show in this issue, we have an unexpected failure: http://qa.drupal.org/pifr/test/131434

The problem is that taxonomy_permission loads vocabularies, and entity_controller() is throwing a fatal error when loading the entity.

Fatal error: Class name must be a valid object or a string in /Users/andrew/Documents/workspace/drupal7/includes/common.inc on line 7463

salvis’s picture

I read

as soon as no module defines a permission

as "as soon as the module defining the permission is disabled".

Disabled modules don't define permissions, or do they?

Status: Needs review » Needs work

The last submitted patch, 607238_drupal.user-permission-unique.81.patch, failed testing.

sun’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
deviantintegral’s picture

Status: Needs work » Needs review
FileSize
7.95 KB

The failure I found earlier wasn't from the names of the module, but the order in which they were uninstalled. If you uninstalled modules a followed by b each with the same permissions, and then uninstalled b followed by a, things broke due to the condition that the permission be defined by a module that was being uninstalled.

Here's a new patch that keeps track of all permissions that were candidates to be uninstalled, but kept because some other module defines them. When that happens, the {role_permission} table is updated to point the permission to the remaining module.

Status: Needs review » Needs work

The last submitted patch, 607238_drupal.user-permission-unique.87.patch, failed testing.

catch’s picture

Issue tags: -D7 upgrade path

Removing upgrade path tag, as far as I can tell that's no longer affected.

deviantintegral’s picture

Status: Needs work » Needs review
FileSize
8.9 KB

Here's a patch that fixes the test failure, but I'm not sure it's the right solution. The problem is that taxonomy_permission() loads vocabularies, which eventually hits entity_load() which expects {taxonomy_vocabulary} to exist.

The attached patch moves hook_modules_uninstalled() to be fired before module tables are dropped, but not after. The docs do indicate that the hook "Perform[s] necessary actions after modules are uninstalled", but I'm not sure if changing this to be before the actual uninstallation would really be an API break for D7. Let's start and see if this broke any other core tests.

Status: Needs review » Needs work
Issue tags: -Security, -D7DX, -Needs backport to D7

The last submitted patch, 607238_drupal.user-permission-unique.90.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Security, +D7DX, +Needs backport to D7

The last submitted patch, 607238_drupal.user-permission-unique.90.patch, failed testing.

tim.plunkett’s picture

There was a patch committed in #74, and since then I'm not sure what the direction of this issue is, or if it's still major.

Can someone either write up an issue summary and perhaps retitle, or open a follow-up and close this one?

nadavoid’s picture

FileSize
533 bytes
533 bytes

To help pearcraft in debugging, I created 2 test modules. They both define a permission called "test test", plus one defines "test test 1" and the other defines "test test 2". I was able to enable both, then disable and uninstall both, without receiving any error messages. I did set some of the new permissions, and noticed that only the "test test" permission appeared only in the "Test 1" group.

kay_v’s picture

Issue summary: View changes

Updated issue summary. Made the summary very brief, as the issue is pretty wide ranging.

fubhy’s picture

We are trying to convert role permission assignments to config over at #1872876: Turn role permission assignments into configuration.. As we definitely do NOT want to store the module name of the providing module in the config (because, well, it's config) we do have a bit of a problem with the clean-up upon module uninstallation there. The problem is that we can not invoke hook_permission() for a module when it is being uninstalled because the module is already disabled at that point (and even with #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed we would still be missing a hook that fires BEFORE a module is uninstalled). And even if we could we would have problems with dynamic permissions that are no-longer there (like assigned permissions of node types that got deleted afterwards).

I'd therefore propose to solve this issue over at #1872876: Turn role permission assignments into configuration. by making the providing module part of the permission name.

Actually, I find it pretty strange that we currently don't enforce namespaces for permssions as I see the same problems that @agentrickad already pointed out in #62 in 2010!

fubhy’s picture

Doesn't it bother anyone that modules like node expose permissions like "delete revisions" or "access content"? That's not very verbose and basically screams for clashes with other modules that try to expose the same permissions.

David_Rothstein’s picture

I'm definitely +1 on having Drupal 8 solve this by namespacing permissions to the module (in some way or another). I worry about the possibility of permission name conflicts all the time... and I think it would also be better developer experience to see a permission in the code and immediately know where that permission is coming from.

fubhy’s picture

Same here... What are we going to do with the whitespaces in the permission names though? Or would it be okay to simply go 'node.administer nodes' for example?

fubhy’s picture

Issue summary: View changes

Updated issue summary.

tim.plunkett’s picture

jhedstrom’s picture

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

I think it is too late for 8.0 at this time.

dawehner’s picture

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

Well, its a bug.

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.

  • Dries committed 06daeb7 on 8.3.x
    - Patch #607238 by agentrickard: fixed user_modules_uninstalled()...
  • Dries committed 25ac9eb on 8.3.x
    - Patch #607238 by ctmattice1, catch: made the upgrade path work again.
    
    
  • Dries committed 0777093 on 8.3.x
    - Patch #607238 by agentrickard, catch, salvis: permissions are assumed...

  • Dries committed 06daeb7 on 8.3.x
    - Patch #607238 by agentrickard: fixed user_modules_uninstalled()...
  • Dries committed 25ac9eb on 8.3.x
    - Patch #607238 by ctmattice1, catch: made the upgrade path work again.
    
    
  • Dries committed 0777093 on 8.3.x
    - Patch #607238 by agentrickard, catch, salvis: permissions are assumed...
steinmb’s picture

This issue confuse me a bit. The commits in #104 and #105 was to all our branches so... Anything left in here or could we go ahead and close it?

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.

  • Dries committed 06daeb7 on 8.4.x
    - Patch #607238 by agentrickard: fixed user_modules_uninstalled()...
  • Dries committed 25ac9eb on 8.4.x
    - Patch #607238 by ctmattice1, catch: made the upgrade path work again.
    
    
  • Dries committed 0777093 on 8.4.x
    - Patch #607238 by agentrickard, catch, salvis: permissions are assumed...

  • Dries committed 06daeb7 on 8.4.x
    - Patch #607238 by agentrickard: fixed user_modules_uninstalled()...
  • Dries committed 25ac9eb on 8.4.x
    - Patch #607238 by ctmattice1, catch: made the upgrade path work again.
    
    
  • Dries committed 0777093 on 8.4.x
    - Patch #607238 by agentrickard, catch, salvis: permissions are assumed...

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.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

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

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

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

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

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

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

  • Dries committed 06daeb7 on 9.1.x
    - Patch #607238 by agentrickard: fixed user_modules_uninstalled()...
  • Dries committed 25ac9eb on 9.1.x
    - Patch #607238 by ctmattice1, catch: made the upgrade path work again.
    
    
  • Dries committed 0777093 on 9.1.x
    - Patch #607238 by agentrickard, catch, salvis: permissions are assumed...

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

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

quietone’s picture

Version: 8.9.x-dev » 9.3.x-dev
Status: Needs work » Fixed
Issue tags: -Needs issue summary update +Bug Smash Initiative

A patch for this issue was committed on Mon Aug 9 19:49:40 2010, see #74. Then discussion continued until now, according to the IS the remaining task here is to open followups and then close this one. It is difficult to know what the followups should be and none have been created in the intervening 8 years.

Therefor, it is time to close this as fixed (because of the patch). Any experiencing any problems with the permissions can open new issues.

Updated the credit to match the commit message.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.