Entity normalization should check field access to avoid leaking data

As mentioned in the other issue, if you add read checks, we should check write too?

And as also mentioned, it makes the serializer aware of the current user, and possibly needs a way to override that?

Should we remove the checks from rest.module then?

Wouldn't/Shouldn't the rest module take care of write access if it is saving a resource? I don't know if a Normalizer should care about that too?

Well, assuming that it checks view access then it should care about edit access too?

That said, as mentioned in the other issue, I'm -1 on doing this inside the serializer in the first place. But it should be at least consistent?

Yes, good point. Consistency is a good thing, but denormalization is only creating an instance of a class. Nothing else. So edit access doesn't matter at that stage? view is slightly different IMO as we need that to actually access the original data. Denormalization we are just using data we have.

EDIT: Yes, I ideally wanted to keep this out of the serializer but that didn't seem to get a good reception :)

If we go down this route, berdir is right, we have to be able to pass in an account for access. To do that we have no other choice really other than using the serializer $context parameter.

Do I have this correct?

EntityTest testComment() has an integrity check comparing the following two objects

    $normalized = $this->serializer->normalize($comment, $this->format, ['account' => $account]);
    $denormalized_comment = $this->serializer->denormalize($normalized, 'Drupal\comment\Entity\Comment', $this->format, ['account' => $account]);

I think this exposes the problem that

\Drupal\comment\Entity\Comment::baseFieldDefinitions() contains the appropriate entry for hostname

    $fields['hostname'] = BaseFieldDefinition::create('string')
      ->setLabel(t('Hostname'))
      ->setDescription(t("The comment author's hostname."))
      ->setTranslatable(TRUE)
      ->setSetting('max_length', 128);

and yet comment.views.schema.yml does not!!!

If so Yeay for integrity checks. I am still looking into this but I thought is was worth a mention. I only have a limited amount of time to work on this for the next few days :(

This has nothing to do with comment.views.schema.yml, it has to do with permissions. One way to fix that test would be to log in as user 1 and then do the serialization. But that's exactly what I find scary about this issue.

This should just leave the rest tests.

Here is a unit test for the new ContentEntityNormalizer class too.

Let's try this.

+++ b/core/modules/hal/src/Normalizer/ContentEntityNormalizer.php
@@ -90,9 +95,12 @@ public function normalize($entity, $format = NULL, array $context = array()) {
+      // Continue if this is an excluded field or the current user does not have
+      // access to view it.
+      if (in_array($field->getFieldDefinition()->getName(), $exclude) || !$field->access('view', $context['account'])) {
         continue;
       }
+

It just is odd that you can't even opt out of access checking, even if you really want to have just the data.

People really want this locked down, what can I say. If you want to show a password field or something, you can override the AccessHandler for Users (or whatever other entity) and allow it there?

Forwarddrop will solve that for us.

I don't see a reason why we can't also add an ignore_field_access flag to context. Default would be false.

I don't see a reason why we can't also add an ignore_field_access flag to context. Default would be false.

Well, that was not the point, but people argued it should be really hard to bypass it (so changing the service).

We can look at an additional flag or some sort, but I think that should be in a follow up, let's fix this first so we are actually checking access.

Created #2387679: Allow bypassing access checking (using serializer context) for that.

This looks great. Agreed on keeping it locked down here, if we want to provide an easier way to open it up that's easier to add, as opposed to trying to remove a feature later on which would be much harder.

Committed/pushed to 8.0.x, thanks!