[F4KRP-116] Auth middleware

[Jam-Cai]

F4KRP-116

Implementation Description

• Added auth dependencies in auth.py, role-based (require_admin, require_authorization_by_role), identity-based (require_authorization_by_user_id, require_authorization_by_email), and token extraction helpers (get_current_user_id, get_current_user_email, get_current_database_user_id).
• Added role based route protection
• Added unittests

Steps to Test

1. run docker-compose exec backend python -m pytest tests/test_auth_middleware.py -v --no-cov
2. login as a user and test rbac gated endpoints

What Should Reviewers Focus On?

• double check if roles for endpoints are correct

Checklist

• PR name and commits are descriptive, imperative, and atomic (trivial commits squashed)
• I have requested a review from Claude, understood its suggestions, and implemented fixes where needed (or documented disagreements)
• I have requested a review from the PL and relevant devs with background on this PR

[claude]

Claude Code Review — Auth Middleware

Good foundation! The middleware structure is clean and the test suite is well-organized. A few issues worth addressing before merging:

---

Bugs / Correctness

1. require_self_or_admin is never applied to routes

The function was written for endpoints like GET /drivers/{driver_id} (as the docstring states), but the route still uses require_driver_or_admin. This means any authenticated driver can currently read any other driver's data.

# driver_routes.py — should this be require_self_or_admin instead?
_auth: bool = Depends(require_driver_or_admin),

Similarly, GET /drivers/ (list all drivers) uses require_driver_or_admin, so a driver can enumerate all other drivers. Please confirm this is intentional.

2. get_current_database_user_id swallows its own HTTPException

The if database_user_id is None path raises HTTPException(detail="User not found") inside the try block, which is then caught by the broad except Exception and re-raised as "Invalid or expired token". The real error is hidden from the caller. Fix by re-raising HTTPException before the generic handler:

except HTTPException:
    raise
except Exception as e:
    ...

The same pattern exists in require_authorization_by_user_id and require_authorization_by_email (HTTPException raised inside a try/except Exception), though in those cases the messages happen to be the same so there's no observable difference.

3. require_self_or_admin — unguarded UUID() call

UUID(user_id) will raise ValueError if the string isn't a valid UUID, and this exception is not caught in check_self_or_admin. All other similar functions wrap the entire body in try/except. Add handling or let it propagate as a 422.

---

Security Concerns

4. GET /database/table allows drivers

This endpoint's docstring says it "Gets ALL tables and their data - show everything in the database". Allowing drivers (not just admins) to hit this endpoint seems overly permissive. Please double-check this is intentional.

5. Write operations allow drivers

Many mutating endpoints (create/update/delete for locations, location groups, route groups, driver history) are now gated to require_driver_or_admin. Should drivers have write access to all of these? Worth explicitly confirming with the team.

---

Minor Issues

6. Wrong status code in get_current_user_role

Returning HTTP_401_UNAUTHORIZED when a user has no role assigned is misleading — the user is authenticated, they just lack authorization. HTTP_403_FORBIDDEN would be more appropriate here (consistent with how require_authorization_by_role handles it).

7. Redundant token verification in require_self_or_admin

The function calls firebase_admin.auth.verify_id_token() directly, and then auth_service.is_authorized_by_driver_id() likely verifies the token again internally. Consider passing the decoded token to the service to avoid the double round-trip.

---

Test Coverage Gaps

The following new functions have no tests:

• require_self_or_admin
• require_authenticated
• get_current_user_role
• require_driver / require_driver_or_admin (pre-built shortcuts)

Also worth adding:

• A test for require_self_or_admin where the user is an admin (should pass without checking driver ID)
• A test for require_self_or_admin where the user is a driver accessing someone else's resource (should 403)

---

Nitpick

In TestRequireAuthorizationByUserId.test_non_matching_user_id_returns_401, the test name says 401 but the comment would be clearer if it noted this is an authorization failure (403 is typically more correct for authz vs authn failures — the existing implementation uses 401 here, which is pre-existing behavior worth reconsidering).

[ludavidca]

Hey James, thanks for putting this up! I've gone through and left comments throughout.

Honestly, I think a good chunk of the issues come down to me not being as clear enough in the ticket requirements. That's on me, and I want to make sure we get aligned before you spend more time on revisions.

There are also a few bugs and logic issues I flagged inline, take a look when you get a chance.

Can we grab some time to sync on this today? I want to walk through the comments together so we're on the same page about the direction, and then the fixes should be a lot more straightforward from there.

Requesting changes for now, but we'll sort it out soon :)