Vote #80250
完了Update request_store gem to 1.4
0%
説明
Redmine 4.0 and the current trunk depend on request_store gem 1.0.5 which was released in 2013. The reason why Redmine uses such an old version is that request_store 1.0.6 and later breaks some tests (see r13181).
However, we should not continue to use the very old version of request_store. We should make efforts to update the gem to the latest version.
journals
--------------------------------------------------------------------------------
https://www.redmine.org/issues/16685#note-14
Holger Just wrote:
> This is probably caused by "this commit":https://github.com/steveklabnik/request_store/commit/cf01d128ceb0425d4d652b95c4333afc9934290f. 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.
+1
The thread store is cleared after the request ends. Therefore, the value of @User.current@ using RequestStore is undefined.
https://github.com/steveklabnik/request_store/compare/v1.0.5...v1.0.6#diff-f0d601018bf0879f1221695eb92982bb
source:/trunk/app/models/user.rb#L818
I don't think it makes sense to validate @User.current@ after the request ends.
You can update request_store gem by changing the test as follows.
<pre><code class="diff">
diff --git a/test/integration/api_test/authentication_test.rb b/test/integration/api_test/authentication_test.rb
index 30fc4b351..0a4c12e00 100644
--- a/test/integration/api_test/authentication_test.rb
+++ b/test/integration/api_test/authentication_test.rb
@@ -29,7 +29,6 @@ class Redmine::ApiTest::AuthenticationTest < Redmine::ApiTest::Base
def test_api_should_deny_without_credentials
get '/users/current.xml'
assert_response 401
- assert_equal User.anonymous, User.current
assert response.headers.has_key?('WWW-Authenticate')
end
@@ -39,7 +38,6 @@ class Redmine::ApiTest::AuthenticationTest < Redmine::ApiTest::Base
end
get '/users/current.xml', :headers => credentials(user.login, 'my_password')
assert_response 200
- assert_equal user, User.current
end
def test_api_should_deny_http_basic_auth_using_username_and_wrong_password
@@ -48,7 +46,6 @@ class Redmine::ApiTest::AuthenticationTest < Redmine::ApiTest::Base
end
get '/users/current.xml', :headers => credentials(user.login, 'wrong_password')
assert_response 401
- assert_equal User.anonymous, User.current
end
def test_api_should_accept_http_basic_auth_using_api_key
@@ -56,7 +53,6 @@ class Redmine::ApiTest::AuthenticationTest < Redmine::ApiTest::Base
token = Token.create!(:user => user, :action => 'api')
get '/users/current.xml', :headers => credentials(token.value, 'X')
assert_response 200
- assert_equal user, User.current
end
def test_api_should_deny_http_basic_auth_using_wrong_api_key
@@ -64,7 +60,6 @@ class Redmine::ApiTest::AuthenticationTest < Redmine::ApiTest::Base
token = Token.create!(:user => user, :action => 'feeds') # not the API key
get '/users/current.xml', :headers => credentials(token.value, 'X')
assert_response 401
- assert_equal User.anonymous, User.current
end
def test_api_should_accept_auth_using_api_key_as_parameter
@@ -72,7 +67,6 @@ class Redmine::ApiTest::AuthenticationTest < Redmine::ApiTest::Base
token = Token.create!(:user => user, :action => 'api')
get "/users/current.xml?key=#{token.value}"
assert_response 200
- assert_equal user, User.current
end
def test_api_should_deny_auth_using_wrong_api_key_as_parameter
@@ -80,7 +74,6 @@ class Redmine::ApiTest::AuthenticationTest < Redmine::ApiTest::Base
token = Token.create!(:user => user, :action => 'feeds') # not the API key
get "/users/current.xml?key=#{token.value}"
assert_response 401
- assert_equal User.anonymous, User.current
end
def test_api_should_accept_auth_using_api_key_as_request_header
@@ -88,7 +81,6 @@ class Redmine::ApiTest::AuthenticationTest < Redmine::ApiTest::Base
token = Token.create!(:user => user, :action => 'api')
get "/users/current.xml", :headers => {'X-Redmine-API-Key' => token.value.to_s}
assert_response 200
- assert_equal user, User.current
end
def test_api_should_deny_auth_using_wrong_api_key_as_request_header
@@ -96,7 +88,6 @@ class Redmine::ApiTest::AuthenticationTest < Redmine::ApiTest::Base
token = Token.create!(:user => user, :action => 'feeds') # not the API key
get "/users/current.xml", :headers => {'X-Redmine-API-Key' => token.value.to_s}
assert_response 401
- assert_equal User.anonymous, User.current
end
def test_api_should_trigger_basic_http_auth_with_basic_authorization_header
@@ -136,7 +127,6 @@ class Redmine::ApiTest::AuthenticationTest < Redmine::ApiTest::Base
get '/users/current', :headers => {'X-Redmine-API-Key' => user.api_key, 'X-Redmine-Switch-User' => su.login}
assert_response :success
assert_select 'h2', :text => su.name
- assert_equal su, User.current
end
def test_api_should_respond_with_412_when_trying_to_switch_to_a_invalid_user
@@ -159,6 +149,5 @@ class Redmine::ApiTest::AuthenticationTest < Redmine::ApiTest::Base
get '/users/current', :headers => {'X-Redmine-API-Key' => user.api_key, 'X-Redmine-Switch-User' => su.login}
assert_response :success
assert_select 'h2', :text => user.name
- assert_equal user, User.current
end
end
diff --git a/test/integration/api_test/disabled_rest_api_test.rb b/test/integration/api_test/disabled_rest_api_test.rb
index 5eaedd978..a8a1139a8 100644
--- a/test/integration/api_test/disabled_rest_api_test.rb
+++ b/test/integration/api_test/disabled_rest_api_test.rb
@@ -44,11 +44,9 @@ class Redmine::ApiTest::DisabledRestApiTest < Redmine::ApiTest::Base
get "/news.xml?key=#{@token.value}"
assert_response :forbidden
- assert_equal User.anonymous, User.current
get "/news.json?key=#{@token.value}"
assert_response :forbidden
- assert_equal User.anonymous, User.current
end
def test_with_valid_username_password_http_authentication
@@ -58,11 +56,9 @@ class Redmine::ApiTest::DisabledRestApiTest < Redmine::ApiTest::Base
get "/news.xml", :headers => credentials(@user.login, 'my_password')
assert_response :forbidden
- assert_equal User.anonymous, User.current
get "/news.json", :headers => credentials(@user.login, 'my_password')
assert_response :forbidden
- assert_equal User.anonymous, User.current
end
def test_with_valid_token_http_authentication
@@ -71,10 +67,8 @@ class Redmine::ApiTest::DisabledRestApiTest < Redmine::ApiTest::Base
get "/news.xml", :headers => credentials(@token.value, 'X')
assert_response :forbidden
- assert_equal User.anonymous, User.current
get "/news.json", :headers => credentials(@token.value, 'X')
assert_response :forbidden
- assert_equal User.anonymous, User.current
end
end
</code></pre>
--------------------------------------------------------------------------------
--------------------------------------------------------------------------------
Setting the target version to 4.2.0.
--------------------------------------------------------------------------------
Committed the patch. Thank you for your contribution.
--------------------------------------------------------------------------------
If there are no further objections and/or side-effects, I'd like to have this change shipped in the upcoming release.
What do you think?
--------------------------------------------------------------------------------
Merged.
--------------------------------------------------------------------------------
--------------------------------------------------------------------------------
related_issues
relates,Closed,16685,Introduce the request_store gem to hold User.current and prevent data leakage in error messages
relates,Closed,36336,Executing test helper "log_user" function behavior is different between Redmine 4.0 and >= 4.1