Adding unit tests for rbac checks

coder · Emyrk · Mar 21, 2023 · Mar 16, 2023 · Mar 17, 2023 · Mar 17, 2023
commit 7672f6e60dad72822c6e808e9009d79f35dc877d
diff --git a/coderd/coderdtest/authorize.go b/coderd/coderdtest/authorize.go
@@ -3,6 +3,7 @@ package coderdtest
 import (
 	"context"
 	"fmt"
+	"path/filepath"
 	"runtime"
 	"strings"
 	"sync"
@@ -22,19 +23,6 @@ import (
 	"github.com/coder/coder/cryptorand"
 )
 
-type CoderSDKObject interface {
-	codersdk.User
-}
-
-func RBACObject[C CoderSDKObject](o C) rbac.Object {
-	switch ro := any(o).(type) {
-	case codersdk.User:
-		return rbac.ResourceUser.WithID(ro.ID)
-	default:
-		panic("unknown object type")
-	}
-}
-
 // RBACAsserter is a helper for asserting that the correct RBAC checks are
 // performed. This struct is tied to a given user, and only authorizes calls
 // for this user are checked.
@@ -108,7 +96,7 @@ type AuthCall struct {
 	rbac.AuthCall
 
 	asserted bool
-	caller   string
+	callers  []string
 }
 
 var _ rbac.Authorizer = (*RecordingAuthorizer)(nil)
@@ -219,27 +207,38 @@ func (r *RecordingAuthorizer) recordAuthorize(subject rbac.Subject, action rbac.
 	r.Lock()
 	defer r.Unlock()
 
-	pc, file, line, ok := runtime.Caller(2)
-	i := strings.Index(file, "coder")
-	if i >= 0 {
-		file = file[i:]
-	}
-	caller := fmt.Sprintf("%s:%d", file, line)
-	if ok {
-		f := runtime.FuncForPC(pc)
-		caller += " " + strings.TrimPrefix(f.Name(), "github.com/coder")
-	}
-
 	r.Called = append(r.Called, AuthCall{
 		AuthCall: rbac.AuthCall{
 			Actor:  subject,
 			Action: action,
 			Object: object,
 		},
-		caller: caller,
+		callers: []string{
+			// This is a decent stack trace for debugging.
+			// Some dbauthz calls are a bit nested, so we skip a few.
+			caller(2),
+			caller(3),
+			caller(4),
+			caller(5),
+		},
 	})
 }
 
+func caller(skip int) string {
+	pc, file, line, ok := runtime.Caller(skip + 1)
+	i := strings.Index(file, "coder")
+	if i >= 0 {
+		file = file[i:]
+	}
+	str := fmt.Sprintf("%s:%d", file, line)
+	if ok {
+		f := runtime.FuncForPC(pc)
+		filepath.Base(f.Name())
+		str += " | " + filepath.Base(f.Name())
+	}
+	return str
+}
+
 func (r *RecordingAuthorizer) Authorize(ctx context.Context, subject rbac.Subject, action rbac.Action, object rbac.Object) error {
 	r.recordAuthorize(subject, action, object)
 	if r.Wrapped == nil {

diff --git a/coderd/members.go b/coderd/members.go
@@ -34,7 +34,6 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) {
 		organization = httpmw.OrganizationParam(r)
 		member       = httpmw.OrganizationMemberParam(r)
 		apiKey       = httpmw.APIKey(r)
-		actorRoles   = httpmw.UserAuthorization(r)
 	)
 
 	if apiKey.UserID == member.UserID {

diff --git a/coderd/templateversions.go b/coderd/templateversions.go
@@ -1134,10 +1134,8 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht
 		return
 	}
 
-	var template database.Template
 	if req.TemplateID != uuid.Nil {
-		var err error
-		template, err = api.Database.GetTemplateByID(ctx, req.TemplateID)
+		_, err := api.Database.GetTemplateByID(ctx, req.TemplateID)
 		if errors.Is(err, sql.ErrNoRows) {
 			httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{
 				Message: "Template does not exist.",
@@ -1153,17 +1151,6 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht
 		}
 	}
 
-	if template.ID != uuid.Nil {
-		if !api.Authorize(r, rbac.ActionCreate, template) {
-			httpapi.ResourceNotFound(rw)
-			return
-		}
-	} else if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceTemplate.InOrg(organization.ID)) {
-		// Making a new template version is the same permission as creating a new template.
-		httpapi.ResourceNotFound(rw)
-		return
-	}
-
 	// Ensures the "owner" is properly applied.
 	tags := provisionerdserver.MutateTags(apiKey.UserID, req.ProvisionerTags)
 

diff --git a/coderd/templateversions_test.go b/coderd/templateversions_test.go
@@ -17,6 +17,7 @@ import (
 	"github.com/coder/coder/coderd/database"
 	"github.com/coder/coder/coderd/gitauth"
 	"github.com/coder/coder/coderd/provisionerdserver"
+	"github.com/coder/coder/coderd/rbac"
 	"github.com/coder/coder/codersdk"
 	"github.com/coder/coder/examples"
 	"github.com/coder/coder/provisioner/echo"
@@ -28,14 +29,19 @@ func TestTemplateVersion(t *testing.T) {
 	t.Parallel()
 	t.Run("Get", func(t *testing.T) {
 		t.Parallel()
-		client := coderdtest.New(t, nil)
+		client, _, api := coderdtest.NewWithAPI(t, nil)
 		user := coderdtest.CreateFirstUser(t, client)
+		authz := coderdtest.AssertRBAC(t, api, client).Reset()
+
 		version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
+		authz.AssertChecked(t, rbac.ActionCreate, rbac.ResourceTemplate.InOrg(user.OrganizationID))
 
 		ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
 		defer cancel()
 
-		_, err := client.TemplateVersion(ctx, version.ID)
+		authz.Reset()
+		tv, err := client.TemplateVersion(ctx, version.ID)
+		authz.AssertChecked(t, rbac.ActionRead, tv.RBACObjectNoTemplate())
 		require.NoError(t, err)
 	})
 

diff --git a/coderd/users.go b/coderd/users.go
@@ -798,7 +798,6 @@ func (api *API) putUserRoles(rw http.ResponseWriter, r *http.Request) {
 		ctx = r.Context()
 		// User is the user to modify.
 		user              = httpmw.UserParam(r)
-		actorRoles        = httpmw.UserAuthorization(r)
 		apiKey            = httpmw.APIKey(r)
 		auditor           = *api.Auditor.Load()
 		aReq, commitAudit = audit.InitRequest[database.User](rw, &audit.RequestParams{

diff --git a/coderd/users_test.go b/coderd/users_test.go
@@ -288,21 +288,27 @@ func TestDeleteUser(t *testing.T) {
 	t.Parallel()
 	t.Run("Works", func(t *testing.T) {
 		t.Parallel()
-		api := coderdtest.New(t, nil)
-		user := coderdtest.CreateFirstUser(t, api)
-		_, another := coderdtest.CreateAnotherUser(t, api, user.OrganizationID)
-		err := api.DeleteUser(context.Background(), another.ID)
+		client, _, api := coderdtest.NewWithAPI(t, nil)
+		user := coderdtest.CreateFirstUser(t, client)
+		authz := coderdtest.AssertRBAC(t, api, client)
+
+		_, another := coderdtest.CreateAnotherUser(t, client, user.OrganizationID)
+		err := client.DeleteUser(context.Background(), another.ID)
 		require.NoError(t, err)
 		// Attempt to create a user with the same email and username, and delete them again.
-		another, err = api.CreateUser(context.Background(), codersdk.CreateUserRequest{
+		another, err = client.CreateUser(context.Background(), codersdk.CreateUserRequest{
 			Email:          another.Email,
 			Username:       another.Username,
 			Password:       "SomeSecurePassword!",
 			OrganizationID: user.OrganizationID,
 		})
 		require.NoError(t, err)
-		err = api.DeleteUser(context.Background(), another.ID)
+		err = client.DeleteUser(context.Background(), another.ID)
 		require.NoError(t, err)
+
+		// RBAC checks
+		authz.AssertChecked(t, rbac.ActionCreate, rbac.ResourceUser)
+		authz.AssertChecked(t, rbac.ActionDelete, coderdtest.RBACObject(another))
 	})
 	t.Run("NoPermission", func(t *testing.T) {
 		t.Parallel()

diff --git a/codersdk/rbac.go b/codersdk/rbac.go
@@ -0,0 +1,19 @@
+package codersdk
+
+import "github.com/coder/coder/coderd/rbac"
+
+// This mirrors modelmethods.go in /database.
+// For unit testing, it helps to have these convenience functions for asserting
+// rbac authorization calls.
+
+func (t Template) RBACObject(userACL, groupACL map[string][]rbac.Action) rbac.Object {
+	return rbac.ResourceTemplate.WithID(t.ID).
+		InOrg(t.OrganizationID).
+		WithACLUserList(userACL).
+		WithGroupACL(groupACL)
+}
+
+// RBACObjectNoTemplate is for orphaned template versions.
+func (v TemplateVersion) RBACObjectNoTemplate() rbac.Object {
+	return rbac.ResourceTemplate.InOrg(v.OrganizationID)
+}