Skip to content

Comments

feat: add OIDC auth with PKCE#1271

Open
casey-brooks wants to merge 5 commits intomainfrom
noa/issue-1270
Open

feat: add OIDC auth with PKCE#1271
casey-brooks wants to merge 5 commits intomainfrom
noa/issue-1270

Conversation

@casey-brooks
Copy link
Contributor

Summary

  • add OIDC login + session services with PKCE, guards, and configurable fallback user
  • enforce per-user scoping across REST + websocket flows, update Prisma schema/backfill stubs, and expand controller coverage
  • update UI graph socket + auth modules to consume new endpoints and surface user context

Testing

  • pnpm lint
  • cd packages/platform-server && pnpm vitest run

Closes #1270

@casey-brooks casey-brooks requested a review from a team as a code owner January 28, 2026 11:51
@casey-brooks
Copy link
Contributor Author

Test & Lint Summary

  • pnpm lint
  • cd packages/platform-server && pnpm vitest run (706 passed / 0 failed / 12 skipped)

@noa-lucent
Copy link
Contributor

[major] Multi-tenant scoping isn’t complete yet: none of the /api/memory endpoints enforce the authenticated principal. packages/platform-server/src/graph/controllers/memory.controller.ts still accepts any threadId and passes it straight to MemoryService, so a logged-in user can read or mutate another user’s per-thread memory by providing their UUID (list/read/append/etc.). listDocs also returns per-thread doc entries for every user because the controller never filters by ownerUserId. Please plumb @CurrentPrincipal into these handlers (and/or enforce the owner in MemoryService) so a request is rejected unless the thread belongs to principal.userId, and hide other users’ threads when listing docs. Without that, the new OIDC mode does not actually isolate user data.

Copy link
Contributor

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the massive auth refactor—OIDC + per-user guards are shaping up well. I still need a couple of blockers addressed before we can ship:

  1. The new cross-tenant guard in createThreadWithInitialMessage throws thread_parent_owner_mismatch, but AgentsThreadsController.createThread never catches that error. Attempts to create a child under someone else’s thread now bubble up as a 500 (create_failed) instead of a deterministic 404/403. Please map that error to the proper HTTP response.
  2. The /api/memory endpoints were left untouched, so any authenticated user can read or mutate another user’s per-thread memory simply by providing their thread UUID. We need to plumb the principal through those handlers (and/or enforce the owner in MemoryService) and hide other users’ threads when listing docs.

Happy to re-review once those are addressed.}

@casey-brooks
Copy link
Contributor Author

Test & Lint Summary

  • pnpm lint
  • cd packages/platform-server && pnpm vitest run (709 passed / 0 failed / 12 skipped)

noa-lucent
noa-lucent previously approved these changes Jan 28, 2026
Copy link
Contributor

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All prior blockers are resolved. createThread now enforces parent ownership (and maps thread_parent_owner_mismatch to a 404) before persisting, and every /api/memory endpoint now requires the principal and checks thread ownership so per-thread docs can’t leak. Tests cover both scenarios. Looks good to ship.

@casey-brooks
Copy link
Contributor Author

Test & Lint Summary

  • pnpm --filter @agyn/platform-server run typecheck
  • pnpm lint
  • cd packages/platform-server && pnpm vitest run (709 passed / 0 failed / 12 skipped)

noa-lucent
noa-lucent previously approved these changes Jan 28, 2026
Copy link
Contributor

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-confirmed after the latest updates (type fixes and tighter thread validation). Ownership guarding and OIDC/session changes still look solid. Approved.

@casey-brooks
Copy link
Contributor Author

Test & Lint Summary

  • pnpm --filter @agyn/platform-server run typecheck
  • pnpm lint
  • cd packages/platform-server && pnpm vitest run (709 passed / 0 failed / 12 skipped)

@casey-brooks
Copy link
Contributor Author

Test & Lint Summary

  • pnpm --filter @agyn/platform-server run typecheck
  • pnpm lint
  • cd packages/platform-server && pnpm vitest run (709 passed / 0 failed / 12 skipped)

Copy link
Contributor

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs + migration refresh look good. README now documents the OIDC env contract comprehensively, and the migration rename consolidates the reminder FK so we still add the cascade constraint in one step. Nothing further blocking merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add OIDC IdP auth with per-user conversations + single-user fallback

2 participants