Adds custom expiration time based on grant type #1049

rishabhsairawat · 2018-03-12T07:48:07Z

Summary

This PR aims to solve #996 Expiration Time Base On Grant Type

nbulaj · 2018-03-12T08:25:21Z

Hi @rishabhsairawat . Thank you for th PR!

I think that we can extract grant type names (like 'client_credentials' and other) in some constant like:

module Doorkeeper 
  module OAuth
    GRANT_TYPES = [
      CLIENT_CREDENTIALS = 'client_credentials'.freeze,
      REFRESH_TOKEN = 'refresh_token'.freeze,
      # ...
    ].freeze
  
    # ...
  end
end

And use it on generation custom expiration time

nbulaj · 2018-03-12T09:22:18Z

lib/doorkeeper/oauth/base_request.rb

@@ -2,6 +2,8 @@ module Doorkeeper
  module OAuth
    class BaseRequest
      include Validations
+
+      attr_accessor :grant_type


Do we really need accessor ? I think attr_reader would be enough

nbulaj · 2018-03-12T09:24:08Z

lib/doorkeeper/oauth/authorization/token.rb

@@ -36,7 +36,7 @@ def issue_token
            pre_auth.client,
            resource_owner.id,
            pre_auth.scopes,
-            self.class.access_token_expires_in(configuration, pre_auth),
+            self.class.access_token_expires_in(configuration, pre_auth, 'implicit'),


Use constants for such things as I described in the topic comments.

Hi @nbulaj Where to define these constants for grant flows as there is no class for grants in Doorkeeper::OAuth module. Should I define them in BaseRequest class?

Let's place them into Doorkeeper::Request module as GRANT_TYPES

No, let's introduce lib/doorkeeper/oauth.rb with explicit OAuth module:

module Doorkeeper module OAuth GRANT_TYPES = [ # ... ]. freeze end end

Don't forget to require it in main doorkeeper.rb:

require 'doorkeeper/oauth' require 'doorkeeper/oauth/authorization/code' require 'doorkeeper/oauth/authorization/token' # ...

I was thinking the same as It would be better to place them in separate file.

nbulaj · 2018-03-12T09:25:01Z

spec/controllers/authorizations_controller_spec.rb

  before do
    allow(Doorkeeper.configuration).to receive(:grant_flows).and_return(["implicit"])
    allow(controller).to receive(:current_resource_owner).and_return(user)
+    allow(Doorkeeper.configuration).to receive(:custom_access_token_expires_in).and_return(proc {|app, grant|
+        grant == 'implicit' ?  1234 : nil


Use flows constants (mentioned above) in specs too.

houndci-bot · 2018-03-12T15:36:54Z

spec/lib/oauth/token_request_spec.rb

@@ -51,8 +51,8 @@ module Doorkeeper::OAuth
      before do
        Doorkeeper.configure do
          orm DOORKEEPER_ORM
-          custom_access_token_expires_in do |_oauth_client|
-            1234
+          custom_access_token_expires_in do |_oauth_client, _grant_type|


Do not use prefix _ for a variable that is used.

houndci-bot · 2018-03-12T15:36:54Z

spec/lib/oauth/refresh_token_request_spec.rb

@@ -30,7 +30,7 @@ module Doorkeeper::OAuth
    it 'issues a new token for the client with custom expires_in' do
      server = double :server,
                      access_token_expires_in: 2.minutes,
-                      custom_access_token_expires_in: ->(_oauth_client) { 1234 }
+                      custom_access_token_expires_in: ->(_app, _grant) { _grant == Doorkeeper::OAuth::REFRESH_TOKEN ? 1234 : nil }


Do not use prefix _ for a variable that is used.

houndci-bot · 2018-03-12T15:36:54Z

spec/lib/oauth/refresh_token_request_spec.rb

@@ -9,7 +9,7 @@ module Doorkeeper::OAuth
    let(:server) do
      double :server,
             access_token_expires_in: 2.minutes,
-             custom_access_token_expires_in: -> (_oauth_client) { nil }
+             custom_access_token_expires_in: -> (_oauth_client, _grant) { nil }


Do not use spaces between -> and opening brace in lambda literals

houndci-bot · 2018-03-12T15:36:54Z

spec/lib/oauth/password_access_token_request_spec.rb

@@ -8,7 +8,7 @@ module Doorkeeper::OAuth
        default_scopes: Doorkeeper::OAuth::Scopes.new,
        access_token_expires_in: 2.hours,
        refresh_token_enabled?: false,
-        custom_access_token_expires_in: ->(_app) { nil }
+        custom_access_token_expires_in: ->(_app, _grant) { _grant == Doorkeeper::OAuth::PASSWORD ? 1234 : nil }


Do not use prefix _ for a variable that is used.

houndci-bot · 2018-03-12T15:36:54Z

spec/lib/oauth/client_credentials/issuer_spec.rb

        let(:server) do
          double(
            :server,
-            custom_access_token_expires_in: ->(_app) { custom_ttl }
+            custom_access_token_expires_in: ->(_app, _grant) { _grant == Doorkeeper::OAuth::CLIENT_CREDENTIALS ? custom_ttl : nil }


Do not use prefix _ for a variable that is used.

houndci-bot · 2018-03-12T15:36:56Z

lib/doorkeeper/oauth/base_request.rb

@@ -37,7 +39,7 @@ def find_or_create_access_token(client, resource_owner_id, scopes, server)
          client,
          resource_owner_id,
          scopes,
-          Authorization::Token.access_token_expires_in(server, client),
+          Authorization::Token.access_token_expires_in(server, client, grant_type),


Line is too long. [83/80]

houndci-bot · 2018-03-12T15:36:56Z

lib/doorkeeper/oauth/base_request.rb

@@ -2,6 +2,8 @@ module Doorkeeper
  module OAuth
    class BaseRequest
      include Validations
+


Trailing whitespace detected.

houndci-bot · 2018-03-12T15:36:56Z

lib/doorkeeper/oauth/authorization/token.rb

@@ -36,7 +36,7 @@ def issue_token
            pre_auth.client,
            resource_owner.id,
            pre_auth.scopes,
-            self.class.access_token_expires_in(configuration, pre_auth),
+            self.class.access_token_expires_in(configuration, pre_auth, Doorkeeper::OAuth::IMPLICIT),


Line is too long. [101/80]

houndci-bot · 2018-03-12T15:36:56Z

lib/doorkeeper/oauth/authorization/token.rb

-          def access_token_expires_in(server, pre_auth_or_oauth_client)
-            if (expiration = custom_expiration(server, pre_auth_or_oauth_client))
+          def access_token_expires_in(server, pre_auth_or_oauth_client, grant_type)
+            if (expiration = custom_expiration(server, pre_auth_or_oauth_client, grant_type))


Line is too long. [93/80]

houndci-bot · 2018-03-12T15:36:56Z

lib/doorkeeper/oauth/authorization/token.rb

@@ -5,8 +5,8 @@ class Token
        attr_accessor :pre_auth, :resource_owner, :token

        class << self
-          def access_token_expires_in(server, pre_auth_or_oauth_client)
-            if (expiration = custom_expiration(server, pre_auth_or_oauth_client))
+          def access_token_expires_in(server, pre_auth_or_oauth_client, grant_type)


Line is too long. [83/80]

houndci-bot · 2018-03-12T16:20:20Z

lib/doorkeeper/oauth.rb

+        REFRESH_TOKEN = 'refresh_token'.freeze
+      ].freeze
+    end
+end


Final newline missing.

houndci-bot · 2018-03-12T16:20:20Z

lib/doorkeeper/oauth.rb

+        PASSWORD = 'password'.freeze,
+        CLIENT_CREDENTIALS = 'client_credentials'.freeze,
+        REFRESH_TOKEN = 'refresh_token'.freeze
+      ].freeze


Indent the right bracket the same as the start of the line where the left bracket is.

houndci-bot · 2018-03-12T16:20:20Z

lib/doorkeeper/oauth.rb

+module Doorkeeper
+    module OAuth
+    GRANT_TYPES = [
+        AUTHORIZATION_CODE = 'authorization_code'.freeze,


Use 2 spaces for indentation in an array, relative to the start of the line where the left square bracket is.

houndci-bot · 2018-03-12T16:20:20Z

lib/doorkeeper/oauth.rb

@@ -0,0 +1,11 @@
+module Doorkeeper
+    module OAuth
+    GRANT_TYPES = [


Use 2 (not 0) spaces for indentation.

houndci-bot · 2018-03-12T16:20:21Z

lib/doorkeeper/oauth.rb

@@ -0,0 +1,11 @@
+module Doorkeeper
+    module OAuth


Use 2 (not 4) spaces for indentation.

houndci-bot · 2018-03-12T16:20:21Z

lib/doorkeeper/oauth.rb

@@ -0,0 +1,11 @@
+module Doorkeeper


Missing magic comment # frozen_string_literal: true.

houndci-bot · 2018-03-12T16:49:56Z

lib/doorkeeper/oauth.rb

@@ -0,0 +1,12 @@
+# frozen_string_literal: true
+module Doorkeeper


Add an empty line after magic comments.

nbulaj · 2018-03-12T21:41:19Z

Thank you @rishabhsairawat 👍

nbulaj self-requested a review March 12, 2018 09:18

nbulaj self-assigned this Mar 12, 2018

nbulaj added the enhancement label Mar 12, 2018

nbulaj added this to the 5.0 milestone Mar 12, 2018

nbulaj reviewed Mar 12, 2018

View reviewed changes

rishabhsairawat force-pushed the master branch from 33932e6 to e9bf05b Compare March 12, 2018 15:36

houndci-bot reviewed Mar 12, 2018

View reviewed changes

rishabhsairawat force-pushed the master branch from e9bf05b to 225f1f8 Compare March 12, 2018 16:19

houndci-bot reviewed Mar 12, 2018

View reviewed changes

rishabhsairawat force-pushed the master branch 2 times, most recently from 5ccc413 to 0cd20d7 Compare March 12, 2018 16:49

houndci-bot reviewed Mar 12, 2018

View reviewed changes

Adds custom expiration time based on grant type

62fd33b

rishabhsairawat force-pushed the master branch from 0cd20d7 to 62fd33b Compare March 12, 2018 16:52

nbulaj merged commit 55439ee into doorkeeper-gem:master Mar 12, 2018

thatsmydoing mentioned this pull request May 31, 2018

Add custom expiration time based on scopes #1102

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds custom expiration time based on grant type #1049

Adds custom expiration time based on grant type #1049

rishabhsairawat commented Mar 12, 2018

nbulaj commented Mar 12, 2018 •

edited

Loading

nbulaj Mar 12, 2018

nbulaj Mar 12, 2018

rishabhsairawat Mar 12, 2018 •

edited

Loading

nbulaj Mar 12, 2018

nbulaj Mar 12, 2018 •

edited

Loading

rishabhsairawat Mar 12, 2018

nbulaj Mar 12, 2018 •

edited

Loading

houndci-bot Mar 12, 2018

houndci-bot Mar 12, 2018

houndci-bot Mar 12, 2018

houndci-bot Mar 12, 2018

houndci-bot Mar 12, 2018

houndci-bot Mar 12, 2018

houndci-bot Mar 12, 2018

houndci-bot Mar 12, 2018

houndci-bot Mar 12, 2018

houndci-bot Mar 12, 2018

houndci-bot Mar 12, 2018

houndci-bot Mar 12, 2018

houndci-bot Mar 12, 2018

houndci-bot Mar 12, 2018

houndci-bot Mar 12, 2018

houndci-bot Mar 12, 2018

houndci-bot Mar 12, 2018

nbulaj commented Mar 12, 2018

		@@ -0,0 +1,12 @@
		# frozen_string_literal: true
		module Doorkeeper

Adds custom expiration time based on grant type #1049

Adds custom expiration time based on grant type #1049

Conversation

rishabhsairawat commented Mar 12, 2018

Summary

nbulaj commented Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rishabhsairawat Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbulaj Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbulaj Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbulaj commented Mar 12, 2018

nbulaj commented Mar 12, 2018 •

edited

Loading

rishabhsairawat Mar 12, 2018 •

edited

Loading

nbulaj Mar 12, 2018 •

edited

Loading

nbulaj Mar 12, 2018 •

edited

Loading