Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix unreadable Grant instruction validator error message #2369

Closed
Arjentix opened this issue Jun 18, 2022 · 2 comments
Closed

Fix unreadable Grant instruction validator error message #2369

Arjentix opened this issue Jun 18, 2022 · 2 comments
Assignees
Labels
Enhancement New feature or request iroha2-dev The re-implementation of a BFT hyperledger in RUST Security This issue asks for improved security

Comments

@Arjentix
Copy link
Contributor

Arjentix commented Jun 18, 2022

Problem

Take a look at default_permissions() at permissions_validators/src/public_blockchain/mod.rs. This configuration is used for checking instrucitons in Iroha 2 by default right now.

The first part of validators configuration is called grant_instruction_validator. If this part fails then it produces error message that says pretty nothing:

Error: Transaction was rejected

Caused by:
   0: Transaction rejected due to insufficient authorisation
   1: Action not permitted: None of the instructions succeeded in Any permission check block with name: Grant instruction validator.

All we can retrieve from this message is that something is wrong with some Grant instruction. We don't know which instruction and what is exactly wrong.
That was a problem while debugging #2081

Suggested solution

I, personally, think that using Result doesn't fit well for validators, because we can't tell what (Err or Ok) should be returned from validator if it gets unexpected instruction. Right now Grant validators return Err, but Instruction validators return Ok. We need a unified way to do it. We need 3 variants: Ok, Err and Skip. We could use Option<Result<...>> but it looks ugly in my opinion.

So my suggestions is to introduce another return type for validators and refactor complex validators construction. This will allow us to tell which validators have failed in any_should_succeed() and etc. Altough I think that any_should_succeed() and all_should_succeed() will be less usable then and we can introduce noone_should_fail() which would meen "All should return Skip or Ok". And maybe at_least_one_success()

@Arjentix Arjentix added Enhancement New feature or request iroha2-dev The re-implementation of a BFT hyperledger in RUST Security This issue asks for improved security labels Jun 18, 2022
@Arjentix
Copy link
Contributor Author

Arjentix commented Jun 18, 2022

Yes, something like:

enum ValidatorVerdict {
    Allow,
    Deny(DenialReason),
    Skip,
}

@Arjentix
Copy link
Contributor Author

Our validation scheme reminds me about chain-of-responsibility pattern

@Arjentix Arjentix self-assigned this Jun 22, 2022
Arjentix added a commit that referenced this issue Jul 12, 2022
Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request iroha2-dev The re-implementation of a BFT hyperledger in RUST Security This issue asks for improved security
Projects
None yet
Development

No branches or pull requests

1 participant