From 5b0e33aef95b80cac2a2aa39f97af9af0c6cb464 Mon Sep 17 00:00:00 2001
From: Gregor Best <gbe@unobtanium.de>
Date: Sun, 6 Feb 2022 18:23:56 +0100
Subject: [PATCH] a whole lot of cleanups
---
.golangci.yaml | 6 ++-
auth/auth.go | 9 ++---
auth/auth_test.go | 1 +
handler-details.go | 88 ++++++++++++++++++++---------------------
handler-index.go | 71 +++++++++++++++------------------
handler-user.go | 2 +
session/authprovider.go | 85 ++++++++++++---------------------------
storage/storage.go | 54 +++++++++++++------------
8 files changed, 140 insertions(+), 176 deletions(-)
diff --git a/.golangci.yaml b/.golangci.yaml
index e9b0045..c64a3b2 100644
--- a/.golangci.yaml
+++ b/.golangci.yaml
@@ -9,4 +9,8 @@ issues:
source: "Log\\("
- linters:
- gomnd
- source: "\\*time.(Second|Minute|Hour)"
\ No newline at end of file
+ source: "\\*time.(Second|Minute|Hour)"
+linters-settings:
+ funlen:
+ statements: 50
+ lines: 70
\ No newline at end of file
diff --git a/auth/auth.go b/auth/auth.go
index 4fc6830..d447e0c 100644
--- a/auth/auth.go
+++ b/auth/auth.go
@@ -57,8 +57,7 @@ func Require(next http.Handler, authFailed http.Handler, provider Provider) http
return
}
- level.Info(log.Get(r)).
- Log("msg", "no session, using anon auth")
+ level.Info(log.Get(r)).Log("msg", "no session, using anon auth")
user := &User{
Name: "anon",
@@ -90,8 +89,7 @@ func Require(next http.Handler, authFailed http.Handler, provider Provider) http
}
// Some error that isn't because the session is expired or doesn't exist (DB?)
- level.Error(log.Get(r)).
- Log("error", err, "msg", "can't authenticate user")
+ level.Error(log.Get(r)).Log("error", err, "msg", "can't authenticate user")
http.Error(w, err.Error(), http.StatusInternalServerError)
return
@@ -99,8 +97,7 @@ func Require(next http.Handler, authFailed http.Handler, provider Provider) http
user.CanWrite = true
- level.Info(log.Get(r)).
- Log("user", *user, "msg", "user authenticated")
+ level.Info(log.Get(r)).Log("user", *user, "msg", "user authenticated")
ctx := context.WithValue(r.Context(), key, *user)
diff --git a/auth/auth_test.go b/auth/auth_test.go
index 1bef78a..a3c6ff5 100644
--- a/auth/auth_test.go
+++ b/auth/auth_test.go
@@ -19,6 +19,7 @@ func (a authProvider) Valid(ctx context.Context, token string) (*User, error) {
return nil, ErrAuthFailed
}
+//nolint:funlen
func TestRequire(t *testing.T) {
var (
ap authProvider
diff --git a/handler-details.go b/handler-details.go
index 0ce2b30..a22c22f 100644
--- a/handler-details.go
+++ b/handler-details.go
@@ -62,31 +62,44 @@ const (
const maxCommentLen = 1024
-func (h Handler) detailsPost(w http.ResponseWriter, r *http.Request) {
- action := r.FormValue("action")
- if strings.HasPrefix(action, "delete-comment-") {
- level.Debug(log.Get(r)).
- Log("action", action, "msg", "handling comment action")
+func (h Handler) deleteComment(w http.ResponseWriter, r *http.Request, action string) {
+ level.Debug(log.Get(r)).
+ Log("action", action, "msg", "handling comment action")
- parts := strings.Split(action, "-")
+ parts := strings.Split(action, "-")
- commentID, err := strconv.Atoi(parts[len(parts)-1])
- if err != nil {
- httpError(w, r, "bad request", err, http.StatusBadRequest)
- return
- }
+ commentID, err := strconv.Atoi(parts[len(parts)-1])
+ if err != nil {
+ httpError(w, r, "bad request", err, http.StatusBadRequest)
+ return
+ }
- err = h.Q.DeleteComment(r.Context(), int32(commentID))
- if err != nil {
- httpError(w, r, "internal server error", err, http.StatusInternalServerError)
- return
- }
+ err = h.Q.DeleteComment(r.Context(), int32(commentID))
+ if err != nil {
+ httpError(w, r, "internal server error", err, http.StatusInternalServerError)
+ return
+ }
+
+ level.Info(log.Get(r)).
+ Log("id", commentID, "msg", "deleted comment")
+
+ http.Redirect(w, r, "/details?id="+r.FormValue("id"), http.StatusSeeOther)
+}
- level.Info(log.Get(r)).
- Log("id", commentID, "msg", "deleted comment")
+func (h Handler) deleteEntry(w http.ResponseWriter, r *http.Request, id int) {
+ err := h.Q.DeleteWine(r.Context(), int32(id))
+ if err != nil {
+ httpError(w, r, "can't delete wine", err, http.StatusInternalServerError)
+ return
+ }
- http.Redirect(w, r, "/details?id="+r.FormValue("id"), http.StatusSeeOther)
+ http.Redirect(w, r, "/", http.StatusSeeOther)
+}
+func (h Handler) detailsPost(w http.ResponseWriter, r *http.Request) {
+ action := r.FormValue("action")
+ if strings.HasPrefix(action, "delete-comment-") {
+ h.deleteComment(w, r, action)
return
}
@@ -106,14 +119,7 @@ func (h Handler) detailsPost(w http.ResponseWriter, r *http.Request) {
}
if action == actionDelete {
- err = h.Q.DeleteWine(r.Context(), int32(id))
- if err != nil {
- httpError(w, r, "can't delete wine", err, http.StatusInternalServerError)
- return
- }
-
- http.Redirect(w, r, "/", http.StatusSeeOther)
-
+ h.deleteEntry(w, r, id)
return
}
@@ -155,8 +161,7 @@ func (h Handler) detailsPost(w http.ResponseWriter, r *http.Request) {
Comment: comment,
})
if err != nil {
- httpError(w, r, "can't add comment", err, http.StatusInternalServerError)
- return err
+ return fmt.Errorf("adding comment: %w", err)
}
}
@@ -164,15 +169,13 @@ func (h Handler) detailsPost(w http.ResponseWriter, r *http.Request) {
if len(r.MultipartForm.File["picture"]) != 0 {
picFile, picHdr, err := r.FormFile("picture")
if err != nil {
- httpError(w, r, "can't open picture file", err, http.StatusInternalServerError)
- return err
+ return fmt.Errorf("opening picture: %w", err)
}
defer picFile.Close()
err = storage.AddPicture(ctx, q, id, picFile, picHdr.Header.Get("Content-Type"))
if err != nil {
- httpError(w, r, "can't store picture", err, http.StatusBadRequest)
- return err
+ return fmt.Errorf("storing picture: %w", err)
}
}
@@ -190,21 +193,19 @@ func (h Handler) detailsPost(w http.ResponseWriter, r *http.Request) {
},
}
- level.Debug(log.Get(r)).Log("update", fmt.Sprint(params))
-
err = q.UpdateWine(ctx, params)
if err != nil {
- httpError(w, r, "can't store update", err, http.StatusInternalServerError)
- return err
+ return fmt.Errorf("updating wine: %w", err)
}
return nil
})
if err != nil {
+ httpError(w, r, "can't store update", err, http.StatusInternalServerError)
return
}
- http.Redirect(w, r, "/details?id="+r.FormValue("id"), http.StatusSeeOther)
+ http.Redirect(w, r, "/details?id="+strconv.Itoa(id), http.StatusSeeOther)
}
func (h Handler) details() http.Handler {
@@ -249,27 +250,24 @@ func (h Handler) details() http.Handler {
err = h.Q.RunTx(r.Context(), func(ctx context.Context, q *query.Queries) error {
data.Wine, err = q.LoadWine(ctx, int32(id))
if err != nil {
- httpError(w, r, "can't load wine data", err, http.StatusInternalServerError)
- return err
+ return fmt.Errorf("loading wine data: %w", err)
}
data.Comments, err = q.ListComments(ctx, int32(id))
if err != nil {
- httpError(w, r, "can't load comments", err, http.StatusInternalServerError)
- return err
+ return fmt.Errorf("loading comments: %w", err)
}
return nil
})
if err != nil {
+ httpError(w, r, "loading data failed", err, http.StatusInternalServerError)
return
}
err = tpl.ExecuteTemplate(w, "details.tpl", data)
if err != nil {
- level.Error(log.Get(r)).
- Log("error", err,
- "msg", "can't execute details template")
+ level.Error(log.Get(r)).Log("error", err, "msg", "can't execute details template")
return
}
diff --git a/handler-index.go b/handler-index.go
index 99bdc27..519297b 100644
--- a/handler-index.go
+++ b/handler-index.go
@@ -4,6 +4,7 @@ import (
"context"
"database/sql"
"errors"
+ "fmt"
"html/template"
"net/http"
"strconv"
@@ -17,45 +18,44 @@ import (
"git.c3pb.de/gbe/invinoveritas/storage/query"
)
+type indexTemplateData struct {
+ Wines []query.ListWinesRow
+ User auth.User
+}
+
+func (h Handler) listWines(w http.ResponseWriter, r *http.Request, tpl *template.Template) {
+ wines, err := h.Q.ListWines(r.Context())
+ if err != nil {
+ httpError(w, r, "can't list wines", err, http.StatusInternalServerError)
+ return
+ }
+
+ w.Header().Add("content-type", "text/html")
+
+ data := indexTemplateData{
+ Wines: wines,
+ User: auth.Get(r),
+ }
+
+ err = tpl.ExecuteTemplate(w, "index.tpl", data)
+ if err != nil {
+ level.Error(log.Get(r)).Log("error", err, "msg", "can't execute index template")
+ }
+}
+
func (h Handler) index() http.Handler {
var (
once sync.Once
tpl *template.Template
)
- type templateData struct {
- Wines []query.ListWinesRow
- User auth.User
- }
-
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
once.Do(func() {
tpl = template.Must(template.ParseFS(templateFS, "templates/base.tpl", "templates/index.tpl"))
})
- level.Debug(log.Get(r)).Log("user", auth.Get(r))
-
if r.Method == "GET" {
- wines, err := h.Q.ListWines(r.Context())
- if err != nil {
- httpError(w, r, "can't list wines", err, http.StatusInternalServerError)
- return
- }
-
- w.Header().Add("content-type", "text/html")
-
- data := templateData{
- Wines: wines,
- User: auth.Get(r),
- }
-
- err = tpl.ExecuteTemplate(w, "index.tpl", data)
- if err != nil {
- level.Error(log.Get(r)).
- Log("error", err,
- "msg", "can't execute index template")
- }
-
+ h.listWines(w, r, tpl)
return
}
@@ -104,20 +104,14 @@ func (h Handler) index() http.Handler {
res, err := q.InsertWine(ctx, params)
if err != nil {
- httpError(w, r, "can't store wine", err, http.StatusInternalServerError)
-
- return err
+ return fmt.Errorf("storing wine: %w", err)
}
id, err = res.LastInsertId()
if err != nil {
- httpError(w, r, "can't store wine", err, http.StatusInternalServerError)
-
- return err
+ return fmt.Errorf("storing wine: %w", err)
}
- level.Debug(log.Get(r)).Log("msg", "inserted new wine", "id", id)
-
// See if there's an image file uploaded and persist it if that's the case.
if r.MultipartForm == nil || len(r.MultipartForm.File["picture"]) == 0 {
return nil
@@ -125,20 +119,19 @@ func (h Handler) index() http.Handler {
picFile, picHdr, err := r.FormFile("picture")
if err != nil {
- httpError(w, r, "can't open picture file", err, http.StatusInternalServerError)
- return err
+ return fmt.Errorf("loading picture data: %w", err)
}
defer picFile.Close()
err = storage.AddPicture(ctx, q, int(id), picFile, picHdr.Header.Get("Content-Type"))
if err != nil {
- httpError(w, r, "can't load picture", err, http.StatusBadRequest)
- return err
+ return fmt.Errorf("storing picture: %w", err)
}
return nil
})
if err != nil {
+ httpError(w, r, "updating data failed", err, http.StatusInternalServerError)
return
}
diff --git a/handler-user.go b/handler-user.go
index a0442e3..1879b19 100644
--- a/handler-user.go
+++ b/handler-user.go
@@ -10,6 +10,7 @@ import (
"git.c3pb.de/gbe/invinoveritas/auth"
"git.c3pb.de/gbe/invinoveritas/log"
"git.c3pb.de/gbe/invinoveritas/session"
+ "github.com/go-kit/kit/log/level"
)
const maxUserNameLen = 80
@@ -205,6 +206,7 @@ func (h Handler) user(page pageName) http.Handler {
err = h.up.UpdatePassword(r.Context(), user.Name, r.PostForm.Get("old-pw"), r.PostForm.Get("new-pw"), false)
if err != nil {
+ level.Error(log.GetContext(r.Context())).Log("msg", "user update failed", "error", err)
data.FormErrors["OldPW"] = err
return
}
diff --git a/session/authprovider.go b/session/authprovider.go
index 9fe93a1..ee03304 100644
--- a/session/authprovider.go
+++ b/session/authprovider.go
@@ -128,6 +128,7 @@ func (a Provider) Valid(ctx context.Context, token string) (*auth.User, error) {
// Handler returns an HTTP handler that can be used to ask a user to authenticate themselves. It loads templates
// from the given FS.
+//nolint:funlen
func (a Provider) Handler(templateFS fs.FS) http.Handler {
var (
once sync.Once
@@ -138,16 +139,30 @@ func (a Provider) Handler(templateFS fs.FS) http.Handler {
Error string
}
+ respondUnauthorized := func(w http.ResponseWriter) error {
+ w.WriteHeader(http.StatusUnauthorized)
+
+ rd := responseData{
+ Error: "authentication failed",
+ }
+
+ err := tpl.ExecuteTemplate(w, "auth.tpl", rd)
+ if err != nil {
+ return fmt.Errorf("executing auth template: %w", err)
+ }
+
+ return auth.ErrAuthFailed
+ }
+
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
once.Do(func() {
tpl = template.Must(template.ParseFS(templateFS, "templates/base.tpl", "templates/auth.tpl"))
})
- if r.Method == "GET" {
+ if r.Method == http.MethodGet {
err := tpl.ExecuteTemplate(w, "auth.tpl", nil)
if err != nil {
- level.Error(log.Get(r)).
- Log("error", err, "msg", "can't execute auth template")
+ level.Error(log.Get(r)).Log("error", err, "msg", "can't execute auth template")
}
return
@@ -159,38 +174,15 @@ func (a Provider) Handler(templateFS fs.FS) http.Handler {
log := kitlog.With(log.Get(r), "user", userName)
- level.Debug(log).
- Log("pass", password, "debug", "attempting auth")
-
var token string
err := a.q.RunTx(r.Context(), func(ctx context.Context, q *query.Queries) error {
userData, err := q.GetAuthData(ctx, userName)
if errors.Is(err, sql.ErrNoRows) {
- // Either no such user or no users at all
- count, err := q.CountUsers(ctx)
- if err != nil {
- http.Error(w, err.Error(), http.StatusInternalServerError)
- return fmt.Errorf("getting user count: %w", err)
- }
-
- level.Debug(log).
- Log("num_users", count, "msg", "got user count")
-
- w.WriteHeader(http.StatusUnauthorized)
-
- rd := responseData{
- Error: "authentication failed",
- }
-
- err = tpl.ExecuteTemplate(w, "auth.tpl", rd)
- if err != nil {
- return fmt.Errorf("executing auth template: %w", err)
- }
-
- return auth.ErrAuthFailed
+ return respondUnauthorized(w)
}
if err != nil {
+ http.Error(w, err.Error(), http.StatusInternalServerError)
return fmt.Errorf("getting salted password: %w", err)
}
@@ -201,29 +193,13 @@ func (a Provider) Handler(templateFS fs.FS) http.Handler {
}
if hashed != userData.Password {
- level.Error(log).
- Log("msg", "password mismatch")
-
- w.WriteHeader(http.StatusUnauthorized)
-
- rd := responseData{
- Error: "authentication failed",
- }
+ level.Error(log).Log("msg", "password mismatch")
- err := tpl.ExecuteTemplate(w, "auth.tpl", rd)
- if err != nil {
- return fmt.Errorf("executing auth template: %w", err)
- }
-
- return auth.ErrAuthFailed
+ return respondUnauthorized(w)
}
log = kitlog.With(log, "user_id", userData.UserID)
- // All good
- level.Info(log).
- Log("msg", "auth looks good")
-
addr := r.Header.Get("X-Forwarded-For")
if addr == "" {
addr = r.RemoteAddr
@@ -251,9 +227,6 @@ func (a Provider) Handler(templateFS fs.FS) http.Handler {
HttpOnly: true, // Might need to be unset if we do a REST-style API
}
- level.Debug(log).
- Log("cookie", fmt.Sprintf("%#v", cookie), "msg", "setting token cookie")
-
http.SetCookie(w, &cookie)
// Redirect to initially requested URL
@@ -321,16 +294,13 @@ func (a Provider) ListUsers(ctx context.Context) ([]string, error) {
}
func (a Provider) UpdatePassword(ctx context.Context, userName, passOld, passNew string, createNew bool) error {
- err := a.q.RunTx(ctx, func(ctx context.Context, q *query.Queries) error {
+ return a.q.RunTx(ctx, func(ctx context.Context, q *query.Queries) error {
hashedNew, err := hashPassword(ctx, q, userName, passNew)
if err != nil {
return err
}
if createNew {
- level.Info(log.GetContext(ctx)).
- Log("name", userName, "msg", "creating new user")
-
err = q.InsertUser(ctx, query.InsertUserParams{
Name: userName,
Password: hashedNew,
@@ -383,16 +353,11 @@ func (a Provider) UpdatePassword(ctx context.Context, userName, passOld, passNew
return nil
})
- if err != nil {
- level.Error(log.GetContext(ctx)).
- Log("msg", "user update failed", "error", err)
- return err
- }
-
- return nil
}
func (a Provider) CreateUser(ctx context.Context, userName string) (string, error) {
+ level.Info(log.GetContext(ctx)).Log("name", userName, "msg", "creating new user")
+
// generate random password
const chars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
diff --git a/storage/storage.go b/storage/storage.go
index 3d8451c..5faa733 100644
--- a/storage/storage.go
+++ b/storage/storage.go
@@ -76,44 +76,25 @@ func AddPicture(ctx context.Context, q *query.Queries, wineID int, fh io.Reader,
//go:embed migrations/*.sql
var migrationFS embed.FS //nolint:gochecknoglobals
-func Open(ctx context.Context, dbPath, dumpPath string, logger kitlog.Logger) (*sql.DB, error) {
- db, err := sql.Open("sqlite", dbPath)
- if err != nil {
- return nil, err
- }
-
- level.Debug(logger).Log("db_path", dbPath)
-
- pragmas := []string{
- "PRAGMA busy_timeout = 600000",
- "PRAGMA journal_mode = WAL",
- }
-
- for _, p := range pragmas {
- _, err = db.ExecContext(ctx, p)
- if err != nil {
- return nil, fmt.Errorf("running %q: %w", p, err)
- }
- }
-
+func migrateDB(ctx context.Context, db *sql.DB) error {
// Make sure that we have a table for the migrations, the rest runs in a transaction.
- _, err = db.ExecContext(ctx, `CREATE TABLE IF NOT EXISTS migrations (name text, unique(name))`)
+ _, err := db.ExecContext(ctx, `CREATE TABLE IF NOT EXISTS migrations (name text, unique(name))`)
if err != nil {
- return nil, fmt.Errorf("creating migrations table: %w", err)
+ return fmt.Errorf("creating migrations table: %w", err)
}
q := query.New(db)
entries, err := migrationFS.ReadDir("migrations")
if err != nil {
- return nil, fmt.Errorf("reading embedded migration FS: %w", err)
+ return fmt.Errorf("reading embedded migration FS: %w", err)
}
sort.Slice(entries, func(i, j int) bool {
return entries[i].Name() < entries[j].Name()
})
- err = q.RunTx(ctx, func(ctx context.Context, q *query.Queries) error {
+ return q.RunTx(ctx, func(ctx context.Context, q *query.Queries) error {
for _, e := range entries {
// Check if we need to apply that migration
count, err := q.CountMigrations(ctx, e.Name())
@@ -148,8 +129,31 @@ func Open(ctx context.Context, dbPath, dumpPath string, logger kitlog.Logger) (*
return nil
})
+}
+
+func Open(ctx context.Context, dbPath, dumpPath string, logger kitlog.Logger) (*sql.DB, error) {
+ db, err := sql.Open("sqlite", dbPath)
+ if err != nil {
+ return nil, err
+ }
+
+ level.Debug(logger).Log("db_path", dbPath)
+
+ pragmas := []string{
+ "PRAGMA busy_timeout = 600000",
+ "PRAGMA journal_mode = WAL",
+ }
+
+ for _, p := range pragmas {
+ _, err = db.ExecContext(ctx, p)
+ if err != nil {
+ return nil, fmt.Errorf("running %q: %w", p, err)
+ }
+ }
+
+ err = migrateDB(ctx, db)
if err != nil {
- return nil, fmt.Errorf("applying migrations: %w", err)
+ return nil, fmt.Errorf("migrating db: %w", err)
}
return db, nil
--
GitLab