Use one-time use tokens when triggering or batch triggering from the frontend #1515
Conversation
WalkthroughThe changes in this pull request enhance the API's functionality by integrating a one-time use token mechanism across various routes and services. Key modifications include updates to authorization actions, the introduction of new methods for token generation, and adjustments to the handling of task runs, particularly in relation to error management and validation. Additionally, database migrations and schema updates reflect these enhancements, ensuring that the new token features are supported at both the application and database levels. Changes
Possibly related PRs
Suggested reviewers
Poem
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: 9
🧹 Outside diff range and nitpick comments (16)
apps/webapp/app/routes/api.v1.tasks.$taskId.trigger.ts (1)
Line range hint
62-84: Consider implementing token expiration monitoringWhile the one-time use token implementation is solid, consider adding monitoring for token usage patterns and failed attempts to detect potential security issues.
Suggestions:
- Add metrics for token generation and usage
- Set up alerts for unusual patterns of token failures
- Consider implementing rate limiting for token generation
apps/webapp/app/routes/api.v1.tasks.batch.ts (2)
59-60: Consider adding explicit error handling for token generation.While the function has general error handling, consider adding specific handling for token generation failures.
- const oneTimeUseToken = await getOneTimeUseToken(authentication); + let oneTimeUseToken; + try { + oneTimeUseToken = await getOneTimeUseToken(authentication); + } catch (error) { + logger.error("Failed to generate one-time use token", { + error: { message: error.message, stack: error.stack }, + }); + return json( + { error: "Failed to generate authorization token" }, + { status: 500 } + ); + }
Line range hint
59-91: Consider implementing token expiration monitoring.While the one-time use token implementation looks solid, consider adding monitoring or metrics for token usage and potential failures. This would help track:
- Token generation failures
- Token usage patterns
- Invalid or expired token attempts
references/v3-catalog/src/management.ts (2)
290-415: Consider enhancing test coverage and error validationWhile the function comprehensively tests various authentication scenarios, consider these improvements:
- Add test cases for concurrent token usage to verify token invalidation
- Add assertions for specific error messages instead of just logging them
- Consider using a structured logging approach for better CI debugging
Example improvement for error validation:
} catch (error) { - console.error(error); + const expectedError = error as Error; + if (!expectedError.message.includes("Token has already been used")) { + throw new Error(`Unexpected error message: ${expectedError.message}`); + } + console.log("✓ Received expected token reuse error"); }
423-424: Consider maintaining both test executionsWhile testing the new token functionality is important, consider keeping the
doRescheduleRuntest active to maintain test coverage. You could run both tests sequentially.-// doRescheduleRun().catch(console.error); -doOneTimeUseTrigger().catch(console.error); +async function runAllTests() { + await doRescheduleRun().catch(console.error); + await doOneTimeUseTrigger().catch(console.error); +} + +runAllTests();apps/webapp/test/authorization.test.ts (1)
56-76: Consider adding more edge cases for comprehensive coverage.Consider adding these test cases to strengthen the test suite:
- Testing write permissions for multiple tasks
- Verifying behavior with other actions (read/delete)
Example test case:
it("should handle multiple tasks with mixed permissions", () => { const result = checkAuthorization(publicJwtEntityWithTaskWritePermissions, "write", { tasks: ["task-1", "task-2"], }); expect(result.authorized).toBe(false); if (!result.authorized) { expect(result.reason).toContain("Public Access Token is missing required permissions"); } });apps/webapp/app/services/routeBuilders/apiBuilder.server.ts (1)
573-591: LGTM! Consider enhancing error response documentation.The improved error response structure with detailed fields (
code,param,type) provides better context for API consumers. However, consider the following enhancements:
- Define a shared interface for error responses
- Document these error fields in the API documentation
Consider adding a type definition for the error response:
interface ApiErrorResponse { error: string; code: 'unauthorized' | 'invalid_request' | 'internal_error'; // Add other possible codes param?: string; type: 'authorization' | 'validation' | 'system'; // Add other possible types }packages/trigger-sdk/src/v3/auth.ts (4)
28-29: Ensure new functions are properly exported and documentedThe new functions
createTriggerPublicToken,createBatchTriggerPublicToken,withPublicToken,withTriggerPublicToken, andwithBatchTriggerPublicTokenare correctly added to theauthexport object. Please ensure that they are thoroughly documented and have corresponding unit tests to maintain code quality.Also applies to: 31-33
59-65: Deprecation ofwriteproperty should be clearly communicatedThe
writeproperty inPublicTokenPermissionsis marked as deprecated in favor oftrigger. Ensure that this deprecation is clearly communicated to users, and consider adding deprecation warnings where applicable. Plan for its removal in a future major release to prevent potential confusion.
208-229: Consider refactoring to eliminate code duplication in token creation functionsThe functions
createTriggerPublicTokenandcreateBatchTriggerPublicTokenshare similar logic. To improve maintainability and reduce potential errors, consider abstracting the shared code into a helper function.Suggested refactor:
Create a generic function to handle token creation:
async function createPublicTriggerToken( task: string | string[], scopeType: "trigger" | "batchTrigger", options?: CreateTriggerTokenOptions ): Promise<string> { const apiClient = apiClientManager.clientOrThrow(); const claims = await apiClient.generateJWTClaims(); return await internal_generateJWT({ secretKey: apiClient.accessToken, payload: { ...claims, otu: typeof options?.multipleUse === "boolean" ? !options.multipleUse : true, scopes: flattenScopes({ [scopeType]: { tasks: task, }, }), }, expirationTime: options?.expirationTime, }); }Refactor the original functions to use this helper:
async function createTriggerPublicToken( task: string | string[], options?: CreateTriggerTokenOptions ): Promise<string> { return createPublicTriggerToken(task, "trigger", options); } async function createBatchTriggerPublicToken( task: string | string[], options?: CreateTriggerTokenOptions ): Promise<string> { return createPublicTriggerToken(task, "batchTrigger", options); }Also applies to: 279-301
231-247: Refactor to reduce duplication inwithTriggerPublicTokenfunctionsSimilarly, the
withTriggerPublicTokenandwithBatchTriggerPublicTokenfunctions have redundant code. Consider creating a generic helper function to encapsulate the shared logic.Suggested refactor:
Create a generic function:
async function withPublicTriggerToken( task: string | string[], scopeType: "trigger" | "batchTrigger", options: CreateTriggerTokenOptions = {}, fn: () => Promise<void> ) { const token = scopeType === "trigger" ? await createTriggerPublicToken(task, options) : await createBatchTriggerPublicToken(task, options); await withAuth({ accessToken: token }, fn); }Refactor the original functions:
async function withTriggerPublicToken( task: string | string[], options: CreateTriggerTokenOptions = {}, fn: () => Promise<void> ) { await withPublicTriggerToken(task, "trigger", options, fn); } async function withBatchTriggerPublicToken( task: string | string[], options: CreateTriggerTokenOptions = {}, fn: () => Promise<void> ) { await withPublicTriggerToken(task, "batchTrigger", options, fn); }Also applies to: 302-318
apps/webapp/app/v3/services/batchTriggerV2.server.ts (4)
51-51: Add unit tests foroneTimeUseTokenfunctionalityTo ensure the new
oneTimeUseTokenfeature works as intended, consider adding unit tests that cover:
- Successful usage of a valid token.
- Attempts to reuse an expired or already used token.
- Error handling when an invalid or missing token is provided.
Would you like assistance in creating these unit tests or opening a GitHub issue to track this task?
311-335: Generalize error handling for unique constraint violationsThe current error handling specifically checks for
oneTimeUseTokenandidempotencyKeyunique constraint violations. To make the code more robust, consider generalizing the error handling to cover other potential unique constraints. Logging unexpected constraint violations can also aid in debugging.
324-327: Enhance robustness of error target checkingTo prevent potential runtime errors, consider adding safeguards for unexpected values in
error.meta.target. This could involve:
- Verifying that
targetis an array of strings.- Adding a default case or error message if
targetdoesn't match expected patterns.
60-310: Refactorcallmethod for improved maintainabilityThe
callmethod is quite lengthy and handles multiple responsibilities, including:
- Idempotency checks.
- Dependent attempt handling.
- Entitlement verification.
- Cached runs processing.
- Queue size guarding.
- Payload packet handling.
- Batch processing initiation.
Consider refactoring this method into smaller, well-defined helper functions to enhance readability and maintainability.
apps/webapp/app/v3/services/triggerTask.server.ts (1)
42-42: Document theoneTimeUseTokenoptionConsider adding comments or documentation for the new
oneTimeUseTokenproperty inTriggerTaskServiceOptionsto clarify its purpose and usage. This will enhance code readability and help other developers understand its functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
apps/webapp/app/routes/api.v1.tasks.$taskId.trigger.ts(4 hunks)apps/webapp/app/routes/api.v1.tasks.batch.ts(4 hunks)apps/webapp/app/services/apiAuth.server.ts(5 hunks)apps/webapp/app/services/authorization.server.ts(1 hunks)apps/webapp/app/services/routeBuilders/apiBuilder.server.ts(1 hunks)apps/webapp/app/v3/services/batchTriggerV2.server.ts(5 hunks)apps/webapp/app/v3/services/triggerTask.server.ts(4 hunks)apps/webapp/test/authorization.test.ts(2 hunks)internal-packages/database/prisma/migrations/20241129141824_add_one_time_use_tokens/migration.sql(1 hunks)internal-packages/database/prisma/schema.prisma(4 hunks)packages/trigger-sdk/src/v3/auth.ts(3 hunks)references/v3-catalog/src/management.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal-packages/database/prisma/migrations/20241129141824_add_one_time_use_tokens/migration.sql
🔇 Additional comments (18)
apps/webapp/app/services/authorization.server.ts (2)
Line range hint 42-95: Ensure test coverage for new action types
With the addition of new action types like "trigger" and "batchTrigger", we should verify that the authorization checks are properly tested.
Line range hint 42-95: Verify the usage of superScopes parameter
The superScopes parameter provides a powerful bypass mechanism. Let's verify its usage across the codebase to ensure it's not misused.
Consider adding:
- Documentation for what constitutes a valid super scope
- Logging when super scope bypasses are used
- Runtime validation of super scope values
apps/webapp/app/routes/api.v1.tasks.$taskId.trigger.ts (5)
6-6: LGTM: Import addition aligns with one-time use token implementation
The addition of getOneTimeUseToken import is consistent with the PR's objective of implementing one-time use tokens for frontend triggering.
62-63: LGTM: Secure token generation placement
The one-time use token generation is well-placed after trace context setup and before logging, maintaining security by not exposing the token in logs.
Line range hint 116-150: LGTM: Response handling remains secure
The existing response structure appropriately handles JWT generation for browser clients while maintaining security. No changes were needed here as the one-time use token is handled upstream.
36-39: LGTM: Enhanced permission granularity with "trigger" action
The change from "write" to "trigger" action provides more specific permission control while maintaining compatibility through superScopes.
Let's verify this authorization change is consistent across related files:
✅ Verification successful
Let me gather more information about authorization patterns in the codebase to ensure a comprehensive verification.
Let me try one more search to specifically look for task-related authorization patterns and their associated scopes.
Authorization change is consistent with codebase patterns
The change from "write" to "trigger" action is well-aligned with the codebase's authorization patterns:
- Task-related endpoints use specific actions: "trigger" for single tasks and "batchTrigger" for batch operations
- The superScopes ["write:tasks", "admin"] are consistently used across task-related endpoints
- This maintains proper permission hierarchy while adding granular control
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other routes using similar authorization patterns
rg -l 'createActionApiRoute.*authorization.*action:' | xargs rg 'action:.*"(write|trigger)"'
Length of output: 174
Script:
#!/bin/bash
# Search for all authorization patterns in routes to understand the action types
rg -A 2 'authorization.*\{' apps/webapp/app/routes/
# Also search for any task-related routes and their authorization
rg -A 3 'createActionApiRoute.*tasks'
Length of output: 2545
Script:
#!/bin/bash
# Search for task-related routes with their authorization and scope definitions
rg -A 5 'tasks.*\$taskId|tasks\.batch' apps/webapp/app/routes/
# Also look for any superScope definitions related to tasks
rg -A 2 'superScopes.*write:tasks|superScopes.*admin'
Length of output: 3443
Line range hint 75-84: Verify error handling for token generation
While the token is correctly passed to service.call, we should ensure proper error handling if token generation fails.
Let's check the error handling implementation:
✅ Verification successful
Token generation and error handling is properly implemented
The token generation is safely implemented with proper error handling:
- Returns
undefinedif auth type is not "PUBLIC_JWT" or one-time use is not enabled - Uses crypto.subtle.digest for secure token generation
- Any crypto operation failures will be caught by the outer try-catch block in the route handler (as seen in the route file)
- The service layer also handles token uniqueness violations with clear error messages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check implementation of getOneTimeUseToken and its error handling
ast-grep --pattern 'function getOneTimeUseToken($_) {
$$$
}'
Length of output: 64
Script:
#!/bin/bash
# Let's try to find the token generation and error handling logic with a broader search
rg -A 10 "oneTimeUseToken"
Length of output: 14906
Script:
#!/bin/bash
# Let's find the implementation of getOneTimeUseToken function
rg -A 5 "async function getOneTimeUseToken|const getOneTimeUseToken"
Length of output: 494
Script:
#!/bin/bash
# Let's see the complete implementation of getOneTimeUseToken and its error handling
rg -A 15 "export async function getOneTimeUseToken"
Length of output: 1131
apps/webapp/app/routes/api.v1.tasks.batch.ts (3)
15-15: LGTM: Import changes align with one-time use token implementation.
The addition of getOneTimeUseToken and AuthenticatedEnvironment imports is appropriate for implementing the one-time use token feature.
91-91: Verify service handling of one-time use token.
The token is passed to the service call, but we should verify the service's implementation handles it correctly.
✅ Verification successful
Let me gather more information about how the token is used in the service implementation.
Token handling implementation is properly integrated
The service implementation correctly handles the one-time use token:
- The token is properly defined in the database schema for both
TaskRunandBatchTaskRuntables - The token is correctly passed through the service chain: BatchTriggerV2Service → TriggerTaskService
- There's validation in place to handle token-related errors
- The schema indicates a TODO for adding a unique constraint on the token, which is a good security practice
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for BatchTriggerV2Service implementation and usage of oneTimeUseToken
ast-grep --pattern 'class BatchTriggerV2Service {
$$$
call($$$) {
$$$
}
$$$
}'
# Search for type definition of service options
rg 'BatchTriggerTaskServiceOptions' -A 10
Length of output: 7946
Script:
#!/bin/bash
# Search for TriggerTaskService implementation to see how oneTimeUseToken is handled
ast-grep --pattern 'class TriggerTaskService {
$$$
call($$$) {
$$$
}
$$$
}'
# Search for any usage of oneTimeUseToken in the codebase
rg 'oneTimeUseToken' -B 2 -A 2
Length of output: 7188
Line range hint 25-30: Verify the authorization action change impact.
The change from "write" to "batchTrigger" represents a more granular permission model. This change needs verification to ensure all clients have the correct permissions.
✅ Verification successful
Let me analyze the search results and generate additional verification scripts to check the authorization configuration.
The first command will help us find other occurrences of the "batchTrigger" action in authorization configurations. The second command will help locate authorization-related files that might contain relevant permission definitions.
Let me check the authorization service implementation to understand the permission model better.
Based on the comprehensive search results, I can now generate the final response:
The authorization action change to "batchTrigger" is consistent with the codebase's permission model.
The change from "write" to "batchTrigger" is a valid refinement of the permission model because:
- The authorization service (
authorization.server.ts) supports custom action strings beyond basic "read"/"write" - The superScopes ["write:tasks", "admin"] are correctly maintained, ensuring backward compatibility
- The action is consistently used in the batch trigger endpoint configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of authorization actions to ensure consistency
rg -A 3 'authorization:\s*{'
# Search for any existing uses of 'batchTrigger' action
rg 'batchTrigger'
Length of output: 32690
Script:
#!/bin/bash
# Search for authorization configurations in the codebase
rg -A 3 'action:.*"batchTrigger"'
# Search for authorization-related files
fd -e ts -e js auth
Length of output: 1519
Script:
#!/bin/bash
# Check the authorization service implementation
cat apps/webapp/app/services/authorization.server.ts
# Check for any other uses of write:tasks or admin scopes
rg "write:tasks|admin" -A 3
Length of output: 49716
references/v3-catalog/src/management.ts (1)
1-1: LGTM: Import addition is appropriate
The addition of the auth import aligns with the PR's objective to implement one-time use tokens.
apps/webapp/test/authorization.test.ts (2)
13-16: LGTM! Well-structured test entity definition.
The new entity follows the established patterns and naming conventions, with a clear and specific scope definition.
56-76: LGTM! Well-structured test cases with good coverage.
The test cases effectively verify both successful and failed authorization scenarios for task-specific write permissions.
packages/trigger-sdk/src/v3/auth.ts (4)
66-78: Addition of trigger and batchTrigger enhances permission granularity
The introduction of trigger and batchTrigger properties improves clarity and control over token permissions. This enhancement allows for more precise permission management.
151-155: withPublicToken function correctly implements token scoping
The withPublicToken function appropriately creates a temporary public token and executes the provided function within the scoped authentication context. This implementation aligns with the intended functionality.
157-174: CreateTriggerTokenOptions type is well-defined
The CreateTriggerTokenOptions type is clearly defined with proper documentation for expirationTime and multipleUse properties. This provides flexibility in token creation options.
208-229: Verify the logic for setting the otu (one-time-use) flag
In the token payload, the otu flag is set using:
otu: typeof options?.multipleUse === "boolean" ? !options.multipleUse : true,Please verify that this logic correctly reflects the intended behavior:
- When
multipleUseistrue,otubecomesfalse, allowing multiple uses. - When
multipleUseisfalseor undefined,otudefaults totrue, enforcing one-time use.
Ensure that this meets the design requirements for token usage.
apps/webapp/app/services/apiAuth.server.ts (1)
550-551: 🛠️ Refactor suggestion
Use Node.js crypto module for hashing
The use of crypto.subtle.digest and TextEncoder may not be fully supported in all Node.js environments. To ensure compatibility and avoid potential issues, consider using the Node.js crypto module's createHash method instead.
Apply this diff:
- const hash = await crypto.subtle.digest("SHA-256", new TextEncoder().encode(auth.apiKey));
- return Buffer.from(hash).toString("hex");
+ const hash = crypto.createHash('sha256').update(auth.apiKey).digest('hex');
+ return hash;Likely invalid or redundant comment.
| runIds: runs.map((r) => r.id), | ||
| status: "COMPLETED", | ||
| batchVersion: "v2", | ||
| oneTimeUseToken: options.oneTimeUseToken, |
There was a problem hiding this comment.
Ensure secure handling of oneTimeUseToken storage
Storing oneTimeUseToken values in plain text may pose security risks, such as unauthorized access or token leakage. Consider hashing or encrypting the token before storing it in the database to enhance security and prevent potential misuse.
Also applies to: 373-373, 451-451
| if (error instanceof Prisma.PrismaClientKnownRequestError) { | ||
| logger.debug("TriggerTask: Prisma transaction error", { | ||
| code: error.code, | ||
| message: error.message, | ||
| meta: error.meta, | ||
| }); | ||
|
|
||
| return run; | ||
| if (error.code === "P2002") { | ||
| const target = error.meta?.target; | ||
|
|
||
| if ( | ||
| Array.isArray(target) && | ||
| target.length > 0 && | ||
| typeof target[0] === "string" && | ||
| target[0].includes("oneTimeUseToken") | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify and strengthen error handling for unique constraint violations
The current condition checks only the first element of error.meta.target to determine if the unique constraint violation is related to oneTimeUseToken. If multiple fields are involved in the constraint, and oneTimeUseToken is not the first element, this check might fail.
Consider updating the condition to check if target includes "oneTimeUseToken" anywhere in the array. This makes the error handling more robust and ensures the appropriate error message is displayed.
Apply this diff to improve the condition:
- if (
- Array.isArray(target) &&
- target.length > 0 &&
- typeof target[0] === "string" &&
- target[0].includes("oneTimeUseToken")
- ) {
+ if (Array.isArray(target) && target.includes("oneTimeUseToken")) {📝 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.
| if (error instanceof Prisma.PrismaClientKnownRequestError) { | |
| logger.debug("TriggerTask: Prisma transaction error", { | |
| code: error.code, | |
| message: error.message, | |
| meta: error.meta, | |
| }); | |
| return run; | |
| if (error.code === "P2002") { | |
| const target = error.meta?.target; | |
| if ( | |
| Array.isArray(target) && | |
| target.length > 0 && | |
| typeof target[0] === "string" && | |
| target[0].includes("oneTimeUseToken") | |
| ) { | |
| if (error instanceof Prisma.PrismaClientKnownRequestError) { | |
| logger.debug("TriggerTask: Prisma transaction error", { | |
| code: error.code, | |
| message: error.message, | |
| meta: error.meta, | |
| }); | |
| if (error.code === "P2002") { | |
| const target = error.meta?.target; | |
| if (Array.isArray(target) && target.includes("oneTimeUseToken")) { |
@trigger.dev/build
trigger.dev
@trigger.dev/react-hooks
@trigger.dev/rsc
@trigger.dev/sdk
@trigger.dev/core
commit: |
🦋 Changeset detectedLatest commit: dee4f9d The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.changeset/bright-lizards-wait.md (1)
4-5: Consider enhancing the change description with more details.While the description is clear, it would be helpful to add:
- The security benefits of one-time use tokens
- Any migration steps required
- Impact on existing implementations
Example enhancement:
-Add one-time use public tokens to trigger and batch trigger +Add one-time use public tokens to trigger and batch trigger + +This security enhancement: +- Introduces single-use tokens for improved security +- Prevents token replay attacks +- Maintains backward compatibility with existing implementations + +No migration steps required for existing implementations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.changeset/bright-lizards-wait.md(1 hunks)
🔇 Additional comments (1)
.changeset/bright-lizards-wait.md (1)
1-3: LGTM on the version bump!
The patch version bump is appropriate for this security enhancement since it adds functionality in a backward-compatible way.
No description provided.