From d75257ef0faadf8d5b0e02ae9063ea3506628c09 Mon Sep 17 00:00:00 2001 From: Alexander VT Date: Thu, 10 Jun 2021 11:12:08 -0500 Subject: [PATCH 1/3] feat(email): populate email from IdPs, update deprecated .warn to .warning --- .pre-commit-config.yaml | 2 +- fence/auth.py | 18 +++++++++++++++--- fence/blueprints/login/__init__.py | 8 ++++---- fence/blueprints/login/base.py | 12 ++++++++---- fence/blueprints/login/fence_login.py | 2 ++ fence/resources/audit_service_client.py | 2 +- fence/resources/openid/idp_oauth2.py | 2 +- fence/sync/sync_users.py | 2 +- tests/oidc/discovery/test_configuration.py | 2 +- 9 files changed, 34 insertions(+), 16 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index a29604a6e0..d2206399e6 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -13,6 +13,6 @@ repos: - id: no-commit-to-branch args: [--branch, develop, --branch, master, --pattern, release/.*] - repo: https://github.com/psf/black - rev: 21.5b1 + rev: 21.5b2 hooks: - id: black diff --git a/fence/auth.py b/fence/auth.py index b7e86b729c..929cee67c0 100644 --- a/fence/auth.py +++ b/fence/auth.py @@ -57,7 +57,7 @@ def build_redirect_url(hostname, path): return redirect_base + path -def login_user(username, provider, fence_idp=None, shib_idp=None): +def login_user(username, provider, fence_idp=None, shib_idp=None, email=None): """ Login a user with the given username and provider. Set values in Flask session to indicate the user being logged in. In addition, commit the user @@ -66,7 +66,10 @@ def login_user(username, provider, fence_idp=None, shib_idp=None): Args: username (str): specific username of user to be logged in provider (str): specfic idp of user to be logged in - + fence_idp (str, optional): Downstreawm fence IdP + shib_idp (str, optional): Downstreawm shibboleth IdP + email (str, optional): email of user (may or may not match username depending + on the IdP) """ def set_flask_session_values(user): @@ -96,7 +99,10 @@ def set_flask_session_values(user): set_flask_session_values(user) return else: - user = User(username=username) + if email: + user = User(username=username, email=email) + else: + user = User(username=username) idp = ( current_session.query(IdentityProvider) @@ -106,6 +112,12 @@ def set_flask_session_values(user): if not idp: idp = IdentityProvider(name=provider) + if email and user.email != email: + logger.info( + f"Updating username {user.username}'s email from {user.email} to {email}" + ) + user.email = email + user.identity_provider = idp current_session.add(user) current_session.commit() diff --git a/fence/blueprints/login/__init__.py b/fence/blueprints/login/__init__.py index 8b5f56baa0..62b526c99c 100644 --- a/fence/blueprints/login/__init__.py +++ b/fence/blueprints/login/__init__.py @@ -70,7 +70,7 @@ def default_login(): # fall back on ENABLED_IDENTITY_PROVIDERS.default default_idp = config["ENABLED_IDENTITY_PROVIDERS"]["default"] else: - logger.warn("DEFAULT_LOGIN_IDP not configured") + logger.warning("DEFAULT_LOGIN_IDP not configured") default_idp = None # other login options @@ -89,7 +89,7 @@ def default_login(): for idp, details in enabled_providers.items() ] else: - logger.warn("LOGIN_OPTIONS not configured or empty") + logger.warning("LOGIN_OPTIONS not configured or empty") login_options = [] def absolute_login_url(provider_id, fence_idp=None, shib_idp=None): @@ -325,7 +325,7 @@ def get_all_shib_idps(): all_shib_idps = [] for shib_idp in res.json(): if "entityID" not in shib_idp: - logger.warn( + logger.warning( f"get_all_shib_idps(): 'entityID' field not in IDP data: {shib_idp}. Skipping this IDP." ) continue @@ -333,7 +333,7 @@ def get_all_shib_idps(): if len(shib_idp.get("DisplayNames", [])) > 0: name = get_shib_idp_en_name(shib_idp["DisplayNames"]) else: - logger.warn( + logger.warning( f"get_all_shib_idps(): 'DisplayNames' field not in IDP data: {shib_idp}. Using IDP ID '{idp}' as IDP name." ) name = idp diff --git a/fence/blueprints/login/base.py b/fence/blueprints/login/base.py index 215164b759..06f0aea2e0 100644 --- a/fence/blueprints/login/base.py +++ b/fence/blueprints/login/base.py @@ -56,7 +56,7 @@ def get(self): class DefaultOAuth2Callback(Resource): - def __init__(self, idp_name, client, username_field="email"): + def __init__(self, idp_name, client, username_field="email", email_field="email"): """ Construct a resource for a login callback endpoint @@ -66,10 +66,13 @@ def __init__(self, idp_name, client, username_field="email"): Some instaniation of this base client class or a child class username_field (str, optional): default field from response to retrieve the username + email_field (str, optional): default field from response to + retrieve the email (if available) """ self.idp_name = idp_name self.client = client self.username_field = username_field + self.email_field = email_field def get(self): # Check if user granted access @@ -97,8 +100,9 @@ def get(self): code = flask.request.args.get("code") result = self.client.get_user_id(code) username = result.get(self.username_field) + email = result.get(self.email_field) if username: - resp = _login(username, self.idp_name) + resp = _login(username, self.idp_name, email=email) self.post_login(flask.g.user, result) return resp raise UserError(result) @@ -118,12 +122,12 @@ def create_login_log(idp_name): ) -def _login(username, idp_name): +def _login(username, idp_name, email=None): """ Login user with given username, then redirect if session has a saved redirect. """ - login_user(username, idp_name) + login_user(username, idp_name, email=email) if flask.session.get("redirect"): return flask.redirect(flask.session.get("redirect")) return flask.jsonify({"username": username}) diff --git a/fence/blueprints/login/fence_login.py b/fence/blueprints/login/fence_login.py index dd12e79172..dbbba3f27f 100644 --- a/fence/blueprints/login/fence_login.py +++ b/fence/blueprints/login/fence_login.py @@ -95,11 +95,13 @@ def get(self): tokens["id_token"], scope="openid", purpose="id", attempt_refresh=True ) username = id_token_claims["context"]["user"]["name"] + email = id_token_claims["context"]["user"].get("email") login_user( username, IdentityProvider.fence, fence_idp=flask.session.get("fence_idp"), shib_idp=flask.session.get("shib_idp"), + email=email, ) self.post_login() diff --git a/fence/resources/audit_service_client.py b/fence/resources/audit_service_client.py index b06217985d..bceb8c5b33 100644 --- a/fence/resources/audit_service_client.py +++ b/fence/resources/audit_service_client.py @@ -31,7 +31,7 @@ def __init__(self, service_url, logger): logger.info("Enabling audit logs") self.ping() else: - logger.warn("NOT enabling audit logs") + logger.warning("NOT enabling audit logs") def ping(self): max_tries = 3 diff --git a/fence/resources/openid/idp_oauth2.py b/fence/resources/openid/idp_oauth2.py index 770bf6b5b7..0a1931ee0d 100644 --- a/fence/resources/openid/idp_oauth2.py +++ b/fence/resources/openid/idp_oauth2.py @@ -128,7 +128,7 @@ def get_auth_url(self): def get_user_id(self, code): """ - Must implement in inheriting class. Should return dictionary with "email" field + Must implement in inheriting class. Should return dictionary with necessary field(s) for successfully logged in user OR "error" field with details of the error. """ raise NotImplementedError() diff --git a/fence/sync/sync_users.py b/fence/sync/sync_users.py index 5c72a6f685..9eaa9434c6 100644 --- a/fence/sync/sync_users.py +++ b/fence/sync/sync_users.py @@ -1806,7 +1806,7 @@ def _add_dbgap_study_to_arborist(self, dbgap_study, dbgap_config): def _is_arborist_healthy(self): if not self.arborist_client: - self.logger.warn("no arborist client set; skipping arborist dbgap sync") + self.logger.warning("no arborist client set; skipping arborist dbgap sync") return False if not self.arborist_client.healthy(): # TODO (rudyardrichter, 2019-01-07): add backoff/retry here diff --git a/tests/oidc/discovery/test_configuration.py b/tests/oidc/discovery/test_configuration.py index e6014c8b13..d5811598c6 100644 --- a/tests/oidc/discovery/test_configuration.py +++ b/tests/oidc/discovery/test_configuration.py @@ -36,6 +36,6 @@ def test_oidc_config_fields(app, client): for field in recommended_fields: if field not in response.json: - warnings.warn( + warnings.warning( "OIDC configuration response missing recommended field: " + field ) From 90ee0442f8b414a2a13b028bf4c6dad6dab394b4 Mon Sep 17 00:00:00 2001 From: Alexander VT Date: Fri, 11 Jun 2021 11:54:52 -0500 Subject: [PATCH 2/3] fix(email): make sure to update email in db even if user has logged in before --- fence/auth.py | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/fence/auth.py b/fence/auth.py index 929cee67c0..eb36316d5e 100644 --- a/fence/auth.py +++ b/fence/auth.py @@ -92,6 +92,8 @@ def set_flask_session_values(user): user = query_for_user(session=current_session, username=username) if user: + _update_users_email(user, email) + # This expression is relevant to those users who already have user and # idp info persisted to the database. We return early to avoid # unnecessarily re-saving that user and idp info. @@ -112,11 +114,7 @@ def set_flask_session_values(user): if not idp: idp = IdentityProvider(name=provider) - if email and user.email != email: - logger.info( - f"Updating username {user.username}'s email from {user.email} to {email}" - ) - user.email = email + _update_users_email(user, email) user.identity_provider = idp current_session.add(user) @@ -261,3 +259,16 @@ def wrapper(*args, **kwargs): def admin_login_required(function): """Compose the login required and admin required decorators.""" return login_required({"admin"})(admin_required(function)) + + +def _update_users_email(user, email): + """ + Update email if provided and doesn't match db entry. + + NOTE: This does NOT commit to the db, do so outside this function + """ + if email and user.email != email: + logger.info( + f"Updating username {user.username}'s email from {user.email} to {email}" + ) + user.email = email From 2762484ec61f65e5753ad5694facc8b15c277a34 Mon Sep 17 00:00:00 2001 From: Alexander VT Date: Tue, 15 Jun 2021 11:02:53 -0500 Subject: [PATCH 3/3] fix(email): remove redundancy and correctly get email for RAS --- fence/auth.py | 7 +- fence/resources/audit_service_client.py | 120 ------------------------ fence/resources/openid/ras_oauth2.py | 2 +- 3 files changed, 4 insertions(+), 125 deletions(-) delete mode 100644 fence/resources/audit_service_client.py diff --git a/fence/auth.py b/fence/auth.py index eb36316d5e..8cd9ed15c7 100644 --- a/fence/auth.py +++ b/fence/auth.py @@ -114,8 +114,6 @@ def set_flask_session_values(user): if not idp: idp = IdentityProvider(name=provider) - _update_users_email(user, email) - user.identity_provider = idp current_session.add(user) current_session.commit() @@ -264,11 +262,12 @@ def admin_login_required(function): def _update_users_email(user, email): """ Update email if provided and doesn't match db entry. - - NOTE: This does NOT commit to the db, do so outside this function """ if email and user.email != email: logger.info( f"Updating username {user.username}'s email from {user.email} to {email}" ) user.email = email + + current_session.add(user) + current_session.commit() diff --git a/fence/resources/audit_service_client.py b/fence/resources/audit_service_client.py deleted file mode 100644 index bceb8c5b33..0000000000 --- a/fence/resources/audit_service_client.py +++ /dev/null @@ -1,120 +0,0 @@ -import flask -import requests -import time - -from fence.config import config -from fence.errors import InternalError - - -def get_request_url(): - request_url = flask.request.url - base_url = config.get("BASE_URL", "") - if request_url.startswith(base_url): - request_url = request_url[len(base_url) :] - return request_url - - -def is_audit_enabled(category=None): - enable_audit_logs = config.get("ENABLE_AUDIT_LOGS") or {} - if category: - return enable_audit_logs and enable_audit_logs.get(category, False) - return enable_audit_logs and any(v for v in enable_audit_logs.values()) - - -class AuditServiceClient: - def __init__(self, service_url, logger): - self.service_url = service_url.rstrip("/") - self.logger = logger - - # audit logs should not be enabled if the audit-service is unavailable - if is_audit_enabled(): - logger.info("Enabling audit logs") - self.ping() - else: - logger.warning("NOT enabling audit logs") - - def ping(self): - max_tries = 3 - status_url = f"{self.service_url}/_status" - self.logger.debug(f"Checking audit-service availability at {status_url}") - wait_time = 1 - for t in range(max_tries): - r = requests.get(status_url) - if r.status_code == 200: - return # all good! - if t + 1 < max_tries: - self.logger.debug(f"Retrying... (got status code {r.status_code})") - time.sleep(wait_time) - wait_time *= 2 - raise Exception( - f"Audit logs are enabled but audit-service is unreachable at {status_url}: {r.text}" - ) - - def check_response(self, resp, body): - # The audit-service returns 201 before inserting the log in the DB. - # This request should only error if the input is incorrect (status - # code 422) or if the service is unreachable. - if resp.status_code != 201: - try: - err = resp.json() - except Exception: - err = resp.text - self.logger.error(f"Unable to POST audit log `{body}`. Details:\n{err}") - raise InternalError("Unable to create audit log") - - def create_presigned_url_log( - self, - username, - sub, - guid, - resource_paths, - action, - protocol, - ): - if not is_audit_enabled("presigned_url"): - return - - url = f"{self.service_url}/log/presigned_url" - body = { - "request_url": get_request_url(), - "status_code": 200, # only record successful requests for now - "username": username, - "sub": sub, - "guid": guid, - "resource_paths": resource_paths, - "action": action, - "protocol": protocol, - } - resp = requests.post(url, json=body) - self.check_response(resp, body) - - def create_login_log( - self, - username, - sub, - idp, - fence_idp=None, - shib_idp=None, - client_id=None, - ): - if not is_audit_enabled("login"): - return - - # special case for idp=fence when falling back on - # fence_idp=shibboleth and shib_idp=NIH - if shib_idp == "None": - shib_idp = None - - url = f"{self.service_url}/log/login" - body = { - "request_url": get_request_url(), - "status_code": 200, # only record successful requests for now - "username": username, - "sub": sub, - "idp": idp, - "fence_idp": fence_idp, - "shib_idp": shib_idp, - "client_id": client_id, - } - resp = requests.post(url, json=body) - self.check_response(resp, body) diff --git a/fence/resources/openid/ras_oauth2.py b/fence/resources/openid/ras_oauth2.py index cd34a4a3dd..42951c97f8 100644 --- a/fence/resources/openid/ras_oauth2.py +++ b/fence/resources/openid/ras_oauth2.py @@ -113,7 +113,7 @@ def get_user_id(self, code): self.logger.exception("{}: {}".format(err_msg, e)) return {"error": err_msg} - return {"username": username} + return {"username": username, "email": userinfo.get("email")} @backoff.on_exception(backoff.expo, Exception, **DEFAULT_BACKOFF_SETTINGS) def update_user_visas(self, user, db_session=current_session):