diff options
author | Douwe Maan <douwe@gitlab.com> | 2018-04-05 18:21:24 +0300 |
---|---|---|
committer | Douwe Maan <douwe@gitlab.com> | 2018-04-05 18:21:24 +0300 |
commit | 2693edc4fe32fc51e966fe939a890edaa0042671 (patch) | |
tree | aeadf9b8ae46431f6715c18ba304dab6ebb8d01f /app | |
parent | 082bf1c6ee97aefc9c53e7462cf870ce8ce0ad2b (diff) | |
parent | e7b1d201dd56611eff7e796e9a390a7b21df51d1 (diff) |
Merge branch '43098-controller-projects-issuescontroller-show-executes-more-than-100-sql-queries' into 'master'
Resolve "Controller Projects::IssuesController#show executes more than 100 SQL queries"
Closes #43098
See merge request gitlab-org/gitlab-ce!17986
Diffstat (limited to 'app')
-rw-r--r-- | app/policies/issuable_policy.rb | 14 | ||||
-rw-r--r-- | app/policies/issue_policy.rb | 2 | ||||
-rw-r--r-- | app/policies/merge_request_policy.rb | 1 | ||||
-rw-r--r-- | app/policies/project_policy.rb | 35 |
4 files changed, 34 insertions, 18 deletions
diff --git a/app/policies/issuable_policy.rb b/app/policies/issuable_policy.rb index 3f6d7d04667..e86d1c8f98e 100644 --- a/app/policies/issuable_policy.rb +++ b/app/policies/issuable_policy.rb @@ -2,20 +2,6 @@ class IssuablePolicy < BasePolicy delegate { @subject.project } condition(:locked, scope: :subject, score: 0) { @subject.discussion_locked? } - - # We aren't checking `:read_issue` or `:read_merge_request` in this case - # because it could be possible for a user to see an issuable-iid - # (`:read_issue_iid` or `:read_merge_request_iid`) but then wouldn't be allowed - # to read the actual issue after a more expensive `:read_issue` check. - # - # `:read_issue` & `:read_issue_iid` could diverge in gitlab-ee. - condition(:visible_to_user, score: 4) do - Project.where(id: @subject.project) - .public_or_visible_to_user(@user) - .with_feature_available_for_user(@subject, @user) - .any? - end - condition(:is_project_member) { @user && @subject.project && @subject.project.team.member?(@user) } desc "User is the assignee or author" diff --git a/app/policies/issue_policy.rb b/app/policies/issue_policy.rb index ed499511999..263c6e3039c 100644 --- a/app/policies/issue_policy.rb +++ b/app/policies/issue_policy.rb @@ -17,6 +17,4 @@ class IssuePolicy < IssuablePolicy prevent :update_issue prevent :admin_issue end - - rule { can?(:read_issue) | visible_to_user }.enable :read_issue_iid end diff --git a/app/policies/merge_request_policy.rb b/app/policies/merge_request_policy.rb index e003376d219..c3fe857f8a2 100644 --- a/app/policies/merge_request_policy.rb +++ b/app/policies/merge_request_policy.rb @@ -1,3 +1,2 @@ class MergeRequestPolicy < IssuablePolicy - rule { can?(:read_merge_request) | visible_to_user }.enable :read_merge_request_iid end diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 57ab0c23dcd..b1ed034cd00 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -66,6 +66,22 @@ class ProjectPolicy < BasePolicy project.merge_requests_allowing_push_to_user(user).any? end + # We aren't checking `:read_issue` or `:read_merge_request` in this case + # because it could be possible for a user to see an issuable-iid + # (`:read_issue_iid` or `:read_merge_request_iid`) but then wouldn't be + # allowed to read the actual issue after a more expensive `:read_issue` + # check. These checks are intended to be used alongside + # `:read_project_for_iids`. + # + # `:read_issue` & `:read_issue_iid` could diverge in gitlab-ee. + condition(:issues_visible_to_user, score: 4) do + @subject.feature_available?(:issues, @user) + end + + condition(:merge_requests_visible_to_user, score: 4) do + @subject.feature_available?(:merge_requests, @user) + end + features = %w[ merge_requests issues @@ -81,6 +97,10 @@ class ProjectPolicy < BasePolicy condition(:"#{f}_disabled", score: 32) { !feature_available?(f.to_sym) } end + # `:read_project` may be prevented in EE, but `:read_project_for_iids` should + # not. + rule { guest | admin }.enable :read_project_for_iids + rule { guest }.enable :guest_access rule { reporter }.enable :reporter_access rule { developer }.enable :developer_access @@ -150,6 +170,7 @@ class ProjectPolicy < BasePolicy # where we enable or prevent it based on other coditions. rule { (~anonymous & public_project) | internal_access }.policy do enable :public_user_access + enable :read_project_for_iids end rule { can?(:public_user_access) }.policy do @@ -255,7 +276,11 @@ class ProjectPolicy < BasePolicy end rule { anonymous & ~public_project }.prevent_all - rule { public_project }.enable(:public_access) + + rule { public_project }.policy do + enable :public_access + enable :read_project_for_iids + end rule { can?(:public_access) }.policy do enable :read_project @@ -305,6 +330,14 @@ class ProjectPolicy < BasePolicy enable :update_pipeline end + rule do + (can?(:read_project_for_iids) & issues_visible_to_user) | can?(:read_issue) + end.enable :read_issue_iid + + rule do + (can?(:read_project_for_iids) & merge_requests_visible_to_user) | can?(:read_merge_request) + end.enable :read_merge_request_iid + private def team_member? |