Skip to content
This repository was archived by the owner on Jun 2, 2026. It is now read-only.

Commit bbe58c3

Browse files
pbretonpbreton
committed
more fixes
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
1 parent 17a0382 commit bbe58c3

3 files changed

Lines changed: 136 additions & 57 deletions

File tree

‎api/pkg/api/handler/operatingsystem.go‎

Lines changed: 91 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -113,21 +113,23 @@ func (csh CreateOperatingSystemHandler) Handle(c echo.Context) error {
113113
}
114114

115115
// Role check: either Provider Admin or Tenant Admin can create OSes.
116-
// When both roles are present, Provider Admin takes priority — the OS is created as
117-
// provider-owned (infrastructure_provider_id set, tenant_id nil).
116+
// Provider Admin is limited to iPXE Template-based OSes; Tenant Admin may
117+
// create any type. Both roles are evaluated independently so a dual-role
118+
// caller is permitted if either role authorizes the operation. When both
119+
// allow the action, Provider Admin takes priority (provider-owned OS).
118120
isProviderAdmin := auth.ValidateUserRoles(dbUser, org, nil, auth.ProviderAdminRole)
119121
isTenantAdmin := auth.ValidateUserRoles(dbUser, org, nil, auth.TenantAdminRole)
120-
if !isProviderAdmin && !isTenantAdmin {
122+
allowedByProvider := isProviderAdmin && apiRequest.IpxeTemplateName != nil
123+
allowedByTenant := isTenantAdmin
124+
if !allowedByProvider && !allowedByTenant {
125+
if isProviderAdmin {
126+
logger.Warn().Msg("provider admin attempted to create non-template OS without tenant admin role")
127+
return cutil.NewAPIErrorResponse(c, http.StatusForbidden, "Provider Admin can only create iPXE Template-based Operating Systems", nil)
128+
}
121129
logger.Warn().Msg("user has neither Provider Admin nor Tenant Admin role, access denied")
122130
return cutil.NewAPIErrorResponse(c, http.StatusForbidden, "Creating Operating Systems requires Provider Admin or Tenant Admin role", nil)
123131
}
124-
125-
// Provider Admin may only create iPXE Template-based Operating Systems.
126-
// Raw iPXE scripts and image-based OSes are restricted to Tenant Admins.
127-
if isProviderAdmin && apiRequest.IpxeTemplateName == nil {
128-
logger.Warn().Msg("provider admin attempted to create non-template OS")
129-
return cutil.NewAPIErrorResponse(c, http.StatusForbidden, "Provider Admin can only create iPXE Template-based Operating Systems", nil)
130-
}
132+
actingAsProvider := allowedByProvider
131133

132134
// Validate request attributes
133135
verr := apiRequest.Validate()
@@ -169,7 +171,7 @@ func (csh CreateOperatingSystemHandler) Handle(c echo.Context) error {
169171
// Look up InfrastructureProvider now if acting as provider — used for ownership assignment,
170172
// name-uniqueness check, and site validation throughout the rest of the handler.
171173
var ip *cdbm.InfrastructureProvider
172-
if isProviderAdmin {
174+
if actingAsProvider {
173175
ipVal, iperr := common.GetInfrastructureProviderForOrg(ctx, nil, csh.dbSession, org)
174176
if iperr != nil {
175177
logger.Error().Err(iperr).Msg("error retrieving Infrastructure Provider for org")
@@ -181,7 +183,7 @@ func (csh CreateOperatingSystemHandler) Handle(c echo.Context) error {
181183
// Check for name uniqueness within the owner's scope.
182184
osDAO := cdbm.NewOperatingSystemDAO(csh.dbSession)
183185
var uniquenessFilter cdbm.OperatingSystemFilterInput
184-
if isProviderAdmin && ip != nil {
186+
if actingAsProvider && ip != nil {
185187
uniquenessFilter = cdbm.OperatingSystemFilterInput{
186188
InfrastructureProviderIDs: []uuid.UUID{ip.ID},
187189
Names: []string{apiRequest.Name},
@@ -245,7 +247,7 @@ func (csh CreateOperatingSystemHandler) Handle(c echo.Context) error {
245247
// Determine how to validate the sites:
246248
// - limited-scope iPXE or provider image: sites must belong to the provider
247249
// - tenant image: sites must be in TenantSite
248-
isProviderSiteValidation := isLimited || (osType == cdbm.OperatingSystemTypeImage && isProviderAdmin)
250+
isProviderSiteValidation := isLimited || (osType == cdbm.OperatingSystemTypeImage && actingAsProvider)
249251

250252
var providerSiteID uuid.UUID
251253
if isProviderSiteValidation {
@@ -355,7 +357,7 @@ func (csh CreateOperatingSystemHandler) Handle(c echo.Context) error {
355357
// This aligns with the sync model where OSes from carbide-core are provider-owned.
356358
var ownerTenantID *uuid.UUID
357359
var ownerProviderID *uuid.UUID
358-
if isProviderAdmin {
360+
if actingAsProvider {
359361
if ip == nil {
360362
// ip may not yet be resolved if the request had no scope/sites requiring it.
361363
ipVal, iperr := common.GetInfrastructureProviderForOrg(ctx, nil, csh.dbSession, org)
@@ -1179,25 +1181,35 @@ func (ush UpdateOperatingSystemHandler) Handle(c echo.Context) error {
11791181
return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to retrieve tenant for org", nil)
11801182
}
11811183

1182-
// Enforce ownership: Provider Admin takes priority when both roles are present (same rule as Create).
1183-
// Provider Admin can only update provider-owned OSes; Tenant Admin can only update tenant-owned OSes.
1184+
// Enforce ownership: both roles are evaluated independently so a dual-role
1185+
// caller is permitted if either role authorizes the operation.
11841186
ownedByTenant := os.TenantID != nil && *os.TenantID == tenant.ID && os.InfrastructureProviderID == nil
1185-
ownedByProvider := os.InfrastructureProviderID != nil
1186-
if isProviderAdmin {
1187-
// Provider Admin path: must own the OS via infrastructure_provider_id matching their provider.
1188-
if !ownedByProvider {
1189-
provider, iperr := common.GetInfrastructureProviderForOrg(ctx, nil, ush.dbSession, org)
1190-
if iperr != nil || os.InfrastructureProviderID == nil || *os.InfrastructureProviderID != provider.ID {
1191-
logger.Warn().Msg("provider admin cannot update tenant-owned operating system")
1192-
return cutil.NewAPIErrorResponse(c, http.StatusForbidden, "Provider Admin can only update provider-owned Operating Systems", nil)
1193-
}
1187+
ownedByProvider := false
1188+
if isProviderAdmin && os.InfrastructureProviderID != nil {
1189+
ip, iperr := common.GetInfrastructureProviderForOrg(ctx, nil, ush.dbSession, org)
1190+
if iperr != nil {
1191+
logger.Error().Err(iperr).Msg("error retrieving Infrastructure Provider for org")
1192+
return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to retrieve Infrastructure Provider for org", nil)
11941193
}
1195-
} else {
1196-
// Tenant Admin (only) path: must own the OS via tenant_id and it must not be provider-owned.
1197-
if !ownedByTenant || ownedByProvider {
1194+
if *os.InfrastructureProviderID != ip.ID {
1195+
logger.Warn().Msg("provider admin cannot update operating system owned by a different provider")
1196+
return cutil.NewAPIErrorResponse(c, http.StatusForbidden, "Provider Admin can only update Operating Systems owned by their own provider", nil)
1197+
}
1198+
ownedByProvider = true
1199+
}
1200+
allowedByProvider := isProviderAdmin && ownedByProvider
1201+
allowedByTenant := isTenantAdmin && ownedByTenant
1202+
if !allowedByProvider && !allowedByTenant {
1203+
if isProviderAdmin && !isTenantAdmin {
1204+
logger.Warn().Msg("provider admin cannot update tenant-owned operating system")
1205+
return cutil.NewAPIErrorResponse(c, http.StatusForbidden, "Provider Admin can only update provider-owned Operating Systems", nil)
1206+
}
1207+
if isTenantAdmin && !isProviderAdmin {
11981208
logger.Warn().Msg("tenant admin cannot update provider-owned operating system")
11991209
return cutil.NewAPIErrorResponse(c, http.StatusForbidden, "Tenant Admin can only update their own Operating Systems", nil)
12001210
}
1211+
logger.Warn().Msg("user does not have permission to update this operating system")
1212+
return cutil.NewAPIErrorResponse(c, http.StatusForbidden, "Operating System does not belong to your tenant or infrastructure provider", nil)
12011213
}
12021214

12031215
// check for name uniqueness for the tenant, ie, tenant cannot have another os with same name
@@ -1567,6 +1579,32 @@ func (ush UpdateOperatingSystemHandler) Handle(c echo.Context) error {
15671579
}
15681580

15691581
for _, dbossa := range dbossas {
1582+
_, err = ossaDAO.Update(
1583+
ctx,
1584+
tx,
1585+
cdbm.OperatingSystemSiteAssociationUpdateInput{
1586+
OperatingSystemSiteAssociationID: dbossa.ID,
1587+
Status: cdb.GetStrPtr(cdbm.OperatingSystemSiteAssociationStatusSyncing),
1588+
},
1589+
)
1590+
if err != nil {
1591+
logger.Error().Err(err).Msg("unable to update the Operating System association record in DB")
1592+
return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to update Operating System Site Association status, DB error", nil)
1593+
}
1594+
1595+
_, serr = sdDAO.CreateFromParams(ctx, tx, dbossa.ID.String(), *cdb.GetStrPtr(cdbm.OperatingSystemSiteAssociationStatusSyncing),
1596+
cdb.GetStrPtr("received Operating System Association update request, syncing"))
1597+
if serr != nil {
1598+
logger.Error().Err(serr).Msg("error creating Status Detail DB entry")
1599+
return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to create Status Detail for Operating System Site Association", nil)
1600+
}
1601+
1602+
updatedOssa, err := ossaDAO.GenerateAndUpdateVersion(ctx, tx, dbossa.ID)
1603+
if err != nil {
1604+
logger.Error().Err(err).Msg("error updating version for updated Operating System Association")
1605+
return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to set version for updated Operating System Site Association, DB error", nil)
1606+
}
1607+
15701608
stc, err := ush.scp.GetClientByID(dbossa.SiteID)
15711609
if err != nil {
15721610
logger.Error().Err(err).Msg("failed to retrieve Temporal client for Site")
@@ -1589,7 +1627,7 @@ func (ush UpdateOperatingSystemHandler) Handle(c echo.Context) error {
15891627
}
15901628

15911629
workflowOptions := temporalClient.StartWorkflowOptions{
1592-
ID: "ipxe-os-update-" + dbossa.SiteID.String() + "-" + uos.ID.String(),
1630+
ID: "ipxe-os-update-" + updatedOssa.SiteID.String() + "-" + uos.ID.String() + "-" + *updatedOssa.Version,
15931631
WorkflowExecutionTimeout: cutil.WorkflowExecutionTimeout,
15941632
TaskQueue: queue.SiteTaskQueue,
15951633
}
@@ -1735,25 +1773,35 @@ func (dsh DeleteOperatingSystemHandler) Handle(c echo.Context) error {
17351773
return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Could not retrieve Operating System to delete", nil)
17361774
}
17371775

1738-
// Enforce ownership: Provider Admin takes priority when both roles are present (same rule as Create).
1739-
// Provider Admin can only delete provider-owned OSes; Tenant Admin can only delete tenant-owned OSes.
1776+
// Enforce ownership: both roles are evaluated independently so a dual-role
1777+
// caller is permitted if either role authorizes the operation.
17401778
ownedByTenantD := os.TenantID != nil && *os.TenantID == tenant.ID && os.InfrastructureProviderID == nil
1741-
ownedByProviderD := os.InfrastructureProviderID != nil
1742-
if isProviderAdmin {
1743-
// Provider Admin path: must own the OS via infrastructure_provider_id matching their provider.
1744-
if !ownedByProviderD {
1745-
provider, iperr := common.GetInfrastructureProviderForOrg(ctx, nil, dsh.dbSession, org)
1746-
if iperr != nil || os.InfrastructureProviderID == nil || *os.InfrastructureProviderID != provider.ID {
1747-
logger.Warn().Msg("provider admin cannot delete tenant-owned operating system")
1748-
return cutil.NewAPIErrorResponse(c, http.StatusForbidden, "Provider Admin can only delete provider-owned Operating Systems", nil)
1749-
}
1779+
ownedByProviderD := false
1780+
if isProviderAdmin && os.InfrastructureProviderID != nil {
1781+
ip, iperr := common.GetInfrastructureProviderForOrg(ctx, nil, dsh.dbSession, org)
1782+
if iperr != nil {
1783+
logger.Error().Err(iperr).Msg("error retrieving Infrastructure Provider for org")
1784+
return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to retrieve Infrastructure Provider for org", nil)
17501785
}
1751-
} else {
1752-
// Tenant Admin (only) path: must own the OS via tenant_id and it must not be provider-owned.
1753-
if !ownedByTenantD || ownedByProviderD {
1786+
if *os.InfrastructureProviderID != ip.ID {
1787+
logger.Warn().Msg("provider admin cannot delete operating system owned by a different provider")
1788+
return cutil.NewAPIErrorResponse(c, http.StatusForbidden, "Provider Admin can only delete Operating Systems owned by their own provider", nil)
1789+
}
1790+
ownedByProviderD = true
1791+
}
1792+
allowedByProvider := isProviderAdmin && ownedByProviderD
1793+
allowedByTenant := isTenantAdmin && ownedByTenantD
1794+
if !allowedByProvider && !allowedByTenant {
1795+
if isProviderAdmin && !isTenantAdmin {
1796+
logger.Warn().Msg("provider admin cannot delete tenant-owned operating system")
1797+
return cutil.NewAPIErrorResponse(c, http.StatusForbidden, "Provider Admin can only delete provider-owned Operating Systems", nil)
1798+
}
1799+
if isTenantAdmin && !isProviderAdmin {
17541800
logger.Warn().Msg("tenant admin cannot delete provider-owned operating system")
17551801
return cutil.NewAPIErrorResponse(c, http.StatusForbidden, "Tenant Admin can only delete their own Operating Systems", nil)
17561802
}
1803+
logger.Warn().Msg("user does not have permission to delete this operating system")
1804+
return cutil.NewAPIErrorResponse(c, http.StatusForbidden, "Operating System does not belong to your tenant or infrastructure provider", nil)
17571805
}
17581806

17591807
// Retrieve site associations for this Operating System (both Image and iPXE types

‎api/pkg/api/handler/operatingsystem_ownership_test.go‎

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -23,12 +23,14 @@
2323
// Roles under test
2424
// - ipUser — FORGE_PROVIDER_ADMIN only
2525
// - tnUser — FORGE_TENANT_ADMIN only
26-
// - dualUser — both roles (provider takes priority per handler logic)
26+
// - dualUser — both roles (either role may authorize the operation)
2727
//
2828
// Ownership invariants verified
2929
// - Provider Admin → InfrastructureProviderID set, TenantID nil
3030
// - Tenant Admin → TenantID set, InfrastructureProviderID nil
31-
// - Dual-role → treated as Provider Admin
31+
// - Dual-role → permitted if either role authorizes the action;
32+
// when both allow it, Provider Admin takes priority
33+
// for ownership assignment
3234
//
3335
// Cross-ownership visibility (GetAll / GetByID)
3436
// - Any user whose org has both a Tenant and an InfrastructureProvider
@@ -37,7 +39,7 @@
3739
// Mutation enforcement (Update / Delete)
3840
// - Provider Admin can mutate only provider-owned OSes.
3941
// - Tenant Admin can mutate only tenant-owned OSes.
40-
// - Dual-role user follows the provider path.
42+
// - Dual-role user is permitted if either role authorizes the action.
4143

4244
package handler
4345

@@ -199,7 +201,8 @@ func otelTraceCtx(t *testing.T) (interface{}, context.Context) {
199201
//
200202
// - Provider Admin → InfrastructureProviderID = provider's ID, TenantID = nil
201203
// - Tenant Admin → TenantID = tenant's ID, InfrastructureProviderID = nil
202-
// - Dual-role user → treated as Provider Admin (provider takes priority)
204+
// - Dual-role user → permitted if either role authorizes the action;
205+
// when both allow it, provider ownership takes priority
203206
//
204207
// The test also covers the "new" iPXE OS type (template-based with parameters
205208
// and artifacts) to ensure those fields round-trip correctly.
@@ -280,13 +283,15 @@ func TestOperatingSystemHandler_Create_ProviderAndTenantOwnership(t *testing.T)
280283
wantProviderNil: true,
281284
},
282285
{
283-
name: "dual-role user raw iPXE → forbidden (provider takes priority, must use template)",
286+
name: "dual-role user raw iPXE → tenant-owned (tenant role authorizes)",
284287
user: env.dualUser,
285288
body: model.APIOperatingSystemCreateRequest{
286289
Name: "dual-ipxe-" + uuid.NewString(),
287290
IpxeScript: &ipxeScript,
288291
},
289-
wantStatus: http.StatusForbidden,
292+
wantStatus: http.StatusCreated,
293+
wantTenantID: &env.tenant.ID,
294+
wantProviderNil: true,
290295
},
291296
{
292297
name: "dual-role user template iPXE → provider-owned",
@@ -543,7 +548,7 @@ func TestOperatingSystemHandler_GetByID_CrossOwnership(t *testing.T) {
543548
//
544549
// - Provider Admin can update only provider-owned OSes → 200 / 403
545550
// - Tenant Admin can update only tenant-owned OSes → 200 / 403
546-
// - Dual-role user follows the provider path → 200 for provider-owned
551+
// - Dual-role user is permitted if either role authorizes the action
547552
func TestOperatingSystemHandler_Update_OwnershipEnforcement(t *testing.T) {
548553
env := newOwnershipTestEnv(t)
549554
ctx := context.Background()
@@ -630,14 +635,14 @@ func TestOperatingSystemHandler_Update_OwnershipEnforcement(t *testing.T) {
630635
assert.Equal(t, http.StatusForbidden, rec.Code, rec.Body.String())
631636
})
632637

633-
t.Run("dual-role user updates provider-owned OS → 200 (provider takes priority)", func(t *testing.T) {
638+
t.Run("dual-role user updates provider-owned OS → 200 (provider role authorizes)", func(t *testing.T) {
634639
rec := execUpdate(t, env.dualUser, provOS.ID.String())
635640
assert.Equal(t, http.StatusOK, rec.Code, rec.Body.String())
636641
})
637642

638-
t.Run("dual-role user updates tenant-owned OS → 403 (provider takes priority, cannot update tenant-owned)", func(t *testing.T) {
643+
t.Run("dual-role user updates tenant-owned OS → 200 (tenant role authorizes)", func(t *testing.T) {
639644
rec := execUpdate(t, env.dualUser, tnOS.ID.String())
640-
assert.Equal(t, http.StatusForbidden, rec.Code, rec.Body.String())
645+
assert.Equal(t, http.StatusOK, rec.Code, rec.Body.String())
641646
})
642647
}
643648

@@ -651,7 +656,8 @@ func TestOperatingSystemHandler_Update_OwnershipEnforcement(t *testing.T) {
651656
// - Provider Admin deletes tenant-owned OS → 403
652657
// - Tenant Admin deletes tenant-owned OS → 202
653658
// - Tenant Admin deletes provider-owned OS → 403
654-
// - Dual-role user deletes provider-owned OS → 202 (provider takes priority)
659+
// - Dual-role user deletes provider-owned OS → 202 (provider role authorizes)
660+
// - Dual-role user deletes tenant-owned OS → 202 (tenant role authorizes)
655661
func TestOperatingSystemHandler_Delete_OwnershipEnforcement(t *testing.T) {
656662
env := newOwnershipTestEnv(t)
657663
ctx := context.Background()
@@ -740,15 +746,15 @@ func TestOperatingSystemHandler_Delete_OwnershipEnforcement(t *testing.T) {
740746
assert.Equal(t, http.StatusForbidden, rec.Code, rec.Body.String())
741747
})
742748

743-
t.Run("dual-role user deletes provider-owned OS → 202 (provider takes priority)", func(t *testing.T) {
749+
t.Run("dual-role user deletes provider-owned OS → 202 (provider role authorizes)", func(t *testing.T) {
744750
os := newProvOS(uuid.NewString())
745751
rec := execDelete(t, env.dualUser, os.ID.String())
746752
assert.Equal(t, http.StatusAccepted, rec.Code, rec.Body.String())
747753
})
748754

749-
t.Run("dual-role user deletes tenant-owned OS → 403 (provider path, cannot delete tenant-owned)", func(t *testing.T) {
755+
t.Run("dual-role user deletes tenant-owned OS → 202 (tenant role authorizes)", func(t *testing.T) {
750756
os := newTnOS(uuid.NewString())
751757
rec := execDelete(t, env.dualUser, os.ID.String())
752-
assert.Equal(t, http.StatusForbidden, rec.Code, rec.Body.String())
758+
assert.Equal(t, http.StatusAccepted, rec.Code, rec.Body.String())
753759
})
754760
}

‎db/pkg/db/model/operatingsystem.go‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,11 @@ type OperatingSystemClearInput struct {
240240
IpxeScript bool
241241
UserData bool
242242
DeactivationNote bool
243+
IpxeTemplateName bool
244+
IpxeParameters bool
245+
IpxeArtifacts bool
246+
DefinitionHash bool
247+
Scope bool
243248
}
244249

245250
type OperatingSystemFilterInput struct {
@@ -763,6 +768,26 @@ func (ossd OperatingSystemSQLDAO) Clear(ctx context.Context, tx *db.Tx, input Op
763768
it.DeactivationNote = nil
764769
updatedFields = append(updatedFields, "deactivation_note")
765770
}
771+
if input.IpxeTemplateName {
772+
it.IpxeTemplateName = nil
773+
updatedFields = append(updatedFields, "ipxe_template_name")
774+
}
775+
if input.IpxeParameters {
776+
it.IpxeParameters = nil
777+
updatedFields = append(updatedFields, "ipxe_parameters")
778+
}
779+
if input.IpxeArtifacts {
780+
it.IpxeArtifacts = nil
781+
updatedFields = append(updatedFields, "ipxe_artifacts")
782+
}
783+
if input.DefinitionHash {
784+
it.DefinitionHash = nil
785+
updatedFields = append(updatedFields, "definition_hash")
786+
}
787+
if input.Scope {
788+
it.Scope = nil
789+
updatedFields = append(updatedFields, "scope")
790+
}
766791

767792
if len(updatedFields) > 0 {
768793
updatedFields = append(updatedFields, "updated")

0 commit comments

Comments
 (0)