-
Notifications
You must be signed in to change notification settings - Fork 33
fix: returnUrl from back end was being discarded #1807
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
base: master
Are you sure you want to change the base?
Conversation
feat: improve dynamic dataset detail view (SciCatProject#1805)
…racted and used. Instead, a value of "returnUrl" was being set using front end browsing history.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @GBirkel - I've reviewed your changes - here's some feedback:
Overall Comments:
- It might be clearer to use a constant for the query parameter name
returnUrl
to avoid typos and improve maintainability.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Proposal to discard this PR and the related backend PR: SciCatProject/scicat-backend-next#1815. The feature works as expected, please get in touch if you are having issues with the exisiting implementation. |
Fine by me. |
Are we all in agreement to close it? |
This feature definitely didn't work for me until I made the above changes, along with the accompanying back-end PR. I could not successfully pass a return URL from the front end to the back end and have it used in a redirect, and as a result, users who were unauthenticated were always being dropped at the default starting location rather than where they'd originally been. |
That said, if it is breaking current deployments, go ahead and back it out. I'm using a fork for my own deployment and will just avoid pulling in master until I have another look at the situation. |
} else { | ||
// A returnUrl coming from v4 should be respected as the destination redirect | ||
// after login and user info fetching. | ||
this.returnUrl = params["returnUrl"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tested, the returnUrl was only being respected when OIDC_SUCCESS_URL was /auth-callback. This does fix the bug when OIDC_SUCCESS_URL is /login, thanks!
Added my comment about /auth-callback vs /login. Can confirm the feature did not work when OIDC_SUCCESS_URL set to /login, and only worked when it was /auth-callback. Sorry for the confusion. Also in favor to merge this PR. |
Description
The "returnUrl" value passed to/from the back end was being ignored at the end of the journey. These minor changes detect a "returnUrl" that's part of a successful OIDC login from the v4 back end, and use the value in subsequent navigation after the login page.
Motivation
Together with the changes in SciCatProject/scicat-backend-next#1815 , this makes "returnUrl" work as designed.
Fixes / Changes:
Detecting and extracting "returnUrl" in a successful OIDC redirect. Using "returnUrl" in place of "returnURL" for consistency.
Tests included
Tests pass as before.
Documentation
No documentation changes needed.
Backend version
No specific back end is mandatory, but for "returnUrl" to fully work, the changes in SciCatProject/scicat-backend-next#1815 should be integrated as well.
Summary by Sourcery
Improve handling of returnUrl during OIDC authentication to ensure consistent redirect behavior across different backend versions
Bug Fixes:
Enhancements: