From eb1004f7890d25a86beb0ca0a7eca802d9fce665 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 29 Nov 2012 07:29:11 +0300 Subject: [PATCH] Refactor abilities. Added ProjectUpdate context. Fixed few bugs with namespaces --- app/contexts/project_update_context.rb | 21 +++++++++ app/controllers/admin/projects_controller.rb | 8 +--- app/controllers/application_controller.rb | 13 ++++-- app/controllers/dashboard_controller.rb | 2 +- app/controllers/groups_controller.rb | 9 ++-- app/controllers/project_resource_controller.rb | 2 - app/controllers/projects_controller.rb | 14 +----- app/models/ability.rb | 64 ++++++++++++++++++++------ app/models/group.rb | 4 +- app/models/namespace.rb | 12 +++-- app/models/project.rb | 2 +- app/models/user.rb | 7 +++ app/views/admin/projects/_form.html.haml | 2 +- app/views/errors/access_denied.html.haml | 2 +- app/views/groups/show.html.haml | 2 +- db/fixtures/development/010_groups.rb | 11 +++++ spec/models/group_spec.rb | 2 +- spec/models/namespace_spec.rb | 3 +- 18 files changed, 127 insertions(+), 53 deletions(-) create mode 100644 app/contexts/project_update_context.rb create mode 100644 db/fixtures/development/010_groups.rb diff --git a/app/contexts/project_update_context.rb b/app/contexts/project_update_context.rb new file mode 100644 index 000000000..e28d43d0e --- /dev/null +++ b/app/contexts/project_update_context.rb @@ -0,0 +1,21 @@ +class ProjectUpdateContext < BaseContext + def execute(role = :default) + namespace_id = params[:project].delete(:namespace_id) + + if namespace_id.present? + if namespace_id == Namespace.global_id + if project.namespace.present? + # Transfer to global namespace from anyone + project.transfer(nil) + end + elsif namespace_id.to_i != project.namespace_id + # Transfer to someone namespace + namespace = Namespace.find(namespace_id) + project.transfer(namespace) + end + end + + project.update_attributes(params[:project], as: role) + end +end + diff --git a/app/controllers/admin/projects_controller.rb b/app/controllers/admin/projects_controller.rb index c3a419afd..e61f94f8c 100644 --- a/app/controllers/admin/projects_controller.rb +++ b/app/controllers/admin/projects_controller.rb @@ -24,13 +24,9 @@ class Admin::ProjectsController < AdminController end def update - owner_id = params[:project].delete(:owner_id) + status = ProjectUpdateContext.new(project, current_user, params).execute(:admin) - if owner_id - @project.owner = User.find(owner_id) - end - - if @project.update_attributes(params[:project], as: :admin) + if status redirect_to [:admin, @project], notice: 'Project was successfully updated.' else render action: "edit" diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 2be9a54da..66f2e87de 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -2,6 +2,7 @@ class ApplicationController < ActionController::Base before_filter :authenticate_user! before_filter :reject_blocked! before_filter :set_current_user_for_observers + before_filter :add_abilities before_filter :dev_tools if Rails.env == 'development' protect_from_forgery @@ -65,11 +66,17 @@ class ApplicationController < ActionController::Base def project id = params[:project_id] || params[:id] - @project ||= current_user.projects.find_with_namespace(id) - @project || render_404 + @project = Project.find_with_namespace(id) + + if @project and can?(current_user, :read_project, @project) + @project + else + @project = nil + render_404 + end end - def add_project_abilities + def add_abilities abilities << Ability end diff --git a/app/controllers/dashboard_controller.rb b/app/controllers/dashboard_controller.rb index 4f874a965..e01b586a3 100644 --- a/app/controllers/dashboard_controller.rb +++ b/app/controllers/dashboard_controller.rb @@ -5,7 +5,7 @@ class DashboardController < ApplicationController before_filter :event_filter, only: :index def index - @groups = Group.where(id: current_user.projects.pluck(:namespace_id)) + @groups = current_user.accessed_groups @projects = @projects.page(params[:page]).per(30) @events = Event.in_projects(current_user.project_ids) @events = @event_filter.apply_filter(@events) diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index c969f41eb..6fd5de8ab 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -4,7 +4,6 @@ class GroupsController < ApplicationController before_filter :group before_filter :projects - before_filter :add_project_abilities def show @events = Event.in_projects(project_ids).limit(20).offset(params[:offset] || 0) @@ -45,7 +44,7 @@ class GroupsController < ApplicationController end def people - @users = group.users.all + @users = group.users end protected @@ -55,7 +54,11 @@ class GroupsController < ApplicationController end def projects - @projects ||= current_user.projects_sorted_by_activity.where(namespace_id: @group.id) + @projects ||= if can?(current_user, :manage_group, @group) + @group.projects.all + else + current_user.projects_sorted_by_activity.where(namespace_id: @group.id) + end end def project_ids diff --git a/app/controllers/project_resource_controller.rb b/app/controllers/project_resource_controller.rb index d297bba63..81bc3a91b 100644 --- a/app/controllers/project_resource_controller.rb +++ b/app/controllers/project_resource_controller.rb @@ -1,5 +1,3 @@ class ProjectResourceController < ApplicationController before_filter :project - # Authorize - before_filter :add_project_abilities end diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index ed3406918..a6e7f1f93 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -34,20 +34,10 @@ class ProjectsController < ProjectResourceController end def update - if params[:project].has_key?(:namespace_id) - namespace_id = params[:project].delete(:namespace_id) - if namespace_id == Namespace.global_id and project.namespace.present? - # Transfer to global namespace from anyone - project.transfer(nil) - elsif namespace_id.present? and namespace_id.to_i != project.namespace_id - # Transfer to someone namespace - namespace = Namespace.find(namespace_id) - project.transfer(namespace) - end - end + status = ProjectUpdateContext.new(project, current_user, params).execute respond_to do |format| - if project.update_attributes(params[:project]) + if status flash[:notice] = 'Project was successfully updated.' format.html { redirect_to edit_project_path(project), notice: 'Project was successfully updated.' } format.js diff --git a/app/models/ability.rb b/app/models/ability.rb index e55e77093..96d3ac6dd 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -15,7 +15,37 @@ class Ability def project_abilities(user, project) rules = [] - rules << [ + # Rules based on role in project + if project.master_access_for?(user) + # TODO: replace with master rules. + # Only allow project administration for owners + rules << project_admin_rules + + elsif project.dev_access_for?(user) + rules << project_dev_rules + + elsif project.report_access_for?(user) + rules << project_report_rules + + elsif project.guest_access_for?(user) + rules << project_guest_rules + end + + # If user own project namespace (Ex. group owner or account owner) + if project.namespace && project.namespace.owner == user + rules << project_admin_rules + end + + # If user was set as direct project owner + if project.owner == user + rules << project_admin_rules + end + + rules.flatten + end + + def project_guest_rules + [ :read_project, :read_wiki, :read_issue, @@ -27,28 +57,30 @@ class Ability :write_project, :write_issue, :write_note - ] if project.guest_access_for?(user) + ] + end - rules << [ + def project_report_rules + project_guest_rules + [ :download_code, :write_merge_request, :write_snippet - ] if project.report_access_for?(user) + ] + end - rules << [ + def project_dev_rules + project_report_rules + [ :write_wiki, :push_code - ] if project.dev_access_for?(user) - - rules << [ - :push_code_to_protected_branches - ] if project.master_access_for?(user) + ] + end - rules << [ + def project_master_rules + project_dev_rules + [ + :push_code_to_protected_branches, :modify_issue, :modify_snippet, :modify_merge_request, - :admin_project, :admin_issue, :admin_milestone, :admin_snippet, @@ -57,9 +89,13 @@ class Ability :admin_note, :accept_mr, :admin_wiki - ] if project.master_access_for?(user) || project.owner == user + ] + end - rules.flatten + def project_admin_rules + project_master_rules + [ + :admin_project + ] end def group_abilities user, group diff --git a/app/models/group.rb b/app/models/group.rb index 66267c569..b668f5560 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -13,7 +13,9 @@ class Group < Namespace def users - User.joins(:users_projects).where(users_projects: {project_id: project_ids}).uniq + users = User.joins(:users_projects).where(users_projects: {project_id: project_ids}) + users = users << owner + users.uniq end def human_name diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 5762bfc57..e1c24de94 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -53,12 +53,14 @@ class Namespace < ActiveRecord::Base end def move_dir - old_path = File.join(Gitlab.config.git_base_path, path_was) - new_path = File.join(Gitlab.config.git_base_path, path) - if File.exists?(new_path) - raise "Already exists" + if path_changed? + old_path = File.join(Gitlab.config.git_base_path, path_was) + new_path = File.join(Gitlab.config.git_base_path, path) + if File.exists?(new_path) + raise "Already exists" + end + system("mv #{old_path} #{new_path}") end - system("mv #{old_path} #{new_path}") end def rm_dir diff --git a/app/models/project.rb b/app/models/project.rb index 0c74c0bd8..8df662db9 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -29,7 +29,7 @@ class Project < ActiveRecord::Base attr_accessible :name, :path, :description, :default_branch, :issues_enabled, :wall_enabled, :merge_requests_enabled, :wiki_enabled, as: [:default, :admin] - attr_accessible :namespace_id, as: :admin + attr_accessible :namespace_id, :owner_id, as: :admin attr_accessor :error_code diff --git a/app/models/user.rb b/app/models/user.rb index 43163404e..d43e3cbb6 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -123,4 +123,11 @@ class User < ActiveRecord::Base self.password = self.password_confirmation = Devise.friendly_token.first(8) end end + + def accessed_groups + @accessed_groups ||= begin + groups = Group.where(id: self.projects.pluck(:namespace_id)).all + groups + self.groups + end + end end diff --git a/app/views/admin/projects/_form.html.haml b/app/views/admin/projects/_form.html.haml index eb12c61f7..110ff04a5 100644 --- a/app/views/admin/projects/_form.html.haml +++ b/app/views/admin/projects/_form.html.haml @@ -22,7 +22,7 @@ - unless project.new_record? .clearfix = f.label :namespace_id - .input= f.select :namespace_id, namespaces_options, {}, {class: 'chosen'} + .input= f.select :namespace_id, namespaces_options(@project.namespace_id), {}, {class: 'chosen'} .clearfix = f.label :owner_id diff --git a/app/views/errors/access_denied.html.haml b/app/views/errors/access_denied.html.haml index a6e4f0b94..f2d082cb7 100644 --- a/app/views/errors/access_denied.html.haml +++ b/app/views/errors/access_denied.html.haml @@ -1,4 +1,4 @@ -%h1 403 +%h1.http_status_code 403 %h3.page_title Access Denied %hr %p You are not allowed to access this page. diff --git a/app/views/groups/show.html.haml b/app/views/groups/show.html.haml index 72d7ad9a5..0c2eb150f 100644 --- a/app/views/groups/show.html.haml +++ b/app/views/groups/show.html.haml @@ -10,7 +10,7 @@ - if @events.any? .content_list= render @events - else - %h4.nothing_here_message Projects activity will be displayed here + %p.nothing_here_message Projects activity will be displayed here .loading.hide .side = render "projects", projects: @projects diff --git a/db/fixtures/development/010_groups.rb b/db/fixtures/development/010_groups.rb new file mode 100644 index 000000000..09371b007 --- /dev/null +++ b/db/fixtures/development/010_groups.rb @@ -0,0 +1,11 @@ +Group.seed(:id, [ + { id: 100, name: "Gitlab", path: 'gitlab', owner_id: 1}, + { id: 101, name: "Rails", path: 'rails', owner_id: 1 }, + { id: 102, name: "KDE", path: 'kde', owner_id: 1 } +]) + +Project.seed(:id, [ + { id: 10, name: "kdebase", path: "kdebase", owner_id: 1, namespace_id: 102 }, + { id: 11, name: "kdelibs", path: "kdelibs", owner_id: 1, namespace_id: 102 }, + { id: 12, name: "amarok", path: "amarok", owner_id: 1, namespace_id: 102 } +]) diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 884726266..108bc3035 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -24,7 +24,7 @@ describe Group do it { should validate_presence_of :owner } describe :users do - it { group.users.should == [] } + it { group.users.should == [group.owner] } end describe :human_name do diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index 1f1d66150..d0de4a7b7 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -55,9 +55,10 @@ describe Namespace do describe :move_dir do before do @namespace = create :namespace + @namespace.stub(path_changed?: true) end - it "should raise error when called directly" do + it "should raise error when dirtory exists" do expect { @namespace.move_dir }.to raise_error("Already exists") end -- 2.11.0