Add AAD support for EG #19421

rakshith91 · 2021-06-23T22:48:46Z

fixes #17963

rakshith91 · 2021-06-28T08:52:46Z

/azp run python - eventgrid - tests

azure-pipelines · 2021-06-28T08:52:58Z

Azure Pipelines successfully started running 1 pipeline(s).

swathipil

small nit, but o/w lgtm

sdk/eventgrid/azure-eventgrid/README.md

yunhaoling · 2021-06-28T16:41:40Z

sdk/eventgrid/azure-eventgrid/azure/eventgrid/aio/_helpers_async.py

+from .. import _constants as constants
+from .._signature_credential_policy import EventGridSasCredentialPolicy
+
+def _get_authentication_policy_async(credential):


nit: I think we could avoid duplicating the _get_authentication_policy method for async by adding a policy type argument in the sync helper.

def _get_authentication_policy(credential, bearer_token_policy=BearerTokenCredentialPolicy): if hasattr(credential, "get_token"): return bearer_token_policy( credential, constants.DEFAULT_EVENTGRID_SCOPE )

then in the async, we do

_get_authentication_policy(credential, AsyncBearerTokenCredentialPolicy)

yunhaoling · 2021-06-28T16:44:06Z

sdk/eventgrid/azure-eventgrid/azure/eventgrid/aio/_helpers_async.py

+def _get_authentication_policy_async(credential):
+    if credential is None:
+        raise ValueError("Parameter 'self._credential' must not be None.")
+    if hasattr(credential, "get_token"):


dumb question: what error would be raised if a user passed a sync AD credential into the async client?

It'll be a type error

rakshith91 · 2021-07-15T18:05:30Z

/azp run python - eventgrid - tests

azure-pipelines · 2021-07-15T18:05:43Z

Azure Pipelines successfully started running 1 pipeline(s).

rakshith91 · 2021-07-16T18:06:17Z

/azp run python - eventgrid - tests

azure-pipelines · 2021-07-16T18:06:30Z

Azure Pipelines successfully started running 1 pipeline(s).

rakshith91 · 2021-07-16T21:38:55Z

/azp run python - eventgrid - tests

azure-pipelines · 2021-07-16T21:39:08Z

Azure Pipelines successfully started running 1 pipeline(s).

Co-authored-by: swathipil <[email protected]>

rakshith91 · 2021-07-16T22:54:42Z

/azp run python - eventgrid - tests

azure-pipelines · 2021-07-16T22:54:54Z

Azure Pipelines successfully started running 1 pipeline(s).

JoshLove-msft · 2021-07-16T23:32:38Z

sdk/eventgrid/azure-eventgrid/README.md

@@ -38,6 +38,34 @@ az eventgrid domain --create --location <location> --resource-group <resource-gr
 In order to interact with the Event Grid service, you will need to create an instance of a client.
 An **endpoint** and **credential** are necessary to instantiate the client object.

+#### Using Azure Active Directory (AAD)


Nice, this reminds me I need to add this for .NET 😄

johanste · 2021-07-16T23:47:56Z

sdk/eventgrid/azure-eventgrid/README.md

+
+To send events to a topic or domain with a `TokenCredential`, the authenticated identity should have the "EventGrid Data Sender" role assigned.
+
+With the `azure-identity` package, you can seamlessly authorize requests in both development and production environments. To learn more about Azure Active Directory, see the [`azure-identity` README](https://github.com/Azure/azure-sdk-for-python/blob/master/sdk/identity/azure-identity/README.md).


We shouldn't link to the old master branch.

good catch - updated

sdk/eventgrid/azure-eventgrid/README.md

johanste · 2021-07-16T23:51:08Z

sdk/eventgrid/azure-eventgrid/azure/eventgrid/aio/_publisher_client_async.py

    _is_cloud_event,
    _is_eventgrid_event,
    _eventgrid_data_typecheck,
    _build_request,
    _cloud_event_to_generated,
+    _get_authentication_policy


Nitpick: missing trailing comma - did you run this through black?

nope - i did not - can do that

EDIT: done

lmazuel · 2021-07-17T00:03:10Z

sdk/eventgrid/azure-eventgrid/azure/eventgrid/aio/_publisher_client_async.py

@@ -73,7 +78,7 @@ class EventGridPublisherClient:
    def __init__(
        self,
        endpoint: str,
-        credential: Union[AzureKeyCredential, AzureSasCredential],
+        credential: Union["AsyncTokenCredential", AzureKeyCredential, AzureSasCredential],


That seems inconsistent, if not used those types should be in the TYPE_CHECKING as well, but I see now reason why some type would be string and some would be types

lmazuel · 2021-07-17T00:04:46Z

sdk/eventgrid/azure-eventgrid/tests/asynctestcase.py

+
+class AsyncEventGridTest(EventGridTest):
+
+    def generate_oauth_token(self):


You shouldn't need that, there is everything you need in devtools to care care of that for free

lmazuel · 2021-07-17T00:05:15Z

sdk/eventgrid/azure-eventgrid/tests/eventgrid_preparer.py

+    def get_oauth_endpoint(self):
+        return os.getenv("EG_TOPIC_HOSTNAME")
+
+    def generate_oauth_token(self):


Same, you don't need that

lmazuel · 2021-07-17T00:05:36Z

sdk/eventgrid/azure-eventgrid/tests/test_eg_publisher_client.py

            client = EventGridPublisherClient("eventgrid_endpoint", bad_credential)
+
+    @pytest.mark.live_test_only


why live only?

little tricky to generate recording given we use resource group preparers and envvars for secrets

rakshith91 · 2021-07-19T05:25:47Z

/azp run python - eventgrid - tests

azure-pipelines · 2021-07-19T05:25:59Z

Azure Pipelines successfully started running 1 pipeline(s).

rakshith91 requested review from lmazuel, swathipil and yunhaoling as code owners June 23, 2021 22:48

ghost added the Event Grid label Jun 23, 2021

swathipil reviewed Jun 28, 2021

View reviewed changes

sdk/eventgrid/azure-eventgrid/README.md Outdated Show resolved Hide resolved

yunhaoling reviewed Jun 28, 2021

View reviewed changes

rakshith91 requested review from abhahn, amishra-dev, ankitarorabit, annatisch, benbp, cartertinney, catalinaperalta, chlowell, danieljurek, digimaun, giakas, iscai-msft, johanste, kasobol-msft, kristapratico, LarryOsterman and lzchen as code owners July 16, 2021 19:00

rakshith91 and others added 8 commits July 16, 2021 15:53

Add AAD support

3722b52

lint

31e4478

tests fix

ca3f361

comments

b73b0f7

unskip tests

af4a5e7

aad tests

7f03c04

Update sdk/eventgrid/azure-eventgrid/README.md

cb12665

Co-authored-by: swathipil <[email protected]>

emdpoint

e7f9405

rakshith91 force-pushed the aad branch from 39b0920 to e7f9405 Compare July 16, 2021 22:54

JoshLove-msft reviewed Jul 16, 2021

View reviewed changes

johanste reviewed Jul 16, 2021

View reviewed changes

rakshith91 commented Jul 16, 2021

View reviewed changes

sdk/eventgrid/azure-eventgrid/README.md Outdated Show resolved Hide resolved

Update sdk/eventgrid/azure-eventgrid/README.md

562faeb

johanste reviewed Jul 16, 2021

View reviewed changes

lmazuel reviewed Jul 17, 2021

View reviewed changes

swathipil approved these changes Jul 18, 2021

View reviewed changes

rakshith91 added 2 commits July 18, 2021 22:20

black formatting

55bd8aa

oauth

f21370d

rakshith91 merged commit 95787ba into Azure:main Jul 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AAD support for EG #19421

Add AAD support for EG #19421

rakshith91 commented Jun 23, 2021

rakshith91 commented Jun 28, 2021

azure-pipelines bot commented Jun 28, 2021

swathipil left a comment

yunhaoling Jun 28, 2021 •

edited

Loading

yunhaoling Jun 28, 2021

rakshith91 Jul 13, 2021

rakshith91 commented Jul 15, 2021

azure-pipelines bot commented Jul 15, 2021

rakshith91 commented Jul 16, 2021

azure-pipelines bot commented Jul 16, 2021

rakshith91 commented Jul 16, 2021

azure-pipelines bot commented Jul 16, 2021

rakshith91 commented Jul 16, 2021

azure-pipelines bot commented Jul 16, 2021

JoshLove-msft Jul 16, 2021

johanste Jul 16, 2021 •

edited

Loading

rakshith91 Jul 16, 2021

johanste Jul 16, 2021

rakshith91 Jul 16, 2021 •

edited

Loading

lmazuel Jul 17, 2021

lmazuel Jul 17, 2021

lmazuel Jul 17, 2021

lmazuel Jul 17, 2021

rakshith91 Jul 19, 2021

rakshith91 commented Jul 19, 2021

azure-pipelines bot commented Jul 19, 2021


		To send events to a topic or domain with a `TokenCredential`, the authenticated identity should have the "EventGrid Data Sender" role assigned.

		With the `azure-identity` package, you can seamlessly authorize requests in both development and production environments. To learn more about Azure Active Directory, see the [`azure-identity` README](https://github.com/Azure/azure-sdk-for-python/blob/master/sdk/identity/azure-identity/README.md).


		class AsyncEventGridTest(EventGridTest):

		def generate_oauth_token(self):

		client = EventGridPublisherClient("eventgrid_endpoint", bad_credential)

		@pytest.mark.live_test_only

Add AAD support for EG #19421

Add AAD support for EG #19421

Conversation

rakshith91 commented Jun 23, 2021

rakshith91 commented Jun 28, 2021

azure-pipelines bot commented Jun 28, 2021

swathipil left a comment

Choose a reason for hiding this comment

yunhaoling Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rakshith91 commented Jul 15, 2021

azure-pipelines bot commented Jul 15, 2021

rakshith91 commented Jul 16, 2021

azure-pipelines bot commented Jul 16, 2021

rakshith91 commented Jul 16, 2021

azure-pipelines bot commented Jul 16, 2021

rakshith91 commented Jul 16, 2021

azure-pipelines bot commented Jul 16, 2021

Choose a reason for hiding this comment

johanste Jul 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rakshith91 Jul 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rakshith91 commented Jul 19, 2021

azure-pipelines bot commented Jul 19, 2021

yunhaoling Jun 28, 2021 •

edited

Loading

johanste Jul 16, 2021 •

edited

Loading

rakshith91 Jul 16, 2021 •

edited

Loading