[management] adding account id validation to accessible peers handler#5246
[management] adding account id validation to accessible peers handler#5246pascal-fischer merged 2 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughIntegrates a permissions manager into the peers HTTP handler and wiring. Handler struct, constructor, and AddEndpoints signature are updated; GetAccessiblePeers now checks permissions before loading account data. Server HTTP initialization passes the new permissionsManager to peers.AddEndpoints. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Router as HTTP Router
participant Peers as Peers Handler
participant Perm as Permissions Manager
participant Acct as Account Manager
participant NMap as Network Map Controller
Client->>Router: GET /peers/access
Router->>Peers: Route request
Peers->>Perm: ValidateAccountAccess
alt Permission denied
Perm-->>Peers: Denied
Peers-->>Client: 403 Forbidden
else Permission granted
Perm-->>Peers: Allowed
Peers->>Acct: Load account
Acct-->>Peers: Account details
Peers->>NMap: Fetch accessible peers
NMap-->>Peers: Peer list
Peers-->>Client: 200 OK + peers
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
|
![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
🤖 Fix all issues with AI agents
In `@management/server/http/handlers/peers/peers_handler_test.go`:
- Around line 16-24: The test mixes two gomock
forks—github.com/golang/mock/gomock and go.uber.org/mock/gomock—causing
matcher/controller type mismatches; pick one fork and update imports and mock
types accordingly (prefer the existing github.com/golang/mock/gomock used by
permissions and settings mocks). Replace usages of ugomock (and
network_map.MockController generated for the uber fork) with the golang/mock
equivalents (network_map.MockController regenerated or updated to the
golang/mock package), change any ugomock.NewController calls to
gomock.NewController, and ensure all matcher uses (e.g., gomock.Any()) and mock
expectations reference the same gomock package throughout the file.
| "github.com/golang/mock/gomock" | ||
| "github.com/gorilla/mux" | ||
| "go.uber.org/mock/gomock" | ||
| ugomock "go.uber.org/mock/gomock" | ||
| "golang.org/x/exp/maps" | ||
|
|
||
| "github.com/netbirdio/netbird/management/internals/controllers/network_map" | ||
| nbcontext "github.com/netbirdio/netbird/management/server/context" | ||
| nbpeer "github.com/netbirdio/netbird/management/server/peer" | ||
| "github.com/netbirdio/netbird/management/server/permissions" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the test file
head -30 management/server/http/handlers/peers/peers_handler_test.goRepository: netbirdio/netbird
Length of output: 833
🏁 Script executed:
# Search for mock generation code to determine which gomock fork is used
rg -n --type=go 'NewMockManager|MockManager' -A 3 -B 3Repository: netbirdio/netbird
Length of output: 50373
🏁 Script executed:
# Check gomock imports across the mock files
rg --type=go 'go\.uber\.org/mock/gomock|github\.com/golang/mock/gomock' -l | head -20Repository: netbirdio/netbird
Length of output: 1002
🏁 Script executed:
# Examine the permissions mock specifically
find . -name '*mock*.go' -type f | xargs rg -l 'MockManager' 2>/dev/nullRepository: netbirdio/netbird
Length of output: 286
🏁 Script executed:
# Check the actual test file for the specific lines mentioned
sed -n '16,24p; 107,118p; 208p' management/server/http/handlers/peers/peers_handler_test.goRepository: netbirdio/netbird
Length of output: 895
🏁 Script executed:
# Find network_map mock files
find . -name '*mock*.go' -type f | xargs grep -l 'NewMockController\|type.*Controller' | grep -i networkRepository: netbirdio/netbird
Length of output: 125
🏁 Script executed:
# Search for NewMockController definition in network_map
rg -A 5 'func NewMockController' --type=goRepository: netbirdio/netbird
Length of output: 608
🏁 Script executed:
# Check the network_map module structure
find . -path '*/network_map/*' -name '*.go' | head -20Repository: netbirdio/netbird
Length of output: 914
🏁 Script executed:
# Check which gomock fork is imported in network_map's mock file
head -20 management/internals/controllers/network_map/interface_mock.go | grep gomockRepository: netbirdio/netbird
Length of output: 94
🏁 Script executed:
# Get more context from the network_map mock file
head -30 management/internals/controllers/network_map/interface_mock.goRepository: netbirdio/netbird
Length of output: 1039
🏁 Script executed:
# Verify the exact line where gomock.Any() is used with the uber controller
sed -n '107,120p' management/server/http/handlers/peers/peers_handler_test.goRepository: netbirdio/netbird
Length of output: 531
🏁 Script executed:
# Let me verify the exact imports at the top of the test file to confirm both forks are used
sed -n '1,30p' management/server/http/handlers/peers/peers_handler_test.goRepository: netbirdio/netbird
Length of output: 833
Avoid mixing gomock forks in a single test.
You import both github.com/golang/mock/gomock and go.uber.org/mock/gomock (aliased as ugomock) and create controllers from each fork. The issue is more subtle than initially apparent: the network_map.MockController uses the uber fork, but at line 111, you call GetDNSDomain(gomock.Any()) using the golang fork's matcher with an uber-fork controller. This creates a type mismatch. Additionally, the permissions mocks already use the golang fork, so the test inconsistently mixes both forks across different components.
Standardize on the golang fork throughout this test (already used by permissions and settings mocks), or switch entirely to the uber fork. The simpler fix is to consolidate on the golang fork:
✅ Suggested consolidation (standardize on golang/mock fork)
"github.com/golang/mock/gomock"
"github.com/gorilla/mux"
- ugomock "go.uber.org/mock/gomock"
"golang.org/x/exp/maps"
@@
- ctrl := ugomock.NewController(t)
+ ctrl := gomock.NewController(t)
networkMapController := network_map.NewMockController(ctrl)
networkMapController.EXPECT().
GetDNSDomain(gomock.Any()).
Return("domain").
AnyTimes()
- ctrl2 := gomock.NewController(t)
- permissionsManager := permissions.NewMockManager(ctrl2)
+ permissionsManager := permissions.NewMockManager(ctrl)
permissionsManager.EXPECT().ValidateAccountAccess(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes()Note: This requires regenerating or updating network_map.MockController to use the golang/mock fork.
🤖 Prompt for AI Agents
In `@management/server/http/handlers/peers/peers_handler_test.go` around lines 16
- 24, The test mixes two gomock forks—github.com/golang/mock/gomock and
go.uber.org/mock/gomock—causing matcher/controller type mismatches; pick one
fork and update imports and mock types accordingly (prefer the existing
github.com/golang/mock/gomock used by permissions and settings mocks). Replace
usages of ugomock (and network_map.MockController generated for the uber fork)
with the golang/mock equivalents (network_map.MockController regenerated or
updated to the golang/mock package), change any ugomock.NewController calls to
gomock.NewController, and ensure all matcher uses (e.g., gomock.Any()) and mock
expectations reference the same gomock package throughout the file.
2 participants
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
New Features
Tests