Split auth demo into Keycloak vs Entra, add a tool for Entra MCP server with group-restricted access #27
Conversation
pamelafox
changed the title
There was a problem hiding this comment.
Pull request overview
This PR refactors the authentication demo by splitting a single auth_mcp module into separate auth_keycloak_mcp and auth_entra_mcp modules to better demonstrate provider-specific functionality. The Entra implementation now includes a group-restricted tool that uses the OAuth On-Behalf-Of (OBO) flow with Microsoft Graph API to verify admin group membership before granting access to aggregate expense statistics.
Key changes include:
- Separation of Keycloak and Entra authentication implementations into dedicated modules
- Addition of group-based access control using Entra ID and Microsoft Graph API
- Automated admin consent grants for required API scopes during app registration
- Updated Python dependencies (fastmcp, azure-monitor-opentelemetry, opentelemetry packages, and others)
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| servers/auth_entra_mcp.py | New Entra-specific MCP server with group-restricted expense statistics tool using OBO flow |
| servers/auth_keycloak_mcp.py | Refactored Keycloak-only authentication server, removing Entra-specific code |
| infra/auth_init.py | Added automated admin consent grant logic with GrantDefinition dataclass |
| infra/server.bicep | Updated MCP entrypoint selection logic to choose between auth_keycloak_mcp, auth_entra_mcp, or deployed_mcp |
| infra/main.bicep | Added entraAdminGroupId parameter for group-based access control |
| infra/main.parameters.json | Added entraAdminGroupId parameter mapping from environment variable |
| infra/write_env.sh | Added ENTRA_ADMIN_GROUP_ID to environment export script |
| infra/write_env.ps1 | Added ENTRA_ADMIN_GROUP_ID to PowerShell environment export script |
| servers/entrypoint.sh | Simplified entrypoint logic with improved default value handling |
| servers/Dockerfile | Updated default MCP_ENTRY value to deployed_mcp for consistency |
| pyproject.toml | Updated dependency versions for fastmcp, azure-monitor-opentelemetry, and opentelemetry packages |
| uv.lock | Lock file updates reflecting new dependency versions and additions |
| README.md | Updated server entrypoint reference from auth_mcp to auth_entra_mcp |
| spanish/README.md | Updated Spanish documentation with correct server entrypoint |
| AGENTS.md | Added documentation for Python dependency update workflow |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except APIError as error: | ||
| status_code = error.response_status_code | ||
| if status_code in {401, 403}: | ||
| print(f"Failed to grant admin consent: {error.message}") | ||
| return |
There was a problem hiding this comment.
When admin consent granting fails with 401 or 403 errors, the function prints an error message and returns early, but the script continues with 'print("✅ Entra app registration setup is complete.")' in the main function. This could mislead users into thinking the setup was successful when admin consent actually failed. Consider having grant_application_admin_consent return a boolean indicating success or failure, and update the main function to reflect the actual outcome.
There was a problem hiding this comment.
Raising a ValueError as other functions do, instead
| graph_resource_access_token = confidential_client.acquire_token_on_behalf_of( | ||
| user_assertion=auth_token, scopes=["https://graph.microsoft.com/.default"] | ||
| ) | ||
| if "error" in graph_resource_access_token: | ||
| logger.error( | ||
| "OBO token acquisition failed: %s", | ||
| graph_resource_access_token.get("error_description", "Unknown error"), | ||
| ) | ||
| return "Error: Unable to verify permissions. Please try again later." | ||
|
|
||
| graph_auth_token = graph_resource_access_token["access_token"] |
There was a problem hiding this comment.
In the get_expense_stats function, the OBO token acquisition is done using MSAL's acquire_token_on_behalf_of without checking the token cache first. While MSAL has built-in caching, the TokenCache() instance is created once at module initialization (line 110). Consider whether caching behavior across requests is intended, as the same cache will be shared by all requests and users. If multiple users call this function concurrently, there could be token cache collisions or unexpected behavior. Consider using a per-request token cache or ensuring thread-safety of the cached tokens.
There was a problem hiding this comment.
Added a comment to TokenCache initialization line above
| query = "SELECT c.category FROM c" | ||
| stats = {} | ||
| async for item in cosmos_container.query_items(query=query): |
There was a problem hiding this comment.
The query 'SELECT c.category FROM c' in get_expense_stats retrieves all expense items from all partitions without any filtering or pagination. For large datasets, this could result in high RU consumption and potentially slow performance. Consider adding pagination using continuation tokens or limiting the query scope, especially if this is intended for production use with many users and expenses.
| query = "SELECT c.category FROM c" | |
| stats = {} | |
| async for item in cosmos_container.query_items(query=query): | |
| # Use TOP to bound the number of items processed per request and avoid unbounded RU consumption | |
| query = "SELECT TOP 1000 c.category FROM c" | |
| stats = {} | |
| async for item in cosmos_container.query_items(query=query, enable_cross_partition_query=True): |
There was a problem hiding this comment.
Adding a comment with suggestions
3 participants
This branch splits the auth_mcp module into auth_entra_mcp and auth_keycloak_mcp, so that we can show more Entra-specific functionality without making the code impossible to follow.
In the improved Entra MCP server, it includes a tool with group-restricted access, using the OBO flow with the Graph API to determine group membership.
How it works:
First it modifies the Entra App registration process to grant admin consent. This is required since we do not have preauthorized applications:
https://learn.microsoft.com/entra/identity-platform/v2-oauth2-on-behalf-of-flow#admin-consent
Then in the actual tool, we use the graph API to list the user's memberships:
https://learn.microsoft.com/en-us/graph/api/user-list-memberof?view=graph-rest-1.0&tabs=http
We only receive back the ID of the groups, no other metadata, as that would likely require higher permissions, like User.Read.All:
https://learn.microsoft.com/en-us/graph/permissions-reference#userreadall
All we need is the ID, however.
Copilot-generated summary of changes:
Entra ID Integration and Infrastructure Updates:
entraAdminGroupIdparameter toinfra/main.bicep,infra/server.bicep, andinfra/main.parameters.json, and ensured it is passed to the server module and exported in environment scripts, enabling admin group-based access control for expense statistics when using Entra Proxy. [1] [2] [3] [4] [5] [6] [7]infra/server.bicepto select the correct MCP entrypoint (auth_keycloak_mcp,auth_entra_mcp, ordeployed_mcp) based on the configured authentication provider.Python Dependency and Documentation Updates:
pyproject.tomlto newer versions for several packages, includingfastmcp,azure-monitor-opentelemetry, andopentelemetry-instrumentation-starlette, and added clear instructions for updating dependencies inAGENTS.md. [1] [2] [3]App Registration and Admin Consent Automation:
infra/auth_init.pyto automate Entra app registration and admin consent grants, including a newGrantDefinitiondataclass and logic to grant admin consent for required scopes if not already present. [1] [2] [3] [4]Deployment and Entrypoint Configuration:
MCP_ENTRYenvironment variable in the Dockerfile todeployed_mcpfor consistency with new entrypoint logic.auth_entra_mcp:app) for local testing with OAuth enabled.branch shows how to use the Graph API with OBO flow to find out the groups of the signed in user, and use that to decide whether to allow access to a particular group.