From 463929f3493d5708e575df82f36ebc074da11974 Mon Sep 17 00:00:00 2001 From: Anders Kaseorg Date: Fri, 11 Sep 2020 19:18:53 -0700 Subject: [PATCH] urls: Migrate re_path routes to path. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Django treats path("") like re_path(r"(?P[^/]+)") and path("") like re_path(r"(?P.+)"). This is more readable and consistent than the mix of slightly different regexes we had before, and fixes various bugs: • The r'apps/(.*)$' regex was missing a start anchor ^, so it incorrectly matched all URLs that included apps/ as a substring anywhere. • The r'accounts/login/(google)/$' regex was missing a start anchor ^, so it incorrectly matched all URLs that ended with accounts/login/google/. • The type annotation of zerver.views.realm_export.delete_realm_export takes export_id as an int, but it was previously passed as a string. • The type annotation of zerver.views.users.avatar takes medium as a bool, but it was previously passed as a string. • The [0-9A-Za-z]+ pattern for uidb64 was missing the - and _ characters that can validly be part of a base64url encoded string (although I think the id is actually a decimal integer here, in which case only 012345ADEIMNOQTUYcgjkwxyz are present in its base64url encoding). Signed-off-by: Anders Kaseorg --- static/js/portico/integrations_dev_panel.js | 2 +- zerver/lib/test_helpers.py | 2 +- zerver/tests/test_docs.py | 5 + zerver/tests/test_realm_emoji.py | 2 +- zerver/views/portico.py | 4 +- zproject/dev_urls.py | 8 +- zproject/urls.py | 186 +++++++++++--------- 7 files changed, 113 insertions(+), 96 deletions(-) diff --git a/static/js/portico/integrations_dev_panel.js b/static/js/portico/integrations_dev_panel.js index b082c80298..ae567e276c 100644 --- a/static/js/portico/integrations_dev_panel.js +++ b/static/js/portico/integrations_dev_panel.js @@ -220,7 +220,7 @@ function get_fixtures(integration_name) { // We don't have the fixtures for this integration; fetch them // from the backend. Relative url pattern: - // /devtools/integrations/(?P.+)/fixtures + // /devtools/integrations//fixtures channel.get({ url: "/devtools/integrations/" + integration_name + "/fixtures", // Since the user may add or modify fixtures as they edit. diff --git a/zerver/lib/test_helpers.py b/zerver/lib/test_helpers.py index fa1ab3061c..09294b7e1e 100644 --- a/zerver/lib/test_helpers.py +++ b/zerver/lib/test_helpers.py @@ -429,7 +429,7 @@ def write_instrumentation_reports(full_suite: bool, include_webhooks: bool) -> N 'node-coverage/(?P.+)', 'docs/(?P.+)', 'casper/(?P.+)', - 'static/(?P.*)', + 'static/(?P.+)', *(webhook.url for webhook in WEBHOOK_INTEGRATIONS if not include_webhooks), } diff --git a/zerver/tests/test_docs.py b/zerver/tests/test_docs.py index c41ca86894..6c0fcfa91a 100644 --- a/zerver/tests/test_docs.py +++ b/zerver/tests/test_docs.py @@ -460,6 +460,11 @@ class AppsPageTest(ZulipTestCase): self.assertEqual(result.status_code, 301) self.assertTrue(result['Location'] == 'https://zulip.com/apps/') + with self.settings(ZILENCER_ENABLED=False): + result = self.client_get('/apps/linux') + self.assertEqual(result.status_code, 301) + self.assertTrue(result['Location'] == 'https://zulip.com/apps/') + with self.settings(ZILENCER_ENABLED=True): result = self.client_get('/apps/') self.assertEqual(result.status_code, 200) diff --git a/zerver/tests/test_realm_emoji.py b/zerver/tests/test_realm_emoji.py index 69483a9bfa..2651cdac07 100644 --- a/zerver/tests/test_realm_emoji.py +++ b/zerver/tests/test_realm_emoji.py @@ -107,7 +107,7 @@ class RealmEmojiTest(ZulipTestCase): self.login('iago') with get_test_image_file('img.png') as fp1: emoji_data = {'f1': fp1} - result = self.client_post('/json/realm/emoji/', info=emoji_data) + result = self.client_post('/json/realm/emoji/%20', info=emoji_data) self.assert_json_error(result, 'Emoji name is missing') def test_upload_admins_only(self) -> None: diff --git a/zerver/views/portico.py b/zerver/views/portico.py index c854bd7040..47709a1192 100644 --- a/zerver/views/portico.py +++ b/zerver/views/portico.py @@ -1,3 +1,5 @@ +from typing import Optional + import orjson from django.conf import settings from django.http import HttpRequest, HttpResponse, HttpResponseRedirect @@ -10,7 +12,7 @@ from zerver.models import Realm @add_google_analytics -def apps_view(request: HttpRequest, _: str) -> HttpResponse: +def apps_view(request: HttpRequest, platform: Optional[str] = None) -> HttpResponse: if settings.ZILENCER_ENABLED: return TemplateResponse( request, diff --git a/zproject/dev_urls.py b/zproject/dev_urls.py index 5f0ab9fbf0..1cb4592fd4 100644 --- a/zproject/dev_urls.py +++ b/zproject/dev_urls.py @@ -5,7 +5,7 @@ from django.conf import settings from django.conf.urls.static import static from django.contrib.staticfiles.views import serve as staticfiles_serve from django.http import HttpRequest, HttpResponse -from django.urls import path, re_path +from django.urls import path from django.views.generic import TemplateView from django.views.static import serve @@ -68,7 +68,7 @@ urls = [ # Serve static assets via the Django server if use_prod_static: urls += [ - re_path(r'^static/(?P.*)$', serve, {'document_root': settings.STATIC_ROOT}), + path('static/', serve, {'document_root': settings.STATIC_ROOT}), ] else: def serve_static(request: HttpRequest, path: str) -> HttpResponse: @@ -84,8 +84,8 @@ i18n_urls = [ # On a production instance, these files would be served by nginx. if settings.LOCAL_UPLOADS_DIR is not None: - avatars_url = re_path( - r'^user_avatars/(?P.*)$', + avatars_url = path( + 'user_avatars/', serve, {'document_root': os.path.join(settings.LOCAL_UPLOADS_DIR, "avatars")}, ) diff --git a/zproject/urls.py b/zproject/urls.py index 477d8046f7..f45fdf64df 100644 --- a/zproject/urls.py +++ b/zproject/urls.py @@ -9,7 +9,7 @@ from django.contrib.auth.views import ( PasswordResetConfirmView, PasswordResetDoneView, ) -from django.urls import path, re_path +from django.urls import path from django.utils.module_loading import import_string from django.views.generic import RedirectView, TemplateView @@ -83,23 +83,23 @@ v1_api_and_json_patterns = [ path('generate_204', zerver.views.registration.generate_204, name='zerver.views.registration.generate_204'), - re_path(r'^realm/subdomain/(?P\S+)$', zerver.views.realm.check_subdomain_available, - name='zerver.views.realm.check_subdomain_available'), + path('realm/subdomain/', zerver.views.realm.check_subdomain_available, + name='zerver.views.realm.check_subdomain_available'), # realm/domains -> zerver.views.realm_domains path('realm/domains', rest_dispatch, {'GET': 'zerver.views.realm_domains.list_realm_domains', 'POST': 'zerver.views.realm_domains.create_realm_domain'}), - re_path(r'^realm/domains/(?P\S+)$', rest_dispatch, - {'PATCH': 'zerver.views.realm_domains.patch_realm_domain', - 'DELETE': 'zerver.views.realm_domains.delete_realm_domain'}), + path('realm/domains/', rest_dispatch, + {'PATCH': 'zerver.views.realm_domains.patch_realm_domain', + 'DELETE': 'zerver.views.realm_domains.delete_realm_domain'}), # realm/emoji -> zerver.views.realm_emoji path('realm/emoji', rest_dispatch, {'GET': 'zerver.views.realm_emoji.list_emoji'}), - re_path(r'^realm/emoji/(?P.*)$', rest_dispatch, - {'POST': 'zerver.views.realm_emoji.upload_emoji', - 'DELETE': ('zerver.views.realm_emoji.delete_emoji', {"intentionally_undocumented"})}), + path('realm/emoji/', rest_dispatch, + {'POST': 'zerver.views.realm_emoji.upload_emoji', + 'DELETE': ('zerver.views.realm_emoji.delete_emoji', {"intentionally_undocumented"})}), # this endpoint throws a status code 400 JsonableError when it should be a 404. # realm/icon -> zerver.views.realm_icon @@ -250,10 +250,10 @@ v1_api_and_json_patterns = [ # user_uploads -> zerver.views.upload path('user_uploads', rest_dispatch, {'POST': 'zerver.views.upload.upload_file_backend'}), - re_path(r'^user_uploads/(?P(\d*|unk))/(?P.*)$', - rest_dispatch, - {'GET': ('zerver.views.upload.serve_file_url_backend', - {'override_api_url_scheme'})}), + path('user_uploads//', + rest_dispatch, + {'GET': ('zerver.views.upload.serve_file_url_backend', + {'override_api_url_scheme'})}), # bot_storage -> zerver.views.storage path('bot_storage', rest_dispatch, @@ -421,10 +421,12 @@ v1_api_and_json_patterns = [ path('export/realm', rest_dispatch, {'POST': 'zerver.views.realm_export.export_realm', 'GET': 'zerver.views.realm_export.get_realm_exports'}), - re_path(r'^export/realm/(?P.*)$', rest_dispatch, - {'DELETE': 'zerver.views.realm_export.delete_realm_export'}), + path('export/realm/', rest_dispatch, + {'DELETE': 'zerver.views.realm_export.delete_realm_export'}), ] +integrations_view = IntegrationView.as_view() + # These views serve pages (HTML). As such, their internationalization # must depend on the url. # @@ -442,24 +444,23 @@ i18n_urls = [ # apps; see https://github.com/zulip/zulip/issues/13081 for # background. We can remove this once older versions of the # mobile app are no longer present in the wild. - re_path(r'accounts/login/(google)/$', zerver.views.auth.start_social_login, - name='login-social'), + path('accounts/login/google/', zerver.views.auth.start_social_login, {"backend": "google"}), path('accounts/login/start/sso/', zerver.views.auth.start_remote_user_sso, name='start-login-sso'), path('accounts/login/sso/', zerver.views.auth.remote_user_sso, name='login-sso'), path('accounts/login/jwt/', zerver.views.auth.remote_user_jwt, name='login-jwt'), - re_path(r'^accounts/login/social/([\w,-]+)$', zerver.views.auth.start_social_login, - name='login-social'), - re_path(r'^accounts/login/social/([\w,-]+)/([\w,-]+)$', zerver.views.auth.start_social_login, - name='login-social-extra-arg'), - re_path(r'^accounts/register/social/([\w,-]+)$', - zerver.views.auth.start_social_signup, - name='signup-social'), - re_path(r'^accounts/register/social/([\w,-]+)/([\w,-]+)$', - zerver.views.auth.start_social_signup, - name='signup-social-extra-arg'), - re_path(r'^accounts/login/subdomain/([^/]+)$', zerver.views.auth.log_into_subdomain, - name='zerver.views.auth.log_into_subdomain'), + path('accounts/login/social/', zerver.views.auth.start_social_login, + name='login-social'), + path('accounts/login/social//', zerver.views.auth.start_social_login, + name='login-social-extra-arg'), + path('accounts/register/social/', + zerver.views.auth.start_social_signup, + name='signup-social'), + path('accounts/register/social//', + zerver.views.auth.start_social_signup, + name='signup-social-extra-arg'), + path('accounts/login/subdomain/', zerver.views.auth.log_into_subdomain, + name='zerver.views.auth.log_into_subdomain'), path('accounts/login/local/', zerver.views.auth.dev_direct_login, name='zerver.views.auth.dev_direct_login'), # We have two entries for accounts/login; only the first one is @@ -481,11 +482,11 @@ i18n_urls = [ name='zerver.views.auth.password_reset'), path('accounts/password/reset/done/', PasswordResetDoneView.as_view(template_name='zerver/reset_emailed.html')), - re_path(r'^accounts/password/reset/(?P[0-9A-Za-z]+)/(?P.+)/$', - PasswordResetConfirmView.as_view(success_url='/accounts/password/done/', - template_name='zerver/reset_confirm.html', - form_class=zerver.forms.LoggingSetPasswordForm), - name='django.contrib.auth.views.password_reset_confirm'), + path('accounts/password/reset///', + PasswordResetConfirmView.as_view(success_url='/accounts/password/done/', + template_name='zerver/reset_confirm.html', + form_class=zerver.forms.LoggingSetPasswordForm), + name='django.contrib.auth.views.password_reset_confirm'), path('accounts/password/done/', PasswordResetCompleteView.as_view(template_name='zerver/reset_done.html')), path('accounts/deactivated/', @@ -498,29 +499,29 @@ i18n_urls = [ # Registration views, require a confirmation ID. path('accounts/home/', zerver.views.registration.accounts_home, name='zerver.views.registration.accounts_home'), - re_path(r'^accounts/send_confirm/(?P[\S]+)?$', - TemplateView.as_view( - template_name='zerver/accounts_send_confirm.html'), - name='signup_send_confirm'), - re_path(r'^accounts/new/send_confirm/(?P[\S]+)?$', - TemplateView.as_view( - template_name='zerver/accounts_send_confirm.html'), - {'realm_creation': True}, name='new_realm_send_confirm'), + path('accounts/send_confirm/', + TemplateView.as_view( + template_name='zerver/accounts_send_confirm.html'), + name='signup_send_confirm'), + path('accounts/new/send_confirm/', + TemplateView.as_view( + template_name='zerver/accounts_send_confirm.html'), + {'realm_creation': True}, name='new_realm_send_confirm'), path('accounts/register/', zerver.views.registration.accounts_register, name='zerver.views.registration.accounts_register'), - re_path(r'^accounts/do_confirm/(?P[\w]+)$', - zerver.views.registration.check_prereg_key_and_redirect, - name='check_prereg_key_and_redirect'), + path('accounts/do_confirm/', + zerver.views.registration.check_prereg_key_and_redirect, + name='check_prereg_key_and_redirect'), - re_path(r'^accounts/confirm_new_email/(?P[\w]+)$', - zerver.views.user_settings.confirm_email_change, - name='zerver.views.user_settings.confirm_email_change'), + path('accounts/confirm_new_email/', + zerver.views.user_settings.confirm_email_change, + name='zerver.views.user_settings.confirm_email_change'), # Email unsubscription endpoint. Allows for unsubscribing from various types of emails, # including the welcome emails (day 1 & 2), missed PMs, etc. - re_path(r'^accounts/unsubscribe/(?P[\w]+)/(?P[\w]+)$', - zerver.views.unsubscribe.email_unsubscribe, - name='zerver.views.unsubscribe.email_unsubscribe'), + path('accounts/unsubscribe//', + zerver.views.unsubscribe.email_unsubscribe, + name='zerver.views.unsubscribe.email_unsubscribe'), # Portico-styled page used to provide email confirmation of terms acceptance. path('accounts/accept_terms/', zerver.views.home.accounts_accept_terms, @@ -537,12 +538,12 @@ i18n_urls = [ # Realm Creation path('new/', zerver.views.registration.create_realm, name='zerver.views.create_realm'), - re_path(r'^new/(?P[\w]+)$', - zerver.views.registration.create_realm, name='zerver.views.create_realm'), + path('new/', + zerver.views.registration.create_realm, name='zerver.views.create_realm'), # Realm Reactivation - re_path(r'^reactivate/(?P[\w]+)$', zerver.views.realm.realm_reactivation, - name='zerver.views.realm.realm_reactivation'), + path('reactivate/', zerver.views.realm.realm_reactivation, + name='zerver.views.realm.realm_reactivation'), # Global public streams (Zulip's way of doing archives) path('archive/streams//topics/', @@ -557,9 +558,9 @@ i18n_urls = [ path('login/', zerver.views.auth.login_page, {'template_name': 'zerver/login.html'}, name='zerver.views.auth.login_page'), - re_path(r'^join/(?P\S+)/$', - zerver.views.registration.accounts_home_from_multiuse_invite, - name='zerver.views.registration.accounts_home_from_multiuse_invite'), + path('join//', + zerver.views.registration.accounts_home_from_multiuse_invite, + name='zerver.views.registration.accounts_home_from_multiuse_invite'), # Used to generate a Zoom video call URL path('calls/zoom/register', zerver.views.video_calls.register_zoom_user), @@ -573,14 +574,16 @@ i18n_urls = [ path('integrations/doc-html/', zerver.views.documentation.integration_doc, name="zerver.views.documentation.integration_doc"), - re_path(r'^integrations/(.*)$', IntegrationView.as_view()), + path('integrations/', integrations_view), + path('integrations/', integrations_view), # Landing page, features pages, signup form, etc. path('hello/', zerver.views.portico.hello_view, name='landing-page'), path('new-user/', RedirectView.as_view(url='/hello', permanent=True)), path('features/', zerver.views.portico.landing_view, {'template_name': 'zerver/features.html'}), path('plans/', zerver.views.portico.plans_view, name='plans'), - re_path(r'apps/(.*)$', zerver.views.portico.apps_view, name='zerver.views.home.apps_view'), + path('apps/', zerver.views.portico.apps_view, name='zerver.views.home.apps_view'), + path('apps/', zerver.views.portico.apps_view, name='zerver.views.home.apps_view'), path('team/', zerver.views.portico.team_view), path('history/', zerver.views.portico.landing_view, {'template_name': 'zerver/history.html'}), path('why-zulip/', zerver.views.portico.landing_view, {'template_name': 'zerver/why-zulip.html'}), @@ -598,10 +601,10 @@ i18n_urls = [ # Terms of Service and privacy pages. path('terms/', zerver.views.portico.terms_view, name='terms'), path('privacy/', zerver.views.portico.privacy_view, name='privacy'), - re_path(r'^config-error/(?P[\w,-]+)$', zerver.views.auth.config_error_view, - name='config_error'), - re_path(r'^config-error/remoteuser/(?P[\w,-]+)$', - zerver.views.auth.config_error_view), + path('config-error/', zerver.views.auth.config_error_view, + name='config_error'), + path('config-error/remoteuser/', + zerver.views.auth.config_error_view), ] # Make a copy of i18n_urls so that they appear without prefix for english @@ -622,13 +625,13 @@ urls += [ # having to rewrite URLs, and is implemented using the # 'override_api_url_scheme' flag passed to rest_dispatch urls += [ - re_path(r'^user_uploads/temporary/([0-9A-Za-z]+)/([^/]+)$', - zerver.views.upload.serve_local_file_unauthed, - name='zerver.views.upload.serve_local_file_unauthed'), - re_path(r'^user_uploads/(?P(\d*|unk))/(?P.*)$', - rest_dispatch, - {'GET': ('zerver.views.upload.serve_file_backend', - {'override_api_url_scheme'})}), + path('user_uploads/temporary//', + zerver.views.upload.serve_local_file_unauthed, + name='zerver.views.upload.serve_local_file_unauthed'), + path('user_uploads//', + rest_dispatch, + {'GET': ('zerver.views.upload.serve_file_backend', + {'override_api_url_scheme'})}), # This endpoint serves thumbnailed versions of images using thumbor; # it requires an exception for the same reason. path('thumbnail', rest_dispatch, @@ -636,14 +639,15 @@ urls += [ {'override_api_url_scheme'})}), # Avatars have the same constraint because their URLs are included # in API data structures used by both the mobile and web clients. - re_path(r'^avatar/(?P[\S]+)/(?P[\S]+)?$', - rest_dispatch, - {'GET': ('zerver.views.users.avatar', - {'override_api_url_scheme'})}), - re_path(r'^avatar/(?P[\S]+)$', - rest_dispatch, - {'GET': ('zerver.views.users.avatar', - {'override_api_url_scheme'})}), + path('avatar/', + rest_dispatch, + {'GET': ('zerver.views.users.avatar', + {'override_api_url_scheme'})}), + path('avatar//medium', + rest_dispatch, + {'GET': ('zerver.views.users.avatar', + {'override_api_url_scheme'}), + 'medium': True}), ] # This url serves as a way to receive CSP violation reports from the users. @@ -658,9 +662,9 @@ urls += [ # such links with images previews. Now thumbor can be used for serving such # images. urls += [ - re_path(r'^external_content/(?P[\S]+)/(?P[\S]+)$', - zerver.views.camo.handle_camo_url, - name='zerver.views.camo.handle_camo_url'), + path('external_content//', + zerver.views.camo.handle_camo_url, + name='zerver.views.camo.handle_camo_url'), ] # Incoming webhook URLs @@ -743,12 +747,18 @@ urls += [path('', include('social_django.urls', namespace='social'))] urls += [path('saml/metadata.xml', zerver.views.auth.saml_sp_metadata)] # User documentation site -urls += [re_path(r'^help/(?P
.*)$', - MarkdownDirectoryView.as_view(template_name='zerver/documentation_main.html', - path_template='/zerver/help/%s.md'))] -urls += [re_path(r'^api/(?P
[-\w]*\/?)$', - MarkdownDirectoryView.as_view(template_name='zerver/documentation_main.html', - path_template='/zerver/api/%s.md'))] +help_documentation_view = MarkdownDirectoryView.as_view( + template_name='zerver/documentation_main.html', path_template='/zerver/help/%s.md' +) +api_documentation_view = MarkdownDirectoryView.as_view( + template_name='zerver/documentation_main.html', path_template='/zerver/api/%s.md' +) +urls += [ + path('help/', help_documentation_view), + path('help/', help_documentation_view), + path('api/', api_documentation_view), + path('api/', api_documentation_view), +] # Two Factor urls if settings.TWO_FACTOR_AUTHENTICATION_ENABLED: