chore(memory): promote strong-fit authentication drafts from memory-pipeline for review

[Hareet]

Promotes 39 strong-fit authentication drafts from agent-memory/_pending/ into agent-memory/domains/authentication/issues/ for squad content review.

Categories: feature (15), bug (15), improvement (9)
Themes: SSO/OIDC login flows, token-login, permission checks & role revocation, password-reset/edit guards, multi-facility users.

All 39 carry domainFit: strong + a ## Domain Rationale section. 4 weak-fit drafts deferred for later.

• Base: seeding-claude-cli-v2 (feat(#108): seeding pipeline - CLI provider, domain-rationale, infrastructure domain, concurrency #119) — retarget to main after the schema lands.
• validate-schema: passing, 0 failures.
• Review for content (domain fit + distillation accuracy), not code.

[sugat009]

Requesting changes, though to be clear the fix is upstream and not something to patch by hand here. This seeder faithfully promotes what the distillation pipeline generated, so nothing is wrong with the promotion itself. The problem is in the drafts the pipeline produced, and I verified it against the live medic/cht-core API for every draft in this PR.

The defect: issueNumber / issueUrl name the merge PR, not the resolved issue

One precise note up front, because the intuitive version of this is not what is happening: issueNumber and issueUrl always agree with each other (the URL is literally /issues/<issueNumber>). The bug is that in 19 of 39 drafts here, that shared number is a pull request, so /issues/N silently redirects to /pull/N. The real resolved issue survives only in the PR-title slug. For example:

• 8675-feat6530-add-rate-limiting... stores issueNumber: 8675 / issueUrl: .../issues/8675, but 8675 is the merge PR ("feat(#6530): add rate limiting for authentication requests"). The real issue is #6530 (closed): "Add rate limiting to authentication endpoints".
• 10414-fix6784-safari-unsupported-browser... stores issueNumber: 10414 / issueUrl: .../issues/10414, but 10414 is the merge PR ("fix(#6784): safari unsupported browser message in login page"). The real issue is #6784 (closed): "Alert Safari users CHT doesn't support their browser".

This is the same scraper behaviour flagged on #121, and it is pipeline-wide: across the four clean seeders, 60 of 107 drafts are affected.

Duplicates in this PR. Because each draft is keyed by its PR, some resolved issues appear more than once:

• Issue #8868 is promoted twice (8924, 8933).
• Three issues are also promoted in the contacts seeder #132 (cross-domain duplicates): #9835, #9065, #6543.

Also. One draft (8843) comes from a PR titled feat(na): ... with no linked issue at all, so there is no real issue for it to reference.

Why request-changes rather than merge-and-fix-later

This corpus is the agent's memory of resolved issues (consumed by the Context Analysis Agent, see #135). A reference that resolves to a PR instead of the issue is simply wrong data, and it specifically defeats #135's planned de-duplication by issue id, since the "id" ends up being the PR id. Regenerating the drafts after the upstream fix will rewrite most of these files anyway, so merging now would churn the corpus twice.

Suggested fix (at the source, not file by file)

1. Fix the distiller/scraper to take the resolved issue from the type(#N): PR title (the filename slug already extracts it correctly), and keep the PR number in source_pr where it belongs.
2. Regenerate and re-promote this domain's drafts.
3. Add de-duplication by real issue id (also a #135 acceptance item).

Open to discussing the approach. Once the drafts carry the real issue references, this should be a quick re-review.

[Hareet]

Ready for review. @sugat009
Rebased onto main and addressed the review: identity keys relinked via #129's tool (19 files; all 39 PR→issue mappings re-verified against the cht-core API). The #8868 backport pair is collapsed with source_prs[]; 8843 (feat(na), closes no tracked issue) is dropped per the skip-and-flag policy. Cross-domain dupes resolved: #6543 is canonical here (the webapp display facet from the contacts seeder is folded in), while #9065/#9835/#9203 are canonically in the contacts corpus — their auth-side PR refs (#9205/#10222/#9204) are recorded there on #132. One correction from verification: 9955 was keyed correctly all along — its PR body explicitly closes #9735, the SSO epic.

[sugat009]

Content re-review. Identity/dedup/schema clean. This PR has the most distillation-fidelity issues — five drafts name permissions/handlers/fields that do not exist, clustered in the SSO/authorization work (all inline). An agent grepping the corpus would hit non-existent symbols, so these are the priority.

• nitpick (leakage): 9731, 10599, 10994, 9128 Domain Rationale scaffolding ("seed #2", "per the taxonomy", …); strip.
• nitpick: 10502 related_issues omits in-batch #6784; backfill.

Cross-cutting (whole promote batch, not blocking this PR alone): filenames still encode the PR number while frontmatter is issue-keyed, and two draft schemas coexist (machine-distilled vs hand-authored) — worth a cleanup pass. Full detail + re-runnable Verify commands are in my consolidated review notes.

[sugat009]

issue (accuracy, high): the draft uses the permission con_create_people (including in a tag), but the real permission is can_create_people (PR #8738 uses can_create_people/can_create_places throughout; grep con_create_people = 0). Replace everywhere.

[sugat009]

issue (accuracy): names a getOidc handler in login.js, but the handler is oidcLogin (plus oidcAuthorize); grep getOidc = 0 in api/src. Rename.

[sugat009]

issue (accuracy): says the guard inspects the user's oidc_provider field, but the user field is oidc (a truthy flag); oidc_provider is the app_settings provider config (holding client_id), not a user property. Correct the field reference.

[sugat009]

issue (accuracy): says a "checkbox toggle sets the user's oidc_provider", but the PR adds a text input (<input id="sso-login" type="text">) that sets oidc_username; oidc_provider is only the app_settings gate. Correct the control and field.

[sugat009]

issue (accuracy): says a chtRolesSettings param was added to hasPermissions()/hasAnyPermission(), but those became curried (ctx) => (permissions, userRoles) and read roles internally via ctx.settings.getAll() (filterRolesByConfigured). chtRolesSettings exists only on that internal helper, not on the public functions. Correct the API shape.

[Hareet]

PR #131 — reply to round 2 + request for re-review

All five inline items addressed. All five held when I re-derived them at each draft's own
anchor, so they went in as you described them. Both nitpicks too: the leakage was in eight
drafts, not four, and the gate now catches the phrasings that let it through.

Then, instead of stopping at the review, I ran this domain through everything the other five
domains converged on before merging: the full gate suite on frozen bytes, plus a per-draft audit
of all 34 against cht-core (anchor, master, and the commits between). It found a good deal the
review did not name. The worst of it is in Worst first below, after the gate ledger, which is
what says whether this round can be trusted.

---

Your five items

Draft	Was	Now	Verified
8738	con_create_people	can_create_people everywhere, incl. title and tags	git grep -Fw con_create_people 91c934920 → 0; can_create_people → actionbar.component.html:41,44,54,69, contacts-content.component.ts:280
9833	getOidc handler	oidcAuthorize (GET …/login/oidc/authorize) + oidcLogin (GET …/login/oidc)	routing.js:305-306, login.js:544/565 at bd9b23243. getOidc came from the #9765 issue body and never shipped
9901	user's oidc_provider	per-user truthy oidc flag; oidc_provider is the app_settings config holding client_id	token-login.js:286 if (user.oidc && config.get().oidc_provider?.client_id); settings.js:54
9900	checkbox toggle setting oidc_provider	text input #sso-login bound to editUserModel.oidc_username; oidc_provider is only the allowSSOLogin gate	edit_user.html:80, edit-user.js:72 at fd3bf4a40
10795	chtRolesSettings param on the public functions	hasPermissions/hasAnyPermission are curried on ctx and read ctx.settings.getAll(); chtRolesSettings is only filterRolesByConfigured's	cht-datasource/src/auth.js:49, :96, :132 at 058554e4d

One refinement to your 10795 wording: the inner function is
(permissions, userRoles, chtPermissionsSettings) — a third, optional, deprecated permissions
override survives (auth.js:94). The draft says so.

Nitpicks. Rubric leakage: 9731, 10599, 10994, 9128 as you listed, plus 8738, 9107, 9109 and
10795 ("Per the classification rules", "Per the roles/permissions rule", "per the rule that…").
Each Domain Rationale now argues from the code. verify-drafts flagged only one of the eight,
so its leakage patterns are widened, and a spec is built from these exact sentences. 10502
related_issues gains #6784, and 10414 gains #10494 so the pair is symmetric.

Gate ledger (frozen bytes; working tree == the pushed commit)

Tier	Result
validate-schema	98 / 0
verify-drafts, whole corpus (main + this branch, 201 drafts)	0 findings in authentication (the 6 blocking are main's known dedup backlog)
verify-drafts on this branch, with git history	0 blocking, 0 warnings, incl. stale-timestamp and uniform-domain-fit
verify-drafts --online	0 blocking, 0 warnings, 0 unverified
ground-claims --added-lines --base 112b4c7	609 / 609 grounded over the 819 lines this round added
ground-claims ×3	0 ungrounded, 0 unverifiable, 0 stale on each (1143 / 1124 / 1115 grounded); no degraded drafts
check-coherence ×3	0 contradictions on each, 34 / 34 drafts checked every pass

Passes that did not count: the baseline at 112b4c7 found 20 ungrounded + 5 stale-as-written.
Several passes were degraded and discarded rather than counted, because the reports said so:
the CLI was reinstalled mid-run (20 of 34 drafts unchecked once, one extraction another time)
and one run hit the Claude session limit. Everything the non-counting passes found is fixed, and
the counted run used a pinned copy of the CLI.

Worst first

Mechanism inverted or invented. No gate sees this class. All of it was found by reading the
issue and the diff:

1. 8776 said the PR added admin detection. It removed the #7410 admin-password path
   (updateAdminPassword, getPasswordHash, getCouchNodes deleted from shared-libs/settings).
   saveUserUpdates() now throws Admin passwords must be changed manually in the database.
   The issue's symptom was also inverted: the UI said "Password is not correct." for a change
   CouchDB had already applied.
2. 8738 had the polarity backwards. #8730 is users with only can_create_people being
   unable to create people, not over-exposure. The fix corrects existing checks: per type in
   the FAB, and widened to either permission in the old actionbar. It does not add "a separate
   check".
3. 9723 "clears browser history" → navigateToLogin() calls pushState(null, null, '/'),
   which adds an entry.
4. 9131 named the wrong redirect vector. A requested //host was already reduced to its
   path. The real vector is a URL whose path begins //. Checked by running the old and new
   logic.
5. 10414 called the Safari message informational. On Safari the PR hides the
   id="login-fields" wrapper.
6. 9107 named fields that are not protected. Only roles is compared, and only in
   medic-client; the medic ddoc got an isDbAdmin fix. It also called a deleted service
   (privacy-policies.js, D) "companion changes".
7. 10795, beyond your item: "user-management hasPermission()" and "api hasPermission()"
   came from the PR description. The PR deleted both. "Backwards compatible when roles are
   absent" is false: the spec pins "Restrictive".
8. 10004 described settings-level validation. There are two runtime throws, and the
   token-login one fires after the user docs are saved.
9. 9887: the guard runs after CouchDB validates credentials, not before. The unit case
   named 'should return 400 if is SSO User' asserted password-short at this anchor.
10. 9961: the email-claim check is the api's getIdToken. user-management has none.

Fabricated names, the class your review was about:

• 10827: getRtlLocales, data-rtl-locales (×3 templates) and rtlLocales exist nowhere
  in the tree. The draft is rewritten from the diff: rtl on each locale entry,
  data-rtl="true", <html dir="{{ defaultDir }}">, setDirection().
• 9877: buildAuthorizationUrl. The PR builds no URL, and openid-client was not yet a
  dependency at d60a08fdd.

Provenance. Eight drafts are epic children and said nothing about it:

• The seven SSO children (9800, 9833, 9877, 9887, 9900, 9901, 9961) merged into 9735_sso and
  reached master as #9955 (2cbe9c109).
• 10994 merged into 10224-ui-extensions and reached master as #11050 (180c29ecf).

Each draft now carries the Epic-child banner and the refs/pull fetch that makes its
source_sha resolvable. Merge state was checked via the API (merged: true, base.ref = the
epic branch). Five also carry a "Renamed/Superseded before landing" banner for what #9961
changed on the branch (oidc → oidc_username + oidc_login).

9955: its filename token (9760) contradicted its verified key (#9735), so the file is
renamed 9955-feat9735-….

Wrong file, wrong status, drift. The baseline's 20 ground-claims findings fall into two
groups:

• Symbols attributed to the wrong file: hasPermission, users_by_field,
  password_change_required, can_skip_password_change, loginByToken.
• Files described as added when the PR modified them, and vice versa: two package.jsons,
  libs/facility.js, users.spec.js.

It also found five stale-as-written paths:

• authorization.js, moved by #10823
• purge.wdio-spec.js (twice), removed by #11139
• add-user.wdio-spec.js, renamed by #10153
• a migration test removed by #10187

Beyond those, 8857's plugin registration was removed from master entirely by the PouchDB 9
upgrade (#9988), and 9126's "updated several e2e suites" was false: #9126 describe.skip'd
four of them. stale: true is now set on 17 drafts.

Linkage and classification.

• Linkage:
  • related_issues went from 9 drafts populated to 22 (+39 entries), symmetric within the
    domain.
  • PRs cited as issues are now labelled "PR #N".
  • Every gloss quotes the real title (several were forum-thread titles or invented).
• Categories: changed on five drafts, each to match its issue's Type label (9901, 8735, 10414,
  9961, 9422). Where the issue carries no category label (8857, 10994), the original category
  stands.
• domainFit: → weak on 9671, 10827 and 9422 (UI, i18n or error logging where
  authentication is the least-bad home). 10414 and 10502 stay strong: they decide whether a
  login can proceed in Safari. Fit lines agree.

My own repairs produced 39 delta-gate findings (20 ungrounded, 11 unverifiable, 8 stale)
before they converged. Most were paraphrased literals, CSS #id forms where the template
spells id="…", and create verbs attached to the wrong path. Three were gaps in the tools
themselves. Those are fixed on memory/draft-verification with tests, along with the leakage
patterns. A last sweep expanded ~110 bare or relative file names in the prose (users.js,
login.spec.js, libs/facility.js, test/auth.spec.js …) to full paths. A bare basename only
resolves at the repo root, so which of those sentences a sampled pass flagged was luck.

A regression pass over my own changes. The gates prove what the drafts now say; they cannot
see what a rewrite removed or quietly made wrong. So every sentence this round changed (427 of
the 776 you reviewed; 349 are untouched) went to independent reviewers who had not written the
changes, with the instruction to prove the old text right before accepting any correction. They
found 2 regressions, both in 9887: issue #9763 edits its own body with strikethrough
("get post", "oidc_provider oidc"), the rewrite read the rendered page, and a correct
sentence about the scope shift became a wrong one. Restored from the raw issue body. Also
2 losses (9955's reason the client secret stays out of the replicated settings doc; 9800's
note that the payload boolean is the one stored on the _users doc), both put back, and four
over- or mis-stated sentences my rewrite introduced (9900, 9128, 9437, 10414), corrected. Two
category changes (8857, 10994) and two domainFit downgrades (10414, 10502) had no issue label
or principled reason behind them and were reverted to what you reviewed.

Disclosed, not fixed

• 9126 keeps #9094/#9099 in source_prs: their master subjects are scoped feat(#6543), but
  their GitHub closing links point at medic/care-teams. PR #9093, a fourth feat(#6543), is
  absent from the corpus.
• 8857 source_prs += PR #9030 (chore(#8338), cited in its prose).
• 9955 stays stale: false. On master, #10004 replaced serverUtils.getAppUrl in the OIDC
  handlers; 9955 scopes that "at this squash" and cross-links #9983.
• 8735's old "confirmed reduced response times" credited a test with an observation whose only
  source is a review comment; the sentence is gone rather than reattributed.
• 8857 gains #9102, the PR's own tracking issue (Type: Technical issue); its identity stays on
  #8338, which the PR title names and PR #9030 also resolves to.
• Your cross-cutting note on filenames still encoding the PR number: agreed, and left to the
  batch-wide cleanup.

Still open outside this PR (landed corpus)

• configuration/11057 is on the same ui-extensions epic as 10994, but has no banner,
  stale: false, and empty related_issues despite linking #10224.
• tasks-and-targets/9232 lacks the reverse link to #6543.
• contacts/9203 credits #9128 with the facility_id string→Array change; #9126's API already
  wrote arrays.
• data-sync/10798 "cf. seed Inter-Agent Communication #4", which the widened leakage check now surfaces.

[sugat009]

Round 3 is a large improvement. I checked all 34 drafts sentence by sentence against the PR diffs, the code at each anchor, and current master (1,238 sentences; 1,192 verified true). An independent fact-check re-verified every finding. All five of my round-2 items are resolved, no draft regressed, and every batch-level claim in your comment holds.

Gates at 118a7bd: the merge with main is clean; build, lint and check-pending are green; validate-schema passes 201; 1,171 tests pass.

I request changes for one wrong mechanism and 14 smaller accuracy items. All 15 are inline, each with a suggestion:

• blocking: 8738 says the old actionbar's contacts-list pane lost create-person actions. That pane never listed person types.
• non-blocking (14): two more sentences in 8738; a wrong Node version in 8924; an invented remedy in 8735; an overstated gap in 9016; a false "never" in 9731; an incomplete 401 list in 9833; an omitted design rationale in 10502; dropped verification evidence in 10994; and missing stale notes or flags in 8857, 9126, 9128 (two) and 9955.

Two of 94 quoted issue titles are truncated (both in 9955). Two policy items are not for this PR (stale versus confidence in the schema, and stale-as-written as a banner label in five drafts); I will raise both separately.

Nits (48, not blocking)

• 10004:53 call-path compression: the api controllers called createUsers, createMultiFacilityUser and updateUser, not createUser directly.
• 10153:54 the widget condition is one async step too early (the widget runs in the second .then after ContactTypes.getAll()).
• 10153:70 Related Files omits admin/src/js/controllers/edit-user.js, the PR's only modified production file (it appears only under entities; same gap as the 8928 and 9887 nits).
• 10153:83 missing related issue #10155 (filed by the PR author while working this issue; still open — PR #10824 attempted it but closed unmerged).
• 10414:67 "six of the nine bundled locales" has drifted: master has a tenth locale (pt, #10555) and the key is in seven files.
• 10502:64 "runs first" overstates the execution order (locale setup precedes checkUnsupportedBrowser()).
• 10502:78 round 3 dropped two true facts from round 2 (the reviewer's manual Safari-spoofed check).
• 10599:87 the same spec also gained direct isDbAdmin/hasOnlineRole tests.
• 10599:88 half right: USER_ROLES.ONLINE already existed; #10747 added ADMIN and COUCHDB_ADMIN. Only one catch-all stub was replaced.
• 10795:71 subject and object swapped: the factory returns a promise that resolves to the data-context object. The second clause is vacuous for search.js.
• 10795:80 round 2's provenance (failing e2e tests surfaced the passivity requirement) was dropped.
• 10827:64 setDirection() is the second statement of baseTranslate(), after the locale guard.
• 10827:69 plural overstates: one of the two RTL rules targets an absolutely-positioned element.
• 8675:45 two open issues report defects in this exact mechanism (#10705 and the second rate-limiter issue) and are not linked.
• 8735:53 #8928 is a PR cited bare; your convention labels it "PR #8928" (issue #8877).
• 8735:73 round 3 removed every outcome statement; one neutral sentence on the measured effect would help.
• 8776:89 plural: one describe block with one it was removed.
• 8857:27 source_prs lists #9030, but Related Files and Testing cover only #8857's diff.
• 8857:68 imprecise: the root package.json already declared the package at the anchor.
• 8924:50 three real connections in the issue record are unrecorded (#6250 and two others).
• 8924:80 Related Files has no added/modified labels; four files were created.
• 8924:87 the test title was renamed on master by PR #9746 (optional note).
• 8928:84 api/tests/integration/migrations/utils.js was also modified and is not listed.
• 9016:51 imprecise: both GET /api/v1/users and /api/v2/users existed, and v2 already had the facility_id/contact_id filters.
• 9107:38 admin/src/js/services/privacy-policies.js is an entities path that this PR deleted.
• 9107:47 the issue body cites #8337 (open) and one more issue; neither is linked.
• 9109:41 the prose names PR #11050 (#10224) and PR #10187 (#9639); related_issues is empty.
• 9109:72 "creation cases" reach the ddoc with oldDoc === newDoc (the helpers default the argument).
• 9126:107 the same PR also skipped the whole POST/GET api/v2/users integration block (re-enabled by #9128).
• 9422:90 the e2e case drives the modal in its Add User state, not Edit User.
• 9671:35 api/src/public-error.js is an entities path that this PR deleted.
• 9723:52 the admin MainCtrl fetch override also calls navigateToLogin(); pushState discards forward entries.
• 9731:67 "lacks full access (a user changing their own password)" equates two different things (see the inline comment at :81).
• 9731:71 a _admin doc is never flagged, because hasAllPermissions() returns true for db admins first.
• 9800:65 there is no /api/v2 update route at the anchor (update exists for v1 and v3).
• 9800:81 the draft never says when or why oidc became oidc_username.
• 9877:57 stale-as-written is a tool term, not a cht-core term (also in 9800, 9833, 9887, 9901).
• 9887:92 Related Files lists only the two spec files; api/src/controllers/login.js appears only under entities.
• 9900:55 the sha does not resolve because the squash lives only on the epic branch, not because the PR number is absent from master's log.
• 9955:125 only the login integration spec and the SSO e2e spec use the mock provider.
• 9955:133 truncated title: #9763 ends with "for user".
• 9955:134 truncated title: #9764 ends with "for user." (with the period).
• 9955:152 "strong-fit domain" restates the Fit line; name the rejected alternative instead.
• 9961:60 stale: true rests only on a renamed test title; decide whether that counts.
• 9961:126 "#9938 referenced by this PR" understates: the PR body says "Closes #9938".

[sugat009]

issue (blocking): The contacts-list (left) pane of the old actionbar never listed a person type, so it could not lose a create-person action. At the anchor, contacts.component.ts filters children with children.filter(child => !child.person) in both branches (lines 283-298). The left action bar and the list-page FAB receive that filtered list (allowedChildPlaces, lines 437 and 473). The pane's label is "Add place". The same code is at 4.4.1 and on master. Only the contact-detail FAB lists person types: contacts-content.component.ts feeds it getChildren(selectedContact.type.id) unfiltered (line 318). The PR's own e2e spec drives contact-detail views only. The pre-PR gating hid the whole create container from users without can_create_places, Export link included. The suggestion rewrites the first two sentences and keeps the rest.

Suggested change

In the contacts-page FAB and in the create menu on the contacts-list (left) pane of the old actionbar, which at this PR non-admin users with `can_view_old_action_bar` saw (the old actionbar and that permission were removed on master by PR #9361), create-contact actions were gated on `can_create_places` (together with `can_edit`), whether the contact type being created was a person or a place. A user granted `can_create_people` but not `can_create_places` therefore lost those create-person actions: the issue reports users unable to enroll patients after upgrading to 4.4.1, with granting `can_create_places` as the workaround. Conversely, a user with `can_create_places` but not `can_create_people` was offered person creation in the FAB. The old actionbar's contact-detail (right-pane) create actions were not affected; they already took a per-type `can_create_people`/`can_create_places` permission from webapp/src/ts/modules/contacts/contacts-content.component.ts.

In the contacts FAB, `FastActionButtonService.getContactFormActions()` gated every child contact type on `can_create_places` (together with `can_edit`), whether the type was a person or a place. Only the contact-detail FAB lists person types (webapp/src/ts/modules/contacts/contacts-content.component.ts feeds it `getChildren(selectedContact.type.id)` unfiltered), so a user granted `can_create_people` but not `can_create_places` lost the create-person actions there: the issue reports users unable to enroll patients after upgrading to 4.4.1, with granting `can_create_places` as the workaround. The contacts-list (left) pane of the old actionbar, which at this PR non-admin users with `can_view_old_action_bar` saw (the old actionbar and that permission were removed on master by PR #9361), listed place types only (`getChildren()` in webapp/src/ts/modules/contacts/contacts.component.ts drops `person` types); its create links required `can_edit` and `can_create_places`, and the container that holds them and the Export link required `can_create_places`, so users without that permission also lost the Export link. Conversely, a user with `can_create_places` but not `can_create_people` was offered person creation in the FAB. The old actionbar's contact-detail (right-pane) create actions were not affected; they already took a per-type `can_create_people`/`can_create_places` permission from webapp/src/ts/modules/contacts/contacts-content.component.ts.

[sugat009]

issue (non-blocking): The summary repeats the same error: "could not create people from them". The contacts-list create menu offered place types only, so nobody could create people from it, before or after the PR. The suggestion keeps the FAB half and corrects the actionbar half.

Suggested change

summary: The contacts-page FAB and the old actionbar's contacts-list create menu gated contact creation only on `can_create_places`, so a user with `can_create_people` but not `can_create_places` could not create people from them. The fix makes the FAB require `can_create_people` for person contact types and `can_create_places` for place types, and lets the old actionbar's contacts-list create links show for either permission.

summary: The contacts FAB and the old actionbar's contacts-list create menu gated contact creation only on `can_create_places`, so a user with `can_create_people` but not `can_create_places` could not create people from the contact-detail FAB, and the old actionbar's contacts-list create container (place types only, Export link included) was hidden from them. The fix makes the FAB require `can_create_people` for person contact types and `can_create_places` for place types, and lets the old actionbar's contacts-list create links show for either permission.

[sugat009]

issue (non-blocking): Each code fact here is exact (I matched the pre-PR attributes at 91c9349^). But under Root Cause the paragraph reads as a cause of lost person creation. The pane had no person action to lose; the gating hid the whole container. The suggestion appends one sentence that says this.

Suggested change

In `FastActionButtonService.getContactFormActions()` (`webapp/src/ts/services/fast-action-button.service.ts`), each child contact type's `canDisplay` called `this.authService.has(['can_edit', 'can_create_places'])`, with no branch on the contact type. In the old actionbar's contacts-tab left pane (`webapp/src/ts/components/actionbar/actionbar.component.html`), the create container was gated by `[mmAuthAny]="[ actionBar?.left?.childPlaces && 'can_create_places' ]"` and each create link or menu by `mmAuth="can_edit,can_create_places"`; the template did not mention `can_create_people` at all.

In `FastActionButtonService.getContactFormActions()` (`webapp/src/ts/services/fast-action-button.service.ts`), each child contact type's `canDisplay` called `this.authService.has(['can_edit', 'can_create_places'])`, with no branch on the contact type. In the old actionbar's contacts-tab left pane (`webapp/src/ts/components/actionbar/actionbar.component.html`), the create container was gated by `[mmAuthAny]="[ actionBar?.left?.childPlaces && 'can_create_places' ]"` and each create link or menu by `mmAuth="can_edit,can_create_places"`; the template did not mention `can_create_people` at all. That pane listed place types only (`getChildren()` in webapp/src/ts/modules/contacts/contacts.component.ts filters out `person` types), so this gating hid no person action; it hid the whole container, Export link included, from users without `can_create_places`.

[sugat009]

issue (non-blocking): The api image never ran Node 19. api/Dockerfile has nodejs~=16 at 4.5.2 and nodejs~=20 at every 4.6.0 tag and at the anchor (PR #8824 for #7993, commit 900218e9b, 2024-02-01). The issue comments name the bump to Node 20 as the cause. Node 19 only introduced the keep-alive default. The mechanism itself is correct. The same version error is in the node-19 tag (line 27) and in the gloss at line 91. Please also rename the tag node-19 to node-20, and in the line 91 gloss write: "under the keep-alive default that Node 19 introduced (the api ran Node 20 from the 4.6.0 line)".

Suggested change

When authenticating a user, the API copied all headers from the original request onto a GET /_session request sent to CouchDB. If the original request was a POST carrying a content-length header, that header was forwarded onto the bodyless GET. Under Node 19 (which enables keep-alive by default), api's connection to HAProxy was reused, and HAProxy, treating the bodyless GET as unfinished, consumed content-length characters of the following request on that connection, producing an invalid request and a 400 status code. It was observed only when hitting API directly or via the AWS load balancer, never through nginx. Issue #8868 reported it after upgrading to 4.6.0-beta.2: users could not log in to the webapp and authenticated REST API calls failed with 400.

When authenticating a user, the API copied all headers from the original request onto a GET /_session request sent to CouchDB. If the original request was a POST carrying a content-length header, that header was forwarded onto the bodyless GET. Under Node 20 (the 4.6.0 line moved the api image from Node 16 to Node 20 in PR #8824 for #7993, and Node 19 had made outgoing HTTP keep-alive the default), api's connection to HAProxy was reused, and HAProxy, treating the bodyless GET as unfinished, consumed content-length characters of the following request on that connection, producing an invalid request and a 400 status code. It was observed only when hitting API directly or via the AWS load balancer, never through nginx. Issue #8868 reported it after upgrading to 4.6.0-beta.2: users could not log in to the webapp and authenticated REST API calls failed with 400.

[sugat009]

issue (non-blocking): "(which would require additional per-user scoping)" names a remedy that is in no record. The PR body only says the endpoint "will still perform badly with large numbers of users". The author's issue comments name streaming or pagination as the further work (2024-02-07). The fix also already scopes the facility fetch to the ids the returned users reference; the remaining cost is O(users), which more scoping cannot reduce.

Suggested change

	The author deliberately shipped a simple, high-impact partial fix: it removes the cost that scaled with the total number of contacts but acknowledges the endpoint will still degrade with very large numbers of users (which would require additional per-user scoping). Low-hanging-fruit optimization chosen over a full rewrite.
	The author deliberately shipped a simple, high-impact partial fix: it removes the cost that scaled with the total number of contacts but acknowledges the endpoint will still degrade with very large numbers of users (the issue thread names pagination or streaming as the further work large deployments would need). Low-hanging-fruit optimization chosen over a full rewrite.

[sugat009]

suggestion (non-blocking): True at the anchor, but the draft names request-promise-native as current, and api and sentinel no longer use it: api/src/db.js and sentinel/src/db.js switched to @medic/couch-request in PR #8833 (2402acdf9, 2024-04-23), five days after this PR. The stale notes cover only the plugin registrations and the two scripts.

Suggested change

Added the `pouchdb-session-authentication` package (`^1.1.0`) as a dependency in the existing api/package.json, sentinel/package.json and root package.json (plus their lockfiles) and registered it with `PouchDB.plugin(require('pouchdb-session-authentication'))`, immediately after `pouchdb-adapter-http`, in api/src/db.js and sentinel/src/db.js, in the test utilities tests/utils/index.js and api/tests/integration/migrations/utils.js, in scripts/get_users_meta_docs.js, scripts/conflicts/auto-resolve.js and scripts/conflicts/diff.js, and in tests/scalability/replicate-real-world-docs/add-docs-to-remote.js. No PouchDB constructor call changed: per the plugin's README it takes the credentials already present in the database URL (or an `auth` option), generates and stores a session cookie per user + CouchDB server pair, appends it as a `Cookie` header to outgoing requests, and regenerates the cookie and retries when it expires. Only PouchDB HTTP traffic is affected; requests the services make outside PouchDB (for example with `request-promise-native`) are unchanged.

Added the `pouchdb-session-authentication` package (`^1.1.0`) as a dependency in the existing api/package.json, sentinel/package.json and root package.json (plus their lockfiles) and registered it with `PouchDB.plugin(require('pouchdb-session-authentication'))`, immediately after `pouchdb-adapter-http`, in api/src/db.js and sentinel/src/db.js, in the test utilities tests/utils/index.js and api/tests/integration/migrations/utils.js, in scripts/get_users_meta_docs.js, scripts/conflicts/auto-resolve.js and scripts/conflicts/diff.js, and in tests/scalability/replicate-real-world-docs/add-docs-to-remote.js. No PouchDB constructor call changed: per the plugin's README it takes the credentials already present in the database URL (or an `auth` option), generates and stores a session cookie per user + CouchDB server pair, appends it as a `Cookie` header to outgoing requests, and regenerates the cookie and retries when it expires. Only PouchDB HTTP traffic is affected; requests the services made outside PouchDB at this PR's anchor (for example with `request-promise-native`) were not changed. (On master api and sentinel no longer use request-promise-native: api/src/db.js and sentinel/src/db.js switched to `@medic/couch-request` in PR #8833, `2402acdf9`, 2024-04-23.)

[sugat009]

suggestion (non-blocking): Historically true, but PR #9221 (74b43147e, 2024-07-12) removed tests/e2e/default/contacts/edit-person-home-place.wdio-spec.js from master; it moved the one test into tests/e2e/default/contacts/edit.wdio-spec.js. The parenthesis also stops before PR #9258 re-enabled all-permissions (2024-07-23), so a reader infers that block is still skipped. The draft's stale notes name only authorization.js and hasAllPermissions.

Suggested change

Updated the existing Mocha unit specs for the users controller, authorization service, user-management roles and users, and contacts places. Integration: replication (`should return all relevant ids with multiple facilities`, plus depth and sensitive-report cases for multiple facilities) and bulk-docs (`should filter offline user requests with multi facility`) cover multi-facility doc download/upload permissions; the users controller spec adds `POST api/v3/users` cases (create with multiple facilities, refusal without the permission, adding facilities on edit, malformed facilities); the sentinel create-user-for-contacts test now expects an array `facility_id`; tests/integration/api/controllers/login.spec.js only gains a random `X-Forwarded-For` header per request. In the wdio e2e suites, db-sync now syncs as a user with two facilities and replace-user expects an array `facility_id`, while edit-person-home-place, person-under-area, offline-user all-permissions and target-aggregates were set to `describe.skip` (PR #9099 re-enabled target-aggregates; PR #9128 re-enabled edit-person-home-place and person-under-area). Karma specs for contacts.effects (child places loaded for a multi-facility user, not for a single-facility user's own place) and contacts.component (single vs multi-facility homeplaces) cover the display branching; contacts-content.component.spec only switches its `getUserFacilityId` selector fixture to an array (PR #9094). The target-aggregates.service spec covers `isEnabled()` returning false for more than one facility, analytics-modules.service.spec stubs `isEnabled()`, and the target-aggregates e2e adds `should disable content when user has many facilities associated` (PR #9099).

Updated the existing Mocha unit specs for the users controller, authorization service, user-management roles and users, and contacts places. Integration: replication (`should return all relevant ids with multiple facilities`, plus depth and sensitive-report cases for multiple facilities) and bulk-docs (`should filter offline user requests with multi facility`) cover multi-facility doc download/upload permissions; the users controller spec adds `POST api/v3/users` cases (create with multiple facilities, refusal without the permission, adding facilities on edit, malformed facilities); the sentinel create-user-for-contacts test now expects an array `facility_id`; tests/integration/api/controllers/login.spec.js only gains a random `X-Forwarded-For` header per request. In the wdio e2e suites, db-sync now syncs as a user with two facilities and replace-user expects an array `facility_id`, while edit-person-home-place, person-under-area, offline-user all-permissions and target-aggregates were set to `describe.skip` (PR #9099 re-enabled target-aggregates; PR #9128 re-enabled edit-person-home-place and person-under-area; PR #9258 re-enabled all-permissions; PR #9221 later removed tests/e2e/default/contacts/edit-person-home-place.wdio-spec.js from master, moving its one test into tests/e2e/default/contacts/edit.wdio-spec.js as `should sync and update the offline user's home place`). Karma specs for contacts.effects (child places loaded for a multi-facility user, not for a single-facility user's own place) and contacts.component (single vs multi-facility homeplaces) cover the display branching; contacts-content.component.spec only switches its `getUserFacilityId` selector fixture to an array (PR #9094). The target-aggregates.service spec covers `isEnabled()` returning false for more than one facility, analytics-modules.service.spec stubs `isEnabled()`, and the target-aggregates e2e adds `should disable content when user has many facilities associated` (PR #9099).

[sugat009]

suggestion (non-blocking): stale: false, but the Testing section (line 94) names tests/e2e/default/users/add-user.wdio-spec.js, which PR #10153 renamed to tests/e2e/default/users/user.wdio-spec.js (2025-07-29). Both cited test names now live there (lines 80 and 204). In this same commit, 9422 (line 81) and 9900 (line 91) annotate this exact rename and set stale: true; your comment lists it among the stale-as-written paths. 9128 has neither the note nor the flag. Keep confidence: medium. My comment on line 94 carries the Testing wording.

Suggested change

	stale: false
	stale: true

[sugat009]

suggestion (non-blocking): Two files in this line drifted on master. add-user.wdio-spec.js was renamed by PR #10153 (see my comment on line 56). edit-person-home-place.wdio-spec.js was removed by PR #9221, which moved its only test into edit.wdio-spec.js under a new name; the same commit annotates removed specs in 8928 and 10795, and "deleted" alone would misstate the coverage. The suggestion applies both notes in one edit, so it also carries the rename wording from my line 56 comment.

Suggested change

Updated admin unit tests: admin/tests/unit/controllers/edit-user.spec.js adds `should allow only user with permission to have multiple places` and `user is updated with multiple places`, and admin/tests/unit/services/update-user.spec.js now expects CreateUser to post to `/api/v3/users`; the shared-libs contact-types-utils tests cover `isSameContactType`. In tests/integration/api/controllers/users.spec.js the `POST/GET api/v2/users` suite was re-enabled (it had been `describe.skip`); no cases were added. E2E: the new tests/e2e/default/contacts/delete-assigned-place.wdio-spec.js logs in as a user with two places and checks that the Delete menu option is disabled on one of them; tests/e2e/default/users/add-user.wdio-spec.js adds `should add user with multiple places with permission` and `should require user to have permission for multiple places` (checks the not-allowed message); edit-person-home-place and person-under-area were re-enabled from `describe.skip`; the users page object gained multiselect place input.

Updated admin unit tests: admin/tests/unit/controllers/edit-user.spec.js adds `should allow only user with permission to have multiple places` and `user is updated with multiple places`, and admin/tests/unit/services/update-user.spec.js now expects CreateUser to post to `/api/v3/users`; the shared-libs contact-types-utils tests cover `isSameContactType`. In tests/integration/api/controllers/users.spec.js the `POST/GET api/v2/users` suite was re-enabled (it had been `describe.skip`); no cases were added. E2E: the new tests/e2e/default/contacts/delete-assigned-place.wdio-spec.js logs in as a user with two places and checks that the Delete menu option is disabled on one of them; tests/e2e/default/users/add-user.wdio-spec.js (present at this PR's anchor; renamed on master to tests/e2e/default/users/user.wdio-spec.js by PR #10153, where both cases still exist) adds `should add user with multiple places with permission` and `should require user to have permission for multiple places` (checks the not-allowed message); tests/e2e/default/contacts/edit-person-home-place.wdio-spec.js (present at this PR's anchor; removed on master by PR #9221, which moved its only test into tests/e2e/default/contacts/edit.wdio-spec.js as `should sync and update the offline user's home place`) and tests/e2e/default/contacts/person-under-area.wdio-spec.js (still on master) were re-enabled from `describe.skip`; the users page object gained multiselect place input.

[sugat009]

suggestion (non-blocking): stale: false, but the draft names serverUtils.getAppUrl(req) (line 146) and lists api/src/server-utils.js (line 108), and that function was this squash's only change to the file. PR #10004 (1dfd7e60d, 2025-08-22) removed it and replaced its use in oidcLogin and oidcAuthorize with a module-local getAppUrl(). Your rule in this commit is a scoping note AND stale: true when named code no longer exists as described: 10599 (DB_ADMIN_ROLES relocated) and 9131 (resolveUrl body replaced) both do exactly that. The text at line 146 is accurate and can stay. Keep confidence: medium. Optional, in Related Files: - api/src/server-utils.js (its getAppUrl, this squash's only change to the file, was removed on master by PR #10004).

Suggested change

	stale: false
	stale: true

[Hareet]

PR #131 — reply to round 3 and request for re-review.

Thanks. All fifteen inline suggestions are applied verbatim, including the 8924 tag
(node-19 → node-20) and the line-91 gloss you wrote out in that comment. Before applying, I
re-checked each one at the draft's anchor and on master. Every one held, the blocking 8738 item
included: contacts.component.ts:284 filters !child.person in both branches at 91c9349^, at
4.4.1 and on master.

Nits applied where you gave the exact text: 8735:53 (PR #8928), 9955:133/134 (full #9763 and
#9764 titles), 9961:126 (Closes #9938), and the PR-modified file added to Related Files in 8928
(api/tests/integration/migrations/utils.js), 9887 (api/src/controllers/login.js) and 10153
(admin/src/js/controllers/edit-user.js). I'll take the remaining nits separately rather than
widen this round.

Three edits went beyond the suggestion text. Each one keeps a suggestion from leaving the draft
inconsistent:

• 9731:67. Applying your :81 suggestion made :67's "(a user changing their own password)"
  contradict it. Your nit 9731:67 flags exactly this. :67 now uses your own gloss from :81:
  "(can_edit + can_update_users, which is the webapp self-service case for ordinary users)".
• 8924:10. The summary had the same Node 19 framing as the old line 91. It now uses your
  line-91 wording.
• 8857:66. Your parenthetical says api and sentinel no longer use request-promise-native, then
  credits that to PR #8833. But the @medic/couch-request that #8833 introduced (2402acdf9)
  still required request-promise-native (couch-request.js:1 at that commit). It was dropped by PR
  #9746 (2c5a640c5, 2025-01-27), which rebuilt the library on fetch. I kept your text and
  appended: "that library wrapped request-promise-native until PR #9746, 2c5a640c5, 2025-01-27,
  rebuilt it on fetch."

lastUpdated is 2026-10-05 on the 16 touched drafts. Nothing else changed.

Gate ledger (touched drafts; frozen bytes)

Tier	Result
validate-schema	98 / 0
verify-drafts, whole corpus (current main + this branch, 201)	0 findings in authentication
verify-drafts, authentication with git history (34)	0 blocking, 0 warnings
verify-drafts --online (touched drafts)	0 blocking, 0 unverified
ground-claims --added-lines (round delta, base = 118a7bd)	31 / 31 grounded
ground-claims ×3 (16 touched drafts)	0 ungrounded / 0 unverifiable / 0 stale each pass (b10: 534, 537, 537 grounded; the 3 drafts re-edited after b10: 92, 100, 92); none degraded
check-coherence ×3 (16 touched drafts)	0 contradictions each pass; 16/16 (b10) and 3/3 (re-edited) checked every pass

Merge with main (ee82a46) is clean.

[sugat009]

I checked 0652261 against my round-3 review. All 15 suggestions are applied, 14 of them verbatim. Your 8857 addition is right: PR #9746 removed request-promise-native, not PR #8833, so my suggestion named the wrong PR. Thank you for the catch. The three consistency edits and the seven nits are correct. A trial merge with main is clean: validate-schema passes 201 and 1,171 tests pass.

Five small items are inline, each with a suggestion. Three of them correct my own round-3 wording (8738:52, 8738:10 and 9833:87). The other 41 round-3 nits stay open, as you said. Please track them in the follow-up.

I approve. Please apply the five suggestions before you merge.