-
Notifications
You must be signed in to change notification settings - Fork 153
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
feat: redirect to custom URL when third-party auth account is unlinked #1078
base: master
Are you sure you want to change the base?
feat: redirect to custom URL when third-party auth account is unlinked #1078
Conversation
Thanks for the pull request, @ArturGaspar! What's next?Please work through the following steps to get your changes 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. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by Where 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. |
90eac4e
to
1acae39
Compare
830c2bc
to
43efc72
Compare
06687b9
to
f4d1db5
Compare
f4d1db5
to
ba65290
Compare
Hi @openedx/vanguards! This is ready for review. |
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.
- I tested this: Followed the testing instructions and verified that on TPA unlinked state, the user is redirected to the external url set in
MFE_CONFIG
. - I read through the code
- I checked for accessibility issues
- Includes documentation
@@ -19,6 +19,7 @@ const configuration = { | |||
SEARCH_CATALOG_URL: process.env.SEARCH_CATALOG_URL || null, | |||
TOS_AND_HONOR_CODE: process.env.TOS_AND_HONOR_CODE || null, | |||
TOS_LINK: process.env.TOS_LINK || null, | |||
TPA_UNLINKED_ACCOUNT_PROVISION_URL: process.env.TPA_UNLINKED_ACCOUNT_PROVISION_URL || null, |
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.
@ArturGaspar Is the TPA_UNLINKED_ACCOUNT_PROVISION_URL
required here and in the .env
? I tested without setting the value in the .env and the change works fine as long as the MFE_CONFIG
has this information.
I guess it doesn't hurt to have a default value here, maybe we could document this better somewhere?
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.
@tecoholic, here it is necessary because someone might want to enable this feature without the MFE_CONFIG API. In .env it is not necessary (as this file defaults it to null) but it seemed appropriate to have it there for reference.
Note for core reviewers: The typical use case for this is when an organization wants to do registration of a user across multiple services or use an external service to collect extra registration data that is not available from the TPA provider. So, when the user lands without an account, the get redirected to the external service specified in the |
@@ -119,6 +119,10 @@ The authentication micro-frontend also requires the following additional variabl | |||
- Enables the image layout feature within the authn. When set to True, this feature allows the inclusion of images in the base container layout. For more details on configuring this feature, please refer to the `Modifying base container <docs/how_tos/modifying_base_container.rst>`_. | |||
- ``true`` | ``''`` (empty strings are falsy) | |||
|
|||
* - ``TPA_UNLINKED_ACCOUNT_PROVISION_URL`` | |||
- URL to redirect to when the identity provided by third-party authentication is not yet linked to a platform account. This allows for redirecting to a custom sign-up flow handled by an external service to create the linked account. An empty string (the default) disables this feature. |
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.
Apologies for holding this up, but it's still not clear to me how an account can get in this situation. It would be a big help if you could describe the logistration flow that benefits from this (maybe with a diagram?), and why one of the existing flows is not sufficient.
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.
On a more general note, shouldn't this be a per-provider thing, as opposed to a single URL for everything?
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.
@arbrandes In this particular use case, we don't want registration to be handled by Open edX from third-party auth (it will be handled by an external service distinct from the third-party auth provider which then creates the appropriate user account in the Open edX platform via API calls), so if the user tries to log in via the third-party auth we end up in the situation where Open edX has third-party auth but doesn't know which user account it should link to (because the account for this user was not created yet, and we don't want Open edX to create one automatically). Instead of the default behaviour of prompting the user to manually link an existing account, this would then redirect to the aforementioned external service.
While the concrete use case only has one provider, I think it still makes sense to have a single URL if there were more.
If this explanation is sensible enough, I can update the readme.
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.
Hi @arbrandes. Would you know when you might be able to look into my explanation?
Sandbox deployment failed 💥 |
Sandbox deployment successful 🚀 |
Sandbox deployment successful 🚀 |
Sandbox deployment successful 🚀 |
Sandbox deployment successful 🚀 |
d813aea
to
1fdb02e
Compare
Sandbox deployment successful 🚀 |
Sandbox deployment successful 🚀 |
Sandbox deployment failed 💥 |
Sandbox deployment successful 🚀 |
Sandbox deployment successful 🚀 |
Hi @ArturGaspar! Just checking on this. Are the requested changes ready for review? |
@mphilbrick211 Yes, please see discussion at #1078 (comment) |
@arbrandes @openedx/2u-vanguards this is ready! |
Sandbox deployment successful 🚀 |
Sandbox deployment successful 🚀 |
Hey @arbrandes, do you have any updates on when you'll be able to complete a final round of review here? |
Sandbox deployment successful 🚀 |
Sandbox deployment successful 🚀 |
Sandbox deployment successful 🚀 |
Settings
Description
Allow redirecting to a custom URL when signing in via third-party auth when the account is not linked.
How Has This Been Tested?
ENABLE_MFE_CONFIG_API = True
ENABLE_COMBINED_LOGIN_REGISTRATION = True
ENABLE_THIRD_PARTY_AUTH = True
AUTHN_MICROFRONTEND_URL = 'http://localhost:1999'
MFE_CONFIG = {"TPA_UNLINKED_ACCOUNT_PROVISION_URL": "http://example.com"}
'ENABLE_THIRD_PARTY_AUTH': True
and'ENABLE_AUTHN_MICROFRONTEND': True
in FEATURES dict in LMS settings (edx-platform/lms/envs/common.py
).MFE_CONFIG_API_URL='http://localhost:18000/api/mfe_config/v1'
in frontend-app-authn (e.g. in.env.development
).The login page will briefly show before redirect but I see no way to prevent this as the request to http://localhost:18000/api/mfe_context is made asynchronously and we don't know the third-party auth context until it is made, but the login page can be rendered before that.
Merge Checklist
Post-merge Checklist