[client] Display QR code for device auth login URL#5415
[client] Display QR code for device auth login URL#5415typhoon1217 wants to merge 4 commits intonetbirdio:mainfrom
Conversation
When running `netbird up` or `netbird login` on a headless machine, users must manually type the SSO URL into a browser. This adds a scannable QR code alongside the URL for convenience. The QR code is only displayed when stdout is a terminal, so piped or redirected output remains clean. Uses half-block rendering for a compact display.
|
|
📝 WalkthroughWalkthroughAdds terminal QR code rendering to the SSO login flow: new unexported Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
🧹 Nitpick comments (2)
go.mod (1)
229-229:qrterminal/v3is a direct dependency, not indirect.
github.com/mdp/qrterminal/v3is directly imported byclient/cmd/qr.goandclient/cmd/qr_test.go, so the// indirectannotation and placement in the indirect-onlyrequireblock are wrong. Runninggo mod tidywill move it to the direct-dependency block and drop the annotation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 229, The go.mod lists github.com/mdp/qrterminal/v3 as indirect but it is directly imported by client/cmd/qr.go and client/cmd/qr_test.go; fix by removing the "// indirect" annotation and ensuring the module appears in the direct require block (best done by running `go mod tidy` or manually moving the require entry for github.com/mdp/qrterminal/v3 to the main require section), so the dependency is correctly recorded as direct.client/cmd/qr_test.go (1)
31-40:TestPrintQRCode_NonTerminalFilehas no assertion.The comment at line 38 says "no QR should be generated," but the test only verifies absence of a panic — it cannot confirm suppression since there's no output buffer. This is acceptable as a crash-guard, but a brief inline comment explaining the limitation (and that the non-terminal path is already validated by
TestPrintQRCode_NonTerminalWriter) would make the intent clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/cmd/qr_test.go` around lines 31 - 40, TestPrintQRCode_NonTerminalFile lacks an assertion to confirm suppression; update the test by adding a brief inline comment inside TestPrintQRCode_NonTerminalFile explaining that /dev/null is used only to ensure no panic (we cannot capture output from a non-terminal *os.File here) and that the non-terminal behavior is validated by the existing TestPrintQRCode_NonTerminalWriter test; reference the function names printQRCode, TestPrintQRCode_NonTerminalFile, and TestPrintQRCode_NonTerminalWriter in the comment so future readers understand why no assertion is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/cmd/qr.go`:
- Around line 24-33: The QuietZone value in the qrterminal.GenerateWithConfig
call is set to 1 which violates the QR spec; update the qrterminal.Config passed
to qrterminal.GenerateWithConfig so QuietZone uses the library default constant
(qrterminal.QUIET_ZONE) or at least 4 instead of 1; change the QuietZone field
in the qrterminal.Config literal used in this call to reference
qrterminal.QUIET_ZONE (or the numeric value 4) to ensure the required 4-module
margin.
---
Nitpick comments:
In `@client/cmd/qr_test.go`:
- Around line 31-40: TestPrintQRCode_NonTerminalFile lacks an assertion to
confirm suppression; update the test by adding a brief inline comment inside
TestPrintQRCode_NonTerminalFile explaining that /dev/null is used only to ensure
no panic (we cannot capture output from a non-terminal *os.File here) and that
the non-terminal behavior is validated by the existing
TestPrintQRCode_NonTerminalWriter test; reference the function names
printQRCode, TestPrintQRCode_NonTerminalFile, and
TestPrintQRCode_NonTerminalWriter in the comment so future readers understand
why no assertion is present.
In `@go.mod`:
- Line 229: The go.mod lists github.com/mdp/qrterminal/v3 as indirect but it is
directly imported by client/cmd/qr.go and client/cmd/qr_test.go; fix by removing
the "// indirect" annotation and ensuring the module appears in the direct
require block (best done by running `go mod tidy` or manually moving the require
entry for github.com/mdp/qrterminal/v3 to the main require section), so the
dependency is correctly recorded as direct.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
client/cmd/login.goclient/cmd/qr.goclient/cmd/qr_test.gogo.mod
There was a problem hiding this comment.
♻️ Duplicate comments (2)
client/cmd/qr.go (2)
32-32:QuietZone: qrterminal.QUIET_ZONE— past issue resolved.The previous
QuietZone: 1violation of ISO/IEC 18004 has been corrected to use the library-provided default.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/cmd/qr.go` at line 32, QuietZone was previously hard-coded to 1 (violating ISO/IEC 18004); update the QR config to use the library default by setting QuietZone to qrterminal.QUIET_ZONE in the QR generation configuration (locate the struct or options creation where QuietZone is set in client/cmd/qr.go, replace any literal 1 with qrterminal.QUIET_ZONE).
32-32:QuietZone: qrterminal.QUIET_ZONE— past issue resolved.The prior
QuietZone: 1has been corrected to use the library-provided default constant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/cmd/qr.go` at line 32, Summary: Replace hardcoded QuietZone value with library constant and ensure consistency. Locate uses of the QuietZone field (e.g., the struct literal that currently sets QuietZone) and replace any numeric literal (like 1) with qrterminal.QUIET_ZONE, ensure the qrterminal package is imported where this struct is constructed, and audit the repo for any other occurrences of a hardcoded QuietZone and update them to use qrterminal.QUIET_ZONE for consistency.
🧹 Nitpick comments (2)
client/cmd/qr.go (2)
25-25: Consider upgrading error correction toLevel Mfor better real-world scan reliability.
qrterminal.L(7% error correction) is the lowest tier. OAuthverificationURICompleteURLs are typically scanned from a screen — terminal font rendering, sub-pixel anti-aliasing, and screen glare can introduce pixel-level noise that Level L cannot recover from.qrterminal.M(15%) is the standard choice for URL QR codes and adds negligible size overhead.♻️ Proposed change
- Level: qrterminal.L, + Level: qrterminal.M,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/cmd/qr.go` at line 25, The QR generation currently sets the error correction Level to qrterminal.L; update it to qrterminal.M to increase error correction from 7% to 15% for more reliable real-world scanning. Locate the QR options where Level: qrterminal.L is set (in the QR generation function/variable that constructs the qrterminal config) and change it to Level: qrterminal.M; no other changes required.
25-25: ConsiderLevel MoverLevel Lfor better real-world scan reliability.
qrterminal.Lprovides only 7% error correction. OAuthverificationURICompleteURLs are typically scanned from a screen where terminal font rendering, sub-pixel anti-aliasing, and glare introduce pixel-level noise that Level L cannot recover from. The library's own README examples useLevel: qrterminal.Mas the standard choice. Level M (15%) adds negligible size overhead for this use case.♻️ Proposed change
- Level: qrterminal.L, + Level: qrterminal.M,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/cmd/qr.go` at line 25, Update the QR terminal configuration to use higher error correction: change the QR config that sets "Level: qrterminal.L" to "Level: qrterminal.M" so generated OTP/verificationURIComplete QR codes use 15% EC; locate the QR generation code where the qrterminal config is constructed (the struct or map containing the Level field) and replace qrterminal.L with qrterminal.M to improve scan reliability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@client/cmd/qr.go`:
- Line 32: QuietZone was previously hard-coded to 1 (violating ISO/IEC 18004);
update the QR config to use the library default by setting QuietZone to
qrterminal.QUIET_ZONE in the QR generation configuration (locate the struct or
options creation where QuietZone is set in client/cmd/qr.go, replace any literal
1 with qrterminal.QUIET_ZONE).
- Line 32: Summary: Replace hardcoded QuietZone value with library constant and
ensure consistency. Locate uses of the QuietZone field (e.g., the struct literal
that currently sets QuietZone) and replace any numeric literal (like 1) with
qrterminal.QUIET_ZONE, ensure the qrterminal package is imported where this
struct is constructed, and audit the repo for any other occurrences of a
hardcoded QuietZone and update them to use qrterminal.QUIET_ZONE for
consistency.
---
Nitpick comments:
In `@client/cmd/qr.go`:
- Line 25: The QR generation currently sets the error correction Level to
qrterminal.L; update it to qrterminal.M to increase error correction from 7% to
15% for more reliable real-world scanning. Locate the QR options where Level:
qrterminal.L is set (in the QR generation function/variable that constructs the
qrterminal config) and change it to Level: qrterminal.M; no other changes
required.
- Line 25: Update the QR terminal configuration to use higher error correction:
change the QR config that sets "Level: qrterminal.L" to "Level: qrterminal.M" so
generated OTP/verificationURIComplete QR codes use 15% EC; locate the QR
generation code where the qrterminal config is constructed (the struct or map
containing the Level field) and replace qrterminal.L with qrterminal.M to
improve scan reliability.
typhoon1217
force-pushed
the
feat/client-qr-device-auth
branch
from
fdb58d1 to
0193d7c
Compare
- Upgrade error correction Level L to M for better scan reliability - Replace hardcoded QuietZone with qrterminal.QUIET_ZONE constant - Clarify test comment for non-terminal file guard
Remove automatic TTY-based QR display. Users on headless machines without browser access can now explicitly request a QR code via --qr on both netbird login and netbird up commands.
typhoon1217
force-pushed
the
feat/client-qr-device-auth
branch
from
0193d7c to
cc70276
Compare
|
|
@mlsmaycon @heisbrot — would appreciate your review when you have a moment! This adds an optional |
2 participants
When running
netbird upornetbird loginon a headless machine without browser access, users must manually type the SSO device auth URL into a browser. This adds a--qrflag to both commands that prints a scannable QR code alongside the URL, allowing users to simply scan with their phone instead.Describe your changes
printQRCode(writer io.Writer, url string)helper inclient/cmd/qr.gothat renders a QR code to the terminal usingqrterminal.GenerateWithConfig--qrpersistent flag to theupandlogincommands; QR display is opt-in and off by defaultopenURLto accept ashowQR boolparameter and callprintQRCodewhen the flag is setIssue ticket number and link
N/A — this is a UX improvement for a pain point on headless/server setups where users have no browser access during SSO device auth flow.
Stack
Checklist
Documentation
Select exactly one:
CLI-only UX improvement, no config, API, or protocol changes. The
--qrflag is self-explanatory and surfaces vianetbird up --help/netbird login --help.Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__