Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@resource.allow_password_change is not persisted across requests #481

Closed
Chosko opened this issue Dec 21, 2015 · 13 comments
Closed

@resource.allow_password_change is not persisted across requests #481

Chosko opened this issue Dec 21, 2015 · 13 comments

Comments

@Chosko
Copy link
Contributor

Chosko commented Dec 21, 2015

My application is set to require current_password when users want to change their password.
The problem is when an user wants to reset the password. After the mail is sent and the user is prompted to insert the new password, the server returns 422 with the error: "current_password can't be blank".

I'm using v0.1.37.beta4, and this version contains a pull request: d33bcdb
which is supposed to fix this problem, but it doesn't.

I guess the problem here is that @resource.allow_password change is set to true in projects_controller#edit but it's not persisted to the DB, and in the next call (projcets_controller#update) its value is false again.

I made a quick and dirty fix to store allow_password to the DB, though I'm not sure it's a good solution because this means changing the generators too....
I don't have the time to implement tests, so I didn't even open the PR. However, you can see the changes here Chosko@e3a4d3d.

@biomancer
Copy link
Contributor

I suppose we can fix that without flag persistence by passing reset_password_token to redirect url and checking it again in update action. I am using currently a quick fix which is below, it can be done cleaner and some checks are required, but I can't find now a minute to prepare proper fix and PR, so I hope it may help someone:

While using custom passwords controller

class PasswordsController < DeviseTokenAuth::PasswordsController

 def edit
   super do # TODO there is minor bug too - block does not pass resource        
     # TODO build url correctly when it already has some params (I did not need it in my case)
     params[:redirect_url] += "?reset_password_token=#{@resource.reset_password_token}"
   end
 end

 protected 

 def resource_update_method
    @resource.allow_password_change = @resource.reset_password_token.present? && resource_params[:reset_password_token] == @resource.reset_password_token
    super
 end

end

That will change interface for PUT /auth/passwords, requiring to pass reset_password_token when there is no current password specified.

@paulius005
Copy link

I added a simple allow flag that I am passing in when i call update, also not the best solution, but it works for now

@paulius005
Copy link

This seems to be the same as #526 I should probably close mine

@biomancer
Copy link
Contributor

@paulius005 using some flag param is less secure, as one may update password without knowing old password or reset token, which should not happen as I suppose. That does not look like serious issue as user still must be authorized at this point, but reusing reset token instead of flag solves it anyway, so why not use it.

@paulius005
Copy link

Yep! Agreed

bobmshannon pushed a commit to StratusPrint/devise_token_auth that referenced this issue Apr 6, 2016
@bobmshannon
Copy link

Having this same issue, but none of the fixes mentioned here worked for me. Using the master branch of device token auth and rails 5 beta 3.

@stex
Copy link

stex commented Jun 1, 2016

I extended @biomancer's solution a bit to support URLs with already existing parameters:

Update Due to the resource save! in PasswordsController#edit, the raw reset token is persisted instead of the encrypted. I updated the workaround to restore the encrypted one.

class Api::PasswordsController < DeviseTokenAuth::PasswordsController
  def edit
    super do
      redirect_url          = URI.parse(params[:redirect_url])
      reset_params          = {reset_password_token: @resource.reset_password_token}
      redirect_url.query    = Rack::Utils.parse_query(redirect_url.query).merge(reset_params).to_query
      params[:redirect_url] = redirect_url.to_s

      # Devise's reset_password_by_token sets the @resource's +reset_password_token+ attribute
      # to its raw value after performing a database lookup. Due to devise_token_auth's +save!+
      # to persist the newly generate auth token, the raw value is written to the database.
      # This causes a recovery link to be only valid once, even if no new password was specified
      # as well as an inconsistency to Device's use of `token_generator.digest`
      @resource.reset_password_token = Devise.token_generator.digest(@resource, :reset_password_token,
                                                                     @resource.reset_password_token)
      @resource.save(validate: false)
    end
  end

  protected

  def resource_update_method
    # Only allow setting a new password without specifying the current one if the user
    # is currently in the middle of a password recovery process and the client passed
    # in his current +password_reset_token+
    @resource.allow_password_change = valid_reset_password_token?
    super
  end

  #
  # Checks whether a password reset token is given and if it matches the current user
  #
  def valid_reset_password_token?
    return false if @resource.reset_password_token.blank? || @resource.reset_password_token.blank?
    digest = Devise.token_generator.digest(@resource, :reset_password_token, resource_params[:reset_password_token])
    digest == @resource.reset_password_token
  end
end

@volfgox
Copy link

volfgox commented Sep 4, 2016

I tested the above solutions suggested by @stex and @biomancer. The problem still persisted for me. So, I decided to add an additional persistent allow password change field to user model. My solution is:

class AddPersistentAllowPasswordChangeToUsers < ActiveRecord::Migration
  def change
    add_column :users, :persistent_allow_password_change, :boolean, null: false, default: false, after: :reset_password_sent_at 
  end
end

Then, I override passwords controller of devise_token_auth as bellow:

module Api
  module V1
    module Overrides
      class PasswordsController < DeviseTokenAuth::PasswordsController
        def edit
          super do |resource|
            resource.persistent_allow_password_change = true
            resource.save!
          end
        end

        def update
          super do |resource|
            resource.persistent_allow_password_change = false
            resource.save!
          end
        end

         protected 

           def resource_update_method
             @resource.allow_password_change = @resource.persistent_allow_password_change
             super
           end
      end
    end
  end
end

And, I changed devise_token_auth routes in routes.rb as below:

mount_devise_token_auth_for 'User', at: 'auth', controllers: {
  sessions: 'api/v1/overrides/sessions',
  omniauth_callbacks: 'api/v1/overrides/omniauth_callbacks',
  registrations: 'api/v1/overrides/registrations',
  passwords: 'api/v1/overrides/passwords'
}

With this solution, every time user redirected from reset password email to application, when edit action of PasswordsController is executed, persistent_allow_password_change is set to true. When update password request of client receives the server, the value of persistent_allow_password_change is assigned to allow_password_change in resource_update_method, before super. After password changing in update action, the value of persistent_allow_password_change is reset to false.

@zachfeldman
Copy link
Contributor

Sounds like we have an accepted solution here, so closing this for now!

@sickrandir
Copy link

Is it fixed upstream in some released version?

@zachfeldman
Copy link
Contributor

@sickrandir it might be, we are testing this release right now:
https://github.com/lynndylanhurley/devise_token_auth/tree/v0.1.43.beta1

which is the latest. If not feel free to submit a pull request!

@sickrandir
Copy link

@zachfeldman thanx. I installed 0.1.43.beta1 and I can confirm that it fixes this issue for me.

@zachfeldman
Copy link
Contributor

That's great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants