diff --git a/.env.example b/.env.example index 7e2ec80..7d8e4ef 100755 --- a/.env.example +++ b/.env.example @@ -22,6 +22,7 @@ LDAP_URL=ldaps://ldap.eom.dev # Set to 1 ONLY with ldap:// on a trusted network to skip StartTLS. # Bind and registration passwords will be sent unencrypted. LDAPS stays verified. LDAP_ALLOW_INSECURE=0 +# Service credentials for registration/worker group sync, not Dex login. LDAP_BIND_DN= LDAP_BIND_PASSWORD= LDAP_CA_CERT_FILE= diff --git a/README.md b/README.md index 11ee9c7..ae3453a 100644 --- a/README.md +++ b/README.md @@ -16,8 +16,10 @@ Both payment methods feed the same LDAP group reconciliation and local expiry. 2. Sign-in uses the single Dex LDAP option with Authorization Code + PKCE, state and nonce. Authlib verifies the RS256 ID token, issuer, audience, signature and lifetime. The signed connector/UID must match the configured connector - and LDAP identity; account ownership is anchored to immutable `entryUUID`, - never email alone. + and username. Local account ownership is pinned to the verified Dex issuer + and subject, never email alone. Membership makes **no LDAP connection during + login**, including first login by an existing Dex user. The worker separately + resolves and pins LDAP `entryUUID` before synchronizing paid groups. 3. Authenticated users choose a server-allowlisted recurring Stripe Price. Stripe hosts Checkout and the billing portal. Return URLs never grant access. 4. Stripe webhooks are signature-verified and durably deduplicated. The worker @@ -116,8 +118,8 @@ of these five prices when inspected. preserve subjects in other apps. Add the website `inetOrgPerson` branch to the LDAP filter; this allows those entries to authenticate to all Dex clients, so review that scope. Dex uses signed `federated:id` claims, while the app resolves - UID to LDAP and checks immutable `entryUUID`. RS256 and all discovery endpoints - must be HTTPS on the issuer's origin. Existing lowercase LDAP usernames + UID from verified claims without querying LDAP during login. RS256 and all + discovery endpoints must be HTTPS on the issuer's origin. Existing lowercase LDAP usernames matching the registration syntax can sign in without signup. 5. The supplied tier file contains live Price IDs. Set `STRIPE_LIVE_MODE=1` only with the matching live Stripe API key. For sandbox testing, override @@ -167,6 +169,64 @@ No production credentials or live account changes are made by the test suite. ## Operations and limitations +### Dex-only login and upgrade + +Deploy the new membership image and apply migration `0004_dex_login_identity` +before serving it. **Keep `DEX_ISSUER` unchanged during the upgrade**: the +migration pins existing non-null subjects to the currently configured issuer, +because older records did not store their issuer. Back up the database and +coordinate replacing web and worker together; older code cannot handle accounts +whose directory UUID has not yet been resolved. Do not roll back to the old +schema while unresolved accounts exist. + +- Returning users sign in using their existing issuer/subject binding, without + LDAP or payment-provider calls. Signed verified email updates their contact + address; it never identifies an account or attaches billing history. +- First-time Dex users with no local username collision receive a free local + account. The worker discovers their LDAP DN/UUID on its next successful group + sync and thereafter uses only that UUID. Missing/ambiguous directory entries + stop synchronization; a missing pinned UUID never falls back to username. +- Website registration still provisions LDAP. Its browser session retains a + one-hour, server-side proof permitting the first Dex login to bind only the + just-created local account, with the same signed connector/username. Successful + login consumes that proof. Without it, existing unbound accounts require an + administrator mapping; matching email or username alone is insufficient. +- For an existing **unbound** account, the management command `bind_dex_identity` + requires `--account` (local account UUID), `--uid` and `--subject` (the exact + Dex `sub` for this issuer/client/connector). Independently verify the identity + with trusted Dex/directory administration before running it. Do not accept + subjects asserted by users or decode an unverified token as proof. It makes + no LDAP calls, preserves directory/billing data and refuses to replace an + existing binding or reuse another account's subject. Lost registration proof + can be recovered through this reviewed mapping, not by repeating registration. +- Issuer, connector, username and subject changes require an explicit identity + migration. The old automatic rebinding of retired `www-ldap` subjects is + intentionally removed; review those bindings separately before deployment. + +The existing Dex LDAP connector uses `idAttr: uid`. **Do not recycle usernames**: +Dex itself cannot distinguish a recreated LDAP entry with the same connector/uid +from the previous identity. Directory UUID pinning still protects group writes, +but membership no longer checks directory recreation during login. Preserve the +connector mapping for other services; migrating Dex to immutable user IDs is a +separate coordinated change, not an automatic part of this release. + +### LDAP service credentials still required for directory writes + +LDAP is still needed for registration and worker group synchronization. Configure +the least-privilege `LDAP_BIND_DN` and its **plaintext bind password** in +`LDAP_BIND_PASSWORD`, not a stored `{CRYPT}$1$...` hash or already-base64-encoded +value. Kubernetes Secret `data` encodes original values once. Check service +password rotation/account locks and redeploy web and worker after Secret updates; +existing pods do not refresh environment credentials automatically. Never use +root/admin credentials or disable TLS verification to bypass failures. + +Older images may log `LDAP service bind rejected during Dex callback` or +`Dex callback rejected kind=LDAPInvalidCredentialsResult`. These login-time +service binds are removed by this release. OpenLDAP remains responsible for +verifying users' existing password hashes through Dex. `LDAP_PASSWORD_SCHEME` +only gates new registration; its Argon2 requirement is unchanged. Plan migration +away from weak legacy password hashes separately. + ### SHKeeper prepaid crypto Stripe remains the automatically recurring payment method. SHKeeper accepts diff --git a/membership/members/billing.py b/membership/members/billing.py index 9c650dc..0f87848 100755 --- a/membership/members/billing.py +++ b/membership/members/billing.py @@ -105,7 +105,7 @@ def apply_groups(account): account.ldap_dn = reconcile_groups(account, desired, managed) if managed else account.ldap_dn account.last_ldap_sync = now account.membership_level = membership_level - account.save(update_fields=["ldap_dn", "last_ldap_sync", "membership_level"]) + account.save(update_fields=["ldap_dn", "ldap_uuid", "last_ldap_sync", "membership_level"]) return len(desired) diff --git a/membership/members/directory.py b/membership/members/directory.py index a100f73..c6e74d4 100755 --- a/membership/members/directory.py +++ b/membership/members/directory.py @@ -8,10 +8,16 @@ from urllib.parse import urlsplit from django.conf import settings from django.core.exceptions import ImproperlyConfigured from ldap3 import BASE, MODIFY_ADD, MODIFY_DELETE, Connection, Server, Tls -from ldap3.core.exceptions import LDAPException +from ldap3.core.exceptions import LDAPException, LDAPInvalidCredentialsResult from ldap3.utils.conv import escape_filter_chars from ldap3.utils.dn import escape_rdn +from .models import Account + + +class LDAPServiceBindError(LDAPException): + """The application's LDAP service credentials were rejected, not a user's.""" + @contextmanager def connection(): @@ -25,7 +31,12 @@ def connection(): # Plaintext requires explicit operator consent; LDAPS is never downgraded. if url.scheme == "ldap" and not settings.LDAP_ALLOW_INSECURE and not conn.start_tls(): raise LDAPException("StartTLS failed; refusing to bind.") - if not conn.bind(): + try: + bound = conn.bind() + except LDAPInvalidCredentialsResult: + # Do not expose server diagnostics or retry with a user's password. + raise LDAPServiceBindError("Check LDAP_BIND_DN and LDAP_BIND_PASSWORD: use the service account's plaintext password, not its stored hash.") from None + if not bound: raise LDAPException("LDAP bind failed.") yield conn finally: @@ -41,16 +52,6 @@ def _identity(entry): return {"uid": str(entry.uid.value), "email": str(entry.mail.value), "ldap_dn": entry.entry_dn, "ldap_uuid": str(entry.entryUUID.value)} -def identity_for_login(uid): - if not isinstance(uid, str) or not re.fullmatch(r"[a-z][a-z0-9._-]{2,47}", uid): - raise ValueError("Dex must return a valid LDAP uid as preferred_username.") - with connection() as conn: - entries = _find(conn, f"(&(objectClass=inetOrgPerson)(uid={escape_filter_chars(uid)}))") - if len(entries) != 1: - raise ValueError("LDAP identity is missing or ambiguous.") - return _identity(entries[0]) - - def create_account(uid, email, password): if settings.LDAP_PASSWORD_SCHEME != "{ARGON2}": raise ImproperlyConfigured("Verify slapd's Argon2 password policy before enabling registration.") @@ -82,9 +83,22 @@ def reconcile_groups(account, desired, managed): if not placeholder: raise ImproperlyConfigured("LDAP_GROUP_PLACEHOLDER_DN is required to revoke the final member.") with connection() as conn: - entries = _find(conn, f"(entryUUID={escape_filter_chars(account.ldap_uuid)})") + if account.ldap_uuid: + query = f"(entryUUID={escape_filter_chars(account.ldap_uuid)})" + else: + # Only the worker resolves first-time Dex users. Never fall back to + # username once an entryUUID has been pinned (including deletions). + if account.oidc_issuer != settings.DEX_ISSUER or not account.oidc_sub or not re.fullmatch(r"[a-z][a-z0-9._-]{2,47}", account.uid): + raise ValueError("A verified Dex mapping is required to resolve the directory identity.") + query = f"(&(objectClass=inetOrgPerson)(uid={escape_filter_chars(account.uid)}))" + entries = _find(conn, query) if len(entries) != 1: raise ValueError("Membership identity missing or ambiguous; refusing to modify groups.") + identity = _identity(entries[0]) + if not identity["ldap_uuid"] or identity["ldap_uuid"] == "None" or len(identity["ldap_uuid"]) > 64: + raise ValueError("A directory entryUUID is required to synchronize groups.") + if not account.ldap_uuid and Account.objects.filter(ldap_uuid=identity["ldap_uuid"]).exclude(pk=account.pk).exists(): + raise ValueError("Directory entry is already pinned to another local account; refusing to modify groups.") dn = entries[0].entry_dn if placeholder.casefold() == dn.casefold(): raise ImproperlyConfigured("The LDAP sentinel must not be a login account.") @@ -107,4 +121,6 @@ def reconcile_groups(account, desired, managed): changes.append((MODIFY_ADD, [dn])) if changes and not conn.modify(group_dn, {"uniqueMember": changes}): raise LDAPException("LDAP group synchronization failed.") + # Caller holds the account row lock and persists this pin with sync state. + account.ldap_uuid = identity["ldap_uuid"] return dn diff --git a/membership/members/management/commands/bind_dex_identity.py b/membership/members/management/commands/bind_dex_identity.py new file mode 100755 index 0000000..431fd19 --- /dev/null +++ b/membership/members/management/commands/bind_dex_identity.py @@ -0,0 +1,38 @@ +from django.conf import settings +from django.core.exceptions import ValidationError +from django.core.management.base import BaseCommand, CommandError +from django.db import IntegrityError, transaction + +from members.models import Account + + +class Command(BaseCommand): + help = "Explicitly bind an unlinked local account to an independently verified Dex subject." + + def add_arguments(self, parser): + parser.add_argument("--account", required=True, help="Local account UUID, not an email address.") + parser.add_argument("--uid", required=True, help="Independently verified LDAP connector uid.") + parser.add_argument("--subject", required=True, help="Exact verified Dex sub for the configured issuer/connector.") + + def handle(self, *args, **options): + subject = options["subject"] + if not subject or len(subject) > 512: + raise CommandError("A nonempty Dex subject of at most 512 characters is required.") + try: + with transaction.atomic(): + account = Account.objects.select_for_update().get(pk=options["account"]) + if account.uid != options["uid"]: + raise CommandError("The verified uid does not match the selected account.") + if account.oidc_sub: + if account.oidc_sub == subject and account.oidc_issuer == settings.DEX_ISSUER: + self.stdout.write("Dex identity already bound.") + return + raise CommandError("Account already has a Dex binding; automatic rebinding is forbidden.") + account.oidc_issuer = settings.DEX_ISSUER + account.oidc_sub = subject + account.save(update_fields=["oidc_issuer", "oidc_sub"]) + except (Account.DoesNotExist, ValidationError, ValueError) as exc: + raise CommandError("Select an existing account by UUID.") from exc + except IntegrityError: + raise CommandError("Dex subject is already bound to another account.") from None + self.stdout.write(self.style.SUCCESS("Dex identity bound. No LDAP or billing data changed.")) diff --git a/membership/members/migrations/0004_dex_login_identity.py b/membership/members/migrations/0004_dex_login_identity.py new file mode 100755 index 0000000..3d294cb --- /dev/null +++ b/membership/members/migrations/0004_dex_login_identity.py @@ -0,0 +1,29 @@ +from django.conf import settings +from django.db import migrations, models + + +def pin_existing_issuer(apps, schema_editor): + # Upgrade with the same issuer that authenticated the existing subjects. + Account = apps.get_model("members", "Account") + Account.objects.using(schema_editor.connection.alias).filter( + oidc_sub__isnull=False, + ).update(oidc_issuer=settings.DEX_ISSUER) + + +class Migration(migrations.Migration): + dependencies = [("members", "0003_membership_levels")] + operations = [ + migrations.AlterField( + model_name="account", name="ldap_dn", + field=models.CharField(blank=True, max_length=1024), + ), + migrations.AlterField( + model_name="account", name="ldap_uuid", + field=models.CharField(blank=True, max_length=64, null=True, unique=True), + ), + migrations.AddField( + model_name="account", name="oidc_issuer", + field=models.CharField(blank=True, default="", max_length=2048), + ), + migrations.RunPython(pin_existing_issuer, migrations.RunPython.noop), + ] diff --git a/membership/members/models.py b/membership/members/models.py index f985137..08eff0d 100755 --- a/membership/members/models.py +++ b/membership/members/models.py @@ -7,8 +7,9 @@ class Account(models.Model): id = models.UUIDField(primary_key=True, default=uuid.uuid4, editable=False) uid = models.CharField(max_length=48, unique=True) email = models.EmailField() - ldap_dn = models.CharField(max_length=1024) - ldap_uuid = models.CharField(max_length=64, unique=True) + ldap_dn = models.CharField(max_length=1024, blank=True) + ldap_uuid = models.CharField(max_length=64, unique=True, null=True, blank=True) + oidc_issuer = models.CharField(max_length=2048, blank=True, default="") oidc_sub = models.CharField(max_length=512, unique=True, null=True) membership_level = models.PositiveSmallIntegerField(default=0) stripe_customer = models.CharField(max_length=128, unique=True, null=True) diff --git a/membership/members/tests/test_membership.py b/membership/members/tests/test_membership.py index eef9c63..0495a03 100755 --- a/membership/members/tests/test_membership.py +++ b/membership/members/tests/test_membership.py @@ -5,11 +5,14 @@ import re import time from contextlib import contextmanager from datetime import timedelta +from io import StringIO from unittest.mock import MagicMock, patch from django.conf import settings from django.core import mail -from django.test import Client, SimpleTestCase, TestCase, override_settings +from django.core.management import call_command +from django.core.management.base import CommandError +from django.test import Client, TestCase, TransactionTestCase, override_settings from django.utils import timezone from ldap3 import Connection, MOCK_SYNC, OFFLINE_SLAPD_2_4, Server @@ -28,6 +31,8 @@ TIERS = { def account(**kwargs): + if kwargs.get("oidc_sub"): + kwargs.setdefault("oidc_issuer", settings.DEX_ISSUER) return Account.objects.create(uid="alice", email="alice@example.org", ldap_dn=DN, ldap_uuid="11111111-1111-1111-1111-111111111111", **kwargs) @@ -70,6 +75,7 @@ class RegistrationTests(TestCase): self.assertFalse(Subscription.objects.exists()) self.assertFalse(Registration.objects.exists()) self.assertNotIn("account_id", self.client.session) + self.assertEqual(self.client.session["registration_identity"]["account_id"], str(Account.objects.get().pk)) self.assertNotIn(password, str(list(Account.objects.values()))) self.client.post("/members/register/finish/", {"password": password, "confirm_password": password}) self.assertEqual(provision.call_count, 1) @@ -134,46 +140,118 @@ class IdentityTests(TestCase): def claims(self, **changes): return {"iss": settings.DEX_ISSUER, "sub": "dex-alice", "preferred_username": "alice", "email_verified": True, "email": "alice@example.org", "federated_claims": {"connector_id": settings.DEX_CONNECTOR_ID, "user_id": "alice"}, **changes} - @patch("members.views.directory.identity_for_login") - def test_existing_directory_account_links_without_password(self, identity): - identity.return_value = {"uid": "alice", "email": "alice@example.org", "ldap_dn": DN, "ldap_uuid": "uuid-alice"} + @patch("members.directory.connection", side_effect=AssertionError("Login must not contact LDAP")) + def test_first_and_returning_dex_login_need_no_ldap(self, connection): first = views.link_identity(self.claims()) - second = views.link_identity(self.claims(sub="ldap-alice")) + second = views.link_identity(self.claims(email="updated@example.org")) self.assertEqual(first.pk, second.pk) - first.refresh_from_db() - self.assertEqual(first.oidc_sub, "ldap-alice") + second.refresh_from_db() + self.assertEqual(second.oidc_sub, "dex-alice") + self.assertEqual(second.oidc_issuer, settings.DEX_ISSUER) + self.assertEqual(second.email, "updated@example.org") + self.assertIsNone(second.ldap_uuid) + self.assertEqual(second.ldap_dn, "") + self.assertEqual(second.membership_level, 0) + self.assertEqual(ManagedGroup.objects.filter(account=second).count(), len(TIERS)) self.assertFalse(Subscription.objects.exists()) - views.link_identity(self.claims(sub="next-verified-subject")) - first.refresh_from_db() - self.assertEqual(first.oidc_sub, "next-verified-subject") + connection.assert_not_called() - @patch("members.views.directory.identity_for_login") - def test_no_email_only_or_unverified_link(self, identity): - for changes in ({"email_verified": False}, {"iss": "https://evil.example"}, {"sub": ""}): - with self.assertRaises(ValueError): + def test_invalid_claims_rejected(self): + from django.core.exceptions import ValidationError + + for changes in ({"email_verified": False}, {"iss": "https://evil.example"}, {"sub": ""}, {"sub": "x" * 513}, {"preferred_username": "*)(uid=*)"}, {"email": None}, {"email": "invalid"}): + with self.subTest(changes=changes), self.assertRaises((ValueError, ValidationError)): views.link_identity(self.claims(**changes)) - identity.assert_not_called() - identity.return_value = {"uid": "alice", "email": "other@example.org", "ldap_dn": DN, "ldap_uuid": "uuid-alice"} - with self.assertRaises(ValueError): - views.link_identity(self.claims()) self.assertFalse(Account.objects.exists()) - @patch("members.views.directory.identity_for_login") - def test_other_connector_or_recreated_ldap_account_rejected(self, identity): - identity.return_value = {"uid": "alice", "email": "alice@example.org", "ldap_dn": DN, "ldap_uuid": "uuid-alice"} - for federated in ({"connector_id": "github", "user_id": "alice"}, {"connector_id": settings.DEX_CONNECTOR_ID, "user_id": "different-uid"}, {}): + def test_wrong_or_missing_signed_connector_identity_rejected(self): + for federated in ({"connector_id": "github", "user_id": "alice"}, {"connector_id": settings.DEX_CONNECTOR_ID, "user_id": "different-uid"}, {}, ["malformed"]): with self.assertRaises(ValueError): views.link_identity(self.claims(federated_claims=federated)) self.assertFalse(Account.objects.exists()) - @patch("members.views.directory.identity_for_login") - def test_same_uid_different_uuid_cannot_take_over_account(self, identity): + def test_existing_subject_and_directory_pin_preserved(self): + original = account(oidc_sub="dex-alice", stripe_customer="cus_existing") + linked = views.link_identity(self.claims()) + self.assertEqual(linked.pk, original.pk) + self.assertEqual(linked.ldap_uuid, original.ldap_uuid) + self.assertEqual(linked.ldap_dn, original.ldap_dn) + self.assertEqual(linked.stripe_customer, "cus_existing") + + def test_different_subject_cannot_take_over_existing_account(self): original = account(oidc_sub="original") - identity.return_value = {"uid": "alice", "email": "alice@example.org", "ldap_dn": DN, "ldap_uuid": "new-directory-entry"} + with self.assertRaises(ValueError): + views.link_identity(self.claims(), registration_account_id=str(original.pk)) + original.refresh_from_db() + self.assertEqual(original.oidc_sub, "original") + + def test_issuer_change_does_not_reuse_subject(self): + original = account(oidc_sub="dex-alice", oidc_issuer="https://previous.example/") with self.assertRaises(ValueError): views.link_identity(self.claims()) original.refresh_from_db() - self.assertEqual(original.oidc_sub, "original") + self.assertEqual(original.oidc_issuer, "https://previous.example/") + + def test_email_match_does_not_link_different_identity(self): + original = account(oidc_sub="original") + other = views.link_identity(self.claims(sub="dex-bob", preferred_username="bob", federated_claims={"connector_id": settings.DEX_CONNECTOR_ID, "user_id": "bob"})) + self.assertNotEqual(other.pk, original.pk) + self.assertIsNone(other.stripe_customer) + + def test_subject_cannot_be_reused_with_another_uid(self): + account(oidc_sub="dex-alice") + with self.assertRaises(ValueError): + views.link_identity(self.claims(preferred_username="bob", federated_claims={"connector_id": settings.DEX_CONNECTOR_ID, "user_id": "bob"})) + self.assertEqual(Account.objects.count(), 1) + + def test_unbound_account_requires_registration_proof(self): + original = account() + for proof in (None, "another-account"): + with self.assertRaises(ValueError): + views.link_identity(self.claims(), registration_account_id=proof) + linked = views.link_identity(self.claims(), registration_account_id=str(original.pk)) + self.assertEqual(linked.pk, original.pk) + self.assertEqual(linked.ldap_uuid, original.ldap_uuid) + self.assertEqual(linked.oidc_sub, "dex-alice") + + @patch("members.views.dex") + def test_callback_consumes_registration_proof_and_rejects_expired_proof(self, dex): + original = account() + dex.return_value.authorize_access_token.return_value = {"id_token": "private-token", "userinfo": self.claims()} + session = self.client.session + session["registration_identity"] = {"account_id": str(original.pk), "expires_at": time.time() - 10} + session.save() + self.assertContains(self.client.get("/members/oidc/callback/"), "could not be verified") + original.refresh_from_db() + self.assertIsNone(original.oidc_sub) + session = self.client.session + session["registration_identity"] = {"account_id": str(original.pk), "expires_at": time.time() + 300} + session.save() + self.assertRedirects(self.client.get("/members/oidc/callback/"), "/members/") + self.assertEqual(dict(self.client.session), {"account_id": str(original.pk)}) + + def test_operator_binding_is_explicit_idempotent_and_cannot_rebind(self): + original = account() + options = {"account": str(original.pk), "uid": "alice", "subject": "dex-alice", "stdout": StringIO()} + with self.assertRaises(CommandError): + call_command("bind_dex_identity", **{**options, "uid": "bob"}) + call_command("bind_dex_identity", **options) + call_command("bind_dex_identity", **options) + self.assertEqual(views.link_identity(self.claims()).pk, original.pk) + with self.assertRaises(CommandError): + call_command("bind_dex_identity", **{**options, "subject": "different-sub"}) + + def test_operator_cannot_bind_subject_owned_by_another_account(self): + original = account() + Account.objects.create(uid="bob", email="bob@example.org", oidc_sub="dex-alice", oidc_issuer=settings.DEX_ISSUER) + with self.assertRaises(CommandError): + call_command("bind_dex_identity", account=str(original.pk), uid="alice", subject="dex-alice", stdout=StringIO()) + original.refresh_from_db() + self.assertIsNone(original.oidc_sub) + + def test_operator_binding_rejects_invalid_account_ids(self): + with self.assertRaises(CommandError): + call_command("bind_dex_identity", account="not-a-uuid", uid="alice", subject="dex-alice", stdout=StringIO()) @patch("members.views.dex") @patch("members.views.link_identity") @@ -196,6 +274,18 @@ class IdentityTests(TestCase): self.assertContains(response, "could not be verified") self.assertNotIn("account_id", self.client.session) + @patch("members.views.dex") + @patch("members.directory.Connection") + @override_settings(LDAP_URL="", LDAP_BIND_DN="", LDAP_BIND_PASSWORD="", LDAP_PASSWORD_SCHEME="{CRYPT}") + def test_callback_works_without_ldap_configuration(self, conn_factory, dex): + from ldap3.core.exceptions import LDAPInvalidCredentialsResult + + dex.return_value.authorize_access_token.return_value = {"id_token": "private-token", "userinfo": self.claims()} + conn_factory.return_value.bind.side_effect = LDAPInvalidCredentialsResult(description="private LDAP diagnostic") + self.assertRedirects(self.client.get("/members/oidc/callback/?code=private-code"), "/members/") + self.assertEqual(dict(self.client.session), {"account_id": str(Account.objects.get().pk)}) + conn_factory.assert_not_called() + @override_settings(MEMBERSHIP_TIERS=TIERS, STRIPE_WEBHOOK_SECRET="whsec_test_only", STRIPE_PORTAL_CONFIGURATION="bpc_test") class BillingViewsTests(TestCase): @@ -438,7 +528,7 @@ class ReconciliationTests(TestCase): @override_settings(LDAP_SEARCH_BASE_DN=BASE, LDAP_GROUP_PLACEHOLDER_DN=SENTINEL) -class DirectoryTests(SimpleTestCase): +class DirectoryTests(TestCase): def setUp(self): # ldap3 supports offline schemas; its third-party annotation omits them. server = Server("mock", get_info=OFFLINE_SLAPD_2_4) # pyright: ignore[reportArgumentType] @@ -476,9 +566,54 @@ class DirectoryTests(SimpleTestCase): with self.assertRaises(ValueError): directory.reconcile_groups(self.member, {GROUP}, {GROUP}) - def test_login_uid_rejects_filter_injection(self): - with self.assertRaises(ValueError): - directory.identity_for_login("*)(uid=*)") + def test_worker_resolves_and_pins_first_time_dex_identity(self): + self.member.ldap_uuid = None + self.member.ldap_dn = "" + self.member.oidc_issuer = settings.DEX_ISSUER + self.member.oidc_sub = "dex-alice" + with patch("members.directory.connection", self.mock_connection), patch.object(self.conn, "search", wraps=self.conn.search) as search: + self.assertEqual(directory.reconcile_groups(self.member, {GROUP}, {GROUP}), DN) + self.assertEqual(search.call_args_list[0].kwargs["attributes"], ["uid", "mail", "entryUUID"]) + self.assertIn("(uid=alice)", search.call_args_list[0].args[1]) + self.assertEqual(self.member.ldap_uuid, "11111111-1111-1111-1111-111111111111") + # A recreated uid must never substitute for the now-pinned LDAP entry. + self.conn.delete(DN) + self.conn.strategy.add_entry(DN, {"objectClass": ["inetOrgPerson"], "uid": "alice", "cn": "alice", "sn": "alice", "mail": "alice@example.org", "entryUUID": "22222222-2222-2222-2222-222222222222"}) + with patch("members.directory.connection", self.mock_connection), patch.object(self.conn, "modify", wraps=self.conn.modify) as modify: + with self.assertRaises(ValueError): + directory.reconcile_groups(self.member, {GROUP}, {GROUP}) + modify.assert_not_called() + + def test_worker_unpinned_identity_requires_trusted_mapping(self): + self.member.ldap_uuid = None + for uid, issuer, subject in (("alice", "", None), ("alice", "https://other.example", "sub"), ("*)(uid=*)", settings.DEX_ISSUER, "sub")): + self.member.uid, self.member.oidc_issuer, self.member.oidc_sub = uid, issuer, subject + with patch("members.directory.connection", self.mock_connection): + with self.assertRaises(ValueError): + directory.reconcile_groups(self.member, {GROUP}, {GROUP}) + + def test_worker_refuses_directory_uuid_already_owned_by_another_account(self): + account() + self.member.ldap_uuid = None + self.member.oidc_issuer = settings.DEX_ISSUER + self.member.oidc_sub = "dex-alice" + with patch("members.directory.connection", self.mock_connection), patch.object(self.conn, "modify", wraps=self.conn.modify) as modify: + with self.assertRaises(ValueError): + directory.reconcile_groups(self.member, {GROUP}, {GROUP}) + modify.assert_not_called() + + @override_settings(LDAP_URL="ldaps://ldap.example.org", LDAP_BIND_DN="cn=service", LDAP_BIND_PASSWORD="test-only", LDAP_ACCOUNT_BASE_DN=BASE) + @patch("members.directory.Connection") + def test_service_bind_failure_is_redacted_and_unbound(self, conn_factory): + from ldap3.core.exceptions import LDAPInvalidCredentialsResult + + conn_factory.return_value.bind.side_effect = LDAPInvalidCredentialsResult(description="private diagnostic") + with self.assertRaises(directory.LDAPServiceBindError) as raised: + with directory.connection(): + self.fail("Rejected credentials must not reach LDAP operations") + self.assertIn("LDAP_BIND_PASSWORD", str(raised.exception)) + self.assertNotIn("private diagnostic", str(raised.exception)) + conn_factory.return_value.unbind.assert_called_once() @override_settings(LDAP_URL="ldap://ldap.example.org", LDAP_ALLOW_INSECURE=False, LDAP_BIND_DN="cn=service", LDAP_BIND_PASSWORD="test-only", LDAP_ACCOUNT_BASE_DN=BASE) @patch("members.directory.Connection") @@ -515,3 +650,58 @@ class DirectoryTests(SimpleTestCase): self.assertEqual(options["use_ssl"], scheme == "ldaps") self.assertEqual(options["port"], port) self.assertEqual(options["tls"].validate, ssl.CERT_REQUIRED) + + +@override_settings(LDAP_SEARCH_BASE_DN=BASE, LDAP_GROUP_PLACEHOLDER_DN=SENTINEL, MEMBERSHIP_TIERS=TIERS) +class DeferredDirectoryPersistenceTests(TestCase): + def test_worker_persists_uuid_and_dn_after_real_offline_ldap_sync(self): + member = views.link_identity({"iss": settings.DEX_ISSUER, "sub": "dex-alice", "preferred_username": "alice", "email_verified": True, "email": "alice@example.org", "federated_claims": {"connector_id": settings.DEX_CONNECTOR_ID, "user_id": "alice"}}) + server = Server("mock", get_info=OFFLINE_SLAPD_2_4) # pyright: ignore[reportArgumentType] + conn = Connection(server, client_strategy=MOCK_SYNC) + conn.bind() + conn.strategy.add_entry(DN, {"objectClass": ["inetOrgPerson"], "uid": "alice", "cn": "alice", "sn": "alice", "mail": "alice@example.org", "entryUUID": "11111111-1111-1111-1111-111111111111"}) + conn.strategy.add_entry(SENTINEL, {"objectClass": ["organizationalRole"], "cn": "sentinel"}) + for group in (GROUP, OTHER_GROUP): + conn.strategy.add_entry(group, {"objectClass": ["groupOfUniqueNames"], "cn": "paid", "uniqueMember": [SENTINEL]}) + + @contextmanager + def mock_connection(): + yield conn + + with patch("members.directory.connection", mock_connection): + self.assertTrue(billing.sync_account(member.pk, refresh_stripe=False)) + member.refresh_from_db() + self.assertEqual(member.ldap_uuid, "11111111-1111-1111-1111-111111111111") + self.assertEqual(member.ldap_dn, DN) + self.assertIsNotNone(member.last_ldap_sync) + self.assertEqual(member.membership_level, 0) + + +class DexIdentityMigrationTests(TransactionTestCase): + @override_settings(DEX_ISSUER="https://original-issuer.example/") + def test_upgrade_preserves_existing_bindings_and_allows_unresolved_users(self): + from django.db import connection + from django.db.migrations.executor import MigrationExecutor + + previous = [("members", "0003_membership_levels")] + current = [("members", "0004_dex_login_identity")] + executor = MigrationExecutor(connection) + executor.migrate(previous) + try: + OldAccount = executor.loader.project_state(previous).apps.get_model("members", "Account") + linked = OldAccount.objects.create(uid="alice", email="alice@example.org", ldap_dn=DN, ldap_uuid="uuid-alice", oidc_sub="existing-sub", stripe_customer="cus_existing") + unlinked = OldAccount.objects.create(uid="bob", email="bob@example.org", ldap_dn="uid=bob," + BASE, ldap_uuid="uuid-bob") + executor = MigrationExecutor(connection) + executor.migrate(current) + NewAccount = executor.loader.project_state(current).apps.get_model("members", "Account") + restored = NewAccount.objects.values("oidc_issuer", "oidc_sub", "ldap_uuid", "stripe_customer").get(pk=linked.pk) + self.assertEqual(restored["oidc_issuer"], "https://original-issuer.example/") + self.assertEqual(restored["oidc_sub"], "existing-sub") + self.assertEqual(restored["ldap_uuid"], "uuid-alice") + self.assertEqual(restored["stripe_customer"], "cus_existing") + self.assertEqual(NewAccount.objects.values_list("oidc_issuer", flat=True).get(pk=unlinked.pk), "") + for uid in ("charlie", "david"): + new = NewAccount.objects.create(uid=uid, email=f"{uid}@example.org", oidc_issuer=settings.DEX_ISSUER, oidc_sub=f"dex-{uid}") + self.assertIsNone(NewAccount.objects.values_list("ldap_uuid", flat=True).get(pk=new.pk)) + finally: + MigrationExecutor(connection).migrate(current) diff --git a/membership/members/tests/test_oidc.py b/membership/members/tests/test_oidc.py index c870ad6..4b6046b 100755 --- a/membership/members/tests/test_oidc.py +++ b/membership/members/tests/test_oidc.py @@ -48,6 +48,19 @@ class AuthlibFlowTests(TestCase): with self.assertRaises(Exception): self.exchange(remote, callback, token) + def test_verified_callback_creates_account_without_ldap(self): + from members import views + from members.models import Account + + remote, callback, token = self.flow(preferred_username="alice", email="alice@example.org", email_verified=True, federated_claims={"connector_id": settings.DEX_CONNECTOR_ID, "user_id": "alice"}) + with patch("members.views.dex", return_value=remote), patch.object(remote, "fetch_access_token", return_value=token), patch.object(remote, "fetch_jwk_set", return_value={"keys": [self.key.as_dict(private=False)]}), patch("members.directory.Connection", side_effect=AssertionError("Login must not bind to LDAP")) as ldap: + response = views.callback(callback) + self.assertEqual(response.status_code, 302) + member = Account.objects.get(oidc_issuer=settings.DEX_ISSUER, oidc_sub="test-sub") + self.assertIsNone(member.ldap_uuid) + self.assertEqual(dict(callback.session), {"account_id": str(member.pk)}) + ldap.assert_not_called() + def test_wrong_nonce_issuer_audience_expiry_rejected(self): for changes in ({"nonce": "attacker"}, {"iss": "https://evil.example"}, {"aud": "another-client"}, {"exp": int(time.time()) - 100}): with self.subTest(changes=changes): diff --git a/membership/members/views.py b/membership/members/views.py index 9b240e5..71def4a 100755 --- a/membership/members/views.py +++ b/membership/members/views.py @@ -1,5 +1,6 @@ import hashlib import logging +import re import secrets import uuid from datetime import timedelta @@ -9,6 +10,7 @@ from urllib.parse import urlsplit import stripe from authlib.integrations.django_client import OAuth from django.conf import settings +from django.core.validators import validate_email from django.core.mail import send_mail from django.db import transaction from django.http import HttpResponse @@ -129,6 +131,9 @@ def finish_registration(request): remember_groups(account) Registration.objects.filter(uid=registration.uid).delete() request.session.pop("registration_hash", None) + # Server-side, short-lived proof permits first binding of ONLY the + # account just provisioned in this browser, never an email match. + request.session["registration_identity"] = {"account_id": str(account.pk), "expires_at": (timezone.now() + timedelta(hours=1)).timestamp()} return page(request, title="Account created", message="Your free eom.dev account is ready. Sign in to explore the community and its services, or choose an optional paid membership for additional benefits.") except ValueError: form.add_error(None, "An account with those details may already exist. Try signing in or contact us for help.") @@ -163,32 +168,37 @@ def login(request): return page(request, title="Sign-in unavailable", message="Please try again later.") -def link_identity(claims): +def link_identity(claims, *, registration_account_id=None): if claims.get("iss") != settings.DEX_ISSUER or claims.get("email_verified") is not True or not isinstance(claims.get("sub"), str) or not claims["sub"] or len(claims["sub"]) > 512: raise ValueError("A verified Dex identity is required.") - identity = directory.identity_for_login(claims.get("preferred_username")) + uid, email = claims.get("preferred_username"), claims.get("email") + if not isinstance(uid, str) or not re.fullmatch(r"[a-z][a-z0-9._-]{2,47}", uid): + raise ValueError("Dex must return a valid uid as preferred_username.") + if not isinstance(email, str) or len(email) > 254: + raise ValueError("A verified Dex email is required.") + validate_email(email) federated = claims.get("federated_claims") or {} - if federated.get("connector_id") != settings.DEX_CONNECTOR_ID or federated.get("user_id") != identity["uid"]: + if not isinstance(federated, dict) or federated.get("connector_id") != settings.DEX_CONNECTOR_ID or federated.get("user_id") != uid: raise ValueError("Dex must authenticate the configured LDAP connector with uid as idAttr.") - if identity["email"].casefold() != str(claims.get("email", "")).casefold(): - raise ValueError("Dex and LDAP email disagree.") - # Resolve by immutable LDAP entryUUID, never by email. Dex's federated user - # ID remains uid so the unified connector preserves existing OIDC subjects. + # Authentication is entirely OIDC. Never contact LDAP or silently rebind an + # existing account to a different subject based on username/email matches. with transaction.atomic(): - uid_owner = Account.objects.filter(uid=identity["uid"]).first() - if uid_owner and uid_owner.ldap_uuid != identity["ldap_uuid"]: - raise ValueError("Username is already linked to another directory entry.") - account, _ = Account.objects.get_or_create(ldap_uuid=identity["ldap_uuid"], defaults=identity) - account = Account.objects.select_for_update().get(pk=account.pk) - subject_owner = Account.objects.filter(oidc_sub=claims["sub"]).exclude(pk=account.pk).exists() - if subject_owner: - raise ValueError("Dex subject is already linked to another directory entry.") - # A previous login may have used the retired www-ldap connector, whose - # subjects used entryUUID. Rebind after validating the same LDAP entry. + account = Account.objects.select_for_update().filter(oidc_issuer=claims["iss"], oidc_sub=claims["sub"]).first() + if account: + if account.uid != uid: + raise ValueError("Dex identity username changed; an explicit migration is required.") + else: + account, created = Account.objects.select_for_update().get_or_create(uid=uid, defaults={"email": email, "oidc_issuer": claims["iss"], "oidc_sub": claims["sub"]}) + if not created: + if account.oidc_sub: + if account.oidc_issuer != claims["iss"] or account.oidc_sub != claims["sub"]: + raise ValueError("Existing account requires an explicit Dex identity mapping.") + elif str(account.pk) != registration_account_id: + raise ValueError("Existing account requires an explicit Dex identity mapping.") + account.oidc_issuer = claims["iss"] account.oidc_sub = claims["sub"] - account.email = identity["email"] - account.ldap_dn = identity["ldap_dn"] - account.save(update_fields=["oidc_sub", "email", "ldap_dn"]) + account.email = email + account.save(update_fields=["oidc_issuer", "oidc_sub", "email"]) remember_groups(account) return account @@ -199,7 +209,10 @@ def callback(request): token = dex().authorize_access_token(request, claims_options={"iss": {"essential": True, "value": settings.DEX_ISSUER}}, leeway=30) if not token.get("id_token"): raise ValueError("ID token is required.") - account = link_identity(token.get("userinfo") or {}) + proof = request.session.get("registration_identity") or {} + expires_at = proof.get("expires_at") if isinstance(proof, dict) else None + registration_account_id = proof.get("account_id") if isinstance(expires_at, (int, float)) and expires_at > timezone.now().timestamp() else None + account = link_identity(token.get("userinfo") or {}, registration_account_id=registration_account_id) except Exception as exc: logger.error("Dex callback rejected kind=%s", type(exc).__name__) return page(request, title="Sign-in could not be verified", message="Try signing in again, or contact the administrator.")