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.

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

Adding group attribute to credentials #289

Closed
wants to merge 22 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
af60c13
credentials now have a group attribute
GarretReece Oct 11, 2018
7504048
WIP. If USE_GROUPS is set to true, users must be a memmebr of
GarretReece Oct 30, 2018
b7ba9e6
corrected logic for setting group, including setting empty group
GarretReece Nov 8, 2018
e1b5b2b
dropdown box on credentials now lists user's groups properly
GarretReece Dec 4, 2018
1600761
credentials that a user isn't authorized to see will display everythi…
GarretReece Dec 4, 2018
2c6906d
Edit button disabled for a credential if a user isn't a member of the…
GarretReece Dec 5, 2018
e9703b9
A warning is displayed if a user is not a member of a credential's gr…
GarretReece Dec 5, 2018
77ce22c
The unauthorized user warning no longer displays while creating a cre…
GarretReece Dec 6, 2018
e9b0250
Return an empty list as default so downstream group checks don't fail…
elewis-stripe Jun 5, 2019
7e57226
Ensure group list is included when using header authentication
elewis-stripe Jun 10, 2019
5b64932
Ensure username field exists
elewis-stripe Jun 10, 2019
735330f
Actually set username and given names as expected
elewis-stripe Jun 10, 2019
b760ece
Apply authz check to other places that credentials are returned
elewis-stripe Jun 10, 2019
2d23faf
Allow services to bypass group authz checks (they are already restric…
elewis-stripe Jun 10, 2019
969cc6d
Allow NullUserAuthenticator to inherit abstract methods for checking …
elewis-stripe Jun 11, 2019
d54455e
Ensure returned credential pairs are a consistent type even if unauth…
elewis-stripe Jun 11, 2019
3fc761a
Perform authorization check when un/assigning creds to a service
elewis-stripe Jun 11, 2019
5c42671
Improve error message when unauthorized
elewis-stripe Jun 12, 2019
fb42943
Use standardized authz check and error message
elewis-stripe Jun 12, 2019
059f440
Merge branch 'master' into group_authz
GarretReece Oct 2, 2019
8ba1be5
fix for some copy paste error that crept in
GarretReece Oct 2, 2019
8dee816
Merge branch 'elewis/group-authz' of git://github.com/stripe/confidan…
Apr 3, 2020
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
23 changes: 23 additions & 0 deletions confidant/authnz/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,16 @@ def get_logged_in_user():
raise UserUnknownError()


def get_user_groups():
if user_mod.is_authenticated():
return user_mod.current_groups()
raise UserUnknownError()


def user_is_member(groupname):
return groupname in get_user_groups()


def user_is_user_type(user_type):
if not app.config.get('USE_AUTH'):
return True
Expand Down Expand Up @@ -90,6 +100,19 @@ def user_type_has_privilege(user_type, privilege):
return False


def user_is_authorized(groupname):
# Fail open if:
# - the principal is not a human user
# - authz checks are disabled globally
# - OR there is no group specified
return (
not user_is_user_type('user')
or not app.config.get('USE_GROUPS')
or not groupname
or user_is_member(groupname)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be implemented as an ACL check against actions of a resource: https://lyft.github.io/confidant/acls.html



def require_csrf_token(f):
@wraps(f)
def decorated(*args, **kwargs):
Expand Down
53 changes: 39 additions & 14 deletions confidant/authnz/userauth.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import logging
import datetime
import random
import grp

import yaml
from six.moves.urllib.parse import urlparse
Expand Down Expand Up @@ -113,13 +114,24 @@ def set_expiration(self):
max_expiration = now + datetime.timedelta(seconds=max_lifetime)
session['max_expiration'] = max_expiration

def set_current_user(self, email, first_name=None, last_name=None):
def set_current_user(self, email, first_name=None, last_name=None, username=None):
if email is None or email == '':
raise errors.UserUnknownError('No email provided')

session['user'] = {
'email': email,
'first_name': first_name,
'last_name': last_name,
'groups': []
}

if username is None:
username = email.strip().split('@')[0]

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]
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 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.


def current_email(self):
ret = self.current_user()['email'].lower()
# when migrating from 2 -> 3, the session email object may be bytes
Expand All @@ -131,6 +143,13 @@ def current_first_name(self):
def current_last_name(self):
return self.current_user()['last_name']

def current_groups(self):
groups = self.current_user().get('groups')
if groups is None:
return []
else:
return groups
Comment on lines +147 to +151
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
groups = self.current_user().get('groups')
if groups is None:
return []
else:
return groups
groups = self.current_user().get('groups', [])


def redirect_to_index(self):
return redirect(flask.url_for('index'))

Expand Down Expand Up @@ -251,6 +270,7 @@ def current_user(self):
'email': 'unauthenticated user',
'first_name': 'unauthenticated',
'last_name': 'user',
'groups': []
}

def is_authenticated(self):
Expand Down Expand Up @@ -299,22 +319,27 @@ def assert_headers(self):
def current_user(self):
self.assert_headers()

info = {
'email': request.headers[self.email_header],

# TODO: should we use a string like "unknown", fall back to the
# email/username, ...?
'first_name': '',
'last_name': '',
}

first_name = None
if self.first_name_header and self.first_name_header in request.headers:
info['first_name'] = request.headers[self.first_name_header]
first_name = request.headers[self.first_name_header]

last_name = None
if self.last_name_header and self.last_name_header in request.headers:
info['last_name'] = request.headers[self.last_name_header]

return info
last_name = request.headers[self.last_name_header]

username = None
if self.username_header and self.username_header in request.headers:
username = request.headers[self.username_header]

# Set current user on every request to ensure that the user's group
# list is updated (TODO: caching?)
self.set_current_user(
request.headers[self.email_header],
first_name = first_name,
last_name = last_name,
username = username
Copy link
Contributor

Choose a reason for hiding this comment

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

first_name and last_name here are changing their defaults from '' to None. Will this cause typing issues anywhere?

)
return session['user']

def is_authenticated(self):
"""Any user that is able to make requests is authenticated"""
Expand Down
1 change: 1 addition & 0 deletions confidant/models/credential.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,4 @@ class Meta:
modified_date = UTCDateTimeAttribute(default=datetime.now)
modified_by = UnicodeAttribute()
documentation = UnicodeAttribute(null=True)
group = UnicodeAttribute(null=True)
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,10 @@
$scope.shown = true;
}

$scope.groups = function(usergroups) {
return [""].concat(usergroups);
}

$scope.showValue = function(credentialPair) {
if (credentialPair.shown) {
return credentialPair.value;
Expand Down Expand Up @@ -154,6 +158,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 = {};
$scope.saveError = '';
Expand Down
11 changes: 10 additions & 1 deletion confidant/public/modules/resources/views/credential-details.html
Original file line number Diff line number Diff line change
Expand Up @@ -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">
Copy link
Contributor

Choose a reason for hiding this comment

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

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

<label for="credentialGroup">Credential Group</label>
<span editable-select="credential.group"
e-ng-options="x for x in groups( {{user.groups}} )"
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>
Copy link
Contributor

Choose a reason for hiding this comment

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

We return permission hints from individual resource level endpoints, for ACL checks; see: https://lyft.github.io/confidant/api.html#get--v1-credentials-(id)

</div>
<div class="form-group">
<label for="credentialPairInputs">Credential Pairs <span class="glyphicon glyphicon-lock"></span></label>
<div class="well well-sm" id="credentialPairInputs">
Expand Down Expand Up @@ -148,7 +157,7 @@
</div>

<div class="buttons has-margin-bottom-lg">
<button type="button" class="btn btn-default" ng-click="editableForm.$show()" ng-show="!editableForm.$visible">Edit</button>
<button type="button" class="btn btn-default" ng-click="editableForm.$show()" ng-show="!editableForm.$visible && credential.authorized">Edit</button>
<span ng-show="editableForm.$visible">
<button type="submit" class="btn" ng-disabled="editableForm.$waiting">Save</button>
<button type="button" class="btn btn-alternate" ng-disabled="editableForm.$waiting" ng-click="editableForm.$cancel()">Cancel</button>
Expand Down
Loading