From 72beb7479a6a586e05de382fc9dc19aa17759c75 Mon Sep 17 00:00:00 2001 From: Rayyan Khan Date: Sat, 7 Feb 2026 14:21:51 +0530 Subject: [PATCH 1/3] test/refactored user elevate cmd and wrote tests for the same Signed-off-by: Rayyan Khan --- cmd/harbor/root/user/elevate.go | 98 +++++++++----- cmd/harbor/root/user/elevate_test.go | 185 +++++++++++++++++++++++++++ 2 files changed, 250 insertions(+), 33 deletions(-) create mode 100644 cmd/harbor/root/user/elevate_test.go diff --git a/cmd/harbor/root/user/elevate.go b/cmd/harbor/root/user/elevate.go index 060f8380..53bdcfea 100644 --- a/cmd/harbor/root/user/elevate.go +++ b/cmd/harbor/root/user/elevate.go @@ -21,6 +21,69 @@ import ( "github.com/spf13/cobra" ) +type UserElevator interface { + GetUserIDByName(username string) (int64, error) + GetUserIDFromUser() int64 + ConfirmElevation() (bool, error) + ElevateUser(userID int64) error +} + +type DefaultUserElevator struct{} + +func (d *DefaultUserElevator) GetUserIDByName(username string) (int64, error) { + return api.GetUsersIdByName(username) +} + +func (d *DefaultUserElevator) GetUserIDFromUser() int64 { + return prompt.GetUserIdFromUser() +} + +func (d *DefaultUserElevator) ConfirmElevation() (bool, error) { + return views.ConfirmElevation() +} + +func (d *DefaultUserElevator) ElevateUser(userID int64) error { + return api.ElevateUser(userID) +} + +func ElevateUser(args []string, userElevator UserElevator) { + var err error + var userID int64 + + if len(args) > 0 { + userID, err = userElevator.GetUserIDByName(args[0]) + if err != nil { + log.Errorf("failed to get user id for '%s': %v", args[0], err) + return + } + if userID == 0 { + log.Errorf("User with name '%s' not found", args[0]) + return + } + } else { + userID = userElevator.GetUserIDFromUser() + } + + confirm, err := userElevator.ConfirmElevation() + if err != nil { + log.Errorf("failed to confirm elevation: %v", err) + return + } + if !confirm { + log.Error("User did not confirm elevation. Aborting command.") + return + } + + err = userElevator.ElevateUser(userID) + if err != nil { + if isUnauthorizedError(err) { + log.Error("Permission denied: Admin privileges are required to execute this command.") + } else { + log.Errorf("failed to elevate user: %v", err) + } + } +} + func ElevateUserCmd() *cobra.Command { cmd := &cobra.Command{ Use: "elevate", @@ -28,39 +91,8 @@ func ElevateUserCmd() *cobra.Command { Long: "elevate user to admin role", Args: cobra.MaximumNArgs(1), Run: func(cmd *cobra.Command, args []string) { - var err error - var userId int64 - if len(args) > 0 { - userId, err = api.GetUsersIdByName(args[0]) - if err != nil { - log.Errorf("failed to get user id for '%s': %v", args[0], err) - return - } - if userId == 0 { - log.Errorf("User with name '%s' not found", args[0]) - return - } - } else { - userId = prompt.GetUserIdFromUser() - } - confirm, err := views.ConfirmElevation() - if err != nil { - log.Errorf("failed to confirm elevation: %v", err) - return - } - if !confirm { - log.Error("User did not confirm elevation. Aborting command.") - return - } - - err = api.ElevateUser(userId) - if err != nil { - if isUnauthorizedError(err) { - log.Error("Permission denied: Admin privileges are required to execute this command.") - } else { - log.Errorf("failed to elevate user: %v", err) - } - } + d := &DefaultUserElevator{} + ElevateUser(args, d) }, } diff --git a/cmd/harbor/root/user/elevate_test.go b/cmd/harbor/root/user/elevate_test.go new file mode 100644 index 00000000..efcde6e5 --- /dev/null +++ b/cmd/harbor/root/user/elevate_test.go @@ -0,0 +1,185 @@ +// Copyright Project Harbor Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package user + +import ( + "bytes" + "fmt" + "testing" + + log "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" +) + +type MockUserElevator struct { + id map[string]int64 + admins map[int64]bool + userCnt int + expectAuthError bool + confirmElevation bool + confirmElevationErr error +} + +func (m *MockUserElevator) GetUserIDByName(username string) (int64, error) { + if v, ok := m.id[username]; ok { + return v, nil + } + return 0, fmt.Errorf("username %s not found", username) +} + +func (m *MockUserElevator) GetUserIDFromUser() int64 { + return 999 +} + +func (m *MockUserElevator) ConfirmElevation() (bool, error) { + return m.confirmElevation, m.confirmElevationErr +} + +func (m *MockUserElevator) ElevateUser(userID int64) error { + if m.expectAuthError { + return fmt.Errorf("403") + } + if _, ok := m.admins[userID]; !ok { + m.admins[userID] = true + return nil + } + return fmt.Errorf("user %d is already an admin", userID) +} + +func initMockUserElevator(userCnt int, expectAuthError bool, confirmElevation bool, confirmElevationErr error) *MockUserElevator { + m := &MockUserElevator{ + userCnt: userCnt, + expectAuthError: expectAuthError, + confirmElevation: confirmElevation, + confirmElevationErr: confirmElevationErr, + id: make(map[string]int64), + admins: make(map[int64]bool), + } + for i := 0; i < userCnt; i++ { + m.id[fmt.Sprintf("test%d", i+1)] = int64(i + 1) + } + return m +} + +func TestElevateUser(t *testing.T) { + tests := []struct { + name string + setup func() *MockUserElevator + args []string + expectedAdminID []int64 + expectedErr string + }{ + { + name: "successfully elevate user by username", + setup: func() *MockUserElevator { + return initMockUserElevator(5, false, true, nil) + }, + args: []string{"test1"}, + expectedAdminID: []int64{1}, + expectedErr: "", + }, + { + name: "elevate user via interactive prompt", + setup: func() *MockUserElevator { + m := initMockUserElevator(5, false, true, nil) + m.id["promptuser"] = 999 + return m + }, + args: []string{}, + expectedAdminID: []int64{999}, + expectedErr: "", + }, + { + name: "user not found logs error", + setup: func() *MockUserElevator { + return initMockUserElevator(5, false, true, nil) + }, + args: []string{"nonexistent"}, + expectedAdminID: []int64{}, + expectedErr: "failed to get user id", + }, + { + name: "permission denied error", + setup: func() *MockUserElevator { + return initMockUserElevator(5, true, true, nil) + }, + args: []string{"test1"}, + expectedAdminID: []int64{}, + expectedErr: "Permission denied", + }, + { + name: "user declines elevation confirmation", + setup: func() *MockUserElevator { + return initMockUserElevator(5, false, false, nil) + }, + args: []string{"test1"}, + expectedAdminID: []int64{}, + expectedErr: "User did not confirm elevation", + }, + { + name: "confirmation prompt returns error", + setup: func() *MockUserElevator { + return initMockUserElevator(5, false, false, fmt.Errorf("terminal error")) + }, + args: []string{"test1"}, + expectedAdminID: []int64{}, + expectedErr: "failed to confirm elevation", + }, + { + name: "elevate user that is already admin", + setup: func() *MockUserElevator { + m := initMockUserElevator(5, false, true, nil) + m.admins[1] = true + return m + }, + args: []string{"test1"}, + expectedAdminID: []int64{1}, + expectedErr: "already an admin", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var buf bytes.Buffer + originalLogOutput := log.StandardLogger().Out + log.SetOutput(&buf) + defer log.SetOutput(originalLogOutput) + + m := tt.setup() + ElevateUser(tt.args, m) + + logs := buf.String() + + if tt.expectedErr != "" { + assert.Contains(t, logs, tt.expectedErr, "Expected error logs to contain %s but got %s", tt.expectedErr, logs) + } else { + assert.Empty(t, logs, "Expected no error logs but got: %s", logs) + } + + for _, id := range tt.expectedAdminID { + isAdmin, exists := m.admins[id] + assert.True(t, exists && isAdmin, "User with ID %d should be an admin", id) + } + }) + } +} + +func TestElevateUserCmd(t *testing.T) { + cmd := ElevateUserCmd() + + assert.Equal(t, "elevate", cmd.Use) + assert.Equal(t, "elevate user", cmd.Short) + assert.Equal(t, "elevate user to admin role", cmd.Long) + assert.NotNil(t, cmd.Args, "Args validator should be set") +} From 459434b6a9ff7c24a95537e6c2dd9da906c7e9a3 Mon Sep 17 00:00:00 2001 From: Rayyan Khan Date: Sat, 7 Feb 2026 17:47:50 +0530 Subject: [PATCH 2/3] tests/added a test case suggested in review by copilot Signed-off-by: Rayyan Khan --- cmd/harbor/root/user/elevate_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/cmd/harbor/root/user/elevate_test.go b/cmd/harbor/root/user/elevate_test.go index efcde6e5..2eaf5257 100644 --- a/cmd/harbor/root/user/elevate_test.go +++ b/cmd/harbor/root/user/elevate_test.go @@ -109,6 +109,17 @@ func TestElevateUser(t *testing.T) { expectedAdminID: []int64{}, expectedErr: "failed to get user id", }, + { + name: "user id is zero logs not found", + setup: func() *MockUserElevator { + m := initMockUserElevator(5, false, true, nil) + m.id["ghost"] = 0 + return m + }, + args: []string{"ghost"}, + expectedAdminID: []int64{}, + expectedErr: "not found", + }, { name: "permission denied error", setup: func() *MockUserElevator { From 526420ba1f9db2ac7ba7e0ca700f39a33d681562 Mon Sep 17 00:00:00 2001 From: Rayyan Khan Date: Sat, 21 Feb 2026 11:31:28 +0530 Subject: [PATCH 3/3] changed the test design to avoid using interface for mocking, reduced tight coupling of tests Signed-off-by: Rayyan Khan --- cmd/harbor/root/user/elevate.go | 49 +++++--------- cmd/harbor/root/user/elevate_test.go | 95 ++++++++++++++++------------ 2 files changed, 69 insertions(+), 75 deletions(-) diff --git a/cmd/harbor/root/user/elevate.go b/cmd/harbor/root/user/elevate.go index 53bdcfea..f318b8c7 100644 --- a/cmd/harbor/root/user/elevate.go +++ b/cmd/harbor/root/user/elevate.go @@ -21,63 +21,45 @@ import ( "github.com/spf13/cobra" ) -type UserElevator interface { - GetUserIDByName(username string) (int64, error) - GetUserIDFromUser() int64 - ConfirmElevation() (bool, error) - ElevateUser(userID int64) error -} - -type DefaultUserElevator struct{} - -func (d *DefaultUserElevator) GetUserIDByName(username string) (int64, error) { - return api.GetUsersIdByName(username) -} - -func (d *DefaultUserElevator) GetUserIDFromUser() int64 { - return prompt.GetUserIdFromUser() -} - -func (d *DefaultUserElevator) ConfirmElevation() (bool, error) { - return views.ConfirmElevation() -} - -func (d *DefaultUserElevator) ElevateUser(userID int64) error { - return api.ElevateUser(userID) -} +var ( + getUsersIDByName = api.GetUsersIdByName + getUserIDFromUser = prompt.GetUserIdFromUser + confirmElevation = views.ConfirmElevation + elevateUserAPI = api.ElevateUser +) -func ElevateUser(args []string, userElevator UserElevator) { +func ElevateUser(args []string) { var err error var userID int64 if len(args) > 0 { - userID, err = userElevator.GetUserIDByName(args[0]) + userID, err = getUsersIDByName(args[0]) if err != nil { log.Errorf("failed to get user id for '%s': %v", args[0], err) return } if userID == 0 { - log.Errorf("User with name '%s' not found", args[0]) + log.Errorf("user with name '%s' not found", args[0]) return } } else { - userID = userElevator.GetUserIDFromUser() + userID = getUserIDFromUser() } - confirm, err := userElevator.ConfirmElevation() + confirm, err := confirmElevation() if err != nil { log.Errorf("failed to confirm elevation: %v", err) return } if !confirm { - log.Error("User did not confirm elevation. Aborting command.") + log.Error("user did not confirm elevation. Aborting command.") return } - err = userElevator.ElevateUser(userID) + err = elevateUserAPI(userID) if err != nil { if isUnauthorizedError(err) { - log.Error("Permission denied: Admin privileges are required to execute this command.") + log.Error("permission denied: Admin privileges are required to execute this command.") } else { log.Errorf("failed to elevate user: %v", err) } @@ -91,8 +73,7 @@ func ElevateUserCmd() *cobra.Command { Long: "elevate user to admin role", Args: cobra.MaximumNArgs(1), Run: func(cmd *cobra.Command, args []string) { - d := &DefaultUserElevator{} - ElevateUser(args, d) + ElevateUser(args) }, } diff --git a/cmd/harbor/root/user/elevate_test.go b/cmd/harbor/root/user/elevate_test.go index 2eaf5257..47ebdb4e 100644 --- a/cmd/harbor/root/user/elevate_test.go +++ b/cmd/harbor/root/user/elevate_test.go @@ -22,31 +22,30 @@ import ( "github.com/stretchr/testify/assert" ) -type MockUserElevator struct { - id map[string]int64 - admins map[int64]bool - userCnt int - expectAuthError bool - confirmElevation bool - confirmElevationErr error +type mockUserElevator struct { + id map[string]int64 + admins map[int64]bool + expectAuthError bool + confirmElevationFromUser bool + confirmElevationErr error } -func (m *MockUserElevator) GetUserIDByName(username string) (int64, error) { +func (m *mockUserElevator) getUserIDByName(username string) (int64, error) { if v, ok := m.id[username]; ok { return v, nil } return 0, fmt.Errorf("username %s not found", username) } -func (m *MockUserElevator) GetUserIDFromUser() int64 { +func (m *mockUserElevator) getUserIDFromUser() int64 { return 999 } -func (m *MockUserElevator) ConfirmElevation() (bool, error) { - return m.confirmElevation, m.confirmElevationErr +func (m *mockUserElevator) confirmElevation() (bool, error) { + return m.confirmElevationFromUser, m.confirmElevationErr } -func (m *MockUserElevator) ElevateUser(userID int64) error { +func (m *mockUserElevator) elevateUser(userID int64) error { if m.expectAuthError { return fmt.Errorf("403") } @@ -57,33 +56,46 @@ func (m *MockUserElevator) ElevateUser(userID int64) error { return fmt.Errorf("user %d is already an admin", userID) } -func initMockUserElevator(userCnt int, expectAuthError bool, confirmElevation bool, confirmElevationErr error) *MockUserElevator { - m := &MockUserElevator{ - userCnt: userCnt, - expectAuthError: expectAuthError, - confirmElevation: confirmElevation, - confirmElevationErr: confirmElevationErr, - id: make(map[string]int64), - admins: make(map[int64]bool), +func newmockUserElevator(userCnt int, expectAuthError bool, confirmElevationFromUser bool, confirmElevationErr error) *mockUserElevator { + m := &mockUserElevator{ + expectAuthError: expectAuthError, + confirmElevationFromUser: confirmElevationFromUser, + confirmElevationErr: confirmElevationErr, + id: make(map[string]int64), + admins: make(map[int64]bool), } for i := 0; i < userCnt; i++ { m.id[fmt.Sprintf("test%d", i+1)] = int64(i + 1) } + getUserIDFromUser = m.getUserIDFromUser + getUsersIDByName = m.getUserIDByName + confirmElevation = m.confirmElevation + elevateUserAPI = m.elevateUser return m } func TestElevateUser(t *testing.T) { + origGetUsersId := getUsersIDByName + origPrompt := getUserIDFromUser + origConfirm := confirmElevation + origElevate := elevateUserAPI + defer func() { + getUsersIDByName = origGetUsersId + getUserIDFromUser = origPrompt + confirmElevation = origConfirm + elevateUserAPI = origElevate + }() tests := []struct { name string - setup func() *MockUserElevator + setup func() *mockUserElevator args []string expectedAdminID []int64 expectedErr string }{ { name: "successfully elevate user by username", - setup: func() *MockUserElevator { - return initMockUserElevator(5, false, true, nil) + setup: func() *mockUserElevator { + return newmockUserElevator(5, false, true, nil) }, args: []string{"test1"}, expectedAdminID: []int64{1}, @@ -91,8 +103,8 @@ func TestElevateUser(t *testing.T) { }, { name: "elevate user via interactive prompt", - setup: func() *MockUserElevator { - m := initMockUserElevator(5, false, true, nil) + setup: func() *mockUserElevator { + m := newmockUserElevator(5, false, true, nil) m.id["promptuser"] = 999 return m }, @@ -102,8 +114,8 @@ func TestElevateUser(t *testing.T) { }, { name: "user not found logs error", - setup: func() *MockUserElevator { - return initMockUserElevator(5, false, true, nil) + setup: func() *mockUserElevator { + return newmockUserElevator(5, false, true, nil) }, args: []string{"nonexistent"}, expectedAdminID: []int64{}, @@ -111,8 +123,8 @@ func TestElevateUser(t *testing.T) { }, { name: "user id is zero logs not found", - setup: func() *MockUserElevator { - m := initMockUserElevator(5, false, true, nil) + setup: func() *mockUserElevator { + m := newmockUserElevator(5, false, true, nil) m.id["ghost"] = 0 return m }, @@ -122,35 +134,35 @@ func TestElevateUser(t *testing.T) { }, { name: "permission denied error", - setup: func() *MockUserElevator { - return initMockUserElevator(5, true, true, nil) + setup: func() *mockUserElevator { + return newmockUserElevator(5, true, true, nil) }, args: []string{"test1"}, expectedAdminID: []int64{}, - expectedErr: "Permission denied", + expectedErr: "permission denied", }, { name: "user declines elevation confirmation", - setup: func() *MockUserElevator { - return initMockUserElevator(5, false, false, nil) + setup: func() *mockUserElevator { + return newmockUserElevator(5, false, false, nil) }, args: []string{"test1"}, expectedAdminID: []int64{}, - expectedErr: "User did not confirm elevation", + expectedErr: "did not confirm", }, { name: "confirmation prompt returns error", - setup: func() *MockUserElevator { - return initMockUserElevator(5, false, false, fmt.Errorf("terminal error")) + setup: func() *mockUserElevator { + return newmockUserElevator(5, false, false, fmt.Errorf("terminal error")) }, args: []string{"test1"}, expectedAdminID: []int64{}, - expectedErr: "failed to confirm elevation", + expectedErr: "failed to confirm", }, { name: "elevate user that is already admin", - setup: func() *MockUserElevator { - m := initMockUserElevator(5, false, true, nil) + setup: func() *mockUserElevator { + m := newmockUserElevator(5, false, true, nil) m.admins[1] = true return m }, @@ -168,7 +180,8 @@ func TestElevateUser(t *testing.T) { defer log.SetOutput(originalLogOutput) m := tt.setup() - ElevateUser(tt.args, m) + + ElevateUser(tt.args) logs := buf.String()