Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide more error details when calling checkSession() #618

Closed
osdiab opened this issue Mar 16, 2022 · 7 comments
Closed

Provide more error details when calling checkSession() #618

osdiab opened this issue Mar 16, 2022 · 7 comments
Labels
needs investigation This needs to be investigated further before proceeding

Comments

@osdiab
Copy link

osdiab commented Mar 16, 2022

Describe the problem you'd like to have solved

I want a more precise error to be returned from the checkSession() helper on the frontend. Right now it just returns a generic error, and doesn't say why the session failed to be checked.

This has important implications for my app, where we are using the checkSession() function to decide if we should automatically log out a user. In particular, we cannot distinguish if it failed because the user's internet connection cut, or if it really received a 401 when calling /api/auth/me. As a result, we've been logging people out even when we don't need to, causing a degraded experience.

Describe the ideal solution

Ideally we'd have an enum of values that says exactly what the error was:

enum CheckSessionErrorType {
  NETWORK_REQUEST_FAILURE,
  SESSION_EXPIRED,
  INTERNAL_SERVER_ERROR,
  // ...
}

And then we can act based on that. Another option is to have the checkSession() function return more details of why checking the session failed. For example, A) if the fetch itself ran into a network error; B) what the status code was when the session check failed, C) any details from the body. This is not as good since it would tie us to whatever the scheme for fetching the data is at the time of integration, whereas a thorough enum would put the burden of keeping it up-to-date in the library, which makes more sense to me.

Alternatives and current work-arounds

  • Just call /api/auth/me ourselves and forego the checkSession() helper.
  • as @adamjmcgrath described, checkwindow.navigator.onLine before logging people out

Additional information, if any

In our application, we use Auth0 to handle authentication, and Hasura to access data on our database, via Webhook Authentication. What that means is, our Hasura server will forward the user's headers when they made the API request to a webhook on our server, and our server will respond with headers that Hasura can use to authenticate the user.

The problem is that if the session is expired, then the webhook will just return that the user isn't logged in at all. Then, Hasura will try to execute an API call that requires authentication, but will have no credentials, so it will fail. But the way that Hasura fails is not to return a 401 or something similar, but instead to say that the API endpoint that the user wished to use literally doesn't exist.

The result is that on Hasura's end, we don't have a clear way to distinguish if the invocation was bad, or if the session expired. So we end up needing to check it ourselves. That's why we turned to checkSession(), but it turns out that checkSession() can return failures that don't actually indicate session expiry, hence this problem.

@osdiab osdiab changed the title Provide more details when calling checkSession() Provide more error details when calling checkSession() Mar 16, 2022
@adamjmcgrath
Copy link
Contributor

Hi @osdiab - thanks for raising this

In particular, we cannot distinguish if it failed because the user's internet connection cut, or if it really received a 401 when calling /api/auth/me.

Could you not use window.navigator.onLine to handle the error differently?

@adamjmcgrath adamjmcgrath added the question Further information is requested label Mar 16, 2022
@osdiab
Copy link
Author

osdiab commented Mar 18, 2022

That might work - I just think that the check itself is a bit blunt. If the request fails for any reason, then based on the library code linked above, useUser will clear out the user object, which has a lot of cascading effects throughout my app (anywhere we check for presence of a user will be rerendered as though there was no user due to hook propagation). So perhaps even more significantly than having checkSession() return a better error reason, it seems like a weird choice that losing an internet connection would then cause a previously fetched and valid user session to get cleared out from our frontend (and perhaps may also need to listen on window.navigator.onLine to re-check the session after, for example, your mobile connection cut for a bit, or you got off wifi - quite a bit of legwork).

Hope that makes sense, but yeah I'll give it a shot to see if window.navigator.onLine would be good enough to stop providing a frustrating user experience to our users. My ideal is that this works out of box for our use case as smoothly as possible, right now that's not the case.

@adamjmcgrath adamjmcgrath added needs investigation This needs to be investigated further before proceeding and removed question Further information is requested labels Mar 23, 2022
@adamjmcgrath
Copy link
Contributor

Hi @osdiab - this deserves some more investigation.

You can use the workaround I've described for now, but I'll take a look at a more permanent solution at some point and update this issue when I do.

@adamjmcgrath
Copy link
Contributor

Hi @osdiab

seems like a weird choice that losing an internet connection would then cause a previously fetched and valid user session to get cleared out from our frontend

I agree that we shouldn't clear the user when you get a non 401 error from /api/auth/me - unfortunately, because this behaviour is wrapped up in the customisable fetcher, it means we can't change it until we do a breaking change. So I'll add this to the list of things for the next major.

In the meantime, please use the workarounds I've described. You can also workaround this by customising the fetcher to only throw on a 401.

@osdiab
Copy link
Author

osdiab commented Mar 25, 2022

Sounds good - that's what we're going with. Thanks!

@osdiab
Copy link
Author

osdiab commented Jun 11, 2022

I just saw that this PR happened: #639

I'm guessing that if I do a version bump this might not happen anymore. is that the case?

@adamjmcgrath
Copy link
Contributor

HI @osdiab - correct, this got fixed in 1.8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs investigation This needs to be investigated further before proceeding
Projects
None yet
Development

No branches or pull requests

2 participants