Skip to content

feat(mqtt): route MQTT channel permissions through channel_database#3108

Merged
Yeraze merged 1 commit into
mainfrom
feat/mqtt-channel-permissions
May 20, 2026
Merged

feat(mqtt): route MQTT channel permissions through channel_database#3108
Yeraze merged 1 commit into
mainfrom
feat/mqtt-channel-permissions

Conversation

@Yeraze

@Yeraze Yeraze commented May 20, 2026

Copy link
Copy Markdown
Owner

Summary

The Permissions dialog showed an 8-slot channel grid (channel_0..channel_7) for every source type, including MQTT. For MQTT that grid keyed permissions on the sender's device slot (packet.channel), which is unstable: two upstream nodes that use different channels in their own slot 0 fight over the same (sourceId, slot) row, and a grant on channel_0 silently protects whichever channel name won the slot race most recently.

This PR re-keys MQTT channel permissions on the existing channel_database (Virtual Channels) — a global, name-based registry that already has its own per-user permission table (channel_database_permissions). No schema migration is needed: nodeEnhancer.ts already enforces virtual-channel permissions whenever a row's channel value is >= CHANNEL_DB_OFFSET (100), with channelDatabaseId = channel - 100.

Changes

  • src/server/mqttIngestion.ts — on every MQTT ingest, resolve a channelDatabaseId (priority: server-decrypt result, then envelope.channelId via name lookup with find-or-create), then stamp the persisted channel column as CHANNEL_DB_OFFSET + id. Falls back to the raw slot when nothing resolves. Passes effectiveChannel to traceroute / store-forward writes too. Adds a per-process name→id cache and a _resetMqttIngestCachesForTest export.
  • src/db/repositories/channelDatabase.ts — new findOrCreatePassiveByNameAsync(name) that creates a disabled (no-PSK) row for the name if none exists, so the permission target is always materialized without affecting decryption.
  • src/components/UsersTab.tsx — when the per-user permission scope is an mqtt_broker or mqtt_bridge, the slot grid hides channel_0..7 rows and shows a hint pointing to the existing Virtual Channel Permissions section below.
  • Tests — 6 new MQTT-ingest cases (encoded channel for known/unknown name, decrypt-path preference, fallback, traceroute encoding, name-lookup memoization), 1 new UsersTab case (slot grid hidden for MQTT scope), and a flakiness fix in mqttBrokerManager.test.ts so it waits for the ingestion .then chain (my added await introduced a microtask race).

Out of scope

  • Historical rows with raw 0..7 channel values keep the old channel_${slot} enforcement until they age out. The cutover is forward-only.
  • The per-source channels table is still populated by recordChannelFromEnvelope for the channel picker — unchanged.

Test plan

  • npx vitest run — 5179/5179 pass
  • npx tsc -p tsconfig.server.json --noEmit — clean
  • npx tsc -p tsconfig.json --noEmit — clean (UsersTab)
  • Manual: connect an MQTT broker, observe that messages on LongFast show up under channel_database permissions, and that an admin can grant/revoke a non-admin user access via the Virtual Channel Permissions section in the user editor.
  • Manual: open the Permissions dialog with an MQTT source selected — confirm the channel_0..7 rows are gone and the hint banner is shown.

🤖 Generated with Claude Code

The slot index in `packet.channel` is the *sender's* device slot — when
two upstream nodes use different channels in their own slot 0 (extremely
common on a public broker), the same per-source `channels` row gets its
name flipped on every packet, and channel_0..7 grants protect "whichever
name won the slot race." That's the wrong identity for MQTT.

Instead, resolve every MQTT-ingested packet to a channel_database row:
either the one server-side decryption already identified, or one looked
up by `envelope.channelId`. Unknown names auto-register a passive row
(isEnabled=false, no PSK) so admins can grant access without forcing
operators to pre-declare every observed channel. The persisted `channel`
column gets the existing `CHANNEL_DB_OFFSET + id` encoding nodeEnhancer
already enforces, so no schema migration is required — slot fallback is
preserved when no name resolves.

UI: the per-user Permissions dialog hides the channel_0..7 grid when an
MQTT source is selected and points admins at the existing Virtual
Channel Permissions section below.

Out of scope:
 - Historical MQTT rows with raw 0..7 channel values keep the old
   channel_${slot} enforcement until they age out.
 - The per-source `channels` table is still populated by
   recordChannelFromEnvelope (channel picker), unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented May 20, 2026

Copy link
Copy Markdown

Claude finished @Yeraze's task —— View job


Code Review Complete ✅

This is a high-quality PR that addresses a real stability issue in MQTT channel permissions with an elegant, well-tested solution.

🎯 Summary

The PR fixes unstable MQTT channel permissions by routing them through the existing channel_database system instead of using collision-prone device slots. The solution is backward-compatible and maintains excellent performance through smart caching.

Strengths

Architecture & Design

  • Excellent Problem Analysis: Clearly identifies and solves the slot collision issue where different nodes using different channels in slot 0 would fight over the same permission row
  • Clean Reuse: Leverages existing channel_database infrastructure with the clever CHANNEL_DB_OFFSET + id encoding
  • Backward Compatibility: Historical rows with raw slots continue working until they age out

Implementation Quality

  • Comprehensive Error Handling: Proper fallbacks ensure ingestion never fails due to permission resolution issues (src/server/mqttIngestion.ts:852-857)
  • Performance Optimization: Process-lifetime caching prevents repeated DB lookups (channelNameToDbIdCache)
  • Type Safety: Excellent TypeScript usage with proper null checks and interfaces

Test Coverage

  • 130+ new test lines covering all critical paths:
    • Channel resolution via envelope.channelId
    • Server-side decrypt path preference over name lookup
    • Cache memoization behavior
    • Fallback scenarios (missing names, null responses)
    • UI integration tests for MQTT scope handling
  • Edge case coverage: Empty names, concurrent packets, traceroute encoding

🔍 Minor Suggestions for Future Enhancement

Performance Considerations

  • src/db/repositories/channelDatabase.ts:104: The case-insensitive name lookup iterates all entries client-side rather than using SQL LOWER() comparison - could be optimized for large channel databases
  • Cache Strategy: Process-lifetime only cache could benefit from invalidation hooks for external DB modifications

Security & Validation

  • Input Validation: Channel names from MQTT are trimmed but not sanitized - consider validation for production safety
  • Permission Cleanup: No automatic cleanup when channel database entries are deleted (though this may be intentional)

🧪 Test Quality Assessment

Excellent coverage including:

  • ✅ Happy path scenarios with proper mocking
  • ✅ Error conditions and graceful fallbacks
  • ✅ Cache behavior verification
  • ✅ UI state management for MQTT sources
  • ✅ Cross-component integration (permissions dialog)

Test architecture highlight: The _resetMqttIngestCachesForTest export enables clean test isolation without complex mocking.

🎯 Final Recommendation: APPROVE

This PR successfully:

  1. Solves the core stability issue with MQTT channel permissions
  2. Maintains backward compatibility and existing functionality
  3. Provides comprehensive test coverage for all scenarios
  4. Follows MeshMonitor architecture patterns correctly
  5. Includes thorough manual testing checklist

The implementation is production-ready and represents a significant improvement to MQTT source reliability. The minor suggestions above are enhancements for future consideration, not blockers.

Manual Testing: The provided checklist covers the key scenarios. I recommend testing the permission inheritance behavior when switching between sources to ensure the UI state transitions correctly.

@Yeraze Yeraze merged commit 47cd26c into main May 20, 2026
16 checks passed
@Yeraze Yeraze deleted the feat/mqtt-channel-permissions branch May 20, 2026 17:03
Yeraze added a commit that referenced this pull request May 20, 2026
…ons (#3110)

* fix(mqtt): stamp node.channel on ingest so the map filter honors VC perms

#3108 routed MQTT message ingest through `channel_database` and stamped
`message.channel = CHANNEL_DB_OFFSET + dbId`. The matching change to
the node-upsert paths was missed: `NODEINFO_APP` and `POSITION_APP`
built `Partial<DbNode>` rows without setting `channel`, so the column
stayed NULL.

`filterNodesByChannelPermission` in `nodeEnhancer.ts` then evaluated
`node.channel ?? 0` for every MQTT-sourced node, falling back to slot
0 and requiring `permissions[channel_0]?.viewOnMap`. But the #3108 UI
hides the channel_0..7 toggles for MQTT scopes and directs admins to
Virtual Channel Permissions — leaving no way to grant map access.
Result: anonymous and non-admin users saw "No nodes visible" on the
map for every MQTT source, no matter what they granted.

Fix: pass `channel: effectiveChannel` into the upsertNode calls in the
NODEINFO_APP and POSITION_APP branches, matching the message-side
encoding. Existing rows that already have channel=NULL recover as
each node re-broadcasts NODEINFO (typically every few hours under
default firmware config); no migration backfill needed for short-
term correctness.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(sources): channel-gate traceroutes and neighbor-info on the map

Companion to the node.channel stamping fix in the same PR. With nodes
correctly filtered out for users who lack viewOnMap on the channel,
two endpoints still leaked enough position data to render lines:

1. GET /api/sources/:id/traceroutes returned every row including the
   embedded `routePositions` JSON snapshot. The frontend's
   `useTraceroutePaths` hook prefers that snapshot over the live nodes
   list, so it drew line segments between coordinates of nodes the
   user had no permission to see. Apply `maskTraceroutesByChannel` to
   the response.
2. GET /api/sources/:id/neighbor-info enriched each record with
   positions pulled from `getNodesByNums` without checking channel
   access. Build the visible-node set via `filterNodesByChannelPermission`
   and drop neighbor records whose endpoints aren't in it.

User-visible symptom this fixes: anonymous viewers seeing "lines
floating in space" on the map for MQTT sources whose nodes had been
correctly filtered out of the nodes endpoint.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Yeraze added a commit that referenced this pull request May 20, 2026
* chore(release): bump version to 4.6.3

Five user-visible files bumped per the CLAUDE.md version recipe:
package.json, package-lock.json (regenerated), helm/meshmonitor/Chart.yaml,
desktop/src-tauri/tauri.conf.json, desktop/package.json. CLAUDE.md
banner line bumped to match.

CHANGELOG entry covers the five PRs since 4.6.2-1:
- #3105 unified tapback metadata fix
- #3106 docs: drop worktree restriction
- #3107 meshcore contact advType persistence
- #3108 MQTT channel permissions via channel_database
- #3109 hint banner Catppuccin restyle
- #3110 node.channel ingest + traceroute/neighbor channel gate

Companion blog post (docs/blog/2026-05-20-v4.6.3-permissions.md)
walks operators through the new Virtual Channel Permissions flow,
the map-visibility behavior change, and the floating-lines fix.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* chore(release): regenerate desktop/package-lock.json for 4.6.3

The desktop sub-project carries its own lockfile and the bump to 4.6.3
left it pinned to 4.6.1. The Windows Desktop CI job runs `npm install`
without `--legacy-peer-deps` and fails on the package.json /
package-lock.json version mismatch. Regenerate to match.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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