[FIXED] Restrict query results on beforeFind() table call back without breaking buildRules() table callback in CakePHP

Issue

I am currently running into an issue with some legacy code on an old Cake inventory applications I maintain. The application contains inventory items and those items should only be seen by users that are at the same location as the item. For that reason we have the items, along with a number of other models, extend the BaseTable. This essentially grabs the ids of locations that the logged in user is able to view inventory items at using getUserLocationIds() and then appends the output to the query on the beforeFindRule() to sort out items the user should not be able to see:

<?php

class BaseTable extends Cake\ORM\Table
{
    public function beforeFind(Event $event, Query $query, ArrayObject $options)
    {
        parent::beforeFind($event, $query, $options);

        $repositoryTable = $query->getRepository()->getAlias();

        $loacationIds = Cake\ORM\TableRegistry::getTableLocator()
            ->get("Locations")
            ->getUserLocationIds();

        $query->andWhere([$repositoryTable . ".location_id IN " => $locationIds]);
        
    }
}

The above code has worked in production great for years. Although now we are getting a user that is that is not able to see an existing item at their location, because the software has it assigned to another location they do not have access to. They are assuming that that item is not in the system and creating another item using the same id_number and client_id.

The ItemsTable should restrict this from being possible with the buildRule() listed below:

<?php

class ItemsTable extends BaseTable
{
    public function buildRules(Cake\ORM\RulesChecker $rules): Cake\ORM\RulesChecker
    {
        $rules->add($rules->isUnique(['id_number', 'client_id'], 'The Number you selected is in use, please use another'));
        return $rules;
    }
}

However the build rule is failing to restrict this duplicate item because the code mentioned in beforeFind() in the first code snippet is causing isUnique() to only examine entries returned by getUserLocationIds().

I am wondering how I can make the logic in beforeFind() not apply to the function calls in buildRules()? Or if there is a better way to do this?

Bonus Question: Should I be validating this rule in MySQL as well as CakePHP? What is the best practice for this sort of thing in a CakePHP application?

Thanks!

Edit: I am not 100% sure yet, but but after further inspection this question may be in relation to a bug in CakePHP and not the implementation of the code. I have posted the following issue on GitHub.

Solution

I wouldn't say that's a bug, it looks more like the intended behavior to me, after all beforeFind is ment to affect all ORM queries.

If the rule would ignore beforeFind modifications (which would be a kinda complicated thing to accomplish), then this would cause lots of problems for the other end, ie for those situations where one wants the query modifications to apply.

IMHO the cleanest approach would currently be for you to for example use a custom finder that applies the location restrictions, and then you explicitly use it where you want it to be used, or to use a custom/extended rule where you can pass query options that your beforeFind callback could evaluate to make the filter optional.

As far as the core is concerned, maybe it would be helpful if the rules would for example accept a callback that allows to modify the query.

ps. IMHO yes, you should also look into adding proper unique indexes to the database table, so that data integrity is ensured even if fancy feedback application rules do fail.

Answered By - ndm