feature_get_users_groups - #59
Conversation
BinoyOza-okta
left a comment
BinoyOza-okta
left a comment
There was a problem hiding this comment.
Hi @emanor-okta,
Thanks for this — list_user_groups fills a real gap (today you can list group→users but not user→groups). The scope wiring, ID validation, and README/scope-matrix updates all look right, and the test coverage of the validation/error paths is genuinely good.
Before this can merge, three things need to change:
- Missing
@json_responsedecorator. Every tool in this codebase uses the order@mcp.tool()→@require_scopes→@validate_ids→@json_response→async def. This tool skips@json_responseand returns raw Okta SDKGroupmodel objects, which aren't JSON-serializable — this is exactly the bug class fixed in #90 (issue #14, "standardize MCP tool responses to valid JSON"). Confirmed locally:json.dumps(Group(...))raisesTypeError: Object of type Group is not JSON serializable. Please add@json_responseas the innermost decorator, matching e.g.get_applicationinapplications.py. - No pagination.
list_group_users(the mirror-image tool) takesfetch_all/after/limitand routes throughbuild_query_params/paginate_all_results/create_paginated_response. This tool makes one unpaginated call, so a user in more groups than fit on one page gets silently truncated with no signal that more exist. Please mirror thelist_group_userspattern. - Stale branch.
uv.lock'sokta-mcp-serverversion hunk reverts0.1.0 → 1.1.0;mainis currently at1.1.3. Please rebase onto latestmainand regenerate the lockfile so this hunk disappears.
A couple of smaller nits inline below. Happy to re-review once these are addressed!
|
|
||
| @mcp.tool() | ||
| @require_scopes("okta.users.read", error_return_type="list") | ||
| @validate_ids("user_id") |
There was a problem hiding this comment.
Missing @json_response here as the innermost decorator (should sit between this and async def). Without it, this tool returns raw Okta SDK objects instead of JSON — see the top-level comment for details.
| @mcp.tool() | ||
| @require_scopes("okta.users.read", error_return_type="list") | ||
| @validate_ids("user_id") | ||
| async def list_user_groups(user_id: str, ctx: Context = None) -> list: |
There was a problem hiding this comment.
Nit: most tools in this codebase take ctx: Context as the first parameter with no default. list_group_users uses the same ctx placement as this PR, so this isn't strictly wrong, but double-check against whichever sibling tool you're most closely mirroring for consistency. Not a functional issue since FastMCP injects Context by type annotation regardless of position.
| client = await get_okta_client(manager) | ||
| logger.debug(f"Calling Okta API to list groups for user {user_id}") | ||
|
|
||
| groups, _, err = await client.list_user_groups(user_id) |
There was a problem hiding this comment.
This call isn't paginated. list_group_users in groups.py (the read-the-other-direction tool) accepts fetch_all/after/limit and calls build_query_params(...) / paginate_all_results(...). A user belonging to more groups than one page returns will be silently truncated here. Please add pagination support to match.
| return [] | ||
|
|
||
| logger.info(f"Successfully retrieved {len(groups)} groups for user {user_id}") | ||
| return groups |
There was a problem hiding this comment.
This returns a list of raw Okta SDK Group Pydantic objects. Without @json_response wrapping this function, FastMCP can't serialize these to JSON content. Once @json_response is added above, this line can stay as-is — the decorator will normalize the return value via to_jsonable.
| except Exception as e: | ||
| logger.error(f"Exception while listing groups for user {user_id}: {type(e).__name__}: {e}") | ||
| return [f"Exception: {e}"] |
There was a problem hiding this comment.
Once @json_response is added, this outer catch-all becomes redundant — the decorator already traps any exception that escapes the tool body into a structured failure envelope. Other tools only use their own try/except to translate the SDK's (result, response, err) tuple into an error response, letting genuinely unexpected exceptions bubble up to @json_response. Recommend dropping this except Exception block so error shapes stay consistent across the codebase.
| client.list_user_groups.return_value = (groups, MagicMock(), None) | ||
| mock_get_client.return_value = client | ||
|
|
||
| result = await list_user_groups(user_id=USER_ID, ctx=_make_ctx()) |
There was a problem hiding this comment.
This test (and its assertions) will keep passing even after @json_response is added, since it asserts on live attribute access against MagicMock objects rather than verifying the tool's actual output is JSON-serializable. Once @json_response is added, please also add/update a test asserting the returned shape is plain dict/list/primitives (e.g. json.dumps(result) doesn't raise), so a future regression here gets caught.
| [[package]] | ||
| name = "okta-mcp-server" | ||
| version = "0.1.0" | ||
| version = "1.1.0" |
There was a problem hiding this comment.
This reverts the published version from 1.1.3 (current main) down to 1.1.0 — looks like an artifact of this branch being cut a while back. Please rebase onto latest main and regenerate uv.lock so this unrelated diff goes away.
2 participants
Adds the capability to retrieve all the groups a user is assigned to using API https://developer.okta.com/docs/api/openapi/okta-management/management/tags/userresources/other/listusergroups
Currently the MCP server allows getting all users assigned to a group, but there is no way to get all groups a user is assigned to without an exhaustive search.