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

feat: redirect to custom URL when third-party auth account is unlinked #1078

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ArturGaspar
Copy link

@ArturGaspar ArturGaspar commented Oct 13, 2023

Settings

TUTOR_GROVE_MFE_LMS_COMMON_SETTINGS:
  MFE_CONFIG["TPA_UNLINKED_ACCOUNT_PROVISION_URL"] = "http://example.com/"

TUTOR_GROVE_LMS_ENV_FEATURES: |
  ENABLE_THIRD_PARTY_AUTH: True
  ENABLE_COMBINED_LOGIN_REGISTRATION: True

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?

  1. Set the following settings in the LMS:
    • 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"}
  2. Set 'ENABLE_THIRD_PARTY_AUTH': True and 'ENABLE_AUTHN_MICROFRONTEND': True in FEATURES dict in LMS settings (edx-platform/lms/envs/common.py).
  3. Create a dummy backend Oauth2 provider at http://localhost:18000/admin/third_party_auth/oauth2providerconfig/ and enable its "Visible" setting.
  4. Set MFE_CONFIG_API_URL='http://localhost:18000/api/mfe_config/v1' in frontend-app-authn (e.g. in .env.development).
  5. Go to http://localhost:1999/login.
  6. Sign in with the "Dummy" provider.
  7. See that you are redirected to example.com

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

  • If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Sandbox, if applicable. - N/A
  • Is there adequate test coverage for your changes?

Post-merge Checklist

  • Deploy the changes to prod after verifying on stage or ask @openedx/vanguards to do it.
  • 🎉 🙌 Celebrate! Thanks for your contribution.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 13, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Oct 13, 2023

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 approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

  • If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
    • This process (including the steps you'll need to take) is documented here.
  • If it doesn't, simply proceed with the next step.

🔘 Provide context

To 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:

  • Dependencies

    This PR must be merged before / after / at the same time as ...

  • Blockers

    This PR is waiting for OEP-1234 to be accepted.

  • Timeline information

    This PR must be merged by XX date because ...

  • Partner information

    This is for a course on edx.org.

  • Supporting documentation
  • Relevant Open edX discussion forum threads

🔘 Get a green build

If 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 @openedx/2u-infinity. Tag them in a comment and let them know that your changes are ready for review.

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:

  • The size and impact of the changes that it introduces
  • The need for product review
  • Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

@ArturGaspar ArturGaspar force-pushed the artur/redirect-on-tpa-unlinked branch 2 times, most recently from 90eac4e to 1acae39 Compare October 16, 2023 10:55
@ArturGaspar ArturGaspar marked this pull request as ready for review October 16, 2023 10:57
@ArturGaspar ArturGaspar requested a review from a team as a code owner October 16, 2023 10:57
@ArturGaspar ArturGaspar force-pushed the artur/redirect-on-tpa-unlinked branch 4 times, most recently from 830c2bc to 43efc72 Compare October 16, 2023 17:18
@ArturGaspar ArturGaspar force-pushed the artur/redirect-on-tpa-unlinked branch 3 times, most recently from 06687b9 to f4d1db5 Compare October 23, 2023 17:30
@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Oct 24, 2023
@e0d e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Oct 25, 2023
@mphilbrick211
Copy link

Hi @openedx/vanguards! This is ready for review.

@mphilbrick211 mphilbrick211 added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Oct 25, 2023
Copy link

@tecoholic tecoholic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ArturGaspar 👍

  • 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,

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?

Copy link
Author

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.

@tecoholic
Copy link

tecoholic commented Oct 26, 2023

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 TPA_UNLINKED_ACCOUNT_PROVISION_URL where they enter extra details and then get sent back to the login/registration page.

@mphilbrick211 mphilbrick211 added waiting for eng review PR is ready for review. Review and merge it, or suggest changes. needs test run Author's first PR to this repository, awaiting test authorization from Axim and removed waiting for eng review PR is ready for review. Review and merge it, or suggest changes. labels Oct 26, 2023
@e0d e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Oct 26, 2023
@mphilbrick211 mphilbrick211 added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Oct 27, 2023
@mphilbrick211 mphilbrick211 added needs test run Author's first PR to this repository, awaiting test authorization from Axim and removed waiting for eng review PR is ready for review. Review and merge it, or suggest changes. labels Nov 14, 2023
@@ -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.
Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Author

@ArturGaspar ArturGaspar Jun 10, 2024

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.

Copy link
Author

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?

@mphilbrick211 mphilbrick211 removed the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label May 17, 2024
@itsjeyd itsjeyd added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label May 22, 2024
@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@itsjeyd itsjeyd added waiting for eng review PR is ready for review. Review and merge it, or suggest changes. and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Jun 11, 2024
@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@ArturGaspar ArturGaspar force-pushed the artur/redirect-on-tpa-unlinked branch from d813aea to 1fdb02e Compare July 9, 2024 04:56
@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@mphilbrick211
Copy link

Hi @ArturGaspar! Just checking on this. Are the requested changes ready for review?

@ArturGaspar
Copy link
Author

@mphilbrick211 Yes, please see discussion at #1078 (comment)

@mphilbrick211
Copy link

@arbrandes @openedx/2u-vanguards this is ready!

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@itsjeyd
Copy link

itsjeyd commented Sep 18, 2024

Hey @arbrandes, do you have any updates on when you'll be able to complete a final round of review here?

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
create-sandbox open-craft-grove should create a sandbox environment from this PR open-source-contribution PR author is not from Axim or 2U
Projects
Status: In Eng Review
Development

Successfully merging this pull request may close these issues.

9 participants