feat: ✨ add support for QR code login#174
Conversation
# Conflicts: # pkg/connector/e2ee_keys.go
# Conflicts: # pkg/connector/client.go # pkg/line/client.go # pkg/runner.go
|
Warning Review limit reached
More reviews will be available in 6 minutes and 37 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThis PR adds QR-code-based login as the primary authentication flow for the LINE bridge, alongside email/password as a secondary option. It introduces multi-key E2EE infrastructure to support multiple login key derivations, implements the complete LINE QR protocol with session/code/verification/PIN steps, synchronizes token updates with a mutex, and refactors completion helpers to use parameterized login key IDs. ChangesQR Code Login with Multi-Key E2EE Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/connector/connector.go (1)
550-560:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersist the verified MID into login metadata.
LoadUserLoginrebuildsLineClient.MidfromUserLoginMetadata.Mid, but this metadata is populated fromres.Midwhile the canonical identity here is alreadyprofile.Mid. When QR login omitsmid, the stored login reloads with an empty MID after restart even thoughul.IDpoints at the real LINE account.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/connector/connector.go` around lines 550 - 560, The UserLoginMetadata.Mid is being set from res.Mid but the canonical verified MID is profile.Mid; update the metadata to persist the verified MID (set meta.Mid = profile.Mid) before calling fetchLoginKeys and before creating the login via user.NewLogin (ensure UserLoginMetadata passed to NewLogin contains profile.Mid rather than res.Mid) so that LineClient.Mid rebuilds correctly on reload; reference UserLoginMetadata, meta.Mid, profile.Mid, res.Mid, fetchLoginKeys, and user.NewLogin when making the change.
🧹 Nitpick comments (3)
pkg/connector/handlers/handler.go (3)
36-36: ⚡ Quick winExtract magic string to a named constant.
The error message literal
"OBS download failed (401)"should be extracted as a package-level constant to improve maintainability and make the error contract explicit.♻️ Proposed constant extraction
At package level:
const obsAuthFailureMsg = "OBS download failed (401)"Then update the check:
-isOBSAuthFailure := strings.Contains(err.Error(), "OBS download failed (401)") +isOBSAuthFailure := strings.Contains(err.Error(), obsAuthFailureMsg)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/connector/handlers/handler.go` at line 36, Extract the magic string "OBS download failed (401)" into a package-level constant (e.g., obsAuthFailureMsg) and replace the inline literal used in the strings.Contains check inside handler.go where isOBSAuthFailure is computed; update any other occurrences of the same literal in the file to reference the new constant so the error contract is centralized and maintainable (ensure the constant name is unique and placed at top of the package).
36-40: ⚡ Quick winAdd explanatory comment for the early-return path.
The OBS auth failure recovery bypasses the general 401/refresh/logout handling below (lines 41-48). This non-obvious early return deserves a comment explaining why OBS auth failures are handled differently (the OBS token is derived from the main token; when the main token was already refreshed elsewhere, the cached OBS token becomes stale and just needs cache invalidation, not a full token recovery).
📝 Proposed comment
+// OBS auth failures require clearing the derived OBS token cache rather than +// refreshing the main token; the main token may have been rotated elsewhere, +// leaving the OBS cache stale. Clear it and return a fresh client. isOBSAuthFailure := strings.Contains(err.Error(), "OBS download failed (401)") if isOBSAuthFailure {Based on learnings from context snippet 2: "The OBS token is derived from the main LINE access token; when the latter is rotated (refresh or re-login) any previously-issued OBS token is invalidated server-side, but the cache here would keep handing it out until its original TTL expires."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/connector/handlers/handler.go` around lines 36 - 40, Add a clear explanatory comment above the OBS-auth-failure early-return (the isOBSAuthFailure check) stating that OBS tokens are derived from the main LINE access token and can become stale when the main token is rotated elsewhere, so we only clear the OBS token cache (call line.ClearEncryptedAccessTokenCache()) and instantiate a new client (h.NewClient()) instead of invoking the general 401/refresh/logout logic; reference the isOBSAuthFailure variable, line.ClearEncryptedAccessTokenCache(), and h.NewClient() in the comment to make the rationale and control-flow choice explicit.
36-40: ⚡ Quick winAdd logging for OBS auth failure recovery path.
The general token recovery path logs a warning on failure (line 45), but the OBS-specific recovery path is silent. For consistency and observability, consider adding a structured log entry when clearing the OBS token cache.
📊 Proposed logging addition
isOBSAuthFailure := strings.Contains(err.Error(), "OBS download failed (401)") if isOBSAuthFailure { + h.Log.Debug().Str("error", err.Error()).Msg("Clearing stale OBS token cache after auth failure") line.ClearEncryptedAccessTokenCache() return h.NewClient(), true }As per coding guidelines: "Use
zerologfor logging throughout the codebase" and "Do not useMsgfin logging; useMsgwith structured fields instead."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/connector/handlers/handler.go` around lines 36 - 40, Add a zerolog structured warning when the OBS-specific recovery path triggers: detect the OBS auth failure (isOBSAuthFailure) and log a warning with fields such as error (err.Error()), action="clearing_encrypted_access_token_cache", and any identifying info available on the line or handler (e.g., line.ID or h.Name) before calling line.ClearEncryptedAccessTokenCache() and returning h.NewClient(); use zerolog.Msg with fields (not Msgf) to match project logging conventions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/connector/client.go`:
- Around line 145-146: Multiple goroutines still read/write lc.AccessToken and
lc.RefreshToken without using the new tokenMu, causing races; ensure all token
access is serialized either by centralizing token replacement into a single
locked helper (e.g., create a setTokensLocked method used by refreshAndSave) or
by taking lc.tokenMu in every place that reads/writes tokens (notably tryLogin,
Connect, and the E2EE recovery codepaths) so every read/write of
lc.AccessToken/lc.RefreshToken is protected by lc.tokenMu.
In `@pkg/connector/connector.go`:
- Around line 248-259: The current logic treats any error from
lq.client.VerifyCertificate as a rejection and falls back to creating a PIN;
instead, change the VerifyCertificate error handling so only an explicit
certificate-rejected response triggers the PIN path—propagate any
transport/5xx/unknown errors back to the caller. Concretely, inspect the error
(or returned status) from lq.client.VerifyCertificate and if it indicates
"certificate rejected" (use whatever sentinel/error type the client returns)
then call lq.client.CreatePinCode(lq.AuthSessionID) and proceed; otherwise
return the VerifyCertificate error immediately. Keep references to
lq.client.VerifyCertificate, lq.client.CreatePinCode, QRCodeLoginV2,
finishLineLoginWithLoginKey and lq.AuthSessionID to locate and update the
branch.
- Around line 196-223: CreateQRCode currently returns longPollingMaxCount and
longPollingIntervalSec but the flow ignores them and calls
CheckQRCodeVerifiedContext only once; change the code that calls
lq.client.CreateQRCode to capture the returned longPollingMaxCount and
longPollingIntervalSec and use them to repeatedly call
lq.client.CheckQRCodeVerifiedContext (and likewise use longPolling values for
CheckPinCodeVerifiedContext in the PIN flow) by updating lq.startPoll (or
wrapping the provided checker) so it retries up to longPollingMaxCount times
with longPollingIntervalSec between attempts and only fails after exhausting
those retries or on an explicit error; reference symbols:
lq.client.CreateQRCode, longPollingMaxCount, longPollingIntervalSec,
lq.startPoll, lq.client.CheckQRCodeVerifiedContext, CheckPinCodeVerifiedContext.
In `@pkg/connector/e2ee_keys.go`:
- Around line 112-116: The retry logic around E2EE public-key calls currently
only checks lc.isRefreshRequired(err); change both places (the
NegotiateE2EEPublicKey and GetE2EEPublicKey retry blocks) to also treat
lc.isLoggedOut(err) as recoverable by using a combined condition (e.g., if err
!= nil && (lc.isRefreshRequired(err) || lc.isLoggedOut(err))) and then call
lc.recoverToken(ctx) as before; after successful recoverToken set client =
line.NewClient(lc.AccessToken) and re-invoke the respective method
(NegotiateE2EEPublicKey or GetE2EEPublicKey).
In `@pkg/runner.go`:
- Around line 343-345: LoginUnwrapKeyChainWithKey allows selecting a login key
for unwrap but GenerateConfirmHash still reads the mutable r.loginCurveKey, so
concurrent logins can clobber confirm hashing; add a keyed confirm-hash path
(e.g. GenerateConfirmHashWithKey(loginKeyID int, ...)) that mirrors
LoginUnwrapKeyChainWithKey’s lookup logic and uses the selected key instead of
r.loginCurveKey, update confirmE2EELogin to accept and pass loginKeyID to the
new GenerateConfirmHashWithKey, and then thread that loginKeyID through callers
(including places that call GenerateE2EESecret/GetRunner flows) so confirmation
uses the stable per-key path rather than the mutable r.loginCurveKey.
---
Outside diff comments:
In `@pkg/connector/connector.go`:
- Around line 550-560: The UserLoginMetadata.Mid is being set from res.Mid but
the canonical verified MID is profile.Mid; update the metadata to persist the
verified MID (set meta.Mid = profile.Mid) before calling fetchLoginKeys and
before creating the login via user.NewLogin (ensure UserLoginMetadata passed to
NewLogin contains profile.Mid rather than res.Mid) so that LineClient.Mid
rebuilds correctly on reload; reference UserLoginMetadata, meta.Mid,
profile.Mid, res.Mid, fetchLoginKeys, and user.NewLogin when making the change.
---
Nitpick comments:
In `@pkg/connector/handlers/handler.go`:
- Line 36: Extract the magic string "OBS download failed (401)" into a
package-level constant (e.g., obsAuthFailureMsg) and replace the inline literal
used in the strings.Contains check inside handler.go where isOBSAuthFailure is
computed; update any other occurrences of the same literal in the file to
reference the new constant so the error contract is centralized and maintainable
(ensure the constant name is unique and placed at top of the package).
- Around line 36-40: Add a clear explanatory comment above the OBS-auth-failure
early-return (the isOBSAuthFailure check) stating that OBS tokens are derived
from the main LINE access token and can become stale when the main token is
rotated elsewhere, so we only clear the OBS token cache (call
line.ClearEncryptedAccessTokenCache()) and instantiate a new client
(h.NewClient()) instead of invoking the general 401/refresh/logout logic;
reference the isOBSAuthFailure variable, line.ClearEncryptedAccessTokenCache(),
and h.NewClient() in the comment to make the rationale and control-flow choice
explicit.
- Around line 36-40: Add a zerolog structured warning when the OBS-specific
recovery path triggers: detect the OBS auth failure (isOBSAuthFailure) and log a
warning with fields such as error (err.Error()),
action="clearing_encrypted_access_token_cache", and any identifying info
available on the line or handler (e.g., line.ID or h.Name) before calling
line.ClearEncryptedAccessTokenCache() and returning h.NewClient(); use
zerolog.Msg with fields (not Msgf) to match project logging conventions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0849f30-f280-46b1-a2ad-dd352ededd41
📒 Files selected for processing (12)
README.mdpkg/connector/client.gopkg/connector/connector.gopkg/connector/e2ee_keys.gopkg/connector/handlers/handler.gopkg/e2ee/manager.gopkg/line/client.gopkg/line/methods.gopkg/line/qr.gopkg/line/secret/secret.gopkg/line/structs.gopkg/runner.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint with 1.25
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Usego fmtfor code formatting across all Go files
Usegoimportswith-local "github.com/highesttt/matrix-line-messenger"flag to group project-local imports correctly
Usezerologfor logging throughout the codebase
Do not useMsgfin logging; useMsgwith structured fields instead
UseStringerinterface where applicable in Go code
Files:
pkg/connector/handlers/handler.gopkg/line/structs.gopkg/line/qr.gopkg/e2ee/manager.gopkg/line/secret/secret.gopkg/line/client.gopkg/connector/e2ee_keys.gopkg/runner.gopkg/connector/client.gopkg/line/methods.gopkg/connector/connector.go
**/!(ltsm)/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/!(ltsm)/**/*.go: Runstaticcheckon all Go files excludingpkg/ltsmpackage (transpiled WASM code)
Rungo veton all Go files excludingpkg/ltsmpackage (transpiled WASM code)
Files:
pkg/connector/handlers/handler.gopkg/line/structs.gopkg/line/qr.gopkg/e2ee/manager.gopkg/line/secret/secret.gopkg/line/client.gopkg/connector/e2ee_keys.gopkg/runner.gopkg/connector/client.gopkg/line/methods.gopkg/connector/connector.go
pkg/connector/connector.go
📄 CodeRabbit inference engine (AGENTS.md)
Implement
bridgev2.NetworkConnectorandbridgev2.NetworkAPIinterfaces in the connector package for bridge logic
Files:
pkg/connector/connector.go
🧠 Learnings (2)
📚 Learning: 2026-05-29T10:13:37.093Z
Learnt from: CR
Repo: beeper/matrix-line-messenger PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-29T10:13:37.093Z
Learning: Implement proactive token refresh in LineClient based on server-provided duration with automatic recovery on expiry
Applied to files:
pkg/line/client.gopkg/connector/client.go
📚 Learning: 2026-05-29T10:13:37.093Z
Learnt from: CR
Repo: beeper/matrix-line-messenger PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-29T10:13:37.093Z
Learning: Applies to pkg/connector/connector.go : Implement `bridgev2.NetworkConnector` and `bridgev2.NetworkAPI` interfaces in the connector package for bridge logic
Applied to files:
pkg/connector/connector.go
🔇 Additional comments (2)
README.md (1)
286-294: LGTM!Also applies to: 316-319
pkg/connector/handlers/handler.go (1)
36-40: Verify the OBS error message format (and scope the cache-clear fragility).
DownloadOBSWithSIDOptionsformats non-200 responses asfmt.Errorf("OBS download failed (%d): %s", resp.StatusCode, ...), so for HTTP 401 the error string includes the substringOBS download failed (401)and the cache-clear path will trigger for this download code path. Even if that exact substring ever changed,tryRecoverClientstill attemptsRecoverTokenwhenevererr.Error()contains"401"—so onlyline.ClearEncryptedAccessTokenCache()would be at risk.
Refactor to avoid brittle string matching by returning a typed error that carries the HTTP status code (so callers canerrors.Ason it).> Likely an incorrect or invalid review comment.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
in light of coderabbitai's comments i'll move this pr to draft. need to have another look at it eventually |
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Replacement for #124 after GitHub refused reopening the closed PR. This branch is pushed from a Beeper-network fork and includes the conflict-resolution merge from current main.
Fixes #94
Summary
This PR adds LINE QR code login as a new login flow while keeping the existing email/password flow intact.
Although the user-facing change is simple (scan QR), implementation touches multiple layers because LINE’s QR login uses a different protocol path with long-poll state transitions and E2EE login key bootstrap.
Why this is larger than it looks
QR login is not a variant of email login. It requires:
SecondaryQrCodeLoginService+ permit-notice long polling)X-Line-Session-ID,X-LST, context-aware long polling)What changed (by area)
1) Connector login flow wiring
QR Code(recommended/default)Email and Password(existing fallback)LineQRLoginprocess implementing display-and-wait behaviorFiles:
pkg/connector/connector.go2) LINE QR protocol client
CreateQRSessionCreateQRCodeCheckQRCodeVerified(Context)VerifyCertificateCreatePinCodeCheckPinCodeVerified(Context)QRCodeLoginV2Files:
pkg/line/methods.gopkg/line/structs.go3) Transport updates for QR long-polling
x-line-applicationX-Line-Session-IDX-LSTFile:
pkg/line/client.go4) E2EE QR bootstrap support
Files:
pkg/line/qr.go(new)pkg/line/secret/secret.gopkg/runner.go5) Docs
File:
README.mdLogin flow behavior
qrCodeLoginV2and stores login metadataFollow-up commits in this PR
33eab2c: QR login token recovery fixes54a2f91: QR login key-handling hardening2f0207c: stale OBS token recovery fix (small isolated bugfix)Reviewer guide
Recommended review order:
pkg/line/methods.go+pkg/line/client.go(protocol + transport)pkg/connector/connector.go(bridge login state machine)pkg/line/qr.go,pkg/line/secret/secret.go,pkg/runner.go)