Add custom expiration time based on scopes #1102

thatsmydoing · 2018-05-31T03:26:35Z

Summary

This allows customizing the expiration time based on the requested scopes. This works much in the same way as #1049

Other Information

I personally think custom_access_token_expires_in is starting to get quite unwieldy with all the parameters. The method definition itself is getting quite long. Is this okay as it is now? Or would something like bundling everything into an object as a single parameter be something you'd like to happen?

Also, I'm not exactly sure how to write tests for this change as well. I'd like some direction on that.

houndci-bot · 2018-05-31T03:27:41Z

lib/doorkeeper/oauth/base_request.rb

@@ -35,7 +35,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, grant_type),
+ Authorization::Token.access_token_expires_in(server, client, grant_type, scopes),


Metrics/LineLength: Line is too long. [91/80]

houndci-bot · 2018-05-31T03:27:42Z

lib/doorkeeper/oauth/authorization/token.rb

 oauth_client = if pre_auth_or_oauth_client.respond_to?(:client)
 pre_auth_or_oauth_client.client
 else
 pre_auth_or_oauth_client
 end

- server.custom_access_token_expires_in.call(oauth_client, grant_type)
+ server.custom_access_token_expires_in.call(oauth_client, grant_type, scopes)


Metrics/LineLength: Line is too long. [88/80]

houndci-bot · 2018-05-31T03:27:42Z

lib/doorkeeper/oauth/authorization/token.rb

@@ -15,14 +15,14 @@ def access_token_expires_in(server, pre_auth_or_oauth_client, grant_type)

 private

- def custom_expiration(server, pre_auth_or_oauth_client, grant_type)
+ def custom_expiration(server, pre_auth_or_oauth_client, grant_type, scopes)


Metrics/LineLength: Line is too long. [85/80]

houndci-bot · 2018-05-31T03:27:42Z

lib/doorkeeper/oauth/authorization/token.rb

- 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))
+ def access_token_expires_in(server, pre_auth_or_oauth_client, grant_type, scopes)
+ if (expiration = custom_expiration(server, pre_auth_or_oauth_client, grant_type, scopes))


Metrics/LineLength: Line is too long. [101/80]

houndci-bot · 2018-05-31T03:27:42Z

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, grant_type)
- if (expiration = custom_expiration(server, pre_auth_or_oauth_client, grant_type))
+ def access_token_expires_in(server, pre_auth_or_oauth_client, grant_type, scopes)


Metrics/LineLength: Line is too long. [91/80]

houndci-bot · 2018-05-31T03:27:42Z

lib/doorkeeper/config.rb

@@ -236,7 +236,7 @@ def option(name, options = {})
 default: ->(_request, _response) {}
 option :skip_authorization, default: ->(_routes) {}
 option :access_token_expires_in, default: 7200
- option :custom_access_token_expires_in, default: ->(_app, _grant) { nil }
+ option :custom_access_token_expires_in, default: ->(_app, _grant, _scopes) { nil }


Metrics/LineLength: Line is too long. [86/80]

houndci-bot · 2018-05-31T03:27:43Z

lib/doorkeeper/oauth/base_request.rb

@@ -35,7 +35,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, grant_type),
+ Authorization::Token.access_token_expires_in(server, client, grant_type, scopes),


Metrics/LineLength: Line is too long. [91/80]

houndci-bot · 2018-05-31T03:27:43Z

lib/doorkeeper/oauth/authorization/token.rb

 oauth_client = if pre_auth_or_oauth_client.respond_to?(:client)
 pre_auth_or_oauth_client.client
 else
 pre_auth_or_oauth_client
 end

- server.custom_access_token_expires_in.call(oauth_client, grant_type)
+ server.custom_access_token_expires_in.call(oauth_client, grant_type, scopes)


Metrics/LineLength: Line is too long. [88/80]

houndci-bot · 2018-05-31T03:27:43Z

lib/doorkeeper/oauth/authorization/token.rb

@@ -15,14 +15,14 @@ def access_token_expires_in(server, pre_auth_or_oauth_client, grant_type)

 private

- def custom_expiration(server, pre_auth_or_oauth_client, grant_type)
+ def custom_expiration(server, pre_auth_or_oauth_client, grant_type, scopes)


Metrics/LineLength: Line is too long. [85/80]

houndci-bot · 2018-05-31T03:27:43Z

lib/doorkeeper/oauth/authorization/token.rb

- 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))
+ def access_token_expires_in(server, pre_auth_or_oauth_client, grant_type, scopes)
+ if (expiration = custom_expiration(server, pre_auth_or_oauth_client, grant_type, scopes))


Metrics/LineLength: Line is too long. [101/80]

houndci-bot · 2018-05-31T03:27:43Z

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, grant_type)
- if (expiration = custom_expiration(server, pre_auth_or_oauth_client, grant_type))
+ def access_token_expires_in(server, pre_auth_or_oauth_client, grant_type, scopes)


Metrics/LineLength: Line is too long. [91/80]

houndci-bot · 2018-05-31T03:27:43Z

lib/doorkeeper/config.rb

@@ -236,7 +236,7 @@ def option(name, options = {})
 default: ->(_request, _response) {}
 option :skip_authorization, default: ->(_routes) {}
 option :access_token_expires_in, default: 7200
- option :custom_access_token_expires_in, default: ->(_app, _grant) { nil }
+ option :custom_access_token_expires_in, default: ->(_app, _grant, _scopes) { nil }


Metrics/LineLength: Line is too long. [86/80]

nbulaj · 2018-05-31T07:19:18Z

Hi @thatsmydoing . Thanks for your work! For sure it can be useful. I also think that this option (and not only it) becomes to bloated and it's definitely requires refactoring. We can extract it to a class, yep.

What about specs - you can check the sample in the relevant PR

thatsmydoing · 2018-05-31T08:45:57Z

Alright, I'll try to extract it out into a class then. For testing, I'd like to know how to mock in the scopes since that's not quite obvious to me.

houndci-bot · 2018-06-04T02:50:13Z

spec/lib/oauth/refresh_token_request_spec.rb

@@ -85,7 +87,9 @@ module Doorkeeper::OAuth
 let(:server) do
 double :server,
 access_token_expires_in: 2.minutes,
- custom_access_token_expires_in: ->(_oauth_client, grant) { grant == Doorkeeper::OAuth::REFRESH_TOKEN ? 1234 : nil }
+ custom_access_token_expires_in: ->(context) {


Style/Lambda: Use the lambda method for multiline lambdas.

houndci-bot · 2018-06-04T02:50:13Z

spec/lib/oauth/refresh_token_request_spec.rb

@@ -30,7 +30,9 @@ 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: ->(_app, grant) { grant == Doorkeeper::OAuth::REFRESH_TOKEN ? 1234 : nil }
+ custom_access_token_expires_in: ->(context) {


Style/Lambda: Use the lambda method for multiline lambdas.

houndci-bot · 2018-06-04T02:50:13Z

spec/lib/oauth/password_access_token_request_spec.rb

+ expect do
+ subject.authorize
+ end.to change { Doorkeeper::AccessToken.count }.by(1)
+ expect(Doorkeeper::AccessToken.last.expires_in).to eq(12345)


Style/NumericLiterals: Use underscores(_) as decimal mark and separate every 3 digits with them.

houndci-bot · 2018-06-04T02:50:13Z

spec/lib/oauth/password_access_token_request_spec.rb

+ access_token_expires_in: 2.hours,
+ refresh_token_enabled?: false,
+ custom_access_token_expires_in: ->(context) {
+ context.scopes.exists?('public') ? 12345 : nil


Style/NumericLiterals: Use underscores(_) as decimal mark and separate every 3 digits with them.

houndci-bot · 2018-06-04T02:50:14Z

spec/lib/oauth/password_access_token_request_spec.rb

+ default_scopes: Doorkeeper::OAuth::Scopes.new,
+ access_token_expires_in: 2.hours,
+ refresh_token_enabled?: false,
+ custom_access_token_expires_in: ->(context) {


Style/Lambda: Use the lambda method for multiline lambdas.

houndci-bot · 2018-06-04T02:50:14Z

spec/lib/oauth/client_credentials/issuer_spec.rb

@@ -7,7 +7,7 @@ class Doorkeeper::OAuth::ClientCredentialsRequest
 double(
 :server,
 access_token_expires_in: 100,
- custom_access_token_expires_in: ->(_app, _grant) { nil }
+ custom_access_token_expires_in: ->(context) { nil }


Lint/UnusedBlockArgument: Unused block argument - context. If it's necessary, use _ or _context as an argument name to indicate that it won't be used. Also consider using a proc without arguments instead of a lambda if you want it to accept any arguments but don't care about them.

houndci-bot · 2018-06-04T02:50:14Z

spec/lib/oauth/base_request_spec.rb

+ "public",
+ server
+ )
+ expect(result.expires_in === 500)


Style/CaseEquality: Avoid the use of the case equality operator ===.

houndci-bot · 2018-06-04T02:50:14Z

spec/lib/oauth/base_request_spec.rb

@@ -20,7 +20,7 @@ module Doorkeeper::OAuth
 let(:server) do
 double :server,
 access_token_expires_in: 100,
- custom_access_token_expires_in: ->(_app, _grant) { nil },
+ custom_access_token_expires_in: ->(context) { nil },


Lint/UnusedBlockArgument: Unused block argument - context. If it's necessary, use _ or _context as an argument name to indicate that it won't be used. Also consider using a proc without arguments instead of a lambda if you want it to accept any arguments but don't care about them.

houndci-bot · 2018-06-04T02:50:14Z

spec/lib/oauth/authorization_code_request_spec.rb

@@ -6,7 +6,9 @@ module Doorkeeper::OAuth
 double :server,
 access_token_expires_in: 2.days,
 refresh_token_enabled?: false,
- custom_access_token_expires_in: ->(_app, grant) { grant == Doorkeeper::OAuth::AUTHORIZATION_CODE ? 1234 : nil }
+ custom_access_token_expires_in: ->(context) {


Style/Lambda: Use the lambda method for multiline lambdas.

houndci-bot · 2018-06-04T02:50:15Z

lib/doorkeeper/oauth/authorization/context.rb

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


Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.

thatsmydoing · 2018-06-05T02:15:32Z

I think this should be good. Can someone look over it?

nbulaj · 2018-06-05T21:22:31Z

lib/generators/doorkeeper/templates/initializer.rb

- # custom_access_token_expires_in do |oauth_client, grant_type|
- # oauth_client.application.additional_settings.implicit_oauth_expiration
+ # custom_access_token_expires_in do |context|
+ # context.client.application.additional_settings.implicit_oauth_expiration


Could you please add a note about available context methods?

nbulaj · 2018-06-05T21:26:15Z

Could you please squash commits and add an entry to news.md file? Good work btw!

This also involves changes to how parameters are passed into `custom_access_token_expires_in`. They're now all bundled into one context object instead of being passed separately.

houndci-bot · 2018-06-06T00:51:09Z

spec/lib/oauth/client_credentials/issuer_spec.rb

+ custom_ttl_scope
+ elsif context.grant_type == Doorkeeper::OAuth::CLIENT_CREDENTIALS
+ custom_ttl_grant
+ else


Style/EmptyElse: Redundant else-clause.

thatsmydoing · 2018-06-06T01:13:05Z

I've squashed the commits and added a news entry. I think this would also need an update in the migration guide as people will need to update their custom_access_token_expires_in in the config.

I'm not sure why coverage is failing since I did not change any code at all aside from squashing.

nbulaj · 2018-06-06T13:20:14Z

I think this would also need an update in the migration guide as people will need to update their custom_access_token_expires_in in the config.

Completely agree. We can do it right now because we bump a new major release. Do you want to edit Wiki page yourself or I'll do it?

houndci-bot reviewed May 31, 2018

View reviewed changes

houndci-bot reviewed Jun 4, 2018

View reviewed changes

thatsmydoing force-pushed the scope-based-expiry branch 2 times, most recently from d58edd7 to 8aa11ff Compare June 5, 2018 01:57

nbulaj reviewed Jun 5, 2018

View reviewed changes

Add custom expiration time based on scopes

0b336a1

This also involves changes to how parameters are passed into `custom_access_token_expires_in`. They're now all bundled into one context object instead of being passed separately.

thatsmydoing force-pushed the scope-based-expiry branch from 8aa11ff to 0b336a1 Compare June 6, 2018 00:50

houndci-bot reviewed Jun 6, 2018

View reviewed changes

nbulaj merged commit 37abb5e into doorkeeper-gem:master Jun 6, 2018

thatsmydoing mentioned this pull request Jun 7, 2018

Implement custom use_refresh_token #1103

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add custom expiration time based on scopes #1102

Add custom expiration time based on scopes #1102

thatsmydoing commented May 31, 2018

houndci-bot May 31, 2018

houndci-bot May 31, 2018

houndci-bot May 31, 2018

houndci-bot May 31, 2018

houndci-bot May 31, 2018

houndci-bot May 31, 2018

houndci-bot May 31, 2018

houndci-bot May 31, 2018

houndci-bot May 31, 2018

houndci-bot May 31, 2018

houndci-bot May 31, 2018

houndci-bot May 31, 2018

nbulaj commented May 31, 2018

thatsmydoing commented May 31, 2018

houndci-bot Jun 4, 2018

houndci-bot Jun 4, 2018

houndci-bot Jun 4, 2018

houndci-bot Jun 4, 2018

houndci-bot Jun 4, 2018

houndci-bot Jun 4, 2018

houndci-bot Jun 4, 2018

houndci-bot Jun 4, 2018

houndci-bot Jun 4, 2018

houndci-bot Jun 4, 2018

thatsmydoing commented Jun 5, 2018

nbulaj Jun 5, 2018

nbulaj commented Jun 5, 2018 •

edited

Loading

houndci-bot Jun 6, 2018

thatsmydoing commented Jun 6, 2018

nbulaj commented Jun 6, 2018 •

edited

Loading

Add custom expiration time based on scopes #1102

Add custom expiration time based on scopes #1102

Conversation

thatsmydoing commented May 31, 2018

Summary

Other Information

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 May 31, 2018

thatsmydoing commented May 31, 2018

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

thatsmydoing commented Jun 5, 2018

Choose a reason for hiding this comment

nbulaj commented Jun 5, 2018 • edited Loading

Choose a reason for hiding this comment

thatsmydoing commented Jun 6, 2018

nbulaj commented Jun 6, 2018 • edited Loading

nbulaj commented Jun 5, 2018 •

edited

Loading

nbulaj commented Jun 6, 2018 •

edited

Loading