chore: Ensure cancelled errors return proper (#6200)

Emyrk · web-flow · commit 80bde1e2c908 · 2023-02-14T10:57:19.000-06:00
The authz library returns a 404 if the authorization fails. If the
context is cancelled, then a 404 message is inaccurate.
Add a unit test to ensure context cancelled errors are raised
properly
diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go
@@ -61,6 +61,7 @@ func logNotAuthorizedError(ctx context.Context, logger slog.Logger, err error) e
 			slog.Error(err),
 		)
 	}
+
 	return NotAuthorizedError{
 		Err: err,
 	}
diff --git a/coderd/database/dbauthz/querier.go b/coderd/database/dbauthz/querier.go
@@ -438,12 +438,16 @@ func (q *querier) canAssignRoles(ctx context.Context, orgID *uuid.UUID, added, r
 		}
 	}
 
-	if len(added) > 0 && q.authorizeContext(ctx, rbac.ActionCreate, roleAssign) != nil {
-		return logNotAuthorizedError(ctx, q.log, xerrors.Errorf("not authorized to assign roles"))
+	if len(added) > 0 {
+		if err := q.authorizeContext(ctx, rbac.ActionCreate, roleAssign); err != nil {
+			return err
+		}
 	}
 
-	if len(removed) > 0 && q.authorizeContext(ctx, rbac.ActionDelete, roleAssign) != nil {
-		return logNotAuthorizedError(ctx, q.log, xerrors.Errorf("not authorized to delete roles"))
+	if len(removed) > 0 {
+		if err := q.authorizeContext(ctx, rbac.ActionDelete, roleAssign); err != nil {
+			return err
+		}
 	}
 
 	for _, roleName := range grantedRoles {
@@ -1221,7 +1225,7 @@ func (q *querier) GetWorkspaceAgentsByResourceIDs(ctx context.Context, ids []uui
 			continue
 		}
 		// Otherwise, we cannot read the workspace, so we cannot read the agent.
-		return nil, logNotAuthorizedError(ctx, q.log, err)
+		return nil, err
 	}
 	return agents, nil
 }
diff --git a/coderd/database/dbauthz/setup_test.go b/coderd/database/dbauthz/setup_test.go
@@ -9,11 +9,11 @@ import (
 	"strings"
 	"testing"
 
-	"golang.org/x/xerrors"
-
 	"github.com/google/uuid"
+	"github.com/open-policy-agent/opa/topdown"
 	"github.com/stretchr/testify/require"
 	"github.com/stretchr/testify/suite"
+	"golang.org/x/xerrors"
 
 	"cdr.dev/slog"
 	"github.com/coder/coder/coderd/coderdtest"
@@ -225,6 +225,26 @@ func (s *MethodTestSuite) NotAuthorizedErrorTest(ctx context.Context, az *coderd
 			s.ErrorAs(err, &dbauthz.NotAuthorizedError{}, "error should be NotAuthorizedError")
 		}
 	})
+
+	s.Run("Cancelled", func() {
+		// Pass in a cancelled context
+		ctx, cancel := context.WithCancel(ctx)
+		cancel()
+		az.AlwaysReturn = rbac.ForbiddenWithInternal(&topdown.Error{Code: topdown.CancelErr},
+			rbac.Subject{}, "", rbac.Object{}, nil)
+
+		// If we have assertions, that means the method should FAIL
+		// if RBAC will disallow the request. The returned error should
+		// be expected to be a NotAuthorizedError.
+		resp, err := callMethod(ctx)
+
+		// This is unfortunate, but if we are using `Filter` the error returned will be nil. So filter out
+		// any case where the error is nil and the response is an empty slice.
+		if err != nil || !hasEmptySliceResponse(resp) {
+			s.Errorf(err, "method should an error with cancellation")
+			s.ErrorIsf(err, context.Canceled, "error should match context.Cancelled")
+		}
+	})
 }
 
 func hasEmptySliceResponse(values []reflect.Value) bool {

Original file line number	Diff line number	Diff line change
`@@ -61,6 +61,7 @@ func logNotAuthorizedError(ctx context.Context, logger slog.Logger, err error) e`
`61`	`61`	`slog.Error(err),`
`62`	`62`	`)`
`63`	`63`	`}`
	`64`	`+`
`64`	`65`	`return NotAuthorizedError{`
`65`	`66`	`Err: err,`
`66`	`67`	`}`
Original file line number	Diff line number	Diff line change
`@@ -438,12 +438,16 @@ func (q querier) canAssignRoles(ctx context.Context, orgID uuid.UUID, added, r`
`438`	`438`	`}`
`439`	`439`	`}`
`440`	`440`
`441`		`- if len(added) > 0 && q.authorizeContext(ctx, rbac.ActionCreate, roleAssign) != nil {`
`442`		`- return logNotAuthorizedError(ctx, q.log, xerrors.Errorf("not authorized to assign roles"))`
	`441`	`+ if len(added) > 0 {`
	`442`	`+ if err := q.authorizeContext(ctx, rbac.ActionCreate, roleAssign); err != nil {`
	`443`	`+ return err`
	`444`	`+ }`
`443`	`445`	`}`
`444`	`446`
`445`		`- if len(removed) > 0 && q.authorizeContext(ctx, rbac.ActionDelete, roleAssign) != nil {`
`446`		`- return logNotAuthorizedError(ctx, q.log, xerrors.Errorf("not authorized to delete roles"))`
	`447`	`+ if len(removed) > 0 {`
	`448`	`+ if err := q.authorizeContext(ctx, rbac.ActionDelete, roleAssign); err != nil {`
	`449`	`+ return err`
	`450`	`+ }`
`447`	`451`	`}`
`448`	`452`
`449`	`453`	`for _, roleName := range grantedRoles {`
`@@ -1221,7 +1225,7 @@ func (q *querier) GetWorkspaceAgentsByResourceIDs(ctx context.Context, ids []uui`
`1221`	`1225`	`continue`
`1222`	`1226`	`}`
`1223`	`1227`	`// Otherwise, we cannot read the workspace, so we cannot read the agent.`
`1224`		`- return nil, logNotAuthorizedError(ctx, q.log, err)`
	`1228`	`+ return nil, err`
`1225`	`1229`	`}`
`1226`	`1230`	`return agents, nil`
`1227`	`1231`	`}`