Human-delegated protected-resource jobs with Entra OBO adapter (#285) - #320
VirtualAdam wants to merge 2 commits into
Conversation
Add bounded human authorization, private preview and receipt release with durable server/runner verification, local fixture evidence, and portable Git hook suppression. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f07ba18-a200-4f43-858f-525421a64ca3
There was a problem hiding this comment.
🔵 Needs a closer look
Production identity storage is incompatible with Entra OBO, with additional unresolved cancellation, expiry, and process-safety issues.
Pull request overview
Adds human-delegated protected-resource jobs across server admission, OAuth/OBO brokerage, runner execution, verification, UI, and local fixtures.
Changes:
- Adds Entra OBO and test-only Keycloak delegation flows.
- Adds durable operations, audit history, receipt-only runner execution, and UI controls.
- Adds synthetic integration evidence and safer Git hook suppression.
File summaries
| File | Description |
|---|---|
.gitignore |
Ignores Phoenix state. |
Cargo.lock |
Locks new dependencies. |
apps/web/src/App.tsx |
Integrates delegation panel. |
apps/web/src/DelegatedPanel.css |
Styles delegation UI. |
apps/web/src/DelegatedPanel.test.mjs |
Tests panel lifecycle. |
apps/web/src/DelegatedPanel.tsx |
Implements delegation UI. |
crates/crony-runner/Cargo.toml |
Adds HTTP dependency. |
crates/crony-runner/src/adapter/codex.rs |
Updates request fixture. |
crates/crony-runner/src/adapter/copilot.rs |
Updates request fixture. |
crates/crony-runner/src/adapter/delegated.rs |
Adds receipt-only adapter. |
crates/crony-runner/src/adapter/delegated/tests.rs |
Tests delegated adapter. |
crates/crony-runner/src/adapter/external.rs |
Updates request fixture. |
crates/crony-runner/src/adapter/mod.rs |
Registers delegated adapter. |
crates/crony-runner/src/connection_setup/fixtures/fake-gh.mjs |
Uses explicit empty hooks. |
crates/crony-runner/src/connection_setup/github.rs |
Hardens hook suppression. |
crates/crony-runner/src/connection_setup/integration_tests.rs |
Tests real Git hooks. |
crates/crony-runner/src/main.rs |
Binds delegation to transport. |
crates/crony-server/Cargo.toml |
Adds JWT dependencies. |
crates/crony-server/src/delegated.rs |
Implements broker lifecycle. |
crates/crony-server/src/delegated_provider.rs |
Implements provider adapters. |
crates/crony-server/src/factory_connection_tests.rs |
Updates application fixture. |
crates/crony-server/src/main.rs |
Registers broker routes. |
crates/crony-store/src/lib.rs |
Exposes transactional admission. |
db/migrations/0042_delegated_operations.sql |
Adds delegation storage. |
db/migrations/0043_delegated_decision_audit.sql |
Adds decision auditing. |
db/migrations/manifest.json |
Registers migrations. |
docs/DELEGATED_CONNECTIONS.md |
Documents architecture and usage. |
docs/evidence/2026-09-17-delegated-285.md |
Records validation evidence. |
tools/delegated_local.ps1 |
Starts integrated local fixture. |
tools/e2e_delegated.mjs |
Exercises delegated lifecycle. |
tools/fixtures/delegated-keycloak/.gitignore |
Excludes fixture state. |
tools/fixtures/delegated-keycloak/README.md |
Documents local proof. |
tools/fixtures/delegated-keycloak/Stop.ps1 |
Stops fixture processes. |
tools/fixtures/delegated-keycloak/browser.mjs |
Automates isolated browser flows. |
tools/fixtures/delegated-keycloak/compose.yaml |
Defines Keycloak fixture. |
tools/fixtures/delegated-keycloak/login.mjs |
Runs synthetic login. |
tools/fixtures/delegated-keycloak/oauth.mjs |
Implements fixture OAuth. |
tools/fixtures/delegated-keycloak/package-lock.json |
Locks fixture packages. |
tools/fixtures/delegated-keycloak/package.json |
Defines fixture tooling. |
tools/fixtures/delegated-keycloak/security.mjs |
Adds fixture authorization guards. |
tools/fixtures/delegated-keycloak/server.mjs |
Serves fixture components. |
tools/fixtures/delegated-keycloak/setup.mjs |
Provisions synthetic realm. |
tools/fixtures/delegated-keycloak/test/live.test.mjs |
Tests live fixture flow. |
tools/fixtures/delegated-keycloak/test/security.test.mjs |
Tests security helpers. |
tools/fixtures/delegated-postgres.compose.yml |
Defines isolated Postgres. |
tools/local_stack_start.ps1 |
Passes delegated configuration. |
Review details
Files not reviewed (1)
- tools/fixtures/delegated-keycloak/package-lock.json: Generated file
- Files reviewed: 43/46 changed files
- Comments generated: 7
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let subject: Option<String> = sqlx::query_scalar( | ||
| "SELECT hi.subject FROM human_identities hi JOIN actors a ON a.id=hi.actor_id | ||
| JOIN room_memberships rm ON rm.actor_id=a.id JOIN rooms room ON room.id=rm.room_id | ||
| WHERE hi.actor_id=$1 AND hi.issuer=$2 AND a.corp_id=$3 AND room.id=$4 AND room.corp_id=$3", |
| let row=sqlx::query("UPDATE delegated_auth_transactions t SET opened=true,cookie_hash=$2 | ||
| FROM delegated_operations o WHERE t.ticket_hash=$1 AND t.operation_id=o.id AND NOT t.opened AND NOT t.consumed | ||
| AND t.expires_at>now() AND o.expires_at>now() AND o.status='authenticating' | ||
| RETURNING t.*,o.corp_id") |
| if (action === 'cancel') { | ||
| invalidated.current.add(operation.id) | ||
| setInvalidIds(new Set(invalidated.current)) | ||
| tickets.current.delete(operation.id) |
| "UPDATE delegated_operations SET status='expired',token_ciphertext=NULL,token_nonce=NULL | ||
| WHERE status NOT IN ('cancelled','expired','failed','completed') | ||
| AND (expires_at<=now() OR (token_expires_at<=now() AND preview IS NULL))", |
| let count=sqlx::query("UPDATE delegated_operations SET released=true,released_at=COALESCE(released_at,now()) | ||
| WHERE id=$1 AND actor_id=$2 AND status='authorized' AND expires_at>now() AND preview IS NOT NULL") |
| artifacts, | ||
| artifact_retention_days: args.artifact_retention_days.clamp(1, 3_650), | ||
| workspace_sign_in: Arc::new(DashMap::new()), | ||
| delegated: delegated::Broker::from_env(args.mode).await?, |
| [Math]::Abs(($started - $created).TotalSeconds) -gt 30) { | ||
| throw 'Recorded PID no longer matches this lab. No process was stopped.' | ||
| } | ||
| Stop-Process -Id $runtime.pid |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Match migration manifests to committed LF bytes and document fixture compatibility. Generate per-transaction test nonces and PKCE verifiers, cover cross-transaction nonce rejection, and clarify random CSRF state naming without changing its SHA-256 lookup contract. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: de47c7d1-8cf2-458b-a118-d89ee98a2e38
There was a problem hiding this comment.
🔵 Needs a closer look
Startup ordering, cancellation handling, digest normalization, and PID-safe cleanup have unresolved correctness and safety issues.
Review details
Files not reviewed (1)
- tools/fixtures/delegated-keycloak/package-lock.json: Generated file
Suppressed comments (5)
Previously missed (2) — in code that hasn't changed since the last review.
crates/crony-server/src/delegated.rs:32
- This validation accepts uppercase hexadecimal, but
hash(value)returns lowercase and the later comparison is case-sensitive. An uppercase SHA-256 value therefore passes startup validation yet guarantees every protected read is denied. Normalize the configured digest before storing it.
crates/crony-server/src/delegated.rs:572 - The update result is discarded, so cancellation reports
{cancelled:true}even when no row changed—for example, after a receipt was released or the operation already became terminal. The UI can then claim cancellation while the released job continues. Return a denial/conflict whenrows_affected()is not one.
crates/crony-server/src/main.rs:490
- The delegated provider is prepared only after the database connection, migrations, artifact activation, and recovery. A bad delegated URL or failed OIDC discovery can therefore abort startup after those side effects, contrary to the startup boundary in
docs/EVALS.md:863-872. Prepare the broker alongsidePreparedStartupbefore connecting to Postgres, then place the prepared value inAppState.
delegated: delegated::Broker::from_env(args.mode).await?,
tools/fixtures/delegated-keycloak/Stop.ps1:15
- The CIM identity check and
Stop-Process -Idare separate PID lookups. If the recorded process exits between them and Windows reuses the PID, this can terminate an unrelated process. Follow the repository lifecycle boundary intools/local_stack.psm1:65-88: retain aSystem.Diagnostics.Processhandle across path/start-time validation and invokeKill()on that same handle.
apps/web/src/DelegatedPanel.tsx:301 - A failed cancellation permanently leaves this operation in
invalidated: subsequent successful polls never remove it, so Resume sign-in and Cancel stay hidden until the panel remounts even though the durable job may still be active. Roll back the optimistic invalidation in the cancellation error path (or invalidate only after the POST succeeds).
tickets.current.delete(operation.id)
const assignment = owned.current
- Files reviewed: 43/46 changed files
- Comments generated: 0 new
- Review effort level: Balanced
shyamsridhar123
left a comment
shyamsridhar123
left a comment
There was a problem hiding this comment.
Requesting changes on 765aff1f8dd812cf68620177f77e722860a9ecab, reviewed against the current product/security/architecture/evaluation contracts and main aa2ef457d86d727232ae353f53a13c8c9f149bf6.
-
P1 — Join the production login identity to the delegated Entra identity explicitly.
auth.rs:243-248authenticates the human as UserInfosub;delegated.rs:274-286copies that identity's subject, whiledelegated_provider.rs:466-468,560-567compares it with Entraoid. With the same issuer, admission can succeed and the callback then rejects the differingoid; with a different issuer, admission cannot find the mapping. This is narrower than the existing bot comment's unconditional admission-denial claim. The actual migration-10actor_iduniqueness constraint also rejects adding a second mapping for the same human. Preserve the login binding and add a verified external identity association; test the real login/admission/callback path with distinct pairwisesubvalues andoid, rather than supplying the expectedoiddirectly to provider unit tests. -
P1 — Fence ticket opening and consumption before provider effects. The SQL at
delegated.rs:435-439,516-532accepts a still-authenticating delegated row after normal run/task/mission cancellation. The callback performs code redemption and exchange before the current-run persistence guard at lines 539-547. Independently executing these exact predicates against all 43 PR migrations updated one ticket on opening and one on consumption with a cancelled run/task/mission; the same happened with a suspended breaker and with revoked room membership. Serialize current actor/room/task/mission/run/assignment authority with the relevant transitions before issuing or consuming the handoff and before provider effects; add negative cases for these races. A later refusal to persist the token is insufficient. -
P2 — Validate the bound live run when recording release.
delegated.rs:595-602checks membership and the delegated operation but not its bound execution authority. With a cancelled run and an authorized, unexpired preview, the exact update affected one row and persistedreleased=trueplus a release timestamp. Revalidate and lock the same original run/task/mission and current authority in the release transaction. This reproduction proves an invalid disclosure decision; it does not claim the runner subsequently bypassed its separate read guard. -
P2 — Do not acknowledge a cancellation that did not happen.
delegated.rs:570-586discardsrows_affectedand returns{"cancelled":true}. After release, the exact update affects zero rows, leaving the operation authorized, released and retaining its preview. Return the authoritative terminal/conflict outcome when cancellation is no longer allowed, and make the UI reconcile that outcome instead of reporting a successful cancellation.
The database checks used a new owned loopback PostgreSQL database, exact-head migration bytes verified against the manifest, and SQL extracted directly from this revision. Provider calls and HTTP handlers were assessed from source, not claimed as live Entra acceptance. Existing unresolved feedback on expiry cleanup, startup preparation, failed-cancel UI recovery and fixture process ownership also still needs resolution. Before approval, validate the corrected revision against current main, rerun the required contributor checks, and demonstrate the complete native path with migration 43 and these revocation cases. Keep #285 open for its broader acceptance scope.
Description
Implement bounded human-delegated protected-resource jobs through existing mission admission, scheduling, assigned runners, isolated worktrees, artifact upload, and persisted verification.
Related to #285. This PR does not close the issue: the demonstrated bounded protected-flag GET workflow does not qualify the original Azure SQL/Cosmos or arbitrary-resource scope.
Dependencies: configured provider/resource endpoints for real deployment; Keycloak/Postgres for the local fixture. No Base dependency or factory execution was added.
Type of change
How Has This Been Tested?
The preceding implementation session recorded the following results for commit
3d7efae968a65ca548f5085362db6acc583bfc5d. These are prior execution results, not a new test run during PR publication.The user reported successful human validation. A subsequent read-only check recorded a completed mission/task/original run, passed verifier, released receipt, and artifact digest matching signed metadata. An earlier expired unreleased attempt remains recorded as a failure, not reclassified as success.
Evidence:
docs/evidence/2026-09-17-delegated-285.md.Reproduction and configuration:
docs/DELEGATED_CONNECTIONS.md.Test configuration: Windows, Git
2.55.0.vfs.0.8, local Keycloak/Postgres fixture, existing pnpm shims, and a short owned artifact directory outside the source checkout for native Git isolation/path limits. Browser screenshots are not attached to this PR.Scope and remaining limitations
main; no rebase or merge was performed during publication.Checklist
Implementation-session evidence, not an assertion of personal review by the user:
Drafted by Adam's Copilot (AI assistant).