fix(suggestions): resolve seq-based identifiers on apply (#177)#180
Conversation
Tasks can be marked private: a private task is visible only to its assignee, its creator, and managers/admins. PUBLIC by default, so existing tasks and existing deployments are unchanged; an org opts into privacy-by-default via the task_default_private setting. - Migration (v0.7.1): tasks.private column + task_default_private settings-catalog row (renders as a boolean toggle in Admin -> Settings). - db.TaskViewer + visibilityClause: one predicate (NOT private OR assignee=me OR created_by=me), applied to the task read paths (ListTasks/PaginatedTasks/ ListTasksWhere). CanSeeAll (manager/admin) leaves queries unchanged. NO RLS. - Single by-id fetches enforce canViewTask in the handler -> 404 (not 403) so a private task's existence isn't revealed. Entity-reference title resolution hides a private task's title from non-viewers. - Token safety: visibility keys off the token USER's role, never a token flag, so a reader/contributor/agent token can't see others' private tasks; there is deliberately no 'see-private' token attribute (a manager/admin service user with a read-only token is the way to read all private tasks). - Create form gets a Public/Private control seeded from the org default (exposed via /config); the API create/update accept an optional private field. - Universal search indexes public tasks only (shared/cached index can't be filtered per viewer) so titles can't leak; owner-searchable private tasks are a tracked follow-up. Tests: db.TaskViewer.visibilityClause + api.canViewTask (public/manager/admin/ assignee/creator visible; unrelated reader/contributor incl. agents hidden).
…ctive/system/asset) (#177) applyChange/Objective/System/AssetUpdate resolved the target entity with parseEntityID (strip prefix -> int) and then GetX(thatInt). The identifier suffix is a per-org sequence, not the primary key, so on a shared/persistent stack an identifier-form suggestion (CR-n / SYSTEM-n / ASSET-n / objective display_id) silently hit the WRONG row — or 'not found'. Same class of bug #174 fixed for task/incident/corrective_action. - New resolveChangeID/resolveObjectiveID/resolveSystemID/resolveAssetID: numeric -> id; otherwise look it up (GetChangeRequestByIdentifier [new] / GetObjectiveByDisplayID / GetSystemByIdentifier / GetAssetByIdentifier). These intentionally skip a fixed-prefix check: asset ids exist as ASSET-/AST-, systems as SYSTEM-, and objective identifiers are program-key display IDs (e.g. ISMS-1). - The four apply handlers now resolve via those helpers instead of parseEntityID. - Regression test (Python/integration): create each entity, apply an update addressing it by its identifier form, assert the correct row changed. Note (separate, not fixed here): the stale-detection path (parseEntityID at the top of the apply/get flow) has the same mis-resolution for these types, but it only affects staleness accuracy, not which row is mutated.
unidoc-alip
left a comment
unidoc-alip
left a comment
There was a problem hiding this comment.
Closes #177 cleanly and follows the #174 precedent (resolveTaskID et al.). resolveChangeID/resolveObjectiveID/resolveSystemID/resolveAssetID correctly fall back to a per-org-unique lookup (identifier/display_id all carry UNIQUE(organization_id, ...) constraints) when the suggestion's EntityID isn't numeric, fixing the wrong-row-on-apply bug for change/objective/system/asset updates. The new Python integration test exercises all four types end-to-end.
Note: this branch is stacked on the unmerged feat/task-visibility branch (commit b0646f5), so the diff GitHub shows also includes that unrelated commit. That work has its own review; this review scopes only to 7bc8297, the actual #177 fix.
1. [nit] internal/isms/api/server.go line 3218
Unlike resolveTaskID/resolveIncidentID/etc. (which 400 via errInvalidID on a malformed, non-numeric, wrong-prefix param), these four helpers skip the prefix check entirely and always fall through to the DB lookup — so a garbage param and a well-formed-but-missing identifier both surface as the same "not found" error class. The PR description explains this is intentional (no fixed prefix works for all four: assets use both ASSET-/AST-, and objective identifiers are program-key-scoped). Worth a one-line code comment calling out that this is a deliberate 400-vs-404 tradeoff, not an oversight, since it's easy for a future reader to "fix" it back to the resolveTaskID shape and reintroduce a wrong assumption.
Not a correctness bug — org-scoped UNIQUE constraints on identifier/display_id mean there's no ambiguity risk, and every caller already treats the resolve error as "not found" regardless of cause.
2. [nit] internal/isms/api/api_suggestions.go line 542
parseEntityID is still used for stale-detection at lines 219/389/420/1671 (and for other entity types' apply paths, which are fine since they resolve by primary key). The PR body flags this as a known, deliberately out-of-scope follow-up for staleness-warning accuracy on change/objective/system/asset — no action needed here, just confirming the described scope matches the diff.
Follow-up to alip's #185 review: this PR made the SPA send raw identifiers to the GET-by-id endpoints, but several still only took a numeric id (or prefix-stripped), so off-page deep-links 400'd or served the wrong row. - handleGetAsset: wired the existing resolveAssetID. - handleGetRisk: new resolveRiskID (backed by GetRiskByIdentifier) + wired. - handleGetChange: wired resolveChangeID (int64 -> int cast for GetChangeRequest). - handleGetSystem: parseID prefix-strip -> resolveSystemID (DB lookup) — fixes the wrong-row-on-seq-drift class PR #180 fixed for suggestion-apply. - handleGetSupplier: new resolveSupplierID (backed by GetSupplierByIdentifier), same DB-lookup switch. Test: tests/test_deep_link_get_by_identifier.py (parametrized risk/asset/change/ system/supplier — GET by identifier resolves the same row as numeric). build-go / test-go green.
…) (#185) * feat(ui): identifier deep-links in URLs + copy-link button on all modules (#166) Detail views now use the entity's human identifier in the URL and expose a copy-link button, so a link to any item is shareable and stable. - New CopyLinkButton.vue: copies the current URL (secure-context clipboard with an execCommand fallback for plain-http self-hosted boxes, mirroring App.vue). Added to every module's detail header (12 views). - selectX now navigates to the identifier (…/risks/RISK-3, …/tasks/TASK-6; objectives use display_id, programs use key) instead of the numeric id. - openXFromRoute matches by identifier (in-memory) and passes the raw param to the identifier-capable GET endpoints (#174/#177) instead of parseInt, so identifier deep-links resolve for off-page items too; watcher guards updated to match. - handleGetObjective now routes through resolveObjectiveID, so GET /objectives/<display_id> resolves (was numeric-only) — closes the last off-page gap. Create-nav and Audit's /audit/:tab/:itemId keep numeric ids (Audit gets the copy-link button; its identifier routing is a small follow-up). Test: tests/test_objective_display_id_url.py (GET by display_id resolves the same objective as numeric). just build-web / build-go / test-go pass. * fix(api): resolve identifiers in register GET-by-id handlers (#166) Follow-up to alip's #185 review: this PR made the SPA send raw identifiers to the GET-by-id endpoints, but several still only took a numeric id (or prefix-stripped), so off-page deep-links 400'd or served the wrong row. - handleGetAsset: wired the existing resolveAssetID. - handleGetRisk: new resolveRiskID (backed by GetRiskByIdentifier) + wired. - handleGetChange: wired resolveChangeID (int64 -> int cast for GetChangeRequest). - handleGetSystem: parseID prefix-strip -> resolveSystemID (DB lookup) — fixes the wrong-row-on-seq-drift class PR #180 fixed for suggestion-apply. - handleGetSupplier: new resolveSupplierID (backed by GetSupplierByIdentifier), same DB-lookup switch. Test: tests/test_deep_link_get_by_identifier.py (parametrized risk/asset/change/ system/supplier — GET by identifier resolves the same row as numeric). build-go / test-go green.
2 participants
Closes #177
What
Suggestion-apply now resolves the target entity by lookup for change / objective / system / asset, so an update suggestion addressing an entity by its per-org identifier form lands on the right row.
Why
applyChange/Objective/System/AssetUpdatedidparseEntityID(sg.EntityID)(strip prefix → int) thenGetX(thatInt). The identifier suffix is a per-org sequence, not the primary key — on a shared/persistent stack they differ, so an identifier-form suggestion (CR-n,SYSTEM-n,ASSET-n, objectivedisplay_id) silently updated the wrong entity, or errored "not found". This is the same class of bug #174 fixed for task / incident / corrective_action.How
resolveChangeID/resolveObjectiveID/resolveSystemID/resolveAssetID: numeric → id; otherwise look up (GetChangeRequestByIdentifier[new] /GetObjectiveByDisplayID/GetSystemByIdentifier/GetAssetByIdentifier). They deliberately skip a fixed-prefix check — asset ids exist asASSET-/AST-, systems asSYSTEM-, and objective identifiers are program-key display IDs (e.g.ISMS-1), so a hardcoded prefix would be wrong.parseEntityID.Testing
tests/test_suggestion_apply_identifier.py: for each of the four types, create it, apply an update addressing it by its identifier form, assert the correct row changed (force=trueto bypass stale-detection).just fmt,just build-go,just test-gopass. The Python test runs against the stack (integration).Noted, not fixed here
The stale-detection path (
parseEntityIDat the top of the apply/get flow) has the same mis-resolution for these types, but it only affects staleness-warning accuracy, not which row is mutated — separate follow-up if we want it.