Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue/personal keys #5859

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Issue/personal keys #5859

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
602bf2b
Added implementation for crud operation of personal apikeys when 'per…
bharath-orchestral Dec 21, 2022
f45874c
Merge branch 'master' into issue/personal_keys
bharath-orchestral Jan 5, 2023
b4c16f3
Removed test data in htpasswd file
bharath-orchestral Jan 5, 2023
9e79c80
Small changes in auth.py
bharath-orchestral Jan 5, 2023
1b1d70c
Disabling rbac, auth in config due to errors in integration tests
bharath-orchestral Jan 5, 2023
94b895b
Small changes as suggested by reviewer
bharath-orchestral Jan 6, 2023
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
1 change: 1 addition & 0 deletions conf/st2.dev.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ stream_output = True

[rbac]
enable = False
personal_keys = True
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want the default to be True, or should it be False for backwards compatibility?


[auth]
host = 127.0.0.1
Expand Down
96 changes: 75 additions & 21 deletions st2api/st2api/controllers/v1/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,21 @@ 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(
user_db=requester_user,
resource_db=api_key_db,
permission_type=permission_type,
)
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use python 3 f-strings instead of the older method of formatting up strings?

)
abort(http_client.BAD_REQUEST, "")
else:
rbac_utils.assert_user_has_resource_db_permission(
user_db=requester_user,
resource_db=api_key_db,
permission_type=permission_type,
)

try:
mask_secrets = self._get_mask_secrets(
Expand Down Expand Up @@ -126,18 +136,42 @@ def get_all(self, requester_user, show_secrets=None, limit=None, offset=0):

return resp

def checkPersonalAPIKeyPermission(self, api_key_api, requester_user):
Copy link
Contributor

Choose a reason for hiding this comment

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

api_key_api seems an odd name, is there a more expressive name for variable we can use?

"""
Checks whether requested user is the creating/updating/fetching/deleting the api key. This is used when
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Checks whether requested user is the creating/updating/fetching/deleting the api key. This is used when
Checks whether requested user is creating/updating/fetching/deleting the api key. This is used when

'personal_keys' flag is set of 'rbac' group in conf file.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'personal_keys' flag is set of 'rbac' group in conf file.
'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
nzlosh marked this conversation as resolved.
Show resolved Hide resolved

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(
user_db=requester_user,
resource_api=api_key_api,
permission_type=permission_type,
)
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
Expand Down Expand Up @@ -184,11 +218,21 @@ 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(
user_db=requester_user,
resource_db=api_key_db,
permission_type=permission_type,
)
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_db_permission(
user_db=requester_user,
resource_db=api_key_db,
permission_type=permission_type,
)

old_api_key_db = api_key_db
api_key_db = ApiKeyAPI.to_model(api_key_api)
Expand Down Expand Up @@ -233,11 +277,21 @@ 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(
user_db=requester_user,
resource_db=api_key_db,
permission_type=permission_type,
)
if cfg.CONF.rbac.personal_keys:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think having this in here rather than in rbac_utils doesn't work with the idea that the rbac is optional.
So for instance if we were configured without rbac, then the no-op rbac backend would be being used, and then the options like this would always be allowed.
This now means that if I don't have RBAC backend I have to have the personal_api_keys set in the config.

Perhaps would be better that we always use rbac_utils to ask if user has db permission, and only if that returns false then you'd want to check the personal keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you help me as to where to add the code? I cannot understand where the rbac_utils methods are implemented.I have checked base.py in st2common/st2common/rbac/backends/ . Do I need to add the method there, check if conf.rbac.enabled is true and proceed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was more suggesting that you reverse the order in this method. e.g. First call rbac_utils method to see if they have permission to access database. Then if the rbac_utils calls says No, you add in your check for personal keys.
That way when the noop backend is used (which will always say yes to permission), you don't need to check personal keys - and can keep personal keys default to false.

There are multiple rbac backends, so if you wanted to add a new method you would need to add into all implementations. The base.py is just the base rbac class which always says not implemented. The noop.py is a always yes utils. Then in the st2-rbac-backend GIT repo you will find the implementation that is used when you really do use RBAC - which uses the rbac roles and auth files.

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_db_permission(
user_db=requester_user,
resource_db=api_key_db,
permission_type=permission_type,
)

ApiKey.delete(api_key_db)

Expand Down
1 change: 1 addition & 0 deletions st2api/tests/unit/controllers/v1/test_auth_api_keys.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion st2auth/conf/htpasswd_dev
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
testu:{SHA}V1t6eZLxnehb7CTBuj61Nq3lIh4=
testu:{SHA}V1t6eZLxnehb7CTBuj61Nq3lIh4=
5 changes: 5 additions & 0 deletions st2common/st2common/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

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

As above do we want default to be True? As that's not backwards compatible with previous versions of STackStorm?

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)
Expand Down
Empty file removed st2tests/st2tests/fixtures/packs/dummy_pack_23/actions/workflows/__init__.py
View file
Open in desktop
Empty file.