Add Google Groups (Cloud Identity API) service#566
Conversation
📝 WalkthroughWalkthroughThis PR introduces comprehensive Google Groups (Cloud Identity API) support to the MCP toolset. Changes include new OAuth scopes and permissions, tool tier definitions, a complete new Google Groups tool module with seven public functions, and integration into the service registry and main application entry points. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main.py (1)
146-159:⚠️ Potential issue | 🟡 MinorMissing "groups" in
--toolsargument choices.The "groups" service is added to
tool_importsandtool_iconsbut is missing from the--toolschoices list. Users won't be able to selectively load the groups tool via--tools groups.🔧 Proposed fix
parser.add_argument( "--tools", nargs="*", choices=[ "gmail", "drive", "calendar", "docs", "sheets", "chat", "forms", "slides", "tasks", "contacts", + "groups", "search", "appscript", ], help="Specify which tools to register. If not provided, all tools are registered.", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 146 - 159, The choices array for the --tools CLI option in main.py is missing "groups", so add "groups" to the choices list (the same place where the current array contains "gmail","drive",...,"appscript") so it matches the entries in tool_imports and tool_icons and allows users to enable the groups tool via --tools groups.
🧹 Nitpick comments (3)
tests/ggroups/test_groups_tools.py (2)
7-10: Consider using pytest'sconftest.pyfor path setup instead ofsys.pathmanipulation.Direct
sys.pathmanipulation works but is brittle. Aconftest.pyat the tests root with proper path configuration would be more maintainable and consistent with pytest best practices.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ggroups/test_groups_tools.py` around lines 7 - 10, Replace the ad-hoc sys.path manipulation in test_groups_tools.py (the sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), "../.."))) / import os / import sys lines) with a pytest conftest.py at the tests root that performs the project-root path setup (e.g., in pytest_configure or by adding to sys.path there) so individual tests don't modify sys.path; then remove the sys.path/os.path import and insertion from test_groups_tools.py and verify imports still resolve under pytest.
223-257: Tests verify module structure and constants correctly.The import and constant tests ensure the module exports expected functions and maintains the expected constant values.
Consider adding integration tests with mocked Google APIs (using httpretty or similar) for the actual tool functions (
search_groups,get_group, etc.) to ensure end-to-end behavior. As per coding guidelines: "Write unit tests for every FastMCP tool and integration tests for high-risk flows."Do you want me to generate example test stubs for the tool functions with mocked Cloud Identity API responses?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ggroups/test_groups_tools.py` around lines 223 - 257, Add integration-style test stubs that mock the Cloud Identity/Google Groups HTTP responses for the tool functions (search_groups, get_group, list_group_members, manage_group_members, list_groups, lookup_group) and assert expected outputs and error handling; use a HTTP mocking library (httpretty or responses) to simulate successful and failing API responses, import and exercise the actual functions from groups_tools, and include a test that verifies the GOOGLE_GROUP_LABEL constant is used correctly in requests or returned values; ensure mocks are torn down between tests and cover at least one success path and one error/edge path per function.ggroups/groups_tools.py (1)
12-13: IncorrectResourcetype import.The
Resourceimported frommcpis the MCP Resource type, not the Google API client resource type. Theserviceparameter in all tools is actually agoogleapiclient.discovery.Resourceobject returned bybuild(). While the type hint won't cause runtime errors (Python's duck typing), it's semantically incorrect.Consider either:
- Using
Anyfor the service type hint (common pattern for dynamically built services)- Importing from
googleapiclient.discoveryif a proper type is needed♻️ Suggested fix
from googleapiclient.errors import HttpError -from mcp import Resource +from typing import Any, Dict, List, OptionalThen update all function signatures to use
service: Anyinstead ofservice: Resource.Alternatively, keep
Resourceunused and useAny:-from mcp import Resource🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ggroups/groups_tools.py` around lines 12 - 13, The file currently imports Resource from mcp which is the wrong type for the Google API client; replace that import with a correct typing approach and update all function signatures that use service: Resource to use service: Any (import Any from typing) OR alternately import Resource from googleapiclient.discovery and use that type; specifically change the top-level import (remove "from mcp import Resource") and update every function that declares service: Resource to service: Any (or service: googleapiclient.discovery.Resource if you choose the latter) so the type hints match the object returned by googleapiclient.discovery.build().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ggroups/groups_tools.py`:
- Around line 479-494: The request body passed to
service.groups().memberships().modifyMembershipRoles (variable body) is invalid:
modifyMembershipRoles must not mix addRoles and removeRoles in one call and both
fields expect arrays of strings (e.g., "MANAGER"), not objects; update the logic
so either (A) only set removeRoles (delete addRoles and send removeRoles as
["MEMBER","OWNER",...]) or (B) perform two calls to modifyMembershipRoles
sequentially — first with addRoles as a list of role strings [role] and then a
second call with removeRoles as a list of role strings (or vice versa); ensure
you call .execute() on each request and keep membership_name and
service.groups().memberships().modifyMembershipRoles as the target for these
corrected calls.
---
Outside diff comments:
In `@main.py`:
- Around line 146-159: The choices array for the --tools CLI option in main.py
is missing "groups", so add "groups" to the choices list (the same place where
the current array contains "gmail","drive",...,"appscript") so it matches the
entries in tool_imports and tool_icons and allows users to enable the groups
tool via --tools groups.
---
Nitpick comments:
In `@ggroups/groups_tools.py`:
- Around line 12-13: The file currently imports Resource from mcp which is the
wrong type for the Google API client; replace that import with a correct typing
approach and update all function signatures that use service: Resource to use
service: Any (import Any from typing) OR alternately import Resource from
googleapiclient.discovery and use that type; specifically change the top-level
import (remove "from mcp import Resource") and update every function that
declares service: Resource to service: Any (or service:
googleapiclient.discovery.Resource if you choose the latter) so the type hints
match the object returned by googleapiclient.discovery.build().
In `@tests/ggroups/test_groups_tools.py`:
- Around line 7-10: Replace the ad-hoc sys.path manipulation in
test_groups_tools.py (the sys.path.insert(0,
os.path.abspath(os.path.join(os.path.dirname(__file__), "../.."))) / import os /
import sys lines) with a pytest conftest.py at the tests root that performs the
project-root path setup (e.g., in pytest_configure or by adding to sys.path
there) so individual tests don't modify sys.path; then remove the
sys.path/os.path import and insertion from test_groups_tools.py and verify
imports still resolve under pytest.
- Around line 223-257: Add integration-style test stubs that mock the Cloud
Identity/Google Groups HTTP responses for the tool functions (search_groups,
get_group, list_group_members, manage_group_members, list_groups, lookup_group)
and assert expected outputs and error handling; use a HTTP mocking library
(httpretty or responses) to simulate successful and failing API responses,
import and exercise the actual functions from groups_tools, and include a test
that verifies the GOOGLE_GROUP_LABEL constant is used correctly in requests or
returned values; ensure mocks are torn down between tests and cover at least one
success path and one error/edge path per function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd40e63b-d121-4ec7-9d06-69835ea311ca
📒 Files selected for processing (11)
README.mdauth/permissions.pyauth/scopes.pyauth/service_decorator.pycore/tool_tiers.yamlfastmcp_server.pyggroups/__init__.pyggroups/groups_tools.pymain.pytests/ggroups/__init__.pytests/ggroups/test_groups_tools.py
| # action == "modify_role" | ||
| body = { | ||
| "addRoles": [{"name": role}], | ||
| "removeRoles": [], | ||
| } | ||
|
|
||
| current_roles = {"MEMBER", "MANAGER", "OWNER"} | ||
| current_roles.discard(role) | ||
| body["removeRoles"] = [r for r in current_roles] | ||
|
|
||
| result = await asyncio.to_thread( | ||
| service.groups() | ||
| .memberships() | ||
| .modifyMembershipRoles(name=membership_name, body=body) | ||
| .execute | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Cloud Identity API modifyMembershipRoles removeRoles format
💡 Result:
For Cloud Identity API groups.memberships.modifyMembershipRoles, the request body field removeRoles is an array of strings, where each string is the name of a MembershipRole you want removed (role names are OWNER, MANAGER, MEMBER). You cannot remove MEMBER; delete the membership instead. [1][2]
Example (remove MANAGER from a membership):
POST https://cloudidentity.googleapis.com/v1/groups/{group}/memberships/{membership}:modifyMembershipRoles
Content-Type: application/json
{
"removeRoles": ["MANAGER"]
}[1][2]
Note: Don’t mix removeRoles with addRoles or updateRolesParams in the same request. [1]
Sources:
[1] Cloud Identity API – groups.memberships.modifyMembershipRoles (REST)
[2] Cloud Identity API – MembershipRole type (role name values)
Remove addRoles or use separate API calls—modifyMembershipRoles does not allow mixing role modifications in a single request.
The Cloud Identity API modifyMembershipRoles endpoint requires that you either add roles OR remove roles in a single request, not both. The current implementation violates this constraint by including both addRoles (line 480) and removeRoles (line 487) in the same request body.
Additionally, both addRoles and removeRoles expect arrays of strings (role names like "MANAGER"), not objects. The current addRoles uses the wrong format [{"name": role}] instead of [role].
To fix, either:
- Only remove roles (delete the
addRoleslogic), or - Make separate sequential calls—one to add the role, then one to remove others
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ggroups/groups_tools.py` around lines 479 - 494, The request body passed to
service.groups().memberships().modifyMembershipRoles (variable body) is invalid:
modifyMembershipRoles must not mix addRoles and removeRoles in one call and both
fields expect arrays of strings (e.g., "MANAGER"), not objects; update the logic
so either (A) only set removeRoles (delete addRoles and send removeRoles as
["MEMBER","OWNER",...]) or (B) perform two calls to modifyMembershipRoles
sequentially — first with addRoles as a list of role strings [role] and then a
second call with removeRoles as a list of role strings (or vice versa); ensure
you call .execute() on each request and keep membership_name and
service.groups().memberships().modifyMembershipRoles as the target for these
corrected calls.
lookup_group is the simplest path from a group email to a resource name needed by other tools — it belongs in the default tool set. Made-with: Cursor
darkdreamingdan
force-pushed
the
feat/add_ggroups
branch
from
a95d956 to
32077cd
Compare
1 participant
Summary
cloudidentityv1)search_groups,get_group,list_group_members,manage_group,manage_group_members,list_groups,lookup_groupcloud-identity.groups,cloud-identity.groups.readonly), service config, permissions, tool tiers, and both entry points (main.py,fastmcp_server.py)Changes
ggroups/__init__.pyggroups/groups_tools.pyauth/scopes.pyGROUPS_SCOPE,GROUPS_READONLY_SCOPE, hierarchy, scope mapsauth/service_decorator.pycloudidentityservice config + scope group aliasesauth/permissions.pygroupspermission levels (readonly/full)core/tool_tiers.yamlgroupstier definitionsmain.pyfastmcp_server.pyREADME.mdtests/ggroups/test_groups_tools.pyTest plan
lookup_groupverified working withgroupKey_idparameter naming (googleapiclient converts dotted REST params to underscored Python kwargs)AI Usage
I am a professional dev, am totally anti-AI Slop. This PR is mostly coded using AI with detailed Plan function and considered prompting. I have tested the MCP for read-only use cases and it has been great. I will move out of Draft when i have reviewed the code, and tested other use cases.
Made with Cursor