fix: issues with letter sealing off groups#216
Conversation
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughUpdates E2EE group-key fetching, joined-member discovery, registration fallback, and related tests. ChangesE2EE group-key handling
FlexibleMidMap decoding
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant autoRegisterGroupKey
participant getChatMemberMIDs
participant GetChats
participant registerGroupKey
participant E2EE
autoRegisterGroupKey->>getChatMemberMIDs: discover joined member MIDs
getChatMemberMIDs->>GetChats: retrieve chat metadata
GetChats-->>getChatMemberMIDs: members and pending invitees
getChatMemberMIDs-->>autoRegisterGroupKey: joined MIDs and pending flag
autoRegisterGroupKey->>registerGroupKey: register group key
registerGroupKey->>E2EE: generate and wrap group key
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Comment |
![indent-zero[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
| if !errors.Is(err, line.ErrNoUsableE2EEGroupKey) { | ||
| t.Fatalf("error = %v, want ErrNoUsableE2EEGroupKey", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Test doesn't exercise the intended path: newPeerKeyTestClient() returns a LineClient with E2EE unset, so registerGroupKey returns via the new lc.E2EE == nil guard on the very first line — the filtered-to-self member handling this PR added is never touched. Consider either (a) renaming this to reflect that it covers the E2EE-nil case, or (b) constructing a client with a real (or stub) E2EE manager and asserting that registerGroupKey(ctx, chatMid, []string{lc.Mid}) now proceeds to a self-only RegisterE2EEGroupKey call instead of erroring — which is the actual behavior change relative to the removed no other members to register group key for error.
| // getChatMemberMIDs fetches joined member MIDs for a group chat via GetChats. | ||
| // Pending invitees are deliberately excluded: LINE validates group keys against the | ||
| // current joined-member set and rejects keys that include invitees. | ||
| func (lc *LineClient) getChatMemberMIDs(ctx context.Context, chatMid string) ([]string, bool, error) { |
There was a problem hiding this comment.
Doc missing new return value: Comment above explains "why invitees are excluded" but doesn't document that the signature now returns (mids []string, hasPendingInvitees bool, error). A short line noting that the bool is used by autoRegisterGroupKey to gate self-only fallbacks would keep the contract clear for future readers.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/line/structs_test.go (1)
9-29: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExpand coverage for the new object-decoding contract.
The decoder now ignores arbitrary object value types and preserves
nullas a nil map, but the table only covers strings, booleans, and arrays. Add mixed numeric/null/nested values and assertgot == nilfor JSONnull.Suggested test extension
want []string + wantNil bool @@ + { + name: "mixed valued object", + data: `{"U-one":1,"U-two":null,"U-three":[],"U-four":{}}`, + want: []string{"U-one", "U-two", "U-three", "U-four"}, + }, + { + name: "null", + data: `null`, + wantNil: true, + }, @@ + if test.wantNil != (got == nil) { + t.Fatalf("decoded map nil = %v, want %v", got == nil, test.wantNil) + }Also applies to: 31-45
🤖 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 `@pkg/line/structs_test.go` around lines 9 - 29, Expand the table-driven coverage in the visible test cases to include an object containing mixed numeric, null, and nested values, asserting the expected keys regardless of value types. Add a JSON null case whose expected result is nil, and update the test assertions to compare the decoded result with the expected nil map correctly.pkg/connector/e2ee_keys_test.go (1)
246-269: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider adding coverage for the
hasPendingInviteesfallback gating inautoRegisterGroupKey.
joinedGroupMemberMIDsitself is well covered, but the consuming gate (len(members) == 1 && members[0] == lc.Mid && !hasPendingInviteesinautoRegisterGroupKey, e2ee_keys.go lines 289 and 302) — the actual behavior change this PR is fixing — has no direct test confirming that a single-joined-member group with pending invitees skips the cached/Matrix fallback.🤖 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 `@pkg/connector/e2ee_keys_test.go` around lines 246 - 269, Add a direct autoRegisterGroupKey test covering a group with only the local member joined and pending invitees, asserting the cached/Matrix fallback is skipped when hasPendingInvitees is true. Exercise the consuming len(members) == 1, local-MID, and !hasPendingInvitees gate in autoRegisterGroupKey rather than only testing joinedGroupMemberMIDs.
🤖 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.
Nitpick comments:
In `@pkg/connector/e2ee_keys_test.go`:
- Around line 246-269: Add a direct autoRegisterGroupKey test covering a group
with only the local member joined and pending invitees, asserting the
cached/Matrix fallback is skipped when hasPendingInvitees is true. Exercise the
consuming len(members) == 1, local-MID, and !hasPendingInvitees gate in
autoRegisterGroupKey rather than only testing joinedGroupMemberMIDs.
In `@pkg/line/structs_test.go`:
- Around line 9-29: Expand the table-driven coverage in the visible test cases
to include an object containing mixed numeric, null, and nested values,
asserting the expected keys regardless of value types. Add a JSON null case
whose expected result is nil, and update the test assertions to compare the
decoded result with the expected nil map correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d2849d0a-78aa-485d-a45f-c1e9e8c26238
📒 Files selected for processing (5)
pkg/connector/creategroup.gopkg/connector/e2ee_keys.gopkg/connector/e2ee_keys_test.gopkg/line/structs.gopkg/line/structs_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: build-docker
- GitHub Check: Lint with 1.25
- GitHub Check: build-docker
- GitHub Check: Lint with 1.25
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Usego fmtfor code formatting across all Go files
Usegoimportswith-local "github.com/highesttt/matrix-line-messenger"flag to group project-local imports correctly
Usezerologfor logging throughout the codebase
Do not useMsgfin logging; useMsgwith structured fields instead
UseStringerinterface where applicable in Go code
Files:
pkg/line/structs_test.gopkg/line/structs.gopkg/connector/creategroup.gopkg/connector/e2ee_keys.gopkg/connector/e2ee_keys_test.go
**/!(ltsm)/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/!(ltsm)/**/*.go: Runstaticcheckon all Go files excludingpkg/ltsmpackage (transpiled WASM code)
Rungo veton all Go files excludingpkg/ltsmpackage (transpiled WASM code)
Files:
pkg/line/structs_test.gopkg/line/structs.gopkg/connector/creategroup.gopkg/connector/e2ee_keys.gopkg/connector/e2ee_keys_test.go
🔇 Additional comments (10)
pkg/line/structs.go (1)
8-17: LGTM!pkg/connector/e2ee_keys.go (5)
26-31: LGTM!
43-51: LGTM!
221-244: LGTM!
joinedGroupMemberMIDscorrectly dedups and excludes non-user MIDs, and the updatedgetChatMemberMIDssignature/doc clearly documents the invitee-exclusion rationale.
278-316: LGTM!The
!hasPendingInviteesguard correctly prevents overwriting a genuinely-single-joined-member state (with pending invitees) with stale cached/Matrix member lists that would otherwise incorrectly include invitees in group-key registration.
244-276: 🎯 Functional CorrectnessCheck for any remaining two-value
getChatMemberMIDscall sites, including tests.pkg/connector/creategroup.go (2)
73-81: LGTM!
213-295: LGTM!The added
lc.E2EE == nilguard prevents a nil-pointer panic onlc.E2EE.GenerateGroupKey()that would otherwise occur when this is called with an uninitialized E2EE manager, and skippingresolveGroupMemberPublicKeyswhenmembersis empty avoids an unnecessary batch call while still registering a self-only key.pkg/connector/e2ee_keys_test.go (2)
214-234: LGTM!
236-244: 🎯 Functional CorrectnessConfirm
newPeerKeyTestClientkeeps this test off the real RPC pathpkg/connector/e2ee_keys_test.go:236-244
IfnewPeerKeyTestClient()leaveslc.E2EEnon-nil,registerGroupKeywill reachclient.RegisterE2EEGroupKey(...)and the failure source may be transport-related rather thanline.ErrNoUsableE2EEGroupKey. Either nil outlc.E2EEhere or stub the RPC transport so the assertion exercises the intended path.
1 participant
No description provided.