Rewrite auth module

Discovered many problems with the abstractions along the way
and did small fixes to get to the end of the auth module.

- Use more constants for random strings
- Create forcing functions to deal with the upgrades to different
  interfaces

Author: aarondl

auth/auth.go

@@ -2,17 +2,17 @@
 package auth
 
 import (
-	"fmt"
 	"net/http"
 
+	"golang.org/x/crypto/bcrypt"
+
 	"github.com/pkg/errors"
 	"github.com/volatiletech/authboss"
-	"github.com/volatiletech/authboss/internal/response"
-	"golang.org/x/crypto/bcrypt"
 )
 
 const (
-	tplLogin = "login.html.tpl"
+	// PageLogin is for identifying the login page for parsing & validation
+	PageLogin = "login"
 )
 
 func init() {
@@ -28,7 +28,7 @@ type Auth struct {
 func (a *Auth) Init(ab *authboss.Authboss) (err error) {
 	a.Authboss = ab
 
-	if err := a.Authboss.Config.Core.ViewRenderer.Load(tplLogin); err != nil {
+	if err = a.Authboss.Config.Core.ViewRenderer.Load(PageLogin); err != nil {
 		return err
 	}
 
@@ -44,106 +44,93 @@ func (a *Auth) Init(ab *authboss.Authboss) (err error) {
 		return errors.Errorf("auth wants to register a logout route but is given an invalid method: %s", a.Authboss.Config.Modules.LogoutMethod)
 	}
 
-	a.Authboss.Config.Core.Router.Get("/login", http.HandlerFunc(loginGet))
-	a.Authboss.Config.Core.Router.Post("/login", http.HandlerFunc(loginPost))
-	logoutRouteMethod("/logout", http.HandlerFunc(logout))
+	a.Authboss.Config.Core.Router.Get("/login", a.Authboss.Core.ErrorHandler.Wrap(a.LoginGet))
+	a.Authboss.Config.Core.Router.Post("/login", a.Authboss.Core.ErrorHandler.Wrap(a.LoginPost))
+	logoutRouteMethod("/logout", a.Authboss.Core.ErrorHandler.Wrap(a.Logout))
 
 	return nil
 }
 
-func (a *Auth) loginGet(w http.ResponseWriter, r *http.Request) error {
-	data := authboss.NewHTMLData(
-		"showRemember", a.IsLoaded("remember"),
-		"showRecover", a.IsLoaded("recover"),
-		"showRegister", a.IsLoaded("register"),
-		"primaryID", a.PrimaryID,
-		"primaryIDValue", "",
-	)
-	return a.templates.Render(ctx, w, r, tplLogin, data)
+// LoginGet simply displays the login form
+func (a *Auth) LoginGet(w http.ResponseWriter, r *http.Request) error {
+	return a.Core.Responder.Respond(w, r, http.StatusOK, PageLogin, nil)
 }
 
-func (a *Auth) loginPost(w http.ResponseWriter, r *http.Request) error {
-	switch r.Method {
-	case methodGET:
-	case methodPOST:
-		key := r.FormValue(a.PrimaryID)
-		password := r.FormValue("password")
-
-		errData := authboss.NewHTMLData(
-			"error", fmt.Sprintf("invalid %s and/or password", a.PrimaryID),
-			"primaryID", a.PrimaryID,
-			"primaryIDValue", key,
-			"showRemember", a.IsLoaded("remember"),
-			"showRecover", a.IsLoaded("recover"),
-			"showRegister", a.IsLoaded("register"),
-		)
-
-		if valid, err := validateCredentials(ctx, key, password); err != nil {
-			errData["error"] = "Internal server error"
-			fmt.Fprintf(ctx.LogWriter, "auth: validate credentials failed: %v\n", err)
-			return a.templates.Render(ctx, w, r, tplLogin, errData)
-		} else if !valid {
-			if err := a.Events.FireAfter(authboss.EventAuthFail, ctx); err != nil {
-				fmt.Fprintf(ctx.LogWriter, "EventAuthFail callback error'd out: %v\n", err)
-			}
-			return a.templates.Render(ctx, w, r, tplLogin, errData)
-		}
+// LoginPost attempts to validate the credentials passed in
+// to log in a user.
+func (a *Auth) LoginPost(w http.ResponseWriter, r *http.Request) error {
+	validatable, err := a.Authboss.Core.BodyReader.Read(PageLogin, r)
+	if err != nil {
+		return err
+	}
 
-		interrupted, err := a.Events.FireBefore(authboss.EventAuth, ctx)
-		if err != nil {
-			return err
-		} else if interrupted != authboss.InterruptNone {
-			var reason string
-			switch interrupted {
-			case authboss.InterruptAccountLocked:
-				reason = "Your account has been locked."
-			case authboss.InterruptAccountNotConfirmed:
-				reason = "Your account has not been confirmed."
-			}
-			response.Redirect(ctx, w, r, a.AuthLoginFailPath, "", reason, false)
-			return nil
-		}
+	// Skip validation since all the validation happens during the database lookup and
+	// password check.
+	creds := authboss.MustHaveUserValues(validatable)
 
-		ctx.SessionStorer.Put(authboss.SessionKey, key)
-		ctx.SessionStorer.Del(authboss.SessionHalfAuthKey)
-		ctx.Values = map[string]string{authboss.CookieRemember: r.FormValue(authboss.CookieRemember)}
+	pid := creds.GetPID()
+	pidUser, err := a.Authboss.Storage.Server.Load(r.Context(), pid)
+	if err == authboss.ErrUserNotFound {
+		data := authboss.HTMLData{authboss.DataErr: "Invalid Credentials"}
+		return a.Authboss.Core.Responder.Respond(w, r, http.StatusOK, PageLogin, data)
+	} else if err != nil {
+		return err
+	}
 
-		if err := a.Events.FireAfter(authboss.EventAuth, ctx); err != nil {
-			return err
-		}
-		response.Redirect(ctx, w, r, a.AuthLoginOKPath, "", "", true)
-	default:
-		w.WriteHeader(http.StatusMethodNotAllowed)
+	authUser := authboss.MustBeAuthable(pidUser)
+	password, err := authUser.GetPassword(r.Context())
+	if err != nil {
+		return err
 	}
 
-	return nil
-}
+	err = bcrypt.CompareHashAndPassword([]byte(password), []byte(creds.GetPassword()))
+	if err != nil {
+		err = a.Authboss.Events.FireAfter(r.Context(), authboss.EventAuthFail)
+		if err != nil {
+			return err
+		}
 
-func validateCredentials(key, password string) (bool, error) {
-	if err := ctx.LoadUser(key); err == authboss.ErrUserNotFound {
-		return false, nil
-	} else if err != nil {
-		return false, err
+		data := authboss.HTMLData{authboss.DataErr: "Invalid Credentials"}
+		return a.Authboss.Core.Responder.Respond(w, r, http.StatusOK, PageLogin, data)
 	}
 
-	actualPassword, err := ctx.User.StringErr(authboss.StorePassword)
+	interrupted, err := a.Events.FireBefore(r.Context(), authboss.EventAuth)
 	if err != nil {
-		return false, err
+		return err
+	} else if interrupted != authboss.InterruptNone {
+		var reason string
+		switch interrupted {
+		case authboss.InterruptAccountLocked:
+			reason = "Your account is locked"
+		case authboss.InterruptAccountNotConfirmed:
+			reason = "Your account is not confirmed"
+		}
+		data := authboss.HTMLData{authboss.DataErr: reason}
+		return a.Authboss.Core.Responder.Respond(w, r, http.StatusOK, PageLogin, data)
 	}
 
-	if err := bcrypt.CompareHashAndPassword([]byte(actualPassword), []byte(password)); err != nil {
-		return false, nil
+	authboss.PutSession(w, authboss.SessionKey, pid)
+	authboss.DelSession(w, authboss.SessionHalfAuthKey)
+
+	if err := a.Authboss.Events.FireAfter(r.Context(), authboss.EventAuth); err != nil {
+		return err
 	}
 
-	return true, nil
+	ro := authboss.RedirectOptions{
+		RedirectPath: a.Authboss.Paths.AuthLogoutOK,
+	}
+	return a.Authboss.Core.Redirector.Redirect(w, r, ro)
 }
 
-func (a *Auth) logout(w http.ResponseWriter, r *http.Request) error {
-	ctx.SessionStorer.Del(authboss.SessionKey)
-	ctx.CookieStorer.Del(authboss.CookieRemember)
-	ctx.SessionStorer.Del(authboss.SessionLastAction)
-
-	response.Redirect(ctx, w, r, a.AuthLogoutOKPath, "You have logged out", "", true)
+// Logout a user
+func (a *Auth) Logout(w http.ResponseWriter, r *http.Request) error {
+	authboss.DelSession(w, authboss.SessionKey)
+	authboss.DelSession(w, authboss.SessionLastAction)
+	authboss.DelCookie(w, authboss.CookieRemember)
 
-	return nil
+	ro := authboss.RedirectOptions{
+		RedirectPath: a.Authboss.Paths.AuthLogoutOK,
+		Success:      "You have been logged out",
+	}
+	return a.Authboss.Core.Redirector.Redirect(w, r, ro)
 }

config.go

@@ -12,8 +12,6 @@ type Config struct {
 
 		// AuthLoginOK is the redirect path after a successful authentication.
 		AuthLoginOK string
-		// AuthLoginFail is the redirect path after a failed authentication.
-		AuthLoginFail string
 		// AuthLogoutOK is the redirect path after a log out.
 		AuthLogoutOK string
 
@@ -97,9 +95,9 @@ type Config struct {
 		// only for redirection.
 		Redirector HTTPRedirector
 
-		// Validator helps validate an http request, it's given a name that describes
-		// the form it's validating so that conditional logic may be applied.
-		Validator Validator
+		// BodyReader reads validatable data from the body of a request to be able
+		// to get data from the user's client.
+		BodyReader BodyReader
 
 		// ViewRenderer loads the templates for the application.
 		ViewRenderer Renderer

defaults/defaults.go

@@ -0,0 +1,22 @@
+package defaults
+
+import (
+	"os"
+
+	"github.com/volatiletech/authboss"
+)
+
+// SetDefaultCore creates instances of all the default pieces
+//
+// Assumes you have a ViewRenderer already set.
+func SetDefaultCore(config *authboss.Config, useUsername bool) {
+	logger := NewLogger(os.Stdout)
+
+	config.Core.Router = NewRouter()
+	config.Core.ErrorHandler = ErrorHandler{LogWriter: logger}
+	config.Core.Responder = &Responder{Renderer: config.Core.ViewRenderer}
+	config.Core.Redirector = &Redirector{Renderer: config.Core.ViewRenderer, FormValueName: "redir"}
+	config.Core.BodyReader = NewHTTPFormReader(useUsername)
+	config.Core.Mailer = NewLogMailer(os.Stdout)
+	config.Core.Logger = logger
+}

defaults/error_handler.go

@@ -2,8 +2,9 @@ package defaults
 
 import (
 	"fmt"
-	"io"
 	"net/http"
+
+	"github.com/volatiletech/authboss"
 )
 
 // ErrorHandler wraps http handlers with errors with itself
@@ -12,7 +13,7 @@ import (
 // The pieces provided to this struct must be thread-safe
 // since they will be handed to many pointers to themselves.
 type ErrorHandler struct {
-	LogWriter io.Writer
+	LogWriter authboss.Logger
 }
 
 // Wrap an http handler with an error
@@ -25,7 +26,7 @@ func (e ErrorHandler) Wrap(handler func(w http.ResponseWriter, r *http.Request)
 
 type errorHandler struct {
 	Handler   func(w http.ResponseWriter, r *http.Request) error
-	LogWriter io.Writer
+	LogWriter authboss.Logger
 }
 
 // ServeHTTP handles errors
@@ -35,5 +36,5 @@ func (e errorHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	fmt.Fprintf(e.LogWriter, "error at %s: %+v", r.URL.String(), err)
+	e.LogWriter.Error(fmt.Sprintf("error at %s: %+v", r.URL.String(), err))
 }

defaults/error_handler_test.go

@@ -15,7 +15,7 @@ func TestErrorHandler(t *testing.T) {
 
 	b := &bytes.Buffer{}
 
-	eh := ErrorHandler{LogWriter: b}
+	eh := ErrorHandler{LogWriter: NewLogger(b)}
 
 	handler := eh.Wrap(func(w http.ResponseWriter, r *http.Request) error {
 		return errors.New("error occurred")

defaults/router.go

@@ -56,8 +56,8 @@ func (r *Router) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 	case "DELETE":
 		router = r.deletes
 	default:
-		w.WriteHeader(http.StatusBadRequest)
-		io.WriteString(w, "bad request, this method not allowed")
+		w.WriteHeader(http.StatusMethodNotAllowed)
+		io.WriteString(w, "method not allowed")
 		return
 	}
 

defaults/router_test.go

@@ -77,7 +77,7 @@ func TestRouterBadMethod(t *testing.T) {
 
 	r.ServeHTTP(wr, req)
 
-	if wr.Code != http.StatusBadRequest {
-		t.Error("want bad request code, got:", wr.Code)
+	if wr.Code != http.StatusMethodNotAllowed {
+		t.Error("want method not allowed code, got:", wr.Code)
 	}
 }

defaults/values.go

@@ -101,7 +101,7 @@ func (h HTTPFormReader) Read(page string, r *http.Request) (authboss.Validator,
 		validator := HTTPFormValidator{
 			Values:        values,
 			Ruleset:       rules,
-			ConfirmFields: []string{FormValuePassword, "confirm_" + FormValuePassword},
+			ConfirmFields: []string{FormValuePassword, authboss.ConfirmPrefix + FormValuePassword},
 		}
 		password := values[FormValuePassword]
 

html_data.go

@@ -1,5 +1,14 @@
 package authboss
 
+// Keys for use in HTMLData that are meaningful
+const (
+	// DataErr is for one off errors that don't really belong to
+	// a particular field
+	DataErr = "error"
+	// DataValidation is for validation errors
+	DataValidation = "errors"
+)
+
 // HTMLData is used to render templates with.
 type HTMLData map[string]interface{}
 

renderer.go

@@ -5,8 +5,8 @@ import "context"
 // Renderer is a type that can render a given template with some data.
 type Renderer interface {
 	// Load the given templates, will most likely be called multiple times
-	Load(name ...string) error
+	Load(names ...string) error
 
 	// Render the given template
-	Render(ctx context.Context, name string, data HTMLData) (output []byte, contentType string, err error)
+	Render(ctx context.Context, page string, data HTMLData) (output []byte, contentType string, err error)
 }

storage.go

@@ -106,3 +106,12 @@ type OAuth2User interface {
 	PutRefreshToken(ctx context.Context, refreshToken string) error
 	PutExpiry(ctx context.Context, expiry time.Duration) error
 }
+
+// MustBeAuthable forces an upgrade conversion to Authable
+// or will panic.
+func MustBeAuthable(u User) AuthableUser {
+	if au, ok := u.(AuthableUser); ok {
+		return au
+	}
+	panic("could not upgrade user to an authable user, check your user struct")
+}

values.go

@@ -1,6 +1,9 @@
 package authboss
 
-import "net/http"
+import (
+	"fmt"
+	"net/http"
+)
 
 // BodyReader reads data from the request
 // and returns it in an abstract form.
@@ -27,3 +30,13 @@ type UserValuer interface {
 	GetPID() string
 	GetPassword() string
 }
+
+// MustHaveUserValues upgrades a validatable set of values
+// to ones specific to the user.
+func MustHaveUserValues(v Validator) UserValuer {
+	if u, ok := v.(UserValuer); ok {
+		return u
+	}
+
+	panic(fmt.Sprintf("bodyreader returned a type that could not be upgraded to UserValuer: %T", v))
+}