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

Roll-your-own session stores are broken #1545

Merged
merged 1 commit into from Jan 4, 2014

Conversation

Projects
None yet
2 participants
@skade
Member

skade commented Jan 3, 2014

https://github.com/padrino/padrino-framework/blob/master/padrino-core/lib/padrino-core/application.rb#L369

makes it impossible to replace the session store, because that requires to switch sessions to false.

@ghost ghost assigned skade Jan 3, 2014

Don't raise on protect_from_csrf without sessions
Sessions can be implemented in a multitude of ways and not
all lead to the `sessions` option being set.
@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Jan 3, 2014

Member

I agree that the issue exists but I don't like inventing new non-functional flags.

What do you think about something like this:

# override sinatra session setup:
def setup_sessions(builder)
  if sessions.kind_of? Proc
    sessions.call builder
    enable :sessions
  else
    super builder
  end
end

This way we will have internals getting proper answer on sessions? and introduce a new syntax for custom session storage:

set :sessions do |builder|
  builder.use Rack::Session::DataMapper, :key => 'sid', :path => '/', :secret => 'DympNaneu', :expire_after => 1.month
end
Member

ujifgc commented Jan 3, 2014

I agree that the issue exists but I don't like inventing new non-functional flags.

What do you think about something like this:

# override sinatra session setup:
def setup_sessions(builder)
  if sessions.kind_of? Proc
    sessions.call builder
    enable :sessions
  else
    super builder
  end
end

This way we will have internals getting proper answer on sessions? and introduce a new syntax for custom session storage:

set :sessions do |builder|
  builder.use Rack::Session::DataMapper, :key => 'sid', :path => '/', :secret => 'DympNaneu', :expire_after => 1.month
end
@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Jan 3, 2014

Member

Well, at the same time I don't like overriding base library behavior with self-cooked implementations. We defer to the Sinatra documentation for all such things, so we should try to keep that intact.

Also, consider that any reasonable multi-app setup doesn't touch :sessions at all, but instead uses:

Padrino.use Rack::Session::DataMapper #...

The other option would be to get rid of the warning for 0.12, it was mainly introduced to make migrating easier.

Member

skade commented Jan 3, 2014

Well, at the same time I don't like overriding base library behavior with self-cooked implementations. We defer to the Sinatra documentation for all such things, so we should try to keep that intact.

Also, consider that any reasonable multi-app setup doesn't touch :sessions at all, but instead uses:

Padrino.use Rack::Session::DataMapper #...

The other option would be to get rid of the warning for 0.12, it was mainly introduced to make migrating easier.

ujifgc added a commit that referenced this pull request Jan 4, 2014

Merge pull request #1545 from skade/fix/issue1545
Roll-your-own session stores are broken

@ujifgc ujifgc merged commit 8a4ac43 into padrino:master Jan 4, 2014

1 check failed

default The Travis CI build could not complete due to an error
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment