Redmine

Test fails on drone.io.

https://drone.io/github.com/marutosi/redmine/211
https://drone.io/github.com/marutosi/redmine/212

  1) Failure:
test_api_should_trigger_basic_http_auth_with_basic_authorization_header(Redmine::ApiTest::AuthenticationTest) 
[/xxxxxxxx/redmine/test/integration/api_test/authentication_test.rb:34]:
Expected response to be a <401>, but was <500>

Tests on CI Server pass and I cannot reproduce on my CentOS 6.5.
http://www.redmine.org/builds/index.html

On Ubuntu 12.04.4 LTS,
ruby 1.9.3p545 (2014-02-24 revision 45159) [x86_64-linux]

  1) Failure:
test_api_should_trigger_basic_http_auth_with_basic_authorization_header(Redmine::ApiTest::AuthenticationTest)
[test/integration/api_test/authentication_test.rb:34]:
Expected response to be a <401>, but was <500>

Mocha::ExpectationError (unexpected invocation: #<AnyInstance:UsersController>.authenticate_with_http_basic()
unsatisfied expectations:
- expected exactly once, not yet invoked: #<AnyInstance:ApplicationController>.authenticate_with_http_basic(any_parameters)
):
  app/controllers/application_controller.rb:125:in `find_current_user'
  app/controllers/application_controller.rb:100:in `user_setup'
  test/integration/api_test/authentication_test.rb:33:in `test_api_should_trigger_basic_http_auth_with_basic_authorization_header'

Is it really useful to add a new runtime dependency? Wouldn't a complete test be enough instead? And if jot, could that change be targeted to next major release?

Etienne Massip wrote:

Is it really useful to add a new runtime dependency?

mocha is in "test" group.

Wouldn't a complete test be enough instead?

It depends web servers, so we cannot add new tests.
https://github.com/steveklabnik/request_store#the-problem

And if not, could that change be targeted to next major release?

This issue is security fix depends on #16511 with r13041,
so I think it should be targeted to minor release.

I meant request store, not mocha. This should be handled by RoR, not by a specific dependency.

We usually upgrade Rails version for security fix in minor release.
And r13110 does not change application behaviors.

Sure but this is not a security fix, only a potential fix (which from my very personal pov may be a bit of overkill since it seems that all possibilities of having a wrong user in thread are now prevented).

Thanks, it was just my personal opinion; it may be safer than adding a new gem which might itself contain some security issue we're not aware of!

Etienne: It's way too easy to introduce data leaks when dealing with before filters, either in the core or in Plugins, so this adds an additional safety net.

Also, the gem is actually about 10 lines of code, so not that much of a dependency burden. You could add it into the core if you would be willing to support this code (which I think is pointless here). If you read the code on github, you'll notice that there isn't much room for bugs. Also, the library is well tested.

In my opinion, it adds a rather easy-to-use safty net under some easy to break code paths.

request_store 1.0.6 breaks many tests.
http://www.redmine.org/builds/logs/build_trunk_postgresql_ruby-2.0.0-p0_1969.html

 1) Failure: test_api_should_accept_switch_user_header_for_admin_user(Redmine::ApiTest::AuthenticationTest) 
...

This is probably caused by this commit. In 1.0.6, the thread store is cleared after a request has finished (and not before a request started as before). That means, that after the middleware was passed on the way up again, User.current is not valid anymore.

The solution for this would be to fix the integration tests to not rely on an "old" User.current. In fact, not having this old value still set after a request is a good thing and solves many potential leakage issues.