diff --git a/python/nav/web/auth/__init__.py b/python/nav/web/auth/__init__.py index 5da0222906..3835eff229 100644 --- a/python/nav/web/auth/__init__.py +++ b/python/nav/web/auth/__init__.py @@ -28,7 +28,7 @@ from nav.models.profiles import Account, AccountGroup from nav.web.auth import ldap, remote_user from nav.web.auth.sudo import desudo -from nav.web.auth.utils import ACCOUNT_ID_VAR +from nav.web.auth.utils import clear_session _logger = logging.getLogger(__name__) @@ -151,10 +151,7 @@ def logout(request, sudo=False): return reverse('webfront-index') else: account = request.account - del request.session[ACCOUNT_ID_VAR] - del request.account - request.session.set_expiry(datetime.now()) - request.session.save() + clear_session(request) _logger.debug('logout: logout %s', account.login) LogEntry.add_log_entry(account, 'log-out', '{actor} logged out', before=account) _logger.debug('logout: redirect to "/" after logout') diff --git a/python/nav/web/auth/sudo.py b/python/nav/web/auth/sudo.py index f3ed2f920d..1997516cda 100644 --- a/python/nav/web/auth/sudo.py +++ b/python/nav/web/auth/sudo.py @@ -22,7 +22,7 @@ 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 +from nav.web.auth.utils import set_account, clear_session _logger = logging.getLogger(__name__) @@ -68,8 +68,7 @@ def desudo(request): 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] + clear_session(request) set_account(request, original_user) _logger.info( 'DeSudo: "%s" no longer acting as "%s"', original_user, request.account diff --git a/python/nav/web/auth/utils.py b/python/nav/web/auth/utils.py index 05fe0df35a..1a17d285dd 100644 --- a/python/nav/web/auth/utils.py +++ b/python/nav/web/auth/utils.py @@ -43,6 +43,14 @@ def set_account(request, account, cycle_session_id=True): request.session.save() +def clear_session(request): + """Clears the session and logs out the current account""" + if hasattr(request, "account"): + del request.account + request.session.flush() + request.session.save() + + def ensure_account(request): """Guarantee that valid request.account is set""" session = request.session @@ -51,6 +59,9 @@ def ensure_account(request): account = Account.objects.get(id=account_id) if account.locked: + # logout of locked account + clear_session(request) + # Switch back to fallback, the anonymous user # Assumes nobody has locked it.. account = Account.objects.get(id=Account.DEFAULT_ACCOUNT) diff --git a/tests/integration/web/auth/auth_test.py b/tests/integration/web/auth/auth_test.py new file mode 100644 index 0000000000..48d4d18daa --- /dev/null +++ b/tests/integration/web/auth/auth_test.py @@ -0,0 +1,54 @@ +from nav.web.auth import logout +from nav.web.auth.utils import ACCOUNT_ID_VAR, set_account +from nav.web.auth.sudo import sudo + + +class TestLogout: + def test_non_sudo_logout_should_remove_session_data( + self, db, session_request, admin_account + ): + # login with admin acount + set_account(session_request, admin_account) + logout(session_request) + assert not hasattr(session_request, 'account') + assert ACCOUNT_ID_VAR not in session_request.session + + def test_non_sudo_logout_should_return_path_to_index( + self, db, session_request, admin_account + ): + # login with admin acount + set_account(session_request, admin_account) + result = logout(session_request) + assert result == '/' + + def test_sudo_logout_should_set_session_to_original_user( + self, db, session_request, admin_account, account + ): + # login with admin acount + set_account(session_request, admin_account) + sudo(session_request, account) + assert session_request.account is account + result = logout(session_request, sudo=True) + assert result == '/' + assert session_request.account == admin_account + + def test_non_sudo_logout_should_change_session_id( + self, db, session_request, admin_account + ): + # login with admin acount + set_account(session_request, admin_account) + pre_session_id = session_request.session.session_key + logout(session_request) + post_session_id = session_request.session.session_key + assert post_session_id != pre_session_id + + def test_sudo_logout_should_change_session_id( + self, db, session_request, admin_account, account + ): + # login with admin acount + set_account(session_request, admin_account) + sudo(session_request, account) + pre_session_id = session_request.session.session_key + logout(session_request, sudo=True) + post_session_id = session_request.session.session_key + assert post_session_id != pre_session_id diff --git a/tests/integration/web/auth/conftest.py b/tests/integration/web/auth/conftest.py index ebaa41f645..5b5641c1a9 100644 --- a/tests/integration/web/auth/conftest.py +++ b/tests/integration/web/auth/conftest.py @@ -15,3 +15,31 @@ def session_request(db): middleware.process_request(session_request) session_request.session.save() return session_request + + +@pytest.fixture() +def account(db): + from nav.models.profiles import Account + + account = Account(login="other_user") + account.set_password("password") + account.save() + yield account + account.delete() + + +@pytest.fixture() +def locked_account(db): + from nav.models.profiles import Account + + account = Account(login="locked_user") + account.save() + yield account + account.delete() + + +@pytest.fixture() +def default_account(db): + from nav.models.profiles import Account + + return Account.objects.get(id=Account.DEFAULT_ACCOUNT) diff --git a/tests/integration/web/auth/middleware_test.py b/tests/integration/web/auth/middleware_test.py index 274d5b912e..fd7accb9ec 100644 --- a/tests/integration/web/auth/middleware_test.py +++ b/tests/integration/web/auth/middleware_test.py @@ -1,28 +1,15 @@ -import pytest from mock import patch from nav.web.auth.middleware import AuthenticationMiddleware -from nav.models.profiles import Account def test_when_remote_user_logs_in_it_should_change_the_session_id( - db, session_request, remote_account + db, session_request, account ): pre_login_session_id = session_request.session.session_key - with patch( - 'nav.web.auth.remote_user.get_username', return_value=remote_account.login - ): + with patch('nav.web.auth.remote_user.get_username', return_value=account.login): middleware = AuthenticationMiddleware(lambda request: None) middleware.process_request(session_request) - assert session_request.account == remote_account + assert session_request.account == account post_login_session_id = session_request.session.session_key assert pre_login_session_id != post_login_session_id - - -@pytest.fixture() -def remote_account(db): - account = Account(login="remote") - account.set_password("supersecret") - account.save() - yield account - account.delete() diff --git a/tests/integration/web/auth/sudo_test.py b/tests/integration/web/auth/sudo_test.py index c2ef48d2a8..373c6c9bf7 100644 --- a/tests/integration/web/auth/sudo_test.py +++ b/tests/integration/web/auth/sudo_test.py @@ -1,43 +1,26 @@ -import pytest - -from django.test import RequestFactory - from nav.web.auth.utils import set_account from nav.web.auth.sudo import sudo, desudo -from nav.models.profiles import Account -def test_sudo_should_change_session_id( - db, session_request, admin_account, other_account -): +def test_sudo_should_change_session_id(db, session_request, admin_account, account): # login with admin acount set_account(session_request, admin_account) pre_sudo_session_id = session_request.session.session_key - sudo(session_request, other_account) + sudo(session_request, account) post_sudo_session_id = session_request.session.session_key assert pre_sudo_session_id != post_sudo_session_id -def test_desudo_should_change_session_id( - db, session_request, admin_account, other_account -): +def test_desudo_should_change_session_id(db, session_request, admin_account, account): # login with admin acount set_account(session_request, admin_account) - sudo(session_request, other_account) + sudo(session_request, account) pre_desudo_session_id = session_request.session.session_key desudo(session_request) post_desudo_session_id = session_request.session.session_key assert pre_desudo_session_id != post_desudo_session_id - - -@pytest.fixture() -def other_account(db): - account = Account(login="other_user") - account.save() - yield account - account.delete() diff --git a/tests/integration/web/auth/utils_test.py b/tests/integration/web/auth/utils_test.py new file mode 100644 index 0000000000..a09c00c91c --- /dev/null +++ b/tests/integration/web/auth/utils_test.py @@ -0,0 +1,69 @@ +from nav.web.auth.utils import ( + set_account, + clear_session, + ACCOUNT_ID_VAR, + ensure_account, +) + + +class TestClearSession: + def test_should_create_new_session_id(self, db, session_request): + pre_session_id = session_request.session.session_key + clear_session(session_request) + post_session_id = session_request.session.session_key + assert pre_session_id != post_session_id + + def test_should_remove_account_from_request( + self, db, session_request, admin_account + ): + # login with admin acount + set_account(session_request, admin_account) + assert session_request.account + clear_session(session_request) + assert not hasattr(session_request, "account") + + def test_should_clear_session_dict(self, db, session_request, admin_account): + set_account(session_request, admin_account) + # Make sure there is something to be deleted + assert session_request.session.keys() + clear_session(session_request) + assert not session_request.session.keys() + + +class TestEnsureAccount: + def test_account_should_be_set_if_request_does_not_already_have_an_account( + self, db, session_request + ): + assert not hasattr(session_request, "account") + ensure_account(session_request) + assert ( + ACCOUNT_ID_VAR in session_request.session + ), 'Account id is not in the session' + assert hasattr(session_request, 'account'), 'Account not set' + assert ( + session_request.account.id == session_request.session[ACCOUNT_ID_VAR] + ), 'Correct user not set' + + def test_account_should_be_switched_to_default_if_locked( + self, db, session_request, locked_account, default_account + ): + set_account(session_request, locked_account) + ensure_account(session_request) + assert session_request.session[ACCOUNT_ID_VAR] == default_account.id + assert session_request.account == default_account, 'Correct user not set' + + def test_account_should_be_unchanged_if_ok(self, db, session_request, account): + set_account(session_request, account) + ensure_account(session_request) + assert session_request.account == account + assert session_request.session[ACCOUNT_ID_VAR] == account.id + + def test_session_id_should_be_changed_if_going_from_locked_to_default_account( + self, db, session_request, locked_account, default_account + ): + set_account(session_request, locked_account) + pre_session_id = session_request.session.session_key + ensure_account(session_request) + assert session_request.account == default_account + post_session_id = session_request.session.session_key + assert post_session_id != pre_session_id diff --git a/tests/unittests/general/web_middleware_test.py b/tests/unittests/general/web_middleware_test.py index 483c4cdcb7..227264b370 100644 --- a/tests/unittests/general/web_middleware_test.py +++ b/tests/unittests/general/web_middleware_test.py @@ -3,7 +3,7 @@ from django.test import RequestFactory -from nav.web.auth.utils import ACCOUNT_ID_VAR, set_account, ensure_account +from nav.web.auth.utils import ACCOUNT_ID_VAR, set_account from nav.web.auth.sudo import SUDOER_ID_VAR from nav.web.auth.middleware import AuthenticationMiddleware from nav.web.auth.middleware import AuthorizationMiddleware @@ -33,45 +33,6 @@ def test_set_account(fake_session): assert request.session[ACCOUNT_ID_VAR] == DEFAULT_ACCOUNT.id -class TestEnsureAccount(object): - def test_account_is_set_if_missing(self, fake_session): - r = RequestFactory() - request = r.get('/') - request.session = fake_session - with patch("nav.web.auth.Account.objects.get", return_value=DEFAULT_ACCOUNT): - ensure_account(request) - assert ( - auth.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[auth.ACCOUNT_ID_VAR] - ), 'Correct user not set' - - def test_account_is_switched_to_default_if_locked(self, fake_session): - r = RequestFactory() - request = r.get('/') - request.session = fake_session - request.session[auth.ACCOUNT_ID_VAR] = LOCKED_ACCOUNT.id - with patch( - "nav.web.auth.Account.objects.get", - side_effect=[LOCKED_ACCOUNT, DEFAULT_ACCOUNT], - ): - ensure_account(request) - assert request.session[auth.ACCOUNT_ID_VAR] == DEFAULT_ACCOUNT.id - assert request.account == DEFAULT_ACCOUNT, 'Correct user not set' - - def test_account_is_left_alone_if_ok(self, fake_session): - r = RequestFactory() - request = r.get('/') - request.session = fake_session - request.session[auth.ACCOUNT_ID_VAR] = return_value = PLAIN_ACCOUNT.id - with patch("nav.web.auth.Account.objects.get", return_value=PLAIN_ACCOUNT): - ensure_account(request) - assert request.account == PLAIN_ACCOUNT - assert request.session[auth.ACCOUNT_ID_VAR] == PLAIN_ACCOUNT.id - - class TestAuthenticationMiddleware(object): def test_process_request_logged_in(self, fake_session): r = RequestFactory() @@ -215,18 +176,6 @@ def test_logout_before_login(self): result = logout(fake_request) assert result == None - def test_non_sudo_logout(self, fake_session): - r = RequestFactory() - fake_request = r.get('/anyurl') - fake_session[ACCOUNT_ID_VAR] = PLAIN_ACCOUNT.id - fake_request.session = fake_session - fake_request.account = PLAIN_ACCOUNT - with patch('nav.web.auth.LogEntry.add_log_entry'): - result = logout(fake_request) - assert result == '/' - assert not hasattr(fake_request, 'account') - assert ACCOUNT_ID_VAR not in fake_request.session - def test_sudo_logout(self, fake_session): r = RequestFactory() fake_request = r.post('/anyurl', data={'submit_desudo': True})