redrabbit / git.limo

A Git source code management tool powered by Elixir with easy installation & high extensibility.
https://git.limo
MIT License
497 stars 42 forks source link

Fix N+1 query when checking authorisation policies for issues and comments #75

Closed redrabbit closed 4 years ago

redrabbit commented 4 years ago

In commit 9e30f7e, we introduced more complex rules for both GitGud.Comment and GitGud.Issue when passing the :admin action. The new rule retrieves the repository maintainers and returns true if the user has at least write permission.

What we basically want to achieve here is redirecting the policy to a different resource:

 # Maintainers with at least write permission can admin the resource.
 def can?(%{repo: %Repo{} = repo}, user, :admin), do: authorized?(user, repo, :write)
 def can?(%{repo_id: repo_id}, %User{} = user, :admin) do
   if maintainer = Repo.maintainer(repo_id, user),
    do: maintainer.permission in ["write", "admin"],
   else: false
end

Now let's see why this implementation is problematic... Say we have a comment thread (issue, commit review) and we want to check for :admin permission for each comment. Calling authorized?/3 for each comment would result in a N+1 query.