Merge pull request hyperledger#1145 from anikitinDSR/public/indy-1963

[INDY-1963] Make auth error messages more cleanly

indy_common/authorize/auth_constraints.py

@@ -1,6 +1,7 @@
 from abc import ABCMeta, abstractmethod
 from typing import List
 
+from indy_common.authorize.helper import get_named_role
 
 ROLE_CONSTRAINT_ID = 'ROLE'
 AND_CONSTRAINT_ID = 'AND'
@@ -11,6 +12,9 @@ class AbstractAuthConstraint(metaclass=ABCMeta):
     def __init__(self):
         self.constraint_id = ''
 
+    def __str__(self):
+        return str(self)
+
 
 class AuthConstraint(AbstractAuthConstraint):
     def __init__(self, role, sig_count, need_to_be_owner=False, metadata={}):
@@ -20,18 +24,44 @@ def __init__(self, role, sig_count, need_to_be_owner=False, metadata={}):
         self.metadata = metadata
         self.constraint_id = ROLE_CONSTRAINT_ID
 
+    def __str__(self):
+        role = get_named_role(self.role) if self.role != '*' else 'ALL'
+        if role != 'ALL' and self.need_to_be_owner and self.sig_count > 1:
+            return "{} {} signatures are required and needs to be owner".format(self.sig_count, role)
+        elif role != 'ALL' and not self.need_to_be_owner and self.sig_count > 1:
+            return "{} {} signatures are required".format(self.sig_count, role)
+        elif role != 'ALL' and not self.need_to_be_owner and self.sig_count == 1:
+            return "1 {} signature is required".format(role)
+        elif role != 'ALL' and self.need_to_be_owner and self.sig_count == 1:
+            return "1 {} signature is required and needs to be owner".format(role)
+
+        elif role == "ALL" and self.need_to_be_owner and self.sig_count == 1:
+            return "1 signature of any role is required and needs to be owner"
+        elif role == 'ALL' and not self.need_to_be_owner and self.sig_count == 1:
+            return "1 signature of any role is required".format(role)
+        elif role == 'ALL' and not self.need_to_be_owner and self.sig_count > 1:
+            return "{} signatures of any role are required".format(self.sig_count)
+        elif role == "ALL" and self.need_to_be_owner and self.sig_count > 1:
+            return "{} signatures of any role are required and needs to be owner".format(self.sig_count)
+
 
 class AuthConstraintAnd(AbstractAuthConstraint):
     def __init__(self, auth_constraints):
         self.auth_constraints = auth_constraints
         self.constraint_id = AND_CONSTRAINT_ID
 
+    def __str__(self):
+        return " AND ".join([str(ac) for ac in self.auth_constraints])
+
 
 class AuthConstraintOr(AbstractAuthConstraint):
     def __init__(self, auth_constraints):
         self.auth_constraints = auth_constraints
         self.constraint_id = OR_CONSTRAINT_ID
 
+    def __str__(self):
+        return " OR ".join([str(ac) for ac in self.auth_constraints])
+
 
 class AbstractAuthConstraintParser(metaclass=ABCMeta):
     @staticmethod

indy_common/authorize/authorizer.py

@@ -3,6 +3,9 @@
 from indy_common.authorize.auth_actions import AbstractAuthAction
 from indy_common.authorize.auth_constraints import AbstractAuthConstraint, AuthConstraint, ROLE_CONSTRAINT_ID, \
     AuthConstraintAnd
+from indy_common.authorize.helper import get_named_role
+from indy_common.constants import NYM, CLAIM_DEF
+from indy_common.transactions import IndyTransactions
 from indy_common.types import Request
 from indy_node.persistence.idr_cache import IdrCache
 
@@ -65,6 +68,9 @@ def is_sig_count_accepted(self, request: Request, auth_constraint: AuthConstrain
 
         return sig_count >= auth_constraint.sig_count
 
+    def get_named_role_from_req(self, request: Request):
+        return get_named_role(self.get_role(request))
+
     def authorize(self,
                   request: Request,
                   auth_constraint: AuthConstraint,
@@ -73,11 +79,18 @@ def authorize(self,
         if is_role_accepted is None:
             return False, "sender's DID {} is not found in the Ledger".format(request.identifier)
         if not is_role_accepted:
-            return False, "role is not accepted"
+            return False, "{} can not do this action".format(self.get_named_role_from_req(request))
         if not self.is_sig_count_accepted(request, auth_constraint):
-            return False, "count of signatures is not accepted"
+            return False, "Not enough signatures"
         if not self.is_owner_accepted(auth_constraint, auth_action):
-            return False, "actor must be owner"
+            if auth_action.field != '*':
+                return False, "{} can not touch {} field since only the owner can modify it".\
+                    format(self.get_named_role_from_req(request),
+                           auth_action.field)
+            else:
+                return False, "{} can not edit {} txn since only owner can modify it".\
+                    format(self.get_named_role_from_req(request),
+                           IndyTransactions.get_name_from_code(auth_action.txn_type))
         return True, ""
 
 
@@ -135,5 +148,5 @@ def authorize(self,
             else:
                 successes.append(True)
         if len(successes) == 0:
-            raise AuthValidationError("There is no accepted constraint")
+            raise AuthValidationError("Rule for this action is: {}".format(auth_constraint))
         return True, ""

indy_common/authorize/helper.py

@@ -0,0 +1,8 @@
+from indy_common.roles import Roles
+
+
+def get_named_role(role_code):
+    try:
+        return Roles.nameFromValue(role_code)
+    except ValueError:
+        return "Unknown role"

indy_common/constants.py

@@ -149,6 +149,7 @@
 GET_REVOC_REG_DEF = IndyTransactions.GET_REVOC_REG_DEF.value
 GET_REVOC_REG = IndyTransactions.GET_REVOC_REG.value
 GET_REVOC_REG_DELTA = IndyTransactions.GET_REVOC_REG_DELTA.value
+CHANGE_KEY = IndyTransactions.CHANGE_KEY.value
 
 POOL_UPGRADE = IndyTransactions.POOL_UPGRADE.value
 NODE_UPGRADE = IndyTransactions.NODE_UPGRADE.value

indy_common/test/auth/test_auth_constraint.py

@@ -0,0 +1,82 @@
+from indy_common.authorize.auth_constraints import AuthConstraint, AuthConstraintOr, AuthConstraintAnd
+from plenum.common.constants import TRUSTEE, STEWARD
+
+
+def test_str_not_any_7_sig_owner():
+    constraint = AuthConstraint(role=TRUSTEE,
+                                sig_count=7,
+                                need_to_be_owner=True)
+    assert str(constraint) == '7 TRUSTEE signatures are required and needs to be owner'
+
+
+def test_str_not_any_7_sig_not_owner():
+    constraint = AuthConstraint(role=TRUSTEE,
+                                sig_count=7,
+                                need_to_be_owner=False)
+    assert str(constraint) == '7 TRUSTEE signatures are required'
+
+
+def test_str_not_any_1_sig_not_owner():
+    constraint = AuthConstraint(role=TRUSTEE,
+                                sig_count=1,
+                                need_to_be_owner=False)
+    assert str(constraint) == '1 TRUSTEE signature is required'
+
+
+def test_str_not_any_1_sig_owner():
+    constraint = AuthConstraint(role=TRUSTEE,
+                                sig_count=1,
+                                need_to_be_owner=True)
+    assert str(constraint) == '1 TRUSTEE signature is required and needs to be owner'
+
+
+def test_str_any_1_sig_owner():
+    constraint = AuthConstraint(role="*",
+                                sig_count=1,
+                                need_to_be_owner=True)
+    assert str(constraint) == '1 signature of any role is required and needs to be owner'
+
+
+def test_str_any_1_sig_not_owner():
+    constraint = AuthConstraint(role='*',
+                                sig_count=1,
+                                need_to_be_owner=False)
+    assert str(constraint) == '1 signature of any role is required'
+
+
+def test_str_any_several_sig_not_owner():
+    constraint = AuthConstraint(role='*',
+                                sig_count=7,
+                                need_to_be_owner=False)
+    assert str(constraint) == '7 signatures of any role are required'
+
+
+def test_str_any_several_sig_owner():
+    constraint = AuthConstraint(role='*',
+                                sig_count=7,
+                                need_to_be_owner=True)
+    assert str(constraint) == '7 signatures of any role are required and needs to be owner'
+
+
+def test_str_for_auth_constraint_or():
+    constraint = AuthConstraintOr([AuthConstraint(role=TRUSTEE,
+                                                  sig_count=1,
+                                                  need_to_be_owner=True),
+                                   AuthConstraint(role=STEWARD,
+                                                  sig_count=1,
+                                                  need_to_be_owner=True)])
+    assert str(constraint) == '1 TRUSTEE signature is required and needs to be owner ' \
+                              'OR ' \
+                              '1 STEWARD signature is required and needs to be owner'
+
+
+def test_str_for_auth_constraint_and():
+    constraint = AuthConstraintAnd([AuthConstraint(role=TRUSTEE,
+                                                   sig_count=1,
+                                                   need_to_be_owner=True),
+                                    AuthConstraint(role=STEWARD,
+                                                   sig_count=1,
+                                                   need_to_be_owner=True)])
+    assert str(constraint) == '1 TRUSTEE signature is required and needs to be owner ' \
+                              'AND ' \
+                              '1 STEWARD signature is required and needs to be owner'

indy_common/test/auth/test_auth_nym_with_new_auth_map.py

@@ -134,3 +134,13 @@ def test_same_role_network_monitor(write_request_validation, req, is_owner):
                                                                   old_value=NETWORK_MONITOR,
                                                                   new_value=NETWORK_MONITOR,
                                                                   is_owner=is_owner)])
+
+
+def test_same_role_none(write_request_validation, req, is_owner):
+    authorized = is_owner
+    assert authorized == write_request_validation(req,
+                                                  [AuthActionEdit(txn_type=NYM,
+                                                                  field=ROLE,
+                                                                  old_value='',
+                                                                  new_value='',
+                                                                  is_owner=is_owner)])

indy_common/test/auth/test_helper.py

@@ -0,0 +1,10 @@
+from indy_common.authorize.helper import get_named_role
+from plenum.common.constants import TRUSTEE_STRING, TRUSTEE
+
+
+def test_for_known_role():
+    assert get_named_role(TRUSTEE) == TRUSTEE_STRING
+
+
+def test_for_unknown_role():
+    assert get_named_role("SomeOtherRole") == "Unknown role"

indy_common/test/auth/test_role_authorizer.py

@@ -88,7 +88,7 @@ def test_role_authorizer_not_authorize_role(idr_cache, req_auth):
     authorizer = RolesAuthorizer(cache=idr_cache)
     authorized, reason = authorizer.authorize(req_auth, AuthConstraint(role="SomeOtherRole", sig_count=1))
     assert not authorized
-    assert reason == "role is not accepted"
+    assert reason == "Unknown role can not do this action"
 
 
 def test_role_authorizer_not_authorize_unknown_nym(idr_cache):

indy_common/test/test_transactions.py

@@ -1,5 +1,6 @@
 from indy_common.constants import NYM, NODE, ATTRIB, SCHEMA, CLAIM_DEF, DISCLO, GET_ATTR, GET_NYM, GET_TXNS, \
-    GET_SCHEMA, GET_CLAIM_DEF, POOL_UPGRADE, NODE_UPGRADE, POOL_CONFIG
+    GET_SCHEMA, GET_CLAIM_DEF, POOL_UPGRADE, NODE_UPGRADE, POOL_CONFIG, REVOC_REG_DEF, REVOC_REG_ENTRY, \
+    GET_REVOC_REG_DEF, GET_REVOC_REG, GET_REVOC_REG_DELTA, POOL_RESTART, VALIDATOR_INFO, CHANGE_KEY
 from indy_common.transactions import IndyTransactions
 
 
@@ -18,6 +19,16 @@ def testTransactionsAreEncoded():
     assert POOL_UPGRADE == "109"
     assert NODE_UPGRADE == "110"
     assert POOL_CONFIG == "111"
+    assert CHANGE_KEY == "112"
+
+    assert REVOC_REG_DEF == "113"
+    assert REVOC_REG_ENTRY == "114"
+    assert GET_REVOC_REG_DEF == "115"
+    assert GET_REVOC_REG == "116"
+    assert GET_REVOC_REG_DELTA == "117"
+
+    assert POOL_RESTART == "118"
+    assert VALIDATOR_INFO == "119"
 
 
 def testTransactionEnumDecoded():
@@ -29,15 +40,26 @@ def testTransactionEnumDecoded():
     assert IndyTransactions.CLAIM_DEF.name == "CLAIM_DEF"
 
     assert IndyTransactions.DISCLO.name == "DISCLO"
+
     assert IndyTransactions.GET_ATTR.name == "GET_ATTR"
     assert IndyTransactions.GET_NYM.name == "GET_NYM"
     assert IndyTransactions.GET_TXNS.name == "GET_TXNS"
     assert IndyTransactions.GET_SCHEMA.name == "GET_SCHEMA"
     assert IndyTransactions.GET_CLAIM_DEF.name == "GET_CLAIM_DEF"
+
     assert IndyTransactions.POOL_UPGRADE.name == "POOL_UPGRADE"
     assert IndyTransactions.NODE_UPGRADE.name == "NODE_UPGRADE"
     assert IndyTransactions.POOL_CONFIG.name == "POOL_CONFIG"
     assert IndyTransactions.POOL_RESTART.name == "POOL_RESTART"
+    assert IndyTransactions.CHANGE_KEY.name == "CHANGE_KEY"
+
+    assert IndyTransactions.REVOC_REG_DEF.name == "REVOC_REG_DEF"
+    assert IndyTransactions.REVOC_REG_ENTRY.name == "REVOC_REG_ENTRY"
+    assert IndyTransactions.GET_REVOC_REG_DEF.name == "GET_REVOC_REG_DEF"
+    assert IndyTransactions.GET_REVOC_REG.name == "GET_REVOC_REG"
+    assert IndyTransactions.GET_REVOC_REG_DELTA.name == "GET_REVOC_REG_DELTA"
+
+    assert IndyTransactions.VALIDATOR_INFO.name == "VALIDATOR_INFO"
 
 
 def testTransactionEnumEncoded():
@@ -57,4 +79,41 @@ def testTransactionEnumEncoded():
     assert IndyTransactions.POOL_UPGRADE.value == "109"
     assert IndyTransactions.NODE_UPGRADE.value == "110"
     assert IndyTransactions.POOL_CONFIG.value == "111"
+    assert IndyTransactions.CHANGE_KEY.value == "112"
+    assert IndyTransactions.REVOC_REG_DEF.value == "113"
+    assert IndyTransactions.REVOC_REG_ENTRY.value == "114"
+    assert IndyTransactions.GET_REVOC_REG_DEF.value == "115"
+    assert IndyTransactions.GET_REVOC_REG.value == "116"
+    assert IndyTransactions.GET_REVOC_REG_DELTA.value == "117"
     assert IndyTransactions.POOL_RESTART.value == "118"
+    assert IndyTransactions.VALIDATOR_INFO.value == "119"
+
+
+def test_get_name_from_code():
+    assert IndyTransactions.get_name_from_code(IndyTransactions.NODE.value) == "NODE"
+    assert IndyTransactions.get_name_from_code(IndyTransactions.NYM.value) == "NYM"
+
+    assert IndyTransactions.get_name_from_code(IndyTransactions.ATTRIB.value) == "ATTRIB"
+    assert IndyTransactions.get_name_from_code(IndyTransactions.SCHEMA.value) == "SCHEMA"
+    assert IndyTransactions.get_name_from_code(IndyTransactions.CLAIM_DEF.value) == "CLAIM_DEF"
+
+    assert IndyTransactions.get_name_from_code(IndyTransactions.DISCLO.value) == "DISCLO"
+    assert IndyTransactions.get_name_from_code(IndyTransactions.GET_ATTR.value) == "GET_ATTR"
+    assert IndyTransactions.get_name_from_code(IndyTransactions.GET_NYM.value) == "GET_NYM"
+    assert IndyTransactions.get_name_from_code(IndyTransactions.GET_TXNS.value) == "GET_TXNS"
+    assert IndyTransactions.get_name_from_code(IndyTransactions.GET_SCHEMA.value) == "GET_SCHEMA"
+    assert IndyTransactions.get_name_from_code(IndyTransactions.GET_CLAIM_DEF.value) == "GET_CLAIM_DEF"
+    assert IndyTransactions.get_name_from_code(IndyTransactions.POOL_UPGRADE.value) == "POOL_UPGRADE"
+    assert IndyTransactions.get_name_from_code(IndyTransactions.NODE_UPGRADE.value) == "NODE_UPGRADE"
+    assert IndyTransactions.get_name_from_code(IndyTransactions.POOL_CONFIG.value) == "POOL_CONFIG"
+    assert IndyTransactions.get_name_from_code(IndyTransactions.POOL_RESTART.value) == "POOL_RESTART"
+
+    assert IndyTransactions.get_name_from_code(IndyTransactions.CHANGE_KEY.value) == "CHANGE_KEY"
+    assert IndyTransactions.get_name_from_code(IndyTransactions.REVOC_REG_DEF.value) == "REVOC_REG_DEF"
+    assert IndyTransactions.get_name_from_code(IndyTransactions.REVOC_REG_ENTRY.value) == "REVOC_REG_ENTRY"
+    assert IndyTransactions.get_name_from_code(IndyTransactions.GET_REVOC_REG_DEF.value) == "GET_REVOC_REG_DEF"
+    assert IndyTransactions.get_name_from_code(IndyTransactions.GET_REVOC_REG.value) == "GET_REVOC_REG"
+    assert IndyTransactions.get_name_from_code(IndyTransactions.GET_REVOC_REG_DELTA.value) == "GET_REVOC_REG_DELTA"
+    assert IndyTransactions.get_name_from_code(IndyTransactions.VALIDATOR_INFO.value) == "VALIDATOR_INFO"
+
+    assert IndyTransactions.get_name_from_code("some_unexpected_code") == "Unknown_transaction_type"

indy_common/transactions.py

@@ -33,3 +33,10 @@ class IndyTransactions(Transactions):
 
     POOL_RESTART = "118"
     VALIDATOR_INFO = "119"
+
+    @staticmethod
+    def get_name_from_code(code: str):
+        try:
+            return IndyTransactions(code).name
+        except ValueError:
+            return "Unknown_transaction_type"

indy_node/test/nym_txn/test_nym_additional.py

@@ -36,24 +36,31 @@ def trust_anchor_did_verkey(looper, sdk_wallet_client):
 def test_pool_nodes_started(nodeSet):
     pass
 
+@pytest.fixture(scope='function', params=['trustee', 'steward'])
+def sdk_wallet(request, sdk_wallet_steward, sdk_wallet_trustee):
+    if request.param == 'steward':
+        yield sdk_wallet_steward
+    elif request.param == 'trustee':
+        yield sdk_wallet_trustee
+
 
 def test_send_same_nyms_only_first_gets_written(
-        looper, sdk_pool_handle, sdk_wallet_steward):
-    wh, _ = sdk_wallet_steward
+        looper, sdk_pool_handle, sdk_wallet):
+    wh, _ = sdk_wallet
     seed = randomString(32)
     did, verkey = looper.loop.run_until_complete(
         create_and_store_my_did(wh, json.dumps({'seed': seed})))
 
     # request 1
-    _, did1 = sdk_add_new_nym(looper, sdk_pool_handle, sdk_wallet_steward, dest=did, verkey=verkey)
+    _, did1 = sdk_add_new_nym(looper, sdk_pool_handle, sdk_wallet, dest=did, verkey=verkey)
 
     seed = randomString(32)
     _, verkey = looper.loop.run_until_complete(
         create_and_store_my_did(wh, json.dumps({'seed': seed})))
     # request 2
     with pytest.raises(RequestRejectedException) as e:
-        _, did2 = sdk_add_new_nym(looper, sdk_pool_handle, sdk_wallet_steward, dest=did, verkey=verkey)
-    e.match('actor must be owner')
+        _, did2 = sdk_add_new_nym(looper, sdk_pool_handle, sdk_wallet, dest=did, verkey=verkey)
+    e.match('can not touch verkey field since only the owner can modify it')
 
 
 def get_nym(looper, sdk_pool_handle, sdk_wallet_steward, t_did):