From 41d89533e61f6009b5d800afc00c184e2807eafd Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 19 Aug 2016 22:22:52 -0700 Subject: Fix assorted rspec failures due to stale, cached user permissions RequestStore is disabled in tests, but the Ability class was caching user permissions based on the user and project ID of previous test runs. Revise code to use RequestStore only if it is active. --- app/models/ability.rb | 48 +++++++++++++++++++++++++++--------------------- 1 file changed, 27 insertions(+), 21 deletions(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index 07f703f205d..b82632ccc0b 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -166,38 +166,44 @@ class Ability end 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)) + if RequestStore.active? + RequestStore.store[key] ||= uncached_project_abilities(user, project) + else + uncached_project_abilities(user, project) + end + end - owner = user.admin? || - project.owner == user || - (project.group && project.group.has_owner?(user)) + def uncached_project_abilities(user, project) + rules = [] + # Push abilities on the users team role + rules.push(*project_team_rules(project.team, user)) - if owner - rules.push(*project_owner_rules) - end + owner = user.admin? || + project.owner == user || + (project.group && project.group.has_owner?(user)) - 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) -- cgit v1.2.1 From be95e03a5633696d79260ae61bb9e8f4c6755855 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 19 Aug 2016 23:19:42 -0700 Subject: Add spec for Ability.project_abilities --- spec/models/ability_spec.rb | 63 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index 853f6943cef..fe32d367176 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -171,6 +171,69 @@ describe Ability, lib: true do end end + shared_examples_for ".project_abilities" do |enable_request_store| + before do + RequestStore.begin! if enable_request_store + end + + after do + if enable_request_store + RequestStore.end! + RequestStore.clear! + end + end + + describe '.project_abilities' do + let!(:project) { create(:empty_project, :public) } + let!(:user) { create(:user) } + + it 'returns permissions for admin user' do + admin = create(:admin) + + results = described_class.project_abilities(admin, project) + expect(results.count).to eq(90) + end + + it 'returns permissions for an owner' do + results = described_class.project_abilities(project.owner, project) + + expect(results.count).to eq(90) + end + + it 'returns permissions for a guest' do + project.team << [user, :guest] + + results = described_class.project_abilities(user, project) + + expect(results.count).to eq(22) + end + + it 'returns permissions for a developer' do + project.team << [user, :developer] + + results = described_class.project_abilities(user, project) + + expect(results.count).to eq(22) + end + + it 'returns permissions for a master' do + project.team << [user, :master] + + results = described_class.project_abilities(user, project) + + expect(results.count).to eq(22) + end + end + end + + describe '.project_abilities with RequestStore' do + it_behaves_like ".project_abilities", true + end + + describe '.project_abilities without RequestStore' do + it_behaves_like ".project_abilities", false + end + describe '.issues_readable_by_user' do context 'with an admin user' do it 'returns all given issues' do -- cgit v1.2.1 From 1954bb17eca49d375c92a4b8fa7f52fa39873a7d Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sat, 20 Aug 2016 06:33:53 -0700 Subject: Make Ability#project_abilities return unique values and fix counts --- app/models/ability.rb | 2 +- spec/models/ability_spec.rb | 19 ++++++++++--------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index b82632ccc0b..a49dd703926 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -203,7 +203,7 @@ class Ability rules -= project_archived_rules end - rules - project_disabled_features_rules(project) + (rules - project_disabled_features_rules(project)).uniq end def project_team_rules(team, user) diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index fe32d367176..aa3b2bbf471 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -191,21 +191,22 @@ describe Ability, lib: true do admin = create(:admin) results = described_class.project_abilities(admin, project) - expect(results.count).to eq(90) + + expect(results.count).to eq(68) end it 'returns permissions for an owner' do results = described_class.project_abilities(project.owner, project) - expect(results.count).to eq(90) + expect(results.count).to eq(68) end - it 'returns permissions for a guest' do - project.team << [user, :guest] + it 'returns permissions for a master' do + project.team << [user, :master] results = described_class.project_abilities(user, project) - expect(results.count).to eq(22) + expect(results.count).to eq(60) end it 'returns permissions for a developer' do @@ -213,15 +214,15 @@ describe Ability, lib: true do results = described_class.project_abilities(user, project) - expect(results.count).to eq(22) + expect(results.count).to eq(44) end - it 'returns permissions for a master' do - project.team << [user, :master] + it 'returns permissions for a guest' do + project.team << [user, :guest] results = described_class.project_abilities(user, project) - expect(results.count).to eq(22) + expect(results.count).to eq(21) end end end -- cgit v1.2.1