search

CruGlobal / conf-registration-web

Event Registration Tool
https://www.eventregistrationtool.com
MIT License
2 stars 1 forks source link

Fix-Double-Login-Modal-Bug #792

Closed TheNoodleMoose closed 2 years ago

TheNoodleMoose commented 2 years ago

The API recently updated so that the /profile endpoint correctly returns a 401 instead of a 503. However, this has led to a double modal being presented to the user, that persists even after they finish signing in. The unauthorizedInterceptor just checks to see if the request status was a 401 and if the user currently has a crsToken cookie(which most likely has expired). If so, show the modal. So when the user's token expired, they get presented a modal for the first request that was unauthorized, and then a second modal for the profile data request. I figured we don't necessarily need to have them sign in to get their profile data, as even if they were on the landing page(a page that does not require auth), the modal would pop up. Maybe this would be resolved if we removed the crsToken if we hit a 401 error(i.e always delete the crsToken if 401, but only show modal if the request was not for profile data)? Let me know what you think!

wrandall22 commented 2 years ago

My initial thoughts:

  1. It is often hard to maintain things like if not x endpoint for handling logic like this (these lists tend to be always growing)
  2. Is there any way for the front-end to verify if the crsToken is expired?
  3. If we were to delete crsToken on 401 response, how easy is it to get a new one that is not expired?
  4. I'm not quite sure I understand fully why two modals are popping up in the first place. Are we forcing a modal every time they go to get profile data regardless of the logged-in status, and before the 401 comes back?
TheNoodleMoose commented 2 years ago

@wrandall22 Thanks for looking at this and providing feedback! I'll try to go ahead and reply back to each point you made.

  1. I agree. This seemed to be the fastest way to resolve this issue. Not sure how long it would take to rework the logic to prevent this from happening.
  2. Possibly via a library maybe?
  3. If we delete it, it would require the user to sign in. I believe once it expires, the authentication process needs to take place again.
  4. There's an interceptor that listens to all requests, and if it gets a 401 AND the crsToken exists(regardless of being expired), it just displays the login modal. So on every page load, it's requesting the data for that page, and the profile data. So if both requests require a valid token, both will get a 401 and two modals will popup. Maybe we can delete the crsToken if it runs into a 401 the first time. But I'm not sure if that has any effects on other logic that is in place. There are several places where regardless of logged-in status, we are requesting the profile data(I believe it's related to analytics). I could investigate cleaning that up, but not sure how long/how much effort that will take to do so.
wrandall22 commented 2 years ago

I guess the part that seems to need to be smarter is this:

So on every page load, it's requesting the data for that page, and the profile data.

The profile page should be smart enough to know that the data for that page = the profile data.

TheNoodleMoose commented 2 years ago

@wrandall22 There's actually no profile page, it's just an endpoint that holds user profile data that is updated based on their answers to questions like address, name, email, etc. The front end, however, only requests this data. All updating happens on the API side. But we seem to be requesting that profile data on every page load, route change, etc.

There's actually logic that will not fetch the profile data if there is no crsToken, but that token does not get deleted on a 401, so the request happens: https://github.com/CruGlobal/conf-registration-web/blob/36f56a001b35b14965eec6a2dcc7857713586864/app/scripts/services/ProfileCache.js#L23

wrandall22 commented 2 years ago

Ah, I see. I guess this is where GraphQL could help. One request that gets page data + profile data. Otherwise it would be slowing down the page by requesting them sequentially. Probably lots of ways it could go to be a good solution.