fix: redirect to account MFE when using any legacy account URL - #36894
Conversation
Redirect to the account MFE URL configured each time a legacy account URL like http(s)://lms/account/ or http(s)://lms/account/settings is used to avoid 404 errors while linking SSO accounts or simply trying to access the account view via URLs.
|
Thanks for the pull request, @mariajgrimaldi! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
|
Sandbox deployment successful 🚀 |
|
|
||
| urlpatterns = [ | ||
| re_path(r'^account(?:/settings)?/?$', RedirectView.as_view(url=settings.ACCOUNT_MICROFRONTEND_URL)), | ||
| path('user_api/v1/', include(USER_API_ROUTER.urls)), |
There was a problem hiding this comment.
Why you made the suffix /settings in url path as optional? i
There was a problem hiding this comment.
It looks like LMS/account also returns 404 which I think should also redirect to the account mfe. What do you think?
There was a problem hiding this comment.
Note sure if it's suppose to redierct from /account, AFAIK lms_base/account on sumac didn't redirect to account mfe.
There was a problem hiding this comment.
Mmm I see what you say. But when using LMS base domain == MFE domain, /account will redirect to the account MFE. Does that make sense?
There was a problem hiding this comment.
I made a correction in my earlier comment, I meant from lms.base/account
Mmm I see what you say. But when using LMS base domain == MFE domain, /account will redirect to the account MFE. Does that make sense?
when/if (LMS base domain == MFE domain), then it will a add new diemension to the problem, of which I haven't tried or setup before. Is there a sandbox with such setup?
There was a problem hiding this comment.
edunext has a few with that setup, it has another issues I think (not related to this, but to MFEs themselves) but all of them are solved here https://github.com/eduNEXT/tutor-contrib-mfe-extensions
There was a problem hiding this comment.
Aha gotcha... I think then may be since redirection is at caddy level, it would get redirected before reaching django urls roles.
Nonetheless, the project provide an intresting solution for the multiesite and MFEs.
|
@BryanttV: since you were able to reproduce this issue, can you help me testing it in the remote environment? Credentials are: |
|
Hi @mariajgrimaldi!, I have just tested, and the redirection works correctly, both when accessing |
|
Thanks for reviewing, @BryanttV!
I agree that we should remove the references to all deprecated endpoints where it's possible, but I'm not sure how much that would impact other authentication flows from other providers. I'd prefer the most straightforward approach while we coordinate how to entirely deprecate the old URLs. This might be a simple change, but we don't have a lot of time for the release. If how well this is done becomes a blocker for merging this, then we can discuss how the other solution might look like. I think replacing the redirects to be constructed considering the MFE endpoints instead of the default backed configurations would need to be done. Another approach would be to create a DEPR ticket to properly investigate this issue instead of rushing a fix mid-release that could break other things. But that's just my take - I'm definitely open to hearing other solutions! |
| USER_API_ROUTER.register(r'user_prefs', user_api_views.UserPreferenceViewSet) | ||
|
|
||
| urlpatterns = [ | ||
| re_path(r'^account(?:/settings)?/?$', RedirectView.as_view(url=settings.ACCOUNT_MICROFRONTEND_URL)), |
There was a problem hiding this comment.
Please add a comment here explaining why this exists. I'm generally fine with adding backwards compatibility redirects, even if we've already ripped out references to it in our own codebase (because URLs stick around for a long time in other systems).
There was a problem hiding this comment.
Thank you, agreed!
|
Sandbox deployment failed 💥 |
|
Sandbox deployment successful 🚀 |
* fix: redirect to account MFE when using any legacy account URL Redirect to the account MFE URL configured each time a legacy account URL like http(s)://lms/account/ or http(s)://lms/account/settings is used to avoid 404 errors while linking SSO accounts or simply trying to access the account view via URLs.
* fix: redirect to account MFE when using any legacy account URL Redirect to the account MFE URL configured each time a legacy account URL like http(s)://lms/account/ or http(s)://lms/account/settings is used to avoid 404 errors while linking SSO accounts or simply trying to access the account view via URLs.
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
|
2U Release Notice: This PR has been deployed to the edX production environment. |
* fix: redirect to account MFE when using any legacy account URL Redirect to the account MFE URL configured each time a legacy account URL like http(s)://lms/account/ or http(s)://lms/account/settings is used to avoid 404 errors while linking SSO accounts or simply trying to access the account view via URLs.
…dx#36894) * fix: redirect to account MFE when using any legacy account URL Redirect to the account MFE URL configured each time a legacy account URL like http(s)://lms/account/ or http(s)://lms/account/settings is used to avoid 404 errors while linking SSO accounts or simply trying to access the account view via URLs.
Description
Redirect to the account MFE URL configured each time a legacy account URL like http(s)://lms/account/ or http(s)://lms/account/settings is used to avoid 404 errors while linking SSO accounts or simply trying to access the account view via URLs.
Supporting information
This attempts to solve #36869
Testing instructions
Go to the account or account/settings page, you must be redirected to the account MFE.
Deadline
ASAP. This is a release blocker we are solving.