Conversation
github-actions
Bot
added
calm-hub
There was a problem hiding this comment.
I love the idea of this! But it's a massive change set, so I've mobilized my robots to help review.
🤖 Multi-model review, orchestrated by Claude Fable 5: three agents (Haiku, Sonnet, Opus) reviewed this branch independently and in parallel, and every load-bearing claim below was re-verified against source before posting.
Comment mode, not a block — plenty of real work here, and the read path plus the producer wiring are clean. Flagging what I think stands between this and merge.
Docs
Two things I couldn't answer from the PR itself:
- Use case / user story. None of the 172 changed files is a
.md;calm-hub/README.mdis untouched.calm.database.mode=githubneedscalm.github.namespacesinname|repo|branchform, which appears only in a log-warning string and the parser — so there's no route for an operator to configure this from the repo. - Why not Keycloak.
calm-hub/README.md:187already documentssecureas "Keycloak or another IdP", and :231 scopes Keycloak to local dev.oidc-client-tswith PKCE predates this branch. This adds a third auth profile besidesecure/no-auth/proxy-auth— what doessecurenot do? Worth a paragraph, since it's the whole premise.
Blocking
- Account linking has no CSRF protection.
statecarries caller-supplied identity and both endpoints are unauthenticated. Inline atGitHubLinkResource.java. id_tokenas API bearer, plusissuer=anyto make the server accept it. The audience mismatch is routed around at both ends rather than fixed at the source. Inline.- The write path is a stub. Every mutating method across the 15 GitHub stores throws, and
createPullRequesthas no callers. GitHub mode is read-only — which is fine as a phase 1, but it means the entire linking subsystem shipped here (resource, cookie service, request context,PendingWriteException,GitHubLinkStatus) currently feeds a method that always throws. Please say so in the description.
Scope
The layout wire-format change (pins→nodes, both migration steps, node w/h persistence across the visualizer) is roughly half the UI diff and unrelated to OIDC or GitHub storage — its own Javadoc cites #2942. Bundled here neither half can be reverted without the other. Worth splitting; the migration steps themselves look correct and idempotent.
Non-findings, recorded so they don't get re-raised
FIELD_SEPARATORis0x1F, not""— two of the three reviewers independently filed thesplit("")explosion as a critical before checking the bytes. The cookie crypto (AES-GCM, per-encrypt IV, tag, expiry, subject binding) is sound; the weakness is what gets bound as the subject, not the crypto.- The producer refactor leaves the Mongo/Nitrite arms and the unknown-mode fallback intact.
- Existing Keycloak deployments are untouched — new behaviour is confined to the
oidcprofile. - Flipping
seed-demo-datato false breaks nothing; the seeder is introduced by this PR.
Rest is inline — the majors, and a few minors/nits with suggestions where the fix is obvious.
| return user.access_token; | ||
| // Entra ID: access_token audience is MS Graph, not our API. | ||
| // Send the id_token which has our client_id as audience. | ||
| return user.id_token || user.access_token; |
There was a problem hiding this comment.
🤖 ID token as an API credential is the specific anti-pattern the OAuth 2.0 Security BCP calls out — it's an authentication receipt for the client, not an authorization credential for the resource server.
The Entra audience problem is real, but the fix is exposing an API scope (api://<client-id>/…), requesting it from the SPA, and sending the resulting access token. As it stands this and issuer=any route around the same mismatch from both ends, which is why the server had to stop checking.
There was a problem hiding this comment.
🤖 Verified at 3e24aed: the server half is now coherent (audience=${CALM_OIDC_CLIENT_ID} validates the ID token it is actually sent), but authService.tsx:80-82 is unchanged and still sends id_token as the API credential — the BCP anti-pattern this comment is about. Fine to defer past this PR, but it should land as a tracked follow-up: expose an API scope (api://<client-id>/...), request it in the SPA, send the access token, and point audience at the API. Leaving open until there's an issue to point at (or a fix here).
Thanks @eddie-knight for the thorough multi-model review — really appreciate the depth here. We've addressed every item. Here's the breakdown: Blocking issues — all resolved
Fixed. The linking flow is now CSRF-protected:
An attacker cannot forge a valid state without the server's AES key, and cannot initiate a flow without a valid OIDC token.
Fixed. Token validation now enforces both audience and issuer: quarkus.oidc.token.audience=${CALM_OIDC_CLIENT_ID} These reuse the same env vars already passed for OIDC discovery. A token from another app in the same Entra tenant is now rejected (different aud). JWKS signature validation was always active — this adds the audience/issuer layer on top. Regarding id_token vs access_token: in our Entra setup, the SPA uses PKCE and the id_token carries the correct audience (client-id) and is signed by the tenant's JWKS. Registering a dedicated API scope requires Entra admin-level configuration that isn't in our control. The id_token path is validated (signature + audience + issuer + expiry) and sufficient for this read-only phase.
Once CALM Hub becomes RW we wanted to follow Fork & PR Process which is ideated here #2925 (comment) Acknowledged. GitHub mode is read-only in this phase. The linking subsystem is scaffolding for future write support (fork + PR creation). Updated PR description to state this explicitly. Technical issues — resolved Cookie security — Both /callback and /unlink now use calm.github.cookie.secure config (default true) + SameSite=Lax Hardcoded api.github.com — GitHubLinkResource uses calm.github.api-url; GitHubRepoSync uses calm.github.oauth.base-url Hand-rolled JSON parsing — Replaced with Jackson ObjectMapper in both GitHubLinkResource and GitHubVersionService UnsupportedOperationExceptionMapper scope — Created GitHubWriteNotSupportedException (extends UnsupportedOperationException). Mapper only catches that. All 15 GitHub stores updated. JDK's UnsupportedOperationException from List.of() etc. now propagates normally. Math.abs(Integer.MIN_VALUE) — All GitHub stores use hashCode() & 0x7FFFFFFF (always non-negative) parallelStream over blocking HTTP — Switched to sequential .stream() in GitHubArchitectureStore SHA extraction grabs tree/parent — GitHubVersionService.extractShas() now parses JSON with Jackson — only reads top-level sha from each commit array element Synchronous startup clone — GitHubStartupInitializer now uses ManagedExecutor.runAsync() — Quarkus boots immediately, health endpoints bind while cloning runs in the background git pull on shallow clone — GitHubRepoSync.pullRepo() uses fetch + reset --hard origin/ — handles upstream force-pushes gracefully Unread config properties — Removed: RBAC block (7 props), session-claim, clone-parallelism, clone-timeout, sync-failure-threshold No test for GitHubLinkResource — Extracted GitHubOAuthClient (injectable HTTP seam). Added 16 unit tests covering all endpoints and error paths. Removed from JaCoCo exclusion list. Regex widening partial — PatternResource and FlowResource GET-version endpoints now use VERSION_OR_SHA_REGEX (consistent with Architecture/Standard) NoOpSchemaVersionStore lock — Intentional design — no-op store has no real lock mechanism. Test documents this. GitHubLinkStatus.tsx missing ?user= — Eliminated entirely — /link is now authenticated and reads identity from the token. UI fetches with bearer, then navigates to the returned authorizeUrl. Auth fails open — fetchAuthConfig() now retries once (1s delay) before throwing. No longer silently caches oidc.enabled: false. Nits — resolved
Scope concern (layout migration) The layout wire-format change (pins→nodes) is functionally coupled to the GitHub backend — it was developed and tested together. Splitting retroactively would require significant rebasing effort with risk of introducing regressions. The migration itself is idempotent and both paths are tested. Test coverage
Non-findings confirmed Agree with the reviewer's non-findings:
Thank You. |
|
🤖 Follow-up verification of the multi-model review posted at ab1b6ed, re-run against head 3e24aed: 19 of 24 findings are verified fixed and their threads resolved — including both CSRF criticals (server-minted AES-GCM state with nonce + expiry, authenticated
Same method as the original review: three independent models re-verified every finding against source, and every verdict was re-checked by the orchestrator before posting. |
Adding edit capability to calm hub has been discussed in WG and Gaurav Shah( discussed as part of #2857) from OpsWork has volunteered to look into it and this is first step to support Git as backend, for starters read only mostly like current setup. All other issues are fixed now. Thank You. |
byrash
changed the title
|
Hello @rocketstack-matt , @markscott-ms & @jonfreedman , As we are looking at write capability later into calm hub, we have decided Git integration with OAuth could be staged for later iterations. We shall adjust PR to reflect it. Thank You |
byrash
changed the title
008379d to
a03d8f7
Compare
This PR has been split into 5 layered PRsThis is a large change (183 files, +11.8k/-0.4k) bundling several independent pieces of work with the GitHub-storage-backend feature. To make review tractable, it's been split into 5 stacked PRs — each targets the previous one, not
Authorship: every commit across all 5 PRs is attributed to @byrash (this PR's author), committed by @jpgough-ms as part of the split. A permanent, unmodified copy of this PR's exact head is preserved at Content note: the GitHub OAuth account-linking surface that was present earlier in this PR's history was removed in a later force-push, per @byrash's comment below on 2026-08-29 ("Git integration with OAuth could be staged for later iterations") — none of the 5 PRs above include it. Each PR's description lists the specific open findings relevant to it. This PR (#3001) stays open as the umbrella until the slices is merged. Once #3066 merges to |
Extracted from #3001. Adds a GitHub storage mode (calm.database.mode=github): namespaces map to cloned repos, resources are read from an in-memory registry built off the clone, and writes return 501 pending the account-linking work staged for a later iteration. Includes the producer wiring for all resource types, the BuildingBlock resource type, SHA-based version reads, and markdown rendering for raw documents served from a repo. Original-PR: #3001
Extracted from finos#3001 (OIDC + SCM backend support), which bundled these unrelated UI changes with the GitHub storage backend work. - Explore rail and mobile nav show loading state while counts resolve - Namespace/type in the section header render as links - Sparkline overflow fix (finos#2728) Original-PR: finos#3001
Extracted from #3001. Serves OIDC config to the SPA from the server (/api/calm/auth/config) instead of build-time constants, adds the VS Code plugin's browser-based OIDC login flow (PluginAuthResource + OidcPluginAuthClient), and lets namespace/domain read access be granted via any UserAccessStore namespace grant, not just the existing UserAccessValidator path — needed for backends where that validator isn't resolvable. Original-PR: #3001
Extracted from #3001. Adds a GitHub storage mode (calm.database.mode=github): namespaces map to cloned repos, resources are read from an in-memory registry built off the clone, and writes return 501 pending the account-linking work staged for a later iteration. Includes the producer wiring for all resource types, the BuildingBlock resource type, SHA-based version reads, and markdown rendering for raw documents served from a repo. Original-PR: #3001
…ndencies (#3063) * build(calm-hub): add observability and GitHub-backend dependencies Extracted from #3001. Adds caffeine, jgit, quarkus-scheduler and the OpenTelemetry/Micrometer stack — all inert until consumed by later slices. OTEL is env-gated (CALM_OTEL_ENABLED=false by default), so this changes no runtime behaviour on its own. Original-PR: #3001 * fix(calm-hub): actually disable Micrometer in the test profile The comment said the test profile disables both OTEL and Micrometer, but only quarkus.otel.enabled was %test.-prefixed — Micrometer's core registry (and its auto-enabled binders) stayed active during mvn test despite the stated intent. Claude-Session: https://claude.ai/code/session_0199XmacMNTrxWL4x5CSXyp1 * fix(calm-hub): disable OTEL/Micrometer under every custom test profile, not just 'test' quarkus.otel.enabled and quarkus.micrometer.enabled are both build/ run-time-fixed per active profile NAME (the same pitfall the jacoco comment in this file already documents) — a %test. prefix only applies when a test's QuarkusTestProfile.getConfigProfile() is literally "test". This module has four other custom profile names (integration-test, nitrite-integration-test, secure, proxy-auth), so both properties stayed enabled under mvn -P integration verify despite the previous fix. Verified: full integration suite (540 tests) still passes with all profiles now suppressed. Claude-Session: https://claude.ai/code/session_0199XmacMNTrxWL4x5CSXyp1 * fix(calm-hub): make the OTel production toggle actually work at deploy time quarkus.otel.enabled is build-time-fixed (verified via javap against the Quarkus 3.34.7 jars: OTelBuildConfig.enabled() is BUILD_AND_RUN_TIME_FIXED). Empirically confirmed the bug: packaging with CALM_OTEL_ENABLED unset (default false) and then running the same jar with CALM_OTEL_ENABLED=true at runtime produced zero OTel activity — the env var was silently ignored either direction, despite the adjacent comment claiming a deploy-time toggle. In a build-once- deploy-many pipeline this meant OTel could never actually be turned on without rebuilding the artifact. Fixed by always compiling the extension in (quarkus.otel.enabled=true) and gating it via the genuinely RUN_TIME-scoped quarkus.otel.sdk.disabled instead, defaulting to disabled. Renamed the env var to CALM_OTEL_DISABLED (inverted polarity to match) since the old CALM_OTEL_ENABLED name described a toggle that never worked; nothing in the tree references it yet. Verified empirically: packaged an uber-jar once, then ran it twice with only the env var changed — CALM_OTEL_DISABLED unset produced no OTel activity, CALM_OTEL_DISABLED=false produced live OTLP export attempts (connection-refused, since nothing was listening — proving the SDK was actually active). Full unit (2868) and integration (540) suites still pass. Micrometer has no equivalent runtime-mutable switch (confirmed via javap: its binder .enabled properties are nested under the same BUILD_AND_RUN_TIME_FIXED root as the master switch, and the runtime- scoped HttpServerConfig/HttpClientConfig classes carry no enabled field) — updated the comment to say so honestly rather than implying a deploy-time toggle Quarkus doesn't provide for it. Claude-Session: https://claude.ai/code/session_0199XmacMNTrxWL4x5CSXyp1 * fix(calm-hub): use the quarkus-caffeine extension instead of the bare caffeine jar Caffeine builds its cache implementation classes reflectively. quarkus-caffeine supplies the native-image reflection config for that; the bare com.github.ben-manes.caffeine:caffeine jar would only surface this as a native build failure, and calm-hub's native image builds don't run in PR CI. The extension pulls the same caffeine jar in transitively, so nothing else changes. --------- Co-authored-by: Shivaji Byrapaneni <Shivaji.Byrapaneni@fmr.com>
Extracted from #3001. Adds a GitHub storage mode (calm.database.mode=github): namespaces map to cloned repos, resources are read from an in-memory registry built off the clone, and writes return 501 pending the account-linking work staged for a later iteration. Includes the producer wiring for all resource types, the BuildingBlock resource type, SHA-based version reads, and markdown rendering for raw documents served from a repo. Original-PR: #3001
Extracted from #3001. Serves OIDC config to the SPA from the server (/api/calm/auth/config) instead of build-time constants, adds the VS Code plugin's browser-based OIDC login flow (PluginAuthResource + OidcPluginAuthClient), and lets namespace/domain read access be granted via any UserAccessStore namespace grant, not just the existing UserAccessValidator path — needed for backends where that validator isn't resolvable. Original-PR: #3001
Extracted from #3001. Adds a GitHub storage mode (calm.database.mode=github): namespaces map to cloned repos, resources are read from an in-memory registry built off the clone, and writes return 501 pending the account-linking work staged for a later iteration. Includes the producer wiring for all resource types, the BuildingBlock resource type, SHA-based version reads, and markdown rendering for raw documents served from a repo. Original-PR: #3001
Extracted from #3001. Serves OIDC config to the SPA from the server (/api/calm/auth/config) instead of build-time constants, adds the VS Code plugin's browser-based OIDC login flow (PluginAuthResource + OidcPluginAuthClient), and lets namespace/domain read access be granted via any UserAccessStore namespace grant, not just the existing UserAccessValidator path — needed for backends where that validator isn't resolvable. Original-PR: #3001
Extracted from #3001. Adds a GitHub storage mode (calm.database.mode=github): namespaces map to cloned repos, resources are read from an in-memory registry built off the clone, and writes return 501 pending the account-linking work staged for a later iteration. Includes the producer wiring for all resource types, the BuildingBlock resource type, SHA-based version reads, and markdown rendering for raw documents served from a repo. Original-PR: #3001
* feat(calm-hub): add generic Caffeine-backed cache service Extracted from #3001. CalmCacheService/CaffeineCacheService is a generic TTL cache with no GitHub types — unused until a later slice wires a consumer. Original-PR: #3001 * fix(calm-hub): address review findings on the cache service slice - Collapse CalmCacheService/CaffeineCacheService into a single concrete CalmCacheService bean. calm-hub puts an interface in front of a service only where multiple backends are selected at runtime (see store/ and its Mongo/Nitrite producers); a cache with one implementation and no near-term second one doesn't fit that pattern. - Add getList(key, elementType), so the one known consumer (GitHubVersionService, landing in slice 5) doesn't need get(key, List.class) plus an unchecked cast to use a typed list. - Document the class contract: null values are ignored on put, a type mismatch on get/getList returns empty rather than throwing, and TTL is not refreshed on read. - Reject a null ttl in put() with a clear NPE at the call site instead of failing later inside Caffeine's Expiry callback. - Make maximumSize configurable via calm.cache.max-size (default 10000, constructor-injected) instead of hardcoded, following the module's @ConfigProperty convention. - Make the Caffeine Ticker injectable via a package-private constructor, matching the LongSupplier pattern already used by SchemaMigrationInProgressFilter, and use it to make TTL expiry tests deterministic instead of Thread.sleep. - Rewrite the concurrent-access test to assert on final cache state via the submitted Futures, instead of only checking a CountDownLatch that a finally-block would trip even if every task had thrown. * fix(calm-hub): rescope the cache as GitHub-only, not a calm-wide generic primitive CalmCacheService was framed as a generic, calm-hub-wide TTL cache (org.finos.calm.cache package, generic get/put/getList/evict API). That's a real trap in a multi-instance deployment: it's a per-JVM, in-memory cache with no cross-instance coordination, and a generic-shaped, generic-packaged, generic-Javadoc'd class invites being reached for to cache a Mongo/Nitrite-backed read, where a write on one instance would never invalidate another instance's cached read. It's safe for its actual sole use (GitHub API responses) only because that backend is read-only through calm-hub and already tolerates per-instance eventual consistency by design. See #3073 for the full writeup. - Move + rewrite as org.finos.calm.store.github.util.GitHubApiResponseCache (the established package for GitHub-only helpers), with @LookupIfProperty(calm.database.mode=github) matching every sibling. - Replace the generic get/put/getList API with purpose-built methods — getVersions/putVersions, getContentAtSha/putContentAtSha — baking both TTLs (5 min, 365 days) and both key formats in as private constants. This is the real structural barrier: reusing this for Mongo-backed data now requires editing the class, not just calling it differently. - Drop evict/evictByPrefix: unused by all production code, and unnecessary once both TTLs are fixed rather than caller-supplied. - Drop the custom Expiry/CacheEntry machinery (it existed specifically to support a variable per-call TTL) for two plain Caffeine caches with expireAfterWrite. Keep the injectable Ticker for deterministic expiry tests. - Rename calm.cache.max-size to calm.github.cache.max-size. - Full Javadoc rewrite stating the cross-instance limitation plainly, why it's safe here, and an explicit prohibition on reuse for Mongo/Nitrite data. Test file rewritten to match: the type-mismatch and evict tests no longer apply under the new API; added independent-expiry coverage for the two caches. * fix(calm-hub): address code review findings on the GitHub cache rework - getVersions/putVersions aliased the cache's internal List to whatever the caller passed in or read back — a caller mutating either reference would silently corrupt the shared cache entry for every other concurrent reader until TTL expiry. putVersions now stores an immutable List.copyOf(...), so both directions are safe. - Consolidated the four near-identical get/put method bodies into private generic read/write helpers, and the two near-identical Caffeine.newBuilder() chains into a private buildCache helper. --------- Co-authored-by: Shivaji Byrapaneni <Shivaji.Byrapaneni@fmr.com>
Extracted from #3001. Adds a GitHub storage mode (calm.database.mode=github): namespaces map to cloned repos, resources are read from an in-memory registry built off the clone, and writes return 501 pending the account-linking work staged for a later iteration. Includes the producer wiring for all resource types, the BuildingBlock resource type, SHA-based version reads, and markdown rendering for raw documents served from a repo. Original-PR: #3001
Extracted from #3001. Serves OIDC config to the SPA from the server (/api/calm/auth/config) instead of build-time constants, adds the VS Code plugin's browser-based OIDC login flow (PluginAuthResource + OidcPluginAuthClient), and lets namespace/domain read access be granted via any UserAccessStore namespace grant, not just the existing UserAccessValidator path — needed for backends where that validator isn't resolvable. Original-PR: #3001
Extracted from #3001. Adds a GitHub storage mode (calm.database.mode=github): namespaces map to cloned repos, resources are read from an in-memory registry built off the clone, and writes return 501 pending the account-linking work staged for a later iteration. Includes the producer wiring for all resource types, the BuildingBlock resource type, SHA-based version reads, and markdown rendering for raw documents served from a repo. Original-PR: #3001
Extracted from #3001. Adds a GitHub storage mode (calm.database.mode=github): namespaces map to cloned repos, resources are read from an in-memory registry built off the clone, and writes return 501 pending the account-linking work staged for a later iteration. Includes the producer wiring for all resource types, the BuildingBlock resource type, SHA-based version reads, and markdown rendering for raw documents served from a repo. Original-PR: #3001
Extracted from #3001. Adds a GitHub storage mode (calm.database.mode=github): namespaces map to cloned repos, resources are read from an in-memory registry built off the clone, and writes return 501 pending the account-linking work staged for a later iteration. Includes the producer wiring for all resource types, the BuildingBlock resource type, SHA-based version reads, and markdown rendering for raw documents served from a repo. Original-PR: #3001
Extracted from #3001. Adds a GitHub storage mode (calm.database.mode=github): namespaces map to cloned repos, resources are read from an in-memory registry built off the clone, and writes return 501 pending the account-linking work staged for a later iteration. Includes the producer wiring for all resource types, the BuildingBlock resource type, SHA-based version reads, and markdown rendering for raw documents served from a repo. Original-PR: #3001
Thanks a ton @jpgough-ms . Great work here. Hopefully we can move faster on these changes now. |
This branch has not been deployed
3 participants
Description
Type of Change
Affected Components
cli/)calm/)calm-ai/)calm-hub/)calm-hub-ui/)calm-server/)calm-widgets/)docs/)shared/)calm-plugins/vscode/)Commit Message Format ✅
Testing
Checklist