From ec7b33484b78d874b8543847de72dc7489d55001 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Fri, 21 Aug 2015 16:34:52 +0200 Subject: [PATCH 1/3] Force integral mode ENV-based configuration passes a string to the chmod value, which seemingly can't be turned into an integer. Thus we work around this by casting the value to integer, will also help users who are using strings in their configs --- app/workers/scm/create_repository_job.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/workers/scm/create_repository_job.rb b/app/workers/scm/create_repository_job.rb index 0cec2307f5..03c8d6dbec 100644 --- a/app/workers/scm/create_repository_job.rb +++ b/app/workers/scm/create_repository_job.rb @@ -43,7 +43,8 @@ class Scm::CreateRepositoryJob def perform # Create the repository locally. - mode = config[:mode] || default_mode + # chmod requires integer value + mode = (config[:mode] || default_mode).to_i create(mode) # Allow adapter to act upon the created repository From c9d3dabab226de0677d8f625c5335da4fada3414 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Fri, 21 Aug 2015 16:35:49 +0200 Subject: [PATCH 2/3] Delete repository: Fix internal error on lacking permissions While rolling out on dev, we found that apache by default modifies the access permissions of a repository. This in turn causes the deletion of a repository to no longer complete sucessfully, but return with a permission error. This error was not caught an escalated into a 500 on the repository settings. --- .../scm/delete_managed_repository_service.rb | 41 +++++++++++-------- config/locales/en.yml | 2 +- .../create_managed_repository_service_spec.rb | 2 +- .../delete_managed_repository_service_spec.rb | 8 ++++ 4 files changed, 34 insertions(+), 19 deletions(-) diff --git a/app/services/scm/delete_managed_repository_service.rb b/app/services/scm/delete_managed_repository_service.rb index c5990bc169..65af32e4d1 100644 --- a/app/services/scm/delete_managed_repository_service.rb +++ b/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 diff --git a/config/locales/en.yml b/config/locales/en.yml index e2ed3f8781..53fd145ebd 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1299,7 +1299,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." diff --git a/spec/app/services/scm/create_managed_repository_service_spec.rb b/spec/app/services/scm/create_managed_repository_service_spec.rb index 4b67fdbc6d..a0156083f0 100644 --- a/spec/app/services/scm/create_managed_repository_service_spec.rb +++ b/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 diff --git a/spec/app/services/scm/delete_managed_repository_service_spec.rb b/spec/app/services/scm/delete_managed_repository_service_spec.rb index 0999048453..01b2221c0f 100644 --- a/spec/app/services/scm/delete_managed_repository_service_spec.rb +++ b/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) } From 701c49ab310ecf21e5af60a43d4939e95d62d9b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Tue, 25 Aug 2015 11:08:05 +0200 Subject: [PATCH 3/3] Correct parsing of string and octal chmod values --- app/workers/scm/create_repository_job.rb | 12 +- config/configuration.yml.example | 2 +- .../workers/scm/create_repository_job_spec.rb | 103 ++++++++++++++++++ 3 files changed, 114 insertions(+), 3 deletions(-) create mode 100644 spec/workers/scm/create_repository_job_spec.rb diff --git a/app/workers/scm/create_repository_job.rb b/app/workers/scm/create_repository_job.rb index 03c8d6dbec..7fd6af795e 100644 --- a/app/workers/scm/create_repository_job.rb +++ b/app/workers/scm/create_repository_job.rb @@ -38,13 +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. - # chmod requires integer value - mode = (config[:mode] || default_mode).to_i + 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 diff --git a/config/configuration.yml.example b/config/configuration.yml.example index 5f8706db47..8988cdeea7 100644 --- a/config/configuration.yml.example +++ b/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. diff --git a/spec/workers/scm/create_repository_job_spec.rb b/spec/workers/scm/create_repository_job_spec.rb new file mode 100644 index 0000000000..1b2a938aa9 --- /dev/null +++ b/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