ContentEntityForm::addRevisionableFormFields contain invalid access checks

This JSON:API test coverage was written without my involvement, see #2943899: Moderation state field cannot be updated via REST, because special handling in ModerationStateFieldItemList.

The failing assertion:

      // Information disclosure prevented: when a malicious user correctly
      // guesses the current invalid value of a field, ensure a 200 is not sent
      // because this would disclose to the attacker what the current value is.
      // @see rest_test_entity_field_access()
      $response = $this->request('PATCH', $url, $request_options);
      $this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\nrest_test_validation: REST test validation failed\n", $response);

That is now a 403 response instead of a 422.

3 observations:

1. Did you already step through the code starting in NodeAccessControlHandler, where access to modifying vid is being denied, to follow the trace to how this gets mapped to a 422 response? If you did, I can’t find it in this issue.
2. That test coverage was added in #2821077: PATCHing entities validates the entire entity, also unmodified fields, so unmodified fields can throw validation errors. Did you read that issue, to try to understand why your change is causing the 403, and whether that is indeed also a sec vulnerability?
3. Possibly something is causing a new revision to be created automatically, which then results in vid being validated, and that causes the 403?

Removing tag, reviewed and tested

Added a few comments.

Also, that the media type settings basically has the same info as node type, with the administer media permission, but media isn't fixed yet here.

One last thing, to make this issue even more fun. I'm not even 100% sure that fixing access to do what the permission says is even the right thing to do at this point. It's likely been like this for a very long time, and sites probably rely on it, because while they might to allow users to decide if a new revision should be created or not, but not things like changing the author or sticky/promoted flags and whatever else these administer permissions allow.

which I now realize is exactly what #3364973: Make Add content/media default publishing option descriptions consistent is about, it's updating the description to match how it actually works, which is IMHO fine. There's still things to fix and improve here, like the invalid field access operations, but we wouldn't actually change NodeAccessControlHandler here.

Added some new comments.

To be clear, as I said in #12, I'm not convinced that this is the right thing to do. I think the referenced issue that just fixes the docs is the better direction. If someone wants to alter access they can do so, but this is going to be a behavior change for many sites that currently rely on this being visible.

I would suggest that we extract the few unrelatd bits that make sense, like the JS fix, and then instead get the other issue in and close this as won't fix?

To keep this from stalling, unless anyone has a counter argument for #14 going to move to NW for #14

Rescoped based on #12 and #14

We can just remove these access() calls entirely.

Refining IS with more detail.

Possible to add test coverage though? Not sure if anything can be pulled from the original MR?

No, this is just removing invalid code.

If it’s causing a bug then shouldn’t we have test coverage.

What bug is it causing?

Was filed as a bug, assuming it was a bad access check. So would say test coverage should be added. But I’ll leave for others to review

The bug is that the access checks are invalid and functionally don't do anything, the issue summary is up to date with this information and the original bug report is there as well.

Code changes look fine. I guess since this does not fix an actual bug, it could be reclassified as a task, but doesn't matter too much.

Had to read through this a couple of times, but overall agreed this is just dead code that should be removed, we have the issue open to make whether this stuff shows configurable which would be good to land.

Committed/pushed to 11.x, thanks!