Merge pull request #2828 from stveit/flush-session-on-logout

Flush session on logout

Author: stveit

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')

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

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)

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

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)

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()

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()

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

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})