From 6ba4cb1d2b0bcfaf8459c8936513637acc92da5e Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 28 May 2014 19:01:19 +0300 Subject: [PATCH 1/7] Remove old rake task Signed-off-by: Dmitriy Zaporozhets --- lib/tasks/gitlab/enable_namespaces.rake | 111 ------------------------ 1 file changed, 111 deletions(-) delete mode 100644 lib/tasks/gitlab/enable_namespaces.rake diff --git a/lib/tasks/gitlab/enable_namespaces.rake b/lib/tasks/gitlab/enable_namespaces.rake deleted file mode 100644 index 201f34ab546..00000000000 --- a/lib/tasks/gitlab/enable_namespaces.rake +++ /dev/null @@ -1,111 +0,0 @@ -namespace :gitlab do - desc "GITLAB | Enable usernames and namespaces for user projects" - task enable_namespaces: :environment do - warn_user_is_not_gitlab - - migrate_user_namespaces - migrate_groups - migrate_projects - end - - def migrate_user_namespaces - puts "\nGenerate usernames for users without one: ".blue - User.find_each(batch_size: 500) do |user| - if user.namespace - print '-'.cyan - next - end - - username = if user.username.present? - # if user already has username filled - user.username - else - build_username(user) - end - - begin - User.transaction do - user.update_attributes!(username: username) - print '.'.green - end - rescue - print 'F'.red - end - end - puts "\nDone" - end - - def build_username(user) - username = nil - - # generate username - username = user.email.match(/^[^@]*/)[0] - username.gsub!("+", ".") - - # return username if no matches - return username unless User.find_by(username: username) - - # look for same username - (1..10).each do |i| - suffixed_username = "#{username}#{i}" - - return suffixed_username unless User.find_by(username: suffixed_username) - end - end - - def migrate_groups - puts "\nCreate directories for groups: ".blue - - Group.find_each(batch_size: 500) do |group| - begin - if group.dir_exists? - print '-'.cyan - else - if group.ensure_dir_exist - print '.'.green - else - print 'F'.red - end - end - rescue - print 'F'.red - end - end - puts "\nDone" - end - - def migrate_projects - git_path = Gitlab.config.gitlab_shell.repos_path - puts "\nMove projects in groups into respective directories ... ".blue - Project.where('namespace_id IS NOT NULL').find_each(batch_size: 500) do |project| - next unless project.group - - group = project.group - - print "#{project.name_with_namespace.yellow} ... " - - new_path = File.join(git_path, project.path_with_namespace + '.git') - - if File.exists?(new_path) - puts "already at #{new_path}".green - next - end - - old_path = File.join(git_path, project.path + '.git') - - unless File.exists?(old_path) - puts "couldn't find it at #{old_path}".red - next - end - - begin - project.transfer(group.path) - puts "moved to #{new_path}".green - rescue - puts "failed moving to #{new_path}".red - end - end - - puts "\nDone" - end -end From aca6be50d3dc1963491c9dcff61dac3b3ec937ca Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 28 May 2014 19:02:26 +0300 Subject: [PATCH 2/7] Refactor project transfer service. Add security check when create or transfer project into group Signed-off-by: Dmitriy Zaporozhets --- app/services/project_transfer_service.rb | 46 -------------- app/services/projects/create_service.rb | 2 +- app/services/projects/transfer_service.rb | 73 +++++++++++++++++++---- 3 files changed, 62 insertions(+), 59 deletions(-) delete mode 100644 app/services/project_transfer_service.rb diff --git a/app/services/project_transfer_service.rb b/app/services/project_transfer_service.rb deleted file mode 100644 index 7055ef32aee..00000000000 --- a/app/services/project_transfer_service.rb +++ /dev/null @@ -1,46 +0,0 @@ -# ProjectTransferService class -# -# Used for transfer project to another namespace -# -class ProjectTransferService - include Gitlab::ShellAdapter - - class TransferError < StandardError; end - - attr_accessor :project - - def transfer(project, new_namespace) - Project.transaction do - old_path = project.path_with_namespace - new_path = File.join(new_namespace.try(:path) || '', project.path) - - if Project.where(path: project.path, namespace_id: new_namespace.try(:id)).present? - raise TransferError.new("Project with same path in target namespace already exists") - end - - # Remove old satellite - project.satellite.destroy - - # Apply new namespace id - project.namespace = new_namespace - project.save! - - # Move main repository - unless gitlab_shell.mv_repository(old_path, new_path) - raise TransferError.new('Cannot move project') - end - - # Move wiki repo also if present - gitlab_shell.mv_repository("#{old_path}.wiki", "#{new_path}.wiki") - - # Create a new satellite (reload project from DB) - Project.find(project.id).ensure_satellite_exists - - # clear project cached events - project.reset_events_cache - - true - end - end -end - diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb index 11b65256502..7d322f25e44 100644 --- a/app/services/projects/create_service.rb +++ b/app/services/projects/create_service.rb @@ -97,7 +97,7 @@ module Projects def allowed_namespace?(user, namespace_id) namespace = Namespace.find_by(id: namespace_id) - current_user.can?(:manage_namespace, namespace) + current_user.can?(:create_projects, namespace) end end end diff --git a/app/services/projects/transfer_service.rb b/app/services/projects/transfer_service.rb index f8e27f6f1c6..d115e92a105 100644 --- a/app/services/projects/transfer_service.rb +++ b/app/services/projects/transfer_service.rb @@ -1,22 +1,71 @@ +# Projects::TransferService class +# +# Used for transfer project to another namespace +# +# Ex. +# # Move projects to namespace with ID 17 by user +# Projects::TransferService.new(project, user, namespace_id: 17).execute +# module Projects class TransferService < BaseService - def execute(role = :default) - namespace_id = params[:project].delete(:namespace_id) - allowed_transfer = can?(current_user, :change_namespace, project) || role == :admin + include Gitlab::ShellAdapter + class TransferError < StandardError; end - if allowed_transfer && namespace_id.present? - if namespace_id.to_i != project.namespace_id - # Transfer to someone namespace - namespace = Namespace.find(namespace_id) - project.transfer(namespace) - end + def execute + namespace_id = params.delete(:namespace_id) + namespace = Namespace.find_by(id: namespace_id) + + if allowed_transfer?(current_user, project, namespace) + transfer(project, namespace) + else + project.errors.add(:namespace, 'is invalid') + false end - - rescue ProjectTransferService::TransferError => ex + rescue Projects::TransferService::TransferError => ex project.reload project.errors.add(:namespace_id, ex.message) false end + + def transfer(project, new_namespace) + Project.transaction do + old_path = project.path_with_namespace + new_path = File.join(new_namespace.try(:path) || '', project.path) + + if Project.where(path: project.path, namespace_id: new_namespace.try(:id)).present? + raise TransferError.new("Project with same path in target namespace already exists") + end + + # Remove old satellite + project.satellite.destroy + + # Apply new namespace id + project.namespace = new_namespace + project.save! + + # Move main repository + unless gitlab_shell.mv_repository(old_path, new_path) + raise TransferError.new('Cannot move project') + end + + # Move wiki repo also if present + gitlab_shell.mv_repository("#{old_path}.wiki", "#{new_path}.wiki") + + # Create a new satellite (reload project from DB) + Project.find(project.id).ensure_satellite_exists + + # clear project cached events + project.reset_events_cache + + true + end + end + + def allowed_transfer?(current_user, project, namespace) + namespace && + can?(current_user, :change_namespace, project) && + namespace.id != project.namespace_id && + current_user.can?(:create_projects, namespace) + end end end - From aea79b80351109506bd089694df6f22785456f68 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 28 May 2014 19:03:01 +0300 Subject: [PATCH 3/7] Add ability rule for creating project in namespace Signed-off-by: Dmitriy Zaporozhets --- app/models/ability.rb | 8 ++++++++ app/models/group.rb | 6 +++++- app/models/project.rb | 4 ---- app/models/user.rb | 2 ++ 4 files changed, 15 insertions(+), 5 deletions(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index 1afe8a4638f..70c26caded8 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -188,6 +188,13 @@ class Ability rules << :read_group end + # Only group masters and group owners can create new projects in group + if group.has_master?(user) || group.has_owner?(user) || user.admin? + rules += [ + :create_projects, + ] + end + # Only group owner and administrators can manage group if group.has_owner?(user) || user.admin? rules += [ @@ -205,6 +212,7 @@ class Ability # Only namespace owner and administrators can manage it if namespace.owner == user || user.admin? rules += [ + :create_projects, :manage_namespace ] end diff --git a/app/models/group.rb b/app/models/group.rb index 3cbf30a20df..2e68779d367 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -26,7 +26,7 @@ class Group < Namespace validates :avatar, file_size: { maximum: 100.kilobytes.to_i } mount_uploader :avatar, AttachmentUploader - + def self.accessible_to(user) accessible_ids = Project.accessible_to(user).pluck(:namespace_id) accessible_ids += user.groups.pluck(:id) if user @@ -60,6 +60,10 @@ class Group < Namespace owners.include?(user) end + def has_master?(user) + members.masters.where(user_id: user).any? + end + def last_owner?(user) has_owner?(user) && owners.size == 1 end diff --git a/app/models/project.rb b/app/models/project.rb index fc7c7d042f3..1e74ae735ba 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -387,10 +387,6 @@ class Project < ActiveRecord::Base end end - def transfer(new_namespace) - ProjectTransferService.new.transfer(self, new_namespace) - end - def execute_hooks(data, hooks_scope = :push_hooks) hooks.send(hooks_scope).each do |hook| hook.async_execute(data) diff --git a/app/models/user.rb b/app/models/user.rb index 16961e5413b..f1b6139745e 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -90,6 +90,8 @@ class User < ActiveRecord::Base has_many :users_groups, dependent: :destroy has_many :groups, through: :users_groups has_many :owned_groups, -> { where users_groups: { group_access: UsersGroup::OWNER } }, through: :users_groups, source: :group + has_many :masters_groups, -> { where users_groups: { group_access: UsersGroup::MASTER } }, through: :users_groups, source: :group + # Projects has_many :groups_projects, through: :groups, source: :projects has_many :personal_projects, through: :namespace, source: :projects From 900d30798b7eef1301978ff6f97b4cee7414b696 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 28 May 2014 19:03:45 +0300 Subject: [PATCH 4/7] Use new Projects::TransferService class Signed-off-by: Dmitriy Zaporozhets --- app/controllers/projects_controller.rb | 2 +- lib/api/groups.rb | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 07ccbd57faf..b9af36a0c7e 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -44,7 +44,7 @@ class ProjectsController < ApplicationController end def transfer - ::Projects::TransferService.new(project, current_user, params).execute + ::Projects::TransferService.new(project, current_user, params[:project]).execute end def show diff --git a/lib/api/groups.rb b/lib/api/groups.rb index 03f027706de..caa2ca97a3e 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -87,10 +87,12 @@ module API # POST /groups/:id/projects/:project_id post ":id/projects/:project_id" do authenticated_as_admin! - @group = Group.find(params[:id]) + group = Group.find(params[:id]) project = Project.find(params[:project_id]) - if project.transfer(@group) - present @group + result = ::Projects::TransferService.new(project, current_user, namespace_id: group.id).execute + + if result + present group else not_found! end From 14f78d067d703c20b788498cd90c40903ba9f84e Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 28 May 2014 19:04:18 +0300 Subject: [PATCH 5/7] Modify specs for new project transfer code Signed-off-by: Dmitriy Zaporozhets --- spec/requests/api/groups_spec.rb | 4 +- .../projects/transfer_service_spec.rb | 41 ++++++++++++------- 2 files changed, 29 insertions(+), 16 deletions(-) diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index 6fcab85335a..f27a60e4bc0 100644 --- a/spec/requests/api/groups_spec.rb +++ b/spec/requests/api/groups_spec.rb @@ -147,7 +147,7 @@ describe API::API, api: true do describe "POST /groups/:id/projects/:project_id" do let(:project) { create(:project) } before(:each) do - project.stub(:transfer).and_return(true) + Projects::TransferService.any_instance.stub(execute: true) Project.stub(:find).and_return(project) end @@ -160,8 +160,8 @@ describe API::API, api: true do context "when authenticated as admin" do it "should transfer project to group" do - project.should_receive(:transfer) post api("/groups/#{group1.id}/projects/#{project.id}", admin) + response.status.should == 201 end end end diff --git a/spec/services/projects/transfer_service_spec.rb b/spec/services/projects/transfer_service_spec.rb index 109b429967e..563a8b0c01c 100644 --- a/spec/services/projects/transfer_service_spec.rb +++ b/spec/services/projects/transfer_service_spec.rb @@ -1,16 +1,20 @@ require 'spec_helper' -describe ProjectTransferService do +describe Projects::TransferService do before(:each) { enable_observers } after(:each) {disable_observers} - context 'namespace -> namespace' do - let(:user) { create(:user) } - let(:group) { create(:group) } - let(:project) { create(:project, namespace: user.namespace) } + let(:user) { create(:user) } + let(:group) { create(:group) } + let(:group2) { create(:group) } + let(:project) { create(:project, namespace: user.namespace) } + context 'namespace -> namespace' do before do - @result = service.transfer(project, group) + group.add_owner(user) + @service = Projects::TransferService.new(project, user, namespace_id: group.id) + @service.gitlab_shell.stub(mv_repository: true) + @result = @service.execute end it { @result.should be_true } @@ -18,16 +22,25 @@ describe ProjectTransferService do end context 'namespace -> no namespace' do - let(:user) { create(:user) } - let(:project) { create(:project, namespace: user.namespace) } + before do + group.add_owner(user) + @service = Projects::TransferService.new(project, user, namespace_id: nil) + @service.gitlab_shell.stub(mv_repository: true) + @result = @service.execute + end - it { lambda{service.transfer(project, nil)}.should raise_error(ActiveRecord::RecordInvalid) } + it { @result.should be_false } + it { project.namespace.should == user.namespace } end - def service - service = ProjectTransferService.new - service.gitlab_shell.stub(mv_repository: true) - service + context 'namespace -> not allowed namespace' do + before do + @service = Projects::TransferService.new(project, user, namespace_id: group2.id) + @service.gitlab_shell.stub(mv_repository: true) + @result = @service.execute + end + + it { @result.should be_false } + it { project.namespace.should == user.namespace } end end - From 1932f902d51d17ff4eb4ebfd15a16fac55cf9eb4 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 28 May 2014 19:04:42 +0300 Subject: [PATCH 6/7] Allow masters to create projects in groups Signed-off-by: Dmitriy Zaporozhets --- app/helpers/namespaces_helper.rb | 2 +- app/views/groups/_projects.html.haml | 2 +- doc/permissions/permissions.md | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/helpers/namespaces_helper.rb b/app/helpers/namespaces_helper.rb index c363c7ffd74..bf25dce2301 100644 --- a/app/helpers/namespaces_helper.rb +++ b/app/helpers/namespaces_helper.rb @@ -1,6 +1,6 @@ module NamespacesHelper def namespaces_options(selected = :current_user, scope = :default) - groups = current_user.owned_groups + groups = current_user.owned_groups + current_user.masters_groups users = [current_user.namespace] group_opts = ["Groups", groups.sort_by(&:human_name).map {|g| [g.human_name, g.id]} ] diff --git a/app/views/groups/_projects.html.haml b/app/views/groups/_projects.html.haml index 41f8cb9da40..4ded28058ed 100644 --- a/app/views/groups/_projects.html.haml +++ b/app/views/groups/_projects.html.haml @@ -1,7 +1,7 @@ .ui-box .title Projects (#{projects.count}) - - if can? current_user, :manage_group, @group + - if can? current_user, :create_projects, @group %span.pull-right = link_to new_project_path(namespace_id: @group.id), class: "btn btn-new" do %i.icon-plus diff --git a/doc/permissions/permissions.md b/doc/permissions/permissions.md index 840bb90163b..95b47a86129 100644 --- a/doc/permissions/permissions.md +++ b/doc/permissions/permissions.md @@ -40,7 +40,7 @@ If a user is a GitLab administrator they receive all permissions. |------|-----|--------|---------|------|-----| |Browse group|✓|✓|✓|✓|✓| |Edit group|||||✓| -|Create project in group|||||✓| +|Create project in group||||✓|✓| |Manage group members|||||✓| |Remove group|||||✓| From cea0fc1de80242a5849cff343148328028b81914 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 28 May 2014 19:33:49 +0300 Subject: [PATCH 7/7] Fix tests Signed-off-by: Dmitriy Zaporozhets --- spec/models/project_spec.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index b42d7bfe606..a2519fbd684 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -84,7 +84,6 @@ describe Project do it { should respond_to(:satellite) } it { should respond_to(:update_merge_requests) } it { should respond_to(:execute_hooks) } - it { should respond_to(:transfer) } it { should respond_to(:name_with_namespace) } it { should respond_to(:owner) } it { should respond_to(:path_with_namespace) }