-
Notifications
You must be signed in to change notification settings - Fork 111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Authorization framework with Action Policy #466
Conversation
…xcpet devise related controllers
…ests back to green
…to help define cases where user does not exist
…tionPolicy pre_check verify_organization!
@q = Pet.org_pets_with_apps(current_user.staff_account.organization_id).ransack(params[:q]) | ||
authorize! AdopterApplication, | ||
context: {organization: current_user.organization}, | ||
with: Organizations::AdopterApplicationReviewPolicy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the with
here? It looks like it would be handled correctly -> https://github.com/palkan/action_policy/blob/master/docs/namespaces.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would need with
here, at least with how I had the policy written. I thought we could use namespace here as well. However, the lookup pattern is looking at the model of the object passed which does not match the controllers name, so if we do:
authorize! AdopterApplication,
context: {organization: current_user.organization},
namespace: Organizations
It would look for the policy Organizations::AdopterApplicationPolicy
, not Organizations::AdoptionApplicationReviewPolicy
.
However, you got me thinking, we should just rename the policy to match the model in the first place, and then we can also use the namespace lookup. I've implemented that in 33e8ab8. This is also better since the policy actually didn't match the controller anyway because the controller name uses "adoption" not "adopter".
And actually, I realized with the renaming, we don't even need to specify the namespace because Action Policy looks up the policy based off the namespace of the controller. Super cool 😎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BUT, this brings up a different point for the authorized_scope usage here. I was forcing the scope to use the Organizations::AdopterApplicationReviewPolicy
for getting the relation_scope
here before. If I remove that with
, it actually is going to go for the Organizations::PetPolicy
to check the relation_scope
there. In this particular case, both policies are using the same defaults from ApplicationPolicy
, so it does not make an immediate difference.
It makes me almost wonder if @q
is grabbing the wrong relation here. This is an index
for AdoptionApplicationReviews
but it's not indexing applications, it's indexing pets (with the apps merged with pets). But AdopterApplications already have a pet reference so couldn't we just use ransack on the adopter applications themselves?
I've made a commit to remove the with
from the scope and change the controller test to check that the controller scope is grabbing from Organizations::PetPolicy
. c47110f
Not sure how I feel about it. I'll sleep on it and would love to here your thoughts @nsiwnf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😵💫 Yeah, i see how this page indexing is weird - and yeah, seems like we should be able to group by pet through adoption applications - that might be more within the integrity of this controller., but honestly, I think it's fine as it is if the end result is what we expect! Also, I totally missed the "reviews" part of the policy 🤦. The rename to AdopterApplicationPolicy
makes more sense to me tho!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think I agree with you to leave the ransack search as is. I looked at a bit, and I can see why it was done this way. The page is using a partial from pets to show each pet's associated applications together. Changing it to index the applications, we would ultimately end up mapping the applications into pets anyway to keep using the shared partial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm 🎉 - sorry for all the one off comments btw
…::AdopterApplicationPolicy
…cy for scoping instead of Organizations::AdopterApplicationPolicy
No problem at all! I appreciate your thoroughness a ton! I felt a little guilty making a big PR knowing y'all would have to review it. |
if allowed_to?( | ||
:index?, Pet, namespace: Organizations, | ||
context: {organization: Current.organization} | ||
) | ||
pets_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can now redirect staff to the dashboard_index_path, now we have a solid grasp on what the dashboard looks like. I think we set it to pets_path before the dashboard index was a thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the redirect to dashboard now in ada4ef4
end | ||
else | ||
redirect_to staff_index_path, alert: "You can't deactivate yourself." | ||
@staff_account.deactivate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's stopping a staff user deactivating themself here, now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah nevermind I see it in the staff policy!
@@ -0,0 +1,44 @@ | |||
class AdopterApplicationPolicy < ApplicationPolicy | |||
authorize :pet, optional: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to change the naming of adopter_profile to adopter_foster_profile here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I will update this to just verify_profile!
instead to decouple the method from the account type naming conventions. Any objections to that?
@@ -0,0 +1,16 @@ | |||
class Organizations::AdopterApplicationPolicy < ApplicationPolicy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we name this policy AdopterApplicationReviewPolicy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AdopterApplicationPolicy
makes more sense since it's still related to how applications are managed - #466 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I was initially a little confused as we have a controller AdopterApplicationsController that enables an adopter to make an application then we have the AdopterApplicationReviews controller for reviewing said applications on the staff side. This current naming is not aligned in that sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I see there was discussion on this already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phew what a PR! Great work @mononoken this is a huge upgrade. I have left a couple of small comments. Otherwise, I think it is good to go. There might be one or two things we missed in the review here given the PR's size, but we can fix those as we find them (if any).
layout "dashboard" | ||
|
||
def index | ||
@q = Pet.org_pets_with_apps(current_user.staff_account.organization_id).ransack(params[:q]) | ||
authorize! AdopterApplication, | ||
context: {organization: current_user.organization} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed this - should we have a before_action authenticate_user
or do this in case someone tries to access this page without logging in?
context: {organization: Current.organization}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right! That should have been Current.organization and was def a mistake.
🔗 Issue
This is the beginning of #450 , but it does not close it yet. My goal is to apply the framework to every controller action by the end of it. I will continue to work on the rest of the controllers in the meantime.
✍️ Description
This PR is the beginning of the authorization framework with Action Policy. I have not applied the framework to the whole app, but I have applied it to 5 controllers which require both shared and unique configuration.
I wanted to show this PR so that reviewers can get eyes on the progress and confirm it's going in the right direction. If any patterns seem like they could be improved or should be abandoned, please let me know.
This PR will most likely be an intro to Action Policy to anyone who reviews this. I think you will find most of Action Policy intuitive and their docs are awesome. I have a quick summary of some of its features below if it will help you but please feel free to skip it unless you are confused.
Summary of Action Policy in pet-rescue
Anything I do not cover can most likely be found in the Action Policy docs.
Take a quick look at the current
Organizations::PetsController
on main. Now check out the new one in the PR. Hopefully, you see the improvement!All of the authorization logic has been removed from the controller and instead exists in the
Organizations::PetPolicy
. This Policy is used viaauthorize!
calls.For actions involving existing records such as
update
,destroy
, andshow
. We can call authorize on the record that is fetched in theset_
method. This will pass the record as a default context to the policy:Action Policy finds the correct policy implicitly using the name of the model object passed to
authorize!
. If the controller is namespaced, it will look for the policy in the same namespace too. The lookup behavior is further explained in the docs.However, some actions don't use an existing record, such as
new
. In this case, you can pass the class name to help Action Policy find the correct policy. In cases where there is no record, you may require more context than the defaults. The defaults are the current user and the record passed. Innew
here, I pass theCurrent.organization
as an organization context so that in the Policy, we can make sure the current user's organization matches the current one.Action Policy also support scoping. This is very useful for
index
actions. In thisindex
, I call the scope usingauthorized_scope
which returnsrelation.where(organization: user.organization)
(relation being the@q.result
ActiveRecord::Relation
it was passed).In
Organizations::PetPolicy
, you will see that you can alias rules so that rules that share common logic don't have to be rewritten.If you are not familiar with Pundit, Action Policy looks up the rule for the action by looking in the policy for a rule that matches the method name plus
?
. Action Policy also can have a default rule specified and out-of-the-box comes withmanage?
as a default rule, except towardindex?
,new?
, andcreate?
. So, inOrganizations::PetPolicy
, we just need to define whatmanage?
checks which in this case is a call topermission?
with:manage_pets
.permission?
is not a part of Action Policy. I will explain that below.For logic that is shared between many rules, you can define pre-checks. In this case, I have a pre-check for both
verify_organization!
andverify_active_staff!
. These are defined inApplicationPolicy
since they are shared.For
verify_organization!
, I made some adjustments so that we can usemanage?
for both actions that have an existing record and those that don't. Recall thatupdate
usesauthorize! @pet
butnew
does not have an existing@pet
to pass, so it is given the organization context directly. I have:The organization getter will get the
@organization
context if it exists. If it does not, it assumes a record was passed and gets the organization from that. That way,organization
is available forverify_organization!
with the same logic for both cases.I have tested authorization using the recommendations from Action Policy. palkan recommends that you unit test each Policy and then you simply need to test that the controller actions are using the proper authorization. No need to make multiple request tests for different user contexts. I really like this method and it works cleanly and is fast.
One note on tests though, is that I currently create the context objects in the policies unit tests. I want to refactor these into using
build_stubbed
instead but the one thing that I think may presently hinder that isrolify
. However, I think fixing that will be easy.permission?
and authorizable.rbIn our policies, we could have rules like this instead:
We could DRY it for other tests like this:
What I do not like about that is that it only becomes clear what each role is permitted to do in the policy. These permissions can exist either in our heads or in the code. The
Authorizable
concern is an attempt to get this in the code. It also has central definitions for each role, so you can look in one space to find what is permitted for each role.I am not completely happy with
Authorizable
as I have it written. For one, it is not open/closed. To make it so, I think we could easily create apermissions
table and model, that can be associated with users. It would be useful for customizing individual permissions, if that is something clients may want. We could use roles simply for setting default permissions to users in this scenario.