Skip to content

Commit

Permalink
Revoke AccessGrants along with AccessTokens
Browse files Browse the repository at this point in the history
- When revoking authorization for an application, revoke both
  AccessTokens and AccessGrants via Application.revoke_all_access_for
- Adds AccessGrant.revoke_all_for
- Update AccessToken.revoke_all_for specs to verify scope is respected.
  These specs asserted that `AccessToken.all` wasn't empty but `.all`
  returns tokens that are revoked and therefore the tests weren't
  actually verifying that the `revoke_all_for` method didn't revoke
  `AccessToken`s outside of the intended scope.
  • Loading branch information
id4ho committed Jul 2, 2018
1 parent 3b61b6c commit 9e010ce
Show file tree
Hide file tree
Showing 7 changed files with 99 additions and 5 deletions.
2 changes: 2 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ User-visible changes worth mentioning.
- [#1106] Restrict access to AdminController with 'Forbidden 403' if admin_authenticator is not
configured by developers..
- [#1108] Simple formating of callback URLs when listing oauth applications
- [#1116] `AccessGrant`s will now be revoked along with `AccessToken`s when
hitting the `AuthorizedApplicationController#destroy` route.

## 5.0.0.rc1

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ def index
end

def destroy
AccessToken.revoke_all_for params[:id], current_resource_owner
Application.revoke_all_access_for params[:id], current_resource_owner

respond_to do |format|
format.html do
Expand Down
15 changes: 15 additions & 0 deletions lib/doorkeeper/models/access_grant_mixin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,21 @@ def by_token(token)
find_by(token: token.to_s)
end

# Revokes AccessGrant records that have not been revoked and associated
# with the specific Application and Resource Owner.
#
# @param application_id [Integer]
# ID of the Application
# @param resource_owner [ActiveRecord::Base]
# instance of the Resource Owner model
#
def revoke_all_for(application_id, resource_owner, clock = Time)
where(application_id: application_id,
resource_owner_id: resource_owner.id,
revoked_at: nil).
update_all(revoked_at: clock.now.utc)
end

# Implements PKCE code_challenge encoding without base64 padding as described in the spec.
# https://tools.ietf.org/html/rfc7636#appendix-A
# Appendix A. Notes on Implementing Base64url Encoding without Padding
Expand Down
11 changes: 11 additions & 0 deletions lib/doorkeeper/orm/active_record/application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,17 @@ def self.authorized_for(resource_owner)
where(id: resource_access_tokens.select(:application_id).distinct)
end

# Revokes AccessToken and AccessGrant records that have not been revoked and
# associated with the specific Application and Resource Owner.
#
# @param resource_owner [ActiveRecord::Base]
# instance of the Resource Owner model
#
def self.revoke_all_access_for(id, resource_owner)
AccessToken.revoke_all_for(id, resource_owner)
AccessGrant.revoke_all_for(id, resource_owner)
end

private

def generate_uid
Expand Down
43 changes: 43 additions & 0 deletions spec/models/doorkeeper/access_grant_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,47 @@
expect(subject).not_to be_valid
end
end

describe '.revoke_all_for' do
let(:resource_owner) { double(id: 100) }
let(:application) { FactoryBot.create :application }
let(:default_attributes) do
{
application: application,
resource_owner_id: resource_owner.id
}
end

it 'revokes all tokens for given application and resource owner' do
FactoryBot.create :access_grant, default_attributes

described_class.revoke_all_for(application.id, resource_owner)

described_class.all.each do |token|
expect(token).to be_revoked
end
end

it 'matches application' do
access_grant_for_different_app = FactoryBot.create(
:access_grant,
default_attributes.merge(application: FactoryBot.create(:application))
)

described_class.revoke_all_for(application.id, resource_owner)

expect(access_grant_for_different_app.reload).not_to be_revoked
end

it 'matches resource owner' do
access_grant_for_different_owner = FactoryBot.create(
:access_grant,
default_attributes.merge(resource_owner_id: 90)
)

described_class.revoke_all_for application.id, resource_owner

expect(access_grant_for_different_owner.reload).not_to be_revoked
end
end
end
18 changes: 14 additions & 4 deletions spec/models/doorkeeper/access_token_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -306,15 +306,25 @@ def self.generate(_opts = {})
end

it 'matches application' do
FactoryBot.create :access_token, default_attributes.merge(application: FactoryBot.create(:application))
access_token_for_different_app = FactoryBot.create(
:access_token,
default_attributes.merge(application: FactoryBot.create(:application))
)

AccessToken.revoke_all_for application.id, resource_owner
expect(AccessToken.all).not_to be_empty

expect(access_token_for_different_app.reload).not_to be_revoked
end

it 'matches resource owner' do
FactoryBot.create :access_token, default_attributes.merge(resource_owner_id: 90)
access_token_for_different_owner = FactoryBot.create(
:access_token,
default_attributes.merge(resource_owner_id: 90)
)

AccessToken.revoke_all_for application.id, resource_owner
expect(AccessToken.all).not_to be_empty

expect(access_token_for_different_owner.reload).not_to be_revoked
end
end

Expand Down
13 changes: 13 additions & 0 deletions spec/models/doorkeeper/application_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,19 @@ module Doorkeeper
end
end

describe :revoke_all_access_for do
it 'revokes all access tokens and access grants' do
application_id = 42
resource_owner = double
expect(Doorkeeper::AccessToken).
to receive(:revoke_all_for).with(application_id, resource_owner)
expect(Doorkeeper::AccessGrant).
to receive(:revoke_all_for).with(application_id, resource_owner)

Application.revoke_all_access_for(application_id, resource_owner)
end
end

describe :by_uid_and_secret do
context "when application is private/confidential" do
it "finds the application via uid/secret" do
Expand Down

0 comments on commit 9e010ce

Please sign in to comment.