From 3e05056145b2bdba7f25e95100dd04b978b0e2ec Mon Sep 17 00:00:00 2001 From: Tommy Hughes Date: Mon, 15 Jun 2026 12:50:00 -0500 Subject: [PATCH 1/5] OSAC-496: Remove tenant fallback dead code in fulfillment-service Remove the SharedTenants variable and stop defaulting universal-access subjects to the shared tenant when no explicit tenant is provided. Defer default-tenant resolution in the generic server until the request and current object both omit a tenant, and update integration tests to set shared tenant explicitly on admin creates. --- internal/auth/default_tenancy_logic.go | 13 +++-- internal/auth/default_tenancy_logic_test.go | 12 ++--- internal/auth/guest_tenancy_logic.go | 2 +- internal/auth/guest_tenancy_logic_test.go | 3 +- internal/auth/tenancy_logic.go | 3 -- internal/servers/generic_server.go | 38 +++++++------- internal/servers/servers_tenancy_test.go | 57 +++++++++++++++++++++ it/it_annotations_test.go | 4 +- it/it_cluster_reconciler_test.go | 4 +- it/it_compute_subnet_test.go | 8 ++- it/it_emergency_access_test.go | 5 ++ it/it_labels_test.go | 4 +- it/it_metadata.go | 36 +++++++++++++ it/it_multitenancy_test.go | 12 +++-- it/it_nodeset_removal_test.go | 7 ++- it/it_private_cluster_templates_test.go | 4 ++ it/it_private_host_types_test.go | 6 +++ it/it_public_clusters_test.go | 4 +- it/it_rest_gateway_test.go | 6 +++ it/it_role_binding_reconciler_test.go | 8 +-- it/it_role_reconciler_test.go | 8 +-- it/it_tool.go | 3 ++ it/it_version_test.go | 4 +- 23 files changed, 192 insertions(+), 59 deletions(-) create mode 100644 it/it_metadata.go diff --git a/internal/auth/default_tenancy_logic.go b/internal/auth/default_tenancy_logic.go index b12d2af02..5572fda91 100644 --- a/internal/auth/default_tenancy_logic.go +++ b/internal/auth/default_tenancy_logic.go @@ -75,15 +75,20 @@ func (p *DefaultTenancyLogic) DetermineAssignableTenants(ctx context.Context) (r } // DetermineDefaultTenant extracts the subject from the auth context and returns the tenant that will be assigned -// by default to objects. When the subject has access to all tenants (e.g. an admin), the default is the shared -// tenant because a universal set can't be stored as the tenant of an object. +// by default to objects when the request does not specify one explicitly. func (p *DefaultTenancyLogic) DetermineDefaultTenant(ctx context.Context) (result string, err error) { assignable, err := p.DetermineAssignableTenants(ctx) if err != nil { return } if !assignable.Finite() { - result = SharedTenant + subject := SubjectFromContext(ctx) + p.logger.ErrorContext( + ctx, + "Subject has access to all tenants but no explicit tenant was provided", + slog.String("user", subject.User), + ) + err = fmt.Errorf("explicit tenant is required when subject has access to all tenants") return } inclusions := assignable.Inclusions() @@ -100,7 +105,7 @@ func (p *DefaultTenancyLogic) DetermineVisibleTenants(ctx context.Context) (resu subject := SubjectFromContext(ctx) result = subject.Tenants if result.Finite() { - result = SharedTenants.Union(result) + result = collections.NewSet(SharedTenant).Union(result) } return } diff --git a/internal/auth/default_tenancy_logic_test.go b/internal/auth/default_tenancy_logic_test.go index ca5eb110e..fdcd0c0bf 100644 --- a/internal/auth/default_tenancy_logic_test.go +++ b/internal/auth/default_tenancy_logic_test.go @@ -119,15 +119,15 @@ var _ = Describe("Default tenancy logic", func() { Expect(result).To(BeElementOf("tenant-a", "tenant-b")) }) - It("Returns shared when tenants is universal", func() { + It("Fails when tenants is universal", func() { subject := &Subject{ User: "my_user", Tenants: AllTenants, } ctx = ContextWithSubject(ctx, subject) - result, err := logic.DetermineDefaultTenant(ctx) - Expect(err).ToNot(HaveOccurred()) - Expect(result).To(Equal(SharedTenant)) + _, err := logic.DetermineDefaultTenant(ctx) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("explicit tenant is required")) }) It("Fails if the subject has an empty tenants set", func() { @@ -151,7 +151,7 @@ var _ = Describe("Default tenancy logic", func() { ctx = ContextWithSubject(ctx, subject) result, err := logic.DetermineVisibleTenants(ctx) Expect(err).ToNot(HaveOccurred()) - Expect(result.Equal(SharedTenants.Union(collections.NewSet("tenant-a", "tenant-b")))).To(BeTrue()) + Expect(result.Equal(collections.NewSet(SharedTenant).Union(collections.NewSet("tenant-a", "tenant-b")))).To(BeTrue()) }) It("Returns universal set when tenants is universal", func() { @@ -173,7 +173,7 @@ var _ = Describe("Default tenancy logic", func() { ctx = ContextWithSubject(ctx, subject) result, err := logic.DetermineVisibleTenants(ctx) Expect(err).ToNot(HaveOccurred()) - Expect(result.Equal(SharedTenants)).To(BeTrue()) + Expect(result.Equal(collections.NewSet(SharedTenant))).To(BeTrue()) }) }) }) diff --git a/internal/auth/guest_tenancy_logic.go b/internal/auth/guest_tenancy_logic.go index f8bf2918f..144d966b8 100644 --- a/internal/auth/guest_tenancy_logic.go +++ b/internal/auth/guest_tenancy_logic.go @@ -67,7 +67,7 @@ func (p *GuestTenancyLogic) DetermineDefaultTenant(_ context.Context) (result st // DetermineVisibleTenants returns a set containing both the guest and shared tenants, allowing guest users to see // objects from both tenants. func (p *GuestTenancyLogic) DetermineVisibleTenants(_ context.Context) (result collections.Set[string], err error) { - result = GuestTenants.Union(SharedTenants) + result = GuestTenants.Union(collections.NewSet(SharedTenant)) return } diff --git a/internal/auth/guest_tenancy_logic_test.go b/internal/auth/guest_tenancy_logic_test.go index 9d1340b2c..521c37a13 100644 --- a/internal/auth/guest_tenancy_logic_test.go +++ b/internal/auth/guest_tenancy_logic_test.go @@ -18,6 +18,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/osac-project/fulfillment-service/internal/collections" ) var _ = Describe("Guest tenancy logic", func() { @@ -60,7 +61,7 @@ var _ = Describe("Guest tenancy logic", func() { It("Should return the guest and shared tenants", func() { result, err := logic.DetermineVisibleTenants(ctx) Expect(err).ToNot(HaveOccurred()) - Expect(result.Equal(GuestTenants.Union(SharedTenants))).To(BeTrue()) + Expect(result.Equal(GuestTenants.Union(collections.NewSet(SharedTenant)))).To(BeTrue()) }) }) }) diff --git a/internal/auth/tenancy_logic.go b/internal/auth/tenancy_logic.go index fe73caac1..9a83ddafa 100644 --- a/internal/auth/tenancy_logic.go +++ b/internal/auth/tenancy_logic.go @@ -46,8 +46,5 @@ var SystemTenants = collections.NewSet(SystemTenant) // SharedTenant is the tenant that is always visible to all users. const SharedTenant = "shared" -// SharedTenants is the set of tenants that are always visible to all users. -var SharedTenants = collections.NewSet(SharedTenant) - // AllTenants is the set of all tenants that are possible. var AllTenants = collections.NewUniversalSet[string]() diff --git a/internal/servers/generic_server.go b/internal/servers/generic_server.go index 3bfa3a9dc..03b295b34 100644 --- a/internal/servers/generic_server.go +++ b/internal/servers/generic_server.go @@ -1197,22 +1197,6 @@ func (s *GenericServer[O]) determineAssignedTenant(ctx context.Context, return } - // Determine the default tenant: - defaultTenant, err := s.tenancyLogic.DetermineDefaultTenant(ctx) - if err != nil { - s.logger.ErrorContext( - ctx, - "Failed to determine default tenant", - slog.Any("error", err), - ) - err = grpcstatus.Errorf(grpccodes.Internal, "failed to determine default tenant") - return - } - if defaultTenant == "" { - err = grpcstatus.Errorf(grpccodes.PermissionDenied, "there is no default tenant") - return - } - // Get the tenant from the request and current object: requestTenant := s.getTenant(requestObject) currentTenant := s.getTenant(currentObject) @@ -1249,12 +1233,28 @@ func (s *GenericServer[O]) determineAssignedTenant(ctx context.Context, return } - // Fall back to the current tenant or the default: + // Fall back to the current tenant when the request does not specify one: if currentTenant != "" { result = currentTenant - } else { - result = defaultTenant + return + } + + // Determine the default tenant only when neither request nor current object specify one: + defaultTenant, err := s.tenancyLogic.DetermineDefaultTenant(ctx) + if err != nil { + s.logger.ErrorContext( + ctx, + "Failed to determine default tenant", + slog.Any("error", err), + ) + err = grpcstatus.Errorf(grpccodes.Internal, "failed to determine default tenant") + return + } + if defaultTenant == "" { + err = grpcstatus.Errorf(grpccodes.PermissionDenied, "there is no default tenant") + return } + result = defaultTenant return } diff --git a/internal/servers/servers_tenancy_test.go b/internal/servers/servers_tenancy_test.go index 80381b239..b435d7a38 100644 --- a/internal/servers/servers_tenancy_test.go +++ b/internal/servers/servers_tenancy_test.go @@ -300,6 +300,63 @@ var _ = Describe("Tenancy logic", func() { Expect(status.Message()).To(Equal("tenant 'your-tenant' doesn't exist")) }) + It("Uses explicit tenant when subject has access to all tenants", func() { + // Create a tenancy logic that represents a universal admin: + tenancy := auth.NewMockTenancyLogic(ctrl) + tenancy.EXPECT().DetermineAssignableTenants(gomock.Any()). + Return(auth.AllTenants, nil). + AnyTimes() + tenancy.EXPECT().DetermineVisibleTenants(gomock.Any()). + Return(auth.AllTenants, nil). + AnyTimes() + + // Create the template using the DAO: + templatesDao, err := dao.NewGenericDAO[*privatev1.ClusterTemplate](). + SetLogger(logger). + SetTenancyLogic(tenancy). + Build() + Expect(err).ToNot(HaveOccurred()) + _, err = templatesDao.Create(). + SetObject( + privatev1.ClusterTemplate_builder{ + Id: "my-template", + Title: "My template", + Description: "My template", + Metadata: privatev1.Metadata_builder{ + Tenant: "my-tenant", + }.Build(), + }.Build(), + ). + Do(ctx) + Expect(err).ToNot(HaveOccurred()) + + // Create the clusters server: + clustersServer, err := NewClustersServer(). + SetLogger(logger). + SetAttributionLogic(attribution). + SetTenancyLogic(tenancy). + SetScheme(testScheme). + Build() + Expect(err).ToNot(HaveOccurred()) + + // Create a cluster with an explicit tenant and verify it succeeds without a default tenant: + response, err := clustersServer.Create(ctx, publicv1.ClustersCreateRequest_builder{ + Object: publicv1.Cluster_builder{ + Metadata: publicv1.Metadata_builder{ + Tenant: "my-tenant", + }.Build(), + Spec: publicv1.ClusterSpec_builder{ + Template: "my-template", + }.Build(), + }.Build(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + + cluster := response.GetObject() + Expect(cluster).ToNot(BeNil()) + Expect(cluster.GetMetadata().GetTenant()).To(Equal("my-tenant")) + }) + It("Rejects object creation when tenant is visible to the user, but doesn't exist in the database", func() { // Create a tenancy logic that returns visible tenants: tenancy := auth.NewMockTenancyLogic(ctrl) diff --git a/it/it_annotations_test.go b/it/it_annotations_test.go index c76633f3f..32a6b6d17 100644 --- a/it/it_annotations_test.go +++ b/it/it_annotations_test.go @@ -46,7 +46,8 @@ var _ = Describe("Annotations", func() { hostTypeId = fmt.Sprintf("my-host-type-%s", uuid.New()) _, err := hostTypesClient.Create(ctx, privatev1.HostTypesCreateRequest_builder{ Object: privatev1.HostType_builder{ - Id: hostTypeId, + Metadata: sharedMetadata(), + Id: hostTypeId, }.Build(), }.Build()) Expect(err).ToNot(HaveOccurred()) @@ -60,6 +61,7 @@ var _ = Describe("Annotations", func() { templateId = fmt.Sprintf("my-template-%s", uuid.New()) _, err = templatesClient.Create(ctx, privatev1.ClusterTemplatesCreateRequest_builder{ Object: privatev1.ClusterTemplate_builder{ + Metadata: sharedMetadata(), Id: templateId, Title: "My template %s", Description: "My template.", diff --git a/it/it_cluster_reconciler_test.go b/it/it_cluster_reconciler_test.go index 1672c1b07..b19a50e5f 100644 --- a/it/it_cluster_reconciler_test.go +++ b/it/it_cluster_reconciler_test.go @@ -62,7 +62,8 @@ var _ = Describe("Cluster reconciler", func() { hostTypeId = fmt.Sprintf("my_host_type_%s", uuid.New()) _, err := hostTypesClient.Create(ctx, privatev1.HostTypesCreateRequest_builder{ Object: privatev1.HostType_builder{ - Id: hostTypeId, + Metadata: sharedMetadata(), + Id: hostTypeId, }.Build(), }.Build()) Expect(err).ToNot(HaveOccurred()) @@ -71,6 +72,7 @@ var _ = Describe("Cluster reconciler", func() { templateId = fmt.Sprintf("my_template_%s", uuid.New()) _, err = templatesClient.Create(ctx, privatev1.ClusterTemplatesCreateRequest_builder{ Object: privatev1.ClusterTemplate_builder{ + Metadata: sharedMetadata(), Id: templateId, Title: "My template %s", Description: "My template.", diff --git a/it/it_compute_subnet_test.go b/it/it_compute_subnet_test.go index f9f168a7e..9ef794af0 100644 --- a/it/it_compute_subnet_test.go +++ b/it/it_compute_subnet_test.go @@ -56,6 +56,7 @@ var _ = Describe("ComputeInstance with Subnet attachment", func() { computeInstanceTemplateId = fmt.Sprintf("test-ci-template-%s", uuid.New()) _, err := computeInstanceTemplatesClient.Create(ctx, privatev1.ComputeInstanceTemplatesCreateRequest_builder{ Object: privatev1.ComputeInstanceTemplate_builder{ + Metadata: sharedMetadata(), Id: computeInstanceTemplateId, Title: "Test CI Template", Description: "Template for compute instance subnet test.", @@ -66,6 +67,7 @@ var _ = Describe("ComputeInstance with Subnet attachment", func() { // Create NetworkClass ncResp, err := networkClassesClient.Create(ctx, privatev1.NetworkClassesCreateRequest_builder{ Object: privatev1.NetworkClass_builder{ + Metadata: sharedMetadata(), Title: "Test CUDN Network Class", ImplementationStrategy: "cudn", }.Build(), @@ -77,7 +79,8 @@ var _ = Describe("ComputeInstance with Subnet attachment", func() { virtualNetworkId = fmt.Sprintf("test-vnet-%s", uuid.New()) _, err = virtualNetworksClient.Create(ctx, privatev1.VirtualNetworksCreateRequest_builder{ Object: privatev1.VirtualNetwork_builder{ - Id: virtualNetworkId, + Metadata: sharedMetadata(), + Id: virtualNetworkId, Spec: privatev1.VirtualNetworkSpec_builder{ NetworkClass: networkClassId, Region: "us-east-1", @@ -107,7 +110,8 @@ var _ = Describe("ComputeInstance with Subnet attachment", func() { subnetId = fmt.Sprintf("test-subnet-%s", uuid.New()) _, err = subnetsClient.Create(ctx, privatev1.SubnetsCreateRequest_builder{ Object: privatev1.Subnet_builder{ - Id: subnetId, + Metadata: sharedMetadata(), + Id: subnetId, Spec: privatev1.SubnetSpec_builder{ VirtualNetwork: virtualNetworkId, Ipv4Cidr: new("10.100.1.0/24"), diff --git a/it/it_emergency_access_test.go b/it/it_emergency_access_test.go index 4ca8413e6..03dfc53f5 100644 --- a/it/it_emergency_access_test.go +++ b/it/it_emergency_access_test.go @@ -24,6 +24,7 @@ import ( . "github.com/onsi/gomega" privatev1 "github.com/osac-project/fulfillment-service/internal/api/osac/private/v1" + "github.com/osac-project/fulfillment-service/internal/auth" "github.com/osac-project/fulfillment-service/internal/uuid" ) @@ -42,6 +43,7 @@ var _ = Describe("Emergency access", func() { id := fmt.Sprintf("emergency_grpc_%s", uuid.New()) _, err := client.Create(ctx, privatev1.ClusterTemplatesCreateRequest_builder{ Object: privatev1.ClusterTemplate_builder{ + Metadata: sharedMetadata(), Id: id, Title: "Emergency gRPC template", Description: "Template created via gRPC using emergency admin service account.", @@ -63,6 +65,9 @@ var _ = Describe("Emergency access", func() { "id": id, "title": "Emergency REST template", "description": "Template created via REST using emergency admin service account.", + "metadata": map[string]any{ + "tenant": auth.SharedTenant, + }, } requestData, err := json.Marshal(requestBody) Expect(err).ToNot(HaveOccurred()) diff --git a/it/it_labels_test.go b/it/it_labels_test.go index 0f0452744..a39f81278 100644 --- a/it/it_labels_test.go +++ b/it/it_labels_test.go @@ -46,7 +46,8 @@ var _ = Describe("Labels", func() { hostTypeId = fmt.Sprintf("my-host-type-%s", uuid.New()) _, err := hostTypesClient.Create(ctx, privatev1.HostTypesCreateRequest_builder{ Object: privatev1.HostType_builder{ - Id: hostTypeId, + Metadata: sharedMetadata(), + Id: hostTypeId, }.Build(), }.Build()) Expect(err).ToNot(HaveOccurred()) @@ -60,6 +61,7 @@ var _ = Describe("Labels", func() { templateId = fmt.Sprintf("my-template-%s", uuid.New()) _, err = templatesClient.Create(ctx, privatev1.ClusterTemplatesCreateRequest_builder{ Object: privatev1.ClusterTemplate_builder{ + Metadata: sharedMetadata(), Id: templateId, Title: "My template %s", Description: "My template.", diff --git a/it/it_metadata.go b/it/it_metadata.go new file mode 100644 index 000000000..09fa1613c --- /dev/null +++ b/it/it_metadata.go @@ -0,0 +1,36 @@ +/* +Copyright (c) 2026 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 it + +import ( + privatev1 "github.com/osac-project/fulfillment-service/internal/api/osac/private/v1" + "github.com/osac-project/fulfillment-service/internal/auth" +) + +// sharedMetadata returns metadata that assigns objects to the shared tenant. Admin integration +// tests must set an explicit tenant because admin subjects have universal access and no longer +// receive an implicit shared default. +func sharedMetadata() *privatev1.Metadata { + return privatev1.Metadata_builder{ + Tenant: auth.SharedTenant, + }.Build() +} + +// sharedMetadataWithName returns shared-tenant metadata with the given resource name. +func sharedMetadataWithName(name string) *privatev1.Metadata { + return privatev1.Metadata_builder{ + Name: name, + Tenant: auth.SharedTenant, + }.Build() +} diff --git a/it/it_multitenancy_test.go b/it/it_multitenancy_test.go index c1e347bb3..a89c9e9b9 100644 --- a/it/it_multitenancy_test.go +++ b/it/it_multitenancy_test.go @@ -106,7 +106,8 @@ var _ = Describe("Multitenancy basic tenant isolation", Ordered, Label("multiten hostTypeId := fmt.Sprintf("sa-isolation-hosttype-%s", uuid.New()) _, err := hostTypesClient.Create(ctx, privatev1.HostTypesCreateRequest_builder{ Object: privatev1.HostType_builder{ - Id: hostTypeId, + Metadata: sharedMetadata(), + Id: hostTypeId, }.Build(), }.Build()) Expect(err).ToNot(HaveOccurred()) @@ -139,7 +140,8 @@ var _ = Describe("Multitenancy basic tenant isolation", Ordered, Label("multiten templatesClient := privatev1.NewClusterTemplatesClient(tool.InternalView().AdminConn()) _, err = templatesClient.Create(ctx, privatev1.ClusterTemplatesCreateRequest_builder{ Object: privatev1.ClusterTemplate_builder{ - Id: templateId, + Metadata: sharedMetadata(), + Id: templateId, NodeSets: map[string]*privatev1.ClusterTemplateNodeSet{ "my-node-set": privatev1.ClusterTemplateNodeSet_builder{ HostType: hostTypeId, @@ -350,7 +352,8 @@ var _ = Describe("Multitenancy basic tenant isolation", Ordered, Label("multiten hostTypesClient := privatev1.NewHostTypesClient(tool.InternalView().AdminConn()) _, err := hostTypesClient.Create(ctx, privatev1.HostTypesCreateRequest_builder{ Object: privatev1.HostType_builder{ - Id: hostTypeId, + Metadata: sharedMetadata(), + Id: hostTypeId, }.Build(), }.Build()) Expect(err).ToNot(HaveOccurred()) @@ -366,7 +369,8 @@ var _ = Describe("Multitenancy basic tenant isolation", Ordered, Label("multiten templatesClient := privatev1.NewClusterTemplatesClient(tool.InternalView().AdminConn()) _, err = templatesClient.Create(ctx, privatev1.ClusterTemplatesCreateRequest_builder{ Object: privatev1.ClusterTemplate_builder{ - Id: templateId, + Metadata: sharedMetadata(), + Id: templateId, NodeSets: map[string]*privatev1.ClusterTemplateNodeSet{ "my-node-set": privatev1.ClusterTemplateNodeSet_builder{ HostType: hostTypeId, diff --git a/it/it_nodeset_removal_test.go b/it/it_nodeset_removal_test.go index 9d72ec8f2..bf6232f3a 100644 --- a/it/it_nodeset_removal_test.go +++ b/it/it_nodeset_removal_test.go @@ -48,7 +48,8 @@ var _ = Describe("Node set removal", func() { workerHostTypeId = fmt.Sprintf("worker_type_%s", uuid.New()) _, err := hostTypesClient.Create(ctx, privatev1.HostTypesCreateRequest_builder{ Object: privatev1.HostType_builder{ - Id: workerHostTypeId, + Metadata: sharedMetadata(), + Id: workerHostTypeId, }.Build(), }.Build()) Expect(err).ToNot(HaveOccurred()) @@ -57,7 +58,8 @@ var _ = Describe("Node set removal", func() { storageHostTypeId = fmt.Sprintf("storage_type_%s", uuid.New()) _, err = hostTypesClient.Create(ctx, privatev1.HostTypesCreateRequest_builder{ Object: privatev1.HostType_builder{ - Id: storageHostTypeId, + Metadata: sharedMetadata(), + Id: storageHostTypeId, }.Build(), }.Build()) Expect(err).ToNot(HaveOccurred()) @@ -66,6 +68,7 @@ var _ = Describe("Node set removal", func() { templateId = fmt.Sprintf("template_2_nodesets_%s", uuid.New()) _, err = templatesClient.Create(ctx, privatev1.ClusterTemplatesCreateRequest_builder{ Object: privatev1.ClusterTemplate_builder{ + Metadata: sharedMetadata(), Id: templateId, Title: "Template with 2 node sets", Description: "A template with workers and storage node sets.", diff --git a/it/it_private_cluster_templates_test.go b/it/it_private_cluster_templates_test.go index 10c9c0b1c..07cc97206 100644 --- a/it/it_private_cluster_templates_test.go +++ b/it/it_private_cluster_templates_test.go @@ -50,6 +50,7 @@ var _ = Describe("Private cluster templates", func() { id := fmt.Sprintf("my_template_%s", uuid.New()) _, err := client.Create(ctx, privatev1.ClusterTemplatesCreateRequest_builder{ Object: privatev1.ClusterTemplate_builder{ + Metadata: sharedMetadata(), Id: id, Title: "My title", Description: "My description.", @@ -78,6 +79,7 @@ var _ = Describe("Private cluster templates", func() { id := fmt.Sprintf("my_template_%s", uuid.New()) response, err := client.Create(ctx, privatev1.ClusterTemplatesCreateRequest_builder{ Object: privatev1.ClusterTemplate_builder{ + Metadata: sharedMetadata(), Id: id, Title: "My title", Description: "My description.", @@ -101,6 +103,7 @@ var _ = Describe("Private cluster templates", func() { id := fmt.Sprintf("my_template_%s", uuid.New()) _, err := client.Create(ctx, privatev1.ClusterTemplatesCreateRequest_builder{ Object: privatev1.ClusterTemplate_builder{ + Metadata: sharedMetadata(), Id: id, Title: "My title", Description: "My description.", @@ -150,6 +153,7 @@ var _ = Describe("Private cluster templates", func() { id := fmt.Sprintf("my_template_%s", uuid.New()) _, err := client.Create(ctx, privatev1.ClusterTemplatesCreateRequest_builder{ Object: privatev1.ClusterTemplate_builder{ + Metadata: sharedMetadata(), Id: id, Title: "My title", Description: "My description.", diff --git a/it/it_private_host_types_test.go b/it/it_private_host_types_test.go index 2a5b6d9e5..2088a4dc9 100644 --- a/it/it_private_host_types_test.go +++ b/it/it_private_host_types_test.go @@ -21,6 +21,7 @@ import ( . "github.com/onsi/gomega" privatev1 "github.com/osac-project/fulfillment-service/internal/api/osac/private/v1" + "github.com/osac-project/fulfillment-service/internal/auth" "github.com/osac-project/fulfillment-service/internal/uuid" ) @@ -40,6 +41,7 @@ var _ = Describe("Private host types", func() { id := fmt.Sprintf("my_host_type_%s", uuid.New()) _, err := client.Create(ctx, privatev1.HostTypesCreateRequest_builder{ Object: privatev1.HostType_builder{ + Metadata: sharedMetadata(), Id: id, Title: "My title", Description: "My description.", @@ -60,6 +62,7 @@ var _ = Describe("Private host types", func() { id := fmt.Sprintf("my_host_type_%s", uuid.New()) _, err := client.Create(ctx, privatev1.HostTypesCreateRequest_builder{ Object: privatev1.HostType_builder{ + Metadata: sharedMetadata(), Id: id, Title: "My title", Description: "My description.", @@ -88,6 +91,7 @@ var _ = Describe("Private host types", func() { id := fmt.Sprintf("my_template_%s", uuid.New()) response, err := client.Create(ctx, privatev1.HostTypesCreateRequest_builder{ Object: privatev1.HostType_builder{ + Metadata: sharedMetadata(), Id: id, Title: "My title", Description: "My description.", @@ -111,6 +115,7 @@ var _ = Describe("Private host types", func() { id := fmt.Sprintf("my_host_type_%s", uuid.New()) _, err := client.Create(ctx, privatev1.HostTypesCreateRequest_builder{ Object: privatev1.HostType_builder{ + Metadata: sharedMetadata(), Id: id, Title: "My title", Description: "My description.", @@ -162,6 +167,7 @@ var _ = Describe("Private host types", func() { Object: privatev1.HostType_builder{ Metadata: privatev1.Metadata_builder{ Finalizers: []string{"a"}, + Tenant: auth.SharedTenant, }.Build(), Id: id, Title: "My title", diff --git a/it/it_public_clusters_test.go b/it/it_public_clusters_test.go index 8ff32ef7f..f6a8d76a2 100644 --- a/it/it_public_clusters_test.go +++ b/it/it_public_clusters_test.go @@ -58,7 +58,8 @@ var _ = Describe("Public clusters", func() { hostTypeId = fmt.Sprintf("my-host-type-%s", uuid.New()) _, err := hostTypesClient.Create(ctx, privatev1.HostTypesCreateRequest_builder{ Object: privatev1.HostType_builder{ - Id: hostTypeId, + Metadata: sharedMetadata(), + Id: hostTypeId, }.Build(), }.Build()) Expect(err).ToNot(HaveOccurred()) @@ -73,6 +74,7 @@ var _ = Describe("Public clusters", func() { templateId = fmt.Sprintf("my-template-%s", uuid.New()) _, err = templatesClient.Create(ctx, privatev1.ClusterTemplatesCreateRequest_builder{ Object: privatev1.ClusterTemplate_builder{ + Metadata: sharedMetadata(), Id: templateId, Title: "My template %s", Description: "My template.", diff --git a/it/it_rest_gateway_test.go b/it/it_rest_gateway_test.go index d28ebd6b4..ae99e721d 100644 --- a/it/it_rest_gateway_test.go +++ b/it/it_rest_gateway_test.go @@ -46,6 +46,7 @@ var _ = Describe("REST gateway", func() { gpuHostTypeID := fmt.Sprintf("gpus_%s", uuid.New()) _, err := hostTypesClient.Create(ctx, privatev1.HostTypesCreateRequest_builder{ Object: privatev1.HostType_builder{ + Metadata: sharedMetadata(), Id: computeHostTypeID, Title: "Compute", Description: "Compute.", @@ -60,6 +61,7 @@ var _ = Describe("REST gateway", func() { }) _, err = hostTypesClient.Create(ctx, privatev1.HostTypesCreateRequest_builder{ Object: privatev1.HostType_builder{ + Metadata: sharedMetadata(), Id: gpuHostTypeID, Title: "GPU", Description: "GPU.", @@ -87,6 +89,7 @@ var _ = Describe("REST gateway", func() { } _, err = templatesClient.Create(ctx, privatev1.ClusterTemplatesCreateRequest_builder{ Object: privatev1.ClusterTemplate_builder{ + Metadata: sharedMetadata(), Id: templateID, Title: "My template", Description: "My template.", @@ -129,6 +132,7 @@ var _ = Describe("REST gateway", func() { gpuHostTypeID := fmt.Sprintf("gpus_%s", uuid.New()) _, err := hostTypesClient.Create(ctx, privatev1.HostTypesCreateRequest_builder{ Object: privatev1.HostType_builder{ + Metadata: sharedMetadata(), Id: computeHostTypeID, Title: "Compute", Description: "Compute.", @@ -143,6 +147,7 @@ var _ = Describe("REST gateway", func() { }) _, err = hostTypesClient.Create(ctx, privatev1.HostTypesCreateRequest_builder{ Object: privatev1.HostType_builder{ + Metadata: sharedMetadata(), Id: gpuHostTypeID, Title: "GPU", Description: "GPU.", @@ -170,6 +175,7 @@ var _ = Describe("REST gateway", func() { } _, err = templatesClient.Create(ctx, privatev1.ClusterTemplatesCreateRequest_builder{ Object: privatev1.ClusterTemplate_builder{ + Metadata: sharedMetadata(), Id: templateID, Title: "My private template", Description: "My private template.", diff --git a/it/it_role_binding_reconciler_test.go b/it/it_role_binding_reconciler_test.go index 7128acf47..818aedf10 100644 --- a/it/it_role_binding_reconciler_test.go +++ b/it/it_role_binding_reconciler_test.go @@ -43,9 +43,7 @@ var _ = Describe("Role binding reconciler", func() { // Create the role binding and remember to delete it after the test: createResponse, err := client.Create(ctx, privatev1.RoleBindingsCreateRequest_builder{ Object: privatev1.RoleBinding_builder{ - Metadata: privatev1.Metadata_builder{ - Name: fmt.Sprintf("my-%s", uuid.New()), - }.Build(), + Metadata: sharedMetadataWithName(fmt.Sprintf("my-%s", uuid.New())), Spec: privatev1.RoleBindingSpec_builder{ Role: "my-role", Groups: []string{ @@ -84,9 +82,7 @@ var _ = Describe("Role binding reconciler", func() { // Create the role binding: createResponse, err := client.Create(ctx, privatev1.RoleBindingsCreateRequest_builder{ Object: privatev1.RoleBinding_builder{ - Metadata: privatev1.Metadata_builder{ - Name: fmt.Sprintf("my-%s", uuid.New()), - }.Build(), + Metadata: sharedMetadataWithName(fmt.Sprintf("my-%s", uuid.New())), Spec: privatev1.RoleBindingSpec_builder{ Role: "my-role", Groups: []string{ diff --git a/it/it_role_reconciler_test.go b/it/it_role_reconciler_test.go index 3ffe80802..1487ba908 100644 --- a/it/it_role_reconciler_test.go +++ b/it/it_role_reconciler_test.go @@ -43,9 +43,7 @@ var _ = Describe("Role reconciler", func() { // Create the role and remember to delete it after the test: createResponse, err := client.Create(ctx, privatev1.RolesCreateRequest_builder{ Object: privatev1.Role_builder{ - Metadata: privatev1.Metadata_builder{ - Name: fmt.Sprintf("my-%s", uuid.New()), - }.Build(), + Metadata: sharedMetadataWithName(fmt.Sprintf("my-%s", uuid.New())), Spec: privatev1.RoleSpec_builder{ Title: "My role", Description: "My role.", @@ -80,9 +78,7 @@ var _ = Describe("Role reconciler", func() { // Create the role: createResponse, err := client.Create(ctx, privatev1.RolesCreateRequest_builder{ Object: privatev1.Role_builder{ - Metadata: privatev1.Metadata_builder{ - Name: fmt.Sprintf("my-%s", uuid.New()), - }.Build(), + Metadata: sharedMetadataWithName(fmt.Sprintf("my-%s", uuid.New())), Spec: privatev1.RoleSpec_builder{ Title: "My role", Description: "My role.", diff --git a/it/it_tool.go b/it/it_tool.go index dcded283e..9a117b57b 100644 --- a/it/it_tool.go +++ b/it/it_tool.go @@ -2099,6 +2099,9 @@ func (t *Tool) registerHub(ctx context.Context) error { _, err = hubsClient.Create(ctx, privatev1.HubsCreateRequest_builder{ Object: privatev1.Hub_builder{ Id: hubId, + Metadata: privatev1.Metadata_builder{ + Tenant: auth.SharedTenant, + }.Build(), Spec: privatev1.HubSpec_builder{ Kubeconfig: hubKcBytes, Namespace: hubNamespace, diff --git a/it/it_version_test.go b/it/it_version_test.go index a4dfcadd4..e125a0b35 100644 --- a/it/it_version_test.go +++ b/it/it_version_test.go @@ -48,7 +48,8 @@ var _ = Describe("Version", func() { hostTypeId = fmt.Sprintf("my-host-type-%s", uuid.New()) _, err := hostTypesClient.Create(ctx, privatev1.HostTypesCreateRequest_builder{ Object: privatev1.HostType_builder{ - Id: hostTypeId, + Metadata: sharedMetadata(), + Id: hostTypeId, }.Build(), }.Build()) Expect(err).ToNot(HaveOccurred()) @@ -62,6 +63,7 @@ var _ = Describe("Version", func() { templateId = fmt.Sprintf("my-template-%s", uuid.New()) _, err = templatesClient.Create(ctx, privatev1.ClusterTemplatesCreateRequest_builder{ Object: privatev1.ClusterTemplate_builder{ + Metadata: sharedMetadata(), Id: templateId, Title: "My template", Description: "My template.", From ce63eca848b072b8d6432fe88886df4e16409cd4 Mon Sep 17 00:00:00 2001 From: Tommy Hughes Date: Mon, 15 Jun 2026 13:25:34 -0500 Subject: [PATCH 2/5] OSAC-496: Return PermissionDenied when explicit tenant is required Introduce auth.ErrExplicitTenantRequired and map it to PermissionDenied in determineAssignedTenant so universal-access callers get a client-actionable status instead of Internal. --- internal/auth/default_tenancy_logic.go | 2 +- internal/auth/default_tenancy_logic_test.go | 5 +- internal/auth/tenancy_logic.go | 7 +++ internal/servers/generic_server.go | 4 ++ internal/servers/servers_tenancy_test.go | 54 +++++++++++++++++++++ 5 files changed, 69 insertions(+), 3 deletions(-) diff --git a/internal/auth/default_tenancy_logic.go b/internal/auth/default_tenancy_logic.go index 5572fda91..24eb8fcdc 100644 --- a/internal/auth/default_tenancy_logic.go +++ b/internal/auth/default_tenancy_logic.go @@ -88,7 +88,7 @@ func (p *DefaultTenancyLogic) DetermineDefaultTenant(ctx context.Context) (resul "Subject has access to all tenants but no explicit tenant was provided", slog.String("user", subject.User), ) - err = fmt.Errorf("explicit tenant is required when subject has access to all tenants") + err = ErrExplicitTenantRequired return } inclusions := assignable.Inclusions() diff --git a/internal/auth/default_tenancy_logic_test.go b/internal/auth/default_tenancy_logic_test.go index fdcd0c0bf..65b6b191e 100644 --- a/internal/auth/default_tenancy_logic_test.go +++ b/internal/auth/default_tenancy_logic_test.go @@ -15,6 +15,7 @@ package auth import ( "context" + "errors" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -126,8 +127,8 @@ var _ = Describe("Default tenancy logic", func() { } ctx = ContextWithSubject(ctx, subject) _, err := logic.DetermineDefaultTenant(ctx) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("explicit tenant is required")) + Expect(err).To(MatchError(ErrExplicitTenantRequired)) + Expect(errors.Is(err, ErrExplicitTenantRequired)).To(BeTrue()) }) It("Fails if the subject has an empty tenants set", func() { diff --git a/internal/auth/tenancy_logic.go b/internal/auth/tenancy_logic.go index 9a83ddafa..bda4fdb5e 100644 --- a/internal/auth/tenancy_logic.go +++ b/internal/auth/tenancy_logic.go @@ -15,10 +15,17 @@ package auth import ( "context" + "errors" "github.com/osac-project/fulfillment-service/internal/collections" ) +// ErrExplicitTenantRequired is returned when a subject with universal tenant access +// creates or updates an object without specifying a tenant. +var ErrExplicitTenantRequired = errors.New( + "explicit tenant is required when subject has access to all tenants", +) + // TenancyLogic defines the logic for determining object tenancy and access control. // //go:generate mockgen -destination=tenancy_logic_mock.go -package=auth . TenancyLogic diff --git a/internal/servers/generic_server.go b/internal/servers/generic_server.go index 03b295b34..6646aeaa6 100644 --- a/internal/servers/generic_server.go +++ b/internal/servers/generic_server.go @@ -1242,6 +1242,10 @@ func (s *GenericServer[O]) determineAssignedTenant(ctx context.Context, // Determine the default tenant only when neither request nor current object specify one: defaultTenant, err := s.tenancyLogic.DetermineDefaultTenant(ctx) if err != nil { + if errors.Is(err, auth.ErrExplicitTenantRequired) { + err = grpcstatus.Error(grpccodes.PermissionDenied, err.Error()) + return + } s.logger.ErrorContext( ctx, "Failed to determine default tenant", diff --git a/internal/servers/servers_tenancy_test.go b/internal/servers/servers_tenancy_test.go index b435d7a38..fb4320ff0 100644 --- a/internal/servers/servers_tenancy_test.go +++ b/internal/servers/servers_tenancy_test.go @@ -357,6 +357,60 @@ var _ = Describe("Tenancy logic", func() { Expect(cluster.GetMetadata().GetTenant()).To(Equal("my-tenant")) }) + It("Rejects create without tenant when subject has access to all tenants", func() { + tenancy := auth.NewMockTenancyLogic(ctrl) + tenancy.EXPECT().DetermineAssignableTenants(gomock.Any()). + Return(auth.AllTenants, nil). + AnyTimes() + tenancy.EXPECT().DetermineVisibleTenants(gomock.Any()). + Return(auth.AllTenants, nil). + AnyTimes() + tenancy.EXPECT().DetermineDefaultTenant(gomock.Any()). + Return("", auth.ErrExplicitTenantRequired). + Times(1) + + templatesDao, err := dao.NewGenericDAO[*privatev1.ClusterTemplate](). + SetLogger(logger). + SetTenancyLogic(tenancy). + Build() + Expect(err).ToNot(HaveOccurred()) + _, err = templatesDao.Create(). + SetObject( + privatev1.ClusterTemplate_builder{ + Id: "my-template", + Title: "My template", + Description: "My template", + Metadata: privatev1.Metadata_builder{ + Tenant: "my-tenant", + }.Build(), + }.Build(), + ). + Do(ctx) + Expect(err).ToNot(HaveOccurred()) + + clustersServer, err := NewClustersServer(). + SetLogger(logger). + SetAttributionLogic(attribution). + SetTenancyLogic(tenancy). + SetScheme(testScheme). + Build() + Expect(err).ToNot(HaveOccurred()) + + response, err := clustersServer.Create(ctx, publicv1.ClustersCreateRequest_builder{ + Object: publicv1.Cluster_builder{ + Spec: publicv1.ClusterSpec_builder{ + Template: "my-template", + }.Build(), + }.Build(), + }.Build()) + Expect(response).To(BeNil()) + Expect(err).To(HaveOccurred()) + status, ok := grpcstatus.FromError(err) + Expect(ok).To(BeTrue()) + Expect(status.Code()).To(Equal(grpccodes.PermissionDenied)) + Expect(status.Message()).To(Equal(auth.ErrExplicitTenantRequired.Error())) + }) + It("Rejects object creation when tenant is visible to the user, but doesn't exist in the database", func() { // Create a tenancy logic that returns visible tenants: tenancy := auth.NewMockTenancyLogic(ctrl) From f7524bbd83c67bdcd57c7320d5480b3d7c0b83ff Mon Sep 17 00:00:00 2001 From: Tommy Hughes Date: Mon, 15 Jun 2026 15:29:18 -0500 Subject: [PATCH 3/5] OSAC-496: Set shared tenant on osac create hub by default Hub registration in CI and installer scripts uses the admin token, which now requires an explicit tenant. Default the create hub CLI to the shared tenant and accept an optional --tenant override. --- internal/cmd/cli/create/hub/create_hub_cmd.go | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/internal/cmd/cli/create/hub/create_hub_cmd.go b/internal/cmd/cli/create/hub/create_hub_cmd.go index 15c7f0c72..215893b8f 100644 --- a/internal/cmd/cli/create/hub/create_hub_cmd.go +++ b/internal/cmd/cli/create/hub/create_hub_cmd.go @@ -21,6 +21,7 @@ import ( "google.golang.org/protobuf/proto" privatev1 "github.com/osac-project/fulfillment-service/internal/api/osac/private/v1" + "github.com/osac-project/fulfillment-service/internal/auth" "github.com/osac-project/fulfillment-service/internal/config" "github.com/osac-project/fulfillment-service/internal/terminal" ) @@ -54,6 +55,12 @@ func Cmd() *cobra.Command { "", namespaceFlagHelp, ) + flags.StringVar( + &runner.tenant, + "tenant", + auth.SharedTenant, + tenantFlagHelp, + ) return result } @@ -62,6 +69,7 @@ type runnerContext struct { id string kubeconfig string namespace string + tenant string } func (c *runnerContext) run(cmd *cobra.Command, args []string) error { @@ -87,8 +95,8 @@ func (c *runnerContext) run(cmd *cobra.Command, args []string) error { if c.kubeconfig == "" { return fmt.Errorf("kubeconfig file is required") } - if c.namespace == "" { - return fmt.Errorf("namespace name is required") + if c.tenant == "" { + return fmt.Errorf("tenant is required") } // Create the gRPC connection from the configuration: @@ -110,6 +118,9 @@ func (c *runnerContext) run(cmd *cobra.Command, args []string) error { // Prepare the hub: hub := privatev1.Hub_builder{ Id: c.id, + Metadata: privatev1.Metadata_builder{ + Tenant: c.tenant, + }.Build(), Spec: privatev1.HubSpec_builder{ Kubeconfig: kubeconfig, Namespace: c.namespace, @@ -149,3 +160,7 @@ API. const namespaceFlagHelp = ` _NAMESPACE_ - Namespace where cluster orders will be created. ` + +const tenantFlagHelp = ` +_TENANT_ - Tenant that owns the hub. Defaults to the shared tenant. +` From 45f5aa7a3393afde3af51a7cfd51d5ebc3156a3a Mon Sep 17 00:00:00 2001 From: Tommy Hughes Date: Mon, 15 Jun 2026 15:39:10 -0500 Subject: [PATCH 4/5] OSAC-496: Use sharedMetadata() helper in hub registration IT Address CodeRabbit nitpick for consistent metadata construction in registerHub. --- it/it_tool.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/it/it_tool.go b/it/it_tool.go index 9a117b57b..f1417a78b 100644 --- a/it/it_tool.go +++ b/it/it_tool.go @@ -2098,10 +2098,8 @@ func (t *Tool) registerHub(ctx context.Context) error { // Create the hub: _, err = hubsClient.Create(ctx, privatev1.HubsCreateRequest_builder{ Object: privatev1.Hub_builder{ - Id: hubId, - Metadata: privatev1.Metadata_builder{ - Tenant: auth.SharedTenant, - }.Build(), + Id: hubId, + Metadata: sharedMetadata(), Spec: privatev1.HubSpec_builder{ Kubeconfig: hubKcBytes, Namespace: hubNamespace, From 9100a7075b893e23edc07b21410cb4f89492c39e Mon Sep 17 00:00:00 2001 From: Tommy Hughes Date: Mon, 15 Jun 2026 15:54:15 -0500 Subject: [PATCH 5/5] OSAC-496: Hardcode shared tenant on osac create hub Remove the --tenant flag and always set hub metadata tenant to shared. Platform hub registration only needs the shared tenant for now; a configurable tenant flag can follow in a separate change. --- internal/cmd/cli/create/hub/create_hub_cmd.go | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/internal/cmd/cli/create/hub/create_hub_cmd.go b/internal/cmd/cli/create/hub/create_hub_cmd.go index 215893b8f..2ec4b118c 100644 --- a/internal/cmd/cli/create/hub/create_hub_cmd.go +++ b/internal/cmd/cli/create/hub/create_hub_cmd.go @@ -55,12 +55,6 @@ func Cmd() *cobra.Command { "", namespaceFlagHelp, ) - flags.StringVar( - &runner.tenant, - "tenant", - auth.SharedTenant, - tenantFlagHelp, - ) return result } @@ -69,7 +63,6 @@ type runnerContext struct { id string kubeconfig string namespace string - tenant string } func (c *runnerContext) run(cmd *cobra.Command, args []string) error { @@ -95,9 +88,6 @@ func (c *runnerContext) run(cmd *cobra.Command, args []string) error { if c.kubeconfig == "" { return fmt.Errorf("kubeconfig file is required") } - if c.tenant == "" { - return fmt.Errorf("tenant is required") - } // Create the gRPC connection from the configuration: conn, err := cfg.Connect(ctx, cmd.Flags()) @@ -115,11 +105,11 @@ func (c *runnerContext) run(cmd *cobra.Command, args []string) error { return fmt.Errorf("failed to read kubeconfig file '%s': %w", c.kubeconfig, err) } - // Prepare the hub: + // Platform hubs live in the shared tenant for broad visibility. hub := privatev1.Hub_builder{ Id: c.id, Metadata: privatev1.Metadata_builder{ - Tenant: c.tenant, + Tenant: auth.SharedTenant, }.Build(), Spec: privatev1.HubSpec_builder{ Kubeconfig: kubeconfig, @@ -160,7 +150,3 @@ API. const namespaceFlagHelp = ` _NAMESPACE_ - Namespace where cluster orders will be created. ` - -const tenantFlagHelp = ` -_TENANT_ - Tenant that owns the hub. Defaults to the shared tenant. -`