Extract create/update/delete helpers to api/mutation - #97
Merged
Merged
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.
ramonski
force-pushed
the
refactor/api-mutation
branch
from
July 25, 2026 20:08
8bd3cf6 to
4a53157
Compare
xispa
approved these changes
Jul 31, 2026
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
Continues the incremental split of
senaite.jsonapi.api(previous: #93 users, #94 settings, #95 serialization, #96 typed exceptions). This PR extracts the biggest remaining slice — the create/update/delete surface — soapi/__init__.pyshrinks to the utility helpers only and the mutation code lives in one focused place. Prerequisite for wrapping these into MCP tools cleanly.What this PR does
Moves out of
api/__init__.pyinto a newapi/mutation.py:Route orchestrators (called by
v1/routes/content.actionviagetattr(api, "<action>_items")):create_items,update_items,patch_items,put_items,delete_itemsLow-level building blocks:
create_object— dispatches toICreateadapter / AnalysisRequest special-case / standardbika_api.create+update_object_with_datacreate_analysisrequest— thin wrapper aroundbika.lims.utils.analysisrequest.create_analysisrequestupdate_object_with_data—IUpdateadapter orIDataManagerset-loop + validation + optional transitiondeactivate_object— SENAITE's soft-delete viadeactivateworkflow transitionfind_target_container— resolvesparent_uid/parent_pathfrom the recordvalidate_object(still@deprecated — kept for compat)Permission checks:
is_creation_allowed— portal/setup/senaite_setup denylist + adapter opinionis_update_allowed— same denylist against the object's parent + adapter opinionAll 13 names remain importable from
senaite.jsonapi.apivia explicit re-exports at the bottom of__init__.py. Route code (v1/routes/content.action) that doesgetattr(api, "create_items")etc. keeps working unchanged.Semantic tightening while moving
Every
fail(401, ...)inside the moved code becomes a properForbiddenError(HTTP 403). 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, ...)becomesBadRequestError,fail(404, ...)becomesNotFoundError. Backward compat is preserved through the sharedAPIErrorbase.Drop unused imports from init.py
Nine imports were only used by the extracted functions:
copy,transactionAccessControl.Unauthorizedbika.lims.utils.analysisrequest.create_analysisrequest as create_arsenaite.jsonapi.interfaces.ICreate,IUpdate,IInfozope.component.getAdapterszope.deprecation.deprecateBase branch
Stacked on top of #96 (
refactor/typed-exceptions). Merge order: #92 → #93 → #94 → #95 → #96 → #97.Verify
--
I confirm I have tested the PR thoroughly and coded it according to PEP8 standards.