From 62796a14544edc918ac1503bc241f22c46060d46 Mon Sep 17 00:00:00 2001
From: Semir Patel <semir.patel@hashicorp.com>
Date: Mon, 18 Sep 2023 17:04:29 -0500
Subject: [PATCH] resource: mutate and validate before acls on write (#18868)

---
 .../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 2511e2f5da..7110122313 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 db3e4d3d0c..b8afa68405 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.