Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors many controller tests to table-driven patterns with testify, adds a test-only AuthService method to clear rate-limits and cancel lockdown, updates go.mod to require testify, changes health JSON Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #731 +/- ##
==========================================
+ Coverage 16.81% 18.93% +2.11%
==========================================
Files 50 50
Lines 3829 3844 +15
==========================================
+ Hits 644 728 +84
+ Misses 3121 3044 -77
- Partials 64 72 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (12)
internal/controller/resources_controller_test.go (2)
66-87: Consider usingt.Cleanupfor robust teardown.If any assertion fails, the cleanup code at lines 80-87 may not execute, leaving test artifacts behind. Using
t.Cleanupensures cleanup runs regardless of test outcome.♻️ Proposed refactor
err := os.MkdirAll(resourcesControllerCfg.Path, 0777) assert.NoError(t, err) + t.Cleanup(func() { + os.Remove(testFilePath) + os.Remove(testFilePathParent) + os.Remove(resourcesControllerCfg.Path) + }) + testFilePath := resourcesControllerCfg.Path + "/testfile.txt" err = os.WriteFile(testFilePath, []byte("This is a test file."), 0777) assert.NoError(t, err) testFilePathParent := resourcesControllerCfg.Path + "/../somefile.txt" err = os.WriteFile(testFilePathParent, []byte("This file should not be accessible."), 0777) assert.NoError(t, err) for _, test := range tests { t.Run(test.description, func(t *testing.T) { // ... }) } - - err = os.Remove(testFilePath) - assert.NoError(t, err) - - err = os.Remove(testFilePathParent) - assert.NoError(t, err) - - err = os.Remove(resourcesControllerCfg.Path) - assert.NoError(t, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/resources_controller_test.go` around lines 66 - 87, Move the teardown file removals into t.Cleanup so they always run even if assertions fail: inside the t.Run closure (where the router, group, resourcesController and recorder are created) register cleanup functions with t.Cleanup that remove testFilePath, testFilePathParent and resourcesControllerCfg.Path (or use os.RemoveAll for robust removal) and assert/remove errors inside those cleanup functions; this ensures the removals execute for each subtest regardless of test failures.
67-70: Movegin.SetModeoutside the test loop.
gin.SetMode(gin.TestMode)at line 70 is called inside each subtest iteration. This should be called once at the beginning of the test function for efficiency.♻️ Proposed fix
+ gin.SetMode(gin.TestMode) + for _, test := range tests { t.Run(test.description, func(t *testing.T) { router := gin.Default() group := router.Group("/") - gin.SetMode(gin.TestMode)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/resources_controller_test.go` around lines 67 - 70, The call to gin.SetMode(gin.TestMode) is inside each subtest; move it out so it's executed once at the start of the test function (before creating per-test routers and before the t.Run loop). Locate the usage of gin.SetMode in the test that builds router := gin.Default() and group := router.Group("/") and place gin.SetMode(gin.TestMode) above the loop that iterates over test cases so each subtest still creates its own router but the mode is set only once.internal/controller/proxy_controller_test.go (1)
366-370: Consider usingt.Cleanupfor database teardown.Consistent with the pattern suggested in other test files, using
t.Cleanupensures cleanup runs regardless of test outcome.♻️ Proposed refactor
db, err := app.SetupDatabase("/tmp/tinyauth_test.db") assert.NoError(t, err) + t.Cleanup(func() { + db.Close() + os.Remove("/tmp/tinyauth_test.db") + }) + queries := repository.New(db) // ... for _, test := range tests { // ... } - - err = db.Close() - assert.NoError(t, err) - - err = os.Remove("/tmp/tinyauth_test.db") - assert.NoError(t, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/proxy_controller_test.go` around lines 366 - 370, Replace the explicit teardown at the end of the test (the direct calls to db.Close() and os.Remove("/tmp/tinyauth_test.db") with assertions) by registering cleanup functions immediately after the database is opened/created: call t.Cleanup(func(){ _ = db.Close() }) and t.Cleanup(func(){ _ = os.Remove("/tmp/tinyauth_test.db") }) (or assert within the cleanup if you prefer), and remove the trailing err = db.Close() / err = os.Remove(...) blocks so cleanup always runs regardless of test failure.internal/controller/well_known_controller_test.go (1)
122-126: Consider usingt.Cleanupand cleaning up generated key files.The test configures key file paths at
/tmp/tinyauth_testing_key.pemand/tmp/tinyauth_testing_key.pub. IfOIDCService.Init()generates these files, they should be cleaned up after the test. Also, usingt.Cleanupensures cleanup runs even on test failure.♻️ Proposed refactor
db, err := app.SetupDatabase("/tmp/tinyauth_test.db") assert.NoError(t, err) + t.Cleanup(func() { + db.Close() + os.Remove("/tmp/tinyauth_test.db") + os.Remove(oidcServiceCfg.PrivateKeyPath) + os.Remove(oidcServiceCfg.PublicKeyPath) + }) + queries := repository.New(db) oidcService := service.NewOIDCService(oidcServiceCfg, queries) // ... - err = db.Close() - assert.NoError(t, err) - - err = os.Remove("/tmp/tinyauth_test.db") - assert.NoError(t, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/well_known_controller_test.go` around lines 122 - 126, The test currently manually removes the DB file and misses cleanup for key files potentially created by OIDCService.Init(); modify the test to use t.Cleanup to remove generated artifacts so cleanup runs on failure: register t.Cleanup calls to close the DB (or call db.Close() inside a cleanup) and to remove "/tmp/tinyauth_test.db" as well as the key files "/tmp/tinyauth_testing_key.pem" and "/tmp/tinyauth_testing_key.pub"; ensure this is done in the scope where OIDCService.Init() and DB creation occur (refer to OIDCService.Init and the db variable) so all temporary files are always removed.internal/controller/user_controller_test.go (2)
306-335: Fragile string matching for middleware selection.Using
slices.Contains(setTotpMiddlewareOverrides, test.description)to conditionally apply middleware is fragile - if a test description changes, the middleware won't be applied and the test may pass or fail for wrong reasons.Consider restructuring to include middleware configuration directly in the test case struct.
♻️ Proposed refactor
type testCase struct { description string middlewares []gin.HandlerFunc + setTotpContext bool // flag to inject TOTP context run func(t *testing.T, router *gin.Engine, recorder *httptest.ResponseRecorder) } // Then in test cases: { description: "Should be able to login with totp", middlewares: []gin.HandlerFunc{}, + setTotpContext: true, run: func(t *testing.T, router *gin.Engine, recorder *httptest.ResponseRecorder) {And in the loop:
- setTotpMiddlewareOverrides := []string{ - "Should be able to login with totp", - "Totp should rate limit on multiple invalid attempts", - } - for _, test := range tests { beforeEach() t.Run(test.description, func(t *testing.T) { router := gin.Default() for _, middleware := range test.middlewares { router.Use(middleware) } - if slices.Contains(setTotpMiddlewareOverrides, test.description) { + if test.setTotpContext { router.Use(func(c *gin.Context) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/user_controller_test.go` around lines 306 - 335, The test currently uses fragile string matching (slices.Contains(setTotpMiddlewareOverrides, test.description)) to decide whether to inject the TOTP context middleware; instead add an explicit field to the test case (e.g. OverrideTotpMiddleware bool or ExtraMiddlewares []gin.HandlerFunc) and populate it in each test entry, then in the loop use that field (or append ExtraMiddlewares to test.middlewares) to call router.Use(...) with the TOTP-setting middleware that sets config.UserContext on gin.Context; update references to test.middlewares and remove setTotpMiddlewareOverrides and the slices.Contains check so middleware selection is driven by test data, not the test.description string.
349-353: Consider usingt.Cleanupfor database teardown.If assertions fail before lines 349-353, the database file won't be cleaned up. Using
t.Cleanupensures cleanup regardless of test outcome.♻️ Proposed refactor
db, err := app.SetupDatabase("/tmp/tinyauth_test.db") assert.NoError(t, err) + t.Cleanup(func() { + db.Close() + os.Remove("/tmp/tinyauth_test.db") + }) + queries := repository.New(db) // ... rest of setup ... for _, test := range tests { // ... } - - err = db.Close() - assert.NoError(t, err) - - err = os.Remove("/tmp/tinyauth_test.db") - assert.NoError(t, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/user_controller_test.go` around lines 349 - 353, Replace the manual teardown at the end of the test with a t.Cleanup that closes the DB and removes the file so cleanup always runs even on failures; after creating/opening the DB (and after determining the file path, e.g. "/tmp/tinyauth_test.db"), call t.Cleanup(func() { _ = db.Close(); _ = os.Remove("/tmp/tinyauth_test.db") }) or call require.NoError(t, db.Close()) / require.NoError(t, os.Remove(path)) inside the cleanup to surface errors—this removes the explicit db.Close() and os.Remove(...) at the end of the test.internal/controller/health_controller_test.go (3)
53-55: Setgin.SetModebefore creating the router.
gin.SetMode(gin.TestMode)should be called beforegin.Default()to suppress debug output during router initialization. Currently it's called after, so the first iteration may emit debug logs.Proposed fix
t.Run(test.description, func(t *testing.T) { + gin.SetMode(gin.TestMode) router := gin.Default() group := router.Group("/api") - gin.SetMode(gin.TestMode)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/health_controller_test.go` around lines 53 - 55, Move the gin.SetMode call to run before the router is created: call gin.SetMode(gin.TestMode) prior to invoking gin.Default() (which returns the router variable) so that debug output is suppressed during initialization; update the test in health_controller_test.go to set the mode before constructing router/group.
35-48: HEAD response body assertion may not reflect real HTTP behavior.Per HTTP spec, HEAD responses should not include a body. While
httptest.ResponseRecordercaptures what Gin writes internally, in production the body would be discarded. Consider either removing the body assertion for HEAD or documenting that this tests internal behavior rather than wire-level conformance.Option: Skip body assertion for HEAD
{ description: "Ensure health endpoint returns 200 OK for HEAD request", path: "/api/healthz", method: "HEAD", - expected: func() string { - expectedHealthResponse := map[string]any{ - "status": 200, - "message": "Healthy", - } - bytes, err := json.Marshal(expectedHealthResponse) - assert.NoError(t, err) - return string(bytes) - }(), + expected: "", // HEAD responses have no body per HTTP spec },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/health_controller_test.go` around lines 35 - 48, The test is asserting a response body for an HTTP HEAD request (the table entry with description "Ensure health endpoint returns 200 OK for HEAD request" and method "HEAD"), but per HTTP semantics HEAD responses have no body; update the test so it does not assert the body for HEAD requests: either remove the expected body string for that table case (make expected ""), or change the test runner to only compare the response body when testCase.method != "HEAD", while still asserting the status code (e.g., keep the status check in the loop that exercises the health handler).
25-33: Consider extracting the expected response to avoid duplication.The same expected response map is defined twice (for GET and HEAD). Consider defining it once at the test level or using a constant.
Proposed refactor
func TestHealthController(t *testing.T) { + expectedBody := func() string { + resp := map[string]any{"status": 200, "message": "Healthy"} + bytes, _ := json.Marshal(resp) + return string(bytes) + }() + tests := []struct { description string path string method string expected string }{ { description: "Ensure health endpoint returns 200 OK", path: "/api/healthz", method: "GET", - expected: func() string { - expectedHealthResponse := map[string]any{ - "status": 200, - "message": "Healthy", - } - bytes, err := json.Marshal(expectedHealthResponse) - assert.NoError(t, err) - return string(bytes) - }(), + expected: expectedBody, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/health_controller_test.go` around lines 25 - 33, The test currently duplicates the same expectedHealthResponse map inside anonymous funcs for both GET and HEAD assertions; extract that map (or the final marshaled string) into a single variable or helper at the test scope (e.g., expectedHealthResponse or expectedHealthJSON) and reuse it in both test cases instead of redefining the map and calling json.Marshal twice, ensuring you still assert.NoError on the marshal once when creating the shared value.internal/controller/oidc_controller_test.go (3)
57-64: Fragile test interdependency via string matching.Using
getTestByDescriptionto find and invoke other tests by their description string is fragile—if the description changes, dependent tests silently fail to find the prerequisite test. Consider extracting shared setup logic into reusable helper functions instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/oidc_controller_test.go` around lines 57 - 64, The test uses getTestByDescription to locate dependent tests by their description (iterating over tests and matching test.description) which is fragile; instead extract the shared setup logic into a reusable helper function (e.g., create a setup helper that returns the common router/recorder/state or a reusable run function) and update tests to call that helper directly rather than calling getTestByDescription/test.run by description; remove the string-matching lookup and reference the new helper from the test cases that previously depended on another test.
38-47: Test coverage gap:simpleCtxdoesn't set group fields.The
simpleCtxmiddleware creates aUserContextwithoutOAuthGroupsorLdapGroupsfields. Based oninternal/service/oidc_service.go, the "groups" scope is one of theSupportedScopes, and the service populates theGroupsclaim in ID tokens from these fields. Consider adding a test case that exercises the "groups" scope with populated group fields to ensure this claim is correctly included in tokens.Example middleware with groups for additional test coverage
ctxWithGroups := func(c *gin.Context) { c.Set("context", &config.UserContext{ Username: "test", Name: "Test User", Email: "test@example.com", IsLoggedIn: true, Provider: "local", OAuthGroups: "admin,users", }) c.Next() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/oidc_controller_test.go` around lines 38 - 47, simpleCtx currently sets a UserContext without group fields so tests don't exercise the "groups" scope; add a new middleware (e.g., ctxWithGroups) that sets OAuthGroups and/or LdapGroups on the config.UserContext (e.g., OAuthGroups: "admin,users") and add a test case that requests the "groups" scope through the OIDC flow to assert that the generated ID token contains the Groups claim populated (verify behavior in the code paths used by internal/service/oidc_service.go and its SupportedScopes handling).
462-472: Cleanup code may be skipped on test failure or panic.If any test panics or
t.FailNow()is called, the cleanup code after the test loop won't execute, leaving temporary files and database behind. Uset.Cleanup()to ensure resources are always cleaned up.Proposed fix using t.Cleanup
err = oidcService.Init() assert.NoError(t, err) + t.Cleanup(func() { + db.Close() + os.Remove("/tmp/tinyauth_test.db") + os.Remove(oidcServiceCfg.PrivateKeyPath) + os.Remove(oidcServiceCfg.PublicKeyPath) + }) + for _, test := range tests { t.Run(test.description, func(t *testing.T) { // ... }) } - - err = db.Close() - assert.NoError(t, err) - - err = os.Remove("/tmp/tinyauth_test.db") - assert.NoError(t, err) - - err = os.Remove(oidcServiceCfg.PrivateKeyPath) - assert.NoError(t, err) - - err = os.Remove(oidcServiceCfg.PublicKeyPath) - assert.NoError(t, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/oidc_controller_test.go` around lines 462 - 472, The manual cleanup after the test loop can be skipped on panic or FailNow; replace the explicit post-test removals with t.Cleanup registrations so they always run: when you open the DB (variable db) call t.Cleanup(func(){ _ = db.Close() }) and when you create the temp DB file "/tmp/tinyauth_test.db" and the keys (oidcServiceCfg.PrivateKeyPath and oidcServiceCfg.PublicKeyPath) register t.Cleanup(func(){ _ = os.Remove(path) }) for each; remove the trailing os.Remove and db.Close() calls and ensure the t.Cleanup registrations happen immediately after each resource is created/initialized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/context_controller_test.go`:
- Around line 128-129: The first assert.Equal call in context_controller_test.go
has the arguments reversed; change assert.Equal(t, recorder.Result().StatusCode,
http.StatusOK) to assert.Equal(t, http.StatusOK, recorder.Result().StatusCode)
so it follows testify's assert.Equal(t, expected, actual) convention (the second
assertion comparing test.expected and recorder.Body.String() is already
correct).
In `@internal/controller/oidc_controller_test.go`:
- Line 332: Fix the typo in the test case description string "Ensure token
endpoint deletes code afer use" by changing "afer" to "after" so it reads
"Ensure token endpoint deletes code after use"; locate the string used as the
test description in the oidc controller tests (the description field in the
failing test case) and update it accordingly.
In `@internal/controller/proxy_controller_test.go`:
- Line 285: Fix the typos in the test description strings in
proxy_controller_test.go: change the description "Ensure user allow ACL allows
correct user (should allow testuer)" to "Ensure user allow ACL allows correct
user (should allow testuser)" and change the other occurrence of "testuer" in
the failing-block description to "totpuser" so the messages correctly reflect
the intended users.
In `@internal/controller/user_controller_test.go`:
- Line 77: The assert.Equal calls in user_controller_test.go use the arguments
in the wrong order; change them to assert.Equal(t, expected, actual) so the
expected value is first and the actual value (cookie.MaxAge or
totpCookie.MaxAge) is second: replace assert.Equal(t, cookie.MaxAge, 10) with
assert.Equal(t, 10, cookie.MaxAge), assert.Equal(t, cookie.MaxAge, 3600) with
assert.Equal(t, 3600, cookie.MaxAge), and assert.Equal(t, totpCookie.MaxAge, 10)
with assert.Equal(t, 10, totpCookie.MaxAge).
In `@internal/controller/well_known_controller_test.go`:
- Line 69: The assert call in well_known_controller_test.go uses the wrong
argument order: change the testify call assert.Equal(t, res, expected) to
assert.Equal(t, expected, res) so the expected value (expected) is the second
parameter and the actual value (res) is the third; update the single occurrence
of assert.Equal that references res and expected accordingly.
In `@internal/service/auth_service.go`:
- Around line 799-805: The ClearRateLimitsTestingOnly method has mixed
indentation: the assignment to auth.loginAttempts uses spaces while the file
uses tabs; update the method in AuthService to use tabs consistently (replace
the space-indented line with a tab-indented line), ensuring the block around
ClearRateLimitsTestingOnly, including references to auth.loginMutex,
auth.loginAttempts and the method declaration, uses the same tab indentation
style as the rest of the file.
---
Nitpick comments:
In `@internal/controller/health_controller_test.go`:
- Around line 53-55: Move the gin.SetMode call to run before the router is
created: call gin.SetMode(gin.TestMode) prior to invoking gin.Default() (which
returns the router variable) so that debug output is suppressed during
initialization; update the test in health_controller_test.go to set the mode
before constructing router/group.
- Around line 35-48: The test is asserting a response body for an HTTP HEAD
request (the table entry with description "Ensure health endpoint returns 200 OK
for HEAD request" and method "HEAD"), but per HTTP semantics HEAD responses have
no body; update the test so it does not assert the body for HEAD requests:
either remove the expected body string for that table case (make expected ""),
or change the test runner to only compare the response body when testCase.method
!= "HEAD", while still asserting the status code (e.g., keep the status check in
the loop that exercises the health handler).
- Around line 25-33: The test currently duplicates the same
expectedHealthResponse map inside anonymous funcs for both GET and HEAD
assertions; extract that map (or the final marshaled string) into a single
variable or helper at the test scope (e.g., expectedHealthResponse or
expectedHealthJSON) and reuse it in both test cases instead of redefining the
map and calling json.Marshal twice, ensuring you still assert.NoError on the
marshal once when creating the shared value.
In `@internal/controller/oidc_controller_test.go`:
- Around line 57-64: The test uses getTestByDescription to locate dependent
tests by their description (iterating over tests and matching test.description)
which is fragile; instead extract the shared setup logic into a reusable helper
function (e.g., create a setup helper that returns the common
router/recorder/state or a reusable run function) and update tests to call that
helper directly rather than calling getTestByDescription/test.run by
description; remove the string-matching lookup and reference the new helper from
the test cases that previously depended on another test.
- Around line 38-47: simpleCtx currently sets a UserContext without group fields
so tests don't exercise the "groups" scope; add a new middleware (e.g.,
ctxWithGroups) that sets OAuthGroups and/or LdapGroups on the config.UserContext
(e.g., OAuthGroups: "admin,users") and add a test case that requests the
"groups" scope through the OIDC flow to assert that the generated ID token
contains the Groups claim populated (verify behavior in the code paths used by
internal/service/oidc_service.go and its SupportedScopes handling).
- Around line 462-472: The manual cleanup after the test loop can be skipped on
panic or FailNow; replace the explicit post-test removals with t.Cleanup
registrations so they always run: when you open the DB (variable db) call
t.Cleanup(func(){ _ = db.Close() }) and when you create the temp DB file
"/tmp/tinyauth_test.db" and the keys (oidcServiceCfg.PrivateKeyPath and
oidcServiceCfg.PublicKeyPath) register t.Cleanup(func(){ _ = os.Remove(path) })
for each; remove the trailing os.Remove and db.Close() calls and ensure the
t.Cleanup registrations happen immediately after each resource is
created/initialized.
In `@internal/controller/proxy_controller_test.go`:
- Around line 366-370: Replace the explicit teardown at the end of the test (the
direct calls to db.Close() and os.Remove("/tmp/tinyauth_test.db") with
assertions) by registering cleanup functions immediately after the database is
opened/created: call t.Cleanup(func(){ _ = db.Close() }) and t.Cleanup(func(){ _
= os.Remove("/tmp/tinyauth_test.db") }) (or assert within the cleanup if you
prefer), and remove the trailing err = db.Close() / err = os.Remove(...) blocks
so cleanup always runs regardless of test failure.
In `@internal/controller/resources_controller_test.go`:
- Around line 66-87: Move the teardown file removals into t.Cleanup so they
always run even if assertions fail: inside the t.Run closure (where the router,
group, resourcesController and recorder are created) register cleanup functions
with t.Cleanup that remove testFilePath, testFilePathParent and
resourcesControllerCfg.Path (or use os.RemoveAll for robust removal) and
assert/remove errors inside those cleanup functions; this ensures the removals
execute for each subtest regardless of test failures.
- Around line 67-70: The call to gin.SetMode(gin.TestMode) is inside each
subtest; move it out so it's executed once at the start of the test function
(before creating per-test routers and before the t.Run loop). Locate the usage
of gin.SetMode in the test that builds router := gin.Default() and group :=
router.Group("/") and place gin.SetMode(gin.TestMode) above the loop that
iterates over test cases so each subtest still creates its own router but the
mode is set only once.
In `@internal/controller/user_controller_test.go`:
- Around line 306-335: The test currently uses fragile string matching
(slices.Contains(setTotpMiddlewareOverrides, test.description)) to decide
whether to inject the TOTP context middleware; instead add an explicit field to
the test case (e.g. OverrideTotpMiddleware bool or ExtraMiddlewares
[]gin.HandlerFunc) and populate it in each test entry, then in the loop use that
field (or append ExtraMiddlewares to test.middlewares) to call router.Use(...)
with the TOTP-setting middleware that sets config.UserContext on gin.Context;
update references to test.middlewares and remove setTotpMiddlewareOverrides and
the slices.Contains check so middleware selection is driven by test data, not
the test.description string.
- Around line 349-353: Replace the manual teardown at the end of the test with a
t.Cleanup that closes the DB and removes the file so cleanup always runs even on
failures; after creating/opening the DB (and after determining the file path,
e.g. "/tmp/tinyauth_test.db"), call t.Cleanup(func() { _ = db.Close(); _ =
os.Remove("/tmp/tinyauth_test.db") }) or call require.NoError(t, db.Close()) /
require.NoError(t, os.Remove(path)) inside the cleanup to surface errors—this
removes the explicit db.Close() and os.Remove(...) at the end of the test.
In `@internal/controller/well_known_controller_test.go`:
- Around line 122-126: The test currently manually removes the DB file and
misses cleanup for key files potentially created by OIDCService.Init(); modify
the test to use t.Cleanup to remove generated artifacts so cleanup runs on
failure: register t.Cleanup calls to close the DB (or call db.Close() inside a
cleanup) and to remove "/tmp/tinyauth_test.db" as well as the key files
"/tmp/tinyauth_testing_key.pem" and "/tmp/tinyauth_testing_key.pub"; ensure this
is done in the scope where OIDCService.Init() and DB creation occur (refer to
OIDCService.Init and the db variable) so all temporary files are always removed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 40e90bee-2590-4274-9b05-630b3510a5c6
📒 Files selected for processing (11)
go.modinternal/controller/context_controller_test.gointernal/controller/health_controller.gointernal/controller/health_controller_test.gointernal/controller/oidc_controller.gointernal/controller/oidc_controller_test.gointernal/controller/proxy_controller_test.gointernal/controller/resources_controller_test.gointernal/controller/user_controller_test.gointernal/controller/well_known_controller_test.gointernal/service/auth_service.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
internal/controller/oidc_controller_test.go (1)
57-64: Don’t compose these flows by subtest description strings.Looking up another case by its human-readable description makes the suite rename-sensitive and hides the real setup behind another test body. Extract small
t.Helper()utilities that return the auth code or token response directly instead of dispatching throughgetTestByDescription.Also applies to: 251-255, 290-293, 337-340, 401-404
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/oidc_controller_test.go` around lines 57 - 64, The test suite currently looks up other cases by their human-readable description using getTestByDescription and iterating over tests, which is fragile; instead extract focused helper functions (marked with t.Helper()) that perform the specific shared setups and return the needed auth code or token response (e.g., createAuthCode(t, router, recorder) and fetchTokenResponse(t, router, recorder)) and replace calls to getTestByDescription(...).run with direct calls to these helpers; update any places referencing getTestByDescription, tests, or test.run (including the other occurrences at the noted ranges) to use the new helpers so setup is explicit and independent of test description strings.internal/controller/proxy_controller_test.go (1)
321-370: Use a temp database path per test run.Hard-coding
/tmp/tinyauth_test.dbmakes this suite Unix-specific, shares state withinternal/controller/user_controller_test.go, and only cleans up at the very end. If a run exits early, later tests can inherit stale data.t.TempDir()plust.Cleanup()will isolate this reliably.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/proxy_controller_test.go` around lines 321 - 370, The test currently uses a fixed DB path "/tmp/tinyauth_test.db" when calling app.SetupDatabase in the proxy controller tests; change it to create an isolated temp DB per test run using t.TempDir() (e.g. dbPath := filepath.Join(t.TempDir(), "tinyauth_test.db")) and ensure cleanup with t.Cleanup if needed; update the call to app.SetupDatabase(dbPath) and remove the final os.Remove and shared teardown so each test invocation of NewBootstrapApp/SetupDatabase runs against its own file and is cleaned up automatically.internal/controller/user_controller_test.go (2)
306-335: Make the TOTP setup explicit on the test case instead of keying offdescription.Using the description string as control flow is brittle: a rename silently changes middleware wiring. Since
test.middlewaresalready exists androuter.Use(...)runs before routes are registered, these TOTP cases can declare their middleware directly (or use a dedicated flag/setup hook).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/user_controller_test.go` around lines 306 - 335, The test relies on matching test.description against setTotpMiddlewareOverrides to inject a TOTP context, which is brittle; instead add an explicit signal on each test case (e.g., a TotpSetup bool or a TotpMiddleware in the tests slice) and use that to install the TOTP context middleware before routes are registered. Update the tests table (the tests variable) to set TotpSetup=true for the TOTP scenarios and replace the slices.Contains(setTotpMiddlewareOverrides, test.description) branch with a check of test.TotpSetup (or simply include the middleware in test.middlewares), then call router.Use with the same anonymous middleware that sets the config.UserContext when that flag or middleware is present.
278-304: Prefer a fresh fixture per subtest here.This DB/auth stack is created once for the whole suite, and
beforeEach()only partially resets it via the new testing-onlyAuthServicehook. Building the fixture inside eacht.Runkeeps login/session state from bleeding across cases and avoids needing production code just to support test cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/user_controller_test.go` around lines 278 - 304, The test currently creates the entire DB/auth stack once (calling bootstrap.NewBootstrapApp, app.SetupDatabase, repository.New, service.NewDockerService.Init, service.NewLdapService.Init, service.NewOAuthBrokerService.Init and service.NewAuthService.Init) and then calls authService.ClearRateLimitsTestingOnly() in beforeEach; change the tests to instantiate that full fixture inside each subtest (move the bootstrap.NewBootstrapApp..SetupDatabase..New..Init..NewAuthService..Init sequence into each t.Run) so each subtest gets a fresh DB and AuthService and remove reliance on ClearRateLimitsTestingOnly as the per-test reset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/oidc_controller_test.go`:
- Around line 7-9: Tests in oidc_controller_test.go use hard-coded /tmp
filenames which makes the suite non-reentrant; change those to per-test temp
files by creating a temp directory via t.TempDir() and placing the DB and
keypair files inside it (e.g., dbPath := filepath.Join(t.TempDir(),
"oidc_test.db") and keyPath := filepath.Join(t.TempDir(), "oidc_test.key")) so
each test gets isolated state and the harness cleans up automatically; update
every place that currently references the shared /tmp paths (the setups around
the DB/key creation and teardown — occurrences around the earlier setup block
and the sections flagged at lines ~20-34 and ~434-472) to use these local
dbPath/keyPath variables and remove manual global cleanup logic.
- Around line 168-177: The tests build token requests by JSON-marshaling
controller.TokenRequest (e.g., variables reqBody, reqBodyBytes and the request
created with httptest.NewRequest), but the struct has no json tags so the server
never sees real OAuth parameter names; replace these JSON bodies with
form-encoded bodies using url.Values (set keys like "grant_type", "code",
"redirect_uri", "client_id", "client_secret", "refresh_token" as appropriate),
pass strings.NewReader(values.Encode()) into httptest.NewRequest, and set the
request header to "Content-Type: application/x-www-form-urlencoded" so the token
endpoint receives the real OAuth/OIDC parameter names.
In `@internal/controller/well_known_controller_test.go`:
- Around line 29-31: The test currently uses fixed /tmp file paths for
PrivateKeyPath and PublicKeyPath which creates shared state; change the test to
create a temp directory via t.TempDir(), construct unique file paths inside it
for the private/public key files, update any code that sets PrivateKeyPath and
PublicKeyPath to use those temp paths, and remove the manual os.Remove/cleanup
calls (the t.TempDir() lifecycle handles cleanup) — look for the test that sets
PrivateKeyPath/PublicKeyPath and the subsequent manual deletion calls and
replace them accordingly.
- Around line 50-52: Replace non-fatal setup assertions with fatal ones or early
returns so dependent code can't run on invalid state: change assert.NoError(t,
err) after json.Unmarshal to require.NoError(t, err) (or check err and return)
before calling repository.New(db), change assert.Len(t, keys, 1) to
require.Len(t, keys, 1) (or guard the access to keys[0]) to avoid panics, and
register DB cleanup with t.Cleanup(func(){ _ = db.Close() }) instead of relying
on lines that may not run; update the tests around json.Unmarshal,
repository.New, keys usage and DB close to use these require/cleanup changes.
In `@internal/service/auth_service.go`:
- Around line 801-805: The ClearRateLimitsTestingOnly helper currently resets
auth.loginAttempts but does not clear the global lockdown state checked by
IsAccountLocked; update ClearRateLimitsTestingOnly to also clear the lockdown
fields (e.g., set auth.lockdown = false and reset any related timers/fields such
as auth.lockdownExpires to a zero value) while holding auth.loginMutex (or the
same lock used for lockdown mutations) so the service is fully unlocked for
tests and matches the behavior expected by IsAccountLocked and lockdownMode.
---
Nitpick comments:
In `@internal/controller/oidc_controller_test.go`:
- Around line 57-64: The test suite currently looks up other cases by their
human-readable description using getTestByDescription and iterating over tests,
which is fragile; instead extract focused helper functions (marked with
t.Helper()) that perform the specific shared setups and return the needed auth
code or token response (e.g., createAuthCode(t, router, recorder) and
fetchTokenResponse(t, router, recorder)) and replace calls to
getTestByDescription(...).run with direct calls to these helpers; update any
places referencing getTestByDescription, tests, or test.run (including the other
occurrences at the noted ranges) to use the new helpers so setup is explicit and
independent of test description strings.
In `@internal/controller/proxy_controller_test.go`:
- Around line 321-370: The test currently uses a fixed DB path
"/tmp/tinyauth_test.db" when calling app.SetupDatabase in the proxy controller
tests; change it to create an isolated temp DB per test run using t.TempDir()
(e.g. dbPath := filepath.Join(t.TempDir(), "tinyauth_test.db")) and ensure
cleanup with t.Cleanup if needed; update the call to app.SetupDatabase(dbPath)
and remove the final os.Remove and shared teardown so each test invocation of
NewBootstrapApp/SetupDatabase runs against its own file and is cleaned up
automatically.
In `@internal/controller/user_controller_test.go`:
- Around line 306-335: The test relies on matching test.description against
setTotpMiddlewareOverrides to inject a TOTP context, which is brittle; instead
add an explicit signal on each test case (e.g., a TotpSetup bool or a
TotpMiddleware in the tests slice) and use that to install the TOTP context
middleware before routes are registered. Update the tests table (the tests
variable) to set TotpSetup=true for the TOTP scenarios and replace the
slices.Contains(setTotpMiddlewareOverrides, test.description) branch with a
check of test.TotpSetup (or simply include the middleware in test.middlewares),
then call router.Use with the same anonymous middleware that sets the
config.UserContext when that flag or middleware is present.
- Around line 278-304: The test currently creates the entire DB/auth stack once
(calling bootstrap.NewBootstrapApp, app.SetupDatabase, repository.New,
service.NewDockerService.Init, service.NewLdapService.Init,
service.NewOAuthBrokerService.Init and service.NewAuthService.Init) and then
calls authService.ClearRateLimitsTestingOnly() in beforeEach; change the tests
to instantiate that full fixture inside each subtest (move the
bootstrap.NewBootstrapApp..SetupDatabase..New..Init..NewAuthService..Init
sequence into each t.Run) so each subtest gets a fresh DB and AuthService and
remove reliance on ClearRateLimitsTestingOnly as the per-test reset.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 17aa68cf-9a50-450b-868c-74a9e06fd935
📒 Files selected for processing (6)
internal/controller/context_controller_test.gointernal/controller/oidc_controller_test.gointernal/controller/proxy_controller_test.gointernal/controller/user_controller_test.gointernal/controller/well_known_controller_test.gointernal/service/auth_service.go
✅ Files skipped from review due to trivial changes (1)
- internal/controller/context_controller_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/controller/user_controller_test.go (1)
309-312: Make the TOTP setup explicit in the test case instead of keying offdescription.The
setTotpMiddlewareOverrides+slices.Contains(test.description)branch makes these cases depend on an exact string match even thoughtestCasealready has amiddlewaresfield. A rename now silently changes the setup. Putting the TOTP context middleware directly on those cases, or adding an explicit boolean, will make the suite much less brittle.Also applies to: 323-338
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/user_controller_test.go` around lines 309 - 312, The test suite currently uses setTotpMiddlewareOverrides and slices.Contains(test.description) to decide TOTP setup which is brittle; update the relevant tests to set TOTP explicitly by adding the TOTP middleware to the testCase.middlewares (or introduce a boolean like testCase.withTOTP) for the cases that need it instead of matching test.description, and remove the slices.Contains(test.description) branch that checks setTotpMiddlewareOverrides; ensure any helper that applies middlewares (the code reading testCase.middlewares) recognizes the TOTP middleware so the two cases formerly listed in setTotpMiddlewareOverrides are configured directly.internal/controller/oidc_controller_test.go (1)
61-68: Avoid dispatching other cases bydescription.
getTestByDescriptionturns the table description into an API between tests. A simple rename makes the lookup returnnil, and the caller immediately invokes it. Extracting small helpers for “authorize” / “exchange token” would remove that hidden coupling and make each case’s setup explicit.Also applies to: 255-259, 294-297, 341-344
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/oidc_controller_test.go` around lines 61 - 68, getTestByDescription currently turns the test table's description into an implicit API and enables callers to look up-and-invoke arbitrary cases; remove this helper and stop dispatching test behavior by description. Instead, replace uses of getTestByDescription with explicit per-scenario helpers (e.g. authorizeTestHelper, exchangeTokenTestHelper) that build the exact setup and call the target test.run directly, or reference the specific test.run from the tests slice in-place so each case's setup is explicit; if you must keep a lookup, rename getTestByDescription to findTestByDescription and return only the test struct (not a callable) so callers cannot accidentally invoke the wrong case. Ensure references to the tests slice and the run field are updated accordingly (search for getTestByDescription and uses in the file and update callers to use the new helpers or direct test.run invocation).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/service/auth_service.go`:
- Around line 801-805: ClearRateLimitsTestingOnly currently nils auth.lockdown
while the timer/goroutine started by lockdownMode may still be running, causing
stale timers to later clear new lockdowns; fix by holding auth.loginMutex,
stopping/canceling the existing lockdown timer before setting auth.lockdown =
nil (e.g., call a cancel/Stop/Close method on the Lockdown instance or add a
Lockdown.Cancel/Stop method that signals the goroutine to exit), and only then
clear the pointer and reset auth.loginAttempts so the timer cannot outlive the
cleared state.
---
Nitpick comments:
In `@internal/controller/oidc_controller_test.go`:
- Around line 61-68: getTestByDescription currently turns the test table's
description into an implicit API and enables callers to look up-and-invoke
arbitrary cases; remove this helper and stop dispatching test behavior by
description. Instead, replace uses of getTestByDescription with explicit
per-scenario helpers (e.g. authorizeTestHelper, exchangeTokenTestHelper) that
build the exact setup and call the target test.run directly, or reference the
specific test.run from the tests slice in-place so each case's setup is
explicit; if you must keep a lookup, rename getTestByDescription to
findTestByDescription and return only the test struct (not a callable) so
callers cannot accidentally invoke the wrong case. Ensure references to the
tests slice and the run field are updated accordingly (search for
getTestByDescription and uses in the file and update callers to use the new
helpers or direct test.run invocation).
In `@internal/controller/user_controller_test.go`:
- Around line 309-312: The test suite currently uses setTotpMiddlewareOverrides
and slices.Contains(test.description) to decide TOTP setup which is brittle;
update the relevant tests to set TOTP explicitly by adding the TOTP middleware
to the testCase.middlewares (or introduce a boolean like testCase.withTOTP) for
the cases that need it instead of matching test.description, and remove the
slices.Contains(test.description) branch that checks setTotpMiddlewareOverrides;
ensure any helper that applies middlewares (the code reading
testCase.middlewares) recognizes the TOTP middleware so the two cases formerly
listed in setTotpMiddlewareOverrides are configured directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 64f06737-0b71-4921-a609-667e478bc6ac
📒 Files selected for processing (6)
internal/controller/oidc_controller_test.gointernal/controller/proxy_controller_test.gointernal/controller/resources_controller_test.gointernal/controller/user_controller_test.gointernal/controller/well_known_controller_test.gointernal/service/auth_service.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/resources_controller_test.go
Rycochet
reviewed
Mar 29, 2026
Rycochet
previously approved these changes
Mar 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rework all tests with a better structure for more readable and thorough testing.
Summary by CodeRabbit
Bug Fixes
statusin JSON.WWW-Authenticateheader now sends a proper Basic challenge (includes realm).Tests
Chores