RemoteID: restore operator ID validation and sanitizing - #14262
RemoteID: restore operator ID validation and sanitizing#14262Junior00619 wants to merge 3 commits into
Conversation
|
Thanks for your first pull request! 🎉 A maintainer will review this soon. Please ensure:
We appreciate your contribution to QGroundControl! |
Build ResultsPlatform Status
All builds passed. Pre-commit
Pre-commit hooks: 4 passed, 32 failed, 7 skipped. Test Resultslinux-sanitizers: 68 passed, 0 skipped Artifact Sizes
|
|
@Davidsastresas Can you look at this and verify this is a correct thing to do? |
|
Thanks for tackling this, wiring validation to the Fact signals is the right approach, since the generated settings UI writes the A couple of things before merge: 1. The PR description doesn't match the tests that shipped. The summary lists coverage for "switching from FAA to EU after entering a validated full ID," but that case isn't in the diff, there are only 2. Minor:
|
There was a problem hiding this comment.
Pull request overview
This PR restores Remote ID Operator ID validation/sanitization within RemoteIDManager so EU Operator IDs are checksum-validated and any validated “full” Operator ID is sanitized back to the public 16-character form before being stored/broadcast. It also adds integration test scaffolding and CMake registration for RemoteIDManagerTest.
Changes:
- Reconnect Operator ID validation to
RemoteIDSettings::operatorIDchanges and centralize state refresh to prevent the settings UI from bypassing EU checksum validation. - Sanitize validated EU Operator IDs down to the public 16-character value when stored.
- Add initial
RemoteIDManagerTestcoverage and wire it into the test build.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/Vehicle/RemoteIDManager.h |
Adds private helpers/guard for Operator ID change handling and state refresh. |
src/Vehicle/RemoteIDManager.cc |
Implements Operator ID validation on Fact changes and sanitizes validated EU IDs to 16 chars. |
test/Vehicle/RemoteIDManagerTest.h |
Introduces a new vehicle integration test class for Remote ID operator ID behavior. |
test/Vehicle/RemoteIDManagerTest.cc |
Adds tests for EU Operator ID sanitization and invalid-ID trusted-state clearing. |
test/Vehicle/CMakeLists.txt |
Adds the new RemoteID test sources to the vehicle test target. |
test/CMakeLists.txt |
Registers RemoteIDManagerTest as an integration/vehicle test. |
| // We check whether it actually changed to avoid triggering this on startup. | ||
| if (operatorID != _settings->operatorID()->rawValueString()) { | ||
| _settings->operatorIDValid()->setRawValue(_isEUOperatorIDValid(operatorID)); | ||
| const bool operatorIDValid = (operatorID.length() > 16) && _isEUOperatorIDValid(operatorID); |
| const QString operatorID = value.toString(); | ||
| const bool operatorIDValid = (operatorID.length() > 16) && _isEUOperatorIDValid(operatorID); | ||
| if (_settings->operatorIDValid()->rawValue().toBool() != operatorIDValid) { | ||
| _settings->operatorIDValid()->setRawValue(operatorIDValid); | ||
| } |
| private slots: | ||
| void init() override; | ||
| void cleanup() override; | ||
|
|
||
| void _validEUOperatorIDIsSanitized(); | ||
| void _invalidEUOperatorIDClearsTrustedState(); | ||
|
|
|
This is still out of whack. This may validate the id internally and not send crap, but it doesn't tell the user through the ui that the id is bad like it used to. The other part of this got screwed up with the change the json settings page definitions. I'll take a look at taking this and also in combination fixing up the UI in a replacement pull that is a superset of this. |
|
@Davidsastresas Questions on stored settings: The current/original code allows storage of invalid operator ids into settings. Is there some specific spec/usage need for that? If instead it showed you the validation error and didn't persist bad values would that be a bad thing? |
|
@Junior00619 @Davidsastresas Can you try a new daily sometime after today? |
|
Sorry for the late response @DonLakeFlyer. I don't have anymore access to remote ID hardware so I can not help with testing at the moment. On the stored-settings question — no, there's no spec or usage requirement to persist invalid operator IDs. Looking back at the original code it's really just a side effect of two things:
So an invalid value only lingers in settings because nothing overwrites it until you type a valid one. In practice it's harmless — the send path is already gated on _operatorIDGood, so an invalid ID is never broadcast — but there's no reason it has to persist. So showing the validation error and not persisting bad values wouldn't break anything, and it's cleaner. The only constraint to keep is that the field still needs the full value (incl. the secret) transiently to validate, while only the 16-char public part is what actually gets stored/sent. Your superset approach sounds right to me. |
|
Great, thanks for the info @Davidsastresas |
Fixes #14204.
Summary
RemoteIDManagerso the generated settings UI no longer bypasses the EU checksum pathRemoteIDManagerTestcoverage for valid EU IDs, invalid EU IDs, and switching from FAA to EU after entering a validated full IDVerification
-DQGC_BUILD_TESTING=ONMAVLinkEnums.hin the build treecompile_commands.jsoninside the container:src/Vehicle/RemoteIDManager.cctest/Vehicle/RemoteIDManagerTest.ccI did not finish a full
QGroundControl --unittest:RemoteIDManagerTestrun locally because the complete application build is much larger than the changed scope here, so the remaining end-to-end validation should come from CI.