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

fix: Handle external URLs correctly in ProviderFactory (Fixes external redirect URL handling) #452

Merged

Conversation

bumblecoder
Copy link
Contributor

Problem Description:
In the current implementation of the createProvider method in the ProviderFactory class, any redirectUri is processed via Symfony's UrlGeneratorInterface::generate. This causes an issue when an absolute URL (external link), such as https://external-site.com/callback, is passed, since generate is only meant to work with Symfony routes. As a result, an error is thrown when trying to handle external URLs. It could be inconvenient if you work with react apps.

Solution:
This feature adds a check to the createProvider method to differentiate between internal routes and external URLs. If the redirectUri is an absolute URL (determined using filter_var with FILTER_VALIDATE_URL), the URL is used directly without invoking the generate method. This prevents calling the route generator for external URLs and fixes the issue.

Changes:
Added a check in ProviderFactory to identify absolute URLs before attempting to generate a Symfony route.
If the redirectUri is an external URL, it is used directly without passing through the UrlGeneratorInterface.
Updated tests to cover cases where external URLs are passed.

Tests:
Added a new test testShouldCreateProviderWithExternalRedirectUrl, which verifies that the generate method is not called for external URLs.
Ran all existing tests to ensure no functionality was broken by this change.

@bumblecoder bumblecoder changed the title Handle external URLs correctly in ProviderFactory (Fixes external redirect URL handling) fix: Handle external URLs correctly in ProviderFactory (Fixes external redirect URL handling) Sep 27, 2024
@bumblecoder bumblecoder marked this pull request as draft September 27, 2024 05:40
@bumblecoder bumblecoder marked this pull request as ready for review September 27, 2024 05:43
@bumblecoder
Copy link
Contributor Author

@bocharsky-bw please review

Copy link
Member

@bocharsky-bw bocharsky-bw left a comment

Choose a reason for hiding this comment

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

The code looks good to me, but why would we need to use external URLs for the $redirectUri argument? Why we can't always use relative URLs?

@bumblecoder
Copy link
Contributor Author

bumblecoder commented Sep 27, 2024

@bocharsky-bw

The code looks good to me, but why would we need to use external URLs for the $redirectUri argument? Why we can't always use relative URLs?

The reason for supporting external URLs in the $redirectUri argument is to provide flexibility for scenarios where redirection might need to occur to a different domain or an external service.

Here are some common cases where external URLs would be necessary:

  1. Multi-domain setups: In situations where the application interacts across multiple domains, such as with external authentication services or subdomains (e.g., redirecting to auth.example.com), we may need to handle external URLs.
  2. Integration with third-party services: For cases like OAuth or payment gateways, where the user might need to be redirected to a third-party service and then back to the application, external URLs become essential.
  3. Dynamic redirect destinations: Sometimes, the destination URL is dynamic or controlled by an external source, and it may not always be relative to the current domain.
  4. Simplifying the logic: Allowing both relative and external URLs provides greater flexibility, avoids unnecessary conversions, and supports use cases that involve cross-domain interaction.
  5. API Platform issue: And personally for me it makes impossible to redirect to my react application that is on different domain. Relative links are not working, moreover, I prefer to use ENV variable in order to specify a redirect domain.

@bumblecoder
Copy link
Contributor Author

@bocharsky-bw , appreciate your response. Can we merge this?

Copy link
Member

@bocharsky-bw bocharsky-bw left a comment

Choose a reason for hiding this comment

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

OK, it sounds like valid reasons, thanks for providing more context behind this change!

P.S. Sorry for the long reply

@bocharsky-bw bocharsky-bw merged commit c38ca88 into knpuniversity:main Oct 2, 2024
8 of 10 checks passed
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.

2 participants