address more review feedback; attempt to simplify cached workspace is

cstyan · cstyan · commit a2ee19d77d30 · 2025-11-21T18:29:40.000Z
valid/present checks, plus ensure proper fallback (less noisy logging)
when a cached workspace is not found/valid in dbauthz

Signed-off-by: Callum Styan &lt;callumstyan@gmail.com&gt;
diff --git a/coderd/agentapi/cached_workspace.go b/coderd/agentapi/cached_workspace.go
@@ -73,9 +73,14 @@ func (cws *CachedWorkspaceFields) UpdateValues(ws database.Workspace) {
 	cws.AutostartSchedule = ws.AutostartSchedule
 }
 
-func (cws *CachedWorkspaceFields) AsDatabaseWorkspace() database.Workspace {
+// Returns the Workspace, true, unless the workspace has not been cached (nuked or was a prebuild).
+func (cws *CachedWorkspaceFields) AsDatabaseWorkspace() (database.Workspace, bool) {
 	cws.lock.RLock()
 	defer cws.lock.RUnlock()
+	// Should we be more explicit about all fields being set to be valid?
+	if cws.Equal(&CachedWorkspaceFields{}) {
+		return database.Workspace{}, false
+	}
 	return database.Workspace{
 		ID:                cws.ID,
 		OwnerID:           cws.OwnerID,
@@ -85,5 +90,5 @@ func (cws *CachedWorkspaceFields) AsDatabaseWorkspace() database.Workspace {
 		OwnerUsername:     cws.OwnerUsername,
 		TemplateName:      cws.TemplateName,
 		AutostartSchedule: cws.AutostartSchedule,
-	}
+	}, true
 }
diff --git a/coderd/agentapi/metadata.go b/coderd/agentapi/metadata.go
@@ -111,12 +111,16 @@ func (a *MetadataAPI) BatchUpdateMetadata(ctx context.Context, req *agentproto.B
 
 	// Inject RBAC object into context for dbauthz fast path, avoid having to
 	// call GetWorkspaceByAgentID on every metadata update.
-	rbacCtx, err := dbauthz.WithWorkspaceRBAC(ctx, a.Workspace.AsDatabaseWorkspace().RBACObject())
-	if err != nil {
-		// Don't use error level here; that will fail the call but in reality an invalid object here is okay
-		// since we will then just fallback to the actual GetWorkspaceByAgentID call in dbauthz.
-		a.Log.Warn(ctx, "cached RBAC Workspace context wrapping was invalid", slog.Error(err))
+	rbacCtx := ctx
+	if dbws, ok := a.Workspace.AsDatabaseWorkspace(); ok {
+		rbacCtx, err = dbauthz.WithWorkspaceRBAC(ctx, dbws.RBACObject())
+		if err != nil {
+			// Don't error level log here, will exit the function. We want to fall back to GetWorkspaceByAgentID.
+			//nolint:gocritic
+			a.Log.Debug(ctx, "Cached workspace was present but RBAC object was invalid", slog.F("err", err))
+		}
 	}
+
 	err = a.Database.UpdateWorkspaceAgentMetadata(rbacCtx, dbUpdate)
 	if err != nil {
 		return nil, xerrors.Errorf("update workspace agent metadata in database: %w", err)
diff --git a/coderd/agentapi/stats.go b/coderd/agentapi/stats.go
@@ -4,7 +4,6 @@ import (
 	"context"
 	"time"
 
-	"github.com/google/uuid"
 	"golang.org/x/xerrors"
 	"google.golang.org/protobuf/types/known/durationpb"
 
@@ -50,8 +49,9 @@ func (a *StatsAPI) UpdateStats(ctx context.Context, req *agentproto.UpdateStatsR
 	}
 
 	// If cache is empty (prebuild or invalid), fall back to DB
-	ws := a.Workspace.AsDatabaseWorkspace()
-	if a.Workspace.ID == uuid.Nil {
+	var ws database.Workspace
+	var ok bool
+	if ws, ok = a.Workspace.AsDatabaseWorkspace(); !ok {
 		ws, err = a.Database.GetWorkspaceByAgentID(ctx, workspaceAgent.ID)
 		if err != nil {
 			return nil, xerrors.Errorf("get workspace by agent ID %q: %w", workspaceAgent.ID, err)
diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go
@@ -5510,25 +5510,15 @@ func (q *querier) UpdateWorkspaceAgentMetadata(ctx context.Context, arg database
 	// Fast path: Check if we have an RBAC object in context.
 	// This is set by the workspace agent RPC handler to avoid the expensive
 	// GetWorkspaceByAgentID query for every metadata update.
-	//
-	// NOTE: The cached RBAC object is refreshed every 5 minutes. After a prebuild
-	// claim, there may be up to a 5-minute window where this uses stale owner_id.
+	// NOTE: The cached RBAC object is refreshed every 5 minutes in agentapi/api.go.
 	if rbacObj, ok := WorkspaceRBACFromContext(ctx); ok {
-		// Verify the RBAC object is valid (has required fields).
-		if !rbacObj.IsEmpty() && rbacObj.Type == rbac.ResourceWorkspace.Type {
-			act, ok := ActorFromContext(ctx)
-			if !ok {
-				return ErrNoActor
-			}
-			// Errors here will result in falling back to the GetWorkspaceAgentByID query, skipping
-			// the cache in case the cached data is stale.
-
-			if err := q.auth.Authorize(ctx, act, policy.ActionUpdate, rbacObj); err == nil {
-				return q.db.UpdateWorkspaceAgentMetadata(ctx, arg)
-			}
-			q.log.Debug(ctx, "fast path authorization failed, using slow path",
-				slog.F("agent_id", arg.WorkspaceAgentID))
+		// Errors here will result in falling back to the GetWorkspaceAgentByID query, skipping
+		// the cache in case the cached data is stale.
+		if err := q.authorizeContext(ctx, policy.ActionUpdate, rbacObj); err == nil {
+			return q.db.UpdateWorkspaceAgentMetadata(ctx, arg)
 		}
+		q.log.Debug(ctx, "fast path authorization failed, using slow path",
+			slog.F("agent_id", arg.WorkspaceAgentID))
 	}
 
 	// Slow path: Fallback to fetching the workspace for authorization if the RBAC object is not present (or is invalid)
