Merge pull request #1754 from stanhu/sh-fix-refresh-token-with-dynami…

…c-scopes

Fix refresh tokens with dynamic scopes

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
 

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

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
 

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?

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

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