Skip to content

check twice for forbidden before return (fixes #554)#555

Merged
flbla merged 1 commit intogoharbor:mainfrom
shoeffner:forbidden-check
Nov 26, 2025
Merged

check twice for forbidden before return (fixes #554)#555
flbla merged 1 commit intogoharbor:mainfrom
shoeffner:forbidden-check

Conversation

@shoeffner
Copy link
Contributor

5248a3a introduced better error handling for FORBIDDEN errors.
This caused a slight regression in the authentication handling for OIDC logins (introduced in #497).

This PR moves the error message outside of the "retry" block.

@shoeffner shoeffner requested a review from a team as a code owner November 10, 2025 18:28
Signed-off-by: Sebastian Höffner <info@sebastian-hoeffner.de>
@shoeffner
Copy link
Contributor Author

I signed-off the commit and force-pushed it, the changes remain the same.

@flbla
Copy link
Contributor

flbla commented Nov 20, 2025

hi @shoeffner
I don't understand why you want to add another if resp.StatusCode == http.StatusForbidden ?

@shoeffner
Copy link
Contributor Author

Hm, you are right, using an else instead might already do the trick, or am I missing something? Have to try it out though.

diff --git a/client/client.go b/client/client.go
index 09564f1..40dc56c 100644
--- a/client/client.go
+++ b/client/client.go
@@ -116,8 +116,9 @@ func (c *Client) SendRequest(method string, path string, payload interface{}, st
 					return "", "", http.StatusBadGateway, err
 				}
 			}
+		} else {
+			return "", "", resp.StatusCode, fmt.Errorf("[ERROR] forbidden: status=%s, code=%d \nIf you are using a robot account, this is likely due to RBAC limitations. See: https://github.com/goharbor/community/blob/main/proposals/new/Robot-Account-Expand.md", resp.Status, resp.StatusCode)
 		}
-		return "", "", resp.StatusCode, fmt.Errorf("[ERROR] forbidden: status=%s, code=%d \nIf you are using a robot account, this is likely due to RBAC limitations. See: https://github.com/goharbor/community/blob/main/proposals/new/Robot-Account-Expand.md", resp.Status, resp.StatusCode)
 	}
 
 	body, err := io.ReadAll(resp.Body)

@shoeffner
Copy link
Contributor Author

No, I remember: The reason is that otherwise an unsuccessful login attempt via OIDC will not get the same error message but fall into return "", "", resp.StatusCode, fmt.Errorf("[ERROR] unexpected status code got: %v expected: %v \n %v", resp.StatusCode, statusCode, strbody).

With the second check, we can present the forbidden message to both, actual robots and failed OIDC attempts (because the resp object is overwritten with the second c.sendRequestWithHeaders call).

@flbla
Copy link
Contributor

flbla commented Nov 26, 2025

indeed, thank you!

@flbla flbla merged commit 67cfed5 into goharbor:main Nov 26, 2025
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants