hold: look crew members up by rkey instead of walking the collection

ValidateBlobWriteAccess, ValidateBlobReadAccess, ValidateOwnerOrCrewAdmin and getCrewTier each listed every crew record to find one member: open a carstore session, walk the MST, CBOR-decode each record, compare DIDs. That ran on every multipart call from the appview, including the part URL request for every 10MB, and on every getBlob presign, so on a hold with hundreds of crew each part cost hundreds of decodes. lookupCrewMember tries the deterministic rkey first (one record read) and only falls back to the walk on a not-found miss. The fallback is required: records created before the hash-rkey scheme sit at a TID rkey, and the boot-time migration that rekeyed them only existed between e0a2dda and b2d6842, so a hold that upgraded across that window still has them. Members hit the O(1) path; only genuine non-members pay for the walk, and they are denied anyway. Every authorization decision and error string is unchanged. Tests cover the deterministic hit, a legacy TID-keyed member found only through the fallback, a non-member, and a storage error surfacing as an error rather than a silent denial. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Yf1ZVA7sXYhQNb9tCo1m5
2026-09-20 01:04:15 +00:00 · 2026-09-09 09:31:16 -05:00
parent 9228579b07
commit 61a934debb
4 changed files with 182 additions and 38 deletions
@@ -276,20 +276,18 @@ func ValidateOwnerOrCrewAdmin(r *http.Request, pds *HoldPDS, httpClient HTTPClie
 	}

 	// Check if user is crew with admin permission
-	crew, err := pds.ListCrewMembers(r.Context())
+	member, isCrew, err := pds.lookupCrewMember(r.Context(), user.DID)
 	if err != nil {
 		return nil, fmt.Errorf("failed to check crew membership: %w", err)
 	}

-	for _, member := range crew {
-		if member.Record.Member == user.DID {
-			// Check if this crew member has crew:admin permission
-			if slices.Contains(member.Record.Permissions, "crew:admin") {
-				return user, nil
-			}
-			// User is crew but doesn't have admin permission
-			return nil, NewAuthError("crew:admin", "crew member lacks permission", "crew:admin")
+	if isCrew {
+		// Check if this crew member has crew:admin permission
+		if slices.Contains(member.Permissions, "crew:admin") {
+			return user, nil
 		}
+		// User is crew but doesn't have admin permission
+		return nil, NewAuthError("crew:admin", "crew member lacks permission", "crew:admin")
 	}

 	// User is neither owner nor authorized crew
@@ -408,20 +406,18 @@ func ValidateBlobWriteAccess(r *http.Request, pds *HoldPDS, httpClient HTTPClien
 	}

 	// Check if user is crew with blob:write permission
-	crew, err := pds.ListCrewMembers(r.Context())
+	member, isCrew, err := pds.lookupCrewMember(r.Context(), user.DID)
 	if err != nil {
 		return nil, fmt.Errorf("failed to check crew membership: %w", err)
 	}

-	for _, member := range crew {
-		if member.Record.Member == user.DID {
-			// Check if this crew member has blob:write permission
-			if slices.Contains(member.Record.Permissions, "blob:write") {
-				return user, nil
-			}
-			// User is crew but doesn't have write permission
-			return nil, NewAuthError("blob:write", "crew member lacks permission", "blob:write")
+	if isCrew {
+		// Check if this crew member has blob:write permission
+		if slices.Contains(member.Permissions, "blob:write") {
+			return user, nil
 		}
+		// User is crew but doesn't have write permission
+		return nil, NewAuthError("blob:write", "crew member lacks permission", "blob:write")
 	}

 	// User is neither owner nor authorized crew
@@ -477,22 +473,20 @@ func ValidateBlobReadAccess(r *http.Request, pds *HoldPDS, httpClient HTTPClient

 	// Check if user is crew with blob:read or blob:write permission
 	// Note: blob:write implicitly grants blob:read access
-	crew, err := pds.ListCrewMembers(r.Context())
+	member, isCrew, err := pds.lookupCrewMember(r.Context(), user.DID)
 	if err != nil {
 		return nil, fmt.Errorf("failed to check crew membership: %w", err)
 	}

-	for _, member := range crew {
-		if member.Record.Member == user.DID {
-			// Check if this crew member has blob:read or blob:write permission
-			// blob:write implicitly grants read access (can't push without pulling)
-			if slices.Contains(member.Record.Permissions, "blob:read") ||
-				slices.Contains(member.Record.Permissions, "blob:write") {
-				return user, nil
-			}
-			// User is crew but doesn't have read or write permission
-			return nil, NewAuthError("blob:read", "crew member lacks permission", "blob:read", "blob:write")
+	if isCrew {
+		// Check if this crew member has blob:read or blob:write permission
+		// blob:write implicitly grants read access (can't push without pulling)
+		if slices.Contains(member.Permissions, "blob:read") ||
+			slices.Contains(member.Permissions, "blob:write") {
+			return user, nil
 		}
+		// User is crew but doesn't have read or write permission
+		return nil, NewAuthError("blob:read", "crew member lacks permission", "blob:read", "blob:write")
 	}

 	// User is neither owner nor authorized crew
@@ -71,6 +71,43 @@ func (p *HoldPDS) GetCrewMemberByDID(ctx context.Context, memberDID string) (cid
 	return p.GetCrewMember(ctx, rkey)
 }

+// lookupCrewMember resolves this hold's crew record for a DID, preferring the
+// O(1) deterministic-rkey read over a full walk of the crew collection.
+//
+// Every current writer (AddCrewMember, UpdateCrewMemberTier, bootstrap, crew
+// import, the join endpoint) stores crew records at atproto.CrewRecordKey(did),
+// but records created before that scheme landed sit at a TID rkey. The boot-time
+// rekey migration that fixed those only shipped between 2026-01-06 and
+// 2026-05-04, so a hold upgrading straight across that window still has
+// TID-keyed crew records. A miss therefore falls back to the full walk rather
+// than denying access to a legacy member.
+//
+// Returns (nil, false, nil) when the DID is genuinely not crew, and a non-nil
+// error only for a real storage failure.
+func (p *HoldPDS) lookupCrewMember(ctx context.Context, memberDID string) (*atproto.CrewRecord, bool, error) {
+	_, record, err := p.GetCrewMemberByDID(ctx, memberDID)
+	if err == nil {
+		return record, true, nil
+	}
+	if !errors.Is(err, ErrCrewMemberNotFound) {
+		return nil, false, err
+	}
+
+	// Deterministic rkey absent: fall back to the walk in case this member
+	// predates the hash-rkey scheme.
+	crew, err := p.ListCrewMembers(ctx)
+	if err != nil {
+		return nil, false, err
+	}
+	for _, member := range crew {
+		if member.Record.Member == memberDID {
+			return member.Record, true, nil
+		}
+	}
+
+	return nil, false, nil
+}
+
 // CrewMemberWithKey pairs a crew record with its rkey and CID
 type CrewMemberWithKey struct {
 	Rkey   string
@@ -2,7 +2,10 @@ package pds

 import (
 	"bytes"
+	"context"
 	"errors"
+	"os"
+	"path/filepath"
 	"strings"
 	"testing"

@@ -592,3 +595,119 @@ func TestListCrewMembers_MixedDIDs(t *testing.T) {
 		}
 	}
 }
+
+// TestLookupCrewMember_DeterministicRkey verifies the O(1) fast path finds a
+// crew member written by AddCrewMember.
+func TestLookupCrewMember_DeterministicRkey(t *testing.T) {
+	pds, ctx := setupTestPDS(t)
+	defer pds.Close()
+
+	memberDID := "did:plc:fastpath"
+	if _, err := pds.AddCrewMember(ctx, memberDID, "writer", []string{"blob:write"}, "bosun"); err != nil {
+		t.Fatalf("AddCrewMember failed: %v", err)
+	}
+
+	record, isCrew, err := pds.lookupCrewMember(ctx, memberDID)
+	if err != nil {
+		t.Fatalf("lookupCrewMember returned error: %v", err)
+	}
+	if !isCrew {
+		t.Fatal("Expected member to be found")
+	}
+	if record.Member != memberDID {
+		t.Errorf("Expected member %s, got %s", memberDID, record.Member)
+	}
+	if record.Tier != "bosun" {
+		t.Errorf("Expected tier bosun, got %s", record.Tier)
+	}
+}
+
+// TestLookupCrewMember_LegacyTIDRkey verifies the fallback walk still finds a
+// crew record left at a TID rkey by a pre-migration hold. Without the fallback
+// these members would silently lose authorization.
+func TestLookupCrewMember_LegacyTIDRkey(t *testing.T) {
+	pds, ctx := setupTestPDS(t)
+	defer pds.Close()
+
+	memberDID := "did:plc:legacytid"
+
+	// Write the record the way pre-2026-01-06 holds did: auto-generated TID rkey.
+	legacy := &atproto.CrewRecord{
+		Type:        atproto.CrewCollection,
+		Member:      memberDID,
+		Role:        "writer",
+		Permissions: []string{"blob:write"},
+		Tier:        "deckhand",
+	}
+	rkey, _, err := pds.repomgr.CreateRecord(ctx, pds.uid, atproto.CrewCollection, legacy)
+	if err != nil {
+		t.Fatalf("CreateRecord failed: %v", err)
+	}
+	if rkey == atproto.CrewRecordKey(memberDID) {
+		t.Fatalf("Test setup is wrong: CreateRecord produced the deterministic rkey %s", rkey)
+	}
+
+	// The O(1) read must miss, proving the fallback is what found the record.
+	if _, _, err := pds.GetCrewMemberByDID(ctx, memberDID); !errors.Is(err, ErrCrewMemberNotFound) {
+		t.Fatalf("Expected ErrCrewMemberNotFound from deterministic lookup, got %v", err)
+	}
+
+	record, isCrew, err := pds.lookupCrewMember(ctx, memberDID)
+	if err != nil {
+		t.Fatalf("lookupCrewMember returned error: %v", err)
+	}
+	if !isCrew {
+		t.Fatal("Expected legacy TID-keyed member to be found via fallback walk")
+	}
+	if record.Tier != "deckhand" {
+		t.Errorf("Expected tier deckhand, got %s", record.Tier)
+	}
+}
+
+// TestLookupCrewMember_NotFound verifies a genuine non-member reports
+// (nil, false, nil) rather than an error, so callers deny with an AuthError.
+func TestLookupCrewMember_NotFound(t *testing.T) {
+	pds, ctx := setupTestPDS(t)
+	defer pds.Close()
+
+	if _, err := pds.AddCrewMember(ctx, "did:plc:someone", "writer", []string{"blob:write"}, ""); err != nil {
+		t.Fatalf("AddCrewMember failed: %v", err)
+	}
+
+	record, isCrew, err := pds.lookupCrewMember(ctx, "did:plc:stranger")
+	if err != nil {
+		t.Fatalf("Expected nil error for a non-member, got %v", err)
+	}
+	if isCrew {
+		t.Error("Expected non-member to report isCrew=false")
+	}
+	if record != nil {
+		t.Errorf("Expected nil record for a non-member, got %+v", record)
+	}
+}
+
+// TestLookupCrewMember_StorageError verifies a real storage failure surfaces as
+// an error instead of being mistaken for "not a crew member", which would turn
+// a broken CAR store into a silent authorization denial.
+func TestLookupCrewMember_StorageError(t *testing.T) {
+	ctx := context.Background()
+	keyPath := filepath.Join(t.TempDir(), "signing-key")
+	if err := os.WriteFile(keyPath, sharedTestKey, 0600); err != nil {
+		t.Fatalf("Failed to write signing key: %v", err)
+	}
+
+	// Deliberately skip InitNewActor, so the repo has no head to read.
+	pds, err := NewHoldPDS(ctx, "did:web:hold.example.com", "https://hold.example.com", "https://atcr.io", ":memory:", keyPath, false)
+	if err != nil {
+		t.Fatalf("Failed to create test PDS: %v", err)
+	}
+	defer pds.Close()
+
+	record, isCrew, err := pds.lookupCrewMember(ctx, "did:plc:anyone")
+	if err == nil {
+		t.Fatalf("Expected a storage error, got (%+v, %v, nil)", record, isCrew)
+	}
+	if isCrew {
+		t.Error("Expected isCrew=false alongside a storage error")
+	}
+}
@@ -216,18 +216,12 @@ func (p *HoldPDS) GetQuotaForUserWithTier(ctx context.Context, userDID string, q

 // getCrewTier returns the tier for a crew member, or empty string if not found
 func (p *HoldPDS) getCrewTier(ctx context.Context, userDID string) string {
-	crewMembers, err := p.ListCrewMembers(ctx)
-	if err != nil {
+	member, isCrew, err := p.lookupCrewMember(ctx, userDID)
+	if err != nil || !isCrew {
 		return ""
 	}

-	for _, member := range crewMembers {
-		if member.Record.Member == userDID {
-			return member.Record.Tier
-		}
-	}
-
-	return ""
+	return member.Tier
 }

 // ListLayerRecordsForUser returns all layer records uploaded by a specific user