feat: Enterprise organization management#117
Conversation
Summary of ChangesHello @ilblackdragon, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request fundamentally transforms the application's architecture by introducing a suite of enterprise-grade features. It establishes a multi-tenant model through organizations and workspaces, implements a flexible Role-Based Access Control system, integrates SAML for secure single sign-on, and provides domain verification capabilities. Additionally, a comprehensive audit logging system ensures traceability and compliance. These changes lay the groundwork for advanced enterprise functionality and improved security. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant set of features for enterprise organization management, including organizations, workspaces, RBAC, SAML SSO, audit logging, and domain verification. The changes are extensive, adding new API routes, services, database repositories, and database migrations. The overall structure is well-organized, with clear separation of concerns between the API, service, and data layers.
My review has identified a critical security vulnerability in the SAML implementation where response signatures are not being verified. I've also pointed out several areas for improvement in terms of code clarity, efficiency, and robustness, such as conditional service initialization and avoiding fragile error handling. Addressing these points will significantly improve the quality and security of the new features.
| if response.signature.is_some() { | ||
| let cert = X509::from_pem(idp_certificate_pem.as_bytes())?; | ||
| let public_key = cert.public_key()?; | ||
|
|
||
| // samael handles signature verification internally when parsing | ||
| // But we need to verify against our known certificate | ||
| tracing::debug!("SAML response has signature, validating against IdP certificate"); | ||
|
|
||
| // For now, we trust the parsed response if it has a valid XML structure | ||
| // Full cryptographic verification would require samael's verify_signature | ||
| } |
There was a problem hiding this comment.
This implementation appears to parse the SAML response without cryptographically verifying its signature against the IdP's certificate. The comment on line 111 confirms this. This is a critical security vulnerability, as it would allow an attacker to forge SAML responses and impersonate any user. The signature on the SAML response or assertion must be verified using the configured IdP certificate before trusting its contents. The samael crate likely provides a method to do this, such as response.verify_signature(&public_key).
| if response.signature.is_some() { | |
| let cert = X509::from_pem(idp_certificate_pem.as_bytes())?; | |
| let public_key = cert.public_key()?; | |
| // samael handles signature verification internally when parsing | |
| // But we need to verify against our known certificate | |
| tracing::debug!("SAML response has signature, validating against IdP certificate"); | |
| // For now, we trust the parsed response if it has a valid XML structure | |
| // Full cryptographic verification would require samael's verify_signature | |
| } | |
| if let Some(signature) = &response.signature { | |
| tracing::debug!("SAML response has signature, validating against IdP certificate"); | |
| let cert = X509::from_pem(idp_certificate_pem.as_bytes())?; | |
| let public_key = cert.public_key()?; | |
| signature.verify_signed_info(&public_key) | |
| .map_err(|e| anyhow::anyhow!("SAML response signature verification failed: {}", e))?; | |
| tracing::debug!("SAML response signature is valid"); | |
| } else { | |
| // Depending on security requirements, you might want to reject unsigned responses. | |
| tracing::warn!("SAML response is not signed. This may be a security risk."); | |
| } |
| let saml_service: Option<Arc<dyn services::saml::ports::SamlService>> = if config.saml.enabled { | ||
| tracing::info!("Initializing SAML SSO service..."); | ||
| tracing::info!( | ||
| "SAML SP Base URL: {}, Entity ID: {}", | ||
| config.saml.sp_base_url, | ||
| config.saml.get_sp_entity_id() | ||
| ); | ||
| Some(Arc::new(SamlServiceImpl::new( | ||
| saml_idp_config_repo, | ||
| saml_auth_state_repo, | ||
| config.saml.sp_base_url.clone(), | ||
| ))) | ||
| } else { | ||
| tracing::info!("SAML SSO is disabled (set SAML_ENABLED=true to enable)"); | ||
| let _ = (saml_idp_config_repo, saml_auth_state_repo); // Suppress unused warnings | ||
| None | ||
| }; | ||
|
|
There was a problem hiding this comment.
To improve resource management and code clarity, it's better to initialize saml_idp_config_repo and saml_auth_state_repo only when SAML is enabled. This avoids initializing them unconditionally and then having to suppress unused variable warnings.
With the suggested change, you should also remove the unconditional initialization of these repositories on lines 90-91.
| let saml_service: Option<Arc<dyn services::saml::ports::SamlService>> = if config.saml.enabled { | |
| tracing::info!("Initializing SAML SSO service..."); | |
| tracing::info!( | |
| "SAML SP Base URL: {}, Entity ID: {}", | |
| config.saml.sp_base_url, | |
| config.saml.get_sp_entity_id() | |
| ); | |
| Some(Arc::new(SamlServiceImpl::new( | |
| saml_idp_config_repo, | |
| saml_auth_state_repo, | |
| config.saml.sp_base_url.clone(), | |
| ))) | |
| } else { | |
| tracing::info!("SAML SSO is disabled (set SAML_ENABLED=true to enable)"); | |
| let _ = (saml_idp_config_repo, saml_auth_state_repo); // Suppress unused warnings | |
| None | |
| }; | |
| let saml_service: Option<Arc<dyn services::saml::ports::SamlService>> = if config.saml.enabled { | |
| tracing::info!("Initializing SAML SSO service..."); | |
| tracing::info!( | |
| "SAML SP Base URL: {}, Entity ID: {}", | |
| config.saml.sp_base_url, | |
| config.saml.get_sp_entity_id() | |
| ); | |
| let saml_idp_config_repo = db.saml_idp_config_repository(); | |
| let saml_auth_state_repo = db.saml_auth_state_repository(); | |
| Some(Arc::new(SamlServiceImpl::new( | |
| saml_idp_config_repo, | |
| saml_auth_state_repo, | |
| config.saml.sp_base_url.clone(), | |
| ))) | |
| } else { | |
| tracing::info!("SAML SSO is disabled (set SAML_ENABLED=true to enable)"); | |
| None | |
| }; |
| jit_provisioning_enabled: request.jit_provisioning_enabled.unwrap_or(true), | ||
| jit_default_role: request.jit_default_role.unwrap_or_else(|| "member".to_string()), | ||
| jit_default_workspace_id: request.jit_default_workspace_id, |
There was a problem hiding this comment.
The jit_default_role is taken as a string and defaults to "member". However, there's no validation to ensure it's a valid role (owner, admin, or member). This could lead to unexpected behavior if an invalid role is configured, as it will always default to member during JIT provisioning. It would be more robust to validate this string against the OrgRole enum here or in the service layer when the SAML configuration is created or updated.
| let cookie_value = format!( | ||
| "session_token={}; Path=/; HttpOnly; SameSite=Lax; Max-Age={}", | ||
| session_token, | ||
| 60 * 60 * 24 * 7 // 7 days | ||
| ); | ||
|
|
||
| Ok(( | ||
| StatusCode::FOUND, | ||
| [ | ||
| (header::LOCATION, redirect_location), | ||
| (header::SET_COOKIE, &cookie_value), | ||
| ], | ||
| "", | ||
| ) | ||
| .into_response()) | ||
| } |
There was a problem hiding this comment.
Manually constructing the Set-Cookie header string is error-prone and can lead to security issues if not formatted correctly. It's better to use a library like cookie to build the cookie. This ensures correct formatting and handling of attributes like HttpOnly, SameSite, and Max-Age.
I've added the necessary use statement. You'll also need to add cookie = "0.18" to your api crate's Cargo.toml.
use cookie::{Cookie, SameSite};
// Set cookie and redirect
let cookie = Cookie::build(("session_token", session_token))
.path("/")
.http_only(true)
.same_site(SameSite::Lax)
.max_age(Duration::days(7));
Ok((
StatusCode::FOUND,
[
(header::LOCATION, redirect_location),
(header::SET_COOKIE, cookie.to_string().as_str()),
],
"",
)
.into_response())1 participant
See nearai/private-chat#269 for details