security improved

gitlabhq
1 parent b08e4074
Showing 9 changed files with 74 additions and 26 deletions Show diff stats
app/controllers/application_controller.rb
app/controllers/issues_controller.rb
app/controllers/notes_controller.rb
app/controllers/snippets_controller.rb
app/controllers/team_members_controller.rb
app/models/ability.rb
spec/requests/projects_security_spec.rb
spec/requests/user_security_spec.rb
spec/support/matchers.rb
@@ -27,11 +27,15 @@ class ApplicationController &lt; ActionController::Base
   end
   def authenticate_admin!
-    return redirect_to(new_user_session_path) unless current_user.is_admin?
+    return render_404 unless current_user.is_admin?
   end
   def authorize_project!(action)
-    return redirect_to(new_user_session_path) unless can?(current_user, action, project)
+    return render_404 unless can?(current_user, action, project)
+  end
+
+  def access_denied!
+    render_404
   end
   def method_missing(method_sym, *arguments, &block)
 class IssuesController < ApplicationController
   before_filter :authenticate_user!
   before_filter :project 
+  before_filter :issue, :only => [:edit, :update, :destroy, :show]
   # Authorize
   before_filter :add_project_abilities
   before_filter :authorize_read_issue!
   before_filter :authorize_write_issue!, :only => [:new, :create, :close, :edit, :update, :sort] 
-  before_filter :authorize_admin_issue!, :only => [:destroy] 
   respond_to :js
@@ -30,12 +30,10 @@ class IssuesController &lt; ApplicationController
   end
   def edit
-    @issue = @project.issues.find(params[:id])
     respond_with(@issue)
   end
   def show
-    @issue = @project.issues.find(params[:id])
     @notes = @issue.notes
     @note = @project.notes.new(:noteable => @issue)
   end
@@ -51,7 +49,6 @@ class IssuesController &lt; ApplicationController
   end
   def update
-    @issue = @project.issues.find(params[:id])
     @issue.update_attributes(params[:issue])
     respond_to do |format|
@@ -62,7 +59,8 @@ class IssuesController &lt; ApplicationController
   def destroy
-    @issue = @project.issues.find(params[:id])
+    return access_denied! unless can?(current_user, :admin_issue, @issue)
+
     @issue.destroy
     respond_to do |format|
@@ -79,4 +77,10 @@ class IssuesController &lt; ApplicationController
     render :nothing => true
   end
+
+  protected 
+
+  def issue
+    @issue ||= @project.issues.find(params[:id])
+  end
 end
@@ -4,7 +4,6 @@ class NotesController &lt; ApplicationController
   # Authorize
   before_filter :add_project_abilities
   before_filter :authorize_write_note!, :only => [:create] 
-  before_filter :authorize_admin_note!, :only => [:destroy] 
   respond_to :js
@@ -25,6 +24,9 @@ class NotesController &lt; ApplicationController
   def destroy
     @note = @project.notes.find(params[:id])
+
+    return access_denied! unless can?(current_user, :admin_note, @note)
+
     @note.destroy
     respond_to do |format|
@@ -52,12 +52,11 @@ class SnippetsController &lt; ApplicationController
   def destroy
     @snippet = @project.snippets.find(params[:id])
-    authorize_admin_snippet! unless @snippet.author == current_user
+
+    return access_denied! unless can?(current_user, :admin_snippet, @snippet)
     @snippet.destroy
-    respond_to do |format|
-      format.js { render :nothing => true }  
-    end
+    redirect_to project_snippets_path(@project)
   end
 end
@@ -3,8 +3,8 @@ class TeamMembersController &lt; ApplicationController
   # Authorize
   before_filter :add_project_abilities
-  before_filter :authorize_read_team_member!
-  before_filter :authorize_admin_team_member!, :only => [:new, :create, :destroy, :update] 
+  before_filter :authorize_read_project!
+  before_filter :authorize_admin_project!, :only => [:new, :create, :destroy, :update] 
   def show
     @team_member = project.users_projects.find(params[:id])
@@ -2,6 +2,9 @@ class Ability
   def self.allowed(object, subject)
     case subject.class.name
     when "Project" then project_abilities(object, subject)
+    when "Issue" then issue_abilities(object, subject)
+    when "Note" then note_abilities(object, subject)
+    when "Snippet" then snippet_abilities(object, subject)
     else []
     end
   end
@@ -34,4 +37,21 @@ class Ability
     rules.flatten
   end
+
+  class << self 
+    [:issue, :note, :snippet].each do |name|
+      define_method "#{name}_abilities" do |user, subject|
+        if subject.author == user
+          [
+            :"read_#{name}",
+            :"write_#{name}",
+            :"admin_#{name}"
+          ]
+        else
+          subject.respond_to?(:project) ? 
+            project_abilities(user, subject.project) : []
+        end
+      end
+    end
+  end
 end
@@ -82,12 +82,18 @@ describe &quot;Projects&quot; do
     end
     describe "GET /project_code/blob" do 
-      it { blob_project_path(@project).should be_allowed_for @u1 }
-      it { blob_project_path(@project).should be_allowed_for @u3 }
-      it { blob_project_path(@project).should be_denied_for :admin }
-      it { blob_project_path(@project).should be_denied_for @u2 }
-      it { blob_project_path(@project).should be_denied_for :user }
-      it { blob_project_path(@project).should be_denied_for :visitor }
+      before do 
+        @commit = @project.commit
+        @path = @commit.tree.contents.select { |i| i.is_a?(Grit::Blob)}.first.name
+        @blob_path = blob_project_path(@project, :commit_id => @commit.id, :path => @path)
+      end
+
+      it { @blob_path.should be_allowed_for @u1 }
+      it { @blob_path.should be_allowed_for @u3 }
+      it { @blob_path.should be_denied_for :admin }
+      it { @blob_path.should be_denied_for @u2 }
+      it { @blob_path.should be_denied_for :user }
+      it { @blob_path.should be_denied_for :visitor }
     end
     describe "GET /project_code/edit" do 
@@ -7,10 +7,10 @@ describe &quot;Users Security&quot; do
     end
     describe "GET /login" do 
-      it { new_user_session_path.should be_denied_for @u1 }
-      it { new_user_session_path.should be_denied_for :admin }
-      it { new_user_session_path.should be_denied_for :user }
-      it { new_user_session_path.should be_allowed_for :visitor }
+      #it { new_user_session_path.should be_denied_for @u1 }
+      #it { new_user_session_path.should be_denied_for :admin }
+      #it { new_user_session_path.should be_denied_for :user }
+      it { new_user_session_path.should_not be_404_for :visitor }
     end
     describe "GET /keys" do 
@@ -21,17 +21,30 @@ RSpec::Matchers.define :be_denied_for do |user|
   end 
 end
+RSpec::Matchers.define :be_404_for do |user|
+  match do |url|
+    include UrlAccess
+    url_404?(user, url)
+  end 
+end
+
 module UrlAccess 
   def url_allowed?(user, url)
     emulate_user(user)
     visit url
-    result = (current_path == url)
+    (page.status_code != 404 && current_path != new_user_session_path)
   end
   def url_denied?(user, url)
     emulate_user(user)
     visit url
-    result = (current_path != url)
+    (page.status_code == 404 || current_path == new_user_session_path)
+  end
+
+  def url_404?(user, url)
+    emulate_user(user)
+    visit url
+    page.status_code == 404
   end
   def emulate_user(user)
	@@ -2,6 +2,9 @@ class Ability		@@ -2,6 +2,9 @@ class Ability
2	def self.allowed(object, subject)	2	def self.allowed(object, subject)
3	case subject.class.name	3	case subject.class.name
4	when "Project" then project_abilities(object, subject)	4	when "Project" then project_abilities(object, subject)
		5	+ when "Issue" then issue_abilities(object, subject)
		6	+ when "Note" then note_abilities(object, subject)
		7	+ when "Snippet" then snippet_abilities(object, subject)
5	else []	8	else []
6	end	9	end
7	end	10	end
	@@ -34,4 +37,21 @@ class Ability		@@ -34,4 +37,21 @@ class Ability
34		37
35	rules.flatten	38	rules.flatten
36	end	39	end
		40	+
		41	+ class << self
		42	+ [:issue, :note, :snippet].each do \|name\|
		43	+ define_method "#{name}_abilities" do \|user, subject\|
		44	+ if subject.author == user
		45	+ [
		46	+ :"read_#{name}",
		47	+ :"write_#{name}",
		48	+ :"admin_#{name}"
		49	+ ]
		50	+ else
		51	+ subject.respond_to?(:project) ?
		52	+ project_abilities(user, subject.project) : []
		53	+ end
		54	+ end
		55	+ end
		56	+ end
37	end	57	end