feat(spp_attendance): migrate from openspp-modules - #435
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.82% +0.58%
==========================================
Files 419 433 +14
Lines 29813 30658 +845
==========================================
+ Hits 21539 22328 +789
- Misses 8274 8330 +56
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. |
…te; wizard ir.rule Address emjay0921's review: - /auth/token nosemgrep justification was false: nothing verifies a JWT upstream of the endpoint that mints it. State the real reason (secret checked against the stored scrypt hash). - The one-time credential wizard was readable by every attendance manager until the transient vacuum; an ir.rule now limits it to its creator (test first, red then green). - HISTORY: narrow the read-back claim to the actual plaintext lifetime; 'Initial migration to OpenSPP2' per repo convention.
|
Review addressed in 5dfc416. The # Unauthenticated token request by design: this line IS the authentication step.
# nosemgrep: odoo-sudo-without-context — secret verified against the stored scrypt hash, not read back(Two lines with the nosemgrep line kept directly above the match, since Semgrep only honors the immediately preceding line.) The other 13 were scanned as you suggested — all hold, with one nuance worth recording. Eleven sit directly inside route handlers that call Also picked up from the non-blocking set, since both are cheap and this PR introduced the surface:
Deferred, deliberately: never-persisting the plaintext (the show-once flow currently depends on the stored plaintext between form-create and first Show — needs the creation path itself to hand off through an action, which is a design change), the |
emjay0921
left a comment
emjay0921
left a comment
There was a problem hiding this comment.
Approved.
The suppression note is fixed, including the part that's easy to get wrong. The nosemgrep: line sits directly above the matching statement with the prose comment above it, so the suppression still binds — putting the explanation between the pragma and the code would have silently unsuppressed it. The new wording is accurate: that line is the authentication step, and the secret is checked against the stored hash rather than read back.
The call-graph note on lines 107/110 is the right thing to have written down, and it checks out — validate_attendance_type/validate_attendance_location are reached only from create_attendance_list (validates at line 182), update_attendance_list via get_time_card_vals (338), attendance_list_person (426) and attendance_list (496), each returning 401 before the sudo(). Recording that the helper's own claim is inherited from its callers is more useful than the pragma itself, since that's the assumption a future caller can break.
The wizard rule is correct and the test is the right test. Leaving groups off makes it global, which is what you want here — the restriction should hold for every user rather than one group, and Odoo's transient vacuum uses sudo() so cleanup still works. The test asserting both that manager B's search() comes back empty and that read() raises AccessError covers the two ways this leaks; a search-only assertion would have passed against a rule that only filtered lists.
HISTORY is honest now. "Until the dialog is used, the stored plaintext remains readable by attendance managers" is exactly the caveat the previous wording buried, and it's better to ship an accurate limitation than an aspirational claim.
The deferrals are reasonable — never-persisting the plaintext genuinely needs the creation path to hand off through an action, which is a design change and not this PR's job. #434 is the right home for the enumeration cap, the search/search_count pagination mismatch and expression.AND → Domain. Worth actually appending them to that issue rather than leaving them in this thread, since PR comments stop being findable once this merges.
One thing to sort out: the note says the gender_char/gender_id question is going "to Edwin for a decision" — that's you, so as written it has no owner. Given it's about where gender belongs in the registry data model rather than anything in this module, Jeremi looks like the right person, and it wants its own issue so it doesn't ride along with the API-alignment items in #434.
Nothing else blocking from me. Squash-merge whenever.
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.