Fix broken access control and refactor avatar upload

This commit moves the note folder from
/public/uploads/note
to
/uploads/note
and changes the uploader accordingly.
Now it's no longer possible to avoid the access control by modifing the url.
The Avatar upload has been refactored to use an own uploader as well
to cleanly seperate the two upload types.

6 files changed, 39 insertions, 11 deletions

diff --git a/app/controllers/files_controller.rb b/app/controllers/files_controller.rb
index 9671245d3f4..561af8084c3 100644
--- a/app/controllers/files_controller.rb
+++ b/app/controllers/files_controller.rb
@@ -6,7 +6,9 @@ class FilesController < ApplicationController
     if uploader.file_storage?
       if can?(current_user, :read_project, note.project)
         disposition = uploader.image? ? 'inline' : 'attachment'
-        send_file uploader.file.path, disposition: disposition
+        # Replace old notes location in /public with the new one in / and send the file
+        path = uploader.file.path.gsub("#{Rails.root}/public",Rails.root.to_s)
+        send_file path, disposition: disposition
       else
         not_found!
       end
diff --git a/app/models/group.rb b/app/models/group.rb
index d6ec0be6081..da9621a2a1a 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -23,7 +23,7 @@ class Group < Namespace
   validate :avatar_type, if: ->(user) { user.avatar_changed? }
   validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
 
-  mount_uploader :avatar, AttachmentUploader
+  mount_uploader :avatar, AvatarUploader
 
   after_create :post_create_hook
   after_destroy :post_destroy_hook
diff --git a/app/models/project.rb b/app/models/project.rb
index 56e1aa29040..e2c7f76eb09 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -138,7 +138,7 @@ class Project < ActiveRecord::Base
     if: ->(project) { project.avatar && project.avatar_changed? }
   validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
 
-  mount_uploader :avatar, AttachmentUploader
+  mount_uploader :avatar, AvatarUploader
 
   # Scopes
   scope :sorted_by_activity, -> { reorder(last_activity_at: :desc) }
diff --git a/app/models/user.rb b/app/models/user.rb
index d4018f0c897..2ffcd1478d8 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -177,7 +177,7 @@ class User < ActiveRecord::Base
     end
   end
 
-  mount_uploader :avatar, AttachmentUploader
+  mount_uploader :avatar, AvatarUploader
 
   # Scopes
   scope :admins, -> { where(admin:  true) }
diff --git a/app/uploaders/attachment_uploader.rb b/app/uploaders/attachment_uploader.rb
index b122b6c8658..22742d287a4 100644
--- a/app/uploaders/attachment_uploader.rb
+++ b/app/uploaders/attachment_uploader.rb
@@ -3,10 +3,8 @@
 class AttachmentUploader < CarrierWave::Uploader::Base
   storage :file
 
-  after :store, :reset_events_cache
-
   def store_dir
-    "uploads/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}"
+    "#{Rails.root}/uploads/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}"
   end
 
   def image?
@@ -29,8 +27,4 @@ class AttachmentUploader < CarrierWave::Uploader::Base
   def file_storage?
     self.class.storage == CarrierWave::Storage::File
   end
-
-  def reset_events_cache(file)
-    model.reset_events_cache if model.is_a?(User)
-  end
 end
diff --git a/app/uploaders/avatar_uploader.rb b/app/uploaders/avatar_uploader.rb
new file mode 100644
index 00000000000..7cad044555b
--- /dev/null
+++ b/app/uploaders/avatar_uploader.rb
@@ -0,0 +1,32 @@
+# encoding: utf-8
+
+class AvatarUploader < CarrierWave::Uploader::Base
+  storage :file
+
+  after :store, :reset_events_cache
+
+  def store_dir
+    "uploads/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}"
+  end
+
+  def image?
+    img_ext = %w(png jpg jpeg gif bmp tiff)
+    if file.respond_to?(:extension)
+      img_ext.include?(file.extension.downcase)
+    else
+      # Not all CarrierWave storages respond to :extension
+      ext = file.path.split('.').last.downcase
+      img_ext.include?(ext)
+    end
+  rescue
+    false
+  end
+
+  def file_storage?
+    self.class.storage == CarrierWave::Storage::File
+  end
+
+  def reset_events_cache(file)
+    model.reset_events_cache if model.is_a?(User)
+  end
+end