Skip to content

Commit dd44a4c

Browse files
onutconutc
committed
fix(api): harden external owner idempotency
1 parent 7c071af commit dd44a4c

6 files changed

Lines changed: 334 additions & 74 deletions

‎api/create_request_normalization.go‎

Lines changed: 33 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -56,22 +56,21 @@ func writeCreateRequestError(c echo.Context, err error) error {
5656
}
5757

5858
type normalizedCreateRequest struct {
59-
body createRequest
60-
fingerprintRequest createRequest
61-
namespace string
62-
owner spritzv1.SpritzOwner
63-
resolvedExternalOwner *externalOwnerResolution
64-
userConfigKeys map[string]json.RawMessage
65-
userConfigPayload userConfigPayload
66-
normalizedUserConfig json.RawMessage
67-
requestedImage bool
68-
requestedRepo bool
69-
requestedNamespace bool
70-
nameProvided bool
71-
requestedNamePrefix string
59+
body createRequest
60+
fingerprintRequest createRequest
61+
namespace string
62+
owner spritzv1.SpritzOwner
63+
userConfigKeys map[string]json.RawMessage
64+
userConfigPayload userConfigPayload
65+
normalizedUserConfig json.RawMessage
66+
requestedImage bool
67+
requestedRepo bool
68+
requestedNamespace bool
69+
nameProvided bool
70+
requestedNamePrefix string
7271
}
7372

74-
func (s *server) normalizeCreateRequest(ctx context.Context, principal principal, body createRequest) (*normalizedCreateRequest, error) {
73+
func (s *server) normalizeCreateRequest(_ context.Context, principal principal, body createRequest) (*normalizedCreateRequest, error) {
7574
body.Name = strings.TrimSpace(body.Name)
7675
body.NamePrefix = strings.TrimSpace(body.NamePrefix)
7776
applyTopLevelCreateFields(&body)
@@ -87,22 +86,21 @@ func (s *server) normalizeCreateRequest(ctx context.Context, principal principal
8786
}
8887
requestedNamespace := s.namespaceOverrideRequested(body.Namespace, namespace)
8988

90-
owner, resolvedExternalOwner, err := s.resolveCreateOwner(ctx, &body, principal)
89+
owner, err := normalizeCreateOwnerRequest(&body, principal, s.auth.enabled())
9190
if err != nil {
92-
var resolutionErr externalOwnerResolutionError
93-
if errors.As(err, &resolutionErr) {
94-
return nil, newCreateRequestErrorWithData(resolutionErr.status, resolutionErr.message, resolutionErr.responseData(), err)
95-
}
9691
if errors.Is(err, errForbidden) {
9792
return nil, newCreateRequestError(http.StatusForbidden, err)
9893
}
9994
return nil, newCreateRequestError(http.StatusBadRequest, err)
10095
}
101-
body.Spec.Owner = owner
102-
fingerprintRequest := body
103-
if resolvedExternalOwner != nil {
104-
fingerprintRequest.Spec.Owner = spritzv1.SpritzOwner{}
96+
if body.OwnerRef != nil && strings.EqualFold(strings.TrimSpace(body.OwnerRef.Type), "external") {
97+
if !principalCanUseProvisionerFlow(principal) {
98+
return nil, newCreateRequestError(http.StatusForbidden, errForbidden)
99+
}
100+
} else {
101+
body.Spec.Owner = owner
105102
}
103+
fingerprintRequest := body
106104

107105
requestedImage := strings.TrimSpace(body.Spec.Image) != ""
108106
requestedRepo := body.Spec.Repo != nil || len(body.Spec.Repos) > 0
@@ -145,19 +143,18 @@ func (s *server) normalizeCreateRequest(ctx context.Context, principal principal
145143
}
146144

147145
return &normalizedCreateRequest{
148-
body: body,
149-
fingerprintRequest: fingerprintRequest,
150-
namespace: namespace,
151-
owner: owner,
152-
resolvedExternalOwner: resolvedExternalOwner,
153-
userConfigKeys: userConfigKeys,
154-
userConfigPayload: userConfigPayload,
155-
normalizedUserConfig: normalizedUserConfig,
156-
requestedImage: requestedImage,
157-
requestedRepo: requestedRepo,
158-
requestedNamespace: requestedNamespace,
159-
nameProvided: body.Name != "",
160-
requestedNamePrefix: strings.TrimSpace(fingerprintRequest.NamePrefix),
146+
body: body,
147+
fingerprintRequest: fingerprintRequest,
148+
namespace: namespace,
149+
owner: owner,
150+
userConfigKeys: userConfigKeys,
151+
userConfigPayload: userConfigPayload,
152+
normalizedUserConfig: normalizedUserConfig,
153+
requestedImage: requestedImage,
154+
requestedRepo: requestedRepo,
155+
requestedNamespace: requestedNamespace,
156+
nameProvided: body.Name != "",
157+
requestedNamePrefix: strings.TrimSpace(fingerprintRequest.NamePrefix),
161158
}, nil
162159
}
163160

‎api/main.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -368,7 +368,7 @@ func (s *server) createSpritz(c echo.Context) error {
368368
body = normalized.body
369369
namespace := normalized.namespace
370370
owner := normalized.owner
371-
resolvedExternalOwner := normalized.resolvedExternalOwner
371+
var resolvedExternalOwner *externalOwnerResolution
372372
userConfigKeys := normalized.userConfigKeys
373373
userConfigPayload := normalized.userConfigPayload
374374
nameProvided := normalized.nameProvided
@@ -417,6 +417,7 @@ func (s *server) createSpritz(c echo.Context) error {
417417
return writeProvisionerCreateError(c, err)
418418
}
419419
owner = body.Spec.Owner
420+
resolvedExternalOwner = provisionerTx.resolvedExternalOwner
420421
provisionerFingerprint = provisionerTx.provisionerFingerprint
421422
if !nameProvided {
422423
if err := buildNameGenerator(body); err != nil {

‎api/main_create_external_owner_test.go‎

Lines changed: 133 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -53,15 +53,19 @@ func newCreateSpritzAPI(t *testing.T, s *server) *echo.Echo {
5353
}
5454

5555
func newServiceCreateRequest(body []byte) (*http.Request, *httptest.ResponseRecorder) {
56+
return newServiceCreateRequestWithScopes(body,
57+
scopeInstancesCreate,
58+
scopeInstancesAssignOwner,
59+
scopeExternalResolveViaCreate,
60+
)
61+
}
62+
63+
func newServiceCreateRequestWithScopes(body []byte, scopes ...string) (*http.Request, *httptest.ResponseRecorder) {
5664
req := httptest.NewRequest(http.MethodPost, "/api/spritzes", bytes.NewReader(body))
5765
req.Header.Set(echo.HeaderContentType, echo.MIMEApplicationJSON)
5866
req.Header.Set("X-Spritz-User-Id", "zenobot")
5967
req.Header.Set("X-Spritz-Principal-Type", "service")
60-
req.Header.Set("X-Spritz-Principal-Scopes", strings.Join([]string{
61-
scopeInstancesCreate,
62-
scopeInstancesAssignOwner,
63-
scopeExternalResolveViaCreate,
64-
}, ","))
68+
req.Header.Set("X-Spritz-Principal-Scopes", strings.Join(scopes, ","))
6569
return req, httptest.NewRecorder()
6670
}
6771

@@ -229,6 +233,130 @@ func TestCreateSpritzReplaysExternalOwnerProvisioningAfterResolverMappingChanges
229233
}
230234
}
231235

236+
func TestCreateSpritzReplaysExternalOwnerProvisioningWhenResolverBecomesUnavailable(t *testing.T) {
237+
s := newCreateSpritzTestServer(t)
238+
configureProvisionerTestServer(s)
239+
resolverCalls := 0
240+
configureExternalOwnerTestServer(s, fakeExternalOwnerResolver{
241+
resolve: func(_ context.Context, _ externalOwnerPolicy, _ principal, _ ownerRef, _ string) (externalOwnerResolution, error) {
242+
resolverCalls++
243+
if resolverCalls == 1 {
244+
return externalOwnerResolution{
245+
Status: externalOwnerResolved,
246+
OwnerID: "user-123",
247+
}, nil
248+
}
249+
return externalOwnerResolution{}, context.DeadlineExceeded
250+
},
251+
})
252+
e := newCreateSpritzAPI(t, s)
253+
254+
body := []byte(`{"presetId":"openclaw","ownerRef":{"type":"external","provider":"msteams","tenant":"72f988bf-86f1-41af-91ab-2d7cd011db47","subject":"6f0f9d4f-9b0e-4d52-8c3a-ef0fd64b9b9f"},"idempotencyKey":"teams-replay-unavailable"}`)
255+
256+
req1, rec1 := newServiceCreateRequest(body)
257+
e.ServeHTTP(rec1, req1)
258+
if rec1.Code != http.StatusCreated {
259+
t.Fatalf("expected first create status 201, got %d: %s", rec1.Code, rec1.Body.String())
260+
}
261+
262+
req2, rec2 := newServiceCreateRequest(body)
263+
e.ServeHTTP(rec2, req2)
264+
if rec2.Code != http.StatusOK {
265+
t.Fatalf("expected replay status 200, got %d: %s", rec2.Code, rec2.Body.String())
266+
}
267+
if resolverCalls != 1 {
268+
t.Fatalf("expected replay to avoid a second resolver call, got %d calls", resolverCalls)
269+
}
270+
}
271+
272+
func TestCreateSpritzRejectsExternalOwnerResolutionWithoutCreateScopes(t *testing.T) {
273+
s := newCreateSpritzTestServer(t)
274+
configureProvisionerTestServer(s)
275+
resolverCalls := 0
276+
configureExternalOwnerTestServer(s, fakeExternalOwnerResolver{
277+
resolve: func(_ context.Context, _ externalOwnerPolicy, _ principal, _ ownerRef, _ string) (externalOwnerResolution, error) {
278+
resolverCalls++
279+
return externalOwnerResolution{Status: externalOwnerUnresolved}, nil
280+
},
281+
})
282+
e := newCreateSpritzAPI(t, s)
283+
284+
body := []byte(`{"presetId":"openclaw","ownerRef":{"type":"external","provider":"msteams","tenant":"72f988bf-86f1-41af-91ab-2d7cd011db47","subject":"6f0f9d4f-9b0e-4d52-8c3a-ef0fd64b9b9f"},"idempotencyKey":"teams-no-create-scope"}`)
285+
286+
req, rec := newServiceCreateRequestWithScopes(body, scopeExternalResolveViaCreate)
287+
e.ServeHTTP(rec, req)
288+
289+
if rec.Code != http.StatusForbidden {
290+
t.Fatalf("expected status 403, got %d: %s", rec.Code, rec.Body.String())
291+
}
292+
if resolverCalls != 0 {
293+
t.Fatalf("expected resolver to be skipped when create scopes are missing, got %d calls", resolverCalls)
294+
}
295+
}
296+
297+
func TestCreateRequestFingerprintCanonicalizesEquivalentOwnerInputs(t *testing.T) {
298+
directFingerprint, err := createRequestFingerprint(createRequest{
299+
OwnerID: "user-123",
300+
Spec: spritzv1.SpritzSpec{
301+
Image: "example.com/spritz-openclaw:latest",
302+
},
303+
}, "spritz-test", "", "", nil)
304+
if err != nil {
305+
t.Fatalf("createRequestFingerprint failed for direct owner: %v", err)
306+
}
307+
308+
ownerRefFingerprint, err := createRequestFingerprint(createRequest{
309+
OwnerRef: &ownerRef{
310+
Type: "owner",
311+
ID: "user-123",
312+
},
313+
Spec: spritzv1.SpritzSpec{
314+
Image: "example.com/spritz-openclaw:latest",
315+
},
316+
}, "spritz-test", "", "", nil)
317+
if err != nil {
318+
t.Fatalf("createRequestFingerprint failed for ownerRef owner: %v", err)
319+
}
320+
321+
if directFingerprint != ownerRefFingerprint {
322+
t.Fatalf("expected equivalent direct and ownerRef owner inputs to share a fingerprint")
323+
}
324+
325+
lowerFingerprint, err := createRequestFingerprint(createRequest{
326+
OwnerRef: &ownerRef{
327+
Type: "external",
328+
Provider: "msteams",
329+
Tenant: "72f988bf-86f1-41af-91ab-2d7cd011db47",
330+
Subject: "6f0f9d4f-9b0e-4d52-8c3a-ef0fd64b9b9f",
331+
},
332+
Spec: spritzv1.SpritzSpec{
333+
Image: "example.com/spritz-openclaw:latest",
334+
},
335+
}, "spritz-test", "", "", nil)
336+
if err != nil {
337+
t.Fatalf("createRequestFingerprint failed for lowercase msteams identity: %v", err)
338+
}
339+
340+
upperFingerprint, err := createRequestFingerprint(createRequest{
341+
OwnerRef: &ownerRef{
342+
Type: "external",
343+
Provider: "msteams",
344+
Tenant: "72F988BF-86F1-41AF-91AB-2D7CD011DB47",
345+
Subject: "6F0F9D4F-9B0E-4D52-8C3A-EF0FD64B9B9F",
346+
},
347+
Spec: spritzv1.SpritzSpec{
348+
Image: "example.com/spritz-openclaw:latest",
349+
},
350+
}, "spritz-test", "", "", nil)
351+
if err != nil {
352+
t.Fatalf("createRequestFingerprint failed for uppercase msteams identity: %v", err)
353+
}
354+
355+
if lowerFingerprint != upperFingerprint {
356+
t.Fatalf("expected equivalent msteams UUID casing to share a fingerprint")
357+
}
358+
}
359+
232360
func TestNormalizeCreateOwnerSupportsOwnerRefOwner(t *testing.T) {
233361
body := &createRequest{
234362
OwnerRef: &ownerRef{Type: "owner", ID: "user-123"},

‎api/main_create_owner_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1094,7 +1094,7 @@ func TestCreateSpritzUsesPendingReservationPayloadAfterDefaultPresetChanges(t *t
10941094
if err != nil {
10951095
t.Fatalf("createRequestFingerprint failed: %v", err)
10961096
}
1097-
resolvedPayload, err := createResolvedProvisionerPayload(requestBody, s.resolvedCreateNamePrefix(requestBody, requestFingerprintBody.NamePrefix))
1097+
resolvedPayload, err := createResolvedProvisionerPayload(requestBody, s.resolvedCreateNamePrefix(requestBody, requestFingerprintBody.NamePrefix), nil)
10981098
if err != nil {
10991099
t.Fatalf("createResolvedProvisionerPayload failed: %v", err)
11001100
}
@@ -1547,7 +1547,7 @@ func TestCreateSpritzRetriesPendingIdempotencyReservationWithConflictingOccupant
15471547
if err != nil {
15481548
t.Fatalf("createRequestFingerprint failed: %v", err)
15491549
}
1550-
resolvedPayload, err := createResolvedProvisionerPayload(body, s.resolvedCreateNamePrefix(body, requestFingerprintBody.NamePrefix))
1550+
resolvedPayload, err := createResolvedProvisionerPayload(body, s.resolvedCreateNamePrefix(body, requestFingerprintBody.NamePrefix), nil)
15511551
if err != nil {
15521552
t.Fatalf("createResolvedProvisionerPayload failed: %v", err)
15531553
}
@@ -1641,7 +1641,7 @@ func TestCreateSpritzReplaysPendingIdempotentCreateBeforeQuotaCheck(t *testing.T
16411641
if err != nil {
16421642
t.Fatalf("createRequestFingerprint failed: %v", err)
16431643
}
1644-
resolvedPayload, err := createResolvedProvisionerPayload(body, s.resolvedCreateNamePrefix(body, requestFingerprintBody.NamePrefix))
1644+
resolvedPayload, err := createResolvedProvisionerPayload(body, s.resolvedCreateNamePrefix(body, requestFingerprintBody.NamePrefix), nil)
16451645
if err != nil {
16461646
t.Fatalf("createResolvedProvisionerPayload failed: %v", err)
16471647
}

0 commit comments

Comments
 (0)