feat: multi-site login via portal directory + provisioning-gated JWT minting#295
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImplements a multi-site login discovery architecture by introducing portal-service for dynamic endpoint resolution, enforcing per-site account provisioning in auth-service, updating shared infrastructure (error codes, OIDC claims, account validation, Gin middleware), rewiring the frontend to call portal lookup before auth/NATS connection, removing manual Site ID from login UI, and updating API docs, implementation plan, and deployment artifacts. ChangesPortal-service site discovery with enforced provisioning
Sequence DiagramsequenceDiagram
participant Browser
participant Frontend as Frontend<br/>(NatsContext)
participant Portal as Portal Service<br/>(/api/userInfo)
participant Auth as Auth Service<br/>(/auth)
participant NATS as NATS Server
Browser->>Frontend: Login complete, account known
Frontend->>Portal: GET /api/userInfo?account=alice
Portal-->>Frontend: {authServiceUrl, natsUrl, siteId}
Frontend->>Auth: POST /auth with token
alt Provisioned for site
Auth-->>Frontend: NATS JWT
Frontend->>NATS: Connect + WebSocket upgrade
NATS-->>Frontend: Connection established
else Not provisioned (403)
Auth-->>Frontend: Forbidden: account_not_provisioned
Frontend-->>Browser: Error message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
docs/superpowers/plans/2026-06-11-portal-service.md (1)
23-23: 💤 Low valueConsider adding an h2 heading before Task 1.
The document jumps from the h1 title directly to h3 task headings. Adding an h2 section header (e.g.,
## Implementation Tasks) before Task 1 would fix the heading hierarchy and improve document structure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-06-11-portal-service.md` at line 23, The document jumps from the top-level title to h3 task headings (e.g., "Task 1: `account_not_provisioned` reason in `pkg/errcode`"); insert an h2 heading (for example "## Implementation Tasks" or similar) immediately before the Task 1 heading to fix the heading hierarchy and improve structure, ensuring all subsequent task headings remain at h3.Source: Linters/SAST tools
docs/superpowers/specs/2026-06-11-portal-service-design.md (1)
54-72: 💤 Low valueConsider adding a language specifier to the fenced code block.
The ASCII art flow diagram could use
```textinstead of```to satisfy the markdownlint rule and make the intent explicit, though the block renders correctly either way.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/specs/2026-06-11-portal-service-design.md` around lines 54 - 72, The fenced ASCII-art flow diagram block (the block starting with "Browser portal-service site auth-service site NATS" and the following diagram) should include an explicit language specifier to satisfy markdownlint; replace the opening triple backticks (```) with triple backticks plus "text" (```text) so the block reads as a text code fence.Source: Linters/SAST tools
chat-frontend/src/api/_transport/asyncJob.test.js (1)
122-157: Useawait vi.advanceTimersByTimeAsync(...)in the async-timeout testsThese timeout failures are driven by a
setTimeoutthat resolves a Promise (used inPromise.race), so usingadvanceTimersByTimeAsyncmakes the test more deterministic by flushing the resulting promise/microtask chain before asserting the rejection/unsubscribe behavior inchat-frontend/src/api/_transport/asyncJob.test.js.🛠 Proposed fix
- vi.advanceTimersByTime(600) + await vi.advanceTimersByTimeAsync(600) @@ - vi.advanceTimersByTime(200) + await vi.advanceTimersByTimeAsync(200)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@chat-frontend/src/api/_transport/asyncJob.test.js` around lines 122 - 157, Replace synchronous timer advancement with the async variant in the two tests that rely on a setTimeout-based Promise resolution: in the "rejects with kind=async-timeout if no async result arrives in time" and "unsubscribes the response subscription on async timeout" tests, swap the calls to vi.advanceTimersByTime(600) and vi.advanceTimersByTime(200) to await vi.advanceTimersByTimeAsync(600) and await vi.advanceTimersByTimeAsync(200) respectively so the timer callback’s resulting promise/microtask chain is flushed before asserting the rejection and that nc.sub.unsubscribe was called (references: requestWithAsyncResult, ASYNC_JOB_ERROR_KINDS.AsyncTimeout, and unsubSpy).Source: Coding guidelines
auth-service/store_mongo.go (1)
23-30: Consider adding a compound index for the compound query predicate.The
AccountProvisionedquery (line 34) filters by bothaccountandsiteId, butEnsureIndexescreates only a single-field index onaccount. MongoDB will use the account index then scan matching documents forsiteIdin memory.For optimal performance, consider creating a compound index
{account: 1, siteId: 1}in addition to the single-field index. Multiple indexes can coexist: theaccountindex preserves compatibility with room-service (per the comment), while the compound index accelerates this service's lookup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@auth-service/store_mongo.go` around lines 23 - 30, EnsureIndexes currently creates only a single-field index on "account" which does not cover the AccountProvisioned query that filters by both account and siteId; update EnsureIndexes (in mongoProvisionStore) to also create a compound index {account: 1, siteId: 1} in addition to the existing account index so AccountProvisioned can use an index-backed lookup while preserving the single-field "account" index for room-service compatibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/client-api.md`:
- Around line 3694-3695: The catalog entry for `missing_fields` in
docs/client-api.md is inconsistent with the `portal-service POST /lookup`
request schema: update the `missing_fields` row to remove "account" from the
portal-service clause (leave only `ssoToken`) OR, if `account` is actually
supported, add `account` to the `POST /lookup` request schema and its example;
specifically edit the `missing_fields` table row and/or the `POST /lookup`
request body definition so the documented fields and the table entry
(`missing_fields`, portal-service `POST /lookup`) match exactly.
In `@portal-service/handler.go`:
- Around line 126-127: The dev-fallback branch creates a model.User without
EmployeeID which causes an empty string to be returned; update the fallback to
set a placeholder EmployeeID (e.g., "dev-employee" or similar) when constructing
the model.User in the case handling errors.Is(err, ErrUserNotFound) &&
devFallback so the response emits a non-empty employeeId; locate the branch in
handler.go where user = &model.User{Account: account, SiteID:
h.devFallbackSiteID} and add the EmployeeID field on that struct initialization
(or alternatively set user.EmployeeID immediately after creation) so frontend
consumers receive a meaningful value.
---
Nitpick comments:
In `@auth-service/store_mongo.go`:
- Around line 23-30: EnsureIndexes currently creates only a single-field index
on "account" which does not cover the AccountProvisioned query that filters by
both account and siteId; update EnsureIndexes (in mongoProvisionStore) to also
create a compound index {account: 1, siteId: 1} in addition to the existing
account index so AccountProvisioned can use an index-backed lookup while
preserving the single-field "account" index for room-service compatibility.
In `@chat-frontend/src/api/_transport/asyncJob.test.js`:
- Around line 122-157: Replace synchronous timer advancement with the async
variant in the two tests that rely on a setTimeout-based Promise resolution: in
the "rejects with kind=async-timeout if no async result arrives in time" and
"unsubscribes the response subscription on async timeout" tests, swap the calls
to vi.advanceTimersByTime(600) and vi.advanceTimersByTime(200) to await
vi.advanceTimersByTimeAsync(600) and await vi.advanceTimersByTimeAsync(200)
respectively so the timer callback’s resulting promise/microtask chain is
flushed before asserting the rejection and that nc.sub.unsubscribe was called
(references: requestWithAsyncResult, ASYNC_JOB_ERROR_KINDS.AsyncTimeout, and
unsubSpy).
In `@docs/superpowers/plans/2026-06-11-portal-service.md`:
- Line 23: The document jumps from the top-level title to h3 task headings
(e.g., "Task 1: `account_not_provisioned` reason in `pkg/errcode`"); insert an
h2 heading (for example "## Implementation Tasks" or similar) immediately before
the Task 1 heading to fix the heading hierarchy and improve structure, ensuring
all subsequent task headings remain at h3.
In `@docs/superpowers/specs/2026-06-11-portal-service-design.md`:
- Around line 54-72: The fenced ASCII-art flow diagram block (the block starting
with "Browser portal-service site auth-service
site NATS" and the following diagram) should include an explicit language
specifier to satisfy markdownlint; replace the opening triple backticks (```)
with triple backticks plus "text" (```text) so the block reads as a text code
fence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82cb6694-e94b-4ad3-bf00-ce23272d4c24
📒 Files selected for processing (50)
auth-service/deploy/docker-compose.ymlauth-service/handler.goauth-service/handler_test.goauth-service/integration_test.goauth-service/main.goauth-service/mock_store_test.goauth-service/store.goauth-service/store_mongo.gochat-frontend/CLAUDE.mdchat-frontend/deploy/30-render-config.shchat-frontend/deploy/config.js.templatechat-frontend/deploy/docker-compose.ymlchat-frontend/src/api/_transport/asyncJob.test.jschat-frontend/src/api/_transport/asyncJob.tschat-frontend/src/api/auth/oidcClient.jschat-frontend/src/context/NatsContext/NatsContext.jsxchat-frontend/src/context/NatsContext/NatsContext.test.jsxchat-frontend/src/context/NatsContext/useJwtRefresh.jschat-frontend/src/context/NatsContext/useJwtRefresh.test.jschat-frontend/src/lib/runtimeConfig.jschat-frontend/src/lib/runtimeConfig.test.jschat-frontend/src/pages/LoginPage/LoginPage.jsxchat-frontend/src/pages/LoginPage/LoginPage.test.jsxchat-frontend/src/pages/OidcCallback/OidcCallback.jsxchat-frontend/src/pages/OidcCallback/OidcCallback.test.jsxchat-frontend/vite.config.jsdocker-local/compose.services.yamldocker-local/setup.shdocs/client-api.mddocs/superpowers/plans/2026-06-11-portal-service.mddocs/superpowers/specs/2026-06-11-portal-service-design.mdpkg/errcode/codes_portal.gopkg/errcode/codes_test.gopkg/ginutil/middleware.gopkg/ginutil/middleware_test.gopkg/oidc/oidc.gopkg/oidc/oidc_test.gopkg/subject/subject.gopkg/subject/subject_test.goportal-service/deploy/Dockerfileportal-service/deploy/azure-pipelines.ymlportal-service/deploy/docker-compose.ymlportal-service/handler.goportal-service/handler_test.goportal-service/integration_test.goportal-service/main.goportal-service/mock_store_test.goportal-service/routes.goportal-service/store.goportal-service/store_mongo.go
💤 Files with no reviewable changes (3)
- chat-frontend/src/pages/OidcCallback/OidcCallback.jsx
- chat-frontend/vite.config.js
- chat-frontend/src/pages/OidcCallback/OidcCallback.test.jsx
vjauhari-work
force-pushed
the
claude/eager-einstein-7u2je6
branch
3 times, most recently
from
4f8c5e0 to
56f16d2
Compare
Collaborator
Author
|
@coderabbitai full-review |
|
✅ Action performedFull review finished. |
Collaborator
Author
|
@coderabbitai full-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/errcode/codes_test.go (1)
8-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the second new portal reason to the validation set.
Line 18 adds
PortalAccountNotProvisioned, butPortalAccountNotReady(frompkg/errcode/codes_portal.go) is missing fromallReasons, so snake_case/uniqueness checks won’t cover it.Suggested patch
var allReasons = []Reason{ @@ AuthTokenExpired, AuthInvalidToken, AuthInvalidRequest, AuthInvalidNKey, AuthMissingFields, PortalAccountNotProvisioned, + PortalAccountNotReady, RequestIDRequired, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/errcode/codes_test.go` around lines 8 - 20, The test's allReasons slice is missing the new PortalAccountNotReady Reason, so add PortalAccountNotReady alongside PortalAccountNotProvisioned in the allReasons declaration in pkg/errcode/codes_test.go to ensure the snake_case/uniqueness validation covers it; locate the allReasons variable and append PortalAccountNotReady to the list (the relevant symbols are PortalAccountNotProvisioned and PortalAccountNotReady).
♻️ Duplicate comments (1)
docs/client-api.md (1)
221-267:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument the dev-mode
accountlogin path too.The published contract is still out of sync with the shipped frontend flow:
chat-frontend/src/context/NatsContext/NatsContext.jsxposts{ account }toPOST /lookupand{ account, natsPublicKey }toPOST /authin dev mode, but §2.3 still documentsssoTokenas the only lookup input and §2.2 still reads likessoTokenis mandatory for auth. That makes the API reference inaccurate for the only reviewed dev login path and leaves the updatedmissing_fieldscatalog hard to reconcile.📝 Suggested doc alignment
#### Request body | Field | Type | Required | Notes | |---|---|---|---| -| `ssoToken` | string | yes | OIDC-issued SSO token. | +| `ssoToken` | string | yes* | OIDC-issued SSO token. Required for SSO login. | +| `account` | string | yes* | Dev-mode account lookup key. Required for dev login. | ```json -{ "ssoToken": "<sso-token>" } +{ "ssoToken": "<sso-token>" }
+
json +{ "account": "alice" } +@@
Status codereasonExample body 400 bad_requestmissing_fields{ "code": "bad_request", "reason": "missing_fields", "error": "ssoToken is required" }And in §2.2, document the same dev-mode alternative for `POST /auth` so the request schema matches the existing `missing_fields` catalog entry. </details> Also applies to: 3694-3696 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/client-api.mdaround lines 221 - 267, Update the API docs to include
the dev-mode login path: in section §2.3 (POST /lookup) add the alternative
request body example{ "account": "alice" }and note that dev-mode clients may
post account instead of ssoToken; in section §2.2 (POST /auth) add the matching
dev-mode schema that accepts{ "account", "natsPublicKey" }as an alternative
to the SSO flow; update themissing_fieldserror examples to show both
possible missing-field messages (e.g. "ssoToken is required" and "account is
required") so the documented error catalog matches the frontend behavior.</details> <!-- cr-comment:v1:2e5c3f5b19c032f97ba320c2 --> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (3)</summary><blockquote> <details> <summary>pkg/ginutil/middleware.go (1)</summary><blockquote> `1-2`: _💤 Low value_ **Consider renaming `ginutil` to a more descriptive package name.** The coding guidelines state: "Never name packages 'utils', 'helpers', 'common', or 'base' — use descriptive names that convey specific functionality." The name `ginutil` contains 'util'. A name like `ginmw` (Gin middleware) or `httputil` might better convey the package's purpose. That said, this is extracted from existing code and the current name is functional. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@pkg/ginutil/middleware.goaround lines 1 - 2, The package name ginutil
should be renamed to a more descriptive name (e.g., ginmw) to avoid "util" in
package names; update the package declaration from "package ginutil" to "package
ginmw", update all import paths that reference the ginutil package to use the
new name, rename any local references in code that import or refer to ginutil,
and run gofmt/goimports and go test to ensure builds/exports still work; search
for the symbol "ginutil" across the repository and replace usages with the new
package identifier (e.g., ginmw).</details> <!-- cr-comment:v1:d308dd1985fe47a5eef0fd20 --> _Source: Coding guidelines_ </blockquote></details> <details> <summary>auth-service/store_mongo.go (1)</summary><blockquote> `23-30`: _💤 Low value_ **Consider a compound index on `{account, siteId}` for the provisioning query.** The `AccountProvisioned` query filters on both `account` and `siteId`, but the index is only on `account`. A compound index would allow the query to be fully index-covered. However, the comment notes this mirrors room-service's spec for cross-service compatibility. If room-service already has the single-field index and both services share the collection, adding a compound index here would require coordination. This is a performance optimization to consider for a future pass. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@auth-service/store_mongo.goaround lines 23 - 30, Ensure the provisioning
query is index-covered by creating a compound index on account and siteId in
EnsureIndexes: update mongoProvisionStore.EnsureIndexes to add an IndexModel
with Keys bson.D{{Key: "account", Value: 1}, {Key: "siteId", Value: 1}} (in
addition to or replacing the existing single-field account index as decided),
handle and wrap any error like the existing pattern, and note that if the same
physical collection is shared with room-service you must coordinate changes
before replacing or removing the existing single-field index.</details> <!-- cr-comment:v1:999f2bb7931a48ada0ebe928 --> </blockquote></details> <details> <summary>portal-service/handler.go (1)</summary><blockquote> `21-24`: _⚡ Quick win_ **Keep the portal handler API unexported inside the service.** `TokenValidator`, `PortalHandler`, and `NewPortalHandler` are only used inside `package main`, so exporting them widens the service surface for no benefit. The single-method interface name also misses the repo’s `-er` suffix rule. Rename these to unexported service-internal identifiers before more call sites accumulate. As per coding guidelines, "Export only what other packages consume; keep handler/store implementations unexported within services" and "Interfaces must use '-er' suffix for single-method interfaces". Also applies to: 45-59 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@portal-service/handler.goaround lines 21 - 24, The exported identifiers
TokenValidator, PortalHandler, and NewPortalHandler should be made unexported
and the single-method interface should follow the '-er' suffix rule: rename
TokenValidator to tokenVerifier (unexported, -er suffix), rename PortalHandler
to portalHandler, and rename NewPortalHandler to newPortalHandler; update all
references/usages in this package (including any function signatures and
variable declarations that reference TokenValidator, PortalHandler, or
NewPortalHandler) so the types and constructors are unexported and the interface
name uses the '-er' suffix.</details> <!-- cr-comment:v1:ba9cb84773822984e46d3f50 --> _Source: Coding guidelines_ </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Inline comments:
In@chat-frontend/src/context/NatsContext/NatsContext.jsx:
- Line 68: The code stages credentials and sets authUrlRef.current before
completing the NATS WebSocket handshake, so if natsConnect() fails the refresh
loop keeps using staged JWTs; fix by deferring assignment to authUrlRef.current
until after a successful natsConnect(), wrap the connection attempt in a
try/catch around natsConnect() and on any error call stop() to halt background
refresh and rollback staged credentials (e.g., clear what setCredentials() set
or call setCredentials(null)/revert to previous), then rethrow the error; apply
the same change to the other dial site referencing
setCredentials()/natsConnect() (the block that currently sets authUrlRef.current
and invokes natsConnect()) so both places consistently commit authUrlRef.current
only after a successful dial.In
@chat-frontend/src/lib/runtimeConfig.js:
- Around line 6-7: Remove the silent fallback for PORTAL_URL so builds fail when
the value is missing: change the PORTAL_URL export in
chat-frontend/src/lib/runtimeConfig.js (the PORTAL_URL constant that currently
falls back to 'http://localhost:8081' via runtime.PORTAL_URL ||
import.meta.env.VITE_PORTAL_URL || 'http://localhost:8081') to require a value
(throw or return undefined/error when neither runtime.PORTAL_URL nor
import.meta.env.VITE_PORTAL_URL is present) and update
chat-frontend/deploy/30-render-config.sh to validate/exit non‑zero if the
rendered PORTAL_URL is empty before writing the config; keep the local-dev
default only in a single dev-specific location (not in runtimeConfig.js) so
NatsContext.jsx will never get a silent localhost value in deployed builds.In
@docker-local/setup.sh:
- Around line 111-115: The current block only writes VITE_PORTAL_URL when
FRONTEND_ENV_FILE doesn't exist, leaving older .env.local without the new
variable; update the logic around FRONTEND_ENV_FILE so that after creating the
file if missing you also check for VITE_PORTAL_URL and append
"VITE_PORTAL_URL=http://localhost:8081" if it is not present (use grep or
similar to detect presence) so existing files are backfilled; target the
FRONTEND_ENV_FILE handling and ensure the check/appending is idempotent.In
@docs/superpowers/plans/2026-06-11-portal-service.md:
- Line 1: The plan is out of sync with the spec amendments: update the plan
document to reflect the amended architecture (make the amendments the primary
narrative or add an explicit "Spec amendments override" section at the top),
change the data model references from Portal Mongo collections (users,
sites) to the HR-ownedhr_employeecache source and document the in-memory
cache behavior (load at startup + periodic refresh), replace store interface
descriptions with cache-only semantics (ListEmployees-style reads), update auth
URL derivation to reference PORTAL_AUTH_URL_TEMPLATE instead of an
authServiceUrl stored insites, change error strings/semantics to use
account_not_ready(403) withaccount_not_provisionedreserved for
auth-service, add GET /readyz to the endpoints list in addition to POST /lookup
and GET /healthz, and remove or mark any dev fallback that requires seeded
sitesrecords in favor of the synthesized fallback; ensure these specific
symbols (hr_employee, PORTAL_AUTH_URL_TEMPLATE, POST /lookup, GET /healthz, GET
/readyz, account_not_ready, account_not_provisioned) are updated throughout the
plan and that the plan's diagrams and store interface examples align with the
amended design.- Around line 1-14: The plan implements the original design instead of the
"Amendments (2026-06-12)" changes: update Task 5 to use an hr_employee in-memory
cache loaded at startup with periodic refresh (use ListEmployees semantics
instead of CRUD/EnsureIndexes), add config keys PORTAL_AUTH_URL_TEMPLATE and
PORTAL_CACHE_REFRESH_INTERVAL, change portal lookup miss error to
account_not_ready (reserve account_not_provisioned for auth-service mint gate),
add GET /readyz alongside /lookup and /healthz in Task 6, and change the dev
fallback behavior in Task 7 to synthesize a fallback site at runtime rather than
seeding Mongo; alternatively add an "Amendments override plan" section at the
top that clearly documents these deviations.In
@docs/superpowers/specs/2026-06-11-portal-service-design.md:
- Around line 7-30: The top "Amendments" summary must be promoted to the primary
narrative and the rest of the document updated to match the shipped design:
replace the "Data model" section with the HR-owned hr_employee cache
description, update the "Store interface" to only document ListEmployees (remove
FindUserByAccount, FindSiteByID, EnsureIndexes), add PORTAL_AUTH_URL_TEMPLATE
and PORTAL_CACHE_REFRESH_INTERVAL to "Config", add GET /readyz to "Endpoints"
and mark /healthz as liveness-only, change portal error docs to emit
account_not_ready (403) and note account_not_provisioned is only from the
auth-service, and move the original portal-owned-collections design into an
"Earlier approach" or "Design evolution" appendix so implementers see the
amended design first.In
@portal-service/cache.go:
- Around line 49-54: The refresh currently always calls c.replace(emps) even
when store.ListEmployees(ctx) returns an empty slice, which wipes a previously
populated cache and causes Ready()/HandleLookup to fail; change the refresh
logic (the block using store.ListEmployees(ctx), emps, err, and c.replace(emps))
to treat a post-bootstrap empty result as a failed refresh: if emps is non-nil
but len(emps)==0 and the existing cache already has entries (use whatever
accessor like c.count() or check current snapshot), do not call c.replace(emps)
and instead return an error (or log and skip replacement) so the previous
snapshot continues to be served. Ensure the code still replaces the cache when
it was previously empty (bootstrap) or when the new slice is non-empty.- Around line 58-66: The replace function currently silently overwrites
duplicate employee Account keys; change it to detect duplicates before swapping
the snapshot and abort the refresh if any duplicate Account is found. In
directoryCache.replace, first scan emps and build entries while checking if
entries[e.Account] already exists; if a duplicate is detected return an error
(or bool) and do not acquire c.mu or modify c.entries/c.loaded so the previous
snapshot is preserved; otherwise, under c.mu.Lock(), assign c.entries = entries
and c.loaded = true and unlock. Update the function signature and callers
accordingly to propagate the failure instead of publishing a nondeterministic
directory.In
@portal-service/deploy/azure-pipelines.yml:
- Around line 41-45: The pipeline runs tests and writes coverage files
(coverage-pkg.out and coverage-$(SERVICE_NAME).out) but never enforces the 80%
threshold; add a post-test step that runs "go tool cover -func=coverage-*.out"
(or equivalent) for each coverage file, parses the "total:" line to extract the
percentage, and exits with non-zero status if any package total is below 80%;
update the pipeline after the two test script steps (the ones running "go test
./pkg/... -coverprofile=coverage-pkg.out" and "go test ./$(SERVICE_NAME)/...
-coverprofile=coverage-$(SERVICE_NAME).out") to include this check so CI fails
when coverage < 80%.
Outside diff comments:
In@pkg/errcode/codes_test.go:
- Around line 8-20: The test's allReasons slice is missing the new
PortalAccountNotReady Reason, so add PortalAccountNotReady alongside
PortalAccountNotProvisioned in the allReasons declaration in
pkg/errcode/codes_test.go to ensure the snake_case/uniqueness validation covers
it; locate the allReasons variable and append PortalAccountNotReady to the list
(the relevant symbols are PortalAccountNotProvisioned and
PortalAccountNotReady).
Duplicate comments:
In@docs/client-api.md:
- Around line 221-267: Update the API docs to include the dev-mode login path:
in section §2.3 (POST /lookup) add the alternative request body example{ "account": "alice" }and note that dev-mode clients may post account instead of
ssoToken; in section §2.2 (POST /auth) add the matching dev-mode schema that
accepts{ "account", "natsPublicKey" }as an alternative to the SSO flow;
update themissing_fieldserror examples to show both possible missing-field
messages (e.g. "ssoToken is required" and "account is required") so the
documented error catalog matches the frontend behavior.
Nitpick comments:
In@auth-service/store_mongo.go:
- Around line 23-30: Ensure the provisioning query is index-covered by creating
a compound index on account and siteId in EnsureIndexes: update
mongoProvisionStore.EnsureIndexes to add an IndexModel with Keys bson.D{{Key:
"account", Value: 1}, {Key: "siteId", Value: 1}} (in addition to or replacing
the existing single-field account index as decided), handle and wrap any error
like the existing pattern, and note that if the same physical collection is
shared with room-service you must coordinate changes before replacing or
removing the existing single-field index.In
@pkg/ginutil/middleware.go:
- Around line 1-2: The package name ginutil should be renamed to a more
descriptive name (e.g., ginmw) to avoid "util" in package names; update the
package declaration from "package ginutil" to "package ginmw", update all import
paths that reference the ginutil package to use the new name, rename any local
references in code that import or refer to ginutil, and run gofmt/goimports and
go test to ensure builds/exports still work; search for the symbol "ginutil"
across the repository and replace usages with the new package identifier (e.g.,
ginmw).In
@portal-service/handler.go:
- Around line 21-24: The exported identifiers TokenValidator, PortalHandler, and
NewPortalHandler should be made unexported and the single-method interface
should follow the '-er' suffix rule: rename TokenValidator to tokenVerifier
(unexported, -er suffix), rename PortalHandler to portalHandler, and rename
NewPortalHandler to newPortalHandler; update all references/usages in this
package (including any function signatures and variable declarations that
reference TokenValidator, PortalHandler, or NewPortalHandler) so the types and
constructors are unexported and the interface name uses the '-er' suffix.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `31ad5c37-81a9-4154-a6b5-9df09126e9ea` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 22587086e4d68328d25810c3dea294ec53be24a3 and 56f16d29f7f7b78800c9b04aa694aaa7609c3cf3. </details> <details> <summary>📒 Files selected for processing (52)</summary> * `auth-service/deploy/docker-compose.yml` * `auth-service/handler.go` * `auth-service/handler_test.go` * `auth-service/integration_test.go` * `auth-service/main.go` * `auth-service/mock_store_test.go` * `auth-service/store.go` * `auth-service/store_mongo.go` * `chat-frontend/CLAUDE.md` * `chat-frontend/deploy/30-render-config.sh` * `chat-frontend/deploy/config.js.template` * `chat-frontend/deploy/docker-compose.yml` * `chat-frontend/src/api/_transport/asyncJob.test.js` * `chat-frontend/src/api/_transport/asyncJob.ts` * `chat-frontend/src/api/auth/oidcClient.js` * `chat-frontend/src/context/NatsContext/NatsContext.jsx` * `chat-frontend/src/context/NatsContext/NatsContext.test.jsx` * `chat-frontend/src/context/NatsContext/useJwtRefresh.js` * `chat-frontend/src/context/NatsContext/useJwtRefresh.test.js` * `chat-frontend/src/lib/runtimeConfig.js` * `chat-frontend/src/lib/runtimeConfig.test.js` * `chat-frontend/src/pages/LoginPage/LoginPage.jsx` * `chat-frontend/src/pages/LoginPage/LoginPage.test.jsx` * `chat-frontend/src/pages/OidcCallback/OidcCallback.jsx` * `chat-frontend/src/pages/OidcCallback/OidcCallback.test.jsx` * `chat-frontend/vite.config.js` * `docker-local/compose.services.yaml` * `docker-local/setup.sh` * `docs/client-api.md` * `docs/superpowers/plans/2026-06-11-portal-service.md` * `docs/superpowers/specs/2026-06-11-portal-service-design.md` * `pkg/errcode/codes_portal.go` * `pkg/errcode/codes_test.go` * `pkg/ginutil/middleware.go` * `pkg/ginutil/middleware_test.go` * `pkg/oidc/oidc.go` * `pkg/oidc/oidc_test.go` * `pkg/subject/subject.go` * `pkg/subject/subject_test.go` * `portal-service/cache.go` * `portal-service/cache_test.go` * `portal-service/deploy/Dockerfile` * `portal-service/deploy/azure-pipelines.yml` * `portal-service/deploy/docker-compose.yml` * `portal-service/handler.go` * `portal-service/handler_test.go` * `portal-service/integration_test.go` * `portal-service/main.go` * `portal-service/mock_store_test.go` * `portal-service/routes.go` * `portal-service/store.go` * `portal-service/store_mongo.go` </details> <details> <summary>💤 Files with no reviewable changes (3)</summary> * chat-frontend/vite.config.js * chat-frontend/src/pages/OidcCallback/OidcCallback.test.jsx * chat-frontend/src/pages/OidcCallback/OidcCallback.jsx </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
vjauhari-work
force-pushed
the
claude/eager-einstein-7u2je6
branch
2 times, most recently
from
14a725f to
18ad056
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
portal-service/store_mongo.go (1)
16-18: ⚡ Quick winRename constructor to follow the
New<Type>pattern.Line 16 uses
newMongoDirectoryStore, which conflicts with the constructor naming rule. Please rename it toNewMongoDirectoryStore(and update call sites) for guideline consistency.As per coding guidelines, constructors must follow the
New<Type>pattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@portal-service/store_mongo.go` around lines 16 - 18, The constructor function newMongoDirectoryStore must be renamed to follow the New<Type> pattern; rename the function to NewMongoDirectoryStore and update all call sites to use NewMongoDirectoryStore, leaving the returned type mongoDirectoryStore and its field employees unchanged so signatures remain compatible; ensure exported name capitalization is updated wherever newMongoDirectoryStore was referenced (tests, other packages, and any factory usages).Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-06-11-portal-service.md`:
- Line 3: The plan's Task 2 and associated code snippets instruct
Claims.Account() to fall back to name, which reintroduces unsafe account
derivation; update every occurrence in the plan and snippets where
Claims.Account() or similar account-derivation logic uses a `name` fallback
(including the examples in Task 2 and the snippets around the Claims.Account()
usage) to remove the `name` fallback and use only `preferred_username` (or a
single trusted claim) for account identity, and ensure any explanatory text and
checkboxes referring to the fallback are revised to state that only
preferred_username is trusted.
---
Nitpick comments:
In `@portal-service/store_mongo.go`:
- Around line 16-18: The constructor function newMongoDirectoryStore must be
renamed to follow the New<Type> pattern; rename the function to
NewMongoDirectoryStore and update all call sites to use NewMongoDirectoryStore,
leaving the returned type mongoDirectoryStore and its field employees unchanged
so signatures remain compatible; ensure exported name capitalization is updated
wherever newMongoDirectoryStore was referenced (tests, other packages, and any
factory usages).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0f00c965-4687-4675-a8be-738df861ec1c
📒 Files selected for processing (52)
auth-service/deploy/docker-compose.ymlauth-service/handler.goauth-service/handler_test.goauth-service/integration_test.goauth-service/main.goauth-service/mock_store_test.goauth-service/store.goauth-service/store_mongo.gochat-frontend/CLAUDE.mdchat-frontend/deploy/30-render-config.shchat-frontend/deploy/config.js.templatechat-frontend/deploy/docker-compose.ymlchat-frontend/src/api/_transport/asyncJob.test.jschat-frontend/src/api/_transport/asyncJob.tschat-frontend/src/api/auth/oidcClient.jschat-frontend/src/context/NatsContext/NatsContext.jsxchat-frontend/src/context/NatsContext/NatsContext.test.jsxchat-frontend/src/context/NatsContext/useJwtRefresh.jschat-frontend/src/context/NatsContext/useJwtRefresh.test.jschat-frontend/src/lib/runtimeConfig.jschat-frontend/src/lib/runtimeConfig.test.jschat-frontend/src/pages/LoginPage/LoginPage.jsxchat-frontend/src/pages/LoginPage/LoginPage.test.jsxchat-frontend/src/pages/OidcCallback/OidcCallback.jsxchat-frontend/src/pages/OidcCallback/OidcCallback.test.jsxchat-frontend/vite.config.jsdocker-local/compose.services.yamldocker-local/setup.shdocs/client-api.mddocs/superpowers/plans/2026-06-11-portal-service.mddocs/superpowers/specs/2026-06-11-portal-service-design.mdpkg/errcode/codes_portal.gopkg/errcode/codes_test.gopkg/ginutil/middleware.gopkg/ginutil/middleware_test.gopkg/oidc/oidc.gopkg/oidc/oidc_test.gopkg/subject/subject.gopkg/subject/subject_test.goportal-service/cache.goportal-service/cache_test.goportal-service/deploy/Dockerfileportal-service/deploy/azure-pipelines.ymlportal-service/deploy/docker-compose.ymlportal-service/handler.goportal-service/handler_test.goportal-service/integration_test.goportal-service/main.goportal-service/mock_store_test.goportal-service/routes.goportal-service/store.goportal-service/store_mongo.go
💤 Files with no reviewable changes (3)
- chat-frontend/src/pages/OidcCallback/OidcCallback.jsx
- chat-frontend/src/pages/OidcCallback/OidcCallback.test.jsx
- chat-frontend/vite.config.js
✅ Files skipped from review due to trivial changes (4)
- docker-local/compose.services.yaml
- auth-service/mock_store_test.go
- portal-service/mock_store_test.go
- docs/superpowers/specs/2026-06-11-portal-service-design.md
🚧 Files skipped from review as they are similar to previous changes (37)
- pkg/errcode/codes_portal.go
- pkg/oidc/oidc_test.go
- pkg/oidc/oidc.go
- chat-frontend/src/context/NatsContext/useJwtRefresh.test.js
- docker-local/setup.sh
- chat-frontend/src/api/_transport/asyncJob.ts
- chat-frontend/src/lib/runtimeConfig.test.js
- portal-service/routes.go
- pkg/subject/subject.go
- chat-frontend/deploy/config.js.template
- pkg/subject/subject_test.go
- chat-frontend/src/context/NatsContext/useJwtRefresh.js
- chat-frontend/src/api/_transport/asyncJob.test.js
- portal-service/deploy/azure-pipelines.yml
- chat-frontend/CLAUDE.md
- chat-frontend/src/api/auth/oidcClient.js
- auth-service/store.go
- docs/client-api.md
- portal-service/deploy/Dockerfile
- pkg/ginutil/middleware.go
- auth-service/main.go
- auth-service/store_mongo.go
- portal-service/store.go
- chat-frontend/deploy/docker-compose.yml
- chat-frontend/src/context/NatsContext/NatsContext.jsx
- auth-service/handler.go
- portal-service/cache.go
- chat-frontend/src/pages/LoginPage/LoginPage.test.jsx
- auth-service/integration_test.go
- auth-service/handler_test.go
- portal-service/main.go
- portal-service/deploy/docker-compose.yml
- pkg/ginutil/middleware_test.go
- portal-service/handler.go
- chat-frontend/src/context/NatsContext/NatsContext.test.jsx
- chat-frontend/src/pages/LoginPage/LoginPage.jsx
- portal-service/handler_test.go
vjauhari-work
force-pushed
the
claude/eager-einstein-7u2je6
branch
from
18ad056 to
4d89321
Compare
vjauhari-work
force-pushed
the
claude/eager-einstein-7u2je6
branch
from
4d89321 to
7526465
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/client-api.md`:
- Around line 42-43: Update all stale references from the deprecated POST
/lookup endpoint to the current GET /api/userInfo endpoint throughout the
documentation to maintain consistency and fix broken fragment links. In
docs/client-api.md, update the TOC entry at line 42 to reference GET
/api/userInfo instead of POST /lookup (portal-service), update the error
descriptions at lines 3694-3696 to reference GET /api/userInfo instead of POST
/lookup, and update the error description at line 3702 similarly to reference
GET /api/userInfo instead of POST /lookup. This ensures all references align
with Section 2.3 which already describes the correct GET /api/userInfo endpoint.
In `@portal-service/handler.go`:
- Around line 59-72: The `HandleUserInfo` method in `PortalHandler` currently
extracts the `account` directly from the HTTP query parameter without validating
an SSO token, creating an unauthenticated account-enumeration vulnerability. Add
SSO token validation to the method (either directly or by ensuring mandatory
auth middleware protects this endpoint) and derive the `account` from trusted
token claims (such as the preferred_username claim) instead of accepting it
directly from the query parameter. Reserve the raw query-parameter-based account
lookup only for explicitly designated dev-mode code paths with appropriate
safeguards.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 50248ed0-09d5-45f3-a25a-74d23d9f9c18
📒 Files selected for processing (52)
auth-service/deploy/docker-compose.ymlauth-service/handler.goauth-service/handler_test.goauth-service/integration_test.goauth-service/main.goauth-service/mock_store_test.goauth-service/store.goauth-service/store_mongo.gochat-frontend/CLAUDE.mdchat-frontend/deploy/30-render-config.shchat-frontend/deploy/config.js.templatechat-frontend/deploy/docker-compose.ymlchat-frontend/src/api/_transport/asyncJob.test.jschat-frontend/src/api/_transport/asyncJob.tschat-frontend/src/api/auth/oidcClient.jschat-frontend/src/context/NatsContext/NatsContext.jsxchat-frontend/src/context/NatsContext/NatsContext.test.jsxchat-frontend/src/context/NatsContext/useJwtRefresh.jschat-frontend/src/context/NatsContext/useJwtRefresh.test.jschat-frontend/src/lib/runtimeConfig.jschat-frontend/src/lib/runtimeConfig.test.jschat-frontend/src/pages/LoginPage/LoginPage.jsxchat-frontend/src/pages/LoginPage/LoginPage.test.jsxchat-frontend/src/pages/OidcCallback/OidcCallback.jsxchat-frontend/src/pages/OidcCallback/OidcCallback.test.jsxchat-frontend/vite.config.jsdocker-local/compose.services.yamldocker-local/setup.shdocs/client-api.mddocs/superpowers/plans/2026-06-11-portal-service.mddocs/superpowers/specs/2026-06-11-portal-service-design.mdpkg/errcode/codes_portal.gopkg/errcode/codes_test.gopkg/ginutil/middleware.gopkg/ginutil/middleware_test.gopkg/oidc/oidc.gopkg/oidc/oidc_test.gopkg/subject/subject.gopkg/subject/subject_test.goportal-service/cache.goportal-service/cache_test.goportal-service/deploy/Dockerfileportal-service/deploy/azure-pipelines.ymlportal-service/deploy/docker-compose.ymlportal-service/handler.goportal-service/handler_test.goportal-service/integration_test.goportal-service/main.goportal-service/mock_store_test.goportal-service/routes.goportal-service/store.goportal-service/store_mongo.go
💤 Files with no reviewable changes (1)
- chat-frontend/vite.config.js
✅ Files skipped from review due to trivial changes (4)
- chat-frontend/src/api/_transport/asyncJob.ts
- chat-frontend/CLAUDE.md
- portal-service/mock_store_test.go
- docs/superpowers/specs/2026-06-11-portal-service-design.md
🚧 Files skipped from review as they are similar to previous changes (36)
- auth-service/deploy/docker-compose.yml
- pkg/errcode/codes_test.go
- pkg/oidc/oidc_test.go
- chat-frontend/src/pages/OidcCallback/OidcCallback.jsx
- chat-frontend/src/lib/runtimeConfig.test.js
- chat-frontend/deploy/docker-compose.yml
- chat-frontend/src/api/_transport/asyncJob.test.js
- docker-local/compose.services.yaml
- auth-service/mock_store_test.go
- auth-service/store.go
- docker-local/setup.sh
- pkg/oidc/oidc.go
- pkg/errcode/codes_portal.go
- chat-frontend/src/pages/LoginPage/LoginPage.jsx
- chat-frontend/deploy/config.js.template
- portal-service/deploy/Dockerfile
- chat-frontend/src/api/auth/oidcClient.js
- chat-frontend/src/context/NatsContext/NatsContext.jsx
- pkg/ginutil/middleware.go
- portal-service/deploy/azure-pipelines.yml
- chat-frontend/src/context/NatsContext/useJwtRefresh.test.js
- chat-frontend/src/pages/LoginPage/LoginPage.test.jsx
- auth-service/integration_test.go
- auth-service/main.go
- chat-frontend/src/context/NatsContext/NatsContext.test.jsx
- auth-service/store_mongo.go
- portal-service/integration_test.go
- pkg/subject/subject_test.go
- portal-service/cache.go
- portal-service/store_mongo.go
- auth-service/handler.go
- pkg/subject/subject.go
- portal-service/store.go
- portal-service/cache_test.go
- auth-service/handler_test.go
- pkg/ginutil/middleware_test.go
vjauhari-work
force-pushed
the
claude/eager-einstein-7u2je6
branch
from
7526465 to
396fd1b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
chat-frontend/src/context/NatsContext/NatsContext.jsx (1)
53-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard async connection writes with a generation check.
The connect lifecycle and
nc.closed().then(...)callback can commit stale state after a newer connect attempt, which can incorrectly flipconnected/errorfor the active session.As per coding guidelines, long-lived async callbacks in `chat-frontend/src/context/**/*.{js,jsx,ts,tsx}` must guard writes with a generation check to prevent stale-cycle state updates on reconnects.Suggested fix
export function NatsProvider({ children }) { const ncRef = useRef(null) + const connectGenRef = useRef(0) @@ const connectToNats = useCallback(async (opts) => { + const gen = ++connectGenRef.current + const isStale = () => gen !== connectGenRef.current setError(null) @@ const nc = await natsConnect({ servers: portal.natsUrl, authenticator, }) + if (isStale()) { + await nc.drain().catch(() => {}) + stop() + return + } authUrlRef.current = nextAuthUrl ncRef.current = nc setUser({ ...userInfo, siteId: portal.siteId }) setConnected(true) nc.closed().then((err) => { + if (isStale()) return if (err) { setError(`Disconnected: ${err.message}`) } setConnected(false) }) @@ const disconnect = useCallback(async () => { + connectGenRef.current += 1 stop()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@chat-frontend/src/context/NatsContext/NatsContext.jsx` around lines 53 - 123, The nc.closed() callback in the connectToNats function can update state from stale connection attempts when multiple connect calls occur in quick succession. Guard the state updates in the nc.closed().then() callback (which sets setError and setConnected) with a generation check to prevent stale callbacks from overwriting state from newer connections. Implement this by creating a generation ref at the component level, incrementing it at the start of connectToNats before any async operations, capturing the current generation value in the callback closure, and only executing setError and setConnected if the captured generation matches the current generation ref value.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/client-api.md`:
- Around line 236-238: Add a language tag to the fenced code block containing
the GET request example. The opening fence for the HTTP request code block needs
to specify the language by changing the opening triple backticks to triple
backticks followed by "http" to comply with markdownlint MD040 and keep
documentation linting clean.
---
Outside diff comments:
In `@chat-frontend/src/context/NatsContext/NatsContext.jsx`:
- Around line 53-123: The nc.closed() callback in the connectToNats function can
update state from stale connection attempts when multiple connect calls occur in
quick succession. Guard the state updates in the nc.closed().then() callback
(which sets setError and setConnected) with a generation check to prevent stale
callbacks from overwriting state from newer connections. Implement this by
creating a generation ref at the component level, incrementing it at the start
of connectToNats before any async operations, capturing the current generation
value in the callback closure, and only executing setError and setConnected if
the captured generation matches the current generation ref value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a5e95512-a581-403b-9a16-dcb01e76dc4e
📒 Files selected for processing (52)
auth-service/deploy/docker-compose.ymlauth-service/handler.goauth-service/handler_test.goauth-service/integration_test.goauth-service/main.goauth-service/mock_store_test.goauth-service/store.goauth-service/store_mongo.gochat-frontend/CLAUDE.mdchat-frontend/deploy/30-render-config.shchat-frontend/deploy/config.js.templatechat-frontend/deploy/docker-compose.ymlchat-frontend/src/api/_transport/asyncJob.test.jschat-frontend/src/api/_transport/asyncJob.tschat-frontend/src/api/auth/oidcClient.jschat-frontend/src/context/NatsContext/NatsContext.jsxchat-frontend/src/context/NatsContext/NatsContext.test.jsxchat-frontend/src/context/NatsContext/useJwtRefresh.jschat-frontend/src/context/NatsContext/useJwtRefresh.test.jschat-frontend/src/lib/runtimeConfig.jschat-frontend/src/lib/runtimeConfig.test.jschat-frontend/src/pages/LoginPage/LoginPage.jsxchat-frontend/src/pages/LoginPage/LoginPage.test.jsxchat-frontend/src/pages/OidcCallback/OidcCallback.jsxchat-frontend/src/pages/OidcCallback/OidcCallback.test.jsxchat-frontend/vite.config.jsdocker-local/compose.services.yamldocker-local/setup.shdocs/client-api.mddocs/superpowers/plans/2026-06-11-portal-service.mddocs/superpowers/specs/2026-06-11-portal-service-design.mdpkg/errcode/codes_portal.gopkg/errcode/codes_test.gopkg/ginutil/middleware.gopkg/ginutil/middleware_test.gopkg/oidc/oidc.gopkg/oidc/oidc_test.gopkg/subject/subject.gopkg/subject/subject_test.goportal-service/cache.goportal-service/cache_test.goportal-service/deploy/Dockerfileportal-service/deploy/azure-pipelines.ymlportal-service/deploy/docker-compose.ymlportal-service/handler.goportal-service/handler_test.goportal-service/integration_test.goportal-service/main.goportal-service/mock_store_test.goportal-service/routes.goportal-service/store.goportal-service/store_mongo.go
💤 Files with no reviewable changes (1)
- chat-frontend/vite.config.js
✅ Files skipped from review due to trivial changes (4)
- pkg/errcode/codes_portal.go
- chat-frontend/CLAUDE.md
- portal-service/mock_store_test.go
- docs/superpowers/specs/2026-06-11-portal-service-design.md
🚧 Files skipped from review as they are similar to previous changes (40)
- chat-frontend/deploy/docker-compose.yml
- auth-service/deploy/docker-compose.yml
- chat-frontend/deploy/config.js.template
- chat-frontend/src/pages/OidcCallback/OidcCallback.jsx
- auth-service/store.go
- pkg/errcode/codes_test.go
- portal-service/integration_test.go
- pkg/oidc/oidc_test.go
- chat-frontend/src/context/NatsContext/useJwtRefresh.test.js
- chat-frontend/src/api/_transport/asyncJob.test.js
- docker-local/compose.services.yaml
- auth-service/store_mongo.go
- chat-frontend/src/api/_transport/asyncJob.ts
- portal-service/routes.go
- auth-service/mock_store_test.go
- portal-service/store.go
- pkg/oidc/oidc.go
- chat-frontend/src/api/auth/oidcClient.js
- portal-service/deploy/docker-compose.yml
- chat-frontend/src/pages/OidcCallback/OidcCallback.test.jsx
- pkg/subject/subject_test.go
- docker-local/setup.sh
- chat-frontend/src/lib/runtimeConfig.js
- portal-service/main.go
- pkg/ginutil/middleware.go
- chat-frontend/src/lib/runtimeConfig.test.js
- auth-service/handler.go
- portal-service/deploy/azure-pipelines.yml
- portal-service/deploy/Dockerfile
- portal-service/cache_test.go
- pkg/ginutil/middleware_test.go
- chat-frontend/src/pages/LoginPage/LoginPage.test.jsx
- auth-service/handler_test.go
- portal-service/handler.go
- chat-frontend/src/pages/LoginPage/LoginPage.jsx
- portal-service/handler_test.go
- portal-service/cache.go
- pkg/subject/subject.go
- portal-service/store_mongo.go
- auth-service/main.go
vjauhari-work
force-pushed
the
claude/eager-einstein-7u2je6
branch
from
396fd1b to
a214678
Compare
mliu33
requested changes
Jun 16, 2026
vjauhari-work
force-pushed
the
claude/eager-einstein-7u2je6
branch
4 times, most recently
from
d28531a to
72294af
Compare
Joey0538
pushed a commit
that referenced
this pull request
Jun 16, 2026
Single batch covering the decisions accepted in design review on
2026-06-16. Bot-account-namespace fix deferred per user direction;
to be addressed in a follow-up.
Auth spec — Part 1 (requirements):
§3a Per-site hostname scoping clarification (no central front door,
each site reaches its own botplatform-service at
{service}-{site}.chat-test.test.xx.com). Validate response
returns principal.class for downstream traffic isolation.
§5 Critical constraints — provisioning gate (REQUIRE_PROVISIONED,
fail-closed, mirrors PR #295); single home site per bot; dual
token format (legacy + bp1_ v1) replacing the old single-format
constraint.
§6 Migration coordinates credential import with provisioning row.
Auth spec — Part 2 (technical design):
§5.1 Login flow gains step 3 (provisioning gate after credential
verify, before session issue).
§9.8 /v1/auth/validate response schema expanded with principal.class,
siteId; documented as the contract the bot-traffic isolation
design consumes.
§16 Config: REQUIRE_PROVISIONED, SITE_ID added; pkg/ginutil
(from PR #295) named as the HTTP middleware source.
Auth spec — Part 3 (components):
§4.1 ApiGW injects X-Principal-Class header (sourced from validate
response) alongside X-User-Id / X-Account under mTLS.
§4.2 WebSocket server caches principal class on the connection for
its lifetime; tags every message published by the bot.
§6 Data-flow summary covers provisioning gate, per-site scope,
and explicitly notes that bots SKIP portal-service (humans use
three-step portal->auth->NATS; bots are two-step login->validate).
§7 Q10 (token format) marked DECIDED with the upgraded answer
cross-ref to Part 2 §4.3.
Bot-traffic isolation — Part 1:
§3 Architecture decision flipped from RECOMMENDED to DECIDED
(Option A — subject namespace split).
§9.1 New "Coupling with the auth migration" subsection — strict
sequencing: Phase 0 (instrument) independent; Phase 1+ requires
auth Phase 2+ (validate is the dual-token authority).
§9.2 New "Scope: fz1/wsp only" — split does not apply to legacy
fz2/chat services being sunset.
Bot-traffic isolation — Part 2:
§3.1 Class source locked to /v1/auth/validate.principal.class
(cross-ref auth Part 2 §9.8). Three-classes-two-lanes rule
documented (admin collapses into user lane for routing).
Legacy-token-class invariant noted (class derived from
principal, not token format).
§5.x Per-service split tables now reference per-class streams
by name (separate streams, §6).
§6 REWRITTEN: separate streams per class
(MESSAGES_CANONICAL_USER_{siteID} and
MESSAGES_CANONICAL_BOT_{siteID}), reversing the prior
shared-stream-with-filter default. Includes rationale table,
new pkg/stream constructors, and a JetStream-Mirror-based
rename migration for the existing stream.
§11 Replaced open-questions list with RESOLVED DECISIONS:
Q1 (separate streams), Q2 (envelope class field), Q3 (no
room-class), Q4 (no automatic threshold, quarterly review),
Q5 (no fixed pool quotas, alerts), Q6 (Phase 0 federation),
plus new resolutions: Q-admin-class (two lanes), Q-legacy-
token-class (derived from principal), Q-fz1-only (scope).
Q-codeaudit kept pending as the only follow-up.
Joey0538
pushed a commit
that referenced
this pull request
Jun 16, 2026
Walked the actual code on this branch and rewrote Part 2 §2 with
file:line citations matching the auth Part 2 §2 style. Found two
inaccuracies in the prior draft, fixed both, and flagged one
deferred-decision dependency.
What the audit confirmed:
- pkg/subject IS fully centralized (1070 lines, no fmt.Sprintf
leakage in loud-trio services). Class-aware mirror builders
propagate cleanly.
- pkg/stream.MessagesCanonical at stream.go:22 → subjects are
chat.msg.canonical.{siteID}.> (NOT chat.user.canonical.… as
prior drafts implied).
- pkg/model.MessageEvent at event.go:20 already has Timestamp;
adding Class string is the natural mirror.
- model.IsBotAccount at account.go:11 — *.bot suffix OR p_ prefix
— is the canonical class-derivation helper for non-validate paths.
- Existing isValidAccountToken at subject.go:738 does NOT reject
dots; PR #295 is the new stricter check (deferred per user).
What the audit corrected (vs prior spec drafts):
1. Subject namespace gotcha (§2.3): canonical is chat.msg.canonical.…
not chat.user.canonical.… The mirror table option (rename vs
asymmetric) depends on the bot-account-namespace fix that the
user deferred — flagged but kept the table provisionally
symmetric for now.
2. message-gatekeeper consumes from MESSAGES_{siteID} (upstream
stream), not MESSAGES_CANONICAL. The prior Part 2 §5.1 split
table claimed otherwise — fixed. Decision: message-gatekeeper
STAYS SHARED in Phase 1; only message-worker + broadcast-worker
actually split. Rationale: the user-scoped RPC subject space
chat.user.*.room.… doesn't carry a class token yet, and the
dominant bot cost is downstream (Cassandra writes + fan-out),
not validation. Gatekeeper derives class from IsBotAccount and
routes to the right downstream canonical stream — that's enough
isolation for Phase 1. Re-evaluate post-Phase-5 if metrics show
shared-gatekeeper saturation.
3. broadcast-worker has TWO subscriptions: JetStream consumer on
MESSAGES_CANONICAL (splits) AND core-NATS queue subscription on
chat.server.broadcast.{siteID}.> (stays shared — server-side
fire-and-forget badge events are class-agnostic by nature).
What the audit found but doesn't change:
- OUTBOX stream still in pkg/stream/stream.go:36-41 despite the
earlier supercluster-routing discussion. User deferred the
OUTBOX removal; treat as transitional.
Updated Part 1 §5 "loud pair" table to match the revised Phase 1
scope (message-gatekeeper deferred). Part 2 §5.1 rewritten with
the codeaudit rationale and a sketch of the publish-side class
routing.
Joey0538
pushed a commit
that referenced
this pull request
Jun 16, 2026
…zation User authorized rolling this in as part of the spec set (was deferred in an earlier batch). Resolves the PR #295 ACL/validation conflict and locks in the chat.{class}.… ontology across both specs. Auth Part 1 §5 — Critical constraints: New bullet documenting the *.bot account constraint, the subject- side dot normalization (xxx.bot -> xxx_bot via BotAccountToken), the chat.bot.> top-level namespace for bot JWTs, and that the strict IsValidAccountToken from PR #295 keeps applying to chat.user.> humans (no carve-out, no special case). Auth Part 2 §4.4 — NEW subsection "Bot account namespace & subject tokens (DECIDED 2026-06-16)": Layer 1 — separate top-level namespace: bots on chat.bot.>, humans on chat.user.>; top-level token disambiguates classes so human "xxx" cannot shadow bot "xxx.bot" no matter how the account token is derived (eliminates the chat.user.xxx.bot.foo ACL escape). Layer 2 — normalize account into a subject-safe token: BotAccountToken("xxx.bot") -> "xxx_bot" (round-trippable), IsValidBotAccount with ^[A-Za-z0-9_-]+\.bot$ as the legacy-bot validator (parallel to PR #295's IsValidAccountToken for humans). Per-class validator dispatch documented. PR #295 conflict resolution explained in detail. Bot-traffic isolation Part 2 §2.3 — flipped from open question to DECIDED: symmetric rename (chat.msg.canonical.… -> chat.user.canonical.…), justified by the new chat.{class}.… ontology — leaving the canonical stream on the chat.msg.… second-token would be the lone asymmetry forever. Files-touched inventory carried over from the earlier audit. Bot-traffic isolation Part 2 §4.2 — Builder API extended with AccountToken(class, account) helper that calls BotAccountToken for the bot class. Cross-spec invariants section pointing at the auth Part 2 §4.4 helpers as the source of truth. Bot-traffic isolation Part 2 §4.3 — Mirror table updated: {botToken} notation introduced; bot JWT scope corrected from the prior chat.bot.{account}.> to chat.bot.{botToken}.>; row 3 (canonical) flagged as the load-bearing Phase-1 change; rows 1-2 deferred to Phase 5+ when (and if) user-scoped RPC subjects get classed. Bot-traffic isolation Part 2 §6.3 — Rename migration extended to cover the subject-prefix rename in addition to the stream-name rename. Mirror + SubjectTransform on the user stream maps chat.msg.canonical.… -> chat.user.canonical.… during the dual- publish window. Bot stream created fresh (no Mirror, no historical data). Rollback path documented. Net: bot account ns fix is now coherent end-to-end across the auth spec (identity + JWT scope) and the bot-traffic isolation spec (subject namespace + per-service routing). No code changes yet — spec-only. Implementation PRs cite these decisions verbatim.
mliu33
reviewed
Jun 17, 2026
mliu33
left a comment
mliu33
left a comment
Collaborator
There was a problem hiding this comment.
Great work! Just one minor suggestion, thanks
vjauhari-work
force-pushed
the
claude/eager-einstein-7u2je6
branch
from
72294af to
35b13d7
Compare
Joey0538
pushed a commit
that referenced
this pull request
Jun 17, 2026
Operational companion to the design spec set. Covers the three Mongo collections touched by the bot-platform-nextgen rollout, with explicit AS-IS (current state) and TO-BE (target state) shapes per collection plus the delta and the migration mechanic. Collections covered: - users (modified, shared): the provisioning-gate fields PR #295 introduced + the {account, siteId} compound index we depend on. Migration of users itself is owned by PR #295 and the live users-sync — this runbook just reads from it. - credentials (NEW): extracted from legacy users.services.password. Schema, indexes, bcrypt-verbatim import, idempotent upsert pseudocode, reconciliation queries, rollback. - sessions (NEW): extracted from legacy users.services.resume. loginTokens[]. Token-hash dual-scheme (legacy SHA-256 + v1 HMAC, cross-ref auth Part 2 §4.3), partial TTL on expiresAt, PAT skip rule, per-site siteId pinning, reconciliation queries, rollback. Each collection gets: - AS-IS schema (legacy RC + current nextgen state) - TO-BE schema (post-rollout) - Per-field delta table with transformation - Index list (existing + new) - Idempotent + resumable migration pseudocode - Reconciliation queries for ops to verify counts - Rollback procedure End-to-end §5 ordering table walks the per-site rollout step by step (0 dark deploy -> 1 indexes -> 2 credentials import -> 3 sessions import -> 4 activate live users-sync -> 5 chat-GW canary -> 6 sunset), with pre-check / post-check / rollback per step. §6 reconciliation cheat sheet collects the canary-phase health-check queries in one place — count matches, site-affinity checks, validate- path performance spot-check. §7 pending-verification list flags the 5 things ops/external teams still need to confirm before the runbook moves out of DRAFT. This is intentionally separated from the auth design spec — runbooks evolve as ops learns things during the canary, and design specs shouldn't churn for operational details.
Joey0538
pushed a commit
that referenced
this pull request
Jun 17, 2026
Layers all the design decisions accepted in review on 2026-06-16 onto
the existing auth spec set. No new docs — modifications to Parts 1, 2,
and 3 only.
Token format (Q10 upgraded):
- Native v1 tokens: bp1_<43-char base64url of 32 random bytes>
(always-on versioned prefix; gates validator hash-dispatch, enables
forward-versioning to bp2_..., detected by standard secret scanners).
- Storage hash for v1: base64(HMAC-SHA-256(server_secret, token))
instead of plain SHA-256 — defense in depth against offline attack
on a stolen sessions table. Cost identical; storage size identical.
- Legacy tokens stay byte-for-byte compatible (base64(sha256(token)))
for zero-bot-change migration; validator dispatches on prefix.
- NOT bcrypt/Argon2 for tokens (256-bit entropy makes slow-hash pure
waste); NOT JWT (cheap revocation + session enumeration + sliding
expiry all need a session store anyway). Documented in Part 2 §4.3.
Provisioning-gated login (mirrors PR #295's auth-service gate):
- REQUIRE_PROVISIONED=true (default) — after credential verify, the
account's {userId, siteId} must exist in this site's users
collection; else 403 account_not_provisioned. Fail-closed on store
errors. Single home site per bot. Part 1 §5, §6; Part 2 §5.1, §16.
principal.class on /v1/auth/validate response:
- Validate returns {valid, principal:{userId, account, username,
roles, class, siteId}} where class ∈ {"bot","user","admin"}.
- ApiGW injects X-Principal-Class on the forwarded request (mTLS);
WS server caches class on the connection for its lifetime;
EventConsumer tags every webhook event. Sourced ONCE at the edge
from the validated principal — never re-derived downstream.
- Part 2 §9.8 (response schema); Part 3 §4.1 (ApiGW flow), §4.2 (WS).
Per-site hostname scoping:
- Each site reaches its own botplatform-service at
{service}-{site}.chat-test.test.xx.com. No central front door.
- Bots SKIP portal-service entirely (humans use 3-step portal -> auth
-> NATS; bots are 2-step login -> validate; site is in the URL).
- Part 1 §3a; Part 3 §6 ("Skip portal-service" subsection).
Bot-account namespace fix (resolves the PR #295 conflict):
- Bots use chat.bot.{botToken}.> JWT scope, NOT chat.user.>
(top-level namespace disambiguation — human "xxx" cannot shadow
bot "xxx.bot" no matter how the account token is derived).
- Account-to-subject normalization via subject.BotAccountToken:
"xxx.bot" -> "xxx_bot" (dot-replace, round-trippable).
- subject.IsValidBotAccount allows ^[A-Za-z0-9_-]+\.bot$ shape;
PR #295's strict subject.IsValidAccountToken keeps applying to
humans on chat.user.>.
- Part 1 §5 constraint; Part 2 §4.4 full design.
Other:
- pkg/ginutil (from PR #295) cited as the HTTP middleware source for
botplatform-service; pkg/restyutil for outbound HTTP.
- TOKEN_HMAC_KEY (required) + TOKEN_HMAC_KEY_PREVIOUS (graceful
rollover) + REQUIRE_PROVISIONED + SITE_ID added to §16 config.
- Q10 marked DECIDED with the upgraded answer in Part 2 §12 + Part 3
§7 cross-ref.
- Migration coordinates credential import with provisioning-row write
in one transaction per bot (Part 1 §6).
Joey0538
pushed a commit
that referenced
this pull request
Jun 17, 2026
Operational companion to the design spec set. Covers the three Mongo collections touched by the bot-platform-nextgen rollout, with explicit AS-IS (current state) and TO-BE (target state) shapes per collection plus the delta and the migration mechanic. Collections covered: - users (modified, shared): provisioning-gate fields owned by PR #295 + the {account, siteId} compound index our gate depends on. Migration of users itself is owned by PR #295 + the live users-sync; this runbook just reads from it. - credentials (NEW): extracted from legacy users.services.password. Schema, indexes, bcrypt-verbatim import, idempotent upsert pseudocode, reconciliation queries, rollback. - sessions (NEW): extracted from legacy users.services.resume. loginTokens[]. Dual-scheme token hash (legacy SHA-256 + v1 HMAC, cross-ref auth Part 2 §4.3), partial TTL on expiresAt, PAT skip rule, per-site siteId pinning, reconciliation queries, rollback. Each collection gets: AS-IS schema, TO-BE schema, per-field delta table with transformation, index list (existing + new), idempotent + resumable migration pseudocode, reconciliation queries, rollback. §5 end-to-end ordering table walks the per-site rollout step by step (0 dark deploy -> 1 indexes -> 2 credentials import -> 3 sessions import -> 4 activate live users-sync -> 5 chat-GW canary -> 6 sunset), with pre-check / post-check / rollback per step. §6 reconciliation cheat sheet collects the canary-phase health-check queries in one place — count matches, site-affinity checks, validate- path performance spot-check. §7 pending-verification list flags the 5 items ops/external teams must confirm before the runbook moves out of DRAFT (most importantly the users.{account, siteId} compound index — confirm PR #295 added it or add in step 1). Intentionally separated from the auth design spec — runbooks evolve as ops learns things during the canary; design specs shouldn't churn for operational detail.
7 tasks
Joey0538
pushed a commit
that referenced
this pull request
Jun 17, 2026
…s I-III)
Single-file design spec for the bot-platform-nextgen auth migration —
password auth + durable sessions for bots and admins, migrating from
legacy Rocket.Chat to the nextgen NATS-native stack with zero bot
code changes. Combines what were three separate Part 1/2/3 markdown
files into one document under H1 part dividers (consolidation
2026-06-17 to reduce file count + cross-reference churn).
Part I — Architecture & Requirements:
- DECIDED: Option B / DEDICATED-SERVICE (new botplatform-service)
over EXTEND-AUTH. Selected 2026-06-15 because the bot edge grew
into a browser-facing web app (HTML/CSRF/cookies) + dual-token
validation; isolating from the JWT-signing, human-SSO auth-service
wins on blast-radius/key safety first and independent scaling
second.
- 8 user stories US1-US8 covering bot login, session API access,
long-lived sessions, admin bot creation, password rotation,
session management, web login, web change-password.
- Critical constraints: 17-char Meteor IDs preserved, bcrypt
passwords, dual-format tokens (legacy + bp1_), provisioning gate
(REQUIRE_PROVISIONED, mirrors PR #295), single home site per bot,
bot account namespace fix (chat.bot.{botToken}.> + dot
normalization xxx.bot -> xxx_bot).
- Cross-cluster cutover: chat gateway (fz2) stays permanent front
door; weight-split to wsp gateway (fz1). DNS unchanged. Per-ns
DNS binding makes DNS-repoint infeasible — permanent thin
forwarder steady state.
- Diagrams §10 points at current FigJam source of truth + STALE
callout for the pre-decision SVGs.
Part II — Technical Design:
- Data model: credentials + sessions schemas with field tables.
- Token format §4.3 (DECIDED Q10): native v1 = bp1_<43-char
base64url of 32 random bytes>, stored base64(HMAC-SHA-256(
server_secret, token)); legacy = base64(sha256(token)) byte-for-
byte. Prefix dispatches validator hash function (no double-lookup
on native tokens). HMAC storage hardens against offline attack on
stolen sessions table; cost identical to plain SHA-256.
- Bot account namespace §4.4 (DECIDED 2026-06-16): two-layer fix —
chat.bot.> top-level disambiguation + BotAccountToken/
IsValidBotAccount helpers. Resolves the PR #295 strict-validator
conflict (existing bot accounts contain dots which the new
IsValidAccountToken rejects) and eliminates the chat.user.xxx.>
vs chat.user.xxx.bot.> ACL escape.
- Login flow §5.1 with provisioning-gate step between credential
verify and session issue.
- Validate path §9.8 returns {valid, principal:{userId, account,
username, roles, class, siteId}} — class ∈ {bot, user, admin} is
the authoritative source consumed by ApiGW / WS / EventConsumer
and by the bot-traffic isolation spec.
- Config §16: TOKEN_HMAC_KEY (required), TOKEN_HMAC_KEY_PREVIOUS
(rollover), REQUIRE_PROVISIONED, SITE_ID, pkg/ginutil for HTTP
middleware (from PR #295), pkg/restyutil for outbound.
- Test plan §18 golden fixtures for both hash schemes, dispatch
test, v1 token shape test.
- §10 cross-cluster cutover cleaned up: duplicate §10.2 deleted
(stale single-cluster narrative); DNS-repoint alternative removed
(verified infeasible per-ns DNS binding); §10.1 heading flipped to
PERMANENT front door with steady-state implication noted.
Part III — Components & Integration:
- botplatform-service (new, ours) vs botplatform-server (existing,
external) naming disambiguation in §1.
- ApiGW injects X-Principal-Class on the forwarded request under
mTLS so Server can trust the injected principal — load-bearing for
bot-traffic isolation routing.
- WebSocket server caches principal class on the connection for
its lifetime; tags every published message.
- Data-flow §6 covers provisioning gate per-site scope and
explicitly notes bots SKIP portal-service (humans use 3-step
portal -> auth -> NATS; bots are 2-step login -> validate).
Consistency cleanup baked in (resolved naming conflict between this
spec and the bot-traffic spec — both used Option A/B labels for
different decisions; suffixes EXTEND-AUTH / DEDICATED-SERVICE added
here, SUBJECT-SPLIT / QUEUE-GROUP-ONLY / HYBRID added in the
bot-traffic spec). Stale "(recommended)" tag on the EXTEND-AUTH
heading removed.
Joey0538
pushed a commit
that referenced
this pull request
Jun 17, 2026
Operational companion to the design specs. Covers the three Mongo
collections touched by the bot-platform-nextgen rollout, in AS-IS
(current state) vs TO-BE (target state) format with explicit delta
and migration mechanic per collection.
Collections covered:
- users (modified, shared): provisioning-gate fields owned by PR
#295 + the {account, siteId} compound index our gate depends on.
- credentials (NEW): extracted from legacy users.services.password.
Schema, indexes, bcrypt-verbatim import, idempotent upsert
pseudocode, reconciliation queries, rollback.
- sessions (NEW): extracted from legacy users.services.resume.
loginTokens[]. Dual-scheme token hash (legacy SHA-256 + v1 HMAC,
cross-ref auth spec Part II §4.3), partial TTL on expiresAt, PAT
skip rule, per-site siteId pinning.
§5 end-to-end ordering table walks the per-site rollout step by step
(0 dark deploy -> 1 indexes -> 2 credentials import -> 3 sessions
import -> 4 activate live users-sync -> 5 chat-GW canary -> 6 sunset),
with pre-check / post-check / rollback per step.
§6 reconciliation cheat sheet collects the canary-phase health-check
queries (count matches, site-affinity checks, validate-path
performance spot-check).
§7 pending-verification list flags the 5 items ops/external teams
must confirm before the runbook moves out of DRAFT.
Intentionally kept as a separate file from the auth design spec —
runbooks evolve as ops learns things during the canary; design specs
shouldn't churn for operational detail.
Cross-references updated to point at the consolidated design files
(bot-platform-nextgen-auth.md combined Parts I+II+III;
bot-traffic-isolation.md combined Parts I+II).
Joey0538
pushed a commit
that referenced
this pull request
Jun 17, 2026
…ombined)
Single-file design spec for password auth + durable sessions for bots
and admins, migrating from legacy Rocket.Chat to the nextgen NATS-
native stack with zero bot code changes. Three parts under H1 dividers:
Part I = requirements & architecture (for product/architects),
Part II = technical design (for implementers), Part III = components &
integration (for downstream service teams).
Part I — Architecture & Requirements:
- DECIDED 2026-06-15: Option B / DEDICATED-SERVICE (new
botplatform-service) over EXTEND-AUTH. The bot edge grew into a
browser-facing web app (HTML/CSRF/cookies) + dual-token validation;
isolating from the JWT-signing, human-SSO auth-service wins on
blast-radius/key safety + independent scaling.
- 8 user stories (US1-US8): bot login, session API access, long-
lived sessions, admin bot creation, password rotation, session
management, web login, web change-password.
- Critical constraints: 17-char Meteor IDs preserved, bcrypt
passwords, dual-format tokens (legacy + bp1_), provisioning gate
(REQUIRE_PROVISIONED, mirrors PR #295), single home site per bot,
bot account namespace fix (chat.bot.{botToken}.> + dot
normalization xxx.bot -> xxx_bot).
- Cross-cluster cutover: chat gateway (fz2) is the PERMANENT front
door — DNS is bound per-namespace in this environment so
DNS-repoint to wsp gateway is infeasible. Weight-split cross-
cluster to wsp gateway (fz1). chat-GW becomes a permanent thin
forwarder steady state; HA accordingly.
- §10 Diagrams section embeds the rendered PNGs (from the diagrams/
commit) and points at the .drawio source files.
Part II — Technical Design:
- Data model: credentials + sessions schemas with full field tables.
- Token format §4.3 (DECIDED Q10): native v1 = bp1_<43-char
base64url of 32 random bytes>, stored base64(HMAC-SHA-256(
server_secret, token)); legacy = base64(sha256(token)) byte-for-
byte. Validator dispatches on bp1_ prefix (no double-lookup for
native tokens). HMAC storage hardens against offline attack on a
stolen sessions table; cost identical to plain SHA-256. Forward
versioning headroom (bp2_...) without DB migration. Detected by
standard secret scanners. NOT bcrypt/Argon2 — tokens are 256-bit
entropy, slow-hashing them is pure cost. NOT JWT — cheap
revocation + enumeration need a session store anyway. NOT
rotating bot tokens — bots are long-lived processes.
- Bot account namespace §4.4 (DECIDED 2026-06-16): two-layer fix —
chat.bot.> top-level disambiguation + BotAccountToken /
IsValidBotAccount helpers. Resolves PR #295 strict-validator
conflict (existing bot accounts contain dots) AND eliminates the
chat.user.xxx.> vs chat.user.xxx.bot.> ACL escape.
- Login flow §5.1 with the provisioning-gate step between credential
verify and session issue (fail-closed on store errors).
- Validate path §9.8 returns { valid, principal:{ userId, account,
username, roles, class, siteId } }. class ∈ {bot, user, admin} is
the authoritative source consumed by ApiGW / WS / EventConsumer
and by the bot-traffic isolation spec.
- Config §16: TOKEN_HMAC_KEY (required), TOKEN_HMAC_KEY_PREVIOUS
(rollover), REQUIRE_PROVISIONED, SITE_ID, pkg/ginutil for HTTP
middleware (from PR #295), pkg/restyutil for outbound.
- §10 cross-cluster cutover: DNS-repoint alternative removed
(verified infeasible per-ns DNS binding); duplicate §10.2 deleted;
§10.1 heading flipped to PERMANENT front door.
- Test plan §18: golden fixtures for both hash schemes, prefix
dispatch test, v1 token shape test.
Part III — Components & Integration:
- botplatform-service (new, ours) vs botplatform-server (existing,
external) naming disambiguation in §1.
- ApiGW injects X-Principal-Class on the forwarded request under
mTLS — load-bearing for bot-traffic isolation routing downstream.
- WebSocket server caches principal class on the connection for its
lifetime; tags every published message.
- Data-flow §6 covers provisioning gate per-site scope; explicitly
notes bots SKIP portal-service (humans use 3-step portal -> auth ->
NATS; bots are 2-step login -> validate; site is in the hostname).
Naming convention: Option labels use descriptive suffixes (Option A /
EXTEND-AUTH, Option B / DEDICATED-SERVICE) to disambiguate from the
bot-traffic-isolation spec's own Option A/B/C decision (which uses
SUBJECT-SPLIT / QUEUE-GROUP-ONLY / HYBRID suffixes).
Joey0538
pushed a commit
that referenced
this pull request
Jun 17, 2026
Single-file design spec for isolating bot traffic from human traffic
in the chat backend without forking any service implementation. Two
parts under H1 dividers: Part I = requirements & architecture, Part
II = technical design. Companion to the auth migration spec — together
they cover identity (auth) + routing (this).
Part I — Architecture & Requirements:
- DECIDED 2026-06-16: Option A / SUBJECT-SPLIT (chat.bot.… mirrors
chat.user.…) over QUEUE-GROUP-ONLY (no real isolation, round-robin
LB still starves human pool on a bot burst) and HYBRID (two
routing models, hard to reason about). Naming uses descriptive
suffixes to disambiguate from the auth spec's own Option A/B
decision (which decided DEDICATED-SERVICE).
- Scope: split the loud pair (message-worker + broadcast-worker)
first. message-gatekeeper STAYS SHARED in Phase 1 — its consumer
reads from MESSAGES_{siteID} whose subjects don't carry a class
token; defer until per-class metrics show shared-gatekeeper
saturation.
- 9 user stories US1-US9: human SLOs survive bot burst, independent
scaling, automatic routing, no code fork, cross-site federation,
per-class observability, independent canary, connection-pool
isolation, reversibility.
- 5-phase reversible rollout (instrument -> dual-publish ->
dual-subscribe -> split deployments -> cutover -> sunset), coupled
with auth migration phases (Phase 0 independent; Phase 1+ gated
on auth Phase 2+ where validate is the dual-token authority).
- fz1/wsp only — no splits in legacy fz2/chat being sunset.
Part II — Technical Design:
- §2 Current state (grounded) with file:line citations against
pkg/subject (1070 lines, centralized; isValidAccountToken at
subject.go:738 accepts dots — PR #295's stricter validator is the
deferred-fix concern), pkg/stream (canonical subject is
chat.msg.canonical.{siteID}.>, NOT chat.user.canonical.… as
earlier drafts implied), pkg/model/event.go (Timestamp pattern
confirmed — Class field mirrors cleanly), pkg/model/account.go
(IsBotAccount as the class derivation helper for non-validate
paths), each loud-trio service's main.go (consumer setup
verified).
- §2.3 canonical subject rename DECIDED symmetric:
chat.msg.canonical.{siteID}.> -> chat.user.canonical.{siteID}.>
+ chat.bot.canonical.{siteID}.> (preserves chat.{class}.…
ontology everywhere; the lone asymmetry of chat.msg.… would be
confusing forever).
- §3 principal classification: class set ONCE at the edge from
/v1/auth/validate.principal.class (auth spec Part II §9.8). Three
classes (bot, user, admin) collapse to two routing lanes
(admin -> user). Legacy-token-class invariant: class derived from
principal, not token format.
- §4 pkg/subject builder API with Class parameter +
AccountToken(class, account) helper dispatching to BotAccountToken
for bots; mirror table with {botToken} notation.
- §6 separate streams per class: MESSAGES_CANONICAL_USER_{siteID} +
MESSAGES_CANONICAL_BOT_{siteID} (reverses spec-draft default of
shared+filter — shared streams couple back-pressure on the write
path and at the storage-budget level, undermining the isolation
thesis). JetStream Mirror + SubjectTransform for the rename
migration during the dual-publish window; rollback path
documented.
- §7 NATS supercluster permissions extended to chat.bot.>.
- §11 RESOLVED DECISIONS (Q1-Q6 + Q-admin-class + Q-legacy-token-
class + Q-fz1-only); Q-codeaudit closed by §2.
Diagrams (FigJam, kept as faster first-pass-review):
- View A: https://www.figma.com/board/hScsGyDTbGhT7laIwJsVkx
- View B: https://www.figma.com/board/6vkFEKMJ0WyES2VTpBVstM
Joey0538
pushed a commit
that referenced
this pull request
Jun 17, 2026
Operational companion to the design specs. Covers the three Mongo
collections touched by the bot-platform-nextgen rollout, in AS-IS
(current state) vs TO-BE (target state) format with explicit delta
and migration mechanic per collection. Audience: SRE/ops running the
migration; engineers writing the migration jobs.
Collections covered:
- users (modified, shared): provisioning-gate fields owned by PR
#295 + the {account, siteId} compound index our gate depends on.
users itself isn't migrated by this work — PR #295 + live users-
sync own that; this runbook just reads from it.
- credentials (NEW): extracted from legacy users.services.password.
Bcrypt copied verbatim (we don't have the plaintext). Schema,
indexes, idempotent upsert pseudocode, reconciliation queries,
rollback procedure.
- sessions (NEW): extracted from legacy users.services.resume.
loginTokens[]. Dual-scheme token hash (legacy SHA-256 + v1 HMAC,
cross-ref auth spec Part II §4.3), partial TTL on expiresAt, PAT
skip rule (type:"personalAccessToken" — human-only), per-site
siteId pinning, reconciliation queries, rollback.
Each collection gets: AS-IS schema, TO-BE schema, per-field delta
table with transformation, index list (existing + new), idempotent +
resumable migration pseudocode, reconciliation queries, rollback.
§5 end-to-end ordering table walks the per-site rollout step by step
(0 dark deploy -> 1 indexes -> 2 credentials import -> 3 sessions
import -> 4 activate live users-sync -> 5 chat-GW canary -> 6 sunset),
with pre-check / post-check / rollback per step.
§6 reconciliation cheat sheet collects the canary-phase health-check
queries (count matches, site-affinity checks, validate-path
performance spot-check).
§7 pending-verification list flags the 5 items ops/external teams
must confirm before the runbook moves out of DRAFT (most importantly:
does PR #295 add the users.{account, siteId} compound index, or do we?
And: per-site DB topology — one chat DB per site, or shared cluster?).
Intentionally kept as a separate file from the auth design spec —
runbooks evolve as ops learns things during the canary; design specs
shouldn't churn for operational detail.
Joey0538
pushed a commit
that referenced
this pull request
Jun 17, 2026
…ombined)
Single-file design spec for password auth + durable sessions for bots
and admins, migrating from legacy Rocket.Chat to the nextgen NATS-
native stack with zero bot code changes. Three parts under H1 dividers:
Part I = requirements & architecture, Part II = technical design,
Part III = components & integration.
Architecture DECIDED 2026-06-15: Option B / DEDICATED-SERVICE (new
botplatform-service) over EXTEND-AUTH. The bot edge grew into a
browser-facing web app + dual-token validation; isolating from the
JWT-signing, human-SSO auth-service wins on blast-radius/key safety
plus independent scaling.
Token format DECIDED 2026-06-16 (Q10 upgraded): native v1 tokens use
bp1_<43-char base64url of 32 random bytes> with HMAC-SHA-256(
TOKEN_HMAC_KEY, token) storage hash. Legacy tokens unchanged for
byte-for-byte compatibility. Validator dispatches on bp1_ prefix.
Bot account namespace fix DECIDED 2026-06-16: bots scope to
chat.bot.{botToken}.> where botToken = dot-normalized account
(xxx.bot -> xxx_bot via subject.BotAccountToken). Resolves PR #295
strict-validator conflict + eliminates the chat.user.xxx.> vs
chat.user.xxx.bot.> ACL escape. IsValidBotAccount validates the
*.bot shape; PR #295's strict IsValidAccountToken keeps applying to
humans on chat.user.>.
Provisioning gate (REQUIRE_PROVISIONED default true) mirrors auth-
service from PR #295: after credential verify, account's
{userId, siteId} must exist in this site's users collection.
Fail-closed on store errors. Single home site per bot.
Cross-cluster cutover: chat gateway (fz2) = PERMANENT front door —
DNS bound per-namespace, DNS-repoint to wsp gateway infeasible.
Weight-split cross-cluster to wsp gateway. After sunset chat-GW
becomes a permanent thin forwarder (HA accordingly).
§9.8 /v1/auth/validate response carries principal.class ∈ {bot, user,
admin} — authoritative source consumed by ApiGW (injects
X-Principal-Class under mTLS), WS server (caches per connection),
EventConsumer (per event), and by the bot-traffic isolation spec.
§16 config adds TOKEN_HMAC_KEY (required), TOKEN_HMAC_KEY_PREVIOUS
(graceful rollover), REQUIRE_PROVISIONED, SITE_ID. pkg/ginutil for
HTTP middleware (from PR #295), pkg/restyutil for outbound.
§10 Diagrams section embeds 4 PNG previews of the .drawio files
committed under docs/specs/diagrams/ (login old-vs-new comparison,
token generation/validation flow, bot login end-to-end, cross-
cluster cutover topology).
Option label disambiguation: this spec uses Option A/B with
descriptive suffixes (EXTEND-AUTH, DEDICATED-SERVICE) to disambiguate
from the bot-traffic-isolation spec's own Option A/B/C labels (which
use SUBJECT-SPLIT / QUEUE-GROUP-ONLY / HYBRID suffixes for a totally
different decision).
User stories US1-US8: bot login, session API access, long-lived
sessions, admin bot creation, password rotation, session management,
web login, web change-password.
Cross-cluster cutover §10 cleaned up: stale (recommended) tag on
EXTEND-AUTH heading removed; duplicate §10.2 deleted; DNS-repoint
alternative removed (verified infeasible).
Joey0538
pushed a commit
that referenced
this pull request
Jun 17, 2026
Operational companion to the design specs. Covers the three Mongo
collections touched by the bot-platform-nextgen rollout, in AS-IS
(current state) vs TO-BE (target state) format with explicit delta
and migration mechanic per collection. Audience: SRE/ops running the
migration; engineers writing the migration jobs.
Collections covered:
- users (modified, shared): provisioning-gate fields owned by PR
#295 + the {account, siteId} compound index our gate depends on.
users itself isn't migrated by this work — PR #295 + the live
users-sync own that; this runbook just reads from it.
- credentials (NEW): extracted from legacy users.services.password.
Bcrypt copied verbatim (we don't have the plaintext). Schema,
indexes, idempotent upsert pseudocode, reconciliation queries,
rollback procedure.
- sessions (NEW): extracted from legacy users.services.resume.
loginTokens[]. Dual-scheme token hash (legacy SHA-256 + v1 HMAC,
cross-ref auth spec Part II §4.3), partial TTL on expiresAt, PAT
skip rule (type:"personalAccessToken" — human-only), per-site
siteId pinning.
Each collection: AS-IS schema, TO-BE schema, per-field delta table
with transformation, index list (existing + new), idempotent +
resumable migration pseudocode, reconciliation queries, rollback.
§5 end-to-end ordering table walks the per-site rollout step by step
(0 dark deploy -> 1 indexes -> 2 credentials import -> 3 sessions
import -> 4 activate live users-sync -> 5 chat-GW canary -> 6 sunset),
with pre-check / post-check / rollback per step.
§6 reconciliation cheat sheet collects the canary-phase health-check
queries (count matches, site-affinity checks, validate-path
performance spot-check).
§7 pending-verification list flags 5 items ops/external teams must
confirm before the runbook moves out of DRAFT.
Intentionally kept as a separate file from the auth design spec —
runbooks evolve as ops learns things during the canary; design specs
shouldn't churn for operational detail.
Joey0538
pushed a commit
that referenced
this pull request
Jun 17, 2026
…ombined)
Single-file design spec for password auth + durable sessions for bots
and admins, migrating from legacy Rocket.Chat to the nextgen NATS-
native stack with zero bot code changes. Three parts under H1 dividers:
Part I = requirements & architecture, Part II = technical design,
Part III = components & integration.
Architecture DECIDED 2026-06-15: Option B / DEDICATED-SERVICE (new
botplatform-service) over EXTEND-AUTH. The bot edge grew into a
browser-facing web app + dual-token validation; isolating from the
JWT-signing, human-SSO auth-service wins on blast-radius/key safety
plus independent scaling.
Token format DECIDED 2026-06-16 (Q10 upgraded): native v1 tokens use
bp1_<43-char base64url of 32 random bytes> with HMAC-SHA-256(
TOKEN_HMAC_KEY, token) storage hash. Legacy tokens unchanged for
byte-for-byte compatibility. Validator dispatches on bp1_ prefix.
Bot account namespace fix DECIDED 2026-06-16: bots scope to
chat.bot.{botToken}.> where botToken = dot-normalized account
(xxx.bot -> xxx_bot via subject.BotAccountToken).
Provisioning gate (REQUIRE_PROVISIONED default true) mirrors auth-
service from PR #295.
Cross-cluster cutover: chat gateway (fz2) = PERMANENT front door —
DNS bound per-namespace, DNS-repoint to wsp gateway infeasible.
§9.8 /v1/auth/validate response carries principal.class ∈ {bot, user,
admin} — authoritative source consumed by ApiGW (injects
X-Principal-Class under mTLS), WS server (caches per connection),
EventConsumer (per event), and by the bot-traffic isolation spec.
CodeRabbit review fixes folded in:
• §5.1 step 5 (line 485): native-bot JWT minting clarified — botplatform-
service NEVER signs JWTs directly (Option B / DEDICATED-SERVICE chose
to keep the signing key in auth-service). Native JWT minting is
DEFERRED from July scope; when wired later it's a service-to-service
call to auth-service via AUTH_SERVICE_URL.
• §14 algorithms pseudocode (line 884): "nats.Request(auth.session.
validate, h)" was misleading — botplatform-service exposes NO NATS
surface (§15). Rewrote as in-process sessionStore.Lookup with a
comment explaining that downstream consumers reach the same logic
via POST /v1/auth/validate (§9.8), not via NATS.
• Part III §1: "API proxy" removed from the botplatform-service one-
line summary — explicitly contradicted Part III §4.1 (we are NOT
in the /api/v2/* data path). Replaced with the accurate "Auth + web
UI + token validation + admin RPCs" plus an explicit non-proxy note.
• MD040: 7 unlabeled code fences in §7 (Architecture decision), §9
(Topology), §14 (Algorithms), §15 (Routes), and Part III §4.3
(token compatibility) got "text" / "go" language tags.
• MD058: blank line added before the table in Part III §4.3.
Cross-spec citations now use full filenames (docs/specs/README.md added
in the diagrams commit explains the convention). Within-doc "Part 1/2/3"
references normalized to "Part I/II/III" to match the H1 dividers.
Option label disambiguation: Option A / EXTEND-AUTH (rejected),
Option B / DEDICATED-SERVICE (selected). Descriptive suffixes avoid
collision with the bot-traffic-isolation spec's own Option A/B/C labels
(which use SUBJECT-SPLIT / QUEUE-GROUP-ONLY / HYBRID).
User stories US1-US8: bot login, session API access, long-lived sessions,
admin bot creation, password rotation, session management, web login,
web change-password.
§10 Diagrams section embeds 4 PNG previews of the .drawio files
committed under docs/specs/diagrams/ (login old-vs-new comparison,
token generation/validation flow, bot login end-to-end, cross-cluster
cutover topology).
Stale (recommended) tag on EXTEND-AUTH heading removed; duplicate
§10.2 deleted; DNS-repoint alternative removed (verified infeasible).
…minting
Portal-service (new): resolves a logged-in account's home site and
returns its auth-service / NATS coordinates. The directory is an
in-memory cache of the HR-owned hr_employee collection (account,
employeeId, siteId, natsUrl — rewritten by a daily HR cron): loaded by
a startup goroutine, swapped wholesale on a periodic refresh
(PORTAL_CACHE_REFRESH_INTERVAL, default 24h, no per-entry TTL), with
30s retries after a failed load. /lookup is a single in-memory hit —
no per-request Mongo queries; a miss returns 403 account_not_ready.
authServiceUrl is derived, not stored: PORTAL_AUTH_URL_TEMPLATE
substitutes {siteId} into the placeholder URL (used verbatim when no
placeholder, e.g. single-site local dev). /healthz is liveness-only;
GET /readyz returns 503 until the cache holds directory data.
OIDC-validated in prod, fallback-site dev mode for local stacks.
Auth-service: optional provisioning gate at minting authority — in prod
with REQUIRE_PROVISIONED=true (default), an {account, siteId} document
must exist in the site's users collection before a NATS user JWT is
signed (403 account_not_provisioned otherwise; store errors fail closed).
Loud warnings whenever the gate is off.
Identity hardening: Claims.Account() trusts only preferred_username
(name is user-editable display data); accounts are validated against the
routing layer's token invariant via the new subject.IsValidAccountToken
(non-empty, no '.'/'*'/'>'/whitespace/control) before gate + sign, so
dots and wildcards can never reach chat.user.{account}.> permissions.
Shared middleware: request-ID, access-log and CORS Gin middleware moved
from auth-service to pkg/ginutil, consumed by both HTTP services.
Frontend: login is now portal-lookup → auth → connect with the resolved
URLs; JWT refresh re-mints against the resolved auth URL (late-bound);
errcode envelope errors surface reason/code to the UI with
account_not_provisioned and account_not_ready mapped to
admin-contact copy.
Docs: client-api.md §2.1 login sequence, §2.2 gate errors + account rule,
new §2.3 portal lookup spec, §6 reason registry.
Tests: unit + integration (testutil Mongo) for both services, pkg/oidc,
pkg/ginutil, pkg/subject; frontend vitest for the two-step flow, envelope
propagation, refresh URL late-binding and error copy.
https://claude.ai/code/session_01QK7uzcWZyGDfR7vr24suPi
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Squashed response to mliu's review comments on the multi-site login PR. Cache (portal) [#4, #6, #8]: replace the directoryCache RWMutex with an atomic.Pointer copy-on-write snapshot — reads (Get/Ready) are lock-free; the daily HR refresh builds a fresh map and swaps it in with one atomic store. Duplicate accounts are skipped with a warn log (first occurrence wins) instead of rejecting the whole snapshot. Site URLs (portal) [#2, #7]: replace the single {siteId} auth-URL template with PORTAL_SITE_URLS, a registry mapping each siteId to {authServiceUrl, baseUrl}, validated at startup. A single template can't form correct URLs for sites on different domains. The userInfo lookup now also returns baseUrl — the site's own origin, a distinct URL, not the auth-service URL. Access gate [#1, #3, #5]: remove the MongoDB provisioning gate from auth-service (no Mongo dependency there) and move the check to the portal, where the directory cache already lives. On each /userInfo lookup the portal confirms the account exists in BOTH hr_employee (home site) and the users collection (the canonical user record every other service reads) — an existence check that projects _id only. account_not_provisioned collapses into account_not_ready. Updates the errcode registry, the frontend reason->copy map and catalog, and client-api.md. https://claude.ai/code/session_01P8NukFmmxT1tD2ZSyQ5W9Y
Addresses mliu's follow-up: instead of a per-request users existence check,
intersect hr_employee with users inside ListEmployees via a $lookup keyed on
{account, siteId}, projecting users._id as userId. The in-memory cache now
holds only provisioned users, so a cache hit already means "employee AND
provisioned" and the portal makes no per-request DB call — every login is
served from memory.
- ListEmployees: Find -> aggregate with $lookup + $match (intersection) +
$project (userId)
- employee gains UserID (kept in memory; not returned on the wire)
- remove provisionChecker / AccountProvisioned and the per-request resolve check
- refresh interval default 24h -> 2h so newly provisioned users appear sooner
- dev seed adds a matching users row so the local $lookup resolves
$lookup cannot cross databases, so users must live in the same MongoDB
database as hr_employee (the portal's MONGO_DB).
https://claude.ai/code/session_01P8NukFmmxT1tD2ZSyQ5W9Y
vjauhari-work
force-pushed
the
claude/eager-einstein-7u2je6
branch
from
35b13d7 to
8434aed
Compare
Joey0538
pushed a commit
that referenced
this pull request
Jun 17, 2026
…ombined)
Single-file design spec for password auth + durable sessions for bots
and admins, migrating from legacy Rocket.Chat to the nextgen NATS-
native stack with zero bot code changes. Three parts under H1 dividers:
Part I = requirements & architecture, Part II = technical design,
Part III = components & integration. Lives under docs/specs/botplatform/
namespace (folder added in the README+runbook commit).
Architecture DECIDED 2026-06-15: Option B / DEDICATED-SERVICE (new
botplatform-service) over EXTEND-AUTH. Bot edge grew into a browser-
facing web app + dual-token validation; isolating from the JWT-signing,
human-SSO auth-service wins on blast-radius/key safety + independent
scaling.
Token format DECIDED 2026-06-16 (Q10): native v1 = bp1_<43-char
base64url of 32 random bytes>, stored base64(HMAC-SHA-256(
TOKEN_HMAC_KEY, token)). Legacy tokens unchanged for byte-for-byte
compatibility. Validator dispatches on bp1_ prefix.
Bot account namespace fix DECIDED 2026-06-16: bots scope to
chat.bot.{botToken}.>; xxx.bot -> xxx_bot via subject.BotAccountToken.
Provisioning gate (REQUIRE_PROVISIONED default true) mirrors auth-
service from PR #295. Cross-cluster cutover: chat gateway (fz2) =
PERMANENT front door — DNS bound per-namespace.
§9.8 /v1/auth/validate returns principal.class ∈ {bot, user, admin} —
authoritative source for ApiGW / WS / EventConsumer and for the
traffic-isolation spec.
CodeRabbit fixes folded in:
• §5.1 step 5: native-bot JWT minting clarified — botplatform-service
NEVER signs (Option B keeps signing key in auth-service); native
JWT minting deferred from July scope.
• §14 algorithms pseudocode: replaced misleading nats.Request(
auth.session.validate, h) — botplatform-service exposes NO NATS
surface; rewrote as in-process sessionStore.Lookup with comment
pointing at POST /v1/auth/validate for downstream consumers.
• Part III §1: removed "API proxy" from service summary
(contradicted Part III §4.1 — we are NOT in /api/v2/* data path).
• MD040: 7 unlabeled code fences got text/go language tags.
• MD058: blank line added before Part III §4.3 table.
Cross-spec citations use full filenames (e.g. "auth.md Part II §9.8");
within-doc "Part 1/2/3" normalized to "Part I/II/III" to match H1
dividers.
User stories US1-US8: bot login, session API access, long-lived
sessions, admin bot creation, password rotation, session management,
web login, web change-password.
§10 Diagrams embeds 4 PNG previews of the .drawio files under
botplatform/diagrams/ (login old-vs-new, token gen+validate, bot
login end-to-end, cross-cluster cutover).
Stale "(recommended)" tag on EXTEND-AUTH heading removed; duplicate
§10.2 deleted; DNS-repoint alternative removed (verified infeasible).
Filename: auth.md (was bot-platform-nextgen-auth.md; folder
namespacing makes the prefix redundant).
Joey0538
pushed a commit
that referenced
this pull request
Jun 17, 2026
…ombined)
Single-file design spec for password auth + durable sessions for bots
and admins, migrating from legacy Rocket.Chat to the nextgen NATS-
native stack with zero bot code changes. Three parts under H1 dividers
(Part I requirements & architecture; Part II technical design;
Part III components & integration). Lives under docs/specs/botplatform/.
Architecture DECIDED 2026-06-15: Option B / DEDICATED-SERVICE.
Token format DECIDED 2026-06-16: bp1_ prefix + HMAC-SHA-256 storage.
Bot account namespace fix DECIDED 2026-06-16: chat.bot.{botToken}.>.
Provisioning gate (REQUIRE_PROVISIONED) mirrors PR #295.
Cross-cluster cutover: chat-GW (fz2) is the PERMANENT front door.
§9.8 /v1/auth/validate returns principal.class ∈ {bot, user, admin}.
CodeRabbit fixes folded in (across two review rounds):
Round 1:
• §5.1 step 5: native-bot JWT minting clarified — DEFERRED;
delegates to auth-service when wired.
• §14 algorithms pseudocode: replaced misleading nats.Request(
auth.session.validate, h) with in-process sessionStore.Lookup.
• Part III §1: removed "API proxy" from service summary.
• MD040: 7 unlabeled code fences got text/go language tags.
• MD058: blank line added before Part III §4.3 table.
Round 2:
• §3a interfaces table: /v1/auth/validate response row updated from
{valid,account,userId} to {valid,principal} where principal carries
{userId,account,username,roles,class,siteId}. Was contradicting
§9.8's canonical contract and would have led ApiGW/WS integrations
to consume the wrong shape.
• Part III §1 botplatform-server description: removed "We proxy to
it after auth, transparently" — contradicted Part III §4.1.
Replaced with explicit "ApiGW (not us) routes /api/v2/* to
botplatform-server under mTLS, with X-User-Id / X-Account /
X-Principal-Class injected from our validate response."
• Part III §3 admin endpoints: relabeled /v1/admin/bots… (REST API)
to /admin/bots… (role-gated web handlers) — was contradicting
Part II §8 (admin = HTML form posts on /admin/...) and Q15/Q18
(no separate JSON admin API).
Cross-spec citations use full filenames; within-doc Part 1/2/3
normalized to Part I/II/III to match H1 dividers.
Filename: auth.md (was bot-platform-nextgen-auth.md; folder
namespacing makes the prefix redundant).
Joey0538
pushed a commit
that referenced
this pull request
Jun 17, 2026
…ombined)
Single-file design spec for password auth + durable sessions for bots
and admins, migrating from legacy Rocket.Chat to the nextgen NATS-
native stack with zero bot code changes. Three parts under H1 dividers.
Architecture DECIDED 2026-06-15: Option B / DEDICATED-SERVICE.
Token format DECIDED 2026-06-16: bp1_ prefix + HMAC-SHA-256 storage.
Bot account namespace fix DECIDED 2026-06-16: chat.bot.{botToken}.>.
Provisioning gate (REQUIRE_PROVISIONED) mirrors PR #295.
Cross-cluster cutover: chat-GW (fz2) is the PERMANENT front door.
§9.8 /v1/auth/validate returns principal.class ∈ {bot, user, admin}.
CodeRabbit fixes folded in (across two review rounds):
Round 1:
• §5.1 step 5: native-bot JWT minting DEFERRED; delegates to
auth-service when wired.
• §14 algorithms: replaced misleading nats.Request with in-process
sessionStore.Lookup.
• Part III §1: removed "API proxy" from service summary.
• MD040: code fences got language tags.
• MD058: blank line added before Part III §4.3 table.
Round 2:
• §3a interfaces table: validate row updated from {valid,account,
userId} to {valid,principal} with full principal struct + class
enum — matches §9.8's canonical contract.
• Part III §1 botplatform-server: removed "we proxy after auth";
replaced with explicit "ApiGW (not us) routes /api/v2/* with
injected X-Principal-Class under mTLS".
• Part III §3 admin endpoints: /v1/admin/bots… → /admin/bots…
(role-gated web handlers, not JSON API per Q15/Q18).
Scope clarification added:
• Part I §11 "Out of scope": new bullet on general user
administration (list/search/role-change/deactivate ALL users —
humans + bots). NOT in this spec. The /admin/bots… surface here
is bot-specific (create/rotate/revoke for a bot account); general
user admin is a separate spec because (a) it writes to the shared
users collection owned by PR #295 / portal-service team (we only
read for the provisioning gate), (b) it introduces site-admin
permission boundaries the bot-only admin surface doesn't need,
and (c) it has a much bigger UX surface (search, bulk ops, audit).
Likely follow-up home: docs/specs/botplatform/admin-user-
management.md or docs/specs/portal/… depending on ownership.
Removed the misleading "Admin UI (next phase)" bullet that previously
sat in §11 — the actual admin web UI (for bot management) IS in this
phase; what's out of scope is GENERAL user administration, which the
new bullet spells out unambiguously.
Cross-spec citations use full filenames; within-doc Part 1/2/3
normalized to Part I/II/III.
Filename: auth.md (was bot-platform-nextgen-auth.md).
Joey0538
pushed a commit
that referenced
this pull request
Jun 24, 2026
…isolation + runbook + diagrams + Claude plugin)
Initial design-record drop for the bot-platform-nextgen migration —
password auth + durable sessions for bots/admins, cross-cluster
cutover behind the chat-gateway, and a companion bot-traffic
isolation spec.
Layout (all under docs/specs/botplatform/):
* README.md — spec-set index + reading order + citation convention
* auth.md — main design spec, combined Parts I (architecture &
requirements), II (technical design), III (components &
integration). Covers the architecture decision (Option B /
DEDICATED-SERVICE), data model, hashing/verify algorithms, NATS
subjects, gateway topology, performance design, configuration,
test plan, verification checklist, open questions, and bot/admin
integration with ApiGW / WebSocket / EventConsumer.
* traffic-isolation.md — companion spec for routing bot traffic to
separate worker pools (Option A / SUBJECT-SPLIT decision). Covers
principal.class propagation, NATS subject namespaces, JetStream
stream design, per-service deployment split plan, supercluster
permissions, observability, and rollout phases.
* migration-runbook.md — operational AS-IS vs TO-BE runbook for the
Mongo collection deltas (users + sessions), per-site cutover
ordering, reconciliation queries, rollback steps.
* diagrams/ — 4 editable .drawio sources + rendered PNGs (embedded
inline in auth.md §10): login-old-vs-new, token-gen-validate-flow,
bot-login-flow, cross-cluster-cutover.
Key decisions baked in:
* Auth architecture: Option B / DEDICATED-SERVICE — new
botplatform-service (decided 2026-06-15 in design review). Bot
edge is a browser-facing web app + dual-token validation; isolating
from auth-service's JWT signing wins on blast-radius/key safety.
* Token format: native v1 = bp_<43-char base64url of 32 random bytes>,
stored as base64(HMAC-SHA-256(TOKEN_HMAC_KEY, token)). Legacy
unchanged for byte-for-byte compatibility. Validator dispatches on
the bp_ prefix.
* Bot account namespace: bots scope to chat.bot.{account}.>;
xxx.bot → xxx normalization via subject.BotSubjectName. Resolves
PR #295 strict-validator conflict + eliminates ACL escape between
human xxx and bot xxx.bot.
* Provisioning gate: REQUIRE_PROVISIONED=true default; mirrors
auth-service from PR #295. Bots can only log in at their home site.
* Cross-cluster cutover: chat-GW (fz2) is the PERMANENT front door
(DNS bound per-namespace, DNS-repoint infeasible). Weight-split
cross-cluster to wsp-GW.
* Traffic isolation: Option A / SUBJECT-SPLIT. message-worker +
broadcast-worker split per-class first; message-gatekeeper stays
shared through Phase 4.
Tooling:
* .claude/settings.json — adds the Agents365-ai/365-skills marketplace
(SHA-pinned for reproducibility) and enables the drawio plugin
team-wide. Used to generate the four spec diagrams; anyone working
in this repo gets the diagram-rendering tooling for free.
Joey0538
added a commit
that referenced
this pull request
Jun 29, 2026
…migration runbook (#334) * docs: bot-platform nextgen migration — full spec set (auth + traffic-isolation + runbook + diagrams + Claude plugin) Initial design-record drop for the bot-platform-nextgen migration — password auth + durable sessions for bots/admins, cross-cluster cutover behind the chat-gateway, and a companion bot-traffic isolation spec. Layout (all under docs/specs/botplatform/): * README.md — spec-set index + reading order + citation convention * auth.md — main design spec, combined Parts I (architecture & requirements), II (technical design), III (components & integration). Covers the architecture decision (Option B / DEDICATED-SERVICE), data model, hashing/verify algorithms, NATS subjects, gateway topology, performance design, configuration, test plan, verification checklist, open questions, and bot/admin integration with ApiGW / WebSocket / EventConsumer. * traffic-isolation.md — companion spec for routing bot traffic to separate worker pools (Option A / SUBJECT-SPLIT decision). Covers principal.class propagation, NATS subject namespaces, JetStream stream design, per-service deployment split plan, supercluster permissions, observability, and rollout phases. * migration-runbook.md — operational AS-IS vs TO-BE runbook for the Mongo collection deltas (users + sessions), per-site cutover ordering, reconciliation queries, rollback steps. * diagrams/ — 4 editable .drawio sources + rendered PNGs (embedded inline in auth.md §10): login-old-vs-new, token-gen-validate-flow, bot-login-flow, cross-cluster-cutover. Key decisions baked in: * Auth architecture: Option B / DEDICATED-SERVICE — new botplatform-service (decided 2026-06-15 in design review). Bot edge is a browser-facing web app + dual-token validation; isolating from auth-service's JWT signing wins on blast-radius/key safety. * Token format: native v1 = bp_<43-char base64url of 32 random bytes>, stored as base64(HMAC-SHA-256(TOKEN_HMAC_KEY, token)). Legacy unchanged for byte-for-byte compatibility. Validator dispatches on the bp_ prefix. * Bot account namespace: bots scope to chat.bot.{account}.>; xxx.bot → xxx normalization via subject.BotSubjectName. Resolves PR #295 strict-validator conflict + eliminates ACL escape between human xxx and bot xxx.bot. * Provisioning gate: REQUIRE_PROVISIONED=true default; mirrors auth-service from PR #295. Bots can only log in at their home site. * Cross-cluster cutover: chat-GW (fz2) is the PERMANENT front door (DNS bound per-namespace, DNS-repoint infeasible). Weight-split cross-cluster to wsp-GW. * Traffic isolation: Option A / SUBJECT-SPLIT. message-worker + broadcast-worker split per-class first; message-gatekeeper stays shared through Phase 4. Tooling: * .claude/settings.json — adds the Agents365-ai/365-skills marketplace (SHA-pinned for reproducibility) and enables the drawio plugin team-wide. Used to generate the four spec diagrams; anyone working in this repo gets the diagram-rendering tooling for free. * docs(auth): clarify transport choice for /v1/auth/validate + bridge shape (Q13/Q15) Two clarifications that surfaced during PR review discussion. Both are spec-additive — they document decisions already implied by the rest of the spec but not pinned anywhere, preventing two predictable re-litigation rounds during the internal team's build. §15.1 (NEW subsection) — "Transport choice: why HTTP, not NATS RPC" Reviewer FAQ. CLAUDE.md §6 says "use NATS request/reply for synchronous operations" — so why is /v1/auth/validate HTTP? Both transports are synchronous; the choice is operational fit. We pick HTTP because (1) the service inherently serves a web UI (HTML + CSRF + cookies), forcing HTTP anyway; (2) ApiGW is Envoy-native and HTTP routing is its native skill; (3) in-mesh HTTP hop (~1ms) is well inside §9.5's <5ms SLA. Reversible later if production data justifies adding a NATS-RPC sidecar. Explicitly states sync-vs-async is NOT the differentiator here (both HTTP and NATS request/reply are synchronous request/reply). Pub/sub and JetStream would be wrong shapes (no reply value), but NATS request/reply would have worked — just at higher operational cost. Q13 (extended) — "Bridge shape: synchronous NATS request/reply, NOT pub/sub or JetStream" Q13 already says the bridge lives in botplatform-server / data- plane track, but didn't pin the SHAPE. An implementer could read it and pick pub/sub or JetStream, silently breaking the bot's HTTP response expectation. Pins the shape: HTTP request → nc.Request(subj, payload, timeout) → reply → HTTP response Documents the helpers (pkg/subject, pkg/natsrouter, pkg/natsutil, pkg/errcode + errnats.Reply) and the timeout pyramid (bot 30s > ApiGW 25s > bridge 20s > nc.Request 5-10s) so chained-sync-hop timeouts don't cascade into stuck 504s. Calls out the wrong shapes explicitly (pub/sub, JetStream) so the "wrong tool" mistake is hard to make. Also fixed: §15's admin route reference (/v1/admin/bots… → /admin/bots…) to match the round-2 CodeRabbit fix in Part III §3. The route table itself was already updated in Part I §3a + Part III §3; §15 had one straggler. Also: added `account_not_provisioned` to §15's error reasons list — mirrors the provisioning gate from Part I §5 + Part II §5.1. * docs(auth): mixed-storage model (password on users, sessions in own collection) + admin split (admin-portal + admin-service) Two intertwined pivots merged with the prior cap+permanent-sessions work into one commit, both driven by user/ops feedback: 1. MIXED-STORAGE MODEL (corrects the prior "collapse everything onto users" pivot). Passwords stay on users.services.password.bcrypt in-place (no credentials collection — the credential side of the 2026-06-24 collapse stands). Sessions go BACK to a dedicated `sessions` collection, one doc per session keyed by token hash: - O(1) validate via sessions.findOne({_id: hash}) — no in-doc array scan, no multikey index needed for the hot path - Bounded users-doc size — sessions don't bloat the identity doc that every nextgen service caches via pkg/userstore - Per-session revocation primitive — admin revoke is one deleteOne; revoke-all is one deleteMany({userId}) - Cap enforcement reverts from atomic $push+$slice (which only works on an in-doc array) to count + delete-oldest-by-issuedAt at login. Race tolerance: brief cap+1/cap+2 overshoot heals on the next login; no Mongo transactions needed - Index requirement: {userId: 1, issuedAt: 1} compound — IXSCAN serves both the cap-eviction victim lookup AND admin's list-sessions-by-user query - schemaVersion field intentionally absent (PR feedback) — skip until we actually need it 2. ADMIN SPLIT (reverses Q18). Admin becomes its own portal + service, distinct from botplatform-service: - botplatform-service narrows to login + validate + changepwd (universal HTML pages serving both chat and admin portals). No more /admin/* HTML routes here. - admin-service (NEW) — REST JSON APIs for all admin operations: list/create/suspend bots, rotate password, revoke single/all sessions, future rate-limit config. Every route requires class:"admin" verified via /v1/auth/validate against the inbound session cookie. - admin-portal (NEW) — static web frontend hosted at admin-{site}.…; calls admin-service via XHR under the admin's session cookie. - /dev-login stays the universal login form on botplatform-service. Post-login redirect target depends on the calling portal (?next= / Referer) — same admin user can land on chat frontend (via chat.xxx.com/dev-login) or admin portal (via admin-{site}.…/dev-login). - Q18 reversed: previous "no separate admin API" stance was wrong for the actual ops UX needs (distinct deploy cadence, isolated authz boundary, distinct frontend). Carryover from the previous combined pivot (still applies): - Permanent sessions (no expiresAt, no lastUsedAt, no TTL reap) - Pure-read validate (Valkey GETEX refreshes in-memory TTL; cache miss does sessions.findOne; no Mongo writes on the hot path) - FIFO eviction by issuedAt at login (cap-evicted entries valkey.del'd explicitly, not left to age out) - Dual-token hash dispatch by bp1_ prefix (legacy SHA-256 vs v1 HMAC-SHA-256) - sessionExpired error reason removed - SESSION_IDLE_WINDOW / SESSION_LASTUSED_THROTTLE env vars removed Migration impact (migration-runbook.md): - Passwords: no extraction needed — identity-sync already carries services.password.bcrypt end-to-end - Sessions: ONE bulk-import job at cutover from legacy users.services.resume.loginTokens[] → new sessions collection (skip PATs; verbatim hashedToken → _id; scheme:"legacy"). After cutover, legacy continues to write the legacy array; nextgen never re-reads it (canary monotonically shifts forward; bot re-login on miss is cheap) - Cutover ordering simplified to 6 steps; added admin-service + admin-portal as step 4 (dark deploy before canary) - schemaVersion intentionally omitted from sessions row schema (PR feedback) Topology clarifications (auth.md §10.1): - Gateway host claims and DNS: chat-gw and wsp-gw can both claim *.chat.fx.abc.com in their server blocks; DNS (not Istio config) decides routing — point *.chat.fx.abc.com only at chat-gw's external IP. wsp-gw's claim is only for cross-cluster forwarded traffic from chat-gw. - Deploy nextgen pods in fz1 only during the ramp: botplatform-service.wsp.svc has zero endpoints in fz2, so the mesh resolves cross-cluster to fz1. If you ever deploy in both clusters, Istio's default locality-aware LB would prefer same-cluster pods — override via DestinationRule's localityLbSetting or keep fz1-only. Diagrams (re-rendered): - bot-login-flow.drawio: step 13 reverted to sessions.insertOne; step 13b cap-eviction is count + delete oldest by issuedAt (not $slice). - login-old-vs-new.drawio: NEW panel step 5 reverted to dedicated sessions collection + count/delete-oldest cap. Wins panel updated to reflect O(1) validate via sessions.findOne, bounded users-doc, dedicated collection benefits. Compat panel: passwords stay on users, sessions are bulk-imported once at cutover. - token-gen-validate-flow.drawio: g_persist reverted to sessions.insertOne; v_mongo cold path is sessions.findOne({_id:hash}) — O(1), single IXSCAN. * docs(auth): scrub NATS-JWT mint from botplatform-service; move to auth-service POST /auth kind discriminator Addresses mliu33's PR-review comments #3 (login should not return a NATS JWT — only authToken + userId) and #4 (Bot SDK is REST-only, existing behavior). Architectural pivot, not a feature change: * botplatform-service NEVER mints NATS JWTs and NEVER holds the signing key. The login endpoint (/api/v1/login, /v1/bot/login, /dev-login) returns ONLY {authToken, userId, me}. The natsPublicKey request field is gone, the optional jwt response field is gone, the §3 Key Decisions "NATS JWT" row is restated. * auth-service stays the single NATS-JWT minter for the whole system, with a unified 3-field POST /auth body: { kind, token, natsPublicKey } - kind:"sso" → token = OIDC bearer (existing humans). - kind:"bot" → token = bp_ session token; auth-service calls botplatform-service /v1/auth/validate to verify before minting. No separate userId field — the session token self-identifies; /v1/auth/validate returns the full principal. Migration: just update the existing endpoint in place — no new route, no compat shape, the endpoint is still under active development (no live external callers to coast for). * Response envelope {natsJwt, user} is identical across kinds. user.account always present; SSO populates the full HR profile (email, employeeId, engName, chineseName, deptName, deptId); bot populates account only (others omit-empty). Frontend reads user.account universally. * The §5.2 primitive previously phrased as "internal session→JWT exchange on botplatform-service" is recast as the auth-service kind:"bot" extension. It exists ONLY for flow B (chat-frontend using a bot account); flow A (pure REST bot SDK pods, the 99% case) never invokes it. Spec edits (auth.md): * NEW §3a — explicit Flow A vs Flow B table at the top of Part II so the rest of the spec reads against a clear consumer model. * §3 Key Decisions — "NATS JWT" row rewritten: minted by auth-service only, never by login, never by botplatform-service. * §5.1 step 6 — login response is {authToken, userId, me} only; removed the deferred natsPublicKey delegation step. * §5.2 — completely rewritten: auth-service POST /auth extension shape (kind + token + natsPublicKey), bot-kind dispatch flow (validate → mint chat.bot.{token}.> grant), unified response envelope with field-presence notes. Caller always knows kind because the caller knows which login it just performed — no server-side guessing. * §5.7 — "Resume / session reuse" reframed: flow B primitive lives on auth-service; Q1b session.refresh routes through the same auth-service extension if/when a native bot SDK lands. * §7 Option B — "signing key stays in auth-service; botplatform-service never mints" tightened. * §9.2 topology comment — auth-service calls back IN to botplatform-service /v1/auth/validate for kind:"bot" mints (not the other direction). * §13 Go types — drop NATSKey field from loginRequest; drop optional jwt field from loginResponse. * §15 transport note — botplatform-service has no NATS surface and no AUTH_SERVICE_URL outbound; the JWT-mint call is the OTHER direction (auth-service → botplatform-service). * §16 config — explicit NO AUTH_SERVICE_URL on botplatform-service. * §20 client-api delta — login request: drop natsPublicKey. * §12 Q1b + Q5 — restated to reflect the auth-service ownership of the bot-kind mint. Original ticket scope (§3 Part I context "Signing key stays put") sharpened to: botplatform-service has zero JWT involvement. * docs(auth): rename bp1_ → bp_, add ad_ for admin tokens, expand /auth kind to sso/bot/admin Unversioned per-class prefixes — class is self-evident from X-Auth-Token without a DB lookup: * bp_<43-char base64url> = bot session token * ad_<43-char base64url> = admin session token * legacy unprefixed = imported RC SHA-256 tokens (unchanged) No version digit (bp_ not bp1_) — login mechanism isn't planned to rotate; YAGNI. §3 Key Decisions table updated: * Token wire format row: per-class prefixes, no version * NATS JWT row: kind ∈ {sso, bot, admin}; admin → chat.> god-mode (decided 2026-06-24), bot → chat.bot.{token}.>, sso → chat.user.{account}.>. For kind:"bot" and kind:"admin", auth-service calls back into botplatform-service /v1/auth/validate before minting. Bulk rename across the spec — every bp1_ reference updated. Followup commits will add IsAdminAccount helper, the validator dispatch table, and the §5.2 kind:"admin" branch. * docs(auth+traffic-isolation): rename chat.bot.{botToken}→chat.bot.{account} (strip .bot); add IsAdminAccount + class:admin Cross-spec naming cleanup applied to both auth.md and traffic-isolation.md. (1) Subject parameter rename: chat.bot.{botToken}.> → chat.bot.{account}.> The old "botToken" name overloaded with the auth credential (bp_…); the value was always the bot's account stripped of the .bot suffix. Rename to {account} for clarity. Also drops the dot-normalization ("xxx.bot"→"xxx_bot") in favor of plain suffix-strip ("xxx.bot"→"xxx") — the chat.bot.> namespace already encodes the class, so .bot becomes redundant inside it. Helpers (auth.md §4.7): BotSubjectName(account) "alice.bot" → "alice" BotAccountFromSubjectName(name) "alice" → "alice.bot" Replaces BotAccountToken (which did dot-normalization). (2) Admin account classification (auth.md §4.7 new subsection): model.IsAdminAccount(account) — accepts the legacy p_xxx prefix (e.g. "p_jeff"). Admin login path validates user with IsAdminAccount; /v1/auth/validate returns class:"admin" when roles ∋ "admin" (roles are authoritative, the helper is a login-side sanity gate). Cross-spec touches in auth.md: * §3 Key Decisions NATS JWT row: kind:"bot" mints chat.bot.{stripped}.>, kind:"admin" mints chat.> god-mode. * Part I §3a flow tables: stripped-account form. * §4.7 Layer 2 rewritten with BotSubjectName, plus a new "Admin account namespace" subsection. * §4.7 result table: now shows alice / alice.bot disjointness, plus the admin row (n/a / chat.>). * §4.7 validator dispatch: three validators (human SSO / bot login / admin login), each with its own login-path use. * §5.2 grant-mint step: scope selected from principal.class. Cross-spec touches in traffic-isolation.md (sync with auth.md): * §4.2 sample helper code: BotAccountToken → BotSubjectName, dot- normalization → suffix-strip. * §4.3 mirror table: bot subjects use {account} stripped form; admin row added to JWT-grant list (chat.> god-mode). * §4.3 canonical row nudged to reflect Phase 1 shared canonical (full pros/cons discussion lands in the follow-up commit). * Cross-spec invariant updated to point at auth.md §4.7 (the new section number). * docs(traffic-isolation): add §4.4a — shared vs split canonical streams (Phase 1 = shared) Phase 1 decision: keep MESSAGES_CANONICAL_{siteID} and ROOMS_{siteID} shared across classes. Publisher namespace stays split per class (chat.user vs chat.bot) — that's enforced by JWT grants and resolves the ACL-escape problem. The downstream CANONICAL stream split is deferred until measured noisy-neighbor pressure justifies it. §4.4a captures the full trade-off so the team can revisit with data: * PROS of SHARED (Phase 1): simpler infra (2 streams not 4), single dashboards/alerts, single-Deployment workers, half the K8s objects, trivial mixed-room handling, simpler federation, easier ordering invariants, no canonical-subject rename needed. * CONS of SHARED (the watch-list): no independent worker-pool scaling per class, shared failure domain (poison-pill bot msg halts user processing on the same pod), noisy-neighbor risk, metric/SLO granularity is label-based rather than stream-based, cross-class rate limiting lives in app code, HPA precision is coarser per-class. * INVARIANTS preserved either way: publisher ACL isolation, subject collision safety, per-class metric tagging (via class field on payload), mixed-room correctness, federation correctness. * SPLIT-LATER ESCAPE HATCH: documented triggers + the mechanic (add chat.bot.canonical.… stream + rename existing to chat.user.canonical.… via JetStream Mirror; non-breaking because publisher namespace is already split). * WHEN TO REVISIT: 30-day prod review; specific operational pain; adding 6th+ canonical consumer. Also nudges §4.3 mirror-table footnote + §4.4 bullet to point at §4.4a as the source of truth for the canonical-split question. * docs(traffic-isolation): §5.0 bot publish = core NATS R/R + defer per-class worker split per §4.4a Two related Phase-1 changes bundled together because they're a single coherent stance ("Phase 1 = shared canonical stream, no per-class split yet, bot publish stays sync-fail-fast"): (1) §5.0 NEW — Bot publish edge = core NATS R/R * Bots use REST end-to-end from the SDK's POV; bp-api translates to core NATS request/reply (NOT JetStream js.Publish) to talk to message-gatekeeper. Preserves legacy v2 REST fail-fast semantics. * Per-RPC transport table: - Query / lookup → core NATS R/R - Mutation w/ single sync response → core NATS R/R - Message publish (fan-out required) → core NATS R/R to gatekeeper; gatekeeper synchronously js.Publish to MESSAGES_CANONICAL_{siteID} and waits for PubAck before replying to bot. * NO MESSAGES_BOT_{siteID} JetStream submit stream. The user-side MESSAGES_{siteID} stream stays user-only; bots never publish into JetStream at all. * Failure modes spelled out (gatekeeper unreachable / validation reject / canonical PubAck fail). * User publish vs bot publish comparison table. * Implementation impact on message-gatekeeper: existing JS pull- consumer on MESSAGES_{siteID} unchanged + NEW core-NATS queue- subscribe on chat.bot.*.room.*.{siteID}.msg.> for bot R/R path. (2) §5.1 / §5.2 / §5.3 / §5.4 / §6 — Defer per-class worker split * §5.1 message-gatekeeper: Phase 1 publishes to shared MESSAGES_CANONICAL_{siteID} (single stream) instead of class-split canonical. Per-class subject variant (chat.user.canonical / chat.bot.canonical) deferred per §4.4a escape hatch. * §5.2 message-worker: Phase 1 = single shared Deployment on the shared canonical stream. Per-class -user/-bot deployment table preserved as "Phase N+ when split" reference for the escape-hatch implementation. * §5.3 broadcast-worker: same treatment as §5.2. * §5.4 worker binary changes: Phase 1 = nothing; class-aware WORKER_CLASS env / filter code activates only when split triggers. * §6 stream design: existing MESSAGES_CANONICAL_{siteID} kept as-is in Phase 1; no rename, no per-class split. Original "separate streams per class" rationale preserved as §6.1.1 future-design reference for the escape-hatch implementation. The Option A / SUBJECT-SPLIT architecture decision still stands at the PUBLISHER namespace level (JWT grants enforce chat.user.> vs chat.bot.> vs admin chat.>). Phase 1 just defers the DOWNSTREAM stream / worker split as YAGNI until measured noisy-neighbor pressure justifies the operational overhead — per §4.4a. * docs(auth): clarify provisioning gate is in-process field check, not a separate Mongo query The §5.1 step 3 "provisioning gate" was being drawn and described as if it were a second Mongo round-trip after the password verify. It's not — the users doc loaded in step 2 already carries siteId, so the gate is just: if cfg.RequireProvisioned && u.SiteID != cfg.SiteID { return errcode.Forbidden("account_not_provisioned") } Zero extra DB calls, zero extra latency on the login hot path. The gate is a distinct LOGICAL security control (validates site affinity, separate concern from credential check), but operationally it's an O(1) field comparison on a struct already in memory. Spec edits: (1) auth.md §5.1 step 3 — rewritten to say "in-process field check, NO extra Mongo query" explicitly. Shows the actual Go pattern. (2) auth.md §1.5 (Critical Constraints) — earlier wording was "the account's {userId, siteId} must exist in this site's users collection", which read like a separate findOne. Replaced with "the siteId field on the users doc (already loaded for the password check) must match SITE_ID". Diagram edits (all three login-flow diagrams): (3) bot-login-flow.drawio — step 10/11 were drawn as a Mongo arrow + reply (PROVISIONING GATE — find {account, siteId} in users + found ✓). Replaced with a single red in-process activation box on the botplatform-service lifeline ("PROVISIONING GATE (in-process, no extra Mongo) — if u.siteId != SITE_ID → 403 account_not_provisioned"). Side-panel annotation and legend updated accordingly. PNG re-rendered. (4) login-old-vs-new.drawio — NEW panel had a dedicated orange "Step 3 PROVISIONING GATE" box drawn as if it were a separate operation. Removed it entirely and folded the field check into Step 2's body ("if u.siteId != SITE_ID → 403 — field check on the doc above, no extra Mongo query"). Steps 4/5/6 renumbered to 3/4/5; the new-side column now has 5 steps, symmetric with the legacy side. Token prefix also corrected bp1_ → bp_ (per earlier rename in commit 0c47c7e; this diagram lagged). PNG re-rendered. (5) cross-cluster-cutover.drawio — Mongo cylinder caption said "credentials + sessions + users / (provisioning gate reads users)" — both stale and ambiguous (no credentials collection after 2026-06-24 pivot; "reads users" implied a separate query). Rewritten to "users (shared, password in-place) + sessions (new, one doc per session) / Provisioning gate = in-process field check on the user doc loaded for password verify (no extra Mongo round-trip)". PNG re-rendered. No semantic change to the gate's behavior or REQUIRE_PROVISIONED default — just making explicit that it doesn't cost a round-trip, correcting all three diagrams, and tightening the §1.5 wording. * docs(diagrams): sync token-prefix rename across all login-flow diagrams (bp1_ → bp_; drop bp2_) Diagram catch-up for the C5 token-prefix decision (commit 0c47c7e) which renamed bp1_ → bp_ in auth.md text but the diagrams lagged. Also drops the bp2_ forward-versioning reference per C5's "unversioned prefix; we won't rotate the login mechanism, YAGNI" stance. Files touched (all PNGs re-rendered): * bot-login-flow.drawio — 4 spots: subtitle, step 12 token-issue box, step 15 response label, side-panel annotation (12). bp1_ → bp_ throughout. * token-gen-validate-flow.drawio — 5 spots: g_prefix box, g_response, g_coexist_body, va_v1 edge label, va_legacy_fb edge label. bp1_ → bp_ throughout. The g_coexist_body rationale bullet ("forward versioning bp2_…") is rewritten to reflect the per-class prefix decision instead: "class is self-evident from X-Auth-Token (bp_ = bot, ad_ = admin) — no DB lookup needed to know principal class". * login-old-vs-new.drawio — 2 stragglers (subtitle + n_wins_body bullet) the earlier bulk-rename missed because the reverted-by-linter dance landed bp1_ back into those strings. The wins-bullet "Versioned wire format (bp1_)" is also rewritten to "Per-class wire prefix (bp_ for bot, ad_ for admin)" to match the C5 rationale. No semantic change beyond what C5 already decided in the spec text; this just brings the diagrams into alignment so reviewers don't see bp1_ in pictures while the spec text says bp_. --------- Co-authored-by: Claude <noreply@anthropic.com>
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
3 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.
What
Login is now a three-step sequence: portal lookup → auth → NATS connect, and JWT minting is gated on per-site provisioning.
POST /lookupvalidates the SSO token and resolves the account's home site (auth URL, NATS URL, siteId) from the ops-owned portal directory (users+sitescollections in the portal DB). OIDC in prod; dev mode accepts a raw account with a configurable fallback site.REQUIRE_PROVISIONED=true(default), an{account, siteId}document must exist in the site'suserscollection before a NATS user JWT is signed — otherwise403 account_not_provisioned. Store errors fail closed; disabling the gate logs a loud warning.MONGO_URI/SITE_IDare validated at startup when the gate is active.Claims.Account()trusts onlypreferred_username(nameis user-editable display data). Accounts are validated against the routing layer's token invariant via the newsubject.IsValidAccountToken(non-empty; no./*/>/whitespace/control) at both portal and auth before any account reacheschat.user.{account}.>permissions.pkg/ginutil(new): request-ID, access-log and CORS Gin middleware extracted from auth-service, now shared by both HTTP services.connect()resolves the site via the portal, then auths and connects with the resolved URLs; JWT refresh re-mints against the late-bound auth URL; errcode envelopes surfacereason/codeto the UI (account_not_provisioned→ "contact your administrator" copy).client-api.md— §2.1 login sequence, §2.2 gate errors + account rule, new §2.3 portal lookup spec (upload endpoints renumbered to §2.4), §6 registry.Why
Multi-site federation needs per-user site discovery at login instead of static client config, and the JWT minter is the right authority to refuse unprovisioned accounts before they gain NATS permissions.
Testing
pkg/oidc,pkg/ginutil,pkg/subjecttable tests.make lint0 issues ·make test57 packages · frontend 634 tests + typecheck clean.Config notes for rollout
preferred_username(frontend requestsopenid profile email) and that no production accounts contain./@-style names that violate the token rule (dots were never routable).REQUIRE_PROVISIONED=falseto stage the gate before the directory is fully populated; flip totrue(default) to enforce.https://claude.ai/code/session_01QK7uzcWZyGDfR7vr24suPi
Generated by Claude Code
Summary by CodeRabbit
New Features
PORTAL_URL; “Site ID” input is removed from login.Bug Fixes & Improvements
account_not_provisionedandaccount_not_ready.Documentation