Fix API token support for create/update/list worker routes (cloudflare#369)

Previously, creating or updating a worker route would default to the deprecated `filters` endpoints when Enabled == false and ScriptName == "". This would cause errors when using API tokens since the filter endpoints don't support API tokens. This change will use the `routes` endpoints in that scenario instead, which do support API tokens.

It also updates `ListWorkerRoutes` to use the `routes` endpoint instead of `filters` when an API token is supplied.

Deleting worker routes suffers from the same problem, but that will be fixed in cloudflare#367

Author: jRiest

workers.go

@@ -30,13 +30,13 @@ type WorkerScriptParams struct {
 	Bindings map[string]WorkerBinding
 }
 
-// WorkerRoute aka filters are patterns used to enable or disable workers that match requests.
+// WorkerRoute is used to map traffic matching a URL pattern to a workers
 //
-// API reference: https://api.cloudflare.com/#worker-filters-properties
+// API reference: https://api.cloudflare.com/#worker-routes-properties
 type WorkerRoute struct {
 	ID      string `json:"id,omitempty"`
 	Pattern string `json:"pattern"`
-	Enabled bool   `json:"enabled"`
+	Enabled bool   `json:"enabled"` // this is deprecated: https://api.cloudflare.com/#worker-filters-deprecated--properties
 	Script  string `json:"script,omitempty"`
 }
 
@@ -566,14 +566,9 @@ func formatMultipartBody(params *WorkerScriptParams) (string, []byte, error) {
 //
 // API reference: https://api.cloudflare.com/#worker-filters-create-filter, https://api.cloudflare.com/#worker-routes-create-route
 func (api *API) CreateWorkerRoute(zoneID string, route WorkerRoute) (WorkerRouteResponse, error) {
-	// Check whether a script name is defined in order to determine whether
-	// to use the single-script or multi-script endpoint.
-	pathComponent := "filters"
-	if route.Script != "" {
-		if api.AccountID == "" {
-			return WorkerRouteResponse{}, errors.New("account ID required for enterprise only request")
-		}
-		pathComponent = "routes"
+	pathComponent, err := getRouteEndpoint(api, route)
+	if err != nil {
+		return WorkerRouteResponse{}, err
 	}
 
 	uri := "/zones/" + zoneID + "/workers/" + pathComponent
@@ -611,7 +606,16 @@ func (api *API) DeleteWorkerRoute(zoneID string, routeID string) (WorkerRouteRes
 // API reference: https://api.cloudflare.com/#worker-filters-list-filters, https://api.cloudflare.com/#worker-routes-list-routes
 func (api *API) ListWorkerRoutes(zoneID string) (WorkerRoutesResponse, error) {
 	pathComponent := "filters"
-	if api.AccountID != "" {
+	// Unfortunately we don't have a good signal of whether the user is wanting
+	// to use the deprecated filters endpoint (https://api.cloudflare.com/#worker-filters-list-filters)
+	// or the multi-script routes endpoint (https://api.cloudflare.com/#worker-script-list-workers)
+	//
+	// The filters endpoint does not support API tokens, so if an API token is specified we need to use
+	// the routes endpoint. Otherwise, since the multi-script API endpoints that operate on a script
+	// require an AccountID, we assume that anyone specifying an AccountID is using the routes endpoint.
+	// This is likely too presumptuous. In the next major version, we should just remove the deprecated
+	// filter endpoints entirely to avoid this ambiguity.
+	if api.AccountID != "" || api.APIToken != "" {
 		pathComponent = "routes"
 	}
 	uri := "/zones/" + zoneID + "/workers/" + pathComponent
@@ -640,14 +644,9 @@ func (api *API) ListWorkerRoutes(zoneID string) (WorkerRoutesResponse, error) {
 //
 // API reference: https://api.cloudflare.com/#worker-filters-update-filter, https://api.cloudflare.com/#worker-routes-update-route
 func (api *API) UpdateWorkerRoute(zoneID string, routeID string, route WorkerRoute) (WorkerRouteResponse, error) {
-	// Check whether a script name is defined in order to determine whether
-	// to use the single-script or multi-script endpoint.
-	pathComponent := "filters"
-	if route.Script != "" {
-		if api.AccountID == "" {
-			return WorkerRouteResponse{}, errors.New("account ID required for enterprise only request")
-		}
-		pathComponent = "routes"
+	pathComponent, err := getRouteEndpoint(api, route)
+	if err != nil {
+		return WorkerRouteResponse{}, err
 	}
 	uri := "/zones/" + zoneID + "/workers/" + pathComponent + "/" + routeID
 	res, err := api.makeRequest("PUT", uri, route)
@@ -661,3 +660,18 @@ func (api *API) UpdateWorkerRoute(zoneID string, routeID string, route WorkerRou
 	}
 	return r, nil
 }
+
+func getRouteEndpoint(api *API, route WorkerRoute) (string, error) {
+	if route.Script != "" && route.Enabled == true {
+		return "", errors.New("Only `Script` or `Enabled` may be specified for a WorkerRoute, not both")
+	}
+
+	// For backwards-compatability, fallback to the deprecated filter
+	// endpoint if Enabled == true
+	// https://api.cloudflare.com/#worker-filters-deprecated--properties
+	if route.Enabled == true {
+		return "filters", nil
+	}
+
+	return "routes", nil
+}

workers_test.go

@@ -628,6 +628,29 @@ func TestWorkers_CreateWorkerRouteSingleScriptWithAccount(t *testing.T) {
 	}
 }
 
+func TestWorkers_CreateWorkerRouteErrorsWhenMixingSingleAndMultiScriptProperties(t *testing.T) {
+	setup(UsingAccount("foo"))
+	defer teardown()
+
+	route := WorkerRoute{Pattern: "app1.example.com/*", Script: "test_script", Enabled: true}
+	_, err := client.CreateWorkerRoute("foo", route)
+	assert.EqualError(t, err, "Only `Script` or `Enabled` may be specified for a WorkerRoute, not both")
+}
+
+func TestWorkers_CreateWorkerRouteWithNoScript(t *testing.T) {
+	setup(UsingAccount("foo"))
+
+	mux.HandleFunc("/zones/foo/workers/routes", func(w http.ResponseWriter, r *http.Request) {
+		assert.Equal(t, "POST", r.Method, "Expected method 'POST', got %s", r.Method)
+		w.Header().Set("content-type", "application-json")
+		fmt.Fprintf(w, createWorkerRouteResponse)
+	})
+
+	route := WorkerRoute{Pattern: "app1.example.com/*"}
+	_, err := client.CreateWorkerRoute("foo", route)
+	assert.NoError(t, err)
+}
+
 func TestWorkers_DeleteWorkerRoute(t *testing.T) {
 	setup()
 	defer teardown()
@@ -822,3 +845,26 @@ func TestWorkers_ListWorkerBindingsMultiScript(t *testing.T) {
 	})
 	assert.Equal(t, WorkerInheritBindingType, res.BindingList[2].Binding.Type())
 }
+
+func TestWorkers_UpdateWorkerRouteErrorsWhenMixingSingleAndMultiScriptProperties(t *testing.T) {
+	setup(UsingAccount("foo"))
+	defer teardown()
+
+	route := WorkerRoute{Pattern: "app1.example.com/*", Script: "test_script", Enabled: true}
+	_, err := client.UpdateWorkerRoute("foo", "e7a57d8746e74ae49c25994dadb421b1", route)
+	assert.EqualError(t, err, "Only `Script` or `Enabled` may be specified for a WorkerRoute, not both")
+}
+
+func TestWorkers_UpdateWorkerRouteWithNoScript(t *testing.T) {
+	setup(UsingAccount("foo"))
+
+	mux.HandleFunc("/zones/foo/workers/routes/e7a57d8746e74ae49c25994dadb421b1", func(w http.ResponseWriter, r *http.Request) {
+		assert.Equal(t, "PUT", r.Method, "Expected method 'PUT', got %s", r.Method)
+		w.Header().Set("content-type", "application-json")
+		fmt.Fprintf(w, updateWorkerRouteEntResponse)
+	})
+
+	route := WorkerRoute{Pattern: "app1.example.com/*"}
+	_, err := client.UpdateWorkerRoute("foo", "e7a57d8746e74ae49c25994dadb421b1", route)
+	assert.NoError(t, err)
+}