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 <aaron.segal@rpsolutions.com> Co-authored-by: Chris Lu <chris.lu@gmail.com>
2026-03-06 15:53:23 -05:00
parent f9311a3422
commit 14cd0f53ba
8 changed files with 106 additions and 33 deletions
--- a/weed/iam/error_response_test.go
+++ 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, "<Error>")
+	requestIDIndex := strings.Index(xmlString, "<RequestId>request-123</RequestId>")
+
+	assert.NotEqual(t, -1, errorIndex, "Error should be present")
+	assert.NotEqual(t, -1, requestIDIndex, "RequestId should be present")
+	assert.NotContains(t, xmlString, "<ResponseMetadata>")
+	assert.Less(t, errorIndex, requestIDIndex, "RequestId should appear after Error at the root level")
+}
--- 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
 }
--- a/weed/iam/responses_test.go
+++ 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, "<ListUsersResult>")
+	responseMetadataIndex := strings.Index(xmlString, "<ResponseMetadata>")
+
+	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")
+}
--- 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
 }

--- 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
 	}
--- 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(), "<RequestId>")
+	assert.NotContains(t, response.Body.String(), "<ResponseMetadata>")
 }

 func extractErrorCodeAndMessage(response *httptest.ResponseRecorder) (string, string) {
--- 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
 }

--- 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(), "<RequestId>")
+	assert.NotContains(t, rr.Body.String(), "<ResponseMetadata>")
 }

 // TestGetPolicyDocument tests parsing of policy documents