diff options
55 files changed, 1252 insertions, 66 deletions
diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index 929965de5c1..b0143b12cfe 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -1478,7 +1478,7 @@ const normalizeNewlines = function(str) { const cachedNoteBodyText = $noteBodyText.html(); // Show updated comment content temporarily - $noteBodyText.html(formContent); + $noteBodyText.html(_.escape(formContent)); $editingNote.removeClass('is-editing fade-in-full').addClass('being-posted fade-in-half'); $editingNote.find('.note-headline-meta a').html('<i class="fa fa-spinner fa-spin" aria-label="Comment is being updated" aria-hidden="true"></i>'); @@ -1491,7 +1491,7 @@ const normalizeNewlines = function(str) { }) .fail(() => { // Submission failed, revert back to original note - $noteBodyText.html(cachedNoteBodyText); + $noteBodyText.html(_.escape(cachedNoteBodyText)); $editingNote.removeClass('being-posted fade-in'); $editingNote.find('.fa.fa-spinner').remove(); diff --git a/app/controllers/autocomplete_controller.rb b/app/controllers/autocomplete_controller.rb index 907717dcb96..fe331a883c1 100644 --- a/app/controllers/autocomplete_controller.rb +++ b/app/controllers/autocomplete_controller.rb @@ -21,7 +21,7 @@ class AutocompleteController < ApplicationController @users = [current_user, *@users].uniq end - if params[:author_id].present? + if params[:author_id].present? && current_user author = User.find_by_id(params[:author_id]) @users = [author, *@users].uniq if author end diff --git a/app/policies/project_snippet_policy.rb b/app/policies/project_snippet_policy.rb index cf8ff92617f..bc5c4f32f79 100644 --- a/app/policies/project_snippet_policy.rb +++ b/app/policies/project_snippet_policy.rb @@ -1,5 +1,10 @@ class ProjectSnippetPolicy < BasePolicy def rules + # We have to check both project feature visibility and a snippet visibility and take the stricter one + # This will be simplified - check https://gitlab.com/gitlab-org/gitlab-ce/issues/27573 + return unless @subject.project.feature_available?(:snippets, @user) + return unless Ability.allowed?(@user, :read_project, @subject.project) + can! :read_project_snippet if @subject.public? return unless @user diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index 7e94218c23d..652277e3b78 100644 --- a/app/uploaders/file_uploader.rb +++ b/app/uploaders/file_uploader.rb @@ -13,6 +13,13 @@ class FileUploader < GitlabUploader ) end + # Not using `GitlabUploader.base_dir` because all project namespaces are in + # the `public/uploads` dir. + # + def self.base_dir + root_dir + end + # Returns the part of `store_dir` that can change based on the model's current # path # diff --git a/app/uploaders/gitlab_uploader.rb b/app/uploaders/gitlab_uploader.rb index 02afddb8c6a..489613030e6 100644 --- a/app/uploaders/gitlab_uploader.rb +++ b/app/uploaders/gitlab_uploader.rb @@ -3,16 +3,26 @@ class GitlabUploader < CarrierWave::Uploader::Base File.join(CarrierWave.root, upload_record.path) end - def self.base_dir + def self.root_dir 'uploads' end - delegate :base_dir, to: :class + # When object storage is used, keep the `root_dir` as `base_dir`. + # The files aren't really in folders there, they just have a name. + # The files that contain user input in their name, also contain a hash, so + # the names are still unique + # + # This method is overridden in the `FileUploader` + def self.base_dir + return root_dir unless file_storage? + end - def file_storage? - storage.is_a?(CarrierWave::Storage::File) + def self.file_storage? + self.storage.is_a?(CarrierWave::Storage::File) end + delegate :base_dir, :file_storage?, to: :class + def file_cache_storage? cache_storage.is_a?(CarrierWave::Storage::File) end diff --git a/config/routes/uploads.rb b/config/routes/uploads.rb index ae8747d766d..a49e244af1a 100644 --- a/config/routes/uploads.rb +++ b/config/routes/uploads.rb @@ -1,6 +1,6 @@ scope path: :uploads do # Note attachments and User/Group/Project avatars - get ":model/:mounted_as/:id/:filename", + get "system/:model/:mounted_as/:id/:filename", to: "uploads#show", constraints: { model: /note|user|group|project/, mounted_as: /avatar|attachment/, filename: /[^\/]+/ } @@ -15,7 +15,7 @@ scope path: :uploads do constraints: { filename: /[^\/]+/ } # Appearance - get ":model/:mounted_as/:id/:filename", + get "system/:model/:mounted_as/:id/:filename", to: "uploads#show", constraints: { model: /appearance/, mounted_as: /logo|header_logo/, filename: /.+/ } diff --git a/db/migrate/20170316163800_rename_system_namespaces.rb b/db/migrate/20170316163800_rename_system_namespaces.rb new file mode 100644 index 00000000000..b5408fbf112 --- /dev/null +++ b/db/migrate/20170316163800_rename_system_namespaces.rb @@ -0,0 +1,231 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. +class RenameSystemNamespaces < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + include Gitlab::ShellAdapter + disable_ddl_transaction! + + class User < ActiveRecord::Base + self.table_name = 'users' + end + + class Namespace < ActiveRecord::Base + self.table_name = 'namespaces' + belongs_to :parent, class_name: 'RenameSystemNamespaces::Namespace' + has_one :route, as: :source + has_many :children, class_name: 'RenameSystemNamespaces::Namespace', foreign_key: :parent_id + belongs_to :owner, class_name: 'RenameSystemNamespaces::User' + + # Overridden to have the correct `source_type` for the `route` relation + def self.name + 'Namespace' + end + + def full_path + if route && route.path.present? + @full_path ||= route.path + else + update_route if persisted? + + build_full_path + end + end + + def build_full_path + if parent && path + parent.full_path + '/' + path + else + path + end + end + + def update_route + prepare_route + route.save + end + + def prepare_route + route || build_route(source: self) + route.path = build_full_path + route.name = build_full_name + @full_path = nil + @full_name = nil + end + + def build_full_name + if parent && name + parent.human_name + ' / ' + name + else + name + end + end + + def human_name + owner&.name + end + end + + class Route < ActiveRecord::Base + self.table_name = 'routes' + belongs_to :source, polymorphic: true + end + + class Project < ActiveRecord::Base + self.table_name = 'projects' + + def repository_storage_path + Gitlab.config.repositories.storages[repository_storage]['path'] + end + end + + DOWNTIME = false + + def up + return unless system_namespace + + old_path = system_namespace.path + old_full_path = system_namespace.full_path + # Only remove the last occurrence of the path name to get the parent namespace path + namespace_path = remove_last_occurrence(old_full_path, old_path) + new_path = rename_path(namespace_path, old_path) + new_full_path = join_namespace_path(namespace_path, new_path) + + Namespace.where(id: system_namespace).update_all(path: new_path) # skips callbacks & validations + + replace_statement = replace_sql(Route.arel_table[:path], old_full_path, new_full_path) + route_matches = [old_full_path, "#{old_full_path}/%"] + + update_column_in_batches(:routes, :path, replace_statement) do |table, query| + query.where(Route.arel_table[:path].matches_any(route_matches)) + end + + clear_cache_for_namespace(system_namespace) + + # tasks here are based on `Namespace#move_dir` + move_repositories(system_namespace, old_full_path, new_full_path) + move_namespace_folders(uploads_dir, old_full_path, new_full_path) if file_storage? + move_namespace_folders(pages_dir, old_full_path, new_full_path) + end + + def down + # nothing to do + end + + def remove_last_occurrence(string, pattern) + string.reverse.sub(pattern.reverse, "").reverse + end + + def move_namespace_folders(directory, old_relative_path, new_relative_path) + old_path = File.join(directory, old_relative_path) + return unless File.directory?(old_path) + + new_path = File.join(directory, new_relative_path) + FileUtils.mv(old_path, new_path) + end + + def move_repositories(namespace, old_full_path, new_full_path) + repo_paths_for_namespace(namespace).each do |repository_storage_path| + # Ensure old directory exists before moving it + gitlab_shell.add_namespace(repository_storage_path, old_full_path) + + unless gitlab_shell.mv_namespace(repository_storage_path, old_full_path, new_full_path) + say "Exception moving path #{repository_storage_path} from #{old_full_path} to #{new_full_path}" + end + end + end + + def rename_path(namespace_path, path_was) + counter = 0 + path = "#{path_was}#{counter}" + + while route_exists?(join_namespace_path(namespace_path, path)) + counter += 1 + path = "#{path_was}#{counter}" + end + + path + end + + def route_exists?(full_path) + Route.where(Route.arel_table[:path].matches(full_path)).any? + end + + def join_namespace_path(namespace_path, path) + if namespace_path.present? + File.join(namespace_path, path) + else + path + end + end + + def system_namespace + @system_namespace ||= Namespace.where(parent_id: nil). + where(arel_table[:path].matches(system_namespace_path)). + first + end + + def system_namespace_path + "system" + end + + def clear_cache_for_namespace(namespace) + project_ids = projects_for_namespace(namespace).pluck(:id) + + update_column_in_batches(:projects, :description_html, nil) do |table, query| + query.where(table[:id].in(project_ids)) + end + + update_column_in_batches(:issues, :description_html, nil) do |table, query| + query.where(table[:project_id].in(project_ids)) + end + + update_column_in_batches(:merge_requests, :description_html, nil) do |table, query| + query.where(table[:target_project_id].in(project_ids)) + end + + update_column_in_batches(:notes, :note_html, nil) do |table, query| + query.where(table[:project_id].in(project_ids)) + end + + update_column_in_batches(:milestones, :description_html, nil) do |table, query| + query.where(table[:project_id].in(project_ids)) + end + end + + def projects_for_namespace(namespace) + namespace_ids = child_ids_for_parent(namespace, ids: [namespace.id]) + namespace_or_children = Project.arel_table[:namespace_id].in(namespace_ids) + Project.unscoped.where(namespace_or_children) + end + + # This won't scale to huge trees, but it should do for a handful of namespaces + # called `system`. + def child_ids_for_parent(namespace, ids: []) + namespace.children.each do |child| + ids << child.id + child_ids_for_parent(child, ids: ids) if child.children.any? + end + ids + end + + def repo_paths_for_namespace(namespace) + projects_for_namespace(namespace).distinct. + select(:repository_storage).map(&:repository_storage_path) + end + + def uploads_dir + File.join(Rails.root, "public", "uploads") + end + + def pages_dir + Settings.pages.path + end + + def file_storage? + CarrierWave::Uploader::Base.storage == CarrierWave::Storage::File + end + + def arel_table + Namespace.arel_table + end +end diff --git a/db/migrate/20170316163845_move_uploads_to_system_dir.rb b/db/migrate/20170316163845_move_uploads_to_system_dir.rb new file mode 100644 index 00000000000..564ee10b5ab --- /dev/null +++ b/db/migrate/20170316163845_move_uploads_to_system_dir.rb @@ -0,0 +1,59 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class MoveUploadsToSystemDir < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + disable_ddl_transaction! + + DOWNTIME = false + DIRECTORIES_TO_MOVE = %w(user project note group appearance).freeze + + def up + return unless file_storage? + + FileUtils.mkdir_p(new_upload_dir) + + DIRECTORIES_TO_MOVE.each do |dir| + source = File.join(old_upload_dir, dir) + destination = File.join(new_upload_dir, dir) + next unless File.directory?(source) + next if File.directory?(destination) + + say "Moving #{source} -> #{destination}" + FileUtils.mv(source, destination) + FileUtils.ln_s(destination, source) + end + end + + def down + return unless file_storage? + return unless File.directory?(new_upload_dir) + + DIRECTORIES_TO_MOVE.each do |dir| + source = File.join(new_upload_dir, dir) + destination = File.join(old_upload_dir, dir) + next unless File.directory?(source) + next if File.directory?(destination) && !File.symlink?(destination) + + say "Moving #{source} -> #{destination}" + FileUtils.rm(destination) if File.symlink?(destination) + FileUtils.mv(source, destination) + end + end + + def file_storage? + CarrierWave::Uploader::Base.storage == CarrierWave::Storage::File + end + + def base_directory + Rails.root + end + + def old_upload_dir + File.join(base_directory, "public", "uploads") + end + + def new_upload_dir + File.join(base_directory, "public", "uploads", "system") + end +end diff --git a/db/post_migrate/20170317162059_update_upload_paths_to_system.rb b/db/post_migrate/20170317162059_update_upload_paths_to_system.rb new file mode 100644 index 00000000000..9a77b0bbdfb --- /dev/null +++ b/db/post_migrate/20170317162059_update_upload_paths_to_system.rb @@ -0,0 +1,55 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class UpdateUploadPathsToSystem < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + AFFECTED_MODELS = %w(User Project Note Namespace Appearance) + + def up + update_column_in_batches(:uploads, :path, replace_sql(arel_table[:path], base_directory, new_upload_dir)) do |_table, query| + query.where(uploads_to_switch_to_new_path) + end + end + + def down + update_column_in_batches(:uploads, :path, replace_sql(arel_table[:path], new_upload_dir, base_directory)) do |_table, query| + query.where(uploads_to_switch_to_old_path) + end + end + + # "SELECT \"uploads\".* FROM \"uploads\" WHERE \"uploads\".\"model_type\" IN ('User', 'Project', 'Note', 'Namespace', 'Appearance') AND (\"uploads\".\"path\" ILIKE 'uploads/%' AND NOT (\"uploads\".\"path\" ILIKE 'uploads/system/%'))" + def uploads_to_switch_to_new_path + affected_uploads.and(starting_with_base_directory).and(starting_with_new_upload_directory.not) + end + + # "SELECT \"uploads\".* FROM \"uploads\" WHERE \"uploads\".\"model_type\" IN ('User', 'Project', 'Note', 'Namespace', 'Appearance') AND (\"uploads\".\"path\" ILIKE 'uploads/%' AND \"uploads\".\"path\" ILIKE 'uploads/system/%')" + def uploads_to_switch_to_old_path + affected_uploads.and(starting_with_new_upload_directory) + end + + def starting_with_base_directory + arel_table[:path].matches("#{base_directory}/%") + end + + def starting_with_new_upload_directory + arel_table[:path].matches("#{new_upload_dir}/%") + end + + def affected_uploads + arel_table[:model_type].in(AFFECTED_MODELS) + end + + def base_directory + "uploads" + end + + def new_upload_dir + File.join(base_directory, "system") + end + + def arel_table + Arel::Table.new(:uploads) + end +end diff --git a/db/post_migrate/20170406111121_clean_upload_symlinks.rb b/db/post_migrate/20170406111121_clean_upload_symlinks.rb new file mode 100644 index 00000000000..3ac9a6c10bc --- /dev/null +++ b/db/post_migrate/20170406111121_clean_upload_symlinks.rb @@ -0,0 +1,52 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class CleanUploadSymlinks < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + disable_ddl_transaction! + + DOWNTIME = false + DIRECTORIES_TO_MOVE = %w(user project note group appeareance) + + def up + return unless file_storage? + + DIRECTORIES_TO_MOVE.each do |dir| + symlink_location = File.join(old_upload_dir, dir) + next unless File.symlink?(symlink_location) + say "removing symlink: #{symlink_location}" + FileUtils.rm(symlink_location) + end + end + + def down + return unless file_storage? + + DIRECTORIES_TO_MOVE.each do |dir| + symlink = File.join(old_upload_dir, dir) + destination = File.join(new_upload_dir, dir) + + next if File.directory?(symlink) + next unless File.directory?(destination) + + say "Creating symlink #{symlink} -> #{destination}" + FileUtils.ln_s(destination, symlink) + end + end + + def file_storage? + CarrierWave::Uploader::Base.storage == CarrierWave::Storage::File + end + + def base_directory + Rails.root + end + + def old_upload_dir + File.join(base_directory, "public", "uploads") + end + + def new_upload_dir + File.join(base_directory, "public", "uploads", "system") + end +end diff --git a/db/post_migrate/20170606202615_move_appearance_to_system_dir.rb b/db/post_migrate/20170606202615_move_appearance_to_system_dir.rb new file mode 100644 index 00000000000..561de59ec69 --- /dev/null +++ b/db/post_migrate/20170606202615_move_appearance_to_system_dir.rb @@ -0,0 +1,57 @@ +class MoveAppearanceToSystemDir < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + disable_ddl_transaction! + + DOWNTIME = false + DIRECTORY_TO_MOVE = 'appearance'.freeze + + def up + source = File.join(old_upload_dir, DIRECTORY_TO_MOVE) + destination = File.join(new_upload_dir, DIRECTORY_TO_MOVE) + + move_directory(source, destination) + end + + def down + source = File.join(new_upload_dir, DIRECTORY_TO_MOVE) + destination = File.join(old_upload_dir, DIRECTORY_TO_MOVE) + + move_directory(source, destination) + end + + def move_directory(source, destination) + unless file_storage? + say 'Not using file storage, skipping' + return + end + + unless File.directory?(source) + say "#{source} did not exist, skipping" + return + end + + if File.directory?(destination) + say "#{destination} already existed, skipping" + return + end + + say "Moving #{source} -> #{destination}" + FileUtils.mv(source, destination) + end + + def file_storage? + CarrierWave::Uploader::Base.storage == CarrierWave::Storage::File + end + + def base_directory + Rails.root + end + + def old_upload_dir + File.join(base_directory, "public", "uploads") + end + + def new_upload_dir + File.join(base_directory, "public", "uploads", "system") + end +end diff --git a/db/schema.rb b/db/schema.rb index 83172a92b49..b93630a410d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,8 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20170603200744) do - +ActiveRecord::Schema.define(version: 20170606202615) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" enable_extension "pg_trgm" diff --git a/features/steps/groups.rb b/features/steps/groups.rb index 83d8abbab1f..25bb374b868 100644 --- a/features/steps/groups.rb +++ b/features/steps/groups.rb @@ -81,7 +81,7 @@ class Spinach::Features::Groups < Spinach::FeatureSteps step 'I should see new group "Owned" avatar' do expect(owned_group.avatar).to be_instance_of AvatarUploader - expect(owned_group.avatar.url).to eq "/uploads/group/avatar/#{Group.find_by(name: "Owned").id}/banana_sample.gif" + expect(owned_group.avatar.url).to eq "/uploads/system/group/avatar/#{Group.find_by(name: "Owned").id}/banana_sample.gif" end step 'I should see the "Remove avatar" button' do diff --git a/features/steps/profile/profile.rb b/features/steps/profile/profile.rb index 24cfbaad7fe..254c26bb6af 100644 --- a/features/steps/profile/profile.rb +++ b/features/steps/profile/profile.rb @@ -36,7 +36,7 @@ class Spinach::Features::Profile < Spinach::FeatureSteps step 'I should see new avatar' do expect(@user.avatar).to be_instance_of AvatarUploader - expect(@user.avatar.url).to eq "/uploads/user/avatar/#{@user.id}/banana_sample.gif" + expect(@user.avatar.url).to eq "/uploads/system/user/avatar/#{@user.id}/banana_sample.gif" end step 'I should see the "Remove avatar" button' do diff --git a/features/steps/project/project.rb b/features/steps/project/project.rb index de32c9afcca..7d34331db46 100644 --- a/features/steps/project/project.rb +++ b/features/steps/project/project.rb @@ -38,7 +38,7 @@ class Spinach::Features::Project < Spinach::FeatureSteps step 'I should see new project avatar' do expect(@project.avatar).to be_instance_of AvatarUploader url = @project.avatar.url - expect(url).to eq "/uploads/project/avatar/#{@project.id}/banana_sample.gif" + expect(url).to eq "/uploads/system/project/avatar/#{@project.id}/banana_sample.gif" end step 'I should see the "Remove avatar" button' do diff --git a/lib/api/api.rb b/lib/api/api.rb index 88f91c07194..d767af36e8e 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -45,6 +45,7 @@ module API end before { allow_access_with_scope :api } + before { header['X-Frame-Options'] = 'SAMEORIGIN' } before { Gitlab::I18n.locale = current_user&.preferred_language } after { Gitlab::I18n.use_default_locale } diff --git a/lib/banzai/reference_parser/base_parser.rb b/lib/banzai/reference_parser/base_parser.rb index d99a3bfa625..1e2536231d8 100644 --- a/lib/banzai/reference_parser/base_parser.rb +++ b/lib/banzai/reference_parser/base_parser.rb @@ -62,7 +62,7 @@ module Banzai nodes.select do |node| if node.has_attribute?(project_attr) - can_read_reference?(user, projects[node]) + can_read_reference?(user, projects[node], node) else true end @@ -231,7 +231,7 @@ module Banzai # see reference comments. # Override this method on subclasses # to check if user can read resource - def can_read_reference?(user, ref_project) + def can_read_reference?(user, ref_project, node) raise NotImplementedError end diff --git a/lib/banzai/reference_parser/commit_parser.rb b/lib/banzai/reference_parser/commit_parser.rb index 8c54a041cb8..30dc87248b4 100644 --- a/lib/banzai/reference_parser/commit_parser.rb +++ b/lib/banzai/reference_parser/commit_parser.rb @@ -32,7 +32,7 @@ module Banzai private - def can_read_reference?(user, ref_project) + def can_read_reference?(user, ref_project, node) can?(user, :download_code, ref_project) end end diff --git a/lib/banzai/reference_parser/commit_range_parser.rb b/lib/banzai/reference_parser/commit_range_parser.rb index 0878b6afba3..a50e6f8ef8f 100644 --- a/lib/banzai/reference_parser/commit_range_parser.rb +++ b/lib/banzai/reference_parser/commit_range_parser.rb @@ -36,7 +36,7 @@ module Banzai private - def can_read_reference?(user, ref_project) + def can_read_reference?(user, ref_project, node) can?(user, :download_code, ref_project) end end diff --git a/lib/banzai/reference_parser/external_issue_parser.rb b/lib/banzai/reference_parser/external_issue_parser.rb index 6e7b7669578..6307c1b571a 100644 --- a/lib/banzai/reference_parser/external_issue_parser.rb +++ b/lib/banzai/reference_parser/external_issue_parser.rb @@ -23,7 +23,7 @@ module Banzai private - def can_read_reference?(user, ref_project) + def can_read_reference?(user, ref_project, node) can?(user, :read_issue, ref_project) end end diff --git a/lib/banzai/reference_parser/label_parser.rb b/lib/banzai/reference_parser/label_parser.rb index aa76c64ac5f..30e2a012f09 100644 --- a/lib/banzai/reference_parser/label_parser.rb +++ b/lib/banzai/reference_parser/label_parser.rb @@ -9,7 +9,7 @@ module Banzai private - def can_read_reference?(user, ref_project) + def can_read_reference?(user, ref_project, node) can?(user, :read_label, ref_project) end end diff --git a/lib/banzai/reference_parser/merge_request_parser.rb b/lib/banzai/reference_parser/merge_request_parser.rb index 8b0662749fd..75cbc7fdac4 100644 --- a/lib/banzai/reference_parser/merge_request_parser.rb +++ b/lib/banzai/reference_parser/merge_request_parser.rb @@ -40,6 +40,10 @@ module Banzai self.class.data_attribute ) end + + def can_read_reference?(user, ref_project, node) + can?(user, :read_merge_request, ref_project) + end end end end diff --git a/lib/banzai/reference_parser/milestone_parser.rb b/lib/banzai/reference_parser/milestone_parser.rb index d3968d6b229..68675abe22a 100644 --- a/lib/banzai/reference_parser/milestone_parser.rb +++ b/lib/banzai/reference_parser/milestone_parser.rb @@ -9,7 +9,7 @@ module Banzai private - def can_read_reference?(user, ref_project) + def can_read_reference?(user, ref_project, node) can?(user, :read_milestone, ref_project) end end diff --git a/lib/banzai/reference_parser/snippet_parser.rb b/lib/banzai/reference_parser/snippet_parser.rb index 63b592137bb..3ade168b566 100644 --- a/lib/banzai/reference_parser/snippet_parser.rb +++ b/lib/banzai/reference_parser/snippet_parser.rb @@ -9,8 +9,8 @@ module Banzai private - def can_read_reference?(user, ref_project) - can?(user, :read_project_snippet, ref_project) + def can_read_reference?(user, ref_project, node) + can?(user, :read_project_snippet, referenced_by([node]).first) end end end diff --git a/lib/banzai/reference_parser/user_parser.rb b/lib/banzai/reference_parser/user_parser.rb index 09b66cbd8fb..3efbd2fd631 100644 --- a/lib/banzai/reference_parser/user_parser.rb +++ b/lib/banzai/reference_parser/user_parser.rb @@ -103,7 +103,7 @@ module Banzai flat_map { |p| p.team.members.to_a } end - def can_read_reference?(user, ref_project) + def can_read_reference?(user, ref_project, node) can?(user, :read_project, ref_project) end end diff --git a/lib/gitlab/path_regex.rb b/lib/gitlab/path_regex.rb index 9ff6829cd49..10eb99fb461 100644 --- a/lib/gitlab/path_regex.rb +++ b/lib/gitlab/path_regex.rb @@ -49,6 +49,7 @@ module Gitlab sent_notifications services snippets + system teams u unicorn_test diff --git a/lib/gitlab/uploads_transfer.rb b/lib/gitlab/uploads_transfer.rb index 7d0c47c5361..b5f41240529 100644 --- a/lib/gitlab/uploads_transfer.rb +++ b/lib/gitlab/uploads_transfer.rb @@ -1,7 +1,7 @@ module Gitlab class UploadsTransfer < ProjectTransfer def root_dir - File.join(CarrierWave.root, GitlabUploader.base_dir) + File.join(CarrierWave.root, FileUploader.base_dir) end end end diff --git a/spec/controllers/autocomplete_controller_spec.rb b/spec/controllers/autocomplete_controller_spec.rb index 2c9d1ffc9c2..4c3a5ec49ef 100644 --- a/spec/controllers/autocomplete_controller_spec.rb +++ b/spec/controllers/autocomplete_controller_spec.rb @@ -170,22 +170,32 @@ describe AutocompleteController do end context 'author of issuable included' do - before do - sign_in(user) - end - let(:body) { JSON.parse(response.body) } - it 'includes the author' do - get(:users, author_id: non_member.id) + context 'authenticated' do + before do + sign_in(user) + end + + it 'includes the author' do + get(:users, author_id: non_member.id) + + expect(body.first["username"]).to eq non_member.username + end + + it 'rejects non existent user ids' do + get(:users, author_id: 99999) - expect(body.first["username"]).to eq non_member.username + expect(body.collect { |u| u['id'] }).not_to include(99999) + end end - it 'rejects non existent user ids' do - get(:users, author_id: 99999) + context 'without authenticating' do + it 'returns empty result' do + get(:users, author_id: non_member.id) - expect(body.collect { |u| u['id'] }).not_to include(99999) + expect(body).to be_empty + end end end diff --git a/spec/factories/uploads.rb b/spec/factories/uploads.rb new file mode 100644 index 00000000000..1383420fb44 --- /dev/null +++ b/spec/factories/uploads.rb @@ -0,0 +1,8 @@ +FactoryGirl.define do + factory :upload do + model { build(:project) } + path { "uploads/system/project/avatar/avatar.jpg" } + size 100.kilobytes + uploader "AvatarUploader" + end +end diff --git a/spec/features/admin/admin_appearance_spec.rb b/spec/features/admin/admin_appearance_spec.rb index 96d715ef383..595366ce352 100644 --- a/spec/features/admin/admin_appearance_spec.rb +++ b/spec/features/admin/admin_appearance_spec.rb @@ -63,11 +63,11 @@ feature 'Admin Appearance', feature: true do end def logo_selector - '//img[@src^="/uploads/appearance/logo"]' + '//img[@src^="/uploads/system/appearance/logo"]' end def header_logo_selector - '//img[@src^="/uploads/appearance/header_logo"]' + '//img[@src^="/uploads/system/appearance/header_logo"]' end def logo_fixture diff --git a/spec/features/uploads/user_uploads_avatar_to_group_spec.rb b/spec/features/uploads/user_uploads_avatar_to_group_spec.rb index f88a515f7fc..d9d6f2e2382 100644 --- a/spec/features/uploads/user_uploads_avatar_to_group_spec.rb +++ b/spec/features/uploads/user_uploads_avatar_to_group_spec.rb @@ -18,7 +18,7 @@ feature 'User uploads avatar to group', feature: true do visit group_path(group) - expect(page).to have_selector(%Q(img[src$="/uploads/group/avatar/#{group.id}/dk.png"])) + expect(page).to have_selector(%Q(img[src$="/uploads/system/group/avatar/#{group.id}/dk.png"])) # Cheating here to verify something that isn't user-facing, but is important expect(group.reload.avatar.file).to exist diff --git a/spec/features/uploads/user_uploads_avatar_to_profile_spec.rb b/spec/features/uploads/user_uploads_avatar_to_profile_spec.rb index 0dfd29045e5..eb8dbd76aab 100644 --- a/spec/features/uploads/user_uploads_avatar_to_profile_spec.rb +++ b/spec/features/uploads/user_uploads_avatar_to_profile_spec.rb @@ -16,7 +16,7 @@ feature 'User uploads avatar to profile', feature: true do visit user_path(user) - expect(page).to have_selector(%Q(img[src$="/uploads/user/avatar/#{user.id}/dk.png"])) + expect(page).to have_selector(%Q(img[src$="/uploads/system/user/avatar/#{user.id}/dk.png"])) # Cheating here to verify something that isn't user-facing, but is important expect(user.reload.avatar.file).to exist diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb index 785fb724132..49df91b236f 100644 --- a/spec/helpers/application_helper_spec.rb +++ b/spec/helpers/application_helper_spec.rb @@ -1,3 +1,4 @@ +# coding: utf-8 require 'spec_helper' describe ApplicationHelper do @@ -58,13 +59,13 @@ describe ApplicationHelper do describe 'project_icon' do it 'returns an url for the avatar' do project = create(:empty_project, avatar: File.open(uploaded_image_temp_path)) - avatar_url = "/uploads/project/avatar/#{project.id}/banana_sample.gif" + avatar_url = "/uploads/system/project/avatar/#{project.id}/banana_sample.gif" expect(helper.project_icon(project.full_path).to_s). to eq "<img src=\"#{avatar_url}\" alt=\"Banana sample\" />" allow(ActionController::Base).to receive(:asset_host).and_return(gitlab_host) - avatar_url = "#{gitlab_host}/uploads/project/avatar/#{project.id}/banana_sample.gif" + avatar_url = "#{gitlab_host}/uploads/system/project/avatar/#{project.id}/banana_sample.gif" expect(helper.project_icon(project.full_path).to_s). to eq "<img src=\"#{avatar_url}\" alt=\"Banana sample\" />" @@ -84,12 +85,12 @@ describe ApplicationHelper do it 'returns an url for the avatar' do user = create(:user, avatar: File.open(uploaded_image_temp_path)) - avatar_url = "/uploads/user/avatar/#{user.id}/banana_sample.gif" + avatar_url = "/uploads/system/user/avatar/#{user.id}/banana_sample.gif" expect(helper.avatar_icon(user.email).to_s).to match(avatar_url) allow(ActionController::Base).to receive(:asset_host).and_return(gitlab_host) - avatar_url = "#{gitlab_host}/uploads/user/avatar/#{user.id}/banana_sample.gif" + avatar_url = "#{gitlab_host}/uploads/system/user/avatar/#{user.id}/banana_sample.gif" expect(helper.avatar_icon(user.email).to_s).to match(avatar_url) end @@ -102,7 +103,7 @@ describe ApplicationHelper do user = create(:user, avatar: File.open(uploaded_image_temp_path)) expect(helper.avatar_icon(user.email).to_s). - to match("/gitlab/uploads/user/avatar/#{user.id}/banana_sample.gif") + to match("/gitlab/uploads/system/user/avatar/#{user.id}/banana_sample.gif") end it 'calls gravatar_icon when no User exists with the given email' do @@ -116,7 +117,7 @@ describe ApplicationHelper do user = create(:user, avatar: File.open(uploaded_image_temp_path)) expect(helper.avatar_icon(user).to_s). - to match("/uploads/user/avatar/#{user.id}/banana_sample.gif") + to match("/uploads/system/user/avatar/#{user.id}/banana_sample.gif") end end end diff --git a/spec/helpers/emails_helper_spec.rb b/spec/helpers/emails_helper_spec.rb index cd112dbb2fb..c68e4f56b05 100644 --- a/spec/helpers/emails_helper_spec.rb +++ b/spec/helpers/emails_helper_spec.rb @@ -52,7 +52,7 @@ describe EmailsHelper do ) expect(header_logo).to eq( - %{<img style="height: 50px" src="/uploads/appearance/header_logo/#{appearance.id}/dk.png" alt="Dk" />} + %{<img style="height: 50px" src="/uploads/system/appearance/header_logo/#{appearance.id}/dk.png" alt="Dk" />} ) end end diff --git a/spec/helpers/groups_helper_spec.rb b/spec/helpers/groups_helper_spec.rb index c8b0d86425f..0337afa4452 100644 --- a/spec/helpers/groups_helper_spec.rb +++ b/spec/helpers/groups_helper_spec.rb @@ -9,7 +9,7 @@ describe GroupsHelper do group.avatar = fixture_file_upload(avatar_file_path) group.save! expect(group_icon(group.path).to_s). - to match("/uploads/group/avatar/#{group.id}/banana_sample.gif") + to match("/uploads/system/group/avatar/#{group.id}/banana_sample.gif") end it 'gives default avatar_icon when no avatar is present' do diff --git a/spec/helpers/page_layout_helper_spec.rb b/spec/helpers/page_layout_helper_spec.rb index 2cc0b40b2d0..dff2784f21f 100644 --- a/spec/helpers/page_layout_helper_spec.rb +++ b/spec/helpers/page_layout_helper_spec.rb @@ -60,7 +60,7 @@ describe PageLayoutHelper do %w(project user group).each do |type| context "with @#{type} assigned" do it "uses #{type.titlecase} avatar if available" do - object = double(avatar_url: 'http://example.com/uploads/avatar.png') + object = double(avatar_url: 'http://example.com/uploads/system/avatar.png') assign(type, object) expect(helper.page_image).to eq object.avatar_url diff --git a/spec/javascripts/notes_spec.js b/spec/javascripts/notes_spec.js index 24335614e09..bfd8b8648a6 100644 --- a/spec/javascripts/notes_spec.js +++ b/spec/javascripts/notes_spec.js @@ -461,6 +461,45 @@ import '~/notes'; }); }); + describe('update comment with script tags', () => { + const sampleComment = '<script></script>'; + const updatedComment = '<script></script>'; + const note = { + id: 1234, + html: `<li class="note note-row-1234 timeline-entry" id="note_1234"> + <div class="note-text">${sampleComment}</div> + </li>`, + note: sampleComment, + valid: true + }; + let $form; + let $notesContainer; + + beforeEach(() => { + this.notes = new Notes('', []); + window.gon.current_username = 'root'; + window.gon.current_user_fullname = 'Administrator'; + $form = $('form.js-main-target-form'); + $notesContainer = $('ul.main-notes-list'); + $form.find('textarea.js-note-text').html(sampleComment); + }); + + it('should not render a script tag', () => { + const deferred = $.Deferred(); + spyOn($, 'ajax').and.returnValue(deferred.promise()); + $('.js-comment-button').click(); + + deferred.resolve(note); + const $noteEl = $notesContainer.find(`#note_${note.id}`); + $noteEl.find('.js-note-edit').click(); + $noteEl.find('textarea.js-note-text').html(updatedComment); + $noteEl.find('.js-comment-save-button').click(); + + const $updatedNoteEl = $notesContainer.find(`#note_${note.id}`).find('.js-task-list-container'); + expect($updatedNoteEl.find('.note-text').text().trim()).toEqual(''); + }); + }); + describe('getFormData', () => { let $form; let sampleComment; diff --git a/spec/javascripts/vue_shared/components/commit_spec.js b/spec/javascripts/vue_shared/components/commit_spec.js index 050170a54e9..540245fe71e 100644 --- a/spec/javascripts/vue_shared/components/commit_spec.js +++ b/spec/javascripts/vue_shared/components/commit_spec.js @@ -22,7 +22,7 @@ describe('Commit component', () => { shortSha: 'b7836edd', title: 'Commit message', author: { - avatar_url: 'https://gitlab.com/uploads/user/avatar/300478/avatar.png', + avatar_url: 'https://gitlab.com/uploads/system/user/avatar/300478/avatar.png', web_url: 'https://gitlab.com/jschatz1', path: '/jschatz1', username: 'jschatz1', @@ -45,7 +45,7 @@ describe('Commit component', () => { shortSha: 'b7836edd', title: 'Commit message', author: { - avatar_url: 'https://gitlab.com/uploads/user/avatar/300478/avatar.png', + avatar_url: 'https://gitlab.com/uploads/system/user/avatar/300478/avatar.png', web_url: 'https://gitlab.com/jschatz1', path: '/jschatz1', username: 'jschatz1', diff --git a/spec/lib/banzai/reference_parser/base_parser_spec.rb b/spec/lib/banzai/reference_parser/base_parser_spec.rb index d5746107ee1..f4f42bfc3ed 100644 --- a/spec/lib/banzai/reference_parser/base_parser_spec.rb +++ b/spec/lib/banzai/reference_parser/base_parser_spec.rb @@ -30,7 +30,7 @@ describe Banzai::ReferenceParser::BaseParser, lib: true do it 'checks if user can read the resource' do link['data-project'] = project.id.to_s - expect(subject).to receive(:can_read_reference?).with(user, project) + expect(subject).to receive(:can_read_reference?).with(user, project, link) subject.nodes_visible_to_user(user, [link]) end diff --git a/spec/lib/banzai/reference_parser/snippet_parser_spec.rb b/spec/lib/banzai/reference_parser/snippet_parser_spec.rb index d217a775802..620875ece20 100644 --- a/spec/lib/banzai/reference_parser/snippet_parser_spec.rb +++ b/spec/lib/banzai/reference_parser/snippet_parser_spec.rb @@ -4,20 +4,199 @@ describe Banzai::ReferenceParser::SnippetParser, lib: true do include ReferenceParserHelpers let(:project) { create(:empty_project, :public) } + let(:user) { create(:user) } - let(:snippet) { create(:snippet, project: project) } + let(:external_user) { create(:user, :external) } + let(:project_member) { create(:user) } + subject { described_class.new(project, user) } let(:link) { empty_html_link } + def visible_references(snippet_visibility, user = nil) + snippet = create(:project_snippet, snippet_visibility, project: project) + link['data-project'] = project.id.to_s + link['data-snippet'] = snippet.id.to_s + + subject.nodes_visible_to_user(user, [link]) + end + + before do + project.add_user(project_member, :developer) + end + describe '#nodes_visible_to_user' do - context 'when the link has a data-issue attribute' do - before { link['data-snippet'] = snippet.id.to_s } + context 'when a project is public and the snippets feature is enabled for everyone' do + before do + project.project_feature.update_attribute(:snippets_access_level, ProjectFeature::ENABLED) + end + + it 'creates a reference for guest for a public snippet' do + expect(visible_references(:public)).to eq([link]) + end + + it 'creates a reference for a regular user for a public snippet' do + expect(visible_references(:public, user)).to eq([link]) + end + + it 'creates a reference for a regular user for an internal snippet' do + expect(visible_references(:internal, user)).to eq([link]) + end + + it 'does not create a reference for an external user for an internal snippet' do + expect(visible_references(:internal, external_user)).to be_empty + end + + it 'creates a reference for a project member for a private snippet' do + expect(visible_references(:private, project_member)).to eq([link]) + end + + it 'does not create a reference for a regular user for a private snippet' do + expect(visible_references(:private, user)).to be_empty + end + end + + context 'when a project is public and the snippets feature is enabled for project team members' do + before do + project.project_feature.update_attribute(:snippets_access_level, ProjectFeature::PRIVATE) + end + + it 'creates a reference for a project member for a public snippet' do + expect(visible_references(:public, project_member)).to eq([link]) + end + + it 'does not create a reference for guest for a public snippet' do + expect(visible_references(:public, nil)).to be_empty + end + + it 'does not create a reference for a regular user for a public snippet' do + expect(visible_references(:public, user)).to be_empty + end + + it 'creates a reference for a project member for an internal snippet' do + expect(visible_references(:internal, project_member)).to eq([link]) + end + + it 'does not create a reference for a regular user for an internal snippet' do + expect(visible_references(:internal, user)).to be_empty + end + + it 'creates a reference for a project member for a private snippet' do + expect(visible_references(:private, project_member)).to eq([link]) + end + + it 'does not create a reference for a regular user for a private snippet' do + expect(visible_references(:private, user)).to be_empty + end + end + + context 'when a project is internal and the snippets feature is enabled for everyone' do + before do + project.update_attribute(:visibility, Gitlab::VisibilityLevel::INTERNAL) + project.project_feature.update_attribute(:snippets_access_level, ProjectFeature::ENABLED) + end + + it 'does not create a reference for guest for a public snippet' do + expect(visible_references(:public)).to be_empty + end + + it 'does not create a reference for an external user for a public snippet' do + expect(visible_references(:public, external_user)).to be_empty + end - it_behaves_like "referenced feature visibility", "snippets" + it 'creates a reference for a regular user for a public snippet' do + expect(visible_references(:public, user)).to eq([link]) + end + + it 'creates a reference for a regular user for an internal snippet' do + expect(visible_references(:internal, user)).to eq([link]) + end + + it 'does not create a reference for an external user for an internal snippet' do + expect(visible_references(:internal, external_user)).to be_empty + end + + it 'creates a reference for a project member for a private snippet' do + expect(visible_references(:private, project_member)).to eq([link]) + end + + it 'does not create a reference for a regular user for a private snippet' do + expect(visible_references(:private, user)).to be_empty + end + end + + context 'when a project is internal and the snippets feature is enabled for project team members' do + before do + project.update_attribute(:visibility, Gitlab::VisibilityLevel::INTERNAL) + project.project_feature.update_attribute(:snippets_access_level, ProjectFeature::PRIVATE) + end + + it 'creates a reference for a project member for a public snippet' do + expect(visible_references(:public, project_member)).to eq([link]) + end + + it 'does not create a reference for guest for a public snippet' do + expect(visible_references(:public, nil)).to be_empty + end + + it 'does not create reference for a regular user for a public snippet' do + expect(visible_references(:public, user)).to be_empty + end + + it 'creates a reference for a project member for an internal snippet' do + expect(visible_references(:internal, project_member)).to eq([link]) + end + + it 'does not create a reference for a regular user for an internal snippet' do + expect(visible_references(:internal, user)).to be_empty + end + + it 'creates a reference for a project member for a private snippet' do + expect(visible_references(:private, project_member)).to eq([link]) + end + + it 'does not create reference for a regular user for a private snippet' do + expect(visible_references(:private, user)).to be_empty + end + end + + context 'when a project is private and the snippets feature is enabled for project team members' do + before do + project.update_attribute(:visibility, Gitlab::VisibilityLevel::PRIVATE) + project.project_feature.update_attribute(:snippets_access_level, ProjectFeature::PRIVATE) + end + + it 'creates a reference for a project member for a public snippet' do + expect(visible_references(:public, project_member)).to eq([link]) + end + + it 'does not create a reference for guest for a public snippet' do + expect(visible_references(:public, nil)).to be_empty + end + + it 'does not create a reference for a regular user for a public snippet' do + expect(visible_references(:public, user)).to be_empty + end + + it 'creates a reference for a project member for an internal snippet' do + expect(visible_references(:internal, project_member)).to eq([link]) + end + + it 'does not create a reference for a regular user for an internal snippet' do + expect(visible_references(:internal, user)).to be_empty + end + + it 'creates a reference for a project member for a private snippet' do + expect(visible_references(:private, project_member)).to eq([link]) + end + + it 'does not create a reference for a regular user for a private snippet' do + expect(visible_references(:private, user)).to be_empty + end end end describe '#referenced_by' do + let(:snippet) { create(:snippet, project: project) } describe 'when the link has a data-snippet attribute' do context 'using an existing snippet ID' do it 'returns an Array of snippets' do @@ -31,7 +210,7 @@ describe Banzai::ReferenceParser::SnippetParser, lib: true do it 'returns an empty Array' do link['data-snippet'] = '' - expect(subject.referenced_by([link])).to eq([]) + expect(subject.referenced_by([link])).to be_empty end end end diff --git a/spec/lib/gitlab/uploads_transfer_spec.rb b/spec/lib/gitlab/uploads_transfer_spec.rb new file mode 100644 index 00000000000..109559bb01c --- /dev/null +++ b/spec/lib/gitlab/uploads_transfer_spec.rb @@ -0,0 +1,11 @@ +require 'spec_helper' + +describe Gitlab::UploadsTransfer do + it 'leaves avatar uploads where they are' do + project_with_avatar = create(:empty_project, :with_avatar) + + described_class.new.rename_namespace('project', 'project-renamed') + + expect(File.exist?(project_with_avatar.avatar.path)).to be_truthy + end +end diff --git a/spec/migrations/clean_upload_symlinks_spec.rb b/spec/migrations/clean_upload_symlinks_spec.rb new file mode 100644 index 00000000000..cecb3ddac53 --- /dev/null +++ b/spec/migrations/clean_upload_symlinks_spec.rb @@ -0,0 +1,46 @@ +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20170406111121_clean_upload_symlinks.rb') + +describe CleanUploadSymlinks do + let(:migration) { described_class.new } + let(:test_dir) { File.join(Rails.root, "tmp", "tests", "move_uploads_test") } + let(:uploads_dir) { File.join(test_dir, "public", "uploads") } + let(:new_uploads_dir) { File.join(uploads_dir, "system") } + let(:original_path) { File.join(new_uploads_dir, 'user') } + let(:symlink_path) { File.join(uploads_dir, 'user') } + + before do + FileUtils.remove_dir(test_dir) if File.directory?(test_dir) + FileUtils.mkdir_p(uploads_dir) + allow(migration).to receive(:base_directory).and_return(test_dir) + allow(migration).to receive(:say) + end + + describe "#up" do + before do + FileUtils.mkdir_p(original_path) + FileUtils.ln_s(original_path, symlink_path) + end + + it 'removes the symlink' do + migration.up + + expect(File.symlink?(symlink_path)).to be(false) + end + end + + describe '#down' do + before do + FileUtils.mkdir_p(File.join(original_path)) + FileUtils.touch(File.join(original_path, 'dummy.file')) + end + + it 'creates a symlink' do + expected_path = File.join(symlink_path, "dummy.file") + migration.down + + expect(File.exist?(expected_path)).to be(true) + expect(File.symlink?(symlink_path)).to be(true) + end + end +end diff --git a/spec/migrations/move_uploads_to_system_dir_spec.rb b/spec/migrations/move_uploads_to_system_dir_spec.rb new file mode 100644 index 00000000000..37d66452447 --- /dev/null +++ b/spec/migrations/move_uploads_to_system_dir_spec.rb @@ -0,0 +1,68 @@ +require "spec_helper" +require Rails.root.join("db", "migrate", "20170316163845_move_uploads_to_system_dir.rb") + +describe MoveUploadsToSystemDir do + let(:migration) { described_class.new } + let(:test_dir) { File.join(Rails.root, "tmp", "move_uploads_test") } + let(:uploads_dir) { File.join(test_dir, "public", "uploads") } + let(:new_uploads_dir) { File.join(uploads_dir, "system") } + + before do + FileUtils.remove_dir(test_dir) if File.directory?(test_dir) + FileUtils.mkdir_p(uploads_dir) + allow(migration).to receive(:base_directory).and_return(test_dir) + allow(migration).to receive(:say) + end + + describe "#up" do + before do + FileUtils.mkdir_p(File.join(uploads_dir, 'user')) + FileUtils.touch(File.join(uploads_dir, 'user', 'dummy.file')) + end + + it 'moves the directory to the new path' do + expected_path = File.join(new_uploads_dir, 'user', 'dummy.file') + + migration.up + + expect(File.exist?(expected_path)).to be(true) + end + + it 'creates a symlink in the old location' do + symlink_path = File.join(uploads_dir, 'user') + expected_path = File.join(symlink_path, 'dummy.file') + + migration.up + + expect(File.exist?(expected_path)).to be(true) + expect(File.symlink?(symlink_path)).to be(true) + end + end + + describe "#down" do + before do + FileUtils.mkdir_p(File.join(new_uploads_dir, 'user')) + FileUtils.touch(File.join(new_uploads_dir, 'user', 'dummy.file')) + end + + it 'moves the directory to the old path' do + expected_path = File.join(uploads_dir, 'user', 'dummy.file') + + migration.down + + expect(File.exist?(expected_path)).to be(true) + end + + it 'removes the symlink if it existed' do + FileUtils.ln_s(File.join(new_uploads_dir, 'user'), File.join(uploads_dir, 'user')) + + directory = File.join(uploads_dir, 'user') + expected_path = File.join(directory, 'dummy.file') + + migration.down + + expect(File.exist?(expected_path)).to be(true) + expect(File.symlink?(directory)).to be(false) + end + end +end diff --git a/spec/migrations/rename_system_namespaces_spec.rb b/spec/migrations/rename_system_namespaces_spec.rb new file mode 100644 index 00000000000..ad1b83d8e2e --- /dev/null +++ b/spec/migrations/rename_system_namespaces_spec.rb @@ -0,0 +1,252 @@ +require "spec_helper" +require Rails.root.join("db", "migrate", "20170316163800_rename_system_namespaces.rb") + +describe RenameSystemNamespaces, truncate: true do + let(:migration) { described_class.new } + let(:test_dir) { File.join(Rails.root, "tmp", "tests", "rename_namespaces_test") } + let(:uploads_dir) { File.join(test_dir, "public", "uploads") } + let(:system_namespace) do + namespace = build(:namespace, path: "system") + namespace.save(validate: false) + namespace + end + + def save_invalid_routable(routable) + routable.__send__(:prepare_route) + routable.save(validate: false) + end + + before do + FileUtils.remove_dir(test_dir) if File.directory?(test_dir) + FileUtils.mkdir_p(uploads_dir) + FileUtils.remove_dir(TestEnv.repos_path) if File.directory?(TestEnv.repos_path) + allow(migration).to receive(:say) + allow(migration).to receive(:uploads_dir).and_return(uploads_dir) + end + + describe "#system_namespace" do + it "only root namespaces called with path `system`" do + system_namespace + system_namespace_with_parent = build(:namespace, path: 'system', parent: create(:namespace)) + system_namespace_with_parent.save(validate: false) + + expect(migration.system_namespace.id).to eq(system_namespace.id) + end + end + + describe "#up" do + before do + system_namespace + end + + it "doesn't break if there are no namespaces called system" do + Namespace.delete_all + + migration.up + end + + it "renames namespaces called system" do + migration.up + + expect(system_namespace.reload.path).to eq("system0") + end + + it "renames the route to the namespace" do + migration.up + + expect(system_namespace.reload.full_path).to eq("system0") + end + + it "renames the route for projects of the namespace" do + project = build(:project, path: "project-path", namespace: system_namespace) + save_invalid_routable(project) + + migration.up + + expect(project.route.reload.path).to eq("system0/project-path") + end + + it "doesn't touch routes of namespaces that look like system" do + namespace = create(:group, path: 'systemlookalike') + project = create(:project, namespace: namespace, path: 'the-project') + + migration.up + + expect(project.route.reload.path).to eq('systemlookalike/the-project') + expect(namespace.route.reload.path).to eq('systemlookalike') + end + + it "moves the the repository for a project in the namespace" do + project = build(:project, namespace: system_namespace, path: "system-project") + save_invalid_routable(project) + TestEnv.copy_repo(project) + expected_repo = File.join(TestEnv.repos_path, "system0", "system-project.git") + + migration.up + + expect(File.directory?(expected_repo)).to be(true) + end + + it "moves the uploads for the namespace" do + allow(migration).to receive(:move_namespace_folders).with(Settings.pages.path, "system", "system0") + expect(migration).to receive(:move_namespace_folders).with(uploads_dir, "system", "system0") + + migration.up + end + + it "moves the pages for the namespace" do + allow(migration).to receive(:move_namespace_folders).with(uploads_dir, "system", "system0") + expect(migration).to receive(:move_namespace_folders).with(Settings.pages.path, "system", "system0") + + migration.up + end + + describe "clears the markdown cache for projects in the system namespace" do + let!(:project) do + project = build(:project, namespace: system_namespace) + save_invalid_routable(project) + project + end + + it 'removes description_html from projects' do + migration.up + + expect(project.reload.description_html).to be_nil + end + + it 'removes issue descriptions' do + issue = create(:issue, project: project, description_html: 'Issue description') + + migration.up + + expect(issue.reload.description_html).to be_nil + end + + it 'removes merge request descriptions' do + merge_request = create(:merge_request, + source_project: project, + target_project: project, + description_html: 'MergeRequest description') + + migration.up + + expect(merge_request.reload.description_html).to be_nil + end + + it 'removes note html' do + note = create(:note, + project: project, + noteable: create(:issue, project: project), + note_html: 'note description') + + migration.up + + expect(note.reload.note_html).to be_nil + end + + it 'removes milestone description' do + milestone = create(:milestone, + project: project, + description_html: 'milestone description') + + migration.up + + expect(milestone.reload.description_html).to be_nil + end + end + + context "system namespace -> subgroup -> system0 project" do + it "updates the route of the project correctly" do + subgroup = build(:group, path: "subgroup", parent: system_namespace) + save_invalid_routable(subgroup) + project = build(:project, path: "system0", namespace: subgroup) + save_invalid_routable(project) + + migration.up + + expect(project.route.reload.path).to eq("system0/subgroup/system0") + end + end + end + + describe "#move_repositories" do + let(:namespace) { create(:group, name: "hello-group") } + it "moves a project for a namespace" do + create(:project, namespace: namespace, path: "hello-project") + expected_path = File.join(TestEnv.repos_path, "bye-group", "hello-project.git") + + migration.move_repositories(namespace, "hello-group", "bye-group") + + expect(File.directory?(expected_path)).to be(true) + end + + it "moves a namespace in a subdirectory correctly" do + child_namespace = create(:group, name: "sub-group", parent: namespace) + create(:project, namespace: child_namespace, path: "hello-project") + + expected_path = File.join(TestEnv.repos_path, "hello-group", "renamed-sub-group", "hello-project.git") + + migration.move_repositories(child_namespace, "hello-group/sub-group", "hello-group/renamed-sub-group") + + expect(File.directory?(expected_path)).to be(true) + end + + it "moves a parent namespace with subdirectories" do + child_namespace = create(:group, name: "sub-group", parent: namespace) + create(:project, namespace: child_namespace, path: "hello-project") + expected_path = File.join(TestEnv.repos_path, "renamed-group", "sub-group", "hello-project.git") + + migration.move_repositories(child_namespace, "hello-group", "renamed-group") + + expect(File.directory?(expected_path)).to be(true) + end + end + + describe "#move_namespace_folders" do + it "moves a namespace with files" do + source = File.join(uploads_dir, "parent-group", "sub-group") + FileUtils.mkdir_p(source) + destination = File.join(uploads_dir, "parent-group", "moved-group") + FileUtils.touch(File.join(source, "test.txt")) + expected_file = File.join(destination, "test.txt") + + migration.move_namespace_folders(uploads_dir, File.join("parent-group", "sub-group"), File.join("parent-group", "moved-group")) + + expect(File.exist?(expected_file)).to be(true) + end + + it "moves a parent namespace uploads" do + source = File.join(uploads_dir, "parent-group", "sub-group") + FileUtils.mkdir_p(source) + destination = File.join(uploads_dir, "moved-parent", "sub-group") + FileUtils.touch(File.join(source, "test.txt")) + expected_file = File.join(destination, "test.txt") + + migration.move_namespace_folders(uploads_dir, "parent-group", "moved-parent") + + expect(File.exist?(expected_file)).to be(true) + end + end + + describe "#child_ids_for_parent" do + it "collects child ids for all levels" do + parent = create(:namespace) + first_child = create(:namespace, parent: parent) + second_child = create(:namespace, parent: parent) + third_child = create(:namespace, parent: second_child) + all_ids = [parent.id, first_child.id, second_child.id, third_child.id] + + collected_ids = migration.child_ids_for_parent(parent, ids: [parent.id]) + + expect(collected_ids).to contain_exactly(*all_ids) + end + end + + describe "#remove_last_ocurrence" do + it "removes only the last occurance of a string" do + input = "this/is/system/namespace/with/system" + + expect(migration.remove_last_occurrence(input, "system")).to eq("this/is/system/namespace/with/") + end + end +end diff --git a/spec/migrations/update_upload_paths_to_system_spec.rb b/spec/migrations/update_upload_paths_to_system_spec.rb new file mode 100644 index 00000000000..7df44515424 --- /dev/null +++ b/spec/migrations/update_upload_paths_to_system_spec.rb @@ -0,0 +1,53 @@ +require "spec_helper" +require Rails.root.join("db", "post_migrate", "20170317162059_update_upload_paths_to_system.rb") + +describe UpdateUploadPathsToSystem do + let(:migration) { described_class.new } + + before do + allow(migration).to receive(:say) + end + + describe "#uploads_to_switch_to_new_path" do + it "contains only uploads with the old path for the correct models" do + _upload_for_other_type = create(:upload, model: create(:ci_pipeline), path: "uploads/ci_pipeline/avatar.jpg") + _upload_with_system_path = create(:upload, model: create(:empty_project), path: "uploads/system/project/avatar.jpg") + _upload_with_other_path = create(:upload, model: create(:empty_project), path: "thelongsecretforafileupload/avatar.jpg") + old_upload = create(:upload, model: create(:empty_project), path: "uploads/project/avatar.jpg") + group_upload = create(:upload, model: create(:group), path: "uploads/group/avatar.jpg") + + expect(Upload.where(migration.uploads_to_switch_to_new_path)).to contain_exactly(old_upload, group_upload) + end + end + + describe "#uploads_to_switch_to_old_path" do + it "contains only uploads with the new path for the correct models" do + _upload_for_other_type = create(:upload, model: create(:ci_pipeline), path: "uploads/ci_pipeline/avatar.jpg") + upload_with_system_path = create(:upload, model: create(:empty_project), path: "uploads/system/project/avatar.jpg") + _upload_with_other_path = create(:upload, model: create(:empty_project), path: "thelongsecretforafileupload/avatar.jpg") + _old_upload = create(:upload, model: create(:empty_project), path: "uploads/project/avatar.jpg") + + expect(Upload.where(migration.uploads_to_switch_to_old_path)).to contain_exactly(upload_with_system_path) + end + end + + describe "#up", truncate: true do + it "updates old upload records to the new path" do + old_upload = create(:upload, model: create(:empty_project), path: "uploads/project/avatar.jpg") + + migration.up + + expect(old_upload.reload.path).to eq("uploads/system/project/avatar.jpg") + end + end + + describe "#down", truncate: true do + it "updates the new system patsh to the old paths" do + new_upload = create(:upload, model: create(:empty_project), path: "uploads/system/project/avatar.jpg") + + migration.down + + expect(new_upload.reload.path).to eq("uploads/project/avatar.jpg") + end + end +end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 316bf153660..3d437ca0fcc 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -179,7 +179,7 @@ describe Group, models: true do let!(:group) { create(:group, :access_requestable, :with_avatar) } let(:user) { create(:user) } let(:gitlab_host) { "http://#{Gitlab.config.gitlab.host}" } - let(:avatar_path) { "/uploads/group/avatar/#{group.id}/dk.png" } + let(:avatar_path) { "/uploads/system/group/avatar/#{group.id}/dk.png" } context 'when avatar file is uploaded' do before { group.add_master(user) } diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index 0e74f1ab1bd..145c7ad5770 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -43,6 +43,12 @@ describe Namespace, models: true do end end + context "is case insensitive" do + let(:group) { build(:group, path: "System") } + + it { expect(group).not_to be_valid } + end + context 'top-level group' do let(:group) { build(:group, path: 'tree') } @@ -178,8 +184,8 @@ describe Namespace, models: true do let(:parent) { create(:group, name: 'parent', path: 'parent') } let(:child) { create(:group, name: 'child', path: 'child', parent: parent) } let!(:project) { create(:project_empty_repo, path: 'the-project', namespace: child) } - let(:uploads_dir) { File.join(CarrierWave.root, 'uploads') } - let(:pages_dir) { TestEnv.pages_path } + let(:uploads_dir) { File.join(CarrierWave.root, FileUploader.base_dir) } + let(:pages_dir) { File.join(TestEnv.pages_path) } before do FileUtils.mkdir_p(File.join(uploads_dir, 'parent', 'child', 'the-project')) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 3ed52d42f86..454eeb58ecd 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -812,7 +812,7 @@ describe Project, models: true do context 'when avatar file is uploaded' do let(:project) { create(:empty_project, :with_avatar) } - let(:avatar_path) { "/uploads/project/avatar/#{project.id}/dk.png" } + let(:avatar_path) { "/uploads/system/project/avatar/#{project.id}/dk.png" } let(:gitlab_host) { "http://#{Gitlab.config.gitlab.host}" } it 'shows correct url' do diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index a83726b48a0..d5bd9946ab6 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -987,7 +987,7 @@ describe User, models: true do context 'when avatar file is uploaded' do let(:gitlab_host) { "http://#{Gitlab.config.gitlab.host}" } - let(:avatar_path) { "/uploads/user/avatar/#{user.id}/dk.png" } + let(:avatar_path) { "/uploads/system/user/avatar/#{user.id}/dk.png" } it 'shows correct avatar url' do expect(user.avatar_url).to eq(avatar_path) diff --git a/spec/policies/project_snippet_policy_spec.rb b/spec/policies/project_snippet_policy_spec.rb index e1771b636b8..ddbed5f781e 100644 --- a/spec/policies/project_snippet_policy_spec.rb +++ b/spec/policies/project_snippet_policy_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe ProjectSnippetPolicy, models: true do let(:regular_user) { create(:user) } let(:external_user) { create(:user, :external) } - let(:project) { create(:empty_project) } + let(:project) { create(:empty_project, :public) } let(:author_permissions) do [ @@ -107,7 +107,7 @@ describe ProjectSnippetPolicy, models: true do end context 'snippet author' do - let(:snippet) { create(:project_snippet, :private, author: regular_user) } + let(:snippet) { create(:project_snippet, :private, author: regular_user, project: project) } subject { described_class.abilities(regular_user, snippet).to_set } diff --git a/spec/requests/openid_connect_spec.rb b/spec/requests/openid_connect_spec.rb index 05176c3beaa..6d1f0b24196 100644 --- a/spec/requests/openid_connect_spec.rb +++ b/spec/requests/openid_connect_spec.rb @@ -79,7 +79,7 @@ describe 'OpenID Connect requests' do 'email_verified' => true, 'website' => 'https://example.com', 'profile' => 'http://localhost/alice', - 'picture' => "http://localhost/uploads/user/avatar/#{user.id}/dk.png" + 'picture' => "http://localhost/uploads/system/user/avatar/#{user.id}/dk.png" }) end end diff --git a/spec/services/projects/participants_service_spec.rb b/spec/services/projects/participants_service_spec.rb index 0657b7e93fe..d75851134ee 100644 --- a/spec/services/projects/participants_service_spec.rb +++ b/spec/services/projects/participants_service_spec.rb @@ -13,7 +13,7 @@ describe Projects::ParticipantsService, services: true do groups = participants.groups expect(groups.size).to eq 1 - expect(groups.first[:avatar_url]).to eq("/uploads/group/avatar/#{group.id}/dk.png") + expect(groups.first[:avatar_url]).to eq("/uploads/system/group/avatar/#{group.id}/dk.png") end it 'should return an url for the avatar with relative url' do @@ -24,7 +24,7 @@ describe Projects::ParticipantsService, services: true do groups = participants.groups expect(groups.size).to eq 1 - expect(groups.first[:avatar_url]).to eq("/gitlab/uploads/group/avatar/#{group.id}/dk.png") + expect(groups.first[:avatar_url]).to eq("/gitlab/uploads/system/group/avatar/#{group.id}/dk.png") end end end diff --git a/spec/uploaders/attachment_uploader_spec.rb b/spec/uploaders/attachment_uploader_spec.rb index ea714fb08f0..d82dbe871d5 100644 --- a/spec/uploaders/attachment_uploader_spec.rb +++ b/spec/uploaders/attachment_uploader_spec.rb @@ -3,6 +3,17 @@ require 'spec_helper' describe AttachmentUploader do let(:uploader) { described_class.new(build_stubbed(:user)) } + describe "#store_dir" do + it "stores in the system dir" do + expect(uploader.store_dir).to start_with("uploads/system/user") + end + + it "uses the old path when using object storage" do + expect(described_class).to receive(:file_storage?).and_return(false) + expect(uploader.store_dir).to start_with("uploads/user") + end + end + describe '#move_to_cache' do it 'is true' do expect(uploader.move_to_cache).to eq(true) diff --git a/spec/uploaders/avatar_uploader_spec.rb b/spec/uploaders/avatar_uploader_spec.rb index c4d558805ab..201fe6949aa 100644 --- a/spec/uploaders/avatar_uploader_spec.rb +++ b/spec/uploaders/avatar_uploader_spec.rb @@ -3,6 +3,17 @@ require 'spec_helper' describe AvatarUploader do let(:uploader) { described_class.new(build_stubbed(:user)) } + describe "#store_dir" do + it "stores in the system dir" do + expect(uploader.store_dir).to start_with("uploads/system/user") + end + + it "uses the old path when using object storage" do + expect(described_class).to receive(:file_storage?).and_return(false) + expect(uploader.store_dir).to start_with("uploads/user") + end + end + describe '#move_to_cache' do it 'is false' do expect(uploader.move_to_cache).to eq(false) diff --git a/spec/uploaders/file_uploader_spec.rb b/spec/uploaders/file_uploader_spec.rb index d9113ef4095..47e9365e13d 100644 --- a/spec/uploaders/file_uploader_spec.rb +++ b/spec/uploaders/file_uploader_spec.rb @@ -15,6 +15,16 @@ describe FileUploader do end end + describe "#store_dir" do + it "stores in the namespace path" do + project = build_stubbed(:empty_project) + uploader = described_class.new(project) + + expect(uploader.store_dir).to include(project.path_with_namespace) + expect(uploader.store_dir).not_to include("system") + end + end + describe 'initialize' do it 'generates a secret if none is provided' do expect(SecureRandom).to receive(:hex).and_return('secret') |