Attempt to fix login redirect behaviors #211
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mfisher87
commented
Dec 18, 2023
| assert resp.ok, resp.text | ||
|
|
||
| user = ensure_user_exists(resp.json()) | ||
| login_user(user) |
There was a problem hiding this comment.
@rmarow I think our problem is here! When the user hits this endpoint, they get redirected to the google login endpoint and never reach this code that actually logs them in from the application's perspective (login_user)
Must be tested behind proxy to know if it works. I identified two problems here: 1) `flask-dance` was redirecting to `/` by default, but the app in production is hosted behind a proxy at `/apps/benefit-tool`. We need to generate the correct URL with `url_for` for the proxy to be taken in to account. 2) The old login route needs to be hit twice to actually log in a user. The first time through, it sends the user to Google, and at the end of that, the user winds back up in the app without having hit the `login_user()` call. At this point, Google has authorized the user, so when they hit `/login` again, they'll hit the `login_user()` call. Both of these problems are addressed by hooking the `oauth_authorized` signal (https://flask-dance.readthedocs.io/en/latest/signals.html) and moving the login & user-insertion behavior there. Resolves #178 Resolves #58
mfisher87
force-pushed
the
issue-178-login-redirect
branch
from
bf7cc0f
to
3334099
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Must be tested behind proxy to know if it works. I identified two problems here:
flask-dancewas redirecting to/by default, but the app in production is hosted behind a proxy at/apps/benefit-tool. We need to generate the correct URL withurl_forfor the proxy to be takenin to account.
login_user()call. At this point, Google has authorized the user, so when they hit/loginagain, they'll hit thelogin_user()call.Both of these problems are addressed by hooking the
oauth_authorizedsignal (https://flask-dance.readthedocs.io/en/latest/signals.html) and moving the login & user-insertion behavior there.Resolves #178
Resolves #58
馃摎 Documentation preview 馃摎: https://usaon-benefit-tool--211.org.readthedocs.build/en/211/