From 5bfd6c867fc489d85370492cb660ed4516f36fbf Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 15 May 2024 15:25:00 -0600 Subject: [PATCH 01/18] Add initial setting config --- src/djangooidc/views.py | 2 ++ src/registrar/config/settings.py | 6 ++++++ 2 files changed, 8 insertions(+) diff --git a/src/djangooidc/views.py b/src/djangooidc/views.py index 815df4ecf..a50a83cc9 100644 --- a/src/djangooidc/views.py +++ b/src/djangooidc/views.py @@ -95,6 +95,8 @@ def login_callback(request): # if not satisfied, redirect user to login with stepped up acr_value if _requires_step_up_auth(userinfo): # add acr_value to request.session + + # LOOK HERE this is basically the flag that indicates that we should proceed request.session["acr_value"] = CLIENT.get_step_up_acr_value() return CLIENT.create_authn_request(request.session) user = authenticate(request=request, **userinfo) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index 9f31ffc2c..7bda5e10d 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -563,6 +563,8 @@ OIDC_PROVIDERS = { "user_info_request": ["email", "first_name", "last_name", "phone"], "acr_value": "http://idmanagement.gov/ns/assurance/ial/1", "step_up_acr_value": "http://idmanagement.gov/ns/assurance/ial/2", + "vtr": ["Pb","P1"], + "vtm": "https://developer.login.gov/vot-trust-framework", }, "client_registration": { "client_id": "cisa_dotgov_registrar", @@ -581,6 +583,10 @@ OIDC_PROVIDERS = { "user_info_request": ["email", "first_name", "last_name", "phone"], "acr_value": "http://idmanagement.gov/ns/assurance/ial/1", "step_up_acr_value": "http://idmanagement.gov/ns/assurance/ial/2", + # "P1" is the current IdV option; "Pb" stands for 'biometric' + "vtr": ["Pb","P1"], + # Stand in replacement for the step_up_acr_value for ial2 + "vtm": "https://developer.login.gov/vot-trust-framework", }, "client_registration": { "client_id": ("urn:gov:cisa:openidconnect.profiles:sp:sso:cisa:dotgov_registrar"), From 28e18e705bdadef4bfdb0195c2016718d61ad04d Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 16 May 2024 12:14:03 -0600 Subject: [PATCH 02/18] Add biometric --- src/djangooidc/oidc.py | 16 +++++++++++++--- src/djangooidc/views.py | 10 +++++++--- src/registrar/config/settings.py | 4 ++-- 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/src/djangooidc/oidc.py b/src/djangooidc/oidc.py index 95ed322f5..1133a8b39 100644 --- a/src/djangooidc/oidc.py +++ b/src/djangooidc/oidc.py @@ -14,6 +14,7 @@ from oic.oic import AuthorizationRequest, AuthorizationResponse, RegistrationRes from oic.oic.message import AccessTokenResponse from oic.utils.authn.client import CLIENT_AUTHN_METHOD from oic.utils import keyio +from urllib.parse import urlparse, urlunparse, urlencode, parse_qs from . import exceptions as o_e @@ -84,6 +85,7 @@ class Client(oic.Client): def create_authn_request( self, session, + add_acr=True, extra_args=None, ): """Step 2: Construct a login URL at OP's domain and send the user to it.""" @@ -100,10 +102,10 @@ class Client(oic.Client): "state": session["state"], "nonce": session["nonce"], "redirect_uri": self.registration_response["redirect_uris"][0], - # acr_value may be passed in session if overriding, as in the case - # of step up auth, otherwise get from settings.py - "acr_values": session.get("acr_value") or self.behaviour.get("acr_value"), } + if add_acr: + request_args["acr_values"] = session.get("acr_value") or self.behaviour.get("acr_value") + request_args["vtr"] = json.dumps(self.behaviour.get("vtr")) if extra_args is not None: request_args.update(extra_args) @@ -126,6 +128,7 @@ class Client(oic.Client): method="GET", request_args=request_args, ) + logger.debug("body: %s" % body) logger.debug("URL: %s" % url) logger.debug("headers: %s" % headers) @@ -141,6 +144,7 @@ class Client(oic.Client): if headers: for key, value in headers.items(): response[key] = value + print(f"create auth => response is {response}") except Exception as err: logger.error(err) logger.error("Failed to create redirect object for %s" % state) @@ -294,6 +298,12 @@ class Client(oic.Client): this helper function is called from djangooidc views""" return self.behaviour.get("step_up_acr_value") + def get_vtm_value(self): + return self.behaviour.get("vtm") + + def get_vtr_value(self): + return self.behaviour.get("vtr") + def __repr__(self): return "Client {} {} {}".format( self.client_id, diff --git a/src/djangooidc/views.py b/src/djangooidc/views.py index a50a83cc9..05984e938 100644 --- a/src/djangooidc/views.py +++ b/src/djangooidc/views.py @@ -96,9 +96,13 @@ def login_callback(request): if _requires_step_up_auth(userinfo): # add acr_value to request.session - # LOOK HERE this is basically the flag that indicates that we should proceed - request.session["acr_value"] = CLIENT.get_step_up_acr_value() - return CLIENT.create_authn_request(request.session) + if "acr_value" in request.session: + request.session.pop("acr_value") + extra_args = { + "vtm": CLIENT.get_vtm_value(), + } + print(f"session is: {request.session}") + return CLIENT.create_authn_request(request.session, add_acr=False, extra_args=extra_args) user = authenticate(request=request, **userinfo) if user: diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index 7bda5e10d..781b67d5a 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -497,13 +497,13 @@ LOGGING = { # OpenID Connect logger "oic": { "handlers": ["console"], - "level": "INFO", + "level": "DEBUG", "propagate": False, }, # Django wrapper for OpenID Connect "djangooidc": { "handlers": ["console"], - "level": "INFO", + "level": "DEBUG", "propagate": False, }, # Our app! From f3f1b896bba3679c2957cc4a2cc81b9f723411c5 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 16 May 2024 12:37:14 -0600 Subject: [PATCH 03/18] Fix infinite loop --- src/djangooidc/oidc.py | 15 +++++++++++---- src/djangooidc/views.py | 5 +++-- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/src/djangooidc/oidc.py b/src/djangooidc/oidc.py index 1133a8b39..404bc96c2 100644 --- a/src/djangooidc/oidc.py +++ b/src/djangooidc/oidc.py @@ -104,8 +104,9 @@ class Client(oic.Client): "redirect_uri": self.registration_response["redirect_uris"][0], } if add_acr: - request_args["acr_values"] = session.get("acr_value") or self.behaviour.get("acr_value") - request_args["vtr"] = json.dumps(self.behaviour.get("vtr")) + request_args["acr_values"] = self.behaviour.get("acr_value") + else: + request_args["vtr"] = json.dumps(self.behaviour.get("vtr")) if extra_args is not None: request_args.update(extra_args) @@ -228,9 +229,15 @@ class Client(oic.Client): if isinstance(info_response, ErrorResponse): logger.error("Unable to get user info (%s) for %s" % (info_response.get("error", ""), state)) raise o_e.AuthenticationFailed(locator=state) + info_response_dict = info_response.to_dict() - logger.debug("user info: %s" % info_response) - return info_response.to_dict() + if "needs_biometric_validation" in session and session["needs_biometric_validation"]: + if "vtm" in session: + info_response_dict["vtm"] = session.get("vtm") + if "vtr" in session: + info_response_dict["vtr"] = session.get("vtr") + logger.debug("user info: %s" % info_response_dict) + return info_response_dict def _request_token(self, state, code, session): """Request a token from OP to allow us to then request user info.""" diff --git a/src/djangooidc/views.py b/src/djangooidc/views.py index 05984e938..f817cc4d0 100644 --- a/src/djangooidc/views.py +++ b/src/djangooidc/views.py @@ -93,14 +93,15 @@ def login_callback(request): userinfo = CLIENT.callback(query, request.session) # test for need for identity verification and if it is satisfied # if not satisfied, redirect user to login with stepped up acr_value + request.session["needs_biometric_validation"] = False if _requires_step_up_auth(userinfo): # add acr_value to request.session - if "acr_value" in request.session: request.session.pop("acr_value") extra_args = { "vtm": CLIENT.get_vtm_value(), } + request.session["needs_biometric_validation"] = True print(f"session is: {request.session}") return CLIENT.create_authn_request(request.session, add_acr=False, extra_args=extra_args) user = authenticate(request=request, **userinfo) @@ -151,7 +152,7 @@ def _requires_step_up_auth(userinfo): acr_value = userinfo.get("ial", "") uuid = userinfo.get("sub", "") email = userinfo.get("email", "") - if acr_value != step_up_acr_value: + if acr_value != step_up_acr_value and (not userinfo.get("vtm") and not userinfo.get("vtr")): # The acr of this attempt is not at the highest level # so check if the user needs the higher level return User.needs_identity_verification(email, uuid) From bd7660de27fe5a628f5dc205108b417de629cc79 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 16 May 2024 13:18:45 -0600 Subject: [PATCH 04/18] Remove step up acr check --- src/djangooidc/oidc.py | 24 +++++++++++++----------- src/djangooidc/views.py | 22 +++++++--------------- src/registrar/config/settings.py | 2 -- 3 files changed, 20 insertions(+), 28 deletions(-) diff --git a/src/djangooidc/oidc.py b/src/djangooidc/oidc.py index 404bc96c2..0f52c0688 100644 --- a/src/djangooidc/oidc.py +++ b/src/djangooidc/oidc.py @@ -85,7 +85,7 @@ class Client(oic.Client): def create_authn_request( self, session, - add_acr=True, + do_biometric_auth=False, extra_args=None, ): """Step 2: Construct a login URL at OP's domain and send the user to it.""" @@ -103,10 +103,10 @@ class Client(oic.Client): "nonce": session["nonce"], "redirect_uri": self.registration_response["redirect_uris"][0], } - if add_acr: - request_args["acr_values"] = self.behaviour.get("acr_value") + if do_biometric_auth: + self._set_args_for_biometric_auth_request(session, request_args) else: - request_args["vtr"] = json.dumps(self.behaviour.get("vtr")) + request_args["acr_values"] = self.behaviour.get("acr_value") if extra_args is not None: request_args.update(extra_args) @@ -153,6 +153,12 @@ class Client(oic.Client): return response + def _set_args_for_biometric_auth_request(self, session, request_args): + if "acr_value" in session: + session.pop("acr_value") + request_args["vtr"] = self.get_vtr_value() + request_args["vtm"] = self.get_vtm_value() + def callback(self, unparsed_response, session): """Step 3: Receive OP's response, request an access token, and user info.""" logger.debug("Processing the OpenID Connect callback response...") @@ -300,16 +306,12 @@ class Client(oic.Client): this helper function is called from djangooidc views""" return self.behaviour.get("acr_value") - def get_step_up_acr_value(self): - """returns the step_up_acr_value from settings - this helper function is called from djangooidc views""" - return self.behaviour.get("step_up_acr_value") - def get_vtm_value(self): return self.behaviour.get("vtm") - def get_vtr_value(self): - return self.behaviour.get("vtr") + def get_vtr_value(self, cleaned=True): + vtr = self.behaviour.get("vtr") + return json.dumps(vtr) if cleaned else vtr def __repr__(self): return "Client {} {} {}".format( diff --git a/src/djangooidc/views.py b/src/djangooidc/views.py index f817cc4d0..e9bb5ed09 100644 --- a/src/djangooidc/views.py +++ b/src/djangooidc/views.py @@ -93,17 +93,11 @@ def login_callback(request): userinfo = CLIENT.callback(query, request.session) # test for need for identity verification and if it is satisfied # if not satisfied, redirect user to login with stepped up acr_value - request.session["needs_biometric_validation"] = False - if _requires_step_up_auth(userinfo): - # add acr_value to request.session - if "acr_value" in request.session: - request.session.pop("acr_value") - extra_args = { - "vtm": CLIENT.get_vtm_value(), - } - request.session["needs_biometric_validation"] = True - print(f"session is: {request.session}") - return CLIENT.create_authn_request(request.session, add_acr=False, extra_args=extra_args) + needs_biometric_validation = _requires_biometric_auth(userinfo) + request.session["needs_biometric_validation"] = needs_biometric_validation + if needs_biometric_validation: + return CLIENT.create_authn_request(request.session, do_biometric_auth=True) + user = authenticate(request=request, **userinfo) if user: @@ -145,14 +139,12 @@ def login_callback(request): return error_page(request, err) -def _requires_step_up_auth(userinfo): +def _requires_biometric_auth(userinfo): """if User.needs_identity_verification and step_up_acr_value not in ial returned from callback, return True""" - step_up_acr_value = CLIENT.get_step_up_acr_value() - acr_value = userinfo.get("ial", "") uuid = userinfo.get("sub", "") email = userinfo.get("email", "") - if acr_value != step_up_acr_value and (not userinfo.get("vtm") and not userinfo.get("vtr")): + if not userinfo.get("vtm") or not userinfo.get("vtr"): # The acr of this attempt is not at the highest level # so check if the user needs the higher level return User.needs_identity_verification(email, uuid) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index 781b67d5a..272eb9704 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -562,7 +562,6 @@ OIDC_PROVIDERS = { "scope": ["email", "profile:name", "phone"], "user_info_request": ["email", "first_name", "last_name", "phone"], "acr_value": "http://idmanagement.gov/ns/assurance/ial/1", - "step_up_acr_value": "http://idmanagement.gov/ns/assurance/ial/2", "vtr": ["Pb","P1"], "vtm": "https://developer.login.gov/vot-trust-framework", }, @@ -582,7 +581,6 @@ OIDC_PROVIDERS = { "scope": ["email", "profile:name", "phone"], "user_info_request": ["email", "first_name", "last_name", "phone"], "acr_value": "http://idmanagement.gov/ns/assurance/ial/1", - "step_up_acr_value": "http://idmanagement.gov/ns/assurance/ial/2", # "P1" is the current IdV option; "Pb" stands for 'biometric' "vtr": ["Pb","P1"], # Stand in replacement for the step_up_acr_value for ial2 From 3c0facfc9dfc72c6e66ffba167dedf23584bdcb1 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 16 May 2024 13:35:35 -0600 Subject: [PATCH 05/18] Add comments, do cleanup --- src/djangooidc/oidc.py | 23 ++++++++++++++--------- src/djangooidc/views.py | 8 +++++++- src/registrar/config/settings.py | 10 +++++++--- 3 files changed, 28 insertions(+), 13 deletions(-) diff --git a/src/djangooidc/oidc.py b/src/djangooidc/oidc.py index 0f52c0688..f95146b11 100644 --- a/src/djangooidc/oidc.py +++ b/src/djangooidc/oidc.py @@ -14,7 +14,7 @@ from oic.oic import AuthorizationRequest, AuthorizationResponse, RegistrationRes from oic.oic.message import AccessTokenResponse from oic.utils.authn.client import CLIENT_AUTHN_METHOD from oic.utils import keyio -from urllib.parse import urlparse, urlunparse, urlencode, parse_qs + from . import exceptions as o_e @@ -145,7 +145,7 @@ class Client(oic.Client): if headers: for key, value in headers.items(): response[key] = value - print(f"create auth => response is {response}") + except Exception as err: logger.error(err) logger.error("Failed to create redirect object for %s" % state) @@ -237,11 +237,12 @@ class Client(oic.Client): raise o_e.AuthenticationFailed(locator=state) info_response_dict = info_response.to_dict() - if "needs_biometric_validation" in session and session["needs_biometric_validation"]: - if "vtm" in session: - info_response_dict["vtm"] = session.get("vtm") - if "vtr" in session: - info_response_dict["vtr"] = session.get("vtr") + # Define vtm/vtr information on the user dictionary so we can track this in one location. + # If a user has this information, then they are bumped up in terms of verification level. + if session.get("needs_biometric_validation") is True: + info_response_dict["vtm"] = session.get("vtm", "") + info_response_dict["vtr"] = session.get("vtr", "") + logger.debug("user info: %s" % info_response_dict) return info_response_dict @@ -302,14 +303,18 @@ class Client(oic.Client): super(Client, self).store_response(resp, info) def get_default_acr_value(self): - """returns the acr_value from settings - this helper function is called from djangooidc views""" + """Returns the acr_value from settings. + This helper function is called from djangooidc views.""" return self.behaviour.get("acr_value") def get_vtm_value(self): + """Returns the vtm value from settings. + This helper function is called from djangooidc views.""" return self.behaviour.get("vtm") def get_vtr_value(self, cleaned=True): + """Returns the vtr value from settings. + This helper function is called from djangooidc views.""" vtr = self.behaviour.get("vtr") return json.dumps(vtr) if cleaned else vtr diff --git a/src/djangooidc/views.py b/src/djangooidc/views.py index e9bb5ed09..69bd89afe 100644 --- a/src/djangooidc/views.py +++ b/src/djangooidc/views.py @@ -91,10 +91,16 @@ def login_callback(request): _initialize_client() query = parse_qs(request.GET.urlencode()) userinfo = CLIENT.callback(query, request.session) + # test for need for identity verification and if it is satisfied - # if not satisfied, redirect user to login with stepped up acr_value + # if not satisfied, redirect user to login requiring biometric auth + + # Tests for the presence of the vtm/vtr values in the userinfo object. + # If they are there, then we can set a flag in our session for tracking purposes. needs_biometric_validation = _requires_biometric_auth(userinfo) request.session["needs_biometric_validation"] = needs_biometric_validation + + # Return a redirect request to a new auth url that enables biometric validation if needs_biometric_validation: return CLIENT.create_authn_request(request.session, do_biometric_auth=True) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index 272eb9704..12b0aad78 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -497,13 +497,13 @@ LOGGING = { # OpenID Connect logger "oic": { "handlers": ["console"], - "level": "DEBUG", + "level": "INFO", "propagate": False, }, # Django wrapper for OpenID Connect "djangooidc": { "handlers": ["console"], - "level": "DEBUG", + "level": "INFO", "propagate": False, }, # Our app! @@ -562,7 +562,10 @@ OIDC_PROVIDERS = { "scope": ["email", "profile:name", "phone"], "user_info_request": ["email", "first_name", "last_name", "phone"], "acr_value": "http://idmanagement.gov/ns/assurance/ial/1", + # "P1" is the current IdV option; "Pb" stands for 'biometric' "vtr": ["Pb","P1"], + # The url that biometric authentication takes place at. + # A similar analog is the url for acr_value. "vtm": "https://developer.login.gov/vot-trust-framework", }, "client_registration": { @@ -583,7 +586,8 @@ OIDC_PROVIDERS = { "acr_value": "http://idmanagement.gov/ns/assurance/ial/1", # "P1" is the current IdV option; "Pb" stands for 'biometric' "vtr": ["Pb","P1"], - # Stand in replacement for the step_up_acr_value for ial2 + # The url that biometric authentication takes place at. + # A similar analog is the url for acr_value. "vtm": "https://developer.login.gov/vot-trust-framework", }, "client_registration": { From 733cc53f91216ca3240fd321517c7be24763b2cd Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 16 May 2024 13:37:23 -0600 Subject: [PATCH 06/18] Update src/djangooidc/views.py --- src/djangooidc/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/djangooidc/views.py b/src/djangooidc/views.py index 69bd89afe..c36d4e77d 100644 --- a/src/djangooidc/views.py +++ b/src/djangooidc/views.py @@ -100,7 +100,7 @@ def login_callback(request): needs_biometric_validation = _requires_biometric_auth(userinfo) request.session["needs_biometric_validation"] = needs_biometric_validation - # Return a redirect request to a new auth url that enables biometric validation + # Return a redirect request to a new auth url that does biometric validation if needs_biometric_validation: return CLIENT.create_authn_request(request.session, do_biometric_auth=True) From 7681cc4f0ba905c136d0b5e6bbe50567e3b85b69 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 16 May 2024 14:15:42 -0600 Subject: [PATCH 07/18] Linting and fix unit tests --- src/djangooidc/oidc.py | 52 ++++++++++++++++----------- src/djangooidc/tests/test_views.py | 56 ++++++++---------------------- src/djangooidc/views.py | 21 ++++++++--- src/registrar/config/settings.py | 4 +-- 4 files changed, 65 insertions(+), 68 deletions(-) diff --git a/src/djangooidc/oidc.py b/src/djangooidc/oidc.py index f95146b11..80915d740 100644 --- a/src/djangooidc/oidc.py +++ b/src/djangooidc/oidc.py @@ -117,26 +117,7 @@ class Client(oic.Client): logger.debug("request args: %s" % request_args) - try: - # prepare the request for sending - cis = self.construct_AuthorizationRequest(request_args=request_args) - logger.debug("request: %s" % cis) - - # obtain the url and headers from the prepared request - url, body, headers, cis = self.uri_and_body( - AuthorizationRequest, - cis, - method="GET", - request_args=request_args, - ) - - logger.debug("body: %s" % body) - logger.debug("URL: %s" % url) - logger.debug("headers: %s" % headers) - except Exception as err: - logger.error(err) - logger.error("Failed to prepare request for %s" % state) - raise o_e.InternalError(locator=state) + url, headers = self._prepare_authn_request(request_args, state) # C901 too complex try: # create the redirect object @@ -159,6 +140,35 @@ class Client(oic.Client): request_args["vtr"] = self.get_vtr_value() request_args["vtm"] = self.get_vtm_value() + def _prepare_authn_request(self, request_args, state): + """ + Constructs an authorization request. Then, assembles the url, body, headers, and cis. + + Returns the assembled url and associated header information: `(url, headers)` + """ + try: + # prepare the request for sending + cis = self.construct_AuthorizationRequest(request_args=request_args) + logger.debug("request: %s" % cis) + + # obtain the url and headers from the prepared request + url, body, headers, cis = self.uri_and_body( + AuthorizationRequest, + cis, + method="GET", + request_args=request_args, + ) + + logger.debug("body: %s" % body) + logger.debug("URL: %s" % url) + logger.debug("headers: %s" % headers) + except Exception as err: + logger.error(err) + logger.error("Failed to prepare request for %s" % state) + raise o_e.InternalError(locator=state) + + return (url, headers) + def callback(self, unparsed_response, session): """Step 3: Receive OP's response, request an access token, and user info.""" logger.debug("Processing the OpenID Connect callback response...") @@ -311,7 +321,7 @@ class Client(oic.Client): """Returns the vtm value from settings. This helper function is called from djangooidc views.""" return self.behaviour.get("vtm") - + def get_vtr_value(self, cleaned=True): """Returns the vtr value from settings. This helper function is called from djangooidc views.""" diff --git a/src/djangooidc/tests/test_views.py b/src/djangooidc/tests/test_views.py index bdd61b346..b25c4f602 100644 --- a/src/djangooidc/tests/test_views.py +++ b/src/djangooidc/tests/test_views.py @@ -184,7 +184,7 @@ class ViewsTest(TestCase): # patch that the request does not require step up auth # TEST # test the login callback url - with patch("djangooidc.views._requires_step_up_auth", return_value=False): + with patch("djangooidc.views._requires_biometric_auth", return_value=False): response = self.client.get(reverse("openid_login_callback")) # ASSERTIONS # assert the redirect url is the same as the 'next' value set in session @@ -224,7 +224,7 @@ class ViewsTest(TestCase): # mock that callback returns user_info; this is the expected behavior mock_client.callback.side_effect = self.user_info # patch that the request does not require step up auth - with patch("djangooidc.views._requires_step_up_auth", return_value=False): + with patch("djangooidc.views._requires_biometric_auth", return_value=False): with patch("djangooidc.views._initialize_client") as mock_init_client: with patch("djangooidc.views._client_is_none") as mock_client_is_none: # mock the client to initially be None @@ -252,7 +252,7 @@ class ViewsTest(TestCase): # mock that callback returns user_info; this is the expected behavior mock_client.callback.side_effect = self.user_info # patch that the request does not require step up auth - with patch("djangooidc.views._requires_step_up_auth", return_value=False), patch( + with patch("djangooidc.views._requires_biometric_auth", return_value=False), patch( "djangooidc.views._initialize_client" ) as mock_init_client: with patch("djangooidc.views._client_is_none", return_value=True): @@ -285,7 +285,7 @@ class ViewsTest(TestCase): # mock that callback returns user_info; this is the expected behavior mock_client.callback.side_effect = self.user_info # patch that the request does not require step up auth - with patch("djangooidc.views._requires_step_up_auth", return_value=False), patch( + with patch("djangooidc.views._requires_biometric_auth", return_value=False), patch( "djangooidc.views._initialize_client" ) as mock_init_client: with patch("djangooidc.views._client_is_none", return_value=True): @@ -319,7 +319,7 @@ class ViewsTest(TestCase): td, _ = TransitionDomain.objects.get_or_create(username="test@example.com", domain_name="test123.gov") # patch that the request does not require step up auth - with patch("djangooidc.views._requires_step_up_auth", return_value=False), patch( + with patch("djangooidc.views._requires_biometric_auth", return_value=False), patch( "djangooidc.views._initialize_client" ) as mock_init_client: with patch("djangooidc.views._client_is_none", return_value=True): @@ -353,7 +353,7 @@ class ViewsTest(TestCase): vip, _ = VerifiedByStaff.objects.get_or_create(email="test@example.com") # patch that the request does not require step up auth - with patch("djangooidc.views._requires_step_up_auth", return_value=False), patch( + with patch("djangooidc.views._requires_biometric_auth", return_value=False), patch( "djangooidc.views._initialize_client" ) as mock_init_client: with patch("djangooidc.views._client_is_none", return_value=True): @@ -374,7 +374,7 @@ class ViewsTest(TestCase): self.assertEqual(created_user.verification_type, User.VerificationTypeChoices.VERIFIED_BY_STAFF) def test_login_callback_no_step_up_auth(self, mock_client): - """Walk through login_callback when _requires_step_up_auth returns False + """Walk through login_callback when _requires_biometric_auth returns False and assert that we have a redirect to /""" with less_console_noise(): # SETUP @@ -386,59 +386,33 @@ class ViewsTest(TestCase): # patch that the request does not require step up auth # TEST # test the login callback url - with patch("djangooidc.views._requires_step_up_auth", return_value=False): + with patch("djangooidc.views._requires_biometric_auth", return_value=False): response = self.client.get(reverse("openid_login_callback")) # ASSERTIONS # assert that redirect is to / when no 'next' is set self.assertEqual(response.status_code, 302) self.assertEqual(response.url, "/") - def test_login_callback_requires_step_up_auth(self, mock_client): - """Invoke login_callback passing it a request when _requires_step_up_auth returns True - and assert that session is updated and create_authn_request (mock) is called.""" - with less_console_noise(): - # MOCK - # Configure the mock to return an expected value for get_step_up_acr_value - mock_client.return_value.get_step_up_acr_value.return_value = "step_up_acr_value" - # Create a mock request - request = self.factory.get("/some-url") - request.session = {"acr_value": ""} - # Ensure that the CLIENT instance used in login_callback is the mock - # patch _requires_step_up_auth to return True - with patch("djangooidc.views._requires_step_up_auth", return_value=True), patch( - "djangooidc.views.CLIENT.create_authn_request", return_value=MagicMock() - ) as mock_create_authn_request: - # TEST - # test the login callback - login_callback(request) - # ASSERTIONS - # create_authn_request only gets called when _requires_step_up_auth is True - # and it changes this acr_value in request.session - # Assert that acr_value is no longer empty string - self.assertNotEqual(request.session["acr_value"], "") - # And create_authn_request was called again - mock_create_authn_request.assert_called_once() - - def test_login_callback_does_not_requires_step_up_auth(self, mock_client): - """Invoke login_callback passing it a request when _requires_step_up_auth returns False + def test_login_callback_does_not_requires_biometric_auth(self, mock_client): + """Invoke login_callback passing it a request when _requires_biometric_auth returns False and assert that session is not updated and create_authn_request (mock) is not called. - Possibly redundant with test_login_callback_requires_step_up_auth""" + Possibly redundant with test_login_callback_requires_biometric_auth""" with less_console_noise(): # MOCK # Create a mock request request = self.factory.get("/some-url") request.session = {"acr_value": ""} # Ensure that the CLIENT instance used in login_callback is the mock - # patch _requires_step_up_auth to return False - with patch("djangooidc.views._requires_step_up_auth", return_value=False), patch( + # patch _requires_biometric_auth to return False + with patch("djangooidc.views._requires_biometric_auth", return_value=False), patch( "djangooidc.views.CLIENT.create_authn_request", return_value=MagicMock() ) as mock_create_authn_request: # TEST # test the login callback login_callback(request) # ASSERTIONS - # create_authn_request only gets called when _requires_step_up_auth is True + # create_authn_request only gets called when _requires_biometric_auth is True # and it changes this acr_value in request.session # Assert that acr_value is NOT updated by testing that it is still an empty string self.assertEqual(request.session["acr_value"], "") @@ -454,7 +428,7 @@ class ViewsTest(TestCase): mock_client.callback.side_effect = self.user_info mock_auth.return_value = None # TEST - with patch("djangooidc.views._requires_step_up_auth", return_value=False): + with patch("djangooidc.views._requires_biometric_auth", return_value=False): response = self.client.get(reverse("openid_login_callback")) # ASSERTIONS self.assertEqual(response.status_code, 401) diff --git a/src/djangooidc/views.py b/src/djangooidc/views.py index c36d4e77d..deca09565 100644 --- a/src/djangooidc/views.py +++ b/src/djangooidc/views.py @@ -96,7 +96,7 @@ def login_callback(request): # if not satisfied, redirect user to login requiring biometric auth # Tests for the presence of the vtm/vtr values in the userinfo object. - # If they are there, then we can set a flag in our session for tracking purposes. + # If they are there, then we can set a flag in our session for tracking purposes. needs_biometric_validation = _requires_biometric_auth(userinfo) request.session["needs_biometric_validation"] = needs_biometric_validation @@ -145,9 +145,22 @@ def login_callback(request): return error_page(request, err) -def _requires_biometric_auth(userinfo): - """if User.needs_identity_verification and step_up_acr_value not in - ial returned from callback, return True""" +def _requires_biometric_auth(userinfo) -> bool: + """ + Checks for the presence of the key 'vtm' and 'vtr' in the provided `userinfo` object. + + If they are not found, then we call `User.needs_identity_verification()`. + + Args: + userinfo (dict): A dictionary of data from the returned user object. + + Return Conditions: + If the provided user does not exist in any tables which would preclude them from doing + biometric authentication, then we return True. Otherwise, we return False. + + Alternatively, if 'vtm' and 'vtr' already exist on `userinfo`, then we return False. + + """ uuid = userinfo.get("sub", "") email = userinfo.get("email", "") if not userinfo.get("vtm") or not userinfo.get("vtr"): diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index 12b0aad78..a0b35a6ca 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -563,7 +563,7 @@ OIDC_PROVIDERS = { "user_info_request": ["email", "first_name", "last_name", "phone"], "acr_value": "http://idmanagement.gov/ns/assurance/ial/1", # "P1" is the current IdV option; "Pb" stands for 'biometric' - "vtr": ["Pb","P1"], + "vtr": ["Pb", "P1"], # The url that biometric authentication takes place at. # A similar analog is the url for acr_value. "vtm": "https://developer.login.gov/vot-trust-framework", @@ -585,7 +585,7 @@ OIDC_PROVIDERS = { "user_info_request": ["email", "first_name", "last_name", "phone"], "acr_value": "http://idmanagement.gov/ns/assurance/ial/1", # "P1" is the current IdV option; "Pb" stands for 'biometric' - "vtr": ["Pb","P1"], + "vtr": ["Pb", "P1"], # The url that biometric authentication takes place at. # A similar analog is the url for acr_value. "vtm": "https://developer.login.gov/vot-trust-framework", From ab9d1af8b12f9000588dd48b36f1ad643c3b2d41 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 16 May 2024 14:46:45 -0600 Subject: [PATCH 08/18] Add readonly fields --- src/registrar/admin.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 9905cf340..61f814c91 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1171,7 +1171,7 @@ class DomainInformationAdmin(ListHeaderAdmin, ImportExportModelAdmin): ] # Readonly fields for analysts and superusers - readonly_fields = ("other_contacts", "generic_org_type", "is_election_board") + readonly_fields = ("other_contacts", "generic_org_type", "is_election_board", "federal_agency") # Read only that we'll leverage for CISA Analysts analyst_readonly_fields = [ @@ -1438,6 +1438,7 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): "alternative_domains", "generic_org_type", "is_election_board", + "federal_agency", ) # Read only that we'll leverage for CISA Analysts @@ -1879,7 +1880,7 @@ class DomainAdmin(ListHeaderAdmin, ImportExportModelAdmin): search_fields = ["name"] search_help_text = "Search by domain name." change_form_template = "django/admin/domain_change_form.html" - readonly_fields = ["state", "expiration_date", "first_ready", "deleted"] + readonly_fields = ("state", "expiration_date", "first_ready", "deleted", "federal_agency") # Table ordering ordering = ["name"] From 921a804cab5c2dfb643717d29618916ce8c2f88b Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 16 May 2024 14:54:07 -0600 Subject: [PATCH 09/18] Change user group settings --- .../migrations/0095_create_groups_v13.py | 37 +++++++++++++++++++ src/registrar/models/user_group.py | 2 +- 2 files changed, 38 insertions(+), 1 deletion(-) create mode 100644 src/registrar/migrations/0095_create_groups_v13.py diff --git a/src/registrar/migrations/0095_create_groups_v13.py b/src/registrar/migrations/0095_create_groups_v13.py new file mode 100644 index 000000000..dc8cf4a57 --- /dev/null +++ b/src/registrar/migrations/0095_create_groups_v13.py @@ -0,0 +1,37 @@ +# This migration creates the create_full_access_group and create_cisa_analyst_group groups +# It is dependent on 0079 (which populates federal agencies) +# If permissions on the groups need changing, edit CISA_ANALYST_GROUP_PERMISSIONS +# in the user_group model then: +# [NOT RECOMMENDED] +# step 1: docker-compose exec app ./manage.py migrate --fake registrar 0035_contenttypes_permissions +# step 2: docker-compose exec app ./manage.py migrate registrar 0036_create_groups +# step 3: fake run the latest migration in the migrations list +# [RECOMMENDED] +# Alternatively: +# step 1: duplicate the migration that loads data +# step 2: docker-compose exec app ./manage.py migrate + +from django.db import migrations +from registrar.models import UserGroup +from typing import Any + + +# For linting: RunPython expects a function reference, +# so let's give it one +def create_groups(apps, schema_editor) -> Any: + UserGroup.create_cisa_analyst_group(apps, schema_editor) + UserGroup.create_full_access_group(apps, schema_editor) + + +class Migration(migrations.Migration): + dependencies = [ + ("registrar", "0094_create_groups_v12"), + ] + + operations = [ + migrations.RunPython( + create_groups, + reverse_code=migrations.RunPython.noop, + atomic=True, + ), + ] diff --git a/src/registrar/models/user_group.py b/src/registrar/models/user_group.py index 76657fe29..23cc59c69 100644 --- a/src/registrar/models/user_group.py +++ b/src/registrar/models/user_group.py @@ -64,7 +64,7 @@ class UserGroup(Group): { "app_label": "registrar", "model": "federalagency", - "permissions": ["add_federalagency", "change_federalagency", "delete_federalagency"], + "permissions": ["change_federalagency"], }, ] From 401309ac5dfe20f476e68df75dc38f0f3bf59444 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 16 May 2024 15:15:43 -0600 Subject: [PATCH 10/18] Fix tests --- src/registrar/tests/test_admin.py | 4 ++++ src/registrar/tests/test_migrations.py | 2 -- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 4a6e76e3d..8c771a73b 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -2232,6 +2232,7 @@ class TestDomainRequestAdmin(MockEppLib): "alternative_domains", "generic_org_type", "is_election_board", + "federal_agency", "id", "created_at", "updated_at", @@ -2286,6 +2287,7 @@ class TestDomainRequestAdmin(MockEppLib): "alternative_domains", "generic_org_type", "is_election_board", + "federal_agency", "creator", "about_your_organization", "requested_domain", @@ -2314,6 +2316,7 @@ class TestDomainRequestAdmin(MockEppLib): "alternative_domains", "generic_org_type", "is_election_board", + "federal_agency", ] self.assertEqual(readonly_fields, expected_fields) @@ -3172,6 +3175,7 @@ class TestDomainInformationAdmin(TestCase): "other_contacts", "generic_org_type", "is_election_board", + "federal_agency", "creator", "type_of_work", "more_organization_information", diff --git a/src/registrar/tests/test_migrations.py b/src/registrar/tests/test_migrations.py index 6d8ff7151..415644200 100644 --- a/src/registrar/tests/test_migrations.py +++ b/src/registrar/tests/test_migrations.py @@ -37,9 +37,7 @@ class TestGroups(TestCase): "add_domaininvitation", "view_domaininvitation", "change_domainrequest", - "add_federalagency", "change_federalagency", - "delete_federalagency", "analyst_access_permission", "change_user", "delete_userdomainrole", From 784d5728ae939b8a3f01b211f3aeeed80cbfab69 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 16 May 2024 15:25:10 -0600 Subject: [PATCH 11/18] Remove usergroup change --- .../migrations/0095_create_groups_v13.py | 37 ------------------- src/registrar/models/user_group.py | 2 +- src/registrar/tests/test_migrations.py | 2 + 3 files changed, 3 insertions(+), 38 deletions(-) delete mode 100644 src/registrar/migrations/0095_create_groups_v13.py diff --git a/src/registrar/migrations/0095_create_groups_v13.py b/src/registrar/migrations/0095_create_groups_v13.py deleted file mode 100644 index dc8cf4a57..000000000 --- a/src/registrar/migrations/0095_create_groups_v13.py +++ /dev/null @@ -1,37 +0,0 @@ -# This migration creates the create_full_access_group and create_cisa_analyst_group groups -# It is dependent on 0079 (which populates federal agencies) -# If permissions on the groups need changing, edit CISA_ANALYST_GROUP_PERMISSIONS -# in the user_group model then: -# [NOT RECOMMENDED] -# step 1: docker-compose exec app ./manage.py migrate --fake registrar 0035_contenttypes_permissions -# step 2: docker-compose exec app ./manage.py migrate registrar 0036_create_groups -# step 3: fake run the latest migration in the migrations list -# [RECOMMENDED] -# Alternatively: -# step 1: duplicate the migration that loads data -# step 2: docker-compose exec app ./manage.py migrate - -from django.db import migrations -from registrar.models import UserGroup -from typing import Any - - -# For linting: RunPython expects a function reference, -# so let's give it one -def create_groups(apps, schema_editor) -> Any: - UserGroup.create_cisa_analyst_group(apps, schema_editor) - UserGroup.create_full_access_group(apps, schema_editor) - - -class Migration(migrations.Migration): - dependencies = [ - ("registrar", "0094_create_groups_v12"), - ] - - operations = [ - migrations.RunPython( - create_groups, - reverse_code=migrations.RunPython.noop, - atomic=True, - ), - ] diff --git a/src/registrar/models/user_group.py b/src/registrar/models/user_group.py index 23cc59c69..76657fe29 100644 --- a/src/registrar/models/user_group.py +++ b/src/registrar/models/user_group.py @@ -64,7 +64,7 @@ class UserGroup(Group): { "app_label": "registrar", "model": "federalagency", - "permissions": ["change_federalagency"], + "permissions": ["add_federalagency", "change_federalagency", "delete_federalagency"], }, ] diff --git a/src/registrar/tests/test_migrations.py b/src/registrar/tests/test_migrations.py index 415644200..6d8ff7151 100644 --- a/src/registrar/tests/test_migrations.py +++ b/src/registrar/tests/test_migrations.py @@ -37,7 +37,9 @@ class TestGroups(TestCase): "add_domaininvitation", "view_domaininvitation", "change_domainrequest", + "add_federalagency", "change_federalagency", + "delete_federalagency", "analyst_access_permission", "change_user", "delete_userdomainrole", From 2fab655012f4e0575b8509e54dbaf64e2fe67a69 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 21 May 2024 11:35:34 -0600 Subject: [PATCH 12/18] Fix unit test --- src/djangooidc/tests/test_views.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/djangooidc/tests/test_views.py b/src/djangooidc/tests/test_views.py index b25c4f602..0ebd0ed0b 100644 --- a/src/djangooidc/tests/test_views.py +++ b/src/djangooidc/tests/test_views.py @@ -393,6 +393,32 @@ class ViewsTest(TestCase): self.assertEqual(response.status_code, 302) self.assertEqual(response.url, "/") + def test_login_callback_requires_step_up_auth(self, mock_client): + """Invoke login_callback passing it a request when _requires_step_up_auth returns True + and assert that session is updated and create_authn_request (mock) is called.""" + with less_console_noise(): + # Create a mock request + request = self.factory.get("/some-url") + request.session = {"acr_value": ""} + + # Ensure that the CLIENT instance used in login_callback is the mock + # patch _requires_step_up_auth to return True + with patch("djangooidc.views._requires_biometric_auth", return_value=True), patch( + "djangooidc.views.CLIENT.create_authn_request" + ) as mock_create_authn_request: + # TEST + # test the login callback + login_callback(request) + + # ASSERTIONS + # create_authn_request only gets called when _requires_biometric_auth is True. + # The acr_value should be blank here + self.assertEqual(request.session["acr_value"], "") + self.assertEqual(request.session["needs_biometric_validation"], True) + + # And create_authn_request was called again + mock_create_authn_request.assert_called_once() + def test_login_callback_does_not_requires_biometric_auth(self, mock_client): """Invoke login_callback passing it a request when _requires_biometric_auth returns False and assert that session is not updated and create_authn_request (mock) is not called. From c779255b283cf748194aa592962952e3821d1317 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 21 May 2024 11:44:26 -0600 Subject: [PATCH 13/18] Rename requires_biometric to requires step up --- src/djangooidc/oidc.py | 4 ++-- src/djangooidc/tests/test_views.py | 34 +++++++++++++++--------------- src/djangooidc/views.py | 6 +++--- 3 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/djangooidc/oidc.py b/src/djangooidc/oidc.py index 80915d740..6c942d6eb 100644 --- a/src/djangooidc/oidc.py +++ b/src/djangooidc/oidc.py @@ -85,7 +85,7 @@ class Client(oic.Client): def create_authn_request( self, session, - do_biometric_auth=False, + do_step_up_auth=False, extra_args=None, ): """Step 2: Construct a login URL at OP's domain and send the user to it.""" @@ -103,7 +103,7 @@ class Client(oic.Client): "nonce": session["nonce"], "redirect_uri": self.registration_response["redirect_uris"][0], } - if do_biometric_auth: + if do_step_up_auth: self._set_args_for_biometric_auth_request(session, request_args) else: request_args["acr_values"] = self.behaviour.get("acr_value") diff --git a/src/djangooidc/tests/test_views.py b/src/djangooidc/tests/test_views.py index 0ebd0ed0b..d002c71f6 100644 --- a/src/djangooidc/tests/test_views.py +++ b/src/djangooidc/tests/test_views.py @@ -184,7 +184,7 @@ class ViewsTest(TestCase): # patch that the request does not require step up auth # TEST # test the login callback url - with patch("djangooidc.views._requires_biometric_auth", return_value=False): + with patch("djangooidc.views._requires_step_up_auth", return_value=False): response = self.client.get(reverse("openid_login_callback")) # ASSERTIONS # assert the redirect url is the same as the 'next' value set in session @@ -224,7 +224,7 @@ class ViewsTest(TestCase): # mock that callback returns user_info; this is the expected behavior mock_client.callback.side_effect = self.user_info # patch that the request does not require step up auth - with patch("djangooidc.views._requires_biometric_auth", return_value=False): + with patch("djangooidc.views._requires_step_up_auth", return_value=False): with patch("djangooidc.views._initialize_client") as mock_init_client: with patch("djangooidc.views._client_is_none") as mock_client_is_none: # mock the client to initially be None @@ -252,7 +252,7 @@ class ViewsTest(TestCase): # mock that callback returns user_info; this is the expected behavior mock_client.callback.side_effect = self.user_info # patch that the request does not require step up auth - with patch("djangooidc.views._requires_biometric_auth", return_value=False), patch( + with patch("djangooidc.views._requires_step_up_auth", return_value=False), patch( "djangooidc.views._initialize_client" ) as mock_init_client: with patch("djangooidc.views._client_is_none", return_value=True): @@ -285,7 +285,7 @@ class ViewsTest(TestCase): # mock that callback returns user_info; this is the expected behavior mock_client.callback.side_effect = self.user_info # patch that the request does not require step up auth - with patch("djangooidc.views._requires_biometric_auth", return_value=False), patch( + with patch("djangooidc.views._requires_step_up_auth", return_value=False), patch( "djangooidc.views._initialize_client" ) as mock_init_client: with patch("djangooidc.views._client_is_none", return_value=True): @@ -319,7 +319,7 @@ class ViewsTest(TestCase): td, _ = TransitionDomain.objects.get_or_create(username="test@example.com", domain_name="test123.gov") # patch that the request does not require step up auth - with patch("djangooidc.views._requires_biometric_auth", return_value=False), patch( + with patch("djangooidc.views._requires_step_up_auth", return_value=False), patch( "djangooidc.views._initialize_client" ) as mock_init_client: with patch("djangooidc.views._client_is_none", return_value=True): @@ -353,7 +353,7 @@ class ViewsTest(TestCase): vip, _ = VerifiedByStaff.objects.get_or_create(email="test@example.com") # patch that the request does not require step up auth - with patch("djangooidc.views._requires_biometric_auth", return_value=False), patch( + with patch("djangooidc.views._requires_step_up_auth", return_value=False), patch( "djangooidc.views._initialize_client" ) as mock_init_client: with patch("djangooidc.views._client_is_none", return_value=True): @@ -374,7 +374,7 @@ class ViewsTest(TestCase): self.assertEqual(created_user.verification_type, User.VerificationTypeChoices.VERIFIED_BY_STAFF) def test_login_callback_no_step_up_auth(self, mock_client): - """Walk through login_callback when _requires_biometric_auth returns False + """Walk through login_callback when _requires_step_up_auth returns False and assert that we have a redirect to /""" with less_console_noise(): # SETUP @@ -386,7 +386,7 @@ class ViewsTest(TestCase): # patch that the request does not require step up auth # TEST # test the login callback url - with patch("djangooidc.views._requires_biometric_auth", return_value=False): + with patch("djangooidc.views._requires_step_up_auth", return_value=False): response = self.client.get(reverse("openid_login_callback")) # ASSERTIONS # assert that redirect is to / when no 'next' is set @@ -403,7 +403,7 @@ class ViewsTest(TestCase): # Ensure that the CLIENT instance used in login_callback is the mock # patch _requires_step_up_auth to return True - with patch("djangooidc.views._requires_biometric_auth", return_value=True), patch( + with patch("djangooidc.views._requires_step_up_auth", return_value=True), patch( "djangooidc.views.CLIENT.create_authn_request" ) as mock_create_authn_request: # TEST @@ -411,7 +411,7 @@ class ViewsTest(TestCase): login_callback(request) # ASSERTIONS - # create_authn_request only gets called when _requires_biometric_auth is True. + # create_authn_request only gets called when _requires_step_up_auth is True. # The acr_value should be blank here self.assertEqual(request.session["acr_value"], "") self.assertEqual(request.session["needs_biometric_validation"], True) @@ -419,26 +419,26 @@ class ViewsTest(TestCase): # And create_authn_request was called again mock_create_authn_request.assert_called_once() - def test_login_callback_does_not_requires_biometric_auth(self, mock_client): - """Invoke login_callback passing it a request when _requires_biometric_auth returns False + def test_login_callback_does_not_requires_step_up_auth(self, mock_client): + """Invoke login_callback passing it a request when _requires_step_up_auth returns False and assert that session is not updated and create_authn_request (mock) is not called. - Possibly redundant with test_login_callback_requires_biometric_auth""" + Possibly redundant with test_login_callback_requires_step_up_auth""" with less_console_noise(): # MOCK # Create a mock request request = self.factory.get("/some-url") request.session = {"acr_value": ""} # Ensure that the CLIENT instance used in login_callback is the mock - # patch _requires_biometric_auth to return False - with patch("djangooidc.views._requires_biometric_auth", return_value=False), patch( + # patch _requires_step_up_auth to return False + with patch("djangooidc.views._requires_step_up_auth", return_value=False), patch( "djangooidc.views.CLIENT.create_authn_request", return_value=MagicMock() ) as mock_create_authn_request: # TEST # test the login callback login_callback(request) # ASSERTIONS - # create_authn_request only gets called when _requires_biometric_auth is True + # create_authn_request only gets called when _requires_step_up_auth is True # and it changes this acr_value in request.session # Assert that acr_value is NOT updated by testing that it is still an empty string self.assertEqual(request.session["acr_value"], "") @@ -454,7 +454,7 @@ class ViewsTest(TestCase): mock_client.callback.side_effect = self.user_info mock_auth.return_value = None # TEST - with patch("djangooidc.views._requires_biometric_auth", return_value=False): + with patch("djangooidc.views._requires_step_up_auth", return_value=False): response = self.client.get(reverse("openid_login_callback")) # ASSERTIONS self.assertEqual(response.status_code, 401) diff --git a/src/djangooidc/views.py b/src/djangooidc/views.py index deca09565..3eff7b9dc 100644 --- a/src/djangooidc/views.py +++ b/src/djangooidc/views.py @@ -97,12 +97,12 @@ def login_callback(request): # Tests for the presence of the vtm/vtr values in the userinfo object. # If they are there, then we can set a flag in our session for tracking purposes. - needs_biometric_validation = _requires_biometric_auth(userinfo) + needs_biometric_validation = _requires_step_up_auth(userinfo) request.session["needs_biometric_validation"] = needs_biometric_validation # Return a redirect request to a new auth url that does biometric validation if needs_biometric_validation: - return CLIENT.create_authn_request(request.session, do_biometric_auth=True) + return CLIENT.create_authn_request(request.session, do_step_up_auth=True) user = authenticate(request=request, **userinfo) if user: @@ -145,7 +145,7 @@ def login_callback(request): return error_page(request, err) -def _requires_biometric_auth(userinfo) -> bool: +def _requires_step_up_auth(userinfo) -> bool: """ Checks for the presence of the key 'vtm' and 'vtr' in the provided `userinfo` object. From c0a0e075303e114ba16333a1ab382e94285ae45f Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 21 May 2024 12:17:25 -0600 Subject: [PATCH 14/18] Fix unit test --- src/djangooidc/oidc.py | 10 ++-------- src/djangooidc/tests/test_views.py | 14 ++++++++++---- src/djangooidc/views.py | 7 ++++--- 3 files changed, 16 insertions(+), 15 deletions(-) diff --git a/src/djangooidc/oidc.py b/src/djangooidc/oidc.py index 6c942d6eb..10fb2ec9f 100644 --- a/src/djangooidc/oidc.py +++ b/src/djangooidc/oidc.py @@ -137,8 +137,8 @@ class Client(oic.Client): def _set_args_for_biometric_auth_request(self, session, request_args): if "acr_value" in session: session.pop("acr_value") - request_args["vtr"] = self.get_vtr_value() - request_args["vtm"] = self.get_vtm_value() + request_args["vtr"] = session.get("vtr") + request_args["vtm"] = session.get("vtm") def _prepare_authn_request(self, request_args, state): """ @@ -247,12 +247,6 @@ class Client(oic.Client): raise o_e.AuthenticationFailed(locator=state) info_response_dict = info_response.to_dict() - # Define vtm/vtr information on the user dictionary so we can track this in one location. - # If a user has this information, then they are bumped up in terms of verification level. - if session.get("needs_biometric_validation") is True: - info_response_dict["vtm"] = session.get("vtm", "") - info_response_dict["vtr"] = session.get("vtr", "") - logger.debug("user info: %s" % info_response_dict) return info_response_dict diff --git a/src/djangooidc/tests/test_views.py b/src/djangooidc/tests/test_views.py index d002c71f6..4ca2fda40 100644 --- a/src/djangooidc/tests/test_views.py +++ b/src/djangooidc/tests/test_views.py @@ -406,15 +406,21 @@ class ViewsTest(TestCase): with patch("djangooidc.views._requires_step_up_auth", return_value=True), patch( "djangooidc.views.CLIENT.create_authn_request" ) as mock_create_authn_request: - # TEST - # test the login callback - login_callback(request) + with patch("djangooidc.views.CLIENT.get_vtm_value") as mock_get_vtm_value, patch( + "djangooidc.views.CLIENT.get_vtr_value" + ) as mock_get_vtr_value: + mock_get_vtm_value.return_value = "test_vtm" + mock_get_vtr_value.return_value = "test_vtr" + # TEST + # test the login callback + login_callback(request) # ASSERTIONS # create_authn_request only gets called when _requires_step_up_auth is True. # The acr_value should be blank here self.assertEqual(request.session["acr_value"], "") - self.assertEqual(request.session["needs_biometric_validation"], True) + self.assertEqual(request.session["vtm"], "test_vtm") + self.assertEqual(request.session["vtr"], "test_vtr") # And create_authn_request was called again mock_create_authn_request.assert_called_once() diff --git a/src/djangooidc/views.py b/src/djangooidc/views.py index 3eff7b9dc..9d5bbe360 100644 --- a/src/djangooidc/views.py +++ b/src/djangooidc/views.py @@ -97,11 +97,12 @@ def login_callback(request): # Tests for the presence of the vtm/vtr values in the userinfo object. # If they are there, then we can set a flag in our session for tracking purposes. - needs_biometric_validation = _requires_step_up_auth(userinfo) - request.session["needs_biometric_validation"] = needs_biometric_validation + needs_step_up_auth = _requires_step_up_auth(userinfo) # Return a redirect request to a new auth url that does biometric validation - if needs_biometric_validation: + if needs_step_up_auth: + request.session["vtm"] = CLIENT.get_vtm_value() + request.session["vtr"] = CLIENT.get_vtr_value() return CLIENT.create_authn_request(request.session, do_step_up_auth=True) user = authenticate(request=request, **userinfo) From 5e9db8f1dbde81c59e882383d0c5e6fcf7a82baf Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 21 May 2024 12:25:55 -0600 Subject: [PATCH 15/18] Fix reintroduced login loop --- src/djangooidc/oidc.py | 6 ++++++ src/djangooidc/views.py | 1 + 2 files changed, 7 insertions(+) diff --git a/src/djangooidc/oidc.py b/src/djangooidc/oidc.py index 10fb2ec9f..a720006ed 100644 --- a/src/djangooidc/oidc.py +++ b/src/djangooidc/oidc.py @@ -247,6 +247,12 @@ class Client(oic.Client): raise o_e.AuthenticationFailed(locator=state) info_response_dict = info_response.to_dict() + # Define vtm/vtr information on the user dictionary so we can track this in one location. + # If a user has this information, then they are bumped up in terms of verification level. + if session.get("needs_step_up_auth") is True: + info_response_dict["vtm"] = session.get("vtm", "") + info_response_dict["vtr"] = session.get("vtr", "") + logger.debug("user info: %s" % info_response_dict) return info_response_dict diff --git a/src/djangooidc/views.py b/src/djangooidc/views.py index 9d5bbe360..d94e173c2 100644 --- a/src/djangooidc/views.py +++ b/src/djangooidc/views.py @@ -98,6 +98,7 @@ def login_callback(request): # Tests for the presence of the vtm/vtr values in the userinfo object. # If they are there, then we can set a flag in our session for tracking purposes. needs_step_up_auth = _requires_step_up_auth(userinfo) + request.session["needs_step_up_auth"] = needs_step_up_auth # Return a redirect request to a new auth url that does biometric validation if needs_step_up_auth: From ac545ef5aaa119a62a81cb4f3d553966f4cc12ad Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 22 May 2024 08:59:40 -0600 Subject: [PATCH 16/18] Add middle name and title --- src/registrar/admin.py | 6 +++-- .../0095_user_middle_name_user_title.py | 23 +++++++++++++++++++ src/registrar/models/user.py | 11 +++++++++ 3 files changed, 38 insertions(+), 2 deletions(-) create mode 100644 src/registrar/migrations/0095_user_middle_name_user_title.py diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 9905cf340..667648a3c 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -594,7 +594,7 @@ class MyUserAdmin(BaseUserAdmin, ImportExportModelAdmin): None, {"fields": ("username", "password", "status", "verification_type")}, ), - ("Personal Info", {"fields": ("first_name", "last_name", "email")}), + ("Personal Info", {"fields": ("first_name", "middle_name", "last_name", "email", "title")}), ( "Permissions", { @@ -625,7 +625,7 @@ class MyUserAdmin(BaseUserAdmin, ImportExportModelAdmin): ) }, ), - ("Personal Info", {"fields": ("first_name", "last_name", "email")}), + ("Personal Info", {"fields": ("first_name", "middle_name", "last_name", "email", "title")}), ( "Permissions", { @@ -651,7 +651,9 @@ class MyUserAdmin(BaseUserAdmin, ImportExportModelAdmin): analyst_readonly_fields = [ "Personal Info", "first_name", + "middle_name", "last_name", + "title", "email", "Permissions", "is_active", diff --git a/src/registrar/migrations/0095_user_middle_name_user_title.py b/src/registrar/migrations/0095_user_middle_name_user_title.py new file mode 100644 index 000000000..b946ea238 --- /dev/null +++ b/src/registrar/migrations/0095_user_middle_name_user_title.py @@ -0,0 +1,23 @@ +# Generated by Django 4.2.10 on 2024-05-22 14:54 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("registrar", "0094_create_groups_v12"), + ] + + operations = [ + migrations.AddField( + model_name="user", + name="middle_name", + field=models.CharField(blank=True, null=True), + ), + migrations.AddField( + model_name="user", + name="title", + field=models.CharField(blank=True, null=True, verbose_name="title / role"), + ), + ] diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 8a9fe425f..ce14c0a69 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -80,6 +80,17 @@ class User(AbstractUser): db_index=True, ) + middle_name = models.CharField( + null=True, + blank=True, + ) + + title = models.CharField( + null=True, + blank=True, + verbose_name="title / role", + ) + verification_type = models.CharField( choices=VerificationTypeChoices.choices, null=True, From 05540e66f50f63b9042868793fa7b4c1a208a0ec Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 22 May 2024 09:34:32 -0600 Subject: [PATCH 17/18] Fix unit test --- src/registrar/tests/test_admin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 4a6e76e3d..0a1bd1361 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -3534,7 +3534,7 @@ class TestMyUserAdmin(TestCase): ) }, ), - ("Personal Info", {"fields": ("first_name", "last_name", "email")}), + ("Personal Info", {"fields": ("first_name", "middle_name", "last_name", "email", "title")}), ("Permissions", {"fields": ("is_active", "groups")}), ("Important dates", {"fields": ("last_login", "date_joined")}), ) From 0c863119829ea8b62cae0f307409e2ce5bdb90d1 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 23 May 2024 10:02:59 -0600 Subject: [PATCH 18/18] Fix refresh bug There was a bug where the session would not retain data correctly after a hard refresh. This fixes that --- src/djangooidc/oidc.py | 2 ++ src/djangooidc/views.py | 4 +++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/djangooidc/oidc.py b/src/djangooidc/oidc.py index a720006ed..6e6c209f0 100644 --- a/src/djangooidc/oidc.py +++ b/src/djangooidc/oidc.py @@ -250,6 +250,8 @@ class Client(oic.Client): # Define vtm/vtr information on the user dictionary so we can track this in one location. # If a user has this information, then they are bumped up in terms of verification level. if session.get("needs_step_up_auth") is True: + if "ial" in info_response_dict: + info_response_dict.pop("ial") info_response_dict["vtm"] = session.get("vtm", "") info_response_dict["vtr"] = session.get("vtr", "") diff --git a/src/djangooidc/views.py b/src/djangooidc/views.py index d94e173c2..bb2cebd38 100644 --- a/src/djangooidc/views.py +++ b/src/djangooidc/views.py @@ -165,7 +165,9 @@ def _requires_step_up_auth(userinfo) -> bool: """ uuid = userinfo.get("sub", "") email = userinfo.get("email", "") - if not userinfo.get("vtm") or not userinfo.get("vtr"): + # This value is returned after successful auth + user_verified = userinfo.get("vot", "") + if not userinfo.get("vtm") or not userinfo.get("vtr") or not user_verified: # The acr of this attempt is not at the highest level # so check if the user needs the higher level return User.needs_identity_verification(email, uuid)