Allow user send new password with reset pasword token without auth headers

[MaicolBen]

This allows a new flow (thanks to @lynndylanhurley):

1. user fills out password reset request form
2. user is sent email
3. user clicks confirmation link
4. link leads to the client (instead of the API) with a reset_password_token
5. user submits password along with reset_password_token
6. user is now authorized and has a new password

I will update the section with the form submitting in the wiki:

• the user submits the form on this frontend page, which sends a request to API: PUT /auth/password with the password and password_confirmation parameters. There are 2 ways to identify the user who is resetting the password:
  1. The auth headers need to be included from the url params. A side note, ensure that the header names follow the convention outlined in config/initializers/devise_token_auth.rb; at this time of writing it is: uid, client and access-token.
     -Ensure that the uid sent in the headers is not URL-escaped. e.g. it should be bob@example.com, not bob%40example.com-
  2. Include the reset password token, this is particularly useful for the forgot password from a mobile app.

[lynndylanhurley]

Thanks @MaicolBen!! I'll review this today.

[lynndylanhurley]

@MaicolBen - just to clarify, how does the client get the reset_password_token in the first place?

My understanding was that with this approach, the password reset link contained in the confirmation email would lead to the client, and the reset_password_token would be passed to the client using a querystring param.

But the edit_password_url method in the password reset email template uses this devise url helper, and I believe that it leads to the API first, not the client.

Do we need to do something with the edit_password_url helper so that we can specify where the confirmation link should lead to?

[MaicolBen]

@lynndylanhurley You're right, I missed that, there are 2 ways:

1. Override the edit action and redirect with the token instead of responding with the headers
2. Override reset_password_instructions where you replace the edit reset password url with the one in the frontend (redirect url), you can skip the edit action here.

The first one is the harmless in the gem, it's just adding a new parameter to the url. What do you think?

[lynndylanhurley]

Cool I think I follow. Just to clarify with option 1:

• This keeps the old behavior where the password-reset email link leads to the API first, then redirects to the client.
• The difference is that, instead of passing auth headers to the client, it passes the reset_password_token instead.

Is that correct?

If so, then we will need a way for the password edit controller action to know when to pass headers back to the client (old behavior) and when to pass the reset_password_token (new behavior). I can think of a couple of ways to do this:

1. Using an initializer setting. For example:

   DeviseTokenAuth.setup do |config|
     # ...
   
     # By default, the password-reset confirmation link redirects to the client
     # with valid session credentials as querypstring params. With this option
     # enabled, the redirect will NOT include the valid session credentials.
     # Instead the redirect will include a password_reset_token querystring param
     # that can be used to reset the users password. Once the user has reset their
     # password, the password-reset success response headers will contain valid
     # session credentials.
     config.require_client_password_reset_token = true
   end

2. Using a querystring param in the password-reset confirmation email template:

   <!-- app/views/devise/mailer/reset_password_instructions.html.erb -->
   <p><%= t(:hello).capitalize %> <%= @resource.email %>!</p>
   
   <p><%= t '.request_reset_link_msg' %></p>
   
   <p><%= link_to t('.password_change_link'), 
     edit_password_url(@resource, 
       reset_password_token: @token, 
       config: message['client-config'].to_s, 
       redirect_url: message['redirect-url'].to_s, 
       require_client_password_reset_token: true # <-- set flag in confirm link
   ).html_safe %></p>
   
   <p><%= t '.ignore_mail_msg' %></p>
   <p><%= t '.no_changes_msg' %></p>

Then at the top of the password edit action:

# app/controllers/devise_token_auth/passwords_controller.rb
module DeviseTokenAuth
  class PasswordsController < DeviseTokenAuth::ApplicationController
    # ...

    def edit
      # check if reset_password_token is required from client
      if (
        # if we're using initializers
        DeviseTokenAuth.require_client_password_reset_token ||
        # or if we're using the email querystring param
        resource_params[:require_client_password_reset_token]
      )
        callback = URI.parse(params[:redirect_url])

        # add password_reset_token while preserving existing query params
        callback.query_values = (callback.query_values || {}).merge(
          reset_password_token: resource_params[:reset_password_token]
        )

        # redirect to client
        return redirect_to callback.to_s
      end

      # ... otherwise use old behavior
    end

    # ...
  end
end

What do you think @MaicolBen?

[MaicolBen]

Sounds good! I think that we can keep both ways without harming each other, but if you prefer it be configurable, I can make it. Also, this is an alternative behavior, not a new one, I don't want this replace the original behavior that most of the people uses, I want an alternative way that is helpful for mobile apps with a reset password through deep/universal linking.

[fameoflight]

@MaicolBen @lynndylanhurley hey guys just wondering what is the status here? Happy to work on my own fork but just wondering if this is still being worked on?

[MaicolBen]

@fameoflight Sorry, I've been busy, I expect to finish this by this week

[fameoflight]

For now, I found a good hack but would love to remove the hack in future.

For anyone else in the same boat, here is my solution. Override passwords controller

in api/passwords_controller.rb

module Api
  class PasswordsController < DeviseTokenAuth::PasswordsController
    before_action :set_user_by_reset_token, only: [:update]

    def set_user_by_reset_token
      return if resource_params[:reset_password_token].nil?

      @resource = with_reset_password_token(resource_params[:reset_password_token])

      if @resource&.reset_password_period_valid?
        sign_in(:user, @resource, store: false, event: :fetch, bypass: DeviseTokenAuth.bypass_sign_in)
      else
        render_error(401, 'Reset token has expired. Please request a new token and try again.')
      end
    end
  end
end

In routes.rb

mount_devise_token_auth_for 'User', at: 'auth', controllers: {
      passwords: 'api/passwords',
    }

[ruimiguelsantos]

@MaicolBen Thanks for this!

Just a suggestion: maybe you could wrap the helper_method call in respond_to?(:helper_method). helper_method is defined in AbstractController::Helpers, which in turn is included in ActionController::Base, but it is not included in ActionController::Metal. Check heartcombo/devise/pull/3732 for details.

[MaicolBen]

No need, I removed it, I only needed the method in the controller

[ruimiguelsantos]

@MaicolBen I think the current approach might fail when require_client_password_reset_token is true and check_current_password_before_update is set to :password.

This is because allow_password_change isn't set to true in the first case.

Perhaps changing that line will fix it. For the folks skipping the edit action, resource_update_method could be changed instead, e.g

def resource_update_method
  allow_password_change = recoverable_enabled? && 
                        ( @resource.allow_password_change == true ||
                          DeviseTokenAuth.require_client_password_reset_token )
  if DeviseTokenAuth.check_current_password_before_update == false || allow_password_change
    "update_attributes"
  else
    "update_with_password"
  end
end

so that 'update_attributes' would be used when running #update.

[MaicolBen]

@ruimiguelsantos Thanks for noticing that, wouldn't it better change the edit action:

def edit
      # if a user is not found, return nil
      @resource = with_reset_password_token(resource_params[:reset_password_token])

      if @resource && @resource.reset_password_period_valid?
        if DeviseTokenAuth.require_client_password_reset_token
          update_allow_password_change
          callback = URI.parse(params[:redirect_url])

          # add password_reset_token while preserving existing query params
          callback.query_values = (callback.query_values || {}).merge(
            reset_password_token: resource_params[:reset_password_token]
          )
          redirect_to callback.to_s
        else
          client_id, token = @resource.create_token

          update_allow_password_change
          
          redirect_header_options = {reset_password: true}
          redirect_headers = build_redirect_headers(token,
                                                    client_id,
                                                    redirect_header_options)
          redirect_to(@resource.build_auth_url(params[:redirect_url],
                                               redirect_headers))

        end
      else
        render_edit_error
      end
 end

def update_allow_password_change
      # ensure that user is confirmed
      @resource.skip_confirmation! if confirmable_enabled? && !@resource.confirmed_at

      # allow user to change password once without current_password
      @resource.allow_password_change = true if recoverable_enabled?

      @resource.save!

      yield @resource if block_given?
 end

[ruimiguelsantos]

@MaicolBen You're right, that's indeed better. Looks good to me!

In that case I would just take the yield call out of update_allow_password_change, otherwise this would stop working for people who try to add behaviour to #edit by passing a block. Alternatively you could reference the block argument explicitly and pass it on (or implicitly with &Proc.new and a couple of nil checks):

def edit &block
      # if a user is not found, return nil
      @resource = with_reset_password_token(resource_params[:reset_password_token])

      if @resource && @resource.reset_password_period_valid?
        if DeviseTokenAuth.require_client_password_reset_token
          update_allow_password_change &block
          callback = URI.parse(params[:redirect_url])

          # add password_reset_token while preserving existing query params
          callback.query_values = (callback.query_values || {}).merge(
            reset_password_token: resource_params[:reset_password_token]
          )
          redirect_to callback.to_s
        else
          client_id, token = @resource.create_token

          update_allow_password_change &block
          
          redirect_header_options = {reset_password: true}
          redirect_headers = build_redirect_headers(token,
                                                    client_id,
                                                    redirect_header_options)
          redirect_to(@resource.build_auth_url(params[:redirect_url],
                                               redirect_headers))

        end
      else
        render_edit_error
      end
 end

def update_allow_password_change
      # ensure that user is confirmed
      @resource.skip_confirmation! if confirmable_enabled? && !@resource.confirmed_at

      # allow user to change password once without current_password
      @resource.allow_password_change = true if recoverable_enabled?

      @resource.save!

      yield @resource if block_given?
 end

[MaicolBen]

@lynndylanhurley @ruimiguelsantos Done!

I won't fix the code climate issue now, because this is an engine, and adding more concerns could confuse people when they have to override, I will add a Codeclimate config when #1125 is merged. Maybe in the future we can come up with better refactors.

Also, I added doc for this flow, since I was writing the doc and it was huge as a FAQ, I moved it to its own page.

[sofianegargouri]

Will this be merged ?

[MaicolBen]

@sofianegargouri we don't have enough contributors in this repo to review, @dks17 can review it but this will go to 1.1.0 or 1.2.0

[jkeen]

What else needs to be done here @MaicolBen @lynndylanhurley? I don't want this PR to die!

I found one bug that I fixed locally—if the token isn't valid and the user isn't found, the code tries to call #create_token on nil and throws a 500 instead of a 401.

[jkeen]

Made that update, fixed some tests, and rebased against master bringing this branch up to date with the rest of the project. Feel free to pull in those changes to this branch @MaicolBen, or if you give me access to this branch I can do it.