2026-07-20 11:08:52 +02:00
|
|
|
|
# Channel-Visibility Unification (backlog item 3) — Design
|
|
|
|
|
|
|
2026-08-07 21:20:48 +02:00
|
|
|
|
**Status:** implemented 2026-07-20 (D9) — re-verified 2026-08-04
|
|
|
|
|
|
(`permissions/checker.go:110` `VisibleChannelIDs`; four delegating call sites;
|
|
|
|
|
|
REST/WS agreement test `Server/ws/channel_visibility_agreement_test.go`)
|
2026-07-20 11:08:52 +02:00
|
|
|
|
**Decision:** D9 in [audit-2026-07-19-decisions.md](audit-2026-07-19-decisions.md)
|
|
|
|
|
|
**Closes:** audit finding A-2026-07-07 (backlog §6 item 3)
|
|
|
|
|
|
|
|
|
|
|
|
## Problem
|
|
|
|
|
|
|
|
|
|
|
|
Per the audit, the "does this role see this channel" rule is implemented ~4× with
|
|
|
|
|
|
comments telling each copy it "must mirror" the others:
|
|
|
|
|
|
|
|
|
|
|
|
- `service.ChannelService.ListVisibleChannels` (REST `GET /channels`),
|
|
|
|
|
|
- `ws.buildReady` (the ready payload's channel list),
|
|
|
|
|
|
- `ws.computeAllowedChannels` (replay-buffer filtering on reconnect),
|
|
|
|
|
|
- `ws.Hub.RefreshChannelVisibility` (targeted channel_create/delete after an
|
|
|
|
|
|
override change).
|
|
|
|
|
|
|
|
|
|
|
|
All four inline the same predicate — `HasAdmin(perms) || EffectivePerms(perms,
|
|
|
|
|
|
allow, deny)&ReadMessages == ReadMessages` — plus the same "skip `dm` channels,
|
|
|
|
|
|
fail closed on nil role" scaffolding. The recent private-channel fixes show this
|
|
|
|
|
|
churns; a drift between copies is an information-disclosure bug.
|
|
|
|
|
|
|
|
|
|
|
|
## Approach — funnel all four through the checker that already exists
|
|
|
|
|
|
|
|
|
|
|
|
The unified predicate is **already written**: `permissions.Checker.HasChannelPermBatch`
|
|
|
|
|
|
(pre-fetched overrides map) and `.HasChannelPerm` (single-channel). Three of the
|
|
|
|
|
|
four sites bypass it by re-inlining the bit math; `RefreshChannelVisibility` does
|
|
|
|
|
|
its own `GetChannelPermissions` + `EffectivePerms`. The fix is to route every
|
|
|
|
|
|
site through the checker, not to invent a new abstraction:
|
|
|
|
|
|
|
|
|
|
|
|
1. Add one thin helper to `permissions.Checker`:
|
|
|
|
|
|
`VisibleChannelIDs(rolePerms int64, channels []ChannelRef, overrides map[int64]ChannelOverride) map[int64]bool`
|
|
|
|
|
|
— iterates, skips `dm`, and calls the existing `HasChannelPermBatch` per
|
|
|
|
|
|
channel. `ChannelRef{ID int64; Type string}` is declared in `permissions`
|
|
|
|
|
|
(the package deliberately avoids importing `db`, so callers map their
|
|
|
|
|
|
`[]db.Channel` down to `[]ChannelRef`).
|
|
|
|
|
|
2. `ListVisibleChannels`, `buildReady`, and `computeAllowedChannels` call it.
|
|
|
|
|
|
`buildReady` still fetches the overrides map once (it reuses it for `can_send`
|
|
|
|
|
|
and unread), then hands the same map to `VisibleChannelIDs`.
|
|
|
|
|
|
`computeAllowedChannels` unions the result with the user's open DM channels
|
|
|
|
|
|
(unchanged). Nil role → empty set, which the helper yields naturally.
|
|
|
|
|
|
3. `RefreshChannelVisibility`'s per-role check calls the existing
|
|
|
|
|
|
`HasChannelPerm(rolePerms, roleID, ch.ID, ReadMessages)` instead of its inline
|
|
|
|
|
|
copy. Its targeted-send + `visibilityChangeSeq` watermark mechanics are
|
|
|
|
|
|
untouched.
|
|
|
|
|
|
|
|
|
|
|
|
## Files touched
|
|
|
|
|
|
|
|
|
|
|
|
- `Server/permissions/checker.go` — add `ChannelRef` + `VisibleChannelIDs`.
|
|
|
|
|
|
- `Server/permissions/checker_test.go` — cover the helper (admin bypass, deny
|
|
|
|
|
|
override hides, no-override inherits base, dm skipped).
|
|
|
|
|
|
- `Server/service/channel.go` — `ListVisibleChannels` delegates.
|
|
|
|
|
|
- `Server/ws/serve.go` — `buildReady`, `computeAllowedChannels` delegate; drop
|
|
|
|
|
|
the "mirrors …" comments.
|
|
|
|
|
|
- `Server/ws/hub.go` — `RefreshChannelVisibility` uses `HasChannelPerm`.
|
|
|
|
|
|
- Docs: `docs/audit-2026-07-19.md` (A-2026-07-07 → RESOLVED),
|
|
|
|
|
|
`docs/architecture/server.md`/`websocket.md` if a visibility callout names the
|
|
|
|
|
|
duplication.
|
|
|
|
|
|
|
|
|
|
|
|
## Test plan
|
|
|
|
|
|
|
|
|
|
|
|
- Unit tests for `VisibleChannelIDs` in `permissions`.
|
|
|
|
|
|
- **REST/WS agreement test** (the audit's explicit ask): seed a real in-memory
|
|
|
|
|
|
`db` with roles, per-channel overrides, and text/announcement/voice/dm
|
|
|
|
|
|
channels; assert `ListVisibleChannels`, the channel set inside `buildReady`,
|
|
|
|
|
|
and the non-DM subset of `computeAllowedChannels` are byte-for-byte the same
|
|
|
|
|
|
set for the same user across admin / member-with-deny / nil-role cases.
|
|
|
|
|
|
- Existing `serve_test.go`, `hub_test.go`, `channel_handler` tests stay green
|
|
|
|
|
|
unchanged (behavior is identical; only the code path is shared).
|
|
|
|
|
|
|
|
|
|
|
|
## Non-goals
|
|
|
|
|
|
|
|
|
|
|
|
- No change to permission bits, override storage, or the `EffectivePerms`
|
|
|
|
|
|
algebra — only where the existing predicate is called from.
|
|
|
|
|
|
- DM visibility stays membership-based (added on top of the server-channel set,
|
|
|
|
|
|
not routed through the checker).
|
|
|
|
|
|
- `can_send`, unread counts, and voice-state filtering in `buildReady` are
|
|
|
|
|
|
unchanged.
|
|
|
|
|
|
- Not touching `RefreshChannelVisibility`'s targeted-send / watermark design
|
|
|
|
|
|
(backlog 11/12 territory) — only its read predicate.
|
