amadan

oficina / meet Public

agents: fail-closed lookups, an owned agentByID, the cap and dup check in the store

Task 1 review fix round 1:

1. agentByToken/agentByID no longer fall back to a bare-ID Person
   when the operator row is missing; both resolve the operator
   through store.personByID and answer false (not-found) when it is
   gone, deleted this round's operatorFor helper.
2. agentByID gains an operator parameter and is scoped on
   operator_person_id itself (the 404 shape every owned row gets);
   handlers_settings.go's agentForOperator now leans on that scoping
   instead of comparing OperatorPersonID by hand.
3. createAgent refuses a zero-value operator.
4. createAgent refuses a 21st live agent with errAgentLimit (moved
   from handlers_settings.go, along with maxLiveAgents); the handler
   drops its own pre-check and renders the store's error instead.
5. createAgent refuses a duplicate live name with errAgentDuplicate
   (a Go-side count, not a unique index, so a revoked name is free
   to reuse) — moved from handlers_settings.go the same way, with a
   new handler-level test.
6. Unit-test hygiene: no test prints a token on failure, only its
   shape (prefix, length, body charset); two mints are asserted to
   differ; a new test mints two agents, revokes the older, and checks
   both newest-first ordering and that the revoked row stays listed.

Both errAgentLimit and errAgentDuplicate now live in agents.go, next
to errAgentName.

Note for the record: a developer database that had already applied
the pre-rename 0002_create_agents migration (Task 1, before the
rename to 0002_agents.sql) would fail to boot against this tree,
since the ledger would carry an ID this Schema no longer has. That
migration never shipped, so no such database exists.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CzvnuKhLzudvq7jZ7SvVt2
Paul Campbell pushed by paul@keymail.dev e9539a34dd07fd13d4ff8c9c98adea6735ea8b2f parent d9f3b9e
4 files changed, +302 −102
  • internal/meet/agents.go +69 −26
  • internal/meet/agents_test.go +191 −8
  • internal/meet/handlers_settings.go +10 −68
  • internal/meettest/agents_test.go +32 −0

CI passed — run details

diff --git a/internal/meet/agents.go b/internal/meet/agents.go
index 26bc192..955f7b4 100644
--- a/internal/meet/agents.go
+++ b/internal/meet/agents.go
@@ -18,6 +18,25 @@ var agentNameRe = regexp.MustCompile(`^[A-Za-z0-9_-]{1,64}$`)
var errAgentName = errors.New("meet: an agent name is 1 to 64 letters, digits, - or _")
+// errAgentOperator guards against a caller that forgot to load a real
+// Person before minting: a zero Person is never a legitimate operator.
+var errAgentOperator = errors.New("meet: an agent needs a real operator")
+
+// maxLiveAgents is the operator-level ceiling (fix round 1, item 4):
+// an operator who can mint agents without limit can mint without
+// limit, and twenty is generous for the handful-of-automations-per-
+// person shape this slice is aimed at. Revoked agents do not count
+// against it — only live ones do.
+const maxLiveAgents = 20
+
+var errAgentLimit = errors.New("meet: an operator may hold at most 20 live agents")
+
+// errAgentDuplicate is a Go-side check (fix round 1, item 5), not a
+// unique index: a name is free to reuse the moment its old row is
+// revoked, which a partial unique index would not express as cleanly
+// as a plain query does.
+var errAgentDuplicate = errors.New("meet: an operator may not have two live agents with the same name")
+
// newAgentToken mints "mt_" + 32 random bytes, base64url without
// padding, and its SHA-256 hex. The token exists in plaintext only on
// its way to the one page that shows it (§8.1).
@@ -42,17 +61,47 @@ func principal(ag Agent, operator Person) string { return ag.Name + "~" + operat
func actorForAgent(ag Agent) string { return "agent:" + strconv.FormatInt(ag.ID, 10) }
+// createAgent validates the name, refuses a made-up operator, and —
+// inside one transaction, so a concurrent create from the same
+// operator cannot slip past either check — refuses past the live cap
+// or a live duplicate name before minting and storing the token's
+// hash.
func (s *store) createAgent(ctx context.Context, name string, operator Person) (Agent, string, error) {
name = strings.TrimSpace(name)
if !agentNameRe.MatchString(name) {
return Agent{}, "", errAgentName
}
+ if operator.ID == 0 {
+ return Agent{}, "", errAgentOperator
+ }
token, hash, err := newAgentToken()
if err != nil {
return Agent{}, "", err
}
- ag := Agent{Name: name, OperatorPersonID: operator.ID, TokenHash: hash, CreatedAt: s.now().UTC()}
- if err := s.g.WithContext(ctx).Create(&ag).Error; err != nil {
+ var ag Agent
+ err = s.g.WithContext(ctx).Transaction(func(tx *gorm.DB) error {
+ var live int64
+ if err := tx.Model(&Agent{}).
+ Where("operator_person_id = ? AND revoked_at IS NULL", operator.ID).
+ Count(&live).Error; err != nil {
+ return err
+ }
+ if live >= maxLiveAgents {
+ return errAgentLimit
+ }
+ var dup int64
+ if err := tx.Model(&Agent{}).
+ Where("operator_person_id = ? AND name = ? AND revoked_at IS NULL", operator.ID, name).
+ Count(&dup).Error; err != nil {
+ return err
+ }
+ if dup > 0 {
+ return errAgentDuplicate
+ }
+ ag = Agent{Name: name, OperatorPersonID: operator.ID, TokenHash: hash, CreatedAt: s.now().UTC()}
+ return tx.Create(&ag).Error
+ })
+ if err != nil {
return Agent{}, "", err
}
return ag, token, nil
@@ -73,11 +122,12 @@ func (s *store) revokeAgent(ctx context.Context, id int64, operator Person) (boo
return res.RowsAffected == 1, res.Error
}
-// agentByToken resolves a bearer to its agent and operator. Unknown and
-// revoked both answer false; the caller must not tell them apart.
-// Whether the operator currently holds member standing is a separate
-// question the caller re-derives (spec §8.1, §13); finding the agent
-// row is all this answers.
+// agentByToken resolves a bearer to its agent and operator. Unknown,
+// revoked, and an agent whose operator row no longer exists all
+// answer false with no error — the caller must not tell them apart
+// (fix round 1, item 1: fail closed, not a bare-ID Person). Whether
+// the operator currently holds member standing is a separate question
+// the caller re-derives (spec §8.1, §13).
func (s *store) agentByToken(ctx context.Context, token string) (Agent, Person, bool, error) {
var ag Agent
err := s.g.WithContext(ctx).Where("token_hash = ? AND revoked_at IS NULL", hashAgentToken(token)).Take(&ag).Error
@@ -87,41 +137,34 @@ func (s *store) agentByToken(ctx context.Context, token string) (Agent, Person,
if err != nil {
return Agent{}, Person{}, false, err
}
- op, err := s.operatorFor(ctx, ag)
+ op, found, err := s.personByID(ctx, ag.OperatorPersonID)
if err != nil {
return Agent{}, Person{}, false, err
}
+ if !found {
+ return Agent{}, Person{}, false, nil
+ }
return ag, op, true, nil
}
-func (s *store) agentByID(ctx context.Context, id int64) (Agent, Person, bool, error) {
+// agentByID resolves an agent scoped to its operator (fix round 1,
+// item 2): a foreign or unknown id both answer false, the same 404
+// shape scope.Owned gives every other owned row.
+func (s *store) agentByID(ctx context.Context, id int64, operator Person) (Agent, Person, bool, error) {
var ag Agent
- err := s.g.WithContext(ctx).Where("id = ?", id).Take(&ag).Error
+ err := s.g.WithContext(ctx).Where("id = ? AND operator_person_id = ?", id, operator.ID).Take(&ag).Error
if errors.Is(err, gorm.ErrRecordNotFound) {
return Agent{}, Person{}, false, nil
}
if err != nil {
return Agent{}, Person{}, false, err
}
- op, err := s.operatorFor(ctx, ag)
- if err != nil {
- return Agent{}, Person{}, false, err
- }
- return ag, op, true, nil
-}
-
-// operatorFor loads the agent's operator by id, falling back to a
-// bare-ID Person when the row cannot be found: resolving the agent
-// itself never fails on that account, since the operator's current
-// standing is re-derived by the caller (an absent row simply derives
-// to no standing at all).
-func (s *store) operatorFor(ctx context.Context, ag Agent) (Person, error) {
op, found, err := s.personByID(ctx, ag.OperatorPersonID)
if err != nil {
- return Person{}, err
+ return Agent{}, Person{}, false, err
}
if !found {
- return Person{ID: ag.OperatorPersonID}, nil
+ return Agent{}, Person{}, false, nil
}
- return op, nil
+ return ag, op, true, nil
}
diff --git a/internal/meet/agents_test.go b/internal/meet/agents_test.go
index 174fb26..e6fb824 100644
--- a/internal/meet/agents_test.go
+++ b/internal/meet/agents_test.go
@@ -2,32 +2,61 @@ package meet
import (
"context"
+ "errors"
+ "regexp"
"strconv"
"strings"
"testing"
+ "time"
)
// itoa is the test package's own decimal formatter, used only to spell
// out the actor string an assertion expects.
func itoa(id int64) string { return strconv.FormatInt(id, 10) }
+var agentTokenBodyRe = regexp.MustCompile(`^[A-Za-z0-9_-]+$`)
+
func TestAgentTokenIsShownOnceAndStoredHashed(t *testing.T) {
s, _ := testStore(t)
ctx := context.Background()
- op := Person{ID: 1, Email: "paul@example.com", Name: "paul"}
+ op, err := s.ensurePerson(ctx, "paul@example.com")
+ if err != nil {
+ t.Fatal(err)
+ }
ag, token, err := s.createAgent(ctx, "claude", op)
if err != nil {
t.Fatal(err)
}
- if !strings.HasPrefix(token, "mt_") || len(token) < 40 {
- t.Fatalf("token shape %q", token)
+ // The token's own value must never appear in a failure message —
+ // only its shape does (fix round 1, item 6).
+ if !strings.HasPrefix(token, "mt_") {
+ t.Fatal("token lacks the mt_ prefix")
+ }
+ if len(token) != 46 {
+ t.Fatalf("token length %d, want 46", len(token))
+ }
+ if !agentTokenBodyRe.MatchString(token[3:]) {
+ t.Fatal("token body after mt_ is not [A-Za-z0-9_-]+")
+ }
+ if ag.TokenHash != hashAgentToken(token) || len(ag.TokenHash) != 64 {
+ t.Fatalf("stored hash %q does not match the minted token's hash", ag.TokenHash)
+ }
+ if ag.TokenHash == token {
+ t.Fatal("the hash equals the plaintext token")
+ }
+
+ // Two mints never coincide.
+ token2, _, err := newAgentToken()
+ if err != nil {
+ t.Fatal(err)
}
- if ag.TokenHash != hashAgentToken(token) || ag.TokenHash == token || len(ag.TokenHash) != 64 {
- t.Fatalf("stored %q for %q", ag.TokenHash, token)
+ if token2 == token {
+ t.Fatal("two mints produced the same token")
}
+
got, opGot, ok, err := s.agentByToken(ctx, token)
if err != nil || !ok || got.ID != ag.ID || opGot.ID != op.ID {
- t.Fatalf("lookup %+v %+v %v %v", got, opGot, ok, err)
+ t.Fatalf("lookup: ok=%v err=%v got.ID=%d opGot.ID=%d", ok, err, got.ID, opGot.ID)
}
if _, _, ok, _ := s.agentByToken(ctx, "mt_nope"); ok {
t.Fatal("an unknown token must not resolve")
@@ -40,11 +69,42 @@ func TestAgentTokenIsShownOnceAndStoredHashed(t *testing.T) {
}
}
+// Fix round 1, item 1: resolving an agent whose operator row no longer
+// exists must fail closed (false, no error, indistinguishable from an
+// unknown token) rather than fall back to a bare-ID Person.
+func TestAgentByTokenFailsClosedWhenTheOperatorRowIsGone(t *testing.T) {
+ s, _ := testStore(t)
+ ctx := context.Background()
+ op, err := s.ensurePerson(ctx, "ghost@example.com")
+ if err != nil {
+ t.Fatal(err)
+ }
+ ag, token, err := s.createAgent(ctx, "claude", op)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err := s.g.WithContext(ctx).Where("id = ?", op.ID).Delete(&Person{}).Error; err != nil {
+ t.Fatal(err)
+ }
+ if _, _, ok, err := s.agentByToken(ctx, token); ok || err != nil {
+ t.Fatalf("agentByToken with a deleted operator: ok=%v err=%v", ok, err)
+ }
+ if _, _, ok, err := s.agentByID(ctx, ag.ID, op); ok || err != nil {
+ t.Fatalf("agentByID with a deleted operator: ok=%v err=%v", ok, err)
+ }
+}
+
func TestRevokedAgentDoesNotResolveAndOnlyTheOperatorMayRevoke(t *testing.T) {
s, _ := testStore(t)
ctx := context.Background()
- op := Person{ID: 1, Email: "paul@example.com", Name: "paul"}
- other := Person{ID: 2, Email: "bea@example.com", Name: "bea"}
+ op, err := s.ensurePerson(ctx, "paul@example.com")
+ if err != nil {
+ t.Fatal(err)
+ }
+ other, err := s.ensurePerson(ctx, "bea@example.com")
+ if err != nil {
+ t.Fatal(err)
+ }
ag, token, _ := s.createAgent(ctx, "claude", op)
if ok, _ := s.revokeAgent(ctx, ag.ID, other); ok {
t.Fatal("another person revoked the agent")
@@ -67,6 +127,66 @@ func TestRevokedAgentDoesNotResolveAndOnlyTheOperatorMayRevoke(t *testing.T) {
}
}
+// Fix round 1, item 2: agentByID is scoped to its operator, the same
+// 404 shape as every other owned row.
+func TestAgentByIDIsScopedToItsOperator(t *testing.T) {
+ s, _ := testStore(t)
+ ctx := context.Background()
+ op, _ := s.ensurePerson(ctx, "paul@example.com")
+ other, _ := s.ensurePerson(ctx, "bea@example.com")
+ ag, _, err := s.createAgent(ctx, "claude", op)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if _, _, ok, err := s.agentByID(ctx, ag.ID, other); ok || err != nil {
+ t.Fatalf("another operator's agentByID: ok=%v err=%v", ok, err)
+ }
+ got, gotOp, ok, err := s.agentByID(ctx, ag.ID, op)
+ if err != nil || !ok || got.ID != ag.ID || gotOp.ID != op.ID {
+ t.Fatalf("the real operator's agentByID: got=%+v ok=%v err=%v", got, ok, err)
+ }
+ if _, _, ok, err := s.agentByID(ctx, 999999, op); ok || err != nil {
+ t.Fatalf("unknown id: ok=%v err=%v", ok, err)
+ }
+}
+
+// Fix round 1, item 6: mint two agents, revoke the older, and check
+// both that the list stays newest-first and that the revoked row is
+// still there (revoking removes nothing, only marks it).
+func TestAgentsForOrdersNewestFirstAndKeepsRevokedRows(t *testing.T) {
+ s, now := testStore(t)
+ ctx := context.Background()
+ op, _ := s.ensurePerson(ctx, "paul@example.com")
+ older, _, err := s.createAgent(ctx, "older", op)
+ if err != nil {
+ t.Fatal(err)
+ }
+ *now = now.Add(time.Minute)
+ newer, _, err := s.createAgent(ctx, "newer", op)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if ok, err := s.revokeAgent(ctx, older.ID, op); !ok || err != nil {
+ t.Fatalf("revoke: ok=%v err=%v", ok, err)
+ }
+ list, err := s.agentsFor(ctx, op)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(list) != 2 {
+ t.Fatalf("agentsFor returned %d rows, want 2", len(list))
+ }
+ if list[0].ID != newer.ID || list[1].ID != older.ID {
+ t.Fatalf("order = [%d, %d], want newest [%d] then oldest [%d]", list[0].ID, list[1].ID, newer.ID, older.ID)
+ }
+ if list[1].RevokedAt == nil {
+ t.Fatal("the revoked row is missing from the list")
+ }
+ if list[0].RevokedAt != nil {
+ t.Fatal("the live row was reported revoked")
+ }
+}
+
func TestAgentNameRules(t *testing.T) {
s, _ := testStore(t)
ctx := context.Background()
@@ -80,3 +200,66 @@ func TestAgentNameRules(t *testing.T) {
t.Fatalf("trimmed valid name refused: %v", err)
}
}
+
+// Fix round 1, item 3: a zero Person is never a legitimate operator.
+func TestCreateAgentRefusesAZeroOperator(t *testing.T) {
+ s, _ := testStore(t)
+ ctx := context.Background()
+ if _, _, err := s.createAgent(ctx, "claude", Person{}); err == nil {
+ t.Fatal("created an agent for a zero-value operator")
+ }
+}
+
+// Fix round 1, item 4: the cap lives in the store now, as
+// errAgentLimit; revoked agents never count against it.
+func TestCreateAgentRefusesTheTwentyFirstLiveAgent(t *testing.T) {
+ s, _ := testStore(t)
+ ctx := context.Background()
+ op, _ := s.ensurePerson(ctx, "paul@example.com")
+ for i := 0; i < maxLiveAgents; i++ {
+ if _, _, err := s.createAgent(ctx, "agent"+itoa(int64(i)), op); err != nil {
+ t.Fatalf("create agent %d: %v", i, err)
+ }
+ }
+ if _, _, err := s.createAgent(ctx, "one-too-many", op); !errors.Is(err, errAgentLimit) {
+ t.Fatalf("21st create returned %v, want errAgentLimit", err)
+ }
+ list, _ := s.agentsFor(ctx, op)
+ if len(list) != maxLiveAgents {
+ t.Fatalf("agent count after the refused create: %d, want %d", len(list), maxLiveAgents)
+ }
+
+ // Revoking one frees a slot.
+ if ok, _ := s.revokeAgent(ctx, list[0].ID, op); !ok {
+ t.Fatal("revoke to free a slot failed")
+ }
+ if _, _, err := s.createAgent(ctx, "one-more", op); err != nil {
+ t.Fatalf("create after revoking one: %v", err)
+ }
+}
+
+// Fix round 1, item 5: a duplicate live name is a Go-side check, not a
+// unique index — so a revoked name is free to reuse.
+func TestCreateAgentRefusesADuplicateLiveNameButAllowsReuseAfterRevoke(t *testing.T) {
+ s, _ := testStore(t)
+ ctx := context.Background()
+ op, _ := s.ensurePerson(ctx, "paul@example.com")
+ first, _, err := s.createAgent(ctx, "scheduler", op)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if _, _, err := s.createAgent(ctx, "scheduler", op); !errors.Is(err, errAgentDuplicate) {
+ t.Fatalf("duplicate create returned %v, want errAgentDuplicate", err)
+ }
+ // Another operator may use the same name freely.
+ other, _ := s.ensurePerson(ctx, "bea@example.com")
+ if _, _, err := s.createAgent(ctx, "scheduler", other); err != nil {
+ t.Fatalf("another operator's same name was refused: %v", err)
+ }
+ if ok, _ := s.revokeAgent(ctx, first.ID, op); !ok {
+ t.Fatal("revoke failed")
+ }
+ if _, _, err := s.createAgent(ctx, "scheduler", op); err != nil {
+ t.Fatalf("reuse after revoke was refused: %v", err)
+ }
+}
diff --git a/internal/meet/handlers_settings.go b/internal/meet/handlers_settings.go
index c4af922..4a9a575 100644
--- a/internal/meet/handlers_settings.go
+++ b/internal/meet/handlers_settings.go
@@ -8,32 +8,6 @@ import (
"github.com/go-chi/chi/v5"
)
-// maxLiveAgents is the operator-level ceiling the reviewer asked for
-// ahead of a written spec number: an operator who can mint agents
-// without limit can mint without limit, and twenty is generous for the
-// handful-of-automations-per-person shape this slice is aimed at.
-// Revoked agents do not count against it — only live ones do.
-//
-// Enforced here, in the handler, rather than in store.createAgent:
-// the Task 1 review flagged this cap (and a duplicate-live-name
-// refusal) for a fix round that moves both into createAgent as
-// sentinel errors on the store side. errAgentLimit and errAgentDuplicate
-// below are that fix round's sentinels, declared here rather than in
-// agents.go because this task does not write that file. Today
-// store.createAgent never returns either one, so the switch in
-// createAgentHandler that matches them is inert scaffolding and this
-// constant plus liveAgentCount below is what actually enforces the
-// cap. When the fix round lands createAgent's own check, delete
-// maxLiveAgents, liveAgentCount and the handler's own count (keeping
-// only the switch's existing cases), and delete these two var
-// declarations in favour of agents.go's real ones.
-const maxLiveAgents = 20
-
-var (
- errAgentLimit = errors.New("meet: an operator may hold at most 20 live agents")
- errAgentDuplicate = errors.New("meet: an operator may not have two live agents with the same name")
-)
-
// settingsRow is one line of the agent list: never the token, which
// exists in plaintext only on the page that minted it (spec §8.1).
// A revoked row shows that and nothing else; a live one shows the
@@ -73,38 +47,16 @@ func (a *app) renderSettings(w http.ResponseWriter, r *http.Request, name, nameE
}))
}
-// liveAgentCount is agentsFor filtered to the rows still worth
-// something: this task's cap is a handler-level rule, not a store
-// method, so it counts here rather than growing the store's interface
-// for one caller.
-func (a *app) liveAgentCount(agents []Agent) int {
- live := 0
- for _, ag := range agents {
- if ag.RevokedAt == nil {
- live++
- }
- }
- return live
-}
-
// createAgentHandler is POST /settings/agents: mints a token and shows
// it exactly once, on agent_once.html, alongside the principal string
-// the meetings list will show for anything this agent creates.
+// the meetings list will show for anything this agent creates. The
+// cap and the duplicate-name refusal are store.createAgent's own
+// checks (fix round 1, items 4-5); this handler only renders whatever
+// it answers.
func (a *app) createAgentHandler(w http.ResponseWriter, r *http.Request) {
_ = r.ParseForm()
name := r.FormValue("name")
p, _ := currentPerson(r)
- agents, err := a.store.agentsFor(r.Context(), p)
- if err != nil {
- a.logger.Error("meet: list agents", "err", err)
- a.ErrorPage(w, r, http.StatusInternalServerError, "")
- return
- }
- if a.liveAgentCount(agents) >= maxLiveAgents {
- w.WriteHeader(http.StatusUnprocessableEntity)
- a.renderSettings(w, r, name, "You already have 20 live agents. Revoke one before creating another.")
- return
- }
ag, token, err := a.store.createAgent(r.Context(), name, p)
if err != nil {
switch {
@@ -112,9 +64,6 @@ func (a *app) createAgentHandler(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusUnprocessableEntity)
a.renderSettings(w, r, name, "Give the agent a name of 1 to 64 letters, digits, - or _.")
case errors.Is(err, errAgentLimit):
- // Not reachable today (see maxLiveAgents' comment); kept so
- // the fix round's store-side cap renders correctly the
- // moment createAgent starts returning it.
w.WriteHeader(http.StatusUnprocessableEntity)
a.renderSettings(w, r, name, "You already have 20 live agents. Revoke one before creating another.")
case errors.Is(err, errAgentDuplicate):
@@ -133,30 +82,23 @@ func (a *app) createAgentHandler(w http.ResponseWriter, r *http.Request) {
// agentForOperator loads the agent named by the URL's id, refusing with
// the same 404 shape meetingForMember uses (never a 403) whether the id
-// is unknown or belongs to another operator: the two must be
-// indistinguishable from outside.
-//
-// store.agentByID is not itself ownership-scoped (Task 1 review): it
-// answers whether the id exists at all, for anyone. The comparison
-// below is what actually keeps a foreign agent out — done before this
-// function returns true, so nothing downstream ever sees a row it
-// should not. When agentByID's signature grows an operator parameter,
-// call it with p here and this comparison becomes redundant defense in
-// depth rather than the only check.
+// is unknown or belongs to another operator: store.agentByID is itself
+// ownership-scoped now (fix round 1, item 2), so there is nothing left
+// for this function to check beyond what agentByID already answers.
func (a *app) agentForOperator(w http.ResponseWriter, r *http.Request) (Agent, bool) {
id, err := strconv.ParseInt(chi.URLParam(r, "id"), 10, 64)
if err != nil {
a.ErrorPage(w, r, http.StatusNotFound, "")
return Agent{}, false
}
- ag, _, ok, err := a.store.agentByID(r.Context(), id)
+ p, _ := currentPerson(r)
+ ag, _, ok, err := a.store.agentByID(r.Context(), id, p)
if err != nil {
a.logger.Error("meet: load agent", "err", err)
a.ErrorPage(w, r, http.StatusInternalServerError, "")
return Agent{}, false
}
- p, _ := currentPerson(r)
- if !ok || ag.OperatorPersonID != p.ID {
+ if !ok {
a.ErrorPage(w, r, http.StatusNotFound, "")
return Agent{}, false
}
diff --git a/internal/meettest/agents_test.go b/internal/meettest/agents_test.go
index 732bb0e..1273fa0 100644
--- a/internal/meettest/agents_test.go
+++ b/internal/meettest/agents_test.go
@@ -255,6 +255,38 @@ func TestCreateIsRefusedAtTwentyLiveAgents(t *testing.T) {
}
}
+// Fix round 1, item 5: a duplicate live name is refused with a
+// sentence and mints nothing; revoking the original frees the name.
+func TestCreateIsRefusedForADuplicateLiveNameButFreedByRevoke(t *testing.T) {
+ x, _, _ := memberWithMeeting(t)
+ c := x.browser(t)
+ c.signIn("paul@example.com")
+
+ rec := c.post("/settings/agents", url.Values{"name": {"scheduler"}})
+ if rec.Code != http.StatusOK {
+ t.Fatalf("first create: %d\n%s", rec.Code, rec.Body.String())
+ }
+ rec = c.post("/settings/agents", url.Values{"name": {"scheduler"}})
+ if rec.Code != http.StatusUnprocessableEntity {
+ t.Fatalf("duplicate create: %d\n%s", rec.Code, rec.Body.String())
+ }
+ if !strings.Contains(rec.Body.String(), "live agent with that name") {
+ t.Fatalf("no duplicate-name sentence on the refusal:\n%s", rec.Body.String())
+ }
+ list := c.get("/settings").Body.String()
+ if got := strings.Count(list, "Revoke"); got != 1 {
+ t.Fatalf("expected exactly 1 live agent, found %d Revoke links:\n%s", got, list)
+ }
+
+ id := agentRevokeID(t, list)
+ if rec := c.post("/settings/agents/"+id+"/revoke", nil); rec.Code != http.StatusSeeOther {
+ t.Fatalf("revoke: %d", rec.Code)
+ }
+ if rec := c.post("/settings/agents", url.Values{"name": {"scheduler"}}); rec.Code != http.StatusOK {
+ t.Fatalf("recreate after revoke: %d\n%s", rec.Code, rec.Body.String())
+ }
+}
+
// spec §0.1/§4.4's rule, applied to the mint token: nothing an app logs
// may ever carry it.
func TestMintedAgentTokenNeverAppearsInTheLog(t *testing.T) {