-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[Communication] - Phone Number Management - Added support for AAD auth #16075
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 |
---|---|---|
|
@@ -38,8 +38,7 @@ | |
UpdatePhoneNumberCapabilitiesResponse | ||
) | ||
|
||
from .._shared.utils import parse_connection_str | ||
from .._shared.policy import HMACCredentialsPolicy | ||
from .._shared.utils import parse_connection_str, get_authentication_policy | ||
|
||
class PhoneNumberAdministrationClient(object): | ||
"""Azure Communication Services Phone Number Management client. | ||
|
@@ -70,7 +69,7 @@ def __init__( | |
self._endpoint = endpoint | ||
self._phone_number_administration_client = PhoneNumberAdministrationClientGen( | ||
self._endpoint, | ||
authentication_policy=HMACCredentialsPolicy(endpoint, credential), | ||
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. wondering if we really need the isAsync flag here? 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. yes, because the get_authentication_policy function receives that optional parameter and passes it to the HMACCredentialPolicy object. Like this:
|
||
authentication_policy=get_authentication_policy(endpoint, credential, is_async=True), | ||
sdk_moniker=SDK_MONIKER, | ||
**kwargs) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
# ------------------------------------------------------------------------- | ||
# Copyright (c) Microsoft Corporation. All rights reserved. | ||
# Licensed under the MIT License. See License.txt in the project root for | ||
# license information. | ||
# -------------------------------------------------------------------------- | ||
from azure.core.credentials import AccessToken | ||
|
||
class FakeTokenCredential(object): | ||
def __init__(self): | ||
self.token = AccessToken("Fake Token", 0) | ||
|
||
def get_token(self, *args): | ||
return self.token |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
# ------------------------------------------------------------------------ | ||
# Copyright (c) Microsoft Corporation. All rights reserved. | ||
# Licensed under the MIT License. See License.txt in the project root for | ||
# license information. | ||
# ------------------------------------------------------------------------- | ||
from .fake_token_credential import FakeTokenCredential | ||
from azure.identity import DefaultAzureCredential | ||
|
||
def create_token_credential(): | ||
# type: () -> FakeTokenCredential or DefaultAzureCredential | ||
from devtools_testutils import is_live | ||
if not is_live(): | ||
return FakeTokenCredential() | ||
return DefaultAzureCredential() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
interactions: | ||
- request: | ||
body: null | ||
headers: | ||
Accept: | ||
- application/json | ||
Accept-Encoding: | ||
- gzip, deflate | ||
Connection: | ||
- keep-alive | ||
User-Agent: | ||
- azsdk-python-communication-administration/1.0.0b4 Python/3.8.5 (Windows-10-10.0.19041-SP0) | ||
method: GET | ||
uri: https://sanitized.communication.azure.com/administration/phonenumbers/phonenumbers?locale=en-US&skip=0&take=100&api-version=2020-07-20-preview1 | ||
response: | ||
body: | ||
string: '{"phoneNumbers": "sanitized", "nextLink": null}' | ||
headers: | ||
content-type: | ||
- application/json; charset=utf-8 | ||
date: | ||
- Thu, 28 Jan 2021 18:48:48 GMT | ||
ms-cv: | ||
- /rMw3sZH40i74G71li+lDA.0 | ||
request-context: | ||
- appId= | ||
transfer-encoding: | ||
- chunked | ||
x-processing-time: | ||
- 1124ms | ||
status: | ||
code: 200 | ||
message: OK | ||
version: 1 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
interactions: | ||
- request: | ||
body: null | ||
headers: | ||
Accept: | ||
- application/json | ||
User-Agent: | ||
- azsdk-python-communication-administration/1.0.0b4 Python/3.8.5 (Windows-10-10.0.19041-SP0) | ||
method: GET | ||
uri: https://sanitized.communication.azure.com/administration/phonenumbers/phonenumbers?locale=en-US&skip=0&take=100&api-version=2020-07-20-preview1 | ||
response: | ||
body: | ||
string: '{"phoneNumbers": "sanitized", "nextLink": null}' | ||
headers: | ||
content-type: application/json; charset=utf-8 | ||
date: Thu, 28 Jan 2021 18:49:47 GMT | ||
ms-cv: jqpEP/JsFUKDau9Y0tMhbw.0 | ||
request-context: appId= | ||
transfer-encoding: chunked | ||
x-processing-time: 765ms | ||
status: | ||
code: 200 | ||
message: OK | ||
url: https://sanitized.communication.azure.com/administration/phonenumbers/phonenumbers?locale=en-US&skip=0&take=100&api-version=2020-07-20-preview1 | ||
version: 1 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,15 +12,17 @@ | |
NumberUpdateCapabilities, | ||
CreateSearchOptions | ||
) | ||
from azure.core.credentials import AccessToken | ||
from phone_number_helper import PhoneNumberUriReplacer | ||
from phone_number_testcase import PhoneNumberCommunicationTestCase | ||
from _shared.testcase import BodyReplacerProcessor | ||
from _shared.utils import create_token_credential | ||
from azure.communication.administration._shared.utils import parse_connection_str | ||
|
||
SKIP_PHONE_NUMBER_TESTS = True | ||
PHONE_NUMBER_TEST_SKIP_REASON= "Phone Number Administration live tests infra not ready yet" | ||
|
||
class PhoneNumberAdministrationClientTest(PhoneNumberCommunicationTestCase): | ||
|
||
def setUp(self): | ||
super(PhoneNumberAdministrationClientTest, self).setUp() | ||
self.recording_processors.extend([ | ||
|
@@ -125,6 +127,15 @@ def setUp(self): | |
self.capabilities_id = "capabilities_id" | ||
self.release_id = "release_id" | ||
|
||
@pytest.mark.live_test_only | ||
@pytest.mark.skipif(SKIP_PHONE_NUMBER_TESTS, reason=PHONE_NUMBER_TEST_SKIP_REASON) | ||
def test_list_all_phone_numbers_from_managed_identity(self): | ||
endpoint, access_key = parse_connection_str(self.connection_str) | ||
credential = create_token_credential() | ||
phone_number_client = PhoneNumberAdministrationClient(endpoint, credential) | ||
sacheu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
pages = phone_number_client.list_all_phone_numbers() | ||
assert pages.next() | ||
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. should we check if the # of phone numbers we get match with what we expected? 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. We don't know which (or how many) phone numbers we're going to receive exactly, that's why this test only checks that we indeed returned something. This test is also copied from the current list_all_phone_numbers test from this same file, just with the added managed identity authentication. |
||
|
||
@pytest.mark.live_test_only | ||
@pytest.mark.skipif(SKIP_PHONE_NUMBER_TESTS, reason=PHONE_NUMBER_TEST_SKIP_REASON) | ||
def test_list_all_phone_numbers(self): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
# -------------------------------------------------------------------------- | ||
import pytest | ||
from azure.communication.administration.aio import PhoneNumberAdministrationClient | ||
from azure.communication.administration._shared.utils import parse_connection_str | ||
from azure.communication.administration import ( | ||
PstnConfiguration, | ||
NumberUpdateCapabilities, | ||
|
@@ -14,6 +15,7 @@ | |
from phone_number_helper import PhoneNumberUriReplacer | ||
from phone_number_testcase_async import AsyncPhoneNumberCommunicationTestCase | ||
from _shared.testcase import BodyReplacerProcessor, ResponseReplacerProcessor | ||
from _shared.utils import create_token_credential | ||
import os | ||
|
||
SKIP_PHONE_NUMBER_TESTS = True | ||
|
@@ -126,6 +128,20 @@ def setUp(self): | |
self.capabilities_id = "capabilities_id" | ||
self.release_id = "release_id" | ||
|
||
@AsyncPhoneNumberCommunicationTestCase.await_prepared_test | ||
@pytest.mark.live_test_only | ||
@pytest.mark.skipif(SKIP_PHONE_NUMBER_TESTS, reason=PHONE_NUMBER_TEST_SKIP_REASON) | ||
async def test_list_all_phone_numbers_from_managed_identity(self): | ||
endpoint, access_key = parse_connection_str(self.connection_str) | ||
credential = create_token_credential() | ||
phone_number_client = PhoneNumberAdministrationClient(endpoint, credential) | ||
async with phone_number_client: | ||
pages = phone_number_client.list_all_phone_numbers() | ||
items = [] | ||
async for item in pages: | ||
items.append(item) | ||
assert len(items) > 0 | ||
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. should we add assert to check for what we get in item? instead of just making sure the len(items) is not empty? 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. Same as other comment. |
||
|
||
@AsyncPhoneNumberCommunicationTestCase.await_prepared_test | ||
@pytest.mark.live_test_only | ||
@pytest.mark.skipif(SKIP_PHONE_NUMBER_TESTS, reason=PHONE_NUMBER_TEST_SKIP_REASON) | ||
|
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.
Do we need to remove this line (line 37), when there is an addition of line 34?
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.
We're keeping both forms of authentication, so line 37 is still relevant
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.
True, but it is quite weird to look at it in the code snippet so maybe keep only one in the first snippet.. then write a regular text saying that you could also authenticate using the connection string and then have a separate snippet containing just the separate constructor.. that way it would easier to look at and understand better.