From 0d36df38f40d31e4c30137f65fb20690597f2f7d Mon Sep 17 00:00:00 2001 From: Claudia Richoux Date: Tue, 14 Apr 2020 19:21:43 -0500 Subject: [PATCH 1/8] backend works and has tests, still need docs and frontend fixes --- .gitignore | 2 ++ Dockerfile | 6 ++-- Makefile | 4 ++- confidant/authnz/__init__.py | 11 +++++-- confidant/authnz/errors.py | 1 - confidant/authnz/rbac.py | 9 +++++- confidant/models/blind_credential.py | 3 ++ confidant/models/credential.py | 8 +++++ .../controllers/CredentialDetailsCtrl.js | 4 ++- .../resources/views/credential-details.html | 4 +++ confidant/routes/certificates.py | 2 -- confidant/routes/credentials.py | 11 ++++++- confidant/schema/credentials.py | 3 ++ docs/README.md | 2 ++ docs/root/acls.md | 2 +- package.json | 1 + pytest.ini | 2 +- tests/unit/confidant/authnz/authnz_test.py | 8 +++++ tests/unit/confidant/authnz/rbac_test.py | 30 +++++++++++++++++- .../unit/confidant/models/credential_test.py | 31 +++++++++++++++++++ .../confidant/routes/certificates_test.py | 2 ++ .../unit/confidant/routes/credentials_test.py | 6 ++++ tests/unit/confidant/routes/identity_test.py | 7 +++-- 23 files changed, 141 insertions(+), 18 deletions(-) diff --git a/.gitignore b/.gitignore index 063cc147..2da48923 100644 --- a/.gitignore +++ b/.gitignore @@ -31,6 +31,8 @@ pip-log.txt .tox nosetests.xml karma-test-results.xml +coverage.xml +package-lock.json # Translations *.mo diff --git a/Dockerfile b/Dockerfile index da8949f9..45a3a15c 100644 --- a/Dockerfile +++ b/Dockerfile @@ -6,7 +6,7 @@ WORKDIR /srv/confidant RUN apt-get update \ && DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \ curl ca-certificates \ - && /usr/bin/curl -sL --fail https://deb.nodesource.com/setup_8.x | bash - + && /usr/bin/curl -sL --fail https://deb.nodesource.com/setup_12.x | bash - RUN apt-get update \ && DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \ # For frontend @@ -32,10 +32,10 @@ RUN virtualenv /venv -ppython3 && \ COPY .jshintrc Gruntfile.js /srv/confidant/ COPY confidant/public /srv/confidant/confidant/public -RUN node_modules/grunt-cli/bin/grunt build - COPY . /srv/confidant +RUN node_modules/grunt-cli/bin/grunt build + EXPOSE 80 CMD ["gunicorn", "confidant.wsgi:app", "--workers=2", "-k", "gevent", "--access-logfile=-", "--error-logfile=-"] diff --git a/Makefile b/Makefile index aec983f6..b1909e6f 100644 --- a/Makefile +++ b/Makefile @@ -42,7 +42,9 @@ test_unit: clean pytest --strict --junitxml=build/unit.xml --cov=confidant --cov-report=html --cov-report=xml --cov-report=term --no-cov-on-fail tests/unit test_frontend: - node_modules/grunt-cli/bin/grunt test + npm install grunt-cli && npm install + node_modules/grunt-cli/bin/grunt build + ./node_modules/grunt-cli/bin/grunt test .PHONY: compile_deps # freeze requirements.in to requirements3.txt compile_deps: diff --git a/confidant/authnz/__init__.py b/confidant/authnz/__init__.py index 12b7893d..a3dfb79e 100644 --- a/confidant/authnz/__init__.py +++ b/confidant/authnz/__init__.py @@ -15,12 +15,13 @@ ) from confidant.authnz import userauth +import grp + _VALIDATOR = None logger = logging.getLogger(__name__) user_mod = userauth.init_user_auth_class() - def _get_validator(): global _VALIDATOR if _VALIDATOR is None: @@ -58,7 +59,6 @@ def user_is_user_type(user_type): return True return False - def user_is_service(service): if not settings.USE_AUTH: return True @@ -66,6 +66,13 @@ def user_is_service(service): return True return False +def user_in_group(groupname): + try: + group = grp.getgrnam(groupname) + except KeyError: + return False + user = get_logged_in_user() + return user in group[3] def service_in_account(account): # We only scope to account, if an account is specified. diff --git a/confidant/authnz/errors.py b/confidant/authnz/errors.py index 357951f7..45713979 100644 --- a/confidant/authnz/errors.py +++ b/confidant/authnz/errors.py @@ -1,6 +1,5 @@ # authentication / authorization related error classes - class UserUnknownError(Exception): pass diff --git a/confidant/authnz/rbac.py b/confidant/authnz/rbac.py index 707844c8..d3b07909 100644 --- a/confidant/authnz/rbac.py +++ b/confidant/authnz/rbac.py @@ -2,7 +2,7 @@ from confidant import authnz from confidant.services import certificatemanager - +from confidant.models.credential import Credential def default_acl(*args, **kwargs): """ Default ACLs for confidant: Allow access to all resource types @@ -25,6 +25,13 @@ def default_acl(*args, **kwargs): action = kwargs.get('action') resource_id = kwargs.get('resource_id') resource_kwargs = kwargs.get('kwargs') + if resource_type == 'credential' and resource_id is not None: + try: + cred = Credential.get(resource_id) + except: + return False + if cred.group is not None and not authnz.user_in_group(cred.group): + return False if authnz.user_is_user_type('user'): if resource_type == 'certificate': return False diff --git a/confidant/models/blind_credential.py b/confidant/models/blind_credential.py index 5d832293..cb67bbef 100644 --- a/confidant/models/blind_credential.py +++ b/confidant/models/blind_credential.py @@ -51,6 +51,7 @@ class Meta: modified_date = UTCDateTimeAttribute(default=datetime.now) modified_by = UnicodeAttribute() documentation = UnicodeAttribute(null=True) + group = UnicodeAttribute(null=True) def equals(self, other_cred): if self.name != other_cred.name: @@ -71,4 +72,6 @@ def equals(self, other_cred): return False if self.documentation != other_cred.documentation: return False + if self.group != other_cred.group: + return False return True diff --git a/confidant/models/credential.py b/confidant/models/credential.py index 6db38c3b..3253633e 100644 --- a/confidant/models/credential.py +++ b/confidant/models/credential.py @@ -56,6 +56,8 @@ class CredentialBase(Model): tags = ListAttribute(default=list) last_decrypted_date = UTCDateTimeAttribute(null=True) last_rotation_date = UTCDateTimeAttribute(null=True) + # if a user is not in the group, cannot interact w credential + group = UnicodeAttribute(null=True) class Credential(CredentialBase): @@ -82,6 +84,8 @@ def equals(self, other_cred): return False if set(self.tags) != set(other_cred.tags): return False + if self.group != other_cred.group: + return False return True def diff(self, other_cred): @@ -131,6 +135,8 @@ def diff(self, other_cred): 'added': new.modified_date, 'removed': old.modified_date, } + if old.group != new.group: + diff['group'] = {'added': new.group, 'removed': old.group} return diff def _diff_dict(self, old, new): @@ -228,6 +234,7 @@ def from_archive_credential(cls, archive_credential): tags=archive_credential.tags, last_decrypted_date=archive_credential.last_decrypted_date, last_rotation_date=archive_credential.last_rotation_date, + group=archive_credential.group, ) @@ -261,4 +268,5 @@ def from_credential(cls, credential): tags=credential.tags, last_decrypted_date=credential.last_decrypted_date, last_rotation_date=credential.last_rotation_date, + group=credential.group, ) diff --git a/confidant/public/modules/resources/controllers/CredentialDetailsCtrl.js b/confidant/public/modules/resources/controllers/CredentialDetailsCtrl.js index e9d3223c..38313135 100644 --- a/confidant/public/modules/resources/controllers/CredentialDetailsCtrl.js +++ b/confidant/public/modules/resources/controllers/CredentialDetailsCtrl.js @@ -106,7 +106,8 @@ enabled: true, credentialPairs: [{'key': '', 'value': ''}], mungedMetadata: [], - mungedTags: [] + mungedTags: [], + group: "", }; credentialCopy = angular.copy($scope.credential); $scope.shown = true; @@ -221,6 +222,7 @@ _credential.name = $scope.credential.name; _credential.enabled = $scope.credential.enabled; _credential.documentation = $scope.credential.documentation; + _credential.group = $scope.credential.group; _credential.credential_pairs = {}; _credential.metadata = {}; _credential.tags = []; diff --git a/confidant/public/modules/resources/views/credential-details.html b/confidant/public/modules/resources/views/credential-details.html index 36125dfc..a219532b 100644 --- a/confidant/public/modules/resources/views/credential-details.html +++ b/confidant/public/modules/resources/views/credential-details.html @@ -175,6 +175,10 @@ {{ credential.revision }} (view history) +
+ + {{ credential.group }} +
diff --git a/confidant/routes/certificates.py b/confidant/routes/certificates.py index a5d839f0..bb88972b 100644 --- a/confidant/routes/certificates.py +++ b/confidant/routes/certificates.py @@ -248,7 +248,6 @@ def list_cas(): GET /v1/cas **Example response**: - .. sourcecode:: http HTTP/1.1 200 OK @@ -310,7 +309,6 @@ def get_ca(ca): :type ca: str **Example response**: - .. sourcecode:: http HTTP/1.1 200 OK diff --git a/confidant/routes/credentials.py b/confidant/routes/credentials.py index 5aa3ed8e..0d80dcb1 100644 --- a/confidant/routes/credentials.py +++ b/confidant/routes/credentials.py @@ -76,7 +76,8 @@ def get_credential_list(): "documentation": "Example documentation", "modified_date": "2019-12-16T23:16:11.413299+00:00", "modified_by": "rlane@example.com", - "permissions": {} + "permissions": {}, + "group": "security-group" }, ... ], @@ -144,6 +145,7 @@ def get_credential(id): "documentation": "Example documentation", "modified_date": "2019-12-16T23:16:11.413299+00:00", "modified_by": "rlane@example.com", + "group": "security-group", "permissions": { "metadata": true, "get": true, @@ -611,6 +613,7 @@ def create_credential(): documentation=data.get('documentation'), tags=data.get('tags', []), last_rotation_date=last_rotation_date, + group=data.get('group', None), ).save(id__null=True) # Make this the current revision cred = Credential( @@ -627,6 +630,7 @@ def create_credential(): documentation=data.get('documentation'), tags=data.get('tags', []), last_rotation_date=last_rotation_date, + group=data.get('group', None) ) cred.save() permissions = { @@ -783,6 +787,7 @@ def update_credential(id): 'metadata': data.get('metadata', _cred.metadata), 'documentation': data.get('documentation', _cred.documentation), 'tags': data.get('tags', _cred.tags), + 'group': data.get('group', _cred.group), } # Enforce documentation, EXCEPT if we are restoring an old revision if (not update['documentation'] and @@ -850,6 +855,7 @@ def update_credential(id): documentation=update['documentation'], tags=update['tags'], last_rotation_date=update['last_rotation_date'], + group=update['group'], ).save(id__null=True) except PutError as e: logger.error(e) @@ -869,6 +875,7 @@ def update_credential(id): documentation=update['documentation'], tags=update['tags'], last_rotation_date=update['last_rotation_date'], + group=update['group'], ) cred.save() except PutError as e: @@ -1024,6 +1031,7 @@ def revert_credential_to_revision(id, to_revision): documentation=revert_credential.documentation, tags=revert_credential.tags, last_rotation_date=revert_credential.last_rotation_date, + group=revert_credential.group, ).save(id__null=True) except PutError as e: logger.error(e) @@ -1043,6 +1051,7 @@ def revert_credential_to_revision(id, to_revision): documentation=revert_credential.documentation, tags=revert_credential.tags, last_rotation_date=revert_credential.last_rotation_date, + group=revert_credential.group, ) cred.save() except PutError as e: diff --git a/confidant/schema/credentials.py b/confidant/schema/credentials.py index a045152d..badea080 100644 --- a/confidant/schema/credentials.py +++ b/confidant/schema/credentials.py @@ -22,6 +22,7 @@ class CredentialResponse(object): tags = attr.ib(default=list) last_rotation_date = attr.ib(default=None) next_rotation_date = attr.ib(default=None) + group = attr.ib(default=None) @classmethod def from_credential( @@ -42,6 +43,7 @@ def from_credential( tags=credential.tags, last_rotation_date=credential.last_rotation_date, next_rotation_date=credential.next_rotation_date, + group=credential.group, ) if include_credential_keys: ret.credential_keys = credential.credential_keys @@ -70,6 +72,7 @@ class Meta: tags = fields.List(fields.Str()) last_rotation_date = fields.DateTime() next_rotation_date = fields.DateTime() + group = fields.Str() @attr.s diff --git a/docs/README.md b/docs/README.md index 4e528982..a046abe6 100644 --- a/docs/README.md +++ b/docs/README.md @@ -18,3 +18,5 @@ The output can be found in `generated/docs`. 2. The docs are published to [docs/confidant](https://github.com/lyft/confidant.github.io/tree/master/docs/confidant) in a directory named after every tagged commit in this repo. Thus, on every tagged release there are snapped docs. + +// CRTODO: add documentation and CHANGELOG for group support diff --git a/docs/root/acls.md b/docs/root/acls.md index 5a51af13..c7232f7f 100644 --- a/docs/root/acls.md +++ b/docs/root/acls.md @@ -1,5 +1,5 @@ # Access Controls (ACLs) - +# CRTODO: add documentation ## Design The design for managing fine-grained ACLs in confidant is relatively simple. Hookpoints are called whenever a resource type will be accessed by an end-user; these hookpoints look like: diff --git a/package.json b/package.json index 02e79545..9e78c0a0 100644 --- a/package.json +++ b/package.json @@ -18,6 +18,7 @@ "angular-xeditable": "~0.1.9", "bootstrap": "3.4.1", "es5-shim": "~4.1.13", + "grunt-cli": "^1.3.2", "json3": "~3.3.2", "lodash": "~4.17.15", "spin.js": "~2.3.2" diff --git a/pytest.ini b/pytest.ini index 971ddc0a..b763b7e2 100644 --- a/pytest.ini +++ b/pytest.ini @@ -1,6 +1,6 @@ [pytest] -addopts = -p no:warnings --no-print-logs --strict +addopts = -p no:warnings --no-print-logs --strict -vvv env = SESSION_SECRET=secret DYNAMODB_CREATE_TABLE=False diff --git a/tests/unit/confidant/authnz/authnz_test.py b/tests/unit/confidant/authnz/authnz_test.py index 8cf35614..c62d8ba9 100644 --- a/tests/unit/confidant/authnz/authnz_test.py +++ b/tests/unit/confidant/authnz/authnz_test.py @@ -97,6 +97,14 @@ def test_user_is_service(mocker): assert authnz.user_is_service('notconfidant-unitttest') is False +def test_user_in_group(mocker): + my_group = ('my_group', '', 1337, ['user1']) + mocker.patch('grp.getgrnam', return_value=my_group) + mocker.patch('confidant.authnz.get_logged_in_user', return_value='user1') + assert authnz.user_in_group('my_group') + mocker.patch('confidant.authnz.get_logged_in_user', return_value='user2') + assert not authnz.user_in_group('my_group') + def test_service_in_account(mocker): # If we aren't scoping, this should pass assert authnz.service_in_account(None) is True diff --git a/tests/unit/confidant/authnz/rbac_test.py b/tests/unit/confidant/authnz/rbac_test.py index c16c6602..da4ec8b5 100644 --- a/tests/unit/confidant/authnz/rbac_test.py +++ b/tests/unit/confidant/authnz/rbac_test.py @@ -1,6 +1,8 @@ from confidant.app import create_app from confidant.authnz import rbac +from confidant.models.credential import Credential +from datetime import datetime def test_default_acl(mocker): mocker.patch('confidant.settings.USE_AUTH', True) @@ -113,7 +115,33 @@ def test_default_acl(mocker): # Test for bad user type g_mock.user_type = 'badtype' assert rbac.default_acl(resource_type='service', action='get') is False - + # Test groups + g_mock.user_type = 'user' + g_mock.username = 'test-user' + mocker.patch('confidant.authnz.user_in_group', lambda _: False) + credential = Credential( + id='1234', + revision=1, + data_type='credential', + enabled=True, + name='Test credential', + credential_pairs='akjlaklkaj==', + data_key='slkjlksfjklsdjf==', + cipher_version=2, + metadata={}, + modified_date=datetime.now(), + modified_by='test@example.com', + documentation='', + last_rotation_date=datetime(2020, 1, 1), + group='testgroup' + ) + mocker.patch( + 'confidant.routes.credentials.Credential.get', + return_value=credential, + ) + assert rbac.default_acl(resource_type='credential', resource_id='cred', action='get') is False + mocker.patch('confidant.authnz.user_in_group', lambda _: True) + assert rbac.default_acl(resource_type='credential', resource_id='cred', action='get') is True def test_no_acl(): app = create_app() diff --git a/tests/unit/confidant/models/credential_test.py b/tests/unit/confidant/models/credential_test.py index 2e19d6da..b71c4a70 100644 --- a/tests/unit/confidant/models/credential_test.py +++ b/tests/unit/confidant/models/credential_test.py @@ -15,6 +15,7 @@ def test_equals(mocker): documentation='', metadata={}, tags=['ADMIN_PRIV'], + group='testgroup', ) cred2 = Credential( name='test', @@ -22,6 +23,7 @@ def test_equals(mocker): documentation='', metadata={}, tags=['ADMIN_PRIV'], + group='testgroup', ) assert cred1.equals(cred2) is True @@ -69,6 +71,29 @@ def test_not_equals_different_tags(mocker): assert cred1.equals(cred2) is False +def test_not_equals_different_groups(mocker): + decrypted_pairs_mock = mocker.patch( + 'confidant.models.credential.Credential.decrypted_credential_pairs' + ) + decrypted_pairs_mock.return_value = {'test': 'me'} + cred1 = Credential( + name='test', + enabled=True, + documentation='', + metadata={}, + tags=['ADMIN_PRIV'], + group='g1', + ) + cred2 = Credential( + name='test', + enabled=True, + documentation='', + metadata={}, + tags=['ADMIN_PRIV'], + group='g2', + ) + assert cred1.equals(cred2) is False + def test_diff(mocker): mocker.patch( 'confidant.models.credential.Credential' @@ -87,6 +112,7 @@ def test_diff(mocker): modified_by=modified_by, modified_date=modified_date_old, tags=['FINANCIALLY_SENSITIVE', 'IMPORTANT'], + group='g1', ) new = Credential( name='test2', @@ -97,6 +123,7 @@ def test_diff(mocker): modified_by=modified_by, modified_date=modified_date_new, tags=['ADMIN_PRIV', 'IMPORTANT'], + group='g2', ) # TODO: figure out how to test decrypted_credential_pairs. Mocking # it is turning out to be difficult. @@ -125,6 +152,10 @@ def test_diff(mocker): 'removed': ['FINANCIALLY_SENSITIVE'], 'added': ['ADMIN_PRIV'], }, + 'group': { + 'removed': 'g1', + 'added': 'g2', + }, } assert old.diff(new) == expectedDiff diff --git a/tests/unit/confidant/routes/certificates_test.py b/tests/unit/confidant/routes/certificates_test.py index 094c9db4..5970ec08 100644 --- a/tests/unit/confidant/routes/certificates_test.py +++ b/tests/unit/confidant/routes/certificates_test.py @@ -8,6 +8,8 @@ def test_get_certificate(mocker): app = create_app() mocker.patch('confidant.settings.USE_AUTH', False) + # making sure that group settings for credentials don't affect certs + mocker.patch('confidant.authnz.user_in_group', lambda _: False) mocker.patch( 'confidant.authnz.get_logged_in_user', return_value='badservice', diff --git a/tests/unit/confidant/routes/credentials_test.py b/tests/unit/confidant/routes/credentials_test.py index f0e85395..0ad0600b 100644 --- a/tests/unit/confidant/routes/credentials_test.py +++ b/tests/unit/confidant/routes/credentials_test.py @@ -24,6 +24,7 @@ def credential(mocker): modified_by='test@example.com', documentation='', last_rotation_date=datetime(2020, 1, 1), + group='testgroup' ) @@ -43,6 +44,7 @@ def archive_credential(mocker): modified_by='test@example.com', documentation='', tags=['OLD TAG'], + group='archivegroup' ) @@ -62,6 +64,7 @@ def credential_list(mocker): modified_date=datetime.now(), modified_by='test@example.com', documentation='', + group='g1', ), Credential( id='5678', @@ -76,6 +79,7 @@ def credential_list(mocker): modified_date=datetime.now(), modified_by='test@example.com', documentation='', + group='g2', ), ] return credentials @@ -294,6 +298,7 @@ def test_diff_credential(mocker, credential): def test_create_credential(mocker, credential): + # CRTODO: user can create a credential with a group they are not in app = create_app() mocker.patch('confidant.settings.USE_AUTH', False) mocker.patch( @@ -381,6 +386,7 @@ def test_create_credential(mocker, credential): 'credential_pairs': {'key': 'value'}, 'name': 'shiny new key', 'tags': ['ADMIN_PRIV', 'MY_SPECIAL_TAG'], + 'group': 'g1' }), ) json_data = json.loads(ret.data) diff --git a/tests/unit/confidant/routes/identity_test.py b/tests/unit/confidant/routes/identity_test.py index 7655e91e..359134a6 100644 --- a/tests/unit/confidant/routes/identity_test.py +++ b/tests/unit/confidant/routes/identity_test.py @@ -1,6 +1,6 @@ from confidant.authnz import UserUnknownError from confidant.app import create_app - +from confidant import settings def test_get_user_info(mocker): mocker.patch('confidant.settings.USE_AUTH', False) @@ -25,7 +25,6 @@ def test_get_user_info_no_user(mocker): assert ret.status_code == 200 assert ret.json == {'email': None} - def test_get_client_config(mocker): def acl_module_check(resource_type, action): if resource_type == 'credential': @@ -59,7 +58,7 @@ def acl_module_check(resource_type, action): 'xsrf_cookie_name': 'CSRF_TOKEN', 'maintenance_mode': True, 'history_page_limit': 50, - 'defined_tags': [], + 'defined_tags': set(['ROTATION_EXCLUDED', 'FINANCIALLY_SENSITIVE']), 'permissions': { 'credentials': { 'list': True, @@ -79,5 +78,7 @@ def acl_module_check(resource_type, action): app = create_app() ret = app.test_client().get('/v1/client_config', follow_redirects=False) + ret_json = dict(ret.json) + ret_json['generated']['defined_tags'] = set(ret_json['generated']['defined_tags']) assert ret.status_code == 200 assert ret.json == expected From 5622f41adea02fb37718efa9f1ccc5132cf3640e Mon Sep 17 00:00:00 2001 From: kaisaf <10202018+kaisaf@users.noreply.github.com> Date: Thu, 16 Apr 2020 16:31:17 -0400 Subject: [PATCH 2/8] adds group field to form --- .../controllers/CredentialHistoryCtrl.js | 14 ++++++++++++- .../controllers/CredentialDetailsCtrl.js | 20 ++++++++++++------- .../resources/views/credential-details.html | 4 ++++ 3 files changed, 30 insertions(+), 8 deletions(-) diff --git a/confidant/public/modules/history/controllers/CredentialHistoryCtrl.js b/confidant/public/modules/history/controllers/CredentialHistoryCtrl.js index db589881..e8004b1e 100644 --- a/confidant/public/modules/history/controllers/CredentialHistoryCtrl.js +++ b/confidant/public/modules/history/controllers/CredentialHistoryCtrl.js @@ -54,12 +54,24 @@ $scope.noDiff = true; } }, function(res) { - $scope.getError = res.data.error; + if (res.status === 500) { + $scope.getError = 'Unexpected server error.'; + $log.error(res); + } else { + $scope.getError = res.data.error; + } }); } if ($scope.currentRevision === $scope.credentialRevision) { $scope.isCurrentRevision = true; } + }, function(res) { + if (res.status === 500) { + $scope.getError = 'Unexpected server error.'; + $log.error(res); + } else { + $scope.getError = res.data.error; + } }); $scope.shouldDisplayList = function(value) { diff --git a/confidant/public/modules/resources/controllers/CredentialDetailsCtrl.js b/confidant/public/modules/resources/controllers/CredentialDetailsCtrl.js index 38313135..f4167f4b 100644 --- a/confidant/public/modules/resources/controllers/CredentialDetailsCtrl.js +++ b/confidant/public/modules/resources/controllers/CredentialDetailsCtrl.js @@ -85,11 +85,18 @@ if ($scope.credentialId) { CredentialServices.get({'id': $scope.credentialId}).$promise.then(function(credentialServices) { $scope.credentialServices = credentialServices.services; - }); - - Credential.get({'id': $scope.credentialId, 'metadata_only': true}).$promise.then(function(credential) { - $scope.shown = false; - populateCredential(credential); + Credential.get({'id': $scope.credentialId, 'metadata_only': true}).$promise.then(function(credential) { + $scope.shown = false; + populateCredential(credential); + }, function(res) { + if (res.status === 500) { + $scope.getError = 'Unexpected server error.'; + $log.error(res); + } else { + $scope.getError = res.data.error; + } + deferred.reject(); + }); }, function(res) { if (res.status === 500) { $scope.getError = 'Unexpected server error.'; @@ -97,7 +104,6 @@ } else { $scope.getError = res.data.error; } - deferred.reject(); }); } else { // A new credential is being created @@ -107,7 +113,7 @@ credentialPairs: [{'key': '', 'value': ''}], mungedMetadata: [], mungedTags: [], - group: "", + group: '', }; credentialCopy = angular.copy($scope.credential); $scope.shown = true; diff --git a/confidant/public/modules/resources/views/credential-details.html b/confidant/public/modules/resources/views/credential-details.html index a219532b..2c6923c6 100644 --- a/confidant/public/modules/resources/views/credential-details.html +++ b/confidant/public/modules/resources/views/credential-details.html @@ -93,6 +93,10 @@
+
+ + {{ credential.group || 'Not set' }} +
From cc8b1fc0c4fabe61addebdb7d811a5d6f707d1e4 Mon Sep 17 00:00:00 2001 From: Claudia Richoux Date: Mon, 20 Apr 2020 11:39:59 -0500 Subject: [PATCH 3/8] tests work again --- Dockerfile | 5 ----- Makefile | 4 ++-- .../public/modules/resources/views/credential-details.html | 4 ---- 3 files changed, 2 insertions(+), 11 deletions(-) diff --git a/Dockerfile b/Dockerfile index 45a3a15c..8409a01c 100644 --- a/Dockerfile +++ b/Dockerfile @@ -19,9 +19,6 @@ RUN apt-get update \ COPY package.json /srv/confidant/ -RUN npm install grunt-cli && \ - npm install - COPY piptools_requirements*.txt requirements*.txt /srv/confidant/ ENV PATH=/venv/bin:$PATH @@ -34,8 +31,6 @@ COPY confidant/public /srv/confidant/confidant/public COPY . /srv/confidant -RUN node_modules/grunt-cli/bin/grunt build - EXPOSE 80 CMD ["gunicorn", "confidant.wsgi:app", "--workers=2", "-k", "gevent", "--access-logfile=-", "--error-logfile=-"] diff --git a/Makefile b/Makefile index b1909e6f..e7b335c1 100644 --- a/Makefile +++ b/Makefile @@ -11,6 +11,8 @@ down: docker-compose down docker_build: clean + npm install grunt-cli && npm install + node_modules/grunt-cli/bin/grunt build docker build -t lyft/confidant . docker_test: docker_build docker_test_unit docker_test_integration docker_test_frontend down @@ -42,8 +44,6 @@ test_unit: clean pytest --strict --junitxml=build/unit.xml --cov=confidant --cov-report=html --cov-report=xml --cov-report=term --no-cov-on-fail tests/unit test_frontend: - npm install grunt-cli && npm install - node_modules/grunt-cli/bin/grunt build ./node_modules/grunt-cli/bin/grunt test .PHONY: compile_deps # freeze requirements.in to requirements3.txt diff --git a/confidant/public/modules/resources/views/credential-details.html b/confidant/public/modules/resources/views/credential-details.html index 2c6923c6..f4801061 100644 --- a/confidant/public/modules/resources/views/credential-details.html +++ b/confidant/public/modules/resources/views/credential-details.html @@ -179,10 +179,6 @@ {{ credential.revision }} (view history)
-
- - {{ credential.group }} -
From 975b91b65adb66ff12bd14ad4b307d9d1baf2915 Mon Sep 17 00:00:00 2001 From: Claudia Richoux Date: Mon, 20 Apr 2020 13:02:50 -0500 Subject: [PATCH 4/8] fixing builds, making it work for tests --- Dockerfile | 5 +++++ Makefile | 2 +- confidant/authnz/__init__.py | 4 ++++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/Dockerfile b/Dockerfile index 8409a01c..4207b072 100644 --- a/Dockerfile +++ b/Dockerfile @@ -33,4 +33,9 @@ COPY . /srv/confidant EXPOSE 80 +# added this to test the saml stuff +RUN groupadd confidantTestGroup +RUN useradd confidant-user +RUN usermod -a -G confidantTestGroup confidant-user + CMD ["gunicorn", "confidant.wsgi:app", "--workers=2", "-k", "gevent", "--access-logfile=-", "--error-logfile=-"] diff --git a/Makefile b/Makefile index e7b335c1..aa597da2 100644 --- a/Makefile +++ b/Makefile @@ -13,7 +13,7 @@ down: docker_build: clean npm install grunt-cli && npm install node_modules/grunt-cli/bin/grunt build - docker build -t lyft/confidant . + docker build -t lyft/confidant . --no-cache docker_test: docker_build docker_test_unit docker_test_integration docker_test_frontend down diff --git a/confidant/authnz/__init__.py b/confidant/authnz/__init__.py index a3dfb79e..39ca3e03 100644 --- a/confidant/authnz/__init__.py +++ b/confidant/authnz/__init__.py @@ -66,12 +66,16 @@ def user_is_service(service): return True return False +# TODO: ASK STRIPE ABOUT THIS! +# IF GET_LOGGED_IN_USER IS AN EMAIL, WE CHECK THE GROUP MEMBERSHIP OF THE EMAIL PREFIX! def user_in_group(groupname): try: group = grp.getgrnam(groupname) except KeyError: return False user = get_logged_in_user() + if '@' in user: + user = user.split('@')[0] return user in group[3] def service_in_account(account): From e7057e314fc1846764c40c07867c62d2b1aab2fa Mon Sep 17 00:00:00 2001 From: Claudia Richoux Date: Mon, 20 Apr 2020 14:43:33 -0500 Subject: [PATCH 5/8] removing nocache from docker build --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index aa597da2..e7b335c1 100644 --- a/Makefile +++ b/Makefile @@ -13,7 +13,7 @@ down: docker_build: clean npm install grunt-cli && npm install node_modules/grunt-cli/bin/grunt build - docker build -t lyft/confidant . --no-cache + docker build -t lyft/confidant . docker_test: docker_build docker_test_unit docker_test_integration docker_test_frontend down From 54478ddca0e104b65627de553fe6c2f9a0d87633 Mon Sep 17 00:00:00 2001 From: Claudia Richoux Date: Mon, 20 Apr 2020 14:46:09 -0500 Subject: [PATCH 6/8] removing TODOs --- docs/README.md | 2 -- docs/root/acls.md | 1 - tests/unit/confidant/routes/credentials_test.py | 1 - 3 files changed, 4 deletions(-) diff --git a/docs/README.md b/docs/README.md index a046abe6..4e528982 100644 --- a/docs/README.md +++ b/docs/README.md @@ -18,5 +18,3 @@ The output can be found in `generated/docs`. 2. The docs are published to [docs/confidant](https://github.com/lyft/confidant.github.io/tree/master/docs/confidant) in a directory named after every tagged commit in this repo. Thus, on every tagged release there are snapped docs. - -// CRTODO: add documentation and CHANGELOG for group support diff --git a/docs/root/acls.md b/docs/root/acls.md index c7232f7f..b326c957 100644 --- a/docs/root/acls.md +++ b/docs/root/acls.md @@ -1,5 +1,4 @@ # Access Controls (ACLs) -# CRTODO: add documentation ## Design The design for managing fine-grained ACLs in confidant is relatively simple. Hookpoints are called whenever a resource type will be accessed by an end-user; these hookpoints look like: diff --git a/tests/unit/confidant/routes/credentials_test.py b/tests/unit/confidant/routes/credentials_test.py index 0ad0600b..2cb62842 100644 --- a/tests/unit/confidant/routes/credentials_test.py +++ b/tests/unit/confidant/routes/credentials_test.py @@ -298,7 +298,6 @@ def test_diff_credential(mocker, credential): def test_create_credential(mocker, credential): - # CRTODO: user can create a credential with a group they are not in app = create_app() mocker.patch('confidant.settings.USE_AUTH', False) mocker.patch( From 48d3dbcc08431a5114aaa17a77c31a3769c1e2bb Mon Sep 17 00:00:00 2001 From: Garret Reece Date: Fri, 1 May 2020 12:05:45 -0500 Subject: [PATCH 7/8] multi group support and updated error messages --- confidant/authnz/__init__.py | 21 ++-- confidant/authnz/rbac.py | 5 +- confidant/models/blind_credential.py | 7 +- confidant/models/credential.py | 17 ++-- .../controllers/CredentialDetailsCtrl.js | 12 ++- .../resources/views/credential-details.html | 4 +- confidant/routes/credentials.py | 96 +++++++++++++++---- confidant/schema/credentials.py | 6 +- 8 files changed, 122 insertions(+), 46 deletions(-) diff --git a/confidant/authnz/__init__.py b/confidant/authnz/__init__.py index 39ca3e03..30ef6edf 100644 --- a/confidant/authnz/__init__.py +++ b/confidant/authnz/__init__.py @@ -68,15 +68,18 @@ def user_is_service(service): # TODO: ASK STRIPE ABOUT THIS! # IF GET_LOGGED_IN_USER IS AN EMAIL, WE CHECK THE GROUP MEMBERSHIP OF THE EMAIL PREFIX! -def user_in_group(groupname): - try: - group = grp.getgrnam(groupname) - except KeyError: - return False - user = get_logged_in_user() - if '@' in user: - user = user.split('@')[0] - return user in group[3] +def user_in_groups(groupnames): + for groupname in groupnames: + try: + group = grp.getgrnam(groupname) + except KeyError: + continue + user = get_logged_in_user() + if '@' in user: + user = user.split('@')[0] + if user in group[3]: + return True + return False def service_in_account(account): # We only scope to account, if an account is specified. diff --git a/confidant/authnz/rbac.py b/confidant/authnz/rbac.py index d3b07909..f78014f6 100644 --- a/confidant/authnz/rbac.py +++ b/confidant/authnz/rbac.py @@ -30,7 +30,9 @@ def default_acl(*args, **kwargs): cred = Credential.get(resource_id) except: return False - if cred.group is not None and not authnz.user_in_group(cred.group): + if len(cred.groups) and not authnz.user_in_groups(cred.groups): + if 'error_message_handler' in kwargs: + kwargs['error_message_handler']("Not a member of any of the following groups: {}".format(cred.groups)) return False if authnz.user_is_user_type('user'): if resource_type == 'certificate': @@ -70,7 +72,6 @@ def default_acl(*args, **kwargs): # This should never happen, but paranoia wins out return False - def no_acl(*args, **kwargs): """ Stub function that always returns true This function is set by settings.py by the variable ACL_MODULE diff --git a/confidant/models/blind_credential.py b/confidant/models/blind_credential.py index cb67bbef..64f04011 100644 --- a/confidant/models/blind_credential.py +++ b/confidant/models/blind_credential.py @@ -6,7 +6,8 @@ NumberAttribute, BooleanAttribute, UTCDateTimeAttribute, - JSONAttribute + JSONAttribute, + ListAttribute ) from pynamodb.indexes import GlobalSecondaryIndex, AllProjection @@ -51,7 +52,7 @@ class Meta: modified_date = UTCDateTimeAttribute(default=datetime.now) modified_by = UnicodeAttribute() documentation = UnicodeAttribute(null=True) - group = UnicodeAttribute(null=True) + groups = ListAttribute(default=list) def equals(self, other_cred): if self.name != other_cred.name: @@ -72,6 +73,6 @@ def equals(self, other_cred): return False if self.documentation != other_cred.documentation: return False - if self.group != other_cred.group: + if set(self.groups) != set(other_cred.groups): return False return True diff --git a/confidant/models/credential.py b/confidant/models/credential.py index 3253633e..7854934c 100644 --- a/confidant/models/credential.py +++ b/confidant/models/credential.py @@ -56,8 +56,8 @@ class CredentialBase(Model): tags = ListAttribute(default=list) last_decrypted_date = UTCDateTimeAttribute(null=True) last_rotation_date = UTCDateTimeAttribute(null=True) - # if a user is not in the group, cannot interact w credential - group = UnicodeAttribute(null=True) + # if a user is not in one of the groups, cannot interact w credential + groups = ListAttribute(default=list) class Credential(CredentialBase): @@ -84,7 +84,7 @@ def equals(self, other_cred): return False if set(self.tags) != set(other_cred.tags): return False - if self.group != other_cred.group: + if set(self.groups) != set(other_cred.groups): return False return True @@ -135,8 +135,11 @@ def diff(self, other_cred): 'added': new.modified_date, 'removed': old.modified_date, } - if old.group != new.group: - diff['group'] = {'added': new.group, 'removed': old.group} + if set(old.groups) != set(new.groups): + diff['groups'] = { + 'added': list(set(new.groups) - set(old.groups)), + 'removed': list(set(old.groups) - set(new.groups)), + } return diff def _diff_dict(self, old, new): @@ -234,7 +237,7 @@ def from_archive_credential(cls, archive_credential): tags=archive_credential.tags, last_decrypted_date=archive_credential.last_decrypted_date, last_rotation_date=archive_credential.last_rotation_date, - group=archive_credential.group, + groups=archive_credential.groups, ) @@ -268,5 +271,5 @@ def from_credential(cls, credential): tags=credential.tags, last_decrypted_date=credential.last_decrypted_date, last_rotation_date=credential.last_rotation_date, - group=credential.group, + groups=credential.groups, ) diff --git a/confidant/public/modules/resources/controllers/CredentialDetailsCtrl.js b/confidant/public/modules/resources/controllers/CredentialDetailsCtrl.js index f4167f4b..5522a3f9 100644 --- a/confidant/public/modules/resources/controllers/CredentialDetailsCtrl.js +++ b/confidant/public/modules/resources/controllers/CredentialDetailsCtrl.js @@ -113,7 +113,7 @@ credentialPairs: [{'key': '', 'value': ''}], mungedMetadata: [], mungedTags: [], - group: '', + groups: [], }; credentialCopy = angular.copy($scope.credential); $scope.shown = true; @@ -228,7 +228,7 @@ _credential.name = $scope.credential.name; _credential.enabled = $scope.credential.enabled; _credential.documentation = $scope.credential.documentation; - _credential.group = $scope.credential.group; + _credential.groups = []; _credential.credential_pairs = {}; _credential.metadata = {}; _credential.tags = []; @@ -261,6 +261,14 @@ } _credential.metadata[metadataItem.key] = metadataItem.value; } + // Remove whitespace from group names + var unmungedGroups = $scope.credential.groups.split(","); + for (i = 0; i < unmungedGroups.length; i++) { + var grp = unmungedGroups[i].trim(); + if (grp.length > 0) { + _credential.groups.push(grp); + } + } for (i = $scope.credential.mungedTags.length; i--;) { var tagItem = $scope.credential.mungedTags[i]; if (tagItem.isDeleted) { diff --git a/confidant/public/modules/resources/views/credential-details.html b/confidant/public/modules/resources/views/credential-details.html index f4801061..639d63ac 100644 --- a/confidant/public/modules/resources/views/credential-details.html +++ b/confidant/public/modules/resources/views/credential-details.html @@ -94,8 +94,8 @@
- - {{ credential.group || 'Not set' }} + + {{ credential.groups.join(",") || 'Not set' }}
diff --git a/confidant/routes/credentials.py b/confidant/routes/credentials.py index 0d80dcb1..14fd7801 100644 --- a/confidant/routes/credentials.py +++ b/confidant/routes/credentials.py @@ -77,7 +77,7 @@ def get_credential_list(): "modified_date": "2019-12-16T23:16:11.413299+00:00", "modified_by": "rlane@example.com", "permissions": {}, - "group": "security-group" + "groups": [] }, ... ], @@ -88,10 +88,16 @@ def get_credential_list(): :statuscode 200: Success :statuscode 403: Client does not have permissions to list credentials. """ - if not acl_module_check(resource_type='credential', action='list'): + sub_error_msg = "" + def set_sub_error(x): + nonlocal sub_error_msg + sub_error_msg = x + if not acl_module_check(resource_type='credential', action='list', error_message_handler=set_sub_error): msg = "{} does not have access to list credentials".format( authnz.get_logged_in_user() ) + if sub_error_msg: + msg = "{} : {}".format(msg, sub_error_msg) error_msg = {'error': msg} return jsonify(error_msg), 403 @@ -145,7 +151,7 @@ def get_credential(id): "documentation": "Example documentation", "modified_date": "2019-12-16T23:16:11.413299+00:00", "modified_by": "rlane@example.com", - "group": "security-group", + "groups": ["security-group"], "permissions": { "metadata": true, "get": true, @@ -161,13 +167,20 @@ def get_credential(id): """ metadata_only = misc.get_boolean(request.args.get('metadata_only')) + sub_error_msg = "" + def set_sub_error(x): + nonlocal sub_error_msg + sub_error_msg = x if not acl_module_check(resource_type='credential', action='metadata', - resource_id=id): + resource_id=id, + error_message_handler=set_sub_error): msg = "{} does not have access to credential {}".format( authnz.get_logged_in_user(), id ) + if sub_error_msg: + msg = "{} : {}".format(msg, sub_error_msg) error_msg = {'error': msg, 'reference': id} return jsonify(error_msg), 403 @@ -304,13 +317,20 @@ def diff_credential(id, old_revision, new_revision): :statuscode 404: The provided credential ID or one of the provided revisions does not exist. """ + sub_error_msg = "" + def set_sub_error(x): + nonlocal sub_error_msg + sub_error_msg = x if not acl_module_check(resource_type='credential', action='metadata', - resource_id=id): + resource_id=id, + error_message_handler=set_sub_error): msg = "{} does not have access to diff credential {}".format( authnz.get_logged_in_user(), id ) + if sub_error_msg: + msg = "{} : {}".format(msg, sub_error_msg) error_msg = {'error': msg, 'reference': id} return jsonify(error_msg), 403 @@ -388,13 +408,20 @@ def get_archive_credential_revisions(id): metadata for the provided credential ID. :statuscode 404: The provided credential ID does not exist. """ + sub_error_msg = "" + def set_sub_error(x): + nonlocal sub_error_msg + sub_error_msg = x if not acl_module_check(resource_type='credential', action='metadata', - resource_id=id): + resource_id=id, + error_message_handler=set_sub_error): msg = "{} does not have access to credential {} revisions".format( authnz.get_logged_in_user(), id ) + if sub_error_msg: + msg = "{} : {}".format(msg, sub_error_msg) error_msg = {'error': msg} return jsonify(error_msg), 403 @@ -467,10 +494,16 @@ def get_archive_credential_list(): :statuscode 200: Success :statuscode 403: Client does not have permissions to list credentials """ - if not acl_module_check(resource_type='credential', action='list'): + sub_error_msg = "" + def set_sub_error(x): + nonlocal sub_error_msg + sub_error_msg = x + if not acl_module_check(resource_type='credential', action='list', error_message_handler=set_sub_error): msg = "{} does not have access to list credentials".format( authnz.get_logged_in_user() ) + if sub_error_msg: + msg = "{} : {}".format(msg, sub_error_msg) error_msg = {'error': msg} return jsonify(error_msg), 403 @@ -562,10 +595,16 @@ def create_credential(): correct format, or a required field was not provided. :statuscode 403: Client does not have access to create credentials. ''' - if not acl_module_check(resource_type='credential', action='create'): + sub_error_msg = "" + def set_sub_error(x): + nonlocal sub_error_msg + sub_error_msg = x + if not acl_module_check(resource_type='credential', action='create', error_message_handler=set_sub_error): msg = "{} does not have access to create credentials".format( authnz.get_logged_in_user() ) + if sub_error_msg: + msg = "{} : {}".format(msg, sub_error_msg) error_msg = {'error': msg} return jsonify(error_msg), 403 @@ -613,7 +652,7 @@ def create_credential(): documentation=data.get('documentation'), tags=data.get('tags', []), last_rotation_date=last_rotation_date, - group=data.get('group', None), + groups=data.get('groups', None), ).save(id__null=True) # Make this the current revision cred = Credential( @@ -630,7 +669,7 @@ def create_credential(): documentation=data.get('documentation'), tags=data.get('tags', []), last_rotation_date=last_rotation_date, - group=data.get('group', None) + groups=data.get('groups', []) ) cred.save() permissions = { @@ -678,11 +717,18 @@ def get_credential_dependencies(id): :statuscode 403: Client does not have permissions to get metadata for the provided credential. """ + sub_error_msg = "" + def set_sub_error(x): + nonlocal sub_error_msg + sub_error_msg = x if not acl_module_check(resource_type='credential', action='metadata', - resource_id=id): + resource_id=id, + error_message_handler=set_sub_error): msg = "{} does not have access to get dependencies for credential {}" msg = msg.format(authnz.get_logged_in_user(), id) + if sub_error_msg: + msg = "{} : {}".format(msg, sub_error_msg) error_msg = {'error': msg, 'reference': id} return jsonify(error_msg), 403 @@ -757,13 +803,20 @@ def update_credential(id): :statuscode 403: Client does not have access to update the provided credential ID. ''' + sub_error_msg = "" + def set_sub_error(x): + nonlocal sub_error_msg + sub_error_msg = x if not acl_module_check(resource_type='credential', action='update', - resource_id=id): + resource_id=id, + error_message_handler=set_sub_error): msg = "{} does not have access to update credential {}".format( authnz.get_logged_in_user(), id ) + if sub_error_msg: + msg = "{} : {}".format(msg, sub_error_msg) error_msg = {'error': msg, 'reference': id} return jsonify(error_msg), 403 @@ -787,7 +840,7 @@ def update_credential(id): 'metadata': data.get('metadata', _cred.metadata), 'documentation': data.get('documentation', _cred.documentation), 'tags': data.get('tags', _cred.tags), - 'group': data.get('group', _cred.group), + 'groups': data.get('groups', _cred.groups), } # Enforce documentation, EXCEPT if we are restoring an old revision if (not update['documentation'] and @@ -855,7 +908,7 @@ def update_credential(id): documentation=update['documentation'], tags=update['tags'], last_rotation_date=update['last_rotation_date'], - group=update['group'], + groups=update['groups'], ).save(id__null=True) except PutError as e: logger.error(e) @@ -875,7 +928,7 @@ def update_credential(id): documentation=update['documentation'], tags=update['tags'], last_rotation_date=update['last_rotation_date'], - group=update['group'], + groups=update['groups'], ) cred.save() except PutError as e: @@ -958,13 +1011,20 @@ def revert_credential_to_revision(id, to_revision): :statuscode 403: Client does not have access to revert the provided credential ID. ''' + sub_error_msg = "" + def set_sub_error(x): + nonlocal sub_error_msg + sub_error_msg = x if not acl_module_check(resource_type='credential', action='revert', - resource_id=id): + resource_id=id, + error_message_handler=set_sub_error): msg = "{} does not have access to revert credential {}".format( authnz.get_logged_in_user(), id ) + if sub_error_msg: + msg = "{} : {}".format(msg, sub_error_msg) error_msg = {'error': msg, 'reference': id} return jsonify(error_msg), 403 @@ -1031,7 +1091,7 @@ def revert_credential_to_revision(id, to_revision): documentation=revert_credential.documentation, tags=revert_credential.tags, last_rotation_date=revert_credential.last_rotation_date, - group=revert_credential.group, + groups=revert_credential.groups, ).save(id__null=True) except PutError as e: logger.error(e) @@ -1051,7 +1111,7 @@ def revert_credential_to_revision(id, to_revision): documentation=revert_credential.documentation, tags=revert_credential.tags, last_rotation_date=revert_credential.last_rotation_date, - group=revert_credential.group, + groups=revert_credential.groups, ) cred.save() except PutError as e: diff --git a/confidant/schema/credentials.py b/confidant/schema/credentials.py index badea080..c0e2f8ed 100644 --- a/confidant/schema/credentials.py +++ b/confidant/schema/credentials.py @@ -22,7 +22,7 @@ class CredentialResponse(object): tags = attr.ib(default=list) last_rotation_date = attr.ib(default=None) next_rotation_date = attr.ib(default=None) - group = attr.ib(default=None) + groups = attr.ib(default=list) @classmethod def from_credential( @@ -43,7 +43,7 @@ def from_credential( tags=credential.tags, last_rotation_date=credential.last_rotation_date, next_rotation_date=credential.next_rotation_date, - group=credential.group, + groups=credential.groups, ) if include_credential_keys: ret.credential_keys = credential.credential_keys @@ -72,7 +72,7 @@ class Meta: tags = fields.List(fields.Str()) last_rotation_date = fields.DateTime() next_rotation_date = fields.DateTime() - group = fields.Str() + groups = fields.List(fields.Str()) @attr.s From 036e93096e92922d3fce384be638b35877cc9e30 Mon Sep 17 00:00:00 2001 From: Garret Reece Date: Thu, 7 May 2020 11:35:37 -0500 Subject: [PATCH 8/8] test updates --- .../controllers/CredentialDetailsCtrl.js | 2 +- tests/unit/confidant/authnz/authnz_test.py | 6 +++--- tests/unit/confidant/authnz/rbac_test.py | 6 +++--- tests/unit/confidant/models/credential_test.py | 18 +++++++++--------- .../unit/confidant/routes/certificates_test.py | 2 +- .../unit/confidant/routes/credentials_test.py | 12 ++++++------ 6 files changed, 23 insertions(+), 23 deletions(-) diff --git a/confidant/public/modules/resources/controllers/CredentialDetailsCtrl.js b/confidant/public/modules/resources/controllers/CredentialDetailsCtrl.js index 5522a3f9..04886e5d 100644 --- a/confidant/public/modules/resources/controllers/CredentialDetailsCtrl.js +++ b/confidant/public/modules/resources/controllers/CredentialDetailsCtrl.js @@ -262,7 +262,7 @@ _credential.metadata[metadataItem.key] = metadataItem.value; } // Remove whitespace from group names - var unmungedGroups = $scope.credential.groups.split(","); + var unmungedGroups = $scope.credential.groups.split(','); for (i = 0; i < unmungedGroups.length; i++) { var grp = unmungedGroups[i].trim(); if (grp.length > 0) { diff --git a/tests/unit/confidant/authnz/authnz_test.py b/tests/unit/confidant/authnz/authnz_test.py index c62d8ba9..bbb2adc5 100644 --- a/tests/unit/confidant/authnz/authnz_test.py +++ b/tests/unit/confidant/authnz/authnz_test.py @@ -97,13 +97,13 @@ def test_user_is_service(mocker): assert authnz.user_is_service('notconfidant-unitttest') is False -def test_user_in_group(mocker): +def test_user_in_groups(mocker): my_group = ('my_group', '', 1337, ['user1']) mocker.patch('grp.getgrnam', return_value=my_group) mocker.patch('confidant.authnz.get_logged_in_user', return_value='user1') - assert authnz.user_in_group('my_group') + assert authnz.user_in_groups(['my_group']) mocker.patch('confidant.authnz.get_logged_in_user', return_value='user2') - assert not authnz.user_in_group('my_group') + assert not authnz.user_in_groups(['my_group']) def test_service_in_account(mocker): # If we aren't scoping, this should pass diff --git a/tests/unit/confidant/authnz/rbac_test.py b/tests/unit/confidant/authnz/rbac_test.py index da4ec8b5..64a02985 100644 --- a/tests/unit/confidant/authnz/rbac_test.py +++ b/tests/unit/confidant/authnz/rbac_test.py @@ -118,7 +118,7 @@ def test_default_acl(mocker): # Test groups g_mock.user_type = 'user' g_mock.username = 'test-user' - mocker.patch('confidant.authnz.user_in_group', lambda _: False) + mocker.patch('confidant.authnz.user_in_groups', lambda _: False) credential = Credential( id='1234', revision=1, @@ -133,14 +133,14 @@ def test_default_acl(mocker): modified_by='test@example.com', documentation='', last_rotation_date=datetime(2020, 1, 1), - group='testgroup' + groups=['testgroup'] ) mocker.patch( 'confidant.routes.credentials.Credential.get', return_value=credential, ) assert rbac.default_acl(resource_type='credential', resource_id='cred', action='get') is False - mocker.patch('confidant.authnz.user_in_group', lambda _: True) + mocker.patch('confidant.authnz.user_in_groups', lambda _: True) assert rbac.default_acl(resource_type='credential', resource_id='cred', action='get') is True def test_no_acl(): diff --git a/tests/unit/confidant/models/credential_test.py b/tests/unit/confidant/models/credential_test.py index b71c4a70..0b51d82f 100644 --- a/tests/unit/confidant/models/credential_test.py +++ b/tests/unit/confidant/models/credential_test.py @@ -15,7 +15,7 @@ def test_equals(mocker): documentation='', metadata={}, tags=['ADMIN_PRIV'], - group='testgroup', + groups=['testgroup'], ) cred2 = Credential( name='test', @@ -23,7 +23,7 @@ def test_equals(mocker): documentation='', metadata={}, tags=['ADMIN_PRIV'], - group='testgroup', + groups=['testgroup'], ) assert cred1.equals(cred2) is True @@ -82,7 +82,7 @@ def test_not_equals_different_groups(mocker): documentation='', metadata={}, tags=['ADMIN_PRIV'], - group='g1', + groups=['g1'], ) cred2 = Credential( name='test', @@ -90,7 +90,7 @@ def test_not_equals_different_groups(mocker): documentation='', metadata={}, tags=['ADMIN_PRIV'], - group='g2', + groups=['g2'], ) assert cred1.equals(cred2) is False @@ -112,7 +112,7 @@ def test_diff(mocker): modified_by=modified_by, modified_date=modified_date_old, tags=['FINANCIALLY_SENSITIVE', 'IMPORTANT'], - group='g1', + groups=['g1'], ) new = Credential( name='test2', @@ -123,7 +123,7 @@ def test_diff(mocker): modified_by=modified_by, modified_date=modified_date_new, tags=['ADMIN_PRIV', 'IMPORTANT'], - group='g2', + groups=['g2'], ) # TODO: figure out how to test decrypted_credential_pairs. Mocking # it is turning out to be difficult. @@ -152,9 +152,9 @@ def test_diff(mocker): 'removed': ['FINANCIALLY_SENSITIVE'], 'added': ['ADMIN_PRIV'], }, - 'group': { - 'removed': 'g1', - 'added': 'g2', + 'groups': { + 'removed': ['g1'], + 'added': ['g2'], }, } assert old.diff(new) == expectedDiff diff --git a/tests/unit/confidant/routes/certificates_test.py b/tests/unit/confidant/routes/certificates_test.py index 5970ec08..17e4788b 100644 --- a/tests/unit/confidant/routes/certificates_test.py +++ b/tests/unit/confidant/routes/certificates_test.py @@ -9,7 +9,7 @@ def test_get_certificate(mocker): mocker.patch('confidant.settings.USE_AUTH', False) # making sure that group settings for credentials don't affect certs - mocker.patch('confidant.authnz.user_in_group', lambda _: False) + mocker.patch('confidant.authnz.user_in_groups', lambda _: False) mocker.patch( 'confidant.authnz.get_logged_in_user', return_value='badservice', diff --git a/tests/unit/confidant/routes/credentials_test.py b/tests/unit/confidant/routes/credentials_test.py index 2cb62842..744c198d 100644 --- a/tests/unit/confidant/routes/credentials_test.py +++ b/tests/unit/confidant/routes/credentials_test.py @@ -24,7 +24,7 @@ def credential(mocker): modified_by='test@example.com', documentation='', last_rotation_date=datetime(2020, 1, 1), - group='testgroup' + groups=['testgroup'] ) @@ -44,7 +44,7 @@ def archive_credential(mocker): modified_by='test@example.com', documentation='', tags=['OLD TAG'], - group='archivegroup' + groups=['archivegroup'] ) @@ -64,7 +64,7 @@ def credential_list(mocker): modified_date=datetime.now(), modified_by='test@example.com', documentation='', - group='g1', + groups=['g1'], ), Credential( id='5678', @@ -79,7 +79,7 @@ def credential_list(mocker): modified_date=datetime.now(), modified_by='test@example.com', documentation='', - group='g2', + groups=['g2'], ), ] return credentials @@ -132,7 +132,7 @@ def test_get_credential(mocker, credential): ret = app.test_client().get('/v1/credentials/1234', follow_redirects=False) assert ret.status_code == 403 - def acl_module_check(resource_type, action, resource_id): + def acl_module_check(resource_type, action, resource_id, **kwargs): if action == 'metadata': if resource_id == '5678': return False @@ -385,7 +385,7 @@ def test_create_credential(mocker, credential): 'credential_pairs': {'key': 'value'}, 'name': 'shiny new key', 'tags': ['ADMIN_PRIV', 'MY_SPECIAL_TAG'], - 'group': 'g1' + 'groups': ['g1'] }), ) json_data = json.loads(ret.data)