agentic-backend-pr-comparison.md

Agentic-Backend PR Comparison: #363 vs #362

PR #363 (sso_user_id_updates by CapitolCoder) — 527 additions, 8 deletions, 3 files PR #362 (feature/COM-18-ad-phase-1 by jordotech) — 8 additions, 1 deletion, 1 file

Both PRs add Authorization: Bearer header support to get_current_user() so SSO users can authenticate with agentic-backend.

Diff Comparison

Aspect	PR #363	PR #362 (ours)
Lines changed (impl)	19 additions, 8 deletions	8 additions, 1 deletion
Lines changed (tests)	508 additions	0 (manual test plan)
Files	3 (auth.py + 2 new test files)	1 (auth.py only)
Bearer parsing	startswith("Bearer ") + removeprefix("Bearer ")	split(None, 1) + casefold() (RFC 7235)
check_token() change	Sends both X-User-Token and Authorization: Bearer headers	No change — sends X-User-Token only

The Core Implementation

Both PRs add the same 3-line pattern to get_current_user():

# PR #363
token = x_user_token
if not token and authorization and authorization.startswith("Bearer "):
    token = authorization.removeprefix("Bearer ")

# PR #362
token = x_user_token
if not token and authorization:
    parts = authorization.split(None, 1)
    if len(parts) == 2 and parts[0].casefold() == "bearer":
        token = parts[1].strip()

Issues with PR #363

1. Case-sensitive Bearer parsing — startswith("Bearer ") fails on bearer, BEARER, or bearer which are all valid per RFC 7235 Section 2.1. Our PR uses casefold() for case-insensitive matching, which was specifically flagged and fixed based on GitHub Copilot review feedback.

2. Unnecessary check_token() dual-header change — PR #363 modifies check_token() to forward both X-User-Token and Authorization: Bearer to platform-api. This is unnecessary because:

   • Platform-api's /validate endpoint reads X-User-Token header
   • The token is the same regardless of which header it came from
   • Sending both just doubles the header traffic with no benefit
   • It also means platform-api's /validate now needs to handle Authorization header parsing, adding scope to a different PR

3. 508 lines of tests for a 3-line change — Two new test files with extensive mocking of aiohttp context managers. The tests are thorough but the ratio is extreme for what is essentially a header extraction change.

4. Import reordering — Reformats all imports in auth.py (isort-style). While clean, it creates unnecessary diff noise and potential merge conflicts.

What PR #363 Gets Right

• Good test coverage for edge cases (empty Bearer, missing prefix, priority ordering)
• Documents the dual-header contract clearly in docstrings
• Tests verify check_token() sends headers correctly

Recommendation

PR #362 is the better choice. It's a minimal, focused change (8 lines, 1 file) that solves the exact problem — SSO users sending Authorization: Bearer couldn't authenticate. The RFC 7235-compliant parsing handles edge cases that PR #363 misses. The check_token() dual-header change in PR #363 is unnecessary and adds coupling between agentic-backend and platform-api's header handling.

The test files from PR #363 could be adapted and added as a follow-up if test coverage is desired, but the implementation from PR #362 should be used.