#10396 closed task (blessed) (fixed)
Pick an username during the install
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (69)
#2
@
16 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
@
16 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
@
16 years ago
Theres also the issue of shared-user tables that can cause small issues like that..
#5
@
16 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
@
16 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...
#7
@
16 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
@
16 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
@
16 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:
↓ 11
@
16 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
@
16 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
@
16 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:
↓ 14
@
16 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
@
16 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
@
16 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:
↓ 17
@
16 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
@
16 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
@
16 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..
#19
@
15 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
@
15 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
@
15 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
@
15 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.
#25
@
15 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
@
15 years ago
- Type changed from enhancement to feature request
Changing type to "feature request" since that's what it is, really.
#28
@
15 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?
#30
@
15 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
@
15 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
@
15 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.
#34
@
15 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
@
15 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. :)
#36
@
15 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
@
15 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.
#39
@
15 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.
#40
@
15 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
#42
@
15 years ago
- Keywords tested removed
oops, I didn't see dd32's commets. I'll make those changed.
#44
@
15 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
@
15 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
@
15 years ago
I just realized I nuked this patch in [13124]. Will try to refresh it later myself if I have a moment.
#49
@
15 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.
#50
@
15 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
@
15 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.
#53
@
15 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
@
15 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.
#55
@
15 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:
↓ 57
@
15 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?
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?