[#399] Add OPTIONS method for object operations #399
7 changed files with 197 additions and 8 deletions
|
@ -187,8 +187,8 @@ func (h *handler) Preflight(w http.ResponseWriter, r *http.Request) {
|
|||
if !checkSubslice(rule.AllowedHeaders, headers) {
|
||||
continue
|
||||
}
|
||||
w.Header().Set(api.AccessControlAllowOrigin, o)
|
||||
w.Header().Set(api.AccessControlAllowMethods, strings.Join(rule.AllowedMethods, ", "))
|
||||
w.Header().Set(api.AccessControlAllowOrigin, origin)
|
||||
w.Header().Set(api.AccessControlAllowMethods, method)
|
||||
dkirillov marked this conversation as resolved
Outdated
|
||||
if headers != nil {
|
||||
w.Header().Set(api.AccessControlAllowHeaders, requestHeaders)
|
||||
}
|
||||
|
|
|
@ -7,6 +7,7 @@ import (
|
|||
|
||||
"git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api"
|
||||
"git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/middleware"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
func TestCORSOriginWildcard(t *testing.T) {
|
||||
|
@ -39,3 +40,181 @@ func TestCORSOriginWildcard(t *testing.T) {
|
|||
hc.Handler().GetBucketCorsHandler(w, r)
|
||||
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))
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
|
|
@ -5,7 +5,7 @@ const (
|
|||
|
||||
// bucket operations.
|
||||
|
||||
OptionsOperation = "Options"
|
||||
OptionsBucketOperation = "OptionsBucket"
|
||||
HeadBucketOperation = "HeadBucket"
|
||||
ListMultipartUploadsOperation = "ListMultipartUploads"
|
||||
GetBucketLocationOperation = "GetBucketLocation"
|
||||
|
@ -51,6 +51,7 @@ const (
|
|||
|
||||
// object operations.
|
||||
|
||||
OptionsObjectOperation = "OptionsObject"
|
||||
HeadObjectOperation = "HeadObject"
|
||||
ListPartsOperation = "ListParts"
|
||||
GetObjectACLOperation = "GetObjectACL"
|
||||
|
|
|
@ -103,7 +103,7 @@ func stats(f http.HandlerFunc, resolveCID cidResolveFunc, appMetrics *metrics.Ap
|
|||
|
||||
func requestTypeFromAPI(api string) metrics.RequestType {
|
||||
switch api {
|
||||
case OptionsOperation, HeadObjectOperation, HeadBucketOperation:
|
||||
case OptionsBucketOperation, OptionsObjectOperation, HeadObjectOperation, HeadBucketOperation:
|
||||
return metrics.HEADRequest
|
||||
case CreateMultipartUploadOperation, UploadPartCopyOperation, UploadPartOperation, CompleteMultipartUploadOperation,
|
||||
PutObjectACLOperation, PutObjectTaggingOperation, CopyObjectOperation, PutObjectRetentionOperation, PutObjectLegalHoldOperation,
|
||||
|
|
|
@ -253,7 +253,7 @@ func determineBucketOperation(r *http.Request) string {
|
|||
query := r.URL.Query()
|
||||
switch r.Method {
|
||||
case http.MethodOptions:
|
||||
return OptionsOperation
|
||||
return OptionsBucketOperation
|
||||
case http.MethodHead:
|
||||
return HeadBucketOperation
|
||||
case http.MethodGet:
|
||||
|
@ -356,6 +356,8 @@ func determineBucketOperation(r *http.Request) string {
|
|||
func determineObjectOperation(r *http.Request) string {
|
||||
query := r.URL.Query()
|
||||
switch r.Method {
|
||||
case http.MethodOptions:
|
||||
return OptionsObjectOperation
|
||||
case http.MethodHead:
|
||||
return HeadObjectOperation
|
||||
case http.MethodGet:
|
||||
|
|
|
@ -91,9 +91,9 @@ func TestDetermineBucketOperation(t *testing.T) {
|
|||
expected string
|
||||
}{
|
||||
{
|
||||
name: "OptionsOperation",
|
||||
name: "OptionsBucketOperation",
|
||||
method: http.MethodOptions,
|
||||
expected: OptionsOperation,
|
||||
expected: OptionsBucketOperation,
|
||||
},
|
||||
{
|
||||
name: "HeadBucketOperation",
|
||||
|
@ -367,6 +367,11 @@ func TestDetermineObjectOperation(t *testing.T) {
|
|||
headerKeys []string
|
||||
expected string
|
||||
}{
|
||||
{
|
||||
name: "OptionsObjectOperation",
|
||||
method: http.MethodOptions,
|
||||
expected: OptionsObjectOperation,
|
||||
},
|
||||
{
|
||||
name: "HeadObjectOperation",
|
||||
method: http.MethodHead,
|
||||
|
|
|
@ -223,7 +223,7 @@ func bucketRouter(h Handler, log *zap.Logger) chi.Router {
|
|||
|
||||
bktRouter.Mount("/", objectRouter(h, log))
|
||||
|
||||
bktRouter.Options("/", h.Preflight)
|
||||
bktRouter.Options("/", named(s3middleware.OptionsBucketOperation, h.Preflight))
|
||||
dkirillov marked this conversation as resolved
Outdated
dkirillov
commented
Maybe we should distinguish bucket options operation and object options operation (I mean naming Maybe we should distinguish bucket options operation and object options operation (I mean naming `s3middleware.OptionsOperation`)
|
||||
|
||||
bktRouter.Head("/", named(s3middleware.HeadBucketOperation, h.HeadBucketHandler))
|
||||
|
||||
|
@ -372,6 +372,8 @@ func objectRouter(h Handler, l *zap.Logger) chi.Router {
|
|||
objRouter := chi.NewRouter()
|
||||
objRouter.Use(s3middleware.AddObjectName(l))
|
||||
|
||||
objRouter.Options("/*", named(s3middleware.OptionsObjectOperation, h.Preflight))
|
||||
dkirillov marked this conversation as resolved
Outdated
dkirillov
commented
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 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))
|
||||
|
||||
// GET method handlers
|
||||
|
|
Loading…
Add table
Reference in a new issue
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