From 84e136120826c2b64c08711178aa604be6ea3370 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 24 May 2024 12:21:20 -0600 Subject: [PATCH] Revert "Ticket #2158: Use biometric auth instead of IAL2" --- src/djangooidc/oidc.py | 84 +++++++++--------------------- src/djangooidc/tests/test_views.py | 28 ++++------ src/djangooidc/views.py | 44 ++++------------ src/registrar/config/settings.py | 12 +---- 4 files changed, 48 insertions(+), 120 deletions(-) diff --git a/src/djangooidc/oidc.py b/src/djangooidc/oidc.py index 6e6c209f0..95ed322f5 100644 --- a/src/djangooidc/oidc.py +++ b/src/djangooidc/oidc.py @@ -15,7 +15,6 @@ from oic.oic.message import AccessTokenResponse from oic.utils.authn.client import CLIENT_AUTHN_METHOD from oic.utils import keyio - from . import exceptions as o_e __author__ = "roland" @@ -85,7 +84,6 @@ class Client(oic.Client): def create_authn_request( self, session, - do_step_up_auth=False, extra_args=None, ): """Step 2: Construct a login URL at OP's domain and send the user to it.""" @@ -102,11 +100,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 do_step_up_auth: - self._set_args_for_biometric_auth_request(session, request_args) - else: - request_args["acr_values"] = self.behaviour.get("acr_value") if extra_args is not None: request_args.update(extra_args) @@ -117,35 +114,6 @@ class Client(oic.Client): logger.debug("request args: %s" % request_args) - url, headers = self._prepare_authn_request(request_args, state) # C901 too complex - - try: - # create the redirect object - response = HttpResponseRedirect(str(url)) - # add headers to the object, if any - if headers: - for key, value in headers.items(): - response[key] = value - - except Exception as err: - logger.error(err) - logger.error("Failed to create redirect object for %s" % state) - raise o_e.InternalError(locator=state) - - 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"] = session.get("vtr") - request_args["vtm"] = session.get("vtm") - - 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) @@ -158,7 +126,6 @@ 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) @@ -167,7 +134,19 @@ class Client(oic.Client): logger.error("Failed to prepare request for %s" % state) raise o_e.InternalError(locator=state) - return (url, headers) + try: + # create the redirect object + response = HttpResponseRedirect(str(url)) + # add headers to the object, if any + if headers: + for key, value in headers.items(): + response[key] = value + except Exception as err: + logger.error(err) + logger.error("Failed to create redirect object for %s" % state) + raise o_e.InternalError(locator=state) + + return response def callback(self, unparsed_response, session): """Step 3: Receive OP's response, request an access token, and user info.""" @@ -245,18 +224,9 @@ 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() - # 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", "") - - logger.debug("user info: %s" % info_response_dict) - return info_response_dict + logger.debug("user info: %s" % info_response) + return info_response.to_dict() def _request_token(self, state, code, session): """Request a token from OP to allow us to then request user info.""" @@ -315,20 +285,14 @@ 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 + 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 __repr__(self): return "Client {} {} {}".format( diff --git a/src/djangooidc/tests/test_views.py b/src/djangooidc/tests/test_views.py index 4ca2fda40..bdd61b346 100644 --- a/src/djangooidc/tests/test_views.py +++ b/src/djangooidc/tests/test_views.py @@ -397,31 +397,25 @@ class ViewsTest(TestCase): """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" + "djangooidc.views.CLIENT.create_authn_request", return_value=MagicMock() ) as mock_create_authn_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) - + # 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["vtm"], "test_vtm") - self.assertEqual(request.session["vtr"], "test_vtr") - + # 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() diff --git a/src/djangooidc/views.py b/src/djangooidc/views.py index bb2cebd38..815df4ecf 100644 --- a/src/djangooidc/views.py +++ b/src/djangooidc/views.py @@ -91,21 +91,12 @@ 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 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_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: - 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) - + # if not satisfied, redirect user to login with stepped up acr_value + if _requires_step_up_auth(userinfo): + # add acr_value to request.session + request.session["acr_value"] = CLIENT.get_step_up_acr_value() + return CLIENT.create_authn_request(request.session) user = authenticate(request=request, **userinfo) if user: @@ -147,27 +138,14 @@ def login_callback(request): return error_page(request, err) -def _requires_step_up_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. - - """ +def _requires_step_up_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", "") - # 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: + if acr_value != step_up_acr_value: # 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 a0b35a6ca..9f31ffc2c 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -562,11 +562,7 @@ 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", + "step_up_acr_value": "http://idmanagement.gov/ns/assurance/ial/2", }, "client_registration": { "client_id": "cisa_dotgov_registrar", @@ -584,11 +580,7 @@ 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", + "step_up_acr_value": "http://idmanagement.gov/ns/assurance/ial/2", }, "client_registration": { "client_id": ("urn:gov:cisa:openidconnect.profiles:sp:sso:cisa:dotgov_registrar"),