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

Compatibility with Devise #1160

Closed
qortex opened this issue Oct 25, 2018 · 7 comments
Closed

Compatibility with Devise #1160

qortex opened this issue Oct 25, 2018 · 7 comments

Comments

@qortex
Copy link

qortex commented Oct 25, 2018

Steps to reproduce

Fresh install of devise & doorkeeper, for example following https://scotch.io/@jiggs/rails-api-doorkeeper-devise

I was getting devise to work with doorkeeper (classic: frontend auth to rails API with login/password first, gets a token, then speaks with API using token).

I struggled because whenever I used some of devise/RegistrationsController actions, it failed with an error message like "You need to sign in or sign up before continuing", even though "Bearer xxxx" was used in the header.

It turns out in Devise Registrations Controller, there is:

prepend_before_action :authenticate_scope!, only: [:edit, :update, :destroy]

which in turns is:

# Authenticates the current scope and gets the current resource from the session.
  def authenticate_scope!
    send(:"authenticate_#{resource_name}!", force: true)
    self.resource = send(:"current_#{resource_name}")
  end

The first call is the one failing, because it does not call Doorkeeper as far as I understand.
The second one, it's easy to make it work defining my own functions in the Application Controller:

  def current_resource_owner
    User.find(doorkeeper_token.resource_owner_id) if doorkeeper_token
  end

  def current_user
    current_resource_owner
  end

Thus, I overwrite the authenticate_scope! method in my own RegistrationsController, just removing the second line.

This way, everything works fine now.

Expected behavior

Should work as Wiki explains.

Actual behavior

Does not, because of above as far as I understand.

My question

Is my workaround safe? Is it the way to go? If so, is it a compatibility mismatch on Devise end, or Doorkeeper end?

If it is the way to go, I could go ahead and update the wiki (already did on https://github.com/doorkeeper-gem/doorkeeper/wiki/Using-Resource-Owner-Password-Credentials-flow because there lacked a test when :confirmable option is used in Devise), and/or post a bug report in Devise.

@nbulaj
Copy link
Member

nbulaj commented Nov 8, 2018

Hi @MicMicMon . I don't sure I can help you with Devise because I don't use it (or do it very rarely), especially with Doorkeeper. And looks like you already solved your problem, because how do you think Devise knows about current resource without your overridden method (also you can use any authorization / authentication library with Devise and you need to overload it's controllers to implement required behavior)? Also maybe you can contact an author of the original article..

@nbulaj
Copy link
Member

nbulaj commented Dec 4, 2018

Hi @MicMicMon . Does this heartcombo/devise#4950 solved your problem?

@qortex
Copy link
Author

qortex commented Dec 4, 2018

Hi @nbulaj
Thanks for the follow-up.
Actually no, I didn't have to modify the flash_message stuff, I guess it's bypassed since I use JSON. (initial issue for the one you linked was: heartcombo/devise#4947)

My guess is, we should just update the Wiki so that people following the process don't stumble on this. My concern was to make sure I don't break some security requirement with not running send(:"authenticate_#{resource_name}!", force: true) in my new code.

@nbulaj
Copy link
Member

nbulaj commented Jan 4, 2019

So can we close this issue, @MicMicMon ? Also could you please fix this in the Wiki?

@qortex
Copy link
Author

qortex commented Jan 25, 2019

Done, updated the wiki with my above fix. --> https://github.com/doorkeeper-gem/doorkeeper/wiki/Running-Doorkeeper-with-Devise

@qortex qortex closed this as completed Jan 25, 2019
@nbulaj
Copy link
Member

nbulaj commented Jan 25, 2019

Thanks @MicMicMon ! 👍

@pjmartorell
Copy link

Thanks @MicMicMon! I'm trying to run Doorkeeper + Devise + a custom devise strategy to authenticate with LDAP and I'm struggling a lot to make all the strategies work out of the box in a non-hacky way

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants