Problem/Motivation

We have a way to natively specify that some fields should be patch protected. However, we don't have the same for view protected. In my case a field may supplied during POST or PATCH but not viewable after.

Other areas of the ResourceTestBase allow the user to customize the expected document enough to remove the view protected fields, however some areas escape developer control.

Steps to reproduce

Allow POST or PATCH of a resource with a field that will not be returned on a response. Tests that validate the fields will fail.

Proposed resolution

Add a static property for the developer to specify that a field should not be expected on a response. Similar to the property for patch protected fields.

Remaining tasks

None

User interface changes

None

Introduced terminology

Introduces the view protected fields to the JSON API testing suite.

API changes

Should be none. As far as I can tell the addition of this property as provided should not conflict with any existing work.

Data model changes

None

Release notes snippet

Added '$viewProtectedFieldNames' to describe view protected field for JSON API Resources.

CommentFileSizeAuthor
#20 3491135-nr-bot.txt2.3 KBneeds-review-queue-bot

Issue fork drupal-3491135

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

rowrowrowrow created an issue. See original summary.

rowrowrowrow’s picture

Status: Needs work » Needs review
rowrowrowrow’s picture

Issue summary: View changes
quietone’s picture

Version: 11.1.x-dev » 11.x-dev

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.

quietone’s picture

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.

rowrowrowrow’s picture

Assigned: rowrowrowrow » Unassigned

I'm working on the code changes. These are done and ready for review.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Change should have test coverage.

bbrala’s picture

Thanks for opening this issue, you've been on fire lately.

I was thinking, wouldn't this "just work" (tm) if you set field permissions? Although i'm a little weary if this is possible to allow post/path but then not view. But this might just work because of entity api.

function YOUR_MODULE_NAME_entity_field_access($operation, FieldDefinitionInterface $field_definition, AccountInterface $account, FieldItemListInterface $items = NULL) {
  $result = AccessResult::neutral();
  if ($field_definition->getName() == 'field_we_care_about') {
    if ($operation == 'edit' && !in_array('administrator', $account->getRoles())) {
      $result = AccessResult::forbidden();
    }
  }
  return $result->addCacheContexts(['user.roles:administrator']);
}
rowrowrowrow’s picture

Title: Support View Protected Fields » Support Testing View Protected Fields
Status: Needs work » Needs review

@bbrala, Ya modifying the field access would be one of the ways to protect a field from view. I guess the naming of the issue is misleading. I'll update the name to "Support Testing View Protected Fields". This update is to provide the same sort of patch protected testing that's used on the User entity, and maybe others, to view protected fields.

@smustgrave What do you suggest for test coverage when modifying the tests?

smustgrave’s picture

Status: Needs review » Needs work

Would see if there is an existing test in jsonapi that can be expanded or would have to write a brand new one.

rowrowrowrow’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Needs work

I don't see anything that calls that variable to testing.

Before moving to review please check that the pipeline is passes the code standard checks.
If a test fails that's fine just shows the fix needs to be tweaked usually
When adding test coverage, if the pipeline could run, there's a test-only feature that checks if the test covers the change.

rowrowrowrow’s picture

Status: Needs work » Needs review

Yep apologies, I marked it ready for review while still working on updates. I opted to move the view protected field checking to its own method as we would want to test in various places, namely after a successful get, post, and patch. I then applied this method after the expected successful requests

smustgrave’s picture

Status: Needs review » Needs work

Thanks for working on it.

So the summary and or title don’t really make sense. The summary seems like it wants to add a new feature but I just see changes to test files. Yes feature requests need test coverage but also needs the new feature added to the MR so we can see that the tests pass.

rowrowrowrow’s picture

Yep, I can add a description to the method. I don't think we are currently testing that the User's "pass" field is unavailable on any get request so it's been implemented on the UserTest.

If we need a test for a test I'm not sure how to implement that seeing as it would require an entity that has exactly what the User entity has, a field that is modifiable but not viewable. Please let me know how best to proceed.

bbrala’s picture

Yeah, i dont think you need test coverage there, you do need to address the other small nits though. Although the out of scope space for TRUE i'd say is ok.

joshua1234511 made their first commit to this issue’s fork.

joshua1234511’s picture

Status: Needs work » Needs review
needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new2.3 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

oily made their first commit to this issue’s fork.

Fixed PHPCS/STAN.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.