Skip to content

Unnecessary SQL query to get the refresh token, even if it's not enabled in the doorkeeper.rb #1099

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

Closed
thromera opened this issue May 28, 2018 · 3 comments

Comments

@thromera
Copy link
Contributor

Steps to reproduce

Api::V1::HelloController:

class Api::V1::HelloController < ActionController::Base
  before_action :doorkeeper_authorize! 

  def hi
    render json: {hello: 'world'}
  end
end

Upon requests, rails logs give me this:

Started GET "/api/v1/hello/hi" for 127.0.0.1 at 2018-05-28 18:58:43 +0200
Processing by Api::V1::HelloController#hi as */*
  Doorkeeper::AccessToken Load (0.3ms)  SELECT  "oauth_access_tokens".* FROM "oauth_access_tokens" WHERE "oauth_access_tokens"."token" = $1 LIMIT $2  [["token", "123456789blablablaMyAccessToken"], ["LIMIT", 1]]
  Doorkeeper::AccessToken Load (0.2ms)  SELECT  "oauth_access_tokens".* FROM "oauth_access_tokens" WHERE "oauth_access_tokens"."refresh_token" = $1 LIMIT $2  [["refresh_token", ""], ["LIMIT", 1]]
Completed 200 OK in 4ms (Views: 0.1ms | ActiveRecord: 0.5ms)

Note the second SQL Doorkeeper::AccessToken Load (0.2ms) SELECT "oauth_access_tokens".* FROM "oauth_access_tokens" WHERE "oauth_access_tokens"."refresh_token" = $1 LIMIT $2 [["refresh_token", ""], ["LIMIT", 1]] that I don't get since I specifically disabled refresh tokens.

Expected behavior

Only one SQL Query to match the access_token with the one registered in the oauth_access_tokens table

Actual behavior

Another query is made to match the refresh token while it is not needed.

System configuration

You can help us to understand your problem if you will share some very
useful information about your project environment (don't forget to
remove any confidential data if it exists).

Doorkeeper initializer:

Doorkeeper.configure do
  # Change the ORM that doorkeeper will use (needs plugins)
  orm :active_record

  # This block will be called to check whether the resource owner is authenticated or not.
  resource_owner_authenticator do
    sign_out(current_user) unless current_user&.is_a?(Admin)
    session[:user_return_to] = request.fullpath
    current_user&.is_a?(Admin) ? current_user : redirect_to(new_user_session_url)
  end

  # If you want to restrict access to the web interface for adding oauth authorized applications, you need to declare the block below.
  admin_authenticator do
    current_user.is_a?(Admin) || redirect_to(new_user_session_url)
  end

  # resource_owner_from_credentials do |routes|
  #   admin = Admin.find_for_database_authentication(email: params[:username])
  #   if admin && admin.valid_for_authentication? { admin.valid_password?(params[:password]) }
  #     admin
  #   end
  # end

  # Authorization Code expiration time (default 10 minutes).
  # authorization_code_expires_in 10.minutes

  # Access token expiration time (default 2 hours).
  # If you want to disable expiration, set this to nil.
  # access_token_expires_in 2.hours

  # Assign a custom TTL for implicit grants.
  # custom_access_token_expires_in do |oauth_client|
  #   oauth_client.application.additional_settings.implicit_oauth_expiration
  # end

  # Use a custom class for generating the access token.
  # https://github.com/doorkeeper-gem/doorkeeper#custom-access-token-generator
  # access_token_generator '::Doorkeeper::JWT'

  # The controller Doorkeeper::ApplicationController inherits from.
  # Defaults to ActionController::Base.
  # https://github.com/doorkeeper-gem/doorkeeper#custom-base-controller
  base_controller 'ActionController::API'

  # Reuse access token for the same resource owner within an application (disabled by default)
  # Rationale: https://github.com/doorkeeper-gem/doorkeeper/issues/383
  # reuse_access_token

  # Issue access tokens with refresh token (disabled by default)
  # use_refresh_token

  # Provide support for an owner to be assigned to each registered application (disabled by default)
  # Optional parameter confirmation: true (default false) if you want to enforce ownership of
  # a registered application
  # Note: you must also run the rails g doorkeeper:application_owner generator to provide the necessary support
  # enable_application_owner confirmation: false

  # Define access token scopes for your provider
  # For more information go to
  # https://github.com/doorkeeper-gem/doorkeeper/wiki/Using-Scopes
  # default_scopes  :public
  # optional_scopes :write, :update

  # Change the way client credentials are retrieved from the request object.
  # By default it retrieves first from the `HTTP_AUTHORIZATION` header, then
  # falls back to the `:client_id` and `:client_secret` params from the `params` object.
  # Check out the wiki for more information on customization
  # client_credentials :from_basic, :from_params

  # Change the way access token is authenticated from the request object.
  # By default it retrieves first from the `HTTP_AUTHORIZATION` header, then
  # falls back to the `:access_token` or `:bearer_token` params from the `params` object.
  # Check out the wiki for more information on customization
  # access_token_methods :from_bearer_authorization, :from_access_token_param, :from_bearer_param

  # Change the native redirect uri for client apps
  # When clients register with the following redirect uri, they won't be redirected to any server and the authorization code will be displayed within the provider
  # The value can be any string. Use nil to disable this feature. When disabled, clients must provide a valid URL
  # (Similar behaviour: https://developers.google.com/accounts/docs/OAuth2InstalledApp#choosingredirecturi)
  #
  # native_redirect_uri 'urn:ietf:wg:oauth:2.0:oob'

  # Forces the usage of the HTTPS protocol in non-native redirect uris (enabled
  # by default in non-development environments). OAuth2 delegates security in
  # communication to the HTTPS protocol so it is wise to keep this enabled.
  #
  # force_ssl_in_redirect_uri !Rails.env.development?

  # Specify what grant flows are enabled in array of Strings. The valid
  # strings and the flows they enable are:
  #
  # "authorization_code" => Authorization Code Grant Flow
  # "implicit"           => Implicit Grant Flow
  # "password"           => Resource Owner Password Credentials Grant Flow
  # "client_credentials" => Client Credentials Grant Flow
  #
  # If not specified, Doorkeeper enables authorization_code and
  # client_credentials.
  #
  # implicit and password grant flows have risks that you should understand
  # before enabling:
  #   http://tools.ietf.org/html/rfc6819#section-4.4.2
  #   http://tools.ietf.org/ht§l/rfc6819#section-4.4.3
  #
  grant_flows %w(implicit)

  # Under some circumstances you might want to have applications auto-approved,
  # so that the user skips the authorization step.
  # For example if dealing with a trusted application.
  # skip_authorization do |resource_owner, client|
  #   client.superapp? or resource_owner.admin?
  # end

  # WWW-Authenticate Realm (default "Doorkeeper").
  # realm "Doorkeeper"
end

Ruby version:
2.4.x

Doorkeeper version
4.3.2

@thromera thromera changed the title Unnecessary SQL query to get the refresh token, even if it's not enabled in the application.rb Unnecessary SQL query to get the refresh token, even if it's not enabled in the doorkeeper.rb May 28, 2018
@nbulaj
Copy link
Member

nbulaj commented May 28, 2018

Hi @Erowlin . Yep, sounds reasonable. Would you like to submit a PR?

@thromera
Copy link
Contributor Author

I will try to fix it tomorrow yes!

@thromera
Copy link
Contributor Author

Here we go :). I let you review the PR and let me know if any changes are needed.

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

No branches or pull requests

2 participants