Disallow password login via crafted requests

evap/evaluation/auth.py

@@ -8,7 +8,7 @@
 from mozilla_django_oidc.auth import OIDCAuthenticationBackend
 
 from evap.evaluation.models import UserProfile
-from evap.evaluation.tools import clean_email
+from evap.evaluation.tools import clean_email, openid_login_is_active, password_login_is_active
 from evap.rewards.tools import can_reward_points_be_used_by
 
 
@@ -36,6 +36,7 @@ def authenticate(self, request, key):  # pylint: disable=arguments-differ
 class EmailAuthenticationBackend(ModelBackend):
     # https://docs.djangoproject.com/en/3.1/topics/auth/customizing/#writing-an-authentication-backend
     def authenticate(self, request, email=None, password=None):  # pylint: disable=arguments-differ,arguments-renamed
+        assert password_login_is_active()
         try:
             user = UserProfile.objects.get(email=email)
         except UserProfile.DoesNotExist:
@@ -133,6 +134,7 @@ def reward_user_required(user):
 # see https://mozilla-django-oidc.readthedocs.io/en/stable/
 class OpenIDAuthenticationBackend(OIDCAuthenticationBackend):
     def filter_users_by_claims(self, claims):
+        assert openid_login_is_active()
         email = claims.get("email")
         if not email:
             return []
@@ -143,13 +145,15 @@ def filter_users_by_claims(self, claims):
             return []
 
     def create_user(self, claims):
+        assert openid_login_is_active()
         return self.UserModel.objects.create(
             email=claims.get("email"),
             first_name_given=claims.get("given_name", ""),
             last_name=claims.get("family_name", ""),
         )
 
     def update_user(self, user, claims):
+        assert openid_login_is_active()
         if not user.first_name_given:
             user.first_name_given = claims.get("given_name", "")
             user.save()

evap/evaluation/models.py

@@ -38,6 +38,7 @@
     date_to_datetime,
     is_external_email,
     is_prefetched,
+    password_login_is_active,
     translate,
     vote_end_datetime,
 )
@@ -1735,6 +1736,11 @@ class Meta:
     def save(self, *args, **kwargs):
         # This is not guaranteed to be called on every insert. For example, the importers use bulk insertion.
 
+        if self.has_usable_password and not password_login_is_active():
+            # We don't want this to happen, but if it happens, it shouldn't be a showstopper since password login
+            # isn't possible anyway -> if triggered, debug how the situation came to be, and prevent that
+            logger.warning("User %s has a usable password set while password login is disabled", self)
+
         self.email = clean_email(self.email)
         super().save(*args, **kwargs)
 

evap/evaluation/tests/test_auth.py

@@ -2,7 +2,8 @@
 from unittest.mock import patch
 
 from django.conf import settings
-from django.contrib.auth.models import Group
+from django.contrib.auth.hashers import make_password
+from django.contrib.auth.models import AnonymousUser, Group
 from django.core import mail
 from django.core.exceptions import PermissionDenied
 from django.http import HttpRequest, HttpResponse
@@ -20,6 +21,7 @@
 @override_settings(PASSWORD_HASHERS=["django.contrib.auth.hashers.MD5PasswordHasher"])
 class LoginTests(WebTest):
     csrf_checks = False
+    url = reverse("evaluation:index")
 
     @classmethod
     def setUpTestData(cls):
@@ -60,6 +62,7 @@ def test_login_url_works(self):
         self.assertContains(page, self.external_user.full_name)
 
         page = self.app.post(url_with_key).follow().follow()
+        self.assertEqual(page.context["user"], self.external_user)
         self.assertContains(page, "Logout")
         self.assertContains(page, self.external_user.full_name)
 
@@ -69,9 +72,11 @@ def test_login_key_valid_only_once(self):
 
         url_with_key = reverse("evaluation:login_key_authentication", args=[self.external_user.login_key])
         page = self.app.post(url_with_key).follow().follow()
+        self.assertEqual(page.context["user"], self.external_user)
         self.assertContains(page, "Logout")
 
         page = page.forms["logout-form"].submit().follow()
+        self.assertIsInstance(page.context["user"], AnonymousUser)
         self.assertNotContains(page, "Logout")
 
         page = self.app.get(url_with_key).follow()
@@ -87,11 +92,12 @@ def test_inactive_external_users_can_not_login(self):
             reverse("evaluation:login_key_authentication", args=[self.inactive_external_user.login_key])
         ).follow()
         self.assertContains(page, "Inactive users are not allowed to login")
+        self.assertIsInstance(page.context["user"], AnonymousUser)
         self.assertNotContains(page, "Logout")
 
     def test_login_key_resend_if_still_valid(self):
         old_key = self.external_user.login_key
-        page = self.app.post("/", params={"submit_type": "new_key", "email": self.external_user.email}).follow()
+        page = self.app.post(self.url, params={"submit_type": "new_key", "email": self.external_user.email}).follow()
         new_key = UserProfile.objects.get(id=self.external_user.id).login_key
 
         self.assertEqual(old_key, new_key)
@@ -104,9 +110,9 @@ def test_login_key_resend_if_still_valid(self):
     )
     def test_oidc_login(self):
         # This should send them to /oidc/authenticate
-        page = self.app.get("/").click("Login")
+        page = self.app.get(self.url).click("Login")
 
-        # which should then redirect them to OIDC_OP_AUTHORIZTATION_ENDPOINT
+        # which should then redirect them to OIDC_OP_AUTHORIZATION_ENDPOINT
         location = page.headers["location"]
         self.assertIn(settings.OIDC_OP_AUTHORIZATION_ENDPOINT, location)
 
@@ -128,15 +134,46 @@ def test_oidc_login(self):
         # Thus, at this point, the user should be logged in and be redirected back to the start page.
         location = page.headers["location"]
         parse_result = urllib.parse.urlparse(location)
-        self.assertEqual(parse_result.path, "/")
+        self.assertEqual(parse_result.path, self.url)
 
         page = self.app.get(location)
         # A GET here should then redirect to the users real start page.
         # This should be a 403 since the user is external and has no course participation
         page = page.follow(status=403)
 
-        # user should see the Logout button then.
         self.assertIn("Logout", page.body.decode())
+        self.assertEqual(page.context["user"], user)
+
+    @override_settings(INSTITUTION_EMAIL_DOMAINS=["example.com"])
+    def test_passworduser_login(self):
+        """Tests whether a user can login with an incorrect and a correct password."""
+        user = baker.make(UserProfile, email="user@example.com", password=make_password("evap"))
+
+        # wrong password
+        with override_settings(ACTIVATE_OPEN_ID_LOGIN=False):
+            page = self.app.get(self.url)
+            password_form = page.forms["email-login-form"]
+            password_form["email"] = user.email
+            password_form["password"] = "asd"  # nosec
+            response = password_form.submit()
+            self.assertIsInstance(response.context["user"], AnonymousUser)
+            self.assertNotContains(response, "Logout")
+
+        # correct password while password login is disabled
+        with override_settings(ACTIVATE_OPEN_ID_LOGIN=True):
+            self.assertFalse(auth.password_login_is_active())
+
+            password_form["password"] = "evap"  # nosec
+            response = password_form.submit(status=400)
+            self.assertFalse(response.context["user"])
+
+        # correct password while password login is enabled
+        with override_settings(ACTIVATE_OPEN_ID_LOGIN=False):
+            self.assertTrue(auth.password_login_is_active())
+
+            response = password_form.submit(status=302).follow().follow()
+            self.assertEqual(response.context["user"], user)
+            self.assertContains(response, "Logout")
 
 
 @override_settings(PASSWORD_HASHERS=["django.contrib.auth.hashers.MD5PasswordHasher"])

evap/evaluation/tests/test_views.py

@@ -28,18 +28,6 @@ def render_pages(self):
 class TestIndexView(WebTest):
     url = "/"
 
-    @override_settings(ACTIVATE_OPEN_ID_LOGIN=False)
-    def test_passworduser_login(self):
-        """Tests whether a user can login with an incorrect and a correct password."""
-        baker.make(UserProfile, email="password.user", password=make_password("evap"))
-        response = self.app.get(self.url)
-        password_form = response.forms["email-login-form"]
-        password_form["email"] = "password.user"
-        password_form["password"] = "asd"  # nosec
-        password_form.submit()
-        password_form["password"] = "evap"  # nosec
-        password_form.submit(status=302)
-
     @override_settings(ACTIVATE_OPEN_ID_LOGIN=False)
     def test_login_for_staff_users_correctly_redirects(self):
         """Regression test for #1523: Access denied on manager login"""

evap/evaluation/tools.py

@@ -33,6 +33,14 @@
 CV = TypeVar("CV", bound=CellValue)
 
 
+def openid_login_is_active() -> bool:
+    return settings.ACTIVATE_OPEN_ID_LOGIN
+
+
+def password_login_is_active() -> bool:
+    return not openid_login_is_active()
+
+
 def unordered_groupby(key_value_pairs: Iterable[tuple[Key, Value]]) -> dict[Key, list[Value]]:
     """
     We need this in several places: Take list of (key, value) pairs and make

evap/evaluation/views.py

@@ -17,7 +17,7 @@
 
 from evap.evaluation.forms import LoginEmailForm, NewKeyForm, NotebookForm, ProfileForm
 from evap.evaluation.models import EmailTemplate, FaqSection, Semester, UserProfile
-from evap.evaluation.tools import HttpResponseNoContent
+from evap.evaluation.tools import HttpResponseNoContent, openid_login_is_active, password_login_is_active
 from evap.middleware import no_login_required
 
 logger = logging.getLogger(__name__)
@@ -61,6 +61,10 @@ def index(request):
     # parse the form data into the respective form
     submit_type = request.POST.get("submit_type", "no_submit")
     new_key_form = NewKeyForm(request.POST if submit_type == "new_key" else None)
+
+    if submit_type == "login_email" and not password_login_is_active():
+        raise SuspiciousOperation
+
     login_email_form = LoginEmailForm(request, request.POST if submit_type == "login_email" else None)
 
     # process form data
@@ -77,6 +81,7 @@ def index(request):
             return redirect("evaluation:index")
 
         if login_email_form.is_valid():
+            assert password_login_is_active()
             # user would like to login with email and password and passed password test
             auth.login(request, login_email_form.get_user())
 
@@ -97,7 +102,7 @@ def index(request):
         template_data = {
             "new_key_form": new_key_form,
             "login_email_form": login_email_form,
-            "openid_active": settings.ACTIVATE_OPEN_ID_LOGIN,
+            "openid_active": openid_login_is_active(),
         }
         return render(request, "index.html", template_data)