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

Documentation on sign_in hooks is incorrect #424

Closed
geoffharcourt opened this issue Apr 19, 2014 · 16 comments
Closed

Documentation on sign_in hooks is incorrect #424

geoffharcourt opened this issue Apr 19, 2014 · 16 comments

Comments

@geoffharcourt
Copy link
Contributor

https://github.com/thoughtbot/clearance/wiki/usage

The suggested override of ApplicationController#sign_in produces an exception about calling a protected method on ApplicationController. I think this may solely be an issue in Rails 4, as I was able to use the last signed in example in a Rails 3.2.x application.

@derekprior
Copy link
Contributor

Thanks, Geoff. I'll take a look sometime this week. Much of the wiki documentation is out of date. I've been considering removing it all and adding back only what is accurate and helpful.

@geoffharcourt
Copy link
Contributor Author

@derekprior Is there a preferred way of doing a sign-in hook in Rails 4?

@derekprior
Copy link
Contributor

Hmm. I don't have an app in front of me to test right now, but I'm actually surprised that doesn't work. Can you paste the exception and stack trace as well as your code from ApplicationController?

I'll have some time to look into this myself tomorrow as well.

@geoffharcourt
Copy link
Contributor Author

Here's the ApplicationController code. I have the exact same code in a Rails 3.2.17 app and it works fine:

##
# Base controller for application.
##
class ApplicationController < ActionController::Base
  include Clearance::Controller

  # Prevent CSRF attacks by raising an exception.
  # For APIs, you may want to use :null_session instead.
  protect_from_forgery with: :exception

  private

  def default_serializer_options
    {root: false}
  end

  protected

  def sign_in(user)
    # store current time to display "last signed in at" message
    user.update_attribute(:last_signed_in_at, Time.now)
    super user
  end
end

Stacktrace (this is from a test step where I use Clearance::BackDoor, but it's causing a failure in every previously working step where I used #sign_in_as):

Failures:

  1) Admin::PasswordsController PUT update when the old password matches
     Failure/Error: sign_in_as(user)
     NoMethodError:
       protected method `sign_in' called for #<Admin::PasswordsController:0x007fe51f248068>
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/clearance-1.3.0/lib/clearance/testing/helpers.rb:22:in `sign_in_as'
     # ./spec/controllers/admin/passwords_controller_spec.rb:9:in `block (4 levels) in <top (required)>'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/example.rb:237:in `instance_eval'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/example.rb:237:in `instance_eval'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/hooks.rb:21:in `run'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/hooks.rb:85:in `block in run'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/hooks.rb:85:in `each'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/hooks.rb:85:in `run'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/hooks.rb:446:in `run_hook'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/example_group.rb:345:in `run_before_each_hooks'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/example.rb:294:in `run_before_each'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/example.rb:113:in `block in run'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/extensions/instance_eval_with_args.rb:16:in `instance_exec'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/extensions/instance_eval_with_args.rb:16:in `instance_eval_with_args'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/example.rb:247:in `instance_eval_with_args'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/hooks.rb:106:in `block (2 levels) in run'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/example.rb:179:in `call'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/example.rb:179:in `run'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/extensions/instance_eval_with_args.rb:16:in `instance_exec'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/extensions/instance_eval_with_args.rb:16:in `instance_eval_with_args'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/example.rb:247:in `instance_eval_with_args'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/hooks.rb:106:in `block (2 levels) in run'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/hooks.rb:108:in `call'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/hooks.rb:108:in `run'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/hooks.rb:446:in `run_hook'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/example_group.rb:340:in `run_around_each_hooks'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/example.rb:256:in `with_around_each_hooks'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/example.rb:111:in `run'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/example_group.rb:390:in `block in run_examples'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/example_group.rb:386:in `map'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/example_group.rb:386:in `run_examples'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/example_group.rb:371:in `run'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/example_group.rb:372:in `block in run'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/example_group.rb:372:in `map'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/example_group.rb:372:in `run'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/example_group.rb:372:in `block in run'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/example_group.rb:372:in `map'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/example_group.rb:372:in `run'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/command_line.rb:28:in `block (2 levels) in run'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/command_line.rb:28:in `map'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/command_line.rb:28:in `block in run'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/reporter.rb:58:in `report'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/command_line.rb:25:in `run'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/runner.rb:80:in `run'
     # /Users/geoff/.rbenv/versions/2.1.1/lib/ruby/gems/2.1.0/gems/rspec-core-2.14.8/lib/rspec/core/runner.rb:17:in `block in autorun'

@derekprior
Copy link
Contributor

This actually seems to work for me in my Rails 4.0.4 app. I wonder if it has to do with the fact that your controller controller is namespaced. Does Admin::Passwords controller inherit directly from ApplicationController?

@geoffharcourt
Copy link
Contributor Author

No, it inherits from a subclass of ApplicationController whose function is to require login.

@derekprior
Copy link
Contributor

@geoffharcourt Have you had any luck with this? I haven't looked into it at all yet, though I was going to try to this weekend.

@geoffharcourt
Copy link
Contributor Author

No, I haven't. My initial attempt wasn't successful, so if you have the time I'd go ahead.

@derekprior
Copy link
Contributor

I'm still not able to reproduce this, even trying with what I think are similar chains of inheritance and namespaced controllers:

class ApplicationController < ActionController::Base
  include Clearance::Controller
  protect_from_forgery with: :exception

  protected

  def sign_in(user)
    puts "SIGN IN #{user.email}"
    super
  end
end
class BaseController < ApplicationController
  before_filter :authorize
end
class Admin::PostsController < BaseController
  def index
    render text: 'admin posts controller'
  end
end
Clearance.configure do |config|
  config.mailer_sender = "reply@example.com"
  config.redirect_url = '/admin/posts'
end

This all works just fine. I even tried it via a feature spec with the backdoor, like so:

feature 'user visits admin posts' do
  scenario 'successfully' do
    user = User.create(email: 'user@domain.com', password: 'password')
    visit admin_posts_path(as: user)
    expect(page).to have_content 'admin posts controller'
  end
end

This is now Rails 4.1.4, but way earlier I was also unable to reproduce on 4.0.4. Do you see something that I'm doing that you're not? Are you able to produce a minimal reproduction you could share?

@geoffharcourt
Copy link
Contributor Author

@derekprior, thanks for taking a look at this. I'm working on a project over the weekend that's due on Monday, but I'll try to build a minimal reproduction to help demonstrate the problem outside of my private app after the weekend.

@geoffharcourt
Copy link
Contributor Author

@derekprior, I just built a simple app with a Posts controller that inherits from a controller that requires authorization (BaseController) that in turn inherits from ApplicationController, and can't reproduce the problem on a small app, but it is still occurring in my big production Rails 4.1.6 app.

Stack trace isn't very useful:

32) DashboardController GET index
     Failure/Error: sign_in_as(user)
     NoMethodError:
       protected method `sign_in' called for #<DashboardController:0x007f9445d6da18>
     # /Users/geoff/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/clearance-1.4.2/lib/clearance/testing/helpers.rb:22:in `sign_in_as'
     # ./spec/controllers/dashboard_controller_spec.rb:22:in `block (2 levels) in <top (required)>'

  ... (rspec gem backtrace) ...

The only thing I can think of is that in my tests I'm not using the backdoor, but rather the sign_in_as helper:

  before do
    Timecop.freeze(Time.utc(2014, 5, 21, 12))
    create(:building_user, building: building, user: user)

    sign_in_as(user)
  end

  describe "GET index" do
    before { get :index }

    it { should respond_with(:redirect) }
    it { should redirect_to(today_dashboard_path(building)) }
  end

If I make #sign_in a public method in ApplicationController, the tests pass. It's only as soon as I move the method into protected or private scope that I get an error.

I should note that this only produces an error in my app during testing (I had previously abandoned this approach when the test failed). If I sign in to the development environment with an overridden #sign_in method on ApplicationController (in the protected scope), it updates my user record with the last logged in time and the app. The failures only occur in controller tests, all my request and integration specs (all of which use the Clearance "backdoor") pass.

@geoffharcourt
Copy link
Contributor Author

@derekprior, wondering if calling #sign_in in the #sign_in_as helper using #send(:sign_in, user) might be the solution, avoiding the restriction on calling a protected method?

@derekprior
Copy link
Contributor

Yeah, I was thinking about that. I probably won't get a chance to look at
it until next week.

On Wed, Sep 17, 2014 at 12:18 PM, Geoff Harcourt notifications@github.com
wrote:

@derekprior https://github.com/derekprior, wondering if calling #sign_in
in the #sign_in_as helper using #send(:sign_in, user) might be the
solution, avoiding the restriction on calling a protected method?


Reply to this email directly or view it on GitHub
#424 (comment)
.

@geoffharcourt
Copy link
Contributor Author

No prob, just wanted to jot that down before I forgot.

geoffharcourt added a commit to geoffharcourt/clearance that referenced this issue Sep 21, 2014
I was overriding `sign_in` in a subclassed controller (ApplicationController was
public, but a subclass was used for all pages requiring authentication), and
experienced problems during testing where exceptions were being raised by
Clearance's testing helpers calling the protected method `#sign_in` on the
controller.

Changed the `sign_in_as` helper to use `Object#send`, which will not raise
no method exceptions if a controller's `#sign_in` method is protected, which as
a non-action controller method it should be.

Close thoughtbot#424.
@geoffharcourt
Copy link
Contributor Author

@derekprior I was able to reproduce this in a Clearance spec, and resolved it by using #send to access the controller's #sign_in method despite being in protected scope.

@derekprior
Copy link
Contributor

I updated the wiki per our conversations in #464

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

Successfully merging a pull request may close this issue.

2 participants