Add client_secret to device authorization request#517
Conversation
|
|
📝 WalkthroughWalkthroughThe change adds explicit inclusion of Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
🧹 Nitpick comments (1)
internal/auth/device_flow_test.go (1)
136-138: Prefer parsing form data over substring matching in the body assertion.At Line 137,
strings.Containscan miss edge cases when values are URL-encoded. Parse the form and assert the decoded field value directly.Suggested improvement
+import "net/url" ... - body := string(stub.CapturedBody) - if !strings.Contains(body, "client_secret=secret_b") { - t.Errorf("expected client_secret in form body, got %q", body) - } + values, err := url.ParseQuery(string(stub.CapturedBody)) + if err != nil { + t.Fatalf("failed to parse form body: %v", err) + } + if got := values.Get("client_secret"); got != "secret_b" { + t.Errorf("expected client_secret=secret_b in form body, got %q (raw=%q)", got, string(stub.CapturedBody)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auth/device_flow_test.go` around lines 136 - 138, Replace the brittle substring check against stub.CapturedBody with proper form parsing: parse the body string (from stub.CapturedBody / variable body) using url.ParseQuery (or equivalent form decoder) and assert that values.Get("client_secret") == "secret_b"; update the test assertion in the device flow test (where body := string(stub.CapturedBody) and the strings.Contains check occurs) to use the parsed form values instead of substring matching so URL-encoded payloads are handled correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/auth/device_flow_test.go`:
- Around line 136-138: Replace the brittle substring check against
stub.CapturedBody with proper form parsing: parse the body string (from
stub.CapturedBody / variable body) using url.ParseQuery (or equivalent form
decoder) and assert that values.Get("client_secret") == "secret_b"; update the
test assertion in the device flow test (where body := string(stub.CapturedBody)
and the strings.Contains check occurs) to use the parsed form values instead of
substring matching so URL-encoded payloads are handled correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0a99296a-44f4-4f1b-9a05-b8b15cf90592
📒 Files selected for processing (2)
internal/auth/device_flow.gointernal/auth/device_flow_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #517 +/- ##
=======================================
Coverage ? 59.05%
=======================================
Files ? 384
Lines ? 32637
Branches ? 0
=======================================
Hits ? 19275
Misses ? 11553
Partials ? 1809 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@d2cd6e4bb9de1512ca6c1196419eed8f1b1cb421🧩 Skill updatenpx skills add utafrali/cli#fix/issue-509-lark-cli-auth-login-device-authorization -y -g |
|
Thank you for your contribution |
This reverts commit 663c24a.
Summary
Device auth was failing because the endpoint requires client_secret but we weren't sending it. Added the missing parameter and a test to verify it gets included.
Changes
Added client_secret to the form payload in RequestDeviceAuthorization. Also added a test case that checks client_secret is in the request body.
Test Plan
New test passes. Tested locally with lark auth login and it works now.
Related Issues
Fixes #509
Summary by CodeRabbit