Support signed remote Admin in licensed mode#2068
Conversation
|
@coderabbitai review |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughLicensed remote-admin packets are normalized at the outbound send boundary, settings wording and icons vary by local licensing state, localization entries are added, and tests cover licensed, normal, self, broadcast, and unknown-owner scenarios. ChangesRemote Admin Alignment
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Settings
participant AccessoryManager
participant SwiftData
participant Connection
Settings->>AccessoryManager: Request remote admin operation
AccessoryManager->>SwiftData: Fetch connected owner license state
SwiftData-->>AccessoryManager: Return UserEntity.isLicensed
AccessoryManager->>Connection: Send normalized admin packet
Possibly related issues
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
✅ Action performedReview finished.
|
|
The docs-coverage job did not report a source failure; its warning-comment step received |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
MeshtasticTests/RemoteAdminTransportPolicyTests.swift (1)
207-213: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winTest the signature-shield semantics, not only wording.
conditionalWording()checks only localized strings, so a regression that shows PKI iconography in licensed mode—or omits the verified-signature shield—would pass. Add assertions for the licensed and normal icon/signature-state mapping.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MeshtasticTests/RemoteAdminTransportPolicyTests.swift` around lines 207 - 213, The conditionalWording() test verifies only text and misses iconography and signature-state regressions. Add assertions covering both licensed and unlicensed modes, confirming the licensed path uses the signed/verified-signature shield mapping while the normal path uses the PKI icon and corresponding unverified state; reference the existing RemoteAdminWording icon and signature-state APIs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@MeshtasticTests/RemoteAdminTransportPolicyTests.swift`:
- Around line 1-4: Replace the placeholder header comments at the top of
RemoteAdminTransportPolicyTests.swift with the required file-level header, such
as “// MARK: RemoteAdminTransportPolicyTests”, or an approved file-level
copyright comment.
- Around line 144-158: Update normalRemoteAdminIsUnchanged to create the remote
destination user with isLicensed: true before sending the packet, while keeping
the owner non-licensed. Retain the existing assertions for channel, encryption,
and public key to verify normal-mode behavior remains unchanged with a licensed
destination.
---
Nitpick comments:
In `@MeshtasticTests/RemoteAdminTransportPolicyTests.swift`:
- Around line 207-213: The conditionalWording() test verifies only text and
misses iconography and signature-state regressions. Add assertions covering both
licensed and unlicensed modes, confirming the licensed path uses the
signed/verified-signature shield mapping while the normal path uses the PKI icon
and corresponding unverified state; reference the existing RemoteAdminWording
icon and signature-state APIs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ddb5b4ac-daca-4447-bb39-5fb36cec2843
📒 Files selected for processing (5)
Localizable.xcstringsMeshtastic.xcodeproj/project.pbxprojMeshtastic/Accessory/Accessory Manager/AccessoryManager.swiftMeshtastic/Views/Settings/Settings.swiftMeshtasticTests/RemoteAdminTransportPolicyTests.swift
|
Addressed the remaining CodeRabbit presentation test request in ea84f7f: a shared mapping now drives the licensed signed/verified shield and normal PKI icon, with both states asserted. @coderabbitai review |
|
✅ Action performedReview finished.
|
Closes #2067
Design: meshtastic/design#123
Firmware dependency: meshtastic/firmware#10972
Summary
ADMIN_APPtraffic uses channel 0 withpkiEncrypted=falseand an empty public key;This aligns the client with the existing XEdDSA signing path. It does not add a new authentication protocol or bypass firmware authorization/session checks.
Validation
git diff --check,jq empty Localizable.xcstrings, andplutil -lint Meshtastic.xcodeproj/project.pbxprojpass;ea84f7f0ce58f83f8e977261d16db8b854a22c4f.Coverage includes licensed direct Admin, shared device-config requests, normal mode, self, unknown owner, exact conditional wording, and broadcast preservation. Generated Xcode package and asset artifacts are excluded.
Draft / hardware status
This remains draft until firmware#10972 is validated with two licensed RF nodes. Required hardware checks include signed plaintext getter/session/mutation/reply traffic, rejection of broadcast/unauthorized/stale-session requests, and unchanged normal-mode PKI behavior.
Summary by CodeRabbit
New Features
Bug Fixes
Tests