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

Bug: AUTH_COOKIE user_id persist as long as the server runs #94

Open
mtreacy002 opened this issue Jul 23, 2020 · 12 comments
Open

Bug: AUTH_COOKIE user_id persist as long as the server runs #94

mtreacy002 opened this issue Jul 23, 2020 · 12 comments
Labels
Category: Coding Changes to code base or refactored code that doesn't fix a bug. Category: Outreach/Research Changes or improvements related to external research of the app. Status: Available Issue was approved and available to claim or abandoned for over 3 days.

Comments

@mtreacy002
Copy link
Member

Describe the bug
AUTH_COOKIE user_id from the previous logged in user persist and can be used by the next logged in user to get previous logged in user information

To Reproduce
Steps to reproduce the behavior:

  1. Make sure you have 2 users in the database (if you don't have them, create and confirm email verifications). Here I have test0101 and test0202
    Screen Shot 2020-07-23 at 11 53 04 am

  2. Make sure one of your user has created an additional information and the other has none. Here I have user test0101 has an additional information
    Screen Shot 2020-07-23 at 11 53 11 am

  3. Logged in as test0101, get details by sending GET /user/peersonal_details which will set the AUTH_COOKIE user_id. Then view additional information, which should be returning additional information for user_id 2 (test0101 user_id)

  4. Now login as test0202 and try get user additional information (which supposed to be for test0202) without sending GET /user/personal_details for user test0202. Notice that the api will responded by returning user test0101 additional information since the AUTH_COOKIE user_id still has the user test0101 user id.

  5. Only when test0202 send the GET /user/personal_details this AUTH_COOKIE will change to test0202 user_id (== 3) then when you try send GET /user/additional_information request for user test0202, it will return with the proper error message : "No additional information found with your data. Please provide them now."

ezgif com-video-to-gif (5)

Expected behavior
Only authenticated user should be allowed/able to retrieve their own user additional_info with GET/user/additional_info request.

Screenshots
see gifs above

Desktop (please complete the following information):

  • OS: MAC OS
  • Browser Safari
  • Version ?

Additional context
TO DO: upon login, previous AUTH_COOKIE user_id (if any) need to be removed.

@mtreacy002 mtreacy002 self-assigned this Jul 23, 2020
@mtreacy002 mtreacy002 added Category: Coding Changes to code base or refactored code that doesn't fix a bug. Program: GSOC Related to work completed during the Google Summer of Code Program. labels Jul 23, 2020
@mtreacy002
Copy link
Member Author

mtreacy002 commented Jul 23, 2020

What: When a user logged in and retrieved their personal details through GET /user/personal_details, the user_id is kept as AUTH_COOKIE user_id. This cookie persist even though the next user logged in, until that new logged in user reetrieved their personal details.

Why is this a problem?

  • The new logged in user will be able to retrieved the previous user information even with new token is provided (since new token is not attached to the user id).
  • If the AUTH_COOKIE is kept at backend in this manner, when the app is deployed to remote server where many users login at the same time, then having AUTH_COOKIE user_id at backend doesn't make sense.

Potential solution?
Option 1. keep AUTH_COOKIE at frontend and send it as header if sending request to BIT REST API. Keep token cookie as header only when request is related to MS REST API.

Option 2. Originally thought removing previous AUTH_COOKIE usre_id through POST /login might be an option, but if the app deployed remotely and many users login, the issue will persist. (I personally don't think this is a good option)

Option 3. Probably create BIT JWT token specific to BIT requests. Keep the 2 tokens (BIT and MS) as cookies and use them accordingly, BIT token for BIT requests, MS token for MS requests. Not sure how to do it, but that sounds the most logical option. Thoughts anyone?

@anitab-org/bridgeintech-maintainers . Do you have any suggestion?

@mtreacy002 mtreacy002 added Status: On Hold Issue or PR needs more info, a discussion, a review or approval from a Maintainer/Code Owner. and removed Program: GSOC Related to work completed during the Google Summer of Code Program. labels Jul 25, 2020
@mtreacy002 mtreacy002 added the Category: Outreach/Research Changes or improvements related to external research of the app. label Aug 31, 2020
@mtreacy002 mtreacy002 removed the Status: On Hold Issue or PR needs more info, a discussion, a review or approval from a Maintainer/Code Owner. label Feb 20, 2021
@mtreacy002 mtreacy002 removed their assignment Feb 28, 2021
@Rahulm2310
Copy link
Contributor

@mtreacy002 I don't understand why are we not removing the auth-cookie on user logout ? 🤔

@b-thebest
Copy link

Can I work on this issue ?

@mtreacy002
Copy link
Member Author

Sure, @b-thebest. I'll assign this to you now. Thanks for picking up the issue 😉.

@mtreacy002
Copy link
Member Author

mtreacy002 commented Mar 4, 2021

@mtreacy002 I don't understand why are we not removing the auth-cookie on user logout ? 🤔

@Rahulm2310 . The MS backend that deals with authentication has no logout feature, that's also why the BIT backend doesn't have this feature.
Only at frontend (both MS and BIT) we're removing auth-cookie on user logout.

@b-thebest
Copy link

@mtreacy002 I was trying to understand what else needed to be done from now in this project. Could you please help me understand, what should be done actually after these merge requests.

@mtreacy002
Copy link
Member Author

mtreacy002 commented Mar 8, 2021

After these merge requests , @b-thebest ? I’m not following. Do you mean what you need to do on this PR? If so, Please check the “expected behaviour” and the “additional context” on the issue description for what is expected from this PR 😉

@mtreacy002
Copy link
Member Author

@b-thebest , can you please give an update on this?

@b-thebest
Copy link

@mtreacy002 I am working on this issue, need some help

@mtreacy002
Copy link
Member Author

mtreacy002 commented Mar 13, 2021

Sure, can you tell me what issue you're facing atm, @b-thebest?

@mtreacy002
Copy link
Member Author

mtreacy002 commented Mar 17, 2021

@b-thebest , how are you going with your issue? Are you able to solve the issue?

@mtreacy002
Copy link
Member Author

I'm making this issue available again as there is no response received from the contributor.

@mtreacy002 mtreacy002 added the Status: Available Issue was approved and available to claim or abandoned for over 3 days. label Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Coding Changes to code base or refactored code that doesn't fix a bug. Category: Outreach/Research Changes or improvements related to external research of the app. Status: Available Issue was approved and available to claim or abandoned for over 3 days.
Projects
None yet
Development

No branches or pull requests

4 participants