Skip to content

Commit 6a09aa8

Browse files
authored
ROX-13466: return unauthorized when deleting central in org but not owned (#2432)
return unauthorized when deleting central in org but not owned
1 parent f5a39c4 commit 6a09aa8

File tree

4 files changed

+32
-30
lines changed

4 files changed

+32
-30
lines changed

internal/central/pkg/handlers/admin_central.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,11 @@ func (h adminCentralHandler) Delete(w http.ResponseWriter, r *http.Request) {
195195
Action: func() (i interface{}, serviceError *errors.ServiceError) {
196196
id := mux.Vars(r)["id"]
197197
ctx := r.Context()
198-
err := h.service.RegisterCentralDeprovisionJob(ctx, id)
198+
centralRequest, err := h.service.Get(ctx, id)
199+
if err != nil {
200+
return nil, err
201+
}
202+
err = h.service.RegisterCentralDeprovisionJob(ctx, centralRequest)
199203
h.telemetry.TrackDeletionRequested(ctx, id, true, err.AsError())
200204
return nil, err
201205
},

internal/central/pkg/handlers/central.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ func (h centralHandler) Delete(w http.ResponseWriter, r *http.Request) {
108108
if err != nil {
109109
return nil, err
110110
}
111-
err = h.service.RegisterCentralDeprovisionJob(ctx, id)
111+
err = h.service.RegisterCentralDeprovisionJob(ctx, centralRequest)
112112
if !centralRequest.Internal {
113113
h.telemetry.TrackDeletionRequested(ctx, id, false, err.AsError())
114114
}

internal/central/pkg/services/central.go

Lines changed: 12 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ type CentralService interface {
103103
ChangeCentralCNAMErecords(centralRequest *dbapi.CentralRequest, action CentralRoutesAction) (*route53.ChangeResourceRecordSetsOutput, *errors.ServiceError)
104104
GetCNAMERecordStatus(centralRequest *dbapi.CentralRequest) (*CNameRecordStatus, error)
105105
DetectInstanceType(centralRequest *dbapi.CentralRequest) types.CentralInstanceType
106-
RegisterCentralDeprovisionJob(ctx context.Context, id string) *errors.ServiceError
106+
RegisterCentralDeprovisionJob(ctx context.Context, centralRequest *dbapi.CentralRequest) *errors.ServiceError
107107
// DeprovisionCentralForUsers registers all centrals for deprovisioning given the list of owners
108108
DeprovisionCentralForUsers(users []string) *errors.ServiceError
109109
DeprovisionExpiredCentrals() *errors.ServiceError
@@ -482,8 +482,8 @@ func (k *centralService) GetByID(id string) (*dbapi.CentralRequest, *errors.Serv
482482
}
483483

484484
// RegisterCentralDeprovisionJob registers a central deprovision job in the central table
485-
func (k *centralService) RegisterCentralDeprovisionJob(ctx context.Context, id string) *errors.ServiceError {
486-
if id == "" {
485+
func (k *centralService) RegisterCentralDeprovisionJob(ctx context.Context, centralRequest *dbapi.CentralRequest) *errors.ServiceError {
486+
if centralRequest.ID == "" {
487487
return errors.Validation("id is undefined")
488488
}
489489

@@ -493,29 +493,27 @@ func (k *centralService) RegisterCentralDeprovisionJob(ctx context.Context, id s
493493
return errors.NewWithCause(errors.ErrorUnauthenticated, err, "user not authenticated")
494494
}
495495

496-
dbConn := k.connectionFactory.New()
497-
496+
isAuthorizedToDelete := false
498497
if auth.GetIsAdminFromContext(ctx) {
499-
dbConn = dbConn.Where("id = ?", id)
498+
isAuthorizedToDelete = true
500499
} else if claims.IsOrgAdmin() {
501500
orgID, _ := claims.GetOrgID()
502-
dbConn = dbConn.Where("id = ?", id).Where("organisation_id = ?", orgID)
501+
isAuthorizedToDelete = centralRequest.OrganisationID == orgID
503502
} else {
504503
user, _ := claims.GetUsername()
505-
dbConn = dbConn.Where("id = ?", id).Where("owner = ? ", user)
504+
isAuthorizedToDelete = centralRequest.Owner == user
506505
}
507506

508-
var centralRequest dbapi.CentralRequest
509-
if err := dbConn.First(&centralRequest).Error; err != nil {
510-
return services.HandleGetError("CentralResource", "id", id, err)
507+
if !isAuthorizedToDelete {
508+
return errors.Unauthorized("user not authorized to delete central")
511509
}
512-
metrics.IncreaseCentralTotalOperationsCountMetric(constants.CentralOperationDeprovision)
513510

511+
metrics.IncreaseCentralTotalOperationsCountMetric(constants.CentralOperationDeprovision)
514512
deprovisionStatus := constants.CentralRequestStatusDeprovision
515513

516-
if executed, err := k.UpdateStatus(id, deprovisionStatus); executed {
514+
if executed, err := k.UpdateStatus(centralRequest.ID, deprovisionStatus); executed {
517515
if err != nil {
518-
return services.HandleGetError("CentralResource", "id", id, err)
516+
return services.HandleGetError("CentralResource", "id", centralRequest.ID, err)
519517
}
520518
metrics.IncreaseCentralSuccessOperationsCountMetric(constants.CentralOperationDeprovision)
521519
metrics.UpdateCentralRequestsStatusSinceCreatedMetric(deprovisionStatus, centralRequest.ID, centralRequest.ClusterID, time.Since(centralRequest.CreatedAt))

internal/central/pkg/services/centralservice_moq.go

Lines changed: 14 additions & 14 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)