diff --git a/weed/s3api/s3_action_resolver.go b/weed/s3api/s3_action_resolver.go index 987a244cb..3a1e7db4c 100644 --- a/weed/s3api/s3_action_resolver.go +++ b/weed/s3api/s3_action_resolver.go @@ -116,11 +116,6 @@ var bucketQueryActions = map[string]map[string]string{ http.MethodPut: s3_constants.S3_ACTION_PUT_BUCKET_OWNERSHIP_CONTROLS, http.MethodDelete: s3_constants.S3_ACTION_PUT_BUCKET_OWNERSHIP_CONTROLS, // DELETE uses same permission as PUT }, - // SeaweedFS extension: bucket quota subresource - "seaweedfs-quota": { - http.MethodGet: s3_constants.S3_ACTION_GET_BUCKET_QUOTA, - http.MethodPut: s3_constants.S3_ACTION_PUT_BUCKET_QUOTA, - }, } // resolveFromQueryParameters checks query parameters to determine specific S3 actions @@ -267,6 +262,17 @@ func resolveFromQueryParameters(query url.Values, method string, hasObject bool) return s3_constants.S3_ACTION_DELETE_OBJECT } + // SeaweedFS extension: the quota routes are registered last among the + // bucket subresource routes, so resolve seaweedfs-quota last to match. + if !hasObject && query.Has("seaweedfs-quota") { + switch method { + case http.MethodGet: + return s3_constants.S3_ACTION_GET_BUCKET_QUOTA + case http.MethodPut: + return s3_constants.S3_ACTION_PUT_BUCKET_QUOTA + } + } + return "" } diff --git a/weed/s3api/s3_action_resolver_test.go b/weed/s3api/s3_action_resolver_test.go index 7aa0ee03d..077b23dbd 100644 --- a/weed/s3api/s3_action_resolver_test.go +++ b/weed/s3api/s3_action_resolver_test.go @@ -185,6 +185,33 @@ func TestResolveS3Action_ListType(t *testing.T) { } } +// The seaweedfs-quota routes are registered last among the bucket subresource +// routes, so the resolver must resolve it last as well: a request combining it +// with another selector is served by that selector's handler, and authorization +// must name the same operation. +func TestResolveS3Action_Quota(t *testing.T) { + tests := []struct { + name string + method string + query string + want string + }{ + {"quota get", http.MethodGet, "seaweedfs-quota", s3_constants.S3_ACTION_GET_BUCKET_QUOTA}, + {"quota put", http.MethodPut, "seaweedfs-quota", s3_constants.S3_ACTION_PUT_BUCKET_QUOTA}, + {"policy wins over quota", http.MethodPut, "policy&seaweedfs-quota", s3_constants.S3_ACTION_PUT_BUCKET_POLICY}, + {"quota after policy wins too", http.MethodPut, "seaweedfs-quota&policy", s3_constants.S3_ACTION_PUT_BUCKET_POLICY}, + {"tagging wins over quota", http.MethodPut, "tagging&seaweedfs-quota", s3_constants.S3_ACTION_PUT_BUCKET_TAGGING}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r, _ := http.NewRequest(tt.method, "http://localhost/bucket?"+tt.query, nil) + if got := ResolveS3Action(r, s3_constants.ACTION_ADMIN, "bucket", ""); got != tt.want { + t.Errorf("ResolveS3Action() = %q, want %q", got, tt.want) + } + }) + } +} + // A base action naming another service carries no S3 request shape, so a query // parameter on the request must not redirect it to an S3 action. func TestResolveS3ActionKeepsNonS3Service(t *testing.T) { diff --git a/weed/s3api/s3api_ambiguous_subresource_test.go b/weed/s3api/s3api_ambiguous_subresource_test.go index a51a9a643..7af21e0a3 100644 --- a/weed/s3api/s3api_ambiguous_subresource_test.go +++ b/weed/s3api/s3api_ambiguous_subresource_test.go @@ -3,8 +3,10 @@ package s3api import ( "net/http" "net/http/httptest" + "strings" "testing" + "github.com/gorilla/mux" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -27,6 +29,7 @@ func TestAmbiguousSubresource(t *testing.T) { "uploads=&prefix=a&x-id=CreateMultipartUpload", "list-type=2&prefix=a&continuation-token=x", "acl=&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Signature=deadbeef", + "seaweedfs-quota=", } { req, _ := http.NewRequest("GET", "http://localhost/bucket/key?"+query, nil) assert.False(t, hasAmbiguousSubresource(req.URL.Query()), "%q names one operation", query) @@ -49,6 +52,9 @@ func TestAmbiguousSubresource(t *testing.T) { "list-type=2&tagging=", "ownershipControls=&list-type=2", "list-type=2&versions=", + "policy=&seaweedfs-quota=", + "seaweedfs-quota=&policy=", + "seaweedfs-quota=&tagging=", } { req, _ := http.NewRequest("PUT", "http://localhost/bucket?"+query, nil) assert.True(t, hasAmbiguousSubresource(req.URL.Query()), "%q names two operations", query) @@ -101,3 +107,36 @@ func TestListTypeOwnershipControlsRejectedBeforeHandler(t *testing.T) { handler.ServeHTTP(rec, req) assert.True(t, served, "a plain list-type request must still be served") } + +// Every query key a route can be selected by must be classified: an operation +// subresource counted by the ambiguity guard, or a parameter that only +// modifies the operation it accompanies. A selector left out of both +// authorizes as one operation while the router serves another. +func TestRouteQueryKeysAreClassified(t *testing.T) { + // Action selects STS routes on the root router, id narrows a config + // subresource, partNumber accompanies uploadId; none select an S3 + // operation on their own. + modifiers := map[string]bool{ + "Action": true, "id": true, "partNumber": true, + } + + router := mux.NewRouter() + setupRoutingTestServer(t).registerRouter(router) + err := router.Walk(func(route *mux.Route, _ *mux.Router, _ []*mux.Route) error { + templates, err := route.GetQueriesTemplates() + if err != nil { + return nil + } + for _, q := range templates { + key, _, _ := strings.Cut(q, "=") + if modifiers[key] { + continue + } + _, resolved := bucketQueryActions[key] + assert.True(t, operationSubresources[key] || resolved, + "route query key %q selects an operation but is not counted by hasAmbiguousSubresource", key) + } + return nil + }) + require.NoError(t, err) +} diff --git a/weed/s3api/s3api_path_validation.go b/weed/s3api/s3api_path_validation.go index 6a76554c9..3e5d489c1 100644 --- a/weed/s3api/s3api_path_validation.go +++ b/weed/s3api/s3api_path_validation.go @@ -53,14 +53,17 @@ var operationSubresources = map[string]bool{ "location": true, "logging": true, "metrics": true, "notification": true, "object-lock": true, "ownershipControls": true, "policy": true, "policyStatus": true, "publicAccessBlock": true, "renameObject": true, "replication": true, - "requestPayment": true, "retention": true, "tagging": true, "uploadId": true, + "requestPayment": true, "retention": true, "seaweedfs-quota": true, + "tagging": true, "uploadId": true, "uploads": true, "versioning": true, "versions": true, "website": true, } func hasAmbiguousSubresource(query url.Values) bool { seen := 0 for key := range query { - if !operationSubresources[key] { + // bucketQueryActions keys select an operation by definition, so they + // count even if operationSubresources was not updated for them. + if _, ok := bucketQueryActions[key]; !ok && !operationSubresources[key] { continue } if seen++; seen > 1 {