iroha icon indicating copy to clipboard operation
iroha copied to clipboard

Optimize how `PermissionToken`s are stored

Open Arjentix opened this issue 2 years ago • 6 comments

Right now Account stores permission tokens and roles. Roles are stored as ids, their permission tokens aren't stored at Account, instead they are stored in WorldStateView. The problem is that it's misleading and leads to bugs, i.e. #2477. Programmer without knowing full context thinks that just using Account::contains_permission() he can check if this account can perform action or not, although this account can also have permissions granted by roles, so that programmer should check it using WorldStateView::account_permission_tokens(), which returns all tokens inclidung tokens from roles.

We should consider refactoring that. There is no perfect solution, but here are some ideas:

  1. Remove Grant<Account, PermissionToken> and use only Grant<Account, Role> (same for Revoke). This will break the API
  2. Store permission tokens from roles inside the account. Leads to more usage of memory and time 2.1. (optional) Use interning for permission tokens. Less memory usage but can be too complex

Arjentix avatar Jul 14 '22 17:07 Arjentix

I think 2 also leads to more complex code, because we will need to keep up to date account permission tokens. Also small change in role permission would lead to huge amount of work, because we must update tokens for EVERY account. IMO, the more straightforward the solution, the harder it will be to find the exploit. How critical it would be to break API?

Erigara avatar Jul 14 '22 18:07 Erigara

AFAIK right now is the right time to break the API if we need it. I also like more the first idea. Also we don't have a way to change Role. Only register it, grant, revoke and unregister

Arjentix avatar Jul 14 '22 18:07 Arjentix

My thought is near to the first idea, and I think the Grant<Account, PermissionToken> API is okay -- just internally represent the grantable permission as a singleton role. Then we can unify the internal process

s8sato avatar Jul 15 '22 05:07 s8sato

@s8sato then it will look odd, when user will try to get all account roles and receive a lot of new strange roles

Arjentix avatar Jul 15 '22 11:07 Arjentix

The primary achievement here will be the distinction that an account has references (role ids) and a world has entities (permission tokens). My suggestion is just to keep the "grantable permission" concept in line with the primary distinction. If the singleton roles and the ordinary roles are internally distinguished at the type level, they could be combined or not depending on external needs @Arjentix

s8sato avatar Jul 15 '22 15:07 s8sato

I'm more inline with @s8sato's proposal. It should be possible to store permissions inside wsv instead of storing them inside Account. This can be done as a singleton role, or a new mapping AccountId -> PermissionToken. I think the additional memory footprint is the same for both approaches; it's just the map key. I would prefer adding a new mapping in the wsv

mversic avatar Jul 18 '22 08:07 mversic