Skip to content

Conversation

silva-fj
Copy link
Contributor

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

Summary

Add Apple OAuth2 authentication support and refactor OAuth2 flow.

Changes

  • Add Apple as OAuth2 provider (alongside existing Google support)
  • Add Apple identity type to primitives (Identity, UserId, Web2IdentityType)

OAuth2 Flow Refactoring (Implicit Flow)

Security enhancements:

  • Add nonce generation and verification (prevents replay attacks)
  • Verify id_token from client before code exchange (aud + nonce checks)
  • Store both state and nonce in OAuth2VerificationData

API changes:

  • Rename: omni_getOAuth2AuthorizationUrlomni_getOAuth2AuthorizationData
  • Return structured data object (url, state, nonce, scope, etc.) instead of just URL
  • Add id_token parameter to OAuth2Data
  • Remove Identity/sender from OmniAuth::OAuth2 variant

Environment Variables

OE_APPLE_CLIENT_ID_{CLIENT}=<client_id>
OE_APPLE_CLIENT_SECRET_{CLIENT}=<client_secret>

- Add Apple variant to Identity enum (codec index 11)
- Add Apple to Web2IdentityType enum
- Add Apple to UserId enum with conversions
- Add Apple variant to OAuth2Provider enum
- Update identity helpers (to_native_account, to_omni_account, to_did)
- Add AppleOAuth2Config struct to config-loader
- Implement env var loading (OE_APPLE_CLIENT_ID_*, OE_APPLE_CLIENT_SECRET_*)
- Add AppleOAuth2Factory with caching support
- Export AppleOAuth2Config in public API
- Implement AppleOAuth2Client for token exchange
- Add Apple authorization URL helper with scope 'email'
- Add ID token decoder for Apple tokens
- Export apple module from oauth-providers and identity-verification
- Rename omni_getOAuth2GoogleAuthorizationUrl -> omni_getOAuth2AuthorizationUrl
- Add provider parameter to support both Google and Apple
- Update login_with_oauth2 to accept Apple provider
- Add Apple factory to RpcContext
- Update error messages to include both providers
- Implement verify_apple_oauth2 function
- Add state verifier validation for Apple
- Add code-to-token exchange for Apple
- Extract email from Apple ID token
- Update verify_oauth2_authentication to handle Apple provider
Makes OAuth2Provider more ergonomic to use by avoiding unnecessary clones
- Merge GoogleOAuth2Config and AppleOAuth2Config into unified OAuth2Config
- Replace separate config maps with single oauth2_configs map
- Create generic load_oauth2_configs function for all providers
- Add unified get_oauth2_config(client_id, provider) method
- Reduce config loading code by ~100 lines
- Add OAuth2ProviderConfig trait for provider-specific settings
- Implement GoogleProviderConfig and AppleProviderConfig
- Create unified OAuth2Client replacing duplicate implementations
- Remove separate google.rs and apple.rs files (110 lines eliminated)
- Add oauth2_common module with shared AuthorizeData struct
- Create generic get_authorize_data function
- Refactor Google and Apple modules to use shared logic
- Eliminate ~40 lines of duplicated authorization code
- Create OAuth2ConfigFactory supporting all providers
- Remove GoogleOAuth2Factory and AppleOAuth2Factory (157 lines deleted)
- Unified caching with (client_id, provider) key
- Single get_config method for all providers
- Replace verify_google_oauth2 and verify_apple_oauth2 with verify_oauth2_provider
- Eliminate 80+ lines of duplicated verification code
- Share state validation, config fetching, and token exchange logic
- Provider-specific handling only for token decoding
- Update get_oauth2_authorization_url to use single factory
- Replace two factory fields in RpcContext with one oauth2_factory
- Cleaner initialization with single factory instantiation
- Streamline authorization URL generation logic
@linear
Copy link

linear bot commented Oct 15, 2025

P-1707 login with Apple

@claude
Copy link

claude bot commented Oct 15, 2025

Claude encountered an error —— View job


Code Review: Apple OAuth2 Authentication Support

I'll review this PR for code quality, security, performance, and potential issues.

Todo List:

  • Read repository guidelines (CLAUDE.md)
  • Review OAuth2 refactoring architecture
  • Examine Apple-specific implementation
  • Check security considerations
  • Assess performance implications
  • Evaluate test coverage needs
  • Provide comprehensive feedback

@silva-fj silva-fj marked this pull request as ready for review October 15, 2025 13:18
@claude
Copy link

claude bot commented Oct 15, 2025

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


Code Review: Apple OAuth2 Authentication Support ✅

I've completed a comprehensive review of this PR that adds Apple OAuth2 authentication support. The implementation is well-architected and follows security best practices.

Todo List:

  • Read repository guidelines (CLAUDE.md)
  • Review OAuth2 refactoring architecture
  • Examine Apple-specific implementation
  • Check security considerations
  • Assess performance implications
  • Evaluate test coverage needs
  • Provide comprehensive feedback

Strengths

Architecture & Design

  • Excellent refactoring: The unified OAuth2 architecture eliminates code duplication between Google and Apple providers
  • Clean separation of concerns: Provider-specific logic is isolated in modules while sharing common OAuth2 functionality
  • Consistent API: The new unified omni_getOAuth2AuthorizationUrl RPC method works seamlessly for both providers
  • Proper error handling: Comprehensive error handling with detailed error messages and appropriate HTTP status codes

Security Implementation

  • State parameter validation: Proper CSRF protection using cryptographically secure state parameters (32-char alphanumeric)
  • Token verification: JWT token parsing and validation for both Google and Apple ID tokens
  • Secure configuration: Environment variable-based client credentials (OE_APPLE_CLIENT_ID_{CLIENT}, OE_APPLE_CLIENT_SECRET_{CLIENT})
  • Proper scope handling: Minimal scope requests (only "email" for Apple, "openid email" for Google)
  • Identity verification: Email-based identity verification with proper type mapping

Code Quality

  • Type safety: Strong typing throughout with proper enum usage for OAuth2Provider
  • Error propagation: Consistent error handling patterns with detailed context
  • Configuration caching: Smart caching in OAuth2ConfigFactory to avoid repeated config loading
  • Test coverage: Good test coverage for core functionality including edge cases

⚠️ Areas for Improvement

Security Considerations

  1. Token signature verification: The ID token parsing in apple/mod.rs:24-32 and similar Google implementation only decode tokens but don't verify JWT signatures. For production security, consider adding signature verification against Apple's/Google's public keys.

  2. Token expiration: The decoded tokens include exp fields but expiration isn't validated in apple/mod.rs:24-32.

Error Handling

  1. Generic error messages: In apple/mod.rs:27, generic "Invalid token format" could be more specific (e.g., "JWT must have exactly 3 parts separated by dots").

  2. Base64 decoding: The base64_decode function returns "Failed to decode base64" but could provide more context about the specific decoding failure.

Testing

  1. Integration tests missing: While unit tests exist for individual components, there are no integration tests for the complete OAuth2 flow end-to-end.

  2. Apple-specific test coverage: The Apple implementation has minimal test coverage compared to the Google implementation. Consider adding tests for Apple ID token decoding and authorization URL generation.


🔧 Specific Code Issues

tee-worker/omni-executor/heima/identity-verification/src/web2/apple/mod.rs:24-32

pub fn decode_id_token(token: &str) -> Result<IdToken, &'static str> {
    let parts: Vec<&str> = token.split('.').collect();
    if parts.len() != 3 {
        return Err("Invalid token format"); // Too generic
    }
    // Missing signature verification
    let payload = base64_decode(parts[1])?;
    let claims: IdToken = serde_json::from_str(&payload).map_err(|_| "Failed to parse claims")?;
    Ok(claims)
}

Recommendation: Add JWT signature verification for production security.

parachain/primitives/src/identity.rs:560-562

The from_did method is missing Apple support in the DID parsing logic:

} else if v[0] == "google" {
    return Ok(Identity::Google(IdentityString::new(v[1].as_bytes().to_vec())));
} else if v[0] == "pumpx" {
    // Missing Apple case here

Recommendation: Add Apple DID parsing support for consistency.


📊 Performance Assessment

  • Caching strategy: The OAuth2ConfigFactory implements efficient caching to avoid repeated config loading
  • Memory usage: Minimal memory overhead from the unified architecture
  • Network efficiency: Appropriate HTTP client configuration with redirect policy
  • Database operations: Efficient state storage using Blake2 hashing for keys

🧪 Test Coverage Assessment

Good coverage for:

  • OAuth2 factory caching (both Google and Apple)
  • Configuration loading and validation
  • Error handling for missing clients
  • Web3 authentication flows

Missing coverage for:

  • Apple ID token decoding edge cases
  • Complete OAuth2 flow integration tests
  • Authorization URL generation validation
  • Error scenarios in token exchange

🎯 Recommendations

  1. Add JWT signature verification for production security
  2. Implement Apple DID parsing support in identity.rs
  3. Add integration tests for the complete OAuth2 flow
  4. Consider rate limiting for OAuth2 endpoints
  5. Add Apple-specific test cases for better coverage

📋 Summary

This is a well-implemented feature that successfully adds Apple OAuth2 support while improving the overall OAuth2 architecture. The refactoring eliminates code duplication and provides a clean, extensible foundation for future OAuth2 providers.

Security: Generally solid with proper state validation and token handling, though JWT signature verification would strengthen it further.

Code Quality: High quality with good separation of concerns, proper error handling, and reasonable test coverage.

Architecture: Excellent unified design that's maintainable and extensible.

Recommendation: ✅ Approve with minor suggestions - The core functionality is sound and the improvements mentioned above can be addressed in follow-up work.


- Add Apple case to prometheus metrics identity match
- Add Apple variant to OAuth2Provider enum in authentication_utils
- Add placeholder verification (returns not yet supported error)
- Fix 6 exhaustive match patterns in test functions
- Apple is web2 (true), not web3/substrate/evm/bitcoin/solana (false)
- Move decode_id_token logic to oauth2_common as generic function
- Move base64_decode helper to oauth2_common
- Update Apple and Google modules to use shared implementation
- Eliminate 28 lines of duplicated JWT decoding code
- Add OAuth2VerificationData struct to store state and nonce
- Add id_token field to OAuth2Data for client-side token verification
- Remove Identity parameter from OmniAuth::OAuth2 variant
- Update OAuth2StateVerifierStorage to store OAuth2VerificationData
Google IdToken:
- email_verified: bool -> String (Google returns "true"/"false")
- hd: String -> Option<String> (only for Workspace accounts)
- at_hash: String -> Option<String>
- nonce: String -> Option<String>

Apple IdToken:
- email_verified: Option<String> -> bool
- nonce: String -> Option<String>
- Add optional fields: is_private_email, c_hash, auth_time
- Add OAuth2ProviderConfig struct to centralize provider configuration
- Export BASE_URL and SCOPES as public constants in provider modules
- Remove wrapper functions (get_authorize_data, decode_id_token)
- Add nonce parameter to authorization data
- Rename omni_getOAuth2AuthorizationUrl -> omni_getOAuth2AuthorizationData
- Return structured data (url, state, nonce, scope, response_type, etc)
- Use OAuth2ProviderConfig for cleaner provider-specific configuration
- Store both state and nonce in OAuth2VerificationData
- Add id_token parameter to login flow
- Verify id_token before token exchange (decode, aud, nonce)
- Add verify_id_token_claims helper to reduce code duplication
- Remove Identity parameter from OAuth2 verification
- Use oauth2_common::decode_id_token for both providers
@silva-fj silva-fj requested a review from a team October 16, 2025 16:22
Copy link
Collaborator

@BillyWooo BillyWooo left a comment

Choose a reason for hiding this comment

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

@Kailai-Wang @kziemianek Please also take a look. Thank you.

@silva-fj silva-fj enabled auto-merge (squash) October 20, 2025 12:19
@silva-fj silva-fj merged commit 1c73046 into dev Oct 20, 2025
25 of 47 checks passed
@silva-fj silva-fj deleted the p-1707-login-with-apple branch October 20, 2025 15:07
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