maintenance: scope AI conversations by creator - #4280
Conversation
|
Author remediation update: The ownership boundary now includes conversations, messages, stream and security-data reads, plus every SOP schedule create, list, get, update, delete, and toggle path. A schedule persists its owner, creation verifies the target conversation, and background execution revalidates that persisted relationship without a login-time subject. Historical null creators use an explicit migration and isolation path with supporting indexes. Two-user IDOR and scheduled-message injection regressions are covered. Focused AI tests (50) and the startup reactor (24 modules) passed locally; the current GitHub backend, E2E, docs, license, and label checks are green. The author-side blocker is resolved; maintainer review is still required. |
|
Scoping these by creator is the right call. On master, ConversationServiceImpl reaches conversations through findById() / findAll() with no owner check, and MonitorToolsImpl.java:312 loads a conversation's securityData — AES-encrypted monitor credentials — through that same unscoped lookup, so any authenticated user could reach another user's conversation and the secrets attached to it. The DAO-level findByIdAndCreator / findByConversationIdAndCreator approach closes that cleanly, and re-validating ownership in getScheduleForExecution before both result and error delivery is a nice touch for the background path. Two things I checked that are not problems, so they don't get re-raised:
What needs a maintainer decision The PR discloses this, so it isn't a hidden bug, but it's the part I'd want agreed before merge:
For an operator upgrading a running instance, "your AI history is gone unless you hand-edit the database" is a rough landing. An admin-facing recovery path in the app — list ownerless conversations, assign an owner, re-enable the schedule — would turn this from a support ticket into a UI action. Happy to see it as a follow-up rather than in this PR, but it'd be good to have a plan on record. Minor requireCurrentUserId() is duplicated verbatim in ConversationServiceImpl and SopScheduleServiceImpl. The subject lookup already appears in several places (JpaAuditorConfig, AccountServiceImpl, AuthTokenController), so a single shared helper would be worth extracting while this is fresh. Also, IllegalStateException("No authenticated user") will likely surface as a 500 rather than a 401 — worth checking how the controllers map it. |
…nversation-access # Conflicts: # hertzbeat-ai/src/test/java/org/apache/hertzbeat/ai/schedule/SopScheduleExecutorTest.java # hertzbeat-ai/src/test/java/org/apache/hertzbeat/ai/service/impl/SopScheduleServiceImplTest.java
…nversation-access
Aias00
left a comment
There was a problem hiding this comment.
Review: #4280 maintenance — scope AI conversation / SOP schedule owners
Reviewed the full diff (+685 / 18 files). This enforces authenticated-creator ownership across the AI conversation and SOP schedule operations.
What's correct and well done
- Every conversation and schedule operation now scopes by the authenticated creator (
SurenessContextHolderprincipal):streamChat,getConversation,getAllConversations,deleteConversation,saveSecurityData, and all schedule CRUD. The schedulecreatoris taken from the server-side subject, never from the request body (createSchedulebuilds a fresh entity withcreator(creator)). MonitorToolsImpl.addMonitorProtectednow requiresMcpContextHoldersubject and usesfindByIdAndCreator, so a protected monitor can no longer load another user's secure-form (securityData) via an unscoped lookup.ChatConversation.securityDatais now@JsonIgnore— encrypted security data is no longer serialized in API responses (good leakage fix).- Background execution (
SopScheduleExecutor+getScheduleForExecution) re-checks the persisted owner before and after execution; if the owner changes mid-flight the result/error is not delivered. Ownerless schedules are disabled by migration V182 (h2/mysql/postgres), and missing-owner/mismatch is rechecked before every execution. - Tests cover cross-creator get/list/delete rejection, mask-restore, owner-loss auto-disable, and the proxy/rate-limit edge cases in the OAuth sibling.
Non-blocking (expected behavior)
- After upgrade, all schedules without a recorded creator are disabled and ownerless conversations are isolated. The recovery path is documented in
upgrade.md(backup → verify owner → set creator → re-enable). This is the intended secure default; just make sure operators read the upgrade note.
No blocking issues. Authorization model is substantially stronger and the contracts are tested.
Verdict: APPROVE
Summary
Upgrade impact
Validation
./mvnw -pl hertzbeat-ai -am -Dtest=SopScheduleServiceImplTest,SopScheduleExecutorTest,ConversationServiceImplTest,ChatConversationTest test -DskipITs -Dsurefire.failIfNoSpecifiedTests=false -DfailIfNoTests=false./mvnw -pl hertzbeat-ai -Dtest=MonitorToolsImplTest,SopScheduleServiceImplTest,SopScheduleExecutorTest,ConversationServiceImplTest test -DskipITs -Dsurefire.failIfNoSpecifiedTests=false -DfailIfNoTests=false(20 tests)./mvnw -pl hertzbeat-ai test -DskipITs(50 tests)./mvnw -pl hertzbeat-startup -am -DskipTests packagegit diff --cached --checkAI assistance: used for draft implementation and test iteration.
Human validation: focused ownership tests, the complete AI module suite, a fresh H2 migration proof, and the startup source package reactor all completed successfully; Checkstyle reported no violations.
Risk notes: legacy ownerless conversations and schedules require deliberate administrator recovery after verifying the target conversation owner; the migration does not guess or bulk-assign ownership.