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

Support for SAML roles (readonly / write access for credentials) #188

Open
wants to merge 17 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@jrosco
Copy link

commented Jun 5, 2019

Hi, The idea behind this PR is to allow setting SAML roles in the credential metadata which would allow a user with correct SAML 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 SAML_ADMIN_ROLE)
  • Able to disable/enable SAML role permissions (This is set up in the env config file SAML_USE_ROLE)
  • Show the users role on the Web GUI
  • Able to view role via the API (/v1/user/role)
  • Add roles to the credentials metadata (These are set up in the env config file SAML_ROLE_RW_NAME and SAML_ROLE_R_NAME )
  • Add the SAML Role name (This is set up in the env config file SAML_MAPPING_ROLE)
  • Added unittest

We currently have this setup in our production environment and it's working with no known issues

jrosco and others added some commits Apr 24, 2019

added group support to settings
SAML_GROUPS = Enable group support
SAML_ADMIN_GROUP = The Admin Group (Permissionless Access)
SAML_TEST_GROUP = test user removed later (this should come from SAML
creds)
added some try exception statments
more work with adding group support
Merged in feature/PE-5760_add_group_support_to_confidant (pull request
…#1)

updated to use readonly and read/write group permission logic

remove duplicate exception

typos

Approved-by: Edward Kim <edward.kim@fairfaxmedia.com.au>
Merged in feature/userauth.py-saml-role-changes (pull request #2)
PE-5760 hanges to userauth.py to insert SAML role

Signed-off-by: ed kim <edward.kim@fairfaxmedia.com.au>

Approved-by: Joel Cumberland <joel.cumberland@fairfaxmedia.com.au>
Merged in feature/display-role-in-ui (pull request #4)
PE-5760 Display the role in the Confidant UI

Signed-off-by: ed kim <edward.kim@fairfaxmedia.com.au>

Approved-by: Joel Cumberland <joel.cumberland@fairfaxmedia.com.au>
Merged in fix/if_no_group_everyone_can_view (pull request #5)
If no groups allow read/write

* Display the role in the Confidant UI

    Signed-off-by: ed kim <edward.kim@fairfaxmedia.com.au>

* if no groups allow read/write

    Read only is set by default
    If no groups allow all to read/write by default
    If only readonly exist allow others to read/write by default

* Extract the user's group/role from saml

    Signed-off-by: ed kim <edward.kim@fairfaxmedia.com.au>

* added the lower() function as the keys must be lower case

    Signed-off-by: ed kim <edward.kim@fairfaxmedia.com.au>

* fixed up if statements logic

* moved the lower() functions around. applied to the admin group as well

    Signed-off-by: ed kim <edward.kim@fairfaxmedia.com.au>

* remove mockup user

* Fixed the case when we have 'None' for the role value and we try run the lower() function

    Added a mapping via environment variable to settings

    Signed-off-by: ed kim <edward.kim@fairfaxmedia.com.au>

* return 'no_role' for a list

    Signed-off-by: ed kim <edward.kim@fairfaxmedia.com.au>

* removed service mapping permission - testing only

Approved-by: Edward Kim <edward.kim@fairfaxmedia.com.au>
Merged in cleanup/renaming_remove_debug_stuff (pull request #8)
Cleanup/renaming remove debug stuff

* Display the role in the Confidant UI

    Signed-off-by: ed kim <edward.kim@fairfaxmedia.com.au>

* if no groups allow read/write

    Read only is set by default
    If no groups allow all to read/write by default
    If only readonly exist allow others to read/write by default

* Extract the user's group/role from saml

    Signed-off-by: ed kim <edward.kim@fairfaxmedia.com.au>

* added the lower() function as the keys must be lower case

    Signed-off-by: ed kim <edward.kim@fairfaxmedia.com.au>

* fixed up if statements logic

* moved the lower() functions around. applied to the admin group as well

    Signed-off-by: ed kim <edward.kim@fairfaxmedia.com.au>

* remove mockup user

* Fixed the case when we have 'None' for the role value and we try run the lower() function

    Added a mapping via environment variable to settings

    Signed-off-by: ed kim <edward.kim@fairfaxmedia.com.au>

* return 'no_role' for a list

    Signed-off-by: ed kim <edward.kim@fairfaxmedia.com.au>

* removed service mapping permission - testing only

* renaming

* code cleanup

* more code cleanups

* refactoring

* reverted README

* reverted some files

Approved-by: Edward Kim <edward.kim@fairfaxmedia.com.au>
updated http return codes
changed to http Forbidden
Merged in unittest/authnz (pull request #9)
PE-6026 - Added test_require_saml_role unittest

* added saml_required unittest

* test for multiplue roles name (with spaces)

    support spaces in role names

Approved-by: Michael Lorant <michael.lorant@fairfaxmedia.com.au>
Approved-by: Mark Walford <mark.walford@fairfaxmedia.com.au>
Approved-by: Frode Egeland <frode.egeland@fairfaxmedia.com.au>
added some more checks to unittest
made readonly false by default

fixed indentation
@ryan-lane

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

Hey, thanks for the PR! Can you sign the CLA? I'll start taking a look through the PR :)

@@ -50,14 +49,24 @@ def get_user_info():
response = jsonify({'email': None})
return response

@app.route('/v1/user/role', methods=['GET', 'POST'])
@authnz.require_auth
def get_role_info():

This comment has been minimized.

Copy link
@ryan-lane

ryan-lane Jun 12, 2019

Contributor

Any reason not to include this info in the get_user_info() endpoint?

This comment has been minimized.

Copy link
@jrosco

jrosco Jun 25, 2019

Author

Yeah, I think that would be a better place for this endpoint

@@ -374,6 +383,10 @@ def get_credential(id):
'Item with id {0} does not exist.'.format(id)
)
return jsonify({}), 404

if not authnz.require_saml_role(cred.metadata)['read_only']:

This comment has been minimized.

Copy link
@ryan-lane

ryan-lane Jun 12, 2019

Contributor

I think it's maybe better to have this as a decorator that can be applied to endpoints, rather than being checked in-function.

For example:

@authnz.require_role('read_only')

This comment has been minimized.

Copy link
@jrosco

jrosco Jun 25, 2019

Author

I will work on this

@@ -780,6 +794,10 @@ def update_credential(id):
credential_pairs = cipher.encrypt(update['credential_pairs'])
update['metadata'] = data.get('metadata', _cred.metadata)
update['documentation'] = data.get('documentation', _cred.documentation)

if not authnz.require_saml_role(_cred.metadata)['read_write']:
return jsonify({'error': 'Credential Metadata found Role does not have write permissions'}), 403

This comment has been minimized.

Copy link
@ryan-lane

ryan-lane Jun 12, 2019

Contributor

Same as above. Would be good to make a decorator for this.

This comment has been minimized.

Copy link
@ryan-lane

ryan-lane Jun 12, 2019

Contributor

I think you also need to apply this to create_credential(), and map_service_credentials()

@@ -238,6 +245,52 @@ def decorated(*args, **kwargs):
return decorated


def require_saml_role(credential):

This comment has been minimized.

Copy link
@ryan-lane

ryan-lane Jun 12, 2019

Contributor

Would be nice for this to be require_role, rather than require_saml_role, so other modules can implement it, if necessary.

This comment has been minimized.

Copy link
@jrosco

jrosco Jun 25, 2019

Author

Agreed

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