From 99b45bd1a361a68f7768b5e26dcde2578af35782 Mon Sep 17 00:00:00 2001 From: Christian Franke Date: Mon, 12 Oct 2020 13:44:22 +0200 Subject: [PATCH] Verify user_can_authenticate when authenticating user The standard Django `ModelBackend` only returns a user from `authenticate` if `user_can_authenticate` is `True`. The `OIDCAuthenticationBackend` doesn't check `user_can_authenticate`, in its `authenticate` method, allowing a user to log-in that has `is_active` set to `False`. This behavior is a surprising deviation from the way `ModelBackend` works. It can lead to security problems because other important authentication checks may inadvertently be bypassed, if the user implements them in `user_can_authenticate`, expecting `OIDCAuthenticationBackend` to call it the same as any other `ModelBackend`. It seems proper for the `OIDCAuthenticationBackend` to verify `user_can_authenticate` and it seems to me like there are no problems associated with doing so, so let's just call `user_can_authenticate` to resolve the problems described above. --- HISTORY.rst | 6 +++ mozilla_django_oidc/auth.py | 5 +- tests/test_auth.py | 104 ++++++++++++++++++++++++++++++++++++ 3 files changed, 113 insertions(+), 2 deletions(-) diff --git a/HISTORY.rst b/HISTORY.rst index 75d87821..b30a3ebf 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -6,6 +6,12 @@ History pending ======= +Backwards-incompatible changes: + +* ``OIDCAuthenticationBackend.authenticate`` now calls ``user_can_authenticate``. + +Minor: + * Make `get_or_create_user` compatible with custom scope configuration by moving scope specific code to `describe_user_by_claims` diff --git a/mozilla_django_oidc/auth.py b/mozilla_django_oidc/auth.py index b211571a..068c7013 100644 --- a/mozilla_django_oidc/auth.py +++ b/mozilla_django_oidc/auth.py @@ -319,7 +319,8 @@ def get_or_create_user(self, access_token, id_token, payload): users = self.filter_users_by_claims(user_info) if len(users) == 1: - return self.update_user(users[0], user_info) + user = self.update_user(users[0], user_info) + return user if self.user_can_authenticate(user) else None elif len(users) > 1: # In the rare case that two user accounts have the same email address, # bail. Randomly selecting one seems really wrong. @@ -327,7 +328,7 @@ def get_or_create_user(self, access_token, id_token, payload): raise SuspiciousOperation(msg) elif self.get_settings('OIDC_CREATE_USER', True): user = self.create_user(user_info) - return user + return user if self.user_can_authenticate(user) else None else: LOGGER.debug('Login failed: No user with %s found, and ' 'OIDC_CREATE_USER is False', diff --git a/tests/test_auth.py b/tests/test_auth.py index fde90d7d..eed73e73 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -365,6 +365,54 @@ def test_successful_authentication_existing_user_upper_case(self, token_mock, re self.assertEqual(auth_request.session.get('oidc_id_token'), 'id_token') self.assertEqual(auth_request.session.get('oidc_access_token'), 'access_granted') + @patch('mozilla_django_oidc.auth.requests') + @patch('mozilla_django_oidc.auth.OIDCAuthenticationBackend.verify_token') + def test_failed_authentication_inactive_user(self, token_mock, request_mock): + """Test successful authentication for existing user.""" + auth_request = RequestFactory().get('/foo', {'code': 'foo', + 'state': 'bar'}) + auth_request.session = {} + + User.objects.create_user(username='a_username', + email='email@example.com', + is_active=False) + token_mock.return_value = True + get_json_mock = Mock() + get_json_mock.json.return_value = { + 'nickname': 'a_username', + 'email': 'email@example.com' + } + request_mock.get.return_value = get_json_mock + post_json_mock = Mock() + post_json_mock.json.return_value = { + 'id_token': 'id_token', + 'access_token': 'access_granted' + } + request_mock.post.return_value = post_json_mock + + post_data = { + 'client_id': 'example_id', + 'client_secret': 'client_secret', + 'grant_type': 'authorization_code', + 'code': 'foo', + 'redirect_uri': 'http://testserver/callback/' + } + self.assertIsNone(self.backend.authenticate(request=auth_request)) + token_mock.assert_called_once_with('id_token', nonce=None) + request_mock.post.assert_called_once_with('https://server.example.com/token', + data=post_data, + auth=None, + verify=True, + timeout=None, + proxies=None) + request_mock.get.assert_called_once_with( + 'https://server.example.com/user', + headers={'Authorization': 'Bearer access_granted'}, + verify=True, + timeout=None, + proxies=None + ) + @patch('mozilla_django_oidc.auth.requests') @patch('mozilla_django_oidc.auth.OIDCAuthenticationBackend.verify_token') @patch('mozilla_django_oidc.auth.OIDCAuthenticationBackend.verify_claims') @@ -468,6 +516,62 @@ def test_successful_authentication_new_user(self, token_mock, request_mock, algo proxies=None, ) + @patch.object(settings, 'OIDC_USERNAME_ALGO') + @patch('mozilla_django_oidc.auth.requests') + @patch('mozilla_django_oidc.auth.OIDCAuthenticationBackend.verify_token') + @patch('mozilla_django_oidc.auth.OIDCAuthenticationBackend.user_can_authenticate') + def test_failed_authentication_new_user_cannot_authenticate( + self, can_authenticate_mock, token_mock, request_mock, algo_mock + ): + """Test successful authentication and user creation.""" + auth_request = RequestFactory().get('/foo', {'code': 'foo', + 'state': 'bar'}) + auth_request.session = {} + + can_authenticate_mock.return_value = False + algo_mock.return_value = 'username_algo' + token_mock.return_value = True + get_json_mock = Mock() + get_json_mock.json.return_value = { + 'nickname': 'a_username', + 'email': 'email@example.com' + } + request_mock.get.return_value = get_json_mock + post_json_mock = Mock() + post_json_mock.json.return_value = { + 'id_token': 'id_token', + 'access_token': 'access_granted' + } + request_mock.post.return_value = post_json_mock + post_data = { + 'client_id': 'example_id', + 'client_secret': 'client_secret', + 'grant_type': 'authorization_code', + 'code': 'foo', + 'redirect_uri': 'http://testserver/callback/', + } + self.assertEqual(User.objects.all().count(), 0) + self.assertIsNone(self.backend.authenticate(request=auth_request)) + self.assertEqual(User.objects.all().count(), 1) + user = User.objects.all()[0] + self.assertEqual(user.email, 'email@example.com') + self.assertEqual(user.username, 'username_algo') + + token_mock.assert_called_once_with('id_token', nonce=None) + request_mock.post.assert_called_once_with('https://server.example.com/token', + data=post_data, + auth=None, + verify=True, + timeout=None, + proxies=None) + request_mock.get.assert_called_once_with( + 'https://server.example.com/user', + headers={'Authorization': 'Bearer access_granted'}, + verify=True, + timeout=None, + proxies=None, + ) + @override_settings(OIDC_TOKEN_USE_BASIC_AUTH=True) @override_settings(OIDC_STORE_ACCESS_TOKEN=True) @override_settings(OIDC_STORE_ID_TOKEN=True)