Merge pull request #1031 from niemeyer/fix-auth

client,daemon,overlord: fix authentication

10 files changed, 65 insertions, 29 deletions

diff --git a/client/client.go b/client/client.go
index 350da961b8..bfb8c01e7e 100644
--- a/client/client.go
+++ b/client/client.go
@@ -158,8 +158,10 @@ func (client *Client) doSync(method, path string, query url.Values, headers map[
	return nil, fmt.Errorf("expected sync response, got %q", rsp.Type)
	}
 
-	if err := json.Unmarshal(rsp.Result, v); err != nil {
-	return nil, fmt.Errorf("cannot unmarshal: %v", err)
+	if v != nil {
+	if err := json.Unmarshal(rsp.Result, v); err != nil {
+	return nil, fmt.Errorf("cannot unmarshal: %v", err)
+	}
	}
 
	return &rsp.ResultInfo, nil
diff --git a/client/login.go b/client/login.go
index 8c0b821562..4526b1a105 100644
--- a/client/login.go
+++ b/client/login.go
@@ -103,3 +103,9 @@ func readAuthData() (*User, error) {
 
	return &user, nil
 }
+
+// removeAuthData removes any previously written authentication details.
+func removeAuthData() error {
+	filename := storeAuthDataFilename()
+	return os.Remove(filename)
+}
diff --git a/client/login_test.go b/client/login_test.go
index 49feebd00e..894e16d776 100644
--- a/client/login_test.go
+++ b/client/login_test.go
@@ -20,6 +20,7 @@
 package client_test
 
 import (
+	"fmt"
	"io/ioutil"
	"os"
	"path/filepath"
@@ -59,7 +60,7 @@ func (cs *clientSuite) TestClientLoginError(c *check.C) {
	cs.rsp = `{
	"result": {},
	"status": "Bad Request",
-	"status_code": 400,
+	"status-code": 400,
	"type": "error"
	}`
 
@@ -76,6 +77,28 @@ func (cs *clientSuite) TestClientLoginError(c *check.C) {
	c.Check(osutil.FileExists(outFile), check.Equals, false)
 }
 
+func (cs *clientSuite) TestClientLogout(c *check.C) {
+	cs.rsp = `{"type": "sync", "result": {}}`
+
+	home := os.Getenv("HOME")
+	tmpdir := c.MkDir()
+	os.Setenv("HOME", tmpdir)
+	defer os.Setenv("HOME", home)
+
+	err := os.Mkdir(filepath.Join(tmpdir, ".snap"), 0700)
+	c.Assert(err, check.IsNil)
+	authPath := filepath.Join(tmpdir, ".snap", "auth.json")
+	err = ioutil.WriteFile(authPath, []byte(`{"macaroon":"macaroon","discharges":["discharged"]}`), 0600)
+	c.Assert(err, check.IsNil)
+
+	err = cs.cli.Logout()
+	c.Assert(err, check.IsNil)
+	c.Check(cs.req.Method, check.Equals, "POST")
+	c.Check(cs.req.URL.Path, check.Equals, fmt.Sprintf("/v2/logout"))
+
+	c.Check(osutil.FileExists(authPath), check.Equals, false)
+}
+
 func (cs *clientSuite) TestWriteAuthData(c *check.C) {
	home := os.Getenv("HOME")
	tmpdir := c.MkDir()
diff --git a/client/logout.go b/client/logout.go
index 9036eb5a49..ffb8afe230 100644
--- a/client/logout.go
+++ b/client/logout.go
@@ -19,8 +19,11 @@
 
 package client
 
-// Logout logs the user out
+// Logout logs the user out.
 func (client *Client) Logout() error {
	_, err := client.doSync("POST", "/v2/logout", nil, nil, nil, nil)
-	return err
+	if err != nil {
+	return err
+	}
+	return removeAuthData()
 }
diff --git a/daemon/api.go b/daemon/api.go
index de9ac352ec..16ede8ff7e 100644
--- a/daemon/api.go
+++ b/daemon/api.go
@@ -250,7 +250,7 @@ func UserFromRequest(st *state.State, req *http.Request) (*auth.UserState, error
	// extract macaroons data from request
	header := req.Header.Get("Authorization")
	if header == "" {
-	return nil, errNoAuth
+	return nil, auth.ErrInvalidAuth
	}
 
	authorizationData := strings.SplitN(header, " ", 2)
@@ -308,7 +308,7 @@ func getSnapInfo(c *Command, r *http.Request) Response {
	}
 
	auther, err := c.d.auther(r)
-	if err != nil && err != errNoAuth {
+	if err != nil && err != auth.ErrInvalidAuth {
	return InternalError("%v", err)
	}
 
@@ -404,7 +404,7 @@ func getSnapsInfo(c *Command, r *http.Request) Response {
	remoteRepo := newRemoteRepo()
 
	auther, err := c.d.auther(r)
-	if err != nil && err != errNoAuth {
+	if err != nil && err != auth.ErrInvalidAuth {
	return InternalError("%v", err)
	}
 
@@ -775,7 +775,7 @@ func postSnap(c *Command, r *http.Request) Response {
 
	if err == nil {
	inst.userID = user.ID
-	} else if err != errNoAuth {
+	} else if err != auth.ErrInvalidAuth {
	return InternalError("%v", err)
	}
 
@@ -867,7 +867,7 @@ out:
	user, err := UserFromRequest(state, r)
	if err == nil {
	userID = user.ID
-	} else if err != errNoAuth {
+	} else if err != auth.ErrInvalidAuth {
	return InternalError("%v", err)
	}
 
diff --git a/daemon/api_test.go b/daemon/api_test.go
index 4a9f316305..942710e2fb 100644
--- a/daemon/api_test.go
+++ b/daemon/api_test.go
@@ -679,7 +679,7 @@ func (s *apiSuite) TestUserFromRequestNoHeader(c *check.C) {
	user, err := UserFromRequest(state, req)
	state.Unlock()
 
-	c.Check(err, check.Equals, errNoAuth)
+	c.Check(err, check.Equals, auth.ErrInvalidAuth)
	c.Check(user, check.IsNil)
 }
 
@@ -718,7 +718,7 @@ func (s *apiSuite) TestUserFromRequestHeaderCorrectMissingUser(c *check.C) {
	user, err := UserFromRequest(state, req)
	state.Unlock()
 
-	c.Check(err, check.IsNil)
+	c.Check(err, check.Equals, auth.ErrInvalidAuth)
	c.Check(user, check.IsNil)
 }
 
diff --git a/daemon/daemon.go b/daemon/daemon.go
index 2c5cb7b57b..70a7562733 100644
--- a/daemon/daemon.go
+++ b/daemon/daemon.go
@@ -20,7 +20,6 @@
 package daemon
 
 import (
-	"errors"
	"fmt"
	"net"
	"net/http"
@@ -235,21 +234,16 @@ func (d *Daemon) Dying() <-chan struct{} {
	return d.tomb.Dying()
 }
 
-var errNoAuth = errors.New("no authorization data provided")
-
 func (d *Daemon) auther(r *http.Request) (store.Authenticator, error) {
	overlord := d.overlord
	state := overlord.State()
	state.Lock()
	user, err := UserFromRequest(state, r)
	state.Unlock()
-
-	if err == nil {
-	return user.Authenticator(), nil
-	} else if err != errNoAuth {
+	if err != nil {
	return nil, err
	}
-	return nil, errNoAuth
+	return user.Authenticator(), nil
 }
 
 // New Daemon
diff --git a/daemon/daemon_test.go b/daemon/daemon_test.go
index 1cf87eb01d..941dc14bc9 100644
--- a/daemon/daemon_test.go
+++ b/daemon/daemon_test.go
@@ -214,7 +214,7 @@ func (s *daemonSuite) TestAutherNoAuth(c *check.C) {
	d := newTestDaemon(c)
	user, err := d.auther(req)
 
-	c.Check(err, check.ErrorMatches, errNoAuth.Error())
+	c.Check(err, check.Equals, auth.ErrInvalidAuth)
	c.Check(user, check.IsNil)
 }
 
diff --git a/overlord/auth/auth.go b/overlord/auth/auth.go
index 965af096dd..7d2a6a13c3 100644
--- a/overlord/auth/auth.go
+++ b/overlord/auth/auth.go
@@ -101,7 +101,6 @@ func User(st *state.State, id int) (*UserState, error) {
	var authStateData AuthState
 
	err := st.Get("auth", &authStateData)
-
	if err != nil {
	return nil, err
	}
@@ -114,12 +113,14 @@ func User(st *state.State, id int) (*UserState, error) {
	return nil, fmt.Errorf("invalid user")
 }
 
+var ErrInvalidAuth = fmt.Errorf("invalid authentication")
+
 // CheckMacaroon returns the UserState for the given macaroon/discharges credentials
 func CheckMacaroon(st *state.State, macaroon string, discharges []string) (*UserState, error) {
	var authStateData AuthState
	err := st.Get("auth", &authStateData)
	if err != nil {
-	return nil, nil
+	return nil, ErrInvalidAuth
	}
 
 NextUser:
@@ -139,7 +140,7 @@ NextUser:
	}
	return &user, nil
	}
-	return nil, fmt.Errorf("invalid authentication")
+	return nil, ErrInvalidAuth
 }
 
 // Authenticator returns MacaroonAuthenticator for current authenticated user represented by UserState
diff --git a/overlord/auth/auth_test.go b/overlord/auth/auth_test.go
index ee2673a607..72ba5bb73d 100644
--- a/overlord/auth/auth_test.go
+++ b/overlord/auth/auth_test.go
@@ -128,21 +128,28 @@ func (as *authSuite) TestCheckMacaroonNoAuthData(c *C) {
	user, err := auth.CheckMacaroon(as.state, "macaroon", []string{"discharge"})
	as.state.Unlock()
 
-	c.Check(err, IsNil)
+	c.Check(err, Equals, auth.ErrInvalidAuth)
	c.Check(user, IsNil)
 }
 
-func (as *authSuite) TestCheckMacaroonNoValidUser(c *C) {
+func (as *authSuite) TestCheckMacaroonInvalidAuth(c *C) {
	as.state.Lock()
-	_, err := auth.NewUser(as.state, "username", "macaroon", []string{"discharge"})
+	user, err := auth.CheckMacaroon(as.state, "other-macaroon", []string{"discharge"})
+	as.state.Unlock()
+
+	c.Check(err, Equals, auth.ErrInvalidAuth)
+	c.Check(user, IsNil)
+
+	as.state.Lock()
+	_, err = auth.NewUser(as.state, "username", "macaroon", []string{"discharge"})
	as.state.Unlock()
	c.Check(err, IsNil)
 
	as.state.Lock()
-	user, err := auth.CheckMacaroon(as.state, "other-macaroon", []string{"discharge"})
+	user, err = auth.CheckMacaroon(as.state, "other-macaroon", []string{"discharge"})
	as.state.Unlock()
 
-	c.Check(err, ErrorMatches, "invalid authentication")
+	c.Check(err, Equals, auth.ErrInvalidAuth)
	c.Check(user, IsNil)
 }