From d39fcfd62ab7cf956a30249da40bba53f8b8feab Mon Sep 17 00:00:00 2001 From: Maycon Santos Date: Mon, 1 Sep 2025 13:00:45 -0300 Subject: [PATCH] [management] Add user approval (#4411) This PR adds user approval functionality to the management system, allowing administrators to manually approve new users joining via domain matching. When enabled, users are blocked with pending approval status until explicitly approved by an admin. Adds UserApprovalRequired setting to control manual user approval requirement Introduces user approval and rejection endpoints with corresponding business logic Prevents pending approval users from adding peers or logging in --- management/server/account.go | 25 ++- management/server/account/manager.go | 2 + management/server/account_test.go | 90 +++++++++ management/server/activity/codes.go | 4 + .../handlers/accounts/accounts_handler.go | 6 +- .../accounts/accounts_handler_test.go | 4 +- .../http/handlers/users/users_handler.go | 87 +++++++-- .../http/handlers/users/users_handler_test.go | 134 ++++++++++++- management/server/mock_server/account_mock.go | 16 ++ management/server/peer.go | 3 + management/server/peer_test.go | 183 ++++++++++++++++++ management/server/permissions/manager.go | 6 +- management/server/types/settings.go | 4 + management/server/types/user.go | 46 +++-- management/server/user.go | 74 +++++++ management/server/user_test.go | 114 +++++++++++ shared/management/http/api/openapi.yml | 67 +++++++ shared/management/http/api/types.gen.go | 20 +- shared/management/status/error.go | 5 + 19 files changed, 842 insertions(+), 48 deletions(-) diff --git a/management/server/account.go b/management/server/account.go index b57550f14..d9638b41a 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -1136,7 +1136,18 @@ func (am *DefaultAccountManager) addNewPrivateAccount(ctx context.Context, domai func (am *DefaultAccountManager) addNewUserToDomainAccount(ctx context.Context, domainAccountID string, userAuth nbcontext.UserAuth) (string, error) { newUser := types.NewRegularUser(userAuth.UserId) newUser.AccountID = domainAccountID - err := am.Store.SaveUser(ctx, newUser) + + settings, err := am.Store.GetAccountSettings(ctx, store.LockingStrengthNone, domainAccountID) + if err != nil { + return "", err + } + + if settings != nil && settings.Extra != nil && settings.Extra.UserApprovalRequired { + newUser.Blocked = true + newUser.PendingApproval = true + } + + err = am.Store.SaveUser(ctx, newUser) if err != nil { return "", err } @@ -1146,7 +1157,11 @@ func (am *DefaultAccountManager) addNewUserToDomainAccount(ctx context.Context, return "", err } - am.StoreEvent(ctx, userAuth.UserId, userAuth.UserId, domainAccountID, activity.UserJoined, nil) + if newUser.PendingApproval { + am.StoreEvent(ctx, userAuth.UserId, userAuth.UserId, domainAccountID, activity.UserJoined, map[string]any{"pending_approval": true}) + } else { + am.StoreEvent(ctx, userAuth.UserId, userAuth.UserId, domainAccountID, activity.UserJoined, nil) + } return domainAccountID, nil } @@ -1795,6 +1810,9 @@ func newAccountWithId(ctx context.Context, accountID, userID, domain string, dis PeerInactivityExpirationEnabled: false, PeerInactivityExpiration: types.DefaultPeerInactivityExpiration, RoutingPeerDNSResolutionEnabled: true, + Extra: &types.ExtraSettings{ + UserApprovalRequired: true, + }, }, Onboarding: types.AccountOnboarding{ OnboardingFlowPending: true, @@ -1901,6 +1919,9 @@ func (am *DefaultAccountManager) GetOrCreateAccountByPrivateDomain(ctx context.C PeerInactivityExpirationEnabled: false, PeerInactivityExpiration: types.DefaultPeerInactivityExpiration, RoutingPeerDNSResolutionEnabled: true, + Extra: &types.ExtraSettings{ + UserApprovalRequired: true, + }, }, } diff --git a/management/server/account/manager.go b/management/server/account/manager.go index c7329a1da..30fbbbc3e 100644 --- a/management/server/account/manager.go +++ b/management/server/account/manager.go @@ -32,6 +32,8 @@ type Manager interface { DeleteUser(ctx context.Context, accountID, initiatorUserID string, targetUserID string) error DeleteRegularUsers(ctx context.Context, accountID, initiatorUserID string, targetUserIDs []string, userInfos map[string]*types.UserInfo) error InviteUser(ctx context.Context, accountID string, initiatorUserID string, targetUserID string) error + ApproveUser(ctx context.Context, accountID, initiatorUserID, targetUserID string) (*types.UserInfo, error) + RejectUser(ctx context.Context, accountID, initiatorUserID, targetUserID string) error ListSetupKeys(ctx context.Context, accountID, userID string) ([]*types.SetupKey, error) SaveUser(ctx context.Context, accountID, initiatorUserID string, update *types.User) (*types.UserInfo, error) SaveOrAddUser(ctx context.Context, accountID, initiatorUserID string, update *types.User, addIfNotExists bool) (*types.UserInfo, error) diff --git a/management/server/account_test.go b/management/server/account_test.go index 66cf93286..81a921bf9 100644 --- a/management/server/account_test.go +++ b/management/server/account_test.go @@ -3606,3 +3606,93 @@ func TestDefaultAccountManager_UpdatePeerIP(t *testing.T) { require.Error(t, err, "should fail with invalid peer ID") }) } + +func TestAddNewUserToDomainAccountWithApproval(t *testing.T) { + manager, err := createManager(t) + if err != nil { + t.Fatal(err) + } + + // Create a domain-based account with user approval enabled + existingAccountID := "existing-account" + account := newAccountWithId(context.Background(), existingAccountID, "owner-user", "example.com", false) + account.Settings.Extra = &types.ExtraSettings{ + UserApprovalRequired: true, + } + err = manager.Store.SaveAccount(context.Background(), account) + require.NoError(t, err) + + // Set the account as domain primary account + account.IsDomainPrimaryAccount = true + account.DomainCategory = types.PrivateCategory + err = manager.Store.SaveAccount(context.Background(), account) + require.NoError(t, err) + + // Test adding new user to existing account with approval required + newUserID := "new-user-id" + userAuth := nbcontext.UserAuth{ + UserId: newUserID, + Domain: "example.com", + DomainCategory: types.PrivateCategory, + } + + acc, err := manager.Store.GetAccount(context.Background(), existingAccountID) + require.NoError(t, err) + require.True(t, acc.IsDomainPrimaryAccount, "Account should be primary for the domain") + require.Equal(t, "example.com", acc.Domain, "Account domain should match") + + returnedAccountID, err := manager.getAccountIDWithAuthorizationClaims(context.Background(), userAuth) + require.NoError(t, err) + require.Equal(t, existingAccountID, returnedAccountID) + + // Verify user was created with pending approval + user, err := manager.Store.GetUserByUserID(context.Background(), store.LockingStrengthNone, newUserID) + require.NoError(t, err) + assert.True(t, user.Blocked, "User should be blocked when approval is required") + assert.True(t, user.PendingApproval, "User should be pending approval") + assert.Equal(t, existingAccountID, user.AccountID) +} + +func TestAddNewUserToDomainAccountWithoutApproval(t *testing.T) { + manager, err := createManager(t) + if err != nil { + t.Fatal(err) + } + + // Create a domain-based account without user approval + ownerUserAuth := nbcontext.UserAuth{ + UserId: "owner-user", + Domain: "example.com", + DomainCategory: types.PrivateCategory, + } + existingAccountID, err := manager.getAccountIDWithAuthorizationClaims(context.Background(), ownerUserAuth) + require.NoError(t, err) + + // Modify the account to disable user approval + account, err := manager.Store.GetAccount(context.Background(), existingAccountID) + require.NoError(t, err) + account.Settings.Extra = &types.ExtraSettings{ + UserApprovalRequired: false, + } + err = manager.Store.SaveAccount(context.Background(), account) + require.NoError(t, err) + + // Test adding new user to existing account without approval required + newUserID := "new-user-id" + userAuth := nbcontext.UserAuth{ + UserId: newUserID, + Domain: "example.com", + DomainCategory: types.PrivateCategory, + } + + returnedAccountID, err := manager.getAccountIDWithAuthorizationClaims(context.Background(), userAuth) + require.NoError(t, err) + require.Equal(t, existingAccountID, returnedAccountID) + + // Verify user was created without pending approval + user, err := manager.Store.GetUserByUserID(context.Background(), store.LockingStrengthNone, newUserID) + require.NoError(t, err) + assert.False(t, user.Blocked, "User should not be blocked when approval is not required") + assert.False(t, user.PendingApproval, "User should not be pending approval") + assert.Equal(t, existingAccountID, user.AccountID) +} diff --git a/management/server/activity/codes.go b/management/server/activity/codes.go index 6f9619597..5c5989f84 100644 --- a/management/server/activity/codes.go +++ b/management/server/activity/codes.go @@ -177,6 +177,8 @@ const ( AccountNetworkRangeUpdated Activity = 87 PeerIPUpdated Activity = 88 + UserApproved Activity = 89 + UserRejected Activity = 90 AccountDeleted Activity = 99999 ) @@ -284,6 +286,8 @@ var activityMap = map[Activity]Code{ AccountNetworkRangeUpdated: {"Account network range updated", "account.network.range.update"}, PeerIPUpdated: {"Peer IP updated", "peer.ip.update"}, + UserApproved: {"User approved", "user.approve"}, + UserRejected: {"User rejected", "user.reject"}, } // StringCode returns a string code of the activity diff --git a/management/server/http/handlers/accounts/accounts_handler.go b/management/server/http/handlers/accounts/accounts_handler.go index 9f2afe29d..f1552d0ea 100644 --- a/management/server/http/handlers/accounts/accounts_handler.go +++ b/management/server/http/handlers/accounts/accounts_handler.go @@ -11,11 +11,11 @@ import ( "github.com/netbirdio/netbird/management/server/account" nbcontext "github.com/netbirdio/netbird/management/server/context" + "github.com/netbirdio/netbird/management/server/settings" + "github.com/netbirdio/netbird/management/server/types" "github.com/netbirdio/netbird/shared/management/http/api" "github.com/netbirdio/netbird/shared/management/http/util" - "github.com/netbirdio/netbird/management/server/settings" "github.com/netbirdio/netbird/shared/management/status" - "github.com/netbirdio/netbird/management/server/types" ) const ( @@ -198,6 +198,7 @@ func (h *handler) updateAccount(w http.ResponseWriter, r *http.Request) { if req.Settings.Extra != nil { settings.Extra = &types.ExtraSettings{ PeerApprovalEnabled: req.Settings.Extra.PeerApprovalEnabled, + UserApprovalRequired: req.Settings.Extra.UserApprovalRequired, FlowEnabled: req.Settings.Extra.NetworkTrafficLogsEnabled, FlowGroups: req.Settings.Extra.NetworkTrafficLogsGroups, FlowPacketCounterEnabled: req.Settings.Extra.NetworkTrafficPacketCounterEnabled, @@ -327,6 +328,7 @@ func toAccountResponse(accountID string, settings *types.Settings, meta *types.A if settings.Extra != nil { apiSettings.Extra = &api.AccountExtraSettings{ PeerApprovalEnabled: settings.Extra.PeerApprovalEnabled, + UserApprovalRequired: settings.Extra.UserApprovalRequired, NetworkTrafficLogsEnabled: settings.Extra.FlowEnabled, NetworkTrafficLogsGroups: settings.Extra.FlowGroups, NetworkTrafficPacketCounterEnabled: settings.Extra.FlowPacketCounterEnabled, diff --git a/management/server/http/handlers/accounts/accounts_handler_test.go b/management/server/http/handlers/accounts/accounts_handler_test.go index 1dad33a6f..4b9b79fdc 100644 --- a/management/server/http/handlers/accounts/accounts_handler_test.go +++ b/management/server/http/handlers/accounts/accounts_handler_test.go @@ -15,11 +15,11 @@ import ( "github.com/stretchr/testify/assert" nbcontext "github.com/netbirdio/netbird/management/server/context" - "github.com/netbirdio/netbird/shared/management/http/api" "github.com/netbirdio/netbird/management/server/mock_server" "github.com/netbirdio/netbird/management/server/settings" - "github.com/netbirdio/netbird/shared/management/status" "github.com/netbirdio/netbird/management/server/types" + "github.com/netbirdio/netbird/shared/management/http/api" + "github.com/netbirdio/netbird/shared/management/status" ) func initAccountsTestData(t *testing.T, account *types.Account) *handler { diff --git a/management/server/http/handlers/users/users_handler.go b/management/server/http/handlers/users/users_handler.go index bcd637db4..4e03e5e9b 100644 --- a/management/server/http/handlers/users/users_handler.go +++ b/management/server/http/handlers/users/users_handler.go @@ -9,11 +9,11 @@ import ( log "github.com/sirupsen/logrus" "github.com/netbirdio/netbird/management/server/account" + "github.com/netbirdio/netbird/management/server/types" + "github.com/netbirdio/netbird/management/server/users" "github.com/netbirdio/netbird/shared/management/http/api" "github.com/netbirdio/netbird/shared/management/http/util" "github.com/netbirdio/netbird/shared/management/status" - "github.com/netbirdio/netbird/management/server/types" - "github.com/netbirdio/netbird/management/server/users" nbcontext "github.com/netbirdio/netbird/management/server/context" ) @@ -31,6 +31,8 @@ func AddEndpoints(accountManager account.Manager, router *mux.Router) { router.HandleFunc("/users/{userId}", userHandler.deleteUser).Methods("DELETE", "OPTIONS") router.HandleFunc("/users", userHandler.createUser).Methods("POST", "OPTIONS") router.HandleFunc("/users/{userId}/invite", userHandler.inviteUser).Methods("POST", "OPTIONS") + router.HandleFunc("/users/{userId}/approve", userHandler.approveUser).Methods("POST", "OPTIONS") + router.HandleFunc("/users/{userId}/reject", userHandler.rejectUser).Methods("DELETE", "OPTIONS") addUsersTokensEndpoint(accountManager, router) } @@ -323,17 +325,76 @@ func toUserResponse(user *types.UserInfo, currenUserID string) *api.User { } isCurrent := user.ID == currenUserID + return &api.User{ - Id: user.ID, - Name: user.Name, - Email: user.Email, - Role: user.Role, - AutoGroups: autoGroups, - Status: userStatus, - IsCurrent: &isCurrent, - IsServiceUser: &user.IsServiceUser, - IsBlocked: user.IsBlocked, - LastLogin: &user.LastLogin, - Issued: &user.Issued, + Id: user.ID, + Name: user.Name, + Email: user.Email, + Role: user.Role, + AutoGroups: autoGroups, + Status: userStatus, + IsCurrent: &isCurrent, + IsServiceUser: &user.IsServiceUser, + IsBlocked: user.IsBlocked, + LastLogin: &user.LastLogin, + Issued: &user.Issued, + PendingApproval: user.PendingApproval, } } + +// approveUser is a POST request to approve a user that is pending approval +func (h *handler) approveUser(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost { + util.WriteErrorResponse("wrong HTTP method", http.StatusMethodNotAllowed, w) + return + } + + vars := mux.Vars(r) + targetUserID := vars["userId"] + if len(targetUserID) == 0 { + util.WriteErrorResponse("invalid user ID", http.StatusBadRequest, w) + return + } + + userAuth, err := nbcontext.GetUserAuthFromContext(r.Context()) + if err != nil { + util.WriteError(r.Context(), err, w) + return + } + user, err := h.accountManager.ApproveUser(r.Context(), userAuth.AccountId, userAuth.UserId, targetUserID) + if err != nil { + util.WriteError(r.Context(), err, w) + return + } + + userResponse := toUserResponse(user, userAuth.UserId) + util.WriteJSONObject(r.Context(), w, userResponse) +} + +// rejectUser is a DELETE request to reject a user that is pending approval +func (h *handler) rejectUser(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodDelete { + util.WriteErrorResponse("wrong HTTP method", http.StatusMethodNotAllowed, w) + return + } + + vars := mux.Vars(r) + targetUserID := vars["userId"] + if len(targetUserID) == 0 { + util.WriteErrorResponse("invalid user ID", http.StatusBadRequest, w) + return + } + + userAuth, err := nbcontext.GetUserAuthFromContext(r.Context()) + if err != nil { + util.WriteError(r.Context(), err, w) + return + } + err = h.accountManager.RejectUser(r.Context(), userAuth.AccountId, userAuth.UserId, targetUserID) + if err != nil { + util.WriteError(r.Context(), err, w) + return + } + + util.WriteJSONObject(r.Context(), w, util.EmptyObject{}) +} diff --git a/management/server/http/handlers/users/users_handler_test.go b/management/server/http/handlers/users/users_handler_test.go index f7dc81919..e08004218 100644 --- a/management/server/http/handlers/users/users_handler_test.go +++ b/management/server/http/handlers/users/users_handler_test.go @@ -16,13 +16,13 @@ import ( "github.com/stretchr/testify/require" nbcontext "github.com/netbirdio/netbird/management/server/context" - "github.com/netbirdio/netbird/shared/management/http/api" "github.com/netbirdio/netbird/management/server/mock_server" "github.com/netbirdio/netbird/management/server/permissions/modules" "github.com/netbirdio/netbird/management/server/permissions/roles" - "github.com/netbirdio/netbird/shared/management/status" "github.com/netbirdio/netbird/management/server/types" "github.com/netbirdio/netbird/management/server/users" + "github.com/netbirdio/netbird/shared/management/http/api" + "github.com/netbirdio/netbird/shared/management/status" ) const ( @@ -725,3 +725,133 @@ func stringifyPermissionsKeys(permissions roles.Permissions) map[string]map[stri } return modules } + +func TestApproveUserEndpoint(t *testing.T) { + adminUser := &types.User{ + Id: "admin-user", + Role: types.UserRoleAdmin, + AccountID: existingAccountID, + AutoGroups: []string{}, + } + + pendingUser := &types.User{ + Id: "pending-user", + Role: types.UserRoleUser, + AccountID: existingAccountID, + Blocked: true, + PendingApproval: true, + AutoGroups: []string{}, + } + + tt := []struct { + name string + expectedStatus int + expectedBody bool + requestingUser *types.User + }{ + { + name: "approve user as admin should return 200", + expectedStatus: 200, + expectedBody: true, + requestingUser: adminUser, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + am := &mock_server.MockAccountManager{} + am.ApproveUserFunc = func(ctx context.Context, accountID, initiatorUserID, targetUserID string) (*types.UserInfo, error) { + approvedUserInfo := &types.UserInfo{ + ID: pendingUser.Id, + Email: "pending@example.com", + Name: "Pending User", + Role: string(pendingUser.Role), + AutoGroups: []string{}, + IsServiceUser: false, + IsBlocked: false, + PendingApproval: false, + LastLogin: time.Now(), + Issued: types.UserIssuedAPI, + } + return approvedUserInfo, nil + } + + handler := newHandler(am) + router := mux.NewRouter() + router.HandleFunc("/users/{userId}/approve", handler.approveUser).Methods("POST") + + req, err := http.NewRequest("POST", "/users/pending-user/approve", nil) + require.NoError(t, err) + + userAuth := nbcontext.UserAuth{ + AccountId: existingAccountID, + UserId: tc.requestingUser.Id, + } + ctx := nbcontext.SetUserAuthInContext(req.Context(), userAuth) + req = req.WithContext(ctx) + + rr := httptest.NewRecorder() + router.ServeHTTP(rr, req) + + assert.Equal(t, tc.expectedStatus, rr.Code) + + if tc.expectedBody { + var response api.User + err = json.Unmarshal(rr.Body.Bytes(), &response) + require.NoError(t, err) + assert.Equal(t, "pending-user", response.Id) + assert.False(t, response.IsBlocked) + assert.False(t, response.PendingApproval) + } + }) + } +} + +func TestRejectUserEndpoint(t *testing.T) { + adminUser := &types.User{ + Id: "admin-user", + Role: types.UserRoleAdmin, + AccountID: existingAccountID, + AutoGroups: []string{}, + } + + tt := []struct { + name string + expectedStatus int + requestingUser *types.User + }{ + { + name: "reject user as admin should return 200", + expectedStatus: 200, + requestingUser: adminUser, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + am := &mock_server.MockAccountManager{} + am.RejectUserFunc = func(ctx context.Context, accountID, initiatorUserID, targetUserID string) error { + return nil + } + + handler := newHandler(am) + router := mux.NewRouter() + router.HandleFunc("/users/{userId}/reject", handler.rejectUser).Methods("DELETE") + + req, err := http.NewRequest("DELETE", "/users/pending-user/reject", nil) + require.NoError(t, err) + + userAuth := nbcontext.UserAuth{ + AccountId: existingAccountID, + UserId: tc.requestingUser.Id, + } + ctx := nbcontext.SetUserAuthInContext(req.Context(), userAuth) + req = req.WithContext(ctx) + + rr := httptest.NewRecorder() + router.ServeHTTP(rr, req) + + assert.Equal(t, tc.expectedStatus, rr.Code) + }) + } +} diff --git a/management/server/mock_server/account_mock.go b/management/server/mock_server/account_mock.go index caba58c8b..003385eb5 100644 --- a/management/server/mock_server/account_mock.go +++ b/management/server/mock_server/account_mock.go @@ -95,6 +95,8 @@ type MockAccountManager struct { LoginPeerFunc func(ctx context.Context, login types.PeerLogin) (*nbpeer.Peer, *types.NetworkMap, []*posture.Checks, error) SyncPeerFunc func(ctx context.Context, sync types.PeerSync, accountID string) (*nbpeer.Peer, *types.NetworkMap, []*posture.Checks, error) InviteUserFunc func(ctx context.Context, accountID string, initiatorUserID string, targetUserEmail string) error + ApproveUserFunc func(ctx context.Context, accountID, initiatorUserID, targetUserID string) (*types.UserInfo, error) + RejectUserFunc func(ctx context.Context, accountID, initiatorUserID, targetUserID string) error GetAllConnectedPeersFunc func() (map[string]struct{}, error) HasConnectedChannelFunc func(peerID string) bool GetExternalCacheManagerFunc func() account.ExternalCacheManager @@ -607,6 +609,20 @@ func (am *MockAccountManager) InviteUser(ctx context.Context, accountID string, return status.Errorf(codes.Unimplemented, "method InviteUser is not implemented") } +func (am *MockAccountManager) ApproveUser(ctx context.Context, accountID, initiatorUserID, targetUserID string) (*types.UserInfo, error) { + if am.ApproveUserFunc != nil { + return am.ApproveUserFunc(ctx, accountID, initiatorUserID, targetUserID) + } + return nil, status.Errorf(codes.Unimplemented, "method ApproveUser is not implemented") +} + +func (am *MockAccountManager) RejectUser(ctx context.Context, accountID, initiatorUserID, targetUserID string) error { + if am.RejectUserFunc != nil { + return am.RejectUserFunc(ctx, accountID, initiatorUserID, targetUserID) + } + return status.Errorf(codes.Unimplemented, "method RejectUser is not implemented") +} + // GetNameServerGroup mocks GetNameServerGroup of the AccountManager interface func (am *MockAccountManager) GetNameServerGroup(ctx context.Context, accountID, userID, nsGroupID string) (*nbdns.NameServerGroup, error) { if am.GetNameServerGroupFunc != nil { diff --git a/management/server/peer.go b/management/server/peer.go index 3c2ebe6b6..81f037499 100644 --- a/management/server/peer.go +++ b/management/server/peer.go @@ -489,6 +489,9 @@ func (am *DefaultAccountManager) AddPeer(ctx context.Context, setupKey, userID s if err != nil { return nil, nil, nil, status.Errorf(status.NotFound, "failed adding new peer: user not found") } + if user.PendingApproval { + return nil, nil, nil, status.Errorf(status.PermissionDenied, "user pending approval cannot add peers") + } groupsToAdd = user.AutoGroups opEvent.InitiatorID = userID opEvent.Activity = activity.PeerAddedByUser diff --git a/management/server/peer_test.go b/management/server/peer_test.go index c77bf5e25..31c309430 100644 --- a/management/server/peer_test.go +++ b/management/server/peer_test.go @@ -2383,3 +2383,186 @@ func TestBufferUpdateAccountPeers(t *testing.T) { assert.Less(t, totalNewRuns, totalOldRuns, "Expected new approach to run less than old approach. New runs: %d, Old runs: %d", totalNewRuns, totalOldRuns) t.Logf("New runs: %d, Old runs: %d", totalNewRuns, totalOldRuns) } + +func TestAddPeer_UserPendingApprovalBlocked(t *testing.T) { + manager, err := createManager(t) + if err != nil { + t.Fatal(err) + } + + // Create account + account := newAccountWithId(context.Background(), "test-account", "owner", "", false) + err = manager.Store.SaveAccount(context.Background(), account) + require.NoError(t, err) + + // Create user pending approval + pendingUser := types.NewRegularUser("pending-user") + pendingUser.AccountID = account.Id + pendingUser.Blocked = true + pendingUser.PendingApproval = true + err = manager.Store.SaveUser(context.Background(), pendingUser) + require.NoError(t, err) + + // Try to add peer with pending approval user + key, err := wgtypes.GenerateKey() + require.NoError(t, err) + + peer := &nbpeer.Peer{ + Key: key.PublicKey().String(), + Name: "test-peer", + Meta: nbpeer.PeerSystemMeta{ + Hostname: "test-peer", + OS: "linux", + }, + } + + _, _, _, err = manager.AddPeer(context.Background(), "", pendingUser.Id, peer) + require.Error(t, err) + assert.Contains(t, err.Error(), "user pending approval cannot add peers") +} + +func TestAddPeer_ApprovedUserCanAddPeers(t *testing.T) { + manager, err := createManager(t) + if err != nil { + t.Fatal(err) + } + + // Create account + account := newAccountWithId(context.Background(), "test-account", "owner", "", false) + err = manager.Store.SaveAccount(context.Background(), account) + require.NoError(t, err) + + // Create regular user (not pending approval) + regularUser := types.NewRegularUser("regular-user") + regularUser.AccountID = account.Id + err = manager.Store.SaveUser(context.Background(), regularUser) + require.NoError(t, err) + + // Try to add peer with regular user + key, err := wgtypes.GenerateKey() + require.NoError(t, err) + + peer := &nbpeer.Peer{ + Key: key.PublicKey().String(), + Name: "test-peer", + Meta: nbpeer.PeerSystemMeta{ + Hostname: "test-peer", + OS: "linux", + }, + } + + _, _, _, err = manager.AddPeer(context.Background(), "", regularUser.Id, peer) + require.NoError(t, err, "Regular user should be able to add peers") +} + +func TestLoginPeer_UserPendingApprovalBlocked(t *testing.T) { + manager, err := createManager(t) + if err != nil { + t.Fatal(err) + } + + // Create account + account := newAccountWithId(context.Background(), "test-account", "owner", "", false) + err = manager.Store.SaveAccount(context.Background(), account) + require.NoError(t, err) + + // Create user pending approval + pendingUser := types.NewRegularUser("pending-user") + pendingUser.AccountID = account.Id + pendingUser.Blocked = true + pendingUser.PendingApproval = true + err = manager.Store.SaveUser(context.Background(), pendingUser) + require.NoError(t, err) + + // Create a peer using AddPeer method for the pending user (simulate existing peer) + key, err := wgtypes.GenerateKey() + require.NoError(t, err) + + // Set the user to not be pending initially so peer can be added + pendingUser.Blocked = false + pendingUser.PendingApproval = false + err = manager.Store.SaveUser(context.Background(), pendingUser) + require.NoError(t, err) + + // Add peer using regular flow + newPeer := &nbpeer.Peer{ + Key: key.PublicKey().String(), + Name: "test-peer", + Meta: nbpeer.PeerSystemMeta{ + Hostname: "test-peer", + OS: "linux", + WtVersion: "0.28.0", + }, + } + existingPeer, _, _, err := manager.AddPeer(context.Background(), "", pendingUser.Id, newPeer) + require.NoError(t, err) + + // Now set the user back to pending approval after peer was created + pendingUser.Blocked = true + pendingUser.PendingApproval = true + err = manager.Store.SaveUser(context.Background(), pendingUser) + require.NoError(t, err) + + // Try to login with pending approval user + login := types.PeerLogin{ + WireGuardPubKey: existingPeer.Key, + UserID: pendingUser.Id, + Meta: nbpeer.PeerSystemMeta{ + Hostname: "test-peer", + OS: "linux", + }, + } + + _, _, _, err = manager.LoginPeer(context.Background(), login) + require.Error(t, err) + e, ok := status.FromError(err) + require.True(t, ok, "error is not a gRPC status error") + assert.Equal(t, status.PermissionDenied, e.Type(), "expected PermissionDenied error code") +} + +func TestLoginPeer_ApprovedUserCanLogin(t *testing.T) { + manager, err := createManager(t) + if err != nil { + t.Fatal(err) + } + + // Create account + account := newAccountWithId(context.Background(), "test-account", "owner", "", false) + err = manager.Store.SaveAccount(context.Background(), account) + require.NoError(t, err) + + // Create regular user (not pending approval) + regularUser := types.NewRegularUser("regular-user") + regularUser.AccountID = account.Id + err = manager.Store.SaveUser(context.Background(), regularUser) + require.NoError(t, err) + + // Add peer using regular flow for the regular user + key, err := wgtypes.GenerateKey() + require.NoError(t, err) + + newPeer := &nbpeer.Peer{ + Key: key.PublicKey().String(), + Name: "test-peer", + Meta: nbpeer.PeerSystemMeta{ + Hostname: "test-peer", + OS: "linux", + WtVersion: "0.28.0", + }, + } + existingPeer, _, _, err := manager.AddPeer(context.Background(), "", regularUser.Id, newPeer) + require.NoError(t, err) + + // Try to login with regular user + login := types.PeerLogin{ + WireGuardPubKey: existingPeer.Key, + UserID: regularUser.Id, + Meta: nbpeer.PeerSystemMeta{ + Hostname: "test-peer", + OS: "linux", + }, + } + + _, _, _, err = manager.LoginPeer(context.Background(), login) + require.NoError(t, err, "Regular user should be able to login peers") +} diff --git a/management/server/permissions/manager.go b/management/server/permissions/manager.go index 0ab244243..891fa59bb 100644 --- a/management/server/permissions/manager.go +++ b/management/server/permissions/manager.go @@ -54,10 +54,14 @@ func (m *managerImpl) ValidateUserPermissions( return false, status.NewUserNotFoundError(userID) } - if user.IsBlocked() { + if user.IsBlocked() && !user.PendingApproval { return false, status.NewUserBlockedError() } + if user.IsBlocked() && user.PendingApproval { + return false, status.NewUserPendingApprovalError() + } + if err := m.ValidateAccountAccess(ctx, accountID, user, false); err != nil { return false, err } diff --git a/management/server/types/settings.go b/management/server/types/settings.go index 56c33da3b..b4afb2f5e 100644 --- a/management/server/types/settings.go +++ b/management/server/types/settings.go @@ -83,6 +83,9 @@ type ExtraSettings struct { // PeerApprovalEnabled enables or disables the need for peers bo be approved by an administrator PeerApprovalEnabled bool + // UserApprovalRequired enables or disables the need for users joining via domain matching to be approved by an administrator + UserApprovalRequired bool + // IntegratedValidator is the string enum for the integrated validator type IntegratedValidator string // IntegratedValidatorGroups list of group IDs to be used with integrated approval configurations @@ -99,6 +102,7 @@ type ExtraSettings struct { func (e *ExtraSettings) Copy() *ExtraSettings { return &ExtraSettings{ PeerApprovalEnabled: e.PeerApprovalEnabled, + UserApprovalRequired: e.UserApprovalRequired, IntegratedValidatorGroups: slices.Clone(e.IntegratedValidatorGroups), IntegratedValidator: e.IntegratedValidator, FlowEnabled: e.FlowEnabled, diff --git a/management/server/types/user.go b/management/server/types/user.go index 783fe14da..beb3586df 100644 --- a/management/server/types/user.go +++ b/management/server/types/user.go @@ -64,6 +64,7 @@ type UserInfo struct { NonDeletable bool `json:"non_deletable"` LastLogin time.Time `json:"last_login"` Issued string `json:"issued"` + PendingApproval bool `json:"pending_approval"` IntegrationReference integration_reference.IntegrationReference `json:"-"` } @@ -84,6 +85,8 @@ type User struct { PATsG []PersonalAccessToken `json:"-" gorm:"foreignKey:UserID;references:id;constraint:OnDelete:CASCADE;"` // Blocked indicates whether the user is blocked. Blocked users can't use the system. Blocked bool + // PendingApproval indicates whether the user requires approval before being activated + PendingApproval bool // LastLogin is the last time the user logged in to IdP LastLogin *time.Time // CreatedAt records the time the user was created @@ -141,16 +144,17 @@ func (u *User) ToUserInfo(userData *idp.UserData) (*UserInfo, error) { if userData == nil { return &UserInfo{ - ID: u.Id, - Email: "", - Name: u.ServiceUserName, - Role: string(u.Role), - AutoGroups: u.AutoGroups, - Status: string(UserStatusActive), - IsServiceUser: u.IsServiceUser, - IsBlocked: u.Blocked, - LastLogin: u.GetLastLogin(), - Issued: u.Issued, + ID: u.Id, + Email: "", + Name: u.ServiceUserName, + Role: string(u.Role), + AutoGroups: u.AutoGroups, + Status: string(UserStatusActive), + IsServiceUser: u.IsServiceUser, + IsBlocked: u.Blocked, + LastLogin: u.GetLastLogin(), + Issued: u.Issued, + PendingApproval: u.PendingApproval, }, nil } if userData.ID != u.Id { @@ -163,16 +167,17 @@ func (u *User) ToUserInfo(userData *idp.UserData) (*UserInfo, error) { } return &UserInfo{ - ID: u.Id, - Email: userData.Email, - Name: userData.Name, - Role: string(u.Role), - AutoGroups: autoGroups, - Status: string(userStatus), - IsServiceUser: u.IsServiceUser, - IsBlocked: u.Blocked, - LastLogin: u.GetLastLogin(), - Issued: u.Issued, + ID: u.Id, + Email: userData.Email, + Name: userData.Name, + Role: string(u.Role), + AutoGroups: autoGroups, + Status: string(userStatus), + IsServiceUser: u.IsServiceUser, + IsBlocked: u.Blocked, + LastLogin: u.GetLastLogin(), + Issued: u.Issued, + PendingApproval: u.PendingApproval, }, nil } @@ -194,6 +199,7 @@ func (u *User) Copy() *User { ServiceUserName: u.ServiceUserName, PATs: pats, Blocked: u.Blocked, + PendingApproval: u.PendingApproval, LastLogin: u.LastLogin, CreatedAt: u.CreatedAt, Issued: u.Issued, diff --git a/management/server/user.go b/management/server/user.go index e5a4dbcea..04b2ce2d0 100644 --- a/management/server/user.go +++ b/management/server/user.go @@ -1207,3 +1207,77 @@ func (am *DefaultAccountManager) GetCurrentUserInfo(ctx context.Context, userAut return userWithPermissions, nil } + +// ApproveUser approves a user that is pending approval +func (am *DefaultAccountManager) ApproveUser(ctx context.Context, accountID, initiatorUserID, targetUserID string) (*types.UserInfo, error) { + allowed, err := am.permissionsManager.ValidateUserPermissions(ctx, accountID, initiatorUserID, modules.Users, operations.Update) + if err != nil { + return nil, status.NewPermissionValidationError(err) + } + if !allowed { + return nil, status.NewPermissionDeniedError() + } + + user, err := am.Store.GetUserByUserID(ctx, store.LockingStrengthNone, targetUserID) + if err != nil { + return nil, err + } + + if user.AccountID != accountID { + return nil, status.NewUserNotFoundError(targetUserID) + } + + if !user.PendingApproval { + return nil, status.Errorf(status.InvalidArgument, "user %s is not pending approval", targetUserID) + } + + user.Blocked = false + user.PendingApproval = false + + err = am.Store.SaveUser(ctx, user) + if err != nil { + return nil, err + } + + am.StoreEvent(ctx, initiatorUserID, targetUserID, accountID, activity.UserApproved, nil) + + userInfo, err := am.getUserInfo(ctx, user, accountID) + if err != nil { + return nil, err + } + + return userInfo, nil +} + +// RejectUser rejects a user that is pending approval by deleting them +func (am *DefaultAccountManager) RejectUser(ctx context.Context, accountID, initiatorUserID, targetUserID string) error { + allowed, err := am.permissionsManager.ValidateUserPermissions(ctx, accountID, initiatorUserID, modules.Users, operations.Delete) + if err != nil { + return status.NewPermissionValidationError(err) + } + if !allowed { + return status.NewPermissionDeniedError() + } + + user, err := am.Store.GetUserByUserID(ctx, store.LockingStrengthNone, targetUserID) + if err != nil { + return err + } + + if user.AccountID != accountID { + return status.NewUserNotFoundError(targetUserID) + } + + if !user.PendingApproval { + return status.Errorf(status.InvalidArgument, "user %s is not pending approval", targetUserID) + } + + err = am.DeleteUser(ctx, accountID, initiatorUserID, targetUserID) + if err != nil { + return err + } + + am.StoreEvent(ctx, initiatorUserID, targetUserID, accountID, activity.UserRejected, nil) + + return nil +} diff --git a/management/server/user_test.go b/management/server/user_test.go index 8ab0c1565..9638559f9 100644 --- a/management/server/user_test.go +++ b/management/server/user_test.go @@ -1746,3 +1746,117 @@ func mergeRolePermissions(role roles.RolePermissions) roles.Permissions { return permissions } + +func TestApproveUser(t *testing.T) { + manager, err := createManager(t) + if err != nil { + t.Fatal(err) + } + + // Create account with admin and pending approval user + account := newAccountWithId(context.Background(), "account-1", "admin-user", "example.com", false) + err = manager.Store.SaveAccount(context.Background(), account) + require.NoError(t, err) + + // Create admin user + adminUser := types.NewAdminUser("admin-user") + adminUser.AccountID = account.Id + err = manager.Store.SaveUser(context.Background(), adminUser) + require.NoError(t, err) + + // Create user pending approval + pendingUser := types.NewRegularUser("pending-user") + pendingUser.AccountID = account.Id + pendingUser.Blocked = true + pendingUser.PendingApproval = true + err = manager.Store.SaveUser(context.Background(), pendingUser) + require.NoError(t, err) + + // Test successful approval + approvedUser, err := manager.ApproveUser(context.Background(), account.Id, adminUser.Id, pendingUser.Id) + require.NoError(t, err) + assert.False(t, approvedUser.IsBlocked) + assert.False(t, approvedUser.PendingApproval) + + // Verify user is updated in store + updatedUser, err := manager.Store.GetUserByUserID(context.Background(), store.LockingStrengthNone, pendingUser.Id) + require.NoError(t, err) + assert.False(t, updatedUser.Blocked) + assert.False(t, updatedUser.PendingApproval) + + // Test approval of non-pending user should fail + _, err = manager.ApproveUser(context.Background(), account.Id, adminUser.Id, pendingUser.Id) + require.Error(t, err) + assert.Contains(t, err.Error(), "not pending approval") + + // Test approval by non-admin should fail + regularUser := types.NewRegularUser("regular-user") + regularUser.AccountID = account.Id + err = manager.Store.SaveUser(context.Background(), regularUser) + require.NoError(t, err) + + pendingUser2 := types.NewRegularUser("pending-user-2") + pendingUser2.AccountID = account.Id + pendingUser2.Blocked = true + pendingUser2.PendingApproval = true + err = manager.Store.SaveUser(context.Background(), pendingUser2) + require.NoError(t, err) + + _, err = manager.ApproveUser(context.Background(), account.Id, regularUser.Id, pendingUser2.Id) + require.Error(t, err) +} + +func TestRejectUser(t *testing.T) { + manager, err := createManager(t) + if err != nil { + t.Fatal(err) + } + + // Create account with admin and pending approval user + account := newAccountWithId(context.Background(), "account-1", "admin-user", "example.com", false) + err = manager.Store.SaveAccount(context.Background(), account) + require.NoError(t, err) + + // Create admin user + adminUser := types.NewAdminUser("admin-user") + adminUser.AccountID = account.Id + err = manager.Store.SaveUser(context.Background(), adminUser) + require.NoError(t, err) + + // Create user pending approval + pendingUser := types.NewRegularUser("pending-user") + pendingUser.AccountID = account.Id + pendingUser.Blocked = true + pendingUser.PendingApproval = true + err = manager.Store.SaveUser(context.Background(), pendingUser) + require.NoError(t, err) + + // Test successful rejection + err = manager.RejectUser(context.Background(), account.Id, adminUser.Id, pendingUser.Id) + require.NoError(t, err) + + // Verify user is deleted from store + _, err = manager.Store.GetUserByUserID(context.Background(), store.LockingStrengthNone, pendingUser.Id) + require.Error(t, err) + + // Test rejection of non-pending user should fail + regularUser := types.NewRegularUser("regular-user") + regularUser.AccountID = account.Id + err = manager.Store.SaveUser(context.Background(), regularUser) + require.NoError(t, err) + + err = manager.RejectUser(context.Background(), account.Id, adminUser.Id, regularUser.Id) + require.Error(t, err) + assert.Contains(t, err.Error(), "not pending approval") + + // Test rejection by non-admin should fail + pendingUser2 := types.NewRegularUser("pending-user-2") + pendingUser2.AccountID = account.Id + pendingUser2.Blocked = true + pendingUser2.PendingApproval = true + err = manager.Store.SaveUser(context.Background(), pendingUser2) + require.NoError(t, err) + + err = manager.RejectUser(context.Background(), account.Id, regularUser.Id, pendingUser2.Id) + require.Error(t, err) +} diff --git a/shared/management/http/api/openapi.yml b/shared/management/http/api/openapi.yml index cf4b6d625..9a531b2ff 100644 --- a/shared/management/http/api/openapi.yml +++ b/shared/management/http/api/openapi.yml @@ -158,6 +158,10 @@ components: description: (Cloud only) Enables or disables peer approval globally. If enabled, all peers added will be in pending state until approved by an admin. type: boolean example: true + user_approval_required: + description: Enables manual approval for new users joining via domain matching. When enabled, users are blocked with pending approval status until explicitly approved by an admin. + type: boolean + example: false network_traffic_logs_enabled: description: Enables or disables network traffic logging. If enabled, all network traffic events from peers will be stored. type: boolean @@ -174,6 +178,7 @@ components: example: true required: - peer_approval_enabled + - user_approval_required - network_traffic_logs_enabled - network_traffic_logs_groups - network_traffic_packet_counter_enabled @@ -235,6 +240,10 @@ components: description: Is true if this user is blocked. Blocked users can't use the system type: boolean example: false + pending_approval: + description: Is true if this user requires approval before being activated. Only applicable for users joining via domain matching when user_approval_required is enabled. + type: boolean + example: false issued: description: How user was issued by API or Integration type: string @@ -249,6 +258,7 @@ components: - auto_groups - status - is_blocked + - pending_approval UserPermissions: type: object properties: @@ -2544,6 +2554,63 @@ paths: "$ref": "#/components/responses/forbidden" '500': "$ref": "#/components/responses/internal_error" + /api/users/{userId}/approve: + post: + summary: Approve user + description: Approve a user that is pending approval + tags: [ Users ] + security: + - BearerAuth: [ ] + - TokenAuth: [ ] + parameters: + - in: path + name: userId + required: true + schema: + type: string + description: The unique identifier of a user + responses: + '200': + description: Returns the approved user + content: + application/json: + schema: + "$ref": "#/components/schemas/User" + '400': + "$ref": "#/components/responses/bad_request" + '401': + "$ref": "#/components/responses/requires_authentication" + '403': + "$ref": "#/components/responses/forbidden" + '500': + "$ref": "#/components/responses/internal_error" + /api/users/{userId}/reject: + delete: + summary: Reject user + description: Reject a user that is pending approval by removing them from the account + tags: [ Users ] + security: + - BearerAuth: [ ] + - TokenAuth: [ ] + parameters: + - in: path + name: userId + required: true + schema: + type: string + description: The unique identifier of a user + responses: + '200': + description: User rejected successfully + content: {} + '400': + "$ref": "#/components/responses/bad_request" + '401': + "$ref": "#/components/responses/requires_authentication" + '403': + "$ref": "#/components/responses/forbidden" + '500': + "$ref": "#/components/responses/internal_error" /api/users/current: get: summary: Retrieve current user diff --git a/shared/management/http/api/types.gen.go b/shared/management/http/api/types.gen.go index cffc9e735..28b89633c 100644 --- a/shared/management/http/api/types.gen.go +++ b/shared/management/http/api/types.gen.go @@ -268,6 +268,9 @@ type AccountExtraSettings struct { // PeerApprovalEnabled (Cloud only) Enables or disables peer approval globally. If enabled, all peers added will be in pending state until approved by an admin. PeerApprovalEnabled bool `json:"peer_approval_enabled"` + + // UserApprovalRequired Enables manual approval for new users joining via domain matching. When enabled, users are blocked with pending approval status until explicitly approved by an admin. + UserApprovalRequired bool `json:"user_approval_required"` } // AccountOnboarding defines model for AccountOnboarding. @@ -1015,8 +1018,6 @@ type OSVersionCheck struct { // Peer defines model for Peer. type Peer struct { - // CreatedAt Peer creation date (UTC) - CreatedAt time.Time `json:"created_at"` // ApprovalRequired (Cloud only) Indicates whether peer needs approval ApprovalRequired bool `json:"approval_required"` @@ -1032,6 +1033,9 @@ type Peer struct { // CountryCode 2-letter ISO 3166-1 alpha-2 code that represents the country CountryCode CountryCode `json:"country_code"` + // CreatedAt Peer creation date (UTC) + CreatedAt time.Time `json:"created_at"` + // DnsLabel Peer's DNS label is the parsed peer name for domain resolution. It is used to form an FQDN by appending the account's domain to the peer label. e.g. peer-dns-label.netbird.cloud DnsLabel string `json:"dns_label"` @@ -1098,8 +1102,6 @@ type Peer struct { // PeerBatch defines model for PeerBatch. type PeerBatch struct { - // CreatedAt Peer creation date (UTC) - CreatedAt time.Time `json:"created_at"` // AccessiblePeersCount Number of accessible peers AccessiblePeersCount int `json:"accessible_peers_count"` @@ -1118,6 +1120,9 @@ type PeerBatch struct { // CountryCode 2-letter ISO 3166-1 alpha-2 code that represents the country CountryCode CountryCode `json:"country_code"` + // CreatedAt Peer creation date (UTC) + CreatedAt time.Time `json:"created_at"` + // DnsLabel Peer's DNS label is the parsed peer name for domain resolution. It is used to form an FQDN by appending the account's domain to the peer label. e.g. peer-dns-label.netbird.cloud DnsLabel string `json:"dns_label"` @@ -1774,8 +1779,11 @@ type User struct { LastLogin *time.Time `json:"last_login,omitempty"` // Name User's name from idp provider - Name string `json:"name"` - Permissions *UserPermissions `json:"permissions,omitempty"` + Name string `json:"name"` + + // PendingApproval Is true if this user requires approval before being activated. Only applicable for users joining via domain matching when user_approval_required is enabled. + PendingApproval bool `json:"pending_approval"` + Permissions *UserPermissions `json:"permissions,omitempty"` // Role User's NetBird account role Role string `json:"role"` diff --git a/shared/management/status/error.go b/shared/management/status/error.go index 52d27b062..1e914babb 100644 --- a/shared/management/status/error.go +++ b/shared/management/status/error.go @@ -113,6 +113,11 @@ func NewUserBlockedError() error { return Errorf(PermissionDenied, "user is blocked") } +// NewUserPendingApprovalError creates a new Error with PermissionDenied type for a blocked user pending approval +func NewUserPendingApprovalError() error { + return Errorf(PermissionDenied, "user is pending approval") +} + // NewPeerNotRegisteredError creates a new Error with Unauthenticated type unregistered peer func NewPeerNotRegisteredError() error { return Errorf(Unauthenticated, "peer is not registered")