Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing compatibility with Rails 7 and API mode #5474

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

morenocarullo
Copy link

This PR addresses this issue #5443 .

The test coverage for it is not ideal yet, as we are working to find a good way to have API-mode tests properly integrated (quick hack of forcing it to true does not work).

@morenocarullo
Copy link
Author

Hey @carlosantoniodasilva , this is a first part of the work to allow API mode to work as well. See #5443.

@hms
Copy link

hms commented Mar 3, 2022

Why the hard lock to Rails 7+ ? The PR looks like it will work with Rails 6 without a hitch.

@morenocarullo
Copy link
Author

Why the hard lock to Rails 7+ ? The PR looks like it will work with Rails 6 without a hitch.

you are right: but in Rails 6 it would not have any effect because rack.session will be a valid one.

@dnpp73
Copy link

dnpp73 commented Mar 4, 2022

I think this is a good hack as bandaid. but I don't think this is a good solution. Some users may be using it in API mode and with sessions enabled. I think it would be broken from those users.

DisabledSessionError is useful information. because we can realize an implicit dependency on the "rack.session"

I think the problem is that devise implicitly depends on session.
following lines ...

How about providing an option called ignore_session? like this

Devise.setup do |config|
  config.ignore_session = true # default false
end

As a side note, there seems to be a similar discussion on warden's side.
wardencommunity/warden#201

@morenocarullo
Copy link
Author

I think this is a good hack as bandaid. but I don't think this is a good solution. Some users may be using it in API mode and with sessions enabled. I think it would be broken from those users.

DisabledSessionError is useful information. because we can realize an implicit dependency on the "rack.session"

I think the problem is that devise implicitly depends on session. following lines ...

* https://github.com/heartcombo/devise/blob/v4.8.1/lib/devise/controllers/sign_in_out.rb#L48

* https://github.com/heartcombo/devise/blob/v4.8.1/lib/devise/controllers/sign_in_out.rb#L68

* https://github.com/heartcombo/devise/blob/v4.8.1/lib/devise/test/controller_helpers.rb#L81

* https://github.com/heartcombo/devise/blob/v4.8.1/lib/devise/test/controller_helpers.rb#L96

How about providing an option called ignore_session? like this

Devise.setup do |config|
  config.ignore_session = true # default false
end

As a side note, there seems to be a similar discussion on warden's side. wardencommunity/warden#201

I agree with you. I believe that, before beginning a journey to the "right thing" it would be great to have some blessing from the mantainers, or it will result in a lot of wasted work.

@ghost
Copy link

ghost commented Mar 19, 2022

@morenocarullo I forked and merged in my Devise version for an API ( Rails 7, API Only, JWT Auth). Works like a charm, thanks for your insight.

@alhajrahmoun
Copy link

Getting

NoMethodError (undefined method `destroy' for {}:Devise::Controllers::Rails7ApiMode::FakeRackSession):

when trying to sign out. Any thoughts?

@internet-exploder
Copy link

internet-exploder commented Mar 20, 2022

Didn't work on rails 7.0.2.3 for me. Works only if comment "load_for_write" line in actionpack's gem at lib/action_dispatch/request/session.rb

      # Writes given value to given key of the session.
      def []=(key, value)
        # load_for_write!
        @delegate[key.to_s] = value
      end

I don't know if this is correct in any way.

module ActionDispatch
  class Request
    class Session
      def []=(key, value)
        @delegate[key.to_s] = value
      end
    end
  end
end

is it safe??

@heyapricot
Copy link

Worked for me. However I'm getting the following error when running rspec. What am I missing?

      LoadError:
        cannot load such file -- devise/controllers/rails7_api_mode

@justinshaw
Copy link

Is this likely to be merged any time soon? Or has anyone discovered an alternative for dealing with the problem this PR solves?

@alexjoseps
Copy link

Any news?

smui added a commit to redfast/devise that referenced this pull request Feb 16, 2023
@graveetone
Copy link

graveetone commented Apr 19, 2023

any updates on merging this solution or providing another one?

juanarias93 added a commit to wyeworks/finder that referenced this pull request Aug 26, 2023
`authenticate_user!`

  * This is a known issue reported here: heartcombo/devise#5443
  * There's also an open PR to devise for fixing it: heartcombo/devise#5474
  * If the PR gets merged eventually, this fix/patch won't be necessary
    anymore.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

10 participants