Problem/Motivation

Please refer the problem/motivation section of #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method

Proposed resolution

Write EntityResourceTestBase subclass for the ShortcutSet entity.

Unfortunately, ShortcutSet does not have an access control handler for 'view' operation, so that needs to be added too. Fortunately, ShortcutSet and Shortcut entities are closely related. And Shortcut has 'access shortcuts' fot 'view' operation. So, the logic use it for ShortcutSet too!

Unfortunately, ShortcutSet has 'customize-form' link relation types, but drupal core hasn't info about this type, sot that needs to be added too. We can use RFC6861 for this, like edit form.

Remaining tasks

References

1. Follow-up of #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method
2. Subtask of #2824572: Write EntityResourceTestBase subclasses for every other entity type.

CommentFileSizeAuthor
#13 interdiff.txt862 bytesAnonymous (not verified)
#13 entityresource_provide-2843779-13.patch8.82 KBAnonymous (not verified)
#11 interdiff-9-11.txt813 bytesAnonymous (not verified)
#11 entityresource_provide-2843779-11.patch8.95 KBAnonymous (not verified)
#9 interdiff-2843779-5-9.txt1.99 KByogeshmpawar
#9 entityresource_provide-2843779-9.patch8.96 KByogeshmpawar
#5 2843779-5.patch8.95 KBAnonymous (not verified)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

naveenvalecha created an issue. See original summary.

sumanthkumarc’s picture

Assigned: Unassigned » sumanthkumarc
sumanthkumarc’s picture

Issue tags: +SprintWeekend2017

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Anonymous’s picture

Status: Active » Needs review
FileSize
8.95 KB

The patch contains two additional changes:
1.

+++ b/core/core.link_relation_types.yml
...
+customize-form:
+  description: "The target IRI points to a resource where a submission form for customizing associated resource can be obtained."
+  reference: '[RFC6861]'

Without this change we have problem here:

# EntityResourceTestBase.php:440
Failed asserting that Array &0 (
    0 => 'https://drupal.org/link-relations/delete-form'
    1 => 'edit-form'
    2 => 'collection'
) is identical to Array &0 (
    0 => 'customize-form'
    1 => 'https://drupal.org/link-relations/delete-form'
    2 => 'edit-form'
    3 => 'collection'
).

2.

+++ b/core/modules/shortcut/src/ShortcutSetAccessControlHandler.php
@@ -19,6 +19,8 @@ class ShortcutSetAccessControlHandler extends EntityAccessControlHandler {
    */
   protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
     switch ($operation) {
+      case 'view':
+        return AccessResult::allowedIf($account->hasPermission('access shortcuts'))->cachePerPermissions();

Without this change we have problem here:

# EntityResourceTestBase.php:388
Failed asserting that 403 is identical to 200.
sumanthkumarc’s picture

Assigned: sumanthkumarc » Unassigned

@vaplas , thanks for working on issue. You can now assign yourself.

Wim Leers’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/hal/tests/src/Functional/EntityResource/ShortcutSet/ShortcutSetHalJsonCookieTest.php
    @@ -0,0 +1,18 @@
    +  use CookieResourceTestTrait;
    +  /**
    

    Supernit: needs blank line between these.

  2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/ShortcutSet/ShortcutSetResourceTestBase.php
    @@ -0,0 +1,97 @@
    + * ResourceTestBase for Shortcut entity.
    ...
    +   * The Shortcut entity.
    ...
    +   * @var \Drupal\Shortcut\ShortcutInterface
    

    ShortcutSet

If it was only the first point, I'd have RTBC'd. But with that second point, it can't go in as-is. So, marking NW just for that. Easy fix! :)

yogeshmpawar’s picture

Assigned: Unassigned » yogeshmpawar
yogeshmpawar’s picture

Assigned: yogeshmpawar » Unassigned
Status: Needs work » Needs review
FileSize
8.96 KB
1.99 KB

Updated patch as per comment #7 & also added interdiff.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

Anonymous’s picture

Thank you @Wim Leers for review and @Yogesh Pawar for help! A small additional trivial cleanup. (I was so happy that the tests passed, that I sent the patch faster than necessary, sorry for this).

Wim Leers’s picture

Hah :)

Nice additional clean-up!

Anonymous’s picture

Added @todo in getNormalizedPostEntity, by analogy with the other config cases. Please, point me this is right change or we need some other change? (sorry for the eternal flaws)

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Even better :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: entityresource_provide-2843779-13.patch, failed testing.

Anonymous’s picture

Status: Needs work » Reviewed & tested by the community

Random fail.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: entityresource_provide-2843779-13.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

DrupalCI infra fail.

catch’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

Committed 3d292b3 and pushed to 8.4.x. Thanks!

Moving to 8.3.x for cherry-pick after the commit freeze.

Fixed this on commit:

+++ b/core/core.link_relation_types.yml
@@ -76,6 +76,9 @@ create-form:
+  description: "The target IRI points to a resource where a submission form for customizing associated resource can be obtained."

s/IRI/URI/

  • catch committed 3d292b3 on 8.4.x
    Issue #2843779 by vaplas, Yogesh Pawar: EntityResource: Provide...
alexpott’s picture

Category: Task » Bug report
Status: Patch (to be ported) » Needs work
Issue tags: +Needs issue summary update

In order for this to be backported with need an issue summary update to clarify what's being fixed on this issue. Since it went beyond just providing tests and into the land of fixing access control.

Anonymous’s picture

Issue summary: View changes

#19: I'm not sure about s/IRI/URI/. Looks like IRI - Internationalised Resource Identifier. But maybe URI makes sense too.

customize-form:
  description: "The target IRI points to a resource where a submission form for customizing associated resource can be obtained."
  reference: '[RFC6861]'

This is copy from

edit-form:
  description: "The target IRI points to a resource where a submission form for editing associated resource can be obtained."
  reference: '[RFC6861]'

because it have similar behavior, which looks like in the scope of the RFC6861 specification.

#21: Tried to do it.

Wim Leers’s picture

Status: Needs work » Patch (to be ported)
Issue tags: -Needs issue summary update

Thanks, @vaplas!

  • Gábor Hojtsy committed 876400b on 8.3.x authored by catch
    Issue #2843779 by vaplas, Yogesh Pawar: EntityResource: Provide...
Gábor Hojtsy’s picture

Status: Patch (to be ported) » Fixed

Looks good to me for a cherry-pick to 8.3.x as per #19. The view permission on shortcut sets was neutral before and is now tied to the access shortcuts permission which makes total sense.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.