jywarren / plots2

The Public Lab website!
http://publiclab.org
GNU General Public License v3.0
17 stars 2 forks source link

Cannot save user password to database on local system #166

Closed btbonval closed 11 years ago

btbonval commented 11 years ago

Steps to reproduce on my system:

  1. rails console
  2. user = User.find_by_username('btbonvaltest')
  3. user.password = 'some really long password'
  4. user.save({}) or user.save() or user.save or user.save!

If using .save!, a seemingly legit error message is returned about the password length needing to be greater than 4 characters long. Of course, the password is longer than 4 characters, so the error message is actually not legit.

If using .save or any variant sans bang, a NoMethodError is returned for TrueClass. Such an error shows up all over the internet and seems to be entirely unhelpful towards finding what the real problem is.

btbonval commented 11 years ago

This does work:

  1. rails console
  2. user = User.find_by_username('btbonvaltest')
  3. user.reset_key = 'sweetresetmagic'
  4. user.save!

No errors, evvyting be irriee.

jywarren commented 11 years ago

huh, i don't seem to experience this... I had been mystified by the need for save({}), but glumly accepted the ideosyncracy. Your issue sounds worse... can you post the error message about password length as a gist?

On Wed, Jul 31, 2013 at 1:59 AM, Bryan Bonvallet notifications@github.comwrote:

Steps to reproduce on my system:

  1. rails console
  2. user = User.find_by_username('btbonvaltest')
  3. user.password = 'some really long password'
  4. user.save({}) or user.save() or user.save or user.save!

If using .save!, a seemingly legit error message is returned about the password length needing to be greater than 4 characters long.

If using .save or any variant sans bang, a NoMethodError is returned for TrueClass. Such an error shows up all over the internet and seems to be entirely unhelpful towards finding what the real problem is.

— Reply to this email directly or view it on GitHubhttps://github.com/jywarren/plots2/issues/166 .

jywarren commented 11 years ago

Now, I think this may be a vulnerability, actually -- mass assignment is not great -- but take a look at https://github.com/jywarren/plots2/blob/master/app/controllers/users_controller.rb#L40 and see if you can update pwd locally by creating an object like user = {:password => 'lalala'} the way it's done on line 40...

btbonval commented 11 years ago

I agree about it being a vulnerability. It's also a bit black magic, which I don't like. So I was experimenting with what it does exactly and how it could be done differently.

params[:user] is just something like {:username => 'awesomeguy42', :password => 'magicloveharmony'}. It ends up breaking the key/value pairs and assigning them. I guess you wouldn't be assigning a username at this juncture in code, but the example isn't meant to be literal.

The right way (I think) to do it is:

@user = current_user
  @user.password = params[:user][:password]
  @user.otherthing = params[:user][:otherthing]
  @user.etc = params[:user][:etc]
  @user.drupal_user.set_bio(params[:drupal_user][:bio])
  @user.save({}) do |result|
 ....

It's a bit more tedius, but a lot more explicit. Being explicit is better for vulnerability protection, as you suggest.

Working on posting a gist, never done one of those before.

btbonval commented 11 years ago

I didn't notice this before, but I ran another rake db:migrate just to be sure.

bryan@zetool-mini:~/Documents/programmin/git/plots2$ rake db:migrate
rake aborted!
Mysql2::Error: SELECT command denied to user ''@'localhost' for column 'nid' in table 'node_selections': SHOW FIELDS FROM `view_node_like_count`

It looks like my migration isn't proper. Invalid database might explain problems saving to it! I'm not entirely sure how my migrations got screwed up or how to fix it. The error isn't very enlightening. I'm guessing column nid doesn't exist in node_selections; there shouldn't be permissions problems.

btbonval commented 11 years ago

Very weird. I can run EXPLAIN and SHOW on just about every table I test. All of the view_* tables, but:

mysql> EXPLAIN view_node_like_count;
ERROR 1143 (42000): SELECT command denied to user ''@'localhost' for column 'nid' in table 'node_selections'

I'm the superuser on the database, too, so ... that's weird.

mysql> SHOW GRANTS;
+----------------------------------------------------------------------------------------------------------------------------------------+
| Grants for root@localhost                                                                                                              |
+----------------------------------------------------------------------------------------------------------------------------------------+
| GRANT ALL PRIVILEGES ON *.* TO 'root'@'localhost' IDENTIFIED BY PASSWORD '*2470C0C06DEE42FD1618BB99005ADCA2EC9D1E19' WITH GRANT OPTION |
| GRANT PROXY ON ''@'' TO 'root'@'localhost' WITH GRANT OPTION                                                                           |
+----------------------------------------------------------------------------------------------------------------------------------------+
2 rows in set (0.00 sec)

Thus ''@'localhost' is proxy for 'root'@'localhost' which gets evvytang on evvytang, although I'm using root@localhost for the command. Not sure why it's saying ''@'localhost'.

btbonval commented 11 years ago

https://gist.github.com/btbonval/6129191

I show that I am able to save the User model when I change secret key (the first attempt failed but the second worked), but I cannot save the User model at all when I change password. You can check encrypted_password and password_salt to see when it changes in the memory. save and save() give one error, save({}) gives another related error, and save! gives a completely new error. save! worked fine to save the secret key to the database.

EDIT: oh sorry, I meant to address rake. I dropped that view entirely then ran rake db:migrate again; it completed successfully without errors. Database is up to spec.

btbonval commented 11 years ago

Not sure if edits get sent as emails:

I meant to address rake. I dropped that view entirely then ran rake db:migrate again; it completed successfully without errors. Database was up to rake's specifications before the gist was generated.

btbonval commented 11 years ago

Dumped the current database from plots2 server and restored it locally.

rake db:migrate gave the same error re: view_node_like_count. Dropped that view and rake db:migrate worked fine.

Password change, as per the process at the beginning of this ticket, worked just fine. No errors (using .save! in particular, but I suspect it should work for all the fun ways to call save).