Add more endpoints to the unit test

coder · Emyrk · May 17, 2022 · May 11, 2022 · May 12, 2022 · May 12, 2022
commit af6dc5fce6104a8ff9ffb82751fbbb29ea3c30a0
diff --git a/coderd/coderd.go b/coderd/coderd.go
@@ -83,8 +83,8 @@ 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
+	authorize := func(f http.HandlerFunc, actions ...rbac.Action) http.HandlerFunc {
+		return httpmw.Authorize(api.Logger, api.Authorizer, actions...)(f).ServeHTTP
 	}
 
 	r := chi.NewRouter()
@@ -131,18 +131,21 @@ func New(options *Options) (http.Handler, func()) {
 				// This number is arbitrary, but reading/writing
 				// file content is expensive so it should be small.
 				httpmw.RateLimitPerMinute(12),
+				// TODO: @emyrk (rbac) Currently files are owned by the site?
+				//	Should files be org scoped? User scoped?
 				httpmw.WithRBACObject(rbac.ResourceTypeFile),
 			)
-			r.Get("/{hash}", api.fileByHash)
-			r.Post("/", api.postFile)
+			r.Get("/{hash}", authorize(api.fileByHash, rbac.ActionRead))
+			r.Post("/", authorize(api.postFile, rbac.ActionCreate, rbac.ActionUpdate))
 		})
 		r.Route("/organizations/{organization}", func(r chi.Router) {
 			r.Use(
 				apiKeyMiddleware,
 				authRolesMiddleware,
 				httpmw.ExtractOrganizationParam(options.Database),
 			)
-			r.Get("/", api.organization)
+			r.With(httpmw.WithRBACObject(rbac.ResourceOrganization)).
+				Get("/", authorize(api.organization, rbac.ActionRead))
 			r.Get("/provisionerdaemons", api.provisionerDaemonsByOrganization)
 			r.Post("/templateversions", api.postTemplateVersionsByOrganization)
 			r.Route("/templates", func(r chi.Router) {

diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go
@@ -2,7 +2,9 @@ package coderd_test
 
 import (
 	"context"
+	"fmt"
 	"net/http"
+	"strings"
 	"testing"
 
 	"github.com/go-chi/chi/v5"
@@ -32,39 +34,65 @@ func TestBuildInfo(t *testing.T) {
 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
 
+	// skipRoutes allows skipping routes from being checked.
+	type routeCheck struct {
+		NoAuthorize  bool
+		AssertObject rbac.Object
+	}
+	assertRoute := map[string]routeCheck{
+		"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},
+
+		// TODO: @emyrk these need to be fixed by adding authorize calls
+		"/api/v2/organizations/{organization}/provisionerdaemons": {NoAuthorize: true},
+		"GET:/api/v2/organizations/{organization}":                {AssertObject: rbac.ResourceOrganization.InOrg(admin.OrganizationID)},
+	}
+
 	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]
+			routeAssertions, ok := assertRoute[strings.TrimRight(name, "/")]
 			if !ok {
 				// By default, all omitted routes check for just "authorize" called
 				routeAssertions = routeCheck{}
 			}
 
+			// Replace all url params with expected
+			route = strings.ReplaceAll(route, "{organization}", admin.OrganizationID.String())
+
 			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, http.StatusUnauthorized, resp.StatusCode, "expect unauthorized")
+				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")
 			}
@@ -92,7 +120,7 @@ func (f *fakeAuthorizer) AuthorizeByRoleName(ctx context.Context, subjectID stri
 		Action:    action,
 		Object:    object,
 	}
-	return nil
+	return rbac.ForbiddenWithInternal(fmt.Errorf("fake implementation"), nil, nil)
 }
 
 func (f *fakeAuthorizer) reset() {

diff --git a/coderd/httpmw/authorize.go b/coderd/httpmw/authorize.go
@@ -17,7 +17,7 @@ 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.Authorizer, action rbac.Action) func(http.Handler) http.Handler {
+func Authorize(logger slog.Logger, auth rbac.Authorizer, actions ...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)
@@ -51,26 +51,28 @@ func Authorize(logger slog.Logger, auth rbac.Authorizer, action rbac.Action) fun
 				}
 			}
 
-			err := auth.AuthorizeByRoleName(r.Context(), roles.ID.String(), roles.Roles, action, object)
-			if err != nil {
-				internalError := new(rbac.UnauthorizedError)
-				if xerrors.As(err, internalError) {
-					logger = logger.With(slog.F("internal", internalError.Internal()))
+			for _, action := range actions {
+				err := auth.AuthorizeByRoleName(r.Context(), roles.ID.String(), roles.Roles, action, object)
+				if err != nil {
+					internalError := new(rbac.UnauthorizedError)
+					if xerrors.As(err, internalError) {
+						logger = logger.With(slog.F("internal", internalError.Internal()))
+					}
+					// Log information for debugging. This will be very helpful
+					// in the early days if we over secure endpoints.
+					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),
+					)
+					httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
+						Message: err.Error(),
+					})
+					return
 				}
-				// Log information for debugging. This will be very helpful
-				// in the early days if we over secure endpoints.
-				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),
-				)
-				httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
-					Message: err.Error(),
-				})
-				return
 			}
 			next.ServeHTTP(rw, r)
 		})