feature: add GDPR compliance #9865
Conversation
|
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthroughThis PR introduces comprehensive GDPR-compliant audit logging for Parse Server. It adds infrastructure to log user authentication, data access, data modifications, ACL changes, schema modifications, and push notifications. The implementation includes a Winston-based daily-rotating file adapter, configurable event filtering, TypeScript-based event types, and integration points across multiple routers to capture relevant operations. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Client
participant Parse as Parse Server<br/>(Router)
participant Audit as AuditLogController
participant Filter as AuditLogFilter
participant Adapter as WinstonFileAuditLogAdapter
participant Logger as Winston Logger
participant FS as File System
User->>Parse: Login / Data Access / Write
Parse->>Parse: Process Operation
Parse->>Audit: log[EventType](params)
Audit->>Audit: Build AuditEvent
Audit->>Filter: shouldLog(event)
Filter->>Filter: Check eventType, class,<br/>masterKey, roles, custom
Filter-->>Audit: true/false
alt shouldLog() returns true
Audit->>Adapter: log[EventType](event)
Adapter->>Adapter: maskSensitiveData(event)
Adapter->>Logger: logger.info('audit_event',<br/>auditEntry)
Logger->>FS: Write to daily rotation file<br/>(parse-server-audit-YYYY-MM-DD.log)
FS-->>Logger: ✓ Written
Logger-->>Adapter: ✓ Complete
Adapter-->>Audit: Promise resolved
Audit-->>Parse: (fire-and-forget)
end
Parse-->>User: Return Response
Note over Parse: Audit logging errors<br/>do not affect main flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.39.7)spec/ParseQuery.spec.jsThanks 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 |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
GDPR_COMPLIANCE_GUIDE.md(1 hunks)spec/AuditLogAdapter.spec.js(1 hunks)spec/AuditLogController.spec.js(1 hunks)spec/AuditLogSchemas.spec.js(1 hunks)spec/AuditLogging.e2e.spec.js(1 hunks)spec/Auth.spec.js(1 hunks)spec/ParseObject.spec.js(1 hunks)spec/ParseQuery.spec.js(1 hunks)src/Adapters/Logger/AuditLogAdapter.js(1 hunks)src/Adapters/Logger/AuditLogger.js(1 hunks)src/Controllers/AuditLogController.js(1 hunks)src/Controllers/index.js(5 hunks)src/Options/Definitions.js(2 hunks)src/Options/index.js(2 hunks)src/RestQuery.js(2 hunks)src/RestWrite.js(2 hunks)src/Routers/PushRouter.js(2 hunks)src/Routers/SchemasRouter.js(3 hunks)src/Routers/UsersRouter.js(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (14)
src/Controllers/index.js (3)
src/Adapters/Logger/AuditLogger.js (1)
options(81-86)src/Controllers/AuditLogController.js (1)
AuditLogController(8-320)src/Adapters/Logger/AuditLogAdapter.js (1)
AuditLogAdapter(9-293)
src/Adapters/Logger/AuditLogAdapter.js (3)
src/Adapters/Logger/LoggerAdapter.js (1)
LoggerAdapter(9-18)src/Adapters/Logger/AuditLogger.js (4)
options(81-86)configureAuditLogger(58-89)isAuditLogEnabled(139-141)logAuditEvent(105-133)src/Controllers/index.js (11)
options(62-71)options(85-85)options(104-112)options(125-125)options(133-133)options(153-153)options(163-163)options(164-164)options(184-184)options(196-196)options(234-234)
spec/ParseObject.spec.js (1)
spec/helper.js (6)
require(6-6)require(8-8)require(52-52)path(42-42)reconfigureServer(171-205)Parse(4-4)
spec/AuditLogController.spec.js (2)
src/Controllers/AuditLogController.js (1)
AuditLogController(8-320)src/Adapters/Logger/AuditLogAdapter.js (1)
AuditLogAdapter(9-293)
spec/AuditLogging.e2e.spec.js (3)
spec/AuditLogAdapter.spec.js (4)
fs(5-5)require(4-4)path(6-6)testLogFolder(9-9)spec/helper.js (1)
reconfigureServer(171-205)spec/ParseQuery.spec.js (8)
Parse(7-7)i(374-374)i(599-599)i(2398-2398)i(2658-2658)i(3067-3067)i(3109-3109)request(8-8)
spec/AuditLogSchemas.spec.js (1)
spec/helper.js (1)
reconfigureServer(171-205)
src/Controllers/AuditLogController.js (3)
spec/AuditLogController.spec.js (2)
AuditLogController(3-3)AuditLogAdapter(4-4)src/Controllers/index.js (11)
options(62-71)options(85-85)options(104-112)options(125-125)options(133-133)options(153-153)options(163-163)options(164-164)options(184-184)options(196-196)options(234-234)src/Adapters/Logger/AuditLogAdapter.js (1)
AuditLogAdapter(9-293)
spec/Auth.spec.js (1)
spec/helper.js (5)
require(6-6)require(8-8)require(52-52)reconfigureServer(171-205)Parse(4-4)
src/Routers/PushRouter.js (2)
src/PromiseRouter.js (1)
body(141-141)src/Controllers/index.js (1)
pushController(213-213)
src/Adapters/Logger/AuditLogger.js (1)
src/Controllers/index.js (11)
options(62-71)options(85-85)options(104-112)options(125-125)options(133-133)options(153-153)options(163-163)options(164-164)options(184-184)options(196-196)options(234-234)
src/Options/Definitions.js (1)
resources/buildConfigDefinitions.js (1)
parsers(12-12)
src/Routers/UsersRouter.js (1)
src/RestWrite.js (9)
user(330-330)user(901-901)user(1514-1514)user(1787-1787)userId(537-537)userId(547-547)RestWrite(988-995)RestWrite(1153-1159)sessionData(1010-1019)
spec/ParseQuery.spec.js (1)
spec/helper.js (6)
require(6-6)require(8-8)require(52-52)reconfigureServer(171-205)Parse(4-4)i(324-324)
spec/AuditLogAdapter.spec.js (2)
src/Adapters/Logger/AuditLogAdapter.js (1)
AuditLogAdapter(9-293)src/Adapters/Logger/AuditLogger.js (1)
logAuditEvent(105-133)
🪛 Biome (2.1.2)
src/Controllers/index.js
[error] 84-84: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 84-84: return types can only be used in TypeScript files
(parse)
src/Options/index.js
[error] 217-217: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
[error] 545-545: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
[error] 545-545: Expected a statement but instead found '}'.
Expected a statement here.
(parse)
[error] 548-548: Expected a type but instead found '?'.
Expected a type here.
(parse)
[error] 548-548: Expected a property, or a signature but instead found ';'.
Expected a property, or a signature here.
(parse)
[error] 553-553: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
[error] 558-558: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
🪛 GitHub Check: Lint
src/RestQuery.js
[warning] 981-981:
Unexpected console statement
src/RestWrite.js
[warning] 1859-1859:
Unexpected console statement
spec/AuditLogging.e2e.spec.js
[failure] 442-442:
'request' is not defined
spec/AuditLogSchemas.spec.js
[failure] 226-226:
'request' is not defined
[failure] 197-197:
'request' is not defined
[failure] 164-164:
'request' is not defined
[failure] 133-133:
'request' is not defined
[failure] 117-117:
'request' is not defined
[failure] 82-82:
'request' is not defined
[failure] 66-66:
'request' is not defined
[failure] 35-35:
'request' is not defined
spec/Auth.spec.js
[failure] 321-321:
Empty block statement
src/Adapters/Logger/AuditLogger.js
[warning] 77-77:
Unexpected console statement
[warning] 46-46:
Unexpected console statement
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Docker Build
- GitHub Check: Code Analysis (javascript)
🔇 Additional comments (7)
src/RestWrite.js (2)
157-159: LGTM! Audit logging correctly placed after cleanUserAuthData.The audit logging is invoked at the right point in the pipeline—after
cleanUserAuthDataensures sensitive auth data is cleaned from the response, and before the final response assembly. This prevents logging of raw authentication tokens.
1823-1830: AuditLogController masks sensitive fields in create logs
Confirmed thatlogDataCreatecalls_maskSensitiveData(data), replacing any sensitive values with'***masked***'. No further action needed.src/Routers/SchemasRouter.js (3)
91-119: LGTM! Schema create audit logging is correctly implemented.The try/catch wrapper ensures both success and failure scenarios are logged before propagating errors. Logging happens after the operation completes, ensuring the audit trail reflects actual outcomes.
122-163: LGTM! Schema modify audit logging is correctly implemented.The conversion to async/await with try/catch provides clean error handling, and audit events are emitted for both success and failure paths.
166-210: LGTM! Schema delete audit logging is correctly implemented.The async/await refactor maintains the existing validation logic while adding comprehensive audit logging for both success and failure scenarios.
src/Routers/UsersRouter.js (2)
359-418: LGTM! Master key login audit logging is well-structured.The handleLogInAs refactor properly wraps the entire flow in try/catch, ensuring that both successful and failed master key login attempts are audited. The loginMethod is correctly set to 'masterkey' to distinguish this authentication path.
202-342: Audit logging covers all login failure paths. Errors thrown synchronously or via promise rejections in_authenticateUserFromRequestalways propagate intohandleLogIn’s catch block, ensuringauditLogController.logUserLoginrecords every failure.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
spec/AuditLogging.e2e.spec.js (2)
53-53: Consider making the wait time configurable or event-driven.The 500ms hardcoded wait on line 53 (and similar waits throughout the suite) could cause flakiness on slower CI systems or pass prematurely on fast systems. Consider either using a polling approach to check for log completion or making the timeout configurable via environment variable.
Example polling approach:
// Helper function to wait for logs async function waitForLogs(folder, minLines = 1, timeout = 5000) { const start = Date.now(); while (Date.now() - start < timeout) { if (fs.existsSync(folder)) { const files = fs.readdirSync(folder); if (files.length > 0) { const content = fs.readFileSync(path.join(folder, files[0]), 'utf8'); const lines = content.split('\n').filter(l => l.trim()); if (lines.length >= minLines) return; } } await new Promise(resolve => setTimeout(resolve, 50)); } } // Usage: await waitForLogs(testLogFolder, 5);
388-426: Consider making performance threshold configurable.The 10-second threshold on line 415 provides a baseline performance check, but different CI environments may have vastly different performance characteristics. Consider making this threshold configurable via environment variable or adjusting it based on empirical CI data.
Example:
const perfThreshold = parseInt(process.env.AUDIT_LOG_PERF_THRESHOLD) || 10000; expect(duration).toBeLessThan(perfThreshold);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
spec/AuditLogController.spec.js(1 hunks)spec/AuditLogSchemas.spec.js(1 hunks)spec/AuditLogging.e2e.spec.js(1 hunks)spec/Auth.spec.js(2 hunks)spec/ParseObject.spec.js(1 hunks)src/Adapters/Logger/AuditLogger.js(1 hunks)src/Controllers/AuditLogController.js(1 hunks)src/RestQuery.js(2 hunks)src/RestWrite.js(2 hunks)src/Routers/UsersRouter.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Adapters/Logger/AuditLogger.js
- spec/AuditLogController.spec.js
- src/Controllers/AuditLogController.js
🧰 Additional context used
🧬 Code graph analysis (5)
src/Routers/UsersRouter.js (2)
src/RestWrite.js (16)
user(330-330)user(901-901)user(1514-1514)user(1787-1787)authData(453-453)Auth(8-8)Auth(575-578)validatedAuthData(560-563)res(624-628)RestWrite(988-995)RestWrite(1153-1159)expiresAt(1009-1009)sessionData(1010-1019)error(666-666)userId(537-537)userId(547-547)src/triggers.js (2)
maybeRunTrigger(849-952)error(644-644)
spec/AuditLogSchemas.spec.js (2)
src/RestWrite.js (4)
request(821-827)request(956-962)schema(346-346)error(666-666)spec/helper.js (1)
reconfigureServer(171-205)
spec/ParseObject.spec.js (1)
spec/helper.js (6)
require(6-6)require(8-8)require(52-52)path(42-42)reconfigureServer(171-205)Parse(4-4)
spec/Auth.spec.js (2)
src/RestWrite.js (3)
request(821-827)request(956-962)error(666-666)spec/helper.js (2)
reconfigureServer(171-205)Parse(4-4)
spec/AuditLogging.e2e.spec.js (1)
spec/helper.js (1)
reconfigureServer(171-205)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: Node 20
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: Docker Build
- GitHub Check: Code Analysis (javascript)
🔇 Additional comments (34)
spec/Auth.spec.js (6)
3-3: LGTM! Clean test suite setup.The request import and test suite initialization follow best practices with proper temp folder cleanup in beforeEach/afterEach hooks.
Also applies to: 260-276
277-306: LGTM! Comprehensive successful login test.The test properly creates a user, performs logout/login, waits for logs to flush, and validates all expected log markers including masking.
308-340: LGTM! Failed login test properly handles errors.The empty catch block from the previous review has been correctly addressed with explicit error assertions (lines 324-327). The test now validates both the expected authentication failure and the corresponding audit log entry.
342-380: LGTM! loginAs test validates master key flow.The test properly exercises the loginAs endpoint with master key authentication and verifies the corresponding audit log entries.
382-405: LGTM! IP address capture test.Clean test validating that client IP addresses are captured in authentication logs.
407-422: LGTM! Negative test for disabled logging.Important test confirming that audit logging is properly disabled when the auditLog configuration is not present.
spec/ParseObject.spec.js (7)
2176-2197: LGTM! Clean CRUD audit test suite setup.The test suite initialization follows the established pattern with proper folder cleanup and server reconfiguration.
2199-2222: LGTM! Object creation audit test.Properly validates that DATA_CREATE events are logged with the expected className and objectId.
2224-2256: LGTM! Object update audit test.The test properly isolates the update operation by clearing initial creation logs, then verifies DATA_UPDATE logging.
2258-2291: LGTM! Object deletion audit test.Correctly captures the objectId before deletion and verifies DATA_DELETE logging.
2293-2326: LGTM! ACL modification audit test.Properly tests that ACL changes are logged separately as ACL_MODIFY events.
2328-2371: LGTM! Masking and user context tests.These tests validate two critical GDPR requirements: sensitive field masking (password) and user context capture (userId in logs).
2373-2389: LGTM! Master key without user test uses correct pattern.The test correctly uses the per-operation
{ useMasterKey: true }option (line 2377) rather than the globalParse.Cloud.useMasterKey()toggle. This validates that internal master-key operations without a user context are properly excluded from audit logs.src/RestQuery.js (2)
305-307: LGTM! Clean integration of audit logging into query execution.The logAuditDataView call is properly inserted after handleAuthAdapters and before returning the response, ensuring audit logging doesn't interfere with the main query flow.
952-985: LGTM! logAuditDataView implementation with past issue resolved.The method properly:
- Guards against disabled/missing audit controller
- Skips master operations without user context (GDPR requirement)
- Limits logged objectIds to 100 (performance consideration)
- Uses
this.config.loggerController.error(line 981) instead ofconsole.erroras flagged in the previous review- Never fails the query on audit logging errors
src/RestWrite.js (2)
157-159: LGTM! Clean integration of audit logging into write execution.The logAuditDataWrite call is properly positioned after cleanUserAuthData and before returning the response, mirroring the pattern in RestQuery.
1801-1867: LGTM! logAuditDataWrite implementation with all past issues resolved.The method correctly:
- Skips only master operations without user context (line 1808), not maintenance mode - addressing previous consistency concern
- Detects ACL changes including added/removed cases using
?? nullfallback (lines 1821-1823) - addressing previous incomplete detection concern- Uses
this.config.loggerController.error(line 1863) instead ofconsole.error- addressing previous linting concern- Logs create events with full data, update events with changed fields only, and ACL modifications separately
- Never fails the write on audit logging errors
spec/AuditLogSchemas.spec.js (6)
1-26: LGTM! Imports and setup with past issue resolved.The request helper import (line 5) that was missing in the previous review has been properly added. The test suite setup follows the established pattern with proper temp folder cleanup.
28-57: LGTM! Schema creation audit test.Properly tests that schema creation via the API generates SCHEMA_MODIFY audit logs with the create operation marker.
59-108: LGTM! Schema update audit test.Correctly isolates the update operation and verifies SCHEMA_MODIFY logging with the update operation marker.
110-154: LGTM! Schema deletion audit test.Properly tests that schema deletion generates SCHEMA_MODIFY audit logs with the delete operation marker.
156-215: LGTM! Failed schema creation and user context tests.The failed creation test properly handles the expected error for invalid class names and verifies failure logging. The user context test confirms that schema operations include user information in audit logs.
217-246: LGTM! Schema field details test.Validates that schema modifications include field-level details in the audit logs, providing comprehensive audit trail for schema changes.
src/Routers/UsersRouter.js (4)
318-326: SessionToken masking implemented correctly.The past review concern about sessionToken leakage has been addressed. Line 320 now masks the sessionToken as
'***masked***'before passing it toauditLogController.logUserLogin, preventing credential exposure in audit logs.
329-341: LGTM! Failure audit logging is well-structured.The catch block correctly logs failed login attempts with error details, extracts username from available sources, determines loginMethod, and rethrows the error to preserve error semantics.
393-402: LGTM! Master-key login audit logging is correct.The audit log correctly masks sessionToken, sets loginMethod to 'masterkey', includes appropriate username fallbacks, and follows the same pattern as handleLogIn.
405-417: LGTM! Master-key login failure logging is consistent.The catch block appropriately logs failed loginAs attempts with userId extraction, error details, and correct loginMethod, while preserving error semantics by rethrowing.
spec/AuditLogging.e2e.spec.js (7)
3-5: Import issue resolved.The missing
requestimport flagged in the previous review has been addressed. Line 5 now correctly imports the request helper.
80-151: LGTM! File management tests are comprehensive.The test suite thoroughly validates log file naming patterns, automatic folder creation, and custom date patterns. The regex patterns correctly match the expected formats based on configuration.
153-203: LGTM! Configuration tests validate dynamic behavior.The tests properly verify that audit logging can be disabled and dynamically enabled at runtime, which is important for operational flexibility.
205-290: LGTM! Content validation is thorough and security-focused.The test suite validates required fields, proper JSON formatting, and critically verifies that sensitive data like passwords and session tokens are masked and never appear in plain text in audit logs.
292-330: LGTM! Concurrent operations test validates thread-safety.The test appropriately validates that the audit logger handles concurrent operations correctly, ensuring all events are captured without loss or corruption.
332-385: LGTM! Error handling tests ensure audit logging is non-blocking.The tests appropriately verify that audit logging failures don't prevent core operations from succeeding, and that failed operations are properly logged with error details. This is critical for GDPR compliance without compromising system reliability.
428-463: LGTM! Cross-feature test validates schema audit integration.The test appropriately validates that schema modifications are audited with correct event types and entities, demonstrating integration beyond user operations as required for comprehensive GDPR compliance.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
GDPR_COMPLIANCE_GUIDE.md (4)
26-26: Scope correction: not “database‑level” loggingAudit events are emitted at the Parse Server request/operation layer (RestQuery/RestWrite/routers), not at the database layer. Please adjust wording to avoid overpromising.
- - Automatic, transparent logging at the database level + - Automatic, transparent logging at the request/operation layer in Parse Server
468-476: IP capture in Cloud Code:request.ipmay be unavailable
request.ipis not guaranteed in Cloud Code. Prefer reading IP from the inbound request context (e.g., Expressreq.ip) exposed by Parse Server if supported, or from a trusted header behind your proxy (X‑Forwarded‑For), normalized server‑side. Otherwise omit IP.- acceptance.set('ipAddress', request.ip); + // If available in your Cloud Code runtime: + const ip = (request.headers && request.headers['x-forwarded-for']) + ? request.headers['x-forwarded-for'].split(',')[0].trim() + : undefined; + if (ip) acceptance.set('ipAddress', ip);Please confirm which fields Parse exposes on
requestin your deployment.
525-565: Breach detection example: hard‑coded log filename and dateUsing a fixed filename (
2025-10-01) will break with rotation. Parameterize the folder/pattern and iterate recent files.- const logFile = './audit-logs/parse-server-audit-2025-10-01.log'; + const path = require('path'); + const folder = process.env.AUDIT_LOG_FOLDER || './audit-logs'; + const files = fs.readdirSync(folder) + .filter(f => f.startsWith('parse-server-audit-') && f.endsWith('.log')) + .sort() // latest last if date in name + .slice(-3); // check last N files + for (const f of files) { + const fileStream = fs.createReadStream(path.join(folder, f)); + // ... process lines as below + }Also define
notifySecurityTeamor make the call path explicit.
639-655: Tamper‑evidence: prefer proven WORM/immutability over “blockchain”Recommend S3 Object Lock, Azure Immutable Blob, or append‑only/WORM filesystems with retention enforcement instead of suggesting blockchain.
- - Backup to immutable storage - - Consider blockchain for tamper-evidence + - Backup to immutable storage (e.g., S3 Object Lock, Azure Immutable Blob) + - Enforce WORM/retention policies for tamper‑evidence
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
GDPR_COMPLIANCE_GUIDE.md(1 hunks)
🔇 Additional comments (2)
GDPR_COMPLIANCE_GUIDE.md (2)
433-440: Replace in-app scheduling in docs with external schedulerUsing
node-schedulein multi-instance servers duplicates jobs. Invert control: remove theschedule.scheduleJobsnippet inGDPR_COMPLIANCE_GUIDE.mdand instead call the job endpoint from an external scheduler. For example:-// Run daily at 2 AM -schedule.scheduleJob('0 2 * * *', async () => { - await Parse.Cloud.startJob('enforceDataRetention'); -}); +// Use external scheduler (cron/K8s CronJob/GitHub Actions) to POST /jobs/enforceDataRetention +// Example (cron): +// 0 2 * * * curl -X POST "https://your-server/jobs/enforceDataRetention" \ +// -H "X-Parse-Master-Key: <masterKey>"
Parse.Cloud.startJobis supported by Parse Server and can be invoked via the REST endpoint when called externally.
39-49: Config snippet: align keys/defaults and add security caveat
- Use exact
auditLogconfig keys and defaults from src/Options/Definitions.js (auditLogFolder, datePattern, maxSize, maxFiles).- Add a note that audit logs may contain personal data and must be stored on restricted, immutable storage.
|
@dblythy, this looks great and something we will use extensively rather than our current home-brewed audit log. My only concern is that the logs are stored on the server, which means they will be lost on each deploy for Docker users. Is there an external storage (S3) approach that would be easy to implement? |
|
I wonder whether storing in log files serves the purpose. To be efficiently searchable in regular audits it would better be stored in a DB. That also allows to easily set up triggers, automated reports, etc. So maybe there should be an auditLogger adapter interface that allows to plug in adapters to store on S3, in DB, or any other 3rd party system. |
This sounds great to me. From experience running audit logs for write and delete events, it gets huge really quickly. Currently 10x the size of our largest collection. It would be great to have a Redis-style policy for audit logs so that those older than 30 days are automatically deleted; however, it would need a background job in the current parse setup. |
|
I think you can add a logger adapter: https://community.parseplatform.org/t/logging-using-external-services-cloudwatch-datadog-etc/1036/3 |
|
I believe the GDPR auditable information should be logged separately from other server logs; is this going into distinct files? Logging as JSON or CSV is fine I think; then it can be ingested and parsed like with a AWS Firehose pipeline. |
|
@coderabbitai explain the architecture of the audit logging this PR adds. Does it allow to save the GDPR logs into distinct log files? Is the feature and logging output suitable for the industry standard of GDPR auditable logging? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 24771 @mtrezza, I'll explain the audit logging architecture added in this PR: Architecture OverviewThe implementation uses a three-layer architecture: 1. AuditLogger Layer (
|
That makes sense to me. For example, storing on S3 would allow to store immutably through S3 bucket settings. This brings a higher data confidence level than storing as log files on the instance and waiting for a job to ingest the log files and store them somewhere else where they only then become immutable. I think we also need some filter settings to define what should be logged, to reduce unnecessary logging and tailor to purpose and jurisdiction. |
There was a problem hiding this comment.
Actionable comments posted: 16
♻️ Duplicate comments (2)
GDPR_COMPLIANCE_GUIDE.md (2)
188-194: Avoid exporting full user JSON; whitelist explicit fields.Using request.user.toJSON() can leak private fields (e.g., sessionToken). Build the user object explicitly.
Apply:
- const exportData = { - exportDate: new Date().toISOString(), - user: request.user.toJSON(), - relatedData: {} - }; + const exportData = { + exportDate: new Date().toISOString(), + user: { + id: request.user.id, + username: request.user.get('username'), + email: request.user.get('email'), + createdAt: request.user.createdAt, + updatedAt: request.user.updatedAt, + }, + relatedData: {} + };
1013-1016: Broken link: GDPR_AUDIT_LOGGING.md is missing.Fix the reference or add the document to the repo. Validate all intra-doc links.
- - [GDPR_AUDIT_LOGGING.md](./GDPR_AUDIT_LOGGING.md) - Audit logging setup and configuration + - [Audit logging setup and configuration](./README.md#audit-logging) <!-- or the correct existing doc path -->
🧹 Nitpick comments (14)
GDPR_COMPLIANCE_GUIDE.md (2)
26-31: Wording: not “database-level” logging.This is server-level auditing integrated with REST/DB operations, not DB-engine level. Soften phrasing to avoid overclaim.
- - Automatic, transparent logging at the database level + - Automatic, transparent server-level logging integrated with database operations
482-489: markdownlint MD036: use a heading instead of bold text.Convert emphasis-only line to a proper heading.
-**Note on Tracking User Inactivity:** +#### Note on tracking user inactivitysrc/rest.js (1)
167-179: Same as above in error path.Propagate a real req or omit it; keep logs non-blocking.
- config.auditLogController?.logDataDelete({ - auth, - req: { config }, + config.auditLogController?.logDataDelete({ + auth, + req: context?.req, className, objectId, success: false, error: error.message, });src/Options/Definitions.js (1)
1011-1017: Generated ‘filter’ is required; should be optional.After changing src/Options/index.js (make filter optional), regenerate this file to drop required: true here.
Run the generator to refresh Definitions.js.
spec/Auth.spec.js (1)
408-423: Assertion for “audit logging disabled” relies on folder absence.If shorthand support is added server-side, ensure disabled path still yields no folder. Consider asserting adapter disabled flag instead when available.
spec/ParseObject.spec.js (1)
2212-2213: Replace fixed sleeps with a polling waitForLogs helperFixed 200ms waits are brittle under CI load. Poll for expected content with a timeout to eliminate test flakes. Based on learnings.
+const waitForLogs = async (file, predicate, timeoutMs = 5000, intervalMs = 100) => { + const start = Date.now(); + while (Date.now() - start < timeoutMs) { + if (fs.existsSync(file)) { + const text = fs.readFileSync(file, 'utf8'); + if (predicate(text)) return text; + } + await new Promise(r => setTimeout(r, intervalMs)); + } + throw new Error('Timed out waiting for expected log content'); +}; @@ - await new Promise(resolve => setTimeout(resolve, 200)); + const logFiles = getLogFiles(testLogFolder); + expect(logFiles.length).toBeGreaterThan(0); + const logFile = path.join(testLogFolder, logFiles[0]); + const logContent = await waitForLogs(logFile, t => t.includes('DATA_CREATE')); @@ - await new Promise(resolve => setTimeout(resolve, 200)); + const logContent = await waitForLogs(path.join(testLogFolder, getLogFiles(testLogFolder)[0]), t => + t.includes('DATA_UPDATE') + );Also applies to: 2240-2241, 2270-2271, 2303-2304, 2327-2328, 2349-2350, 2365-2366
spec/AuditLogFilter.spec.js (3)
91-113: Add test: includeRoles should exclude events with no rolesSpec codifies that when includeRoles is set and event.roles is empty, event should be excluded. Add a direct test.
describe('role filtering', () => { @@ it('should filter by includeRoles', () => { @@ }); + + it('should exclude events with no roles when includeRoles is set', () => { + const filter = new AuditLogFilter({ + includeRoles: ['admin'], + }); + expect(filter.shouldLog(createEvent({ roles: [] }))).toBe(false); + expect(filter.shouldLog(createEvent({ roles: undefined }))).toBe(false); + });
91-113: Clarify precedence: excludeRoles should win over includeRolesAdd a test demonstrating that an excluded role on the user prevents logging even if an included role is also present.
describe('role filtering', () => { @@ it('should filter by excludeRoles', () => { @@ }); + + it('should prioritize excludeRoles over includeRoles on overlap', () => { + const filter = new AuditLogFilter({ + includeRoles: ['admin'], + excludeRoles: ['banned'], + }); + expect(filter.shouldLog(createEvent({ roles: ['admin', 'banned'] }))).toBe(false); + });Also applies to: 137-217
38-69: Test includeClasses and excludeClasses togetherWhen both are set, excluded classes should be filtered out even if included, and non-included classes should remain excluded. Add coverage.
describe('class name filtering', () => { @@ it('should handle events without className', () => { @@ }); + + it('should apply excludeClasses before includeClasses', () => { + const filter = new AuditLogFilter({ + includeClasses: ['_User', 'Order'], + excludeClasses: ['_User'], + }); + expect(filter.shouldLog(createEvent({ className: '_User' }))).toBe(false); + expect(filter.shouldLog(createEvent({ className: 'Order' }))).toBe(true); + expect(filter.shouldLog(createEvent({ className: 'Other' }))).toBe(false); + });spec/ParseQuery.spec.js (1)
5418-5419: Use a reusable waitForLogs instead of fixed 200ms sleepsPolling for specific markers eliminates timing flakes in CI. Based on learnings.
+const waitForLogs = async (folder, includes, timeoutMs = 5000) => { + const start = Date.now(); + while (Date.now() - start < timeoutMs) { + const files = getLogFiles(folder); + if (files.length) { + const text = fs.readFileSync(path.join(folder, files[0]), 'utf8'); + if (includes.every(s => text.includes(s))) return text; + } + await new Promise(r => setTimeout(r, 100)); + } + throw new Error('Timed out waiting for expected log content'); +}; @@ - await new Promise(resolve => setTimeout(resolve, 200)); - const logFiles = getLogFiles(testLogFolder); - expect(logFiles.length).toBeGreaterThan(0); - const logFile = path.join(testLogFolder, logFiles[0]); - const logContent = fs.readFileSync(logFile, 'utf8'); + const logContent = await waitForLogs(testLogFolder, ['DATA_VIEW', 'AuditTest']);Also applies to: 5451-5452, 5479-5480, 5500-5501, 5524-5525, 5555-5556
spec/AuditLogging.e2e.spec.js (2)
11-15: Add getLogFiles existence guardSame rationale as other suites: avoid readdirSync ENOENT.
-const getLogFiles = (folder) => fs.readdirSync(folder).filter(f => f.endsWith('.log')); +const getLogFiles = folder => + fs.existsSync(folder) ? fs.readdirSync(folder).filter(f => f.endsWith('.log')) : [];
56-63: Replace fixed sleeps with a reusable waitForLogs helperUse a polling helper to wait for specific markers to reduce flakes across e2e tests. Based on learnings.
+const waitForLogs = async (folder, includes, timeoutMs = 10000) => { + const start = Date.now(); + while (Date.now() - start < timeoutMs) { + const files = getLogFiles(folder); + if (files.length) { + const text = fs.readFileSync(path.join(folder, files[0]), 'utf8'); + if (includes.every(s => text.includes(s))) return text; + } + await new Promise(r => setTimeout(r, 100)); + } + throw new Error('Timed out waiting for expected log content'); +}; @@ - await new Promise(resolve => setTimeout(resolve, 500)); - const logFiles = getLogFiles(testLogFolder); - expect(logFiles.length).toBeGreaterThan(0); - const logFile = path.join(testLogFolder, logFiles[0]); - const logContent = fs.readFileSync(logFile, 'utf8'); + const logContent = await waitForLogs(testLogFolder, ['USER_LOGIN','DATA_CREATE','DATA_VIEW','DATA_UPDATE','DATA_DELETE']);Also applies to: 100-107, 124-131, 148-155, 172-175, 186-189, 204-209, 231-248, 265-275, 293-302, 328-336, 389-401, 436-444, 474-483
src/Adapters/AuditLog/AuditLogFilter.ts (1)
125-131: Consider using a type guard instead of type assertion.The type assertion
(event as any).classNamebypasses TypeScript's type safety. Consider using a type guard to check if the event has a className property in a type-safe manner.Apply this diff to improve type safety:
/** * Extract className from event based on event structure */ private extractClassName(event: AuditEvent): string | undefined { - // Type-safe extraction based on event structure - if ('className' in event) { - return (event as any).className; + // Type-safe extraction using type guard + if ('className' in event && typeof (event as Record<string, unknown>).className === 'string') { + return (event as Record<string, unknown>).className as string; } return undefined; }src/Adapters/Logger/AuditLogger.js (1)
45-47: Consider using the application logger instead of console.error.While console.error is functional for configuration errors, using the application's standard logger would provide better consistency and configurability for production deployments.
Also applies to: 76-78
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
GDPR_COMPLIANCE_GUIDE.md(1 hunks)resources/buildConfigDefinitions.js(4 hunks)spec/AuditLogAdapter.spec.js(1 hunks)spec/AuditLogController.spec.js(1 hunks)spec/AuditLogFilter.spec.js(1 hunks)spec/AuditLogSchemas.spec.js(1 hunks)spec/AuditLogging.e2e.spec.js(1 hunks)spec/Auth.spec.js(2 hunks)spec/ParseObject.spec.js(1 hunks)spec/ParseQuery.spec.js(1 hunks)src/Adapters/AuditLog/AuditLogAdapterInterface.ts(1 hunks)src/Adapters/AuditLog/AuditLogFilter.ts(1 hunks)src/Adapters/AuditLog/WinstonFileAuditLogAdapter.ts(1 hunks)src/Adapters/AuditLog/index.ts(1 hunks)src/Adapters/Logger/AuditLogger.js(1 hunks)src/Controllers/AuditLogController.ts(1 hunks)src/Controllers/index.js(5 hunks)src/Options/Definitions.js(2 hunks)src/Options/docs.js(2 hunks)src/Options/index.js(2 hunks)src/rest.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- spec/AuditLogSchemas.spec.js
- spec/AuditLogAdapter.spec.js
- spec/AuditLogController.spec.js
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-10-16T19:27:05.311Z
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 9883
File: spec/CloudCodeLogger.spec.js:410-412
Timestamp: 2025-10-16T19:27:05.311Z
Learning: In spec/CloudCodeLogger.spec.js, the test "should log cloud function triggers using the silent log level" (around lines 383-420) is known to be flaky and requires the extra `await new Promise(resolve => setTimeout(resolve, 100))` timeout after awaiting `afterSavePromise` for reliability, even though it may appear redundant.
Applied to files:
spec/AuditLogging.e2e.spec.jsspec/AuditLogFilter.spec.jsspec/ParseQuery.spec.jsspec/Auth.spec.jsspec/ParseObject.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
spec/AuditLogging.e2e.spec.jsspec/ParseQuery.spec.jsspec/Auth.spec.jsspec/ParseObject.spec.js
📚 Learning: 2025-04-30T19:31:35.344Z
Learnt from: RahulLanjewar93
Repo: parse-community/parse-server PR: 9744
File: spec/ParseLiveQuery.spec.js:0-0
Timestamp: 2025-04-30T19:31:35.344Z
Learning: In the Parse Server codebase, the functions in QueryTools.js are typically tested through end-to-end behavior tests rather than direct unit tests, even though the functions are exported from the module.
Applied to files:
spec/AuditLogging.e2e.spec.jsspec/ParseQuery.spec.jsspec/Auth.spec.jsspec/ParseObject.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
spec/AuditLogging.e2e.spec.jsspec/ParseQuery.spec.jsspec/Auth.spec.jsspec/ParseObject.spec.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
spec/AuditLogging.e2e.spec.jsspec/ParseQuery.spec.jsspec/Auth.spec.jsspec/ParseObject.spec.js
📚 Learning: 2025-08-27T12:33:06.237Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:467-477
Timestamp: 2025-08-27T12:33:06.237Z
Learning: In the Parse Server codebase, maybeRunAfterFindTrigger is called in production with Parse.Query objects constructed via withJSON(), so the plain object query handling bug only affects tests, not production code paths.
Applied to files:
spec/ParseQuery.spec.jsspec/ParseObject.spec.js
📚 Learning: 2025-08-27T09:08:34.252Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:446-454
Timestamp: 2025-08-27T09:08:34.252Z
Learning: When analyzing function signature changes in Parse Server codebase, verify that call sites are actually incorrect before flagging them. Passing tests are a strong indicator that function calls are already properly aligned with new signatures.
Applied to files:
spec/Auth.spec.jsspec/ParseObject.spec.js
🧬 Code graph analysis (12)
spec/AuditLogging.e2e.spec.js (3)
src/RestWrite.js (3)
request(821-827)request(956-962)error(666-666)src/defaults.js (1)
folder(4-4)spec/helper.js (1)
reconfigureServer(171-205)
spec/AuditLogFilter.spec.js (1)
src/Adapters/AuditLog/AuditLogFilter.ts (1)
AuditLogFilter(27-206)
src/Options/Definitions.js (2)
resources/buildConfigDefinitions.js (1)
parsers(12-12)src/Adapters/AdapterLoader.js (1)
module(49-49)
src/Adapters/Logger/AuditLogger.js (1)
src/Controllers/index.js (11)
options(62-71)options(85-85)options(110-118)options(131-131)options(139-139)options(159-159)options(169-169)options(170-170)options(190-190)options(202-202)options(240-240)
src/Options/index.js (2)
src/Adapters/AuditLog/WinstonFileAuditLogAdapter.ts (1)
WinstonFileAuditLogAdapterOptions(22-31)src/Adapters/AuditLog/index.ts (1)
WinstonFileAuditLogAdapterOptions(19-19)
spec/ParseQuery.spec.js (5)
spec/AuditLogAdapter.spec.js (4)
fs(4-4)path(5-5)testLogFolder(8-8)getLogFiles(9-9)spec/AuditLogging.e2e.spec.js (4)
fs(3-3)path(4-4)testLogFolder(8-8)getLogFiles(9-9)spec/ParseObject.spec.js (9)
fs(2177-2177)path(2178-2178)testLogFolder(2179-2179)getLogFiles(2180-2180)i(1341-1341)i(1419-1419)i(1496-1496)i(1563-1563)i(1605-1605)spec/helper.js (6)
require(6-6)require(8-8)path(42-42)reconfigureServer(171-205)Parse(4-4)i(324-324)src/defaults.js (1)
folder(4-4)
spec/Auth.spec.js (3)
spec/AuditLogging.e2e.spec.js (5)
request(5-5)fs(3-3)path(4-4)testLogFolder(8-8)getLogFiles(9-9)spec/AuditLogAdapter.spec.js (5)
require(3-3)fs(4-4)path(5-5)testLogFolder(8-8)getLogFiles(9-9)spec/helper.js (6)
require(6-6)require(8-8)require(52-52)path(42-42)reconfigureServer(171-205)Parse(4-4)
src/Controllers/index.js (5)
types/Options/index.d.ts (1)
ParseServerOptions(29-125)src/Controllers/AuditLogController.ts (1)
AuditLogController(33-423)spec/helper.js (1)
loadAdapter(41-41)src/Adapters/AuditLog/WinstonFileAuditLogAdapter.ts (1)
WinstonFileAuditLogAdapter(37-240)src/Adapters/AuditLog/index.ts (1)
WinstonFileAuditLogAdapter(2-2)
spec/ParseObject.spec.js (1)
spec/helper.js (6)
require(6-6)require(8-8)require(52-52)path(42-42)reconfigureServer(171-205)Parse(4-4)
src/Adapters/AuditLog/AuditLogFilter.ts (1)
src/Adapters/AuditLog/AuditLogAdapterInterface.ts (1)
AuditEvent(18-29)
src/Adapters/AuditLog/WinstonFileAuditLogAdapter.ts (2)
src/Adapters/Logger/AuditLogger.js (2)
logFolder(69-69)auditEntry(111-123)src/Adapters/AuditLog/AuditLogAdapterInterface.ts (9)
AuditEvent(18-29)UserLoginEvent(34-38)DataViewEvent(43-49)DataCreateEvent(54-59)DataUpdateEvent(64-69)DataDeleteEvent(74-78)ACLModifyEvent(83-88)SchemaModifyEvent(93-98)PushSendEvent(103-108)
src/Controllers/AuditLogController.ts (5)
src/Adapters/AuditLog/AuditLogFilter.ts (2)
AuditLogFilterConfig(6-21)AuditLogFilter(27-206)src/Adapters/AuditLog/index.ts (12)
AuditLogFilterConfig(18-18)AuditLogAdapterInterface(1-1)AuditLogFilter(3-3)AuditEvent(6-6)UserLoginEvent(8-8)DataViewEvent(9-9)DataCreateEvent(10-10)DataUpdateEvent(11-11)DataDeleteEvent(12-12)ACLModifyEvent(13-13)SchemaModifyEvent(14-14)PushSendEvent(15-15)src/Controllers/index.js (11)
options(62-71)options(85-85)options(110-118)options(131-131)options(139-139)options(159-159)options(169-169)options(170-170)options(190-190)options(202-202)options(240-240)src/Adapters/AuditLog/AuditLogAdapterInterface.ts (9)
AuditEvent(18-29)UserLoginEvent(34-38)DataViewEvent(43-49)DataCreateEvent(54-59)DataUpdateEvent(64-69)DataDeleteEvent(74-78)ACLModifyEvent(83-88)SchemaModifyEvent(93-98)PushSendEvent(103-108)src/RestWrite.js (2)
updatedFields(1837-1837)newACL(1822-1822)
🪛 Biome (2.1.2)
src/Options/index.js
[error] 216-216: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
[error] 544-544: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
[error] 544-544: Expected a statement but instead found '}'.
Expected a statement here.
(parse)
[error] 549-549: Expected a type but instead found '?'.
Expected a type here.
(parse)
[error] 549-549: Expected a property, or a signature but instead found ';'.
Expected a property, or a signature here.
(parse)
[error] 553-553: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
[error] 557-557: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
[error] 559-559: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
[error] 563-563: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
[error] 567-567: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
[error] 575-575: Expected a statement but instead found '}'.
Expected a statement here.
(parse)
[error] 578-578: Expected a type but instead found '?'.
Expected a type here.
(parse)
[error] 578-578: Expected a property, or a signature but instead found ';'.
Expected a property, or a signature here.
(parse)
[error] 580-580: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
[error] 582-582: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
[error] 586-586: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
[error] 586-586: Expected a statement but instead found '}'.
Expected a statement here.
(parse)
[error] 600-600: Expected a type but instead found '?'.
Expected a type here.
(parse)
[error] 601-601: Expected a property, or a signature but instead found ';'.
Expected a property, or a signature here.
(parse)
src/Controllers/index.js
[error] 84-84: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 84-84: return types can only be used in TypeScript files
(parse)
🪛 GitHub Check: Lint
src/Adapters/Logger/AuditLogger.js
[warning] 77-77:
Unexpected console statement
[warning] 46-46:
Unexpected console statement
🪛 markdownlint-cli2 (0.18.1)
GDPR_COMPLIANCE_GUIDE.md
573-573: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Docker Build
🔇 Additional comments (13)
src/Options/docs.js (1)
23-23: LGTM – auditLog option documented clearly.resources/buildConfigDefinitions.js (2)
34-38: Env prefixes for audit options: good.Prefixes for AuditLogOptions and AuditLogFilterOptions are consistent with docs.
289-297: Safer union-default parsing guard: good.The added Array.isArray check prevents crashes on union defaults.
src/Controllers/index.js (1)
30-59: Expose auditLogController in getControllers: LGTM.Wiring follows existing adapter/controller patterns.
src/Adapters/AuditLog/index.ts (1)
3-3: The verification confirms thatshouldLogEventis properly exported fromAuditLogFilter.ts. The script found the export at line 211:src/Adapters/AuditLog/AuditLogFilter.ts:211:export function shouldLogEvent(event: AuditEvent, config: AuditLogFilterConfig): booleanThe barrel export in
src/Adapters/AuditLog/index.tsis correct and will not cause build failures.Likely an incorrect or invalid review comment.
src/Adapters/AuditLog/AuditLogFilter.ts (1)
188-205: LGTM - Good fail-open error handling.The fail-open behavior with console.error is appropriate here. It ensures that filter bugs don't block all audit logging, which is critical for compliance. The console.error provides visibility while maintaining system robustness.
src/Adapters/Logger/AuditLogger.js (1)
105-133: LGTM - Solid implementation with good defensive checks.The audit event logging properly checks for enabled state, masks sensitive data (sessionToken), and removes undefined fields for clean output. The defaults are sensible.
src/Adapters/AuditLog/WinstonFileAuditLogAdapter.ts (2)
115-127: LGTM - Proper cleanup lifecycle.The close method properly checks state, closes the logger, and updates the enabled flag. Good lifecycle management.
181-239: LGTM - Clean delegation pattern.All logging methods appropriately delegate to the centralized writeEvent method. This ensures consistent behavior across all event types.
src/Adapters/AuditLog/AuditLogAdapterInterface.ts (1)
1-188: LGTM - Comprehensive and well-structured interface definitions.The type definitions and abstract class provide a clear, type-safe contract for audit logging:
- Complete event type coverage for GDPR requirements
- Consistent base interface with appropriate optional fields
- Specific event interfaces with relevant data
- Clear adapter interface with lifecycle hooks
src/Controllers/AuditLogController.ts (3)
50-67: LGTM - Comprehensive IP address extraction.The IP address extraction properly handles proxied environments by checking X-Forwarded-For and X-Real-IP headers before falling back to the connection's remote address. The X-Forwarded-For parsing correctly takes the first IP in the chain.
112-146: LGTM - Appropriate fire-and-forget pattern.The fire-and-forget approach ensures audit logging doesn't block application operations. The filter check before logging enables efficient event filtering, and error handling logs failures without throwing, which is correct for audit logging infrastructure.
151-415: LGTM - Consistent logging method implementations.All logging methods follow a consistent pattern:
- Extract user context from auth
- Build typed event object with all required fields
- Delegate to writeEvent for filtering and persistence
The consistency makes the code maintainable and the type-safe event construction ensures compliance with the adapter interface.
| consent.set('ipAddress', request.ip); | ||
|
|
||
| await consent.save(null, { useMasterKey: true }); |
There was a problem hiding this comment.
Use reliable client IP source in Cloud Code.
request.ip may be undefined in Parse Cloud. Prefer headers (X-Forwarded-For) or store IP via server middleware/audit logger.
Example:
const ip =
request.headers?.['x-forwarded-for']?.split(',')[0]?.trim() ||
request.headers?.['x-real-ip'] ||
undefined;
// consent.set('ipAddress', ip);Also applies to: 641-643
🤖 Prompt for AI Agents
In GDPR_COMPLIANCE_GUIDE.md around lines 419-421 (and similarly 641-643), the
code uses request.ip which can be undefined in Parse Cloud; update the Cloud
Code to extract the client IP reliably from request.headers (check
X-Forwarded-For, take the first comma-separated value and trim, then fallback to
X-Real-IP) and only set consent.ipAddress when a value is present, or
alternatively populate the IP via server middleware/audit logger before this
code path; ensure you handle missing headers cleanly (leave undefined) and keep
the existing save call with master key.
| await reconfigureServer({ | ||
| auditLog: { | ||
| adapterOptions: { | ||
| auditLogFolder: testLogFolder, | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Unify auditLog config shape across tests
Some tests set auditLog.auditLogFolder; others set auditLog.adapterOptions.auditLogFolder. Verify which is supported by Options/Definitions and standardize to prevent false negatives.
Would you like me to update the specs once we confirm the canonical shape?
Also applies to: 85-93, 112-116, 134-141, 189-195
🏁 Script executed:
#!/bin/bash
# Inspect Option definitions and adapter for expected auditLog shape.
rg -nP --type=ts -C3 '\bauditLog\b' src/Options src/Adapters src | sed -n '1,120p'
rg -nP -C3 'WinstonFileAuditLogAdapterOptions|auditLogFolder|adapterOptions' srcLength of output: 10338
🏁 Script executed:
#!/bin/bash
# Inspect test file at flagged line ranges to identify config shape usage
sed -n '20,40p' spec/AuditLogging.e2e.spec.js
sed -n '80,100p' spec/AuditLogging.e2e.spec.js
sed -n '107,120p' spec/AuditLogging.e2e.spec.js
sed -n '130,150p' spec/AuditLogging.e2e.spec.js
sed -n '185,200p' spec/AuditLogging.e2e.spec.jsLength of output: 2397
Fix auditLog config shape in two tests to match canonical structure
The codebase standardizes on auditLog.adapterOptions.auditLogFolder, but lines 112–116 and 134–141 use incorrect direct auditLog.auditLogFolder. Update both locations to wrap auditLogFolder inside adapterOptions.
Lines 112–116: Change from auditLog: { auditLogFolder: newFolder, } to auditLog: { adapterOptions: { auditLogFolder: newFolder, }, }
Lines 134–141: Change from auditLog: { auditLogFolder: testLogFolder, datePattern: 'YYYY-MM', } to auditLog: { adapterOptions: { auditLogFolder: testLogFolder, datePattern: 'YYYY-MM', }, }
🤖 Prompt for AI Agents
In spec/AuditLogging.e2e.spec.js around lines 112-116 and 134-141, the auditLog
configuration uses the wrong shape (auditLog.auditLogFolder and
auditLog.datePattern) — update both to nest these under adapterOptions so they
read auditLog: { adapterOptions: { auditLogFolder: <value>, [datePattern:
'YYYY-MM' where applicable], }, } ensuring both tests match the canonical
auditLog.adapterOptions.auditLogFolder structure.
| const userPromises = []; | ||
| for (let i = 0; i < 10; i++) { | ||
| const user = new Parse.User(); | ||
| userPromises.push( | ||
| user.signUp({ | ||
| username: `concurrent${i}`, | ||
| password: 'password123', | ||
| }) | ||
| ); | ||
| } | ||
|
|
||
| await Promise.all(userPromises); | ||
|
|
||
| await new Promise(resolve => setTimeout(resolve, 300)); | ||
|
|
||
| const logFiles = getLogFiles(testLogFolder); | ||
| const logFile = path.join(testLogFolder, logFiles[0]); | ||
| const logContent = fs.readFileSync(logFile, 'utf8'); | ||
| const logLines = logContent.split('\n').filter(line => line.trim().length > 0); | ||
|
|
||
| expect(logLines.length).toBeGreaterThanOrEqual(10); | ||
|
|
||
| const parsedLogs = logLines.map(line => JSON.parse(line)); | ||
| expect(parsedLogs.length).toBeGreaterThanOrEqual(10); | ||
|
|
||
| for (let i = 0; i < 10; i++) { | ||
| const userLogs = logLines.filter(line => line.includes(`concurrent${i}`)); | ||
| expect(userLogs.length).toBeGreaterThan(0); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Decouple assertions from usernames; assert by event type or userId
Logs may intentionally omit usernames for GDPR. Prefer counting USER_LOGIN events or checking userId presence.
- for (let i = 0; i < 10; i++) {
- const userLogs = logLines.filter(line => line.includes(`concurrent${i}`));
- expect(userLogs.length).toBeGreaterThan(0);
- }
+ const loginCount = parsedLogs.filter(l => l.eventType === 'USER_LOGIN' && l.success === true).length;
+ expect(loginCount).toBeGreaterThanOrEqual(10);
+ // Or verify userId field exists on each login
+ parsedLogs.filter(l => l.eventType === 'USER_LOGIN').forEach(l => {
+ expect(typeof l.userId).toBe('string');
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const userPromises = []; | |
| for (let i = 0; i < 10; i++) { | |
| const user = new Parse.User(); | |
| userPromises.push( | |
| user.signUp({ | |
| username: `concurrent${i}`, | |
| password: 'password123', | |
| }) | |
| ); | |
| } | |
| await Promise.all(userPromises); | |
| await new Promise(resolve => setTimeout(resolve, 300)); | |
| const logFiles = getLogFiles(testLogFolder); | |
| const logFile = path.join(testLogFolder, logFiles[0]); | |
| const logContent = fs.readFileSync(logFile, 'utf8'); | |
| const logLines = logContent.split('\n').filter(line => line.trim().length > 0); | |
| expect(logLines.length).toBeGreaterThanOrEqual(10); | |
| const parsedLogs = logLines.map(line => JSON.parse(line)); | |
| expect(parsedLogs.length).toBeGreaterThanOrEqual(10); | |
| for (let i = 0; i < 10; i++) { | |
| const userLogs = logLines.filter(line => line.includes(`concurrent${i}`)); | |
| expect(userLogs.length).toBeGreaterThan(0); | |
| } | |
| }); | |
| const userPromises = []; | |
| for (let i = 0; i < 10; i++) { | |
| const user = new Parse.User(); | |
| userPromises.push( | |
| user.signUp({ | |
| username: `concurrent${i}`, | |
| password: 'password123', | |
| }) | |
| ); | |
| } | |
| await Promise.all(userPromises); | |
| await new Promise(resolve => setTimeout(resolve, 300)); | |
| const logFiles = getLogFiles(testLogFolder); | |
| const logFile = path.join(testLogFolder, logFiles[0]); | |
| const logContent = fs.readFileSync(logFile, 'utf8'); | |
| const logLines = logContent.split('\n').filter(line => line.trim().length > 0); | |
| expect(logLines.length).toBeGreaterThanOrEqual(10); | |
| const parsedLogs = logLines.map(line => JSON.parse(line)); | |
| expect(parsedLogs.length).toBeGreaterThanOrEqual(10); | |
| const loginCount = parsedLogs.filter(l => l.eventType === 'USER_LOGIN' && l.success === true).length; | |
| expect(loginCount).toBeGreaterThanOrEqual(10); | |
| // Or verify userId field exists on each login | |
| parsedLogs.filter(l => l.eventType === 'USER_LOGIN').forEach(l => { | |
| expect(typeof l.userId).toBe('string'); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In spec/AuditLogging.e2e.spec.js around lines 315 to 344, the test currently
asserts log entries by searching for usernames, which may be omitted for GDPR;
change the assertions to decouple from usernames by instead parsing each log
line as JSON and asserting by event type (e.g., count entries where event ===
'USER_LOGIN' or the equivalent) and/or by checking presence of userId fields
that match the signed-up users' ids (capture the returned user objects/ids when
signUp resolves). Ensure you await Promise.all(signUpPromises) to collect user
ids, parse logLines into JSON first, then assert parsedLogs.filter(l => l.event
=== 'USER_LOGIN').length >= 10 and/or that each created userId appears in at
least one parsed log entry.
| const startTime = Date.now(); | ||
|
|
||
| for (let i = 0; i < 100; i++) { | ||
| const obj = new TestClass(); | ||
| obj.set('index', i); | ||
| await obj.save(); | ||
| } | ||
|
|
||
| const endTime = Date.now(); | ||
| const duration = endTime - startTime; | ||
|
|
||
| // Should complete in reasonable time even with audit logging enabled | ||
| expect(duration).toBeLessThan(10000); | ||
|
|
There was a problem hiding this comment.
Avoid brittle wall-clock assertions in perf test
A 10s cap for 100 sequential saves can flake on CI. Either parallelize with bounded concurrency or drop the timing check and assert only on log count.
- const startTime = Date.now();
- for (let i = 0; i < 100; i++) {
- const obj = new TestClass();
- obj.set('index', i);
- await obj.save();
- }
- const duration = Date.now() - startTime;
- expect(duration).toBeLessThan(10000);
+ const tasks = [];
+ for (let i = 0; i < 100; i++) {
+ const obj = new TestClass();
+ obj.set('index', i);
+ tasks.push(obj.save());
+ }
+ await Promise.all(tasks);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In spec/AuditLogging.e2e.spec.js around lines 422 to 435, the test uses a
brittle wall-clock assertion (expect(duration).toBeLessThan(10000)) for 100
sequential saves which flakes on CI; replace this by either running the 100
saves with bounded concurrency (e.g., use a small concurrency limiter like
p-limit or implement batches and await Promise.all for each batch) to keep
runtime fast and stable, or remove the timing assertion entirely and instead
assert deterministically on side-effects such as verifying the audit log count
equals 100 (or expected entries) after all saves complete; implement one of
these two changes and remove the flaky duration expectation.
| it('should log successful user login', async () => { | ||
| await reconfigureServer({ | ||
| auditLog: { | ||
| auditLogFolder: testLogFolder, | ||
| }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Config shape mismatch with new auditLog options.
Tests use shorthand auditLog: { auditLogFolder }, but the code expects { adapter, adapterOptions }. Either update tests or make server accept shorthand.
Option A (update test config):
- await reconfigureServer({
- auditLog: {
- auditLogFolder: testLogFolder,
- },
- });
+ await reconfigureServer({
+ auditLog: {
+ adapter: 'winston-file',
+ adapterOptions: { auditLogFolder: testLogFolder },
+ },
+ });Option B (preferred): keep tests and add shorthand support in getAuditLogController (see suggested diff in src/Controllers/index.js).
🤖 Prompt for AI Agents
In spec/Auth.spec.js around lines 278 to 284 the test passes auditLog: {
auditLogFolder } but the server now expects auditLog to be { adapter,
adapterOptions }; update getAuditLogController to accept the shorthand shape:
detect when auditLog.auditLogFolder (or auditLog.auditLogPath) is present and
normalize it into the expected object (e.g. set adapter to the file adapter and
populate adapterOptions.folder/path from auditLogFolder), otherwise leave
existing {adapter, adapterOptions} untouched; add minimal validation and
unit-test-friendly defaults so current tests using the shorthand continue to
work.
| private getUserContext(auth: any): UserContext { | ||
| if (!auth) { | ||
| return { | ||
| userId: undefined, | ||
| sessionToken: undefined, | ||
| isMasterKey: false, | ||
| roles: [], | ||
| }; | ||
| } | ||
| return { | ||
| userId: auth.user?.id || auth.user?.objectId, | ||
| sessionToken: auth.sessionToken, | ||
| isMasterKey: auth.isMaster || auth.isMasterKey || false, | ||
| roles: auth.user?.get('roles') || [], | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify roles extraction works with all auth types.
Line 85 uses auth.user?.get('roles') which assumes a Parse Object with a get() method. Verify that all authentication flows provide user objects in this format, or handle cases where roles might be stored as a direct property.
Run the following script to check how roles are accessed across the codebase:
🏁 Script executed:
#!/bin/bash
# Search for role access patterns in auth/user objects
rg -nP --type=ts --type=js -C3 '\bauth\.user\b.*\broles\b|\buser\b.*\.roles\b'Length of output: 496
---
🏁 Script executed:
```shell
#!/bin/bash
# Check the type definition of auth parameter and imports in AuditLogController
head -30 src/Controllers/AuditLogController.ts
Length of output: 658
🏁 Script executed:
#!/bin/bash
# Search for Parse Object usage patterns (.get method calls)
rg -nP --type=ts --type=js '\.get\(' -C2 | head -50Length of output: 2540
🏁 Script executed:
#!/bin/bash
# Search for auth object type definitions or interfaces
rg -nP --type=ts 'interface.*[Aa]uth|type.*[Aa]uth' -C3Length of output: 366
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the full getUserContext method signature with context
rg -nP --type=ts 'private\s+getUserContext|getUserContext\s*\(' -A 20 src/Controllers/AuditLogController.ts
Length of output: 5420
🏁 Script executed:
#!/bin/bash
# Search for calls to getUserContext
rg -nP --type=ts 'getUserContext\(' src/Controllers/AuditLogController.ts -B 3 -A 1Length of output: 1645
🏁 Script executed:
#!/bin/bash
# Check for Parse imports and usage in AuditLogController
rg -nP --type=ts '^import|^from' src/Controllers/AuditLogController.ts | head -20Length of output: 73
🏁 Script executed:
#!/bin/bash
# Search for how auth object is used elsewhere in this file
rg -nP --type=ts '\bauth\.' src/Controllers/AuditLogController.ts -B 2 -A 2Length of output: 251
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for where auth object is created or populated in middleware/controllers
rg -nP --type=ts --type=js 'auth\s*[:=]|auth\s*\{' -B 2 -A 3 | head -80
Length of output: 3780
🏁 Script executed:
#!/bin/bash
# Search for test files that might show auth object structure
fd -e 'test.ts' -e 'spec.ts' -e 'test.js' -e 'spec.js' | xargs rg -l 'AuditLogController|getUserContext' 2>/dev/null || echo "No test files found"Length of output: 31
🏁 Script executed:
#!/bin/bash
# Check if Parse Objects allow both direct property access and .get() method
rg -nP --type=ts --type=js 'auth\.user\?\.(get|roles|id|objectId)' -C 1Length of output: 694
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the spec file to understand auth object structure
cat spec/AuditLogController.spec.js
Length of output: 9064
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for where roles are set on user objects
rg -nP --type=ts --type=js 'user.*roles\s*=|user\[.*roles|roles.*=' -B 2 -A 2 | head -60
Length of output: 2905
🏁 Script executed:
#!/bin/bash
# Search for Auth class/module to understand auth.user structure
rg -nP --type=ts --type=js 'class Auth|module.exports.*Auth|export.*Auth' -A 5 | head -40Length of output: 2319
🏁 Script executed:
#!/bin/bash
# Check if Parse is imported in RestWrite.js where auth is used
head -50 src/RestWrite.js | grep -E 'import|require|Parse'Length of output: 622
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for Auth class definition and getUserRoles method
fd -name 'Auth.js' -o -name 'Auth.ts' | xargs cat | head -150
Length of output: 230
🏁 Script executed:
#!/bin/bash
# Search for how getUserRoles is implemented
rg -nP --type=js 'getUserRoles\s*\(' -B 2 -A 10Length of output: 5331
🏁 Script executed:
#!/bin/bash
# Check if auth.user ever has a get() method or if it's always a plain object
rg -nP --type=js --type=ts 'this\.auth\.user\s*=' -B 2 -A 5 | head -80Length of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and examine Auth.js file
find . -name 'Auth.js' -type f
Length of output: 13
🏁 Script executed:
#!/bin/bash
# Search for Auth class definition more carefully
rg -nP --type=js 'class Auth|function Auth' -A 30 | head -100Length of output: 3563
🏁 Script executed:
#!/bin/bash
# Look for where auth.user is a Parse Object vs plain object
rg -nP --type=js 'auth\.user.*get\(' -B 5 -A 2Length of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the full Auth.js file to see getUserRoles implementation and how user is constructed
cat src/Auth.js
Length of output: 18201
🏁 Script executed:
#!/bin/bash
# Check how auth object is constructed in middleware/rest handlers
rg -nP --type=js 'new Auth\(|Auth\s*\(' -B 3 -A 8 | head -100Length of output: 5083
🏁 Script executed:
#!/bin/bash
# Verify if Parse Object's get() method is ever used in the context of auth.user
rg -nP --type=ts --type=js 'get\([\'"]roles[\'"]\)' -B 2 -A 1Length of output: 68
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for where roles attribute is set on user Parse Objects
rg -nP --type=js 'user\.set\(' -B 2 -A 2 | grep -A 3 -B 3 'roles'
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for any evidence of roles being stored on user objects
rg -nP --type=js 'set.*roles|roles.*set' -B 2 -A 2Length of output: 2920
🏁 Script executed:
#!/bin/bash
# Check what Parse Object properties are actually set on users in real usage
rg -nP --type=js 'fromJSON.*user|user.*fromJSON' -B 3 -A 3 | head -50Length of output: 2841
Fix roles extraction to use auth.getUserRoles() or make the method async.
Line 85 calls auth.user?.get('roles') which will always return undefined because roles are not stored as an attribute on user objects in Parse. Roles are loaded separately from the _Role table via auth.getUserRoles(). The current fallback || [] silently masks this failure, causing audit logs to always record empty role arrays.
Either:
- Make
getUserContextasync and callawait auth.getUserRoles(), or - Pass the roles separately to
getUserContext()from callers that have already loaded them
🤖 Prompt for AI Agents
In src/Controllers/AuditLogController.ts around lines 72 to 87, getUserContext
currently reads roles via auth.user?.get('roles') which is incorrect for Parse
(roles are loaded via auth.getUserRoles()) so audit logs end up with empty
arrays; change getUserContext to accept or fetch real roles: either make
getUserContext async and call const roles = await auth.getUserRoles() (and
return roles in the UserContext), or add a roles parameter and have callers pass
in roles they already loaded; also update all callers to await the new async
method or supply roles accordingly and remove the fallback
auth.user?.get('roles') || [].
| private maskSensitiveData(data: any): any { | ||
| if (!data || typeof data !== 'object') { | ||
| return data; | ||
| } | ||
|
|
||
| const masked = { ...data }; | ||
| const sensitiveFields = ['password', 'sessionToken', 'authData', '_hashed_password']; | ||
|
|
||
| sensitiveFields.forEach(field => { | ||
| if (masked[field]) { | ||
| masked[field] = '***masked***'; | ||
| } | ||
| }); | ||
|
|
||
| return masked; | ||
| } |
There was a problem hiding this comment.
Major: Shallow masking may leak nested sensitive data.
The current implementation only masks top-level fields. Nested sensitive data (e.g., authData.facebook.token, nested password fields) will not be masked, potentially violating GDPR requirements by logging sensitive information.
Consider implementing deep masking with recursive traversal:
private maskSensitiveData(data: any): any {
if (!data || typeof data !== 'object') {
return data;
}
if (Array.isArray(data)) {
return data.map(item => this.maskSensitiveData(item));
}
const masked = { ...data };
const sensitiveFields = ['password', 'sessionToken', 'authData', '_hashed_password', 'token'];
Object.keys(masked).forEach(key => {
if (sensitiveFields.includes(key)) {
masked[key] = '***masked***';
} else if (typeof masked[key] === 'object' && masked[key] !== null) {
// Recursively mask nested objects
masked[key] = this.maskSensitiveData(masked[key]);
}
});
return masked;
}🤖 Prompt for AI Agents
In src/Controllers/AuditLogController.ts around lines 92 to 107, the
maskSensitiveData function only masks top-level fields which can leak nested
sensitive values; change it to perform a deep recursive traversal: detect arrays
and map over them, iterate object keys, if a key is in the sensitiveFields list
replace its value with the mask, otherwise if the value is a non-null object or
array call maskSensitiveData recursively, and return a new cloned structure
(avoid mutating original input); also include common sensitive keys like "token"
and ensure null checks to prevent errors.
| export function getAuditLogController(options: ParseServerOptions): AuditLogController { | ||
| const { appId, auditLog } = options; | ||
|
|
||
| // If no audit log config, return a controller with no adapter (disabled) | ||
| if (!auditLog) { | ||
| return new AuditLogController(null, appId, {}); | ||
| } | ||
|
|
||
| // Extract configuration | ||
| const { adapter, logFilter, adapterOptions } = auditLog; | ||
|
|
||
| // Load the audit log adapter using loadAdapter pattern | ||
| // Defaults to WinstonFileAuditLogAdapter if no adapter specified | ||
| const auditLogAdapter = loadAdapter( | ||
| adapter, | ||
| WinstonFileAuditLogAdapter, | ||
| adapterOptions || {} | ||
| ); | ||
|
|
||
| // Create controller with adapter and filter configuration | ||
| return new AuditLogController(auditLogAdapter, appId, { | ||
| logFilter: logFilter || {}, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Support shorthand auditLog config and keep adapter defaulting.
Accept auditLog as either full AuditLogOptions or WinstonFileAuditLogAdapterOptions to match tests/docs.
-export function getAuditLogController(options: ParseServerOptions): AuditLogController {
+export function getAuditLogController(options: ParseServerOptions): AuditLogController {
const { appId, auditLog } = options;
// If no audit log config, return a controller with no adapter (disabled)
if (!auditLog) {
return new AuditLogController(null, appId, {});
}
- // Extract configuration
- const { adapter, logFilter, adapterOptions } = auditLog;
+ // Support both full and shorthand shapes:
+ // - Full: { adapter, adapterOptions, logFilter }
+ // - Shorthand: { auditLogFolder, datePattern, maxSize, maxFiles }
+ const hasShorthand =
+ auditLog &&
+ typeof auditLog === 'object' &&
+ !('adapter' in auditLog) &&
+ ('auditLogFolder' in auditLog || 'datePattern' in auditLog || 'maxFiles' in auditLog || 'maxSize' in auditLog);
+ const adapter = hasShorthand ? undefined : auditLog.adapter;
+ const adapterOptions = hasShorthand ? auditLog : auditLog.adapterOptions || {};
+ const logFilter = hasShorthand ? undefined : auditLog.logFilter;
// Load the audit log adapter using loadAdapter pattern
// Defaults to WinstonFileAuditLogAdapter if no adapter specified
const auditLogAdapter = loadAdapter(
adapter,
WinstonFileAuditLogAdapter,
- adapterOptions || {}
+ adapterOptions
);
// Create controller with adapter and filter configuration
return new AuditLogController(auditLogAdapter, appId, {
logFilter: logFilter || {},
});
}🧰 Tools
🪛 Biome (2.1.2)
[error] 84-84: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 84-84: return types can only be used in TypeScript files
(parse)
| excludeRoles: ?(string[]); | ||
| /* Custom filter function for advanced filtering. | ||
| <br><br> | ||
| Function receives an audit event object and returns true to log or false to skip. | ||
| <br><br> | ||
| Example: (event) => event.userId !== 'system' */ | ||
| filter: any; | ||
| } |
There was a problem hiding this comment.
Make AuditLogFilterOptions.filter optional.
Docs show filter is optional; Definitions currently require it. Align by marking as optional.
- filter: any;
+ filter: ?any;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| excludeRoles: ?(string[]); | |
| /* Custom filter function for advanced filtering. | |
| <br><br> | |
| Function receives an audit event object and returns true to log or false to skip. | |
| <br><br> | |
| Example: (event) => event.userId !== 'system' */ | |
| filter: any; | |
| } | |
| excludeRoles: ?(string[]); | |
| /* Custom filter function for advanced filtering. | |
| <br><br> | |
| Function receives an audit event object and returns true to log or false to skip. | |
| <br><br> | |
| Example: (event) => event.userId !== 'system' */ | |
| filter: ?any; | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 567-567: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
🤖 Prompt for AI Agents
In src/Options/index.js around lines 566 to 573, the type definition for
AuditLogFilterOptions currently requires the filter property but the docs state
it is optional; update the definition to mark filter as optional (e.g., add a ?
or make its type include undefined) so callers can omit it, and ensure any
related exports/types are updated consistently.
| // Audit log successful delete | ||
| try { | ||
| config.auditLogController?.logDataDelete({ | ||
| auth, | ||
| req: { config }, | ||
| className, | ||
| objectId, | ||
| success: true, | ||
| }); | ||
| } catch (error) { | ||
| config.loggerController.error('Audit logging error in rest.del', { error }); | ||
| } |
There was a problem hiding this comment.
Pass a real request context (or omit) instead of req: { config }.
AuditLogController expects a request (for IP). { config } is not a request and adds no value.
- config.auditLogController?.logDataDelete({
- auth,
- req: { config },
+ config.auditLogController?.logDataDelete({
+ auth,
+ req: context?.req, // if available; otherwise omit this field
className,
objectId,
success: true,
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/rest.js around lines 144 to 155, the audit log call is passing req: {
config } which is not a real request object; replace that with the actual
request context (e.g., pass the existing req variable so the AuditLogController
can read IP/user info) or remove the req property entirely if no request is
available; update the call signature accordingly and keep the try/catch logging
behavior unchanged.
4 participants
Pull Request
Issue
Closes: #5378
Approach
Tasks
Summary by CodeRabbit
Release Notes