| Project: | Account Types |
| Version: | 5.x-1.4 |
| Component: | Code |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs work |
Issue Summary
I have found a problem where it appears that the accounttype module strips an account of roles during user_save() if the users.data column is missing a setting for "selectAT" even though the value in acounttypes_users is set.
I added this module to an existing web site with 1000 or so users and then populated the accounttypes_users table with proper values for all the existing accounts. This seems to have mostly worked well. However, we have recently noticed Access Denied messages on the click after login for some accounts. When looking at these accounts, we find that the column for the account in accounttypes_users has been removed the user's roles stripped back to just "authenticated user". This seemed to be happening fairly randomly.
After some further examination, we noticed that these problems were always preceded by the timezone setting being updated by the autotimezone module, which we also use. I have been able to piece together the following way of recreating the problem, if you have autotimezone and accounttypes installed.
- Setup an account with an account type as usual.
- Go into your database server and set the timezone to an incorrect value (e.g., 0 since the setting should be -21600 for me)
- Also modify the data column for the account to remove the "selectAT" value, which means searching for a string like 's:8:"selectAT";s:1:"4";' and removing it from the data column.
- Now login as the account you have just setup.
On my setup, the first page looks fine, but the second page I load (if I need a role I don't have) will show "Access Denied" because the first hit after login resulted in user_save() being called to update the timezone, which resulted in the accounttypes module also stripping the roles out because selectAT doesn't seem to be set correctly during this save, but _accounttypes_user_exists() is returning a true value.
I have not tried to recreate this problem without the autotimezone module.
I'm trying to pin down the problem further and produce a patch, but I wanted to go ahead and post the problem here in the meantime. I will add a patch as soon as I have one.
Comments
#1
So in your initial setup, did you manually populate the accounttypes_users table rather than do it from within Drupal? That seems like a very bad idea to me. I understand that you have a lot of users and that the users/users page doesn't show many per page. Perhaps that could be adjusted so that perhaps 100-200 users could be listed at a time. Then, using the select all checkbox and the drop down to set accounttype, saving the change should include the SelectAT user item.
Either that, or if manually filling the accounttypes tables, fill the user's data field as well.
Let me know if I totally misunderstood you.
#2
I went in through our MySQL server and executed several "REPLACE INTO accounttypes_users SELECT ..." statements.
As far as I can tell at this point, the source of the problem is that Account Types module seems to save the roles on an account at every single user_save(). This is problematic for it's own reasons. It should be that roles are not updated except when the user makes a change. Otherwise, custom role settings might be obliterated by Account Types. I am still reading through the code to ascertain the exact conditions where the problem will come up so that I can craft a patch for review that makes sense.
#3
Okay, I understand what is happening now. I learned something new and significant in the process. The
$GLOBALS['user']user is not loaded usinguser_load(). As such,hook_user()is never called and, thus, the selectAT value stored within the data column is used (and empty) whenuser_save()is applied to the current user, which results in the Account Types module stripping all roles since it appears to be empty.This is, as far as I'm concerned, a deficiency in the Account Types, for two reasons:
accounttypes_user(), it looks to me like Account Types tries to avoid this by setting$edit['selectAT'] = NULLat the end of the "update" case. However, since$editis not passed by reference (i.e., not declared&$editin the function definition), this has no effect as "selectAT" is saved in user.data anyway.$edit['selectAT']has been set in the form. Otherwise, if someone has set roles on a user other than those that have the accounttypes_roles.initial field set, those roles will be stripped from the account, which I think is an undesireable side-effect if any other module (such as autotimezone) performs updates automatically or without the full user update screen.I am currently working up a patch to fix both of those problems and will attach it shortly.
#4
Here's my patch. I have replaced the
_accounttypes_user_exists()function with theaccounttypes_user_accounttype()function which performs the same task, but also returns the actual atid for the given account.I also removed all of the code for updating roles, since this can be performed just by modifying the
$edit['roles']value and lettinguser_save(). Do the hard work.This patch makes the following changes in functionality:
user_save().$edit['selectAT']has been set, which prevents the hook from trying to update the account when no change concerning the account types module is being made to the account.I have tried to be thorough with code comments to make it clear what is happening at each step.
#5
I'm behind in my module updates, but look forward to reviewing this as soon as possible. This was the second module I made and I'm afraid it is showing. Hopefully, you've fixed things. Thanks for the patch.
#6
No problem. I can totally relate, my modules are in a similar state, unfortunately. Cheers.
#7
Have you tested the case of implementing this on an existing site with existing users? Also does this account for both the user/user and user/#/edit cases or just for the latter?
Otherwise it looks good and cleans up some of the code. Thanks.
-Luke
#8
The patch does not handle the user/user case properly and may also have a problem on inserts.
I'm not sure about the latter because there is a possible interaction between the Promotion module (which I also use on the same site I'm using Account Types), which might also be causing this problem and is a bug in that module (which I maintain).
#9
Subscribing.
#10
subscribing.
i am having issues with logintoboggin and its interaction with account types for specific scenerios.
Chris
#11
Subscribing.
#12
In a case I experienced, no issues with timezone, not using autotimezone either.
When storing the account_type matrix, only the top row is stored, all other rows blanked out.
After manually inserting the correct values directly to the database using phpMyadmin, we cannot click on the save button of the account_types matrix window (where you specify the desired settings for the roles), or the table will be emptied again, only retaining the values for the first custom role (role #3). I dont know if this is because it is the first custom role, or if it is because it happens not only to be the first one, but also to be both an admin role AND sorted first in the alphabetic listing of roles. So not sure if that is relevant to why this does not happen with that role in particular. It is the role of the current user who clicks on save, though, but this user also has 4 other roles whose account_types settings also are emptied if saving.
So we are left dealing with this through phpMyAdmin for now.
The module seems to work as long as we dont use the GUI save function.
Edit: "only the top row is stored, all other rows blanked out".
Bad formulation. The settings for the first role, the first horizontal row in the GUI are stored. That may be several rows in the db, though. But the only ones retained after the save are related to the first role shown in the matrix.
#13
My problem was not with account type at all
#14
Does this patch work/fix the problem?
Anyone who can confirm?