feat: Enforce authorize call on all endpoints

- Make 'request()' exported for running custom requests
coder · Emyrk · May 17, 2022 · May 11, 2022 · May 12, 2022 · May 12, 2022
commit bed0f8f120ea094776f4a4651ea5e5bb30f86e62
diff --git a/coderd/coderd.go b/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.
@@ -127,18 +127,20 @@ func New(options *Options) (http.Handler, func()) {
 		r.Route("/files", func(r chi.Router) {
 			r.Use(
 				apiKeyMiddleware,
+				authRolesMiddleware,
 				// This number is arbitrary, but reading/writing
 				// file content is expensive so it should be small.
 				httpmw.RateLimitPerMinute(12),
+				httpmw.WithRBACObject(rbac.ResourceTypeFile),
 			)
 			r.Get("/{hash}", api.fileByHash)
 			r.Post("/", api.postFile)
 		})
 		r.Route("/organizations/{organization}", func(r chi.Router) {
 			r.Use(
 				apiKeyMiddleware,
-				httpmw.ExtractOrganizationParam(options.Database),
 				authRolesMiddleware,
+				httpmw.ExtractOrganizationParam(options.Database),
 			)
 			r.Get("/", api.organization)
 			r.Get("/provisionerdaemons", api.provisionerDaemonsByOrganization)
@@ -149,12 +151,16 @@ func New(options *Options) (http.Handler, func()) {
 				r.Get("/{templatename}", api.templateByOrganizationAndName)
 			})
 			r.Route("/workspaces", func(r chi.Router) {
-				r.Post("/", api.postWorkspacesByOrganization)
-				r.Get("/", api.workspacesByOrganization)
+				r.Use(httpmw.WithRBACObject(rbac.ResourceWorkspace))
+				// Posting a workspace is inherently owned by the api key creating it.
+				r.With(httpmw.WithAPIKeyAsOwner()).
+					Post("/", authorize(api.postWorkspacesByOrganization, rbac.ActionCreate))
+				r.Get("/", authorize(api.workspacesByOrganization, rbac.ActionRead))
 				r.Route("/{user}", func(r chi.Router) {
 					r.Use(httpmw.ExtractUserParam(options.Database))
-					r.Get("/{workspace}", api.workspaceByOwnerAndName)
-					r.Get("/", api.workspacesByOwner)
+					// TODO: @emyrk add the resource id to this authorize.
+					r.Get("/{workspace}", authorize(api.workspaceByOwnerAndName, rbac.ActionRead))
+					r.Get("/", authorize(api.workspacesByOwner, rbac.ActionRead))
 				})
 			})
 			r.Route("/members", func(r chi.Router) {

diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go
@@ -2,14 +2,17 @@ package coderd_test
 
 import (
 	"context"
+	"net/http"
 	"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"
 
 	"github.com/coder/coder/buildinfo"
 	"github.com/coder/coder/coderd/coderdtest"
+	"github.com/coder/coder/coderd/rbac"
 )
 
 func TestMain(m *testing.M) {
@@ -24,3 +27,74 @@ 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()
+
+	// skipRoutes allows skipping routes from being checked.
+	type routeCheck struct {
+		NoAuthorize bool
+	}
+	assertRoute := map[string]routeCheck{
+		"GET:/api/v2":           {NoAuthorize: true},
+		"GET:/api/v2/buildinfo": {NoAuthorize: true},
+	}
+
+	authorizer := &fakeAuthorizer{}
+	srv, client := coderdtest.NewMemoryCoderd(t, &coderdtest.Options{
+		Authorizer: authorizer,
+	})
+	admin := coderdtest.CreateFirstUser(t, client)
+	var _ = admin
+
+	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[name]
+			if !ok {
+				// By default, all omitted routes check for just "authorize" called
+				routeAssertions = routeCheck{}
+			}
+
+			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")
+			} 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
+}
+
+func (f *fakeAuthorizer) AuthorizeByRoleName(ctx 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 nil
+}
+
+func (f *fakeAuthorizer) reset() {
+	f.Called = nil
+}
diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go
@@ -24,6 +24,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/coder/coder/coderd/rbac"
+
 	"cloud.google.com/go/compute/metadata"
 	"github.com/fullsailor/pkcs7"
 	"github.com/golang-jwt/jwt"
@@ -57,11 +59,10 @@ type Options struct {
 	GoogleTokenValidator *idtoken.Validator
 	SSHKeygenAlgorithm   gitsshkey.Algorithm
 	APIRateLimit         int
+	Authorizer           rbac.Authorizer
 }
 
-// New constructs an in-memory coderd instance and returns
-// the connected client.
-func New(t *testing.T, options *Options) *codersdk.Client {
+func NewMemoryCoderd(t *testing.T, options *Options) (*httptest.Server, *codersdk.Client) {
 	if options == nil {
 		options = &Options{}
 	}
@@ -129,6 +130,7 @@ func New(t *testing.T, options *Options) *codersdk.Client {
 		SSHKeygenAlgorithm:   options.SSHKeygenAlgorithm,
 		TURNServer:           turnServer,
 		APIRateLimit:         options.APIRateLimit,
+		Authorizer:           options.Authorizer,
 	})
 	t.Cleanup(func() {
 		cancelFunc()
@@ -137,7 +139,14 @@ func New(t *testing.T, options *Options) *codersdk.Client {
 		closeWait()
 	})
 
-	return codersdk.New(serverURL)
+	return srv, codersdk.New(serverURL)
+}
+
+// New constructs an in-memory coderd instance and returns
+// the connected client.
+func New(t *testing.T, options *Options) *codersdk.Client {
+	_, cli := NewMemoryCoderd(t, options)
+	return cli
 }
 
 // NewProvisionerDaemon launches a provisionerd instance configured to work

diff --git a/coderd/httpmw/authorize.go b/coderd/httpmw/authorize.go
@@ -4,6 +4,8 @@ import (
 	"context"
 	"net/http"
 
+	"github.com/google/uuid"
+
 	"golang.org/x/xerrors"
 
 	"cdr.dev/slog"
@@ -15,11 +17,12 @@ import (
 // Authorize will enforce if the user roles can complete the action on the AuthObject.
 // The organization and owner are found using the ExtractOrganization and
 // ExtractUser middleware if present.
-func Authorize(logger slog.Logger, auth *rbac.RegoAuthorizer, action rbac.Action) func(http.Handler) http.Handler {
+func Authorize(logger slog.Logger, auth rbac.Authorizer, action rbac.Action) func(http.Handler) http.Handler {
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
 			roles := UserRoles(r)
-			object := rbacObject(r)
+			authObject := rbacObject(r)
+			object := authObject.Object
 
 			if object.Type == "" {
 				panic("developer error: auth object has no type")
@@ -34,12 +37,18 @@ func Authorize(logger slog.Logger, auth *rbac.RegoAuthorizer, action rbac.Action
 				object = object.InOrg(organization.ID)
 			}
 
-			unknownOwner := r.Context().Value(userParamContextKey{})
-			if owner, castOK := unknownOwner.(database.User); unknownOwner != nil {
-				if !castOK {
-					panic("developer error: user param middleware not provided for authorize")
+			if authObject.WithOwner != nil {
+				owner := authObject.WithOwner(r)
+				object = object.WithOwner(owner.String())
+			} else {
+				// Attempt to find the resource owner id
+				unknownOwner := r.Context().Value(userParamContextKey{})
+				if owner, castOK := unknownOwner.(database.User); unknownOwner != nil {
+					if !castOK {
+						panic("developer error: user param middleware not provided for authorize")
+					}
+					object = object.WithOwner(owner.ID.String())
 				}
-				object = object.WithOwner(owner.ID.String())
 			}
 
 			err := auth.AuthorizeByRoleName(r.Context(), roles.ID.String(), roles.Roles, action, object)
@@ -70,21 +79,59 @@ func Authorize(logger slog.Logger, auth *rbac.RegoAuthorizer, action rbac.Action
 
 type authObjectKey struct{}
 
+type AuthObject struct {
+	Object rbac.Object
+
+	WithOwner func(r *http.Request) uuid.UUID
+}
+
 // APIKey returns the API key from the ExtractAPIKey handler.
-func rbacObject(r *http.Request) rbac.Object {
-	obj, ok := r.Context().Value(authObjectKey{}).(rbac.Object)
+func rbacObject(r *http.Request) AuthObject {
+	obj, ok := r.Context().Value(authObjectKey{}).(AuthObject)
 	if !ok {
 		panic("developer error: auth object middleware not provided")
 	}
 	return obj
 }
 
+func WithAPIKeyAsOwner() func(http.Handler) http.Handler {
+	return WithOwner(func(r *http.Request) uuid.UUID {
+		key := APIKey(r)
+		return key.UserID
+	})
+}
+
+// WithOwner sets the object owner for 'Authorize()' for all routes handled
+// by this middleware.
+func WithOwner(withOwner func(r *http.Request) uuid.UUID) func(http.Handler) http.Handler {
+	return func(next http.Handler) http.Handler {
+		return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
+			obj, ok := r.Context().Value(authObjectKey{}).(AuthObject)
+			if ok {
+				obj.WithOwner = withOwner
+			} else {
+				obj = AuthObject{WithOwner: withOwner}
+			}
+
+			ctx := context.WithValue(r.Context(), authObjectKey{}, obj)
+			next.ServeHTTP(rw, r.WithContext(ctx))
+		})
+	}
+}
+
 // WithRBACObject sets the object for 'Authorize()' for all routes handled
 // by this middleware. The important field to set is 'Type'
 func WithRBACObject(object rbac.Object) func(http.Handler) http.Handler {
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
-			ctx := context.WithValue(r.Context(), authObjectKey{}, object)
+			obj, ok := r.Context().Value(authObjectKey{}).(AuthObject)
+			if ok {
+				obj.Object = object
+			} else {
+				obj = AuthObject{Object: object}
+			}
+
+			ctx := context.WithValue(r.Context(), authObjectKey{}, obj)
 			next.ServeHTTP(rw, r.WithContext(ctx))
 		})
 	}

diff --git a/coderd/httpmw/oauth2.go b/coderd/httpmw/oauth2.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"net/http"
+	"reflect"
 
 	"golang.org/x/oauth2"
 
@@ -46,7 +47,8 @@ func OAuth2(r *http.Request) OAuth2State {
 func ExtractOAuth2(config OAuth2Config) func(http.Handler) http.Handler {
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
-			if config == nil {
+			// Interfaces can hold a nil value
+			if config == nil || reflect.ValueOf(config).IsNil() {
 				httpapi.Write(rw, http.StatusPreconditionRequired, httpapi.Response{
 					Message: "The oauth2 method requested is not configured!",
 				})

diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go
@@ -9,6 +9,10 @@ import (
 	"github.com/open-policy-agent/opa/rego"
 )
 
+type Authorizer interface {
+	AuthorizeByRoleName(ctx context.Context, subjectID string, roleNames []string, action Action, object Object) error
+}
+
 // RegoAuthorizer will use a prepared rego query for performing authorize()
 type RegoAuthorizer struct {
 	query rego.PreparedEvalQuery

diff --git a/coderd/rbac/object.go b/coderd/rbac/object.go
@@ -9,6 +9,10 @@ const WildcardSymbol = "*"
 // Resources are just typed objects. Making resources this way allows directly
 // passing them into an Authorize function and use the chaining api.
 var (
+	// ResourceWorkspace CRUD. Org + User owner
+	//	create/delete = make or delete workspaces
+	// 	read = access workspace
+	//	update = edit workspace variables
 	ResourceWorkspace = Object{
 		Type: "workspace",
 	}
@@ -17,6 +21,18 @@ var (
 		Type: "template",
 	}
 
+	ResourceTypeFile = Object{
+		Type: "file",
+	}
+
+	// ResourceOrganization CRUD. Org owner
+	//	create/delete = make or delete organizations
+	// 	read = view org information
+	//	update = ??
+	ResourceOrganization = Object{
+		Type: "organization",
+	}
+
 	// ResourceUserRole might be expanded later to allow more granular permissions
 	// to modifying roles. For now, this covers all possible roles, so having this permission
 	// allows granting/deleting **ALL** roles.

diff --git a/codersdk/buildinfo.go b/codersdk/buildinfo.go
@@ -18,7 +18,7 @@ type BuildInfoResponse struct {
 
 // BuildInfo returns build information for this instance of Coder.
 func (c *Client) BuildInfo(ctx context.Context) (BuildInfoResponse, error) {
-	res, err := c.request(ctx, http.MethodGet, "/api/v2/buildinfo", nil)
+	res, err := c.Request(ctx, http.MethodGet, "/api/v2/buildinfo", nil)
 	if err != nil {
 		return BuildInfoResponse{}, err
 	}

diff --git a/codersdk/client.go b/codersdk/client.go
@@ -35,9 +35,9 @@ type Client struct {
 
 type requestOption func(*http.Request)
 
-// request performs an HTTP request with the body provided.
+// Request performs an HTTP request with the body provided.
 // The caller is responsible for closing the response body.
-func (c *Client) request(ctx context.Context, method, path string, body interface{}, opts ...requestOption) (*http.Response, error) {
+func (c *Client) Request(ctx context.Context, method, path string, body interface{}, opts ...requestOption) (*http.Response, error) {
 	serverURL, err := c.URL.Parse(path)
 	if err != nil {
 		return nil, xerrors.Errorf("parse url: %w", err)