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 @@

Typical API Call Flow for Tenant

<p>Org must have an Infrastructure Provider entity. User must have authorization role with <code>PROVIDER_ADMIN</code> suffix.</p> <p>Infrastructure Provider must own the Expected Rack.</p> <p>Alternatively, Tenant Admins with <code>TargetedInstanceCreation</code> capability can also update Expected Racks if they have an account with the Site&#39;s Infrastructure Provider.</p> +<p><code>rackId</code> is immutable: an update that changes it is rejected with <code>400</code> before any database mutation.</p> " class="sc-iJSMbW sc-cBEgGa fiNpIH ewCFMV">

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.

Authorizations:
JWTBearerToken
path Parameters
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

id
string or null <uuid>

Unique identifier (UUID) of the Expected Rack to update. Optional for single Expected Rack update (must be empty or match the id from the URL path).

-
rackId
string or null non-empty

Optional new operator-supplied rack identifier. If provided, must be non-empty and unique within the Site.

+
rackId
string or null non-empty
Deprecated

Operator-supplied rack identifier. Immutable on update: omit this field, send null, or provide the existing value as a compatibility no-op. A changed value is rejected because Core and Flow use rackId as the identity key for expected racks.

rackProfileId
string or null non-empty

Optional new Rack Profile identifier. If provided, must be non-empty.

name
string or null non-empty
Typical API Call Flow for Tenant