Classify errors and return meaningful HTTP error codes

Also start adding some tests for the AccountService code.

actions.go

@@ -2,6 +2,8 @@ package accountserver
 
 import (
 	"context"
+	"crypto/rand"
+	"encoding/base64"
 	"errors"
 
 	"git.autistici.org/ai3/go-common/pwhash"
@@ -35,8 +37,8 @@ type Backend interface {
 	DeleteApplicationSpecificPassword(context.Context, *User, string) error
 }
 
-// AccountService contains the business logic and functionality of the
-// user accounts service.
+// AccountService implements the business logic and high-level
+// functionality of the user accounts management service.
 type AccountService struct {
 	backend Backend
 
@@ -76,18 +78,18 @@ func (s *AccountService) authorizeUser(ctx context.Context, username, ssoToken s
 	// username (or that the SSO ticket has admin permissions).
 	tkt, err := s.validator.Validate(ssoToken, "", s.ssoService, s.ssoGroups)
 	if err != nil {
-		return nil, ErrUnauthorized
+		return nil, newAuthError(err)
 	}
 
 	// Requests are allowed if the SSO ticket corresponds to an admin, or if
 	// it identifies the same user that we're querying.
 	if !s.isAdmin(tkt) && tkt.User != username {
-		return nil, ErrUnauthorized
+		return nil, newAuthError(ErrUnauthorized)
 	}
 
 	user, err := s.backend.GetUser(ctx, username)
 	if err != nil {
-		return nil, err
+		return nil, newBackendError(err)
 	}
 	if user == nil {
 		return nil, ErrUserNotFound
@@ -108,15 +110,19 @@ func (s *AccountService) GetUser(ctx context.Context, req *GetUserRequest) (*Use
 	return s.authorizeUser(ctx, req.Username, req.SSO)
 }
 
-var errResourceNotFound = errors.New("resource not found")
-
 func (s *AccountService) setResourceStatus(ctx context.Context, username, resourceID, status string) error {
 	r, err := s.backend.GetResource(ctx, username, resourceID)
 	if err != nil {
-		return errResourceNotFound
+		return newBackendError(err)
+	}
+	if r == nil {
+		return ErrResourceNotFound
 	}
 	r.Status = status
-	return s.backend.UpdateResource(ctx, username, r)
+	if err := s.backend.UpdateResource(ctx, username, r); err != nil {
+		return newBackendError(err)
+	}
+	return nil
 }
 
 type DisableResourceRequest struct {
@@ -145,28 +151,42 @@ func (s *AccountService) EnableResource(ctx context.Context, req *EnableResource
 
 type ChangeUserPasswordRequest struct {
 	RequestBase
-	OldPassword string `json:"old_password"`
+	CurPassword string `json:"cur_password"`
 	Password    string `json:"password"`
 }
 
+func (r *ChangeUserPasswordRequest) Validate() error {
+	if r.CurPassword == "" {
+		return errors.New("empty 'cur_password' attribute")
+	}
+	if r.Password == "" {
+		return errors.New("empty 'password' attribute")
+	}
+	return nil
+}
+
 func (s *AccountService) ChangeUserPassword(ctx context.Context, req *ChangeUserPasswordRequest) error {
 	user, err := s.authorizeUser(ctx, req.Username, req.SSO)
 	if err != nil {
 		return err
 	}
 
-	if err := s.updateUserEncryptionKeys(ctx, user, req.OldPassword, req.Password, UserEncryptionKeyMainID); err != nil {
-		return err
+	if err := req.Validate(); err != nil {
+		return newRequestError(err)
+	}
+
+	if err := s.updateUserEncryptionKeys(ctx, user, req.CurPassword, req.Password, UserEncryptionKeyMainID); err != nil {
+		return newBackendError(err)
 	}
 
 	// Set the encrypted password attribute on the user and email resources.
 	encPass := pwhash.Encrypt(req.Password)
 	if err := s.backend.SetUserPassword(ctx, user, encPass); err != nil {
-		return err
+		return newBackendError(err)
 	}
 	for _, r := range user.GetResourcesByType(ResourceTypeEmail) {
 		if err := s.backend.SetResourcePassword(ctx, user.Name, r, encPass); err != nil {
-			return err
+			return newBackendError(err)
 		}
 	}
 
@@ -177,14 +197,16 @@ func (s *AccountService) updateUserEncryptionKeys(ctx context.Context, user *Use
 	// Re-encrypt the user encryption key with the new password.
 	keys, err := s.backend.GetUserEncryptionKeys(ctx, user)
 	if err != nil {
-		return err
+		return newBackendError(err)
 	}
-
 	keys, err = reEncryptUserKeys(keys, curPassword, newPassword, keyID)
 	if err != nil {
-		return err
+		return newRequestError(err)
 	}
-	return s.backend.SetUserEncryptionKeys(ctx, user, keys)
+	if err := s.backend.SetUserEncryptionKeys(ctx, user, keys); err != nil {
+		return newBackendError(err)
+	}
+	return nil
 }
 
 // Decode the user encyrption key using the given password, then generate a new
@@ -220,9 +242,19 @@ func reEncryptUserKeys(keys []*UserEncryptionKey, curPassword, newPassword, keyI
 
 type CreateApplicationSpecificPasswordRequest struct {
 	RequestBase
-	Password string `json:"password"` // User password
-	Service  string `json:"service"`
-	Comment  string `json:"comment"`
+	CurPassword string `json:"cur_password"` // User password.
+	Service     string `json:"service"`
+	Comment     string `json:"comment"`
+}
+
+func (r *CreateApplicationSpecificPasswordRequest) Validate() error {
+	if r.CurPassword == "" {
+		return errors.New("empty 'cur_password' attribute")
+	}
+	if r.Service == "" {
+		return errors.New("empty 'service' attribute")
+	}
+	return nil
 }
 
 type CreateApplicationSpecificPasswordResponse struct {
@@ -235,6 +267,10 @@ func (s *AccountService) CreateApplicationSpecificPassword(ctx context.Context,
 		return nil, err
 	}
 
+	if err := req.Validate(); err != nil {
+		return nil, newRequestError(err)
+	}
+
 	// Create a new application-specific password and set it in
 	// the database. We don't need to update the User object as
 	// we're not reusing it.
@@ -246,14 +282,14 @@ func (s *AccountService) CreateApplicationSpecificPassword(ctx context.Context,
 	password := randomAppSpecificPassword()
 	encPass := pwhash.Encrypt(password)
 	if err := s.backend.SetApplicationSpecificPassword(ctx, user, asp, encPass); err != nil {
-		return nil, err
+		return nil, newBackendError(err)
 	}
 
 	// Create or update the user encryption key associated with
 	// this password. The user encryption key IDs for ASPs all
 	// have an 'asp_' prefix, followed by the ASP ID.
 	keyID := "asp_" + asp.ID
-	if err := s.updateUserEncryptionKeys(ctx, user, req.Password, password, keyID); err != nil {
+	if err := s.updateUserEncryptionKeys(ctx, user, req.CurPassword, password, keyID); err != nil {
 		return nil, err
 	}
 
@@ -273,7 +309,7 @@ func (s *AccountService) DeleteApplicationSpecificPassword(ctx context.Context,
 		return err
 	}
 
-	if err := s.backend.DeleteApplicationSpecificPassword(ctx, user, req.AspID); err != nil {
+	if err = s.backend.DeleteApplicationSpecificPassword(ctx, user, req.AspID); err != nil {
 		return err
 	}
 
@@ -302,19 +338,36 @@ type ChangeResourcePasswordRequest struct {
 	Password   string `json:"password"`
 }
 
+func (r *ChangeResourcePasswordRequest) Validate() error {
+	if r.Password == "" {
+		return errors.New("empty 'password' attribute")
+	}
+	return nil
+}
+
 func (s *AccountService) ChangeResourcePassword(ctx context.Context, req *ChangeResourcePasswordRequest) error {
 	_, err := s.authorizeUser(ctx, req.Username, req.SSO)
 	if err != nil {
 		return err
 	}
 
+	if err = req.Validate(); err != nil {
+		return newRequestError(err)
+	}
+
 	r, err := s.backend.GetResource(ctx, req.Username, req.ResourceID)
 	if err != nil {
-		return err
+		return newBackendError(err)
+	}
+	if r == nil {
+		return ErrResourceNotFound
 	}
 
 	encPass := pwhash.Encrypt(req.Password)
-	return s.backend.SetResourcePassword(ctx, req.Username, r, encPass)
+	if err := s.backend.SetResourcePassword(ctx, req.Username, r, encPass); err != nil {
+		return newBackendError(err)
+	}
+	return nil
 }
 
 type MoveResourceRequest struct {
@@ -340,9 +393,7 @@ func (s *AccountService) MoveResource(ctx context.Context, req *MoveResourceRequ
 	}
 	var resources []*Resource
 	if r.Group != "" {
-		for _, r := range user.GetResourcesByGroup(r.Group) {
-			resources = append(resources, r)
-		}
+		resources = append(resources, user.GetResourcesByGroup(r.Group)...)
 	} else {
 		resources = []*Resource{r}
 	}
@@ -359,10 +410,23 @@ func (s *AccountService) MoveResource(ctx context.Context, req *MoveResourceRequ
 	return &resp, nil
 }
 
+const appSpecificPasswordLen = 64
+
+func randomBase64(n int) string {
+	b := make([]byte, n/4*3)
+	_, err := rand.Read(b[:])
+	if err != nil {
+		panic(err)
+	}
+	return base64.StdEncoding.EncodeToString(b[:])
+}
+
 func randomAppSpecificPassword() string {
-	return "haha"
+	return randomBase64(appSpecificPasswordLen)
 }
 
+const appSpecificPasswordIDLen = 4
+
 func randomAppSpecificPasswordID() string {
-	return "1234"
+	return randomBase64(appSpecificPasswordIDLen)
 }

actions_test.go

+package accountserver
+
+import (
+	"context"
+	"testing"
+
+	sso "git.autistici.org/id/go-sso"
+)
+
+type fakeBackend struct {
+	users                map[string]*User
+	resources            map[string]map[string]*Resource
+	passwords            map[string]string
+	appSpecificPasswords map[string][]*AppSpecificPasswordInfo
+	encryptionKeys       map[string][]*UserEncryptionKey
+}
+
+func (b *fakeBackend) GetUser(_ context.Context, username string) (*User, error) {
+	return b.users[username], nil
+}
+
+func (b *fakeBackend) GetResource(_ context.Context, username, resourceID string) (*Resource, error) {
+	return b.resources[username][resourceID], nil
+}
+
+func (b *fakeBackend) UpdateResource(_ context.Context, username string, r *Resource) error {
+	return nil
+}
+
+func (b *fakeBackend) SetUserPassword(_ context.Context, user *User, password string) error {
+	b.passwords[user.Name] = password
+	return nil
+}
+
+func (b *fakeBackend) SetResourcePassword(_ context.Context, username string, r *Resource, password string) error {
+	return nil
+}
+
+func (b *fakeBackend) GetUserEncryptionKeys(_ context.Context, user *User) ([]*UserEncryptionKey, error) {
+	return b.encryptionKeys[user.Name], nil
+}
+
+func (b *fakeBackend) SetUserEncryptionKeys(_ context.Context, user *User, keys []*UserEncryptionKey) error {
+	b.encryptionKeys[user.Name] = keys
+	return nil
+}
+
+func (b *fakeBackend) SetApplicationSpecificPassword(_ context.Context, user *User, info *AppSpecificPasswordInfo, _ string) error {
+	b.appSpecificPasswords[user.Name] = append(b.appSpecificPasswords[user.Name], info)
+	return nil
+}
+
+func (b *fakeBackend) DeleteApplicationSpecificPassword(_ context.Context, user *User, id string) error {
+	return nil
+}
+
+const testAdminGroupName = "admins"
+
+type fakeValidator struct {
+	adminUser string
+}
+
+func (v *fakeValidator) Validate(tkt string, nonce string, service string, _ []string) (*sso.Ticket, error) {
+	// The sso ticket username is just the ticket itself.
+	var groups []string
+	if tkt == v.adminUser {
+		groups = []string{testAdminGroupName}
+	}
+	return &sso.Ticket{
+		User:    tkt,
+		Service: service,
+		Domain:  "test",
+		Groups:  groups,
+	}, nil
+}
+
+func createFakeBackend() *fakeBackend {
+	fb := &fakeBackend{
+		users: map[string]*User{
+			"testuser": &User{
+				Name: "testuser",
+				Resources: []*Resource{
+					{
+						ID:     "email/testuser@example.com",
+						Name:   "testuser@example.com",
+						Type:   ResourceTypeEmail,
+						Status: ResourceStatusActive,
+						Email:  &Email{},
+					},
+				},
+			},
+		},
+	}
+	return fb
+}
+
+func TestService_GetUser(t *testing.T) {
+	svc := NewAccountService(createFakeBackend(), &fakeValidator{}, "service/", nil, testAdminGroupName)
+
+	req := &GetUserRequest{
+		RequestBase: RequestBase{
+			Username: "testuser",
+			SSO:      "testuser",
+		},
+	}
+	resp, err := svc.GetUser(context.TODO(), req)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if resp.Name != "testuser" {
+		t.Fatalf("bad response: %+v", resp)
+	}
+}
+
+func TestService_Auth(t *testing.T) {
+	svc := NewAccountService(createFakeBackend(), &fakeValidator{"adminuser"}, "service/", nil, "admins")
+
+	for _, td := range []struct {
+		sso        string
+		expectedOk bool
+	}{
+		{"testuser", true},
+		{"otheruser", false},
+		{"adminuser", true},
+	} {
+		req := &GetUserRequest{
+			RequestBase: RequestBase{
+				Username: "testuser",
+				SSO:      td.sso,
+			},
+		}
+		_, err := svc.GetUser(context.TODO(), req)
+		if err != nil {
+			if !IsAuthError(err) {
+				t.Errorf("error for sso_user=%s is not an auth error: %v", td.sso, err)
+			} else if td.expectedOk {
+				t.Errorf("error with sso_user=%s: %v", td.sso, err)
+			}
+		} else if !td.expectedOk {
+			t.Errorf("no error with sso_user=%s", td.sso)
+		}
+	}
+}
+
+func TestService_ChangePassword(t *testing.T) {
+	fb := createFakeBackend()
+	svc := NewAccountService(fb, &fakeValidator{}, "service/", nil, testAdminGroupName)
+
+	req := &ChangeUserPasswordRequest{
+		RequestBase: RequestBase{
+			Username: "testuser",
+			SSO:      "testuser",
+		},
+		CurPassword: "cur",
+		Password:    "password",
+	}
+	err := svc.ChangeUserPassword(context.TODO(), req)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if fb.passwords["testuser"] != "password" {
+		t.Fatalf("password was not set on the backend")
+	}
+}

debian/control

@@ -7,7 +7,7 @@ Standards-Version: 3.9.6
 
 Package: accountserver
 Architecture: any
-Depends: ${shlibs:Depends}, ${misc:Depends}, auth-server
+Depends: ${shlibs:Depends}, ${misc:Depends}
 Description: Account metadata server.
  Privileged daemon that manages user accounts.
 

errors.go

+package accountserver
+
+// It is important to distinguish between different classes of errors,
+// so that they can be translated into distinct HTTP status codes and
+// transmitted back to the client. Since we also want to retain the
+// original error message, we wrap the original error with our custom
+// types.
+
+type authError struct {
+	error
+}
+
+func newAuthError(err error) error {
+	return &authError{err}
+}
+
+func IsAuthError(err error) bool {
+	_, ok := err.(*authError)
+	return ok
+}
+
+type requestError struct {
+	error
+}
+
+func newRequestError(err error) error {
+	return &requestError{err}
+}
+
+func IsRequestError(err error) bool {
+	_, ok := err.(*requestError)
+	return ok
+}
+
+type backendError struct {
+	error
+}
+
+func newBackendError(err error) error {
+	return &backendError{err}
+}
+
+func IsBackendError(err error) bool {
+	_, ok := err.(*backendError)
+	return ok
+}

server/server.go

@@ -20,11 +20,13 @@ func New(service *as.AccountService) *AccountServer {
 var emptyResponse = map[string]string{}
 
 func errToStatus(err error) int {
-	switch err {
-	case as.ErrUserNotFound, as.ErrResourceNotFound:
+	switch {
+	case err == as.ErrUserNotFound, err == as.ErrResourceNotFound:
 		return http.StatusNotFound
-	case as.ErrUnauthorized:
+	case as.IsAuthError(err):
 		return http.StatusUnauthorized
+	case as.IsRequestError(err):
+		return http.StatusBadRequest
 	default:
 		return http.StatusInternalServerError
 	}

types.go

@@ -6,15 +6,6 @@ import (
 	"time"
 )
 
-const (
-	StatusActive   = "active"
-	StatusInactive = "inactive"
-	StatusReadonly = "readonly"
-
-	// StatusTemporarilyMoved is OBSOLETE.
-	StatusTemporarilyMoved = "temporary"
-)
-
 // Some considerations about the model design:
 //
 // * Every user has a unique name (email)