mikker / passwordless

πŸ— Authentication for your Rails app without the icky-ness of passwords
MIT License
1.25k stars 86 forks source link

NoMethodError: undefined method `session' for nil #207

Closed yshmarov closed 4 months ago

yshmarov commented 5 months ago

When running passwordless_sign_in(@user), I get the error NoMethodError: undefined methodsession' for nil. It breaks [here](https://github.com/mikker/passwordless//blob/3b12614dd0ca377e7dc8c57ba9221a56538e5816/lib/passwordless/test_helpers.rb#L14-L15). It looks like@requestisnil`

rails 7.1.3
ruby 3.3.0
passwordless 1.4

curiously, I am having errors in different versions of passwordless

Screenshot 2024-02-10 at 17 41 36
mikker commented 5 months ago

These are your system tests? (I see /integration/ in the path) Can you make sure the right test helper is being included? We want it to be these https://github.com/mikker/passwordless/blob/master/lib/passwordless/test_helpers.rb#L46-L65 however it looks like your tests are calling the controller ones (as they're the only ones referencing @request, https://github.com/mikker/passwordless/blob/master/lib/passwordless/test_helpers.rb#L15)

yshmarov commented 5 months ago

The inegrations path has the controller, not system tests.

I am including 'passwordless/test_helpers'.

When I run a controller test within ActionDispatch::IntegrationTest:

Somewhere around here https://github.com/mikker/passwordless/blob/master/lib/passwordless/test_helpers.rb#L71

Screenshot 2024-02-12 at 17 23 28
mikker commented 5 months ago

Does your project include https://github.com/rails/rails-controller-testing ? Maybe that adds something?

bcasci commented 4 months ago

I'm having this issue in a new Rails 7.1 project as well. Though I am not in a 7.0 project that also uses the passwords gem. I've only just started looking into it. Maybe it's an issue brought on by a newer version or Rails.

bcasci commented 4 months ago

For the record, I tried downgrading to several lower version of the passwordless gem, but that hasn't changed the behavior. The code breaks because as @mikker pointed out, this like is being executed:

https://github.com/mikker/passwordless/blob/master/lib/passwordless/test_helpers.rb#L15


require 'passwordless/test_helpers'

class AlbumsControllerTest < ActionDispatch::IntegrationTest
  test 'this is what breaks' do
     passwordless_sign_in(users(:org_one_admin))
  end
end
bcasci commented 4 months ago

Here is a bit more information about this error, specific to ActionDispatch::IntegrationTest. It doesn't seem to be an issue with controller tests, which you can still do with Rspec or the spec flavor of Minitest, but they have been phased out by the Rails team for a while. So if you're working with a fairly default test environment, you will be ActionDispatch::IntegrationTest when testing controllers.

require 'test_helper'

module Manage
  class AlbumsControllerTest < ActionDispatch::IntegrationTest

    test 'this fails' do
      # @request is nil until a request is made, or alternatively until @request is manually set.
      # This is normal behavior for ActionDispatch::IntegrationTest, a request must be made in order
      # for @request to be set to an instance of ActionDispatch::Request
      # So you will always get a an error when ControllerHelpers.passwordless_sign_in is called  first, because @request is nil

      get "/" 

      # At this point @request is set to #<ActionDispatch::Request GET "http://www.example.com/" for 127.0.0.1> 

      # The passwordless helper can be called without error now,
      # however what it does will not do you any good with ActionDispatch::IntegrationTest

      passwordless_sign_in(users(:org_one_admin))

      get new_manage_album_url(subdomain: organizations(:one).subdomain)

      assert_response :success # fails because it's a 303 to the sign_in page
    end

    test 'this passes' do

      def passwordless_sign_in(resource)
        # Taken from Passwordless::TestHelpers::RequestTestCase
          session = Passwordless::Session.create!(authenticatable: resource)

          magic_link = Passwordless.context.path_for(
            session,
            action: "confirm",
            id: session.to_param,
            token: session.token
          )

          get(magic_link)
          follow_redirect!

           get new_manage_album_url(subdomain: organizations(:one).subdomain)

           assert_response :success # passes 200
      end
    end

I think its possible that including the Passwordless::TestHelpers::ControllerTestCase on ActiveSupport::TestCase might not work for some recent versions of Rails. https://github.com/mikker/passwordless/blob/09ed49293bef7e993bec7c0b205a82d224075f33/lib/passwordless/test_helpers.rb#L71

mikker commented 4 months ago

Nice digging πŸ‘ do you have an idea for a fix?

bcasci commented 4 months ago

I’m not sure what the best approach is. Maybe:

It would be interesting to run this situation by Copilot for an opinion.

bcasci commented 4 months ago

The issue was resolved when I added this to my test_helper.rb

if defined?(ActionDispatch::IntegrationTest)
  ActionDispatch::IntegrationTest.send(:include, ::Passwordless::TestHelpers::RequestTestCase)
end

So I guess that's the only change the gem source needs for it to work with a newer default rails configuration. Again, anyone using Rspec probably wouldn't encounter this issue, or people using the minitest DSL with describe(MyController) instead of the class form, because I believe minitest is using ActionDispatch::TestCase behind the DSL.

bcasci commented 4 months ago

I could submit a change after I get to a stopping point on my current project....you could assign this to me if you want so I won't forget.

mikker commented 4 months ago

Appreciate it! πŸ˜…

yshmarov commented 4 months ago

The issue was resolved when I added this to my test_helper.rb

if defined?(ActionDispatch::IntegrationTest)
  ActionDispatch::IntegrationTest.send(:include, ::Passwordless::TestHelpers::RequestTestCase)
end

So I guess that's the only change the gem source needs for it to work with a newer default rails configuration. Again, anyone using Rspec probably wouldn't encounter this issue, or people using the minitest DSL with describe(MyController) instead of the class form, because I believe minitest is using ActionDispatch::TestCase behind the DSL.

this workaround works for me