From 8484d2d59adad0c3b770d07f963202d3047fd383 Mon Sep 17 00:00:00 2001 From: FrankApiyo Date: Wed, 18 Sep 2024 17:20:25 +0300 Subject: [PATCH 1/6] Update username regex and also use email when username is invalid --- README.md | 2 +- oidc/settings.py | 2 +- oidc/viewsets.py | 23 ++++++++++++++--------- tests/test_viewsets.py | 3 ++- 4 files changed, 18 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 0eeb093..809323c 100644 --- a/README.md +++ b/README.md @@ -55,7 +55,7 @@ OPENID_CONNECT_VIEWSET_CONFIG = { # that's used to validate all field inputs retrieved for the particular key "FIELD_VALIDATION_REGEX": { "username": { - "regex": "(?!^\d+$)^.+$", + "regex": "^(?!\d+$).{4,}$", "help_text": "Username should only contain alpha numeric characters", } }, diff --git a/oidc/settings.py b/oidc/settings.py index 4c41b5a..c8a3477 100644 --- a/oidc/settings.py +++ b/oidc/settings.py @@ -18,7 +18,7 @@ "JWT_ALGORITHM": "HS256", "FIELD_VALIDATION_REGEX": { "username": { - "regex": "(?!^\d+$)^.+$", # noqa + "regex": "^(?!\d+$).{4,}$", # noqa "help_text": "Username should only contain alpha numeric characters", } }, diff --git a/oidc/viewsets.py b/oidc/viewsets.py index 7c3dc01..c8e2b6d 100644 --- a/oidc/viewsets.py +++ b/oidc/viewsets.py @@ -227,9 +227,15 @@ def _clean_user_data(self, user_data) -> Tuple[dict, Optional[list]]: user_data["first_name"] = user_data["last_name"] missing_fields.remove("first_name") - # use email as username if username is missing + # use email as username if username is missing or username is invalid if self.use_email_as_username: - if "username" in missing_fields and "email" in user_data: + username_regex = re.compile( + self.field_validation_regex["username"].get("regex") + ) + if ( + "username" in missing_fields + or not username_regex.search(user_data["username"]) + ) and "email" in user_data: username = user_data["email"].split("@")[0] if ( self.user_model.objects.filter(username__iexact=username).count() @@ -246,13 +252,12 @@ def _clean_user_data(self, user_data) -> Tuple[dict, Optional[list]]: ) # Validate retrieved username matches regex - if "username" in self.field_validation_regex: - regex = re.compile( - self.field_validation_regex["username"].get("regex") - ) - if regex.search(username): - user_data["username"] = username - missing_fields.remove("username") + if ( + "username" in self.field_validation_regex + and username_regex.search(username) + ): + user_data["username"] = username + missing_fields.remove("username") return user_data, missing_fields diff --git a/tests/test_viewsets.py b/tests/test_viewsets.py index 6d5e706..b18e15e 100644 --- a/tests/test_viewsets.py +++ b/tests/test_viewsets.py @@ -1,6 +1,7 @@ """ Tests for the OpenID Client """ + from django.contrib.auth import get_user_model from django.test import TestCase from django.test.utils import override_settings @@ -46,7 +47,7 @@ "JWT_SECRET_KEY": "abc", "FIELD_VALIDATION_REGEX": { "username": { - "regex": "^[\w\d]*$", + "regex": "^(?!\d+$).{4,}$", "help_text": "Username should only contain word characters & numbers i.e datatester23", }, }, From 0ad93dcb8cc7b5e52189afdba540a28794f64e83 Mon Sep 17 00:00:00 2001 From: FrankApiyo Date: Wed, 18 Sep 2024 18:52:47 +0300 Subject: [PATCH 2/6] Support python 3.12 --- tox.ini | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tox.ini b/tox.ini index 97cce03..e9a7178 100644 --- a/tox.ini +++ b/tox.ini @@ -1,6 +1,6 @@ [tox] envlist = - py{38,39}-django{32,40} + py{38,39,312}-django{32,40} lint [testenv:lint] @@ -21,6 +21,7 @@ deps = basepython = py38: python3.8 py39: python3.9 + py312: python3.12 commands = django32: pip install Django>=3.2.13,<4 django40: pip install Django>=4,<5 From f5cf192e5126259933d7c22495e2b0419d90a259 Mon Sep 17 00:00:00 2001 From: FrankApiyo Date: Wed, 18 Sep 2024 18:53:39 +0300 Subject: [PATCH 3/6] Update rules for setting usernames from emails If an email has a valid username we will use it to set the username --- README.md | 2 +- oidc/settings.py | 2 +- oidc/viewsets.py | 3 +- tests/test_viewsets.py | 74 +++++++++++++++++++++++++++++++++++++++++- 4 files changed, 77 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 809323c..6454932 100644 --- a/README.md +++ b/README.md @@ -55,7 +55,7 @@ OPENID_CONNECT_VIEWSET_CONFIG = { # that's used to validate all field inputs retrieved for the particular key "FIELD_VALIDATION_REGEX": { "username": { - "regex": "^(?!\d+$).{4,}$", + "regex": "^(?!\d+$)[a-zA-Z0-9]{3,}$", "help_text": "Username should only contain alpha numeric characters", } }, diff --git a/oidc/settings.py b/oidc/settings.py index c8a3477..c5d9076 100644 --- a/oidc/settings.py +++ b/oidc/settings.py @@ -18,7 +18,7 @@ "JWT_ALGORITHM": "HS256", "FIELD_VALIDATION_REGEX": { "username": { - "regex": "^(?!\d+$).{4,}$", # noqa + "regex": "^(?!\d+$)[a-zA-Z0-9_]{3,}$", # noqa "help_text": "Username should only contain alpha numeric characters", } }, diff --git a/oidc/viewsets.py b/oidc/viewsets.py index c8e2b6d..e994069 100644 --- a/oidc/viewsets.py +++ b/oidc/viewsets.py @@ -257,7 +257,8 @@ def _clean_user_data(self, user_data) -> Tuple[dict, Optional[list]]: and username_regex.search(username) ): user_data["username"] = username - missing_fields.remove("username") + if "username" in missing_fields: + missing_fields.remove("username") return user_data, missing_fields diff --git a/tests/test_viewsets.py b/tests/test_viewsets.py index b18e15e..154f9e7 100644 --- a/tests/test_viewsets.py +++ b/tests/test_viewsets.py @@ -45,9 +45,10 @@ "SSO_COOKIE_DATA": "email", "JWT_ALGORITHM": "HS256", "JWT_SECRET_KEY": "abc", + "REPLACE_USERNAME_CHARACTERS": "-.", "FIELD_VALIDATION_REGEX": { "username": { - "regex": "^(?!\d+$).{4,}$", + "regex": "^(?!\d+$)[a-zA-Z0-9_]{3,}$", "help_text": "Username should only contain word characters & numbers i.e datatester23", }, }, @@ -84,6 +85,77 @@ def test_returns_data_entry_template_on_missing_username_claim(self): self.assertEqual(response.status_code, 200) self.assertEqual(response.template_name, "oidc/oidc_user_data_entry.html") + @override_settings(OPENID_CONNECT_VIEWSET_CONFIG=OPENID_CONNECT_VIEWSET_CONFIG) + def test_user_created_successfully_when_email_has_a_valid_username(self): + """ + Test that the user is created ok when + username is not present in decoded token but email has a valid username + """ + view = UserModelOpenIDConnectViewset.as_view({"post": "callback"}) + with patch( + "oidc.viewsets.OpenIDClient.verify_and_decode_id_token" + ) as mock_func: + mock_func.return_value = { + "family_name": "bob", + "given_name": "just bob", + "username": "boby@example.com", + "email": "boby@example.com", + } + + data = {"id_token": "sadsdaio3209lkasdlkas0d.sdojdsiad.iosdadia"} + request = self.factory.post("/", data=data) + response = view(request, auth_server="default") + self.assertEqual(response.status_code, 302) + user = User.objects.get(username="boby") + self.assertEqual(user.email, "boby@example.com") + + @override_settings(OPENID_CONNECT_VIEWSET_CONFIG=OPENID_CONNECT_VIEWSET_CONFIG) + def test_returns_data_entry_template_on_invalid_username(self): + """ + Test that users are redirected to the data entry + page when username is not present in decoded token and + provided email also does not provide a valid username + """ + view = UserModelOpenIDConnectViewset.as_view({"post": "callback"}) + with patch( + "oidc.viewsets.OpenIDClient.verify_and_decode_id_token" + ) as mock_func: + mock_func.return_value = { + "family_name": "bob", + "given_name": "just bob", + "email": "bo@example.com", + } + + data = {"id_token": "sadsdaio3209lkasdlkas0d.sdojdsiad.iosdadia"} + request = self.factory.post("/", data=data) + response = view(request, auth_server="default") + self.assertEqual(response.status_code, 200) + self.assertEqual(response.template_name, "oidc/oidc_user_data_entry.html") + + @override_settings(OPENID_CONNECT_VIEWSET_CONFIG=OPENID_CONNECT_VIEWSET_CONFIG) + def test_returns_data_entry_template_on_invalid_username_and_bad_email(self): + """ + Test that users are redirected to the data entry + page when username is not present in decoded token and + provided email also does not provide a valid username + """ + view = UserModelOpenIDConnectViewset.as_view({"post": "callback"}) + with patch( + "oidc.viewsets.OpenIDClient.verify_and_decode_id_token" + ) as mock_func: + mock_func.return_value = { + "family_name": "bob", + "given_name": "just bob", + "username": "bob@example.com", + "email": "bo@example.com", + } + + data = {"id_token": "sadsdaio3209lkasdlkas0d.sdojdsiad.iosdadia"} + request = self.factory.post("/", data=data) + response = view(request, auth_server="default") + self.assertEqual(response.status_code, 400) + self.assertEqual(response.template_name, "oidc/oidc_user_data_entry.html") + def test_unrecoverable_error_on_missing_claim(self): """ Test that an error is returned when a required claim field other than the From 7427d35d000a0b4813bfd28bf155619884ceece4 Mon Sep 17 00:00:00 2001 From: FrankApiyo Date: Thu, 19 Sep 2024 10:20:32 +0300 Subject: [PATCH 4/6] Add python 3.10 & 3.11, drop 3.8 and 3.9 --- tox.ini | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/tox.ini b/tox.ini index e9a7178..d37b714 100644 --- a/tox.ini +++ b/tox.ini @@ -1,6 +1,6 @@ [tox] envlist = - py{38,39,312}-django{32,40} + py{310,311,312}-django{32,40} lint [testenv:lint] @@ -8,22 +8,23 @@ deps = pipenv flake8 black -basepython = python3.9 +basepython = python3.10 commands = pipenv sync --dev flake8 {toxinidir}/oidc - black -v {toxinidir}/oidc --check -t py38 -t py39 + black -v {toxinidir}/oidc --check -t py310 -t py311 isort -c -v {toxinidir}/oidc [testenv] deps = pipenv basepython = - py38: python3.8 - py39: python3.9 + py310: python3.10 + py311: python3.11 py312: python3.12 commands = django32: pip install Django>=3.2.13,<4 django40: pip install Django>=4,<5 pipenv sync --dev python manage.py test {toxinidir}/tests + From 4b8e4fdb82c57125d0b3a8c8731bfa9c25612f06 Mon Sep 17 00:00:00 2001 From: FrankApiyo Date: Thu, 19 Sep 2024 10:37:34 +0300 Subject: [PATCH 5/6] Update test case: Show expected error message --- oidc/settings.py | 4 ++-- tests/test_viewsets.py | 11 ++++++++--- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/oidc/settings.py b/oidc/settings.py index c5d9076..fcf24e0 100644 --- a/oidc/settings.py +++ b/oidc/settings.py @@ -18,8 +18,8 @@ "JWT_ALGORITHM": "HS256", "FIELD_VALIDATION_REGEX": { "username": { - "regex": "^(?!\d+$)[a-zA-Z0-9_]{3,}$", # noqa - "help_text": "Username should only contain alpha numeric characters", + "regex": r"^(?!\d+$)[a-zA-Z0-9_]{3,}$", # noqa + "help_text": "Username should only contain alpha numeric characters and should be at least 3 characters", } }, "REPLACE_USERNAME_CHARACTERS": "-.", diff --git a/tests/test_viewsets.py b/tests/test_viewsets.py index 154f9e7..e9c3cc5 100644 --- a/tests/test_viewsets.py +++ b/tests/test_viewsets.py @@ -48,8 +48,8 @@ "REPLACE_USERNAME_CHARACTERS": "-.", "FIELD_VALIDATION_REGEX": { "username": { - "regex": "^(?!\d+$)[a-zA-Z0-9_]{3,}$", - "help_text": "Username should only contain word characters & numbers i.e datatester23", + "regex": r"^(?!\d+$)[a-zA-Z0-9_]{3,}$", + "help_text": "Username should only contain word characters & numbers and should have 3 or more characters", }, }, } @@ -136,7 +136,7 @@ def test_returns_data_entry_template_on_invalid_username(self): def test_returns_data_entry_template_on_invalid_username_and_bad_email(self): """ Test that users are redirected to the data entry - page when username is not present in decoded token and + page when username provided in decoded token is invalid and provided email also does not provide a valid username """ view = UserModelOpenIDConnectViewset.as_view({"post": "callback"}) @@ -154,6 +154,11 @@ def test_returns_data_entry_template_on_invalid_username_and_bad_email(self): request = self.factory.post("/", data=data) response = view(request, auth_server="default") self.assertEqual(response.status_code, 400) + self.assertTrue( + response.rendered_content.startswith( + b'{"error":"Username should only contain word characters & numbers and should have 3 or more characters"' + ) + ) self.assertEqual(response.template_name, "oidc/oidc_user_data_entry.html") def test_unrecoverable_error_on_missing_claim(self): From 80c2708b52cf615d9440b11cf4706215466ec079 Mon Sep 17 00:00:00 2001 From: FrankApiyo Date: Thu, 19 Sep 2024 10:41:18 +0300 Subject: [PATCH 6/6] Update ci to use python 3.10 and 3.11 --- .github/workflows/ci.yml | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1c271d6..6312300 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -13,15 +13,15 @@ jobs: fail-fast: false matrix: include: - - python_version: 3.8 - toxenv: "py38-django32" - - python_version: 3.9 - toxenv: "py39-django32" - - python_version: 3.8 - toxenv: "py38-django40" - - python_version: 3.9 - toxenv: "py39-django40" - - python_version: 3.9 + - python_version: "3.10" + toxenv: "py310-django32" + - python_version: 3.11 + toxenv: "py311-django32" + - python_version: "3.10" + toxenv: "py310-django40" + - python_version: 3.11 + toxenv: "py311-django40" + - python_version: "3.10" toxenv: "lint" steps: - name: Checkout code