From 93307829992a76202eb8f69be0117143ba898925 Mon Sep 17 00:00:00 2001 From: "russell@unturf.com" Date: Thu, 29 Jan 2026 08:33:35 -0500 Subject: [PATCH] Fix case-sensitive email duplicates and add merge script - Normalize emails to lowercase on user creation - Use case-insensitive lookup in get_user_by_email - Add merge_duplicate_email_users script to merge existing duplicates - Add unit and integration tests to prevent regression - Update CLAUDE.md with tmux-hosts production troubleshooting --- CLAUDE.md | 10 + remarkbox/models/user.py | 4 +- .../scripts/merge_duplicate_email_users.py | 231 ++++++++++++++++++ remarkbox/tests/test_models.py | 12 + remarkbox/tests/test_views.py | 102 ++++++++ setup.py | 1 + 6 files changed, 358 insertions(+), 2 deletions(-) create mode 100644 remarkbox/scripts/merge_duplicate_email_users.py diff --git a/CLAUDE.md b/CLAUDE.md index 36dea23..aae2838 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -34,3 +34,13 @@ When adding new columns or modifying the database schema: 3. **Generate migration**: `alembic -c development.ini revision --autogenerate -m "description"` 4. **Clean up migration**: Remove extra autogenerated changes, keep only the new field 5. **Run migration**: `alembic -c development.ini upgrade head` + +## Production Troubleshooting + +For read-only troubleshooting on production, use tmux-hosts: + +``` +tmux-hosts +``` + +**Important**: This is for read-only investigation only. Do not make changes to production systems. diff --git a/remarkbox/models/user.py b/remarkbox/models/user.py index a1da0f2..a08c7da 100644 --- a/remarkbox/models/user.py +++ b/remarkbox/models/user.py @@ -279,7 +279,7 @@ class User(RBase, Base): self.name = unicode(generate_password(size=8)) self.created = now_timestamp() self.id = uuid.uuid1() - self.email = unicode(email) + self.email = unicode(email.lower()) # TODO: this field was never used & should be sunset. self.email_id = unicode(generate_password(size=8)) @@ -417,7 +417,7 @@ def get_user_by_id(dbsession, user_id): def get_user_by_email(dbsession, email): """Try to get User object by email or return None""" if email: - return dbsession.query(User).filter(User.email == unicode(email)).one_or_none() + return dbsession.query(User).filter(func.lower(User.email) == unicode(email.lower())).one_or_none() def get_or_create_user_by_email(dbsession, email): diff --git a/remarkbox/scripts/merge_duplicate_email_users.py b/remarkbox/scripts/merge_duplicate_email_users.py new file mode 100644 index 0000000..e04ba0e --- /dev/null +++ b/remarkbox/scripts/merge_duplicate_email_users.py @@ -0,0 +1,231 @@ +""" +Merge duplicate user accounts that have the same email (case-insensitive). + +This script finds users with emails that differ only by case (e.g., +'user@example.com' and 'User@Example.com') and merges them into a single +account, keeping the oldest one. + +Usage: + remarkbox_merge_duplicate_email_users -c production.ini --dry-run + remarkbox_merge_duplicate_email_users -c production.ini +""" +from collections import defaultdict + +from pyramid.paster import bootstrap, setup_logging + +from sqlalchemy import func + +from ..models import User + +from . import base_parser + + +def get_arg_parser(): + parser = base_parser("Find and merge user accounts with duplicate emails (case-insensitive).") + parser.add_argument( + "--dry-run", + action="store_true", + default=False, + help="Show what would be merged without making changes.", + ) + return parser + + +def find_duplicate_email_users(dbsession): + """ + Find all users with duplicate emails (case-insensitive). + + Returns a dict mapping lowercase email -> list of User objects. + Only includes emails with more than one user. + """ + duplicates = defaultdict(list) + + # Get all users + users = dbsession.query(User).all() + + for user in users: + email_lower = user.email.lower() + duplicates[email_lower].append(user) + + # Filter to only duplicates + return { + email: users + for email, users in duplicates.items() + if len(users) > 1 + } + + +def merge_users(dbsession, keep_user, delete_user, dry_run=False): + """ + Merge delete_user into keep_user by transferring all related records. + + Transfers: + - nodes (comments) + - votes + - watchers + - node_event_notifications + - namespace_user associations + - namespace_requests + - oauth records + - payments + - pay_what_you_can + """ + keep_id = keep_user.id + delete_id = delete_user.id + + print(f" Merging '{delete_user.name}' ({delete_user.email}) into '{keep_user.name}' ({keep_user.email})") + + if dry_run: + # Count records that would be transferred + from ..models import ( + Node, Vote, Watcher, NodeEventNotification, + NamespaceUser, NamespaceRequest, Oauth, Payment, PayWhatYouCan + ) + + counts = { + 'nodes': dbsession.query(Node).filter(Node.user_id == delete_id).count(), + 'votes': dbsession.query(Vote).filter(Vote.user_id == delete_id).count(), + 'watchers': dbsession.query(Watcher).filter(Watcher.user_id == delete_id).count(), + 'notifications': dbsession.query(NodeEventNotification).filter(NodeEventNotification.user_id == delete_id).count(), + 'namespace_users': dbsession.query(NamespaceUser).filter(NamespaceUser.user_id == delete_id).count(), + 'namespace_requests': dbsession.query(NamespaceRequest).filter(NamespaceRequest.user_id == delete_id).count(), + 'oauth': dbsession.query(Oauth).filter(Oauth.user_id == delete_id).count(), + 'payments': dbsession.query(Payment).filter(Payment.user_id == delete_id).count(), + 'pay_what_you_can': dbsession.query(PayWhatYouCan).filter(PayWhatYouCan.user_id == delete_id).count(), + } + + for table, count in counts.items(): + if count > 0: + print(f" Would transfer {count} {table}") + + return + + # Transfer all related records + from ..models import ( + Node, Vote, Watcher, NodeEventNotification, + NamespaceUser, NamespaceRequest, Oauth, Payment, PayWhatYouCan + ) + + # Update nodes + updated = dbsession.query(Node).filter(Node.user_id == delete_id).update( + {Node.user_id: keep_id}, synchronize_session=False + ) + if updated: + print(f" Transferred {updated} nodes") + + # Update votes + updated = dbsession.query(Vote).filter(Vote.user_id == delete_id).update( + {Vote.user_id: keep_id}, synchronize_session=False + ) + if updated: + print(f" Transferred {updated} votes") + + # Update watchers + updated = dbsession.query(Watcher).filter(Watcher.user_id == delete_id).update( + {Watcher.user_id: keep_id}, synchronize_session=False + ) + if updated: + print(f" Transferred {updated} watchers") + + # Update notifications + updated = dbsession.query(NodeEventNotification).filter(NodeEventNotification.user_id == delete_id).update( + {NodeEventNotification.user_id: keep_id}, synchronize_session=False + ) + if updated: + print(f" Transferred {updated} notifications") + + # Update namespace_user associations + updated = dbsession.query(NamespaceUser).filter(NamespaceUser.user_id == delete_id).update( + {NamespaceUser.user_id: keep_id}, synchronize_session=False + ) + if updated: + print(f" Transferred {updated} namespace associations") + + # Update namespace requests + updated = dbsession.query(NamespaceRequest).filter(NamespaceRequest.user_id == delete_id).update( + {NamespaceRequest.user_id: keep_id}, synchronize_session=False + ) + if updated: + print(f" Transferred {updated} namespace requests") + + # Update oauth records + updated = dbsession.query(Oauth).filter(Oauth.user_id == delete_id).update( + {Oauth.user_id: keep_id}, synchronize_session=False + ) + if updated: + print(f" Transferred {updated} oauth records") + + # Update payments + updated = dbsession.query(Payment).filter(Payment.user_id == delete_id).update( + {Payment.user_id: keep_id}, synchronize_session=False + ) + if updated: + print(f" Transferred {updated} payments") + + # Update pay_what_you_can (1-to-1, may need special handling) + existing_pwuc = dbsession.query(PayWhatYouCan).filter(PayWhatYouCan.user_id == keep_id).first() + delete_pwuc = dbsession.query(PayWhatYouCan).filter(PayWhatYouCan.user_id == delete_id).first() + if delete_pwuc: + if existing_pwuc: + # Keep user already has one, delete the duplicate's + dbsession.delete(delete_pwuc) + print(f" Deleted duplicate pay_what_you_can record") + else: + delete_pwuc.user_id = keep_id + print(f" Transferred pay_what_you_can record") + + dbsession.flush() + + # Delete the duplicate user + dbsession.delete(delete_user) + dbsession.flush() + print(f" Deleted user '{delete_user.name}'") + + +def main(): + parser = get_arg_parser() + args = parser.parse_args() + setup_logging(args.config) + + with bootstrap(args.config) as env, env["request"].tm as tm: + request = env["request"] + dbsession = request.dbsession + + print("Searching for duplicate email users (case-insensitive)...") + duplicates = find_duplicate_email_users(dbsession) + + if not duplicates: + print("No duplicate email users found.") + return + + print(f"Found {len(duplicates)} email(s) with duplicate users:\n") + + for email, users in duplicates.items(): + # Sort by created timestamp (oldest first) + users_sorted = sorted(users, key=lambda u: u.created) + keep_user = users_sorted[0] + delete_users = users_sorted[1:] + + print(f"Email: {email}") + print(f" Keeping: '{keep_user.name}' (id={keep_user.id}, created={keep_user.created})") + + for delete_user in delete_users: + print(f" Deleting: '{delete_user.name}' (id={delete_user.id}, created={delete_user.created})") + merge_users(dbsession, keep_user, delete_user, dry_run=args.dry_run) + + # Normalize the kept user's email to lowercase + if keep_user.email != email: + if args.dry_run: + print(f" Would normalize email from '{keep_user.email}' to '{email}'") + else: + keep_user.email = email + dbsession.add(keep_user) + dbsession.flush() + print(f" Normalized email to '{email}'") + + print() + + if args.dry_run: + print("DRY RUN - No changes were made. Run without --dry-run to apply changes.") + else: + print("Done. All duplicate users have been merged.") diff --git a/remarkbox/tests/test_models.py b/remarkbox/tests/test_models.py index 3be883e..d44bd40 100644 --- a/remarkbox/tests/test_models.py +++ b/remarkbox/tests/test_models.py @@ -36,6 +36,18 @@ class TestUser(unittest.TestCase): def test_email_set(self): self.assertEqual(self.user.email, "russell@ballestrini.net") + @mock.patch("remarkbox.models.user.is_user_name_available", mock_always_true) + def test_email_normalized_to_lowercase(self): + """Emails should be stored lowercase to prevent case-sensitive duplicates.""" + user = User("Russell@Ballestrini.NET") + self.assertEqual(user.email, "russell@ballestrini.net") + + @mock.patch("remarkbox.models.user.is_user_name_available", mock_always_true) + def test_email_mixed_case_normalized(self): + """Mixed case emails should be normalized to lowercase.""" + user = User("Grop3r@Protonmail.com") + self.assertEqual(user.email, "grop3r@protonmail.com") + def test_new_password(self): raw_password = self.user.new_password() self.assertEqual(len(raw_password), 6) diff --git a/remarkbox/tests/test_views.py b/remarkbox/tests/test_views.py index a34f4ce..73563b5 100644 --- a/remarkbox/tests/test_views.py +++ b/remarkbox/tests/test_views.py @@ -746,3 +746,105 @@ class AnonymousCommentingFunctionalTests(FunctionalTests): self.dbsession.expire(ns) self.assertTrue(ns.allow_anonymous) + + +class EmailCaseInsensitivityTests(FunctionalTests): + """ + Tests to prevent duplicate accounts from case-insensitive email addresses. + + Regression test for issue where users 'G' and 'Groupr' both had emails + 'grop3r@protonmail.com' and 'Grop3r@protonmail.com' causing duplicate notifications. + """ + + @classmethod + def setUpClass(cls): + try: + FunctionalTests.setUpClass.im_func(cls) + except AttributeError: + FunctionalTests.setUpClass.__func__(cls) + + def tearDown(self): + super(EmailCaseInsensitivityTests, self).tearDown() + # Clean up any test users created + for email in ["casetest@example.com", "CASETEST@EXAMPLE.COM", "CaseTest@Example.com"]: + user = get_user_by_email(self.dbsession, email) + if user: + self.dbsession.delete(user) + self.dbsession.flush() + self.tm.commit() + + def test_get_user_by_email_case_insensitive(self): + """get_user_by_email should find users regardless of email case.""" + # Create user with lowercase email + user = get_or_create_user_by_email(self.dbsession, "casetest@example.com") + self.dbsession.add(user) + self.dbsession.flush() + self.tm.commit() + + # Should find with different cases + found_lower = get_user_by_email(self.dbsession, "casetest@example.com") + found_upper = get_user_by_email(self.dbsession, "CASETEST@EXAMPLE.COM") + found_mixed = get_user_by_email(self.dbsession, "CaseTest@Example.com") + + self.assertIsNotNone(found_lower) + self.assertIsNotNone(found_upper) + self.assertIsNotNone(found_mixed) + self.assertEqual(found_lower.id, found_upper.id) + self.assertEqual(found_lower.id, found_mixed.id) + + def test_get_or_create_returns_existing_regardless_of_case(self): + """get_or_create_user_by_email should return existing user regardless of email case.""" + # Create user with lowercase email + user1 = get_or_create_user_by_email(self.dbsession, "casetest@example.com") + self.dbsession.add(user1) + self.dbsession.flush() + user1_id = user1.id + self.tm.commit() + + # Try to get/create with uppercase - should return same user + user2 = get_or_create_user_by_email(self.dbsession, "CASETEST@EXAMPLE.COM") + self.assertEqual(user1_id, user2.id) + + # Try to get/create with mixed case - should return same user + user3 = get_or_create_user_by_email(self.dbsession, "CaseTest@Example.com") + self.assertEqual(user1_id, user3.id) + + def test_new_user_email_stored_lowercase(self): + """New user emails should be normalized to lowercase.""" + user = get_or_create_user_by_email(self.dbsession, "CASETEST@EXAMPLE.COM") + self.dbsession.add(user) + self.dbsession.flush() + user_id = user.id + self.tm.commit() + + # Requery to avoid detached instance error + user = get_user_by_email(self.dbsession, "casetest@example.com") + # Email should be stored lowercase + self.assertEqual(user.email, "casetest@example.com") + + def test_no_duplicate_accounts_from_case_variations(self): + """Ensure case variations don't create duplicate accounts.""" + # Create first user + user1 = get_or_create_user_by_email(self.dbsession, "casetest@example.com") + self.dbsession.add(user1) + self.dbsession.flush() + user1_id = user1.id + self.tm.commit() + + # Attempt to create with different cases - should all return same user + user2 = get_or_create_user_by_email(self.dbsession, "CASETEST@EXAMPLE.COM") + user3 = get_or_create_user_by_email(self.dbsession, "CaseTest@Example.com") + user4 = get_or_create_user_by_email(self.dbsession, "casetest@EXAMPLE.COM") + + # All should be the same user + self.assertEqual(user1_id, user2.id) + self.assertEqual(user1_id, user3.id) + self.assertEqual(user1_id, user4.id) + + # Verify only one user exists with this email (case-insensitive) + from remarkbox.models.user import User + from sqlalchemy import func + count = self.dbsession.query(User).filter( + func.lower(User.email) == "casetest@example.com" + ).count() + self.assertEqual(count, 1) diff --git a/setup.py b/setup.py index f65ea14..7ef8786 100644 --- a/setup.py +++ b/setup.py @@ -67,6 +67,7 @@ setup( "remarkbox_json_import = remarkbox.scripts.json_import:main", "remarkbox_json_import2 = remarkbox.scripts.json_import2:main", "remarkbox_merge_dupes = remarkbox.scripts.merge_dupes:main", + "remarkbox_merge_duplicate_email_users = remarkbox.scripts.merge_duplicate_email_users:main", "remarkbox_invalidate_node_cache = remarkbox.scripts.invalidate_node_cache:main", "remarkbox_recompute_node_depths = remarkbox.scripts.recompute_node_depths:main", "remarkbox_safe_approve_all_nodes = remarkbox.scripts.safe_approve_all_nodes:main",