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

temp: Add configuration option to redirect to external site when TPA account is unlinked #540

Merged

Conversation

xitij2000
Copy link
Member

@xitij2000 xitij2000 commented May 24, 2023

Testing instructions

  1. Add the dummy OAuth provider
  2. Try logging in using the dummy oauth provider
  3. You will see an error message about an unlinked TPA account, asking you to log in.
  4. Check out this branch
  5. Add a site config variable called OC_REDIRECT_ON_TPA_UNLINKED_ACCOUNT with the URL to redirect to.
  6. Clear cookies or us a fresh browser session, and try logging in with the dummy provider again.
  7. You should be redirected to the URl set above.

Private-ref: BB-7394

Copy link
Member

@kaustavb12 kaustavb12 left a comment

Choose a reason for hiding this comment

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

👍

Approved, subject to the resolution of the review comments

  • I tested this: Tested in a sandbox.
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation
  • I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.

@@ -99,7 +99,13 @@ def _do_third_party_auth(request):
)
)

raise AuthFailedError(message, error_code='third-party-auth-with-no-linked-account') # lint-amnesty, pylint: disable=raise-missing-from
redirect_url = configuration_helpers.get_value('OC_REDIRECT_ON_TPA_UNLINKED_ACCOUNT', None)
Copy link
Member

Choose a reason for hiding this comment

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

I think we should leave a comment here explaining the motivation for this change and that this is intended to be a temporary change. We should also indicate this in the PR commit message, for the benefit of the person preparing the next common branch.

@xitij2000 xitij2000 force-pushed the kshitij/redirect-on-tpa-unlinked branch 2 times, most recently from 0bd8531 to 7a37621 Compare May 25, 2023 17:22
@xitij2000 xitij2000 force-pushed the kshitij/redirect-on-tpa-unlinked branch from 7a37621 to 91f8d36 Compare May 26, 2023 06:26
@xitij2000 xitij2000 merged commit b63af53 into opencraft-release/nutmeg.2 May 29, 2023
@xitij2000 xitij2000 deleted the kshitij/redirect-on-tpa-unlinked branch May 29, 2023 06:29
@Agrendalath
Copy link
Member

@xitij2000, please add a ticket number to the PR. Also, could you ensure we have a follow-up to create a permanent solution?

@0x29a
Copy link
Member

0x29a commented Apr 10, 2024

Hey @xitij2000, would you like me to create a follow-up ticket to create a permanent solution for this? Redwood isn't cut yet, so it's a good chance to get rid of this code drift altogether.

@kaustavb12
Copy link
Member

@0x29a I believe this PR via BB-7742 is the permanent solution for this problem. We won't need the changes here once that is merged and we switch to AuthN MFE. cc. @xitij2000

@0x29a
Copy link
Member

0x29a commented Apr 10, 2024

Thanks for letting me know, @kaustavb12. I'll mention this in the code drift project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants