Included entities that have access denied must comply with the spec

Setting back to Needs work since we expect tests to fail.

I don't know the JSON API normalizers remotely well enough to give any advice at all. Pinged e0ipso on IRC.

That makes quite a lot of sense. I think this included behavior was an artifact of the normalization and the inclusion process.

If anything I think that we should just exclude any entity from the included pool and add an entry in the errors section of the metadata. Additionally we'll need to make sure that we maintain full linkage, excluding any possible children entities from the tree. This may or may not happen already with the current implementation.

It sounds like the spec is ambiguous in this respect. Should we not discuss this with the JSON API spec authors to A) clarify this, B) make the spec more specific?

Note that a response can't contain both a data and an error top-level member, so the response would need to be either a clear failure or a successful response without any error info.

To avoid that violation the Partial Success specification adds the errors inside of the meta section. We are doing that already for collections, so we should treat includes the same way.

This discussion made me think of the 'view label' entity access operation — created #2843922: Show label of inaccessible entities ('view' access denied) when 'view label' access is allowed for that.

However, the id and type members, which are required by spec

This is only if we add them in the included section. What I meant in #11.

If anything I think that we should just exclude any entity from the included pool and add an entry in the errors section of the metadata

So, that means:

1. Do not include any reference to any non-accessible entity in the included section of the payload
2. For each non-accessible entity add an error object to the meta > errors section of the payload. In there, we need no type nor id to comply with the spec, if I'm not mistaken.

I'm proposing this.

The patch will give this output for a request that tries to include 3 related articles, but 2 of them are unpublished.

GET /jsonapi/node/article/d6ad5f38-056c-44b0-9377-7a6006a76dca?_format=api_json&fields[node--article]=nid,field_related&include=field_related&XDEBUG_SESSION_START=PHPSTORM HTTP/1.1
Host: d8dev.local

Outputs:

{
    "data": {
        "type": "node--article",
        "id": "d6ad5f38-056c-44b0-9377-7a6006a76dca",
        "attributes": {
            "nid": "2492"
        },
        "relationships": {
            "field_related": {
                "data": [
                    {
                        "type": "node--article",
                        "id": "674aa079-a9a5-4ea9-84a5-fcaa7ff89f3c"
                    },
                    {
                        "type": "node--article",
                        "id": "db15f994-d921-4776-ad26-44971db7f236"
                    },
                    {
                        "type": "node--article",
                        "id": "0cf799ee-f3c2-47b2-aefe-94a04e965cc6"
                    }
                ],
                "links": {
                    "self": "http://d8dev.local/jsonapi/node/article/d6ad5f38-056c-44b0-9377-7a6006a76dca/relationships/field_related?_format=api_json",
                    "related": "http://d8dev.local/jsonapi/node/article/d6ad5f38-056c-44b0-9377-7a6006a76dca/field_related?_format=api_json"
                }
            }
        },
        "links": {
            "self": "http://d8dev.local/jsonapi/node/article/d6ad5f38-056c-44b0-9377-7a6006a76dca?_format=api_json"
        }
    },
    "links": {
        "self": "http://d8dev.local/jsonapi/node/article/d6ad5f38-056c-44b0-9377-7a6006a76dca?_format=api_json&fields[node--article]=nid%2Cfield_related&include=field_related&XDEBUG_SESSION_START=PHPSTORM"
    },
    "included": [
        {
            "type": "node--article",
            "id": "0cf799ee-f3c2-47b2-aefe-94a04e965cc6",
            "attributes": {
                "nid": "2125"
            },
            "relationships": {
                "field_related": {
                    "data": [
                        {
                            "type": "node--article",
                            "id": "55da880f-6ab0-4b8d-bae0-b20fc1d3be53"
                        },
                        {
                            "type": "node--article",
                            "id": "c61ddb0a-862a-4bb9-8c65-d80008acda05"
                        }
                    ],
                    "links": {
                        "self": "http://d8dev.local/jsonapi/node/article/0cf799ee-f3c2-47b2-aefe-94a04e965cc6/relationships/field_related?_format=api_json",
                        "related": "http://d8dev.local/jsonapi/node/article/0cf799ee-f3c2-47b2-aefe-94a04e965cc6/field_related?_format=api_json"
                    }
                }
            },
            "links": {
                "self": "http://d8dev.local/jsonapi/node/article/0cf799ee-f3c2-47b2-aefe-94a04e965cc6?_format=api_json"
            }
        }
    ],
    "meta": {
        "errors": [
            {
                "title": "Forbidden",
                "status": 403,
                "detail": "Access checks failed for entity node:2475.",
                "links": {
                    "info": "http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.4"
                },
                "code": 0
            },
            {
                "title": "Forbidden",
                "status": 403,
                "detail": "Access checks failed for entity node:2112.",
                "links": {
                    "info": "http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.4"
                },
                "code": 0
            }
        ]
    }
}

I created a follow up to improve the contents of the error object: #2844130: Create a custom EntityAccessDeniedHttpException.

But what about

Note that a response can't contain both a data and an error top-level member, so the response would need to be either a clear failure or a successful response without any error info.

Isn't #21 sneakily circumventing that by putting the errors in meta? I guess there's no other way? I guess errors = hard errors, and meta > errors = soft errors?

This is partially discussed in http://discuss.jsonapi.org/t/multi-status-responses-partial-success-in-p...

Short story, the spec would have you fail the whole request if one of the includes is not success (like one item being 403 in a collection or in a list of includes). So yeah, this is correct:

I guess there's no other way? I guess errors = hard errors, and meta > errors = soft errors?

Thanks, that confirms my suspicion.

Leaving it to @hampercm to RTBC.

the spec would have you fail the whole request if one of the includes is not success

Therefore, shouldn't the whole request fail instead of giving a partial response? Does this not mean that if the module is to follow the spec that this "needs work"?

@DamienMcKenna sorry for the confusion, but I meant that if we wanted to add the errors straight to the errors section in the top level we'd have to fail the whole request. I feel very strongly not to fail a request if one included element (via includes or i the collection) has access denied.

In any case this module is following the spec strictly with this patch applied.

Given that:

1. @hampercm is good with the approach
2. This is consistent with the current implementation with collections
3. @Wim Leers didn't have any code related suggestions
4. This fixes the current API compliance issue

I'm going to merge this to unblock #2840677: Validate responses against the JSON API specification.

@DamienMcKenna, @hampercm please feel free to create a follow up ticket if you feel we should resolve this problem in some other way. I just want to bring includes and collections to the same situation. If we need to change the approach, it needs to change in both scenarios.