feat(auth): Enhance Google OAuth integration with PKCE support and CSRF validation improvements

imphnen-iam/src/v1/auth/google/google_oauth_dto.rs

@@ -4,12 +4,13 @@ use serde::{Deserialize, Serialize};
 pub struct GoogleUser {
     pub id: String,
     pub email: String,
+    #[serde(default)]
     pub verified_email: bool,
-    pub name: String,
-    pub given_name: String,
-    pub family_name: String,
-    pub picture: String,
-    pub locale: String,
+    pub name: Option<String>,
+    pub given_name: Option<String>,
+    pub family_name: Option<String>,
+    pub picture: Option<String>,
+    pub locale: Option<String>,
 }
 
 #[derive(Debug, Serialize, Deserialize)]

imphnen-iam/src/v1/auth/google/google_oauth_service.rs

@@ -1,7 +1,7 @@
 use anyhow::Result;
 use async_trait::async_trait;
 use oauth2::{
-    basic::BasicClient, AuthUrl, ClientId, ClientSecret, CsrfToken, PkceCodeChallenge,
+    basic::BasicClient, AuthUrl, ClientId, ClientSecret, CsrfToken, PkceCodeChallenge, PkceCodeVerifier,
     RedirectUrl, Scope, TokenResponse, TokenUrl,
 };
 use serde::{Deserialize, Serialize};
@@ -10,7 +10,7 @@ use tracing::{info, error};
 
 use imphnen_entities::error_dto::error::Error;
 use imphnen_libs::{jsonwebtoken::{encode_access_token, encode_refresh_token}, enviroment::Env};
-use imphnen_utils::{generate_csrf_token, validate_csrf_token};
+use imphnen_utils::{generate_oauth_csrf_token, validate_oauth_csrf_token, validate_csrf_token};
 use crate::v1::auth::TokenDto;
 use crate::v1::auth::auth_service::AuthServiceTrait;
 use crate::v1::users::users_dto::{UsersCreateRequestDto, UsersDetailItemDto};
@@ -38,23 +38,42 @@ impl AuthRequest {
         }
         
         // Basic format validation for authorization code
-        if !self.code.chars().all(|c| c.is_alphanumeric() || c == '-' || c == '_' || c == '.' || c == '~') {
+        // OAuth 2.0 authorization codes can contain URL-safe characters including base64 characters
+        if !self.code.chars().all(|c| c.is_alphanumeric() || c == '-' || c == '_' || c == '.' || c == '~' || c == '/' || c == '+' || c == '=') {
             return Err(Error::Validation("Authorization code contains invalid characters".to_string()));
         }
         
         Ok(())
     }
     
-    /// Validate CSRF state token with signature verification
-    pub fn validate_csrf_state(&self, secret: &str) -> Result<(), Error> {
+    /// Validate CSRF state token with signature verification and extract PKCE verifier
+    pub fn validate_csrf_state_and_get_pkce_verifier(&self, secret: &str) -> Result<PkceCodeVerifier, Error> {
         // Maximum age of 10 minutes for OAuth flow
         const MAX_AGE_SECONDS: u64 = 600;
         
-        validate_csrf_token(&self.state, secret, MAX_AGE_SECONDS)
+        let pkce_verifier_secret = validate_oauth_csrf_token(&self.state, secret, MAX_AGE_SECONDS)
             .map_err(|e| {
-                error!("CSRF validation failed: {:?}", e);
-                Error::Auth("Invalid or expired CSRF state token".to_string())
-            })
+                error!("OAuth CSRF validation failed: {:?}", e);
+                Error::Auth("Invalid or expired OAuth CSRF state token".to_string())
+            })?;
+            
+        Ok(PkceCodeVerifier::new(pkce_verifier_secret))
+    }
+    
+    /// Validate CSRF state token with signature verification (legacy method for backward compatibility)
+    pub fn validate_csrf_state(&self, secret: &str) -> Result<(), Error> {
+        // Try OAuth CSRF validation first, if it fails, fall back to regular CSRF validation
+        match validate_oauth_csrf_token(&self.state, secret, 600) {
+            Ok(_) => Ok(()),
+            Err(_) => {
+                // Fallback to regular CSRF validation for backward compatibility
+                validate_csrf_token(&self.state, secret, 600)
+                    .map_err(|e| {
+                        error!("CSRF validation failed: {:?}", e);
+                        Error::Auth("Invalid or expired CSRF state token".to_string())
+                    })
+            }
+        }
     }
 }
 
@@ -123,10 +142,10 @@ where
 
     fn generate_auth_url(&self) -> (Url, CsrfToken) {
         let client = self.google_oauth_client();
-        let (pkce_code_challenge, _pkce_code_verifier) = PkceCodeChallenge::new_random_sha256();
+        let (pkce_code_challenge, pkce_code_verifier) = PkceCodeChallenge::new_random_sha256();
 
-        // Generate a signed CSRF token for stateless validation
-        let csrf_token_str = generate_csrf_token(&self.env.access_token_secret)
+        // Generate a signed CSRF token with PKCE verifier for stateless validation
+        let csrf_token_str = generate_oauth_csrf_token(&self.env.access_token_secret, pkce_code_verifier.secret())
             .unwrap_or_else(|_| uuid::Uuid::new_v4().to_string()); // Fallback to UUID if signing fails
         
         let csrf_token = CsrfToken::new(csrf_token_str);
@@ -143,8 +162,8 @@ where
         // Validate input parameters first
         auth_request.validate()?;
         
-        // CRITICAL: Validate CSRF state token
-        auth_request.validate_csrf_state(&self.env.access_token_secret)?;
+        // CRITICAL: Validate CSRF state token and extract PKCE verifier
+        let pkce_verifier = auth_request.validate_csrf_state_and_get_pkce_verifier(&self.env.access_token_secret)?;
         
         info!("Starting Google OAuth callback process");
         
@@ -152,6 +171,7 @@ where
 
         let token_response = client
             .exchange_code(oauth2::AuthorizationCode::new(auth_request.code))
+            .set_pkce_verifier(pkce_verifier)
             .request_async(oauth2::reqwest::async_http_client)
             .await
             .map_err(|e| {
@@ -182,12 +202,24 @@ where
             })?;
 
         info!("Successfully retrieved user info for email: {}", google_user.email);
+        info!("Google user picture URL: {:?}", google_user.picture);
+        info!("Google user data: name={:?}, given_name={:?}, family_name={:?}, picture={:?}", 
+              google_user.name, google_user.given_name, google_user.family_name, google_user.picture);
 
         let user = self.users_service.get_user_by_email(&google_user.email).await?;
 
         let user = match user {
-            Some(user) => {
+            Some(mut user) => {
                 info!("Existing user found for email: {}", google_user.email);
+                
+                // Update avatar if user doesn't have one and Google provides one
+                if user.avatar.is_none() && google_user.picture.is_some() {
+                    info!("Updating avatar for existing user: {}", google_user.email);
+                    // Note: We would need to implement an update_user_avatar method in the user service
+                    // For now, we'll just log this
+                    info!("Avatar would be updated to: {:?}", google_user.picture);
+                }
+                
                 user
             },
             None => {
@@ -203,10 +235,22 @@ where
                 let new_user = UsersCreateRequestDto {
                     email: google_user.email.clone(),
                     password: format!("GOOGLE_OAUTH_{}", uuid::Uuid::new_v4()), // Random placeholder
-                    fullname: google_user.name.clone(),
+                    fullname: google_user.name.clone().unwrap_or_else(|| {
+                        // Fallback: use given_name + family_name if available, otherwise use email prefix
+                        match (&google_user.given_name, &google_user.family_name) {
+                            (Some(given), Some(family)) => format!("{} {}", given, family),
+                            (Some(given), None) => given.clone(),
+                            (None, Some(family)) => family.clone(),
+                            (None, None) => {
+                                // Extract email prefix as last resort
+                                google_user.email.split('@').next().unwrap_or("User").to_string()
+                            }
+                        }
+                    }),
                     phone_number: "".to_string(), // Will be updated by user later
                     is_active: true,
                     role_id: default_role_id,
+                    avatar: google_user.picture.clone(), // Set avatar from Google user picture
                 };
                 
                 self.users_service.create_user_by_dto(new_user).await?
@@ -233,4 +277,133 @@ where
         info!("Successfully completed Google OAuth for user: {}", user.email);
         Ok((user, token_dto))
     }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use imphnen_utils::generate_oauth_csrf_token;
+    
+    #[test]
+    fn test_auth_request_validation_with_base64_characters() {
+        // Test case that was failing before the fix
+        let auth_request = AuthRequest {
+            code: "4/0-ARAA6EeEKN8rlQ_Dh5XAAA_dCpKFwKa3-Jl9cO7I".to_string(),
+            state: "valid_state".to_string(),
+        };
+        
+        let result = auth_request.validate();
+        assert!(result.is_ok(), "Authorization code with base64-like characters should be valid");
+    }
+    
+    #[test]
+    fn test_auth_request_validation_with_slash() {
+        let auth_request = AuthRequest {
+            code: "authorization/code/with/slashes".to_string(),
+            state: "valid_state".to_string(),
+        };
+        
+        let result = auth_request.validate();
+        assert!(result.is_ok(), "Authorization code with forward slashes should be valid");
+    }
+    
+    #[test]
+    fn test_auth_request_validation_with_plus() {
+        let auth_request = AuthRequest {
+            code: "authorization+code+with+plus".to_string(),
+            state: "valid_state".to_string(),
+        };
+        
+        let result = auth_request.validate();
+        assert!(result.is_ok(), "Authorization code with plus signs should be valid");
+    }
+    
+    #[test]
+    fn test_auth_request_validation_with_equals() {
+        let auth_request = AuthRequest {
+            code: "authorization=code=with=equals=".to_string(),
+            state: "valid_state".to_string(),
+        };
+        
+        let result = auth_request.validate();
+        assert!(result.is_ok(), "Authorization code with equals signs should be valid");
+    }
+    
+    #[test]
+    fn test_auth_request_validation_with_invalid_chars() {
+        let auth_request = AuthRequest {
+            code: "authorization@code#with$invalid%chars".to_string(),
+            state: "valid_state".to_string(),
+        };
+        
+        let result = auth_request.validate();
+        assert!(result.is_err(), "Authorization code with invalid characters should be rejected");
+    }
+    
+    #[test]
+    fn test_auth_request_validation_empty_code() {
+        let auth_request = AuthRequest {
+            code: "".to_string(),
+            state: "valid_state".to_string(),
+        };
+        
+        let result = auth_request.validate();
+        assert!(result.is_err(), "Empty authorization code should be rejected");
+    }
+    
+    #[test]
+    fn test_oauth_csrf_with_pkce_verifier() {
+        let secret = "test_secret";
+        let pkce_verifier = "test_pkce_verifier";
+        
+        // Generate OAuth CSRF token with PKCE verifier
+        let token = generate_oauth_csrf_token(secret, pkce_verifier).unwrap();
+        
+        // Create auth request with the token
+        let auth_request = AuthRequest {
+            code: "test_code".to_string(),
+            state: token,
+        };
+        
+        // Validate and extract PKCE verifier
+        let extracted_verifier = auth_request.validate_csrf_state_and_get_pkce_verifier(secret).unwrap();
+        assert_eq!(extracted_verifier.secret(), pkce_verifier);
+    }
+    
+    #[test]
+    fn test_oauth_csrf_backwards_compatibility() {
+        let secret = "test_secret";
+        
+        // Generate regular CSRF token (legacy)
+        let token = imphnen_utils::generate_csrf_token(secret).unwrap();
+        
+        // Create auth request with the token
+        let auth_request = AuthRequest {
+            code: "test_code".to_string(),
+            state: token,
+        };
+        
+        // Legacy validation should still work
+        let result = auth_request.validate_csrf_state(secret);
+        assert!(result.is_ok(), "Legacy CSRF validation should still work");
+    }
+
+    #[test]
+    fn test_user_creation_with_avatar() {
+        use crate::v1::users::users_dto::UsersCreateRequestDto;
+        
+        let google_user_picture = Some("https://lh3.googleusercontent.com/a/default-user".to_string());
+        
+        let new_user = UsersCreateRequestDto {
+            email: "test@example.com".to_string(),
+            password: "password123".to_string(),
+            fullname: "Test User".to_string(),
+            phone_number: "1234567890".to_string(),
+            is_active: true,
+            role_id: "test_role_id".to_string(),
+            avatar: google_user_picture.clone(),
+        };
+        
+        assert_eq!(new_user.avatar, google_user_picture, "Avatar should be set from Google user picture");
+    }
 }