Feature #31911: Update request_store gem to 1.4 - Redmine

Actions

Copy link

Feature #31911

closed

Update request_store gem to 1.4

Added by Go MAEDA over 5 years ago. Updated about 5 years ago.

Status:

Closed

Priority:

Normal

Assignee:

Jean-Philippe Lang

Category:

Gems support

Target version:

4.1.0

Start date:

Due date:

% Done:

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 Hide

$ bin/rails test test/integration/api_test/authentication_test.rb
Run options: --seed 46704

# Running:

....F

Failure:
Redmine::ApiTest::AuthenticationTest#test_api_should_not_accept_switch_user_header_for_non_admin_user [/Users/maeda/redmines/redmine-trunk/test/integration/api_test/authentication_test.rb:162]:
--- expected
+++ actual
@@ -1 +1 @@
-#<User id: 2, login: "jsmith", hashed_password: "bfbe06043353a677d0215b26a5800d128d5413bc", firstname: "John", lastname: "Smith", admin: false, status: 1, last_login_on: "2006-07-19 20:42:15", language: "en", auth_source_id: nil, created_on: "2006-07-19 17:32:09", updated_on: "2006-07-19 20:42:15", type: "User", identity_url: nil, mail_notification: "all", salt: "67eb4732624d5a7753dcea7ce0bb7d7d", must_change_passwd: false, passwd_changed_on: nil>
+#<AnonymousUser id: 6, login: "", hashed_password: "1", firstname: "", lastname: "Anonymous", admin: false, status: 0, last_login_on: nil, language: "", auth_source_id: nil, created_on: "2006-07-19 17:33:19", updated_on: "2006-07-19 17:33:19", type: "AnonymousUser", identity_url: nil, mail_notification: "only_my_events", salt: nil, must_change_passwd: false, passwd_changed_on: nil>

bin/rails test test/integration/api_test/authentication_test.rb:155

F

Failure:
Redmine::ApiTest::AuthenticationTest#test_api_should_accept_auth_using_api_key_as_request_header [/Users/maeda/redmines/redmine-trunk/test/integration/api_test/authentication_test.rb:91]:
--- expected
+++ actual
@@ -1 +1 @@
-#<User id: 14, login: "user2", hashed_password: "", firstname: "Bob", lastname: "Doe", admin: false, status: 1, last_login_on: nil, language: "", auth_source_id: nil, created_on: "2019-08-16 02:19:58", updated_on: "2019-08-16 02:19:58", type: "User", identity_url: nil, mail_notification: "only_my_events", salt: nil, must_change_passwd: false, passwd_changed_on: nil>
+#<AnonymousUser id: 6, login: "", hashed_password: "1", firstname: "", lastname: "Anonymous", admin: false, status: 0, last_login_on: nil, language: "", auth_source_id: nil, created_on: "2006-07-19 17:33:19", updated_on: "2006-07-19 17:33:19", type: "AnonymousUser", identity_url: nil, mail_notification: "only_my_events", salt: nil, must_change_passwd: false, passwd_changed_on: nil>

bin/rails test test/integration/api_test/authentication_test.rb:86

F

Failure:
Redmine::ApiTest::AuthenticationTest#test_api_should_accept_http_basic_auth_using_api_key [/Users/maeda/redmines/redmine-trunk/test/integration/api_test/authentication_test.rb:59]:
--- expected
+++ actual
@@ -1 +1 @@
-#<User id: 14, login: "user3", hashed_password: "", firstname: "Bob", lastname: "Doe", admin: false, status: 1, last_login_on: nil, language: "", auth_source_id: nil, created_on: "2019-08-16 02:19:58", updated_on: "2019-08-16 02:19:58", type: "User", identity_url: nil, mail_notification: "only_my_events", salt: nil, must_change_passwd: false, passwd_changed_on: nil>
+#<AnonymousUser id: 6, login: "", hashed_password: "1", firstname: "", lastname: "Anonymous", admin: false, status: 0, last_login_on: nil, language: "", auth_source_id: nil, created_on: "2006-07-19 17:33:19", updated_on: "2006-07-19 17:33:19", type: "AnonymousUser", identity_url: nil, mail_notification: "only_my_events", salt: nil, must_change_passwd: false, passwd_changed_on: nil>

bin/rails test test/integration/api_test/authentication_test.rb:54

F

Failure:
Redmine::ApiTest::AuthenticationTest#test_api_should_accept_http_basic_auth_using_username_and_password [/Users/maeda/redmines/redmine-trunk/test/integration/api_test/authentication_test.rb:42]:
--- expected
+++ actual
@@ -1 +1 @@
-#<User id: 14, login: "user4", hashed_password: "e605300cad931688aa35d19fb77b4d36d452e786", firstname: "Bob", lastname: "Doe", admin: false, status: 1, last_login_on: nil, language: "", auth_source_id: nil, created_on: "2019-08-16 02:19:58", updated_on: "2019-08-16 02:19:58", type: "User", identity_url: nil, mail_notification: "only_my_events", salt: "a1dad2185f1cc71b310b8d33eeca6053", must_change_passwd: false, passwd_changed_on: "2019-08-16 02:19:58">
+#<AnonymousUser id: 6, login: "", hashed_password: "1", firstname: "", lastname: "Anonymous", admin: false, status: 0, last_login_on: nil, language: "", auth_source_id: nil, created_on: "2006-07-19 17:33:19", updated_on: "2006-07-19 17:33:19", type: "AnonymousUser", identity_url: nil, mail_notification: "only_my_events", salt: nil, must_change_passwd: false, passwd_changed_on: nil>

bin/rails test test/integration/api_test/authentication_test.rb:36

.F

Failure:
Redmine::ApiTest::AuthenticationTest#test_api_should_accept_switch_user_header_for_admin_user [/Users/maeda/redmines/redmine-trunk/test/integration/api_test/authentication_test.rb:139]:
--- expected
+++ actual
@@ -1 +1 @@
-#<User id: 4, login: "rhill", hashed_password: "9e4dd7eeb172c12a0691a6d9d3a269f7e9fe671b", firstname: "Robert", lastname: "Hill", admin: false, status: 1, last_login_on: nil, language: "en", auth_source_id: nil, created_on: "2006-07-19 17:34:07", updated_on: "2006-07-19 17:34:07", type: "User", identity_url: nil, mail_notification: "all", salt: "3126f764c3c5ac61cbfc103f25f934cf", must_change_passwd: false, passwd_changed_on: nil>
+#<AnonymousUser id: 6, login: "", hashed_password: "1", firstname: "", lastname: "Anonymous", admin: false, status: 0, last_login_on: nil, language: "", auth_source_id: nil, created_on: "2006-07-19 17:33:19", updated_on: "2006-07-19 17:33:19", type: "AnonymousUser", identity_url: nil, mail_notification: "only_my_events", salt: nil, must_change_passwd: false, passwd_changed_on: nil>

bin/rails test test/integration/api_test/authentication_test.rb:132

.F

Failure:
Redmine::ApiTest::AuthenticationTest#test_api_should_accept_auth_using_api_key_as_parameter [/Users/maeda/redmines/redmine-trunk/test/integration/api_test/authentication_test.rb:75]:
--- expected
+++ actual
@@ -1 +1 @@
-#<User id: 14, login: "user6", hashed_password: "", firstname: "Bob", lastname: "Doe", admin: false, status: 1, last_login_on: nil, language: "", auth_source_id: nil, created_on: "2019-08-16 02:19:59", updated_on: "2019-08-16 02:19:59", type: "User", identity_url: nil, mail_notification: "only_my_events", salt: nil, must_change_passwd: false, passwd_changed_on: nil>
+#<AnonymousUser id: 6, login: "", hashed_password: "1", firstname: "", lastname: "Anonymous", admin: false, status: 0, last_login_on: nil, language: "", auth_source_id: nil, created_on: "2006-07-19 17:33:19", updated_on: "2006-07-19 17:33:19", type: "AnonymousUser", identity_url: nil, mail_notification: "only_my_events", salt: nil, must_change_passwd: false, passwd_changed_on: nil>

bin/rails test test/integration/api_test/authentication_test.rb:70

.....

Finished in 4.439638s, 3.8291 runs/s, 8.5593 assertions/s.
17 runs, 38 assertions, 6 failures, 0 errors, 0 skips

Files

31911.patch (5.9 KB) 31911.patch

Patch by Yuichi HARADA (#31911#note-2)

Go MAEDA, 2019-12-14 08:54

Related issues

Actions

Copy link

Updated by Go MAEDA over 5 years ago

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

Actions

Copy link

Updated by Yuichi HARADA about 5 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

Copy link

Updated by Go MAEDA about 5 years ago

Target version set to Candidate for next major release

Actions

Copy link

Updated by Go MAEDA about 5 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

Copy link