Merge pull request #3403 from oliverguenther/fix/delete_managed_repo_permission

Fix/delete managed repo permission

app/services/scm/delete_managed_repository_service.rb

@@ -36,25 +36,32 @@ Scm::DeleteManagedRepositoryService = Struct.new :repository do
   #
   def call
     if repository.managed?
-
-      # Create necessary changes to repository to mark
-      # it as managed by OP, but delete asynchronously.
-      managed_path = repository.root_url
-
-      if File.directory?(managed_path)
-        ##
-        # We want to move this functionality in a Delayed Job,
-        # but this heavily interferes with the error handling of the whole
-        # repository management process.
-        # Instead, this will be refactored into a single service wrapper for
-        # creating and deleting repositories, which provides transactional DB access
-        # as well as filesystem access.
-        Scm::DeleteRepositoryJob.new(managed_path).perform
-      end
-
-      true
+      delete_repository
     else
       false
     end
   end
+
+  def delete_repository
+    # Create necessary changes to repository to mark
+    # it as managed by OP, but delete asynchronously.
+    managed_path = repository.root_url
+
+    if File.directory?(managed_path)
+      ##
+      # We want to move this functionality in a Delayed Job,
+      # but this heavily interferes with the error handling of the whole
+      # repository management process.
+      # Instead, this will be refactored into a single service wrapper for
+      # creating and deleting repositories, which provides transactional DB access
+      # as well as filesystem access.
+      Scm::DeleteRepositoryJob.new(managed_path).perform
+    end
+
+    true
+  rescue SystemCallError => e
+    Rails.logger.error("An error occurred while accessing the repository '#{repository.root_url}' \
+                        on filesystem: #{e.message}")
+    false
+  end
 end

app/workers/scm/create_repository_job.rb

@@ -38,12 +38,21 @@ class Scm::CreateRepositoryJob
   include OpenProject::BeforeDelayedJob
 
   def initialize(repository)
+    # TODO currently uses the full repository object,
+    # as the Job is performed synchronously.
+    # Change this to serialize the ID once its turned to process asynchronously.
     @repository = repository
   end
 
   def perform
     # Create the repository locally.
-    mode = config[:mode] || default_mode
+    mode = (config[:mode] || default_mode)
+
+    # Ensure that chmod receives an octal number
+    unless mode.is_a? Integer
+      mode = mode.to_i(8)
+    end
+
     create(mode)
 
     # Allow adapter to act upon the created repository

config/configuration.yml.example

@@ -223,7 +223,7 @@ default:
   #   when created in the frontend.
   #   NOTE: Disabling :managed repositories using disabled_types takes precedence over this setting.
   # mode:
-  #   The file mode to set the repository folder to (defaults to 0700).
+  #   The octal file mode to set the repository folder to (defaults to 0700).
   #
   # group:
   #   The group that should own the repository folder.

config/locales/en.yml

@@ -1318,7 +1318,7 @@ en:
       build_failed: "Unable to create the repository with the selected configuration. %{reason}"
       empty_repository: "The repository exists, but is empty. It does not contain any revisions yet."
       exists_on_filesystem: "The repository directory exists already on filesystem."
-      filesystem_access_failed: "An error occurred while creating the repository on filesystem: %{message}."
+      filesystem_access_failed: "An error occurred while accessing the repository on filesystem: %{message}."
       not_manageable: "This repository vendor cannot be managed by OpenProject."
       path_permission_failed: "An error occurred trying to create the following path: %{path}. Please ensure that OpenProject may write to that folder."
       unauthorized: "You're not authorized to access the repository or the credentials are invalid."

spec/app/services/scm/create_managed_repository_service_spec.rb

@@ -130,7 +130,7 @@ describe Scm::CreateManagedRepositoryService do
       it 'returns the correct error' do
         expect(service.call).to be false
         expect(service.localized_rejected_reason)
-          .to include('An error occurred while creating the repository on filesystem')
+          .to include('An error occurred while accessing the repository on filesystem')
       end
     end
   end

spec/app/services/scm/delete_managed_repository_service_spec.rb

@@ -92,6 +92,14 @@ describe Scm::DeleteManagedRepositoryService do
       expect(File.directory?(repository.root_url)).to be false
     end
 
+    it 'does not raise an exception upon permission errors' do
+      expect(File.directory?(repository.root_url)).to be true
+      expect(Scm::DeleteRepositoryJob)
+        .to receive(:new).and_raise(Errno::EACCES)
+
+      expect(service.call).to be false
+    end
+
     context 'and parent project' do
       let(:parent) { FactoryGirl.create(:project) }
       let(:project) { FactoryGirl.create(:project, parent: parent) }

spec/workers/scm/create_repository_job_spec.rb

@@ -0,0 +1,103 @@
+#-- encoding: UTF-8
+#-- copyright
+# OpenProject is a project management system.
+# Copyright (C) 2012-2015 the OpenProject Foundation (OPF)
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See doc/COPYRIGHT.rdoc for more details.
+#++
+
+require 'spec_helper'
+
+describe Scm::CreateRepositoryJob do
+  subject { described_class.new(repository) }
+
+  # Allow to override configuration values to determine
+  # whether to activate managed repositories
+  let(:enabled_scms) { %w[subversion git] }
+  let(:config) { nil }
+
+  before do
+    allow(Setting).to receive(:enabled_scm).and_return(enabled_scms)
+
+    allow(OpenProject::Configuration).to receive(:[]).and_call_original
+    allow(OpenProject::Configuration).to receive(:[]).with('scm').and_return(config)
+  end
+
+  describe 'with a managed repository' do
+    include_context 'with tmpdir'
+
+    let(:project) { FactoryGirl.build(:project) }
+    let(:repository) {
+      repo = Repository::Subversion.new(scm_type: :managed)
+      repo.project = project
+      repo.configure(:managed, nil)
+      repo
+    }
+
+    let(:config) {
+      { subversion: { mode: mode, manages: tmpdir } }
+    }
+
+    shared_examples 'creates a directory with mode' do |expected|
+      it 'creates the directory' do
+        subject.perform
+        expect(Dir.exists?(repository.root_url)).to be true
+
+        file_mode = File.stat(repository.root_url).mode
+        expect(sprintf("%o", file_mode)).to end_with(expected)
+      end
+    end
+
+    context 'with mode set' do
+      let(:mode) { 0770 }
+
+      it 'uses the correct mode' do
+        expect(subject).to receive(:create).with(mode)
+        subject.perform
+      end
+
+      it_behaves_like 'creates a directory with mode', '0770'
+    end
+
+    context 'with string mode' do
+      let(:mode) { '0770' }
+      it 'uses the correct mode' do
+        expect(subject).to receive(:create).with(0770)
+        subject.perform
+      end
+
+      it_behaves_like 'creates a directory with mode', '0770'
+    end
+
+    context 'with no mode set' do
+      let(:mode) { nil }
+      it 'uses the default mode' do
+        expect(subject).to receive(:create).with(0700)
+        subject.perform
+      end
+
+      it_behaves_like 'creates a directory with mode', '0700'
+    end
+  end
+end