From 214b382e4f2de9005fd2a68deffd432e86a5744e Mon Sep 17 00:00:00 2001 From: Semir Patel Date: Mon, 18 Sep 2023 16:33:44 -0500 Subject: [PATCH] resource: mutate and validate before acls on write --- .../grpc-external/services/resource/write.go | 34 +++++++++---------- internal/resource/registry.go | 8 +++-- 2 files changed, 23 insertions(+), 19 deletions(-) diff --git a/agent/grpc-external/services/resource/write.go b/agent/grpc-external/services/resource/write.go index 2511e2f5da22..7110122313fa 100644 --- a/agent/grpc-external/services/resource/write.go +++ b/agent/grpc-external/services/resource/write.go @@ -49,15 +49,6 @@ func (s *Server) Write(ctx context.Context, req *pbresource.WriteRequest) (*pbre } v1EntMetaToV2Tenancy(reg, v1EntMeta, req.Resource.Id.Tenancy) - // ACL check comes before tenancy existence checks to not leak tenancy "existence". - err = reg.ACLs.Write(authz, authzContext, req.Resource) - switch { - case acl.IsErrPermissionDenied(err): - return nil, status.Error(codes.PermissionDenied, err.Error()) - case err != nil: - return nil, status.Errorf(codes.Internal, "failed write acl: %v", err) - } - // Check the user sent the correct type of data. if req.Resource.Data != nil && !req.Resource.Data.MessageIs(reg.Proto) { got := strings.TrimPrefix(req.Resource.Data.TypeUrl, "type.googleapis.com/") @@ -70,6 +61,23 @@ func (s *Server) Write(ctx context.Context, req *pbresource.WriteRequest) (*pbre ) } + if err = reg.Mutate(req.Resource); err != nil { + return nil, status.Errorf(codes.Internal, "failed mutate hook: %v", err.Error()) + } + + if err = reg.Validate(req.Resource); err != nil { + return nil, status.Error(codes.InvalidArgument, err.Error()) + } + + // ACL check comes before tenancy existence checks to not leak tenancy "existence". + err = reg.ACLs.Write(authz, authzContext, req.Resource) + switch { + case acl.IsErrPermissionDenied(err): + return nil, status.Error(codes.PermissionDenied, err.Error()) + case err != nil: + return nil, status.Errorf(codes.Internal, "failed write acl: %v", err) + } + // Check V1 tenancy exists for the V2 resource if err = v1TenancyExists(reg, s.TenancyBridge, req.Resource.Id.Tenancy, codes.InvalidArgument); err != nil { return nil, err @@ -80,14 +88,6 @@ func (s *Server) Write(ctx context.Context, req *pbresource.WriteRequest) (*pbre return nil, err } - if err = reg.Mutate(req.Resource); err != nil { - return nil, status.Errorf(codes.Internal, "failed mutate hook: %v", err.Error()) - } - - if err = reg.Validate(req.Resource); err != nil { - return nil, status.Error(codes.InvalidArgument, err.Error()) - } - // At the storage backend layer, all writes are CAS operations. // // This makes it possible to *safely* do things like keeping the Uid stable diff --git a/internal/resource/registry.go b/internal/resource/registry.go index db3e4d3d0c32..b8afa6840501 100644 --- a/internal/resource/registry.go +++ b/internal/resource/registry.go @@ -49,14 +49,18 @@ type Registration struct { Proto proto.Message // ACLs are hooks called to perform authorization on RPCs. + // The hooks can assume that Validate has been called. ACLs *ACLHooks // Validate is called to structurally validate the resource (e.g. - // check for required fields). + // check for required fields). Validate can assume that Mutate + // has been called. Validate func(*pbresource.Resource) error // Mutate is called to fill out any autogenerated fields (e.g. UUIDs) or - // apply defaults before validation. + // apply defaults before validation. Mutate can assume that + // Resource.ID is populated and has non-empty tenancy fields. This does + // not mean those tenancy fields actually exist. Mutate func(*pbresource.Resource) error // Scope describes the tenancy scope of a resource.