Specify access checking in content entity queries

Changing this to a "Task" because it's fixing deprecated code, not a bug.

I reviewed MR !30 and it looks like you hit all the uses of getQuery() @paul121 - nice job!

I could only think of one place where we might want to disable access checking: The UniqueBirthLogConstraintValidator

Doesn't look like this is being addressed with these changes, correct? It's setting ->accessCheck(TRUE) like all the rest. I think I agree that this should be ->accessCheck(FALSE) in this case, and that's OK because we are not displaying the log(s) in question in the constraint violation message, so it is not exposing data that the user doesn't have access to.

Related to this, we use a similar query to look up the asset's birth log so that we can link to it from their birthdate field when viewing an animal asset.

https://github.com/farmOS/farmOS/blob/e6f183512c8d00a0b580ecd5e2ad16f637...

We can use ->accessCheck(TRUE) (as you did already) here because it would basically just prevent the birthdate field from becoming a link to the log if the user doesn't have access to the log. 👍

I see that you also added support for this to the farm.log_query service, with an access_check option to disable it. I like that! We may need to think through how this is used by some of the downstream code though...

For example, this service is used by the asset.location and group.location services to determine an asset's current location and group membership. These look for the most recent logs that assign location/group to the asset. If we filter out logs that the user doesn't have access to, this could result in the asset appearing to be in a location/group that is not correct. For example, if the user doesn't have access to the most recent log, but does have access to the previous log.

I think we will want to explicitly set the access_check to FALSE for these, and think through how we should deal with the access implications downstream from that. Which may be just "leave it as is". I think in both of these cases (location and membership) we would still want to be able to say which location/group the asset is in, regardless of whether or not the user has access to the log that puts it there... and also maybe regardless of whether or not the user has access to that location/group asset too!

Ultimately, they can't access the referenced asset itself, so I don't think there is a security concern there. Although maybe even being able to see the name of the asset is arguably a concern.

Lastly, we should document the new access_check option in https://github.com/farmOS/farmOS/blob/e6f183512c8d00a0b580ecd5e2ad16f637... - I can push a commit to that effect...

This also raises questions about how we should treat inventory calculations (probably deserves a dedicated issue). We don't use an entity query for those, but we do query log quantities. What happens if someone doesn't have access to the logs that are being queried? Should they still be able to view the asset's inventory? 🤔

by setting a default here we're kinda re-creating the same issue Drupal core had.

This thought occurred to me too. It does make sense to make it an explicit decision. I think *most* of the time you'll actually want it set to FALSE when you're using farm.log_query, because as it states in the docs (emphasis mine):

The log query service is a helper service for building a standard log database query. This is primarily used to query for the "latest" log of an asset. The asset location and group membership services use this.

... and in those "typical" cases, filtering out logs that the user doesn't have access to changes the results in undesirable ways.

So if we *did* have a default, it might actually make more sense for it to be FALSE - but that feels like a security hole waiting to happen too.

So I agree - let's remove that from farm.log_query and require that it be set explicitly by anything that uses that service.

I'll sketch that up in a commit real quick and push it... standby...

Merged - thanks again for kicking this off @paul121!