From 2a3db599b84b5eb14e4b2c01eb2b8cad4331b2bb Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Mon, 8 Aug 2016 10:07:15 -0700
Subject: [PATCH 1/7] remove six, and use a Set instead

---
 Gemfile                                    |  3 ---
 Gemfile.lock                               |  2 --
 app/models/ability.rb                      | 17 ++++++++++++-----
 lib/api/helpers.rb                         |  6 +-----
 spec/models/members/project_member_spec.rb |  3 +--
 spec/models/note_spec.rb                   |  3 +--
 spec/models/project_security_spec.rb       |  3 +--
 7 files changed, 16 insertions(+), 21 deletions(-)

diff --git a/Gemfile b/Gemfile
index 8f94ee72a322..3a7f156b2e65 100644
--- a/Gemfile
+++ b/Gemfile
@@ -97,9 +97,6 @@ gem 'fog-rackspace', '~> 0.1.1'
 # for aws storage
 gem 'unf', '~> 0.1.4'
 
-# Authorization
-gem 'six', '~> 0.2.0'
-
 # Seed data
 gem 'seed-fu', '~> 2.3.5'
 
diff --git a/Gemfile.lock b/Gemfile.lock
index 870f9397b9e1..594ce36ab8b4 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -682,7 +682,6 @@ GEM
       rack (~> 1.5)
       rack-protection (~> 1.4)
       tilt (>= 1.3, < 3)
-    six (0.2.0)
     slack-notifier (1.2.1)
     slop (3.6.0)
     spinach (0.8.10)
@@ -964,7 +963,6 @@ DEPENDENCIES
   sidekiq-cron (~> 0.4.0)
   simplecov (= 0.12.0)
   sinatra (~> 1.4.4)
-  six (~> 0.2.0)
   slack-notifier (~> 1.2.0)
   spinach-rails (~> 0.2.1)
   spinach-rerun-reporter (~> 0.0.2)
diff --git a/app/models/ability.rb b/app/models/ability.rb
index d9113ffd99a6..6822274defaa 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -1,5 +1,15 @@
 class Ability
   class << self
+    def cached_allowed(user, subject)
+      user_key = user ? user.id : 'anonymous'
+      key = "/ability/#{user_key}/#{subject.object_id}"
+      RequestStore[key] ||= Set.new(allowed(user, subject))
+    end
+
+    def allowed?(user, action, subject)
+      cached_allowed(user, subject).include?(action)
+    end
+
     # rubocop: disable Metrics/CyclomaticComplexity
     def allowed(user, subject)
       return anonymous_abilities(user, subject) if user.nil?
@@ -570,11 +580,8 @@ def user_abilities
     end
 
     def abilities
-      @abilities ||= begin
-        abilities = Six.new
-        abilities << self
-        abilities
-      end
+      warn 'Ability.abilities is deprecated, use Ability.allowed?(user, action, subject) instead'
+      self
     end
 
     private
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 130509cdad6c..91113d69d6ec 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -400,11 +400,7 @@ def pagination_links(paginated_data)
     end
 
     def abilities
-      @abilities ||= begin
-                       abilities = Six.new
-                       abilities << Ability
-                       abilities
-                     end
+      Ability
     end
 
     def secret_token
diff --git a/spec/models/members/project_member_spec.rb b/spec/models/members/project_member_spec.rb
index e7c0c5064631..c8605a444aa5 100644
--- a/spec/models/members/project_member_spec.rb
+++ b/spec/models/members/project_member_spec.rb
@@ -70,8 +70,7 @@
 
   describe :import_team do
     before do
-      @abilities = Six.new
-      @abilities << Ability
+      @abilities = Ability
 
       @project_1 = create :project
       @project_2 = create :project
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index 1243f5420a74..b30fd9855b7a 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -83,8 +83,7 @@
       @u1 = create(:user)
       @u2 = create(:user)
       @u3 = create(:user)
-      @abilities = Six.new
-      @abilities << Ability
+      @abilities = Ability
     end
 
     describe 'read' do
diff --git a/spec/models/project_security_spec.rb b/spec/models/project_security_spec.rb
index 2142c7c13ef9..7dfaf6a7317b 100644
--- a/spec/models/project_security_spec.rb
+++ b/spec/models/project_security_spec.rb
@@ -10,8 +10,7 @@
       @u3 = create(:user)
       @u4 = @p1.owner
 
-      @abilities = Six.new
-      @abilities << Ability
+      @abilities = Ability
     end
 
     let(:guest_actions) { Ability.project_guest_rules }
-- 
GitLab


From 0a696bd2d0da9c3d190129d1bb87d9259c14a3b1 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Mon, 8 Aug 2016 11:55:13 -0700
Subject: [PATCH 2/7] remove Ability.abilities

---
 app/controllers/application_controller.rb     |  8 ++------
 app/finders/issuable_finder.rb                |  2 +-
 app/finders/todos_finder.rb                   |  2 +-
 app/mailers/base_mailer.rb                    |  2 +-
 app/models/ability.rb                         |  5 -----
 app/models/event.rb                           |  2 +-
 app/models/merge_request.rb                   |  2 +-
 app/models/user.rb                            |  6 +-----
 app/services/base_service.rb                  |  6 +-----
 lib/api/helpers.rb                            |  8 ++------
 lib/banzai/reference_parser/base_parser.rb    |  2 +-
 .../reference_parser/base_parser_spec.rb      |  8 ++++----
 .../reference_parser/user_parser_spec.rb      | 10 +++++-----
 spec/models/members/project_member_spec.rb    |  6 ++----
 spec/models/note_spec.rb                      | 19 +++++++++---------
 spec/models/project_security_spec.rb          | 20 +++++++++----------
 16 files changed, 41 insertions(+), 67 deletions(-)

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 634d36a44671..55b7511930fe 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -23,7 +23,7 @@ class ApplicationController < ActionController::Base
 
   protect_from_forgery with: :exception
 
-  helper_method :abilities, :can?, :current_application_settings
+  helper_method :can?, :current_application_settings
   helper_method :import_sources_enabled?, :github_import_enabled?, :github_import_configured?, :gitlab_import_enabled?, :gitlab_import_configured?, :bitbucket_import_enabled?, :bitbucket_import_configured?, :gitorious_import_enabled?, :google_code_import_enabled?, :fogbugz_import_enabled?, :git_import_enabled?, :gitlab_project_import_enabled?
 
   rescue_from Encoding::CompatibilityError do |exception|
@@ -118,12 +118,8 @@ def after_sign_out_path_for(resource)
     current_application_settings.after_sign_out_path.presence || new_user_session_path
   end
 
-  def abilities
-    Ability.abilities
-  end
-
   def can?(object, action, subject)
-    abilities.allowed?(object, action, subject)
+    Ability.allowed?(object, action, subject)
   end
 
   def access_denied!
diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb
index 33daac0399e2..60996b181f22 100644
--- a/app/finders/issuable_finder.rb
+++ b/app/finders/issuable_finder.rb
@@ -64,7 +64,7 @@ def project
     if project?
       @project = Project.find(params[:project_id])
 
-      unless Ability.abilities.allowed?(current_user, :read_project, @project)
+      unless Ability.allowed?(current_user, :read_project, @project)
         @project = nil
       end
     else
diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb
index ff866c2faa50..882fd053b987 100644
--- a/app/finders/todos_finder.rb
+++ b/app/finders/todos_finder.rb
@@ -81,7 +81,7 @@ def project
     if project?
       @project = Project.find(params[:project_id])
 
-      unless Ability.abilities.allowed?(current_user, :read_project, @project)
+      unless Ability.allowed?(current_user, :read_project, @project)
         @project = nil
       end
     else
diff --git a/app/mailers/base_mailer.rb b/app/mailers/base_mailer.rb
index 8b83bbd93b74..61a574d3dc0d 100644
--- a/app/mailers/base_mailer.rb
+++ b/app/mailers/base_mailer.rb
@@ -9,7 +9,7 @@ class BaseMailer < ActionMailer::Base
   default reply_to: Proc.new { default_reply_to_address.format }
 
   def can?
-    Ability.abilities.allowed?(current_user, action, subject)
+    Ability.allowed?(current_user, action, subject)
   end
 
   private
diff --git a/app/models/ability.rb b/app/models/ability.rb
index 6822274defaa..f5a89c4623af 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -579,11 +579,6 @@ def user_abilities
       [:read_user]
     end
 
-    def abilities
-      warn 'Ability.abilities is deprecated, use Ability.allowed?(user, action, subject) instead'
-      self
-    end
-
     private
 
     def restricted_public_level?
diff --git a/app/models/event.rb b/app/models/event.rb
index fd736d123593..a0b7b0dc2b59 100644
--- a/app/models/event.rb
+++ b/app/models/event.rb
@@ -65,7 +65,7 @@ def visible_to_user?(user = nil)
     elsif created_project?
       true
     elsif issue? || issue_note?
-      Ability.abilities.allowed?(user, :read_issue, note? ? note_target : target)
+      Ability.allowed?(user, :read_issue, note? ? note_target : target)
     else
       ((merge_request? || note?) && target.present?) || milestone?
     end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index b1fb3ce5d697..97adb10aafb3 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -386,7 +386,7 @@ def can_cancel_merge_when_build_succeeds?(current_user)
   def can_remove_source_branch?(current_user)
     !source_project.protected_branch?(source_branch) &&
       !source_project.root_ref?(source_branch) &&
-      Ability.abilities.allowed?(current_user, :push_code, source_project) &&
+      Ability.allowed?(current_user, :push_code, source_project) &&
       diff_head_commit == source_branch_head
   end
 
diff --git a/app/models/user.rb b/app/models/user.rb
index db7474349598..3a0cb2f692d9 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -453,16 +453,12 @@ def can_create_group?
     can?(:create_group, nil)
   end
 
-  def abilities
-    Ability.abilities
-  end
-
   def can_select_namespace?
     several_namespaces? || admin
   end
 
   def can?(action, subject)
-    abilities.allowed?(self, action, subject)
+    Ability.allowed?(self, action, subject)
   end
 
   def first_name
diff --git a/app/services/base_service.rb b/app/services/base_service.rb
index 0d55ba5a9816..0c208150fb86 100644
--- a/app/services/base_service.rb
+++ b/app/services/base_service.rb
@@ -7,12 +7,8 @@ def initialize(project, user, params = {})
     @project, @current_user, @params = project, user, params.dup
   end
 
-  def abilities
-    Ability.abilities
-  end
-
   def can?(object, action, subject)
-    abilities.allowed?(object, action, subject)
+    Ability.allowed?(object, action, subject)
   end
 
   def notification_service
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 91113d69d6ec..632b9656d161 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -135,7 +135,7 @@ def authenticated_as_admin!
     end
 
     def authorize!(action, subject)
-      forbidden! unless abilities.allowed?(current_user, action, subject)
+      forbidden! unless can?(current_user, action, subject)
     end
 
     def authorize_push_project
@@ -153,7 +153,7 @@ def require_gitlab_workhorse!
     end
 
     def can?(object, action, subject)
-      abilities.allowed?(object, action, subject)
+      Ability.allowed?(object, action, subject)
     end
 
     # Checks the occurrences of required attributes, each attribute must be present in the params hash
@@ -399,10 +399,6 @@ def pagination_links(paginated_data)
       links.join(', ')
     end
 
-    def abilities
-      Ability
-    end
-
     def secret_token
       File.read(Gitlab.config.gitlab_shell.secret_file).chomp
     end
diff --git a/lib/banzai/reference_parser/base_parser.rb b/lib/banzai/reference_parser/base_parser.rb
index 6cf218aaa0d3..e8e03e4a98fe 100644
--- a/lib/banzai/reference_parser/base_parser.rb
+++ b/lib/banzai/reference_parser/base_parser.rb
@@ -211,7 +211,7 @@ def projects_for_nodes(nodes)
       end
 
       def can?(user, permission, subject)
-        Ability.abilities.allowed?(user, permission, subject)
+        Ability.allowed?(user, permission, subject)
       end
 
       def find_projects_for_hash_keys(hash)
diff --git a/spec/lib/banzai/reference_parser/base_parser_spec.rb b/spec/lib/banzai/reference_parser/base_parser_spec.rb
index ac9c66e2663b..9095d2b1345b 100644
--- a/spec/lib/banzai/reference_parser/base_parser_spec.rb
+++ b/spec/lib/banzai/reference_parser/base_parser_spec.rb
@@ -30,7 +30,7 @@
       it 'returns the nodes if the attribute value equals the current project ID' do
         link['data-project'] = project.id.to_s
 
-        expect(Ability.abilities).not_to receive(:allowed?)
+        expect(Ability).not_to receive(:allowed?)
         expect(subject.nodes_visible_to_user(user, [link])).to eq([link])
       end
 
@@ -39,7 +39,7 @@
 
         link['data-project'] = other_project.id.to_s
 
-        expect(Ability.abilities).to receive(:allowed?).
+        expect(Ability).to receive(:allowed?).
           with(user, :read_project, other_project).
           and_return(true)
 
@@ -57,7 +57,7 @@
 
         link['data-project'] = other_project.id.to_s
 
-        expect(Ability.abilities).to receive(:allowed?).
+        expect(Ability).to receive(:allowed?).
           with(user, :read_project, other_project).
           and_return(false)
 
@@ -221,7 +221,7 @@
     it 'delegates the permissions check to the Ability class' do
       user = double(:user)
 
-      expect(Ability.abilities).to receive(:allowed?).
+      expect(Ability).to receive(:allowed?).
         with(user, :read_project, project)
 
       subject.can?(user, :read_project, project)
diff --git a/spec/lib/banzai/reference_parser/user_parser_spec.rb b/spec/lib/banzai/reference_parser/user_parser_spec.rb
index 9a82891297d3..4e7f82a6e093 100644
--- a/spec/lib/banzai/reference_parser/user_parser_spec.rb
+++ b/spec/lib/banzai/reference_parser/user_parser_spec.rb
@@ -82,7 +82,7 @@
         end
 
         it 'returns the nodes if the user can read the group' do
-          expect(Ability.abilities).to receive(:allowed?).
+          expect(Ability).to receive(:allowed?).
             with(user, :read_group, group).
             and_return(true)
 
@@ -90,7 +90,7 @@
         end
 
         it 'returns an empty Array if the user can not read the group' do
-          expect(Ability.abilities).to receive(:allowed?).
+          expect(Ability).to receive(:allowed?).
             with(user, :read_group, group).
             and_return(false)
 
@@ -103,7 +103,7 @@
           it 'returns the nodes if the attribute value equals the current project ID' do
             link['data-project'] = project.id.to_s
 
-            expect(Ability.abilities).not_to receive(:allowed?)
+            expect(Ability).not_to receive(:allowed?)
 
             expect(subject.nodes_visible_to_user(user, [link])).to eq([link])
           end
@@ -113,7 +113,7 @@
 
             link['data-project'] = other_project.id.to_s
 
-            expect(Ability.abilities).to receive(:allowed?).
+            expect(Ability).to receive(:allowed?).
               with(user, :read_project, other_project).
               and_return(true)
 
@@ -125,7 +125,7 @@
 
             link['data-project'] = other_project.id.to_s
 
-            expect(Ability.abilities).to receive(:allowed?).
+            expect(Ability).to receive(:allowed?).
               with(user, :read_project, other_project).
               and_return(false)
 
diff --git a/spec/models/members/project_member_spec.rb b/spec/models/members/project_member_spec.rb
index c8605a444aa5..bfb7dda0e45b 100644
--- a/spec/models/members/project_member_spec.rb
+++ b/spec/models/members/project_member_spec.rb
@@ -70,8 +70,6 @@
 
   describe :import_team do
     before do
-      @abilities = Ability
-
       @project_1 = create :project
       @project_2 = create :project
 
@@ -90,8 +88,8 @@
       it { expect(@project_2.users).to include(@user_1) }
       it { expect(@project_2.users).to include(@user_2) }
 
-      it { expect(@abilities.allowed?(@user_1, :create_project, @project_2)).to be_truthy }
-      it { expect(@abilities.allowed?(@user_2, :read_project, @project_2)).to be_truthy }
+      it { expect(Ability.allowed?(@user_1, :create_project, @project_2)).to be_truthy }
+      it { expect(Ability.allowed?(@user_2, :read_project, @project_2)).to be_truthy }
     end
 
     describe 'project 1 should not be changed' do
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index b30fd9855b7a..dd52a77a3682 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -83,7 +83,6 @@
       @u1 = create(:user)
       @u2 = create(:user)
       @u3 = create(:user)
-      @abilities = Ability
     end
 
     describe 'read' do
@@ -92,9 +91,9 @@
         @p2.project_members.create(user: @u3, access_level: ProjectMember::GUEST)
       end
 
-      it { expect(@abilities.allowed?(@u1, :read_note, @p1)).to be_falsey }
-      it { expect(@abilities.allowed?(@u2, :read_note, @p1)).to be_truthy }
-      it { expect(@abilities.allowed?(@u3, :read_note, @p1)).to be_falsey }
+      it { expect(Ability.allowed?(@u1, :read_note, @p1)).to be_falsey }
+      it { expect(Ability.allowed?(@u2, :read_note, @p1)).to be_truthy }
+      it { expect(Ability.allowed?(@u3, :read_note, @p1)).to be_falsey }
     end
 
     describe 'write' do
@@ -103,9 +102,9 @@
         @p2.project_members.create(user: @u3, access_level: ProjectMember::DEVELOPER)
       end
 
-      it { expect(@abilities.allowed?(@u1, :create_note, @p1)).to be_falsey }
-      it { expect(@abilities.allowed?(@u2, :create_note, @p1)).to be_truthy }
-      it { expect(@abilities.allowed?(@u3, :create_note, @p1)).to be_falsey }
+      it { expect(Ability.allowed?(@u1, :create_note, @p1)).to be_falsey }
+      it { expect(Ability.allowed?(@u2, :create_note, @p1)).to be_truthy }
+      it { expect(Ability.allowed?(@u3, :create_note, @p1)).to be_falsey }
     end
 
     describe 'admin' do
@@ -115,9 +114,9 @@
         @p2.project_members.create(user: @u3, access_level: ProjectMember::MASTER)
       end
 
-      it { expect(@abilities.allowed?(@u1, :admin_note, @p1)).to be_falsey }
-      it { expect(@abilities.allowed?(@u2, :admin_note, @p1)).to be_truthy }
-      it { expect(@abilities.allowed?(@u3, :admin_note, @p1)).to be_falsey }
+      it { expect(Ability.allowed?(@u1, :admin_note, @p1)).to be_falsey }
+      it { expect(Ability.allowed?(@u2, :admin_note, @p1)).to be_truthy }
+      it { expect(Ability.allowed?(@u3, :admin_note, @p1)).to be_falsey }
     end
   end
 
diff --git a/spec/models/project_security_spec.rb b/spec/models/project_security_spec.rb
index 7dfaf6a7317b..d1c19ea8f370 100644
--- a/spec/models/project_security_spec.rb
+++ b/spec/models/project_security_spec.rb
@@ -9,8 +9,6 @@
       @u2 = create(:user)
       @u3 = create(:user)
       @u4 = @p1.owner
-
-      @abilities = Ability
     end
 
     let(:guest_actions) { Ability.project_guest_rules }
@@ -22,7 +20,7 @@
     describe "Non member rules" do
       it "should deny for non-project users any actions" do
         owner_actions.each do |action|
-          expect(@abilities.allowed?(@u1, action, @p1)).to be_falsey
+          expect(Ability.allowed?(@u1, action, @p1)).to be_falsey
         end
       end
     end
@@ -34,7 +32,7 @@
 
       it "should allow for project user any guest actions" do
         guest_actions.each do |action|
-          expect(@abilities.allowed?(@u2, action, @p1)).to be_truthy
+          expect(Ability.allowed?(@u2, action, @p1)).to be_truthy
         end
       end
     end
@@ -46,7 +44,7 @@
 
       it "should allow for project user any report actions" do
         report_actions.each do |action|
-          expect(@abilities.allowed?(@u2, action, @p1)).to be_truthy
+          expect(Ability.allowed?(@u2, action, @p1)).to be_truthy
         end
       end
     end
@@ -59,13 +57,13 @@
 
       it "should deny for developer master-specific actions" do
         [dev_actions - report_actions].each do |action|
-          expect(@abilities.allowed?(@u2, action, @p1)).to be_falsey
+          expect(Ability.allowed?(@u2, action, @p1)).to be_falsey
         end
       end
 
       it "should allow for project user any dev actions" do
         dev_actions.each do |action|
-          expect(@abilities.allowed?(@u3, action, @p1)).to be_truthy
+          expect(Ability.allowed?(@u3, action, @p1)).to be_truthy
         end
       end
     end
@@ -78,13 +76,13 @@
 
       it "should deny for developer master-specific actions" do
         [master_actions - dev_actions].each do |action|
-          expect(@abilities.allowed?(@u2, action, @p1)).to be_falsey
+          expect(Ability.allowed?(@u2, action, @p1)).to be_falsey
         end
       end
 
       it "should allow for project user any master actions" do
         master_actions.each do |action|
-          expect(@abilities.allowed?(@u3, action, @p1)).to be_truthy
+          expect(Ability.allowed?(@u3, action, @p1)).to be_truthy
         end
       end
     end
@@ -97,13 +95,13 @@
 
       it "should deny for masters admin-specific actions" do
         [owner_actions - master_actions].each do |action|
-          expect(@abilities.allowed?(@u2, action, @p1)).to be_falsey
+          expect(Ability.allowed?(@u2, action, @p1)).to be_falsey
         end
       end
 
       it "should allow for project owner any admin actions" do
         owner_actions.each do |action|
-          expect(@abilities.allowed?(@u4, action, @p1)).to be_truthy
+          expect(Ability.allowed?(@u4, action, @p1)).to be_truthy
         end
       end
     end
-- 
GitLab


From 665ef990d07902f280834070e1e09330de3d12cc Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Mon, 8 Aug 2016 14:02:29 -0700
Subject: [PATCH 3/7] don't double-cache project_abilites

---
 app/models/ability.rb | 41 +++++++++++++++++++----------------------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/app/models/ability.rb b/app/models/ability.rb
index f5a89c4623af..3b4ce8b934a2 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -175,37 +175,34 @@ def global_abilities(user)
 
     def project_abilities(user, project)
       rules = []
-      key = "/user/#{user.id}/project/#{project.id}"
 
-      RequestStore.store[key] ||= begin
-        # Push abilities on the users team role
-        rules.push(*project_team_rules(project.team, user))
+      # Push abilities on the users team role
+      rules.push(*project_team_rules(project.team, user))
 
-        owner = user.admin? ||
-                project.owner == user ||
-                (project.group && project.group.has_owner?(user))
+      owner = user.admin? ||
+              project.owner == user ||
+              (project.group && project.group.has_owner?(user))
 
-        if owner
-          rules.push(*project_owner_rules)
-        end
-
-        if project.public? || (project.internal? && !user.external?)
-          rules.push(*public_project_rules)
+      if owner
+        rules.push(*project_owner_rules)
+      end
 
-          # Allow to read builds for internal projects
-          rules << :read_build if project.public_builds?
+      if project.public? || (project.internal? && !user.external?)
+        rules.push(*public_project_rules)
 
-          unless owner || project.team.member?(user) || project_group_member?(project, user)
-            rules << :request_access if project.request_access_enabled
-          end
-        end
+        # Allow to read builds for internal projects
+        rules << :read_build if project.public_builds?
 
-        if project.archived?
-          rules -= project_archived_rules
+        unless owner || project.team.member?(user) || project_group_member?(project, user)
+          rules << :request_access if project.request_access_enabled
         end
+      end
 
-        rules - project_disabled_features_rules(project)
+      if project.archived?
+        rules -= project_archived_rules
       end
+
+      rules - project_disabled_features_rules(project)
     end
 
     def project_team_rules(team, user)
-- 
GitLab


From 0719b8ed1242e8506ee0e5797ec2ba1d61211ba8 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Tue, 9 Aug 2016 13:56:54 -0700
Subject: [PATCH 4/7] re-enable the cyclomatic complexity checker

---
 app/models/ability.rb | 1 -
 1 file changed, 1 deletion(-)

diff --git a/app/models/ability.rb b/app/models/ability.rb
index 3b4ce8b934a2..68d926bfbbdd 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -10,7 +10,6 @@ def allowed?(user, action, subject)
       cached_allowed(user, subject).include?(action)
     end
 
-    # rubocop: disable Metrics/CyclomaticComplexity
     def allowed(user, subject)
       return anonymous_abilities(user, subject) if user.nil?
       return [] unless user.is_a?(User)
-- 
GitLab


From 176cde89806c7e1ca5efb909a36ad33222b003db Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Tue, 9 Aug 2016 14:16:38 -0700
Subject: [PATCH 5/7] move cached_allowed and make almost everything private

---
 app/models/ability.rb | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/app/models/ability.rb b/app/models/ability.rb
index 68d926bfbbdd..93d14e5b139a 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -1,15 +1,11 @@
 class Ability
   class << self
-    def cached_allowed(user, subject)
-      user_key = user ? user.id : 'anonymous'
-      key = "/ability/#{user_key}/#{subject.object_id}"
-      RequestStore[key] ||= Set.new(allowed(user, subject))
-    end
-
     def allowed?(user, action, subject)
       cached_allowed(user, subject).include?(action)
     end
 
+    private
+
     def allowed(user, subject)
       return anonymous_abilities(user, subject) if user.nil?
       return [] unless user.is_a?(User)
@@ -18,6 +14,12 @@ def allowed(user, subject)
       abilities_by_subject_class(user: user, subject: subject)
     end
 
+    def cached_allowed(user, subject)
+      user_key = user ? user.id : 'anonymous'
+      key = "/ability/#{user_key}/#{subject.object_id}"
+      RequestStore[key] ||= Set.new(allowed(user, subject))
+    end
+
     def abilities_by_subject_class(user:, subject:)
       case subject
       when CommitStatus then commit_status_abilities(user, subject)
@@ -575,8 +577,6 @@ def user_abilities
       [:read_user]
     end
 
-    private
-
     def restricted_public_level?
       current_application_settings.restricted_visibility_levels.include?(Gitlab::VisibilityLevel::PUBLIC)
     end
-- 
GitLab


From 7254721b175f5b09f97833579ca1fb44fc1cb925 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Tue, 9 Aug 2016 15:15:32 -0700
Subject: [PATCH 6/7] reinstate necessary methods as public

---
 app/models/ability.rb | 64 +++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/app/models/ability.rb b/app/models/ability.rb
index 93d14e5b139a..70e37babd392 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -1,5 +1,37 @@
 class Ability
   class << self
+    # Given a list of users and a project this method returns the users that can
+    # read the given project.
+    def users_that_can_read_project(users, project)
+      if project.public?
+        users
+      else
+        users.select do |user|
+          if user.admin?
+            true
+          elsif project.internal? && !user.external?
+            true
+          elsif project.owner == user
+            true
+          elsif project.team.members.include?(user)
+            true
+          else
+            false
+          end
+        end
+      end
+    end
+
+    # Returns an Array of Issues that can be read by the given user.
+    #
+    # issues - The issues to reduce down to those readable by the user.
+    # user - The User for which to check the issues
+    def issues_readable_by_user(issues, user = nil)
+      return issues if user && user.admin?
+
+      issues.select { |issue| issue.visible_to_user?(user) }
+    end
+
     def allowed?(user, action, subject)
       cached_allowed(user, subject).include?(action)
     end
@@ -40,38 +72,6 @@ def abilities_by_subject_class(user:, subject:)
       end.concat(global_abilities(user))
     end
 
-    # Given a list of users and a project this method returns the users that can
-    # read the given project.
-    def users_that_can_read_project(users, project)
-      if project.public?
-        users
-      else
-        users.select do |user|
-          if user.admin?
-            true
-          elsif project.internal? && !user.external?
-            true
-          elsif project.owner == user
-            true
-          elsif project.team.members.include?(user)
-            true
-          else
-            false
-          end
-        end
-      end
-    end
-
-    # Returns an Array of Issues that can be read by the given user.
-    #
-    # issues - The issues to reduce down to those readable by the user.
-    # user - The User for which to check the issues
-    def issues_readable_by_user(issues, user = nil)
-      return issues if user && user.admin?
-
-      issues.select { |issue| issue.visible_to_user?(user) }
-    end
-
     # List of possible abilities for anonymous user
     def anonymous_abilities(user, subject)
       if subject.is_a?(PersonalSnippet)
-- 
GitLab


From 143ae092712531f09fce5ae06c2ab065835fc1c6 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Tue, 9 Aug 2016 15:50:47 -0700
Subject: [PATCH 7/7] make can_edit_note? public as well

---
 app/models/ability.rb | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/app/models/ability.rb b/app/models/ability.rb
index 70e37babd392..228a0641faf1 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -32,6 +32,19 @@ def issues_readable_by_user(issues, user = nil)
       issues.select { |issue| issue.visible_to_user?(user) }
     end
 
+    # TODO: make this private and use the actual abilities stuff for this
+    def can_edit_note?(user, note)
+      return false if !note.editable? || !user.present?
+      return true if note.author == user || user.admin?
+
+      if note.project
+        max_access_level = note.project.team.max_member_access(user.id)
+        max_access_level >= Gitlab::Access::MASTER
+      else
+        false
+      end
+    end
+
     def allowed?(user, action, subject)
       cached_allowed(user, subject).include?(action)
     end
@@ -410,18 +423,6 @@ def can_read_group?(user, group)
       GroupProjectsFinder.new(group).execute(user).any?
     end
 
-    def can_edit_note?(user, note)
-      return false if !note.editable? || !user.present?
-      return true if note.author == user || user.admin?
-
-      if note.project
-        max_access_level = note.project.team.max_member_access(user.id)
-        max_access_level >= Gitlab::Access::MASTER
-      else
-        false
-      end
-    end
-
     def namespace_abilities(user, namespace)
       rules = []
 
-- 
GitLab