fix: emit confirm-publish events with permission-aware subscriber fanout - #735
Conversation
Signed-off-by: 千乘妍 (Xiaoyaner) <258399167+xiaoyaner0201@users.noreply.github.com>
d72f461 to
e071afb
Compare
FenjuFu
left a comment
There was a problem hiding this comment.
Deep-reviewed against #731 and the repo's visibility model. The core fix is correct and the extra hardening is well thought through — a few actionable points below.
Core fix (resolves #731): confirmPublish now publishes SkillPublishedEvent after persisting the PUBLISHED version and latestVersionId, so NotificationEventListener#onSkillPublishedForSubscribers is finally reached — consistent with the autoPublish branch in SkillPublishService.
On the recipient filtering: I initially flagged that the issue's scenario (a regular namespace member subscribing to a PRIVATE skill and expecting a notification) is at odds with SubscriptionMetadataAccessPolicy returning owner || manager for PRIVATE. After checking docs/oss-02-core-semantic-rules.md, the canonical read model is PRIVATE -> owner only, not marketplace-visible, so the original "member gets notified" behavior was itself fail-open. Filtering non-eligible subscribers is the correct call, and allowing manager (ADMIN/OWNER) is a reasonable governance extension. The batch account/member loading (findByIdIn / findByNamespaceIdAndUserIdIn) avoids an N+1 in the async fan-out. Good.
Points worth addressing / a maintainer decision:
-
Missing i18n key.
subscribe()throwsnew DomainForbiddenException("error.skill.subscription.noPermission"), but I don't see that key added tomessages.properties/messages_zh.properties(the diff is Java-only).ApiResponseFactoryresolves viagetMessage(code, args, code, locale), so this degrades to showing the raw code rather than a 500 — not fatal, but existing forbidden keys (e.g.error.namespace.membership.required) are registered, so please add EN/zh entries for consistency. -
subscribe()is now a behavior change on a public endpoint. PreviouslyPUT /skills/{id}/subscriptionsucceeded for any existing skill; it now returns 403 for users without current read access, and existing regular-member subscriptions to PRIVATE/NAMESPACE_ONLY skills will be silently filtered out of all future notifications. That's the right hardening, but it's a maintainer call whether a one-time prune/migration of now-ineligibleskill_subscriptionrows is wanted so the DB state matches the new policy. -
Fail-closed on a null namespace. In the listener,
namespaceRepository.findById(...).orElse(null)feeding an eligibility check that denies whennamespace == nullmeans a namespace-load hiccup silently drops the whole fan-out. Safe by default, but a debug/warn log there would make such a case diagnosable rather than invisible.
Only DCO has reported so far — worth waiting for the backend build/test job before merge so the new listener/policy tests are confirmed green in CI. Correctness and design look right to me; leaving the merge and the policy/migration decision to the maintainers.
|
Thanks for the PR. The narrow #731 regression has now been fixed by #738, which is merged into main. The broader permission-aware subscriber fanout in this PR is still valuable, but it should be handled as a separate authorization hardening change rather than bundled with the confirmPublish event fix. Please keep this PR open as draft/design work, rebase it on latest main, remove the already-merged confirmPublish change, and narrow the scope to:
We should not close this PR, but it is not mergeable as-is. |
Summary
SkillPublishedEventafterconfirmPublishpersists the published version and latest skill versionFixes #731.
Why
The confirm-publish path currently updates persistence without reaching the existing subscriber listener. For PRIVATE skills, fanout also needs to re-check current read eligibility so removed or stale subscribers do not receive recipient-visible metadata.
Validation
Commands and results:
Risk
Limits
Not exercised against production infrastructure, a real browser EventSource client, or a configured Redis cluster. The synchronous production listener/eligibility/dispatcher path and final persistence/SSE arguments are covered.
Notes