fix(GraphQl): Panic fix when subscription expiry is not present in jwt.

[JatinDev543]

This PR fixes panic error when expiry is not given in jwt for subscriptions using authorization.

---

This change is 

[pawanrawal]

we should add an automated test as well for when expiry is nil

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jatindevdg and @MichaelJCompton)

---

graphql/web/http.go, line 156 at r1 (raw file):

	}

	if len(header) == 0 || customClaims.StandardClaims.ExpiresAt == nil {

Do you need the if len(header) == 0 check here? Seems like only the second check is enough.

Also, add a comment here that for cases where no Header was given or for when expiry is nil we set to the zero value for time.

[pawanrawal]

Why have we committed the main file here?

Reviewed 4 of 4 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jatindevdg, @manishrjain, @MichaelJCompton, and @vvbalaji-dgraph)

---

graphql/e2e/subscription/subscription_test.go, line 503 at r2 (raw file):

		string(resp.Data))

	// Add a TODO for alice which should not be visible in the update because JWT belongs to

You don't need anything below, do you? If the first query above worked then you have tested the fix right? Try to keep the tests as minimal as possible to test only what is required.

---

graphql/web/http.go, line 155 at r2 (raw file):

		}
	}
	//for the cases when no expiry is given in jwt or subscription doesn't have any authorization,

Add a space after // please.

[JatinDev543]

Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)

---

graphql/e2e/subscription/subscription_test.go, line 503 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

You don't need anything below, do you? If the first query above worked then you have tested the fix right? Try to keep the tests as minimal as possible to test only what is required.

yeah, deleted extra thing. i was just making sure auth is working correctly after this change.

---

graphql/web/http.go, line 156 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Do you need the if len(header) == 0 check here? Seems like only the second check is enough.

Also, add a comment here that for cases where no Header was given or for when expiry is nil we set to the zero value for time.

changed and added comment.

---

graphql/web/http.go, line 155 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Add a space after // please.

done.

[JatinDev543]

Why have we committed the main file here?

sorry, it was by mistake.

Reviewed 4 of 4 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jatindevdg, @manishrjain, @MichaelJCompton, and @vvbalaji-dgraph)

graphql/e2e/subscription/subscription_test.go, line 503 at r2 (raw file):

		string(resp.Data))

	// Add a TODO for alice which should not be visible in the update because JWT belongs to

You don't need anything below, do you? If the first query above worked then you have tested the fix right? Try to keep the tests as minimal as possible to test only what is required.

graphql/web/http.go, line 155 at r2 (raw file):

		}
	}
	//for the cases when no expiry is given in jwt or subscription doesn't have any authorization,

Add a space after // please.

[pawanrawal]

:lgtm

Reviewed 2 of 3 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jatindevdg, @manishrjain, @MichaelJCompton, and @vvbalaji-dgraph)

---

graphql/e2e/subscription/subscription_test.go, line 503 at r2 (raw file):

Previously, JatinDevDG (Jatin Dev) wrote…

yeah, deleted extra thing. i was just making sure auth is working correctly after this change.

That would already be taken care by the tests that we have already I think.

[JatinDev543]

:lgtm

Reviewed 2 of 3 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jatindevdg, @manishrjain, @MichaelJCompton, and @vvbalaji-dgraph)

graphql/e2e/subscription/subscription_test.go, line 503 at r2 (raw file):

Previously, JatinDevDG (Jatin Dev) wrote…

yeah, deleted extra thing. i was just making sure auth is working correctly after this change.

That would already be taken care by the tests that we have already I think.

yeah.