[MISC] Preserve view function identity in platform-service auth middleware#2160
Conversation
Summary by CodeRabbit
WalkthroughThe platform authentication middleware now preserves wrapped function metadata, allowing Flask routes to register under their function names without explicit endpoint arguments. Test defaults and pytest coverage were added for token validation, database errors, and route endpoint registration. ChangesPlatform routing and authentication
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
fd7cf75 to
3427d73
Compare
`authentication_middleware` returned an unwrapped closure, so every decorated view function's `__name__` was "wrapper". Flask derives an endpoint name from `__name__`, which meant: - Six routes carried a redundant `endpoint="..."` kwarg purely to dodge the resulting name collision. - `/usage`, the one authenticated route without that kwarg, registered as endpoint `platform.wrapper`. - Adding any new authenticated route without `endpoint=` would fail at import with "View function mapping is overwriting an existing endpoint function: platform.wrapper". Add `functools.wraps` and drop the now-redundant `endpoint=` kwargs. URL rules and methods are unchanged; only the internal Flask endpoint names change (`platform.wrapper` -> `platform.usage`). Nothing in the repo resolves routes via `url_for`, and callers reach platform-service over HTTP by path, so this is not externally observable. x2text-service's equivalent middleware already patched `wrapper.__name__` by hand; this brings platform-service in line. Also repair `tests/test_auth_middleware.py`, which imported `unstract.platform_service.main` (removed) and `get_account_from_bearer_token` (removed) and so broke collection of the entire platform-service suite. It now covers `validate_bearer_token` against a mocked cursor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014gUmgQQ7xV73qn6kcB7mXG
3427d73 to
8cdc7e5
Compare
|
|
| Filename | Overview |
|---|---|
| platform-service/src/unstract/platform_service/controller/platform.py | Adds functools.wraps to the auth wrapper and removes endpoint overrides that matched the function names. |
| platform-service/tests/conftest.py | Sets test-scope defaults for environment variables required during platform-service imports. |
| platform-service/tests/test_auth_middleware.py | Adds hermetic tests for valid, missing, unknown, inactive, and DB-error bearer-token cases. |
| platform-service/tests/test_route_endpoints.py | Adds a regression test that ensures authenticated routes do not register as platform.wrapper. |
Reviews (1): Last reviewed commit: "fix: preserve view function identity in ..." | Re-trigger Greptile
There was a problem hiding this comment.
🧹 Nitpick comments (1)
platform-service/tests/test_route_endpoints.py (1)
11-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting the other six endpoint names to prevent silent regressions.
The test correctly verifies the core fix (
platform.wrapperabsent,platform.usagepresent), but the six routes that had explicitendpoint=arguments removed are not checked. Adding assertions for those endpoints would catch accidental function renames that silently change endpoint names and breakurl_forcalls.♻️ Proposed additional assertions
def test_routes_register_under_their_function_names() -> None: # NOSONAR — app is never served; bearer-token routes carry no CSRF surface. app = Flask(__name__) # NOSONAR app.register_blueprint(platform_bp) endpoints = {rule.endpoint for rule in app.url_map.iter_rules()} assert "platform.wrapper" not in endpoints assert "platform.usage" in endpoints + assert "platform.page_usage" in endpoints + assert "platform.platform_details" in endpoints + assert "platform.cache" in endpoints + assert "platform.adapter_instance" in endpoints + assert "platform.custom_tool_instance" in endpoints + assert "platform.agentic_tool_instance" in endpoints + assert "platform.llm_profile_instance" in endpoints🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-service/tests/test_route_endpoints.py` around lines 11 - 18, Extend test_routes_register_under_their_function_names to assert that all six remaining route endpoint names are present in the endpoints set, alongside the existing platform.wrapper and platform.usage checks. Use the route view function names from the blueprint as the expected endpoint strings so accidental renames are detected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@platform-service/tests/test_route_endpoints.py`:
- Around line 11-18: Extend test_routes_register_under_their_function_names to
assert that all six remaining route endpoint names are present in the endpoints
set, alongside the existing platform.wrapper and platform.usage checks. Use the
route view function names from the blueprint as the expected endpoint strings so
accidental renames are detected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 78b14d02-71c6-4fe4-950e-7be3de7b1e29
📒 Files selected for processing (4)
platform-service/src/unstract/platform_service/controller/platform.pyplatform-service/tests/conftest.pyplatform-service/tests/test_auth_middleware.pyplatform-service/tests/test_route_endpoints.py
Unstract test resultsPer-group results
Critical paths
|
Declining this one, deliberately. The stated risk doesn't exist in this repo: there are zero So pinning the other six endpoint names would assert on an internal Flask registry key that no code reads, and would need editing every time a route is added. The one assertion that carries signal is Happy to add them if a maintainer wants the endpoint names pinned as a contract, but today they aren't one. |



What
Adds
functools.wrapstoauthentication_middlewareinplatform-service, and drops the six now-redundantendpoint="..."kwargs it was forcing onto@platform_bp.route(...).Follow-up to the discussion on #1964. That PR fixed a decorator-order bug in
prompt-service(auth stacked aboveroute, so auth never ran). That bug does not exist inplatform-service— all its routes already have@routeon top. But a sibling defect in the same middleware does.Why
authentication_middlewarereturned a bare closure:Flask derives an endpoint name from the view function's
__name__. So every authenticated view registered aswrapper. Consequences:endpoint="..."kwarg whose only purpose was to dodge the resulting collision — each value already identical to its function name./usagewas the one authenticated route without that kwarg, so it registered as endpointplatform.wrapper. It worked only because it was the last one left defaulting.endpoint=fails at import withAssertionError: View function mapping is overwriting an existing endpoint function: platform.wrapper. A landmine, not a live bug.Reproduced on
mainbefore the fix:x2text-service's equivalent middleware already patches this by hand (wrapper.__name__ = func.__name__,app/authentication_middleware.py:19). This bringsplatform-servicein line, using the stdlib decorator rather than a manual assignment.How
controller/platform.py@functools.wraps(func)on the middleware'swrapper; drop 6 redundantendpoint=kwargstests/test_route_endpoints.pytests/test_auth_middleware.pytests/conftest.pyplatform_service.envdemands at importCan this PR break any existing features. If yes, please list possible items. If no, please explain why.
No.
URL paths and methods are byte-identical before and after — verified by dumping
app.url_mapon both sides. Only Flask's internal endpoint registry key changes, and only for one route:platform.wrapper→platform.usage.An endpoint name is what
url_for()andapp.view_functions[...]look up. It is not the URL.url_for,view_functions, orurl_mapanywhere in the repo (venvs excluded). Nothing resolves a route by endpoint name./usageisunstract/sdk1/src/unstract/sdk1/audit.py:128—url = f"{base_url}/usage", a plain HTTP path. Backend and workers reach platform-service over HTTP by path, never by endpoint name.v1/usage/hits inbackend/andworkers/are Django's own REST APIs, a different service — unrelated to this Flask route.Auth enforcement itself is unchanged: these routes were already correctly ordered and already authenticated.
Database Migrations
N/A
Env Config
N/A
Relevant Docs
N/A
Related Issues or PRs
Follow-up to #1964 (which targets
prompt-service). Rebased on top of #2115 / #2151.Dependencies Versions
N/A
Notes on Testing
Runs as the rig's existing
unit-platform-servicegroup (tests/groups.yaml) — no new group needed:test_route_endpoints.py— a single test that registers the realplatform_bpand asserts no endpoint is namedplatform.wrapper. Verified as a real regression test: with@functools.wrapsremoved it fails with exactly the collision this PR prevents —test_auth_middleware.py— #2115 deleted this file because it importedunstract.platform_service.main(removed) andget_account_from_bearer_token(removed), and made live Postgres calls. That deletion was correct, and it left the wholeplatform-servicesuite uncollectable until then. This PR restores it as a hermetic test ofvalidate_bearer_tokenagainst a mocked cursor: accepts a valid active token; rejects missing, unknown, and inactive tokens; and fails closed when the token lookup raises. No live DB, no removed imports.Five auth cases + one route case. I deliberately did not test
validate_bearer_token'splatform_key != tokenbranch: the query isWHERE key = %swithparams=(token,), so that branch is unreachable defensive code and a test for it would be manufacturing coverage.Sonar
The initial push failed the quality gate with three issues, all in the new test files:
Reliability C) — valid. Ayieldafterraisein the DB-error mock was genuinely unreachable; the@contextmanagerwas unnecessary sincesafe_cursor(...)is invoked inside the function's owntry. Fixed properly rather than suppressed.Security D) — false positive. It flagsFlask(__name__)as "CSRF protection disabled". The app is constructed in a unit test, never served, and exists only sourl_mapmaterialises; platform-service authenticates with bearer tokens and serves no cookie- or form-backed routes, so CSRF does not apply. Suppressed with# NOSONARplus justification, matching the existing convention inbackend/utils/tests/test_cors_origin.py.Quality gate now passes with 0 open issues.
Checklist
I have read and understood the Contribution Guidelines.
🤖 Generated with Claude Code
https://claude.ai/code/session_014gUmgQQ7xV73qn6kcB7mXG