Skip to content

Commit 8aca851

Browse files
CopilotMattB-msft
andauthored
Add test coverage for multi-tenant auth methods and fix regex pattern bug (#309)
* Initial plan * Add comprehensive test coverage for _resolve_authority and _resolve_tenant_id methods; fix regex bug Co-authored-by: MattB-msft <10568244+MattB-msft@users.noreply.github.com> * Address code review feedback: improve test naming and add clarifying comment Co-authored-by: MattB-msft <10568244+MattB-msft@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: MattB-msft <10568244+MattB-msft@users.noreply.github.com>
1 parent a63c920 commit 8aca851

2 files changed

Lines changed: 151 additions & 1 deletion

File tree

libraries/microsoft-agents-authentication-msal/microsoft_agents/authentication/msal/msal_auth.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,7 @@ def _resolve_authority(
164164

165165
if config.AUTHORITY:
166166
return re.sub(
167-
r"/\/(?:common|[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12})(?=\/|$)/",
167+
r"/(?:common|[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12})(?=/|$)",
168168
f"/{tenant_id}",
169169
config.AUTHORITY,
170170
)

tests/authentication_msal/test_msal_auth.py

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,10 @@
33

44
from microsoft_agents.authentication.msal import MsalAuth
55
from microsoft_agents.hosting.core import Connections
6+
from microsoft_agents.hosting.core.authorization import (
7+
AgentAuthConfiguration,
8+
AuthTypes,
9+
)
610

711
from tests._common.testing_objects import MockMsalAuth
812

@@ -63,6 +67,152 @@ async def test_acquire_token_on_behalf_of_confidential(self, mocker):
6367
)
6468

6569

70+
class TestMsalAuthTenantResolution:
71+
"""
72+
Test suite for testing tenant resolution methods in MsalAuth.
73+
These methods are critical for multi-tenant authentication support.
74+
"""
75+
76+
def test_resolve_tenant_id_with_override_parameter(self):
77+
"""Test that tenant_id parameter takes precedence when provided"""
78+
config = AgentAuthConfiguration(
79+
tenant_id="12345678-1234-1234-1234-123456789abc"
80+
)
81+
result = MsalAuth._resolve_tenant_id(config, "tenant-override")
82+
assert result == "tenant-override"
83+
84+
def test_resolve_tenant_id_with_common_and_tenant_parameter(self):
85+
"""Test that tenant_id parameter is used when config.TENANT_ID is 'common'"""
86+
config = AgentAuthConfiguration(tenant_id="common")
87+
result = MsalAuth._resolve_tenant_id(config, "specific-tenant")
88+
assert result == "specific-tenant"
89+
90+
def test_resolve_tenant_id_with_common_no_tenant_parameter(self):
91+
"""Test that None is returned when config.TENANT_ID is 'common' and no tenant_id provided"""
92+
config = AgentAuthConfiguration(tenant_id="common")
93+
result = MsalAuth._resolve_tenant_id(config, None)
94+
assert result is None
95+
96+
def test_resolve_tenant_id_with_specific_tenant(self):
97+
"""Test that config.TENANT_ID is returned when it's a specific value"""
98+
config = AgentAuthConfiguration(
99+
tenant_id="12345678-1234-1234-1234-123456789abc"
100+
)
101+
result = MsalAuth._resolve_tenant_id(config, None)
102+
assert result == "12345678-1234-1234-1234-123456789abc"
103+
104+
def test_resolve_tenant_id_no_config_tenant_with_parameter(self):
105+
"""Test that tenant_id parameter is used when config.TENANT_ID is not set.
106+
Note: tenant_id can be any string, not just GUID format."""
107+
config = AgentAuthConfiguration()
108+
result = MsalAuth._resolve_tenant_id(config, "fallback-tenant")
109+
assert result == "fallback-tenant"
110+
111+
def test_resolve_tenant_id_no_config_tenant_no_parameter(self):
112+
"""Test that ValueError is raised when neither config.TENANT_ID nor tenant_id are set"""
113+
config = AgentAuthConfiguration()
114+
with pytest.raises(
115+
ValueError, match="TENANT_ID is not set in the configuration"
116+
):
117+
MsalAuth._resolve_tenant_id(config, None)
118+
119+
def test_resolve_authority_with_common_replacement(self):
120+
"""Test that /common is replaced with the resolved tenant_id in authority URL"""
121+
config = AgentAuthConfiguration(
122+
tenant_id="12345678-1234-1234-1234-123456789abc",
123+
authority="https://login.microsoftonline.com/common",
124+
)
125+
result = MsalAuth._resolve_authority(config, None)
126+
assert (
127+
result
128+
== "https://login.microsoftonline.com/12345678-1234-1234-1234-123456789abc"
129+
)
130+
131+
def test_resolve_authority_with_tenant_guid_replacement(self):
132+
"""Test that existing tenant GUID is replaced with new tenant_id in authority URL"""
133+
config = AgentAuthConfiguration(
134+
tenant_id="12345678-1234-1234-1234-123456789abc",
135+
authority="https://login.microsoftonline.com/aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee",
136+
)
137+
result = MsalAuth._resolve_authority(
138+
config, "new-tenant-11111111-2222-3333-4444-555555555555"
139+
)
140+
assert (
141+
result
142+
== "https://login.microsoftonline.com/new-tenant-11111111-2222-3333-4444-555555555555"
143+
)
144+
145+
def test_resolve_authority_with_common_and_tenant_parameter(self):
146+
"""Test that /common is replaced with provided tenant_id parameter"""
147+
config = AgentAuthConfiguration(
148+
tenant_id="common", authority="https://login.microsoftonline.com/common"
149+
)
150+
result = MsalAuth._resolve_authority(
151+
config, "override-22222222-3333-4444-5555-666666666666"
152+
)
153+
assert (
154+
result
155+
== "https://login.microsoftonline.com/override-22222222-3333-4444-5555-666666666666"
156+
)
157+
158+
def test_resolve_authority_no_authority_configured(self):
159+
"""Test fallback to default URL when no authority is configured"""
160+
config = AgentAuthConfiguration(
161+
tenant_id="12345678-1234-1234-1234-123456789abc"
162+
)
163+
result = MsalAuth._resolve_authority(config, None)
164+
assert (
165+
result
166+
== "https://login.microsoftonline.com/12345678-1234-1234-1234-123456789abc"
167+
)
168+
169+
def test_resolve_authority_no_authority_with_tenant_override(self):
170+
"""Test fallback to default URL with tenant override when no authority is configured"""
171+
config = AgentAuthConfiguration(
172+
tenant_id="12345678-1234-1234-1234-123456789abc"
173+
)
174+
result = MsalAuth._resolve_authority(
175+
config, "override-99999999-8888-7777-6666-555555555555"
176+
)
177+
assert (
178+
result
179+
== "https://login.microsoftonline.com/override-99999999-8888-7777-6666-555555555555"
180+
)
181+
182+
def test_resolve_authority_with_common_no_tenant_parameter(self):
183+
"""Test behavior when config.TENANT_ID is 'common' and no tenant_id parameter"""
184+
config = AgentAuthConfiguration(
185+
tenant_id="common", authority="https://login.microsoftonline.com/common"
186+
)
187+
# When tenant_id is None after resolution, should return original authority
188+
result = MsalAuth._resolve_authority(config, None)
189+
assert result == "https://login.microsoftonline.com/common"
190+
191+
def test_resolve_authority_regex_with_trailing_slash(self):
192+
"""Test that regex correctly handles authority URLs with trailing slashes"""
193+
config = AgentAuthConfiguration(
194+
tenant_id="12345678-1234-1234-1234-123456789abc",
195+
authority="https://login.microsoftonline.com/common/",
196+
)
197+
result = MsalAuth._resolve_authority(config, None)
198+
assert (
199+
result
200+
== "https://login.microsoftonline.com/12345678-1234-1234-1234-123456789abc/"
201+
)
202+
203+
def test_resolve_authority_regex_preserves_path(self):
204+
"""Test that regex correctly replaces tenant while preserving additional path segments"""
205+
config = AgentAuthConfiguration(
206+
tenant_id="12345678-1234-1234-1234-123456789abc",
207+
authority="https://login.microsoftonline.com/common/oauth2/v2.0/authorize",
208+
)
209+
result = MsalAuth._resolve_authority(config, None)
210+
assert (
211+
result
212+
== "https://login.microsoftonline.com/12345678-1234-1234-1234-123456789abc/oauth2/v2.0/authorize"
213+
)
214+
215+
66216
# class TestMsalAuthAgentic:
67217

68218
# @pytest.mark.asyncio

0 commit comments

Comments
 (0)