laravel / ideas

Issues board used for Laravel internals discussions.
939 stars 28 forks source link

Allow Anonymous / Guest policy checks #1222

Closed garygreen closed 6 years ago

garygreen commented 6 years ago

The problem

At the moment, if you don't have an authed user all policy checks will be skipped and authorization is always denied:

// No authed user, so the policy check isn't even called (Auth::user() = null)
Gate::allows('view', $picture);

class PicturePolicy {

    public function view(User $user, Picture $picture)
    {
        // Won't be called.
    }

}

You can get around this by using a null-object pattern and passing a blank user to be checked:

// Policy check will be called.
// You can test for anonymous by doing $user->exists in your policy.
Gate::forUser(Auth::user() ?? new User)->allows('view', $picture);

However this breaks down when you use things like blade directives:

// Policy won't be called, again.
@can('view', $picture)

Current Solution

You could solve this by doing the above - manually passing a null user, but with that you need to be aware across your whole codebase which policy checks accepts anonymous users. Or you could have the permission logic exist on your model, but then that defeats the purpose of having separate logic for checking of permissions in your policies.

Proposed Solution

Allow policies to be defined as accepting a null User.

You hint to the gate which policies accept anonymous user checks:

// Allow anonymous check for specific policy and method
Gate::acceptsAnonymousCheck('App\Policies\PicturePolicy@view');

// Allow anonymous check for whole policy
Gate::acceptsAnonymousCheck('App\Policies\PicturePolicy');

// Allow anonymous checks for all policies.
Gate::acceptsAnonymousCheck(true);

class PicturePolicy {

    public function view(User $user = null, Picture $picture)
    {
         if (is_null($user)) {
            // Anonymous user.
        }
    }

}
Patryk27 commented 6 years ago

I think a better, non-breaking-change way could be to have two methods:

class PicturePolicy {

  public function view(User $user, Picture $picture)
  {
    /* ... */
  }

  public function viewUnauthorized(Picture $picture)
  {
    /* ... */
  }

}

That is: the original one (like view) and the "guest one" (like viewUnauthorized - the method's name would be automatically built by Laravel, no need to register it anywhere).

Apart from the realization method, I'm all in for the guest policies anyway.

koenhoeijmakers commented 6 years ago

What is there to check when a guest goes into the policy?, shouldn't you just not call $this->authorize() or whatever in the case that the "guest" can view it?

What is the situation that you need to check authorization on "nobody"?

You have to have a key to open the door, if its possible to open it without a key then it wasn't locked in the first place.

garygreen commented 6 years ago

@koenhoeijmakers see here for example use cases: https://github.com/laravel/framework/pull/20660#issuecomment-396629249

Looks like there has been various PRs done but Taylor has rejected them all (not necessarily because anonymous / guest checks shouldn't be added, but implementation details are tricky)

lukeramsden commented 6 years ago

My use-case is that I am checking factors on the model as well as the user (i.e. whether a listing is published or not) and for some policies I would like guest users to be able to pass authorization (i.e. a 'view' page).

public function view(User $user, JobListing $jobListing)
{
    return $jobListing->isPublished()
        || ($user->isValidCompany()
            && $jobListing->company_id === $user->userable->company_id);
}

It's a shame that the maintainers are very much against any sort of implementation of this, although I do understand the arguments against it, and my fix is as simple as:

if(Auth::check())
    $this->authorize('view', $jobListing);
else if(!$jobListing->isPublished())
    throw new AuthorizationException();
mfn commented 6 years ago

I like the idea but I don't like the null-pattern but do like the null-User pattern.

IMHO the policy methods should always receive "a User".

Would it be possible to have a middleware or something which checks Auth::user() and if empty, sets a special GuestUser extends User?

donnysim commented 6 years ago

?User $user guest approach was merged into 5.7, so I don't thing there's anything left to argue about 😅