Mirror
/
zulip
mirror of https://github.com/zulip/zulip.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

auth: Render config error page on configuration error. 

We previously used to to redirect to config error page with
a different URL. This commit renders config error in the same
URL where configuration error is encountered. This way when
conifguration error is fixed the user can refresh to continue
normally or go back to login page from the link provided to
choose any other backend auth.

Also moved those URLs to dev_urls.py so that they can be easily
accessed to work on styling etc.

In tests, removed some of the asserts checking status code to be 200
as the function `assert_in_success_response` does that check.
This commit is contained in:
Dinesh 2020-06-17 17:55:25 +05:30 committed by Tim Abbott
parent 8674287192
commit 232eb8b7cf
7 changed files with 69 additions and 79 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
templates/zerver/config_error.html
View File

@ -97,6 +97,7 @@
{% endif %}
<p>After making your changes, remember to restart
the Zulip server.</p>
<p><a href=""> Refresh</a> to try again or <a href="/login/">click here</a> to go back to the login page.</p>
</div>
</div>

53
zerver/tests/test_auth_backends.py
View File

@ -884,16 +884,13 @@ class SocialAuthBase(DesktopFlowTestingLib, ZulipTestCase):
with self.settings(**{self.CLIENT_KEY_SETTING: None}):
result = self.social_auth_test(account_data_dict,
subdomain='zulip', next='/user_uploads/image')
self.assertEqual(result.status_code, 302)
self.assertEqual(result.url, self.CONFIG_ERROR_URL)
self.assert_in_success_response(["Configuration error"], result)
def test_config_error_development(self) -> None:
if hasattr(self, 'CLIENT_KEY_SETTING') and hasattr(self, 'CLIENT_SECRET_SETTING'):
with self.settings(**{self.CLIENT_KEY_SETTING: None}):
result = self.client_get(self.LOGIN_URL)
self.assertEqual(result.status_code, 302)
self.assertEqual(result.url, self.CONFIG_ERROR_URL)
result = self.client_get(result.url)
self.assert_in_success_response(["Configuration error"], result)
self.assert_in_success_response([self.CLIENT_KEY_SETTING.lower()], result)
self.assert_in_success_response([self.CLIENT_SECRET_SETTING.lower()], result)
self.assert_in_success_response(["zproject/dev-secrets.conf"], result)
@ -907,9 +904,7 @@ class SocialAuthBase(DesktopFlowTestingLib, ZulipTestCase):
if hasattr(self, 'CLIENT_KEY_SETTING') and hasattr(self, 'CLIENT_SECRET_SETTING'):
with self.settings(**{self.CLIENT_KEY_SETTING: None}):
result = self.client_get(self.LOGIN_URL)
self.assertEqual(result.status_code, 302)
self.assertEqual(result.url, self.CONFIG_ERROR_URL)
result = self.client_get(result.url)
self.assert_in_success_response(["Configuration error"], result)
self.assert_in_success_response([self.CLIENT_KEY_SETTING], result)
self.assert_in_success_response(["/etc/zulip/settings.py"], result)
self.assert_in_success_response([self.CLIENT_SECRET_SETTING.lower()], result)
@ -1595,14 +1590,12 @@ class SAMLAuthBackendTest(SocialAuthBase):
with self.settings(SOCIAL_AUTH_SAML_ENABLED_IDPS=None):
result = self.social_auth_test(account_data_dict,
subdomain='zulip', next='/user_uploads/image')
self.assertEqual(result.status_code, 302)
self.assertEqual(result.url, self.CONFIG_ERROR_URL)
self.assert_in_success_response(["Configuration error", "SAML authentication"], result)
# Test the signup path too:
result = self.social_auth_test(account_data_dict, is_signup=True,
subdomain='zulip', next='/user_uploads/image')
self.assertEqual(result.status_code, 302)
self.assertEqual(result.url, self.CONFIG_ERROR_URL)
self.assert_in_success_response(["Configuration error", "SAML authentication"], result)
def test_config_error_page(self) -> None:
with self.assertLogs(level='INFO') as info_log:
@ -1610,10 +1603,7 @@ class SAMLAuthBackendTest(SocialAuthBase):
self.assertEqual(info_log.output, [
'INFO:root:Attempted to initiate SAML authentication with wrong idp argument: None'
])
self.assertEqual(result.status_code, 302)
self.assertEqual(result.url, '/config-error/saml')
result = self.client_get(result.url)
self.assert_in_success_response(["SAML authentication"], result)
self.assert_in_success_response(["Configuration error", "SAML authentication"], result)
def test_saml_auth_works_without_private_public_keys(self) -> None:
with self.settings(SOCIAL_AUTH_SAML_SP_PUBLIC_CERT='', SOCIAL_AUTH_SAML_SP_PRIVATE_KEY=''):
@ -1862,8 +1852,7 @@ class SAMLAuthBackendTest(SocialAuthBase):
with self.assertLogs(level='INFO') as info_log:
result = self.client_get(f'/accounts/{action}/social/saml')
# Missing idp argument.
self.assertEqual(result.status_code, 302)
self.assertEqual(result.url, '/config-error/saml')
self.assert_in_success_response(["Configuration error", "SAML authentication"], result)
self.assertEqual(info_log.output, [
'INFO:root:Attempted to initiate SAML authentication with wrong idp argument: None'
])
@ -1871,11 +1860,10 @@ class SAMLAuthBackendTest(SocialAuthBase):
with self.assertLogs(level='INFO') as info_log:
result = self.client_get(f'/accounts/{action}/social/saml/nonexistent_idp')
# No such IdP is configured.
self.assertEqual(result.status_code, 302)
self.assertEqual(result.url, '/config-error/saml')
self.assertEqual(info_log.output, [
'INFO:root:Attempted to initiate SAML authentication with wrong idp argument: nonexistent_idp'
])
self.assert_in_success_response(["Configuration error", "SAML authentication"], result)
result = self.client_get(f'/accounts/{action}/social/saml/')
# No matching url pattern.
@ -3615,7 +3603,7 @@ class TestDevAuthBackend(ZulipTestCase):
data = {'direct_email': email}
with self.settings(AUTHENTICATION_BACKENDS=('zproject.backends.EmailAuthBackend',)):
response = self.client_post('/accounts/login/local/', data)
self.assertRedirects(response, reverse('config_error', kwargs={'error_category_name': 'dev'}))
self.assert_in_success_response(["Configuration error", "DevAuthBackend"], response)
def test_dev_direct_production_config_error(self) -> None:
result = self.client_get("/config-error/dev")
@ -3627,7 +3615,7 @@ class TestDevAuthBackend(ZulipTestCase):
data = {'direct_email': email}
response = self.client_post('/accounts/login/local/', data)
self.assertRedirects(response, reverse('config_error', kwargs={'error_category_name': 'dev'}))
self.assert_in_success_response(["Configuration error", "DevAuthBackend"], response)
class TestZulipRemoteUserBackend(DesktopFlowTestingLib, ZulipTestCase):
def test_start_remote_user_sso(self) -> None:
@ -3672,10 +3660,8 @@ class TestZulipRemoteUserBackend(DesktopFlowTestingLib, ZulipTestCase):
def test_login_failure(self) -> None:
email = self.example_email("hamlet")
result = self.client_get('/accounts/login/sso/', REMOTE_USER=email)
self.assertEqual(result.status_code, 302)
result = self.client_get(result["Location"])
self.assert_in_response("Authentication via the REMOTE_USER header is", result)
self.assert_in_success_response(["Configuration error", "Authentication via the REMOTE_USER header is"],
result)
self.assert_logged_in_user_id(None)
def test_login_failure_due_to_nonexisting_user(self) -> None:
@ -3695,10 +3681,8 @@ class TestZulipRemoteUserBackend(DesktopFlowTestingLib, ZulipTestCase):
def test_login_failure_due_to_missing_field(self) -> None:
with self.settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipRemoteUserBackend',)):
result = self.client_get('/accounts/login/sso/')
self.assertEqual(result.status_code, 302)
result = self.client_get(result["Location"])
self.assert_in_response("The REMOTE_USER header is not set.", result)
self.assert_in_success_response(["Configuration error", "The REMOTE_USER header is not set."],
result)
def test_login_failure_due_to_wrong_subdomain(self) -> None:
email = self.example_email("hamlet")
@ -5034,12 +5018,9 @@ class LDAPBackendTest(ZulipTestCase):
mock.patch('django_auth_ldap.backend._LDAPUser._authenticate_user_dn'), \
self.assertLogs('django_auth_ldap', 'WARNING') as warn_log:
response = self.client_post('/login/', data)
self.assertEqual(response.status_code, 302)
self.assertEqual(response.url, reverse('config_error', kwargs={'error_category_name': 'ldap'}))
response = self.client_get(response.url)
self.assert_in_response('You are trying to log in using LDAP '
'without creating an',
response)
self.assert_in_success_response(["Configuration error",
"You are trying to log in using LDAP without creating an"],
response)
self.assertEqual(warn_log.output, [
"WARNING:django_auth_ldap:('Realm is None', 1) while authenticating hamlet"
])

20
zerver/tests/test_urls.py
View File

@ -7,7 +7,7 @@ import orjson
from django.test import Client
from zerver.lib.test_classes import ZulipTestCase
from zerver.models import Stream
from zerver.models import Realm, Stream
from zproject import urls
@ -98,6 +98,24 @@ class PublicURLTest(ZulipTestCase):
self.assertEqual('success', data['result'])
self.assertEqual('ABCD', data['google_client_id'])
def test_config_error_endpoints_dev_env(self) -> None:
'''
The content of these pages is tested separately.
Here we simply sanity-check that all the URLs load
correctly.
'''
auth_types = [auth.lower() for auth in Realm.AUTHENTICATION_FLAGS]
for auth in ['azuread', 'email', 'remoteuser']: # We do not have configerror pages for AzureAD and Email.
auth_types.remove(auth)
auth_types += ['smtp', 'remoteuser/remote_user_backend_disabled',
'remoteuser/remote_user_header_missing']
urls = [f'/config-error/{auth_type}' for auth_type in auth_types]
with self.settings(DEVELOPMENT=True):
for url in urls:
response = self.client_get(url)
self.assert_in_success_response(['Configuration error'], response)
class URLResolutionTest(ZulipTestCase):
def get_callback_string(self, pattern: django.urls.resolvers.URLPattern) -> Optional[str]:
callback_str = hasattr(pattern, 'lookup_str') and 'lookup_str'

47
zerver/views/auth.py
View File

@ -20,7 +20,6 @@ from django.utils.http import is_safe_url
from django.utils.translation import ugettext as _
from django.views.decorators.csrf import csrf_exempt
from django.views.decorators.http import require_safe
from django.views.generic import TemplateView
from social_django.utils import load_backend, load_strategy
from two_factor.forms import BackupTokenForm
from two_factor.views import LoginView as BaseTwoFactorLoginView
@ -72,7 +71,6 @@ from zproject.backends import (
dev_auth_enabled,
ldap_auth_enabled,
password_auth_enabled,
redirect_to_config_error,
saml_auth_enabled,
validate_otp_params,
)
@ -356,12 +354,12 @@ def remote_user_sso(
realm = None
if not auth_enabled_helper([ZulipRemoteUserBackend.auth_backend_name], realm):
return redirect_to_config_error("remoteuser/backend_disabled")
return config_error(request, "remote_user_backend_disabled")
try:
remote_user = request.META["REMOTE_USER"]
except KeyError:
return redirect_to_config_error("remoteuser/remote_user_header_missing")
return config_error(request, "remote_user_header_missing")
# Django invokes authenticate methods by matching arguments, and this
# authentication flow will not invoke LDAP authentication because of
@ -505,29 +503,26 @@ def start_social_login(request: HttpRequest, backend: str, extra_arg: Optional[s
backend_url = reverse('social:begin', args=[backend])
extra_url_params: Dict[str, str] = {}
if backend == "saml":
result = SAMLAuthBackend.check_config()
if result is not None:
return result
if not SAMLAuthBackend.check_config():
return config_error(request, 'saml')
# This backend requires the name of the IdP (from the list of configured ones)
# to be passed as the parameter.
if not extra_arg or extra_arg not in settings.SOCIAL_AUTH_SAML_ENABLED_IDPS:
logging.info("Attempted to initiate SAML authentication with wrong idp argument: %s",
extra_arg)
return redirect_to_config_error("saml")
return config_error(request, "saml")
extra_url_params = {'idp': extra_arg}
if backend == "apple":
result = AppleAuthBackend.check_config()
if result is not None:
return result
if backend == "apple" and not AppleAuthBackend.check_config():
return config_error(request, 'apple')
# TODO: Add AzureAD also.
if backend in ["github", "google", "gitlab"]:
key_setting = "SOCIAL_AUTH_" + backend.upper() + "_KEY"
secret_setting = "SOCIAL_AUTH_" + backend.upper() + "_SECRET"
if not (getattr(settings, key_setting) and getattr(settings, secret_setting)):
return redirect_to_config_error(backend)
return config_error(request, backend)
return oauth_redirect_to_root(request, backend_url, 'social', extra_url_params=extra_url_params)
@ -537,14 +532,13 @@ def start_social_signup(request: HttpRequest, backend: str, extra_arg: Optional[
backend_url = reverse('social:begin', args=[backend])
extra_url_params: Dict[str, str] = {}
if backend == "saml":
result = SAMLAuthBackend.check_config()
if result is not None:
return result
if not SAMLAuthBackend.check_config():
return config_error(request, 'saml')
if not extra_arg or extra_arg not in settings.SOCIAL_AUTH_SAML_ENABLED_IDPS:
logging.info("Attempted to initiate SAML authentication with wrong idp argument: %s",
extra_arg)
return redirect_to_config_error("saml")
return config_error(request, "saml")
extra_url_params = {'idp': extra_arg}
return oauth_redirect_to_root(request, backend_url, 'social', is_signup=True,
extra_url_params=extra_url_params)
@ -598,9 +592,9 @@ def get_dev_users(realm: Optional[Realm]=None, extra_users_count: int=10) -> Lis
users = list(shakespearian_users) + list(extra_users)
return users
def redirect_to_misconfigured_ldap_notice(error_type: int) -> HttpResponse:
def redirect_to_misconfigured_ldap_notice(request: HttpResponse, error_type: int) -> HttpResponse:
if error_type == ZulipLDAPAuthBackend.REALM_IS_NONE_ERROR:
return redirect_to_config_error('ldap')
return config_error(request, 'ldap')
else:
raise AssertionError("Invalid error type")
@ -733,7 +727,7 @@ def login_page(
extra_context=extra_context, **kwargs)(request)
except ZulipLDAPConfigurationError as e:
assert len(e.args) > 1
return redirect_to_misconfigured_ldap_notice(e.args[1])
return redirect_to_misconfigured_ldap_notice(request, e.args[1])
if isinstance(template_response, SimpleTemplateResponse):
# Only those responses that are rendered using a template have
@ -788,13 +782,13 @@ def dev_direct_login(
if (not dev_auth_enabled()) or settings.PRODUCTION:
# This check is probably not required, since authenticate would fail without
# an enabled DevAuthBackend.
return redirect_to_config_error('dev')
return config_error(request, 'dev')
email = request.POST['direct_email']
subdomain = get_subdomain(request)
realm = get_realm(subdomain)
user_profile = authenticate(dev_auth_username=email, realm=realm)
if user_profile is None:
return redirect_to_config_error('dev')
return config_error(request, 'dev')
do_login(request, user_profile)
redirect_to = get_safe_redirect_to(next, user_profile.realm.uri)
@ -994,7 +988,7 @@ def saml_sp_metadata(request: HttpRequest, **kwargs: Any) -> HttpResponse: # no
Taken from https://python-social-auth.readthedocs.io/en/latest/backends/saml.html
"""
if not saml_auth_enabled():
return redirect_to_config_error("saml")
return config_error(request, "saml")
complete_url = reverse('social:complete', args=("saml",))
saml_backend = load_backend(load_strategy(request), "saml",
@ -1006,7 +1000,7 @@ def saml_sp_metadata(request: HttpRequest, **kwargs: Any) -> HttpResponse: # no
return HttpResponseServerError(content=', '.join(errors))
def config_error_view(request: HttpRequest, error_category_name: str) -> HttpResponse:
def config_error(request: HttpRequest, error_category_name: str) -> HttpResponse:
contexts = {
'apple': {'social_backend_name': 'apple', 'has_markdown_file': True},
'google': {'social_backend_name': 'google', 'has_markdown_file': True},
@ -1016,9 +1010,8 @@ def config_error_view(request: HttpRequest, error_category_name: str) -> HttpRes
'dev': {'error_name': 'dev_not_supported_error'},
'saml': {'social_backend_name': 'saml'},
'smtp': {'error_name': 'smtp_error'},
'backend_disabled': {'error_name': 'remoteuser_error_backend_disabled'},
'remote_user_backend_disabled': {'error_name': 'remoteuser_error_backend_disabled'},
'remote_user_header_missing': {'error_name': 'remoteuser_error_remote_user_header_missing'},
}
return TemplateView.as_view(template_name='zerver/config_error.html',
extra_context=contexts[error_category_name])(request)
return render(request, 'zerver/config_error.html', contexts[error_category_name])

15
zproject/backends.py
View File

@ -155,9 +155,6 @@ def any_social_backend_enabled(realm: Optional[Realm]=None) -> bool:
for social_auth_subclass in EXTERNAL_AUTH_METHODS]
return auth_enabled_helper(social_backend_names, realm)
def redirect_to_config_error(error_type: str) -> HttpResponseRedirect:
return HttpResponseRedirect(f"/config-error/{error_type}")
def require_email_format_usernames(realm: Optional[Realm]=None) -> bool:
if ldap_auth_enabled(realm):
if settings.LDAP_EMAIL_ATTR or settings.LDAP_APPEND_DOMAIN:
@ -1572,7 +1569,7 @@ class AppleAuthBackend(SocialAuthMixin, AppleIdAuth):
SCOPE_SEPARATOR = "%20" # https://github.com/python-social-auth/social-core/issues/470
@classmethod
def check_config(cls) -> Optional[HttpResponse]:
def check_config(cls) -> bool:
obligatory_apple_settings_list = [
settings.SOCIAL_AUTH_APPLE_TEAM,
settings.SOCIAL_AUTH_APPLE_SERVICES_ID,
@ -1580,9 +1577,9 @@ class AppleAuthBackend(SocialAuthMixin, AppleIdAuth):
settings.SOCIAL_AUTH_APPLE_SECRET,
]
if any(not setting for setting in obligatory_apple_settings_list):
return redirect_to_config_error("apple")
return False
return None
return True
def is_native_flow(self) -> bool:
return self.strategy.request_data().get('native_flow', False)
@ -1955,7 +1952,7 @@ class SAMLAuthBackend(SocialAuthMixin, SAMLAuth):
return True
@classmethod
def check_config(cls) -> Optional[HttpResponse]:
def check_config(cls) -> bool:
obligatory_saml_settings_list = [
settings.SOCIAL_AUTH_SAML_SP_ENTITY_ID,
settings.SOCIAL_AUTH_SAML_ORG_INFO,
@ -1964,9 +1961,9 @@ class SAMLAuthBackend(SocialAuthMixin, SAMLAuth):
settings.SOCIAL_AUTH_SAML_ENABLED_IDPS,
]
if any(not setting for setting in obligatory_saml_settings_list):
return redirect_to_config_error("saml")
return False
return None
return True
@classmethod
def dict_representation(cls, realm: Optional[Realm]=None) -> List[ExternalAuthMethodDictT]:

7
zproject/dev_urls.py
View File

@ -9,7 +9,7 @@ from django.urls import path
from django.views.generic import TemplateView
from django.views.static import serve
from zerver.views.auth import login_page
from zerver.views.auth import config_error, login_page
from zerver.views.development.email_log import clear_emails, email_page, generate_all_emails
from zerver.views.development.integrations import (
check_send_webhook_fixture_message,
@ -72,6 +72,11 @@ urls = [
send_all_webhook_fixture_messages),
path('devtools/integrations/<integration_name>/fixtures',
get_fixtures),
path('config-error/<error_category_name>', config_error,
name='config_error'),
path('config-error/remoteuser/<error_category_name>',
config_error),
]
# Serve static assets via the Django server

5
zproject/urls.py
View File

@ -26,7 +26,6 @@ from zerver.views.auth import (
api_fetch_api_key,
api_fetch_google_client_id,
api_get_server_settings,
config_error_view,
dev_direct_login,
json_fetch_api_key,
log_into_subdomain,
@ -747,10 +746,6 @@ i18n_urls = [
# Terms of Service and privacy pages.
path('terms/', terms_view),
path('privacy/', privacy_view),
path('config-error/<error_category_name>', config_error_view,
name='config_error'),
path('config-error/remoteuser/<error_category_name>',
config_error_view),
]
# Make a copy of i18n_urls so that they appear without prefix for english