User creation setting should be "Visitors, with admin approval"

I would vote for a warning somewhere in text-guides, as many sites (MOST sites, I believe) need the user registration working the currently default way, so "secure defaults" would mean breaking the out-of-box functionality, making Drupal more difficult to set up in common cases.

Have to agree with Gaele here.

I guess it depend on use cases [will you site allow open registration or not].

My opinion is that open registration should be a conscious choice, and should be done when access rules have been tested and refined.

I would see this as a "usability bug/feature" and thus still OK for Drupal 6.

Alan

I have no strong reason to push on this - fine with me with one comment: If default changes to disabled free registration, then the step to allow visitors for registration should be definitely mentioned somewhere in the installation process.

Well, this is an "install profile" thing. There was a push to include more install profiles in Drupal 6, but it did not happen and we are in bugfix mode, so new profiles will not be accepted. Whether you set up a community site or a brochure site makes a difference in how you allow user registration.

And the default install profile is aimed at what?

Neither, both.

I think having the default behaviour to block new user registrations would result in a lot of support requests, agree this would be good in a brochure site profile though.

I agree with:

A) defaulting to closed (or at least requires-admin-approval).
B) making this setting visible in the installer.

We're now going to bother everyone installing drupal with a question of if they want to enable the update.module to help keep their site more secure, in the interest of appeasing the <1% of our user base who's so freaked out about privacy they don't want to have their site check for updates (see http://drupal.org/node/178581). Therefore, I think it makes perfect sense to have a question in the installer about such a fundamentally important aspect of how the site is configured. This is something that nearly 100% of drupal installations should think about, and no matter what we choose as the default, we'll probably be wrong for 2/3rds of all sites.

The patch inserts a new fieldset to the configure form during installation where the registration options can be set.
There should be some additional explanation on how this setting affects the site behaviour so literally this patch needs some work.

This patch would get a thumbs up from me, perhaps also with an addition to the frontpage checklist of things to do when setting up a site, saying something like 'you may now wish to allow users register for your site by changing the /Public registration/ option on the User settings page'.

Often when a site is started, even if it's a community site, you wouldn't want to let people in until you were ready to launch it anwyay, so the default is good in my opinion.

Just stumbled upon the issue. I frequently set up small websites with Drupal and with the time I had overlooked this aspect. As a result, one of my sites (by luck, one in development) was left with the default setting of "Everybody can register without anyone being notified"... This is a dangerous default behavior.

I'm 100% supporting a patch like the above, or at least that the default be changed to "Visitors can create accounts but administrator approval is required."

Let the testbot have a look

Testbot is ignoring the patch. Probably because it's from 2007.

This is probably dead code, but possibly not a dead issue.

Merging in some duplicate issues:
#698144: D7: Fresh install allows visitors to create accounts
#794892: Change default account creation setting to "Administrators only"

We should get this taken care of.

I guess there are quite a few tests that assume the (former) default.

Security should trump the historical default.

People who know what to do, will configure who can create accounts. Novices, who are initially unaware that such an option exists, will be glad that their site is not left open to anonymous sign-ups.

OK, hopefully the bot will like this one. It just adds the traditional default to the tests that failed in #19.

One more try. I think I have them all now.

Security should trump the historical default.

This is not a security issue - there is nothing insecure about allowing people to sign up for accounts on your site.

As we discussed in one of the duplicate issues, the default install allows people to sign up for accounts and then to write comments and upload user profile pictures... however, this isn't an issue of security; it's one of expectations. Plus, only other registered users can see these by default (so your site won't be a particularly effective spam engine if people try to use it that way), and they're easy for an administrator to delete. (Note also that the latest patch at #364159: Enable 'access comments' permission for anonymous users by default. would enable comment moderation for registered users from the get-go.)

Considering that one of the biggest strengths of Drupal is that it allows you to build community websites, we should be pretty wary of turning this off by default. I can sort of see the argument for why it should be the default value of the variable (in the user.module itself) and therefore inherited as such by the Minimal profile, but for the Standard install profile to do this too the argument seems much weaker. I think one of the primary points of the Standard install profile is for people to get a quick sense of what Drupal is and the main things it has to offer, and user registrations are a big part of that.

The suggestions/patches earlier in the issue of making this an option in the installer are kind of an interesting compromise, although anyone who suggests adding yet another option to the installer comes under great scrutiny these days (and rightfully so) from anyone who is interested in usability :)

@David_Rothstein, I disagree with you on a practical level. On many sites, and probably by default "authenticated user" has some privileges that the site owner did not intend to grant to unknown people. Thus new site, unintended users, unintended privileges.

I apologize for only skimming the above comments. If I missed something important please find me in irc and point me to it.

At the OWASP conference in DC in 2009 I talked to a lot of "security" folks and they all said "why would you allow this by default?" And then at the security BOF in San Francisco Micah Tapman mentioned this as something that should be changed on every site.

In Colorado 2 years ago there was a state political candidate who had a site built for her where the authenticated role had full administrative permissions and where this default ability to create accounts was still present, essentially allowing anyone who knew how to find /user to become site admin. A person who happened to be a political adversary used that to put her site offline which was both a favor to her because it prevented other attacks and a real nuisance (background on this site).

So, I would say that this is generally perceived as a security issue and that we have real world data that it created security vulnerabilities on a site.

My preference would be to require administrative approval. It adds to security while alerting an admin if/when someone tries to create an account. That seems safe enough while still giving context appropriate information in the case someone is signing up rather than just an "access denied."

As discussed above, the install profile needs to be set, as well. This patch adds that.

greggles makes a strong argument for allowing visitors to create accounts, but with Admin approval. This prevents the security side of this issue, but is softer toward the traditional community-centered nature of Drupal. A site by default would allow visitor account creation, but they wouldn't be able to do anything until the admin approves. There will still be some confusing spam accounts to clean up, but this is a smaller step away from the traditional default, and stays closer to Drupal's values.

Changing the title.

New patch that changes the default to "Visitors, with admin approval".

This is not a security issue - there is nothing insecure about allowing people to sign up for accounts on your site.

If I'm not expecting people to be able to sign up by default, then I consider this to be a security issue, even if it is not in the usual sense.

I'm OK with this patch in principle, although I don't really consider it a security issue. But let's do it right:

+++ modules/comment/comment.module
@@ -2251,7 +2251,7 @@ function theme_comment_post_forbidden($variables) {
-      if (variable_get('user_register', 1)) {
+      if (variable_get('user_register', 2)) {

We should be using constants for this instead of integer values.

+++ profiles/standard/standard.install
@@ -272,6 +272,9 @@ function standard_install() {
+  // Allow visitor account creation with admin approval.
+  variable_set('user_register', 2);

In documentation we write 'administrator' instead of 'admin'.

@Dries, all of the code in all of core uses the raw integer values (and always has). My temptation was just to use string values for the variable, but that would break compatibility for upgrade. Your approach preserves compatibility, but it was not the intent of this humble patch to fix Drupal's approach to this variable. That said, I agree with you, we should fix this ugliness. I guess a new constant definition can go into common.inc, even though this is a user.module variable. Do you have a better suggestion of where the constant definition should go?

The constant should go into user.module. Unless for some reason it's required earlier than user.module is loaded, which I really hope it isn't.

Here's a patch addressing #33, using constants instead of integer values.

Just a typo, but took me about 20 minutes to get it :-(

This one should pass the bot.

I guess this compromise setting makes some sense - it has a bit of the benefits of both approaches. At some point, we really need to figure out what the purpose of our install profiles are, though, as it would make these kinds of things easier to decide :) Pretty much anything can be construed as a "security risk" for certain types of sites; it all depends on what they do with the site after they install it.

Anyway, the patch:

1.      '#title' => t('Who can register accounts?'),
   -    '#default_value' => variable_get('user_register', 1),
   +    '#default_value' => variable_get('user_register', USER_REGISTER_VISITORS),
        '#options' => array(
          t('Administrators only'),
          t('Visitors'),
   

   The #options array should be keyed by the new constants - i.e., USER_REGISTER_ADMINISTRATORS_ONLY => t('Administrators only'), USER_REGISTER_VISITORS => t('Visitors'), etc.

   (I have no idea what adding these constants has to do with this issue by the way, except for the "while we are touching some of the same lines of code" factor... but it's a good idea anyway.)

2.  /**
   + * User creation permission options.
   + */
   +define('USER_REGISTER_ADMINISTRATORS_ONLY', 0);
   +define('USER_REGISTER_VISITORS', 1);
   +define('USER_REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL', 2);
   

   Each of these should get its own docblock.

3. In the latest patch you are leaving the user.module default at USER_REGISTER_VISITORS but changing it in the install profiles, but previous patches actually modified the user.module default itself... why the change? I guess it makes some sense in that it minimizes the effect of this patch this late in the release cycle, but it seems a little odd to me to have the Minimal profile explicitly have to override this setting.

This patch addresses the excellent comments in #39. #3 there was an error on my part.

David_Rothstein++
Community Review++

I think we need a 6-7 update which keeps the existing variable default for sites which upgrade, but don't have this explicitly set. Either that or display a message on update that the default is being changed and they might want to review it.

I suggest we go with the first idea (keeps the existing variable default for sites which upgrade, but don't have this explicitly set) to minimize changes to the UI.

This adds a user_update_N() to set the user_register variable to the D6 default if it was unset. Other than that, it's exactly the same as #40.

Do you think this looks good to go? Would appreciate either a comment or an RTBC. Seems like pretty simple stuff.

Sure, seems like a great idea to me - let's RTBC.

Oops, lost track of this one, but yeah, the latest patch looks good.

However, I'm still not understanding something about the changes to the install profiles in this patch... If USER_REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL is the new variable default used throughout the user module, then why do the install profiles have to bother calling variable_set('user_register', USER_REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL)? It seems like that doesn't accomplish much except store an extra row in the database...

@David: Reading the issue it looks like I put the install profile variable setting in as a response to your #27, but it kind of looks like I misunderstood what you were saying. However, I don't think it does any damage. IMO it's probably better to have it explicitly in the variable table than just implicitly in the code in a thousand places. For example, a contrib module may still have a different default in the variable_get() (logintoboggan for example). Better to have it explicit.

Committed to CVS HEAD. Thanks.

OK, I guess that makes sense. Probably not a precedent we want to set for all variables, but this is kind of an important one :)

Setting this to "needs work" for documentation... seems like the new constants as well as the new default value for the variable should be documented in the module upgrade guide?

I'll put it there.

The more important thing is to notify end users of the change. I wonder how we do that.

Added to the D6->D7 upgrade guide, http://drupal.org/update/modules/6/7#changed_user_default

Marked #723124: make the possible values of user_register explicit as a duplicate

I updated http://drupal.org/update/modules/6/7#changed_user_default a bit to also refer to the new defined constants that were introduced here.