The pre-auth user feature does not work properly

Mine is broken,too.

But I do not have something called og.module.

can this be caused by any call to user_load()?

just verified the workaround mentioned above. It did *not* work for me.

On account verification, there are two calls to user_load() before I reach the page, informing me, that I have successfully validated my account. Sorry, but I can't find the origin of those two calls right now.

My workaround is probably unwise but working for now:

in logintoboggan.module comment out the line removing the temp role and replace it with a query updating the table to authenticated user.

      // Remove the pre-auth role from the user, unless they haven't been approved yet.
      if ($account->status) {
        //db_query("DELETE FROM {users_roles} WHERE uid = %d AND rid = %d", $account->uid, logintoboggan_validating_id());
        db_query("UPDATE {users_roles} SET rid = '%d' WHERE uid = %d", 2 ,$account->uid);
      }

My question is: how can someone be logged in without having a role assigned?

I think, this is a general bug in the user.module.

user_load() considers the authenticated role to be "magic". It includes the code:

    $user->roles = array();
    if ($user->uid) {
      $user->roles[DRUPAL_AUTHENTICATED_RID] = 'authenticated user';
    }

so no matter roles the user has, they *always* get to be an "authenticated user" on load.

To fix this, logintoboggan should unset this on load. Here's how I've done it: modify the end of logintoboggan's hook_user to implement hook_user('load', ...) as follows:

function logintoboggan_user($op, &$edit, &$user_edit, $category = NULL) {
  if ($op == 'form' && $category == 'account' && $user->uid == arg(1)) {
    ...
  }
  elseif ($op == 'load') {
    // Just loaded the user into $user_edit.
    // If the user has the pre-auth role, unset the authenticated role
    $id = logintoboggan_validating_id();
    $in_pre_auth_role = in_array($id, array_keys($user_edit->roles));
    if ($user_edit->uid && $in_pre_auth_role) {
      $user_edit->status = 1;
      if ($id != DRUPAL_AUTHENTICATED_RID) {
        unset($user_edit->roles[DRUPAL_AUTHENTICATED_RID]);
      }
    }    
  }

This works for me so far (as verified with the invaluable devel.module "object" view of users).
I may have a patch later to fix this one (currently battling missing username clash verification problem), but in the meantime someone else may want make one?

Best wishes,

Mark
www.PostgraduateStudentships.co.uk - where ideas and funding meet

Regarding the *magic* of authenticated user role:

When a user registers and logs in (no special role, just an authenticated user):
does drupal write his user id and role id to the roles table?
Or does drupal only write other roles than authuser to that table?

I've not gone through it in detail, but in user.module, user_save() does

function user_save($account, $array = array(), $category = 'account') {
...
      foreach (array_keys($array['roles']) as $rid) {
        if (!in_array($rid, array(DRUPAL_ANONYMOUS_RID, DRUPAL_AUTHENTICATED_RID))) {
          db_query('INSERT INTO {users_roles} (uid, rid) VALUES (%d, %d)', $account->uid, $rid);
        }
      }
...
}

In other words, DRUPAL_AUTHENTICATED_RID (the "authenticaed user" role) is never actually written in the database. Drupal (user.module) assumes that anyone who isn't anonymous is an authenticated user: anyone with $uid != 0 is assumed to be an "authenticaed user", anyone with $uid == 0 is assumed to be anonymous. My guess is that this is to save space in the {users_roles}, since it would (as the assumption goes) apply to everyone.

Logintoboggan goes against this assumption, so it has to "undo" it wherever it happens.

As far as I can see, it only happens in user_load(), so by responding to hook_user('load') we get to undo this magic after the user has just loaded.

I've tried to extract this bit to make a possible patch for 4.7.x - please see if this works, but beware possible syntax errors due to cut and paste. Apologies I've not had the chance to run it myself in this separated-out form [I wouldn't normally submit something untested but I've got too many changes in my local version by now to extract easily].

Hope this helps, Mark.

(Forgot to change status. Mark.)

i don't believe the patch will fully solve the issue.

the removal of the auth user in hook_init() is necessary, because it must be removed before the menu items are built--otherwise the user gets menu items listed that they are not permitted to access.

it's probably not a bad idea to also have the removal code in the load op of hook_user() -- it would be another safety net if another module overwrites the changes being made. ultimately, though, this problem could occur at any time another module writes to the user object during hook_init(), hook_menu(), hook_load(), and maybe some others :)

i believe it's possible since drupal 4.7 to customize the order that modules are called by the hook system. my recommendation would be to get some doc in the module that explains how to put LT last in the workflow, and probably also commit this patch. drupal core's handling of the auth user has made this feature of LT a bit hard to guarantee... :)

Yes you're right that the role removal code is necessary in hook_init() too (as already in logintoboggan), as well as in hook_user('load', ...) as introduced in the present patch.

So the role removal code in hook_init() must not be removed, but the present patch is also needed to guarantee it is not reintroduced if user_load() is called anywhere. E.g. Organic Groups does this during its hook_init() implementation.

Here's some relevant bits of Drupal bootstrap process to see how this goes (pre-auth user assumed):

1. In bootstrap phase DRUPAL_BOOTSTRAP_SESSION, after the session handlers are set session_start() is called which calls to sess_read() in session.module
2. sess_read() performs
   

     if ($user->uid) {
       $user->roles[DRUPAL_AUTHENTICATED_RID] = 'authenticated user';
       ...
     }
   

   which is a copy-paste of the code in user_load() in user.module. It can't call user_load(), which does this too, because modules aren't loaded yet. IMHO this is a candiate for refactoring, but that is another issue...

3. In bootstrap phase DRUPAL_BOOTSTRAP_FULL, drupal_bootstrap_full() in common.inc calls module_load_all() and module_invoke_all('init').
4. Function module_invoke_all('init') invokes modules in order given by module_list(), which is SELECT ... FROM {system} ... ORDER BY weight ASC, filename ASC. (Changing weight is what you need if you want to get modules loaded in a different order.)
5. Modules listed before logintoboggan get their hook_init() called, with user appearing to be in "authenticated user" role.
6. logintoboggan_init() is called, which (in existing logintoboggan module) unsets the "authenticated user" role. User is now not in "authenticated user" role.
7. If Organic Groups is installed, og_init() calls user_load(), which again calls
   

     if ($user->uid) {
       $user->roles[DRUPAL_AUTHENTICATED_RID] = 'authenticated user';
       ...
     }
   

   Without the present patch, user is now in "authenticated user" role again. Oops.

8. Initialization completes, and menu_execute_active_handler() is called to determine and execute the requested menu item, if allowed.

You could argue that OG (or other modules) shouldn't mess with the user object in bootstrap mode, but with the present patch its no problem anyway.

Actually, to be really safe, logintoboggan should be initialized first, in case modules check on the permissions of the user object during hook_init(). So it could set its entry in {system} to some weight=-large during logintoboggan_install() to try to get it to be initialized first. E.g. something like

      db_query('UPDATE {system} SET weight = -10 WHERE name = "logintoboggan" ');

might do the trick (see e.g. og_install for a similar manipulation on install), but I would suggest that this should be in a separate issue/patch.

Finally, if any modules change user data, user_save() in user.module refreshes the user object by calling user_load() towards the end. Without the present patch, the "authenticated user" will again be reintroduced.

Another reason not to rely on hook_init() is when e.g. the admin user looks at the profile of another user. This will call hook_load(), which (without the present patch) will erroneously set the "authenticated user" role. This means that e.g. the devel.module object view will incorrectly report the roles for that user.

So while I agree that additional work (e.g. setting weight as above) would be needed to completely remove any chance that an additional module will see an unintended "authenticated user" during the bootstrap (hook_init()) phase, I suggest the code in the present patch (or its equivalent) will ensure that any post-bootstrap code will not see a pre-auth user as authenticated. Hence I believe that the present patch can be applied as is.

Best wishes,

Mark.

@mark -- a very sensible and thorough look at the issue -- thanks!

before i apply the patch, i'd like to consider if there are any other places besides user_load() where we might need to watch out for this same kind of problem. i don't want to hold it up too long, but if anyone can see a similar problem elsewhere, speak now. :) it's possible that if we find some other holes, we might want to refactor how this is accomplished...

A quick fgrep for '{role}' and '{users_roles}' in 4.7 suggests that only session.inc and user.module in core have anything to do with user roles. The only exception is database/updates.inc, in which system_update_171() calls

  update_sql('DELETE FROM {users_roles} WHERE rid IN ('. DRUPAL_ANONYMOUS_RID. ', '. DRUPAL_AUTHENTICATED_RID. ')');
}

which in hindsight gives the game away a bit :-)

As long as other modules behave properly, it should be OK. I'm aware that legal.module previously tried to load users directly rather than call user_load() - see http://drupal.org/node/74023 for the issue that fixed that - but I've not come across it anywhere else.

Best wishes,

Mark.

so at the end of the day, we're saying that other modules should be using user_load to get any user objects, and if they're not, then it's a bug in their code. i'm ok with that :)

i thought of another reason why we need this: you can user_load() any user, not just the active user -- so logintoboggan would currently fail in any kinds of checks like that.

this leaves us with the module loading order problem, but it's certainly much more minor after this patch. we'll address that later. :)

iirc, this patch uses basically the same logic as what's in logintoboggan_init() -- can we refactor things into some kind of a logintoboggan_check_auth() function, so we don't have repeated code?

OK, try this, Mark.

subscribing

committed the patch to 4.7 and HEAD -- great work, mark!

i'm setting the status back to active for a bit -- although i've tested that it works properly for me, i don't have OG installed anywhere, and have never really had the problem. can i get some feedback that this in fact fully solves the issue?

since nobody has reported any problems, i'm marking this as fixed. please reopen if necessary.