Feat: oauth pkce support workspace id #23
Conversation
WalkthroughThis pull request introduces several modifications across multiple files. The Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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: 3
🧹 Outside diff range and nitpick comments (5)
examples/coze-js-node/src/auth/auth-oauth-device.ts (1)
Line range hint
1-100: Consider adding production-ready featuresAs this is an example implementation, consider documenting or implementing these production-ready features:
- Token persistence mechanism
- Automatic token refresh before expiration
- Retry strategy for failed refresh attempts
- Token rotation security best practices
Consider creating a TokenManager class that encapsulates these concerns:
class TokenManager { private storage: TokenStorage; private refreshTimer: NodeJS.Timeout | null = null; constructor( private baseURL: string, private clientId: string, storage: TokenStorage ) { this.storage = storage; } async setupTokenRefresh(tokens: OAuthTokens) { // Store tokens await this.storage.saveTokens(tokens); // Setup automatic refresh const refreshBuffer = 5 * 60 * 1000; // 5 minutes before expiry const expiresIn = (tokens.expires_in || 3600) * 1000; this.scheduleRefresh(expiresIn - refreshBuffer); } private scheduleRefresh(timeUntilRefresh: number) { // Implementation details... } }packages/coze-js/src/auth.ts (2)
160-165: Improve code quality and reduce duplicationA few suggestions to enhance this code segment:
- Use
constinstead ofletsince the variable is only assigned once- Consider extracting the workspace ID URL construction logic into a shared utility function since it's used in multiple places
Consider this refactoring:
- let apiUrl; - if (config.workspaceId) { - apiUrl = `/api/permission/oauth2/workspace_id/${config.workspaceId}/device/code`; - } else { - apiUrl = '/api/permission/oauth2/device/code'; - } + const apiUrl = config.workspaceId + ? `/api/permission/oauth2/workspace_id/${config.workspaceId}/device/code` + : '/api/permission/oauth2/device/code';Also, consider creating a utility function for workspace URL construction:
const getWorkspaceUrl = (basePath: string, workspaceId?: string) => { if (!workspaceId) return basePath; if (!/^[a-zA-Z0-9-_]+$/.test(workspaceId)) { throw new Error('Invalid workspace ID format'); } return `/api/permission/oauth2/workspace_id/${workspaceId}${basePath}`; };
278-278: Add JSDoc comments for workspaceId propertyConsider adding documentation to explain the purpose and format requirements of the workspaceId property.
Add JSDoc comments to both interfaces:
export interface PKCEAuthenticationConfig extends WebAuthenticationConfig { code_challenge_method?: string; + /** Workspace ID for organization-specific authentication. Must be alphanumeric with hyphens and underscores only. */ workspaceId?: string; } export interface DeviceCodeConfig { baseURL?: string; clientId: string; + /** Workspace ID for organization-specific authentication. Must be alphanumeric with hyphens and underscores only. */ workspaceId?: string; }Also applies to: 307-307
packages/coze-js/test/auth.spec.ts (2)
130-140: LGTM! Consider adding edge cases.The test correctly validates the PKCE authentication URL construction with workspaceId.
Consider adding test cases for:
- Invalid workspaceId formats
- Empty workspaceId
- Special characters in workspaceId that might need URL encoding
200-222: Enhance test coverage and consistency.While the test correctly validates the workspace-specific endpoint, consider these improvements:
- Add APIClient initialization verification similar to other tests:
expect(APIClient).toHaveBeenCalledWith({ token: '', baseURL: mockConfig.baseURL, });
- Verify the response handling:
const response = await getDeviceCode({...}); expect(response).toEqual({ device_code: 'test-device-code' });
- Align the test name with other tests in the file (e.g., "should call APIClient.post with correct parameters when workspaceId is provided")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
common/config/subspaces/default/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
config/vitest-config/src/preset-default.ts(1 hunks)examples/coze-js-node/src/auth/auth-oauth-device.ts(2 hunks)packages/coze-js/src/auth.ts(4 hunks)packages/coze-js/test/auth.spec.ts(2 hunks)packages/realtime-api/test/client.spec.ts(2 hunks)
🔇 Additional comments (4)
examples/coze-js-node/src/auth/auth-oauth-device.ts (1)
10-15: LGTM: Import changes are well-organized
The new refreshOAuthToken import is appropriately grouped with related API functions.
packages/coze-js/src/auth.ts (1)
67-72: Verify test coverage and usage
Please ensure that:
- Test cases cover both workspace ID and non-workspace ID scenarios
- All existing code using these functions is updated to handle the new workspace ID parameter
Let's verify the test coverage and usage:
Also applies to: 160-165, 278-278, 307-307
✅ Verification successful
Based on the results, I can now generate the final response:
Test coverage and usage verification completed
The changes are well-covered and properly implemented:
- Test coverage exists for both workspace ID (
test/auth.spec.ts) and non-workspace ID scenarios - All existing usage of
getPKCEAuthenticationUrlandgetDeviceCodefunctions are properly handling the optionalworkspaceIdparameter through their config objects - No instances of direct parameter passing found that would require updates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check test coverage for workspace ID functionality
echo "Checking test coverage..."
rg -l "workspaceId.*test|test.*workspaceId" --type ts
# Find usage of affected functions
echo -e "\nChecking function usage..."
rg -l "getPKCEAuthenticationUrl|getDeviceCode" --type ts
# Look for potential missed updates in function calls
echo -e "\nChecking for potential missed updates..."
rg "getPKCEAuthenticationUrl\(|getDeviceCode\(" -A 2 --type ts
Length of output: 3504
packages/realtime-api/test/client.spec.ts (2)
3-3: LGTM! Clean import statement.
The import is properly placed and uses appropriate namespace import syntax.
100-100: LGTM! Good refactoring of device enumeration logic.
Moving the device enumeration logic to RealtimeUtils improves code maintainability by centralizing this functionality. The test cases properly validate both success and error scenarios.
Let's verify the utility implementation:
Also applies to: 107-107
✅ Verification successful
Let me gather more information about the implementation of getAudioDevices with a different pattern since the previous one didn't match.
Verified: Device enumeration is properly centralized in RealtimeUtils
The implementation in utils.ts is clean and follows best practices:
- Single source of truth for device enumeration via
VERTC.enumerateDevices() - Proper filtering of audio input/output devices
- Well-documented with JSDoc comments
- Handles empty device list gracefully
The ripgrep results confirm that all enumerateDevices calls outside of utils.ts are mock implementations in test files, indicating proper encapsulation of this functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the RealtimeUtils implementation and its usage
# Expected: The utility should be the single source of truth for device enumeration
# Check if getAudioDevices is the only method accessing VERTC.enumerateDevices
rg -p "enumerateDevices" --type ts
# Verify the implementation of getAudioDevices
ast-grep --pattern 'export function getAudioDevices() {
$$$
}'
Length of output: 874
Script:
#!/bin/bash
# Check the implementation of getAudioDevices in RealtimeUtils
ast-grep --pattern 'getAudioDevices = async () => {
$$$
}'
# Also try with static method pattern
ast-grep --pattern 'static getAudioDevices() {
$$$
}'
# Let's also see the full utils.ts file since we know it's there
cat packages/realtime-api/src/utils.ts
Length of output: 1319
jackshen310
force-pushed
the
feat/pkce-workspace
branch
from
bc5c521 to
94414bb
Compare
jackshen310
force-pushed
the
feat/pkce-workspace
branch
from
94414bb to
4a9b6e3
Compare
2 participants
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests