mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-11 03:13:24 +00:00
fix(pairing): address review — docs, error mapping, approve feedback, unreadable expiry
- docs: device.pair.update and the `permanent` option on approve in docs/04-gateway-protocol.md, docs/19-websocket-rpc.md and websocket-protocol.md; the paired-device TTL row in docs/09-security.md now mentions the admin opt-out. - store.ErrPairedDeviceNotFound: SetPairingPermanent wraps it in both stores. device.pair.update maps it to NOT_FOUND and any other store error to INTERNAL, so a DB failure no longer reads as "not found". - web UI: approve and make-permanent/set-expiry now toast the server error and reload the list in `finally`. A partially applied approve (paired, but the permanent write failed) shows up in the table instead of leaving the dialog dead-ended. - SQLite ListPaired: a stored expiry that fails to parse stays 0 (expires, date unknown) rather than being mistaken for permanent; the UI renders it as "--" instead of a 1970 date. Tests: gateway handler error mapping (NOT_FOUND / INTERNAL / OK), sentinel checks in the PG and SQLite store tests, SQLite unreadable-expiry case. Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
1 parent
5379cc1163
commit
ea4890c257
18 files changed
+161
-23
No files matched your search
@@ -112,7 +112,7 @@ flowchart LR
|
||||
|------|--------------------|
|
||||
| viewer | `agents.list`, `config.get`, `sessions.list`, `sessions.preview`, `health`, `status`, `providers.models`, `skills.list`, `skills.get`, `channels.list`, `channels.status`, `cron.list`, `cron.status`, `cron.runs`, `usage.get`, `usage.summary` |
|
||||
| operator | All viewer methods plus: `chat.send`, `chat.abort`, `chat.history`, `chat.inject`, `sessions.delete`, `sessions.reset`, `sessions.patch`, `cron.create`, `cron.update`, `cron.delete`, `cron.toggle`, `cron.run`, `skills.update`, `send`, `exec.approval.list`, `exec.approval.approve`, `exec.approval.deny`, `device.pair.request`, `device.pair.list` |
|
||||
| admin | All operator methods plus: `config.apply`, `config.patch`, `config.permissions.*`, `agents.create`, `agents.update`, `agents.delete`, `agents.files.*`, `teams.*`, `channels.toggle`, `device.pair.approve`, `device.pair.revoke` |
|
||||
| admin | All operator methods plus: `config.apply`, `config.patch`, `config.permissions.*`, `agents.create`, `agents.update`, `agents.delete`, `agents.files.*`, `teams.*`, `channels.toggle`, `device.pair.approve`, `device.pair.revoke`, `device.pair.update` |
|
||||
|
||||
---
|
||||
|
||||
@@ -233,9 +233,10 @@ flowchart TD
|
||||
| Method | Description |
|
||||
|--------|-------------|
|
||||
| `device.pair.request` | Request a pairing code |
|
||||
| `device.pair.approve` | Approve a pairing request |
|
||||
| `device.pair.approve` | Approve a pairing request (optional `permanent`) |
|
||||
| `device.pair.list` | List paired devices |
|
||||
| `device.pair.revoke` | Revoke a paired device |
|
||||
| `device.pair.update` | Make a paired device permanent or restore the default TTL |
|
||||
| `browser.pairing.status` | Poll browser pairing approval status |
|
||||
|
||||
### Exec Approval
|
||||
|
||||
+1
-1
@@ -451,7 +451,7 @@ Browser pairing allows web UI clients to authenticate without full admin credent
|
||||
|-----------|--------|
|
||||
| Pairing code | 8-character alphanumeric code (A-Z, 2-9, excludes I/O/L for clarity), generated via `generatePairingCode()` in `internal/store/pg/pairing.go` |
|
||||
| Code TTL | 60 minutes; expired codes are auto-pruned from database |
|
||||
| Paired device TTL | 30 days; provides defense-in-depth expiry (paired devices auto-cleaned if unused) |
|
||||
| Paired device TTL | 30 days by default; provides defense-in-depth expiry (expired pairings are auto-cleaned). An admin can opt a single pairing out of expiry (`device.pair.approve` with `permanent`, or `device.pair.update`); such a pairing lasts until revoked |
|
||||
| Pending limit | Max 3 pending pairing requests per account; prevents spam/enumeration |
|
||||
| HTTP access | Paired browsers access HTTP APIs via `X-GoClaw-Sender-Id` header (requires `channel=browser`). Fail-closed: `IsPaired()` check blocks unpaired sessions. Logs failed HTTP pairing auth attempts for security monitoring. |
|
||||
| Approval flow | Requires WebSocket `device.pair.approve` method from authenticated admin session, triggered by `pairing.approve` command. Admin approval adds sender to `paired_devices` table with `paired_by` audit field. |
|
||||
|
||||
@@ -378,10 +378,11 @@ Get JSON schema for config form generation.
|
||||
| Method | Description | Auth |
|
||||
|--------|-------------|------|
|
||||
| `device.pair.request` | Request pairing (from device) | Unauthenticated |
|
||||
| `device.pair.approve` | Approve request (from admin) | Admin |
|
||||
| `device.pair.approve` | Approve request (from admin); optional `permanent: true` skips the 30-day TTL | Admin |
|
||||
| `device.pair.deny` | Deny request | Admin |
|
||||
| `device.pair.list` | List pending + paired devices | Admin |
|
||||
| `device.pair.revoke` | Revoke device | Admin |
|
||||
| `device.pair.update` | `{senderId, channel, permanent}` — `true` clears expiry, `false` restarts the 30-day TTL from now; an expired pairing is not revived | Admin |
|
||||
| `browser.pairing.status` | Poll pairing status | Unauthenticated |
|
||||
|
||||
### Pairing Flow
|
||||
@@ -391,7 +392,7 @@ sequenceDiagram
|
||||
Device->>Gateway: device.pair.request {senderId, channel}
|
||||
Gateway-->>Device: {code: "A1B2C3D4"}
|
||||
Device->>Gateway: browser.pairing.status {sender_id} (poll)
|
||||
Admin->>Gateway: device.pair.approve {code, approvedBy}
|
||||
Admin->>Gateway: device.pair.approve {code, approvedBy, permanent?}
|
||||
Gateway-->>Device: {status: "approved"}
|
||||
```
|
||||
|
||||
@@ -816,7 +817,7 @@ Methods are gated by role. The role is determined at `connect` time from the tok
|
||||
|
||||
### Admin-Only Methods
|
||||
|
||||
`config.apply`, `config.patch`, `agents.create`, `agents.update`, `agents.delete`, `channels.toggle`, `device.pair.approve`, `device.pair.deny`, `device.pair.revoke`, `teams.*`, `api_keys.*`, `tenants.*`
|
||||
`config.apply`, `config.patch`, `agents.create`, `agents.update`, `agents.delete`, `channels.toggle`, `device.pair.approve`, `device.pair.deny`, `device.pair.revoke`, `device.pair.update`, `teams.*`, `api_keys.*`, `tenants.*`
|
||||
|
||||
### Write Methods (Operator+)
|
||||
|
||||
|
||||
@@ -3,6 +3,7 @@ package methods
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"log/slog"
|
||||
"regexp"
|
||||
|
||||
@@ -258,7 +259,11 @@ func (m *PairingMethods) handleUpdate(ctx context.Context, client *gateway.Clien
|
||||
}
|
||||
|
||||
if err := m.service.SetPairingPermanent(ctx, params.SenderID, params.Channel, *params.Permanent); err != nil {
|
||||
client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrNotFound, err.Error()))
|
||||
code := protocol.ErrInternal
|
||||
if errors.Is(err, store.ErrPairedDeviceNotFound) {
|
||||
code = protocol.ErrNotFound
|
||||
}
|
||||
client.SendResponse(protocol.NewErrorResponse(req.ID, code, err.Error()))
|
||||
return
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,66 @@
|
||||
package methods
|
||||
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"fmt"
|
||||
"testing"
|
||||
|
||||
"github.com/nextlevelbuilder/goclaw/internal/bus"
|
||||
"github.com/nextlevelbuilder/goclaw/internal/gateway"
|
||||
"github.com/nextlevelbuilder/goclaw/internal/permissions"
|
||||
"github.com/nextlevelbuilder/goclaw/internal/store"
|
||||
"github.com/nextlevelbuilder/goclaw/pkg/protocol"
|
||||
)
|
||||
|
||||
// stubPermanentPairingStore answers SetPairingPermanent with a fixed error.
|
||||
type stubPermanentPairingStore struct {
|
||||
store.PairingStore
|
||||
err error
|
||||
}
|
||||
|
||||
func (s *stubPermanentPairingStore) SetPairingPermanent(context.Context, string, string, bool) error {
|
||||
return s.err
|
||||
}
|
||||
|
||||
func callPairingUpdate(t *testing.T, storeErr error) protocol.ResponseFrame {
|
||||
t.Helper()
|
||||
m := NewPairingMethods(&stubPermanentPairingStore{err: storeErr}, bus.New(), nil)
|
||||
client, out := gateway.NewCapturingTestClient(permissions.RoleAdmin, store.MasterTenantID, "admin-1", 1)
|
||||
raw, _ := json.Marshal(map[string]any{"senderId": "u1", "channel": "telegram", "permanent": true})
|
||||
|
||||
m.handleUpdate(t.Context(), client, &protocol.RequestFrame{ID: "r1", Params: raw})
|
||||
|
||||
var frame protocol.ResponseFrame
|
||||
if err := json.Unmarshal(<-out, &frame); err != nil {
|
||||
t.Fatalf("decode response: %v", err)
|
||||
}
|
||||
return frame
|
||||
}
|
||||
|
||||
func TestPairingUpdate_ErrorMapping(t *testing.T) {
|
||||
cases := []struct {
|
||||
name string
|
||||
storeErr error
|
||||
wantCode string
|
||||
}{
|
||||
{"not found", fmt.Errorf("%w: telegram/u1", store.ErrPairedDeviceNotFound), protocol.ErrNotFound},
|
||||
{"db failure", errors.New("connection reset"), protocol.ErrInternal},
|
||||
}
|
||||
for _, tc := range cases {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
frame := callPairingUpdate(t, tc.storeErr)
|
||||
if frame.OK || frame.Error == nil || frame.Error.Code != tc.wantCode {
|
||||
t.Fatalf("want error %s, got ok=%v error=%+v", tc.wantCode, frame.OK, frame.Error)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestPairingUpdate_OK(t *testing.T) {
|
||||
frame := callPairingUpdate(t, nil)
|
||||
if !frame.OK {
|
||||
t.Fatalf("want ok, got error %+v", frame.Error)
|
||||
}
|
||||
}
|
||||
@@ -1,6 +1,13 @@
|
||||
package store
|
||||
|
||||
import "context"
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
)
|
||||
|
||||
// ErrPairedDeviceNotFound is returned by SetPairingPermanent when there is no
|
||||
// live (non-expired) pairing for the sender/channel.
|
||||
var ErrPairedDeviceNotFound = errors.New("paired device not found")
|
||||
|
||||
// PairingRequest represents a pending pairing code.
|
||||
type PairingRequestData struct {
|
||||
@@ -15,7 +22,8 @@ type PairingRequestData struct {
|
||||
}
|
||||
|
||||
// PairedDeviceData represents an approved pairing.
|
||||
// ExpiresAt is Unix ms; nil means the pairing never expires.
|
||||
// ExpiresAt is Unix ms; nil means the pairing never expires, 0 means it
|
||||
// expires but the stored date could not be read.
|
||||
type PairedDeviceData struct {
|
||||
SenderID string `json:"sender_id" db:"sender_id"`
|
||||
Channel string `json:"channel" db:"channel"`
|
||||
|
||||
@@ -173,7 +173,7 @@ func (s *PGPairingStore) SetPairingPermanent(ctx context.Context, senderID, chan
|
||||
}
|
||||
n, _ := result.RowsAffected()
|
||||
if n == 0 {
|
||||
return fmt.Errorf("paired device not found: %s/%s", channel, senderID)
|
||||
return fmt.Errorf("%w: %s/%s", store.ErrPairedDeviceNotFound, channel, senderID)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
@@ -2,6 +2,7 @@ package pg
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
@@ -92,8 +93,8 @@ func TestPGPairing_SetPermanentDoesNotReviveExpired(t *testing.T) {
|
||||
t.Fatalf("expire pairing: %v", err)
|
||||
}
|
||||
|
||||
if err := s.SetPairingPermanent(ctx, "u1", "telegram", true); err == nil {
|
||||
t.Fatal("SetPairingPermanent on expired pairing: want error, got nil")
|
||||
if err := s.SetPairingPermanent(ctx, "u1", "telegram", true); !errors.Is(err, store.ErrPairedDeviceNotFound) {
|
||||
t.Fatalf("SetPairingPermanent on expired pairing: want ErrPairedDeviceNotFound, got %v", err)
|
||||
}
|
||||
if ok, _ := s.IsPaired(ctx, "u1", "telegram"); ok {
|
||||
t.Fatal("expired pairing was revived")
|
||||
|
||||
@@ -166,7 +166,7 @@ func (s *SQLitePairingStore) SetPairingPermanent(ctx context.Context, senderID,
|
||||
}
|
||||
n, _ := result.RowsAffected()
|
||||
if n == 0 {
|
||||
return fmt.Errorf("paired device not found: %s/%s", channel, senderID)
|
||||
return fmt.Errorf("%w: %s/%s", store.ErrPairedDeviceNotFound, channel, senderID)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
@@ -249,6 +249,8 @@ func (s *SQLitePairingStore) ListPaired(ctx context.Context) []store.PairedDevic
|
||||
}
|
||||
d.PairedAt = parseTimeToMillis(pairedAtStr)
|
||||
if expiresAtStr.Valid {
|
||||
// A parse miss yields 0: the pairing still expires (SQL compares the
|
||||
// raw value), only the date is unknown. Never report it as permanent.
|
||||
ms := parseTimeToMillis(expiresAtStr.String)
|
||||
d.ExpiresAt = &ms
|
||||
}
|
||||
|
||||
@@ -4,6 +4,7 @@ package sqlitestore
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"path/filepath"
|
||||
"testing"
|
||||
"time"
|
||||
@@ -102,8 +103,8 @@ func TestSQLitePairing_SetPermanentDoesNotReviveExpired(t *testing.T) {
|
||||
t.Fatalf("expire pairing: %v", err)
|
||||
}
|
||||
|
||||
if err := s.SetPairingPermanent(ctx, "u1", "telegram", true); err == nil {
|
||||
t.Fatal("SetPairingPermanent on expired pairing: want error, got nil")
|
||||
if err := s.SetPairingPermanent(ctx, "u1", "telegram", true); !errors.Is(err, store.ErrPairedDeviceNotFound) {
|
||||
t.Fatalf("SetPairingPermanent on expired pairing: want ErrPairedDeviceNotFound, got %v", err)
|
||||
}
|
||||
if ok, _ := s.IsPaired(ctx, "u1", "telegram"); ok {
|
||||
t.Fatal("expired pairing was revived")
|
||||
@@ -113,7 +114,23 @@ func TestSQLitePairing_SetPermanentDoesNotReviveExpired(t *testing.T) {
|
||||
func TestSQLitePairing_SetPermanentUnknownDevice(t *testing.T) {
|
||||
s, ctx := newTestSQLitePairingStore(t)
|
||||
|
||||
if err := s.SetPairingPermanent(ctx, "nobody", "telegram", true); err == nil {
|
||||
t.Fatal("want error for unknown device, got nil")
|
||||
if err := s.SetPairingPermanent(ctx, "nobody", "telegram", true); !errors.Is(err, store.ErrPairedDeviceNotFound) {
|
||||
t.Fatalf("want ErrPairedDeviceNotFound for unknown device, got %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
// An expiry the parser cannot read must not turn into "never expires".
|
||||
func TestSQLitePairing_UnreadableExpiryIsNotPermanent(t *testing.T) {
|
||||
s, ctx := newTestSQLitePairingStore(t)
|
||||
pairTestDevice(t, s, ctx, "u1")
|
||||
|
||||
if _, err := s.db.ExecContext(ctx, "UPDATE paired_devices SET expires_at = ? WHERE sender_id = ?",
|
||||
"9999-garbage", "u1"); err != nil {
|
||||
t.Fatalf("corrupt expiry: %v", err)
|
||||
}
|
||||
|
||||
got := findPaired(s.ListPaired(ctx), "u1")
|
||||
if got == nil || got.ExpiresAt == nil || *got.ExpiresAt != 0 {
|
||||
t.Fatalf("want u1 with ExpiresAt=0 (unknown date), got %+v", got)
|
||||
}
|
||||
}
|
||||
@@ -46,5 +46,9 @@
|
||||
"title": "Set Pairing Expiry",
|
||||
"description": "Return the pairing for {{channel}}:{{senderId}} to the standard term? It will expire in 30 days from now.",
|
||||
"confirmLabel": "Set expiry"
|
||||
},
|
||||
"toast": {
|
||||
"approveFailed": "Could not approve pairing",
|
||||
"updateFailed": "Could not change pairing expiry"
|
||||
}
|
||||
}
|
||||
@@ -46,5 +46,9 @@
|
||||
"title": "페어링 만료 설정",
|
||||
"description": "{{channel}}:{{senderId}} 페어링을 기본 기간으로 되돌리시겠습니까? 지금부터 30일 후 만료됩니다.",
|
||||
"confirmLabel": "만료 설정"
|
||||
},
|
||||
"toast": {
|
||||
"approveFailed": "페어링을 승인할 수 없습니다",
|
||||
"updateFailed": "페어링 만료를 변경할 수 없습니다"
|
||||
}
|
||||
}
|
||||
@@ -46,5 +46,9 @@
|
||||
"title": "Вернуть срок привязки",
|
||||
"description": "Вернуть привязке {{channel}}:{{senderId}} стандартный срок? Она истечёт через 30 дней, считая от сейчас.",
|
||||
"confirmLabel": "Вернуть срок"
|
||||
},
|
||||
"toast": {
|
||||
"approveFailed": "Не удалось одобрить привязку",
|
||||
"updateFailed": "Не удалось изменить срок привязки"
|
||||
}
|
||||
}
|
||||
@@ -46,5 +46,9 @@
|
||||
"title": "Đặt thời hạn ghép nối",
|
||||
"description": "Đưa ghép nối {{channel}}:{{senderId}} về thời hạn mặc định? Nó sẽ hết hạn sau 30 ngày kể từ bây giờ.",
|
||||
"confirmLabel": "Đặt thời hạn"
|
||||
},
|
||||
"toast": {
|
||||
"approveFailed": "Không thể phê duyệt ghép nối",
|
||||
"updateFailed": "Không thể thay đổi thời hạn ghép nối"
|
||||
}
|
||||
}
|
||||
@@ -46,5 +46,9 @@
|
||||
"title": "设置配对期限",
|
||||
"description": "将 {{channel}}:{{senderId}} 的配对恢复为默认期限?将从现在起 30 天后过期。",
|
||||
"confirmLabel": "设置期限"
|
||||
},
|
||||
"toast": {
|
||||
"approveFailed": "无法批准配对",
|
||||
"updateFailed": "无法更改配对期限"
|
||||
}
|
||||
}
|
||||
@@ -3,6 +3,8 @@ import { useWs } from "@/hooks/use-ws";
|
||||
import { useAuthStore } from "@/stores/use-auth-store";
|
||||
import { useWsEvent } from "@/hooks/use-ws-event";
|
||||
import { Methods, Events } from "@/api/protocol";
|
||||
import { toast } from "@/stores/use-toast-store";
|
||||
import i18next from "i18next";
|
||||
|
||||
export interface PendingPairing {
|
||||
code: string;
|
||||
@@ -20,7 +22,7 @@ export interface PairedDevice {
|
||||
chat_id: string;
|
||||
paired_at: number;
|
||||
paired_by: string;
|
||||
/** Unix ms; null means the pairing never expires. */
|
||||
/** Unix ms; null means the pairing never expires, 0 means it expires on an unknown date. */
|
||||
expires_at: number | null;
|
||||
}
|
||||
|
||||
@@ -62,8 +64,15 @@ export function useNodes() {
|
||||
|
||||
const approvePairing = useCallback(
|
||||
async (code: string, permanent = false) => {
|
||||
await ws.call(Methods.PAIRING_APPROVE, { code, permanent });
|
||||
load();
|
||||
try {
|
||||
await ws.call(Methods.PAIRING_APPROVE, { code, permanent });
|
||||
} catch (err) {
|
||||
// With permanent=true the device may already be paired with the default
|
||||
// TTL when this fails; the reload below shows the actual state.
|
||||
toast.error(i18next.t("nodes:toast.approveFailed"), err instanceof Error ? err.message : "");
|
||||
} finally {
|
||||
load();
|
||||
}
|
||||
},
|
||||
[ws, load],
|
||||
);
|
||||
@@ -86,8 +95,13 @@ export function useNodes() {
|
||||
|
||||
const setPairingPermanent = useCallback(
|
||||
async (senderId: string, channel: string, permanent: boolean) => {
|
||||
await ws.call(Methods.PAIRING_UPDATE, { senderId, channel, permanent });
|
||||
load();
|
||||
try {
|
||||
await ws.call(Methods.PAIRING_UPDATE, { senderId, channel, permanent });
|
||||
} catch (err) {
|
||||
toast.error(i18next.t("nodes:toast.updateFailed"), err instanceof Error ? err.message : "");
|
||||
} finally {
|
||||
load();
|
||||
}
|
||||
},
|
||||
[ws, load],
|
||||
);
|
||||
|
||||
@@ -151,8 +151,10 @@ export function NodesPage() {
|
||||
<Badge variant="secondary" className="gap-1">
|
||||
<InfinityIcon className="h-3 w-3" /> {t("never")}
|
||||
</Badge>
|
||||
) : (
|
||||
) : d.expires_at > 0 ? (
|
||||
formatDate(new Date(d.expires_at))
|
||||
) : (
|
||||
"--"
|
||||
)}
|
||||
</td>
|
||||
<td className="px-4 py-3 text-right whitespace-nowrap">
|
||||
|
||||
@@ -42,6 +42,7 @@ The first request must be a `connect` handshake. Authentication supports three p
|
||||
| `device.pair.approve` | Approve a pairing code |
|
||||
| `device.pair.list` | List pending and approved pairings |
|
||||
| `device.pair.revoke` | Revoke a pairing |
|
||||
| `device.pair.update` | Make a pairing permanent or restore the default TTL |
|
||||
|
||||
## Events (server push)
|
||||
|
||||
|
||||
Reference in new issue
Block a user