diff --git a/api/confidentiality_idor_test.go b/api/confidentiality_idor_test.go index 59056bacf..23175d424 100644 --- a/api/confidentiality_idor_test.go +++ b/api/confidentiality_idor_test.go @@ -204,7 +204,7 @@ func TestSurveyResponseMetadata_ACLGated(t *testing.T) { t.Run("triage metadata GET denies external as 404", func(t *testing.T) { c, w := CreateTestGinContext("GET", fmt.Sprintf("/triage/survey_responses/%s/metadata", responseID)) - TestUsers.External.SetContext(c) + asTriageUser(t, c) // not a reviewer server.GetTriageSurveyResponseMetadata(c, responseID) @@ -212,9 +212,9 @@ func TestSurveyResponseMetadata_ACLGated(t *testing.T) { }) } -func TestListTriageSurveyResponses_FiltersByACL(t *testing.T) { - // Previously this endpoint had a TODO for the ACL filter and returned - // every survey response in the system regardless of caller identity. +func TestListTriageSurveyResponses_NonReviewerSeesNothing(t *testing.T) { + // A caller in neither reviewer group must see an empty list with total=0 + // even when the store holds rows they own (#357, #1100). gin.SetMode(gin.TestMode) @@ -222,31 +222,19 @@ func TestListTriageSurveyResponses_FiltersByACL(t *testing.T) { saveSurveyStores(t, nil, respStore) surveyID := uuid.New() - - // Three responses: owner-A, owner-B, owner-A confidential. idA := seedSurveyResponse(respStore, surveyID, ResponseStatusReadyForReview, TestUsers.Owner.InternalUUID) - idB := seedSurveyResponse(respStore, surveyID, ResponseStatusReadyForReview, TestUsers.Writer.InternalUUID) idC := seedSurveyResponse(respStore, surveyID, ResponseStatusReadyForReview, TestUsers.Owner.InternalUUID) conf := true respStore.responses[idC].IsConfidential = &conf + respStore.listItems = []SurveyResponseListItem{{Id: &idA}, {Id: &idC}} + respStore.listTotal = 2 - // Wire list-items so the mock returns all three from List(...) - respStore.listItems = []SurveyResponseListItem{ - {Id: &idA}, - {Id: &idB}, - {Id: &idC}, - } - respStore.listTotal = 3 - - // External user has no access to any response — must see empty list, - // total=0, even though the underlying store returns 3 rows. server := &Server{} c, w := CreateTestGinContext("GET", "/triage/survey_responses") - TestUsers.External.SetContext(c) + asTriageUser(t, c) server.ListTriageSurveyResponses(c, ListTriageSurveyResponsesParams{}) assert.Equal(t, http.StatusOK, w.Code) - body := w.Body.String() - assert.Contains(t, body, `"total":0`, "external user must see total=0 — list count must NOT leak existence of confidential or other-user responses") + assert.Contains(t, w.Body.String(), `"total":0`) } diff --git a/api/create_threat_model_from_survey_test.go b/api/create_threat_model_from_survey_test.go index debd1cd4b..ed3336cac 100644 --- a/api/create_threat_model_from_survey_test.go +++ b/api/create_threat_model_from_survey_test.go @@ -126,9 +126,10 @@ func TestCreateThreatModelFromSurveyResponse_WrongStatus(t *testing.T) { mockResponseStore := &mockSurveyResponseStore{ responses: map[uuid.UUID]*SurveyResponse{ responseID: { - Id: &responseID, - Status: &draftStatus, - Owner: owner, + Id: &responseID, + Status: &draftStatus, + Owner: owner, + IsConfidential: new(bool), }, }, accessMap: map[string]AuthorizationRole{ @@ -142,9 +143,7 @@ func TestCreateThreatModelFromSurveyResponse_WrongStatus(t *testing.T) { w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) c.Request = httptest.NewRequest("POST", "/triage/survey_responses/"+responseID.String()+"/create_threat_model", nil) - c.Set("userInternalUUID", "user-123") - c.Set("userEmail", "alice@example.com") - c.Set("userID", "alice-provider-id") + asTriageUser(t, c, GroupSecurityReviewers) server.CreateThreatModelFromSurveyResponse(c, responseID) @@ -162,7 +161,7 @@ func TestCreateThreatModelFromSurveyResponse_DuplicateTM(t *testing.T) { mockResponseStore := &mockSurveyResponseStore{ responses: map[uuid.UUID]*SurveyResponse{ - responseID: {Id: &responseID, Status: &readyStatus, Owner: owner, CreatedThreatModelId: &existingTMID}, + responseID: {Id: &responseID, Status: &readyStatus, Owner: owner, IsConfidential: new(bool), CreatedThreatModelId: &existingTMID}, }, accessMap: map[string]AuthorizationRole{responseID.String() + ":user-123": AuthorizationRoleOwner}, } @@ -173,9 +172,7 @@ func TestCreateThreatModelFromSurveyResponse_DuplicateTM(t *testing.T) { w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) c.Request = httptest.NewRequest("POST", "/triage/survey_responses/"+responseID.String()+"/create_threat_model", nil) - c.Set("userInternalUUID", "user-123") - c.Set("userEmail", "alice@example.com") - c.Set("userID", "alice") + asTriageUser(t, c, GroupSecurityReviewers) server.CreateThreatModelFromSurveyResponse(c, responseID) assert.Equal(t, http.StatusConflict, w.Code) @@ -190,7 +187,7 @@ func TestCreateThreatModelFromSurveyResponse_NilOwner(t *testing.T) { mockResponseStore := &mockSurveyResponseStore{ responses: map[uuid.UUID]*SurveyResponse{ - responseID: {Id: &responseID, Status: &readyStatus, Owner: nil}, + responseID: {Id: &responseID, Status: &readyStatus, Owner: nil, IsConfidential: new(bool)}, }, accessMap: map[string]AuthorizationRole{responseID.String() + ":user-123": AuthorizationRoleOwner}, } @@ -201,9 +198,7 @@ func TestCreateThreatModelFromSurveyResponse_NilOwner(t *testing.T) { w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) c.Request = httptest.NewRequest("POST", "/triage/survey_responses/"+responseID.String()+"/create_threat_model", nil) - c.Set("userInternalUUID", "user-123") - c.Set("userEmail", "alice@example.com") - c.Set("userID", "alice") + asTriageUser(t, c, GroupSecurityReviewers) server.CreateThreatModelFromSurveyResponse(c, responseID) assert.Equal(t, http.StatusInternalServerError, w.Code) @@ -230,9 +225,7 @@ func TestCreateThreatModelFromSurveyResponse_AccessDenied(t *testing.T) { w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) c.Request = httptest.NewRequest("POST", "/triage/survey_responses/"+responseID.String()+"/create_threat_model", nil) - c.Set("userInternalUUID", "user-123") - c.Set("userEmail", "alice@example.com") - c.Set("userID", "alice") + asTriageUser(t, c) server.CreateThreatModelFromSurveyResponse(c, responseID) // T5 (#357): access denied collapses to 404 to avoid existence disclosure. diff --git a/api/server_survey.go b/api/server_survey.go index b7f1d2e3b..7d9667273 100644 --- a/api/server_survey.go +++ b/api/server_survey.go @@ -141,18 +141,18 @@ func (s *Server) UpdateIntakeSurveyResponseMetadataByKey(c *gin.Context, surveyR // Survey Response Metadata Methods - Triage (read-only) // GetTriageSurveyResponseMetadata gets triage survey response metadata -// SEM@368e91d91cb110162c64b6ea10d49562a59bf3f0: authorize reader access then list all triage survey response metadata entries (reads DB) +// SEM@49259ca96d8bf22bf9c7a384c528d9a39bebe16d: authorize triage reviewer access then list triage survey response metadata entries (reads DB) func (s *Server) GetTriageSurveyResponseMetadata(c *gin.Context, surveyResponseId SurveyResponseId) { - if _, ok := RequireSurveyResponseAccess(c, surveyResponseId, AuthorizationRoleReader); !ok { + if _, ok := RequireTriageAccess(c, surveyResponseId); !ok { return } s.surveyResponseMetadata.List(c) } // GetTriageSurveyResponseMetadataByKey gets triage survey response metadata by key -// SEM@368e91d91cb110162c64b6ea10d49562a59bf3f0: authorize reader access then fetch triage survey response metadata by key (reads DB) +// SEM@49259ca96d8bf22bf9c7a384c528d9a39bebe16d: authorize triage reviewer access then fetch triage survey response metadata by key (reads DB) func (s *Server) GetTriageSurveyResponseMetadataByKey(c *gin.Context, surveyResponseId SurveyResponseId, key MetadataKey) { - if _, ok := RequireSurveyResponseAccess(c, surveyResponseId, AuthorizationRoleReader); !ok { + if _, ok := RequireTriageAccess(c, surveyResponseId); !ok { return } s.surveyResponseMetadata.GetByKey(c) @@ -161,22 +161,31 @@ func (s *Server) GetTriageSurveyResponseMetadataByKey(c *gin.Context, surveyResp // Survey Response Triage Notes Methods - Triage (create + read) // ListTriageSurveyResponseTriageNotes lists triage notes for a survey response -// SEM@28792aa3991e394010e49c040d3db2d5f14a6eff: list triage notes for a survey response via the triage stage (reads DB) +// SEM@49259ca96d8bf22bf9c7a384c528d9a39bebe16d: authorize triage reviewer access then list triage notes for a survey response (reads DB) func (s *Server) ListTriageSurveyResponseTriageNotes(c *gin.Context, surveyResponseId SurveyResponseId, params ListTriageSurveyResponseTriageNotesParams) { + if _, ok := RequireTriageAccess(c, surveyResponseId); !ok { + return + } c.Params = append(c.Params, gin.Param{Key: "survey_response_id", Value: surveyResponseId.String()}) s.triageNoteHandler.ListTriageNotes(c) } // CreateTriageSurveyResponseTriageNote creates a triage note -// SEM@28792aa3991e394010e49c040d3db2d5f14a6eff: store a new triage note for a survey response via the triage stage (mutates shared state) +// SEM@49259ca96d8bf22bf9c7a384c528d9a39bebe16d: authorize triage reviewer access then store a new triage note (mutates shared state) func (s *Server) CreateTriageSurveyResponseTriageNote(c *gin.Context, surveyResponseId SurveyResponseId) { + if _, ok := RequireTriageAccess(c, surveyResponseId); !ok { + return + } c.Params = append(c.Params, gin.Param{Key: "survey_response_id", Value: surveyResponseId.String()}) s.triageNoteHandler.CreateTriageNote(c) } // GetTriageSurveyResponseTriageNote gets a specific triage note -// SEM@28792aa3991e394010e49c040d3db2d5f14a6eff: fetch a single triage note for a survey response via the triage stage (reads DB) +// SEM@49259ca96d8bf22bf9c7a384c528d9a39bebe16d: authorize triage reviewer access then fetch a single triage note (reads DB) func (s *Server) GetTriageSurveyResponseTriageNote(c *gin.Context, surveyResponseId SurveyResponseId, triageNoteId TriageNoteId) { + if _, ok := RequireTriageAccess(c, surveyResponseId); !ok { + return + } c.Params = append(c.Params, gin.Param{Key: "survey_response_id", Value: surveyResponseId.String()}) c.Params = append(c.Params, gin.Param{Key: "triage_note_id", Value: strconv.Itoa(triageNoteId)}) s.triageNoteHandler.GetTriageNote(c) diff --git a/api/survey_handlers.go b/api/survey_handlers.go index 1b3cd42af..198edce59 100644 --- a/api/survey_handlers.go +++ b/api/survey_handlers.go @@ -110,7 +110,7 @@ func (s *Server) ListAdminSurveys(c *gin.Context, params ListAdminSurveysParams) // CreateAdminSurvey creates a new survey. // POST /admin/surveys -// SEM@ed719f753d3a02b551f5a7a01daff2f01a221118: store a new survey template and emit a webhook event (mutates shared state) +// SEM@db94db3c8118cffb5220bf9a44c2fde0ada20427: store a new survey template and emit a webhook event (mutates shared state) func (s *Server) CreateAdminSurvey(c *gin.Context) { logger := slogging.Get() ctx := c.Request.Context() @@ -227,7 +227,7 @@ func (s *Server) GetAdminSurvey(c *gin.Context, surveyId SurveyId) { // UpdateAdminSurvey fully updates a survey. // PUT /admin/surveys/{survey_id} -// SEM@ed719f753d3a02b551f5a7a01daff2f01a221118: fully replace a non-archived survey and emit a webhook event (mutates shared state) +// SEM@db94db3c8118cffb5220bf9a44c2fde0ada20427: fully replace a non-archived survey and emit a webhook event (mutates shared state) func (s *Server) UpdateAdminSurvey(c *gin.Context, surveyId SurveyId) { logger := slogging.Get() ctx := c.Request.Context() @@ -856,10 +856,23 @@ func (s *Server) UpdateIntakeSurveyResponse(c *gin.Context, surveyResponseId Sur c.JSON(http.StatusOK, updated) } +// validateIntakeStatusTarget rejects any status an intake caller may not set. +// The submitter may only move a response to draft or submitted; approval and +// review states belong to triage reviewers (#1100). PUT and create carry no +// status field, so PATCH is the only intake path that reaches this check. +// SEM@fea6b3be63afa09c0668f3eaa2f51f933c5e05f1: validate that an intake caller may set a survey response status only to draft or submitted (pure) +func validateIntakeStatusTarget(status string) error { + if status != ResponseStatusDraft && status != ResponseStatusSubmitted { + return fmt.Errorf("status %q cannot be set through intake endpoints; only %q or %q is allowed", + status, ResponseStatusDraft, ResponseStatusSubmitted) + } + return nil +} + // PatchIntakeSurveyResponse partially updates a survey response. // PATCH /intake/survey_responses/{response_id} // Supports status transitions: draft->submitted, needs_revision->submitted -// SEM@db94db3c8118cffb5220bf9a44c2fde0ada20427: apply a JSON Patch to a draft or needs-revision survey response and handle status transitions (mutates DB) +// SEM@fea6b3be63afa09c0668f3eaa2f51f933c5e05f1: apply a JSON Patch to a draft or needs-revision survey response, allowing only draft or submitted status targets (mutates DB) func (s *Server) PatchIntakeSurveyResponse(c *gin.Context, surveyResponseId SurveyResponseId, _ PatchIntakeSurveyResponseParams) { logger := slogging.Get() ctx := c.Request.Context() @@ -924,6 +937,10 @@ func (s *Server) PatchIntakeSurveyResponse(c *gin.Context, surveyResponseId Surv // transition never lets the CAS-guarded write commit only to fail on an // unrelated validation afterward. if hasStatusChange && patched.Status != nil && *patched.Status != *existing.Status { + if err := validateIntakeStatusTarget(*patched.Status); err != nil { + HandleRequestError(c, InvalidInputError(err.Error())) + return + } if err := ValidateSurveyResponseStatusTransition(*patched.Status, nil); err != nil { HandleRequestError(c, InvalidInputError(err.Error())) // validation failure, not a conflict (#987) return @@ -1054,13 +1071,12 @@ func (s *Server) DeleteIntakeSurveyResponse(c *gin.Context, surveyResponseId Sur // ListTriageSurveyResponses returns survey responses for triage. // GET /triage/survey_responses -// SEM@db94db3c8118cffb5220bf9a44c2fde0ada20427: list survey responses visible to the triage reviewer, filtered by per-row ACL (reads DB) +// SEM@49259ca96d8bf22bf9c7a384c528d9a39bebe16d: list survey responses scoped to the caller's reviewer groups by confidentiality (reads DB) func (s *Server) ListTriageSurveyResponses(c *gin.Context, params ListTriageSurveyResponsesParams) { logger := slogging.Get() ctx := c.Request.Context() - userInternalUUID, ok := getUserUUID(c) - if !ok { + if _, ok := getUserUUID(c); !ok { return } @@ -1083,54 +1099,45 @@ func (s *Server) ListTriageSurveyResponses(c *gin.Context, params ListTriageSurv } } - // Authorization model (T5, #357): - // The triage queue is intentionally not owner-scoped — security reviewers - // must see responses they did not submit. Per-row ACL (seeded at create) - // already encodes who can see what: - // - non-confidential responses grant Security Reviewers group owner role - // - confidential responses grant Confidential Project Reviewers group owner role - // - the submitter has direct owner role - // We unconditionally fetch the page from the store (no ACL pre-filter) and - // apply HasAccess in-memory to the returned items + recompute total. This - // keeps the list, per-row, and confidentiality decisions consistent and - // closes the previous TODO that left this endpoint enumerating all - // responses regardless of caller identity. - // - // We over-fetch here (full table for the requested filter) and paginate in - // memory; this is an N-roundtrip pattern that mirrors ListWithCounts on - // threat models and is acceptable for the triage queue size today. If the - // queue grows past a few thousand rows this should move to a SQL JOIN - // against survey_response_access + group_members (tracked separately). - const overFetchLimit = 10000 - allItems, _, err := GlobalSurveyResponseStore.List(ctx, overFetchLimit, 0, filters) + // Authorization model (#1100): only reviewer-group members may triage. + // Security Reviewers see non-confidential responses, Confidential Project + // Reviewers see confidential ones, both see all; ownership and the row ACL + // grant nothing here. The confidentiality scope is pushed into the store + // query so pagination and totals stay correct. + confidential, hasScope, err := triageReviewerScope(c) if err != nil { - logger.Error("Failed to list survey responses: %v", err) - c.JSON(http.StatusInternalServerError, Error{ - Error: ErrorError(errcode.ServerError), - ErrorDescription: "Failed to list survey responses", - }) + logger.Error("Failed to resolve triage reviewer scope: %v", err) + HandleRequestError(c, ServerError("Failed to check access")) return } - - visible := FilterSurveyResponseListItemsByAccess(ctx, userInternalUUID, allItems, AuthorizationRoleReader) - total := len(visible) - - if offset >= total { + if !hasScope { c.JSON(http.StatusOK, ListSurveyResponsesResponse{ SurveyResponses: []SurveyResponseListItem{}, - Total: total, + Total: 0, Limit: limit, Offset: offset, }) return } - end := offset + limit - if end > total || limit <= 0 { - end = total + if confidential != nil { + if filters == nil { + filters = &SurveyResponseFilters{} + } + filters.IsConfidential = confidential + } + + items, total, err := GlobalSurveyResponseStore.List(ctx, limit, offset, filters) + if err != nil { + logger.Error("Failed to list survey responses: %v", err) + c.JSON(http.StatusInternalServerError, Error{ + Error: ErrorError(errcode.ServerError), + ErrorDescription: "Failed to list survey responses", + }) + return } c.JSON(http.StatusOK, ListSurveyResponsesResponse{ - SurveyResponses: visible[offset:end], + SurveyResponses: items, Total: total, Limit: limit, Offset: offset, @@ -1139,9 +1146,9 @@ func (s *Server) ListTriageSurveyResponses(c *gin.Context, params ListTriageSurv // GetTriageSurveyResponse returns a specific survey response for triage. // GET /triage/survey_responses/{response_id} -// SEM@368e91d91cb110162c64b6ea10d49562a59bf3f0: fetch an authorized survey response for triage review (reads DB) +// SEM@49259ca96d8bf22bf9c7a384c528d9a39bebe16d: fetch a survey response for a caller in the matching triage reviewer group (reads DB) func (s *Server) GetTriageSurveyResponse(c *gin.Context, surveyResponseId SurveyResponseId) { - response, ok := RequireSurveyResponseAccess(c, surveyResponseId, AuthorizationRoleReader) + response, ok := RequireTriageAccess(c, surveyResponseId) if !ok { return } @@ -1151,12 +1158,12 @@ func (s *Server) GetTriageSurveyResponse(c *gin.Context, surveyResponseId Survey // PatchTriageSurveyResponse partially updates a survey response for triage. // PATCH /triage/survey_responses/{response_id} // Supports status transitions: submitted->ready_for_review, submitted->needs_revision, ready_for_review->needs_revision -// SEM@db94db3c8118cffb5220bf9a44c2fde0ada20427: apply a JSON Patch to update a survey response's triage status (mutates DB) +// SEM@49259ca96d8bf22bf9c7a384c528d9a39bebe16d: apply a JSON Patch to a survey response's triage status for a matching triage reviewer (mutates DB) func (s *Server) PatchTriageSurveyResponse(c *gin.Context, surveyResponseId SurveyResponseId) { logger := slogging.Get() ctx := c.Request.Context() - existing, ok := RequireSurveyResponseAccess(c, surveyResponseId, AuthorizationRoleOwner) + existing, ok := RequireTriageAccess(c, surveyResponseId) if !ok { return } @@ -1498,18 +1505,18 @@ func createThreatModelFromResponse(ctx context.Context, response *SurveyResponse // CreateThreatModelFromSurveyResponse creates a threat model from an approved survey response. // POST /triage/survey_responses/{response_id}/create_threat_model -// SEM@db94db3c8118cffb5220bf9a44c2fde0ada20427: build a threat model from a ready-for-review survey response and notify webhooks (mutates DB) +// SEM@49259ca96d8bf22bf9c7a384c528d9a39bebe16d: build a threat model from a ready-for-review response for a matching triage reviewer and notify webhooks (mutates DB) func (s *Server) CreateThreatModelFromSurveyResponse(c *gin.Context, surveyResponseId SurveyResponseId) { logger := slogging.Get() ctx := c.Request.Context() - // Step 1: Load + access check via centralized helper - response, ok := RequireSurveyResponseAccess(c, surveyResponseId, AuthorizationRoleOwner) + // Step 1: Load + triage reviewer check via centralized helper + response, ok := RequireTriageAccess(c, surveyResponseId) if !ok { return } - // Step 2: Extract user identity (already validated by RequireSurveyResponseAccess) + // Step 2: Extract user identity (already validated by RequireTriageAccess) user, err := GetAuthenticatedUser(c) if err != nil { HandleRequestError(c, err) diff --git a/api/survey_handlers_test.go b/api/survey_handlers_test.go index 3bcaa18e1..8a85f7f19 100644 --- a/api/survey_handlers_test.go +++ b/api/survey_handlers_test.go @@ -250,13 +250,26 @@ func (m *mockSurveyResponseStore) List(_ context.Context, _, _ int, filters *Sur if m.err != nil { return nil, 0, m.err } + if filters != nil && filters.IsConfidential != nil { + out := make([]SurveyResponseListItem, 0, len(m.listItems)) + for _, it := range m.listItems { + isConf := it.IsConfidential != nil && *it.IsConfidential + if isConf == *filters.IsConfidential { + out = append(out, it) + } + } + return out, len(out), nil + } return m.listItems, m.listTotal, nil } -func (m *mockSurveyResponseStore) UpdateStatus(_ context.Context, _ uuid.UUID, _ string, _ *string, _ *string) error { +func (m *mockSurveyResponseStore) UpdateStatus(_ context.Context, id uuid.UUID, newStatus string, _ *string, _ *string) error { if m.updateStatusErr != nil { return m.updateStatusErr } + if r, ok := m.responses[id]; ok { + r.Status = &newStatus + } return nil } @@ -348,6 +361,9 @@ func seedSurveyResponse(store *mockSurveyResponseStore, surveyID uuid.UUID, stat Id: &id, SurveyId: surveyID, Status: strPtr(status), + // Mirrors the column default; a nil value is unknown and fails closed + // as confidential in triage access. + IsConfidential: new(bool), } now := time.Now().UTC() r.CreatedAt = &now @@ -1607,12 +1623,10 @@ func TestListTriageSurveyResponses(t *testing.T) { server := &Server{} t.Run("success", func(t *testing.T) { - // T5 (#357): triage list now ACL-filters in memory; the caller must - // have at least reader role on each row to see it. Seed the listItems - // with a UUID for which the owner has access. + // #1100: the triage list is scoped by reviewer-group membership, not + // the row ACL; a Security Reviewers member sees non-confidential rows. respStore := newMockSurveyResponseStore() visibleID := uuid.New() - respStore.accessMap[respStore.accessKey(visibleID, TestUsers.Owner.InternalUUID)] = AuthorizationRoleOwner respStore.listItems = []SurveyResponseListItem{ {Id: &visibleID, Status: "submitted", SurveyId: uuid.New(), CreatedAt: time.Now().UTC()}, } @@ -1620,7 +1634,7 @@ func TestListTriageSurveyResponses(t *testing.T) { saveSurveyStores(t, nil, respStore) c, w := CreateTestGinContext("GET", "/triage/survey_responses") - TestUsers.Owner.SetContext(c) + asTriageUser(t, c, GroupSecurityReviewers) server.ListTriageSurveyResponses(c, ListTriageSurveyResponsesParams{}) @@ -1647,7 +1661,7 @@ func TestListTriageSurveyResponses(t *testing.T) { saveSurveyStores(t, nil, respStore) c, w := CreateTestGinContext("GET", "/triage/survey_responses") - TestUsers.Owner.SetContext(c) + asTriageUser(t, c, GroupSecurityReviewers) server.ListTriageSurveyResponses(c, ListTriageSurveyResponsesParams{}) @@ -1667,7 +1681,7 @@ func TestGetTriageSurveyResponse(t *testing.T) { responseID := seedSurveyResponse(respStore, surveyID, ResponseStatusSubmitted, TestUsers.Owner.InternalUUID) c, w := CreateTestGinContext("GET", fmt.Sprintf("/triage/survey_responses/%s", responseID)) - TestUsers.Owner.SetContext(c) + asTriageUser(t, c, GroupSecurityReviewers) server.GetTriageSurveyResponse(c, responseID) @@ -1679,7 +1693,7 @@ func TestGetTriageSurveyResponse(t *testing.T) { saveSurveyStores(t, nil, respStore) c, w := CreateTestGinContext("GET", "/triage/survey_responses/"+uuid.New().String()) - TestUsers.Owner.SetContext(c) + asTriageUser(t, c, GroupSecurityReviewers) server.GetTriageSurveyResponse(c, uuid.New()) @@ -1694,7 +1708,7 @@ func TestGetTriageSurveyResponse(t *testing.T) { responseID := seedSurveyResponse(respStore, surveyID, ResponseStatusSubmitted, TestUsers.Owner.InternalUUID) c, w := CreateTestGinContext("GET", fmt.Sprintf("/triage/survey_responses/%s", responseID)) - TestUsers.External.SetContext(c) + asTriageUser(t, c) // not a reviewer server.GetTriageSurveyResponse(c, responseID) diff --git a/api/survey_response_access.go b/api/survey_response_access.go index 91a383ae3..4f3d9a96a 100644 --- a/api/survey_response_access.go +++ b/api/survey_response_access.go @@ -1,16 +1,15 @@ package api import ( - "context" - "github.com/ericfitz/tmi/internal/slogging" "github.com/gin-gonic/gin" ) // RequireSurveyResponseAccess centralizes the existence check + access check // for survey-response sub-resources. It is the single enforcement point for -// the four-question authorization decision on /intake/survey_responses/* and -// /triage/survey_responses/* paths. +// the authorization decision on /intake/survey_responses/* paths. Triage paths +// use RequireTriageAccess (reviewer-group membership) instead; the row ACL +// grants no triage access (#1100). // // On success, it returns the loaded survey response. On any failure // (not found, access denied, internal error) it writes the appropriate error @@ -78,42 +77,3 @@ func RequireSurveyResponseAccess( return response, true } - -// FilterSurveyResponseListItemsByAccess narrows a list of survey-response list -// items to only those the caller can read. Used by triage/intake list endpoints -// where the underlying store query does not enforce per-row ACL (e.g. -// ListTriageSurveyResponses, which intentionally does not filter by owner). -// -// This is an in-memory filter against the same HasAccess(...) the per-row -// handlers use, so list and per-row decisions stay in sync. The returned -// total reflects the filtered count, so pagination metadata never leaks the -// existence of confidential responses the caller cannot read (matching the -// list-leak acceptance criterion in #357). -// SEM@368e91d91cb110162c64b6ea10d49562a59bf3f0: filter a list of survey response items to only those the caller can read, without leaking existence of others (reads DB) -func FilterSurveyResponseListItemsByAccess( - ctx context.Context, - userInternalUUID string, - items []SurveyResponseListItem, - requiredRole AuthorizationRole, -) []SurveyResponseListItem { - if len(items) == 0 || GlobalSurveyResponseStore == nil { - return items - } - out := make([]SurveyResponseListItem, 0, len(items)) - logger := slogging.Get() - for _, item := range items { - if item.Id == nil { - continue - } - hasAccess, err := GlobalSurveyResponseStore.HasAccess(ctx, *item.Id, userInternalUUID, requiredRole) - if err != nil { - logger.Warn("FilterSurveyResponseListItemsByAccess: HasAccess error for response %s user %s: %v — excluding from list", - item.Id.String(), userInternalUUID, err) - continue - } - if hasAccess { - out = append(out, item) - } - } - return out -} diff --git a/api/survey_response_store_gorm.go b/api/survey_response_store_gorm.go index ccc920b65..e5af4a96d 100644 --- a/api/survey_response_store_gorm.go +++ b/api/survey_response_store_gorm.go @@ -61,11 +61,15 @@ type SurveyResponseStore interface { } // SurveyResponseFilters defines filter options for listing responses -// SEM@0bd9c0e0e0c0649294d164b9dc945b801cfd507c: filter parameters for listing survey responses by status, survey, or owner (pure) +// SEM@49259ca96d8bf22bf9c7a384c528d9a39bebe16d: filter parameters for listing survey responses by status, survey, owner, or confidentiality (pure) type SurveyResponseFilters struct { Status *string SurveyID *uuid.UUID OwnerID *string + + // IsConfidential, when set, restricts results to responses whose + // is_confidential flag equals the value (triage reviewer scoping, #1100). + IsConfidential *bool } // GormSurveyResponseStore implements SurveyResponseStore using GORM @@ -508,7 +512,7 @@ func (s *GormSurveyResponseStore) Delete(ctx context.Context, id uuid.UUID) erro } // List retrieves survey responses with pagination and optional filters -// SEM@b11b7d1f947994479701d4db877ed4964b3bfaa6: list paginated survey responses with optional status/survey/owner filters (reads DB) +// SEM@49259ca96d8bf22bf9c7a384c528d9a39bebe16d: list paginated survey responses with optional status/survey/owner/confidentiality filters (reads DB) func (s *GormSurveyResponseStore) List(ctx context.Context, limit, offset int, filters *SurveyResponseFilters) ([]SurveyResponseListItem, int, error) { logger := slogging.Get() @@ -531,6 +535,11 @@ func (s *GormSurveyResponseStore) List(ctx context.Context, limit, offset int, f if filters.OwnerID != nil { query = query.Where("owner_internal_uuid = ?", *filters.OwnerID) } + if filters.IsConfidential != nil { + query = query.Where(ColumnMap(query.Name(), map[string]any{ + "is_confidential": models.DBBool(*filters.IsConfidential), + })) + } } // Get total count diff --git a/api/survey_status_validation_test.go b/api/survey_status_validation_test.go index a3e2d979f..9dbc55a41 100644 --- a/api/survey_status_validation_test.go +++ b/api/survey_status_validation_test.go @@ -15,7 +15,7 @@ import ( func TestPatchSurveyResponse_StatusValidationReturns400(t *testing.T) { patch := []byte(`[{"op":"replace","path":"/status","value":"needs_revision"}]`) - t.Run("intake: needs_revision without revision_notes", func(t *testing.T) { + t.Run("intake: needs_revision is not an intake target (#1100)", func(t *testing.T) { store := newMockSurveyResponseStore() saveSurveyStores(t, nil, store) id := seedSurveyResponse(store, uuid.New(), ResponseStatusDraft, TestUsers.Owner.InternalUUID) @@ -25,7 +25,7 @@ func TestPatchSurveyResponse_StatusValidationReturns400(t *testing.T) { (&Server{}).PatchIntakeSurveyResponse(c, id, PatchIntakeSurveyResponseParams{}) assert.Equal(t, http.StatusBadRequest, w.Code, w.Body.String()) - assert.Contains(t, w.Body.String(), "revision_notes required") + assert.Contains(t, w.Body.String(), "cannot be set through intake") }) t.Run("triage: store reports missing revision_notes", func(t *testing.T) { @@ -35,7 +35,7 @@ func TestPatchSurveyResponse_StatusValidationReturns400(t *testing.T) { id := seedSurveyResponse(store, uuid.New(), ResponseStatusSubmitted, TestUsers.Owner.InternalUUID) c, w := CreateTestGinContextWithBody("PATCH", fmt.Sprintf("/triage/survey_responses/%s", id), "application/json-patch+json", patch) - TestUsers.Owner.SetContext(c) + asTriageUser(t, c, GroupSecurityReviewers) (&Server{}).PatchTriageSurveyResponse(c, id) assert.Equal(t, http.StatusBadRequest, w.Code, w.Body.String()) diff --git a/api/survey_triage_access.go b/api/survey_triage_access.go new file mode 100644 index 000000000..c49b34ad0 --- /dev/null +++ b/api/survey_triage_access.go @@ -0,0 +1,104 @@ +package api + +import ( + "github.com/ericfitz/tmi/internal/slogging" + "github.com/gin-gonic/gin" +) + +// Triage access model (#1100, Human decision Eric 2026-10-17): +// triage endpoints are open only to members of Security Reviewers (for +// non-confidential responses) or Confidential Project Reviewers (for +// confidential responses), regardless of response ownership or the row ACL. +// Membership is resolved per request via IsGroupMemberFromContext, so +// IdP-mapped and nested group membership counts. There is no admin bypass. + +// triageAccessVerifiedKey marks a Gin context whose triage route has already +// passed RequireTriageAccess, so shared sub-resource handlers skip the +// intake-oriented ACL check. +const triageAccessVerifiedKey = "triageAccessVerified" + +// triageReviewerGroup returns the reviewer group whose members may triage a +// survey response with the given confidentiality. +// SEM@49259ca96d8bf22bf9c7a384c528d9a39bebe16d: select the reviewer group authorized to triage a response by confidentiality (pure) +func triageReviewerGroup(isConfidential bool) BuiltInGroup { + if isConfidential { + return GroupConfidentialProjectReviewers + } + return GroupSecurityReviewers +} + +// RequireTriageAccess loads a survey response and authorizes the caller to +// triage it: the caller must belong to the reviewer group matching the +// response's confidentiality. A missing response and a denied caller both +// surface as 404 (non-disclosure, T5 #357). On failure it writes the error +// and returns (nil, false). +// SEM@c200ebeeb27cdc988a6ca70a79561a67add75afb: authorize a caller by reviewer-group membership for a survey response's confidentiality, 404 on denial (reads DB) +func RequireTriageAccess(c *gin.Context, responseID SurveyResponseId) (*SurveyResponse, bool) { + logger := slogging.Get().WithContext(c) + + if _, ok := getUserUUID(c); !ok { + return nil, false + } + + response, err := GlobalSurveyResponseStore.Get(c.Request.Context(), responseID) + if err != nil { + logger.Error("RequireTriageAccess: failed to get survey response %s: %v", responseID.String(), err) + HandleRequestError(c, ServerError("Failed to get survey response")) + return nil, false + } + if response == nil { + HandleRequestError(c, NotFoundError("Survey response not found")) + return nil, false + } + + // A NULL confidentiality is unknown, so it fails closed as confidential, + // matching the list filter, which excludes NULL rows from the plain scope. + isConfidential := response.IsConfidential == nil || *response.IsConfidential + isReviewer, err := IsGroupMemberFromContext(c, triageReviewerGroup(isConfidential)) + if err != nil { + logger.Error("RequireTriageAccess: membership check failed for response %s: %v", responseID.String(), err) + HandleRequestError(c, ServerError("Failed to check access")) + return nil, false + } + if !isReviewer { + logger.Debug("RequireTriageAccess: caller is not a triage reviewer for response %s (confidential=%t) - returning 404", + responseID.String(), isConfidential) + HandleRequestError(c, NotFoundError("Survey response not found")) + return nil, false + } + + c.Set(triageAccessVerifiedKey, true) + return response, true +} + +// triageReviewerScope resolves which response confidentiality values the +// caller may triage. hasScope is false when the caller belongs to neither reviewer +// group; otherwise confidential, when non-nil, is the single permitted value +// and nil means both. +// SEM@fea6b3be63afa09c0668f3eaa2f51f933c5e05f1: resolve the confidentiality scope a caller may triage from reviewer group membership (reads DB) +func triageReviewerScope(c *gin.Context) (confidential *bool, hasScope bool, err error) { + mc, err := ResolveMembershipContext(c) + if err != nil { + return nil, false, err + } + isSR, err := IsGroupMember(c.Request.Context(), mc, GroupSecurityReviewers) + if err != nil { + return nil, false, err + } + isCPR, err := IsGroupMember(c.Request.Context(), mc, GroupConfidentialProjectReviewers) + if err != nil { + return nil, false, err + } + switch { + case isSR && isCPR: + return nil, true, nil + case isSR: + v := false + return &v, true, nil + case isCPR: + v := true + return &v, true, nil + default: + return nil, false, nil + } +} diff --git a/api/survey_triage_access_test.go b/api/survey_triage_access_test.go new file mode 100644 index 000000000..5bc4c723d --- /dev/null +++ b/api/survey_triage_access_test.go @@ -0,0 +1,360 @@ +package api + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "testing" + + "github.com/gin-gonic/gin" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Tests for #1100: triage endpoints are open only to Security Reviewers +// (non-confidential responses) and Confidential Project Reviewers +// (confidential responses). Ownership and the row ACL grant no triage access. + +// triageMembershipRepo is a GroupMemberRepository that answers +// IsEffectiveMember from a per-user set of built-in group UUIDs. +type triageMembershipRepo struct { + GroupMemberRepository + members map[uuid.UUID]map[uuid.UUID]bool // user -> group -> member + err error +} + +func (r *triageMembershipRepo) IsEffectiveMember(_ context.Context, groupUUID, userUUID uuid.UUID, _ []uuid.UUID) (bool, error) { + if r.err != nil { + return false, r.err + } + return r.members[userUUID][groupUUID], nil +} + +func triageTestUser(name string) TestUserIdentity { + return TestUserIdentity{ + Email: name + "@tmi.local", + ProviderID: name + "-provider-id", + InternalUUID: uuid.New().String(), + IdP: "tmi", + Groups: []string{}, + } +} + +func (r *triageMembershipRepo) grant(u TestUserIdentity, groups ...BuiltInGroup) { + uid := uuid.MustParse(u.InternalUUID) + if r.members[uid] == nil { + r.members[uid] = map[uuid.UUID]bool{} + } + for _, g := range groups { + r.members[uid][g.UUID] = true + } +} + +// installTriageMembership swaps GlobalGroupMemberRepository for the test. +func installTriageMembership(t *testing.T) *triageMembershipRepo { + t.Helper() + repo := &triageMembershipRepo{members: map[uuid.UUID]map[uuid.UUID]bool{}} + orig := GlobalGroupMemberRepository + GlobalGroupMemberRepository = repo + t.Cleanup(func() { GlobalGroupMemberRepository = orig }) + return repo +} + +type triageFixture struct { + alice, sr, cpr, both TestUserIdentity + respStore *mockSurveyResponseStore + server *Server + normalID, confID uuid.UUID +} + +func newTriageFixture(t *testing.T) *triageFixture { + t.Helper() + gin.SetMode(gin.TestMode) + f := &triageFixture{ + alice: triageTestUser("alice"), + sr: triageTestUser("sr"), + cpr: triageTestUser("cpr"), + both: triageTestUser("both"), + } + repo := installTriageMembership(t) + repo.grant(f.sr, GroupSecurityReviewers) + repo.grant(f.cpr, GroupConfidentialProjectReviewers) + repo.grant(f.both, GroupSecurityReviewers, GroupConfidentialProjectReviewers) + + f.respStore = newMockSurveyResponseStore() + saveSurveyStores(t, nil, f.respStore) + + surveyID := uuid.New() + f.normalID = seedSurveyResponse(f.respStore, surveyID, ResponseStatusSubmitted, f.alice.InternalUUID) + f.confID = seedSurveyResponse(f.respStore, surveyID, ResponseStatusSubmitted, f.alice.InternalUUID) + yes, no := true, false + f.respStore.responses[f.confID].IsConfidential = &yes + f.respStore.responses[f.normalID].IsConfidential = &no + // The reviewer groups hold ACL rows too, as seeded in production; the + // guard must not depend on them. + f.respStore.listItems = []SurveyResponseListItem{ + {Id: &f.normalID, IsConfidential: &no, Status: ResponseStatusSubmitted}, + {Id: &f.confID, IsConfidential: &yes, Status: ResponseStatusSubmitted}, + } + f.respStore.listTotal = 2 + + noteStore := newMockTriageNoteStore() + f.server = &Server{triageNoteHandler: NewTriageNoteSubResourceHandler(noteStore)} + return f +} + +func (f *triageFixture) listIDs(t *testing.T, u TestUserIdentity) []uuid.UUID { + t.Helper() + c, w := CreateTestGinContext("GET", "/triage/survey_responses") + u.SetContext(c) + f.server.ListTriageSurveyResponses(c, ListTriageSurveyResponsesParams{}) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + var resp ListSurveyResponsesResponse + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &resp)) + assert.Equal(t, len(resp.SurveyResponses), resp.Total) + ids := []uuid.UUID{} + for _, it := range resp.SurveyResponses { + ids = append(ids, *it.Id) + } + return ids +} + +func TestTriageList_ScopedByReviewerGroup(t *testing.T) { + f := newTriageFixture(t) + + assert.Empty(t, f.listIDs(t, f.alice), "submitter without reviewer group sees nothing") + assert.Equal(t, []uuid.UUID{f.normalID}, f.listIDs(t, f.sr)) + assert.Equal(t, []uuid.UUID{f.confID}, f.listIDs(t, f.cpr)) + assert.ElementsMatch(t, []uuid.UUID{f.normalID, f.confID}, f.listIDs(t, f.both)) +} + +func TestTriageList_MembershipLookupFailureIs500(t *testing.T) { + f := newTriageFixture(t) + GlobalGroupMemberRepository.(*triageMembershipRepo).err = fmt.Errorf("db down") + c, w := CreateTestGinContext("GET", "/triage/survey_responses") + f.sr.SetContext(c) + f.server.ListTriageSurveyResponses(c, ListTriageSurveyResponsesParams{}) + assert.Equal(t, http.StatusInternalServerError, w.Code) +} + +func TestTriagePerID_DeniedWithout404Leak(t *testing.T) { + f := newTriageFixture(t) + patch := []byte(`[{"op":"replace","path":"/status","value":"ready_for_review"}]`) + + type call func(u TestUserIdentity, id uuid.UUID) int + calls := map[string]call{ + "get": func(u TestUserIdentity, id uuid.UUID) int { + c, w := CreateTestGinContext("GET", "/x") + u.SetContext(c) + f.server.GetTriageSurveyResponse(c, id) + return w.Code + }, + "patch": func(u TestUserIdentity, id uuid.UUID) int { + c, w := CreateTestGinContextWithBody("PATCH", "/x", "application/json-patch+json", patch) + u.SetContext(c) + f.server.PatchTriageSurveyResponse(c, id) + return w.Code + }, + "create_threat_model": func(u TestUserIdentity, id uuid.UUID) int { + c, w := CreateTestGinContext("POST", "/x") + u.SetContext(c) + f.server.CreateThreatModelFromSurveyResponse(c, id) + return w.Code + }, + "metadata": func(u TestUserIdentity, id uuid.UUID) int { + c, w := CreateTestGinContext("GET", "/x") + u.SetContext(c) + f.server.GetTriageSurveyResponseMetadata(c, id) + return w.Code + }, + "metadata_by_key": func(u TestUserIdentity, id uuid.UUID) int { + c, w := CreateTestGinContext("GET", "/x") + u.SetContext(c) + f.server.GetTriageSurveyResponseMetadataByKey(c, id, "k") + return w.Code + }, + "triage_notes_list": func(u TestUserIdentity, id uuid.UUID) int { + c, w := CreateTestGinContext("GET", "/x") + u.SetContext(c) + f.server.ListTriageSurveyResponseTriageNotes(c, id, ListTriageSurveyResponseTriageNotesParams{}) + return w.Code + }, + "triage_notes_create": func(u TestUserIdentity, id uuid.UUID) int { + c, w := CreateTestGinContextWithBody("POST", "/x", "application/json", []byte(`{"name":"n","content":"c"}`)) + u.SetContext(c) + f.server.CreateTriageSurveyResponseTriageNote(c, id) + return w.Code + }, + "triage_notes_get": func(u TestUserIdentity, id uuid.UUID) int { + c, w := CreateTestGinContext("GET", "/x") + u.SetContext(c) + f.server.GetTriageSurveyResponseTriageNote(c, id, 1) + return w.Code + }, + } + + for name, do := range calls { + t.Run(name, func(t *testing.T) { + // Submitter owns both rows and must be denied on both. + assert.Equal(t, http.StatusNotFound, do(f.alice, f.normalID), "alice/N") + assert.Equal(t, http.StatusNotFound, do(f.alice, f.confID), "alice/C") + // Cross-group denial. + assert.Equal(t, http.StatusNotFound, do(f.sr, f.confID), "sr/C") + assert.Equal(t, http.StatusNotFound, do(f.cpr, f.normalID), "cpr/N") + // Nonexistent id is indistinguishable. + assert.Equal(t, http.StatusNotFound, do(f.sr, uuid.New()), "sr/missing") + }) + } +} + +func TestTriagePerID_ReviewersAllowed(t *testing.T) { + f := newTriageFixture(t) + + get := func(u TestUserIdentity, id uuid.UUID) int { + c, w := CreateTestGinContext("GET", "/x") + u.SetContext(c) + f.server.GetTriageSurveyResponse(c, id) + return w.Code + } + assert.Equal(t, http.StatusOK, get(f.sr, f.normalID)) + assert.Equal(t, http.StatusOK, get(f.cpr, f.confID)) + assert.Equal(t, http.StatusOK, get(f.both, f.normalID)) + assert.Equal(t, http.StatusOK, get(f.both, f.confID)) + + // sr can patch N even though the ACL grants sr no row. + c, w := CreateTestGinContextWithBody("PATCH", "/x", "application/json-patch+json", + []byte(`[{"op":"replace","path":"/status","value":"ready_for_review"}]`)) + f.sr.SetContext(c) + f.server.PatchTriageSurveyResponse(c, f.normalID) + assert.Equal(t, http.StatusOK, w.Code, w.Body.String()) + + // Triage notes: a reviewer who has no ACL row reaches the note store. + c2, w2 := CreateTestGinContext("GET", "/x") + f.cpr.SetContext(c2) + f.server.ListTriageSurveyResponseTriageNotes(c2, f.confID, ListTriageSurveyResponseTriageNotesParams{}) + assert.Equal(t, http.StatusOK, w2.Code, w2.Body.String()) +} + +func TestTriageAccess_IntakeUnchangedForSubmitter(t *testing.T) { + f := newTriageFixture(t) + + c, w := CreateTestGinContext("GET", "/x") + f.alice.SetContext(c) + f.server.GetIntakeSurveyResponse(c, f.normalID) + assert.Equal(t, http.StatusOK, w.Code, w.Body.String()) + + draftID := seedSurveyResponse(f.respStore, uuid.New(), ResponseStatusDraft, f.alice.InternalUUID) + c2, w2 := CreateTestGinContextWithBody("PATCH", "/x", "application/json-patch+json", + []byte(`[{"op":"replace","path":"/ui_state","value":{"a":1}}]`)) + f.alice.SetContext(c2) + f.server.PatchIntakeSurveyResponse(c2, draftID, PatchIntakeSurveyResponseParams{}) + assert.Equal(t, http.StatusOK, w2.Code, "submitter keeps intake patch access: %s", w2.Body.String()) + + c3, w3 := CreateTestGinContext("GET", "/x") + f.alice.SetContext(c3) + f.server.ListIntakeSurveyResponseTriageNotes(c3, f.normalID, ListIntakeSurveyResponseTriageNotesParams{}) + assert.Equal(t, http.StatusOK, w3.Code, w3.Body.String()) +} + +func TestTriagePerID_NilConfidentialityFailsClosed(t *testing.T) { + f := newTriageFixture(t) + f.respStore.responses[f.normalID].IsConfidential = nil + + get := func(u TestUserIdentity) int { + c, w := CreateTestGinContext("GET", "/x") + u.SetContext(c) + f.server.GetTriageSurveyResponse(c, f.normalID) + return w.Code + } + // Unknown confidentiality is treated as confidential: only the + // Confidential Project Reviewers group may triage it. + assert.Equal(t, http.StatusNotFound, get(f.sr), "sr/nil") + assert.Equal(t, http.StatusOK, get(f.cpr), "cpr/nil") +} + +func TestTriageReviewerGroup(t *testing.T) { + assert.Equal(t, GroupConfidentialProjectReviewers, triageReviewerGroup(true)) + assert.Equal(t, GroupSecurityReviewers, triageReviewerGroup(false)) +} + +// asTriageUser installs a membership repository and authenticates c as a fresh +// user belonging to the given built-in groups (none = a non-reviewer). +func asTriageUser(t *testing.T, c *gin.Context, groups ...BuiltInGroup) TestUserIdentity { + t.Helper() + u := triageTestUser("triage") + installTriageMembership(t).grant(u, groups...) + u.SetContext(c) + return u +} + +func TestTriageMetadata_ReviewerAllowed(t *testing.T) { + f := newTriageFixture(t) + md := &MockMetadataStore{} + md.On("List", mock.Anything, "survey_response", f.normalID.String()).Return([]Metadata{{Key: "k", Value: "v"}}, nil) + md.On("Get", mock.Anything, "survey_response", f.normalID.String(), "k").Return(&Metadata{Key: "k", Value: "v"}, nil) + f.server.surveyResponseMetadata = NewGenericMetadataHandler(md, "survey_response", "survey_response_id", nil) + + c, w := CreateTestGinContext("GET", "/x") + f.sr.SetContext(c) + c.Params = append(c.Params, gin.Param{Key: "survey_response_id", Value: f.normalID.String()}) + f.server.GetTriageSurveyResponseMetadata(c, f.normalID) + assert.Equal(t, http.StatusOK, w.Code, w.Body.String()) + + c2, w2 := CreateTestGinContext("GET", "/x") + f.sr.SetContext(c2) + c2.Params = append(c2.Params, + gin.Param{Key: "survey_response_id", Value: f.normalID.String()}, + gin.Param{Key: "key", Value: "k"}) + f.server.GetTriageSurveyResponseMetadataByKey(c2, f.normalID, "k") + assert.Equal(t, http.StatusOK, w2.Code, w2.Body.String()) +} + +// Intake callers may only set status to draft or submitted; approval states +// belong to triage reviewers (#1100). +func TestIntakePatch_SubmitterCannotSetReviewStatus(t *testing.T) { + f := newTriageFixture(t) + + patchStatus := func(id uuid.UUID, status string) (int, string) { + body := []byte(fmt.Sprintf(`[{"op":"replace","path":"/status","value":%q}]`, status)) + c, w := CreateTestGinContextWithBody("PATCH", "/x", "application/json-patch+json", body) + f.alice.SetContext(c) + f.server.PatchIntakeSurveyResponse(c, id, PatchIntakeSurveyResponseParams{}) + return w.Code, w.Body.String() + } + + for _, from := range []string{ResponseStatusDraft, ResponseStatusNeedsRevision} { + for _, target := range []string{ResponseStatusReadyForReview, ResponseStatusNeedsRevision, ResponseStatusReviewCreated} { + if from == target { + continue + } + t.Run(from+"_to_"+target, func(t *testing.T) { + id := seedSurveyResponse(f.respStore, uuid.New(), from, f.alice.InternalUUID) + code, body := patchStatus(id, target) + assert.Equal(t, http.StatusBadRequest, code, body) + assert.Equal(t, from, *f.respStore.responses[id].Status, "status must be unchanged") + }) + } + } + + t.Run("draft_to_submitted_allowed", func(t *testing.T) { + id := seedSurveyResponse(f.respStore, uuid.New(), ResponseStatusDraft, f.alice.InternalUUID) + code, body := patchStatus(id, ResponseStatusSubmitted) + assert.Equal(t, http.StatusOK, code, body) + }) + t.Run("needs_revision_to_submitted_allowed", func(t *testing.T) { + id := seedSurveyResponse(f.respStore, uuid.New(), ResponseStatusNeedsRevision, f.alice.InternalUUID) + code, body := patchStatus(id, ResponseStatusSubmitted) + assert.Equal(t, http.StatusOK, code, body) + }) +} + +func TestValidateIntakeStatusTarget(t *testing.T) { + assert.NoError(t, validateIntakeStatusTarget(ResponseStatusDraft)) + assert.NoError(t, validateIntakeStatusTarget(ResponseStatusSubmitted)) + assert.Error(t, validateIntakeStatusTarget(ResponseStatusReadyForReview)) + assert.Error(t, validateIntakeStatusTarget(ResponseStatusReviewCreated)) + assert.Error(t, validateIntakeStatusTarget("bogus")) +} diff --git a/api/triage_note_handlers.go b/api/triage_note_handlers.go index 3cb3c62a8..5e562f259 100644 --- a/api/triage_note_handlers.go +++ b/api/triage_note_handlers.go @@ -31,15 +31,20 @@ func NewTriageNoteSubResourceHandler(store TriageNoteStore) *TriageNoteSubResour // this check, anyone authenticated could enumerate or write triage notes on // arbitrary survey responses (T5, #357). // -// Returns the parent survey-response ID string and true on success. On -// failure (not found OR access denied — collapsed into 404 to avoid existence -// disclosure) writes the error response and returns false. -// SEM@368e91d91cb110162c64b6ea10d49562a59bf3f0: authorize the caller's required role on a parent survey response, returning 404 on any failure +// Returns true on success. On failure (not found OR access denied, collapsed +// into 404 to avoid existence disclosure) writes the error response and +// returns false. +// SEM@49259ca96d8bf22bf9c7a384c528d9a39bebe16d: authorize the caller on a parent survey response via row ACL, or accept a pre-verified triage reviewer, returning 404 on failure func (h *TriageNoteSubResourceHandler) requireSurveyResponseAccessForTriageNote( c *gin.Context, surveyResponseID string, requiredRole AuthorizationRole, ) bool { + // Triage routes authorize by reviewer-group membership (RequireTriageAccess) + // before delegating here; the intake-oriented row ACL does not apply. + if verified, _ := c.Get(triageAccessVerifiedKey); verified == true { + return true + } surveyResponseUUID, _ := ParseUUID(surveyResponseID) // already validated format by caller if _, ok := RequireSurveyResponseAccess(c, surveyResponseUUID, requiredRole); !ok { return false @@ -178,7 +183,7 @@ func (h *TriageNoteSubResourceHandler) GetTriageNote(c *gin.Context) { } // CreateTriageNote creates a new triage note in a survey response -// SEM@b01ccb8e475aed5b956de76b96fe25b3de6076d0: store a new sanitized triage note under a survey response, enforcing writer access (writes DB) +// SEM@db94db3c8118cffb5220bf9a44c2fde0ada20427: store a new sanitized triage note under a survey response after access check (writes DB) func (h *TriageNoteSubResourceHandler) CreateTriageNote(c *gin.Context) { logger := slogging.GetContextLogger(c) logger.Debug("CreateTriageNote - creating new triage note") diff --git a/docs/superpowers/specs/2026-10-17-adr-triage-access-by-reviewer-group.md b/docs/superpowers/specs/2026-10-17-adr-triage-access-by-reviewer-group.md new file mode 100644 index 000000000..694f4dd9c --- /dev/null +++ b/docs/superpowers/specs/2026-10-17-adr-triage-access-by-reviewer-group.md @@ -0,0 +1,38 @@ +# ADR: Restrict survey triage to reviewer groups by confidentiality + +- Status: Accepted +- Date: 2026-10-17 +- Decision maker: Eric Fitzgerald (human decision, 2026-10-17) +- Issue: #1100 + +## Context + +The `/triage/survey_responses/*` endpoints authorized callers with the per-response ACL (`survey_response_access`). Response creation seeds that ACL with the submitter as `owner`, so a submitter could read and change their own response through the triage endpoints (including patching status and creating a threat model from it). The submitter is supposed to reach their response through the intake endpoints only. + +## Human decision (Eric, 2026-10-17) + +Triage endpoints allow only members of **Security Reviewers** to see or change **non-confidential** survey responses, and only members of **Confidential Project Reviewers** to see or change **confidential** ones, regardless of ownership. Ownership grants no triage access. The submitter keeps access through the intake endpoints only. + +## Implementation choices (controller rulings under that decision) + +- **Per-id denial is 404 `not_found`**, the existing non-disclosure convention (T5, #357), so a denied caller cannot tell an existing response from a missing one. The same 404 covers a missing response. +- **List returns 200 with an empty page** for a caller in neither reviewer group. A caller in one group sees only responses with the matching `is_confidential`; a caller in both sees all. The confidentiality filter is applied in the store query (GORM `ColumnMap` predicate, no dialect-specific SQL), so totals and pagination stay correct and do not leak counts. +- **No administrator bypass.** Administrators are not reviewers by virtue of the role; they need the group membership like anyone else. +- **ACL seeding is unchanged.** The submitter's owner row in `survey_response_access` stays because intake depends on it. Triage simply stops consulting that table. +- **Membership is resolved per request** with `IsGroupMemberFromContext`, so IdP-mapped and nested group membership counts, unlike the store's `HasAccess`, which reads direct `group_members` rows only. +- **No OpenAPI change.** 404 and 403 are already documented on the per-id triage operations, and the list returns 200. +- **Triage sub-resources** (metadata, triage notes) use the same guard. Triage notes are shared between the intake (read-only) and triage routes; the triage routes mark the request as verified so the shared handler skips the intake-oriented ACL check, while the intake routes keep the ACL check. + +## Why a guard rather than an `x-tmi-authz` role + +A static role gate on the route cannot see the per-row `is_confidential` value, so it cannot choose between the two groups. A gate allowing either group would let a Security Reviewers member reach confidential responses. The handler-level guard `RequireTriageAccess` loads the row and checks membership in `triageReviewerGroup(is_confidential)`. + +## Consequences + +- A submitter who is not a reviewer gets 404 on every triage per-id route and an empty triage list. +- A reviewer no longer needs an ACL row on a response to triage it, and an ACL row alone no longer grants triage. +- Existing tests that encoded owner-based triage access were updated to authenticate as reviewer-group members. + +## Intake status writes (controller ruling, confirmed by Eric as a human decision, 2026-10-17) + +Restricting triage endpoints is not enough if the submitter can reach the same outcome through intake: `PATCH /intake/survey_responses/{id}` previously accepted any valid status value, so a submitter could move their own response to `ready_for_review` or `review_created` and bypass triage. Intake writes may now set status only to `draft` or `submitted`; any other target returns 400 `invalid_input` from one shared helper, `validateIntakeStatusTarget`. `PUT` and create carry no status field, so PATCH is the only intake path affected. Triage status transitions are unchanged. diff --git a/test/integration/workflows/survey_response_workflow_test.go b/test/integration/workflows/survey_response_workflow_test.go index 763ac846c..9caf43e26 100644 --- a/test/integration/workflows/survey_response_workflow_test.go +++ b/test/integration/workflows/survey_response_workflow_test.go @@ -2,6 +2,7 @@ package workflows import ( "encoding/json" + "fmt" "os" "testing" @@ -21,6 +22,43 @@ import ( // - PATCH /triage/survey_responses/{survey_response_id} (PatchTriageSurveyResponse) // // Total: 10 operations +// confidentialReviewersGroupUUID is the well-known UUID of the built-in +// Confidential Project Reviewers group. +const confidentialReviewersGroupUUID = "00000000-0000-0000-0000-000000000003" + +// newReviewerClient authenticates userID and adds that user to the given +// built-in group in the server's database. Group membership is resolved per +// request, so no re-authentication is needed. +func newReviewerClient(t *testing.T, serverURL, userID, groupUUID string) *framework.IntegrationClient { + t.Helper() + tokens, err := framework.AuthenticateUser(userID) + framework.AssertNoError(t, err, "Authentication failed for "+userID) + c, err := framework.NewClient(serverURL, tokens) + framework.AssertNoError(t, err, "Failed to create client for "+userID) + + db, err := framework.NewDevDatabase() + framework.AssertNoError(t, err, "Failed to open database") + defer db.Close() + userUUID, err := db.QueryString(fmt.Sprintf( + "SELECT internal_uuid FROM users WHERE provider_user_id = '%s' AND provider = 'tmi' FETCH FIRST 1 ROWS ONLY", userID)) + framework.AssertNoError(t, err, "Failed to look up user") + if userUUID == "" { + t.Fatalf("user %s not found in database", userID) + } + err = db.InsertIgnoreDuplicate(fmt.Sprintf( + "INSERT INTO group_members (id, group_internal_uuid, user_internal_uuid, subject_type, added_at, notes) "+ + "VALUES ('%s-%s', '%s', '%s', 'user', CURRENT_TIMESTAMP, 'Integration test reviewer')", + userID, groupUUID[len(groupUUID)-4:], groupUUID, userUUID)) + framework.AssertNoError(t, err, "Failed to add group member") + t.Cleanup(func() { + if cdb, cerr := framework.NewDevDatabase(); cerr == nil { + defer cdb.Close() + _ = cdb.ExecSQL(fmt.Sprintf("DELETE FROM group_members WHERE id = '%s-%s'", userID, groupUUID[len(groupUUID)-4:])) + } + }) + return c +} + func TestSurveyResponseWorkflow(t *testing.T) { if os.Getenv("INTEGRATION_TESTS") != "true" { t.Skip("Skipping integration test (set INTEGRATION_TESTS=true to run)") @@ -248,8 +286,49 @@ func TestSurveyResponseWorkflow(t *testing.T) { t.Logf("Listed %d triage survey responses (total: %d)", len(response.SurveyResponses), response.Total) }) + // #1100: triage is open only to reviewer-group members; the submitter (here + // the admin user, who owns the response) gets 404 on the per-id routes. + srClient := newReviewerClient(t, serverURL, "test-sr-reviewer", securityReviewersGroupUUID) + cprClient := newReviewerClient(t, serverURL, "test-cpr-reviewer", confidentialReviewersGroupUUID) + + markReady := []map[string]interface{}{ + {"op": "replace", "path": "/status", "value": "ready_for_review"}, + } + + t.Run("TriageDeniedToSubmitter", func(t *testing.T) { + resp, err := client.Do(framework.Request{Method: "GET", Path: "/triage/survey_responses/" + responseID}) + framework.AssertNoError(t, err, "Failed to get triage survey response") + framework.AssertStatusCode(t, resp, 404) + + resp, err = client.Do(framework.Request{Method: "PATCH", Path: "/triage/survey_responses/" + responseID, Body: markReady}) + framework.AssertNoError(t, err, "Failed to patch triage survey response") + framework.AssertStatusCode(t, resp, 404) + + // Cross-group: a Confidential Project Reviewers member cannot triage a + // non-confidential response. + resp, err = cprClient.Do(framework.Request{Method: "GET", Path: "/triage/survey_responses/" + responseID}) + framework.AssertNoError(t, err, "Failed to get triage survey response as cpr") + framework.AssertStatusCode(t, resp, 404) + }) + + t.Run("SubmitterCannotSelfApproveViaIntake", func(t *testing.T) { + fixture := framework.NewSurveyResponseFixture(surveyID).WithAuthorization(userAuth) + resp, err := client.Do(framework.Request{Method: "POST", Path: "/intake/survey_responses", Body: fixture}) + framework.AssertNoError(t, err, "Failed to create draft survey response") + framework.AssertStatusCreated(t, resp) + draftID := framework.ExtractID(t, resp, "id") + + resp, err = client.Do(framework.Request{Method: "PATCH", Path: "/intake/survey_responses/" + draftID, Body: markReady}) + framework.AssertNoError(t, err, "Failed to patch intake survey response") + framework.AssertStatusCode(t, resp, 400) + + resp, err = client.Do(framework.Request{Method: "DELETE", Path: "/intake/survey_responses/" + draftID}) + framework.AssertNoError(t, err, "Failed to delete draft survey response") + framework.AssertStatusCode(t, resp, 204) + }) + t.Run("GetTriageResponse", func(t *testing.T) { - resp, err := client.Do(framework.Request{ + resp, err := srClient.Do(framework.Request{ Method: "GET", Path: "/triage/survey_responses/" + responseID, }) @@ -263,18 +342,10 @@ func TestSurveyResponseWorkflow(t *testing.T) { }) t.Run("PatchTriageResponse_MarkReadyForReview", func(t *testing.T) { - patchPayload := []map[string]interface{}{ - { - "op": "replace", - "path": "/status", - "value": "ready_for_review", - }, - } - - resp, err := client.Do(framework.Request{ + resp, err := srClient.Do(framework.Request{ Method: "PATCH", Path: "/triage/survey_responses/" + responseID, - Body: patchPayload, + Body: markReady, }) framework.AssertNoError(t, err, "Failed to patch triage survey response") framework.AssertStatusOK(t, resp) @@ -284,6 +355,33 @@ func TestSurveyResponseWorkflow(t *testing.T) { t.Logf("Marked survey response as ready_for_review: %s", responseID) }) + t.Run("ConfidentialResponse_TriageByCPROnly", func(t *testing.T) { + fixture := framework.NewSurveyResponseFixture(surveyID). + WithAuthorization(userAuth). + WithConfidential(true) + resp, err := client.Do(framework.Request{Method: "POST", Path: "/intake/survey_responses", Body: fixture}) + framework.AssertNoError(t, err, "Failed to create confidential survey response") + framework.AssertStatusCreated(t, resp) + confID := framework.ExtractID(t, resp, "id") + + submit := []map[string]interface{}{{"op": "replace", "path": "/status", "value": "submitted"}} + resp, err = client.Do(framework.Request{Method: "PATCH", Path: "/intake/survey_responses/" + confID, Body: submit}) + framework.AssertNoError(t, err, "Failed to submit confidential survey response") + framework.AssertStatusOK(t, resp) + + resp, err = srClient.Do(framework.Request{Method: "GET", Path: "/triage/survey_responses/" + confID}) + framework.AssertNoError(t, err, "Failed to get confidential response as sr") + framework.AssertStatusCode(t, resp, 404) + + resp, err = cprClient.Do(framework.Request{Method: "GET", Path: "/triage/survey_responses/" + confID}) + framework.AssertNoError(t, err, "Failed to get confidential response as cpr") + framework.AssertStatusOK(t, resp) + + resp, err = cprClient.Do(framework.Request{Method: "PATCH", Path: "/triage/survey_responses/" + confID, Body: markReady}) + framework.AssertNoError(t, err, "Failed to patch confidential response as cpr") + framework.AssertStatusOK(t, resp) + }) + // Create a second response for delete testing (only draft responses can be deleted) var draftResponseID string diff --git a/test/seeds/cats-seed-data.json b/test/seeds/cats-seed-data.json index 2239d81d2..06e12accd 100644 --- a/test/seeds/cats-seed-data.json +++ b/test/seeds/cats-seed-data.json @@ -11,7 +11,8 @@ "email": "charlie@tmi.local", "display_name": "Charlie (Seed User)", "roles": { - "is_admin": true + "is_admin": true, + "is_security_reviewer": true }, "oauth_provider": "tmi", "api_quota": {