Fix capture and handle expired token #4956

MUzairS15 · 2022-01-18T15:52:18Z

Signed-off-by: MUzairS15 muzair.shaikh810@gmail.com

Description

This PR fixes #3935

Notes for Reviewers
This PR also addresses #3919

Signed commits

Yes, I signed my commits.

1.mov

codecov-commenter · 2022-01-18T15:57:17Z

Codecov Report

Merging #4956 (8b7c671) into master (40216c6) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff            @@
##           master   #4956      +/-   ##
=========================================
- Coverage    6.02%   6.01%   -0.01%     
=========================================
  Files         107     107              
  Lines        9850    9851       +1     
=========================================
  Hits          593     593              
- Misses       9100    9101       +1     
  Partials      157     157

Flag	Coverage Δ
gointegrationtests	`6.01% <0.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
handlers/middlewares.go	`0.00% <0.00%> (ø)`
handlers/provider_handler.go	`0.00% <ø> (ø)`
router/server.go	`0.00% <0.00%> (ø)`

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 ff73237...8b7c671. Read the comment docs.

MUzairS15 · 2022-01-18T18:41:28Z

I am not able to see the full coverage report.
Can anyone guide me to increase coverage?

bariqhibat

A comment! Thanks for the contribution! @MUzairS15

bariqhibat · 2022-01-19T15:10:23Z

ui/components/MesheryPerformance/PerformanceProfiles.js

@@ -156,19 +208,27 @@ function PerformanceProfile({ updateProgress, enqueueSnackbar, closeSnackbar })

  function handleError(msg) {
    return function (error) {
-      updateProgress({ showProgress : false });
+      if (error.source.errors[0].message != undefined) {


Can we do a type check here? And additionally use optional chaining

leecalcote · 2022-01-19T15:48:00Z

[Grover] "Cookie Monster, come out and play."

leecalcote

@MUzairS15, thanks for this PR!

This PR was discussed on today's Meshery meeting (we missed you ;) ). A couple comments were given such as "Can the cookie be used to determine session state / token expiry?", "How is this going to be applied to each UI / endpoint? It could be easy to miss an endpoint.", "Can this be achieved in NextJS with one central configuration?", "The golang middleware attached to each endpoint verifies token expiration and can redirect the user. This would be a central approach, too."

MUzairS15 · 2022-01-20T03:37:24Z

"The golang middleware attached to each endpoint verifies token expiration and can redirect the user. This would be a central approach, too."

Current implementation redirects user to login but it fails.

Similar to #3919
Even if, we happen to solve this, without any alert user will be redirected.

"Can the cookie be used to determine session state/token expiry?"

In the UI itself? Yes, we can do this.I thought about this but later resolved to rely on the server as in AuthMiddleware we were doing the same,

"How is this going to be applied to each UI / endpoint? It could be easy to miss an endpoint."

In data-fetch.js const dataFetch = (url, options = {}, successFn, errorFn) => {, we can handle the error here itself, (this will secure all the endpoint making a query),

Pls correct me if I am wrong.

MUzairS15 · 2022-01-20T13:34:33Z

@leecalcote Can you suggest, should I handle error or add check for whether token is still valid or not in const dataFetch = (url, options = {}, successFn, errorFn) => {
OR I am getting things wrong
(Now, I regret not being able to join the meeting🥲, things have been clear)

Abhishek-kumar09 · 2022-01-21T10:07:55Z

Current implementation redirects user to login but it fails.

Are you referring to the current implementation (on master branch)?
It seems like the login request can't be sent directly from the server to the cloud, just by clients like mesheryctl or UI which is the required behaviour. Youy should try for UI redirection to login page, instead. Is it what you are doing now??

Abhishek-kumar09 · 2022-01-21T10:10:55Z

t, should I handle error or add check for whether token is still valid or not in const dataFetch = (url, options = {}, successFn, errorFn) => {

Token validation can only be done on the server/cloud. Rather you can check for the cookie key (token) be present on all requests that includes credential(also called cookies).

MUzairS15 · 2022-01-21T11:24:58Z

Current implementation redirects user to login but it fails.

Are you referring to the current implementation (on master branch)? It seems like the login request can't be sent directly from the server to the cloud, just by clients like mesheryctl or UI which is the required behaviour. Youy should try for UI redirection to login page, instead. Is it what you are doing now??

Yes, on master branch (server code, redirect fails, see screenshot above). And I have added logic to redirect from UI.
Based on #4956 (review), concern is to apply to each endpoint. So, if I instead of checking for errors in individual query call, what if I move this to dataFetch?

MUzairS15 · 2022-01-21T11:26:10Z

t, should I handle error or add check for whether token is still valid or not in const dataFetch = (url, options = {}, successFn, errorFn) => {

Token validation can only be done on the server/cloud. Rather you can check for the cookie key (token) be present on all requests that includes credential(also called cookies).

When token expires, cookie is not cleared, so cookie (token) will be present.

Abhishek-kumar09 · 2022-01-21T11:42:30Z

Based on #4956 (review), concern is to apply to each endpoint. So, if I instead of checking for errors in individual query call, what if I move this to dataFetch?

Yeah that's what you should do, intercept every request that passes cookie to server. Do it in data fetch itself.

When token expires, cookie is not cleared, so cookie (token) will be present.

That's a good thought, but token is set to session expire policy, i.e, it will not expire frequently and only in two conditions:

The user closes the browser (session is lost and so is the token)
Token has timeout for 1 day or probably more (@leecalcote could confirm).

SO the case is extremely rare, meanwhile in case it is not correct (i.e., the auth token expired) the server will automatically redirect to the provider UI.

@Utkarsh-pro do you remember the token expiry time?

MUzairS15 · 2022-01-21T11:46:23Z

SO the case is extremely rare, meanwhile in case it is not correct (i.e., the auth token expired) the server will automatically redirect to the provider UI.

I tried with an expired token but was not redirected to provider UI.

Abhishek-kumar09 · 2022-01-21T12:01:53Z

Okay let's have a check at two levels:

the client level for auth token present which is just one liner.
the server for auth token expiry, and redirection to the provider UI instead of handling the login request itself.

What do you say @MUzairS15 ? What are the things needed for the 2 to be done, if you have any idea.

MUzairS15 · 2022-01-21T12:14:30Z

Okay let's have a check at two levels:

the client level for auth token present which is just one liner.

the server for auth token expiry, and redirection to the provider UI instead of handling the login request itself.

What do you say @MUzairS15 ? What are the things needed for the 2 to be done, if you have any idea.

In server AuthMiddleware checks for session validity, I can use the error to handle the redirect. But, then it will not be graceful handling

Irrespective of the page, users should universally see the same message - a modal informing the user that they their session has expired and that they will be directed to login in 3... 2... 1...

Abhishek-kumar09

Thanks for the PR @MUzairS15
So what you have done on the server is:

check for token cookie being present
If not present redirect to provider UI
else marshal the cookie token and authenticate the user.

Just one concern, WHy you have added graphql in between? Is there any specific usecase?
I think this is not touching any graphql domain. Do we?

Abhishek-kumar09 · 2022-01-22T12:09:58Z

handlers/middlewares.go

+					provider.Logout(w, req)
+				}
+
+				graphQlError := []Body{{"expired"}}


What is this doing?

No, we are not touching graphQL, (just for sake of the name graphQlError) I will change the name.
I am sending `{
"errors": {[
"error" : "expired"
]}

}
` as json to get it when we do a query when token becomes invalid.

I think giving more verbose error will help, like auth token expired

Abhishek-kumar09 · 2022-01-22T12:11:46Z

handlers/middlewares.go

@@ -56,7 +65,26 @@ func (h *Handler) AuthMiddleware(next http.Handler) http.Handler {
 			// }
 			// return
 			if provider.GetProviderType() == models.RemoteProviderType {
-				provider.Logout(w, req)
+				if err.Error() == "http: named cookie not present" {


use contains function for this. https://pkg.go.dev/strings#Contains

Abhishek-kumar09 · 2022-01-22T12:13:42Z

handlers/middlewares.go

+func (h *Handler) validateAuth(provider models.Provider, req *http.Request) (bool, error) {
+	err := provider.GetSession(req)
+	if err == nil {
 		// logrus.Debugf("session: %v", sess)
-		return true
+		return true, err
 	}
 	// logrus.Errorf("session invalid, error: %v", err)
-	return false
+	return false, err


You can delete the unusable comments like seen here.

Yes, will remove.

Abhishek-kumar09 · 2022-01-22T12:15:24Z

ui/components/MesheryPerformance/PerformanceProfiles.js

+      <GenericModal
+        open={open}
+        handleClose={handleClose}
+        Content={
+          <Box sx = {style}>
+            <Typography id="transition-modal-title" variant="h6" component="h2" className={classes.modal}>
+              <span className = {classes.span}>&#9432;</span>Session Expired
+            </Typography>
+            { counter && <Typography id="transition-modal-description" className = {classes.modalText} >
+               Your session has expired.
+               You will now be redirected to login... {counter}
+            </Typography>
+            }
+          </Box>
+        } />


Please extract this dialog and it's style in a separate component. They have been used twice on different pages.

Abhishek-kumar09 · 2022-01-22T12:16:38Z

ui/components/MesheryPerformance/Dashboard.js

-          );
-        },
-        autoHideDuration : 8000, });
+      if (error.source.errors[0].message != undefined) {


Suggested change

if (error.source.errors[0].message != undefined) {

if (error?.source?.errors?.[0]?.message) {

Abhishek-kumar09 · 2022-01-22T12:18:36Z

ui/components/MesheryPerformance/Dashboard.js

+  useEffect(() => {
+    const timer = setTimeout(() => {
+      setCounter(counter - 1)
+    }, 1000)
+    return () => clearTimeout(timer)
+  })


The useEffect Body runs on every state change which is not desired. Actually this modal can be placed in it's own functional component for the purpose of re-usability and then this implementation will look good.

Abhishek-kumar09 · 2022-01-22T12:21:06Z

handlers/middlewares.go

@@ -56,7 +65,26 @@ func (h *Handler) AuthMiddleware(next http.Handler) http.Handler {
 			// }
 			// return
 			if provider.GetProviderType() == models.RemoteProviderType {
-				provider.Logout(w, req)
+				if err.Error() == "http: named cookie not present" {
+					provider.Logout(w, req)


Earlier we had the problem for wildcard response set,

Are we solving it now?

Yes, this is solved with the proposed changes, as we will not redirect from the server in case of an invalid session, instead query will fail, and UI will handle it by alerting the user.

Abhishek-kumar09 · 2022-01-26T17:09:20Z

ui/components/ExpiredModal.js

+        open={open}
+        onClose={handleClose}
+      >
+        <Box sx={style}>


Suggested change

<Box sx={style}>

<Box className={classes.box}>

Abhishek-kumar09 · 2022-01-26T17:15:02Z

handlers/middlewares.go

+				authError := []Body{{"auth-token expired"}}
+				errors := Error{authError}
+				jsonMsg, err := json.Marshal(errors)
+				if err != nil {
+					logrus.Errorf("Error marshaling response")
+					http.Error(w, "unable to authenticate user", http.StatusInternalServerError)
+					return
+				}


Suggested change

authError := []Body{{"auth-token expired"}}

errors := Error{authError}

jsonMsg, err := json.Marshal(errors)

if err != nil {

logrus.Errorf("Error marshaling response")

http.Error(w, "unable to authenticate user", http.StatusInternalServerError)

return

}

Signed-off-by: MUzairS15 <muzair.shaikh810@gmail.com>

Abhishek-kumar09

After 30 mins long iteration, we got the root of the problem.

SO the /user/login is redirecting to the meshery cloud for the user login, but since the redirection is from the server the cloud takes server on localhost:9081 as client and further failing because server can't act as a client and this is the reason behind the cors error.

Cross origin request from the server is denied on the cloud for the Login and that's how it should behave. You can't [instantiate] a login from the server.

Now since we are redirecting to the provider UI now, the actual instantiation of login can be done through react-client.

Cookie Policy:

The cookie expiration policy is set to session, i.e., the cookie invalidates on the new session.
It has its own expiry time more than 24 hours in case the session is not closed,

We are touching both the cases and all the cases the user can open meshery and cannot use any meshery service without being authenticated.

There can be a need of slight UX improvemnt in terms of the first load with invalid token sent to meshery and being validated, but that's a seperate concern and is out of the scope of this PR.

I think this PR can be merged, and you can see the changes with your eyes, the all functions being working the way they should.

UI improvements (minor flash between meshery homepage to going to provider login page (only once)) can be done seprately.

Abhishek-kumar09 · 2022-01-27T13:05:17Z

// @leecalcote

leecalcote · 2022-02-16T14:33:24Z

@MUzairS15, it'd be great for you to quick a quick demo of this work on a Meshery Development Meeting. There's one today in 30 minutes, if you're free. Details here: https://meet.layer5.io

stale · 2022-03-28T15:34:24Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

leecalcote · 2022-03-29T20:35:02Z

@sudo-NithishKarthik what do you think? #4956 (review)

MUzairS15 · 2022-05-06T18:04:21Z

@sudo-NithishKarthik ping

stale · 2022-06-15T19:53:11Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

leecalcote · 2022-06-15T21:41:58Z

@MUzairS15 will you put this on the agenda for next week's Meshery Dev meeting?

MUzairS15 · 2022-06-16T03:47:03Z

Yes @leecalcote

leecalcote · 2022-06-20T16:47:32Z

Discussed again today. The current server-side redirect is good and the return of the user-facing modal is also desirable.

Signed-off-by: MUzairS15 <muzair.shaikh810@gmail.com>

MUzairS15 · 2022-07-11T04:57:15Z

handlers/provider_handler.go

@@ -24,7 +23,6 @@ func (h *Handler) ProviderHandler(w http.ResponseWriter, r *http.Request) {
 			http.SetCookie(w, &http.Cookie{
 				Name:     h.config.ProviderCookieName,
 				Value:    p.Name(),
-				Expires:  time.Now().Add(h.config.ProviderCookieDuration),


Changes meshery-provider cookie as Session cookie.

Signed-off-by: MUzairS15 <muzair.shaikh810@gmail.com>

github-actions bot added the component/ui User Interface label Jan 18, 2022

MUzairS15 force-pushed the feature/MUzairS/3935 branch from b5ffc67 to 78eac4e Compare January 19, 2022 08:24

bariqhibat reviewed Jan 19, 2022

View reviewed changes

leecalcote requested changes Jan 19, 2022

View reviewed changes

Abhishek-kumar09 suggested changes Jan 22, 2022

View reviewed changes

MUzairS15 mentioned this pull request Jan 24, 2022

Reduced code duplication in handling API calls #4979

Closed

1 task

MUzairS15 force-pushed the feature/MUzairS/3935 branch 2 times, most recently from bafa989 to ab905ed Compare January 25, 2022 18:12

Abhishek-kumar09 reviewed Jan 26, 2022

View reviewed changes

Changed redirect endpoint

1c864f5

Signed-off-by: MUzairS15 <muzair.shaikh810@gmail.com>

MUzairS15 force-pushed the feature/MUzairS/3935 branch from 910a1d9 to 1c864f5 Compare January 26, 2022 18:47

github-actions bot removed the component/ui User Interface label Jan 26, 2022

Abhishek-kumar09 previously approved these changes Jan 26, 2022

View reviewed changes

pottekkat mentioned this pull request Feb 16, 2022

[UI] Update the user on "Token" expire #3919

Closed

Chadha93 mentioned this pull request Mar 9, 2022

[UI] Capture and Handle Expired Token #3935

Closed

stale bot added the issue/stale Issue has not had any activity for an extended period of time label Mar 28, 2022

stale bot removed the issue/stale Issue has not had any activity for an extended period of time label Mar 29, 2022

MUzairS15 added 3 commits July 11, 2022 10:21

Merge branch 'meshery:master' into feature/MUzairS/3935

da42a8a

Graceful handling of expired session

6d847be

Signed-off-by: MUzairS15 <muzair.shaikh810@gmail.com>

Update Makefile

e8c386e

Signed-off-by: MUzairS15 <muzair.shaikh810@gmail.com>

MUzairS15 dismissed Abhishek-kumar09’s stale review via e8c386e July 11, 2022 04:55

github-actions bot added the component/ui User Interface label Jul 11, 2022

MUzairS15 commented Jul 11, 2022

View reviewed changes

MUzairS15 changed the title ~~Fix capture and handle expired token in performance page~~ Fix capture and handle expired token Jul 11, 2022

Fix lint failure

8b7c671

Signed-off-by: MUzairS15 <muzair.shaikh810@gmail.com>

leecalcote merged commit 54a7706 into meshery:master Jul 11, 2022

MUzairS15 added kind/enhancement Improvement in current feature language/go Issues or pull requests that use Golang security Issues or pull requests that address a security vulnerability labels Jul 13, 2022

	if (error.source.errors[0].message != undefined) {
	if (error?.source?.errors?.[0]?.message) {

Fix capture and handle expired token #4956

Fix capture and handle expired token #4956

Conversation

MUzairS15 commented Jan 18, 2022 • edited

codecov-commenter commented Jan 18, 2022 • edited

Codecov Report

MUzairS15 commented Jan 18, 2022 • edited

bariqhibat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leecalcote commented Jan 19, 2022

leecalcote left a comment

Choose a reason for hiding this comment

MUzairS15 commented Jan 20, 2022

MUzairS15 commented Jan 20, 2022 • edited

Abhishek-kumar09 commented Jan 21, 2022

Abhishek-kumar09 commented Jan 21, 2022

MUzairS15 commented Jan 21, 2022

MUzairS15 commented Jan 21, 2022

Abhishek-kumar09 commented Jan 21, 2022 • edited

MUzairS15 commented Jan 21, 2022

Abhishek-kumar09 commented Jan 21, 2022

MUzairS15 commented Jan 21, 2022

Abhishek-kumar09 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

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

Abhishek-kumar09 left a comment • edited

Choose a reason for hiding this comment

Abhishek-kumar09 commented Jan 27, 2022

leecalcote commented Feb 16, 2022

stale bot commented Mar 28, 2022

leecalcote commented Mar 29, 2022

MUzairS15 commented May 6, 2022

stale bot commented Jun 15, 2022

leecalcote commented Jun 15, 2022

MUzairS15 commented Jun 16, 2022

leecalcote commented Jun 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MUzairS15 commented Jan 18, 2022 •

edited

codecov-commenter commented Jan 18, 2022 •

edited

MUzairS15 commented Jan 18, 2022 •

edited

MUzairS15 commented Jan 20, 2022 •

edited

Abhishek-kumar09 commented Jan 21, 2022 •

edited

Abhishek-kumar09 left a comment •

edited