Merge lp:~axwalk/juju-core/httpstorage-cleanup into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

environs/httpstorage: cleanup

- s/authoris/authoriz/
- use net.SplitHostPort, and cater for req.Host
  not containing a port
- test HEAD response Location URL, to ensure
  it's valid for the storage

https://codereview.appspot.com/14483043/

environs/httpstorage: cleanup

- s/authoris/authoriz/
- use net.SplitHostPort, and cater for req.Host
  not containing a port
- test HEAD response Location URL, to ensure
  it's valid for the storage

https://codereview.appspot.com/14483043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+189545_code.launchpad.net,

Message:
Please take a look.

Description:
environs/httpstorage: cleanup

- s/authoris/authoriz/
- use net.SplitHostPort, and cater for req.Host
   not containing a port
- test HEAD response Location URL, to ensure
   it's valid for the storage

https://code.launchpad.net/~axwalk/juju-core/httpstorage-cleanup/+merge/189545

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/14483043/

Affected files (+60, -25 lines):
   [revision details]
   environs/httpstorage/backend.go
   environs/httpstorage/backend_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20131004160159-i9eoormyw24etfr4
+New revision: <email address hidden>

Index: environs/httpstorage/backend.go
=== modified file 'environs/httpstorage/backend.go'
--- environs/httpstorage/backend.go 2013-10-04 07:38:49 +0000
+++ environs/httpstorage/backend.go 2013-10-07 08:24:22 +0000
@@ -36,9 +36,9 @@
   switch req.Method {
   case "PUT", "DELETE":
    // Don't allow modifying operations if there's an HTTPS backend
- // to handle that, and ensure the user is authorised/authenticated.
- if s.httpsPort != 0 || !s.authorised(req) {
- http.Error(w, "unauthorised access", http.StatusUnauthorized)
+ // to handle that, and ensure the user is authorized/authenticated.
+ if s.httpsPort != 0 || !s.authorized(req) {
+ http.Error(w, "unauthorized access", http.StatusUnauthorized)
     return
    }
   }
@@ -60,30 +60,43 @@
   }
  }

-// authorised checks that either the storage does not require
-// authorisation, or the user has specified the correct auth key.
-func (s *storageBackend) authorised(req *http.Request) bool {
+// authorized checks that either the storage does not require
+// authorization, or the user has specified the correct auth key.
+func (s *storageBackend) authorized(req *http.Request) bool {
   if s.authkey == "" {
    return true
   }
   return req.URL.Query().Get("authkey") == s.authkey
  }

+// hostOnly splits a host of the form host, or host:port,
+// into its host and port parts, and returns the host part.
+func hostOnly(host string) (string, error) {
+ hostonly, _, err := net.SplitHostPort(host)
+ if err != nil {
+ // err may be because of missing :port. Checking
+ // the error message is brittle, so let's try
+ // again with ":0" tacked on the end.
+ var err2 error
+ hostonly, _, err = net.SplitHostPort(host + ":0")
+ if err2 != nil {
+ // something heinous, return the original error
+ return "", err
+ }
+ }
+ return hostonly, nil
+}
+
  // handleHead returns the HTTPS URL for the specified
  // path in the Location header.
  func (s *storageBackend) handleHead(w http.ResponseWriter, req
*http.Request) {
   if s.httpsPort != 0 {
- hostonly := req.Host
- if i := strings.LastIndex(hostonly, "]"); i != -1 {
- // [ipv6]:port
- hostonly = hostonly[:i+1]
- } else {
- // host:port
- if i := strings.LastIndex(hostonly, ":"); i != -1 {
- hostonly = hostonly[:i]
- }
+...

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/14483043/

[William Reade (fwereade)]

LGTM, looks like an improvement to me :).

https://codereview.appspot.com/14483043/

[Go Bot (go-bot)]

The attempt to merge lp:~axwalk/juju-core/httpstorage-cleanup into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 0.291s
ok launchpad.net/juju-core/agent/tools 0.227s
ok launchpad.net/juju-core/bzr 24.417s
ok launchpad.net/juju-core/cert 3.888s
ok launchpad.net/juju-core/charm 0.765s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.100s
ok launchpad.net/juju-core/cmd 0.298s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 279.074s
ok launchpad.net/juju-core/cmd/jujud 76.216s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 2.171s
ok launchpad.net/juju-core/constraints 0.028s
ok launchpad.net/juju-core/container/lxc 0.377s
? launchpad.net/juju-core/container/lxc/mock [no test files]
ok launchpad.net/juju-core/downloader 5.293s
ok launchpad.net/juju-core/environs 13.326s
ok launchpad.net/juju-core/environs/bootstrap 4.940s
ok launchpad.net/juju-core/environs/cloudinit 0.472s
ok launchpad.net/juju-core/environs/config 0.747s
ok launchpad.net/juju-core/environs/configstore 0.098s
ok launchpad.net/juju-core/environs/filestorage 0.031s
ok launchpad.net/juju-core/environs/httpstorage 1.218s
ok launchpad.net/juju-core/environs/imagemetadata 0.505s
ok launchpad.net/juju-core/environs/instances 0.054s
ok launchpad.net/juju-core/environs/jujutest 0.261s
ok launchpad.net/juju-core/environs/manual 5.175s
ok launchpad.net/juju-core/environs/simplestreams 0.360s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 1.288s
ok launchpad.net/juju-core/environs/storage 1.176s
ok launchpad.net/juju-core/environs/sync 37.287s
ok launchpad.net/juju-core/environs/testing 0.227s
ok launchpad.net/juju-core/environs/tools 7.697s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.022s
ok launchpad.net/juju-core/instance 0.036s
? launchpad.net/juju-core/instance/testing [no test files]

----------------------------------------------------------------------
FAIL: apiconn_test.go:0: DeployLocalSuite.TearDownTest

Waiting for sockets to die: 0 in use, 1 alive
Waiting for sockets to die: 0 in use, 1 alive
Waiting for sockets to die: 0 in use, 1 alive
Waiting for sockets to die: 0 in use, 1 alive
Waiting for sockets to die: 0 in use, 1 alive
Waiting for sockets to die: 0 in use, 1 alive
Waiting for sockets to die: 0 in use, 1 alive
Waiting for sockets to die: 0 in use, 1 alive
Waiting for sockets to die: 0 in use, 1 alive
Waiting for sockets to die: 0 in use, 1 alive
Waiting for sockets to die: 0 in use, 1 alive
Waiting for sockets to die: 0 in use, 1 alive
Waiting for sockets to die: 0 in use, 1 alive
Waiting for sockets to die: 0 in use, 1 alive
Waiting for sockets to die: 0 in use, 1 alive
Waiting for sockets to die: 0 in use, 1 alive
Waiting for sockets to die: 0 in use, 1 alive
Waiting for sockets to die: 0 in use, 1 alive
Waiting for sockets to die: 0 in use, 1 alive
Waiting for s...

=== modified file 'environs/httpstorage/backend.go'
--- environs/httpstorage/backend.go	2013-10-04 07:38:49 +0000
+++ environs/httpstorage/backend.go	2013-10-07 08:39:18 +0000
@@ -36,9 +36,9 @@
 	switch req.Method {
 	case "PUT", "DELETE":
 		// Don't allow modifying operations if there's an HTTPS backend
-		// to handle that, and ensure the user is authorised/authenticated.
-		if s.httpsPort != 0 || !s.authorised(req) {
-			http.Error(w, "unauthorised access", http.StatusUnauthorized)
+		// to handle that, and ensure the user is authorized/authenticated.
+		if s.httpsPort != 0 || !s.authorized(req) {
+			http.Error(w, "unauthorized access", http.StatusUnauthorized)
 			return
 		}
 	}
@@ -60,30 +60,43 @@
 	}
 }
 
-// authorised checks that either the storage does not require
-// authorisation, or the user has specified the correct auth key.
-func (s *storageBackend) authorised(req *http.Request) bool {
+// authorized checks that either the storage does not require
+// authorization, or the user has specified the correct auth key.
+func (s *storageBackend) authorized(req *http.Request) bool {
 	if s.authkey == "" {
 		return true
 	}
 	return req.URL.Query().Get("authkey") == s.authkey
 }
 
+// hostOnly splits a host of the form host, or host:port,
+// into its host and port parts, and returns the host part.
+func hostOnly(host string) (string, error) {
+	hostonly, _, err := net.SplitHostPort(host)
+	if err != nil {
+		// err may be because of missing :port. Checking
+		// the error message is brittle, so let's try
+		// again with ":0" tacked on the end.
+		var err2 error
+		hostonly, _, err = net.SplitHostPort(host + ":0")
+		if err2 != nil {
+			// something heinous, return the original error
+			return "", err
+		}
+	}
+	return hostonly, nil
+}
+
 // handleHead returns the HTTPS URL for the specified
 // path in the Location header.
 func (s *storageBackend) handleHead(w http.ResponseWriter, req *http.Request) {
 	if s.httpsPort != 0 {
-		hostonly := req.Host
-		if i := strings.LastIndex(hostonly, "]"); i != -1 {
-			// [ipv6]:port
-			hostonly = hostonly[:i+1]
-		} else {
-			// host:port
-			if i := strings.LastIndex(hostonly, ":"); i != -1 {
-				hostonly = hostonly[:i]
-			}
+		host, err := hostOnly(req.Host)
+		if err != nil {
+			http.Error(w, fmt.Sprintf("failed to split host: %v", err), http.StatusBadRequest)
+			return
 		}
-		url := fmt.Sprintf("https://%s:%d%s", hostonly, s.httpsPort, req.URL.Path)
+		url := fmt.Sprintf("https://%s:%d%s", host, s.httpsPort, req.URL.Path)
 		w.Header().Set("Location", url)
 	} else {
 		http.Error(w, "method HEAD is not supported", http.StatusMethodNotAllowed)
@@ -139,8 +152,8 @@
 
 // handleDelete removes a file from the storage.
 func (s *storageBackend) handleDelete(w http.ResponseWriter, req *http.Request) {
-	if !s.authorised(req) {
-		http.Error(w, "unauthorised access", http.StatusUnauthorized)
+	if !s.authorized(req) {
+		http.Error(w, "unauthorized access", http.StatusUnauthorized)
 		return
 	}
 	err := s.backend.Remove(req.URL.Path[1:])

=== modified file 'environs/httpstorage/backend_test.go'
--- environs/httpstorage/backend_test.go	2013-10-04 08:06:32 +0000
+++ environs/httpstorage/backend_test.go	2013-10-07 08:39:18 +0000
@@ -134,22 +134,42 @@
 	},
 }
 
-func (s *backendSuite) TestHead(c *gc.C) {
+func (s *backendSuite) TestHeadNonAuth(c *gc.C) {
 	// HEAD is unsupported for non-authenticating servers.
 	listener, url, _ := startServer(c)
 	defer listener.Close()
 	resp, err := http.Head(url)
 	c.Assert(err, gc.IsNil)
 	c.Assert(resp.StatusCode, gc.Equals, http.StatusMethodNotAllowed)
+}
 
+func (s *backendSuite) TestHeadAuth(c *gc.C) {
 	// HEAD on an authenticating server will return the HTTPS counterpart URL.
+	client, url, datadir := s.tlsServerAndClient(c)
+	createTestData(c, datadir)
+
+	resp, err := client.Head(url)
+	c.Assert(err, gc.IsNil)
+	c.Assert(resp.StatusCode, gc.Equals, http.StatusOK)
+	location, err := resp.Location()
+	c.Assert(err, gc.IsNil)
+	c.Assert(location.String(), gc.Matches, "https://localhost:[0-9]{5}/")
+	testGet(c, client, location.String())
+}
+
+func (s *backendSuite) TestHeadCustomHost(c *gc.C) {
+	// HEAD with a custom "Host:" header; the server should respond
+	// with a Location with the specified Host header.
 	client, url, _ := s.tlsServerAndClient(c)
-	resp, err = client.Head(url + "arbitrary")
+	req, err := http.NewRequest("HEAD", url+"arbitrary", nil)
+	c.Assert(err, gc.IsNil)
+	req.Host = "notarealhost"
+	resp, err := client.Do(req)
 	c.Assert(err, gc.IsNil)
 	c.Assert(resp.StatusCode, gc.Equals, http.StatusOK)
 	location, err := resp.Location()
 	c.Assert(err, gc.IsNil)
-	c.Assert(location.String(), gc.Matches, "https://localhost:[0-9]{5}/arbitrary")
+	c.Assert(location.String(), gc.Matches, "https://notarealhost:[0-9]{5}/arbitrary")
 }
 
 func (s *backendSuite) TestGet(c *gc.C) {
@@ -283,7 +303,7 @@
 	testPut(c, http.DefaultClient, url, dataDir, true)
 }
 
-func testPut(c *gc.C, client *http.Client, url, dataDir string, authorised bool) {
+func testPut(c *gc.C, client *http.Client, url, dataDir string, authorized bool) {
 	check := func(tc testCase) {
 		req, err := http.NewRequest("PUT", url+tc.name, bytes.NewBufferString(tc.content))
 		c.Assert(err, gc.IsNil)
@@ -293,7 +313,7 @@
 		if tc.status != 0 {
 			c.Assert(resp.StatusCode, gc.Equals, tc.status)
 			return
-		} else if !authorised {
+		} else if !authorized {
 			c.Assert(resp.StatusCode, gc.Equals, http.StatusUnauthorized)
 			return
 		}
@@ -341,7 +361,7 @@
 	testRemove(c, http.DefaultClient, url, dataDir, true)
 }
 
-func testRemove(c *gc.C, client *http.Client, url, dataDir string, authorised bool) {
+func testRemove(c *gc.C, client *http.Client, url, dataDir string, authorized bool) {
 	check := func(tc testCase) {
 		fp := filepath.Join(dataDir, tc.name)
 		dir, _ := filepath.Split(fp)
@@ -357,7 +377,7 @@
 		if tc.status != 0 {
 			c.Assert(resp.StatusCode, gc.Equals, tc.status)
 			return
-		} else if !authorised {
+		} else if !authorized {
 			c.Assert(resp.StatusCode, gc.Equals, http.StatusUnauthorized)
 			return
 		}