From b609aad51e43abb6e6e2b61677439185b80ace34 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Fri, 23 Feb 2024 13:16:23 +0100 Subject: [PATCH 01/22] Add func for clearing session takes a request and clears everything session related from it --- python/nav/web/auth/utils.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/python/nav/web/auth/utils.py b/python/nav/web/auth/utils.py index 05fe0df35a..c4b35bc739 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 From 798eb67d193c0de6168b0b24a092923f4267fbb2 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Fri, 23 Feb 2024 13:16:53 +0100 Subject: [PATCH 02/22] Use clear_session to logout current user --- python/nav/web/auth/__init__.py | 7 ++----- python/nav/web/auth/sudo.py | 5 ++--- 2 files changed, 4 insertions(+), 8 deletions(-) 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..10d187df55 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, ACCOUNT_ID_VAR, 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 From f9dfc83aafc11dd2fe4167da19335b5dad4b5a3d Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Fri, 1 Mar 2024 10:42:46 +0100 Subject: [PATCH 03/22] Log out and clear session if account is locked --- python/nav/web/auth/utils.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/python/nav/web/auth/utils.py b/python/nav/web/auth/utils.py index c4b35bc739..1a17d285dd 100644 --- a/python/nav/web/auth/utils.py +++ b/python/nav/web/auth/utils.py @@ -59,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) From 6741029462e6ba303e9fbcaf5676e2563503569e Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Fri, 1 Mar 2024 12:42:38 +0100 Subject: [PATCH 04/22] Add tests for clear_session --- tests/integration/web/auth/utils_test.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 tests/integration/web/auth/utils_test.py diff --git a/tests/integration/web/auth/utils_test.py b/tests/integration/web/auth/utils_test.py new file mode 100644 index 0000000000..190a40cbcd --- /dev/null +++ b/tests/integration/web/auth/utils_test.py @@ -0,0 +1,24 @@ +from nav.web.auth.utils import set_account, clear_session + + +def test_clear_session_creates_new_session_id(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_clear_session_removes_account_from_request(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_clear_session_clears_session_dict(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() From 6309e7d52c65b6185afb21332d7349b41ae3a837 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Fri, 1 Mar 2024 13:29:57 +0100 Subject: [PATCH 05/22] Move fixture up a step in hierarchy --- tests/integration/web/auth/conftest.py | 10 ++++++++++ tests/integration/web/auth/sudo_test.py | 13 ------------- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/tests/integration/web/auth/conftest.py b/tests/integration/web/auth/conftest.py index ebaa41f645..edcd0add81 100644 --- a/tests/integration/web/auth/conftest.py +++ b/tests/integration/web/auth/conftest.py @@ -15,3 +15,13 @@ def session_request(db): middleware.process_request(session_request) session_request.session.save() return session_request + + +@pytest.fixture() +def other_account(db): + from nav.models.profiles import Account + + account = Account(login="other_user") + 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..f25be0d2d7 100644 --- a/tests/integration/web/auth/sudo_test.py +++ b/tests/integration/web/auth/sudo_test.py @@ -1,10 +1,5 @@ -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( @@ -33,11 +28,3 @@ def test_desudo_should_change_session_id( 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() From d1a4a26ed8ba687bfa5d74a438e1197624e4b363 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Fri, 1 Mar 2024 13:30:33 +0100 Subject: [PATCH 06/22] Add logout integration tests --- tests/integration/web/auth/logout_test.py | 30 +++++++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 tests/integration/web/auth/logout_test.py diff --git a/tests/integration/web/auth/logout_test.py b/tests/integration/web/auth/logout_test.py new file mode 100644 index 0000000000..8f1d7465a1 --- /dev/null +++ b/tests/integration/web/auth/logout_test.py @@ -0,0 +1,30 @@ +from nav.web.auth import logout +from nav.web.auth.utils import ACCOUNT_ID_VAR, set_account +from nav.web.auth.sudo import sudo + + +def test_non_sudo_logout_removes_session_data(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_returns_path_to_index(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_sets_session_to_original_user( + db, session_request, admin_account, other_account +): + # login with admin acount + set_account(session_request, admin_account) + sudo(session_request, other_account) + assert session_request.account is other_account + result = logout(session_request, sudo=True) + assert result == '/' + assert session_request.account == admin_account From f246985ace8e94764bb136401bbe08e9967b7f2c Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Fri, 1 Mar 2024 13:33:49 +0100 Subject: [PATCH 07/22] Remove test that got replaced by integration tests --- tests/unittests/general/web_middleware_test.py | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/tests/unittests/general/web_middleware_test.py b/tests/unittests/general/web_middleware_test.py index 483c4cdcb7..d3c91874ec 100644 --- a/tests/unittests/general/web_middleware_test.py +++ b/tests/unittests/general/web_middleware_test.py @@ -215,18 +215,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}) From b3411c1cf76b9540ec41b7a536ff5fa2df0830bd Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Fri, 1 Mar 2024 13:55:40 +0100 Subject: [PATCH 08/22] Make account fixture not locked --- tests/integration/web/auth/conftest.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/integration/web/auth/conftest.py b/tests/integration/web/auth/conftest.py index edcd0add81..2c38a8b474 100644 --- a/tests/integration/web/auth/conftest.py +++ b/tests/integration/web/auth/conftest.py @@ -22,6 +22,7 @@ def other_account(db): from nav.models.profiles import Account account = Account(login="other_user") + account.set_password("password") account.save() yield account account.delete() From e8dab3368f510377b290a36266020de7c0c72d97 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Fri, 1 Mar 2024 13:56:10 +0100 Subject: [PATCH 09/22] Add locked account fixture --- tests/integration/web/auth/conftest.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/integration/web/auth/conftest.py b/tests/integration/web/auth/conftest.py index 2c38a8b474..8bbe4cfbec 100644 --- a/tests/integration/web/auth/conftest.py +++ b/tests/integration/web/auth/conftest.py @@ -26,3 +26,13 @@ def other_account(db): 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() From 78987963055acd028bf9a3762e1bbce8a5caf9a3 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Fri, 1 Mar 2024 13:58:49 +0100 Subject: [PATCH 10/22] Rename fixture --- tests/integration/web/auth/conftest.py | 2 +- tests/integration/web/auth/logout_test.py | 6 +++--- tests/integration/web/auth/sudo_test.py | 12 ++++-------- 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/tests/integration/web/auth/conftest.py b/tests/integration/web/auth/conftest.py index 8bbe4cfbec..bdb3b00263 100644 --- a/tests/integration/web/auth/conftest.py +++ b/tests/integration/web/auth/conftest.py @@ -18,7 +18,7 @@ def session_request(db): @pytest.fixture() -def other_account(db): +def account(db): from nav.models.profiles import Account account = Account(login="other_user") diff --git a/tests/integration/web/auth/logout_test.py b/tests/integration/web/auth/logout_test.py index 8f1d7465a1..cef712fccd 100644 --- a/tests/integration/web/auth/logout_test.py +++ b/tests/integration/web/auth/logout_test.py @@ -19,12 +19,12 @@ def test_non_sudo_logout_returns_path_to_index(db, session_request, admin_accoun def test_sudo_logout_sets_session_to_original_user( - db, session_request, admin_account, other_account + db, session_request, admin_account, account ): # login with admin acount set_account(session_request, admin_account) - sudo(session_request, other_account) - assert session_request.account is other_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 diff --git a/tests/integration/web/auth/sudo_test.py b/tests/integration/web/auth/sudo_test.py index f25be0d2d7..373c6c9bf7 100644 --- a/tests/integration/web/auth/sudo_test.py +++ b/tests/integration/web/auth/sudo_test.py @@ -2,26 +2,22 @@ from nav.web.auth.sudo import sudo, desudo -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) From 0200f4bcd51b10c197e94ef602a1980fba871cfb Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Fri, 1 Mar 2024 14:01:03 +0100 Subject: [PATCH 11/22] Use conftest fixture instead of defining a new one --- tests/integration/web/auth/middleware_test.py | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) 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() From 7a64d4083a84f2f61a23c6468ace328b28633a87 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Mon, 4 Mar 2024 14:43:19 +0100 Subject: [PATCH 12/22] Port ensure_account tests to integration tests --- .../web/auth/ensure_account_test.py | 28 +++++++++++++ .../unittests/general/web_middleware_test.py | 39 ------------------- 2 files changed, 28 insertions(+), 39 deletions(-) create mode 100644 tests/integration/web/auth/ensure_account_test.py diff --git a/tests/integration/web/auth/ensure_account_test.py b/tests/integration/web/auth/ensure_account_test.py new file mode 100644 index 0000000000..ca3aac51fc --- /dev/null +++ b/tests/integration/web/auth/ensure_account_test.py @@ -0,0 +1,28 @@ +from nav.web.auth import logout, Account +from nav.web.auth.utils import ACCOUNT_ID_VAR, set_account, ensure_account +from nav.web.auth.sudo import sudo + + +def test_account_is_set_if_missing(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_is_switched_to_default_if_locked(session_request, locked_account): + set_account(session_request, locked_account) + ensure_account(session_request) + default_account = Account.objects.get(id=Account.DEFAULT_ACCOUNT) + assert session_request.session[ACCOUNT_ID_VAR] == default_account.id + assert session_request.account == default_account, 'Correct user not set' + + +def test_account_is_left_alone_if_ok(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 diff --git a/tests/unittests/general/web_middleware_test.py b/tests/unittests/general/web_middleware_test.py index d3c91874ec..af729d3646 100644 --- a/tests/unittests/general/web_middleware_test.py +++ b/tests/unittests/general/web_middleware_test.py @@ -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() From 67679a4bf27cb611691958ff516a966c267cb242 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Mon, 4 Mar 2024 15:16:14 +0100 Subject: [PATCH 13/22] Change tests names --- tests/integration/web/auth/ensure_account_test.py | 10 +++++++--- tests/integration/web/auth/logout_test.py | 8 +++++--- tests/integration/web/auth/utils_test.py | 8 +++++--- 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/tests/integration/web/auth/ensure_account_test.py b/tests/integration/web/auth/ensure_account_test.py index ca3aac51fc..17ced3dc33 100644 --- a/tests/integration/web/auth/ensure_account_test.py +++ b/tests/integration/web/auth/ensure_account_test.py @@ -3,7 +3,9 @@ from nav.web.auth.sudo import sudo -def test_account_is_set_if_missing(session_request): +def test_account_should_be_set_if_request_does_not_already_have_an_account( + 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' @@ -13,7 +15,9 @@ def test_account_is_set_if_missing(session_request): ), 'Correct user not set' -def test_account_is_switched_to_default_if_locked(session_request, locked_account): +def test_account_should_be_switched_to_default_if_locked( + session_request, locked_account +): set_account(session_request, locked_account) ensure_account(session_request) default_account = Account.objects.get(id=Account.DEFAULT_ACCOUNT) @@ -21,7 +25,7 @@ def test_account_is_switched_to_default_if_locked(session_request, locked_accoun assert session_request.account == default_account, 'Correct user not set' -def test_account_is_left_alone_if_ok(session_request, account): +def test_account_should_be_unchanged_if_ok(session_request, account): set_account(session_request, account) ensure_account(session_request) assert session_request.account == account diff --git a/tests/integration/web/auth/logout_test.py b/tests/integration/web/auth/logout_test.py index cef712fccd..ce38b3e696 100644 --- a/tests/integration/web/auth/logout_test.py +++ b/tests/integration/web/auth/logout_test.py @@ -3,7 +3,7 @@ from nav.web.auth.sudo import sudo -def test_non_sudo_logout_removes_session_data(db, session_request, admin_account): +def test_non_sudo_logout_should_remove_session_data(db, session_request, admin_account): # login with admin acount set_account(session_request, admin_account) logout(session_request) @@ -11,14 +11,16 @@ def test_non_sudo_logout_removes_session_data(db, session_request, admin_account assert ACCOUNT_ID_VAR not in session_request.session -def test_non_sudo_logout_returns_path_to_index(db, session_request, admin_account): +def test_non_sudo_logout_should_return_path_to_index( + 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_sets_session_to_original_user( +def test_sudo_logout_should_set_session_to_original_user( db, session_request, admin_account, account ): # login with admin acount diff --git a/tests/integration/web/auth/utils_test.py b/tests/integration/web/auth/utils_test.py index 190a40cbcd..ba342a2c3f 100644 --- a/tests/integration/web/auth/utils_test.py +++ b/tests/integration/web/auth/utils_test.py @@ -1,14 +1,16 @@ from nav.web.auth.utils import set_account, clear_session -def test_clear_session_creates_new_session_id(db, session_request): +def test_clear_session_should_create_new_session_id(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_clear_session_removes_account_from_request(db, session_request, admin_account): +def test_clear_session_should_remove_account_from_request( + db, session_request, admin_account +): # login with admin acount set_account(session_request, admin_account) assert session_request.account @@ -16,7 +18,7 @@ def test_clear_session_removes_account_from_request(db, session_request, admin_a assert not hasattr(session_request, "account") -def test_clear_session_clears_session_dict(db, session_request, admin_account): +def test_clear_session_should_clear_session_dict(db, session_request, admin_account): set_account(session_request, admin_account) # Make sure there is something to be deleted assert session_request.session.keys() From 8aa62233bca83c840dd24e4c53c61c8256574fe4 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Mon, 4 Mar 2024 15:25:22 +0100 Subject: [PATCH 14/22] Add fixture for default account --- tests/integration/web/auth/conftest.py | 7 +++++++ tests/integration/web/auth/ensure_account_test.py | 3 +-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/integration/web/auth/conftest.py b/tests/integration/web/auth/conftest.py index bdb3b00263..5b5641c1a9 100644 --- a/tests/integration/web/auth/conftest.py +++ b/tests/integration/web/auth/conftest.py @@ -36,3 +36,10 @@ def locked_account(db): 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/ensure_account_test.py b/tests/integration/web/auth/ensure_account_test.py index 17ced3dc33..a6ed144ef0 100644 --- a/tests/integration/web/auth/ensure_account_test.py +++ b/tests/integration/web/auth/ensure_account_test.py @@ -16,11 +16,10 @@ def test_account_should_be_set_if_request_does_not_already_have_an_account( def test_account_should_be_switched_to_default_if_locked( - session_request, locked_account + session_request, locked_account, default_account ): set_account(session_request, locked_account) ensure_account(session_request) - default_account = Account.objects.get(id=Account.DEFAULT_ACCOUNT) assert session_request.session[ACCOUNT_ID_VAR] == default_account.id assert session_request.account == default_account, 'Correct user not set' From a90a6aa3b42d295460ffa47461efe16b7b8c2196 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Mon, 4 Mar 2024 15:26:05 +0100 Subject: [PATCH 15/22] Add test for session cycling during logout --- tests/integration/web/auth/ensure_account_test.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/integration/web/auth/ensure_account_test.py b/tests/integration/web/auth/ensure_account_test.py index a6ed144ef0..260aab6b81 100644 --- a/tests/integration/web/auth/ensure_account_test.py +++ b/tests/integration/web/auth/ensure_account_test.py @@ -29,3 +29,14 @@ def test_account_should_be_unchanged_if_ok(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( + 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 From be9d219b73fd4dfc5f205d40bb9a2aae6ead121c Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Mon, 4 Mar 2024 15:33:17 +0100 Subject: [PATCH 16/22] Add session cycling tests for logout --- tests/integration/web/auth/logout_test.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/integration/web/auth/logout_test.py b/tests/integration/web/auth/logout_test.py index ce38b3e696..ef4846c338 100644 --- a/tests/integration/web/auth/logout_test.py +++ b/tests/integration/web/auth/logout_test.py @@ -30,3 +30,24 @@ def test_sudo_logout_should_set_session_to_original_user( result = logout(session_request, sudo=True) assert result == '/' assert session_request.account == admin_account + + +def test_non_sudo_logout_should_change_session_id(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( + 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 From ce854422b909a16026b70d419a753ad9f508fc28 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Mon, 4 Mar 2024 15:41:48 +0100 Subject: [PATCH 17/22] Remove unused imports --- tests/integration/web/auth/ensure_account_test.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/integration/web/auth/ensure_account_test.py b/tests/integration/web/auth/ensure_account_test.py index 260aab6b81..a7b3af4e6f 100644 --- a/tests/integration/web/auth/ensure_account_test.py +++ b/tests/integration/web/auth/ensure_account_test.py @@ -1,6 +1,5 @@ -from nav.web.auth import logout, Account +from nav.web.auth import logout from nav.web.auth.utils import ACCOUNT_ID_VAR, set_account, ensure_account -from nav.web.auth.sudo import sudo def test_account_should_be_set_if_request_does_not_already_have_an_account( From 0ce69a1c95d51f1a8a4e790c60ec3a425a02bd8f Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Tue, 5 Mar 2024 14:32:13 +0100 Subject: [PATCH 18/22] Remove more unused imports --- python/nav/web/auth/sudo.py | 2 +- tests/unittests/general/web_middleware_test.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/python/nav/web/auth/sudo.py b/python/nav/web/auth/sudo.py index 10d187df55..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, clear_session +from nav.web.auth.utils import set_account, clear_session _logger = logging.getLogger(__name__) diff --git a/tests/unittests/general/web_middleware_test.py b/tests/unittests/general/web_middleware_test.py index af729d3646..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 From d11f301f65f6b20470be901e0317f033a34f4f48 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Wed, 6 Mar 2024 14:33:22 +0100 Subject: [PATCH 19/22] Put tests in class --- tests/integration/web/auth/utils_test.py | 41 ++++++++++++------------ 1 file changed, 20 insertions(+), 21 deletions(-) diff --git a/tests/integration/web/auth/utils_test.py b/tests/integration/web/auth/utils_test.py index ba342a2c3f..c9a06b3b32 100644 --- a/tests/integration/web/auth/utils_test.py +++ b/tests/integration/web/auth/utils_test.py @@ -1,26 +1,25 @@ from nav.web.auth.utils import set_account, clear_session -def test_clear_session_should_create_new_session_id(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 +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_clear_session_should_remove_account_from_request( - 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_clear_session_should_clear_session_dict(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() + 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() From 2528e5a7e74b3b6aae2ae6456cb19f43bfb0c618 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Wed, 6 Mar 2024 14:42:23 +0100 Subject: [PATCH 20/22] Move tests to other file --- .../web/auth/ensure_account_test.py | 41 ---------------- tests/integration/web/auth/utils_test.py | 48 ++++++++++++++++++- 2 files changed, 47 insertions(+), 42 deletions(-) delete mode 100644 tests/integration/web/auth/ensure_account_test.py diff --git a/tests/integration/web/auth/ensure_account_test.py b/tests/integration/web/auth/ensure_account_test.py deleted file mode 100644 index a7b3af4e6f..0000000000 --- a/tests/integration/web/auth/ensure_account_test.py +++ /dev/null @@ -1,41 +0,0 @@ -from nav.web.auth import logout -from nav.web.auth.utils import ACCOUNT_ID_VAR, set_account, ensure_account - - -def test_account_should_be_set_if_request_does_not_already_have_an_account( - 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( - 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(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( - 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/integration/web/auth/utils_test.py b/tests/integration/web/auth/utils_test.py index c9a06b3b32..fd96d0a8f0 100644 --- a/tests/integration/web/auth/utils_test.py +++ b/tests/integration/web/auth/utils_test.py @@ -1,4 +1,9 @@ -from nav.web.auth.utils import set_account, clear_session +from nav.web.auth.utils import ( + set_account, + clear_session, + ACCOUNT_ID_VAR, + ensure_account, +) class TestClearSession: @@ -23,3 +28,44 @@ def test_should_clear_session_dict(self, db, session_request, admin_account): 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 From 364ff695bd06f5cfb2a59edd6c6ea7e1548ec99a Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Wed, 6 Mar 2024 14:46:52 +0100 Subject: [PATCH 21/22] Move logout tests to new file --- tests/integration/web/auth/auth_test.py | 54 +++++++++++++++++++++++ tests/integration/web/auth/logout_test.py | 53 ---------------------- 2 files changed, 54 insertions(+), 53 deletions(-) create mode 100644 tests/integration/web/auth/auth_test.py delete mode 100644 tests/integration/web/auth/logout_test.py 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/logout_test.py b/tests/integration/web/auth/logout_test.py deleted file mode 100644 index ef4846c338..0000000000 --- a/tests/integration/web/auth/logout_test.py +++ /dev/null @@ -1,53 +0,0 @@ -from nav.web.auth import logout -from nav.web.auth.utils import ACCOUNT_ID_VAR, set_account -from nav.web.auth.sudo import sudo - - -def test_non_sudo_logout_should_remove_session_data(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( - 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( - 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(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( - 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 From 45cea1fee40ea0a66ab786af7499489024fe9573 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Wed, 6 Mar 2024 14:50:45 +0100 Subject: [PATCH 22/22] Fix weird format --- tests/integration/web/auth/utils_test.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/integration/web/auth/utils_test.py b/tests/integration/web/auth/utils_test.py index fd96d0a8f0..a09c00c91c 100644 --- a/tests/integration/web/auth/utils_test.py +++ b/tests/integration/web/auth/utils_test.py @@ -32,9 +32,7 @@ def test_should_clear_session_dict(self, db, session_request, admin_account): class TestEnsureAccount: def test_account_should_be_set_if_request_does_not_already_have_an_account( - self, - db, - session_request, + self, db, session_request ): assert not hasattr(session_request, "account") ensure_account(session_request)