opt: allow invalid api key if allow no auth, close #1201#1208
opt: allow invalid api key if allow no auth, close #1201#1208looplj merged 1 commit intorelease/v0.9.xfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request simplifies the API key authentication middleware by consolidating the fallback logic and updates error messaging in the AuthService. It also removes tests for malformed authorization headers. A review comment suggests refactoring the middleware's error handling to improve readability and avoid subtle variable mutations.
| if err == nil { | ||
| apiKey, err = auth.AuthenticateAPIKey(c.Request.Context(), key) | ||
| } | ||
| if err != nil { | ||
| if !errors.Is(err, ErrAPIKeyRequired) { | ||
| AbortWithError(c, http.StatusUnauthorized, err) | ||
| return | ||
| } | ||
|
|
||
| apiKey, err = auth.AuthenticateNoAuth(c.Request.Context()) | ||
| } else { | ||
| apiKey, err = auth.AuthenticateAPIKey(c.Request.Context(), key) | ||
| } |
There was a problem hiding this comment.
The authentication logic here is correct, but its reliance on the sequential mutation of the err variable can be a bit subtle and hard to follow. For better readability and maintainability, I suggest making the authentication flow more explicit. This refactoring clarifies that AuthenticateNoAuth is a fallback for any failure in the API key authentication path (either key extraction or validation), without changing the logic.
authErr := err
if authErr == nil {
apiKey, authErr = auth.AuthenticateAPIKey(c.Request.Context(), key)
}
if authErr != nil {
apiKey, err = auth.AuthenticateNoAuth(c.Request.Context())
} else {
err = nil
}![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
1 participant
Uh oh!
There was an error while loading. Please reload this page.