diff --git a/gcp/storage/permissions.go b/gcp/storage/permissions.go index c63b1e0..6532e81 100644 --- a/gcp/storage/permissions.go +++ b/gcp/storage/permissions.go @@ -37,17 +37,11 @@ func (s *Server) needs(method, resource string, r *http.Request) []string { perms = append(perms, more...) } } - full := q.Get("projection") == "full" switch method { case "storage.buckets.insert": add(q.Get("enableObjectRetention") == "true", "storage.buckets.enableObjectRetention") - case "storage.buckets.list", "storage.buckets.get": - add(full, "storage.buckets.getIamPolicy") case "storage.buckets.patch": add(q.Get("predefinedAcl") != "" || q.Get("predefinedDefaultObjectAcl") != "", "storage.buckets.setIamPolicy") - add(full, "storage.buckets.getIamPolicy") - case "storage.objects.list": - add(full, "storage.objects.getIamPolicy") case "storage.objects.insert": add(s.exists(resource), "storage.objects.delete") } diff --git a/gcp/storage/server.go b/gcp/storage/server.go index 841d988..cddc27e 100644 --- a/gcp/storage/server.go +++ b/gcp/storage/server.go @@ -65,10 +65,12 @@ // [WithPolicy] grants the caller a set of IAM permissions and refuses a call // that needs one it lacks with 403 forbidden, before any fault. A call needs // its method's permission, and those its parameters add, as each method's -// reference lists them: projection=full needs getIamPolicy, a predefined ACL -// on a patch storage.buckets.setIamPolicy, and an upload that replaces a live -// object storage.objects.delete. Permissions are not checked against the -// bucket they act on: a grant covers every bucket. +// reference lists them: a predefined ACL on a patch needs +// storage.buckets.setIamPolicy, and an upload that replaces a live object +// storage.objects.delete. projection=full needs nothing more: Cloud Storage +// leaves out the ACLs a caller may not read rather than refusing the call, +// and the official clients ask for it on every get and list. Permissions are +// not checked against the bucket they act on: a grant covers every bucket. // // A lost response aborts the handler with [http.ErrAbortHandler] after // sending the response header, so the [http.Server] serving it closes the diff --git a/sdktest/gcp/storage/permissions_test.go b/sdktest/gcp/storage/permissions_test.go index 042b3b4..79f4576 100644 --- a/sdktest/gcp/storage/permissions_test.go +++ b/sdktest/gcp/storage/permissions_test.go @@ -13,6 +13,26 @@ import ( "github.com/parallelworks/squall/gcp/storage" ) +// The client asks for projection=full on every get and list; Cloud Storage leaves out the ACLs rather than refusing. +func TestPolicyLetsAReaderWithoutIamPolicyGetAndList(t *testing.T) { + policy := access.Allow("storage.buckets.create", "storage.objects.create", "storage.buckets.get", "storage.objects.list") + c := newClient(t, storage.New(storage.WithPolicy(policy))) + if err := c.Bucket("data").Create(t.Context(), project, nil); err != nil { + t.Fatal(err) + } + write(t, c, "data", "a.txt", "one") + + if _, err := c.Bucket("data").Attrs(t.Context()); err != nil { + t.Errorf("get bucket: %v", err) + } + if _, err := c.Bucket("data").Objects(t.Context(), nil).Next(); err != nil { + t.Errorf("list objects: %v", err) + } + if denials := policy.Denials(); len(denials) != 0 { + t.Errorf("Denials() = %+v, want none", denials) + } +} + func TestPolicyNeedsDeleteToOverwriteAnObject(t *testing.T) { policy := access.Allow("storage.buckets.create", "storage.objects.create", "storage.objects.list") c := newClient(t, storage.New(storage.WithPolicy(policy)))