[#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) {
|
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
|
|||||||
if headers != nil {
|
if headers != nil {
|
||||||
w.Header().Set(api.AccessControlAllowHeaders, requestHeaders)
|
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"
|
||||||
"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))
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
|
@ -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"
|
||||||
|
|
|
@ -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,
|
||||||
|
|
|
@ -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:
|
||||||
|
|
|
@ -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,
|
||||||
|
|
|
@ -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
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))
|
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
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))
|
objRouter.Head("/*", named(s3middleware.HeadObjectOperation, h.HeadObjectHandler))
|
||||||
|
|
||||||
// GET method handlers
|
// 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