From 5a413edd3f202753806e896b7f9f2324fd74b418 Mon Sep 17 00:00:00 2001 From: Tami Takamiya Date: Mon, 13 Nov 2023 20:02:25 -0500 Subject: [PATCH 1/3] Allow wildcard for redirect_urls in OAuth2 apps --- ansible_wisdom/main/settings/base.py | 13 +++ ansible_wisdom/wildcard_oauth2/__init__.py | 9 ++ ansible_wisdom/wildcard_oauth2/apps.py | 14 +++ ansible_wisdom/wildcard_oauth2/models.py | 98 +++++++++++++++++++ .../wildcard_oauth2/tests/__init__.py | 0 .../tests/test_wildcard_oauth2.py | 72 ++++++++++++++ 6 files changed, 206 insertions(+) create mode 100644 ansible_wisdom/wildcard_oauth2/__init__.py create mode 100644 ansible_wisdom/wildcard_oauth2/apps.py create mode 100644 ansible_wisdom/wildcard_oauth2/models.py create mode 100644 ansible_wisdom/wildcard_oauth2/tests/__init__.py create mode 100644 ansible_wisdom/wildcard_oauth2/tests/test_wildcard_oauth2.py diff --git a/ansible_wisdom/main/settings/base.py b/ansible_wisdom/main/settings/base.py index 2b125354f..b0ff395c3 100644 --- a/ansible_wisdom/main/settings/base.py +++ b/ansible_wisdom/main/settings/base.py @@ -11,6 +11,7 @@ """ import os +import sys from pathlib import Path # Build paths inside the project like this: BASE_DIR / 'subdir'. @@ -170,6 +171,18 @@ 'REFRESH_TOKEN_EXPIRE_SECONDS': 864_000, # = 10 days } +# +# We need to run 'manage.py migrate' before adding our own OAuth2 application model. +# See https://django-oauth-toolkit.readthedocs.io/en/latest/advanced_topics.html +# #extending-the-application-model +# +# Also, if these lines are executed in testing, test fails with: +# django.db.utils.ProgrammingError: relation "users_user" does not exist +# +if sys.argv[1:2] not in [['migrate'], ['test']]: + INSTALLED_APPS.append('wildcard_oauth2') + OAUTH2_PROVIDER_APPLICATION_MODEL = 'wildcard_oauth2.Application' + # OAUTH: todo # - remove ansible_wisdom/users/auth.py module # - remove ansible_wisdom/users/views.py module diff --git a/ansible_wisdom/wildcard_oauth2/__init__.py b/ansible_wisdom/wildcard_oauth2/__init__.py new file mode 100644 index 000000000..ce1656e24 --- /dev/null +++ b/ansible_wisdom/wildcard_oauth2/__init__.py @@ -0,0 +1,9 @@ +""" +The module offers a custom OAuth2 Application that allows wildcard URLs. + (From: https://github.com/open-craft/oauth2-wildcard-application) +""" + + +default_app_config = ( + 'wildcard_oauth2.apps.WildcardOauth2ApplicationConfig' # pylint: disable=invalid-name +) diff --git a/ansible_wisdom/wildcard_oauth2/apps.py b/ansible_wisdom/wildcard_oauth2/apps.py new file mode 100644 index 000000000..09bf16e8c --- /dev/null +++ b/ansible_wisdom/wildcard_oauth2/apps.py @@ -0,0 +1,14 @@ +""" +Django App Configuration +""" + +from django.apps import AppConfig + + +class WildcardOauth2ApplicationConfig(AppConfig): + """ + Configures wildcard_oauth2 as a Django app plugin + """ + + name = 'wildcard_oauth2' + verbose_name = "Wildcard OAuth2 Application" diff --git a/ansible_wisdom/wildcard_oauth2/models.py b/ansible_wisdom/wildcard_oauth2/models.py new file mode 100644 index 000000000..48258fbf0 --- /dev/null +++ b/ansible_wisdom/wildcard_oauth2/models.py @@ -0,0 +1,98 @@ +""" +A custom OAuth2 application that allows wildcard for redirect_uris + +https://github.com/jazzband/django-oauth-toolkit/issues/443#issuecomment-420255286 +""" +import re +from urllib.parse import parse_qsl, unquote, urlparse + +from django.core.exceptions import ValidationError +from oauth2_provider.models import AbstractApplication +from oauth2_provider.settings import oauth2_settings + + +def validate_uris(value): + """Ensure that `value` contains valid blank-separated URIs.""" + urls = value.split() + for url in urls: + obj = urlparse(url) + if obj.fragment: + raise ValidationError('Redirect URIs must not contain fragments') + if obj.scheme.lower() not in oauth2_settings.ALLOWED_REDIRECT_URI_SCHEMES: + raise ValidationError('Redirect URI scheme is not allowed.') + if not obj.netloc: + raise ValidationError('Redirect URI must contain a domain.') + + +class Application(AbstractApplication): + """Subclass of application to allow for regular expressions for the redirect uri.""" + + @staticmethod + def _uri_is_allowed(allowed_uri, uri): + """Check that the URI conforms to these rules.""" + schemes_match = allowed_uri.scheme == uri.scheme + netloc_matches_pattern = re.fullmatch(allowed_uri.netloc, uri.netloc) + + # The original code allowed only fixed paths only with: + # paths_match = allowed_uri.path == uri.path + # However, since paths can contain variable portions (e.g. code-server), + # code was modified to support regex patterns in paths as well. + paths_match = re.fullmatch(allowed_uri.path, uri.path) + + return all([schemes_match, netloc_matches_pattern, paths_match]) + + def __init__(self, *args, **kwargs): + """Relax the validator to allow for uris with regular expressions.""" + self._meta.get_field('redirect_uris').validators = [ + validate_uris, + ] + super().__init__(*args, **kwargs) + + def redirect_uri_allowed(self, uri): + """ + Check if given url is one of the items in :attr:`redirect_uris` string. + A Redirect uri domain may be a regular expression e.g. `^(.*).example.com$` will + match all subdomains of example.com. + A Redirect uri may be `https://(.*).example.com/some/path/?q=x` + :param uri: Url to check + """ + for allowed_uri in self.redirect_uris.split(): + parsed_allowed_uri = urlparse(allowed_uri) + parsed_uri = urlparse(uri) + + if self._uri_is_allowed(parsed_allowed_uri, parsed_uri): + aqs_set = set(parse_qsl(parsed_allowed_uri.query)) + uqs_set = set(parse_qsl(parsed_uri.query)) + + if aqs_set.issubset(uqs_set): + return True + + return False + + def clean(self): + uris_with_wildcard = [uri for uri in self.redirect_uris.split(' ') if '*' in uri] + if uris_with_wildcard: + self.redirect_uris = ' '.join( + [uri for uri in self.redirect_uris.split(' ') if '*' not in uri] + ) + super().clean() + if uris_with_wildcard: + self.redirect_uris += ' ' + ' '.join(uris_with_wildcard) + + def is_usable(self, request): + # This is a hacky way to decode redirect_uri stored in an oauthlib.Request instance. + # Once the oauthlib.Request class started decoding redirect_uri correctly, this will + # be removed. + if getattr(request, '_params'): + redirect_uri = request._params.get('redirect_uri') + if redirect_uri: + request._params['redirect_uri'] = unquote(redirect_uri) + + return True + + class Meta: + db_table = 'oauth2_provider_application' + # Without the following line, tests fail with: + # RuntimeError: Model class wildcard_oauth2.models.Application doesn't declare + # an explicit app_label and isn't in an application in INSTALLED_APPS. + app_label = 'wildcard_oauth2' diff --git a/ansible_wisdom/wildcard_oauth2/tests/__init__.py b/ansible_wisdom/wildcard_oauth2/tests/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/ansible_wisdom/wildcard_oauth2/tests/test_wildcard_oauth2.py b/ansible_wisdom/wildcard_oauth2/tests/test_wildcard_oauth2.py new file mode 100644 index 000000000..dcd52d263 --- /dev/null +++ b/ansible_wisdom/wildcard_oauth2/tests/test_wildcard_oauth2.py @@ -0,0 +1,72 @@ +from django.core.exceptions import ValidationError +from django.test import TestCase + +from ..models import Application, validate_uris + +redirect_uris = [ + 'vscode://redhat.ansible', + r'https://.*\.github\.dev/extension-auth-callback?.*', + r'http://.*/.*?.*', +] + + +class _AppLabel: + app_label = 'wildcard_oauth2' + + +class WildcardOAuth2Test(TestCase): + def setUp(self): + self.app = Application(redirect_uris=' '.join(redirect_uris)) + + def test_standalone_vscode_callback_uri(self): + rc = self.app.redirect_uri_allowed('vscode://redhat.ansible') + self.assertTrue(rc) + self.app.clean() + + def test_invalid_callback_uri(self): + rc = self.app.redirect_uri_allowed('vscode://othercompany.ansible') + self.assertFalse(rc) + + def test_valid_codespases_callback_uri(self): + rc = self.app.redirect_uri_allowed( + 'https://jubilant-engine-wv4w5xw9vq9f9gg9.github.dev/' + 'extension-auth-callback?state=6766a56164972ebe9ab0350c00d9041c' + ) + self.assertTrue(rc) + + def test_invalid_codespases_callback_uri(self): + rc = self.app.redirect_uri_allowed( + 'https://jubilant-engine-wv4w5xw9vq9f9gg9.github.com/' + 'extension-auth-callback?state=6766a56164972ebe9ab0350c00d9041c' + ) + self.assertFalse(rc) + + def test_valid_code_server_callback_uri(self): + rc = self.app.redirect_uri_allowed( + 'http://localhost:18080/stable-9658969084238651b6dde258e04f4abd9b14bfd1/callback' + '?vscode-reqid=2&vscode-scheme=code-oss&vscode-authority=redhat.ansible' + ) + self.assertTrue(rc) + + +class ValidateUrisTest(TestCase): + def test_uri_no_error(self): + validate_uris('https://example.com/callback') + + def test_uri_containing_fragment(self): + try: + validate_uris('https://example.com/callback#fragment') + except ValidationError as e: + self.assertEqual(e.message, 'Redirect URIs must not contain fragments') + + def test_uri_containing_invalid_scheme(self): + try: + validate_uris('myapp://example.com/callback') + except ValidationError as e: + self.assertEqual(e.message, 'Redirect URI scheme is not allowed.') + + def test_uri_containing_no_domain(self): + try: + validate_uris('vscode:redhat.ansible') + except ValidationError as e: + self.assertEqual(e.message, 'Redirect URI must contain a domain.') From 37daf1dffb54f30f4570c089c496cb6bdd39e78a Mon Sep 17 00:00:00 2001 From: Tami Takamiya Date: Tue, 5 Dec 2023 14:37:01 -0500 Subject: [PATCH 2/3] Changes based on review comments --- ansible_wisdom/wildcard_oauth2/__init__.py | 9 --- ansible_wisdom/wildcard_oauth2/models.py | 30 ++++++++- .../tests/test_wildcard_oauth2.py | 64 ++++++++++++++++--- 3 files changed, 82 insertions(+), 21 deletions(-) diff --git a/ansible_wisdom/wildcard_oauth2/__init__.py b/ansible_wisdom/wildcard_oauth2/__init__.py index ce1656e24..e69de29bb 100644 --- a/ansible_wisdom/wildcard_oauth2/__init__.py +++ b/ansible_wisdom/wildcard_oauth2/__init__.py @@ -1,9 +0,0 @@ -""" -The module offers a custom OAuth2 Application that allows wildcard URLs. - (From: https://github.com/open-craft/oauth2-wildcard-application) -""" - - -default_app_config = ( - 'wildcard_oauth2.apps.WildcardOauth2ApplicationConfig' # pylint: disable=invalid-name -) diff --git a/ansible_wisdom/wildcard_oauth2/models.py b/ansible_wisdom/wildcard_oauth2/models.py index 48258fbf0..29ff108c5 100644 --- a/ansible_wisdom/wildcard_oauth2/models.py +++ b/ansible_wisdom/wildcard_oauth2/models.py @@ -3,6 +3,7 @@ https://github.com/jazzband/django-oauth-toolkit/issues/443#issuecomment-420255286 """ +import ipaddress import re from urllib.parse import parse_qsl, unquote, urlparse @@ -22,7 +23,24 @@ def validate_uris(value): raise ValidationError('Redirect URI scheme is not allowed.') if not obj.netloc: raise ValidationError('Redirect URI must contain a domain.') + if not is_acceptable_netloc(obj.netloc): + raise ValidationError('Redirect URI is not acceptable.') +def wildcard_string_to_regex(value): + return re.escape(value).replace('\\*', '[^\\/]*') + +def is_acceptable_netloc(value): + if '*' not in value: + return True + else: + return re.fullmatch(r'.*\.[^\.\*]+\.[^\.\*]+', value) != None + +def is_ip_address(address): + try: + ipaddress.ip_address(address) + return True + except ValueError: + return False class Application(AbstractApplication): """Subclass of application to allow for regular expressions for the redirect uri.""" @@ -31,13 +49,21 @@ class Application(AbstractApplication): def _uri_is_allowed(allowed_uri, uri): """Check that the URI conforms to these rules.""" schemes_match = allowed_uri.scheme == uri.scheme - netloc_matches_pattern = re.fullmatch(allowed_uri.netloc, uri.netloc) + + # Wildcards ('*') in netloc is supposed to be matched to a hostname, + # not an ip address. + if '*' in allowed_uri.netloc and is_ip_address(uri.hostname): + netloc_matches_pattern = None + else: + regex = wildcard_string_to_regex(allowed_uri.netloc) + netloc_matches_pattern = re.fullmatch(regex, uri.netloc) # The original code allowed only fixed paths only with: # paths_match = allowed_uri.path == uri.path # However, since paths can contain variable portions (e.g. code-server), # code was modified to support regex patterns in paths as well. - paths_match = re.fullmatch(allowed_uri.path, uri.path) + regex = wildcard_string_to_regex(allowed_uri.path) + paths_match = re.fullmatch(regex, uri.path) return all([schemes_match, netloc_matches_pattern, paths_match]) diff --git a/ansible_wisdom/wildcard_oauth2/tests/test_wildcard_oauth2.py b/ansible_wisdom/wildcard_oauth2/tests/test_wildcard_oauth2.py index dcd52d263..ec8d576e0 100644 --- a/ansible_wisdom/wildcard_oauth2/tests/test_wildcard_oauth2.py +++ b/ansible_wisdom/wildcard_oauth2/tests/test_wildcard_oauth2.py @@ -1,19 +1,22 @@ +import re + from django.core.exceptions import ValidationError from django.test import TestCase +from urllib.parse import urlparse -from ..models import Application, validate_uris +from ..models import ( + Application, + is_acceptable_netloc, + validate_uris, + wildcard_string_to_regex, +) redirect_uris = [ 'vscode://redhat.ansible', - r'https://.*\.github\.dev/extension-auth-callback?.*', - r'http://.*/.*?.*', + 'https://*.github.dev/extension-auth-callback?*', + 'http://127.0.0.1:8000/*/callback?*', ] - -class _AppLabel: - app_label = 'wildcard_oauth2' - - class WildcardOAuth2Test(TestCase): def setUp(self): self.app = Application(redirect_uris=' '.join(redirect_uris)) @@ -43,7 +46,7 @@ def test_invalid_codespases_callback_uri(self): def test_valid_code_server_callback_uri(self): rc = self.app.redirect_uri_allowed( - 'http://localhost:18080/stable-9658969084238651b6dde258e04f4abd9b14bfd1/callback' + 'http://127.0.0.1:8000/stable-9658969084238651b6dde258e04f4abd9b14bfd1/callback' '?vscode-reqid=2&vscode-scheme=code-oss&vscode-authority=redhat.ansible' ) self.assertTrue(rc) @@ -51,7 +54,9 @@ def test_valid_code_server_callback_uri(self): class ValidateUrisTest(TestCase): def test_uri_no_error(self): - validate_uris('https://example.com/callback') + validate_uris('https://example.com/callback ' + 'https://*.github.dev/extension-auth-callback?.* ' + 'http://127.0.0.1:8000/.*?.*') def test_uri_containing_fragment(self): try: @@ -70,3 +75,42 @@ def test_uri_containing_no_domain(self): validate_uris('vscode:redhat.ansible') except ValidationError as e: self.assertEqual(e.message, 'Redirect URI must contain a domain.') + + def test_uri_containing_wildcard_in_root_domain(self): + try: + validate_uris('https://*.github.*/extension-auth-callback?.*') + except ValidationError as e: + self.assertEqual(e.message, 'Redirect URI is not acceptable.') + + def test_ip_address(self): + allowed_uri = urlparse('http://*.0.1/callback') + uri = urlparse('http://123.123.0.1/callback') + self.assertFalse(Application._uri_is_allowed(allowed_uri, uri)) + + +class AcceptableNetlocTest(TestCase): + def test_valid_netlocs(self): + self.assertTrue(is_acceptable_netloc('subdomain.example.com')) + self.assertTrue(is_acceptable_netloc('*.example.com')) + self.assertTrue(is_acceptable_netloc('sub*.example.com')) + self.assertTrue(is_acceptable_netloc('*sub.example.com')) + self.assertTrue(is_acceptable_netloc('*sub*.sub.example.com')) + self.assertTrue(is_acceptable_netloc('*.*.example.com')) + + def test_invalid_netlocs(self): + self.assertFalse(is_acceptable_netloc('subdomain.*.com')) + self.assertFalse(is_acceptable_netloc('*.example.*')) + self.assertFalse(is_acceptable_netloc('sub*.example*.com')) + self.assertFalse(is_acceptable_netloc('*.com')) + + +class WildcardStringToRegExTest(TestCase): + def test_wildcard_string_to_regex(self): + self.assertEqual(wildcard_string_to_regex('*'), + '[^\\/]*') + p = wildcard_string_to_regex('*sub*.sub.example.com') + self.assertEqual(p,'[^\\/]*sub[^\\/]*\\.sub\\.example\\.com') + self.assertTrue(re.match(p, 'sub.sub.example.com')) + self.assertTrue(re.match(p, 'abcsubxyz.sub.example.com')) + self.assertTrue(re.match(p, 'abc.sub.sub.example.com')) + self.assertFalse(re.match(p, 'sub/xyz.sub.example.com')) From 8698ea6b2489a81d6f1fdcc5c470d2fec77c7cfe Mon Sep 17 00:00:00 2001 From: Tami Takamiya Date: Tue, 5 Dec 2023 15:02:42 -0500 Subject: [PATCH 3/3] pre-commit issues --- ansible_wisdom/wildcard_oauth2/models.py | 10 +++++++--- .../tests/test_wildcard_oauth2.py | 16 +++++++++------- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/ansible_wisdom/wildcard_oauth2/models.py b/ansible_wisdom/wildcard_oauth2/models.py index 29ff108c5..9841be84a 100644 --- a/ansible_wisdom/wildcard_oauth2/models.py +++ b/ansible_wisdom/wildcard_oauth2/models.py @@ -26,14 +26,17 @@ def validate_uris(value): if not is_acceptable_netloc(obj.netloc): raise ValidationError('Redirect URI is not acceptable.') + def wildcard_string_to_regex(value): return re.escape(value).replace('\\*', '[^\\/]*') + def is_acceptable_netloc(value): - if '*' not in value: - return True + if '*' in value: + return re.fullmatch(r'.*\.[^\.\*]+\.[^\.\*]+', value) is not None else: - return re.fullmatch(r'.*\.[^\.\*]+\.[^\.\*]+', value) != None + return True + def is_ip_address(address): try: @@ -42,6 +45,7 @@ def is_ip_address(address): except ValueError: return False + class Application(AbstractApplication): """Subclass of application to allow for regular expressions for the redirect uri.""" diff --git a/ansible_wisdom/wildcard_oauth2/tests/test_wildcard_oauth2.py b/ansible_wisdom/wildcard_oauth2/tests/test_wildcard_oauth2.py index ec8d576e0..048d0d2fd 100644 --- a/ansible_wisdom/wildcard_oauth2/tests/test_wildcard_oauth2.py +++ b/ansible_wisdom/wildcard_oauth2/tests/test_wildcard_oauth2.py @@ -1,8 +1,8 @@ import re +from urllib.parse import urlparse from django.core.exceptions import ValidationError from django.test import TestCase -from urllib.parse import urlparse from ..models import ( Application, @@ -17,6 +17,7 @@ 'http://127.0.0.1:8000/*/callback?*', ] + class WildcardOAuth2Test(TestCase): def setUp(self): self.app = Application(redirect_uris=' '.join(redirect_uris)) @@ -54,9 +55,11 @@ def test_valid_code_server_callback_uri(self): class ValidateUrisTest(TestCase): def test_uri_no_error(self): - validate_uris('https://example.com/callback ' - 'https://*.github.dev/extension-auth-callback?.* ' - 'http://127.0.0.1:8000/.*?.*') + validate_uris( + 'https://example.com/callback ' + 'https://*.github.dev/extension-auth-callback?.* ' + 'http://127.0.0.1:8000/.*?.*' + ) def test_uri_containing_fragment(self): try: @@ -106,10 +109,9 @@ def test_invalid_netlocs(self): class WildcardStringToRegExTest(TestCase): def test_wildcard_string_to_regex(self): - self.assertEqual(wildcard_string_to_regex('*'), - '[^\\/]*') + self.assertEqual(wildcard_string_to_regex('*'), '[^\\/]*') p = wildcard_string_to_regex('*sub*.sub.example.com') - self.assertEqual(p,'[^\\/]*sub[^\\/]*\\.sub\\.example\\.com') + self.assertEqual(p, '[^\\/]*sub[^\\/]*\\.sub\\.example\\.com') self.assertTrue(re.match(p, 'sub.sub.example.com')) self.assertTrue(re.match(p, 'abcsubxyz.sub.example.com')) self.assertTrue(re.match(p, 'abc.sub.sub.example.com'))