Rails 3.1rc1 streaming breaks AuthLogic session #262

Open
subelsky opened this Issue Jun 8, 2011 · 24 comments

Comments

Projects
None yet
@subelsky

subelsky commented Jun 8, 2011

I ran into this problem trying to use Authlogic with Rails 3.1rc1; looks like a problem on the Authlogic side with how cookies are handled;

rails/rails#1452

I'm going to dig around but wanted to post this to get on the radar..

@aeden

This comment has been minimized.

Show comment Hide comment
@aeden

aeden Jun 8, 2011

I've put together a simple example of this error in practice with Cucumber. https://github.com/aeden/rails31_authlogic

Just run cucumber to see the error.

aeden commented Jun 8, 2011

I've put together a simple example of this error in practice with Cucumber. https://github.com/aeden/rails31_authlogic

Just run cucumber to see the error.

@moiristo

This comment has been minimized.

Show comment Hide comment
@moiristo

moiristo Jun 20, 2011

ok, so this seems to be an authlogic bug. Is someone able to fix this or should I move a different authentication solution?

ok, so this seems to be an authlogic bug. Is someone able to fix this or should I move a different authentication solution?

@subelsky

This comment has been minimized.

Show comment Hide comment
@subelsky

subelsky Jun 20, 2011

it's not an authlogic bug - it affects other frameworks because they all do stuff with cookies. If you look at the Rails ticket you'll see people complaining about Devise

it's not an authlogic bug - it affects other frameworks because they all do stuff with cookies. If you look at the Rails ticket you'll see people complaining about Devise

@moiristo

This comment has been minimized.

Show comment Hide comment
@moiristo

moiristo Jun 23, 2011

As I don't have the time to wait for someone to eventually find a solution, I stripped authlogic out and went with using has_secure_password instead.. seems like it contains some 'bad' code anyway according to the other ticket.

As I don't have the time to wait for someone to eventually find a solution, I stripped authlogic out and went with using has_secure_password instead.. seems like it contains some 'bad' code anyway according to the other ticket.

@dmonopoly

This comment has been minimized.

Show comment Hide comment
@dmonopoly

dmonopoly Jun 25, 2011

I'm hoping Authlogic will work perfectly with Rails 3.1rc soon... for now, though, it's apparent that the problem is on Rails 3.1's side?

I'm hoping Authlogic will work perfectly with Rails 3.1rc soon... for now, though, it's apparent that the problem is on Rails 3.1's side?

@subelsky

This comment has been minimized.

Show comment Hide comment
@subelsky

subelsky Jun 26, 2011

That Rails ticket states this is an authlogic bug: "It is storing an instance of the controller in a thread variables and modifying the cookies after the response is sent back to the client.

You can clearly see in the backtrace that the error comes from a model callback. This is nasty. Storing the controller in threads variables so it can be accessible from the model is one of the worst ways to violate MVC."

that's sort of core to how Authlogic works, no? Is there a different where to share this state?

That Rails ticket states this is an authlogic bug: "It is storing an instance of the controller in a thread variables and modifying the cookies after the response is sent back to the client.

You can clearly see in the backtrace that the error comes from a model callback. This is nasty. Storing the controller in threads variables so it can be accessible from the model is one of the worst ways to violate MVC."

that's sort of core to how Authlogic works, no? Is there a different where to share this state?

@dmonopoly

This comment has been minimized.

Show comment Hide comment
@dmonopoly

dmonopoly Jun 27, 2011

Are you saying that Rails 3.1 users shouldn't use Authlogic? :/ I like Authlogic...

Are you saying that Rails 3.1 users shouldn't use Authlogic? :/ I like Authlogic...

@aeden

This comment has been minimized.

Show comment Hide comment
@aeden

aeden Jun 27, 2011

No, I think he's saying that it may take a little bit of effort to figure out a way that doesn't break in Rails 3.1.

aeden commented Jun 27, 2011

No, I think he's saying that it may take a little bit of effort to figure out a way that doesn't break in Rails 3.1.

@dmonopoly

This comment has been minimized.

Show comment Hide comment
@dmonopoly

dmonopoly Jun 27, 2011

I'm wondering about the timeframe, then. For new applications in Rails 3.1, what is recommended for user authentication if this problem won't be resolved anytime soon?
When do we think this issue could be resolved?

I'm wondering about the timeframe, then. For new applications in Rails 3.1, what is recommended for user authentication if this problem won't be resolved anytime soon?
When do we think this issue could be resolved?

@dmonopoly

This comment has been minimized.

Show comment Hide comment
@dmonopoly

dmonopoly Jun 29, 2011

In a new Rails 3.1.0rc4 app, I finally encountered this error in a cucumber test.
Interestingly, I did not encounter it in my first feature, in which I call this step definition:

Given /^I am the registered (.+) "(.+)"$/ do |role, login|
  @user = User.create!(
    :role => role,
    :login => login,
    :password => "password",
    :password_confirmation => "password",
    :email => "johndoe@somewhere.com"
  )
end

I encounter the issue when I run the broad command "cucumber" or "rake cucumber" - i.e. when all my features are run. The feature that first calls this step is fine, but the second feature that calls this step yields the "Cannot modify cookies because it was closed. This means it was already streamed back to the client or converted to HTTP headers. (ActionDispatch::ClosedError)." If I run one feature at a time (with cucumber features/featureX.feature), this step works fine... so individually, all good; together, not all.

Interested in knowing when the fix could be made.

In a new Rails 3.1.0rc4 app, I finally encountered this error in a cucumber test.
Interestingly, I did not encounter it in my first feature, in which I call this step definition:

Given /^I am the registered (.+) "(.+)"$/ do |role, login|
  @user = User.create!(
    :role => role,
    :login => login,
    :password => "password",
    :password_confirmation => "password",
    :email => "johndoe@somewhere.com"
  )
end

I encounter the issue when I run the broad command "cucumber" or "rake cucumber" - i.e. when all my features are run. The feature that first calls this step is fine, but the second feature that calls this step yields the "Cannot modify cookies because it was closed. This means it was already streamed back to the client or converted to HTTP headers. (ActionDispatch::ClosedError)." If I run one feature at a time (with cucumber features/featureX.feature), this step works fine... so individually, all good; together, not all.

Interested in knowing when the fix could be made.

@shwoodard

This comment has been minimized.

Show comment Hide comment
@shwoodard

shwoodard Aug 12, 2011

Can this be fixed soon?

Can this be fixed soon?

@snelson

This comment has been minimized.

Show comment Hide comment
@snelson

snelson Aug 15, 2011

I think I found the cause ...

It has to do with AuthLogic's automatic session maintenance. When you create a model that acts_as_authentic (User.create), by default it will create a new UserSession instance for you, or, in other words, log the user in.

If you create a User outside of a controller request, like when you factory a User to login with, that's when you get the ActionDispatch::ClosedError, not when you post to the controller.

If you disable automatic session maintenance, you don't get the error. Here's how you do that:

class User < ActiveRecord::Base
  acts_as_authentic do |c|
    c.maintain_sessions = false
  end
end

But, disabling automatic session maintenance means you'll need to add some code anywhere you expected a User.save or User.create to log in automatically. For me, it just meant adding this one line after a successful User.save wherever login was expected:

if @user.save
  UserSession.create @user
end

If you don't want to change your code, just do this whenever you need to factory a User account in your tests:

# ActiveRecord
user = User.new(attributes)
user.save_without_session_maintenance

# FactoryGirl
user = Factory.build(:user)
user.save_without_session_maintenance

There is one other way I did try that worked as a temporary hack, but I really wouldn't recommended it because I'm sure it would cause other problems. I stubbed out ActionDispatch::Cookies::CookieJar to never think it was closed, in an initializer:

# DON'T USE THIS

if Rails.env.test?
  module ActionDispatch
    class Cookies
      class CookieJar
        def closed?
          return false
        end
      end
    end
  end
end

I'm not sure what the approach will be for a long term fix, but I suspect it would involve checking if the User create or save is happening within a controller request, and if its not, don't try to do any session maintenance. I personally wouldn't mind if it became an opt-in instead of being default, perhaps by passing an attribute :login => true or adding a save_and_login or create_and_login, etc.

snelson commented Aug 15, 2011

I think I found the cause ...

It has to do with AuthLogic's automatic session maintenance. When you create a model that acts_as_authentic (User.create), by default it will create a new UserSession instance for you, or, in other words, log the user in.

If you create a User outside of a controller request, like when you factory a User to login with, that's when you get the ActionDispatch::ClosedError, not when you post to the controller.

If you disable automatic session maintenance, you don't get the error. Here's how you do that:

class User < ActiveRecord::Base
  acts_as_authentic do |c|
    c.maintain_sessions = false
  end
end

But, disabling automatic session maintenance means you'll need to add some code anywhere you expected a User.save or User.create to log in automatically. For me, it just meant adding this one line after a successful User.save wherever login was expected:

if @user.save
  UserSession.create @user
end

If you don't want to change your code, just do this whenever you need to factory a User account in your tests:

# ActiveRecord
user = User.new(attributes)
user.save_without_session_maintenance

# FactoryGirl
user = Factory.build(:user)
user.save_without_session_maintenance

There is one other way I did try that worked as a temporary hack, but I really wouldn't recommended it because I'm sure it would cause other problems. I stubbed out ActionDispatch::Cookies::CookieJar to never think it was closed, in an initializer:

# DON'T USE THIS

if Rails.env.test?
  module ActionDispatch
    class Cookies
      class CookieJar
        def closed?
          return false
        end
      end
    end
  end
end

I'm not sure what the approach will be for a long term fix, but I suspect it would involve checking if the User create or save is happening within a controller request, and if its not, don't try to do any session maintenance. I personally wouldn't mind if it became an opt-in instead of being default, perhaps by passing an attribute :login => true or adding a save_and_login or create_and_login, etc.

@dmonopoly

This comment has been minimized.

Show comment Hide comment
@dmonopoly

dmonopoly Aug 18, 2011

@snelson: Great info. It looks like I'll be using Factory.build(:user) and save_without_session_maintenance. The other options look great too.
Truly, truly appreciated.

@snelson: Great info. It looks like I'll be using Factory.build(:user) and save_without_session_maintenance. The other options look great too.
Truly, truly appreciated.

@snelson

This comment has been minimized.

Show comment Hide comment
@snelson

snelson Aug 18, 2011

@dmonopoly: No problem man, it was driving me nuts not being able to run my acceptance specs.

snelson commented Aug 18, 2011

@dmonopoly: No problem man, it was driving me nuts not being able to run my acceptance specs.

@peternixey

This comment has been minimized.

Show comment Hide comment
@peternixey

peternixey Sep 1, 2011

@snelson - thank you, that fix worked straight off the bat for me too. Much appreciated

@snelson - thank you, that fix worked straight off the bat for me too. Much appreciated

@trkoch

This comment has been minimized.

Show comment Hide comment
@trkoch

trkoch Sep 18, 2011

@snelson, saving without session maintenance did the trick. Many thanks! Maybe one should a notice to the README?

trkoch commented Sep 18, 2011

@snelson, saving without session maintenance did the trick. Many thanks! Maybe one should a notice to the README?

@trkoch

This comment has been minimized.

Show comment Hide comment
@trkoch

trkoch Sep 18, 2011

Unfortunately I ran into another problem. Turns out I need an existing session for a couple of tests. Reluctantly applied the monkey patch.

trkoch commented Sep 18, 2011

Unfortunately I ran into another problem. Turns out I need an existing session for a couple of tests. Reluctantly applied the monkey patch.

@volpe

This comment has been minimized.

Show comment Hide comment
@volpe

volpe Sep 29, 2011

Added @snelson 's User patch to your support folder (for cucumber) and you'll solve this issue. (Works a charm for me).

volpe commented Sep 29, 2011

Added @snelson 's User patch to your support folder (for cucumber) and you'll solve this issue. (Works a charm for me).

@masonmark

This comment has been minimized.

Show comment Hide comment
@masonmark

masonmark Oct 28, 2011

@snelson's the c.maintain_sessions = false modification was the most straightforward way to solve this issue for me (thanks!). I agree with @trkoch that this should probably be mentioned in the README.

@snelson's the c.maintain_sessions = false modification was the most straightforward way to solve this issue for me (thanks!). I agree with @trkoch that this should probably be mentioned in the README.

@andyferra

This comment has been minimized.

Show comment Hide comment
@andyferra

andyferra Nov 3, 2011

If you like, you can also alter the configuration in your spec setup with:

User.acts_as_authentic_config[:maintain_sessions] = false

I find this preferable as opposed to re-opening the class.

If you like, you can also alter the configuration in your spec setup with:

User.acts_as_authentic_config[:maintain_sessions] = false

I find this preferable as opposed to re-opening the class.

@tmcallister

This comment has been minimized.

Show comment Hide comment
@tmcallister

tmcallister Jan 9, 2012

Here's what we did in the user model to address.

class User < ActiveRecord::Base
  acts_as_authentic do |auth|
    auth.maintain_sessions = false   if Rails.env == "test" # authlogic/issues/262
  end
 end

Here's what we did in the user model to address.

class User < ActiveRecord::Base
  acts_as_authentic do |auth|
    auth.maintain_sessions = false   if Rails.env == "test" # authlogic/issues/262
  end
 end
@tilsammans

This comment has been minimized.

Show comment Hide comment
@tilsammans

tilsammans Jan 24, 2012

The fix mentioned by @andyferra works and I prefer it too.

As far as I am concerned, the automatic session maintenance is removed from Authlogic completely. It's not how Authlogic is documented anyway, right? The docs always mention creating sessions explicitly, the fact this hook is there surprised me to begin with.

The fix mentioned by @andyferra works and I prefer it too.

As far as I am concerned, the automatic session maintenance is removed from Authlogic completely. It's not how Authlogic is documented anyway, right? The docs always mention creating sessions explicitly, the fact this hook is there surprised me to begin with.

@shwoodard

This comment has been minimized.

Show comment Hide comment
@shwoodard

shwoodard Jan 24, 2012

Cheers, @andyferra!

On Tuesday, January 24, 2012 at 2:02 AM, Joost Baaij wrote:

The fix mentioned by @andyferra works and I prefer it too.

As far as I am concerned, the automatic session maintenance is removed from Authlogic completely. It's not how Authlogic is documented anyway, right? The docs always mention creating sessions explicitly, the fact this hook is there surprised me to begin with.


Reply to this email directly or view it on GitHub:
#262 (comment)

Cheers, @andyferra!

On Tuesday, January 24, 2012 at 2:02 AM, Joost Baaij wrote:

The fix mentioned by @andyferra works and I prefer it too.

As far as I am concerned, the automatic session maintenance is removed from Authlogic completely. It's not how Authlogic is documented anyway, right? The docs always mention creating sessions explicitly, the fact this hook is there surprised me to begin with.


Reply to this email directly or view it on GitHub:
#262 (comment)

@cluesque

This comment has been minimized.

Show comment Hide comment
@cluesque

cluesque Apr 28, 2014

This is a dup of #52, no?

This is a dup of #52, no?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment