feat: allow no auth for api, close #1104#1107
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces the concept of a 'noauth' API key, designed to allow certain API endpoints to be accessed without authentication. This feature is controlled by a new configuration option, Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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 introduces a new 'noauth' API key type, which is integrated across the application's schema, backend logic, and frontend. The changes include updating database schemas, GraphQL definitions, and API key management services to support this new type, along with a configuration option to enable or disable its use. When enabled, a system-managed 'noauth' API key is ensured on startup, and authentication middleware is adjusted to handle it. Review comments highlight a potential issue in the authentication middleware where malformed API keys might be incorrectly treated as 'noauth' requests, and consistently point out a typo in the AnthenticateAPIKey function name across multiple files.
internal/server/middleware/auth.go
Outdated
| if err != nil && allowNoAuth { | ||
| key = biz.NoAuthAPIKeyValue | ||
| } |
There was a problem hiding this comment.
The logic if err != nil && allowNoAuth { key = biz.NoAuthAPIKeyValue } might be too broad. If ExtractAPIKeyFromRequest returns an error because the key is malformed (e.g., "Bearer malformed_key"), and allowNoAuth is true, this code will treat it as a request for NoAuthAPIKeyValue. This could mask actual invalid key attempts. It might be better to only default to NoAuthAPIKeyValue if err specifically indicates that no key was provided (e.g., http.ErrNoCookie or similar, or a custom error indicating absence). If a key was provided but was invalid, it should still result in an unauthorized error.
internal/server/biz/auth_test.go
Outdated
| require.Contains(t, err.Error(), "api key project not valid") | ||
| } | ||
|
|
||
| func TestAuthService_AnthenticateAPIKey_NoAuthKeyRequiresDisabledMode(t *testing.T) { |
There was a problem hiding this comment.
The function name AnthenticateAPIKey_NoAuthKeyRequiresDisabledMode appears to have a typo and should be AuthenticateAPIKey_NoAuthKeyRequiresDisabledMode. This should be corrected for clarity and consistency.
| func TestAuthService_AnthenticateAPIKey_NoAuthKeyRequiresDisabledMode(t *testing.T) { | |
| func TestAuthService_AuthenticateAPIKey_NoAuthKeyRequiresDisabledMode(t *testing.T) { |
internal/server/biz/auth_test.go
Outdated
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), "noauth api key is only available when api auth is disabled") | ||
|
|
||
| authenticatedAPIKey, err := authService.AnthenticateAPIKey(ctx, noAuthKey.Key, true) |
There was a problem hiding this comment.
The function name AnthenticateAPIKey appears to have a typo and should be AuthenticateAPIKey. This should be corrected for clarity and consistency.
| authenticatedAPIKey, err := authService.AnthenticateAPIKey(ctx, noAuthKey.Key, true) | |
| authenticatedAPIKey, err := authService.AuthenticateAPIKey(ctx, noAuthKey.Key, true) |
internal/server/biz/auth_test.go
Outdated
|
|
||
| // Test invalid API key | ||
| _, err = authService.AnthenticateAPIKey(ctx, "invalid-api-key") | ||
| _, err = authService.AnthenticateAPIKey(ctx, "invalid-api-key", false) |
There was a problem hiding this comment.
The function name AnthenticateAPIKey appears to have a typo and should be AuthenticateAPIKey. This should be corrected for clarity and consistency.
| _, err = authService.AnthenticateAPIKey(ctx, "invalid-api-key", false) | |
| _, err = authService.AuthenticateAPIKey(ctx, "invalid-api-key", false) |
internal/server/biz/auth_test.go
Outdated
| authService.APIKeyService.APIKeyCache.Invalidate(buildAPIKeyCacheKey(apiKeyString)) | ||
|
|
||
| _, err = authService.AnthenticateAPIKey(ctx, apiKeyString) | ||
| _, err = authService.AnthenticateAPIKey(ctx, apiKeyString, false) |
There was a problem hiding this comment.
The function name AnthenticateAPIKey appears to have a typo and should be AuthenticateAPIKey. This should be corrected for clarity and consistency.
| _, err = authService.AnthenticateAPIKey(ctx, apiKeyString, false) | |
| _, err = authService.AuthenticateAPIKey(ctx, apiKeyString, false) |
internal/server/biz/auth_test.go
Outdated
|
|
||
| // Second call - cache should be expired, should hit database again | ||
| authenticatedAPIKey2, err := authService.AnthenticateAPIKey(ctx, apiKeyString) | ||
| authenticatedAPIKey2, err := authService.AnthenticateAPIKey(ctx, apiKeyString, false) |
There was a problem hiding this comment.
The function name AnthenticateAPIKey appears to have a typo and should be AuthenticateAPIKey. This should be corrected for clarity and consistency.
| authenticatedAPIKey2, err := authService.AnthenticateAPIKey(ctx, apiKeyString, false) | |
| authenticatedAPIKey2, err := authService.AuthenticateAPIKey(ctx, apiKeyString, false) |
internal/server/biz/auth_test.go
Outdated
|
|
||
| // Test successful API key authentication | ||
| authenticatedAPIKey, err := authService.AnthenticateAPIKey(ctx, apiKeyString) | ||
| authenticatedAPIKey, err := authService.AnthenticateAPIKey(ctx, apiKeyString, false) |
There was a problem hiding this comment.
The function name AnthenticateAPIKey appears to have a typo and should be AuthenticateAPIKey. This should be corrected for clarity and consistency.
| authenticatedAPIKey, err := authService.AnthenticateAPIKey(ctx, apiKeyString, false) | |
| authenticatedAPIKey, err := authService.AuthenticateAPIKey(ctx, apiKeyString, false) |
internal/server/middleware/auth.go
Outdated
| key = biz.NoAuthAPIKeyValue | ||
| } | ||
|
|
||
| apiKey, err := auth.AnthenticateAPIKey(c.Request.Context(), key, allowNoAuth) |
There was a problem hiding this comment.
The function name AnthenticateAPIKey appears to have a typo and should be AuthenticateAPIKey. This should be corrected for clarity and consistency.
| apiKey, err := auth.AnthenticateAPIKey(c.Request.Context(), key, allowNoAuth) | |
| apiKey, err := auth.AuthenticateAPIKey(c.Request.Context(), key, allowNoAuth) |
internal/server/middleware/auth.go
Outdated
| } | ||
|
|
||
| apiKey, err := auth.AnthenticateAPIKey(c.Request.Context(), key) | ||
| apiKey, err := auth.AnthenticateAPIKey(c.Request.Context(), key, false) |
There was a problem hiding this comment.
The function name AnthenticateAPIKey appears to have a typo and should be AuthenticateAPIKey. This should be corrected for clarity and consistency.
| apiKey, err := auth.AnthenticateAPIKey(c.Request.Context(), key, false) | |
| apiKey, err := auth.AuthenticateAPIKey(c.Request.Context(), key, false) |
internal/server/middleware/auth.go
Outdated
| } | ||
|
|
||
| apiKey, err := auth.AnthenticateAPIKey(c.Request.Context(), key) | ||
| apiKey, err := auth.AnthenticateAPIKey(c.Request.Context(), key, false) |
There was a problem hiding this comment.
The function name AnthenticateAPIKey appears to have a typo and should be AuthenticateAPIKey. This should be corrected for clarity and consistency.
| apiKey, err := auth.AnthenticateAPIKey(c.Request.Context(), key, false) | |
| apiKey, err := auth.AuthenticateAPIKey(c.Request.Context(), key, false) |
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'noauth' API key type, enabling API access without explicit authentication when configured. The changes involve updating API key type enums across frontend, backend, and GraphQL schemas, adding a configuration option server.api.auth.allow_no_auth, and modifying the API key service and authentication middleware to handle this new type. A lifecycle hook ensures the 'noauth' key is present if the feature is enabled. Overall, the implementation is robust and includes necessary checks to prevent unauthorized manipulation of the 'noauth' key. The addition of new test cases for the authentication middleware is also a positive step.
| func TestWithAPIKeyConfig_AllowsMissingAuthorizationWhenNoAuthAllowed(t *testing.T) { | ||
| gin.SetMode(gin.TestMode) | ||
|
|
||
| router := gin.New() | ||
| router.Use(func(c *gin.Context) { | ||
| key, err := ExtractAPIKeyFromRequest(c.Request, &APIKeyConfig{ | ||
| Headers: []string{"Authorization"}, | ||
| RequireBearer: true, | ||
| }) | ||
| if errors.Is(err, ErrAPIKeyRequired) { | ||
| c.Status(http.StatusNoContent) | ||
| c.Abort() | ||
|
|
||
| return | ||
| } | ||
|
|
||
| if err != nil || key != "" { | ||
| c.Status(http.StatusTeapot) | ||
| c.Abort() | ||
|
|
||
| return | ||
| } | ||
| }) | ||
|
|
||
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | ||
|
|
||
| recorder := httptest.NewRecorder() | ||
| router.ServeHTTP(recorder, req) | ||
|
|
||
| if recorder.Code != http.StatusNoContent { | ||
| t.Fatalf("expected status %d, got %d", http.StatusNoContent, recorder.Code) | ||
| } | ||
| } |
There was a problem hiding this comment.
The test TestWithAPIKeyConfig_AllowsMissingAuthorizationWhenNoAuthAllowed directly tests ExtractAPIKeyFromRequest within a custom middleware, rather than testing the WithAPIKeyConfig middleware's behavior for this scenario. The WithAPIKeyConfig middleware has specific logic (if errors.Is(err, ErrAPIKeyRequired) && allowNoAuth) that should be tested directly to ensure it correctly sets key = biz.NoAuthAPIKeyValue and proceeds with authentication when allowNoAuth is true and no API key is provided. The current test bypasses this specific middleware logic.
func TestWithAPIKeyConfig_AllowsMissingAuthorizationWhenNoAuthAllowed(t *testing.T) {
gin.SetMode(gin.TestMode)
authService := &biz.AuthService{} // Mock or setup a real AuthService if needed
router := gin.New()
router.Use(WithAPIKeyConfig(authService, true, &APIKeyConfig{
Headers: []string{"Authorization"},
RequireBearer: true,
}))
router.GET("/test", func(c *gin.Context) {
// If the middleware correctly processed the missing key as noauth, this handler should be reached.
c.Status(http.StatusOK)
})
req := httptest.NewRequest(http.MethodGet, "/test", nil)
recorder := httptest.NewRecorder()
router.ServeHTTP(recorder, req)
if recorder.Code != http.StatusOK {
t.Fatalf("expected status %d, got %d", http.StatusOK, recorder.Code)
}
}1 participant
No description provided.