-
Notifications
You must be signed in to change notification settings - Fork 10
[client] add JWT support #79
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
|
Warning Rate limit exceeded@capcom6 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (8)
WalkthroughAdds JWT token-based authentication alongside existing Basic Auth: introduces TokenRequest and TokenResponse domain classes, extends Client with GenerateToken and RevokeToken methods, updates Client constructor/authorization handling to support bearer tokens, updates README docs, adds tests, and tweaks Makefile phpstan memory flag. Changes
Sequence DiagramsequenceDiagram
participant ClientApp as Client Code
participant SDK as SDK Client
participant API as API Server
rect rgb(230,245,255)
Note over ClientApp,API: JWT token lifecycle
ClientApp->>SDK: GenerateToken(TokenRequest)
SDK->>API: POST /3rdparty/v1/auth/token (Basic Auth)
API-->>SDK: TokenResponse (accessToken, tokenType, id, expiresAt)
SDK-->>ClientApp: TokenResponse
ClientApp->>SDK: __construct(?login, passwordOrToken)
SDK->>SDK: store Authorization header (Bearer or Basic)
ClientApp->>SDK: SendMessage(SmsMessage)
SDK->>API: POST /3rdparty/v1/messages (Authorization: Bearer {token})
API-->>SDK: MessageResponse
SDK-->>ClientApp: MessageResponse
ClientApp->>SDK: RevokeToken(jti)
SDK->>API: DELETE /3rdparty/v1/auth/token/{jti} (Authorization)
API-->>SDK: 204 No Content
SDK-->>ClientApp: void
end
rect rgb(245,245,230)
Note over ClientApp,API: Basic Authentication (legacy)
ClientApp->>SDK: __construct(login, password)
SDK->>SDK: store Basic auth header
ClientApp->>SDK: SendMessage(SmsMessage)
SDK->>API: POST /3rdparty/v1/messages (Authorization: Basic ...)
API-->>SDK: MessageResponse
SDK-->>ClientApp: MessageResponse
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 1
🧹 Nitpick comments (5)
src/Domain/TokenRequest.php (1)
1-67: TokenRequest VO is clean and matches the intended JSON shapeThe value object is straightforward, typed, and its
toObject/FromObjectbehavior (including omittingttlwhen null and defaulting to[]/null) aligns with how the tests and client use it.If you expect the API to ever send unexpected types for
scopesorttl, you could defensively coerce inFromObject(e.g.,(array)($obj->scopes ?? [])) to avoidTypeErrors, but it’s not strictly necessary given current usage.README.md (1)
8-215: JWT documentation accurately reflects the new API; minor example tweakThe new Authentication/JWT sections line up well with the
Client,TokenRequest, andTokenResponseAPIs and clearly show both Basic and JWT flows.One small improvement: in the “Using a JWT Token” example (around Line 155), you use
MessageBuilderwithout an explicitusein that snippet. Either repeat theuse AndroidSmsGateway\Domain\MessageBuilder;line or mention that this snippet continues from the earlier Quickstart example to avoid confusion when copy‑pasting.tests/ClientTest.php (1)
11-12: New token/JWT client tests cover the critical pathsThe added tests for
GenerateToken,RevokeToken, and JWT‑configured clients correctly verify:
- HTTP verb and URL (
/auth/tokenand/auth/token/{id}),- Authorization header for both Basic and Bearer flows,
- Response mapping into
TokenResponse.You’ve covered the main behavioral surface for the new API.
If you want to lock down the request schema further, you could add an assertion in
testGenerateTokenthat the request body JSON matches the expectedscopes/ttl, but it’s not strictly required.Also applies to: 389-453
src/Client.php (1)
26-56: JWT support wiring (auth header, GenerateToken/RevokeToken) looks correct
- The new
$authHeaderfield and constructor logic correctly choose betweenBasicandBearerbased on whether$loginis null.sendRequestnow consistently addsAuthorization: {Basic|Bearer ...}plus the existingUser-Agent, which aligns with both the README examples and the tests.GenerateTokenandRevokeTokenuse the expected/auth/tokenand/auth/token/{jti}paths and reuse the common request/response pipeline, with properis_objectvalidation and mapping viaTokenResponse::FromObject.Overall, the JWT integration in
Clientis cohesive and matches the new tests and documentation.If you ever want stronger typing on
$payload, you could consider type‑hinting it as?Interfaces\SerializableInterfaceinsendRequestand adjusting call sites accordingly, but that would be a separate, larger refactor.Also applies to: 383-417, 433-440
src/Domain/TokenResponse.php (1)
1-82: TokenResponse implementation aligns with client and test usageThe mapping between internal properties and the wire format (
access_token,token_type,id,expires_at) is clear, andFromObject’s empty‑string defaults match the tests and keep the object usable even with partial responses.If you ever need richer date handling, you might eventually wrap
expiresAtin a\DateTimeImmutableor similar, but the current string representation is perfectly adequate for now.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Makefile(1 hunks)README.md(4 hunks)src/Client.php(5 hunks)src/Domain/TokenRequest.php(1 hunks)src/Domain/TokenResponse.php(1 hunks)tests/ClientTest.php(2 hunks)tests/Domain/TokenRequestTest.php(1 hunks)tests/Domain/TokenResponseTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/Client.php (2)
src/Domain/TokenRequest.php (2)
TokenRequest(7-67)FromObject(61-66)src/Domain/TokenResponse.php (2)
TokenResponse(7-83)FromObject(75-82)
src/Domain/TokenRequest.php (1)
src/Domain/TokenResponse.php (3)
__construct(13-23)toObject(61-69)FromObject(75-82)
tests/ClientTest.php (3)
src/Domain/TokenRequest.php (1)
TokenRequest(7-67)src/Domain/TokenResponse.php (5)
TokenResponse(7-83)AccessToken(25-27)TokenType(34-36)ID(43-45)ExpiresAt(52-54)src/Client.php (4)
GenerateToken(389-402)RevokeToken(410-417)Client(22-460)SendMessage(81-110)
src/Domain/TokenResponse.php (2)
src/Client.php (1)
__construct(35-56)src/Domain/TokenRequest.php (3)
__construct(16-19)toObject(46-55)FromObject(61-66)
tests/Domain/TokenResponseTest.php (1)
src/Domain/TokenResponse.php (11)
TokenResponse(7-83)AccessToken(25-27)TokenType(34-36)ID(43-45)ExpiresAt(52-54)setAccessToken(29-32)setTokenType(38-41)setId(47-50)setExpiresAt(56-59)toObject(61-69)FromObject(75-82)
tests/Domain/TokenRequestTest.php (1)
src/Domain/TokenRequest.php (7)
TokenRequest(7-67)Scopes(24-26)TTL(37-39)setScopes(32-35)setTtl(41-44)toObject(46-55)FromObject(61-66)
🔇 Additional comments (3)
Makefile (1)
17-17: PHPStan memory limit flag is a sensible tweakBumping phpstan’s memory limit to 256M is a pragmatic change for larger analyses and doesn’t affect runtime behavior of the library.
tests/Domain/TokenResponseTest.php (1)
1-81: Comprehensive TokenResponse test coverageThe tests exercise construction, all accessors/mutators,
toObject, andFromObject(including default values), which is exactly what this VO needs.tests/Domain/TokenRequestTest.php (1)
1-91: TokenRequest tests cover all key behaviorsCreation, setters,
toObject, andFromObject(including ttl presence/absence and default values) are all exercised, which gives solid confidence in this VO’s behavior.
b3617b2 to
ad711dd
Compare
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.