Merge pull request #1209 from ZogStriP/fix-s3-image-uploads-are-being-improperly-pathed

Fix s3 image uploads are being improperly pathed

app/models/upload.rb

@@ -2,7 +2,7 @@ require 'digest/sha1'
 require 'image_sizer'
 require 'tempfile'
 require 'pathname'
-require_dependency 's3'
+require_dependency 's3_store'
 require_dependency 'local_store'
 
 class Upload < ActiveRecord::Base
@@ -77,12 +77,12 @@ class Upload < ActiveRecord::Base
   end
 
   def self.store_file(file, sha1, upload_id)
-    return S3.store_file(file, sha1, upload_id) if SiteSetting.enable_s3_uploads?
+    return S3Store.store_file(file, sha1, upload_id) if SiteSetting.enable_s3_uploads?
     return LocalStore.store_file(file, sha1, upload_id)
   end
 
   def self.remove_file(url)
-    return S3.remove_file(url) if SiteSetting.enable_s3_uploads?
+    return S3Store.remove_file(url) if SiteSetting.enable_s3_uploads?
     return LocalStore.remove_file(url)
   end
 
@@ -99,7 +99,7 @@ class Upload < ActiveRecord::Base
   end
 
   def self.is_on_s3?(url)
-    SiteSetting.enable_s3_uploads? && url.start_with?(S3.base_url)
+    SiteSetting.enable_s3_uploads? && (url.start_with?(S3Store.base_url) || url.start_with?(S3Store.base_url_old))
   end
 
   def self.get_from_url(url)

lib/cooked_post_processor.rb

@@ -223,7 +223,7 @@ class CookedPostProcessor
 
   def attachments
     if SiteSetting.enable_s3_uploads?
-      @doc.css("a[href^=\"#{S3.base_url}\"]")
+      @doc.css("a[href^=\"#{S3Store.base_url}\"]")
     else
       # local uploads are identified using a relative uri
       @doc.css("a[href^=\"#{LocalStore.directory}\"]")

lib/s3_store.rb

@@ -1,27 +1,31 @@
-module S3
+module S3Store
 
   def self.store_file(file, sha1, upload_id)
-    S3.check_missing_site_settings
+    S3Store.check_missing_site_settings
 
-    directory = S3.get_or_create_directory(SiteSetting.s3_upload_bucket)
+    directory = S3Store.get_or_create_directory(SiteSetting.s3_upload_bucket)
     extension = File.extname(file.original_filename)
     remote_filename = "#{upload_id}#{sha1}#{extension}"
 
     # if this fails, it will throw an exception
-    file = S3.upload(file, remote_filename, directory)
+    file = S3Store.upload(file, remote_filename, directory)
-    "#{S3.base_url}/#{remote_filename}"
+    "#{S3Store.base_url}/#{remote_filename}"
   end
 
   def self.base_url
-    "//#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com"
+    "//s3.amazonaws.com/#{SiteSetting.s3_upload_bucket}"
+  end
+
+  def self.base_url_old
+    "//#{SiteSetting.s3_upload_bucket.downcase}.s3.amazonaws.com"
   end
 
   def self.remove_file(url)
-    S3.check_missing_site_settings
+    S3Store.check_missing_site_settings
 
-    directory = S3.get_or_create_directory(SiteSetting.s3_upload_bucket)
+    directory = S3Store.get_or_create_directory(SiteSetting.s3_upload_bucket)
 
-    file = S3.destroy(url, directory)
+    file = S3Store.destroy(url, directory)
   end
 
   def self.check_missing_site_settings
@@ -33,7 +37,7 @@ module S3
   def self.get_or_create_directory(name)
     @fog_loaded = require 'fog' unless @fog_loaded
 
-    options = S3.generate_options
+    options = S3Store.generate_options
 
     fog = Fog::Storage.new(options)
 

spec/components/s3_store_spec.rb

@@ -1,8 +1,8 @@
 require 'spec_helper'
 require 'fog'
-require 's3'
+require 's3_store'
 
-describe S3 do
+describe S3Store do
 
   describe "store_file" do
 
@@ -17,14 +17,15 @@ describe S3 do
     let(:image_info) { FastImage.new(file) }
 
     before(:each) do
-      SiteSetting.stubs(:s3_upload_bucket).returns("s3_upload_bucket")
+      SiteSetting.stubs(:s3_upload_bucket).returns("S3_Upload_Bucket")
       SiteSetting.stubs(:s3_access_key_id).returns("s3_access_key_id")
       SiteSetting.stubs(:s3_secret_access_key).returns("s3_secret_access_key")
       Fog.mock!
     end
 
     it 'returns the url of the S3 upload if successful' do
-      S3.store_file(file, "SHA", 1).should == '//s3_upload_bucket.s3.amazonaws.com/1SHA.png'
+      # NOTE: s3 bucket's name are case sensitive so we can't use it as a subdomain...
+      S3Store.store_file(file, "SHA", 1).should == '//s3.amazonaws.com/S3_Upload_Bucket/1SHA.png'
     end
 
     after(:each) do

spec/models/upload_spec.rb

@@ -119,12 +119,12 @@ describe Upload do
     it "store files on s3 when enabled" do
       SiteSetting.expects(:enable_s3_uploads?).returns(true)
       LocalStore.expects(:store_file).never
-      S3.expects(:store_file)
+      S3Store.expects(:store_file)
       Upload.store_file(image, image_sha1, 1)
     end
 
     it "store files locally by default" do
-      S3.expects(:store_file).never
+      S3Store.expects(:store_file).never
       LocalStore.expects(:store_file)
       Upload.store_file(image, image_sha1, 1)
     end
@@ -136,12 +136,12 @@ describe Upload do
     it "remove files on s3 when enabled" do
       SiteSetting.expects(:enable_s3_uploads?).returns(true)
       LocalStore.expects(:remove_file).never
-      S3.expects(:remove_file)
+      S3Store.expects(:remove_file)
       Upload.remove_file(upload.url)
     end
 
     it "remove files locally by default" do
-      S3.expects(:remove_file).never
+      S3Store.expects(:remove_file).never
       LocalStore.expects(:remove_file)
       Upload.remove_file(upload.url)
     end
@@ -163,13 +163,31 @@ describe Upload do
 
     it "identifies S3 uploads" do
       SiteSetting.stubs(:enable_s3_uploads).returns(true)
-      SiteSetting.stubs(:s3_upload_bucket).returns("bucket")
+      SiteSetting.stubs(:s3_upload_bucket).returns("Bucket")
-      Upload.has_been_uploaded?("//bucket.s3.amazonaws.com/1337.png").should == true
+      Upload.has_been_uploaded?("//s3.amazonaws.com/Bucket/1337.png").should == true
     end
 
     it "identifies external urls" do
       Upload.has_been_uploaded?("http://domain.com/uploads/default/42/0123456789ABCDEF.jpg").should == false
-      Upload.has_been_uploaded?("//bucket.s3.amazonaws.com/1337.png").should == false
+      Upload.has_been_uploaded?("//s3.amazonaws.com/Bucket/1337.png").should == false
+    end
+
+  end
+
+  context ".is_on_s3?" do
+
+    before do
+      SiteSetting.stubs(:enable_s3_uploads).returns(true)
+      SiteSetting.stubs(:s3_upload_bucket).returns("BuCkEt")
+   end
+
+    it "case-insensitively matches the old subdomain format" do
+      Upload.is_on_s3?("//bucket.s3.amazonaws.com/1337.png").should == true
+    end
+
+    it "case-sensitively matches the new folder format" do
+      Upload.is_on_s3?("//s3.amazonaws.com/BuCkEt/1337.png").should == true
+      Upload.is_on_s3?("//s3.amazonaws.com/bucket/1337.png").should == false
     end
 
   end