From f91daedd265fed64ec894ed1a0e027b6e2cabba6 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Thu, 6 Apr 2023 16:53:22 +0200 Subject: [PATCH 01/13] extending user endpoints to handle service users + backend --- management/server/account.go | 25 +- management/server/http/handler.go | 1 + .../server/http/middleware/access_control.go | 4 +- management/server/http/users_handler.go | 81 +++++- management/server/user.go | 267 +++++++++++++----- 5 files changed, 288 insertions(+), 90 deletions(-) diff --git a/management/server/account.go b/management/server/account.go index 78c9237b80f..b442e9b39a7 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -50,14 +50,16 @@ type AccountManager interface { autoGroups []string, usageLimit int, userID string) (*SetupKey, error) SaveSetupKey(accountID string, key *SetupKey, userID string) (*SetupKey, error) CreateUser(accountID, userID string, key *UserInfo) (*UserInfo, error) + DeleteUser(accountID, executingUserID string, targetUserID string) error ListSetupKeys(accountID, userID string) ([]*SetupKey, error) SaveUser(accountID, userID string, update *User) (*UserInfo, error) GetSetupKey(accountID, userID, keyID string) (*SetupKey, error) GetAccountByUserOrAccountID(userID, accountID, domain string) (*Account, error) + GetAccountByUserID(userID string) (*Account, error) GetAccountFromToken(claims jwtclaims.AuthorizationClaims) (*Account, *User, error) GetAccountFromPAT(pat string) (*Account, *User, *PersonalAccessToken, error) MarkPATUsed(tokenID string) error - IsUserAdmin(claims jwtclaims.AuthorizationClaims) (bool, error) + IsUserAdmin(userID string) (bool, error) AccountExists(accountId string) (*bool, error) GetPeerByKey(peerKey string) (*Peer, error) GetPeers(accountID, userID string) ([]*Peer, error) @@ -171,12 +173,13 @@ type Account struct { } type UserInfo struct { - ID string `json:"id"` - Email string `json:"email"` - Name string `json:"name"` - Role string `json:"role"` - AutoGroups []string `json:"auto_groups"` - Status string `json:"-"` + ID string `json:"id"` + Email string `json:"email"` + Name string `json:"name"` + Role string `json:"role"` + AutoGroups []string `json:"auto_groups"` + Status string `json:"-"` + IsServiceUser bool `json:"is_service_user"` } // getRoutesToSync returns the enabled routes for the peer ID and the routes @@ -981,10 +984,10 @@ func (am *DefaultAccountManager) lookupCache(accountUsers map[string]struct{}, a if reload { // reload cache once avoiding loops - data, err = am.refreshCache(accountID) - if err != nil { - return nil, err - } + // data, err = am.refreshCache(accountID) + // if err != nil { + // return nil, err + // } } return data, err diff --git a/management/server/http/handler.go b/management/server/http/handler.go index d8117a43684..3ca76d8c9ca 100644 --- a/management/server/http/handler.go +++ b/management/server/http/handler.go @@ -111,6 +111,7 @@ func (apiHandler *apiHandler) addUsersEndpoint() { userHandler := NewUsersHandler(apiHandler.AccountManager, apiHandler.AuthCfg) apiHandler.Router.HandleFunc("/users", userHandler.GetAllUsers).Methods("GET", "OPTIONS") apiHandler.Router.HandleFunc("/users/{id}", userHandler.UpdateUser).Methods("PUT", "OPTIONS") + apiHandler.Router.HandleFunc("/users/{id}", userHandler.DeleteUser).Methods("DELETE", "OPTIONS") apiHandler.Router.HandleFunc("/users", userHandler.CreateUser).Methods("POST", "OPTIONS") } diff --git a/management/server/http/middleware/access_control.go b/management/server/http/middleware/access_control.go index 5f8389dfa24..c4f79f8cb0e 100644 --- a/management/server/http/middleware/access_control.go +++ b/management/server/http/middleware/access_control.go @@ -12,7 +12,7 @@ import ( "github.com/netbirdio/netbird/management/server/jwtclaims" ) -type IsUserAdminFunc func(claims jwtclaims.AuthorizationClaims) (bool, error) +type IsUserAdminFunc func(userID string) (bool, error) // AccessControl middleware to restrict to make POST/PUT/DELETE requests by admin only type AccessControl struct { @@ -37,7 +37,7 @@ func (a *AccessControl) Handler(h http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { claims := a.claimsExtract.FromRequestContext(r) - ok, err := a.isUserAdmin(claims) + ok, err := a.isUserAdmin(claims.UserId) if err != nil { util.WriteError(status.Errorf(status.Unauthorized, "invalid JWT"), w) return diff --git a/management/server/http/users_handler.go b/management/server/http/users_handler.go index 5dae50decfb..bdbefe39f13 100644 --- a/management/server/http/users_handler.go +++ b/management/server/http/users_handler.go @@ -3,8 +3,10 @@ package http import ( "encoding/json" "net/http" + "strconv" "github.com/gorilla/mux" + log "github.com/sirupsen/logrus" "github.com/netbirdio/netbird/management/server/http/api" "github.com/netbirdio/netbird/management/server/http/util" @@ -77,6 +79,36 @@ func (h *UsersHandler) UpdateUser(w http.ResponseWriter, r *http.Request) { util.WriteJSONObject(w, toUserResponse(newUser, claims.UserId)) } +// DeleteUser is a DELETE request to delete a user (only works for service users right now) +func (h *UsersHandler) DeleteUser(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodDelete { + util.WriteErrorResponse("wrong HTTP method", http.StatusMethodNotAllowed, w) + return + } + + claims := h.claimsExtractor.FromRequestContext(r) + account, user, err := h.accountManager.GetAccountFromToken(claims) + if err != nil { + util.WriteError(err, w) + return + } + + vars := mux.Vars(r) + targetUserID := vars["id"] + if len(targetUserID) == 0 { + util.WriteError(status.Errorf(status.InvalidArgument, "invalid user ID"), w) + return + } + + err = h.accountManager.DeleteUser(account.Id, user.Id, targetUserID) + if err != nil { + util.WriteError(err, w) + return + } + + util.WriteJSONObject(w, emptyObject{}) +} + // CreateUser creates a User in the system with a status "invited" (effectively this is a user invite). func (h *UsersHandler) CreateUser(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodPost { @@ -103,11 +135,17 @@ func (h *UsersHandler) CreateUser(w http.ResponseWriter, r *http.Request) { return } + email := "" + if req.Email != nil { + email = *req.Email + } + newUser, err := h.accountManager.CreateUser(account.Id, user.Id, &server.UserInfo{ - Email: req.Email, - Name: *req.Name, - Role: req.Role, - AutoGroups: req.AutoGroups, + Email: email, + Name: *req.Name, + Role: req.Role, + AutoGroups: req.AutoGroups, + IsServiceUser: req.IsServiceUser, }) if err != nil { util.WriteError(err, w) @@ -137,9 +175,27 @@ func (h *UsersHandler) GetAllUsers(w http.ResponseWriter, r *http.Request) { return } + serviceUser := r.URL.Query().Get("service_user") + + log.Debugf("UserCount: %v", len(data)) + users := make([]*api.User, 0) for _, r := range data { - users = append(users, toUserResponse(r, claims.UserId)) + if serviceUser == "" { + 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 + } + log.Debugf("User %v is service user: %v", r.Name, r.IsServiceUser) + if includeServiceUser == r.IsServiceUser { + log.Debugf("Found service user: %v", r.Name) + users = append(users, toUserResponse(r, claims.UserId)) + } } util.WriteJSONObject(w, users) @@ -163,12 +219,13 @@ func toUserResponse(user *server.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, + Id: user.ID, + Name: user.Name, + Email: user.Email, + Role: user.Role, + AutoGroups: autoGroups, + Status: userStatus, + IsCurrent: &isCurrent, + IsServiceUser: &user.IsServiceUser, } } diff --git a/management/server/user.go b/management/server/user.go index 692a2833acd..63e35b2f6ad 100644 --- a/management/server/user.go +++ b/management/server/user.go @@ -4,11 +4,11 @@ import ( "fmt" "strings" + "github.com/google/uuid" log "github.com/sirupsen/logrus" "github.com/netbirdio/netbird/management/server/activity" "github.com/netbirdio/netbird/management/server/idp" - "github.com/netbirdio/netbird/management/server/jwtclaims" "github.com/netbirdio/netbird/management/server/status" ) @@ -42,8 +42,11 @@ type UserRole string // User represents a user of the system type User struct { - Id string - Role UserRole + Id string + Role UserRole + IsServiceUser 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 AutoGroups []string PATs map[string]*PersonalAccessToken @@ -63,12 +66,13 @@ func (u *User) toUserInfo(userData *idp.UserData) (*UserInfo, error) { if userData == nil { return &UserInfo{ - ID: u.Id, - Email: "", - Name: "", - Role: string(u.Role), - AutoGroups: u.AutoGroups, - Status: string(UserStatusActive), + ID: u.Id, + Email: "", + Name: u.ServiceUserName, + Role: string(u.Role), + AutoGroups: u.AutoGroups, + Status: string(UserStatusActive), + IsServiceUser: u.IsServiceUser, }, nil } if userData.ID != u.Id { @@ -81,12 +85,13 @@ 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), + ID: u.Id, + Email: userData.Email, + Name: userData.Name, + Role: string(u.Role), + AutoGroups: autoGroups, + Status: string(userStatus), + IsServiceUser: u.IsServiceUser, }, nil } @@ -101,34 +106,80 @@ func (u *User) Copy() *User { pats[k] = patCopy } return &User{ - Id: u.Id, - Role: u.Role, - AutoGroups: autoGroups, - PATs: pats, + Id: u.Id, + Role: u.Role, + AutoGroups: autoGroups, + IsServiceUser: u.IsServiceUser, + ServiceUserName: u.ServiceUserName, + PATs: pats, } } // NewUser creates a new user -func NewUser(id string, role UserRole) *User { +func NewUser(id string, role UserRole, isServiceUser bool, serviceUserName string, autoGroups []string) *User { return &User{ - Id: id, - Role: role, - AutoGroups: []string{}, + Id: id, + Role: role, + IsServiceUser: isServiceUser, + ServiceUserName: serviceUserName, + AutoGroups: autoGroups, } } // NewRegularUser creates a new user with role UserRoleAdmin func NewRegularUser(id string) *User { - return NewUser(id, UserRoleUser) + return NewUser(id, UserRoleUser, false, "", []string{}) } // NewAdminUser creates a new user with role UserRoleAdmin func NewAdminUser(id string) *User { - return NewUser(id, UserRoleAdmin) + return NewUser(id, UserRoleAdmin, false, "", []string{}) +} + +// createServiceUser creates a new service user under the given account. +func (am *DefaultAccountManager) createServiceUser(accountID string, role UserRole, serviceUserName string, autoGroups []string) (*UserInfo, error) { + unlock := am.Store.AcquireAccountLock(accountID) + defer unlock() + + account, err := am.Store.GetAccount(accountID) + if err != nil { + return nil, status.Errorf(status.NotFound, "account %s doesn't exist", accountID) + } + + newUserID := uuid.New().String() + newUser := NewUser(newUserID, role, true, serviceUserName, autoGroups) + log.Debugf("New User: %v", newUser) + account.Users[newUserID] = newUser + + err = am.Store.SaveAccount(account) + if err != nil { + return nil, err + } + + return &UserInfo{ + ID: newUser.Id, + Email: "", + Name: newUser.ServiceUserName, + Role: string(newUser.Role), + AutoGroups: newUser.AutoGroups, + Status: string(UserStatusActive), + IsServiceUser: true, + }, nil } // CreateUser creates a new user under the given account. Effectively this is a user invite. -func (am *DefaultAccountManager) CreateUser(accountID, userID string, invite *UserInfo) (*UserInfo, error) { +func (am *DefaultAccountManager) CreateUser(accountID, userID string, user *UserInfo) (*UserInfo, error) { + if user.IsServiceUser { + log.Debugf("Creting service user") + return am.createServiceUser(accountID, StrRoleToUserRole(user.Role), user.Name, user.AutoGroups) + } else { + log.Debugf("Inviting regular user") + return am.inviteNewUser(accountID, userID, user) + } +} + +// inviteNewUser Invites a USer to a given account and creates reference in datastore +func (am *DefaultAccountManager) inviteNewUser(accountID, userID string, invite *UserInfo) (*UserInfo, error) { unlock := am.Store.AcquireAccountLock(accountID) defer unlock() @@ -193,8 +244,45 @@ func (am *DefaultAccountManager) CreateUser(accountID, userID string, invite *Us } +func (am *DefaultAccountManager) DeleteUser(accountID, userID, targetUserID string) error { + unlock := am.Store.AcquireAccountLock(accountID) + defer unlock() + + account, err := am.Store.GetAccount(accountID) + if err != nil { + return err + } + + targetUser := account.Users[targetUserID] + if targetUser == nil { + return status.Errorf(status.NotFound, "user not found") + } + + if !targetUser.IsServiceUser { + return status.Errorf(status.PermissionDenied, "regular users can not be deleted") + } + + delete(account.Users, targetUserID) + + err = am.Store.SaveAccount(account) + if err != nil { + return err + } + + // refresh only needs to happen if we support delete of regular users + + // _, err = am.refreshCache(account.Id) + // if err != nil { + // return err + // } + + // am.storeEvent(userID, targetUserID, accountID, activity.UserDeleted, nil) + + return nil +} + // CreatePAT creates a new PAT for the given user -func (am *DefaultAccountManager) CreatePAT(accountID string, executingUserID string, targetUserId string, tokenName string, expiresIn int) (*PersonalAccessTokenGenerated, error) { +func (am *DefaultAccountManager) CreatePAT(accountID string, executingUserID string, targetUserID string, tokenName string, expiresIn int) (*PersonalAccessTokenGenerated, error) { unlock := am.Store.AcquireAccountLock(accountID) defer unlock() @@ -206,20 +294,25 @@ func (am *DefaultAccountManager) CreatePAT(accountID string, executingUserID str return nil, status.Errorf(status.InvalidArgument, "expiration has to be between 1 and 365") } - if executingUserID != targetUserId { - return nil, status.Errorf(status.PermissionDenied, "no permission to create PAT for this user") - } - account, err := am.Store.GetAccount(accountID) if err != nil { return nil, err } - targetUser := account.Users[targetUserId] + targetUser := account.Users[targetUserID] if targetUser == nil { return nil, status.Errorf(status.NotFound, "targetUser not found") } + executingUser := account.Users[executingUserID] + if targetUser == nil { + return nil, status.Errorf(status.NotFound, "user not found") + } + + if !(executingUserID == targetUserID || (executingUser.IsAdmin() && targetUser.IsServiceUser)) { + return nil, status.Errorf(status.PermissionDenied, "no permission to create PAT for this user") + } + pat, err := CreateNewPAT(tokenName, expiresIn, targetUser.Id) if err != nil { return nil, status.Errorf(status.Internal, "failed to create PAT: %v", err) @@ -233,7 +326,7 @@ func (am *DefaultAccountManager) CreatePAT(accountID string, executingUserID str } meta := map[string]any{"name": pat.Name} - am.storeEvent(executingUserID, targetUserId, accountID, activity.PersonalAccessTokenCreated, meta) + am.storeEvent(executingUserID, targetUserID, accountID, activity.PersonalAccessTokenCreated, meta) return pat, nil } @@ -243,21 +336,26 @@ func (am *DefaultAccountManager) DeletePAT(accountID string, executingUserID str unlock := am.Store.AcquireAccountLock(accountID) defer unlock() - if executingUserID != targetUserID { - return status.Errorf(status.PermissionDenied, "no permission to delete PAT for this user") - } - account, err := am.Store.GetAccount(accountID) if err != nil { return status.Errorf(status.NotFound, "account not found: %s", err) } - user := account.Users[targetUserID] - if user == nil { + targetUser := account.Users[targetUserID] + if targetUser == nil { return status.Errorf(status.NotFound, "user not found") } - pat := user.PATs[tokenID] + executingUser := account.Users[executingUserID] + if targetUser == nil { + return status.Errorf(status.NotFound, "user not found") + } + + if !(executingUserID == targetUserID || (executingUser.IsAdmin() && targetUser.IsServiceUser)) { + return status.Errorf(status.PermissionDenied, "no permission to delete PAT for this user") + } + + pat := targetUser.PATs[tokenID] if pat == nil { return status.Errorf(status.NotFound, "PAT not found") } @@ -274,7 +372,7 @@ func (am *DefaultAccountManager) DeletePAT(accountID string, executingUserID str meta := map[string]any{"name": pat.Name} am.storeEvent(executingUserID, targetUserID, accountID, activity.PersonalAccessTokenDeleted, meta) - delete(user.PATs, tokenID) + delete(targetUser.PATs, tokenID) err = am.Store.SaveAccount(account) if err != nil { @@ -288,21 +386,26 @@ func (am *DefaultAccountManager) GetPAT(accountID string, executingUserID string unlock := am.Store.AcquireAccountLock(accountID) defer unlock() - if executingUserID != targetUserID { - return nil, status.Errorf(status.PermissionDenied, "no permission to get PAT for this user") - } - account, err := am.Store.GetAccount(accountID) if err != nil { return nil, status.Errorf(status.NotFound, "account not found: %s", err) } - user := account.Users[targetUserID] - if user == nil { + targetUser := account.Users[targetUserID] + if targetUser == nil { + return nil, status.Errorf(status.NotFound, "user not found") + } + + executingUser := account.Users[executingUserID] + if targetUser == nil { return nil, status.Errorf(status.NotFound, "user not found") } - pat := user.PATs[tokenID] + if !(executingUserID == targetUserID || (executingUser.IsAdmin() && targetUser.IsServiceUser)) { + return nil, status.Errorf(status.PermissionDenied, "no permission to get PAT for this userser") + } + + pat := targetUser.PATs[tokenID] if pat == nil { return nil, status.Errorf(status.NotFound, "PAT not found") } @@ -315,22 +418,27 @@ func (am *DefaultAccountManager) GetAllPATs(accountID string, executingUserID st unlock := am.Store.AcquireAccountLock(accountID) defer unlock() - if executingUserID != targetUserID { - return nil, status.Errorf(status.PermissionDenied, "no permission to get PAT for this user") - } - account, err := am.Store.GetAccount(accountID) if err != nil { return nil, status.Errorf(status.NotFound, "account not found: %s", err) } - user := account.Users[targetUserID] - if user == nil { + targetUser := account.Users[targetUserID] + if targetUser == nil { + return nil, status.Errorf(status.NotFound, "user not found") + } + + executingUser := account.Users[executingUserID] + if targetUser == nil { return nil, status.Errorf(status.NotFound, "user not found") } + if !(executingUserID == targetUserID || (executingUser.IsAdmin() && targetUser.IsServiceUser)) { + return nil, status.Errorf(status.PermissionDenied, "no permission to get PAT for this user") + } + var pats []*PersonalAccessToken - for _, pat := range user.PATs { + for _, pat := range targetUser.PATs { pats = append(pats, pat) } @@ -404,7 +512,7 @@ func (am *DefaultAccountManager) SaveUser(accountID, userID string, update *User } }() - if !isNil(am.idpManager) { + if !isNil(am.idpManager) && !newUser.IsServiceUser { userData, err := am.lookupUserInCache(newUser.Id, account) if err != nil { return nil, err @@ -454,14 +562,19 @@ func (am *DefaultAccountManager) GetOrCreateAccountByUser(userID, domain string) return account, nil } -// IsUserAdmin flag for current user authenticated by JWT token -func (am *DefaultAccountManager) IsUserAdmin(claims jwtclaims.AuthorizationClaims) (bool, error) { - account, _, err := am.GetAccountFromToken(claims) +// GetAccountByUserID returns an existing account for a given user id +func (am *DefaultAccountManager) GetAccountByUserID(userID string) (*Account, error) { + return am.Store.GetAccountByUser(userID) +} + +// IsUserAdmin looks up a user by his ID and returns true if he is an admin +func (am *DefaultAccountManager) IsUserAdmin(userID string) (bool, error) { + account, err := am.GetAccountByUserID(userID) if err != nil { return false, fmt.Errorf("get account: %v", err) } - user, ok := account.Users[claims.UserId] + user, ok := account.Users[userID] if !ok { return false, status.Errorf(status.NotFound, "user not found") } @@ -512,20 +625,44 @@ func (am *DefaultAccountManager) GetUsersFromAccount(accountID, userID string) ( return userInfos, nil } - for _, queriedUser := range queriedUsers { - if !user.IsAdmin() && user.Id != queriedUser.ID { + for _, localUser := range account.Users { + if !user.IsAdmin() && user.Id != localUser.Id { // if user is not an admin then show only current user and do not show other users continue } - if localUser, contains := account.Users[queriedUser.ID]; contains { - info, err := localUser.toUserInfo(queriedUser) + var info *UserInfo + if queriedUser, contains := findUserInIDPUserdata(localUser.Id, queriedUsers); contains { + info, err = localUser.toUserInfo(queriedUser) if err != nil { return nil, err } - userInfos = append(userInfos, info) + } else { + name := "" + if localUser.IsServiceUser { + name = localUser.ServiceUserName + } + info = &UserInfo{ + ID: localUser.Id, + Email: "", + Name: name, + Role: string(localUser.Role), + AutoGroups: localUser.AutoGroups, + Status: string(UserStatusActive), + IsServiceUser: localUser.IsServiceUser, + } } + userInfos = append(userInfos, info) } return userInfos, nil } + +func findUserInIDPUserdata(userID string, userData []*idp.UserData) (*idp.UserData, bool) { + for _, user := range userData { + if user.ID == userID { + return user, true + } + } + return nil, false +} From a02dca5c4770bd317a4e9a64a69d6e0ca4513717 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Thu, 6 Apr 2023 16:53:41 +0200 Subject: [PATCH 02/13] update API definition --- management/server/http/api/openapi.yml | 39 ++++++++++++++++++++++++- management/server/http/api/types.gen.go | 14 ++++++++- 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/management/server/http/api/openapi.yml b/management/server/http/api/openapi.yml index eaeb5693c73..461bfe59ce3 100644 --- a/management/server/http/api/openapi.yml +++ b/management/server/http/api/openapi.yml @@ -77,6 +77,10 @@ components: description: Is true if authenticated user is the same as this user type: boolean readOnly: true + is_service_user: + description: Is true if this user is a service user + type: boolean + readOnly: true required: - id - email @@ -115,10 +119,13 @@ components: type: array items: type: string + is_service_user: + description: Is true if this user is a service user + type: boolean required: - role - auto_groups - - email + - is_service_user PeerMinimum: type: object properties: @@ -825,6 +832,12 @@ paths: tags: [ Users ] security: - BearerAuth: [ ] + parameters: + - in: query + name: service_user + schema: + type: boolean + description: Filters users and returns either normal users or service users responses: '200': description: A JSON array of Users @@ -903,6 +916,30 @@ paths: "$ref": "#/components/responses/forbidden" '500': "$ref": "#/components/responses/internal_error" + delete: + summary: Delete a User + tags: [ Users ] + security: + - BearerAuth: [ ] + parameters: + - in: path + name: id + required: true + schema: + type: string + description: The User ID + responses: + '200': + description: Delete status code + 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/{userId}/tokens: get: summary: Returns a list of all tokens for a user diff --git a/management/server/http/api/types.gen.go b/management/server/http/api/types.gen.go index 930a9df54f2..296249d4eb7 100644 --- a/management/server/http/api/types.gen.go +++ b/management/server/http/api/types.gen.go @@ -677,6 +677,9 @@ type User struct { // IsCurrent Is true if authenticated user is the same as this user IsCurrent *bool `json:"is_current,omitempty"` + // IsServiceUser Is true if this user is a service user + IsServiceUser *bool `json:"is_service_user,omitempty"` + // Name User's name from idp provider Name string `json:"name"` @@ -696,7 +699,10 @@ type UserCreateRequest struct { AutoGroups []string `json:"auto_groups"` // Email User's Email to send invite to - Email string `json:"email"` + Email *string `json:"email,omitempty"` + + // IsServiceUser Is true if this user is a service user + IsServiceUser bool `json:"is_service_user"` // Name User's full name Name *string `json:"name,omitempty"` @@ -787,6 +793,12 @@ type PutApiRulesIdJSONBody struct { Sources *[]string `json:"sources,omitempty"` } +// GetApiUsersParams defines parameters for GetApiUsers. +type GetApiUsersParams struct { + // ServiceUser Filters users and returns either normal users or service users + ServiceUser *bool `form:"service_user,omitempty" json:"service_user,omitempty"` +} + // PutApiAccountsIdJSONRequestBody defines body for PutApiAccountsId for application/json ContentType. type PutApiAccountsIdJSONRequestBody PutApiAccountsIdJSONBody From 38f48a5472e343ec2f2f533e6c087f4aa3000945 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Thu, 6 Apr 2023 17:12:57 +0200 Subject: [PATCH 03/13] update account mock --- management/server/mock_server/account_mock.go | 27 ++++++++++++------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/management/server/mock_server/account_mock.go b/management/server/mock_server/account_mock.go index eb473d03c5d..cd75bc31367 100644 --- a/management/server/mock_server/account_mock.go +++ b/management/server/mock_server/account_mock.go @@ -15,12 +15,12 @@ import ( type MockAccountManager struct { GetOrCreateAccountByUserFunc func(userId, domain string) (*server.Account, error) - GetAccountByUserFunc func(userId string) (*server.Account, error) + GetAccountByUserIDFunc func(userId string) (*server.Account, error) CreateSetupKeyFunc func(accountId string, keyName string, keyType server.SetupKeyType, expiresIn time.Duration, autoGroups []string, usageLimit int, userID string) (*server.SetupKey, error) GetSetupKeyFunc func(accountID, userID, keyID string) (*server.SetupKey, error) GetAccountByUserOrAccountIdFunc func(userId, accountId, domain string) (*server.Account, error) - IsUserAdminFunc func(claims jwtclaims.AuthorizationClaims) (bool, error) + IsUserAdminFunc func(userID string) (bool, error) AccountExistsFunc func(accountId string) (*bool, error) GetPeerByKeyFunc func(peerKey string) (*server.Peer, error) GetPeersFunc func(accountID, userID string) ([]*server.Peer, error) @@ -61,6 +61,7 @@ type MockAccountManager struct { SaveSetupKeyFunc func(accountID string, key *server.SetupKey, userID string) (*server.SetupKey, error) ListSetupKeysFunc func(accountID, userID string) ([]*server.SetupKey, error) SaveUserFunc func(accountID, userID string, user *server.User) (*server.UserInfo, error) + DeleteUserFunc func(accountID string, executingUserID string, targetUserID string) error CreatePATFunc func(accountID string, executingUserID string, targetUserId string, tokenName string, expiresIn int) (*server.PersonalAccessTokenGenerated, error) DeletePATFunc func(accountID string, executingUserID string, targetUserId string, tokenID string) error GetPATFunc func(accountID string, executingUserID string, targetUserId string, tokenID string) (*server.PersonalAccessToken, error) @@ -112,12 +113,12 @@ func (am *MockAccountManager) GetOrCreateAccountByUser( ) } -// GetAccountByUser mock implementation of GetAccountByUser from server.AccountManager interface -func (am *MockAccountManager) GetAccountByUser(userId string) (*server.Account, error) { - if am.GetAccountByUserFunc != nil { - return am.GetAccountByUserFunc(userId) +// GetAccountByUserID mock implementation of GetAccountByUserID from server.AccountManager interface +func (am *MockAccountManager) GetAccountByUserID(userId string) (*server.Account, error) { + if am.GetAccountByUserIDFunc != nil { + return am.GetAccountByUserIDFunc(userId) } - return nil, status.Errorf(codes.Unimplemented, "method GetAccountByUser is not implemented") + return nil, status.Errorf(codes.Unimplemented, "method GetAccountByUserID is not implemented") } // CreateSetupKey mock implementation of CreateSetupKey from server.AccountManager interface @@ -394,9 +395,9 @@ func (am *MockAccountManager) UpdatePeerMeta(peerID string, meta server.PeerSyst } // IsUserAdmin mock implementation of IsUserAdmin from server.AccountManager interface -func (am *MockAccountManager) IsUserAdmin(claims jwtclaims.AuthorizationClaims) (bool, error) { +func (am *MockAccountManager) IsUserAdmin(userID string) (bool, error) { if am.IsUserAdminFunc != nil { - return am.IsUserAdminFunc(claims) + return am.IsUserAdminFunc(userID) } return false, status.Errorf(codes.Unimplemented, "method IsUserAdmin is not implemented") } @@ -500,6 +501,14 @@ func (am *MockAccountManager) SaveUser(accountID, userID string, user *server.Us return nil, status.Errorf(codes.Unimplemented, "method SaveUser is not implemented") } +// DeleteUser mocks DeleteUser of the AccountManager interface +func (am *MockAccountManager) DeleteUser(accountID string, executingUserID string, targetUserID string) error { + if am.DeleteUserFunc != nil { + return am.DeleteUserFunc(accountID, executingUserID, targetUserID) + } + return status.Errorf(codes.Unimplemented, "method DeleteUser is not implemented") +} + // GetNameServerGroup mocks GetNameServerGroup of the AccountManager interface func (am *MockAccountManager) GetNameServerGroup(accountID, nsGroupID string) (*nbdns.NameServerGroup, error) { if am.GetNameServerGroupFunc != nil { From c2f4cc7b9b1ef59a3572db85bb4e6a8364cac49d Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Fri, 7 Apr 2023 17:15:39 +0200 Subject: [PATCH 04/13] added tests --- management/server/account.go | 2 +- management/server/http/users_handler.go | 4 +- management/server/http/users_handler_test.go | 232 ++++++++++--- management/server/mock_server/account_mock.go | 6 +- management/server/user.go | 6 +- management/server/user_test.go | 327 +++++++++++++++++- 6 files changed, 516 insertions(+), 61 deletions(-) diff --git a/management/server/account.go b/management/server/account.go index b442e9b39a7..34bf1236672 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -50,7 +50,7 @@ type AccountManager interface { autoGroups []string, usageLimit int, userID string) (*SetupKey, error) SaveSetupKey(accountID string, key *SetupKey, userID string) (*SetupKey, error) CreateUser(accountID, userID string, key *UserInfo) (*UserInfo, error) - DeleteUser(accountID, executingUserID string, targetUserID string) error + DeleteUser(accountID, targetUserID string) error ListSetupKeys(accountID, userID string) ([]*SetupKey, error) SaveUser(accountID, userID string, update *User) (*UserInfo, error) GetSetupKey(accountID, userID, keyID string) (*SetupKey, error) diff --git a/management/server/http/users_handler.go b/management/server/http/users_handler.go index bdbefe39f13..c6af1d5704b 100644 --- a/management/server/http/users_handler.go +++ b/management/server/http/users_handler.go @@ -87,7 +87,7 @@ func (h *UsersHandler) DeleteUser(w http.ResponseWriter, r *http.Request) { } claims := h.claimsExtractor.FromRequestContext(r) - account, user, err := h.accountManager.GetAccountFromToken(claims) + account, _, err := h.accountManager.GetAccountFromToken(claims) if err != nil { util.WriteError(err, w) return @@ -100,7 +100,7 @@ func (h *UsersHandler) DeleteUser(w http.ResponseWriter, r *http.Request) { return } - err = h.accountManager.DeleteUser(account.Id, user.Id, targetUserID) + err = h.accountManager.DeleteUser(account.Id, targetUserID) if err != nil { util.WriteError(err, w) return diff --git a/management/server/http/users_handler_test.go b/management/server/http/users_handler_test.go index 9bba1c47259..88697aeaa83 100644 --- a/management/server/http/users_handler_test.go +++ b/management/server/http/users_handler_test.go @@ -1,52 +1,91 @@ package http import ( + "bytes" "encoding/json" "io" "net/http" "net/http/httptest" "testing" - "github.com/magiconair/properties/assert" + "github.com/gorilla/mux" + "github.com/stretchr/testify/assert" "github.com/netbirdio/netbird/management/server" + "github.com/netbirdio/netbird/management/server/http/api" "github.com/netbirdio/netbird/management/server/jwtclaims" "github.com/netbirdio/netbird/management/server/mock_server" + "github.com/netbirdio/netbird/management/server/status" ) -func initUsers(user ...*server.User) *UsersHandler { +const ( + serviceUserID = "serviceUserID" + regularUserID = "regularUserID" +) + +var usersTestAccount = &server.Account{ + Id: existingAccountID, + Domain: domain, + Users: map[string]*server.User{ + existingUserID: { + Id: existingUserID, + Role: "admin", + IsServiceUser: false, + }, + regularUserID: { + Id: regularUserID, + Role: "user", + IsServiceUser: false, + }, + serviceUserID: { + Id: serviceUserID, + Role: "user", + IsServiceUser: true, + }, + }, +} + +func initUsersTestData() *UsersHandler { return &UsersHandler{ accountManager: &mock_server.MockAccountManager{ GetAccountFromTokenFunc: func(claims jwtclaims.AuthorizationClaims) (*server.Account, *server.User, error) { - users := make(map[string]*server.User, 0) - for _, u := range user { - users[u.Id] = u - } - return &server.Account{ - Id: "12345", - Domain: "netbird.io", - Users: users, - }, users[claims.UserId], nil + return usersTestAccount, usersTestAccount.Users[claims.UserId], nil }, GetUsersFromAccountFunc: func(accountID, userID string) ([]*server.UserInfo, error) { users := make([]*server.UserInfo, 0) - for _, v := range user { + for _, v := range usersTestAccount.Users { users = append(users, &server.UserInfo{ - ID: v.Id, - Role: string(v.Role), - Name: "", - Email: "", + ID: v.Id, + Role: string(v.Role), + Name: "", + Email: "", + IsServiceUser: v.IsServiceUser, }) } return users, nil }, + CreateUserFunc: func(accountID, userID string, key *server.UserInfo) (*server.UserInfo, error) { + if userID != existingUserID { + return nil, status.Errorf(status.NotFound, "user with ID %s does not exists", userID) + } + return key, nil + }, + DeleteUserFunc: func(accountID string, targetUserID string) error { + if targetUserID == notFoundUserID { + return status.Errorf(status.NotFound, "user with ID %s does not exists", targetUserID) + } + if !usersTestAccount.Users[targetUserID].IsServiceUser { + return status.Errorf(status.PermissionDenied, "user with ID %s is not a service user and can not be deleted", targetUserID) + } + return nil + }, }, claimsExtractor: jwtclaims.NewClaimsExtractor( jwtclaims.WithFromRequestContext(func(r *http.Request) jwtclaims.AuthorizationClaims { return jwtclaims.AuthorizationClaims{ - UserId: "1", - Domain: "hotmail.com", - AccountId: "test_id", + UserId: existingUserID, + Domain: domain, + AccountId: existingAccountID, } }), ), @@ -54,8 +93,84 @@ func initUsers(user ...*server.User) *UsersHandler { } func TestGetUsers(t *testing.T) { - users := []*server.User{{Id: "1", Role: "admin"}, {Id: "2", Role: "user"}, {Id: "3", Role: "user"}} - userHandler := initUsers(users...) + tt := []struct { + name string + expectedStatus int + requestType string + requestPath string + expectedUserIDs []string + }{ + {name: "GetAllUsers", requestType: http.MethodGet, requestPath: "/api/users", expectedStatus: http.StatusOK, expectedUserIDs: []string{existingUserID, regularUserID, serviceUserID}}, + {name: "GetOnlyServiceUsers", requestType: http.MethodGet, requestPath: "/api/users?service_user=true", expectedStatus: http.StatusOK, expectedUserIDs: []string{serviceUserID}}, + {name: "GetOnlyRegularUsers", requestType: http.MethodGet, requestPath: "/api/users?service_user=false", expectedStatus: http.StatusOK, expectedUserIDs: []string{existingUserID, regularUserID}}, + } + + userHandler := initUsersTestData() + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + recorder := httptest.NewRecorder() + req := httptest.NewRequest(tc.requestType, tc.requestPath, nil) + + userHandler.GetAllUsers(recorder, req) + + res := recorder.Result() + defer res.Body.Close() + + content, err := io.ReadAll(res.Body) + if err != nil { + t.Fatalf("I don't know what I expected; %v", err) + } + + if status := recorder.Code; status != tc.expectedStatus { + t.Errorf("handler returned wrong status code: got %v want %v, content: %s", + status, tc.expectedStatus, string(content)) + return + } + + respBody := []*server.UserInfo{} + err = json.Unmarshal(content, &respBody) + if err != nil { + t.Fatalf("Sent content is not in correct json format; %v", err) + } + + assert.Equal(t, len(respBody), len(tc.expectedUserIDs)) + for _, v := range respBody { + assert.Contains(t, tc.expectedUserIDs, v.ID) + assert.Equal(t, v.ID, usersTestAccount.Users[v.ID].Id) + assert.Equal(t, v.Role, string(usersTestAccount.Users[v.ID].Role)) + assert.Equal(t, v.IsServiceUser, usersTestAccount.Users[v.ID].IsServiceUser) + } + }) + } +} + +func TestCreateUser(t *testing.T) { + name := "name" + email := "email" + serviceUserToAdd := api.UserCreateRequest{ + AutoGroups: []string{}, + Email: nil, + IsServiceUser: true, + Name: &name, + Role: "admin", + } + serviceUserString, err := json.Marshal(serviceUserToAdd) + if err != nil { + t.Fatal(err) + } + + regularUserToAdd := api.UserCreateRequest{ + AutoGroups: []string{}, + Email: &email, + IsServiceUser: true, + Name: &name, + Role: "admin", + } + regularUserString, err := json.Marshal(regularUserToAdd) + if err != nil { + t.Fatal(err) + } tt := []struct { name string @@ -65,40 +180,79 @@ func TestGetUsers(t *testing.T) { requestBody io.Reader expectedResult []*server.User }{ - {name: "GetAllUsers", requestType: http.MethodGet, requestPath: "/api/users/", expectedStatus: http.StatusOK, expectedResult: users}, + {name: "CreateServiceUser", requestType: http.MethodPost, requestPath: "/api/users", expectedStatus: http.StatusOK, requestBody: bytes.NewBuffer(serviceUserString)}, + // right now creation is blocked in AC middleware, will be refactored in the future + {name: "CreateRegularUser", requestType: http.MethodPost, requestPath: "/api/users", expectedStatus: http.StatusOK, requestBody: bytes.NewBuffer(regularUserString)}, } + userHandler := initUsersTestData() + for _, tc := range tt { t.Run(tc.name, func(t *testing.T) { - req := httptest.NewRequest(tc.requestType, tc.requestPath, nil) + req := httptest.NewRequest(tc.requestType, tc.requestPath, tc.requestBody) rr := httptest.NewRecorder() - userHandler.GetAllUsers(rr, req) + userHandler.CreateUser(rr, req) res := rr.Result() defer res.Body.Close() if status := rr.Code; status != tc.expectedStatus { t.Fatalf("handler returned wrong status code: got %v want %v", - status, http.StatusOK) + status, tc.expectedStatus) } + }) + } +} - content, err := io.ReadAll(res.Body) - if err != nil { - t.Fatal(err) - } +func TestDeleteUser(t *testing.T) { + tt := []struct { + name string + expectedStatus int + expectedBody bool + requestType string + requestPath string + requestVars map[string]string + requestBody io.Reader + }{ + { + name: "Delete Regular User", + requestType: http.MethodDelete, + requestPath: "/api/users/" + regularUserID, + requestVars: map[string]string{"id": regularUserID}, + expectedStatus: http.StatusForbidden, + }, + { + name: "Delete Service User", + requestType: http.MethodDelete, + requestPath: "/api/users/" + serviceUserID, + requestVars: map[string]string{"id": serviceUserID}, + expectedStatus: http.StatusOK, + }, + { + name: "Delete Not Existing User", + requestType: http.MethodDelete, + requestPath: "/api/users/" + notFoundUserID, + requestVars: map[string]string{"id": notFoundUserID}, + expectedStatus: http.StatusNotFound, + }, + } - respBody := []*server.UserInfo{} - err = json.Unmarshal(content, &respBody) - if err != nil { - t.Fatalf("Sent content is not in correct json format; %v", err) - } + userHandler := initUsersTestData() + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequest(tc.requestType, tc.requestPath, nil) + req = mux.SetURLVars(req, tc.requestVars) + rr := httptest.NewRecorder() - if tc.expectedResult != nil { - for i, resp := range respBody { - assert.Equal(t, resp.ID, tc.expectedResult[i].Id) - assert.Equal(t, string(resp.Role), string(tc.expectedResult[i].Role)) - } + userHandler.DeleteUser(rr, req) + + res := rr.Result() + defer res.Body.Close() + + if status := rr.Code; status != tc.expectedStatus { + t.Fatalf("handler returned wrong status code: got %v want %v", + status, tc.expectedStatus) } }) } diff --git a/management/server/mock_server/account_mock.go b/management/server/mock_server/account_mock.go index cd75bc31367..7fd8a58a7f1 100644 --- a/management/server/mock_server/account_mock.go +++ b/management/server/mock_server/account_mock.go @@ -61,7 +61,7 @@ type MockAccountManager struct { SaveSetupKeyFunc func(accountID string, key *server.SetupKey, userID string) (*server.SetupKey, error) ListSetupKeysFunc func(accountID, userID string) ([]*server.SetupKey, error) SaveUserFunc func(accountID, userID string, user *server.User) (*server.UserInfo, error) - DeleteUserFunc func(accountID string, executingUserID string, targetUserID string) error + DeleteUserFunc func(accountID string, targetUserID string) error CreatePATFunc func(accountID string, executingUserID string, targetUserId string, tokenName string, expiresIn int) (*server.PersonalAccessTokenGenerated, error) DeletePATFunc func(accountID string, executingUserID string, targetUserId string, tokenID string) error GetPATFunc func(accountID string, executingUserID string, targetUserId string, tokenID string) (*server.PersonalAccessToken, error) @@ -502,9 +502,9 @@ func (am *MockAccountManager) SaveUser(accountID, userID string, user *server.Us } // DeleteUser mocks DeleteUser of the AccountManager interface -func (am *MockAccountManager) DeleteUser(accountID string, executingUserID string, targetUserID string) error { +func (am *MockAccountManager) DeleteUser(accountID string, targetUserID string) error { if am.DeleteUserFunc != nil { - return am.DeleteUserFunc(accountID, executingUserID, targetUserID) + return am.DeleteUserFunc(accountID, targetUserID) } return status.Errorf(codes.Unimplemented, "method DeleteUser is not implemented") } diff --git a/management/server/user.go b/management/server/user.go index 63e35b2f6ad..0c9cd16793a 100644 --- a/management/server/user.go +++ b/management/server/user.go @@ -170,10 +170,8 @@ func (am *DefaultAccountManager) createServiceUser(accountID string, role UserRo // 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 { - log.Debugf("Creting service user") return am.createServiceUser(accountID, StrRoleToUserRole(user.Role), user.Name, user.AutoGroups) } else { - log.Debugf("Inviting regular user") return am.inviteNewUser(accountID, userID, user) } } @@ -244,7 +242,7 @@ func (am *DefaultAccountManager) inviteNewUser(accountID, userID string, invite } -func (am *DefaultAccountManager) DeleteUser(accountID, userID, targetUserID string) error { +func (am *DefaultAccountManager) DeleteUser(accountID, targetUserID string) error { unlock := am.Store.AcquireAccountLock(accountID) defer unlock() @@ -276,8 +274,6 @@ func (am *DefaultAccountManager) DeleteUser(accountID, userID, targetUserID stri // return err // } - // am.storeEvent(userID, targetUserID, accountID, activity.UserDeleted, nil) - return nil } diff --git a/management/server/user_test.go b/management/server/user_test.go index 29e6bc2bcb7..920ac9dd876 100644 --- a/management/server/user_test.go +++ b/management/server/user_test.go @@ -1,25 +1,33 @@ package server import ( + "errors" + "fmt" + "reflect" "testing" + "time" + "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/assert" "github.com/netbirdio/netbird/management/server/activity" ) const ( - mockAccountID = "accountID" - mockUserID = "userID" - mockTargetUserId = "targetUserID" - mockTokenID1 = "tokenID1" - mockToken1 = "SoMeHaShEdToKeN1" - mockTokenID2 = "tokenID2" - mockToken2 = "SoMeHaShEdToKeN2" - mockTokenName = "tokenName" - mockEmptyTokenName = "" - mockExpiresIn = 7 - mockWrongExpiresIn = 4506 + mockAccountID = "accountID" + mockUserID = "userID" + mockServiceUserID = "serviceUserID" + mockRole = "user" + mockServiceUserName = "serviceUserName" + mockTargetUserId = "targetUserID" + mockTokenID1 = "tokenID1" + mockToken1 = "SoMeHaShEdToKeN1" + mockTokenID2 = "tokenID2" + mockToken2 = "SoMeHaShEdToKeN2" + mockTokenName = "tokenName" + mockEmptyTokenName = "" + mockExpiresIn = 7 + mockWrongExpiresIn = 4506 ) func TestUser_CreatePAT_ForSameUser(t *testing.T) { @@ -207,3 +215,300 @@ func TestUser_GetAllPATs(t *testing.T) { assert.Equal(t, 2, len(pats)) } + +func TestUser_Copy(t *testing.T) { + // this is an imaginary case which will never be in DB this way + user := User{ + Id: "userId", + Role: "role", + IsServiceUser: true, + ServiceUserName: "servicename", + AutoGroups: []string{"group1", "group2"}, + PATs: map[string]*PersonalAccessToken{ + "pat1": { + ID: "pat1", + Name: "First PAT", + HashedToken: "SoMeHaShEdToKeN", + ExpirationDate: time.Now().AddDate(0, 0, 7), + CreatedBy: "userId", + CreatedAt: time.Now(), + LastUsed: time.Now(), + }, + }, + } + + err := validateStruct(user) + if err != nil { + t.Fatalf("Test needs update: dummy struct has not all fields set : %s", err) + } + + copiedUser := user.Copy() + + assert.True(t, cmp.Equal(user, *copiedUser)) +} + +// based on https://medium.com/@anajankow/fast-check-if-all-struct-fields-are-set-in-golang-bba1917213d2 +func validateStruct(s interface{}) (err error) { + + structType := reflect.TypeOf(s) + structVal := reflect.ValueOf(s) + fieldNum := structVal.NumField() + + for i := 0; i < fieldNum; i++ { + field := structVal.Field(i) + fieldName := structType.Field(i).Name + + isSet := field.IsValid() && !field.IsZero() + + if !isSet { + err = errors.New(fmt.Sprintf("%v%s in not set; ", err, fieldName)) + } + + } + + return err +} + +func TestUser_CreateServiceUser(t *testing.T) { + store := newStore(t) + account := newAccountWithId(mockAccountID, mockUserID, "") + + err := store.SaveAccount(account) + if err != nil { + t.Fatalf("Error when saving account: %s", err) + } + + am := DefaultAccountManager{ + Store: store, + eventStore: &activity.InMemoryEventStore{}, + } + + user, err := am.createServiceUser(mockAccountID, mockRole, mockServiceUserName, []string{"group1", "group2"}) + if err != nil { + t.Fatalf("Error when creating service user: %s", err) + } + + assert.Equal(t, 2, len(store.Accounts[mockAccountID].Users)) + assert.NotNil(t, store.Accounts[mockAccountID].Users[user.ID]) + assert.True(t, store.Accounts[mockAccountID].Users[user.ID].IsServiceUser) + assert.Equal(t, mockServiceUserName, store.Accounts[mockAccountID].Users[user.ID].ServiceUserName) + assert.Equal(t, UserRole(mockRole), store.Accounts[mockAccountID].Users[user.ID].Role) + assert.Equal(t, []string{"group1", "group2"}, store.Accounts[mockAccountID].Users[user.ID].AutoGroups) + assert.Equal(t, map[string]*PersonalAccessToken{}, store.Accounts[mockAccountID].Users[user.ID].PATs) + + assert.Zero(t, user.Email) + assert.True(t, user.IsServiceUser) + assert.Equal(t, "active", user.Status) +} + +func TestUser_CreateUser_ServiceUser(t *testing.T) { + store := newStore(t) + account := newAccountWithId(mockAccountID, mockUserID, "") + + err := store.SaveAccount(account) + if err != nil { + t.Fatalf("Error when saving account: %s", err) + } + + am := DefaultAccountManager{ + Store: store, + eventStore: &activity.InMemoryEventStore{}, + } + + user, err := am.CreateUser(mockAccountID, "", &UserInfo{ + Name: mockServiceUserName, + Role: mockRole, + IsServiceUser: true, + AutoGroups: []string{"group1", "group2"}, + }) + + if err != nil { + t.Fatalf("Error when creating user: %s", err) + } + + assert.True(t, user.IsServiceUser) + assert.Equal(t, 2, len(store.Accounts[mockAccountID].Users)) + assert.True(t, store.Accounts[mockAccountID].Users[user.ID].IsServiceUser) + assert.Equal(t, mockServiceUserName, store.Accounts[mockAccountID].Users[user.ID].ServiceUserName) + assert.Equal(t, UserRole(mockRole), store.Accounts[mockAccountID].Users[user.ID].Role) + assert.Equal(t, []string{"group1", "group2"}, store.Accounts[mockAccountID].Users[user.ID].AutoGroups) + + assert.Equal(t, mockServiceUserName, user.Name) + assert.Equal(t, mockRole, user.Role) + assert.Equal(t, []string{"group1", "group2"}, user.AutoGroups) + assert.Equal(t, "active", user.Status) +} + +func TestUser_CreateUser_RegularUser(t *testing.T) { + store := newStore(t) + account := newAccountWithId(mockAccountID, mockUserID, "") + + err := store.SaveAccount(account) + if err != nil { + t.Fatalf("Error when saving account: %s", err) + } + + am := DefaultAccountManager{ + Store: store, + eventStore: &activity.InMemoryEventStore{}, + } + + _, err = am.CreateUser(mockAccountID, "", &UserInfo{ + Name: mockServiceUserName, + Role: mockRole, + IsServiceUser: false, + AutoGroups: []string{"group1", "group2"}, + }) + + assert.Errorf(t, err, "Not configured IDP will throw error but right path used") +} + +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{}, + } + + err = am.DeleteUser(mockAccountID, mockServiceUserID) + if err != nil { + t.Fatalf("Error when deleting user: %s", err) + } + + assert.Equal(t, 1, len(store.Accounts[mockAccountID].Users)) + assert.Nil(t, store.Accounts[mockAccountID].Users[mockServiceUserID]) +} + +func TestUser_DeleteUser_regularUser(t *testing.T) { + store := newStore(t) + account := newAccountWithId(mockAccountID, mockUserID, "") + + 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) + + assert.Errorf(t, err, "Regular users can not be deleted (yet)") +} + +func TestUser_IsUserAdmin_ForAdmin(t *testing.T) { + store := newStore(t) + account := newAccountWithId(mockAccountID, mockUserID, "") + + err := store.SaveAccount(account) + if err != nil { + t.Fatalf("Error when saving account: %s", err) + } + + am := DefaultAccountManager{ + Store: store, + eventStore: &activity.InMemoryEventStore{}, + } + + ok, err := am.IsUserAdmin(mockUserID) + if err != nil { + t.Fatalf("Error when checking user role: %s", err) + } + + assert.True(t, ok) +} + +func TestUser_IsUserAdmin_ForUser(t *testing.T) { + store := newStore(t) + account := newAccountWithId(mockAccountID, mockUserID, "") + account.Users[mockUserID] = &User{ + Id: mockUserID, + Role: "user", + } + + err := store.SaveAccount(account) + if err != nil { + t.Fatalf("Error when saving account: %s", err) + } + + am := DefaultAccountManager{ + Store: store, + eventStore: &activity.InMemoryEventStore{}, + } + + ok, err := am.IsUserAdmin(mockUserID) + if err != nil { + t.Fatalf("Error when checking user role: %s", err) + } + + assert.False(t, ok) +} + +func TestUser_GetUsersFromAccount_ForAdmin(t *testing.T) { + store := newStore(t) + account := newAccountWithId(mockAccountID, mockUserID, "") + account.Users[mockServiceUserID] = &User{ + Id: mockServiceUserID, + Role: "user", + IsServiceUser: true, + } + + err := store.SaveAccount(account) + if err != nil { + t.Fatalf("Error when saving account: %s", err) + } + + am := DefaultAccountManager{ + Store: store, + eventStore: &activity.InMemoryEventStore{}, + } + + users, err := am.GetUsersFromAccount(mockAccountID, mockUserID) + if err != nil { + t.Fatalf("Error when getting users from account: %s", err) + } + + assert.Equal(t, 2, len(users)) +} + +func TestUser_GetUsersFromAccount_ForUser(t *testing.T) { + store := newStore(t) + account := newAccountWithId(mockAccountID, mockUserID, "") + account.Users[mockServiceUserID] = &User{ + Id: mockServiceUserID, + Role: "user", + IsServiceUser: true, + } + + err := store.SaveAccount(account) + if err != nil { + t.Fatalf("Error when saving account: %s", err) + } + + am := DefaultAccountManager{ + Store: store, + eventStore: &activity.InMemoryEventStore{}, + } + + users, err := am.GetUsersFromAccount(mockAccountID, mockServiceUserID) + if err != nil { + t.Fatalf("Error when getting users from account: %s", err) + } + + assert.Equal(t, 1, len(users)) + assert.Equal(t, mockServiceUserID, users[0].ID) +} From d58fb4c97c640b573b18bab5102903a5837370ab Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Fri, 7 Apr 2023 17:24:53 +0200 Subject: [PATCH 05/13] re-enable idp lookup and filter service users --- management/server/account.go | 8 ++++---- management/server/user.go | 4 +++- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/management/server/account.go b/management/server/account.go index 34bf1236672..9651535acc7 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -984,10 +984,10 @@ func (am *DefaultAccountManager) lookupCache(accountUsers map[string]struct{}, a if reload { // reload cache once avoiding loops - // data, err = am.refreshCache(accountID) - // if err != nil { - // return nil, err - // } + data, err = am.refreshCache(accountID) + if err != nil { + return nil, err + } } return data, err diff --git a/management/server/user.go b/management/server/user.go index 0c9cd16793a..b1b81af243f 100644 --- a/management/server/user.go +++ b/management/server/user.go @@ -595,7 +595,9 @@ func (am *DefaultAccountManager) GetUsersFromAccount(accountID, userID string) ( if !isNil(am.idpManager) { users := make(map[string]struct{}, len(account.Users)) for _, user := range account.Users { - users[user.Id] = struct{}{} + if !user.IsServiceUser { + users[user.Id] = struct{}{} + } } queriedUsers, err = am.lookupCache(users, accountID) if err != nil { From ae69d6d0da3a93284520cebf99bf71c215d44abf Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Fri, 7 Apr 2023 17:34:49 +0200 Subject: [PATCH 06/13] fix linter --- management/server/user_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/management/server/user_test.go b/management/server/user_test.go index 920ac9dd876..9d4eef9bf7e 100644 --- a/management/server/user_test.go +++ b/management/server/user_test.go @@ -1,7 +1,6 @@ package server import ( - "errors" "fmt" "reflect" "testing" @@ -261,7 +260,7 @@ func validateStruct(s interface{}) (err error) { isSet := field.IsValid() && !field.IsZero() if !isSet { - err = errors.New(fmt.Sprintf("%v%s in not set; ", err, fieldName)) + err = fmt.Errorf("%v%s in not set; ", err, fieldName) } } From dc10fc7c9bdba049afae966c6d69dc4bd2ff22da Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Fri, 7 Apr 2023 17:52:02 +0200 Subject: [PATCH 07/13] fix codacy --- management/server/mock_server/account_mock.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/management/server/mock_server/account_mock.go b/management/server/mock_server/account_mock.go index 7fd8a58a7f1..92bcd149651 100644 --- a/management/server/mock_server/account_mock.go +++ b/management/server/mock_server/account_mock.go @@ -15,7 +15,7 @@ import ( type MockAccountManager struct { GetOrCreateAccountByUserFunc func(userId, domain string) (*server.Account, error) - GetAccountByUserIDFunc func(userId string) (*server.Account, error) + GetAccountByUserIDFunc func(userID string) (*server.Account, error) CreateSetupKeyFunc func(accountId string, keyName string, keyType server.SetupKeyType, expiresIn time.Duration, autoGroups []string, usageLimit int, userID string) (*server.SetupKey, error) GetSetupKeyFunc func(accountID, userID, keyID string) (*server.SetupKey, error) @@ -114,9 +114,9 @@ func (am *MockAccountManager) GetOrCreateAccountByUser( } // GetAccountByUserID mock implementation of GetAccountByUserID from server.AccountManager interface -func (am *MockAccountManager) GetAccountByUserID(userId string) (*server.Account, error) { +func (am *MockAccountManager) GetAccountByUserID(userID string) (*server.Account, error) { if am.GetAccountByUserIDFunc != nil { - return am.GetAccountByUserIDFunc(userId) + return am.GetAccountByUserIDFunc(userID) } return nil, status.Errorf(codes.Unimplemented, "method GetAccountByUserID is not implemented") } From 372686f31739140588877b2f1f4a1863eeb854c8 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Fri, 7 Apr 2023 17:52:02 +0200 Subject: [PATCH 08/13] fix codacy --- management/server/mock_server/account_mock.go | 6 +++--- management/server/user.go | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/management/server/mock_server/account_mock.go b/management/server/mock_server/account_mock.go index 7fd8a58a7f1..92bcd149651 100644 --- a/management/server/mock_server/account_mock.go +++ b/management/server/mock_server/account_mock.go @@ -15,7 +15,7 @@ import ( type MockAccountManager struct { GetOrCreateAccountByUserFunc func(userId, domain string) (*server.Account, error) - GetAccountByUserIDFunc func(userId string) (*server.Account, error) + GetAccountByUserIDFunc func(userID string) (*server.Account, error) CreateSetupKeyFunc func(accountId string, keyName string, keyType server.SetupKeyType, expiresIn time.Duration, autoGroups []string, usageLimit int, userID string) (*server.SetupKey, error) GetSetupKeyFunc func(accountID, userID, keyID string) (*server.SetupKey, error) @@ -114,9 +114,9 @@ func (am *MockAccountManager) GetOrCreateAccountByUser( } // GetAccountByUserID mock implementation of GetAccountByUserID from server.AccountManager interface -func (am *MockAccountManager) GetAccountByUserID(userId string) (*server.Account, error) { +func (am *MockAccountManager) GetAccountByUserID(userID string) (*server.Account, error) { if am.GetAccountByUserIDFunc != nil { - return am.GetAccountByUserIDFunc(userId) + return am.GetAccountByUserIDFunc(userID) } return nil, status.Errorf(codes.Unimplemented, "method GetAccountByUserID is not implemented") } diff --git a/management/server/user.go b/management/server/user.go index b1b81af243f..6996cd6b101 100644 --- a/management/server/user.go +++ b/management/server/user.go @@ -171,9 +171,8 @@ func (am *DefaultAccountManager) createServiceUser(accountID string, role UserRo func (am *DefaultAccountManager) CreateUser(accountID, userID string, user *UserInfo) (*UserInfo, error) { if user.IsServiceUser { return am.createServiceUser(accountID, StrRoleToUserRole(user.Role), user.Name, user.AutoGroups) - } else { - return am.inviteNewUser(accountID, userID, user) } + return am.inviteNewUser(accountID, userID, user) } // inviteNewUser Invites a USer to a given account and creates reference in datastore @@ -242,6 +241,7 @@ func (am *DefaultAccountManager) inviteNewUser(accountID, userID string, invite } +// DeleteUser deletes a user from the given account. func (am *DefaultAccountManager) DeleteUser(accountID, targetUserID string) error { unlock := am.Store.AcquireAccountLock(accountID) defer unlock() From a2418441d97bd3c6d66e2cfe23bbbb75d3d89dce Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Mon, 10 Apr 2023 11:48:04 +0200 Subject: [PATCH 09/13] don't check redeem invite for service users --- management/server/account.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/management/server/account.go b/management/server/account.go index 9651535acc7..26ad4005670 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -1231,9 +1231,11 @@ func (am *DefaultAccountManager) GetAccountFromToken(claims jwtclaims.Authorizat return nil, nil, status.Errorf(status.NotFound, "user %s not found", claims.UserId) } - err = am.redeemInvite(account, claims.UserId) - if err != nil { - return nil, nil, err + if !user.IsServiceUser { + err = am.redeemInvite(account, claims.UserId) + if err != nil { + return nil, nil, err + } } return account, user, nil From f9895f703219959ed85007d2767443e742a99199 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Mon, 10 Apr 2023 15:45:26 +0200 Subject: [PATCH 10/13] activity + permission check --- management/server/account.go | 4 +-- management/server/activity/codes.go | 16 +++++++++ management/server/http/users_handler.go | 4 +-- management/server/http/users_handler_test.go | 2 +- management/server/mock_server/account_mock.go | 6 ++-- management/server/user.go | 36 +++++++++++++++---- management/server/user_test.go | 10 +++--- 7 files changed, 58 insertions(+), 20 deletions(-) diff --git a/management/server/account.go b/management/server/account.go index 26ad4005670..b5b7eaea16f 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -49,8 +49,8 @@ type AccountManager interface { CreateSetupKey(accountID string, keyName string, keyType SetupKeyType, expiresIn time.Duration, autoGroups []string, usageLimit int, userID string) (*SetupKey, error) SaveSetupKey(accountID string, key *SetupKey, userID string) (*SetupKey, error) - CreateUser(accountID, userID string, key *UserInfo) (*UserInfo, error) - DeleteUser(accountID, targetUserID string) error + CreateUser(accountID, executingUserID string, key *UserInfo) (*UserInfo, error) + DeleteUser(accountID, executingUserID string, targetUserID string) error ListSetupKeys(accountID, userID string) ([]*SetupKey, error) SaveUser(accountID, userID string, update *User) (*UserInfo, error) GetSetupKey(accountID, userID, keyID string) (*SetupKey, error) diff --git a/management/server/activity/codes.go b/management/server/activity/codes.go index dacac4129fc..a3396a2f8a0 100644 --- a/management/server/activity/codes.go +++ b/management/server/activity/codes.go @@ -87,6 +87,10 @@ const ( PersonalAccessTokenCreated // PersonalAccessTokenDeleted indicates that a user deleted a personal access token PersonalAccessTokenDeleted + // ServiceUserCreated indicates that a user created a service user + ServiceUserCreated + // ServiceUserDeleted indicates that a user deleted a service user + ServiceUserDeleted ) const ( @@ -176,6 +180,10 @@ const ( PersonalAccessTokenCreatedMessage string = "Personal access token created" // PersonalAccessTokenDeletedMessage is a human-readable text message of the PersonalAccessTokenDeleted activity PersonalAccessTokenDeletedMessage string = "Personal access token deleted" + // ServiceUserCreatedMessage is a human-readable text message of the ServiceUserCreated activity + ServiceUserCreatedMessage string = "Service user created" + // ServiceUserDeletedMessage is a human-readable text message of the ServiceUserDeleted activity + ServiceUserDeletedMessage string = "Service user deleted" ) // Activity that triggered an Event @@ -270,6 +278,10 @@ func (a Activity) Message() string { return PersonalAccessTokenCreatedMessage case PersonalAccessTokenDeleted: return PersonalAccessTokenDeletedMessage + case ServiceUserCreated: + return ServiceUserCreatedMessage + case ServiceUserDeleted: + return ServiceUserDeletedMessage default: return "UNKNOWN_ACTIVITY" } @@ -364,6 +376,10 @@ func (a Activity) StringCode() string { return "personal.access.token.create" case PersonalAccessTokenDeleted: return "personal.access.token.delete" + case ServiceUserCreated: + return "service.user.create" + case ServiceUserDeleted: + return "service.user.delete" default: return "UNKNOWN_ACTIVITY" } diff --git a/management/server/http/users_handler.go b/management/server/http/users_handler.go index c6af1d5704b..bdbefe39f13 100644 --- a/management/server/http/users_handler.go +++ b/management/server/http/users_handler.go @@ -87,7 +87,7 @@ func (h *UsersHandler) DeleteUser(w http.ResponseWriter, r *http.Request) { } claims := h.claimsExtractor.FromRequestContext(r) - account, _, err := h.accountManager.GetAccountFromToken(claims) + account, user, err := h.accountManager.GetAccountFromToken(claims) if err != nil { util.WriteError(err, w) return @@ -100,7 +100,7 @@ func (h *UsersHandler) DeleteUser(w http.ResponseWriter, r *http.Request) { return } - err = h.accountManager.DeleteUser(account.Id, targetUserID) + err = h.accountManager.DeleteUser(account.Id, user.Id, targetUserID) if err != nil { util.WriteError(err, w) return diff --git a/management/server/http/users_handler_test.go b/management/server/http/users_handler_test.go index 88697aeaa83..7b3bdb92a67 100644 --- a/management/server/http/users_handler_test.go +++ b/management/server/http/users_handler_test.go @@ -70,7 +70,7 @@ func initUsersTestData() *UsersHandler { } return key, nil }, - DeleteUserFunc: func(accountID string, targetUserID string) error { + DeleteUserFunc: func(accountID string, executingUserID string, targetUserID string) error { if targetUserID == notFoundUserID { return status.Errorf(status.NotFound, "user with ID %s does not exists", targetUserID) } diff --git a/management/server/mock_server/account_mock.go b/management/server/mock_server/account_mock.go index 92bcd149651..b39d27b27cd 100644 --- a/management/server/mock_server/account_mock.go +++ b/management/server/mock_server/account_mock.go @@ -61,7 +61,7 @@ type MockAccountManager struct { SaveSetupKeyFunc func(accountID string, key *server.SetupKey, userID string) (*server.SetupKey, error) ListSetupKeysFunc func(accountID, userID string) ([]*server.SetupKey, error) SaveUserFunc func(accountID, userID string, user *server.User) (*server.UserInfo, error) - DeleteUserFunc func(accountID string, targetUserID string) error + DeleteUserFunc func(accountID string, executingUserID string, targetUserID string) error CreatePATFunc func(accountID string, executingUserID string, targetUserId string, tokenName string, expiresIn int) (*server.PersonalAccessTokenGenerated, error) DeletePATFunc func(accountID string, executingUserID string, targetUserId string, tokenID string) error GetPATFunc func(accountID string, executingUserID string, targetUserId string, tokenID string) (*server.PersonalAccessToken, error) @@ -502,9 +502,9 @@ func (am *MockAccountManager) SaveUser(accountID, userID string, user *server.Us } // DeleteUser mocks DeleteUser of the AccountManager interface -func (am *MockAccountManager) DeleteUser(accountID string, targetUserID string) error { +func (am *MockAccountManager) DeleteUser(accountID string, executingUserID string, targetUserID string) error { if am.DeleteUserFunc != nil { - return am.DeleteUserFunc(accountID, targetUserID) + return am.DeleteUserFunc(accountID, executingUserID, targetUserID) } return status.Errorf(codes.Unimplemented, "method DeleteUser is not implemented") } diff --git a/management/server/user.go b/management/server/user.go index 6996cd6b101..b3b7b533f2d 100644 --- a/management/server/user.go +++ b/management/server/user.go @@ -137,7 +137,7 @@ func NewAdminUser(id string) *User { } // createServiceUser creates a new service user under the given account. -func (am *DefaultAccountManager) createServiceUser(accountID string, role UserRole, serviceUserName string, autoGroups []string) (*UserInfo, error) { +func (am *DefaultAccountManager) createServiceUser(accountID string, executingUserID string, role UserRole, serviceUserName string, autoGroups []string) (*UserInfo, error) { unlock := am.Store.AcquireAccountLock(accountID) defer unlock() @@ -146,6 +146,14 @@ func (am *DefaultAccountManager) createServiceUser(accountID string, role UserRo return nil, status.Errorf(status.NotFound, "account %s doesn't exist", accountID) } + executingUser := account.Users[executingUserID] + if executingUser == nil { + return nil, status.Errorf(status.NotFound, "user not found") + } + if executingUser.Role != UserRoleAdmin { + return nil, status.Errorf(status.PermissionDenied, "only admins can create service users") + } + newUserID := uuid.New().String() newUser := NewUser(newUserID, role, true, serviceUserName, autoGroups) log.Debugf("New User: %v", newUser) @@ -156,6 +164,9 @@ func (am *DefaultAccountManager) createServiceUser(accountID string, role UserRo return nil, err } + meta := map[string]any{"name": newUser.ServiceUserName} + am.storeEvent(executingUserID, newUser.Id, accountID, activity.ServiceUserCreated, meta) + return &UserInfo{ ID: newUser.Id, Email: "", @@ -170,7 +181,7 @@ func (am *DefaultAccountManager) createServiceUser(accountID string, role UserRo // 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, StrRoleToUserRole(user.Role), user.Name, user.AutoGroups) + return am.createServiceUser(accountID, userID, StrRoleToUserRole(user.Role), user.Name, user.AutoGroups) } return am.inviteNewUser(accountID, userID, user) } @@ -242,7 +253,7 @@ func (am *DefaultAccountManager) inviteNewUser(accountID, userID string, invite } // DeleteUser deletes a user from the given account. -func (am *DefaultAccountManager) DeleteUser(accountID, targetUserID string) error { +func (am *DefaultAccountManager) DeleteUser(accountID, executingUserID string, targetUserID string) error { unlock := am.Store.AcquireAccountLock(accountID) defer unlock() @@ -256,10 +267,21 @@ func (am *DefaultAccountManager) DeleteUser(accountID, targetUserID string) erro return status.Errorf(status.NotFound, "user not found") } + executingUser := account.Users[executingUserID] + if executingUser == nil { + return status.Errorf(status.NotFound, "user not found") + } + if executingUser.Role != UserRoleAdmin { + return status.Errorf(status.PermissionDenied, "only admins can delete service users") + } + if !targetUser.IsServiceUser { return status.Errorf(status.PermissionDenied, "regular users can not be deleted") } + meta := map[string]any{"name": targetUser.ServiceUserName} + am.storeEvent(executingUserID, targetUserID, accountID, activity.ServiceUserDeleted, meta) + delete(account.Users, targetUserID) err = am.Store.SaveAccount(account) @@ -321,7 +343,7 @@ func (am *DefaultAccountManager) CreatePAT(accountID string, executingUserID str return nil, status.Errorf(status.Internal, "failed to save account: %v", err) } - meta := map[string]any{"name": pat.Name} + meta := map[string]any{"name": pat.Name, "is_service_user": targetUser.IsServiceUser, "user_name": targetUser.ServiceUserName} am.storeEvent(executingUserID, targetUserID, accountID, activity.PersonalAccessTokenCreated, meta) return pat, nil @@ -365,7 +387,7 @@ func (am *DefaultAccountManager) DeletePAT(accountID string, executingUserID str return status.Errorf(status.Internal, "Failed to delete hashed token index: %s", err) } - meta := map[string]any{"name": pat.Name} + meta := map[string]any{"name": pat.Name, "is_service_user": targetUser.IsServiceUser, "user_name": targetUser.ServiceUserName} am.storeEvent(executingUserID, targetUserID, accountID, activity.PersonalAccessTokenDeleted, meta) delete(targetUser.PATs, tokenID) @@ -490,7 +512,7 @@ func (am *DefaultAccountManager) SaveUser(accountID, userID string, update *User group := account.GetGroup(g) if group != nil { am.storeEvent(userID, oldUser.Id, accountID, activity.GroupRemovedFromUser, - map[string]any{"group": group.Name, "group_id": group.ID}) + map[string]any{"group": group.Name, "group_id": group.ID, "is_service_user": newUser.IsServiceUser, "user_name": newUser.ServiceUserName}) } else { log.Errorf("group %s not found while saving user activity event of account %s", g, account.Id) } @@ -501,7 +523,7 @@ func (am *DefaultAccountManager) SaveUser(accountID, userID string, update *User group := account.GetGroup(g) if group != nil { am.storeEvent(userID, oldUser.Id, accountID, activity.GroupAddedToUser, - map[string]any{"group": group.Name, "group_id": group.ID}) + map[string]any{"group": group.Name, "group_id": group.ID, "is_service_user": newUser.IsServiceUser, "user_name": newUser.ServiceUserName}) } else { log.Errorf("group %s not found while saving user activity event of account %s", g, account.Id) } diff --git a/management/server/user_test.go b/management/server/user_test.go index 9d4eef9bf7e..997c908ba3d 100644 --- a/management/server/user_test.go +++ b/management/server/user_test.go @@ -282,7 +282,7 @@ func TestUser_CreateServiceUser(t *testing.T) { eventStore: &activity.InMemoryEventStore{}, } - user, err := am.createServiceUser(mockAccountID, mockRole, mockServiceUserName, []string{"group1", "group2"}) + user, err := am.createServiceUser(mockAccountID, mockUserID, mockRole, mockServiceUserName, []string{"group1", "group2"}) if err != nil { t.Fatalf("Error when creating service user: %s", err) } @@ -314,7 +314,7 @@ func TestUser_CreateUser_ServiceUser(t *testing.T) { eventStore: &activity.InMemoryEventStore{}, } - user, err := am.CreateUser(mockAccountID, "", &UserInfo{ + user, err := am.CreateUser(mockAccountID, mockUserID, &UserInfo{ Name: mockServiceUserName, Role: mockRole, IsServiceUser: true, @@ -352,7 +352,7 @@ func TestUser_CreateUser_RegularUser(t *testing.T) { eventStore: &activity.InMemoryEventStore{}, } - _, err = am.CreateUser(mockAccountID, "", &UserInfo{ + _, err = am.CreateUser(mockAccountID, mockUserID, &UserInfo{ Name: mockServiceUserName, Role: mockRole, IsServiceUser: false, @@ -381,7 +381,7 @@ func TestUser_DeleteUser_ServiceUser(t *testing.T) { eventStore: &activity.InMemoryEventStore{}, } - err = am.DeleteUser(mockAccountID, mockServiceUserID) + err = am.DeleteUser(mockAccountID, mockUserID, mockServiceUserID) if err != nil { t.Fatalf("Error when deleting user: %s", err) } @@ -404,7 +404,7 @@ func TestUser_DeleteUser_regularUser(t *testing.T) { eventStore: &activity.InMemoryEventStore{}, } - err = am.DeleteUser(mockAccountID, mockUserID) + err = am.DeleteUser(mockAccountID, mockUserID, mockUserID) assert.Errorf(t, err, "Regular users can not be deleted (yet)") } From e828ee00dd4036d54b1bdc9061628606b1017864 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Thu, 13 Apr 2023 12:59:44 +0200 Subject: [PATCH 11/13] remove unused code and fix comment --- management/server/user.go | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/management/server/user.go b/management/server/user.go index b3b7b533f2d..ae22415b8d8 100644 --- a/management/server/user.go +++ b/management/server/user.go @@ -126,7 +126,7 @@ func NewUser(id string, role UserRole, isServiceUser bool, serviceUserName strin } } -// NewRegularUser creates a new user with role UserRoleAdmin +// NewRegularUser creates a new user with role UserRoleUser func NewRegularUser(id string) *User { return NewUser(id, UserRoleUser, false, "", []string{}) } @@ -289,13 +289,6 @@ func (am *DefaultAccountManager) DeleteUser(accountID, executingUserID string, t return err } - // refresh only needs to happen if we support delete of regular users - - // _, err = am.refreshCache(account.Id) - // if err != nil { - // return err - // } - return nil } From 00e05278c40b63705297ebd94afad6b54960165f Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Mon, 17 Apr 2023 17:20:25 +0200 Subject: [PATCH 12/13] fix created by wrong ID and update tests --- management/server/user.go | 2 +- management/server/user_test.go | 29 ++++++++++++++++++++++++++++- 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/management/server/user.go b/management/server/user.go index ae22415b8d8..00eeb83c701 100644 --- a/management/server/user.go +++ b/management/server/user.go @@ -324,7 +324,7 @@ func (am *DefaultAccountManager) CreatePAT(accountID string, executingUserID str return nil, status.Errorf(status.PermissionDenied, "no permission to create PAT for this user") } - pat, err := CreateNewPAT(tokenName, expiresIn, targetUser.Id) + pat, err := CreateNewPAT(tokenName, expiresIn, executingUser.Id) if err != nil { return nil, status.Errorf(status.Internal, "failed to create PAT: %v", err) } diff --git a/management/server/user_test.go b/management/server/user_test.go index 997c908ba3d..d407b11ded0 100644 --- a/management/server/user_test.go +++ b/management/server/user_test.go @@ -48,6 +48,8 @@ func TestUser_CreatePAT_ForSameUser(t *testing.T) { t.Fatalf("Error when adding PAT to user: %s", err) } + assert.Equal(t, pat.CreatedBy, mockUserID) + fileStore := am.Store.(*FileStore) tokenID := fileStore.HashedPAT2TokenID[pat.HashedToken] @@ -67,7 +69,10 @@ func TestUser_CreatePAT_ForSameUser(t *testing.T) { func TestUser_CreatePAT_ForDifferentUser(t *testing.T) { store := newStore(t) account := newAccountWithId(mockAccountID, mockUserID, "") - + account.Users[mockTargetUserId] = &User{ + Id: mockTargetUserId, + IsServiceUser: false, + } err := store.SaveAccount(account) if err != nil { t.Fatalf("Error when saving account: %s", err) @@ -82,6 +87,28 @@ func TestUser_CreatePAT_ForDifferentUser(t *testing.T) { assert.Errorf(t, err, "Creating PAT for different user should thorw error") } +func TestUser_CreatePAT_ForServiceUser(t *testing.T) { + store := newStore(t) + account := newAccountWithId(mockAccountID, mockUserID, "") + account.Users[mockTargetUserId] = &User{ + Id: mockTargetUserId, + IsServiceUser: true, + } + err := store.SaveAccount(account) + if err != nil { + t.Fatalf("Error when saving account: %s", err) + } + + am := DefaultAccountManager{ + Store: store, + eventStore: &activity.InMemoryEventStore{}, + } + + pat, err := am.CreatePAT(mockAccountID, mockUserID, mockTargetUserId, mockTokenName, mockExpiresIn) + + assert.Equal(t, pat.CreatedBy, mockUserID) +} + func TestUser_CreatePAT_WithWrongExpiration(t *testing.T) { store := newStore(t) account := newAccountWithId(mockAccountID, mockUserID, "") From 7ca1eabc57c5e820b4969d86ec3a3667b2492b6e Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Mon, 17 Apr 2023 17:42:58 +0200 Subject: [PATCH 13/13] add error handling in test --- management/server/user_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/management/server/user_test.go b/management/server/user_test.go index d407b11ded0..b9876be4e4c 100644 --- a/management/server/user_test.go +++ b/management/server/user_test.go @@ -105,6 +105,9 @@ func TestUser_CreatePAT_ForServiceUser(t *testing.T) { } pat, err := am.CreatePAT(mockAccountID, mockUserID, mockTargetUserId, mockTokenName, mockExpiresIn) + if err != nil { + t.Fatalf("Error when adding PAT to user: %s", err) + } assert.Equal(t, pat.CreatedBy, mockUserID) }