Skip to content
Snippets Groups Projects

Refactor user authorization check for a single project to avoid querying all user projects

Merged username-removed-367626 requested to merge 18586-user-authorized_projects-is-slow into master

What does this MR do?

Refactor user authorization check for a single project to avoid querying all user projects

Are there points in the code the reviewer needs to double check?

Is there any alternative to duplicating the logic of Users#authorized_projects on Project#authorized_for_user?

Why was this MR needed?

Users#authorized_projects is slow, and we are calling it unnecessarily when we are already scoped to a single project.

What are the relevant issue numbers?

#18586 (closed) (but it doesn't close it, it doesn't actually changes Users#authorized_projects, so it's still slow, but it does reduce it's uses)

Does this MR meet the acceptance criteria?

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Reassigned to @eReGeBe

  • @yorickpeterse I changed to class << self mainly to add a private class method, but I can use the block only for that method if you'd like. To further explain the changes, in some places we do project.issues.visible_to(user); Issue.visible_to_user in turn calls User#authorized_projects, which is the big slow union query. We don't need to run that query if we know that we're processing issues of only one project, we just need to know if the user is authorized to see that project. I detect if the current Issues relation belongs to a project using proxy_association, and if that's the case check only that project against that user and skip the User#authorized_projects call.

  • Added 1 commit:

    • 02f963a5 - Refactor user authorization check for a single project to avoid querying all user projects
  • Added 746 commits:

    • 02f963a5...4b6e6d0a - 745 commits from branch master
    • c1f3864b - Refactor user authorization check for a single project to avoid querying all user projects
  • 52 52 attributes
    53 53 end
    54 54
    55 class << self
    56 private
    57
    58 def owner_project
  • 52 52 attributes
    53 53 end
    54 54
    55 class << self
    56 private
    57
    58 def owner_project
    59 # No owner if we're not being called from an association
    60 return nil unless all.respond_to?(:proxy_association)
  • 57
    58 def owner_project
    59 # No owner if we're not being called from an association
    60 return nil unless all.respond_to?(:proxy_association)
    61
    62 owner = all.proxy_association.owner
    63
    64 # Check if the association is or belongs to a project
    65 if owner.is_a?(Project)
    66 owner
    67 else
    68 begin
    69 owner.association(:project).target
    70 rescue ActiveRecord::AssociationNotFoundError
    71 nil
    72 end
  • 69 owner.association(:project).target
    70 rescue ActiveRecord::AssociationNotFoundError
    71 nil
    72 end
    73 end
    74 end
    75 end
    76
    55 77 def self.visible_to_user(user)
    56 78 return where('issues.confidential IS NULL OR issues.confidential IS FALSE') if user.blank?
    57 79 return all if user.admin?
    58 80
    81 # Check if we are scoped to a specific project's issues
    82 if owner_project
    83 if owner_project.authorized_for_user?(user, Gitlab::Access::REPORTER)
    84 # If the project is authorized for the user, he can see all issues in the project
  • 72 end
    73 end
    74 end
    75 end
    76
    55 77 def self.visible_to_user(user)
    56 78 return where('issues.confidential IS NULL OR issues.confidential IS FALSE') if user.blank?
    57 79 return all if user.admin?
    58 80
    81 # Check if we are scoped to a specific project's issues
    82 if owner_project
    83 if owner_project.authorized_for_user?(user, Gitlab::Access::REPORTER)
    84 # If the project is authorized for the user, he can see all issues in the project
    85 return all
    86 else
    87 # else only non confidential and authored/assigned to him
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading