From 2f27f226507db449ba47b42f69bdefc7daaf8749 Mon Sep 17 00:00:00 2001 From: Evan Jarrett Date: Sat, 25 Oct 2025 01:13:57 -0500 Subject: [PATCH] fix ListCrewMembers --- pkg/appview/storage/crew.go | 8 +- pkg/hold/pds/crew.go | 6 +- pkg/hold/pds/xrpc_test.go | 195 ++++++++++++++++++++++++++++++++++++ 3 files changed, 206 insertions(+), 3 deletions(-) diff --git a/pkg/appview/storage/crew.go b/pkg/appview/storage/crew.go index 72a3557..723a8c4 100644 --- a/pkg/appview/storage/crew.go +++ b/pkg/appview/storage/crew.go @@ -3,6 +3,7 @@ package storage import ( "context" "fmt" + "io" "log/slog" "net/http" @@ -75,7 +76,12 @@ func requestCrewMembership(ctx context.Context, holdEndpoint, serviceToken strin defer resp.Body.Close() if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated { - return fmt.Errorf("requestCrew failed with status %d", resp.StatusCode) + // Read response body to capture actual error message from hold + body, readErr := io.ReadAll(resp.Body) + if readErr != nil { + return fmt.Errorf("requestCrew failed with status %d (failed to read error body: %w)", resp.StatusCode, readErr) + } + return fmt.Errorf("requestCrew failed with status %d: %s", resp.StatusCode, string(body)) } return nil diff --git a/pkg/hold/pds/crew.go b/pkg/hold/pds/crew.go index d7da520..118c1fe 100644 --- a/pkg/hold/pds/crew.go +++ b/pkg/hold/pds/crew.go @@ -3,6 +3,7 @@ package pds import ( "bytes" "context" + "errors" "fmt" "strings" "time" @@ -122,9 +123,10 @@ func (p *HoldPDS) ListCrewMembers(ctx context.Context) ([]*CrewMemberWithKey, er if err != nil { // ErrDoneIterating is expected when we stop walking early - if err == repo.ErrDoneIterating { + // Use errors.Is to handle wrapped errors (indigo wraps with %w in MST walk) + if errors.Is(err, repo.ErrDoneIterating) { // Successfully stopped at collection boundary - } else if err.Error() == "mst: not found" || strings.Contains(err.Error(), "not found") { + } else if strings.Contains(err.Error(), "not found") { // If the collection doesn't exist yet (empty repo or no records created), // return empty list instead of error return []*CrewMemberWithKey{}, nil diff --git a/pkg/hold/pds/xrpc_test.go b/pkg/hold/pds/xrpc_test.go index 1e728db..837b7ae 100644 --- a/pkg/hold/pds/xrpc_test.go +++ b/pkg/hold/pds/xrpc_test.go @@ -1289,6 +1289,201 @@ func TestHandleRequestCrew_MethodNotAllowed(t *testing.T) { t.Skip("Method validation is now handled by chi router, not individual handlers") } +// TestHandleRequestCrew_WithAuth tests successful crew membership request with authenticated user +// This test exercises the complete flow including the ListCrewMembers path with empty crew list, +// which previously caused a 500 error due to improper handling of wrapped ErrDoneIterating +func TestHandleRequestCrew_WithAuth(t *testing.T) { + handler, ctx := setupTestXRPCHandler(t) + + // Update captain record to allow all crew + _, err := handler.pds.UpdateCaptainRecord(ctx, true, true, false) // public=true, allowAllCrew=true + if err != nil { + t.Fatalf("Failed to update captain record: %v", err) + } + + // Remove the bootstrap-created owner crew member to get an empty crew list + // (Bootstrap automatically adds the owner as a crew admin) + crewBefore, err := handler.pds.ListCrewMembers(ctx) + if err != nil { + t.Fatalf("Failed to list crew members before test: %v", err) + } + for _, member := range crewBefore { + if err := handler.pds.RemoveCrewMember(ctx, member.Rkey); err != nil { + t.Fatalf("Failed to remove bootstrap crew member: %v", err) + } + } + + // Verify crew list is now empty (this is the critical condition that triggers the bug) + crewAfterCleanup, err := handler.pds.ListCrewMembers(ctx) + if err != nil { + t.Fatalf("Failed to list crew members after cleanup: %v", err) + } + if len(crewAfterCleanup) != 0 { + t.Fatalf("Expected empty crew list after cleanup, got %d members", len(crewAfterCleanup)) + } + + // Create authenticated user (injected into context to bypass auth middleware) + testUserDID := "did:plc:newuser123" + user := &ValidatedUser{ + DID: testUserDID, + Handle: "newuser.test", + PDS: "https://pds.test", + Authorized: true, + } + + // Create request with user in context + body := map[string]any{ + "role": "member", + "permissions": []string{"blob:read", "blob:write"}, + } + bodyBytes, _ := json.Marshal(body) + req := httptest.NewRequest(http.MethodPost, atproto.HoldRequestCrew, bytes.NewReader(bodyBytes)) + req.Header.Set("Content-Type", "application/json") + + // Inject user into request context + reqCtx := context.WithValue(req.Context(), contextKeyUser, user) + req = req.WithContext(reqCtx) + + w := httptest.NewRecorder() + + // Call handler - this should NOT return 500 even with empty crew list + handler.HandleRequestCrew(w, req) + + // Verify successful response + if w.Code != http.StatusCreated { + t.Errorf("Expected status 201 Created, got %d", w.Code) + t.Logf("Response body: %s", w.Body.String()) + } + + // Verify response structure + var response map[string]any + if err := json.Unmarshal(w.Body.Bytes(), &response); err != nil { + t.Fatalf("Failed to parse response JSON: %v", err) + } + + // Check response fields + if cid, ok := response["cid"].(string); !ok || cid == "" { + t.Error("Expected cid string in response") + } + + if status, ok := response["status"].(string); !ok || status != "created" { + t.Errorf("Expected status='created', got %v", response["status"]) + } + + // Verify the crew member was actually added + crewAfter, err := handler.pds.ListCrewMembers(ctx) + if err != nil { + t.Fatalf("Failed to list crew members after request: %v", err) + } + + if len(crewAfter) != 1 { + t.Fatalf("Expected 1 crew member after request, got %d", len(crewAfter)) + } + + // Verify it's the correct user + if crewAfter[0].Record.Member != testUserDID { + t.Errorf("Expected crew member DID %s, got %s", testUserDID, crewAfter[0].Record.Member) + } +} + +// TestHandleRequestCrew_AlreadyMember tests requesting crew membership when already a member +// Should return success without creating duplicate records +func TestHandleRequestCrew_AlreadyMember(t *testing.T) { + handler, ctx := setupTestXRPCHandler(t) + + // Update captain record to allow all crew + _, err := handler.pds.UpdateCaptainRecord(ctx, true, true, false) + if err != nil { + t.Fatalf("Failed to update captain record: %v", err) + } + + // Pre-add the user as a crew member + testUserDID := "did:plc:existinguser123" + _, err = handler.pds.AddCrewMember(ctx, testUserDID, "member", []string{"blob:read", "blob:write"}) + if err != nil { + t.Fatalf("Failed to pre-add crew member: %v", err) + } + + // Verify crew list has our test user (+ the bootstrap owner) + crewBefore, err := handler.pds.ListCrewMembers(ctx) + if err != nil { + t.Fatalf("Failed to list crew members: %v", err) + } + + // Find our test user in the crew list + var foundTestUser bool + var testUserCountBefore int + for _, member := range crewBefore { + if member.Record.Member == testUserDID { + foundTestUser = true + testUserCountBefore++ + } + } + if !foundTestUser { + t.Fatalf("Expected to find test user %s in crew list", testUserDID) + } + if testUserCountBefore != 1 { + t.Fatalf("Expected test user to appear once in crew list, got %d times", testUserCountBefore) + } + + // Create authenticated user (same DID as pre-added member) + user := &ValidatedUser{ + DID: testUserDID, + Handle: "existinguser.test", + PDS: "https://pds.test", + Authorized: true, + } + + // Create request + req := httptest.NewRequest(http.MethodPost, atproto.HoldRequestCrew, nil) + reqCtx := context.WithValue(req.Context(), contextKeyUser, user) + req = req.WithContext(reqCtx) + + w := httptest.NewRecorder() + + // Call handler - should return success with "already_member" status + handler.HandleRequestCrew(w, req) + + // Verify successful response (200 OK for already member) + if w.Code != http.StatusOK { + t.Errorf("Expected status 200 OK, got %d", w.Code) + t.Logf("Response body: %s", w.Body.String()) + } + + // Verify response indicates already a member + var response map[string]any + if err := json.Unmarshal(w.Body.Bytes(), &response); err != nil { + t.Fatalf("Failed to parse response JSON: %v", err) + } + + if status, ok := response["status"].(string); !ok || status != "already_member" { + t.Errorf("Expected status='already_member', got %v", response["status"]) + } + + // Verify no duplicate was created + crewAfter, err := handler.pds.ListCrewMembers(ctx) + if err != nil { + t.Fatalf("Failed to list crew members after request: %v", err) + } + + // Count how many times our test user appears + var testUserCountAfter int + for _, member := range crewAfter { + if member.Record.Member == testUserDID { + testUserCountAfter++ + } + } + + if testUserCountAfter != 1 { + t.Errorf("Expected test user to appear exactly once (no duplicate), got %d times", testUserCountAfter) + } + + // Verify crew list size didn't change (no duplicates added) + if len(crewAfter) != len(crewBefore) { + t.Errorf("Expected crew list size to stay the same (%d), got %d", len(crewBefore), len(crewAfter)) + } +} + // Tests for DID document endpoints // TestHandleDIDDocument tests /.well-known/did.json endpoint