From 5eefcd27b9016f04ca6623518ab53b466f6ad55a Mon Sep 17 00:00:00 2001 From: Fredrik Thulin Date: Fri, 2 Jun 2023 15:47:11 +0200 Subject: [PATCH 01/30] echo authn_id in status response to make life easier in the frontend --- src/eduid/webapp/eidas/schemas.py | 1 + src/eduid/webapp/eidas/views.py | 1 + src/eduid/webapp/security/schemas.py | 2 ++ src/eduid/webapp/svipe_id/schemas.py | 1 + src/eduid/webapp/svipe_id/views.py | 1 + 5 files changed, 6 insertions(+) diff --git a/src/eduid/webapp/eidas/schemas.py b/src/eduid/webapp/eidas/schemas.py index 65e4753c2..dd45270c6 100644 --- a/src/eduid/webapp/eidas/schemas.py +++ b/src/eduid/webapp/eidas/schemas.py @@ -31,6 +31,7 @@ class EidasStatusRequestSchema(EduidSchema, CSRFRequestMixin): class EidasStatusResponseSchema(EduidSchema, CSRFResponseMixin): class StatusResponsePayload(EduidSchema, CSRFResponseMixin): + authn_id = fields.String(required=False) frontend_action = fields.String(required=True) frontend_state = fields.String(required=False) method = fields.String(required=True) diff --git a/src/eduid/webapp/eidas/views.py b/src/eduid/webapp/eidas/views.py index 972572f40..af20be055 100644 --- a/src/eduid/webapp/eidas/views.py +++ b/src/eduid/webapp/eidas/views.py @@ -69,6 +69,7 @@ def get_status(authn_id: AuthnRequestRef) -> FluxData: return error_response(message=EidasMsg.not_found) payload = { + "authn_id": authn_id, "frontend_action": authn.frontend_action, "frontend_state": authn.frontend_state, "method": authn.method, diff --git a/src/eduid/webapp/security/schemas.py b/src/eduid/webapp/security/schemas.py index 5bc079bb5..f8fafd5b1 100644 --- a/src/eduid/webapp/security/schemas.py +++ b/src/eduid/webapp/security/schemas.py @@ -67,6 +67,7 @@ class ChpassResponsePayload(CredentialList): class ChangePasswordRequestSchema(EduidSchema, CSRFRequestMixin): + authn_id = fields.String(required=False) old_password = fields.String(required=False) new_password = fields.String(required=True) @@ -91,6 +92,7 @@ class ChangePasswordSchema(PasswordSchema): csrf_token = fields.String(required=True) old_password = fields.String(required=True) new_password = fields.String(required=True) + authn_id = fields.String(required=False) @validates("new_password") def validate_custom_password(self, value, **kwargs): diff --git a/src/eduid/webapp/svipe_id/schemas.py b/src/eduid/webapp/svipe_id/schemas.py index 08815d450..9b02ef0a8 100644 --- a/src/eduid/webapp/svipe_id/schemas.py +++ b/src/eduid/webapp/svipe_id/schemas.py @@ -12,6 +12,7 @@ class SvipeIDStatusRequestSchema(EduidSchema, CSRFRequestMixin): class SvipeIDStatusResponseSchema(EduidSchema, CSRFResponseMixin): class StatusResponsePayload(EduidSchema, CSRFResponseMixin): + authn_id = fields.String(required=True) frontend_action = fields.String(required=True) frontend_state = fields.String(required=False) method = fields.String(required=True) diff --git a/src/eduid/webapp/svipe_id/views.py b/src/eduid/webapp/svipe_id/views.py index deb9a02d0..e89f699d6 100644 --- a/src/eduid/webapp/svipe_id/views.py +++ b/src/eduid/webapp/svipe_id/views.py @@ -50,6 +50,7 @@ def get_status(authn_id: OIDCState) -> FluxData: return error_response(message=SvipeIDMsg.not_found) payload = { + "authn_id": str(authn_id), "frontend_action": authn.frontend_action, "frontend_state": authn.frontend_state, "method": authn.method, From 98d896a47c339776665ff5c5107f321b750fa6c8 Mon Sep 17 00:00:00 2001 From: Fredrik Thulin Date: Fri, 2 Jun 2023 15:48:36 +0200 Subject: [PATCH 02/30] opt-in support signalling authentication requirement with 401 and JSON --- src/eduid/common/config/base.py | 2 + src/eduid/webapp/authn/helpers.py | 13 ++ src/eduid/webapp/authn/views.py | 142 +++++++++++++++--- src/eduid/webapp/common/api/messages.py | 5 +- src/eduid/webapp/common/authn/middleware.py | 34 ++++- src/eduid/webapp/common/session/namespaces.py | 3 + src/eduid/webapp/security/helpers.py | 4 + .../webapp/security/views/change_password.py | 23 ++- src/eduid/webapp/security/views/security.py | 2 +- 9 files changed, 191 insertions(+), 37 deletions(-) diff --git a/src/eduid/common/config/base.py b/src/eduid/common/config/base.py index 5d54e55aa..a4092d580 100644 --- a/src/eduid/common/config/base.py +++ b/src/eduid/common/config/base.py @@ -388,6 +388,8 @@ class EduIDBaseAppConfig(RootConfig, LoggingConfigMixin, StatsConfigMixin, Redis # The list is a list of regex that are matched against the path of the # requested URL ex. ^/test$. no_authn_urls: list[str] = Field(default=["^/status/healthy$", "^/status/sanity-check$"]) + # Feature opt-in for new-style authn responses, requires new frontend code. + enable_authn_json_response: bool = False status_cache_seconds: int = 10 # All AuthnBaseApps need this to redirect not-logged-in requests to the authn service token_service_url: str diff --git a/src/eduid/webapp/authn/helpers.py b/src/eduid/webapp/authn/helpers.py index 81e0ec525..452cedea5 100644 --- a/src/eduid/webapp/authn/helpers.py +++ b/src/eduid/webapp/authn/helpers.py @@ -1,8 +1,10 @@ +from enum import unique import logging from typing import Optional from eduid.common.misc.timeutil import utc_now from eduid.userdb.credentials import Credential +from eduid.webapp.common.api.messages import TranslatableMsg from eduid.webapp.common.authn.acs_enums import AuthnAcsAction from eduid.webapp.common.session import session from eduid.webapp.common.session.namespaces import SP_AuthnRequest @@ -31,3 +33,14 @@ def _credential_recently_used(credential: Credential, action: Optional[SP_AuthnR if 0 < age < max_age: return True return False + + +@unique +class AuthnMsg(TranslatableMsg): + """ + Messages sent to the front end with information on the results of the + attempted operations on the back end. + """ + + # Status requested for unknown authn_id + not_found = "authn.not_found" diff --git a/src/eduid/webapp/authn/views.py b/src/eduid/webapp/authn/views.py index 8d1b7d11c..638569b4b 100644 --- a/src/eduid/webapp/authn/views.py +++ b/src/eduid/webapp/authn/views.py @@ -30,6 +30,7 @@ # ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE # POSSIBILITY OF SUCH DAMAGE. # +from dataclasses import dataclass import uuid from typing import Optional @@ -39,14 +40,30 @@ from saml2.ident import decode from saml2.metadata import entity_descriptor from saml2.saml import NAMEID_FORMAT_UNSPECIFIED, NameID, Subject +from saml2.request import AuthnRequest from werkzeug.exceptions import Forbidden from werkzeug.wrappers import Response as WerkzeugResponse from eduid.userdb.exceptions import MultipleUsersReturned, UserDoesNotExist from eduid.webapp.authn import acs_actions # acs_action needs to be imported to be loaded from eduid.webapp.authn.app import current_authn_app as current_app +from eduid.webapp.authn.helpers import AuthnMsg +from eduid.webapp.authn.schemas import ( + AuthnAuthenticateRequestSchema, + AuthnCommonResponseSchema, + AuthnStatusRequestSchema, + AuthnStatusResponseSchema, +) +from eduid.webapp.common.api.decorators import MarshalWith, UnmarshalWith from eduid.webapp.common.api.errors import EduidErrorsContext, goto_errors_response -from eduid.webapp.common.api.messages import CommonMsg, redirect_with_msg +from eduid.webapp.common.api.messages import ( + CommonMsg, + FluxData, + TranslatableMsg, + error_response, + redirect_with_msg, + success_response, +) from eduid.webapp.common.api.utils import sanitise_redirect_url from eduid.webapp.common.authn.acs_enums import AuthnAcsAction from eduid.webapp.common.authn.acs_registry import ACSArgs, get_action @@ -69,7 +86,7 @@ def login() -> WerkzeugResponse: """ login view, redirects to SAML2 IdP """ - return _authn(AuthnAcsAction.login, same_user=False) + return _old_authn(AuthnAcsAction.login, same_user=False) @authn_views.route("/reauthn") @@ -78,7 +95,7 @@ def reauthn() -> WerkzeugResponse: login view with force authn, redirects to SAML2 IdP """ session.common.is_logged_in = False - return _authn(AuthnAcsAction.reauthn, force_authn=True) + return _old_authn(AuthnAcsAction.reauthn, force_authn=True) @authn_views.route("/chpass") @@ -86,7 +103,7 @@ def chpass() -> WerkzeugResponse: """ Reauthn view, sends a SAML2 reauthn request to the IdP. """ - return _authn(AuthnAcsAction.change_password, force_authn=True) + return _old_authn(AuthnAcsAction.change_password, force_authn=True) @authn_views.route("/terminate") @@ -94,16 +111,65 @@ def terminate() -> WerkzeugResponse: """ Reauthn view, sends a SAML2 reauthn request to the IdP. """ - return _authn(AuthnAcsAction.terminate_account, force_authn=True) + return _old_authn(AuthnAcsAction.terminate_account, force_authn=True) + + +@authn_views.route("/authenticate", methods=["POST"]) +@UnmarshalWith(AuthnAuthenticateRequestSchema) +@MarshalWith(AuthnCommonResponseSchema) +def authenticate( + method: str, + frontend_action: str, + frontend_state: Optional[str] = None, + same_user: Optional[bool] = None, + force_authn: Optional[bool] = None, +) -> FluxData: + current_app.logger.debug(f"authenticate called with frontend_action: {frontend_action}") + + # TODO: One idea would be for the backend to enforce the parameters needed for a certain frontend operation + # (such as re-authn before password change) here, instead of leaving it up to the frontend to know what + # the requirements will actually be for a certain operation, and fail to present an authentication that + # meets those requirements later. + + sp_authn = SP_AuthnRequest( + post_authn_action=AuthnAcsAction.login, + redirect_url=None, + frontend_action=frontend_action, + frontend_state=frontend_state, + method=method, + ) + result = _authn(sp_authn, force_authn=bool(force_authn), same_user=bool(same_user), idp=_get_idp()) -def _authn(action: AuthnAcsAction, force_authn: bool = False, same_user: bool = True) -> WerkzeugResponse: - # TODO: Stop using the "next" parameter, because it opens up for redirect attacks. - # Instead, let frontend say "frontend_action=chpass" and we look up the finish_url - # for "chpass" in configuration. - redirect_url = sanitise_redirect_url(request.args.get("next"), current_app.conf.saml2_login_redirect_url) - frontend_action = request.args.get("frontend_action", FALLBACK_FRONTEND_ACTION) + if result.error: + return error_response(message=result.error) + + return success_response(payload={"location": result.url}) + +# get_status to not get tangled up in /status/healthy and the like +@authn_views.route("/get_status", methods=["POST"]) +@UnmarshalWith(AuthnStatusRequestSchema) +@MarshalWith(AuthnStatusResponseSchema) +def get_status(authn_id: AuthnRequestRef) -> FluxData: + authn = session.authn.sp.authns.get(authn_id) + if not authn: + return error_response(message=AuthnMsg.not_found) + + payload = { + "authn_id": authn_id, + "frontend_action": authn.frontend_action, + "frontend_state": authn.frontend_state, + "method": authn.method, + "error": bool(authn.error), + } + if authn.status is not None: + payload["status"] = authn.status + + return success_response(payload=payload) + + +def _get_idp() -> str: # In the future, we might want to support choosing the IdP somehow but for now # the only supported configuration is one (1) IdP. _configured_idps = current_app.saml2_config.getattr("idp") @@ -113,6 +179,19 @@ def _authn(action: AuthnAcsAction, force_authn: bool = False, same_user: bool = raise RuntimeError("Unknown SAML2 idp config") # For now, we will only ever use the single configured IdP idp = list(_configured_idps.keys())[0] + assert isinstance(idp, str) + return idp + + +def _old_authn(action: AuthnAcsAction, force_authn: bool = False, same_user: bool = True) -> WerkzeugResponse: + # TODO: Stop using the "next" parameter, because it opens up for redirect attacks. + # Instead, let frontend say "frontend_action=chpass" and we look up the finish_url + # for "chpass" in configuration. + redirect_url = sanitise_redirect_url(request.args.get("next"), current_app.conf.saml2_login_redirect_url) + frontend_action = request.args.get("frontend_action", FALLBACK_FRONTEND_ACTION) + + idp = _get_idp() + # Be somewhat backwards compatible and check the provided IdP parameter _requested_idp = request.args.get("idp") if _requested_idp and _requested_idp != idp: @@ -120,20 +199,32 @@ def _authn(action: AuthnAcsAction, force_authn: bool = False, same_user: bool = # TODO: use goto_errors_response() raise Forbidden("Requested IdP not allowed") - # finish_url = current_app.conf.frontend_action_finish_url.get(frontend_action) - # if not finish_url: - # current_app.logger.warning(f'No finish_url for frontend_action {frontend_action}') - # # TODO: use goto_errors_response() - # raise Forbidden('Unknown frontend_action') + sp_authn = SP_AuthnRequest(post_authn_action=action, redirect_url=redirect_url, frontend_action=frontend_action) + result = _authn(sp_authn, force_authn, same_user, idp) + if not result.url: + raise RuntimeError("No redirect URL returned from _authn") + + current_app.logger.debug(f"Redirecting user to the IdP: {result.url}") + return redirect(result.url) + + +@dataclass +class AuthnResult: + authn_id: Optional[AuthnRequestRef] = None + error: Optional[TranslatableMsg] = None + url: Optional[str] = None + + +def _authn(sp_authn: SP_AuthnRequest, force_authn: bool, same_user: bool, idp: str) -> AuthnResult: _authn_id = AuthnRequestRef(str(uuid.uuid4())) # Filter out any previous authns with the same post_authn_action, both to keep the size of the session # below an upper bound, and because we currently need to use the post_authn_action value to find the # authn data for a specific action. - session.authn.sp.authns = {k: v for k, v in session.authn.sp.authns.items() if v.post_authn_action != action} - session.authn.sp.authns[_authn_id] = SP_AuthnRequest( - post_authn_action=action, redirect_url=redirect_url, frontend_action=frontend_action - ) + session.authn.sp.authns = { + k: v for k, v in session.authn.sp.authns.items() if v.post_authn_action != sp_authn.post_authn_action + } + session.authn.sp.authns[_authn_id] = sp_authn subject = None if same_user: @@ -152,11 +243,10 @@ def _authn(action: AuthnAcsAction, force_authn: bool = False, same_user: bool = digest_alg=current_app.conf.authn_digest_alg, subject=subject, ) - current_app.logger.info(f"Redirecting the user to the IdP for {action} (frontend_action {frontend_action})") + current_app.logger.info(f"Redirecting the user to the IdP for {sp_authn}") current_app.logger.debug(f"Stored SP_AuthnRequest[{_authn_id}]: {session.authn.sp.authns[_authn_id]}") _idp_redirect_url = get_location(authn_request) - current_app.logger.debug(f"Redirecting user to the IdP: {_idp_redirect_url}") - return redirect(_idp_redirect_url) + return AuthnResult(authn_id=_authn_id, url=_idp_redirect_url) @authn_views.route("/saml2-acs", methods=["POST"]) @@ -204,13 +294,15 @@ def assertion_consumer_service() -> WerkzeugResponse: rp=current_app.saml2_config.entityid, ) + formatted_finish_url = finish_url.format(app_name=current_app.conf.app_name, authn_id=assertion.authn_req_ref) + if not result.success: current_app.logger.info(f"SAML ACS action failed: {result.message}") # update session so this error can be retrieved from the /status endpoint _msg = result.message or CommonMsg.temp_problem args.authn_req.error = _msg.value # Including the error in the redirect URL is deprecated and should be removed once frontend stops using it - return redirect_with_msg(finish_url, _msg, error=True) + return redirect_with_msg(formatted_finish_url, _msg, error=True) current_app.logger.debug(f"SAML ACS action successful") @@ -218,7 +310,7 @@ def assertion_consumer_service() -> WerkzeugResponse: current_app.logger.debug(f"SAML ACS action returned a response") return result.response - return redirect(finish_url) + return redirect(formatted_finish_url) def _get_authn_name_id(session: EduidSession) -> Optional[NameID]: diff --git a/src/eduid/webapp/common/api/messages.py b/src/eduid/webapp/common/api/messages.py index 583870e3a..4b85d8a5f 100644 --- a/src/eduid/webapp/common/api/messages.py +++ b/src/eduid/webapp/common/api/messages.py @@ -30,7 +30,7 @@ # POSSIBILITY OF SUCH DAMAGE. # from copy import copy -from dataclasses import dataclass +from dataclasses import asdict, dataclass from enum import Enum, unique from typing import Any, Mapping, Optional, Union from urllib.parse import parse_qsl, urlencode, urlsplit, urlunsplit @@ -85,6 +85,9 @@ class FluxData: status: FluxResponseStatus payload: Mapping[str, Any] + def to_dict(self) -> dict[str, Any]: + return asdict(self) + def success_response( payload: Optional[Mapping[str, Any]] = None, message: Optional[Union[TranslatableMsg, str]] = None diff --git a/src/eduid/webapp/common/authn/middleware.py b/src/eduid/webapp/common/authn/middleware.py index 488202f88..3ae652e26 100644 --- a/src/eduid/webapp/common/authn/middleware.py +++ b/src/eduid/webapp/common/authn/middleware.py @@ -1,16 +1,19 @@ +import json import logging import re from abc import ABCMeta -from typing import Any, Callable, Union +from typing import Any, AnyStr, Callable, Mapping, Union, cast from urllib.parse import parse_qs, urlencode, urlparse, urlunparse -from flask import current_app +from flask import current_app, jsonify, make_response from werkzeug.wrappers import Response from werkzeug.wsgi import get_current_url from eduid.common.config.base import EduIDBaseAppConfig from eduid.common.utils import urlappend from eduid.webapp.common.api.app import EduIDBaseApp +from eduid.webapp.common.api.messages import error_response +from eduid.webapp.common.api.schemas.base import FluxStandardAction from eduid.webapp.common.session import session from eduid.webapp.common.session.redis_session import NoSessionDataFoundException @@ -27,7 +30,9 @@ class AuthnBaseApp(EduIDBaseApp, metaclass=ABCMeta): and in case it isn't, redirects to the authn service. """ - def __call__(self, environ: dict[str, Any], start_response: TStartResponse) -> Union[Response, list[str]]: + def __call__( + self, environ: dict[str, Any], start_response: TStartResponse + ) -> Union[Response, list[bytes], Mapping[str, Any]]: # let request with method OPTIONS pass through if environ["REQUEST_METHOD"] == "OPTIONS": return super().__call__(environ, start_response) @@ -62,9 +67,26 @@ def __call__(self, environ: dict[str, Any], start_response: TStartResponse) -> U # If HTTP_COOKIE is not removed self.request_context(environ) below # will try to look up the Session data in the backend - ts_url = urlappend(conf.token_service_url, "login") - - params = {"next": next_url} + ts_url = urlappend(conf.token_service_url, "login") + + params = {"next": next_url} + + if conf.enable_authn_json_response: + # NEW way, respond with a 401 with a JSON payload + params["login_url"] = ts_url + res = error_response(message="Authentication required", payload=params) + _encoded = cast(Mapping[str, Any], FluxStandardAction().dump(res.to_dict())) + body = json.dumps(_encoded).encode("utf-8") + headers = [ + ("Content-Type", "application/json"), + ("Content-Length", str(len(body))), + ("WWW-Authenticate", "eduID"), + ] + start_response("401 Unauthorized", headers) + # return make_response(jsonify(_encoded), 401) + return [body] + + # OLD way, respond with a 301 redirect url_parts = list(urlparse(ts_url)) query = parse_qs(url_parts[4]) diff --git a/src/eduid/webapp/common/session/namespaces.py b/src/eduid/webapp/common/session/namespaces.py index 76fabb7c3..005f73ee6 100644 --- a/src/eduid/webapp/common/session/namespaces.py +++ b/src/eduid/webapp/common/session/namespaces.py @@ -258,6 +258,9 @@ class SP_AuthnRequest(BaseAuthnRequest): # proofing_credential_id is the credential being person-proofed, when doing that proofing_credential_id: Optional[ElementKey] = None redirect_url: Optional[str] # Deprecated, use frontend_action to get return URL from config instead + force_authn: bool = False # a new authentication was required + same_user: bool = False # the same user was required to log in, such as when entering the security center + consumed: bool = False # an operation that requires a new authentication has used this one already PySAML2Dicts = NewType("PySAML2Dicts", dict[str, dict[str, Any]]) diff --git a/src/eduid/webapp/security/helpers.py b/src/eduid/webapp/security/helpers.py index 49e0b6bd0..788301642 100644 --- a/src/eduid/webapp/security/helpers.py +++ b/src/eduid/webapp/security/helpers.py @@ -157,6 +157,10 @@ def check_reauthn(authn: Optional[SP_AuthnRequest], max_age: timedelta) -> Optio current_app.logger.info(f"Action requires re-authentication") return error_response(message=SecurityMsg.no_reauthn) + if authn.consumed: + current_app.logger.info(f"The authentication presented has already been consumed") + return error_response(message=SecurityMsg.no_reauthn) + delta = utc_now() - authn.authn_instant if delta > max_age: diff --git a/src/eduid/webapp/security/views/change_password.py b/src/eduid/webapp/security/views/change_password.py index d9ac384d7..4880e5103 100644 --- a/src/eduid/webapp/security/views/change_password.py +++ b/src/eduid/webapp/security/views/change_password.py @@ -44,6 +44,7 @@ from eduid.webapp.common.authn.acs_enums import AuthnAcsAction from eduid.webapp.common.authn.vccs import change_password from eduid.webapp.common.session import session +from eduid.webapp.common.session.namespaces import AuthnRequestRef from eduid.webapp.security.app import current_security_app as current_app from eduid.webapp.security.helpers import ( SecurityMsg, @@ -63,7 +64,7 @@ @change_password_views.route("/suggested-password", methods=["GET"]) @MarshalWith(SuggestedPasswordResponseSchema) @require_user -def get_suggested(user) -> FluxData: +def get_suggested(user: User) -> FluxData: """ View to get a suggested password for the logged user. """ @@ -83,16 +84,29 @@ def get_suggested(user) -> FluxData: @UnmarshalWith(ChangePasswordRequestSchema) @MarshalWith(SecurityResponseSchema) @require_user -def change_password_view(user: User, new_password: str, old_password: Optional[str] = None) -> FluxData: +def change_password_view( + user: User, new_password: str, old_password: Optional[str] = None, authn_id: Optional[str] = None +) -> FluxData: """ View to change the password """ - authn = session.authn.sp.get_authn_for_action(AuthnAcsAction.change_password) + current_app.logger.debug(f"Set password called with authn_id {authn_id}") + authn = None + if authn_id: + authn = session.authn.sp.authns.get(AuthnRequestRef(authn_id)) + + if not authn_id: + # TODO: Remove this after a release of the new frontend to production. + authn = session.authn.sp.get_authn_for_action(AuthnAcsAction.change_password) + current_app.logger.debug(f"change_password called with authn {authn}") + _need_reauthn = check_reauthn(authn, current_app.conf.chpass_reauthn_timeout) if _need_reauthn: return _need_reauthn + assert authn is not None # please mypy (if authn was None we would have returned with _need_reauthn above) + if not new_password or (current_app.conf.chpass_old_password_needed and not old_password): return error_response(message=SecurityMsg.chpass_no_data) @@ -100,7 +114,6 @@ def change_password_view(user: User, new_password: str, old_password: Optional[s if old_password is None: # Try to find the password credential that the user used for reauthn. That one should be revoked. # If we do not find it we will revoke all of the users passwords. - assert authn is not None # please mypy (if authn was None we would have returned with _need_reauthn above) for cred_id in authn.credentials_used: credential = user.credentials.find(cred_id) if isinstance(credential, Password): @@ -146,6 +159,8 @@ def change_password_view(user: User, new_password: str, old_password: Optional[s current_app.stats.count(name="security_password_changed") current_app.logger.info(f"Changed password for user") + authn.consumed = True + return success_response( payload={"credentials": compile_credential_list(security_user)}, message=SecurityMsg.change_password_success, diff --git a/src/eduid/webapp/security/views/security.py b/src/eduid/webapp/security/views/security.py index fcb48616d..f46eb4579 100644 --- a/src/eduid/webapp/security/views/security.py +++ b/src/eduid/webapp/security/views/security.py @@ -107,7 +107,7 @@ def get_suggested(user: User): return suggested -# TODO: Remove this when frontend for new change password view exist +# TODO: Remove this when frontend for new change password view exist (new endpoint is /change-password/set-password) @security_views.route("/change-password", methods=["POST"]) @MarshalWith(ChpassResponseSchema) @require_user From e5727eeeac17f6f47accc360fdb6e74806f48c1a Mon Sep 17 00:00:00 2001 From: Fredrik Thulin Date: Wed, 7 Jun 2023 14:15:16 +0200 Subject: [PATCH 03/30] implement asking for MFA --- src/eduid/webapp/authn/views.py | 49 ++++++++++++++++--- src/eduid/webapp/common/session/namespaces.py | 13 +++-- .../security/tests/test_change_password.py | 4 +- .../webapp/security/views/change_password.py | 10 ++++ 4 files changed, 65 insertions(+), 11 deletions(-) diff --git a/src/eduid/webapp/authn/views.py b/src/eduid/webapp/authn/views.py index 638569b4b..548965cc6 100644 --- a/src/eduid/webapp/authn/views.py +++ b/src/eduid/webapp/authn/views.py @@ -43,6 +43,7 @@ from saml2.request import AuthnRequest from werkzeug.exceptions import Forbidden from werkzeug.wrappers import Response as WerkzeugResponse +from eduid.userdb.credentials.fido import FidoCredential from eduid.userdb.exceptions import MultipleUsersReturned, UserDoesNotExist from eduid.webapp.authn import acs_actions # acs_action needs to be imported to be loaded @@ -71,7 +72,7 @@ from eduid.webapp.common.authn.eduid_saml2 import get_authn_request, process_assertion, saml_logout from eduid.webapp.common.authn.utils import check_previous_identification, get_location from eduid.webapp.common.session import EduidSession, session -from eduid.webapp.common.session.namespaces import AuthnRequestRef, LoginApplication, SP_AuthnRequest +from eduid.webapp.common.session.namespaces import AuthnParameters, AuthnRequestRef, LoginApplication, SP_AuthnRequest assert acs_actions # make sure nothing optimises away the import of this, as it is needed to execute @acs_actions @@ -80,6 +81,8 @@ # use this as frontend_action to fall back to the old mechanism using redirect_url FALLBACK_FRONTEND_ACTION = "fallback-redirect-url" +REFEDS_MFA = "https://refeds.org/profile/mfa" + @authn_views.route("/login") def login() -> WerkzeugResponse: @@ -123,6 +126,8 @@ def authenticate( frontend_state: Optional[str] = None, same_user: Optional[bool] = None, force_authn: Optional[bool] = None, + high_security: Optional[bool] = None, + force_mfa: Optional[bool] = None, ) -> FluxData: current_app.logger.debug(f"authenticate called with frontend_action: {frontend_action}") @@ -131,15 +136,37 @@ def authenticate( # the requirements will actually be for a certain operation, and fail to present an authentication that # meets those requirements later. + req_authn_ctx = [] + _request_mfa = False + if high_security: + user = None + if session.common.eppn: + user = current_app.central_userdb.get_user_by_eppn(session.common.eppn) + if user.credentials.filter(FidoCredential): + _request_mfa = True + current_app.logger.debug( + f"High security authentication for user user {session.common.eppn} requested, available: {_request_mfa}" + ) + + if force_mfa or _request_mfa: + req_authn_ctx = [REFEDS_MFA] + sp_authn = SP_AuthnRequest( post_authn_action=AuthnAcsAction.login, redirect_url=None, frontend_action=frontend_action, frontend_state=frontend_state, method=method, + req_authn_ctx=req_authn_ctx, + params=AuthnParameters( + force_authn=bool(force_authn), + force_mfa=bool(force_mfa), + high_security=bool(high_security), + same_user=bool(same_user), + ), ) - result = _authn(sp_authn, force_authn=bool(force_authn), same_user=bool(same_user), idp=_get_idp()) + result = _authn(sp_authn, idp=_get_idp()) if result.error: return error_response(message=result.error) @@ -199,9 +226,17 @@ def _old_authn(action: AuthnAcsAction, force_authn: bool = False, same_user: boo # TODO: use goto_errors_response() raise Forbidden("Requested IdP not allowed") - sp_authn = SP_AuthnRequest(post_authn_action=action, redirect_url=redirect_url, frontend_action=frontend_action) + sp_authn = SP_AuthnRequest( + post_authn_action=action, + redirect_url=redirect_url, + frontend_action=frontend_action, + params=AuthnParameters( + force_authn=bool(force_authn), + same_user=bool(same_user), + ), + ) - result = _authn(sp_authn, force_authn, same_user, idp) + result = _authn(sp_authn, idp) if not result.url: raise RuntimeError("No redirect URL returned from _authn") @@ -216,7 +251,7 @@ class AuthnResult: url: Optional[str] = None -def _authn(sp_authn: SP_AuthnRequest, force_authn: bool, same_user: bool, idp: str) -> AuthnResult: +def _authn(sp_authn: SP_AuthnRequest, idp: str) -> AuthnResult: _authn_id = AuthnRequestRef(str(uuid.uuid4())) # Filter out any previous authns with the same post_authn_action, both to keep the size of the session # below an upper bound, and because we currently need to use the post_authn_action value to find the @@ -227,7 +262,7 @@ def _authn(sp_authn: SP_AuthnRequest, force_authn: bool, same_user: bool, idp: s session.authn.sp.authns[_authn_id] = sp_authn subject = None - if same_user: + if sp_authn.params.same_user: name_id = NameID(format=NAMEID_FORMAT_UNSPECIFIED, text=session.common.eppn) subject = Subject(name_id=name_id) current_app.logger.debug(f"Requesting re-login by the same user with {subject}") @@ -238,7 +273,7 @@ def _authn(sp_authn: SP_AuthnRequest, force_authn: bool, same_user: bool, idp: s relay_state="", authn_id=_authn_id, selected_idp=idp, - force_authn=force_authn, + force_authn=sp_authn.params.force_authn, sign_alg=current_app.conf.authn_sign_alg, digest_alg=current_app.conf.authn_digest_alg, subject=subject, diff --git a/src/eduid/webapp/common/session/namespaces.py b/src/eduid/webapp/common/session/namespaces.py index 005f73ee6..3a1952ce9 100644 --- a/src/eduid/webapp/common/session/namespaces.py +++ b/src/eduid/webapp/common/session/namespaces.py @@ -5,7 +5,7 @@ from copy import deepcopy from datetime import datetime from enum import Enum, unique -from typing import Any, Mapping, NewType, Optional, TypeVar, Union, cast +from typing import Any, List, Mapping, NewType, Optional, TypeVar, Union, cast from uuid import uuid4 from fido2.webauthn import AuthenticatorAttachment @@ -242,6 +242,13 @@ def log_credential_used( self.pending_requests[request_ref].credentials_used[credential.key] = timestamp +class AuthnParameters(BaseModel): + force_authn: bool = False # a new authentication was required + force_mfa: bool = False # require MFA even if the user has no token (use Freja or other) + high_security: bool = False # opportunistic MFA, request it if the user has a token + same_user: bool = False # the same user was required to log in, such as when entering the security center + + class BaseAuthnRequest(BaseModel, ABC): frontend_state: Optional[str] = None # opaque data from frontend, returned in /status method: Optional[str] = None # proofing method that frontend is invoking @@ -258,9 +265,9 @@ class SP_AuthnRequest(BaseAuthnRequest): # proofing_credential_id is the credential being person-proofed, when doing that proofing_credential_id: Optional[ElementKey] = None redirect_url: Optional[str] # Deprecated, use frontend_action to get return URL from config instead - force_authn: bool = False # a new authentication was required - same_user: bool = False # the same user was required to log in, such as when entering the security center consumed: bool = False # an operation that requires a new authentication has used this one already + req_authn_ctx: List[str] = [] # the authentication contexts requested for this authentication + params: AuthnParameters = Field(default_factory=AuthnParameters) PySAML2Dicts = NewType("PySAML2Dicts", dict[str, dict[str, Any]]) diff --git a/src/eduid/webapp/security/tests/test_change_password.py b/src/eduid/webapp/security/tests/test_change_password.py index 19ba273fe..744a94169 100644 --- a/src/eduid/webapp/security/tests/test_change_password.py +++ b/src/eduid/webapp/security/tests/test_change_password.py @@ -11,7 +11,7 @@ from eduid.webapp.common.api.testing import EduidAPITestCase from eduid.webapp.common.api.utils import hash_password from eduid.webapp.common.authn.acs_enums import AuthnAcsAction -from eduid.webapp.common.session.namespaces import AuthnRequestRef, SP_AuthnRequest +from eduid.webapp.common.session.namespaces import AuthnParameters, AuthnRequestRef, SP_AuthnRequest from eduid.webapp.security.app import SecurityApp, security_init_app from eduid.webapp.security.helpers import SecurityMsg @@ -97,6 +97,7 @@ def _change_password( redirect_url="/test", authn_instant=utc_now() - timedelta(seconds=reauthn), frontend_action=FALLBACK_FRONTEND_ACTION, + params=AuthnParameters(force_authn=True, same_user=True, high_security=True), ) data = {"new_password": "0ieT/(.edW76", "old_password": "5678", "csrf_token": sess.get_csrf_token()} if data1 == {}: @@ -143,6 +144,7 @@ def _get_suggested_and_change( authn_instant=utc_now() - timedelta(seconds=reauthn), credentials_used=[ElementKey("112345678901234567890123")], frontend_action=FALLBACK_FRONTEND_ACTION, + params=AuthnParameters(force_authn=True, same_user=True, high_security=True), ) response2 = client.get("/change-password/suggested-password") passwd = json.loads(response2.data) diff --git a/src/eduid/webapp/security/views/change_password.py b/src/eduid/webapp/security/views/change_password.py index 4880e5103..dcfeae10b 100644 --- a/src/eduid/webapp/security/views/change_password.py +++ b/src/eduid/webapp/security/views/change_password.py @@ -107,6 +107,16 @@ def change_password_view( assert authn is not None # please mypy (if authn was None we would have returned with _need_reauthn above) + if ( + not authn.params.force_authn + or not authn.params.same_user + or not (authn.params.force_mfa or authn.params.high_security) + ): + # The current policy for changing the password is to require a re-authentication, + # and require at least opportunistic MFA to have been requested. + current_app.logger.info(f"Change password authentication policy not met: {authn.params}") + return error_response(message=SecurityMsg.no_reauthn) + if not new_password or (current_app.conf.chpass_old_password_needed and not old_password): return error_response(message=SecurityMsg.chpass_no_data) From 10c4800e4f94ba9a0c48784715b3544cd4133654 Mon Sep 17 00:00:00 2001 From: Johan Lundberg Date: Tue, 3 Oct 2023 10:55:03 +0200 Subject: [PATCH 04/30] make reformat --- src/eduid/common/config/parsers/__init__.py | 2 +- src/eduid/common/decorators.py | 3 --- src/eduid/common/fastapi/exceptions.py | 1 - src/eduid/common/logging.py | 2 +- src/eduid/queue/tests/test_client.py | 1 - src/eduid/scimapi/routers/users.py | 1 - src/eduid/scimapi/test-scripts/scim-util.py | 1 - src/eduid/scimapi/tests/test_sciminvite.py | 3 --- src/eduid/userdb/admin/__init__.py | 2 +- src/eduid/userdb/db.py | 2 -- src/eduid/userdb/deprecation.py | 3 --- src/eduid/userdb/ladok.py | 1 - src/eduid/userdb/proofing/state.py | 7 ------- src/eduid/userdb/support/db.py | 8 -------- src/eduid/userdb/support/models.py | 14 -------------- src/eduid/userdb/tests/test_logs.py | 2 -- src/eduid/userdb/tests/test_orcid.py | 1 - src/eduid/userdb/userdb.py | 1 - src/eduid/vccs/client/__init__.py | 2 +- src/eduid/vccs/server/run.py | 2 +- src/eduid/webapp/authn/helpers.py | 2 +- src/eduid/webapp/authn/tests/test_authn.py | 4 ---- src/eduid/webapp/authn/views.py | 6 +++--- src/eduid/webapp/common/api/debug.py | 1 - src/eduid/webapp/common/api/exceptions.py | 2 -- src/eduid/webapp/common/api/sanitation.py | 1 - src/eduid/webapp/common/api/schemas/base.py | 2 -- src/eduid/webapp/common/api/schemas/csrf.py | 2 -- src/eduid/webapp/common/api/testing.py | 1 - .../webapp/common/api/tests/test_backdoor.py | 6 ------ src/eduid/webapp/common/api/tests/test_inputs.py | 15 --------------- src/eduid/webapp/common/api/tests/test_logging.py | 1 - src/eduid/webapp/common/authn/cache.py | 1 - src/eduid/webapp/common/authn/tests/test_cache.py | 1 - .../webapp/common/authn/tests/test_fido_tokens.py | 1 - src/eduid/webapp/common/proofing/base.py | 1 - src/eduid/webapp/common/session/eduid_session.py | 2 +- src/eduid/webapp/common/session/namespaces.py | 2 -- src/eduid/webapp/common/session/redis_session.py | 1 - .../common/session/tests/test_eduid_session.py | 1 - .../common/session/tests/test_namespaces.py | 1 - src/eduid/webapp/eidas/helpers.py | 1 - src/eduid/webapp/eidas/tests/test_app.py | 1 - src/eduid/webapp/email/schemas.py | 5 ----- src/eduid/webapp/group_management/schemas.py | 4 ---- src/eduid/webapp/idp/login_context.py | 2 -- src/eduid/webapp/idp/tests/test_SSO.py | 2 -- src/eduid/webapp/idp/tests/test_known_device.py | 2 -- src/eduid/webapp/idp/views/mfa_auth.py | 1 - src/eduid/webapp/jsconfig/app.py | 1 - src/eduid/webapp/jsconfig/tests/test_app.py | 1 - src/eduid/webapp/ladok/tests/test_app.py | 1 - src/eduid/webapp/letter_proofing/schemas.py | 2 -- .../webapp/letter_proofing/tests/test_pdf.py | 3 --- .../webapp/lookup_mobile_proofing/schemas.py | 1 - .../lookup_mobile_proofing/tests/test_app.py | 3 --- src/eduid/webapp/oidc_proofing/schemas.py | 1 - src/eduid/webapp/personal_data/schemas.py | 2 -- src/eduid/webapp/phone/schemas.py | 5 ----- src/eduid/webapp/phone/settings/common.py | 5 +++-- src/eduid/webapp/phone/tests/test_app.py | 2 -- src/eduid/webapp/phone/views.py | 5 ++--- src/eduid/webapp/reset_password/schemas.py | 8 -------- src/eduid/webapp/security/schemas.py | 6 ------ src/eduid/webapp/security/tests/test_webauthn.py | 1 - src/eduid/webapp/signup/schemas.py | 3 --- src/eduid/webapp/signup/tests/test_app.py | 2 -- src/eduid/webapp/signup/views.py | 1 - src/eduid/webapp/svipe_id/proofing.py | 1 - src/eduid/workers/am/ams/__init__.py | 14 -------------- src/eduid/workers/am/ams/common.py | 1 - src/eduid/workers/am/testing.py | 1 - .../workers/am/tests/test_proofing_fetchers.py | 9 --------- src/eduid/workers/lookup_mobile/decorators.py | 1 - src/eduid/workers/msg/cache.py | 1 - 75 files changed, 15 insertions(+), 197 deletions(-) diff --git a/src/eduid/common/config/parsers/__init__.py b/src/eduid/common/config/parsers/__init__.py index d9746a5ce..c8e495c45 100644 --- a/src/eduid/common/config/parsers/__init__.py +++ b/src/eduid/common/config/parsers/__init__.py @@ -1,6 +1,6 @@ import json -import sys import os +import sys from pathlib import Path from typing import Any, Mapping, Optional diff --git a/src/eduid/common/decorators.py b/src/eduid/common/decorators.py index 28910bd0e..2c0e62e66 100644 --- a/src/eduid/common/decorators.py +++ b/src/eduid/common/decorators.py @@ -12,7 +12,6 @@ def deprecated(reason): """ if isinstance(reason, str): - # The @deprecated is used with a 'reason'. # # .. code-block:: python @@ -22,7 +21,6 @@ def deprecated(reason): # pass def decorator(func1): - if inspect.isclass(func1): fmt1 = "Call to deprecated class {name} ({reason})." else: @@ -42,7 +40,6 @@ def new_func1(*args, **kwargs): return decorator elif inspect.isclass(reason) or inspect.isfunction(reason): - # The @deprecated is used without any 'reason'. # # .. code-block:: python diff --git a/src/eduid/common/fastapi/exceptions.py b/src/eduid/common/fastapi/exceptions.py index 7cee660c0..ce8fe01b1 100644 --- a/src/eduid/common/fastapi/exceptions.py +++ b/src/eduid/common/fastapi/exceptions.py @@ -58,7 +58,6 @@ def __init__( status_code: int, detail: Optional[str] = None, ): - self._error_detail = ErrorDetail(detail=detail, status=status_code) self._extra_headers: Optional[dict] = None diff --git a/src/eduid/common/logging.py b/src/eduid/common/logging.py index 3fcfd4564..55aa0f568 100644 --- a/src/eduid/common/logging.py +++ b/src/eduid/common/logging.py @@ -72,7 +72,7 @@ def filter(self, record: logging.LogRecord) -> bool: ("eduid.common.", "e.c."), ("eduid.userdb.", "e.u."), ] - for (k, v) in shorten: + for k, v in shorten: if name.startswith(k): record.__setattr__("name", v + name[len(k) :]) break diff --git a/src/eduid/queue/tests/test_client.py b/src/eduid/queue/tests/test_client.py index a9b090d6f..147f00d45 100644 --- a/src/eduid/queue/tests/test_client.py +++ b/src/eduid/queue/tests/test_client.py @@ -47,7 +47,6 @@ def _create_queue_item(self, payload: Payload): ) def test_eduid_invite_mail(self): - payload = EduidInviteEmail( email="mail@example.com", reference="ref_id", diff --git a/src/eduid/scimapi/routers/users.py b/src/eduid/scimapi/routers/users.py index 6f9e2f8fe..1fb183db8 100644 --- a/src/eduid/scimapi/routers/users.py +++ b/src/eduid/scimapi/routers/users.py @@ -101,7 +101,6 @@ async def on_put(req: ContextRequest, resp: Response, update_request: UserUpdate nutid_changed = False if SCIMSchema.NUTID_USER_V1 in update_request.schemas and update_request.nutid_user_v1 is not None: - if not acceptable_linked_accounts(update_request.nutid_user_v1.linked_accounts): raise BadRequest(detail="Invalid nutid linked_accounts") diff --git a/src/eduid/scimapi/test-scripts/scim-util.py b/src/eduid/scimapi/test-scripts/scim-util.py index 99e675592..a46d73b87 100755 --- a/src/eduid/scimapi/test-scripts/scim-util.py +++ b/src/eduid/scimapi/test-scripts/scim-util.py @@ -220,7 +220,6 @@ def post_event( level: str = "info", data: Optional[dict[str, Any]] = None, ) -> None: - if resource_type == "User": resource = get_user_resource(api=api, scim_id=resource_scim_id) elif resource_type == "Group": diff --git a/src/eduid/scimapi/tests/test_sciminvite.py b/src/eduid/scimapi/tests/test_sciminvite.py index 7a9404f19..8f6186262 100644 --- a/src/eduid/scimapi/tests/test_sciminvite.py +++ b/src/eduid/scimapi/tests/test_sciminvite.py @@ -348,7 +348,6 @@ def _perform_search( # self.assertEqual(self.userdb.db_count(), len(resources)) def test_create_invite(self): - req = { "schemas": [ SCIMSchema.NUTID_INVITE_CORE_V1.value, @@ -400,7 +399,6 @@ def test_create_invite(self): assert event.data["status"] == EventStatus.CREATED.value def test_create_invite_missing_mandatory_attributes(self): - req = { "schemas": [ SCIMSchema.NUTID_INVITE_CORE_V1.value, @@ -444,7 +442,6 @@ def test_create_invite_missing_mandatory_attributes(self): ) def test_create_invite_do_not_send_email(self): - req = { "schemas": [ SCIMSchema.NUTID_INVITE_CORE_V1.value, diff --git a/src/eduid/userdb/admin/__init__.py b/src/eduid/userdb/admin/__init__.py index 5a59cdf0f..eae3c3f49 100644 --- a/src/eduid/userdb/admin/__init__.py +++ b/src/eduid/userdb/admin/__init__.py @@ -263,7 +263,7 @@ def pretty(self) -> list[str]: Format for simple pretty-printing as key: value pairs. """ res: list[str] = [] - for (key, value) in self.doc.items(): + for key, value in self.doc.items(): if isinstance(value, str): res.extend([" {!s:>25}: {!s}".format(key, value.encode("utf-8"))]) elif isinstance(value, datetime.datetime): diff --git a/src/eduid/userdb/db.py b/src/eduid/userdb/db.py index eabb67781..0247df143 100644 --- a/src/eduid/userdb/db.py +++ b/src/eduid/userdb/db.py @@ -260,7 +260,6 @@ def __init__( safe_writes: bool = False, driver: Optional[DatabaseDriver] = None, ): - self._db_uri = db_uri self._coll_name = collection self._db = MongoDB(db_uri, db_name=db_name, driver=driver) @@ -328,7 +327,6 @@ def _get_documents_by_attr(self, attr: str, value: str) -> list[TUserDbDocument] def _get_documents_by_aggregate( self, match: Mapping[str, Any], sort: Optional[Mapping[str, Any]] = None, limit: Optional[int] = None ) -> list[TUserDbDocument]: - pipeline: list[dict[str, Any]] = [{"$match": match}] if sort is not None: diff --git a/src/eduid/userdb/deprecation.py b/src/eduid/userdb/deprecation.py index 18ad26ecc..5a2200495 100644 --- a/src/eduid/userdb/deprecation.py +++ b/src/eduid/userdb/deprecation.py @@ -43,7 +43,6 @@ def deprecated(reason): """ if isinstance(reason, str): - # The @deprecated is used with a 'reason'. # # .. code-block:: python @@ -53,7 +52,6 @@ def deprecated(reason): # pass def decorator(func1): - if inspect.isclass(func1): fmt1 = "Call to deprecated class {name} ({reason})." else: @@ -73,7 +71,6 @@ def new_func1(*args, **kwargs): return decorator elif inspect.isclass(reason) or inspect.isfunction(reason): - # The @deprecated is used without any 'reason'. # # .. code-block:: python diff --git a/src/eduid/userdb/ladok.py b/src/eduid/userdb/ladok.py index f194c3bde..30d284c82 100644 --- a/src/eduid/userdb/ladok.py +++ b/src/eduid/userdb/ladok.py @@ -15,7 +15,6 @@ class UniversityName(BaseModel): class University(Element): - ladok_name: str name: UniversityName diff --git a/src/eduid/userdb/proofing/state.py b/src/eduid/userdb/proofing/state.py index 958017a96..75301b9e6 100644 --- a/src/eduid/userdb/proofing/state.py +++ b/src/eduid/userdb/proofing/state.py @@ -58,7 +58,6 @@ @dataclass() class ProofingState: - # __post_init__ will mint a new ObjectId if `id' is None id: Optional[bson.ObjectId] eppn: str @@ -142,7 +141,6 @@ def is_throttled(self, min_wait_seconds: int) -> bool: @dataclass() class NinProofingState(ProofingState): - nin: NinProofingElement @classmethod @@ -160,7 +158,6 @@ def to_dict(self) -> TUserDbDocument: @dataclass() class LetterProofingState(NinProofingState): - proofing_letter: SentLetterElement @classmethod @@ -179,7 +176,6 @@ def to_dict(self) -> TUserDbDocument: @dataclass() class OrcidProofingState(ProofingState): - state: str nonce: str @@ -190,7 +186,6 @@ def from_dict(cls, data: Mapping[str, Any]) -> OrcidProofingState: @dataclass() class OidcProofingState(NinProofingState): - state: str nonce: str token: str @@ -204,7 +199,6 @@ def from_dict(cls, data: Mapping[str, Any]) -> OidcProofingState: @dataclass() class EmailProofingState(ProofingState): - verification: EmailProofingElement @classmethod @@ -222,7 +216,6 @@ def to_dict(self) -> TUserDbDocument: @dataclass() class PhoneProofingState(ProofingState): - verification: PhoneProofingElement @classmethod diff --git a/src/eduid/userdb/support/db.py b/src/eduid/userdb/support/db.py index 738d629b7..590484207 100644 --- a/src/eduid/userdb/support/db.py +++ b/src/eduid/userdb/support/db.py @@ -47,12 +47,10 @@ def search_users(self, query: str) -> list[SupportUser]: class SupportSignupUserDB(SignupUserDB): - pass class SupportAuthnInfoDB(BaseDB): - model = models.UserAuthnInfo def __init__(self, db_uri: str): @@ -86,7 +84,6 @@ def get_credential_info(self, credential_id: str) -> dict[str, Any]: class SupportProofingDB(BaseDB): - model: type[GenericFilterDict] = GenericFilterDict def __init__(self, db_uri: str, db_name: str, collection: str): @@ -112,7 +109,6 @@ def get_proofing_states(self, eppn: str) -> list[dict[str, Any]]: class SupportLetterProofingDB(SupportProofingDB): - model = models.UserLetterProofing def __init__(self, db_uri: str): @@ -133,7 +129,6 @@ def get_proofing_state(self, eppn: str) -> dict[str, Any]: class SupportOidcProofingDB(SupportProofingDB): - model = models.UserOidcProofing def __init__(self, db_uri: str): @@ -143,7 +138,6 @@ def __init__(self, db_uri: str): class SupportEmailProofingDB(SupportProofingDB): - model = models.UserEmailProofing def __init__(self, db_uri: str): @@ -153,7 +147,6 @@ def __init__(self, db_uri: str): class SupportPhoneProofingDB(SupportProofingDB): - model = models.UserPhoneProofing def __init__(self, db_uri: str): @@ -163,7 +156,6 @@ def __init__(self, db_uri: str): class SupportProofingLogDB(BaseDB): - model = models.ProofingLogEntry def __init__(self, db_uri: str): diff --git a/src/eduid/userdb/support/models.py b/src/eduid/userdb/support/models.py index c7bf5688f..bf808b795 100644 --- a/src/eduid/userdb/support/models.py +++ b/src/eduid/userdb/support/models.py @@ -6,7 +6,6 @@ # Models for filtering out unneeded or unwanted data from eduID database objects class GenericFilterDict(dict): - add_keys: Optional[list[str]] = None remove_keys: Optional[list[str]] = None @@ -37,7 +36,6 @@ def __init__(self, data): class SupportUserFilter(GenericFilterDict): - remove_keys = ["_id", "letter_proofing_data"] def __init__(self, data): @@ -50,7 +48,6 @@ def __init__(self, data): class SupportSignupUserFilter(GenericFilterDict): - remove_keys = ["_id", "letter_proofing_data"] def __init__(self, data): @@ -64,7 +61,6 @@ def __init__(self, data): class MailAlias(GenericFilterDict): - remove_keys = ["verification_code"] @@ -73,7 +69,6 @@ class PendingMailAddress(MailAlias): class Credential(GenericFilterDict): - add_keys = ["_id", "created_by", "created_ts", "type", "success_ts"] def __init__(self, data): @@ -87,12 +82,10 @@ def __init__(self, data): class ToU(GenericFilterDict): - remove_keys = ["id"] class UserAuthnInfo(GenericFilterDict): - add_keys = ["success_ts", "fail_count", "success_count"] def __init__(self, data): @@ -106,17 +99,14 @@ def __init__(self, data): class UserVerifications(GenericFilterDict): - add_keys = ["verified", "obj_id", "timestamp", "model_name", "verified_timestamp"] class UserActions(GenericFilterDict): - add_keys = ["action", "params"] class ProofingLogEntry(GenericFilterDict): - add_keys = ["verified_data", "created_ts", "proofing_method", "proofing_version", "created_by", "vetting_by"] def __init__(self, data): @@ -130,7 +120,6 @@ def __init__(self, data): class UserLetterProofing(GenericFilterDict): - add_keys = ["nin", "proofing_letter"] class Nin(GenericFilterDict): @@ -147,7 +136,6 @@ def __init__(self, data): class UserOidcProofing(GenericFilterDict): - add_keys = ["nin", "modified_ts", "state"] class Nin(GenericFilterDict): @@ -160,7 +148,6 @@ def __init__(self, data): class UserEmailProofing(GenericFilterDict): - add_keys = ["verification", "modified_ts"] class Verification(GenericFilterDict): @@ -173,7 +160,6 @@ def __init__(self, data): class UserPhoneProofing(GenericFilterDict): - add_keys = ["verification", "modified_ts"] class Verification(GenericFilterDict): diff --git a/src/eduid/userdb/tests/test_logs.py b/src/eduid/userdb/tests/test_logs.py index 80b6fcde8..57cb38875 100644 --- a/src/eduid/userdb/tests/test_logs.py +++ b/src/eduid/userdb/tests/test_logs.py @@ -38,7 +38,6 @@ def tearDown(self): self.proofing_log_db._drop_whole_collection() def test_id_proofing_data(self): - proofing_element = ProofingLogElement( eppn=self.user.eppn, created_by="test", proofing_method="test", proofing_version="test" ) @@ -323,7 +322,6 @@ def test_boolean_false_proofing_data(self): self.assertTrue(self.proofing_log_db.save(proofing_element)) def test_deregistered_proofing_data(self): - proofing_element = NinProofingLogElement( eppn=self.user.eppn, created_by="test", diff --git a/src/eduid/userdb/tests/test_orcid.py b/src/eduid/userdb/tests/test_orcid.py index 1d1f1d406..767d07e68 100644 --- a/src/eduid/userdb/tests/test_orcid.py +++ b/src/eduid/userdb/tests/test_orcid.py @@ -34,7 +34,6 @@ class TestOrcid(unittest.TestCase): - maxDiff = None def test_id_token(self): diff --git a/src/eduid/userdb/userdb.py b/src/eduid/userdb/userdb.py index 87bcc4767..37a8fd006 100644 --- a/src/eduid/userdb/userdb.py +++ b/src/eduid/userdb/userdb.py @@ -74,7 +74,6 @@ class UserDB(BaseDB, Generic[UserVar], ABC): """ def __init__(self, db_uri: str, db_name: str, collection: str = "userdb"): - if db_name == "eduid_am" and collection == "userdb": # Hack to get right collection name while the configuration points to the old database collection = "attributes" diff --git a/src/eduid/vccs/client/__init__.py b/src/eduid/vccs/client/__init__.py index 605b4ad88..4243e785f 100644 --- a/src/eduid/vccs/client/__init__.py +++ b/src/eduid/vccs/client/__init__.py @@ -290,7 +290,7 @@ def to_dict(self, action: str) -> dict[str, Any]: raise NotImplementedError() else: raise ValueError("Unknown 'action' value (not auth or add_creds)") - for (k, v) in res.items(): + for k, v in res.items(): if v is None: raise ValueError(f"{action!r} property {k!r} not provided") return res diff --git a/src/eduid/vccs/server/run.py b/src/eduid/vccs/server/run.py index 856de8558..645436a1b 100644 --- a/src/eduid/vccs/server/run.py +++ b/src/eduid/vccs/server/run.py @@ -1,5 +1,5 @@ -from asyncio import Lock import sys +from asyncio import Lock from binascii import unhexlify from typing import Any, Mapping, Optional diff --git a/src/eduid/webapp/authn/helpers.py b/src/eduid/webapp/authn/helpers.py index 452cedea5..a75016956 100644 --- a/src/eduid/webapp/authn/helpers.py +++ b/src/eduid/webapp/authn/helpers.py @@ -1,5 +1,5 @@ -from enum import unique import logging +from enum import unique from typing import Optional from eduid.common.misc.timeutil import utc_now diff --git a/src/eduid/webapp/authn/tests/test_authn.py b/src/eduid/webapp/authn/tests/test_authn.py index f48082141..06e306a96 100644 --- a/src/eduid/webapp/authn/tests/test_authn.py +++ b/src/eduid/webapp/authn/tests/test_authn.py @@ -490,7 +490,6 @@ def test_logout_loggedin(self): ) def test_logout_service_startingSP(self): - came_from = "/afterlogin/" session_id = self.add_outstanding_query(came_from) cookie = self.dump_session_cookie(session_id) @@ -510,7 +509,6 @@ def test_logout_service_startingSP(self): self.assertIn("testing-relay-state", response.location) def test_logout_service_startingSP_already_logout(self): - came_from = "/afterlogin/" session_id = self.add_outstanding_query(came_from) @@ -528,7 +526,6 @@ def test_logout_service_startingSP_already_logout(self): self.assertIn("testing-relay-state", response.location) def test_logout_service_startingIDP(self): - res = self.acs("/login", eppn=self.test_user.eppn, next_url="/afterlogin/") cookie = self.dump_session_cookie(res.session.meta.cookie_val) @@ -550,7 +547,6 @@ def test_logout_service_startingIDP(self): ) def test_logout_service_startingIDP_no_subject_id(self): - eppn = "hubba-bubba" came_from = "/afterlogin/" session_id = self.add_outstanding_query(came_from) diff --git a/src/eduid/webapp/authn/views.py b/src/eduid/webapp/authn/views.py index 548965cc6..4013f8c8e 100644 --- a/src/eduid/webapp/authn/views.py +++ b/src/eduid/webapp/authn/views.py @@ -30,8 +30,8 @@ # ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE # POSSIBILITY OF SUCH DAMAGE. # -from dataclasses import dataclass import uuid +from dataclasses import dataclass from typing import Optional from flask import Blueprint, abort, make_response, redirect, request @@ -39,12 +39,12 @@ from saml2.client import Saml2Client from saml2.ident import decode from saml2.metadata import entity_descriptor -from saml2.saml import NAMEID_FORMAT_UNSPECIFIED, NameID, Subject from saml2.request import AuthnRequest +from saml2.saml import NAMEID_FORMAT_UNSPECIFIED, NameID, Subject from werkzeug.exceptions import Forbidden from werkzeug.wrappers import Response as WerkzeugResponse -from eduid.userdb.credentials.fido import FidoCredential +from eduid.userdb.credentials.fido import FidoCredential from eduid.userdb.exceptions import MultipleUsersReturned, UserDoesNotExist from eduid.webapp.authn import acs_actions # acs_action needs to be imported to be loaded from eduid.webapp.authn.app import current_authn_app as current_app diff --git a/src/eduid/webapp/common/api/debug.py b/src/eduid/webapp/common/api/debug.py index 6387d1b98..ed02ae170 100644 --- a/src/eduid/webapp/common/api/debug.py +++ b/src/eduid/webapp/common/api/debug.py @@ -29,7 +29,6 @@ def log_endpoints(app: Flask): output: list[str] = [] with app.app_context(): for rule in app.url_map.iter_rules(): - options = {} for arg in rule.arguments: options[arg] = f"[{arg}]" diff --git a/src/eduid/webapp/common/api/exceptions.py b/src/eduid/webapp/common/api/exceptions.py index 8d0704ace..d4c9a38b3 100644 --- a/src/eduid/webapp/common/api/exceptions.py +++ b/src/eduid/webapp/common/api/exceptions.py @@ -66,7 +66,6 @@ class ProofingLogFailure(Exception): class ThrottledException(Exception): - state: "ResetPasswordEmailState" def __init__(self, state: "ResetPasswordEmailState"): @@ -75,7 +74,6 @@ def __init__(self, state: "ResetPasswordEmailState"): def init_exception_handlers(app): - # Init error handler for raised exceptions @app.errorhandler(400) def _handle_flask_http_exception(error): diff --git a/src/eduid/webapp/common/api/sanitation.py b/src/eduid/webapp/common/api/sanitation.py index 578bb0b0e..9608befcd 100644 --- a/src/eduid/webapp/common/api/sanitation.py +++ b/src/eduid/webapp/common/api/sanitation.py @@ -127,7 +127,6 @@ def _sanitize_input( content_type = request.mimetype if isinstance(content_type, str) and content_type: - if content_type == "application/x-www-form-urlencoded": use_percent_encoding = True else: diff --git a/src/eduid/webapp/common/api/schemas/base.py b/src/eduid/webapp/common/api/schemas/base.py index f737e344b..a45722922 100644 --- a/src/eduid/webapp/common/api/schemas/base.py +++ b/src/eduid/webapp/common/api/schemas/base.py @@ -4,7 +4,6 @@ class EduidSchema(Schema): - message = fields.String(required=False) class Meta: @@ -12,7 +11,6 @@ class Meta: class FluxStandardAction(EduidSchema): - type = fields.String(required=True) payload = fields.Field(required=False) error = fields.Boolean(required=False) diff --git a/src/eduid/webapp/common/api/schemas/csrf.py b/src/eduid/webapp/common/api/schemas/csrf.py index c87e6c50e..59452a5e0 100644 --- a/src/eduid/webapp/common/api/schemas/csrf.py +++ b/src/eduid/webapp/common/api/schemas/csrf.py @@ -12,7 +12,6 @@ class CSRFRequestMixin(Schema): - csrf_token = fields.String(required=True) @validates("csrf_token") @@ -38,7 +37,6 @@ def remove_csrf_token(in_data, **kwargs): class CSRFResponseMixin(Schema): - csrf_token = fields.String(required=True) @pre_dump diff --git a/src/eduid/webapp/common/api/testing.py b/src/eduid/webapp/common/api/testing.py index 665ae84ce..5079c5953 100644 --- a/src/eduid/webapp/common/api/testing.py +++ b/src/eduid/webapp/common/api/testing.py @@ -460,7 +460,6 @@ def _check_nin_not_verified(self, user: User, number: Optional[str] = None, crea class CSRFTestClient(FlaskClient): - # Add the custom csrf headers to every call to post def post(self, *args: Any, **kwargs: Any) -> TestResponse: """ diff --git a/src/eduid/webapp/common/api/tests/test_backdoor.py b/src/eduid/webapp/common/api/tests/test_backdoor.py index 3de8ba1f1..3f1683cd3 100644 --- a/src/eduid/webapp/common/api/tests/test_backdoor.py +++ b/src/eduid/webapp/common/api/tests/test_backdoor.py @@ -101,7 +101,6 @@ def load_app(self, config: Mapping[str, Any]) -> BackdoorTestApp: def test_backdoor_get_code(self): """""" with self.session_cookie_anon(self.browser) as client: - assert self.app.conf.magic_cookie_name is not None assert self.app.conf.magic_cookie is not None client.set_cookie("localhost", key=self.app.conf.magic_cookie_name, value=self.app.conf.magic_cookie) @@ -113,7 +112,6 @@ def test_no_backdoor_in_pro(self): self.app.conf.environment = EduidEnvironment("production") with self.session_cookie_anon(self.browser) as client: - assert self.app.conf.magic_cookie_name is not None assert self.app.conf.magic_cookie is not None client.set_cookie("localhost", key=self.app.conf.magic_cookie_name, value=self.app.conf.magic_cookie) @@ -123,14 +121,12 @@ def test_no_backdoor_in_pro(self): def test_no_backdoor_without_cookie(self): """""" with self.session_cookie_anon(self.browser) as client: - response = client.get("/get-code?eppn=pepin-pepon") self.assertEqual(response.status_code, 400) def test_wrong_cookie_no_backdoor(self): """""" with self.session_cookie_anon(self.browser) as client: - assert self.app.conf.magic_cookie_name is not None assert self.app.conf.magic_cookie is not None client.set_cookie("localhost", key=self.app.conf.magic_cookie_name, value="no-magic") @@ -142,7 +138,6 @@ def test_no_magic_cookie_no_backdoor(self): self.app.conf.magic_cookie = "" with self.session_cookie_anon(self.browser) as client: - assert self.app.conf.magic_cookie_name is not None assert self.app.conf.magic_cookie is not None client.set_cookie("localhost", key=self.app.conf.magic_cookie_name, value=self.app.conf.magic_cookie) @@ -154,7 +149,6 @@ def test_no_magic_cookie_name_no_backdoor(self): self.app.conf.magic_cookie_name = "" with self.session_cookie_anon(self.browser) as client: - assert self.app.conf.magic_cookie_name is not None assert self.app.conf.magic_cookie is not None client.set_cookie("localhost", key=self.app.conf.magic_cookie_name, value=self.app.conf.magic_cookie) diff --git a/src/eduid/webapp/common/api/tests/test_inputs.py b/src/eduid/webapp/common/api/tests/test_inputs.py index b98da95fc..d9d93392b 100644 --- a/src/eduid/webapp/common/api/tests/test_inputs.py +++ b/src/eduid/webapp/common/api/tests/test_inputs.py @@ -147,7 +147,6 @@ def test_get_param(self): """""" url = "/test-get-param?test-param=test-param" with self.app.test_request_context(url, method="GET"): - response = self.app.dispatch_request() self.assertIn(b"test-param", response.data) @@ -155,21 +154,18 @@ def test_get_param_script(self): """""" url = '/test-get-param?test-param=' with self.app.test_request_context(url, method="GET"): - response = self.app.dispatch_request() self.assertNotIn(b"'}): - response = self.app.dispatch_request() self.assertNotIn(b"", "csrf_token": "failing-token"}', ): - response = self.app.dispatch_request() self.assertNotIn(b"') with self.app.test_request_context(url, method="GET", headers={"Cookie": cookie}): - response = self.app.dispatch_request() self.assertNotIn(b"' with self.app.test_request_context(url, method="GET", headers={"X-TEST": script}): - response = self.app.dispatch_request() self.assertNotIn(b"'}): - response = self.app.dispatch_request() self.assertNotIn(b"