Skip to content
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

It is not possible to revoke refresh token bound to the expired access token #1671

Open
syakovyn opened this issue Sep 18, 2023 · 3 comments

Comments

@syakovyn
Copy link

syakovyn commented Sep 18, 2023

A copy of #1579 but with a focus on refresh tokens.

Steps to reproduce

Request to revoke a refresh token bound to the expired access token.

Expected behavior

The refresh token is revoked and can not be used to get a new access token.

Actual behavior

The refresh token is not revoked and can be used to get a new access token.

System configuration

Doorkeeper initializer:

# config/initializers/doorkeeper.rb
Doorkeeper.configure do
  ...
  use_refresh_token
  use_polymorphic_resource_owner
  ...
end

Ruby version: 3.1.4

Gemfile.lock:

Gemfile.lock content
...
doorkeeper (5.6.6)
...

Reproduction tests

RSpec test to show the issue (simplified):

# Simplified User that has no validations and has only an ID column. That is enough to reproduce the issue
class User < ActiveRecord::Base
end

# The spec
require 'rails_helper'

describe Doorkeeper::TokensController, type: :request do
  it 'revokes refresh token bound to expired access token' do
    mobile_app = Doorkeeper::Application.create!(name: 'Mobile App', redirect_uri: 'mobile://callback', confidential: false)
    user = User.create!
    access_token =  Dookeeper::AccessToken.create!(resource_owner: user, application: mobile_app, use_refresh_token: true, expires_in: 0) # Using expires_in = 0 to create an expired access token

    post '/oauth/revoke', params: { client_id: mobile_app.uid, token: access_token.refresh_token, token_type_hint: 'refresh_token' }

    expect(response).to have_http_status :ok
    expect(access_token.reload).to be_revoked # That means the refresh token remains valid despite the fact that the revoke endpoint returned OK

    # Comment`expect(access_token.reload).to be_revoked` to get here
    basic_auth = "Basic #{Base64.strict_encode64("#{mobile_app.uid}:#{mobile_app.secret}")}"
    post '/oauth/token', params: { grant_type: :refresh_token, refresh_token: access_token.refresh_token }, headers: { Authorization: basic_auth }

    expect(response).not_to have_http_status :ok # It fails meaning that the "revoked" refresh token can be successfully used to get a new access token.
  end
end
@syakovyn
Copy link
Author

Moreover, introspection of such a refresh token (bound to an expired access token) will respond with a misleading "active": false.

@ThisIsMissEm
Copy link
Contributor

I think this is because the refresh tokens aren't actually Doorkeeper::AccessTokens themselves, but just a string value on an AccessToken, so you can't "revoke" the refresh token, but you can revoke the access token it belongs to.

I think what you're seeing with the revoke endpoint is that in order to not disclose that the token did exist, it's always return 200, regardless of whether or not a token was revoked.

@mquan
Copy link

mquan commented Dec 1, 2023

imo this is a bug. The token parameter used in the oauth/revoke endpoint is the refresh_token, the expires_at field is only applicable to the access_token.

Taking a step back, the point of revocation is to remove further usages. It's a serious flaw if tokens cannot be revoked b/c of expiration but can be refreshed. It's kinda silly to workaround by refreshing for a new token in order to revoke.

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

3 participants