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
Comment | File | Size | Author |
---|---|---|---|
#95 | test1.zip | 533 bytes | nadavoid |
#95 | test2.zip | 533 bytes | nadavoid |
#90 | 607238_drupal.user-permission-unique.90.patch | 8.9 KB | deviantintegral |
#87 | 607238_drupal.user-permission-unique.87.patch | 7.95 KB | deviantintegral |
#81 | 607238_drupal.user-permission-unique.81.patch | 6.46 KB | deviantintegral |
Comments
Comment #1
alexanderpas CreditAttribution: alexanderpas commentedBTW: drupal is giving me:
Notice: Undefined index: taxonomy_vocabulary in entity_get_info() (line 5936 of includes/common.inc).
right afterwards!
Comment #2
hefox CreditAttribution: hefox commentedJust 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 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?
Comment #3
hefox CreditAttribution: hefox commentedEr, 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?
Comment #4
hefox CreditAttribution: hefox commentedadditional 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.
Comment #5
hefox CreditAttribution: hefox commentedThe 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.
Comment #6
catchThis 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
Then it would be up to taxonomy.module it ensure its hook_permission() worked when its disabled.
Comment #7
agentrickardI 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().
Comment #8
agentrickardWe 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...
Comment #9
hefox CreditAttribution: hefox commentedif 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?
Comment #10
catchThis 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).
Comment #11
agentrickard@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.
Comment #12
agentrickardWell, 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().
Comment #13
hefox CreditAttribution: hefox commentedPatch 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. :/
Comment #14
Dries CreditAttribution: Dries commentedMmm, 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.
Comment #15
agentrickard@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.
Comment #16
moshe weitzman CreditAttribution: moshe weitzman commentedTo 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.
Comment #17
catchI 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
Comment #18
agentrickard@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.
Comment #19
catch@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.
Comment #20
agentrickardAs it happens, I already have a D7 module for testing/maintaining that admin role case. http://drupal.org/project/secure_permissions
Comment #21
agentrickardWe 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.
Comment #22
moshe weitzman CreditAttribution: moshe weitzman commentedChanging 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.
Comment #23
Dries CreditAttribution: Dries commentedI 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.
Comment #24
agentrickard@moshe
Not really disagreeing. I can roll a quick patch later. And I suppose it is critical, since its currently broken.
Comment #25
agentrickardAnd a new patch.
This contains a new {role_permission}.module field and and update_N for user.module to track the needed data.
Comment #26
moshe weitzman CreditAttribution: moshe weitzman commentedis there a use for NOT NULL with default''. i think the default line can go.
how often is this function called on a page/ does it really need a static cache?
Powered by Dreditor.
Comment #27
catchWe should also add taxonomy to the install/uninstall tests if they already exist, or open an issue for that if they don't yet.
Comment #28
Dries CreditAttribution: Dries commentedFirst, 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 ituser_module_permissions()
? Not sure that would be better but it would provide some extra consistency.Thoughts?
Comment #29
agentrickard@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.
Comment #30
catchuser_permission_get_module() works for me.
Comment #31
moshe weitzman CreditAttribution: moshe weitzman commentedSurely 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.
Comment #32
agentrickard@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().
Comment #33
moshe weitzman CreditAttribution: moshe weitzman commentedI 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.
Comment #34
catchtaxonomy_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...
Comment #35
agentrickardAlright. /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.
Comment #36
moshe weitzman CreditAttribution: moshe weitzman commentedOh 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().
Comment #37
agentrickardI like having modules in the function name somewhere. It tells me why we're using this instead of module_invoke_all().
Comment #38
moshe weitzman CreditAttribution: moshe weitzman commentedOK
Comment #39
Dries CreditAttribution: Dries commentedThis looks great to me too. Committed to CVS HEAD.
Comment #40
catchThis 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
Comment #41
catchWhile this is trivial I didn't actually mean to mark RTBC.
Comment #42
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD.
Comment #43
catchAlso broke the installer, I get this with minimum profile:
Comment #44
catchComment #45
catchNo, not that at all.
Comment #46
catchActual bug here, this patch just found it: #706558: Minimal profile is broken squared
Comment #48
David_Rothstein CreditAttribution: David_Rothstein commentedThis 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)
Comment #49
salvisIn #35, which was committed in #39 there's
Shouldn't this have been
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.
Comment #50
BenK CreditAttribution: BenK commentedSubscribing... I'm experiencing the same duplicate key error described in #49.
Comment #51
salvisHere's the patch.
Comment #52
BenK CreditAttribution: BenK commented+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
Comment #53
lotyrin CreditAttribution: lotyrin commentedRe #52: Is this RTBC then?
Comment #54
Stevel CreditAttribution: Stevel commentedThis 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.
Comment #55
salvisUnfortunately, 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?
Comment #56
BenK CreditAttribution: BenK commentedI 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
Comment #57
Stevel CreditAttribution: Stevel commented"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.
Comment #58
sun*cough*
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.
Comment #59
Stevel CreditAttribution: Stevel commentedSo 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.
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.
Comment #60
Stevel CreditAttribution: Stevel commented(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.
Comment #61
David_Rothstein CreditAttribution: David_Rothstein commentedSee 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?
Comment #62
agentrickardThe 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.
Comment #63
Stevel CreditAttribution: Stevel commentedThinking 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.
Comment #64
tstoecklerCan 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.
Comment #65
moshe weitzman CreditAttribution: moshe weitzman commentedSeems 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?
Comment #66
Damien Tournoud CreditAttribution: Damien Tournoud commentedAs 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.
Comment #67
tstoecklerCorrecting 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.
Comment #68
David_Rothstein CreditAttribution: David_Rothstein commentedAs 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.
Comment #69
David_Rothstein CreditAttribution: David_Rothstein commentedThe scenario I described in #68 seems to be happening:
#875744: user_role_grant_permissions throws a primary key violation when it should do a merge
Comment #70
JacobSingh CreditAttribution: JacobSingh commentedWhy 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?
Comment #71
marcingy CreditAttribution: marcingy commentedPatch still applies without the need for a reroll.
Comment #72
Dries CreditAttribution: Dries commentedHaving read the issue, I'm actually inclined to commit the patch.
Comment #73
steinmb CreditAttribution: steinmb commentedPosted 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.
Comment #74
Dries CreditAttribution: Dries commentedSeems everyone wants to get the small patch committed first (including myself). Committed to CVS HEAD. Thanks.
Comment #75
sunAlright, putting this up for consideration. Should be properly fixed in this issue, IMHO.
Comment #76
tstoecklerI 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.
Comment #77
sunheh, that was a bogus attempt to describe an edge-case, which actually applies to module_implements() only. Thus, simplified :)
Comment #78
sunOk, 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.
Comment #79
marcingy CreditAttribution: marcingy commentedShould this now be major as the fix atm resolves the upgrade path issue or seems too. Downgrading.
Comment #80
sun.core CreditAttribution: sun.core commented#77: drupal.user-permission-unique.77.patch queued for re-testing.
Comment #81
deviantintegral CreditAttribution: deviantintegral commentedThis 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.
Comment #82
salvisDisabling 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.
Comment #83
deviantintegral CreditAttribution: deviantintegral commentedYes, 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
Comment #84
salvisI read
as "as soon as the module defining the permission is disabled".
Disabled modules don't define permissions, or do they?
Comment #86
sunComment #87
deviantintegral CreditAttribution: deviantintegral commentedThe 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.
Comment #89
catchRemoving upgrade path tag, as far as I can tell that's no longer affected.
Comment #90
deviantintegral CreditAttribution: deviantintegral commentedHere'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.
Comment #92
mgifford#90: 607238_drupal.user-permission-unique.90.patch queued for re-testing.
Comment #94
tim.plunkettThere 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?
Comment #95
nadavoid CreditAttribution: nadavoid commentedTo 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.
Comment #95.0
kay_v CreditAttribution: kay_v commentedUpdated issue summary. Made the summary very brief, as the issue is pretty wide ranging.
Comment #96
fubhy CreditAttribution: fubhy commentedWe 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!
Comment #97
fubhy CreditAttribution: fubhy commentedDoesn'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.
Comment #98
David_Rothstein CreditAttribution: David_Rothstein commentedI'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.
Comment #99
fubhy CreditAttribution: fubhy commentedSame 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?
Comment #99.0
fubhy CreditAttribution: fubhy commentedUpdated issue summary.
Comment #100
tim.plunkett#2026983: [regression] Grants for permissions defined by a module are not revoked / removed from Role config entities when that module is uninstalled was marked as a dupe of this
Comment #101
jhedstromI think it is too late for 8.0 at this time.
Comment #102
dawehnerWell, its a bug.
Comment #106
steinmb CreditAttribution: steinmb as a volunteer commentedThis 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?
Comment #117
quietone CreditAttribution: quietone as a volunteer commentedA 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.