Skip to content

Commit

Permalink
Fix ACL errors for newly created and pre-existing blobs (#1016)
Browse files Browse the repository at this point in the history 
* Fix ACL errors for existing and pre-existing blobs

* internal/backend: generalize code a bit

* Remove some comments

Co-authored-by: francisco souza <108725+fsouza@users.noreply.github.com>
  • Loading branch information
@RachitSharma2001 @fsouza
RachitSharma2001 and fsouza committed Jan 8, 2023
1 parent cd642dc commit 34afa14
Show file tree
Hide file tree
Showing 7 changed files with 113 additions and 27 deletions.
26 changes: 23 additions & 3 deletions fakestorage/object.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -938,17 +938,37 @@ func (s *Server) patchObject(r *http.Request) jsonResponse {
vars := unescapeMuxVars(mux.Vars(r))
bucketName := vars["bucketName"]
objectName := vars["objectName"]
var metadata struct {

type acls struct {
Entity string
Role string
}

var payload struct {
Metadata map[string]string `json:"metadata"`
Acl []acls
}
err := json.NewDecoder(r.Body).Decode(&metadata)
err := json.NewDecoder(r.Body).Decode(&payload)
if err != nil {
return jsonResponse{
status: http.StatusBadRequest,
errorMessage: "Metadata in the request couldn't decode",
}
}
backendObj, err := s.backend.PatchObject(bucketName, objectName, metadata.Metadata)

var attrsToUpdate backend.ObjectAttrs

attrsToUpdate.Metadata = payload.Metadata

if len(payload.Acl) > 0 {
attrsToUpdate.ACL = []storage.ACLRule{}
for _, aclData := range payload.Acl {
newAcl := storage.ACLRule{Entity: storage.ACLEntity(aclData.Entity), Role: storage.ACLRole(aclData.Role)}
attrsToUpdate.ACL = append(attrsToUpdate.ACL, newAcl)
}
}

backendObj, err := s.backend.PatchObject(bucketName, objectName, attrsToUpdate)
if err != nil {
return jsonResponse{
status: http.StatusNotFound,
Expand Down
20 changes: 7 additions & 13 deletions internal/backend/fs.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -323,24 +323,18 @@ func (s *storageFS) DeleteObject(bucketName, objectName string) error {
return os.Remove(path)
}

// PatchObject patches the given object metadata.
func (s *storageFS) PatchObject(bucketName, objectName string, metadata map[string]string) (StreamingObject, error) {
func (s *storageFS) PatchObject(bucketName, objectName string, attrsToUpdate ObjectAttrs) (StreamingObject, error) {
obj, err := s.GetObject(bucketName, objectName)
if err != nil {
return StreamingObject{}, err
}
defer obj.Close()
if obj.Metadata == nil {
obj.Metadata = map[string]string{}
}
for k, v := range metadata {
obj.Metadata[k] = v
}
obj.Generation = 0 // reset generation id
return s.CreateObject(obj, NoConditions{}) // recreate object

obj.patch(attrsToUpdate)
obj.Generation = 0 // reset generation id
return s.CreateObject(obj, NoConditions{})
}

// UpdateObject replaces the given object metadata.
func (s *storageFS) UpdateObject(bucketName, objectName string, metadata map[string]string) (StreamingObject, error) {
obj, err := s.GetObject(bucketName, objectName)
if err != nil {
Expand All @@ -351,8 +345,8 @@ func (s *storageFS) UpdateObject(bucketName, objectName string, metadata map[str
for k, v := range metadata {
obj.Metadata[k] = v
}
obj.Generation = 0 // reset generation id
return s.CreateObject(obj, NoConditions{}) // recreate object
obj.Generation = 0 // reset generation id
return s.CreateObject(obj, NoConditions{})
}

type concatenatedContent struct {
Expand Down
15 changes: 5 additions & 10 deletions internal/backend/memory.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -299,19 +299,14 @@ func (s *storageMemory) DeleteObject(bucketName, objectName string) error {
return nil
}

// PatchObject updates an object metadata.
func (s *storageMemory) PatchObject(bucketName, objectName string, metadata map[string]string) (StreamingObject, error) {
func (s *storageMemory) PatchObject(bucketName, objectName string, attrsToUpdate ObjectAttrs) (StreamingObject, error) {
obj, err := s.GetObject(bucketName, objectName)
if err != nil {
return StreamingObject{}, err
}
if obj.Metadata == nil {
obj.Metadata = map[string]string{}
}
for k, v := range metadata {
obj.Metadata[k] = v
}
s.CreateObject(obj, NoConditions{}) // recreate object

obj.patch(attrsToUpdate)
s.CreateObject(obj, NoConditions{})
return obj, nil
}

Expand All @@ -325,7 +320,7 @@ func (s *storageMemory) UpdateObject(bucketName, objectName string, metadata map
for k, v := range metadata {
obj.Metadata[k] = v
}
s.CreateObject(obj, NoConditions{}) // recreate object
s.CreateObject(obj, NoConditions{})
return obj, nil
}

Expand Down
21 changes: 21 additions & 0 deletions internal/backend/object.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"bytes"
"fmt"
"io"
"reflect"

"cloud.google.com/go/storage"
)
Expand Down Expand Up @@ -81,3 +82,23 @@ func (o *StreamingObject) BufferedObject() (Object, error) {
Content: data,
}, err
}

func (o *StreamingObject) patch(attrsToUpdate ObjectAttrs) {
currObjValues := reflect.ValueOf(&(o.ObjectAttrs)).Elem()
currObjType := currObjValues.Type()
newObjValues := reflect.ValueOf(attrsToUpdate)
for i := 0; i < newObjValues.NumField(); i++ {
if reflect.Value.IsZero(newObjValues.Field(i)) {
continue
} else if currObjType.Field(i).Name == "Metadata" {
if o.Metadata == nil {
o.Metadata = map[string]string{}
}
for k, v := range attrsToUpdate.Metadata {
o.Metadata[k] = v
}
} else {
currObjValues.Field(i).Set(newObjValues.Field(i))
}
}
}
2 changes: 1 addition & 1 deletion internal/backend/storage.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ type Storage interface {
GetObject(bucketName, objectName string) (StreamingObject, error)
GetObjectWithGeneration(bucketName, objectName string, generation int64) (StreamingObject, error)
DeleteObject(bucketName, objectName string) error
PatchObject(bucketName, objectName string, metadata map[string]string) (StreamingObject, error)
PatchObject(bucketName, objectName string, attrsToUpdate ObjectAttrs) (StreamingObject, error)
UpdateObject(bucketName, objectName string, metadata map[string]string) (StreamingObject, error)
ComposeObject(bucketName string, objectNames []string, destinationName string, metadata map[string]string, contentType string) (StreamingObject, error)
}
Expand Down
7 changes: 7 additions & 0 deletions main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"path/filepath"
"syscall"

"cloud.google.com/go/storage"
"github.com/fsouza/fake-gcs-server/fakestorage"
"github.com/fsouza/fake-gcs-server/internal/checksum"
"github.com/fsouza/fake-gcs-server/internal/config"
Expand Down Expand Up @@ -104,6 +105,12 @@ func objectsFromBucket(localBucketPath, bucketName string) ([]fakestorage.Object
}
objects = append(objects, fakestorage.Object{
ObjectAttrs: fakestorage.ObjectAttrs{
ACL: []storage.ACLRule{
{
Entity: "projectOwner-test-project",
Role: "OWNER",
},
},
BucketName: bucketName,
Name: objectKey,
ContentType: mime.TypeByExtension(filepath.Ext(path)),
Expand Down
49 changes: 49 additions & 0 deletions main_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"os"
"testing"

"cloud.google.com/go/storage"
"github.com/fsouza/fake-gcs-server/fakestorage"
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
Expand Down Expand Up @@ -48,6 +49,12 @@ func TestGenerateObjectsFromFiles(t *testing.T) {
expectedObjects: []fakestorage.Object{
{
ObjectAttrs: fakestorage.ObjectAttrs{
ACL: []storage.ACLRule{
{
Entity: "projectOwner-test-project",
Role: "OWNER",
},
},
BucketName: "sample-bucket",
Name: "some_file.txt",
ContentType: testContentType,
Expand All @@ -63,6 +70,12 @@ func TestGenerateObjectsFromFiles(t *testing.T) {
expectedObjects: []fakestorage.Object{
{
ObjectAttrs: fakestorage.ObjectAttrs{
ACL: []storage.ACLRule{
{
Entity: "projectOwner-test-project",
Role: "OWNER",
},
},
BucketName: "some-bucket",
Name: "a/b/c/d/e/f/object1.txt",
ContentType: testContentType,
Expand All @@ -71,6 +84,12 @@ func TestGenerateObjectsFromFiles(t *testing.T) {
},
{
ObjectAttrs: fakestorage.ObjectAttrs{
ACL: []storage.ACLRule{
{
Entity: "projectOwner-test-project",
Role: "OWNER",
},
},
BucketName: "some-bucket",
Name: "a/b/c/d/e/f/object2.txt",
ContentType: testContentType,
Expand All @@ -79,6 +98,12 @@ func TestGenerateObjectsFromFiles(t *testing.T) {
},
{
ObjectAttrs: fakestorage.ObjectAttrs{
ACL: []storage.ACLRule{
{
Entity: "projectOwner-test-project",
Role: "OWNER",
},
},
BucketName: "some-bucket",
Name: "root-object.txt",
ContentType: testContentType,
Expand All @@ -101,6 +126,12 @@ func TestGenerateObjectsFromFiles(t *testing.T) {
expectedObjects: []fakestorage.Object{
{
ObjectAttrs: fakestorage.ObjectAttrs{
ACL: []storage.ACLRule{
{
Entity: "projectOwner-test-project",
Role: "OWNER",
},
},
BucketName: "bucket1",
Name: "object1.txt",
ContentType: testContentType,
Expand All @@ -109,6 +140,12 @@ func TestGenerateObjectsFromFiles(t *testing.T) {
},
{
ObjectAttrs: fakestorage.ObjectAttrs{
ACL: []storage.ACLRule{
{
Entity: "projectOwner-test-project",
Role: "OWNER",
},
},
BucketName: "bucket1",
Name: "object2.txt",
ContentType: testContentType,
Expand All @@ -117,6 +154,12 @@ func TestGenerateObjectsFromFiles(t *testing.T) {
},
{
ObjectAttrs: fakestorage.ObjectAttrs{
ACL: []storage.ACLRule{
{
Entity: "projectOwner-test-project",
Role: "OWNER",
},
},
BucketName: "bucket2",
Name: "object1.txt",
ContentType: testContentType,
Expand All @@ -125,6 +168,12 @@ func TestGenerateObjectsFromFiles(t *testing.T) {
},
{
ObjectAttrs: fakestorage.ObjectAttrs{
ACL: []storage.ACLRule{
{
Entity: "projectOwner-test-project",
Role: "OWNER",
},
},
BucketName: "bucket2",
Name: "object2.txt",
ContentType: testContentType,
Expand Down

0 comments on commit 34afa14

Please sign in to comment.