Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I have a "user manager" role that contains the permission to assign roles.
As that user, I edit my own account, and grant me two additional roles.
After saving, I have those two roles, but no longer have my original "user manager" role.
---
I really don't understand the use of a static cache to temporarily store roles - and I'm not surprised that it's not working, as the static cache is populated during the form alter - but it will obviously be gone after the form is submitted.
?
Comment | File | Size | Author |
---|---|---|---|
#17 | NonAssignableRole.png | 5.97 KB | salvis |
#12 | roleassign-store_existing_roles_in_forms-1954322-12.patch | 1.54 KB | Agileware |
#1 | roleassign-store_existing_roles_in_forms-1954332-1.patch | 1.76 KB | mstef |
Comments
Comment #1
mstef CreditAttribution: mstef commentedHere's a patch to do away with the confusing method of using a static cache to store the user's current roles. The data should be stored in the form, so it persists between page loads and the submitting of the form. The data will appear in hook_user_presave(), where it's needed.
I don't understand quite how this is working for anyone else..
Comment #2
mstef CreditAttribution: mstef commentedI also don't understand the IF statement here:
$new_sticky_roles will always be set regardless, so it will set $sticky_roles = NULL if it's excluded from the call.
The patch removes this entire function.
Comment #3
salvisI don't understand the problem. Please explain using names like Bob and Alice rather than terms like "that user" and "my own account".
No. If $new_sticky_roles is NULL, then isset() is FALSE.
Comment #4
mstef CreditAttribution: mstef commentedOkay, let's go over both issues.
The first issue: This module doesn't work at all for me. The problem lies in the use of a static cache to store the user's current roles.
I'll recite my original issue. And there is just 1 user in question:
1) I have a "user manager" role that contains the permission to assign roles.
2) As that user, I edit my own account, and grant me two additional roles.
3) After saving, I have those two roles, but no longer have my original "user manager" role.
Storing the user's current roles in a static cache can not possibly work. The static cache only remains populated within the given page request. The first page request presents the user's edit form which contains the roles - you populate the static cache then with the user's current roles. The next page request comes when the form is submitted. In the user presave hook, you attempt to fetch what's in that static cache - but nothing will be in there because this is a new request.
My patch eliminates that strange workflow, which doesn't work. It stores the data in the form, which is then passed to the presave hook.
--
As for the isset() issue, that function returns TRUE if the variable is defined; regardless of whether or not it's TRUE or FALSE. Your function parameter defines it: function _roleassign_sticky_roles($new_sticky_roles = NULL); therefore, it will always return TRUE for isset(). How could it possibly be not set?
Comment #5
salvisLet's try to identify the issue before we jump to conclusions and throw around code and judgmental language.
I have a role Webmaster (with the 'assign roles' permission) and a user webmaster who has that role. Did you add the Webmaster role to the roles controlled by RoleAssign? I don't think that's a good idea, but it works both ways just fine for me: user webmaster is able assign himself roles among those controlled by RoleAssign, and he does not lose the Webmaster role.
Well, in my PHP, isset() returns FALSE for a variable that is set to NULL. If your PHP works differently, then we are definitely incompatible and none of my modules will ever work for you.
You were on the right track for a moment there — too bad you lost that thought. If you continue to be unwilling to learn and to let your preconceptions go, then please stop wasting my time.
Comment #6
mstef CreditAttribution: mstef commentedNo judgmental language.
Regarding the isset(), I did jump the gun on that. I was rushing and improperly tested that. With my patch, that code is removed, so it's not worth continuing this that.
My role is "user manager" that contains the "assign roles" permission. I did not check it on the roleassign admin config page.
I navigate to edit my own account, and check some of the available permissions to assign myself. After submitting, I get a 403 because I no longer have the "user manager" role.
This is because the module calls the function that returns what's in the static cache, that was set on the form, but it's empty now because it's a new page request. So I'm left with only the roles I selected on the form.
Comment #7
mstef CreditAttribution: mstef commentedTalking arbitrarily here, without specific examples, and as I mentioned before, I don't understand how storing the current roles inside a static cache when viewing the form could work after the form is submitted. Can you explain how that works, if it does, and why that's used instead of storing the data in the form itself (like my patch does). I'm confused - maybe I'm missing something, but after the form is submitted, that static cache is no longer populated.
Comment #8
salvisExplaining language features and programming techniques is beyond the normal level of support, especially to people who burst in, "not surprised that it's not working," because "the confusing method" is "obviously" flawed, and who immediately want to "patch" what they "don't understand." A more humble attitude and an occasional "please" would make this a more pleasant experience for everyone involved.
The purpose of _roleassign_sticky_roles() is exactly what the comment above the call says — during the POST, when roleassign_user_presave() is actually called. It's not to keep the list of roles from the GET to the POST. There's no need for that.
Drupal rebuilds its forms during the POST page request, and _roleassign_sticky_roles() is loaded again from scratch in order to be used by roleassign_user_presave().
If _roleassign_sticky_roles() is different between your GET and your POST, specifically if your "user manager" role assignment has vanished, then it was removed by some third-party module on your site, and only you can find out what is happening. Try RoleAssign on a virgin Drupal installation and you'll see that it works just fine.
Comment #9
mstef CreditAttribution: mstef commentedOkay, so we'll just ignore the fact that it's not working - whether or not you take the patch has no bearing on me - I took the time to create it, contribute it and provide the context to help you out (not me); so I don't know where a "please" fits into this conversation. I don't know why you take bug report so personally.
Even if the module did work perfectly, my suggestion is still a good one rather than using a static cache - the data belongs stored in the form.
Comment #10
mstef CreditAttribution: mstef commentedLets keep the attributes accurate for other people who might come across this.
Comment #11
mstef CreditAttribution: mstef commentedComment #12
Agileware CreditAttribution: Agileware commentedRe-opening and confirming. You can't rely on Drupal rebuilding the form when you submit it unless you explicitly tell it to do so, and a debug session confirmed this doesn't always happen for user profile form.
I've edited the patch to be a little clearer what's happening during presave.
Comment #13
salvisSorry, Agileware, but this issue is not going anywhere anymore.
You are welcome to start a new issue, explain what the problem is and how to reproduce it.
I'll need some proof for this claim.
Sometimes it does and sometimes it doesn't? Depending on what?
I won't consider a solution before I understand the problem.
Comment #14
mjpa CreditAttribution: mjpa commentedSeems silly to refuse a patch that fixes an issue that people are having.
Ran into this problem on a site. If user Foo edits Bar's account, and Bar has a role that Foo can't assign, that role is removed from Bar's account every single time.
The patch in #12 fixes this!
Comment #15
salvisWell, I'm sure you wouldn't want me to commit changes to a security-relevant module for your site without understanding what I'm doing. You should be happy about how I'm handling this, because I'm doing my best to keep your site secure.
As long as I'm not convinced that something is broken, I won't accept any fix. Describing the problem is the first step and #14 finally provides that -- thank you! Without the first step, no other steps can be taken.
I gather that you've tested the patch, but I see no indication that you've reviewed it, so setting to NR.
Comment #16
vegardjo CreditAttribution: vegardjo commentedI can also confirm the problem. In my case all users had the role "employee" (primarily used as a filter in a view), then I had an "administrator" role, that could assign other users an "editor" role. Every time a user was assigned the editor role, they lost their employee role.
Haven't tested the patch, as in my case I solved it with also setting employee as an assignable role, which fixes it. But in other cases you might not want that role to be assignable / changable at all. In other words, pre existing non-assignable roles are removed when new roles are set via role assign.
Comment #17
salvisThank you, vegardjo, that's a lead.
I have created and configured an "Assignable Role" and a "Non-assignable Role". My restricted user admin sees the following:
After checking the Assignable Role and saving, the edited user has both the Assignable Role and the Non-assignable Role. IOW, I still cannot reproduce this issue.
Comment #18
RayCascellaOddly enough, I'm having the same issue, but as Salvis states, a vanilla install of Drupal with only the module on SimplyTest.me, does work. An nonassignable role remains after a user self edits or when an admin edits their account.
Hoping to do more testing today to figure out what's going on. Updates to follow =-D
Comment #19
RayCascellaSo, I've done some testing and I cannot figure out how there's a difference between my servers install, and those on SimplyTest.me servers, which cannot replicate the error. I didn't find any instances, of any of my other modules, saving the user's information twice, or submitting the form multiple times, perhaps with empty values in the role fields?
Applying the patch in #12 did fix my issue, so I'd endorse applying this patch to core, simply to more consistently track the user's inherited roles, to help other users avoid this bug.
Comment #20
salvisThank you for your testing and reporting, RayCascella!
I see your point, but apparently there exists an environment where RoleAssign behaves differently. This troubles me, and I have no way of knowing what else might be wrong in that other environment.
I'm reluctant to cover up symptoms without knowing exactly what I'm up against, and risking to maybe break something else in a very stable module.
Comment #21
geekygnr CreditAttribution: geekygnr commentedFirst of all I would like to say that the patch in #12 solves this problem for me.
Next I am going to describe how I came about this problem because it is slightly different and will hopefully help with tracking it down.
I tried on simplytest.me and couldn't reproduce it myself but the problem does exist somewhere in these complex systems we have. The big question is where.
Comment #22
salvisHere's a patch that adds a test to check for this issue.
The test passes on my machine — let's see what the testbot says...
@geekygnr: You mention having "a set of fields on their profile". Could this be the trigger for this issue?
Comment #23
salvisGreat, #22 has come back green. This means that the testbot cannot reproduce this issue either.
I will now commit #22, which means that if anyone triggers a re-test of that patch, it won't apply anymore (and turn red), because it can't be applied a second time, of course.
Comment #25
salvisSo, now, what is the configuration needed to make the test fail?
Are you using Profile 2 or what?
Comment #26
geekygnr CreditAttribution: geekygnr commented@salvis it is possible that was part of the problem. To tell you the truth I don't remember everything I did to try to recreate this and I no longer work at the same place so I can't even look at the project I implemented this on.
I wasn't using profile 2
Comment #27
AnybodyHello @all and thank you for your great work!
This issue is currently in "Needs review" status. I think we can set it Fixed and see if this is truely fixed for all users.
Anyway I'd suggest to create a new stable release after 3 years as it seems that the current stable release still contains the bug we're talking about here, is that right? (I'm having the trouble with losing roles when using it).
Thanks a lot for your great work and what do you think about a new stable?
Comment #28
AnybodyI'm sorry to reopen this, but I can't confirm this to be fixed.
Please decide if we should proceed here or in a new issue (but I would suggest to proceed here):
I think one problem with the simpletest might be that we use user1 (admin) which has all roles and permissions anyway. It would be better to use a dummy user or something like that.
The test case runs for me but a normal non-admin user still loses his role when he saves his profile and is not allowed to select the role he has.
Example:
User 22 has roles:
- Moderator
- Lower Admin
He is allowed to set roles:
-Moderator
After he saves his profile he loses the role "Lower admin"
I've installed the latest .dev version and mo profile2 or something like that.
Comment #29
AnybodyI can finally confirm that #12 works and from my developer point of view is a very good solution because it uses the form functionality to save the data over the form validation and submit process.
My vote goes for #12 and optionally an additional TestCase to check the problem for a non-admin user, then your conditions from #14 will surely apply because you should see that this bug still exists.
Thank you all very very much for your help. Let's get this fixed finally :)
Comment #30
salvis@Anybody: Thank you for picking this up, but you're somewhat confused.
1. Nothing has been committed in the way of "fixing" this, because I'm still not able to reproduce any brokeness.
2. #22 adds a test that proves that the testbot does not see the issue either.
3. Contrary to your claim, the test uses user 1 to set up the $deputy user and the RoleAssign settings only. The actual test is then run using the $deputy user.
4. I'm not sure what you mean with non-admin user. $deputy has exactly the permissions that are needed for the test, no more and no less.
Have you applied #22 (without #12, of course!) to your test system and run the test? What's the result?
Comment #31
AnybodyThanks a lot for your reply, salvis.
I've run the test and it's OK which lead me to my suggestions above.
Anyway the bug still exists and I can clearly reproduce it in my environment manually. Actually I have no more idea currently where the problem comes from, if the test is right! In #28 I described how I reproduced the problem in my case.
Comment #32
salvisWell, I can't guarantee that the test is bug free, of course. If you (or anyone) can point to a bug, I'll be glad to fix it...
The one difference that I see is that in your #28 scenario user 22 is editing his own profile, while in the test $deputy is editing $account's profile.
Comment #33
webservant316 CreditAttribution: webservant316 commentedThis problem observed here. I need to use this module to restrict the roles that my 'webassistants' can edit in other users. They are able to edit the roles of other users nicely, however, when they edit their own profile they loose the 'webassistant' role. However, I don't want to give them access to the 'webassistant' permission.
How can I help debug this?
Comment #34
salvisIf you apply the patch in #22 and run the RoleAssign tests, do you get a test failure?
Comment #35
webservant316 CreditAttribution: webservant316 commentedwish I could help. when I visit /admin/config/development/testing I get this error
One of my modules must have a bad test module.
Comment #36
salvisEnable Devel module and its Krumo Backtrace error handler (in Settings).
That should give you a call stack trace that may tell you which module is causing the issue.
Comment #37
AnybodyI finally switched to role_delegation module which works for me.
Comment #38
salvisI tried again to reproduce this issue.
I have a "Webmaster" role that can assign the "Member" and "Editor" roles, but not the "Webmaster" role, because I don't want my webmaster to add more webmasters without my knowledge. I think this is a pretty reasonable pattern that most sites will use in some form. Then I create user "webmaster" (lower case) with all three roles. Now, as user "webmaster", when I edit myself, I see only the "Member" and "Editor" roles, and I can add or remove either of them, without losing my "Webmaster" role.
It's just not reproducible here, and with about 10K known sites for the D7 version we have very few complaints about this...
Comment #39
salvisBTW, I've also done the test above on the current D88 and did not find this issue either.