Doorkeeper: Getting internal error 500 instead of 401 for invalid credentials

[esnrRahman]

Steps to reproduce

• Return false from resource_owner_from_credentials code block in initializer/doorkeeper.rb

NOTE: The call works successfully and I get the access_token if the credentials are correct and user.authenticate returns the user. The call fails only on invalid credentials or when false is returned. Using the POST endpoint /api/v1/oauth/token

Also used Doorkeeper 4.4.3 in the past and it follows the expected behaviour of getting 401

============ BACKTRACE ===========

[2019-12-30T18:50:55.050423 #62560]  INFO -- : Started POST "/api/v1/oauth/token" for 127.0.0.1 at 2019-12-30 18:50:55 -0500
D, [2019-12-30T18:50:55.151261 #62560] DEBUG -- :    (1.0ms)  SELECT "schema_migrations"."version" FROM "schema_migrations" ORDER BY "schema_migrations"."version" ASC
I, [2019-12-30T18:50:55.184290 #62560]  INFO -- : Processing by Doorkeeper::TokensController#create as HTML
I, [2019-12-30T18:50:55.184353 #62560]  INFO -- :   Parameters: {"grant_type"=>"password", "username"=>"[email protected]", "password"=>"[FILTERED]"}
D, [2019-12-30T18:50:55.195573 #62560] DEBUG -- :   User Load (0.6ms)  SELECT "users".* FROM "users" WHERE "users"."email" = $1 LIMIT $2  [["email", "[email protected]"], ["LIMIT", 1]]
D, [2019-12-30T18:50:55.195977 #62560] DEBUG -- :   ↳ app/repositories/base_repository.rb:22:in `find_by'
I, [2019-12-30T18:50:55.197024 #62560]  INFO -- : Completed 500 Internal Server Error in 13ms (ActiveRecord: 3.6ms | Allocations: 4937)


F, [2019-12-30T18:50:55.197635 #62560] FATAL -- :
NoMethodError (undefined method `id' for false:FalseClass):

doorkeeper (5.2.3) lib/doorkeeper/oauth/password_access_token_request.rb:28:in `before_successful_response'
doorkeeper (5.2.3) lib/doorkeeper/oauth/base_request.rb:14:in `authorize'
doorkeeper (5.2.3) lib/doorkeeper/request/strategy.rb:8:in `authorize'
doorkeeper (5.2.3) app/controllers/doorkeeper/tokens_controller.rb:93:in `authorize_response'
doorkeeper (5.2.3) app/controllers/doorkeeper/tokens_controller.rb:6:in `create'
actionpack (6.0.0) lib/action_controller/metal/basic_implicit_render.rb:6:in `send_action'
actionpack (6.0.0) lib/abstract_controller/base.rb:196:in `process_action'
actionpack (6.0.0) lib/action_controller/metal/rendering.rb:30:in `process_action'
actionpack (6.0.0) lib/abstract_controller/callbacks.rb:42:in `block in process_action'
activesupport (6.0.0) lib/active_support/callbacks.rb:135:in `run_callbacks'
actionpack (6.0.0) lib/abstract_controller/callbacks.rb:41:in `process_action'
actionpack (6.0.0) lib/action_controller/metal/rescue.rb:22:in `process_action'
actionpack (6.0.0) lib/action_controller/metal/instrumentation.rb:33:in `block in process_action'
activesupport (6.0.0) lib/active_support/notifications.rb:180:in `block in instrument'
activesupport (6.0.0) lib/active_support/notifications/instrumenter.rb:24:in `instrument'
activesupport (6.0.0) lib/active_support/notifications.rb:180:in `instrument'
actionpack (6.0.0) lib/action_controller/metal/instrumentation.rb:32:in `process_action'
actionpack (6.0.0) lib/action_controller/metal/params_wrapper.rb:245:in `process_action'
activerecord (6.0.0) lib/active_record/railties/controller_runtime.rb:27:in `process_action'
actionpack (6.0.0) lib/abstract_controller/base.rb:136:in `process'
actionpack (6.0.0) lib/action_controller/metal.rb:191:in `dispatch'
actionpack (6.0.0) lib/action_controller/metal.rb:252:in `dispatch'
actionpack (6.0.0) lib/action_dispatch/routing/route_set.rb:51:in `dispatch'
actionpack (6.0.0) lib/action_dispatch/routing/route_set.rb:33:in `serve'
actionpack (6.0.0) lib/action_dispatch/journey/router.rb:49:in `block in serve'
actionpack (6.0.0) lib/action_dispatch/journey/router.rb:32:in `each'
actionpack (6.0.0) lib/action_dispatch/journey/router.rb:32:in `serve'
actionpack (6.0.0) lib/action_dispatch/routing/route_set.rb:837:in `call'
rack (2.0.8) lib/rack/etag.rb:25:in `call'
rack (2.0.8) lib/rack/conditional_get.rb:38:in `call'
rack (2.0.8) lib/rack/head.rb:12:in `call'
activerecord (6.0.0) lib/active_record/migration.rb:567:in `call'
actionpack (6.0.0) lib/action_dispatch/middleware/callbacks.rb:27:in `block in call'
activesupport (6.0.0) lib/active_support/callbacks.rb:101:in `run_callbacks'
actionpack (6.0.0) lib/action_dispatch/middleware/callbacks.rb:26:in `call'
actionpack (6.0.0) lib/action_dispatch/middleware/executor.rb:14:in `call'
actionpack (6.0.0) lib/action_dispatch/middleware/actionable_exceptions.rb:17:in `call'
actionpack (6.0.0) lib/action_dispatch/middleware/debug_exceptions.rb:32:in `call'
actionpack (6.0.0) lib/action_dispatch/middleware/show_exceptions.rb:33:in `call'
railties (6.0.0) lib/rails/rack/logger.rb:38:in `call_app'
railties (6.0.0) lib/rails/rack/logger.rb:28:in `call'
actionpack (6.0.0) lib/action_dispatch/middleware/remote_ip.rb:81:in `call'
actionpack (6.0.0) lib/action_dispatch/middleware/request_id.rb:27:in `call'
rack (2.0.8) lib/rack/runtime.rb:22:in `call'
activesupport (6.0.0) lib/active_support/cache/strategy/local_cache_middleware.rb:29:in `call'
actionpack (6.0.0) lib/action_dispatch/middleware/executor.rb:14:in `call'
actionpack (6.0.0) lib/action_dispatch/middleware/static.rb:126:in `call'
rack (2.0.8) lib/rack/sendfile.rb:111:in `call'
actionpack (6.0.0) lib/action_dispatch/middleware/host_authorization.rb:83:in `call'
rack-cors (1.0.3) lib/rack/cors.rb:95:in `call'
railties (6.0.0) lib/rails/engine.rb:526:in `call'
puma (3.12.1) lib/puma/configuration.rb:227:in `call'
puma (3.12.1) lib/puma/server.rb:660:in `handle_request'
puma (3.12.1) lib/puma/server.rb:474:in `process_client'
puma (3.12.1) lib/puma/server.rb:334:in `block in run'
puma (3.12.1) lib/puma/thread_pool.rb:135:in `block in spawn_thread'
I, [2019-12-30T18:50:55.351427 #62560]  INFO -- :   Rendering /Users/ehsan/.rvm/gems/ruby-2.6.5@lawdesh-backend/gems/actionpack-6.0.0/lib/action_dispatch/middleware/templates/rescues/diagnostics.html.erb within rescues/layout
I, [2019-12-30T18:50:55.353532 #62560]  INFO -- :   Rendered /Users/ehsan/.rvm/gems/ruby-2.6.5@lawdesh-backend/gems/actionpack-6.0.0/lib/action_dispatch/middleware/templates/rescues/_actions.html.erb (Duration: 0.4ms | Allocations: 189)
I, [2019-12-30T18:50:55.355913 #62560]  INFO -- :   Rendered /Users/ehsan/.rvm/gems/ruby-2.6.5@lawdesh-backend/gems/actionpack-6.0.0/lib/action_dispatch/middleware/templates/rescues/_source.html.erb (Duration: 1.9ms | Allocations: 2137)
I, [2019-12-30T18:50:55.357942 #62560]  INFO -- :   Rendered /Users/ehsan/.rvm/gems/ruby-2.6.5@lawdesh-backend/gems/actionpack-6.0.0/lib/action_dispatch/middleware/templates/rescues/_trace.html.erb (Duration: 1.4ms | Allocations: 1593)
I, [2019-12-30T18:50:55.358512 #62560]  INFO -- :   Rendering /Users/ehsan/.rvm/gems/ruby-2.6.5@lawdesh-backend/gems/actionpack-6.0.0/lib/action_dispatch/middleware/templates/rescues/_request_and_response.html.erb
I, [2019-12-30T18:50:55.359259 #62560]  INFO -- :   Rendered /Users/ehsan/.rvm/gems/ruby-2.6.5@lawdesh-backend/gems/actionpack-6.0.0/lib/action_dispatch/middleware/templates/rescues/_request_and_response.html.erb (Duration: 0.7ms | Allocations: 457)
I, [2019-12-30T18:50:55.359331 #62560]  INFO -- :   Rendered /Users/ehsan/.rvm/gems/ruby-2.6.5@lawdesh-backend/gems/actionpack-6.0.0/lib/action_dispatch/middleware/templates/rescues/diagnostics.html.erb within rescues/layout (Duration: 7.8ms | Allocations: 6135)

=================================

Expected behavior

Should get 401 (:unauthorized)

Actual behavior

Getting 500 (internal error)

System configuration

Mac OS mojave 10.14.2

Doorkeeper initializer:

# config/initializers/doorkeeper.rb
# frozen_string_literal: true

Doorkeeper.configure do
  orm :active_record

  resource_owner_from_credentials do |routes|
    user = User.find_by({email: params[:username]})
    if user.nil?
      false
    else
      user.authenticate(params[:password]) # Using bcrypt to authenticate
    end
  end

  api_only

  base_controller 'DoorkeeperController'

  grant_flows %w[password]
end

module CustomTokenResponse
  def body
    user = User.find_by({id: @token.resource_owner_id})
    additional_data = UserSerializer.new(user).serialized_json # using fastjson_api to serialize data
    super.merge(JSON.parse(additional_data))
  end
end

Doorkeeper::OAuth::TokenResponse.send :prepend, CustomTokenResponse

Ruby version: ``
Ruby version 2.6.5
Rails 6.0.0
Gemfile.lock:

  remote: https://rubygems.org/
  specs:
    aasm (5.0.6)
      concurrent-ruby (~> 1.0)
    actioncable (6.0.0)
      actionpack (= 6.0.0)
      nio4r (~> 2.0)
      websocket-driver (>= 0.6.1)
    actionmailbox (6.0.0)
      actionpack (= 6.0.0)
      activejob (= 6.0.0)
      activerecord (= 6.0.0)
      activestorage (= 6.0.0)
      activesupport (= 6.0.0)
      mail (>= 2.7.1)
    actionmailer (6.0.0)
      actionpack (= 6.0.0)
      actionview (= 6.0.0)
      activejob (= 6.0.0)
      mail (~> 2.5, >= 2.5.4)
      rails-dom-testing (~> 2.0)
    actionpack (6.0.0)
      actionview (= 6.0.0)
      activesupport (= 6.0.0)
      rack (~> 2.0)
      rack-test (>= 0.6.3)
      rails-dom-testing (~> 2.0)
      rails-html-sanitizer (~> 1.0, >= 1.2.0)
    actiontext (6.0.0)
      actionpack (= 6.0.0)
      activerecord (= 6.0.0)
      activestorage (= 6.0.0)
      activesupport (= 6.0.0)
      nokogiri (>= 1.8.5)
    actionview (6.0.0)
      activesupport (= 6.0.0)
      builder (~> 3.1)
      erubi (~> 1.4)
      rails-dom-testing (~> 2.0)
      rails-html-sanitizer (~> 1.1, >= 1.2.0)
    activejob (6.0.0)
      activesupport (= 6.0.0)
      globalid (>= 0.3.6)
    activemodel (6.0.0)
      activesupport (= 6.0.0)
    activerecord (6.0.0)
      activemodel (= 6.0.0)
      activesupport (= 6.0.0)
    activestorage (6.0.0)
      actionpack (= 6.0.0)
      activejob (= 6.0.0)
      activerecord (= 6.0.0)
      marcel (~> 0.3.1)
    activesupport (6.0.0)
      concurrent-ruby (~> 1.0, >= 1.0.2)
      i18n (>= 0.7, < 2)
      minitest (~> 5.1)
      tzinfo (~> 1.1)
      zeitwerk (~> 2.1, >= 2.1.8)
    ast (2.4.0)
    awesome_print (1.8.0)
    bcrypt (3.1.13)
    bootsnap (1.4.5)
      msgpack (~> 1.0)
    brakeman (4.7.1)
    builder (3.2.3)
    byebug (11.0.1)
    colorize (0.8.1)
    concurrent-ruby (1.1.5)
    connection_pool (2.2.2)
    crass (1.0.5)
    docile (1.3.2)
    doorkeeper (5.2.1)
      railties (>= 5)
    dotenv (2.7.5)
    dotenv-rails (2.7.5)
      dotenv (= 2.7.5)
      railties (>= 3.2, < 6.1)
    erubi (1.9.0)
    factory_bot (5.1.1)
      activesupport (>= 4.2.0)
    factory_bot_rails (5.1.1)
      factory_bot (~> 5.1.0)
      railties (>= 4.2.0)
    faker (2.6.0)
      i18n (>= 1.6, < 1.8)
    fast_jsonapi (1.5)
      activesupport (>= 4.2)
    ffi (1.11.1)
    globalid (0.4.2)
      activesupport (>= 4.2.0)
    i18n (1.7.0)
      concurrent-ruby (~> 1.0)
    interactor (3.1.1)
    jaro_winkler (1.5.4)
    json (2.2.0)
    listen (3.1.5)
      rb-fsevent (~> 0.9, >= 0.9.4)
      rb-inotify (~> 0.9, >= 0.9.7)
      ruby_dep (~> 1.2)
    loofah (2.3.1)
      crass (~> 1.0.2)
      nokogiri (>= 1.5.9)
    mail (2.7.1)
      mini_mime (>= 0.1.1)
    marcel (0.3.3)
      mimemagic (~> 0.3.2)
    metaclass (0.0.4)
    method_source (0.9.2)
    mimemagic (0.3.3)
    mini_mime (1.0.2)
    mini_portile2 (2.4.0)
    minitest (5.12.2)
    mocha (1.9.0)
      metaclass (~> 0.0.1)
    msgpack (1.3.1)
    nio4r (2.5.2)
    nokogiri (1.10.4)
      mini_portile2 (~> 2.4.0)
    parallel (1.18.0)
    parser (2.6.5.0)
      ast (~> 2.4.0)
    pg (1.1.4)
    power_assert (1.1.3)
    puma (3.12.1)
    pundit (2.1.0)
      activesupport (>= 3.0.0)
    rack (2.0.7)
    rack-cors (1.0.3)
    rack-protection (2.0.7)
      rack
    rack-test (1.1.0)
      rack (>= 1.0, < 3)
    rails (6.0.0)
      actioncable (= 6.0.0)
      actionmailbox (= 6.0.0)
      actionmailer (= 6.0.0)
      actionpack (= 6.0.0)
      actiontext (= 6.0.0)
      actionview (= 6.0.0)
      activejob (= 6.0.0)
      activemodel (= 6.0.0)
      activerecord (= 6.0.0)
      activestorage (= 6.0.0)
      activesupport (= 6.0.0)
      bundler (>= 1.3.0)
      railties (= 6.0.0)
      sprockets-rails (>= 2.0.0)
    rails-dom-testing (2.0.3)
      activesupport (>= 4.2.0)
      nokogiri (>= 1.6)
    rails-html-sanitizer (1.3.0)
      loofah (~> 2.3)
    railties (6.0.0)
      actionpack (= 6.0.0)
      activesupport (= 6.0.0)
      method_source
      rake (>= 0.8.7)
      thor (>= 0.20.3, < 2.0)
    rainbow (3.0.0)
    rake (13.0.0)
    rb-fsevent (0.10.3)
    rb-inotify (0.10.0)
      ffi (~> 1.0)
    redis (4.1.3)
    redis-actionpack (5.1.0)
      actionpack (>= 4.0, < 7)
      redis-rack (>= 1, < 3)
      redis-store (>= 1.1.0, < 2)
    redis-activesupport (5.2.0)
      activesupport (>= 3, < 7)
      redis-store (>= 1.3, < 2)
    redis-rack (2.0.6)
      rack (>= 1.5, < 3)
      redis-store (>= 1.2, < 2)
    redis-rails (5.0.2)
      redis-actionpack (>= 5.0, < 6)
      redis-activesupport (>= 5.0, < 6)
      redis-store (>= 1.2, < 2)
    redis-store (1.8.1)
      redis (>= 4, < 5)
    rubocop (0.76.0)
      jaro_winkler (~> 1.5.1)
      parallel (~> 1.10)
      parser (>= 2.6)
      rainbow (>= 2.2.2, < 4.0)
      ruby-progressbar (~> 1.7)
      unicode-display_width (>= 1.4.0, < 1.7)
    rubocop-rails (2.3.2)
      rack (>= 1.1)
      rubocop (>= 0.72.0)
    ruby-progressbar (1.10.1)
    ruby_dep (1.5.0)
    seed-fu (2.3.9)
      activerecord (>= 3.1)
      activesupport (>= 3.1)
    shoulda (3.5.0)
      shoulda-context (~> 1.0, >= 1.0.1)
      shoulda-matchers (>= 1.4.1, < 3.0)
    shoulda-context (1.2.2)
    shoulda-matchers (2.8.0)
      activesupport (>= 3.0.0)
    sidekiq (6.0.3)
      connection_pool (>= 2.2.2)
      rack (>= 2.0.0)
      rack-protection (>= 2.0.0)
      redis (>= 4.1.0)
    simplecov (0.17.1)
      docile (~> 1.1)
      json (>= 1.8, < 3)
      simplecov-html (~> 0.10.0)
    simplecov-html (0.10.2)
    spring (2.1.0)
    spring-watcher-listen (2.0.1)
      listen (>= 2.7, < 4.0)
      spring (>= 1.2, < 3.0)
    sprockets (4.0.0)
      concurrent-ruby (~> 1.0)
      rack (> 1, < 3)
    sprockets-rails (3.2.1)
      actionpack (>= 4.0)
      activesupport (>= 4.0)
      sprockets (>= 3.0.0)
    test-unit (3.2.9)
      power_assert
    thor (0.20.3)
    thread_safe (0.3.6)
    tzinfo (1.2.5)
      thread_safe (~> 0.1)
    unicode-display_width (1.6.0)
    websocket-driver (0.7.1)
      websocket-extensions (>= 0.1.0)
    websocket-extensions (0.1.4)
    zeitwerk (2.2.0)

PLATFORMS
  ruby

DEPENDENCIES
  aasm
  awesome_print
  bcrypt (~> 3.1.7)
  bootsnap (>= 1.4.2)
  brakeman
  byebug
  colorize
  doorkeeper
  dotenv-rails
  factory_bot_rails
  faker
  fast_jsonapi
  interactor
  listen (>= 3.0.5, < 3.2)
  mocha
  pg (>= 0.18, < 2.0)
  puma (~> 3.11)
  pundit
  rack-cors
  rails (~> 6.0.0)
  redis-rails
  rubocop-rails
  seed-fu (~> 2.3)
  shoulda (~> 3.5)
  shoulda-matchers (~> 2.0)
  sidekiq
  simplecov
  spring
  spring-watcher-listen (~> 2.0.0)
  test-unit
  tzinfo-data

RUBY VERSION
   ruby 2.6.5p114

BUNDLED WITH
   2.0.2

[nbulaj]

Hi @esnrRahman . Actually resource_owner_from_credentials is not configured to process FalseClass values. This block must return Resource Owner entity or nil in case it doesn't found. In your case it's boolean value.

But from the other side we can face a lot of such issues with this configuration block so it would be great to protect from them. I'll fix it, thanks for the report!

[esnrRahman]

Hi @nbulaj ! Thank you so much for the quick reply! So my expectation of resource_owner_from_credentials handling FalseClass stems from using it in V4.4.3. I returned user.authenticate there and it is handled as expected (i.e. returns 401)

I actually tried returning nil and I got 400 (invalid_grant) but my expectation was to get 401. Please let me know whether its expected to return nil and handle bad request as invalid credentials

SIDENOTE: I was observing this wiki -> https://github.com/doorkeeper-gem/doorkeeper/wiki/Using-Resource-Owner-Password-Credentials-flow

And in the first example I observed it returned user.authenticate!. So I am not sure whether devise or warden is being used. But I was assuming either user object or false gets returned

[nbulaj]

@esnrRahman I've made a change in #1339 to validate resource owner against false and nil values, you could check the behavior with the Doorkeeper gem from the github (it's not released).

If it once again works not as expected (in comparison with 4.x version) - let me know, I'll check what was done between this two major releases to find why it's broken. But only after holidays :)

[esnrRahman]

Thank you so much! I will definitely take a look. Really appreciate you checking this out during the holidays.

Wishing you happy holidays!

[esnrRahman]

Hello! I am just reporting my observation. Please take a look when you want to :)

V.4.4.3 ->
Status Code -> 401
Response ->

{
    "error": "invalid_grant",
    "error_description": "The provided authorization grant is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client."
}

V.5.2.3 [With fix]
Status code -> 400
Response ->

{
    "error": "invalid_grant",
    "error_description": "The provided authorization grant is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client."
}

The fix definitely works as we are no longer getting Internal Error (i.e. 500). But I will leave the decision upto you whether you want to keep the status code the same :). I believe 401 makes sense if user is unauthenticated.

Thank you!

[nbulaj]

Oh, now I remember we done somr statuses refactoring according to OAuth RFC! So maybe it's a required change. Try to check upgrade guides, AFAIR there must be some notes on this.

[esnrRahman]

Oh crap! Yes you are correct! For people stumbling on this thread ->

Error responses now return a 400 HTTP status by default (previously 401), and a 401 only for invalid_client and invalid_token errors.

Its in the wiki!