mirror of
https://github.com/HKUDS/nanobot.git
synced 2026-05-19 16:12:30 +00:00
simplify(pairing): address review findings — constants, TOCTOU, nesting
- Remove TOCTOU exists() check in _load(); rely on FileNotFoundError - Define PAIRING_CODE_META_KEY and PAIRING_COMMAND_META_KEY constants in nanobot.pairing, replacing magic strings across base.py, slack.py, and builtin.py - Flatten nested revoke logic in handle_pairing_command() - Trim redundant docstring/comment noise in is_allowed() and generate_code()
This commit is contained in:
parent
b0d79af968
commit
3527b5377b
@ -11,6 +11,7 @@ from loguru import logger
|
|||||||
from nanobot.bus.events import InboundMessage, OutboundMessage
|
from nanobot.bus.events import InboundMessage, OutboundMessage
|
||||||
from nanobot.bus.queue import MessageBus
|
from nanobot.bus.queue import MessageBus
|
||||||
from nanobot.pairing import (
|
from nanobot.pairing import (
|
||||||
|
PAIRING_CODE_META_KEY,
|
||||||
format_pairing_reply,
|
format_pairing_reply,
|
||||||
generate_code,
|
generate_code,
|
||||||
is_approved,
|
is_approved,
|
||||||
@ -181,18 +182,7 @@ class BaseChannel(ABC):
|
|||||||
return bool(streaming) and type(self).send_delta is not BaseChannel.send_delta
|
return bool(streaming) and type(self).send_delta is not BaseChannel.send_delta
|
||||||
|
|
||||||
def is_allowed(self, sender_id: str) -> bool:
|
def is_allowed(self, sender_id: str) -> bool:
|
||||||
"""Check if *sender_id* is permitted.
|
"""Check sender permission: star > allowlist > pairing store > deny."""
|
||||||
|
|
||||||
Priority:
|
|
||||||
1. ``allowFrom: ["*"]`` → allow all.
|
|
||||||
2. ``allowFrom`` list → allow if sender_id is present.
|
|
||||||
3. Pairing store approved list → allow if previously approved.
|
|
||||||
4. Otherwise deny.
|
|
||||||
|
|
||||||
An empty ``allowFrom`` list does not cause a hard exit; instead it
|
|
||||||
defers to the pairing store so that unknown DM senders can request
|
|
||||||
access via a pairing code.
|
|
||||||
"""
|
|
||||||
if isinstance(self.config, dict):
|
if isinstance(self.config, dict):
|
||||||
if "allow_from" in self.config:
|
if "allow_from" in self.config:
|
||||||
allow_list = self.config.get("allow_from") or []
|
allow_list = self.config.get("allow_from") or []
|
||||||
@ -228,7 +218,7 @@ class BaseChannel(ABC):
|
|||||||
channel=self.name,
|
channel=self.name,
|
||||||
chat_id=str(chat_id),
|
chat_id=str(chat_id),
|
||||||
content=format_pairing_reply(code),
|
content=format_pairing_reply(code),
|
||||||
metadata={"_pairing_code": code},
|
metadata={PAIRING_CODE_META_KEY: code},
|
||||||
)
|
)
|
||||||
)
|
)
|
||||||
self.logger.info(
|
self.logger.info(
|
||||||
|
|||||||
@ -18,7 +18,7 @@ from nanobot.bus.queue import MessageBus
|
|||||||
from nanobot.channels.base import BaseChannel
|
from nanobot.channels.base import BaseChannel
|
||||||
from nanobot.config.paths import get_media_dir
|
from nanobot.config.paths import get_media_dir
|
||||||
from nanobot.config.schema import Base
|
from nanobot.config.schema import Base
|
||||||
from nanobot.pairing import format_pairing_reply, generate_code, is_approved
|
from nanobot.pairing import PAIRING_CODE_META_KEY, format_pairing_reply, generate_code, is_approved
|
||||||
from nanobot.utils.helpers import safe_filename, split_message
|
from nanobot.utils.helpers import safe_filename, split_message
|
||||||
|
|
||||||
|
|
||||||
@ -351,7 +351,7 @@ class SlackChannel(BaseChannel):
|
|||||||
channel=self.name,
|
channel=self.name,
|
||||||
chat_id=chat_id,
|
chat_id=chat_id,
|
||||||
content=reply,
|
content=reply,
|
||||||
metadata={"_pairing_code": code},
|
metadata={PAIRING_CODE_META_KEY: code},
|
||||||
)
|
)
|
||||||
)
|
)
|
||||||
return
|
return
|
||||||
|
|||||||
@ -548,14 +548,14 @@ async def cmd_history(ctx: CommandContext) -> OutboundMessage:
|
|||||||
|
|
||||||
async def cmd_pairing(ctx: CommandContext) -> OutboundMessage:
|
async def cmd_pairing(ctx: CommandContext) -> OutboundMessage:
|
||||||
"""List, approve, deny or revoke pairing requests."""
|
"""List, approve, deny or revoke pairing requests."""
|
||||||
from nanobot.pairing import handle_pairing_command
|
from nanobot.pairing import PAIRING_COMMAND_META_KEY, handle_pairing_command
|
||||||
|
|
||||||
reply = handle_pairing_command(ctx.msg.channel, ctx.args)
|
reply = handle_pairing_command(ctx.msg.channel, ctx.args)
|
||||||
return OutboundMessage(
|
return OutboundMessage(
|
||||||
channel=ctx.msg.channel,
|
channel=ctx.msg.channel,
|
||||||
chat_id=ctx.msg.chat_id,
|
chat_id=ctx.msg.chat_id,
|
||||||
content=reply,
|
content=reply,
|
||||||
metadata={"_pairing_command": True},
|
metadata={PAIRING_COMMAND_META_KEY: True},
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@ -13,6 +13,10 @@ from nanobot.pairing.store import (
|
|||||||
revoke,
|
revoke,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
# Metadata keys used by channels and commands to tag pairing-related messages.
|
||||||
|
PAIRING_CODE_META_KEY = "_pairing_code"
|
||||||
|
PAIRING_COMMAND_META_KEY = "_pairing_command"
|
||||||
|
|
||||||
__all__ = [
|
__all__ = [
|
||||||
"approve_code",
|
"approve_code",
|
||||||
"deny_code",
|
"deny_code",
|
||||||
@ -24,4 +28,6 @@ __all__ = [
|
|||||||
"is_approved",
|
"is_approved",
|
||||||
"list_pending",
|
"list_pending",
|
||||||
"revoke",
|
"revoke",
|
||||||
|
"PAIRING_CODE_META_KEY",
|
||||||
|
"PAIRING_COMMAND_META_KEY",
|
||||||
]
|
]
|
||||||
|
|||||||
@ -35,11 +35,11 @@ def _store_path() -> Path:
|
|||||||
|
|
||||||
def _load() -> dict[str, Any]:
|
def _load() -> dict[str, Any]:
|
||||||
path = _store_path()
|
path = _store_path()
|
||||||
if not path.exists():
|
|
||||||
return {"approved": {}, "pending": {}}
|
|
||||||
try:
|
try:
|
||||||
with open(path, encoding="utf-8") as f:
|
with open(path, encoding="utf-8") as f:
|
||||||
data = json.load(f)
|
data = json.load(f)
|
||||||
|
except FileNotFoundError:
|
||||||
|
return {"approved": {}, "pending": {}}
|
||||||
except (json.JSONDecodeError, OSError):
|
except (json.JSONDecodeError, OSError):
|
||||||
logger.warning("Corrupted pairing store, resetting")
|
logger.warning("Corrupted pairing store, resetting")
|
||||||
return {"approved": {}, "pending": {}}
|
return {"approved": {}, "pending": {}}
|
||||||
@ -82,8 +82,6 @@ def generate_code(
|
|||||||
with _LOCK:
|
with _LOCK:
|
||||||
data = _load()
|
data = _load()
|
||||||
_gc_pending(data)
|
_gc_pending(data)
|
||||||
# Collision probability is negligible (~1e-12 with 20 pending codes),
|
|
||||||
# so we skip an existence check for simplicity.
|
|
||||||
raw = "".join(secrets.choice(_ALPHABET) for _ in range(_CODE_LENGTH))
|
raw = "".join(secrets.choice(_ALPHABET) for _ in range(_CODE_LENGTH))
|
||||||
code = f"{raw[:4]}-{raw[4:]}"
|
code = f"{raw[:4]}-{raw[4:]}"
|
||||||
|
|
||||||
@ -236,22 +234,19 @@ def handle_pairing_command(channel: str, subcommand_text: str) -> str:
|
|||||||
return f"Pairing code `{arg}` not found or already expired"
|
return f"Pairing code `{arg}` not found or already expired"
|
||||||
|
|
||||||
elif sub == "revoke":
|
elif sub == "revoke":
|
||||||
if arg is None:
|
if len(parts) == 2:
|
||||||
return "Usage: `/pairing revoke <user_id>` or `/pairing revoke <channel> <user_id>`"
|
|
||||||
elif len(parts) == 2:
|
|
||||||
return (
|
return (
|
||||||
f"Revoked {arg} from {channel}"
|
f"Revoked {arg} from {channel}"
|
||||||
if revoke(channel, arg)
|
if revoke(channel, arg)
|
||||||
else f"{arg} was not in the approved list for {channel}"
|
else f"{arg} was not in the approved list for {channel}"
|
||||||
)
|
)
|
||||||
elif len(parts) == 3:
|
if len(parts) == 3:
|
||||||
return (
|
return (
|
||||||
f"Revoked {parts[2]} from {arg}"
|
f"Revoked {parts[2]} from {arg}"
|
||||||
if revoke(arg, parts[2])
|
if revoke(arg, parts[2])
|
||||||
else f"{parts[2]} was not in the approved list for {arg}"
|
else f"{parts[2]} was not in the approved list for {arg}"
|
||||||
)
|
)
|
||||||
else:
|
return "Usage: `/pairing revoke <user_id>` or `/pairing revoke <channel> <user_id>`"
|
||||||
return "Usage: `/pairing revoke <user_id>` or `/pairing revoke <channel> <user_id>`"
|
|
||||||
|
|
||||||
return (
|
return (
|
||||||
"Unknown pairing command.\n"
|
"Unknown pairing command.\n"
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user