MF-1317 - Configurable regexp rule for password #1355

blokovi · 2021-02-04T22:51:22Z

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

manuio · 2021-02-12T11:43:48Z

users/users_test.go

-		"validate user with empty password": {
-			user: users.User{
-				Email:    email,
-				Password: "",
-			},
-			err: users.ErrMalformedEntity,
-		},


There is a reason to remove this test?

Removed because password validation is removed from users/api/requests.go

codecov-io · 2021-02-12T11:50:45Z

Codecov Report

Merging #1355 (0681120) into master (56d04cd) will decrease coverage by 0.02%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##           master    #1355      +/-   ##
==========================================
- Coverage   60.55%   60.53%   -0.03%     
==========================================
  Files         123      123              
  Lines        9411     9416       +5     
==========================================
+ Hits         5699     5700       +1     
- Misses       3219     3221       +2     
- Partials      493      495       +2

Impacted Files	Coverage Δ
users/users.go	`77.77% <ø> (-1.17%)`	⬇️
users/service.go	`64.13% <42.85%> (-2.53%)`	⬇️
users/api/requests.go	`42.30% <100.00%> (+1.56%)`	⬆️
users/api/transport.go	`63.57% <100.00%> (+0.25%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56d04cd...0681120. Read the comment docs.

nmarcetic · 2021-02-12T11:55:18Z

cmd/users/main.go

@@ -108,6 +111,8 @@ const (
 	envAuthTimeout = "MF_AUTH_GRPC_TIMEOUT"
 )

+var passRegex = regexp.MustCompile(mainflux.Env(envPassRegex, defPassRegex))


Question: This will panic if not compile, is that expected behavior? Or we should use regexp.Compile and check for errors?

In my opinion nothing should fail without being logged and log sent remotely. But this is main(), so I do not know if this should be tolerated, as you will be probably looking the logs directly in console when deploying. However, I can not guarantee that, as there can be automated deployments (even without a console), and logs should be watched remotely.

I have used MustCompile because I saw it's used on other places:
https://github.com/mainflux/mainflux/blob/master/users/users.go#L26-L28

@blokovi I would use Compile instead of MustCompile and check the errors: https://golang.org/pkg/regexp/#Compile
You can maybe also fix it in users service in this PR

I'd do something like that

logger, err := logger.New(os.Stdout, cfg.logLevel) if err != nil { log.Fatalf(err.Error()) } passRegex, err := regexp.Compile(mainflux.Env(envPassRegex, defPassRegex)) if err != nil { passRegex, err := regexp.MustCompile(defPassRegex) }

nmarcetic · 2021-02-12T11:56:47Z

pkg/sdk/go/users_test.go

@@ -24,6 +25,10 @@ const (
 	invalidEmail = "userexample.com"
 )

+var (
+	passRegex = regexp.MustCompile("^.{8,}$")


I would rather export defPassRegex from main if you need it in multiple places.
Or set a new exported constant which will be used in other files, if you want to keep consistency (no exported def values) and then bind defPassRegex = new constant

mteodor · 2021-02-17T11:13:12Z

users/api/requests.go

@@ -17,6 +16,9 @@ type userReq struct {
 }

 func (req userReq) validate() error {
+	if !passRegex.MatchString(req.user.Password) {
+		return users.ErrMalformedEntity


maybe the error should state exactly what the problem is password doesn't match policy, something like that

mteodor · 2021-02-17T11:14:51Z

users/users.go

@@ -47,10 +46,6 @@ func (u User) Validate() error {
 		return ErrMalformedEntity
 	}

-	if len(u.Password) < minPassLen {


why we dont check password here?

two reasons:

we already check it on other places (validating requests for registering and password changing)

can't import passRegex from api package here

@blokovi I would check it here as it was done before and simply do:

func (req userReq) validate() error { return req.user.Validate() }

@manuio can't use passRegex from api package here in users (because api imports users)

@blokovi first remove it from the api layer :)

manuio · 2021-02-18T12:39:03Z

users/api/endpoint_test.go

 	invalidData := toJSON(users.User{Email: invalidEmail, Password: "password"})
+	invalidPasswordData := toJSON(users.User{Email: "user@example.com", Password: "pass"})


Can you create consts for validEmail, validPass and invalidPass please

manuio · 2021-02-18T14:53:18Z

users/users.go

@@ -47,10 +46,6 @@ func (u User) Validate() error {
 		return ErrMalformedEntity
 	}

-	if len(u.Password) < minPassLen {


@blokovi I would check it here as it was done before and simply do:

func (req userReq) validate() error { return req.user.Validate() }

drasko · 2021-02-21T22:58:49Z

cmd/users/main.go

@@ -108,6 +111,8 @@ const (
 	envAuthTimeout = "MF_AUTH_GRPC_TIMEOUT"
 )

+var passRegex = regexp.MustCompile(mainflux.Env(envPassRegex, defPassRegex))


In my opinion nothing should fail without being logged and log sent remotely. But this is main(), so I do not know if this should be tolerated, as you will be probably looking the logs directly in console when deploying. However, I can not guarantee that, as there can be automated deployments (even without a console), and logs should be watched remotely.

manuio · 2021-02-23T06:56:54Z

cmd/users/main.go

@@ -111,7 +111,7 @@ const (
 	envAuthTimeout = "MF_AUTH_GRPC_TIMEOUT"
 )

-var passRegex = regexp.MustCompile(mainflux.Env(envPassRegex, defPassRegex))
+// var passRegex = regexp.MustCompile(mainflux.Env(envPassRegex, defPassRegex))


Remove this please

manuio · 2021-02-23T06:59:14Z

users/service.go

 	return &usersService{
 		users:      users,
 		groups:     groups,
 		hasher:     hasher,
 		auth:       auth,
 		email:      m,
 		idProvider: idp,
+		// passRegex:  passRegex,


Uncomment and use this one

manuio · 2021-02-23T06:59:20Z

users/service.go

 }

 // 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 {
+	PassRegex = passRegex


Remove this one

manuio · 2021-02-23T06:59:46Z

users/api/requests.go

+	// if !passRegex.MatchString(req.user.Password) {
+	// 	return users.ErrPasswordPolicy
+	// }
 	return req.user.Validate()
+	// if err := req.user.Validate(); err != nil {
+	// 	return err
+	// }
+	// if !passRegex.MatchString(req.user.Password) {
+	// 	return users.ErrPasswordPolicy
+	// }
+	// return nil


Remove this comments please

dborovcanin

While I see why we added password regex as a field to the Users service, I find it a bit cleaner to use a package-level variable in the api package because we offload data validation to the transport layer (that's what we usually do). While package-level variables are generally not a good idea, this one is 1) safe for concurrent use and 2) set only on call of MakeHandler and never changed (i.e. does not hold the state and is easy to test and mock).

dborovcanin · 2021-02-23T18:15:28Z

cmd/users/main.go

+	if err != nil {
+		passRegex = regexp.MustCompile(defPassRegex)
+	}


No need to fail silently. We can do it similar to the other env var handling:

passRegex, err := regexp.Compile(mainflux.Env(envPassRegex, defPassRegex)) if err != nil { log.Fatalf("Invalid password validation rules %s\n", envPassRegex) }

dborovcanin · 2021-02-23T18:17:35Z

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")
+
+	// ErrPasswordPolicy indicates weak password.
+	ErrPasswordPolicy = errors.New("password doesn't match policy")


Using Policy terminology can be confusing; maybe ErrPasswordFormat.

dborovcanin · 2021-02-23T18:21:43Z

users/service.go

@@ -302,6 +310,9 @@ func (svc usersService) ChangePassword(ctx context.Context, authToken, password,
 	if err != nil {
 		return errors.Wrap(ErrUnauthorizedAccess, err)
 	}
+	if !svc.passRegex.MatchString(password) {


Should we validate password format in the ResetPassword metod?

manuio

LGTM

manuio

LGTM

Signed-off-by: Ivan Milosevic <iva@blokovi.com>

… var Signed-off-by: Ivan Milosevic <iva@blokovi.com>

Signed-off-by: Ivan Milosevic <iva@blokovi.com>

drasko

LGTM

* read and validate regex envar Signed-off-by: Ivan Milosevic <iva@blokovi.com> * pass regexp to user/api Signed-off-by: Ivan Milosevic <iva@blokovi.com> * resolve conflicts Signed-off-by: Ivan Milosevic <iva@blokovi.com> * use exported regexp variable Signed-off-by: Ivan Milosevic <iva@blokovi.com> * move password validation from users package Signed-off-by: Ivan Milosevic <iva@blokovi.com> * remove dead code Signed-off-by: Ivan Milosevic <iva@blokovi.com> * add password change request Signed-off-by: Ivan Milosevic <iva@blokovi.com> * move regexp from api to users package Signed-off-by: Ivan Milosevic <iva@blokovi.com> * fix tests Signed-off-by: Ivan Milosevic <iva@blokovi.com> * remove commented code Signed-off-by: Ivan Milosevic <iva@blokovi.com> * add regexp as field in userService, remove it as user exported global var Signed-off-by: Ivan Milosevic <iva@blokovi.com> * add passwd validation in service Signed-off-by: Ivan Milosevic <iva@blokovi.com> * Add psswd validation for change password in service Signed-off-by: Ivan Milosevic <iva@blokovi.com> * add password validation in password reset Signed-off-by: Ivan Milosevic <iva@blokovi.com> * Remove password validation from user validation test Signed-off-by: Ivan Milosevic <iva@blokovi.com> * Replace email and passwords in test with constants Signed-off-by: Ivan Milosevic <iva@blokovi.com> * compile error not fail silently Signed-off-by: Ivan Milosevic <iva@blokovi.com> * fix tempate path Signed-off-by: Ivan Milosevic <iva@blokovi.com>

blokovi added the work-in-progress label Feb 4, 2021

blokovi force-pushed the MF-1317 branch from c1b067b to c3d4ecb Compare February 4, 2021 22:53

manuio mentioned this pull request Feb 8, 2021

MF-1317 #1359

Closed

blokovi removed the work-in-progress label Feb 12, 2021

blokovi force-pushed the MF-1317 branch from 709194d to ac4c884 Compare February 12, 2021 11:41

blokovi marked this pull request as ready for review February 12, 2021 11:41

blokovi requested a review from a team as a code owner February 12, 2021 11:41

manuio reviewed Feb 12, 2021

View reviewed changes

nmarcetic reviewed Feb 12, 2021

View reviewed changes

mteodor reviewed Feb 17, 2021

View reviewed changes

mteodor 8000 reviewed Feb 17, 2021

View reviewed changes

manuio suggested changes Feb 18, 2021

View reviewed changes

drasko requested changes Feb 21, 2021

View reviewed changes

manuio suggested changes Feb 23, 2021

View reviewed changes

blokovi force-pushed the MF-1317 branch from c096f9e to db3b684 Compare February 23, 2021 11:06

dborovcanin requested changes Feb 23, 2021

View reviewed changes

blokovi force-pushed the MF-1317 branch from 5a80ac7 to b7ab72d Compare February 25, 2021 19:49

manuio previously approved these changes Feb 26, 2021

View reviewed changes

mteodor approved these changes Feb 26, 2021

View reviewed changes

blokovi mentioned this pull request Feb 26, 2021

MF-1346 - Create Groups API - add grouping of entities #1334

Merged

mteodor previously approved these changes Mar 1, 2021

View reviewed changes

dborovcanin previously approved these changes Mar 1, 2021

View reviewed changes

blokovi dismissed stale reviews from dborovcanin, mteodor, and manuio via 83c60e8 March 1, 2021 10:40

blokovi force-pushed the MF-1317 branch from 0681120 to 83c60e8 Compare March 1, 2021 10:40

manuio previously approved these changes Mar 1, 2021

View reviewed changes

mteodor previously approved these changes Mar 1, 2021

View reviewed changes

read and validate regex envar

04d4d79

Signed-off-by: Ivan Milosevic <iva@blokovi.com>

blokovi added 17 commits March 1, 2021 12:42

pass regexp to user/api

b764b0e

Signed-off-by: Ivan Milosevic <iva@blokovi.com>

BE96 resolve conflicts

d634dcf

Signed-off-by: Ivan Milosevic <iva@blokovi.com>

use exported regexp variable

49bbf5d

Signed-off-by: Ivan Milosevic <iva@blokovi.com>

move password validation from users package

7961004

Signed-off-by: Ivan Milosevic <iva@blokovi.com>

remove dead code

05ddd6c

Signed-off-by: Ivan Milosevic <iva@blokovi.com>

add password change request

a581c27

Signed-off-by: Ivan Milosevic <iva@blokovi.com>

move regexp from api to users package

441bcbc

Signed-off-by: Ivan Milosevic <iva@blokovi.com>

fix tests

fdaff10

Signed-off-by: Ivan Milosevic <iva@blokovi.com>

remove commented code

5869932

Signed-off-by: Ivan Milosevic <iva@blokovi.com>

add regexp as field in userService, remove it as user exported global…

2d3ef52

… var Signed-off-by: Ivan Milosevic <iva@blokovi.com>

add passwd validation in service

edb1ab5

Signed-off-by: Ivan Milosevic <iva@blokovi.com>

Add psswd validation for change password in service

a6b84ae

Signed-off-by: Ivan Milosevic <iva@blokovi.com>

add password validation in password reset

2a6f40b

Signed-off-by: Ivan Milosevic <iva@blokovi.com>

Remove password validation from user validation test

5e5c5ac

Signed-off-by: Ivan Milosevic <iva@blokovi.com>

Replace email and passwords in test with constants

ae88fec

Signed-off-by: Ivan Milosevic <iva@blokovi.com>

compile error not fail silently

545858b

Signed-off-by: Ivan Milosevic <iva@blokovi.com>

fix tempate path

d1f3985

Signed-off-by: Ivan Milosevic <iva@blokovi.com>

blokovi dismissed stale reviews from mteodor and manuio via d1f3985 March 1, 2021 11:42

blokovi force-pushed the MF-1317 branch from 83c60e8 to d1f3985 Compare March 1, 2021 11:42

drasko approved these changes Mar 1, 2021

View reviewed changes

drasko merged commit 7bcaa32 into absmach:master Mar 1, 2021

blokovi deleted the MF-1317 branch March 11, 2021 10:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

MF-1317 - Configurable regexp rule for password #1355

MF-1317 - Configurable regexp rule for password #1355

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

		invalidData := toJSON(users.User{Email: invalidEmail, Password: "password"})
		invalidPasswordData := toJSON(users.User{Email: "user@example.com", Password: "pass"})

MF-1317 - Configurable regexp rule for password #1355

MF-1317 - Configurable regexp rule for password #1355

Uh oh!

Conversation

Uh oh!

Which issue(s) does this PR fix/relate to?

List any changes that modify/break current functionality

Have you included tests for your changes?

Did you document any new/modified functionality?

Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!