lynndylanhurley / devise_token_auth

Token based authentication for Rails JSON APIs. Designed to work with jToker and ng-token-auth.
Do What The F*ck You Want To Public License
3.54k stars 1.13k forks source link

Password change broken after password reset initiated #1397

Open iMacTia opened 4 years ago

iMacTia commented 4 years ago

When posting issues, please include the following information to speed up the troubleshooting process:

activemodel (6.0.2.2) lib/active_model/attribute_assignment.rb:53:in _assign_attribute' activemodel (6.0.2.2) lib/active_model/attribute_assignment.rb:44:inblock in _assign_attributes' activemodel (6.0.2.2) lib/active_model/attribute_assignment.rb:43:in each' activemodel (6.0.2.2) lib/active_model/attribute_assignment.rb:43:in_assign_attributes' activerecord (6.0.2.2) lib/active_record/attribute_assignment.rb:22:in _assign_attributes' activemodel (6.0.2.2) lib/active_model/attribute_assignment.rb:35:inassign_attributes' activerecord (6.0.2.2) lib/active_record/persistence.rb:620:in block in update' activerecord (6.0.2.2) lib/active_record/transactions.rb:375:inblock in with_transaction_returning_status' activerecord (6.0.2.2) lib/active_record/connection_adapters/abstract/database_statements.rb:281:in block in transaction' activerecord (6.0.2.2) lib/active_record/connection_adapters/abstract/transaction.rb:280:inblock in within_new_transaction' activerecord (6.0.2.2) lib/active_record/connection_adapters/abstract/transaction.rb:278:in synchronize' activerecord (6.0.2.2) lib/active_record/connection_adapters/abstract/transaction.rb:278:inwithin_new_transaction' activerecord (6.0.2.2) lib/active_record/connection_adapters/abstract/database_statements.rb:281:in transaction' activerecord (6.0.2.2) lib/active_record/transactions.rb:212:intransaction' activerecord (6.0.2.2) lib/active_record/transactions.rb:366:in with_transaction_returning_status' activerecord (6.0.2.2) lib/active_record/persistence.rb:619:inupdate' devise_token_auth (275da3c1960b) app/controllers/devise_token_auth/passwords_controller.rb:89:in `update'

* **Environmental Info**:
  * **Routes**: Just a plain, simple API versioning

Rails.application.routes.draw do namespace :api, defaults: { format: 'json' } do scope module: :v1, constraints: ApiConstraints.new(version: 1, default: true) do mount_devise_token_auth_for 'User', at: 'auth' end end end


  * **Gems**: Standard Rails API (`--api-only`) app
  * **Custom Overrides**: None
  * **Custom Frontend**: No front end yet!

## Steps to reproduce

1. Initiate a password reset (`POST /password`) and click on the email link (`GET /password/edit`)
2. Do not complete the password reset (e.g. you suddenly remember your password!)
3. Try to update your password calling `PUT /password` `DeviseTokenAuth.check_current_password_before_update` set to `true`.

This will cause the exception `ActiveModel::UnknownAttributeError (unknown attribute 'current_password' for User.)`.

### Explanation and initial investigation

When clicking on the email link to reset your password, the `GET /password/edit` is called, causing `@resource.allow_password_change` being set to `true` (https://github.com/lynndylanhurley/devise_token_auth/blob/3bf9b9be40661f9024083962ee9660cc45ac5dbc/app/controllers/devise_token_auth/passwords_controller.rb#L45).
This is necessary to allow the password reset without knowing the current password.

However, assuming you then remember your password and try to change it with `DeviseTokenAuth.check_current_password_before_update` setting set to `true`, you'll theoretically need to send your `current_password` as well. This is however overridden by the `allow_password_change` flag (https://github.com/lynndylanhurley/devise_token_auth/blob/3bf9b9be40661f9024083962ee9660cc45ac5dbc/app/controllers/devise_token_auth/passwords_controller.rb#L103), so `@resource.update` is called instead of `@resource.update_with_password`.

This results in the error mentioned above.
The error can be avoided by NOT sending the `current_password`, this works because the `allow_password_change` flag overrides the `DeviseTokenAuth.check_current_password_before_update` setting, but this is not expected behaviour.

### Expected behaviour

IMHO, `PUT /password` should ALWAYS require the `current_password` if `DeviseTokenAuth.check_current_password_before_update` is set to `true`, regardless of the `@resource. allow_password_change ` flag.
I believe this can easily be reproduced with a test and fixed in the `PasswordsController`, but before opening a PR I wanted to double-check with you if you agree with this behaviour or if you believe the current one is better (no test at the moment test this scenario, so I believe it's an unexpected behaviour).
It's worth saying that a FE application page allowing to change the password will unlikely check the `@resource. allow_password_change ` flag, so this could cause the situation where users are unable to change their password after initiating the password reset flow.

This is very probably related to #1068 
rodrigovcortezi commented 4 years ago

@iMacTia I was having the same problem, but in a slightly different way. I was using the passwords#update entry to change user password using the current_password attribute too, but I guess that is not correct. It broken after I set the require_client_password_reset_token config to reset passwords. So after reading this issue I started to investigate the problem. And what I concluded is that we are not suppose to use passwords#update action to change users password, but registrations#update. It worked for me! Give it a try!

iMacTia commented 4 years ago

@rodrigovcortezi I can see what you're referring to. Looking again at the code it seems, as you said, that the passwords#update action should only be used for password reset, hence for cases where we don't have a valid access token, although for some reason there's an else loading the user based on the actual acces-token, which doesn't really make sense under that assumption (https://github.com/lynndylanhurley/devise_token_auth/blob/387306a180e955da077a67a6b77b1feda9497202/app/controllers/devise_token_auth/passwords_controller.rb#L73).

I guess there's a bit of confusion around the correct password reset flow, but as you correctly pointed out I can successfully change the password using the registrations#update endpoint instead.

Still, it would be good to hear from a maintainer about why is passwords#update allowing to set the user by token, if that should not be possible during a password reset?

ziaulrehman40 commented 5 months ago

Exactly the same issue as of @rodrigovcortezi , and the solution provided still works. Thanks