Finish Up Restructuring User System

[kingMonkeh]

JIRA ticket link

Finish Up Restructuring User System

Implementation description

• Wrote small python script to go through all firebase users and set to 'driver' role, except for admin user [email protected]
• deleted oauth routes since we decided we are not using them
• reenabled require_authorization_by_role
• added cascade delete to driver, admin

Steps to test

1. I already ran the python script so you don't need to run it again unless you made a new account without firebase roles yet
2. update your local db user role to be 'driver' and sign in with your account, alternative is to use the seeded admin user from seed_database.py
3. with your auth token try hitting the example admin only route GET http://localhost:8080/admins/test, should return a string if on the admin account, otherwise should return forbidden

What should reviewers focus on?

• checking that requiring role is working properly
• using delete user as the main source of truth, ie should I get rid of delete_driver service then ? since you should always be deleting the user to cascade

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]

Code Review

Overall this is a good cleanup PR — re-enabling role-based authorization and adding cascade deletes are both solid improvements. I found a few issues that should be addressed before merging.

---

Critical Bug: Role Case Mismatch

There is a case inconsistency across the codebase that will break driver authorization silently.

• update_firebase.py sets the Firebase custom claim to "driver" (lowercase)
• auth.py:131 defines require_driver = require_authorization_by_role({"Driver"}) (capitalized)

When a driver tries to access a protected route, user_role in _roles will evaluate "driver" in {"Driver"} → False, denying all drivers. The same issue exists if you ever use require_authorization_by_role({"driver"}) vs {"Driver"} elsewhere.

Suggestion: Pick one convention (e.g. all lowercase) and apply it consistently. The admin route already uses {"admin"} (lowercase), so lowercase for "driver" would be consistent.

# auth.py — should match what Firebase claims actually store
require_driver = require_authorization_by_role({"driver"})

---

update_firebase.py Should Not Live in the App Package

This is a one-time migration script but it's been added as app/update_firebase.py, making it part of the importable application package. It also hard-codes a production email address. Consider:

1. Moving it to a scripts/ directory at the repo root or backend/python/scripts/
2. Reading the admin email from an environment variable or config instead of hard-coding it

# Instead of:
admin_email = "[email protected]"

# Consider:
admin_email = os.environ.get("ADMIN_EMAIL", "[email protected]")

---

Misleading Underscore Prefix on Used Parameters

In auth_service.py, is_authorized_by_role uses _roles and _session prefixes (conventionally meaning "unused"), but _roles is actively used on line 169:

async def is_authorized_by_role(
    self, _session: AsyncSession, access_token: str, _roles: set[str]
) -> bool:
    ...
    return user_role in _roles  # _roles IS used here

_session is genuinely unused, so that prefix is fine. But _roles should be renamed to roles to avoid confusion.

---

Minor: auth.py Lost Exception Handling for check_role

The old code had a try/except wrapping check_role. It was incorrectly catching and re-raising as 401 for all exceptions including internal errors. The new code is correct in removing that — is_authorized_by_role handles its own exceptions internally and returns False. This is actually the right behavior, just noting it's intentional and good.

---

Question from PR Description

should I get rid of delete_driver service since you should always be deleting the user to cascade?

Yes, if cascade delete from user → driver/admin is the intended deletion path, having a separate delete_driver service method creates a footgun (you could orphan the User record). I'd keep the service method but have it call delete_user internally, or remove it and document that users should always be deleted at the user level.

---

Migration Downgrade Has Unnamed Constraint

# In downgrade():
op.drop_constraint(None, 'drivers', type_='foreignkey')  # None!

If this migration is ever rolled back, dropping None will fail. The downgrade should reference the actual constraint name. Consider running alembic to regenerate with a named constraint, or manually fill in the constraint name.

[claude]

PR Review: Finish Up Restructuring User System

Overall this is a solid cleanup PR — removing unused OAuth code, re-enabling role-based authorization, and adding cascade deletes are all good improvements. A few things to address:

---

Bugs / Issues

1. HTTP 401 used for authorization failures (should be 403)

In auth.py, when is_authorized_by_role returns False (the user IS authenticated but lacks permission), the code raises HTTP_401_UNAUTHORIZED. Semantically, 401 means "not authenticated" and 403 means "authenticated but not authorized." This should use HTTP_403_FORBIDDEN:

raise HTTPException(
    status_code=status.HTTP_403_FORBIDDEN,
    detail="You are not authorized to make this request.",
)

This applies to both the re-enabled check_role function and the existing check_user_id / check_email functions (pre-existing issue, but worth noting).

2. update_firebase.py hardcodes the admin email

The admin email [email protected] is hardcoded directly in the script. If this script is ever re-run after an admin change, it could misclassify users. Consider reading the admin email from an environment variable or config:

from app.config import settings
admin_email = settings.admin_email  # or similar

3. Migration downgrade uses None as constraint name

In the downgrade path of 01f342ea9ad6_add_cascade_delete_to_driver_and_admin.py, the drop_constraint calls pass None as the constraint name:

op.drop_constraint(None, 'drivers', type_='foreignkey')

This will fail at runtime if the downgrade is ever needed. The generated constraint names should be explicitly named in both create_foreign_key calls in upgrade() so they can be properly referenced in downgrade().

---

Design Question (from PR description)

should I get rid of delete_driver service then since you should always be deleting the user to cascade

Yes, if the design intent is that a driver/admin record cannot exist without a user, then deleting through the user (and relying on cascade) is the correct approach. You can keep delete_driver if there's a use case to remove the driver role without deleting the account, but otherwise it adds confusion. Worth making the decision explicit in a comment or updated docstring.

---

Minor / Style

• The update_firebase.py script is a one-off migration utility. Consider placing it in a scripts/ directory (or a migrations/scripts/ folder) rather than inside the app/ package, since it's not part of the application runtime.
• The test response string in admin_routes.py ("Admin only hehehehehehe - hy lac") should be replaced with something neutral before merge (e.g., "Admin access confirmed.").
• The _session parameter in is_authorized_by_role is prefixed with _ to indicate it's unused — that's fine, but the comment that used to accompany it (about a potential DB role check) has been removed. Consider whether that's a decision worth documenting briefly.

---

What's working well

• Removing the dead OAuth flow is a clean simplification.
• Cascade deletes are correctly implemented at both the model and migration levels.
• Role string normalization ("Driver" → "driver") is the right fix.
• is_authorized_by_role correctly reads the custom claim from the Firebase token.

[kingMonkeh]

@claude review

[claude]

Pull Request Review: Finish Up Restructuring User System

Summary

This PR successfully completes the user system restructuring by re-enabling role-based authorization, removing unused OAuth routes, and adding cascade delete functionality to driver and admin relationships. The implementation is solid with a few areas that could be improved.

---

Positive Changes ✅

1. Cascade Delete Implementation: Adding ondelete="CASCADE" to the foreign keys in Admin and Driver models (backend/python/app/models/admin.py:32, backend/python/app/models/driver.py:32) is correct and will prevent orphaned records when users are deleted.

2. Re-enabled Authorization: Properly restored role-based authorization in backend/python/app/dependencies/auth.py:60-68, which was previously disabled.

3. Correct HTTP Status Code: Changed from HTTP_401_UNAUTHORIZED to HTTP_403_FORBIDDEN (backend/python/app/dependencies/auth.py:65) for failed authorization checks. This is the correct semantic distinction - 401 is for authentication failures, 403 is for authorization failures.

4. Code Cleanup: Removed unused OAuth functionality that was decided against, keeping the codebase lean.

---

Issues & Concerns 🔴

1. Security: Hardcoded Admin Email

Location: backend/python/app/update_firebase.py:5

admin_email = "[email protected]"

Issue: The admin email is hardcoded in the migration script. This creates several problems:

• If the admin email changes, the script won't work correctly
• The script assumes this user exists, but doesn't handle the case where they don't
• This could be a security concern if the email is compromised

Recommendation: Either:

• Load this from environment variables/config
• Add validation to ensure this user exists before running
• Add comments explaining this is a one-time migration script

2. Inconsistent Error Handling in auth.py

Location: backend/python/app/dependencies/auth.py:86-99

The require_authorization_by_user_id function still uses HTTP_401_UNAUTHORIZED for authorization failures, while require_authorization_by_role correctly uses HTTP_403_FORBIDDEN. This inconsistency should be fixed.

Recommendation:

# Line 90 and 97 should use HTTP_403_FORBIDDEN instead of HTTP_401_UNAUTHORIZED
raise HTTPException(
    status_code=status.HTTP_403_FORBIDDEN,  # Changed from 401
    detail="You are not authorized to make this request.",
)

3. Role Case Sensitivity Mismatch

Location: backend/python/app/dependencies/auth.py:131

require_driver = require_authorization_by_role({"driver"})

Issue: The role is now lowercase "driver", but I notice in tests and migrations there are references to "Admin" with capital A. Need to verify all role strings are consistently lowercase throughout the codebase.

Recommendation: Search for all role string comparisons and ensure consistent casing. The Firebase custom claims use lowercase "admin" and "driver" (from update_firebase.py), so all checks should be lowercase.

4. Missing Error Handling in Migration Script

Location: backend/python/app/update_firebase.py:21-32

Issue: The script catches exceptions per-user but doesn't:

• Track which users failed
• Return a non-zero exit code if any users failed
• Provide a summary of failures

Recommendation: Add proper error tracking and exit codes for CI/CD integration.

---

Code Quality Issues ⚠️

1. Simplified User Service Parameters

Location: backend/python/app/services/implementations/user_service.py:82-86

Observation: The removal of auth_id and signup_method parameters is good cleanup since OAuth is removed. However, this change needs to be verified against all callers to ensure nothing is passing these removed parameters.

Recommendation: Search the codebase for all create_user calls to verify compatibility.

2. Dead Code in Admin Route

Location: backend/python/app/routers/admin_routes.py:22

return "Admin only hehehehehehe - hy lac"

Issue: This test route contains a playful message that should be cleaned up before production.

Recommendation: Either remove this test route or replace with a proper response message.

3. Migration Lacks Down Migration Constraint Names

Location: backend/python/migrations/versions/01f342ea9ad6_add_cascade_delete_to_driver_and_admin.py:27-31

Issue: The downgrade function uses None for constraint names which relies on SQLAlchemy to infer the names. This could fail on some database systems.

Recommendation: Use explicit constraint names:

def downgrade():
    op.drop_constraint('drivers_user_id_fkey', 'drivers', type_='foreignkey')
    op.create_foreign_key('drivers_user_id_fkey', 'drivers', 'users', ['user_id'], ['user_id'])
    op.drop_constraint('admin_info_user_id_fkey', 'admin_info', type_='foreignkey')
    op.create_foreign_key('admin_info_user_id_fkey', 'admin_info', 'users', ['user_id'], ['user_id'])

---

Testing Concerns 🧪

Missing Test Coverage

The PR doesn't include tests for:

1. ✗ Cascade delete functionality - should verify that deleting a user also deletes associated driver/admin records
2. ✗ Role-based authorization - should verify that "driver" and "admin" roles are properly enforced
3. ✗ Firebase custom claims - should verify that the role claim is correctly set and checked

Recommendation: Add integration tests to verify:

• Deleting a user cascades to driver/admin tables
• Admin-only routes reject driver users
• Driver routes accept driver users
• Firebase claims are properly set on user creation

---

Performance Considerations ⚡

No performance concerns identified. The cascade delete happens at the database level which is efficient.

---

Security Assessment 🔒

Issues Found:

1. Potential Token Verification Bypass: The is_authorized_by_role function (backend/python/app/services/implementations/auth_service.py:155-168) catches all exceptions and returns False. While this is safe from an authorization perspective, it could hide bugs like Firebase connectivity issues.

   Recommendation: Log the specific exception type and consider differentiating between "unauthorized" and "error checking authorization" cases.

2. Firebase Custom Claims Trust: The system now fully trusts Firebase custom claims for role authorization without a database role check. This was explicitly noted in a removed TODO comment. While this is acceptable for performance, ensure:

   • Firebase security rules prevent users from modifying their own claims
   • Regular audits of Firebase user claims vs database roles
   • Clear documentation of this trust relationship

---

Recommendations Summary

Must Fix Before Merge:

1. Fix inconsistent HTTP status codes in authorization checks (401 vs 403)
2. Verify role casing is consistent throughout codebase
3. Add tests for cascade delete and role authorization

Should Fix Before Merge:

1. Move hardcoded admin email to configuration
2. Clean up test route message
3. Add proper error tracking to migration script
4. Use explicit constraint names in migration downgrade

Consider for Future:

1. Add database-level role validation for critical operations
2. Implement role audit logging
3. Add integration tests for Firebase claims

---

Reviewer's Question

using delete user as the main source of truth, ie should I get rid of delete_driver service then?

Answer: With cascade delete enabled, you should keep delete_driver_by_id but update its implementation. Currently it likely tries to manually delete the driver record. Instead, it should:

• Call user_service.delete_user_by_id() with the driver's user_id
• Let cascade delete handle removing the driver record
• This maintains a clear separation of concerns

Alternatively, deprecate delete_driver_by_id and always use delete_user_by_id, but update documentation to make this clear to other developers.

---

Overall Assessment

Status: ⚠️ Needs Minor Revisions

The PR accomplishes its goals well, but has a few issues that should be addressed before merging. The core functionality is sound, but testing, error handling, and minor inconsistencies need attention.

Estimated Effort to Address: Small - most issues are quick fixes.

[ludavidca]

Just a quick food for thought, in our codebase, there are two roles drivers and admin, and some routes are admin only. This means that by default, admin should be able to access all routes, so it might not make sense to have a require driver.

This gives 2 solutions:

• Simpler: Delete the require driver entirely and only use require_admin, and only require valid auth checks for some api routes.
• More Complex but better: delete require_driver, and use ID-based checks for driver self-service (e.g., driver can only update their own profile)
  Admin ID bypasses all id checks or has access to all IDs

Lmk which one you would prefer and your thoughts!

[ludavidca]

Great Job making these changes! User System is definitely one of the most critical and important and complex parts of the project and I'm really happy with how much work you've put into this. There are still a couple of small comments and nice to haves. Lmk your thoughts!

[ludavidca]

Might be best to have this as an env variable to avoid hardcoded values

[ludavidca]

Great job on implementing a way to update all roles!

I realize that I should have probably better specified this in the ticket, but can we add another function to update only one user to admin or driver instead? This way, we won't have people changing other people's roles in the middle of development. Thanks!

[ludavidca]

Once the update_your_user_role is implemented (with input param being your email). Please feel free to make that the default main function that runs, and make a variable your_email = "" to be used by the function!