WIP: add user email to google mcp #485
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This comment has been minimized.
This comment has been minimized.
MichaelClifford
force-pushed
the
google-mcp-add-user-email
branch
from
a4bb582 to
b6d3857
Compare
Claude Code ReviewSummaryThis PR adds user email support to the Google Workspace MCP integration by propagating the authenticated user's email through the backend → operator → runner pipeline. The email is extracted from the Overall Assessment: Good implementation that follows established patterns, but has several critical security and error handling issues that must be addressed before merge. Issues by Severity🚫 Blocker Issues1. Email Logging Violates Security Standards
2. Missing Type Safety in Operator
🔴 Critical Issues3. Hardcoded Fallback Email in Production
4. Email Fallback Logic is Questionable
🟡 Major Issues5. Missing Test Coverage
6. Inconsistent Error Handling in Python
7. No Validation of Email Format
🔵 Minor Issues8. Documentation Missing
9. Type Definition Could Be More Specific
10. Environment Variable Not Documented
Positive Highlights✅ Follows Established Patterns: Email extraction mirrors existing RecommendationsPriority 1 (Must Fix Before Merge)
Priority 2 (Should Fix Before Merge)
Priority 3 (Nice to Have)
Security Checklist ReviewBased on
Code Quality ChecklistBased on CLAUDE.md backend standards:
Files Changed Analysis
Suggested FixesFix 1: Redact Email in Logs (Blocker)- log.Printf("Session %s initiated by user: %s (userId: %s, email: %s)", name, userName, userID, userEmail)
+ log.Printf("Session %s initiated by user: %s (userId: %s, emailSet: %v)", name, userName, userID, userEmail != "")Fix 2: Type-Safe Access (Blocker)- if v, ok := userContext["email"].(string); ok {
- userEmail = strings.TrimSpace(v)
- }
+ if email, found, err := unstructured.NestedString(spec, "userContext", "email"); found && err == nil {
+ userEmail = strings.TrimSpace(email)
+ }Fix 3: Remove Invalid Default (Critical)- if not os.getenv("USER_GOOGLE_EMAIL"):
- os.environ["USER_GOOGLE_EMAIL"] = "user@example.com"
- logger.info("USER_GOOGLE_EMAIL not set, using default: user@example.com")
- else:
- logger.info(f"USER_GOOGLE_EMAIL set to: {os.getenv('USER_GOOGLE_EMAIL')}")
+ if not os.getenv("USER_GOOGLE_EMAIL"):
+ logger.warning("USER_GOOGLE_EMAIL not set - Google Workspace MCP may not function correctly")
+ else:
+ logger.info(f"USER_GOOGLE_EMAIL configured (length: {len(os.getenv('USER_GOOGLE_EMAIL'))})")Fix 4: Improve Email Fallback (Critical)- if email == "" {
- email = uid
- }
+ if email == "" && strings.Contains(uid, "@") {
+ email = uid
+ log.Printf("Using userID as email for user %s", uid)
+ }Recommendation: Address blocker issues #1 and #2, and critical issue #3 before merging. The other issues can be addressed in follow-up PRs, but would strengthen the implementation if fixed now. 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
MichaelClifford
force-pushed
the
google-mcp-add-user-email
branch
from
b6d3857 to
70d694c
Compare
Claude Code ReviewSummaryThis PR adds user email propagation through the entire vTeam stack to enable Google Workspace MCP integration. The implementation extracts email from X-Forwarded-Email header, stores it in the AgenticSession userContext, and passes it to the Claude Code runner as USER_GOOGLE_EMAIL environment variable. Overall Assessment: ✅ APPROVED WITH MINOR RECOMMENDATIONS The implementation follows established patterns from CLAUDE.md and context files. Security and architectural standards are maintained. Minor improvements recommended for robustness. Issues by Severity🚫 Blocker IssuesNone - PR is ready to merge. 🔴 Critical IssuesNone 🟡 Major Issues1. Email Logging in Operator May Contain PII Location: log.Printf("Session %s initiated by user: %s (userId: %s, email: %s)", name, userName, userID, userEmail)Issue: Logging user email addresses may violate privacy policies or data protection regulations (GDPR, CCPA). While useful for debugging, emails are PII that should be handled carefully. Recommendation:
Justification: Backend already redacts tokens (server/server.go:22-34). Email PII should follow similar privacy standards. 2. Missing Email Validation Location: if v, ok := c.Get("userEmail"); ok {
if s, ok2 := v.(string); ok2 {
email = strings.TrimSpace(s)
}
}Issue: Email extracted from X-Forwarded-Email header is not validated. Malformed or malicious email values could be passed through to Google MCP. Recommendation: Add basic email validation if v, ok := c.Get("userEmail"); ok {
if s, ok2 := v.(string); ok2 {
email = strings.TrimSpace(s)
// Basic email validation
if email != "" && !isValidEmail(email) {
log.Printf("Warning: Invalid email format from X-Forwarded-Email: %s", email)
email = "" // Clear invalid email
}
}
}Add helper function: func isValidEmail(email string) bool {
// Basic RFC 5322 validation
const emailRegex = `^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$`
matched, _ := regexp.MatchString(emailRegex, email)
return matched
}Justification: Input validation is a security best practice (security-standards.md). Even trusted headers should be validated. 🔵 Minor Issues1. Type Definition Could Be More Precise Location: Email string `json:"email,omitempty"`Observation: Field is defined as Recommendation: Either:
Email string `json:"email"` // Always populated; falls back to userId if X-Forwarded-Email not present2. Environment Variable Documentation Missing Location: Issue: New
Recommendation: Add documentation in ### Environment Variables
- `USER_GOOGLE_EMAIL`: User's email address for Google Workspace MCP operations (defaults to user@example.com if not set)3. Consistency: Email Fallback Pattern Observation:
Recommendation: Document this fallback strategy in code comments to explain why different defaults exist: // Fallback to userID if no explicit email (userID is often the email in OAuth/OIDC)
// This provides a best-effort email for upstream identity propagation
if email == "" {
email = uid
}// Determine Google email: use userEmail from session context if available
// Otherwise default to placeholder (Google MCP will require valid email for actual operations)
googleEmail := "user@example.com"
if userEmail != "" {
googleEmail = userEmail
}Positive Highlights✅ Excellent Pattern Adherence
RecommendationsHigh Priority
Medium Priority
Low Priority
Pre-Commit Checklist StatusBased on CLAUDE.md standards: Backend Checklist
Run Before Mergecd components/backend && gofmt -l . && go vet ./... && golangci-lint run
cd components/operator && gofmt -l . && go vet ./... && golangci-lint runFinal Recommendation✅ APPROVE - Merge after addressing email logging (Major Issue #1) and email validation (Major Issue #2) The PR successfully extends user context propagation to support Google Workspace MCP integration. The implementation is architecturally sound and follows established patterns. The recommended changes enhance security and robustness but are not blockers for merge. Suggested merge timeline: After addressing the 2 major issues (estimated: 15 minutes of changes). 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
Signed-off-by: Michael Clifford <mcliffor@redhat.com>
MichaelClifford
force-pushed
the
google-mcp-add-user-email
branch
from
70d694c to
627a91d
Compare
Claude Code ReviewSummaryThis PR adds user email support to the Google MCP (Model Context Protocol) integration. It extracts user email from Google OAuth responses and propagates it through the system (backend → operator → runner) to enable Google Workspace MCP tools that require user email for delegation. Overall Assessment: Good implementation with solid security practices. There are a few critical security issues and several improvements needed before merge. Issues by Severity🚫 Blocker Issues1. Security: Email logged in plaintext (oauth.go:309) log.Printf("Fetched user email from Google OAuth: %s", userEmail)
2. Security: Email logged in plaintext (oauth.go:787, 804) log.Printf("✓ Updated OAuth credentials Secret %s/%s (email: %s)", projectName, secretName, userEmail)
log.Printf("✓ Created OAuth credentials Secret %s/%s (email: %s)", projectName, secretName, userEmail)
3. Security: Email logged in plaintext (operator/sessions.go:997, 1127, 1133) log.Printf("Session %s initiated by user: %s (userId: %s, email: %s)", name, userName, userID, userEmail)
log.Printf("Using Google email from OAuth secret for session %s: %s", name, googleEmail)
log.Printf("Using Google email from session userContext for session %s: %s", name, googleEmail)
🔴 Critical Issues1. Error Handling: Non-fatal error treated as warning (oauth.go:305-310) if err != nil {
log.Printf("Warning: Failed to fetch user email from Google: %v", err)
} else {
userEmail = email
log.Printf("Fetched user email from Google OAuth: %s", userEmail)
}
2. Type Safety: Missing error check (adapter.py:1564) email = secret_email_path.read_text().strip()
if email and email != "user@example.com":
# ...
3. Documentation: Missing docstring context (oauth.go:413-418) // GoogleUserInfo represents the minimal user info response from Google (email only)
type GoogleUserInfo struct {
Email string `json:"email"`
VerifiedEmail bool `json:"verified_email"`
}
🟡 Major Issues1. Code Pattern: Inconsistent email extraction logic (sessions.go:667-677) // Extract email from authenticated user
email := ""
if v, ok := c.Get("userEmail"); ok {
if s, ok2 := v.(string); ok2 {
email = strings.TrimSpace(s)
}
}
// Fallback to userID if no explicit email (userID is often the email)
if email == "" {
email = uid
}
2. Complexity: Three-tier email priority logic (operator/sessions.go:1114-1136)
3. Race Condition Risk: Secret read without retry (operator/sessions.go:1123) if oauthSecret, err := config.K8sClient.CoreV1().Secrets(sessionNamespace).Get(context.TODO(), googleOAuthSecretName, v1.GetOptions{}); err == nil {
🔵 Minor Issues1. Naming: Variable shadowing (oauth.go:305) email, err := fetchGoogleUserEmail(c.Request.Context(), tokenData.AccessToken)
if err != nil {
log.Printf("Warning: Failed to fetch user email from Google: %v", err)
} else {
userEmail = email
2. Code Style: Context usage (oauth.go:421) req, err := http.NewRequestWithContext(ctx, http.MethodGet, "https://www.googleapis.com/oauth2/v2/userinfo", nil)
3. Testing: Missing test coverage
4. Documentation: Missing comments for new behavior
Positive Highlights✅ Excellent error handling in
✅ Good defensive coding in adapter.py
✅ Proper type safety
✅ Security: OAuth secret properly mounted
✅ Good separation of concerns
RecommendationsPrioritized Action ItemsMust Fix (Blockers):
Should Fix (Critical):
Nice to Have (Major): Code Quality Improvements// oauth.go:305 - Fix variable shadowing
userEmail, err := fetchGoogleUserEmail(c.Request.Context(), tokenData.AccessToken)
if err != nil {
log.Printf("Warning: Failed to fetch Google user email: %v", err)
userEmail = "" // Explicitly clear on error
}
// oauth.go:434 - Consider validating verified_email
if userInfo.Email == "" {
return "", fmt.Errorf("no email in user info response")
}
if !userInfo.VerifiedEmail {
return "", fmt.Errorf("email not verified by Google")
}Security Checklist ResultsBased on security-standards.md:
ConclusionThis is a well-structured PR that follows the project's architectural patterns. The main concerns are:
Once the logging issues are resolved, this PR will be ready to merge. The implementation correctly propagates user email through the system and handles the OAuth timing dependencies appropriately. 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
MichaelClifford
changed the title
1 participant
No description provided.