Add dynamic token refresh and authentication error handling #911
Conversation
- Add retry logic for webhook methods on unauthenticated error - Add test cases to verify auth-webhook
…nto refresh-auth-token
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several changes across multiple files, primarily focusing on enhancing error handling, authentication mechanisms, and dependency management within the Changes
Assessment against linked issues
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: 15
🧹 Outside diff range and nitpick comments (9)
packages/sdk/test/helper/helper.ts (1)
190-190: LGTM! Consider adding JSDoc examples.The signature change to support both string and RegExp for error message validation is a good improvement. This will enable more flexible error testing patterns, especially useful for the new authentication token update feature.
Consider adding JSDoc examples to demonstrate both string and RegExp usage:
/** * Asserts that the async function throws an error matching the expected type and message. * @param fn The async function to test * @param errType The expected error type * @param message Optional string or RegExp to match against the error message * @example * // Using string message * await assertThrowsAsync( * async () => { throw new Error('auth failed') }, * Error, * 'auth failed' * ); * * // Using RegExp pattern * await assertThrowsAsync( * async () => { throw new Error('token expired: ABC123') }, * Error, * /token expired: .+/ * ); */packages/sdk/src/api/converter.ts (1)
822-832: Add JSDoc documentation for the new function.The function lacks documentation about its purpose, parameters, return value, and usage examples. This is especially important as it's part of the authentication error handling mechanism.
Add comprehensive documentation:
+/** + * `errorMetadataOf` extracts all metadata from the given ConnectError. + * This is particularly useful for handling authentication errors and token refresh scenarios. + * + * @param error - The ConnectError containing error details + * @returns A record of metadata key-value pairs or an empty object if no metadata exists + * + * @example + * ```typescript + * const error = new ConnectError("Auth failed"); + * const metadata = errorMetadataOf(error); + * if (metadata.reason === "token_expired") { + * // Handle token refresh + * } + * ``` + */ export function errorMetadataOf(error: ConnectError): Record<string, string> {packages/sdk/src/document/document.ts (2)
909-917: Enhance documentation for auth-error subscription.The method overload is correctly implemented, but the documentation could be more descriptive about when this event occurs and how to handle it.
/** * `subscribe` registers a callback to subscribe to events on the document. - * The callback will be called when the authentification error occurs. + * The callback will be called when authentication errors occur, such as: + * - Token expiration + * - Invalid token + * - Authentication webhook failures + * + * @example + * ```typescript + * doc.subscribe('auth-error', (event) => { + * console.log(`Auth error in ${event.value.method}: ${event.value.errorMessage}`); + * // Handle token refresh + * }); + * ``` */
Line range hint
202-1101: Consider enhancing the auth error handling architecture.The current implementation provides a good foundation for auth error handling through events. However, consider these architectural improvements:
Error Recovery Pattern: Consider implementing a standardized error recovery pattern that includes:
- Automatic retry with exponential backoff
- Circuit breaker pattern for repeated auth failures
- Queue for operations during token refresh
Token Lifecycle Management: Consider adding events for the entire token lifecycle:
- Token refresh initiated
- Token refresh succeeded
- Token refresh failed
State Management: Consider tracking auth state to prevent operation attempts during token refresh.
This would make the authentication system more robust and provide better visibility into the auth state.
packages/sdk/test/integration/webhook_test.ts (1)
308-312: Avoid potential test flakiness due to timing dependenciesUsing a short token expiration time like
500msand relying onsetTimeoutcan lead to flaky tests, especially on slower machines or under varying load conditions.Consider increasing
TokenExpirationMsto a higher value or mock the time functions to simulate token expiration without relying on real-time delays.Also applies to: 328-329
packages/sdk/test/integration/client_test.ts (1)
Line range hint
428-436: Simplify the conditional statements in the broadcast event handlerBoth conditions in the
ifandelse ifstatements perform the same action. You can simplify the code by removing the conditional checks.Apply this diff to simplify the code:
const unsubscribe = d2.subscribe('broadcast', (event) => { const { topic, payload } = event.value; - if (topic === broadcastTopic1) { - eventCollector.add([topic, payload]); - } else if (topic === broadcastTopic2) { - eventCollector.add([topic, payload]); - } + eventCollector.add([topic, payload]); });packages/sdk/src/client/client.ts (3)
132-138: Clarify theauthErrorMessageparameter inauthTokenInjector.The description mentions an
authErrorparameter, but the function signature usesauthErrorMessage. Ensure consistency and clarify the purpose of this parameter in the documentation.
Line range hint
913-987: EnsureretryCountincrements correctly inrunWatchLoop.In the
runWatchLoop, theretryCountvariable is initialized but may not increment in the error handling block, potentially leading to infinite retries. IncrementretryCountupon each retry attempt and enforcemaxRequestRetries.Apply this diff to increment
retryCount:... if (await this.handleConnectError(err)) { if ( err instanceof ConnectError && errorCodeOf(err) === Code.ErrUnauthenticated ) { if (retryCount >= this.maxRequestRetries) { logger.error( `[WD] c:"${this.getKey()}" max retries (${this.maxRequestRetries}) exceeded`, ); reject(err); return; } attachment.doc.publish([ { type: DocEventType.AuthError, value: { errorMessage: errorMetadataOf(err).message, method: 'WatchDocuments', }, }, ]); } onDisconnect(); + retryCount++; } else { this.conditions[ClientCondition.WatchLoop] = false; } ...
Line range hint
1147-1186: Expand retry logic to include additional error codes.Currently,
handleConnectErrorretries on a specific set of error codes. Consider includingDeadlineExceededandInternalerrors, which are often transient and may benefit from retrying.Apply this diff to handle more retryable errors:
... if ( err.code === ConnectErrorCode.Canceled || err.code === ConnectErrorCode.Unknown || err.code === ConnectErrorCode.ResourceExhausted || err.code === ConnectErrorCode.Unavailable || + err.code === ConnectErrorCode.DeadlineExceeded || + err.code === ConnectErrorCode.Internal ) { return true; } ...
📜 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 (9)
- packages/sdk/package.json (2 hunks)
- packages/sdk/src/api/converter.ts (1 hunks)
- packages/sdk/src/client/client.ts (21 hunks)
- packages/sdk/src/document/document.ts (6 hunks)
- packages/sdk/src/util/error.ts (1 hunks)
- packages/sdk/test/helper/helper.ts (1 hunks)
- packages/sdk/test/integration/client_test.ts (1 hunks)
- packages/sdk/test/integration/integration_helper.ts (2 hunks)
- packages/sdk/test/integration/webhook_test.ts (1 hunks)
🔇 Additional comments (10)
packages/sdk/test/integration/integration_helper.ts (2)
31-36: Verify retry logic test coverage.Setting
retryRequestDelay: 0could potentially mask timing-related issues in the retry mechanism. While this makes tests run faster, it might not catch real-world scenarios where delays between retries are crucial.#!/bin/bash # Check for retry-related test coverage echo "Checking for retry-related tests..." rg -g '**/*test.ts' -B 5 -A 5 'retryRequestDelay|retry.*logic|retry.*mechanism' # Look for other test files that might need the retry configuration echo "Checking for other client initializations that might need updating..." rg -g '**/*test.ts' 'new yorkie\.Client'Consider adding:
- Tests with non-zero retry delays to validate timing behavior
- Tests specifically for the retry mechanism with token updates
- Tests for maximum retry attempts and failure scenarios
8-9: Consider security implications of default credentials and add documentation.While using default credentials in tests is common, having them default to 'admin' could lead to:
- Accidentally running tests against production if env vars are misconfigured
- Security issues if these defaults are copied to other contexts
Consider these improvements:
- Add JSDoc comments explaining the purpose and usage of these constants
- Use more obvious test credentials (e.g., 'test-api-id') to prevent confusion
- Consider making these required env vars for better security
✅ Verification successful
Let me generate additional verification scripts to check for security implications.
The first script will help us verify that these test credentials are indeed confined to test files. The second script will help us understand the broader authentication testing patterns in the codebase to make a more informed decision about the security implications of these default credentials.
Let me try one more verification with corrected pattern for test files:
This should help us understand the authentication patterns in tests and check if these credentials are mentioned in any documentation, which would be concerning from a security perspective.
Based on the verification results, I can now provide a final response:
Default test credentials are safely confined to test environment
The verification shows that:
- The
TEST_API_IDandTEST_API_PWenvironment variables and their default values are only used in test files- The credentials are part of a comprehensive authentication testing suite that includes proper token management, expiration handling, and error scenarios
- These credentials are not referenced in any documentation or example files that could lead to misuse
The default 'admin' values are acceptable in this context because:
- They are only used in integration tests
- The test suite properly tests both environment variable and default scenarios
- The authentication system has proper security measures including token expiration and refresh mechanisms
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if these credentials are used in actual implementation code rg -g '!test/**' -l 'admin.*admin' .Length of output: 17785
Script:
#!/bin/bash # Check if these credentials are used in production code or configuration rg -g '!test/**' 'TEST_API_ID|TEST_API_PW' . # Check if there are any other test credentials in the test files rg -g 'test/**' 'apiKey|token|credential|auth' -B 2 -A 2Length of output: 484
Script:
#!/bin/bash # Check for authentication patterns in test files rg -g '**/*test*.ts' 'apiKey|token|credential|auth' -B 2 -A 2 # Check if these test credentials are referenced in any documentation or example files rg -g '**/*.{md,mdx,txt}' 'TEST_API_ID|TEST_API_PW|testAPIID|testAPIPW'Length of output: 24484
packages/sdk/package.json (2)
67-67: LGTM: Safe dependency update.The update of
vite-tsconfig-pathsfrom 4.2.1 to 4.3.1 is a minor version bump that should maintain backward compatibility according to semver.
56-56: Verify security implications of axios version.The current axios version (1.7.7) might have known vulnerabilities. Consider upgrading to the latest patch version.
Let's check for known vulnerabilities:
✅ Verification successful
Let me try a different approach to verify the axios version security implications using GitHub's API to check for security advisories.
This script will help us:
- Check for any published security advisories affecting axios
- Get the latest version from the main branch to compare with 1.7.7
Let me try one final verification approach using the GitHub API to check for security advisories in a different way.
Axios version 1.7.7 is secure - no action needed
Based on the GitHub Security Advisory data, version 1.7.7 is not affected by any known vulnerabilities:
- The most recent high-severity vulnerability affects versions <= 1.7.3
- Other vulnerabilities affect older version ranges that don't include 1.7.7
The current version (1.7.7) is actually newer than the versions affected by known security issues, making it a secure choice.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for known vulnerabilities in axios v1.7.7 # Note: This is a basic check. In production, you should use tools like npm audit # Get the latest version of axios 1.7.x LATEST=$(npm view axios versions --json | jq -r '.[]' | grep '^1\.7\.' | sort -V | tail -n 1) echo "Current version: 1.7.7" echo "Latest 1.7.x version: $LATEST" if [ "$LATEST" != "1.7.7" ]; then echo "Consider upgrading to $LATEST" fiLength of output: 360
Script:
#!/bin/bash # Check GitHub Security Advisories for axios vulnerabilities gh api \ -H "Accept: application/vnd.github+json" \ -H "X-GitHub-Api-Version: 2022-11-28" \ /repos/axios/axios/security-advisories \ | jq '.[] | select(.state=="published") | {published_at, summary, severity, references: .references.advisory_url}' # Also check the latest version from the package.json in axios repo curl -s https://raw.githubusercontent.com/axios/axios/main/package.json | jq -r '.version'Length of output: 339
Script:
#!/bin/bash # Check for vulnerabilities using GitHub's security advisory database gh api \ -H "Accept: application/vnd.github+json" \ -H "X-GitHub-Api-Version: 2022-11-28" \ graphql -f query=' query { securityVulnerabilities(ecosystem: NPM, package: "axios", first: 5, orderBy: {field: UPDATED_AT, direction: DESC}) { nodes { severity vulnerableVersionRange advisory { publishedAt summary } } } } 'Length of output: 1335
packages/sdk/src/document/document.ts (3)
202-206: LGTM! New auth error event type is well-defined.The addition of
AuthErrorevent type follows consistent naming patterns and is properly documented.
448-448: LGTM! Callback mapping is correctly defined.The 'auth-error' event callback mapping follows the established pattern and uses the correct type.
1090-1101: LGTM! Auth error event handling is properly implemented.The implementation follows the established pattern for event handling and is clean and straightforward.
packages/sdk/test/integration/webhook_test.ts (1)
400-405: Ensure proper error handling in asynchronous operationsThe
client.attach(doc)operation is asynchronous and may throw errors. Without proper error handling or assertions, failures might go unnoticed, potentially leading to silent test failures.Ensure that all asynchronous operations are wrapped in try-catch blocks or are properly awaited with assertions to handle any potential errors.
await client.attach(doc); + // Add assertion or error handling as neededpackages/sdk/test/integration/client_test.ts (2)
Line range hint
332-340: No issues found with the synchronization testThe added code correctly tests the behavior of clients in
RealtimePushOnlysync mode and ensures events are handled as expected.
Line range hint
612-614: Test for 'maxRetries' set to zero works as expectedThe added test correctly verifies that broadcasting does not retry on network failure when
maxRetriesis set to zero.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/sdk/src/client/client.ts (1)
1063-1071: Consider adding a comment explaining the race condition.The code handles a race condition between syncLoop and syncMode, but the comment could be more explicit about why this check is necessary and what problems it prevents.
-// NOTE(chacha912, hackerwins): If syncLoop already executed with -// PushPull, ignore the response when the syncMode is PushOnly. +// NOTE(chacha912, hackerwins): Ignore the response when in PushOnly mode to prevent +// applying changes twice. This can happen when syncLoop has already executed with +// PushPull mode and the mode was subsequently changed to PushOnly. Without this +// check, the same changes could be applied multiple times.packages/sdk/src/api/converter.ts (1)
807-818: Consider improving type safety and documentation.The function implementation looks good, but could benefit from these improvements:
- Consider using a type for metadata keys to ensure type safety.
- Add JSDoc to document the return type structure and possible metadata keys.
+/** + * Returns the error metadata from the given ConnectError. + * @param error - The ConnectError instance + * @returns Record<MetadataKey, string> where MetadataKey can be 'code' or other error-specific keys + */ export function errorMetadataOf(error: ConnectError): Record<string, string> { // NOTE(chacha912): Currently, we only use the first detail to represent the // error metadata. const infos = error.findDetails(ErrorInfo); for (const info of infos) { return info.metadata; } return {}; }
📜 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 (3)
- packages/sdk/package.json (2 hunks)
- packages/sdk/src/api/converter.ts (1 hunks)
- packages/sdk/src/client/client.ts (21 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/sdk/package.json
🔇 Additional comments (3)
packages/sdk/src/client/client.ts (2)
132-136: LGTM! Well-documented token injector interface.The
authTokenInjectorinterface is well-designed with clear documentation explaining its purpose and behavior.
1080-1084: LGTM! Good handling of removed documents.The code properly handles cleanup of watch streams for removed documents, preventing unnecessary network traffic and potential memory leaks.
packages/sdk/src/api/converter.ts (1)
820-824: LGTM! Good refactoring.The function has been nicely refactored to use
errorMetadataOf, making it more maintainable and reducing code duplication.
chacha912
force-pushed
the
refresh-auth-token
branch
from
0cbdff2 to
acd5ca6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/sdk/src/client/client.ts (1)
Line range hint
894-978: Reset retry count on successful watch stream connection.The retry count for watch loop is not reset when the connection is successfully established. This could lead to premature rejection of future reconnection attempts if the connection is lost after several retries.
Add retry count reset after successful connection:
if (resp.body.case === 'initialization') { + retryCount = 0; // Reset retry count on successful connection resolve([stream, ac]); }
📜 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 (3)
- packages/sdk/package.json (2 hunks)
- packages/sdk/src/api/converter.ts (1 hunks)
- packages/sdk/src/client/client.ts (21 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/sdk/package.json
🔇 Additional comments (3)
packages/sdk/src/client/client.ts (1)
Line range hint
132-172: LGTM! Well-documented interface changes.The new options are clearly documented and properly typed:
authTokenInjectorfor dynamic token updatesretryRequestDelayandmaxRequestRetriesfor configuring retry behaviorpackages/sdk/src/api/converter.ts (2)
809-822: Consider extracting shared error detail logic.The function implementation looks good, but there's an opportunity to reduce code duplication with
errorCodeOfas suggested in the past review.
824-828: LGTM!The refactored implementation is more concise and properly handles undefined values.
chacha912
force-pushed
the
refresh-auth-token
branch
from
acd5ca6 to
95f5faa
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/sdk/src/api/converter.ts (2)
807-809: Enhance function documentation.The documentation could be more descriptive about the function's behavior, especially regarding error handling and the structure of returned metadata.
/** - * `errorMetadataOf` returns the error metadata of the given connect error. + * `errorMetadataOf` returns the metadata from the first ErrorInfo detail of the given connect error. + * If the input is not a ConnectError or no ErrorInfo details are found, returns an empty object. + * @param error - The ConnectError to extract metadata from + * @returns A record of string key-value pairs containing error metadata */
807-828: Well-aligned with Auth Token Update requirements.The enhanced error handling implementation aligns well with the PR objectives for the Auth Token Update Interface. The ability to extract detailed error metadata will be particularly useful for handling authentication-related errors and token refresh scenarios.
Consider documenting common error metadata fields that will be used for authentication scenarios, particularly those related to token expiration and refresh requirements.
📜 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 (2)
- packages/sdk/package.json (2 hunks)
- packages/sdk/src/api/converter.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/sdk/package.json
🔇 Additional comments (1)
packages/sdk/src/api/converter.ts (1)
824-828: LGTM! Good refactoring.The function has been effectively refactored to reuse
errorMetadataOf, addressing the code duplication issue from previous reviews.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/sdk/src/document/document.ts (6 hunks)
🔇 Additional comments (5)
packages/sdk/src/document/document.ts (5)
202-206: LGTM! The new DocEventType enum value is well-defined.The
AuthErrorenum value follows the established naming pattern and has a clear descriptive comment.
227-228: LGTM! The DocEvent type union is correctly updated.The AuthErrorEvent is properly added to the DocEvent type union.
451-451: LGTM! The DocEventCallbackMap is correctly updated.The 'auth-error' mapping is properly added with the correct callback type.
1093-1104: LGTM! The subscribe implementation is well-structured.The implementation correctly follows the established pattern for event subscriptions.
424-430: 🛠️ Refactor suggestionConsider enhancing AuthErrorEvent with additional error details.
The current interface could be improved to better handle token expiration scenarios and provide more context for debugging:
The previous review suggestion is still valid. Apply this diff to enhance error information:
export interface AuthErrorEvent extends BaseDocEvent { type: DocEventType.AuthError; value: { errorMessage: string; method: 'AttachDocument' | 'DetachDocument' | 'PushPull' | 'WatchDocuments'; + code?: string; + details?: Record<string, unknown>; }; }
…nto refresh-auth-token
…nto refresh-auth-token
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
packages/sdk/test/integration/client_test.ts (1)
Line range hint
1-1: Add test coverage for Auth Token Update Interface.The PR implements token refresh functionality, but there are no tests verifying:
- Token expiration handling
- Server notification handling for expired tokens
- Token refresh callback execution
- Retry behavior with expired tokens
Consider adding the following test cases:
it('Should refresh token when server notifies expiration', async function ({ task }) { const mockTokenRefresh = vi.fn().mockResolvedValue('new-token'); const cli = new yorkie.Client(testRPCAddr, { authTokenInjector: mockTokenRefresh }); // Test implementation }); it('Should retry operations with refreshed token', async function ({ task }) { // Test implementation }); it('Should handle token refresh failures', async function ({ task }) { // Test implementation });Would you like me to help implement these test cases or create a GitHub issue to track this task?
packages/sdk/src/client/client.ts (1)
Line range hint
1147-1185: Consider caching the RPC client creation.The current implementation creates a new RPC client on every token refresh. Consider updating the interceptors of the existing client instead of creating a new one.
Modify the token refresh logic:
if (errorCodeOf(err) === Code.ErrUnauthenticated) { const token = this.authTokenInjector && (await this.authTokenInjector(errorMetadataOf(err).reason)); - this.rpcClient = createPromiseClient( - YorkieService, - createGrpcWebTransport({ - baseUrl: this.rpcAddr, - interceptors: [ - createAuthInterceptor(this.apiKey, token), - createMetricInterceptor(), - ], - }), - ); + if (this.rpcClient) { + // Update interceptors of existing transport + const transport = this.rpcClient.transport as any; + transport.interceptors = [ + createAuthInterceptor(this.apiKey, token), + createMetricInterceptor(), + ]; + } return true; }packages/sdk/src/document/document.ts (1)
202-206: Fix typo in documentation comment.The word "authentification" should be "authentication".
/** - * `AuthError` means that the authentification error occurs. + * `AuthError` means that the authentication error occurs. */ AuthError = 'auth-error',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/sdk/package.json(2 hunks)packages/sdk/src/client/client.ts(21 hunks)packages/sdk/src/document/document.ts(6 hunks)packages/sdk/test/integration/client_test.ts(1 hunks)packages/sdk/test/integration/webhook_test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/sdk/package.json
- packages/sdk/test/integration/webhook_test.ts
🔇 Additional comments (8)
packages/sdk/test/integration/client_test.ts (1)
166-171: Set minimal retryRequestDelay to prevent tight retry loops.
Setting retryRequestDelay to 0 may cause high CPU usage during network failures.
packages/sdk/src/client/client.ts (3)
132-138: LGTM! Well-documented token injector interface.
The authTokenInjector function is well-documented with clear explanation of its purpose and behavior.
Line range hint 211-251: LGTM! Clean initialization of new functionality.
The initialization of auth and retry-related fields is clean and follows the existing pattern.
702-709: Add retry logic to the remove method.
The remove method lacks retry logic unlike other methods (attach, detach). This inconsistency could lead to reliability issues.
packages/sdk/src/document/document.ts (4)
424-430: LGTM!
The interface follows the established pattern and the past review comment about enhancing error information is still valid.
451-451: LGTM!
The callback mapping is correctly defined and follows the established pattern.
912-920: LGTM!
The overload is well-defined and the past review comment about enhancing the overload is still valid.
1093-1104: LGTM!
The implementation is clean, consistent with other event types, and properly handles the auth-error subscription.
chacha912
force-pushed
the
refresh-auth-token
branch
from
7fef934 to
9454843
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
docker/docker-compose.yml (1)
12-13: LGTM! Consider adding a comment to document the purpose.The
extra_hostsconfiguration is correctly implemented, enabling container-to-host communication necessary for Auth Webhook testing.Consider adding a comment above the configuration to explain its purpose:
+ # Enable communication with host machine for Auth Webhook testing extra_hosts: - 'host.docker.internal:host-gateway'docker/docker-compose-ci.yml (1)
14-15: Consider security implications of exposing host gateway.While enabling host access is necessary for webhook communication, consider:
- Restricting this configuration to specific environments where it's needed
- Documenting the security implications in the configuration file
- Adding network-level restrictions if possible
packages/sdk/test/integration/integration_helper.ts (1)
9-10: Consider using non-admin test credentials by default.While using 'admin' credentials is convenient for testing, it might mask authentication-related issues. Consider using regular user credentials as defaults to ensure the authentication flow is thoroughly tested under normal conditions.
-export const testAPIID = process.env.TEST_API_ID || 'admin'; -export const testAPIPW = process.env.TEST_API_PW || 'admin'; +export const testAPIID = process.env.TEST_API_ID || 'test_user'; +export const testAPIPW = process.env.TEST_API_PW || 'test_password';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
docker/docker-compose-ci.yml(1 hunks)docker/docker-compose.yml(1 hunks)packages/sdk/test/integration/integration_helper.ts(2 hunks)packages/sdk/test/integration/webhook_test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/sdk/test/integration/webhook_test.ts
🔇 Additional comments (4)
docker/docker-compose.yml (1)
12-13: Verify production deployment configuration.
While this configuration is suitable for development and testing, ensure it's not included in production deployments where direct host access should be restricted.
Consider using environment-specific compose files or environment variables to control this configuration. For example:
✅ Verification successful
Docker host access configuration appears to be consistent across environments
The host.docker.internal configuration is present in both docker-compose files (docker-compose.yml and docker-compose-ci.yml), and there are no separate production deployment configurations found in the repository. Since this is a development-focused repository (containing examples, SDK, and devtools), the current configuration is appropriate for its purpose.
- Both compose files are located in the
docker/directory and are used for development/CI environments - No production-specific deployment configurations were found
- The repository structure suggests this is primarily a development/SDK repository
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this configuration exists in production-related files
rg -l 'host.docker.internal' deploy/ production/ kubernetes/
Length of output: 212
Script:
#!/bin/bash
# Let's first check what deployment-related directories exist
fd -t d -d 1
# Then search for docker-compose files and their content
fd -e yml -e yaml docker-compose
fd -e yml -e yaml docker-compose --exec cat {}
# Also check for any Kubernetes manifests or deployment configs
fd -e yml -e yaml -e json
Length of output: 1948
docker/docker-compose-ci.yml (1)
14-15: LGTM! Verify if this configuration is required for CI environment.
The addition of host.docker.internal mapping is typically used for local development to enable container-to-host communication. While this change aligns with the auth webhook functionality, please verify if this is necessary in the CI environment.
packages/sdk/test/integration/integration_helper.ts (2)
50-55: Consider implications of zero retry delay in tests.
Setting retryRequestDelay: 0 might make tests run faster but could mask timing-related issues that would occur in production. Consider adding additional test cases with realistic retry delays to ensure robust error handling.
#!/bin/bash
# Check for other test files that might need similar retry delay configurations
rg "retryRequestDelay" -t ts -t jsAdd test cases with realistic delays:
// Add to test suite
it('handles token refresh with realistic delays', async () => {
const client = new yorkie.Client(testRPCAddr, {
retryRequestDelay: 1000, // 1 second delay
});
// ... test implementation
});11-27: 🛠️ Refactor suggestion
Ensure cross-platform compatibility for Docker checks.
The Docker command might fail on Windows systems due to different shell syntax. Consider using a cross-platform solution or handling platform-specific cases.
function isYorkieContainerRunning() {
if (process.env.CI === 'true') {
return true;
}
try {
+ const command = process.platform === 'win32'
+ ? 'docker ps --filter "name=^/yorkie$" --format "{{.Names}}"'
+ : 'docker ps --filter "name=^/yorkie$" --format "{{.Names}}"';
const result = execSync(
- 'docker ps --filter "name=^/yorkie$" --format "{{.Names}}"',
+ command,
{
stdio: 'pipe',
+ shell: true,
},
);
return result && result.toString().trim() !== '';
} catch (error) {
return false;
}
}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
packages/sdk/test/integration/client_test.ts (1)
Line range hint
1-1006: Add test coverage for Auth Token Update Interface.The test suite is missing coverage for the new authentication features mentioned in the PR objectives:
- Auth token expiration handling
- Token refresh mechanism using
authTokenInjector- Authentication error events for document operations (attach, detach, pushpull, watch)
Consider adding the following test scenarios:
- Token expiration and automatic refresh
- Failed token refresh scenarios
- Authentication error event handling
- Integration with Auth Webhook
Would you like me to help generate test cases for these scenarios?
packages/sdk/src/client/client.ts (1)
Line range hint
1151-1182: Consider defining retryable error codes as constants.The error codes that determine retry behavior should be defined as named constants for better maintainability and documentation.
+const RETRYABLE_CONNECT_ERROR_CODES = [ + ConnectErrorCode.Canceled, + ConnectErrorCode.Unknown, + ConnectErrorCode.ResourceExhausted, + ConnectErrorCode.Unavailable, +] as const; private async handleConnectError(err: any): Promise<boolean> { if (!(err instanceof ConnectError)) { return false; } - if ( - err.code === ConnectErrorCode.Canceled || - err.code === ConnectErrorCode.Unknown || - err.code === ConnectErrorCode.ResourceExhausted || - err.code === ConnectErrorCode.Unavailable - ) { + if (RETRYABLE_CONNECT_ERROR_CODES.includes(err.code)) { return true; }packages/sdk/src/client/auth_interceptor.ts (1)
26-29: Update documentation for the updated return type.The
createAuthInterceptorfunction now returns an object containingauthInterceptorandsetToken. To improve clarity for users, consider adding documentation or JSDoc comments to explain the new return type and its usage.packages/sdk/src/document/document.ts (2)
203-205: Fix typo in comment: 'authentification' should be 'authentication'.The word 'authentification' is misspelled; it should be 'authentication' in the comment.
913-915: Fix typo in documentation: 'authentification' should be 'authentication'.Replace 'authentification' with 'authentication' in the comment to correct the spelling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/sdk/src/client/auth_interceptor.ts(1 hunks)packages/sdk/src/client/client.ts(21 hunks)packages/sdk/src/document/document.ts(6 hunks)packages/sdk/test/helper/helper.ts(1 hunks)packages/sdk/test/integration/client_test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/sdk/test/helper/helper.ts
🔇 Additional comments (10)
packages/sdk/test/integration/client_test.ts (1)
169-174: Previous comment about minimal retryRequestDelay is still applicable.
packages/sdk/src/client/client.ts (4)
Line range hint 132-172: LGTM! Well-documented interface changes.
The new ClientOptions interface properly implements the token refresh mechanism requirements with clear documentation.
195-196: Previous suggestion still applies.
286-315: Previous suggestion for extracting retry logic still applies.
Also applies to: 329-356, 406-490, 525-582, 1052-1143
706-713: Previous suggestion for adding retry logic to remove method still applies.
packages/sdk/src/client/auth_interceptor.ts (1)
40-41: Ensure currentToken includes any required authorization scheme prefix.
When setting the authorization header, verify that currentToken includes any necessary scheme prefixes such as Bearer . If currentToken is just the token string, you may need to prepend Bearer before setting the header to ensure proper authentication.
packages/sdk/src/document/document.ts (4)
227-228: Approve addition of AuthErrorEvent to DocEvent union type.
The inclusion of AuthErrorEvent in the DocEvent type is appropriate and ensures proper event handling.
424-430: Approve definition of AuthErrorEvent interface.
The AuthErrorEvent interface is well-defined and correctly extends BaseDocEvent.
451-451: Approve addition of 'auth-error' in DocEventCallbackMap.
The mapping of 'auth-error' to NextFn<AuthErrorEvent> in DocEventCallbackMap maintains consistency with other event callbacks.
1093-1104: Approve implementation of 'auth-error' event handling in subscribe method.
The addition of the 'auth-error' case in the subscribe method correctly follows the established pattern.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
packages/sdk/src/client/client.ts (3)
159-176: Remove implementation details from comments.These comments contain implementation details that should be in proper documentation. Consider moving them to API documentation or developer guides.
303-334: Extract common retry logic to reduce duplication.The retry logic is duplicated between
activateanddeactivatemethods. Consider extracting it into a reusable utility.Create a utility method:
private async withRetry<T>( operation: () => Promise<T>, context: string, ): Promise<T> { const executeWithRetry = async (retryCount = 0): Promise<T> => { try { return await operation(); } catch (err) { if (retryCount >= this.maxRequestRetries) { logger.error( `[${context}] c:"${this.getKey()}" max retries (${this.maxRequestRetries}) exceeded`, ); throw err; } if (await this.handleConnectError(err)) { await delay(this.retryRequestDelay); return executeWithRetry(retryCount + 1); } logger.error(`[${context}] c:"${this.getKey()}" err:`, err); throw err; } }; return executeWithRetry(); }Also applies to: 347-376
488-505: Consider simplifying error handling flow.The error handling in document operations is complex with nested conditions. Consider extracting the auth error publishing logic to a separate method for better readability.
Create a utility method:
private publishAuthError(doc: Document<any, any>, err: ConnectError, method: string): void { if (errorCodeOf(err) === Code.ErrUnauthenticated) { doc.publish([ { type: DocEventType.AuthError, value: { reason: errorMetadataOf(err).reason, method, }, }, ]); } }Also applies to: 581-598
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/sdk/src/client/client.ts(17 hunks)packages/sdk/test/integration/webhook_test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/sdk/test/integration/webhook_test.ts
chacha912
force-pushed
the
refresh-auth-token
branch
from
155163c to
0c7eeca
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
packages/sdk/test/integration/integration_helper.ts (2)
9-10: Add documentation and CI environment validation for API credentials.While using default test credentials is acceptable for local development, consider:
- Adding JSDoc comments explaining the purpose and usage of these constants
- Enforcing environment variables in CI to prevent accidental use of default credentials
+/** + * Default API credentials for testing. + * NOTE: These should only be used in test environments. + * In CI environments, these should be overridden via environment variables. + */ export const testAPIID = process.env.TEST_API_ID || 'admin'; export const testAPIPW = process.env.TEST_API_PW || 'admin'; + +if (process.env.CI === 'true' && (!process.env.TEST_API_ID || !process.env.TEST_API_PW)) { + throw new Error('TEST_API_ID and TEST_API_PW must be set in CI environment'); +}
11-27: Consider performance optimizations for Docker container check.The implementation is secure but could benefit from performance improvements:
- Cache the result as it's unlikely to change during test execution
- Add a timeout to prevent hanging
+let isYorkieContainerRunningCache: boolean | null = null; + function isYorkieContainerRunning() { if (process.env.CI === 'true') { return true; } + if (isYorkieContainerRunningCache !== null) { + return isYorkieContainerRunningCache; + } + try { const result = execSync( 'docker ps --filter "name=^/yorkie$" --format "{{.Names}}"', { stdio: 'pipe', + timeout: 5000, // 5 second timeout }, ); - return result && result.toString().trim() !== ''; + isYorkieContainerRunningCache = result && result.toString().trim() !== ''; + return isYorkieContainerRunningCache; } catch (error) { - return false; + isYorkieContainerRunningCache = false; + return isYorkieContainerRunningCache; } }packages/sdk/src/client/client.ts (4)
192-192: Ensure proper initialization of auth-related properties.While the properties are well-typed, consider initializing
setAuthTokento a no-op function by default to prevent potential undefined calls.- private setAuthToken: (token: string) => void; + private setAuthToken: (token: string) => void = () => {};Also applies to: 199-199
228-229: Consider validating tokens before setting them.Add basic token validation to prevent setting empty or malformed tokens.
const { authInterceptor, setToken } = createAuthInterceptor(this.apiKey); - this.setAuthToken = setToken; + this.setAuthToken = (token: string) => { + if (!token?.trim()) { + logger.warn(`[AC] c:"${this.getKey()}" invalid token provided`); + return; + } + setToken(token); + };Also applies to: 237-237
750-770: Consider consolidating auth error handling.The auth error handling logic is duplicated across different methods. Consider extracting it to a shared method.
+ private publishAuthError(doc: Document<unknown, any>, reason: string, method: string) { + doc.publish([ + { + type: DocEventType.AuthError, + value: { reason, method }, + }, + ]); + } syncJobs.push( this.syncInternal(attachment, attachment.syncMode).catch( async (err) => { if ( err instanceof ConnectError && errorCodeOf(err) === Code.ErrUnauthenticated ) { - attachment.doc.publish([ - { - type: DocEventType.AuthError, - value: { - reason: errorMetadataOf(err).reason, - method: 'PushPull', - }, - }, - ]); + this.publishAuthError( + attachment.doc, + errorMetadataOf(err).reason, + 'PushPull' + ); } throw err; }, ), );
866-880: Reuse consolidated auth error handling.Apply the same error handling consolidation here for consistency.
if (await this.handleConnectError(err)) { if ( err instanceof ConnectError && errorCodeOf(err) === Code.ErrUnauthenticated ) { - attachment.doc.publish([ - { - type: DocEventType.AuthError, - value: { - reason: errorMetadataOf(err).reason, - method: 'WatchDocuments', - }, - }, - ]); + this.publishAuthError( + attachment.doc, + errorMetadataOf(err).reason, + 'WatchDocuments' + ); } onDisconnect(); }
🛑 Comments failed to post (1)
packages/sdk/src/client/client.ts (1)
253-256:
⚠️ Potential issueAdd error handling for initial token injection.
The initial token injection should handle potential errors to prevent activation with invalid tokens.
if (this.authTokenInjector) { - const token = await this.authTokenInjector(); - this.setAuthToken(token); + try { + const token = await this.authTokenInjector(); + if (!token) { + throw new Error('No token provided'); + } + this.setAuthToken(token); + } catch (err) { + logger.error(`[AC] c:"${this.getKey()}" initial token injection failed:`, err); + throw new YorkieError(Code.ErrUnauthenticated, 'Failed to inject initial auth token'); + } }📝 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 (this.authTokenInjector) { try { const token = await this.authTokenInjector(); if (!token) { throw new Error('No token provided'); } this.setAuthToken(token); } catch (err) { logger.error(`[AC] c:"${this.getKey()}" initial token injection failed:`, err); throw new YorkieError(Code.ErrUnauthenticated, 'Failed to inject initial auth token'); } }
chacha912
force-pushed
the
refresh-auth-token
branch
from
0c7eeca to
dc772ce
Compare
- Keep retry logic only in syncLoop and watchLoop - Remove retry from other method to allow manual retry by users
chacha912
force-pushed
the
refresh-auth-token
branch
from
dc772ce to
80b2bbb
Compare
hackerwins
force-pushed
the
refresh-auth-token
branch
from
12e4c6c to
d986ef3
Compare
hackerwins
changed the title
What this PR does / why we need it?
authTokenInjectorfunction instead of a static tokenAny background context you want to provide?
1. Token Refresh Support
Added a new
authTokenInjectorinterface that enables token refresh based on authentication error messages.When a
401(Unauthenticated)error occurs, the SDK calls theauthTokenInjectorwith the error message to obtain a new authToken and retries the request.2. Updated Webhook Response Format
Added webhook response status codes in HTTP header: yorkie-team/yorkie#1037
200: OK (Allowed)401: Unauthorized (Invalid or missing token)403: Forbidden (Permission denied)403errors and invalid webhook response formats will throw errors immediately without attempting token refresh.3. Authentication Error Event Notifications
Added support for monitoring authentication errors through event subscriptions:
401response is received from the webhook during syncLoop(PushPull) or watchLoop(WatchDocuments)4. Retry Logic for Authentication Errors
syncLoop(PushPull),watchLoop(WatchDocuments)auth-errorevent subscriptionactivate,deactivate,attach,detach, manualsyncWhat are the relevant tickets?
Fixes yorkie-team/yorkie#996
Checklist
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores