From 7e4450776a11ada90c5a306ff00016cbfa963ad7 Mon Sep 17 00:00:00 2001 From: bcmmbaga Date: Wed, 15 Nov 2023 12:11:56 +0300 Subject: [PATCH 1/8] Add non-deletable flag for service users --- management/server/account.go | 1 + management/server/user.go | 7 +++++++ 2 files changed, 8 insertions(+) diff --git a/management/server/account.go b/management/server/account.go index 25b03466869..c8d4a1943c2 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -211,6 +211,7 @@ type UserInfo struct { Status string `json:"-"` IsServiceUser bool `json:"is_service_user"` IsBlocked bool `json:"is_blocked"` + NonDeletable bool `json:"non_deletable"` LastLogin time.Time `json:"last_login"` Issued string `json:"issued"` IntegrationReference IntegrationReference `json:"-"` diff --git a/management/server/user.go b/management/server/user.go index fd92bf35fd1..cbc6f684167 100644 --- a/management/server/user.go +++ b/management/server/user.go @@ -69,6 +69,8 @@ type User struct { AccountID string `json:"-" gorm:"index"` Role UserRole IsServiceUser bool + // NonDeletable indicates whether the service user can be deleted + NonDeletable bool // ServiceUserName is only set if IsServiceUser is true ServiceUserName string // AutoGroups is a list of Group IDs to auto-assign to peers registered by this user @@ -164,6 +166,7 @@ func (u *User) Copy() *User { LastLogin: u.LastLogin, Issued: u.Issued, IntegrationReference: u.IntegrationReference, + NonDeletable: u.NonDeletable, } } @@ -420,6 +423,10 @@ func (am *DefaultAccountManager) DeleteUser(accountID, initiatorUserID string, t // handle service user first and exit, no need to fetch extra data from IDP, etc if targetUser.IsServiceUser { + if targetUser.NonDeletable { + return status.Errorf(status.PermissionDenied, "service user is marked as non-deletable") + } + am.deleteServiceUser(account, initiatorUserID, targetUser) return am.Store.SaveAccount(account) } From b53313fa3c81e95482149e283664155db1945f52 Mon Sep 17 00:00:00 2001 From: bcmmbaga Date: Wed, 15 Nov 2023 12:42:16 +0300 Subject: [PATCH 2/8] fix non deletable service user created as deletable --- management/server/user.go | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/management/server/user.go b/management/server/user.go index cbc6f684167..0ce58859309 100644 --- a/management/server/user.go +++ b/management/server/user.go @@ -160,22 +160,23 @@ func (u *User) Copy() *User { Role: u.Role, AutoGroups: autoGroups, IsServiceUser: u.IsServiceUser, + NonDeletable: u.NonDeletable, ServiceUserName: u.ServiceUserName, PATs: pats, Blocked: u.Blocked, LastLogin: u.LastLogin, Issued: u.Issued, IntegrationReference: u.IntegrationReference, - NonDeletable: u.NonDeletable, } } // NewUser creates a new user -func NewUser(id string, role UserRole, isServiceUser bool, serviceUserName string, autoGroups []string, issued string) *User { +func NewUser(id string, role UserRole, isServiceUser bool, nonDeletable bool, serviceUserName string, autoGroups []string, issued string) *User { return &User{ Id: id, Role: role, IsServiceUser: isServiceUser, + NonDeletable: nonDeletable, ServiceUserName: serviceUserName, AutoGroups: autoGroups, Issued: issued, @@ -184,16 +185,16 @@ func NewUser(id string, role UserRole, isServiceUser bool, serviceUserName strin // NewRegularUser creates a new user with role UserRoleUser func NewRegularUser(id string) *User { - return NewUser(id, UserRoleUser, false, "", []string{}, UserIssuedAPI) + return NewUser(id, UserRoleUser, false, false, "", []string{}, UserIssuedAPI) } // NewAdminUser creates a new user with role UserRoleAdmin func NewAdminUser(id string) *User { - return NewUser(id, UserRoleAdmin, false, "", []string{}, UserIssuedAPI) + return NewUser(id, UserRoleAdmin, false, false, "", []string{}, UserIssuedAPI) } // createServiceUser creates a new service user under the given account. -func (am *DefaultAccountManager) createServiceUser(accountID string, initiatorUserID string, role UserRole, serviceUserName string, autoGroups []string) (*UserInfo, error) { +func (am *DefaultAccountManager) createServiceUser(accountID string, initiatorUserID string, role UserRole, serviceUserName string, nonDeletable bool, autoGroups []string) (*UserInfo, error) { unlock := am.Store.AcquireAccountLock(accountID) defer unlock() @@ -211,7 +212,7 @@ func (am *DefaultAccountManager) createServiceUser(accountID string, initiatorUs } newUserID := uuid.New().String() - newUser := NewUser(newUserID, role, true, serviceUserName, autoGroups, UserIssuedAPI) + newUser := NewUser(newUserID, role, true, nonDeletable, serviceUserName, autoGroups, UserIssuedAPI) log.Debugf("New User: %v", newUser) account.Users[newUserID] = newUser @@ -239,7 +240,7 @@ func (am *DefaultAccountManager) createServiceUser(accountID string, initiatorUs // CreateUser creates a new user under the given account. Effectively this is a user invite. func (am *DefaultAccountManager) CreateUser(accountID, userID string, user *UserInfo) (*UserInfo, error) { if user.IsServiceUser { - return am.createServiceUser(accountID, userID, StrRoleToUserRole(user.Role), user.Name, user.AutoGroups) + return am.createServiceUser(accountID, userID, StrRoleToUserRole(user.Role), user.Name, user.NonDeletable, user.AutoGroups) } return am.inviteNewUser(accountID, userID, user) } From 4f4884705428e0ed5200c7a3f8ee08da8be28885 Mon Sep 17 00:00:00 2001 From: bcmmbaga Date: Wed, 15 Nov 2023 13:24:01 +0300 Subject: [PATCH 3/8] Exclude non deletable service users in service users api response --- management/server/http/users_handler.go | 2 +- management/server/user.go | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/management/server/http/users_handler.go b/management/server/http/users_handler.go index 0441e8cc054..5c2c540c6e7 100644 --- a/management/server/http/users_handler.go +++ b/management/server/http/users_handler.go @@ -207,7 +207,7 @@ func (h *UsersHandler) GetAllUsers(w http.ResponseWriter, r *http.Request) { util.WriteError(status.Errorf(status.InvalidArgument, "invalid service_user query parameter"), w) return } - if includeServiceUser == r.IsServiceUser { + if includeServiceUser == r.IsServiceUser && !r.NonDeletable { users = append(users, toUserResponse(r, claims.UserId)) } } diff --git a/management/server/user.go b/management/server/user.go index 0ce58859309..e2581770350 100644 --- a/management/server/user.go +++ b/management/server/user.go @@ -963,6 +963,7 @@ func (am *DefaultAccountManager) GetUsersFromAccount(accountID, userID string) ( AutoGroups: localUser.AutoGroups, Status: string(UserStatusActive), IsServiceUser: localUser.IsServiceUser, + NonDeletable: localUser.NonDeletable, } } userInfos = append(userInfos, info) From b54a493ee64f1d9fd2228d2b1c18cc8037c917c3 Mon Sep 17 00:00:00 2001 From: bcmmbaga Date: Wed, 15 Nov 2023 13:42:30 +0300 Subject: [PATCH 4/8] Fix broken tests --- management/server/user_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/management/server/user_test.go b/management/server/user_test.go index 818a267d4ae..ef9814bcd98 100644 --- a/management/server/user_test.go +++ b/management/server/user_test.go @@ -332,7 +332,7 @@ func TestUser_CreateServiceUser(t *testing.T) { eventStore: &activity.InMemoryEventStore{}, } - user, err := am.createServiceUser(mockAccountID, mockUserID, mockRole, mockServiceUserName, []string{"group1", "group2"}) + user, err := am.createServiceUser(mockAccountID, mockUserID, mockRole, mockServiceUserName, false, []string{"group1", "group2"}) if err != nil { t.Fatalf("Error when creating service user: %s", err) } From 8cac7afb9b38c075e910d833fff06369cfbe498d Mon Sep 17 00:00:00 2001 From: bcmmbaga Date: Wed, 15 Nov 2023 16:58:33 +0300 Subject: [PATCH 5/8] Add test for non deletable service user --- management/server/user_test.go | 75 ++++++++++++++++++++++++---------- 1 file changed, 53 insertions(+), 22 deletions(-) diff --git a/management/server/user_test.go b/management/server/user_test.go index ef9814bcd98..e628981b0b1 100644 --- a/management/server/user_test.go +++ b/management/server/user_test.go @@ -413,31 +413,62 @@ func TestUser_CreateUser_RegularUser(t *testing.T) { } func TestUser_DeleteUser_ServiceUser(t *testing.T) { - store := newStore(t) - account := newAccountWithId(mockAccountID, mockUserID, "") - account.Users[mockServiceUserID] = &User{ - Id: mockServiceUserID, - IsServiceUser: true, - ServiceUserName: mockServiceUserName, - } - - err := store.SaveAccount(account) - if err != nil { - t.Fatalf("Error when saving account: %s", err) - } - - am := DefaultAccountManager{ - Store: store, - eventStore: &activity.InMemoryEventStore{}, + tests := []struct { + name string + serviceUser *User + assertErrFunc assert.ErrorAssertionFunc + assertErrMessage string + }{ + { + name: "Can delete service user", + serviceUser: &User{ + Id: mockServiceUserID, + IsServiceUser: true, + ServiceUserName: mockServiceUserName, + }, + assertErrFunc: assert.NoError, + }, + { + name: "Cannot delete non-deletable service user", + serviceUser: &User{ + Id: mockServiceUserID, + IsServiceUser: true, + ServiceUserName: mockServiceUserName, + NonDeletable: true, + }, + assertErrFunc: assert.Error, + assertErrMessage: "service user is marked as non-deletable", + }, } - err = am.DeleteUser(mockAccountID, mockUserID, mockServiceUserID) - if err != nil { - t.Fatalf("Error when deleting user: %s", err) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + store := newStore(t) + account := newAccountWithId(mockAccountID, mockUserID, "") + account.Users[mockServiceUserID] = tt.serviceUser + + err := store.SaveAccount(account) + if err != nil { + t.Fatalf("Error when saving account: %s", err) + } + + am := DefaultAccountManager{ + Store: store, + eventStore: &activity.InMemoryEventStore{}, + } + + err = am.DeleteUser(mockAccountID, mockUserID, mockServiceUserID) + tt.assertErrFunc(t, err, tt.assertErrMessage) + + if err != nil { + assert.Equal(t, 2, len(store.Accounts[mockAccountID].Users)) + assert.NotNil(t, store.Accounts[mockAccountID].Users[mockServiceUserID]) + } else { + assert.Equal(t, 1, len(store.Accounts[mockAccountID].Users)) + assert.Nil(t, store.Accounts[mockAccountID].Users[mockServiceUserID]) + } + }) } - - assert.Equal(t, 1, len(store.Accounts[mockAccountID].Users)) - assert.Nil(t, store.Accounts[mockAccountID].Users[mockServiceUserID]) } func TestUser_DeleteUser_SelfDelete(t *testing.T) { From d23debd4ae947d9ffe65c77f5f30eeec5a50beae Mon Sep 17 00:00:00 2001 From: bcmmbaga Date: Wed, 15 Nov 2023 17:47:55 +0300 Subject: [PATCH 6/8] Add handling for non-deletable service users in tests --- management/server/http/users_handler_test.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/management/server/http/users_handler_test.go b/management/server/http/users_handler_test.go index b4d449be385..ff886ca9fda 100644 --- a/management/server/http/users_handler_test.go +++ b/management/server/http/users_handler_test.go @@ -20,8 +20,9 @@ import ( ) const ( - serviceUserID = "serviceUserID" - regularUserID = "regularUserID" + serviceUserID = "serviceUserID" + nonDeletableServiceUserID = "nonDeletableServiceUserID" + regularUserID = "regularUserID" ) var usersTestAccount = &server.Account{ @@ -49,6 +50,13 @@ var usersTestAccount = &server.Account{ AutoGroups: []string{"group_1"}, Issued: server.UserIssuedAPI, }, + nonDeletableServiceUserID: { + Id: serviceUserID, + Role: "admin", + IsServiceUser: true, + NonDeletable: true, + Issued: server.UserIssuedIntegration, + }, }, } @@ -67,6 +75,7 @@ func initUsersTestData() *UsersHandler { Name: "", Email: "", IsServiceUser: v.IsServiceUser, + NonDeletable: v.NonDeletable, Issued: v.Issued, }) } From bfbeabdcd444aea8bbae0a73ac76b1970d918af2 Mon Sep 17 00:00:00 2001 From: bcmmbaga Date: Wed, 15 Nov 2023 17:48:31 +0300 Subject: [PATCH 7/8] Remove non-deletable service users when fetching all users --- management/server/http/users_handler.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/management/server/http/users_handler.go b/management/server/http/users_handler.go index 5c2c540c6e7..303aa4552fa 100644 --- a/management/server/http/users_handler.go +++ b/management/server/http/users_handler.go @@ -198,7 +198,9 @@ func (h *UsersHandler) GetAllUsers(w http.ResponseWriter, r *http.Request) { users := make([]*api.User, 0) for _, r := range data { if serviceUser == "" { - users = append(users, toUserResponse(r, claims.UserId)) + if !r.NonDeletable { + users = append(users, toUserResponse(r, claims.UserId)) + } continue } includeServiceUser, err := strconv.ParseBool(serviceUser) From 84b5df3e6ed251242b5af34d90f5aca59401c90e Mon Sep 17 00:00:00 2001 From: bcmmbaga Date: Wed, 15 Nov 2023 18:06:32 +0300 Subject: [PATCH 8/8] Ensure non-deletable users are filtered out when fetching all user data --- management/server/http/users_handler.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/management/server/http/users_handler.go b/management/server/http/users_handler.go index 303aa4552fa..e2bf77de65b 100644 --- a/management/server/http/users_handler.go +++ b/management/server/http/users_handler.go @@ -197,19 +197,21 @@ func (h *UsersHandler) GetAllUsers(w http.ResponseWriter, r *http.Request) { users := make([]*api.User, 0) for _, r := range data { + if r.NonDeletable { + continue + } if serviceUser == "" { - if !r.NonDeletable { - users = append(users, toUserResponse(r, claims.UserId)) - } + users = append(users, toUserResponse(r, claims.UserId)) continue } + includeServiceUser, err := strconv.ParseBool(serviceUser) log.Debugf("Should include service user: %v", includeServiceUser) if err != nil { util.WriteError(status.Errorf(status.InvalidArgument, "invalid service_user query parameter"), w) return } - if includeServiceUser == r.IsServiceUser && !r.NonDeletable { + if includeServiceUser == r.IsServiceUser { users = append(users, toUserResponse(r, claims.UserId)) } }