Silently fail UserError when record rule denies access

[edbo]

I’m working on a custom app to speed up the performance. The app talks to Tryton using JSON-RPC which the clients are also using. Instead of fetching data in many requests, I’m trying to do it now as much as possible in one fetch. So relational fields and even deeper are also fetched in the same request. This improves the performance and the UX of the app drastically, the overall payload is lighter and the server is more happy to do the query once instead of many times.

Tryton also fully supports ‘dotted’ fields (Models — trytond latest documentation) and it works perfectly.

However, I have some record rules in place and a user is not allowed to read some records which have a certain state. A UserError is send back instead of the data.

Is there a possibility to let the record rule silently fail and return nothing for the requested data? Now the whole request failed because a relational record could not be read because of it’s state. In my case, this record is 3 layers deep so it’s there when needed.

I already digged through the code but I’m lost how it all works.

[ced (Cédric Krier)]

In theory it could be added to the ORM but I do not think it will be a good idea because the JSON-RPC is designed for the client usage. So we want that such error message being raised (to fix them) if the client is trying to read something the user can not because it should not (we added code that normally avoid such case). And we do not want to show something as empty because user is not allowed to see it.

But your use case is probably one of the cases for which I think REST API for user application will be useful.

[edbo]

@ced

In theory it could be added to the ORM but I do not think it will be a good idea because the JSON-RPC is designed for the client usage.

Would it be possible to switch it off by a context parameter? Something like _check_access does.

@ced

In theory it could be added to the ORM but I do not think it will be a good idea because the JSON-RPC is designed for the client usage.

Maybe it’s not a good idea but it works extremely well. I’ve build several custom apps using JSON-RPC and those are acting like clients and not user applications. Also why allowing multi-level ‘dotted’ fields when the clients are not using it to the full extend.

Imagine I want to implement silencing by a context parameter. Where do I start? I found several checks for the model and field, but no record rule.

[ced (Cédric Krier)]

@edbo

Would it be possible to switch it off by a context parameter? Something like _check_access does.

Everything is possible. But I fear about the complexity cost and security implication.

@edbo

Also why allowing multi-level ‘dotted’ fields when the clients are not using it to the full extend.

Well the client is already using it but mainly with one level. But having more level support was a kind of side-effect of the design. Any way I think we could have usage of it in the future for example to display read-only related field in a view without having to define a Function field.

@edbo

Imagine I want to implement silencing by a context parameter. Where do I start? I found several checks for the model and field, but no record rule.

That’s the difficulty the access enforcement is done in many places. Mainly in ModelSQL.read but the API is always expecting to have some values if no exception is raised.

[edbo]

@ced

@edbo

Would it be possible to switch it off by a context parameter? Something like _check_access does.

Everything is possible. But I fear about the complexity cost and security implication.

Complexity: Yes, Security: No. This is about silencing and IMO it should only be done in read and it should return None. This should never happen on a write, create or delete, here the error should be shown because you did something what wasn’t allowed.

@ced

@edbo

Also why allowing multi-level ‘dotted’ fields when the clients are not using it to the full extend.

Well the client is already using it but mainly with one level. But having more level support was a kind of side-effect of the design. Any way I think we could have usage of it in the future …

Thanks, that means it won’t go away .

@ced

@edbo

Imagine I want to implement silencing by a context parameter. Where do I start? I found several checks for the model and field, but no record rule.

That’s the difficulty the access enforcement is done in many places. Mainly in ModelSQL.read but the API is always expecting to have some values if no exception is raised.

I digged a bit further and found the code (trytond/trytond/model/modelsql.py · 0792dc3f046873996c63dcd1e46bff3c4362f7f5 · Tryton / Tryton · GitLab) I have to tinker with. And indeed the API expects data back but there is None. So I have to find out which columns are asked for and return them back with None .

[edbo]

OK, got it working relatively easy. Don’t know if it’s a nasty hack, but it works beautifully. As proof-of-concept I changed https://foss.heptapod.net/tryton/tryton/-/blob/0792dc3f046873996c63dcd1e46bff3c4362f7f5/trytond/trytond/model/modelsql.py#L1082-L1086 from

if not len(fetchall) == len({}.fromkeys(sub_ids)):
    cls.__check_domain_rule(
        ids, 'read', nodomain='ir.msg_read_error')
    cls.__check_domain_rule(ids, 'read')
    raise RuntimeError("Undetected access error")
result.extend(fetchall)

to

if not len(fetchall) == len({}.fromkeys(sub_ids)):
    if transaction.context.get('skip_user_error') is True:
        fetchall = []
        none_keys = {}.fromkeys(columns)
        # id is also None, we must replace that with the actual id
        # we use a set to cancel out ids which exists more then once
        for none_id in set(sub_ids):
            none_keys['id'] = none_id
            fetchall.append(none_keys)
    else:
        cls.__check_domain_rule(
            ids, 'read', nodomain='ir.msg_read_error')
        cls.__check_domain_rule(ids, 'read')
        raise RuntimeError("Undetected access error")
result.extend(fetchall)

There are still some loose ends, there should be a check to silence the UserError, and there is not a parameter yet which tells that.

[JCavallo (Jean Cavallo)]

I understand what you are trying to do, but I think None is not a good value here, because it cannot be distinguished from a real value (i.e. the user does not know that you tried to access a field you were not allowed to).

Maybe use a dedicated data structure (something like {'error': 'Unauthorized access'}, or even better a dedicated Singleton that will be serialized by the dispatcher, but that can be tested on).

Another problem that you may encounter is if a getter tries to use a field that was not actually read because access was forbidden. Using None will lead to other crashes / inconsistent results.

[edbo]

@JCavallo

I understand what you are trying to do, but I think None is not a good value here, because it cannot be distinguished from a real value (i.e. the user does not know that you tried to access a field you were not allowed to).

This is what Tryton comes up with, all values are None. The only thing I’m doing is returning those data with an actual id instead of throw an error.

@JCavallo

Maybe use a dedicated data structure (something like {'error': 'Unauthorized access'}, or even better a dedicated Singleton that will be serialized by the dispatcher, but that can be tested on).

I want my data back and not some error, that’s all. Because I’m fetching many data at one time I just want the data, even when there is an error because I asked for something I’m not allowed to fetch.

BTW, I updated my proof of concept above and you have to tell Tryton specifically to skip the UserError.

@JCavallo

Another problem that you may encounter is if a getter tries to use a field that was not actually read because access was forbidden. Using None will lead to other crashes / inconsistent results.

That’s maybe a problem and should be tested if this change will ever be accepted in the core, but as said you are telling Tryton to specifically skip the error, so you are also responsible that the results can be trusted.

In my case I only need this for two or three requests. All the other requests are working normal.

[ced (Cédric Krier)]

@edbo

This is what Tryton comes up with, all values are None.

I do not think so.

@edbo

That’s maybe a problem and should be tested if this change will ever be accepted in the core

I think a better solution instead of using the value None would be to not set any value. I mean that the dictionary would not have any key for the field that are not allowed.
This would be more similar to what happens when RPC is reading the records of a xxx2Many. The records for which user has no access are just no there.

[JCavallo (Jean Cavallo)]

@edbo

@JCavallo

Maybe use a dedicated data structure (something like {'error': 'Unauthorized access'}, or even better a dedicated Singleton that will be serialized by the dispatcher, but that can be tested on).

I want my data back and not some error, that’s all. Because I’m fetching many data at one time I just want the data, even when there is an error because I asked for something I’m not allowed to fetch.

What I meant was that the value for the data you do not have access to should not be None.

Let’s say you can’t read the field account :

>>> read([12], ['name', 'account'])
[{
    'id': 12,
    'name': "Whatever",
    'account': {'error': 'Unauthorized access'},
}]

So the client is aware that some data were not returned because they were not allowed to access it.

@ced’s solution (removing the key from the returned value) seems a good idea as well.

[edbo]

@ced

@edbo

This is what Tryton comes up with, all values are None.

I do not think so.

Tryton fills a columns variable trytond/trytond/model/modelsql.py · 47809a507263c47b927c318750f22499adecd036 · Tryton / Tryton · GitLab which in the end contains data. All those data have to be returned at the moment because it will error further down in the code.

@ced

I think a better solution instead of using the value None would be to not set any value. I mean that the dictionary would not have any key for the field that are not allowed.
This would be more similar to what happens when RPC is reading the records of a xxx2Many. The records for which user has no access are just no there.

What I discovered is that even I’m not allowed to read the record I am able to get the rec_name from that record.

>>> read([12], ['name', 'account', 'account.rec_name'])
[{
   'id': 12,
   'name': "Whatever",
   'account': 8,
   'account.': {
        'id': 8,
        'rec_name': "The account record name"
   }
}]

So the user can see it, but not change it or open the details of that record.

@ced

Any way I think we could have usage of it in the future for example to display read-only related field in a view without having to define a Function field.

I think most of it is already there.

In the end it doesn’t matter what you return to the client because:

1. skipping the field → when the client receives the data it has to do something with it. When several fields aren’t there, the client keeps the original value for that field
2. returning an error object for a field → the client have to decide what to do. The easiest way is to drop it and keep the original value for that field. The only benefit is that you can display a message that the request returned access denied for some fields
3. returning None → the same as 2. but now you don’t have the message

So looking at those 3. if it is possible to skip the fields, it will lighten the payload send back to the client a bit.

[ced (Cédric Krier)]

@edbo

In the end it doesn’t matter what you return to the client because:

It does matter because caller must be able to distinct. None is not a good value because caller can not distinct it from actual None value and missing access right.