Feature request: special handling for nil current_user?

[jrochkind]

This relates to #32, but since it's been a couple years I thought it was worth opening a new issue.

I would LOVE it if access-granted could have a special "role" that is checked for nil current_user.

With the advice in #32, you can create a special NullUser, which is not so so bad, but you do have to keep it in sync with any possible methods on your real user you may reference in a role condition lambda.

I think there could be a pretty simple implementation in access-control that would save you from boilerplate code requiring ongoing maintenance -- for what I'd think would be a fairly common use case, I'm kind of surprised the authors and current users of access-granted haven't needed it.

If current_user is nil, assume no roles match, for present roles, short circuit it and don't try on nil.

But add some way to declare a "role" that looks just like current role definitions, but applies always and only when current_user is nil. It might be easiest to simply add new syntax:

nil_user_role do
  can :read, Post do |post|
      post.public?
  end
end

[pokonski]

You can achieve it by even now by simply checking for nil? inside role conditions, like this:

role :logged_in, proc {|user| !user.nil?} do
   # permissions only for logged in users
end

role :guest, proc { |user| user.nil? } do
   # and here, just for guests
end

This is something I also did in other projects using a-g, does it seem enough?

[jrochkind]

But if you have:

role manager, proc { |user| user.manager? } # ....

role :not_logged_in, proc { |user| user.nil? } #....

Is that going to cause a raise every time, as it tries to check nil.manager?. You could put the nil check in every one of your qualifying procs.... that is not nice.

For any app that has not-logged-in-users, you gotta either put guard clauses in every role check you do (and probably can't use the hash-style role-check), or use the 'null user' as above with it's problems.

We could imagine adding a !user.nil? && check inside access-granted code before running every role-qualifying proc... but then that would actually break the role :guest, proc { |user| user.nil? } , so would be backwards incompat, oops.

It might require a configuration flag or a backwards-compat next-major-release fix, but I really would like a better API for handling the fact that there may be a nil current_user.

I wonder if the authors/chaps doesn't really do apps without logged in users or something?

If there's any interest/agreement that this is a problem, I could try to find time to work on a PR (either backwards incompat, or figure out a way to make it opt-in not backwards incompat if preferred, please advise).

[jrochkind]

(PS in general I love this gem, it's just right, with all the lessons learned from years of different attempts at the sweet spot for authorization API, it's really close to it).

[jrochkind]

Hmm, we could provide an additional module-mixin that you add on top of include AccessGranted::Policy, say AccessGranted::Policy::FriendlyNilHandling. To be backwards incompat and allow opt-in to better API.

In fact, one could even provide this just separately in ones own app or third-party gem.

But the implementation gets a little bit tricky and tightly-coupled to AccessGranted::Policy. Is one reason it's probably best in a-g itself, and documented for (what I think?) is a very common use case. If I'm correct this is a very common use case that would serve users well for a-g to handle out of the box.

[pokonski]

For any app that has not-logged-in-users, you gotta either put guard clauses in every role check you do (and probably can't use the hash-style role-check), or use the 'null user' as above with it's problems.

I don't see the problem in your example. You add the !user.nil? to the roles you want to make sure they are used for only the signed in users. Unless you have a dozens of them, I don't see it as a big ugly thing requiring an API change.

But if you want to keep different set of roles for a guest and a different set for signed in users I suggest creating separate policies, this will result in a cleaner solution than adding !user.nil? to every role condition.

So in Rails you could override the current_role method like this:

class ApplicationController

  def current_policy
     if current_user.present?
        UserPolicy.new(current_user)
     else
        GuestPolicy.new(nil)
     end
  end

Then you won't need any guard clauses inside the policies :)

I am not convinced about adding more DSL and breaking compatibility considering what you want to achieve is already doable relatively cleanly.

(PS in general I love this gem, it's just right, with all the lessons learned from years of different attempts at the sweet spot for authorization API).

Glad you like it! Indeed it's made from all the mistakes I've seen in other libraries 😄

[jrochkind]

I don't see the problem in your example. You add the !user.nil? to the roles you want to make sure are signed in.

That is, you have to add it to every role that isn't defined as { |u| u.nil? } basically -- whether or not you actually have a { |u| u.nil? } role -- in any app that allows access to pages where auth is going to be checked for non-logged-in-users. No? I think that's a pretty unfortunate API.

But okay, interesting, a separate GuestPolicy object that only contains the role for the nil user? Or I guess the role in the GuestPolicy doesn't even need any qualifications, since it's only instantiated for the nil user.

That does seem like maybe an OK solution. IMO not as great as if the gem supported it out of the box -- I think starting from scratch this "extra DSL" would be very little code. But at this point having to deal with with backwards compatibility issues... that might be the right trade-off and a fine solution, although I'm not hugely excited about being forced to split up my policies like this either. Maybe I'd actually prefer adding !u.nil? check to every role that isn't { |u| u.nil? } as a lesser evil. Not sure.

How about an example in docs though? Or maybe different examples of the different ways to handle it -- while ideally I think a gem like this provides one recommended supported battle-tested way to do this so the newcomer developer doesn't have to think/invent it for themselves, it seems like we're just not there yet, and some guidance is still useful -- if this is as common a use case as I think? (Maybe I'm wrong and this somehow isn't a confusing issue for most others?) If README is getting long, then in some other .md docs page with a link from README maybe.

[pokonski]

Yeah, having examples never hurts 👍 I'll gladly accept a PR with said examples if you are up to it!

And I'll definitely consider adding a built-in solution, but probably when I have to break compatibility for other reasons, I'm not fond of breaking it now for a single user's preference of code style, I hope you understand.

[jrochkind]

OK, I'll see if I can find time to do it, in the meantime this Issue at least serves as a findable thing with some ideas! (In addition to the NullUser idea in #32, which I don't really like for reasons explained, but maybe some do).

I'm not promising not to try a PR too, if I can find one I think will be persuasive. :) But nothing will be persuasive if this doesn't seem like a common use case -- I'm still not sure if most people using this (and it's authors) simply don't have apps where auth is used in areas without a logged in user, or if they instead find the existing solutions nicer than I do, or if there are lots of users and potential users who would agree with me after all. :)

[pokonski]

Most of the apps I used access-granted had 3-6 roles. Not wanting to add user.present? && or !user.nil? && in those 3-6 lines sounds exotic to me.

I'll take explicit over indirection of DSL any given day 😄

[jrochkind]

Part of where I'm coming from is wanting to use this in an 'engine gem' while making it simple for client apps to override or customize things cleanly.

But it may be that adding !user.nil to every role that has conditions that aren't {|u| u.nil?} is sufficient after all, or at least the lesser evil. I guess I'll start there, and maybe PR some docs.

I think maybe in current code that is indeed the best simplest 'general purpose' solution that should be doc'd, rather than the "NullUser" as in #32 or the separate policy objects.

[pokonski]

True, NullUser is useful if you still want it to work with some permissions inside a role, and not disregard the whole role.

[jrochkind]

Perhaps for "Common Examples" section. Does this seem right?

Dealing with non-logged in users

If your app wants to check auth in situations where there can be no logged in user at all (current_user == nil), this can cause errors on any role matchers assuming a user. One solution is to add nil checks to your role matchers:

class AccessPolicy
  include AccessGranted::Policy

  def configure
    # The most important admin role, gets checked first

    role :admin,  proc {|u| !u.nil? && u.admin? } do
      can :manage, Post
      can :manage, Comment
    end

    # Less privileged moderator role
    role :moderator, proc {|u| !u.nil? && u.moderator? } do
      can [:update, :destroy], Post
      can :update, User
    end

    # Applies to every logged-in user.
    role :member, proc {|u| !u.nil? } do
      can [:update, :destroy], Post do |post, user|
        post.author == user && post.comments.empty?
      end
    end

    # applies to no logged-in user at all
    role :no_user, proc {|u| u.nil? } do
       can :read, Post do |post, _nil_user|
           post.public?
       end
    end
  end
end

Note that you can't use hash role matchers (like { is_admin: true}) if you might be checking against nil current_users.

[jrochkind]

Or maybe that last one doesn't even need the proc {|u| u.nil? } check, being last declared it'll apply anyway. You just need to avoid trying to use the second "user" param in your block, either way.

[pokonski]

You are correct, the last one does not need the check and the , _nil_user| part of the argument list can be omitted for can block :) Otherwise looks good!

[pokonski]

@jrochkind can you send a PR with those changes to README?