-
Notifications
You must be signed in to change notification settings - Fork 4
[client] add JWT support #83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
WalkthroughAdds JWT Bearer support alongside Basic Auth, introduces token lifecycle APIs ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client
participant API as SMSGate API
rect rgb(248,249,255)
note over User,Client: Obtain JWT (optional) using Basic credentials
User->>Client: new Client(login, password)
Client->>API: POST /token (Authorization: Basic ...)
API-->>Client: 200 TokenResponse
User->>Client: new Client("", access_token)
end
rect rgb(240,255,240)
note over User,Client: JWT-authenticated operations
User->>Client: sendMessage(...)
Client->>API: POST /messages (Authorization: Bearer access_token)
API-->>Client: 200 OK
User->>Client: getState()
Client->>API: GET /state (Authorization: Bearer access_token)
API-->>Client: 200 State
end
rect rgb(255,250,240)
note over User,Client: Token lifecycle management
User->>Client: generateToken({scopes, ttl})
Client->>API: POST /token (Auth: Basic or Bearer)
API-->>Client: 200 TokenResponse
User->>Client: revokeToken(jti)
Client->>API: POST /token/revoke (Auth)
API-->>Client: 204 No Content
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (2)src/client.test.ts (3)
src/client.ts (2)
🔇 Additional comments (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(9 hunks)src/client.test.ts(3 hunks)src/client.ts(3 hunks)src/domain.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/client.test.ts (3)
src/client.ts (2)
Client(18-320)BASE_URL(16-16)src/http.ts (1)
HttpClient(1-7)src/domain.ts (4)
Message(52-88)MessageState(32-47)TokenRequest(477-487)TokenResponse(492-512)
src/client.ts (2)
src/http.ts (1)
HttpClient(1-7)src/domain.ts (2)
TokenRequest(477-487)TokenResponse(492-512)
🪛 Gitleaks (8.29.0)
src/client.test.ts
[high] 350-350: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 536-536: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 563-563: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (5)
src/domain.ts (1)
474-512: JWT token domain types look consistent with client and docs
TokenRequest/TokenResponseare well-shaped for the new token endpoints and align with howClient.generateToken/revokeTokenand the README describe them. No changes needed here.src/client.test.ts (1)
434-491: Backward-compatibility tests correctly guard the Basic Auth pathThe “Client Backward Compatibility” block re-asserts the legacy Basic Auth behavior (including the missing-password error), which is important after changing the constructor semantics. These tests look correct and should help prevent regressions.
README.md (1)
281-311: Authentication and JWT token management docs are consistent with the client API
- The constructor arguments table and “Authentication Configuration” examples correctly describe the new
Client(login, passwordOrToken, httpClient?, baseUrl?)semantics and how Basic vs JWT is selected based onlogin.- The Core Methods table and JWT Token Management rows (
generateToken,revokeToken) match the signatures and behavior insrc/client.ts.- The
TokenRequest/TokenResponsetype snippets mirror the domain definitions, which helps keep the docs and types aligned.No changes needed here.
Also applies to: 342-345, 412-446
src/client.ts (2)
23-52: Constructor changes correctly separate Basic vs JWT auth and keep legacy usage workingThe updated constructor and header setup:
- Correctly distinguish JWT (
login === "") from Basic (login !== "") and enforce the right required value (passwordOrToken) in each case.- Preserve existing call sites like
new Client('login', 'password', httpClient)while enabling the new two-arg form that uses the built-in HTTP client.- Produce Authorization headers (
Bearer/Basic <base64>) that match the new tests and README examples.This looks coherent and backward compatible.
293-319: Token generation and revocation methods correctly wrap the new endpoints
generateTokenandrevokeToken:
- Use the expected
/auth/tokenand/auth/token/{jti}URLs.- Apply the same header conventions as other JSON endpoints (including
Content-Typewhere appropriate).- Respect the
TokenRequest/TokenResponsetypes from the domain layer.These methods look correct and are well-covered by the new tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(9 hunks)src/client.test.ts(3 hunks)src/client.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/client.test.ts (3)
src/client.ts (2)
Client(18-337)BASE_URL(16-16)src/http.ts (1)
HttpClient(1-7)src/domain.ts (3)
Message(52-88)TokenRequest(477-487)TokenResponse(492-512)
🔇 Additional comments (13)
src/client.test.ts (4)
14-17: LGTM! Import additions align with JWT feature.The new imports (TokenRequest, TokenResponse, WebHookEventType) correctly support the JWT authentication and token management test additions.
346-432: LGTM! JWT authentication tests are comprehensive and address past feedback.The JWT auth tests properly verify:
- Client creation with JWT tokens
- Bearer token authentication headers
- Error handling for missing tokens
The use of
'fake-token-123'successfully avoids triggering secret scanners, resolving the previous gitleaks concern.
434-491: LGTM! Backward compatibility tests ensure Basic Auth still works.The tests properly verify that the legacy authentication path (login + password) continues to function correctly, including:
- Client initialization
- Basic Auth header generation
- Error handling for missing passwords
514-601: LGTM! Token management tests thoroughly cover the JWT lifecycle.The tests properly verify:
- Token generation with and without TTL
- Token revocation by JWT ID
- Correct HTTP methods and endpoints
- Response structure validation
The use of a Basic Auth client to perform token operations follows the expected workflow where initial authentication is required to generate JWT tokens.
src/client.ts (4)
10-12: LGTM! Import additions support new functionality.The added imports (MessagesExportRequest, TokenRequest, TokenResponse) are all used in the file and correctly support the JWT token management and inbox export features.
23-52: LGTM! Constructor cleanly supports both authentication modes.The updated constructor properly:
- Accepts optional httpClient with sensible fallback
- Distinguishes JWT (empty login) from Basic Auth (non-empty login)
- Validates credentials for each mode with clear error messages
- Uses correct header formats (Bearer vs Basic)
Note: Line 50 uses
Buffer.from(), which is Node.js-specific. This is appropriate given the README states the library is server-side focused and requires Node.js v18+.
54-110: LGTM! Default HTTP client properly handles 204, errors, and content types.The implementation successfully addresses previous feedback by:
- Returning null for 204 No Content responses (avoiding JSON parse errors)
- Providing clear error messages with HTTP status and response text
- Checking Content-Type before attempting JSON parsing
- Centralizing response handling across all HTTP methods
This ensures methods like
deleteWebhook,deleteDevice, andrevokeTokenwork correctly when the server returns 204.
310-336: LGTM! Token management methods follow established patterns.Both
generateTokenandrevokeTokenare implemented consistently with other client methods:
- Correct HTTP methods and endpoints
- Proper header configuration
- Appropriate return types
The methods integrate seamlessly with the existing client API surface.
README.md (5)
44-65: LGTM! Authentication explanation is clear and well-structured.The new authentication section effectively:
- Distinguishes Basic Auth from JWT
- Provides clear guidance on when to use each method
- Positions JWT as the production-recommended approach
- Balances simplicity with security considerations
143-176: LGTM! Quickstart correctly uses.stateproperty.The example properly references
updatedState.stateat line 159, correctly matching theMessageStateinterface. This resolves the past review concern about using.statusincorrectly.The JWT-based workflow (generate token → create client → send message → check state → revoke token) provides a complete and realistic usage example.
281-310: LGTM! Authentication configuration examples are clear and correct.The side-by-side examples effectively demonstrate both authentication modes, and the automatic detection explanation helps users understand how the client determines which method to use.
412-446: LGTM! JWT type definitions are accurate and well-documented.The TokenRequest and TokenResponse interface definitions match the actual types in src/domain.ts and include helpful JSDoc comments for developers.
465-476: LGTM! Security notes appropriately cover both authentication modes.The updated security guidance includes:
- Existing best practices for credentials
- JWT-specific recommendations (TTL, scoping, revocation)
- Principle of least privilege
The notes provide comprehensive security guidance for both Basic Auth and JWT users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(9 hunks)
🔇 Additional comments (6)
README.md (6)
142-159: ✅ Past issue resolved:updatedState.stateis now correct.The previous review flagged incorrect use of
updatedState.statusat line 143 (old numbering). The current example correctly usesupdatedState.stateat line 158, aligning with theMessageStateinterface.
129-140: ✅ Past issue resolved: undefinedhttpFetchClientreference removed.The previous review flagged an undefined
httpFetchClientreference. The current Quickstart example correctly instantiates the JWT client without the third parameter (lines 134–137), relying on the built-in HTTP client fallback.
44-307: Excellent authentication documentation and clear JWT/Basic auth distinction.The new Authentication section (lines 44–65) clearly delineates when to use Basic vs. JWT, and the Authentication Configuration subsection (lines 287–307) provides side-by-side examples with automatic method detection logic. The Quickstart JWT workflow (lines 102–176) demonstrates the full token lifecycle: generation, client initialization, message sending, and revocation. This is a significant improvement.
339-342: JWT token management methods properly documented.The Core Methods table now includes
generateToken(request: TokenRequest)andrevokeToken(jti: string)with clear descriptions and return types. Good addition.
409-443: TokenRequest and TokenResponse types clearly defined.Both types are well-documented with JSDoc comments explaining
scopes,ttl, and token metadata fields (access_token,token_type,id,expires_at). Consistent with the implementation described in the AI summary.
462-473: Security notes updated with JWT-specific guidance.New bullet points covering token TTL, least privilege, and token revocation workflows strengthen the security guidance. These align with JWT best practices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(9 hunks)src/client.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/client.ts (2)
src/http.ts (1)
HttpClient(1-7)src/domain.ts (2)
TokenRequest(477-487)TokenResponse(492-512)
🔇 Additional comments (13)
src/client.ts (4)
10-12: LGTM!The addition of
TokenRequestandTokenResponseimports properly supports the new JWT token management features.
54-110: Excellent implementation addressing previous concerns!The default HTTP client now properly handles:
- 204 No Content responses (returns null)
- HTTP errors with meaningful error messages
- Content-Type detection to avoid JSON parse errors on non-JSON responses
This resolves the issues raised in the previous review about methods like
deleteWebhook,revokeToken, etc. breaking on 204 responses. Based on past review comments, this is exactly what was requested.
310-323: LGTM!The
generateTokenmethod follows the established pattern for API methods and provides proper type safety with TokenRequest (scopes and ttl parameters) and TokenResponse (access_token, token_type, id, expires_at fields).
325-336: LGTM!The
revokeTokenmethod is correctly implemented and will properly handle the expected 204 No Content response thanks to the improved default HTTP client.README.md (9)
1-1: LGTM!The updated title "📱 SMSGate JS/TS API Client" is clear and consistent with the project focus.
44-65: Excellent documentation of authentication methods!The new Authentication section provides clear guidance on when to use Basic vs JWT authentication, helping users make informed decisions based on their use case (development vs production, security requirements, etc.).
102-176: Excellent comprehensive example addressing previous issues!The Quickstart now demonstrates a complete JWT workflow:
- Basic Auth client for token generation
- JWT token generation with scopes and TTL
- JWT-authenticated client initialization
- Message sending and state checking
- Token revocation
Previous review issues have been fixed:
- The undefined
httpFetchClientreference has been removed (lines 134-137 now correctly omit the httpClient parameter)- Line 158 now correctly uses
updatedState.stateinstead ofupdatedState.statusBased on past review comments, these were the exact changes requested.
280-286: Configuration table correctly reflects optional httpClient!The table now properly shows httpClient with
fetchas the default, which aligns with the implementation where httpClient is an optional parameter with a default fetch-based client. This addresses the previous review concern about marking it as "Required". Based on past review comments, this is the correct representation.
287-308: Clear authentication configuration guidance!The Authentication Configuration section effectively explains:
- How to configure Basic Authentication (with username and password)
- How to configure JWT Authentication (empty string for login, token for password)
- The automatic detection mechanism
This makes the dual-auth implementation intuitive for users.
340-342: LGTM!The JWT Token Management methods are properly documented in the Core Methods table with correct signatures and return types.
409-443: LGTM!The JWT Authentication type definitions are well-documented and match the actual interfaces, providing users with clear guidance on the token request/response structures.
450-450: LGTM!The updated HTTP Clients section correctly reflects that the library comes with a fetch-based built-in HTTP client, matching the getDefaultHttpClient implementation.
469-473: Excellent security guidance!The updated Security Notes provide important JWT-specific recommendations including password rotation, strong passwords, appropriate TTL values, least privilege, and token revocation workflows. These are essential best practices for production JWT implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/client.ts (2)
src/http.ts (1)
HttpClient(1-7)src/domain.ts (2)
TokenRequest(477-487)TokenResponse(492-512)
🔇 Additional comments (3)
src/client.ts (3)
310-323: LGTM!The
generateTokenmethod correctly implements JWT token generation with proper headers and type safety.
325-336: LGTM!The
revokeTokenmethod correctly implements JWT token revocation. With the 204 handling ingetDefaultHttpClient, this will work properly even if the server returns no content.
50-50: The original review comment is incorrect. The project explicitly requires Node.js v18+ (as stated in README.md), andbtoa()is available in Node.js v16.0.0+. Since v18 exceeds the minimum version wherebtoa()is supported, there is no compatibility issue.Likely an incorrect or invalid review comment.
eca215d to
23653ed
Compare
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.