Repository: mainflux/mainflux (go)

Issue:
Minimal password length limitation
<!--
The GitHub issue tracker is for bug reports and feature requests. General support can be found at
the following locations:

- Google group - https://groups.google.com/forum/#!forum/mainflux
- Gitter - https://gitter.im/mainflux/mainflux
-->

**ENHANCEMENT**

Password validation currently includes limitation of password to minimally 8 characters: https://github.com/mainflux/mainflux/search?q=minPassLen

This decision looks somewhat arbitrary, and it is certainly not documented or stressed enough, so it can lead to errors that are not intuitive to the user.

Password validation should avoid arbitrary limitation (for example 8 chars - why 8?), and be:
- Generic
- Configurable

This can eventually be achieved via user-configurable regexp in an ENV var that will be defined, per deployment, in a deploy-time.

If this turns out to be too complicated, consider removing limitation or at least document it well.

2. Indicate the importance of this enhancement to you (must-have, should-have, nice-to-have).
Should-have

Pull request description:
MF-1317 - Configurable regexp rule for password
Signed-off-by: Ivan Milosevic <iva@blokovi.com>


### Which issue(s) does this PR fix/relate to?
Resolves #1317 

### List any changes that modify/break current functionality

### Have you included tests for your changes?

### Did you document any new/modified functionality?

### Notes


Patch:
diff --git a/.env b/.env
index 8162f629..785ddba8 100644
--- a/.env
+++ b/.env
@@ -46,6 +46,7 @@ MF_USERS_DB=users
 MF_USERS_ADMIN_EMAIL=admin@example.com
 MF_USERS_ADMIN_PASSWORD=12345678
 MF_USERS_RESET_PWD_TEMPLATE=users.tmpl
+MF_USERS_PASS_REGEX=^.{8,}$
 
 ### Email utility
 MF_EMAIL_HOST=smtp.mailtrap.io
diff --git a/cmd/users/main.go b/cmd/users/main.go
index cbe8a33c..ddb48e93 100644
--- a/cmd/users/main.go
+++ b/cmd/users/main.go
@@ -12,6 +12,7 @@ import (
 	"net/http"
 	"os"
 	"os/signal"
+	"regexp"
 	"strconv"
 	"syscall"
 	"time"
@@ -63,6 +64,7 @@ const (
 	defEmailTemplate    = "email.tmpl"
 	defAdminEmail       = ""
 	defAdminPassword    = ""
+	defPassRegex        = "^.{8,}$"
 	defAdminGroup       = "mainflux"
 
 	defTokenResetEndpoint = "/reset-request" // URL where user lands after click on the reset link from email
@@ -89,6 +91,7 @@ const (
 
 	envAdminEmail    = "MF_USERS_ADMIN_EMAIL"
 	envAdminPassword = "MF_USERS_ADMIN_PASSWORD"
+	envPassRegex     = "MF_USERS_PASS_REGEX"
 
 	envEmailHost        = "MF_EMAIL_HOST"
 	envEmailPort        = "MF_EMAIL_PORT"
@@ -123,6 +126,7 @@ type config struct {
 	authTimeout   time.Duration
 	adminEmail    string
 	adminPassword string
+	passRegex     *regexp.Regexp
 }
 
 func main() {
@@ -175,6 +179,11 @@ func loadConfig() config {
 		log.Fatalf("Invalid value passed for %s\n", envAuthTLS)
 	}
 
+	passRegex, err := regexp.Compile(mainflux.Env(envPassRegex, defPassRegex))
+	if err != nil {
+		log.Fatalf("Invalid password validation rules %s\n", envPassRegex)
+	}
+
 	dbConfig := postgres.Config{
 		Host:        mainflux.Env(envDBHost, defDBHost),
 		Port:        mainflux.Env(envDBPort, defDBPort),
@@ -213,6 +222,7 @@ func loadConfig() config {
 		authTimeout:   authTimeout,
 		adminEmail:    mainflux.Env(envAdminEmail, defAdminEmail),
 		adminPassword: mainflux.Env(envAdminPassword, defAdminPassword),
+		passRegex:     passRegex,
 	}
 
 }
@@ -287,7 +297,7 @@ func newService(db *sqlx.DB, tracer opentracing.Tracer, auth mainflux.AuthServic
 
 	idProvider := uuid.New()
 
-	svc := users.New(userRepo, groupRepo, hasher, auth, emailer, idProvider)
+	svc := users.New(userRepo, groupRepo, hasher, auth, emailer, idProvider, c.passRegex)
 	svc = api.LoggingMiddleware(svc, logger)
 	svc = api.MetricsMiddleware(
 		svc,
diff --git a/scripts/run.sh b/scripts/run.sh
index 9ffc7799..666920cd 100755
--- a/scripts/run.sh
+++ b/scripts/run.sh
@@ -38,7 +38,7 @@ done
 ###
 # Users
 ###
-MF_USERS_LOG_LEVEL=info MF_EMAIL_TEMPLATE=../docker/users/emailer/templates/email.tmpl $BUILD_DIR/mainflux-users &
+MF_USERS_LOG_LEVEL=info MF_EMAIL_TEMPLATE=../docker/templates/users.tmpl $BUILD_DIR/mainflux-users &
 
 ###
 # Things
diff --git a/users/api/requests.go b/users/api/requests.go
index 280972f1..59ca18c1 100644
--- a/users/api/requests.go
+++ b/users/api/requests.go
@@ -8,7 +8,6 @@ import (
 )
 
 const (
-	minPassLen  = 8
 	maxNameSize = 1024
 )
 
@@ -100,11 +99,8 @@ func (req passwChangeReq) validate() error {
 	if req.Token == "" {
 		return users.ErrUnauthorizedAccess
 	}
-	if len(req.Password) < minPassLen {
-		return users.ErrMalformedEntity
-	}
 	if req.OldPassword == "" {
-		return users.ErrUnauthorizedAccess
+		return users.ErrMalformedEntity
 	}
 	return nil
 }
diff --git a/users/api/transport.go b/users/api/transport.go
index 086ee554..53acc6ed 100644
--- a/users/api/transport.go
+++ b/users/api/transport.go
@@ -432,6 +432,8 @@ func encodeError(_ context.Context, err error, w http.ResponseWriter) {
 			w.WriteHeader(http.StatusBadRequest)
 		case errors.Contains(errorVal, users.ErrRecoveryToken):
 			w.WriteHeader(http.StatusNotFound)
+		case errors.Contains(errorVal, users.ErrPasswordFormat):
+			w.WriteHeader(http.StatusBadRequest)
 		default:
 			w.WriteHeader(http.StatusInternalServerError)
 		}
diff --git a/users/service.go b/users/service.go
index 61c59b6d..dac7adfd 100644
--- a/users/service.go
+++ b/users/service.go
@@ -66,6 +66,9 @@ var (
 
 	// ErrAssignUserToGroup indicates an error in assigning user to a group.
 	ErrAssignUserToGroup = errors.New("failed assigning user to a group")
+
+	// ErrPasswordFormat indicates weak password.
+	ErrPasswordFormat = errors.New("password does not meet the requirements")
 )
 
 // Service specifies an API that must be fullfiled by the domain service
@@ -164,10 +167,11 @@ type usersService struct {
 	email      Emailer
 	auth       mainflux.AuthServiceClient
 	idProvider mainflux.IDProvider
+	passRegex  *regexp.Regexp
 }
 
 // New instantiates the users service implementation
-func New(users UserRepository, groups GroupRepository, hasher Hasher, auth mainflux.AuthServiceClient, m Emailer, idp mainflux.IDProvider) Service {
+func New(users UserRepository, groups GroupRepository, hasher Hasher, auth mainflux.AuthServiceClient, m Emailer, idp mainflux.IDProvider, passRegex *regexp.Regexp) Service {
 	return &usersService{
 		users:      users,
 		groups:     groups,
@@ -175,6 +179,7 @@ func New(users UserRepository, groups GroupRepository, hasher Hasher, auth mainf
 		auth:       auth,
 		email:      m,
 		idProvider: idp,
+		passRegex:  passRegex,
 	}
 }
 
@@ -182,6 +187,9 @@ func (svc usersService) Register(ctx context.Context, user User) (string, error)
 	if err := user.Validate(); err != nil {
 		return "", err
 	}
+	if !svc.passRegex.MatchString(user.Password) {
+		return "", ErrPasswordFormat
+	}
 	hash, err := svc.hasher.Hash(user.Password)
 	if err != nil {
 		return "", errors.Wrap(ErrMalformedEntity, err)
@@ -290,6 +298,9 @@ func (svc usersService) ResetPassword(ctx context.Context, resetToken, password
 	if err != nil || u.Email == "" {
 		return ErrUserNotFound
 	}
+	if !svc.passRegex.MatchString(password) {
+		return ErrPasswordFormat
+	}
 	password, err = svc.hasher.Hash(password)
 	if err != nil {
 		return err
@@ -302,6 +313,9 @@ func (svc usersService) ChangePassword(ctx context.Context, authToken, password,
 	if err != nil {
 		return errors.Wrap(ErrUnauthorizedAccess, err)
 	}
+	if !svc.passRegex.MatchString(password) {
+		return ErrPasswordFormat
+	}
 	u := User{
 		Email:    email,
 		Password: oldPassword,
diff --git a/users/users.go b/users/users.go
index 7b90ace7..d7dedd94 100644
--- a/users/users.go
+++ b/users/users.go
@@ -13,7 +13,6 @@ import (
 )
 
 const (
-	minPassLen   = 8
 	maxLocalLen  = 64
 	maxDomainLen = 255
 	maxTLDLen    = 24 // longest TLD currently in existence
@@ -46,11 +45,6 @@ func (u User) Validate() error {
 	if !isEmail(u.Email) {
 		return ErrMalformedEntity
 	}
-
-	if len(u.Password) < minPassLen {
-		return ErrMalformedEntity
-	}
-
 	return nil
 }
 


Test patch:
diff --git a/pkg/sdk/go/users_test.go b/pkg/sdk/go/users_test.go
index 377dce95..c99a7498 100644
--- a/pkg/sdk/go/users_test.go
+++ b/pkg/sdk/go/users_test.go
@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"net/http"
 	"net/http/httptest"
+	"regexp"
 	"testing"
 
 	"github.com/mainflux/mainflux"
@@ -24,6 +25,10 @@ const (
 	invalidEmail = "userexample.com"
 )
 
+var (
+	passRegex = regexp.MustCompile("^.{8,}$")
+)
+
 func newUserService() users.Service {
 	usersRepo := mocks.NewUserRepository()
 	groupsRepo := mocks.NewGroupRepository()
@@ -32,7 +37,7 @@ func newUserService() users.Service {
 	emailer := mocks.NewEmailer()
 	idProvider := uuid.New()
 
-	return users.New(usersRepo, groupsRepo, hasher, auth, emailer, idProvider)
+	return users.New(usersRepo, groupsRepo, hasher, auth, emailer, idProvider, passRegex)
 }
 
 func newUserServer(svc users.Service) *httptest.Server {
diff --git a/users/api/endpoint_test.go b/users/api/endpoint_test.go
index 70238cfc..7c60c94a 100644
--- a/users/api/endpoint_test.go
+++ b/users/api/endpoint_test.go
@@ -11,6 +11,7 @@ import (
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
+	"regexp"
 	"strings"
 	"testing"
 
@@ -27,17 +28,22 @@ import (
 
 const (
 	contentType  = "application/json"
+	validEmail   = "user@example.com"
 	invalidEmail = "userexample.com"
+	validPass    = "password"
+	invalidPass  = "wrong"
 )
 
 var (
-	user           = users.User{Email: "user@example.com", Password: "password"}
+	user           = users.User{Email: validEmail, Password: validPass}
 	notFoundRes    = toJSON(errorRes{users.ErrUserNotFound.Error()})
 	unauthRes      = toJSON(errorRes{users.ErrUnauthorizedAccess.Error()})
 	malformedRes   = toJSON(errorRes{users.ErrMalformedEntity.Error()})
+	weakPassword   = toJSON(errorRes{users.ErrPasswordFormat.Error()})
 	unsupportedRes = toJSON(errorRes{api.ErrUnsupportedContentType.Error()})
 	failDecodeRes  = toJSON(errorRes{api.ErrFailedDecode.Error()})
 	groupExists    = toJSON(errorRes{users.ErrGroupConflict.Error()})
+	passRegex      = regexp.MustCompile("^.{8,}$")
 )
 
 type testRequest struct {
@@ -73,7 +79,7 @@ func newService() users.Service {
 	email := mocks.NewEmailer()
 	idProvider := uuid.New()
 
-	return users.New(usersRepo, groupRepo, hasher, auth, email, idProvider)
+	return users.New(usersRepo, groupRepo, hasher, auth, email, idProvider, passRegex)
 }
 
 func newServer(svc users.Service) *httptest.Server {
@@ -93,7 +99,8 @@ func TestRegister(t *testing.T) {
 	client := ts.Client()
 
 	data := toJSON(user)
-	invalidData := toJSON(users.User{Email: invalidEmail, Password: "password"})
+	invalidData := toJSON(users.User{Email: invalidEmail, Password: validPass})
+	invalidPasswordData := toJSON(users.User{Email: validEmail, Password: invalidPass})
 	invalidFieldData := fmt.Sprintf(`{"email": "%s", "pass": "%s"}`, user.Email, user.Password)
 
 	cases := []struct {
@@ -105,6 +112,7 @@ func TestRegister(t *testing.T) {
 		{"register new user", data, contentType, http.StatusCreated},
 		{"register existing user", data, contentType, http.StatusConflict},
 		{"register user with invalid email address", invalidData, contentType, http.StatusBadRequest},
+		{"register user with weak password", invalidPasswordData, contentType, http.StatusBadRequest},
 		{"register user with invalid request format", "{", contentType, http.StatusBadRequest},
 		{"register user with empty JSON request", "{}", contentType, http.StatusBadRequest},
 		{"register user with empty request", "", contentType, http.StatusBadRequest},
@@ -138,15 +146,15 @@ func TestLogin(t *testing.T) {
 	data := toJSON(user)
 	invalidEmailData := toJSON(users.User{
 		Email:    invalidEmail,
-		Password: "password",
+		Password: validPass,
 	})
 	invalidData := toJSON(users.User{
-		Email:    "user@example.com",
+		Email:    validEmail,
 		Password: "invalid_password",
 	})
 	nonexistentData := toJSON(users.User{
 		Email:    "non-existentuser@example.com",
-		Password: "password",
+		Password: validPass,
 	})
 	_, err := svc.Register(context.Background(), user)
 	require.Nil(t, err, fmt.Sprintf("register user got unexpected error: %s", err))
@@ -235,7 +243,7 @@ func TestPasswordResetRequest(t *testing.T) {
 
 	nonexistentData := toJSON(users.User{
 		Email:    "non-existentuser@example.com",
-		Password: "password",
+		Password: validPass,
 	})
 
 	expectedExisting := toJSON(struct {
@@ -322,9 +330,12 @@ func TestPasswordReset(t *testing.T) {
 
 	reqData.Token = token
 
-	reqData.ConfPass = "wrong"
+	reqData.ConfPass = invalidPass
 	reqPassNoMatch := toJSON(reqData)
 
+	reqData.Password = invalidPass
+	reqPassWeak := toJSON(reqData)
+
 	cases := []struct {
 		desc        string
 		req         string
@@ -340,6 +351,7 @@ func TestPasswordReset(t *testing.T) {
 		{"password reset request with empty JSON request", "{}", contentType, http.StatusBadRequest, malformedRes, token},
 		{"password reset request with empty request", "", contentType, http.StatusBadRequest, failDecodeRes, token},
 		{"password reset request with missing content type", reqExisting, "", http.StatusUnsupportedMediaType, unsupportedRes, token},
+		{"password reset with weak password", reqPassWeak, contentType, http.StatusBadRequest, weakPassword, token},
 	}
 
 	for _, tc := range cases {
@@ -395,9 +407,13 @@ func TestPasswordChange(t *testing.T) {
 
 	reqNoExist := toJSON(reqData)
 
-	reqData.OldPassw = "wrong"
+	reqData.OldPassw = invalidPass
 	reqWrongPass := toJSON(reqData)
 
+	reqData.OldPassw = user.Password
+	reqData.Password = invalidPass
+	reqWeakPass := toJSON(reqData)
+
 	resData.Msg = users.ErrUnauthorizedAccess.Error()
 
 	cases := []struct {
@@ -411,6 +427,7 @@ func TestPasswordChange(t *testing.T) {
 		{"password change with valid token", dataResExisting, contentType, http.StatusCreated, expectedSuccess, token},
 		{"password change with invalid token", reqNoExist, contentType, http.StatusForbidden, unauthRes, ""},
 		{"password change with invalid old password", reqWrongPass, contentType, http.StatusForbidden, unauthRes, token},
+		{"password change with invalid new password", reqWeakPass, contentType, http.StatusBadRequest, weakPassword, token},
 		{"password change with empty JSON request", "{}", contentType, http.StatusBadRequest, malformedRes, token},
 		{"password change empty request", "", contentType, http.StatusBadRequest, failDecodeRes, token},
 		{"password change missing content type", dataResExisting, "", http.StatusUnsupportedMediaType, unsupportedRes, token},
diff --git a/users/service_test.go b/users/service_test.go
index c316db3d..93b2197e 100644
--- a/users/service_test.go
+++ b/users/service_test.go
@@ -6,6 +6,7 @@ package users_test
 import (
 	"context"
 	"fmt"
+	"regexp"
 	"testing"
 
 	"github.com/mainflux/mainflux"
@@ -27,6 +28,7 @@ var (
 	groupName       = "Mainflux"
 
 	idProvider = uuid.New()
+	passRegex  = regexp.MustCompile("^.{8,}$")
 )
 
 func newService() users.Service {
@@ -36,7 +38,7 @@ func newService() users.Service {
 	auth := mocks.NewAuthService(map[string]string{user.Email: user.Email})
 	e := mocks.NewEmailer()
 
-	return users.New(userRepo, groupRepo, hasher, auth, e, idProvider)
+	return users.New(userRepo, groupRepo, hasher, auth, e, idProvider, passRegex)
 }
 
 func TestRegister(t *testing.T) {
@@ -58,12 +60,12 @@ func TestRegister(t *testing.T) {
 			err:  users.ErrConflict,
 		},
 		{
-			desc: "register new user with empty password",
+			desc: "register new user with weak password",
 			user: users.User{
 				Email:    user.Email,
-				Password: "",
+				Password: "weak",
 			},
-			err: users.ErrMalformedEntity,
+			err: users.ErrPasswordFormat,
 		},
 	}
 
diff --git a/users/users_test.go b/users/users_test.go
index 5da4e3fc..b777f6aa 100644
--- a/users/users_test.go
+++ b/users/users_test.go
@@ -72,13 +72,6 @@ func TestValidate(t *testing.T) {
 			},
 			err: users.ErrMalformedEntity,
 		},
-		"validate user with empty password": {
-			user: users.User{
-				Email:    email,
-				Password: "",
-			},
-			err: users.ErrMalformedEntity,
-		},
 		"validate user with invalid email": {
 			user: users.User{
 				Email:    "userexample.com",