Store the chosen IPA server in the session for both the user and admi…

…n client

This prevents admin commands from running on a server and user commands
running on another.

Fixes: #1079

Signed-off-by: Aurélien Bompard <[email protected]>

news/1079.bug

@@ -0,0 +1,3 @@
+Store the chosen IPA server in the session for both the user client and the
+admin client. This prevents admin commands from running on a server and user
+commands running on another.

noggin/controller/authentication.py

@@ -64,7 +64,7 @@ def otp_sync():
     if form.validate_on_submit():
         with handle_form_errors(form):
             try:
-                ipa = untouched_ipa_client(current_app)
+                ipa = untouched_ipa_client(current_app, session)
                 ipa.otptoken_sync(
                     user=form.username.data,
                     password=form.password.data,

noggin/controller/password.py

@@ -38,7 +38,7 @@
 
 def _validate_change_pw_form(form, username, ipa=None):
     if ipa is None:
-        ipa = untouched_ipa_client(current_app)
+        ipa = untouched_ipa_client(current_app, session)
 
     current_password = form.current_password.data
     password = form.password.data
@@ -236,7 +236,7 @@ def forgot_password_change():
             # example)
             ipa_admin.user_mod(username, userpassword=temp_password)
             # Change the password as the user, so it's not expired.
-            ipa = untouched_ipa_client(current_app)
+            ipa = untouched_ipa_client(current_app, session)
             ipa.change_password(
                 username,
                 new_password=password,

noggin/controller/registration.py

@@ -264,7 +264,7 @@ def activate_account():
                 # First, set it as an admin. This will mark it as expired.
                 ipa_admin.user_mod(user.username, userpassword=password)
                 # And now we set it again as the user, so it is not expired any more.
-                ipa = untouched_ipa_client(current_app)
+                ipa = untouched_ipa_client(current_app, session)
                 ipa.change_password(
                     user.username, new_password=password, old_password=password
                 )

noggin/security/ipa.py

@@ -123,12 +123,26 @@ def raise_on_failed(result):
     raise ValidationError(failed)
 
 
+def choose_server(app, session=None):
+    """
+    Choose a server among the configured IPA server and store the result in the session.
+    """
+    server = None
+    if session is not None:
+        server = session.get('noggin_ipa_server_hostname', None)
+    if server is None:
+        server = random.choice(app.config['FREEIPA_SERVERS'])
+    if session is not None:
+        session['noggin_ipa_server_hostname'] = server
+    return server
+
+
 # Construct an IPA client from app config, but don't attempt to log in with it
 # or to form a session of any kind with it. This is useful for one-off cases
 # like password resets where a session isn't actually required.
-def untouched_ipa_client(app):
+def untouched_ipa_client(app, session):
     return Client(
-        random.choice(app.config['FREEIPA_SERVERS']),
+        choose_server(app, session),
         verify_ssl=app.config['FREEIPA_CACERT'],
     )
 
@@ -142,7 +156,7 @@ def untouched_ipa_client(app):
 # It will be None if no session was provided or was provided but invalid.
 def maybe_ipa_session(app, session):
     encrypted_session = session.get('noggin_session', None)
-    server_hostname = session.get('noggin_ipa_server_hostname', None)
+    server_hostname = choose_server(app, session)
     if encrypted_session and server_hostname:
         fernet = Fernet(app.config['FERNET_SECRET'])
         ipa_session = fernet.decrypt(encrypted_session)
@@ -174,8 +188,9 @@ def maybe_ipa_login(app, session, username, userpassword):
     # in the session and just always use that. Flask sessions are signed, so we
     # are safe in later assuming that the server hostname cookie has not been
     # altered.
-    chosen_server = random.choice(app.config['FREEIPA_SERVERS'])
-    client = Client(chosen_server, verify_ssl=app.config['FREEIPA_CACERT'])
+    client = Client(
+        choose_server(app, session), verify_ssl=app.config['FREEIPA_CACERT']
+    )
 
     auth = client.login(username, userpassword)
 
@@ -185,7 +200,6 @@ def maybe_ipa_login(app, session, username, userpassword):
             bytes(client._session.cookies['ipa_session'], 'utf8')
         )
         session['noggin_session'] = encrypted_session
-        session['noggin_ipa_server_hostname'] = chosen_server
         session['noggin_username'] = username
         return client
 

noggin/security/ipa_admin.py

@@ -1,9 +1,8 @@
-import random
 from functools import wraps
 
-from flask import current_app
+from flask import current_app, session
 
-from .ipa import Client
+from .ipa import choose_server, Client
 
 
 class IPAAdmin:
@@ -66,7 +65,7 @@ def __maybe_ipa_admin_session(self):
         username = current_app.extensions["ipa-admin"]["username"]
         password = current_app.extensions["ipa-admin"]["password"]
         client = Client(
-            random.choice(current_app.config['FREEIPA_SERVERS']),
+            choose_server(current_app, session),
             verify_ssl=current_app.config['FREEIPA_CACERT'],
         )
         client.login(username, password)

tests/unit/conftest.py

@@ -4,6 +4,7 @@
 
 import pytest
 import python_freeipa
+from flask import session
 from vcr import VCR
 
 from noggin.app import create_app, ipa_admin
@@ -158,7 +159,7 @@ def _make_user(name):
             o_loginshell='/bin/bash',
             fascreationtime=f"{now.isoformat()}Z",
         )
-        ipa = untouched_ipa_client(app)
+        ipa = untouched_ipa_client(app, session)
         ipa.change_password(name, password, password)
         created_users.append(name)
 

tests/unit/controller/test_forgot_password.py

@@ -6,7 +6,7 @@
 import python_freeipa
 from bs4 import BeautifulSoup
 from fedora_messaging import testing as fml_testing
-from flask import current_app
+from flask import current_app, session
 
 from noggin.app import ipa_admin, mailer
 from noggin.representation.user import User
@@ -75,7 +75,7 @@ def dummy_user_no_password(ipa_testing_config, app):
         o_loginshell='/bin/bash',
         fascreationtime=f"{now.isoformat()}Z",
     )
-    # ipa = untouched_ipa_client(app)
+    # ipa = untouched_ipa_client(app, session)
     # ipa.change_password(name, password, password)
 
     yield
@@ -278,7 +278,7 @@ def test_change_too_old(client, token_for_dummy_user, patched_lock):
 def test_change_recent_password_change(
     client, dummy_user, dummy_group, token_for_dummy_user, patched_lock_active
 ):
-    ipa = untouched_ipa_client(current_app)
+    ipa = untouched_ipa_client(current_app, session)
     ipa.change_password("dummy", "dummy_password", "dummy_password")
     result = client.get(f'/forgot-password/change?token={token_for_dummy_user}')
     patched_lock_active["delete"].assert_called_once()

tests/unit/security/test_ipa.py

@@ -8,6 +8,7 @@
 
 from noggin.app import ipa_admin
 from noggin.security.ipa import (
+    choose_server,
     Client,
     maybe_ipa_login,
     maybe_ipa_session,
@@ -23,6 +24,30 @@ def ipa_call_error():
     }
 
 
+def test_choose_server(client, mocker):
+    random = mocker.patch("noggin.security.ipa.random")
+    random.choice.side_effect = ["a.example.test", "b.example.test", "c.example.test"]
+    with client.session_transaction() as sess:
+        server = choose_server(current_app, sess)
+    assert server == "a.example.test"
+    with client.session_transaction() as sess:
+        server = choose_server(current_app, sess)
+    # After a second call it is still the first result
+    assert server == "a.example.test"
+    random.choice.assert_called_once()
+
+
+def test_choose_server_no_session(client, mocker):
+    random = mocker.patch("noggin.security.ipa.random")
+    random.choice.side_effect = ["a.example.test", "b.example.test", "c.example.test"]
+    server = choose_server(current_app)
+    assert server == "a.example.test"
+    server = choose_server(current_app)
+    # If we can't store the value in the session, we'll call random.choice again.
+    assert server == "b.example.test"
+    assert random.choice.call_count == 2
+
+
 @pytest.mark.vcr
 def test_ipa_session_authed(client, logged_in_dummy_user):
     """Check maybe_ipa_session() when a user is logged in"""
@@ -74,10 +99,9 @@ def test_ipa_login(client, dummy_user):
 
 def test_ipa_untouched_client(client):
     with client.session_transaction() as sess:
-        ipa = untouched_ipa_client(current_app)
+        ipa = untouched_ipa_client(current_app, sess)
         assert ipa is not None
         assert 'noggin_session' not in sess
-        assert 'noggin_ipa_server_hostname' not in sess
         assert 'noggin_username' not in sess