From 17e11346ee3aa9b9ba144685877e07f118f7ee28 Mon Sep 17 00:00:00 2001 From: Alexander VanTol Date: Mon, 12 Mar 2018 08:51:29 -0500 Subject: [PATCH] fix(group-creation): adhere to Google requirements for service account names (#5) * fix(group-api): validation on group names, fix id/email useage * fix(group-creation): return responses for group and service account * fix(tests): update tests to use new naming convention for groups/sa's * chore(tests): general cleanup, move some common functionality and fixtures to conftest * chore(service-account-naming): cleanup algorithm to adhere to google's naming conventions --- cirrus/google_cloud/manager.py | 97 +++++++++++++++--- test/conftest.py | 92 ++++++++++++++++++ test/test_google_cloud_manage.py | 162 +++++++++++++++++-------------- 3 files changed, 268 insertions(+), 83 deletions(-) create mode 100644 test/conftest.py diff --git a/cirrus/google_cloud/manager.py b/cirrus/google_cloud/manager.py index 198de3ce..ebf8fa3c 100644 --- a/cirrus/google_cloud/manager.py +++ b/cirrus/google_cloud/manager.py @@ -124,17 +124,56 @@ def create_proxy_group_for_user(self, user_id, username): username (str): User's name Returns: - str: New proxy group's ID + dict: JSON responses from API calls, which should contain the new group + `Google API Reference `_ + and successfully created service account + `Google API Reference `_ + + .. code-block:: python + + { + "group": { + "kind": "admin#directory#group", + "id": string, + "etag": etag, + "email": string, + "name": string, + "directMembersCount": long, + "description": string, + "adminCreated": boolean, + "aliases": [ + string + ], + "nonEditableAliases": [ + string + ] + } + "primary_service_account": { + "name": string, + "projectId": string, + "uniqueId": string, + "email": string, + "displayName": string, + "etag": string, + "oauth2ClientId": string, + } + } """ group_name = _get_proxy_group_name_for_user(user_id, username) + service_account_id = _get_proxy_group_service_account_id_for_user( + user_id, username + ) # Create group and service account, then add service account to group new_group_response = self.create_group(name=group_name) - new_group_id = new_group_response["id"] - self.create_service_account_for_proxy_group(new_group_id, - account_id=user_id) + new_group_id = new_group_response["email"] + service_account_response = self.create_service_account_for_proxy_group( + new_group_id, account_id=service_account_id) - return new_group_id + return { + "group": new_group_response, + "primary_service_account": service_account_response + } def get_access_key(self, account): """ @@ -236,7 +275,10 @@ def get_primary_service_account(self, proxy_group_id): """ primary_email = None - user_id = _get_user_id_from_proxy_group(proxy_group_id) + proxy_group = self.get_group(proxy_group_id) + + user_id = _get_user_id_from_proxy_group(proxy_group["email"]) + username = _get_user_name_from_proxy_group(proxy_group["email"]) all_service_accounts = self.get_service_accounts_from_group(proxy_group_id) # create dict with first part of email as key and whole email as value @@ -245,8 +287,12 @@ def get_primary_service_account(self, proxy_group_id): for account in all_service_accounts } - if user_id in service_account_emails: - primary_email = service_account_emails[user_id] + service_account_id_for_user = ( + _get_proxy_group_service_account_id_for_user(user_id, username) + ) + + if service_account_id_for_user in service_account_emails: + primary_email = service_account_emails[service_account_id_for_user] return self.get_service_account(primary_email) @@ -318,7 +364,7 @@ def get_service_account(self, account): } """ api_url = _get_google_api_url("projects/" + self.project_id + - "/serviceAccounts/" + account, + "/serviceAccounts/" + str(account), GOOGLE_IAM_API_URL) response = self._authed_request("GET", api_url) @@ -1076,6 +1122,35 @@ def _get_proxy_group_name_for_user(user_id, username): return str(username).strip().replace(" ", "-") + "-" + str(user_id) +def _get_proxy_group_service_account_id_for_user(user_id, username): + """ + Return a valid service account id based on user_id and username + + Currently Google enforces the following: + 6-30 characters + Must match: [a-z][a-z\d\-]*[a-z\d] + + Args: + user_id (str): User's uuid + username (str): user's name + + Returns: + str: service account id + """ + username = str(username).replace(" ", "_") + user_id = str(user_id) + + # Truncate username so full account ID is at most 30 characters. + full_account_id_length = len(username) + len(user_id) + 1 + chars_to_drop = full_account_id_length - 30 + truncated_username = username[:-chars_to_drop] + account_id = truncated_username + '-' + user_id + + # Pad account ID to at least 6 chars long. + account_id += (6 - len(account_id)) * '-' + return account_id + + def _get_user_id_from_proxy_group(proxy_group): """ Return user id by analyzing proxy_group name @@ -1086,7 +1161,7 @@ def _get_user_id_from_proxy_group(proxy_group): Returns: str: User id """ - return proxy_group.split("-")[0].strip() + return proxy_group.split('@')[0].split("-")[1].strip() def _get_user_name_from_proxy_group(proxy_group): @@ -1099,7 +1174,7 @@ def _get_user_name_from_proxy_group(proxy_group): Returns: str: Username """ - return proxy_group.split("-")[1].strip() + return proxy_group.split('@')[0].split("-")[0].strip() class GoogleAuthError(Exception): diff --git a/test/conftest.py b/test/conftest.py new file mode 100644 index 00000000..329f1afd --- /dev/null +++ b/test/conftest.py @@ -0,0 +1,92 @@ +import pytest + +# Python 2 and 3 compatible +try: + from unittest.mock import MagicMock + from unittest.mock import patch +except ImportError: + from mock import MagicMock + from mock import patch + +from cirrus import GoogleCloudManager +from cirrus.google_cloud.manager import _get_proxy_group_name_for_user +from cirrus.google_cloud.manager import _get_proxy_group_service_account_id_for_user + + +def get_test_cloud_manager(): + project_id = "test_project" + manager = GoogleCloudManager(project_id) + manager._authed_session = MagicMock() + manager._admin_service = MagicMock() + manager._storage_client = MagicMock() + return manager + + +@pytest.fixture +def test_cloud_manager(): + return get_test_cloud_manager() + + +@pytest.fixture +def test_cloud_manager_group_and_service_accounts_mocked(): + test_cloud_manager = get_test_cloud_manager() + + test_domain = "test-domain.net" + new_member_1_id = "1" + new_member_1_username = "testuser" + primary_service_account = _get_proxy_group_service_account_id_for_user( + new_member_1_id, new_member_1_username + ) + "@" + test_domain + + group_name = _get_proxy_group_name_for_user( + new_member_1_id, new_member_1_username) + group_email = group_name + "@" + test_domain + mock_get_group(test_cloud_manager, group_name, group_email) + + mock_get_service_accounts_from_group( + test_cloud_manager, primary_service_account) + + mock_get_service_account( + test_cloud_manager, primary_service_account) + + return test_cloud_manager + + +def mock_get_group(test_cloud_manager, group_name, group_email): + test_cloud_manager.get_group = MagicMock() + test_cloud_manager.get_group.return_value = { + "kind": "admin#directory#group", + "id": group_name, + "etag": "", + "email": group_email, + "name": "", + "directMembersCount": 0, + "description": "", + "adminCreated": False, + "aliases": [ + "" + ], + "nonEditableAliases": [ + "" + ] + } + + +def mock_get_service_accounts_from_group( + test_cloud_manager, primary_service_account): + test_cloud_manager.get_service_accounts_from_group = MagicMock() + test_cloud_manager.get_service_accounts_from_group.return_value = [ + primary_service_account] + + +def mock_get_service_account(test_cloud_manager, primary_service_account): + test_cloud_manager.get_service_account = MagicMock() + test_cloud_manager.get_service_account.return_value = { + "name": "", + "projectId": "", + "uniqueId": "", + "email": primary_service_account, + "displayName": "", + "etag": "", + "oauth2ClientId": "", + } diff --git a/test/test_google_cloud_manage.py b/test/test_google_cloud_manage.py index 553ceb82..57ce0f7b 100644 --- a/test/test_google_cloud_manage.py +++ b/test/test_google_cloud_manage.py @@ -1,4 +1,3 @@ -from cirrus import GoogleCloudManager import pytest import json from requests import HTTPError @@ -14,15 +13,13 @@ from mock import MagicMock from mock import patch +from cirrus import GoogleCloudManager +from cirrus.google_cloud.manager import _get_proxy_group_name_for_user +from cirrus.google_cloud.manager import _get_proxy_group_service_account_id_for_user -@pytest.fixture -def test_cloud_manager(): - project_id = "test_project" - manager = GoogleCloudManager(project_id) - manager._authed_session = MagicMock() - manager._admin_service = MagicMock() - manager._storage_client = MagicMock() - return manager +from test.conftest import mock_get_group +from test.conftest import mock_get_service_accounts_from_group +from test.conftest import mock_get_service_account def _fake_response(status_code, json_response_as_dict=None): @@ -37,12 +34,13 @@ def _fake_response(status_code, json_response_as_dict=None): def test_get_service_account_valid(test_cloud_manager): """ - Test that the result from getting service account is the result from the Google API + Test that the result from getting service account is the result from the + Google API """ # Setup # # Google API responds OK with some data - test_cloud_manager._authed_session.get.return_value = _fake_response(200, - {"uniqueId": "123"}) + test_cloud_manager._authed_session.get.return_value = _fake_response( + 200, {"uniqueId": "123"}) # Call # service_account = test_cloud_manager.get_service_account("123") @@ -53,7 +51,8 @@ def test_get_service_account_valid(test_cloud_manager): def test_get_service_accounts_valid(test_cloud_manager): """ - Test that the result from getting service accounts is the result from the Google API + Test that the result from getting service accounts is the result from the + Google API """ # Setup # # Google API responds OK with some data @@ -80,8 +79,8 @@ def test_get_service_accounts_valid(test_cloud_manager): ], "nextPageToken": "", } - test_cloud_manager._authed_session.get.return_value = _fake_response(200, - response) + test_cloud_manager._authed_session.get.return_value = _fake_response( + 200, response) # Call # service_accounts = test_cloud_manager.get_all_service_accounts() @@ -148,12 +147,13 @@ def test_create_service_account_valid(test_cloud_manager): # Setup # service_account_unique_id = "123" test_cloud_manager.set_iam_policy = MagicMock() - test_cloud_manager._authed_session.post.return_value = _fake_response(200, - {"uniqueId": service_account_unique_id}) + test_cloud_manager._authed_session.post.return_value = _fake_response( + 200, {"uniqueId": service_account_unique_id}) account_id = "some_new_service_account" - expected_new_service_account = ("projects/" + test_cloud_manager.project_id + - "/serviceAccounts/" + service_account_unique_id) + expected_new_service_account = ( + "projects/" + test_cloud_manager.project_id + + "/serviceAccounts/" + service_account_unique_id) # Call # service_account = test_cloud_manager.create_service_account(account_id) @@ -162,8 +162,9 @@ def test_create_service_account_valid(test_cloud_manager): assert service_account["uniqueId"] == service_account_unique_id assert test_cloud_manager._authed_session.post.called is True - # Naive check to see if the new account appears in the call to set_iam_policy - # as any argument or keyword argument (in case API changes or kwarg not used during call) + # Naive check to see if the new account appears in the call to + # set_iam_policy as any argument or keyword argument (in case API changes + # or kwarg not used during call) # Merits of this approach can be argued, I don't even know if I like it... args, kwargs = test_cloud_manager.set_iam_policy.call_args assert ( @@ -174,7 +175,8 @@ def test_create_service_account_valid(test_cloud_manager): def test_delete_service_account(test_cloud_manager): """ - Test that deleting a service account actually calls google API with given account + Test that deleting a service account actually calls google API with + given account """ # Setup # test_cloud_manager._authed_session.delete.return_value = _fake_response(200) @@ -205,15 +207,16 @@ def test_create_service_account_key(test_cloud_manager): key_id = "123" key_private_data = "1a2s3d4f5g6h" response = { - "name": "projects/storied-bearing-184114/serviceAccounts/{}/keys/{}".format(account, - key_id), + "name": "projects/storied-bearing-184114/serviceAccounts/{}/keys/{}".format( + account, key_id), "validBeforeTime": "2027-12-05T15:38:03Z", "privateKeyData": "{}".format(key_private_data), "privateKeyType": "TYPE_GOOGLE_CREDENTIALS_FILE", "keyAlgorithm": "KEY_ALG_RSA_2048", "validAfterTime": "2017-12-07T15:38:03Z" } - test_cloud_manager._authed_session.post.return_value = _fake_response(200, response) + test_cloud_manager._authed_session.post.return_value = _fake_response( + 200, response) # Call # key = test_cloud_manager.create_service_account_key(account) @@ -319,7 +322,8 @@ def test_create_service_account_key_invalid_account(test_cloud_manager): account = "account-that-doesnt-exist" fake_response = _fake_response(400, {}) # fancy lambda to throw httperror for the bad response - fake_response.raise_for_status.side_effect = HTTPError(MagicMock(status=400), 'not found') + fake_response.raise_for_status.side_effect = HTTPError( + MagicMock(status=400), 'not found') test_cloud_manager._authed_session.post.return_value = fake_response # Call # @@ -350,13 +354,13 @@ def test_get_service_account_key(test_cloud_manager): "name": key_name, "keyAlgorithm": "", } - test_cloud_manager._authed_session.get.return_value = _fake_response(200, - response) + test_cloud_manager._authed_session.get.return_value = _fake_response( + 200, response) account = "abc" # Call # - key = test_cloud_manager.get_service_account_key(account, - key_name) + key = test_cloud_manager.get_service_account_key( + account, key_name) # Test # assert key["name"] == key_name @@ -371,12 +375,12 @@ def test_get_service_account_policy_valid(test_cloud_manager): # Google API responds OK with some data account = "123" resource = "456" - test_cloud_manager._authed_session.post.return_value = _fake_response(200, - {"some_policy": "some_value"}) + test_cloud_manager._authed_session.post.return_value = _fake_response( + 200, {"some_policy": "some_value"}) # Call # - service_account_policy = test_cloud_manager.get_service_account_policy(account, - resource) + service_account_policy = test_cloud_manager.get_service_account_policy( + account, resource) # Test # assert service_account_policy["some_policy"] == "some_value" @@ -400,8 +404,8 @@ def test_set_iam_policy(test_cloud_manager): # Setup # account = "123" resource = "456" - test_cloud_manager._authed_session.post.return_value = _fake_response(200, - {"some_policy": "some_value"}) + test_cloud_manager._authed_session.post.return_value = _fake_response( + 200, {"some_policy": "some_value"}) # Call # service_account_policy = test_cloud_manager.set_iam_policy(account, resource) @@ -650,36 +654,41 @@ def test_get_primary_service_account(test_cloud_manager): """ Test getting the primary account in a group. """ - # Setup # test_domain = "test-domain.net" - primary_service_account = "primary-account" + test_domain - new_member_1_id = "1" - new_member_1_email = new_member_1_id + "@" + test_domain - group_id = new_member_1_id + "-testuser" # This has to be the user's ID - Username + new_member_1_username = "testuser" + primary_service_account = _get_proxy_group_service_account_id_for_user( + new_member_1_id, new_member_1_username + ) + "@" + test_domain - test_cloud_manager.get_service_accounts_from_group = MagicMock() - test_cloud_manager.get_service_accounts_from_group.return_value = [new_member_1_email] - test_cloud_manager.get_service_account = MagicMock() - test_cloud_manager.get_service_account.return_value = primary_service_account - test_cloud_manager._service_account_email_domain = test_domain + group_name = _get_proxy_group_name_for_user( + new_member_1_id, new_member_1_username) + group_email = group_name + "@" + test_domain + mock_get_group(test_cloud_manager, group_name, group_email) + + mock_get_service_accounts_from_group( + test_cloud_manager, primary_service_account) + + mock_get_service_account( + test_cloud_manager, primary_service_account) # Call # - email = test_cloud_manager.get_primary_service_account(group_id) + response = test_cloud_manager.get_primary_service_account(group_name) + email = response["email"] # Test # - assert email == primary_service_account + assert email == group_email # check if group id is somewhere in the args to insert args, kwargs = test_cloud_manager.get_service_accounts_from_group.call_args assert ( - any(group_id in str(arg) for arg in args) or - any(group_id in str(kwarg) for kwarg in kwargs.values()) + any(group_name in str(arg) for arg in args) or + any(group_name in str(kwarg) for kwarg in kwargs.values()) ) args, kwargs = test_cloud_manager.get_service_account.call_args assert ( - any(new_member_1_email in str(arg) for arg in args) or - any(new_member_1_email in str(kwarg) for kwarg in kwargs.values()) + any(primary_service_account in str(arg) for arg in args) or + any(primary_service_account in str(kwarg) for kwarg in kwargs.values()) ) @@ -688,39 +697,49 @@ def test_get_service_account_from_group_mult_accounts(test_cloud_manager): Test that when a group contains multiple service accounts, we still get the right primary account """ - # Setup # + # Setup test_domain = "test-domain.net" - primary_service_account = "primary-account" + test_domain - new_member_1_id = "1" - new_member_1_email = new_member_1_id + "@" + test_domain - new_member_2_id = "2" - new_member_2_email = "2@" + test_domain + new_member_1_username = "testuser" + primary_service_account = _get_proxy_group_service_account_id_for_user( + new_member_1_id, new_member_1_username + ) + "@" + test_domain + + group_name = _get_proxy_group_name_for_user( + new_member_1_id, new_member_1_username) + group_email = group_name + "@" + test_domain + mock_get_group(test_cloud_manager, group_name, group_email) - group_id = new_member_2_id + "-testuser" # This has to be the user's ID - Username + mock_get_service_accounts_from_group( + test_cloud_manager, primary_service_account) + + mock_get_service_account( + test_cloud_manager, primary_service_account) test_cloud_manager.get_service_accounts_from_group = MagicMock() - test_cloud_manager.get_service_accounts_from_group.return_value = [new_member_1_email, new_member_2_email] - test_cloud_manager.get_service_account = MagicMock() - test_cloud_manager.get_service_account.return_value = primary_service_account - test_cloud_manager._service_account_email_domain = test_domain + test_cloud_manager.get_service_accounts_from_group.return_value = [ + "some-other-account" + "@" + test_domain, + primary_service_account, + "another-account" + "@" + test_domain + ] # Call # - email = test_cloud_manager.get_primary_service_account(group_id) + response = test_cloud_manager.get_primary_service_account(group_name) + email = response["email"] # Test # - assert email == primary_service_account + assert email == group_email - # check if group id is somewhere in the args + # check if group id is somewhere in the args to insert args, kwargs = test_cloud_manager.get_service_accounts_from_group.call_args assert ( - any(group_id in str(arg) for arg in args) or - any(group_id in str(kwarg) for kwarg in kwargs.values()) + any(group_name in str(arg) for arg in args) or + any(group_name in str(kwarg) for kwarg in kwargs.values()) ) args, kwargs = test_cloud_manager.get_service_account.call_args assert ( - any(new_member_2_email in str(arg) for arg in args) or - any(new_member_2_email in str(kwarg) for kwarg in kwargs.values()) + any(primary_service_account in str(arg) for arg in args) or + any(primary_service_account in str(kwarg) for kwarg in kwargs.values()) ) @@ -850,6 +869,7 @@ class NewDatetime(datetime.datetime): def __new__(cls, *args, **kwargs): return datetime.datetime.__new__(datetime.datetime, *args, **kwargs) + @patch("cirrus.google_cloud.manager.datetime", NewDatetime) def test_handle_expired_service_account_keys(monkeypatch, test_cloud_manager): # Setup # @@ -889,8 +909,6 @@ def test_handle_expired_service_account_keys(monkeypatch, test_cloud_manager): test_cloud_manager.get_service_account_keys_info.return_value = keys # Call # - # with patch("cirrus.google_cloud.manager.datetime") as mock_date: - # mock_date.return_value = NewDatetime test_cloud_manager.handle_expired_service_account_keys(account=account) # Test #