Adding group attribute to credentials #289
Conversation
a credential's group to interact with a credential
…g but the encrypted data
… credential's group
…oup, and credentials with no group can be edited by any user
… (NoneType is not iterable)
…ted by the datamodel)
…t into stripe-elewis/group-authz
| or not app.config.get('USE_GROUPS') | ||
| or not groupname | ||
| or user_is_member(groupname) | ||
| ) |
There was a problem hiding this comment.
This should be implemented as an ACL check against actions of a resource: https://lyft.github.io/confidant/acls.html
|
|
||
| if app.config.get('USE_GROUPS'): | ||
| all_groups = grp.getgrall() | ||
| session['user']['groups'] = [g.gr_name for g in all_groups if email in g.gr_mem] |
There was a problem hiding this comment.
I think it would be good if fetching groups was something modular, as it's possible that some folks will get the user's group from a SAML assertion, some will get it from a header, and others may want to use unix groups like this.
| groups = self.current_user().get('groups') | ||
| if groups is None: | ||
| return [] | ||
| else: | ||
| return groups |
There was a problem hiding this comment.
| groups = self.current_user().get('groups') | |
| if groups is None: | |
| return [] | |
| else: | |
| return groups | |
| groups = self.current_user().get('groups', []) |
| request.headers[self.email_header], | ||
| first_name = first_name, | ||
| last_name = last_name, | ||
| username = username |
There was a problem hiding this comment.
first_name and last_name here are changing their defaults from '' to None. Will this cause typing issues anywhere?
| id="credentialGroupInput"> | ||
| {{ credential.group || 'No group set' }} | ||
| </span> | ||
| <span ng-show="credential.id && !credential.authorized">YOU ARE NOT AUTHORIZED TO VIEW OR MODIFY THIS CREDENTIAL'S DETAILS</span> |
There was a problem hiding this comment.
We return permission hints from individual resource level endpoints, for ACL checks; see: https://lyft.github.io/confidant/api.html#get--v1-credentials-(id)
| except authnz.UserUnknownError: | ||
| response = jsonify({'email': None}) | ||
| response = jsonify({'email': None, 'groups': None}) |
There was a problem hiding this comment.
I'm confused about this diff, because confidant/routes/v1.py doesn't exist anymore. It's been split up into files specific to the types of endpoints.
| if len(unauthorized_creds) > 0: | ||
| msg = 'User not authorized to modify credentials: {0}' | ||
| ret = {'error': msg.format(", ".join(unauthorized_creds))} | ||
| return jsonify(ret), 403 |
There was a problem hiding this comment.
This should be built as an ACL check via the new built-in ACL support. This logic looks a bit custom to your organization, so it could be written as a custom ACL module.
| cipher_version = cred.cipher_version | ||
| cipher = CipherManager(data_key, cipher_version) | ||
| _credential_pairs = cipher.decrypt(cred.credential_pairs) | ||
| _credential_pairs = json.loads(_credential_pairs) |
There was a problem hiding this comment.
The newer version of the code handles this logic currently. It will only deliver decrypted credentials if the correct ACL permissions exist.
| data = request.get_json() | ||
| if not data.get('documentation') and settings.get('ENFORCE_DOCUMENTATION'): | ||
| return jsonify({'error': 'documentation is a required field'}), 400 | ||
| if not data.get('credential_pairs'): | ||
| return jsonify({'error': 'credential_pairs is a required field'}), 400 | ||
| if not isinstance(data.get('metadata', {}), dict): | ||
| return jsonify({'error': 'metadata must be a dict'}), 400 | ||
| if app.config.get('USE_GROUPS'): | ||
| if data.get('group') and not authnz.user_is_member(data.get('group')): | ||
| return jsonify({'error': 'Must be a member of the destination group'}), 400 |
There was a problem hiding this comment.
This should be part of a custom ACL check.
| # of the new group. Setting the group to empty is always permitted | ||
| if app.config.get('USE_GROUPS'): | ||
| if update['group'] and not authnz.user_is_member(update['group']): | ||
| return jsonify({'error': 'Must be a member of the destination group'}), 400 |
| @@ -38,6 +38,15 @@ | |||
| <label for="credentialEnabled">Credential Enabled</label> | |||
| <span editable-checkbox="credential.enabled" id="credentialEnabled">{{ credential.enabled }}</span> | |||
| </div> | |||
| <div class="form-group"> | |||
There was a problem hiding this comment.
Since there's a server-side setting for whether groups are enabled or not, it would be good to only display this section if it's enabled. You can send the configuration option back to the client via the client config (https://github.com/lyft/confidant/blob/master/confidant/routes/identity.py#L127-L138). You can access the client config in the view and controller. It's already pulled into the scope; see this for example: https://github.com/lyft/confidant/blob/master/confidant/public/modules/resources/controllers/CredentialDetailsCtrl.js#L52-L53
|
Going to redo this to work with the current version- branches diverged too much. |
This PR adds a group field to credentials and users. If a user is not in the same group as a credential, they will not be able to interact with that credential at all.
Not ready to merge currently- the branches diverged quite a bit in the past few months, and this feature is not completely integrated with credential archiving, tests, the UI, or code to check whether a credential has certain permissions. Currently working on this and will have it ready to go shortly.