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.

14 files changed, 91 insertions, 27 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 4d8fb3585e4..5fe02ff4705 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,4 +1,5 @@
 v 7.8.0 (unreleased)
+  - Fix broken access control for note attachments (Hannes Rosenögger)
   - Replace highlight.js with rouge-fork rugments (Stefan Tatschner)
   - Make project search case insensitive (Hannes Rosenögger)
   - Include issue/mr participants in list of recipients for reassign/close/reopen emails
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
diff --git a/db/migrate/20150213111727_move_note_folder.rb b/db/migrate/20150213111727_move_note_folder.rb
new file mode 100644
index 00000000000..ca7f87d984f
--- /dev/null
+++ b/db/migrate/20150213111727_move_note_folder.rb
@@ -0,0 +1,19 @@
+class MoveNoteFolder < ActiveRecord::Migration
+  def up
+    system(
+      "if [ -d '#{Rails.root}/public/uploads/note' ];
+        then mv #{Rails.root}/public/uploads/note #{Rails.root}/uploads/note;
+        echo 'note folder has been moved successfully';
+      else
+        echo 'note folder has already been moved or does not exist yet. Nothing to do here.'; fi")
+  end
+
+  def down
+    system(
+      "if [ -d '#{Rails.root}/uploads/note' ];
+        then mv #{Rails.root}/uploads/note #{Rails.root}/public/uploads/note;
+        echo 'note folder has been moved successfully';
+      else
+        echo 'note folder has already been moved or does not exist yet. Nothing to do here.'; fi")
+  end
+end
diff --git a/features/steps/groups.rb b/features/steps/groups.rb
index 610e7fd3a48..0a9b4ccba53 100644
--- a/features/steps/groups.rb
+++ b/features/steps/groups.rb
@@ -110,7 +110,7 @@ class Spinach::Features::Groups < Spinach::FeatureSteps
   end
 
   step 'I should see new group "Owned" avatar' do
-    Group.find_by(name: "Owned").avatar.should be_instance_of AttachmentUploader
+    Group.find_by(name: "Owned").avatar.should be_instance_of AvatarUploader
     Group.find_by(name: "Owned").avatar.url.should == "/uploads/group/avatar/#{ Group.find_by(name:"Owned").id }/gitlab_logo.png"
   end
 
diff --git a/features/steps/profile/profile.rb b/features/steps/profile/profile.rb
index a907b0b7dcf..4efd2176782 100644
--- a/features/steps/profile/profile.rb
+++ b/features/steps/profile/profile.rb
@@ -29,7 +29,7 @@ class Spinach::Features::Profile < Spinach::FeatureSteps
   end
 
   step 'I should see new avatar' do
-    @user.avatar.should be_instance_of AttachmentUploader
+    @user.avatar.should be_instance_of AvatarUploader
     @user.avatar.url.should == "/uploads/user/avatar/#{ @user.id }/gitlab_logo.png"
   end
 
diff --git a/features/steps/project/project.rb b/features/steps/project/project.rb
index 033d45e0253..d39c8e7d2db 100644
--- a/features/steps/project/project.rb
+++ b/features/steps/project/project.rb
@@ -35,7 +35,7 @@ class Spinach::Features::Project < Spinach::FeatureSteps
   end
 
   step 'I should see new project avatar' do
-    @project.avatar.should be_instance_of AttachmentUploader
+    @project.avatar.should be_instance_of AvatarUploader
     url = @project.avatar.url
     url.should == "/uploads/project/avatar/#{ @project.id }/gitlab_logo.png"
   end
diff --git a/lib/backup/manager.rb b/lib/backup/manager.rb
index ab8db4e9837..06cd40a5b1c 100644
--- a/lib/backup/manager.rb
+++ b/lib/backup/manager.rb
@@ -1,6 +1,6 @@
 module Backup
   class Manager
-    BACKUP_CONTENTS = %w{repositories/ db/ uploads/ backup_information.yml}
+    BACKUP_CONTENTS = %w{repositories/ db/ public/ uploads/ backup_information.yml}
 
     def pack
       # saving additional informations
diff --git a/lib/backup/uploads.rb b/lib/backup/uploads.rb
index e50e1ff4f13..75d8e18a862 100644
--- a/lib/backup/uploads.rb
+++ b/lib/backup/uploads.rb
@@ -1,29 +1,45 @@
 module Backup
   class Uploads
-    attr_reader :app_uploads_dir, :backup_uploads_dir, :backup_dir
+    attr_reader :app_public_uploads_dir, :app_private_uploads_dir, :backup_public_uploads_dir,
+     :backup_private_uploads_dir, :backup_dir, :backup_public_dir
 
     def initialize
-      @app_uploads_dir = File.realpath(Rails.root.join('public', 'uploads'))
+      @app_public_uploads_dir = File.realpath(Rails.root.join('public', 'uploads'))
+      @app_private_uploads_dir = File.realpath(Rails.root.join('uploads'))
       @backup_dir = Gitlab.config.backup.path
-      @backup_uploads_dir = File.join(Gitlab.config.backup.path, 'uploads')
+      @backup_public_dir = File.join(backup_dir, 'public')
+      @backup_public_uploads_dir = File.join(backup_dir, 'public', 'uploads')
+      @backup_private_uploads_dir = File.join(backup_dir, 'uploads')
     end
 
-    # Copy uploads from public/uploads to backup/uploads
+    # Copy uploads from public/uploads to backup/public/uploads and from /uploads to backup/uploads
     def dump
-      FileUtils.mkdir_p(backup_uploads_dir)
-      FileUtils.cp_r(app_uploads_dir, backup_dir)
+      FileUtils.mkdir_p(backup_public_uploads_dir)
+      FileUtils.cp_r(app_public_uploads_dir, backup_public_dir)
+      
+      FileUtils.mkdir_p(backup_private_uploads_dir)
+      FileUtils.cp_r(app_private_uploads_dir, backup_dir)
     end
 
     def restore
-      backup_existing_uploads_dir
+      backup_existing_public_uploads_dir
+      backup_existing_private_uploads_dir
 
-      FileUtils.cp_r(backup_uploads_dir, app_uploads_dir)
+      FileUtils.cp_r(backup_public_uploads_dir, app_public_uploads_dir)
+      FileUtils.cp_r(backup_private_uploads_dir, app_private_uploads_dir)
     end
 
-    def backup_existing_uploads_dir
-      timestamped_uploads_path = File.join(app_uploads_dir, '..', "uploads.#{Time.now.to_i}")
-      if File.exists?(app_uploads_dir)
-        FileUtils.mv(app_uploads_dir, timestamped_uploads_path)
+    def backup_existing_public_uploads_dir
+      timestamped_public_uploads_path = File.join(app_public_uploads_dir, '..', "uploads.#{Time.now.to_i}")
+      if File.exists?(app_public_uploads_dir)
+        FileUtils.mv(app_public_uploads_dir, timestamped_public_uploads_path)
+      end
+    end
+
+    def backup_existing_private_uploads_dir
+      timestamped_private_uploads_path = File.join(app_private_uploads_dir, '..', "uploads.#{Time.now.to_i}")
+      if File.exists?(app_private_uploads_dir)
+        FileUtils.mv(app_private_uploads_dir, timestamped_private_uploads_path)
       end
     end
   end
diff --git a/uploads/.gitkeep b/uploads/.gitkeep
new file mode 100644
index 00000000000..e69de29bb2d
--- /dev/null
+++ b/uploads/.gitkeep