Skip to content

Commit 975d775

Browse files
sultanimansultaniman
committed
fix project-scoped auth gaps
Fix refresh token cookie scoping so browsers send it to /api/auth refresh and logout endpoints. Enforce person-to-project validation on nested project reads for documents, notes, migration records, person categories, and person tags. Update handlers to surface these failures through domain error mapping instead of returning internal server errors. Add regression coverage for cookie path handling and cross-project IDOR attempts on the affected read paths.
1 parent 986dfb5 commit 975d775

22 files changed

Lines changed: 480 additions & 85 deletions

‎internal/app/container.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -173,10 +173,10 @@ func NewContainer(cfg *config.Config, db database.DB, redisClient *redis.Client)
173173
personCategoryUC := ucproject.NewPersonCategoryUseCase(personCatRepo, personRepo)
174174
personTagUC := ucproject.NewPersonTagUseCase(personTagRepo, personRepo)
175175
supportRecordUC := ucproject.NewSupportRecordUseCase(supportRepo, personRepo, auditUC)
176-
migrationRecordUC := ucproject.NewMigrationRecordUseCase(migrationRepo, auditUC)
177-
householdUC := ucproject.NewHouseholdUseCase(householdRepo, householdMemberRepo, auditUC)
178-
noteUC := ucproject.NewNoteUseCase(noteRepo, auditUC)
179-
documentUC := ucproject.NewDocumentUseCase(documentRepo, fileStorage, auditUC)
176+
migrationRecordUC := ucproject.NewMigrationRecordUseCase(migrationRepo, personRepo, auditUC)
177+
householdUC := ucproject.NewHouseholdUseCase(householdRepo, householdMemberRepo, personRepo, auditUC)
178+
noteUC := ucproject.NewNoteUseCase(noteRepo, personRepo, auditUC)
179+
documentUC := ucproject.NewDocumentUseCase(documentRepo, personRepo, fileStorage, auditUC)
180180
petUC := ucproject.NewPetUseCase(petRepo, petTagRepo, auditUC)
181181
petTagUC := ucproject.NewPetTagUseCase(petTagRepo, petRepo)
182182
reportUC := ucreport.NewReportUseCase(reportRepo)

‎internal/handler/project/document.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,9 +42,10 @@ func (h *DocumentHandler) List(c *gin.Context) {
4242
return
4343
}
4444
personID := c.Param("person_id")
45-
out, err := h.uc.List(c.Request.Context(), personID)
45+
projectID := c.Param("project_id")
46+
out, err := h.uc.List(c.Request.Context(), projectID, personID)
4647
if err != nil {
47-
handler.InternalError(c, "list documents", err)
48+
handler.HandleError(c, err)
4849
return
4950
}
5051
c.JSON(http.StatusOK, gin.H{"documents": out})

‎internal/handler/project/document_test.go‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
"go.uber.org/mock/gomock"
1414

1515
"github.com/lbrty/observer/internal/domain/document"
16+
"github.com/lbrty/observer/internal/domain/person"
1617
"github.com/lbrty/observer/internal/handler/handlertest"
1718
"github.com/lbrty/observer/internal/handler/project"
1819
"github.com/lbrty/observer/internal/middleware"
@@ -43,8 +44,12 @@ func newMultipartUploadContext(filename string, content []byte, projectID, perso
4344

4445
func newDocumentHandler(ctrl *gomock.Controller) (*project.DocumentHandler, *repomock.MockDocumentRepository, *storagemock.MockFileStorage) {
4546
docRepo := repomock.NewMockDocumentRepository(ctrl)
47+
personRepo := repomock.NewMockPersonRepository(ctrl)
4648
fs := storagemock.NewMockFileStorage(ctrl)
47-
uc := ucproject.NewDocumentUseCase(docRepo, fs, nil)
49+
personRepo.EXPECT().GetByID(gomock.Any(), gomock.Any()).AnyTimes().DoAndReturn(func(_ any, id string) (*person.Person, error) {
50+
return &person.Person{ID: id, ProjectID: "x"}, nil
51+
})
52+
uc := ucproject.NewDocumentUseCase(docRepo, personRepo, fs, nil)
4853
return project.NewDocumentHandler(uc), docRepo, fs
4954
}
5055

@@ -58,6 +63,7 @@ func TestDocumentHandler_List_NoPermission(t *testing.T) {
5863

5964
personID := handlertest.TestID().String()
6065
c, w := handlertest.NewTestContextWithParams(http.MethodGet, "/projects/x/people/"+personID+"/documents", nil, gin.Params{
66+
{Key: "project_id", Value: "x"},
6167
{Key: "person_id", Value: personID},
6268
})
6369
setCanViewDocuments(c, false)
@@ -78,6 +84,7 @@ func TestDocumentHandler_List_Success(t *testing.T) {
7884
}, nil)
7985

8086
c, w := handlertest.NewTestContextWithParams(http.MethodGet, "/projects/x/people/"+personID+"/documents", nil, gin.Params{
87+
{Key: "project_id", Value: "x"},
8188
{Key: "person_id", Value: personID},
8289
})
8390
setCanViewDocuments(c, true)
@@ -241,7 +248,7 @@ func TestDocumentHandler_Upload_RejectsForbiddenMIME(t *testing.T) {
241248
ctrl := gomock.NewController(t)
242249
h, _, _ := newDocumentHandler(ctrl)
243250

244-
projectID := handlertest.TestID().String()
251+
projectID := "x"
245252
personID := handlertest.TestID().String()
246253

247254
htmlContent := []byte("<html><body><script>alert(1)</script></body></html>")
@@ -257,7 +264,7 @@ func TestDocumentHandler_Upload_RejectsXMLSVG(t *testing.T) {
257264
ctrl := gomock.NewController(t)
258265
h, _, _ := newDocumentHandler(ctrl)
259266

260-
projectID := handlertest.TestID().String()
267+
projectID := "x"
261268
personID := handlertest.TestID().String()
262269

263270
svgContent := []byte(`<?xml version="1.0"?><svg xmlns="http://www.w3.org/2000/svg"><script>alert(1)</script></svg>`)
@@ -273,7 +280,7 @@ func TestDocumentHandler_Upload_SanitizesFilename(t *testing.T) {
273280
ctrl := gomock.NewController(t)
274281
h, docRepo, fs := newDocumentHandler(ctrl)
275282

276-
projectID := handlertest.TestID().String()
283+
projectID := "x"
277284
personID := handlertest.TestID().String()
278285

279286
docRepo.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil)

‎internal/handler/project/household_test.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"go.uber.org/mock/gomock"
1111

1212
"github.com/lbrty/observer/internal/domain/household"
13+
"github.com/lbrty/observer/internal/domain/person"
1314
"github.com/lbrty/observer/internal/handler/handlertest"
1415
"github.com/lbrty/observer/internal/handler/project"
1516
repomock "github.com/lbrty/observer/internal/repository/mock"
@@ -19,7 +20,11 @@ import (
1920
func newHouseholdHandler(ctrl *gomock.Controller) (*project.HouseholdHandler, *repomock.MockHouseholdRepository, *repomock.MockHouseholdMemberRepository) {
2021
repo := repomock.NewMockHouseholdRepository(ctrl)
2122
memberRepo := repomock.NewMockHouseholdMemberRepository(ctrl)
22-
uc := ucproject.NewHouseholdUseCase(repo, memberRepo, nil)
23+
personRepo := repomock.NewMockPersonRepository(ctrl)
24+
personRepo.EXPECT().GetByID(gomock.Any(), gomock.Any()).AnyTimes().DoAndReturn(func(_ any, id string) (*person.Person, error) {
25+
return &person.Person{ID: id, ProjectID: "proj1"}, nil
26+
})
27+
uc := ucproject.NewHouseholdUseCase(repo, memberRepo, personRepo, nil)
2328
return project.NewHouseholdHandler(uc), repo, memberRepo
2429
}
2530

‎internal/handler/project/migration.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,11 @@ func NewMigrationRecordHandler(uc *ucproject.MigrationRecordUseCase) *MigrationR
2121

2222
// List handles GET /projects/:project_id/people/:person_id/migration-records.
2323
func (h *MigrationRecordHandler) List(c *gin.Context) {
24+
projectID := c.Param("project_id")
2425
personID := c.Param("person_id")
25-
out, err := h.uc.ListByPerson(c.Request.Context(), personID)
26+
out, err := h.uc.ListByPerson(c.Request.Context(), projectID, personID)
2627
if err != nil {
27-
handler.InternalError(c, "list migration records", err)
28+
handler.HandleError(c, err)
2829
return
2930
}
3031
c.JSON(http.StatusOK, gin.H{"records": out})

‎internal/handler/project/migration_test.go‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"go.uber.org/mock/gomock"
1111

1212
"github.com/lbrty/observer/internal/domain/migration"
13+
"github.com/lbrty/observer/internal/domain/person"
1314
"github.com/lbrty/observer/internal/handler/handlertest"
1415
"github.com/lbrty/observer/internal/handler/project"
1516
repomock "github.com/lbrty/observer/internal/repository/mock"
@@ -18,7 +19,11 @@ import (
1819

1920
func newMigrationRecordHandler(ctrl *gomock.Controller) (*project.MigrationRecordHandler, *repomock.MockMigrationRecordRepository) {
2021
repo := repomock.NewMockMigrationRecordRepository(ctrl)
21-
uc := ucproject.NewMigrationRecordUseCase(repo, nil)
22+
personRepo := repomock.NewMockPersonRepository(ctrl)
23+
personRepo.EXPECT().GetByID(gomock.Any(), gomock.Any()).AnyTimes().DoAndReturn(func(_ any, id string) (*person.Person, error) {
24+
return &person.Person{ID: id, ProjectID: "x"}, nil
25+
})
26+
uc := ucproject.NewMigrationRecordUseCase(repo, personRepo, nil)
2227
return project.NewMigrationRecordHandler(uc), repo
2328
}
2429

@@ -34,6 +39,7 @@ func TestMigrationRecordHandler_List_Success(t *testing.T) {
3439
}, nil)
3540

3641
c, w := handlertest.NewTestContextWithParams(http.MethodGet, "/projects/x/people/"+personID+"/migration-records", nil, gin.Params{
42+
{Key: "project_id", Value: "x"},
3743
{Key: "person_id", Value: personID},
3844
})
3945
h.List(c)
@@ -96,6 +102,7 @@ func TestMigrationRecordHandler_Create_Success(t *testing.T) {
96102
c, w := handlertest.NewTestContextWithParams(http.MethodPost, "/projects/x/people/"+personID+"/migration-records", map[string]any{
97103
"movement_reason": reason,
98104
}, gin.Params{
105+
{Key: "project_id", Value: "x"},
99106
{Key: "person_id", Value: personID},
100107
})
101108
h.Create(c)

‎internal/handler/project/note.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,11 @@ func NewNoteHandler(uc *ucproject.NoteUseCase) *NoteHandler {
2222

2323
// List handles GET /projects/:project_id/people/:person_id/notes.
2424
func (h *NoteHandler) List(c *gin.Context) {
25+
projectID := c.Param("project_id")
2526
personID := c.Param("person_id")
26-
out, err := h.uc.List(c.Request.Context(), personID)
27+
out, err := h.uc.List(c.Request.Context(), projectID, personID)
2728
if err != nil {
28-
handler.InternalError(c, "list notes", err)
29+
handler.HandleError(c, err)
2930
return
3031
}
3132
c.JSON(http.StatusOK, gin.H{"notes": out})

‎internal/handler/project/note_test.go‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"go.uber.org/mock/gomock"
1111

1212
"github.com/lbrty/observer/internal/domain/note"
13+
"github.com/lbrty/observer/internal/domain/person"
1314
"github.com/lbrty/observer/internal/handler/handlertest"
1415
"github.com/lbrty/observer/internal/handler/project"
1516
repomock "github.com/lbrty/observer/internal/repository/mock"
@@ -18,7 +19,11 @@ import (
1819

1920
func newNoteHandler(ctrl *gomock.Controller) (*project.NoteHandler, *repomock.MockPersonNoteRepository) {
2021
repo := repomock.NewMockPersonNoteRepository(ctrl)
21-
uc := ucproject.NewNoteUseCase(repo, nil)
22+
personRepo := repomock.NewMockPersonRepository(ctrl)
23+
personRepo.EXPECT().GetByID(gomock.Any(), gomock.Any()).AnyTimes().DoAndReturn(func(_ any, id string) (*person.Person, error) {
24+
return &person.Person{ID: id, ProjectID: "x"}, nil
25+
})
26+
uc := ucproject.NewNoteUseCase(repo, personRepo, nil)
2227
return project.NewNoteHandler(uc), repo
2328
}
2429

@@ -35,6 +40,7 @@ func TestNoteHandler_List_Success(t *testing.T) {
3540
}, nil)
3641

3742
c, w := handlertest.NewTestContextWithParams(http.MethodGet, "/projects/x/people/"+personID+"/notes", nil, gin.Params{
43+
{Key: "project_id", Value: "x"},
3844
{Key: "person_id", Value: personID},
3945
})
4046
h.List(c)
@@ -51,6 +57,7 @@ func TestNoteHandler_Create_ValidationError(t *testing.T) {
5157

5258
personID := handlertest.TestID().String()
5359
c, w := handlertest.NewTestContextWithParams(http.MethodPost, "/projects/x/people/"+personID+"/notes", map[string]any{}, gin.Params{
60+
{Key: "project_id", Value: "x"},
5461
{Key: "person_id", Value: personID},
5562
})
5663
handlertest.SetAuthContext(c, handlertest.TestID())
@@ -71,6 +78,7 @@ func TestNoteHandler_Create_Success(t *testing.T) {
7178
c, w := handlertest.NewTestContextWithParams(http.MethodPost, "/projects/x/people/"+personID+"/notes", map[string]any{
7279
"body": "important note",
7380
}, gin.Params{
81+
{Key: "project_id", Value: "x"},
7482
{Key: "person_id", Value: personID},
7583
})
7684
handlertest.SetAuthContext(c, userID)

‎internal/handler/project/person.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -96,9 +96,9 @@ func (h *PersonHandler) Delete(c *gin.Context) {
9696

9797
// ListCategories handles GET /projects/:project_id/people/:person_id/categories.
9898
func (h *PersonHandler) ListCategories(c *gin.Context) {
99-
ids, err := h.categoryUC.List(c.Request.Context(), c.Param("person_id"))
99+
ids, err := h.categoryUC.List(c.Request.Context(), c.Param("project_id"), c.Param("person_id"))
100100
if err != nil {
101-
handler.InternalError(c, "list person categories", err)
101+
handler.HandleError(c, err)
102102
return
103103
}
104104
c.JSON(http.StatusOK, gin.H{"category_ids": ids})
@@ -119,9 +119,9 @@ func (h *PersonHandler) ReplaceCategories(c *gin.Context) {
119119

120120
// ListTags handles GET /projects/:project_id/people/:person_id/tags.
121121
func (h *PersonHandler) ListTags(c *gin.Context) {
122-
ids, err := h.tagUC.List(c.Request.Context(), c.Param("person_id"))
122+
ids, err := h.tagUC.List(c.Request.Context(), c.Param("project_id"), c.Param("person_id"))
123123
if err != nil {
124-
handler.InternalError(c, "list person tags", err)
124+
handler.HandleError(c, err)
125125
return
126126
}
127127
c.JSON(http.StatusOK, gin.H{"tag_ids": ids})

‎internal/handler/project/person_test.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -250,9 +250,11 @@ func TestPersonHandler_ListCategories_Success(t *testing.T) {
250250
personID := handlertest.TestID().String()
251251
catIDs := []string{handlertest.TestID().String(), handlertest.TestID().String()}
252252

253+
deps.personRepo.EXPECT().GetByID(gomock.Any(), personID).Return(&person.Person{ID: personID, ProjectID: "x"}, nil)
253254
deps.personCatRepo.EXPECT().List(gomock.Any(), personID).Return(catIDs, nil)
254255

255256
c, w := handlertest.NewTestContextWithParams(http.MethodGet, "/projects/x/people/"+personID+"/categories", nil, gin.Params{
257+
{Key: "project_id", Value: "x"},
256258
{Key: "person_id", Value: personID},
257259
})
258260
deps.handler.ListCategories(c)
@@ -294,9 +296,11 @@ func TestPersonHandler_ListTags_Success(t *testing.T) {
294296
personID := handlertest.TestID().String()
295297
tagIDs := []string{handlertest.TestID().String(), handlertest.TestID().String()}
296298

299+
deps.personRepo.EXPECT().GetByID(gomock.Any(), personID).Return(&person.Person{ID: personID, ProjectID: "x"}, nil)
297300
deps.personTagRepo.EXPECT().List(gomock.Any(), personID).Return(tagIDs, nil)
298301

299302
c, w := handlertest.NewTestContextWithParams(http.MethodGet, "/projects/x/people/"+personID+"/tags", nil, gin.Params{
303+
{Key: "project_id", Value: "x"},
300304
{Key: "person_id", Value: personID},
301305
})
302306
deps.handler.ListTags(c)

0 commit comments

Comments
 (0)