[management] Move permissions management from managers to API handlers#5482
[management] Move permissions management from managers to API handlers#5482pascal-fischer wants to merge 37 commits intomainfrom
Conversation
|
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:
📝 WalkthroughWalkthroughShifted permission enforcement to an HTTP middleware layer: added permission wrapper helpers, removed permissions.Manager from many managers and inline ValidateUserPermissions calls, updated handlers to accept explicit userAuth, and adjusted tests, mocks, interfaces, and SQL fixtures to the new wiring. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PermMW as PermissionMiddleware
participant Handler
participant Manager
participant Store
Client->>PermMW: HTTP request
PermMW->>PermMW: extract UserAuth & ValidateUserPermissions
alt denied
PermMW-->>Client: 403 / permission error
else allowed
PermMW->>Handler: call handler with userAuth
Handler->>Manager: call manager (no inline permission checks)
Manager->>Store: query or mutate
Store-->>Manager: result
Manager-->>Handler: result
Handler-->>Client: HTTP response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
# Conflicts: # management/internals/modules/reverseproxy/service/manager/api.go # management/server/http/handler.go
…manager # Conflicts: # management/internals/modules/reverseproxy/domain/manager/manager.go # management/internals/modules/reverseproxy/service/manager/api.go # management/internals/server/modules.go # management/server/http/testing/testing_tools/channel/channel.go
# Conflicts: # management/internals/modules/reverseproxy/service/manager/api.go # management/server/http/testing/testing_tools/channel/channel.go
# Conflicts: # management/server/account_test.go # management/server/http/handlers/networks/routers_handler.go
# Conflicts: # go.mod # go.sum # management/server/account_test.go # management/server/http/testing/testing_tools/channel/channel.go
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
management/server/dns_test.go (1)
97-104:⚠️ Potential issue | 🟡 MinorRemove the stale manager-level permission denial case.
SaveDNSSettingsno longer denies regular users at the manager layer, and this test currently accepts that success becauseshouldFailis only checked inside the error branch. Move this coverage to the HTTP permission-wrapper tests, or make the failure assertion explicit and remove this case here.Proposed fix for the direct manager test
- { - name: "Should Not Update Settings As Regular User", - userID: dnsRegularUserID, - inputSettings: &types.DNSSettings{ - DisabledManagementGroups: []string{dnsGroup1ID}, - }, - shouldFail: true, - }, @@ err = am.SaveDNSSettings(context.Background(), account.Id, testCase.userID, testCase.inputSettings) - if err != nil { - if testCase.shouldFail { - return - } - t.Error(err) + if testCase.shouldFail { + require.Error(t, err) + return } + require.NoError(t, err) updatedAccount, err := am.Store.GetAccount(context.Background(), account.Id)Also applies to: 132-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/dns_test.go` around lines 97 - 104, The test case "Should Not Update Settings As Regular User" is stale because SaveDNSSettings no longer denies regular users at the manager layer; remove or update this case in management/server/dns_test.go (and the duplicate at the other location) by either deleting the entry or changing it to assert success (set shouldFail=false and assert no error / validate settings were saved), and move any HTTP permission-wrapper-specific denial coverage to the HTTP/permission wrapper tests; reference the SaveDNSSettings call and the test case name to locate and update the code.management/server/http/handlers/peers/peers_handler_test.go (1)
306-330:⚠️ Potential issue | 🟡 MinorAssert the recorder that actually served the request.
rris never passed torouter.ServeHTTP, so this status check can miss401/403/500responses from the wrapped handlers. Useres.StatusCodeorrecorder.Codeinstead.Proposed fix
- rr := httptest.NewRecorder() - p := initTestMetaData(t, peer, peer1) @@ - if status := rr.Code; status != tc.expectedStatus { + if status := res.StatusCode; status != tc.expectedStatus { t.Fatalf("handler returned wrong status code: got %v want %v", - status, http.StatusOK) + status, tc.expectedStatus) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/http/handlers/peers/peers_handler_test.go` around lines 306 - 330, The test is asserting the wrong recorder variable (rr) which was never used with router.ServeHTTP; update the status assertion in Test (around the request loop using initTestMetaData, router.HandleFunc and router.ServeHTTP) to check the actual response from the served request — use recorder.Code (or res.StatusCode) instead of rr.Code so that GetAllPeers/GetPeer/UpdatePeer handler responses (401/403/500 etc.) are correctly asserted.management/server/http/handlers/users/users_handler_test.go (1)
581-705:⚠️ Potential issue | 🟡 MinorKeep the “without auth” case routed through the wrapper.
The direct call now always passes a
*auth.UserAuth, so the “without auth” case no longer verifies missing-auth handling. UsewrapHandler(userHandler.getCurrentUser)with no auth in the request context for that case.Suggested direction
- userAuth := &auth.UserAuth{ - UserId: tc.requestAuth.UserId, - AccountId: existingAccountID, - } - userHandler.getCurrentUser(rr, req, userAuth) + router := mux.NewRouter() + router.HandleFunc("/api/users/current", wrapHandler(userHandler.getCurrentUser)).Methods(http.MethodGet) + router.ServeHTTP(rr, req)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/http/handlers/users/users_handler_test.go` around lines 581 - 705, The "without auth" test currently bypasses the HTTP wrapper by calling userHandler.getCurrentUser directly with a non-nil *auth.UserAuth, so it doesn't exercise missing-auth behavior; update the test loop to route the no-auth case through wrapHandler(userHandler.getCurrentUser) and call its ServeHTTP(rr, req) with no user set in the request, while keeping the existing pattern for cases with auth (set nbcontext.SetUserAuthInRequest and call userHandler.getCurrentUser(rr, req, userAuth)). Ensure you only construct and pass the *auth.UserAuth (userAuth variable) for cases where tc.requestAuth.UserId != "" and do not pass any userAuth for the wrapped call.management/internals/modules/reverseproxy/service/manager/manager.go (1)
774-790:⚠️ Potential issue | 🟠 MajorDelete service targets during bulk deletion too.
DeleteServiceremoves targets before deleting the service, butDeleteAllServicesskips that cleanup. Unless every store backend cascades this relationship, bulk deletion can leave orphan targets or fail on FK constraints.Proposed fix
for _, svc := range services { + if err = transaction.DeleteServiceTargets(ctx, accountID, svc.ID); err != nil { + return fmt.Errorf("failed to delete service targets: %w", err) + } if err = transaction.DeleteService(ctx, accountID, svc.ID); err != nil { return fmt.Errorf("failed to delete service: %w", err) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/modules/reverseproxy/service/manager/manager.go` around lines 774 - 790, DeleteAllServices currently deletes services without removing associated targets, which can leave orphans or violate FKs; update Manager.DeleteAllServices so that inside the transaction loop you first call the store method that removes service targets (e.g., transaction.DeleteTargets(ctx, accountID, svc.ID) or whatever the store exposes to delete targets for a service) and then call transaction.DeleteService(ctx, accountID, svc.ID); ensure both calls run within the same ExecuteInTransaction block and propagate errors from the target-deletion call just like the DeleteService call.management/server/account.go (1)
1326-1356:⚠️ Potential issue | 🟠 MajorAccount membership validation gap in IsChild path.
The comment at line 1355–1356 is inaccurate.
GetUserByUserIDvalidates only that a user exists globally—it does not validate that the user belongs to the returnedaccountID.For the
IsChildpath (line 1351–1352),getAccountIDWithAuthorizationClaimsreturnsuserAuth.AccountIdafter only checking that the account exists viaAccountExists. The function never verifies that the user is actually a member of that account. The returned(accountID, user.Id)pair could represent a user in a different account.While
ValidateAccountAccessin the permissions middleware does checkuser.AccountID == accountIDlater in the HTTP flow, this validation happens afterGetAccountIDFromUserAuthreturns and the pair is propagated. Please add an explicit account membership validation here, or document why this ordering is safe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/account.go` around lines 1326 - 1356, GetAccountIDFromUserAuth currently returns (accountID, user.Id) for the userAuth.IsChild path without ensuring the fetched user actually belongs to that account; update GetAccountIDFromUserAuth to validate membership by comparing the retrieved user's AccountID to the computed accountID (from getAccountIDWithAuthorizationClaims) after calling Store.GetUserByUserID, and if they differ return an appropriate error (e.g., permission denied or not found). Ensure you reference GetAccountIDFromUserAuth, getAccountIDWithAuthorizationClaims, and Store.GetUserByUserID when making this check so the IsChild branch cannot return a user/account pair from different accounts.management/server/user.go (1)
906-939:⚠️ Potential issue | 🔴 CriticalNil pointer dereference when
initiatorUserID == activity.SystemInitiator.
useris only populated inside theif initiatorUserID != activity.SystemInitiatorbranch (lines 909-915). When the initiator is the system,userstaysnil, and line 922user.HasAdminPower() || user.IsServiceUserwill panic. The latercase user != nil && user.AccountID == accountIDguard at line 932 confirms nil is expected for this path, so thehasFullAccesscomputation must be guarded too.Note the shadowed
errinside theifblock (line 910) is also dead since it never surfaces outside; collapse it.🛡️ Proposed fix
func (am *DefaultAccountManager) GetUsersFromAccount(ctx context.Context, accountID, initiatorUserID string) (map[string]*types.UserInfo, error) { var user *types.User var err error if initiatorUserID != activity.SystemInitiator { - result, err := am.Store.GetUserByUserID(ctx, store.LockingStrengthNone, initiatorUserID) + user, err = am.Store.GetUserByUserID(ctx, store.LockingStrengthNone, initiatorUserID) if err != nil { return nil, fmt.Errorf("failed to get user: %w", err) } - user = result } // Permission checks are now handled by the HTTP middleware via WithPermission wrapper // This internal method is called from authenticated/authorized handlers accountUsers := []*types.User{} - // Determine if user has full access based on their role - hasFullAccess := user.HasAdminPower() || user.IsServiceUser + // Determine if user has full access based on their role. + // System initiator has full access; otherwise, fall back to the user's role. + hasFullAccess := initiatorUserID == activity.SystemInitiator || + (user != nil && (user.HasAdminPower() || user.IsServiceUser)) switch { case hasFullAccess:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/user.go` around lines 906 - 939, In GetUsersFromAccount, avoid the nil deref and err shadowing: collapse the shadowed err by assigning to the outer err when calling am.Store.GetUserByUserID (use = not :=), and compute hasFullAccess only if user is non-nil (e.g. hasFullAccess := user != nil && (user.HasAdminPower() || user.IsServiceUser)) before the switch so the user.HasAdminPower/IsServiceUser calls cannot panic; leave the rest of the switch logic and final call to BuildUserInfosForAccount unchanged.management/server/http/handlers/policies/posture_checks_handler.go (1)
130-136:⚠️ Potential issue | 🟠 MajorGuard
req.Checksbefore dereferencing it.A payload like
{"name":"x"}leavesreq.Checksnil and panics atreq.Checks.GeoLocationCheck, turning a validation error into a 500/crash path.🐛 Proposed fix
- if geoLocationCheck := req.Checks.GeoLocationCheck; geoLocationCheck != nil { + if req.Checks == nil { + util.WriteError(r.Context(), status.Errorf(status.InvalidArgument, "posture checks are required"), w) + return + } + + if geoLocationCheck := req.Checks.GeoLocationCheck; geoLocationCheck != nil { if p.geolocationManager == nil { util.WriteError(r.Context(), status.Errorf(status.PreconditionFailed, "Geo location database is not initialized. "+ "Check the self-hosted Geo database documentation at https://docs.netbird.io/selfhosted/geo-support"), w) return } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/http/handlers/policies/posture_checks_handler.go` around lines 130 - 136, The code dereferences req.Checks without ensuring it's non-nil, causing a panic for payloads that omit checks; update the handler to guard req.Checks first (e.g., check req.Checks != nil) before accessing req.Checks.GeoLocationCheck inside the posture checks logic in posture_checks_handler.go (the block referencing req.Checks.GeoLocationCheck and p.geolocationManager). Ensure the existing geolocationManager nil check and error response remain unchanged but are executed only when req.Checks is non-nil and GeoLocationCheck is present.management/server/http/handlers/accounts/accounts_handler.go (1)
231-304:⚠️ Potential issue | 🟠 MajorBind
{accountId}to the authenticated account before mutating.
WithPermissionauthorizesuserAuth, but these handlers mutate the pathaccountId. Reject mismatches before validation/update/delete so a user authorized in one account cannot target another account ID.🛡️ Proposed fix
func (h *handler) updateAccount(w http.ResponseWriter, r *http.Request, userAuth *auth.UserAuth) { vars := mux.Vars(r) accountID := vars["accountId"] if len(accountID) == 0 { util.WriteError(r.Context(), status.Errorf(status.InvalidArgument, "invalid accountID ID"), w) return } + if accountID != userAuth.AccountId { + util.WriteError(r.Context(), status.Errorf(status.PermissionDenied, "account ID does not match authenticated account"), w) + return + } var req api.PutApiAccountsAccountIdJSONRequestBodyfunc (h *handler) deleteAccount(w http.ResponseWriter, r *http.Request, userAuth *auth.UserAuth) { vars := mux.Vars(r) targetAccountID := vars["accountId"] if len(targetAccountID) == 0 { util.WriteError(r.Context(), status.Errorf(status.InvalidArgument, "invalid account ID"), w) return } + if targetAccountID != userAuth.AccountId { + util.WriteError(r.Context(), status.Errorf(status.PermissionDenied, "account ID does not match authenticated account"), w) + return + } err := h.accountManager.DeleteAccount(r.Context(), targetAccountID, userAuth.UserId)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/http/handlers/accounts/accounts_handler.go` around lines 231 - 304, The handlers updateAccount and deleteAccount are currently mutating whichever {accountId} is in the path without verifying it matches the caller's authenticated account; add an early check in both updateAccount (after extracting accountID) and deleteAccount (after extracting targetAccountID) that compares the path value to the authenticated account identifier on userAuth (e.g. userAuth.AccountId — if your auth struct uses a different field name substitute that, e.g. userAuth.UserAccountId), and return an appropriate error (status.PermissionDenied or status.InvalidArgument) if they differ so users cannot mutate other accounts before calling UpdateAccountSettings/DeleteAccount/UpdateAccountOnboarding/GetAccountMeta. Ensure the check runs before any validation or calls to h.accountManager.management/server/http/handlers/peers/peers_handler.go (1)
342-361:⚠️ Potential issue | 🟠 MajorNetworkAdmin and other custom roles with Peers:Read permission are blocked by hard-coded role checks.
The middleware validates
Peers:Readpermission and allows the request, but the handler code at lines 343-361 performs its own authorization using only hard-coded checks forAdmin/Owner/ServiceUser/IsChild. This means a NetworkAdmin (or any other role with explicitPeers:Readpermission) receives an empty response whenRegularUsersViewBlockedis enabled or when querying a non-owned peer, despite being authorized by the permission system.The handler does not have access to the permission decision from the middleware. Either call
ValidateUserPermissionswithin the handler to check the actual permission, or remove the hard-coded role checks and trust the middleware authorization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/http/handlers/peers/peers_handler.go` around lines 342 - 361, The handler is re-checking authorization using hard-coded role checks (user.Role, types.UserRoleAdmin, types.UserRoleOwner, user.IsServiceUser, userAuth.IsChild) which blocks roles that were granted Peers:Read by middleware; replace this local role-based gate with a real permission check: either call the existing ValidateUserPermissions (or the middleware-exposed permission result) inside the handler to verify Peers:Read for the current user, or remove the hard-coded branch that uses account.Settings.RegularUsersViewBlocked / peer ownership to rely on the middleware permission decision; ensure the code references the same permission symbol (Peers:Read) and uses ValidateUserPermissions (or the middleware flag) rather than user.Role/User.IsServiceUser/userAuth.IsChild.
🧹 Nitpick comments (14)
management/internals/modules/zones/manager/manager_test.go (1)
29-52: Drop the now-unused gomock controller from the test helper.After removing the permissions mock,
ctrlis only created, returned, and finished. Removing it keeps the refactor tidy.♻️ Proposed cleanup
-func setupTest(t *testing.T) (*managerImpl, store.Store, *mock_server.MockAccountManager, *gomock.Controller, func()) { +func setupTest(t *testing.T) (*managerImpl, store.Store, *mock_server.MockAccountManager, func()) { t.Helper() @@ - ctrl := gomock.NewController(t) mockAccountManager := &mock_server.MockAccountManager{} manager := NewManager(testStore, mockAccountManager, testDNSDomain).(*managerImpl) - return manager, testStore, mockAccountManager, ctrl, cleanup + return manager, testStore, mockAccountManager, cleanup }Apply the same tuple cleanup at each
setupTest(t)call site and remove thegomockimport.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/modules/zones/manager/manager_test.go` around lines 29 - 52, The setupTest helper currently creates and returns an unused gomock controller (ctrl) — remove the gomock.Controller creation (gomock.NewController(t)), drop ctrl from the setupTest return signature and from the returned tuple, and delete any ctrl.Finish() calls in callers; also remove the gomock import. Update all call sites of setupTest to stop expecting the extra ctrl value (e.g., places that destructure manager, store, mockAccountManager, ctrl, cleanup should now destructure manager, store, mockAccountManager, cleanup). Ensure manager := NewManager(...) and mock_account usage are unchanged.management/server/http/handlers/routes/routes_handler_test.go (1)
28-38: Test helper looks good; consider sharing across handler tests.
wrapHandlerduplicates the production middleware's auth-resolution behavior and is likely to be replicated verbatim in other handler test files updated in this PR (e.g., users, networks, etc.). Consider extracting it to a shared test utility (e.g., undermanagement/server/http/testingor similar) to keep test wiring consistent as more handlers migrate to theuserAuth-taking signature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/http/handlers/routes/routes_handler_test.go` around lines 28 - 38, The test helper wrapHandler duplicates auth-resolution and should be extracted to a shared test utility: create a new test-only package (e.g., management/server/http/testing) and move the function there, renaming it to exported WrapHandler(func(w http.ResponseWriter, r *http.Request, userAuth *auth.UserAuth)) http.HandlerFunc so other handler tests can reuse it; implement it using nbcontext.GetUserAuthFromContext and the same error handling, update all test files that used the local wrapHandler to import the new testing package and call testing.WrapHandler, and adjust imports/visibility accordingly.management/server/nameserver_test.go (1)
794-806: Reuse the existingpeersManagerinstead of constructing a second one for the ephemeral manager.Line 795 creates
peersManager, then line 806 constructs a separatepeers.NewManager(store)inline forephemeral_manager.NewEphemeralManager(...). Ifpeers.Managercarries any state (caches, subscribers, in-flight updates), the ephemeral manager will operate against a different instance than the rest of the wiring, which can silently mask test failures or cause races in concurrent tests.♻️ Proposed fix
- networkMapController := controller.NewController(ctx, store, metrics, updateManager, requestBuffer, MockIntegratedValidator{}, settingsMockManager, "netbird.selfhosted", port_forwarding.NewControllerMock(), ephemeral_manager.NewEphemeralManager(store, peers.NewManager(store)), &config.Config{}) + networkMapController := controller.NewController(ctx, store, metrics, updateManager, requestBuffer, MockIntegratedValidator{}, settingsMockManager, "netbird.selfhosted", port_forwarding.NewControllerMock(), ephemeral_manager.NewEphemeralManager(store, peersManager), &config.Config{})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/nameserver_test.go` around lines 794 - 806, The test constructs two peers.Manager instances; replace the inline peers.NewManager(store) passed to ephemeral_manager.NewEphemeralManager with the already-created peersManager so the ephemeral manager shares the same instance (i.e., change the call that currently uses peers.NewManager(store) to use peersManager) to avoid divergent state/caches/subscribers between managers.management/server/http/handlers/instance/instance_handler_test.go (1)
303-311: Assert the exact permission tuple in the endpoint wiring tests.Using
gomock.Any()for module and operation means these tests would still pass if/instance/versionwere accidentally wired to the wrong permission. Since this PR moves authorization to endpoint wrappers, please assert the expectedmodules.*andoperations.*values here.Also applies to: 338-346
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/http/handlers/instance/instance_handler_test.go` around lines 303 - 311, The test currently stubs permissionsManager.WithPermission with gomock.Any() for module and operation, which allows incorrect permission wiring to slip through; modify the mock expectation in instance_handler_test.go so WithPermission is expected with the exact modules.Instance and operations.View (or the correct operation constant used by AddVersionEndpoint) instead of gomock.Any(), e.g., set permissionsManager.EXPECT().WithPermission(modules.Instance, operations.View, gomock.Any(), ...).DoAndReturn(...) (and make the same change in the other block at lines 338-346) so the test asserts the exact permission tuple used by AddVersionEndpoint.management/server/dns.go (1)
21-27: Update the stale authorization comments.These methods no longer validate user roles; authorization now belongs at the API handler layer.
Proposed fix
-// GetDNSSettings validates a user role and returns the DNS settings for the provided account ID +// GetDNSSettings returns the DNS settings for the provided account ID. func (am *DefaultAccountManager) GetDNSSettings(ctx context.Context, accountID string, userID string) (*types.DNSSettings, error) { return am.Store.GetAccountDNSSettings(ctx, store.LockingStrengthNone, accountID) } -// SaveDNSSettings validates a user role and updates the account's DNS settings +// SaveDNSSettings updates the account's DNS settings. func (am *DefaultAccountManager) SaveDNSSettings(ctx context.Context, accountID string, userID string, dnsSettingsToSave *types.DNSSettings) error {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/dns.go` around lines 21 - 27, The comments above DefaultAccountManager.GetDNSSettings and DefaultAccountManager.SaveDNSSettings are stale because these methods no longer perform user role validation; update or remove the misleading “validates a user role” phrasing to reflect that authorization is handled at the API handler layer — locate the comments on the GetDNSSettings and SaveDNSSettings methods in management/server/dns.go and change them to state that these methods fetch/update DNS settings without performing authorization, delegating auth to the API layer.management/server/http/handler.go (1)
156-177: Parameter ordering forpermissionsManager/routeris inconsistent across registration calls.Most
AddEndpointsvariants take(..., router, permissionsManager)(e.g. lines 156, 158–168, 172, 174), butnetworks.AddEndpoints(line 169) andreverseproxymanager.RegisterEndpoints(line 176) take(..., permissionsManager, router), while the zones/recordsRegisterEndpointsputrouterfirst. Consider aligning all signatures to one convention to reduce the risk of mis-wiring when arguments are of compatible types. SincerouterandpermissionsManagerare distinct types the compiler will catch mistakes, so this is purely a readability nit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/http/handler.go` around lines 156 - 177, Several endpoint registration calls use inconsistent parameter ordering for router and permissionsManager which harms readability; standardize all AddEndpoints/RegisterEndpoints signatures and call sites to use the same ordering (choose either ... , router, permissionsManager or ... , permissionsManager, router) and update the implementations and all call sites accordingly—specifically update networks.AddEndpoints, reverseproxymanager.RegisterEndpoints, zonesManager.RegisterEndpoints, recordsManager.RegisterEndpoints (and their function definitions) to match the chosen convention used by accounts.AddEndpoints, peers.AddEndpoints, users.AddEndpoints, policies.AddEndpoints, groups.AddEndpoints, routes.AddEndpoints, dns.AddEndpoints, events.AddEndpoints, idp.AddEndpoints, and instance.AddEndpoints so every registration uses the identical parameter order.management/internals/modules/reverseproxy/domain/manager/manager.go (1)
51-104: UnuseduserIDparameter inGetDomainsafter permissions removal.With the permission check removed,
userIDis no longer referenced anywhere inGetDomains's body.CreateDomain/DeleteDomain/ValidateDomainstill use it forStoreEvent, so this one stands out. You may want to either drop it for clarity or keep it for signature symmetry with the other methods — up to you.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/modules/reverseproxy/domain/manager/manager.go` around lines 51 - 104, GetDomains has an unused userID parameter; remove userID from Manager.GetDomains signature and all call sites (and update the Manager interface if present) so the method becomes GetDomains(ctx context.Context, accountID string) ([]*domain.Domain, error); ensure any tests and usages are updated accordingly (do not change CreateDomain/DeleteDomain/ValidateDomain which still take userID), or if you prefer signature symmetry instead keep userID but actually pass it into a StoreEvent or similar — choose one approach and make the corresponding consistent changes across Manager, its interface, and callers.management/internals/modules/zones/manager/manager.go (1)
28-34: Document that these read methods no longer perform authorization.
userIDis now unused inGetAllZonesandGetZone, and no permission check is performed. Since the method signatures still takeuserID, a future caller could reasonably assume the manager is gating access. A short doc comment stating that authorization is the caller's responsibility (enforced by HTTP middleware) will prevent accidental misuse from non-HTTP code paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/modules/zones/manager/manager.go` around lines 28 - 34, The GetAllZones and GetZone methods on managerImpl currently accept a userID parameter but perform no authorization; update their doc comments to state clearly that these are read-only store accessors that do not enforce permissions, that the userID parameter is unused and retained for signature compatibility, and that authorization must be enforced by callers (e.g., HTTP middleware); mention the exact methods GetAllZones and GetZone and advise keeping the parameter for compatibility while documenting the responsibility shift to callers to avoid accidental misuse from non-HTTP code paths.management/server/group.go (1)
36-51: Redundant nil check after a successfulGetUserByUserID.Per the repository's Go convention, once
err == nilyou can rely on the returned value being populated; theif user == nilcheck is dead code unless the store contract explicitly documents otherwise. Collapse it.♻️ Proposed fix
user, err := am.Store.GetUserByUserID(ctx, store.LockingStrengthNone, userID) if err != nil { return err } - if user == nil { - return status.NewUserNotFoundError(userID) - } - if user.AccountID != accountID { return status.NewUserNotPartOfAccountError() }Based on learnings from netbirdio/netbird PR
#5441: methods returning(T, error)should propagate errors and avoid redundant nil checks on the returned value after the error path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/group.go` around lines 36 - 51, Remove the redundant nil check for user after calling am.Store.GetUserByUserID: since GetUserByUserID returns (user, error) and the code already returned on err != nil, delete the if user == nil { return status.NewUserNotFoundError(userID) } block and let subsequent checks (user.AccountID, user.IsBlocked()) operate on the non-nil user; keep the existing error-returning calls like status.NewUserNotPartOfAccountError() and status.NewUserBlockedError() intact.management/server/http/testing/integration/invites_handler_integration_test.go (3)
114-114: Simplify the URL construction.
strings.Replace("/api/users/invites/{inviteId}", "{inviteId}", "someInviteId", 1)is more verbose than needed;"/api/users/invites/someInviteId"(orfmt.Sprintf) reads more clearly and avoids the placeholder indirection.Also applies to: 145-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/http/testing/integration/invites_handler_integration_test.go` at line 114, The test constructs the request URL using an unnecessary strings.Replace; update the call to testing_tools.BuildRequest in invites_handler_integration_test.go (the req := testing_tools.BuildRequest(...) line) to use a simple literal "/api/users/invites/someInviteId" or fmt.Sprintf("/api/users/invites/%s", "someInviteId") instead of strings.Replace; apply the same simplification to the similar call at the other occurrence around line 145.
22-154: Factor out the shared user matrix across the four invite tests.All four tests (
Test_Invites_List,Test_Invites_Create,Test_Invites_Delete,Test_Invites_Regenerate) use an identical 8-entry user table. Extract it into a package-levelvaror a helper returning the slice to reduce duplication and keep future additions (new roles, new edge tokens) consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/http/testing/integration/invites_handler_integration_test.go` around lines 22 - 154, Extract the repeated 8-entry user table used in Test_Invites_List, Test_Invites_Create, Test_Invites_Delete and Test_Invites_Regenerate into a single shared symbol (e.g., a package-level var inviteTestUsers or a helper func getInviteTestUsers()) and replace each local users := []struct{...}{...} block with a reference to that shared symbol; ensure the shared data uses the same testing_tools constants (testing_tools.TestUserId, TestAdminId, TestOwnerId, TestServiceUserId, TestServiceAdminId, BlockedUserId, OtherUserId, InvalidToken) and keep the same fields (name, userId, expectResponse) so existing test bodies (BuildRequest calls and testing_tools.ReadResponse) require no other changes.
17-20: Test only asserts permission denial, not positive invite outcomes.Because the integration infrastructure doesn't wire an embedded IDP, authorized users can only ever reach the
PreconditionFailedbranch. That's fine for verifying the permission middleware negative path, but it means these tests won't catch regressions in the actual invite handler logic (validation, token generation, etc.). Consider adding a companion test run with embedded IDP enabled, or at least a unit-level test over the handler bodies.Also applies to: 46-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/http/testing/integration/invites_handler_integration_test.go` around lines 17 - 20, The tests in invites_handler_integration_test.go only exercise the permission-denial path (PreconditionFailed) and miss validating the invite handler behavior; add either (a) a companion integration test that enables the embedded IDP so an authorized flow reaches the invite handler and asserts positive outcomes (validation, token generation, success status), or (b) unit tests directly targeting the invite handler logic (the handler function under test in this file and its internal validation/token generation routines) to cover request validation, token creation, and success responses; ensure the new tests verify happy-path status codes and payloads in addition to the existing permission-denial assertions.management/server/http/handlers/policies/geolocations_handler.go (1)
24-44: Consider droppingpermissionsManagerfromgeolocationsHandlernow that WithPermission owns authorization.The
permissionsManagerfield ongeolocationsHandler(line 28) is no longer referenced bygetAllCountries/getCitiesByCountrysince permission validation moved to the route wrapper. The field and its constructor plumbing (line 38-43) can be removed to reduce surface area.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/http/handlers/policies/geolocations_handler.go` around lines 24 - 44, Remove the unused permissionsManager field from the geolocationsHandler struct and stop passing it into the constructor/newGeolocationsHandlerHandler; update newGeolocationsHandlerHandler to only accept account.Manager and geolocation.Geolocation and return a geolocationsHandler without the permissionsManager field, and update AddLocationsEndpoints to construct the handler with only accountManager and locationManager (but continue to use permissionsManager.WithPermission for the routes). Also remove any references to the permissionsManager field inside geolocationsHandler so the type and its constructor no longer carry the unused dependency.management/internals/modules/permissions/manager.go (1)
24-91: Document the dual-purposeAuthErrorHandlercontract.
WithPermissioninvokesonAuthErron two distinct failure modes:
- Validation failure from
ValidateUserPermissions(e.g., user not found, blocked — returns wrapped error).- Permission denied (
status.NewPermissionDeniedError()).Callers must inspect the error via
status.FromErrorto distinguish these (the existinggetOwnUserhandler already relies on this). A short godoc onAuthErrorHandlerand/orWithPermissionnoting that implementations receive different error types for these two cases, and that returningtruesuppresses the defaultutil.WriteError, would prevent misuse.Also: the variadic
authErrHandler ...AuthErrorHandlersilently ignores entries beyond the first. Either document this or tighten the signature to a single optional function pointer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/modules/permissions/manager.go` around lines 24 - 91, The AuthErrorHandler contract is ambiguous: WithPermission calls the provided onAuthErr for both validation failures from ValidateUserPermissions and for a permission-denied case created with status.NewPermissionDeniedError(), and returning true suppresses the default util.WriteError; add a short godoc to AuthErrorHandler (and/or WithPermission) documenting the dual-purpose error types (caller should use status.FromError to distinguish validation vs denied errors) and that returning true prevents the default util.WriteError response, and either change the variadic authErrHandler ...AuthErrorHandler to a single optional AuthErrorHandler parameter or document that only the first element is used (onAuthErr) to avoid silent ignores of additional handlers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ccc03b1e-5c15-4000-9b54-929c7ab1dab6
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (115)
client/cmd/testutil_test.goclient/internal/engine_test.goclient/server/server_test.gogo.modmanagement/internals/modules/peers/manager.gomanagement/internals/modules/permissions/manager.gomanagement/internals/modules/permissions/manager_mock.gomanagement/internals/modules/permissions/modules/module.gomanagement/internals/modules/permissions/operations/operation.gomanagement/internals/modules/permissions/roles/admin.gomanagement/internals/modules/permissions/roles/auditor.gomanagement/internals/modules/permissions/roles/network_admin.gomanagement/internals/modules/permissions/roles/owner.gomanagement/internals/modules/permissions/roles/role_permissions.gomanagement/internals/modules/permissions/roles/user.gomanagement/internals/modules/reverseproxy/accesslogs/manager/api.gomanagement/internals/modules/reverseproxy/accesslogs/manager/manager.gomanagement/internals/modules/reverseproxy/domain/manager/api.gomanagement/internals/modules/reverseproxy/domain/manager/manager.gomanagement/internals/modules/reverseproxy/proxy/proxy.gomanagement/internals/modules/reverseproxy/service/manager/api.gomanagement/internals/modules/reverseproxy/service/manager/l4_port_test.gomanagement/internals/modules/reverseproxy/service/manager/manager.gomanagement/internals/modules/reverseproxy/service/manager/manager_test.gomanagement/internals/modules/zones/manager/api.gomanagement/internals/modules/zones/manager/manager.gomanagement/internals/modules/zones/manager/manager_test.gomanagement/internals/modules/zones/records/manager/api.gomanagement/internals/modules/zones/records/manager/manager.gomanagement/internals/modules/zones/records/manager/manager_test.gomanagement/internals/server/boot.gomanagement/internals/server/modules.gomanagement/server/account.gomanagement/server/account/manager.gomanagement/server/account/manager_mock.gomanagement/server/account_test.gomanagement/server/dns.gomanagement/server/dns_test.gomanagement/server/event.gomanagement/server/group.gomanagement/server/group_test.gomanagement/server/groups/manager.gomanagement/server/http/handler.gomanagement/server/http/handlers/accounts/accounts_handler.gomanagement/server/http/handlers/accounts/accounts_handler_test.gomanagement/server/http/handlers/dns/dns_settings_handler.gomanagement/server/http/handlers/dns/dns_settings_handler_test.gomanagement/server/http/handlers/dns/nameservers_handler.gomanagement/server/http/handlers/dns/nameservers_handler_test.gomanagement/server/http/handlers/events/events_handler.gomanagement/server/http/handlers/events/events_handler_test.gomanagement/server/http/handlers/groups/groups_handler.gomanagement/server/http/handlers/groups/groups_handler_test.gomanagement/server/http/handlers/idp/idp_handler.gomanagement/server/http/handlers/idp/idp_handler_test.gomanagement/server/http/handlers/instance/instance_handler.gomanagement/server/http/handlers/instance/instance_handler_test.gomanagement/server/http/handlers/networks/handler.gomanagement/server/http/handlers/networks/resources_handler.gomanagement/server/http/handlers/networks/routers_handler.gomanagement/server/http/handlers/peers/peers_handler.gomanagement/server/http/handlers/peers/peers_handler_test.gomanagement/server/http/handlers/policies/geolocation_handler_test.gomanagement/server/http/handlers/policies/geolocations_handler.gomanagement/server/http/handlers/policies/policies_handler.gomanagement/server/http/handlers/policies/policies_handler_test.gomanagement/server/http/handlers/policies/posture_checks_handler.gomanagement/server/http/handlers/policies/posture_checks_handler_test.gomanagement/server/http/handlers/routes/routes_handler.gomanagement/server/http/handlers/routes/routes_handler_test.gomanagement/server/http/handlers/setup_keys/setupkeys_handler.gomanagement/server/http/handlers/setup_keys/setupkeys_handler_test.gomanagement/server/http/handlers/users/invites_handler.gomanagement/server/http/handlers/users/invites_handler_test.gomanagement/server/http/handlers/users/pat_handler.gomanagement/server/http/handlers/users/pat_handler_test.gomanagement/server/http/handlers/users/users_handler.gomanagement/server/http/handlers/users/users_handler_test.gomanagement/server/http/testing/integration/groups_handler_integration_test.gomanagement/server/http/testing/integration/invites_handler_integration_test.gomanagement/server/http/testing/integration/posture_checks_handler_integration_test.gomanagement/server/http/testing/integration/users_approve_reject_handler_integration_test.gomanagement/server/http/testing/testdata/posture_checks.sqlmanagement/server/http/testing/testdata/users_approve_reject.sqlmanagement/server/http/testing/testing_tools/channel/channel.gomanagement/server/http/testing/testing_tools/db_verify.gomanagement/server/identity_provider.gomanagement/server/identity_provider_test.gomanagement/server/instance/manager_test.gomanagement/server/management_proto_test.gomanagement/server/management_test.gomanagement/server/mock_server/account_mock.gomanagement/server/nameserver.gomanagement/server/nameserver_test.gomanagement/server/networks/manager.gomanagement/server/networks/manager_test.gomanagement/server/networks/resources/manager.gomanagement/server/networks/resources/manager_test.gomanagement/server/networks/routers/manager.gomanagement/server/networks/routers/manager_test.gomanagement/server/peer.gomanagement/server/peer_test.gomanagement/server/policy.gomanagement/server/posture_checks.gomanagement/server/posture_checks_test.gomanagement/server/route.gomanagement/server/route_test.gomanagement/server/settings/manager.gomanagement/server/setupkey.gomanagement/server/types/account_test.gomanagement/server/user.gomanagement/server/user_invite_test.gomanagement/server/user_test.gomanagement/server/users/user.goshared/management/client/client_test.go
💤 Files with no reviewable changes (3)
- management/server/event.go
- management/server/http/testing/integration/groups_handler_integration_test.go
- management/server/posture_checks_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (3)
management/internals/modules/permissions/manager.go (2)
79-87: Permission-denied log level is arguably too low.Denied permission checks are logged at
Tracef, which is typically disabled in production. Since this is the main signal that an authenticated user attempted an unauthorized action, considerDebugfat minimum (orInfofif you want an audit trail). This is the only place the denial is recorded before the error response is written —Tracefmakes real incidents effectively invisible.- log.WithContext(r.Context()).Tracef("user %s on account %s is not allowed to %s in %s", userAuth.UserId, userAuth.AccountId, operation, module) + log.WithContext(r.Context()).Debugf("user %s on account %s is not allowed to %s in %s", userAuth.UserId, userAuth.AccountId, operation, module)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/modules/permissions/manager.go` around lines 79 - 87, Change the permission-denied logging from Tracef to a higher level so denials are visible in typical production logs: locate the block in manager.go where userAuth, operation and module are used (inside the permission check that constructs permErr and calls onAuthErr and util.WriteError) and replace log.WithContext(...).Tracef(...) with log.WithContext(...).Debugf(...) (or Infof if you want an auditable record) so unauthorized attempts are recorded at a visible log level before WriteError is called.
185-197:WrapHandlersilently bypasses permission checks — document the intended use.
WrapHandleronly extractsUserAuth; it performs no authorization. It's easy to grab this helper by mistake (especially given its position next toWithPermission) and end up registering a route that authenticates but never authorizes. Consider tightening the doc comment to explicitly state it must only be used for endpoints whose authorization is enforced elsewhere (e.g., same-user self-serve routes, or routes guarded by upstream middleware), and/or renaming to something likeWrapHandlerWithoutPermissions/InjectUserAuthto make misuse visually obvious at call sites.📝 Suggested doc tweak
-// WrapHandler wraps a handler that expects UserAuth with context extraction. -// Unlike WithPermission, it does not perform any permission checks. +// WrapHandler injects *auth.UserAuth (from the request context) into a handler +// without performing any permission checks. It must only be used for endpoints +// whose authorization is enforced by other means (e.g., the handler itself +// validates ownership, or an upstream middleware gates access). For any +// role/module-gated endpoint use WithPermission instead. func WrapHandler(h func(w http.ResponseWriter, r *http.Request, userAuth *auth.UserAuth)) http.HandlerFunc {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/modules/permissions/manager.go` around lines 185 - 197, WrapHandler currently only extracts UserAuth and performs no authorization, which can be mistaken for WithPermission; update the doc comment for WrapHandler to explicitly state it does NOT perform permission checks and must only be used when authorization is enforced elsewhere (e.g., same-user self-serve routes or upstream middleware). Optionally rename the function to a clearer identifier such as InjectUserAuth or WrapHandlerWithoutPermissions and update all call sites to use the new name to make misuse obvious; ensure references to WithPermission remain unchanged so reviewers can compare intended behavior.management/server/http/handlers/peers/peers_handler_test.go (1)
16-18: Two mock libraries in the same test file — consider unifying.This file imports both
github.com/golang/mock/gomock(asgomock) andgo.uber.org/mock/gomock(asugomock), creating two separate controllers (ctrlviaugomock,ctrl2viagomock) becausenetwork_map.NewMockControllerandpermissions.NewMockManagerwere generated against different mock packages.github.com/golang/mockis archived;go.uber.org/mockis its active successor. The two APIs are source-compatible, but this pattern is fragile and confusing. Consider regeneratingpermissions.NewMockManagerwithgo.uber.org/mock/mockgen(updating the//go:generatedirective inmanagement/internals/modules/permissions/manager.go) so the codebase uses one mock library and one controller per test. This would require updates to several test files that importpermissions.NewMockManager.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/http/handlers/peers/peers_handler_test.go` around lines 16 - 18, This test imports two mock packages causing two controllers (ctrl via go.uber.org/mock/gomock and ctrl2 via github.com/golang/mock/gomock) because network_map.NewMockController and permissions.NewMockManager were generated with different mock generators; regenerate permissions.NewMockManager using go.uber.org/mock/mockgen (update the //go:generate in management/internals/modules/permissions/manager.go) so both mocks come from go.uber.org/mock, then update this test to import only go.uber.org/mock/gomock (remove github.com/golang/mock/gomock), use a single controller (e.g., ctrl from ugomock.NewController) to create both network_map.NewMockController and permissions.NewMockManager, and adjust any other tests that reference permissions.NewMockManager to the new mock package.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@management/internals/modules/permissions/manager.go`:
- Around line 79-87: Change the permission-denied logging from Tracef to a
higher level so denials are visible in typical production logs: locate the block
in manager.go where userAuth, operation and module are used (inside the
permission check that constructs permErr and calls onAuthErr and
util.WriteError) and replace log.WithContext(...).Tracef(...) with
log.WithContext(...).Debugf(...) (or Infof if you want an auditable record) so
unauthorized attempts are recorded at a visible log level before WriteError is
called.
- Around line 185-197: WrapHandler currently only extracts UserAuth and performs
no authorization, which can be mistaken for WithPermission; update the doc
comment for WrapHandler to explicitly state it does NOT perform permission
checks and must only be used when authorization is enforced elsewhere (e.g.,
same-user self-serve routes or upstream middleware). Optionally rename the
function to a clearer identifier such as InjectUserAuth or
WrapHandlerWithoutPermissions and update all call sites to use the new name to
make misuse obvious; ensure references to WithPermission remain unchanged so
reviewers can compare intended behavior.
In `@management/server/http/handlers/peers/peers_handler_test.go`:
- Around line 16-18: This test imports two mock packages causing two controllers
(ctrl via go.uber.org/mock/gomock and ctrl2 via github.com/golang/mock/gomock)
because network_map.NewMockController and permissions.NewMockManager were
generated with different mock generators; regenerate permissions.NewMockManager
using go.uber.org/mock/mockgen (update the //go:generate in
management/internals/modules/permissions/manager.go) so both mocks come from
go.uber.org/mock, then update this test to import only go.uber.org/mock/gomock
(remove github.com/golang/mock/gomock), use a single controller (e.g., ctrl from
ugomock.NewController) to create both network_map.NewMockController and
permissions.NewMockManager, and adjust any other tests that reference
permissions.NewMockManager to the new mock package.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7bf8fde9-915f-46b7-a73f-8aff966d2f08
📒 Files selected for processing (30)
management/internals/modules/permissions/manager.gomanagement/internals/modules/reverseproxy/domain/manager/api.gomanagement/server/http/handlers/accounts/accounts_handler_test.gomanagement/server/http/handlers/dns/dns_settings_handler_test.gomanagement/server/http/handlers/dns/nameservers_handler_test.gomanagement/server/http/handlers/events/events_handler_test.gomanagement/server/http/handlers/groups/groups_handler_test.gomanagement/server/http/handlers/idp/idp_handler_test.gomanagement/server/http/handlers/peers/peers_handler_test.gomanagement/server/http/handlers/policies/geolocation_handler_test.gomanagement/server/http/handlers/policies/policies_handler_test.gomanagement/server/http/handlers/policies/posture_checks_handler_test.gomanagement/server/http/handlers/routes/routes_handler_test.gomanagement/server/http/handlers/setup_keys/setupkeys_handler_test.gomanagement/server/http/handlers/users/pat_handler_test.gomanagement/server/http/handlers/users/users_handler_test.gomanagement/server/http/testing/testdata/accounts.sqlmanagement/server/http/testing/testdata/dns.sqlmanagement/server/http/testing/testdata/events.sqlmanagement/server/http/testing/testdata/groups.sqlmanagement/server/http/testing/testdata/networks.sqlmanagement/server/http/testing/testdata/peers.sqlmanagement/server/http/testing/testdata/peers_integration.sqlmanagement/server/http/testing/testdata/policies.sqlmanagement/server/http/testing/testdata/posture_checks.sqlmanagement/server/http/testing/testdata/routes.sqlmanagement/server/http/testing/testdata/setup_keys.sqlmanagement/server/http/testing/testdata/users.sqlmanagement/server/http/testing/testdata/users_approve_reject.sqlmanagement/server/http/testing/testdata/users_integration.sql
✅ Files skipped from review due to trivial changes (2)
- management/server/http/handlers/routes/routes_handler_test.go
- management/server/http/handlers/policies/posture_checks_handler_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- management/server/http/handlers/events/events_handler_test.go
- management/server/http/handlers/policies/policies_handler_test.go
- management/server/http/handlers/idp/idp_handler_test.go
- management/server/http/handlers/dns/nameservers_handler_test.go
- management/server/http/handlers/setup_keys/setupkeys_handler_test.go
- management/server/http/handlers/policies/geolocation_handler_test.go
- management/server/http/handlers/dns/dns_settings_handler_test.go
- management/server/http/handlers/accounts/accounts_handler_test.go
- management/server/http/testing/testdata/posture_checks.sql
- management/server/http/testing/testdata/users_approve_reject.sql
- management/server/http/handlers/users/users_handler_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@management/server/http/handlers/groups/groups_handler.go`:
- Around line 45-47: The current canReadPeers suppresses ValidateUserPermissions
errors by returning false; change its signature to return (bool, error) and
propagate the validation error instead of converting it to false (i.e., have
canReadPeers call h.permissionsManager.ValidateUserPermissions and return
(allowed, err)). Update all call sites (handlers that call canReadPeers before
calling GetPeers and the createGroup/updateGroup handlers) to handle the error:
call the new canReadPeers before invoking GetPeers (and for
createGroup/updateGroup call it before CreateGroup/UpdateGroup) and
return/handle the error rather than proceeding with a silently downgraded
permission result. Ensure references: canReadPeers, ValidateUserPermissions,
GetPeers, createGroup, updateGroup, and permissionsManager are updated
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 77d31e63-9bc9-4ef9-8d32-796c2e31a892
📒 Files selected for processing (2)
management/server/http/handlers/groups/groups_handler.gomanagement/server/http/handlers/groups/groups_handler_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- management/server/http/handlers/groups/groups_handler_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
management/server/posture_checks.go (1)
20-31:⚠️ Potential issue | 🔴 CriticalBind create/update behavior to the authorized route operation.
createis still ignored, so the request body ID decides whether this executes the update path. With posture-check permissions now enforced by route middleware, a caller authorized for create can still update an existing posture check by sendingpostureChecks.ID.🛡️ Proposed fix
func (am *DefaultAccountManager) SavePostureChecks(ctx context.Context, accountID, userID string, postureChecks *posture.Checks, create bool) (*posture.Checks, error) { var updateAccountPeers bool var err error - var isUpdate = postureChecks.ID != "" + if create && postureChecks.ID != "" { + return nil, status.Errorf(status.InvalidArgument, "posture check ID must be empty when creating a posture check") + } + if !create && postureChecks.ID == "" { + return nil, status.Errorf(status.InvalidArgument, "posture check ID is required when updating a posture check") + } + + var isUpdate = !create var action = activity.PostureCheckCreated🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/posture_checks.go` around lines 20 - 31, SavePostureChecks currently derives update vs create from postureChecks.ID (isUpdate) and ignores the create parameter; change the flow so the create boolean controls the operation: inside SavePostureChecks use create to decide create vs update (e.g., treat isUpdate = !create), validate that when create==true postureChecks.ID must be empty (return an error if not), and when create==false postureChecks.ID must be present (return an error if missing); ensure the activity action variable (currently activity.PostureCheckCreated) is set appropriately for updates vs creates and that existing validation calls like validatePostureChecks are executed in both paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@management/server/posture_checks.go`:
- Around line 20-31: SavePostureChecks currently derives update vs create from
postureChecks.ID (isUpdate) and ignores the create parameter; change the flow so
the create boolean controls the operation: inside SavePostureChecks use create
to decide create vs update (e.g., treat isUpdate = !create), validate that when
create==true postureChecks.ID must be empty (return an error if not), and when
create==false postureChecks.ID must be present (return an error if missing);
ensure the activity action variable (currently activity.PostureCheckCreated) is
set appropriately for updates vs creates and that existing validation calls like
validatePostureChecks are executed in both paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9190b258-62a2-489c-81d1-a72126229471
📒 Files selected for processing (2)
management/server/policy.gomanagement/server/posture_checks.go
🚧 Files skipped from review as they are similar to previous changes (1)
- management/server/policy.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
management/server/account.go (1)
1343-1360:⚠️ Potential issue | 🟠 MajorValidate the loaded user belongs to the resolved account.
After removing the manager-level permission check, the
IsChildpath can returnuserAuth.AccountIdafter only checking the account exists.GetUserByUserIDproves the user exists, but not thatuser.AccountID == accountID; add that guard before theIsChildearly return.Suggested fix
user, err := am.Store.GetUserByUserID(ctx, store.LockingStrengthNone, userAuth.UserId) if err != nil { // this is not really possible because we got an account by user ID log.Errorf("failed to get user by ID %s: %v", userAuth.UserId, err) return "", "", status.Errorf(status.NotFound, "user %s not found", userAuth.UserId) } + if user.AccountID != accountID { + return "", "", status.Errorf(status.PermissionDenied, "user %s is not part of account %s", user.Id, accountID) + } + if userAuth.IsChild { return accountID, user.Id, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/account.go` around lines 1343 - 1360, The IsChild early-return path in the logic that calls getAccountIDWithAuthorizationClaims and Store.GetUserByUserID must verify the loaded user actually belongs to the resolved account; update the code around getAccountIDWithAuthorizationClaims, Store.GetUserByUserID and the IsChild branch to check that user.AccountID (or the field name used on the returned user) equals accountID and if not return an appropriate NotFound or Permission error instead of returning accountID and user.Id; ensure the check occurs before the early return and use the same status.Errorf pattern already used for missing users.management/server/user_test.go (1)
1915-2000: 🛠️ Refactor suggestion | 🟠 MajorHTTP handler is the only entrypoint; confirm integration tests cover permission rejection.
Verification confirms
ApproveUserandRejectUserare called exclusively frommanagement/server/http/handlers/users/users_handler.go. However, the manager implementations contain no permission checks—they only validate user existence and pending approval status. Permission gating is enforced by the HTTP middleware (permissionsManager.WithPermission).The integration tests (
TestApproveUserEndpoint,TestRejectUserEndpoint) only verify admin success cases and do not test permission rejection for non-admin callers. If a non-HTTP code path (gRPC, background job) calls the manager in the future, there is no permission guard.Action: Either confirm this architecture is intentional and add an integration test case verifying non-admin rejection at the HTTP level, or add a manager-level unit test asserting non-admin users cannot approve/reject.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/user_test.go` around lines 1915 - 2000, The manager methods ApproveUser and RejectUser lack permission checks; add a test to cover permission rejection: either add an HTTP-level integration test (alongside TestApproveUserEndpoint/TestRejectUserEndpoint) that issues the approve/reject requests authenticated as a non-admin user and asserts a 403/permission-denied response, or add unit tests that call manager.ApproveUser and manager.RejectUser using a non-admin caller and assert they return a permission error; reference the handler wrapper permissionsManager.WithPermission to ensure the test verifies the enforcement point you choose.
♻️ Duplicate comments (1)
management/server/http/handlers/users/users_handler.go (1)
30-38:⚠️ Potential issue | 🟠 MajorSelf-service endpoints still need permission-denied fallbacks.
/users/currentand/users/{userId}/passwordnow requiremodules.Userspermissions before reaching handlers. Regular users without those permissions can be blocked from viewing their own profile or changing their own password; mirror the/usersfallback pattern for these self-service routes.Suggested shape
- router.HandleFunc("/users/current", permissionsManager.WithPermission(modules.Users, operations.Read, userHandler.getCurrentUser)).Methods("GET", "OPTIONS") + router.HandleFunc("/users/current", permissionsManager.WithPermission(modules.Users, operations.Read, userHandler.getCurrentUser, userHandler.getCurrentUserFallback)).Methods("GET", "OPTIONS") ... - router.HandleFunc("/users/{userId}/password", permissionsManager.WithPermission(modules.Users, operations.Update, userHandler.changePassword)).Methods("PUT", "OPTIONS") + router.HandleFunc("/users/{userId}/password", permissionsManager.WithPermission(modules.Users, operations.Update, userHandler.changePassword, userHandler.changeOwnPassword)).Methods("PUT", "OPTIONS")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/http/handlers/users/users_handler.go` around lines 30 - 38, The /users/current and /users/{userId}/password routes currently require modules.Users permission and lack the self-service fallback used by the /users route; update the router registrations to use the same WithPermission dual-handler pattern so that users without global Users permission can still access their own data: for "/users/current" call permissionsManager.WithPermission(modules.Users, operations.Read, userHandler.getCurrentUser, userHandler.getOwnUser) and for "/users/{userId}/password" call permissionsManager.WithPermission(modules.Users, operations.Update, userHandler.changePassword, userHandler.changeOwnPassword) (or implement changeOwnPassword if missing) so the second handler acts as the self-service fallback.
🧹 Nitpick comments (4)
management/server/http/testing/testing_tools/tools.go (1)
108-114: Consider also rejecting other 2xx success codes in negative-path assertions.The new guard only fails on
200and201, but a handler that reaches success via202 Acceptedor204 No Contentwould still silently pass in the!expectResponsebranch — which defeats the intent of catching "handler reached and succeeded" when permission middleware should have short-circuited the request. If negative tests never expect any 2xx, a broader check (recorder.Code >= 200 && recorder.Code < 300) would be more robust.Proposed tightening
- if recorder.Code == http.StatusOK || recorder.Code == http.StatusCreated { - t.Fatalf("expected unauthorized/error status code but got %d, content: %s", - recorder.Code, string(content)) - } + if recorder.Code >= http.StatusOK && recorder.Code < http.StatusMultipleChoices { + t.Fatalf("expected unauthorized/error status code but got %d, content: %s", + recorder.Code, string(content)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/http/testing/testing_tools/tools.go` around lines 108 - 114, The negative-path check in testing_tools (using expectResponse and recorder.Code) only rejects 200 and 201; update the logic that currently checks "recorder.Code == http.StatusOK || recorder.Code == http.StatusCreated" to instead treat any 2xx as a success (i.e., check recorder.Code >= 200 && recorder.Code < 300) so tests that should be blocked by middleware also fail when handlers return 202, 204, etc.; adjust the branch that returns nil, false accordingly to use this broader 2xx guard.management/server/http/handlers/accounts/accounts_handler.go (1)
295-309: Nit: UseaccountIDinDeleteAccountcall for consistency.After the mismatch check on line 297,
accountIDanduserAuth.AccountIdare equivalent, butupdateAccountuses the localaccountIDvariable for downstream calls whiledeleteAccountusesuserAuth.AccountId. Aligning them makes the code consistent and slightly more defensive against future edits that might shift the mismatch check.♻️ Proposed change
- err := h.accountManager.DeleteAccount(r.Context(), userAuth.AccountId, userAuth.UserId) + err := h.accountManager.DeleteAccount(r.Context(), accountID, userAuth.UserId)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/http/handlers/accounts/accounts_handler.go` around lines 295 - 309, In deleteAccount, use the local accountID variable when calling h.accountManager.DeleteAccount instead of userAuth.AccountId to match the mismatch check and the pattern used in updateAccount; update the call in the deleteAccount function to pass accountID and keep userAuth.UserId as the second arg so the call reads DeleteAccount(ctx, accountID, userAuth.UserId).management/server/http/testing/integration/users_handler_integration_test.go (1)
640-670: Tighten cross-account assertions to a specific expected status.
assert.NotEqual(t, http.StatusOK, ...)also passes on a 500/panic-recovered response, which would hide a regression where the handler crashes instead of properly denying the request. Prefer asserting the concrete expected status (e.g.,http.StatusForbidden/http.StatusNotFound) so the test fails loudly if the denial path changes to an internal error.♻️ Example
- assert.NotEqual(t, http.StatusOK, recorder.Code, "cross-account user update must be rejected") + assert.Equal(t, http.StatusForbidden, recorder.Code, "cross-account user update must be rejected with 403")Same applies to
Test_Users_Delete_CrossAccountAttackon line 668.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/http/testing/integration/users_handler_integration_test.go` around lines 640 - 670, The tests Test_Users_Update_CrossAccountAttack and Test_Users_Delete_CrossAccountAttack currently use assert.NotEqual(t, http.StatusOK, recorder.Code) which masks 500/internal errors; change these to assert.Equal with the concrete expected denial status (e.g., assert.Equal(t, http.StatusForbidden, recorder.Code, "cross-account user update must be rejected") and similarly for the delete test) so the test fails loudly if the handler returns 500 instead of the intended forbidden/not-found response.management/server/http/testing/integration/accounts_handler_integration_test.go (1)
259-299:Test_Accounts_Deleteshould verify DB side-effect; cross-account tests should assert specific status.Two related test-quality issues:
Test_Accounts_Deleteonly asserts the response code viaReadResponseand never checks that the owner case actually removed the account from the DB (unlikeTest_Users_Deletewhich usesVerifyUserNotInDB). For a destructive operation, missing the DB verification means a silent regression (e.g., handler returning 200 without deleting) would not be caught.
Test_Accounts_Delete_CrossAccountAttack(andTest_Accounts_Update_CrossAccountAttackat lines 236–257) useassert.NotEqual(t, http.StatusOK, ...), which also passes for 500/panic-recovered responses. Prefer asserting the concrete expected status (e.g.,http.StatusForbidden) so a regression that turns the permission denial into an internal error is caught.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/http/testing/integration/accounts_handler_integration_test.go` around lines 259 - 299, Test_Accounts_Delete currently only checks HTTP response but not DB side-effects and the cross-account tests use NotEqual which can hide 500s; update Test_Accounts_Delete so that when the "Owner user" case returns success you also verify the account was removed from the DB (call the existing helper pattern used in Test_Users_Delete — e.g., testing_tools.VerifyAccountNotInDB(t, db/conn, testing_tools.TestAccountId) or add a similar VerifyAccountNotInDB helper if missing) and change Test_Accounts_Delete_CrossAccountAttack (and Test_Accounts_Update_CrossAccountAttack) to assert the concrete expected status (e.g., assert.Equal(t, http.StatusForbidden, recorder.Code)) instead of assert.NotEqual to ensure permission failures, not internal errors, are enforced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@management/server/account.go`:
- Around line 1343-1360: The IsChild early-return path in the logic that calls
getAccountIDWithAuthorizationClaims and Store.GetUserByUserID must verify the
loaded user actually belongs to the resolved account; update the code around
getAccountIDWithAuthorizationClaims, Store.GetUserByUserID and the IsChild
branch to check that user.AccountID (or the field name used on the returned
user) equals accountID and if not return an appropriate NotFound or Permission
error instead of returning accountID and user.Id; ensure the check occurs before
the early return and use the same status.Errorf pattern already used for missing
users.
In `@management/server/user_test.go`:
- Around line 1915-2000: The manager methods ApproveUser and RejectUser lack
permission checks; add a test to cover permission rejection: either add an
HTTP-level integration test (alongside
TestApproveUserEndpoint/TestRejectUserEndpoint) that issues the approve/reject
requests authenticated as a non-admin user and asserts a 403/permission-denied
response, or add unit tests that call manager.ApproveUser and manager.RejectUser
using a non-admin caller and assert they return a permission error; reference
the handler wrapper permissionsManager.WithPermission to ensure the test
verifies the enforcement point you choose.
---
Duplicate comments:
In `@management/server/http/handlers/users/users_handler.go`:
- Around line 30-38: The /users/current and /users/{userId}/password routes
currently require modules.Users permission and lack the self-service fallback
used by the /users route; update the router registrations to use the same
WithPermission dual-handler pattern so that users without global Users
permission can still access their own data: for "/users/current" call
permissionsManager.WithPermission(modules.Users, operations.Read,
userHandler.getCurrentUser, userHandler.getOwnUser) and for
"/users/{userId}/password" call permissionsManager.WithPermission(modules.Users,
operations.Update, userHandler.changePassword, userHandler.changeOwnPassword)
(or implement changeOwnPassword if missing) so the second handler acts as the
self-service fallback.
---
Nitpick comments:
In `@management/server/http/handlers/accounts/accounts_handler.go`:
- Around line 295-309: In deleteAccount, use the local accountID variable when
calling h.accountManager.DeleteAccount instead of userAuth.AccountId to match
the mismatch check and the pattern used in updateAccount; update the call in the
deleteAccount function to pass accountID and keep userAuth.UserId as the second
arg so the call reads DeleteAccount(ctx, accountID, userAuth.UserId).
In
`@management/server/http/testing/integration/accounts_handler_integration_test.go`:
- Around line 259-299: Test_Accounts_Delete currently only checks HTTP response
but not DB side-effects and the cross-account tests use NotEqual which can hide
500s; update Test_Accounts_Delete so that when the "Owner user" case returns
success you also verify the account was removed from the DB (call the existing
helper pattern used in Test_Users_Delete — e.g.,
testing_tools.VerifyAccountNotInDB(t, db/conn, testing_tools.TestAccountId) or
add a similar VerifyAccountNotInDB helper if missing) and change
Test_Accounts_Delete_CrossAccountAttack (and
Test_Accounts_Update_CrossAccountAttack) to assert the concrete expected status
(e.g., assert.Equal(t, http.StatusForbidden, recorder.Code)) instead of
assert.NotEqual to ensure permission failures, not internal errors, are
enforced.
In
`@management/server/http/testing/integration/users_handler_integration_test.go`:
- Around line 640-670: The tests Test_Users_Update_CrossAccountAttack and
Test_Users_Delete_CrossAccountAttack currently use assert.NotEqual(t,
http.StatusOK, recorder.Code) which masks 500/internal errors; change these to
assert.Equal with the concrete expected denial status (e.g., assert.Equal(t,
http.StatusForbidden, recorder.Code, "cross-account user update must be
rejected") and similarly for the delete test) so the test fails loudly if the
handler returns 500 instead of the intended forbidden/not-found response.
In `@management/server/http/testing/testing_tools/tools.go`:
- Around line 108-114: The negative-path check in testing_tools (using
expectResponse and recorder.Code) only rejects 200 and 201; update the logic
that currently checks "recorder.Code == http.StatusOK || recorder.Code ==
http.StatusCreated" to instead treat any 2xx as a success (i.e., check
recorder.Code >= 200 && recorder.Code < 300) so tests that should be blocked by
middleware also fail when handlers return 202, 204, etc.; adjust the branch that
returns nil, false accordingly to use this broader 2xx guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a34ccc88-36ed-4e22-be79-3a8f99ebf09c
📒 Files selected for processing (13)
management/server/account.gomanagement/server/http/handlers/accounts/accounts_handler.gomanagement/server/http/handlers/users/users_handler.gomanagement/server/http/testing/integration/accounts_handler_integration_test.gomanagement/server/http/testing/integration/users_handler_integration_test.gomanagement/server/http/testing/testdata/users_integration.sqlmanagement/server/http/testing/testing_tools/tools.gomanagement/server/store/sql_store.gomanagement/server/store/sql_store_user_invite_test.gomanagement/server/store/store.gomanagement/server/store/store_mock.gomanagement/server/user.gomanagement/server/user_test.go
✅ Files skipped from review due to trivial changes (1)
- management/server/http/testing/testdata/users_integration.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- management/server/user.go
# Conflicts: # management/server/http/testing/testing_tools/channel/channel.go # management/server/policy.go
|
1 participant
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
Refactor
Behavior
Tests