fix: decode and encode extras on presence messages (TP3i) - #2186
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds support for an Changes
Sequence Diagram(s)sequenceDiagram
participant TestClientA as Client A (test subscriber)
participant TestClientB as Client B (jwt-authenticated publisher)
participant AuthEcho as Echo/Auth server
participant Realtime as Realtime service
Note over TestClientB,AuthEcho: Test obtains JWT with user claim
TestClientB->>AuthEcho: request embedded JWT (extraClaims: userClaim)
AuthEcho-->>TestClientB: JWT (contains extras.userClaim)
TestClientB->>Realtime: connect with JWT and enter presence (includes extras)
Realtime-->>TestClientA: presence enter event delivered (payload includes extras)
TestClientA->>TestClientA: assert presenceMessage.extras.userClaim == expected
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
da96304 to
e9dc9ef
Compare
e9dc9ef to
7f48187
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Test/AblyTests/Tests/RealtimeClientPresenceTests.swift`:
- Around line 3913-3927: subscribeOptions clears the test channel prefix but
jwtOptions does not, so client2 ends up using a different channel; after
obtaining jwtOptions (from AblyTests.clientOptions(for:)), set
jwtOptions.testOptions.channelNamePrefix = nil so client2 uses the exact same
channel name as client1 (affecting jwtOptions/client2/channelNamePrefix).
🧹 Nitpick comments (1)
Test/AblyTests/Tests/RealtimeClientPresenceTests.swift (1)
3957-3988: Consider extracting JWT helpers into a shared test utility.
signJWTandbase64urlEncodeare general-purpose test helpers. If other test files need JWT signing in the future (e.g., for message-level user claims), having them in a shared location would avoid duplication.Not blocking — fine to defer to a follow-up if the scope is intentionally limited.
7f48187 to
c68836b
Compare
c68836b to
0ea6710
Compare
0ea6710 to
89ee33f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@Source/ARTJsonLikeEncoder.m`:
- Around line 448-449: Remove the trailing whitespace characters reported by the
linter: delete the extra spaces at the end of the line containing
"message.extras = [input objectForKey:@\"extras\"];" and at the other
trailing-whitespace occurrences around the block at lines corresponding to
757-760 in Source/ARTJsonLikeEncoder.m so the file has no trailing spaces on
those lines; ensure you save the file with those spaces removed.
In `@Test/AblyTests/Tests/RealtimeClientPresenceTests.swift`:
- Around line 3933-3940: The test incorrectly asserts that presence messages
carry a JWT-derived userClaim (it inspects message.extras as NSDictionary and
checks userClaim), but Ably docs state userClaim injection does not apply to
presence/metadata; either remove this presence-related test entirely or change
the assertion to reflect server behavior (e.g., assert userClaim is nil on
presence messages) and add a brief comment referencing the Ably docs; update the
code paths involving message.extras and the userClaim variable
(presenceMessage/message.extras/userClaim) accordingly so the test matches
actual Ably behavior.
🧹 Nitpick comments (1)
Test/AblyTests/Tests/RealtimeClientPresenceTests.swift (1)
3929-3951: Test could hang indefinitely if presence enter fails before subscribe fires.If
channel2.presence.enterfails (e.g., due to the channel prefix mismatch noted above, or an auth issue), the error handler callsdone()but theXCTFailmessage might be missed sincedone()is only called once. More importantly, ifentersucceeds but the subscribe callback never fires (e.g., wrong channel), the test hangs until timeout with no diagnostic. Consider adding a timeout-based failure message or subscribing before entering to ensure ordering.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Test/AblyTests/Test` Utilities/TestUtilities.swift:
- Around line 589-590: Update the incomplete comment above the JWT helper in
TestUtilities.swift so the sentence reads correctly; replace the fragment "it
will the echoserver" with a proper verb (for example "it will use the
echoserver") so the full comment becomes: "By default this will sign the token
locally, but for some cases (e.g. embedded JWTs), it will use the echoserver
that has this functionality." Target the comment block that starts "Gets a JWT
for use in tests." to ensure clarity.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Test/AblyTests/Test Utilities/TestUtilities.swift (1)
636-674: MarksignJWTandbase64urlEncodeasprivate.Both helpers are only called within this file—
signJWTis called once fromgetJWTToken, andbase64urlEncodeis called only from withinsignJWT. Making themprivatetightens encapsulation and prevents accidental coupling from other test files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Test/AblyTests/Test` Utilities/TestUtilities.swift around lines 636 - 674, Make the helper functions signJWT and base64urlEncode private to limit their visibility to this file: update the declarations of signJWT(claims:keyName:keySecret:) and base64urlEncode(_:) to use the private access modifier so they remain usable by getJWTToken in this file but cannot be referenced from other test files, keeping encapsulation tight.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Test/AblyTests/Test` Utilities/TestUtilities.swift:
- Around line 601-615: The current early return when jwtType is non-empty calls
getEmbeddedJWTTokenViaEchoServer and skips merging extraClaims into the JWT,
silently dropping them; update the logic in the function containing this block
so that either (a) you merge extraClaims into the claims passed to
getEmbeddedJWTTokenViaEchoServer (i.e., compute and pass the combined claims) or
(b) add an explicit precondition/guard that extraClaims.isEmpty when jwtType is
non-empty and fail fast with a clear error message; reference jwtType,
extraClaims, getEmbeddedJWTTokenViaEchoServer, and signJWT to locate and modify
the code path accordingly.
---
Nitpick comments:
In `@Test/AblyTests/Test` Utilities/TestUtilities.swift:
- Around line 636-674: Make the helper functions signJWT and base64urlEncode
private to limit their visibility to this file: update the declarations of
signJWT(claims:keyName:keySecret:) and base64urlEncode(_:) to use the private
access modifier so they remain usable by getJWTToken in this file but cannot be
referenced from other test files, keeping encapsulation tight.
6e3f426 to
f81375b
Compare
getJWTToken now signs tokens locally using HMAC-SHA256 for the common case (no jwtType). When jwtType is set (e.g. "embedded"), it falls back to the echo server via a private helper since those require server-side token embedding. Also adds precondition guards to fail fast when unsupported parameter combinations are used: extraClaims on the echo-server path and encrypted on the local signing path.
- presenceMessageFromDictionary: now reads extras from the wire - presenceMessageToDictionary: now writes extras to the wire - Add integration test: JWT with user claim -> presence enter -> verify extras.userClaim on subscribed message
f81375b to
f0f693e
Compare
Summary
ARTPresenceMessageinherits anextrasproperty fromARTBaseMessage, but it was never populated on inbound messages or encoded on outbound messages.Defining the method overloads to include extras will happen in a separate PR.
To unblock: ably/ably-chat-swift#478
CHA-1243
Changes
Source/ARTJsonLikeEncoder.mpresenceMessageFromDictionary:now readsextrasfrom the wirepresenceMessageToDictionary:now writesextrasto the wire when setTest/Tests/RealtimeClientPresenceTests.swiftextras.userClaimis populated on the received presence messageSpec points
Summary by CodeRabbit
New Features
Tests