diff --git a/rest-api/api/pkg/api/handler/expectedrack.go b/rest-api/api/pkg/api/handler/expectedrack.go index 0ac50b6578..842fe6baa1 100644 --- a/rest-api/api/pkg/api/handler/expectedrack.go +++ b/rest-api/api/pkg/api/handler/expectedrack.go @@ -582,27 +582,27 @@ func (uerh UpdateExpectedRackHandler) Handle(c echo.Context) error { return cutil.NewAPIErrorResponse(c, http.StatusForbidden, "Current org is not associated with the Site of the Expected Rack", nil) } - // If RackID is changing, ensure the new value is not already taken in this site + // RackID is immutable: Core and Flow identify expected racks by rackId, so + // PATCH may reassert the existing identity but cannot replace it. A rename + // would first mutate the Cloud record and only then fail in Core's lookup + // by the new rackId, so the mismatch is rejected here before any database + // write or workflow trigger. This mirrors the ExpectedMachine BMC MAC + // identity boundary. if apiRequest.RackID != nil && *apiRequest.RackID != expectedRack.RackID { - _, count, err := erDAO.GetAll(ctx, nil, cdbm.ExpectedRackFilterInput{ - SiteIDs: []uuid.UUID{expectedRack.SiteID}, - RackIDs: []string{*apiRequest.RackID}, - }, paginator.PageInput{Limit: cutil.GetPtr(1)}, nil) - if err != nil { - logger.Error().Err(err).Msg("error checking for duplicate Expected Rack") - return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to validate Expected Rack uniqueness due to DB error", nil) - } - if count > 0 { - return cutil.NewAPIErrorResponse(c, http.StatusConflict, "Expected Rack with specified RackID already exists for Site", validation.Errors{ - "rackId": errors.New(*apiRequest.RackID), - }) - } + logger.Warn(). + Str("requestRackID", *apiRequest.RackID). + Str("currentRackID", expectedRack.RackID). + Msg("RackID cannot be changed after creation") + return cutil.NewAPIErrorResponse(c, http.StatusBadRequest, "Failed to validate ExpectedRack update request data", validation.Errors{ + "rackId": errors.New("RackID cannot be changed after creation"), + }) } - // Build update input from request, mapping flat API fields to DAO fields + // Build update input from request, mapping flat API fields to DAO fields. + // RackID is intentionally not passed through: it is immutable, so the DAO + // update path is structurally incapable of renaming an Expected Rack. updateInput := cdbm.ExpectedRackUpdateInput{ ExpectedRackID: expectedRack.ID, - RackID: apiRequest.RackID, RackProfileID: apiRequest.RackProfileID, Name: apiRequest.Name, Description: apiRequest.Description, diff --git a/rest-api/api/pkg/api/handler/expectedrack_test.go b/rest-api/api/pkg/api/handler/expectedrack_test.go index 4a839ee9b0..8ef125dc5f 100644 --- a/rest-api/api/pkg/api/handler/expectedrack_test.go +++ b/rest-api/api/pkg/api/handler/expectedrack_test.go @@ -844,7 +844,7 @@ func TestUpdateExpectedRackHandler_Handle(t *testing.T) { assert.Nil(t, err) assert.NotNil(t, testER2) - // A third ExpectedRack to anchor the duplicate-rack-id update test + // A third ExpectedRack to anchor the rack_id rename-rejection test testER3, err := erDAO.Create(ctx, nil, cdbm.ExpectedRackCreateInput{ ExpectedRackID: uuid.New(), RackID: "update-rack-003", @@ -894,11 +894,12 @@ func TestUpdateExpectedRackHandler_Handle(t *testing.T) { } tests := []struct { - name string - id string - requestBody model.APIExpectedRackUpdateRequest - setupContext func(c echo.Context) - expectedStatus int + name string + id string + requestBody model.APIExpectedRackUpdateRequest + setupContext func(c echo.Context) + expectedStatus int + checkResponseContent func(t *testing.T, body []byte) }{ { name: "successful update of rack_profile_id", @@ -932,7 +933,7 @@ func TestUpdateExpectedRackHandler_Handle(t *testing.T) { expectedStatus: http.StatusOK, }, { - name: "successful update of rack_id (operator-supplied identifier)", + name: "rack_id cannot be changed (immutable)", id: testER3.ID.String(), requestBody: model.APIExpectedRackUpdateRequest{ RackID: cutil.GetPtr("update-rack-003-renamed"), @@ -942,13 +943,17 @@ func TestUpdateExpectedRackHandler_Handle(t *testing.T) { c.SetParamNames("orgName", "id") c.SetParamValues(org, testER3.ID.String()) }, - expectedStatus: http.StatusOK, + expectedStatus: http.StatusBadRequest, + checkResponseContent: func(t *testing.T, body []byte) { + assert.Contains(t, string(body), "RackID cannot be changed after creation") + }, }, { - name: "duplicate (siteId, rackId) on update should return 409", + name: "rack_id cannot be changed even to an existing value", id: testER3.ID.String(), requestBody: model.APIExpectedRackUpdateRequest{ - // testER's RackID is already taken in this site + // testER's RackID is already taken in this site, but the rename + // is rejected as immutable before any duplicate check. RackID: cutil.GetPtr("update-rack-001"), }, setupContext: func(c echo.Context) { @@ -956,7 +961,31 @@ func TestUpdateExpectedRackHandler_Handle(t *testing.T) { c.SetParamNames("orgName", "id") c.SetParamValues(org, testER3.ID.String()) }, - expectedStatus: http.StatusConflict, + expectedStatus: http.StatusBadRequest, + checkResponseContent: func(t *testing.T, body []byte) { + assert.Contains(t, string(body), "RackID cannot be changed after creation") + }, + }, + { + name: "identical rack_id remains compatible", + id: testER2.ID.String(), + requestBody: model.APIExpectedRackUpdateRequest{ + RackID: cutil.GetPtr("update-rack-002"), + RackProfileID: cutil.GetPtr("profile-update-identical-rack-id"), + }, + setupContext: func(c echo.Context) { + c.Set("user", createMockUser(org)) + c.SetParamNames("orgName", "id") + c.SetParamValues(org, testER2.ID.String()) + }, + expectedStatus: http.StatusOK, + checkResponseContent: func(t *testing.T, body []byte) { + var response model.APIExpectedRack + err := json.Unmarshal(body, &response) + assert.Nil(t, err) + assert.Equal(t, "update-rack-002", response.RackID, + "reasserting the identity must preserve the stored rackId") + }, }, { name: "body ID mismatch with URL should return 400", @@ -1012,6 +1041,22 @@ func TestUpdateExpectedRackHandler_Handle(t *testing.T) { }, expectedStatus: http.StatusForbidden, }, + { + name: "site access is checked before rack_id identity", + id: unmanagedER.ID.String(), + requestBody: model.APIExpectedRackUpdateRequest{ + RackID: cutil.GetPtr("update-rack-unmanaged-renamed"), + }, + setupContext: func(c echo.Context) { + c.Set("user", createMockUser(org)) + c.SetParamNames("orgName", "id") + c.SetParamValues(org, unmanagedER.ID.String()) + }, + expectedStatus: http.StatusForbidden, + checkResponseContent: func(t *testing.T, body []byte) { + assert.NotContains(t, string(body), "RackID cannot be changed after creation") + }, + }, { name: "rack not found", id: "12345678-1234-1234-1234-123456789099", @@ -1050,10 +1095,129 @@ func TestUpdateExpectedRackHandler_Handle(t *testing.T) { if tt.expectedStatus != rec.Code { t.Errorf("Response: %v", rec.Body.String()) } + if tt.checkResponseContent != nil { + tt.checkResponseContent(t, rec.Body.Bytes()) + } }) } } +// TestUpdateExpectedRackHandler_RackIDImmutable verifies that changing an +// ExpectedRack's rackId is rejected before any database mutation or workflow +// trigger, so Cloud, Core, and Flow can never hold different rack IDs for the +// same ExpectedRack. Omitted or identical rackId values remain compatible. +func TestUpdateExpectedRackHandler_RackIDImmutable(t *testing.T) { + e := echo.New() + dbSession := testExpectedRackInitDB(t) + defer dbSession.Close() + + ctx := context.Background() + cfg := common.GetTestConfig() + + tcfg, _ := cfg.GetTemporalConfig() + scp := sc.NewClientPool(tcfg) + + org := "test-org" + _, site, _ := testExpectedRackSetupTestData(t, dbSession, org) + + dbUser := &cdbm.User{ + ID: uuid.New(), + StarfleetID: cutil.GetPtr("test-user"), + } + _, err := dbSession.DB.NewInsert().Model(dbUser).Exec(ctx) + assert.Nil(t, err) + + erDAO := cdbm.NewExpectedRackDAO(dbSession) + rack, err := erDAO.Create(ctx, nil, cdbm.ExpectedRackCreateInput{ + ExpectedRackID: uuid.New(), + RackID: "immutable-rack-001", + SiteID: site.ID, + RackProfileID: "profile-original", + CreatedBy: dbUser.ID, + }) + assert.Nil(t, err) + assert.NotNil(t, rack) + + mockTemporalClient := &tmocks.Client{} + mockWorkflowRun := &tmocks.WorkflowRun{} + mockWorkflowRun.On("GetID").Return("test-workflow-id") + mockWorkflowRun.Mock.On("Get", mock.Anything, mock.Anything).Return(nil) + mockTemporalClient.Mock.On("ExecuteWorkflow", mock.Anything, mock.Anything, "UpdateExpectedRack", mock.Anything).Return(mockWorkflowRun, nil) + scp.IDClientMap[site.ID.String()] = mockTemporalClient + + handler := NewUpdateExpectedRackHandler(dbSession, scp, cfg) + + createMockUser := func() *cdbm.User { + return &cdbm.User{ + ID: dbUser.ID, + StarfleetID: cutil.GetPtr("test-user"), + OrgData: cdbm.OrgData{ + org: cdbm.Org{ + ID: 123, + Name: org, + DisplayName: org, + OrgType: "ENTERPRISE", + Roles: []string{"FORGE_PROVIDER_ADMIN"}, + }, + }, + } + } + + patch := func(body model.APIExpectedRackUpdateRequest) *httptest.ResponseRecorder { + reqBody, _ := json.Marshal(body) + req := httptest.NewRequest(http.MethodPatch, "/v2/org/"+org+"/expected-rack/"+rack.ID.String(), bytes.NewReader(reqBody)) + req.Header.Set(echo.HeaderContentType, echo.MIMEApplicationJSON) + req = req.WithContext(context.Background()) + + rec := httptest.NewRecorder() + c := e.NewContext(req, rec) + c.Set("user", createMockUser()) + c.SetParamNames("orgName", "id") + c.SetParamValues(org, rack.ID.String()) + + err := handler.Handle(c) + assert.Nil(t, err) + return rec + } + + getRackID := func() string { + current, err := erDAO.Get(ctx, nil, rack.ID, nil, false) + assert.Nil(t, err) + return current.RackID + } + + t.Run("changing rack_id is rejected without mutation or workflow", func(t *testing.T) { + rec := patch(model.APIExpectedRackUpdateRequest{ + RackID: cutil.GetPtr("immutable-rack-renamed"), + RackProfileID: cutil.GetPtr("profile-renamed"), + }) + + assert.Equal(t, http.StatusBadRequest, rec.Code) + assert.Contains(t, rec.Body.String(), "RackID cannot be changed after creation") + assert.Equal(t, "immutable-rack-001", getRackID()) + mockTemporalClient.AssertNotCalled(t, "ExecuteWorkflow", mock.Anything, mock.Anything, "UpdateExpectedRack", mock.Anything) + }) + + t.Run("identical rack_id remains compatible", func(t *testing.T) { + rec := patch(model.APIExpectedRackUpdateRequest{ + RackID: cutil.GetPtr("immutable-rack-001"), + RackProfileID: cutil.GetPtr("profile-identical"), + }) + + assert.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, "immutable-rack-001", getRackID()) + }) + + t.Run("omitted rack_id remains compatible", func(t *testing.T) { + rec := patch(model.APIExpectedRackUpdateRequest{ + RackProfileID: cutil.GetPtr("profile-omitted"), + }) + + assert.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, "immutable-rack-001", getRackID()) + }) +} + func TestDeleteExpectedRackHandler_Handle(t *testing.T) { e := echo.New() dbSession := testExpectedRackInitDB(t) diff --git a/rest-api/api/pkg/api/model/expectedrack.go b/rest-api/api/pkg/api/model/expectedrack.go index ef9b20eac7..871db51d25 100644 --- a/rest-api/api/pkg/api/model/expectedrack.go +++ b/rest-api/api/pkg/api/model/expectedrack.go @@ -68,7 +68,10 @@ func (ercr *APIExpectedRackCreateRequest) Validate() error { type APIExpectedRackUpdateRequest struct { // ID is required for batch updates (must be empty or match path value for single update). ID *string `json:"id"` - // RackID is the optional new operator-supplied rack identifier + // RackID is the operator-supplied rack identifier. It is immutable on + // update: it may be omitted or set to the existing value, but a changed + // value is rejected by the handler before any database mutation because + // Core and Flow use rackId as the identity key. RackID *string `json:"rackId"` // RackProfileID is the optional new rack profile ID RackProfileID *string `json:"rackProfileId"` diff --git a/rest-api/api/pkg/api/model/expectedrack_test.go b/rest-api/api/pkg/api/model/expectedrack_test.go index 672e25c583..fd190c8409 100644 --- a/rest-api/api/pkg/api/model/expectedrack_test.go +++ b/rest-api/api/pkg/api/model/expectedrack_test.go @@ -189,7 +189,7 @@ func TestAPIExpectedRackUpdateRequest_Validate(t *testing.T) { expectErr: false, }, { - desc: "ok when RackID rename is provided", + desc: "ok when RackID is provided (structural validation; immutability is enforced by the handler)", obj: APIExpectedRackUpdateRequest{ RackID: &validRackID, }, diff --git a/rest-api/docs/index.html b/rest-api/docs/index.html index be60c4ddd2..48d2a72035 100644 --- a/rest-api/docs/index.html +++ b/rest-api/docs/index.html @@ -6086,10 +6086,12 @@
Update an existing Expected Rack identified by its id.
Org must have an Infrastructure Provider entity. User must have authorization role with PROVIDER_ADMIN suffix.
Infrastructure Provider must own the Expected Rack.
Alternatively, Tenant Admins with TargetedInstanceCreation capability can also update Expected Racks if they have an account with the Site's Infrastructure Provider.
rackId is immutable: an update that changes it is rejected with 400 before any database mutation.
| org required | string Name of the Org | ||||||||||
| id required | string <uuid> Typical API Call Flow for Tenant
" class="sc-iJSMbW sc-cBEgGa fiNpIH ewCFMV"> Expected Rack update request
|