stella initially reported this as a security issue at s.d.o, but hunmonk, greggles and meba (and I) feel this is more of a misconfiguration and should be documented to be avoided.

Below is an excerpt from stella's original report:

Setup:
  • On 'User settings' page, set 'Visitors can create accounts but administrator approval is required' as the registration option (i.e. status = 0)
  • In the logintoboggan settings, enable 'Set password' option but ensure 'immediate login' is disabled
Result:
  • User registers on the site and can enter their password.
  • From looking at the db, I can see the user status = 1 and they have the pre-auth role
  • User receives the email saying your account is awaiting approval, but they can still log in using username and password entered during registration - admittedly this is with pre-auth role but by default that is the 'authenticated user' role.
  • However, if there is an email verification link in the email, then the user can log in as a normal authenticated user, no admin approval required.
I would have expected that even though the user can enter their own password, their account should still be blocked until an administrator approves it, regardless of the fact that a 'pre-authorized' role can be given to the user (which is removed once email address is verified). However if the 'set password' option is enabled, then the account is immediately activated regardless of any other settings.
CommentFileSizeAuthor
#3 improve_configuration_comment-1374134-3.patch894 bytesstevecowie

Comments

dokumori’s picture

dokumori commented
Title: Update the document to avoid misconfiguration » Update the comment to avoid misconfiguration

Adding another line of excerpt from stella's report and changing the title for accuracy:

'. . . [the comment] gives the impression that it should be checking the 'immediate login' setting too.'

The comment in question: (from line 324-327 in logintoboggan.module, 6.x-1.10 , or line 449-452 in 7.x-1.3)
// If we are allowing user selected passwords then skip the auto-generate function
// The new user's status should default to the site settings, unless reg_passwd_set == 1
// (immediate login, we are going to assign a pre-auth role), and we want to allow
// admin approval accounts access to the site.

According to hunmonk, this comment is outdated and should be updated.

stevecowie’s picture

stevecowie commented
Assigned: Unassigned » stevecowie

The comment is broadly correct for D6 but no longer makes much sense on D7. Here's a suggested update.

// If we are allowing user selected passwords then skip the auto-generate function
// The new user's status will be 1 (visitors can create own accounts) if reg_pass_set == 1
// Immediate login, we are going to assign a pre-auth role, until email validation completed

I think only a minor change of the variable name from reg_passwd_set to reg_pass_set is needed for D6.

stevecowie’s picture

stevecowie commented
StatusFileSize
new improve_configuration_comment-1374134-3.patch894 bytes

Here's a patch that applies the change suggested at #2

stevecowie’s picture

stevecowie commented
Status: Active » Fixed

Patch now applied to dev version. Should have marked this previously as 'needs review' but seems so minor, thought I might as well commit.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.