From 97aef7c05ba147481f0d9fdc4acb58ce05d04b74 Mon Sep 17 00:00:00 2001 From: Elad Tabak Date: Wed, 27 May 2026 16:15:45 +0300 Subject: [PATCH 1/9] OSAC-58: filter unpublished catalog items from public API The public List and Get endpoints for both cluster and compute instance catalog items now enforce published visibility. List injects a CEL filter (this.published == true) and Get returns NotFound for unpublished items. Private API endpoints remain unaffected. Generated with [Claude Code](https://claude.com/claude-code) --- internal/servers/catalog_item_validation.go | 11 ++++ .../servers/cluster_catalog_items_server.go | 6 +- .../cluster_catalog_items_server_test.go | 65 ++++++++++++++++--- .../compute_instance_catalog_items_server.go | 6 +- ...pute_instance_catalog_items_server_test.go | 65 ++++++++++++++++--- 5 files changed, 135 insertions(+), 18 deletions(-) diff --git a/internal/servers/catalog_item_validation.go b/internal/servers/catalog_item_validation.go index 866fd2ae5..7b264141f 100644 --- a/internal/servers/catalog_item_validation.go +++ b/internal/servers/catalog_item_validation.go @@ -36,6 +36,17 @@ type catalogItem interface { GetMetadata() *privatev1.Metadata } +const publishedFilter = "this.published == true" + +// addPublishedFilter appends a CEL clause that restricts results to published catalog items. +// Used by public servers to hide unpublished items from end users. +func addPublishedFilter(filter string) string { + if filter == "" { + return publishedFilter + } + return "(" + filter + ") && " + publishedFilter +} + // applyFieldDefinitions processes field definitions from a catalog item against a resource spec. // For non-editable fields: overrides user-provided values with the catalog item default. // For editable fields with user values: validates against the JSON Schema. diff --git a/internal/servers/cluster_catalog_items_server.go b/internal/servers/cluster_catalog_items_server.go index c9e74c6c4..3f3744045 100644 --- a/internal/servers/cluster_catalog_items_server.go +++ b/internal/servers/cluster_catalog_items_server.go @@ -126,7 +126,7 @@ func (s *ClusterCatalogItemsServer) List(ctx context.Context, privateRequest := &privatev1.ClusterCatalogItemsListRequest{} privateRequest.SetOffset(request.GetOffset()) privateRequest.SetLimit(request.GetLimit()) - privateRequest.SetFilter(request.GetFilter()) + privateRequest.SetFilter(addPublishedFilter(request.GetFilter())) privateRequest.SetOrder(request.GetOrder()) privateResponse, err := s.delegate.List(ctx, privateRequest) @@ -163,6 +163,10 @@ func (s *ClusterCatalogItemsServer) Get(ctx context.Context, return nil, err } + if !privateResponse.GetObject().GetPublished() { + return nil, grpcstatus.Errorf(grpccodes.NotFound, "catalog item not found") + } + publicCatalogItem := &publicv1.ClusterCatalogItem{} err = s.outMapper.Copy(ctx, privateResponse.GetObject(), publicCatalogItem) if err != nil { diff --git a/internal/servers/cluster_catalog_items_server_test.go b/internal/servers/cluster_catalog_items_server_test.go index d4ff648e1..5a6e85e99 100644 --- a/internal/servers/cluster_catalog_items_server_test.go +++ b/internal/servers/cluster_catalog_items_server_test.go @@ -19,6 +19,8 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" publicv1 "github.com/osac-project/fulfillment-service/internal/api/osac/public/v1" @@ -126,8 +128,9 @@ var _ = Describe("Cluster catalog items server", func() { for i := range count { _, err := server.Create(ctx, publicv1.ClusterCatalogItemsCreateRequest_builder{ Object: publicv1.ClusterCatalogItem_builder{ - Title: fmt.Sprintf("Catalog item %d", i), - Template: "my-template-id", + Title: fmt.Sprintf("Catalog item %d", i), + Template: "my-template-id", + Published: true, }.Build(), }.Build()) Expect(err).ToNot(HaveOccurred()) @@ -144,8 +147,9 @@ var _ = Describe("Cluster catalog items server", func() { for i := range count { _, err := server.Create(ctx, publicv1.ClusterCatalogItemsCreateRequest_builder{ Object: publicv1.ClusterCatalogItem_builder{ - Title: fmt.Sprintf("Catalog item %d", i), - Template: "my-template-id", + Title: fmt.Sprintf("Catalog item %d", i), + Template: "my-template-id", + Published: true, }.Build(), }.Build()) Expect(err).ToNot(HaveOccurred()) @@ -164,8 +168,9 @@ var _ = Describe("Cluster catalog items server", func() { for i := range count { response, err := server.Create(ctx, publicv1.ClusterCatalogItemsCreateRequest_builder{ Object: publicv1.ClusterCatalogItem_builder{ - Title: fmt.Sprintf("Catalog item %d", i), - Template: "my-template-id", + Title: fmt.Sprintf("Catalog item %d", i), + Template: "my-template-id", + Published: true, }.Build(), }.Build()) Expect(err).ToNot(HaveOccurred()) @@ -182,12 +187,53 @@ var _ = Describe("Cluster catalog items server", func() { } }) + It("List excludes unpublished objects", func() { + _, err := server.Create(ctx, publicv1.ClusterCatalogItemsCreateRequest_builder{ + Object: publicv1.ClusterCatalogItem_builder{ + Title: "Published item", + Template: "my-template-id", + Published: true, + }.Build(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + + _, err = server.Create(ctx, publicv1.ClusterCatalogItemsCreateRequest_builder{ + Object: publicv1.ClusterCatalogItem_builder{ + Title: "Unpublished item", + Template: "my-template-id", + }.Build(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + + response, err := server.List(ctx, publicv1.ClusterCatalogItemsListRequest_builder{}.Build()) + Expect(err).ToNot(HaveOccurred()) + Expect(response.GetItems()).To(HaveLen(1)) + Expect(response.GetItems()[0].GetTitle()).To(Equal("Published item")) + }) + + It("Get returns not found for unpublished object", func() { + createResponse, err := server.Create(ctx, publicv1.ClusterCatalogItemsCreateRequest_builder{ + Object: publicv1.ClusterCatalogItem_builder{ + Title: "Unpublished item", + Template: "my-template-id", + }.Build(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + + _, err = server.Get(ctx, publicv1.ClusterCatalogItemsGetRequest_builder{ + Id: createResponse.GetObject().GetId(), + }.Build()) + Expect(err).To(HaveOccurred()) + Expect(status.Code(err)).To(Equal(codes.NotFound)) + }) + It("Get object", func() { createResponse, err := server.Create(ctx, publicv1.ClusterCatalogItemsCreateRequest_builder{ Object: publicv1.ClusterCatalogItem_builder{ Title: "My catalog item", Description: "My description.", Template: "my-template-id", + Published: true, }.Build(), }.Build()) Expect(err).ToNot(HaveOccurred()) @@ -205,6 +251,7 @@ var _ = Describe("Cluster catalog items server", func() { Title: "Original title", Description: "Original description.", Template: "my-template-id", + Published: true, }.Build(), }.Build()) Expect(err).ToNot(HaveOccurred()) @@ -216,6 +263,7 @@ var _ = Describe("Cluster catalog items server", func() { Title: "Updated title", Description: "Updated description.", Template: "my-template-id", + Published: true, }.Build(), }.Build()) Expect(err).ToNot(HaveOccurred()) @@ -233,8 +281,9 @@ var _ = Describe("Cluster catalog items server", func() { It("Delete object", func() { createResponse, err := server.Create(ctx, publicv1.ClusterCatalogItemsCreateRequest_builder{ Object: publicv1.ClusterCatalogItem_builder{ - Title: "My catalog item", - Template: "my-template-id", + Title: "My catalog item", + Template: "my-template-id", + Published: true, }.Build(), }.Build()) Expect(err).ToNot(HaveOccurred()) diff --git a/internal/servers/compute_instance_catalog_items_server.go b/internal/servers/compute_instance_catalog_items_server.go index 793fb3004..4f0538621 100644 --- a/internal/servers/compute_instance_catalog_items_server.go +++ b/internal/servers/compute_instance_catalog_items_server.go @@ -126,7 +126,7 @@ func (s *ComputeInstanceCatalogItemsServer) List(ctx context.Context, privateRequest := &privatev1.ComputeInstanceCatalogItemsListRequest{} privateRequest.SetOffset(request.GetOffset()) privateRequest.SetLimit(request.GetLimit()) - privateRequest.SetFilter(request.GetFilter()) + privateRequest.SetFilter(addPublishedFilter(request.GetFilter())) privateRequest.SetOrder(request.GetOrder()) privateResponse, err := s.delegate.List(ctx, privateRequest) @@ -163,6 +163,10 @@ func (s *ComputeInstanceCatalogItemsServer) Get(ctx context.Context, return nil, err } + if !privateResponse.GetObject().GetPublished() { + return nil, grpcstatus.Errorf(grpccodes.NotFound, "catalog item not found") + } + publicCatalogItem := &publicv1.ComputeInstanceCatalogItem{} err = s.outMapper.Copy(ctx, privateResponse.GetObject(), publicCatalogItem) if err != nil { diff --git a/internal/servers/compute_instance_catalog_items_server_test.go b/internal/servers/compute_instance_catalog_items_server_test.go index 018787d87..48c99bd60 100644 --- a/internal/servers/compute_instance_catalog_items_server_test.go +++ b/internal/servers/compute_instance_catalog_items_server_test.go @@ -19,6 +19,8 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" publicv1 "github.com/osac-project/fulfillment-service/internal/api/osac/public/v1" @@ -126,8 +128,9 @@ var _ = Describe("Compute instance catalog items server", func() { for i := range count { _, err := server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ Object: publicv1.ComputeInstanceCatalogItem_builder{ - Title: fmt.Sprintf("CI catalog item %d", i), - Template: "my-ci-template-id", + Title: fmt.Sprintf("CI catalog item %d", i), + Template: "my-ci-template-id", + Published: true, }.Build(), }.Build()) Expect(err).ToNot(HaveOccurred()) @@ -144,8 +147,9 @@ var _ = Describe("Compute instance catalog items server", func() { for i := range count { _, err := server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ Object: publicv1.ComputeInstanceCatalogItem_builder{ - Title: fmt.Sprintf("CI catalog item %d", i), - Template: "my-ci-template-id", + Title: fmt.Sprintf("CI catalog item %d", i), + Template: "my-ci-template-id", + Published: true, }.Build(), }.Build()) Expect(err).ToNot(HaveOccurred()) @@ -164,8 +168,9 @@ var _ = Describe("Compute instance catalog items server", func() { for i := range count { response, err := server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ Object: publicv1.ComputeInstanceCatalogItem_builder{ - Title: fmt.Sprintf("CI catalog item %d", i), - Template: "my-ci-template-id", + Title: fmt.Sprintf("CI catalog item %d", i), + Template: "my-ci-template-id", + Published: true, }.Build(), }.Build()) Expect(err).ToNot(HaveOccurred()) @@ -182,12 +187,53 @@ var _ = Describe("Compute instance catalog items server", func() { } }) + It("List excludes unpublished objects", func() { + _, err := server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ + Object: publicv1.ComputeInstanceCatalogItem_builder{ + Title: "Published item", + Template: "my-ci-template-id", + Published: true, + }.Build(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + + _, err = server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ + Object: publicv1.ComputeInstanceCatalogItem_builder{ + Title: "Unpublished item", + Template: "my-ci-template-id", + }.Build(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + + response, err := server.List(ctx, publicv1.ComputeInstanceCatalogItemsListRequest_builder{}.Build()) + Expect(err).ToNot(HaveOccurred()) + Expect(response.GetItems()).To(HaveLen(1)) + Expect(response.GetItems()[0].GetTitle()).To(Equal("Published item")) + }) + + It("Get returns not found for unpublished object", func() { + createResponse, err := server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ + Object: publicv1.ComputeInstanceCatalogItem_builder{ + Title: "Unpublished item", + Template: "my-ci-template-id", + }.Build(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + + _, err = server.Get(ctx, publicv1.ComputeInstanceCatalogItemsGetRequest_builder{ + Id: createResponse.GetObject().GetId(), + }.Build()) + Expect(err).To(HaveOccurred()) + Expect(status.Code(err)).To(Equal(codes.NotFound)) + }) + It("Get object", func() { createResponse, err := server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ Object: publicv1.ComputeInstanceCatalogItem_builder{ Title: "My CI catalog item", Description: "My description.", Template: "my-ci-template-id", + Published: true, }.Build(), }.Build()) Expect(err).ToNot(HaveOccurred()) @@ -205,6 +251,7 @@ var _ = Describe("Compute instance catalog items server", func() { Title: "Original title", Description: "Original description.", Template: "my-ci-template-id", + Published: true, }.Build(), }.Build()) Expect(err).ToNot(HaveOccurred()) @@ -216,6 +263,7 @@ var _ = Describe("Compute instance catalog items server", func() { Title: "Updated title", Description: "Updated description.", Template: "my-ci-template-id", + Published: true, }.Build(), }.Build()) Expect(err).ToNot(HaveOccurred()) @@ -233,8 +281,9 @@ var _ = Describe("Compute instance catalog items server", func() { It("Delete object", func() { createResponse, err := server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ Object: publicv1.ComputeInstanceCatalogItem_builder{ - Title: "My CI catalog item", - Template: "my-ci-template-id", + Title: "My CI catalog item", + Template: "my-ci-template-id", + Published: true, }.Build(), }.Build()) Expect(err).ToNot(HaveOccurred()) From 3c238d828715ad4fa9a5bb2985f35d581b350ac2 Mon Sep 17 00:00:00 2001 From: Elad Tabak Date: Wed, 27 May 2026 16:36:03 +0300 Subject: [PATCH 2/9] OSAC-58: address code review findings for unpublished filter - Add table-driven unit test for addPublishedFilter (both branches) - Add tests for List with user filter combined with unpublished items - Use explicit Published: false in unpublished test cases Generated with [Claude Code](https://claude.com/claude-code) --- .../servers/catalog_item_validation_test.go | 31 ++++++++++++ .../cluster_catalog_items_server_test.go | 49 +++++++++++++++++-- ...pute_instance_catalog_items_server_test.go | 44 ++++++++++++++++- 3 files changed, 118 insertions(+), 6 deletions(-) create mode 100644 internal/servers/catalog_item_validation_test.go diff --git a/internal/servers/catalog_item_validation_test.go b/internal/servers/catalog_item_validation_test.go new file mode 100644 index 000000000..86edc5169 --- /dev/null +++ b/internal/servers/catalog_item_validation_test.go @@ -0,0 +1,31 @@ +/* +Copyright (c) 2025 Red Hat Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the +License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific +language governing permissions and limitations under the License. +*/ + +package servers + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Catalog item validation", func() { + DescribeTable("addPublishedFilter", + func(input string, expected string) { + Expect(addPublishedFilter(input)).To(Equal(expected)) + }, + Entry("empty filter", "", "this.published == true"), + Entry("simple filter", "this.id == '123'", "(this.id == '123') && this.published == true"), + Entry("compound filter", "this.title == 'a' && this.template == 'b'", + "(this.title == 'a' && this.template == 'b') && this.published == true"), + ) +}) diff --git a/internal/servers/cluster_catalog_items_server_test.go b/internal/servers/cluster_catalog_items_server_test.go index 5a6e85e99..3880119ee 100644 --- a/internal/servers/cluster_catalog_items_server_test.go +++ b/internal/servers/cluster_catalog_items_server_test.go @@ -199,8 +199,9 @@ var _ = Describe("Cluster catalog items server", func() { _, err = server.Create(ctx, publicv1.ClusterCatalogItemsCreateRequest_builder{ Object: publicv1.ClusterCatalogItem_builder{ - Title: "Unpublished item", - Template: "my-template-id", + Title: "Unpublished item", + Template: "my-template-id", + Published: false, }.Build(), }.Build()) Expect(err).ToNot(HaveOccurred()) @@ -211,11 +212,51 @@ var _ = Describe("Cluster catalog items server", func() { Expect(response.GetItems()[0].GetTitle()).To(Equal("Published item")) }) + It("List with user filter excludes unpublished objects", func() { + publishedResponse, err := server.Create(ctx, publicv1.ClusterCatalogItemsCreateRequest_builder{ + Object: publicv1.ClusterCatalogItem_builder{ + Title: "Target published", + Template: "my-template-id", + Published: true, + }.Build(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + + _, err = server.Create(ctx, publicv1.ClusterCatalogItemsCreateRequest_builder{ + Object: publicv1.ClusterCatalogItem_builder{ + Title: "Other published", + Template: "my-template-id", + Published: true, + }.Build(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + + unpublishedResponse, err := server.Create(ctx, publicv1.ClusterCatalogItemsCreateRequest_builder{ + Object: publicv1.ClusterCatalogItem_builder{ + Title: "Target unpublished", + Template: "my-template-id", + Published: false, + }.Build(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + + targetID := publishedResponse.GetObject().GetId() + unpublishedID := unpublishedResponse.GetObject().GetId() + filter := fmt.Sprintf("this.id == '%s' || this.id == '%s'", targetID, unpublishedID) + response, err := server.List(ctx, publicv1.ClusterCatalogItemsListRequest_builder{ + Filter: proto.String(filter), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + Expect(response.GetItems()).To(HaveLen(1)) + Expect(response.GetItems()[0].GetId()).To(Equal(targetID)) + }) + It("Get returns not found for unpublished object", func() { createResponse, err := server.Create(ctx, publicv1.ClusterCatalogItemsCreateRequest_builder{ Object: publicv1.ClusterCatalogItem_builder{ - Title: "Unpublished item", - Template: "my-template-id", + Title: "Unpublished item", + Template: "my-template-id", + Published: false, }.Build(), }.Build()) Expect(err).ToNot(HaveOccurred()) diff --git a/internal/servers/compute_instance_catalog_items_server_test.go b/internal/servers/compute_instance_catalog_items_server_test.go index 48c99bd60..ffbce469a 100644 --- a/internal/servers/compute_instance_catalog_items_server_test.go +++ b/internal/servers/compute_instance_catalog_items_server_test.go @@ -199,8 +199,9 @@ var _ = Describe("Compute instance catalog items server", func() { _, err = server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ Object: publicv1.ComputeInstanceCatalogItem_builder{ - Title: "Unpublished item", - Template: "my-ci-template-id", + Title: "Unpublished item", + Template: "my-ci-template-id", + Published: false, }.Build(), }.Build()) Expect(err).ToNot(HaveOccurred()) @@ -211,6 +212,45 @@ var _ = Describe("Compute instance catalog items server", func() { Expect(response.GetItems()[0].GetTitle()).To(Equal("Published item")) }) + It("List with user filter excludes unpublished objects", func() { + publishedResponse, err := server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ + Object: publicv1.ComputeInstanceCatalogItem_builder{ + Title: "Target published", + Template: "my-ci-template-id", + Published: true, + }.Build(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + + _, err = server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ + Object: publicv1.ComputeInstanceCatalogItem_builder{ + Title: "Other published", + Template: "my-ci-template-id", + Published: true, + }.Build(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + + unpublishedResponse, err := server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ + Object: publicv1.ComputeInstanceCatalogItem_builder{ + Title: "Target unpublished", + Template: "my-ci-template-id", + Published: false, + }.Build(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + + targetID := publishedResponse.GetObject().GetId() + unpublishedID := unpublishedResponse.GetObject().GetId() + filter := fmt.Sprintf("this.id == '%s' || this.id == '%s'", targetID, unpublishedID) + response, err := server.List(ctx, publicv1.ComputeInstanceCatalogItemsListRequest_builder{ + Filter: proto.String(filter), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + Expect(response.GetItems()).To(HaveLen(1)) + Expect(response.GetItems()[0].GetId()).To(Equal(targetID)) + }) + It("Get returns not found for unpublished object", func() { createResponse, err := server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ Object: publicv1.ComputeInstanceCatalogItem_builder{ From 943c831ad6859f8e3dd5fce70a4c9959e737fe9e Mon Sep 17 00:00:00 2001 From: Elad Tabak Date: Wed, 27 May 2026 17:38:13 +0300 Subject: [PATCH 3/9] OSAC-58: set Published: false explicitly in unpublished test case Match the cluster catalog item test by explicitly setting Published: false instead of relying on proto zero-value default. Generated with [Claude Code](https://claude.com/claude-code) --- .../servers/compute_instance_catalog_items_server_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/servers/compute_instance_catalog_items_server_test.go b/internal/servers/compute_instance_catalog_items_server_test.go index ffbce469a..d1af6a10c 100644 --- a/internal/servers/compute_instance_catalog_items_server_test.go +++ b/internal/servers/compute_instance_catalog_items_server_test.go @@ -254,8 +254,9 @@ var _ = Describe("Compute instance catalog items server", func() { It("Get returns not found for unpublished object", func() { createResponse, err := server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ Object: publicv1.ComputeInstanceCatalogItem_builder{ - Title: "Unpublished item", - Template: "my-ci-template-id", + Title: "Unpublished item", + Template: "my-ci-template-id", + Published: false, }.Build(), }.Build()) Expect(err).ToNot(HaveOccurred()) From 403b091242b42373be408965ef31b27c4772163f Mon Sep 17 00:00:00 2001 From: Elad Tabak Date: Thu, 28 May 2026 10:54:09 +0300 Subject: [PATCH 4/9] OSAC-58: address Juan's review findings for published filter - Simplify filter from `this.published == true` to `this.published` - Move addPublishedFilter from package-level function to methods on ClusterCatalogItemsServer and ComputeInstanceCatalogItemsServer to avoid naming clashes in the servers package - Fix CEL filter bypass: validate user filter is a syntactically valid CEL expression before composing, preventing injection like `true) || (true` from breaking out of parenthesized composition Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Elad Tabak --- internal/servers/catalog_item_validation.go | 28 ++++++++++------- .../servers/catalog_item_validation_test.go | 30 +++++++++++++++---- .../servers/cluster_catalog_items_server.go | 16 +++++++++- .../compute_instance_catalog_items_server.go | 16 +++++++++- 4 files changed, 71 insertions(+), 19 deletions(-) diff --git a/internal/servers/catalog_item_validation.go b/internal/servers/catalog_item_validation.go index 7b264141f..3356b81ad 100644 --- a/internal/servers/catalog_item_validation.go +++ b/internal/servers/catalog_item_validation.go @@ -15,8 +15,10 @@ package servers import ( "encoding/json" + "fmt" "strings" + "github.com/google/cel-go/cel" "github.com/santhosh-tekuri/jsonschema/v6" grpccodes "google.golang.org/grpc/codes" grpcstatus "google.golang.org/grpc/status" @@ -27,6 +29,21 @@ import ( privatev1 "github.com/osac-project/fulfillment-service/internal/api/osac/private/v1" ) +// validateCELSyntax checks that a filter string is a syntactically valid, complete CEL expression. +// This prevents filter bypass attacks where a malicious filter like "true) || (true" could +// break out of parenthesized composition and change operator precedence. +func validateCELSyntax(filter string) error { + env, err := cel.NewEnv() + if err != nil { + return fmt.Errorf("failed to create CEL environment: %w", err) + } + _, issues := env.Parse(filter) + if issues != nil && issues.Err() != nil { + return fmt.Errorf("syntax error: %w", issues.Err()) + } + return nil +} + // catalogItem is implemented by both ClusterCatalogItem and ComputeInstanceCatalogItem. type catalogItem interface { proto.Message @@ -36,17 +53,6 @@ type catalogItem interface { GetMetadata() *privatev1.Metadata } -const publishedFilter = "this.published == true" - -// addPublishedFilter appends a CEL clause that restricts results to published catalog items. -// Used by public servers to hide unpublished items from end users. -func addPublishedFilter(filter string) string { - if filter == "" { - return publishedFilter - } - return "(" + filter + ") && " + publishedFilter -} - // applyFieldDefinitions processes field definitions from a catalog item against a resource spec. // For non-editable fields: overrides user-provided values with the catalog item default. // For editable fields with user values: validates against the JSON Schema. diff --git a/internal/servers/catalog_item_validation_test.go b/internal/servers/catalog_item_validation_test.go index 86edc5169..9e70f560c 100644 --- a/internal/servers/catalog_item_validation_test.go +++ b/internal/servers/catalog_item_validation_test.go @@ -18,14 +18,32 @@ import ( . "github.com/onsi/gomega" ) -var _ = Describe("Catalog item validation", func() { - DescribeTable("addPublishedFilter", +var _ = Describe("addPublishedFilter", func() { + var server *ClusterCatalogItemsServer + + BeforeEach(func() { + server = &ClusterCatalogItemsServer{} + }) + + DescribeTable("composes filter correctly", func(input string, expected string) { - Expect(addPublishedFilter(input)).To(Equal(expected)) + result, err := server.addPublishedFilter(input) + Expect(err).ToNot(HaveOccurred()) + Expect(result).To(Equal(expected)) }, - Entry("empty filter", "", "this.published == true"), - Entry("simple filter", "this.id == '123'", "(this.id == '123') && this.published == true"), + Entry("empty filter", "", "this.published"), + Entry("simple filter", "this.id == '123'", "(this.id == '123') && this.published"), Entry("compound filter", "this.title == 'a' && this.template == 'b'", - "(this.title == 'a' && this.template == 'b') && this.published == true"), + "(this.title == 'a' && this.template == 'b') && this.published"), + ) + + DescribeTable("rejects malformed filters", + func(input string) { + _, err := server.addPublishedFilter(input) + Expect(err).To(HaveOccurred()) + }, + Entry("unbalanced parens to bypass published", `true) || (true`), + Entry("unbalanced closing paren", `true)`), + Entry("unbalanced opening paren", `(true`), ) }) diff --git a/internal/servers/cluster_catalog_items_server.go b/internal/servers/cluster_catalog_items_server.go index 3f3744045..6316210a6 100644 --- a/internal/servers/cluster_catalog_items_server.go +++ b/internal/servers/cluster_catalog_items_server.go @@ -126,7 +126,11 @@ func (s *ClusterCatalogItemsServer) List(ctx context.Context, privateRequest := &privatev1.ClusterCatalogItemsListRequest{} privateRequest.SetOffset(request.GetOffset()) privateRequest.SetLimit(request.GetLimit()) - privateRequest.SetFilter(addPublishedFilter(request.GetFilter())) + composedFilter, err := s.addPublishedFilter(request.GetFilter()) + if err != nil { + return nil, err + } + privateRequest.SetFilter(composedFilter) privateRequest.SetOrder(request.GetOrder()) privateResponse, err := s.delegate.List(ctx, privateRequest) @@ -263,6 +267,16 @@ func (s *ClusterCatalogItemsServer) Update(ctx context.Context, return } +func (s *ClusterCatalogItemsServer) addPublishedFilter(filter string) (string, error) { + if filter == "" { + return "this.published", nil + } + if err := validateCELSyntax(filter); err != nil { + return "", grpcstatus.Errorf(grpccodes.InvalidArgument, "invalid filter: %v", err) + } + return "(" + filter + ") && this.published", nil +} + func (s *ClusterCatalogItemsServer) Delete(ctx context.Context, request *publicv1.ClusterCatalogItemsDeleteRequest) (response *publicv1.ClusterCatalogItemsDeleteResponse, err error) { privateRequest := &privatev1.ClusterCatalogItemsDeleteRequest{} diff --git a/internal/servers/compute_instance_catalog_items_server.go b/internal/servers/compute_instance_catalog_items_server.go index 4f0538621..57b27960e 100644 --- a/internal/servers/compute_instance_catalog_items_server.go +++ b/internal/servers/compute_instance_catalog_items_server.go @@ -126,7 +126,11 @@ func (s *ComputeInstanceCatalogItemsServer) List(ctx context.Context, privateRequest := &privatev1.ComputeInstanceCatalogItemsListRequest{} privateRequest.SetOffset(request.GetOffset()) privateRequest.SetLimit(request.GetLimit()) - privateRequest.SetFilter(addPublishedFilter(request.GetFilter())) + composedFilter, err := s.addPublishedFilter(request.GetFilter()) + if err != nil { + return nil, err + } + privateRequest.SetFilter(composedFilter) privateRequest.SetOrder(request.GetOrder()) privateResponse, err := s.delegate.List(ctx, privateRequest) @@ -263,6 +267,16 @@ func (s *ComputeInstanceCatalogItemsServer) Update(ctx context.Context, return } +func (s *ComputeInstanceCatalogItemsServer) addPublishedFilter(filter string) (string, error) { + if filter == "" { + return "this.published", nil + } + if err := validateCELSyntax(filter); err != nil { + return "", grpcstatus.Errorf(grpccodes.InvalidArgument, "invalid filter: %v", err) + } + return "(" + filter + ") && this.published", nil +} + func (s *ComputeInstanceCatalogItemsServer) Delete(ctx context.Context, request *publicv1.ComputeInstanceCatalogItemsDeleteRequest) (response *publicv1.ComputeInstanceCatalogItemsDeleteResponse, err error) { privateRequest := &privatev1.ComputeInstanceCatalogItemsDeleteRequest{} From adeabc32fef2fd5f796baad095017f4935992da9 Mon Sep 17 00:00:00 2001 From: Elad Tabak Date: Thu, 28 May 2026 11:04:29 +0300 Subject: [PATCH 5/9] OSAC-58: cache CEL env and add OR composition test - Use sync.Once to initialize CEL env once instead of per-call - Add test case for valid filter with OR to confirm safe composition Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Elad Tabak --- internal/servers/catalog_item_validation.go | 17 +++++++++++++---- .../servers/catalog_item_validation_test.go | 2 ++ 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/internal/servers/catalog_item_validation.go b/internal/servers/catalog_item_validation.go index 3356b81ad..92a98d185 100644 --- a/internal/servers/catalog_item_validation.go +++ b/internal/servers/catalog_item_validation.go @@ -17,6 +17,7 @@ import ( "encoding/json" "fmt" "strings" + "sync" "github.com/google/cel-go/cel" "github.com/santhosh-tekuri/jsonschema/v6" @@ -29,15 +30,23 @@ import ( privatev1 "github.com/osac-project/fulfillment-service/internal/api/osac/private/v1" ) +var ( + celSyntaxEnv *cel.Env + celSyntaxEnvOnce sync.Once + celSyntaxEnvErr error +) + // validateCELSyntax checks that a filter string is a syntactically valid, complete CEL expression. // This prevents filter bypass attacks where a malicious filter like "true) || (true" could // break out of parenthesized composition and change operator precedence. func validateCELSyntax(filter string) error { - env, err := cel.NewEnv() - if err != nil { - return fmt.Errorf("failed to create CEL environment: %w", err) + celSyntaxEnvOnce.Do(func() { + celSyntaxEnv, celSyntaxEnvErr = cel.NewEnv() + }) + if celSyntaxEnvErr != nil { + return fmt.Errorf("failed to create CEL environment: %w", celSyntaxEnvErr) } - _, issues := env.Parse(filter) + _, issues := celSyntaxEnv.Parse(filter) if issues != nil && issues.Err() != nil { return fmt.Errorf("syntax error: %w", issues.Err()) } diff --git a/internal/servers/catalog_item_validation_test.go b/internal/servers/catalog_item_validation_test.go index 9e70f560c..f80914727 100644 --- a/internal/servers/catalog_item_validation_test.go +++ b/internal/servers/catalog_item_validation_test.go @@ -35,6 +35,8 @@ var _ = Describe("addPublishedFilter", func() { Entry("simple filter", "this.id == '123'", "(this.id == '123') && this.published"), Entry("compound filter", "this.title == 'a' && this.template == 'b'", "(this.title == 'a' && this.template == 'b') && this.published"), + Entry("valid filter with OR is safely composed", "true || true", + "(true || true) && this.published"), ) DescribeTable("rejects malformed filters", From 035ed85fe15dacce91fcd07f1d5736da5264f16e Mon Sep 17 00:00:00 2001 From: Elad Tabak Date: Thu, 28 May 2026 11:37:42 +0300 Subject: [PATCH 6/9] OSAC-58: allow creators to see their own unpublished catalog items Published filter now uses creator-based visibility: unpublished items are visible to the user who created them via both List and Get. List filter: (this.published || this.metadata.creator == '') Get: allow access if published or caller is the creator. This ensures tenant admins who create catalog items through the public API retain visibility after setting published to false. Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Elad Tabak --- internal/servers/catalog_item_validation.go | 14 ++++ .../servers/catalog_item_validation_test.go | 28 ++++++-- .../servers/cluster_catalog_items_server.go | 12 ++-- .../cluster_catalog_items_server_test.go | 72 +++++++++++++++++-- .../compute_instance_catalog_items_server.go | 12 ++-- ...pute_instance_catalog_items_server_test.go | 72 +++++++++++++++++-- 6 files changed, 179 insertions(+), 31 deletions(-) diff --git a/internal/servers/catalog_item_validation.go b/internal/servers/catalog_item_validation.go index 92a98d185..3aa655d4c 100644 --- a/internal/servers/catalog_item_validation.go +++ b/internal/servers/catalog_item_validation.go @@ -14,6 +14,7 @@ language governing permissions and limitations under the License. package servers import ( + "context" "encoding/json" "fmt" "strings" @@ -28,6 +29,7 @@ import ( "google.golang.org/protobuf/types/known/structpb" privatev1 "github.com/osac-project/fulfillment-service/internal/api/osac/private/v1" + "github.com/osac-project/fulfillment-service/internal/auth" ) var ( @@ -53,6 +55,18 @@ func validateCELSyntax(filter string) error { return nil } +// buildPublishedClause returns a CEL clause that shows published items to everyone +// and unpublished items only to the user who created them. +func buildPublishedClause(ctx context.Context) string { + user := auth.SubjectFromContext(ctx).User + return fmt.Sprintf("(this.published || this.metadata.creator == '%s')", user) +} + +// isCreator checks whether the current user created the given item. +func isCreator(ctx context.Context, itemCreator string) bool { + return itemCreator != "" && auth.SubjectFromContext(ctx).User == itemCreator +} + // catalogItem is implemented by both ClusterCatalogItem and ComputeInstanceCatalogItem. type catalogItem interface { proto.Message diff --git a/internal/servers/catalog_item_validation_test.go b/internal/servers/catalog_item_validation_test.go index f80914727..4af4c1de6 100644 --- a/internal/servers/catalog_item_validation_test.go +++ b/internal/servers/catalog_item_validation_test.go @@ -14,34 +14,48 @@ language governing permissions and limitations under the License. package servers import ( + "context" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + + "github.com/osac-project/fulfillment-service/internal/auth" + "github.com/osac-project/fulfillment-service/internal/collections" ) var _ = Describe("addPublishedFilter", func() { - var server *ClusterCatalogItemsServer + var ( + server *ClusterCatalogItemsServer + ctx context.Context + ) BeforeEach(func() { server = &ClusterCatalogItemsServer{} + ctx = auth.ContextWithSubject(context.Background(), &auth.Subject{ + User: "test-admin", + Tenants: collections.NewSet("my-tenant"), + }) }) DescribeTable("composes filter correctly", func(input string, expected string) { - result, err := server.addPublishedFilter(input) + result, err := server.addPublishedFilter(ctx, input) Expect(err).ToNot(HaveOccurred()) Expect(result).To(Equal(expected)) }, - Entry("empty filter", "", "this.published"), - Entry("simple filter", "this.id == '123'", "(this.id == '123') && this.published"), + Entry("empty filter", "", + "(this.published || this.metadata.creator == 'test-admin')"), + Entry("simple filter", "this.id == '123'", + "(this.id == '123') && (this.published || this.metadata.creator == 'test-admin')"), Entry("compound filter", "this.title == 'a' && this.template == 'b'", - "(this.title == 'a' && this.template == 'b') && this.published"), + "(this.title == 'a' && this.template == 'b') && (this.published || this.metadata.creator == 'test-admin')"), Entry("valid filter with OR is safely composed", "true || true", - "(true || true) && this.published"), + "(true || true) && (this.published || this.metadata.creator == 'test-admin')"), ) DescribeTable("rejects malformed filters", func(input string) { - _, err := server.addPublishedFilter(input) + _, err := server.addPublishedFilter(ctx, input) Expect(err).To(HaveOccurred()) }, Entry("unbalanced parens to bypass published", `true) || (true`), diff --git a/internal/servers/cluster_catalog_items_server.go b/internal/servers/cluster_catalog_items_server.go index 6316210a6..d98c4f3a9 100644 --- a/internal/servers/cluster_catalog_items_server.go +++ b/internal/servers/cluster_catalog_items_server.go @@ -126,7 +126,7 @@ func (s *ClusterCatalogItemsServer) List(ctx context.Context, privateRequest := &privatev1.ClusterCatalogItemsListRequest{} privateRequest.SetOffset(request.GetOffset()) privateRequest.SetLimit(request.GetLimit()) - composedFilter, err := s.addPublishedFilter(request.GetFilter()) + composedFilter, err := s.addPublishedFilter(ctx, request.GetFilter()) if err != nil { return nil, err } @@ -167,7 +167,8 @@ func (s *ClusterCatalogItemsServer) Get(ctx context.Context, return nil, err } - if !privateResponse.GetObject().GetPublished() { + obj := privateResponse.GetObject() + if !obj.GetPublished() && !isCreator(ctx, obj.GetMetadata().GetCreator()) { return nil, grpcstatus.Errorf(grpccodes.NotFound, "catalog item not found") } @@ -267,14 +268,15 @@ func (s *ClusterCatalogItemsServer) Update(ctx context.Context, return } -func (s *ClusterCatalogItemsServer) addPublishedFilter(filter string) (string, error) { +func (s *ClusterCatalogItemsServer) addPublishedFilter(ctx context.Context, filter string) (string, error) { + publishedClause := buildPublishedClause(ctx) if filter == "" { - return "this.published", nil + return publishedClause, nil } if err := validateCELSyntax(filter); err != nil { return "", grpcstatus.Errorf(grpccodes.InvalidArgument, "invalid filter: %v", err) } - return "(" + filter + ") && this.published", nil + return "(" + filter + ") && " + publishedClause, nil } func (s *ClusterCatalogItemsServer) Delete(ctx context.Context, diff --git a/internal/servers/cluster_catalog_items_server_test.go b/internal/servers/cluster_catalog_items_server_test.go index 3880119ee..f452a5670 100644 --- a/internal/servers/cluster_catalog_items_server_test.go +++ b/internal/servers/cluster_catalog_items_server_test.go @@ -24,6 +24,8 @@ import ( "google.golang.org/protobuf/proto" publicv1 "github.com/osac-project/fulfillment-service/internal/api/osac/public/v1" + "github.com/osac-project/fulfillment-service/internal/auth" + "github.com/osac-project/fulfillment-service/internal/collections" "github.com/osac-project/fulfillment-service/internal/database" ) @@ -36,7 +38,10 @@ var _ = Describe("Cluster catalog items server", func() { BeforeEach(func() { var err error - ctx = context.Background() + ctx = auth.ContextWithSubject(context.Background(), &auth.Subject{ + User: "system", + Tenants: collections.NewUniversalSet[string](), + }) db, err := server.NewInstance().Build() Expect(err).ToNot(HaveOccurred()) @@ -187,7 +192,7 @@ var _ = Describe("Cluster catalog items server", func() { } }) - It("List excludes unpublished objects", func() { + It("List excludes unpublished objects from other creators", func() { _, err := server.Create(ctx, publicv1.ClusterCatalogItemsCreateRequest_builder{ Object: publicv1.ClusterCatalogItem_builder{ Title: "Published item", @@ -206,13 +211,41 @@ var _ = Describe("Cluster catalog items server", func() { }.Build()) Expect(err).ToNot(HaveOccurred()) - response, err := server.List(ctx, publicv1.ClusterCatalogItemsListRequest_builder{}.Build()) + otherUserCtx := auth.ContextWithSubject(ctx, &auth.Subject{ + User: "other-user", + Tenants: collections.NewUniversalSet[string](), + }) + response, err := server.List(otherUserCtx, publicv1.ClusterCatalogItemsListRequest_builder{}.Build()) Expect(err).ToNot(HaveOccurred()) Expect(response.GetItems()).To(HaveLen(1)) Expect(response.GetItems()[0].GetTitle()).To(Equal("Published item")) }) - It("List with user filter excludes unpublished objects", func() { + It("List shows unpublished objects to their creator", func() { + _, err := server.Create(ctx, publicv1.ClusterCatalogItemsCreateRequest_builder{ + Object: publicv1.ClusterCatalogItem_builder{ + Title: "Published item", + Template: "my-template-id", + Published: true, + }.Build(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + + _, err = server.Create(ctx, publicv1.ClusterCatalogItemsCreateRequest_builder{ + Object: publicv1.ClusterCatalogItem_builder{ + Title: "Unpublished item", + Template: "my-template-id", + Published: false, + }.Build(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + + response, err := server.List(ctx, publicv1.ClusterCatalogItemsListRequest_builder{}.Build()) + Expect(err).ToNot(HaveOccurred()) + Expect(response.GetItems()).To(HaveLen(2)) + }) + + It("List with user filter excludes unpublished objects from other creators", func() { publishedResponse, err := server.Create(ctx, publicv1.ClusterCatalogItemsCreateRequest_builder{ Object: publicv1.ClusterCatalogItem_builder{ Title: "Target published", @@ -243,7 +276,11 @@ var _ = Describe("Cluster catalog items server", func() { targetID := publishedResponse.GetObject().GetId() unpublishedID := unpublishedResponse.GetObject().GetId() filter := fmt.Sprintf("this.id == '%s' || this.id == '%s'", targetID, unpublishedID) - response, err := server.List(ctx, publicv1.ClusterCatalogItemsListRequest_builder{ + otherUserCtx := auth.ContextWithSubject(ctx, &auth.Subject{ + User: "other-user", + Tenants: collections.NewUniversalSet[string](), + }) + response, err := server.List(otherUserCtx, publicv1.ClusterCatalogItemsListRequest_builder{ Filter: proto.String(filter), }.Build()) Expect(err).ToNot(HaveOccurred()) @@ -251,7 +288,7 @@ var _ = Describe("Cluster catalog items server", func() { Expect(response.GetItems()[0].GetId()).To(Equal(targetID)) }) - It("Get returns not found for unpublished object", func() { + It("Get returns not found for unpublished object from other creator", func() { createResponse, err := server.Create(ctx, publicv1.ClusterCatalogItemsCreateRequest_builder{ Object: publicv1.ClusterCatalogItem_builder{ Title: "Unpublished item", @@ -261,13 +298,34 @@ var _ = Describe("Cluster catalog items server", func() { }.Build()) Expect(err).ToNot(HaveOccurred()) - _, err = server.Get(ctx, publicv1.ClusterCatalogItemsGetRequest_builder{ + otherUserCtx := auth.ContextWithSubject(ctx, &auth.Subject{ + User: "other-user", + Tenants: collections.NewUniversalSet[string](), + }) + _, err = server.Get(otherUserCtx, publicv1.ClusterCatalogItemsGetRequest_builder{ Id: createResponse.GetObject().GetId(), }.Build()) Expect(err).To(HaveOccurred()) Expect(status.Code(err)).To(Equal(codes.NotFound)) }) + It("Get returns unpublished object to its creator", func() { + createResponse, err := server.Create(ctx, publicv1.ClusterCatalogItemsCreateRequest_builder{ + Object: publicv1.ClusterCatalogItem_builder{ + Title: "Unpublished item", + Template: "my-template-id", + Published: false, + }.Build(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + + getResponse, err := server.Get(ctx, publicv1.ClusterCatalogItemsGetRequest_builder{ + Id: createResponse.GetObject().GetId(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + Expect(getResponse.GetObject().GetTitle()).To(Equal("Unpublished item")) + }) + It("Get object", func() { createResponse, err := server.Create(ctx, publicv1.ClusterCatalogItemsCreateRequest_builder{ Object: publicv1.ClusterCatalogItem_builder{ diff --git a/internal/servers/compute_instance_catalog_items_server.go b/internal/servers/compute_instance_catalog_items_server.go index 57b27960e..9b994a354 100644 --- a/internal/servers/compute_instance_catalog_items_server.go +++ b/internal/servers/compute_instance_catalog_items_server.go @@ -126,7 +126,7 @@ func (s *ComputeInstanceCatalogItemsServer) List(ctx context.Context, privateRequest := &privatev1.ComputeInstanceCatalogItemsListRequest{} privateRequest.SetOffset(request.GetOffset()) privateRequest.SetLimit(request.GetLimit()) - composedFilter, err := s.addPublishedFilter(request.GetFilter()) + composedFilter, err := s.addPublishedFilter(ctx, request.GetFilter()) if err != nil { return nil, err } @@ -167,7 +167,8 @@ func (s *ComputeInstanceCatalogItemsServer) Get(ctx context.Context, return nil, err } - if !privateResponse.GetObject().GetPublished() { + obj := privateResponse.GetObject() + if !obj.GetPublished() && !isCreator(ctx, obj.GetMetadata().GetCreator()) { return nil, grpcstatus.Errorf(grpccodes.NotFound, "catalog item not found") } @@ -267,14 +268,15 @@ func (s *ComputeInstanceCatalogItemsServer) Update(ctx context.Context, return } -func (s *ComputeInstanceCatalogItemsServer) addPublishedFilter(filter string) (string, error) { +func (s *ComputeInstanceCatalogItemsServer) addPublishedFilter(ctx context.Context, filter string) (string, error) { + publishedClause := buildPublishedClause(ctx) if filter == "" { - return "this.published", nil + return publishedClause, nil } if err := validateCELSyntax(filter); err != nil { return "", grpcstatus.Errorf(grpccodes.InvalidArgument, "invalid filter: %v", err) } - return "(" + filter + ") && this.published", nil + return "(" + filter + ") && " + publishedClause, nil } func (s *ComputeInstanceCatalogItemsServer) Delete(ctx context.Context, diff --git a/internal/servers/compute_instance_catalog_items_server_test.go b/internal/servers/compute_instance_catalog_items_server_test.go index d1af6a10c..973cd3257 100644 --- a/internal/servers/compute_instance_catalog_items_server_test.go +++ b/internal/servers/compute_instance_catalog_items_server_test.go @@ -24,6 +24,8 @@ import ( "google.golang.org/protobuf/proto" publicv1 "github.com/osac-project/fulfillment-service/internal/api/osac/public/v1" + "github.com/osac-project/fulfillment-service/internal/auth" + "github.com/osac-project/fulfillment-service/internal/collections" "github.com/osac-project/fulfillment-service/internal/database" ) @@ -36,7 +38,10 @@ var _ = Describe("Compute instance catalog items server", func() { BeforeEach(func() { var err error - ctx = context.Background() + ctx = auth.ContextWithSubject(context.Background(), &auth.Subject{ + User: "system", + Tenants: collections.NewUniversalSet[string](), + }) db, err := server.NewInstance().Build() Expect(err).ToNot(HaveOccurred()) @@ -187,7 +192,7 @@ var _ = Describe("Compute instance catalog items server", func() { } }) - It("List excludes unpublished objects", func() { + It("List excludes unpublished objects from other creators", func() { _, err := server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ Object: publicv1.ComputeInstanceCatalogItem_builder{ Title: "Published item", @@ -206,13 +211,41 @@ var _ = Describe("Compute instance catalog items server", func() { }.Build()) Expect(err).ToNot(HaveOccurred()) - response, err := server.List(ctx, publicv1.ComputeInstanceCatalogItemsListRequest_builder{}.Build()) + otherUserCtx := auth.ContextWithSubject(ctx, &auth.Subject{ + User: "other-user", + Tenants: collections.NewUniversalSet[string](), + }) + response, err := server.List(otherUserCtx, publicv1.ComputeInstanceCatalogItemsListRequest_builder{}.Build()) Expect(err).ToNot(HaveOccurred()) Expect(response.GetItems()).To(HaveLen(1)) Expect(response.GetItems()[0].GetTitle()).To(Equal("Published item")) }) - It("List with user filter excludes unpublished objects", func() { + It("List shows unpublished objects to their creator", func() { + _, err := server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ + Object: publicv1.ComputeInstanceCatalogItem_builder{ + Title: "Published item", + Template: "my-ci-template-id", + Published: true, + }.Build(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + + _, err = server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ + Object: publicv1.ComputeInstanceCatalogItem_builder{ + Title: "Unpublished item", + Template: "my-ci-template-id", + Published: false, + }.Build(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + + response, err := server.List(ctx, publicv1.ComputeInstanceCatalogItemsListRequest_builder{}.Build()) + Expect(err).ToNot(HaveOccurred()) + Expect(response.GetItems()).To(HaveLen(2)) + }) + + It("List with user filter excludes unpublished objects from other creators", func() { publishedResponse, err := server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ Object: publicv1.ComputeInstanceCatalogItem_builder{ Title: "Target published", @@ -243,7 +276,11 @@ var _ = Describe("Compute instance catalog items server", func() { targetID := publishedResponse.GetObject().GetId() unpublishedID := unpublishedResponse.GetObject().GetId() filter := fmt.Sprintf("this.id == '%s' || this.id == '%s'", targetID, unpublishedID) - response, err := server.List(ctx, publicv1.ComputeInstanceCatalogItemsListRequest_builder{ + otherUserCtx := auth.ContextWithSubject(ctx, &auth.Subject{ + User: "other-user", + Tenants: collections.NewUniversalSet[string](), + }) + response, err := server.List(otherUserCtx, publicv1.ComputeInstanceCatalogItemsListRequest_builder{ Filter: proto.String(filter), }.Build()) Expect(err).ToNot(HaveOccurred()) @@ -251,7 +288,7 @@ var _ = Describe("Compute instance catalog items server", func() { Expect(response.GetItems()[0].GetId()).To(Equal(targetID)) }) - It("Get returns not found for unpublished object", func() { + It("Get returns not found for unpublished object from other creator", func() { createResponse, err := server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ Object: publicv1.ComputeInstanceCatalogItem_builder{ Title: "Unpublished item", @@ -261,13 +298,34 @@ var _ = Describe("Compute instance catalog items server", func() { }.Build()) Expect(err).ToNot(HaveOccurred()) - _, err = server.Get(ctx, publicv1.ComputeInstanceCatalogItemsGetRequest_builder{ + otherUserCtx := auth.ContextWithSubject(ctx, &auth.Subject{ + User: "other-user", + Tenants: collections.NewUniversalSet[string](), + }) + _, err = server.Get(otherUserCtx, publicv1.ComputeInstanceCatalogItemsGetRequest_builder{ Id: createResponse.GetObject().GetId(), }.Build()) Expect(err).To(HaveOccurred()) Expect(status.Code(err)).To(Equal(codes.NotFound)) }) + It("Get returns unpublished object to its creator", func() { + createResponse, err := server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ + Object: publicv1.ComputeInstanceCatalogItem_builder{ + Title: "Unpublished item", + Template: "my-ci-template-id", + Published: false, + }.Build(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + + getResponse, err := server.Get(ctx, publicv1.ComputeInstanceCatalogItemsGetRequest_builder{ + Id: createResponse.GetObject().GetId(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + Expect(getResponse.GetObject().GetTitle()).To(Equal("Unpublished item")) + }) + It("Get object", func() { createResponse, err := server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ Object: publicv1.ComputeInstanceCatalogItem_builder{ From 784ae8032466c1f8c605e9317f48221403a531f3 Mon Sep 17 00:00:00 2001 From: Elad Tabak Date: Thu, 28 May 2026 14:42:39 +0300 Subject: [PATCH 7/9] OSAC-58: revert creator-based visibility, add direct validateCELSyntax tests Revert creator-based unpublished item visibility per reviewer feedback: unpublished items should be managed through the upcoming authorization and projects system instead of CEL filter composition. The published filter is now the simple `this.published` for all users. Also add direct unit tests for validateCELSyntax and verify malformed filter tests return InvalidArgument status code. Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Elad Tabak --- internal/servers/catalog_item_validation.go | 14 ---- .../servers/catalog_item_validation_test.go | 50 +++++++------ .../servers/cluster_catalog_items_server.go | 12 ++-- .../cluster_catalog_items_server_test.go | 72 ++----------------- .../compute_instance_catalog_items_server.go | 12 ++-- ...pute_instance_catalog_items_server_test.go | 72 ++----------------- 6 files changed, 53 insertions(+), 179 deletions(-) diff --git a/internal/servers/catalog_item_validation.go b/internal/servers/catalog_item_validation.go index 3aa655d4c..92a98d185 100644 --- a/internal/servers/catalog_item_validation.go +++ b/internal/servers/catalog_item_validation.go @@ -14,7 +14,6 @@ language governing permissions and limitations under the License. package servers import ( - "context" "encoding/json" "fmt" "strings" @@ -29,7 +28,6 @@ import ( "google.golang.org/protobuf/types/known/structpb" privatev1 "github.com/osac-project/fulfillment-service/internal/api/osac/private/v1" - "github.com/osac-project/fulfillment-service/internal/auth" ) var ( @@ -55,18 +53,6 @@ func validateCELSyntax(filter string) error { return nil } -// buildPublishedClause returns a CEL clause that shows published items to everyone -// and unpublished items only to the user who created them. -func buildPublishedClause(ctx context.Context) string { - user := auth.SubjectFromContext(ctx).User - return fmt.Sprintf("(this.published || this.metadata.creator == '%s')", user) -} - -// isCreator checks whether the current user created the given item. -func isCreator(ctx context.Context, itemCreator string) bool { - return itemCreator != "" && auth.SubjectFromContext(ctx).User == itemCreator -} - // catalogItem is implemented by both ClusterCatalogItem and ComputeInstanceCatalogItem. type catalogItem interface { proto.Message diff --git a/internal/servers/catalog_item_validation_test.go b/internal/servers/catalog_item_validation_test.go index 4af4c1de6..6064c8417 100644 --- a/internal/servers/catalog_item_validation_test.go +++ b/internal/servers/catalog_item_validation_test.go @@ -14,52 +14,60 @@ language governing permissions and limitations under the License. package servers import ( - "context" - . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - - "github.com/osac-project/fulfillment-service/internal/auth" - "github.com/osac-project/fulfillment-service/internal/collections" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) var _ = Describe("addPublishedFilter", func() { - var ( - server *ClusterCatalogItemsServer - ctx context.Context - ) + var server *ClusterCatalogItemsServer BeforeEach(func() { server = &ClusterCatalogItemsServer{} - ctx = auth.ContextWithSubject(context.Background(), &auth.Subject{ - User: "test-admin", - Tenants: collections.NewSet("my-tenant"), - }) }) DescribeTable("composes filter correctly", func(input string, expected string) { - result, err := server.addPublishedFilter(ctx, input) + result, err := server.addPublishedFilter(input) Expect(err).ToNot(HaveOccurred()) Expect(result).To(Equal(expected)) }, - Entry("empty filter", "", - "(this.published || this.metadata.creator == 'test-admin')"), - Entry("simple filter", "this.id == '123'", - "(this.id == '123') && (this.published || this.metadata.creator == 'test-admin')"), + Entry("empty filter", "", "this.published"), + Entry("simple filter", "this.id == '123'", "(this.id == '123') && this.published"), Entry("compound filter", "this.title == 'a' && this.template == 'b'", - "(this.title == 'a' && this.template == 'b') && (this.published || this.metadata.creator == 'test-admin')"), + "(this.title == 'a' && this.template == 'b') && this.published"), Entry("valid filter with OR is safely composed", "true || true", - "(true || true) && (this.published || this.metadata.creator == 'test-admin')"), + "(true || true) && this.published"), ) DescribeTable("rejects malformed filters", func(input string) { - _, err := server.addPublishedFilter(ctx, input) + _, err := server.addPublishedFilter(input) Expect(err).To(HaveOccurred()) + Expect(status.Code(err)).To(Equal(codes.InvalidArgument)) }, Entry("unbalanced parens to bypass published", `true) || (true`), Entry("unbalanced closing paren", `true)`), Entry("unbalanced opening paren", `(true`), ) + + DescribeTable("validateCELSyntax", + func(input string, shouldPass bool) { + err := validateCELSyntax(input) + if shouldPass { + Expect(err).ToNot(HaveOccurred()) + } else { + Expect(err).To(HaveOccurred()) + } + }, + Entry("valid simple expression", "true", true), + Entry("valid field reference", "this.published", true), + Entry("valid comparison", "this.id == '123'", true), + Entry("valid compound", "this.a && this.b || this.c", true), + Entry("unbalanced closing paren", "true)", false), + Entry("unbalanced opening paren", "(true", false), + Entry("injection attempt", `true) || (true`, false), + Entry("empty string is not valid CEL", "", false), + ) }) diff --git a/internal/servers/cluster_catalog_items_server.go b/internal/servers/cluster_catalog_items_server.go index d98c4f3a9..6316210a6 100644 --- a/internal/servers/cluster_catalog_items_server.go +++ b/internal/servers/cluster_catalog_items_server.go @@ -126,7 +126,7 @@ func (s *ClusterCatalogItemsServer) List(ctx context.Context, privateRequest := &privatev1.ClusterCatalogItemsListRequest{} privateRequest.SetOffset(request.GetOffset()) privateRequest.SetLimit(request.GetLimit()) - composedFilter, err := s.addPublishedFilter(ctx, request.GetFilter()) + composedFilter, err := s.addPublishedFilter(request.GetFilter()) if err != nil { return nil, err } @@ -167,8 +167,7 @@ func (s *ClusterCatalogItemsServer) Get(ctx context.Context, return nil, err } - obj := privateResponse.GetObject() - if !obj.GetPublished() && !isCreator(ctx, obj.GetMetadata().GetCreator()) { + if !privateResponse.GetObject().GetPublished() { return nil, grpcstatus.Errorf(grpccodes.NotFound, "catalog item not found") } @@ -268,15 +267,14 @@ func (s *ClusterCatalogItemsServer) Update(ctx context.Context, return } -func (s *ClusterCatalogItemsServer) addPublishedFilter(ctx context.Context, filter string) (string, error) { - publishedClause := buildPublishedClause(ctx) +func (s *ClusterCatalogItemsServer) addPublishedFilter(filter string) (string, error) { if filter == "" { - return publishedClause, nil + return "this.published", nil } if err := validateCELSyntax(filter); err != nil { return "", grpcstatus.Errorf(grpccodes.InvalidArgument, "invalid filter: %v", err) } - return "(" + filter + ") && " + publishedClause, nil + return "(" + filter + ") && this.published", nil } func (s *ClusterCatalogItemsServer) Delete(ctx context.Context, diff --git a/internal/servers/cluster_catalog_items_server_test.go b/internal/servers/cluster_catalog_items_server_test.go index f452a5670..3880119ee 100644 --- a/internal/servers/cluster_catalog_items_server_test.go +++ b/internal/servers/cluster_catalog_items_server_test.go @@ -24,8 +24,6 @@ import ( "google.golang.org/protobuf/proto" publicv1 "github.com/osac-project/fulfillment-service/internal/api/osac/public/v1" - "github.com/osac-project/fulfillment-service/internal/auth" - "github.com/osac-project/fulfillment-service/internal/collections" "github.com/osac-project/fulfillment-service/internal/database" ) @@ -38,10 +36,7 @@ var _ = Describe("Cluster catalog items server", func() { BeforeEach(func() { var err error - ctx = auth.ContextWithSubject(context.Background(), &auth.Subject{ - User: "system", - Tenants: collections.NewUniversalSet[string](), - }) + ctx = context.Background() db, err := server.NewInstance().Build() Expect(err).ToNot(HaveOccurred()) @@ -192,7 +187,7 @@ var _ = Describe("Cluster catalog items server", func() { } }) - It("List excludes unpublished objects from other creators", func() { + It("List excludes unpublished objects", func() { _, err := server.Create(ctx, publicv1.ClusterCatalogItemsCreateRequest_builder{ Object: publicv1.ClusterCatalogItem_builder{ Title: "Published item", @@ -211,41 +206,13 @@ var _ = Describe("Cluster catalog items server", func() { }.Build()) Expect(err).ToNot(HaveOccurred()) - otherUserCtx := auth.ContextWithSubject(ctx, &auth.Subject{ - User: "other-user", - Tenants: collections.NewUniversalSet[string](), - }) - response, err := server.List(otherUserCtx, publicv1.ClusterCatalogItemsListRequest_builder{}.Build()) + response, err := server.List(ctx, publicv1.ClusterCatalogItemsListRequest_builder{}.Build()) Expect(err).ToNot(HaveOccurred()) Expect(response.GetItems()).To(HaveLen(1)) Expect(response.GetItems()[0].GetTitle()).To(Equal("Published item")) }) - It("List shows unpublished objects to their creator", func() { - _, err := server.Create(ctx, publicv1.ClusterCatalogItemsCreateRequest_builder{ - Object: publicv1.ClusterCatalogItem_builder{ - Title: "Published item", - Template: "my-template-id", - Published: true, - }.Build(), - }.Build()) - Expect(err).ToNot(HaveOccurred()) - - _, err = server.Create(ctx, publicv1.ClusterCatalogItemsCreateRequest_builder{ - Object: publicv1.ClusterCatalogItem_builder{ - Title: "Unpublished item", - Template: "my-template-id", - Published: false, - }.Build(), - }.Build()) - Expect(err).ToNot(HaveOccurred()) - - response, err := server.List(ctx, publicv1.ClusterCatalogItemsListRequest_builder{}.Build()) - Expect(err).ToNot(HaveOccurred()) - Expect(response.GetItems()).To(HaveLen(2)) - }) - - It("List with user filter excludes unpublished objects from other creators", func() { + It("List with user filter excludes unpublished objects", func() { publishedResponse, err := server.Create(ctx, publicv1.ClusterCatalogItemsCreateRequest_builder{ Object: publicv1.ClusterCatalogItem_builder{ Title: "Target published", @@ -276,11 +243,7 @@ var _ = Describe("Cluster catalog items server", func() { targetID := publishedResponse.GetObject().GetId() unpublishedID := unpublishedResponse.GetObject().GetId() filter := fmt.Sprintf("this.id == '%s' || this.id == '%s'", targetID, unpublishedID) - otherUserCtx := auth.ContextWithSubject(ctx, &auth.Subject{ - User: "other-user", - Tenants: collections.NewUniversalSet[string](), - }) - response, err := server.List(otherUserCtx, publicv1.ClusterCatalogItemsListRequest_builder{ + response, err := server.List(ctx, publicv1.ClusterCatalogItemsListRequest_builder{ Filter: proto.String(filter), }.Build()) Expect(err).ToNot(HaveOccurred()) @@ -288,7 +251,7 @@ var _ = Describe("Cluster catalog items server", func() { Expect(response.GetItems()[0].GetId()).To(Equal(targetID)) }) - It("Get returns not found for unpublished object from other creator", func() { + It("Get returns not found for unpublished object", func() { createResponse, err := server.Create(ctx, publicv1.ClusterCatalogItemsCreateRequest_builder{ Object: publicv1.ClusterCatalogItem_builder{ Title: "Unpublished item", @@ -298,34 +261,13 @@ var _ = Describe("Cluster catalog items server", func() { }.Build()) Expect(err).ToNot(HaveOccurred()) - otherUserCtx := auth.ContextWithSubject(ctx, &auth.Subject{ - User: "other-user", - Tenants: collections.NewUniversalSet[string](), - }) - _, err = server.Get(otherUserCtx, publicv1.ClusterCatalogItemsGetRequest_builder{ + _, err = server.Get(ctx, publicv1.ClusterCatalogItemsGetRequest_builder{ Id: createResponse.GetObject().GetId(), }.Build()) Expect(err).To(HaveOccurred()) Expect(status.Code(err)).To(Equal(codes.NotFound)) }) - It("Get returns unpublished object to its creator", func() { - createResponse, err := server.Create(ctx, publicv1.ClusterCatalogItemsCreateRequest_builder{ - Object: publicv1.ClusterCatalogItem_builder{ - Title: "Unpublished item", - Template: "my-template-id", - Published: false, - }.Build(), - }.Build()) - Expect(err).ToNot(HaveOccurred()) - - getResponse, err := server.Get(ctx, publicv1.ClusterCatalogItemsGetRequest_builder{ - Id: createResponse.GetObject().GetId(), - }.Build()) - Expect(err).ToNot(HaveOccurred()) - Expect(getResponse.GetObject().GetTitle()).To(Equal("Unpublished item")) - }) - It("Get object", func() { createResponse, err := server.Create(ctx, publicv1.ClusterCatalogItemsCreateRequest_builder{ Object: publicv1.ClusterCatalogItem_builder{ diff --git a/internal/servers/compute_instance_catalog_items_server.go b/internal/servers/compute_instance_catalog_items_server.go index 9b994a354..57b27960e 100644 --- a/internal/servers/compute_instance_catalog_items_server.go +++ b/internal/servers/compute_instance_catalog_items_server.go @@ -126,7 +126,7 @@ func (s *ComputeInstanceCatalogItemsServer) List(ctx context.Context, privateRequest := &privatev1.ComputeInstanceCatalogItemsListRequest{} privateRequest.SetOffset(request.GetOffset()) privateRequest.SetLimit(request.GetLimit()) - composedFilter, err := s.addPublishedFilter(ctx, request.GetFilter()) + composedFilter, err := s.addPublishedFilter(request.GetFilter()) if err != nil { return nil, err } @@ -167,8 +167,7 @@ func (s *ComputeInstanceCatalogItemsServer) Get(ctx context.Context, return nil, err } - obj := privateResponse.GetObject() - if !obj.GetPublished() && !isCreator(ctx, obj.GetMetadata().GetCreator()) { + if !privateResponse.GetObject().GetPublished() { return nil, grpcstatus.Errorf(grpccodes.NotFound, "catalog item not found") } @@ -268,15 +267,14 @@ func (s *ComputeInstanceCatalogItemsServer) Update(ctx context.Context, return } -func (s *ComputeInstanceCatalogItemsServer) addPublishedFilter(ctx context.Context, filter string) (string, error) { - publishedClause := buildPublishedClause(ctx) +func (s *ComputeInstanceCatalogItemsServer) addPublishedFilter(filter string) (string, error) { if filter == "" { - return publishedClause, nil + return "this.published", nil } if err := validateCELSyntax(filter); err != nil { return "", grpcstatus.Errorf(grpccodes.InvalidArgument, "invalid filter: %v", err) } - return "(" + filter + ") && " + publishedClause, nil + return "(" + filter + ") && this.published", nil } func (s *ComputeInstanceCatalogItemsServer) Delete(ctx context.Context, diff --git a/internal/servers/compute_instance_catalog_items_server_test.go b/internal/servers/compute_instance_catalog_items_server_test.go index 973cd3257..d1af6a10c 100644 --- a/internal/servers/compute_instance_catalog_items_server_test.go +++ b/internal/servers/compute_instance_catalog_items_server_test.go @@ -24,8 +24,6 @@ import ( "google.golang.org/protobuf/proto" publicv1 "github.com/osac-project/fulfillment-service/internal/api/osac/public/v1" - "github.com/osac-project/fulfillment-service/internal/auth" - "github.com/osac-project/fulfillment-service/internal/collections" "github.com/osac-project/fulfillment-service/internal/database" ) @@ -38,10 +36,7 @@ var _ = Describe("Compute instance catalog items server", func() { BeforeEach(func() { var err error - ctx = auth.ContextWithSubject(context.Background(), &auth.Subject{ - User: "system", - Tenants: collections.NewUniversalSet[string](), - }) + ctx = context.Background() db, err := server.NewInstance().Build() Expect(err).ToNot(HaveOccurred()) @@ -192,7 +187,7 @@ var _ = Describe("Compute instance catalog items server", func() { } }) - It("List excludes unpublished objects from other creators", func() { + It("List excludes unpublished objects", func() { _, err := server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ Object: publicv1.ComputeInstanceCatalogItem_builder{ Title: "Published item", @@ -211,41 +206,13 @@ var _ = Describe("Compute instance catalog items server", func() { }.Build()) Expect(err).ToNot(HaveOccurred()) - otherUserCtx := auth.ContextWithSubject(ctx, &auth.Subject{ - User: "other-user", - Tenants: collections.NewUniversalSet[string](), - }) - response, err := server.List(otherUserCtx, publicv1.ComputeInstanceCatalogItemsListRequest_builder{}.Build()) + response, err := server.List(ctx, publicv1.ComputeInstanceCatalogItemsListRequest_builder{}.Build()) Expect(err).ToNot(HaveOccurred()) Expect(response.GetItems()).To(HaveLen(1)) Expect(response.GetItems()[0].GetTitle()).To(Equal("Published item")) }) - It("List shows unpublished objects to their creator", func() { - _, err := server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ - Object: publicv1.ComputeInstanceCatalogItem_builder{ - Title: "Published item", - Template: "my-ci-template-id", - Published: true, - }.Build(), - }.Build()) - Expect(err).ToNot(HaveOccurred()) - - _, err = server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ - Object: publicv1.ComputeInstanceCatalogItem_builder{ - Title: "Unpublished item", - Template: "my-ci-template-id", - Published: false, - }.Build(), - }.Build()) - Expect(err).ToNot(HaveOccurred()) - - response, err := server.List(ctx, publicv1.ComputeInstanceCatalogItemsListRequest_builder{}.Build()) - Expect(err).ToNot(HaveOccurred()) - Expect(response.GetItems()).To(HaveLen(2)) - }) - - It("List with user filter excludes unpublished objects from other creators", func() { + It("List with user filter excludes unpublished objects", func() { publishedResponse, err := server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ Object: publicv1.ComputeInstanceCatalogItem_builder{ Title: "Target published", @@ -276,11 +243,7 @@ var _ = Describe("Compute instance catalog items server", func() { targetID := publishedResponse.GetObject().GetId() unpublishedID := unpublishedResponse.GetObject().GetId() filter := fmt.Sprintf("this.id == '%s' || this.id == '%s'", targetID, unpublishedID) - otherUserCtx := auth.ContextWithSubject(ctx, &auth.Subject{ - User: "other-user", - Tenants: collections.NewUniversalSet[string](), - }) - response, err := server.List(otherUserCtx, publicv1.ComputeInstanceCatalogItemsListRequest_builder{ + response, err := server.List(ctx, publicv1.ComputeInstanceCatalogItemsListRequest_builder{ Filter: proto.String(filter), }.Build()) Expect(err).ToNot(HaveOccurred()) @@ -288,7 +251,7 @@ var _ = Describe("Compute instance catalog items server", func() { Expect(response.GetItems()[0].GetId()).To(Equal(targetID)) }) - It("Get returns not found for unpublished object from other creator", func() { + It("Get returns not found for unpublished object", func() { createResponse, err := server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ Object: publicv1.ComputeInstanceCatalogItem_builder{ Title: "Unpublished item", @@ -298,34 +261,13 @@ var _ = Describe("Compute instance catalog items server", func() { }.Build()) Expect(err).ToNot(HaveOccurred()) - otherUserCtx := auth.ContextWithSubject(ctx, &auth.Subject{ - User: "other-user", - Tenants: collections.NewUniversalSet[string](), - }) - _, err = server.Get(otherUserCtx, publicv1.ComputeInstanceCatalogItemsGetRequest_builder{ + _, err = server.Get(ctx, publicv1.ComputeInstanceCatalogItemsGetRequest_builder{ Id: createResponse.GetObject().GetId(), }.Build()) Expect(err).To(HaveOccurred()) Expect(status.Code(err)).To(Equal(codes.NotFound)) }) - It("Get returns unpublished object to its creator", func() { - createResponse, err := server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ - Object: publicv1.ComputeInstanceCatalogItem_builder{ - Title: "Unpublished item", - Template: "my-ci-template-id", - Published: false, - }.Build(), - }.Build()) - Expect(err).ToNot(HaveOccurred()) - - getResponse, err := server.Get(ctx, publicv1.ComputeInstanceCatalogItemsGetRequest_builder{ - Id: createResponse.GetObject().GetId(), - }.Build()) - Expect(err).ToNot(HaveOccurred()) - Expect(getResponse.GetObject().GetTitle()).To(Equal("Unpublished item")) - }) - It("Get object", func() { createResponse, err := server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ Object: publicv1.ComputeInstanceCatalogItem_builder{ From f67ff20bd7b5df859387a0990912514f2a553b1f Mon Sep 17 00:00:00 2001 From: Elad Tabak Date: Sun, 31 May 2026 11:13:07 +0300 Subject: [PATCH 8/9] OSAC-58: allow Get of unpublished catalog item referenced by existing resource Per the enhancement proposal, a user can always Get a catalog item referenced by one of their existing clusters or compute instances, even if that item is unpublished. This ensures users retain access to catalog item details after an admin unpublishes it. The check uses a tenant-scoped DAO query so users can only see references from their own visible resources. Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Elad Tabak --- .../servers/cluster_catalog_items_server.go | 50 +++++++++++++++---- .../cluster_catalog_items_server_test.go | 29 +++++++++++ .../compute_instance_catalog_items_server.go | 50 +++++++++++++++---- ...pute_instance_catalog_items_server_test.go | 29 +++++++++++ 4 files changed, 140 insertions(+), 18 deletions(-) diff --git a/internal/servers/cluster_catalog_items_server.go b/internal/servers/cluster_catalog_items_server.go index 6316210a6..561c35e6e 100644 --- a/internal/servers/cluster_catalog_items_server.go +++ b/internal/servers/cluster_catalog_items_server.go @@ -16,6 +16,7 @@ package servers import ( "context" "errors" + "fmt" "log/slog" "github.com/prometheus/client_golang/prometheus" @@ -25,6 +26,7 @@ import ( privatev1 "github.com/osac-project/fulfillment-service/internal/api/osac/private/v1" publicv1 "github.com/osac-project/fulfillment-service/internal/api/osac/public/v1" "github.com/osac-project/fulfillment-service/internal/auth" + "github.com/osac-project/fulfillment-service/internal/database/dao" "github.com/osac-project/fulfillment-service/internal/events" ) @@ -41,10 +43,11 @@ var _ publicv1.ClusterCatalogItemsServer = (*ClusterCatalogItemsServer)(nil) type ClusterCatalogItemsServer struct { publicv1.UnimplementedClusterCatalogItemsServer - logger *slog.Logger - delegate privatev1.ClusterCatalogItemsServer - inMapper *GenericMapper[*publicv1.ClusterCatalogItem, *privatev1.ClusterCatalogItem] - outMapper *GenericMapper[*privatev1.ClusterCatalogItem, *publicv1.ClusterCatalogItem] + logger *slog.Logger + clustersDao *dao.GenericDAO[*privatev1.Cluster] + delegate privatev1.ClusterCatalogItemsServer + inMapper *GenericMapper[*publicv1.ClusterCatalogItem, *privatev1.ClusterCatalogItem] + outMapper *GenericMapper[*privatev1.ClusterCatalogItem, *publicv1.ClusterCatalogItem] } func NewClusterCatalogItemsServer() *ClusterCatalogItemsServerBuilder { @@ -101,6 +104,15 @@ func (b *ClusterCatalogItemsServerBuilder) Build() (result *ClusterCatalogItemsS return } + clustersDao, err := dao.NewGenericDAO[*privatev1.Cluster](). + SetLogger(b.logger). + SetTenancyLogic(b.tenancyLogic). + SetMetricsRegisterer(b.metricsRegisterer). + Build() + if err != nil { + return + } + delegate, err := NewPrivateClusterCatalogItemsServer(). SetLogger(b.logger). SetNotifier(b.notifier). @@ -113,10 +125,11 @@ func (b *ClusterCatalogItemsServerBuilder) Build() (result *ClusterCatalogItemsS } result = &ClusterCatalogItemsServer{ - logger: b.logger, - delegate: delegate, - inMapper: inMapper, - outMapper: outMapper, + logger: b.logger, + clustersDao: clustersDao, + delegate: delegate, + inMapper: inMapper, + outMapper: outMapper, } return } @@ -168,7 +181,13 @@ func (s *ClusterCatalogItemsServer) Get(ctx context.Context, } if !privateResponse.GetObject().GetPublished() { - return nil, grpcstatus.Errorf(grpccodes.NotFound, "catalog item not found") + hasRef, refErr := s.callerHasReferencingCluster(ctx, request.GetId()) + if refErr != nil { + return nil, refErr + } + if !hasRef { + return nil, grpcstatus.Errorf(grpccodes.NotFound, "catalog item not found") + } } publicCatalogItem := &publicv1.ClusterCatalogItem{} @@ -267,6 +286,19 @@ func (s *ClusterCatalogItemsServer) Update(ctx context.Context, return } +func (s *ClusterCatalogItemsServer) callerHasReferencingCluster(ctx context.Context, catalogItemID string) (bool, error) { + filter := fmt.Sprintf("this.spec.catalog_item == %q", catalogItemID) + response, err := s.clustersDao.List(). + SetFilter(filter). + SetLimit(1). + Do(ctx) + if err != nil { + s.logger.ErrorContext(ctx, "Failed to check cluster references", slog.Any("error", err)) + return false, grpcstatus.Errorf(grpccodes.Internal, "failed to check cluster references") + } + return response.GetTotal() > 0, nil +} + func (s *ClusterCatalogItemsServer) addPublishedFilter(filter string) (string, error) { if filter == "" { return "this.published", nil diff --git a/internal/servers/cluster_catalog_items_server_test.go b/internal/servers/cluster_catalog_items_server_test.go index 3880119ee..17f4050a5 100644 --- a/internal/servers/cluster_catalog_items_server_test.go +++ b/internal/servers/cluster_catalog_items_server_test.go @@ -251,6 +251,35 @@ var _ = Describe("Cluster catalog items server", func() { Expect(response.GetItems()[0].GetId()).To(Equal(targetID)) }) + It("Get returns unpublished item when caller has a referencing cluster", func() { + createResponse, err := server.Create(ctx, publicv1.ClusterCatalogItemsCreateRequest_builder{ + Object: publicv1.ClusterCatalogItem_builder{ + Title: "Unpublished item", + Template: "my-template-id", + Published: false, + }.Build(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + catalogItemID := createResponse.GetObject().GetId() + + _, err = tx.Exec(ctx, + `insert into clusters (id, data, tenant) values ($1, $2, $3)`, + "ref-cluster-001", + fmt.Sprintf(`{"spec":{"catalog_item":"%s","template":"my-template-id"}}`, catalogItemID), + "shared", + ) + Expect(err).ToNot(HaveOccurred()) + DeferCleanup(func() { + _, _ = tx.Exec(ctx, `delete from clusters where id = $1`, "ref-cluster-001") + }) + + getResponse, err := server.Get(ctx, publicv1.ClusterCatalogItemsGetRequest_builder{ + Id: catalogItemID, + }.Build()) + Expect(err).ToNot(HaveOccurred()) + Expect(getResponse.GetObject().GetTitle()).To(Equal("Unpublished item")) + }) + It("Get returns not found for unpublished object", func() { createResponse, err := server.Create(ctx, publicv1.ClusterCatalogItemsCreateRequest_builder{ Object: publicv1.ClusterCatalogItem_builder{ diff --git a/internal/servers/compute_instance_catalog_items_server.go b/internal/servers/compute_instance_catalog_items_server.go index 57b27960e..fcf05215a 100644 --- a/internal/servers/compute_instance_catalog_items_server.go +++ b/internal/servers/compute_instance_catalog_items_server.go @@ -16,6 +16,7 @@ package servers import ( "context" "errors" + "fmt" "log/slog" "github.com/prometheus/client_golang/prometheus" @@ -25,6 +26,7 @@ import ( privatev1 "github.com/osac-project/fulfillment-service/internal/api/osac/private/v1" publicv1 "github.com/osac-project/fulfillment-service/internal/api/osac/public/v1" "github.com/osac-project/fulfillment-service/internal/auth" + "github.com/osac-project/fulfillment-service/internal/database/dao" "github.com/osac-project/fulfillment-service/internal/events" ) @@ -41,10 +43,11 @@ var _ publicv1.ComputeInstanceCatalogItemsServer = (*ComputeInstanceCatalogItems type ComputeInstanceCatalogItemsServer struct { publicv1.UnimplementedComputeInstanceCatalogItemsServer - logger *slog.Logger - delegate privatev1.ComputeInstanceCatalogItemsServer - inMapper *GenericMapper[*publicv1.ComputeInstanceCatalogItem, *privatev1.ComputeInstanceCatalogItem] - outMapper *GenericMapper[*privatev1.ComputeInstanceCatalogItem, *publicv1.ComputeInstanceCatalogItem] + logger *slog.Logger + computeInstancesDao *dao.GenericDAO[*privatev1.ComputeInstance] + delegate privatev1.ComputeInstanceCatalogItemsServer + inMapper *GenericMapper[*publicv1.ComputeInstanceCatalogItem, *privatev1.ComputeInstanceCatalogItem] + outMapper *GenericMapper[*privatev1.ComputeInstanceCatalogItem, *publicv1.ComputeInstanceCatalogItem] } func NewComputeInstanceCatalogItemsServer() *ComputeInstanceCatalogItemsServerBuilder { @@ -101,6 +104,15 @@ func (b *ComputeInstanceCatalogItemsServerBuilder) Build() (result *ComputeInsta return } + computeInstancesDao, err := dao.NewGenericDAO[*privatev1.ComputeInstance](). + SetLogger(b.logger). + SetTenancyLogic(b.tenancyLogic). + SetMetricsRegisterer(b.metricsRegisterer). + Build() + if err != nil { + return + } + delegate, err := NewPrivateComputeInstanceCatalogItemsServer(). SetLogger(b.logger). SetNotifier(b.notifier). @@ -113,10 +125,11 @@ func (b *ComputeInstanceCatalogItemsServerBuilder) Build() (result *ComputeInsta } result = &ComputeInstanceCatalogItemsServer{ - logger: b.logger, - delegate: delegate, - inMapper: inMapper, - outMapper: outMapper, + logger: b.logger, + computeInstancesDao: computeInstancesDao, + delegate: delegate, + inMapper: inMapper, + outMapper: outMapper, } return } @@ -168,7 +181,13 @@ func (s *ComputeInstanceCatalogItemsServer) Get(ctx context.Context, } if !privateResponse.GetObject().GetPublished() { - return nil, grpcstatus.Errorf(grpccodes.NotFound, "catalog item not found") + hasRef, refErr := s.callerHasReferencingComputeInstance(ctx, request.GetId()) + if refErr != nil { + return nil, refErr + } + if !hasRef { + return nil, grpcstatus.Errorf(grpccodes.NotFound, "catalog item not found") + } } publicCatalogItem := &publicv1.ComputeInstanceCatalogItem{} @@ -267,6 +286,19 @@ func (s *ComputeInstanceCatalogItemsServer) Update(ctx context.Context, return } +func (s *ComputeInstanceCatalogItemsServer) callerHasReferencingComputeInstance(ctx context.Context, catalogItemID string) (bool, error) { + filter := fmt.Sprintf("this.spec.catalog_item == %q", catalogItemID) + response, err := s.computeInstancesDao.List(). + SetFilter(filter). + SetLimit(1). + Do(ctx) + if err != nil { + s.logger.ErrorContext(ctx, "Failed to check compute instance references", slog.Any("error", err)) + return false, grpcstatus.Errorf(grpccodes.Internal, "failed to check compute instance references") + } + return response.GetTotal() > 0, nil +} + func (s *ComputeInstanceCatalogItemsServer) addPublishedFilter(filter string) (string, error) { if filter == "" { return "this.published", nil diff --git a/internal/servers/compute_instance_catalog_items_server_test.go b/internal/servers/compute_instance_catalog_items_server_test.go index d1af6a10c..32bfe64e4 100644 --- a/internal/servers/compute_instance_catalog_items_server_test.go +++ b/internal/servers/compute_instance_catalog_items_server_test.go @@ -251,6 +251,35 @@ var _ = Describe("Compute instance catalog items server", func() { Expect(response.GetItems()[0].GetId()).To(Equal(targetID)) }) + It("Get returns unpublished item when caller has a referencing compute instance", func() { + createResponse, err := server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ + Object: publicv1.ComputeInstanceCatalogItem_builder{ + Title: "Unpublished item", + Template: "my-ci-template-id", + Published: false, + }.Build(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + catalogItemID := createResponse.GetObject().GetId() + + _, err = tx.Exec(ctx, + `insert into compute_instances (id, data, tenant) values ($1, $2, $3)`, + "ref-ci-001", + fmt.Sprintf(`{"spec":{"catalog_item":"%s","template":"my-ci-template-id"}}`, catalogItemID), + "shared", + ) + Expect(err).ToNot(HaveOccurred()) + DeferCleanup(func() { + _, _ = tx.Exec(ctx, `delete from compute_instances where id = $1`, "ref-ci-001") + }) + + getResponse, err := server.Get(ctx, publicv1.ComputeInstanceCatalogItemsGetRequest_builder{ + Id: catalogItemID, + }.Build()) + Expect(err).ToNot(HaveOccurred()) + Expect(getResponse.GetObject().GetTitle()).To(Equal("Unpublished item")) + }) + It("Get returns not found for unpublished object", func() { createResponse, err := server.Create(ctx, publicv1.ComputeInstanceCatalogItemsCreateRequest_builder{ Object: publicv1.ComputeInstanceCatalogItem_builder{ From 57633d6038b8651e2e734f790992005c7d2831b0 Mon Sep 17 00:00:00 2001 From: Elad Tabak Date: Sun, 31 May 2026 11:49:36 +0300 Subject: [PATCH 9/9] OSAC-58: allow Get of unpublished catalog item referenced by existing resource Per the enhancement proposal, a user can always Get a catalog item referenced by one of their existing clusters or compute instances, even if that item is unpublished. Introduce catalogItemReferenceChecker interface with a DAO-backed implementation (daoReferenceChecker) shared by both cluster and compute instance catalog item servers. The DAO query is tenant-scoped so users can only see references from their own visible resources. Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Elad Tabak --- .../servers/catalog_item_reference_checker.go | 46 +++++++++++++++ .../catalog_item_reference_checker_mock.go | 58 +++++++++++++++++++ .../servers/cluster_catalog_items_server.go | 37 ++++-------- .../cluster_catalog_items_server_test.go | 17 +++--- .../compute_instance_catalog_items_server.go | 37 ++++-------- ...pute_instance_catalog_items_server_test.go | 17 +++--- 6 files changed, 142 insertions(+), 70 deletions(-) create mode 100644 internal/servers/catalog_item_reference_checker.go create mode 100644 internal/servers/catalog_item_reference_checker_mock.go diff --git a/internal/servers/catalog_item_reference_checker.go b/internal/servers/catalog_item_reference_checker.go new file mode 100644 index 000000000..8a8fad25c --- /dev/null +++ b/internal/servers/catalog_item_reference_checker.go @@ -0,0 +1,46 @@ +/* +Copyright (c) 2025 Red Hat Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the +License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific +language governing permissions and limitations under the License. +*/ + +package servers + +import ( + "context" + "fmt" + + grpccodes "google.golang.org/grpc/codes" + grpcstatus "google.golang.org/grpc/status" + + "github.com/osac-project/fulfillment-service/internal/database/dao" +) + +// catalogItemReferenceChecker checks whether the caller has any resources that reference a catalog item. +type catalogItemReferenceChecker interface { + hasReference(ctx context.Context, catalogItemID string) (bool, error) +} + +// daoReferenceChecker implements catalogItemReferenceChecker using a GenericDAO. +type daoReferenceChecker[Resource dao.Object] struct { + resourceDao *dao.GenericDAO[Resource] +} + +func (c *daoReferenceChecker[Resource]) hasReference(ctx context.Context, catalogItemID string) (bool, error) { + filter := fmt.Sprintf("this.spec.catalog_item == %q", catalogItemID) + response, err := c.resourceDao.List(). + SetFilter(filter). + SetLimit(1). + Do(ctx) + if err != nil { + return false, grpcstatus.Errorf(grpccodes.Internal, "failed to check resource references") + } + return response.GetTotal() > 0, nil +} diff --git a/internal/servers/catalog_item_reference_checker_mock.go b/internal/servers/catalog_item_reference_checker_mock.go new file mode 100644 index 000000000..2b9b6e95f --- /dev/null +++ b/internal/servers/catalog_item_reference_checker_mock.go @@ -0,0 +1,58 @@ +/* +Copyright (c) 2025 Red Hat Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the +License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific +language governing permissions and limitations under the License. +*/ + +package servers + +import ( + "context" + "reflect" + + "go.uber.org/mock/gomock" +) + +type MockCatalogItemReferenceChecker struct { + ctrl *gomock.Controller + recorder *MockCatalogItemReferenceCheckerMockRecorder + isgomock struct{} +} + +type MockCatalogItemReferenceCheckerMockRecorder struct { + mock *MockCatalogItemReferenceChecker +} + +func NewMockCatalogItemReferenceChecker(ctrl *gomock.Controller) *MockCatalogItemReferenceChecker { + mock := &MockCatalogItemReferenceChecker{ctrl: ctrl} + mock.recorder = &MockCatalogItemReferenceCheckerMockRecorder{mock} + return mock +} + +func (m *MockCatalogItemReferenceChecker) EXPECT() *MockCatalogItemReferenceCheckerMockRecorder { + return m.recorder +} + +func (m *MockCatalogItemReferenceChecker) hasReference(ctx context.Context, catalogItemID string) (bool, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "hasReference", ctx, catalogItemID) + ret0, _ := ret[0].(bool) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +func (mr *MockCatalogItemReferenceCheckerMockRecorder) hasReference(ctx, catalogItemID any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType( + mr.mock, "hasReference", + reflect.TypeOf((*MockCatalogItemReferenceChecker)(nil).hasReference), + ctx, catalogItemID, + ) +} diff --git a/internal/servers/cluster_catalog_items_server.go b/internal/servers/cluster_catalog_items_server.go index 561c35e6e..c7b188334 100644 --- a/internal/servers/cluster_catalog_items_server.go +++ b/internal/servers/cluster_catalog_items_server.go @@ -16,7 +16,6 @@ package servers import ( "context" "errors" - "fmt" "log/slog" "github.com/prometheus/client_golang/prometheus" @@ -43,11 +42,11 @@ var _ publicv1.ClusterCatalogItemsServer = (*ClusterCatalogItemsServer)(nil) type ClusterCatalogItemsServer struct { publicv1.UnimplementedClusterCatalogItemsServer - logger *slog.Logger - clustersDao *dao.GenericDAO[*privatev1.Cluster] - delegate privatev1.ClusterCatalogItemsServer - inMapper *GenericMapper[*publicv1.ClusterCatalogItem, *privatev1.ClusterCatalogItem] - outMapper *GenericMapper[*privatev1.ClusterCatalogItem, *publicv1.ClusterCatalogItem] + logger *slog.Logger + referenceChecker catalogItemReferenceChecker + delegate privatev1.ClusterCatalogItemsServer + inMapper *GenericMapper[*publicv1.ClusterCatalogItem, *privatev1.ClusterCatalogItem] + outMapper *GenericMapper[*privatev1.ClusterCatalogItem, *publicv1.ClusterCatalogItem] } func NewClusterCatalogItemsServer() *ClusterCatalogItemsServerBuilder { @@ -112,6 +111,7 @@ func (b *ClusterCatalogItemsServerBuilder) Build() (result *ClusterCatalogItemsS if err != nil { return } + referenceChecker := &daoReferenceChecker[*privatev1.Cluster]{resourceDao: clustersDao} delegate, err := NewPrivateClusterCatalogItemsServer(). SetLogger(b.logger). @@ -125,11 +125,11 @@ func (b *ClusterCatalogItemsServerBuilder) Build() (result *ClusterCatalogItemsS } result = &ClusterCatalogItemsServer{ - logger: b.logger, - clustersDao: clustersDao, - delegate: delegate, - inMapper: inMapper, - outMapper: outMapper, + logger: b.logger, + referenceChecker: referenceChecker, + delegate: delegate, + inMapper: inMapper, + outMapper: outMapper, } return } @@ -181,7 +181,7 @@ func (s *ClusterCatalogItemsServer) Get(ctx context.Context, } if !privateResponse.GetObject().GetPublished() { - hasRef, refErr := s.callerHasReferencingCluster(ctx, request.GetId()) + hasRef, refErr := s.referenceChecker.hasReference(ctx, request.GetId()) if refErr != nil { return nil, refErr } @@ -286,19 +286,6 @@ func (s *ClusterCatalogItemsServer) Update(ctx context.Context, return } -func (s *ClusterCatalogItemsServer) callerHasReferencingCluster(ctx context.Context, catalogItemID string) (bool, error) { - filter := fmt.Sprintf("this.spec.catalog_item == %q", catalogItemID) - response, err := s.clustersDao.List(). - SetFilter(filter). - SetLimit(1). - Do(ctx) - if err != nil { - s.logger.ErrorContext(ctx, "Failed to check cluster references", slog.Any("error", err)) - return false, grpcstatus.Errorf(grpccodes.Internal, "failed to check cluster references") - } - return response.GetTotal() > 0, nil -} - func (s *ClusterCatalogItemsServer) addPublishedFilter(filter string) (string, error) { if filter == "" { return "this.published", nil diff --git a/internal/servers/cluster_catalog_items_server_test.go b/internal/servers/cluster_catalog_items_server_test.go index 17f4050a5..e76d44cea 100644 --- a/internal/servers/cluster_catalog_items_server_test.go +++ b/internal/servers/cluster_catalog_items_server_test.go @@ -19,6 +19,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "go.uber.org/mock/gomock" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" @@ -262,16 +263,12 @@ var _ = Describe("Cluster catalog items server", func() { Expect(err).ToNot(HaveOccurred()) catalogItemID := createResponse.GetObject().GetId() - _, err = tx.Exec(ctx, - `insert into clusters (id, data, tenant) values ($1, $2, $3)`, - "ref-cluster-001", - fmt.Sprintf(`{"spec":{"catalog_item":"%s","template":"my-template-id"}}`, catalogItemID), - "shared", - ) - Expect(err).ToNot(HaveOccurred()) - DeferCleanup(func() { - _, _ = tx.Exec(ctx, `delete from clusters where id = $1`, "ref-cluster-001") - }) + mockCtrl := gomock.NewController(GinkgoT()) + mockChecker := NewMockCatalogItemReferenceChecker(mockCtrl) + mockChecker.EXPECT().hasReference(gomock.Any(), catalogItemID).Return(true, nil) + originalChecker := server.referenceChecker + server.referenceChecker = mockChecker + DeferCleanup(func() { server.referenceChecker = originalChecker }) getResponse, err := server.Get(ctx, publicv1.ClusterCatalogItemsGetRequest_builder{ Id: catalogItemID, diff --git a/internal/servers/compute_instance_catalog_items_server.go b/internal/servers/compute_instance_catalog_items_server.go index fcf05215a..54bdef5f5 100644 --- a/internal/servers/compute_instance_catalog_items_server.go +++ b/internal/servers/compute_instance_catalog_items_server.go @@ -16,7 +16,6 @@ package servers import ( "context" "errors" - "fmt" "log/slog" "github.com/prometheus/client_golang/prometheus" @@ -43,11 +42,11 @@ var _ publicv1.ComputeInstanceCatalogItemsServer = (*ComputeInstanceCatalogItems type ComputeInstanceCatalogItemsServer struct { publicv1.UnimplementedComputeInstanceCatalogItemsServer - logger *slog.Logger - computeInstancesDao *dao.GenericDAO[*privatev1.ComputeInstance] - delegate privatev1.ComputeInstanceCatalogItemsServer - inMapper *GenericMapper[*publicv1.ComputeInstanceCatalogItem, *privatev1.ComputeInstanceCatalogItem] - outMapper *GenericMapper[*privatev1.ComputeInstanceCatalogItem, *publicv1.ComputeInstanceCatalogItem] + logger *slog.Logger + referenceChecker catalogItemReferenceChecker + delegate privatev1.ComputeInstanceCatalogItemsServer + inMapper *GenericMapper[*publicv1.ComputeInstanceCatalogItem, *privatev1.ComputeInstanceCatalogItem] + outMapper *GenericMapper[*privatev1.ComputeInstanceCatalogItem, *publicv1.ComputeInstanceCatalogItem] } func NewComputeInstanceCatalogItemsServer() *ComputeInstanceCatalogItemsServerBuilder { @@ -112,6 +111,7 @@ func (b *ComputeInstanceCatalogItemsServerBuilder) Build() (result *ComputeInsta if err != nil { return } + referenceChecker := &daoReferenceChecker[*privatev1.ComputeInstance]{resourceDao: computeInstancesDao} delegate, err := NewPrivateComputeInstanceCatalogItemsServer(). SetLogger(b.logger). @@ -125,11 +125,11 @@ func (b *ComputeInstanceCatalogItemsServerBuilder) Build() (result *ComputeInsta } result = &ComputeInstanceCatalogItemsServer{ - logger: b.logger, - computeInstancesDao: computeInstancesDao, - delegate: delegate, - inMapper: inMapper, - outMapper: outMapper, + logger: b.logger, + referenceChecker: referenceChecker, + delegate: delegate, + inMapper: inMapper, + outMapper: outMapper, } return } @@ -181,7 +181,7 @@ func (s *ComputeInstanceCatalogItemsServer) Get(ctx context.Context, } if !privateResponse.GetObject().GetPublished() { - hasRef, refErr := s.callerHasReferencingComputeInstance(ctx, request.GetId()) + hasRef, refErr := s.referenceChecker.hasReference(ctx, request.GetId()) if refErr != nil { return nil, refErr } @@ -286,19 +286,6 @@ func (s *ComputeInstanceCatalogItemsServer) Update(ctx context.Context, return } -func (s *ComputeInstanceCatalogItemsServer) callerHasReferencingComputeInstance(ctx context.Context, catalogItemID string) (bool, error) { - filter := fmt.Sprintf("this.spec.catalog_item == %q", catalogItemID) - response, err := s.computeInstancesDao.List(). - SetFilter(filter). - SetLimit(1). - Do(ctx) - if err != nil { - s.logger.ErrorContext(ctx, "Failed to check compute instance references", slog.Any("error", err)) - return false, grpcstatus.Errorf(grpccodes.Internal, "failed to check compute instance references") - } - return response.GetTotal() > 0, nil -} - func (s *ComputeInstanceCatalogItemsServer) addPublishedFilter(filter string) (string, error) { if filter == "" { return "this.published", nil diff --git a/internal/servers/compute_instance_catalog_items_server_test.go b/internal/servers/compute_instance_catalog_items_server_test.go index 32bfe64e4..f357628a2 100644 --- a/internal/servers/compute_instance_catalog_items_server_test.go +++ b/internal/servers/compute_instance_catalog_items_server_test.go @@ -19,6 +19,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "go.uber.org/mock/gomock" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" @@ -262,16 +263,12 @@ var _ = Describe("Compute instance catalog items server", func() { Expect(err).ToNot(HaveOccurred()) catalogItemID := createResponse.GetObject().GetId() - _, err = tx.Exec(ctx, - `insert into compute_instances (id, data, tenant) values ($1, $2, $3)`, - "ref-ci-001", - fmt.Sprintf(`{"spec":{"catalog_item":"%s","template":"my-ci-template-id"}}`, catalogItemID), - "shared", - ) - Expect(err).ToNot(HaveOccurred()) - DeferCleanup(func() { - _, _ = tx.Exec(ctx, `delete from compute_instances where id = $1`, "ref-ci-001") - }) + mockCtrl := gomock.NewController(GinkgoT()) + mockChecker := NewMockCatalogItemReferenceChecker(mockCtrl) + mockChecker.EXPECT().hasReference(gomock.Any(), catalogItemID).Return(true, nil) + originalChecker := server.referenceChecker + server.referenceChecker = mockChecker + DeferCleanup(func() { server.referenceChecker = originalChecker }) getResponse, err := server.Get(ctx, publicv1.ComputeInstanceCatalogItemsGetRequest_builder{ Id: catalogItemID,