Support NullUser pattern as well as AnonymousUser pattern.

[christhekeele]

Allows action_authorized? to handle nil send(Authority.configuration.user_method) users without raising undefined method 'can_#{action}?' for nil:NilClass, but SecurityViolation instead.

This works well for lightweight authentication systems that don't mock out a user when unauthenticated.

I meant for this to be a one-liner fix, but my text editor must have trimmed off a trailing space somewhere. Darn.

[nathanl]

@christhekeele - thanks for working on Authority with me! I'm super pumped to have your contributions.

My only concern with this change is that the existing error ("nil doesn't have a can_update? method, buddy") tells you exactly what's going on if you think you have a user object but you don't. Whereas "access denied" might make you think you've got a bug in your authorizer logic.

My understanding of NullObject pattern is that there would be a specific object that knows how to quack like a user, but represents the absence of one. So instead of passing nil, you'd pass a NullUser, which would have a can_update? method that returns false. In that case the developer isn't surprised because they know they're providing that NullUser (or they know that their authentication layer provides one).

If someone accidentally passes nil because they think I have a current_user or whatever, how can we make that easy to figure out? I think the logged message will show that. Do you think that's clear enough?

[christhekeele]

You're right, I suppose I meant I wanted to provide an option to people who don't want to touch the User interface at all, and just accept nil.

I do like the NullUser implementation you describe, and played around with the idea myself--calling it AnonymousUser in my head.

Odds are that developers who want to use that pattern will do so themselves in their current_user implementation. I wanted to provide an option for people who don't want to bother, but as I think about it I can see how substituting a SecurityViolation for an explicit error by default could confuse developers, as well as being an API change.

So re: nil current_users, I propose a configuration option--called something like forbid_nil_users--that defaults to false and persists the current behavior. If true, the result of action_authorized? can check if the user exists and short circuit the the logic that throws the error.

I find the nil current_user pattern easier to maintain than a no-op user and very common in ruby apps, so I think we'd find the option heavily used.

Re: the AnonymousUser pattern, I'd hesitate to have Authority accept the responsibility of providing such an object by itself. What about an Authority::Anonymous module that meta-programs can_#{action}? methods that return false? Then developers can implement their own patterns, on User model subclasses or singleton User instances, or on things not called User, and mixin Authority support?

[nathanl]

@adamhunter and I have been discussing this, and we thought of another problem. But we also came up with what I think is a nice solution.

The other problem we see is that, even if we handle nils in the controller authorization, developers still have to deal with nil users in the view.

See, in my mind, to have the controller authorization deny a user permission to do something should be a very rare case. That's because any forbidden action should not be visible in the view. If somebody gets stopped at the controller level, either the developer forgot to do link_to update_thing_path(@thing) if current_user.can_update?(@thing), or the user deliberately bypassed that by typing the URL in or doing a POST from cURL or something. (This is why the default 403 page doesn't bother being friendly; it's likely the user has deliberately done something bad.)

So, if the developer is taking care of this in the views, making the controller handle nils doesn't solve the fact that they're still going to have if current_user.can_update? blowing up in their views when current_user returns nil.

They can solve this by having current_user return a NullUser of some kind if they want. But we can also solve both the view and controller problem simultaneously for them.

What we can do is put the following line to the options file, with explanation, commented out by default:

nil.extend(Authority::UserAbilities)

Similarly, if the developer wants to have current_user return a NullUser when nobody's logged in, they can mix Authority::UserAbilities into that, too.

With this change, nil.can_update?(@thing) or @null_user.can_update?(@thing) just gets passed to @thing_authorizer.updatable_by?(nil) or @thing_authorizer.updatable_by?(@null_user), and the logic for what a nil/NullUser can do goes back in the authorizer, where it belongs.

It may seem a bit extreme to modify nil, but I think it's a simple solution, and we'll only do it if the developer uncomments that line in the config file, so they won't be surprised.

Any objections? If not, would you update your pull request accordingly?

[nathanl]

By the way, thanks for being patient with my nitpicking. :)

[stfcodes]

I've implemented Authority in some small test projects and a big project currently running in production. I love the simplicity and the overall logic that it provides.

I've tackled the problem of authorizing some links / controller actions when I don't have a user, and I realized that those should be just handled by authentication, and not authorization in most cases. When authentication doesn't suffice, the developer can just implement the NullUser pattern, so the current_user method would return the actual user OR a NullUser. It's simple, fast, and yields results.

So, polluting NilClass is extreme in my opinion. Also making authority handle nil user cases and return security violations, just obfuscates the fact that a user isn't present. We should just be informed of the fact that there isn't an user to authorize and nothing more.

Something like UserMissing Exception, or whatever name and description seems more informative.

[christhekeele]

I'm not sure there is a link_to update_thing_path(@thing) if current_user.can_update?(@thing) problem: The developer wrote that current_user themselves, they can be expected to handle its outcome responsibly. They know what's going on with it. If they don't want to implement a NullUser, they know that's going to mean a lot more if current_user and current_user.can_update?.

That's what this pull request was initially about, really: the current_user method is developer defined, and could return anything under the sun. Unlike the developer, Authority has no idea what's going on with it. While we can't reasonably be expected to handle every situation, I thought we could support the third most common situation (nil) alongside the most common two (User and NullUser).

Encouraging the developer to mixin authorization logic into every class that their current_user can return is the wrong way to accommodate non-standard return values. If that's a concern, I'd sooner document how to manually override Authority#action_authorized?.

On the other hand, I'd argue that nil qualifies as a standard return value from current_user and ought to be supported.

[christhekeele]

Re: @shuriu

Also making authority handle nil user cases and return security violations, just obfuscates the fact that a user isn't present.

I agree this would be an obfuscation and confusing to a developer picking up Authority for the first time, if it were the default behavior. But if there was a configuration variable that had to get changed first, it would make this behavior explicit enough for me to sleep at night.

It might be confusing to existing users of Authority on update, but disabling the configuration variable by default will maintain the current API.

It might be confusing to an end user, but this is all under the assumption that they weren't logged in and were trying to find their way into other portions of the site regardless. Screw those guys.

I'll update the pull request with a configuration variable implementation to further the discussion.

Re: @nathanl

Nitpicking is good! If you didn't care about your library's code, I wouldn't be using your library, let alone trying to contribute to it. :)

[nathanl]

I'm still thinking about this (and also trying to do my normal work). :) Will try to respond soon.

(Sorry about the merge conflict - I saw you fixed a bunch of whitespace and I thought, "dang, what a mess! I'm just going to fix that everywhere", so I did that on master.)

[nathanl]

@christhekeele - I'd like to talk about this some more. Any chance you'd be able to do a Google Hangout or talk on Skype during business hours? (I'm in Eastern time.)

[christhekeele]

I could do Skype in half an hour. (I'm in Central time, so that's 4:45 to me.) Hit me up @tireurroyale.

[nathanl]

OK! For the sake of anyone else reading this issue, Chris and I chatted and agreed about the best way to proceed. Here's the gist of it.

Authority won't specially handle nil users or give a specific option to do so. We want to limit Authority to authorization and keep authentication totally separate. If there's no user signed in, that's an authentication concern; Authority can't meaningfully answer the question "can this user do X?" if it isn't given a user or something that quacks like one.

Besides the philosophical point, having authentication handle this is a better user experience. If an admin has forgotten to sign in and attempts some admin-only action, it would be confusing to them to say "access denied". It would be much more helpful to say "please sign in".

What developers using Authority can do is:

• Have something like Devise's before_filter :authenticate_user! running prior to any Authority checks on the request (since any action that requires authorization clearly requires authentication).
• If they disagree with this strategy of "always authenticate before authorizing", have their user method return a NullUser object that quacks like a user, then have their authorizers know what to do with those

What Authority can do is improve the error it gives you if you pass nil or anything else that doesn't quack like a user. Chris is going to implement this.

Thanks to @christhekeele for working on this and to @shuriu for helping us think through it.

[nathanl]

@christhekeele - Still planning to make the nicer error? I don't mind doing it if you don't want to or have time.

[christhekeele]

I was planning on doing it this weekend. Don't worry, hadn't forgotten! :)

[abrambailey]

Hi I've just put this

  class ApplicationController < ActionController::Base
    def current_or_null_user
      if current_user == nil
        User.new
      else
        current_user
      end
    end
  end

...

Authority.configure do |config|
   config.user_method = :current_or_null_user
end

[nathanl]

@funkdified - Yep, that's a fine way to handle it in your app and will ensure that Authority gets handed an object it can work with. You could go further and have a specific NullUser class if you wanted to be able to do things like call user.preferences and get default settings back.

You may also find that there are areas of your app that simply shouldn't be accessed by users who aren't logged in. In that case, the authentication layer should stop them and ask for credentials.

[abrambailey]

Thanks for the tip Nathan.. I will try to implement this :)

[nathanl]

@christhekeele - no worries on the timeline. FYI, I'll be traveling this coming week, so if you send a pull request, I'll see it when I return.