MF-1008 - Make token duration configurable #1550

rodneyosodo · 2022-01-20T14:17:42Z

Signed-off-by: 0x6f736f646f blackd0t@protonmail.com

What does this do?

Make token duration configurable at service

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

Resolves #1008

List any changes that modify/break current functionality

It makes the token duration configurable

Have you included tests for your changes?

No

Did you document any new/modified functionality?

No

Notes

No

codecov-commenter · 2022-01-20T15:58:31Z

Codecov Report

Merging #1550 (0364c5b) into master (bcc8cf7) will increase coverage by 1.61%.
The diff coverage is 71.87%.

@@            Coverage Diff             @@
##           master    #1550      +/-   ##
==========================================
+ Coverage   67.47%   69.09%   +1.61%     
==========================================
  Files         139      134       -5     
  Lines       11321    11058     -263     
==========================================
+ Hits         7639     7640       +1     
+ Misses       3051     2787     -264     
  Partials      631      631

Impacted Files	Coverage Δ
pkg/sdk/go/sdk.go	`100.00% <ø> (ø)`
provision/api/transport.go	`0.00% <0.00%> (ø)`
pkg/sdk/go/health.go	`46.66% <46.66%> (ø)`
auth/service.go	`74.39% <100.00%> (+0.12%)`	⬆️
bootstrap/api/transport.go	`93.10% <100.00%> (ø)`
consumers/notifiers/api/transport.go	`87.62% <100.00%> (ø)`
http/api/transport.go	`75.00% <100.00%> (ø)`
readers/api/transport.go	`78.12% <100.00%> (ø)`
things/api/things/http/transport.go	`85.30% <100.00%> (ø)`
twins/api/http/transport.go	`92.48% <100.00%> (ø)`
... and 6 more

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 bcc8cf7...0364c5b. Read the comment docs.

dborovcanin · 2022-01-21T08:27:01Z

cmd/auth/main.go

+	lduration := mainflux.Env(envLoginDuration, defLoginDuration)
+	duration, err := strconv.Atoi(lduration)
+	if err != nil {
+		log.Fatal(err)
+	}
+	loginDuration := time.Duration(duration) * time.Minute


Use

Suggested change

lduration := mainflux.Env(envLoginDuration, defLoginDuration)

duration, err := strconv.Atoi(lduration)

if err != nil {

log.Fatal(err)

}

loginDuration := time.Duration(duration) * time.Minute

loginDuration, err := time.PareDuration(mainflux.Env(envLoginDuration, defLoginDuration))

This also implies that duration is passed using the format with units (1h, 15m, 1500s...) which means that defLoginDuration needs to be changed, and it needs to be updated in README.

Initially, the duration is passed in as a string e.g "5" which means 5 minutes. Then we convert this to Integer and finally time.duration

Please use time.PareDuration instead and document that the duration unit needs to be passed with the value.

dborovcanin · 2022-01-21T08:28:54Z

cmd/auth/main.go

@@ -52,6 +54,7 @@ const (
 	defKetoHost      = "mainflux-keto"
 	defKetoWritePort = "4467"
 	defKetoReadPort  = "4466"
+	defLoginDuration = "50"


Make default 10h to be aligned with the old version.

Changed to 600 which is 10h

docker/.env

mteodor · 2022-01-21T15:23:57Z

auth/README.md

+| MF_AUTH_SERVER_KEY            | Path to server key in pem format                                         |               |
+| MF_AUTH_SECRET                | String used for signing tokens                                           | auth          |
+| MF_AUTH_LOGIN_TOKEN_DURATION  | Time in minutes for the login token to last of type time.duration        | 600m          |
+| MF_JAEGER_URL                 | Jaeger server URL                                                        | localhost:6831|


add space after port value

mteodor · 2022-01-21T15:29:52Z

docker/.env

@@ -35,6 +35,7 @@ MF_AUTH_DB_USER=mainflux
 MF_AUTH_DB_PASS=mainflux
 MF_AUTH_DB=auth
 MF_AUTH_SECRET=secret
+MF_AUTH_LOGIN_TOKEN_DURATION="600m"


maybe MF_AUTH_LOGIN_TOKEN_EXPIRATION is better naming

@dusanb94 had suggested this MF_AUTH_LOGIN_TOKEN_DURATION

auth/README.md

auth/service.go

cmd/auth/main.go

dborovcanin · 2022-01-24T12:34:36Z

auth/README.md

+| MF_AUTH_SERVER_CERT           | Path to server certificate in pem format                                 |                |
+| MF_AUTH_SERVER_KEY            | Path to server key in pem format                                         |                |
+| MF_AUTH_SECRET                | String used for signing tokens                                           | auth           |
+| MF_AUTH_LOGIN_TOKEN_DURATION  | Time in hours  for the login token to last of type time.duration         | 10h            |


@0x6f736f646f This is not correct. It is not necessarily in hours, you can change the time unit in the value itself.

dborovcanin · 2022-01-24T12:35:38Z

auth/service.go

@@ -14,9 +14,6 @@ import (
 )

 const (
-	loginDuration    = 10 * time.Hour
-	recoveryDuration = 5 * time.Minute


Don't remove recoveryDuration, please keep using the constant for recovery duration.

It wasn't being used but I have kept it back

dborovcanin · 2022-01-24T12:58:30Z

auth/service.go

@@ -130,9 +131,9 @@ func (svc service) Issue(ctx context.Context, token string, key Key) (Key, strin
 	case APIKey:
 		return svc.userKey(ctx, token, key)
 	case RecoveryKey:
-		return svc.tmpKey(recoveryDuration, key)
+		return svc.tmpKey(svc.loginDuration, key)


As I mentioned in this comment, revert this line back to use the constant.

dborovcanin · 2022-01-24T13:05:42Z

auth/README.md

+| MF_AUTH_SERVER_CERT           | Path to server certificate in pem format                                   |                |
+| MF_AUTH_SERVER_KEY            | Path to server key in pem format                                           |                |
+| MF_AUTH_SECRET                | String used for signing tokens                                             | auth           |
+| MF_AUTH_LOGIN_TOKEN_DURATION  | Time in hours or minutes for the login token to last of type time.duration | 10h            |


It is not hours or minutes, it can be seconds (and milliseconds, microseconds; though that's not advisable), as well. Just use:

The duration that represents how long the login token is valid from the moment of its creation

arvindh123 · 2022-01-24T15:23:10Z

cmd/auth/main.go

@@ -142,6 +146,11 @@ func loadConfig() config {
 		SSLRootCert: mainflux.Env(envDBSSLRootCert, defDBSSLRootCert),
 	}

+	loginDuration, err := time.ParseDuration(mainflux.Env(envLoginDuration, defLoginDuration))


Instead of panic can we use defLoginDuration value
And log the envLoginDuration as warning or error
If the defLoginDuration value got error during parse , then log panic

Like

loginDuration, err := time.ParseDuration(envLoginDuration) if err != nil { logger.Info("Invalid Login Duration %v: %v", envLoginDuration, err) loginDuration , err := time.ParseDuration(defLoginDuration) if err != nil { log.Painc(fmt.Sprintf("Invalid default login duration %v: %v", defLoginDuration err)) } logger.Info("Using default Login Duration %v", defLoginDuration) }

That's is one way to do it. TBH, since default is not exactly fallback (despite the params naming in mainflux.Env), and due to code simplicity, I prefer the current solution. It's also aligned with the rest of the codebase.

manuio · 2022-01-24T17:06:22Z

auth/README.md

+| MF_AUTH_SERVER_CERT           | Path to server certificate in pem format                                                      |                |
+| MF_AUTH_SERVER_KEY            | Path to server key in pem format                                                              |                |
+| MF_AUTH_SECRET                | String used for signing tokens                                                                | auth           |
+| MF_AUTH_LOGIN_TOKEN_DURATION  | The duration that represents how long the login token is valid from the moment of its created | 10h            |


Maybe you can use something simpler here. The login token lifetime or The login token expiration period

manuio · 2022-01-24T17:06:44Z

auth/api/grpc/endpoint_test.go

@@ -37,6 +37,8 @@ const (

 	authoritiesObj = "authorities"
 	memberRelation = "member"
+


Kill this blank line

manuio · 2022-01-25T12:33:29Z

auth/service.go

@@ -14,7 +14,6 @@ import (
 )

 const (
-	loginDuration    = 10 * time.Hour
 	recoveryDuration = 5 * time.Minute



Kill this blank line

manuio · 2022-01-25T12:34:03Z

@arvindh123 can you update your branch please

Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com>

manuio

LGTM

drasko

LGTM

rodneyosodo requested a review from a team as a code owner January 20, 2022 14:17

dborovcanin requested changes Jan 21, 2022

View reviewed changes

mteodor reviewed Jan 21, 2022

View reviewed changes

dborovcanin requested changes Jan 22, 2022

View reviewed changes

auth/README.md Outdated Show resolved Hide resolved

auth/service.go Show resolved Hide resolved

cmd/auth/main.go Outdated Show resolved Hide resolved

dborovcanin requested changes Jan 24, 2022

View reviewed changes

arvindh123 reviewed Jan 24, 2022

View reviewed changes

manuio reviewed Jan 24, 2022

View reviewed changes

dborovcanin previously approved these changes Jan 25, 2022

View reviewed changes

manuio suggested changes Jan 25, 2022

View reviewed changes

rodneyosodo added 13 commits January 25, 2022 15:44

Make token duration configurable

d01b78f

Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com>

Change env variable name

5d8af63

Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com>

Add MF_AUTH_LOGIN_TOKEN_DURATION in environment variable

a81d03f

Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com>

Change tests

b5fa738

Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com>

Document MF_AUTH_LOGIN_TOKEN_DURATION

18d16f0

Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com>

Change default login duration

a165985

Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com>

Change declaration of loginduration

daf6c5c

Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com>

Add space after port

26946d5

Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com>

Change time to hours

672671d

Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com>

Remove constant login duration

84d302e

Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com>

Keep back recovery duration

1430651

Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com>

Change docs for login token duration

2940511

Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com>

Change login description

677b5ab

Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com>

rodneyosodo dismissed dborovcanin’s stale review via 8a8bce2 January 25, 2022 12:51

rodneyosodo force-pushed the MF-1008 branch from 0364c5b to 677b5ab Compare January 25, 2022 13:12

Remove blank lines

e372401

Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com>

dborovcanin approved these changes Jan 25, 2022

View reviewed changes

manuio approved these changes Jan 25, 2022

View reviewed changes

drasko approved these changes Jan 25, 2022

View reviewed changes

drasko merged commit 2abf9da into absmach:master Jan 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MF-1008 - Make token duration configurable #1550

MF-1008 - Make token duration configurable #1550

rodneyosodo commented Jan 20, 2022

codecov-commenter commented Jan 20, 2022 •

edited

Loading

dborovcanin Jan 21, 2022

rodneyosodo Jan 21, 2022

dborovcanin Jan 21, 2022

rodneyosodo Jan 21, 2022

dborovcanin Jan 21, 2022

rodneyosodo Jan 21, 2022 •

edited

Loading

mteodor Jan 21, 2022

rodneyosodo Jan 21, 2022

mteodor Jan 21, 2022

rodneyosodo Jan 21, 2022

dborovcanin Jan 24, 2022

rodneyosodo Jan 24, 2022

dborovcanin Jan 24, 2022

rodneyosodo Jan 24, 2022

dborovcanin Jan 24, 2022

rodneyosodo Jan 24, 2022

dborovcanin Jan 24, 2022

rodneyosodo Jan 24, 2022

arvindh123 Jan 24, 2022 •

edited

Loading

dborovcanin Jan 24, 2022

manuio Jan 24, 2022 •

edited

Loading

rodneyosodo Jan 25, 2022

manuio Jan 24, 2022

rodneyosodo Jan 25, 2022

manuio Jan 25, 2022

rodneyosodo Jan 25, 2022

manuio commented Jan 25, 2022

manuio left a comment

drasko left a comment

		@@ -37,6 +37,8 @@ const (

		authoritiesObj = "authorities"
		memberRelation = "member"

MF-1008 - Make token duration configurable #1550

MF-1008 - Make token duration configurable #1550

Conversation

rodneyosodo commented Jan 20, 2022

What does this do?

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

codecov-commenter commented Jan 20, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodneyosodo Jan 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arvindh123 Jan 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuio Jan 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuio commented Jan 25, 2022

manuio left a comment

Choose a reason for hiding this comment

drasko left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 20, 2022 •

edited

Loading

rodneyosodo Jan 21, 2022 •

edited

Loading

arvindh123 Jan 24, 2022 •

edited

Loading

manuio Jan 24, 2022 •

edited

Loading