Feature #31911

Update request_store gem to 1.4

Added by Go MAEDA 6 months ago. Updated 2 months ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:Jean-Philippe Lang% Done:

0%

Category:Gems support
Target version:4.1.0
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.

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


Related issues

Related to Redmine - Patch #16685: Introduce the request_store gem to hold User.current and ... Closed

Associated revisions

Revision 19373
Added by Go MAEDA 2 months ago

Update request_store gem to 1.4 (#31911).

Patch by Yuichi HARADA.

Revision 19380
Added by Jean-Philippe Lang 2 months ago

Merged r19373 to 4.1-stable (#31911).

History

#1 Updated by Go MAEDA 6 months ago

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

#2 Updated by Yuichi HARADA 3 months 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

#3 Updated by Go MAEDA 3 months ago

  • Target version set to Candidate for next major release

#4 Updated by Go MAEDA 2 months ago

  • File 31911.patchMagnifier 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.

#5 Updated by Go MAEDA 2 months ago

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

Committed the patch. Thank you for your contribution.

#6 Updated by Mischa The Evil 2 months 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?

#7 Updated by Jean-Philippe Lang 2 months ago

  • Status changed from Reopened to Closed

Merged.

Also available in: Atom PDF