Skip to content

Remove redundant channel auth, add per-user notify preference#70

Merged
vaayne merged 2 commits intomainfrom
refactor/remove-notify-allowed-ids
Mar 23, 2026
Merged

Remove redundant channel auth, add per-user notify preference#70
vaayne merged 2 commits intomainfrom
refactor/remove-notify-allowed-ids

Conversation

@vaayne
Copy link
Copy Markdown
Owner

@vaayne vaayne commented Mar 22, 2026

Summary

  • Remove NotifyChat, AllowedIDs, and defaultChat from all channels (telegram, qq, feishu, weixin), dispatcher, gateway, admin UI, and docs. Access control is now fully handled by RBAC (auth_identities + policy engine), and notification targets are resolved from linked identities.

  • Add per-user notify channel preference via notify_identity_id FK on auth_usersauth_identities. NotifyUser now sends to a single channel (preferred identity or first linked) instead of broadcasting to all linked channels. ON DELETE SET NULL auto-clears preference when an identity is unlinked.

Net: -529 lines, 22+16 files changed across 2 commits.

Changes

Commit 1: Remove redundant fields

  • Remove AllowedIDs config + isAllowed() from all 4 channels
  • Remove NotifyChat config + fallback from all Notify() methods
  • Remove defaultChat from Dispatcher.Register()
  • Clean gateway wiring, admin UI, channels.js
  • Update notification-system docs (en/zh/ja) and builtin anna skill

Commit 2: Per-user notify preference

  • Schema: notify_identity_id INTEGER REFERENCES auth_identities(id) ON DELETE SET NULL
  • Auth: NotifyIdentityID *int64 on AuthUser, UpdateUserNotifyIdentity store method
  • Dispatcher: NotifyUser picks single channel (preferred > first linked)
  • Admin UI: "Notify Channel" dropdown on user detail panel
  • API: PUT /api/users/{id}/notify-identity
  • Tests: 3 new NotifyUser tests (first linked, preferred, stale preference fallback)

Test plan

  • mise run lint — 0 issues
  • mise run test — all channel/auth/db tests pass (429 tests)
  • Manual: verify admin UI dropdown renders and saves correctly
  • Manual: verify scheduler notification goes to single channel

vaayne added 2 commits March 22, 2026 23:08
…nels

Access control and notification routing are now fully handled by the
RBAC system (auth_identities + policy engine). Remove the redundant
per-channel AllowedIDs allowlist and NotifyChat/defaultChat static
fallback from all channels (telegram, qq, feishu, weixin), the
dispatcher, gateway wiring, admin UI, and docs.
Add notify_identity_id FK on auth_users pointing to auth_identities.
NotifyUser now sends to a single channel instead of all linked ones:
- If the user set a preferred identity, use that.
- Otherwise use the first linked identity (earliest linked_at).
- ON DELETE SET NULL cascades when an identity is unlinked.

Includes admin UI dropdown, API endpoint, migration, and tests.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 20be4b3569

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines 20 to 23
return func(_ *dto.WSPayload, data *dto.WSC2CMessageData) error {
msg := (*dto.Message)(data)
authorID := msg.Author.ID
if !b.isAllowed(authorID) {
logger().Warn("unauthorized c2c access", "user_id", authorID)
return nil
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Reinstate sender gating before resolving unlinked users

Removing the channel-level isAllowed check here (and in the analogous Telegram/Feishu/Weixin handlers) opens the bot to any external sender, because the RBAC path does not stop unlinked accounts. ResolveUser explicitly returns a zero-value user when no identity is linked (internal/channel/identity.go:30-40), and ResolveAgent treats that as hasAuth == false, so canAccess returns true for any enabled agent (internal/channel/identity.go:66-82). Deployments that previously relied on allowed_ids to keep the bot private will now accept arbitrary inbound messages and run agent tools for them.

Useful? React with 👍 / 👎.

Comment on lines 218 to 221
func (b *Bot) Notify(ctx context.Context, n channel.Notification) error {
chatID := n.ChatID
if chatID == "" {
chatID = b.cfg.NotifyChat
}
if chatID == "" {
return fmt.Errorf("feishu: no target chat ID")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Keep a default target for system notification broadcasts

dispatcher.Notify() is still used for system-owned scheduler jobs and heartbeat results with an empty ChatID (cmd/anna/gateway.go:291-299, internal/scheduler/heartbeat.go:104-107, and internal/channel/notify_tool.go:61-66), but this change makes Feishu fail immediately whenever n.ChatID is blank. Since the same commit also removes notify_chat/registered defaults, existing installs that used Feishu for proactive notifications will stop receiving heartbeat, scheduler, and notify broadcasts unless every caller starts passing an explicit chat ID. The same regression exists in weixin.Notify, and Telegram also regresses when only notify_chat was configured.

Useful? React with 👍 / 👎.

@vaayne vaayne merged commit 1cd286b into main Mar 23, 2026
4 of 6 checks passed
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.

1 participant