Make WordPress Core

Opened 15 years ago

Closed 14 years ago

Last modified 14 years ago

#10396 closed task (blessed) (fixed)

Pick an username during the install

Reported by: dcole07's profile dcole07 Owned by: dancole's profile dancole
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.9
Component: Upgrade/Install Keywords: commit has-patch tested
Focuses: Cc:

Description

When you install WordPress it gives you the user name "admin" and a random password. It should instead allow you to decide your own user name. There are security benefits and as well as personal preference benefits to doing this. People can currently do this manually, but it's a long process of login in and out multiple times, going to the user section and add / deleting accounts. It would just make more sense for the installer to pick a user name, other CMS allow you to do this.

I'd don't know if this has been brought up before. I searched Trac a little and it didn't come up with anything obvious. I'll pick this up if that's what it's going to take to get this into 2.9.

Attachments (10)

10396.diff (2.4 KB) - added by dcole07 15 years ago.
Basic Picking of default user name.
10396_b.diff (2.6 KB) - added by dancole 14 years ago.
Second version, with admin check.
10396_c.diff (12.5 KB) - added by dancole 14 years ago.
Pick user name and password
10396_d.diff (13.0 KB) - added by dancole 14 years ago.
Fied problems nacin brought up and reworded some content.
10396_e.diff (13.1 KB) - added by dancole 14 years ago.
Updated so the patch applies correctly.
10396_f.diff (13.2 KB) - added by dancole 14 years ago.
Made changes that dd32 suggested.
10396_g.diff (13.2 KB) - added by dancole 14 years ago.
Fixed errors in 10396_f.diff
10396_h.diff (13.0 KB) - added by dancole 14 years ago.
Rebuilt after bring nuked by r13124
10396_fix_a.diff (3.8 KB) - added by dancole 14 years ago.
Corrects mistakes in trunk
js_pass_check_a.diff (4.1 KB) - added by dancole 14 years ago.
Dev. JavaScript Password Mismatch Checker

Download all attachments as: .zip

Change History (69)

#1 @dcole07
15 years ago

This should be too hard check out #L119, http://core.trac.wordpress.org/browser/trunk/wp-admin/install.php#L119 We just need to make 'admin' a variable, add the form field, and make sure people aren't doing anything weird with their user name basically.

BTW, did dd32 auto-own this?

#2 @nanochrome
15 years ago

If this gets in this will be a life saver.
btw dcole07 that easy eh?
how hard would it be to be able to enter the tagline?

#3 @dd32
15 years ago

Yes, The upgrade/install component tickets are auto-owned by me, since the component was seemingly designed for plugin/theme/core automatic upgrades/installs rather than WordPress Database Upgrade/Installing

Theres a small gotcha to the idea of choosing the username though..

Currently if the options table crashes, then WP will offer to re-install WP, it'll get to that screen and come up saying "Hey, the admin user already exists, So i'm not going to create you a new user and give you admin access!"

how hard would it be to be able to enter the tagline?

An extra form field, and an extra update_option()..

#4 @dd32
15 years ago

Theres also the issue of shared-user tables that can cause small issues like that..

#5 @dd32
15 years ago

+1 for choosing the username, and only creating it, If there exists no other administrator-level user accounts in the current DB maybe.. this would probably be implemented easily after #10201

#6 @michaelh
15 years ago

If there are other users in the database, is it reasonable to say "user should not be able to pick 'admin' as the username, even if 'admin' does not exist?

Just in case there's old code somewhere (e.g. plugins/themes), that assumes 'admin' is the administrator...

@dcole07
15 years ago

Basic Picking of default user name.

#7 @dcole07
15 years ago

  • Cc dcole07 added
  • Keywords needs-unit-tests added

I created a patch that works for a clean install with no complications, as an initial starting point. I don't have any experience with installing WordPress with problems or anything special, so I really can't grasp what problems dd32 and michaelh say can happen. What does Wordpress currently do if someone installs WordPress, then deletes the 'admin' user account and uses another user name, then does a reinstall or shared-user table or whatever your talking about?

#8 @dd32
15 years ago

Just in case there's old code somewhere (e.g. plugins/themes), that assumes 'admin' is the administrator...

If they're not using the roles/capability system, Its not even worth worrying about IMO, it'll be broken in every other way possible..

#9 @dcole07
15 years ago

Fresh Install Testing: I tested a fresh install with patch 10396.diff on WordPress 2.8.1. It worked like you would expect it to and there were no problems or errors.

Existing Database Install Testing: I deleted the config file and ran the install process again on WordPress 2.8.1 with patch 10396.diff. It didn't allow me to create a new user and said there was already database tables. This is expected. There were no problems or errors.

#10 follow-up: @dd32
15 years ago

Existing Database Install Testing

Try marking the options table as dirty/crashed (if its possible to do that..) or just empty the options table. See if it still acts the same.

#11 in reply to: ↑ 10 @dcole07
15 years ago

Replying to dd32:

Try marking the options table as dirty/crashed (if its possible to do that..) or just empty the options table. See if it still acts the same.

Deleted wp_options Table I deleted the config file and empted the wp_options database table. While installing I created a new user, the install was a success, but resulted in 2 administrators. One from the fresh install and one from the re-install. This was again done on WordPress 2.8.1 with patch 10396.diff. There were also dup errors on the Success page:

  • Duplicate entry 'uncategorized' for key 'slug'
  • Duplicate entry '1-category' for key 'term_id_taxonomy'
  • Duplicate entry 'blogroll' for key 'slug'
  • Duplicate entry '2-link_category' for key 'term_id_taxonomy'
  • Duplicate entry '1-1' for key 'PRIMARY'

#12 @dcole07
15 years ago

Deleted wp_options Table, same username
I repeated the steps in Deleted wp_options Table but instead of creating a new username, I used the one from the original install. I got the same dup errors. As for the username and password, it said the username already existed and that the password was inherited. Overall the install was a success. When I logged in, I used the original information and it worked.

#13 follow-up: @dd32
15 years ago

but resulted in 2 administrators.

That is the exact situation i wanted to avoid, Look around the forums and a few track tickets, and you'll find things related to the options table crashing (more likely than any other table). Once that happens.. administrators are created which the user may not be aware of - I dont think end users should be open to that risk, Technical users may realise and check, but not everyone is going to..

Which is why my suggestion was to only allow it if no other users existed, or no admins existed for the current blog.

#14 in reply to: ↑ 13 @dcole07
15 years ago

Replying to dd32:

That is the exact situation i wanted to avoid [...] administrators are created which the user may not be aware of [...]

I can see how this enhancement would increase that problem, but this probably already happens with the current way of doing things. People can create and delete administrators, whether that be "admin" or not. I see what your bring up as a related problem, but not one necessarily caused solely by this patch. I think that the problem you bring up should be patched, but should it be done by this ticket or another one? ... or doesn't it matter?

#15 @dcole07
15 years ago

If we want add a user check to this ticket:
If there are already users, how do I stop the creation of a new user during re-install? Don't give a username field and pass an empty username!? I hope that doesn't cause problems. The other option is to edit the wp_install function, but I hope it doesn't come to that.


Note: We can get the number of users by counting the result of get_users_of_blog()

#16 follow-up: @dcole07
15 years ago

Not allowing an additional administrator to be created is good for the people that don't check for forgotten administrators after an options table problem, but what if someone deletes all the administrators or gets locked out of all the administrator accounts some how? What rules do you want me to follow? I was planning on checking the number of users, not number of users with the admin role, but that could be a problem.

#17 in reply to: ↑ 16 @dcole07
15 years ago

Replying to dcole07:

Not allowing an additional administrator to be created is good for the people that don't check for forgotten administrators after an options table problem, but what if someone deletes all the administrators or gets locked out of all the administrator accounts some how? What rules do you want me to follow? I was planning on checking the number of users, not number of users with the admin role, but that could be a problem.

I'll wait for #10201 to be patched and check for any administrator roles.

#18 @dd32
15 years ago

but what if someone deletes all the administrators or gets locked out of all the administrator accounts some how?

Then re-installing shouldnt be the way they go about it.. Instead, They should be reseting the users password via the forgotten-password link or manually inserting a md5 into the users table via phpmyadmin.

I'll wait for #10201 to be patched and check for any administrator roles.

That sounds like the best method of attack to me. - Either that.. or add a if ( user_exists('admin') ) $username = 'admin'; for now with a new blocker ticket created to move that from check for admin user, to check for administrator roled users..

@dancole
14 years ago

Second version, with admin check.

#19 @dancole
14 years ago

  • Milestone changed from 2.9 to 3.0

I'm switch over to a different name... dancole instead of dcole07.

I uploaded 10396_b.diff. This version now checks to see if the admin account exists and then forces the installer to use that user name when necessary.

I'm going to push this back a release until 3.0. #10201 got delayed and were in a feature freeze now. 10396_b.diff should be ready to go, even without #10201... it'll just need a new blocker ticket.

#20 @dd32
14 years ago

  • Owner changed from dd32 to dancole
  • Status changed from new to assigned

Just changing the ownership of this ticket over since it assigns this component to me automatically.

#21 @hakre
14 years ago

  • Milestone changed from 3.0 to 2.9
  • Version changed from 2.9 to 2.8.4

Hmm, this feature was requested already for 2.9. If we have a feature freeze, than this means, that this feature has to go in.

While we're on it, the main admins ID shouldn't be 1 if we're in this for security.

#22 @dd32
14 years ago

  • Milestone changed from 2.9 to 3.0
  • Version changed from 2.8.4 to 2.9

No, A feature freeze means that only functionality which was in development in core is set for inclusion in release.

Please leave this set for a future release unless a core developer decides that its important enough for inclusion. This was never talked about during the developer meetings on what in-development-features should be included AFAIK.

#23 @caesarsgrunt
14 years ago

+1

Should also allow you to choose a password.

#24 @hakre
14 years ago

on wishlist30 for dancole

#25 @janeforshort
14 years ago

I think it would be a nice feature, personally, and I'd even lobby for it with the lead developers, but only if:
a) there's a solid patch that people like dd32 think does the job in the best way,
b) that patch is done and well-tested by early in the dev cycle (say, by Feb 1) so that inclusion wouldn't push us, and
c) it doesn't draw lead dev time during 3.0 away from the merge and higher-priority features beyond basic reviews and the actual commit.

#26 @janeforshort
14 years ago

  • Type changed from enhancement to feature request

Changing type to "feature request" since that's what it is, really.

#27 @hakre
14 years ago

Dupe closed: #11599

#28 @hakre
14 years ago

Hey Jane, It's nice to see you argumenting again. Please make up some arguments related to security, sql and injections regarding the same ID and same NAME all the time for the default full access user as well. Just to give some hints why it's actually really important to handle this area more properly. All your queries are belong to us.

As long as the current implementation does not have unit-tests I see no argument to provide such for the changes. It's not mandatory for wordpress development. Even thought their ain't any units to test, right?

#29 @hakre
14 years ago

  • Keywords needs-patch added; needs-unit-tests removed

#30 @dd32
14 years ago

As long as the current implementation does not have unit-tests I see no argument to provide such for the changes. It's not mandatory for wordpress development

By that arguement, None should ever be written for anything.

I do however agree, Unit tests for this is pretty much useless.. for the simple reason its a bit hard to test such items, theres no distinct input/output.

Can a dev step in here and give a generic yay or nay on their thoughts of the function? A secure method can be built, but in the likes of keeping everyone sane, If a core commitor doesnt believe in it, its unlikely to get much attention.

#31 @dancole
14 years ago

If a core developer could make the goals and requirements clear, I'll do as much as I can to create a patch that will be committable, but if things are unclear, it'll take three or four more weeks to get things done, because I'll have to guess and check with people.

Goals:
Client is allowed to pick their own user name (only if there are no admin accounts.)
Client is allowed to pick their password (only if they can create a new admin account.)

#32 @ryan
14 years ago

As discussed on wordpress-dev with dancole, lets make the check against any users existing rather than just admins since #10201 won't happen in 3.0. With that changed, good to go.

#33 @ryan
14 years ago

  • Type changed from feature request to task (blessed)

#34 @nacin
14 years ago

Quoting from from wp-hackers (thread). It'd be best to keep this in the ticket, Trac is the place for this.

1 ) wp_enqueue_script can't load any scripts during the install process, because $wp_scripts is empty. What files do I include so I can call wp_enqueue_script() and have it find the JavaScript for the password-strength-meter?

Simply hard-code <script type="text/javascript" src="js/password-strength-meter.js"></script>, just as we do with the CSS file.

2 ) Is there a better way to get the number of users in the database than using: count( get_users_of_blog() ), because that function creates an error due to the fact that there isn't a database table during step 2 of the install process.

We employ a hack in sample-config.php to allow us to be wpdb aware, so we can and should run the query directly.

Post back here if you need help with the patch. It'd be good to get a rough pass attached at some point that way feedback can be provided early in the process, both programming and UI.

#35 @nacin
14 years ago

Of, of course, and if the table doesn't exist (it may if we're using shared tables), then it'd be easy to assume that the number of users of the blog is 0. :)

@dancole
14 years ago

Pick user name and password

#36 @dancole
14 years ago

  • Keywords needs-testing added; needs-patch removed

10396_c.diff allows you to pick a user name during the install process, but defaults to admin when the page load or when their are users. You can also pick your password. A strength meter will show you how strong it is. If it is left blank, a password will be generated for you.

Think of this patch as a rough draft... it still needs some work, but I'd like some feed back on the UX/UI, how the code looks, how it's done, and of course any bugs.

I should note that 2 errors are generated when it checks for users. In the next patch I'll figure out how to check for the table first or do something that doesn't generate errors.

#37 @nacin
14 years ago

Nice work so far... things I'm noticing on initial review:

  • We can't reorder the arguments on wp_install(), just as we have to keep backwards compatibility by generating a random password if empty($user_password). (The argument should be optional and have a default.)
  • We don't need the commonL10n.warnDelete string, but we would need convertEntities() which is in utils.js.
  • The sql query would be "show tables like '$wpdb->users'" to see if the table exists. Also, I don't think get_users_of_blog() is available at this time, anyway.

@dancole
14 years ago

Fied problems nacin brought up and reworded some content.

#38 @dancole
14 years ago

  • Keywords has-patch added

#39 @dancole
14 years ago

  • Keywords commit added; needs-testing removed

10396_e.diff has been updated to apply correctly to revision 13073. I then tested the patch by using the standard user name, as well as custom user names. I've also tested it with and without passwords. I've received no errors and it works like it should. I've also repeated some of the tests I did back in July, any concerns that have been brought up have been corrected and the things that were done right the first time have stayed the same. I think patch 10396_e.diff is commit ready and should be put into WordPress 3.0 before Feb. 15th.

@dancole
14 years ago

Updated so the patch applies correctly.

#40 @dd32
14 years ago

Quick review of the patch.

nacin: We can't reorder the arguments on wp_install()

Still exists in the latest patch

<input name="admin_password" type="hidden" id="admin_password" value="" />

What is the purpose of that? Is it for the JS?

<td><code><?php echo $user_name; ?></code></td>

Should probably be run through esc_url()

echo 'User(s) already exists.....

Should be translated, and the line can be split up into multiple.

If you want to tidy up those details I'll get it in

#41 @dancole
14 years ago

  • Keywords tested added

#42 @dancole
14 years ago

  • Keywords tested removed

oops, I didn't see dd32's commets. I'll make those changed.

@dancole
14 years ago

Made changes that dd32 suggested.

#43 @dancole
14 years ago

I'm going to do some testing on 10396_f.diff. It has at least one error.

@dancole
14 years ago

Fixed errors in 10396_f.diff

#44 @dancole
14 years ago

  • Keywords tested added

Hopefull 10396_g.diff will be the one. I use esc_html(), which I think is the right command... esc_url adds http:// to the front of the content. I also got ride of the hidden fields which aren't needed and added in the translation functions I missed. Hopefully wp_install() variables are in the right order this time. I gave that patch a quick twice through to see if I noticed errors. Should be good to go.

#45 @dd32
14 years ago

I use esc_html()

I was thinking esc_html() and wrote esc_url(), Sorry! :)

That patch looks better, I'll give it a test run later on

#46 @nacin
14 years ago

I just realized I nuked this patch in [13124]. Will try to refresh it later myself if I have a moment.

@dancole
14 years ago

Rebuilt after bring nuked by r13124

#47 @dd32
14 years ago

Looking into this now.

#48 @nacin
14 years ago

(In [13134]) First pass at allowing username/password selection upon install. Includes some extra cleanup of the patch. Props dancole. See #10396

(In [13135]) Manual L10n JS strings in install.php. Remove stray line

(In [13136]) Actually remove stray line ref. [13135]. see #10396

#49 @nacin
14 years ago

From IRC:

[22:20] <nacin> dancole: We will need to get them to confirm their password
[22:20] <nacin> You want to make an attempt at that?
[22:21] <dd32> nacin: no need for confirmation IMO, It gets displayed on the next page anyway
[22:22] <dd32> and i thought emailed too.. But i dont seem to have an email for it
[22:22] <nacin> Oh, right, it gets displayed on the next page. In which case, why type="password"?
[22:22] <nacin> (If they insert their own password on type="password", then we shouldn't display it on the next page.)
[22:23] <dd32> Good point. Might as well mention that on the ticket then :)
[22:26] <dancole> Yeah, I didn't do two password fields since they get to see their password on the next page, but having the type as password doesn't make sense if they will see it on the next page.

So, a choice. Either we show them their password in clear text on step 1, since we display it in step 2, or if they supply their own password (and confirm it in a second box), we will need to hide the plain-text password in step 2.

I'm in firm support of the latter. It's a user-supplied password and should absolutely be masked.

@dancole
14 years ago

Corrects mistakes in trunk

#50 @dancole
14 years ago

10396_fix_a.diff only shows your user name and password on the next page if you had your password randomly generated. It also adds in the second password field and checks to make sure both are the same. Lastly, it removes a password message that will now never get displayed.

nacin brought up sending the login page your user name in the IRC, but this has some problems. 1) The login page doesn't support sending the user name through $_GET[]. 2) The login page will display an error (empty password field) if you pass the user name through $_POST[]. I like the idea, but didn't include a solution in the above fix.

#51 @dd32
14 years ago

only shows your user name and password on the next page if you had your password randomly generated.

I think it should be displaying the username at a minimum there. And preferably, In $password as <em>..Your chosen password</em>

It also adds in the second password field and checks to make sure both are the same.

I'm not 100% agree'ing on that, however, can see that most people would expect that.

It'll also need to be changed in the new blog notification email.

#52 @dd32
14 years ago

(In [13592]) Do not display user specified password during install. Fixes #12479. See #10396 for feedback

#53 @dd32
14 years ago

dancole: My apologies, I didnt see your patch here before commiting pretty much the exact same thing (with a few extra checks).

This lacks Javascript feedback on mis-matching passwords, Does someone feel like taking a stab at that?

#54 @dancole
14 years ago

Thanks dd32 for working on the patch. My other obligations come in waves, so I was just now checking on this. It was a nice surprise. I've already done a little research on the lack of JavaScript feedback for mis-matched passwords, so I'll make a patch for that.

@dancole
14 years ago

Dev. JavaScript Password Mismatch Checker

#55 @dancole
14 years ago

js_pass_check_a.diff is a development only patch for checking the second password field against the first. I tested out the code and it works perfect. Might want to check over the coding standards though. I didn't change the JavaScript files that are actually being used because I don't know how to minimize them.

#56 follow-up: @dd32
14 years ago

I'm not sure we should modify the password strength script, But at the same time, i'm not sure adding another one is suitable either.

What are other developers thoughts on that?

#57 in reply to: ↑ 56 @dancole
14 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

I moved the JavaScript password strength script to ticket #12576.

#58 @nacin
14 years ago

Forgetting that this was closed, I committed some cleanups: [13696].

#59 @hakre
14 years ago

Related: #14854

Note: See TracTickets for help on using tickets.