Missing default access for all node fields

For now I built a defaultAccess check for the status field. The status field might be the most important one..

To accomplish this check I also introduced two new permissions to the node module:
* "change status of own %type content"
* "change status of any %type content"

..hope you like that.

It would be nice if I could get a review of my current patch. Even if there are more fields which needs to be covered and even if the UnitTest is not covering 100% of the cases.
It's my first patch for D8 and I made some comments in the code where I wasn't sure if I'm applying D8 stuff correctly...

And by the way:
Thanks for the drupalcon in prague!
I enjoyed to see (at least some of) you guys in person.

...just set it back to "needs work" when you put your experienced and valuable 2 cents to my patch ;-)

Okay.
I see.
The Create Resource Test from REST gave me answers to some questions ;-)
I need to refactor the permission checks.

After working on this issue at drupalcon in prague I'm sitting now at the Schnitzelcamp (DrupalCamp Vienna).
Great time again with you guys :-)

I'm submitting a new - refactored - patch to let drupal.org run it's tests on it.
It should fit better than the last one.
I also removed to custom permissions I mentioned above and will propose them in an extra issue.

After discussing with klausi and fago I have to admit, that I was wrong - in a couple things:
Operations in the defaultAccess method may only be "view" or "edit".
I don't need to check the entity for type "node" in the access method.
The permissions "edit [any|own] content" is not applicable for the status field. Only "bypass access control" and "administer nodes" are of interest.
And my test case was unnecessarily expensive by using WebTestBase class.

Thank you two guys for helping me on this.

1. +++ b/core/modules/node/lib/Drupal/node/NodeFieldStatus.php
   @@ -0,0 +1,59 @@
   +
   ...
   +
   ...
   + *
   ...
   +
   

   superflous whitespace

2. +++ b/core/modules/node/lib/Drupal/node/NodeFieldStatus.php
   @@ -0,0 +1,59 @@
   +    switch ($operation) {
   +
   ...
   +        if ($account->hasPermission('bypass node access') || $account->hasPermission('administer nodes')) {
   
   +++ b/core/modules/node/lib/Drupal/node/Tests/NodeFieldAccessTest.php
   @@ -0,0 +1,119 @@
   +  function setUp() {
   

   inheritdoc missing.

3. +++ b/core/modules/node/lib/Drupal/node/NodeFieldStatus.php
   @@ -0,0 +1,59 @@
   +        // The status of a node may be read by everyone who has access to the node.
   

   longer than 80 characters.

4. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeFieldAccessTest.php
   @@ -0,0 +1,119 @@
   +    $this->chuck_norris = $this->createUser(array(), array('bypass node access'));
   

   document that chuck norris is almighty.

5. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeFieldAccessTest.php
   @@ -0,0 +1,119 @@
   +   *
   

   white space

6. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeFieldAccessTest.php
   @@ -0,0 +1,119 @@
   +      if ( ! $node1->status->access("view", $account) ) {
   

   no white space before and after "!"

7. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeFieldAccessTest.php
   @@ -0,0 +1,119 @@
   +        $permission_denied = TRUE;
   

   should be tested for all users

8. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeFieldAccessTest.php
   @@ -0,0 +1,119 @@
   +  // TODO: create more tests for more base-fields of node.
   

   @todo

9. +++ b/core/modules/node/node.module
   @@ -1563,7 +1563,7 @@ function node_node_access($node, $op, $account) {
   + * @param $type
   

   unrelated change, should be removed.

further fields:

administrative edit permission: uid, status, promote, created, updated, sticky

revision_timestamp, revision_uid, log: revision permissions?

Now all administrative fields are handled through the NodeAdministrativeField (which restricts editing withoud administrative permissions).

Test this please.

Here is a new patch based on the approach in #2029855: Missing access control for user base fields, with the tests from this issue. I think this issue has a better chance at getting in and getting the API in. We can implement the other entity types any time, but we need to have the API in asap.

Added revision_log to the administrative list.

revision_uid and revision_timestamp and changed are managed automatically, no user should have access to edit them, so updated accordingly. changed could possibly even do that by default.

Cleaned up and updated the test, but conceptually still the same + tests for the new rules.

Should be critical no?

1. +++ b/core/modules/node/src/NodeAccessController.php
   @@ -131,6 +133,21 @@ protected function checkCreateAccess(AccountInterface $account, array $context,
   +      return $account->hasPermission('administer nodes');
   

   why don't we also check for "bypass node access" here? Please add a comment.

2. +++ b/core/modules/node/src/NodeAdministrativeField.php
   @@ -0,0 +1,39 @@
   +/**
   + * Status field of node.
   + */
   +class NodeAdministrativeField extends FieldItemList {
   

   Status field? I think it applies to more than that?

   And why do we need this administrative field class? Please add a comment.

   And where is this class used? Shouldn't the NodeAccessController cover this already?

1. Because NodeForm is not doing that either.

2. That class is dead, accidently ended up in the patch again.

Then the test case is flawed, because it claims that $chuck_norris is all mighty having only the "bypass node access" permission.

Yep, it is. The uid 1 problem again.

I will update the change record at https://www.drupal.org/node/2101719 to mention the new easier way for base field access, no need to add a new one IMHO.

I like the approach in #26. Patch looks great to me, but the issue summary is now out of date.

Would it make sense to convert NodeForm::form() to use this as part of this patch, or is there a reason for that to be done separately?

1. +++ b/core/modules/node/src/NodeAccessController.php
   @@ -131,6 +133,21 @@ protected function checkCreateAccess(AccountInterface $account, array $context,
   +    return parent::checkFieldAccess($operation, $field_definition, $account, $items);
   

   Not really useful to call the parent here, just return TRUE? And this should have a comment like "Per default all node fields can be viewed."

2. +++ b/core/modules/node/src/Tests/NodeFieldAccessTest.php
   @@ -0,0 +1,128 @@
   +      //'uid' => $chuck_norris->id(),
   

   This should be removed.

Content types have the "Display author and date information" config option. Shouldn't we deny view access to the node author and the created date if this option is set? Might be a consideration for a followup issue.

#2226493: Apply formatters and widgets to Node base fields should take care of removing the manual access checks I think.

I don't think we should restrict access based on that setting. That is about visiblity of that default output, not access. It's perfectly valid to disable that and show that information in a different way.

#31.1: Can't imagine why now, but would not be the first time we'd later on add something to the base class, so I'd prefer calling it.
#32.2, Ups, debug left-over, removed.

Updated the issue summary.

AccessController is now AccessControlHandler.

Re-roll proudly presented by git rebase.

:)

Committed 8d61447 and pushed to 8.0.x. Thanks!

+++ b/core/modules/node/src/NodeAccessControlHandler.php
@@ -129,6 +131,21 @@ protected function checkCreateAccess(AccountInterface $account, array $context,
+    $read_only_fields = array('changed', 'revision_timestamp', 'revision_uid');
+    if ($operation == 'edit' && in_array($field_definition->getName(), $administrative_fields)) {

I was wondered by "readonly" because there's setReadOnly(TRUE) for some base fields but seems that have different meaning