chore(tests): Unit test for edgraph/validateToken () #8470

jbhamra1 · 2022-12-01T10:26:13Z

Problem

Missing unit tests for validateToken (), getAcessJwt (), getRefreshJwt () of edgraph package

Solution

TestValidateToken (), TestGetAccessJwt () and TestRefreshJwt () generates a signed JSON Web token using the static data defined in the test file, using the helper function generateJWT (). The generated token includes the token expiry time which is passed as an argument to the helper function.
TestValidateToken () calls validateToken and extracts data from the signed token which is then compared to validate with the original data.

TestGetAccessJwt() and TestRefreshJwt () are direct wrappers over jwt.NewWithClaims () but with their predefined expiry duration.

getAcessJwt () getRefreshJwt ()

coveralls · 2022-12-01T10:54:58Z

Coverage increased (+27.2%) to 64.428% when pulling 0052337 on edgraph_validatetoken_getaccessjwt_getrefreshjwt into 5ac56a1 on main.

…hjwt

edgraph/access_ee_test.go

all-seeing-code · 2022-12-02T09:53:49Z

We can improve the description with details on the tests.

edgraph/access_ee_test.go

Changes incorporated for @anurags92 offline comments. 'go fmt' auto formatting changes.

edgraph/access_ee_test.go

mangalaman93 · 2022-12-12T17:05:30Z

edgraph/access_ee_test.go

+)
+
+var (
+	userDataList = []userData{


move this inside the test where it is used

mangalaman93 · 2022-12-12T17:06:19Z

edgraph/access_ee_test.go

+		e1 = time.Now().Add(time.Minute * 30).Unix()
+	}
+
+	claims := jwt.MapClaims{


you could put this whole thing in one line, should fit

While I agree with the statement (that it could be collapsed), I am not convinced that readability is improved by doing so. If the length of the entire function was excessive, I would be more inclined to lean toward's Aman's suggestion here. As it stands, I personally would leave it as-is, but I would not object to it being collapsed to a single line.

edgraph/access_ee_test.go

mangalaman93 · 2022-12-12T17:07:59Z

edgraph/access_ee_test.go

+		}
+
+		if t1[p-1] != t2[p-1] {
+			match = false


you could just return here

mangalaman93 · 2022-12-12T17:08:08Z

edgraph/access_ee_test.go

+}
+
+func compareTokenParts(tok1 string, tok2 string, part ...int) bool {
+	var (


you don't need this variable at all

edgraph/access_ee_test.go

mangalaman93 · 2022-12-12T17:08:35Z

edgraph/access_ee_test.go

+	)
+
+	for _, userdata := range userDataList {
+		if tokenString, err = generateJWT(userdata.namespace, userdata.userId, userdata.groupIds, 0); err != nil {


just use := here

mangalaman93 · 2022-12-12T17:09:10Z

edgraph/access_ee_test.go

+}
+
+func TestGetAccessJwt(t *testing.T) {
+	var (


Let's not declare variables again, just use := when needed

+1. Here and elsewhere.
Variable declaration should be deferred as late as possible to the point where it is going to be used.
I think the only one I would pre-declare would be the expiry (sic) variable, since it requires a context switch to initialize this.

mangalaman93 · 2022-12-12T17:10:47Z

edgraph/access_ee_test.go

+		 * One of the factor [signature] would differ is based on the expiry time.
+		 * This will differ for tokstr and jwtstr and hence ignoring it.
+		 */
+		match := compareTokenParts(tokstr, jwtstr, 1, 2, 0) // compare 1st and 2nd part, ignore the 3rd.


I would rather do some sort of validateToken call than actually checking what is inside the token

Personally, I am ok with using a canonical set of tokens compared with the retrieved token.
The validation process for the other parts of the token would likely not look much different from "Hey, I expected it to match THIS string!", so the string comparison, I think is valid.

Having said that, our handling of making sure to ignore the timestamp seems a bit clunky. I agree that we should ignore the timestamp token (or at least have a different comparison mechanism), but having it controlled via a varargs type structure where the value either indicates an index (offset by 1) or a 0 seems odd.

A couple ideas here:

Pass a slice of booleans for whether or not to compare instead of index offsets with 0 having special value.

A bit overboard, but if we want to apply different logic based on token types, we could pass in a slice of types where the validation procedure is decided based on the type.

billprovince

See comments inline. Nothing that really prevents checkin in my opinion, but some suggestions for improvements.

edgraph/access_ee_test.go

billprovince · 2022-12-12T22:40:08Z

edgraph/access_ee_test.go

+		e1 = time.Now().Add(time.Minute * 30).Unix()
+	}
+
+	claims := jwt.MapClaims{


While I agree with the statement (that it could be collapsed), I am not convinced that readability is improved by doing so. If the length of the entire function was excessive, I would be more inclined to lean toward's Aman's suggestion here. As it stands, I personally would leave it as-is, but I would not object to it being collapsed to a single line.

billprovince · 2022-12-12T22:49:57Z

edgraph/access_ee_test.go

+	var token *jwt.Token
+
+	e1 := exp
+	if e1 == 0 {


I don't believe that this logic belongs here: I believe that it belongs in the caller.
There is one place where you invoke generateJWT with a 0 for the exp time, and that is in TestValidateToken.
Instead of having to create additional logic here, you could simply define exp in the call to this function.

billprovince · 2022-12-12T22:51:41Z

edgraph/access_ee_test.go

+}
+
+func compareTokenParts(tok1 string, tok2 string, part ...int) bool {
+	var (


billprovince · 2022-12-12T22:51:53Z

edgraph/access_ee_test.go

+		}
+
+		if t1[p-1] != t2[p-1] {
+			match = false


billprovince · 2022-12-12T22:59:27Z

edgraph/access_ee_test.go

+	}
+)
+
+func generateJWT(namespace uint64, userId string, groupIds []string, exp int64) (string, error) {


Prefer the name "expiry" over "exp". I see "exp", and I think "exponent", not "expiry".

The pedantic golang style prefers very terse names -- similar to c-style. However, descriptive names are much simpler to read.

Comment applies here and later in the file.

billprovince · 2022-12-12T23:02:10Z

edgraph/access_ee_test.go

+}
+
+func TestGetAccessJwt(t *testing.T) {
+	var (


+1. Here and elsewhere.
Variable declaration should be deferred as late as possible to the point where it is going to be used.
I think the only one I would pre-declare would be the expiry (sic) variable, since it requires a context switch to initialize this.

billprovince · 2022-12-12T23:23:45Z

edgraph/access_ee_test.go

+		 * One of the factor [signature] would differ is based on the expiry time.
+		 * This will differ for tokstr and jwtstr and hence ignoring it.
+		 */
+		match := compareTokenParts(tokstr, jwtstr, 1, 2, 0) // compare 1st and 2nd part, ignore the 3rd.


Personally, I am ok with using a canonical set of tokens compared with the retrieved token.
The validation process for the other parts of the token would likely not look much different from "Hey, I expected it to match THIS string!", so the string comparison, I think is valid.

Having said that, our handling of making sure to ignore the timestamp seems a bit clunky. I agree that we should ignore the timestamp token (or at least have a different comparison mechanism), but having it controlled via a varargs type structure where the value either indicates an index (offset by 1) or a 0 seems odd.

A couple ideas here:

Pass a slice of booleans for whether or not to compare instead of index offsets with 0 having special value.

A bit overboard, but if we want to apply different logic based on token types, we could pass in a slice of types where the validation procedure is decided based on the type.

billprovince · 2022-12-12T23:25:43Z

edgraph/access_ee_test.go

+		 * This will differ for tokstr and jwtstr and hence ignoring it.
+		 */
+		match := compareTokenParts(tokstr, jwtstr, 1, 2, 0) // compare 1st and 2nd part, ignore the 3rd.
+		if match == false {


Prefer if !match over if match == false
Or, even better:
if !compareTokenParts(...) (since you don't really need the match variable).

Not a big deal, but I think this would be slightly preferable.

jbhamra1 · 2022-12-13T10:45:43Z

edgraph/access_ee_test.go


-var (
-	userDataList = []userData{


I have moved this slice to TestValidateToken(), TestGetAccessJwt(), TestGetRefreshJwt().
Hope this was the expectation.

mangalaman93

I do not think we should compare tokens first and the second part to check whether a token is valid. A token is valid if it has the right data and signature and that is why we should call a validataToken sort of a function in order to verify the token.

generated by getAccessJwt() and getRefreshJwt().

mangalaman93 · 2022-12-13T16:10:17Z

edgraph/access_ee_test.go

+	}
+
+	for _, userdata := range userDataList {
+		tokstr := generateJWT(userdata.namespace, userdata.userId, nil, expiry)


jwtstr, _ := getRefreshJwt(userdata.userId, userdata.namespace) ud2, _ := validateToken(jwtstr)

This is all you need and then you can check whether the values you passed to getRefreshJwt vs what was returned by validateToken.

mangalaman93 · 2022-12-13T16:10:23Z

edgraph/access_ee_test.go

+	}
+
+	for _, userdata := range userDataList {
+		tokstr := generateJWT(userdata.namespace, userdata.userId, g, expiry)


billprovince

There's an issue with our usage of validateToken. To put it bluntly, I don't see how the tests would pass with the change you specified unless you have additional changes not reflected in github.

billprovince · 2022-12-13T17:19:42Z

edgraph/access_ee_test.go

+
+	for _, userdata := range userDataList {
+		jwtstr, _ := getAccessJwt(userdata.userId, grpLst, userdata.namespace)
+		ud, _ := validateToken (jwtstr)


Similar issue with validateToken.
See definition here:
https://github.com/dgraph-io/dgraph/blob/bc71dc3297880e8c282be638506eddab9270b010/edgraph/access.go#L93

It doesn't do anything aside from return nil, nil.

I think you want to look at the file access_ee.go instead.

And therefore, we need to add the following to the test file

//go:build !oss // +build !oss

billprovince · 2022-12-13T17:21:32Z

edgraph/access_ee_test.go

+	for _, userdata := range userDataList {
+		tokenString := generateJWT(userdata.namespace, userdata.userId, userdata.groupIds, expiry)
+
+		ud, _ := validateToken(tokenString)


I suspect that you have a local change, as otherwise, at line 51 below, we should expect an error since ud would be nil.
See: https://github.com/dgraph-io/dgraph/blob/bc71dc3297880e8c282be638506eddab9270b010/edgraph/access.go#L93

edgraph/access.go and edgraph/access_ee.go declares "func validateToken(jwtStr string) ([]string, error)" and "func validateToken(jwtStr string) (*userData, error)" respectively.

The validateToken(tokenString) call from edgraph/access_ee_test.go resolves to "func validateToken(jwtStr string) (*userData, error)" declared in edgraph/access_ee.go but not to "func validateToken(jwtStr string) ([]string, error)" declared in edgraph/access.go, at least, in my linux workspace. The test output shown below. The text Italic bold is the result of an extra statement I have added to the test code, 'fmt.Println (ud)', for verification.

jassi Wed Dec 14 11:58:08 edgraph_validatetoken_getaccessjwt_getrefreshjwt edgraph > go test -v -count=1 ./...
=== RUN TestValidateToken
&{1234567890 user1 [701 702]}
&{2345678901 user2 [703 701]}
&{3456789012 user3 [702 703]}
--- PASS: TestValidateToken (0.00s)
=== RUN TestGetAccessJwt
--- PASS: TestGetAccessJwt (0.00s)
=== RUN TestGetRefreshJwt
--- PASS: TestGetRefreshJwt (0.00s)
=== RUN TestParseNQuads
--- PASS: TestParseNQuads (0.00s)
=== RUN TestValNquads
. . .

When I write a sample program with an Add() function I see a different behaviour altogether, go interpreter throws a message- 'Add redeclared', whereas this is not seen for edgraph/ above.
This is the sample program code-

jassi Wed Dec 14 12:11:06 edgraph_validatetoken_getaccessjwt_getrefreshjwt math > ls -l
total 12
-rw-r--r-- 1 jassi users 65 Dec 14 07:56 m.go
-rw-r--r-- 1 jassi users 148 Dec 14 08:04 m_1.go
-rw-r--r-- 1 jassi users 515 Dec 14 10:45 m_1_test.go
jassi Wed Dec 14 12:11:08 edgraph_validatetoken_getaccessjwt_getrefreshjwt math >
jassi Wed Dec 14 12:11:09 edgraph_validatetoken_getaccessjwt_getrefreshjwt math > cat m.go
package math

func Add (x int, y int) ([]string) {
return nil
}
jassi Wed Dec 14 12:11:15 edgraph_validatetoken_getaccessjwt_getrefreshjwt math > cat m_1.go
package math

type userData struct {
name string
uid int64
}

func Add (x int, y int) (*userData) {
return &userData{name: "Hello", uid: 2233}
}
jassi Wed Dec 14 12:11:25 edgraph_validatetoken_getaccessjwt_getrefreshjwt math >
jassi Wed Dec 14 12:11:33 edgraph_validatetoken_getaccessjwt_getrefreshjwt math > cat m_1_test.go
package math

import (
"testing"
)

func Test_Add (t testing.T) {
Add (3, 5)
/
got := Add (3, 5)
want := 8

if got != want { t.Errorf ("got %v, wanted %v", got, want) }

*/
}
jassi Wed Dec 14 12:12:03 edgraph_validatetoken_getaccessjwt_getrefreshjwt math > go test -v -count=1 ./...

github.com/dgraph-io/dgraph/gotest/math [github.com/dgraph-io/dgraph/gotest/math.test]

./m_1.go:8:6: Add redeclared in this block
./m.go:3:6: other declaration of Add
FAIL github.com/dgraph-io/dgraph/gotest/math [build failed]
FAIL
jassi Wed Dec 14 12:12:25 edgraph_validatetoken_getaccessjwt_getrefreshjwt math >

I don't know what is causing this discrepancy.

edgraph/access_ee_test.go

…hecks.

meghalims

Looks good

Unit test for edgraph/validateToken ()

8435029

getAcessJwt () getRefreshJwt ()

all-seeing-code added 2 commits December 2, 2022 14:11

Merge branch 'main' into edgraph_validatetoken_getaccessjwt_getrefres…

e95c31f

…hjwt

Improve formatting

1eae37f

all-seeing-code reviewed Dec 2, 2022

View reviewed changes

edgraph/access_ee_test.go Outdated Show resolved Hide resolved

edgraph/access_ee_test.go Outdated Show resolved Hide resolved

all-seeing-code reviewed Dec 2, 2022

View reviewed changes

edgraph/access_ee_test.go Outdated Show resolved Hide resolved

edgraph/access_ee_test.go Outdated Show resolved Hide resolved

edgraph/access_ee_test.go Show resolved Hide resolved

jbhamra1 added 2 commits December 2, 2022 13:23

Incorporated changes for review comments

9f3010f

Missed out changes after the review comments changes.

9166e96

jbhamra1 marked this pull request as ready for review December 6, 2022 13:42

jbhamra1 requested review from akon-dey, gcxml, skrdgraph, darkn3rd, meghalims, matthewmcneely and billprovince as code owners December 6, 2022 13:42

jbhamra1 removed request for akon-dey, darkn3rd, gcxml and skrdgraph December 6, 2022 13:43

matthewmcneely reviewed Dec 6, 2022

View reviewed changes

edgraph/access_ee_test.go Outdated Show resolved Hide resolved

edgraph/access_ee_test.go Show resolved Hide resolved

Changes incorporated for @matthewmcneely comments.

452fb7b

Changes incorporated for @anurags92 offline comments. 'go fmt' auto formatting changes.

meghalims requested a review from joshua-goldstein December 7, 2022 22:16

Optimizing code.

abe7fcf

jbhamra1 requested a review from mangalaman93 December 12, 2022 16:57

mangalaman93 requested changes Dec 12, 2022

View reviewed changes

billprovince reviewed Dec 12, 2022

View reviewed changes

Changes done for Aman and Bill's review comments.

27a1595

jbhamra1 commented Dec 13, 2022

View reviewed changes

mangalaman93 approved these changes Dec 13, 2022

View reviewed changes

mangalaman93 requested changes Dec 13, 2022

View reviewed changes

all-seeing-code changed the title ~~Unit test for edgraph/validateToken ()~~ chore(tests): Unit test for edgraph/validateToken () Dec 13, 2022

jbhamra1 added 2 commits December 13, 2022 14:55

Employed validateToken() to validate data

94baa17

generated by getAccessJwt() and getRefreshJwt().

Formatting changes

dac6681

mangalaman93 reviewed Dec 13, 2022

View reviewed changes

Changes added for Aman's comment.

e422fca

billprovince reviewed Dec 13, 2022

View reviewed changes

mangalaman93 reviewed Dec 14, 2022

View reviewed changes

edgraph/access_ee_test.go Show resolved Hide resolved

Changes for build constraints, formating, test error and validation c…

0052337

…hecks.

mangalaman93 approved these changes Dec 15, 2022

View reviewed changes

meghalims approved these changes Dec 15, 2022

View reviewed changes

jbhamra1 merged commit 14b9121 into main Dec 15, 2022

jbhamra1 deleted the edgraph_validatetoken_getaccessjwt_getrefreshjwt branch December 15, 2022 17:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(tests): Unit test for edgraph/validateToken () #8470

chore(tests): Unit test for edgraph/validateToken () #8470

jbhamra1 commented Dec 1, 2022 •

edited by all-seeing-code

Loading

coveralls commented Dec 1, 2022 •

edited

Loading

all-seeing-code commented Dec 2, 2022

mangalaman93 Dec 12, 2022

mangalaman93 Dec 12, 2022

billprovince Dec 12, 2022

mangalaman93 Dec 12, 2022

billprovince Dec 12, 2022

mangalaman93 Dec 12, 2022

billprovince Dec 12, 2022

mangalaman93 Dec 12, 2022

mangalaman93 Dec 12, 2022

billprovince Dec 12, 2022

mangalaman93 Dec 12, 2022

billprovince Dec 12, 2022

billprovince left a comment

billprovince Dec 12, 2022

billprovince Dec 12, 2022

billprovince Dec 12, 2022

billprovince Dec 12, 2022

billprovince Dec 12, 2022

billprovince Dec 12, 2022

billprovince Dec 12, 2022

billprovince Dec 12, 2022

jbhamra1 Dec 13, 2022

mangalaman93 left a comment

mangalaman93 Dec 13, 2022

mangalaman93 Dec 13, 2022

billprovince left a comment

billprovince Dec 13, 2022

mangalaman93 Dec 14, 2022

mangalaman93 Dec 14, 2022

billprovince Dec 13, 2022

jbhamra1 Dec 14, 2022

meghalims left a comment

chore(tests): Unit test for edgraph/validateToken () #8470

chore(tests): Unit test for edgraph/validateToken () #8470

Conversation

jbhamra1 commented Dec 1, 2022 • edited by all-seeing-code Loading

Problem

Solution

coveralls commented Dec 1, 2022 • edited Loading

all-seeing-code commented Dec 2, 2022

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

billprovince left a 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

mangalaman93 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billprovince left a 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

github.com/dgraph-io/dgraph/gotest/math [github.com/dgraph-io/dgraph/gotest/math.test]

meghalims left a comment

Choose a reason for hiding this comment

jbhamra1 commented Dec 1, 2022 •

edited by all-seeing-code

Loading

coveralls commented Dec 1, 2022 •

edited

Loading