feat: Rbac more coderd endpoints, unit test to confirm (#1437)

* feat: Enforce authorize call on all endpoints
- Make 'request()' exported for running custom requests
* Rbac users endpoints
* 401 -> 403

cli/autostart_test.go

@@ -110,7 +110,7 @@ func TestAutostart(t *testing.T) {
 		clitest.SetupConfig(t, client, root)
 
 		err := cmd.Execute()
-		require.ErrorContains(t, err, "status code 404: no workspace found by name", "unexpected error")
+		require.ErrorContains(t, err, "status code 403: forbidden", "unexpected error")
 	})
 
 	t.Run("Disable_NotFound", func(t *testing.T) {
@@ -128,7 +128,7 @@ func TestAutostart(t *testing.T) {
 		clitest.SetupConfig(t, client, root)
 
 		err := cmd.Execute()
-		require.ErrorContains(t, err, "status code 404: no workspace found by name", "unexpected error")
+		require.ErrorContains(t, err, "status code 403: forbidden", "unexpected error")
 	})
 
 	t.Run("Enable_DefaultSchedule", func(t *testing.T) {

cli/autostop_test.go

@@ -109,7 +109,7 @@ func TestAutostop(t *testing.T) {
 		clitest.SetupConfig(t, client, root)
 
 		err := cmd.Execute()
-		require.ErrorContains(t, err, "status code 404: no workspace found by name", "unexpected error")
+		require.ErrorContains(t, err, "status code 403: forbidden", "unexpected error")
 	})
 
 	t.Run("Disable_NotFound", func(t *testing.T) {
@@ -127,7 +127,7 @@ func TestAutostop(t *testing.T) {
 		clitest.SetupConfig(t, client, root)
 
 		err := cmd.Execute()
-		require.ErrorContains(t, err, "status code 404: no workspace found by name", "unexpected error")
+		require.ErrorContains(t, err, "status code 403: forbidden", "unexpected error")
 	})
 
 	t.Run("Enable_DefaultSchedule", func(t *testing.T) {

coderd/authorize.go

@@ -0,0 +1,43 @@
+package coderd
+
+import (
+	"net/http"
+
+	"golang.org/x/xerrors"
+
+	"cdr.dev/slog"
+
+	"github.com/coder/coder/coderd/httpapi"
+	"github.com/coder/coder/coderd/httpmw"
+	"github.com/coder/coder/coderd/rbac"
+)
+
+func (api *api) Authorize(rw http.ResponseWriter, r *http.Request, action rbac.Action, object rbac.Object) bool {
+	roles := httpmw.UserRoles(r)
+	err := api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, action, object)
+	if err != nil {
+		httpapi.Write(rw, http.StatusForbidden, httpapi.Response{
+			Message: err.Error(),
+		})
+
+		// Log the errors for debugging
+		internalError := new(rbac.UnauthorizedError)
+		logger := api.Logger
+		if xerrors.As(err, internalError) {
+			logger = api.Logger.With(slog.F("internal", internalError.Internal()))
+		}
+		// Log information for debugging. This will be very helpful
+		// in the early days
+		logger.Warn(r.Context(), "unauthorized",
+			slog.F("roles", roles.Roles),
+			slog.F("user_id", roles.ID),
+			slog.F("username", roles.Username),
+			slog.F("route", r.URL.Path),
+			slog.F("action", action),
+			slog.F("object", object),
+		)
+
+		return false
+	}
+	return true
+}

coderd/coderd.go

@@ -50,7 +50,7 @@ type Options struct {
 	SecureAuthCookie     bool
 	SSHKeygenAlgorithm   gitsshkey.Algorithm
 	TURNServer           *turnconn.Server
-	Authorizer           *rbac.RegoAuthorizer
+	Authorizer           rbac.Authorizer
 }
 
 // New constructs the Coder API into an HTTP handler.
@@ -83,10 +83,6 @@ func New(options *Options) (http.Handler, func()) {
 	// TODO: @emyrk we should just move this into 'ExtractAPIKey'.
 	authRolesMiddleware := httpmw.ExtractUserRoles(options.Database)
 
-	authorize := func(f http.HandlerFunc, actions rbac.Action) http.HandlerFunc {
-		return httpmw.Authorize(api.Logger, api.Authorizer, actions)(f).ServeHTTP
-	}
-
 	r := chi.NewRouter()
 
 	r.Use(
@@ -158,10 +154,7 @@ func New(options *Options) (http.Handler, func()) {
 				})
 			})
 			r.Route("/members", func(r chi.Router) {
-				r.Route("/roles", func(r chi.Router) {
-					r.Use(httpmw.WithRBACObject(rbac.ResourceUserRole))
-					r.Get("/", authorize(api.assignableOrgRoles, rbac.ActionRead))
-				})
+				r.Get("/roles", api.assignableOrgRoles)
 				r.Route("/{user}", func(r chi.Router) {
 					r.Use(
 						httpmw.ExtractUserParam(options.Database),
@@ -232,8 +225,7 @@ func New(options *Options) (http.Handler, func()) {
 				r.Get("/", api.users)
 				// These routes query information about site wide roles.
 				r.Route("/roles", func(r chi.Router) {
-					r.Use(httpmw.WithRBACObject(rbac.ResourceUserRole))
-					r.Get("/", authorize(api.assignableSiteRoles, rbac.ActionRead))
+					r.Get("/", api.assignableSiteRoles)
 				})
 				r.Route("/{user}", func(r chi.Router) {
 					r.Use(httpmw.ExtractUserParam(options.Database))
@@ -244,8 +236,7 @@ func New(options *Options) (http.Handler, func()) {
 						r.Put("/active", api.putUserStatus(database.UserStatusActive))
 					})
 					r.Route("/password", func(r chi.Router) {
-						r.Use(httpmw.WithRBACObject(rbac.ResourceUserPasswordRole))
-						r.Put("/", authorize(api.putUserPassword, rbac.ActionUpdate))
+						r.Put("/", api.putUserPassword)
 					})
 					r.Get("/organizations", api.organizationsByUser)
 					r.Post("/organizations", api.postOrganizationsByUser)
@@ -302,6 +293,7 @@ func New(options *Options) (http.Handler, func()) {
 		r.Route("/workspaces/{workspace}", func(r chi.Router) {
 			r.Use(
 				apiKeyMiddleware,
+				authRolesMiddleware,
 				httpmw.ExtractWorkspaceParam(options.Database),
 			)
 			r.Get("/", api.workspace)

coderd/coderd_test.go

@@ -2,14 +2,19 @@ package coderd_test
 
 import (
 	"context"
+	"net/http"
+	"strings"
 	"testing"
 
-	"go.uber.org/goleak"
-
+	"github.com/go-chi/chi/v5"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"go.uber.org/goleak"
+	"golang.org/x/xerrors"
 
 	"github.com/coder/coder/buildinfo"
 	"github.com/coder/coder/coderd/coderdtest"
+	"github.com/coder/coder/coderd/rbac"
 )
 
 func TestMain(m *testing.M) {
@@ -24,3 +29,197 @@ func TestBuildInfo(t *testing.T) {
 	require.Equal(t, buildinfo.ExternalURL(), buildInfo.ExternalURL, "external URL")
 	require.Equal(t, buildinfo.Version(), buildInfo.Version, "version")
 }
+
+// TestAuthorizeAllEndpoints will check `authorize` is called on every endpoint registered.
+func TestAuthorizeAllEndpoints(t *testing.T) {
+	t.Parallel()
+
+	authorizer := &fakeAuthorizer{}
+	srv, client := coderdtest.NewMemoryCoderd(t, &coderdtest.Options{
+		Authorizer: authorizer,
+	})
+	admin := coderdtest.CreateFirstUser(t, client)
+	organization, err := client.Organization(context.Background(), admin.OrganizationID)
+	require.NoError(t, err, "fetch org")
+
+	// Setup some data in the database.
+	coderdtest.NewProvisionerDaemon(t, client)
+	version := coderdtest.CreateTemplateVersion(t, client, admin.OrganizationID, nil)
+	coderdtest.AwaitTemplateVersionJob(t, client, version.ID)
+	template := coderdtest.CreateTemplate(t, client, admin.OrganizationID, version.ID)
+	workspace := coderdtest.CreateWorkspace(t, client, admin.OrganizationID, template.ID)
+
+	// Always fail auth from this point forward
+	authorizer.AlwaysReturn = rbac.ForbiddenWithInternal(xerrors.New("fake implementation"), nil, nil)
+
+	// skipRoutes allows skipping routes from being checked.
+	type routeCheck struct {
+		NoAuthorize  bool
+		AssertObject rbac.Object
+		StatusCode   int
+	}
+	assertRoute := map[string]routeCheck{
+		// These endpoints do not require auth
+		"GET:/api/v2":                   {NoAuthorize: true},
+		"GET:/api/v2/buildinfo":         {NoAuthorize: true},
+		"GET:/api/v2/users/first":       {NoAuthorize: true},
+		"POST:/api/v2/users/first":      {NoAuthorize: true},
+		"POST:/api/v2/users/login":      {NoAuthorize: true},
+		"POST:/api/v2/users/logout":     {NoAuthorize: true},
+		"GET:/api/v2/users/authmethods": {NoAuthorize: true},
+
+		// All workspaceagents endpoints do not use rbac
+		"POST:/api/v2/workspaceagents/aws-instance-identity":      {NoAuthorize: true},
+		"POST:/api/v2/workspaceagents/azure-instance-identity":    {NoAuthorize: true},
+		"POST:/api/v2/workspaceagents/google-instance-identity":   {NoAuthorize: true},
+		"GET:/api/v2/workspaceagents/me/gitsshkey":                {NoAuthorize: true},
+		"GET:/api/v2/workspaceagents/me/iceservers":               {NoAuthorize: true},
+		"GET:/api/v2/workspaceagents/me/listen":                   {NoAuthorize: true},
+		"GET:/api/v2/workspaceagents/me/metadata":                 {NoAuthorize: true},
+		"GET:/api/v2/workspaceagents/me/turn":                     {NoAuthorize: true},
+		"GET:/api/v2/workspaceagents/{workspaceagent}":            {NoAuthorize: true},
+		"GET:/api/v2/workspaceagents/{workspaceagent}/":           {NoAuthorize: true},
+		"GET:/api/v2/workspaceagents/{workspaceagent}/dial":       {NoAuthorize: true},
+		"GET:/api/v2/workspaceagents/{workspaceagent}/iceservers": {NoAuthorize: true},
+		"GET:/api/v2/workspaceagents/{workspaceagent}/pty":        {NoAuthorize: true},
+		"GET:/api/v2/workspaceagents/{workspaceagent}/turn":       {NoAuthorize: true},
+
+		// TODO: @emyrk these need to be fixed by adding authorize calls
+		"GET:/api/v2/workspaceresources/{workspaceresource}":             {NoAuthorize: true},
+		"GET:/api/v2/workspacebuilds/{workspacebuild}":                   {NoAuthorize: true},
+		"GET:/api/v2/workspacebuilds/{workspacebuild}/logs":              {NoAuthorize: true},
+		"GET:/api/v2/workspacebuilds/{workspacebuild}/resources":         {NoAuthorize: true},
+		"GET:/api/v2/workspacebuilds/{workspacebuild}/state":             {NoAuthorize: true},
+		"PATCH:/api/v2/workspacebuilds/{workspacebuild}/cancel":          {NoAuthorize: true},
+		"GET:/api/v2/workspaces/{workspace}/builds/{workspacebuildname}": {NoAuthorize: true},
+
+		"GET:/api/v2/users/oauth2/github/callback": {NoAuthorize: true},
+
+		"POST:/api/v2/users/{user}/organizations/":                          {NoAuthorize: true},
+		"PUT:/api/v2/organizations/{organization}/members/{user}/roles":     {NoAuthorize: true},
+		"GET:/api/v2/organizations/{organization}/provisionerdaemons":       {NoAuthorize: true},
+		"POST:/api/v2/organizations/{organization}/templates":               {NoAuthorize: true},
+		"GET:/api/v2/organizations/{organization}/templates":                {NoAuthorize: true},
+		"GET:/api/v2/organizations/{organization}/templates/{templatename}": {NoAuthorize: true},
+		"POST:/api/v2/organizations/{organization}/templateversions":        {NoAuthorize: true},
+		"POST:/api/v2/organizations/{organization}/workspaces":              {NoAuthorize: true},
+
+		"POST:/api/v2/parameters/{scope}/{id}":          {NoAuthorize: true},
+		"GET:/api/v2/parameters/{scope}/{id}":           {NoAuthorize: true},
+		"DELETE:/api/v2/parameters/{scope}/{id}/{name}": {NoAuthorize: true},
+
+		"GET:/api/v2/provisionerdaemons/me/listen": {NoAuthorize: true},
+
+		"DELETE:/api/v2/templates/{template}":                             {NoAuthorize: true},
+		"GET:/api/v2/templates/{template}":                                {NoAuthorize: true},
+		"GET:/api/v2/templates/{template}/versions":                       {NoAuthorize: true},
+		"PATCH:/api/v2/templates/{template}/versions":                     {NoAuthorize: true},
+		"GET:/api/v2/templates/{template}/versions/{templateversionname}": {NoAuthorize: true},
+
+		"GET:/api/v2/templateversions/{templateversion}":            {NoAuthorize: true},
+		"PATCH:/api/v2/templateversions/{templateversion}/cancel":   {NoAuthorize: true},
+		"GET:/api/v2/templateversions/{templateversion}/logs":       {NoAuthorize: true},
+		"GET:/api/v2/templateversions/{templateversion}/parameters": {NoAuthorize: true},
+		"GET:/api/v2/templateversions/{templateversion}/resources":  {NoAuthorize: true},
+		"GET:/api/v2/templateversions/{templateversion}/schema":     {NoAuthorize: true},
+
+		"POST:/api/v2/users/{user}/organizations": {NoAuthorize: true},
+
+		"GET:/api/v2/workspaces/{workspace}":           {NoAuthorize: true},
+		"PUT:/api/v2/workspaces/{workspace}/autostart": {NoAuthorize: true},
+		"PUT:/api/v2/workspaces/{workspace}/autostop":  {NoAuthorize: true},
+		"GET:/api/v2/workspaces/{workspace}/builds":    {NoAuthorize: true},
+		"POST:/api/v2/workspaces/{workspace}/builds":   {NoAuthorize: true},
+
+		"POST:/api/v2/files":       {NoAuthorize: true},
+		"GET:/api/v2/files/{hash}": {NoAuthorize: true},
+
+		// These endpoints have more assertions. This is good, add more endpoints to assert if you can!
+		"GET:/api/v2/organizations/{organization}":                   {AssertObject: rbac.ResourceOrganization.InOrg(admin.OrganizationID)},
+		"GET:/api/v2/users/{user}/organizations":                     {StatusCode: http.StatusOK, AssertObject: rbac.ResourceOrganization},
+		"GET:/api/v2/users/{user}/workspaces":                        {StatusCode: http.StatusOK, AssertObject: rbac.ResourceWorkspace},
+		"GET:/api/v2/organizations/{organization}/workspaces/{user}": {StatusCode: http.StatusOK, AssertObject: rbac.ResourceWorkspace},
+		"GET:/api/v2/organizations/{organization}/workspaces/{user}/{workspace}": {
+			AssertObject: rbac.ResourceWorkspace.InOrg(organization.ID).WithID(workspace.ID.String()).WithOwner(workspace.OwnerID.String()),
+		},
+		"GET:/api/v2/organizations/{organization}/workspaces": {StatusCode: http.StatusOK, AssertObject: rbac.ResourceWorkspace},
+
+		// These endpoints need payloads to get to the auth part.
+		"PUT:/api/v2/users/{user}/roles": {StatusCode: http.StatusBadRequest, NoAuthorize: true},
+	}
+
+	c, _ := srv.Config.Handler.(*chi.Mux)
+	err = chi.Walk(c, func(method string, route string, handler http.Handler, middlewares ...func(http.Handler) http.Handler) error {
+		name := method + ":" + route
+		t.Run(name, func(t *testing.T) {
+			authorizer.reset()
+			routeAssertions, ok := assertRoute[strings.TrimRight(name, "/")]
+			if !ok {
+				// By default, all omitted routes check for just "authorize" called
+				routeAssertions = routeCheck{}
+			}
+			if routeAssertions.StatusCode == 0 {
+				routeAssertions.StatusCode = http.StatusForbidden
+			}
+
+			// Replace all url params with known values
+			route = strings.ReplaceAll(route, "{organization}", admin.OrganizationID.String())
+			route = strings.ReplaceAll(route, "{user}", admin.UserID.String())
+			route = strings.ReplaceAll(route, "{organizationname}", organization.Name)
+			route = strings.ReplaceAll(route, "{workspace}", workspace.Name)
+
+			resp, err := client.Request(context.Background(), method, route, nil)
+			require.NoError(t, err, "do req")
+			_ = resp.Body.Close()
+
+			if !routeAssertions.NoAuthorize {
+				assert.NotNil(t, authorizer.Called, "authorizer expected")
+				assert.Equal(t, routeAssertions.StatusCode, resp.StatusCode, "expect unauthorized")
+				if authorizer.Called != nil {
+					if routeAssertions.AssertObject.Type != "" {
+						assert.Equal(t, routeAssertions.AssertObject.Type, authorizer.Called.Object.Type, "resource type")
+					}
+					if routeAssertions.AssertObject.Owner != "" {
+						assert.Equal(t, routeAssertions.AssertObject.Owner, authorizer.Called.Object.Owner, "resource owner")
+					}
+					if routeAssertions.AssertObject.OrgID != "" {
+						assert.Equal(t, routeAssertions.AssertObject.OrgID, authorizer.Called.Object.OrgID, "resource org")
+					}
+					if routeAssertions.AssertObject.ResourceID != "" {
+						assert.Equal(t, routeAssertions.AssertObject.ResourceID, authorizer.Called.Object.ResourceID, "resource ID")
+					}
+				}
+			} else {
+				assert.Nil(t, authorizer.Called, "authorize not expected")
+			}
+		})
+		return nil
+	})
+	require.NoError(t, err)
+}
+
+type authCall struct {
+	SubjectID string
+	Roles     []string
+	Action    rbac.Action
+	Object    rbac.Object
+}
+
+type fakeAuthorizer struct {
+	Called       *authCall
+	AlwaysReturn error
+}
+
+func (f *fakeAuthorizer) ByRoleName(_ context.Context, subjectID string, roleNames []string, action rbac.Action, object rbac.Object) error {
+	f.Called = &authCall{
+		SubjectID: subjectID,
+		Roles:     roleNames,
+		Action:    action,
+		Object:    object,
+	}
+	return f.AlwaysReturn
+}
+
+func (f *fakeAuthorizer) reset() {
+	f.Called = nil
+}