Uploaded image for project: 'eZ Publish / Platform'
  1. eZ Publish / Platform
  2. EZP-22068

Add loadRelatedContent() and loadEmbedContent() to the API

    Details

      Description

      These methods should check content/view_embed permissions and filter out those not accessible, which would remove the need to check for the same using Repository::sudo() in:

      • eZ/Publish/Core/FieldType/XmlText/Converter/EmbedToHtml5
      • eZ/Publish/Core/MVC/Symfony/Controller/Content/ViewController

      Ref:

      https://github.com/ezsystems/ezpublish-kernel/pull/490
      https://github.com/ezsystems/ezpublish-kernel/pull/644

      Both methods should check for the same permissions, but if for example content/view_relation is going to be introduced in the future it might make sense to implement them both.

        Issue Links

          Activity

          Hide
          Petar Spanja (Inactive) added a comment -

          As can be seen in https://github.com/ezsystems/ezpublish-kernel/pull/758, we have a need to load both embedded Content and embedded Location. This is all about checking correct permissions, so I would suggest:

          1. ContentService::loadEmbeddedContent() - checks content/view_embed on Content
          2. LocationService::loadEmbeddedLocation() - checks content/view_embed on Content in a Location

          Regarding ContentService::loadRelations(), since Relation contains ContentInfo and not Content, it can check and return Relation for both content/view and content/view_embed.

          Show
          Petar Spanja (Inactive) added a comment - As can be seen in https://github.com/ezsystems/ezpublish-kernel/pull/758 , we have a need to load both embedded Content and embedded Location. This is all about checking correct permissions, so I would suggest: ContentService::loadEmbeddedContent() - checks content/view_embed on Content LocationService::loadEmbeddedLocation() - checks content/view_embed on Content in a Location Regarding ContentService::loadRelations(), since Relation contains ContentInfo and not Content, it can check and return Relation for both content/view and content/view_embed.
          Hide
          André Rømcke added a comment - - edited

          > This solves the problem that if you would return a relation in loadRelations where the user only has view_embed permission the loadContent method will throw an UnauthorizedException because the user has no content/view permission.
          > By now i don't see any usecase for the method loadEmbedContent (when introduced, it should be named loadEmbeddedContent)

          loadRelations() check content/read permissions on all relations, this is why we have this issue open.

          However, yes, there should perhaps be a spec on this, this issue jumps to conclusion that we need new API's, while the use case needs to be defined instead. (Also we are about to change how RichText field type deals with relations so it's done in view layer, this will affect the requirements of such a functionality).

          Show
          André Rømcke added a comment - - edited > This solves the problem that if you would return a relation in loadRelations where the user only has view_embed permission the loadContent method will throw an UnauthorizedException because the user has no content/view permission. > By now i don't see any usecase for the method loadEmbedContent (when introduced, it should be named loadEmbeddedContent) loadRelations() check content/read permissions on all relations, this is why we have this issue open. However, yes, there should perhaps be a spec on this, this issue jumps to conclusion that we need new API's, while the use case needs to be defined instead. (Also we are about to change how RichText field type deals with relations so it's done in view layer, this will affect the requirements of such a functionality).
          Hide
          Christian Bacher (Inactive) added a comment -

          loadRelatedContent is different from loadRelations(). First it loads content - not relations. This solves the problem that if you would return a relation in loadRelations where the user only has view_embed permission the loadContent method will throw an UnauthorizedException because the user has no content/view permission.
          By now i don't see any usecase for the method loadEmbedContent (when introduced, it should be named loadEmbeddedContent)

          Show
          Christian Bacher (Inactive) added a comment - loadRelatedContent is different from loadRelations(). First it loads content - not relations. This solves the problem that if you would return a relation in loadRelations where the user only has view_embed permission the loadContent method will throw an UnauthorizedException because the user has no content/view permission. By now i don't see any usecase for the method loadEmbedContent (when introduced, it should be named loadEmbeddedContent)
          Hide
          André Rømcke added a comment - - edited

          Christian Bacher / Petar Spanja As we already have loadRelations(), should we consider changing it?

          So basically change it to filter instead of throw if user does not have access to one of the related content? And then do the exact same thing in loadEmbedRealtions but using view_embed permission?

          While at it, this function is missing paging, but because of the permission stuff it will be pointless to do this in SPI as it will most likely not match what API returns.

          Show
          André Rømcke added a comment - - edited Christian Bacher / Petar Spanja As we already have loadRelations(), should we consider changing it? So basically change it to filter instead of throw if user does not have access to one of the related content? And then do the exact same thing in loadEmbedRealtions but using view_embed permission? While at it, this function is missing paging, but because of the permission stuff it will be pointless to do this in SPI as it will most likely not match what API returns.

            People

            • Assignee:
              Unassigned
              Reporter:
              Petar Spanja (Inactive)
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:

                Time Tracking

                Estimated:
                Original Estimate - 5 hours
                5h
                Remaining:
                Remaining Estimate - 5 hours
                5h
                Logged:
                Time Spent - Not Specified
                Not Specified