Refactor abilities. Added ProjectUpdate context. Fixed few bugs with namespaces

Dmitriy Zaporozhets
1 parent a1ffc673
Showing 18 changed files with 127 additions and 53 deletions Show diff stats
app/contexts/project_update_context.rb
app/controllers/admin/projects_controller.rb
app/controllers/application_controller.rb
app/controllers/dashboard_controller.rb
app/controllers/groups_controller.rb
app/controllers/project_resource_controller.rb
app/controllers/projects_controller.rb
app/models/ability.rb
app/models/group.rb
app/models/namespace.rb
app/models/project.rb
app/models/user.rb
app/views/admin/projects/_form.html.haml
app/views/errors/access_denied.html.haml
app/views/groups/show.html.haml
db/fixtures/development/010_groups.rb
spec/models/group_spec.rb
spec/models/namespace_spec.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
+
@@ -24,13 +24,9 @@ class Admin::ProjectsController &lt; 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"
@@ -2,6 +2,7 @@ class ApplicationController &lt; 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 &lt; 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
@@ -5,7 +5,7 @@ class DashboardController &lt; 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)
@@ -4,7 +4,6 @@ class GroupsController &lt; 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 &lt; ApplicationController
   end
   def people
-    @users = group.users.all
+    @users = group.users
   end
   protected
@@ -55,7 +54,11 @@ class GroupsController &lt; 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
 class ProjectResourceController < ApplicationController
   before_filter :project
-  # Authorize
-  before_filter :add_project_abilities
 end
@@ -34,20 +34,10 @@ class ProjectsController &lt; 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
@@ -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
@@ -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
@@ -53,12 +53,14 @@ class Namespace &lt; 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
@@ -29,7 +29,7 @@ class Project &lt; 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
@@ -123,4 +123,11 @@ class User &lt; 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
@@ -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
-%h1 403
+%h1.http_status_code 403
 %h3.page_title Access Denied
 %hr
 %p You are not allowed to access this page.
@@ -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
@@ -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 }
+])
@@ -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
@@ -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
@@ -0,0 +1,21 @@		@@ -0,0 +1,21 @@
	1	+class ProjectUpdateContext < BaseContext
	2	+ def execute(role = :default)
	3	+ namespace_id = params[:project].delete(:namespace_id)
	4	+
	5	+ if namespace_id.present?
	6	+ if namespace_id == Namespace.global_id
	7	+ if project.namespace.present?
	8	+ # Transfer to global namespace from anyone
	9	+ project.transfer(nil)
	10	+ end
	11	+ elsif namespace_id.to_i != project.namespace_id
	12	+ # Transfer to someone namespace
	13	+ namespace = Namespace.find(namespace_id)
	14	+ project.transfer(namespace)
	15	+ end
	16	+ end
	17	+
	18	+ project.update_attributes(params[:project], as: role)
	19	+ end
	20	+end
	21	+
	@@ -5,7 +5,7 @@ class DashboardController < ApplicationController		@@ -5,7 +5,7 @@ class DashboardController < ApplicationController
5	before_filter :event_filter, only: :index	5	before_filter :event_filter, only: :index
6		6
7	def index	7	def index
8	- @groups = Group.where(id: current_user.projects.pluck(:namespace_id))	8	+ @groups = current_user.accessed_groups
9	@projects = @projects.page(params[:page]).per(30)	9	@projects = @projects.page(params[:page]).per(30)
10	@events = Event.in_projects(current_user.project_ids)	10	@events = Event.in_projects(current_user.project_ids)
11	@events = @event_filter.apply_filter(@events)	11	@events = @event_filter.apply_filter(@events)
1	class ProjectResourceController < ApplicationController	1	class ProjectResourceController < ApplicationController
2	before_filter :project	2	before_filter :project
3	- # Authorize
4	- before_filter :add_project_abilities
5	end	3	end
	@@ -34,20 +34,10 @@ class ProjectsController < ProjectResourceController		@@ -34,20 +34,10 @@ class ProjectsController < ProjectResourceController
34	end	34	end
35		35
36	def update	36	def update
37	- if params[:project].has_key?(:namespace_id)
38	- namespace_id = params[:project].delete(:namespace_id)
39	- if namespace_id == Namespace.global_id and project.namespace.present?
40	- # Transfer to global namespace from anyone
41	- project.transfer(nil)
42	- elsif namespace_id.present? and namespace_id.to_i != project.namespace_id
43	- # Transfer to someone namespace
44	- namespace = Namespace.find(namespace_id)
45	- project.transfer(namespace)
46	- end
47	- end	37	+ status = ProjectUpdateContext.new(project, current_user, params).execute
48		38
49	respond_to do \|format\|	39	respond_to do \|format\|
50	- if project.update_attributes(params[:project])	40	+ if status
51	flash[:notice] = 'Project was successfully updated.'	41	flash[:notice] = 'Project was successfully updated.'
52	format.html { redirect_to edit_project_path(project), notice: 'Project was successfully updated.' }	42	format.html { redirect_to edit_project_path(project), notice: 'Project was successfully updated.' }
53	format.js	43	format.js
	@@ -29,7 +29,7 @@ class Project < ActiveRecord::Base		@@ -29,7 +29,7 @@ class Project < ActiveRecord::Base
29	attr_accessible :name, :path, :description, :default_branch, :issues_enabled,	29	attr_accessible :name, :path, :description, :default_branch, :issues_enabled,
30	:wall_enabled, :merge_requests_enabled, :wiki_enabled, as: [:default, :admin]	30	:wall_enabled, :merge_requests_enabled, :wiki_enabled, as: [:default, :admin]
31		31
32	- attr_accessible :namespace_id, as: :admin	32	+ attr_accessible :namespace_id, :owner_id, as: :admin
33		33
34	attr_accessor :error_code	34	attr_accessor :error_code
35		35
	@@ -123,4 +123,11 @@ class User < ActiveRecord::Base		@@ -123,4 +123,11 @@ class User < ActiveRecord::Base
123	self.password = self.password_confirmation = Devise.friendly_token.first(8)	123	self.password = self.password_confirmation = Devise.friendly_token.first(8)
124	end	124	end
125	end	125	end
		126	+
		127	+ def accessed_groups
		128	+ @accessed_groups \|\|= begin
		129	+ groups = Group.where(id: self.projects.pluck(:namespace_id)).all
		130	+ groups + self.groups
		131	+ end
		132	+ end
126	end	133	end
	@@ -22,7 +22,7 @@		@@ -22,7 +22,7 @@
22	- unless project.new_record?	22	- unless project.new_record?
23	.clearfix	23	.clearfix
24	= f.label :namespace_id	24	= f.label :namespace_id
25	- .input= f.select :namespace_id, namespaces_options, {}, {class: 'chosen'}	25	+ .input= f.select :namespace_id, namespaces_options(@project.namespace_id), {}, {class: 'chosen'}
26		26
27	.clearfix	27	.clearfix
28	= f.label :owner_id	28	= f.label :owner_id
1	-%h1 403	1	+%h1.http_status_code 403
2	%h3.page_title Access Denied	2	%h3.page_title Access Denied
3	%hr	3	%hr
4	%p You are not allowed to access this page.	4	%p You are not allowed to access this page.
	@@ -10,7 +10,7 @@		@@ -10,7 +10,7 @@
10	- if @events.any?	10	- if @events.any?
11	.content_list= render @events	11	.content_list= render @events
12	- else	12	- else
13	- %h4.nothing_here_message Projects activity will be displayed here	13	+ %p.nothing_here_message Projects activity will be displayed here
14	.loading.hide	14	.loading.hide
15	.side	15	.side
16	= render "projects", projects: @projects	16	= render "projects", projects: @projects
@@ -0,0 +1,11 @@		@@ -0,0 +1,11 @@
	1	+Group.seed(:id, [
	2	+ { id: 100, name: "Gitlab", path: 'gitlab', owner_id: 1},
	3	+ { id: 101, name: "Rails", path: 'rails', owner_id: 1 },
	4	+ { id: 102, name: "KDE", path: 'kde', owner_id: 1 }
	5	+])
	6	+
	7	+Project.seed(:id, [
	8	+ { id: 10, name: "kdebase", path: "kdebase", owner_id: 1, namespace_id: 102 },
	9	+ { id: 11, name: "kdelibs", path: "kdelibs", owner_id: 1, namespace_id: 102 },
	10	+ { id: 12, name: "amarok", path: "amarok", owner_id: 1, namespace_id: 102 }
	11	+])