Skip to content
This repository has been archived by the owner on Apr 17, 2019. It is now read-only.

Add oneof case bound check #1502

Merged
merged 7 commits into from
Jul 3, 2018
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions shared_model/validators/query_validator.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

#include <boost/variant/static_visitor.hpp>

#include "interfaces/queries/query.hpp"
#include "backend/protobuf/queries/proto_query.hpp"
#include "validators/answer.hpp"

namespace shared_model {
Expand Down Expand Up @@ -174,9 +174,20 @@ namespace shared_model {
answer.addReason(std::move(qry_reason));
}

auto reason = boost::apply_visitor(query_field_validator_, qry.get());
if (not reason.second.empty()) {
auto qry_case = static_cast<const shared_model::proto::Query &>(qry)
.getTransport()
.payload()
.query_case();
if (iroha::protocol::Query_Payload::QUERY_NOT_SET == qry_case) {
ReasonsGroupType reason;
reason.first = "Undefined";
reason.second.push_back("query is undefined");
answer.addReason(std::move(reason));
} else {
auto reason = boost::apply_visitor(query_field_validator_, qry.get());
if (not reason.second.empty()) {
answer.addReason(std::move(reason));
}
}

return answer;
Expand Down
12 changes: 12 additions & 0 deletions shared_model/validators/transaction_validator.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include <boost/format.hpp>
#include <boost/variant/static_visitor.hpp>

#include "backend/protobuf/commands/proto_command.hpp"
#include "backend/protobuf/permissions.hpp"
#include "backend/protobuf/transaction.hpp"
#include "validators/answer.hpp"
Expand Down Expand Up @@ -273,6 +274,17 @@ namespace shared_model {
}

for (const auto &command : tx.commands()) {
auto cmd_case =
static_cast<const shared_model::proto::Command &>(command)
.getTransport()
.command_case();
if (iroha::protocol::Command::COMMAND_NOT_SET == cmd_case) {
ReasonsGroupType reason;
reason.first = "Undefined";
reason.second.push_back("command is undefined");
answer.addReason(std::move(reason));
continue;
}
auto reason = boost::apply_visitor(command_validator_, command.get());
if (not reason.second.empty()) {
answer.addReason(std::move(reason));
Expand Down
14 changes: 14 additions & 0 deletions test/module/shared_model/validators/query_validator_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,20 @@ TEST_F(QueryValidatorTest, StatelessValidTest) {
});
}

/**
* @given Protobuf query object with unset query
* @when validate is called
* @then there is a error returned
*/
TEST_F(QueryValidatorTest, UnsetQuery) {
iroha::protocol::Query qry;
qry.mutable_payload()->mutable_meta()->set_created_time(created_time);
qry.mutable_payload()->mutable_meta()->set_creator_account_id(account_id);
qry.mutable_payload()->mutable_meta()->set_query_counter(counter);
auto answer = query_validator.validate(proto::Query(qry));
ASSERT_TRUE(answer.hasErrors());
}

/**
* @given Protobuf query object
* @when Query has no fields set, and each query type has no fields set
Expand Down
18 changes: 15 additions & 3 deletions test/module/shared_model/validators/transaction_validator_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ class TransactionValidatorTest : public ValidatorsTest {
.getTransport();
return tx;
}
shared_model::validation::DefaultTransactionValidator transaction_validator;
};

/**
Expand All @@ -49,7 +50,6 @@ class TransactionValidatorTest : public ValidatorsTest {
TEST_F(TransactionValidatorTest, EmptyTransactionTest) {
auto tx = generateEmptyTransaction();
tx.mutable_payload()->set_created_time(created_time);
shared_model::validation::DefaultTransactionValidator transaction_validator;
auto result = proto::Transaction(iroha::protocol::Transaction(tx));
auto answer = transaction_validator.validate(result);
ASSERT_EQ(answer.getReasonsMap().size(), 1);
Expand Down Expand Up @@ -101,13 +101,26 @@ TEST_F(TransactionValidatorTest, StatelessValidTest) {
},
[] {});

shared_model::validation::DefaultTransactionValidator transaction_validator;
auto result = proto::Transaction(iroha::protocol::Transaction(tx));
auto answer = transaction_validator.validate(result);

ASSERT_FALSE(answer.hasErrors()) << answer.reason();
}

/**
* @given Protobuf transaction object with unset command
* @when validate is called
* @then there is a error returned
*/
TEST_F(TransactionValidatorTest, UnsetCommand) {
iroha::protocol::Transaction tx = generateEmptyTransaction();
tx.mutable_payload()->set_creator_account_id(account_id);
tx.mutable_payload()->set_created_time(created_time);
auto answer = transaction_validator.validate(proto::Transaction(tx));
tx.mutable_payload()->add_commands()->clear_add_asset_quantity();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussion, it seems that clear_add_asset_quantity() is redundant, since add_commands() does not set add_asset_quantity by default, and compiler won't probably optimize any calls. So clear_add_asset_quantity() call can be removed.

ASSERT_TRUE(answer.hasErrors());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Answer here is Transaction: [[Transaction should contain at least one command ]], which is not the described case.
There should be an additional call tx.mutable_payload()->add_commands();

}

/**
* @given transaction made of commands with invalid fields
* @when commands validation is invoked
Expand Down Expand Up @@ -136,7 +149,6 @@ TEST_F(TransactionValidatorTest, StatelessInvalidTest) {
},
[] {});

shared_model::validation::DefaultTransactionValidator transaction_validator;
auto result = proto::Transaction(iroha::protocol::Transaction(tx));
auto answer = transaction_validator.validate(result);

Expand Down