twilio / authy-devise

Authy Devise plugin to add Two-Factor Authentication
MIT License
201 stars 84 forks source link

ActiveRecord::RecordNotFound at /users/[object%20Object] Couldn't find User with 'id'=[object Object] after onetouch authentication #135

Closed wxxiao33 closed 4 years ago

wxxiao33 commented 4 years ago

When login, after one touch authentication, my app will redirect to http://localhost:3000/users/[object%20Object] and get an error, but if I authenticated by entering the token (authy or SMS), it will redirect to the right path: http://localhost:3000/users/3 I followed the basic setup steps, could you please help me figure it out?

my routes.rb looks like this:

  devise_for :users, controllers: { 
        omniauth_callbacks: 'users/omniauth',
        verify_authy: "/verify-token",
        enable_authy: "/enable-two-factor",
        verify_authy_installation: "/verify-installation",
        authy_onetouch_status: "/onetouch-status"
                                  }

  resources :users
philnash commented 4 years ago

That redirect is controlled by this code: https://github.com/twilio/authy-devise/blob/master/app/views/devise/verify_authy.html.erb#L31

Have you changed anything in that JS on your app? Or what is returned from the controller action?

wxxiao33 commented 4 years ago

Thank you for replying. I am pretty new to ruby on rails. I don't think I changed my JS but I'm not completely sure.

I also tried to custom redirect paths, I created ./app/controllers/users/devise_authy_controller.rb

class User::DeviseAuthyController < Devise::DeviseAuthyController

  protected
    def after_authy_enabled_path_for(resource)
      root_path
    end

    def after_authy_verified_path_for(resource)
      root_path
    end

    def after_authy_disabled_path_for(resource)
      root_path
    end

    def invalid_resource_path
      new_user_session_path
    end
end

and change my routes.rb like this

  devise_for :users, controllers: { 
        omniauth_callbacks: 'users/omniauth',
        devise_authy: 'users/devise_authy',
        verify_authy: "/verify-token",
        enable_authy: "/enable-two-factor",
        verify_authy_installation: "/verify-installation",
        authy_onetouch_status: "/onetouch-status"
                                  }

  resources :users

NameError at /users/verify_authy uninitialized constant Users::DeviseAuthyController

philnash commented 4 years ago

Hmm, your NameError at the bottom of your comment looks like it's because you have defined a User::DeviseAuthyController but if your Devise is set up to route to devise_for :users then you should probably use a plural "users" too.

So, make sure your new controller is actually a Users::DeviseAuthyController (note the "s" in "Users").

Have you modified after_sign_in_path_for anywhere in your Devise setup?

wxxiao33 commented 4 years ago

Yes, you are right, I should use Users. But after change it to Users, it still have Couldn't find User with 'id'=[object Object] error.

I haven't modified after_sign_in_path_for anywhere. It is really confusing because token authentication works just fine.

I did modified this in ./app/controller/application_controller.rb:

    def after_sign_in_path_for(resource)
      current_user
    end
    def after_sign_up_path_for(resource)
      current_user
    end

I changed current_user to user_path(current_user) and now I can log in successfully . But I still don't understand why this costumed 'current_user' works in other places but not one touch authetification.

Thank you so much for your prompt response. I am sorry about for wasting your time. Just like I said, I am pretty new to ruby on rails, and it's so easy to lose track of what you have done.

philnash commented 4 years ago

So yeah, the after_sign_in_path_for and after_sign_up_path_for methods need to return a path, not just any object. In this case, you changed it from returning a path to the current user object.

When you sign in with One Touch the JS keeps checking the back-end to see if the sign in is complete. When it is complete, it sends a response with the result of after_sign_in_path_for in it. The JS tries to redirect to that path, but because you changed it to return current_user the JS serialized it into a string [object Object] which is where you see the error.

I think the reason it was working for the other methods was that the controller actions were redirecting with something like:

respond_with resource, :location => after_sign_in_path_for(@resource)

You returned current_user for that and I think that some Rails magic means that :location => user would turn into users_path(user).

It is safest to return a path from those after_sign_in/up_path_for methods and I'm glad you have done now. I'll close this issue, as it's not a problem with devise-authy, but feel free to keep asking questions.

wxxiao33 commented 4 years ago

I guess I am still learning the magic of rails. Thank you for your help.

philnash commented 4 years ago

Yeah, this is one of those points where "Rails magic" doesn't help! Sorry you got caught up with this. Hopefully you're all sorted with it now.

wxxiao33 commented 4 years ago

I am not sure if I need to open a new issue but I get ArgumentError at /users/verify-token First argument in form cannot contain nil or be empty when I trying to use modals popup for log in and sign up.

This error occurs when I tried to log in, after enter email and password in the log in modal, the verify-token page will give this error. I can still get onetouch push in my authy app and log in.

I create partial 'app/views/devise/sessions/_new.html.erb' with only the log in form:

<%= simple_form_for(resource, as: resource_name, url: session_path(resource_name), html: { class: "card-body" }) do |f| %>

  <div class="form-group">
    <%= f.input :email,
                required: false,
                autofocus: true,
                class: "form-label",
                input_html: { autocomplete: "email" } %>
  </div>
  <div class="form-group">
    <%= f.input :password,
                required: false,
                class: "form-label",
                input_html: { autocomplete: "current-password" } %>
  </div>

  <% if devise_mapping.rememberable? -%>
    <div class="form-group">
      <%= f.label :remember_me, class: "form-checkbox" do %>
        <%= f.check_box :remember_me %>
        <i class="form-icon"></i> Remember me
      <% end %>
    </div>
  <% end -%>

  <div class="actions">
    <%= f.button :submit, "Log in", class: "btn btn-primary" %>
  </div>

  <a href="#SignUpModal" data-toggle="modal" data-dismiss="modal">Sign Up</a>
  <%#= link_to "Sign up", new_registration_path(resource_name) %><br />
  <%= link_to "Forgot your password?", new_password_path(resource_name) %><br />

<% end %>

and similarity 'app/views/devise/registrations/_new.html.erb' for sign up:

<%= simple_form_for(resource, as: resource_name, url: registration_path(resource_name), html: { class: "card-body" }) do |f| %>
        <%= f.error_notification %>

        <div class="form-group">
            <%= f.input :name,
                        class: "form-label"%>
        </div>

        <div class="form-group">
            <%= f.input :email,
                        autofocus: true,
                        class: "form-label",
                        input_html: { autocomplete: "email" }%>
        </div>

        <div class="form-group">
            <%= f.input :password,
                        class: "form-label",
                        hint: ("#{@minimum_password_length} characters minimum" if @minimum_password_length),
                        input_html: { autocomplete: "new-password" } %>
        </div>

        <div class="form-group">
            <%= f.input :password_confirmation,
                        class: "form-label",
                        input_html: { autocomplete: "new-password" } %>
        </div>

        <div class="actions">
            <%= f.button :submit, "Sign up", class: "btn btn-primary" %>
        </div>

<% end %>

Then render them in the modals in my header partial 'app/views/layouts/_header.html.erb'.

I did some digging and I guess the reason is that resource helper is not available for the modals. So I add some helper function in my app/helpers/registrations_helper.erb

  def resource_name
    :user
  end

  def resource
    @resource ||= User.new
  end

  def resource_class 
      User 
  end

  def devise_mapping
    @devise_mapping ||= Devise.mappings[:user]
  end

For now registrations seems to be OK, but I still get the same error with app/views/devise/sessions/_new.html.erb. I tried to create a similar sessions_helper.rb, that will error out uninitialized constant SessionsHelper::Session

  def resource_name
    :session
  end

  def resource
    @resource ||= Session.new
  end

  def resource_class 
      Session
  end

  def devise_mapping
    @devise_mapping ||= Devise.mappings[:session]
  end

If I just comment out :authy_authenticatable in the users.rb model, the log in process works just fine without 2FA.

So I think the problems it how to write a resource for modal popup during log in process, (the log in modal in header partial, after type in email and password, but before 2FA)

How could I get it to work with modal popups?

philnash commented 4 years ago

In the case of devise and this plugin, the resource is the user and never the session since you are logging a user into a session.

Try matching your sessions_helper to your registrations_helper and see if that helps.

wxxiao33 commented 4 years ago

Sorry for the delayed reply, I think I break something when troubleshooting earlier because I tried to put these helper functions in application_helper.rb, but I still get the same error. I actually change resource to :user in these forms. Probably not the best way, but it works for now.

philnash commented 4 years ago

I think that’s what I meant to say. It shouldn’t ever be session. If this is for a user model, then user works. resource is the generic version for if you didn’t call your user model “user”.