Entity query access check needed in TwigExtension::entityIdsByLabel

[patdunlavey]

Need to add $query->accessCheck(FALSE); right here.

PR coming...

[DiegoPino]

@patdunlavey cool. Thanks!

[DiegoPino]

@patdunlavey maybe actually a (TRUE) instead of false? You want to make sure the user has permissions for that one since it is exposed to the world?

[patdunlavey]

I almost always default to (FALSE) in these cases since that emulates the D9 behavior. Changing to (TRUE) could generate a regression, and should be more carefully evaluated. Do you object strongly to leaving it as (FALSE)?

[DiegoPino]

@patdunlavey we don't use that extension or have used with any partners for that reason. I can see a point towards security being done there if used blindly with FALSE. Access to unpublished nodes, etc.
For me the most concerning entities there are
"node" and "user"... so maybe adding access(TRUE) if one of those?
That all said, the function returns IDs... so basically once you try to load them/use them in any other way permissions will kick in anyways.
What is your current use case/need of this one? Vocabularies?

[DiegoPino]

Means I don't strongly object to FALSE, but want to be sure that if I have to make it TRUE in the future I will not break every site you have

[patdunlavey]

We're using it in AMI set processing to convert strings to their corresponding taxonomy term ids. I think setting it to (TRUE) will be fine, but I don't want to be wrong and have to spend a lot of time sorting it out.

[patdunlavey]

There's also the possibility of performance impact of access checks during AMI ingests.

[DiegoPino]

performance should not be an issue really, because the ::access() just adds under the hood a query condition, does not really load the entity but ok, let's keep it FALSE and I can write some if "node" alternatives later (taxonomies are kinda safe IMHO) but add (can you add a title to your pull too) a note to the Pull request so we can revisit this in the future? Thanks. Once you do that I can merge. If you need this backwards 1.3.0 please make a pull against 1.3.0 too, but given how different drupal 10.2 is to 10.1 I think 1.4.0 should be ok

[patdunlavey]

I just did some double-checking the above-described AMI set use-case, and I'm satisfied that it's safe to set the accessCheck to (TRUE). We don't use this twig function anywhere else, but if we did, I think it's reasonable to impose the check in those cases. So I'll make that change in the PR. If that causes anybody difficulty, they can raise a new issue!

[patdunlavey]

I'm fine with this being limited to 1.4.x, since the D10 accessCheck requirement comes with 1.4.x.

[DiegoPino]

Appreciate your work and feedback. Ready to merge!