Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement state store cleanup #12

Open
AnthonyRonning opened this issue Feb 18, 2025 · 0 comments
Open

Implement state store cleanup #12

AnthonyRonning opened this issue Feb 18, 2025 · 0 comments

Comments

@AnthonyRonning
Copy link
Contributor

          _:hammer_and_wrench: Refactor suggestion_

Implement state store cleanup.

The in-memory state store could grow indefinitely as there's no cleanup of old states. This could lead to memory leaks.

Add a cleanup mechanism:

+use tokio::time::{interval, Duration};
+
 impl GithubProvider {
     pub async fn new(db: Arc<dyn DBConnection + Send + Sync>) -> Result<Self, Error> {
         let provider = Self {
             auth_url,
             token_url,
             user_info_url,
             state_store: Arc::new(RwLock::new(HashMap::new())),
         };
+        
+        // Start cleanup task
+        let state_store = provider.state_store.clone();
+        tokio::spawn(async move {
+            let mut interval = interval(Duration::from_secs(3600)); // Run every hour
+            loop {
+                interval.tick().await;
+                cleanup_expired_states(&state_store).await;
+            }
+        });
         
         Ok(provider)
     }
 }
+
+async fn cleanup_expired_states(state_store: &Arc<RwLock<HashMap<String, OAuthState>>>) {
+    let now = chrono::Utc::now();
+    state_store.write().await.retain(|_, state| {
+        state.expires_at > now
+    });
+}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

use tokio::time::{interval, Duration};
use std::sync::Arc;
use tokio::sync::RwLock;
use std::collections::HashMap;
use chrono::Utc;

// Assume necessary imports and other parts of the module are present.

pub struct OAuthState {
    pub csrf_token: String,
    pub expires_at: chrono::DateTime<chrono::Utc>,
    // other fields...
}

pub struct GithubProvider {
    auth_url: String,
    token_url: String,
    user_info_url: String,
    state_store: Arc<RwLock<HashMap<String, OAuthState>>>,
    // other fields...
}

impl GithubProvider {
    pub async fn new(db: Arc<dyn DBConnection + Send + Sync>) -> Result<Self, Error> {
         let provider = Self {
             auth_url: "https://example.com/auth".to_string(),    // placeholder
             token_url: "https://example.com/token".to_string(),    // placeholder
             user_info_url: "https://example.com/user".to_string(), // placeholder
             state_store: Arc::new(RwLock::new(HashMap::new())),
         };
         
         // Start cleanup task
         let state_store = provider.state_store.clone();
         tokio::spawn(async move {
             let mut interval = interval(Duration::from_secs(3600)); // Run every hour
             loop {
                 interval.tick().await;
                 cleanup_expired_states(&state_store).await;
             }
         });
         
         Ok(provider)
    }
    
    pub async fn store_state(&self, csrf_token: &str, state: OAuthState) {
         self.state_store
             .write()
             .await
             .insert(csrf_token.to_string(), state);
    }

    pub async fn validate_state(&self, state: &OAuthState) -> bool {
         if let Some(stored_state) = self.state_store.read().await.get(&state.csrf_token) {
              // Validate both the CSRF token and the client_id match
              stored_state == state
         } else {
              false
         }
    }
}

async fn cleanup_expired_states(state_store: &Arc<RwLock<HashMap<String, OAuthState>>>) {
    let now = Utc::now();
    state_store.write().await.retain(|_, state| state.expires_at > now);
}

Originally posted by @coderabbitai[bot] in #3 (comment)

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

No branches or pull requests

1 participant