diff options
author | Hannes Rosenögger <Hannes.Rosenoegger@bva.bund.de> | 2015-02-09 16:35:48 +0300 |
---|---|---|
committer | Douwe Maan <douwe@gitlab.com> | 2015-02-16 22:10:15 +0300 |
commit | 7d5f86f6cbd187e75a6ba164ad6bfd036977dd07 (patch) | |
tree | 43f9cf4d556b95f73481df0e6f258600b59f5a51 | |
parent | 87b413592499ddcf1149d9e2b580f76a13bf625c (diff) |
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.
-rw-r--r-- | CHANGELOG | 1 | ||||
-rw-r--r-- | app/controllers/files_controller.rb | 4 | ||||
-rw-r--r-- | app/models/group.rb | 2 | ||||
-rw-r--r-- | app/models/project.rb | 2 | ||||
-rw-r--r-- | app/models/user.rb | 2 | ||||
-rw-r--r-- | app/uploaders/attachment_uploader.rb | 8 | ||||
-rw-r--r-- | app/uploaders/avatar_uploader.rb | 32 | ||||
-rw-r--r-- | db/migrate/20150213111727_move_note_folder.rb | 19 | ||||
-rw-r--r-- | features/steps/groups.rb | 2 | ||||
-rw-r--r-- | features/steps/profile/profile.rb | 2 | ||||
-rw-r--r-- | features/steps/project/project.rb | 2 | ||||
-rw-r--r-- | lib/backup/manager.rb | 2 | ||||
-rw-r--r-- | lib/backup/uploads.rb | 40 | ||||
-rw-r--r-- | uploads/.gitkeep | 0 |
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 |