Merge branch 'upstream-9-2-security' into master-security-update

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
   #

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
-
-  def file_storage?
-    storage.is_a?(CarrierWave::Storage::File)
+  # 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 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

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: /.+/ }
 

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

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

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

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

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

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"

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

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

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

lib/gitlab/path_regex.rb

@@ -49,6 +49,7 @@ module Gitlab
       sent_notifications
       services
       snippets
+      system
       teams
       u
       unicorn_test

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

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

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

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

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

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

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

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

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

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',

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

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

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

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

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

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) }

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'))

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

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)

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

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

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)

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)

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')