From e7353387668576ebb215f422c17b9fd541bf6dbe Mon Sep 17 00:00:00 2001 From: Hanne Moa Date: Wed, 1 Nov 2023 15:10:58 +0100 Subject: [PATCH 1/8] Split up web.auth into multiple files --- python/nav/web/{auth.py => auth/__init__.py} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename python/nav/web/{auth.py => auth/__init__.py} (100%) diff --git a/python/nav/web/auth.py b/python/nav/web/auth/__init__.py similarity index 100% rename from python/nav/web/auth.py rename to python/nav/web/auth/__init__.py From b64a3abc1967f3716837d11284a92f77635d43f1 Mon Sep 17 00:00:00 2001 From: Hanne Moa Date: Wed, 1 Nov 2023 15:24:32 +0100 Subject: [PATCH 2/8] Move remote user helpers to separate file --- python/nav/web/auth/__init__.py | 179 +------------------------- python/nav/web/auth/remote_user.py | 193 +++++++++++++++++++++++++++++ 2 files changed, 199 insertions(+), 173 deletions(-) create mode 100644 python/nav/web/auth/remote_user.py diff --git a/python/nav/web/auth/__init__.py b/python/nav/web/auth/__init__.py index e3332e61da..c635194fb5 100644 --- a/python/nav/web/auth/__init__.py +++ b/python/nav/web/auth/__init__.py @@ -21,8 +21,6 @@ from datetime import datetime import logging -from os.path import join -import os from urllib import parse @@ -36,27 +34,17 @@ except ImportError: # Django <= 1.9 MiddlewareMixin = object -try: - # Python 3.6+ - import secrets - - def fake_password(length): - return secrets.token_urlsafe(length) - -except ImportError: - from random import choice - import string - - def fake_password(length): - symbols = string.ascii_letters + string.punctuation + string.digits - return u"".join(choice(symbols) for i in range(length)) - from nav.auditlog.models import LogEntry -from nav.config import NAVConfigParser from nav.django.utils import is_admin, get_account from nav.models.profiles import Account, AccountGroup from nav.web import ldapauth +from nav.web.auth.remote_user import ( + authenticate_remote_user, + get_remote_loginurl, + get_remote_logouturl, + get_remote_username, +) _logger = logging.getLogger(__name__) @@ -75,21 +63,6 @@ def fake_password(length): LOGOUT_URL = '/index/logout/' -class RemoteUserConfigParser(NAVConfigParser): - DEFAULT_CONFIG_FILES = [join('webfront', 'webfront.conf')] - DEFAULT_CONFIG = u""" -[remote-user] -enabled=no -login-url= -logout-url= -varname=REMOTE_USER -workaround=none -""" - - -_config = RemoteUserConfigParser() - - def _set_account(request, account): request.session[ACCOUNT_ID_VAR] = account.id request.account = account @@ -166,48 +139,6 @@ def _handle_ldap_admin_status(ldap_user, nav_account): nav_account.groups.remove(admin_group) -def authenticate_remote_user(request): - """Authenticate username from http header REMOTE_USER - - Returns: - - :return: If the user was authenticated, an account. - If the user was blocked from logging in, False. - Otherwise, None. - :rtype: Account, False, None - """ - username = get_remote_username(request) - if not username: - return None - - # We now have a username-ish - - try: - account = Account.objects.get(login=username) - except Account.DoesNotExist: - # Store the remote user in the database and return the new account - account = Account(login=username, name=username, ext_sync='REMOTE_USER') - account.set_password(fake_password(32)) - account.save() - _logger.info("Created user %s from header REMOTE_USER", account.login) - template = 'Account "{actor}" created due to REMOTE_USER HTTP header' - LogEntry.add_log_entry( - account, 'create-account', template=template, subsystem='auth' - ) - return account - - # Bail out! Potentially evil user - if account.locked: - _logger.info("Locked user %s tried to log in", account.login) - template = 'Account "{actor}" was prevented from logging in: blocked' - LogEntry.add_log_entry( - account, 'login-prevent', template=template, subsystem='auth' - ) - return False - - return account - - def get_login_url(request): """Calculate which login_url to use""" path = parse.quote(request.get_full_path()) @@ -227,104 +158,6 @@ def get_logout_url(request): return remote_logouturl if remote_logouturl else LOGOUT_URL -def get_remote_loginurl(request): - """Return a url (if set) to log in to/via a remote service - - :return: Either a string with an url, or None. - :rtype: str, None - """ - return get_remote_url(request, 'login-url') - - -def get_remote_logouturl(request): - """Return a url (if set) to log out to/via a remote service - - :return: Either a string with an url, or None. - :rtype: str, None - """ - return get_remote_url(request, 'logout-url') - - -def get_remote_url(request, urltype): - """Return a url (if set) to a remote service for REMOTE_USER purposes - - :return: Either a string with an url, or None. - :rtype: str, None - """ - remote_url = None - try: - if not _config.getboolean('remote-user', 'enabled'): - return None - remote_url = _config.get('remote-user', urltype) - except ValueError: - return None - if remote_url: - nexthop = request.build_absolute_uri(request.get_full_path()) - remote_url = remote_url.format(nexthop) - return remote_url - - -def get_remote_username(request): - """Return the username in REMOTE_USER if set and enabled - - :return: The username in REMOTE_USER if any, or None. - :rtype: str, None - """ - try: - if not _config.getboolean('remote-user', 'enabled'): - return None - except ValueError: - return None - - if not request: - return None - - workaround = 'none' - try: - workaround_config = _config.get('remote-user', 'workaround') - except ValueError: - pass - else: - if workaround_config in REMOTE_USER_WORKAROUNDS: - workaround = workaround_config - - username = REMOTE_USER_WORKAROUNDS[workaround](request) - - if not username: - return None - - return username - - -def _get_remote_user_varname(): - varname = 'REMOTE_USER' - try: - varname = _config.get('remote-user', 'varname') - except ValueError: - pass - return varname - - -def _workaround_default(request): - varname = _get_remote_user_varname() - username = request.META.get(varname, '').strip() - return username - - -def _workaround_feide_oidc(request): - varname = _get_remote_user_varname() - username = request.META.get(varname, '').strip() - if ':' in username: - username = username.split(':', 1)[1] - return username - - -REMOTE_USER_WORKAROUNDS = { - 'none': _workaround_default, - 'feide-oidc': _workaround_feide_oidc, -} - - # Middleware diff --git a/python/nav/web/auth/remote_user.py b/python/nav/web/auth/remote_user.py new file mode 100644 index 0000000000..0a024fff9e --- /dev/null +++ b/python/nav/web/auth/remote_user.py @@ -0,0 +1,193 @@ +# Copyright (C) 2010, 2011, 2013, 2019 Uninett AS +# Copyright (C) 2022 Sikt +# +# This file is part of Network Administration Visualized (NAV). +# +# NAV is free software: you can redistribute it and/or modify it under +# the terms of the GNU General Public License version 3 as published by +# the Free Software Foundation. +# +# This program is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +# more details. You should have received a copy of the GNU General Public +# License along with NAV. If not, see . +# + +import logging +from os.path import join + +from nav.auditlog.models import LogEntry +from nav.config import NAVConfigParser +from nav.models.profiles import Account + +try: + # Python 3.6+ + import secrets + + def fake_password(length): + return secrets.token_urlsafe(length) + +except ImportError: + from random import choice + import string + + def fake_password(length): + symbols = string.ascii_letters + string.punctuation + string.digits + return u"".join(choice(symbols) for i in range(length)) + + +class RemoteUserConfigParser(NAVConfigParser): + DEFAULT_CONFIG_FILES = [join('webfront', 'webfront.conf')] + DEFAULT_CONFIG = u""" +[remote-user] +enabled=no +login-url= +logout-url= +varname=REMOTE_USER +workaround=none +""" + + +_logger = logging.getLogger(__name__) +_config = RemoteUserConfigParser() + + +def authenticate_remote_user(request): + """Authenticate username from http header REMOTE_USER + + Returns: + + :return: If the user was authenticated, an account. + If the user was blocked from logging in, False. + Otherwise, None. + :rtype: Account, False, None + """ + username = get_remote_username(request) + if not username: + return None + + # We now have a username-ish + + try: + account = Account.objects.get(login=username) + except Account.DoesNotExist: + # Store the remote user in the database and return the new account + account = Account(login=username, name=username, ext_sync='REMOTE_USER') + account.set_password(fake_password(32)) + account.save() + _logger.info("Created user %s from header REMOTE_USER", account.login) + template = 'Account "{actor}" created due to REMOTE_USER HTTP header' + LogEntry.add_log_entry( + account, 'create-account', template=template, subsystem='auth' + ) + return account + + # Bail out! Potentially evil user + if account.locked: + _logger.info("Locked user %s tried to log in", account.login) + template = 'Account "{actor}" was prevented from logging in: blocked' + LogEntry.add_log_entry( + account, 'login-prevent', template=template, subsystem='auth' + ) + return False + + return account + + +def get_remote_loginurl(request): + """Return a url (if set) to log in to/via a remote service + + :return: Either a string with an url, or None. + :rtype: str, None + """ + return get_remote_url(request, 'login-url') + + +def get_remote_logouturl(request): + """Return a url (if set) to log out to/via a remote service + + :return: Either a string with an url, or None. + :rtype: str, None + """ + return get_remote_url(request, 'logout-url') + + +def get_remote_url(request, urltype): + """Return a url (if set) to a remote service for REMOTE_USER purposes + + :return: Either a string with an url, or None. + :rtype: str, None + """ + remote_url = None + try: + if not _config.getboolean('remote-user', 'enabled'): + return None + remote_url = _config.get('remote-user', urltype) + except ValueError: + return None + if remote_url: + nexthop = request.build_absolute_uri(request.get_full_path()) + remote_url = remote_url.format(nexthop) + return remote_url + + +def get_remote_username(request): + """Return the username in REMOTE_USER if set and enabled + + :return: The username in REMOTE_USER if any, or None. + :rtype: str, None + """ + try: + if not _config.getboolean('remote-user', 'enabled'): + return None + except ValueError: + return None + + if not request: + return None + + workaround = 'none' + try: + workaround_config = _config.get('remote-user', 'workaround') + except ValueError: + pass + else: + if workaround_config in REMOTE_USER_WORKAROUNDS: + workaround = workaround_config + + username = REMOTE_USER_WORKAROUNDS[workaround](request) + + if not username: + return None + + return username + + +def _get_remote_user_varname(): + varname = 'REMOTE_USER' + try: + varname = _config.get('remote-user', 'varname') + except ValueError: + pass + return varname + + +def _workaround_default(request): + varname = _get_remote_user_varname() + username = request.META.get(varname, '').strip() + return username + + +def _workaround_feide_oidc(request): + varname = _get_remote_user_varname() + username = request.META.get(varname, '').strip() + if ':' in username: + username = username.split(':', 1)[1] + return username + + +REMOTE_USER_WORKAROUNDS = { + 'none': _workaround_default, + 'feide-oidc': _workaround_feide_oidc, +} From 6e44fe7bcfcf486561d43b048c1c56ac7bac970b Mon Sep 17 00:00:00 2001 From: Hanne Moa Date: Thu, 2 Nov 2023 07:42:44 +0100 Subject: [PATCH 3/8] Move ldapauth.py into nav/auth/ --- python/nav/web/auth/__init__.py | 12 +++--- python/nav/web/{ldapauth.py => auth/ldap.py} | 2 +- python/nav/web/webfront/views.py | 7 ++-- tests/unittests/general/webfront_test.py | 42 ++++++++++---------- tests/unittests/web/ldapauth_test.py | 8 ++-- 5 files changed, 36 insertions(+), 35 deletions(-) rename python/nav/web/{ldapauth.py => auth/ldap.py} (99%) diff --git a/python/nav/web/auth/__init__.py b/python/nav/web/auth/__init__.py index c635194fb5..4b90be3ffc 100644 --- a/python/nav/web/auth/__init__.py +++ b/python/nav/web/auth/__init__.py @@ -38,7 +38,7 @@ from nav.auditlog.models import LogEntry from nav.django.utils import is_admin, get_account from nav.models.profiles import Account, AccountGroup -from nav.web import ldapauth +from nav.web.auth import ldap from nav.web.auth.remote_user import ( authenticate_remote_user, get_remote_loginurl, @@ -83,8 +83,8 @@ def authenticate(username, password): try: account = Account.objects.get(login__iexact=username) except Account.DoesNotExist: - if ldapauth.available: - user = ldapauth.authenticate(username, password) + if ldap.available: + user = ldap.authenticate(username, password) # If we authenticated, store the user in database. if user: account = Account( @@ -102,13 +102,13 @@ def authenticate(username, password): if ( account and account.ext_sync == 'ldap' - and ldapauth.available + and ldap.available and not auth and not account.locked ): try: - auth = ldapauth.authenticate(username, password) - except ldapauth.NoAnswerError: + auth = ldap.authenticate(username, password) + except ldap.NoAnswerError: # Fallback to stored password if ldap is unavailable auth = False else: diff --git a/python/nav/web/ldapauth.py b/python/nav/web/auth/ldap.py similarity index 99% rename from python/nav/web/ldapauth.py rename to python/nav/web/auth/ldap.py index 19fcd99907..344608cd05 100644 --- a/python/nav/web/ldapauth.py +++ b/python/nav/web/auth/ldap.py @@ -25,7 +25,7 @@ import nav.errors from nav.config import NAVConfigParser -_logger = logging.getLogger("nav.web.ldapauth") +_logger = logging.getLogger(__name__) # Set default config params and read rest from file diff --git a/python/nav/web/webfront/views.py b/python/nav/web/webfront/views.py index 87ba364e56..f74be34954 100644 --- a/python/nav/web/webfront/views.py +++ b/python/nav/web/webfront/views.py @@ -39,7 +39,8 @@ from nav.models.profiles import NavbarLink, AccountDashboard, AccountNavlet from nav.web.auth import ACCOUNT_ID_VAR from nav.web.auth import logout as auth_logout -from nav.web import ldapauth, auth +from nav.web import auth +from nav.web.auth import ldap from nav.web.utils import require_param from nav.web.webfront.utils import quick_read, tool_list from nav.web.webfront.forms import ( @@ -222,7 +223,7 @@ def do_login(request): try: account = auth.authenticate(username, password) - except ldapauth.Error as error: + except ldap.Error as error: errors.append('Error while talking to LDAP:\n%s' % error) else: if account: @@ -233,7 +234,7 @@ def do_login(request): try: request.session[ACCOUNT_ID_VAR] = account.id request.account = account - except ldapauth.Error as error: + except ldap.Error as error: errors.append('Error while talking to LDAP:\n%s' % error) else: _logger.info("%s successfully logged in", account.login) diff --git a/tests/unittests/general/webfront_test.py b/tests/unittests/general/webfront_test.py index 16b1593895..470a162d1e 100644 --- a/tests/unittests/general/webfront_test.py +++ b/tests/unittests/general/webfront_test.py @@ -4,7 +4,7 @@ import pytest -import nav.web.ldapauth +import nav.web.auth.ldap from nav.web import auth LDAP_ACCOUNT = auth.Account(login='knight', ext_sync='ldap', password='shrubbery') @@ -28,23 +28,23 @@ class TestLdapAuthenticate(object): def test_authenticate_should_return_account_when_ldap_says_yes(self): ldap_user = Mock() ldap_user.is_admin.return_value = None # mock to avoid database access - with patch("nav.web.ldapauth.available", new=True): - with patch("nav.web.ldapauth.authenticate", return_value=ldap_user): + with patch("nav.web.auth.ldap.available", new=True): + with patch("nav.web.auth.ldap.authenticate", return_value=ldap_user): assert auth.authenticate('knight', 'shrubbery') == LDAP_ACCOUNT def test_authenticate_should_return_false_when_ldap_says_no(self): - with patch("nav.web.ldapauth.available", new=True): - with patch("nav.web.ldapauth.authenticate", return_value=False): + with patch("nav.web.auth.ldap.available", new=True): + with patch("nav.web.auth.ldap.authenticate", return_value=False): assert not auth.authenticate('knight', 'shrubbery') def test_authenticate_should_fallback_when_ldap_is_disabled(self): - with patch("nav.web.ldapauth.available", new=False): + with patch("nav.web.auth.ldap.available", new=False): assert auth.authenticate('knight', 'shrubbery') == LDAP_ACCOUNT @patch("nav.web.auth.Account.save", new=MagicMock(return_value=True)) @patch("nav.web.auth.Account.objects.get", new=MagicMock(return_value=PLAIN_ACCOUNT)) -@patch("nav.web.ldapauth.available", new=False) +@patch("nav.web.auth.ldap.available", new=False) class TestNormalAuthenticate(object): def test_authenticate_should_return_account_when_password_is_ok(self): with patch("nav.web.auth.Account.check_password", return_value=True): @@ -179,7 +179,7 @@ def test_remote_user_set(self): class TestLdapUser(object): @patch.dict( - "nav.web.ldapauth._config._sections", + "nav.web.auth.ldap._config._sections", { 'ldap': { '__name__': 'ldap', @@ -201,12 +201,12 @@ def test_search_result_with_referrals_should_be_considered_empty(self): ] } ) - u = nav.web.ldapauth.LDAPUser("zaphod", conn) - with pytest.raises(nav.web.ldapauth.UserNotFound): + u = nav.web.auth.ldap.LDAPUser("zaphod", conn) + with pytest.raises(nav.web.auth.ldap.UserNotFound): u.search_dn() @patch.dict( - "nav.web.ldapauth._config._sections", + "nav.web.auth.ldap._config._sections", { 'ldap': { '__name__': 'ldap', @@ -228,11 +228,11 @@ def test_non_ascii_password_should_work(self): ), } ) - u = nav.web.ldapauth.LDAPUser(u"zaphod", conn) + u = nav.web.auth.ldap.LDAPUser(u"zaphod", conn) u.bind(u"æøå") @patch.dict( - "nav.web.ldapauth._config._sections", + "nav.web.auth.ldap._config._sections", { 'ldap': { '__name__': 'ldap', @@ -257,12 +257,12 @@ def fake_search(base, scope, filtr): 'search_s.side_effect': fake_search, } ) - u = nav.web.ldapauth.LDAPUser(u"Ægir", conn) + u = nav.web.auth.ldap.LDAPUser(u"Ægir", conn) u.is_group_member('cn=noc-operators,cn=groups,dc=example,dc=com') @patch.dict( - "nav.web.ldapauth._config._sections", + "nav.web.auth.ldap._config._sections", { 'ldap': { '__name__': 'ldap', @@ -278,24 +278,24 @@ def fake_search(base, scope, filtr): ) class TestLdapEntitlements(object): def test_required_entitlement_should_be_verified(self, user_zaphod): - u = nav.web.ldapauth.LDAPUser("zaphod", user_zaphod) + u = nav.web.auth.ldap.LDAPUser("zaphod", user_zaphod) assert u.has_entitlement('president') def test_missing_entitlement_should_not_be_verified(self, user_marvin): - u = nav.web.ldapauth.LDAPUser("marvin", user_marvin) + u = nav.web.auth.ldap.LDAPUser("marvin", user_marvin) assert not u.has_entitlement('president') def test_admin_entitlement_should_be_verified(self, user_zaphod): - u = nav.web.ldapauth.LDAPUser("zaphod", user_zaphod) + u = nav.web.auth.ldap.LDAPUser("zaphod", user_zaphod) assert u.is_admin() def test_missing_admin_entitlement_should_be_verified(self, user_marvin): - u = nav.web.ldapauth.LDAPUser("marvin", user_marvin) + u = nav.web.auth.ldap.LDAPUser("marvin", user_marvin) assert not u.is_admin() @patch.dict( - "nav.web.ldapauth._config._sections", + "nav.web.auth.ldap._config._sections", { 'ldap': { '__name__': 'ldap', @@ -310,7 +310,7 @@ def test_missing_admin_entitlement_should_be_verified(self, user_marvin): }, ) def test_no_admin_entitlement_option_should_make_no_admin_decision(user_zaphod): - u = nav.web.ldapauth.LDAPUser("zaphod", user_zaphod) + u = nav.web.auth.ldap.LDAPUser("zaphod", user_zaphod) assert u.is_admin() is None diff --git a/tests/unittests/web/ldapauth_test.py b/tests/unittests/web/ldapauth_test.py index b4f78e36a5..2655f1166d 100644 --- a/tests/unittests/web/ldapauth_test.py +++ b/tests/unittests/web/ldapauth_test.py @@ -1,5 +1,5 @@ from nav.config import NAVConfigParser -from nav.web.ldapauth import LDAPUser, open_ldap +from nav.web.auth.ldap import LDAPUser, open_ldap from mock import Mock, patch @@ -16,7 +16,7 @@ class LdapTestConfig(NAVConfigParser): """ -@patch('nav.web.ldapauth._config', LdapTestConfig()) +@patch('nav.web.auth.ldap._config', LdapTestConfig()) def test_ldapuser_search_dn_decode_regression(): """Verifies that LDAPUser.search_dn() returns user's DN untouched""" connection = Mock() @@ -47,7 +47,7 @@ class LdapOpenTestConfig(NAVConfigParser): """ -@patch('nav.web.ldapauth._config', LdapOpenTestConfig()) +@patch('nav.web.auth.ldap._config', LdapOpenTestConfig()) def test_open_ldap_should_run_without_error(): with patch('ldap.initialize') as initialize: assert open_ldap() @@ -65,7 +65,7 @@ class LdapOpenTestInvalidEncryptionConfig(NAVConfigParser): """ -@patch('nav.web.ldapauth._config', LdapOpenTestInvalidEncryptionConfig()) +@patch('nav.web.auth.ldap._config', LdapOpenTestInvalidEncryptionConfig()) def test_when_encryption_setting_is_invalid_open_ldap_should_run_without_encryption(): with patch('ldap.initialize') as initialize: assert open_ldap() From ab96b0f7f78194ee0a62c08720f29ceb638c819b Mon Sep 17 00:00:00 2001 From: Hanne Moa Date: Thu, 2 Nov 2023 07:51:38 +0100 Subject: [PATCH 4/8] Make calling of auth functions consistent --- python/nav/web/auth/__init__.py | 18 +++----- python/nav/web/auth/remote_user.py | 13 +++--- tests/unittests/general/webfront_test.py | 52 ++++++++++++------------ 3 files changed, 41 insertions(+), 42 deletions(-) diff --git a/python/nav/web/auth/__init__.py b/python/nav/web/auth/__init__.py index 4b90be3ffc..167b4af3d8 100644 --- a/python/nav/web/auth/__init__.py +++ b/python/nav/web/auth/__init__.py @@ -38,13 +38,7 @@ from nav.auditlog.models import LogEntry from nav.django.utils import is_admin, get_account from nav.models.profiles import Account, AccountGroup -from nav.web.auth import ldap -from nav.web.auth.remote_user import ( - authenticate_remote_user, - get_remote_loginurl, - get_remote_logouturl, - get_remote_username, -) +from nav.web.auth import ldap, remote_user _logger = logging.getLogger(__name__) @@ -146,13 +140,13 @@ def get_login_url(request): default_new_url = LOGIN_URL else: default_new_url = '{0}?origin={1}&noaccess'.format(LOGIN_URL, path) - remote_loginurl = get_remote_loginurl(request) + remote_loginurl = remote_user.get_loginurl(request) return remote_loginurl if remote_loginurl else default_new_url def get_logout_url(request): """Calculate which logout_url to use""" - remote_logouturl = get_remote_logouturl(request) + remote_logouturl = remote_user.get_logouturl(request) if remote_logouturl and remote_logouturl.endswith('='): remote_logouturl += request.build_absolute_uri(LOGOUT_URL) return remote_logouturl if remote_logouturl else LOGOUT_URL @@ -181,7 +175,7 @@ def process_request(self, request): request.get_full_path(), ) - remote_username = get_remote_username(request) + remote_username = remote_user.get_username(request) if remote_username: _logger.debug( ('AuthenticationMiddleware: ' '(REMOTE_USER: "%s") from "%s"'), @@ -232,10 +226,10 @@ def login_remote_user(request): :return: Account for remote user, or None :rtype: Account, None """ - remote_username = get_remote_username(request) + remote_username = remote_user.get_username(request) if remote_username: # Get or create an account from the REMOTE_USER http header - account = authenticate_remote_user(request) + account = remote_user.authenticate(request) if account: request.session[ACCOUNT_ID_VAR] = account.id request.account = account diff --git a/python/nav/web/auth/remote_user.py b/python/nav/web/auth/remote_user.py index 0a024fff9e..2631787cc1 100644 --- a/python/nav/web/auth/remote_user.py +++ b/python/nav/web/auth/remote_user.py @@ -37,6 +37,9 @@ def fake_password(length): return u"".join(choice(symbols) for i in range(length)) +__all__ = [] + + class RemoteUserConfigParser(NAVConfigParser): DEFAULT_CONFIG_FILES = [join('webfront', 'webfront.conf')] DEFAULT_CONFIG = u""" @@ -53,7 +56,7 @@ class RemoteUserConfigParser(NAVConfigParser): _config = RemoteUserConfigParser() -def authenticate_remote_user(request): +def authenticate(request): """Authenticate username from http header REMOTE_USER Returns: @@ -63,7 +66,7 @@ def authenticate_remote_user(request): Otherwise, None. :rtype: Account, False, None """ - username = get_remote_username(request) + username = get_username(request) if not username: return None @@ -95,7 +98,7 @@ def authenticate_remote_user(request): return account -def get_remote_loginurl(request): +def get_loginurl(request): """Return a url (if set) to log in to/via a remote service :return: Either a string with an url, or None. @@ -104,7 +107,7 @@ def get_remote_loginurl(request): return get_remote_url(request, 'login-url') -def get_remote_logouturl(request): +def get_logouturl(request): """Return a url (if set) to log out to/via a remote service :return: Either a string with an url, or None. @@ -132,7 +135,7 @@ def get_remote_url(request, urltype): return remote_url -def get_remote_username(request): +def get_username(request): """Return the username in REMOTE_USER if set and enabled :return: The username in REMOTE_USER if any, or None. diff --git a/tests/unittests/general/webfront_test.py b/tests/unittests/general/webfront_test.py index 470a162d1e..2174e2ee39 100644 --- a/tests/unittests/general/webfront_test.py +++ b/tests/unittests/general/webfront_test.py @@ -6,6 +6,8 @@ import nav.web.auth.ldap from nav.web import auth +from nav.web.auth import remote_user +from nav.web.auth.utils import ACCOUNT_ID_VAR LDAP_ACCOUNT = auth.Account(login='knight', ext_sync='ldap', password='shrubbery') PLAIN_ACCOUNT = auth.Account(login='knight', password='shrubbery') @@ -60,30 +62,30 @@ def test_authenticate_remote_user_should_return_account_if_header_set(self): r = RequestFactory() request = r.get('/') request.META['REMOTE_USER'] = 'knight' - with patch("nav.web.auth._config.getboolean", return_value=True): + with patch("nav.web.auth.remote_user._config.getboolean", return_value=True): with patch( "nav.web.auth.Account.objects.get", new=MagicMock(return_value=REMOTE_USER_ACCOUNT), ): - assert auth.authenticate_remote_user(request) == REMOTE_USER_ACCOUNT + assert remote_user.authenticate(request) == REMOTE_USER_ACCOUNT def test_authenticate_remote_user_should_return_none_if_header_not_set(self): r = RequestFactory() request = r.get('/') - with patch("nav.web.auth._config.getboolean", return_value=True): - assert auth.authenticate_remote_user(request) == None + with patch("nav.web.auth.remote_user._config.getboolean", return_value=True): + assert remote_user.authenticate(request) == None def test_authenticate_remote_user_should_return_false_if_account_locked(self): r = RequestFactory() request = r.get('/') request.META['REMOTE_USER'] = 'knight' - with patch("nav.web.auth._config.getboolean", return_value=True): + with patch("nav.web.auth.remote_user._config.getboolean", return_value=True): with patch( "nav.web.auth.Account.objects.get", return_value=REMOTE_USER_ACCOUNT ): with patch("nav.web.auth.LogEntry.add_log_entry"): with patch("nav.web.auth.Account.locked", return_value=True): - assert auth.authenticate_remote_user(request) == False + assert remote_user.authenticate(request) == False class TestGetStandardUrls(object): @@ -94,12 +96,12 @@ def test_get_login_url_default(self): result = auth.get_login_url(request) assert result.startswith(raw_login_url) - def test_get_login_url_remote_login_url(self): + def test_get_remote_login_url(self): r = RequestFactory() request = r.get('/') request.META['REMOTE_USER'] = 'knight' - with patch("nav.web.auth._config.getboolean", return_value=True): - with patch("nav.web.auth._config.get", return_value='foo'): + with patch("nav.web.auth.remote_user._config.getboolean", return_value=True): + with patch("nav.web.auth.remote_user._config.get", return_value='foo'): result = auth.get_login_url(request) assert result == 'foo' @@ -109,42 +111,42 @@ def test_get_logout_url_default(self): result = auth.get_logout_url(request) assert result == auth.LOGOUT_URL - def test_get_logout_url_remote_logout_url(self): + def test_get_remote_logout_url(self): r = RequestFactory() request = r.get('/') request.META['REMOTE_USER'] = 'knight' - with patch("nav.web.auth._config.getboolean", return_value=True): - with patch("nav.web.auth._config.get", return_value='foo'): + with patch("nav.web.auth.remote_user._config.getboolean", return_value=True): + with patch("nav.web.auth.remote_user._config.get", return_value='foo'): result = auth.get_logout_url(request) assert result == 'foo' class TestGetRemoteUsername(object): def test_no_request(self): - with patch("nav.web.auth._config.getboolean", return_value=False): - result = auth.get_remote_username(None) + with patch("nav.web.auth.remote_user._config.getboolean", return_value=False): + result = remote_user.get_username(None) assert result is None def test_not_enabled(self): r = RequestFactory() request = r.get('/') - with patch("nav.web.auth._config.getboolean", return_value=False): - result = auth.get_remote_username(request) + with patch("nav.web.auth.remote_user._config.getboolean", return_value=False): + result = remote_user.get_username(request) assert result is None def test_enabled_but_remote_user_unset(self): r = RequestFactory() request = r.get('/') - with patch("nav.web.auth._config.getboolean", return_value=True): - result = auth.get_remote_username(request) + with patch("nav.web.auth.remote_user._config.getboolean", return_value=True): + result = remote_user.get_username(request) assert result is None def test_enabled_and_remote_user_set(self): r = RequestFactory() request = r.get('/') request.META['REMOTE_USER'] = 'knight' - with patch("nav.web.auth._config.getboolean", return_value=True): - result = auth.get_remote_username(request) + with patch("nav.web.auth.remote_user._config.getboolean", return_value=True): + result = remote_user.get_username(request) assert result == 'knight' @@ -153,8 +155,8 @@ def test_remote_user_unset(self): r = RequestFactory() request = r.get('/') request.session = FakeSession() - with patch("nav.web.auth.get_remote_username", return_value=False): - auth.login_remote_user(request) + with patch("nav.web.auth.remote_user.get_username", return_value=False): + remote_user.login(request) assert not getattr(request, 'account', False) assert auth.ACCOUNT_ID_VAR not in request.session @@ -162,12 +164,12 @@ def test_remote_user_set(self): r = RequestFactory() request = r.get('/') request.session = FakeSession() - with patch("nav.web.auth.get_remote_username", return_value=True): + with patch("nav.web.auth.remote_user.get_username", return_value=True): with patch( - "nav.web.auth.authenticate_remote_user", + "nav.web.auth.remote_user.authenticate", return_value=REMOTE_USER_ACCOUNT, ): - auth.login_remote_user(request) + remote_user.login(request) assert hasattr(request, 'account') assert request.account == REMOTE_USER_ACCOUNT assert auth.ACCOUNT_ID_VAR in request.session From 9592d18dce645329dc9f70f6d8e9f477c9342f70 Mon Sep 17 00:00:00 2001 From: Hanne Moa Date: Thu, 2 Nov 2023 08:14:40 +0100 Subject: [PATCH 5/8] Move sudo-functionality to sudo.py --- python/nav/django/context_processors.py | 3 +- python/nav/web/auth/__init__.py | 115 +----------------- python/nav/web/auth/remote_user.py | 18 +++ python/nav/web/auth/sudo.py | 108 ++++++++++++++++ python/nav/web/auth/utils.py | 30 +++++ python/nav/web/navlets/__init__.py | 2 +- python/nav/web/webfront/views.py | 2 +- .../unittests/general/web_middleware_test.py | 12 +- tests/unittests/general/webfront_test.py | 7 +- 9 files changed, 174 insertions(+), 123 deletions(-) create mode 100644 python/nav/web/auth/sudo.py create mode 100644 python/nav/web/auth/utils.py diff --git a/python/nav/django/context_processors.py b/python/nav/django/context_processors.py index a8cb22a0e9..e8c6990a9c 100644 --- a/python/nav/django/context_processors.py +++ b/python/nav/django/context_processors.py @@ -24,7 +24,8 @@ from django.conf import settings from nav.config import find_config_file -from nav.web.auth import get_sudoer, get_login_url, get_logout_url +from nav.web.auth import get_login_url, get_logout_url +from nav.web.auth.sudo import get_sudoer from nav.django.utils import get_account, is_admin from nav.web.message import Messages from nav.web.webfront.utils import tool_list, quick_read, split_tools diff --git a/python/nav/web/auth/__init__.py b/python/nav/web/auth/__init__.py index 167b4af3d8..d713183da9 100644 --- a/python/nav/web/auth/__init__.py +++ b/python/nav/web/auth/__init__.py @@ -36,17 +36,15 @@ from nav.auditlog.models import LogEntry -from nav.django.utils import is_admin, get_account from nav.models.profiles import Account, AccountGroup from nav.web.auth import ldap, remote_user +from nav.web.auth.sudo import desudo, get_sudoer +from nav.web.auth.utils import _set_account, ACCOUNT_ID_VAR _logger = logging.getLogger(__name__) -ACCOUNT_ID_VAR = 'account_id' -SUDOER_ID_VAR = 'sudoer' - # This may seem like redundant information, but it seems django's reverse # will hang under some usages of these middleware classes - so until we figure # out what's going on, we'll hardcode this here. @@ -57,13 +55,6 @@ LOGOUT_URL = '/index/logout/' -def _set_account(request, account): - request.session[ACCOUNT_ID_VAR] = account.id - request.account = account - _logger.debug('Set active account to "%s"', account.login) - request.session.save() - - def authenticate(username, password): """Authenticate username and password against database. Returns account object if user was authenticated, else None. @@ -184,7 +175,7 @@ def process_request(self, request): ) if logged_in.id == Account.DEFAULT_ACCOUNT: # Switch from anonymous to REMOTE_USER - login_remote_user(request) + remote_user.login(request) elif remote_username != logged_in.login: # REMOTE_USER has changed, logout logout(request, sudo=bool(sudo_operator)) @@ -220,23 +211,6 @@ def ensure_account(request): _set_account(request, account) -def login_remote_user(request): - """Log in the user in REMOTE_USER, if any and enabled - - :return: Account for remote user, or None - :rtype: Account, None - """ - remote_username = remote_user.get_username(request) - if remote_username: - # Get or create an account from the REMOTE_USER http header - account = remote_user.authenticate(request) - if account: - request.session[ACCOUNT_ID_VAR] = account.id - request.account = account - return account - return None - - class AuthorizationMiddleware(MiddlewareMixin): def process_request(self, request): account = request.account @@ -310,89 +284,6 @@ def logout(request, sudo=False): return u'/' -# -# sudo-related functionality -# - - -def sudo(request, other_user): - """Switches the current session to become other_user""" - if SUDOER_ID_VAR in request.session: - # Already logged in as another user. - raise SudoRecursionError() - if not is_admin(get_account(request)): - # Check if sudoer is acctually admin - raise SudoNotAdminError() - original_user = request.account - request.session[SUDOER_ID_VAR] = original_user.id - _set_account(request, other_user) - _logger.info('Sudo: "%s" acting as "%s"', original_user, other_user) - _logger.debug( - 'Sudo: (session: %s, account: %s)', dict(request.session), request.account - ) - LogEntry.add_log_entry( - original_user, - 'sudo', - '{actor} sudoed to {target}', - subsystem='auth', - target=other_user, - ) - - -def desudo(request): - """Switches the current session to become the original user from before a - call to sudo(). - - """ - if SUDOER_ID_VAR not in request.session: - # We are not sudoing - return - - other_user = request.account - original_user_id = request.session[SUDOER_ID_VAR] - original_user = Account.objects.get(id=original_user_id) - - del request.session[ACCOUNT_ID_VAR] - del request.session[SUDOER_ID_VAR] - _set_account(request, original_user) - _logger.info( - 'DeSudo: "%s" no longer acting as "%s"', original_user, request.account - ) - _logger.debug( - 'DeSudo: (session: %s, account: %s)', dict(request.session), request.account - ) - LogEntry.add_log_entry( - original_user, - 'desudo', - '{actor} no longer sudoed as {target}', - subsystem='auth', - target=other_user, - ) - - -def get_sudoer(request): - """Returns a sudoer's Account, if current session is in sudo-mode""" - if SUDOER_ID_VAR in request.session: - return Account.objects.get(id=request.session[SUDOER_ID_VAR]) - - -class SudoRecursionError(Exception): - msg = u"Already posing as another user" - - def __str__(self): - return self.msg - - -class SudoNotAdminError(Exception): - msg = u"Not admin" - - def __str__(self): - return self.msg - - -# For testing - - def create_session_cookie(username): """Creates an active session for username and returns the resulting session cookie. diff --git a/python/nav/web/auth/remote_user.py b/python/nav/web/auth/remote_user.py index 2631787cc1..f790c4f3c0 100644 --- a/python/nav/web/auth/remote_user.py +++ b/python/nav/web/auth/remote_user.py @@ -20,6 +20,7 @@ from nav.auditlog.models import LogEntry from nav.config import NAVConfigParser from nav.models.profiles import Account +from nav.web.auth.utils import ACCOUNT_ID_VAR try: # Python 3.6+ @@ -98,6 +99,23 @@ def authenticate(request): return account +def login(request): + """Log in the user in REMOTE_USER, if any and enabled + + :return: Account for remote user, or None + :rtype: Account, None + """ + remote_username = get_username(request) + if remote_username: + # Get or create an account from the REMOTE_USER http header + account = authenticate(request) + if account: + request.session[ACCOUNT_ID_VAR] = account.id + request.account = account + return account + return None + + def get_loginurl(request): """Return a url (if set) to log in to/via a remote service diff --git a/python/nav/web/auth/sudo.py b/python/nav/web/auth/sudo.py new file mode 100644 index 0000000000..fd182dfd9c --- /dev/null +++ b/python/nav/web/auth/sudo.py @@ -0,0 +1,108 @@ +# Copyright (C) 2010, 2011, 2013, 2019 Uninett AS +# Copyright (C) 2022 Sikt +# +# This file is part of Network Administration Visualized (NAV). +# +# NAV is free software: you can redistribute it and/or modify it under +# the terms of the GNU General Public License version 3 as published by +# the Free Software Foundation. +# +# This program is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +# more details. You should have received a copy of the GNU General Public +# License along with NAV. If not, see . +# +""" +Contains web authentication and login functionality for NAV. + +The "*Middleware" is Django-specific. +""" + +import logging + +from nav.auditlog.models import LogEntry +from nav.django.utils import is_admin, get_account +from nav.models.profiles import Account +from nav.web.auth.utils import _set_account, ACCOUNT_ID_VAR + + +_logger = logging.getLogger(__name__) + + +SUDOER_ID_VAR = 'sudoer' + + +def sudo(request, other_user): + """Switches the current session to become other_user""" + if SUDOER_ID_VAR in request.session: + # Already logged in as another user. + raise SudoRecursionError() + if not is_admin(get_account(request)): + # Check if sudoer is acctually admin + raise SudoNotAdminError() + original_user = request.account + request.session[SUDOER_ID_VAR] = original_user.id + _set_account(request, other_user) + _logger.info('Sudo: "%s" acting as "%s"', original_user, other_user) + _logger.debug( + 'Sudo: (session: %s, account: %s)', dict(request.session), request.account + ) + LogEntry.add_log_entry( + original_user, + 'sudo', + '{actor} sudoed to {target}', + subsystem='auth', + target=other_user, + ) + + +def desudo(request): + """Switches the current session to become the original user from before a + call to sudo(). + + """ + if SUDOER_ID_VAR not in request.session: + # We are not sudoing + return + + other_user = request.account + original_user_id = request.session[SUDOER_ID_VAR] + original_user = Account.objects.get(id=original_user_id) + + del request.session[ACCOUNT_ID_VAR] + del request.session[SUDOER_ID_VAR] + _set_account(request, original_user) + _logger.info( + 'DeSudo: "%s" no longer acting as "%s"', original_user, request.account + ) + _logger.debug( + 'DeSudo: (session: %s, account: %s)', dict(request.session), request.account + ) + LogEntry.add_log_entry( + original_user, + 'desudo', + '{actor} no longer sudoed as {target}', + subsystem='auth', + target=other_user, + ) + + +def get_sudoer(request): + """Returns a sudoer's Account, if current session is in sudo-mode""" + if SUDOER_ID_VAR in request.session: + return Account.objects.get(id=request.session[SUDOER_ID_VAR]) + + +class SudoRecursionError(Exception): + msg = u"Already posing as another user" + + def __str__(self): + return self.msg + + +class SudoNotAdminError(Exception): + msg = u"Not admin" + + def __str__(self): + return self.msg diff --git a/python/nav/web/auth/utils.py b/python/nav/web/auth/utils.py new file mode 100644 index 0000000000..61e5b73e23 --- /dev/null +++ b/python/nav/web/auth/utils.py @@ -0,0 +1,30 @@ +# Copyright (C) 2010, 2011, 2013, 2019 Uninett AS +# Copyright (C) 2022 Sikt +# +# This file is part of Network Administration Visualized (NAV). +# +# NAV is free software: you can redistribute it and/or modify it under +# the terms of the GNU General Public License version 3 as published by +# the Free Software Foundation. +# +# This program is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +# more details. You should have received a copy of the GNU General Public +# License along with NAV. If not, see . +# + +import logging + + +_logger = logging.getLogger(__name__) + + +ACCOUNT_ID_VAR = 'account_id' + + +def _set_account(request, account): + request.session[ACCOUNT_ID_VAR] = account.id + request.account = account + _logger.debug('Set active account to "%s"', account.login) + request.session.save() diff --git a/python/nav/web/navlets/__init__.py b/python/nav/web/navlets/__init__.py index cce44f9c7e..4f54b3bf16 100644 --- a/python/nav/web/navlets/__init__.py +++ b/python/nav/web/navlets/__init__.py @@ -58,7 +58,7 @@ from nav.models.profiles import AccountNavlet, AccountDashboard from nav.models.manage import Sensor -from nav.web.auth import get_sudoer +from nav.web.auth.sudo import get_sudoer from nav.django.utils import get_account from nav.web.utils import require_param from nav.web.webfront import find_dashboard diff --git a/python/nav/web/webfront/views.py b/python/nav/web/webfront/views.py index f74be34954..8ec0befff3 100644 --- a/python/nav/web/webfront/views.py +++ b/python/nav/web/webfront/views.py @@ -37,7 +37,7 @@ from nav.auditlog.models import LogEntry from nav.django.utils import get_account from nav.models.profiles import NavbarLink, AccountDashboard, AccountNavlet -from nav.web.auth import ACCOUNT_ID_VAR +from nav.web.auth.utils import ACCOUNT_ID_VAR from nav.web.auth import logout as auth_logout from nav.web import auth from nav.web.auth import ldap diff --git a/tests/unittests/general/web_middleware_test.py b/tests/unittests/general/web_middleware_test.py index 247a74e6e3..c943308eaa 100644 --- a/tests/unittests/general/web_middleware_test.py +++ b/tests/unittests/general/web_middleware_test.py @@ -3,8 +3,8 @@ from django.test import RequestFactory -from nav.web.auth import ACCOUNT_ID_VAR -from nav.web.auth import SUDOER_ID_VAR +from nav.web.auth.utils import ACCOUNT_ID_VAR +from nav.web.auth.sudo import SUDOER_ID_VAR from nav.web.auth import AuthenticationMiddleware from nav.web.auth import AuthorizationMiddleware from nav.web.auth import logout @@ -159,7 +159,9 @@ def test_process_request_switch_users(self): side_effect=auth._set_account(fake_request, ANOTHER_PLAIN_ACCOUNT), ): with patch('nav.web.auth.logout'): - AuthenticationMiddleware(lambda x: x).process_request(fake_request) + AuthenticationMiddleware(lambda x: x).process_request( + fake_request + ) assert fake_request.account == ANOTHER_PLAIN_ACCOUNT assert ( ACCOUNT_ID_VAR in fake_request.session @@ -199,7 +201,9 @@ def test_process_request_not_authorized(self): 'nav.web.auth.AuthorizationMiddleware.redirect_to_login', return_value='here', ): - result = AuthorizationMiddleware(lambda x: x).process_request(fake_request) + result = AuthorizationMiddleware(lambda x: x).process_request( + fake_request + ) assert result == 'here' assert os.environ.get('REMOTE_USER', None) != PLAIN_ACCOUNT.login diff --git a/tests/unittests/general/webfront_test.py b/tests/unittests/general/webfront_test.py index 2174e2ee39..5b23d931d5 100644 --- a/tests/unittests/general/webfront_test.py +++ b/tests/unittests/general/webfront_test.py @@ -158,7 +158,7 @@ def test_remote_user_unset(self): with patch("nav.web.auth.remote_user.get_username", return_value=False): remote_user.login(request) assert not getattr(request, 'account', False) - assert auth.ACCOUNT_ID_VAR not in request.session + assert ACCOUNT_ID_VAR not in request.session def test_remote_user_set(self): r = RequestFactory() @@ -172,10 +172,9 @@ def test_remote_user_set(self): remote_user.login(request) assert hasattr(request, 'account') assert request.account == REMOTE_USER_ACCOUNT - assert auth.ACCOUNT_ID_VAR in request.session + assert ACCOUNT_ID_VAR in request.session assert ( - request.session.get(auth.ACCOUNT_ID_VAR, None) - == REMOTE_USER_ACCOUNT.id + request.session.get(ACCOUNT_ID_VAR, None) == REMOTE_USER_ACCOUNT.id ) From f1191b7ae145b35448708ad591c9924f8c334bcd Mon Sep 17 00:00:00 2001 From: Hanne Moa Date: Thu, 2 Nov 2023 08:47:39 +0100 Subject: [PATCH 6/8] Move middleware to middleware.py --- python/nav/django/settings.py | 4 +- python/nav/web/auth/__init__.py | 129 +----------------- python/nav/web/auth/middleware.py | 115 ++++++++++++++++ python/nav/web/auth/utils.py | 38 ++++++ python/nav/web/modpython.py | 2 +- .../unittests/general/web_middleware_test.py | 65 +++++---- 6 files changed, 194 insertions(+), 159 deletions(-) create mode 100644 python/nav/web/auth/middleware.py diff --git a/python/nav/django/settings.py b/python/nav/django/settings.py index 1c339910a7..5e27110c6d 100644 --- a/python/nav/django/settings.py +++ b/python/nav/django/settings.py @@ -127,8 +127,8 @@ MIDDLEWARE = ( 'django.middleware.common.CommonMiddleware', 'django.contrib.sessions.middleware.SessionMiddleware', - 'nav.web.auth.AuthenticationMiddleware', - 'nav.web.auth.AuthorizationMiddleware', + 'nav.web.auth.middleware.AuthenticationMiddleware', + 'nav.web.auth.middleware.AuthorizationMiddleware', 'nav.django.legacy.LegacyCleanupMiddleware', 'django.contrib.messages.middleware.MessageMiddleware', ) diff --git a/python/nav/web/auth/__init__.py b/python/nav/web/auth/__init__.py index d713183da9..61d2c5197c 100644 --- a/python/nav/web/auth/__init__.py +++ b/python/nav/web/auth/__init__.py @@ -26,20 +26,13 @@ from django.conf import settings from django.contrib.sessions.backends.db import SessionStore -from django.http import HttpResponseRedirect, HttpResponse from django.urls import reverse -try: - from django.utils.deprecation import MiddlewareMixin -except ImportError: # Django <= 1.9 - MiddlewareMixin = object - - from nav.auditlog.models import LogEntry from nav.models.profiles import Account, AccountGroup from nav.web.auth import ldap, remote_user -from nav.web.auth.sudo import desudo, get_sudoer -from nav.web.auth.utils import _set_account, ACCOUNT_ID_VAR +from nav.web.auth.sudo import desudo +from nav.web.auth.utils import ACCOUNT_ID_VAR _logger = logging.getLogger(__name__) @@ -143,124 +136,6 @@ def get_logout_url(request): return remote_logouturl if remote_logouturl else LOGOUT_URL -# Middleware - - -class AuthenticationMiddleware(MiddlewareMixin): - def process_request(self, request): - _logger.debug( - 'AuthenticationMiddleware ENTER (session: %s, account: %s) from "%s"', - dict(request.session), - getattr(request, 'account', 'NOT SET'), - request.get_full_path(), - ) - ensure_account(request) - - account = request.account - sudo_operator = get_sudoer(request) # Account or None - logged_in = sudo_operator or account - _logger.debug( - ('AuthenticationMiddleware ' '(logged_in: "%s" acting as "%s") from "%s"'), - logged_in.login, - account.login, - request.get_full_path(), - ) - - remote_username = remote_user.get_username(request) - if remote_username: - _logger.debug( - ('AuthenticationMiddleware: ' '(REMOTE_USER: "%s") from "%s"'), - remote_username, - request.get_full_path(), - ) - if logged_in.id == Account.DEFAULT_ACCOUNT: - # Switch from anonymous to REMOTE_USER - remote_user.login(request) - elif remote_username != logged_in.login: - # REMOTE_USER has changed, logout - logout(request, sudo=bool(sudo_operator)) - sudo_operator = None - # Activate anonymous account for AuthorizationMiddleware's sake - ensure_account(request) - - if sudo_operator is not None: - request.account.sudo_operator = sudo_operator - - _logger.debug( - 'AuthenticationMiddleware EXIT (session: %s, account: %s) from "%s"', - dict(request.session), - getattr(request, 'account', 'NOT SET'), - request.get_full_path(), - ) - - -def ensure_account(request): - """Guarantee that valid request.account is set""" - session = request.session - - if not ACCOUNT_ID_VAR in session: - session[ACCOUNT_ID_VAR] = Account.DEFAULT_ACCOUNT - - account = Account.objects.get(id=session[ACCOUNT_ID_VAR]) - - if account.locked: - # Switch back to fallback, the anonymous user - # Assumes nobody has locked it.. - account = Account.objects.get(id=Account.DEFAULT_ACCOUNT) - - _set_account(request, account) - - -class AuthorizationMiddleware(MiddlewareMixin): - def process_request(self, request): - account = request.account - - authorized = authorization_not_required( - request.get_full_path() - ) or account.has_perm('web_access', request.get_full_path()) - if not authorized: - _logger.warning( - "User %s denied access to %s", account.login, request.get_full_path() - ) - return self.redirect_to_login(request) - else: - if not account.is_default_account(): - os.environ['REMOTE_USER'] = account.login - elif 'REMOTE_USER' in os.environ: - del os.environ['REMOTE_USER'] - - def redirect_to_login(self, request): - """Redirects a request to the NAV login page, unless it was detected - to be an AJAX request, in which case return a 401 Not Authorized - response. - - """ - if request.is_ajax(): - return HttpResponse(status=401) - - new_url = get_login_url(request) - return HttpResponseRedirect(new_url) - - -def authorization_not_required(fullpath): - """Checks is authorization is required for the requested url - - Should the user be able to decide this? Currently not. - - """ - auth_not_required = [ - '/api/', - '/doc/', # No auth/different auth system - '/about/', - '/index/login/', - '/refresh_session', - ] - for url in auth_not_required: - if fullpath.startswith(url): - _logger.debug('authorization_not_required: %s', url) - return True - - def logout(request, sudo=False): """Log out a user from a request diff --git a/python/nav/web/auth/middleware.py b/python/nav/web/auth/middleware.py new file mode 100644 index 0000000000..12ba354775 --- /dev/null +++ b/python/nav/web/auth/middleware.py @@ -0,0 +1,115 @@ +# Copyright (C) 2010, 2011, 2013, 2019 Uninett AS +# Copyright (C) 2022, 2023 Sikt +# +# This file is part of Network Administration Visualized (NAV). +# +# NAV is free software: you can redistribute it and/or modify it under +# the terms of the GNU General Public License version 3 as published by +# the Free Software Foundation. +# +# This program is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +# more details. You should have received a copy of the GNU General Public +# License along with NAV. If not, see . +# + +import logging +import os + +from django.http import HttpResponseRedirect, HttpResponse + +try: + from django.utils.deprecation import MiddlewareMixin +except ImportError: # Django <= 1.9 + MiddlewareMixin = object + +from nav.models.profiles import Account +from nav.web.auth import remote_user, get_login_url, logout +from nav.web.auth.utils import ( + ensure_account, + authorization_not_required, +) +from nav.web.auth.sudo import get_sudoer + + +_logger = logging.getLogger(__name__) + + +class AuthenticationMiddleware(MiddlewareMixin): + def process_request(self, request): + _logger.debug( + 'AuthenticationMiddleware ENTER (session: %s, account: %s) from "%s"', + dict(request.session), + getattr(request, 'account', 'NOT SET'), + request.get_full_path(), + ) + ensure_account(request) + + account = request.account + sudo_operator = get_sudoer(request) # Account or None + logged_in = sudo_operator or account + _logger.debug( + ('AuthenticationMiddleware ' '(logged_in: "%s" acting as "%s") from "%s"'), + logged_in.login, + account.login, + request.get_full_path(), + ) + + remote_username = remote_user.get_username(request) + if remote_username: + _logger.debug( + ('AuthenticationMiddleware: ' '(REMOTE_USER: "%s") from "%s"'), + remote_username, + request.get_full_path(), + ) + if logged_in.id == Account.DEFAULT_ACCOUNT: + # Switch from anonymous to REMOTE_USER + remote_user.login(request) + elif remote_username != logged_in.login: + # REMOTE_USER has changed, logout + logout(request, sudo=bool(sudo_operator)) + sudo_operator = None + # Activate anonymous account for AuthorizationMiddleware's sake + ensure_account(request) + + if sudo_operator is not None: + request.account.sudo_operator = sudo_operator + + _logger.debug( + 'AuthenticationMiddleware EXIT (session: %s, account: %s) from "%s"', + dict(request.session), + getattr(request, 'account', 'NOT SET'), + request.get_full_path(), + ) + + +class AuthorizationMiddleware(MiddlewareMixin): + def process_request(self, request): + account = request.account + + authorized = authorization_not_required( + request.get_full_path() + ) or account.has_perm('web_access', request.get_full_path()) + if not authorized: + _logger.warning( + "User %s denied access to %s", account.login, request.get_full_path() + ) + return self.redirect_to_login(request) + else: + if not account.is_default_account(): + os.environ['REMOTE_USER'] = account.login + elif 'REMOTE_USER' in os.environ: + del os.environ['REMOTE_USER'] + + def redirect_to_login(self, request): + """Redirects a request to the NAV login page, unless it was detected + to be an AJAX request, in which case return a 401 Not Authorized + response. + + """ + if request.is_ajax(): + return HttpResponse(status=401) + + new_url = get_login_url(request) + return HttpResponseRedirect(new_url) diff --git a/python/nav/web/auth/utils.py b/python/nav/web/auth/utils.py index 61e5b73e23..70004ee19c 100644 --- a/python/nav/web/auth/utils.py +++ b/python/nav/web/auth/utils.py @@ -16,6 +16,8 @@ import logging +from nav.models.profiles import Account + _logger = logging.getLogger(__name__) @@ -28,3 +30,39 @@ def _set_account(request, account): request.account = account _logger.debug('Set active account to "%s"', account.login) request.session.save() + + +def ensure_account(request): + """Guarantee that valid request.account is set""" + session = request.session + + if not ACCOUNT_ID_VAR in session: + session[ACCOUNT_ID_VAR] = Account.DEFAULT_ACCOUNT + + account = Account.objects.get(id=session[ACCOUNT_ID_VAR]) + + if account.locked: + # Switch back to fallback, the anonymous user + # Assumes nobody has locked it.. + account = Account.objects.get(id=Account.DEFAULT_ACCOUNT) + + _set_account(request, account) + + +def authorization_not_required(fullpath): + """Checks is authorization is required for the requested url + + Should the user be able to decide this? Currently not. + + """ + auth_not_required = [ + '/api/', + '/doc/', # No auth/different auth system + '/about/', + '/index/login/', + '/refresh_session', + ] + for url in auth_not_required: + if fullpath.startswith(url): + _logger.debug('authorization_not_required: %s', url) + return True diff --git a/python/nav/web/modpython.py b/python/nav/web/modpython.py index 5385535d6b..e5f73a8812 100644 --- a/python/nav/web/modpython.py +++ b/python/nav/web/modpython.py @@ -39,7 +39,7 @@ from nav.bootstrap import bootstrap_django from django.contrib.sessions.middleware import SessionMiddleware -from nav.web.auth import AuthenticationMiddleware, AuthorizationMiddleware +from nav.web.auth.middleware import AuthenticationMiddleware, AuthorizationMiddleware from nav.web import loginit from django.db import connection diff --git a/tests/unittests/general/web_middleware_test.py b/tests/unittests/general/web_middleware_test.py index c943308eaa..5af8914e86 100644 --- a/tests/unittests/general/web_middleware_test.py +++ b/tests/unittests/general/web_middleware_test.py @@ -3,10 +3,10 @@ from django.test import RequestFactory -from nav.web.auth.utils import ACCOUNT_ID_VAR +from nav.web.auth.utils import ACCOUNT_ID_VAR, _set_account, ensure_account from nav.web.auth.sudo import SUDOER_ID_VAR -from nav.web.auth import AuthenticationMiddleware -from nav.web.auth import AuthorizationMiddleware +from nav.web.auth.middleware import AuthenticationMiddleware +from nav.web.auth.middleware import AuthorizationMiddleware from nav.web.auth import logout from nav.web import auth @@ -34,7 +34,7 @@ def test_set_account(): r = RequestFactory() request = r.get('/') request.session = FakeSession() - auth._set_account(request, DEFAULT_ACCOUNT) + _set_account(request, DEFAULT_ACCOUNT) assert ACCOUNT_ID_VAR in request.session, 'Account id is not in the session' assert hasattr(request, 'account'), 'Account not set' assert request.account.id == request.session[ACCOUNT_ID_VAR], 'Correct user not set' @@ -48,7 +48,7 @@ def test_account_is_set_if_missing(self): request.session = {} request.session = FakeSession() with patch("nav.web.auth.Account.objects.get", return_value=DEFAULT_ACCOUNT): - auth.ensure_account(request) + ensure_account(request) assert ( auth.ACCOUNT_ID_VAR in request.session ), 'Account id is not in the session' @@ -66,7 +66,7 @@ def test_account_is_switched_to_default_if_locked(self): "nav.web.auth.Account.objects.get", side_effect=[LOCKED_ACCOUNT, DEFAULT_ACCOUNT], ): - auth.ensure_account(request) + ensure_account(request) assert request.session[auth.ACCOUNT_ID_VAR] == DEFAULT_ACCOUNT.id assert request.account == DEFAULT_ACCOUNT, 'Correct user not set' @@ -76,7 +76,7 @@ def test_account_is_left_alone_if_ok(self): request.session = FakeSession() request.session[auth.ACCOUNT_ID_VAR] = return_value = PLAIN_ACCOUNT.id with patch("nav.web.auth.Account.objects.get", return_value=PLAIN_ACCOUNT): - auth.ensure_account(request) + ensure_account(request) assert request.account == PLAIN_ACCOUNT assert request.session[auth.ACCOUNT_ID_VAR] == PLAIN_ACCOUNT.id @@ -87,8 +87,8 @@ def test_process_request_logged_in(self): fake_request = r.get('/') fake_request.session = FakeSession(ACCOUNT_ID_VAR=PLAIN_ACCOUNT.id) with patch( - 'nav.web.auth.ensure_account', - side_effect=auth._set_account(fake_request, PLAIN_ACCOUNT), + 'nav.web.auth.middleware.ensure_account', + side_effect=_set_account(fake_request, PLAIN_ACCOUNT), ): AuthenticationMiddleware(lambda x: x).process_request(fake_request) assert fake_request.account == PLAIN_ACCOUNT @@ -101,10 +101,10 @@ def test_process_request_set_sudoer(self): ACCOUNT_ID_VAR=PLAIN_ACCOUNT.id, SUDOER_ID_VAR=SUDO_ACCOUNT.id ) with patch( - 'nav.web.auth.ensure_account', - side_effect=auth._set_account(fake_request, PLAIN_ACCOUNT), + 'nav.web.auth.middleware.ensure_account', + side_effect=_set_account(fake_request, PLAIN_ACCOUNT), ): - with patch('nav.web.auth.get_sudoer', return_value=SUDO_ACCOUNT): + with patch('nav.web.auth.middleware.get_sudoer', return_value=SUDO_ACCOUNT): AuthenticationMiddleware(lambda x: x).process_request(fake_request) assert ( getattr(fake_request.account, 'sudo_operator', None) == SUDO_ACCOUNT @@ -115,10 +115,10 @@ def test_process_request_not_logged_in(self): fake_request = r.get('/') fake_request.session = FakeSession() with patch( - 'nav.web.auth.ensure_account', - side_effect=auth._set_account(fake_request, DEFAULT_ACCOUNT), + 'nav.web.auth.middleware.ensure_account', + side_effect=_set_account(fake_request, DEFAULT_ACCOUNT), ): - with patch('nav.web.auth.get_remote_username', return_value=None): + with patch('nav.web.auth.remote_user.get_username', return_value=None): AuthenticationMiddleware(lambda x: x).process_request(fake_request) assert fake_request.account == DEFAULT_ACCOUNT assert fake_request.session[ACCOUNT_ID_VAR] == fake_request.account.id @@ -128,15 +128,16 @@ def test_process_request_log_in_remote_user(self): fake_request = r.get('/') fake_request.session = FakeSession() with patch( - 'nav.web.auth.ensure_account', - side_effect=auth._set_account(fake_request, DEFAULT_ACCOUNT), + 'nav.web.auth.middleware.ensure_account', + side_effect=_set_account(fake_request, DEFAULT_ACCOUNT), ): with patch( - 'nav.web.auth.get_remote_username', return_value=PLAIN_ACCOUNT.login + 'nav.web.auth.remote_user.get_username', + return_value=PLAIN_ACCOUNT.login, ): with patch( - 'nav.web.auth.login_remote_user', - side_effect=auth._set_account(fake_request, PLAIN_ACCOUNT), + 'nav.web.auth.remote_user.login', + side_effect=_set_account(fake_request, PLAIN_ACCOUNT), ): AuthenticationMiddleware(lambda x: x).process_request(fake_request) assert fake_request.account == PLAIN_ACCOUNT @@ -147,16 +148,16 @@ def test_process_request_switch_users(self): fake_request = r.get('/') fake_request.session = FakeSession() with patch( - 'nav.web.auth.ensure_account', - side_effect=auth._set_account(fake_request, PLAIN_ACCOUNT), + 'nav.web.auth.middleware.ensure_account', + side_effect=_set_account(fake_request, PLAIN_ACCOUNT), ): with patch( - 'nav.web.auth.get_remote_username', + 'nav.web.auth.remote_user.get_username', return_value=ANOTHER_PLAIN_ACCOUNT.login, ): with patch( - 'nav.web.auth.login_remote_user', - side_effect=auth._set_account(fake_request, ANOTHER_PLAIN_ACCOUNT), + 'nav.web.auth.remote_user.login', + side_effect=_set_account(fake_request, ANOTHER_PLAIN_ACCOUNT), ): with patch('nav.web.auth.logout'): AuthenticationMiddleware(lambda x: x).process_request( @@ -179,7 +180,9 @@ def test_process_request_anonymous(self): r = RequestFactory() fake_request = r.get('/') fake_request.account = DEFAULT_ACCOUNT - with patch('nav.web.auth.authorization_not_required', return_value=True): + with patch( + 'nav.web.auth.middleware.authorization_not_required', return_value=True + ): AuthorizationMiddleware(lambda x: x).process_request(fake_request) assert 'REMOTE_USER' not in os.environ @@ -187,7 +190,9 @@ def test_process_request_authorized(self): r = RequestFactory() fake_request = r.get('/') fake_request.account = PLAIN_ACCOUNT - with patch('nav.web.auth.authorization_not_required', return_value=True): + with patch( + 'nav.web.auth.middleware.authorization_not_required', return_value=True + ): AuthorizationMiddleware(lambda x: x).process_request(fake_request) assert os.environ.get('REMOTE_USER', None) == PLAIN_ACCOUNT.login @@ -195,10 +200,12 @@ def test_process_request_not_authorized(self): r = RequestFactory() fake_request = r.get('/') fake_request.account = PLAIN_ACCOUNT - with patch('nav.web.auth.authorization_not_required', return_value=False): + with patch( + 'nav.web.auth.middleware.authorization_not_required', return_value=False + ): with patch('nav.web.auth.Account.has_perm', return_value=False): with patch( - 'nav.web.auth.AuthorizationMiddleware.redirect_to_login', + 'nav.web.auth.middleware.AuthorizationMiddleware.redirect_to_login', return_value='here', ): result = AuthorizationMiddleware(lambda x: x).process_request( From 6a7d097e9f8c0419e74dabc942ad5f9e60d159e5 Mon Sep 17 00:00:00 2001 From: Hanne Moa Date: Thu, 2 Nov 2023 09:52:24 +0100 Subject: [PATCH 7/8] Move create_session_cookie to utils.py It isn't used by anything in auth, only in tests. --- python/nav/web/auth/__init__.py | 23 ----------------------- python/nav/web/auth/utils.py | 24 ++++++++++++++++++++++++ tests/functional/conftest.py | 2 +- 3 files changed, 25 insertions(+), 24 deletions(-) diff --git a/python/nav/web/auth/__init__.py b/python/nav/web/auth/__init__.py index 61d2c5197c..c082668a04 100644 --- a/python/nav/web/auth/__init__.py +++ b/python/nav/web/auth/__init__.py @@ -24,8 +24,6 @@ from urllib import parse -from django.conf import settings -from django.contrib.sessions.backends.db import SessionStore from django.urls import reverse from nav.auditlog.models import LogEntry @@ -157,24 +155,3 @@ def logout(request, sudo=False): LogEntry.add_log_entry(account, 'log-out', '{actor} logged out', before=account) _logger.debug('logout: redirect to "/" after logout') return u'/' - - -def create_session_cookie(username): - """Creates an active session for username and returns the resulting - session cookie. - - This is useful to fake login sessions during testing. - - """ - user = Account.objects.get(login=username) - session = SessionStore() - session[ACCOUNT_ID_VAR] = user.id - session.save() - - cookie = { - 'name': settings.SESSION_COOKIE_NAME, - 'value': session.session_key, - 'secure': False, - 'path': '/', - } - return cookie diff --git a/python/nav/web/auth/utils.py b/python/nav/web/auth/utils.py index 70004ee19c..3bfbf0ef8b 100644 --- a/python/nav/web/auth/utils.py +++ b/python/nav/web/auth/utils.py @@ -16,6 +16,9 @@ import logging +from django.conf import settings +from django.contrib.sessions.backends.db import SessionStore + from nav.models.profiles import Account @@ -66,3 +69,24 @@ def authorization_not_required(fullpath): if fullpath.startswith(url): _logger.debug('authorization_not_required: %s', url) return True + + +def create_session_cookie(username): + """Creates an active session for username and returns the resulting + session cookie. + + This is useful to fake login sessions during testing. + + """ + user = Account.objects.get(login=username) + session = SessionStore() + session[ACCOUNT_ID_VAR] = user.id + session.save() + + cookie = { + 'name': settings.SESSION_COOKIE_NAME, + 'value': session.session_key, + 'secure': False, + 'path': '/', + } + return cookie diff --git a/tests/functional/conftest.py b/tests/functional/conftest.py index f974f3316c..0281323a3e 100644 --- a/tests/functional/conftest.py +++ b/tests/functional/conftest.py @@ -66,7 +66,7 @@ def selenium(selenium, base_url): bootstrap_django(__file__) - from nav.web.auth import create_session_cookie + from nav.web.auth.utils import create_session_cookie selenium.implicitly_wait(10) wait = WebDriverWait(selenium, 10) From c6ed8be3e5cde6d2192c90cfc1e88f23fe926153 Mon Sep 17 00:00:00 2001 From: Hanne Moa Date: Mon, 6 Nov 2023 08:13:26 +0100 Subject: [PATCH 8/8] Improve/add module docstrings --- python/nav/web/auth/__init__.py | 2 -- python/nav/web/auth/middleware.py | 3 +++ python/nav/web/auth/remote_user.py | 4 +++- python/nav/web/auth/sudo.py | 4 +--- python/nav/web/auth/utils.py | 5 ++++- 5 files changed, 11 insertions(+), 7 deletions(-) diff --git a/python/nav/web/auth/__init__.py b/python/nav/web/auth/__init__.py index c082668a04..142a0d4e5f 100644 --- a/python/nav/web/auth/__init__.py +++ b/python/nav/web/auth/__init__.py @@ -15,8 +15,6 @@ # """ Contains web authentication and login functionality for NAV. - -The "*Middleware" is Django-specific. """ from datetime import datetime diff --git a/python/nav/web/auth/middleware.py b/python/nav/web/auth/middleware.py index 12ba354775..5d2b55daff 100644 --- a/python/nav/web/auth/middleware.py +++ b/python/nav/web/auth/middleware.py @@ -13,6 +13,9 @@ # more details. You should have received a copy of the GNU General Public # License along with NAV. If not, see . # +""" +Django middleware for handling login, authentication and authorization for NAV. +""" import logging import os diff --git a/python/nav/web/auth/remote_user.py b/python/nav/web/auth/remote_user.py index f790c4f3c0..5b6dac3100 100644 --- a/python/nav/web/auth/remote_user.py +++ b/python/nav/web/auth/remote_user.py @@ -13,7 +13,9 @@ # more details. You should have received a copy of the GNU General Public # License along with NAV. If not, see . # - +""" +Support logging in by having the web server set the REMOTE_USER header. +""" import logging from os.path import join diff --git a/python/nav/web/auth/sudo.py b/python/nav/web/auth/sudo.py index fd182dfd9c..85c8fca61f 100644 --- a/python/nav/web/auth/sudo.py +++ b/python/nav/web/auth/sudo.py @@ -14,9 +14,7 @@ # License along with NAV. If not, see . # """ -Contains web authentication and login functionality for NAV. - -The "*Middleware" is Django-specific. +Sudo functionality for web authentication in NAV. """ import logging diff --git a/python/nav/web/auth/utils.py b/python/nav/web/auth/utils.py index 3bfbf0ef8b..874623c2e3 100644 --- a/python/nav/web/auth/utils.py +++ b/python/nav/web/auth/utils.py @@ -13,7 +13,10 @@ # more details. You should have received a copy of the GNU General Public # License along with NAV. If not, see . # - +""" +Utilities for authentication/authorization in NAV that is independent of +login method. +""" import logging from django.conf import settings