[feature] Add protocol version validation for member and joiner #248
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…into feature/protocol-version-check
This reverts commit b13ed85
- Add MockServerFacade and FakeHubPushHandler test helpers - Add ProtocolVersion_IntegrationTests with 3 test scenarios: * Server returns protocol version incompatible * Server returns success * Joiner with incompatible version sends notification - Mock IPublicKeysManager and ITrustProcessPublicKeysRepository to avoid blocking on async operations
# Conflicts: # tests/ByteSync.Client.UnitTests/Services/Communications/PublicKeysTrusterTests.cs
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements comprehensive protocol version validation to prevent connections between clients with incompatible protocol versions. The implementation spans server-side validation, client-side validation, SignalR notification infrastructure, and extensive test coverage.
Key Changes
- Server-side validation: Added
IsProtocolVersionIncompatibleflag toStartTrustCheckResultand implemented version checking inStartTrustCheckCommandHandlerto validate both joiner and member protocol versions before initiating trust checks - Member-side validation: Added early validation in
PublicKeysTruster.OnPublicKeyCheckDataAskedAsyncto check joiner's protocol version and send notification if incompatible - SignalR notification flow: Created
InformProtocolVersionIncompatibleParametersclass, new SignalR method inIHubByteSyncPush, Azure Function endpoint inTrustFunction, and client-side handler inHubPushHandler2andPublicKeyCheckDataPushReceiver
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
src/ByteSync.Common/Business/Sessions/Cloud/Connections/StartTrustCheckResult.cs |
Added IsProtocolVersionIncompatible property to indicate version incompatibility |
src/ByteSync.Common/Business/Sessions/Cloud/Connections/InformProtocolVersionIncompatibleParameters.cs |
New parameter class for protocol version incompatibility notifications |
src/ByteSync.Common/Interfaces/Hub/IHubByteSyncPush.cs |
Added InformProtocolVersionIncompatible method to SignalR interface |
src/ByteSync.ServerCommon/Commands/Trusts/StartTrustCheckCommandHandler.cs |
Implemented server-side validation logic for protocol versions |
src/ByteSync.ServerCommon/Commands/Trusts/InformProtocolVersionIncompatibleRequest.cs |
New request class for MediatR command |
src/ByteSync.ServerCommon/Commands/Trusts/InformProtocolVersionIncompatibleCommandHandler.cs |
Handler to forward incompatibility notifications to joiner |
src/ByteSync.Functions/Http/TrustFunction.cs |
New Azure Function endpoint for protocol version notifications |
src/ByteSync.Client/Business/Communications/PublicKeysTrusting/JoinerTrustProcessData.cs |
Added tracking for incompatible members and early termination logic |
src/ByteSync.Client/Services/Communications/TrustProcessPublicKeysRepository.cs |
Added methods to set and check protocol version incompatibility |
src/ByteSync.Client/Services/Communications/PublicKeysTruster.cs |
Implemented member-side validation and joiner-side incompatibility handling |
src/ByteSync.Client/Services/Communications/Api/TrustApiClient.cs |
Added client method to call protocol incompatibility endpoint |
src/ByteSync.Client/Services/Communications/SignalR/HubPushHandler2.cs |
Added SignalR subject for incompatibility notifications |
src/ByteSync.Client/Services/Communications/PushReceivers/PublicKeyCheckDataPushReceiver.cs |
Subscribed to and handled incompatibility notifications |
src/ByteSync.Client/Interfaces/Controls/Communications/ITrustProcessPublicKeysRepository.cs |
Added interface methods for protocol version tracking |
src/ByteSync.Client/Interfaces/Controls/Communications/Http/ITrustApiClient.cs |
Added interface method for incompatibility notification |
src/ByteSync.Client/Interfaces/Controls/Communications/SignalR/IHubPushHandler2.cs |
Added interface property for incompatibility subject |
tests/ByteSync.ServerCommon.Tests/Commands/Trusts/StartTrustCheckCommandHandlerTests.cs |
Added 4 comprehensive test scenarios covering all version compatibility cases |
tests/ByteSync.ServerCommon.Tests/Commands/Trusts/InformProtocolVersionIncompatibleCommandHandlerTests.cs |
New test file with 2 tests for the incompatibility command handler |
tests/ByteSync.Functions.UnitTests/Http/TrustFunctionTests.cs |
New test for Azure Function endpoint |
tests/ByteSync.Client.UnitTests/Services/Communications/PublicKeysTrusterTests.cs |
Added 6 unit tests for protocol version validation scenarios |
tests/ByteSync.Client.UnitTests/Services/Communications/Api/TrustApiClientTests.cs |
New test file for TrustApiClient incompatibility method |
tests/ByteSync.Client.UnitTests/Services/Communications/PushReceivers/PublicKeyCheckDataPushReceiverTests.cs |
New test for push receiver incompatibility handling |
tests/ByteSync.Client.IntegrationTests/Scenarios/SessionConnection/ProtocolVersion_IntegrationTests.cs |
New integration test file with 4 end-to-end test scenarios |
tests/ByteSync.Client.IntegrationTests/TestHelpers/Server/MockServerFacade.cs |
New test helper for mocking server behavior |
tests/ByteSync.Client.IntegrationTests/TestHelpers/Server/FakeHubPushHandler.cs |
New test helper for fake SignalR push handler |
Comments suppressed due to low confidence (1)
src/ByteSync.ServerCommon/Commands/Trusts/InformProtocolVersionIncompatibleCommandHandler.cs:21
- This assignment to client is useless, since its value is never read.
var client = request.Client;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/ByteSync.ServerCommon/Commands/Trusts/InformProtocolVersionIncompatibleRequest.cs
Show resolved
Hide resolved
src/ByteSync.ServerCommon/Commands/Trusts/InformProtocolVersionIncompatibleCommandHandler.cs
Outdated
Show resolved
Hide resolved
...Sync.Client.IntegrationTests/Scenarios/SessionConnection/ProtocolVersion_IntegrationTests.cs
Show resolved
Hide resolved
| { | ||
| var sessionId = "test-session-123"; | ||
| var joinerInstanceId = "joiner-instance-id"; | ||
| var memberInstanceId = "joiner-instance-id"; |
There was a problem hiding this comment.
The memberInstanceId is incorrectly set to the same value as joinerInstanceId ("joiner-instance-id"). This appears to be a copy-paste error. The member and joiner should have different instance IDs. The verification on line 153 expects MemberClientInstanceId to be different from JoinerClientInstanceId.
Suggested fix:
var memberInstanceId = "member-instance-id";Also, the member instance ID is set up on line 71 in the Setup method as "joiner-instance-id", which should probably be updated to match the corrected variable name.
Suggested change
| var memberInstanceId = "joiner-instance-id"; | |
| var memberInstanceId = "member-instance-id"; |
Contributor
Author
There was a problem hiding this comment.
Fixed in commit d8805ac. Corrected the joiner/member instance IDs to be distinct values.
tests/ByteSync.Client.IntegrationTests/TestHelpers/Server/FakeHubPushHandler.cs
Outdated
Show resolved
Hide resolved
...lient.UnitTests/Services/Communications/PushReceivers/PublicKeyCheckDataPushReceiverTests.cs
Show resolved
Hide resolved
...lient.UnitTests/Services/Communications/PushReceivers/PublicKeyCheckDataPushReceiverTests.cs
Show resolved
Hide resolved
...lient.UnitTests/Services/Communications/PushReceivers/PublicKeyCheckDataPushReceiverTests.cs
Show resolved
Hide resolved
...lient.UnitTests/Services/Communications/PushReceivers/PublicKeyCheckDataPushReceiverTests.cs
Show resolved
Hide resolved
...lient.UnitTests/Services/Communications/PushReceivers/PublicKeyCheckDataPushReceiverTests.cs
Show resolved
Hide resolved
…ionSource for robust async test - Remove unused 'client' variable in InformProtocolVersionIncompatibleCommandHandler - Remove extra blank lines at end of files - Refactor foreach with implicit filtering to use explicit .Where() - Fix joiner/member instance ID confusion in integration test
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Complete implementation of protocol version validation to block connections between clients with incompatible versions.
Main Changes
Member-side validation
SignalR notification flow
Server-side validation
Integration tests