Merge pull request #1053 from f3ndot/support-redirect_uris-with-query

Support redirect_uri's with registered query param(s)

lib/doorkeeper/oauth/helpers/uri_checker.rb

@@ -12,6 +12,13 @@ def self.valid?(url)
         def self.matches?(url, client_url)
           url = as_uri(url)
           client_url = as_uri(client_url)
+
+          if client_url.query.present?
+            return false unless query_matches?(url.query, client_url.query)
+            # Clear out queries so rest of URI can be tested. This allows query
+            # params to be in the request but order not mattering.
+            client_url.query = nil
+          end
           url.query = nil
           url == client_url
         end
@@ -24,6 +31,13 @@ def self.as_uri(url)
           URI.parse(url)
         end
 
+        def self.query_matches?(query, client_query)
+          return true if client_query.nil? && query.nil?
+          return false if client_query.nil? || query.nil?
+          # Will return true independent of query order
+          client_query.split('&').sort == query.split('&').sort
+        end
+
         def self.native_uri?(url)
           url == Doorkeeper.configuration.native_redirect_uri
         end

spec/lib/oauth/helpers/uri_checker_spec.rb

@@ -69,6 +69,44 @@ module Doorkeeper::OAuth::Helpers
         client_uri = 'http://example.com?app.co=test'
         expect(URIChecker.matches?(uri, client_uri)).to be_falsey
       end
+
+      context "client registered query params" do
+        it "doesn't allow query being absent" do
+          uri = 'http://app.co'
+          client_uri = 'http://app.co/?vendorId=AJ4L7XXW9'
+          expect(URIChecker.matches?(uri, client_uri)).to be_falsey
+        end
+
+        it "is false if query values differ but key same" do
+          uri = 'http://app.co/?vendorId=pancakes'
+          client_uri = 'http://app.co/?vendorId=waffles'
+          expect(URIChecker.matches?(uri, client_uri)).to be_falsey
+        end
+
+        it "is false if query values same but key differs" do
+          uri = 'http://app.co/?foo=pancakes'
+          client_uri = 'http://app.co/?bar=pancakes'
+          expect(URIChecker.matches?(uri, client_uri)).to be_falsey
+        end
+
+        it "is false if query present and match, but unknown queries present" do
+          uri = 'http://app.co/?vendorId=pancakes&unknown=query'
+          client_uri = 'http://app.co/?vendorId=waffles'
+          expect(URIChecker.matches?(uri, client_uri)).to be_falsey
+        end
+
+        it "is true if queries are present and matche" do
+          uri = 'http://app.co/?vendorId=AJ4L7XXW9&foo=bar'
+          client_uri = 'http://app.co/?vendorId=AJ4L7XXW9&foo=bar'
+          expect(URIChecker.matches?(uri, client_uri)).to be_truthy
+        end
+
+        it "is true if queries are present, match and in different order" do
+          uri = 'http://app.co/?bing=bang&foo=bar'
+          client_uri = 'http://app.co/?foo=bar&bing=bang'
+          expect(URIChecker.matches?(uri, client_uri)).to be_truthy
+        end
+      end
     end
 
     describe '.valid_for_authorization?' do
@@ -101,9 +139,75 @@ module Doorkeeper::OAuth::Helpers
       end
 
       it 'is false if invalid' do
-        uri = client_uri = 'http://app.co/aaa?waffles=abc'
+        uri = 'http://app.co/aaa?pankcakes=abc'
+        client_uri = 'http://app.co/aaa?waffles=abc'
         expect(URIChecker.valid_for_authorization?(uri, client_uri)).to be false
       end
+
+      it 'calls .matches?' do
+        uri = 'http://app.co/aaa?pankcakes=abc'
+        client_uri = 'http://app.co/aaa?waffles=abc'
+        expect(URIChecker).to receive(:matches?).with(uri, client_uri).once
+        URIChecker.valid_for_authorization?(uri, client_uri)
+      end
+
+      it 'calls .valid?' do
+        uri = 'http://app.co/aaa?pankcakes=abc'
+        client_uri = 'http://app.co/aaa?waffles=abc'
+        expect(URIChecker).to receive(:valid?).with(uri).once
+        URIChecker.valid_for_authorization?(uri, client_uri)
+      end
+    end
+
+    describe '.query_matches?' do
+      it 'is true if no queries' do
+        expect(URIChecker.query_matches?('', '')).to be_truthy
+        expect(URIChecker.query_matches?(nil, nil)).to be_truthy
+      end
+
+      it 'is true if same query' do
+        expect(URIChecker.query_matches?('foo', 'foo')).to be_truthy
+      end
+
+      it 'is false if different query' do
+        expect(URIChecker.query_matches?('foo', 'bar')).to be_falsey
+      end
+
+      it 'is true if same queries' do
+        expect(URIChecker.query_matches?('foo&bar', 'foo&bar')).to be_truthy
+      end
+
+      it 'is true if same queries, different order' do
+        expect(URIChecker.query_matches?('foo&bar', 'bar&foo')).to be_truthy
+      end
+
+      it 'is false if one different query' do
+        expect(URIChecker.query_matches?('foo&bang', 'foo&bing')).to be_falsey
+      end
+
+      it 'is true if same query with same value' do
+        expect(URIChecker.query_matches?('foo=bar', 'foo=bar')).to be_truthy
+      end
+
+      it 'is true if same queries with same values' do
+        expect(URIChecker.query_matches?('foo=bar&bing=bang', 'foo=bar&bing=bang')).to be_truthy
+      end
+
+      it 'is true if same queries with same values, different order' do
+        expect(URIChecker.query_matches?('foo=bar&bing=bang', 'bing=bang&foo=bar')).to be_truthy
+      end
+
+      it 'is false if same query with different value' do
+        expect(URIChecker.query_matches?('foo=bar', 'foo=bang')).to be_falsey
+      end
+
+      it 'is false if some queries missing' do
+        expect(URIChecker.query_matches?('foo=bar', 'foo=bar&bing=bang')).to be_falsey
+      end
+
+      it 'is false if some queries different value' do
+        expect(URIChecker.query_matches?('foo=bar&bing=bang', 'foo=bar&bing=banana')).to be_falsey
+      end
     end
   end
 end