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

Update auth cookie logic for both backend and frontend (seamless temp auth) #1974

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

spwoodcock
Copy link
Member

What type of PR is this? (check all applicable)

  • πŸ• Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation
  • πŸ§‘β€πŸ’» Refactor
  • βœ… Test
  • πŸ€– Build or CI
  • ❓ Other (please specify)

Related Issue

Follow on from #1948

Describe this PR

  • This is the final things to fix prior to new release 2024.5.0.
  • Please check over all of my logic & test this thoroughly @Sujanadh @Anuj-Gupta4 @NSUWAL123 @manjitapandey
  • I haven't thoroughly tested this yet, as I planned to merge early and test more easily on dev. It's also getting late for me, so I though it best to just push and let you all help me find errors too!

Content

  • I removed the 'Temporary Login' card / option from the mapper frontend.
  • Now the login is handled seamlessly: if the user is not logged in, they are provided a temporary token/cookie.
  • If the user logs in via OSM, they overrides the temp token (temp cookies are revoked).
  • We should update at some point to inform the user of this:
    • First load of mapper frontend, we have a localStorage key / prompt.
    • If the user is not signed in via OSM, we say: 'You are not signed in via OSM. If you wish for your mapping contributions to be linked to your profile, please log in first'.
  • I made the logic a bit more DRY on the backend.
  • The refresh endpoints refresh both cookies, but also return the user details JSON.

Review Guide

Notes for the reviewer. How to test this change?

Checklist before requesting a review

[optional] What gif best describes this PR or how it makes you feel?

@spwoodcock
Copy link
Member Author

Merging early so this can be tested easily, but please still review + test thoroughly everyone πŸ™

When I wake up, if all is well, I will make the release πŸŽ‰

@spwoodcock spwoodcock merged commit 9a936b2 into development Dec 11, 2024
7 of 9 checks passed
@spwoodcock spwoodcock deleted the feat/seamless-temp-login branch December 11, 2024 01:01
@spwoodcock
Copy link
Member Author

On refresh, the json payload was not returning with AuthUser correctly:

image

Added commit 3a9a50f to address this, now deploying.

Heading to bed, but hopefully it works 🀞 😴

@Anuj-Gupta4
Copy link
Contributor

Anuj-Gupta4 commented Dec 11, 2024

image

@spwoodcock
I tried testing and debugging the code. Findings:

The refresh endpoint for mapper worked at first.
Then I cleared all cookies and logged in.
Now the endpoint has stopped working.

If there are no tokens and mapper frontend is accessed:
Refresh mapper looks for mapper login and the authenticate user function throws error saying token not found before the refresh mapper has a chance to set temporary cookies.

Refresh management endpoint also throws error.

Since same functions are being called with different parameters and response is being passed around a lot, the generated response does not always work with other functions and frontend.

Copy link

sentry-io bot commented Dec 15, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ OperationalError: the connection is closed app.db.models in update View Issue

Did you find this useful? React with a πŸ‘ or πŸ‘Ž

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Related to backend code enhancement New feature or request frontend Related to frontend code
Projects
Development

Successfully merging this pull request may close these issues.

2 participants