SSO user account sync (#1279)

* WIP user sync

* Add tests for validation

* Add user sync tests

* Test user needs update check

misago/sso/client.py

@@ -1,11 +1,10 @@
-from django.contrib.auth import get_user_model
+from django.core.exceptions import SuspiciousOperation
 from django.http import Http404
 from simple_sso.sso_client.client import AuthenticateView, Client, LoginView
 
 from ..users.authbackends import MisagoBackend
-from ..users.setupnewuser import setup_new_user
-
-User = get_user_model()
+from .user import get_or_create_user
+from .validators import UserDataValidator
 
 
 class MisagoAuthenticateView(AuthenticateView):
@@ -50,10 +49,8 @@ class ClientMisago(Client):
         self.backend = "%s.%s" % (MisagoBackend.__module__, MisagoBackend.__name__)
 
     def build_user(self, user_data):
-        try:
-            return User.objects.get_by_email(user_data["email"])
-        except User.DoesNotExist:
-            user = User.objects.create_user(user_data["username"], user_data["email"])
-            user.update_acl_key()
-            setup_new_user(self.request.settings, user)
-            return user
+        validator = UserDataValidator(user_data)
+        if not validator.is_valid():
+            failed_fields = ", ".join(validator.errors.keys())
+            raise SuspiciousOperation(f"User data failed to validate: {failed_fields}")
+        return get_or_create_user(self.request, validator.cleaned_data)

misago/sso/tests.py → misago/sso/tests/test_sso_views.py

@@ -1,6 +1,6 @@
 from urllib.parse import urlparse
 
-from itsdangerous.timed import TimestampSigner
+from itsdangerous.timed import TimedSerializer, TimestampSigner
 from requests import Response
 from requests.sessions import Session
 
@@ -9,7 +9,7 @@ from django.shortcuts import reverse
 from django.test import override_settings, TestCase
 from django.utils.timezone import now
 
-from ..conf.test import override_dynamic_settings
+from ...conf.test import override_dynamic_settings
 
 User = get_user_model()
 
@@ -20,12 +20,18 @@ TEST_SSO_SETTINGS = {
     "sso_server": "http://example.com/server/",
 }
 
-SSO_USER_EMAIL = "jkowalski@example.com"
+SSO_USER_ID = 1
+
+
+def create_verify_response(data):
+    signer = TimedSerializer(TEST_SSO_SETTINGS["sso_private_key"])
+    return signer.dumps(data)
 
 
 class ConnectionMock:
-    def __init__(self):
+    def __init__(self, user_data=None):
         self.session = Session
+        self.user_data = user_data
 
     def __enter__(self):
         self.origin_post = Session.post
@@ -36,18 +42,25 @@ class ConnectionMock:
             if "/server/request-token/" == urlparse(requested_url).path:
                 # token generated for private key settings.SSO_PRIVATE_KEY = 'priv1'
                 mocked_response._content = (
-                    b'{"request_token": "XcHtuemqcjnIT6J2WHTFswLQP0W07nI96XfxqGkm6b1zFToF0YGEoIYu3'
-                    b'7QOajkc"}.XTd9sA.quRsXFxqMk-ufwSc79q-_YLDNzg'
+                    b'{"request_token": "XcHtuemqcjnIT6J2WHTFswLQP0W07nI96XfxqGkm6b1zFT'
+                    b'oF0YGEoIYu37QOajkc"}.XTd9sA.quRsXFxqMk-ufwSc79q-_YLDNzg'
                 )
             elif "/server/verify/" == urlparse(requested_url).path:
-                mocked_response._content = (
-                    (
-                        '{"username": "jkowalski", "email": "%s", "first_name": '
-                        '"Jan", "last_name": "Kowalski", "is_staff": false, "is_superuser": false, '
-                        '"is_active": true}.XTg4IQ._cANZR5jHvtwhNzcnNYDfE1nLHE'
-                    )
-                    % SSO_USER_EMAIL
-                ).encode("utf-8")
+                user_data = {
+                    "id": SSO_USER_ID,
+                    "username": "jkowalski",
+                    "email": "jkowalski@example.com",
+                    "first_name": "Jan",
+                    "last_name": "Kowalski",
+                    "is_staff": False,
+                    "is_superuser": False,
+                    "is_active": True,
+                }
+
+                if self.user_data:
+                    user_data.update(self.user_data)
+
+                mocked_response._content = create_verify_response(user_data)
 
             mocked_response.status_code = 200
             return mocked_response
@@ -69,7 +82,7 @@ class TimestampSignerMock:
         def mocked_unsign(*args, **kwargs):
             s = args[1]
             if b'"username": "jkowalski"' in s:
-                value = s[:166]  # {...}
+                value = s[: s.index(b"}.") + 1]  # {...}
                 timestamp_to_datetime = now()
                 return value, timestamp_to_datetime
             else:
@@ -141,7 +154,7 @@ def test_sso_auth_view_creates_new_user(db, client):
 
 @override_dynamic_settings(**TEST_SSO_SETTINGS)
 def test_sso_auth_view_authenticates_existing_user(user, client):
-    user.set_email(SSO_USER_EMAIL)
+    user.sso_id = SSO_USER_ID
     user.save()
 
     url_to_authenticate = reverse("simple-sso-authenticate")
@@ -161,3 +174,46 @@ def test_sso_auth_view_authenticates_existing_user(user, client):
     assert response.url == "/"
 
     assert User.objects.count() == 1
+
+
+@override_dynamic_settings(**TEST_SSO_SETTINGS)
+def test_sso_auth_view_updates_existing_user_using_data_from_sso(user, client):
+    user.sso_id = SSO_USER_ID
+    user.is_active = False
+    user.save()
+
+    url_to_authenticate = reverse("simple-sso-authenticate")
+    assert url_to_authenticate == "/sso/client/authenticate/"
+
+    query = (
+        "next=%2F&access_token=InBBMjllMlNla2ZWdDdJMnR0c3R3QWIxcjQwRzV6TmphZDRSaEprbjlMbnR0TnF"
+        "Ka3Q2d1dNR1lVYkhzVThvZU0i.XTeRVQ.3XiIMg0AFcJKDFCekse6s43uNLI"
+    )
+    url_to_authenticate += "?" + query
+
+    with ConnectionMock():
+        with TimestampSignerMock():
+            client.get(url_to_authenticate)
+
+    user.refresh_from_db()
+    assert user.username == "jkowalski"
+    assert user.email == "jkowalski@example.com"
+    assert user.is_active is True
+
+
+@override_dynamic_settings(**TEST_SSO_SETTINGS)
+def test_sso_auth_view_returns_bad_request_error_for_invalid_user_data(db, client):
+
+    url_to_authenticate = reverse("simple-sso-authenticate")
+    assert url_to_authenticate == "/sso/client/authenticate/"
+
+    query = (
+        "next=%2F&access_token=InBBMjllMlNla2ZWdDdJMnR0c3R3QWIxcjQwRzV6TmphZDRSaEprbjlMbnR0TnF"
+        "Ka3Q2d1dNR1lVYkhzVThvZU0i.XTeRVQ.3XiIMg0AFcJKDFCekse6s43uNLI"
+    )
+    url_to_authenticate += "?" + query
+
+    with ConnectionMock({"email": "invalid"}):
+        with TimestampSignerMock():
+            response = client.get(url_to_authenticate)
+            assert response.status_code == 400

misago/sso/tests/test_user_data_validation.py

@@ -0,0 +1,60 @@
+from ...users.models import Ban
+from ..validators import UserDataValidator
+
+
+def test_valid_data_passess_validation(db):
+    validator = UserDataValidator(
+        {"id": 1, "username": "User", "email": "user@example.com"}
+    )
+    assert validator.is_valid()
+    assert "id" not in validator.errors
+    assert "username" not in validator.errors
+    assert "email" not in validator.errors
+
+
+def test_invalid_id_fails_validation(db):
+    validator = UserDataValidator(
+        {"id": "invalid", "username": "User", "email": "user@example.com"}
+    )
+    assert not validator.is_valid()
+    assert list(validator.errors) == ["id"]
+
+
+def test_invalid_username_fails_validation(db):
+    validator = UserDataValidator(
+        {"id": 1, "username": "User!", "email": "user@example.com"}
+    )
+    assert not validator.is_valid()
+    assert list(validator.errors) == ["username"]
+
+
+def test_banned_username_fails_validation(db):
+    Ban.objects.create(check_type=Ban.USERNAME, banned_value="User")
+    validator = UserDataValidator(
+        {"id": 1, "username": "User", "email": "user@example.com"}
+    )
+    assert not validator.is_valid()
+    assert list(validator.errors) == ["username"]
+
+
+def test_invalid_email_fails_validation(db):
+    validator = UserDataValidator({"id": 1, "username": "User", "email": "invalid"})
+    assert not validator.is_valid()
+    assert list(validator.errors) == ["email"]
+
+
+def test_banned_email_fails_validation(db):
+    Ban.objects.create(check_type=Ban.EMAIL, banned_value="user@example.com")
+    validator = UserDataValidator(
+        {"id": 1, "username": "User", "email": "user@example.com"}
+    )
+    assert not validator.is_valid()
+    assert list(validator.errors) == ["email"]
+
+
+def test_is_active_flag_can_be_included_in_data(db):
+    validator = UserDataValidator(
+        {"id": 1, "username": "User", "email": "user@example.com", "is_active": True}
+    )
+    assert validator.is_valid()
+    assert validator.cleaned_data["is_active"] is True

misago/sso/tests/test_user_syncing.py

@@ -0,0 +1,85 @@
+from unittest.mock import Mock
+
+import pytest
+
+from ..user import get_or_create_user, user_needs_updating
+
+SSO_ID = 1
+
+
+@pytest.fixture
+def sso_user(user):
+    user.sso_id = SSO_ID
+    user.save()
+    return user
+
+
+def test_new_user_is_created_if_user_with_given_sso_id_doesnt_exist(dynamic_settings):
+    user = get_or_create_user(
+        Mock(settings=dynamic_settings),
+        {
+            "id": SSO_ID,
+            "username": "SsoUser",
+            "email": "ssouser@example.com",
+            "is_active": False,
+        },
+    )
+
+    assert user.sso_id == SSO_ID
+    assert user.username == "SsoUser"
+    assert user.email == "ssouser@example.com"
+    assert user.is_active is False
+
+
+def test_user_with_sso_id_is_returned_if_they_exist(dynamic_settings, sso_user):
+    user = get_or_create_user(
+        Mock(settings=dynamic_settings),
+        {"id": SSO_ID, "username": "SsoUser", "email": "ssouser@example.com"},
+    )
+
+    assert user == sso_user
+
+
+def test_user_with_sso_id_is_updated_using_provider_data(dynamic_settings, sso_user):
+    user = get_or_create_user(
+        Mock(settings=dynamic_settings),
+        {
+            "id": SSO_ID,
+            "username": "SsoUser",
+            "email": "ssouser@example.com",
+            "is_active": False,
+        },
+    )
+
+    sso_user.refresh_from_db()
+    assert sso_user.username == "SsoUser"
+    assert sso_user.email == "ssouser@example.com"
+    assert sso_user.is_active is False
+
+
+def test_user_needs_updating_if_their_username_changed(user):
+    assert user_needs_updating(user, {"username": "Changed", "email": user.email})
+
+
+def test_user_needs_updating_if_their_email_changed(user):
+    assert user_needs_updating(
+        user, {"username": user.username, "email": "changed@example.com"}
+    )
+
+
+def test_user_needs_updating_if_their_active_status_changed(user):
+    assert user_needs_updating(
+        user,
+        {
+            "username": user.username,
+            "email": user.email,
+            "is_active": not user.is_active,
+        },
+    )
+
+
+def test_user_doesnt_need_updating_if_their_data_is_same(user):
+    assert not user_needs_updating(
+        user,
+        {"username": user.username, "email": user.email, "is_active": user.is_active},
+    )

misago/sso/user.py

@@ -0,0 +1,43 @@
+from django.contrib.auth import get_user_model
+
+from ..users.setupnewuser import setup_new_user
+
+User = get_user_model()
+
+
+def get_or_create_user(request, user_data):
+    try:
+        user = User.objects.get(sso_id=user_data["id"])
+        if user_needs_updating(user, user_data):
+            update_user(user, user_data)
+        return user
+    except User.DoesNotExist:
+        user = User.objects.create_user(
+            user_data["username"],
+            user_data["email"],
+            is_active=user_data.get("is_active", True),
+            sso_id=user_data["id"],
+        )
+        user.update_acl_key()
+        setup_new_user(request.settings, user)
+        return user
+
+
+def user_needs_updating(user, user_data):
+    return any(
+        (
+            user.username != user_data["username"],
+            user.email != user_data["email"],
+            user.is_active != user_data.get("is_active", user.is_active),
+        )
+    )
+
+
+def update_user(user, user_data):
+    if user.username != user_data["username"]:
+        user.set_username(user_data["username"])
+    if user.email != user_data["email"]:
+        user.set_email(user_data["email"])
+    if user.is_active != user_data.get("is_active", user.is_active):
+        user.is_active = user_data["is_active"]
+    user.save()

misago/sso/validators.py

@@ -0,0 +1,35 @@
+from django import forms
+from django.core.exceptions import ValidationError
+from django.utils.translation import gettext as _
+
+from ..users.validators import (
+    validate_username_content,
+    validate_username_banned,
+    validate_email_content,
+    validate_email_banned,
+)
+
+
+class UserDataValidator(forms.Form):
+    id = forms.IntegerField(min_value=1)
+    username = forms.CharField()
+    email = forms.CharField()
+    is_active = forms.BooleanField(required=False)
+
+    def clean_username(self):
+        data = self.cleaned_data["username"]
+
+        validate_username_content(data)
+        if validate_username_banned(data):
+            raise ValidationError(_("This username is not allowed."))
+
+        return data
+
+    def clean_email(self):
+        data = self.cleaned_data["email"]
+
+        validate_email_content(data)
+        if validate_email_banned(data):
+            raise ValidationError(_("This e-mail address is not allowed."))
+
+        return data

misago/users/migrations/0021_user_sso_id.py

@@ -0,0 +1,16 @@
+# Generated by Django 2.2.3 on 2019-08-15 16:58
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [("misago_users", "0020_set_dj_partial_indexes")]
+
+    operations = [
+        migrations.AddField(
+            model_name="user",
+            name="sso_id",
+            field=models.PositiveIntegerField(blank=True, null=True, unique=True),
+        )
+    ]

misago/users/models/user.py

@@ -216,6 +216,8 @@ class User(AbstractBaseUser, PermissionsMixin):
     profile_fields = HStoreField(default=dict)
     agreements = ArrayField(models.PositiveIntegerField(), default=list)
 
+    sso_id = models.PositiveIntegerField(null=True, blank=True, unique=True)
+
     USERNAME_FIELD = "slug"
     REQUIRED_FIELDS = ["email"]