feat(spp_attendance): migrate from openspp-modules - #435
feat(spp_attendance): migrate from openspp-modules#435gonzalesedwin1123 wants to merge 14 commits into
Conversation
Verbatim copy of spp_attendance from openspp-modules @ 5a1afb71b. Adaptation and fixes follow in separate commits.
Website URL, version 19.0.2.0.0 (above the released 19.0.1.3.1 so the secret-hash migration runs on upgrades), HISTORY/USAGE fragments, spp_session_tracking positioning in DESCRIPTION, drop README.rst.bak. USAGE documents registry-sync configuration against an OpenSPP2 registry (spp_dci_server search + spp_api_v2 token endpoints).
Three code paths stamped Male when gender was unknown: the res.partner
field default, the subscriber related-field default, and the registry
import fallback ('gender or "Male"'). All removed — gender is now only
what synced/imported data actually provides.
Secrets were stored in plaintext forever (the existing show-once button was only a UI gate, bypassable via RPC read), searchable in plaintext by the token endpoint, and readable by the Viewer group. - client_secret_hash (scrypt, $scrypt$salt$hash — same construction as spp_api_v2's spp.api.client) is now the only stored form; authenticate() fetches by client_id and verifies in constant time - the one-time display moves to a transient wizard; the record's plaintext is scrubbed at wizard-open, before the dialog renders - action_regenerate_secret rotates without ever storing plaintext - Viewer group loses read access to the credential model - migrations/19.0.2.0.0: hashes existing plaintext secrets in place and scrubs the column (idempotent); clients keep authenticating with their unchanged secrets — only DB recoverability of the plaintext disappears
Hybrid strategy (option 3): controller logic + all write endpoints run in TransactionCase against a mocked request (writable env, every validation branch); a thin HttpCase layer smokes the true end-to-end paths (token mint, 401 rejection, read endpoints) since HTTP-served requests are read-only against the test transaction. Plus credential hashing/show-once/migration tests, subscriber model tests, and import wizard tests with mocked HTTP. Two tests are intentional TDD reds reproducing wizard bugs found in review (KeyError composing the missing-config message; Basic tokens double-prefixed by an operator-precedence slip) — fixes follow.
date_utils.json_default no longer exists in Odoo 19 — every controller response with a body crashed with AttributeError, which the new test suite exposed on its first run (the module shipped without tests, so this was invisible until now). Use odoo.tools.json_default. Also: element_mapper short-circuits empty containers to None (test expectation corrected) and re-add the ValidationError import the lint hook stripped.
- check_required_fields crashed with KeyError composing the friendly
missing-config message (looked the names up in ir.config_parameter's
fields; they are res.config.settings fields)
- operator precedence in the token-scheme guard double-prefixed tokens
that already carried Basic ('Basic Basic xyz')
Both were verified red before the fix (test_missing_config_raises_
friendly_error, test_basic_auth_token_not_double_prefixed).
… test fix - requests.post calls get timeout=30 (bandit B113 / pylint E8106: a hung remote registry froze the worker) - inverse method renamed _inverse_unique_fields (C8110) - drop _() from field help strings (W8103) and redundant string= attrs (W8113); prettier XML formatting - requirements.txt: pytz (generated from the manifest by the hook) - test: partner-name assertion made case-insensitive — spp_registry normalizes partner names to uppercase when co-installed
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Semgrep OSS found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 19.0 #435 +/- ##
==========================================
+ Coverage 72.24% 72.76% +0.52%
==========================================
Files 419 433 +14
Lines 29813 30656 +843
==========================================
+ Hits 21539 22308 +769
- Misses 8274 8348 +74
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…ADME rendering Every sudo() flagged by odoo-sudo-without-context is by design: the auth=none API routes have no user context — the JWT bearer check is the authentication and sudo is the access path — and the config-parameter reads follow the standard Odoo pattern. Each site carries an own-line nosemgrep with the reason. README.rst/index.html are CI's own rendering applied verbatim.
The HISTORY fragment predated several fixes: the Odoo 19 json_default API break, the wizard error-path bugs, and the request timeouts. Per the version-bump/changelog rule, all in-PR fixes are now listed.
The previous round applied CI's README diff verbatim — but GitHub Actions had masked 'Bearer <token>' in the log as a credential pattern, so the literal '***' landed in the generated files, and every following diff of that line was masked too (an unfixable-from-logs loop). Reword the USAGE line so the rendered text contains nothing the masker triggers on, and repair the two generated lines.
emjay0921
left a comment
emjay0921
left a comment
There was a problem hiding this comment.
One change requested. Everything else I found is non-blocking and in a separate comment — and the security work here is solid, details there.
The /auth/token suppression's justification is not true
All 14 nosemgrep comments in controllers/controllers.py carry the same justification — auth=none route: JWT bearer verified upstream. I walked every route: 13 are accurate, because every endpoint except one calls validate_request_header_and_body() → verify_auth_header() and returns 401 before touching data. The exception is line 152:
# nosemgrep: odoo-sudo-without-context — auth=none route: JWT bearer verified upstream
client = req.env["spp.attendance.api.client.credential"].sudo().authenticate(client_id, client_secret)This is inside auth_get_access_token — the endpoint that mints the bearer. There is no JWT upstream of it; this line is the authentication step.
The sudo() itself is correct and necessary: an unauthenticated caller has to be able to have its credentials checked. Only the stated reason is wrong — and it is wrong on the most security-sensitive line in the module, where a future reader is most likely to take the comment's word for it instead of re-deriving the argument. Blanket-pasting one justification across every suppression site is what produced it.
Suggested replacement for that one line:
# nosemgrep: odoo-sudo-without-context — unauthenticated token request by design;
# secret is verified against the stored scrypt hash, not read backWorth a scan of the other 13 too, since they came from the same paste — I believe they hold, but the author is better placed to confirm each route's ordering than I am from the outside.
|
Non-blocking findings from the same pass. The scrypt migration is real security work and it holds up — verification notes at the bottom. Security follow-upsPlaintext lifetime is unbounded, and the changelog overclaims it. The one-time wizard exposes the plaintext to every manager, not just its creator.
Architectural question
Removing the fabricated Smaller
Verified, for the recordThe scrypt construction is sound, and the uniformity claim in the comment checks out: same constants and The migration is reachable, which I initially doubted for a module new to this repo: the imported manifest was Three real pre-existing bugs, correctly fixed: the And the test suite is real work for a module that shipped none — malformed-hash rejection, one-time-display scrubbing, and secret rotation are all covered. |
3 participants
Summary
Migrates
spp_attendancefrom openspp-modules (@ 5a1afb71b): a standalone, API-first attendanceservice — own participant registry keyed by
person_identifier, OAuth-secured REST endpoints forexternal attendance submission, config-driven participant sync from a registry, CSV import wizard.
First commit is the verbatim import; every adaptation/fix is its own commit. Version goes
19.0.1.3.1 → 19.0.2.0.0 because the security fix ships a data migration (see below).
Self-containment (review goal): deps stay
base+spp_oauth+spp_security; the moduleconsumes only the stable
spp_oauth.toolshelper API; no other module is touched anywhere in thisPR. Positioning vs
spp_session_tracking(internal program-session attendance — different niche,no shared models) is documented in the DESCRIPTION.
Security fix: client secrets (agreed in review)
Secrets were stored in plaintext forever — the "show once" button was a UI gate, bypassable via
RPC read; the token endpoint searched by plaintext; the Viewer group could read the credentials
table. Now, mirroring
spp_api_v2'sspp.api.clientpattern in-module (no new dependency):$scrypt$salt$hash) is stored;authenticate()fetches byclient_idandverifies in constant time
dialog renders;
action_regenerate_secretrotates without ever storing plaintextmigrations/19.0.2.0.0/hashes existing plaintext secrets in place (idempotent). Clients keepauthenticating with unchanged secrets; only DB recoverability of the plaintext disappears.
Fixes found while building the test suite (each verified red first)
date_utils.json_defaultno longer exists, soevery response with a body crashed with AttributeError. Invisible until now — the module
shipped with zero tests. →
odoo.tools.json_default."Male"when gender was unknown(field default, related-field default, import fallback, subscriber-create fallback). All
removed — gender is only what source data provides.
res.config.settings field names in
ir.config_parameter._fields); token-scheme guarddouble-prefixed Basic tokens (
'Basic Basic xyz', operator precedence).requests.postcalls now carrytimeout=30(bandit B113).Tests (module had none)
47 tests, hybrid strategy: controller logic + all write endpoints in TransactionCase against a
mocked
request(writable env, every validation branch); a thin HttpCase layer smokes trueend-to-end paths (token mint round-trip, 401 rejection, read endpoints) since HTTP-served requests
run read-only against the test transaction. Plus credential hashing/show-once/rotation/migration,
subscriber model, and import wizard (mocked HTTP) coverage.
Verification
-i spp_attendanceonly →47 tests, 0 failed, 0 errors.
spp_registry): field co-definitions on res.partner coexistcleanly (registry's uppercase name normalization applies; module needed no change).
Deferred (follow-up issue)
API v2 alignment (optional bridge module pattern — revisit after #114 lands), RFC 9457 error shape,
delete-endpoint authorization, token-endpoint rate limiting: see #434.
Notes for reviewers
readme/USAGE.mddocuments pointing the participant sync at an OpenSPP2 registry(
spp_dci_serversearch +spp_api_v2token endpoints); shipped defaults staylegacy-compatible.
CI-printed diff will be applied verbatim.