feat: Gate SyncBranchableCollection with NAC#4522
feat: Gate SyncBranchableCollection with NAC#4522AndrewSisley merged 1 commit intosourcenetwork:developfrom
Conversation
AndrewSisley
changed the title
AndrewSisley
force-pushed
the
4517-nac-sync-branch-col
branch
from
b81b861 to
e456c72
Compare
|
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:
📝 WalkthroughWalkthroughAdds NAC gating for SyncBranchableCollection: new node permission constant and policy entry, option-based identity propagation through client/DB/P2P/C layers, ACP checks in DB, identity-aware collection retrieval, and integration tests for authorized/unauthorized/admin-relation flows. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Client as Client API
participant Opts as Options Builder
participant DB as Database
participant ACP as ACP Checker
participant P2P as P2P Layer
User->>Client: SyncBranchableCollection(ctx, collectionID, opts...)
Client->>Opts: NewOptions(opts...)
Opts-->>Client: options (incl. identity)
Client->>DB: SyncBranchableCollection(ctx, collectionID, opts)
DB->>ACP: Check NodeP2PSyncBranchableCollectionPerm
ACP-->>DB: allowed / denied
alt allowed
DB->>P2P: SyncBranchableCollection(ctx, collectionID, opts)
P2P->>P2P: GetCollections(use identity if present)
P2P-->>DB: sync result
DB-->>Client: nil
else denied
DB-->>Client: NotAuthorized error
end
Client-->>User: result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Possibly related PRs
Suggested labels
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
http/client_p2p.go (1)
375-416:⚠️ Potential issue | 🔴 CriticalIdentity is lost:
httpCtxis created fromcontext.Background(), discarding the identity set onctx.The HTTP client's
setDefaultHeadersmethod (http/http_client.go:54) extracts identity from the request context usingidentity.FromContext(req.Context())and sets the Authorization header. InSyncBranchableCollection, the identity is injected intoctxat line 381, but line 403 creates a freshhttpCtxfromcontext.Background(). The HTTP request at line 410 is built withhttpCtx, so the request's context has no identity information. WhensetDefaultHeadersruns, it finds no identity and skips setting the Authorization header, causing allSyncBranchableCollectioncalls to appear unauthenticated and fail NAC checks.In contrast,
SyncCollectionVersions(line 366) uses the identity-enrichedctxdirectly for the HTTP request, so the Authorization header is properly set.To fix, inject identity into
httpCtxafter creating it:Proposed fix
httpCtx := context.Background() if hasDeadline { var cancel context.CancelFunc httpCtx, cancel = context.WithTimeout(httpCtx, time.Until(deadline)+500*time.Millisecond) defer cancel() } + httpCtx = identity.WithContext(httpCtx, opt.GetIdentity())
🤖 Fix all issues with AI agents
In `@internal/db/txn.go`:
- Around line 431-437: Txn.SyncBranchableCollection currently accepts opts but
calls txn.db.SyncBranchableCollection without forwarding them, losing identity
options; update the call in Txn.SyncBranchableCollection to pass opts... through
to txn.db.SyncBranchableCollection (mirror how SyncCollectionVersions forwards
opts...) so the options are preserved for NAC checks.
🧹 Nitpick comments (2)
internal/db/p2p/sync_branchable_col.go (1)
54-64: Potential nil-pointer dereference onopts.
optsis a bare pointer. If any caller passesnil, Line 60 (opts.Identity.HasValue()) will panic. Current internal callers go throughutils.NewOptionswhich always returns a non-nil pointer, so this is safe today, but the method is exported on*P2P.A nil guard keeps the contract explicit and safe against future callers:
🛡️ Suggested nil guard
func (p *P2P) SyncBranchableCollection( ctx context.Context, collectionID string, opts *options.SyncBranchableCollectionOptions, ) error { + if opts == nil { + opts = &options.SyncBranchableCollectionOptions{} + } getColOpts := options.GetCollections().SetCollectionID(collectionID) if opts.Identity.HasValue() {tests/integration/acp/nac/p2p_sync_branchable_collection_test.go (1)
79-161: Consider addingConnectPeersto the error-path tests for more realistic scenarios.Tests 2 and 3 omit
ConnectPeers, meaning the NAC rejection is the first failure point. This validates the gate, but it would also be valuable to confirm that the NAC gate blocks an otherwise-valid sync (i.e., with connected peers). Currently, even without NAC, these tests might fail with a timeout error due to no active peers.This is a minor observation — if the goal is purely to test the NAC gate regardless of connectivity, the current setup is sufficient.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
acp/types/types.gocbindings/wrapper.goclient/options/p2p.goclient/p2p.gohttp/client_p2p.gointernal/db/p2p.gointernal/db/p2p/p2p.gointernal/db/p2p/sync_branchable_col.gointernal/db/txn.gotests/action/sync_branchable_collection.gotests/clients/cli/wrapper.gotests/clients/http/wrapper.gotests/integration/acp/nac/p2p_sync_branchable_collection_test.gotests/integration/acp/nac/relation_admin/p2p_sync_branchable_collection_test.go
🧰 Additional context used
🧬 Code graph analysis (11)
client/p2p.go (3)
client/options/p2p.go (2)
SyncBranchableCollection(380-382)SyncBranchableCollectionOptions(364-367)tests/action/sync_branchable_collection.go (1)
SyncBranchableCollection(25-47)client/options/lister.go (1)
Lister(24-24)
client/options/p2p.go (2)
tests/state/identity.go (1)
GetIdentity(63-75)tests/action/sync_branchable_collection.go (1)
SyncBranchableCollection(25-47)
tests/clients/http/wrapper.go (3)
client/options/p2p.go (2)
SyncBranchableCollection(380-382)SyncBranchableCollectionOptions(364-367)tests/action/sync_branchable_collection.go (1)
SyncBranchableCollection(25-47)client/options/lister.go (1)
Lister(24-24)
http/client_p2p.go (4)
client/options/p2p.go (2)
SyncBranchableCollection(380-382)SyncBranchableCollectionOptions(364-367)client/options/lister.go (1)
Lister(24-24)internal/utils/util.go (1)
NewOptions(32-54)acp/identity/context.go (1)
WithContext(36-41)
internal/db/p2p/sync_branchable_col.go (2)
client/options/p2p.go (2)
SyncBranchableCollection(380-382)SyncBranchableCollectionOptions(364-367)acp/identity/factory.go (1)
FromDID(20-22)
internal/db/p2p.go (5)
client/options/p2p.go (2)
SyncBranchableCollection(380-382)SyncBranchableCollectionOptions(364-367)tests/action/sync_branchable_collection.go (1)
SyncBranchableCollection(25-47)internal/utils/util.go (1)
NewOptions(32-54)acp/types/types.go (1)
NodeP2PSyncBranchableCollectionPerm(113-113)internal/db/errors.go (1)
ErrNoP2P(192-192)
internal/db/txn.go (2)
client/options/p2p.go (2)
SyncBranchableCollection(380-382)SyncBranchableCollectionOptions(364-367)client/options/lister.go (1)
Lister(24-24)
tests/action/sync_branchable_collection.go (2)
client/options/p2p.go (1)
SyncBranchableCollection(380-382)internal/keys/systemstore_collection_id.go (1)
CollectionID(20-22)
internal/db/p2p/p2p.go (3)
tests/integration/test_case.go (1)
GetNodeIdentity(586-595)internal/core/crdt/delta.go (1)
Delta(15-18)acp/identity/factory.go (1)
FromDID(20-22)
tests/clients/cli/wrapper.go (5)
client/options/p2p.go (2)
SyncBranchableCollection(380-382)SyncBranchableCollectionOptions(364-367)tests/action/sync_branchable_collection.go (1)
SyncBranchableCollection(25-47)client/options/lister.go (1)
Lister(24-24)internal/utils/util.go (1)
NewOptions(32-54)tests/state/identity.go (1)
GetIdentity(63-75)
tests/integration/acp/nac/relation_admin/p2p_sync_branchable_collection_test.go (7)
tests/integration/test_case.go (2)
TestCase(29-92)Close(124-129)tests/state/client_type.go (5)
ClientType(13-13)HTTPClientType(21-21)CLIClientType(24-24)GoClientType(18-18)CClientType(30-30)tests/integration/identity.go (1)
ClientIdentity(41-48)tests/integration/p2p.go (1)
ConnectPeers(31-58)tests/action/sync_branchable_collection.go (1)
SyncBranchableCollection(25-47)tests/integration/utils.go (1)
FormatExpectedErrorWithPermission(2042-2044)acp/types/types.go (1)
NodeP2PSyncBranchableCollectionPerm(113-113)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (48)
- GitHub Check: Test coverage job (http, memory, collection-save)
- GitHub Check: Test coverage job (http, memory, gql)
- GitHub Check: Test coverage job (c, file, collection-named)
- GitHub Check: Test coverage job (c, memory, gql)
- GitHub Check: Test coverage job (c, memory, collection-save)
- GitHub Check: Test coverage job (c, memory, collection-named)
- GitHub Check: Test coverage job (cli, file, collection-save)
- GitHub Check: Test coverage job (c, file, gql)
- GitHub Check: Test coverage job (cli, memory, collection-save)
- GitHub Check: Test coverage job (c, file, collection-save)
- GitHub Check: Test coverage job (http, file, collection-named)
- GitHub Check: Test coverage job (cli, file, collection-named)
- GitHub Check: Test coverage job (http, file, gql)
- GitHub Check: Test coverage job (cli, memory, gql)
- GitHub Check: Test coverage job (go, file, collection-save)
- GitHub Check: Test coverage job (cli, memory, collection-named)
- GitHub Check: Test coverage job (cli, file, gql)
- GitHub Check: Test coverage job (http, file, collection-save)
- GitHub Check: Test coverage job (go, file, collection-named)
- GitHub Check: Test coverage job (go, memory, gql)
- GitHub Check: Test coverage job (http, memory, collection-named)
- GitHub Check: Test coverage job (go, file, gql)
- GitHub Check: Test coverage job (go, memory, collection-save)
- GitHub Check: Test coverage job (go, memory, collection-named)
- GitHub Check: Test coverage secondary index job
- GitHub Check: Check cli documentation job
- GitHub Check: Test coverage document acp job (c, source-hub)
- GitHub Check: Validate containerfile job
- GitHub Check: Test coverage leveldb job
- GitHub Check: Check http documentation job
- GitHub Check: Test coverage document acp job (cli, source-hub)
- GitHub Check: Test coverage document acp job (http, source-hub)
- GitHub Check: Start binary job
- GitHub Check: Test coverage JS job
- GitHub Check: Test coverage document acp job (go, source-hub)
- GitHub Check: Test coverage telemetry job
- GitHub Check: Test coverage view job
- GitHub Check: Test coverage lens job (wazero)
- GitHub Check: Test coverage encryption job
- GitHub Check: Test Limited Resource job
- GitHub Check: Test macos job
- GitHub Check: Check mocks job
- GitHub Check: Build dependencies job
- GitHub Check: Check vulnerabilities job
- GitHub Check: Check data format changes job
- GitHub Check: Test NPX/JS build job
- GitHub Check: Lint GoLang job
- GitHub Check: Check wizard health job
🔇 Additional comments (12)
acp/types/types.go (1)
113-113: LGTM!The new
NodeP2PSyncBranchableCollectionPermconstant, its string mapping, and the YAML policy entry are all correctly aligned and consistent with the existing pattern.Also applies to: 161-161, 256-257
client/options/p2p.go (1)
363-390: LGTM!The new
SyncBranchableCollectionOptionstype and its builder follow the established pattern consistently.internal/db/p2p.go (1)
385-399: LGTM!The NAC gate is correctly added before the P2P availability check, consistent with the pattern used by other gated operations like
SyncCollectionVersions.internal/db/p2p/p2p.go (1)
350-360: LGTM!The
hasAccessmethod now correctly retrieves and applies the node identity when fetching collections, aligning it with the identity-aware pattern already used intrySelfHasAccess.cbindings/wrapper.go (1)
379-406: LGTM!The C wrapper correctly resolves the options and passes the identity to the underlying C function, consistent with
SyncCollectionVersions.client/p2p.go (1)
126-130: LGTM!Interface signature correctly updated to accept the new variadic options parameter.
tests/integration/acp/nac/relation_admin/p2p_sync_branchable_collection_test.go (1)
24-91: LGTM!The test properly validates the NAC gate by verifying that an unauthorized identity is rejected, then succeeds after being granted the admin relation.
tests/clients/http/wrapper.go (1)
170-175: LGTM!Clean passthrough, consistent with all other wrapper methods in this file.
internal/db/p2p/sync_branchable_col.go (1)
271-284: LGTM!The handler-side identity propagation is well-structured: the node's own identity is retrieved and threaded into the collection lookup, ensuring consistent NAC enforcement for both inbound and outbound sync paths.
tests/action/sync_branchable_collection.go (1)
66-73: LGTM!Option construction and identity propagation follow the established pattern used by other actions in the test framework.
tests/clients/cli/wrapper.go (1)
324-341: LGTM!Consistent with the existing CLI wrapper patterns (e.g.,
SyncCollectionVersions). Options are correctly resolved and identity is appended to CLI args.tests/integration/acp/nac/p2p_sync_branchable_collection_test.go (1)
24-77: LGTM!Good coverage of the authorized-identity happy path. The setup correctly enables NAC on both nodes, establishes peer connectivity, and then confirms that the authorized identity can sync.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
AndrewSisley
force-pushed
the
4517-nac-sync-branch-col
branch
3 times, most recently
from
1195b78 to
de498d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
http/client_p2p.go (1)
396-415:⚠️ Potential issue | 🔴 CriticalBug: Identity is set on
ctxbut the HTTP request useshttpCtx(derived fromcontext.Background()), so the identity is never sent.Line 403 binds identity to
ctx, but line 410 creates the HTTP request withhttpCtxwhich is a freshcontext.Background()— the identity is silently dropped. Compare withSyncCollectionVersions(line 366) which correctly creates the request with the identity-bearingctx.🐛 Proposed fix — derive httpCtx from ctx so identity propagates
httpCtx := context.Background() opt := utils.NewOptions(opts...) - ctx = identity.WithContext(ctx, opt.GetIdentity()) + httpCtx = identity.WithContext(httpCtx, opt.GetIdentity()) if hasDeadline { var cancel context.CancelFunc httpCtx, cancel = context.WithTimeout(httpCtx, time.Until(deadline)+500*time.Millisecond) defer cancel() }Or, alternatively, ensure
httpCtxinherits identity fromctxbefore timeout wrapping.
🤖 Fix all issues with AI agents
In `@internal/db/p2p/sync_branchable_col.go`:
- Around line 54-64: The method P2P.SyncBranchableCollection currently
dereferences opts.Identity without guarding against a nil opts, risking a panic
if callers pass nil; update SyncBranchableCollection to first check if opts is
nil and, if so, initialize it to a default (or return a descriptive error)
before accessing opts.Identity, and then continue using opts.Identity (and
opts.HasValue) as currently implemented so callers can pass nil safely; refer to
the SyncBranchableCollection function and the opts and opts.Identity symbols
when making the change.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
acp/types/types.gocbindings/wrapper.goclient/options/p2p.goclient/p2p.gohttp/client_p2p.gointernal/db/p2p.gointernal/db/p2p/p2p.gointernal/db/p2p/sync_branchable_col.gointernal/db/txn.gotests/action/sync_branchable_collection.gotests/clients/cli/wrapper.gotests/clients/http/wrapper.gotests/clients/js/wrapper.gotests/integration/acp/nac/p2p_sync_branchable_collection_test.gotests/integration/acp/nac/relation_admin/p2p_sync_branchable_collection_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- client/options/p2p.go
- tests/clients/cli/wrapper.go
- tests/integration/acp/nac/relation_admin/p2p_sync_branchable_collection_test.go
- acp/types/types.go
- tests/clients/http/wrapper.go
🧰 Additional context used
🧬 Code graph analysis (9)
client/p2p.go (3)
client/options/p2p.go (2)
SyncBranchableCollection(380-382)SyncBranchableCollectionOptions(364-367)tests/action/sync_branchable_collection.go (1)
SyncBranchableCollection(25-47)client/options/lister.go (1)
Lister(24-24)
internal/db/p2p/p2p.go (2)
tests/integration/test_case.go (1)
GetNodeIdentity(586-595)acp/identity/factory.go (1)
FromDID(20-22)
internal/db/p2p/sync_branchable_col.go (2)
client/options/p2p.go (2)
SyncBranchableCollection(380-382)SyncBranchableCollectionOptions(364-367)acp/identity/factory.go (1)
FromDID(20-22)
cbindings/wrapper.go (6)
client/options/p2p.go (2)
SyncBranchableCollection(380-382)SyncBranchableCollectionOptions(364-367)client/options/lister.go (1)
Lister(24-24)internal/utils/util.go (1)
NewOptions(32-54)cbindings/identity.go (1)
IdentityFree(61-66)cbindings/cutils.go (1)
ConvertAndFreeCResult(185-193)cbindings/p2p.go (1)
P2PbranchableCollectionSync(345-372)
tests/action/sync_branchable_collection.go (2)
client/options/p2p.go (1)
SyncBranchableCollection(380-382)internal/keys/systemstore_collection_id.go (1)
CollectionID(20-22)
internal/db/p2p.go (4)
client/options/p2p.go (2)
SyncBranchableCollection(380-382)SyncBranchableCollectionOptions(364-367)internal/utils/util.go (1)
NewOptions(32-54)acp/types/types.go (1)
NodeP2PSyncBranchableCollectionPerm(113-113)internal/db/errors.go (1)
ErrNoP2P(192-192)
http/client_p2p.go (3)
client/options/p2p.go (2)
SyncBranchableCollection(380-382)SyncBranchableCollectionOptions(364-367)internal/utils/util.go (1)
NewOptions(32-54)acp/identity/context.go (1)
WithContext(36-41)
tests/clients/js/wrapper.go (3)
client/options/p2p.go (2)
SyncBranchableCollection(380-382)SyncBranchableCollectionOptions(364-367)tests/action/sync_branchable_collection.go (1)
SyncBranchableCollection(25-47)client/options/lister.go (1)
Lister(24-24)
tests/integration/acp/nac/p2p_sync_branchable_collection_test.go (6)
tests/integration/test_case.go (1)
TestCase(29-92)tests/integration/identity.go (2)
ClientIdentity(41-48)NoIdentity(26-28)tests/integration/p2p.go (1)
ConnectPeers(31-58)tests/action/sync_branchable_collection.go (1)
SyncBranchableCollection(25-47)tests/integration/utils.go (1)
FormatExpectedErrorWithPermission(2042-2044)acp/types/types.go (1)
NodeP2PSyncBranchableCollectionPerm(113-113)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (42)
- GitHub Check: Test macos job
- GitHub Check: Test coverage job (http, memory, collection-named)
- GitHub Check: Test coverage job (cli, memory, collection-named)
- GitHub Check: Test coverage job (http, file, gql)
- GitHub Check: Test coverage job (c, memory, collection-named)
- GitHub Check: Test coverage job (c, file, collection-named)
- GitHub Check: Test coverage job (go, file, collection-save)
- GitHub Check: Test coverage job (cli, memory, collection-save)
- GitHub Check: Test coverage job (c, memory, gql)
- GitHub Check: Test coverage job (cli, file, collection-named)
- GitHub Check: Test coverage job (c, file, collection-save)
- GitHub Check: Test coverage job (go, file, collection-named)
- GitHub Check: Test coverage job (go, memory, collection-save)
- GitHub Check: Test coverage job (c, memory, collection-save)
- GitHub Check: Test coverage job (go, memory, collection-named)
- GitHub Check: Test coverage job (http, file, collection-save)
- GitHub Check: Test coverage job (cli, file, collection-save)
- GitHub Check: Test coverage job (c, file, gql)
- GitHub Check: Test coverage job (http, file, collection-named)
- GitHub Check: Test coverage job (cli, file, gql)
- GitHub Check: Test coverage job (cli, memory, gql)
- GitHub Check: Test coverage job (http, memory, collection-save)
- GitHub Check: Test coverage job (go, memory, gql)
- GitHub Check: Test coverage job (go, file, gql)
- GitHub Check: Test coverage job (http, memory, gql)
- GitHub Check: Test coverage lens job (wazero)
- GitHub Check: Test coverage leveldb job
- GitHub Check: Test coverage document acp job (c, source-hub)
- GitHub Check: Test coverage encryption job
- GitHub Check: Test coverage secondary index job
- GitHub Check: Test coverage document acp job (http, source-hub)
- GitHub Check: Test coverage document acp job (go, source-hub)
- GitHub Check: Test coverage document acp job (cli, source-hub)
- GitHub Check: Test coverage view job
- GitHub Check: Test coverage telemetry job
- GitHub Check: Test coverage JS job
- GitHub Check: Check data format changes job
- GitHub Check: Test Limited Resource job
- GitHub Check: Lint GoLang job
- GitHub Check: Validate containerfile job
- GitHub Check: Test NPX/JS build job
- GitHub Check: Start binary job
🔇 Additional comments (9)
internal/db/p2p/p2p.go (1)
350-364: LGTM — identity-awareGetCollectionscall inhasAccess.The pattern of fetching the node identity, conditionally setting it on
GetCollectionsoptions, and proceeding with the collection lookup is consistent with the same approach used inprocessSyncBranchableCollection(lines 272-281 insync_branchable_col.go).client/p2p.go (1)
126-130: LGTM — interface signature updated to accept variadic options.Consistent with the pattern used by all other methods in the
P2Pinterface.tests/clients/js/wrapper.go (1)
157-159: LGTM — signature update for interface compliance.internal/db/p2p/sync_branchable_col.go (1)
271-281: LGTM — identity-awareGetCollectionsinprocessSyncBranchableCollection.Pattern is consistent with
hasAccessinp2p.go(lines 350-360).cbindings/wrapper.go (1)
379-406: LGTM — identity plumbing added toSyncBranchableCollection.The pattern (resolve options → extract identity → defer free → pass to C call) is consistent with
SyncCollectionVersions(lines 350-377).tests/action/sync_branchable_collection.go (1)
66-73: LGTM — identity-aware test action forSyncBranchableCollection.The pattern of building options, conditionally setting identity, and passing it to the sync call follows the established test action patterns in this codebase.
internal/db/p2p.go (1)
385-399: LGTM!The NAC gate follows the same pattern used by all other P2P operations in this file: parse options → check node access → delegate to
db.p2p. The permission constantNodeP2PSyncBranchableCollectionPermis correctly used, andoptis forwarded to the p2p layer for identity-aware collection resolution.tests/integration/acp/nac/p2p_sync_branchable_collection_test.go (2)
24-77: LGTM!The authorized-identity test correctly sets up two nodes with NAC, connects peers, creates a branchable collection with a document, and verifies that an authorized identity can successfully sync. The
SupportedClientTypesrestriction appropriately excludes client types that may not support the full P2P networking flow.
79-161: LGTM!The no-identity and wrong-identity tests correctly verify that the NAC gate rejects unauthorized requests before any P2P operation occurs. The expected error format using
FormatExpectedErrorWithPermission(acpTypes.NodeP2PSyncBranchableCollectionPerm)is consistent with the permission constant used in the production code. Not needingConnectPeersorSupportedClientTypesrestrictions makes sense since these tests fail at the access-check stage.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
AndrewSisley
force-pushed
the
4517-nac-sync-branch-col
branch
from
de498d5 to
371e08b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
http/client_p2p.go (1)
396-416:⚠️ Potential issue | 🔴 CriticalIdentity is set on
ctxbut the HTTP request useshttpCtx— identity is lost.At line 403, the identity is injected into
ctx, but the actual HTTP request at line 410 is created withhttpCtx, which is derived fromcontext.Background()and never receives the identity. This means the NAC identity will not be sent to the server, defeating the purpose of this PR.Compare with
SyncCollectionVersions(line 366) which usesctxdirectly for the request and doesn't have this issue.🐛 Proposed fix: derive httpCtx from the identity-enriched ctx
httpCtx := context.Background() opt := utils.NewOptions(opts...) ctx = identity.WithContext(ctx, opt.GetIdentity()) if hasDeadline { var cancel context.CancelFunc - httpCtx, cancel = context.WithTimeout(httpCtx, time.Until(deadline)+500*time.Millisecond) + httpCtx, cancel = context.WithTimeout(ctx, time.Until(deadline)+500*time.Millisecond) + defer cancel() + } else { + httpCtx = ctx + } - defer cancel() - }internal/db/p2p/p2p.go (1)
443-446:⚠️ Potential issue | 🟡 Minor
trySelfHasAccessshould pass node identity toGetCollectionsfor consistency withhasAccess.The
hasAccessmethod (lines 355–358) retrieves the node identity and conditionally passes it toGetCollectionsviaSetIdentity(). However,trySelfHasAccess(lines 443–446) callsGetCollectionswithout identity, only adding it later. This inconsistency could causeGetCollectionsto filter results differently between the two methods, potentially causingtrySelfHasAccessto returnErrCollectionNotFound(line 451) even when the node should have access to a collection.
🤖 Fix all issues with AI agents
In `@internal/db/p2p/p2p.go`:
- Around line 350-360: The call to GetNodeIdentity uses p.ctx while
GetCollections uses the function parameter ctx, causing inconsistent context
lifetimes; update the GetNodeIdentity invocation to use the local ctx parameter
(i.e., call p.db.GetNodeIdentity(ctx)) so both GetNodeIdentity and
GetCollections use the same request/callback context, keeping the rest of the
logic (ident.HasValue(), getColOpts assignment, and p.db usage) unchanged.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
acp/types/types.gocbindings/wrapper.goclient/mocks/txn.goclient/options/p2p.goclient/p2p.gohttp/client_p2p.gointernal/db/p2p.gointernal/db/p2p/p2p.gointernal/db/p2p/sync_branchable_col.gointernal/db/txn.gotests/action/sync_branchable_collection.gotests/clients/cli/wrapper.gotests/clients/http/wrapper.gotests/clients/js/wrapper.gotests/integration/acp/nac/p2p_sync_branchable_collection_test.gotests/integration/acp/nac/relation_admin/p2p_sync_branchable_collection_test.go
🚧 Files skipped from review as they are similar to previous changes (10)
- tests/clients/http/wrapper.go
- acp/types/types.go
- tests/integration/acp/nac/p2p_sync_branchable_collection_test.go
- client/p2p.go
- tests/integration/acp/nac/relation_admin/p2p_sync_branchable_collection_test.go
- cbindings/wrapper.go
- internal/db/p2p.go
- internal/db/txn.go
- tests/clients/js/wrapper.go
- client/options/p2p.go
🧰 Additional context used
🧬 Code graph analysis (4)
http/client_p2p.go (3)
client/options/p2p.go (2)
SyncBranchableCollection(380-382)SyncBranchableCollectionOptions(364-367)internal/utils/util.go (1)
NewOptions(32-54)acp/identity/context.go (1)
WithContext(36-41)
internal/db/p2p/sync_branchable_col.go (2)
client/options/p2p.go (2)
SyncBranchableCollection(380-382)SyncBranchableCollectionOptions(364-367)acp/identity/factory.go (1)
FromDID(20-22)
tests/clients/cli/wrapper.go (4)
client/options/p2p.go (2)
SyncBranchableCollection(380-382)SyncBranchableCollectionOptions(364-367)tests/action/sync_branchable_collection.go (1)
SyncBranchableCollection(25-47)internal/utils/util.go (1)
NewOptions(32-54)tests/state/identity.go (1)
GetIdentity(63-75)
internal/db/p2p/p2p.go (2)
tests/integration/test_case.go (1)
GetNodeIdentity(586-595)acp/identity/factory.go (1)
FromDID(20-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (48)
- GitHub Check: Test macos job
- GitHub Check: Test coverage job (http, file, gql)
- GitHub Check: Test coverage job (go, memory, collection-named)
- GitHub Check: Test coverage job (go, file, collection-save)
- GitHub Check: Test coverage job (c, file, gql)
- GitHub Check: Test coverage job (go, file, gql)
- GitHub Check: Test coverage job (go, memory, collection-save)
- GitHub Check: Test coverage job (go, memory, gql)
- GitHub Check: Test coverage job (cli, memory, collection-save)
- GitHub Check: Test coverage job (http, file, collection-save)
- GitHub Check: Test coverage job (c, file, collection-save)
- GitHub Check: Test coverage job (cli, memory, collection-named)
- GitHub Check: Test coverage job (c, memory, gql)
- GitHub Check: Test coverage job (cli, memory, gql)
- GitHub Check: Test coverage job (c, memory, collection-named)
- GitHub Check: Test coverage job (http, file, collection-named)
- GitHub Check: Test coverage job (http, memory, gql)
- GitHub Check: Test coverage job (http, memory, collection-save)
- GitHub Check: Test coverage job (cli, file, gql)
- GitHub Check: Test coverage job (go, file, collection-named)
- GitHub Check: Test coverage job (cli, file, collection-save)
- GitHub Check: Test coverage job (c, file, collection-named)
- GitHub Check: Test coverage job (http, memory, collection-named)
- GitHub Check: Test coverage job (c, memory, collection-save)
- GitHub Check: Test coverage job (cli, file, collection-named)
- GitHub Check: Test coverage document acp job (http, source-hub)
- GitHub Check: Test coverage JS job
- GitHub Check: Test coverage document acp job (cli, source-hub)
- GitHub Check: Test coverage leveldb job
- GitHub Check: Test coverage encryption job
- GitHub Check: Test coverage document acp job (c, source-hub)
- GitHub Check: Test coverage document acp job (go, source-hub)
- GitHub Check: Test coverage telemetry job
- GitHub Check: Test coverage view job
- GitHub Check: Test coverage lens job (wazero)
- GitHub Check: Test coverage secondary index job
- GitHub Check: Check data format changes job
- GitHub Check: Test Limited Resource job
- GitHub Check: Test NPX/JS build job
- GitHub Check: Check mocks job
- GitHub Check: Check vulnerabilities job
- GitHub Check: Validate containerfile job
- GitHub Check: Build dependencies job
- GitHub Check: Lint GoLang job
- GitHub Check: Start binary job
- GitHub Check: Check cli documentation job
- GitHub Check: Check http documentation job
- GitHub Check: Check wizard health job
🔇 Additional comments (4)
internal/db/p2p/sync_branchable_col.go (1)
271-279: LGTM — identity propagation inprocessSyncBranchableCollection.The node identity retrieval and conditional injection into
GetCollectionsoptions follows the same pattern used elsewhere in the codebase.client/mocks/txn.go (1)
2987-3056: Auto-generated mock — no concerns.The
SyncBranchableCollectionmock updates follow the same variadic-options pattern used by all other mocked methods in this file.tests/clients/cli/wrapper.go (1)
324-341: LGTM — consistent with other CLI wrapper methods.The identity extraction and
appendIdentityArgpattern matches all other P2P methods in this wrapper.tests/action/sync_branchable_collection.go (1)
52-77: LGTM — identity-aware test action follows established patterns.The identity retrieval via
getIdentityForRequestSpecificToNodeand conditionalSetIdentityon the options builder is consistent with other identity-aware test actions in the codebase.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
AndrewSisley
force-pushed
the
4517-nac-sync-branch-col
branch
from
371e08b to
1ef98ac
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #4522 +/- ##
===========================================
+ Coverage 75.80% 76.13% +0.34%
===========================================
Files 521 521
Lines 41866 41892 +26
===========================================
+ Hits 31733 31894 +161
+ Misses 7662 7521 -141
- Partials 2471 2477 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 14 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
AndrewSisley
force-pushed
the
4517-nac-sync-branch-col
branch
3 times, most recently
from
eedc6c0 to
2becdc1
Compare
AndrewSisley
force-pushed
the
4517-nac-sync-branch-col
branch
from
2becdc1 to
1487687
Compare
2 participants
Relevant issue(s)
Resolves #4517
Description
Gates SyncBranchableCollection operation with NAC.
Not all of this PR is boilerplate - the node identity is being fetched and passed on to
GetCollectionsin a few places in thedb/p2ppackage.