generate safe file urls relative to the shard of the files domain

allows it to be a different shard than where the file is. I had
to remove type casting from dynamic finders that don't know how
to deal with non-integral global ids.

also cache s3 urls on the same shard as the attachment

test plan:
 * have multiple shards and S3 storage
 * have a single safe files domain
 * you should be able to upload and download files in all shards
 * verify that it's going against the files domain, not the normal
   domain

Change-Id: I2b498fc1df20d5b43bf20f702580451621eeaf6a
Reviewed-on: https://gerrit.instructure.com/15158
Tested-by: Jenkins <jenkins@instructure.com>
Reviewed-by: Brian Palmer <brianp@instructure.com>

Author: ccutrer

app/controllers/application_controller.rb

@@ -1052,40 +1052,47 @@ def conversations_path(params={})
 
   # escape everything but slashes, see http://code.google.com/p/phusion-passenger/issues/detail?id=113
   FILE_PATH_ESCAPE_PATTERN = Regexp.new("[^#{URI::PATTERN::UNRESERVED}/]")
-  def safe_domain_file_url(attachment, host=nil, verifier = nil, download = false) # TODO: generalize this
-    res = "#{request.protocol}#{host || HostUrl.file_host(@domain_root_account || Account.default, request.host_with_port)}"
-    ts, sig = @current_user && @current_user.access_verifier
-
-    # add parameters so that the other domain can create a session that 
-    # will authorize file access but not full app access.  We need this in 
-    # case there are relative URLs in the file that point to other pieces 
-    # of content.
-    opts = { :user_id => @current_user.try(:id), :ts => ts, :sf_verifier => sig }
-    opts[:verifier] = verifier if verifier.present?
-
-    if download
-      # download "for realz, dude" (see later comments about :download)
-      opts[:download_frd] = 1
-    else
-      # don't set :download here, because file_download_url won't like it. see
-      # comment below for why we'd want to set :download
-      opts[:inline] = 1
+  def safe_domain_file_url(attachment, host_and_shard=nil, verifier = nil, download = false) # TODO: generalize this
+    if !host_and_shard
+      host_and_shard = HostUrl.file_host_with_shard(@domain_root_account || Account.default, request.host_with_port)
     end
+    host, shard = host_and_shard
+    res = "#{request.protocol}#{host}"
+
+    shard.activate do
+      ts, sig = @current_user && @current_user.access_verifier
+
+      # add parameters so that the other domain can create a session that
+      # will authorize file access but not full app access.  We need this in
+      # case there are relative URLs in the file that point to other pieces
+      # of content.
+      opts = { :user_id => @current_user.try(:id), :ts => ts, :sf_verifier => sig }
+      opts[:verifier] = verifier if verifier.present?
+
+      if download
+        # download "for realz, dude" (see later comments about :download)
+        opts[:download_frd] = 1
+      else
+        # don't set :download here, because file_download_url won't like it. see
+        # comment below for why we'd want to set :download
+        opts[:inline] = 1
+      end
 
-    if @context && Attachment.relative_context?(@context.class.base_ar_class) && @context == attachment.context
-      # so yeah, this is right. :inline=>1 wants :download=>1 to go along with
-      # it, so we're setting :download=>1 *because* we want to display inline.
-      opts[:download] = 1 unless download
-
-      # if the context is one that supports relative paths (which requires extra
-      # routes and stuff), then we'll build an actual named_context_url with the
-      # params for show_relative
-      res += named_context_url(@context, :context_file_url, attachment)
-      res += '/' + URI.escape(attachment.full_display_path, FILE_PATH_ESCAPE_PATTERN)
-      res += '?' + opts.to_query
-    else
-      # otherwise, just redirect to /files/:id
-      res += file_download_url(attachment, opts.merge(:only_path => true))
+      if @context && Attachment.relative_context?(@context.class.base_ar_class) && @context == attachment.context
+        # so yeah, this is right. :inline=>1 wants :download=>1 to go along with
+        # it, so we're setting :download=>1 *because* we want to display inline.
+        opts[:download] = 1 unless download
+
+        # if the context is one that supports relative paths (which requires extra
+        # routes and stuff), then we'll build an actual named_context_url with the
+        # params for show_relative
+        res += named_context_url(@context, :context_file_url, attachment)
+        res += '/' + URI.escape(attachment.full_display_path, FILE_PATH_ESCAPE_PATTERN)
+        res += '?' + opts.to_query
+      else
+        # otherwise, just redirect to /files/:id
+        res += file_download_url(attachment, opts.merge(:only_path => true))
+      end
     end
 
     res

app/controllers/files_controller.rb

@@ -342,7 +342,7 @@ def show_relative
   # won't be able to access or update data with AJAX requests.
   def safer_domain_available?
     if !@files_domain && request.host_with_port != HostUrl.file_host(@domain_root_account, request.host_with_port)
-      @safer_domain_host = HostUrl.file_host(@domain_root_account, request.host_with_port)
+      @safer_domain_host = HostUrl.file_host_with_shard(@domain_root_account, request.host_with_port)
     end
     !!@safer_domain_host
   end

app/models/attachment.rb

@@ -963,24 +963,26 @@ def cacheable_s3_inline_url
   end
 
   def cacheable_s3_urls
-    Rails.cache.fetch(['cacheable_s3_urls', self].cache_key, :expires_in => 24.hours) do
-      ascii_filename = Iconv.conv("ASCII//TRANSLIT//IGNORE", "UTF-8", display_name)
-
-      # response-content-disposition will be url encoded in the depths of
-      # aws-s3, doesn't need to happen here. we'll be nice and ghetto http
-      # quote the filename string, though.
-      quoted_ascii = ascii_filename.gsub(/([\x00-\x1f"\x7f])/, '\\\\\\1')
-
-      # awesome browsers will use the filename* and get the proper unicode filename,
-      # everyone else will get the sanitized ascii version of the filename
-      quoted_unicode = "UTF-8''#{URI.escape(display_name, /[^A-Za-z0-9.]/)}"
-      filename = %(filename="#{quoted_ascii}"; filename*=#{quoted_unicode})
-
-      # we need to have versions of the url for each content-disposition
-      {
-        'inline' => authenticated_s3_url(:expires_in => 6.days, "response-content-disposition" => "inline; " + filename),
-        'attachment' => authenticated_s3_url(:expires_in => 6.days, "response-content-disposition" => "attachment; " + filename)
-      }
+    self.shard.activate do
+      Rails.cache.fetch(['cacheable_s3_urls', self].cache_key, :expires_in => 24.hours) do
+        ascii_filename = Iconv.conv("ASCII//TRANSLIT//IGNORE", "UTF-8", display_name)
+
+        # response-content-disposition will be url encoded in the depths of
+        # aws-s3, doesn't need to happen here. we'll be nice and ghetto http
+        # quote the filename string, though.
+        quoted_ascii = ascii_filename.gsub(/([\x00-\x1f"\x7f])/, '\\\\\\1')
+
+        # awesome browsers will use the filename* and get the proper unicode filename,
+        # everyone else will get the sanitized ascii version of the filename
+        quoted_unicode = "UTF-8''#{URI.escape(display_name, /[^A-Za-z0-9.]/)}"
+        filename = %(filename="#{quoted_ascii}"; filename*=#{quoted_unicode})
+
+        # we need to have versions of the url for each content-disposition
+        {
+          'inline' => authenticated_s3_url(:expires_in => 6.days, "response-content-disposition" => "inline; " + filename),
+          'attachment' => authenticated_s3_url(:expires_in => 6.days, "response-content-disposition" => "attachment; " + filename)
+        }
+      end
     end
   end
   protected :cacheable_s3_urls

config/environments/development.rb

@@ -27,7 +27,6 @@
   # Raise an exception on finder type mismatch or nil arguments. Helps us catch
   # these bugs before they hit.
   Canvas.dynamic_finder_nil_arguments_error = :raise
-  Canvas.dynamic_finder_type_cast_error = :raise
 end
 
 # initialize cache store

config/environments/test.rb

@@ -36,7 +36,6 @@
 # Raise an exception on finder type mismatch or nil arguments. Helps us catch
 # these bugs before they hit.
 Canvas.dynamic_finder_nil_arguments_error = :raise
-Canvas.dynamic_finder_type_cast_error = :raise
 
 # eval <env>-local.rb if it exists
 Dir[File.dirname(__FILE__) + "/" + File.basename(__FILE__, ".rb") + "-*.rb"].each { |localfile| eval(File.new(localfile).read) }

config/initializers/active_record.rb

@@ -285,16 +285,7 @@ class DynamicFinderTypeError < Exception; end
   class << self
     def construct_attributes_from_arguments_with_type_cast(attribute_names, arguments)
       log_dynamic_finder_nil_arguments(attribute_names) if current_scoped_methods.nil? && arguments.flatten.compact.empty?
-      attributes = construct_attributes_from_arguments_without_type_cast(attribute_names, arguments)
-      attributes.each_pair do |attribute, value|
-        next unless column = columns.detect{ |col| col.name == attribute.to_s }
-        next if [value].flatten.compact.empty?
-        cast_value = [value].flatten.map{ |v| v.respond_to?(:quoted_id) ? v : column.type_cast(v) }
-        cast_value = cast_value.first unless value.is_a?(Array)
-        next if [value].flatten.map(&:to_s) == [cast_value].flatten.map(&:to_s)
-        log_dynamic_finder_type_cast(value, column)
-        attributes[attribute] = cast_value
-      end
+      construct_attributes_from_arguments_without_type_cast(attribute_names, arguments)
     end
     alias_method_chain :construct_attributes_from_arguments, :type_cast
 
@@ -303,12 +294,6 @@ def log_dynamic_finder_nil_arguments(attribute_names)
       raise DynamicFinderTypeError, error if Canvas.dynamic_finder_nil_arguments_error == :raise
       logger.debug "WARNING: " + error
     end
-
-    def log_dynamic_finder_type_cast(value, column)
-      error = "Cannot cleanly cast #{value.inspect} to #{column.type} (#{self.base_class}\##{column.name})"
-      raise DynamicFinderTypeError, error if Canvas.dynamic_finder_type_cast_error == :raise
-      logger.debug "WARNING: " + error
-    end
   end
 
   def self.merge_includes(first, second)

lib/canvas.rb

@@ -4,13 +4,6 @@ module Canvas
   # this to :raise to raise an exception.
   mattr_accessor :protected_attribute_error
 
-  # defines the behavior around casting arguments passed into dynamic finders.
-  # Arguments are coerced to the appropriate type (if the column exists), so
-  # things like find_by_id('123') become find_by_id(123). The default (:log)
-  # logs a warning if the cast isn't clean (e.g. '123a' -> 123 or '' -> 0).
-  # Set this to :raise to raise an error on unclean casts.
-  mattr_accessor :dynamic_finder_type_cast_error
-
   # defines the behavior when nil or empty array arguments passed into dynamic
   # finders. The default (:log) logs a warning if the finder is not scoped and
   # if *all* arguments are nil/[], e.g.

lib/host_url.rb

@@ -79,6 +79,10 @@ def file_host(account, current_host = nil)
       res ||= @@file_host = default_host
     end
 
+    def file_host_with_shard(account, current_host = nil)
+      [file_host(account, current_host), Shard.default]
+    end
+
     def cdn_host
       # by default only set it for development. useful so that gravatar can
       # proxy our fallback urls

spec/controllers/application_controller_spec.rb

@@ -89,7 +89,10 @@
       @controller.expects(:file_download_url).
         with(@attachment, @common_params.merge(:inline => 1)).
         returns('')
-      @controller.send(:safe_domain_file_url, @attachment)
+      HostUrl.expects(:file_host_with_shard).with(42, '').returns(['myfiles', Shard.default])
+      @controller.instance_variable_set(:@domain_root_account, 42)
+      url = @controller.send(:safe_domain_file_url, @attachment)
+      url.should match /myfiles/
     end
 
     it "should include :download=>1 in inline urls for relative contexts" do