Feat/ws grants bug 215

[solus161]

Description

Issue 215 pointed out the tangled up nature of record keys and topics. This PR decouples these two namespaces by adding distinct logics (paths are relative to aimdb-websocket-connection crate by default). The core idea is to keep this logic clear: clients keep speaking topic, server speaks record key, and the two namespaces joined at outbound route:

• /src/server/auth::Permissions now has read_patterns renamed from subscribe_pattern. This indicates a change of authorization logic from topic to record key. Meanwhile, write_patterns remains with topic. This is followed by a change of fn can_subscribe() to fn can_read(), which is naming change not logic change;
• /src/server/auth::RecordsBits added to encode per-client read permissions. Each bit decice whether the client having read access to a record. Bit indexes are the same as record indexes which are registration order (in AimDbInner.storage. RecordsBits is built during ws upgrade process, and carried by ClientInfo;
• /src/server/auth: several AuthHandler fn become obsolete due to this logic change and being removed, including fn authorize_subscribe() (authorization is resolved at upgrade and enforced at delivery), fn authorize_query() (the filtering logic is now based on record permissions), and fn authorize_list();
• src/server/http: the fn ws_upgrade_handler() now build the per-client permission bitmask based on the server's registered records and configured Permissions. The handler does not deny the upgrade even if the client has no grant (permission bitmask is empty or all bits are zero).
• The server building process now has to incorporate additional record id logic:
  • /src/server/connector::SnapshotCache is updated to carry record id logic. A snapshot is now identified by record id and associated topic, instead of just topic. trait SnapshotProvider output also changes accordingly;
  • aimdb-core/src/builder::AimDb.collect_outbound_routes() now build OutboundRoute with extra key-value pair of "record_index"-"{record_id}" in .config: ConnectorConfig attr. ConnectorConfig carries that pair till /src/server/connector::WsBusSink.publish() where the snapshot cache is built and message is broadcasted. It is the ClientManager deciding which subscriptions/clients get the message;
• As a result of the decoupling logic, some behaviors worth mentioned:
  • Client having no permissions is denied at subscription at WsSession.subscribe(), /src/server/dispatch.rs;
  • Client could subscribe to topics not registered yet by server;
  • record.list returns only records that the client has permissions for;
  • record.query could return a set of records smaller than what the name pattern asks for. If the asked name and grant bits do not overlap, the query is denied;

Also, several tests added to ensure these behaviors hold.

Related Issue

• Closes [BUG] ws grants are ws-topic patterns — record.list/record.query authorize with them in key space #215

Checklist

• I have read the CONTRIBUTING.md document.
• My code follows the project's coding standards.
• I have added tests to cover my changes.
• All new and existing tests passed (make check).
• I have updated the documentation accordingly.

[solus161]

Hi @lxsaah, plz check the PR. The large part of added code is test to cover different behaviors stem from decoupled record and topic. Btw I missed your collab invite, could you send again?

[lxsaah]

Review

Nice piece of work. The decoupling is the right call and the central property holds. Verified locally on dc7dad4: make check passes in full (all 11 targets) and I wrote the negative test that clients_disjoint_grants leaves commented out — a secret.# client subscribed to the shared public_info topic receives nothing across three publishes, while still getting its own record. The record gate works.

I also confirmed the index invariant the whole model rests on: AimDbInner::list_records() enumerates storages and sets record_id = i (builder.rs:191), the same enumeration collect_outbound_routes uses, and connector build() runs after all record registration (builder.rs:819). Out-of-range returns false, so appending records later is fail-closed.

Four things I'd want addressed before merge, then some design questions.

Blocking

1. record.query denies names that match no registered record, even under a full grant — dispatch.rs:230-240

Verified: with grant public.#, querying name: "public.archived" returns {"err":"denied"}. But record.query is a historical query against persistence — a record retired from the current config still has rows in the store, and is now unqueryable. The old authorize_query checked containment against the grant only, not against the live record list.

It also conflates "you have no permission" with "nothing matched", returning an authorization error for what is really an empty result. Suggest: Denied only when the pattern falls entirely outside the grant; otherwise {records: [], total: 0}.

2. The handler's total is silently discarded — dispatch.rs:245, dispatch.rs:300

let (records, _total) = handler.handle_query(...)   // :245
// ...
Ok(json!({ "records": records, "total": records.len() }))   // :300

QueryHandler::handle_query is public and documented to return (records, total_count). With limit set, total was the full match count for pagination; it's now the filtered page length. The built-in persistence handler already returned len() (aimdb-persistence/src/builder_ext.rs:85), so only custom handlers are affected — but the trait doc still promises the old contract. Either keep the handler's total, or update the trait doc.

3. write_patterns is documented as record keys but still matched against the topic

auth.rs:58 says "Record name patterns the client may write to" and the module doc says "gate which records a client may write to" — but dispatch.rs:191 passes the AimX write frame's topic to can_write, whose own parameter is still named topic. So read = record key, write = topic.

The asymmetry is defensible (the PR description says as much), but the doc comments now state the opposite, and an operator granting a record key gets a silent denial. The tests can't catch it: the cfg fixture uses link_from("ws://cfg") on record key cfg, so key == topic.

4. No CHANGELOG entry

aimdb-websocket-connector/CHANGELOG.md ## [Unreleased] is empty, but this is a breaking public-API change right after v2.0.0:

• Permissions::subscribe_patterns → read_patterns, can_subscribe → can_read
• AuthHandler::authorize_subscribe / authorize_query / authorize_list removed
• new pub field on ClientInfo
• ClientManager::subscribe / broadcast signatures changed (both re-exported from the crate root)
• ConnectorConfig::record_index in core

The existing changelog documents changes at exactly this granularity.

Design questions

5. record_index as a typed field on core's ConnectorConfig — transport.rs:36

That struct's own doc says protocol-specific knobs travel in protocol_options "without polluting the base struct". It's also breaking on a public, non-#[non_exhaustive] struct, and record_index silently becomes a reserved key in the public with_config(key, value) API (safe — the authoritative push is last and from_query is last-wins — but undocumented and unvalidated).

The core change itself is unavoidable: the index only exists in core, and topics are many-to-one with records, so nothing downstream can recover it. But the string encoding is avoidable. An alternative, implemented and run against make check:

// builder.rs — typed field on the route, no synthetic config pair
pub struct OutboundRoute { /* … */ pub record_index: usize }

// pump.rs — the join
let mut cfg = ConnectorConfig::from_query(&config);
cfg.record_index = Some(record_index);

Plus dropping the "record_index" arm from from_query and adding #[non_exhaustive] to ConnectorConfig while it's already breaking. Connector::publish is untouched, so no churn across the five implementors. Exactly one in-tree site needed updating (pump.rs); session/client.rs already uses ... Net −15 lines in the route test, which also gets to assert the real invariant (meta[route.record_index].record_key == key) instead of a string round-trip.

Happy for this to land as a follow-up rather than in this PR. Also fixes #6 below.

6. WsBusSink::publish returns Ok(()) and silently drops when record_index is None

Unreachable via pump_sink today (the value round-trips a usize, so the parse can't fail), so this is structural rather than live — but it's the failure mode of a security boundary, and the only signal is a warn! behind the tracing feature. Prefer Err(PublishError::InvalidDestination): pump_sink already does log_error! on publish failure, so it surfaces without a feature gate.

7. Permissions are now frozen at upgrade

Removing the async hooks means no mid-connection revocation and no external/dynamic ACL lookup — the deleted AsyncTopicAuth e2e test existed specifically to prove that was supported. The O(1) bitmask is a good trade, but it's a capability removal that belongs in the PR body and the changelog. Is revocation planned?

8. from_value::<Vec<QueryRecord>> narrows the QueryHandlerFn contract — dispatch.rs:274

The old code passed the handler's JSON through verbatim. Now a handler returning a different shape — or omitting "records" on an empty result, e.g. {"total": 0} — gets RpcError::Internal instead of a result.

Tests

9. clients_disjoint_grants doesn't test what its comment claims. The "Secret client does not receive public.ledger" assertion is commented out and replaced with a positive read of the secret client's own event — but that client is subscribed to secret_info, so plain topic matching already excludes the public record and the record gate is never exercised. Also leaves a stray println! and dead commented code.

Fix: subscribe the secret client to public_info and assert nothing arrives. I wrote this and it passes — so it's an assertion gap, not a bug, but it's the single most important property in the PR.

10. Uncovered behaviors the description claims: subscribe denial for a client with zero read grants (the has_permissions() path, dispatch.rs:156), and partial record.query results where the grant covers only part of the pattern. RecordsBits also only tests set(len + 1), not the set(len) boundary.

Nits

• auth.rs:120 — (0..blocks).into_iter().map(|_| 0u8).collect() → vec[0u8; blocks]; the if length > 0 guard is dead since 0.div_ceil(8) == 0.
• is_empty() (no records) and has_permissions() (no bits set) read alike but mean different things. is_empty exists only to satisfy clippy's len_without_is_empty — worth a doc line, or rename to no_grants().
• block_index(&self, …) takes &self without using it, while offset is associated. offset is just index % 8.
• builder.rs:373 — truncated doc comment: /// The struct hold.
• Typos in new comments: "decice", "ultimatly", "subcribes", "Extrat", "record keyw", "Ouf of index", "our-of-index", "diffent".
• Zero-record server with allow_all → every subscribe gets Denied via has_permissions(). Degenerate, but the error is misleading.
• RecordsBits isn't re-exported from the crate root, though ClientInfo (which carries Arc<RecordsBits>) and ClientManager (whose public subscribe takes one) both are.
• Registration order is now load-bearing for security, not just lookup. Appends are fail-closed, but a future reorder or removal in storages would silently re-point every live client's bitmask. Worth a debug_assert! or a note at AimDbInner.storages pinning the invariant.

---

[solus161]

Hi @lxsaah , highly appreciate you detailed reply. Indeed I am not very familiar with the code base and design, yet, so seveval fixes are not optimal.

On 5: Agree on the suggestion, I'll create an issue accordingly. The round trip usize -> String -> usize could be avoid.

On 7: Frankly, the revocation was not in my mind. The removal of authorize_subscribe, authorize_query, and authorize_list were merely due to redundancy. However, your opinion did push me back on rethinking the revocation design. Here's my two cents on this: The external/dynamic ACL lookup indeed makes the API more robust. However, the "external", in the sense that the server calls external API for authorization, may not needed as 1) the per-client permission bitmap is fixed-length till the server restarts, any permission update better be reflected to that bitmap and grants to upholding integrity, 2) an act of updating permissions in an external ACL could better be done by updating the internal bitmap AND the grants, the admin/operator could perform such update through a dedicated API, and 3) the external API may not be stable, even though these auth checks are not in hot path, a failed call mean no serving for clients. By bringing any ACL/permission update internal: 1) auth checks could be more reliable and faster, and 2) gating could be applied to already-streaming client, a behavior current code does not support. Plz correct me if I get the ACL model wrong.

So I suggest to add mid-session ACL update together with dedicated admin/operator API for 7. That's another issue an another PR.

Another thing, record.query should have filtering handled by QueryHandlerFn instead of doing that internally. That means search pattern/name should be resolved with grant before passed into query handler. This could be another issue/PR.

[lxsaah]

Thanks a lot for this @solus161. Read grants now follow record keys instead of topics, which closes the cross-record leak. Great contribution!