Skip to content

Add SSO as a supported auth provider#133

Merged
roymiloh merged 1 commit intomainfrom
add-sso-provider
Feb 25, 2026
Merged

Add SSO as a supported auth provider#133
roymiloh merged 1 commit intomainfrom
add-sso-provider

Conversation

@roymiloh
Copy link
Collaborator

Summary

  • Adds 'sso' as a supported provider in loginWithProvider
  • SSO uses a distinct URL structure: /api/apps/{appId}/auth/sso/login
  • Refactored URL construction to extract shared query params and assemble the final URL from a common prefix/suffix
  • Updated JSDoc in auth.types.ts to document SSO and add a usage example

Test plan

  • Call loginWithProvider('sso', '/dashboard') and verify redirect goes to /api/apps/{appId}/auth/sso/login?app_id=...&from_url=...
  • Verify existing providers (google, microsoft, facebook, apple) still produce the correct URLs

🤖 Generated with Claude Code

SSO uses a different URL structure (/api/apps/{appId}/auth/sso/login) compared to other providers. Refactored URL construction to share query params and assemble the final URL from a common prefix/suffix.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude
Copy link

claude bot commented Feb 24, 2026

Claude finished @roymiloh's task —— View job


Code Review

I've reviewed the pull request that adds SSO as a supported auth provider. Here's my feedback:

✅ Code Quality and Best Practices

Strengths:

  • Clean refactoring of URL construction logic to handle the distinct SSO URL pattern
  • Good separation of concerns with the query params extracted into a common variable
  • Consistent code style matching the existing codebase
  • Clear variable naming (authPath, queryParams)

Suggestions:

  • Consider extracting the supported providers into a constant or enum at the module level for better maintainability

✅ Implementation Logic

The implementation correctly:

  • Preserves existing behavior for google, microsoft, facebook, and apple providers
  • Adds special handling for SSO which uses /api/apps/{appId}/auth/sso/login pattern
  • Maintains backward compatibility with the default / for fromUrl
  • Properly encodes the redirect URL to handle special characters

⚠️ Potential Issues

  1. No validation of provider parameter: The function accepts any string as a provider. Consider adding validation to ensure only supported providers are used:

    const supportedProviders = ['google', 'microsoft', 'facebook', 'apple', 'sso'];
    if (!supportedProviders.includes(provider)) {
      throw new Error(`Unsupported provider: ${provider}`);
    }
  2. Browser-only functionality: The function uses window.location but doesn't check if it's running in a browser environment (unlike redirectToLogin which has this check)

✅ Performance Considerations

  • No performance concerns identified
  • The refactoring maintains the same computational complexity
  • URL construction is efficient with minimal string operations

✅ Security Considerations

  • Proper URL encoding of fromUrl parameter prevents injection attacks
  • No sensitive data exposure
  • Follows the same security pattern as existing providers

❌ Test Coverage

Critical gap: No unit tests exist for the loginWithProvider method. The test file tests/unit/auth.test.js has comprehensive coverage for other auth methods but completely lacks tests for loginWithProvider.

Recommended tests to add:

  1. Test redirect URL generation for each provider (google, microsoft, facebook, apple, sso)
  2. Test default fromUrl parameter behavior
  3. Test URL encoding of special characters in fromUrl
  4. Test behavior in non-browser environment
  5. Test with invalid provider names (if validation is added)

Example test structure:

describe('loginWithProvider()', () => {
  test('should redirect to SSO login with correct URL structure', () => {
    // Mock window.location
    const mockLocation = { href: '' };
    global.window = { location: mockLocation, origin: 'https://example.com' };
    
    base44.auth.loginWithProvider('sso', '/dashboard');
    
    expect(mockLocation.href).toBe(
      `${appBaseUrl}/api/apps/${appId}/auth/sso/login?app_id=${appId}&from_url=${encodeURIComponent('https://example.com/dashboard')}`
    );
  });
  
  test('should redirect to Google login with correct URL structure', () => {
    // Test for Google provider...
  });
  
  // More tests for other providers...
});

📋 Summary

The implementation is clean and follows good practices, but it needs:

  1. Test coverage - This is the most critical issue
  2. Provider validation - To prevent runtime errors with invalid providers
  3. Browser environment check - For consistency with other auth methods

The PR accomplishes its goal of adding SSO support effectively, but adding tests before merging would significantly improve confidence in the implementation.

@github-actions
Copy link

🚀 Package Preview Available!


Install this PR's preview build with npm:

npm i @base44-preview/sdk@0.8.19-pr.133.511000e

Prefer not to change any import paths? Install using npm alias so your code still imports @base44/sdk:

npm i "@base44/sdk@npm:@base44-preview/sdk@0.8.19-pr.133.511000e"

Or add it to your package.json dependencies:

{
  "dependencies": {
    "@base44/sdk": "npm:@base44-preview/sdk@0.8.19-pr.133.511000e"
  }
}

Preview published to npm registry — try new features instantly!

@roymiloh roymiloh merged commit ead5bf3 into main Feb 25, 2026
6 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