Actions
Feature #31911
closed
Update request_store gem to 1.4
Added by Go MAEDA over 5 years ago. Updated almost 5 years ago.
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.
$ 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
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
Updated by Yuichi HARADA almost 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.currentis 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
Updated by Go MAEDA almost 5 years ago
- Target version set to Candidate for next major release
Updated by Go MAEDA almost 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.
Updated by Go MAEDA almost 5 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.
Updated by Mischa The Evil almost 5 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?
Updated by
Updated by Go MAEDA almost 3 years ago
- Related to Defect #36336: Executing test helper "log_user" function behavior is different between Redmine 4.0 and >= 4.1 added
Actions