From 14cd0f53ba94615069f2a0136bc6b7c966671267 Mon Sep 17 00:00:00 2001 From: Aaron <54863852+asegs@users.noreply.github.com> Date: Fri, 6 Mar 2026 15:53:23 -0500 Subject: [PATCH] Places the CommonResponse struct at the *end* of all IAM responses. (#8537) * Places the CommonResponse struct at the end of all IAM responses, rather than the start. * iam: fix error response request id layout * iam: add XML ordering regression test * iam: share request id generation --------- Co-authored-by: Aaron Segal Co-authored-by: Chris Lu --- weed/iam/error_response_test.go | 34 +++++++++++ weed/iam/responses.go | 69 +++++++++++++---------- weed/iam/responses_test.go | 26 +++++++++ weed/iamapi/iamapi_handlers.go | 1 + weed/iamapi/iamapi_management_handlers.go | 4 +- weed/iamapi/iamapi_test.go | 2 + weed/s3api/s3api_embedded_iam.go | 1 + weed/s3api/s3api_embedded_iam_test.go | 2 + 8 files changed, 106 insertions(+), 33 deletions(-) create mode 100644 weed/iam/error_response_test.go create mode 100644 weed/iam/responses_test.go diff --git a/weed/iam/error_response_test.go b/weed/iam/error_response_test.go new file mode 100644 index 000000000..4e008c157 --- /dev/null +++ b/weed/iam/error_response_test.go @@ -0,0 +1,34 @@ +package iam + +import ( + "encoding/xml" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestErrorResponseXMLUsesTopLevelRequestId(t *testing.T) { + errCode := "NoSuchEntity" + errMsg := "the requested IAM entity does not exist" + + resp := ErrorResponse{ + RequestId: "request-123", + } + resp.Error.Type = "Sender" + resp.Error.Code = &errCode + resp.Error.Message = &errMsg + + output, err := xml.Marshal(resp) + require.NoError(t, err) + + xmlString := string(output) + errorIndex := strings.Index(xmlString, "") + requestIDIndex := strings.Index(xmlString, "request-123") + + assert.NotEqual(t, -1, errorIndex, "Error should be present") + assert.NotEqual(t, -1, requestIDIndex, "RequestId should be present") + assert.NotContains(t, xmlString, "") + assert.Less(t, errorIndex, requestIDIndex, "RequestId should appear after Error at the root level") +} diff --git a/weed/iam/responses.go b/weed/iam/responses.go index c84d49e2b..ac78c5ba7 100644 --- a/weed/iam/responses.go +++ b/weed/iam/responses.go @@ -8,7 +8,7 @@ import ( "github.com/aws/aws-sdk-go/service/iam" ) -// CommonResponse is embedded in all IAM response types to provide RequestId. +// CommonResponse is embedded in IAM success response types to provide RequestId. type CommonResponse struct { ResponseMetadata struct { RequestId string `xml:"RequestId"` @@ -17,183 +17,192 @@ type CommonResponse struct { // SetRequestId sets a unique request ID based on current timestamp. func (r *CommonResponse) SetRequestId() { - r.ResponseMetadata.RequestId = fmt.Sprintf("%d", time.Now().UnixNano()) + r.ResponseMetadata.RequestId = newRequestID() } // ListUsersResponse is the response for ListUsers action. type ListUsersResponse struct { - CommonResponse XMLName xml.Name `xml:"https://iam.amazonaws.com/doc/2010-05-08/ ListUsersResponse"` ListUsersResult struct { Users []*iam.User `xml:"Users>member"` IsTruncated bool `xml:"IsTruncated"` } `xml:"ListUsersResult"` + CommonResponse } // ListAccessKeysResponse is the response for ListAccessKeys action. type ListAccessKeysResponse struct { - CommonResponse XMLName xml.Name `xml:"https://iam.amazonaws.com/doc/2010-05-08/ ListAccessKeysResponse"` ListAccessKeysResult struct { AccessKeyMetadata []*iam.AccessKeyMetadata `xml:"AccessKeyMetadata>member"` IsTruncated bool `xml:"IsTruncated"` } `xml:"ListAccessKeysResult"` + CommonResponse } // DeleteAccessKeyResponse is the response for DeleteAccessKey action. type DeleteAccessKeyResponse struct { - CommonResponse XMLName xml.Name `xml:"https://iam.amazonaws.com/doc/2010-05-08/ DeleteAccessKeyResponse"` + CommonResponse } // CreatePolicyResponse is the response for CreatePolicy action. type CreatePolicyResponse struct { - CommonResponse XMLName xml.Name `xml:"https://iam.amazonaws.com/doc/2010-05-08/ CreatePolicyResponse"` CreatePolicyResult struct { Policy iam.Policy `xml:"Policy"` } `xml:"CreatePolicyResult"` + CommonResponse } // DeletePolicyResponse is the response for DeletePolicy action. type DeletePolicyResponse struct { - CommonResponse XMLName xml.Name `xml:"https://iam.amazonaws.com/doc/2010-05-08/ DeletePolicyResponse"` + CommonResponse } // ListPoliciesResponse is the response for ListPolicies action. type ListPoliciesResponse struct { - CommonResponse XMLName xml.Name `xml:"https://iam.amazonaws.com/doc/2010-05-08/ ListPoliciesResponse"` ListPoliciesResult struct { Policies []*iam.Policy `xml:"Policies>member"` IsTruncated bool `xml:"IsTruncated"` Marker string `xml:"Marker,omitempty"` } `xml:"ListPoliciesResult"` + CommonResponse } // GetPolicyResponse is the response for GetPolicy action. type GetPolicyResponse struct { - CommonResponse XMLName xml.Name `xml:"https://iam.amazonaws.com/doc/2010-05-08/ GetPolicyResponse"` GetPolicyResult struct { Policy iam.Policy `xml:"Policy"` } `xml:"GetPolicyResult"` + CommonResponse } // ListPolicyVersionsResponse is the response for ListPolicyVersions action. type ListPolicyVersionsResponse struct { - CommonResponse XMLName xml.Name `xml:"https://iam.amazonaws.com/doc/2010-05-08/ ListPolicyVersionsResponse"` ListPolicyVersionsResult struct { Versions []*iam.PolicyVersion `xml:"Versions>member"` IsTruncated bool `xml:"IsTruncated"` Marker string `xml:"Marker,omitempty"` } `xml:"ListPolicyVersionsResult"` + CommonResponse } // GetPolicyVersionResponse is the response for GetPolicyVersion action. type GetPolicyVersionResponse struct { - CommonResponse XMLName xml.Name `xml:"https://iam.amazonaws.com/doc/2010-05-08/ GetPolicyVersionResponse"` GetPolicyVersionResult struct { PolicyVersion iam.PolicyVersion `xml:"PolicyVersion"` } `xml:"GetPolicyVersionResult"` + CommonResponse } // CreateUserResponse is the response for CreateUser action. type CreateUserResponse struct { - CommonResponse XMLName xml.Name `xml:"https://iam.amazonaws.com/doc/2010-05-08/ CreateUserResponse"` CreateUserResult struct { User iam.User `xml:"User"` } `xml:"CreateUserResult"` + CommonResponse } // DeleteUserResponse is the response for DeleteUser action. type DeleteUserResponse struct { - CommonResponse XMLName xml.Name `xml:"https://iam.amazonaws.com/doc/2010-05-08/ DeleteUserResponse"` + CommonResponse } // GetUserResponse is the response for GetUser action. type GetUserResponse struct { - CommonResponse XMLName xml.Name `xml:"https://iam.amazonaws.com/doc/2010-05-08/ GetUserResponse"` GetUserResult struct { User iam.User `xml:"User"` } `xml:"GetUserResult"` + CommonResponse } // UpdateUserResponse is the response for UpdateUser action. type UpdateUserResponse struct { - CommonResponse XMLName xml.Name `xml:"https://iam.amazonaws.com/doc/2010-05-08/ UpdateUserResponse"` + CommonResponse } // CreateAccessKeyResponse is the response for CreateAccessKey action. type CreateAccessKeyResponse struct { - CommonResponse XMLName xml.Name `xml:"https://iam.amazonaws.com/doc/2010-05-08/ CreateAccessKeyResponse"` CreateAccessKeyResult struct { AccessKey iam.AccessKey `xml:"AccessKey"` } `xml:"CreateAccessKeyResult"` + CommonResponse } // PutUserPolicyResponse is the response for PutUserPolicy action. type PutUserPolicyResponse struct { - CommonResponse XMLName xml.Name `xml:"https://iam.amazonaws.com/doc/2010-05-08/ PutUserPolicyResponse"` + CommonResponse } // DeleteUserPolicyResponse is the response for DeleteUserPolicy action. type DeleteUserPolicyResponse struct { - CommonResponse XMLName xml.Name `xml:"https://iam.amazonaws.com/doc/2010-05-08/ DeleteUserPolicyResponse"` + CommonResponse } // AttachUserPolicyResponse is the response for AttachUserPolicy action. type AttachUserPolicyResponse struct { - CommonResponse XMLName xml.Name `xml:"https://iam.amazonaws.com/doc/2010-05-08/ AttachUserPolicyResponse"` + CommonResponse } // DetachUserPolicyResponse is the response for DetachUserPolicy action. type DetachUserPolicyResponse struct { - CommonResponse XMLName xml.Name `xml:"https://iam.amazonaws.com/doc/2010-05-08/ DetachUserPolicyResponse"` + CommonResponse } // ListAttachedUserPoliciesResponse is the response for ListAttachedUserPolicies action. type ListAttachedUserPoliciesResponse struct { - CommonResponse XMLName xml.Name `xml:"https://iam.amazonaws.com/doc/2010-05-08/ ListAttachedUserPoliciesResponse"` ListAttachedUserPoliciesResult struct { AttachedPolicies []*iam.AttachedPolicy `xml:"AttachedPolicies>member"` IsTruncated bool `xml:"IsTruncated"` Marker string `xml:"Marker,omitempty"` } `xml:"ListAttachedUserPoliciesResult"` + CommonResponse } // GetUserPolicyResponse is the response for GetUserPolicy action. type GetUserPolicyResponse struct { - CommonResponse XMLName xml.Name `xml:"https://iam.amazonaws.com/doc/2010-05-08/ GetUserPolicyResponse"` GetUserPolicyResult struct { UserName string `xml:"UserName"` PolicyName string `xml:"PolicyName"` PolicyDocument string `xml:"PolicyDocument"` } `xml:"GetUserPolicyResult"` + CommonResponse } // ErrorResponse is the IAM error response format. type ErrorResponse struct { - CommonResponse XMLName xml.Name `xml:"https://iam.amazonaws.com/doc/2010-05-08/ ErrorResponse"` Error struct { iam.ErrorDetails Type string `xml:"Type"` } `xml:"Error"` + RequestId string `xml:"RequestId"` +} + +// SetRequestId sets a unique request ID based on current timestamp. +func (r *ErrorResponse) SetRequestId() { + r.RequestId = newRequestID() +} + +func newRequestID() string { + return fmt.Sprintf("%d", time.Now().UnixNano()) } // Error represents an IAM API error with code and underlying error. @@ -210,14 +219,14 @@ type Policies struct { // SetUserStatusResponse is the response for SetUserStatus action. // This is a SeaweedFS extension to enable/disable users without deleting them. type SetUserStatusResponse struct { - CommonResponse XMLName xml.Name `xml:"https://iam.amazonaws.com/doc/2010-05-08/ SetUserStatusResponse"` + CommonResponse } // UpdateAccessKeyResponse is the response for UpdateAccessKey action. type UpdateAccessKeyResponse struct { - CommonResponse XMLName xml.Name `xml:"https://iam.amazonaws.com/doc/2010-05-08/ UpdateAccessKeyResponse"` + CommonResponse } // ServiceAccountInfo contains service account details for API responses. @@ -234,40 +243,40 @@ type ServiceAccountInfo struct { // CreateServiceAccountResponse is the response for CreateServiceAccount action. type CreateServiceAccountResponse struct { - CommonResponse XMLName xml.Name `xml:"https://iam.amazonaws.com/doc/2010-05-08/ CreateServiceAccountResponse"` CreateServiceAccountResult struct { ServiceAccount ServiceAccountInfo `xml:"ServiceAccount"` } `xml:"CreateServiceAccountResult"` + CommonResponse } // DeleteServiceAccountResponse is the response for DeleteServiceAccount action. type DeleteServiceAccountResponse struct { - CommonResponse XMLName xml.Name `xml:"https://iam.amazonaws.com/doc/2010-05-08/ DeleteServiceAccountResponse"` + CommonResponse } // ListServiceAccountsResponse is the response for ListServiceAccounts action. type ListServiceAccountsResponse struct { - CommonResponse XMLName xml.Name `xml:"https://iam.amazonaws.com/doc/2010-05-08/ ListServiceAccountsResponse"` ListServiceAccountsResult struct { ServiceAccounts []*ServiceAccountInfo `xml:"ServiceAccounts>member"` IsTruncated bool `xml:"IsTruncated"` } `xml:"ListServiceAccountsResult"` + CommonResponse } // GetServiceAccountResponse is the response for GetServiceAccount action. type GetServiceAccountResponse struct { - CommonResponse XMLName xml.Name `xml:"https://iam.amazonaws.com/doc/2010-05-08/ GetServiceAccountResponse"` GetServiceAccountResult struct { ServiceAccount ServiceAccountInfo `xml:"ServiceAccount"` } `xml:"GetServiceAccountResult"` + CommonResponse } // UpdateServiceAccountResponse is the response for UpdateServiceAccount action. type UpdateServiceAccountResponse struct { - CommonResponse XMLName xml.Name `xml:"https://iam.amazonaws.com/doc/2010-05-08/ UpdateServiceAccountResponse"` + CommonResponse } diff --git a/weed/iam/responses_test.go b/weed/iam/responses_test.go new file mode 100644 index 000000000..daf2afc85 --- /dev/null +++ b/weed/iam/responses_test.go @@ -0,0 +1,26 @@ +package iam + +import ( + "encoding/xml" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestListUsersResponseXMLOrdering(t *testing.T) { + resp := ListUsersResponse{} + resp.SetRequestId() + + output, err := xml.Marshal(resp) + require.NoError(t, err) + + xmlString := string(output) + listUsersResultIndex := strings.Index(xmlString, "") + responseMetadataIndex := strings.Index(xmlString, "") + + assert.NotEqual(t, -1, listUsersResultIndex, "ListUsersResult should be present") + assert.NotEqual(t, -1, responseMetadataIndex, "ResponseMetadata should be present") + assert.Less(t, listUsersResultIndex, responseMetadataIndex, "ListUsersResult should appear before ResponseMetadata") +} diff --git a/weed/iamapi/iamapi_handlers.go b/weed/iamapi/iamapi_handlers.go index f959a8b5c..f187c9067 100644 --- a/weed/iamapi/iamapi_handlers.go +++ b/weed/iamapi/iamapi_handlers.go @@ -13,6 +13,7 @@ func newErrorResponse(errCode string, errMsg string) ErrorResponse { errorResp.Error.Type = "Sender" errorResp.Error.Code = &errCode errorResp.Error.Message = &errMsg + errorResp.SetRequestId() return errorResp } diff --git a/weed/iamapi/iamapi_management_handlers.go b/weed/iamapi/iamapi_management_handlers.go index c5f1f20ae..e56ab45b2 100644 --- a/weed/iamapi/iamapi_management_handlers.go +++ b/weed/iamapi/iamapi_management_handlers.go @@ -997,9 +997,7 @@ func (iama *IamApiServer) DoActions(w http.ResponseWriter, r *http.Request) { changed = false default: errNotImplemented := s3err.GetAPIError(s3err.ErrNotImplemented) - errorResponse := ErrorResponse{} - errorResponse.Error.Code = &errNotImplemented.Code - errorResponse.Error.Message = &errNotImplemented.Description + errorResponse := newErrorResponse(errNotImplemented.Code, errNotImplemented.Description) s3err.WriteXMLResponse(w, r, errNotImplemented.HTTPStatusCode, errorResponse) return } diff --git a/weed/iamapi/iamapi_test.go b/weed/iamapi/iamapi_test.go index acf864e11..9177c7d4a 100644 --- a/weed/iamapi/iamapi_test.go +++ b/weed/iamapi/iamapi_test.go @@ -244,6 +244,8 @@ func TestPutUserPolicyError(t *testing.T) { assert.Equal(t, expectedMessage, message) assert.Equal(t, expectedCode, code) + assert.Contains(t, response.Body.String(), "") + assert.NotContains(t, response.Body.String(), "") } func extractErrorCodeAndMessage(response *httptest.ResponseRecorder) (string, string) { diff --git a/weed/s3api/s3api_embedded_iam.go b/weed/s3api/s3api_embedded_iam.go index 9085b2b2e..97123e413 100644 --- a/weed/s3api/s3api_embedded_iam.go +++ b/weed/s3api/s3api_embedded_iam.go @@ -162,6 +162,7 @@ func newIamErrorResponse(errCode string, errMsg string) iamErrorResponse { errorResp.Error.Type = "Sender" errorResp.Error.Code = &errCode errorResp.Error.Message = &errMsg + errorResp.SetRequestId() return errorResp } diff --git a/weed/s3api/s3api_embedded_iam_test.go b/weed/s3api/s3api_embedded_iam_test.go index 9eab6321c..10429a236 100644 --- a/weed/s3api/s3api_embedded_iam_test.go +++ b/weed/s3api/s3api_embedded_iam_test.go @@ -1214,6 +1214,8 @@ func TestEmbeddedIamNotImplementedAction(t *testing.T) { apiRouter.ServeHTTP(rr, req) assert.Equal(t, http.StatusNotImplemented, rr.Code) + assert.Contains(t, rr.Body.String(), "") + assert.NotContains(t, rr.Body.String(), "") } // TestGetPolicyDocument tests parsing of policy documents