Refactor user authorization check for a single project to avoid querying all user projects
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?
-
CHANGELOG entry added -
Documentation created/updated -
API support added - Tests
-
Added for this feature/bug -
All builds are passing
-
-
Conform by the style guides -
Branch has no merge conflicts with master
(if you do - rebase it please) -
Squashed related commits together
Merge request reports
Activity
Marked the task CHANGELOG entry added as completed
Marked the task Documentation created/updated as completed
Marked the task Conform by the style guides as completed
Marked the task Squashed related commits together as completed
@eReGeBe Could you change the diff so it doesn't re-indent the various existing methods into a
class << self
block? I know we use that pattern in other places but using that here instead of the existingdef self.foo
setup adds unnecessary noise. As an aside, I'm not really sure what the changes actually do. Could you maybe explain a bit about how things work here?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 doproject.issues.visible_to(user)
;Issue.visible_to_user
in turn callsUser#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 usingproxy_association
, and if that's the case check only that project against that user and skip theUser#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
-
02f963a5...4b6e6d0a - 745 commits from branch
Reassigned to @yorickpeterse
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 I think not. The source for
association
can be seen at http://apidock.com/rails/ActiveRecord/Associations/association and it callsassociation_instance_get
, which is private.Ah OK, then we'll go with how things are now.
Edited by yorickpeterse-staging
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