Fix validation logic for gchat trigger#26
Merged
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR replaces the Google Chat integration's legacy HMAC-based verification token flow with bearer token authentication using Google-signed JWTs. The implementation adds PyJWT dependency, redesigns 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
RadCod3
force-pushed
the
add-chat-triggers
branch
from
e9059e5 to
4b78af2
Compare
RadCod3
force-pushed
the
add-chat-triggers
branch
2 times, most recently
from
7ff2121 to
ead9595
Compare
Previously we used thread_name to generate the session_id. However, each gchat message has its own thread thereby giving all messages a new session with the agent. We can consider supporting replying within thread later. Until then lets use space_name:user_name as the default session_id temp Fix tests after prev fix
RadCod3
force-pushed
the
add-chat-triggers
branch
from
ead9595 to
9da422e
Compare
Member
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
This PR updates the Google Chat platform-chat integration to replace the previous payload token comparison with JWT-based verification using the Authorization: Bearer ... header and Google JWKS, aligning behavior with the described Ballerina approach.
Changes:
- Replace
platform_config.verification_tokenwithplatform_config.project_numberorplatform_config.endpoint_urland validate mutual exclusivity. - Verify Google Chat requests via JWT + JWKS before JSON parsing (raw-request verification using the Authorization header).
- Update tests and fixture AFM examples to reflect the new configuration and verification behavior.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| python-interpreter/packages/afm-core/src/afm/interfaces/platform_chat/gchat.py | Implements bearer-token extraction and JWT verification via Google JWKS; updates GChat config fields and runtime verification flow. |
| python-interpreter/packages/afm-core/src/afm/cli.py | Updates inline type-suppression comments near router creation calls. |
| python-interpreter/packages/afm-core/pyproject.toml | Adds pyjwt[crypto] dependency required for JWT/JWKS verification. |
| python-interpreter/packages/afm-core/tests/test_platform_chat.py | Updates tests for new GChat config fields and Authorization-header verification; adjusts session id expectations. |
| python-interpreter/packages/afm-core/tests/test_parser.py | Updates parser tests to expect new GChat platform_config fields. |
| python-interpreter/packages/afm-core/tests/fixtures/sample_gchat_platform_chat_agent.afm.md | Updates sample fixture to use project_number. |
| python-interpreter/packages/afm-core/tests/fixtures/sample_gchat_platform_chat_sync_agent.afm.md | Updates sample fixture to use endpoint_url. |
Comments suppressed due to low confidence (1)
python-interpreter/packages/afm-core/src/afm/interfaces/platform_chat/gchat.py:214
get_gchat_session_idno longer considersmessage.thread.namewhen building the session id (it now only usesspace+user). This is a behavior change unrelated to the PR’s stated purpose (token validation) and may break session continuity for threaded conversations. If intentional, it should be called out in the PR description; otherwise consider restoring the thread-based session id logic.
def get_gchat_session_id(payload: object) -> str:
if not isinstance(payload, dict):
return "default"
space = payload.get("space")
space_name = (
_non_empty_string(space.get("name")) if isinstance(space, dict) else None
)
if space_name is None:
return "gchat:unknown-space:default"
user = payload.get("user")
if isinstance(user, dict):
user_name = _non_empty_string(user.get("name"))
if user_name:
return f"gchat:{space_name}:{user_name}"
return f"gchat:{space_name}:default"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
190
to
195
| if webchat_interface is not None: | ||
| webchat_router = create_webchat_router( | ||
| agent, | ||
| webchat_interface.signature, | ||
| webchat_path, # type: ignore[arg-type] | ||
| webchat_path, # ty:ignore[invalid-argument-type] | ||
| ) |
Comment on lines
+114
to
+116
| if not isinstance(auth_header, str) or not auth_header.startswith("Bearer "): | ||
| return None | ||
| token = auth_header[len("Bearer ") :].strip() |
Comment on lines
+131
to
+137
| try: | ||
| if isinstance(config, HttpEndpointUrlConfig): | ||
| return _verify_id_token(token, config.endpoint_url) | ||
| return _verify_project_number_jwt(token, config.project_number) | ||
| except Exception: | ||
| logger.exception("GChat bearer token verification raised an unexpected error") | ||
| return False |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
python-interpreter/packages/afm-core/tests/test_platform_chat.py (1)
775-845: ⚡ Quick winAdd endpoint-url verification-path endpoint tests for parity.
Current endpoint auth tests exercise the
project_numberbranch only. Add at least one accept/reject test for theendpoint_urlbranch so both verification modes are covered at request-handler level.Proposed test addition
+ `@pytest.mark.asyncio` + async def test_gchat_verification_accepts_valid_bearer_with_endpoint_url_config( + self, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + monkeypatch.setattr( + "afm.interfaces.platform_chat.gchat._verify_oidc_id_token", + lambda *_args, **_kwargs: True, + ) + agent = MagicMock(spec=AgentRunner) + agent.afm = MagicMock() + agent.afm.metadata = MagicMock() + agent.afm.metadata.version = "1.0.0" + agent.afm.metadata.interfaces = [ + PlatformChatInterface( + type="platformchat", + platform="gchat", + mode=PlatformChatMode.NOTIFICATION, + prompt="GChat event: ${http:payload.message.text}", + signature=Signature(input=JSONSchema(type="object")), + platform_config={"endpoint_url": "https://example.com/gchat"}, + exposure=Exposure(http=HTTPExposure(path="/gchat")), + ) + ] + agent.arun = MagicMock() + app = create_platform_chat_app(agent, verify_signatures=True) + + transport = ASGITransport(app=app) + async with AsyncClient(transport=transport, base_url="http://test") as client: + response = await client.post( + "/gchat", + headers={"Authorization": "Bearer pretend-jwt"}, + json={"type": "MESSAGE", "message": {"text": "verified", "sender": {"type": "HUMAN"}}}, + ) + + assert response.status_code == 202🤖 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 `@python-interpreter/packages/afm-core/tests/test_platform_chat.py` around lines 775 - 845, Add tests that cover the endpoint_url verification branch by duplicating the existing bearer tests but patching the endpoint_url verifier; specifically, add a test like test_gchat_endpoint_url_rejects_bad_bearer that monkeypatches "afm.interfaces.platform_chat.gchat._verify_endpoint_url_jwt" to return False, creates the app via create_platform_chat_app(..., verify_signatures=True), posts to "/gchat" with Authorization: "Bearer bad-jwt" and asserts 401 + "Invalid GChat bearer token", and another test like test_gchat_endpoint_url_accepts_valid_bearer that monkeypatches the same function to return True, posts a valid MESSAGE payload and asserts 202 and that ran.wait() completes; use the same mock_gchat_notification_agent fixture and AsyncClient/ASGITransport patterns as the existing tests to ensure parity.
🤖 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 `@python-interpreter/packages/afm-core/pyproject.toml`:
- Line 25: The pyproject specifies "pyjwt[crypto]>=2.8" but gchat.py
(python-interpreter/packages/afm-core/src/afm/interfaces/platform_chat/gchat.py)
calls jwt.decode(..., issuer=list(_GOOGLE_OIDC_ISSUERS), ...), which requires
PyJWT >= 2.10.1; update the dependency in pyproject.toml to
"pyjwt[crypto]>=2.10.1" so list-based issuer validation is supported.
In `@python-interpreter/packages/afm-core/src/afm/cli.py`:
- Line 194: Some inline type-check suppressions use the wrong syntax ("#
ty:ignore[...]") so the type-checker doesn't recognize them; update each
occurrence in afm.cli where you see "# ty:ignore[invalid-argument-type]" (for
example next to the webchat_path argument and the other two occurrences in the
same file) to "# ty: ignore[invalid-argument-type]" so the suppression is valid;
search for the exact string "ty:ignore[invalid-argument-type]" and replace with
"ty: ignore[invalid-argument-type]" inside the relevant function
signatures/argument lists (e.g., the webchat_path parameter) to fix the inline
suppression.
---
Nitpick comments:
In `@python-interpreter/packages/afm-core/tests/test_platform_chat.py`:
- Around line 775-845: Add tests that cover the endpoint_url verification branch
by duplicating the existing bearer tests but patching the endpoint_url verifier;
specifically, add a test like test_gchat_endpoint_url_rejects_bad_bearer that
monkeypatches "afm.interfaces.platform_chat.gchat._verify_endpoint_url_jwt" to
return False, creates the app via create_platform_chat_app(...,
verify_signatures=True), posts to "/gchat" with Authorization: "Bearer bad-jwt"
and asserts 401 + "Invalid GChat bearer token", and another test like
test_gchat_endpoint_url_accepts_valid_bearer that monkeypatches the same
function to return True, posts a valid MESSAGE payload and asserts 202 and that
ran.wait() completes; use the same mock_gchat_notification_agent fixture and
AsyncClient/ASGITransport patterns as the existing tests to ensure parity.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 00fb296f-085b-4edf-825b-c38bc3bee602
⛔ Files ignored due to path filters (1)
python-interpreter/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
python-interpreter/packages/afm-core/pyproject.tomlpython-interpreter/packages/afm-core/src/afm/cli.pypython-interpreter/packages/afm-core/src/afm/interfaces/platform_chat/gchat.pypython-interpreter/packages/afm-core/tests/fixtures/sample_gchat_platform_chat_agent.afm.mdpython-interpreter/packages/afm-core/tests/fixtures/sample_gchat_platform_chat_sync_agent.afm.mdpython-interpreter/packages/afm-core/tests/test_parser.pypython-interpreter/packages/afm-core/tests/test_platform_chat.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Replaces the broken payload-token check in the Google Chat provider with proper JWT verification of the auth header against Google's JWKS, mirroring the Ballerina implementation.
What's new
subscription.provider_config):endpoint_url→ OIDC ID token verified via Google's OIDC JWKS; checksaud == endpoint_url,email == chat@system.gserviceaccount.com,email_verified == true.project_number→ self-signed JWT from the Chat service account, verified via the service-account JWKS; checksaud == project_number,iss == chat@system.gserviceaccount.com.provider_config.verification_tokenfield removed.