client: adds unit tests and addresses minor issues.

Change-Id: I9151991794618c11cca9dffb3b79ebbb42989d16
Reviewed-on: https://team-review.git.corp.google.com/c/golang/vulndb/+/1036403
Reviewed-by: Roland Shoemaker <[email protected]>

Author: zpavlinovic

client/cache.go

@@ -16,7 +16,7 @@ import (
 // it does not implement file locking. When ported to the stdlib it
 // should use cmd/go/internal/lockedfile.
 
-// The cahce uses a single JSON index file for each vulnerability database
+// The cache uses a single JSON index file for each vulnerability database
 // which contains the map from packages to the time the last
 // vulnerability for that package was added/modified and the time that
 // the index was retrieved from the vulnerability database. The JSON

client/client.go

@@ -7,7 +7,6 @@ import (
 	"net/http"
 	"net/url"
 	"os"
-	"path"
 	"path/filepath"
 	"strings"
 	"time"
@@ -57,7 +56,7 @@ func (ls *localSource) Index() (osv.DBIndex, error) {
 }
 
 type httpSource struct {
-	url    string
+	url    string // the base URI of the source (without trailing "/"). e.g. https://vuln.golang.org
 	c      *http.Client
 	cache  Cache
 	dbName string
@@ -82,7 +81,7 @@ func (hs *httpSource) Index() (osv.DBIndex, error) {
 		}
 	}
 
-	req, err := http.NewRequest("GET", path.Join(hs.url, "index.json"), nil)
+	req, err := http.NewRequest("GET", fmt.Sprintf("%s/index.json", hs.url), nil)
 	if err != nil {
 		return nil, err
 	}
@@ -135,10 +134,10 @@ func (hs *httpSource) Get(packages []string) ([]*osv.Entry, error) {
 			}
 			if cached, err := hs.cache.ReadEntries(hs.dbName, p); err != nil {
 				return nil, err
-			} else if cached != nil {
+			} else if len(cached) != 0 {
 				var stale bool
-				for _, e := range entries {
-					if e.LastModified.Before(lastModified) {
+				for _, c := range cached {
+					if c.LastModified.Before(lastModified) {
 						stale = true
 						break
 					}
@@ -155,7 +154,7 @@ func (hs *httpSource) Get(packages []string) ([]*osv.Entry, error) {
 	}
 
 	for _, p := range stillNeed {
-		resp, err := hs.c.Get(path.Join(hs.url, p+".json"))
+		resp, err := hs.c.Get(fmt.Sprintf("%s/%s.json", hs.url, p))
 		if err != nil {
 			return nil, err
 		}
@@ -182,7 +181,7 @@ func (hs *httpSource) Get(packages []string) ([]*osv.Entry, error) {
 			}
 		}
 	}
-	return nil, nil
+	return entries, nil
 }
 
 type Client struct {
@@ -197,9 +196,10 @@ type Options struct {
 func NewClient(sources []string, opts Options) (*Client, error) {
 	c := &Client{}
 	for _, uri := range sources {
+		uri = strings.TrimRight(uri, "/")
 		// should parse the URI out here instead of in there
 		switch {
-		case strings.HasPrefix("http://", uri) || strings.HasPrefix("https://", uri):
+		case strings.HasPrefix(uri, "http://") || strings.HasPrefix(uri, "https://"):
 			hs := &httpSource{url: uri}
 			url, err := url.Parse(uri)
 			if err != nil {
@@ -215,7 +215,7 @@ func NewClient(sources []string, opts Options) (*Client, error) {
 				hs.c = new(http.Client)
 			}
 			c.sources = append(c.sources, hs)
-		case strings.HasPrefix("file://", uri):
+		case strings.HasPrefix(uri, "file://"):
 			url, err := url.Parse(uri)
 			if err != nil {
 				return nil, err

client/client_test.go

@@ -0,0 +1,142 @@
+package client
+
+import (
+	"fmt"
+	"io/ioutil"
+	"net/http"
+	"os"
+	"path"
+	"testing"
+	"time"
+
+	"golang.org/x/vulndb/osv"
+)
+
+var testVuln1 string = `[
+	{"ID":"ID1","Package":{"Name":"golang.org/example/one","Ecosystem":"go"}, "Summary":"",
+	 "Severity":2,"Affects":{"Ranges":[{"Type":2,"Introduced":"","Fixed":"v2.2.0"}]},
+	 "ecosystem_specific":{"Symbols":["some_symbol_1"]
+	}}]`
+
+var testVuln2 string = `[
+	{"ID":"ID2","Package":{"Name":"golang.org/example/two","Ecosystem":"go"}, "Summary":"",
+	 "Severity":2,"Affects":{"Ranges":[{"Type":2,"Introduced":"","Fixed":"v2.1.0"}]},
+	 "ecosystem_specific":{"Symbols":["some_symbol_2"]
+	}}]`
+
+// index containing timestamps for packages in testVuln1 and testVuln2.
+var index string = `{
+	"golang.org/example/one": "2020-03-09T10:00:00.81362141-07:00",
+	"golang.org/example/two": "2019-02-05T09:00:00.31561157-07:00"
+	}`
+
+func serveTestVuln1(w http.ResponseWriter, req *http.Request) {
+	fmt.Fprintf(w, testVuln1)
+}
+
+func serveTestVuln2(w http.ResponseWriter, req *http.Request) {
+	fmt.Fprintf(w, testVuln2)
+}
+
+func serveIndex(w http.ResponseWriter, req *http.Request) {
+	fmt.Fprintf(w, index)
+}
+
+// cachedTestVuln2 returns a function creating a local cache
+// for db with `dbName` with a version of testVuln2 where
+// Summary="cached" and LastModified happened after entry
+// in the `index` for the same pkg.
+func cachedTestVuln2(dbName string) func() Cache {
+	return func() Cache {
+		c := &fsCache{}
+		e := &osv.Entry{
+			ID:           "ID2",
+			Summary:      "cached",
+			LastModified: time.Now(),
+		}
+		c.WriteEntries(dbName, "golang.org/example/two", []*osv.Entry{e})
+		return c
+	}
+}
+
+// createDirAndFile creates a directory `dir` if such directory does
+// not exist and creates a `file` with `content` in the directory.
+func createDirAndFile(dir, file, content string) error {
+	if err := os.MkdirAll(dir, 0755); err != nil {
+		return err
+	}
+	return ioutil.WriteFile(path.Join(dir, file), []byte(content), 0644)
+}
+
+// localDB creates a local db with testVuln1, testVuln2, and index as contents.
+func localDB(t *testing.T) (string, error) {
+	dbName := t.TempDir()
+
+	if err := createDirAndFile(path.Join(dbName, "/golang.org/example/"), "one.json", testVuln1); err != nil {
+		return "", err
+	}
+	if err := createDirAndFile(path.Join(dbName, "/golang.org/example/"), "two.json", testVuln2); err != nil {
+		return "", err
+	}
+	if err := createDirAndFile(path.Join(dbName, ""), "index.json", index); err != nil {
+		return "", err
+	}
+	return dbName, nil
+}
+
+func TestClient(t *testing.T) {
+	// Create a local http database.
+	http.HandleFunc("/golang.org/example/one.json", serveTestVuln1)
+	http.HandleFunc("/golang.org/example/two.json", serveTestVuln2)
+	http.HandleFunc("/index.json", serveIndex)
+	go func() { http.ListenAndServe(":8080", nil) }()
+
+	// Create a local file database.
+	localDBName, err := localDB(t)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(localDBName)
+
+	for _, test := range []struct {
+		name        string
+		source      string
+		createCache func() Cache
+		noVulns     int
+		summaries   map[string]string
+	}{
+		// Test the http client without any cache.
+		{name: "http-no-cache", source: "http://localhost:8080", createCache: func() Cache { return nil }, noVulns: 2, summaries: map[string]string{"ID1": "", "ID2": ""}},
+		// Test the http client with empty cache.
+		{name: "http-empty-cache", source: "http://localhost:8080", createCache: func() Cache { return &fsCache{} }, noVulns: 2, summaries: map[string]string{"ID1": "", "ID2": ""}},
+		// Test the client with non-stale cache containing a version of testVuln2 where Summary="cached".
+		{name: "http-cache", source: "http://localhost:8080", createCache: cachedTestVuln2("localhost"), noVulns: 2, summaries: map[string]string{"ID1": "", "ID2": "cached"}},
+		// Repeat the same for local file client.
+		{name: "file-no-cache", source: "file://" + localDBName, createCache: func() Cache { return nil }, noVulns: 2, summaries: map[string]string{"ID1": "", "ID2": ""}},
+		{name: "file-empty-cache", source: "file://" + localDBName, createCache: func() Cache { return &fsCache{} }, noVulns: 2, summaries: map[string]string{"ID1": "", "ID2": ""}},
+		// Cache does not play a role in local file databases.
+		{name: "file-cache", source: "file://" + localDBName, createCache: cachedTestVuln2(localDBName), noVulns: 2, summaries: map[string]string{"ID1": "", "ID2": ""}},
+	} {
+		// Create fresh cache location each time.
+		cacheRoot = t.TempDir()
+
+		client, err := NewClient([]string{test.source}, Options{HTTPCache: test.createCache()})
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		vulns, err := client.Get([]string{"golang.org/example/one", "golang.org/example/two"})
+		if err != nil {
+			t.Fatal(err)
+		}
+		if len(vulns) != test.noVulns {
+			t.Errorf("want %v vulns for %s; got %v", test.noVulns, test.name, len(vulns))
+		}
+
+		for _, v := range vulns {
+			if s, ok := test.summaries[v.ID]; !ok || v.Summary != s {
+				t.Errorf("want '%s' summary for vuln with id %v in %s; got '%s'", s, v.ID, test.name, v.Summary)
+			}
+		}
+	}
+}