feat: tc token - #280
Conversation
📝 WalkthroughWalkthroughAdds Trusted Contact (TC) privacy token support across the codebase: IQ spec and parsing, storage schema and SQLite implementation, client API (TcToken handle), message/profile/presence integration, notification handling, and background pruning. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client
participant IQ
participant Server
participant Store
User->>Client: tc_token().issue_tokens([jid])
Client->>IQ: build IssuePrivacyTokensSpec
IQ->>Server: send IQ set (issue tokens)
Server-->>IQ: response with ReceivedTcToken(s)
IQ-->>Client: parsed tokens
Client->>Store: put_tc_token(jid, TcTokenEntry)
Store-->>Client: Ok
Client-->>User: return ReceivedTcToken(s)
rect rgb(100, 150, 200, 0.5)
Note over Client,Store: Background pruning
Client->>Store: delete_expired_tc_tokens(cutoff)
Store-->>Client: deleted_count
end
sequenceDiagram
participant Sender
participant MessageHandler
participant Client
participant Store
participant Server
Sender->>MessageHandler: send 1:1 message to jid
MessageHandler->>Client: lookup_tc_token_for_jid(jid)
Client->>Store: get_tc_token(jid)
Store-->>Client: Option<TcTokenEntry>
alt token valid
Client-->>MessageHandler: Some(token)
MessageHandler->>MessageHandler: attach <tctoken> to stanza
else token missing/expired
MessageHandler->>Client: tc_token().issue_tokens([jid])
Client->>Server: IQ set issue tokens
Server-->>Client: ReceivedTcToken
Client->>Store: put_tc_token(jid, TcTokenEntry)
Client-->>MessageHandler: token
MessageHandler->>MessageHandler: attach <tctoken> to stanza
end
MessageHandler->>Server: send message (with optional <tctoken>)
Server-->>MessageHandler: ack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
Bencher Report![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/handlers/notification.rs`:
- Around line 386-423: The code currently treats any non-Ok(Some(...)) result
from backend.get_tc_token(&sender_lid) as “no existing entry” and may overwrite
a newer token; change the control flow to match on
backend.get_tc_token(&sender_lid).await and handle three cases:
Ok(Some(existing)) — keep the existing monotonicity check and preserve
existing.sender_timestamp when updating; Ok(None) — create and store a new
TcTokenEntry with sender_timestamp=None as you already do; Err(e) — do NOT
create or overwrite the token, instead log a warning including the error (e) and
skip updating for sender_lid so monotonicity is preserved; reference
get_tc_token, put_tc_token, TcTokenEntry, sender_lid and received_tokens in your
changes.
In `@src/send.rs`:
- Around line 449-455: The code skips adding the tc_token for groups and
newsletters but the comment also says to skip for the client's own JID; update
the condition to include an "own JID" check before calling
self.maybe_include_tc_token(&to, &mut extra_stanza_nodes). Specifically, add a
comparison against the sender's JID (e.g., self.jid or the method/field that
holds the client's JID) so the if becomes something like: if !to.is_group() &&
!to.is_newsletter() && to != self.jid { ... } (use the exact symbol for the
client's JID in this module), then call maybe_include_tc_token as before;
alternatively adjust the comment if you intentionally want to include own JID.
In `@wacore/src/iq/tctoken.rs`:
- Around line 184-187: The code silently creates token_bytes = Vec::new() when
token_node.content isn't Some(NodeContent::Bytes(...)), which can lead to
constructing ReceivedTcToken with an empty token; instead, detect empty
token_bytes after the match and either skip creating/adding the ReceivedTcToken
or return an error, and emit a warning via your crate logger (e.g.,
tracing::warn! or the project's logger) so missing token content is visible;
update the logic around token_node/content handling and where ReceivedTcToken is
constructed to bail/continue when token_bytes.is_empty() and include a clear log
message referencing the token_node/ReceivedTcToken.
- Around line 173-182: Replace the optional_attr(...) + ok_or_else(...) usage
for mandatory attributes with required_attr to match the project's parsing
conventions: use required_attr(token_node, "jid") instead of
optional_attr(token_node, "jid").ok_or_else(...), and required_attr(token_node,
"t") instead of optional_attr(token_node, "t").ok_or_else(...); retain the
subsequent parsing steps and map_err error wrapping for jid.parse() and
t_str.parse() so only the attribute lookup changes (referencing token_node,
jid_str/t_str, and the parse map_err calls).
🧹 Nitpick comments (4)
wacore/src/iq/tctoken.rs (4)
61-100: Consider extracting the repeatednowhelper and accepting a clock parameter for testability.The
SystemTime::now()...as_secs() as i64pattern is duplicated four times (lines 62–65, 81–84, 95–98, 125–128). Beyond the DRY concern, embedding wall-clock reads makes these functions non-deterministic — the test on line 296–303 already notes that boundary alignment makes assertions unreliable.A lightweight improvement: extract a
fn unix_now() -> i64helper for deduplication, and consider acceptingnow: i64as a parameter in the core logic functions (the public wrappers can still default to wall time). This lets tests exercise exact bucket boundaries deterministically.♻️ Sketch
+fn unix_now() -> i64 { + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap_or_default() + .as_secs() as i64 +} + pub fn is_tc_token_expired(token_timestamp: i64) -> bool { - let now = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap_or_default() - .as_secs() as i64; - now - token_timestamp >= TC_TOKEN_TOTAL_DURATION + is_tc_token_expired_at(token_timestamp, unix_now()) +} + +fn is_tc_token_expired_at(token_timestamp: i64, now: i64) -> bool { + now - token_timestamp >= TC_TOKEN_TOTAL_DURATION }Same pattern for
should_send_new_tc_token/tc_token_expiration_cutoff.
123-131: Constructor takes ownedVec<Jid>— consider taking&[Jid]per coding guidelines.The guideline states constructors should accept references and clone internally. Taking
Vec<Jid>forces callers to build an owned collection upfront.♻️ Proposed fix
- pub fn new(jids: Vec<Jid>) -> Self { + pub fn new(jids: &[Jid]) -> Self { let timestamp = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) .unwrap_or_default() .as_secs() as i64; - Self { jids, timestamp } + Self { jids: jids.to_vec(), timestamp } }As per coding guidelines, "IqSpec constructors should take
&Jidinstead ofJidto avoid forcing callers to clone; the clone happens inside the constructor."
231-238: PlaceholderJid::default()creates an incomplete struct — consider a safer representation.Returning
ReceivedTcTokenwith a default JID relies on every caller remembering to patch it. If someone forgets, the token silently carries an invalid/empty JID with no compile-time guard.A safer alternative would be to return an intermediate type (e.g., a tuple of
(Vec<u8>, i64)or a dedicatedParsedTokenDatawithout thejidfield) and let the caller constructReceivedTcTokenwith the resolved JID. This makes the incomplete state unrepresentable.
289-304: Dead code in test:old_tsis computed but never asserted on.
old_tsis calculated (line 296) and then suppressed withlet _ = old_ts;(line 303). Either remove it or add a meaningful assertion. As the comment notes, 1-bucket-back can be unreliable with wall time — this reinforces the earlier suggestion to acceptnowas a parameter for deterministic testing.
1 participant
Summary by CodeRabbit