Merge pull request #202 from SURFscz/feature/mfa-flows

Fix mfa flows

.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/[email protected]
         with:
           keep_latest: 0

client/src/pages/App.jsx

@@ -233,7 +233,8 @@ class App extends React.Component {
                                    return <Redirect to={decodeURIComponent(state)}/>
                                }}/>
                         <Route path="/2fa/:second_fa_uuid?"
-                               render={props => <SecondFactorAuthentication user={currentUser}
+                               render={props => <SecondFactorAuthentication config={config}
+                                                                            user={currentUser}
                                                                             refreshUser={this.refreshUserMemberships}
                                                                             {...props}/>}/>
                         <Route path="/2fa-update"

client/src/pages/SecondFactorAuthentication.jsx

@@ -47,11 +47,20 @@ class SecondFactorAuthentication extends React.Component {
     }
 
     componentDidMount() {
-        const {user, update, match} = this.props;
+        console.log("2fa start");
+
+        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())) {
+            throw new Error(`Invalid continue url: '${continueUrl}'`)
+        }
+
         if (user.guest) {
-            const second_fa_uuid = match.params.second_fa_uuid;
-            const urlSearchParams = new URLSearchParams(window.location.search);
-            const continueUrl = urlSearchParams.get("continue_url");
             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)
@@ -68,8 +77,6 @@ class SecondFactorAuthentication extends React.Component {
             } else {
                 this.props.history.push("/landing");
             }
-        } else if (user.second_factor_confirmed && !update) {
-            this.props.history.push("/home")
         } else if (!user.second_factor_auth || update) {
             get2fa().then(res => {
                 this.setState({
@@ -80,7 +87,11 @@ class SecondFactorAuthentication extends React.Component {
                 this.focusCode();
             });
         } else {
-            this.setState({loading: false});
+            this.setState({
+                secondFaUuid: second_fa_uuid,
+                continueUrl: continueUrl,
+                loading: false
+            });
             this.focusCode();
         }
     }

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
@@ -258,3 +259,16 @@ def sfo():
 @json_endpoint
 def jwks():
     return _get_public_key(), 200
+
+
+@mfa_api.route("/ssid_start/<second_fa_uuid>", strict_slashes=False)
+def do_ssid_redirect(second_fa_uuid):
+    logger = ctx_logger("2fa")
+
+    continue_url = query_param("continue_url", required=True)
+    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}")
+
+    return redirect_to_surf_secure_id(user)

server/api/user.py

@@ -16,13 +16,14 @@
 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
 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, 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
@@ -229,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
@@ -269,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}")
@@ -287,27 +288,66 @@ def resume_session():
         logger.info(f"Updating user {user.uid} with new claims / updated at")
         add_user_claims(user_info_json, uid, user)
 
-    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
+    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
+    #
+    # no need to repeat this logic if we already have made a decision before
+    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)
+
+        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
+
+        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 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
+        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()
 
-    idp_allowed = mfa_idp_allowed(user, user.schac_home_organisation, None)
+            logger.debug(f"Setting 2fa required for SBS login for user {uid} (ssid={ssid_required})")
+    else:
+        fallback_required = False
 
-    second_factor_confirmed = no_mfa_required or idp_mfa or idp_allowed
+    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)
+
+    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()
 
     return _redirect_to_client(cfg, second_factor_confirmed, user)
 
 
 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()
@@ -316,13 +356,21 @@ 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)
 
 
 @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)
@@ -339,6 +387,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)

server/api/user_saml.py

@@ -4,10 +4,11 @@
 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
-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,32 +41,59 @@
 
 
 # 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")
 
+    logger.debug("SBS login flow")
+
     user = User.query.filter(User.uid == uid).first()
     if not user:
         logger.debug("Creating new user in sram_login")
         user = User(uid=uid, created_by="system", updated_by="system")
 
     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)
+        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 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
+        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
+                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 +143,31 @@ 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)
+        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 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
+        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 = 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)
@@ -179,10 +218,14 @@ 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()
-            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