Skip to content

Conversation

@smarcet
Copy link
Collaborator

@smarcet smarcet commented Oct 29, 2025

@smarcet smarcet force-pushed the feature/auth0-3rd-party-provider branch 5 times, most recently from 6f122e1 to a954e36 Compare October 29, 2025 19:35
chore: define custom provider for LFID
@smarcet smarcet force-pushed the feature/auth0-3rd-party-provider branch from a954e36 to 31eb023 Compare October 29, 2025 19:54
@smarcet smarcet requested a review from Copilot November 5, 2025 17:14
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for Auth0 and Zoho social login providers with multi-tenancy capabilities. The implementation introduces a tenant-based system to control which OAuth providers are available for specific tenants.

Key Changes:

  • Added Auth0 (LFID) and Zoho social login provider packages to support additional authentication methods
  • Implemented tenant-based provider filtering logic that restricts available social login providers based on tenant configuration
  • Added tenant parameter support in OAuth2 authorization flow to enable tenant-specific authentication

Reviewed Changes

Copilot reviewed 11 out of 13 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
config/tenants.php New configuration file defining tenant-specific allowed 3rd party providers
config/services.php Added custom Auth0 tenant configuration (LFID) with environment-based settings
composer.json Added socialiteproviders/auth0 and socialiteproviders/zoho packages, alphabetically sorted dependencies
composer.lock Lock file updates reflecting new package installations
app/libs/Auth/SocialLoginProviders.php Enhanced buildSupportedProviders with tenant filtering logic and added LFID constant
app/libs/OAuth2/OAuth2Protocol.php Added Tenant constant for tenant parameter support
app/libs/OAuth2/Requests/OAuth2AuthorizationRequest.php Added getTenant() method to retrieve tenant from request
app/Strategies/OAuth2LoginStrategy.php Modified to extract and pass tenant parameter through login flow
app/Strategies/DisplayResponseUserAgentStrategy.php Updated to pass tenant to buildSupportedProviders
app/libs/OAuth2/Discovery/DiscoveryDocumentBuilder.php Updated to use refactored buildSupportedProviders method
app/Strategies/OTP/OTPChannelEmailStrategy.php Added email verification check before generating password reset link
app/Providers/AppServiceProvider.php Registered LFID as Auth0 provider via Socialite event listener
.gitignore Simplified patterns for ignoring source map files

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

);

// If no tenant param was provided, any enabled provider is allowed.
if ($tenant === '' && count($tenants)==0) {
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

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

Missing space around the '==' operator. Should be 'count($tenants) == 0' for consistency with project formatting standards.

Suggested change
if ($tenant === '' && count($tenants)==0) {
if ($tenant === '' && count($tenants) == 0) {

Copilot uses AI. Check for mistakes.
continue;
}
// 2. check if the tenant has that provider enabled
if (!count($tenants) && !in_array($provider, $allowed_3rd_party_providers)) {
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

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

Using '!count($tenants)' is less explicit than 'count($tenants) === 0' or 'empty($tenants)'. For consistency with line 78, consider using 'count($tenants) == 0' or the more idiomatic 'empty($tenants)'.

Suggested change
if (!count($tenants) && !in_array($provider, $allowed_3rd_party_providers)) {
if (count($tenants) == 0 && !in_array($provider, $allowed_3rd_party_providers)) {

Copilot uses AI. Check for mistakes.
$provider = $data["provider"] ?? null;

$provided_tenant = $data["tenant"] ?? '';
Log::debug("OAuth2LoginStrategy::getLogin", ['provider' => $provider , 'provided_tenant' => $provided_tenant]);
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

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

The log message references 'OAuth2LoginStrategy::getLogin' but this is in 'DisplayResponseUserAgentStrategy::getLoginResponse'. The class/method name should be updated to 'DisplayResponseUserAgentStrategy::getLoginResponse' for accurate logging.

Suggested change
Log::debug("OAuth2LoginStrategy::getLogin", ['provider' => $provider , 'provided_tenant' => $provided_tenant]);
Log::debug("DisplayResponseUserAgentStrategy::getLoginResponse", ['provider' => $provider , 'provided_tenant' => $provided_tenant]);

Copilot uses AI. Check for mistakes.
'client_secret' => env('LFID_CLIENT_SECRET'),
'redirect' => env('LFID_REDIRECT_URI'),
'base_url' => env('LFID_BASE_URL'),
'tenants' => env('LFID_TENANTS','lf'),
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

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

Missing space after the comma in the env() default value. Should be env('LFID_TENANTS', 'lf') for consistent formatting.

Suggested change
'tenants' => env('LFID_TENANTS','lf'),
'tenants' => env('LFID_TENANTS', 'lf'),

Copilot uses AI. Check for mistakes.
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