Skip to content

Conversation

@silva-fj
Copy link
Contributor

@silva-fj silva-fj commented Oct 13, 2025

Summary

This PR adds Google OAuth2 authentication support with per-client credential configuration:

  • Client-specific OAuth2 credentials: Configure separate Google OAuth2 credentials per client via environment variables (OE_GOOGLE_CLIENT_ID_{CLIENT}, OE_GOOGLE_CLIENT_SECRET_{CLIENT})
  • GoogleOAuth2Factory: Factory pattern with caching to retrieve client-specific OAuth2 configurations
  • New RPC method: omni_loginWithOAuth2 accepts OAuth2 authorization code and returns user identity + JWT access token
  • Refactored verification: OAuth2 verification now returns verified Identity from provider, enabling identity extraction without re-verification

This new flow will allow clients to create the omni account and add passkeys using the access token after signing in with the oauth2 provider

Add GoogleOAuth2Config struct and loading logic to support
per-client Google OAuth2 credentials via environment variables
(OE_GOOGLE_CLIENT_ID_{CLIENT}, OE_GOOGLE_CLIENT_SECRET_{CLIENT}).
Create factory pattern to retrieve client-specific Google OAuth2
credentials with caching, following the same pattern as MailerFactory.
Replace direct google_client_id/secret fields with
google_oauth2_factory to enable per-client OAuth2 credentials.
- Use (client_id, uid) tuple encoding for storage keys
- Extract client_id from OmniAuth::OAuth2 enum
- Retrieve client-specific Google OAuth2 config from factory
- Add uid field to OAuth2Data for proper key generation
- Remove sender parameter from verify_oauth2_authentication
- Return verified Identity instead of unit type
- Move identity comparison to verify_auth caller
- Enables direct access to OAuth2-verified identity
- Accept OAuth2 provider, code, state, and redirect_uri
- Verify OAuth2 authentication and extract user identity
- Create JWT access token for authenticated user
- Return user identity string and access token
- Support extensible provider mapping (currently Google)
@linear
Copy link

linear bot commented Oct 13, 2025

P-1706 login with google

@claude
Copy link

claude bot commented Oct 13, 2025

Claude finished @silva-fj's task —— View job


Pull Request Review

I've completed a thorough review of the Google OAuth2 implementation. Here's my comprehensive feedback:

Code Quality Analysis

Strengths
  1. Well-structured factory pattern: The GoogleOAuth2Factory implementation is clean with proper caching using RwLock
  2. Client-specific configuration: Smart approach using environment variables with {CLIENT} suffix pattern
  3. Proper error handling: Comprehensive error propagation with context in most areas
  4. Good separation of concerns: Clear separation between config loading, factory, and RPC methods
  5. Consistent patterns: Follows established patterns in the codebase for authentication methods
⚠️ Areas for Improvement

Security Considerations

🔴 Critical: OAuth2 client secrets are not marked as sensitive in logging:

  • config.rs:498 - Google OAuth2 config loading logs client info but doesn't mark secrets as sensitive
  • Recommendation: Update load_google_oauth2_configs() to avoid logging sensitive credential information

🟡 Medium: Missing rate limiting for OAuth2 endpoints:

  • No rate limiting visible for omni_loginWithOAuth2 or omni_getOAuth2GoogleAuthorizationUrl
  • Recommendation: Consider implementing rate limiting to prevent abuse

Code Issues

🟡 State cleanup: In verify_auth.rs:176-184, the OAuth2 state verifier is checked but there's no cleanup of used state tokens, potentially allowing replay attacks.

🟡 Error information leakage: google_oauth2_factory.rs:29-34 exposes available clients in error messages, which could be sensitive information.

🟠 Minor: login_with_oauth2.rs:143-150 has verbose error handling that could be simplified.

Performance Considerations

✅ Good caching implementation: The factory uses proper read-write locking for config caching.

🟡 Potential improvement: config.rs:463-471 creates a new HashSet for client discovery on every config load. Consider caching this as well.

Test Coverage

🟡 Limited test coverage:

  • google_oauth2_factory.rs has basic tests for caching and missing clients
  • Missing tests for:
    • OAuth2 state verification flow
    • JWT token creation with different Identity types
    • Error scenarios in the login flow
    • Config loading with various environment variable combinations

Best Practices & Recommendations

  1. Environment variable validation: Add validation for OAuth2 config completeness at startup
  2. State token expiration: Implement expiration for stored OAuth2 state tokens
  3. Logging improvements: Reduce sensitive information in logs
  4. Documentation: Consider adding inline documentation for the OAuth2 flow

Minor Issues

  • verify_auth.rs:82 - Passkey authentication marked as todo!()
  • Several TODO comments throughout the codebase (not related to this PR)

Overall Assessment

This is a solid implementation that follows good patterns and provides the necessary functionality. The main concerns are around security logging and state token management. The code is production-ready with the recommended security improvements.

Recommendation: Approve with minor security fixes for sensitive data logging.


@silva-fj silva-fj requested a review from a team October 14, 2025 14:13
@silva-fj silva-fj merged commit b822828 into dev Oct 14, 2025
20 checks passed
@silva-fj silva-fj deleted the p-1706-login-with-google branch October 14, 2025 15:58
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.

3 participants