clean up auth and allow specifying request parameters

lib/aws/s3/authentication.rb

@@ -61,7 +61,7 @@ def initialize(request, access_key_id, secret_access_key, options = {})
         private
 
           def canonical_string            
-            options = {}
+            options = @options.slice(*CanonicalString::SIGNIFICANT_PARAMETERS)
             options[:expires] = expires if expires?
             CanonicalString.new(request, options)
           end
@@ -204,18 +204,63 @@ def set_default_headers
             end
           end
 
+          # see http://docs.amazonwebservices.com/AmazonS3/latest/dev/index.html?RESTAuthentication.html#ConstructingTheCanonicalizedResourceElement
           def path
-            [only_path, extract_significant_parameter].compact.join('?')
+            [only_path, extract_significant_parameters].compact.join('?')
           end
 
-          def extract_significant_parameter
-            request.path[/[&?](acl|torrent|logging)(?:&|=|$)/, 1]
+          SIGNIFICANT_PARAMETERS = [
+            'acl', 'location', 'logging', 'notification', 'partNumber',
+            'policy', 'requestPayment', 'torrent', 'uploadId', 'uploads',
+            'versionId', 'versioning', 'versions', 'website',
+            'response-content-type', 'response-content-language',
+            'response-expires', 'response-cache-control',
+            'response-content-disposition', 'response-content-encoding'
+          ]
+
+          def extract_significant_parameters
+            # only the last value for each key, with preference to those still
+            # in the options hash, will be included in the canonicalized
+            # resource
+            parameters = {}
+
+            # significant parameters may already be in the query string of the
+            # request's path (with values CGI escaped)...
+            if request.path['?']
+              request.path.split('?', 2).last.split('&').each do |p|
+                key, value = p.split('=', 2)
+                next unless SIGNIFICANT_PARAMETERS.include?(key)
+                parameters[key] = value && CGI.unescape(value)
+              end
+            end
+
+            # ...or they may be in the options that will eventually make their
+            # way into the query string (with values not yet CGI escaped)
+            @options.each do |key,value|
+              # treat symbols and string equally (as the string)
+              key = key.to_s
+              next unless SIGNIFICANT_PARAMETERS.include?(key)
+              parameters[key] = value
+            end
+
+            return nil if parameters.empty?
+
+            parameters.keys.sort.map do |key|
+              if parameters[key]
+                # we specifically don't do CGI escaping on the values going
+                # into the signature string
+                [key, parameters[key]].join('=')
+              else
+                key
+              end
+            end.join('&')
           end
 
           def only_path
-            request.path[/^[^?]*/]
+            return request.path unless request.path['?']
+            request.path.split('?', 2).first
           end
       end
     end
   end
-end
+end

lib/aws/s3/connection.rb

@@ -62,10 +62,11 @@ def url_for(path, options = {})
         authenticate = true if authenticate.nil? 
         path         = self.class.prepare_path(path)
         request      = request_method(:get).new(path, {})
-        query_string = query_string_authentication(request, options)
-        returning "#{protocol(options)}#{http.address}#{port_string}#{path}" do |url|
-          url << "?#{query_string}" if authenticate
-        end
+        query_segments = permissible_request_parameters(options)
+        query_segments << query_string_authentication(request, options) if authenticate
+        url = "#{protocol(options)}#{http.address}#{port_string}#{path}"
+        url << (url['?'] ? '&' : '?') << query_segments.join('&') unless query_segments.empty?
+        url
       end
 
       def subdomain
@@ -134,6 +135,22 @@ def add_user_agent!(request)
           request['User-Agent'] ||= "AWS::S3/#{Version}"
         end
 
+        def permissible_request_parameters(options = {})
+          # fill out parameters given options according to
+          # http://docs.amazonwebservices.com/AmazonS3/latest/API/index.html?RESTObjectGET.html
+          # section Requests subsection Request Parameters and
+          # http://docs.amazonwebservices.com/AmazonS3/latest/dev/index.html?RESTAuthentication.html#ConstructingTheCanonicalizedResourceElement
+          [ 'acl', 'location', 'logging', 'notification', 'partNumber',
+            'policy', 'requestPayment', 'torrent', 'uploadId', 'uploads',
+            'versionId', 'versioning', 'versions', 'website',
+            'response-content-type', 'response-content-language',
+            'response-expires', 'response-cache-control',
+            'response-content-disposition', 'response-content-encoding' ].map do |key|
+            key = key.to_sym unless options.has_key?(key)
+            options.has_key?(key) ? [key, CGI.escape(options[key])].join('=') : nil
+          end.compact
+        end
+
         def query_string_authentication(request, options = {})
           Authentication::QueryString.new(request, access_key_id, secret_access_key, options)
         end
@@ -275,4 +292,4 @@ def validate(options)
       end
     end
   end
-end
+end

test/authentication_test.rb

@@ -74,22 +74,64 @@ def test_path_does_not_include_query_string
     assert_equal '/foo/bar', Authentication::CanonicalString.new(request).send(:path)
   end
 
+  # quotes in comments in these next few tests refer to
+  # http://docs.amazonwebservices.com/AmazonS3/latest/dev/index.html?RESTAuthentication.html#ConstructingTheCanonicalizedResourceElement
   def test_path_includes_significant_query_strings
+    # "If the request addresses a sub-resource, like ?versioning, ?location,
+    # ?acl, or ?torrent, or ?versionid append the sub-resource, its value if it
+    # has one, and the question mark. Note that in case of multiple
+    # sub-resources, sub-resources must be lexicographically sorted by
+    # sub-resource name and separated by '&'. e.g. ?acl&versionId=value."
     significant_query_strings = [
-      ['/test/query/string?acl',             '/test/query/string?acl'],
-      ['/test/query/string?acl&foo=bar',     '/test/query/string?acl'],
-      ['/test/query/string?foo=bar&acl',     '/test/query/string?acl'],
-      ['/test/query/string?acl=foo',         '/test/query/string?acl'],
-      ['/test/query/string?torrent=foo',     '/test/query/string?torrent'],
-      ['/test/query/string?logging=foo',     '/test/query/string?logging'],
-      ['/test/query/string?bar=baz&acl=foo', '/test/query/string?acl']
+      ['/test/query/string?acl',                 '/test/query/string?acl'],
+      ['/test/query/string?acl&foo=bar',         '/test/query/string?acl'],
+      ['/test/query/string?foo=bar&acl',         '/test/query/string?acl'],
+      ['/test/query/string?acl=foo',             '/test/query/string?acl=foo'],
+      ['/test/query/string?torrent=foo',         '/test/query/string?torrent=foo'],
+      ['/test/query/string?logging=foo',         '/test/query/string?logging=foo'],
+      ['/test/query/string?bar=baz&acl=foo',     '/test/query/string?acl=foo'],
+      ['/test/query/string?acl=foo&torrent=bar', '/test/query/string?acl=foo&torrent=bar'],
+      ['/test/query/string?torrent=bar&acl=foo', '/test/query/string?acl=foo&torrent=bar']
     ]
 
     significant_query_strings.each do |uncleaned_path, expected_cleaned_path|
       assert_equal expected_cleaned_path, Authentication::CanonicalString.new(Net::HTTP::Get.new(uncleaned_path)).send(:path)
     end
   end
 
+  def test_path_includes_significant_query_strings_from_options
+    assert_equal '/test/query/string?acl=foo', Authentication::CanonicalString.new(Net::HTTP::Get.new('/test/query/string'), :acl => 'foo').send(:path)
+    assert_equal '/test/query/string?acl&torrent=foo', Authentication::CanonicalString.new(Net::HTTP::Get.new('/test/query/string?acl'), :torrent => 'foo').send(:path)
+  end
+
+  def test_path_includes_significant_query_strings_with_unencoded_values
+    # "When signing you do not encode these values. However, when making the
+    # request, you must encode these parameter values."
+    assert_equal '/test/query/string?acl=foo bar', Authentication::CanonicalString.new(Net::HTTP::Get.new('/test/query/string?acl=foo+bar')).send(:path)
+  end
+
+  def test_path_recognizes_all_significant_query_strings
+    # "The list of sub-resources that must be included when constructing the
+    # CanonicalizedResource Element are: acl, location, logging, notification,
+    # partNumber, policy, requestPayment, torrent, uploadId, uploads,
+    # versionId, versioning, versions and website...  If the request specifies
+    # query string parameters overriding the response header values... append
+    # the query string parameters, and its values... The query string
+    # parameters in a GET request include response-content-type,
+    # response-content-language, response-expires, response-cache-control,
+    # response-content-disposition, and response-content-encoding."
+    base_path = '/test/query/string'
+    [ 'acl', 'location', 'logging', 'notification', 'partNumber', 'policy',
+      'requestPayment', 'torrent', 'uploadId', 'uploads', 'versionId',
+      'versioning', 'versions', 'website', 'response-content-type',
+      'response-content-language', 'response-expires',
+      'response-cache-control', 'response-content-disposition',
+      'response-content-encoding' ].each do |significant_parameter|
+      path = "#{base_path}?#{significant_parameter}"
+      assert_equal path, Authentication::CanonicalString.new(Net::HTTP::Get.new(path)).send(:path)
+    end
+  end
+
   def test_default_headers_set
     Authentication::CanonicalString.default_headers.each do |header|
       assert @canonical_string.headers.include?(header)
@@ -111,4 +153,4 @@ def test_canonical_string
     request = AmazonDocExampleData::Example1.request
     assert_equal AmazonDocExampleData::Example1.canonical_string, Authentication::CanonicalString.new(request)
   end
-end
+end

test/connection_test.rb

@@ -95,12 +95,53 @@ def test_url_for_remembers_custom_protocol_server_and_port
     assert_match %r(^https://example\.org:555/foo\?), connection.url_for('/foo')
   end
 
+  def assert_url_has_query_parameter(url, key, value=:unspecified)
+    if value == :unspecified
+      assert_match /\?([^&]+&)*#{Regexp.escape(key)}(=|&|$)/, url
+    elsif value.nil?
+      assert_match /\?([^&]+&)*#{Regexp.escape(key)}=?(&|$)/, url
+    else
+      assert_match /\?([^&]+&)*#{Regexp.escape(key)}=#{Regexp.escape(CGI.escape(value.to_s))}/, url
+    end
+  end
+
+  def assert_url_lacks_query_parameter(url, key)
+    assert_no_match /\?([^&]+&)*#{Regexp.escape(key)}(=|&|$)/, url
+  end
+
+  def assert_url_authenticated(url)
+    assert_url_has_query_parameter(url, 'AWSAccessKeyId')
+  end
+
+  def assert_url_unauthenticated(url)
+    assert_url_lacks_query_parameter(url, 'AWSAccessKeyId')
+  end
+
   def test_url_for_with_and_without_authenticated_urls
     connection = Connection.new(:access_key_id => '123', :secret_access_key => 'abc', :server => 'example.org')
     authenticated = lambda {|url| url['?AWSAccessKeyId']}
-    assert authenticated[connection.url_for('/foo')]
-    assert authenticated[connection.url_for('/foo', :authenticated => true)]
-    assert !authenticated[connection.url_for('/foo', :authenticated => false)]
+    assert_url_authenticated connection.url_for('/foo')
+    assert_url_authenticated connection.url_for('/foo', :authenticated => true)
+    assert_url_unauthenticated connection.url_for('/foo', :authenticated => false)
+  end
+
+  def test_url_for_with_request_parameter
+    connection = Connection.new(:access_key_id => '123', :secret_access_key => 'abc', :server => 'example.org')
+    url = connection.url_for('/foo', 'response-content-disposition' => 'attachment; foo.txt')
+    assert_url_has_query_parameter url, 'response-content-disposition', 'attachment; foo.txt'
+  end
+
+  def test_url_for_with_unknown_request_parameter
+    connection = Connection.new(:access_key_id => '123', :secret_access_key => 'abc', :server => 'example.org')
+    url = connection.url_for('/foo', 'random-parameter' => 'attachment; foo.txt')
+    assert_url_lacks_query_parameter url, 'random-parameter'
+  end
+
+  def test_url_for_with_authenticated_and_request_parameter
+    connection = Connection.new(:access_key_id => '123', :secret_access_key => 'abc', :server => 'example.org')
+    url = connection.url_for('/foo', :authenticated => true, 'response-content-disposition' => 'attachment; foo.txt')
+    assert_url_has_query_parameter url, 'response-content-disposition', 'attachment; foo.txt'
+    assert_url_authenticated url
   end
 
   def test_connecting_through_a_proxy