Don't expose user hashed password (#2419)

controllers/user.go

@@ -19,6 +19,9 @@ var (
 	upgrader = websocket.Upgrader{}
 )
 
+// verifyJWT makes logic.VerifyJWT fakeable/mockable in tests
+var verifyJWT = logic.VerifyJWT
+
 func userHandlers(r *mux.Router) {
 
 	r.HandleFunc("/api/users/adm/hasadmin", hasAdmin).Methods(http.MethodGet)
@@ -152,7 +155,7 @@ func getUser(w http.ResponseWriter, r *http.Request) {
 
 	var params = mux.Vars(r)
 	usernameFetched := params["username"]
-	user, err := logic.GetUser(usernameFetched)
+	user, err := logic.GetReturnUser(usernameFetched)
 
 	if err != nil {
 		logger.Log(0, usernameFetched, "failed to fetch user: ", err.Error())
@@ -230,7 +233,7 @@ func createAdmin(w http.ResponseWriter, r *http.Request) {
 	}
 
 	logger.Log(1, admin.UserName, "was made a new admin")
-	json.NewEncoder(w).Encode(admin)
+	json.NewEncoder(w).Encode(logic.ToReturnUser(admin))
 }
 
 // swagger:route POST /api/users/{username} user createUser
@@ -264,7 +267,7 @@ func createUser(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	logger.Log(1, user.UserName, "was created")
-	json.NewEncoder(w).Encode(user)
+	json.NewEncoder(w).Encode(logic.ToReturnUser(user))
 }
 
 // swagger:route PUT /api/users/networks/{username} user updateUserNetworks
@@ -314,12 +317,13 @@ func updateUserNetworks(w http.ResponseWriter, r *http.Request) {
 	}
 	logger.Log(1, username, "status was updated")
 	// re-read and return the new user struct
-	if userChange, err = logic.GetUser(username); err != nil {
+	returnUser, err := logic.GetReturnUser(username)
+	if err != nil {
 		logger.Log(0, username, "failed to fetch user: ", err.Error())
 		logic.ReturnErrorResponse(w, r, logic.FormatError(err, "internal"))
 		return
 	}
-	json.NewEncoder(w).Encode(userChange)
+	json.NewEncoder(w).Encode(returnUser)
 }
 
 // swagger:route PUT /api/users/{username} user updateUser
@@ -337,7 +341,7 @@ func updateUser(w http.ResponseWriter, r *http.Request) {
 	w.Header().Set("Content-Type", "application/json")
 	var params = mux.Vars(r)
 	// start here
-	jwtUser, _, isadmin, err := logic.VerifyJWT(r.Header.Get("Authorization"))
+	jwtUser, _, isadmin, err := verifyJWT(r.Header.Get("Authorization"))
 	if err != nil {
 		logger.Log(0, "verifyJWT error", err.Error())
 		logic.ReturnErrorResponse(w, r, logic.FormatError(err, "internal"))
@@ -385,7 +389,7 @@ func updateUser(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	logger.Log(1, username, "was updated")
-	json.NewEncoder(w).Encode(user)
+	json.NewEncoder(w).Encode(logic.ToReturnUser(*user))
 }
 
 // swagger:route PUT /api/users/{username}/adm user updateUserAdm
@@ -409,7 +413,7 @@ func updateUserAdm(w http.ResponseWriter, r *http.Request) {
 		logic.ReturnErrorResponse(w, r, logic.FormatError(err, "internal"))
 		return
 	}
-	if auth.IsOauthUser(user) != nil {
+	if auth.IsOauthUser(user) == nil {
 		err := fmt.Errorf("cannot update user info for oauth user %s", username)
 		logger.Log(0, err.Error())
 		logic.ReturnErrorResponse(w, r, logic.FormatError(err, "forbidden"))
@@ -436,7 +440,7 @@ func updateUserAdm(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	logger.Log(1, username, "was updated (admin)")
-	json.NewEncoder(w).Encode(user)
+	json.NewEncoder(w).Encode(logic.ToReturnUser(*user))
 }
 
 // swagger:route DELETE /api/users/{username} user deleteUser

controllers/user_test.go

@@ -1,6 +1,12 @@
 package controller
 
 import (
+	"bytes"
+	"github.com/go-jose/go-jose/v3/json"
+	"github.com/gorilla/mux"
+	"io"
+	"net/http"
+	"net/http/httptest"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -19,6 +25,123 @@ func deleteAllUsers(t *testing.T) {
 	}
 }
 
+func TestGetUserNoHashedPassword(t *testing.T) {
+	// prepare existing user base
+	user := models.User{UserName: "freddie", Password: "password"}
+	haveOnlyOneUser(t, user)
+
+	// prepare request
+	rec, req := prepareUserRequest(t, models.User{}, user.UserName)
+
+	// test response
+	getUser(rec, req)
+	assertUserNameButNoPassword(t, rec.Body, user.UserName)
+}
+
+func TestCreateAdminNoHashedPassword(t *testing.T) {
+	// prepare existing user base
+	deleteAllUsers(t)
+
+	// prepare request
+	user := models.User{UserName: "jonathan", Password: "password"}
+	rec, req := prepareUserRequest(t, user, "")
+
+	// test response
+	createAdmin(rec, req)
+	assertUserNameButNoPassword(t, rec.Body, user.UserName)
+}
+
+func TestCreateUserNoHashedPassword(t *testing.T) {
+	// prepare existing user base
+	deleteAllUsers(t)
+
+	// prepare request
+	user := models.User{UserName: "jonathan", Password: "password"}
+	rec, req := prepareUserRequest(t, user, "")
+
+	// test response
+	createUser(rec, req)
+	assertUserNameButNoPassword(t, rec.Body, user.UserName)
+}
+
+func TestUpdateUserNetworksNoHashedPassword(t *testing.T) {
+	// prepare existing user base
+	user1 := models.User{UserName: "joestar", Password: "jonathan"}
+	haveOnlyOneUser(t, user1)
+
+	// prepare request
+	user2 := models.User{UserName: "joestar", Password: "joseph"}
+	rec, req := prepareUserRequest(t, user2, user1.UserName)
+
+	// test response
+	updateUserNetworks(rec, req)
+	assertUserNameButNoPassword(t, rec.Body, user1.UserName)
+}
+
+func TestUpdateUserNoHashedPassword(t *testing.T) {
+	// prepare existing user base
+	user1 := models.User{UserName: "dio", Password: "brando"}
+	haveOnlyOneUser(t, user1)
+
+	// prepare request
+	user2 := models.User{UserName: "giorno", Password: "giovanna"}
+	rec, req := prepareUserRequest(t, user2, user1.UserName)
+
+	// mock the jwt verification
+	oldVerify := verifyJWT
+	verifyJWT = func(bearerToken string) (username string, networks []string, isadmin bool, err error) {
+		return user1.UserName, user1.Networks, user1.IsAdmin, nil
+	}
+	defer func() { verifyJWT = oldVerify }()
+
+	// test response
+	updateUser(rec, req)
+	assertUserNameButNoPassword(t, rec.Body, user2.UserName)
+}
+
+func TestUpdateUserAdmNoHashedPassword(t *testing.T) {
+	// prepare existing user base
+	user1 := models.User{UserName: "dio", Password: "brando", IsAdmin: true}
+	haveOnlyOneUser(t, user1)
+
+	// prepare request
+	user2 := models.User{UserName: "giorno", Password: "giovanna"}
+	rec, req := prepareUserRequest(t, user2, user1.UserName)
+
+	// test response
+	updateUserAdm(rec, req)
+	assertUserNameButNoPassword(t, rec.Body, user2.UserName)
+}
+
+func prepareUserRequest(t *testing.T, userForBody models.User, userNameForParam string) (*httptest.ResponseRecorder, *http.Request) {
+	bits, err := json.Marshal(userForBody)
+	assert.Nil(t, err)
+	body := bytes.NewReader(bits)
+	rec := httptest.NewRecorder()
+	req := httptest.NewRequest("ANY", "https://example.com", body) // only the body matters here
+	req = mux.SetURLVars(req, map[string]string{"username": userNameForParam})
+	return rec, req
+}
+
+func haveOnlyOneUser(t *testing.T, user models.User) {
+	deleteAllUsers(t)
+	var err error
+	if user.IsAdmin {
+		err = logic.CreateAdmin(&user)
+	} else {
+		err = logic.CreateUser(&user)
+	}
+	assert.Nil(t, err)
+}
+
+func assertUserNameButNoPassword(t *testing.T, r io.Reader, userName string) {
+	var resp models.User
+	err := json.NewDecoder(r).Decode(&resp)
+	assert.Nil(t, err)
+	assert.Equal(t, userName, resp.UserName)
+	assert.Empty(t, resp.Password)
+}
+
 func TestHasAdmin(t *testing.T) {
 	// delete all current users
 	users, _ := logic.GetUsers()

logic/auth.go

@@ -42,20 +42,6 @@ func HasAdmin() (bool, error) {
 	return false, err
 }
 
-// GetReturnUser - gets a user
-func GetReturnUser(username string) (models.ReturnUser, error) {
-
-	var user models.ReturnUser
-	record, err := database.FetchRecord(database.USERS_TABLE_NAME, username)
-	if err != nil {
-		return user, err
-	}
-	if err = json.Unmarshal([]byte(record), &user); err != nil {
-		return models.ReturnUser{}, err
-	}
-	return user, err
-}
-
 // GetUsers - gets users
 func GetUsers() ([]models.ReturnUser, error) {
 

logic/users.go

@@ -26,6 +26,30 @@ func GetUser(username string) (*models.User, error) {
 	return &user, err
 }
 
+// GetReturnUser - gets a user
+func GetReturnUser(username string) (models.ReturnUser, error) {
+
+	var user models.ReturnUser
+	record, err := database.FetchRecord(database.USERS_TABLE_NAME, username)
+	if err != nil {
+		return user, err
+	}
+	if err = json.Unmarshal([]byte(record), &user); err != nil {
+		return models.ReturnUser{}, err
+	}
+	return user, err
+}
+
+// ToReturnUser - gets a user as a return user
+func ToReturnUser(user models.User) models.ReturnUser {
+	return models.ReturnUser{
+		UserName: user.UserName,
+		Networks: user.Networks,
+		IsAdmin:  user.IsAdmin,
+		Groups:   user.Groups,
+	}
+}
+
 // GetGroupUsers - gets users in a group
 func GetGroupUsers(group string) ([]models.ReturnUser, error) {
 	var returnUsers []models.ReturnUser