test/auth: add more tests for auth endpoints

DEVELOP.md

@@ -160,3 +160,12 @@ if err != nil {
 }
 assert.Equal(t, "what we expect", result.A, "result had the wrong value for a")
 ```
+
+### Code Coverage
+
+Run this to both generate a coverage output file usable by go tools,
+`coverage.out`, and open it using the go coverage tool to visualize line-by-line
+coverage.
+```
+make coverage-viz
+```

Makefile

@@ -2,24 +2,45 @@ GOYACC ?= goyacc
 
 _default: bin/arborist
 
-test: bin/arborist db-test
+bin/arborist: arborist/*.go # help: run the server
+	go build -o bin/arborist
+
+test: bin/arborist db-test # help: run the tests
 	go test -v ./arborist/
 
-bin/arborist: arborist/*.go
-	go build -o bin/arborist
+coverage-viz: coverage # help: generate test coverage file and run coverage visualizer
+	go tool cover --html=coverage.out
 
-up: upgrade
+coverage: test # help: generate test coverage file
+	go test --coverprofile=coverage.out ./arborist/
+	@# Remove auto-generated files from test coverage results
+	@mv coverage.out tmp
+	@grep -v "resource_rules.go" tmp > coverage.out
+	@rm tmp
+
+db-test: $(which psql) # help: set up the database for testing (run automatically by `test`)
+	createdb || true
+	./migrations/latest
+
+up: upgrade # help: try to migrate the database to the next more recent version
 upgrade:
 	./migrations/up
 
-down: downgrade
+down: downgrade # help: try to revert the database to the previous version
 downgrade:
 	./migrations/down
 
-db-test: $(which psql)
-	createdb || true
-	./migrations/latest
-
 arborist/resource_rules.go: arborist/resource_rules.y
 	which $(GOYACC) || go get golang.org/x/tools/cmd/goyacc
 	$(GOYACC) -o arborist/resource_rules.go arborist/resource_rules.y
+
+# You can add a comment following a make target starting with "# help:" to have
+# `make help` include that comment in its output.
+help: # help: show this help
+	@echo "Makefile utilities for arborist. Note that most require you to have already"
+	@echo "exported the necessary postgres variables: \`PGDATABASE\`, \`PGUSER\`, \`PGHOST\`,"
+	@echo "and \`PGPORT\`. Set \`PGSSLMODE=disable\` if not using SSL. See README for details."
+	@echo ""
+	@echo "The default command is bin/arborist."
+	@echo ""
+	@grep -h "^.*:.*# help" $(MAKEFILE_LIST) | grep -v grep | sed -e "s/:.*# help:/:/"

README.md

@@ -181,3 +181,9 @@ Run all the tests:
 ```bash
 go test ./...
 ```
+
+### Code Coverage
+
+Use `make coverage` to generate a `coverage.out` file which can be used with go
+testing and coverage tools. `make coverage-viz` will additionally open it using
+the go coverage tools to visualize line-by-line coverage.

arborist/auth.go

@@ -22,7 +22,7 @@ type AuthRequestJSON_User struct {
 	Audiences []string `json:"aud,omitempty"`
 }
 
-func (requestJSON *AuthRequestJSON_User) Unmarshal(data []byte) error {
+func (requestJSON *AuthRequestJSON_User) UnmarshalJSON(data []byte) error {
 	fields := make(map[string]interface{})
 	err := json.Unmarshal(data, &fields)
 	if err != nil {
@@ -59,7 +59,7 @@ type AuthRequestJSON_Request struct {
 // UnmarshalJSON defines the deserialization from JSON into an AuthRequestJSON
 // struct, which includes validating that required fields are present.
 // (Required fields are anything not in the `optionalFields` variable.)
-func (requestJSON *AuthRequestJSON_Request) Unmarshal(data []byte) error {
+func (requestJSON *AuthRequestJSON_Request) UnmarshalJSON(data []byte) error {
 	fields := make(map[string]interface{})
 	err := json.Unmarshal(data, &fields)
 	if err != nil {
@@ -105,6 +105,9 @@ func authorize(request *AuthRequest) (*AuthResponse, error) {
 	// parse the resource string
 	exp, args, err := Parse(request.Resource)
 	if err != nil {
+		// TODO (rudyardrichter, 2019-04-05): this can return some pretty
+		// unintelligible errors from the yacc code. so far callers are OK to
+		// validate inputs, but could do better to return more readable errors
 		return nil, err
 	}
 

arborist/errors.go

@@ -15,32 +15,6 @@ func (e *httpError) Error() string {
 	return e.msg
 }
 
-func nameError(name string, purpose string, reason string) error {
-	msg := fmt.Sprintf("invalid name %s for %s: %s", name, purpose, reason)
-	return &httpError{msg, http.StatusBadRequest}
-}
-
-func notExist(entity string, idType string, id string) error {
-	msg := fmt.Sprintf("%s with %s `%s` does not exist", entity, idType, id)
-	return &httpError{msg, http.StatusNotFound}
-}
-
-func alreadyExists(entity string, idType string, id string) error {
-	msg := fmt.Sprintf("%s with %s %s already exists", entity, idType, id)
-	return &httpError{msg, http.StatusConflict}
-}
-
-func noDelete(entity string, idType string, identifier string, reason string) error {
-	msg := fmt.Sprintf(
-		"can't delete %s with %s %s; %s",
-		entity,
-		idType,
-		identifier,
-		reason,
-	)
-	return &httpError{msg, http.StatusBadRequest}
-}
-
 func missingRequiredField(entity string, field string) error {
 	msg := fmt.Sprintf("input %s is missing required field `%s`", entity, field)
 	return &httpError{msg, http.StatusBadRequest}

arborist/group.go

@@ -33,12 +33,12 @@ func groupWithName(db *sqlx.DB, name string) (*GroupFromQuery, error) {
 		SELECT
 			grp.name,
 			array_remove(array_agg(usr.name), NULL) AS users,
-			array_remove(array_agg(policy.name), NULL) AS policies
+			array_remove(array_agg(DISTINCT policy.name), NULL) AS policies
 		FROM grp
-		LEFT JOIN usr_grp ON usr_grp.grp_id = grp.id
-		LEFT JOIN usr ON usr.id = usr_grp.usr_id
 		LEFT JOIN grp_policy ON grp_policy.grp_id = grp.id
 		LEFT JOIN policy ON policy.id = grp_policy.policy_id
+		LEFT JOIN usr_grp ON usr_grp.grp_id = grp.id
+		LEFT JOIN usr ON usr.id = usr_grp.usr_id
 		WHERE grp.name = $1
 		GROUP BY grp.id
 		LIMIT 1
@@ -116,8 +116,8 @@ func (group *Group) deleteInDb(db *sqlx.DB) *ErrorResponse {
 	_, err := db.Exec(stmt, group.Name)
 	if err != nil {
 		// TODO: verify correct error
-		msg := fmt.Sprintf("failed to delete group: group does not exist: %s", group.Name)
-		return newErrorResponse(msg, 404, nil)
+		// group does not exist; that's fine
+		return nil
 	}
 	return nil
 }

arborist/permission.go

@@ -2,8 +2,6 @@ package arborist
 
 import (
 	"encoding/json"
-
-	"github.com/jmoiron/sqlx"
 )
 
 type Permission struct {
@@ -53,33 +51,3 @@ func (permission *Permission) UnmarshalJSON(data []byte) error {
 
 	return nil
 }
-
-func permissionWithNameExists(db *sqlx.DB, name string) (bool, error) {
-	stmt := `
-		SELECT COUNT(*)
-		FROM permission
-		WHERE name = $1
-	`
-	var count int
-	err := db.Get(&count, stmt, name)
-	if err != nil {
-		return false, err
-	}
-	exists := count == 1
-	return exists, nil
-}
-
-func permissionsWithNamesExist(db *sqlx.DB, names []string) (bool, error) {
-	stmt := `
-		SELECT COUNT(*)
-		FROM permission
-		WHERE name IN ($1)
-	`
-	var count int
-	err := db.Get(&count, stmt, names)
-	if err != nil {
-		return false, err
-	}
-	exists := count == len(names)
-	return exists, nil
-}

arborist/policy.go

@@ -67,12 +67,12 @@ type PolicyFromQuery struct {
 	RoleIDs       pq.StringArray `db:"role_ids" json:"role_ids"`
 }
 
-func (policyFromQuery *PolicyFromQuery) standardize() *Policy {
+func (policyFromQuery *PolicyFromQuery) standardize() Policy {
 	paths := make([]string, len(policyFromQuery.ResourcePaths))
 	for i, queryPath := range policyFromQuery.ResourcePaths {
 		paths[i] = formatDbPath(queryPath)
 	}
-	policy := &Policy{
+	policy := Policy{
 		Name:          policyFromQuery.Name,
 		ResourcePaths: paths,
 		RoleIDs:       policyFromQuery.RoleIDs,

arborist/resource.go

@@ -79,12 +79,12 @@ type ResourceFromQuery struct {
 
 // standardize takes a resource returned from a query and turns it into the
 // standard form.
-func (resourceFromQuery *ResourceFromQuery) standardize() *Resource {
+func (resourceFromQuery *ResourceFromQuery) standardize() Resource {
 	subresources := []string{}
 	for _, subresource := range resourceFromQuery.Subresources {
 		subresources = append(subresources, formatDbPath(subresource))
 	}
-	resource := &Resource{
+	resource := Resource{
 		Name:         resourceFromQuery.Name,
 		Path:         formatDbPath(resourceFromQuery.Path),
 		Subresources: subresources,

arborist/server.go

@@ -209,7 +209,7 @@ func (server *Server) handleAuthProxy(w http.ResponseWriter, r *http.Request) {
 	if authHeader == "" {
 		msg := "auth proxy request missing auth header"
 		server.logger.Info(msg)
-		errResponse := newErrorResponse(msg, 400, nil)
+		errResponse := newErrorResponse(msg, 401, nil)
 		_ = errResponse.write(w, r)
 		return
 	}
@@ -275,6 +275,18 @@ func (server *Server) handleAuthRequest(w http.ResponseWriter, r *http.Request,
 		return
 	}
 
+	// check that the request has minimum necessary information
+	if authRequest.Request.Resource == "" {
+		msg := "missing resource in auth request"
+		_ = newErrorResponse(msg, 400, nil).write(w, r)
+		return
+	}
+	if info.username == "" && (info.policies == nil || len(info.policies) == 0) {
+		msg := "missing both username and policies in request (at least one is required)"
+		_ = newErrorResponse(msg, 400, nil).write(w, r)
+		return
+	}
+
 	request := &AuthRequest{
 		info.username,
 		info.policies,
@@ -346,7 +358,7 @@ func (server *Server) handleListAuthResources(w http.ResponseWriter, r *http.Req
 		return
 	}
 
-	resources := []*Resource{}
+	resources := []Resource{}
 	for _, resourceFromQuery := range resourcesFromQuery {
 		resources = append(resources, resourceFromQuery.standardize())
 	}
@@ -366,15 +378,24 @@ func (server *Server) handleListAuthResources(w http.ResponseWriter, r *http.Req
 }
 
 func (server *Server) handlePolicyList(w http.ResponseWriter, r *http.Request) {
-	policies, err := listPoliciesFromDb(server.db)
+	policiesFromQuery, err := listPoliciesFromDb(server.db)
+	policies := []Policy{}
+	for _, policyFromQuery := range policiesFromQuery {
+		policies = append(policies, policyFromQuery.standardize())
+	}
 	if err != nil {
 		msg := fmt.Sprintf("policies query failed: %s", err.Error())
 		errResponse := newErrorResponse(msg, 500, nil)
 		server.logger.Error(errResponse.Error.Message)
 		_ = errResponse.write(w, r)
 		return
 	}
-	_ = jsonResponseFrom(policies, http.StatusOK).write(w, r)
+	result := struct {
+		Policies []Policy `json:"policies"`
+	}{
+		Policies: policies,
+	}
+	_ = jsonResponseFrom(result, http.StatusOK).write(w, r)
 }
 
 func (server *Server) handlePolicyCreate(w http.ResponseWriter, r *http.Request, body []byte) {
@@ -435,12 +456,12 @@ func (server *Server) handlePolicyDelete(w http.ResponseWriter, r *http.Request)
 		_ = errResponse.write(w, r)
 		return
 	}
-	_ = jsonResponseFrom(nil, http.StatusCreated).write(w, r)
+	_ = jsonResponseFrom(nil, http.StatusNoContent).write(w, r)
 }
 
 func (server *Server) handleResourceList(w http.ResponseWriter, r *http.Request) {
 	resourcesFromQuery, err := listResourcesFromDb(server.db)
-	resources := []*Resource{}
+	resources := []Resource{}
 	for _, resourceFromQuery := range resourcesFromQuery {
 		resources = append(resources, resourceFromQuery.standardize())
 	}
@@ -451,7 +472,12 @@ func (server *Server) handleResourceList(w http.ResponseWriter, r *http.Request)
 		_ = errResponse.write(w, r)
 		return
 	}
-	_ = jsonResponseFrom(resources, http.StatusOK).write(w, r)
+	result := struct {
+		Resources []Resource `json:"resources"`
+	}{
+		Resources: resources,
+	}
+	_ = jsonResponseFrom(result, http.StatusOK).write(w, r)
 }
 
 func (server *Server) handleResourceCreate(w http.ResponseWriter, r *http.Request, body []byte) {
@@ -571,7 +597,12 @@ func (server *Server) handleRoleList(w http.ResponseWriter, r *http.Request) {
 	for _, roleFromQuery := range rolesFromQuery {
 		roles = append(roles, roleFromQuery.standardize())
 	}
-	_ = jsonResponseFrom(roles, http.StatusOK).write(w, r)
+	result := struct {
+		Roles []Role `json:"roles"`
+	}{
+		Roles: roles,
+	}
+	_ = jsonResponseFrom(result, http.StatusOK).write(w, r)
 }
 
 func (server *Server) handleRoleCreate(w http.ResponseWriter, r *http.Request, body []byte) {