EZP-31595: Added embedded permission check in UrlAliasRouter #57
Conversation
barw4
changed the title
eZ/Bundle/EzPublishCoreBundle/Resources/views/default/content/embed.html.twig
Outdated
Show resolved
Hide resolved
| {% set parameters = { | ||
| "isEmbed": true | ||
| } %} | ||
| {% if location is defined %} | ||
| <h3><a href="{{ ez_path(location) }}">{{ content_name }}</a></h3> | ||
| <h3><a href="{{ ez_path(location, parameters) }}">{{ content_name }}</a></h3> | ||
| {% else %} | ||
| <h3><a href="{{ ez_path(content) }}">{{ content_name }}</a></h3> | ||
| <h3><a href="{{ ez_path(content, parameters) }}">{{ content_name }}</a></h3> |
There was a problem hiding this comment.
Shouldn't this rather skip generating link if user does not have access?
As in:
- The link will always go to full view, which implies user needs to have full access
- So rather then doing the checks here in the router, the approach is maybe rather to check rights when rendering embed and pass variable to template to let it skip link if user does not have access to full view (if template is rendered it's already implied user has access to see it, at least embed view of it)
same goes below for inline embed view.
There was a problem hiding this comment.
Shouldn't this rather skip generating link if user does not have access?
@andrerom I'm not really sure right now. The links are present also in other places in the app even when the user does not have access to linked content, so that's why I decided to kinda go with this solution. Should I change it for the solution you mentioned?
There was a problem hiding this comment.
I think so, otherwise we are just pushing the issue elsewhere as the user will click on a link and get exception. But maybe @alongosz has some perspective on this as well
There was a problem hiding this comment.
I'm against this idea and @andrerom seems to agree on that. I don't feel like Router should check any permissions. Generating a route is different from viewing a content item. You should be able to generate any route but it should be decided elsewhere wether the user has an access to view it and he should be shown a link.
| RequestContext $requestContext, | ||
| LoggerInterface $logger = null | ||
| ) { | ||
| $this->locationService = $locationService; | ||
| $this->urlAliasService = $urlAliasService; | ||
| $this->contentService = $contentService; | ||
| $this->generator = $generator; | ||
| $this->repository = $repository; | ||
| $this->permissionResolver = $this->repository->getPermissionResolver(); |
There was a problem hiding this comment.
Never do this. Pass PermissionResolver as another dependency and let container builder to inject it.
There was a problem hiding this comment.
+1, considering PermissionResolver is already decoupled and available as a service.
| * @return string The generated URL | ||
| * | ||
| * @throws NotFoundException |
|
Closing this PR as Router should be left intact in this case. |
Checklist:
$ composer fix-cs).