Add tests for users, fix template NPE, fix user HTTP return codes, show validations

cmd/server/assets/header.html

@@ -55,39 +55,41 @@
     </div>
   {{end}}
 
-  <nav class="nav nav-tabs navbar-expand-md navbar-light bg-light">
-    <div class="container">
-      {{template "navtoggle" .}}
-
-      <div class="collapse navbar-collapse mt-2" id="navbar">
-        <ul class="nav mr-auto flex-column flex-md-row">
-          {{if $currentMembership.Can rbac.CodeIssue}}
-            <li class="nav-item">
-              <a class="nav-link {{if .currentPath.IsDir "/codes/issue"}}active{{end}}" href="/codes/issue">
-                {{t $.locale "nav.issue-code"}}
-              </a>
-            </li>
-          {{end}}
-          {{if and $currentMembership.Realm.AllowBulkUpload ($currentMembership.Can rbac.CodeBulkIssue)}}
-            <li class="nav-item">
-              <a class="nav-link {{if .currentPath.IsDir "/codes/bulk-issue"}}active{{end}}" href="/codes/bulk-issue">
-                {{t $.locale "nav.bulk-issue-codes"}}
-              </a>
-            </li>
-          {{end}}
-          {{if $currentMembership.Can rbac.CodeRead}}
-            <li class="nav-item">
-              <a class="nav-link {{if .currentPath.IsDir "/codes/status"}}active{{end}}" href="/codes/status">
-                {{t $.locale "nav.check-code-status"}}
-              </a>
-            </li>
-          {{end}}
-        </ul>
-
-        {{template "navdropdown" .}}
+  {{if .currentPath}}
+    <nav class="nav nav-tabs navbar-expand-md navbar-light bg-light">
+      <div class="container">
+        {{template "navtoggle" .}}
+
+        <div class="collapse navbar-collapse mt-2" id="navbar">
+          <ul class="nav mr-auto flex-column flex-md-row">
+            {{if $currentMembership.Can rbac.CodeIssue}}
+              <li class="nav-item">
+                <a class="nav-link {{if .currentPath.IsDir "/codes/issue"}}active{{end}}" href="/codes/issue">
+                  {{t $.locale "nav.issue-code"}}
+                </a>
+              </li>
+            {{end}}
+            {{if and $currentMembership.Realm.AllowBulkUpload ($currentMembership.Can rbac.CodeBulkIssue)}}
+              <li class="nav-item">
+                <a class="nav-link {{if .currentPath.IsDir "/codes/bulk-issue"}}active{{end}}" href="/codes/bulk-issue">
+                  {{t $.locale "nav.bulk-issue-codes"}}
+                </a>
+              </li>
+            {{end}}
+            {{if $currentMembership.Can rbac.CodeRead}}
+              <li class="nav-item">
+                <a class="nav-link {{if .currentPath.IsDir "/codes/status"}}active{{end}}" href="/codes/status">
+                  {{t $.locale "nav.check-code-status"}}
+                </a>
+              </li>
+            {{end}}
+          </ul>
+
+          {{template "navdropdown" .}}
+        </div>
       </div>
-    </div>
-  </nav>
+    </nav>
+  {{end}}
 </header>
 {{end}}
 

cmd/server/assets/users/_form.html

@@ -19,14 +19,14 @@
 
     <div class="form-label-group">
       <input type="text" id="name" name="name" class="form-control{{if $user.ErrorsFor "name"}} is-invalid{{end}}"
-        value="{{$user.Name}}" placeholder="Name" required autofocus />
+        value="{{$user.Name}}" placeholder="Name" autofocus />
       <label for="name">Name</label>
       {{template "errorable" $user.ErrorsFor "name"}}
     </div>
 
     <div class="form-label-group">
       <input type="email" id="email" name="email" class="form-control{{if $user.ErrorsFor "email"}} is-invalid{{end}}"
-        value="{{$user.Email}}" placeholder="Email address" {{if $user.ID}}disabled{{else}}required{{end}} />
+        value="{{$user.Email}}" placeholder="Email address" {{disabledIf (ne $user.ID 0)}} />
       <label for="email">Email address</label>
       {{template "errorable" $user.ErrorsFor "email"}}
     </div>

cmd/server/assets/users/edit.html

@@ -19,6 +19,8 @@ <h1>Edit user</h1>
     <div class="card mb-3 shadow-sm">
       <div class="card-header">Details</div>
       <div class="card-body">
+        {{template "errorSummary" .user}}
+        {{template "errorSummary" .userMembership}}
         {{template "users/_form" .}}
       </div>
     </div>

cmd/server/assets/users/index.html

@@ -73,8 +73,11 @@
                 <td class="text-center">
                   {{if not (eq $membership.UserID $currentMembership.UserID)}}
                   {{- /* cannot delete yourself */ -}}
-                  <a href="/realm/users/{{$user.ID}}" class="d-block text-danger" data-method="DELETE"
-                    data-confirm="Are you sure you want to remove '{{$user.Name}}'?" data-toggle="tooltip"
+                  <a href="/realm/users/{{$user.ID}}" id="delete-user-{{$user.ID}}"
+                    class="d-block text-danger"
+                    data-method="DELETE"
+                    data-confirm="Are you sure you want to remove '{{$user.Name}}'?"
+                    data-toggle="tooltip"
                     title="Remove this user">
                     <span class="oi oi-trash" aria-hidden="true"></span>
                   </a>

cmd/server/assets/users/new.html

@@ -19,6 +19,8 @@ <h1>New user</h1>
     <div class="card mb-3 shadow-sm">
       <div class="card-header">User details</div>
       <div class="card-body">
+        {{template "errorSummary" .user}}
+        {{template "errorSummary" .userMembership}}
         {{template "users/_form" .}}
       </div>
     </div>

cmd/server/assets/users/show.html

@@ -36,12 +36,12 @@ <h1>{{$user.Name}}</h1>
       </div>
       <div class="card-body mb-n3">
         <h6 class="card-title">Name</h6>
-        <div class="mb-3 mt-n2">
+        <div id="user-name" class="mb-3 mt-n2">
           {{$user.Name}}
         </div>
 
         <h6 class="card-title">Email</h6>
-        <div class="mb-3 mt-n2">
+        <div id="user-email" class="mb-3 mt-n2">
           {{$user.Email}}
         </div>
 

internal/routes/server.go

@@ -258,7 +258,7 @@ func Server(
 		sub.Use(requireMFA)
 		sub.Use(rateLimit)
 
-		userController := user.New(ctx, authProvider, cacher, cfg, db, h)
+		userController := user.New(authProvider, cacher, db, h)
 		userRoutes(sub, userController)
 	}
 

pkg/controller/middleware/method.go

@@ -18,7 +18,6 @@ import (
 	"net/http"
 	"strings"
 
-	"github.com/google/exposure-notifications-server/pkg/logging"
 	"github.com/gorilla/mux"
 )
 
@@ -30,13 +29,8 @@ const formKeyMethod = "_method"
 func MutateMethod() mux.MiddlewareFunc {
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			ctx := r.Context()
-
-			logger := logging.FromContext(ctx).Named("middleware.MutateMethod")
-
 			method := strings.ToUpper(r.FormValue(formKeyMethod))
 			if method != "" {
-				logger.Debugw("overriding method", "old", r.Method, "new", method)
 				r.Method = method
 			}
 

pkg/controller/user/create.go

@@ -25,12 +25,6 @@ import (
 )
 
 func (c *Controller) HandleCreate() http.Handler {
-	type FormData struct {
-		Email       string            `form:"email"`
-		Name        string            `form:"name"`
-		Permissions []rbac.Permission `form:"permissions"`
-	}
-
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		ctx := r.Context()
 
@@ -41,87 +35,113 @@ func (c *Controller) HandleCreate() http.Handler {
 		}
 		flash := controller.Flash(session)
 
-		membership := controller.MembershipFromContext(ctx)
-		if membership == nil {
+		currentMembership := controller.MembershipFromContext(ctx)
+		if currentMembership == nil {
 			controller.MissingMembership(w, r, c.h)
 			return
 		}
-		if !membership.Can(rbac.UserWrite) {
+		if !currentMembership.Can(rbac.UserWrite) {
 			controller.Unauthorized(w, r, c.h)
 			return
 		}
+		currentRealm := currentMembership.Realm
+		currentUser := currentMembership.User
 
-		currentRealm := membership.Realm
-		currentUser := membership.User
+		user := &database.User{}
+		userMembership := &database.Membership{}
 
-		// Requested form, stop processing.
 		if r.Method == http.MethodGet {
-			c.renderNew(ctx, w)
+			c.renderNew(ctx, w, user, userMembership)
 			return
 		}
 
-		var form FormData
-		if err := controller.BindForm(w, r, &form); err != nil {
-			flash.Error("Failed to process form: %v", err)
-			c.renderNew(ctx, w)
+		if err := bindCreateForm(r, currentMembership, user, userMembership); err != nil {
+			user.AddError("", err.Error())
+			w.WriteHeader(http.StatusUnprocessableEntity)
+			c.renderNew(ctx, w, user, userMembership)
 			return
 		}
 
 		// See if the user already exists by email - they may be a member of another
 		// realm.
-		user, err := c.db.FindUserByEmail(form.Email)
-		if err != nil {
-			if !database.IsNotFound(err) {
-				controller.InternalError(w, r, c.h, err)
+		existing, err := c.db.FindUserByEmail(user.Email)
+		if err != nil && !database.IsNotFound(err) {
+			controller.InternalError(w, r, c.h, err)
+			return
+		}
+		if existing != nil && existing.ID != 0 {
+			user.ID = existing.ID
+		}
+
+		// Create or update user.
+		if err := c.db.SaveUser(user, currentUser); err != nil {
+			if database.IsValidationError(err) {
+				w.WriteHeader(http.StatusUnprocessableEntity)
+				c.renderNew(ctx, w, user, userMembership)
 				return
 			}
 
-			user = new(database.User)
-			user.Email = form.Email
-			user.Name = form.Name
-		}
-		if err := c.db.SaveUser(user, currentUser); err != nil {
-			flash.Error("Failed to create user: %v", err)
-			c.renderNew(ctx, w)
+			controller.InternalError(w, r, c.h, err)
 			return
 		}
 
-		// Create membership properties.
-		permission, err := rbac.CompileAndAuthorize(membership.Permissions, form.Permissions)
-		if err != nil {
-			flash.Error("Failed to update user permissions: %s", err)
-			c.renderNew(ctx, w)
-			return
-		}
-		if err := user.AddToRealm(c.db, currentRealm, permission, currentUser); err != nil {
-			flash.Error("Failed to update user in realm: %v", err)
-			c.renderNew(ctx, w)
+		// Create or update membership properties.
+		if err := user.AddToRealm(c.db, currentRealm, userMembership.Permissions, currentUser); err != nil {
+			if database.IsValidationError(err) {
+				w.WriteHeader(http.StatusUnprocessableEntity)
+				c.renderNew(ctx, w, user, userMembership)
+				return
+			}
+
+			controller.InternalError(w, r, c.h, err)
 			return
 		}
 
+		// Ensure the user exists in the upstream auth provider.
 		inviteComposer, err := controller.SendInviteEmailFunc(ctx, c.db, c.h, user.Email, currentRealm)
 		if err != nil {
 			controller.InternalError(w, r, c.h, err)
 			return
 		}
 
 		if _, err := c.authProvider.CreateUser(ctx, user.Name, user.Email, "", true, inviteComposer); err != nil {
-			flash.Alert("Failed to create user: %v", err)
-			c.renderNew(ctx, w)
+			user.AddError("", err.Error())
+			w.WriteHeader(http.StatusUnprocessableEntity)
+			c.renderNew(ctx, w, user, userMembership)
 			return
 		}
 
-		flash.Alert("Successfully created user %v.", user.Name)
+		flash.Alert("Successfully created user %q", user.Name)
 		http.Redirect(w, r, fmt.Sprintf("/realm/users/%d", user.ID), http.StatusSeeOther)
-		return
 	})
 }
 
-func (c *Controller) renderNew(ctx context.Context, w http.ResponseWriter) {
+func bindCreateForm(r *http.Request, currentMembership *database.Membership, user *database.User, membership *database.Membership) error {
+	type FormData struct {
+		Email       string            `form:"email"`
+		Name        string            `form:"name"`
+		Permissions []rbac.Permission `form:"permissions"`
+	}
+
+	var form FormData
+	formErr := controller.BindForm(nil, r, &form)
+	user.Email = form.Email
+	user.Name = form.Name
+
+	permissions, rbacErr := rbac.CompileAndAuthorize(currentMembership.Permissions, form.Permissions)
+	membership.Permissions = permissions
+
+	if formErr != nil {
+		return formErr
+	}
+	return rbacErr
+}
+
+func (c *Controller) renderNew(ctx context.Context, w http.ResponseWriter, user *database.User, membership *database.Membership) {
 	m := controller.TemplateMapFromContext(ctx)
 	m.Title("New user")
-	m["user"] = &database.User{}
-	m["userMembership"] = &database.Membership{}
+	m["user"] = user
+	m["userMembership"] = membership
 	m["permissions"] = rbac.NamePermissionMap
 	c.h.RenderHTML(w, "users/new", m)
 }