- 🇮🇳India sahil.goyal
Patch #36 is conflicting to apply to D10.1.x, so Updated the patch to make it compatible and trying to Fix the errors.
- 🇳🇱Netherlands bbrala Netherlands
Let me get a review in :)
-
+++ b/core/modules/jsonapi/src/Access/EntityAccessChecker.php @@ -67,9 +67,9 @@ class EntityAccessChecker { + protected $AccessInterface = NULL;
This feels weird, the variable name shouldn't start with a capital letter.
-
+++ b/core/modules/jsonapi/src/Access/EntityAccessChecker.php @@ -95,14 +95,14 @@ public function __construct(ResourceTypeRepositoryInterface $resource_type_repos + public function setLatestRevisionCheck(AccessInterface $latest_revision_check) {
Since we are broadening the hint, i think this is not bc-breaking. So this is fine.
-
+++ b/core/modules/jsonapi/src/Access/EntityAccessChecker.php @@ -196,8 +196,8 @@ protected function checkRevisionViewAccess(EntityInterface $entity, AccountInter + if ($entity_type->getLinkTemplate('latest-version') && $entity->isLatestRevision() && isset($this->AccessInterface)) {
I wonder, if you already have access to view all revisions, do we even need to check for the specific revision? Or could you have permissions to see all revisions and then get denied for some specific revision? Although technically possible i kinda wonder why that would be the case.
Summarizing; some small changes needed, the change seems reasonable now that the blocking issue is merged.
-
- 🇨🇭Switzerland amermod
We are still talking about revision checks while it should be the role of the EntityAccessControlHandler to do the revision access check.
And it actually does that already, so why is there still a revision access check in the jsonAPI module ? Because the EntityAccessControlHandler can not be trusted for revisions, as said in #3, four years ago ?
I just attached a patch that removes simply the revisions check, to see what will fail.
- last update
over 1 year ago 29,374 pass - last update
over 1 year ago 30,325 pass