[#399] Add OPTIONS method for object operations #399

Merged
alexvanin merged 1 commit from mbiryukova/frostfs-s3-gw:bugfix/object_options into master 2024-06-04 12:59:52 +00:00
7 changed files with 197 additions and 8 deletions

View file

@ -187,8 +187,8 @@ func (h *handler) Preflight(w http.ResponseWriter, r *http.Request) {
if !checkSubslice(rule.AllowedHeaders, headers) { if !checkSubslice(rule.AllowedHeaders, headers) {
continue continue
} }
w.Header().Set(api.AccessControlAllowOrigin, o) w.Header().Set(api.AccessControlAllowOrigin, origin)
w.Header().Set(api.AccessControlAllowMethods, strings.Join(rule.AllowedMethods, ", ")) w.Header().Set(api.AccessControlAllowMethods, method)
dkirillov marked this conversation as resolved Outdated

Why do we need these changes?
And if we really need this we should add test for this (currently tests pass with or without these changes)

Why do we need these changes? And if we really need this we should add test for this (currently tests pass with or without these changes)

Changed according to spec. I will add test

Changed according to [spec](https://docs.aws.amazon.com/AmazonS3/latest/API/RESTOPTIONSobject.html#RESTOPTIONSobject-responses). I will add test
if headers != nil { if headers != nil {
w.Header().Set(api.AccessControlAllowHeaders, requestHeaders) w.Header().Set(api.AccessControlAllowHeaders, requestHeaders)
} }

View file

@ -7,6 +7,7 @@ import (
"git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api" "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api"
"git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/middleware" "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/middleware"
"github.com/stretchr/testify/require"
) )
func TestCORSOriginWildcard(t *testing.T) { func TestCORSOriginWildcard(t *testing.T) {
@ -39,3 +40,181 @@ func TestCORSOriginWildcard(t *testing.T) {
hc.Handler().GetBucketCorsHandler(w, r) hc.Handler().GetBucketCorsHandler(w, r)
assertStatus(t, w, http.StatusOK) assertStatus(t, w, http.StatusOK)
} }
func TestPreflight(t *testing.T) {
body := `
<CORSConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/">
<CORSRule>
<AllowedMethod>GET</AllowedMethod>
<AllowedOrigin>http://www.example.com</AllowedOrigin>
<AllowedHeader>Authorization</AllowedHeader>
<ExposeHeader>x-amz-*</ExposeHeader>
<ExposeHeader>X-Amz-*</ExposeHeader>
<MaxAgeSeconds>600</MaxAgeSeconds>
</CORSRule>
</CORSConfiguration>
`
hc := prepareHandlerContext(t)
bktName := "bucket-preflight-test"
box, _ := createAccessBox(t)
w, r := prepareTestRequest(hc, bktName, "", nil)
ctx := middleware.SetBox(r.Context(), &middleware.Box{AccessBox: box})
r = r.WithContext(ctx)
hc.Handler().CreateBucketHandler(w, r)
assertStatus(t, w, http.StatusOK)
w, r = prepareTestPayloadRequest(hc, bktName, "", strings.NewReader(body))
ctx = middleware.SetBox(r.Context(), &middleware.Box{AccessBox: box})
r = r.WithContext(ctx)
hc.Handler().PutBucketCorsHandler(w, r)
assertStatus(t, w, http.StatusOK)
for _, tc := range []struct {
name string
origin string
method string
headers string
expectedStatus int
}{
{
name: "Valid",
origin: "http://www.example.com",
method: "GET",
headers: "Authorization",
expectedStatus: http.StatusOK,
},
{
name: "Empty origin",
method: "GET",
headers: "Authorization",
expectedStatus: http.StatusBadRequest,
},
{
name: "Empty request method",
origin: "http://www.example.com",
headers: "Authorization",
expectedStatus: http.StatusBadRequest,
},
{
name: "Not allowed method",
origin: "http://www.example.com",
method: "PUT",
headers: "Authorization",
expectedStatus: http.StatusForbidden,
},
{
name: "Not allowed headers",
origin: "http://www.example.com",
method: "GET",
headers: "Authorization, Last-Modified",
expectedStatus: http.StatusForbidden,
},
} {
t.Run(tc.name, func(t *testing.T) {
w, r = prepareTestPayloadRequest(hc, bktName, "", nil)
r.Header.Set(api.Origin, tc.origin)
r.Header.Set(api.AccessControlRequestMethod, tc.method)
r.Header.Set(api.AccessControlRequestHeaders, tc.headers)
hc.Handler().Preflight(w, r)
assertStatus(t, w, tc.expectedStatus)
if tc.expectedStatus == http.StatusOK {
require.Equal(t, tc.origin, w.Header().Get(api.AccessControlAllowOrigin))
require.Equal(t, tc.method, w.Header().Get(api.AccessControlAllowMethods))
require.Equal(t, tc.headers, w.Header().Get(api.AccessControlAllowHeaders))
require.Equal(t, "x-amz-*, X-Amz-*", w.Header().Get(api.AccessControlExposeHeaders))
require.Equal(t, "true", w.Header().Get(api.AccessControlAllowCredentials))
require.Equal(t, "600", w.Header().Get(api.AccessControlMaxAge))
}
})
}
}
func TestPreflightWildcardOrigin(t *testing.T) {
body := `
<CORSConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/">
<CORSRule>
<AllowedMethod>GET</AllowedMethod>
<AllowedMethod>PUT</AllowedMethod>
<AllowedOrigin>*</AllowedOrigin>
<AllowedHeader>*</AllowedHeader>
</CORSRule>
</CORSConfiguration>
`
hc := prepareHandlerContext(t)
bktName := "bucket-preflight-wildcard-test"
box, _ := createAccessBox(t)
w, r := prepareTestRequest(hc, bktName, "", nil)
ctx := middleware.SetBox(r.Context(), &middleware.Box{AccessBox: box})
r = r.WithContext(ctx)
hc.Handler().CreateBucketHandler(w, r)
assertStatus(t, w, http.StatusOK)
w, r = prepareTestPayloadRequest(hc, bktName, "", strings.NewReader(body))
ctx = middleware.SetBox(r.Context(), &middleware.Box{AccessBox: box})
r = r.WithContext(ctx)
hc.Handler().PutBucketCorsHandler(w, r)
assertStatus(t, w, http.StatusOK)
for _, tc := range []struct {
name string
origin string
method string
headers string
expectedStatus int
}{
{
name: "Valid get",
origin: "http://www.example.com",
method: "GET",
headers: "Authorization, Last-Modified",
expectedStatus: http.StatusOK,
},
{
name: "Valid put",
origin: "http://example.com",
method: "PUT",
headers: "Authorization, Content-Type",
expectedStatus: http.StatusOK,
},
{
name: "Empty origin",
method: "GET",
headers: "Authorization, Last-Modified",
expectedStatus: http.StatusBadRequest,
},
{
name: "Empty request method",
origin: "http://www.example.com",
headers: "Authorization, Last-Modified",
expectedStatus: http.StatusBadRequest,
},
{
name: "Not allowed method",
origin: "http://www.example.com",
method: "DELETE",
headers: "Authorization, Last-Modified",
expectedStatus: http.StatusForbidden,
},
} {
t.Run(tc.name, func(t *testing.T) {
w, r = prepareTestPayloadRequest(hc, bktName, "", nil)
r.Header.Set(api.Origin, tc.origin)
r.Header.Set(api.AccessControlRequestMethod, tc.method)
r.Header.Set(api.AccessControlRequestHeaders, tc.headers)
hc.Handler().Preflight(w, r)
assertStatus(t, w, tc.expectedStatus)
if tc.expectedStatus == http.StatusOK {
require.Equal(t, tc.origin, w.Header().Get(api.AccessControlAllowOrigin))
require.Equal(t, tc.method, w.Header().Get(api.AccessControlAllowMethods))
require.Equal(t, tc.headers, w.Header().Get(api.AccessControlAllowHeaders))
require.Empty(t, w.Header().Get(api.AccessControlExposeHeaders))
require.Empty(t, w.Header().Get(api.AccessControlAllowCredentials))
require.Equal(t, "0", w.Header().Get(api.AccessControlMaxAge))
}
})
}
}

View file

@ -5,7 +5,7 @@ const (
// bucket operations. // bucket operations.
OptionsOperation = "Options" OptionsBucketOperation = "OptionsBucket"
HeadBucketOperation = "HeadBucket" HeadBucketOperation = "HeadBucket"
ListMultipartUploadsOperation = "ListMultipartUploads" ListMultipartUploadsOperation = "ListMultipartUploads"
GetBucketLocationOperation = "GetBucketLocation" GetBucketLocationOperation = "GetBucketLocation"
@ -51,6 +51,7 @@ const (
// object operations. // object operations.
OptionsObjectOperation = "OptionsObject"
HeadObjectOperation = "HeadObject" HeadObjectOperation = "HeadObject"
ListPartsOperation = "ListParts" ListPartsOperation = "ListParts"
GetObjectACLOperation = "GetObjectACL" GetObjectACLOperation = "GetObjectACL"

View file

@ -103,7 +103,7 @@ func stats(f http.HandlerFunc, resolveCID cidResolveFunc, appMetrics *metrics.Ap
func requestTypeFromAPI(api string) metrics.RequestType { func requestTypeFromAPI(api string) metrics.RequestType {
switch api { switch api {
case OptionsOperation, HeadObjectOperation, HeadBucketOperation: case OptionsBucketOperation, OptionsObjectOperation, HeadObjectOperation, HeadBucketOperation:
return metrics.HEADRequest return metrics.HEADRequest
case CreateMultipartUploadOperation, UploadPartCopyOperation, UploadPartOperation, CompleteMultipartUploadOperation, case CreateMultipartUploadOperation, UploadPartCopyOperation, UploadPartOperation, CompleteMultipartUploadOperation,
PutObjectACLOperation, PutObjectTaggingOperation, CopyObjectOperation, PutObjectRetentionOperation, PutObjectLegalHoldOperation, PutObjectACLOperation, PutObjectTaggingOperation, CopyObjectOperation, PutObjectRetentionOperation, PutObjectLegalHoldOperation,

View file

@ -253,7 +253,7 @@ func determineBucketOperation(r *http.Request) string {
query := r.URL.Query() query := r.URL.Query()
switch r.Method { switch r.Method {
case http.MethodOptions: case http.MethodOptions:
return OptionsOperation return OptionsBucketOperation
case http.MethodHead: case http.MethodHead:
return HeadBucketOperation return HeadBucketOperation
case http.MethodGet: case http.MethodGet:
@ -356,6 +356,8 @@ func determineBucketOperation(r *http.Request) string {
func determineObjectOperation(r *http.Request) string { func determineObjectOperation(r *http.Request) string {
query := r.URL.Query() query := r.URL.Query()
switch r.Method { switch r.Method {
case http.MethodOptions:
return OptionsObjectOperation
case http.MethodHead: case http.MethodHead:
return HeadObjectOperation return HeadObjectOperation
case http.MethodGet: case http.MethodGet:

View file

@ -91,9 +91,9 @@ func TestDetermineBucketOperation(t *testing.T) {
expected string expected string
}{ }{
{ {
name: "OptionsOperation", name: "OptionsBucketOperation",
method: http.MethodOptions, method: http.MethodOptions,
expected: OptionsOperation, expected: OptionsBucketOperation,
}, },
{ {
name: "HeadBucketOperation", name: "HeadBucketOperation",
@ -367,6 +367,11 @@ func TestDetermineObjectOperation(t *testing.T) {
headerKeys []string headerKeys []string
expected string expected string
}{ }{
{
name: "OptionsObjectOperation",
method: http.MethodOptions,
expected: OptionsObjectOperation,
},
{ {
name: "HeadObjectOperation", name: "HeadObjectOperation",
method: http.MethodHead, method: http.MethodHead,

View file

@ -223,7 +223,7 @@ func bucketRouter(h Handler, log *zap.Logger) chi.Router {
bktRouter.Mount("/", objectRouter(h, log)) bktRouter.Mount("/", objectRouter(h, log))
bktRouter.Options("/", h.Preflight) bktRouter.Options("/", named(s3middleware.OptionsBucketOperation, h.Preflight))
dkirillov marked this conversation as resolved Outdated

Maybe we should distinguish bucket options operation and object options operation (I mean naming s3middleware.OptionsOperation)

Maybe we should distinguish bucket options operation and object options operation (I mean naming `s3middleware.OptionsOperation`)
bktRouter.Head("/", named(s3middleware.HeadBucketOperation, h.HeadBucketHandler)) bktRouter.Head("/", named(s3middleware.HeadBucketOperation, h.HeadBucketHandler))
@ -372,6 +372,8 @@ func objectRouter(h Handler, l *zap.Logger) chi.Router {
objRouter := chi.NewRouter() objRouter := chi.NewRouter()
objRouter.Use(s3middleware.AddObjectName(l)) objRouter.Use(s3middleware.AddObjectName(l))
objRouter.Options("/*", named(s3middleware.OptionsObjectOperation, h.Preflight))
dkirillov marked this conversation as resolved Outdated

I'm not sure we need this. Because we have already add (and invoke) this handler in bktRouter, so in object router we inherit it. Currently h.AppendCORSHeaders is being invoked twice for every object request

I'm not sure we need this. Because we have already add (and invoke) this handler in bktRouter, so in object router we inherit it. Currently `h.AppendCORSHeaders` is being invoked twice for every object request
objRouter.Head("/*", named(s3middleware.HeadObjectOperation, h.HeadObjectHandler)) objRouter.Head("/*", named(s3middleware.HeadObjectOperation, h.HeadObjectHandler))
// GET method handlers // GET method handlers