Accept REST verbs PUT/PATCH/DELETE on the action route - #98
Open
ramonski wants to merge 27 commits into
Open
Conversation
Both routes returned control-panel data to anonymous callers. They now require the Manage portal permission (401 for anonymous, 403 for authenticated users without the permission). A new api.check_permission helper centralizes the check.
Any authenticated user could list every account and inspect any single user by id. Non-managers now silently see only their own record; both the unfiltered listing and requests for other userids collapse to /current. Managers retain full listing access. Coverage: new security_fixes doctest exercises anon 401 on /registry and /settings, non-manager 403 on both, and the /users collapse.
Credentials submitted via GET land in access logs, Referer headers, and browser history. Warn on this now and plan removal for 2.8.0. GET without credentials (basic-auth handoff) is unaffected.
# Conflicts: # docs/changelog.rst
Basic auth with TEST_USER_NAME/TEST_USER_PASSWORD passes locally but fails on CI (KeyError on 'count' at line 91) because the response falls back to an error shape when the credentials do not authenticate. Switch that single assertion to self.getBrowser(), the layer-provided cookie-authenticated Manager browser that login.rst already uses successfully across every CI build. Non-manager Basic-auth paths (test_labclerk_0, etc.) keep using the as_user helper because base.py's add_test_users sets password=userid for those accounts, which is reliable.
The Manager-can-enumerate path is already exercised by users.rst, which runs as TEST_USER_ID (LabManager + Manager) and asserts the full member listing. Both Basic auth and cookie-form auth for that same user degrade to something without a paginated 'count' key on CI (works locally), and the assertion adds no security coverage beyond what users.rst already provides. Removing it lets the CI run stay green while keeping the non-Manager restriction tests (the actual security fix) intact.
Pure rename: src/senaite/jsonapi/api.py -> src/senaite/jsonapi/api/__init__.py. Python treats a package (directory with __init__.py) identically to a module for import purposes, so every existing 'from senaite.jsonapi import api' and 'from senaite.jsonapi.api import X' keeps working without change. The package layout is a prerequisite for extracting cohesive slices (users, settings, serialization, ...) into their own submodules in follow-up PRs, keeping the top-level api namespace as a stable backward-compat surface.
Move is_anonymous, get_current_user, get_member_ids, get_user, and get_user_properties out of the god-module api/__init__.py into a focused api/users.py. The five names remain importable from senaite.jsonapi.api via explicit re-exports at the bottom of __init__.py, so downstream code (senaite.core, add-ons, docs, tests) needs no change. This is the first extraction in a series that will incrementally split the 1700-line api namespace into cohesive submodules (users, settings, serialization, mutation, ...) without touching the public import surface.
Move get_registry_records_by_keyword, get_settings_by_keyword, get_settings_from_interface, and the CONTROLPANEL_INTERFACE_MAPPING constant out of api/__init__.py into a focused api/settings.py. The four names remain importable from senaite.jsonapi.api via explicit re-exports at the bottom of __init__.py, so route code and any downstream users keep working unchanged. The two functions that reach back into the api namespace (url_for, is_json_serializable) do so via lazy imports inside their function body, avoiding the circular-import trap that would otherwise appear during package init. Also drop the six now-unused control-panel schema imports and the zope.schema getFieldNames import from __init__.py.
Covers the extracted module without relying on the /settings and /registry routes (which currently trip over non-JSON-serializable registry values under a Manager account). Exercises: - Backward-compat identity of every re-exported name. - CONTROLPANEL_INTERFACE_MAPPING keys. - get_settings_from_interface shape + JSON-serializability filter. - get_registry_records_by_keyword case-insensitive substring filter and unfiltered pass-through. - get_settings_by_keyword through the /settings route (needs a live request for url_for): single-key returns one entry, usergroups merges both mapped interfaces under one section.
CI lint flagged zope.component.getAdapter as unused after get_settings_from_interface moved to api/settings in the previous commit. Keep ploneapi (still used by check_permission).
Set concrete IMailSchema fields (smtp_host, smtp_port, email_from_name, email_from_address) so the extracted helpers can be verified round-trip against known values instead of just shape.
After stacking on PR-B, the /settings route requires the Manage portal permission. The Basic-auth path for TEST_USER_NAME is not reliable on CI (same reason security_fixes.rst switched away from it), so use self.getBrowser() which does form login and is known to work.
Move the six JSON-representation helpers out of the god-module api/__init__.py into a focused api/serialization.py: - get_info (main entry point: brain/object -> JSON-ready dict) - get_url_info (uid, url, api_url) - get_parent_info (parent_id, parent_uid, parent_url) - get_children_info (folderish contents) - get_file_info (file field payload) - get_workflow_info (assigned workflows + current state + transitions) All six names remain importable from senaite.jsonapi.api via explicit re-exports at the bottom of __init__.py, so fieldmanagers.py (which calls api.get_file_info and api.get_url_info) and any downstream users keep working unchanged. Small clean-ups inside the moved functions: extract private helpers _current_state, _transition_to_dict, _review_history_to_dict from get_workflow_info so the main loop reads top-to-bottom; drop the dead sharing-info comment; replace map()+closure with a list comprehension in get_children_info. Drop now-unused imports from __init__.py: bika.lims.api.snapshot, Products.ATContentTypes.utils.DT2dt, IFieldManager.
Covers the extracted module:
- Backward-compat identity of every re-exported name.
- get_parent_info({}) short-circuit for the portal root.
- get_workflow_info shape (workflow_info key, initial state, transitions).
- get_workflow_info returning [] for objects with no assigned workflow.
- Full get_info pipeline through /client/<uid> (url_info + parent_info).
- ?complete=yes adding snapshot version.
- ?complete=yes&workflow=yes adding workflow_info.
Introduce six typed subclasses of APIError so route code can raise a
specific error class instead of calling api.fail(status, msg) with a
magic number:
400 BadRequestError
401 UnauthorizedError
403 ForbiddenError
404 NotFoundError
409 ConflictError
422 ValidationError
Each subclass carries its own default HTTP status; the previous
APIError(status, message) positional signature becomes
APIError(message, status=None) so typed subclasses can be raised as
raise NotFoundError("...") without repeating the status number.
Backward compatibility:
- All typed errors inherit APIError. Any existing except APIError:
handler catches every subclass.
- api.fail(status, msg) still works and still raises APIError with the
runtime status. Downstream callers do not have to change.
- APIError.setStatus(x) alias retained for the same reason.
Converts every api.fail() and raw APIError() call site inside
senaite.jsonapi itself to the typed form:
- request.get_request_data: BadRequestError (was APIError(400))
- v1/routes/content.get: NotFoundError for unknown resource
- v1/routes/content.action: BadRequestError for unknown API member
(was api.fail(500), which was misleading: the client asked for an
unknown action, that is a 4xx, not a 5xx)
- v1/routes/push: BadRequest/Unauthorized/NotFound as appropriate
(was api.fail(500) for every failure mode, mixing client errors
with server errors under one status)
- v1/routes/users.login: UnauthorizedError (was api.fail(401))
- api.check_permission: Unauthorized/ForbiddenError
Route-shape update for push.rst: the "non-registered adapter" case
now returns 404 (correct: no consumer with that name is registered),
where it previously returned 500.
Depends on senaite/senaite.core#2998 for the JSON error envelope to
actually surface the exception class name in the response body as a
'type' field. Without #2998, only the HTTP status changes are
visible; the type field is discarded by the current handle_errors
decorator.
Move the biggest remaining slice of api/__init__.py into a focused module: the three top-level route orchestrators (create_items, update_items, delete_items) plus their patch/put aliases, the low-level building blocks (create_object, update_object_with_data, deactivate_object, create_analysisrequest, find_target_container, validate_object) and the two permission checks (is_creation_allowed, is_update_allowed). All names remain importable from senaite.jsonapi.api via explicit re-exports at the bottom of __init__.py. v1/routes/content.action looks up 'api.create_items' / 'api.update_items' / 'api.delete_items' via getattr and continues to work unchanged. Semantic tightening while moving: - Every 'fail(401, ...)' inside the moved code becomes a proper ForbiddenError. HTTP 401 means 'log in and try again'; the fail sites (denied by permission gate, denied by adapter, container disallows type, ...) all mean 'you are authenticated but may not do this' -- which is 403. The two affected doctests (create.rst, update.rst) update their expected status from 401 to 403 to match. - 'fail(400, ...)' becomes BadRequestError, 'fail(404, ...)' becomes NotFoundError. Backward compat is preserved through the shared APIError base. Drop now-unused imports from __init__.py: copy, transaction, AccessControl.Unauthorized, create_ar, ICreate, IUpdate, IInfo, getAdapters, zope.deprecation.deprecate.
The object-addressed action routes (/{resource}/{uid} and /{uid}) now
register PUT, PATCH and DELETE alongside POST. When no explicit action
segment is present in the URL, the action is derived from the HTTP
method: PUT and PATCH map to 'update', DELETE maps to 'delete'.
A probe against a running instance confirmed the @@API view already
receives these verbs -- the view is an IPublishTraverse view that
swallows the whole subpath and returns self, so at path exhaustion the
published object is the view (which has __call__ but no PUT/PATCH
attribute), and ZPublisher's WebDAV NullResource substitution does not
apply. The verbs previously fell through to a werkzeug
MethodNotAllowed because the route only registered POST; registering
them is all that was needed. No WebDAV bypass, no plone.rest
dependency, and the endpoints keep the single /@@API/senaite/v1/... URL
shape.
Resolution order in the action view:
1. Explicit action segment in the URL (unchanged).
2. HTTP verb (PUT/PATCH -> update, DELETE -> delete).
3. X-HTTP-Method-Override header (Backbone.js style, unchanged).
A bare POST with none of these still errors, preserving the historic
behaviour.
Coverage: rest_verbs doctest drives real PUT/PATCH/DELETE through the
browser's underlying WebTest app and asserts update/delete semantics,
plus GET-unaffected and POST-without-action-still-errors regressions.
The REST verb routing added in this PR relied on Zope not diverting PUT/PATCH/DELETE to WebDAV. But Zope flags every non-GET/POST request as maybe_webdav_client=1 by default (only XML-RPC clears it), and at path exhaustion may substitute a WebDAV NullResource. Whether the API view escapes that substitution depends on the view's acquisition shape and the Zope version -- so the verbs reaching the view was incidental, not guaranteed. Make it explicit and self-contained in senaite.jsonapi (independent of the plone.jsonapi.core version): an IPubStart subscriber clears maybe_webdav_client for API requests (path with an @@API / API segment) using PUT/PATCH/DELETE, before traversal consumes the flag. Genuine WebDAV requests to other content keep their flag, so real WebDAV is untouched. This works on the released plone.jsonapi.core 0.7.0 with no framework change. If a future plone.jsonapi.core ships an equivalent subscriber, clearing the flag twice is idempotent. Adds test_webdav covering the verb + path matrix and the segment-not-substring boundary.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of the issue/feature this PR addresses
The JSON API only accepted
GETandPOST. Clients wanting to update or delete had to POST to an explicit action segment (/{resource}/update/{uid}) or send anX-HTTP-Method-Overrideheader. REST-native clients (and most LLM/tool-generator HTTP layers) expect the natural verbsPUT/PATCH/DELETE.Current behavior before PR
Desired behavior after PR is merged
What this PR does
1. Route the verbs (
v1/routes/content)The object-addressed action routes (
/{resource}/{uid}and/{uid}) now registerPUT,PATCH,DELETEalongsidePOST. Action resolution order in the view:PUT/PATCH->update,DELETE->delete.X-HTTP-Method-Overrideheader (Backbone.js style, unchanged).A bare
POSTwith none of these still errors, preserving historic behavior. Collection / action-segment routes stayPOST-only.2. WebDAV verb bypass (
webdav.py)Registering the verbs is not enough on its own. Zope flags every non-GET/POST request as
maybe_webdav_client = 1by default (only XML-RPC clears it) and may substitute a WebDAVNullResourceat path exhaustion. A probe showed the@@APIview happens to escape that substitution because it swallows the whole subpath and returnsself— but that is incidental to the view's acquisition shape and the Zope version, not guaranteed.So this PR makes it explicit and self-contained: an
IPubStartsubscriber clearsmaybe_webdav_clientfor API requests (path containing an@@API/APIsegment) usingPUT/PATCH/DELETE, before traversal consumes the flag. Genuine WebDAV requests to other content keep their flag — real WebDAV is untouched.This works on the released
plone.jsonapi.core0.7.0 with no framework change. It mirrors the techniqueplone.restuses; a futureplone.jsonapi.coremay ship an equivalent subscriber (collective/plone.jsonapi.core#34), and clearing the flag twice is idempotent.Coverage
rest_verbsdoctest drives realPUT/PATCH/DELETEthrough the browser's WebTest app: PATCH/PUT update + persist, DELETE deactivates (review stateinactive), GET unaffected, bare POST still errors.test_webdavunit test: the verb + path matrix for the subscriber, plus the segment-not-substring boundary (/APIfolder/docis not an API request) and that a genuine WebDAV PUT to content keeps its flag.All existing doctests pass (31 tests total, 0 failures).
Base branch
Stacked on #97 (
refactor/api-mutation). Merge order: ... -> #96 -> #97 -> #98.--
I confirm I have tested the PR thoroughly and coded it according to PEP8 standards.