Adds 'Abilities' for Admin, Artist and Voter and uses them in controllers #74
Conversation
Katee
changed the title
|
I accidentally opened this PR too early, treat it as a work in progress. |
Katee
changed the title
|
This is ready for review. It is an implementation of the 'shim' you mentioned in #73 (comment) and does not disrupt any existing functionality. Previous checks like this: if !admin_logged_in?
redirect_to '/'
endwill be updated with logic that checks the specific ACL for a model in controllers and views: unless can? :manage, GrantsVoter.new(voter: @voter)
redirect_to '/'
endCanCan can automatically load the resources a user has access to in a controller (see Controller Authorization Example). However I cannot currently use this feature yet as the existing controllers break convention. |
| @@ -64,44 +65,46 @@ def create | |||
| end | |||
|
|
|||
| def update | |||
| if !admin_logged_in? | |||
| redirect_to "/" | |||
| # could allow for (timing based) Voter enumeration | |||
There was a problem hiding this comment.
I don't quite understand what timing-based voter enumeration means in this context
| # TODO: using `voter_id`, `artist_id` which will become `user_id` in future | ||
| # after models are unified | ||
|
|
||
| # TODO: why do admins not require activation? |
There was a problem hiding this comment.
Admins currently can only be created by another admin using the Portal, so they don't need activation. But that's a side effect of the implementation, not a design choice. In the future I still wouldn't want randos to be able to try to sign up as an admin, but their initial signup should go through the same flow as everyone else.
| # could allow for (timing based) Voter enumeration | ||
| @voter = Voter.find(params[:id]) | ||
|
|
||
| unless can? :manage, GrantsVoter.new(voter: @voter) |
There was a problem hiding this comment.
ok cool, I like where this is going. Stretch Goal: Is there any way we can make a shim class that's like (pardon the Go code, that's what my head is in these days):
type User struct {
Admin *Admin
Voter *GrantsVoter
Artist *Artist
}
And then the idea is that one or more of the members could be filled in, since right now more than one type of user could be logged in. That would mean that the various can-checks would have to retry all the checks on each non-nil member, and return true if any of them satisfy the check.
Having that structure would allow something along the lines of:
@voter = Voter.find(params[:id])
@user = UserFromVoter(@voter) # Creates a new User object, only sets the Voter member
unless can? :manage, @user
We would have a bunch of helper functions to create User structs in various situations.
With that, there's a clear search-and-replace path to the eventual final form when we swap out the back end:
@user = User.find(params[:id])
unless can? :manage, @user
This would be my most idealist situation, but if that would cause headaches then we don't have to go that way. What you've got here is good enough.
| @@ -1,3 +1,13 @@ | |||
| def sign_in(user) | |||
| if user.is_a?(Admin) | |||
There was a problem hiding this comment.
this is where it gets tricky, are signins cumulative? e.g, if I sign in as admin, and then voter, does the user object gain the properties of both? (is that the user ||= above?)
| include CanCan::Ability | ||
|
|
||
| def initialize(user) | ||
| user ||= User.new # guest user (not logged in) |
This PR does not touch views, although they do need to be updated to use the same logic.
I also purposely did not edit many controllers in this PR as most of them are severely breaking convention and are probably better handled outside of this PR. I did update
VotersControlleras an example.