Skip to content

[management] adding account id validation to accessible peers handler #5246

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pascal-fischer merged 2 commits into from
Feb 3, 2026
Merged

[management] adding account id validation to accessible peers handler #5246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
97f126b
adding account id validation to accessible peers handler
pascal-fischer Feb 3, 2026
8a6f7f4
fix test
pascal-fischer Feb 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions management/server/http/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,11 @@ import (
"time"

"github.com/gorilla/mux"
idpmanager "github.com/netbirdio/netbird/management/server/idp"
"github.com/rs/cors"
log "github.com/sirupsen/logrus"

idpmanager "github.com/netbirdio/netbird/management/server/idp"

"github.com/netbirdio/management-integrations/integrations"
"github.com/netbirdio/netbird/management/internals/controllers/network_map"
"github.com/netbirdio/netbird/management/internals/modules/zones"
Expand Down Expand Up @@ -137,7 +138,7 @@ func NewAPIHandler(ctx context.Context, accountManager account.Manager, networks
}

accounts.AddEndpoints(accountManager, settingsManager, router)
peers.AddEndpoints(accountManager, router, networkMapController)
peers.AddEndpoints(accountManager, router, networkMapController, permissionsManager)
users.AddEndpoints(accountManager, router)
users.AddInvitesEndpoints(accountManager, router)
users.AddPublicInvitesEndpoints(accountManager, router)
Expand Down
19 changes: 14 additions & 5 deletions management/server/http/handlers/peers/peers_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
nbcontext "github.com/netbirdio/netbird/management/server/context"
"github.com/netbirdio/netbird/management/server/groups"
nbpeer "github.com/netbirdio/netbird/management/server/peer"
"github.com/netbirdio/netbird/management/server/permissions"
"github.com/netbirdio/netbird/management/server/types"
"github.com/netbirdio/netbird/shared/management/http/api"
"github.com/netbirdio/netbird/shared/management/http/util"
Expand All @@ -26,11 +27,12 @@ import (
// Handler is a handler that returns peers of the account
type Handler struct {
accountManager account.Manager
permissionsManager permissions.Manager
networkMapController network_map.Controller
}

func AddEndpoints(accountManager account.Manager, router *mux.Router, networkMapController network_map.Controller) {
peersHandler := NewHandler(accountManager, networkMapController)
func AddEndpoints(accountManager account.Manager, router *mux.Router, networkMapController network_map.Controller, permissionsManager permissions.Manager) {
peersHandler := NewHandler(accountManager, networkMapController, permissionsManager)
router.HandleFunc("/peers", peersHandler.GetAllPeers).Methods("GET", "OPTIONS")
router.HandleFunc("/peers/{peerId}", peersHandler.HandlePeer).
Methods("GET", "PUT", "DELETE", "OPTIONS")
Expand All @@ -42,10 +44,11 @@ func AddEndpoints(accountManager account.Manager, router *mux.Router, networkMap
}

// NewHandler creates a new peers Handler
func NewHandler(accountManager account.Manager, networkMapController network_map.Controller) *Handler {
func NewHandler(accountManager account.Manager, networkMapController network_map.Controller, permissionsManager permissions.Manager) *Handler {
return &Handler{
accountManager: accountManager,
networkMapController: networkMapController,
permissionsManager: permissionsManager,
}
}

Expand Down Expand Up @@ -359,13 +362,19 @@ func (h *Handler) GetAccessiblePeers(w http.ResponseWriter, r *http.Request) {
return
}

account, err := h.accountManager.GetAccountByID(r.Context(), accountID, activity.SystemInitiator)
user, err := h.accountManager.GetUserByID(r.Context(), userID)
if err != nil {
util.WriteError(r.Context(), err, w)
return
}

user, err := h.accountManager.GetUserByID(r.Context(), userID)
err = h.permissionsManager.ValidateAccountAccess(r.Context(), accountID, user, false)
if err != nil {
util.WriteError(r.Context(), status.NewPermissionDeniedError(), w)
return
}

account, err := h.accountManager.GetAccountByID(r.Context(), accountID, activity.SystemInitiator)
if err != nil {
util.WriteError(r.Context(), err, w)
return
Expand Down
11 changes: 9 additions & 2 deletions management/server/http/handlers/peers/peers_handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,15 @@ import (
"testing"
"time"

"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"
Comment on lines +16 to +24
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, find and examine the test file
head -30 management/server/http/handlers/peers/peers_handler_test.go

Repository: 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 3

Repository: 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 -20

Repository: 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/null

Repository: 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.go

Repository: 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 network

Repository: netbirdio/netbird

Length of output: 125

🏁 Script executed:

# Search for NewMockController definition in network_map
rg -A 5 'func NewMockController' --type=go

Repository: netbirdio/netbird

Length of output: 608

🏁 Script executed:

# Check the network_map module structure
find . -path '*/network_map/*' -name '*.go' | head -20

Repository: 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 gomock

Repository: 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.go

Repository: 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.go

Repository: 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.go

Repository: 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.

"github.com/netbirdio/netbird/management/server/types"
"github.com/netbirdio/netbird/shared/auth"
"github.com/netbirdio/netbird/shared/management/http/api"
Expand Down Expand Up @@ -102,14 +104,18 @@ func initTestMetaData(t *testing.T, peers ...*nbpeer.Peer) *Handler {
},
}

ctrl := gomock.NewController(t)
ctrl := ugomock.NewController(t)

networkMapController := network_map.NewMockController(ctrl)
networkMapController.EXPECT().
GetDNSDomain(gomock.Any()).
Return("domain").
AnyTimes()

ctrl2 := gomock.NewController(t)
permissionsManager := permissions.NewMockManager(ctrl2)
permissionsManager.EXPECT().ValidateAccountAccess(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes()

return &Handler{
accountManager: &mock_server.MockAccountManager{
UpdatePeerFunc: func(_ context.Context, accountID, userID string, update *nbpeer.Peer) (*nbpeer.Peer, error) {
Expand Down Expand Up @@ -199,6 +205,7 @@ func initTestMetaData(t *testing.T, peers ...*nbpeer.Peer) *Handler {
},
},
networkMapController: networkMapController,
permissionsManager: permissionsManager,
}
}

Expand Down
Loading