Auto Assign Role clobbers other set roles on insert
nicks - October 4, 2008 - 19:52
| Project: | Auto Assign Role |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
The Auto Assign Role module provides useful functionality to give users a role when they sign in for the first time. If LoginToBoggan is enabled then Auto Assign Role cannot work. The reason for this is as follows:
- Auto Assign Role uses hook_user with the insert operation to add the role to the user
- In logintoboggan_user_register_submit(), LoginToBoggan then calls user_save() with a $roles array argument, which has the effect of resetting the roles for the user, and overwriting the changes by Auto Assign Role
Since logintoboggan_user_register_submit() is called after hook_user(), Auto Assign Role cannot avoid this by changing the order in which the modules are invoked.
I suggest that the LoginToBoggan module should populate the $roles array with the user's assigned roles, and only then add the pre-auth role to the $roles array.
#1
subscribing
#2
this doesn't make any sense. insert operation of hook_user should get called _after_ the user_save in logintoboggan_register_submit -- that should absolutely be the first save of the cycle for the user, because they are being created via registration in the LT workflow.
#3
I'm kind of confused by this as well. We have a site we are building that is using both these modules in d6 and have not noticed a problem. I'm going to test this in d5 and see if there is something that needs to change on AAR's side but will likely not have the free time to do so until next weekend.
#4
moving to the right queue.
#5
Adding the following in AAR's hook_user fixes this problem.
$edit['roles'] = NULL;http://drupal.org/cvs?commit=145819
#6
#7
Automatically closed -- issue fixed for two weeks with no activity.
#8
@cyberswat -- can you give more detail on where in hook_user that line is supposed to go??
#9
@cyberswat -- nevermind -- i looked at the cvs commit. for anyone else, that line goes directly above the 'break;' and directly below $user = user_load(array("uid"=>$user->uid));
in autoassignrole_user
#10
The combination of Login Toboggan and Auto Assign Role is still broken. Login Toboggan attempts to assign a "pre-authenticated" role to users when the "Immediate Login" option is selected. However, when the User Role Assignment option is selected under Auto Assign Role and the users are allowed to select other roles (not including the authenticated role or pre-authenticated role, of course), the user is immediately granted authenticated status and bypasses the pre-authenticated role.
The modules being used are
Auto Assign Role 5.x-2.x-dev (11 Oct 2008)
Login Toboggan 5.x-1.3
Drupal 5.14
#11
Does the above fix work or not? ;)
#12
Somebody should modify LoginToboggan to natively support multiple non-activated/activated sets of roles.
AAR overrules LoginToboggan by placing the user in the selected category right away. But even if this was changed so that LoginToboggan would put users in the non-active role, users would still just be placed in the "autheticated user" role, when they activate their account, and so the user's desire of role would be lost, making AAR obsolete.
Therefore the best solution would be to make LoginToboggan somehow support multiple roles.
#13
Any news?
#14
I am also seeing this issue on a Drupal 5 installation, with version 5.x-1.2 of this module. Is there any progress on a fix?
Thanks,
-- Doug
#15
Subscribing...
#16
I am using the 6.x version, but I also realized this issue with Autoassign Role and Logintoboggan.
My Problem is that AAR shows all roles to select from in user registration when user role select is enabled, including roles like admin, authenitcated etc.
This is a huge problem for me, so I have to disable Logintoboggan until a fix is there. Any progress here?
a big sorry. My problem is related to this http://drupal.org/node/384702
#17
+ 1 to wanting to see this fixed.
#18
I'm looking for patches to address this issue and will review any that are submitted.
#19
This issue was raised in our issue queue for http://drupal.org/project/rolekey to investigate. Has anyone tried to just simply updating the $edit array rather than inserting the roles manually? Works in both Drupal 5 & 6
<?php$edit['roles'][$role] = $role;
?>
Also since this module resets $user object in the submit handler, this could lead to strange bugs on modules which rely on an unadulterated user object. Is there a reason for this? Maybe a submit handler appended to the end of the user register #submit list could be better?
#20
Is there any news on this? I really want to use LoginToBoggan, but given the development status of my project, I can't allow for LoginToBoggan to break the login routine, I hardly managed to set it up in the first place. AAR is a must for me.
On this occassion, I really have to say it once: User Management sucks with Drupal. Contentprofile. Autoassign Role, Role Sign Up..... and all this extra modules and configuration to set up 2 different kinds of user profiles. Not to mention the theming of all those... What a hassle. Don't get me wrong, I love Drupal, but all this user profile issues are rather difficult.
#21
"I can't allow for LoginToBoggan to break the login routine"
LoginToBoggan works the Drupal way! Any issues reside here.
#22
I know I know. I don't mean to bash LogginToBoggan. When I first used Drupal it was must-have module.
But I need AAR to realise what I described above. So AAR takes precedence, obviously. If only Drupal would add CCK Support to the core Profile module and allow for more than one Profile on Registration, I wouldn't need Content Profile. Hmm, but then again, I would still need AAR.
Whatever, like I said IMHO the whole user settings / registration / theming etc is such a hassle. It gets so complicated once there is need to support different user groups.
#23
I'm not supporting d5
#24
any fix here yet? [i found one]
Since LoginToBoggan uses logintoboggan_user_register_submit to handle registration, it seems to be overriding the default and other methods of saving a user, in my case, I hacked user.module before to get my roles added at registration, I simply needed to modify logintoboggan.module in the same way.
#25
Guys it is the way that "Auto Assign Role" works that screws up the login work flow. Drupal is a modular system and breaking this chain can upset all of the other modules that depend on it.
@cyberswat If this is still an issue, have a look at rolekeys to see a way to set the user roles - all you have to do is set the roles and Drupal will do the rest. Just remember to unset any "custom" form elements so that they don't end up in the {user}.data field.
I'm not marking as open as I haven't looked at the code of auto assign for a while and this may already be resolved.
#26
@Alan D. All this module does is use standard hooks and functions to edit the roles ... nobody has verified if this is an issue in the d6 version or not. If you'd like to help resolve the issue if it still exists I'd be happy to review the patch.
#27
From memory, you only need to set new fields on the User object for them to be saved latter on - your insert reloads this object which could upset this flow. Sorry haven't the time to create a patch / test
#28
@Alan D. This module doesn't do any inserts in regards to the user object or it's roles ... it simply alters the role array on the user object as it's passed through the hook_user insert op.
#29
I was meaning the line in hook_user - op = insert right at the end of this section.
"$account = user_load(array("uid" => $account->uid));"
This object is being passed around by reference
Q1: Is this needed?
Q2: If the system weight of autoassignrole < logintoboggan, does logintoboggan break? And vice versa. The only thing that I needed to do to set the roles was to populate the $edit['roles'] values. The rest was automatic.
Anyway, I'm going to butt out so not to stand on anyones toes any more than I already have ;\
#30
Oddly enough, that line of code was introduced specifically to address this issue http://drupal.org/node/226765#comment-977324 ... previously I was only using $edit ... for reasons I can't explain this seemed to fix the issue.
I'm going to re-open this in the hopes that someone can either confirm or deny this is still an issue. I'd like to close it out for good before porting to d7.
#31
Postponing this until someone wants to help fix it or establish it as an existing issue.
#32
first, let me clear some things up here about how LoginToboggan functions.
in order for LT to alter the registration workflow to perform it's tasks, it's necessary for it to replace core's standard user_register_submit() function. but to be clear, LT's implementation of this follow's core's very closely -- everything that's done in the standard user_register_submit() function is done in LT's custom submit handler.
LT understands that a user is in a pre-authorized state by adding a special role to the user when they are initially created. it's careful to do this in a way that respects any existing roles!
<?php// Set the roles for the new user -- add the pre-auth role if they can pick their own password,
// and the pre-auth role isn't anon or auth user.
$validating_id = logintoboggan_validating_id();
$roles = isset($form_state['values']['roles']) ? array_filter($form_state['values']['roles']) : array();
if ($reg_pass_set && ($validating_id > DRUPAL_AUTHENTICATED_RID)) {
$roles[$validating_id] = 1;
}
?>
by respecting existing roles in $form_state['values']['roles'] array, LT doesn't step on other module's toes. from my cursory look at AAR's user insert code, it doesn't look like it's always doing the same:
<?php$rolenames = user_roles();
if (_autoassignrole_get_settings('user_active') == 1) {
$roles = array();
$user_roles = _autoassignrole_get_settings('user_roles');
if (is_array($edit['user_roles'])) {
foreach ($edit['user_roles'] as $k => $v) {
if ($v != 0 && in_array($k, $user_roles, TRUE)) {
$roles[$k] = $v;
}
}
$edit['roles'] = $roles;
}
elseif ($edit['user_roles'] != '') {
$edit['roles'] = array($edit['user_roles'] => $edit['user_roles']);
}
}
?>
the whole
$roles = array(); ... $edit['roles'] = $rolesapproach is incorrect IMO, because it unconditionally clobbers anything else any other module is trying to do with roles during a user insert. this is not the drupal way... ;)although i don't have an intimate understanding of AAR, i believe the attached patch will correct this issue.
also, Alan mentioned his concern in #29 for the user_load() call at the end of the insert code. i don't think it's a problem there, but i'm also not sure it does anything. if it's intention is to load the AAR roles into $account so other modules can see them, i don't think that actually works, since at that point in the hook the AAR roles aren't saved to the database yet. unless there's some other reason to reload the user there, i would remove that line of code.
#33
Good enough for me ... simpletest passes after applying the patch and removing the extraneous line Alan mentions in #29
http://drupal.org/cvs?commit=276060
#34
Automatically closed -- issue fixed for 2 weeks with no activity.