From 3fe19eb86d78b7b3c73d43fc03fd811085b13bf7 Mon Sep 17 00:00:00 2001 From: pascal Date: Mon, 21 Sep 2026 16:44:39 +0200 Subject: [PATCH] merge conflict and onDenied handler --- .../internals/modules/permissions/manager.go | 45 ++++--- .../modules/permissions/manager_mock.go | 8 +- .../modules/reverseproxy/service/interface.go | 1 - .../reverseproxy/service/interface_mock.go | 14 --- .../reverseproxy/service/manager/api.go | 6 +- .../reverseproxy/service/manager/manager.go | 37 ------ .../service/manager/manager_test.go | 24 ---- .../shared/grpc/proxy_group_access_test.go | 4 - .../shared/grpc/validate_session_test.go | 4 - management/server/account.go | 2 +- management/server/http/handler.go | 2 +- .../handlers/instance/instance_handler.go | 6 +- .../instance/instance_handler_test.go | 24 +--- .../http/handlers/peers/peers_handler.go | 113 ++++++++++++------ .../http/handlers/peers/peers_handler_test.go | 2 +- .../handlers/policies/geolocations_handler.go | 6 +- .../proxy/auth_callback_integration_test.go | 4 - .../http/handlers/users/users_handler.go | 24 +--- .../peers_handler_integration_test.go | 50 ++++++-- .../users_handler_integration_test.go | 44 +++++++ .../server/http/testing/testdata/peers.sql | 1 + .../testing/testdata/peers_integration.sql | 1 + .../testing/testdata/users_integration.sql | 1 + .../testing/testing_tools/channel/channel.go | 2 +- .../http/testing/testing_tools/tools.go | 1 + management/server/identity_provider.go | 16 +-- management/server/peer.go | 23 +--- management/server/peer_test.go | 17 +-- management/server/user.go | 19 +-- management/server/user_test.go | 4 +- proxy/management_integration_test.go | 4 - 31 files changed, 252 insertions(+), 257 deletions(-) diff --git a/management/internals/modules/permissions/manager.go b/management/internals/modules/permissions/manager.go index c3fd3ffbb..c6eb2a266 100644 --- a/management/internals/modules/permissions/manager.go +++ b/management/internals/modules/permissions/manager.go @@ -21,12 +21,13 @@ import ( "github.com/netbirdio/netbird/shared/management/status" ) -// AuthErrorHandler is called when an auth error occurs during permission validation. -// If it returns true, the error is considered handled and the default error response is skipped. -type AuthErrorHandler func(w http.ResponseWriter, r *http.Request, userAuth *auth.UserAuth, err error) bool +// PermissionDeniedHandler is called when the user's role does not grant the requested operation. +// It is not called for validation failures such as blocked users or foreign accounts. +// If it returns true, the request is considered handled and the default 403 response is skipped. +type PermissionDeniedHandler func(w http.ResponseWriter, r *http.Request, userAuth *auth.UserAuth) bool type Manager interface { - WithPermission(module modules.Module, operation operations.Operation, handlerFunc func(w http.ResponseWriter, r *http.Request, auth *auth.UserAuth), authErrHandler ...AuthErrorHandler) http.HandlerFunc + WithPermission(module modules.Module, operation operations.Operation, handlerFunc func(w http.ResponseWriter, r *http.Request, auth *auth.UserAuth), onDenied ...PermissionDeniedHandler) http.HandlerFunc ValidateUserPermissions(ctx context.Context, accountID, userID string, module modules.Module, operation operations.Operation) (bool, context.Context, error) ValidateRoleModuleAccess(ctx context.Context, accountID string, role roles.RolePermissions, module modules.Module, operation operations.Operation) bool ValidateAccountAccess(ctx context.Context, accountID string, user *types.User, allowOwnerAndAdmin bool) (context.Context, error) @@ -45,18 +46,30 @@ func NewManager(store store.Store) Manager { } } -// WithPermission wraps an HTTP handler with permission checking logic. -// An optional AuthErrorHandler can be provided to intercept auth errors before the default response is written. -// The wrapped handler receives a request whose context is enriched by the permission validation. func (m *managerImpl) WithPermission( module modules.Module, operation operations.Operation, handlerFunc func(w http.ResponseWriter, r *http.Request, auth *auth.UserAuth), - authErrHandler ...AuthErrorHandler, + onDenied ...PermissionDeniedHandler, ) http.HandlerFunc { - var onAuthErr AuthErrorHandler - if len(authErrHandler) > 0 { - onAuthErr = authErrHandler[0] + return WithPermission(m, module, operation, handlerFunc, onDenied...) +} + +// WithPermission wraps an HTTP handler with permission checking performed by the given manager. +// Implementations embedding another Manager must route their own WithPermission through this +// function so that their ValidateUserPermissions override is the one consulted. +// An optional PermissionDeniedHandler can serve a reduced, self-scoped response when the role denies the operation. +// The wrapped handler receives a request whose context is enriched by the permission validation. +func WithPermission( + m Manager, + module modules.Module, + operation operations.Operation, + handlerFunc func(w http.ResponseWriter, r *http.Request, auth *auth.UserAuth), + onDenied ...PermissionDeniedHandler, +) http.HandlerFunc { + var deniedHandler PermissionDeniedHandler + if len(onDenied) > 0 { + deniedHandler = onDenied[0] } return func(w http.ResponseWriter, r *http.Request) { @@ -68,23 +81,19 @@ func (m *managerImpl) WithPermission( } allowed, ctx, err := m.ValidateUserPermissions(r.Context(), userAuth.AccountId, userAuth.UserId, module, operation) - enriched := r.WithContext(ctx) if err != nil { - if onAuthErr != nil && onAuthErr(w, enriched, &userAuth, err) { - return - } log.WithContext(ctx).Errorf("failed to validate permissions for user %s on account %s: %v", userAuth.UserId, userAuth.AccountId, err) util.WriteError(ctx, status.NewPermissionValidationError(err), w) return } + enriched := r.WithContext(ctx) if !allowed { - permErr := status.NewPermissionDeniedError() - if onAuthErr != nil && onAuthErr(w, enriched, &userAuth, permErr) { + if deniedHandler != nil && deniedHandler(w, enriched, &userAuth) { return } log.WithContext(ctx).Tracef("user %s on account %s is not allowed to %s in %s", userAuth.UserId, userAuth.AccountId, operation, module) - util.WriteError(ctx, permErr, w) + util.WriteError(ctx, status.NewPermissionDeniedError(), w) return } diff --git a/management/internals/modules/permissions/manager_mock.go b/management/internals/modules/permissions/manager_mock.go index c41f95bda..e6d456629 100644 --- a/management/internals/modules/permissions/manager_mock.go +++ b/management/internals/modules/permissions/manager_mock.go @@ -120,10 +120,10 @@ func (mr *MockManagerMockRecorder) ValidateUserPermissions(ctx, accountID, userI } // WithPermission mocks base method. -func (m *MockManager) WithPermission(module modules.Module, operation operations.Operation, handlerFunc func(http.ResponseWriter, *http.Request, *auth.UserAuth), authErrHandler ...AuthErrorHandler) http.HandlerFunc { +func (m *MockManager) WithPermission(module modules.Module, operation operations.Operation, handlerFunc func(http.ResponseWriter, *http.Request, *auth.UserAuth), onDenied ...PermissionDeniedHandler) http.HandlerFunc { m.ctrl.T.Helper() varargs := []any{module, operation, handlerFunc} - for _, a := range authErrHandler { + for _, a := range onDenied { varargs = append(varargs, a) } ret := m.ctrl.Call(m, "WithPermission", varargs...) @@ -132,8 +132,8 @@ func (m *MockManager) WithPermission(module modules.Module, operation operations } // WithPermission indicates an expected call of WithPermission. -func (mr *MockManagerMockRecorder) WithPermission(module, operation, handlerFunc any, authErrHandler ...any) *gomock.Call { +func (mr *MockManagerMockRecorder) WithPermission(module, operation, handlerFunc any, onDenied ...any) *gomock.Call { mr.mock.ctrl.T.Helper() - varargs := append([]any{module, operation, handlerFunc}, authErrHandler...) + varargs := append([]any{module, operation, handlerFunc}, onDenied...) return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WithPermission", reflect.TypeOf((*MockManager)(nil).WithPermission), varargs...) } diff --git a/management/internals/modules/reverseproxy/service/interface.go b/management/internals/modules/reverseproxy/service/interface.go index 10d93294a..4c5555130 100644 --- a/management/internals/modules/reverseproxy/service/interface.go +++ b/management/internals/modules/reverseproxy/service/interface.go @@ -16,7 +16,6 @@ type Manager interface { CreateService(ctx context.Context, accountID, userID string, service *Service) (*Service, error) UpdateService(ctx context.Context, accountID, userID string, service *Service) (*Service, error) DeleteService(ctx context.Context, accountID, userID, serviceID string) error - DeleteAllServices(ctx context.Context, accountID, userID string) error SetCertificateIssuedAt(ctx context.Context, accountID, serviceID string) error SetStatus(ctx context.Context, accountID, serviceID string, status Status) error ReloadAllServicesForAccount(ctx context.Context, accountID string) error diff --git a/management/internals/modules/reverseproxy/service/interface_mock.go b/management/internals/modules/reverseproxy/service/interface_mock.go index 6b60f2af1..6f602c91d 100644 --- a/management/internals/modules/reverseproxy/service/interface_mock.go +++ b/management/internals/modules/reverseproxy/service/interface_mock.go @@ -85,20 +85,6 @@ func (mr *MockManagerMockRecorder) DeleteAccountCluster(ctx, accountID, userID, return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteAccountCluster", reflect.TypeOf((*MockManager)(nil).DeleteAccountCluster), ctx, accountID, userID, clusterAddress) } -// DeleteAllServices mocks base method. -func (m *MockManager) DeleteAllServices(ctx context.Context, accountID, userID string) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DeleteAllServices", ctx, accountID, userID) - ret0, _ := ret[0].(error) - return ret0 -} - -// DeleteAllServices indicates an expected call of DeleteAllServices. -func (mr *MockManagerMockRecorder) DeleteAllServices(ctx, accountID, userID any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteAllServices", reflect.TypeOf((*MockManager)(nil).DeleteAllServices), ctx, accountID, userID) -} - // DeleteService mocks base method. func (m *MockManager) DeleteService(ctx context.Context, accountID, userID, serviceID string) error { m.ctrl.T.Helper() diff --git a/management/internals/modules/reverseproxy/service/manager/api.go b/management/internals/modules/reverseproxy/service/manager/api.go index a47cb9be4..953527ddf 100644 --- a/management/internals/modules/reverseproxy/service/manager/api.go +++ b/management/internals/modules/reverseproxy/service/manager/api.go @@ -20,15 +20,13 @@ import ( ) type handler struct { - manager rpservice.Manager - permissionsManager permissions.Manager + manager rpservice.Manager } // RegisterEndpoints registers all service HTTP endpoints. func RegisterEndpoints(manager rpservice.Manager, domainManager domainmanager.Manager, accessLogsManager accesslogs.Manager, permissionsManager permissions.Manager, router *mux.Router) { h := &handler{ - manager: manager, - permissionsManager: permissionsManager, + manager: manager, } domainRouter := router.PathPrefix("/reverse-proxies").Subrouter() diff --git a/management/internals/modules/reverseproxy/service/manager/manager.go b/management/internals/modules/reverseproxy/service/manager/manager.go index fa138edc9..8702b1e1e 100644 --- a/management/internals/modules/reverseproxy/service/manager/manager.go +++ b/management/internals/modules/reverseproxy/service/manager/manager.go @@ -842,43 +842,6 @@ func (m *Manager) DeleteService(ctx context.Context, accountID, userID, serviceI return nil } -func (m *Manager) DeleteAllServices(ctx context.Context, accountID, userID string) error { - var services []*service.Service - err := m.store.ExecuteInTransaction(ctx, func(transaction store.Store) error { - var err error - services, err = transaction.GetAccountServices(ctx, store.LockingStrengthUpdate, accountID) - if err != nil { - return err - } - - 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) - } - } - - return nil - }) - if err != nil { - return err - } - - oidcCfg := m.proxyController.GetOIDCValidationConfig() - - for _, svc := range services { - m.accountManager.StoreEvent(ctx, userID, svc.ID, accountID, activity.ServiceDeleted, svc.EventMeta()) - m.proxyController.SendServiceUpdateToCluster(ctx, accountID, svc.ToProtoMapping(service.Delete, "", oidcCfg), svc.ProxyCluster) - } - - m.accountManager.UpdateAccountPeers(ctx, accountID, types.UpdateReason{Resource: types.UpdateResourceService, Operation: types.UpdateOperationDelete}) - - return nil -} - // SetCertificateIssuedAt sets the certificate issued timestamp to the current time. // Call this when receiving a gRPC notification that the certificate was issued. func (m *Manager) SetCertificateIssuedAt(ctx context.Context, accountID, serviceID string) error { diff --git a/management/internals/modules/reverseproxy/service/manager/manager_test.go b/management/internals/modules/reverseproxy/service/manager/manager_test.go index 064e69daa..7fa44a9c0 100644 --- a/management/internals/modules/reverseproxy/service/manager/manager_test.go +++ b/management/internals/modules/reverseproxy/service/manager/manager_test.go @@ -1052,30 +1052,6 @@ func TestDeleteService_DeletesEphemeralExpose(t *testing.T) { assert.NoError(t, err, "new expose should succeed after API delete") } -func TestDeleteAllServices_DeletesEphemeralExposes(t *testing.T) { - ctx := context.Background() - mgr, _ := setupIntegrationTest(t) - - for i := range 3 { - _, err := mgr.CreateServiceFromPeer(ctx, testAccountID, testPeerID, &rpservice.ExposeServiceRequest{ - Port: uint16(8080 + i), - Mode: "http", - }) - require.NoError(t, err) - } - - count, err := mgr.store.CountEphemeralServicesByPeer(ctx, store.LockingStrengthNone, testAccountID, testPeerID) - require.NoError(t, err) - assert.Equal(t, int64(3), count, "all ephemeral services should exist") - - err = mgr.DeleteAllServices(ctx, testAccountID, testUserID) - require.NoError(t, err) - - count, err = mgr.store.CountEphemeralServicesByPeer(ctx, store.LockingStrengthNone, testAccountID, testPeerID) - require.NoError(t, err) - assert.Equal(t, int64(0), count, "all ephemeral services should be deleted after DeleteAllServices") -} - func TestRenewServiceFromPeer(t *testing.T) { ctx := context.Background() diff --git a/management/internals/shared/grpc/proxy_group_access_test.go b/management/internals/shared/grpc/proxy_group_access_test.go index 5f0c31eb6..4cf849ccc 100644 --- a/management/internals/shared/grpc/proxy_group_access_test.go +++ b/management/internals/shared/grpc/proxy_group_access_test.go @@ -24,10 +24,6 @@ type mockReverseProxyManager struct { err error } -func (m *mockReverseProxyManager) DeleteAllServices(ctx context.Context, accountID, userID string) error { - return nil -} - func (m *mockReverseProxyManager) GetAccountServices(ctx context.Context, accountID string) ([]*service.Service, error) { if m.err != nil { return nil, m.err diff --git a/management/internals/shared/grpc/validate_session_test.go b/management/internals/shared/grpc/validate_session_test.go index 4e70e61e4..6fca8f56b 100644 --- a/management/internals/shared/grpc/validate_session_test.go +++ b/management/internals/shared/grpc/validate_session_test.go @@ -506,10 +506,6 @@ func (m *testValidateSessionServiceManager) DeleteService(_ context.Context, _, return nil } -func (m *testValidateSessionServiceManager) DeleteAllServices(_ context.Context, _, _ string) error { - return nil -} - func (m *testValidateSessionServiceManager) SetCertificateIssuedAt(_ context.Context, _, _ string) error { return nil } diff --git a/management/server/account.go b/management/server/account.go index f51caca42..7e8445364 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -1932,7 +1932,7 @@ func (am *DefaultAccountManager) onPeersInvalidated(ctx context.Context, account peers := []*nbpeer.Peer{} log.WithContext(ctx).Debugf("invalidating peers %v for account %s", peerIDs, accountID) for _, peerID := range peerIDs { - peer, err := am.GetPeer(ctx, accountID, peerID, activity.SystemInitiator) + peer, err := am.Store.GetPeerByID(ctx, store.LockingStrengthNone, accountID, peerID) if err != nil { log.WithContext(ctx).Errorf("failed to get invalidated peer %s for account %s: %v", peerID, accountID, err) continue diff --git a/management/server/http/handler.go b/management/server/http/handler.go index e1424a464..73fa950aa 100644 --- a/management/server/http/handler.go +++ b/management/server/http/handler.go @@ -129,7 +129,7 @@ func NewAPIHandler(ctx context.Context, router *mux.Router, accountManager accou agentnetworkhandlers.RegisterEndpoints(agentNetworkManager, router) } instance.AddEndpoints(instanceManager, accountManager, router) - instance.AddVersionEndpoint(instanceManager, router, permissionsManager) + instance.AddVersionEndpoint(instanceManager, router) if serviceManager != nil && reverseProxyDomainManager != nil { reverseproxymanager.RegisterEndpoints(serviceManager, *reverseProxyDomainManager, reverseProxyAccessLogsManager, permissionsManager, router) } diff --git a/management/server/http/handlers/instance/instance_handler.go b/management/server/http/handlers/instance/instance_handler.go index 1c161d7d0..c2498cb96 100644 --- a/management/server/http/handlers/instance/instance_handler.go +++ b/management/server/http/handlers/instance/instance_handler.go @@ -8,8 +8,6 @@ import ( log "github.com/sirupsen/logrus" "github.com/netbirdio/netbird/management/internals/modules/permissions" - "github.com/netbirdio/netbird/management/internals/modules/permissions/modules" - "github.com/netbirdio/netbird/management/internals/modules/permissions/operations" "github.com/netbirdio/netbird/management/server/account" nbinstance "github.com/netbirdio/netbird/management/server/instance" "github.com/netbirdio/netbird/shared/auth" @@ -36,12 +34,12 @@ func AddEndpoints(instanceManager nbinstance.Manager, accountManager account.Man } // AddVersionEndpoint registers the authenticated version endpoint. -func AddVersionEndpoint(instanceManager nbinstance.Manager, router *mux.Router, permissionsManager permissions.Manager) { +func AddVersionEndpoint(instanceManager nbinstance.Manager, router *mux.Router) { h := &handler{ instanceManager: instanceManager, } - router.HandleFunc("/instance/version", permissionsManager.WithPermission(modules.Settings, operations.Read, h.getVersionInfo)).Methods("GET", "OPTIONS") + router.HandleFunc("/instance/version", permissions.WrapHandler(h.getVersionInfo)).Methods("GET", "OPTIONS") } // getInstanceStatus returns the instance status including whether setup is required. diff --git a/management/server/http/handlers/instance/instance_handler_test.go b/management/server/http/handlers/instance/instance_handler_test.go index 7768cdbd7..6918cce2f 100644 --- a/management/server/http/handlers/instance/instance_handler_test.go +++ b/management/server/http/handlers/instance/instance_handler_test.go @@ -15,10 +15,8 @@ import ( "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" - "github.com/netbirdio/netbird/management/internals/modules/permissions" - "github.com/netbirdio/netbird/management/internals/modules/permissions/modules" - "github.com/netbirdio/netbird/management/internals/modules/permissions/operations" "github.com/netbirdio/netbird/management/server/account" + nbcontext "github.com/netbirdio/netbird/management/server/context" "github.com/netbirdio/netbird/management/server/idp" nbinstance "github.com/netbirdio/netbird/management/server/instance" "github.com/netbirdio/netbird/management/server/mock_server" @@ -550,17 +548,11 @@ func TestSetup_PAT_CreatePATFails_Rollback(t *testing.T) { func TestGetVersionInfo_Success(t *testing.T) { manager := &mockInstanceManager{} - ctrl := gomock.NewController(t) - permissionsManager := permissions.NewMockManager(ctrl) - permissionsManager.EXPECT().WithPermission(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(module modules.Module, operation operations.Operation, handler func(w http.ResponseWriter, r *http.Request, userAuth *auth.UserAuth), authErrHandler ...permissions.AuthErrorHandler) http.HandlerFunc { - return func(w http.ResponseWriter, r *http.Request) { - handler(w, r, &auth.UserAuth{}) - } - }).AnyTimes() router := mux.NewRouter() - AddVersionEndpoint(manager, router, permissionsManager) + AddVersionEndpoint(manager, router) req := httptest.NewRequest(http.MethodGet, "/instance/version", nil) + req = nbcontext.SetUserAuthInRequest(req, auth.UserAuth{}) rec := httptest.NewRecorder() router.ServeHTTP(rec, req) @@ -585,17 +577,11 @@ func TestGetVersionInfo_Error(t *testing.T) { return nil, errors.New("failed to fetch versions") }, } - ctrl := gomock.NewController(t) - permissionsManager := permissions.NewMockManager(ctrl) - permissionsManager.EXPECT().WithPermission(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(module modules.Module, operation operations.Operation, handler func(w http.ResponseWriter, r *http.Request, userAuth *auth.UserAuth), authErrHandler ...permissions.AuthErrorHandler) http.HandlerFunc { - return func(w http.ResponseWriter, r *http.Request) { - handler(w, r, &auth.UserAuth{}) - } - }).AnyTimes() router := mux.NewRouter() - AddVersionEndpoint(manager, router, permissionsManager) + AddVersionEndpoint(manager, router) req := httptest.NewRequest(http.MethodGet, "/instance/version", nil) + req = nbcontext.SetUserAuthInRequest(req, auth.UserAuth{}) rec := httptest.NewRecorder() router.ServeHTTP(rec, req) diff --git a/management/server/http/handlers/peers/peers_handler.go b/management/server/http/handlers/peers/peers_handler.go index 3f2942f8e..1b9d4068b 100644 --- a/management/server/http/handlers/peers/peers_handler.go +++ b/management/server/http/handlers/peers/peers_handler.go @@ -34,11 +34,11 @@ type Handler struct { func AddEndpoints(accountManager account.Manager, router *mux.Router, networkMapController network_map.Controller, permissionsManager permissions.Manager) { peersHandler := NewHandler(accountManager, networkMapController, permissionsManager) - router.HandleFunc("/peers", permissionsManager.WithPermission(modules.Peers, operations.Read, peersHandler.GetAllPeers)).Methods("GET", "OPTIONS") - router.HandleFunc("/peers/{peerId}", permissionsManager.WithPermission(modules.Peers, operations.Read, peersHandler.GetPeer)).Methods("GET", "OPTIONS") + router.HandleFunc("/peers", permissionsManager.WithPermission(modules.Peers, operations.Read, peersHandler.GetAllPeers, peersHandler.getOwnPeers)).Methods("GET", "OPTIONS") + router.HandleFunc("/peers/{peerId}", permissionsManager.WithPermission(modules.Peers, operations.Read, peersHandler.GetPeer, peersHandler.getOwnPeer)).Methods("GET", "OPTIONS") router.HandleFunc("/peers/{peerId}", permissionsManager.WithPermission(modules.Peers, operations.Update, peersHandler.UpdatePeer)).Methods("PUT", "OPTIONS") router.HandleFunc("/peers/{peerId}", permissionsManager.WithPermission(modules.Peers, operations.Delete, peersHandler.DeletePeer)).Methods("DELETE", "OPTIONS") - router.HandleFunc("/peers/{peerId}/accessible-peers", permissionsManager.WithPermission(modules.Peers, operations.Read, peersHandler.GetAccessiblePeers)).Methods("GET", "OPTIONS") + router.HandleFunc("/peers/{peerId}/accessible-peers", permissionsManager.WithPermission(modules.Peers, operations.Read, peersHandler.GetAccessiblePeers, peersHandler.getOwnAccessiblePeers)).Methods("GET", "OPTIONS") router.HandleFunc("/peers/{peerId}/temporary-access", permissionsManager.WithPermission(modules.Peers, operations.Create, peersHandler.CreateTemporaryAccess)).Methods("POST", "OPTIONS") router.HandleFunc("/peers/{peerId}/jobs", permissionsManager.WithPermission(modules.RemoteJobs, operations.Read, peersHandler.ListJobs)).Methods("GET", "OPTIONS") router.HandleFunc("/peers/{peerId}/jobs", permissionsManager.WithPermission(modules.RemoteJobs, operations.Create, peersHandler.CreateJob)).Methods("POST", "OPTIONS") @@ -128,19 +128,46 @@ func (h *Handler) GetJob(w http.ResponseWriter, r *http.Request, userAuth *auth. // GetPeer handles GET request for a single peer func (h *Handler) GetPeer(w http.ResponseWriter, r *http.Request, userAuth *auth.UserAuth) { - vars := mux.Vars(r) - peerID := vars["peerId"] + peer, ok := h.peerFromRequest(w, r, userAuth) + if !ok { + return + } + + h.writePeer(w, r, userAuth, peer) +} + +func (h *Handler) getOwnPeer(w http.ResponseWriter, r *http.Request, userAuth *auth.UserAuth) bool { + peer, ok := h.peerFromRequest(w, r, userAuth) + if !ok { + return true + } + + if peer.UserID != userAuth.UserId { + util.WriteError(r.Context(), status.Errorf(status.NotFound, "peer not found"), w) + return true + } + + h.writePeer(w, r, userAuth, peer) + return true +} + +func (h *Handler) peerFromRequest(w http.ResponseWriter, r *http.Request, userAuth *auth.UserAuth) (*nbpeer.Peer, bool) { + peerID := mux.Vars(r)["peerId"] if len(peerID) == 0 { util.WriteError(r.Context(), status.Errorf(status.InvalidArgument, "invalid peer ID"), w) - return + return nil, false } peer, err := h.accountManager.GetPeer(r.Context(), userAuth.AccountId, peerID, userAuth.UserId) if err != nil { util.WriteError(r.Context(), err, w) - return + return nil, false } + return peer, true +} + +func (h *Handler) writePeer(w http.ResponseWriter, r *http.Request, userAuth *auth.UserAuth, peer *nbpeer.Peer) { if peer.ProxyMeta.Embedded { util.WriteError(r.Context(), status.Errorf(status.InvalidArgument, "not allowed to read peer"), w) return @@ -154,7 +181,7 @@ func (h *Handler) GetPeer(w http.ResponseWriter, r *http.Request, userAuth *auth dnsDomain := h.networkMapController.GetDNSDomain(settings) - grps, _ := h.accountManager.GetPeerGroups(r.Context(), userAuth.AccountId, peerID) + grps, _ := h.accountManager.GetPeerGroups(r.Context(), userAuth.AccountId, peer.ID) grpsInfoMap := groups.ToGroupsInfoMap(grps, 0) validPeers, invalidPeers, err := h.accountManager.GetValidatedPeers(r.Context(), userAuth.AccountId) @@ -167,7 +194,7 @@ func (h *Handler) GetPeer(w http.ResponseWriter, r *http.Request, userAuth *auth _, valid := validPeers[peer.ID] reason := invalidPeers[peer.ID] - util.WriteJSONObject(r.Context(), w, toSinglePeerResponse(peer, grpsInfoMap[peerID], dnsDomain, valid, reason)) + util.WriteJSONObject(r.Context(), w, toSinglePeerResponse(peer, grpsInfoMap[peer.ID], dnsDomain, valid, reason)) } // UpdatePeer handles PUT request to update a peer @@ -280,10 +307,19 @@ func (h *Handler) DeletePeer(w http.ResponseWriter, r *http.Request, userAuth *a // GetAllPeers returns a list of all peers associated with a provided account func (h *Handler) GetAllPeers(w http.ResponseWriter, r *http.Request, userAuth *auth.UserAuth) { + h.listPeers(w, r, userAuth, true) +} + +func (h *Handler) getOwnPeers(w http.ResponseWriter, r *http.Request, userAuth *auth.UserAuth) bool { + h.listPeers(w, r, userAuth, false) + return true +} + +func (h *Handler) listPeers(w http.ResponseWriter, r *http.Request, userAuth *auth.UserAuth, all bool) { nameFilter := r.URL.Query().Get("name") ipFilter := r.URL.Query().Get("ip") - peers, err := h.accountManager.GetPeers(r.Context(), userAuth.AccountId, userAuth.UserId, nameFilter, ipFilter, true) + peers, err := h.accountManager.GetPeers(r.Context(), userAuth.AccountId, userAuth.UserId, nameFilter, ipFilter, all) if err != nil { util.WriteError(r.Context(), err, w) return @@ -354,40 +390,36 @@ func (h *Handler) GetAccessiblePeers(w http.ResponseWriter, r *http.Request, use return } - user, err := h.accountManager.GetUserByID(r.Context(), userAuth.UserId) - if err != nil { - util.WriteError(r.Context(), err, w) - return - } - account, err := h.accountManager.GetAccountByID(r.Context(), userAuth.AccountId, activity.SystemInitiator) if err != nil { util.WriteError(r.Context(), err, w) return } - // Check if user is an admin/service user through their role - isAdmin := user.Role == types.UserRoleAdmin || user.Role == types.UserRoleOwner + h.writeAccessiblePeers(w, r, account, peerID) +} - if !isAdmin && !user.IsServiceUser && !userAuth.IsChild { - if account.Settings.RegularUsersViewBlocked { - util.WriteJSONObject(r.Context(), w, []api.AccessiblePeer{}) - return - } +func (h *Handler) getOwnAccessiblePeers(w http.ResponseWriter, r *http.Request, userAuth *auth.UserAuth) bool { + peerID := mux.Vars(r)["peerId"] - peer, ok := account.Peers[peerID] - if !ok { - util.WriteError(r.Context(), status.Errorf(status.NotFound, "peer not found"), w) - return - } - - if peer.UserID != user.Id { - util.WriteJSONObject(r.Context(), w, []api.AccessiblePeer{}) - return - } + account, err := h.accountManager.GetAccountByID(r.Context(), userAuth.AccountId, activity.SystemInitiator) + if err != nil { + util.WriteError(r.Context(), err, w) + return true } - validPeers, _, err := h.accountManager.GetValidatedPeers(r.Context(), userAuth.AccountId) + peer, ok := account.Peers[peerID] + if account.Settings.RegularUsersViewBlocked || !ok || peer.UserID != userAuth.UserId { + util.WriteJSONObject(r.Context(), w, []api.AccessiblePeer{}) + return true + } + + h.writeAccessiblePeers(w, r, account, peerID) + return true +} + +func (h *Handler) writeAccessiblePeers(w http.ResponseWriter, r *http.Request, account *types.Account, peerID string) { + validPeers, _, err := h.accountManager.GetValidatedPeers(r.Context(), account.Id) if err != nil { log.WithContext(r.Context()).Errorf("failed to list approved peers: %v", err) util.WriteError(r.Context(), fmt.Errorf("internal error"), w) @@ -409,9 +441,18 @@ func (h *Handler) CreateTemporaryAccess(w http.ResponseWriter, r *http.Request, return } - var req api.PeerTemporaryAccessRequest - err := json.NewDecoder(r.Body).Decode(&req) + allowed, _, err := h.permissionsManager.ValidateUserPermissions(r.Context(), userAuth.AccountId, userAuth.UserId, modules.Policies, operations.Create) if err != nil { + util.WriteError(r.Context(), status.NewPermissionValidationError(err), w) + return + } + if !allowed { + util.WriteError(r.Context(), status.NewPermissionDeniedError(), w) + return + } + + var req api.PeerTemporaryAccessRequest + if err = json.NewDecoder(r.Body).Decode(&req); err != nil { util.WriteErrorResponse("couldn't parse JSON request", http.StatusBadRequest, w) return } diff --git a/management/server/http/handlers/peers/peers_handler_test.go b/management/server/http/handlers/peers/peers_handler_test.go index 2d4dc1b0c..ed8bdd15b 100644 --- a/management/server/http/handlers/peers/peers_handler_test.go +++ b/management/server/http/handlers/peers/peers_handler_test.go @@ -501,7 +501,7 @@ func TestGetAccessiblePeers(t *testing.T) { }) router := mux.NewRouter() - router.HandleFunc("/api/peers/{peerId}/accessible-peers", permissions.WrapHandler(p.GetAccessiblePeers)).Methods("GET") + router.HandleFunc("/api/peers/{peerId}/accessible-peers", permissions.WithPermission(p.permissionsManager, modules.Peers, operations.Read, p.GetAccessiblePeers, p.getOwnAccessiblePeers)).Methods("GET") router.ServeHTTP(recorder, req) res := recorder.Result() diff --git a/management/server/http/handlers/policies/geolocations_handler.go b/management/server/http/handlers/policies/geolocations_handler.go index dd9ea7c14..8ed616895 100644 --- a/management/server/http/handlers/policies/geolocations_handler.go +++ b/management/server/http/handlers/policies/geolocations_handler.go @@ -7,8 +7,6 @@ import ( "github.com/gorilla/mux" "github.com/netbirdio/netbird/management/internals/modules/permissions" - "github.com/netbirdio/netbird/management/internals/modules/permissions/modules" - "github.com/netbirdio/netbird/management/internals/modules/permissions/operations" "github.com/netbirdio/netbird/management/server/account" "github.com/netbirdio/netbird/management/server/geolocation" "github.com/netbirdio/netbird/shared/auth" @@ -30,8 +28,8 @@ type geolocationsHandler struct { func AddLocationsEndpoints(accountManager account.Manager, locationManager geolocation.Geolocation, permissionsManager permissions.Manager, router *mux.Router) { locationHandler := newGeolocationsHandlerHandler(accountManager, locationManager, permissionsManager) - router.HandleFunc("/locations/countries", permissionsManager.WithPermission(modules.Policies, operations.Read, locationHandler.getAllCountries)).Methods("GET", "OPTIONS") - router.HandleFunc("/locations/countries/{country}/cities", permissionsManager.WithPermission(modules.Policies, operations.Read, locationHandler.getCitiesByCountry)).Methods("GET", "OPTIONS") + router.HandleFunc("/locations/countries", permissions.WrapHandler(locationHandler.getAllCountries)).Methods("GET", "OPTIONS") + router.HandleFunc("/locations/countries/{country}/cities", permissions.WrapHandler(locationHandler.getCitiesByCountry)).Methods("GET", "OPTIONS") } // newGeolocationsHandlerHandler creates a new Geolocations handler diff --git a/management/server/http/handlers/proxy/auth_callback_integration_test.go b/management/server/http/handlers/proxy/auth_callback_integration_test.go index 1dbfca4cd..578dd0561 100644 --- a/management/server/http/handlers/proxy/auth_callback_integration_test.go +++ b/management/server/http/handlers/proxy/auth_callback_integration_test.go @@ -414,10 +414,6 @@ type testServiceManager struct { store store.Store } -func (m *testServiceManager) DeleteAllServices(ctx context.Context, accountID, userID string) error { - return nil -} - func (m *testServiceManager) GetAllServices(_ context.Context, _, _ string) ([]*service.Service, error) { return nil, nil } diff --git a/management/server/http/handlers/users/users_handler.go b/management/server/http/handlers/users/users_handler.go index ccdcffa2c..f08108286 100644 --- a/management/server/http/handlers/users/users_handler.go +++ b/management/server/http/handlers/users/users_handler.go @@ -28,14 +28,14 @@ type handler struct { func AddEndpoints(accountManager account.Manager, router *mux.Router, permissionsManager permissions.Manager) { userHandler := newHandler(accountManager) router.HandleFunc("/users", permissionsManager.WithPermission(modules.Users, operations.Read, userHandler.getAllUsers, userHandler.getOwnUser)).Methods("GET", "OPTIONS") - router.HandleFunc("/users/current", permissionsManager.WithPermission(modules.Users, operations.Read, userHandler.getCurrentUser, userHandler.getCurrentUserFallback)).Methods("GET", "OPTIONS") + router.HandleFunc("/users/current", permissions.WrapHandler(userHandler.getCurrentUser)).Methods("GET", "OPTIONS") router.HandleFunc("/users/{userId}", permissionsManager.WithPermission(modules.Users, operations.Update, userHandler.updateUser)).Methods("PUT", "OPTIONS") router.HandleFunc("/users/{userId}", permissionsManager.WithPermission(modules.Users, operations.Delete, userHandler.deleteUser)).Methods("DELETE", "OPTIONS") router.HandleFunc("/users", permissionsManager.WithPermission(modules.Users, operations.Create, userHandler.createUser)).Methods("POST", "OPTIONS") router.HandleFunc("/users/{userId}/invite", permissionsManager.WithPermission(modules.Users, operations.Create, userHandler.inviteUser)).Methods("POST", "OPTIONS") router.HandleFunc("/users/{userId}/approve", permissionsManager.WithPermission(modules.Users, operations.Update, userHandler.approveUser)).Methods("POST", "OPTIONS") router.HandleFunc("/users/{userId}/reject", permissionsManager.WithPermission(modules.Users, operations.Delete, userHandler.rejectUser)).Methods("DELETE", "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") addUsersTokensEndpoint(accountManager, router, permissionsManager) } @@ -405,28 +405,16 @@ func (h *handler) changePassword(w http.ResponseWriter, r *http.Request, userAut util.WriteJSONObject(r.Context(), w, util.EmptyObject{}) } -func (h *handler) getCurrentUserFallback(w http.ResponseWriter, r *http.Request, userAuth *auth.UserAuth, err error) bool { - s, ok := status.FromError(err) - if !ok || s.ErrorType != status.PermissionDenied { +func (h *handler) changeOwnPassword(w http.ResponseWriter, r *http.Request, userAuth *auth.UserAuth) bool { + if mux.Vars(r)["userId"] != userAuth.UserId { return false } - user, userErr := h.accountManager.GetCurrentUserInfo(r.Context(), *userAuth) - if userErr != nil { - util.WriteError(r.Context(), userErr, w) - return true - } - - util.WriteJSONObject(r.Context(), w, toUserWithPermissionsResponse(user, userAuth.UserId)) + h.changePassword(w, r, userAuth) return true } -func (h *handler) getOwnUser(w http.ResponseWriter, r *http.Request, userAuth *auth.UserAuth, err error) bool { - s, ok := status.FromError(err) - if !ok || s.ErrorType != status.PermissionDenied { - return false - } - +func (h *handler) getOwnUser(w http.ResponseWriter, r *http.Request, userAuth *auth.UserAuth) bool { if r.URL.Query().Get("service_user") != "" { return false } diff --git a/management/server/http/testing/integration/peers_handler_integration_test.go b/management/server/http/testing/integration/peers_handler_integration_test.go index b06e6679a..f325a42d9 100644 --- a/management/server/http/testing/integration/peers_handler_integration_test.go +++ b/management/server/http/testing/integration/peers_handler_integration_test.go @@ -26,31 +26,37 @@ func Test_Peers_GetAll(t *testing.T) { name string userId string expectResponse bool + expectedPeers int }{ { name: "Regular user", userId: testing_tools.TestUserId, - expectResponse: false, + expectResponse: true, + expectedPeers: 1, }, { name: "Admin user", userId: testing_tools.TestAdminId, expectResponse: true, + expectedPeers: 2, }, { name: "Owner user", userId: testing_tools.TestOwnerId, expectResponse: true, + expectedPeers: 2, }, { name: "Regular service user", userId: testing_tools.TestServiceUserId, - expectResponse: false, + expectResponse: true, + expectedPeers: 0, }, { name: "Admin service user", userId: testing_tools.TestServiceAdminId, expectResponse: true, + expectedPeers: 2, }, { name: "Blocked user", @@ -88,7 +94,7 @@ func Test_Peers_GetAll(t *testing.T) { t.Fatalf("Sent content is not in correct json format; %v", err) } - assert.GreaterOrEqual(t, len(got), 2, "Expected at least 2 peers") + assert.Len(t, got, user.expectedPeers, "regular users must only see their own peers") select { case <-done: @@ -99,17 +105,36 @@ func Test_Peers_GetAll(t *testing.T) { } } +func Test_Peers_GetById_RegularUser(t *testing.T) { + tt := []struct { + name string + peerId string + expectedStatus int + }{ + {"Own peer", testing_tools.TestPeerId, http.StatusOK}, + {"Peer of another user", testPeerId2, http.StatusNotFound}, + {"Non-existing peer", "nonExistingPeerId", http.StatusNotFound}, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + apiHandler, _, _ := channel.BuildApiBlackBoxWithDBState(t, "../testdata/peers_integration.sql", nil, false) + + req := testing_tools.BuildRequest(t, []byte{}, http.MethodGet, "/api/peers/"+tc.peerId, testing_tools.TestUserId) + recorder := httptest.NewRecorder() + apiHandler.ServeHTTP(recorder, req) + + assert.Equal(t, tc.expectedStatus, recorder.Code, "unexpected status, body: %s", recorder.Body.String()) + }) + } +} + func Test_Peers_GetById(t *testing.T) { users := []struct { name string userId string expectResponse bool }{ - { - name: "Regular user", - userId: testing_tools.TestUserId, - expectResponse: false, - }, { name: "Admin user", userId: testing_tools.TestAdminId, @@ -120,6 +145,11 @@ func Test_Peers_GetById(t *testing.T) { userId: testing_tools.TestOwnerId, expectResponse: true, }, + { + name: "Auditor user", + userId: testing_tools.TestAuditorId, + expectResponse: true, + }, { name: "Regular service user", userId: testing_tools.TestServiceUserId, @@ -508,7 +538,7 @@ func Test_Peers_GetAccessiblePeers(t *testing.T) { { name: "Regular user", userId: testing_tools.TestUserId, - expectResponse: false, + expectResponse: true, }, { name: "Admin user", @@ -523,7 +553,7 @@ func Test_Peers_GetAccessiblePeers(t *testing.T) { { name: "Regular service user", userId: testing_tools.TestServiceUserId, - expectResponse: false, + expectResponse: true, }, { name: "Admin service user", diff --git a/management/server/http/testing/integration/users_handler_integration_test.go b/management/server/http/testing/integration/users_handler_integration_test.go index a1c9e48d9..d324bf2a3 100644 --- a/management/server/http/testing/integration/users_handler_integration_test.go +++ b/management/server/http/testing/integration/users_handler_integration_test.go @@ -26,6 +26,7 @@ func Test_Users_GetAll(t *testing.T) { {"Regular user", testing_tools.TestUserId, true}, {"Admin user", testing_tools.TestAdminId, true}, {"Owner user", testing_tools.TestOwnerId, true}, + {"Auditor user", testing_tools.TestAuditorId, true}, {"Regular service user", testing_tools.TestServiceUserId, false}, {"Admin service user", testing_tools.TestServiceAdminId, true}, {"Blocked user", testing_tools.BlockedUserId, false}, @@ -62,6 +63,49 @@ func Test_Users_GetAll(t *testing.T) { } } +func Test_Users_GetAll_ReadOnlyRoleSeesAllUsers(t *testing.T) { + apiHandler, _, _ := channel.BuildApiBlackBoxWithDBState(t, "../testdata/users_integration.sql", nil, false) + + req := testing_tools.BuildRequest(t, []byte{}, http.MethodGet, "/api/users", testing_tools.TestAuditorId) + recorder := httptest.NewRecorder() + apiHandler.ServeHTTP(recorder, req) + + content, _ := testing_tools.ReadResponse(t, recorder, http.StatusOK, true) + + got := []api.User{} + if err := json.Unmarshal(content, &got); err != nil { + t.Fatalf("Sent content is not in correct json format; %v", err) + } + + assert.Greater(t, len(got), 1, "auditor must see every user of the account, not only themselves") +} + +func Test_Users_ChangePassword(t *testing.T) { + tt := []struct { + name string + userId string + targetUserId string + expectedStatus int + }{ + {"Regular user changes own password", testing_tools.TestUserId, testing_tools.TestUserId, http.StatusPreconditionFailed}, + {"Regular user changes another user's password", testing_tools.TestUserId, testing_tools.TestAdminId, http.StatusForbidden}, + {"Admin changes another user's password", testing_tools.TestAdminId, testing_tools.TestUserId, http.StatusPreconditionFailed}, + } + + body := []byte(`{"old_password":"OldPass123!","new_password":"NewPass456!"}`) + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + apiHandler, _, _ := channel.BuildApiBlackBoxWithDBState(t, "../testdata/users_integration.sql", nil, false) + + req := testing_tools.BuildRequest(t, body, http.MethodPut, "/api/users/"+tc.targetUserId+"/password", tc.userId) + recorder := httptest.NewRecorder() + apiHandler.ServeHTTP(recorder, req) + + assert.Equal(t, tc.expectedStatus, recorder.Code, "unexpected status, body: %s", recorder.Body.String()) + }) + } +} + func Test_Users_GetAll_ServiceUsers(t *testing.T) { users := []struct { name string diff --git a/management/server/http/testing/testdata/peers.sql b/management/server/http/testing/testdata/peers.sql index 3593222a7..6830d8329 100644 --- a/management/server/http/testing/testdata/peers.sql +++ b/management/server/http/testing/testdata/peers.sql @@ -7,6 +7,7 @@ INSERT INTO accounts VALUES('testAccountId','','2024-10-02 16:01:38.000000000+00 INSERT INTO users VALUES('testUserId','testAccountId','user',0,0,'','[]',0,NULL,'2024-10-02 16:01:38.000000000+00:00','api',0,''); INSERT INTO users VALUES('testAdminId','testAccountId','admin',0,0,'','[]',0,NULL,'2024-10-02 16:01:38.000000000+00:00','api',0,''); INSERT INTO users VALUES('testOwnerId','testAccountId','owner',0,0,'','[]',0,NULL,'2024-10-02 16:01:38.000000000+00:00','api',0,''); +INSERT INTO users VALUES('testAuditorId','testAccountId','auditor',0,0,'','[]',0,NULL,'2024-10-02 16:01:38.000000000+00:00','api',0,''); INSERT INTO users VALUES('testServiceUserId','testAccountId','user',1,0,'','[]',0,NULL,'2024-10-02 16:01:38.000000000+00:00','api',0,''); INSERT INTO users VALUES('testServiceAdminId','testAccountId','admin',1,0,'','[]',0,NULL,'2024-10-02 16:01:38.000000000+00:00','api',0,''); INSERT INTO users VALUES('blockedUserId','testAccountId','admin',0,0,'','[]',1,NULL,'2024-10-02 16:01:38.000000000+00:00','api',0,''); diff --git a/management/server/http/testing/testdata/peers_integration.sql b/management/server/http/testing/testdata/peers_integration.sql index eb6094f1f..ce17432bb 100644 --- a/management/server/http/testing/testdata/peers_integration.sql +++ b/management/server/http/testing/testdata/peers_integration.sql @@ -8,6 +8,7 @@ INSERT INTO accounts VALUES('testAccountId','','2024-10-02 16:01:38.000000000+00 INSERT INTO users VALUES('testUserId','testAccountId','user',0,0,'','[]',0,NULL,'2024-10-02 16:01:38.000000000+00:00','api',0,''); INSERT INTO users VALUES('testAdminId','testAccountId','admin',0,0,'','[]',0,NULL,'2024-10-02 16:01:38.000000000+00:00','api',0,''); INSERT INTO users VALUES('testOwnerId','testAccountId','owner',0,0,'','[]',0,NULL,'2024-10-02 16:01:38.000000000+00:00','api',0,''); +INSERT INTO users VALUES('testAuditorId','testAccountId','auditor',0,0,'','[]',0,NULL,'2024-10-02 16:01:38.000000000+00:00','api',0,''); INSERT INTO users VALUES('testServiceUserId','testAccountId','user',1,0,'','[]',0,NULL,'2024-10-02 16:01:38.000000000+00:00','api',0,''); INSERT INTO users VALUES('testServiceAdminId','testAccountId','admin',1,0,'','[]',0,NULL,'2024-10-02 16:01:38.000000000+00:00','api',0,''); INSERT INTO users VALUES('blockedUserId','testAccountId','admin',0,0,'','[]',1,NULL,'2024-10-02 16:01:38.000000000+00:00','api',0,''); diff --git a/management/server/http/testing/testdata/users_integration.sql b/management/server/http/testing/testdata/users_integration.sql index 90ce450e3..455cb69a0 100644 --- a/management/server/http/testing/testdata/users_integration.sql +++ b/management/server/http/testing/testdata/users_integration.sql @@ -10,6 +10,7 @@ INSERT INTO accounts VALUES('testAccountId','','2024-10-02 16:01:38.000000000+00 INSERT INTO users VALUES('testUserId','testAccountId','user',0,0,'','[]',0,NULL,'2024-10-02 16:01:38.000000000+00:00','api',0,''); INSERT INTO users VALUES('testAdminId','testAccountId','admin',0,0,'','[]',0,NULL,'2024-10-02 16:01:38.000000000+00:00','api',0,''); INSERT INTO users VALUES('testOwnerId','testAccountId','owner',0,0,'','[]',0,NULL,'2024-10-02 16:01:38.000000000+00:00','api',0,''); +INSERT INTO users VALUES('testAuditorId','testAccountId','auditor',0,0,'','[]',0,NULL,'2024-10-02 16:01:38.000000000+00:00','api',0,''); INSERT INTO users VALUES('testServiceUserId','testAccountId','user',1,0,'testServiceUser','[]',0,NULL,'2024-10-02 16:01:38.000000000+00:00','api',0,''); INSERT INTO users VALUES('testServiceAdminId','testAccountId','admin',1,0,'testServiceAdmin','[]',0,NULL,'2024-10-02 16:01:38.000000000+00:00','api',0,''); INSERT INTO users VALUES('blockedUserId','testAccountId','admin',0,0,'','[]',1,NULL,'2024-10-02 16:01:38.000000000+00:00','api',0,''); diff --git a/management/server/http/testing/testing_tools/channel/channel.go b/management/server/http/testing/testing_tools/channel/channel.go index 80cbf7440..cc6b433cb 100644 --- a/management/server/http/testing/testing_tools/channel/channel.go +++ b/management/server/http/testing/testing_tools/channel/channel.go @@ -298,7 +298,7 @@ func mockValidateAndParseToken(_ context.Context, token string) (auth.UserAuth, userAuth := auth.UserAuth{} switch token { - case "testUserId", "testAdminId", "testOwnerId", "testServiceUserId", "testServiceAdminId", "blockedUserId": + case "testUserId", "testAdminId", "testOwnerId", "testAuditorId", "testServiceUserId", "testServiceAdminId", "blockedUserId": userAuth.UserId = token userAuth.AccountId = "testAccountId" userAuth.Domain = "test.com" diff --git a/management/server/http/testing/testing_tools/tools.go b/management/server/http/testing/testing_tools/tools.go index 755ef85e5..6c543fc54 100644 --- a/management/server/http/testing/testing_tools/tools.go +++ b/management/server/http/testing/testing_tools/tools.go @@ -32,6 +32,7 @@ const ( TestUserId = "testUserId" TestAdminId = "testAdminId" TestOwnerId = "testOwnerId" + TestAuditorId = "testAuditorId" TestServiceUserId = "testServiceUserId" TestServiceAdminId = "testServiceAdminId" BlockedUserId = "blockedUserId" diff --git a/management/server/identity_provider.go b/management/server/identity_provider.go index 9730fe078..324869b4b 100644 --- a/management/server/identity_provider.go +++ b/management/server/identity_provider.go @@ -132,15 +132,15 @@ func (am *DefaultAccountManager) GetIdentityProvider(ctx context.Context, accoun // CreateIdentityProvider creates a new identity provider func (am *DefaultAccountManager) CreateIdentityProvider(ctx context.Context, accountID, userID string, idpConfig *types.IdentityProvider) (*types.IdentityProvider, error) { + if err := validateIdentityProviderConfig(ctx, idpConfig); err != nil { + return nil, err + } + embeddedManager, ok := am.idpManager.(*idp.EmbeddedIdPManager) if !ok { return nil, status.Errorf(status.Internal, "identity provider management requires embedded IdP") } - if err := validateIdentityProviderConfig(ctx, idpConfig); err != nil { - return nil, err - } - // Generate ID if not provided if idpConfig.ID == "" { idpConfig.ID = generateIdentityProviderID(idpConfig.Type) @@ -161,15 +161,15 @@ func (am *DefaultAccountManager) CreateIdentityProvider(ctx context.Context, acc // UpdateIdentityProvider updates an existing identity provider func (am *DefaultAccountManager) UpdateIdentityProvider(ctx context.Context, accountID, idpID, userID string, idpConfig *types.IdentityProvider) (*types.IdentityProvider, error) { + if err := validateIdentityProviderConfig(ctx, idpConfig); err != nil { + return nil, err + } + embeddedManager, ok := am.idpManager.(*idp.EmbeddedIdPManager) if !ok { return nil, status.Errorf(status.Internal, "identity provider management requires embedded IdP") } - if err := validateIdentityProviderConfig(ctx, idpConfig); err != nil { - return nil, err - } - idpConfig.ID = idpID idpConfig.AccountID = accountID diff --git a/management/server/peer.go b/management/server/peer.go index c39269c13..f57b5875f 100644 --- a/management/server/peer.go +++ b/management/server/peer.go @@ -51,7 +51,7 @@ func (am *DefaultAccountManager) GetPeers(ctx context.Context, accountID, userID return nil, err } - if all || user.IsAdminOrServiceUser() { + if all || user.HasAdminPower() { return am.Store.GetAccountPeers(ctx, store.LockingStrengthNone, accountID, nameFilter, ipFilter) } @@ -1453,25 +1453,10 @@ func peerLoginExpired(ctx context.Context, peer *nbpeer.Peer, settings *types.Se return false } -// GetPeer returns a peer visible to the user within an account. -// Users with "peers:read" permission can access any peer. Otherwise, users can access only their own peer. +// GetPeer returns a peer within an account. Callers are expected to have passed the +// "peers:read" permission check at the HTTP layer. func (am *DefaultAccountManager) GetPeer(ctx context.Context, accountID, peerID, userID string) (*nbpeer.Peer, error) { - peer, err := am.Store.GetPeerByID(ctx, store.LockingStrengthNone, accountID, peerID) - if err != nil { - return nil, err - } - - user, err := am.Store.GetUserByUserID(ctx, store.LockingStrengthNone, userID) - if err != nil { - return nil, err - } - - // if admin or user owns this peer, return peer - if user.IsAdminOrServiceUser() || peer.UserID == userID { - return peer, nil - } - - return nil, status.Errorf(status.Internal, "user %s has no access to peer %s under account %s", userID, peer.ID, accountID) + return am.Store.GetPeerByID(ctx, store.LockingStrengthNone, accountID, peerID) } // UpdateAccountPeers updates all peers that belong to an account. diff --git a/management/server/peer_test.go b/management/server/peer_test.go index c25eff813..013933b35 100644 --- a/management/server/peer_test.go +++ b/management/server/peer_test.go @@ -553,7 +553,7 @@ func TestDefaultAccountManager_GetPeer(t *testing.T) { return } - // the user can see its own peer + // authorization is enforced at the HTTP layer, the manager returns any peer of the account peer, err := manager.GetPeer(context.Background(), accountID, peer1.ID, someUser) if err != nil { t.Fatal(err) @@ -561,12 +561,13 @@ func TestDefaultAccountManager_GetPeer(t *testing.T) { } assert.NotNil(t, peer) - // the user can NOT see peer2 because it is not owned by them. - // Regular users only see peers they directly own. - _, err = manager.GetPeer(context.Background(), accountID, peer2.ID, someUser) - assert.Error(t, err) + peer, err = manager.GetPeer(context.Background(), accountID, peer2.ID, someUser) + if err != nil { + t.Fatal(err) + return + } + assert.NotNil(t, peer) - // admin users can always access all the peers peer, err = manager.GetPeer(context.Background(), accountID, peer1.ID, adminUser) if err != nil { t.Fatal(err) @@ -602,7 +603,7 @@ func TestDefaultAccountManager_GetPeers(t *testing.T) { role: types.UserRoleUser, limitedViewSettings: false, isServiceUser: true, - expectedPeerCount: 2, + expectedPeerCount: 1, }, { name: "Regular user, limited view settings", @@ -616,7 +617,7 @@ func TestDefaultAccountManager_GetPeers(t *testing.T) { role: types.UserRoleUser, limitedViewSettings: true, isServiceUser: true, - expectedPeerCount: 2, + expectedPeerCount: 0, }, { name: "Admin, no limited view settings, not a service user", diff --git a/management/server/user.go b/management/server/user.go index 44fb65e91..8607c6fac 100644 --- a/management/server/user.go +++ b/management/server/user.go @@ -16,6 +16,8 @@ import ( log "github.com/sirupsen/logrus" "github.com/netbirdio/netbird/idp/dex" + "github.com/netbirdio/netbird/management/internals/modules/permissions/modules" + "github.com/netbirdio/netbird/management/internals/modules/permissions/operations" "github.com/netbirdio/netbird/management/server/account" "github.com/netbirdio/netbird/management/server/activity" "github.com/netbirdio/netbird/management/server/affectedpeers" @@ -965,8 +967,12 @@ func (am *DefaultAccountManager) GetOrCreateAccountByUser(ctx context.Context, u // GetUsersFromAccount performs a batched request for users from IDP by account ID apply filter on what data to return // based on provided user role. func (am *DefaultAccountManager) GetUsersFromAccount(ctx context.Context, accountID, initiatorUserID string) (map[string]*types.UserInfo, error) { + allowed, ctx, err := am.permissionsManager.ValidateUserPermissions(ctx, accountID, initiatorUserID, modules.Users, operations.Read) + if err != nil { + return nil, status.NewPermissionValidationError(err) + } + var user *types.User - var err error if initiatorUserID != activity.SystemInitiator { result, err := am.Store.GetUserByUserID(ctx, store.LockingStrengthNone, initiatorUserID) if err != nil { @@ -976,11 +982,8 @@ func (am *DefaultAccountManager) GetUsersFromAccount(ctx context.Context, accoun } accountUsers := []*types.User{} - - hasFullAccess := initiatorUserID == activity.SystemInitiator || user.HasAdminPower() || user.IsServiceUser - switch { - case hasFullAccess: + case allowed: start := time.Now() accountUsers, err = am.Store.GetAccountUsers(ctx, store.LockingStrengthNone, accountID) if err != nil { @@ -1465,8 +1468,10 @@ func (am *DefaultAccountManager) GetCurrentUserInfo(ctx context.Context, userAut return nil, status.NewPermissionDeniedError() } - // Permission checks are now handled by the HTTP middleware via WithPermission wrapper - // User account association is already validated above by GetUserByUserID + ctx, err = am.permissionsManager.ValidateAccountAccess(ctx, accountID, user, false) + if err != nil { + return nil, err + } settings, err := am.Store.GetAccountSettings(ctx, store.LockingStrengthNone, accountID) if err != nil { diff --git a/management/server/user_test.go b/management/server/user_test.go index 028e455d1..3b762a8a2 100644 --- a/management/server/user_test.go +++ b/management/server/user_test.go @@ -1414,8 +1414,8 @@ func TestUser_GetUsersFromAccount_ForUser(t *testing.T) { t.Fatalf("Error when getting users from account: %s", err) } - // Service users should see all users - assert.Equal(t, 2, len(users)) + // Service users follow their role like any other user, a role user only sees themselves + assert.Equal(t, 1, len(users)) } func TestDefaultAccountManager_SaveUser(t *testing.T) { diff --git a/proxy/management_integration_test.go b/proxy/management_integration_test.go index df016e790..37fc2df74 100644 --- a/proxy/management_integration_test.go +++ b/proxy/management_integration_test.go @@ -293,10 +293,6 @@ type storeBackedServiceManager struct { tokenStore *nbgrpc.OneTimeTokenStore } -func (m *storeBackedServiceManager) DeleteAllServices(ctx context.Context, accountID, userID string) error { - return nil -} - func (m *storeBackedServiceManager) GetAllServices(ctx context.Context, accountID, userID string) ([]*service.Service, error) { return m.store.GetAccountServices(ctx, store.LockingStrengthNone, accountID) }