Core: Deserializer before security

[vincentchalamon]

I have an ApiResource with an access control to ROLE_EMPLOYEE. When I send anonymously a request (with an iri of a non-existing object) to create an object, I get a 400, but I should receive a 403.

Entity:

/**
 * @ApiResource(attributes={
 *     "access_control"="is_granted('ROLE_EMPLOYEE')"
 * })
 * …
 */
class Shipping
{
    /**
     * @var null|PsOrder
     * …
     */
    private $order;
}

Request:

{
    "order": "/ps_orders/1"
}

Response: Item not found for "/ps_orders/1".

Maybe it's just a question of priority with DenyAccessListener that should be higher than DeserializeListener.

@api-platform/core-team What status code would you expect in this case: 400 or 401/403?

[norkunas]

Current priority allows to deny based on deserialized data so imho this works as expected.

[vincentchalamon]

I've discussed that point with @dunglas, and yep this works as expected, as the access_control requires an object, so it needs to be deserialized first. But I still find it weird that I get a 400 instead of a 401/403

[Taluu]

IMO this is wrong to deny on a modified object on voters. Why ? Because before updating something, you should vote against the fetched object and not the modified object. An example : I have an object that has a relation to another one. I want to vote and accept * only * if this object is myself (so I'm the owner).

But if in my request I change that, well... I'm not voting against the correct object (it's modified), and thus I'm not allowed to do so. It should be the role of a validator.

On the case of POST, it may differ a bit, but no so much. Anyway, on collection operations, you don't vote against an object, so problem solved (you vote against the environment : user, context, ... and so on).

So IMO the security listener should be fired BEFORE the deserializer listener.

[weaverryan]

2779 was merged, which allows you to use the original data in your access_control.

That's great! However, I still think the original problem from @vincentchalamon is valid:

 * @ApiResource(attributes={
 *     "access_control"="is_granted('ROLE_EMPLOYEE')"
 * })

With this setup, my validation rules are executed and the client (who should not have access to this/these endpoints at all) gains the ability to see the validation rules for it.

[dunglas]

Indeed. We could do the security check after deserialization, but before validation. WDYT?

[antograssiot]

but that's already the case @dunglas
https://github.com/api-platform/core/blob/125dec90d05dc383e9214cd4968428323d0a1136/src/Bridge/Symfony/Bundle/Resources/config/security.xml#L24

https://github.com/api-platform/core/blob/2f941fc3c0dc9dd257da3b34a6bb3f5c21c4230c/src/Bridge/Symfony/Bundle/Resources/config/validator.xml#L23

[dunglas]

So I don’t understand how this issue appears 😄. I’ll have to create a reproducer.

[soyuka]

Mhhh so this isn't fixed :sweat_smile: ?

[vincentchalamon]

As explained in the issue description, the problem occurres on deserialization, not on validation. The DeserializeListener priority is higher than the DenyAccessListener, but it should be the opposite.

I think we should reverse those listeners priorities (DenyAccessListener priority 2, DeserializeListener priority 1), but it'll be a BC break (imo).

Registered Listeners for "kernel.request" Event
===============================================

 ------- ------------------------------------------------------------------------------------------------- ---------- 
  Order   Callable                                                                                          Priority  
 ------- ------------------------------------------------------------------------------------------------- ---------- 
  ...
  #18     ApiPlatform\Core\EventListener\ReadListener::onKernelRequest()                                    4         
  #19     ApiPlatform\Core\EventListener\DeserializeListener::onKernelRequest()                             2         
  #20     ApiPlatform\Core\Security\EventListener\DenyAccessListener::onKernelRequest()                     1         
  #21     ApiPlatform\Core\Bridge\Symfony\Bundle\EventListener\SwaggerUiListener::onKernelRequest()         0         
  #22     Nelmio\ApiDocBundle\EventListener\RequestListener::onKernelRequest()                              0         
 ------- ------------------------------------------------------------------------------------------------- ----------

[dunglas]

Doesn’t the new original_data variable fix this?

[vincentchalamon]

No cause I also have the error on a POST operation. For example, sending the following request with a SecuredDummy.owner doctrine relation:

POST /secured_dummies
{
    "owner": "/users/invalid"
}

I get an _Item not found for "/users/invalid"_ error on AbstractItemNormalizer line 423.

[dunglas]

It looks like the intended behavior to me.

Deserialization occurs before checking access control rule, but save occurs after. Then you can check in your deny access control both values contained in the previous object, and in the new one.

[vincentchalamon]

Imo, if a user has no access to a resource (for example: POST /users), it should receive a 401/403 whatever value is sent.

The exception thrown in the AbstractItemNormalizer could reveal some sensitive data even for unauthenticated users: _the user /users/4084c6de-aec9-451c-a3f3-841b29190f4d does not exist_.
This could be, imo, a security leak.

[weaverryan]

Hi again!

Ignoring the need for BC momentarily, I think the order of the listeners is incorrect: the security listener should come before the access control listener. Why? Take an example from a traditional Symfony app where we're POSTing some data to edit a product:

/**
 * @Route("/products/{id}", methods="POST")
 */
public function edit(Product $product, Request $request)
{
    // OPTION A: Check security BEFORE updating Product

    // some fake, simple logic for updating the Product
    $product->setName($request->request->get('name'));

    // OPTION B: Check security AFTER updating Product
}

We would choose OPTION A every time: security is checked on the "original" Product in all cases. If you want to prevent the user from updating the Product in some way, that's always done with validation.

That's why I think DenyAccessListener should come before DeserializeListener and that object should not be exposed as a variable inside access_control.

How do we do this in a BC way?

My proposal is:

A) Deprecate the access_control so that we can change its behavior.
B) Replace it with access_requires and make this run before DeserializeListener.

Btw, I think the expression language is also verbose. I'd prefer to simplify with two different options - e.g.

 *     itemOperations={
 *          "delete" = {
 *              "access_requires" = "ROLE_USER",
 *              "access_if" = "is_granted('ROLE_USER')"
 * .        }
 *     },

Both of these would run before deserialization. The access_requires would automatically pass ROLE_USER to isGranted() internally AND it would pass the "previous object" (if this is an item operation) as the "subject". This would allow people to easily use voters - e.g. "access_requires" = "EDIT". For access_if, the object variable would actually be the "previous object" and no previous_object variable would exist.

And if people really DO want to be able to access the deserialized object in their access controls (which I think is a mistake), we could also add invalidate_if="object.author != user"... but I think that's a bad idea.

Oh AND... somewhat unrelated, we could also add an access_denied_status_code=404 option is we wanted to make it easier to return a "404" for access denied situations... which is part of what @vincentchalamon was talking about.. and is of course what something like GitHub does for private repos.

WDYT?

[Taluu]

Not sure that multiplying the access_ config would be the way to go (too confusing)... but overall, :+1:

[weaverryan]

Yep, I'm mixing 3 proposals into one, and each can be taken independently:

1) Deprecate access_control and rename to access_requires and run that before deserialization
2) Make access_requires not use the expression language (and add access_if for that use-case)
3) Add access_denied_status_code option to allow people to 404 on access denied.

[teohhanhui]

I think we're over-complicating things here. access_control is meant for simple cases. Having it run after DeserializeListener, other than for BC, also has the advantage that we can make authorization decisions based on both before and after states (i.e. "Is this transition allowed?"). To me that's very valuable, and the closest thing we can get to satisfying every needs.

If the user absolutely needs the access control to happen before deserialization, they could create a custom listener. I think trying to fix it in API Platform is just going to make the experience confusing for everyone.

[teohhanhui]

But DenyAccessListener should be renamed to something better / more neutral, e.g. AuthorizeListener, that doesn't sound like it's just going to "deny access".

[weaverryan]

Having it run after DeserializeListener, other than for BC, also has the advantage that we can make authorization decisions based on both before and after states (i.e. "Is this transition allowed?").

I think "deciding if the user has access to something" and "is this transition allowed" are two separate things. The first is authorization (e.g. 403). The second is validation - I HAVE access to perform an operation, but I'm trying to do something invalid (e.g. 400).

Both are valid things to do, which is why I proposed potentially having both. For example, to deny access if I'm not logged in but (for example) fail validation if I try to change this object's "owner" property to some other user:

 *     itemOperations={
 *          "delete" = {
 *              "access_requires" = "ROLE_USER",
 *              "invalid_if" = "object.owner !== user"
 * .        }
 *     },

I'd argue that invalid_if belongs as a validation constraint instead... but this option is handy to prevent needing to create many custom validators.

[teohhanhui]

I think "deciding if the user has access to something" and "is this transition allowed" are two separate things. The first is authorization (e.g. 403). The second is validation - I HAVE access to perform an operation, but I'm trying to do something invalid (e.g. 400).

The answer to the question "Is this transition allowed?" could depend on the currently authenticated user, that's what I was trying to illustrate. For example, an admin might be allowed to perform a certain transition, but not a normal user.

I've tried to make this point before, but I still think validation and authorization are intertwined for most non-trivial scenarios.

[weaverryan]

I've tried to make this point before, but I still think validation and authorization are intertwined for most non-trivial scenarios.

That's very true. So, it comes down to:

A) Keep things as they are now (simple, just access_control, but no built-in ability to deny access before deserialization failures)
B) Split into 2 parts - access_requires (before deserialization) and invalid_if (after deserialization) (now we have 2 ideas instead of one, but more flexibility, including the ability to add an option where we could 404 instead of 401/403 on security failure... not possible now because deserialization failures would still cause a 400 to leak)

I prefer (B), but it is subjective :).

[dunglas]

I understand the arguments to have a check before deserialization, but I agree with @teohhanhui. Let's keep the concepts to learn for the user, and the code simple.
We already provide the extensions points needed for more advanced needs.

Regarding the name of the attribute, and of the listener, shouldn't we name them security, as the @Security Symfony annotation? While I'm not fond of this name, it's an already familiar one for Symfony developers.

[vincentchalamon]

Reading the different proposals, I agree with the name proposed by @dunglas, whatever it's deny or access, it's about security and this name is familiar for Symfony developers.

About the problematic I exposed earlier, I understand that it's a tricky bug and could only happen on some special cases (mainly related to /users resource). So instead of changing api-platform as I recommended, WDYT about adding a section in the documentation to explain this special case and the recommended solution to fix it (create a custom EventSubscriber on PostRead)?

[dunglas]

https://github.com/api-platform/api-platform/issues/1218 looks related... and it’s a very big deal.
I see no other solutions than running the security check before the deserialization.

[dunglas]

Because of api-platform/api-platform#1218, I changed my mind, and I think we actually need @weaverryan's B proposal.

Some questions:

• I don't understand the benefit of not supporting security expressions in invalid_if
• Couldn't we find more explicit names?

Here is my proposal:

/**
  * @ApiResource(security="is_granted('ROLE_ADMIN') && object.getOwner() == user")
  */
// security is executed before deserialization, and object obviously contains the existing data
// it's the rule that we will promote in the documentation, as it's safer
// the name is consistent with Symfony's @Security

/**
  * @ApiResource(lateSecurity="is_granted('ROLE_ADMIN') && previous_object.getOwner() == user")
  */
// works exactly as the current "accessControl" attribute, but the name is more explicit
// alternative proposal: postDeserializationSecurity (longer, but maybe more explicit)

Both can be used together if needed, access_control will be deprecated.

If we want to support using raw strings (I'm not sure that it's very useful), we could add IsGranted variants of these 2 new attributes: isGranted and lateIsGranted (or postDeserializationIsGranted).

WDYT?

[Taluu]

late* looks more confusing to me. How about keeping access_control (that's what it is), but then having a map early and late ? e.g @ApiResource(access_control={"late"="is_granted('ROLE_ADMIN') && previous_object.getOwner() == user", "early"=...})

(Not sure for the syntax, but I think you get my point). We could continue to support the string access_control="is_granted..." which would be translated as "late" as it is currently the case ?

[dunglas]

IMHO early should be the default because it's "more" secure. Also I'm not very fond of maps (as we do for DTOs) because:

• it's very verbose when using annotations
• our infrastructure ($resourceMetadata->get*Attribute() methods aren't designed to handle maps gracefully)

Switching to security give us the opportunity to:

• deprecate the default behavior (late) in favor of the new one early)
• be consistent with Symfony (Symfony used to use access_control but switched to security some versions ago)

[weaverryan]

I don't understand the benefit of not supporting security expressions in invalid_if

Yea, this is an unrelated thing. It's just that the expression is ugly (user comment here https://symfonycasts.com/screencast/api-platform-security/access-control#comment-4583880639) if you're only ever using the is_granted() thing (which I recommend, because I subjectively like voters better than the expression).

Happy to see the issue moving forward - thanks for the work on this!

[dunglas]

Now that we're moving forward, I'm not against new attributes to choose the HTTP code and to support the "raw" role.

which I recommend, because I subjectively like voters better than the expression

I prefer voters too, but even when using voters, expressions are still mandatory to do something like is_granted('READ', object) isn't it?

By the way, as a native speaker, what do you think about the new names: currently in #2992 it's security (early) and securityPostDenormalize (late)?

[weaverryan]

I prefer voters too, but even when using voters, expressions are still mandatory to do something like is_granted('READ', object) isn't it?

Yes and no. What we would need to do is automatically pass the object for the user. So, it would look something like security="READ". Internally, we would call is_granted('READ', object) on their behalf.

By the way, as a native speaker, what do you think about the new names: currently in #2992 it's security (early) and securityPostDenormalize (late)?

I like them. Consistent with the @Security annotation from the SensioFWExtraBundle, which also took an expression.

On that note, in addition to @Security, that bundle also added @IsGranted(), which does not take an expression. That could be inspiration for an "is_granted"="READ" type of non-expression option to go along with security. I would like that... but I realize that we're talking about two different things at once ((A) the new security "system" for running security before normalization and (B) a possible new option that doesn't have an expression).