From f6aabc925a4186f9ee313d88c687c796b2eeb2e2 Mon Sep 17 00:00:00 2001 From: Seeya Vhora Date: Mon, 18 May 2026 02:12:25 +0530 Subject: [PATCH] fix(login): avoid false login failures for robot accounts During harbor login, the connection is actively verified using the GetCurrentUserInfo endpoint. However, robot accounts are system-level identities and not standard users, so GET /users/current returns a 403 Forbidden. Previously, validateClientConnection treated HTTP 403 as a definite authentication validation failure, immediately aborting the login. This broke automation and CI/CD flows utilizing robot accounts. This patch refactors validateClientConnection to: 1. Treat only HTTP 401 Unauthorized as a definite authentication failure. 2. Gracefully fall back to secondary verification endpoints (Ping & ListProjects) when GetCurrentUserInfo returns a 403 or other unexpected API/transport errors. 3. Accept HTTP 403 Forbidden from ListProjects (in addition to 200 OK) as successful verification, as long as Ping succeeds, ensuring highly restricted robot accounts can still log in successfully. Also adds a comprehensive mock-based test suite using httptest.NewServer covering human login, invalid credentials, and standard/restricted robot accounts. Signed-off-by: Seeya Vhora --- cmd/harbor/root/login.go | 16 ++--- cmd/harbor/root/login_test.go | 114 ++++++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 7 deletions(-) diff --git a/cmd/harbor/root/login.go b/cmd/harbor/root/login.go index 5b9906187..a61bc1dd3 100644 --- a/cmd/harbor/root/login.go +++ b/cmd/harbor/root/login.go @@ -218,12 +218,12 @@ func validateClientConnection(client *client.HarborAPI) error { } errorCode := utils.ParseHarborErrorCode(err) - // 401/403 = definite auth failure - if errorCode == "401" || errorCode == "403" { + // 401 = definite auth failure + if errorCode == "401" { return fmt.Errorf("authentication failed, check your credentials: %v", utils.ParseHarborErrorMsg(err)) } - // For other errors (e.g. 412 for robot/OIDC accounts, 5xx), + // For other errors (e.g. 403 for robot accounts, 412 for OIDC accounts, 5xx, or deserialization errors), // fall back to secondary endpoints to verify creds and reachability. _, projectErr := client.Project.ListProjects(ctx, &project.ListProjectsParams{ Page: new(int64(1)), @@ -231,16 +231,18 @@ func validateClientConnection(client *client.HarborAPI) error { }) _, pingErr := client.Ping.GetPing(ctx, &ping.GetPingParams{}) - // If either secondary check returns 401/403, creds are bad. + // If either secondary check returns 401, creds are bad. if projectErr != nil { projCode := utils.ParseHarborErrorCode(projectErr) - if projCode == "401" || projCode == "403" { + if projCode == "401" { return fmt.Errorf("authentication failed, check your credentials: %v", utils.ParseHarborErrorMsg(projectErr)) } } - // Both passed → creds valid, server reachable - if projectErr == nil && pingErr == nil { + // If Ping succeeds, the server is reachable. + // If ListProjects succeeded (nil) or returned 403 (valid but restricted robot account), + // we consider the client connection validated. + if pingErr == nil && (projectErr == nil || utils.ParseHarborErrorCode(projectErr) == "403") { return nil } diff --git a/cmd/harbor/root/login_test.go b/cmd/harbor/root/login_test.go index 22ae6d1ec..45bedb306 100644 --- a/cmd/harbor/root/login_test.go +++ b/cmd/harbor/root/login_test.go @@ -14,6 +14,8 @@ package root_test import ( + "net/http" + "net/http/httptest" "testing" "github.com/goharbor/harbor-cli/cmd/harbor/root" @@ -122,3 +124,115 @@ func Test_Login_Failure_MutuallyExclusiveFlags(t *testing.T) { err := cmd.Execute() assert.Error(t, err, "Expected error when both --password and --password-stdin are set") } + +func Test_Login_Validation_MockServer(t *testing.T) { + tests := []struct { + name string + currentUserStatus int + currentUserResponse string + projectsStatus int + projectsResponse string + pingStatus int + pingResponse string + expectError bool + expectedErrSubstr string + }{ + { + name: "Human Login Success", + currentUserStatus: http.StatusOK, + currentUserResponse: `{"username": "human-user", "sysadmin_flag": false}`, + expectError: false, + }, + { + name: "Invalid Credentials (401)", + currentUserStatus: http.StatusUnauthorized, + currentUserResponse: `{"errors": [{"code": "UNAUTHORIZED", "message": "unauthorized"}]}`, + expectError: true, + expectedErrSubstr: "authentication failed, check your credentials", + }, + { + name: "Robot Login Success (Standard fallback)", + currentUserStatus: http.StatusForbidden, + currentUserResponse: `{"errors": [{"code": "FORBIDDEN", "message": "forbidden"}]}`, + projectsStatus: http.StatusOK, + projectsResponse: `[]`, + pingStatus: http.StatusOK, + pingResponse: `"pong"`, + expectError: false, + }, + { + name: "Robot Login Success (Restricted project access 403 fallback)", + currentUserStatus: http.StatusForbidden, + currentUserResponse: `{"errors": [{"code": "FORBIDDEN", "message": "forbidden"}]}`, + projectsStatus: http.StatusForbidden, + projectsResponse: `{"errors": [{"code": "FORBIDDEN", "message": "forbidden"}]}`, + pingStatus: http.StatusOK, + pingResponse: `"pong"`, + expectError: false, + }, + { + name: "Robot Login Failure (Bad credentials 401 on fallback)", + currentUserStatus: http.StatusForbidden, + currentUserResponse: `{"errors": [{"code": "FORBIDDEN", "message": "forbidden"}]}`, + projectsStatus: http.StatusUnauthorized, + projectsResponse: `{"errors": [{"code": "UNAUTHORIZED", "message": "unauthorized"}]}`, + pingStatus: http.StatusOK, + pingResponse: `"pong"`, + expectError: true, + expectedErrSubstr: "authentication failed, check your credentials", + }, + { + name: "Server Error (500)", + currentUserStatus: http.StatusInternalServerError, + currentUserResponse: `Internal Server Error`, + projectsStatus: http.StatusInternalServerError, + projectsResponse: `Internal Server Error`, + pingStatus: http.StatusInternalServerError, + pingResponse: `Internal Server Error`, + expectError: true, + expectedErrSubstr: "server error", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tempDir := t.TempDir() + data := helpers.Initialize(t, tempDir) + defer helpers.ConfigCleanup(t, data) + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/api/v2.0/users/current": + w.WriteHeader(tt.currentUserStatus) + w.Write([]byte(tt.currentUserResponse)) + case "/api/v2.0/projects": + w.WriteHeader(tt.projectsStatus) + w.Write([]byte(tt.projectsResponse)) + case "/api/v2.0/ping": + w.WriteHeader(tt.pingStatus) + w.Write([]byte(tt.pingResponse)) + default: + w.WriteHeader(http.StatusNotFound) + } + })) + defer server.Close() + + cmd := root.LoginCommand() + cmd.SetArgs([]string{server.URL}) + + assert.NoError(t, cmd.Flags().Set("username", "test-user")) + assert.NoError(t, cmd.Flags().Set("password", "test-password")) + + err := cmd.Execute() + if tt.expectError { + assert.Error(t, err) + if tt.expectedErrSubstr != "" { + assert.Contains(t, err.Error(), tt.expectedErrSubstr) + } + } else { + assert.NoError(t, err) + } + }) + } +}