Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces an audit callback mechanism to the file cleaner, adds a new CategorySystem audit category, and refactors audit logging across API endpoints to use a unified logAudit method instead of specialized audit logging helpers. Changes
Sequence DiagramsequenceDiagram
participant Ticker as File Cleaner<br/>(Ticker Loop)
participant Purge as purgeExpiredFiles()
participant AuditLog as appendFn Callback
participant AuditSvc as Audit Service
Ticker->>Purge: Periodically trigger purge
Purge->>Purge: Scan expired files
alt Files Found
Purge->>Purge: Delete files, collect purgedDates
Purge->>AuditLog: Call appendFn with details<br/>(purged_from, purged_to,<br/>files_removed, retention_days)
AuditLog->>AuditSvc: audit.NewEntry() + Log()
AuditSvc-->>AuditLog: Audit entry recorded
else No Files
Purge-->>Ticker: Return (no audit call)
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/service/frontend/api/v1/auth.go`:
- Line 128: The password-change audit call a.logAudit(ctx, audit.CategoryUser,
"password_change", nil) may omit the user's IP like the login case; update the
password-change path to include IP information by extracting the client IP from
the request context (use the same helper used for login, e.g. getIPFromContext
or requestIPFromCtx) and pass it into logAudit (or include it in the audit
payload) so the logAudit function receives the IP—ensure you update the
a.logAudit invocation in auth.go and, if necessary, the logAudit
signature/handling to accept or read the IP field.
In `@internal/service/frontend/api/v1/users.go`:
- Around line 199-206: The audit details omit the is_disabled change; update the
changes map before calling a.logAudit by checking the request/update input's
disable flag (e.g., input.IsDisabled or the field used to represent is_disabled)
and, when non-nil, add changes["is_disabled"] = *input.IsDisabled (or the
appropriate dereference/type conversion) so the a.logAudit(..., changes) call
includes the disabled/enabled state alongside username and role.
🧹 Nitpick comments (5)
internal/service/frontend/api/v1/sync.go (1)
297-297: Inconsistent details map type:map[string]stringvsmap[string]any.All other
logAuditcall sites in this file (and across the PR) passmap[string]anyfor the details argument. This call passesmap[string]string. While it compiles (the parameter is likely typed asany), using a consistent type reduces confusion and avoids issues iflogAuditever adds type-specific handling.Suggested fix
- a.logAudit(ctx, audit.CategoryGitSync, "sync_discard", map[string]string{"dag_id": req.Name}) + a.logAudit(ctx, audit.CategoryGitSync, "sync_discard", map[string]any{"dag_id": req.Name})internal/service/frontend/api/v1/webhooks.go (1)
121-124: Minor inconsistency:map[string]stringused here whilemap[string]anyis used elsewhere.Same observation as in
sync.go— these three call sites usemap[string]stringwhile Line 233 in this file and the vast majority of otherlogAuditcalls across the PR usemap[string]any. Consider aligning for consistency.Also applies to: 160-164, 194-197
internal/service/frontend/api/v1/apikeys.go (1)
92-96: Inconsistent detail map types across API key operations.
CreateAPIKeyandDeleteAPIKeyusemap[string]string, whileUpdateAPIKeyusesmap[string]any. Althoughjson.Marshalhandles both, using a consistent type improves readability. Consider usingmap[string]anyeverywhere for uniformity (as done inusers.goanddags.go).internal/persis/fileaudit/cleaner_test.go (1)
155-155: Note:newCleanerstarts a background goroutine.
newCleaner(dir, 7, nil)launchesgo c.run()which callspurgeExpiredFiles()immediately before the test callsc.stop(). Since this test is specifically about idempotent stop, it works — but there's a potential race where the goroutine's initialpurgeExpiredFiles()runs concurrently. In practice this is benign (empty temp dir), but if this test becomes flaky, consider usingnewTestCleaner(which doesn't start the goroutine) plus explicitc.stop()/ double-close testing onstopCh.internal/persis/fileaudit/cleaner.go (1)
122-135: Silently discardedjson.Marshalerror.Line 123 discards the error from
json.Marshal. While practically safe here (basic types only), the centralizedlogAuditinapi.gologs a warning on marshal failure. Consider logging for consistency, or add a brief comment explaining why the error is safe to ignore.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1654 +/- ##
==========================================
- Coverage 70.21% 70.20% -0.02%
==========================================
Files 344 344
Lines 38315 38332 +17
==========================================
+ Hits 26902 26910 +8
- Misses 9275 9281 +6
- Partials 2138 2141 +3
... and 10 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
1 participant
Summary by CodeRabbit
New Features
Refactor