Cycle session ID on login

python/nav/web/auth/remote_user.py

@@ -22,7 +22,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
+from nav.web.auth.utils import set_account
 
 try:
     # Python 3.6+
@@ -122,8 +122,7 @@ def login(request):
         # 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
+            set_account(request, account)
             return account
     return None
 

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
 
 
 _logger = logging.getLogger(__name__)
@@ -41,7 +41,7 @@ def sudo(request, other_user):
         raise SudoNotAdminError()
     original_user = request.account
     request.session[SUDOER_ID_VAR] = original_user.id
-    _set_account(request, other_user)
+    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
@@ -70,7 +70,7 @@ def desudo(request):
 
     del request.session[ACCOUNT_ID_VAR]
     del request.session[SUDOER_ID_VAR]
-    _set_account(request, original_user)
+    set_account(request, original_user)
     _logger.info(
         'DeSudo: "%s" no longer acting as "%s"', original_user, request.account
     )

python/nav/web/auth/utils.py

@@ -31,10 +31,15 @@
 ACCOUNT_ID_VAR = 'account_id'
 
 
-def _set_account(request, account):
+def set_account(request, account, cycle_session_id=True):
+    """Updates request with new account.
+    Cycles the session ID by default to avoid session fixation.
+    """
     request.session[ACCOUNT_ID_VAR] = account.id
     request.account = account
     _logger.debug('Set active account to "%s"', account.login)
+    if cycle_session_id:
+        request.session.cycle_key()
     request.session.save()
 
 
@@ -52,7 +57,8 @@ def ensure_account(request):
         # Assumes nobody has locked it..
         account = Account.objects.get(id=Account.DEFAULT_ACCOUNT)
 
-    _set_account(request, account)
+    # Do not cycle to avoid session_id being changed on every request
+    set_account(request, account, cycle_session_id=False)
 
 
 def authorization_not_required(fullpath):

python/nav/web/webfront/views.py

@@ -37,10 +37,10 @@
 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.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
+from nav.web.auth.utils import set_account
 from nav.web.utils import require_param
 from nav.web.webfront.utils import quick_read, tool_list
 from nav.web.webfront.forms import (
@@ -230,8 +230,7 @@ def do_login(request):
                 LogEntry.add_log_entry(
                     account, 'log-in', '{actor} logged in', before=account
                 )
-                request.session[ACCOUNT_ID_VAR] = account.id
-                request.account = account
+                set_account(request, account)
                 _logger.info("%s successfully logged in", account.login)
                 if not origin:
                     origin = reverse('webfront-index')

tests/integration/conftest.py

@@ -217,17 +217,15 @@ def localhost_using_legacy_db():
     conn.commit()
 
 
-@pytest.fixture(scope='session')
-def client():
+@pytest.fixture(scope='function')
+def client(admin_username, admin_password):
     """Provides a Django test Client object already logged in to the web UI as
     an admin"""
     from django.urls import reverse
 
     client_ = Client()
     url = reverse('webfront-login')
-    username = os.environ.get('ADMINUSERNAME', 'admin')
-    password = os.environ.get('ADMINPASSWORD', 'admin')
-    client_.post(url, {'username': username, 'password': password})
+    client_.post(url, {'username': admin_username, 'password': admin_password})
     return client_
 
 
@@ -373,3 +371,13 @@ def admin_account(db):
     from nav.models.profiles import Account
 
     yield Account.objects.get(id=Account.ADMIN_ACCOUNT)
+
+
+@pytest.fixture(scope='session')
+def admin_username():
+    return os.environ.get('ADMINUSERNAME', 'admin')
+
+
+@pytest.fixture(scope='session')
+def admin_password():
+    return os.environ.get('ADMINPASSWORD', 'admin')

tests/integration/web/auth/conftest.py

@@ -0,0 +1,17 @@
+import pytest
+
+from django.test import RequestFactory
+from django.contrib.sessions.middleware import SessionMiddleware
+
+
+@pytest.fixture()
+def session_request(db):
+    """Request object with a real session"""
+    r = RequestFactory()
+    session_request = r.post('/anyurl')
+
+    # use middleware to make session for session_request
+    middleware = SessionMiddleware()
+    middleware.process_request(session_request)
+    session_request.session.save()
+    return session_request

tests/integration/web/auth/middleware_test.py

@@ -0,0 +1,28 @@
+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
+):
+    pre_login_session_id = session_request.session.session_key
+    with patch(
+        'nav.web.auth.remote_user.get_username', return_value=remote_account.login
+    ):
+        middleware = AuthenticationMiddleware()
+        middleware.process_request(session_request)
+    assert session_request.account == remote_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

@@ -0,0 +1,44 @@
+import pytest
+
+from django.test import RequestFactory
+from django.contrib.sessions.middleware import SessionMiddleware
+
+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
+):
+    # login with admin acount
+    set_account(session_request, admin_account)
+
+    pre_sudo_session_id = session_request.session.session_key
+    sudo(session_request, other_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
+):
+    # login with admin acount
+    set_account(session_request, admin_account)
+
+    sudo(session_request, other_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/webfront_test.py

@@ -66,3 +66,29 @@ def test_set_default_dashboard_with_multiple_previous_defaults_should_succeed(
         AccountDashboard.objects.filter(account=admin_account, is_default=True).count()
         == 1
     )
+
+
+def test_when_logging_in_it_should_change_the_session_id(
+    db, client, admin_username, admin_password
+):
+    login_url = reverse('webfront-login')
+    logout_url = reverse('webfront-logout')
+    # log out first to compare before and after being logged in
+    client.post(logout_url)
+    assert client.session.session_key, "the initial session lacks an ID"
+    session_id_pre_login = client.session.session_key
+    client.post(login_url, {'username': admin_username, 'password': admin_password})
+    session_id_post_login = client.session.session_key
+    assert session_id_post_login != session_id_pre_login
+
+
+def test_non_expired_session_id_should_not_be_changed_on_request_unrelated_to_login(
+    db, client
+):
+    """Client should be fresh and guaranteed to not be expired"""
+    index_url = reverse('webfront-index')
+    assert client.session.session_key, "the initial session lacks an ID"
+    session_id_pre_login = client.session.session_key
+    client.get(index_url)
+    session_id_post_login = client.session.session_key
+    assert session_id_post_login == session_id_pre_login

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, ensure_account
 from nav.web.auth.sudo import SUDOER_ID_VAR
 from nav.web.auth.middleware import AuthenticationMiddleware
 from nav.web.auth.middleware import AuthorizationMiddleware
@@ -29,12 +29,15 @@ def set_expiry(self, *_):
     def save(self, *_):
         pass
 
+    def cycle_key(self, *_):
+        pass
+
 
 def test_set_account():
     r = RequestFactory()
     request = r.get('/')
     request.session = FakeSession()
-    _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'
@@ -88,7 +91,7 @@ def test_process_request_logged_in(self):
         fake_request.session = FakeSession(ACCOUNT_ID_VAR=PLAIN_ACCOUNT.id)
         with patch(
             'nav.web.auth.middleware.ensure_account',
-            side_effect=_set_account(fake_request, PLAIN_ACCOUNT),
+            side_effect=set_account(fake_request, PLAIN_ACCOUNT),
         ):
             AuthenticationMiddleware(lambda x: x).process_request(fake_request)
             assert fake_request.account == PLAIN_ACCOUNT
@@ -102,7 +105,7 @@ def test_process_request_set_sudoer(self):
         )
         with patch(
             'nav.web.auth.middleware.ensure_account',
-            side_effect=_set_account(fake_request, PLAIN_ACCOUNT),
+            side_effect=set_account(fake_request, PLAIN_ACCOUNT),
         ):
             with patch('nav.web.auth.middleware.get_sudoer', return_value=SUDO_ACCOUNT):
                 AuthenticationMiddleware(lambda x: x).process_request(fake_request)
@@ -116,7 +119,7 @@ def test_process_request_not_logged_in(self):
         fake_request.session = FakeSession()
         with patch(
             'nav.web.auth.middleware.ensure_account',
-            side_effect=_set_account(fake_request, DEFAULT_ACCOUNT),
+            side_effect=set_account(fake_request, DEFAULT_ACCOUNT),
         ):
             with patch('nav.web.auth.remote_user.get_username', return_value=None):
                 AuthenticationMiddleware(lambda x: x).process_request(fake_request)
@@ -129,15 +132,15 @@ def test_process_request_log_in_remote_user(self):
         fake_request.session = FakeSession()
         with patch(
             'nav.web.auth.middleware.ensure_account',
-            side_effect=_set_account(fake_request, DEFAULT_ACCOUNT),
+            side_effect=set_account(fake_request, DEFAULT_ACCOUNT),
         ):
             with patch(
                 'nav.web.auth.remote_user.get_username',
                 return_value=PLAIN_ACCOUNT.login,
             ):
                 with patch(
                     'nav.web.auth.remote_user.login',
-                    side_effect=_set_account(fake_request, PLAIN_ACCOUNT),
+                    side_effect=set_account(fake_request, PLAIN_ACCOUNT),
                 ):
                     AuthenticationMiddleware(lambda x: x).process_request(fake_request)
                     assert fake_request.account == PLAIN_ACCOUNT
@@ -149,15 +152,15 @@ def test_process_request_switch_users(self):
         fake_request.session = FakeSession()
         with patch(
             'nav.web.auth.middleware.ensure_account',
-            side_effect=_set_account(fake_request, PLAIN_ACCOUNT),
+            side_effect=set_account(fake_request, PLAIN_ACCOUNT),
         ):
             with patch(
                 'nav.web.auth.remote_user.get_username',
                 return_value=ANOTHER_PLAIN_ACCOUNT.login,
             ):
                 with patch(
                     'nav.web.auth.remote_user.login',
-                    side_effect=_set_account(fake_request, ANOTHER_PLAIN_ACCOUNT),
+                    side_effect=set_account(fake_request, ANOTHER_PLAIN_ACCOUNT),
                 ):
                     with patch('nav.web.auth.logout'):
                         AuthenticationMiddleware(lambda x: x).process_request(

tests/unittests/general/webfront_test.py

@@ -23,6 +23,9 @@ def set_expiry(self, *_):
     def save(self, *_):
         pass
 
+    def cycle_key(self, *_):
+        pass
+
 
 @patch("nav.web.auth.Account.save", new=MagicMock(return_value=True))
 @patch("nav.web.auth.Account.objects.get", new=MagicMock(return_value=LDAP_ACCOUNT))