Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.5] Allow Policies to work for unauthenticated calls #24140

Closed
wants to merge 1 commit into from

Conversation

soundsgoodsofar
Copy link

This change is to allow Policies to be used for unauthenticated Users. Currently, Gate.php will return false if the authenticated User does not resolve. This means that the entire Policy concept cannot be used with internal clients or unauthenticated endpoints.

There are cases where we want un-authenticated users to be able to manipulate certain models, but only if they meet some requirements. E.g. we have models that are created by un-authed Users before registration, and we want them to be able to manipulate them as long as they haven't been associated with a real User yet.

Regarding backward compatibility and not breaking lots of existing code, Policies written with User $user as a parameter will still fail because they won't accept null as a value. Only if the Policy is updated to User $user = null will it allow an anonymous call to proceed.

Here is an example of another user trying to accomplish this:
https://laracasts.com/discuss/channels/laravel/user-laravel-policies-with-anonymous-users?page=1

@soundsgoodsofar soundsgoodsofar changed the title Allow Policies to work for unauthenticated calls [5.5] Allow Policies to work for unauthenticated calls May 7, 2018
@taylorotwell
Copy link
Member

Wouldn't change this on a LTS release or current release.

@soundsgoodsofar
Copy link
Author

@taylorotwell is there another option for defining model policy controls for anonymous users? Laravel & Passport seem oddly opinionated that all API calls are going to come from authenticated users.

@browner12
Copy link
Contributor

i'm kind of curious how common this is, since at first glance it seems odd to authorize without authenticating first.

how are you determining the user? some kind of cookie or URL variable?

@soundsgoodsofar
Copy link
Author

@browner12 it's for authorizing actions on models by un-authenticated users. In our case, we allow visitors to start creating a project before they are actually required to register. We use uuids for the project models so they can't be guessed. Until that visitor registers, the Policy should allow un-authenticated users to create / edit / delete the project, but once the user registers a relationship gets set and at that point only the authenticated, owning user should be allowed to edit.

Without the functionality I've added here, I basically would have to create two parallel systems for model policy checks for unauthenticated users vs authed users. That seems like a bad idea.

In general, Laravel/Passport has very poor support for the concept of unauthenticated or internal API calls.

  1. Passport client credentials are insecure (Client credentials are not secure for documented use case passport#691)
  2. Passport does not support the concept of single routes that allow both authenticated & un-authed access (https://stackoverflow.com/questions/39593460/sharing-same-route-with-unauthenticated-and-authenticated-users-in-laravel-5-3)
  3. And here we don't support supposedly model-oriented permissions for un-authenticated users.

Yes, you can write custom middleware and (in the case of passport) jump through some crazy hoops to override the functionality above, but we're talking about just supporting un-authenticated API calls. That's not a particularly exotic thing IMO and certainly shouldn't require rewriting half the stack.

@devcircus
Copy link
Contributor

A custom middleware should be the go-to for these cases. The concept of authorization doesn't even make sense outside the scope of an authenticated user. At that point, you're not authorizing anything. Look back on previous PRs and discussions. This has been discussed several times here as well as on the forums. If you're rewriting 'half the stack' to fit this in, you're over thinking it.

@browner12
Copy link
Contributor

could you give an example of what the policy/gate would look like? I'm trying to wrap my head around what you would even check against, and having trouble figuring it out.

Also, I feel this would require a little more significant rewrite than what you have here, since I'm guessing a lot of users depend on that false being returned when there is no authenticated user.

definitely open to the idea, just trying to understand it better.

@soundsgoodsofar
Copy link
Author

Sure, here's an example:

public function edit(User $currentUser = null, Project $project = null)
{
    return empty($project->owner) || ($currentUser && $currentUser->id = $project->owner->id);
}

This supports a model where I want it to be editable by anyone who happens to have the id until it is owned by someone. Once it's owned, I only want it to be editable by the owner.

You can see another user who was trying to do the same thing in the link in my original post.

@devcircus how is this not authorizing anything? Policies are supposedly oriented around the model, not the user. Why is it invalid to have permission-based actions on models for anonymous users? I have searched through the threads I can find and they all end like the link I posted ("Did you find a solution?"). Can you link to a discussion with more substance on this?

@browner12
Copy link
Contributor

thanks for that example. can you explain what your routes look like for this, and if the auth middleware is attached? it seems like you would have to use 2 separate routes for this, because 1 would need the auth middleware and one could not have it.

@soundsgoodsofar
Copy link
Author

Correct @browner12 , I currently have public versions of my endpoints under a group that prefixes them with /public. That's not ideal either and I realize I could write a custom middleware to handle it, but one thing at a time.

So I have something like this:

Route::group(['prefix' => 'public'], function () {
    Route::resource('projects', 'ProjectController');
});
Route::group(['middleware' => ['auth:api']], function () {
    Route::resource('projects', 'ProjectController');
});

@soundsgoodsofar
Copy link
Author

To elaborate, I've made this change for myself by extending Gate. But Gate is hard-coded, so I had to also extend AuthServiceProvider to use my new Gate class (not Illuminate\Foundation\Support\Providers\AuthServiceProvider, but Illuminate\Auth\AuthServiceProvider).

Once I'd overridden that, I found that I needed to now add laravel/passport to composer.json's dont-discover to prevent auto-discovery, because classes whose namespaces start with Illuminate get registered before vendor classes, but my new class's namespace does not.

Do-able? Yeah I guess, but it took a lot of digging and I felt like this is something worth supporting.

@browner12
Copy link
Contributor

I would have to agree with @devcircus on this then. unless you are identifying a user, the concept of permissions and authorization doesn't seem to really make sense.

your setup with 2 routes seems like a perfect situation for a custom middleware. simply apply this middleware to your 'public projects' group, and in it check if the project has an owner. if so, throw a 401, or redirect somewhere, or whatever you'd like.

@soundsgoodsofar
Copy link
Author

@browner12 Right, but my example was simplified. I don't want to allow all actions on these models for unauthenticated users. For example, maybe an unauthenticated user should be allowed to edit, but not delete. Maybe they should be able to edit, but only if some other attribute is set.

See where that's going? We're re-inventing the whole concept of model Policies in a new middleware(s). Just seems like a bad way to address things to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants