fix(runner): write Google credentials as {email}.json with timezone-naive expiry#1557
Conversation
📝 WalkthroughWalkthroughRefactored Google Workspace MCP credential handling from single hardcoded ChangesGoogle Workspace Credential Storage and Loading
Sequence DiagramsequenceDiagram
participant User as Runtime/User
participant Writer as populate_runtime_credentials<br/>(auth.py)
participant FS as Credential Storage<br/>(Directories/Files)
participant Reader as check_mcp_authentication<br/>(mcp.py)
participant Validator as Credential Validation
User->>Writer: Provide OAuth token or service account
Writer->>FS: Write OAuth: {email}.json with access/refresh/expiry
Writer->>FS: Write ServiceAccount: legacy credentials.json
Writer->>FS: Clean stale credentials.json if switching to per-user
Reader->>FS: Scan *.json files in creds directories (sorted)
FS-->>Reader: List of credential files
Reader->>Reader: Iterate files, load each via _load_credential_file()
Reader->>Validator: Pass first valid credential for email check
Validator-->>Reader: Return is_auth, message
Reader-->>User: Authentication result
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
❌ Deploy Preview for cheerful-kitten-f556a0 failed.
|
Review: fix(runner): write Google credentials as {email}.json with timezone-naive expiryClean, targeted fix with solid root cause analysis. The three-part fix (expiry format, filename, directory scan) maps directly to the two root causes. Test quality improvement (real filesystem over mocks) is a net positive. Two minor things worth a look: 🟡
|
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Minor observations
-
_read_google_credentials(json_file, json_file)— both args are the same path now. Works because the workspace/secret fallback moved to the outer loop, but the function will try the same file twice on a read failure. Could pass a single-path helper or skip the redundant retry. -
sorted(creds_dir.glob("*.json"))priority — if bothcredentials.jsonanduser@example.com.jsonexist, alphabetical sort picks the legacy file first. The write path cleans up the legacy file so this shouldn't happen in practice, but worth noting. -
import shutilinsidefinally(test_shared_session_credentials.py:660) — move to top-of-file imports.
None of these are blocking.
jsell-rh's Ambient Review Bot <jsell+ambient-review-bot@redhat.com>
…aive expiry
workspace-mcp expects credential files named {email}.json (not credentials.json)
and parses expiry via datetime.fromisoformat() which fails with trailing Z on
Python 3.10. This caused workspace-mcp to reject valid pre-fetched credentials
and fall back to its own inaccessible OAuth flow with a PKCE mismatch.
- Strip trailing Z from expiresAt before writing to credential file
- Write credential file as {email}.json instead of credentials.json
- Clean up legacy credentials.json when email-based file is written
- Update check_mcp_authentication to scan credentials directory for any .json file
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <noreply@anthropic.com>
markturansky
force-pushed
the
fix/google-oauth-credential-format
branch
from
c76dc9e to
5aa1978
Compare
|
CodeRabbit chat interactions are restricted to organization members for this repository. Ask an organization member to interact with CodeRabbit, or set |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/mcp.py`:
- Around line 230-239: Prefer the current user's credential file before scanning
the directory: when iterating creds_dir in (_WORKSPACE_CREDS_DIR,
_SECRET_CREDS_DIR) first check for a file matching USER_GOOGLE_EMAIL + ".json"
and attempt to load it via _load_credential_file; if that returns a valid
candidate assign to creds and break out. Only if no USER_GOOGLE_EMAIL.json is
present or valid, fall back to the existing sorted glob("*.json") loop that
loads the first valid candidate. Ensure you reference creds, creds_dir,
_load_credential_file, USER_GOOGLE_EMAIL, _WORKSPACE_CREDS_DIR and
_SECRET_CREDS_DIR so the change is applied in the same block and retains the
existing break logic.
In `@components/runners/ambient-runner/ambient_runner/platform/auth.py`:
- Around line 446-451: The current write to creds_file can leave readers with
partial JSON; update the logic in the block that references
_GOOGLE_WORKSPACE_LEGACY_CREDS_FILE, creds_filename, creds_file and creds_data
to perform an atomic write: create a temporary file in the same directory (e.g.,
creds_file.with_suffix(".tmp" or similar)), write creds_data to that temp file,
fsync if possible, set permissions via chmod(0o600) on the temp file, then
atomically replace the target with os.replace(temp_path, creds_file) and only
after successful replace unlink _GOOGLE_WORKSPACE_LEGACY_CREDS_FILE if needed;
ensure you handle exceptions so a failed write leaves the original file
untouched.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f9a620a0-f065-443a-a2a9-07ab4bbb6f25
📒 Files selected for processing (4)
components/runners/ambient-runner/ambient_runner/bridges/claude/mcp.pycomponents/runners/ambient-runner/ambient_runner/platform/auth.pycomponents/runners/ambient-runner/tests/test_mcp_auth.pycomponents/runners/ambient-runner/tests/test_shared_session_credentials.py
| for creds_dir in (_WORKSPACE_CREDS_DIR, _SECRET_CREDS_DIR): | ||
| if creds_dir.is_dir(): | ||
| for json_file in sorted(creds_dir.glob("*.json")): | ||
| candidate = _load_credential_file(json_file) | ||
| if candidate is not None: | ||
| logger.debug("Using Google credentials from %s", json_file) | ||
| creds = candidate | ||
| break | ||
| if creds is not None: | ||
| break |
There was a problem hiding this comment.
Prefer the current user's credential file before scanning the directory.
This loop picks the first readable *.json alphabetically and ignores USER_GOOGLE_EMAIL. In a reused/shared workspace with multiple {email}.json files, /mcp/status can validate the wrong user's token and return misleading auth state for the active user. Prefer <USER_GOOGLE_EMAIL>.json first, then fall back to the directory scan only for legacy recovery. A regression test with two credential files would lock this down.
Proposed direction
if server_name == "google-workspace":
creds = None
+ configured_user_email = os.environ.get("USER_GOOGLE_EMAIL", "").strip()
for creds_dir in (_WORKSPACE_CREDS_DIR, _SECRET_CREDS_DIR):
if creds_dir.is_dir():
+ preferred_file = (
+ creds_dir / f"{configured_user_email}.json"
+ if configured_user_email
+ and configured_user_email != "user@example.com"
+ else None
+ )
+ if preferred_file is not None:
+ candidate = _load_credential_file(preferred_file)
+ if candidate is not None:
+ logger.debug("Using Google credentials from %s", preferred_file)
+ creds = candidate
+ break
for json_file in sorted(creds_dir.glob("*.json")):
+ if preferred_file is not None and json_file == preferred_file:
+ continue
candidate = _load_credential_file(json_file)
if candidate is not None:
logger.debug("Using Google credentials from %s", json_file)
creds = candidate
break📝 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.
| for creds_dir in (_WORKSPACE_CREDS_DIR, _SECRET_CREDS_DIR): | |
| if creds_dir.is_dir(): | |
| for json_file in sorted(creds_dir.glob("*.json")): | |
| candidate = _load_credential_file(json_file) | |
| if candidate is not None: | |
| logger.debug("Using Google credentials from %s", json_file) | |
| creds = candidate | |
| break | |
| if creds is not None: | |
| break | |
| creds = None | |
| configured_user_email = os.environ.get("USER_GOOGLE_EMAIL", "").strip() | |
| for creds_dir in (_WORKSPACE_CREDS_DIR, _SECRET_CREDS_DIR): | |
| if creds_dir.is_dir(): | |
| preferred_file = ( | |
| creds_dir / f"{configured_user_email}.json" | |
| if configured_user_email | |
| and configured_user_email != "user@example.com" | |
| else None | |
| ) | |
| if preferred_file is not None: | |
| candidate = _load_credential_file(preferred_file) | |
| if candidate is not None: | |
| logger.debug("Using Google credentials from %s", preferred_file) | |
| creds = candidate | |
| break | |
| for json_file in sorted(creds_dir.glob("*.json")): | |
| if preferred_file is not None and json_file == preferred_file: | |
| continue | |
| candidate = _load_credential_file(json_file) | |
| if candidate is not None: | |
| logger.debug("Using Google credentials from %s", json_file) | |
| creds = candidate | |
| break | |
| if creds is not None: | |
| break |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/mcp.py`
around lines 230 - 239, Prefer the current user's credential file before
scanning the directory: when iterating creds_dir in (_WORKSPACE_CREDS_DIR,
_SECRET_CREDS_DIR) first check for a file matching USER_GOOGLE_EMAIL + ".json"
and attempt to load it via _load_credential_file; if that returns a valid
candidate assign to creds and break out. Only if no USER_GOOGLE_EMAIL.json is
present or valid, fall back to the existing sorted glob("*.json") loop that
loads the first valid candidate. Ensure you reference creds, creds_dir,
_load_credential_file, USER_GOOGLE_EMAIL, _WORKSPACE_CREDS_DIR and
_SECRET_CREDS_DIR so the change is applied in the same block and retains the
existing break logic.
| if _GOOGLE_WORKSPACE_LEGACY_CREDS_FILE.exists() and creds_filename != "credentials.json": | ||
| _GOOGLE_WORKSPACE_LEGACY_CREDS_FILE.unlink(missing_ok=True) | ||
| logger.info("Removed legacy credentials.json in favor of %s", creds_filename) | ||
| with open(creds_file, "w") as f: | ||
| _json.dump(creds_data, f, indent=2) | ||
| _GOOGLE_WORKSPACE_CREDS_FILE.chmod(0o600) | ||
| logger.info("Updated Google credentials file for workspace-mcp") | ||
| creds_file.chmod(0o600) |
There was a problem hiding this comment.
Write the Google credential file atomically.
workspace-mcp reads this file from another process. Writing the target in place and deleting credentials.json first creates a window where readers can see empty/partial JSON, and a failed write can leave the session with no usable Google credential file at all. Write to a temp file in the same directory, chmod it, os.replace() it, then remove the legacy file after the replace succeeds.
Safer update pattern
- if _GOOGLE_WORKSPACE_LEGACY_CREDS_FILE.exists() and creds_filename != "credentials.json":
- _GOOGLE_WORKSPACE_LEGACY_CREDS_FILE.unlink(missing_ok=True)
- logger.info("Removed legacy credentials.json in favor of %s", creds_filename)
- with open(creds_file, "w") as f:
+ temp_creds_file = creds_file.with_suffix(f"{creds_file.suffix}.tmp")
+ with open(temp_creds_file, "w") as f:
_json.dump(creds_data, f, indent=2)
- creds_file.chmod(0o600)
+ temp_creds_file.chmod(0o600)
+ os.replace(temp_creds_file, creds_file)
+ if (
+ creds_filename != "credentials.json"
+ and _GOOGLE_WORKSPACE_LEGACY_CREDS_FILE.exists()
+ ):
+ _GOOGLE_WORKSPACE_LEGACY_CREDS_FILE.unlink(missing_ok=True)
+ logger.info(
+ "Removed legacy credentials.json in favor of %s",
+ creds_filename,
+ )
logger.info("Updated Google credentials file for workspace-mcp: %s", creds_filename)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/runners/ambient-runner/ambient_runner/platform/auth.py` around
lines 446 - 451, The current write to creds_file can leave readers with partial
JSON; update the logic in the block that references
_GOOGLE_WORKSPACE_LEGACY_CREDS_FILE, creds_filename, creds_file and creds_data
to perform an atomic write: create a temporary file in the same directory (e.g.,
creds_file.with_suffix(".tmp" or similar)), write creds_data to that temp file,
fsync if possible, set permissions via chmod(0o600) on the temp file, then
atomically replace the target with os.replace(temp_path, creds_file) and only
after successful replace unlink _GOOGLE_WORKSPACE_LEGACY_CREDS_FILE if needed;
ensure you handle exceptions so a failed write leaves the original file
untouched.
2 participants
Summary
ZfromexpiresAtbefore writing to credential file — workspace-mcp'sdatetime.fromisoformat()fails on Python 3.10 with theZsuffix, causing it to treat the expiry asNoneand the token as invalid{email}.jsoninstead ofcredentials.json— workspace-mcp expects this naming convention for proper user identification in single-user mode.jsonfile incheck_mcp_authenticationinstead of hardcodingcredentials.jsonFixes the UAT Google OAuth failure where workspace-mcp rejected valid pre-fetched credentials and fell back to its own inaccessible OAuth flow (PKCE mismatch with backend callback).
Root Cause
Two issues caused workspace-mcp to reject valid pre-fetched Google OAuth credentials:
Expiry format: Backend returns
expiresAtin RFC3339 (2026-05-11T19:58:05Z), but workspace-mcp parses it viadatetime.fromisoformat()which fails on Python 3.10 with the trailingZ. workspace-mcp then falls back to its own OAuth flow.Credential filename: Runner wrote
credentials.json, but workspace-mcp'slist_users()expects{email}.json. In single-user mode it finds and loads any.jsonfile, but the user identifier becomescredentialsinstead of the actual email.When workspace-mcp falls back to its own OAuth flow, it generates PKCE
code_challenge+ hexstate, but the backend's/oauth2callbackdoesn't have the matchingcode_verifier, causing Google to return"Missing code verifier".Test plan
test_google_oauth_access_token_formatverifies email-based filename and Z-stripped expirytest_mcp_auth.pyGoogle workspace auth tests updated for directory scanning🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests