Merge pull request #4634 from userlocalhost/bugfix/st2kv_jinja

Fixed a problem not to decrypt user scope data store value in Jinja expression

CHANGELOG.rst

@@ -93,6 +93,9 @@ Changed
 * Add missing ``--user`` argument to ``st2 execution list`` CLI command. (improvement) #4632
 
   Contributed by Tristan Struthers (@trstruth).
+* Update ``decrypt_kv`` Jinja template filter so it to throws a more user-friendly error message
+  when decryption fails because the variable references a datastore value which doesn't exist.
+  (improvement) #4634
 
 Fixed
 ~~~~~
@@ -115,6 +118,12 @@ Fixed
   command and script actions which use sudo. (bug fix) #4623
 * Update service bootstrap and ``st2-register-content`` script code so non-fatal errors are
   suppressed by default and only logged under ``DEBUG`` log level. (bug fix) #3933 #4626 #4630
+* Fix a bug with not being able to decrypt user-scoped datastore values inside Jinja expressions
+  using ``decrypt_kv`` Jinja filter. (bug fix) #4634
+
+  Contributed by Hiroyasu OHYAMA (@userlocalhost).
+* Fix a bug with user-scoped datastore values not working inside action-chain workflows. (bug fix)
+  #4634
 
 2.10.4 - March 15, 2019
 -----------------------

contrib/examples/actions/chains/decrypt_kv_jinja_filter.yaml

@@ -1,9 +1,16 @@
 ---
 vars:
-  encrypted_cmd: "{{ st2kv.system.my_cmd | decrypt_kv }}"
+  encrypted_cmd_system_scope: "{{ st2kv.system.my_cmd | decrypt_kv }}"
+  encrypted_cmd_user_scope: "{{ st2kv.user.my_cmd | decrypt_kv }}"
 chain:
   -
     name: "c1"
     ref: "core.local"
     parameters:
-      cmd: 'echo {{encrypted_cmd}}'
+      cmd: 'echo {{encrypted_cmd_system_scope}}'
+    on-success: "c2"
+  -
+    name: "c2"
+    ref: "core.local"
+    parameters:
+      cmd: 'echo {{encrypted_cmd_user_scope}}'

contrib/runners/action_chain_runner/action_chain_runner/action_chain_runner.py

@@ -22,6 +22,7 @@
 
 import six
 from jsonschema import exceptions as json_schema_exc
+from oslo_config import cfg
 
 from st2common.runners.base import ActionRunner
 from st2common.runners.base import get_metadata as get_runner_metadata
@@ -83,10 +84,11 @@ def __init__(self, chainspec, chainname):
 
         self.vars = {}
 
-    def init_vars(self, action_parameters):
+    def init_vars(self, action_parameters, action_context=None):
         if self.actionchain.vars:
             self.vars = self._get_rendered_vars(self.actionchain.vars,
-                                                action_parameters=action_parameters)
+                                                action_parameters=action_parameters,
+                                                action_context=action_context)
 
     def restore_vars(self, ctx_vars):
         self.vars.update(fast_deepcopy(ctx_vars))
@@ -203,14 +205,20 @@ def _is_valid_node_name(self, all_node_names, node_name):
         return node_name in all_node_names
 
     @staticmethod
-    def _get_rendered_vars(vars, action_parameters):
+    def _get_rendered_vars(vars, action_parameters, action_context):
         if not vars:
             return {}
+
+        action_context = action_context or {}
+        user = action_context.get('user', cfg.CONF.system_user.user)
+
         context = {}
         context.update({
             kv_constants.DATASTORE_PARENT_SCOPE: {
                 kv_constants.SYSTEM_SCOPE: kv_service.KeyValueLookup(
-                    scope=kv_constants.FULL_SYSTEM_SCOPE)
+                    scope=kv_constants.FULL_SYSTEM_SCOPE),
+                kv_constants.USER_SCOPE: kv_service.UserKeyValueLookup(
+                    scope=kv_constants.FULL_USER_SCOPE, user=user)
             }
         })
         context.update(action_parameters)
@@ -376,7 +384,8 @@ def _run_chain(self, action_parameters, resuming=False):
         try:
             # Initialize vars with the action parameters.
             # This allows action parameers to be referenced from vars.
-            self.chain_holder.init_vars(action_parameters)
+            self.chain_holder.init_vars(action_parameters=action_parameters,
+                                        action_context=self.context)
         except Exception as e:
             chain_status = action_constants.LIVEACTION_STATUS_FAILED
             m = 'Failed initializing ``vars`` in chain.'

st2common/st2common/expressions/functions/datastore.py

@@ -14,22 +14,39 @@
 # limitations under the License.
 
 from __future__ import absolute_import
+
 from oslo_config import cfg
 
 from st2common.services.keyvalues import KeyValueLookup
-from st2common.util.crypto import read_crypto_key, symmetric_decrypt
+from st2common.services.keyvalues import UserKeyValueLookup
+from st2common.util.crypto import read_crypto_key
+from st2common.util.crypto import symmetric_decrypt
 
 __all__ = [
     'decrypt_kv'
 ]
 
 
 def decrypt_kv(value):
-    if isinstance(value, KeyValueLookup):
+    original_value = value
+
+    if isinstance(value, KeyValueLookup) or isinstance(value, UserKeyValueLookup):
         # Since this is a filter the incoming value is still a KeyValueLookup
         # object as the jinja rendering is not yet complete. So we cast
         # the KeyValueLookup object to a simple string before decrypting.
+        is_kv_item = True
         value = str(value)
+    else:
+        is_kv_item = False
+
+    # NOTE: If value is None this indicate key value item doesn't exist and we hrow a more
+    # user-friendly error
+    if is_kv_item and value == '':
+        # Build original key name
+        key_name = original_value.get_key_name()
+        raise ValueError('Referenced datastore item "%s" doesn\'t exist or it contains an empty '
+                         'string' % (key_name))
+
     crypto_key_path = cfg.CONF.keyvalue.encryption_key_path
     crypto_key = read_crypto_key(key_path=crypto_key_path)
     return symmetric_decrypt(decrypt_key=crypto_key, ciphertext=value)

st2common/st2common/services/keyvalues.py

@@ -14,8 +14,10 @@
 # limitations under the License.
 
 from __future__ import absolute_import
+
 from st2common import log as logging
 
+from st2common.constants.keyvalue import DATASTORE_PARENT_SCOPE
 from st2common.constants.keyvalue import SYSTEM_SCOPE, FULL_SYSTEM_SCOPE
 from st2common.constants.keyvalue import USER_SCOPE, FULL_USER_SCOPE
 from st2common.constants.keyvalue import ALLOWED_SCOPES
@@ -66,7 +68,35 @@ def get_values_for_names(names, default_value=None):
     return result
 
 
-class KeyValueLookup(object):
+class BaseKeyValueLookup(object):
+
+    scope = None
+    _key_prefix = None
+
+    def get_key_name(self):
+        """
+        Function which returns an original key name.
+
+        :rtype: ``str``
+        """
+        key_name_parts = [DATASTORE_PARENT_SCOPE, self.scope]
+        key_name = self._key_prefix.split(':', 1)
+
+        if len(key_name) == 1:
+            key_name = key_name[0]
+        elif len(key_name) >= 2:
+            key_name = key_name[1]
+        else:
+            key_name = ''
+
+        key_name_parts.append(key_name)
+        key_name = '.'.join(key_name_parts)
+        return key_name
+
+
+class KeyValueLookup(BaseKeyValueLookup):
+
+    scope = SYSTEM_SCOPE
 
     def __init__(self, prefix=None, key_prefix=None, cache=None, scope=FULL_SYSTEM_SCOPE):
         if not scope:
@@ -125,7 +155,9 @@ def _get_kv(self, key):
         return kvp.value if kvp else ''
 
 
-class UserKeyValueLookup(object):
+class UserKeyValueLookup(BaseKeyValueLookup):
+
+    scope = USER_SCOPE
 
     def __init__(self, user, prefix=None, key_prefix=None, cache=None, scope=FULL_USER_SCOPE):
         if not scope:

st2common/tests/unit/test_jinja_render_crypto_filters.py

@@ -17,25 +17,35 @@
 from oslo_config import cfg
 
 from st2tests.base import CleanDbTestCase
-from st2common.constants.keyvalue import FULL_SYSTEM_SCOPE, SYSTEM_SCOPE, DATASTORE_PARENT_SCOPE
+from st2common.constants.keyvalue import (
+    FULL_SYSTEM_SCOPE,
+    SYSTEM_SCOPE,
+    DATASTORE_PARENT_SCOPE,
+    FULL_USER_SCOPE,
+    USER_SCOPE,
+)
 from st2common.models.db.keyvalue import KeyValuePairDB
 from st2common.persistence.keyvalue import KeyValuePair
-from st2common.services.keyvalues import KeyValueLookup
+from st2common.services.keyvalues import KeyValueLookup, UserKeyValueLookup
 from st2common.util import jinja as jinja_utils
 from st2common.util.crypto import read_crypto_key, symmetric_encrypt
 
 
 class JinjaUtilsDecryptTestCase(CleanDbTestCase):
+    def setUp(self):
+        super(JinjaUtilsDecryptTestCase, self).setUp()
 
-    def test_filter_decrypt_kv(self):
-        secret = 'Build a wall'
         crypto_key_path = cfg.CONF.keyvalue.encryption_key_path
         crypto_key = read_crypto_key(key_path=crypto_key_path)
-        secret_value = symmetric_encrypt(encrypt_key=crypto_key, plaintext=secret)
-        KeyValuePair.add_or_update(KeyValuePairDB(name='k8', value=secret_value,
+
+        self.secret = 'Build a wall'
+        self.secret_value = symmetric_encrypt(encrypt_key=crypto_key, plaintext=self.secret)
+        self.env = jinja_utils.get_jinja_environment()
+
+    def test_filter_decrypt_kv(self):
+        KeyValuePair.add_or_update(KeyValuePairDB(name='k8', value=self.secret_value,
                                                   scope=FULL_SYSTEM_SCOPE,
                                                   secret=True))
-        env = jinja_utils.get_jinja_environment()
 
         context = {}
         context.update({SYSTEM_SCOPE: KeyValueLookup(scope=SYSTEM_SCOPE)})
@@ -46,5 +56,54 @@ def test_filter_decrypt_kv(self):
         })
 
         template = '{{st2kv.system.k8 | decrypt_kv}}'
-        actual = env.from_string(template).render(context)
-        self.assertEqual(actual, secret)
+        actual = self.env.from_string(template).render(context)
+        self.assertEqual(actual, self.secret)
+
+    def test_filter_decrypt_kv_datastore_value_doesnt_exist(self):
+        context = {}
+        context.update({SYSTEM_SCOPE: KeyValueLookup(scope=SYSTEM_SCOPE)})
+        context.update({
+            DATASTORE_PARENT_SCOPE: {
+                SYSTEM_SCOPE: KeyValueLookup(scope=FULL_SYSTEM_SCOPE)
+            }
+        })
+
+        template = '{{st2kv.system.doesnt_exist | decrypt_kv}}'
+
+        expected_msg = ('Referenced datastore item "st2kv.system.doesnt_exist" doesn\'t exist or '
+                        'it contains an empty string')
+        self.assertRaisesRegexp(ValueError, expected_msg, self.env.from_string(template).render,
+                                context)
+
+    def test_filter_decrypt_kv_with_user_scope_value(self):
+        KeyValuePair.add_or_update(KeyValuePairDB(name='stanley:k8', value=self.secret_value,
+                                                  scope=FULL_USER_SCOPE,
+                                                  secret=True))
+
+        context = {}
+        context.update({USER_SCOPE: UserKeyValueLookup(user='stanley', scope=USER_SCOPE)})
+        context.update({
+            DATASTORE_PARENT_SCOPE: {
+                USER_SCOPE: UserKeyValueLookup(user='stanley', scope=FULL_USER_SCOPE)
+            }
+        })
+
+        template = '{{st2kv.user.k8 | decrypt_kv}}'
+        actual = self.env.from_string(template).render(context)
+        self.assertEqual(actual, self.secret)
+
+    def test_filter_decrypt_kv_with_user_scope_value_datastore_value_doesnt_exist(self):
+        context = {}
+        context.update({SYSTEM_SCOPE: KeyValueLookup(scope=SYSTEM_SCOPE)})
+        context.update({
+            DATASTORE_PARENT_SCOPE: {
+                USER_SCOPE: UserKeyValueLookup(user='stanley', scope=FULL_USER_SCOPE)
+            }
+        })
+
+        template = '{{st2kv.user.doesnt_exist | decrypt_kv}}'
+
+        expected_msg = ('Referenced datastore item "st2kv.user.doesnt_exist" doesn\'t exist or '
+                        'it contains an empty string')
+        self.assertRaisesRegexp(ValueError, expected_msg, self.env.from_string(template).render,
+                                context)