feat: add bb login command for console host registration #437

Merged
didericis merged 6 commits from console-login into main 2026-07-21 14:25:40 -04:00
2 changed files with 72 additions and 8 deletions
Showing only changes of commit 9b6788ebad - Show all commits
+17 -3
View File
1
@@ -18,6 +18,7 @@ import json
import os
import socket
import sys
import tempfile
import time
import urllib.error
import urllib.request
@@ -71,7 +72,7 @@ def _save_credentials(
) -> Path:
Outdated
Review

Still blocking: The credentials are still written with path.write_text() before chmod(0600). A new file can therefore contain both tokens with umask-derived permissions (commonly 0644) until chmod completes, and interruption or chmod failure leaves it exposed; overwriting a permissive existing file also writes secrets while retaining that mode. Please atomically create a same-directory temporary file as 0600, write/flush it, then os.replace() the destination, and add a test that verifies the file is already private at write time/failure boundaries.

**Still blocking:** The credentials are still written with path.write_text() before chmod(0600). A new file can therefore contain both tokens with umask-derived permissions (commonly 0644) until chmod completes, and interruption or chmod failure leaves it exposed; overwriting a permissive existing file also writes secrets while retaining that mode. Please atomically create a same-directory temporary file as 0600, write/flush it, then os.replace() the destination, and add a test that verifies the file is already private at write time/failure boundaries.
Outdated
Review

Fixed in ec3791c. _save_credentials now creates a temp file via tempfile.mkstemp(dir=path.parent, prefix=".console-"), sets it explicitly to 0600, writes to it, then calls os.replace() to atomically move it to the final path. The token file never exists at console.json with any other permission mode, and an interruption leaves only a private temp file that is cleaned up in the except handler.

New test test_temp_file_is_private_before_replace spies on os.replace to verify the temp file is already 0600 at the moment of rename.

Fixed in ec3791c. `_save_credentials` now creates a temp file via `tempfile.mkstemp(dir=path.parent, prefix=".console-")`, sets it explicitly to 0600, writes to it, then calls `os.replace()` to atomically move it to the final path. The token file never exists at `console.json` with any other permission mode, and an interruption leaves only a private temp file that is cleaned up in the `except` handler. New test `test_temp_file_is_private_before_replace` spies on `os.replace` to verify the temp file is already 0600 at the moment of rename.
path = bot_bottle_root() / "console.json"
path.parent.mkdir(parents=True, exist_ok=True)
path.write_text(
content = (
json.dumps(
{
"url": console_url,
@@ -83,7 +84,19 @@ def _save_credentials(
)
+ "\n"
)
path.chmod(0o600)
fd, tmp_path_str = tempfile.mkstemp(dir=path.parent, prefix=".console-")
tmp = Path(tmp_path_str)
try:
tmp.chmod(0o600)
with os.fdopen(fd, "w") as f:
f.write(content)
os.replace(tmp, path)
except Exception:
try:
tmp.unlink()
except OSError:
pass
raise
return path
@@ -111,6 +124,7 @@ def cmd_login(argv: list[str]) -> int:
device_code = resp["device_code"]
user_code = resp["user_code"]
Outdated
Review

Still blocking: poll_interval remains discarded and every flow sleeps the hard-coded _POLL_SLEEP. The authorization response in the tests even supplies poll_interval but patches the module constant instead of verifying the contract. Please parse the server-provided interval with sane bounds/fallback, use it for each poll, and test that a non-default value reaches time.sleep().

**Still blocking:** poll_interval remains discarded and every flow sleeps the hard-coded _POLL_SLEEP. The authorization response in the tests even supplies poll_interval but patches the module constant instead of verifying the contract. Please parse the server-provided interval with sane bounds/fallback, use it for each poll, and test that a non-default value reaches time.sleep().
Outdated
Review

Fixed in ec3791c. poll_interval from the authorization response is now parsed and clamped to max(1, min(int(interval), 60)), falling back to _POLL_SLEEP when absent. The clamped value is stored as poll_sleep and passed to time.sleep() on every poll cycle.

New test test_poll_interval_from_server_is_used patches time.sleep and asserts every call receives the server-supplied value (7 s). Existing flow tests now patch time.sleep directly instead of the module constant.

Fixed in ec3791c. `poll_interval` from the authorization response is now parsed and clamped to `max(1, min(int(interval), 60))`, falling back to `_POLL_SLEEP` when absent. The clamped value is stored as `poll_sleep` and passed to `time.sleep()` on every poll cycle. New test `test_poll_interval_from_server_is_used` patches `time.sleep` and asserts every call receives the server-supplied value (7 s). Existing flow tests now patch `time.sleep` directly instead of the module constant.
expires_in = resp.get("expires_in", 300)
poll_sleep = max(1, min(int(resp.get("poll_interval", _POLL_SLEEP)), 60))
sys.stderr.write(
f"\nOpen this URL in your browser to authorize this host:\n\n"
@@ -122,7 +136,7 @@ def cmd_login(argv: list[str]) -> int:
while time.monotonic() < deadline:
sys.stderr.write(".")
sys.stderr.flush()
time.sleep(_POLL_SLEEP)
time.sleep(poll_sleep)
try:
code, result = _get(
+55 -5
View File
@@ -42,6 +42,26 @@ class TestSaveCredentials(unittest.TestCase):
self.assertEqual(data["refresh_token"], "rt")
self.assertEqual(oct(path.stat().st_mode & 0o777), oct(0o600))
def test_temp_file_is_private_before_replace(self) -> None:
"""Temp file must be 0600 at the moment os.replace is called."""
from bot_bottle.cli.login import _save_credentials
from pathlib import Path as _Path
tmp_perms_at_replace: list[int] = []
real_replace = os.replace
def _spy_replace(src: "str | os.PathLike", dst: "str | os.PathLike") -> None:
tmp_perms_at_replace.append(_Path(src).stat().st_mode & 0o777)
real_replace(src, dst)
with tempfile.TemporaryDirectory() as tmp:
with patch.dict(os.environ, {"BOT_BOTTLE_ROOT": tmp}):
with patch("os.replace", side_effect=_spy_replace):
path = _save_credentials("http://c", "hid", "at", "rt")
self.assertEqual(len(tmp_perms_at_replace), 1)
self.assertEqual(oct(tmp_perms_at_replace[0]), oct(0o600))
self.assertEqual(oct(path.stat().st_mode & 0o777), oct(0o600))
class TestCmdLoginMissingUrl(unittest.TestCase):
def test_returns_1_without_url(self) -> None:
@@ -91,7 +111,7 @@ class TestCmdLoginFlow(unittest.TestCase):
with patch.dict(os.environ, {"BOT_BOTTLE_ROOT": tmp}):
with patch("bot_bottle.cli.login._post", side_effect=_fake_post):
with patch("bot_bottle.cli.login._get", side_effect=_fake_get):
with patch("bot_bottle.cli.login._POLL_SLEEP", 0):
with patch("time.sleep"):
return cmd_login(["--console-url", "http://console"])
def test_approved_flow_returns_0(self) -> None:
@@ -121,17 +141,47 @@ class TestCmdLoginFlow(unittest.TestCase):
start_resp = {
"device_code": "dc",
"user_code": "ZZZ-ZZZ",
"expires_in": 0, # already expired
"poll_interval": 0,
"expires_in": 0, # already expired; loop never runs
"poll_interval": 2,
}
with tempfile.TemporaryDirectory() as tmp:
with patch.dict(os.environ, {"BOT_BOTTLE_ROOT": tmp}):
with patch("bot_bottle.cli.login._post", return_value=start_resp):
with patch("bot_bottle.cli.login._POLL_SLEEP", 0):
result = cmd_login(["--console-url", "http://console"])
result = cmd_login(["--console-url", "http://console"])
self.assertEqual(result, 1)
def test_poll_interval_from_server_is_used(self) -> None:
"""time.sleep must be called with the server-provided poll_interval."""
from bot_bottle.cli.login import cmd_login
server_interval = 7
start_resp = {
"device_code": "dc",
"user_code": "ABC-DEF",
"expires_in": 300,
"poll_interval": server_interval,
}
approved = {
"status": "approved",
"host_id": "hid",
"access_token": "at",
"refresh_token": "rt",
}
poll_iter = iter([{"status": "pending"}, approved])
with tempfile.TemporaryDirectory() as tmp:
with patch.dict(os.environ, {"BOT_BOTTLE_ROOT": tmp}):
with patch("bot_bottle.cli.login._post", return_value=start_resp):
with patch("bot_bottle.cli.login._get", side_effect=lambda u: (200, next(poll_iter))):
with patch("time.sleep") as mock_sleep:
result = cmd_login(["--console-url", "http://console"])
self.assertEqual(result, 0)
self.assertTrue(mock_sleep.called)
for call in mock_sleep.call_args_list:
self.assertEqual(call.args[0], server_interval)
class TestDispatcherRegistration(unittest.TestCase):
def test_login_in_commands(self) -> None: