Feature/permissions are checked in sql executors #1605

vdrobnyi · 2018-07-26T13:54:25Z

Description of the Change

Validation and permissions checks are done in sql executors

Benefits

Less db communication, better performance

nickaleks · 2018-07-30T08:12:52Z

irohad/ametsuchi/impl/postgres_block_query.hpp

@@ -82,6 +82,7 @@ namespace iroha {
      std::function<void(std::vector<std::string> &result)> callback(
          const rxcpp::subscriber<wTransaction> &s, uint64_t block_id);

+      std::unique_ptr<soci::session> sql_ptr_;


is this needed?

nickaleks · 2018-07-30T08:16:57Z

irohad/ametsuchi/impl/postgres_command_executor.cpp


      std::vector<std::function<std::string()>> message_gen = {
+          [&] {
+            return std::string(


do we need std::string constructor here?

nickaleks · 2018-07-30T08:19:21Z

irohad/ametsuchi/impl/postgres_wsv_query.hpp

@@ -87,6 +87,7 @@ namespace iroha {
          shared_model::interface::permissions::Grantable permission) override;

     private:
+      std::unique_ptr<soci::session> sql_ptr_;


Is this needed?

Signed-off-by: Victor Drobny <vdrobny@yandex.ru>

nickaleks · 2018-08-01T07:21:06Z

irohad/ametsuchi/impl/postgres_command_executor.cpp

+    const auto bits = shared_model::interface::GrantablePermissionSet::size();
+    std::string query = R"(
+          SELECT COALESCE(bit_or(permission), '0'::bit()"
+        + std::to_string(bits) + R"()) & ')" + std::string(perm_str)


Can we use boost::format here?

nickaleks · 2018-08-01T07:42:09Z

irohad/ametsuchi/impl/temporary_wsv_impl.cpp

@@ -18,7 +18,6 @@ namespace iroha {
          wsv_(std::make_shared<PostgresWsvQuery>(*sql_, factory)),
          executor_(std::make_shared<PostgresWsvCommand>(*sql_)),
          command_executor_(std::make_shared<PostgresCommandExecutor>(*sql_)),


Maybe make unique?

nickaleks · 2018-08-01T07:43:21Z

test/module/irohad/ametsuchi/postgres_executor_test.cpp

+    }
+
+    /**
+     * @given  command ]


nickaleks · 2018-08-01T07:43:37Z

test/module/irohad/ametsuchi/postgres_executor_test.cpp

      ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().createAsset(
          "coin", domain->domainId(), 1)))));
      auto ass = query->getAsset(asset->assetId());
      ASSERT_TRUE(ass);
      ASSERT_EQ(*asset.get(), *ass.get());
    }

+    /**
+     * @given  command ]


Signed-off-by: Victor Drobny <vdrobny@yandex.ru>

lebdron · 2018-08-02T13:17:34Z

irohad/ametsuchi/impl/postgres_command_executor.cpp

+          & '%s' = '%s' FROM role_has_permissions AS rp
+              JOIN account_has_roles AS ar on ar.role_id = rp.role_id
+              WHERE ar.account_id = :%s)")
+                         % bits % perm_str % perm_str % account_alias)


%1%, %2% can be used for positional arguments, so that there is no need to write perm_str twice.

lebdron · 2018-08-03T06:07:30Z

irohad/ametsuchi/impl/postgres_command_executor.cpp

+  std::string checkAccountHasRoleOrGrantablePerm(
+      shared_model::interface::permissions::Role role,
+      shared_model::interface::permissions::Grantable grantable) {
+    return R"(WITH


This also looks like a candidate for boost::format, to reduce the number of raw strings.

lebdron · 2018-08-03T06:37:08Z

irohad/ametsuchi/impl/postgres_command_executor.cpp

@@ -122,7 +151,12 @@ namespace iroha {
                               WHERE account_id = :account_id LIMIT 1),
               has_asset AS (SELECT asset_id FROM asset
                             WHERE asset_id = :asset_id AND
-                             precision >= :precision LIMIT 1),
+                             precision >= :precision LIMIT 1),)" +
+               (do_validation_ ? R"(has_perm AS ()"


How about introducing boost::format to all commands, and doing something like:

boost::format cmd("..."); if (do_validation_) cmd % "has_perm_..."; else cmd % "";

It might be easier to read without ternary operators.

lebdron · 2018-08-07T11:27:06Z

irohad/ametsuchi/impl/postgres_wsv_query.cpp

@@ -124,6 +124,9 @@ namespace iroha {
      processSoci(st, ind, row, [&set](std::string &row) {
        set = shared_model::interface::RolePermissionSet(row);
      });
+      if (set.none()) {


This does not look valid, because empty roles are allowed, and boost::none means db error here. Consider refactoring.

Isn't it IR-1480?

It looks like the task is outdated 😔 Since roles with no permissions are allowed at the moment, boost::none can be returned only as a result of database error.

lebdron · 2018-08-07T11:29:50Z

test/module/irohad/ametsuchi/postgres_executor_test.cpp

@@ -53,8 +53,10 @@ namespace iroha {

      CommandResult execute(
          const std::unique_ptr<shared_model::interface::Command> &command,
+          bool is_genesis = false,


Not sure if genesis block should be mentioned here. Maybe use do_validation?

nickaleks · 2018-08-07T13:17:39Z

irohad/ametsuchi/impl/postgres_command_executor.cpp

-                % peer.pubkey().hex() % peer.address())
-            .str();
+      soci::statement st = sql_.prepare << R"(
+          WITH


Please use boost::format

Signed-off-by: Victor Drobny <vdrobny@yandex.ru>

sorabot · 2018-08-14T12:41:24Z

SonarQube analysis reported 2 issues

postgres_executor_test.cpp#L98: The class 'RemoveSignatory' defines member variable with name 'pubkey' also defined in its parent class 'CommandExecutorTest'.
postgres_executor_test.cpp#L1408: The function 'checkTransfer' is never used.

Validation and permissions checks are done in sql executors Signed-off-by: Victor Drobny <vdrobny@yandex.ru>

nickaleks suggested changes Jul 30, 2018

View reviewed changes

vdrobnyi added 9 commits July 30, 2018 12:40

fix pr issues

f05788a

Signed-off-by: Victor Drobny <vdrobny@yandex.ru>

wip

fc7b5ba

Signed-off-by: Victor Drobny <vdrobny@yandex.ru>

fix tests

373ad4b

Signed-off-by: Victor Drobny <vdrobny@yandex.ru>

remove isValid from command executor

31d9025

Signed-off-by: Victor Drobny <vdrobny@yandex.ru>

remove useless method

f0d1f53

Signed-off-by: Victor Drobny <vdrobny@yandex.ru>

checks perms in sql executors

d629844

Signed-off-by: Victor Drobny <vdrobny@yandex.ru>

fix pr issues

9e79dea

Signed-off-by: Victor Drobny <vdrobny@yandex.ru>

fix issues

69f924f

Signed-off-by: Victor Drobny <vdrobny@yandex.ru>

remove command executor from irohad/execution

25780ce

Signed-off-by: Victor Drobny <vdrobny@yandex.ru>

vdrobnyi force-pushed the feature/sql-permissions branch from cc58ac5 to 25780ce Compare July 30, 2018 09:52

vdrobnyi added 3 commits July 30, 2018 13:08

fix pr issues

5339bf0

Signed-off-by: Victor Drobny <vdrobny@yandex.ru>

refactor has_permission with role and grantable

d3a5504

Signed-off-by: Victor Drobny <vdrobny@yandex.ru>

merge develop

29d99d1

Signed-off-by: Victor Drobny <vdrobny@yandex.ru>

nickaleks suggested changes Aug 1, 2018

View reviewed changes

vdrobnyi requested a review from lebdron August 1, 2018 10:22

fix pr issues

0793884

Signed-off-by: Victor Drobny <vdrobny@yandex.ru>

lebdron suggested changes Aug 7, 2018

View reviewed changes

nickaleks suggested changes Aug 7, 2018

View reviewed changes

fix pr issues

4dc9e63

Signed-off-by: Victor Drobny <vdrobny@yandex.ru>

nickaleks approved these changes Aug 10, 2018

View reviewed changes

fix getRolePermissions

5f7b920

Signed-off-by: Victor Drobny <vdrobny@yandex.ru>

lebdron approved these changes Aug 13, 2018

View reviewed changes

vdrobnyi added 2 commits August 13, 2018 18:47

Merge branch 'develop' into feature/sql-permissions

de09679

Merge branch 'develop' into feature/sql-permissions

6c856b1

vdrobnyi merged commit 1fc88bf into develop Aug 14, 2018

vdrobnyi deleted the feature/sql-permissions branch August 14, 2018 13:41

kamilsa mentioned this pull request Nov 1, 2018

Hyperledger Iroha v1.0 beta-5 version #1825

Closed

bakhtin pushed a commit that referenced this pull request Nov 2, 2018

Feature/permissions are checked in sql executors (#1605)

509067a

Validation and permissions checks are done in sql executors Signed-off-by: Victor Drobny <vdrobny@yandex.ru>

neewy mentioned this pull request Nov 2, 2018

Hyperledger Iroha v1.0 beta-5 version #1832

Closed

kamilsa mentioned this pull request Dec 12, 2018

Hyperledger Iroha v1.0 Release Candidate 1 #1941

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/permissions are checked in sql executors #1605

Feature/permissions are checked in sql executors #1605

vdrobnyi commented Jul 26, 2018 •

edited

Loading

nickaleks Jul 30, 2018

nickaleks Jul 30, 2018

nickaleks Jul 30, 2018

nickaleks Aug 1, 2018

nickaleks Aug 1, 2018

nickaleks Aug 1, 2018

nickaleks Aug 1, 2018

lebdron Aug 2, 2018

lebdron Aug 3, 2018

lebdron Aug 3, 2018

lebdron Aug 7, 2018

vdrobnyi Aug 8, 2018

lebdron Aug 13, 2018

lebdron Aug 7, 2018

nickaleks Aug 7, 2018

sorabot commented Aug 14, 2018

Feature/permissions are checked in sql executors #1605

Feature/permissions are checked in sql executors #1605

Conversation

vdrobnyi commented Jul 26, 2018 • edited Loading

Description of the Change

Benefits

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorabot commented Aug 14, 2018

vdrobnyi commented Jul 26, 2018 •

edited

Loading