Fix the migration to not use auth_id from admin_info

[kingMonkeh]

Bug fix, no corresponding ticket

Implementation description

• Don't query for auth_id from the admin_info table

Steps to test

1. Start from a blank database and try running migrations

What should reviewers focus on?

• How to deal with admin_info

Checklist

• My PR name is descriptive and in imperative tense
• My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
• I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR

[claude]

Pull Request Review: Fix the migration to not use auth_id from admin_info

Summary

This PR addresses a critical bug in the migration ba76119b3e4c_update_user_system.py where it was attempting to query auth_id from the admin_info table, which doesn't exist as a column. The fix correctly removes this query and generates a placeholder auth_id instead.

Code Quality ✅

Strengths:

• The fix is minimal and surgical - only changes what's necessary
• The comment on line 84 is honest about the limitation: # THIS DOESN'T WORK LOL, THIS DOES NOT CORRESPOND TO THE FIREBASE AUTH_ID
• Follows the same pattern used for drivers (lines 122-139) which also generates placeholder auth_ids

Observations:

• The code is clear and easy to understand
• The change correctly prevents the migration from failing when run on a blank database

Potential Bugs or Issues ⚠️

Critical Issue: Generated auth_id Won't Work for Authentication

The migration now generates placeholder auth_ids like admin_{user_id}, but this creates a fundamental authentication problem:

1. Firebase Operations Will Fail: Throughout the codebase, auth_id is expected to be a valid Firebase UID:

   • firebase_admin.auth.revoke_refresh_tokens(auth_id) (auth_service.py:149)
   • firebase_admin.auth.delete_user(auth_id) (auth_routes.py:169)
   • firebase_admin.auth.get_user(uid=auth_id) (user_service.py:99)

2. Authentication Won't Work: When admins try to log in:

   • Firebase ID token contains the real Firebase UID
   • Database lookup in get_user_id_by_auth_id() will search for the real UID
   • Database has admin_{uuid} → authentication fails

3. Data Integrity: The users.auth_id field has a unique constraint and is indexed, so migrated admins with fake auth_ids will occupy those slots but won't be usable.

Question for Reviewers

How are you planning to handle admin authentication after this migration?

Options to consider:

1. Manual post-migration step: Require admins to re-register with Firebase and update their auth_ids
2. Seed script approach: Use environment variables for known admin Firebase UIDs (like the seed database does with ADMIN_AUTH_ID)
3. Delete migrated admin records: If the admin_info table in production doesn't have valid auth_ids, perhaps the migrated records should be deleted and recreated properly
4. Two-phase migration: First gather Firebase UIDs for existing admins, then run the migration

Performance Considerations ✅

• The change actually improves performance by removing an unnecessary column from the SELECT query
• Migration performance is adequate for typical database sizes
• Uses batch operations appropriately

Security Concerns 🔴

Critical Security Issue:

The current approach creates orphaned admin accounts that:

• Exist in the database but cannot authenticate via Firebase
• May create confusion about which accounts are valid
• Could lead to privilege escalation attempts if placeholder auth_ids are predictable

Recommendations:

1. Document which admin records are placeholders vs real
2. Add validation to prevent authentication with placeholder auth_ids
3. Consider adding a flag like requires_firebase_setup: bool to track admins that need proper Firebase credentials

Test Coverage ⚠️

Missing:

• No test coverage for migration scripts
• No verification that the migration succeeds on a blank database
• No test for what happens when auth_id doesn't exist in admin_info

Recommendation:
Consider adding migration tests that:

def test_migration_with_blank_database():
    # Verify migration runs successfully on empty DB
    pass

def test_migration_creates_placeholder_auth_ids():
    # Verify admins get placeholder auth_ids when original auth_id unavailable
    pass

Additional Notes

PR Checklist Items

From your checklist:

• ✅ PR name is descriptive and imperative
• ✅ Commit message is descriptive
• ⚠️ "How to deal with admin_info" - This is the key question that needs resolution

The Bigger Picture

This fix prevents the immediate migration failure, but it's a tactical fix for a strategic problem. The root issue is that the original admin_info table didn't store Firebase auth_id, and now that you're migrating to a unified users table that requires it, you need a strategy for getting real Firebase UIDs for existing admins.

Recommendation: Approve with Conditions

I recommend approving this PR as it fixes the immediate bug preventing migrations from running, BUT it should be accompanied by:

1. A follow-up ticket to properly handle admin Firebase authentication
2. Documentation explaining that migrated admins need to complete Firebase setup
3. A plan for how production admins will transition to the new system

The comment on line 84 acknowledges the issue, which is good for code clarity, but the team needs a concrete plan to resolve it.

---

Overall Assessment: This is a valid bug fix that prevents migration failures, but it reveals a deeper architectural issue with admin authentication that requires a follow-up solution. The code change itself is clean and safe.

[ludavidca]

LGTM, thanks Hy Lac 🐐 !