-
Notifications
You must be signed in to change notification settings - Fork 12
[1/5] Share authoring identity and deployed-agent discovery #269
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: users/rebova/org-announcements-prerelease
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,6 +70,14 @@ def _escape_odata_literal(value: str, name: str) -> str: | |
| return _validate_odata_string(value, name).replace("'", "''") | ||
|
|
||
|
|
||
| def _validate_title_id(title_id: str) -> str: | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Walkthrough — shared This moves the existing landing-page title-ID rule into the shared helpers: reuse the established string validation and reject identifiers longer than 256 characters. The purpose is one rule for both feature clients. This validator does not replace OData key escaping; callers still pass validated IDs through the existing encoding helper when constructing an agent URL. |
||
| """Validate the opaque EmployeeAgents key shared by authoring surfaces.""" | ||
| _validate_odata_string(title_id, "titleId") | ||
| if len(title_id) > 256: | ||
| raise ValueError("titleId must not exceed 256 characters") | ||
| return title_id | ||
|
|
||
|
|
||
| def _require_odata_id(value: str, name: str) -> str: | ||
| """Validate a non-empty, control-char-free id and encode it as an OData key.""" | ||
| return urllib.parse.quote(_escape_odata_literal(value, name), safe="") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| # Copyright (c) Microsoft Corporation. | ||
| # Licensed under the MIT License. | ||
|
|
||
| """Read-only deployed-agent discovery shared by feature-owned MCP providers.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from typing import Any | ||
|
|
||
| from base_client import AgentConfigApiError, AgentConfigBaseClient | ||
|
|
||
|
|
||
| _MAX_SEARCH_LENGTH = 256 | ||
|
|
||
|
|
||
| def _convert_key_case(value: Any, *, upper: bool) -> Any: | ||
| if isinstance(value, list): | ||
| return [_convert_key_case(item, upper=upper) for item in value] | ||
| if not isinstance(value, dict): | ||
| return value | ||
| converted: dict[str, Any] = {} | ||
| for key, item in value.items(): | ||
| if key and key[0].isalpha(): | ||
| first = key[0].upper() if upper else key[0].lower() | ||
| converted_key = first + key[1:] | ||
| else: | ||
| converted_key = key | ||
| converted[converted_key] = _convert_key_case(item, upper=upper) | ||
| return converted | ||
|
|
||
|
|
||
| def _to_api_payload(value: Any) -> Any: | ||
| return _convert_key_case(value, upper=True) | ||
|
|
||
|
|
||
| def _to_tool_payload(value: Any) -> Any: | ||
| return _convert_key_case(value, upper=False) | ||
|
|
||
|
|
||
| def _unwrap_agent_collection(payload: Any) -> list[dict[str, Any]]: | ||
| if isinstance(payload, list): | ||
| return payload | ||
| if isinstance(payload, dict) and isinstance(payload.get("value"), list): | ||
| return payload["value"] | ||
| raise AgentConfigApiError( | ||
| "AgentConfiguration API returned an invalid collection response" | ||
| ) | ||
|
|
||
|
|
||
| class AgentDiscoveryClient(AgentConfigBaseClient): | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Walkthrough — extract discovery once, without adding a server. This new Python client contains the agent-list/search behavior moved out of the landing-page client, along with the existing payload-casing helpers and collection unwrapping. It inherits authentication, HTTP transport, and retries from The operations remain The separate |
||
| """Discover deployed titleIds without initializing any feature configuration. | ||
|
|
||
| Feature clients retain their own base URL, transport, and authentication. | ||
| Explicit response conversion keeps discovery independent of each feature's | ||
| canonical payload casing and collection routes. | ||
| """ | ||
|
|
||
| def _agent_collection_path(self) -> str: | ||
| return f"tenants('{self.tenant_id}')/EmployeeAgents" | ||
|
|
||
| async def list_agent_configs(self) -> list[dict[str, Any]]: | ||
| payload = await self._request( | ||
| "GET", self._agent_collection_path(), transform_payload=False | ||
| ) | ||
| return _unwrap_agent_collection(_to_tool_payload(payload)) | ||
|
|
||
| async def search_agents(self, search_string: str) -> list[dict[str, Any]]: | ||
| if not isinstance(search_string, str) or not search_string.strip(): | ||
| raise ValueError("searchString must be a non-empty string") | ||
| normalized = search_string.strip() | ||
| if len(normalized) > _MAX_SEARCH_LENGTH: | ||
| raise ValueError( | ||
| f"searchString must not exceed {_MAX_SEARCH_LENGTH} characters" | ||
| ) | ||
| payload = await self._request( | ||
| "POST", | ||
| f"{self._agent_collection_path()}/SearchAgents", | ||
| json={"SearchString": normalized}, | ||
| transform_payload=False, | ||
| ) | ||
| return _unwrap_agent_collection(_to_tool_payload(payload)) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,6 +42,7 @@ | |
| from typing import Any, Optional | ||
|
|
||
| import httpx | ||
| from portalocker.exceptions import LockException | ||
|
|
||
| from _tenant_context import configured_tenant_id | ||
| from _token_cache import create_token_cache | ||
|
|
@@ -85,15 +86,11 @@ def _resolve_token() -> str: | |
|
|
||
| def _read_token_file(token_file: str) -> str: | ||
| if not os.path.isfile(token_file): | ||
| raise ValueError( | ||
| f"AGENTCONFIG_ACCESS_TOKEN_FILE={token_file!r} does not exist" | ||
| ) | ||
| raise ValueError("AGENTCONFIG_ACCESS_TOKEN_FILE does not exist") | ||
| with open(token_file, "r", encoding="utf-8") as handle: | ||
| token = handle.read().strip() | ||
|
Comment on lines
90
to
91
|
||
| if not token: | ||
| raise ValueError( | ||
| f"AGENTCONFIG_ACCESS_TOKEN_FILE={token_file!r} is empty" | ||
| ) | ||
| raise ValueError("AGENTCONFIG_ACCESS_TOKEN_FILE is empty") | ||
| return token | ||
|
|
||
|
|
||
|
|
@@ -157,18 +154,19 @@ def acquire_token_msal_interactive(expected_tenant_id: str | None = None) -> str | |
| result = _acquire_token_interactive_form_post(app) | ||
|
|
||
| if "access_token" not in result: | ||
| error = result.get("error", "unknown_error") | ||
| description = result.get("error_description", "") | ||
| raise ValueError(f"MSAL sign-in failed ({error}): {description}") | ||
| raise ValueError("AgentConfiguration sign-in failed. Sign in again and retry.") | ||
| return result["access_token"] | ||
|
|
||
|
|
||
| def _refresh_msal_token(tenant_id: str, object_id: str) -> str: | ||
| def account_for_identity(app: Any, tenant_id: str, object_id: str) -> Any | None: | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Walkthrough — reusable identity checks and controlled credential failures. This is the main behavioral file in the slice; it is not a new login/token-cache implementation.
The account-preserving refresh mechanism was already upstream. This slice extracts its matching/checking logic and changes error handling. The separate existing review thread about an unreadable token file during initial construction remains open; this walkthrough does not claim that path is fixed. |
||
| """Find one MSAL home account through its tenant-local profile. | ||
|
|
||
| get_accounts() groups tenant profiles by home account. Its exposed local id | ||
| may belong to another tenant (notably for guests), so match the original | ||
| tenant profile in the shared cache before selecting a grouped account. | ||
| """ | ||
| import msal | ||
|
|
||
| app = _create_msal_app(tenant_id) | ||
| # get_accounts() groups profiles by home account and can expose another | ||
| # tenant's local id. Resolve the original tenant profile through the cache. | ||
| home_account_ids = { | ||
| account["home_account_id"] | ||
| for account in app.token_cache.search(msal.TokenCache.CredentialType.ACCOUNT) | ||
|
|
@@ -182,13 +180,19 @@ def _refresh_msal_token(tenant_id: str, object_id: str) -> str: | |
| account for account in app.get_accounts() | ||
| if account.get("home_account_id") in home_account_ids | ||
| ] | ||
| if len(accounts) != 1: | ||
| return accounts[0] if len(accounts) == 1 else None | ||
|
|
||
|
|
||
| def _refresh_msal_token(tenant_id: str, object_id: str) -> str: | ||
| app = _create_msal_app(tenant_id) | ||
| account = account_for_identity(app, tenant_id, object_id) | ||
| if account is None: | ||
| raise AgentConfigApiError( | ||
| "The original account is unavailable for token refresh. " | ||
| "Sign in again as that account and retry.", | ||
| http_status=401, | ||
| ) | ||
| result = app.acquire_token_silent(_SCOPE, account=accounts[0], force_refresh=True) | ||
| result = app.acquire_token_silent(_SCOPE, account=account, force_refresh=True) | ||
| if not result or not result.get("access_token"): | ||
| raise AgentConfigApiError( | ||
| "Could not refresh the access token for the original account. " | ||
|
|
@@ -250,11 +254,14 @@ def _decode_jwt_payload(token: str) -> dict[str, Any]: | |
| payload_segment = parts[1] | ||
| padded = payload_segment + "=" * (-len(payload_segment) % 4) | ||
| try: | ||
| return json.loads(base64.urlsafe_b64decode(padded)) | ||
| payload = json.loads(base64.urlsafe_b64decode(padded)) | ||
| except (binascii.Error, json.JSONDecodeError, UnicodeDecodeError) as error: | ||
| raise ValueError( | ||
| f"Could not decode AGENTCONFIG_ACCESS_TOKEN payload: {error}" | ||
| "Could not decode AGENTCONFIG_ACCESS_TOKEN payload" | ||
| ) from error | ||
| if not isinstance(payload, dict): | ||
| raise ValueError("AGENTCONFIG_ACCESS_TOKEN payload must be an object") | ||
| return payload | ||
|
|
||
|
|
||
| def _decode_tenant_id_from_jwt(token: str) -> str: | ||
|
|
@@ -287,6 +294,17 @@ def _decode_object_id_from_jwt(token: str) -> Optional[str]: | |
| return None | ||
|
|
||
|
|
||
| def validate_token_identity(token: str, tenant_id: str, object_id: str | None) -> str: | ||
| """Check resource-token context; services still validate and authorize tokens.""" | ||
| _validate_token_tenant(token, tenant_id) | ||
| if not object_id: | ||
| raise ValueError("The authoring account cannot be identified without an oid claim") | ||
| actual = _decode_object_id_from_jwt(token) | ||
| if actual is None or actual.casefold() != object_id.casefold(): | ||
| raise ValueError("The token does not identify the intended authoring account") | ||
| return token | ||
|
|
||
|
|
||
| class AgentConfigBaseClient: | ||
| """Neutral AgentConfiguration client core shared by the landing-page and planner MCPs. | ||
|
|
||
|
|
@@ -326,6 +344,11 @@ def __repr__(self) -> str: | |
| f"tenant_id={self.tenant_id!r}>" | ||
| ) | ||
|
|
||
| @property | ||
| def object_id(self) -> str | None: | ||
| """Captured tenant-local principal context, never a tool argument.""" | ||
| return self._object_id | ||
|
|
||
| def _transform_response(self, payload: Any) -> Any: | ||
| """Surface-specific response key transform; identity in the neutral core. | ||
|
|
||
|
|
@@ -360,7 +383,7 @@ async def aclose(self) -> None: | |
| self._client = None | ||
|
|
||
| def _acquire_replacement_token(self) -> str: | ||
| if self._object_id is None: | ||
| if self.object_id is None: | ||
| raise AgentConfigApiError( | ||
| "The original account cannot be identified for token refresh. " | ||
| "Provide a token with an object id and recreate the client.", | ||
|
|
@@ -377,23 +400,21 @@ def _acquire_replacement_token(self) -> str: | |
| http_status=401, | ||
| ) | ||
| else: | ||
| token = _refresh_msal_token(self.tenant_id, self._object_id) | ||
|
|
||
| _validate_token_tenant(token, self.tenant_id) | ||
| object_id = _decode_object_id_from_jwt(token) | ||
| if object_id is None or object_id.casefold() != self._object_id.casefold(): | ||
| raise AgentConfigApiError( | ||
| "The replacement token does not identify the original account. " | ||
| "Provide a matching token and retry.", | ||
| http_status=401, | ||
| ) | ||
| return token | ||
| token = _refresh_msal_token(self.tenant_id, self.object_id) | ||
| return validate_token_identity(token, self.tenant_id, self.object_id) | ||
|
|
||
| async def _refresh_token(self, rejected_token: str) -> None: | ||
| async with self._token_lock: | ||
| if self._token != rejected_token: | ||
| return | ||
| token = await asyncio.to_thread(self._acquire_replacement_token) | ||
| try: | ||
| token = await asyncio.to_thread(self._acquire_replacement_token) | ||
| except (ValueError, LockException, OSError) as error: | ||
| raise AgentConfigApiError( | ||
| "Could not renew credentials for the original account. " | ||
| "Provide matching credentials and retry.", | ||
| http_status=401, | ||
| ) from error | ||
| if token == rejected_token: | ||
| raise AgentConfigApiError( | ||
| "No replacement access token is available. " | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,52 +28,20 @@ | |
| from _odata import ( # noqa: E402 | ||
| _require_odata_id, | ||
| _validate_https_base_url, | ||
| _validate_odata_string, | ||
| _validate_title_id, | ||
| ) | ||
| from base_client import AgentConfigApiError, AgentConfigBaseClient # noqa: E402 | ||
| from agent_discovery import ( # noqa: E402 | ||
| AgentDiscoveryClient, | ||
| _to_api_payload, | ||
| _to_tool_payload, | ||
| ) | ||
| from base_client import AgentConfigApiError as AgentConfigApiError # noqa: E402 | ||
|
|
||
|
|
||
| DEFAULT_AGENTCONFIG_BASE_URL = "https://substrate.office.com/weveb2/api/v1.1" | ||
| _MAX_TITLE_ID_LENGTH = 256 | ||
| _MAX_SEARCH_LENGTH = 256 | ||
|
|
||
|
|
||
| def _validate_title_id(title_id: str) -> str: | ||
| _validate_odata_string(title_id, "titleId") | ||
| if len(title_id) > _MAX_TITLE_ID_LENGTH: | ||
| raise ValueError( | ||
| f"titleId must not exceed {_MAX_TITLE_ID_LENGTH} characters" | ||
| ) | ||
| return title_id | ||
|
|
||
|
|
||
| def _convert_key_case(value: Any, *, upper: bool) -> Any: | ||
| """Recursively convert the first character of JSON object keys.""" | ||
| if isinstance(value, list): | ||
| return [_convert_key_case(item, upper=upper) for item in value] | ||
| if not isinstance(value, dict): | ||
| return value | ||
|
|
||
| converted: dict[str, Any] = {} | ||
| for key, item in value.items(): | ||
| if key and key[0].isalpha(): | ||
| first = key[0].upper() if upper else key[0].lower() | ||
| converted_key = first + key[1:] | ||
| else: | ||
| converted_key = key | ||
| converted[converted_key] = _convert_key_case(item, upper=upper) | ||
| return converted | ||
|
|
||
|
|
||
| def _to_api_payload(value: Any) -> Any: | ||
| return _convert_key_case(value, upper=True) | ||
|
|
||
|
|
||
| def _to_tool_payload(value: Any) -> Any: | ||
| return _convert_key_case(value, upper=False) | ||
|
|
||
|
|
||
| class AgentConfigClient(AgentConfigBaseClient): | ||
| class AgentConfigClient(AgentDiscoveryClient): | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Walkthrough — the deleted discovery code moved to the shared layer.
Landing-page-specific create/get/update operations stay here. The client retains its base URL and normal payload conversion, and Read this deletion alongside |
||
| """Async client for production EmployeeAgents list/search/create/get/PATCH. | ||
|
|
||
| Inherits auth, token decode, the httpx session, and the retrying | ||
|
|
@@ -97,41 +65,12 @@ def _transform_response(self, payload: Any) -> Any: | |
| return _to_tool_payload(payload) | ||
|
|
||
| def _collection_path(self) -> str: | ||
| return f"tenants('{self.tenant_id}')/EmployeeAgents" | ||
| return self._agent_collection_path() | ||
|
|
||
| def _agent_path(self, title_id: str) -> str: | ||
| encoded = _require_odata_id(_validate_title_id(title_id), "titleId") | ||
| return f"{self._collection_path()}('{encoded}')" | ||
|
|
||
| @staticmethod | ||
| def _unwrap_collection(payload: Any) -> list[dict[str, Any]]: | ||
| if isinstance(payload, list): | ||
| return payload | ||
| if isinstance(payload, dict) and isinstance(payload.get("value"), list): | ||
| return payload["value"] | ||
| raise AgentConfigApiError( | ||
| "AgentConfiguration API returned an invalid collection response" | ||
| ) | ||
|
|
||
| async def list_agent_configs(self) -> list[dict[str, Any]]: | ||
| payload = await self._request("GET", self._collection_path()) | ||
| return self._unwrap_collection(payload) | ||
|
|
||
| async def search_agents(self, search_string: str) -> list[dict[str, Any]]: | ||
| if not isinstance(search_string, str) or not search_string.strip(): | ||
| raise ValueError("searchString must be a non-empty string") | ||
| normalized = search_string.strip() | ||
| if len(normalized) > _MAX_SEARCH_LENGTH: | ||
| raise ValueError( | ||
| f"searchString must not exceed {_MAX_SEARCH_LENGTH} characters" | ||
| ) | ||
| payload = await self._request( | ||
| "POST", | ||
| f"{self._collection_path()}/SearchAgents", | ||
| json={"SearchString": normalized}, | ||
| ) | ||
| return self._unwrap_collection(payload) | ||
|
|
||
| async def create_agent_config(self, title_id: str) -> dict[str, Any]: | ||
| title_id = _validate_title_id(title_id) | ||
| return await self._request( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Walkthrough — run CI for this review stack, without opening every feature branch.
The PR-target filter adds exactly our prerelease branch and
users/rebova/org-announcements-review-*. Without those entries, the stacked PRs would miss CI because they target each other rather thanmainorrelease/**.The other change in this file adds the existing
tests/scripts/test_auth.pyto the foundation job because authentication is a shared dependency. Existing push filters, PR event types, and permissions are retained. The announcement-specific CI job is not part of this first slice; it arrives with the announcement modules in subsequent chunks.