Add pundit to owners controller #4744
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4744 +/- ##
==========================================
+ Coverage 97.00% 97.06% +0.05%
==========================================
Files 397 399 +2
Lines 8428 8452 +24
==========================================
+ Hits 8176 8204 +28
+ Misses 252 248 -4 ☔ View full report in Codecov by Sentry. |
martinemde
force-pushed
the
martinemde/owner-controller-policy
branch
3 times, most recently
from
4561812
to
f921c72
Compare
| @@ -3,7 +3,7 @@ | |||
| rubygem | |||
| user | |||
| confirmed_at { Time.current } | |||
| authorizer { user } | |||
| authorizer { association :user } | |||
There was a problem hiding this comment.
This makes it so the user and authorizer are different.
app/policies/ownership_policy.rb
Outdated
| class OwnershipPolicy < ApplicationPolicy | ||
| class Scope < Scope | ||
| def resolve | ||
| scope.confirmed.or(scope.where(rubygem: user.rubygems)).or(scope.where(user: user)) |
There was a problem hiding this comment.
This could easily be potentially impactful on performance if someone owns a lot of gems. I'm wondering what benefit applying a scope here really gives us?
There was a problem hiding this comment.
@martinemde can you post what query this resolves to? I think it should collapse to a single left join on an index, so that should be OK
There was a problem hiding this comment.
For the curious, even though I removed it:
SELECT "ownerships".* FROM "ownerships"
WHERE ("ownerships"."confirmed_at" IS NOT NULL OR "ownerships"."rubygem_id" IN (
SELECT "rubygems"."id"
FROM "rubygems"
INNER JOIN "ownerships" ON "rubygems"."id" = "ownerships"."rubygem_id"
WHERE "ownerships"."user_id" = $1 AND "ownerships"."confirmed_at" IS NOT NULL
) OR "ownerships"."user_id" = $2)If we use scopes, we'll probably need to be more careful. That could be optimized at the very least to use rubygem_id from the ownerships table instead of loading the association.
app/policies/ownership_policy.rb
Outdated
| class OwnershipPolicy < ApplicationPolicy | ||
| class Scope < Scope | ||
| def resolve | ||
| scope.confirmed.or(scope.where(rubygem: user.rubygems)).or(scope.where(user: user)) |
There was a problem hiding this comment.
@martinemde can you post what query this resolves to? I think it should collapse to a single left join on an index, so that should be OK
app/policies/rubygem_policy.rb
Outdated
| class RubygemPolicy < ApplicationPolicy | ||
| class Scope < Scope | ||
| def resolve | ||
| scope.with_versions |
There was a problem hiding this comment.
See e.g. https://rubygems.org/gems/hpriocot. We do allow showing gems without versions, so I am not sure this is correct.
|
@segiddins @colby-swandale I removed the scope tests/features because scopes are unused right now. Better to just write it correctly when we do need it. |
martinemde
force-pushed
the
martinemde/owner-controller-policy
branch
2 times, most recently
from
528f081
to
d276a3e
Compare
martinemde
force-pushed
the
martinemde/owner-controller-policy
branch
from
d276a3e
to
a37264b
Compare
|
Merging since I addressed all the comments. We can follow up if there's something else that needs a look. |
Slightly changes behavior: You are now expected to verify before you will be forbidden. I think this makes sense because you shouldn't be able to find out what an account has access to without having passed authentication.
This is an example of pundit-ification of our controllers. I'd like to hear feedback on how this looks so we can decide on the style.