Verify the correct error is returned on disallow auth

Emyrk · Emyrk · commit cf763cb41857 · 2023-02-03T08:37:18.000-06:00
diff --git a/coderd/authzquery/methods_test.go b/coderd/authzquery/methods_test.go
@@ -2,12 +2,15 @@ package authzquery_test
 
 import (
 	"context"
+	"database/sql"
 	"fmt"
 	"reflect"
 	"sort"
 	"strings"
 	"testing"
 
+	"golang.org/x/xerrors"
+
 	"github.com/coder/coder/coderd/rbac/regosql"
 
 	"github.com/google/uuid"
@@ -95,10 +98,11 @@ func (s *MethodTestSuite) RunMethodTest(testCaseF func(t *testing.T, db database
 	s.methodAccounting[methodName]++
 
 	db := dbfake.New()
+	fakeAuthorizer := &coderdtest.FakeAuthorizer{
+		AlwaysReturn: nil,
+	}
 	rec := &coderdtest.RecordingAuthorizer{
-		Wrapped: &coderdtest.FakeAuthorizer{
-			AlwaysReturn: nil,
-		},
+		Wrapped: fakeAuthorizer,
 	}
 	az := authzquery.NewAuthzQuerier(db, rec, slog.Make())
 	actor := rbac.Subject{
@@ -118,22 +122,25 @@ MethodLoop:
 	for i := 0; i < azt.NumMethod(); i++ {
 		method := azt.Method(i)
 		if method.Name == methodName {
+			if len(testCase.Assertions) > 0 {
+				fakeAuthorizer.AlwaysReturn = xerrors.New("Always fail authz")
+				// 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.
+				erroredResp := reflect.ValueOf(az).Method(i).Call(append([]reflect.Value{reflect.ValueOf(ctx)}, testCase.Inputs...))
+				err := findError(t, erroredResp)
+				require.Errorf(t, err, "method %q should an error with disallow authz", testName)
+				require.ErrorIsf(t, err, sql.ErrNoRows, "error should match sql.ErrNoRows")
+				require.ErrorAs(t, err, &authzquery.NotAuthorizedError{}, "error should be NotAuthorizedError")
+				// Set things back to normal.
+				fakeAuthorizer.AlwaysReturn = nil
+				rec.Reset()
+			}
+
 			resp := reflect.ValueOf(az).Method(i).Call(append([]reflect.Value{reflect.ValueOf(ctx)}, testCase.Inputs...))
 			// TODO: Should we assert the object returned is the correct one?
-			for _, r := range resp {
-				if r.Type().Implements(reflect.TypeOf((*error)(nil)).Elem()) {
-					if r.IsNil() {
-						// no error!
-						break
-					}
-					err, ok := r.Interface().(error)
-					if !ok {
-						t.Fatal("error is not an error?!")
-					}
-					require.NoError(t, err, "method %q returned an error", testName)
-					break
-				}
-			}
+			err := findError(t, resp)
+			require.NoError(t, err, "method %q returned an error", testName)
 			found = true
 			break MethodLoop
 		}
@@ -155,6 +162,24 @@ MethodLoop:
 	require.NoError(t, rec.AllAsserted(), "all rbac calls must be asserted")
 }
 
+func findError(t *testing.T, values []reflect.Value) error {
+	for _, r := range values {
+		if r.Type().Implements(reflect.TypeOf((*error)(nil)).Elem()) {
+			if r.IsNil() {
+				// Error is found, but it's nil!
+				return nil
+			}
+			err, ok := r.Interface().(error)
+			if !ok {
+				t.Fatal("error is not an error?!")
+			}
+			return err
+		}
+	}
+	t.Fatal("no expected error value found in responses (error can be nil)")
+	panic("unreachable") // For compile reasons
+}
+
 // A MethodCase contains the inputs to be provided to a single method call,
 // and the assertions to be made on the RBAC checks.
 type MethodCase struct {
diff --git a/coderd/coderdtest/authorize.go b/coderd/coderdtest/authorize.go
@@ -486,7 +486,7 @@ func (a *AuthTester) Test(ctx context.Context, assertRoute map[string]RouteCheck
 				return nil
 			}
 			a.t.Run(name, func(t *testing.T) {
-				a.authorizer.reset()
+				a.authorizer.Reset()
 				routeKey := strings.TrimRight(name, "/")
 
 				routeAssertions, ok := assertRoute[routeKey]
@@ -676,7 +676,7 @@ func (r *RecordingAuthorizer) Prepare(_ context.Context, subject rbac.Subject, a
 	}, nil
 }
 
-func (r *RecordingAuthorizer) reset() {
+func (r *RecordingAuthorizer) Reset() {
 	r.Lock()
 	defer r.Unlock()
 	r.Called = nil
