From 0ff55b0fccfd4b458f54aff233321b73dcdf4df8 Mon Sep 17 00:00:00 2001
From: jmorganca <jmorganca@gmail.com>
Date: Thu, 27 May 2021 11:30:19 -0400
Subject: [PATCH] - Smarter error checking on database - Add unique constraint
 to email field on user model - Validate token expiry on auth check

---
 internal/server/data.go     | 18 +++++++--------
 internal/server/handlers.go | 44 ++++++++++++++++++++++++-------------
 2 files changed, 38 insertions(+), 24 deletions(-)

diff --git a/internal/server/data.go b/internal/server/data.go
index c265346fa6..cfc54d2fa1 100644
--- a/internal/server/data.go
+++ b/internal/server/data.go
@@ -10,7 +10,7 @@ import (
 
 type User struct {
 	ID       uint   `gorm:"primaryKey"`
-	Email    string `json:"email"`
+	Email    string `json:"email" gorm:"unique"`
 	Password []byte `json:"-"`
 	Provider string `json:"provider"`
 	Created  int64  `json:"created" gorm:"autoCreateTime"`
@@ -49,13 +49,13 @@ func SyncUsers(db *gorm.DB, emails []string, provider string) error {
 			user.Email = email
 			user.Provider = "okta"
 
-			if result := tx.Save(&user); result.Error != nil {
-				return result.Error
+			if err := tx.Save(&user).Error; err != nil {
+				return err
 			}
 
 			var users []User
-			if result := tx.Find(&users); result.Error != nil {
-				return result.Error
+			if err := tx.Find(&users).Error; err != nil {
+				return err
 			}
 
 			emailsMap := make(map[string]bool)
@@ -69,13 +69,13 @@ func SyncUsers(db *gorm.DB, emails []string, provider string) error {
 					// Only delete user if they don't have built in auth
 					// TODO (jmorganca): properly refactor this into a provider check
 					if user.Provider == "okta" && len(user.Password) == 0 {
-						if result := tx.Delete(&user); result.Error != nil {
-							return result.Error
+						if err := tx.Delete(&user).Error; err != nil {
+							return err
 						}
 					} else {
 						user.Provider = "infra"
-						if result := tx.Save(&user); result.Error != nil {
-							return result.Error
+						if err := tx.Save(&user).Error; err != nil {
+							return err
 						}
 					}
 				}
diff --git a/internal/server/handlers.go b/internal/server/handlers.go
index 9bd130ce0c..4fa18f46f5 100644
--- a/internal/server/handlers.go
+++ b/internal/server/handlers.go
@@ -3,6 +3,7 @@ package server
 import (
 	"crypto/tls"
 	"crypto/x509"
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"net/http"
@@ -45,8 +46,22 @@ func TokenAuthMiddleware(secret []byte) gin.HandlerFunc {
 			return
 		}
 
+		cl := jwt.Claims{}
 		out := make(map[string]interface{})
-		if err := tok.Claims(secret, &out); err != nil {
+		if err := tok.Claims(secret, &cl, &out); err != nil {
+			c.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{"error": "unauthorized"})
+			return
+		}
+
+		err = cl.Validate(jwt.Expected{
+			Issuer: "infra",
+			Time:   time.Now(),
+		})
+		switch {
+		case errors.Is(err, jwt.ErrExpired):
+			c.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{"error": "expired"})
+			return
+		case err != nil:
 			c.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{"error": "unauthorized"})
 			return
 		}
@@ -223,13 +238,13 @@ func addRoutes(router *gin.Engine, db *gorm.DB, kube *Kubernetes, cfg *Config, s
 		}
 
 		var user User
-		if result := db.Where("email = ?", emailParams.Email).First(&user); result.Error != nil {
-			c.JSON(http.StatusBadRequest, gin.H{"error": "incorrect credentials"})
+		if err := db.Where("email = ?", emailParams.Email).First(&user).Error; err != nil {
+			c.JSON(http.StatusBadRequest, gin.H{"error": "unauthorized"})
 			return
 		}
 
-		if err = bcrypt.CompareHashAndPassword(user.Password, []byte(params.Password)); err != nil {
-			c.JSON(http.StatusBadRequest, gin.H{"error": "incorrect credentials"})
+		if err = bcrypt.CompareHashAndPassword(user.Password, []byte(emailParams.Password)); err != nil {
+			c.JSON(http.StatusBadRequest, gin.H{"error": "unauthorized"})
 			return
 		}
 
@@ -246,9 +261,8 @@ func addRoutes(router *gin.Engine, db *gorm.DB, kube *Kubernetes, cfg *Config, s
 
 	router.GET("/v1/users", TokenAuthMiddleware(settings.TokenSecret), PermissionMiddleware("view", cfg), func(c *gin.Context) {
 		var users []User
-		result := db.Find(&users)
-
-		if result.Error != nil {
+		err := db.Find(&users).Error
+		if err != nil {
 			c.JSON(http.StatusInternalServerError, ErrorResponse{"could not list users"})
 			return
 		}
@@ -284,8 +298,8 @@ func addRoutes(router *gin.Engine, db *gorm.DB, kube *Kubernetes, cfg *Config, s
 		user.Password = hashedPassword
 		user.Provider = "infra"
 
-		result := db.Create(&user)
-		if result.Error != nil {
+		err = db.Create(&user).Error
+		if err != nil {
 			c.JSON(http.StatusInternalServerError, ErrorResponse{"could not create user"})
 			return
 		}
@@ -313,14 +327,14 @@ func addRoutes(router *gin.Engine, db *gorm.DB, kube *Kubernetes, cfg *Config, s
 			return
 		}
 
-		result := db.Where("email = ?", params.Email).Delete(User{})
-		if result.Error != nil {
-			c.JSON(http.StatusInternalServerError, ErrorResponse{"could not delete user"})
+		err := db.Where("email = ?", params.Email).Delete(User{}).Error
+		if errors.Is(err, gorm.ErrRecordNotFound) {
+			c.JSON(http.StatusBadRequest, ErrorResponse{"user does not exist"})
 			return
 		}
 
-		if result.RowsAffected == 0 {
-			c.JSON(http.StatusBadRequest, ErrorResponse{"user does not exist"})
+		if err != nil {
+			c.JSON(http.StatusInternalServerError, ErrorResponse{"could not delete user"})
 			return
 		}