doorkeeper-gem / doorkeeper

Doorkeeper is an OAuth 2 provider for Ruby on Rails / Grape.
https://doorkeeper.gitbook.io/guides/
MIT License
5.32k stars 1.07k forks source link

`Doorkeeper::AccessToken.find_or_create_for` with empty scopes raises NoMethodError #1699

Closed pdany1116 closed 2 months ago

pdany1116 commented 6 months ago

Steps to reproduce

Doorkeeper::AccessToken.find_or_create_for raises NoMethodError when searching with empty scopes.

application = Doorkeeper::Application.create(name: "test_application") # => Doorkeeper::Application
Doorkeeper::AccessToken.create_for(application: application, resource_owner: 42, scopes: 'public') # => Doorkeeper::AccessToken
Doorkeeper::AccessToken.find_or_create_for(application: application, resource_owner: 42, scopes: '') # => NoMethodError

Expected behavior

A new Doorkeeper::AccessToken should be created with empty scopes.

Actual behavior

NoMethodError: undefined method `sort' for "":String
from /usr/local/bundle/gems/doorkeeper-5.6.6/lib/doorkeeper/models/access_token_mixin.rb:155:in `scopes_match?'

System configuration

Ruby version 3.0.6 Doorkeeper version 5.6.6

pdany1116 commented 6 months ago

Hi, thanks for this awesome and useful gem! I can look into the issue and propose some tests + fix. But before getting to work, I want to make sure this is a valid case and I am not missing anything.

Thank you!

nbulaj commented 6 months ago

Hey @pdany1116 . Try to use Doorkeeper::OAuth::Scopes.from_string("") instead.

Also even plain one works without issues for me:

image

Looks like depends from configuration, but try to use above advise.

UPD: yes, requires reuse_access_token config option to be enabled

pdany1116 commented 6 months ago

Indeed, I have the reuse_access_token config option enabled.

Works with Doorkeeper::OAuth::Scopes.from_string(""), thanks!

nbulaj commented 6 months ago

Can we close this issue? Or do you think it makes sense to add some extra protection into #find_or_create_for method?

def find_or_create_for(application:, resource_owner:, scopes:, **token_attributes)
  scopes = Doorkeeper::OAuth::Scopes.from_string(scopes) if scopes.is_a?(String)
  # ...
end
pdany1116 commented 6 months ago

I think it makes sense to add extra protection, I was not expecting this to be the problem, maybe other devs will encounter the same issue and be confused.

I would like to contribute if possible 😄

nbulaj commented 6 months ago

Yeah sure, will be great @pdany1116 , thanks a lot! Please refer CONTRIBUTING guide when open a PR :pray: