Change CI settings for support Ruby3.0+ Rails6.1+ #357

willnet · 2024-01-31T09:59:28Z

Now, Ruby 2.7 is EOL, and Rails 6.0 is also EOL.

So, I changed the CI settings to support only those higher versions and made the changes necessary to pass the tests.

CI result is here.

Now, Ruby 2.7 is EOL, and Rails 6.0 is also EOL. So I changed the CI settings to support only those higher versions. Fix Sorcery#340

Since the supported version of rspec-rails depends on the version of rails we use, move the description to each Gemfile

``` Failures: 1) SorceryController with session timeout features with 'session_timeout_from_last_action' does not logout if there was activity Failure/Error: expect(response).to be_successful expected `#<ActionDispatch::TestResponse:0x0000557bab96c6d0 @mon_data=#<Monitor:0x0000557bab96c630>, @mon_data_...control={}, @request=#<ActionController::TestRequest GET "http://test.host/test_login" for 0.0.0.0>>.successful?` to return true, got false # ./spec/controllers/controller_session_timeout_spec.rb:146:in `block (4 levels) in <top (required)>' Finished in 2.52 seconds (files took 1.66 seconds to load) ``` [Starting with Rails 7.0, instance variables are reset between controller test requests](rails/rails#43735). This causes the second and subsequent requests to be judged as un-logged-in if there are no records in the DB. Putting a record in the DB fixes the failure.

@user

…cation fix failing tests like the following ``` 1) SorceryController using create_from supports nested attributes Failure/Error: @user = user_class.create_from_provider(provider_name, @user_hash[:uid], attrs, &block) #<User(id: integer, username: string, email: string, crypted_password: string, salt: string, created_at: datetime, updated_at: datetime, activation_state: string, activation_token: string, activation_token_expires_at: datetime, last_login_at: datetime, last_logout_at: datetime, last_activity_at: datetime, last_login_from_ip_address: string) (class)> received :create_from_provider with unexpected arguments expected: ("facebook", "123", {:username=>"Haifa, Israel"}) (keyword arguments) got: ("facebook", "123", {:username=>"Haifa, Israel"}) (options hash) # ./lib/sorcery/controller/submodules/external.rb:194:in `create_from' # ./spec/rails_app/app/controllers/sorcery_controller.rb:456:in `test_create_from_provider' # ./spec/controllers/controller_oauth2_spec.rb:44:in `block (3 levels) in <top (required)>' ```

A spec fails like the following in Rails 7.1 ``` 1) SorceryController when activated with sorcery #login when succeeds sets csrf token in session Failure/Error: expect(session[:_csrf_token]).not_to be_nil expected: not nil got: nil # ./spec/controllers/controller_spec.rb:68:in `block (5 levels) in <top (required)>' ``` Delete this because it didn't seem like a useful test.

joshbuker

Need to verify before merging.

joshbuker · 2024-03-08T18:08:42Z

gemfiles/rails_61.gemfile

@@ -3,5 +3,5 @@ source 'https://rubygems.org'
 gem 'rails', '~> 6.1.0'
 gem 'rails-controller-testing'
 gem 'sqlite3', '~> 1.4'
-
+gem 'rspec-rails', '>= 5.0'


>= version locks can cause issues, but I'm not concerned enough about it to block the PR.

sorcery.gemspec

spec/controllers/controller_session_timeout_spec.rb

joshbuker · 2024-03-08T18:11:04Z

spec/controllers/controller_spec.rb

-
-        it 'sets csrf token in session' do
-          expect(session[:_csrf_token]).not_to be_nil
-        end


Because this touches csrf, I'll need to verify before merging this.

This test does not fail, and should not be removed.

joshbuker

NGL, I can't find any mentions of changes to the csrf functionality that would cause the test to fail in Rails 7.1. Has me pretty spooked, but I don't have the time to investigate further and Rails 7.1 is largely broken in its current state for Sorcery v0 anyway.

Really need to finish that v1 rework, but it's been long enough that getting back into it is challenging.

joshbuker · 2024-03-08T18:20:14Z

spec/controllers/controller_spec.rb

-
-        it 'sets csrf token in session' do
-          expect(session[:_csrf_token]).not_to be_nil
-        end


This test does not fail, and should not be removed.

spec/controllers/controller_session_timeout_spec.rb

willnet · 2024-03-13T09:50:21Z

spec/controllers/controller_spec.rb

+        # NOTE: The lack of a CSRF token may mean that sessions will break
+        #       horribly for Sorcery when using Rails 7.1+. We shall see.
        it 'sets csrf token in session' do
+          if Gem::Version.new(Rails.version) >= Gem::Version.new('7.1')
+            pending 'Rails 7.1 is not including the csrf token in the session for unknown reasons'
+          end


@joshbuker Sorcery uses the form_authenticity_token method to assign values to session[:_csrf_token]. However, it seems that due to this commit in Rails 7.1, the behavior of the form_authenticity_token method has changed and it no longer assigns values to session[:_csrf_token].

If it's necessary to assign a value to session[:_csrf_token] during the execution of the login method, then we may need to revise our implementation. The question arises whether this is actually necessary. Rails seems to assign values to session[:_csrf_token] as needed, so there might not be a clear need for Sorcery to explicitly do this. There doesn't seem to be a specific reason mentioned in the commit where form_authenticity_token was introduced, making it hard to infer the rationale behind this implementation.

@joshbuker I did some additional research on this topic. The PR that introduced the form_authenticity_token method is here, and it seems it was merged to fix this issue (though it's mentioned as Fix for #150 in the PR, it seems like a typo and should probably be Fix for #250 based on the content).

However, calling form_authenticity_token during login doesn't resolve the issue mentioned in NoamB/sorcery#250, making this commit seem rather pointless. Moreover, invoking form_authenticity_token has led to a new issue being created, as seen in Using sorcery in an API-only rails app does not work · Issue #330 · Sorcery/sorcery, suggesting that it might be better to remove this implementation and related tests.

This commit is related to Sorcery#340. In Sorcery#357, we limited CI support to Ruby 3.0 and above, but in the gemspec, we still had dependencies set for older Ruby versions.

* Bump up required ruby version to 3.0.0 This commit is related to #340. In #357, we limited CI support to Ruby 3.0 and above, but in the gemspec, we still had dependencies set for older Ruby versions. * Update CHANGELOG.md

Fix Sorcery#330 As detailed in the URL below, the call to `form_authenticity_token` wasn't useful and was causing issues like Sorcery#330, so remove it. ref: - Sorcery#357 (comment) - Sorcery#357 (comment)

* Remove form_authenticity_token method Fix #330 As detailed in the URL below, the call to `form_authenticity_token` wasn't useful and was causing issues like #330, so remove it. ref: - #357 (comment) - #357 (comment) * Update CHANGELOG with removal of form_authenticity_token method

willnet added 6 commits January 31, 2024 18:23

Change CI settings for support Ruby3.0+ Rails6.1+

2591eb2

Now, Ruby 2.7 is EOL, and Rails 6.0 is also EOL. So I changed the CI settings to support only those higher versions. Fix Sorcery#340

Move rspec-rails development dependency to Gemfiles

1018b8b

Since the supported version of rspec-rails depends on the version of rails we use, move the description to each Gemfile

Add Changelog

23aacc1

hoangth-dipro mentioned this pull request Mar 5, 2024

290 eol upgrade lasted version nexway/sorcery#4

Merged

Merge branch 'master' into change-ruby-and-rails-support

f4b659d

joshbuker reviewed Mar 8, 2024

View reviewed changes

joshbuker added 4 commits March 8, 2024 10:28

Readd rspec-rails to dev dependencies for running tests locally

4b7b65b

Do not remove the csrf test

7c202b5

Gitignore new sqlite3 files

e39ce33

Add pending clause for Rails 7.1 for CSRF token

2d2f5cd

joshbuker approved these changes Mar 8, 2024

View reviewed changes

joshbuker merged commit 7a319c2 into Sorcery:master Mar 8, 2024

willnet deleted the change-ruby-and-rails-support branch March 13, 2024 08:34

willnet commented Mar 13, 2024

View reviewed changes

willnet mentioned this pull request Mar 14, 2024

Bump up required ruby version to 3.0.0 #369

Merged

willnet mentioned this pull request Apr 10, 2024

Remove form_authenticity_token method #371

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Change CI settings for support Ruby3.0+ Rails6.1+ #357

Change CI settings for support Ruby3.0+ Rails6.1+ #357

Uh oh!

willnet commented Jan 31, 2024 •

edited

Loading

Uh oh!

joshbuker left a comment

Uh oh!

joshbuker Mar 8, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

joshbuker Mar 8, 2024

Uh oh!

joshbuker Mar 8, 2024

Uh oh!

joshbuker left a comment

Uh oh!

joshbuker Mar 8, 2024

Uh oh!

Uh oh!

willnet Mar 13, 2024

Uh oh!

willnet Apr 10, 2024

Uh oh!

Uh oh!

Change CI settings for support Ruby3.0+ Rails6.1+ #357

Change CI settings for support Ruby3.0+ Rails6.1+ #357

Uh oh!

Conversation

willnet commented Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshbuker left a comment

Choose a reason for hiding this comment

Uh oh!

joshbuker Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joshbuker Mar 8, 2024

Choose a reason for hiding this comment

Uh oh!

joshbuker Mar 8, 2024

Choose a reason for hiding this comment

Uh oh!

joshbuker left a comment

Choose a reason for hiding this comment

Uh oh!

joshbuker Mar 8, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

willnet Mar 13, 2024

Choose a reason for hiding this comment

Uh oh!

willnet Apr 10, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

willnet commented Jan 31, 2024 •

edited

Loading

joshbuker Mar 8, 2024 •

edited

Loading