feat: mostly just duplicate audit logs and other clickhouse stuff#2134
feat: mostly just duplicate audit logs and other clickhouse stuff#2134
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request includes several changes across multiple files, primarily focusing on the removal of a database migration task, updates to naming conventions for ClickHouse tables, and the introduction of new SQL tables and materialized views for telemetry and key verification data. Additionally, modifications were made to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 57
🧹 Outside diff range and nitpick comments (37)
apps/agent/pkg/clickhouse/schema/004_create_key_verifications_per_day.sql (1)
1-14: LGTM! Well-structured table for key verifications.The table structure is well-designed for storing and aggregating key verification data. Good job on using appropriate column types, the AggregatingMergeTree engine, and a clear ordering strategy.
Consider these potential improvements:
For higher precision timestamps, you might want to use
DateTime64:time DateTime64(3)If you expect high data volume or specific query patterns, consider adding a PARTITION BY clause for better performance:
ENGINE = AggregatingMergeTree() PARTITION BY toYYYYMM(time) ORDER BY (workspace_id, key_space_id, time, identity_id, key_id)Add a comment to the table for better documentation:
COMMENT 'Stores daily aggregated key verification data'apps/agent/pkg/clickhouse/schema/003_create_raw_telemetry_sdks_table.sql (3)
2-2: Table naming convention looks good, consider schema organization.The table name
raw_telemetry_sdks_v1follows a clear and informative naming convention. The_v1suffix allows for future versioning, which is a good practice.Consider creating a separate schema for telemetry data (e.g.,
telemetry) instead of using thedefaultschema. This can help with organization as the project grows.
3-15: Column definitions look good, consider clarifying thetimecolumn comment.The column definitions and data types are well-chosen for their intended purposes. The use of
Int64for thetimecolumn andArray(String)for theversionscolumn are particularly good choices.Consider updating the comment for the
timecolumn to be more specific:- -- unix milli + -- timestamp in Unix milliseconds (UTC) time Int64,This clarifies that the timestamp is in UTC, which is important for data consistency.
17-18: Good engine choice and ordering, consider future partitioning.The MergeTree engine is an excellent choice for telemetry data, and ordering by
request_idandtimealigns well with typical query patterns.As the dataset grows, consider adding partitioning to improve query performance and data management. For example:
ENGINE = MergeTree() PARTITION BY toYYYYMM(fromUnixTimestamp64Milli(time)) ORDER BY (request_id, time)This partitions the data by month, which can significantly improve query performance for time-based queries and make data retention policies easier to implement.
apps/agent/pkg/clickhouse/schema/005_create_mv_key_verifications_per_day.sql (2)
1-2: LGTM! Consider adding a corresponding down migration.The creation statement for the materialized view looks good. The use of versioning in the view name is a good practice for managing schema changes.
Consider adding a corresponding
-- +goose downmigration to drop the materialized view. This would make it easier to roll back changes if needed:-- +goose down DROP VIEW IF EXISTS default.mv_key_verifications_per_day_v1;
3-10: LGTM! Consider adding comments for clarity.The SELECT clause looks good. The use of
countState()for aggregation and the transformation of thetimefield for daily grouping are appropriate.Consider adding comments to explain the purpose of the
countState()function and the time transformation. This can help other developers understand the logic:SELECT workspace_id, key_space_id, identity_id, key_id, outcome, -- Use countState() for incremental updates in the materialized view countState() as count, -- Group by day by transforming Unix timestamp to start of day toStartOfDay(fromUnixTimestamp64Milli(time)) AS timeinternal/db/src/schema/index.ts (1)
16-16: LGTM! Consider specific exports if needed.The new export statement for "audit_logs" is consistent with the existing pattern in the file. It successfully makes all exports from the audit_logs module available.
While the wildcard export is consistent with the file's pattern, consider using specific exports if you want more control over what's exposed from the audit_logs module. This can help with better tracking of imports and potentially reduce unintended exposures. For example:
export { AuditLog, createAuditLog /* other specific exports */ } from "./audit_logs";This approach allows for more granular control over the public API of your schema module.
apps/api/src/pkg/cache/namespaces.ts (1)
67-70: LGTM! Consider adding a type for the compound key.The addition of
auditLogBucketByWorkspaceIdAndNameto theCacheNamespacestype is well-structured and consistent with the existing code. The comment explaining the compound key is helpful.To improve type safety and readability, consider defining a separate type for the compound key:
type AuditLogBucketKey = [workspaceId: string, name: string]; // Then use it in CacheNamespaces auditLogBucketByWorkspaceIdAndName: Record<AuditLogBucketKey, { id: string }>;This change would make the structure of the key more explicit and provide better type checking when using this cache namespace.
apps/api/src/pkg/db.ts (1)
13-16: LGTM: Improved type safety for transactionsThe new
Transactiontype alias enhances type safety for database operations by combiningPlanetScaleTransactionwith the schema and its relations. This is a positive change that will provide better type inference when working with transactions.Consider adding a brief JSDoc comment above the
Transactiontype alias to explain its purpose and usage. For example:/** * Represents a type-safe transaction for our database schema. * This type combines PlanetScale's transaction type with our specific schema and its relations. */ export type Transaction = PlanetScaleTransaction< typeof schema, ExtractTablesWithRelations<typeof schema> >;internal/schema/src/auditlog.ts (1)
69-69: LGTM! Consider adding a comment to explain the default behavior.The addition of a default value for the
timeproperty is a good improvement. It ensures that every audit log entry will have a timestamp, even if not explicitly provided, which simplifies the process of creating audit log entries.However, be aware that this automatic timestamping could potentially lead to inconsistencies if not used carefully. For instance, if there's a delay between an event occurring and it being logged, the timestamp might not accurately reflect the event time.
Consider adding a comment above this line to explain the default behavior:
/** * The time of the audit log entry. Defaults to the current timestamp if not provided. */ time: z.number().default(() => Date.now()),This will help other developers understand the automatic timestamping behavior at a glance.
tools/local/src/db.ts (2)
62-70: LGTM: Audit log bucket insertion implemented correctly.The changes effectively replace the webhook key space insertion with an audit log bucket insertion. The use of
newIdfor generating unique IDs and enablingdeleteProtectionare good practices.However, consider adding error handling to this database operation to gracefully handle potential insertion failures.
Consider wrapping the insertion in a try-catch block:
try { await db .insert(schema.auditLogBucket) .values({ id: newId("auditLogBucket"), workspaceId: ROW_IDS.rootWorkspace, name: "unkey_mutations", deleteProtection: true, }) .onDuplicateKeyUpdate({ set: { createdAt: Date.now() } }); s.message("Created audit log bucket"); } catch (error) { s.message(`Failed to create audit log bucket: ${error.message}`); // Consider whether to throw the error or handle it differently }
Line range hint
8-15: Remove unused webhook-related code.With the removal of webhook key space and API setup, there are still some remnants of webhook-related code that should be cleaned up:
In the
ROW_IDSobject, remove the following entries:
webhookKeySpacewebhookApiIn the function's return type and value, remove the
webhooksApiproperty.These changes will ensure consistency and prevent potential confusion or errors in the future.
Apply the following changes:
const ROW_IDS = { rootWorkspace: "ws_local_root", rootKeySpace: "ks_local_root_keys", rootApi: "api_local_root_keys", - webhookKeySpace: "ks_local_webhook_keys", - webhookApi: "api_local_webhook_keys", }; export async function prepareDatabase(url?: string): Promise<{ workspace: { id: string }; api: { id: string }; - webhooksApi: { id: string }; }> { // ... (rest of the function) return { workspace: { id: ROW_IDS.rootWorkspace, }, api: { id: ROW_IDS.rootApi }, - webhooksApi: { id: ROW_IDS.webhookApi }, }; }Also applies to: 101-105
apps/api/src/pkg/middleware/metrics.ts (2)
65-80: New SDK telemetry insertion logic looks good, with a minor suggestion.The addition of SDK telemetry insertion logic enhances data collection capabilities. The use of
waitUntil()and proper error handling are commendable practices.Consider simplifying the event object spread in the
insertSdkTelemetrycall:analytics .insertSdkTelemetry({ ...event, - request_id: event.requestId, })The
requestIdproperty is already present in theeventobject, so explicitly settingrequest_idis redundant.
Line range hint
56-80: Consider combining telemetry operations for improved efficiency.While the additions to SDK telemetry handling are valuable, having two separate telemetry operations (ingestion and insertion) might increase the system load unnecessarily.
Consider refactoring the telemetry handling to combine both operations into a single method call. This could potentially reduce the number of database operations and simplify the code. For example:
c.executionCtx.waitUntil( analytics.handleSdkTelemetry(event).catch((err) => { logger.error("Error handling SDK telemetry", { method: c.req.method, path: c.req.path, error: err.message, telemetry, event, }); }) );This approach would require modifying the
analyticsservice to handle both ingestion and insertion in a single method, potentially improving efficiency and maintainability.apps/api/src/pkg/key_migration/handler.ts (1)
Line range hint
187-207: LGTM! Consider refactoring to reduce duplication.The change from
ingestUnkeyAuditLogstoingestUnkeyAuditLogsTinybirdis consistent with the previous updates. The function now logs multiple audit entries for permission connections, which is appropriate for this context.Consider refactoring the audit logging for roles and permissions to reduce code duplication. You could create a helper function like this:
function logConnections( analytics: Analytics, connections: Array<{ id: string, type: 'role' | 'permission' }>, keyId: string, workspaceId: string, rootKeyId: string, auditLogContext: any ) { return analytics.ingestUnkeyAuditLogsTinybird( connections.map((connection) => ({ workspaceId, actor: { type: "key", id: rootKeyId }, event: `authorization.connect_${connection.type}_and_key`, description: `Connected ${connection.id} and ${keyId}`, resources: [ { type: "key", id: keyId }, { type: connection.type, id: connection.id }, ], context: auditLogContext, })) ); }Then use it like this:
await logConnections( analytics, Object.values(roles).map(roleId => ({ id: roleId, type: 'role' as const })), keyId, message.workspaceId, message.rootKeyId, message.auditLogContext ); // ... and similarly for permissionsThis would make the code more DRY and easier to maintain.
apps/api/src/pkg/analytics.ts (4)
44-55: LGTM! Consider adding a comment for clarity.The new
insertSdkTelemetrymethod is well-structured and uses an appropriate schema for SDK telemetry data. The use of ClickHouse for this purpose is suitable for handling high-volume, time-series data.Consider adding a brief comment explaining the purpose of this method and the structure of the data being inserted. This would enhance code readability and maintainability.
+ /** + * Inserts SDK telemetry data into ClickHouse. + * Data includes request ID, timestamp, runtime, platform, and SDK versions. + */ public get insertSdkTelemetry() { return this.clickhouse.insert({ table: "default.raw_telemetry_sdks_v1", schema: z.object({ request_id: z.string(), time: z.number().int(), runtime: z.string(), platform: z.string(), versions: z.array(z.string()), }), }); }
70-71: Approved. Consider enhancing the comment for clarity.The renaming of
ingestUnkeyAuditLogstoingestUnkeyAuditLogsTinybirdis consistent with the apparent shift towards Tinybird-specific naming conventions. The functionality remains unchanged, which is good for maintaining consistency.The current comment "//tinybird" is quite brief. Consider expanding it to provide more context about the method's purpose and its relationship to Tinybird. For example:
-//tinybird +// Ingests Unkey audit logs into Tinybird data storage +// This method is specific to Tinybird integration and handles audit log events public ingestUnkeyAuditLogsTinybird(logs: MaybeArray<UnkeyAuditLog>) {This would improve code readability and make the purpose of the method clearer to other developers.
95-96: Approved. Consider enhancing the comment for clarity.The renaming of
ingestGenericAuditLogstoingestGenericAuditLogsTinybirdis consistent with the apparent shift towards Tinybird-specific naming conventions. The functionality remains unchanged, which is good for maintaining consistency.As with the previous method, the current comment "//tinybird" is quite brief. Consider expanding it to provide more context about the method's purpose and its relationship to Tinybird. For example:
-//tinybird +// Ingests generic audit logs into Tinybird data storage +// This method is specific to Tinybird integration and handles various audit log events public get ingestGenericAuditLogsTinybird() {This would improve code readability and make the purpose of the method clearer to other developers.
Line range hint
1-238: Overall assessment: Good changes with minor improvements needed.The changes in this file appear to be part of a larger refactoring effort to integrate with Tinybird and improve telemetry data handling. The code quality is generally good, with clear and consistent naming conventions applied.
Key points:
- The new
insertSdkTelemetrymethod is well-structured and appropriate for its purpose.- The renaming of methods to include "Tinybird" in their names is consistent and clear.
- Comments could be improved to provide more context, especially for the Tinybird-related methods.
- The deprecated
ingestKeyVerificationmethod should be properly marked and scheduled for removal.Consider creating a migration plan to fully transition from the old methods to the new Tinybird and ClickHouse implementations. This could include:
- Updating all call sites to use the new methods.
- Creating a deprecation schedule for the old methods.
- Ensuring that the new data storage solutions (Tinybird and ClickHouse) are properly set up and optimized for your use case.
These steps will help ensure a smooth transition and maintain code quality as you evolve your analytics infrastructure.
apps/api/src/routes/v1_identities_updateIdentity.ts (2)
388-389: LGTM with suggestions: Updated audit log handling.The changes improve the audit log handling by separating the insertion and ingestion processes. The use of
c.executionCtx.waitUntilfor the Tinybird ingestion is a good practice for non-blocking operations.However, I have two suggestions:
- Could you clarify the purpose of the
undefinedparameter ininsertUnkeyAuditLog? It might be helpful to add a comment explaining its significance or consider using a named parameter for better readability.- Consider adding a comment explaining that the Tinybird ingestion is intentionally non-blocking:
// Ingest audit logs to Tinybird asynchronously (non-blocking) c.executionCtx.waitUntil(analytics.ingestUnkeyAuditLogsTinybird(auditLogs));
Line range hint
1-400: Summary: Improved audit log handling.The changes in this file focus on enhancing the audit log handling process. The introduction of
insertUnkeyAuditLogand the separation of log insertion and ingestion improve the overall audit logging capabilities. These modifications are minimal and don't alter the core functionality of the identity update route.To further improve the code:
- Consider adding documentation for the
insertUnkeyAuditLogfunction, explaining its parameters and purpose.- Evaluate if any existing tests need to be updated to reflect these changes in audit log handling.
tools/migrate/auditlog-import.ts (1)
86-97: Handle emptyresourcesarray to avoid unnecessary database insertion.If
resourcesis empty, the script attempts to insert an empty array into the database, which may not be necessary and could lead to errors. Check ifresourcesis non-empty before attempting the insertion.Apply this diff to add the check:
- await db.insert(schema.auditLogTarget).values( - resources.map((r) => ({ - workspaceId: log.workspaceId, - bucketId, - auditLogId, - displayName: r.name ?? "", - type: r.type, - id: r.id, - name: r.name, - meta: r.meta, - })), - ); + if (resources.length > 0) { + await db.insert(schema.auditLogTarget).values( + resources.map((r) => ({ + workspaceId: log.workspaceId, + bucketId, + auditLogId, + displayName: r.name ?? "", + type: r.type, + id: r.id, + name: r.name, + meta: r.meta, + })), + ); + }internal/db/src/schema/audit_logs.ts (1)
100-100: Consider renaming the 'id' field to 'targetId' inauditLogTargetThe
idfield represents the target's ID, which might be clearer if namedtargetIdto avoid confusion with otheridfields.Apply this diff:
- id: varchar("id", { length: 256 }).notNull(), + targetId: varchar("target_id", { length: 256 }).notNull(),Remember to update all references to this field in related code and database queries.
apps/api/src/routes/v1_permissions_createPermission.ts (1)
Line range hint
121-137: Ensure proper error handling for asynchronous analytics ingestion.The call to
analytics.ingestUnkeyAuditLogsTinybirdis executed asynchronously usingc.executionCtx.waitUntil(). While this allows the main request to complete without waiting for the analytics process, any errors during ingestion might go unnoticed.Consider implementing error logging or monitoring to capture and handle potential failures in the analytics ingestion process.
apps/api/src/routes/v1_keys_deleteKey.ts (1)
Line range hint
147-160: Ensure consistency between audit logs and analytics dataThe call to
analytics.ingestUnkeyAuditLogsTinybirdis placed outside the transaction. If the transaction commits but the analytics ingestion fails, or vice versa, this could lead to inconsistencies between the audit logs and analytics data. Consider the following options:
- Option 1: Move the analytics ingestion inside the transaction to ensure both operations succeed or fail together.
- Option 2: Implement retry mechanisms or error handling for the analytics ingestion to handle failures gracefully.
Apply this diff if you choose to move the analytics ingestion into the transaction:
await db.primary.transaction(async (tx) => { await tx.update(schema.keys).set({ deletedAt: new Date() }).where(eq(schema.keys.id, key.id)); try { await insertUnkeyAuditLog(c, tx, { // ... audit log data ... }); + await analytics.ingestUnkeyAuditLogsTinybird({ + // ... analytics data ... + }); } catch (error) { // Handle errors appropriately console.error('Failed to process audit log or analytics ingestion:', error); } });apps/api/src/routes/v1_identities_deleteIdentity.ts (1)
114-118: Simplify code by removing unnecessaryas constassertionsThe
as constassertions on string literals like"ratelimit.delete","key","identity", and"ratelimit"may be unnecessary if TypeScript can infer the literal types automatically. Removing these assertions can make the code cleaner and more readable.Also applies to: 121-129
apps/api/src/routes/v1_keys_removeRoles.ts (2)
143-143: Typo in the description fieldThere is a typo in the word "Disconnected" in the
descriptionfield. It is currently spelled as "Disonnected".Apply this diff to fix the typo:
- description: `Disonnected ${r.roleId} and ${req.keyId}`, + description: `Disconnected ${r.roleId} and ${req.keyId}`,Also applies to: 173-173
Line range hint
98-112: Correct the filtering logic indeleteRolesThe current filtering logic in
deleteRolesmay not work as intended. Thefiltercallback function should return a boolean, but since it usesreturninside aforloop, it exits after the first iteration, potentially missing other matches.Here's the corrected code using
someto properly check alldeleteRequestitems:- const deleteRoles = connectedRoles.filter((cr) => { - for (const deleteRequest of req.roles) { - if ("id" in deleteRequest) { - return cr.roleId === deleteRequest.id; - } - if ("name" in deleteRequest) { - return cr.role.name === deleteRequest.name; - } - } - }); + const deleteRoles = connectedRoles.filter((cr) => + req.roles.some((deleteRequest) => { + if ("id" in deleteRequest) { + return cr.roleId === deleteRequest.id; + } + if ("name" in deleteRequest) { + return cr.role.name === deleteRequest.name; + } + return false; + }) + );This ensures that
deleteRolesincludes all roles that match any of thereq.rolescriteria.apps/api/src/routes/v1_keys_removePermissions.ts (1)
144-144: Improve clarity of the 'description' field in audit logsConsider rephrasing the
descriptionto more clearly convey the action taken. This enhances readability and comprehension of the audit logs.Suggested change:
- description: `Disconnected ${r.permissionId} and ${req.keyId}`, + description: `Disconnected permission '${r.permissionId}' from key '${req.keyId}'`,Also applies to: 174-174
apps/api/src/routes/v1_apis_deleteApi.ts (1)
100-116: Ensure proper error handling in audit loggingWhen calling
analytics.ingestUnkeyAuditLogsTinybirdandinsertUnkeyAuditLog, consider adding error handling to catch and log any exceptions that may occur during the audit logging process. This ensures that failures in logging do not interrupt the main transaction flow but are still recorded for troubleshooting.Consider wrapping the audit logging calls in try-catch blocks:
try { await analytics.ingestUnkeyAuditLogsTinybird({ /* parameters */ }); } catch (error) { // Handle or log the error appropriately } try { await insertUnkeyAuditLog(c, tx, { /* parameters */ }); } catch (error) { // Handle or log the error appropriately }apps/api/src/routes/v1_keys_updateRemaining.ts (1)
Line range hint
199-213: Synchronize Logging with Shared DataAfter refactoring the logging data into a shared variable, you can use it here to avoid duplication.
Update the code as follows:
c.executionCtx.waitUntil( analytics.ingestUnkeyAuditLogsTinybird(logData), );This ensures consistency between the two logging mechanisms and simplifies future updates.
apps/api/src/routes/v1_identities_createIdentity.ts (2)
Line range hint
159-201: Refactor to eliminate duplication in audit log creationThe audit log events in lines 159–201 and 203–247 are nearly identical. Refactoring to create a shared
auditLogsvariable reduces code duplication and improves maintainability.Refactored code:
const auditLogs = [ { workspaceId: authorizedWorkspaceId, event: "identity.create", actor: { type: "key", id: rootKeyId, }, description: `Created ${identity.id}`, resources: [ { type: "identity", id: identity.id, }, ], context: { location: c.get("location"), userAgent: c.get("userAgent"), }, }, ...ratelimits.map((r) => ({ workspaceId: authorizedWorkspaceId, event: "ratelimit.create" as const, actor: { type: "key" as const, id: rootKeyId, }, description: `Created ${r.id}`, resources: [ { type: "identity" as const, id: identity.id, }, { type: "ratelimit" as const, id: r.id, }, ], context: { location: c.get("location"), userAgent: c.get("userAgent"), }, })), ]; // Insert audit logs into the database await insertUnkeyAuditLog(c, tx, auditLogs); // Ingest audit logs into Tinybird analytics c.executionCtx.waitUntil( analytics.ingestUnkeyAuditLogsTinybird(auditLogs), );Also applies to: 203-247
Line range hint
159-201: Optimize context retrieval by storing in variablesTo improve readability and performance, store
c.get("location")andc.get("userAgent")in variables since they're used multiple times.Apply this change:
const context = { location: c.get("location"), userAgent: c.get("userAgent"), }; // Use `context` in audit logs await insertUnkeyAuditLog(c, tx, [ { // ...other properties context, }, // ...additional audit logs ]);Apply the same change in the analytics ingestion code.
apps/api/src/routes/v1_keys_addRoles.ts (1)
225-225: Enhance audit log descriptions for clarityConsider making the
descriptionfield more informative by including both the role names and IDs to improve audit log readability.Suggested changes:
For
createRolesaudit logs:- description: `Created ${r.id}`, + description: `Created role ${r.name} (ID: ${r.id})`,For
addRolesaudit logs:- description: `Connected ${r.id} and ${req.keyId}`, + description: `Connected role ${r.name} (ID: ${r.id}) to key ${req.keyId}`,Also applies to: 248-248
apps/api/src/routes/v1_migrations_createKey.ts (3)
Line range hint
500-507: Refactor duplicated audit log insertion codeThe code for inserting audit logs using
insertUnkeyAuditLogis duplicated in lines 500-507 and 556-580. This duplication can make the code harder to maintain and increases the risk of inconsistencies. Consider refactoring this repetitive code into a reusable function to adhere to the DRY (Don't Repeat Yourself) principle.Here's how you might refactor the duplicated code:
+ // Create a reusable function for inserting audit logs + const insertAuditLogs = async ( + c, + tx, + items, + event, + actorId, + descriptionFn, + resourcesFn + ) => { + await insertUnkeyAuditLog( + c, + tx, + items.map((item) => ({ + workspaceId: authorizedWorkspaceId, + event, + actor: { + type: "key", + id: actorId, + }, + description: descriptionFn(item), + resources: resourcesFn(item), + context: { + location: c.get("location"), + userAgent: c.get("userAgent"), + }, + })) + ); + }; - // Original code block at lines 500-507 - await insertUnkeyAuditLog( - c, - tx, - keys.map((k) => ({ - workspaceId: authorizedWorkspaceId, - event: "key.create", - actor: { - type: "key", - id: auth.key.id, - }, - description: `Created ${k.id} in ${k.keyAuthId}`, - resources: [ - { type: "key", id: k.id }, - { type: "keyAuth", id: k.keyAuthId! }, - ], - context: { - location: c.get("location"), - userAgent: c.get("userAgent"), - }, - })) - ); + // Refactored code for key creation audit logs + await insertAuditLogs( + c, + tx, + keys, + "key.create", + auth.key.id, + (k) => `Created ${k.id} in ${k.keyAuthId}`, + (k) => [ + { type: "key", id: k.id }, + { type: "keyAuth", id: k.keyAuthId! }, + ] + ); - // Original code block at lines 556-580 - await insertUnkeyAuditLog( - c, - tx, - roleConnections.map((rc) => ({ - workspaceId: authorizedWorkspaceId, - actor: { type: "key", id: rootKeyId }, - event: "authorization.connect_role_and_key", - description: `Connected ${rc.roleId} and ${rc.keyId}`, - resources: [ - { type: "key", id: rc.keyId }, - { type: "role", id: rc.roleId }, - ], - context: { - location: c.get("location"), - userAgent: c.get("userAgent"), - }, - })) - ); + // Refactored code for role and key connection audit logs + await insertAuditLogs( + c, + tx, + roleConnections, + "authorization.connect_role_and_key", + rootKeyId, + (rc) => `Connected ${rc.roleId} and ${rc.keyId}`, + (rc) => [ + { type: "key", id: rc.keyId }, + { type: "role", id: rc.roleId }, + ] + );Also applies to: 556-580
Line range hint
500-507: Consider combining audit log insertion and analytics ingestionAfter inserting audit logs using
insertUnkeyAuditLog, the same data is used for analytics ingestion withanalytics.ingestUnkeyAuditLogsTinybird. To streamline the process and reduce redundancy, consider enhancing theinsertUnkeyAuditLogfunction to handle analytics ingestion internally. This consolidation can simplify the code and ensure consistency between logging and analytics data.Also applies to: 529-555, 556-580, 580-593
Line range hint
250-274: Deprecation warnings forratelimitpropertiesIn the request schema definition, several properties within the
ratelimitobject are marked as deprecated (e.g.,type,refillRate,refillInterval). While it's important to maintain backward compatibility, consider guiding users towards the preferred properties and updating the documentation accordingly.You might update the OpenAPI specification to emphasize the new properties and suggest alternatives to the deprecated ones.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
🔇 Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (46)
- apps/agent/Taskfile.yml (0 hunks)
- apps/agent/pkg/clickhouse/schema/000_README.md (2 hunks)
- apps/agent/pkg/clickhouse/schema/003_create_raw_telemetry_sdks_table.sql (1 hunks)
- apps/agent/pkg/clickhouse/schema/004_create_key_verifications_per_day.sql (1 hunks)
- apps/agent/pkg/clickhouse/schema/005_create_mv_key_verifications_per_day.sql (1 hunks)
- apps/api/src/pkg/analytics.ts (5 hunks)
- apps/api/src/pkg/audit.ts (1 hunks)
- apps/api/src/pkg/cache/index.ts (1 hunks)
- apps/api/src/pkg/cache/namespaces.ts (1 hunks)
- apps/api/src/pkg/db.ts (1 hunks)
- apps/api/src/pkg/key_migration/handler.ts (3 hunks)
- apps/api/src/pkg/middleware/metrics.ts (2 hunks)
- apps/api/src/routes/legacy_keys_createKey.ts (2 hunks)
- apps/api/src/routes/v1_apis_createApi.ts (2 hunks)
- apps/api/src/routes/v1_apis_deleteApi.ts (3 hunks)
- apps/api/src/routes/v1_identities_createIdentity.ts (3 hunks)
- apps/api/src/routes/v1_identities_deleteIdentity.ts (2 hunks)
- apps/api/src/routes/v1_identities_updateIdentity.ts (2 hunks)
- apps/api/src/routes/v1_keys_addPermissions.ts (2 hunks)
- apps/api/src/routes/v1_keys_addRoles.ts (2 hunks)
- apps/api/src/routes/v1_keys_createKey.ts (2 hunks)
- apps/api/src/routes/v1_keys_deleteKey.ts (2 hunks)
- apps/api/src/routes/v1_keys_removePermissions.ts (2 hunks)
- apps/api/src/routes/v1_keys_removeRoles.ts (2 hunks)
- apps/api/src/routes/v1_keys_setPermissions.ts (2 hunks)
- apps/api/src/routes/v1_keys_setRoles.ts (2 hunks)
- apps/api/src/routes/v1_keys_updateKey.ts (2 hunks)
- apps/api/src/routes/v1_keys_updateRemaining.ts (2 hunks)
- apps/api/src/routes/v1_migrations_createKey.ts (3 hunks)
- apps/api/src/routes/v1_permissions_createPermission.ts (2 hunks)
- apps/api/src/routes/v1_permissions_createRole.ts (2 hunks)
- apps/api/src/routes/v1_permissions_deletePermission.ts (2 hunks)
- apps/api/src/routes/v1_permissions_deleteRole.ts (2 hunks)
- apps/api/src/routes/v1_ratelimit_limit.ts (4 hunks)
- apps/dashboard/lib/trpc/routers/workspace/create.ts (1 hunks)
- deployment/docker-compose.yaml (0 hunks)
- internal/db/package.json (1 hunks)
- internal/db/src/index.ts (1 hunks)
- internal/db/src/schema/audit_logs.ts (1 hunks)
- internal/db/src/schema/index.ts (1 hunks)
- internal/db/src/schema/workspaces.ts (2 hunks)
- internal/id/src/generate.ts (1 hunks)
- internal/schema/src/auditlog.ts (1 hunks)
- tools/local/src/db.ts (2 hunks)
- tools/migrate/auditlog-import.ts (1 hunks)
- tools/migrate/tinybird-export.ts (1 hunks)
💤 Files not reviewed due to no reviewable changes (2)
- apps/agent/Taskfile.yml
- deployment/docker-compose.yaml
🔇 Additional comments not posted (60)
internal/db/src/index.ts (1)
8-8: LGTM: Addition of PlanetScaleTransaction exportThe addition of
type PlanetScaleTransactionto the exports is consistent with the module's purpose of providing database-related types and functions. This new export will allow consumers of this module to use thePlanetScaleTransactiontype, which can be beneficial for type-checking and improved developer experience when working with PlanetScale transactions.The export syntax is correct, and it's added in the appropriate place alongside the existing
PlanetScaleDatabasetype.To ensure this change doesn't introduce any unintended side effects, let's verify its usage across the codebase:
This script will help us understand if there are any existing uses of
PlanetScaleTransactionthat might be affected by this change, and if there are any places where this new type could be beneficial.✅ Verification successful
Verification Successful: PlanetScaleTransaction Export Addition
The addition of
type PlanetScaleTransactionhas been successfully verified. It is consistently used inapps/api/src/pkg/db.tsandinternal/db/src/index.ts, aligning with the module's purpose. No conflicts or unintended side effects were found as a result of this addition.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for existing usage of PlanetScaleTransaction and potential impact # Search for existing usage of PlanetScaleTransaction echo "Searching for existing usage of PlanetScaleTransaction:" rg --type typescript "PlanetScaleTransaction" # Search for potential places where this new type might be useful echo "Searching for potential usage of PlanetScaleTransaction:" rg --type typescript "transaction.*planetscale"Length of output: 409
Script:
#!/bin/bash # Description: Check for existing usage of PlanetScaleTransaction and potential impact # Search for existing usage of PlanetScaleTransaction in TypeScript files echo "Searching for existing usage of PlanetScaleTransaction:" rg --type ts "PlanetScaleTransaction" rg --type tsx "PlanetScaleTransaction" # Search for potential places where this new type might be useful in TypeScript files echo "Searching for potential usage of PlanetScaleTransaction:" rg --type ts "transaction.*planetscale" rg --type tsx "transaction.*planetscale"Length of output: 644
apps/agent/pkg/clickhouse/schema/005_create_mv_key_verifications_per_day.sql (3)
11-11: LGTM! Source table is correctly specified.The FROM clause correctly specifies the source table
default.raw_key_verifications_v1. The consistent use of thedefaultschema and versioning convention is good.
12-18: LGTM! GROUP BY clause is correctly structured.The GROUP BY clause correctly includes all non-aggregated fields from the SELECT clause, including the transformed
timefield. This ensures proper grouping of the data on a daily basis.
1-19: Overall, excellent implementation of the materialized view.This SQL file creates a well-structured materialized view for daily key verification aggregations. It follows ClickHouse best practices for materialized views, including the use of
countState()for incremental updates and proper time transformations for daily grouping. The consistent naming conventions and use of versioning are commendable.A few minor suggestions for improvement have been made in previous comments, primarily around adding comments for clarity and considering a down migration. These are not critical but could enhance maintainability.
Great job on this implementation!
internal/db/package.json (1)
26-26: LGTM! Verify usage of the new dependency.The addition of
"@unkey/id": "workspace:^"as a dependency is appropriate and follows the existing pattern for workspace dependencies. This change suggests that@unkey/dbnow requires functionality from@unkey/id.To ensure this dependency is being used correctly, please run the following script:
This script will help confirm that the new dependency is actually being used within the
@unkey/dbpackage.✅ Verification successful
LGTM! The
@unkey/iddependency is being used in./internal/db/src/schema/audit_logs.ts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of @unkey/id in the @unkey/db package # Test: Search for import statements of @unkey/id echo "Searching for import statements of @unkey/id:" rg --type typescript 'from\s+[\'"]@unkey/id[\'"]' ./internal/db/src # Test: Search for require statements of @unkey/id echo "Searching for require statements of @unkey/id:" rg --type typescript 'require\([\'"]@unkey/id[\'"]\)' ./internal/db/srcLength of output: 195
Script:
#!/bin/bash # Description: Verify the usage of @unkey/id in the @unkey/db package # Test: Search for import statements of @unkey/id echo "Searching for import statements of @unkey/id:" rg --type typescript "from\s+['\"]@unkey/id['\"]" ./internal/db/src # Test: Search for require statements of @unkey/id echo "Searching for require statements of @unkey/id:" rg --type typescript "require\(['\"]@unkey/id['\"]\)" ./internal/db/srcLength of output: 425
Script:
#!/bin/bash # Description: Verify the usage of @unkey/id in the @unkey/db package # Test: Search for import statements of @unkey/id echo "Searching for import statements of @unkey/id:" rg "from\s+['\"]@unkey/id['\"]" ./internal/db/src # Test: Search for require statements of @unkey/id echo "Searching for require statements of @unkey/id:" rg "require\(['\"]@unkey/id['\"]\)" ./internal/db/srcLength of output: 393
internal/id/src/generate.ts (2)
Line range hint
1-53: LGTM with minor suggestionsThe changes to the
prefixesobject are minimal and don't introduce any breaking modifications to the public API. The functionality of thenewIdfunction remains intact.Please consider addressing the suggestions in the previous comment regarding the clarification of
auditLogandauditLogBucketto improve code clarity and maintainability.
28-29: 🛠️ Refactor suggestionConsider clarifying the distinction between
auditLogandauditLogBucketThe addition of
auditLogBucket: "buk"is noted. However, there are a couple of points to consider:
- The re-addition of
auditLog: "log"appears redundant, as it was likely already present in the object. This could lead to confusion.- The distinction between
auditLogandauditLogBucketmight not be immediately clear to other developers.Consider the following improvements:
- Remove the redundant
auditLogentry if it's indeed a duplicate.- Add comments to clarify the specific use cases for
auditLogandauditLogBucket.Example:
const prefixes = { // ... other entries ... auditLogBucket: "buk", // Prefix for audit log storage buckets auditLog: "log", // Prefix for individual audit log entries // ... other entries ... } as const;This will enhance code clarity and maintainability.
To ensure
auditLogwas indeed present before and this is a redundant addition, we can check the git history:This will show us the commit history for this file, focusing on changes around the
auditLogentry.apps/agent/pkg/clickhouse/schema/000_README.md (3)
39-40: Improved naming convention for aggregation suffixes.The changes from '_daily' to '_per_day' and '_monthly' to '_per_month' enhance clarity and consistency in the naming convention. This update makes the time-based aggregation more explicit and aligns well with other time-based suffixes like '_per_hour'.
57-57: Example updated to reflect new naming convention.The materialized view example has been correctly updated to use the new '_per_day' suffix, maintaining consistency with the revised naming convention. This change helps reinforce the new standard through a practical example.
70-70: Example correctly updated to reflect new naming convention.The example for consistency across related objects has been appropriately updated to use the new '_per_day' suffix, aligning with the revised naming convention.
apps/api/src/pkg/db.ts (2)
2-8: LGTM: Enhanced type safety with new importsThe addition of new imports from "@unkey/db" package, including
ExtractTablesWithRelations,PlanetScaleDatabase,PlanetScaleTransaction, anddrizzle, enhances type safety and indicates a more structured approach to database operations. This is a positive change that will likely improve code quality and reduce potential runtime errors.
1-17: Overall assessment: Improved type safety and structureThe changes in this file enhance type safety and provide a more structured approach to database operations, particularly for transactions. These modifications align well with the PR objectives and contribute to improving the overall code quality. No issues or concerns were identified in the changes.
apps/api/src/pkg/cache/index.ts (1)
74-76: LGTM! Verify related changes for completeness.The addition of the
auditLogBucketByWorkspaceIdAndNamenamespace is consistent with the existing code structure and naming conventions. Good job on maintaining consistency!To ensure completeness:
- Confirm that the
CacheNamespacestype in the./namespacesfile has been updated to include this new namespace.- Verify that any necessary updates have been made in other parts of the codebase where cache operations are performed.
Let's verify the related changes:
tools/local/src/db.ts (1)
4-4: LGTM: Import statement fornewIdadded.The addition of the
newIdimport from "@unkey/id" is appropriate for generating unique IDs in the updated code.internal/db/src/schema/workspaces.ts (3)
13-13: LGTM: Import statement for auditLogBucketThe import statement for
auditLogBucketfrom./audit_logsis correctly added and necessary for the new relation introduced later in the file.
13-13: Summary: Audit log integration for workspacesThe changes in this file successfully integrate audit log functionality into the workspace schema:
- The import of
auditLogBucketis added.- A new
auditLogBucketsrelation is established inworkspacesRelations.These modifications enhance the schema's capability to manage and relate workspace data with audit logs, aligning with the PR objectives. The changes are well-structured and maintain consistency with the existing codebase.
Also applies to: 136-136
136-136: LGTM: New relation for auditLogBucketsThe
auditLogBucketsrelation is correctly added to theworkspacesRelationsobject. The many-to-one relationship withauditLogBucketis appropriate for the audit logs feature.To ensure the new relation is properly utilized, please run the following script to check for its usage in related components:
apps/api/src/pkg/middleware/metrics.ts (1)
Line range hint
56-64: Improved error logging for SDK telemetry ingestion.The addition of detailed error logging for SDK telemetry ingestion is a good practice. It provides comprehensive information for debugging, including the method, path, error message, telemetry data, and event details.
apps/api/src/pkg/key_migration/handler.ts (3)
Line range hint
1-224: Summary: Consistent migration to new analytics systemThe changes in this file consistently replace
ingestUnkeyAuditLogswithingestUnkeyAuditLogsTinybird, which appears to be part of a migration to a new analytics system (Tinybird). The modifications maintain the existing functionality while updating the method calls.Key points:
- The changes are consistent across all instances of audit logging.
- The data being logged remains unchanged.
- The new method supports logging multiple entries, which is utilized for role and permission connections.
Recommendations:
- Verify the implementation and performance of the new
ingestUnkeyAuditLogsTinybirdmethod.- Consider refactoring the audit logging for roles and permissions to reduce code duplication.
- Ensure that the new analytics system (Tinybird) is properly set up and configured to handle these audit logs.
Overall, the changes appear to be a straightforward migration to a new analytics system without introducing any apparent issues.
Line range hint
152-172: LGTM! Consider performance impact of multiple log entries.The change from
ingestUnkeyAuditLogstoingestUnkeyAuditLogsTinybirdis consistent with the previous update. The function now logs multiple audit entries for role connections, which is appropriate for this context.To ensure the new method handles multiple log entries efficiently, please run the following script:
#!/bin/bash # Description: Verify the performance impact of logging multiple entries # Test: Check if the new method supports batch inserts or uses any performance optimization techniques ast-grep --lang typescript --pattern 'class Analytics { $$$ ingestUnkeyAuditLogsTinybird($_: $_[]) { $$$ } $$$ }' # Test: Look for any performance-related comments or TODOs rg -i '(performance|optimization).*todo' --type ts
Line range hint
107-128: LGTM! Verify new analytics method implementation.The change from
ingestUnkeyAuditLogstoingestUnkeyAuditLogsTinybirdappears to be part of a migration to a new analytics system. The function signature and usage remain consistent with the previous implementation.To ensure the new method is correctly implemented, please run the following script:
apps/api/src/routes/v1_identities_updateIdentity.ts (1)
5-5: LGTM: New import for audit log handling.The new import of
insertUnkeyAuditLogfrom@/pkg/auditis consistent with the existing import style and appears to be related to the changes in audit log handling.apps/api/src/pkg/audit.ts (1)
85-85: Verify thatlog.context.locationcorrectly representsremoteIpThe
remoteIpfield is being set tolog.context?.location. Please verify thatlocationcontains the client's IP address. If not, consider using the appropriate field fromlog.contextthat represents the remote IP.Run the following script to verify the definition of
location:apps/api/src/routes/v1_permissions_deleteRole.ts (2)
73-100: Good use of transaction to ensure atomicityWrapping the role deletion and audit log insertion within a transaction ensures that both operations succeed or fail together, maintaining data consistency.
Line range hint
103-119: EnsureingestUnkeyAuditLogsTinybirdis correctly integratedSince you've updated the analytics ingestion method to
ingestUnkeyAuditLogsTinybird, verify that this function is correctly implemented and that all necessary adjustments have been made in the codebase to support this change.Run the following script to check for the implementation and usage of
ingestUnkeyAuditLogsTinybirdin the codebase:tools/migrate/auditlog-import.ts (1)
59-78: Verify thatDate.now()provides the correct timestamp format forupdatedAt.The
updatedAtfield is set usingDate.now(), which returns the number of milliseconds since the Unix epoch. Ensure that this format matches the expected data type ofupdatedAtin your database schema. IfupdatedAtexpects aDATETIMEorTIMESTAMPtype, you may need to convert it appropriately.You can adjust the code if necessary:
- .onDuplicateKeyUpdate({ set: { updatedAt: Date.now() } }); + .onDuplicateKeyUpdate({ set: { updatedAt: new Date() } });Alternatively, verify the expected data type and adjust accordingly.
apps/api/src/routes/v1_permissions_deletePermission.ts (3)
4-4: Import statement is correctly addedThe import of
insertUnkeyAuditLogfrom@/pkg/auditis appropriate and allows for the new audit logging functionality.
82-98: Audit logging for permission deletion is properly implementedThe addition of
insertUnkeyAuditLogensures that permission deletions are logged with all the necessary details, enhancing audit traceability.
82-98: Evaluate transaction scope for audit loggingThe
insertUnkeyAuditLogfunction is called within the database transaction, which might increase transaction duration and affect performance. Assess whether the audit log insertion needs to be part of the transaction for consistency. If not, consider moving it outside the transaction or deferring it to reduce potential delays.To verify, check if
insertUnkeyAuditLogdepends on the transaction context (tx) for data consistency or if it can operate independently.apps/api/src/routes/v1_apis_createApi.ts (1)
111-112: Verify compliance with privacy policies when capturinguserAgentdataCollecting and storing user agent information may have privacy implications. Please ensure that capturing this data complies with your application's privacy policies and relevant regulations.
Also applies to: 128-129
apps/api/src/routes/v1_permissions_createRole.ts (3)
4-4: Approved: Added necessary import statementThe
insertUnkeyAuditLogimport is correctly added and will be used within the transaction.
87-118: Good use of transaction to ensure atomicityWrapping the role creation and audit log insertion within a transaction ensures that both operations succeed or fail together, maintaining data consistency.
Line range hint
121-135: Verify the integration ofingestUnkeyAuditLogsTinybirdPlease ensure that the new method
analytics.ingestUnkeyAuditLogsTinybirdcorrectly handles audit log ingestion. If this method replacesingestUnkeyAuditLogs, verify that all instances have been updated accordingly.Run the following script to identify any remaining usages of
ingestUnkeyAuditLogs:✅ Verification successful
Audit log ingestion has been successfully updated.
All instances of
ingestUnkeyAuditLogshave been replaced withingestUnkeyAuditLogsTinybird. No remaining usages were found in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all occurrences of `ingestUnkeyAuditLogs` in the codebase. rg --type typescript 'analytics\.ingestUnkeyAuditLogs\('Length of output: 93
Script:
#!/bin/bash # Description: Find all occurrences of `ingestUnkeyAuditLogs` in TypeScript files. rg --glob '**/*.ts' --glob '**/*.tsx' 'analytics\.ingestUnkeyAuditLogs\('Length of output: 75
apps/api/src/routes/v1_permissions_createPermission.ts (1)
86-118: Verify the safety ofinsertUnkeyAuditLogwithin the transaction.By placing the
insertUnkeyAuditLogoperation inside the database transaction, you ensure atomicity between the permission creation and audit log insertion. However, it's important to verify thatinsertUnkeyAuditLogdoes not perform any operations that could potentially cause the transaction to fail or introduce side effects, such as interacting with external services or causing deadlocks.Run the following script to confirm that
insertUnkeyAuditLogonly interacts with the transaction object and doesn't make external calls:apps/api/src/routes/v1_keys_deleteKey.ts (2)
126-127: Good use of transaction for atomic operationsWrapping the key deletion within a transaction ensures that the deletion and related audit log insertion occur atomically, enhancing data integrity and consistency.
144-145: Verify the availability of context propertiesEnsure that
c.get("location")andc.get("userAgent")return defined values before using them in thecontextobject. If these properties might be undefined, consider providing default values or handling the absence to prevent potential errors.Run the following script to confirm that
locationanduserAgentare properly set:✅ Verification successful
Context Properties are Properly Initialized
The
c.get("location")andc.get("userAgent")properties are consistently set in the following files:
apps/semantic-cache/src/pkg/hono/app.tsapps/api/src/pkg/hono/app.tsAll usages within the
contextobject are preceded by these initializations, ensuring they are defined and preventing potential errors.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check where 'location' and 'userAgent' are set in the codebase. # Test: Search for the assignments of 'location' and 'userAgent'. Expect: Methods that set these values. rg --type typescript 'c\.set\("location"|c\.set\("userAgent"'Length of output: 98
Script:
#!/bin/bash # Description: Recheck where 'location' and 'userAgent' are set in the TypeScript codebase. # Test: Search for the assignments of 'location' and 'userAgent' within .ts and .tsx files. rg --type ts 'c\.set\("location"|c\.set\("userAgent"'Length of output: 365
apps/api/src/routes/v1_identities_deleteIdentity.ts (1)
90-137: Ensure logging failures do not affect the main operationIf
insertUnkeyAuditLogoranalytics.ingestUnkeyAuditLogsTinybirdfail, it might impact the transaction or the overall operation. Consider adding error handling to ensure that failures in logging do not prevent the identity deletion from completing successfully.You can verify the impact of logging failures by simulating an error in the logging functions and observing the transaction behavior.
apps/api/src/routes/v1_keys_removeRoles.ts (2)
Line range hint
164-191: Confirm the use ofwaitUntilfor background tasksUsing
c.executionCtx.waitUntilschedules the analytics ingestion to run asynchronously. Ensure that this behavior is acceptable and that any failure in the background task does not affect the response to the client.[approve]
You may want to verify that any errors in
analytics.ingestUnkeyAuditLogsTinybirdare properly logged and monitored.
122-161: Ensure transaction consistency and error handlingWhile wrapping the deletion and audit log insertion in a transaction ensures atomicity, consider what happens in case of a transaction failure. Additionally, verify that the analytics ingestion reflects the committed state.
Run the following script to check for proper error handling and transaction consistency:
Review the transaction implementations to ensure they handle errors appropriately.
apps/api/src/routes/v1_keys_removePermissions.ts (3)
4-4: Approved import of 'insertUnkeyAuditLog'The import of
insertUnkeyAuditLogfrom"@/pkg/audit"is necessary for the audit logging functionality introduced in this module.
122-162: Good use of transaction for atomic operationsWrapping the deletion of permissions and the insertion of audit logs within a transaction ensures atomicity and data consistency. This approach prevents partial updates in case of failures, which is a best practice.
Line range hint
165-192: Approved asynchronous audit log ingestionUsing
c.executionCtx.waitUntilwithanalytics.ingestUnkeyAuditLogsTinybirdallows for non-blocking ingestion of audit logs, improving response times while ensuring logs are processed.apps/api/src/routes/v1_apis_deleteApi.ts (3)
4-4: Added import forinsertUnkeyAuditLogThe import statement correctly adds
insertUnkeyAuditLogfrom"@/pkg/audit", which is used for logging audit events.
100-116: Consistent usage of context propertiesEnsure that the
contextobject being passed to the logging functions includes all necessary properties. For example, ifuserAgentorlocationcan be undefined, consider providing default values or handling potential undefined values to prevent errors in the logging functions.Confirm that
c.get("location")andc.get("userAgent")always return valid values.
147-171: Optimize the mapping ofkeyIdsIn lines 150-171,
keyIds.mapis used to create an array of audit log entries, which is then passed toinsertUnkeyAuditLog. IfinsertUnkeyAuditLogcan accept multiple entries at once, this is fine. However, ensure that the function handles arrays of entries correctly.Run the following script to verify that
insertUnkeyAuditLogsupports an array of entries:apps/api/src/routes/v1_keys_updateRemaining.ts (2)
4-4: Importing 'insertUnkeyAuditLog' FunctionThe
insertUnkeyAuditLogfunction is correctly imported from"@/pkg/audit", ensuring that audit logs can be inserted as intended.
Line range hint
199-213: Verify Asynchronous Logging ExecutionEnsure that
analytics.ingestUnkeyAuditLogsTinybirdis compatible withc.executionCtx.waitUntil. If the function returns a promise, it's suitable for use here. Otherwise, you might need to adjust the implementation to prevent any issues with the execution context.To confirm the compatibility, you can review the function's return type or test its behavior.
apps/api/src/routes/v1_identities_createIdentity.ts (3)
4-4: Import statement is appropriateThe import of
insertUnkeyAuditLogfrom"@/pkg/audit"aligns with the updated functionality.
144-154: Proper handling of optional ratelimitsThe code correctly processes ratelimits when provided, ensuring flexibility for the caller.
155-157: Conditional insertion of ratelimits enhances efficiencyRatelimits are inserted only when they are provided, which optimizes database operations.
apps/api/src/routes/legacy_keys_createKey.ts (1)
4-4: Import statement for 'insertUnkeyAuditLog' addedThe import of
insertUnkeyAuditLogfrom"@/pkg/audit"is appropriate and aligns with its usage later in the code.apps/api/src/routes/v1_keys_addRoles.ts (1)
237-238: EnsurelocationanduserAgentare safely retrievedIn the
contextobject, you're accessingc.get("location")andc.get("userAgent"). Verify that these values are always defined, or consider providing default values to handle cases where they might be undefined. This will prevent potential runtime errors or incomplete audit logs.Also applies to: 261-262
apps/api/src/routes/v1_keys_setPermissions.ts (1)
365-388:⚠️ Potential issueDuplicate audit log entries for
addPermissionsIt appears that
addPermissionsis being mapped over twice when constructing theauditLogsarray. This will result in duplicate audit log entries for the same event, which could lead to confusion and inaccurate logging data.Apply this diff to remove the redundant mapping over
addPermissions:// Existing mappings over disconnectPermissions and addPermissions // ... // Mapping over createPermissions ...createPermissions.map((r) => ({ workspaceId: auth.authorizedWorkspaceId, event: "permission.create" as const, actor: { type: "key" as const, id: auth.key.id, }, description: `Created ${r.id}`, resources: [ { type: "permission" as const, id: r.id, meta: { name: r.name, }, }, ], context: { location: c.get("location"), userAgent: c.get("userAgent"), }, })), - // Duplicate mapping over addPermissions - ...addPermissions.map((r) => ({ - workspaceId: auth.authorizedWorkspaceId, - event: "authorization.connect_permission_and_key" as const, - actor: { - type: "key" as const, - id: auth.key.id, - }, - description: `Connected ${r.id} and ${keyId}`, - resources: [ - { - type: "permission" as const, - id: r.id, - }, - { - type: "key" as const, - id: keyId, - }, - ], - context: { - location: c.get("location"), - userAgent: c.get("userAgent"), - }, - })), ];Likely invalid or redundant comment.
apps/api/src/routes/v1_ratelimit_limit.ts (1)
50-52: Ensure proper formatting of multi-line string indescriptionThe
descriptionfield spanning lines 50 to 52 is using a template literal for a multi-line string. Ensure that this formatting is intentional and that the backticks are correctly placed to avoid syntax errors.Check that the multi-line string is correctly interpreted and does not introduce unintended whitespace or formatting issues in the API documentation.
apps/api/src/routes/v1_keys_updateKey.ts (2)
4-4: Import statement verifiedThe
insertUnkeyAuditLogfunction is correctly imported from"@/pkg/audit".
385-413: Verify potential duplication of audit loggingBoth
insertUnkeyAuditLogandanalytics.ingestUnkeyAuditLogsTinybirdare called with similar parameters. Verify if both calls are necessary, or if this may result in duplicate audit log entries.Run the following script to check for other usages of these functions:
apps/api/src/routes/v1_keys_createKey.ts (3)
5-5: Import statement is appropriateThe import of
insertUnkeyAuditLogfrom@/pkg/auditis correct and used appropriately in the code.
435-459: Audit log creation logic is soundThe construction of the initial audit log for the key creation event is correctly implemented.
480-482: Verify TypeScript version compatibility with 'satisfies' operatorThe
satisfiesoperator is used to ensure that the objects conform to theUnkeyAuditLogtype. This feature was introduced in TypeScript 4.9. Please verify that your project's TypeScript version supports it.You can check the TypeScript version in your project by running the following script:
Also applies to: 503-505
✅ Verification successful
TypeScript version compatibility verified
The project's TypeScript version is 5.5.3, which supports the
satisfiesoperator introduced in TypeScript 4.9.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the TypeScript version specified in package.json. # Test: Output the TypeScript version. Expect: Version 4.9 or later. jq -r '.devDependencies.typescript' package.jsonLength of output: 55
apps/api/src/routes/v1_migrations_createKey.ts (1)
Line range hint
506-506: Ensure non-null assertion onk.keyAuthId!is safeIn lines 506 and 543,
k.keyAuthId!uses the non-null assertion operator!. While previous checks suggest thatk.keyAuthIdshould not be null, it's important to guarantee this to prevent potential runtime errors.To confirm that
k.keyAuthIdis always defined:This script searches for any instance where
keyAuthIdis assigned a potentially null or undefined value in the routes directory.Also applies to: 543-543
✅ Verification successful
Verified: All instances of
keyAuthId!are confirmed to be non-null, ensuring the safety of the non-null assertion.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all keys have a non-null keyAuthId # Test: Check for any keys where keyAuthId might be null or undefined. rg --type typescript 'keyAuthId\s*:\s*[^;]+;' -A 5 apps/api/src/routes/ | gawk '/keyAuthId/ && /undefined|null/ { print FILENAME ": " $0 }'Length of output: 176
Script:
#!/bin/bash # Description: Verify that all keys have a non-null keyAuthId # Test: Check for any keys where keyAuthId might be null or undefined. rg 'keyAuthId\s*=\s*(null|undefined)' -A 5 apps/api/src/routes/ --type-add 'typescript:*.ts,*.tsx' --type typescript | gawk '/keyAuthId\s*=\s*(null|undefined)/ { print FILENAME ": " $0 }'Length of output: 189
apps/agent/pkg/clickhouse/schema/003_create_raw_telemetry_sdks_table.sql
Show resolved
Hide resolved
There was a problem hiding this comment.
Caution
Inline review comments failed to post
🛑 Comments failed to post (86)
apps/dashboard/lib/trpc/routers/key/createRootKey.ts (1)
3-3: 💡 Codebase verification
Audit Logging Functions Incomplete Refactoring Detected
The old
ingestAuditLogsfunction is still being used in several files, which may lead to inconsistent audit logging behavior. Please update all instances to use the newingestAuditLogsTinybirdorinsertAuditLogsfunctions.Affected files:
- apps/workflows/lib/tinybird.ts
- apps/workflows/jobs/refill-monthly.ts
- apps/workflows/jobs/refill-daily.ts
- apps/workflows/jobs/end-trials.ts
- apps/workflows/jobs/downgrade-requests.ts
🔗 Analysis chain
Verify the impact of audit logging changes.
The changes in imports suggest a shift in the audit logging mechanism:
ingestAuditLogshas been renamed toingestAuditLogsTinybird, indicating a possible move to a Tinybird-specific implementation.- A new
insertAuditLogsfunction has been introduced from@/lib/audit.These changes may affect the audit logging process throughout the application.
To ensure these changes are consistently applied, run the following script:
Also applies to: 11-11
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new audit logging functions across the codebase. # Test 1: Check for any remaining usage of the old 'ingestAuditLogs' function echo "Checking for old 'ingestAuditLogs' usage:" rg --type typescript 'ingestAuditLogs\(' # Test 2: Verify the usage of the new 'ingestAuditLogsTinybird' function echo "Checking 'ingestAuditLogsTinybird' usage:" rg --type typescript 'ingestAuditLogsTinybird\(' # Test 3: Verify the usage of the new 'insertAuditLogs' function echo "Checking 'insertAuditLogs' usage:" rg --type typescript 'insertAuditLogs\('Length of output: 522
Script:
#!/bin/bash # Description: Verify the usage of new audit logging functions across the codebase using specific file extensions. # Test 1: Check for any remaining usage of the old 'ingestAuditLogs' function echo "Checking for old 'ingestAuditLogs' usage in .ts and .tsx files:" rg 'ingestAuditLogs\(' --glob '*.ts' --glob '*.tsx' # Test 2: Verify the usage of the new 'ingestAuditLogsTinybird' function echo "Checking 'ingestAuditLogsTinybird' usage in .ts and .tsx files:" rg 'ingestAuditLogsTinybird\(' --glob '*.ts' --glob '*.tsx' # Test 3: Verify the usage of the new 'insertAuditLogs' function echo "Checking 'insertAuditLogs' usage in .ts and .tsx files:" rg 'insertAuditLogs\(' --glob '*.ts' --glob '*.tsx'Length of output: 12674
apps/dashboard/lib/tinybird.ts (1)
507-507: 💡 Codebase verification
Remaining References to
ingestAuditLogsFoundSeveral files still reference the old function name
ingestAuditLogs. Please update these references toingestAuditLogsTinybirdto ensure consistency and prevent potential errors.
apps/workflows/jobs/refill-daily.tsapps/workflows/jobs/refill-monthly.tsapps/workflows/lib/tinybird.tsapps/workflows/jobs/end-trials.tsapps/workflows/jobs/downgrade-requests.ts🔗 Analysis chain
Approved: Function renamed for clarity
The function
ingestAuditLogshas been renamed toingestAuditLogsTinybird, which more accurately reflects its purpose and the underlying technology used. This change improves code readability and maintainability.To ensure this change doesn't break existing code, please run the following script to check for any remaining references to the old function name:
If any results are found, they should be updated to use the new function name
ingestAuditLogsTinybird.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to 'ingestAuditLogs' rg 'ingestAuditLogs\s*\(' --type tsLength of output: 384
apps/dashboard/lib/audit/index.ts (2)
58-58: 🛠️ Refactor suggestion
Consider using the event's timestamp instead of the current time
Currently, the
timefield is set usingDate.now(), which records the insertion time. If thelogobject includes the actual event time (e.g.,log.timeorlog.timestamp), consider using that value to accurately reflect when the event occurred.
30-46: 🛠️ Refactor suggestion
Extract bucket retrieval and creation logic into a helper function
The code for retrieving or creating a bucket is used within the loop and could be abstracted into a separate function. This enhances readability and promotes code reuse.
apps/dashboard/lib/trpc/routers/llmGateway/delete.ts (2)
39-69:
⚠️ Potential issuePotential duplication in audit logging
The code logs audit events both inside the transaction using
insertAuditLogsand after the transaction usingingestAuditLogsTinybird. This could lead to duplicate audit entries or inconsistencies if one operation succeeds and the other fails.Consider consolidating the audit logging to ensure consistency. If both functions are intended for different purposes, clarify their roles and handle potential failures appropriately. You might want to perform both logging operations within the transaction or ensure that they are idempotent.
43-48:
⚠️ Potential issueError handling may obscure underlying issues
In the catch block for the deletion operation, the original error
_erris caught but not logged or passed along. Throwing a genericINTERNAL_SERVER_ERRORwithout logging the original error could make debugging more difficult.Consider logging the original error or including additional context to aid in troubleshooting. Ensure that sensitive information is not exposed in error messages.
Apply this diff to log the error:
await tx .delete(schema.llmGateways) .where(eq(schema.llmGateways.id, input.gatewayId)) .catch((_err) => { + console.error("Error deleting LLM gateway:", _err); throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: "We are unable to delete the LLM gateway. Please contact support using support@unkey.dev", }); });📝 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..catch((_err) => { console.error("Error deleting LLM gateway:", _err); throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: "We are unable to delete the LLM gateway. Please contact support using support@unkey.dev", });apps/dashboard/lib/trpc/routers/key/updateEnabled.ts (1)
46-51:
⚠️ Potential issueEnsure proper error propagation within the transaction
Catching and rethrowing errors within the transaction's
updateoperation may prevent the transaction from rolling back automatically. The.catch()block at lines 46-51 handles errors locally, which could interfere with rollback mechanisms.To ensure the transaction rolls back correctly on errors, consider removing the
.catch()block and letting the error propagate:.where(eq(schema.keys.id, key.id)) - .catch((_err) => { - throw new TRPCError({ - code: "INTERNAL_SERVER_ERROR", - message: - "We were unable to update enabled on this key. Please contact support using support@unkey.dev", - }); - });Alternatively, handle the error at the transaction level to maintain proper rollback behavior.
📝 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..where(eq(schema.keys.id, key.id))apps/dashboard/lib/trpc/routers/workspace/changeName.ts (1)
46-62:
⚠️ Potential issueEnsure consistent audit logging by aligning transaction context.
Currently,
insertAuditLogsis called within the transactiontx, whileingestAuditLogsTinybirdis called afterward. If the transaction is rolled back due to an error afterinsertAuditLogsexecutes, the audit log in the database will be reverted, but the log sent viaingestAuditLogsTinybirdmay have already been processed. This can lead to inconsistencies between your internal audit logs and the external logging system. Consider movingingestAuditLogsTinybirdinside the transaction to ensure both logging actions are atomic, or implement logic to handle potential inconsistencies.apps/dashboard/lib/trpc/routers/key/updateName.ts (2)
40-74:
⚠️ Potential issueEnsure atomicity of audit logging within the transaction.
While the database update and
insertAuditLogsare wrapped inside a transaction, the call toingestAuditLogsTinybirdoccurs outside of it. If the transaction fails afterinsertAuditLogsbut beforeingestAuditLogsTinybird, this could lead to inconsistencies between your database and external logging system. Consider includingingestAuditLogsTinybirdwithin the transaction or handling failures to maintain data integrity.
62-67: 🛠️ Refactor suggestion
Enhance audit log detail with previous and new key names.
Currently, the audit log description mentions the key ID and the new name. Including the previous name can provide more context and improve traceability in audit logs.
Update the
descriptionfield:- description: `Changed name of ${key.id} to ${input.name}`, + description: `Changed name of key ${key.id} from '${key.name}' to '${input.name}'`,Committable suggestion was skipped due to low confidence.
apps/dashboard/lib/trpc/routers/key/updateOwnerId.ts (1)
40-73:
⚠️ Potential issuePotential inconsistency between database transaction and audit log ingestion
Within the transaction block (lines 40-73), you call
insertAuditLogs(tx, {...})to log audit data. However, the subsequent call toingestAuditLogsTinybird({...})(line 75) occurs outside the transaction. If the transaction fails or rolls back, the audit logs inserted into the database will not correspond with the data ingested into Tinybird, leading to inconsistency.Consider moving the
ingestAuditLogsTinybirdcall inside the transaction to ensure atomicity. This way, both the database update and the audit log ingestion either succeed or fail together.apps/dashboard/lib/trpc/routers/api/updateName.ts (2)
54-73: 🛠️ Refactor suggestion
Consolidate audit logging to reduce duplication
Both
insertAuditLogsandingestAuditLogsTinybirdare logging audit information with similar data. Consider creating a common abstraction or helper function to handle audit logging in one place. This will reduce code duplication and improve maintainability.
40-73:
⚠️ Potential issueEnsure error handling for
insertAuditLogswithin the transactionWithin the transaction block, only the update operation includes error handling with a catch block. If
insertAuditLogsfails, the error might not be properly handled, potentially leaving the transaction in an inconsistent state. Consider adding error handling forinsertAuditLogsto manage possible errors effectively.Apply this diff to add error handling:
await insertAuditLogs(tx, { // parameters }) + .catch((_err) => { + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: + "We were unable to log the audit entry. Please contact support using support@unkey.dev.", + }); + });📝 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.await db.transaction(async (tx) => { await tx .update(schema.apis) .set({ name: input.name, }) .where(eq(schema.apis.id, input.apiId)) .catch((_err) => { throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: "We were unable to update the API name. Please contact support using support@unkey.dev.", }); }); await insertAuditLogs(tx, { workspaceId: api.workspace.id, actor: { type: "user", id: ctx.user.id, }, event: "api.update", description: `Changed ${api.id} name from ${api.name} to ${input.name}`, resources: [ { type: "api", id: api.id, }, ], context: { location: ctx.audit.location, userAgent: ctx.audit.userAgent, }, }) .catch((_err) => { throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: "We were unable to log the audit entry. Please contact support using support@unkey.dev.", }); }); });apps/dashboard/lib/trpc/routers/workspace/optIntoBeta.ts (3)
1-3:
⚠️ Potential issueEnsure consistent audit logging mechanism
Both
insertAuditLogs(imported from"@/lib/audit") andingestAuditLogsTinybird(imported from"@/lib/tinybird") are used in this file. This could lead to duplication of audit logs or inconsistency in how logs are handled. Verify if both functions are necessary or if you can consolidate them to use a single audit logging method to maintain consistency.
57-72:
⚠️ Potential issuePotential duplication of audit logs within transaction
Within the transaction,
insertAuditLogsis called to log the audit event. Later, outside the transaction,ingestAuditLogsTinybirdis also called to ingest audit logs. This may result in the same audit event being logged twice. Consider unifying these calls to prevent duplication.
74-75: 🛠️ Refactor suggestion
Use a structured logging framework instead of
console.errorUsing
console.error(err);directly may not be optimal for error tracking in production environments. It's advisable to utilize a structured logging framework or service that can capture, format, and transport logs appropriately. This enhances log management and aids in monitoring and debugging.apps/dashboard/lib/trpc/routers/api/updateDeleteProtection.ts (2)
54-76: 🛠️ Refactor suggestion
Avoid code duplication by abstracting common audit log parameters.
The parameters passed to both
insertAuditLogsandingestAuditLogsTinybirdare nearly identical. To enhance maintainability and reduce duplication, consider creating a shared variable or function to generate these parameters.You can define a common
auditLogDataobject and reuse it in both function calls:const auditLogData = { workspaceId: api.workspace.id, actor: { type: "user", id: ctx.user.id, }, event: "api.update", description: `API ${api.name} delete protection is now ${ input.enabled ? "enabled" : "disabled" }`, resources: [ { type: "api", id: api.id, meta: { deleteProtection: input.enabled, }, }, ], context: { location: ctx.audit.location, userAgent: ctx.audit.userAgent, }, }; await insertAuditLogs(tx, auditLogData); // ... await ingestAuditLogsTinybird(auditLogData);Also applies to: 79-99
61-63:
⚠️ Potential issueRemove the extra closing brace in the description string.
There's an extra closing brace
}at the end of the template literal in thedescriptionfield, which can cause a syntax error.Apply this diff to fix the issue:
description: `API ${api.name} delete protection is now ${ input.enabled ? "enabled" : "disabled" - }.}`, + }`,📝 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.description: `API ${api.name} delete protection is now ${ input.enabled ? "enabled" : "disabled" }`,apps/dashboard/lib/trpc/routers/rbac/createPermission.ts (1)
47-73: 🛠️ Refactor suggestion
Consider consolidating audit logging to prevent duplication
Within the transaction, you're inserting audit logs using
insertAuditLogs(tx, {...}). After the transaction, you're callingingestAuditLogsTinybird({...})with the same audit log data. This may result in duplicate logging or unnecessary redundancy. If both functions serve different purposes (e.g., one logs to your database and the other sends logs to Tinybird), consider combining them into a single function or module to handle both operations simultaneously, or document the reasoning for separate calls.apps/dashboard/lib/trpc/routers/rbac/disconnectRoleFromKey.ts (1)
46-65:
⚠️ Potential issueEnsure consistency between database and Tinybird audit logs
Currently,
insertAuditLogsis called within the transaction, andingestAuditLogsTinybirdis called after the transaction commits. IfingestAuditLogsTinybirdfails, the audit logs in Tinybird will be out of sync with your database logs.Consider handling failures from
ingestAuditLogsTinybirdby implementing a retry mechanism or logging the failure for later processing. This will help maintain consistency between your internal database logs and Tinybird audit logs.Also applies to: 76-92
apps/dashboard/lib/trpc/routers/rbac/updateRole.ts (3)
35-36:
⚠️ Potential issueUse a proper logging mechanism instead of
console.errorUsing
console.error(err);for error logging is not recommended in production code. Consider using a dedicated logging library or service that allows for configurable logging levels and better management of log outputs.
78-79:
⚠️ Potential issueUse a proper logging mechanism in transaction error handling
Similar to the previous catch block, replace
console.error(err);with a proper logging mechanism to ensure consistent and effective error logging across the application.
61-76: 🛠️ Refactor suggestion
Refactor to avoid duplication of audit log data construction
The audit log data constructed in the transaction (lines 61-76) is very similar to the data used in
ingestAuditLogsTinybird(lines 87-93). Consider creating the audit log data object once and reusing it for both functions to reduce code duplication and improve maintainability.Apply the following changes to extract the audit log data into a single variable:
+ const auditLogData = { + workspaceId: workspace.id, + actor: { type: "user", id: ctx.user.id }, + event: "role.update", + description: `Updated role ${input.id}`, + resources: [ + { + type: "role", + id: input.id, + }, + ], + context: { + location: ctx.audit.location, + userAgent: ctx.audit.userAgent, + }, + }; + await insertAuditLogs(tx, auditLogData);And later:
- await ingestAuditLogsTinybird({ - workspaceId: workspace.id, - actor: { type: "user", id: ctx.user.id }, - event: "role.update", - description: `Updated role ${input.id}`, - resources: [ - { - type: "role", - id: input.id, - }, - ], - context: { - location: ctx.audit.location, - userAgent: ctx.audit.userAgent, - }, - }); + await ingestAuditLogsTinybird(auditLogData);Also applies to: 87-93
apps/dashboard/lib/trpc/routers/rbac/deletePermission.ts (1)
57-72: 🛠️ Refactor suggestion
Refactor to eliminate duplication of audit log data
The audit log data used in both
insertAuditLogsandingestAuditLogsTinybirdfunctions is identical. To adhere to the DRY (Don't Repeat Yourself) principle and enhance maintainability, consider defining the audit log data once and reusing it in both function calls.Apply this diff to refactor the code:
await db .transaction(async (tx) => { await tx .delete(schema.permissions) .where( and( eq(schema.permissions.id, input.permissionId), eq(schema.permissions.workspaceId, workspace.id), ), ); + const auditLogData = { + workspaceId: workspace.id, + actor: { type: "user", id: ctx.user.id }, + event: "permission.delete", + description: `Deleted permission ${input.permissionId}`, + resources: [ + { + type: "permission", + id: input.permissionId, + }, + ], + context: { + location: ctx.audit.location, + userAgent: ctx.audit.userAgent, + }, + }; - await insertAuditLogs(tx, { - workspaceId: workspace.id, - actor: { type: "user", id: ctx.user.id }, - event: "permission.delete", - description: `Deleted permission ${input.permissionId}`, - resources: [ - { - type: "permission", - id: input.permissionId, - }, - ], - context: { - location: ctx.audit.location, - userAgent: ctx.audit.userAgent, - }, - }); + await insertAuditLogs(tx, auditLogData); }) .catch((err) => { console.error(err); throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: "We are unable to delete the permission. Please contact support using support@unkey.dev", }); }); - await ingestAuditLogsTinybird({ - workspaceId: workspace.id, - actor: { type: "user", id: ctx.user.id }, - event: "permission.delete", - description: `Deleted permission ${input.permissionId}`, - resources: [ - { - type: "permission", - id: input.permissionId, - }, - ], - context: { - location: ctx.audit.location, - userAgent: ctx.audit.userAgent, - }, - }); + await ingestAuditLogsTinybird(auditLogData);Also applies to: 83-95
apps/dashboard/lib/trpc/routers/key/delete.ts (2)
60-75: 🛠️ Refactor suggestion
Refactor to eliminate duplicate code when creating audit logs
The mapping over
workspace.keysto create audit log entries is duplicated in lines 60-75 and again in lines 87-102. Extracting this data into a sharedauditLogsvariable enhances maintainability and reduces code duplication.Apply this refactor to define a shared
auditLogsvariable and reuse it:+ const auditLogs = workspace.keys.map((key) => ({ + workspaceId: workspace.id, + actor: { type: "user", id: ctx.user.id }, + event: "key.delete", + description: `Deleted ${key.id}`, + resources: [ + { + type: "key", + id: key.id, + }, + ], + context: { + location: ctx.audit.location, + userAgent: ctx.audit.userAgent, + }, + })); // Inside the transaction - insertAuditLogs( - tx, - workspace.keys.map((key) => ({ ... })) - ); + insertAuditLogs(tx, auditLogs); // After the transaction - await ingestAuditLogsTinybird( - workspace.keys.map((key) => ({ ... })) - ); + await ingestAuditLogsTinybird(auditLogs);Also applies to: 87-102
58-76: 💡 Codebase verification
⚠️ Potential issueDuplicate audit logging confirmed: Both
insertAuditLogsandingestAuditLogsTinybirdwrite to separate audit log destinations, leading to redundant entries.
- Consider consolidating audit logging to a single destination to prevent unnecessary duplication.
🔗 Analysis chain
Duplicate audit logging: potential for redundant entries
Both
insertAuditLogs(lines 58-76) andingestAuditLogsTinybird(lines 86-103) are called to log the same audit events. This might result in duplicate audit log entries. Please verify whether both functions are required, or if logging can be streamlined to prevent duplication.To verify if both functions log to the same destination, you can run the following script:
Also applies to: 86-103
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if 'insertAuditLogs' and 'ingestAuditLogsTinybird' write to the same audit log destination. # Search for the implementation details of both functions. rg --type ts 'function insertAuditLogs' -A 20 rg --type ts 'function ingestAuditLogsTinybird' -A 20Length of output: 2858
apps/dashboard/lib/trpc/routers/key/deleteRootKey.ts (1)
60-78: 🛠️ Refactor suggestion
Reduce code duplication by reusing audit log data.
Both
insertAuditLogsandingestAuditLogsTinybirduse nearly identical audit log entries. To enhance maintainability and reduce duplication, consider creating the audit log entries once and reusing them for both functions.Here's how you might refactor the code:
+ const auditLogEntries = rootKeys.map((key) => ({ + workspaceId: workspace.id, + actor: { type: "user", id: ctx.user.id }, + event: "key.delete", + description: `Deleted ${key.id}`, + resources: [ + { + type: "key", + id: key.id, + }, + ], + context: { + location: ctx.audit.location, + userAgent: ctx.audit.userAgent, + }, + })); await db .transaction(async (tx) => { await tx .update(schema.keys) .set({ deletedAt: new Date() }) .where( inArray( schema.keys.id, rootKeys.map((k) => k.id), ), ); - await insertAuditLogs( - tx, - rootKeys.map((key) => ({ - workspaceId: workspace.id, - actor: { type: "user", id: ctx.user.id }, - event: "key.delete", - description: `Deleted ${key.id}`, - resources: [ - { - type: "key", - id: key.id, - }, - ], - context: { - location: ctx.audit.location, - userAgent: ctx.audit.userAgent, - }, - })), - ); + await insertAuditLogs(tx, auditLogEntries); }) .catch((_err) => { throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: "We are unable to delete the root key. Please contact support using support@unkey.dev", }); }); await ingestAuditLogsTinybird(auditLogEntries);Also applies to: 88-100
apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts (2)
69-74: 🛠️ Refactor suggestion
Use error codes instead of parsing error messages
Parsing error messages to handle duplicate entries may not be reliable, as error messages can change. Consider using specific error codes or exceptions provided by the database library to detect duplicate entry errors.
78-78:
⚠️ Potential issueTypo in error message: 'namspace' should be 'namespace'
There's a typo in the error message; 'namspace' should be corrected to 'namespace' to ensure clarity.
Apply this diff to fix the typo:
- "We are unable to create namspace. Please contact support using support@unkey.dev", + "We are unable to create namespace. Please contact support using support@unkey.dev",📝 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."We are unable to create namespace. Please contact support using support@unkey.dev",apps/dashboard/lib/trpc/routers/rbac/connectPermissionToRole.ts (1)
71-76: 🛠️ Refactor suggestion
Consider Logging the Original Error for Debugging
In the
.catchblock, the original error_erris being discarded when throwing a newTRPCError. While it's important not to expose internal errors to the user, logging the original error internally can aid in debugging without revealing sensitive information to the user.You might modify the code to log the error:
.catch((_err) => { + // Log the original error for internal debugging + console.error('Error inserting into rolesPermissions:', _err); throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: "We are unable to connect the permission to the role. Please contact support using support@unkey.dev.", }); });📝 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..catch((_err) => { // Log the original error for internal debugging console.error('Error inserting into rolesPermissions:', _err); throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: "We are unable to connect the permission to the role. Please contact support using support@unkey.dev.", });apps/dashboard/lib/trpc/routers/rbac/disconnectPermissionFromRole.ts (2)
46-65:
⚠️ Potential issuePotential duplication in audit logging
It appears that audit logs are being recorded twice: once within the transaction using
insertAuditLogs(lines 46-65), and again after the transaction usingingestAuditLogsTinybird(lines 76-88). Verify if both calls are necessary to prevent duplicate audit entries. If both are required for different purposes (e.g., logging to different destinations), consider adding comments to clarify their distinct roles.Also applies to: 76-88
68-68: 🛠️ Refactor suggestion
Use a centralized logging mechanism instead of
console.errorUsing
console.error(err);may not be ideal for error logging in a production environment. Consider utilizing a centralized logging service or framework to handle errors consistently across the application.apps/dashboard/lib/trpc/routers/llmGateway/create.ts (1)
1-3:
⚠️ Potential issueReview the necessity of importing both
insertAuditLogsandingestAuditLogsTinybirdYou're importing both
insertAuditLogsfrom@/lib/auditandingestAuditLogsTinybirdfrom@/lib/tinybird. Since both functions relate to audit logging, consider whether both are necessary or if their functionalities overlap. Consolidating them could simplify the code and prevent potential duplication in audit logs.apps/dashboard/lib/trpc/routers/ratelimit/deleteOverride.ts (2)
63-84: 🛠️ Refactor suggestion
Consolidate audit logging to reduce duplication
Both
insertAuditLogsandingestAuditLogsTinybirdare being called with the same data, resulting in redundant code and potential maintenance overhead.Consider creating a single function that handles both inserting and ingesting audit logs, or ensure that both functions are necessary and serve distinct purposes. This will improve maintainability and reduce the risk of inconsistencies.
51-61:
⚠️ Potential issueEnsure transaction errors properly trigger rollback
Currently, errors in the update operation within the transaction are handled using
.catch, and aTRPCErroris thrown. However, throwing an error inside the.catchblock may not correctly propagate the error to rollback the transaction.Consider using a
try...catchblock inside the transaction callback to handle errors properly, ensuring that any error will cause the transaction to rollback.Here's a suggested refactor:
await db.transaction(async (tx) => { + try { await tx .update(schema.ratelimitOverrides) .set({ deletedAt: new Date() }) .where(eq(schema.ratelimitOverrides.id, override.id)); await insertAuditLogs(tx, { workspaceId: override.namespace.workspace.id, // ...remaining code... }); + } catch (_err) { + throw new TRPCError({ + message: + "We are unable to delete the override. Please contact support using support@unkey.dev", + code: "INTERNAL_SERVER_ERROR", + }); + } });Committable suggestion was skipped due to low confidence.
apps/dashboard/lib/trpc/routers/api/create.ts (3)
74-92:
⚠️ Potential issueHandle errors from 'insertAuditLogs' within the transaction
If
insertAuditLogsfails, the transaction will roll back, but the error isn't explicitly handled. This could result in a generic error being returned to the client.Consider adding error handling for
insertAuditLogsto provide more informative feedback:await insertAuditLogs(tx, { // existing parameters }) + .catch((_err) => { + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: + "We encountered an issue while logging the audit event. Please contact support at support@unkey.dev", + }); + });📝 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.await insertAuditLogs(tx, { workspaceId: ws.id, actor: { type: "user", id: ctx.user.id, }, event: "api.create", description: `Created ${apiId}`, resources: [ { type: "api", id: apiId, }, ], context: { location: ctx.audit.location, userAgent: ctx.audit.userAgent, }, }) .catch((_err) => { throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: "We encountered an issue while logging the audit event. Please contact support at support@unkey.dev", }); });
93-111:
⚠️ Potential issuePotential inconsistency with 'ingestAuditLogsTinybird' outside the transaction
Calling
ingestAuditLogsTinybirdwithin the transaction without being part of it may lead to data inconsistency if the transaction rolls back after this function executes.To ensure consistency, consider moving
ingestAuditLogsTinybirdafter the transaction completes successfully:}); + // Move this outside the transaction + await ingestAuditLogsTinybird({ + workspaceId: ws.id, + actor: { + type: "user", + id: ctx.user.id, + }, + event: "api.create", + description: `Created ${apiId}`, + resources: [ + { + type: "api", + id: apiId, + }, + ], + context: { + location: ctx.audit.location, + userAgent: ctx.audit.userAgent, + }, + });This ensures that
ingestAuditLogsTinybirdis only called if the transaction commits successfully.Committable suggestion was skipped due to low confidence.
4-4: 💡 Codebase verification
Missing 'insertAuditLogs' export in '@/lib/audit'
It appears that
insertAuditLogsis not exported from@/lib/audit. Instead,ingestAuditLogsTinybirdis exported in severaltinybird.tsfiles. Please verify ifinsertAuditLogshas been renamed or moved, and update the import path or function name accordingly.🔗 Analysis chain
Verify the import path and exported function
Ensure that
insertAuditLogsis correctly exported from@/lib/auditand that the import path is accurate.Run the following script to confirm the export:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that 'insertAuditLogs' is exported from '@/lib/audit' # Test: Search for the export statement. Expect: 'insertAuditLogs' should be exported. rg 'export.*insertAuditLogs' $(fd 'audit.*\.ts$')Length of output: 363
apps/dashboard/lib/trpc/routers/rbac/updatePermission.ts (1)
86-87:
⚠️ Potential issueAvoid logging errors directly to the console
Logging errors directly to the console may expose sensitive information in production environments. Consider using a secure logging mechanism that properly handles sensitive data.
Apply this diff to address the issue:
- console.error(err); + // Log the error using a secure logging service + logger.error('Error updating permission:', err);Committable suggestion was skipped due to low confidence.
apps/dashboard/lib/trpc/routers/api/updateIpWhitelist.ts (2)
64-69: 🛠️ Refactor suggestion
Simplify error handling within the transaction
The
catchblock inside the transaction catches errors from theupdateoperation and throws a newTRPCError. Since any error within the transaction will propagate and cause it to roll back, you may not need to catch and rethrow the error here. Letting the error propagate naturally can simplify your code.Consider removing the
catchblock to allow errors to be handled by the outer error handler:await tx .update(schema.apis) .set({ ipWhitelist: newIpWhitelist, }) .where(eq(schema.apis.id, input.apiId)) - .catch((_err) => { - throw new TRPCError({ - code: "INTERNAL_SERVER_ERROR", - message: - "We are unable to update the API whitelist. Please contact support using support@unkey.dev", - }); - });📝 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.await tx .update(schema.apis) .set({ ipWhitelist: newIpWhitelist, }) .where(eq(schema.apis.id, input.apiId))
78-78:
⚠️ Potential issueHandle potential
nullvalues in audit log descriptionIf
api.ipWhitelistornewIpWhitelistisnull, the description in the audit log may contain the string'null', which could be confusing in the logs.Consider updating the description to handle
nullvalues gracefully:description: `Changed ${api.id} IP whitelist from ${api.ipWhitelist} to ${newIpWhitelist}`, + description: \`Changed ${api.id} IP whitelist from \${api.ipWhitelist ?? 'none'} to \${newIpWhitelist ?? 'none'}\`,This ensures that the audit log is clear even when the IP whitelist is empty.
📝 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.description: `Changed ${api.id} IP whitelist from ${api.ipWhitelist ?? 'none'} to ${newIpWhitelist ?? 'none'}`,apps/dashboard/lib/trpc/routers/key/updateExpiration.ts (1)
73-94: 🛠️ Refactor suggestion
Avoid code duplication by reusing the audit log data object
The audit log data is constructed twice for
insertAuditLogsandingestAuditLogsTinybirdwith identical content. To enhance maintainability and adhere to the DRY (Don't Repeat Yourself) principle, consider creating a variable to hold the audit log data and reuse it in both calls.Apply this diff to refactor:
+ const auditLogData = { + workspaceId: key.workspace.id, + actor: { + type: "user", + id: ctx.user.id, + }, + event: "key.update", + description: `${ + input.expiration + ? `Changed expiration of ${key.id} to ${input.expiration.toUTCString()}` + : `Disabled expiration for ${key.id}` + }`, + resources: [ + { + type: "key", + id: key.id, + }, + ], + context: { + location: ctx.audit.location, + userAgent: ctx.audit.userAgent, + }, + }; + await insertAuditLogs(tx, { - workspaceId: key.workspace.id, - actor: { - type: "user", - id: ctx.user.id, - }, - event: "key.update", - description: `${ - input.expiration - ? `Changed expiration of ${key.id} to ${input.expiration.toUTCString()}` - : `Disabled expiration for ${key.id}` - }`, - resources: [ - { - type: "key", - id: key.id, - }, - ], - context: { - location: ctx.audit.location, - userAgent: ctx.audit.userAgent, - }, - }); + ...auditLogData, + }); await ingestAuditLogsTinybird({ - workspaceId: key.workspace.id, - actor: { - type: "user", - id: ctx.user.id, - }, - event: "key.update", - description: `${ - input.expiration - ? `Changed expiration of ${key.id} to ${input.expiration.toUTCString()}` - : `Disabled expiration for ${key.id}` - }`, - resources: [ - { - type: "key", - id: key.id, - }, - ], - context: { - location: ctx.audit.location, - userAgent: ctx.audit.userAgent, - }, - }); + ...auditLogData, + });Also applies to: 98-117
apps/dashboard/lib/trpc/routers/rbac/connectRoleToKey.ts (1)
65-99:
⚠️ Potential issueEnsure atomicity by including all audit logs within the transaction.
While you've correctly wrapped the database insert and the
insertAuditLogsfunction within a transaction, the subsequent call toingestAuditLogsTinybirdoccurs outside the transaction. This might lead to inconsistencies if the transaction fails but the audit log is still ingested by Tinybird. Consider moving theingestAuditLogsTinybirdcall inside the transaction to maintain atomicity and ensure that audit logging is only performed upon successful transaction completion.apps/dashboard/lib/trpc/routers/rbac/addPermissionToRootKey.ts (3)
109-109:
⚠️ Potential issueHandle potential errors from
ingestAuditLogsTinybirdThe
ingestAuditLogsTinybirdfunction is called after the transaction commits. If this function fails, there is currently no error handling, which could result in audit logs not being ingested without any notification or retry mechanism.Consider wrapping the
ingestAuditLogsTinybirdcall in atry...catchblock to handle potential errors gracefully.Apply this diff to add error handling:
-await ingestAuditLogsTinybird([ +try { + await ingestAuditLogsTinybird([ ...auditLogs, newAuditLogEntry, ]); +} catch (error) { + // Handle the error, e.g., log it or send to an error tracking service + console.error('Failed to ingest audit logs to Tinybird:', error); +}📝 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.try { await ingestAuditLogsTinybird([ ...auditLogs, newAuditLogEntry, ]); } catch (error) { // Handle the error, e.g., log it or send to an error tracking service console.error('Failed to ingest audit logs to Tinybird:', error); }
69-84: 🛠️ Refactor suggestion
Refactor error handling within the transaction
Currently, the error handling for the insert operation uses a
.catchmethod chained to the query. In anasync/awaitcontext, it's more idiomatic and clear to use atry...catchblock for error handling. This enhances readability and maintainability.Apply this diff to refactor the error handling:
-await tx - .insert(schema.keysPermissions) - .values({ - keyId: rootKey.id, - permissionId: p.id, - workspaceId: p.workspaceId, - }) - .onDuplicateKeyUpdate({ set: { permissionId: p.id } }) - .catch((_err) => { - throw new TRPCError({ - code: "INTERNAL_SERVER_ERROR", - message: - "We are unable to add permission to the root key. Please contact support using support@unkey.dev.", - }); - }); +try { + await tx + .insert(schema.keysPermissions) + .values({ + keyId: rootKey.id, + permissionId: p.id, + workspaceId: p.workspaceId, + }) + .onDuplicateKeyUpdate({ set: { permissionId: p.id } }); +} catch (_err) { + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: + "We are unable to add permission to the root key. Please contact support using support@unkey.dev.", + }); +}📝 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.await db.transaction(async (tx) => { try { await tx .insert(schema.keysPermissions) .values({ keyId: rootKey.id, permissionId: p.id, workspaceId: p.workspaceId, }) .onDuplicateKeyUpdate({ set: { permissionId: p.id } }); } catch (_err) { throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: "We are unable to add permission to the root key. Please contact support using support@unkey.dev.", }); } });
85-109: 🛠️ Refactor suggestion
Consolidate audit log entries to avoid duplication
The audit log entry added to
insertAuditLogsand the one passed toingestAuditLogsTinybirdare identical. To improve maintainability and reduce the risk of inconsistencies, consider defining the audit log entry once and reusing it in both places.Apply this diff to refactor the code:
+const newAuditLogEntry = { + workspaceId: workspace.id, + actor: { type: "user", id: ctx.user.id }, + event: "authorization.connect_permission_and_key", + description: `Attached ${p.id} to ${rootKey.id}`, + resources: [ + { + type: "key", + id: rootKey.id, + }, + { + type: "permission", + id: p.id, + }, + ], + context: { + location: ctx.audit.location, + userAgent: ctx.audit.userAgent, + }, +}; await insertAuditLogs(tx, [ ...auditLogs, - { - workspaceId: workspace.id, - actor: { type: "user", id: ctx.user.id }, - event: "authorization.connect_permission_and_key", - description: `Attached ${p.id} to ${rootKey.id}`, - resources: [ - { - type: "key", - id: rootKey.id, - }, - { - type: "permission", - id: p.id, - }, - ], - context: { - location: ctx.audit.location, - userAgent: ctx.audit.userAgent, - }, - }, + newAuditLogEntry, ]); await ingestAuditLogsTinybird([ ...auditLogs, - { - workspaceId: workspace.id, - actor: { type: "user", id: ctx.user.id }, - event: "authorization.connect_permission_and_key", - description: `Attached ${p.id} to ${rootKey.id}`, - resources: [ - { - type: "key", - id: rootKey.id, - }, - { - type: "permission", - id: p.id, - }, - ], - context: { - location: ctx.audit.location, - userAgent: ctx.audit.userAgent, - }, - }, + newAuditLogEntry, ]);📝 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 newAuditLogEntry = { workspaceId: workspace.id, actor: { type: "user", id: ctx.user.id }, event: "authorization.connect_permission_and_key", description: `Attached ${p.id} to ${rootKey.id}`, resources: [ { type: "key", id: rootKey.id, }, { type: "permission", id: p.id, }, ], context: { location: ctx.audit.location, userAgent: ctx.audit.userAgent, }, }; await insertAuditLogs(tx, [ ...auditLogs, newAuditLogEntry, ]); }); await ingestAuditLogsTinybird([ ...auditLogs, newAuditLogEntry, ]);apps/dashboard/lib/trpc/routers/ratelimit/createOverride.ts (3)
4-4: 🛠️ Refactor suggestion
Inconsistent naming conventions for audit log functions
The functions
insertAuditLogsandingestAuditLogsTinybirduse different verbs (insertvs.ingest) and naming patterns. For clarity and consistency, consider standardizing the naming convention to improve readability and maintainability.Also applies to: 6-6
84-106: 🛠️ Refactor suggestion
Duplicate audit log data structures
The audit log data constructed at lines 85-106 and 117-128 is identical. To adhere to the DRY (Don't Repeat Yourself) principle, consider extracting this data into a shared variable or helper function.
Refactored code example:
+ const auditLogData = { + workspaceId: namespace.workspace.id, + actor: { + type: "user", + id: ctx.user.id, + }, + event: "ratelimitOverride.create", + description: `Created ${input.identifier}`, + resources: [ + { + type: "ratelimitNamespace", + id: input.namespaceId, + }, + { + type: "ratelimitOverride", + id, + }, + ], + context: { + location: ctx.audit.location, + userAgent: ctx.audit.userAgent, + }, + }; await db .transaction(async (tx) => { - await insertAuditLogs(tx, { - workspaceId: namespace.workspace.id, - actor: { - type: "user", - id: ctx.user.id, - }, - event: "ratelimitOverride.create", - description: `Created ${input.identifier}`, - resources: [ - { - type: "ratelimitNamespace", - id: input.namespaceId, - }, - { - type: "ratelimitOverride", - id, - }, - ], - context: { - location: ctx.audit.location, - userAgent: ctx.audit.userAgent, - }, - }); + await insertAuditLogs(tx, auditLogData); }) .catch((_err) => { // Error handling code }); - await ingestAuditLogsTinybird({ - workspaceId: namespace.workspace.id, - actor: { - type: "user", - id: ctx.user.id, - }, - event: "ratelimitOverride.create", - description: `Created ${input.identifier}`, - resources: [ - { - type: "ratelimitNamespace", - id: input.namespaceId, - }, - { - type: "ratelimitOverride", - id, - }, - ], - context: { - location: ctx.audit.location, - userAgent: ctx.audit.userAgent, - }, - }); + await ingestAuditLogsTinybird(auditLogData);Also applies to: 116-129
84-106:
⚠️ Potential issueLack of error handling for
insertAuditLogsThe
insertAuditLogsfunction within the transaction does not have specific error handling. If this function throws an error, it could cause the entire transaction to fail silently.Consider adding error handling to manage potential exceptions:
await db .transaction(async (tx) => { // Previous code... + try { + await insertAuditLogs(tx, auditLogData); + } catch (error) { + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: "Failed to insert audit logs.", + }); + } }) .catch((_err) => { // Existing error handling });Committable suggestion was skipped due to low confidence.
apps/dashboard/lib/trpc/routers/rbac/removePermissionFromRootKey.ts (3)
103-107: 🛠️ Refactor suggestion
Handle transaction errors more gracefully
In the catch block, you are throwing a generic
INTERNAL_SERVER_ERROR. Consider providing more context or logging additional details to aid in debugging, while still avoiding exposing sensitive information to the end-user.
62-64:
⚠️ Potential issueEnsure
key.permissionsis defined before accessingWhen accessing
key.permissions.find(...), there's a potential risk ifkey.permissionsisundefinedornull. To prevent runtime errors, ensure thatkey.permissionsis always defined.Consider adding a null check:
- const permissionRelation = key.permissions.find( + const permissionRelation = key.permissions?.find( (kp) => kp.permission.name === input.permissionName, );📝 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 permissionRelation = key.permissions?.find( (kp) => kp.permission.name === input.permissionName, );
103-103: 🛠️ Refactor suggestion
Use a structured logger instead of
console.errorIn the catch block of the transaction, you're using
console.error(err);to log errors. In a production environment, it's recommended to use a structured logging library or utility for better log management, filtering, and monitoring.Apply this diff to replace
console.errorwith a structured logger:- console.error(err); + logger.error('Transaction failed in removePermissionFromRootKey', { error: err });Assuming you have a
loggerutility configured in your application.Committable suggestion was skipped due to low confidence.
apps/dashboard/lib/trpc/routers/key/updateRemaining.ts (2)
67-89: 🛠️ Refactor suggestion
Consolidate audit log data to reduce duplication
The data passed to
insertAuditLogsandingestAuditLogsTinybirdis nearly identical. To adhere to the DRY (Don't Repeat Yourself) principle, consider creating a shared audit log object and reusing it in both function calls. This makes the code more maintainable and reduces the risk of inconsistencies.Here's how you might implement this:
const auditLog = { workspaceId: key.workspace.id, actor: { type: "user", id: ctx.user.id, }, event: "key.update", description: input.limitEnabled ? `Changed remaining for ${key.id} to remaining=${input.remaining}, refill=${ input.refill ? `${input.refill.amount}@${input.refill.interval}` : "none" }` : `Disabled limit for ${key.id}`, resources: [ { type: "key", id: key.id, }, ], context: { location: ctx.audit.location, userAgent: ctx.audit.userAgent, }, }; await insertAuditLogs(tx, auditLog); }) .catch((err) => { // ... existing error handling ... }); + await ingestAuditLogsTinybird(auditLog);Also applies to: 90-112
90-112:
⚠️ Potential issueMove
ingestAuditLogsTinybirdcall outside the database transactionCalling external services like
ingestAuditLogsTinybirdwithin a database transaction can lead to prolonged transaction times and potential locking issues, as the transaction remains open during the external operation. This can negatively impact database performance and scalability. Consider moving the call toingestAuditLogsTinybirdoutside the transaction block to avoid holding the transaction open during external operations.To implement this change, you can modify the code as follows:
- Declare the
keyvariable outside the transaction scope.- Assign
keyinside the transaction.- After the transaction completes, call
ingestAuditLogsTinybirdwith the required data.Apply this diff:
+ let key; await db .transaction(async (tx) => { - const key = await tx.query.keys.findFirst({ + key = await tx.query.keys.findFirst({ // ... existing code ... }); // ... existing code ... await insertAuditLogs(tx, { // ... existing code ... }); }) .catch((err) => { // ... existing error handling ... }); + await ingestAuditLogsTinybird({ + workspaceId: key.workspace.id, + actor: { + type: "user", + id: ctx.user.id, + }, + event: "key.update", + description: input.limitEnabled + ? `Changed remaining for ${key.id} to remaining=${input.remaining}, refill=${ + input.refill ? `${input.refill.amount}@${input.refill.interval}` : "none" + }` + : `Disabled limit for ${key.id}`, + resources: [ + { + type: "key", + id: key.id, + }, + ], + context: { + location: ctx.audit.location, + userAgent: ctx.audit.userAgent, + }, + });Committable suggestion was skipped due to low confidence.
apps/dashboard/lib/trpc/routers/api/delete.ts (1)
50-68: 🛠️ Refactor suggestion
Refactor duplicated audit logging code
The blocks of code for
insertAuditLogsandingestAuditLogsTinybirdin lines 50-68 and 99-124 are nearly identical, differing mainly in the event type and resources.Consider refactoring to reduce duplication by creating a helper function that accepts parameters for the differing parts. This will enhance maintainability and reduce the risk of inconsistencies.
Also applies to: 99-124
apps/dashboard/lib/trpc/routers/ratelimit/deleteNamespace.ts (3)
52-52:
⚠️ Potential issuePossible inconsistency in accessing
workspaceIdAt line 52,
workspaceIdis accessed asnamespace.workspaceId, whereas later (line 110), it's accessed asnamespace.workspace.id. Verify thatnamespace.workspaceIdandnamespace.workspace.idrefer to the same value. Using consistent property access improves code readability and reduces the risk of bugs.
70-70:
⚠️ Potential issueInconsistent error handling for
ingestAuditLogsTinybirdThe call to
ingestAuditLogsTinybirdat line 70 lacks error handling, while a similar call at line 133 includes a.catchblock that rolls back the transaction upon failure. This inconsistency could lead to unhandled exceptions or partial data updates if the ingest operation fails at line 70. Consider adding error handling to ensure consistent behavior and data integrity.
110-110:
⚠️ Potential issueConsistency in accessing
workspaceIdAt line 110,
workspaceIdis accessed asnamespace.workspace.id, whereas earlier (line 52), it'snamespace.workspaceId. Ensure that both references are correct and refer to the same value. Consistent property access enhances clarity and helps prevent potential bugs.apps/dashboard/lib/trpc/routers/key/create.ts (1)
112-127:
⚠️ Potential issuePotential Duplication of Audit Logs
The function
insertAuditLogsis called within the transaction at lines 112-127, andingestAuditLogsTinybirdis called after the transaction at lines 137-149 with the same audit log data. This may result in duplicate audit entries. Consider consolidating the audit logging to prevent redundancy.Also applies to: 137-149
apps/dashboard/lib/trpc/routers/rbac/createRole.ts (3)
55-60: 🛠️ Refactor suggestion
Simplify error handling within the transaction
Currently, the error handling for the
tx.insert(schema.roles)operation uses a.catch()block to throw aTRPCError. Since any unhandled exception within the transaction will automatically trigger a rollback, you might consider allowing errors to propagate naturally without the.catch()block. This simplifies the code and ensures that database errors are properly reported.Alternatively, if you need custom error handling, you could wrap the entire transaction in a
try-catchblock to manage exceptions at a higher level.
82-102: 🛠️ Refactor suggestion
Avoid external network calls inside database transactions to improve performance
Calling
ingestAuditLogsTinybirdwithin the database transaction may prolong the transaction duration and potentially block database resources, especially if the external call experiences latency or fails. This can negatively impact database performance and concurrency.Consider moving the
ingestAuditLogsTinybirdcall outside of the transaction block to prevent external dependencies from affecting your database operations.
135-157: 🛠️ Refactor suggestion
Avoid external network calls inside database transactions to improve performance
Similarly, the
ingestAuditLogsTinybirdcalls within the permissions handling are placed inside the transaction. To enhance performance and reduce the risk of long-running transactions, it's advisable to move these external calls outside of the transaction block.apps/dashboard/lib/trpc/routers/key/updateRatelimit.ts (3)
51-53:
⚠️ Potential issueProvide more descriptive error messages
The error message
"Invalid input."is generic and may not be helpful for users to correct their input. Consider providing more specific information about which input is invalid or what is expected.
64-88: 🛠️ Refactor suggestion
Refactor duplicate audit log data construction
The audit log data constructed for
insertAuditLogsandingestAuditLogsTinybirdis duplicated in bothifandelseblocks. To enhance maintainability and reduce the risk of inconsistent data, consider constructing the audit log data once and reusing it for both functions.Here's how you might refactor the code:
+const auditLogData = { + workspaceId: key.workspace.id, + actor: { + type: "user", + id: ctx.user.id, + }, + event: "key.update", + description: input.enabled + ? `Changed ratelimit of ${key.id}` + : `Disabled ratelimit of ${key.id}`, + resources: [ + { + type: "key", + id: key.id, + ...(input.enabled && { + meta: { + "ratelimit.async": input.ratelimitAsync, + "ratelimit.limit": input.ratelimitLimit, + "ratelimit.duration": input.ratelimitDuration, + }, + }), + }, + ], + context: { + location: ctx.audit.location, + userAgent: ctx.audit.userAgent, + }, +}; if (input.enabled) { // validation logic - await db.transaction(async (tx) => { + await db.transaction(async (tx) => { await tx .update(schema.keys) .set({ ratelimitAsync, ratelimitLimit, ratelimitDuration, }) .where(eq(schema.keys.id, key.id)); - await insertAuditLogs(tx, { - // audit log data - }); + await insertAuditLogs(tx, auditLogData); - }); + }).catch((err) => { + // error handling + }); - await ingestAuditLogsTinybird({ - // audit log data - }).catch((err) => { + await ingestAuditLogsTinybird(auditLogData).catch((err) => { // error handling }); } else { await db .transaction(async (tx) => { await tx .update(schema.keys) .set({ ratelimitAsync: null, ratelimitLimit: null, ratelimitDuration: null, }) .where(eq(schema.keys.id, key.id)); - await insertAuditLogs(tx, { - // audit log data - }); + await insertAuditLogs(tx, auditLogData); }) .catch((err) => { // error handling }); - await ingestAuditLogsTinybird({ - // audit log data - }); + await ingestAuditLogsTinybird(auditLogData).catch((err) => { + // error handling + }); }Also applies to: 133-151, 90-119, 162-175
55-88:
⚠️ Potential issueAdd error handling to the transaction
The transaction block starting at line 55 does not have error handling attached to it. In contrast, the transaction in the
elseblock includes a.catchto handle errors. For consistency and to ensure that any errors during the transaction are properly caught and handled, consider adding a.catchblock.Apply this diff to add error handling:
await db.transaction(async (tx) => { // transaction operations -}); +}).catch((err) => { + console.error(err); + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: + "We were unable to update ratelimit on this key. Please contact support using support@unkey.dev", + }); +});📝 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.await db.transaction(async (tx) => { await tx .update(schema.keys) .set({ ratelimitAsync, ratelimitLimit, ratelimitDuration, }) .where(eq(schema.keys.id, key.id)); await insertAuditLogs(tx, { workspaceId: key.workspace.id, actor: { type: "user", id: ctx.user.id, }, event: "key.update", description: `Changed ratelimit of ${key.id}`, resources: [ { type: "key", id: key.id, meta: { "ratelimit.async": ratelimitAsync, "ratelimit.limit": ratelimitLimit, "ratelimit.duration": ratelimitDuration, }, }, ], context: { location: ctx.audit.location, userAgent: ctx.audit.userAgent, }, }); }).catch((err) => { console.error(err); throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: "We were unable to update ratelimit on this key. Please contact support using support@unkey.dev", }); });apps/dashboard/app/new/page.tsx (3)
220-253:
⚠️ Potential issuePotential inconsistency:
ingestAuditLogsTinybirdis called outside the transactionThe workspace creation and database audit log insertion are within a transaction, but
ingestAuditLogsTinybirdis called outside the transaction. If the transaction fails,ingestAuditLogsTinybirdwill still execute, possibly resulting in audit logs without a corresponding workspace. Consider movingingestAuditLogsTinybirdinside the transaction to ensure atomicity.
233-254: 🛠️ Refactor suggestion
DRY Principle: Reuse audit log data to avoid duplication
The audit log data passed to both
insertAuditLogsandingestAuditLogsTinybirdis identical. To adhere to the DRY principle, consider preparing the audit log data once, assigning it to a variable, and reusing it in both calls.
250-250:
⚠️ Potential issueSecurity concern: Validate
x-forwarded-forheader to prevent spoofingThe
x-forwarded-forheader can be manipulated by clients, potentially leading to incorrect location data. Consider using a trusted source for the client's IP address, such as middleware or a service that reliably provides this information.apps/dashboard/lib/trpc/routers/vercel.ts (6)
78-97: 🛠️ Refactor suggestion
Consider encapsulating audit logging to reduce code duplication
The code block between lines 78-97 and similar blocks throughout the file repeat the same logic for logging audit events using
insertAuditLogsandingestAuditLogsTinybird. To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, consider creating a helper function that handles both logging mechanisms.Apply this diff to introduce a helper function:
+// Define a helper function for audit logging +async function logAuditEvent(tx, params) { + await insertAuditLogs(tx, params); + await ingestAuditLogsTinybird(params); +} - await insertAuditLogs(tx, { - workspaceId: integration.workspace.id, - actor: { type: "user", id: ctx.user.id }, - event: "key.create", - description: `Created ${keyId}`, - resources: [ - { - type: "key", - id: keyId, - }, - { - type: "vercelIntegration", - id: integration.id, - }, - ], - context: { - location: ctx.audit.location, - userAgent: ctx.audit.userAgent, - }, - }); - await ingestAuditLogsTinybird({ - workspaceId: integration.workspace.id, - actor: { type: "user", id: ctx.user.id }, - event: "key.create", - description: `Created ${keyId}`, - resources: [ - { - type: "key", - id: keyId, - }, - { - type: "vercelIntegration", - id: integration.id, - }, - ], - context: { - location: ctx.audit.location, - userAgent: ctx.audit.userAgent, - }, - }); + await logAuditEvent(tx, { + workspaceId: integration.workspace.id, + actor: { type: "user", id: ctx.user.id }, + event: "key.create", + description: `Created ${keyId}`, + resources: [ + { + type: "key", + id: keyId, + }, + { + type: "vercelIntegration", + id: integration.id, + }, + ], + context: { + location: ctx.audit.location, + userAgent: ctx.audit.userAgent, + }, + });Committable suggestion was skipped due to low confidence.
299-322: 🛠️ Refactor suggestion
Refactor audit logging to use the helper function
The code between lines 299-322 replicates audit logging logic. Applying the
logAuditEventhelper function enhances code maintainability.Apply this diff:
- await insertAuditLogs(tx, { - workspaceId: integration.workspace.id, - actor: { type: "user", id: ctx.user.id }, - event: "vercelBinding.update", - description: `Updated ${existingBinding.id}`, - resources: [ - { - type: "vercelBinding", - id: existingBinding.id, - meta: { - vercelEnvironment: res.val.created.id, - }, - }, - { - type: "api", - id: input.apiId, - }, - ], - context: { - location: ctx.audit.location, - userAgent: ctx.audit.userAgent, - }, - }); - await ingestAuditLogsTinybird({ - workspaceId: integration.workspace.id, - actor: { type: "user", id: ctx.user.id }, - event: "vercelBinding.update", - description: `Updated ${existingBinding.id}`, - resources: [ - { - type: "vercelBinding", - id: existingBinding.id, - meta: { - vercelEnvironment: res.val.created.id, - }, - }, - { - type: "api", - id: input.apiId, - }, - ], - context: { - location: ctx.audit.location, - userAgent: ctx.audit.userAgent, - }, - }); + await logAuditEvent(tx, { + workspaceId: integration.workspace.id, + actor: { type: "user", id: ctx.user.id }, + event: "vercelBinding.update", + description: `Updated ${existingBinding.id}`, + resources: [ + { + type: "vercelBinding", + id: existingBinding.id, + meta: { + vercelEnvironment: res.val.created.id, + }, + }, + { + type: "api", + id: input.apiId, + }, + ], + context: { + location: ctx.audit.location, + userAgent: ctx.audit.userAgent, + }, + });📝 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.await logAuditEvent(tx, { workspaceId: integration.workspace.id, actor: { type: "user", id: ctx.user.id }, event: "vercelBinding.update", description: `Updated ${existingBinding.id}`, resources: [ { type: "vercelBinding", id: existingBinding.id, meta: { vercelEnvironment: res.val.created.id, }, }, { type: "api", id: input.apiId, }, ], context: { location: ctx.audit.location, userAgent: ctx.audit.userAgent, }, });
567-595: 🛠️ Refactor suggestion
Incorporate the helper function for audit logging
Lines 567-595 contain duplicate audit logging code. Refactoring with the
logAuditEventfunction enhances code quality.Apply this diff:
- await insertAuditLogs(tx, { - workspaceId: integration.workspace.id, - actor: { type: "user", id: ctx.user.id }, - event: "vercelBinding.create", - description: `Created ${vercelBindingId} for ${keyId}`, - resources: [ - { - type: "vercelIntegration", - id: integration.id, - }, - { - type: "vercelBinding", - id: vercelBindingId, - meta: { - environment: input.environment, - projectId: input.projectId, - }, - }, - { - type: "key", - id: keyId, - }, - ], - context: { - location: ctx.audit.location, - userAgent: ctx.audit.userAgent, - }, - }); - await ingestAuditLogsTinybird({ - workspaceId: integration.workspace.id, - actor: { type: "user", id: ctx.user.id }, - event: "vercelBinding.create", - description: `Created ${vercelBindingId} for ${keyId}`, - resources: [ - { - type: "vercelIntegration", - id: integration.id, - }, - { - type: "vercelBinding", - id: vercelBindingId, - meta: { - environment: input.environment, - projectId: input.projectId, - }, - }, - { - type: "key", - id: keyId, - }, - ], - context: { - location: ctx.audit.location, - userAgent: ctx.audit.userAgent, - }, - }); + await logAuditEvent(tx, { + workspaceId: integration.workspace.id, + actor: { type: "user", id: ctx.user.id }, + event: "vercelBinding.create", + description: `Created ${vercelBindingId} for ${keyId}`, + resources: [ + { + type: "vercelIntegration", + id: integration.id, + }, + { + type: "vercelBinding", + id: vercelBindingId, + meta: { + environment: input.environment, + projectId: input.projectId, + }, + }, + { + type: "key", + id: keyId, + }, + ], + context: { + location: ctx.audit.location, + userAgent: ctx.audit.userAgent, + }, + });📝 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.await logAuditEvent(tx, { workspaceId: integration.workspace.id, actor: { type: "user", id: ctx.user.id }, event: "vercelBinding.create", description: `Created ${vercelBindingId} for ${keyId}`, resources: [ { type: "vercelIntegration", id: integration.id, }, { type: "vercelBinding", id: vercelBindingId, meta: { environment: input.environment, projectId: input.projectId, }, }, { type: "key", id: keyId, }, ], context: { location: ctx.audit.location, userAgent: ctx.audit.userAgent, }, });
442-458: 🛠️ Refactor suggestion
Refactor audit logging using the helper function
Between lines 442-458, audit logging code is repeated. Utilize the
logAuditEventhelper function to streamline the logic.Apply this diff:
- await insertAuditLogs(tx, { - workspaceId: integration.workspace.id, - actor: { type: "user", id: ctx.user.id }, - event: "key.create", - description: `Created ${keyId}`, - resources: [ - { - type: "key", - id: keyId, - }, - ], - context: { - location: ctx.audit.location, - userAgent: ctx.audit.userAgent, - }, - }); - await ingestAuditLogsTinybird({ - workspaceId: integration.workspace.id, - actor: { type: "user", id: ctx.user.id }, - event: "key.create", - description: `Created ${keyId}`, - resources: [ - { - type: "key", - id: keyId, - }, - ], - context: { - location: ctx.audit.location, - userAgent: ctx.audit.userAgent, - }, - }); + await logAuditEvent(tx, { + workspaceId: integration.workspace.id, + actor: { type: "user", id: ctx.user.id }, + event: "key.create", + description: `Created ${keyId}`, + resources: [ + { + type: "key", + id: keyId, + }, + ], + context: { + location: ctx.audit.location, + userAgent: ctx.audit.userAgent, + }, + });Committable suggestion was skipped due to low confidence.
503-526: 🛠️ Refactor suggestion
Ensure consistency in audit logging by refactoring
Lines 503-526 show repeated audit logging code. Use the
logAuditEventhelper function for consistency and to reduce duplication.Apply this diff:
- await insertAuditLogs(tx, { - workspaceId: integration.workspace.id, - actor: { type: "user", id: ctx.user.id }, - event: "vercelBinding.update", - description: `Updated ${existingBinding.id}`, - resources: [ - { - type: "vercelBinding", - id: existingBinding.id, - meta: { - vercelEnvironment: res.val.created.id, - }, - }, - { - type: "key", - id: keyId, - }, - ], - context: { - location: ctx.audit.location, - userAgent: ctx.audit.userAgent, - }, - }); - await ingestAuditLogsTinybird({ - workspaceId: integration.workspace.id, - actor: { type: "user", id: ctx.user.id }, - event: "vercelBinding.update", - description: `Updated ${existingBinding.id}`, - resources: [ - { - type: "vercelBinding", - id: existingBinding.id, - meta: { - vercelEnvironment: res.val.created.id, - }, - }, - { - type: "key", - id: keyId, - }, - ], - context: { - location: ctx.audit.location, - userAgent: ctx.audit.userAgent, - }, - }); + await logAuditEvent(tx, { + workspaceId: integration.workspace.id, + actor: { type: "user", id: ctx.user.id }, + event: "vercelBinding.update", + description: `Updated ${existingBinding.id}`, + resources: [ + { + type: "vercelBinding", + id: existingBinding.id, + meta: { + vercelEnvironment: res.val.created.id, + }, + }, + { + type: "key", + id: keyId, + }, + ], + context: { + location: ctx.audit.location, + userAgent: ctx.audit.userAgent, + }, + });📝 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.await logAuditEvent(tx, { workspaceId: integration.workspace.id, actor: { type: "user", id: ctx.user.id }, event: "vercelBinding.update", description: `Updated ${existingBinding.id}`, resources: [ { type: "vercelBinding", id: existingBinding.id, meta: { vercelEnvironment: res.val.created.id, }, }, { type: "key", id: keyId, }, ], context: { location: ctx.audit.location, userAgent: ctx.audit.userAgent, }, });
148-168: 🛠️ Refactor suggestion
Refactor repeated audit logging code using the helper function
Similar to the previous suggestion, the code between lines 148-168 duplicates audit logging logic. Utilize the proposed
logAuditEventhelper function to streamline the code and enhance readability.Apply this diff to use the helper function:
- await insertAuditLogs(tx, { - workspaceId: integration.workspace.id, - actor: { type: "user", id: ctx.user.id }, - event: "vercelBinding.create", - description: `Created ${vercelBindingId} for ${keyId}`, - resources: [ - { - type: "vercelBinding", - id: vercelBindingId, - }, - { - type: "key", - id: keyId, - }, - ], - context: { - location: ctx.audit.location, - userAgent: ctx.audit.userAgent, - }, - }); - await ingestAuditLogsTinybird({ - workspaceId: integration.workspace.id, - actor: { type: "user", id: ctx.user.id }, - event: "vercelBinding.create", - description: `Created ${vercelBindingId} for ${keyId}`, - resources: [ - { - type: "vercelBinding", - id: vercelBindingId, - }, - { - type: "key", - id: keyId, - }, - ], - context: { - location: ctx.audit.location, - userAgent: ctx.audit.userAgent, - }, - }); + await logAuditEvent(tx, { + workspaceId: integration.workspace.id, + actor: { type: "user", id: ctx.user.id }, + event: "vercelBinding.create", + description: `Created ${vercelBindingId} for ${keyId}`, + resources: [ + { + type: "vercelBinding", + id: vercelBindingId, + }, + { + type: "key", + id: keyId, + }, + ], + context: { + location: ctx.audit.location, + userAgent: ctx.audit.userAgent, + }, + });Committable suggestion was skipped due to low confidence.
apps/dashboard/lib/trpc/routers/rbac.ts (9)
783-812:
⚠️ Potential issueMissing Tinybird ingestion after deleting a permission
In the
deletePermissionmethod, audit logs are inserted but not ingested into Tinybird, which could lead to missing audit data.Recommendation:
Add
ingestAuditLogsTinybirdafter the transaction to ensure the audit logs are properly ingested.Committable suggestion was skipped due to low confidence.
209-236:
⚠️ Potential issueEnsure audit logs are ingested into Tinybird
In the
connectPermissionToRolemethod, the audit logs are inserted into the database, butingestAuditLogsTinybirdis not called to ingest them into Tinybird. This could lead to missing audit entries in your analytics.Recommendation:
After the transaction, call
ingestAuditLogsTinybirdto ensure the audit logs are properly ingested.Committable suggestion was skipped due to low confidence.
256-286:
⚠️ Potential issueMissing Tinybird ingestion after disconnecting permission from role
In the
disconnectPermissionToRolemethod, audit logs are inserted into the database, but there's no call toingestAuditLogsTinybird. This could result in the audit logs not being reflected in Tinybird.Recommendation:
Add
ingestAuditLogsTinybirdafter the transaction to ingest the audit logs into Tinybird.Committable suggestion was skipped due to low confidence.
131-161:
⚠️ Potential issueMissing ingestion of audit logs into Tinybird
In the
removePermissionFromRootKeymethod, after inserting audit logs into the database usinginsertAuditLogs, there is no corresponding call toingestAuditLogsTinybirdto ingest these logs into Tinybird. This omission may result in incomplete audit data in Tinybird.Recommendation:
Add a call to
ingestAuditLogsTinybirdafter the transaction to ensure audit logs are ingested into Tinybird.Committable suggestion was skipped due to low confidence.
334-361:
⚠️ Potential issueEnsure audit logs are ingested into Tinybird
In the
connectRoleToKeymethod, after inserting audit logs into the database,ingestAuditLogsTinybirdis not called. This omission may cause incomplete audit data in Tinybird.Recommendation:
Include a call to
ingestAuditLogsTinybirdafter the transaction.Committable suggestion was skipped due to low confidence.
71-82:
⚠️ Potential issueEnsure atomicity by moving Tinybird ingestion inside the transaction
In the
addPermissionToRootKeymethod, the call toingestAuditLogsTinybird(auditLogs);is placed outside the transaction block. If the transaction fails but the audit logs are still ingested into Tinybird, it could lead to inconsistencies between your database and audit logs.Recommendation:
Move the
ingestAuditLogsTinybirdcall inside the transaction to ensure that audit logging is atomic with the database operations....Committable suggestion was skipped due to low confidence.
550-567:
⚠️ Potential issueMissing Tinybird ingestion after updating a role
In the
updateRolemethod, the audit logs are inserted into the database, but they are not ingested into Tinybird. This could lead to inconsistencies in your audit data.Recommendation:
Add a call to
ingestAuditLogsTinybirdafter the transaction to ingest the audit logs.Committable suggestion was skipped due to low confidence.
723-752:
⚠️ Potential issueMissing Tinybird ingestion after updating a permission
The
updatePermissionmethod inserts audit logs into the database without ingesting them into Tinybird. This may result in incomplete audit records in Tinybird.Recommendation:
Call
ingestAuditLogsTinybirdafter the transaction to ingest the audit logs.Committable suggestion was skipped due to low confidence.
598-619:
⚠️ Potential issueMissing Tinybird ingestion after deleting a role
In the
deleteRolemethod, audit logs are inserted into the database but not ingested into Tinybird, potentially causing missing entries in your audit analytics.Recommendation:
Include
ingestAuditLogsTinybirdafter the transaction to ensure audit logs are ingested.Committable suggestion was skipped due to low confidence.
apps/dashboard/lib/trpc/routers/key/updateRootKeyName.ts (1)
73-73:
⚠️ Potential issueHandle potential null values in audit log description
If
input.nameisnullorundefined, the description in the audit logs will readChanged name of ${key.id} to null, which may be confusing. Consider adjusting the description to handle null or undefined values appropriately.Suggested change:
- description: `Changed name of ${key.id} to ${input.name}`, + description: input.name + ? `Changed name of ${key.id} to ${input.name}` + : `Removed name of ${key.id}`,📝 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.description: input.name ? `Changed name of ${key.id} to ${input.name}` : `Removed name of ${key.id}`,apps/dashboard/lib/trpc/routers/workspace/changePlan.ts (2)
141-156: 🛠️ Refactor suggestion
DRY Principle: Extract Repeated Audit Logging Code
The code for inserting audit logs is repeated multiple times. To adhere to the DRY (Don't Repeat Yourself) principle, consider creating a helper function to handle audit logging.
Create a utility function to handle audit log insertion:
function logWorkspaceUpdate(tx, ctx, workspaceId, description) { return insertAuditLogs(tx, { workspaceId, actor: { type: "user", id: ctx.user.id }, event: "workspace.update", description, resources: [{ type: "workspace", id: workspaceId }], context: { location: ctx.audit.location, userAgent: ctx.audit.userAgent, }, }); }Then replace the audit logging calls with:
- await insertAuditLogs(tx, { - // parameters - }); + await logWorkspaceUpdate(tx, ctx, workspace.id, "Requested downgrade to 'free'");
114-115:
⚠️ Potential issueAvoid Logging Sensitive Information
Logging the entire error object may inadvertently expose sensitive information. Consider logging only the necessary details or using a logging utility that sanitizes sensitive data.
Apply this diff to log only the error message:
- console.error(err); + console.error('Transaction error in changeWorkspacePlan:', err.message);📝 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..catch((err) => { console.error('Transaction error in changeWorkspacePlan:', err.message);
…keyed#2134) * feat: mostly just duplicate audit logs and other clickhouse stuff * chore: audit logs for trpc
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores