Duansg commented on PR #4280:
URL: https://github.com/apache/hertzbeat/pull/4280#issuecomment-5162845593
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:
- The migration is type-correct. V181 creates hzb_sop_schedule with enabled
SMALLINT on PostgreSQL and TINYINT on MySQL — not boolean — so SET enabled = 0
is valid on both, and creator VARCHAR(64) already exists in the original CREATE
TABLE, so V182 has a column to filter on. Flyway also runs ahead of Hibernate,
so the new @Index won't collide with the one V182 creates.
- creator is set explicitly even though @CreatedBy would populate it. Both
entities have AuditingEntityListener and JpaAuditorConfig reads the same
SurenessContextHolder subject, so the values agree — and setting it in the
builder is necessary for the ownership check inside the same transaction.
Redundant but correct.
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:
- Administrators lose access to other users' conversations entirely. Every
other resource in HertzBeat (monitors, notice rules, bulletins) is globally
visible, so AI conversations become the one creator-private resource type.
That's a defensible choice, just an inconsistent one worth making deliberately.
- Existing conversations with creator = null become unreachable for
everyone, and V182 disables their schedules. Recovery is documented as manual
SQL against hzb_ai_conversation and hzb_sop_schedule.
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
