From 5a4e602373c02d9b80468fd14d6ec14ccbdd7b5f Mon Sep 17 00:00:00 2001
From: Will Norris <will@willnorris.com>
Date: Fri, 21 Nov 2014 07:51:19 -0800
Subject: [PATCH] small cleanup of check304 and add more tests

Remove unused ResponseWriter parameter from check304, add function docs,
and add TODO for alternate Etag header values that we should handle.

Add tests for Proxy.allowed and check304.
---
 imageproxy.go      |  12 +++--
 imageproxy_test.go | 114 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 123 insertions(+), 3 deletions(-)
 create mode 100644 imageproxy_test.go

diff --git a/imageproxy.go b/imageproxy.go
index 55df86a..3431aef 100644
--- a/imageproxy.go
+++ b/imageproxy.go
@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-// Package proxy provides the image proxy.
+// Package imageproxy provides an image proxy server.
 package imageproxy // import "willnorris.com/go/imageproxy"
 
 import (
@@ -97,7 +97,7 @@ func (p *Proxy) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	w.Header().Add("Expires", resp.Header.Get("Expires"))
 	w.Header().Add("Etag", resp.Header.Get("Etag"))
 
-	if is304 := check304(w, r, resp); is304 {
+	if is304 := check304(r, resp); is304 {
 		w.WriteHeader(http.StatusNotModified)
 		return
 	}
@@ -125,7 +125,13 @@ func (p *Proxy) allowed(u *url.URL) bool {
 	return false
 }
 
-func check304(w http.ResponseWriter, req *http.Request, resp *http.Response) bool {
+// check304 checks whether we should send a 304 Not Modified in response to
+// req, based on the response resp.  This is determined using the last modified
+// time and the entity tag of resp.
+func check304(req *http.Request, resp *http.Response) bool {
+	// TODO(willnorris): if-none-match header can be a comma separated list
+	// of multiple tags to be matched, or the special value "*" which
+	// matches all etags
 	etag := resp.Header.Get("Etag")
 	if etag != "" && etag == req.Header.Get("If-None-Match") {
 		return true
diff --git a/imageproxy_test.go b/imageproxy_test.go
new file mode 100644
index 0000000..5965c4e
--- /dev/null
+++ b/imageproxy_test.go
@@ -0,0 +1,114 @@
+package imageproxy
+
+import (
+	"bufio"
+	"net/http"
+	"net/url"
+	"strings"
+	"testing"
+)
+
+func TestAllowed(t *testing.T) {
+	p := &Proxy{
+		Whitelist: []string{"a.test", "*.b.test", "*c.test"},
+	}
+
+	tests := []struct {
+		url     string
+		allowed bool
+	}{
+		{"http://a.test/image", true},
+		{"http://x.a.test/image", false},
+
+		{"http://b.test/image", true},
+		{"http://x.b.test/image", true},
+		{"http://x.y.b.test/image", true},
+
+		{"http://c.test/image", false},
+		{"http://xc.test/image", false},
+		{"/image", false},
+	}
+
+	for _, tt := range tests {
+		u, err := url.Parse(tt.url)
+		if err != nil {
+			t.Errorf("error parsing url %q: %v", tt.url, err)
+		}
+		if got, want := p.allowed(u), tt.allowed; got != want {
+			t.Errorf("allowed(%q) returned %v, want %v", u, got, want)
+		}
+	}
+}
+
+func TestCheck304(t *testing.T) {
+	tests := []struct {
+		req, resp string
+		is304     bool
+	}{
+		{ // etag match
+			"GET / HTTP/1.1\nIf-None-Match: \"v\"\n\n",
+			"HTTP/1.1 200 OK\nEtag: \"v\"\n\n",
+			true,
+		},
+		{ // last-modified match
+			"GET / HTTP/1.1\nIf-Modified-Since: Sun, 02 Jan 2000 00:00:00 GMT\n\n",
+			"HTTP/1.1 200 OK\nLast-Modified: Sat, 01 Jan 2000 00:00:00 GMT\n\n",
+			true,
+		},
+
+		// mismatches
+		{
+			"GET / HTTP/1.1\n\n",
+			"HTTP/1.1 200 OK\n\n",
+			false,
+		},
+		{
+			"GET / HTTP/1.1\n\n",
+			"HTTP/1.1 200 OK\nEtag: \"v\"\n\n",
+			false,
+		},
+		{
+			"GET / HTTP/1.1\nIf-None-Match: \"v\"\n\n",
+			"HTTP/1.1 200 OK\n\n",
+			false,
+		},
+		{
+			"GET / HTTP/1.1\nIf-None-Match: \"a\"\n\n",
+			"HTTP/1.1 200 OK\nEtag: \"b\"\n\n",
+			false,
+		},
+		{ // last-modified match
+			"GET / HTTP/1.1\n\n",
+			"HTTP/1.1 200 OK\nLast-Modified: Sat, 01 Jan 2000 00:00:00 GMT\n\n",
+			false,
+		},
+		{ // last-modified match
+			"GET / HTTP/1.1\nIf-Modified-Since: Sun, 02 Jan 2000 00:00:00 GMT\n\n",
+			"HTTP/1.1 200 OK\n\n",
+			false,
+		},
+		{ // last-modified match
+			"GET / HTTP/1.1\nIf-Modified-Since: Fri, 31 Dec 1999 00:00:00 GMT\n\n",
+			"HTTP/1.1 200 OK\nLast-Modified: Sat, 01 Jan 2000 00:00:00 GMT\n\n",
+			false,
+		},
+	}
+
+	for _, tt := range tests {
+		buf := bufio.NewReader(strings.NewReader(tt.req))
+		req, err := http.ReadRequest(buf)
+		if err != nil {
+			t.Errorf("http.ReadRequest(%q) returned error: %v", tt.req, err)
+		}
+
+		buf = bufio.NewReader(strings.NewReader(tt.resp))
+		resp, err := http.ReadResponse(buf, req)
+		if err != nil {
+			t.Errorf("http.ReadResponse(%q) returned error: %v", tt.resp, err)
+		}
+
+		if got, want := check304(req, resp), tt.is304; got != want {
+			t.Errorf("check304(%q, %q) returned: %v, want %v", tt.req, tt.resp, got, want)
+		}
+	}
+}