Verify access right only for RPC

Feature
1

Rational

The ORM relies on the contextual _check_access key to enforce the access right.
The problem is that the context is propagated to every calls.
So for example when extending the create method, the developer must be careful about the operations done because they may be executed under the _check_access.
In the past we have already tried to move such check only to the border of the system but we always kept it for the CRWD methods because they may call CRWD of related models with external data for which access must be checked.
Ideally the checks should be performed only for the external input.

The ModelView.button and Workflow.transition needs to be applied to each time the method is extended so the checks are performed multiple times.

Proposal

All the CRWD methods get an extra keyword external=False which is set to True when called by the RPC (depending on the configuration).
Only when external is True the ModelAccess.check, Field.check and Rule.domain_get are used.
The Field.get and Field.set get also the external=False keyword which they pass when calling CRWD methods.

The ModelStorage.copy could have its behavior modified so it does not store in the database the records directly (nor duplicate translations) but only when external is True.
This way some optimization can be applied when copying records to assign them to another.

The base methods using ModelView.button or Workflow.transition are modified to always call a private version of the method (without the decorator).
Ex:

class Sale(ModelSQL, ModelView):
    __name__ = 'sale.sale'

    @classmethod
    @ModelView.button
    @Workflow.transition('confirmed')
    def confirm(cls, sales):
        return cls._confirm(sales)

    @classmethod
    def _confirm(cls, sales):
        ...

Implementation

2

Adding the keyword to all methods and duplicating the methods sounds like a lot of complexity.

As all the methods related to ModelStorage classes I’m wondering if it won’t be simpler to just have the external attribute stored on the class to know if the check should be performed or not.

Then it’s a mather of activating just the attribute when calling the method from RPC and when calling the CRWD methods of related models (only if the pool have it activated).

For buttons, we have the method name on the ir.model.button table, we may just remove the decorator and move the code to the RPC call. This way will be called just one time without the need of adding any manual code.

Not sure if the same can be applied to Workflow.transition.

Also not sure if calling multiple times both methods is really an issue, some extended method may change the state of the records, so the decorator ensures that the other calls are applied only when the state is not updated.

3

Except that storing attribute on the class breaks all the modularity and the thread separation, this is exactly the same as the current context.

This does not work because we need the workflow and button decorator to be executed also for non-external.

It is a performance issue and a risk for bugs (wrong decorator may be applied).

This is precisely one buggy code that this proposal tries to prevent.

4

I already tought there should be something obious I was missing which is the thread separation. Thanks for pointing it.

So the problem with the current context is that once the _check_access is set to True it can not be set to False for all the code included on the decorator. Right?

Instead of storing a True/False value can we just store a list of models to check? The idea is just to set this values on RPC (or in the nested CRUD methods) and if the check function is called for a call that is not on the list of methods it just ignores the check?

I think I used such buggy code sometimes to skip some transitions, so I will consider some customization behaviour. But having said that, by duplicating the methods such code could be already implemented by extending the public method instead of the private one. So we are not fixing anything, am I right?

Having said that, It seems there are two different problems that may required a different solution because as you say:

But RPC check should be only checked for external.

5

Note that I see a problem with all the current code and with people missing migration of the code. Adding a new optional keyword may be missed by the developer of custom modules. So once a method is overriden, if the keyword is missed it will always get the default value no mather what value is set by RPC. Which means that access rights are not checked for any call of such module.

6

This does not solve the problem because:

  • we will need to inspect the arguments of the RPC call to determine the models which duplicate the current code structure
  • the same model could be needed to check and not to check access, for example the One2Many can be written and used for domain validation

If possible we we try to add a test to forbid that.
Any way developers can break Tryton in many ways that we can not prevent.
But it is different to have be able to do something or to have only this way to do it. So we propose a correct way to extend transitions, if developers still do differently, they will be on their own.

Probably for ModelView.button (for now) but not for Workflow.transition, so it is better to have a single way to deal with all those decorators.

No because in such case the code will crash when called from RPC.

7

I’m all for improving performance but to be honest the proposal looks quite invasive to me and most important I think it makes Tryton quite more complex with a risk of very stupid bugs in module development. I’d hope for easier development if possible.

I was thinking of the same that Sergi proposed with something like the dispatcher adding to the context the model and method called. Something like: Transaction().set_context(_check_access={'sale.sale': set('write')}).

This would mean that the checks would only be done in the write operation of sale.sale. If my module wants to override that operation and ensure the check is also done in some other model I can easily add it to the context. Transaction could even add some syntactic sugar to ease adding or removing operations alla Transaction().add_checks(('sale.line', 'create')) or Transaction().remove_checks(('sale.sale', 'write')).

This mechanism could be used by ModelSQL’s create when it calls the create operation in child models.

But you made this comment which I do not understand:

What does that mean exactly? What would be duplicated?

Again, I don’t grasp it. Could you provide a more detailed example? Can’t the removal of the checks using the context solve that by removing the check in subsequent calls?

Thanks, I think I’m missing something deep here.

8

I do not understand. On contrario the proposal makes the design much more explicit and simple.

As I already said this solve nothing. It is exactly the same as what we have now.

The discovery of what is created/modified by the request.

This is already what we do now and it is complex and poor performent as we need to switch context in many placed.