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

Feature/role support #199

Open
wants to merge 8 commits into
base: master
from

Conversation

@jrosco
Copy link

commented Sep 23, 2019

The idea behind this PR is to allow setting roles (SAML for now) in the credential metadata which would allow a user with correct role settings read-only or write access to certain credentials.

A list of features.

  • Setup an Admin role to have access to all credentials (This is set up in the env config file ADMIN_ROLE)
  • Able to disable/enable role permissions (This is set up in the env config file USE_ROLE)
  • Show the users role on the Web GUI (top right-hand corner)
  • Add roles to the credentials metadata (These are set up in the env config file ROLE_RW_NAME and ROLE_R_NAME )
  • Add the Role name attribute (This is set up in the env config file ROLE_MAPPING_NAME)
  • Combine user email and role into one single API endpoint /v1/user/info
  • Able to view role via the API (/v1/user/info)

How to use:
The roles are set up and mapped in the credentials metadata (shown below)

Screenshot at Sep 24 12-51-55

role_ro = read only
role_rw = read write

So in the above example, the user has read-only access and the engineer and engineer-manager have read-write access

N/B There is an Admin role which allows full access to all credentials. This can be set with the ADMIN_ROLE environment variable.

To Do:

  • Add python unit test
  • Check if we can add the require_role decorator to resources/service
  • Support multiple groups for a single user
  • Test/Check if this can work with other Auth methods .e.g Goggle, etc
jrosco added 6 commits Sep 23, 2019
* Added new "get_logged_in_user_role()" and "current_role()" functions

* Added new "require_role()" function does all the heavy work checking
if user is allowed to veiw/edit creds

* Updated endpoint /v1/user/email to be /v1/user/info which contains the
user email and role, this is used in the index.html file to show details
in top right corner

* Added settings for role support (enable/disable, property names etc)
cleanup lint errors
use kwargs with set_current_user() function (cleaner)
use double quotes
reduce line char count too long lint error
@jrosco jrosco marked this pull request as ready for review Sep 24, 2019
@wraps(f)
def wrapper(*args, **kwargs):
cred_id = kwargs['id']
cred = Credential.get(cred_id)

This comment has been minimized.

Copy link
@ryan-lane

ryan-lane Sep 24, 2019

Contributor

You should defer getting the credential until necessary, otherwise it adds load on the system, even when USE_ROLES is disabled, for example

This comment has been minimized.

Copy link
@ryan-lane

ryan-lane Sep 24, 2019

Contributor

Or, as mentioned in another comment, change this to a function (not a decorator) which takes metadata as an argument, which will let you avoid doing a get on the credential.

@@ -366,6 +370,7 @@ def get_credential_list():

@app.route('/v1/credentials/<id>', methods=['GET'])
@authnz.require_auth
@authnz.require_role(role='read_only')

This comment has been minimized.

Copy link
@ryan-lane

ryan-lane Sep 24, 2019

Contributor

Since you need to check against a resource, it may be better to have this as a function call inside of the function, rather than as a decorator, as you'll be able to pass-in the credential model instance, rather than doing a double fetch.

This comment has been minimized.

Copy link
@ryan-lane

ryan-lane Sep 24, 2019

Contributor

Also, if you do it as a function call, you could make it generic and only pass in metadata, which would make the function work with anything that has metadata (credentials, services, blind credentials, etc).

This comment has been minimized.

Copy link
@jrosco

jrosco Sep 27, 2019

Author

do you mean a function call within the get_credential() and update_credential() functions ?

example:

authnz.require_role(Credential.metadata)

This comment has been minimized.

Copy link
@ryan-lane

ryan-lane Oct 3, 2019

Contributor

yep!

@@ -113,11 +113,12 @@ 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, **kwargs):

This comment has been minimized.

Copy link
@ryan-lane

ryan-lane Sep 24, 2019

Contributor

Prefer adding role=None to the kwargs, than using **kwargs. This change as currently made makes all arguments required.

This comment has been minimized.

Copy link
@jrosco

jrosco Sep 26, 2019

Author

fixed

if user_role is not None:
if user_role == app.config['ADMIN_ROLE'].lower():
return make_response(f(*args, **kwargs))
if role_name_rw in cred.metadata:

This comment has been minimized.

Copy link
@ryan-lane

ryan-lane Sep 24, 2019

Contributor

nit: elif

This comment has been minimized.

Copy link
@jrosco

jrosco Sep 26, 2019

Author

added elif

groups_rw = groups_rw.replace(' ', '').split(',')
if user_role in groups_rw:
return make_response(f(*args, **kwargs))
if role_name_r in cred.metadata:

This comment has been minimized.

Copy link
@ryan-lane

ryan-lane Sep 24, 2019

Contributor

nit: elif

This comment has been minimized.

Copy link
@jrosco

jrosco Sep 26, 2019

Author

added elif

role_name_rw = app.config['ROLE_RW_NAME'].lower()
role_name_r = app.config['ROLE_RO_NAME'].lower()
groups_rw = cred.metadata.get(role_name_rw)
groups_r = cred.metadata.get(role_name_r)

This comment has been minimized.

Copy link
@ryan-lane

ryan-lane Sep 24, 2019

Contributor

Should each of these be .lower() as well? Pretty sure we don't force these to lowercase in the model.

This comment has been minimized.

Copy link
@jrosco

jrosco Sep 26, 2019

Author

fixed below

@@ -131,6 +132,12 @@ def current_first_name(self):
def current_last_name(self):
return self.current_user()['last_name']

def current_role(self):
if type(self.current_user()['role']) is list:

This comment has been minimized.

Copy link
@ryan-lane

ryan-lane Sep 24, 2019

Contributor

Doing a type check here seems odd. Shouldn't this be: if not self.current_user()['role']:

This comment has been minimized.

Copy link
@jrosco

jrosco Sep 26, 2019

Author

fixed

jrosco added 2 commits Sep 24, 2019
use lowercase for the group names in credential metadata

removed **kwargs and updated current_role function

Removed the **kwargs in set_current_user function
Update current_role function to return role "not set" message

liniting

add elif and nested elif statements

liniting
if groups_rw is not None:
groups_rw = groups_rw.lower()
groups_rw = groups_rw.replace(' ', '').split(',')

This comment has been minimized.

Copy link
@jrosco

jrosco Sep 26, 2019

Author

added lower() function and split and removed white space functions

@@ -280,7 +280,8 @@ def wrapper(*args, **kwargs):
or role_name_r in cred.metadata:
if groups_rw and user_role in groups_rw:
return make_response(f(*args, **kwargs))
elif groups_r and user_role in groups_r:
elif groups_r and user_role in groups_r \

This comment has been minimized.

Copy link
@ryan-lane

ryan-lane Oct 3, 2019

Contributor

Use parens, rather than backslash

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.