From b9c551302253f86a8a85e8288099696b1d8ccdd6 Mon Sep 17 00:00:00 2001
From: Kamil Trzcinski <ayufan@ayufan.eu>
Date: Tue, 21 Jul 2015 22:48:34 +0200
Subject: Fix: user could steal specific runner

- check if user has manage access to project
- don't cache result of authorized_projects, because it's serialised with User object
- clear user sessions
---
 CHANGELOG                                      |  3 +++
 app/models/user.rb                             |  5 ++++-
 db/migrate/20150706103229_truncate_sessions.rb |  9 ---------
 db/migrate/20150721204649_truncate_sessions.rb |  9 +++++++++
 db/schema.rb                                   |  2 +-
 spec/models/user_spec.rb                       | 20 ++++++++++++++++----
 6 files changed, 33 insertions(+), 15 deletions(-)
 delete mode 100644 db/migrate/20150706103229_truncate_sessions.rb
 create mode 100644 db/migrate/20150721204649_truncate_sessions.rb

diff --git a/CHANGELOG b/CHANGELOG
index 035c7e3..b8fcb24 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,3 +1,6 @@
+v7.13.1
+  - Fix: user could steal specific runner
+
 v7.13.0
   - Allow to specify image and services in yml that can be used with docker
   - Fix: No runner notification can see managers only
diff --git a/app/models/user.rb b/app/models/user.rb
index 138e5e4..1523577 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -71,7 +71,10 @@ class User
   end
 
   def authorized_projects
-    @authorized_projects ||= Project.where(gitlab_id: gitlab_projects.map(&:id))
+    Project.where(gitlab_id: gitlab_projects.map(&:id)).select do |project|
+      # This is slow: it makes request to GitLab for each project to verify manage permission
+      can_manage_project?(project.gitlab_id)
+    end
   end
 
   private
diff --git a/db/migrate/20150706103229_truncate_sessions.rb b/db/migrate/20150706103229_truncate_sessions.rb
deleted file mode 100644
index 32fe651..0000000
--- a/db/migrate/20150706103229_truncate_sessions.rb
+++ /dev/null
@@ -1,9 +0,0 @@
-class TruncateSessions < ActiveRecord::Migration
-  def up
-    execute('DELETE FROM sessions')
-  end
-
-  def down
-    execute('DELETE FROM sessions')
-  end
-end
diff --git a/db/migrate/20150721204649_truncate_sessions.rb b/db/migrate/20150721204649_truncate_sessions.rb
new file mode 100644
index 0000000..32fe651
--- /dev/null
+++ b/db/migrate/20150721204649_truncate_sessions.rb
@@ -0,0 +1,9 @@
+class TruncateSessions < ActiveRecord::Migration
+  def up
+    execute('DELETE FROM sessions')
+  end
+
+  def down
+    execute('DELETE FROM sessions')
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index b42bf0c..1363111 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20150710113851) do
+ActiveRecord::Schema.define(version: 20150721204649) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 4a1e393..73a7a7d 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -53,16 +53,27 @@ describe User do
   end
 
   describe "authorized_projects" do
-    it "returns projects" do
-      project = FactoryGirl.create :project, gitlab_id: 1
-      project1 = FactoryGirl.create :project, gitlab_id: 2
+    let (:user) { User.new({}) }
+
+    before do
+      FactoryGirl.create :project, gitlab_id: 1
+      FactoryGirl.create :project, gitlab_id: 2
       gitlab_project = OpenStruct.new({id: 1})
       gitlab_project1 = OpenStruct.new({id: 2})
       User.any_instance.stub(:gitlab_projects).and_return([gitlab_project, gitlab_project1])
-      user = User.new({})
+    end
+
+    it "returns projects" do
+      User.any_instance.stub(:can_manage_project?).and_return(true)
 
       user.authorized_projects.count.should == 2
     end
+
+    it "empty list if user miss manage permission" do
+      User.any_instance.stub(:can_manage_project?).and_return(false)
+
+      user.authorized_projects.count.should == 0
+    end
   end
 
   describe "authorized_runners" do
@@ -72,6 +83,7 @@ describe User do
       gitlab_project = OpenStruct.new({id: 1})
       gitlab_project1 = OpenStruct.new({id: 2})
       User.any_instance.stub(:gitlab_projects).and_return([gitlab_project, gitlab_project1])
+      User.any_instance.stub(:can_manage_project?).and_return(true)
       user = User.new({})
 
       runner = FactoryGirl.create :specific_runner
-- 
cgit v1.2.1