search

knpuniversity / oauth2-client-bundle

Easily talk to an OAuth2 server for social functionality in Symfony
https://symfonycasts.com
MIT License
787 stars 145 forks source link

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

Closed bumblecoder closed 2 months ago

bumblecoder commented 2 months ago

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 commented 2 months ago

@bocharsky-bw please review

bumblecoder commented 2 months ago

@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 commented 2 months ago

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