From 602bf2b6c5de688c12403842e8366b79bcde3c08 Mon Sep 17 00:00:00 2001 From: Bharath Reddy Date: Wed, 21 Dec 2022 05:40:59 +0000 Subject: [PATCH 1/5] Added implementation for crud operation of personal apikeys when 'personal_keys' flag is added --- conf/st2.dev.conf | 6 +- st2api/st2api/controllers/v1/auth.py | 63 +++++++++++++++---- .../unit/controllers/v1/test_auth_api_keys.py | 1 + st2auth/conf/htpasswd_dev | 2 + st2common/st2common/config.py | 5 ++ .../actions/workflows/__init__.py | 0 6 files changed, 63 insertions(+), 14 deletions(-) delete mode 100644 st2tests/st2tests/fixtures/packs/dummy_pack_23/actions/workflows/__init__.py diff --git a/conf/st2.dev.conf b/conf/st2.dev.conf index cf2b5b6596..63e9b8c828 100644 --- a/conf/st2.dev.conf +++ b/conf/st2.dev.conf @@ -42,14 +42,16 @@ logging = st2actions/conf/logging.conf stream_output = True [rbac] -enable = False +enable = True +backend = default +personal_keys = True [auth] host = 127.0.0.1 port = 9100 use_ssl = False debug = False -enable = False +enable = True logging = st2auth/conf/logging.conf mode = standalone diff --git a/st2api/st2api/controllers/v1/auth.py b/st2api/st2api/controllers/v1/auth.py index d4741c4bf1..bb7591e391 100644 --- a/st2api/st2api/controllers/v1/auth.py +++ b/st2api/st2api/controllers/v1/auth.py @@ -74,11 +74,17 @@ def get_one(self, api_key_id_or_key, requester_user, show_secrets=None): permission_type = PermissionType.API_KEY_VIEW rbac_utils = get_rbac_backend().get_utils_class() - rbac_utils.assert_user_has_resource_db_permission( + if cfg.CONF.rbac.personal_keys: + isKeyCreationAllowed = self.checkPersonalAPIKeyPermission(api_key_db,requester_user) + if not isKeyCreationAllowed: + LOG.exception("User does not have permission to view apikey=%s.", api_key_db) + abort(http_client.BAD_REQUEST, "") + else: + rbac_utils.assert_user_has_resource_api_permission( user_db=requester_user, - resource_db=api_key_db, + resource_api=api_key_db, permission_type=permission_type, - ) + ) try: mask_secrets = self._get_mask_secrets( @@ -126,18 +132,39 @@ def get_all(self, requester_user, show_secrets=None, limit=None, offset=0): return resp + def checkPersonalAPIKeyPermission(self, api_key_api, requester_user): + """ + Checks whether requested user is the creating/updating/fetching/deleting the api key. This is used when + 'personal_keys' flag is set of 'rbac' group in conf file. + """ + if not api_key_api or not requester_user: + return False + rbac_utils = get_rbac_backend().get_utils_class() + user_is_admin = rbac_utils.user_is_admin(user_db=requester_user) + is_same_user = requester_user.name == api_key_api.user + if (user_is_admin) or (is_same_user or api_key_api.user == ""): + return True + else: + return False + + def post(self, api_key_api, requester_user): """ Create a new entry. """ - permission_type = PermissionType.API_KEY_CREATE rbac_utils = get_rbac_backend().get_utils_class() - rbac_utils.assert_user_has_resource_api_permission( + if cfg.CONF.rbac.personal_keys: + isKeyCreationAllowed = self.checkPersonalAPIKeyPermission(api_key_api,requester_user) + if not isKeyCreationAllowed: + LOG.exception("User does not have permission to create apikey=%s.", api_key_api) + abort(http_client.BAD_REQUEST, "") + else: + rbac_utils.assert_user_has_resource_api_permission( user_db=requester_user, resource_api=api_key_api, permission_type=permission_type, - ) + ) api_key_db = None api_key = None @@ -184,11 +211,17 @@ def put(self, api_key_api, api_key_id_or_key, requester_user): permission_type = PermissionType.API_KEY_MODIFY rbac_utils = get_rbac_backend().get_utils_class() - rbac_utils.assert_user_has_resource_db_permission( + if cfg.CONF.rbac.personal_keys: + isKeyCreationAllowed = self.checkPersonalAPIKeyPermission(api_key_db,requester_user) + if not isKeyCreationAllowed: + LOG.exception("User does not have permission to update apikey=%s.", api_key_db) + abort(http_client.BAD_REQUEST, "") + else: + rbac_utils.assert_user_has_resource_api_permission( user_db=requester_user, - resource_db=api_key_db, + resource_api=api_key_db, permission_type=permission_type, - ) + ) old_api_key_db = api_key_db api_key_db = ApiKeyAPI.to_model(api_key_api) @@ -233,11 +266,17 @@ def delete(self, api_key_id_or_key, requester_user): permission_type = PermissionType.API_KEY_DELETE rbac_utils = get_rbac_backend().get_utils_class() - rbac_utils.assert_user_has_resource_db_permission( + if cfg.CONF.rbac.personal_keys: + isKeyCreationAllowed = self.checkPersonalAPIKeyPermission(api_key_db,requester_user) + if not isKeyCreationAllowed: + LOG.exception("User does not have permission to delete apikey=%s.", api_key_db) + abort(http_client.BAD_REQUEST, "") + else: + rbac_utils.assert_user_has_resource_api_permission( user_db=requester_user, - resource_db=api_key_db, + resource_api=api_key_db, permission_type=permission_type, - ) + ) ApiKey.delete(api_key_db) diff --git a/st2api/tests/unit/controllers/v1/test_auth_api_keys.py b/st2api/tests/unit/controllers/v1/test_auth_api_keys.py index ad2e0e5bc4..594ffd608b 100644 --- a/st2api/tests/unit/controllers/v1/test_auth_api_keys.py +++ b/st2api/tests/unit/controllers/v1/test_auth_api_keys.py @@ -51,6 +51,7 @@ def setUpClass(cls): cfg.CONF.set_override(name="mask_secrets", override=True, group="api") cfg.CONF.set_override(name="mask_secrets", override=True, group="log") + cfg.CONF.set_override(name="personal_keys", override=False, group="rbac") models = FixturesLoader().save_fixtures_to_db( fixtures_pack=FIXTURES_PACK, fixtures_dict=TEST_MODELS diff --git a/st2auth/conf/htpasswd_dev b/st2auth/conf/htpasswd_dev index 09e3701940..b20a8545dc 100644 --- a/st2auth/conf/htpasswd_dev +++ b/st2auth/conf/htpasswd_dev @@ -1 +1,3 @@ testu:{SHA}V1t6eZLxnehb7CTBuj61Nq3lIh4= +user1:qwerty +user2:qwerty \ No newline at end of file diff --git a/st2common/st2common/config.py b/st2common/st2common/config.py index c88955e4bb..71174c9c3a 100644 --- a/st2common/st2common/config.py +++ b/st2common/st2common/config.py @@ -72,6 +72,11 @@ def register_opts(ignore_errors=False): "executions. All resources can only be viewed or executed by the owning user " "except the admin and system_user who can view or run everything.", ), + cfg.BoolOpt( + "personal_keys", + default=True, + help="Enable all non-admin users to create/update/delete their own keys. They cannot create/update/delete other's keys.", + ), ] do_register_opts(rbac_opts, "rbac", ignore_errors) diff --git a/st2tests/st2tests/fixtures/packs/dummy_pack_23/actions/workflows/__init__.py b/st2tests/st2tests/fixtures/packs/dummy_pack_23/actions/workflows/__init__.py deleted file mode 100644 index e69de29bb2..0000000000 From b4c16f39897937ae13e3a47e92daecfb1c52bd54 Mon Sep 17 00:00:00 2001 From: Bharath Reddy Date: Thu, 5 Jan 2023 06:49:33 +0000 Subject: [PATCH 2/5] Removed test data in htpasswd file --- st2api/st2api/controllers/v1/auth.py | 59 +++++++++++++++++----------- st2auth/conf/htpasswd_dev | 4 +- 2 files changed, 38 insertions(+), 25 deletions(-) diff --git a/st2api/st2api/controllers/v1/auth.py b/st2api/st2api/controllers/v1/auth.py index bb7591e391..3b7133d038 100644 --- a/st2api/st2api/controllers/v1/auth.py +++ b/st2api/st2api/controllers/v1/auth.py @@ -75,15 +75,19 @@ def get_one(self, api_key_id_or_key, requester_user, show_secrets=None): permission_type = PermissionType.API_KEY_VIEW rbac_utils = get_rbac_backend().get_utils_class() if cfg.CONF.rbac.personal_keys: - isKeyCreationAllowed = self.checkPersonalAPIKeyPermission(api_key_db,requester_user) + isKeyCreationAllowed = self.checkPersonalAPIKeyPermission( + api_key_db, requester_user + ) if not isKeyCreationAllowed: - LOG.exception("User does not have permission to view apikey=%s.", api_key_db) + LOG.exception( + "User does not have permission to view apikey=%s.", api_key_db + ) abort(http_client.BAD_REQUEST, "") else: rbac_utils.assert_user_has_resource_api_permission( - user_db=requester_user, - resource_api=api_key_db, - permission_type=permission_type, + user_db=requester_user, + resource_db=api_key_db, + permission_type=permission_type, ) try: @@ -134,7 +138,7 @@ def get_all(self, requester_user, show_secrets=None, limit=None, offset=0): def checkPersonalAPIKeyPermission(self, api_key_api, requester_user): """ - Checks whether requested user is the creating/updating/fetching/deleting the api key. This is used when + Checks whether requested user is the creating/updating/fetching/deleting the api key. This is used when 'personal_keys' flag is set of 'rbac' group in conf file. """ if not api_key_api or not requester_user: @@ -147,7 +151,6 @@ def checkPersonalAPIKeyPermission(self, api_key_api, requester_user): else: return False - def post(self, api_key_api, requester_user): """ Create a new entry. @@ -155,15 +158,19 @@ def post(self, api_key_api, requester_user): permission_type = PermissionType.API_KEY_CREATE rbac_utils = get_rbac_backend().get_utils_class() if cfg.CONF.rbac.personal_keys: - isKeyCreationAllowed = self.checkPersonalAPIKeyPermission(api_key_api,requester_user) + isKeyCreationAllowed = self.checkPersonalAPIKeyPermission( + api_key_api, requester_user + ) if not isKeyCreationAllowed: - LOG.exception("User does not have permission to create apikey=%s.", api_key_api) + LOG.exception( + "User does not have permission to create apikey=%s.", api_key_api + ) abort(http_client.BAD_REQUEST, "") else: rbac_utils.assert_user_has_resource_api_permission( - user_db=requester_user, - resource_api=api_key_api, - permission_type=permission_type, + user_db=requester_user, + resource_api=api_key_api, + permission_type=permission_type, ) api_key_db = None @@ -212,15 +219,19 @@ def put(self, api_key_api, api_key_id_or_key, requester_user): permission_type = PermissionType.API_KEY_MODIFY rbac_utils = get_rbac_backend().get_utils_class() if cfg.CONF.rbac.personal_keys: - isKeyCreationAllowed = self.checkPersonalAPIKeyPermission(api_key_db,requester_user) + isKeyCreationAllowed = self.checkPersonalAPIKeyPermission( + api_key_db, requester_user + ) if not isKeyCreationAllowed: - LOG.exception("User does not have permission to update apikey=%s.", api_key_db) + LOG.exception( + "User does not have permission to update apikey=%s.", api_key_db + ) abort(http_client.BAD_REQUEST, "") else: rbac_utils.assert_user_has_resource_api_permission( - user_db=requester_user, - resource_api=api_key_db, - permission_type=permission_type, + user_db=requester_user, + resource_db=api_key_db, + permission_type=permission_type, ) old_api_key_db = api_key_db @@ -267,15 +278,19 @@ def delete(self, api_key_id_or_key, requester_user): permission_type = PermissionType.API_KEY_DELETE rbac_utils = get_rbac_backend().get_utils_class() if cfg.CONF.rbac.personal_keys: - isKeyCreationAllowed = self.checkPersonalAPIKeyPermission(api_key_db,requester_user) + isKeyCreationAllowed = self.checkPersonalAPIKeyPermission( + api_key_db, requester_user + ) if not isKeyCreationAllowed: - LOG.exception("User does not have permission to delete apikey=%s.", api_key_db) + LOG.exception( + "User does not have permission to delete apikey=%s.", api_key_db + ) abort(http_client.BAD_REQUEST, "") else: rbac_utils.assert_user_has_resource_api_permission( - user_db=requester_user, - resource_api=api_key_db, - permission_type=permission_type, + user_db=requester_user, + resource_db=api_key_db, + permission_type=permission_type, ) ApiKey.delete(api_key_db) diff --git a/st2auth/conf/htpasswd_dev b/st2auth/conf/htpasswd_dev index b20a8545dc..a7ed367b86 100644 --- a/st2auth/conf/htpasswd_dev +++ b/st2auth/conf/htpasswd_dev @@ -1,3 +1 @@ -testu:{SHA}V1t6eZLxnehb7CTBuj61Nq3lIh4= -user1:qwerty -user2:qwerty \ No newline at end of file +testu:{SHA}V1t6eZLxnehb7CTBuj61Nq3lIh4= \ No newline at end of file From 9e79c808718f9be2a9b90584182fdd705d0df4bb Mon Sep 17 00:00:00 2001 From: Bharath Reddy Date: Thu, 5 Jan 2023 07:21:46 +0000 Subject: [PATCH 3/5] Small changes in auth.py --- st2api/st2api/controllers/v1/auth.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/st2api/st2api/controllers/v1/auth.py b/st2api/st2api/controllers/v1/auth.py index 3b7133d038..be7b6c3e9d 100644 --- a/st2api/st2api/controllers/v1/auth.py +++ b/st2api/st2api/controllers/v1/auth.py @@ -84,7 +84,7 @@ def get_one(self, api_key_id_or_key, requester_user, show_secrets=None): ) abort(http_client.BAD_REQUEST, "") else: - rbac_utils.assert_user_has_resource_api_permission( + rbac_utils.assert_user_has_resource_db_permission( user_db=requester_user, resource_db=api_key_db, permission_type=permission_type, @@ -228,7 +228,7 @@ def put(self, api_key_api, api_key_id_or_key, requester_user): ) abort(http_client.BAD_REQUEST, "") else: - rbac_utils.assert_user_has_resource_api_permission( + rbac_utils.assert_user_has_resource_db_permission( user_db=requester_user, resource_db=api_key_db, permission_type=permission_type, @@ -287,7 +287,7 @@ def delete(self, api_key_id_or_key, requester_user): ) abort(http_client.BAD_REQUEST, "") else: - rbac_utils.assert_user_has_resource_api_permission( + rbac_utils.assert_user_has_resource_db_permission( user_db=requester_user, resource_db=api_key_db, permission_type=permission_type, From 1b1d70ca4e33cd8375cca7bbc1487c6bc736a166 Mon Sep 17 00:00:00 2001 From: Bharath Reddy Date: Thu, 5 Jan 2023 07:30:44 +0000 Subject: [PATCH 4/5] Disabling rbac, auth in config due to errors in integration tests --- conf/st2.dev.conf | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/conf/st2.dev.conf b/conf/st2.dev.conf index 63e9b8c828..1c70b09cb9 100644 --- a/conf/st2.dev.conf +++ b/conf/st2.dev.conf @@ -42,8 +42,7 @@ logging = st2actions/conf/logging.conf stream_output = True [rbac] -enable = True -backend = default +enable = False personal_keys = True [auth] @@ -51,7 +50,7 @@ host = 127.0.0.1 port = 9100 use_ssl = False debug = False -enable = True +enable = False logging = st2auth/conf/logging.conf mode = standalone From 94b895bb032010139894f880e72ef06461e01282 Mon Sep 17 00:00:00 2001 From: Bharath Reddy Date: Fri, 6 Jan 2023 06:25:14 +0000 Subject: [PATCH 5/5] Small changes as suggested by reviewer --- CHANGELOG.rst | 2 ++ conf/st2.dev.conf | 2 +- st2api/st2api/controllers/v1/auth.py | 9 +++------ st2common/st2common/config.py | 2 +- .../packs/dummy_pack_23/actions/workflows/__init__.py | 0 5 files changed, 7 insertions(+), 8 deletions(-) create mode 100644 st2tests/st2tests/fixtures/packs/dummy_pack_23/actions/workflows/__init__.py diff --git a/CHANGELOG.rst b/CHANGELOG.rst index e14c784128..316c401cca 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -19,6 +19,8 @@ Added * Added a joint index to solve the problem of slow mongo queries for scheduled executions. #5805 +* Added a flag in config for a feature where non-admin users can manage their own keys. #5859 + Contributed by @bharath-orchestral 3.8.0 - November 18, 2022 ------------------------- diff --git a/conf/st2.dev.conf b/conf/st2.dev.conf index 1c70b09cb9..92a873cf66 100644 --- a/conf/st2.dev.conf +++ b/conf/st2.dev.conf @@ -43,7 +43,7 @@ stream_output = True [rbac] enable = False -personal_keys = True +personal_keys = False [auth] host = 127.0.0.1 diff --git a/st2api/st2api/controllers/v1/auth.py b/st2api/st2api/controllers/v1/auth.py index be7b6c3e9d..5a13b22818 100644 --- a/st2api/st2api/controllers/v1/auth.py +++ b/st2api/st2api/controllers/v1/auth.py @@ -138,18 +138,15 @@ def get_all(self, requester_user, show_secrets=None, limit=None, offset=0): def checkPersonalAPIKeyPermission(self, api_key_api, requester_user): """ - Checks whether requested user is the creating/updating/fetching/deleting the api key. This is used when - 'personal_keys' flag is set of 'rbac' group in conf file. + Checks whether requested user is creating/updating/fetching/deleting the api key. This is used when + 'personal_keys' flag is set in 'rbac' group in conf file. """ if not api_key_api or not requester_user: return False rbac_utils = get_rbac_backend().get_utils_class() user_is_admin = rbac_utils.user_is_admin(user_db=requester_user) is_same_user = requester_user.name == api_key_api.user - if (user_is_admin) or (is_same_user or api_key_api.user == ""): - return True - else: - return False + return (user_is_admin) or (is_same_user or api_key_api.user == "") def post(self, api_key_api, requester_user): """ diff --git a/st2common/st2common/config.py b/st2common/st2common/config.py index 71174c9c3a..6e62ba1d6f 100644 --- a/st2common/st2common/config.py +++ b/st2common/st2common/config.py @@ -74,7 +74,7 @@ def register_opts(ignore_errors=False): ), cfg.BoolOpt( "personal_keys", - default=True, + default=False, help="Enable all non-admin users to create/update/delete their own keys. They cannot create/update/delete other's keys.", ), ] diff --git a/st2tests/st2tests/fixtures/packs/dummy_pack_23/actions/workflows/__init__.py b/st2tests/st2tests/fixtures/packs/dummy_pack_23/actions/workflows/__init__.py new file mode 100644 index 0000000000..e69de29bb2