Closed benebrice closed 4 years ago
nbulaj
commented
4 years ago Hi @benebrice . It's super weird because we have specs that insure requires fields in the JSON response for password flow (and others actually):
Maybe your specs related to Doorkeeper::Application instances and informs about other missing fields that was removed after https://github.com/doorkeeper-gem/doorkeeper/security/advisories/GHSA-j7vx-8mqj-cqp9 was closed?
benebrice
commented
4 years ago This is weird. I do have an association to Doorkeeper::Application but I don't use it at all here. It's native behaviour. I just changed the root but controller inherits from Doorkeeper::TokensController.
I guess it is not related to GHSA-j7vx-8mqj-cqp9 since I don't have the issue with 5.3.3 right?
Here is my spec (using rswag)
let(:owner) { create(:user) }
let(:lambda_user) do
create(:user,
password: lambda_user_password,
password_confirmation: lambda_user_password)
end
let(:lambda_user_password) { SecureRandom.uuid }
let(:application) { create(:application, :guest_read_write, owner: owner) }
let(:client_id) { application.uid }
let(:client_secret) { application.secret }
let(:token) do
create(:access_token,
resource_owner_id: user.id,
application_id: application.id,
use_refresh_token: true,
scopes: scope)
end
path '/oauth/token' do
post 'Generate an access token' do
tags 'Oauth'
consumes 'application/vnd.api+json'
produces 'application/vnd.api+json'
# Parameters
parameter name: :params,
in: :body,
required: true
let(:grant_type) { 'password' }
let(:email) { lambda_user.email }
let(:password) { lambda_user_password }
let(:scope) { 'read write' }
let(:params) do
{ grant_type: grant_type,
client_id: client_id,
client_secret: client_secret,
email: email,
password: password,
scope: scope }
end
response '200', 'Generate a token' do
let!(:user) do
create(:user, :unconfirmed,
confirmation_token: confirmation_token)
end
let(:confirmation_token) { SecureRandom.uuid }
schema('$ref' => '#/definitions/AccessToken')
keys = %w[access_token token_type expires_in refresh_token created_at]
run_test_with_keys!(keys) do |json|
it 'has scope' do
expect(json['scope']).to eq(scope)
end
end
end
response '200', 'Refresh a token' do
let!(:user) { create(:user) }
let(:grant_type) { 'refresh_token' }
let(:params) do
{ grant_type: grant_type,
client_id: client_id,
client_secret: client_secret,
refresh_token: token.refresh_token }
end
schema('$ref' => '#/definitions/AccessToken')
keys = %w[access_token token_type expires_in refresh_token created_at]
run_test_with_keys!(keys) do |json|
it 'has scope' do
expect(json['scope']).to eq(scope)
end
end
end
context 'when generating new token' do
let(:grant_type) { nil }
let(:scopes) { %w[read write] }
it_behaves_like 'rswag_error',
400,
'Missing required parameter: grant_type.' do
let(:title) { 'Missing required parameter: grant_type.' }
end
end
end
endwith helper
def run_test_with_keys!(keys, &block)
run_test! do |response|
json = JSON.parse(response.body)
keys.each do |k|
expect(json).to have_key(k)
end
block.call(json) if block_given?
end
endSpecs fails when running
keys = %w[access_token token_type expires_in refresh_token created_at]
run_test_with_keys!(keys)I guess it is not related to GHSA-j7vx-8mqj-cqp9 since I don't have the issue with 5.3.3 right?
I think yes.
From what I can see there is no reason not to have an expires_in in the response. I need to check also changelog to see if we changed something in TokensController behavior or acctess token serialization.
nbulaj
commented
4 years ago Checked the changelog again and didn't find anything criminal.
Maybe something here https://github.com/doorkeeper-gem/doorkeeper/compare/38b7333c...d5fff38e
Also could you please check inside your test the actual exires_in value of the access token instance? Maybe it's nil and server just compact the response
nbulaj
commented
4 years ago Hi @benebrice . Did you find the root of the issue?
benebrice
commented
4 years ago Nope. I've downgraded to 5.3.3, no more issue. 😕
nbulaj
commented
4 years ago This is super sad :(
benebrice
commented
4 years ago It is. I did not take enough time to dig on this step by step on the gem but if this issue happens on the 5.4 but not on 5.3.3 it might be an issue of the gem, hidden somewhere.
I'll let you know as soon as I have debugged this.
nbulaj
commented
4 years ago The problem is to find it for gem is that we have a lot of specs that checks for this field.. So maybe it's some interesting use case, I don't know
benebrice
commented
4 years ago I guess you're right. Rails API + Device + Doorkeeper + Jsonapi-resources, it's not that common yet. Maybe some configuration on my side turned off this on the gem side.
storhet
commented
4 years ago We faced the similar problem with the grant_type: 'refresh_token'.
Maybe it will help but in our case we didn't set expires_in for the factory of the token we wanted to refresh and as you don't send the request, you don't get the expires_in to be set by default.
And I have a related question to @nbulaj. Is that the expected behaviour to use 'parent token' to set attributes of the 'new (refreshed) token'? I assume that in case if I will change default expires_in to be something else, then this change will force me to invalidate all the tokens or manually update them (never done it tho 😄 ), since the refresh_token_request will always get the original value from the parent and not from the config.
nbulaj
commented
4 years ago Maybe it will help but in our case we didn't set expires_in for the factory of the token we wanted to refresh and as you don't send the request, you don't get the expires_in to be set by default.
That's sounds like what I've posted above:
Also could you please check inside your test the actual
expires_invalue of the access token instance? Maybe it'sniland server just compact the response
Also I don't sure how Jsonapi-resources serializes data and how it affects on Doorkeeper internals.
@storhet
Is that the expected behaviour to use 'parent token' to set attributes of the 'new (refreshed) token'?
Yes
I assume that in case if I will change default expires_in to be something else,
In this case you 're bring data inconsistency yourself, the same as in #1399 . It's like when you set required scopes for the endpoint like read / write, your application users log in and obtain access tokens, than you just change required scopes to read_repository / write_repository. You need to deal with such cases yourself (create a data migration and update the tokens with a new scopes, etc). So:
I assume that in case if I will change default expires_in to be something else, then this change will force me to invalidate all the tokens or manually update them
Exactly.
Also from RFC6749, p. 1.5. Refresh Token
Refresh tokens are issued to the client by the authorization server and are used to obtain a new access token when the current access token becomes invalid or expires, or to obtain additional access tokens with identical or narrower scope (access tokens may have a shorter lifetime and fewer permissions than authorized by the resource owner).
TheTeaNerd
commented
4 years ago I'm also seeing this:
Rails (full, not API) 5.2.4.3, Devise 4.7.2, fast_jsonapi 1.5
Works fine on 5.3.3, on 5.4.0 I have a regression: my refresh_token response does not have the 'expires_in' attribute.
describe 'grant_type refresh_token' do
let(:user) { create :user, email: 'user@example.com', password: '12345678' }
let(:client_application) { create :client_application }
let(:refresh_token) do
client_application.access_tokens.create!(use_refresh_token: true, resource_owner_id: user.id).r efresh_token
end
it 'returns new refresh_token', reqres_title: 'Get new refresh token' do
post '/oauth/token', params: {
'grant_type' => 'refresh_token',
'refresh_token' => refresh_token,
'client_id' => client_application.uid,
'client_secret' => client_application.secret
}
expect(Doorkeeper::AccessToken.count).to eq 2
expect(Doorkeeper::AccessToken.second.application_id).to eq client_application.id
expect(json['access_token'].size).to eq 43
expect(json['refresh_token'].size).to eq 43
expect(json['refresh_token'].size).not_to eq refresh_token
expect(json['token_type']).to eq 'Bearer'
expect(json['expires_in']).to eq 7200 # FAILS
expect(json['created_at'].present?).to eq true
expect(response.status).to eq 200
end
endIf it helps, the regression is in 5.4.0.rc1
Looking at some of the PRs, #1366 seems to relate, but I don't see a significant change.
TheTeaNerd
commented
4 years ago nbulaj
commented
4 years ago @rishabhsairawat could you please take a look at the above comment? #1366 related
rishabhsairawat
commented
4 years ago @TheTeaNerd What I changed in #1366 was that Refresh token will use expiry from that of its parent token. So Earlier for refresh token, it was using expiry from the configuration but after #1366 It will use from its parent token.
Looking at your test cases looks like you are not setting deny expiry in your token factory. @storhet faced the same problem:
We faced the similar problem with the grant_type: 'refresh_token'. Maybe it will help but in our case we didn't set expires_in for the factory of the token we wanted to refresh and as you don't send the request, you don't get the expires_in to be set by default.
Also, the issue reported by @benebrice is not related to #1366 as that changes doorkeeper behavior only for refresh_token grant type not for any other grant type. @nbulaj I have checked in the doorkeeper code, It already has specs written for token expiry fields.
nbulaj
commented
4 years ago Thanks @rishabhsairawat.
@benebrice could you please check if comment above resolves your issue?
stale[bot]
commented
4 years ago This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
benebrice
commented
3 years ago @nbulaj I finally had time to check it. It's fine if you declare expires_in on the parent. Thanks @rishabhsairawat 👌🏻
# before
let(:token) do
create(:access_token,
resource_owner_id: admin.id,
application_id: application.id,
use_refresh_token: true,
scopes: scope)
end
# after
let(:token) do
create(:access_token,
resource_owner_id: admin.id,
application_id: application.id,
use_refresh_token: true,
expires_in: 2.hours.to_i,
scopes: scope)
end
julienchabanon
commented
3 years ago I tried version 5.5.2 and it appears that it is still not fixed. I have downgraded to 5.3.3!
Steps to reproduce
Just call
/oauth/tokenwith agrant_typetopassword(what I have tested, maybe other types have the same issue). My specs failed after upgrade. After some research,expires_inis just not sent at all. It does not happen with5.3.3Expected behavior
We should receive a json with keys the following keys:
Actual behavior
We receive a json with keys the following keys:
expires_inSystem configuration
Doorkeeper initializer:
Ruby version:
2.6.5Gemfile.lock:
Gemfile.lock content
``` GIT remote: https://github.com/benebrice/jsonapi-resources.git revision: 318bf85e3644f7086c29c58f4cd82784054501e4 branch: fix--link-builder-on-root-engine specs: jsonapi-resources (0.9.11) activerecord (>= 4.1) concurrent-ruby railties (>= 4.1) PATH remote: iso/engines/internal/secret_api specs: secret_api (0.1.0) doorkeeper (~> 5.2) jsonapi-resources rails (~> 6.0.2) GEM remote: https://rubygems.org/ specs: aasm (5.0.8) concurrent-ruby (~> 1.0) actioncable (6.0.3) actionpack (= 6.0.3) nio4r (~> 2.0) websocket-driver (>= 0.6.1) actionmailbox (6.0.3) actionpack (= 6.0.3) activejob (= 6.0.3) activerecord (= 6.0.3) activestorage (= 6.0.3) activesupport (= 6.0.3) mail (>= 2.7.1) actionmailer (6.0.3) actionpack (= 6.0.3) actionview (= 6.0.3) activejob (= 6.0.3) mail (~> 2.5, >= 2.5.4) rails-dom-testing (~> 2.0) actionpack (6.0.3) actionview (= 6.0.3) activesupport (= 6.0.3) rack (~> 2.0, >= 2.0.8) rack-test (>= 0.6.3) rails-dom-testing (~> 2.0) rails-html-sanitizer (~> 1.0, >= 1.2.0) actiontext (6.0.3) actionpack (= 6.0.3) activerecord (= 6.0.3) activestorage (= 6.0.3) activesupport (= 6.0.3) nokogiri (>= 1.8.5) actionview (6.0.3) activesupport (= 6.0.3) builder (~> 3.1) erubi (~> 1.4) rails-dom-testing (~> 2.0) rails-html-sanitizer (~> 1.1, >= 1.2.0) active_delivery (0.4.2) activejob (6.0.3) activesupport (= 6.0.3) globalid (>= 0.3.6) activemodel (6.0.3) activesupport (= 6.0.3) activerecord (6.0.3) activemodel (= 6.0.3) activesupport (= 6.0.3) activestorage (6.0.3) actionpack (= 6.0.3) activejob (= 6.0.3) activerecord (= 6.0.3) marcel (~> 0.3.1) activesupport (6.0.3) concurrent-ruby (~> 1.0, >= 1.0.2) i18n (>= 0.7, < 2) minitest (~> 5.1) tzinfo (~> 1.1) zeitwerk (~> 2.2, >= 2.2.2) acts-as-taggable-on (6.5.0) activerecord (>= 5.0, < 6.1) addressable (2.7.0) public_suffix (>= 2.0.2, < 5.0) airbrussh (1.4.0) sshkit (>= 1.6.1, != 1.7.0) annotate (3.1.1) activerecord (>= 3.2, < 7.0) rake (>= 10.4, < 14.0) ast (2.4.0) awesome_print (1.8.0) aws-eventstream (1.1.0) aws-partitions (1.312.0) aws-sdk-core (3.95.0) aws-eventstream (~> 1, >= 1.0.2) aws-partitions (~> 1, >= 1.239.0) aws-sigv4 (~> 1.1) jmespath (~> 1.0) aws-sdk-kms (1.31.0) aws-sdk-core (~> 3, >= 3.71.0) aws-sigv4 (~> 1.1) aws-sdk-rails (3.1.0) aws-sdk-ses (~> 1) railties (>= 5.2.0) aws-sdk-s3 (1.64.0) aws-sdk-core (~> 3, >= 3.83.0) aws-sdk-kms (~> 1) aws-sigv4 (~> 1.1) aws-sdk-ses (1.29.0) aws-sdk-core (~> 3, >= 3.71.0) aws-sigv4 (~> 1.1) aws-sdk-sns (1.23.0) aws-sdk-core (~> 3, >= 3.71.0) aws-sigv4 (~> 1.1) aws-sigv4 (1.1.3) aws-eventstream (~> 1.0, >= 1.0.2) bcrypt (3.1.13) bootsnap (1.4.6) msgpack (~> 1.0) builder (3.2.4) bullet (6.1.0) activesupport (>= 3.0.0) uniform_notifier (~> 1.11) byebug (11.1.3) capistrano (3.14.0) airbrussh (>= 1.0.0) i18n rake (>= 10.0.0) sshkit (>= 1.9.0) capistrano-bundler (1.6.0) capistrano (~> 3.1) capistrano-multiconfig (3.1.0) capistrano (>= 3.7.0) capistrano-rails (1.4.0) capistrano (~> 3.1) capistrano-bundler (~> 1.1) capistrano-rake (0.2.0) capistrano (>= 3.0) capistrano-rvm (0.1.2) capistrano (~> 3.0) sshkit (~> 1.2) capistrano3-puma (4.0.0) capistrano (~> 3.7) capistrano-bundler puma (~> 4.0) chronic (0.10.2) concurrent-ruby (1.1.6) connection_pool (2.2.2) crass (1.0.6) daemons (1.3.1) database_cleaner (1.8.5) devise (4.7.1) bcrypt (~> 3.0) orm_adapter (~> 0.1) railties (>= 4.1.0) responders warden (~> 1.2.3) diff-lcs (1.3) docile (1.3.2) doorkeeper (5.4.0) railties (>= 5) doorkeeper-jwt (0.4.0) jwt (~> 2.1) dotenv (2.7.5) dotenv-rails (2.7.5) dotenv (= 2.7.5) railties (>= 3.2, < 6.1) erubi (1.9.0) eventmachine (1.2.7) factory_bot (5.2.0) activesupport (>= 4.2.0) factory_bot_rails (5.2.0) factory_bot (~> 5.2.0) railties (>= 4.2.0) faker (2.11.0) i18n (>= 1.6, < 2) faraday (1.0.1) multipart-post (>= 1.2, < 3) ffi (1.12.2) geocoder (1.6.3) gitlab (4.14.1) httparty (~> 0.14, >= 0.14.0) terminal-table (~> 1.5, >= 1.5.1) globalid (0.4.2) activesupport (>= 4.2.0) haml (5.1.2) temple (>= 0.8.0) tilt httparty (0.18.0) mime-types (~> 3.0) multi_xml (>= 0.5.2) i18n (1.8.2) concurrent-ruby (~> 1.0) image_processing (1.10.3) mini_magick (>= 4.9.5, < 5) ruby-vips (>= 2.0.17, < 3) jaro_winkler (1.5.4) jmespath (1.4.0) json (2.3.0) json-schema (2.8.1) addressable (>= 2.4) jwt (2.2.1) listen (3.1.5) rb-fsevent (~> 0.9, >= 0.9.4) rb-inotify (~> 0.9, >= 0.9.7) ruby_dep (~> 1.2) loofah (2.5.0) crass (~> 1.0.2) nokogiri (>= 1.5.9) mail (2.7.1) mini_mime (>= 0.1.1) mailcatcher (0.2.4) eventmachine haml i18n json mail sinatra skinny (>= 0.1.2) sqlite3-ruby thin marcel (0.3.3) mimemagic (~> 0.3.2) method_source (1.0.0) mime-types (3.3.1) mime-types-data (~> 3.2015) mime-types-data (3.2020.0425) mimemagic (0.3.5) mini_magick (4.10.1) mini_mime (1.0.2) mini_portile2 (2.4.0) minitest (5.14.0) msgpack (1.3.3) multi_xml (0.6.0) multipart-post (2.1.1) mustermann (1.1.1) ruby2_keywords (~> 0.0.1) net-scp (3.0.0) net-ssh (>= 2.6.5, < 7.0.0) net-ssh (6.0.2) nio4r (2.5.2) nokogiri (1.10.9) mini_portile2 (~> 2.4.0) octokit (4.18.0) faraday (>= 0.9) sawyer (~> 0.8.0, >= 0.5.3) orm_adapter (0.5.0) parallel (1.19.1) parser (2.7.1.2) ast (~> 2.4.0) pg (1.2.3) pronto (0.10.0) gitlab (~> 4.0, >= 4.0.0) httparty (>= 0.13.7) octokit (~> 4.7, >= 4.7.0) rainbow (>= 2.2, < 4.0) rugged (~> 0.24, >= 0.23.0) thor (~> 0.20.0) pronto-rubocop (0.10.0) pronto (~> 0.10.0) rubocop (~> 0.50, >= 0.49.1) public_suffix (4.0.5) puma (4.3.3) nio4r (~> 2.0) rack (2.2.2) rack-cors (1.1.1) rack (>= 2.0.0) rack-protection (2.0.8.1) rack rack-test (1.1.0) rack (>= 1.0, < 3) rails (6.0.3) actioncable (= 6.0.3) actionmailbox (= 6.0.3) actionmailer (= 6.0.3) actionpack (= 6.0.3) actiontext (= 6.0.3) actionview (= 6.0.3) activejob (= 6.0.3) activemodel (= 6.0.3) activerecord (= 6.0.3) activestorage (= 6.0.3) activesupport (= 6.0.3) bundler (>= 1.3.0) railties (= 6.0.3) 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.3) actionpack (= 6.0.3) activesupport (= 6.0.3) method_source rake (>= 0.8.7) thor (>= 0.20.3, < 2.0) rainbow (3.0.0) rake (13.0.1) rb-fsevent (0.10.4) rb-inotify (0.10.1) ffi (~> 1.0) redis (4.1.4) responders (3.0.0) actionpack (>= 5.0) railties (>= 5.0) rexml (3.2.4) rspec-core (3.9.2) rspec-support (~> 3.9.3) rspec-expectations (3.9.2) diff-lcs (>= 1.2.0, < 2.0) rspec-support (~> 3.9.0) rspec-mocks (3.9.1) diff-lcs (>= 1.2.0, < 2.0) rspec-support (~> 3.9.0) rspec-rails (4.0.0) actionpack (>= 4.2) activesupport (>= 4.2) railties (>= 4.2) rspec-core (~> 3.9) rspec-expectations (~> 3.9) rspec-mocks (~> 3.9) rspec-support (~> 3.9) rspec-support (3.9.3) rswag (2.3.1) rswag-api (= 2.3.1) rswag-specs (= 2.3.1) rswag-ui (= 2.3.1) rswag-api (2.3.1) railties (>= 3.1, < 7.0) rswag-specs (2.3.1) activesupport (>= 3.1, < 7.0) json-schema (~> 2.2) railties (>= 3.1, < 7.0) rswag-ui (2.3.1) actionpack (>= 3.1, < 7.0) railties (>= 3.1, < 7.0) rubocop (0.82.0) jaro_winkler (~> 1.5.1) parallel (~> 1.10) parser (>= 2.7.0.1) rainbow (>= 2.2.2, < 4.0) rexml ruby-progressbar (~> 1.7) unicode-display_width (>= 1.4.0, < 2.0) rubocop-git (0.1.3) rubocop (>= 0.24.1) rubocop-rspec (1.39.0) rubocop (>= 0.68.1) ruby-progressbar (1.10.1) ruby-vips (2.0.17) ffi (~> 1.9) ruby2_keywords (0.0.2) ruby_dep (1.5.0) rugged (0.99.0) sawyer (0.8.2) addressable (>= 2.3.5) faraday (> 0.8, < 2.0) seed-fu (2.3.9) activerecord (>= 3.1) activesupport (>= 3.1) shoulda-matchers (4.3.0) activesupport (>= 4.2.0) sidekiq (6.0.7) connection_pool (>= 2.2.2) rack (~> 2.0) rack-protection (>= 2.0.0) redis (>= 4.1.0) simplecov (0.18.5) docile (~> 1.1) simplecov-html (~> 0.11) simplecov-html (0.12.2) sinatra (2.0.8.1) mustermann (~> 1.0) rack (~> 2.0) rack-protection (= 2.0.8.1) tilt (~> 2.0) skinny (0.2.2) eventmachine (~> 1.0) thin spring (2.1.0) spring-commands-rspec (1.0.4) spring (>= 0.9.1) 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) sqlite3 (1.4.2) sqlite3-ruby (1.3.3) sqlite3 (>= 1.3.3) sshkit (1.21.0) net-scp (>= 1.1.2) net-ssh (>= 2.8.0) temple (0.8.2) terminal-table (1.8.0) unicode-display_width (~> 1.1, >= 1.1.1) thin (1.7.2) daemons (~> 1.0, >= 1.0.9) eventmachine (~> 1.0, >= 1.0.4) rack (>= 1, < 3) thor (0.20.3) thread_safe (0.3.6) tilt (2.0.10) timecop (0.9.1) tzinfo (1.2.7) thread_safe (~> 0.1) unicode-display_width (1.7.0) uniform_notifier (1.13.0) warden (1.2.8) rack (>= 2.0.6) websocket-driver (0.7.1) websocket-extensions (>= 0.1.0) websocket-extensions (0.1.4) whenever (1.0.0) chronic (>= 0.6.3) zeitwerk (2.3.0) PLATFORMS ruby DEPENDENCIES aasm active_delivery acts-as-taggable-on (~> 6.0) annotate awesome_print aws-sdk-rails (~> 3) aws-sdk-s3 aws-sdk-sns bootsnap (>= 1.4.2) bullet byebug capistrano (~> 3.11) capistrano-bundler capistrano-multiconfig (>= 3.0.3) capistrano-rails capistrano-rake capistrano-rvm capistrano3-puma database_cleaner devise doorkeeper doorkeeper-jwt dotenv-rails factory_bot_rails faker geocoder haml image_processing jsonapi-resources! listen (>= 3.0.5, < 3.2) mailcatcher pg pronto pronto-rubocop puma (~> 4.1) rack-cors rails (~> 6.0.2) redis rspec-rails rswag rswag-ui rubocop rubocop-git rubocop-rspec secret_api! seed-fu shoulda-matchers sidekiq simplecov spring spring-commands-rspec spring-watcher-listen (~> 2.0.0) sshkit timecop tzinfo-data whenever RUBY VERSION ruby 2.6.5p114 BUNDLED WITH 2.0.1 ```