Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions fulfillment-service/internal/database/dao/generic_dao_list.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,9 @@ func (r *ListRequest[O]) do(ctx context.Context) (response *ListResponse[O], err
if r.sql.filter.Len() > 0 {
r.sql.filter.WriteString(` and `)
}
r.sql.filter.WriteString(`(`)
r.sql.filter.WriteString(filter)
r.sql.filter.WriteString(`)`)
}

// Calculate the order clause:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,62 @@ var _ = Describe("Tenant visibility", func() {
Expect(getResponse.GetObject().GetMyString()).To(Equal("updated"))
})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] naming-convention

Test description "Does not leak cross-tenant rows when filter has a top-level OR" uses a negation form not seen in other test descriptions in this file. Other tests use positive third-person verb patterns: "Filters field based on...", "Shows all tenants when...", "Allows a tenant to...", "Rejects update of...". Consider rephrasing to follow the established pattern, e.g., "Isolates tenant rows when filter has a top-level OR".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed test description from "Does not leak cross-tenant rows when filter has a top-level OR" to "Isolates tenant rows when filter has a top-level OR" to follow the file's positive third-person verb pattern.

Addressed in 63e1906.

It("Isolates tenant rows when filter has a top-level OR", func() {
// Create an unrestricted tenancy to seed objects in different tenants:
tenancyAll := auth.NewMockTenancyLogic(ctrl)
tenancyAll.EXPECT().DetermineVisibleTenants(gomock.Any()).
Return(collections.NewUniversalSet[string](), nil).
AnyTimes()
daoAll, err := NewGenericDAO[*testsv1.Object]().
SetLogger(logger).
SetTenancyLogic(tenancyAll).
Build()
Expect(err).ToNot(HaveOccurred())

// Create an object in tenant-a (will be visible):
_, err = daoAll.Create().
SetObject(testsv1.Object_builder{
Metadata: testsv1.Metadata_builder{
Tenant: "tenant-a",
Name: "shared-name",
}.Build(),
}.Build()).
Do(ctx)
Expect(err).ToNot(HaveOccurred())

// Create an object in tenant-b with the same name (should be invisible to tenant-a):
_, err = daoAll.Create().
SetObject(testsv1.Object_builder{
Metadata: testsv1.Metadata_builder{
Tenant: "tenant-b",
Name: "shared-name",
}.Build(),
}.Build()).
Do(ctx)
Expect(err).ToNot(HaveOccurred())

// Create a restricted DAO that can only see tenant-a:
tenancyRestricted := auth.NewMockTenancyLogic(ctrl)
tenancyRestricted.EXPECT().DetermineVisibleTenants(gomock.Any()).
Return(collections.NewSet("tenant-a"), nil).
AnyTimes()
daoRestricted, err := NewGenericDAO[*testsv1.Object]().
SetLogger(logger).
SetTenancyLogic(tenancyRestricted).
Build()
Expect(err).ToNot(HaveOccurred())

// List with a top-level OR filter (id-or-name pattern). The tenancy clause
// must apply to the entire filter, not just the first branch:
listResponse, err := daoRestricted.List().
SetFilter(`this.id == "nonexistent" || this.metadata.name == "shared-name"`).
Do(ctx)
Expect(err).ToNot(HaveOccurred())
Expect(listResponse.GetTotal()).To(Equal(int32(1)))
Expect(listResponse.GetItems()).To(HaveLen(1))
Expect(listResponse.GetItems()[0].GetMetadata().GetTenant()).To(Equal("tenant-a"))
})

It("Rejects update of an object belonging to an invisible tenant as not found", func() {
// Create a tenancy logic that makes all tenants visible, used to create the object:
tenancyA := auth.NewMockTenancyLogic(ctrl)
Expand Down
Loading