Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 47 additions & 39 deletions cmd/harbor/root/user/password.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,58 +15,66 @@
package user

import (
"io"

"github.com/goharbor/harbor-cli/pkg/api"
"github.com/goharbor/harbor-cli/pkg/prompt"
"github.com/goharbor/harbor-cli/pkg/views/password/reset"
log "github.com/sirupsen/logrus"
"github.com/spf13/cobra"
)

func UserPasswordChangeCmd() *cobra.Command {
var opts reset.PasswordChangeView
var (
getUserIDByName = api.GetUsersIdByName
getUserIDFromUser = prompt.GetUserIdFromUser
fillPasswordView = reset.ChangePasswordView
resetPassword = api.ResetPassword
)

func ChangePassword(cmd *cobra.Command, args []string, w io.Writer) {
var userId int64
var err error
log.SetOutput(w)
resetView := &reset.PasswordChangeView{}

if len(args) > 0 {
userId, err = 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
}
Comment on lines +40 to +49
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test coverage gap: the production implementation api.GetUsersIdByName returns (0, nil) when the user is not found, and ChangePassword has a dedicated userID == 0 branch. The tests only cover the "not found" case via an error return, so the userID == 0 branch is currently untested.

Copilot uses AI. Check for mistakes.
} else {
userId, err = getUserIDFromUser()
if err != nil {
log.Errorf("failed to get user id: %v", err)
return
}
}

fillPasswordView(resetView)

err = resetPassword(userId, *resetView)
if err != nil {
if isUnauthorizedError(err) {
log.Error("Permission denied: Admin privileges are required to execute this command.")
} else {
log.Errorf("failed to reset user password: %v", err)
}
}
}

func UserPasswordChangeCmd() *cobra.Command {
cmd := &cobra.Command{
Use: "password",
Short: "Reset user password by name or id",
Long: "Allows admin to reset the password for a specified user or select interactively if no username is provided.",
Args: cobra.MinimumNArgs(0),
Args: cobra.MaximumNArgs(1),
Run: func(cmd *cobra.Command, args []string) {
var userId int64
var err error
log.SetOutput(cmd.OutOrStderr())
resetView := &reset.PasswordChangeView{
NewPassword: opts.NewPassword,
ConfirmPassword: opts.ConfirmPassword,
}

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, err = prompt.GetUserIdFromUser()
if err != nil {
log.Errorf("failed to get user id: %v", err)
return
}
}

reset.ChangePasswordView(resetView)

err = api.ResetPassword(userId, *resetView)
if err != nil {
if isUnauthorizedError(err) {
log.Error("Permission denied: Admin privileges are required to execute this command.")
} else {
log.Errorf("failed to reset user password: %v", err)
}
}
ChangePassword(cmd, args, cmd.OutOrStderr())
},
Comment on lines +70 to 78
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR description says the tests are for the "user list" command, but the changed files and new tests are for the user password command (cmd/harbor/root/user/password.go). Please update the PR description to match the actual scope to avoid confusion during review/release notes.

Copilot uses AI. Check for mistakes.
}
return cmd
Expand Down
170 changes: 170 additions & 0 deletions cmd/harbor/root/user/password_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,15 @@
package user

import (
"strings"
"testing"

"bytes"
"fmt"

"github.com/goharbor/harbor-cli/pkg/views/password/reset"
"github.com/spf13/cobra"
"github.com/stretchr/testify/assert"
)

func TestUserPasswordChangeCmd_Metadata(t *testing.T) {
Expand Down Expand Up @@ -50,3 +56,167 @@ func TestUserPasswordChangeCmd_IsCobraCommand(t *testing.T) {
t.Fatal("expected cobra command")
}
}

type MockUserPasswordChanger struct {
id map[string]int64
passwords map[int64]string
userCnt int
expectAuthError bool
}
Comment on lines +60 to +65
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MockUserPasswordChanger.userCnt is never read in the test helper; keeping unused fields makes the mock harder to understand/maintain. Consider removing it, or using it (e.g., to drive GetUserIDFromUser behavior) so it provides value.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is used when populating the users in the init function


func (m *MockUserPasswordChanger) 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 *MockUserPasswordChanger) getUserIDFromUser() (int64, error) {
return 999, nil
}

func (m *MockUserPasswordChanger) fillPasswordView(resetView *reset.PasswordChangeView) {
resetView.NewPassword = "NewPass456"
resetView.ConfirmPassword = "NewPass456"
}

func (m *MockUserPasswordChanger) resetPassword(userID int64, resetView reset.PasswordChangeView) error {
if m.expectAuthError {
return fmt.Errorf("403")
}
if _, ok := m.passwords[userID]; !ok {
return fmt.Errorf("user %d not found", userID)
}
m.passwords[userID] = resetView.NewPassword
return nil
}

func initMockUserPasswordChanger(userCnt int, expectAuthError bool) *MockUserPasswordChanger {
m := &MockUserPasswordChanger{
userCnt: userCnt,
expectAuthError: expectAuthError,
id: make(map[string]int64),
passwords: make(map[int64]string),
}
for i := 0; i < userCnt; i++ {
m.id[fmt.Sprintf("test%d", i+1)] = int64(i + 1)
m.passwords[int64(i+1)] = "InitialPass123"
}
getUserIDByName = m.getUserIDByName
getUserIDFromUser = m.getUserIDFromUser
fillPasswordView = m.fillPasswordView
resetPassword = m.resetPassword
return m
}

func TestChangePassword(t *testing.T) {
origGetUsersId := getUserIDByName
origPrompt := getUserIDFromUser
origFillPassword := fillPasswordView
origReset := resetPassword
defer func() {
getUserIDByName = origGetUsersId
getUserIDFromUser = origPrompt
fillPasswordView = origFillPassword
resetPassword = origReset
}()
tests := []struct {
name string
setup func() *MockUserPasswordChanger
args []string
expectedPasswordID int64
expectedNewPassword string
expectedErr string
}{
{
name: "successfully change password by username",
setup: func() *MockUserPasswordChanger {
return initMockUserPasswordChanger(5, false)
},
args: []string{"test1"},
expectedPasswordID: 1,
expectedNewPassword: "NewPass456",
expectedErr: "",
},
{
name: "change password via interactive prompt",
setup: func() *MockUserPasswordChanger {
m := initMockUserPasswordChanger(5, false)
m.id["promptuser"] = 999
m.passwords[999] = "InitialPass123"
return m
Comment on lines +143 to +147
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the interactive prompt test setup, m.id["promptuser"] = 999 is unused because GetUserIDFromUser() always returns 999 and never consults m.id. Consider either removing these unused setup lines or updating GetUserIDFromUser() to derive the returned ID from the mock state to better reflect the intended scenario.

Copilot uses AI. Check for mistakes.
},
args: []string{},
expectedPasswordID: 999,
expectedNewPassword: "NewPass456",
expectedErr: "",
},
{
name: "user not found logs error",
setup: func() *MockUserPasswordChanger {
return initMockUserPasswordChanger(5, false)
},
args: []string{"nonexistent"},
expectedPasswordID: 0,
expectedNewPassword: "",
expectedErr: "failed to get user id",
},
{
name: "user id is zero logs not found",
setup: func() *MockUserPasswordChanger {
m := initMockUserPasswordChanger(5, false)
m.id["ghost"] = 0
return m
},
args: []string{"ghost"},
expectedPasswordID: 0,
expectedNewPassword: "",
expectedErr: "not found",
},
{
name: "permission denied error",
setup: func() *MockUserPasswordChanger {
return initMockUserPasswordChanger(5, true)
},
args: []string{"test1"},
expectedPasswordID: 0,
expectedNewPassword: "",
expectedErr: "permission denied",
},
{
name: "reset password fails with non-403 error",
setup: func() *MockUserPasswordChanger {
m := initMockUserPasswordChanger(5, false)
delete(m.passwords, 1)
return m
},
args: []string{"test1"},
expectedPasswordID: 0,
expectedNewPassword: "",
expectedErr: "failed to reset",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
var buf bytes.Buffer
m := tt.setup()
ChangePassword(UserPasswordChangeCmd(), tt.args, &buf)

logs := buf.String()
logs = strings.ToLower(logs)

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)
}

if tt.expectedPasswordID != 0 {
password, exists := m.passwords[tt.expectedPasswordID]
assert.True(t, exists, "User with ID %d should exist", tt.expectedPasswordID)
assert.Equal(t, tt.expectedNewPassword, password, "Password for user %d should be changed to %s", tt.expectedPasswordID, tt.expectedNewPassword)
}
})
}
}
Loading