Authorizing a given action

[nathanl]

Authorizing a given action

[nathanl]

So I can picture this better, can you give a little bit more example code of how you'd do this without and and with the new method?

[mantas]

Sure. It would look smth like this. Let's say there's a controller that lets user to administrate his photos. Currently, you'd need Authorizer for both user and user photos. Then use smth like this in each UserAdminPhotos controller action:

authorize_action_for(@photo)

In Photo authorizer, it would check for photo permission by making sure only user himself touches it when administrating pictures.

I think may be better in some cases is smth like this in UserAdminPhotos controller:

authorize_for(:update, @photo.user)

This leverages User authorizer, no need for Photo authorizer. If visitor can edit owner of the picture (e.g. he's the owner himself or admin of the system), he can edit his pictures too.

I know this is nitpicking, but I think it would make Authority much more flexible.

[nathanl]

@mantas - Sorry I'm so slow to respond.

I see what you're saying, but I don't think this is the best direction to go. If we add that controller method, you can avoid the need for a UserAuthorizer in the controller, but you would still want to be able to say in your views: "show this link if current_user.can_edit?(@photo), and to answer that, you'd need a PhotoAuthorizer.

Also, if the logic for photos is really that simple, you don't need a big class.

class PhotoAuthorizer < Authority::Authorizer
  def crudable_by?(user)
    user.can_edit?(resource.owner)
  end
  # Etc
  [:creatable_by?, :deletable_by?].each {|m| alias_method m, :crudable_by? }
end

Does this make sense?

[nathanl]

Thought a bit more. I suppose you may say that you decide whether to show a link to edit a photo by asking current_user.can_edit?(@photo.owner), or something like that. So, again, you avoid needing a PhotoAuthorizer.

But here's the thing: you have, in your head, the knowledge that anyone who can edit a user can edit his/her phots. Where should you encode that knowledge in your application?

If you do current_user.can_edit?(@photo.owner) in your views and authorize_for(:update, @photo.user) in your controller, you're spreading that knowledge out all over the place. It's not self-explanatory for the next programmer who sees it, and if the rules change, you have a lot of code to update.

On the other hand, if you use current_user.can_edit?(@photo) and authorize_action_for(@photo), you can encode the knowledge you have in exactly one place: the PhotoAuthorizer. Its whole reason for existence is to be the canonical place where you store your knowledge about who can do what with photos.

Yes, you have to create another class, but you can make the actual code for it minimal, and I think you get the benefit of better separation of concerns.

This is my opinion, of course, but it's really sort of the philosophy behind Authority.

What do you think?

[focused]

@mantas - I agree with @nathanl, it's more convenient method to store authorization logic in a separate authorizer class for photos.

[mantas]

Hi,

I see what you mean.

Is crudable_by an existing method? I don't see any mentions of it neither in documention, nor in code itself. That way might sort of work, because one of the biggest pains using authorize_action_for was authorizing non-standard-rest methods.

I stand behind my solution though. I don't put authorize_for all around the code too though. I've a before_action that I run in all needed controllers. Same before_action takes care of both photos and albums access management as well.

Anyhow, I guess the best way is for me to pack it as an additional gem.

[focused]

Please look at the section "Defining Your Abilities" in README. You can define crudable in the initializer. But I prefer word "manageable".

[mantas]

I'm aware of defining abilities in initializer. But it seem to require to add each custom method to it. It would be nice if it would be possible to add catch-all method.

[nathanl]

Is crudable_by an existing method? I don't see any mentions of it neither in documention, nor in code itself.

No, sorry, I didn't mean for that to be confusing. It's just a random method I made up in that one authorizer so that I could alias all the normal methods to do the same thing. I could have called it is_made_of_cheese?. 😄 The point was just to show that, if all the permissions work the same way, you don't need much code to declare that. And then if something changes and, for example, createable_by? needs to be different, it's a tiny code change in the authorizer to make it so.

I don't put authorize_for all around the code too though.

What are you doing in your views so that you don't show a link to edit a photo unless the user is authorized to do so? Without a PhotoAuthorizer, aren't you forced to do something like current_user.can_edit?(@photo.owner)?

[mantas]

Ah. .. Adding each custom method there seems a wee overkill to me.

I'm using authorize_for in controller to satisfy ensure_authorization_performed. For views, current_user.can_edit?(@photo.owner) is good enough.

[nathanl]

Right, but can_edit?() what? What do you pass to that method?

[mantas]

Updated previous comment for clarity

[nathanl]

Gotcha. In that case, all of your views have encoded in them the knowledge that, in order to edit a photo, you must be authorized to edit its owner. As opposed to that knowledge being encapsulated in a single authorizer.

That's what I was trying to say.

[mantas]

I'm not using it much in views. If I were, I'd sure use a helper to dry up the code. Since I use it for mostly controller/action authorisation, this is not an issue for me. Catching every custom action would cause more code smell for me.

Anyhow, I got the answer I asked for.

[nathanl]

OK. 😄 If you want this feature enough to make a fork or monkey patch, I think it would be pretty simple; you could just break up authorize_action_for into two methods, so that the call to enforce is done in a method that takes the action as a parameter, and then you could call that directly when you want to.

authority/lib/authority/controller.rb

Line 99 in fa510fb

def authorize_action_for(authority_resource, *options)

Anyway, thanks for using Authority!

[mantas]

Thank you for the help. I've already made this patch and am using it for the past couple weeks. So far so good :)

Keep up the good work on Authority. Since CanCan seems to be abandoned, Authority is the only good authorisation library for Rails :)

[nathanl]

@mantas - you know what? After saying how simple this would be to do and seeing that it's really just a matter of breaking apart a method, I've changed my mind. It's not a bad refactor, it doesn't require any additional tests, and it will help you not have to maintain a fork.

Is this all you've done in your patch?

    def authorize_action_for(authority_resource, *options)
      # `action_name` comes from ActionController
      authority_action = self.class.authority_action_map[action_name.to_sym]
      if authority_action.nil?
        raise MissingAction.new("No authority action defined for #{action_name}")
      end
      authorize_for(authority_action, authority_resource, *options)
    end

    def authorize_for(authority_action, authority_resource, *options)
      Authority.enforce(authority_action, authority_resource, authority_user, *options)
      self.authorization_performed = true
    end

If so, submit a pull request and I'll merge it. I will probably leave the second method as an undocumented feature since I think most people are better off using the normal way, but if anyone lands on the PR, they will see that it's available.

[nathanl]

Keep up the good work on Authority.

Thanks for the kind words. I do want to keep Authority as a great option, but I'm doing non-Rails Ruby work lately, so it's a bit harder for me to stay on top of it. If you like the gem and have time, maybe consider helping keep it updated.

Of course, you have to deal with my "benevolent dictator" attitude if you work on it. 😆

[mantas]

If I'll bump into any bugs, I'll sure submit a patch ;)

I appreciate people who take "benelovent dictator" status seriously. That's why I asked for your take instead of blindly submitting a pull request ;)