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

override setup_session method to support custom session management #1663

Merged
merged 2 commits into from May 8, 2014

Conversation

Projects
None yet
5 participants
@cookiebody
Contributor

cookiebody commented Apr 24, 2014

I always use Rack::Session::MongoSession in Padrino:

class App < Padrino::Application
  use Rack::Session::MongoSession, {
    :host => "localhost",
    :db_name => 'mongo_sessions',
    :marshal_data => true
  }
end

But if enabe csrf protection:

 enable :protect_from_csrf

Because Rack::Protection::AuthenticityToken is used before Rack::Session::MongoSession,
Padrino will warn "protect_from_csrf is activated, but sessions seem to be off. "
I could cancel warn through Padrino::IGNORE_CSRF_SETUP_WARNING.
But, Rack::Protection::AuthenticityToken would use session to store csrf token.

So, I think Padrino should allow custome session in application setup.

@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Apr 24, 2014

Member

@cookiebody Thank you for PR!
I think this proposal looks good.

Member

namusyaka commented Apr 24, 2014

@cookiebody Thank you for PR!
I think this proposal looks good.

Edit setup_sessions in application_setup.rb
*Indentation should be 2 space.
*The argument of super method is not needed.
*Drop if sessions condition
@cookiebody

This comment has been minimized.

Show comment
Hide comment
@cookiebody

cookiebody Apr 24, 2014

Contributor

@namusyaka Edited, many thanks. I need more Ruby practice. ^_^

Contributor

cookiebody commented Apr 24, 2014

@namusyaka Edited, many thanks. I need more Ruby practice. ^_^

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Apr 24, 2014

Member

@namusyaka check this discussion #1545

Does set :sessions, :use => Rack::Session::Pool conform with anything racky or sinatrish?

Member

ujifgc commented Apr 24, 2014

@namusyaka check this discussion #1545

Does set :sessions, :use => Rack::Session::Pool conform with anything racky or sinatrish?

@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Apr 24, 2014

Member

Thanks.
I have just understood skade's opinion.
However, I think Padrino should be able to customize the session, because Padrino is different from Sinatra, Padrino is supporting many components.

Does set :sessions, :use => Rack::Session::Pool conform with anything racky or sinatrish?

We should think about the syntax of this proposal.
I'm not sure that code conforms with racky or sinatrish.
@ujifgc What do you think?

Member

namusyaka commented Apr 24, 2014

Thanks.
I have just understood skade's opinion.
However, I think Padrino should be able to customize the session, because Padrino is different from Sinatra, Padrino is supporting many components.

Does set :sessions, :use => Rack::Session::Pool conform with anything racky or sinatrish?

We should think about the syntax of this proposal.
I'm not sure that code conforms with racky or sinatrish.
@ujifgc What do you think?

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Apr 24, 2014

Member

This should answer the question:

http://www.sinatrarb.com/faq.html#sessions

set :sessions is really only for standard session.

I would silence the warning, it's just there to make sure that people are not stumped if they didn't activate sessions.

Member

skade commented Apr 24, 2014

This should answer the question:

http://www.sinatrarb.com/faq.html#sessions

set :sessions is really only for standard session.

I would silence the warning, it's just there to make sure that people are not stumped if they didn't activate sessions.

@cookiebody

This comment has been minimized.

Show comment
Hide comment
@cookiebody

cookiebody Apr 25, 2014

Contributor

Padrino supports mounting multi apps.
I want to use different session management among mounted apps.

class Admin < Padrino::Application
  use Rack::Session::MongoSession, {
    :host => "localhost",
    :db_name => 'admin_mongo_sessions',
    :marshal_data => true
  }
end
class Api < Padrino::Application
  enable :sessions
end
class App < Padrino::Application
  use Rack::Session::MongoSession, {
    :host => "localhost",
    :db_name => 'app_mongo_sessions',
    :marshal_data => true
  }
 enable :protect_from_csrf
end

Use Padrino::IGNORE_CSRF_SETUP_WARNING = true could stop Padrino's warn,
but rack-proection still throws error: "you need to set up a session middleware before Rack::Protection::AuthenticityToken"
If not to override sinatra's method,
may Padrino invent new settings to use custom session management before use Rack::Protection::AuthenticityToken?

Contributor

cookiebody commented Apr 25, 2014

Padrino supports mounting multi apps.
I want to use different session management among mounted apps.

class Admin < Padrino::Application
  use Rack::Session::MongoSession, {
    :host => "localhost",
    :db_name => 'admin_mongo_sessions',
    :marshal_data => true
  }
end
class Api < Padrino::Application
  enable :sessions
end
class App < Padrino::Application
  use Rack::Session::MongoSession, {
    :host => "localhost",
    :db_name => 'app_mongo_sessions',
    :marshal_data => true
  }
 enable :protect_from_csrf
end

Use Padrino::IGNORE_CSRF_SETUP_WARNING = true could stop Padrino's warn,
but rack-proection still throws error: "you need to set up a session middleware before Rack::Protection::AuthenticityToken"
If not to override sinatra's method,
may Padrino invent new settings to use custom session management before use Rack::Protection::AuthenticityToken?

@nesquena nesquena added this to the 0.12.2 milestone Apr 28, 2014

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc May 4, 2014

Member

I'm in favor of accepting this PR. The underlying issue is not just about using alternative session storage but about initializing it in the right order, meaning it must be used by the builder before using protection module. It cannot be organized by simply configuring Sinatra or Padrino because all the uses inside the application are actually called after initializing default middleware (including protection).

Refs:
#use
#build
#setup_default_middleware
#setup_middleware

Member

ujifgc commented May 4, 2014

I'm in favor of accepting this PR. The underlying issue is not just about using alternative session storage but about initializing it in the right order, meaning it must be used by the builder before using protection module. It cannot be organized by simply configuring Sinatra or Padrino because all the uses inside the application are actually called after initializing default middleware (including protection).

Refs:
#use
#build
#setup_default_middleware
#setup_middleware

@cookiebody

This comment has been minimized.

Show comment
Hide comment
@cookiebody

cookiebody May 4, 2014

Contributor

@ujifgc Exactly, right order is the key.

Contributor

cookiebody commented May 4, 2014

@ujifgc Exactly, right order is the key.

@cookiebody cookiebody changed the title from override setup_session method to support custome session management to override setup_session method to support custom session management May 6, 2014

@ujifgc ujifgc added the feature label May 7, 2014

ujifgc added a commit that referenced this pull request May 8, 2014

Merge pull request #1663 from cookiebody/master
override setup_session method to support custom session management

@ujifgc ujifgc merged commit 0b4d049 into padrino:master May 8, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment