From 1001454a856a2edd002f295aeafd023953082d91 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 7 Dec 2023 16:33:35 -0500 Subject: [PATCH] More oidc tests test_login_callback_requires_step_up_auth and test_login_callback_no_step_up_auth, lint --- src/djangooidc/oidc.py | 4 +- src/djangooidc/tests/test_views.py | 90 +++++++++++++++++++----------- src/djangooidc/views.py | 12 ++-- src/registrar/models/user.py | 23 ++++---- src/registrar/tests/test_models.py | 10 ++-- 5 files changed, 82 insertions(+), 57 deletions(-) diff --git a/src/djangooidc/oidc.py b/src/djangooidc/oidc.py index 798771da2..91bfddc66 100644 --- a/src/djangooidc/oidc.py +++ b/src/djangooidc/oidc.py @@ -147,7 +147,7 @@ class Client(oic.Client): 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.""" logger.debug("Processing the OpenID Connect callback response...") @@ -276,7 +276,7 @@ class Client(oic.Client): """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( self.client_id, diff --git a/src/djangooidc/tests/test_views.py b/src/djangooidc/tests/test_views.py index 79266430d..3ae3bef82 100644 --- a/src/djangooidc/tests/test_views.py +++ b/src/djangooidc/tests/test_views.py @@ -1,9 +1,9 @@ from unittest.mock import MagicMock, patch -from django.http import HttpResponse, HttpResponseRedirect +from django.http import HttpResponse from django.test import Client, TestCase, RequestFactory from django.urls import reverse -from ..views import login_callback, requires_step_up_auth +from ..views import login_callback from .common import less_console_noise @@ -61,46 +61,32 @@ class ViewsTest(TestCase): # mock mock_client.callback.side_effect = self.user_info # test - with patch("djangooidc.views.requires_step_up_auth", return_value=False), \ - less_console_noise(): + with patch("djangooidc.views.requires_step_up_auth", return_value=False), less_console_noise(): response = self.client.get(reverse("openid_login_callback")) # assert self.assertEqual(response.status_code, 302) self.assertEqual(response.url, reverse("logout")) - + def test_login_callback_no_step_up_auth(self, mock_client): + """Walk through login_callback when requires_step_up_auth returns False + and assert that we have a redirect to /""" # setup session = self.client.session session.save() # mock mock_client.callback.side_effect = self.user_info # test - with patch("djangooidc.views.requires_step_up_auth", return_value=False), \ - less_console_noise(): + with patch("djangooidc.views.requires_step_up_auth", return_value=False), less_console_noise(): response = self.client.get(reverse("openid_login_callback")) # assert self.assertEqual(response.status_code, 302) self.assertEqual(response.url, "/") - - @patch.object(requires_step_up_auth, return_value=True) - def test_login_callback_requires_step_up_auth(self, mock_client): - # setup - callback_url = reverse("openid_login_callback") - # session = self.client.session - # session.save() - # mock - # mock_client.callback.side_effect = self.user_info - # mock_client.create_authn_request.side_effect = self.say_hi - # test - # with patch("djangooidc.views.requires_step_up_auth", return_value=True): - - response = self.client.get(reverse("openid_login_callback")) - - # assert - # self.assertEqual(response.status_code, 200) - # self.assertContains(response, "Hi") - + def test_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. + + Possibly redundant with test_login_callback_no_step_up_auth""" # 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" @@ -110,29 +96,35 @@ 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_step_up_auth", return_value=True), \ - patch("djangooidc.views.CLIENT.create_authn_request", return_value=MagicMock()) as mock_create_authn_request: + 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: login_callback(request) # Assert that get_step_up_acr_value was called and session was updated self.assertNotEqual(request.session["acr_value"], "") # And create_authn_request was called again mock_create_authn_request.assert_called_once() - + def test_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_step_up_auth""" # 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("djangooidc.views.CLIENT.create_authn_request", return_value=MagicMock()) as mock_create_authn_request: + 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: login_callback(request) # Assert that get_step_up_acr_value was NOT called and session was NOT updated self.assertEqual(request.session["acr_value"], "") - # create_authn_request was not called + # create_authn_request was not called mock_create_authn_request.assert_not_called() @patch("djangooidc.views.authenticate") @@ -141,8 +133,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), \ - less_console_noise(): + with patch("djangooidc.views.requires_step_up_auth", return_value=False), less_console_noise(): response = self.client.get(reverse("openid_login_callback")) # assert self.assertEqual(response.status_code, 401) @@ -189,3 +180,34 @@ class ViewsTest(TestCase): # assert self.assertEqual(response.status_code, 302) self.assertEqual(response.url, reverse("logout")) + + +class ViewsTestUnpatched(TestCase): + def setUp(self): + self.client = Client() + self.factory = RequestFactory() + + def say_hi(*args): + return HttpResponse("Hi") + + def user_info(*args): + return { + "sub": "TEST", + "email": "test@example.com", + "first_name": "Testy", + "last_name": "Tester", + "phone": "814564000", + } + + def test_login_callback_requires_step_up_auth(self): + """Walk through login_callback when requires_step_up_auth returns True + and assert that create_authn_request is returned.""" + + with patch("djangooidc.views.requires_step_up_auth", return_value=True), patch( + "djangooidc.views.Client.callback", return_value=self.user_info + ), patch("djangooidc.views.Client.create_authn_request", side_effect=self.say_hi): + response = self.client.get(reverse("openid_login_callback")) + + # assert + self.assertEqual(response.status_code, 200) + self.assertContains(response, "Hi") diff --git a/src/djangooidc/views.py b/src/djangooidc/views.py index 2c8e75bb3..f354a43b4 100644 --- a/src/djangooidc/views.py +++ b/src/djangooidc/views.py @@ -70,13 +70,10 @@ 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 - logger.info('login_callback start') if requires_step_up_auth(userinfo): # add acr_value to request.session - logger.info('login_callback inside requires_step_up_auth') request.session["acr_value"] = CLIENT.get_step_up_acr_value() - logger.info('login_callback after get_step_up_acr_value') - # return CLIENT.create_authn_request(request.session) + return CLIENT.create_authn_request(request.session) user = authenticate(request=request, **userinfo) if user: login(request, user) @@ -87,15 +84,17 @@ def login_callback(request): except Exception as err: return error_page(request, err) + def requires_step_up_auth(userinfo): - """ if User.needs_identity_verification and step_up_acr_value not in - ial returned from callback, return True """ + """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", "") return User.needs_identity_verification(email, uuid) and acr_value != step_up_acr_value + def logout(request, next_page=None): """Redirect the user to the authentication provider (OP) logout page.""" try: @@ -125,6 +124,7 @@ def logout(request, next_page=None): if next_page: request.session["next"] = next_page + def logout_callback(request): """Simple redirection view: after logout, redirect to `next`.""" next = request.session.get("next", "/") diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index b4f2c08ab..ae278ef1b 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -70,29 +70,32 @@ class User(AbstractUser): def needs_identity_verification(cls, email, uuid): """A method used by our oidc classes to test whether a user needs email/uuid verification or the full identity PII verification""" - - # An existing user who is a domain manager of a domain (that is, - # they have an entry in UserDomainRole for their User) - try: + + # An existing user who is a domain manager of a domain (that is, + # they have an entry in UserDomainRole for their User) + try: existing_user = cls.objects.get(username=uuid) if existing_user and UserDomainRole.objects.filter(user=existing_user).exists(): return False - except: + except cls.DoesNotExist: + # Do nothing when the user is not found, as we're checking for existence. pass - + except Exception as err: + raise err + # A new incoming user who is a domain manager for one of the domains # that we inputted from Verisign (that is, their email address appears - # in the username field of a TransitionDomain) + # in the username field of a TransitionDomain) if TransitionDomain.objects.filter(username=email).exists(): return False - + # A new incoming user who is being invited to be a domain manager (that is, # their email address is in DomainInvitation for an invitation that is not yet "retrieved"). if DomainInvitation.objects.filter(email=email, status=DomainInvitation.INVITED): return False - + return True - + def check_domain_invitations_on_login(self): """When a user first arrives on the site, we need to retrieve any domain invitations that match their email address.""" diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 0add94ce6..ef65aa24d 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -613,7 +613,7 @@ class TestUser(TestCase): self.email = "mayor@igorville.gov" self.domain_name = "igorvilleInTransition.gov" self.domain, _ = Domain.objects.get_or_create(name="igorville.gov") - self.user, _ = User.objects.get_or_create(email=self.email) + self.user, _ = User.objects.get_or_create(email=self.email) def tearDown(self): super().tearDown() @@ -632,25 +632,25 @@ class TestUser(TestCase): are created.""" self.user.on_each_login() self.assertFalse(Domain.objects.filter(name=self.domain_name).exists()) - + def test_identity_verification_with_domain_manager(self): """A domain manager should return False when tested with class method needs_identity_verification""" UserDomainRole.objects.get_or_create(user=self.user, domain=self.domain, role=UserDomainRole.Roles.MANAGER) self.assertFalse(User.needs_identity_verification(self.user.email, self.user.username)) - + def test_identity_verification_with_transition_user(self): """A user from the Verisign transition should return False when tested with class method needs_identity_verification""" TransitionDomain.objects.get_or_create(username=self.user.email, domain_name=self.domain_name) self.assertFalse(User.needs_identity_verification(self.user.email, self.user.username)) - + def test_identity_verification_with_invited_user(self): """An invited user should return False when tested with class method needs_identity_verification""" DomainInvitation.objects.get_or_create(email=self.user.email, domain=self.domain) self.assertFalse(User.needs_identity_verification(self.user.email, self.user.username)) - + def test_identity_verification_with_new_user(self): """A new user who's neither transitioned nor invited should return True when tested with class method needs_identity_verification"""