Project

General

Profile

Actions

Feature #31911

closed

Update request_store gem to 1.4

Added by Go MAEDA over 4 years ago. Updated over 4 years ago.

Status:
Closed
Priority:
Normal
Category:
Gems support
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:
Resolution:
Fixed

Description

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.

Show


Files

31911.patch (5.9 KB) 31911.patch Patch by Yuichi HARADA (#31911#note-2) Go MAEDA, 2019-12-14 08:54

Related issues

Related to Redmine - Patch #16685: Introduce the request_store gem to hold User.current and prevent data leakage in error messagesClosed

Actions
Related to Redmine - Defect #36336: Executing test helper "log_user" function behavior is different between Redmine 4.0 and >= 4.1Closed

Actions
Actions #1

Updated by Go MAEDA over 4 years ago

  • Related to Patch #16685: Introduce the request_store gem to hold User.current and prevent data leakage in error messages added
Actions #2

Updated by Yuichi HARADA over 4 years ago

https://www.redmine.org/issues/16685#note-14
Holger Just wrote:

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.

+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.

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
Actions #3

Updated by Go MAEDA over 4 years ago

  • Target version set to Candidate for next major release
Actions #4

Updated by Go MAEDA over 4 years ago

  • File 31911.patch 31911.patch added
  • Subject changed from Update request_store gem to Update request_store gem to 1.4
  • Target version changed from Candidate for next major release to 4.2.0

Setting the target version to 4.2.0.

Actions #5

Updated by Go MAEDA over 4 years ago

  • Status changed from New to Closed
  • Assignee set to Go MAEDA
  • Resolution set to Fixed

Committed the patch. Thank you for your contribution.

Actions #6

Updated by Mischa The Evil over 4 years ago

  • Status changed from Closed to Reopened
  • Assignee changed from Go MAEDA to Jean-Philippe Lang
  • Target version changed from 4.2.0 to 4.1.0

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?

Actions #7

Updated by Jean-Philippe Lang over 4 years ago

  • Status changed from Reopened to Closed

Merged.

Actions #8

Updated by Go MAEDA over 2 years ago

  • Related to Defect #36336: Executing test helper "log_user" function behavior is different between Redmine 4.0 and >= 4.1 added
Actions

Also available in: Atom PDF