From e096df9d20ecf27ab23538f1c489ebafce1d9f65 Mon Sep 17 00:00:00 2001 From: Bas Zoetekouw Date: Tue, 31 May 2022 15:35:24 +0200 Subject: [PATCH 01/25] update tests --- server/test/api/test_user_saml.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/server/test/api/test_user_saml.py b/server/test/api/test_user_saml.py index d935c4a88..4d5e9186f 100644 --- a/server/test/api/test_user_saml.py +++ b/server/test/api/test_user_saml.py @@ -38,6 +38,8 @@ def test_proxy_authz_ssid_required(self): sarah = self.find_entity_by_name(User, sarah_name) self.assertTrue(sarah.ssid_required) + self.assertEqual("ssid.org", sarah.schac_home_organisation) + self.assertEqual("sarah", sarah.home_organisation_uid) def test_proxy_authz_including_groups(self): self.add_service_aup_to_user("urn:jane", service_network_entity_id) From a5ce1cd62ac8a5c83166be5d77f24c5b7387c839 Mon Sep 17 00:00:00 2001 From: Bas Zoetekouw Date: Wed, 1 Jun 2022 22:21:13 +0200 Subject: [PATCH 02/25] Try to fix the MFA flow (this is getting waaaay too complicated...) --- .../src/pages/SecondFactorAuthentication.jsx | 2 + server/api/mfa.py | 16 ++ server/api/user_saml.py | 76 ++++++--- server/auth/mfa.py | 33 ++-- server/auth/urls.py | 1 + server/test/api/test_mfa.py | 15 ++ server/test/api/test_user_saml.py | 147 +++++++++++++----- 7 files changed, 216 insertions(+), 74 deletions(-) diff --git a/client/src/pages/SecondFactorAuthentication.jsx b/client/src/pages/SecondFactorAuthentication.jsx index 6ff0d09ac..9f23fc2e8 100644 --- a/client/src/pages/SecondFactorAuthentication.jsx +++ b/client/src/pages/SecondFactorAuthentication.jsx @@ -62,7 +62,9 @@ class SecondFactorAuthentication extends React.Component { qrCode: res.qr_code_base64, idp_name: res.idp_name || I18n.t("mfa.register.unknownIdp"), continueUrl: continueUrl + // hier ssid_needed vars toevoegen in state }); + // hier checken of we ssid moeten doen; zo ja, rediretc url ophalen en op de een of andere manier original_destination in sessie zetten en user redirecten this.focusCode(); }).catch(() => this.props.history.push(`/404?eo=${ErrorOrigins.invalidSecondFactorUUID}`)) } else { diff --git a/server/api/mfa.py b/server/api/mfa.py index de4feb968..5e3d8cfb2 100644 --- a/server/api/mfa.py +++ b/server/api/mfa.py @@ -17,6 +17,7 @@ from server.auth.mfa import ACR_VALUES, decode_jwt_token, store_user_in_session, eligible_users_to_reset_token from server.auth.rate_limit import rate_limit_reached, clear_rate_limit from server.auth.security import current_user_id, generate_token, is_admin_user +from server.auth.ssid import redirect_to_surf_secure_id from server.cron.idp_metadata_parser import idp_display_name from server.db.db import db from server.db.domain import User @@ -78,6 +79,7 @@ def _do_get2fa(schac_home_organisation, user_identifier): img.save(buffered, format="PNG") img_str = base64.b64encode(buffered.getvalue()).decode() idp_name = idp_display_name(schac_home_organisation, "en") + # hier ook ssid_required en sha en uid teruggeven return {"qr_code_base64": img_str, "secret": secret, "idp_name": idp_name}, 200 @@ -142,6 +144,7 @@ def get2fa_proxy_authz(): user = User.query.filter(User.second_fa_uuid == second_fa_uuid).one() if user.second_factor_auth: return {}, 200 + # hier ook ssid_required teruggeven return _do_get2fa(user.schac_home_organisation, user.uid) @@ -258,3 +261,16 @@ def sfo(): @json_endpoint def jwks(): return _get_public_key(), 200 + + +@mfa_api.route("/ssid_start/", strict_slashes=False) +def do_ssid_redirect(second_fa_uuid): + logger = ctx_logger("2fa") + + continue_url = query_param("continue_url", required=True) + session["original_destination"] = continue_url + user = User.query.filter(User.second_fa_uuid == second_fa_uuid).one() + + logger.debug(f"do_ssid_redirect: continu{continue_url}, user={user}") + + return redirect_to_surf_secure_id(user) diff --git a/server/api/user_saml.py b/server/api/user_saml.py index 73db190b8..772f67761 100644 --- a/server/api/user_saml.py +++ b/server/api/user_saml.py @@ -7,7 +7,7 @@ from server.api.base import json_endpoint, send_error_mail from server.api.service_aups import has_agreed_with -from server.auth.mfa import mfa_idp_allowed, surf_secure_id_required +from server.auth.mfa import mfa_idp_allowed, surf_secure_id_required, has_valid_mfa from server.auth.security import confirm_read_access from server.auth.user_claims import user_memberships from server.db.db import db @@ -40,7 +40,7 @@ # See https://github.com/SURFscz/SBS/issues/152 -def _perform_sram_login(uid, home_organisation_uid, schac_home_organisation, issuer_id): +def _perform_sram_login(uid, home_organisation_uid, schac_home_organisation, issuer_id, require_2fa=True): logger = ctx_logger("user_api") user = User.query.filter(User.uid == uid).first() @@ -50,22 +50,45 @@ def _perform_sram_login(uid, home_organisation_uid, schac_home_organisation, iss user.home_organisation_uid = home_organisation_uid user.schac_home_organisation = schac_home_organisation - user.ssid_required = surf_secure_id_required(user, schac_home_organisation, issuer_id) - if not user.ssid_required: + + # TODO: lots of duplicated code below + if require_2fa: idp_allowed = mfa_idp_allowed(user, schac_home_organisation, issuer_id) - if not idp_allowed: - logger.debug(f"Returning interrupt for user {uid} from issuer {issuer_id} to perform 2fa after sram_login") - user.second_fa_uuid = str(uuid.uuid4()) - db.session.merge(user) + ssid_required = surf_secure_id_required(user, schac_home_organisation, issuer_id) + + # this is a configuration conflict and should never happen! + if idp_allowed and ssid_required: + raise Exception(f"Both IdP-based MFA and SSID-based MFA configured for IdP '{schac_home_organisation}'") + + # if IdP-base MFA is set, we assume everything is handled by the IdP, and we skip all checks here + # also skip if user has already recently performed MFA + if not idp_allowed and not has_valid_mfa(user): base_url = current_app.app_config.base_url + if ssid_required: + user.ssid_required = True + user.home_organisation_uid = home_organisation_uid + user.schac_home_organisation = schac_home_organisation + redirect_base_url = f"{base_url}/api/mfa/ssid_start" + else: + redirect_base_url = f"{base_url}/2fa" + + user.second_fa_uuid = str(uuid.uuid4()) + user = db.session.merge(user) + db.session.commit() + + logger.debug(f"Returning interrupt for user {uid} from issuer {issuer_id} to perform 2fa " + f"(ssid={ssid_required})") + return { "status": { "result": "interrupt", - "redirect_url": f"{base_url}/2fa/{user.second_fa_uuid}", + "redirect_url": f"{redirect_base_url}/{user.second_fa_uuid}", "error_status": SECOND_FA_REQUIRED } }, 200 + db.session.merge(user) + db.session.commit() return {"status": {"result": "authorized"}}, 200 @@ -115,20 +138,30 @@ def _do_attributes(uid, service_entity_id, not_authorized_func, authorized_func, f" as none of the collaboration memberships are active") return not_authorized_func(service.name, MEMBERSHIP_NOT_ACTIVE) - # First check if the user needs to stepped up by SURF Secure ID - ssid_required = surf_secure_id_required(user, schac_home_organisation, issuer_id) + # logic should be: first check what type of 2FA the user should use (IdP, SSID, MFA) + # if idp: then always continue (everything is assumed to be handled by IdP) + # if ssid or mfa: check if user has recently done check; if so, than continue + # otherwise, send interrupt # Leave the 2FA and AUP checks as the last checks as these are the only exceptions that can be recovered from if require_2fa: - if ssid_required: - user.ssid_required = True - user.home_organisation_uid = home_organisation_uid - user.schac_home_organisation = schac_home_organisation - user = db.session.merge(user) - db.session.commit() - idp_allowed = mfa_idp_allowed(user, schac_home_organisation, issuer_id) - if not idp_allowed or ssid_required: + ssid_required = surf_secure_id_required(user, schac_home_organisation, issuer_id) + + # this is a configuration conflict and should never happen! + if idp_allowed and ssid_required: + raise Exception(f"Both IdP-based MFA and SSID-based MFA configured for IdP '{schac_home_organisation}'") + + # if IdP-base MFA is set, we assume everything is handled by the IdP, and we skip all checks here + # also skip if user has already recently performed MFA + if not idp_allowed and not has_valid_mfa(user): + if ssid_required: + user.ssid_required = True + user.home_organisation_uid = home_organisation_uid + user.schac_home_organisation = schac_home_organisation + user = db.session.merge(user) + db.session.commit() + logger.debug(f"Returning interrupt for user {uid} from issuer {issuer_id} to perform 2fa " f"(ssid={ssid_required})") return not_authorized_func(user, SECOND_FA_REQUIRED) @@ -182,7 +215,10 @@ def not_authorized_func(service_name, status): user.second_fa_uuid = str(uuid.uuid4()) db.session.merge(user) db.session.commit() - redirect_url = f"{base_url}/2fa/{user.second_fa_uuid}" + if user.ssid_required: + redirect_url = f"{base_url}/api/mfa/ssid_start/{user.second_fa_uuid}" + else: + redirect_url = f"{base_url}/2fa/{user.second_fa_uuid}" result = "interrupt" elif status == AUP_NOT_AGREED: # Internal contract, in case of AUP_NOT_AGREED we get the Service instance returned diff --git a/server/auth/mfa.py b/server/auth/mfa.py index 5c3449d23..05cf68f61 100644 --- a/server/auth/mfa.py +++ b/server/auth/mfa.py @@ -93,33 +93,36 @@ def eligible_users_to_reset_token(user): return user_information -def _idp_configured_or_valid_mfa(user, identity_providers, valid_sso_negates, action, schac_home=None, entity_id=None): +def _idp_configured(user, identity_providers, action, schac_home=None, entity_id=None): entity_id_allowed = entity_id and [idp for idp in identity_providers if idp.entity_id == entity_id.lower()] schac_home_allowed = schac_home and [idp for idp in identity_providers if idp.schac_home == schac_home.lower()] - last_login_date = user.last_login_date - minutes_ago = datetime.now() - timedelta(hours=0, minutes=int(current_app.app_config.mfa_sso_time_in_minutes)) - valid_mfa_sso = last_login_date and last_login_date > minutes_ago - if valid_sso_negates: - result = (entity_id_allowed or schac_home_allowed) and not valid_mfa_sso - else: - result = entity_id_allowed or schac_home_allowed or valid_mfa_sso + result = entity_id_allowed or schac_home_allowed logger = ctx_logger("user_api") - logger.debug(f"{action}: {result} (entity_id_allowed={entity_id_allowed}, schac_home_allowed={schac_home_allowed}, " - f"valid_mfa_sso={valid_mfa_sso}, entity_id={entity_id}, schac_home={schac_home}, " - f"last_login={minutes_ago} minutes ago") + logger.debug(f"{action}: {bool(result)} (entity_id_allowed={entity_id_allowed}, " + f"schac_home_allowed={schac_home_allowed}, " + f"entity_id={entity_id}, schac_home={schac_home}") return bool(result) +def has_valid_mfa(user): + last_login_date = user.last_login_date + login_sso_cutoff = timedelta(hours=0, minutes=int(current_app.app_config.mfa_sso_time_in_minutes)) + valid_mfa_sso = last_login_date and datetime.now() - user.last_login_date < login_sso_cutoff + + logger = ctx_logger("user_api") + logger.debug(f"has_valid_mfa: {valid_mfa_sso} (user={user}, last_login={last_login_date}") + + return valid_mfa_sso + + def mfa_idp_allowed(user, schac_home=None, entity_id=None): allowed_identity_providers = current_app.app_config.mfa_idp_allowed - return _idp_configured_or_valid_mfa(user, allowed_identity_providers, False, "mfa_idp_allowed", schac_home, - entity_id) + return _idp_configured(user, allowed_identity_providers, "mfa_idp_allowed", schac_home, entity_id) def surf_secure_id_required(user, schac_home=None, entity_id=None): ssid_identity_providers = current_app.app_config.ssid_identity_providers - return _idp_configured_or_valid_mfa(user, ssid_identity_providers, True, "surf_secure_id_required", schac_home, - entity_id) + return _idp_configured(user, ssid_identity_providers, "surf_secure_id_required", schac_home, entity_id) diff --git a/server/auth/urls.py b/server/auth/urls.py index fa1a60314..1625ac9d8 100644 --- a/server/auth/urls.py +++ b/server/auth/urls.py @@ -5,6 +5,7 @@ "/api/mfa/get2fa_proxy_authz", "/api/mfa/jwks", "/api/mfa/sfo", + "/api/mfa/ssid_start", "/api/mfa/verify2fa_proxy_authz", "/api/mock", "/api/organisation_invitations/find_by_hash", diff --git a/server/test/api/test_mfa.py b/server/test/api/test_mfa.py index ecdf43f73..0a77120d9 100644 --- a/server/test/api/test_mfa.py +++ b/server/test/api/test_mfa.py @@ -3,8 +3,10 @@ import responses from flask import current_app +from server.auth.ssid import saml_auth from server.db.domain import User from server.test.abstract_test import AbstractTest +from server.test.seed import service_mail_entity_id, sarah_name from server.tools import read_file @@ -33,6 +35,19 @@ def test_2fa_scenario(self): mary = User.query.filter(User.uid == "urn:mary").first() self.assertEqual(secret, mary.second_factor_auth) + def test_ssid_scenario(self): + # initiate proxy_authz call to initialize 2fa + res = self.post("/api/users/proxy_authz", response_status_code=200, + body={"user_id": "urn:sarah", "service_id": service_mail_entity_id, "issuer_id": "issuer.com", + "uid": "sarah", "homeorganization": "ssid.org"}) + sarah = self.find_entity_by_name(User, sarah_name) + + # start the ssid + res = self.get(f"/api/mfa/ssid_start/{sarah.second_fa_uuid}", query_data={"continue_url": "https://foo.bar"}, + response_status_code=302) + saml_sso_url = saml_auth().get_sso_url() + self.assertTrue(res.location.startswith(saml_sso_url)) + def test_2fa_invalid_totp(self): AbstractTest.set_second_factor_auth("urn:mary") diff --git a/server/test/api/test_user_saml.py b/server/test/api/test_user_saml.py index 4d5e9186f..9ad9eb79f 100644 --- a/server/test/api/test_user_saml.py +++ b/server/test/api/test_user_saml.py @@ -28,19 +28,6 @@ def test_proxy_authz(self): self.assertListEqual(["sarah"], attrs["uid"]) self.assertIsNotNone(attrs["sshkey"][0]) - def test_proxy_authz_ssid_required(self): - self.add_service_aup_to_user("urn:sarah", service_mail_entity_id) - - res = self.post("/api/users/proxy_authz", response_status_code=200, - body={"user_id": "urn:sarah", "service_id": service_mail_entity_id, "issuer_id": "issuer.com", - "uid": "sarah", "homeorganization": "ssid.org"}) - self.assertEqual(res["status"]["result"], "interrupt") - - sarah = self.find_entity_by_name(User, sarah_name) - self.assertTrue(sarah.ssid_required) - self.assertEqual("ssid.org", sarah.schac_home_organisation) - self.assertEqual("sarah", sarah.home_organisation_uid) - def test_proxy_authz_including_groups(self): self.add_service_aup_to_user("urn:jane", service_network_entity_id) self.login_user_2fa("urn:jane") @@ -103,23 +90,6 @@ def test_proxy_authz_no_aup(self): parameters = urlencode({"service_id": network_service.uuid4, "service_name": network_service.name}) self.assertEqual(res["status"]["redirect_url"], f"http://localhost:3000/service-aup?{parameters}") - def test_proxy_authz_no_2fa(self): - res = self.post("/api/users/proxy_authz", response_status_code=200, - body={"user_id": "urn:sarah", "service_id": service_mail_entity_id, "issuer_id": "nope", - "uid": "sarah", "homeorganization": "example.com"}) - sarah = self.find_entity_by_name(User, sarah_name) - self.assertEqual(res["status"]["result"], "interrupt") - self.assertEqual(res["status"]["redirect_url"], f"http://localhost:3000/2fa/{sarah.second_fa_uuid}") - - def test_proxy_authz_allowed_idp(self): - self.add_service_aup_to_user("urn:sarah", service_mail_entity_id) - - res = self.post("/api/users/proxy_authz", response_status_code=200, - body={"user_id": "urn:sarah", "service_id": service_mail_entity_id, - "issuer_id": "https://idp.test", "uid": "sarah", "homeorganization": "example.com"}) - attrs = res["attributes"] - self.assertListEqual(["sarah"], attrs["uid"]) - def test_proxy_authz_no_user(self): res = self.post("/api/users/proxy_authz", body={"user_id": "urn:nope", "service_id": service_mail_entity_id, "issuer_id": "https://idp.test", "uid": "sarah", @@ -144,7 +114,10 @@ def test_proxy_authz_service_not_connected(self): self.assertEqual("unauthorized", res["status"]["result"]) self.assertEqual(SERVICE_NOT_CONNECTED, res["status"]["error_status"]) - def test_proxy_authz_sram_login(self): + # + # MFA scenarios: + # logins on SBS + def test_proxy_authz_mfa_sbs_totp(self): res = self.post("/api/users/proxy_authz", response_status_code=200, body={"user_id": "urn:sarah", "service_id": self.app.app_config.oidc.sram_service_entity_id, @@ -158,7 +131,7 @@ def test_proxy_authz_sram_login(self): self.assertEqual(status_["error_status"], SECOND_FA_REQUIRED) self.assertEqual(status_["redirect_url"], f"http://localhost:3000/2fa/{sarah.second_fa_uuid}") - def test_proxy_authz_sram_login_new_user(self): + def test_proxy_authz_mfa_sbs_totp_new_user(self): res = self.post("/api/users/proxy_authz", response_status_code=200, body={"user_id": "urn:new_user", "service_id": self.app.app_config.oidc.sram_service_entity_id, @@ -171,32 +144,128 @@ def test_proxy_authz_sram_login_new_user(self): self.assertEqual("example.com", new_user.schac_home_organisation) self.assertEqual("sarah", new_user.home_organisation_uid) - def test_proxy_authz_sram_login_ssid_required(self): - self.add_service_aup_to_user("urn:sarah", service_mail_entity_id) + def test_proxy_authz_mfa_sbs_totp_sso(self): + self.login_user_2fa("urn:sarah") res = self.post("/api/users/proxy_authz", response_status_code=200, body={"user_id": "urn:sarah", "service_id": self.app.app_config.oidc.sram_service_entity_id, "issuer_id": "idp", "uid": "sarah", - "homeorganization": "ssid.org"}) + "homeorganization": "example.com"}) status_ = res["status"] self.assertEqual(status_["result"], "authorized") sarah = self.find_entity_by_name(User, sarah_name) + self.assertFalse(sarah.ssid_required) + + def test_proxy_authz_mfa_sbs_ssid(self): + res = self.post("/api/users/proxy_authz", response_status_code=200, + body={"user_id": "urn:sarah", + "service_id": self.app.app_config.oidc.sram_service_entity_id, + "issuer_id": "https://ssid.org", + "uid": "sarah", + "homeorganization": "ssid.org"}) + status_ = res["status"] + sarah = self.find_entity_by_name(User, sarah_name) + self.assertEqual(status_["result"], "interrupt") + self.assertEqual(res["status"]["redirect_url"], + f"http://localhost:3000/api/mfa/ssid_start/{sarah.second_fa_uuid}") self.assertTrue(sarah.ssid_required) - def test_proxy_authz_sram_login_no_2fa_required(self): + def test_proxy_authz_mfa_sbs_ssid_sso(self): self.login_user_2fa("urn:sarah") res = self.post("/api/users/proxy_authz", response_status_code=200, body={"user_id": "urn:sarah", "service_id": self.app.app_config.oidc.sram_service_entity_id, - "issuer_id": "idp", + "issuer_id": "https://ssid.org", + "uid": "sarah", + "homeorganization": "ssid.org"}) + sarah = self.find_entity_by_name(User, sarah_name) + self.assertEqual(res["status"]["result"], "authorized") + self.assertFalse(sarah.ssid_required) + + def test_proxy_authz_mfa_sbs_idp(self): + res = self.post("/api/users/proxy_authz", response_status_code=200, + body={"user_id": "urn:sarah", + "service_id": self.app.app_config.oidc.sram_service_entity_id, + "issuer_id": "https://idp.test", + "uid": "sarah", + "homeorganization": "idp.test"}) + sarah = self.find_entity_by_name(User, sarah_name) + self.assertEqual(res["status"]["result"], "authorized") + self.assertFalse(sarah.ssid_required) + + # MFA scenarios: + # login on services + def test_proxy_authz_mfa_service_totp(self): + res = self.post("/api/users/proxy_authz", response_status_code=200, + body={"user_id": "urn:sarah", + "service_id": service_mail_entity_id, + "issuer_id": "nope", "uid": "sarah", "homeorganization": "example.com"}) - status_ = res["status"] - self.assertEqual(status_["result"], "authorized") + sarah = self.find_entity_by_name(User, sarah_name) + self.assertEqual(res["status"]["result"], "interrupt") + self.assertEqual(res["status"]["redirect_url"], f"http://localhost:3000/2fa/{sarah.second_fa_uuid}") + + def test_proxy_authz_mfa_service_totp_sso(self): + self.add_service_aup_to_user("urn:sarah", service_mail_entity_id) + self.login_user_2fa("urn:sarah") + res = self.post("/api/users/proxy_authz", response_status_code=200, + body={"user_id": "urn:sarah", + "service_id": service_mail_entity_id, + "issuer_id": "nope", + "uid": "sarah", + "homeorganization": "example.com"}) sarah = self.find_entity_by_name(User, sarah_name) + self.assertEqual(res["status"]["result"], "authorized") self.assertFalse(sarah.ssid_required) + + def test_proxy_authz_mfa_service_ssid(self): + res = self.post("/api/users/proxy_authz", response_status_code=200, + body={"user_id": "urn:sarah", + "service_id": service_mail_entity_id, + "issuer_id": "https://ssid.org", + "uid": "sarah", + "homeorganization": "ssid.org"}) + sarah = self.find_entity_by_name(User, sarah_name) + + self.assertEqual(res["status"]["result"], "interrupt") + self.assertEqual(res["status"]["redirect_url"], + f"http://localhost:3000/api/mfa/ssid_start/{sarah.second_fa_uuid}") + + sarah = self.find_entity_by_name(User, sarah_name) + self.assertTrue(sarah.ssid_required) + self.assertEqual("ssid.org", sarah.schac_home_organisation) + self.assertEqual("sarah", sarah.home_organisation_uid) + + def test_proxy_authz_mfa_service_ssid_sso(self): + self.add_service_aup_to_user("urn:sarah", service_mail_entity_id) + self.login_user_2fa("urn:sarah") + + res = self.post("/api/users/proxy_authz", response_status_code=200, + body={"user_id": "urn:sarah", + "service_id": service_mail_entity_id, + "issuer_id": "https://ssid.org", + "uid": "sarah", + "homeorganization": "ssid.org"}) + sarah = self.find_entity_by_name(User, sarah_name) + + self.assertEqual(res["status"]["result"], "authorized") + self.assertFalse(sarah.ssid_required) + + def test_proxy_authz_mfa_service_idp(self): + self.add_service_aup_to_user("urn:sarah", service_mail_entity_id) + + res = self.post("/api/users/proxy_authz", response_status_code=200, + body={"user_id": "urn:sarah", + "service_id": service_mail_entity_id, + "issuer_id": "https://idp.test", + "uid": "sarah", + "homeorganization": "idp.test"}) + self.assertEqual(res["status"]["result"], "authorized") + attrs = res["attributes"] + self.assertListEqual(["sarah"], attrs["uid"]) From 472593d250d94a767107d4893f80c0e7ac35ea46 Mon Sep 17 00:00:00 2001 From: Bas Zoetekouw Date: Thu, 2 Jun 2022 09:44:34 +0200 Subject: [PATCH 03/25] fix coverage --- server/config/test_config.yml | 4 ++++ server/test/api/test_user_saml.py | 16 ++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/server/config/test_config.yml b/server/config/test_config.yml index d3f8c326f..ddef7cdee 100644 --- a/server/config/test_config.yml +++ b/server/config/test_config.yml @@ -191,6 +191,8 @@ ldap: mfa_idp_allowed: - schac_home: "idp.test" entity_id: "https://idp.test" + - schac_home: "erroridp.example.edu" + entity_id: "https://erroridp.example.edu" # A MFA login in a different flow is valid for X minutes mfa_sso_time_in_minutes: 10 @@ -200,6 +202,8 @@ mfa_sso_time_in_minutes: 10 ssid_identity_providers: - schac_home: "ssid.org" entity_id: "https://ssid.org" + - schac_home: "erroridp.example.edu" + entity_id: "https://erroridp.example.edu" ssid_config_folder: saml_test diff --git a/server/test/api/test_user_saml.py b/server/test/api/test_user_saml.py index 9ad9eb79f..dbc42c2a6 100644 --- a/server/test/api/test_user_saml.py +++ b/server/test/api/test_user_saml.py @@ -269,3 +269,19 @@ def test_proxy_authz_mfa_service_idp(self): self.assertEqual(res["status"]["result"], "authorized") attrs = res["attributes"] self.assertListEqual(["sarah"], attrs["uid"]) + + def test_proxy_authz_mfa_faulty_config(self): + res = self.post("/api/users/proxy_authz", response_status_code=500, + body={"user_id": "urn:sarah", + "service_id": service_mail_entity_id, + "issuer_id": "https://erroridp.example.edu", + "uid": "sarah", + "homeorganization": "erroridp.example.edu"}) + self.assertTrue(res["error"]) + res = self.post("/api/users/proxy_authz", response_status_code=500, + body={"user_id": "urn:sarah", + "service_id": self.app.app_config.oidc.sram_service_entity_id, + "issuer_id": "https://erroridp.example.edu", + "uid": "sarah", + "homeorganization": "erroridp.example.edu"}) + self.assertTrue(res["error"]) From 350fe4fa1e639074649afd484502bd604124eeda Mon Sep 17 00:00:00 2001 From: Bas Zoetekouw Date: Thu, 2 Jun 2022 10:17:30 +0200 Subject: [PATCH 04/25] fix release --- .github/workflows/main.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 629c02502..75095b334 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -217,7 +217,7 @@ jobs: if: "github.ref_type=='tag'" uses: softprops/action-gh-release@v1 with: - files: "${{steps.fetch_artifact.outputs.download-path}}" + files: "${{steps.fetch_artifact.outputs.download-path}}/*" - name: Advance latest tag if: "github.ref_name=='main'" @@ -227,6 +227,7 @@ jobs: description: "Latest commit in the main branch" - name: remove all previous "latest" releases + if: "github.ref_name=='main'" uses: dev-drprasad/delete-older-releases@v0.2.0 with: keep_latest: 0 From 6d6f00799a7a58a8454923e9ac531cd56541b7f3 Mon Sep 17 00:00:00 2001 From: Bas Zoetekouw Date: Thu, 2 Jun 2022 10:58:59 +0200 Subject: [PATCH 05/25] remove obsolete comments --- client/src/pages/SecondFactorAuthentication.jsx | 2 -- 1 file changed, 2 deletions(-) diff --git a/client/src/pages/SecondFactorAuthentication.jsx b/client/src/pages/SecondFactorAuthentication.jsx index 9f23fc2e8..6ff0d09ac 100644 --- a/client/src/pages/SecondFactorAuthentication.jsx +++ b/client/src/pages/SecondFactorAuthentication.jsx @@ -62,9 +62,7 @@ class SecondFactorAuthentication extends React.Component { qrCode: res.qr_code_base64, idp_name: res.idp_name || I18n.t("mfa.register.unknownIdp"), continueUrl: continueUrl - // hier ssid_needed vars toevoegen in state }); - // hier checken of we ssid moeten doen; zo ja, rediretc url ophalen en op de een of andere manier original_destination in sessie zetten en user redirecten this.focusCode(); }).catch(() => this.props.history.push(`/404?eo=${ErrorOrigins.invalidSecondFactorUUID}`)) } else { From 0991acf6e2d1bac264785504df2f3bb2e43379c9 Mon Sep 17 00:00:00 2001 From: Bas Zoetekouw Date: Thu, 2 Jun 2022 16:32:07 +0200 Subject: [PATCH 06/25] clear up mfa logic --- server/api/user_saml.py | 6 ++++-- server/config/test_config.yml | 13 +++++++++---- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/server/api/user_saml.py b/server/api/user_saml.py index 772f67761..ff585fc5f 100644 --- a/server/api/user_saml.py +++ b/server/api/user_saml.py @@ -55,6 +55,7 @@ def _perform_sram_login(uid, home_organisation_uid, schac_home_organisation, iss if require_2fa: idp_allowed = mfa_idp_allowed(user, schac_home_organisation, issuer_id) ssid_required = surf_secure_id_required(user, schac_home_organisation, issuer_id) + fallback_required = not idp_allowed and not ssid_required and current_app.app_config.mfa_fallback_enabled # this is a configuration conflict and should never happen! if idp_allowed and ssid_required: @@ -62,7 +63,7 @@ def _perform_sram_login(uid, home_organisation_uid, schac_home_organisation, iss # if IdP-base MFA is set, we assume everything is handled by the IdP, and we skip all checks here # also skip if user has already recently performed MFA - if not idp_allowed and not has_valid_mfa(user): + if not idp_allowed and (ssid_required or fallback_required) and not has_valid_mfa(user): base_url = current_app.app_config.base_url if ssid_required: user.ssid_required = True @@ -147,6 +148,7 @@ def _do_attributes(uid, service_entity_id, not_authorized_func, authorized_func, if require_2fa: idp_allowed = mfa_idp_allowed(user, schac_home_organisation, issuer_id) ssid_required = surf_secure_id_required(user, schac_home_organisation, issuer_id) + fallback_required = not idp_allowed and not ssid_required and current_app.app_config.mfa_fallback_enabled # this is a configuration conflict and should never happen! if idp_allowed and ssid_required: @@ -154,7 +156,7 @@ def _do_attributes(uid, service_entity_id, not_authorized_func, authorized_func, # if IdP-base MFA is set, we assume everything is handled by the IdP, and we skip all checks here # also skip if user has already recently performed MFA - if not idp_allowed and not has_valid_mfa(user): + if not idp_allowed and (ssid_required or fallback_required) and not has_valid_mfa(user): if ssid_required: user.ssid_required = True user.home_organisation_uid = home_organisation_uid diff --git a/server/config/test_config.yml b/server/config/test_config.yml index ddef7cdee..eabdcf724 100644 --- a/server/config/test_config.yml +++ b/server/config/test_config.yml @@ -187,16 +187,21 @@ ldap: url: "ldap://ldap.example.com/dc=example,dc=com" bind_account: "cn=admin,dc=entity_id,dc=services,dc=sram-tst,dc=surf,dc=nl" -# Lower case schachome organisations / entity ID's allowed skipping MFA +# A MFA login in a different flow is valid for X minutes +mfa_sso_time_in_minutes: 10 + +# whether to require TOTP for users form IdPs that match neither mfa_idp_allowed +# nor ssid_identity_providers +mfa_fallback_enabled: true + +# Lower case schachome organisations / entity ID's allowed skipping MFA; +# MFA is supposed to be handled at the IdP for these entities mfa_idp_allowed: - schac_home: "idp.test" entity_id: "https://idp.test" - schac_home: "erroridp.example.edu" entity_id: "https://erroridp.example.edu" -# A MFA login in a different flow is valid for X minutes -mfa_sso_time_in_minutes: 10 - # Lower case schachome organisations / entity ID's where SURFSecure ID is used for step-up # If this feature is no longer needed, just replace the value with an empty list [] ssid_identity_providers: From c10d3c32cca9d93ac178f22672a8e6eae1f5039b Mon Sep 17 00:00:00 2001 From: Bas Zoetekouw Date: Thu, 2 Jun 2022 16:32:54 +0200 Subject: [PATCH 07/25] Fix ssid logins for SBS logins --- server/api/user.py | 48 ++++++++++++++++++++++++++++++------ server/test/api/test_user.py | 20 ++++++++++++++- 2 files changed, 59 insertions(+), 9 deletions(-) diff --git a/server/api/user.py b/server/api/user.py index a270ba653..ebc1d7c06 100644 --- a/server/api/user.py +++ b/server/api/user.py @@ -22,7 +22,8 @@ from server.api.base import replace_full_text_search_boolean_mode_chars from server.api.ipaddress import validate_ip_networks -from server.auth.mfa import ACR_VALUES, decode_jwt_token, store_user_in_session, mfa_idp_allowed +from server.auth.mfa import ACR_VALUES, store_user_in_session, mfa_idp_allowed, \ + surf_secure_id_required, has_valid_mfa from server.auth.security import confirm_allow_impersonation, is_admin_user, current_user_id, confirm_read_access, \ confirm_collaboration_admin, confirm_organisation_admin, current_user, confirm_write_access from server.auth.ssid import AUTHN_REQUEST_ID, saml_auth, redirect_to_surf_secure_id, USER_UID @@ -287,20 +288,51 @@ def resume_session(): logger.info(f"Updating user {user.uid} with new claims / updated at") add_user_claims(user_info_json, uid, user) + # we're repeating some of the logic of _perform_sram_login() here + # at least until EduTEAMS has transitioned to inserting a call to proxy_authz in the login flow for SBS itself + # + # no need to repeat this logic if we already have made a decision before + if not user.ssid_required and not has_valid_mfa(user): + schac_home_organisation = user.schac_home_organisation + home_organisation_uid = user_info_json.get('uid', None) + + idp_allowed = mfa_idp_allowed(user=user, schac_home=schac_home_organisation) + ssid_required = surf_secure_id_required(user=user, schac_home=schac_home_organisation) + fallback_required = not idp_allowed and not ssid_required and current_app.app_config.mfa_fallback_enabled + + # this is a configuration conflict and should never happen! + if idp_allowed and ssid_required: + raise Exception(f"Both IdP-based MFA and SSID-based MFA configured for IdP '{schac_home_organisation}'") + + # if IdP-base MFA is set, we assume everything is handled by the IdP, and we skip all checks here + # also skip if user has already recently performed MFA + if not idp_allowed and (ssid_required or fallback_required) and not has_valid_mfa(user): + if ssid_required: + user.ssid_required = True + user.home_organisation_uid = home_organisation_uid + user.schac_home_organisation = schac_home_organisation + + user.second_fa_uuid = str(uuid.uuid4()) + user = db.session.merge(user) + db.session.commit() + + logger.debug(f"Setting 2fa required for SBS login for user {uid} (ssid={ssid_required})") + else: + fallback_required = False + if user.ssid_required: user = db.session.merge(user) db.session.commit() return redirect_to_surf_secure_id(user) - encoded_id_token = token_json["id_token"] - id_token = decode_jwt_token(encoded_id_token) - - no_mfa_required = not oidc_config.second_factor_authentication_required - idp_mfa = id_token.get("acr") == ACR_VALUES + # TODO: we're not using ACR values from OIDC at the moment. Reintroduce this later + # encoded_id_token = token_json["id_token"] + # id_token = decode_jwt_token(encoded_id_token) - idp_allowed = mfa_idp_allowed(user, user.schac_home_organisation, None) + # no_mfa_required = not oidc_config.second_factor_authentication_required + # idp_mfa = id_token.get("acr") == ACR_VALUES - second_factor_confirmed = no_mfa_required or idp_mfa or idp_allowed + second_factor_confirmed = not fallback_required if second_factor_confirmed: user.last_login_date = datetime.datetime.now() diff --git a/server/test/api/test_user.py b/server/test/api/test_user.py index add9bbd9e..043126601 100644 --- a/server/test/api/test_user.py +++ b/server/test/api/test_user.py @@ -331,7 +331,8 @@ def test_resume_session_with_allowed_idp(self): json={"access_token": "some_token", "id_token": self.sign_jwt({"acr": "nope"})}, status=200) responses.add(responses.GET, current_app.app_config.oidc.userinfo_endpoint, - json={"sub": "urn:john", "voperson_external_id": "test@idp.test"}, status=200) + json={"sub": "urn:john", "voperson_external_id": "test@idp.test"}, + status=200) responses.add(responses.GET, current_app.app_config.oidc.jwks_endpoint, read_file("test/data/public.json"), status=200) with requests.Session(): @@ -342,6 +343,23 @@ def test_resume_session_with_allowed_idp(self): self.assertTrue(user["second_factor_confirmed"]) self.assertTrue("organisation_memberships" in user) + @responses.activate + def test_resume_session_with_ssid_idp(self): + responses.add(responses.POST, current_app.app_config.oidc.token_endpoint, + json={"access_token": "some_token", "id_token": self.sign_jwt({"acr": "nope"})}, + status=200) + responses.add(responses.GET, current_app.app_config.oidc.userinfo_endpoint, + json={"sub": "urn:john", "voperson_external_id": "test@ssid.org", "uid": "johnnie"}, + status=200) + responses.add(responses.GET, current_app.app_config.oidc.jwks_endpoint, + read_file("test/data/public.json"), status=200) + with requests.Session(): + res = self.client.get("/api/users/resume-session?code=123456") + self.assertTrue(res.location.startswith( + "https://sa-gw.test.surfconext.nl/second-factor-only/single-sign-on?")) + john = self.find_entity_by_name(User, "urn:john") + self.assertTrue(john.ssid_required) + def test_query(self): res = self.get("/api/users/query", query_data={"q": "AMES"}) self.assertEqual(1, len(res)) From baa433cdbdf4bf76ead95513873cd3ed1566c802 Mon Sep 17 00:00:00 2001 From: Bas Zoetekouw Date: Fri, 3 Jun 2022 13:20:55 +0200 Subject: [PATCH 08/25] Fix mfa login --- server/api/user.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/server/api/user.py b/server/api/user.py index ebc1d7c06..3a45a9811 100644 --- a/server/api/user.py +++ b/server/api/user.py @@ -23,7 +23,7 @@ from server.api.ipaddress import validate_ip_networks from server.auth.mfa import ACR_VALUES, store_user_in_session, mfa_idp_allowed, \ - surf_secure_id_required, has_valid_mfa + surf_secure_id_required, has_valid_mfa, decode_jwt_token from server.auth.security import confirm_allow_impersonation, is_admin_user, current_user_id, confirm_read_access, \ confirm_collaboration_admin, confirm_organisation_admin, current_user, confirm_write_access from server.auth.ssid import AUTHN_REQUEST_ID, saml_auth, redirect_to_surf_secure_id, USER_UID @@ -288,11 +288,16 @@ def resume_session(): logger.info(f"Updating user {user.uid} with new claims / updated at") add_user_claims(user_info_json, uid, user) + encoded_id_token = token_json["id_token"] + id_token = decode_jwt_token(encoded_id_token) + + idp_mfa = id_token.get("acr") == ACR_VALUES + # we're repeating some of the logic of _perform_sram_login() here # at least until EduTEAMS has transitioned to inserting a call to proxy_authz in the login flow for SBS itself # # no need to repeat this logic if we already have made a decision before - if not user.ssid_required and not has_valid_mfa(user): + if not idp_mfa and not user.ssid_required and not has_valid_mfa(user): schac_home_organisation = user.schac_home_organisation home_organisation_uid = user_info_json.get('uid', None) @@ -325,14 +330,8 @@ def resume_session(): db.session.commit() return redirect_to_surf_secure_id(user) - # TODO: we're not using ACR values from OIDC at the moment. Reintroduce this later - # encoded_id_token = token_json["id_token"] - # id_token = decode_jwt_token(encoded_id_token) - - # no_mfa_required = not oidc_config.second_factor_authentication_required - # idp_mfa = id_token.get("acr") == ACR_VALUES - - second_factor_confirmed = not fallback_required + no_mfa_required = not oidc_config.second_factor_authentication_required + second_factor_confirmed = no_mfa_required or not fallback_required if second_factor_confirmed: user.last_login_date = datetime.datetime.now() From 2039e78dc92463194225946d0c98896c408c7c15 Mon Sep 17 00:00:00 2001 From: Bas Zoetekouw Date: Fri, 3 Jun 2022 17:09:03 +0200 Subject: [PATCH 09/25] add debug logging --- server/api/user.py | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/server/api/user.py b/server/api/user.py index 3a45a9811..4c9464860 100644 --- a/server/api/user.py +++ b/server/api/user.py @@ -230,7 +230,7 @@ def authorization(): @user_api.route("/resume-session", strict_slashes=False) def resume_session(): - logger = ctx_logger("oidc") + logger = ctx_logger("resume-session/oidc") cfg = current_app.app_config oidc_config = cfg.oidc @@ -270,7 +270,7 @@ def resume_session(): if response.status_code != 200: return _redirect_with_error(logger, f"Server error: User info endpoint error (http {response.status_code}") - logger = ctx_logger("user") + logger = ctx_logger("resume-session/user") user_info_json = response.json() logger.debug(f"Userinfo endpoint results {user_info_json}") @@ -305,6 +305,10 @@ def resume_session(): ssid_required = surf_secure_id_required(user=user, schac_home=schac_home_organisation) fallback_required = not idp_allowed and not ssid_required and current_app.app_config.mfa_fallback_enabled + logger.debug(f"SBS login for user {uid} MFA check: " + f"idp_allowed={idp_allowed}, ssid={ssid_required}, fallback={fallback_required} " + f"(sho={schac_home_organisation},uid={home_organisation_uid}") + # this is a configuration conflict and should never happen! if idp_allowed and ssid_required: raise Exception(f"Both IdP-based MFA and SSID-based MFA configured for IdP '{schac_home_organisation}'") @@ -326,6 +330,7 @@ def resume_session(): fallback_required = False if user.ssid_required: + logger.debug(f"Redirecting user {uid} to ssid") user = db.session.merge(user) db.session.commit() return redirect_to_surf_secure_id(user) @@ -339,6 +344,8 @@ def resume_session(): def _redirect_to_client(cfg, second_factor_confirmed, user): + logger = ctx_logger("redirect") + user = db.session.merge(user) db.session.commit() user_accepted_aup = user.has_agreed_with_aup() @@ -349,11 +356,15 @@ def _redirect_to_client(cfg, second_factor_confirmed, user): location = f"{cfg.base_url}/2fa" else: location = session.get("original_destination", cfg.base_url) + + logger.debug(f"Redirecting user {user.uid} to {location}") return redirect(location) @user_api.route("/acs", methods=["POST"], strict_slashes=False) def acs(): + logger = ctx_logger("acl") + request_id = session.get(AUTHN_REQUEST_ID, None) auth = saml_auth() auth.process_response(request_id=request_id) @@ -370,6 +381,9 @@ def acs(): # There is no other way to get the status back status = OneLogin_Saml2_Utils.get_status(auth._last_response) second_factor_confirmed = OneLogin_Saml2_Constants.STATUS_SUCCESS == status["code"] + + logger.debug(f"User {user_uid} got SSID response (status={status})") + if second_factor_confirmed: user.last_login_date = datetime.datetime.now() return _redirect_to_client(cfg, second_factor_confirmed, user) From 86b2b902fc1a9401de7a6adf92bb2be6efa5c5b9 Mon Sep 17 00:00:00 2001 From: Bas Zoetekouw Date: Tue, 7 Jun 2022 10:49:03 +0200 Subject: [PATCH 10/25] debug info --- server/api/user_saml.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/server/api/user_saml.py b/server/api/user_saml.py index ff585fc5f..77ea975d0 100644 --- a/server/api/user_saml.py +++ b/server/api/user_saml.py @@ -43,6 +43,8 @@ def _perform_sram_login(uid, home_organisation_uid, schac_home_organisation, issuer_id, require_2fa=True): logger = ctx_logger("user_api") + logger.debug("SBS login flow") + user = User.query.filter(User.uid == uid).first() if not user: logger.debug("Creating new user in sram_login") From fb713bfefa05e9ba3878a9c72f97d53d3c33f1db Mon Sep 17 00:00:00 2001 From: Bas Zoetekouw Date: Tue, 7 Jun 2022 11:16:46 +0200 Subject: [PATCH 11/25] Use separate ssid original_destination var to avoid authn loops --- server/api/mfa.py | 4 ++-- server/api/user.py | 6 +++++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/server/api/mfa.py b/server/api/mfa.py index 5e3d8cfb2..4fea06848 100644 --- a/server/api/mfa.py +++ b/server/api/mfa.py @@ -268,9 +268,9 @@ def do_ssid_redirect(second_fa_uuid): logger = ctx_logger("2fa") continue_url = query_param("continue_url", required=True) - session["original_destination"] = continue_url + session["ssid_original_destination"] = continue_url user = User.query.filter(User.second_fa_uuid == second_fa_uuid).one() - logger.debug(f"do_ssid_redirect: continu{continue_url}, user={user}") + logger.debug(f"do_ssid_redirect: continu={continue_url}, user={user}") return redirect_to_surf_secure_id(user) diff --git a/server/api/user.py b/server/api/user.py index 4c9464860..9840b521f 100644 --- a/server/api/user.py +++ b/server/api/user.py @@ -354,8 +354,12 @@ def _redirect_to_client(cfg, second_factor_confirmed, user): location = f"{cfg.base_url}/aup" elif not second_factor_confirmed: location = f"{cfg.base_url}/2fa" + elif "ssid_original_destination" in session: + location = session.pop("ssid_original_destination") + elif "original_destination" in session: + location = session.pop("original_destination") else: - location = session.get("original_destination", cfg.base_url) + location = cfg.base_url logger.debug(f"Redirecting user {user.uid} to {location}") return redirect(location) From 2b07742fd50e9e8f2a1e37bcbe4604f07a3ca03d Mon Sep 17 00:00:00 2001 From: Bas Zoetekouw Date: Tue, 7 Jun 2022 12:04:16 +0200 Subject: [PATCH 12/25] debug --- client/src/pages/SecondFactorAuthentication.jsx | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/client/src/pages/SecondFactorAuthentication.jsx b/client/src/pages/SecondFactorAuthentication.jsx index 6ff0d09ac..cf06ede44 100644 --- a/client/src/pages/SecondFactorAuthentication.jsx +++ b/client/src/pages/SecondFactorAuthentication.jsx @@ -48,6 +48,11 @@ class SecondFactorAuthentication extends React.Component { componentDidMount() { const {user, update, match} = this.props; + console.log("2fa start"); + debugger; + console.log(user); + console.log(update); + console.log(match); if (user.guest) { const second_fa_uuid = match.params.second_fa_uuid; const urlSearchParams = new URLSearchParams(window.location.search); From fff335171d0949783ef7666e59f2c400dd220ae6 Mon Sep 17 00:00:00 2001 From: Bas Zoetekouw Date: Tue, 7 Jun 2022 13:11:16 +0200 Subject: [PATCH 13/25] Explicitly disable MFA validity if we are asking for mfa --- server/api/user_saml.py | 1 + 1 file changed, 1 insertion(+) diff --git a/server/api/user_saml.py b/server/api/user_saml.py index 77ea975d0..5e5779d5e 100644 --- a/server/api/user_saml.py +++ b/server/api/user_saml.py @@ -216,6 +216,7 @@ def not_authorized_func(service_name, status): if status == SECOND_FA_REQUIRED: # Internal contract, in case of SECOND_FA_REQUIRED we get the User instance returned user = service_name + user.second_factor_confirmed = False user.second_fa_uuid = str(uuid.uuid4()) db.session.merge(user) db.session.commit() From 74a8aa6d94babd901af97639f8349246351e4a49 Mon Sep 17 00:00:00 2001 From: Bas Zoetekouw Date: Tue, 7 Jun 2022 13:57:40 +0200 Subject: [PATCH 14/25] debug --- .github/workflows/main.yml | 168 +++++++++--------- .../src/pages/SecondFactorAuthentication.jsx | 4 + 2 files changed, 88 insertions(+), 84 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 75095b334..48476e062 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -15,89 +15,89 @@ on: workflow_dispatch: jobs: - Server_tests: - name: Server tests - if: always() - - runs-on: ubuntu-latest - - # Test different python versions - strategy: - fail-fast: false - matrix: - python-version: ['3.7', '3.9', '3.10'] - - services: - # How to use MySQL - mysql: - image: mysql:5.7 - env: - MYSQL_ROOT_PASSWORD: root - ports: - - 3306:3306 - options: --health-cmd="mysqladmin ping" --health-interval=10s --health-timeout=5s --health-retries=3 - redis: - # Docker Hub image - image: redis - # Set health checks to wait until redis has started - options: >- - --health-cmd "redis-cli ping" - --health-interval 10s - --health-timeout 5s - --health-retries 5 - ports: - # Maps port 6379 on service container to the host - - 6379:6379 - - steps: - - name: Setup mysql server - run: > - mysql -uroot -proot -h127.0.0.1 -e " - DROP DATABASE IF EXISTS sbs_test; - CREATE DATABASE IF NOT EXISTS sbs_test DEFAULT CHARACTER SET utf8mb4 DEFAULT COLLATE utf8mb4_unicode_ci; - CREATE USER 'sbs'@'%' IDENTIFIED BY 'sbs'; - GRANT ALL PRIVILEGES ON *.* TO 'sbs'@'%' WITH GRANT OPTION; - " - - name: Install SAML2 dependencies - run: | - sudo apt-get update - sudo apt-get install -y libxml2-dev libxmlsec1-dev - # Run Checkout code - - name: Checkout - uses: actions/checkout@v3 - - - name: Set up Python ${{ matrix.python-version }} - uses: actions/setup-python@v3 - with: - python-version: ${{ matrix.python-version }} - cache: 'pip' - cache-dependency-path: 'server/requirements/*.txt' - - - name: Display Python version - run: python -c "import sys; print(sys.version)" - - - name: Install dependencies - run: | - python -m pip install pip setuptools wheel - pip install -r ./server/requirements/test.txt - pip install codecov flake8 - - # Setup tmate session - #- name: Setup tmate session - # uses: mxschmitt/action-tmate@v3 - - - name: Run flake8 - run: | - flake8 ./server/ - - - name: Run tests - run: | - pytest --cov=server server/test - - - name: The job has succeeded - run: codecov --token=${{ secrets.CODECOV_TOKEN }} - if: ${{ success() }} - +# Server_tests: +# name: Server tests +# if: always() +# +# runs-on: ubuntu-latest +# +# # Test different python versions +# strategy: +# fail-fast: false +# matrix: +# python-version: ['3.7', '3.9', '3.10'] +# +# services: +# # How to use MySQL +# mysql: +# image: mysql:5.7 +# env: +# MYSQL_ROOT_PASSWORD: root +# ports: +# - 3306:3306 +# options: --health-cmd="mysqladmin ping" --health-interval=10s --health-timeout=5s --health-retries=3 +# redis: +# # Docker Hub image +# image: redis +# # Set health checks to wait until redis has started +# options: >- +# --health-cmd "redis-cli ping" +# --health-interval 10s +# --health-timeout 5s +# --health-retries 5 +# ports: +# # Maps port 6379 on service container to the host +# - 6379:6379 +# +# steps: +# - name: Setup mysql server +# run: > +# mysql -uroot -proot -h127.0.0.1 -e " +# DROP DATABASE IF EXISTS sbs_test; +# CREATE DATABASE IF NOT EXISTS sbs_test DEFAULT CHARACTER SET utf8mb4 DEFAULT COLLATE utf8mb4_unicode_ci; +# CREATE USER 'sbs'@'%' IDENTIFIED BY 'sbs'; +# GRANT ALL PRIVILEGES ON *.* TO 'sbs'@'%' WITH GRANT OPTION; +# " +# - name: Install SAML2 dependencies +# run: | +# sudo apt-get update +# sudo apt-get install -y libxml2-dev libxmlsec1-dev +# # Run Checkout code +# - name: Checkout +# uses: actions/checkout@v3 +# +# - name: Set up Python ${{ matrix.python-version }} +# uses: actions/setup-python@v3 +# with: +# python-version: ${{ matrix.python-version }} +# cache: 'pip' +# cache-dependency-path: 'server/requirements/*.txt' +# +# - name: Display Python version +# run: python -c "import sys; print(sys.version)" +# +# - name: Install dependencies +# run: | +# python -m pip install pip setuptools wheel +# pip install -r ./server/requirements/test.txt +# pip install codecov flake8 +# +# # Setup tmate session +# #- name: Setup tmate session +# # uses: mxschmitt/action-tmate@v3 +# +# - name: Run flake8 +# run: | +# flake8 ./server/ +# +# - name: Run tests +# run: | +# pytest --cov=server server/test +# +# - name: The job has succeeded +# run: codecov --token=${{ secrets.CODECOV_TOKEN }} +# if: ${{ success() }} +# Client_build: name: Client build @@ -191,7 +191,7 @@ jobs: ( github.ref_type=='tag' || github.ref_name=='main' ) needs: - "Client_build" - - "Server_tests" +# - "Server_tests" runs-on: ubuntu-latest diff --git a/client/src/pages/SecondFactorAuthentication.jsx b/client/src/pages/SecondFactorAuthentication.jsx index cf06ede44..1ee8f4263 100644 --- a/client/src/pages/SecondFactorAuthentication.jsx +++ b/client/src/pages/SecondFactorAuthentication.jsx @@ -54,6 +54,7 @@ class SecondFactorAuthentication extends React.Component { console.log(update); console.log(match); if (user.guest) { + console.log("path 1"); const second_fa_uuid = match.params.second_fa_uuid; const urlSearchParams = new URLSearchParams(window.location.search); const continueUrl = urlSearchParams.get("continue_url"); @@ -74,8 +75,10 @@ class SecondFactorAuthentication extends React.Component { this.props.history.push("/landing"); } } else if (user.second_factor_confirmed && !update) { + console.log("path 2"); this.props.history.push("/home") } else if (!user.second_factor_auth || update) { + console.log("path 3"); get2fa().then(res => { this.setState({ qrCode: res.qr_code_base64, @@ -85,6 +88,7 @@ class SecondFactorAuthentication extends React.Component { this.focusCode(); }); } else { + console.log("path 4"); this.setState({loading: false}); this.focusCode(); } From d4d63fcc8a8908db21ee3a43d4727629af913025 Mon Sep 17 00:00:00 2001 From: Bas Zoetekouw Date: Tue, 7 Jun 2022 14:36:15 +0200 Subject: [PATCH 15/25] debug --- client/src/pages/App.jsx | 3 ++- .../src/pages/SecondFactorAuthentication.jsx | 22 ++++++++++++++----- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/client/src/pages/App.jsx b/client/src/pages/App.jsx index 2958b2105..5f4f1059a 100644 --- a/client/src/pages/App.jsx +++ b/client/src/pages/App.jsx @@ -233,7 +233,8 @@ class App extends React.Component { return }}/> }/> { From ae6d5a3c7a6e24ab99dec8e9c920cce35b545acc Mon Sep 17 00:00:00 2001 From: Bas Zoetekouw Date: Tue, 7 Jun 2022 14:49:22 +0200 Subject: [PATCH 16/25] always ask for 2fa if wew request /2fa page --- client/src/pages/SecondFactorAuthentication.jsx | 7 ------- 1 file changed, 7 deletions(-) diff --git a/client/src/pages/SecondFactorAuthentication.jsx b/client/src/pages/SecondFactorAuthentication.jsx index c7fbaea9f..06575d76a 100644 --- a/client/src/pages/SecondFactorAuthentication.jsx +++ b/client/src/pages/SecondFactorAuthentication.jsx @@ -82,13 +82,6 @@ class SecondFactorAuthentication extends React.Component { } else { this.props.history.push("/landing"); } - } else if (user.second_factor_confirmed && !update) { - console.log("path 2"); - if (continueUrl) { - window.location.href = continueUrl - } else { - this.props.history.push("/home") - } } else if (!user.second_factor_auth || update) { console.log("path 3"); get2fa().then(res => { From 29a55778b24df75b496a08e9c68193d61d304420 Mon Sep 17 00:00:00 2001 From: Bas Zoetekouw Date: Tue, 7 Jun 2022 14:59:42 +0200 Subject: [PATCH 17/25] Set contineuUrl --- client/src/pages/SecondFactorAuthentication.jsx | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/client/src/pages/SecondFactorAuthentication.jsx b/client/src/pages/SecondFactorAuthentication.jsx index 06575d76a..e8bce17bf 100644 --- a/client/src/pages/SecondFactorAuthentication.jsx +++ b/client/src/pages/SecondFactorAuthentication.jsx @@ -94,7 +94,10 @@ class SecondFactorAuthentication extends React.Component { }); } else { console.log("path 4"); - this.setState({loading: false}); + this.setState({ + continueUrl: continueUrl, + loading: false + }); this.focusCode(); } } From ea3866340ed4cac9509078c411650f2850adfdc2 Mon Sep 17 00:00:00 2001 From: Bas Zoetekouw Date: Tue, 7 Jun 2022 15:16:33 +0200 Subject: [PATCH 18/25] Fix 2fa call --- client/src/pages/SecondFactorAuthentication.jsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/client/src/pages/SecondFactorAuthentication.jsx b/client/src/pages/SecondFactorAuthentication.jsx index e8bce17bf..d0e3dc6c4 100644 --- a/client/src/pages/SecondFactorAuthentication.jsx +++ b/client/src/pages/SecondFactorAuthentication.jsx @@ -52,6 +52,8 @@ class SecondFactorAuthentication extends React.Component { const {config, user, update, match} = this.props; const urlSearchParams = new URLSearchParams(window.location.search); + const second_fa_uuid = match.params.second_fa_uuid; + const continueUrl = urlSearchParams.get("continue_url"); const continueUrlTrusted = config.continue_eduteams_redirect_uri; if (continueUrl && !continueUrl.toLowerCase().startsWith(continueUrlTrusted.toLowerCase())) { @@ -65,7 +67,6 @@ class SecondFactorAuthentication extends React.Component { if (user.guest) { console.log("path 1"); - const second_fa_uuid = match.params.second_fa_uuid; if (second_fa_uuid && continueUrl) { //We need to know if this is a new user. We use the second_fa_uuid for this get2faProxyAuthz(second_fa_uuid) @@ -95,6 +96,7 @@ class SecondFactorAuthentication extends React.Component { } else { console.log("path 4"); this.setState({ + secondFaUuid: second_fa_uuid, continueUrl: continueUrl, loading: false }); From a3881ab09033e468d0bd8c23cc5d7d6f9b6df403 Mon Sep 17 00:00:00 2001 From: Bas Zoetekouw Date: Tue, 7 Jun 2022 15:30:22 +0200 Subject: [PATCH 19/25] restore workflows --- .github/workflows/main.yml | 168 ++++++++++++++++++------------------- 1 file changed, 84 insertions(+), 84 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 48476e062..75095b334 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -15,89 +15,89 @@ on: workflow_dispatch: jobs: -# Server_tests: -# name: Server tests -# if: always() -# -# runs-on: ubuntu-latest -# -# # Test different python versions -# strategy: -# fail-fast: false -# matrix: -# python-version: ['3.7', '3.9', '3.10'] -# -# services: -# # How to use MySQL -# mysql: -# image: mysql:5.7 -# env: -# MYSQL_ROOT_PASSWORD: root -# ports: -# - 3306:3306 -# options: --health-cmd="mysqladmin ping" --health-interval=10s --health-timeout=5s --health-retries=3 -# redis: -# # Docker Hub image -# image: redis -# # Set health checks to wait until redis has started -# options: >- -# --health-cmd "redis-cli ping" -# --health-interval 10s -# --health-timeout 5s -# --health-retries 5 -# ports: -# # Maps port 6379 on service container to the host -# - 6379:6379 -# -# steps: -# - name: Setup mysql server -# run: > -# mysql -uroot -proot -h127.0.0.1 -e " -# DROP DATABASE IF EXISTS sbs_test; -# CREATE DATABASE IF NOT EXISTS sbs_test DEFAULT CHARACTER SET utf8mb4 DEFAULT COLLATE utf8mb4_unicode_ci; -# CREATE USER 'sbs'@'%' IDENTIFIED BY 'sbs'; -# GRANT ALL PRIVILEGES ON *.* TO 'sbs'@'%' WITH GRANT OPTION; -# " -# - name: Install SAML2 dependencies -# run: | -# sudo apt-get update -# sudo apt-get install -y libxml2-dev libxmlsec1-dev -# # Run Checkout code -# - name: Checkout -# uses: actions/checkout@v3 -# -# - name: Set up Python ${{ matrix.python-version }} -# uses: actions/setup-python@v3 -# with: -# python-version: ${{ matrix.python-version }} -# cache: 'pip' -# cache-dependency-path: 'server/requirements/*.txt' -# -# - name: Display Python version -# run: python -c "import sys; print(sys.version)" -# -# - name: Install dependencies -# run: | -# python -m pip install pip setuptools wheel -# pip install -r ./server/requirements/test.txt -# pip install codecov flake8 -# -# # Setup tmate session -# #- name: Setup tmate session -# # uses: mxschmitt/action-tmate@v3 -# -# - name: Run flake8 -# run: | -# flake8 ./server/ -# -# - name: Run tests -# run: | -# pytest --cov=server server/test -# -# - name: The job has succeeded -# run: codecov --token=${{ secrets.CODECOV_TOKEN }} -# if: ${{ success() }} -# + Server_tests: + name: Server tests + if: always() + + runs-on: ubuntu-latest + + # Test different python versions + strategy: + fail-fast: false + matrix: + python-version: ['3.7', '3.9', '3.10'] + + services: + # How to use MySQL + mysql: + image: mysql:5.7 + env: + MYSQL_ROOT_PASSWORD: root + ports: + - 3306:3306 + options: --health-cmd="mysqladmin ping" --health-interval=10s --health-timeout=5s --health-retries=3 + redis: + # Docker Hub image + image: redis + # Set health checks to wait until redis has started + options: >- + --health-cmd "redis-cli ping" + --health-interval 10s + --health-timeout 5s + --health-retries 5 + ports: + # Maps port 6379 on service container to the host + - 6379:6379 + + steps: + - name: Setup mysql server + run: > + mysql -uroot -proot -h127.0.0.1 -e " + DROP DATABASE IF EXISTS sbs_test; + CREATE DATABASE IF NOT EXISTS sbs_test DEFAULT CHARACTER SET utf8mb4 DEFAULT COLLATE utf8mb4_unicode_ci; + CREATE USER 'sbs'@'%' IDENTIFIED BY 'sbs'; + GRANT ALL PRIVILEGES ON *.* TO 'sbs'@'%' WITH GRANT OPTION; + " + - name: Install SAML2 dependencies + run: | + sudo apt-get update + sudo apt-get install -y libxml2-dev libxmlsec1-dev + # Run Checkout code + - name: Checkout + uses: actions/checkout@v3 + + - name: Set up Python ${{ matrix.python-version }} + uses: actions/setup-python@v3 + with: + python-version: ${{ matrix.python-version }} + cache: 'pip' + cache-dependency-path: 'server/requirements/*.txt' + + - name: Display Python version + run: python -c "import sys; print(sys.version)" + + - name: Install dependencies + run: | + python -m pip install pip setuptools wheel + pip install -r ./server/requirements/test.txt + pip install codecov flake8 + + # Setup tmate session + #- name: Setup tmate session + # uses: mxschmitt/action-tmate@v3 + + - name: Run flake8 + run: | + flake8 ./server/ + + - name: Run tests + run: | + pytest --cov=server server/test + + - name: The job has succeeded + run: codecov --token=${{ secrets.CODECOV_TOKEN }} + if: ${{ success() }} + Client_build: name: Client build @@ -191,7 +191,7 @@ jobs: ( github.ref_type=='tag' || github.ref_name=='main' ) needs: - "Client_build" -# - "Server_tests" + - "Server_tests" runs-on: ubuntu-latest From 90d27187ae0b1c4f38d0ef27a236b8dc45a1bcba Mon Sep 17 00:00:00 2001 From: Bas Zoetekouw Date: Tue, 7 Jun 2022 16:12:27 +0200 Subject: [PATCH 20/25] Coverage --- server/api/user.py | 4 ++-- server/api/user_saml.py | 3 ++- server/test/api/test_user.py | 12 ++++++++++++ server/test/api/test_user_saml.py | 1 + 4 files changed, 17 insertions(+), 3 deletions(-) diff --git a/server/api/user.py b/server/api/user.py index 9840b521f..39414cb63 100644 --- a/server/api/user.py +++ b/server/api/user.py @@ -16,7 +16,7 @@ from onelogin.saml2.utils import OneLogin_Saml2_Utils from sqlalchemy import text, or_, bindparam, String from sqlalchemy.orm import joinedload, selectinload -from werkzeug.exceptions import Forbidden +from werkzeug.exceptions import Forbidden, InternalServerError from server.api.base import json_endpoint, query_param from server.api.base import replace_full_text_search_boolean_mode_chars @@ -311,7 +311,7 @@ def resume_session(): # this is a configuration conflict and should never happen! if idp_allowed and ssid_required: - raise Exception(f"Both IdP-based MFA and SSID-based MFA configured for IdP '{schac_home_organisation}'") + raise InternalServerError(f"Both IdP-based MFA and SSID-based MFA configured for IdP '{schac_home_organisation}'") # if IdP-base MFA is set, we assume everything is handled by the IdP, and we skip all checks here # also skip if user has already recently performed MFA diff --git a/server/api/user_saml.py b/server/api/user_saml.py index 5e5779d5e..89050f0ae 100644 --- a/server/api/user_saml.py +++ b/server/api/user_saml.py @@ -4,6 +4,7 @@ from urllib.parse import urlencode from flask import Blueprint, current_app, request as current_request +from werkzeug.exceptions import InternalServerError from server.api.base import json_endpoint, send_error_mail from server.api.service_aups import has_agreed_with @@ -154,7 +155,7 @@ def _do_attributes(uid, service_entity_id, not_authorized_func, authorized_func, # this is a configuration conflict and should never happen! if idp_allowed and ssid_required: - raise Exception(f"Both IdP-based MFA and SSID-based MFA configured for IdP '{schac_home_organisation}'") + raise InternalServerError(f"Both IdP-based MFA and SSID-based MFA configured for IdP '{schac_home_organisation}'") # if IdP-base MFA is set, we assume everything is handled by the IdP, and we skip all checks here # also skip if user has already recently performed MFA diff --git a/server/test/api/test_user.py b/server/test/api/test_user.py index 043126601..e27a3f2fd 100644 --- a/server/test/api/test_user.py +++ b/server/test/api/test_user.py @@ -360,6 +360,18 @@ def test_resume_session_with_ssid_idp(self): john = self.find_entity_by_name(User, "urn:john") self.assertTrue(john.ssid_required) + @responses.activate + def test_resume_session_with_invalid_idp(self): + responses.add(responses.POST, current_app.app_config.oidc.token_endpoint, + json={"access_token": "some_token", "id_token": self.sign_jwt({"acr": "nope"})}, + status=200) + responses.add(responses.GET, current_app.app_config.oidc.userinfo_endpoint, + json={"sub": "urn:john", "voperson_external_id": "test@erroridp.example.edu", "uid": "johnnie"}, + status=200) + responses.add(responses.GET, current_app.app_config.oidc.jwks_endpoint, + read_file("test/data/public.json"), status=200) + res = self.get("/api/users/resume-session", query_data={"code": "123456"}, response_status_code=500) + def test_query(self): res = self.get("/api/users/query", query_data={"q": "AMES"}) self.assertEqual(1, len(res)) diff --git a/server/test/api/test_user_saml.py b/server/test/api/test_user_saml.py index dbc42c2a6..7268c37e2 100644 --- a/server/test/api/test_user_saml.py +++ b/server/test/api/test_user_saml.py @@ -278,6 +278,7 @@ def test_proxy_authz_mfa_faulty_config(self): "uid": "sarah", "homeorganization": "erroridp.example.edu"}) self.assertTrue(res["error"]) + res = self.post("/api/users/proxy_authz", response_status_code=500, body={"user_id": "urn:sarah", "service_id": self.app.app_config.oidc.sram_service_entity_id, From 70391b45c44880b00537a2b813d02439d6be99b5 Mon Sep 17 00:00:00 2001 From: Bas Zoetekouw Date: Wed, 8 Jun 2022 13:10:30 +0200 Subject: [PATCH 21/25] coverage --- server/test/api/test_mfa.py | 10 ++++++++++ server/test/api/test_user.py | 16 +++++++++++----- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/server/test/api/test_mfa.py b/server/test/api/test_mfa.py index 0a77120d9..594235e13 100644 --- a/server/test/api/test_mfa.py +++ b/server/test/api/test_mfa.py @@ -48,6 +48,16 @@ def test_ssid_scenario(self): saml_sso_url = saml_auth().get_sso_url() self.assertTrue(res.location.startswith(saml_sso_url)) + # ssid response + xml_authn_b64 = self.get_authn_response("response.ok.xml") + res = self.client.post("/api/users/acs", headers={}, + data={"SAMLResponse": xml_authn_b64, + "RelayState": "http://localhost:8080/api/users/acs"}, + content_type="application/x-www-form-urlencoded") + + self.assertEqual(302, res.status_code) + self.assertEqual("https://foo.bar", res.location) + def test_2fa_invalid_totp(self): AbstractTest.set_second_factor_auth("urn:mary") diff --git a/server/test/api/test_user.py b/server/test/api/test_user.py index e27a3f2fd..37f81bd9c 100644 --- a/server/test/api/test_user.py +++ b/server/test/api/test_user.py @@ -227,14 +227,14 @@ def test_platform_admins_forbidden(self): self.login("urn:sarah") self.get("/api/users/platform_admins", with_basic_auth=False, response_status_code=403) - def test_authorization(self): - res = self.get("/api/users/authorization", query_data={"state": "http://localhost/redirect"}, + def test_authorization(self, redirect_uri="http://localhost/redirect"): + res = self.get("/api/users/authorization", query_data={"state": redirect_uri}, with_basic_auth=False) self.assertTrue("authorization_endpoint" in res) res = self.get("/api/users/authorization", with_basic_auth=False) query_dict = dict(parse.parse_qs(parse.urlsplit(res["authorization_endpoint"]).query)) - self.assertListEqual(["http://localhost/redirect"], query_dict["state"]) + self.assertListEqual([redirect_uri], query_dict["state"]) def test_resume_session_dead_end(self): res = self.get("/api/users/resume-session", response_status_code=302) @@ -326,7 +326,7 @@ def test_resume_session_with_no_acr(self): self.assertTrue(user["admin"]) @responses.activate - def test_resume_session_with_allowed_idp(self): + def test_resume_session_with_allowed_idp(self, redirect_expected="http://localhost:3000"): responses.add(responses.POST, current_app.app_config.oidc.token_endpoint, json={"access_token": "some_token", "id_token": self.sign_jwt({"acr": "nope"})}, status=200) @@ -337,7 +337,7 @@ def test_resume_session_with_allowed_idp(self): read_file("test/data/public.json"), status=200) with requests.Session(): res = self.client.get("/api/users/resume-session?code=123456") - self.assertEqual("http://localhost:3000", res.headers.get("Location")) + self.assertEqual(redirect_expected, res.headers.get("Location")) user = self.client.get("/api/users/me", ).json self.assertTrue(user["second_factor_confirmed"]) @@ -372,6 +372,12 @@ def test_resume_session_with_invalid_idp(self): read_file("test/data/public.json"), status=200) res = self.get("/api/users/resume-session", query_data={"code": "123456"}, response_status_code=500) + @responses.activate + def test_authorization_resume_redirect(self): + redirect_uri = "http://example.org/redirect_test" + self.test_authorization(redirect_uri=redirect_uri) + self.test_resume_session_with_allowed_idp(redirect_expected=redirect_uri) + def test_query(self): res = self.get("/api/users/query", query_data={"q": "AMES"}) self.assertEqual(1, len(res)) From d3540995e8eeedd5332d5efa282ee5c01cc39dce Mon Sep 17 00:00:00 2001 From: Bas Zoetekouw Date: Wed, 8 Jun 2022 13:13:36 +0200 Subject: [PATCH 22/25] flake8 --- server/test/api/test_user.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/test/api/test_user.py b/server/test/api/test_user.py index 37f81bd9c..eb6307955 100644 --- a/server/test/api/test_user.py +++ b/server/test/api/test_user.py @@ -370,7 +370,7 @@ def test_resume_session_with_invalid_idp(self): status=200) responses.add(responses.GET, current_app.app_config.oidc.jwks_endpoint, read_file("test/data/public.json"), status=200) - res = self.get("/api/users/resume-session", query_data={"code": "123456"}, response_status_code=500) + self.get("/api/users/resume-session", query_data={"code": "123456"}, response_status_code=500) @responses.activate def test_authorization_resume_redirect(self): From 1f2257faae4b824c1a5a93ca6d94a06a5db7af1b Mon Sep 17 00:00:00 2001 From: Bas Zoetekouw Date: Wed, 8 Jun 2022 13:54:12 +0200 Subject: [PATCH 23/25] Remove debugging --- client/src/pages/SecondFactorAuthentication.jsx | 8 -------- 1 file changed, 8 deletions(-) diff --git a/client/src/pages/SecondFactorAuthentication.jsx b/client/src/pages/SecondFactorAuthentication.jsx index d0e3dc6c4..7af32bc66 100644 --- a/client/src/pages/SecondFactorAuthentication.jsx +++ b/client/src/pages/SecondFactorAuthentication.jsx @@ -60,13 +60,7 @@ class SecondFactorAuthentication extends React.Component { throw new Error(`Invalid continue url: '${continueUrl}'`) } - console.log(user); - console.log(update); - console.log(match); - console.log(config); - if (user.guest) { - console.log("path 1"); if (second_fa_uuid && continueUrl) { //We need to know if this is a new user. We use the second_fa_uuid for this get2faProxyAuthz(second_fa_uuid) @@ -84,7 +78,6 @@ class SecondFactorAuthentication extends React.Component { this.props.history.push("/landing"); } } else if (!user.second_factor_auth || update) { - console.log("path 3"); get2fa().then(res => { this.setState({ qrCode: res.qr_code_base64, @@ -94,7 +87,6 @@ class SecondFactorAuthentication extends React.Component { this.focusCode(); }); } else { - console.log("path 4"); this.setState({ secondFaUuid: second_fa_uuid, continueUrl: continueUrl, From 9726bd76d7571085f37c91c47240d60499e032a7 Mon Sep 17 00:00:00 2001 From: Bas Zoetekouw Date: Wed, 8 Jun 2022 16:17:53 +0200 Subject: [PATCH 24/25] Add debug message for incoming ACRs --- server/api/user.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/server/api/user.py b/server/api/user.py index 39414cb63..17db070f1 100644 --- a/server/api/user.py +++ b/server/api/user.py @@ -292,6 +292,8 @@ def resume_session(): id_token = decode_jwt_token(encoded_id_token) idp_mfa = id_token.get("acr") == ACR_VALUES + if idp_mfa: + logger.debug(f"user {uid}: idp_mfa={idp_mfa} (ACR = '{id_token.get('acr')}')") # we're repeating some of the logic of _perform_sram_login() here # at least until EduTEAMS has transitioned to inserting a call to proxy_authz in the login flow for SBS itself From db16a6df72975ab1cd139e460aa524aeb069aa32 Mon Sep 17 00:00:00 2001 From: Bas Zoetekouw Date: Wed, 8 Jun 2022 17:02:37 +0200 Subject: [PATCH 25/25] Cleaning up --- server/api/mfa.py | 2 -- server/api/user_saml.py | 3 ++- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/server/api/mfa.py b/server/api/mfa.py index 4fea06848..48df53348 100644 --- a/server/api/mfa.py +++ b/server/api/mfa.py @@ -79,7 +79,6 @@ def _do_get2fa(schac_home_organisation, user_identifier): img.save(buffered, format="PNG") img_str = base64.b64encode(buffered.getvalue()).decode() idp_name = idp_display_name(schac_home_organisation, "en") - # hier ook ssid_required en sha en uid teruggeven return {"qr_code_base64": img_str, "secret": secret, "idp_name": idp_name}, 200 @@ -144,7 +143,6 @@ def get2fa_proxy_authz(): user = User.query.filter(User.second_fa_uuid == second_fa_uuid).one() if user.second_factor_auth: return {}, 200 - # hier ook ssid_required teruggeven return _do_get2fa(user.schac_home_organisation, user.uid) diff --git a/server/api/user_saml.py b/server/api/user_saml.py index 89050f0ae..e204b2aca 100644 --- a/server/api/user_saml.py +++ b/server/api/user_saml.py @@ -62,7 +62,8 @@ def _perform_sram_login(uid, home_organisation_uid, schac_home_organisation, iss # this is a configuration conflict and should never happen! if idp_allowed and ssid_required: - raise Exception(f"Both IdP-based MFA and SSID-based MFA configured for IdP '{schac_home_organisation}'") + raise InternalServerError("Both IdP-based MFA and SSID-based MFA configured " + f"for IdP '{schac_home_organisation}'") # if IdP-base MFA is set, we assume everything is handled by the IdP, and we skip all checks here # also skip if user has already recently performed MFA