Allow to disable sessions #3183
Conversation
|
Ideally your API is backwards compatible with < 4.2, like this comment shows. Maybe something like: Sidekiq::Web.set :sessions, false |
|
@inkstak Let me know if you need any help |
|
So, you can disable sessions or You can also customize session options: (Sorry for the delay, it's crazy days) |
| Sidekiq::Web.new | ||
| end | ||
|
|
||
| after { Sidekiq::Web.enable(:sessions) } |
There was a problem hiding this comment.
Not sure it's necessary
There was a problem hiding this comment.
@inkstak For tests that change the state of the application I recommend changing the app directly, as you can see it is an instance of Sidekiq::Web.
You can port these methods for usage on the instance and test them here. If the implementation on the instance level works and we assume that the instance implementation is a proxy of the class one (or vice-versa) then it's fine.
If you change the class configuration it will be passed for all other tests.
Ideally we would not have the ability to mount Sidekiq::Web directly, just instances of it. But for compatibility and convenience we kept it.
See
- https://github.com/mperham/sidekiq/blob/master/test/test_web.rb#L646
- https://github.com/mperham/sidekiq/blob/master/test/test_web.rb#L620
for example
There was a problem hiding this comment.
I'm not comfortable with that. At the end it's still Sidekiq::Web to mount.
Adding instance methods adds complexity for the sole purpose to be tested.
That's my opinion. If you prefer tests with instance methods, the commit is ready.
There was a problem hiding this comment.
.. the commit is ready, one push behind !
There was a problem hiding this comment.
@inkstak I agree with you, but they are not for the sole purpose of testing.
The ideal design is to have only instance methods actually, the class methods only exist to provide retrocompatibiity with the old implementation, maybe we can change that in the future, depending on how @mperham sees it.
The issue with configuring Sidekiq::Web directly is that this change will replicate to the subsequent tests, unless you undefine the constant and reload the code for each test to restore the initial state.
There was a problem hiding this comment.
I'm always in favor of instance methods over class-level. The Sinatra/Rack API is bad in that regard. I'd much prefer to see something like:
web = Sidekiq::Web.new
web.something = ...
run webThere was a problem hiding this comment.
@mperham It's up to you if you want to introduce this change, my initial design prioritized retrocompatibility so that users of the old Sinatra API would'nt suffer with the migration.
We can discuss this in a new issue if you want.
Also, this likely could trigger a major version bump as it is a big (somewhat) breaking change, even if the fix is simple.
There was a problem hiding this comment.
@inkstak Feel free to push, and thanks for all the attention.
There was a problem hiding this comment.
No, you are right. Compatibility is #1. But that doesn't mean we can't have instance methods for testing.
| @@ -173,7 +173,7 @@ def display_args(args, truncate_after_chars = 2000) | |||
| end | |||
|
|
|||
| def csrf_tag | |||
| "<input type='hidden' name='authenticity_token' value='#{session[:csrf]}'/>" | |||
| "<input type='hidden' name='authenticity_token' value='#{session[:csrf]}'/>" if session | |||
There was a problem hiding this comment.
I don't believe this should ever be disabled. Can you explain why this is necessary?
There was a problem hiding this comment.
Test artefact, removed.
|
Looking much better, let me know when you consider the PR complete. |
|
The PR looks great for me, the last question is about refactoring the tests or not. |
|
|
||
| @sessions | ||
| end | ||
|
|
There was a problem hiding this comment.
If you want to add settings on instance level, which inherits from class,
you can use that :
%w(sessions session_secret app_url redis_pool).each do |method_name|
attr_writer method_name
define_method(method_name) do
var = "@#{method_name}"
unless instance_variable_defined?(var)
value = self.class.send(method_name)
value = value.to_hash.dup if value.respond_to?(:to_hash)
instance_variable_set(var, value)
end
instance_variable_get(var)
end
end
|
This looks great - I'm inclined to merge as is. Any other opinions? |
|
I'm waiting for it. Thanks for your support ! |
|
🎉 @inkstak Thanks! |
|
This has the side effect that if sessions are disabled, CSRF protection is disabled. Users should not be able to disable rack-protection. |
|
One more issue: subclasses of Sidekiq::Web are not inheriting the value set for sessions, etc. |
|
This should be in the FAQ |
|
Then add it.
… On Jul 25, 2017, at 16:28, devlinzed ***@***.***> wrote:
This should be in the FAQ
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub, or mute the thread.
|
GitLab already has its own session store, so this extra Sidekiq session is unnecessary. In addition, the GitLab session store properly sets the Secure flag, unlike the default Rack session. CSRF protection in the Sidekiq /admin page continues to work with the existing GitLab session. See sidekiq/sidekiq#3183 for more details. Part of #49120
According to #3180, this pull request allows to disable session.
The pull request is not ready to be merged : the test I've added won't pass, when executed along with the full test suite.
I'm not yet familiar with rack tests and looking for isolated sessions between tests.