From 2b043b78fa1f8eb6c6e4933cc1d19f20e40ff7a9 Mon Sep 17 00:00:00 2001 From: Kris Runzer Date: Sun, 1 Feb 2015 14:17:18 -0800 Subject: [PATCH] More work on cleaning up recover - Add email layouts --- auth/auth.go | 16 ++++-- config.go | 4 ++ internal/flashutil/flashutil.go | 11 ++++ internal/flashutil/flashutil_test.go | 23 ++++++++ internal/httputil/httputil.go | 11 ---- internal/views/views.go | 40 +++++++++++-- internal/views/views_test.go | 78 +++++++++++++++++++++++++ recover/recover.go | 86 +++++++++++++--------------- storer.go | 14 +++-- 9 files changed, 210 insertions(+), 73 deletions(-) create mode 100644 internal/flashutil/flashutil.go create mode 100644 internal/flashutil/flashutil_test.go delete mode 100644 internal/httputil/httputil.go create mode 100644 internal/views/views_test.go diff --git a/auth/auth.go b/auth/auth.go index 749cf2d..0c03520 100644 --- a/auth/auth.go +++ b/auth/auth.go @@ -38,6 +38,7 @@ type AuthPage struct { ShowRecover bool FlashSuccess string + FlashError string } type Auth struct { @@ -47,7 +48,7 @@ type Auth struct { logoutRedirect string loginRedirect string logger io.Writer - templates *template.Template + templates map[string]*template.Template callbacks *authboss.Callbacks isRememberLoaded bool @@ -55,7 +56,7 @@ type Auth struct { } func (a *Auth) Initialize(config *authboss.Config) (err error) { - if a.templates, err = views.Get(config.ViewsPath, pageLogin); err != nil { + if a.templates, err = views.Get(config.Layout, config.ViewsPath, pageLogin); err != nil { return err } @@ -103,7 +104,9 @@ func (a *Auth) loginHandlerFunc(ctx *authboss.Context, w http.ResponseWriter, r ctx.SessionStorer.Del(authboss.FlashSuccessKey) } - a.templates.ExecuteTemplate(w, pageLogin, page) + tpl := a.templates[pageLogin] + tpl.Execute(w, page) + // tpl.ExecuteTemplate(w, tpl.Name(), page) case methodPOST: u, ok := ctx.FirstPostFormValue("username") if !ok { @@ -112,7 +115,9 @@ func (a *Auth) loginHandlerFunc(ctx *authboss.Context, w http.ResponseWriter, r if err := a.callbacks.FireBefore(authboss.EventAuth, ctx); err != nil { w.WriteHeader(http.StatusForbidden) - a.templates.ExecuteTemplate(w, pageLogin, AuthPage{err.Error(), u, a.isRememberLoaded, a.isRecoverLoaded, ""}) + + tpl := a.templates[pageLogin] + tpl.ExecuteTemplate(w, tpl.Name(), AuthPage{err.Error(), u, a.isRememberLoaded, a.isRecoverLoaded, "", ""}) } p, ok := ctx.FirstPostFormValue("password") @@ -123,7 +128,8 @@ func (a *Auth) loginHandlerFunc(ctx *authboss.Context, w http.ResponseWriter, r if err := a.authenticate(ctx, u, p); err != nil { fmt.Fprintln(a.logger, err) w.WriteHeader(http.StatusForbidden) - a.templates.ExecuteTemplate(w, pageLogin, AuthPage{"invalid username and/or password", u, a.isRememberLoaded, a.isRecoverLoaded, ""}) + tpl := a.templates[pageLogin] + tpl.ExecuteTemplate(w, tpl.Name(), AuthPage{"invalid username and/or password", u, a.isRememberLoaded, a.isRecoverLoaded, "", ""}) return } diff --git a/config.go b/config.go index a8429be..980d580 100644 --- a/config.go +++ b/config.go @@ -1,6 +1,7 @@ package authboss import ( + "html/template" "io" "io/ioutil" "net/smtp" @@ -20,6 +21,9 @@ type Config struct { // BCryptPasswordCost is self explanitory. BCryptCost int + Layout *template.Template + LayoutEmail *template.Template + AuthLogoutRoute string AuthLoginSuccessRoute string diff --git a/internal/flashutil/flashutil.go b/internal/flashutil/flashutil.go new file mode 100644 index 0000000..792fad9 --- /dev/null +++ b/internal/flashutil/flashutil.go @@ -0,0 +1,11 @@ +package flashutil + +import "gopkg.in/authboss.v0" + +// Pull is a convenience func to retreive then delete a flash message. Any ok +// checks are ignored as they don't alter the intended use. +func Pull(storer authboss.ClientStorer, key string) string { + value, _ := storer.Get(key) + storer.Del(key) + return value +} diff --git a/internal/flashutil/flashutil_test.go b/internal/flashutil/flashutil_test.go new file mode 100644 index 0000000..af25a71 --- /dev/null +++ b/internal/flashutil/flashutil_test.go @@ -0,0 +1,23 @@ +package flashutil + +import ( + "testing" + + "gopkg.in/authboss.v0/internal/mocks" +) + +func TestPull(t *testing.T) { + t.Parallel() + + storer := mocks.MockClientStorer{"a": "1"} + + v := Pull(storer, "a") + + if v != "1" { + t.Error(`Expected value "1", got:`, v) + } + + if len(storer) != 0 { + t.Error("Expected length of zero") + } +} diff --git a/internal/httputil/httputil.go b/internal/httputil/httputil.go deleted file mode 100644 index 9cf927c..0000000 --- a/internal/httputil/httputil.go +++ /dev/null @@ -1,11 +0,0 @@ -package httputil - -import "gopkg.in/authboss.v0" - -// PullFlash is a convenience func to retreive then delete a flash message. Any ok -// checks are ignored as they don't alter the intended use. -func PullFlash(storer authboss.ClientStorer, key string) string { - value, _ := storer.Get(key) - storer.Del(key) - return value -} diff --git a/internal/views/views.go b/internal/views/views.go index 5a431b8..dd35964 100644 --- a/internal/views/views.go +++ b/internal/views/views.go @@ -6,15 +6,38 @@ package views //go:generate go-bindata -pkg=views -prefix=templates templates import ( + "errors" "html/template" + "io" "io/ioutil" "os" "path/filepath" ) -// Get all specified templates grouped under single template. -func Get(path string, files ...string) (*template.Template, error) { - root := template.New("") +var ( + // ErrTemplateNotFound should be returned from Get when the view is not found + ErrTemplateNotFound = errors.New("Template not found") +) + +// Templates is a map depicting the forms a template needs wrapped within the specified layout +type Templates map[string]*template.Template + +// ExecuteTemplate is a convenience wrapper for executing a template from the layout. Returns +// ErrTemplateNotFound when the template is missing, othwerise error. +func (t Templates) ExecuteTemplate(w io.Writer, name string, data interface{}) error { + tpl, ok := t[name] + if !ok { + return ErrTemplateNotFound + } + + return tpl.ExecuteTemplate(w, tpl.Name(), data) +} + +// Get parses all speicified files located in path. Each template is wrapped +// in a unique clone of layout. All templates are expecting {{authboss}} handlebars +// for parsing. +func Get(layout *template.Template, path string, files ...string) (Templates, error) { + m := make(Templates) for _, file := range files { b, err := ioutil.ReadFile(filepath.Join(path, file)) @@ -27,11 +50,18 @@ func Get(path string, files ...string) (*template.Template, error) { } } - _, err = root.New(file).Parse(string(b)) + clone, err := layout.Clone() if err != nil { return nil, err } + + _, err = clone.New("authboss").Parse(string(b)) + if err != nil { + return nil, err + } + + m[file] = clone } - return root, nil + return m, nil } diff --git a/internal/views/views_test.go b/internal/views/views_test.go new file mode 100644 index 0000000..b936dbc --- /dev/null +++ b/internal/views/views_test.go @@ -0,0 +1,78 @@ +package views + +import ( + "bytes" + "html/template" + "io/ioutil" + "os" + "path/filepath" + "testing" +) + +func TestTemplates_ExecuteTemplate_ReturnsTemplateWhenFound(t *testing.T) { + t.Parallel() + + tpl, _ := template.New("").Parse("{{.Val}}") + tpls := Templates{"a": tpl} + + b := &bytes.Buffer{} + if err := tpls.ExecuteTemplate(b, "a", struct{ Val string }{"hi"}); err != nil { + t.Error("Unexpected error:", err) + } + + expected := "hi" + actual := b.String() + if expected != actual { + t.Errorf(`Expected "%s", got %s`, expected, actual) + } +} + +func TestTemplates_ExecuteTemplate_ReturnsErrTempalteNotFound(t *testing.T) { + t.Parallel() + + tpls := Templates{} + err := tpls.ExecuteTemplate(ioutil.Discard, "shouldnotbefound", nil) + if err == nil { + t.Error("Expected error") + } + + if err.Error() != "Template not found" { + t.Errorf(`Expected err.Error() to be "Template not found", got: "%s"`, err) + } + +} + +func TestGet(t *testing.T) { + t.Parallel() + + file, err := ioutil.TempFile(os.TempDir(), "authboss") + if err != nil { + t.Error("Unexpected error:", err) + } + if _, err := file.Write([]byte("{{.Val}}")); err != nil { + t.Error("Error writing to temp file", err) + } + + layout, err := template.New("").Parse(`{{template "authboss" .}}`) + if err != nil { + t.Error("Unexpected error:", err) + } + + filename := filepath.Base(file.Name()) + + tpls, err := Get(layout, filepath.Dir(file.Name()), filename) + if err != nil { + t.Error("Unexpected error:", err) + } + + b := &bytes.Buffer{} + if err := tpls.ExecuteTemplate(b, filename, struct{ Val string }{"hi"}); err != nil { + t.Error("Unexpected error:", err) + } + + expected := "hi" + actual := b.String() + if expected != actual { + t.Errorf(`Expected "%s", got %s`, expected, actual) + } +} diff --git a/recover/recover.go b/recover/recover.go index 619ad43..7fd5371 100644 --- a/recover/recover.go +++ b/recover/recover.go @@ -7,14 +7,13 @@ import ( "encoding/base64" "errors" "fmt" - "html/template" "net/http" "time" "golang.org/x/crypto/bcrypt" "gopkg.in/authboss.v0" - "gopkg.in/authboss.v0/internal/httputil" + "gopkg.in/authboss.v0/internal/flashutil" "gopkg.in/authboss.v0/internal/views" ) @@ -43,8 +42,9 @@ func init() { } type RecoverModule struct { - templates *template.Template - config *authboss.Config + templates views.Templates + emailTemplates views.Templates + config *authboss.Config } func (m *RecoverModule) Initialize(config *authboss.Config) (err error) { @@ -56,7 +56,11 @@ func (m *RecoverModule) Initialize(config *authboss.Config) (err error) { return errors.New("recover: RecoverStorer required for recover functionality.") } - if m.templates, err = views.Get(config.ViewsPath, tplRecover, tplRecoverComplete, tplInitHTMLEmail, tplInitTextEmail); err != nil { + if m.templates, err = views.Get(config.Layout, config.ViewsPath, tplRecover, tplRecoverComplete); err != nil { + return err + } + + if m.emailTemplates, err = views.Get(config.LayoutEmail, config.ViewsPath, tplInitHTMLEmail, tplInitTextEmail); err != nil { return err } @@ -84,6 +88,7 @@ func (m *RecoverModule) Storage() authboss.StorageOptions { type pageRecover struct { Username, ConfirmUsername string ErrMap map[string][]string + FlashSuccess string FlashError string } @@ -96,7 +101,7 @@ func (m *RecoverModule) recoverHandlerFunc(ctx *authboss.Context, w http.Respons switch r.Method { case methodGET: - execTpl(pageRecover{FlashError: httputil.PullFlash(ctx.SessionStorer, authboss.FlashErrorKey)}) + execTpl(pageRecover{FlashError: flashutil.Pull(ctx.SessionStorer, authboss.FlashErrorKey)}) case methodPOST: // ignore ok checks as we validate these fields anyways username, _ := ctx.FirstPostFormValue("username") @@ -105,14 +110,14 @@ func (m *RecoverModule) recoverHandlerFunc(ctx *authboss.Context, w http.Respons policies := authboss.FilterValidators(m.config.Policies, "username") if validationErrs := ctx.Validate(policies, m.config.ConfirmFields...); len(validationErrs) > 0 { fmt.Fprintf(m.config.LogWriter, errFormat, "validation failed", validationErrs) - execTpl(pageRecover{username, confirmUsername, validationErrs.Map(), ""}) + execTpl(pageRecover{username, confirmUsername, validationErrs.Map(), "", ""}) return } if err := m.recover(ctx, username); err != nil { // never reveal failed usernames to prevent sniffing fmt.Fprintf(m.config.LogWriter, errFormat, "failed to recover", err) - execTpl(pageRecover{username, confirmUsername, nil, m.config.RecoverFailedErrorFlash}) + execTpl(pageRecover{username, confirmUsername, nil, "", m.config.RecoverFailedErrorFlash}) return } @@ -154,12 +159,12 @@ func (m *RecoverModule) sendRecoverEmail(to string, token []byte) { data := struct{ Link string }{fmt.Sprintf("%s/recover/complete?token=%s", m.config.HostName, base64.URLEncoding.EncodeToString(token))} htmlEmailBody := &bytes.Buffer{} - if err := m.templates.ExecuteTemplate(htmlEmailBody, tplInitHTMLEmail, data); err != nil { + if err := m.emailTemplates.ExecuteTemplate(htmlEmailBody, tplInitHTMLEmail, data); err != nil { fmt.Fprintf(m.config.LogWriter, errFormat, "failed to build html tpl", err) } textEmaiLBody := &bytes.Buffer{} - if err := m.templates.ExecuteTemplate(textEmaiLBody, tplInitTextEmail, data); err != nil { + if err := m.emailTemplates.ExecuteTemplate(textEmaiLBody, tplInitTextEmail, data); err != nil { fmt.Fprintf(m.config.LogWriter, errFormat, "failed to build plaintext tpl", err) } @@ -176,24 +181,24 @@ func (m *RecoverModule) sendRecoverEmail(to string, token []byte) { } type pageRecoverComplete struct { - Token string - ErrMap map[string][]string - FlashError string + Token string + ErrMap map[string][]string + FlashSuccess string + FlashError string } func (m *RecoverModule) recoverCompleteHandlerFunc(ctx *authboss.Context, w http.ResponseWriter, r *http.Request) { + execTpl := func(name string, data interface{}) { + if err := m.templates.ExecuteTemplate(w, name, data); err != nil { + fmt.Fprintf(m.config.LogWriter, errFormat, "unable to execute template", err) + } + } + switch r.Method { case methodGET: - page := pageRecoverComplete{} - - if msg, ok := ctx.SessionStorer.Get(authboss.FlashErrorKey); ok { - page.FlashError = msg - ctx.SessionStorer.Del(authboss.FlashErrorKey) - } - token, ok := ctx.FirstFormValue("token") if !ok { - fmt.Fprintln(m.config.LogWriter, "recover: expected value token") + fmt.Fprintln(m.config.LogWriter, "recover: expected form value token") http.Redirect(w, r, "/", http.StatusFound) return } @@ -214,66 +219,53 @@ func (m *RecoverModule) recoverCompleteHandlerFunc(ctx *authboss.Context, w http return } - page.Token = token - if err := m.templates.ExecuteTemplate(w, tplRecoverComplete, pageRecoverComplete{Token: token}); err != nil { - fmt.Fprintln(m.config.LogWriter, "recover:", err) - } + execTpl(tplRecoverComplete, pageRecoverComplete{ + FlashError: flashutil.Pull(ctx.SessionStorer, authboss.FlashErrorKey), + Token: token, + }) case methodPOST: token, ok := ctx.FirstFormValue("token") if !ok { - fmt.Fprintln(m.config.LogWriter, "recover: expected value token") + fmt.Fprintln(m.config.LogWriter, "recover: expected form value token") + http.Redirect(w, r, "/", http.StatusFound) + return } var err error ctx.User, err = m.verifyToken(token) if err != nil { - fmt.Fprintln(m.config.LogWriter, "recover:", err) + fmt.Fprintln(m.config.LogWriter, "recover 1234:", err) http.Redirect(w, r, "/", http.StatusFound) return } policies := authboss.FilterValidators(m.config.Policies, "password") if validationErrs := ctx.Validate(policies, m.config.ConfirmFields...); len(validationErrs) > 0 { - err := m.templates.ExecuteTemplate(w, tplRecoverComplete, pageRecoverComplete{Token: token, ErrMap: validationErrs.Map()}) - if err != nil { - fmt.Fprintln(m.config.LogWriter, "recover:", err) - } + execTpl(tplRecoverComplete, pageRecoverComplete{Token: token, ErrMap: validationErrs.Map()}) return } password, _ := ctx.FirstFormValue("password") - encryptedPassword, err := bcrypt.GenerateFromPassword([]byte(password), m.config.BCryptCost) if err != nil { fmt.Fprintln(m.config.LogWriter, "recover: failed to encrypt password") - err := m.templates.ExecuteTemplate(w, tplRecoverComplete, pageRecoverComplete{Token: token, FlashError: m.config.RecoverFailedErrorFlash}) - if err != nil { - fmt.Fprintln(m.config.LogWriter, "recover:", err) - } + execTpl(tplRecoverComplete, pageRecoverComplete{Token: token, FlashError: m.config.RecoverFailedErrorFlash}) return } - ctx.User[attrPassword] = string(encryptedPassword) username, ok := ctx.User.String(attrUsername) if !ok { fmt.Println(m.config.LogWriter, "reover: expected user attribue missing:", attrUsername) - err := m.templates.ExecuteTemplate(w, tplRecoverComplete, pageRecoverComplete{Token: token, FlashError: m.config.RecoverFailedErrorFlash}) - if err != nil { - fmt.Fprintln(m.config.LogWriter, "recover:", err) - } + execTpl(tplRecoverComplete, pageRecoverComplete{Token: token, FlashError: m.config.RecoverFailedErrorFlash}) return } - ctx.User[attrRecoverToken] = "" ctx.User[attrRecoverTokenExpiry] = time.Now().UTC() if err := ctx.SaveUser(username, m.config.Storer); err != nil { - fmt.Fprintln(m.config.LogWriter, "recover:", err) - err := m.templates.ExecuteTemplate(w, tplRecoverComplete, pageRecoverComplete{Token: token, FlashError: m.config.RecoverFailedErrorFlash}) - if err != nil { - fmt.Fprintln(m.config.LogWriter, "recover:", err) - } + fmt.Fprintln(m.config.LogWriter, "recover asdf:", err) + execTpl(tplRecoverComplete, pageRecoverComplete{Token: token, FlashError: m.config.RecoverFailedErrorFlash}) return } diff --git a/storer.go b/storer.go index e3c171b..ed7da41 100644 --- a/storer.go +++ b/storer.go @@ -10,9 +10,9 @@ import ( ) var ( - // UserNotFound should be returned from Get when the record is not found. + // ErrUserNotFound should be returned from Get when the record is not found. ErrUserNotFound = errors.New("User not found") - // TokenNotFound should be returned from UseToken when the record is not found. + // ErrTokenNotFound should be returned from UseToken when the record is not found. ErrTokenNotFound = errors.New("Token not found") ) @@ -29,9 +29,9 @@ type Storer interface { // help serialization without using type assertions. Put(key string, attr Attributes) error // Get is for retrieving attributes for a given key. The return value - // must be a struct thot contains a field with the correct type as shown + // must be a struct that contains a field with the correct type as shown // by attrMeta. If the key is not found in the data store simply - // return nil, UserNotFound. + // return nil, ErrUserNotFound. Get(key string, attrMeta AttributeMeta) (interface{}, error) } @@ -45,12 +45,16 @@ type TokenStorer interface { DelTokens(key string) error // UseToken finds the key-token pair, removes the entry in the store // and returns the key that was found. If the token could not be found - // return "", TokenNotFound + // return "", ErrTokenNotFound UseToken(givenKey, token string) (key string, err error) } +// RecoverStorer must be implement in order to satisfy the recover module's +// storage requirements type RecoverStorer interface { Storer + //RecoverUser is for retrieving attributes for a given token. If the key is + //not found in the data store, simply return nil, ErrUserNotFound. RecoverUser(recover string) (interface{}, error) }