From e2b8dabbf4174dea0c26569ede7ecbc95c2a62f1 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Wed, 4 Dec 2024 14:12:39 -0800 Subject: [PATCH] Fix refresh tokens with dynamic scopes Suppose your OAuth application supports the scopes `public user:*`. Previously a refresh token with the `public user:1` scope returned a scope of `public` by default, unless `user:1` were explicitly requested. This happened because `Doorkeeper::Oauth::BaseRequest` does a set intersection: "public user:*" & "public user:1" = "public" With dynamic scopes, this doesn't work because the wildcard `user:*` doesn't match `user:1`. To fix this, introduce `Scopes#allowed` and deprecate the use of `&`. --- CHANGELOG.md | 1 + lib/doorkeeper/oauth/base_request.rb | 3 +- lib/doorkeeper/oauth/pre_authorization.rb | 2 +- lib/doorkeeper/oauth/scopes.rb | 18 +++++++ spec/lib/oauth/refresh_token_request_spec.rb | 49 ++++++++++++++++++++ spec/lib/oauth/scopes_spec.rb | 41 ++++++++++++++++ 6 files changed, 112 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a86f9b73..e681ef566 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ Add your entry here. - [#1752] Bump the range of supported Ruby and Rails versions - [#1747] Fix unknown pkce method error when configured - [#1744] Allow for expired refresh tokens to be revoked +- [#1754] Fix refresh tokens with dynamic scopes ## 5.8.0 diff --git a/lib/doorkeeper/oauth/base_request.rb b/lib/doorkeeper/oauth/base_request.rb index 8d67466f3..81a804201 100644 --- a/lib/doorkeeper/oauth/base_request.rb +++ b/lib/doorkeeper/oauth/base_request.rb @@ -59,7 +59,8 @@ def build_scopes client_scopes = @client&.scopes return default_scopes if client_scopes.blank? - default_scopes & client_scopes + # Avoid using Scope#& for dynamic scopes + client_scopes.allowed(default_scopes) end end end diff --git a/lib/doorkeeper/oauth/pre_authorization.rb b/lib/doorkeeper/oauth/pre_authorization.rb index 050bc2d4f..3c2dab201 100644 --- a/lib/doorkeeper/oauth/pre_authorization.rb +++ b/lib/doorkeeper/oauth/pre_authorization.rb @@ -75,7 +75,7 @@ def build_scopes if client_scopes.blank? server.default_scopes.to_s else - (server.default_scopes & client_scopes).to_s + server.default_scopes.allowed(client_scopes).to_s end end diff --git a/lib/doorkeeper/oauth/scopes.rb b/lib/doorkeeper/oauth/scopes.rb index 21a6f1a91..8dd547294 100644 --- a/lib/doorkeeper/oauth/scopes.rb +++ b/lib/doorkeeper/oauth/scopes.rb @@ -70,10 +70,28 @@ def <=>(other) end end + # DEPRECATED: With dynamic scopes, #allowed should be called because + # A & B doesn't really make sense with dynamic scopes. + # + # For example, if A = user:* and B is user:1, A & B = []. + # If we modified this method to take dynamic scopes into an account, then order + # becomes important, and this would violate the principle that A & B = B & A. def &(other) + return allowed(other) if dynamic_scopes_enabled? + self.class.from_array(all & to_array(other)) end + # Returns a set of scopes that are allowed, taking dynamic + # scopes into account. This instance's scopes is taken as the allowed set, + # and the passed value is the set to filter. + # + # @param other The set of scopes to filter + def allowed(other) + filtered_scopes = other.select { |scope| self.exists?(scope) } + self.class.from_array(filtered_scopes) + end + private def dynamic_scopes_enabled? diff --git a/spec/lib/oauth/refresh_token_request_spec.rb b/spec/lib/oauth/refresh_token_request_spec.rb index b40cb7fab..79fbd0c3e 100644 --- a/spec/lib/oauth/refresh_token_request_spec.rb +++ b/spec/lib/oauth/refresh_token_request_spec.rb @@ -163,4 +163,53 @@ expect(request.error).to eq(Doorkeeper::Errors::InvalidScope) end end + + context "with dynamic scopes enabled" do + subject(:request) { described_class.new(server, refresh_token, credentials, parameters) } + + let(:application_scopes) { "public write user:*" } + let(:application) { FactoryBot.create(:application, scopes: application_scopes) } + let(:token_scopes) { "public write user:1" } + + let(:refresh_token) do + FactoryBot.create :access_token, + use_refresh_token: true, + scopes: token_scopes, + application: application + end + + let(:parameters) { {} } + + before do + Doorkeeper.configure do + enable_dynamic_scopes + end + end + + it "transfers scopes from the old token to the new token" do + request.authorize + expect(Doorkeeper::AccessToken.last.scopes).to eq(%i[public write user:1]) + end + + it "returns an error with invalid scope" do + parameters[:scopes] = "public garbage:*" + + response = request.authorize + + expect(response).to be_a(Doorkeeper::OAuth::ErrorResponse) + expect(response.status).to eq(:bad_request) + end + + it "reduces scopes to the dynamic scope" do + parameters[:scopes] = "user:1" + request.authorize + expect(Doorkeeper::AccessToken.last.scopes).to eq(%i[user:1]) + end + + it "reduces scopes to the public scope" do + parameters[:scopes] = "public" + request.authorize + expect(Doorkeeper::AccessToken.last.scopes).to eq(%i[public]) + end + end end diff --git a/spec/lib/oauth/scopes_spec.rb b/spec/lib/oauth/scopes_spec.rb index 6f1fc3aba..866d1d490 100644 --- a/spec/lib/oauth/scopes_spec.rb +++ b/spec/lib/oauth/scopes_spec.rb @@ -118,6 +118,13 @@ end end + describe "#allowed" do + it "can get intersection with another scope object" do + scopes = described_class.from_string("public admin").allowed(described_class.from_string("write admin")) + expect(scopes.all).to eq(%w[admin]) + end + end + describe "#has_scopes?" do subject(:scopes) { described_class.from_string("public admin") } @@ -192,6 +199,40 @@ it "returns false if dynamic scope does not match" do expect(scopes).not_to have_scopes(described_class.from_string("public userA:1")) end + + describe "#&" do + it "allows user:1 scope" do + scopes = described_class.from_string("public user:*") & (described_class.from_string("public user:1")) + expect(scopes.all).to eq(%w[public user:1]) + end + + it "does not allow user:2 scope" do + scopes = described_class.from_string("public user:1") & (described_class.from_string("public user:2")) + expect(scopes.all).to eq(%w[public]) + end + + it "does not allow user:* scope" do + scopes = described_class.from_string("public user:1") & (described_class.from_string("public user:*")) + expect(scopes.all).to eq(%w[public]) + end + end + + describe "#allowed" do + it "allows user:1 scope" do + scopes = described_class.from_string("public user:*").allowed(described_class.from_string("public user:1")) + expect(scopes.all).to eq(%w[public user:1]) + end + + it "does not allow user:2 scope" do + scopes = described_class.from_string("public user:1").allowed(described_class.from_string("public user:2")) + expect(scopes.all).to eq(%w[public]) + end + + it "does not allow user:* scope" do + scopes = described_class.from_string("public user:1").allowed(described_class.from_string("public user:*")) + expect(scopes.all).to eq(%w[public]) + end + end end context "with specific dynamic scope" do