Adding ADAL wrapper to msrestazure #8

lmazuel · 2016-11-29T20:04:19Z

After discussion with @begoldsm, this allows people other than us (Python team or CLI team) to connect using ADAL, without forcing on ADAL features.

Samples:

from msrestazure.azure_active_directory import AdalAuthentication
import adal

context = adal.AuthenticationContext('https://login.microsoftonline.com/common')
token_retriver = lambda: context.acquire_token_with_username_password(
    'https://management.core.windows.net/',
    'unittest@myaccountoutlook.onmicrosoft.com',
    'PASSWORD!!!',
    '04b07795-8ddb-461a-bbee-02f9e1bf7b46' # Xplat CLI or yours
)
return AdalAuthentication(token_retriver)

This is a 99% copy/paste from CLI code, I almost didn't code anything here, just backport the code from @yugangw-msft. I didn't include ADAL in install_requires for now on purpose, I think it's more interesting to do it in extra_requires. And be clear in the documentation. Also the main advantage here is that we don't own any ADAL features, so any new stuff can be used directly without changing msrestazure.

@annatisch @brettcannon what do you think?

lmazuel · 2016-11-29T20:22:03Z

@brettcannon also suggests:

from msrestazure.azure_active_directory import AdalAuthentication
import adal

context = adal.AuthenticationContext('https://login.microsoftonline.com/common')
return AdalAuthentication(
    context.acquire_token_with_username_password,
    'https://management.core.windows.net/',
    'unittest@myaccountoutlook.onmicrosoft.com',
    'PASSWORD!!!',
    '04b07795-8ddb-461a-bbee-02f9e1bf7b46' # Xplat CLI or yours
)

IOW, use args/kwargs to avoid the lambda syntax

lmazuel · 2016-11-29T20:32:50Z

Adding support for @brettcannon syntax. Now both syntax (lambda + args/kwargs) are supported the same, depending of the coders habits.

begoldsm · 2016-11-29T21:40:41Z

msrestazure/azure_active_directory.py

+            #pylint: disable=no-member
+            if (hasattr(err, 'error_response') and ('error_description' in err.error_response)
+                    and ('AADSTS70008:' in err.error_response['error_description'])):
+                raise Expired("Credentials have expired due to inactivity. Please run 'az login'")


consider changing this error message, since this is not the CLI and they can't run az login.

begoldsm · 2016-11-29T22:02:47Z

brettcannon · 2016-11-29T22:50:31Z

msrestazure/azure_active_directory.py

@@ -39,10 +39,16 @@
    MismatchingStateError,
    OAuth2Error,
    TokenExpiredError)
-from requests import RequestException
+from requests import (
+    RequestException,


There's no need to make this span multiple lines. Same goes for other imports.

brettcannon · 2016-11-29T22:51:17Z

msrestazure/azure_active_directory.py

@@ -525,3 +531,32 @@ def set_token(self, response_url):
            raise_with_traceback(AuthenticationError, "", err)
        else:
            self.token = token
+
+class AdalAuthentication(Authentication):#pylint: disable=too-few-public-methods


Two spaces between : and #.

brettcannon · 2016-11-29T22:51:43Z

msrestazure/azure_active_directory.py

@@ -525,3 +531,32 @@ def set_token(self, response_url):
            raise_with_traceback(AuthenticationError, "", err)
        else:
            self.token = token
+
+class AdalAuthentication(Authentication):#pylint: disable=too-few-public-methods


Probably a docstring that explains what the class is about. Same goes for methods.

brettcannon · 2016-11-29T22:52:21Z

msrestazure/azure_active_directory.py

+    def signed_session(self):
+        session = super(AdalAuthentication, self).signed_session()
+
+        import adal # Adal is not mandatory


If you make adal mandatory then this should move to the top of the file.

brettcannon · 2016-11-29T22:52:38Z

msrestazure/azure_active_directory.py

+            if (hasattr(err, 'error_response') and ('error_description' in err.error_response)
+                    and ('AADSTS70008:' in err.error_response['error_description'])):
+                raise Expired("Credentials have expired due to inactivity.")
+


Replace the blank line with an else: clause.

brettcannon · 2016-11-29T22:53:31Z

msrestazure/azure_active_directory.py

+
+        header = "{} {}".format(scheme, token)
+        session.headers['Authorization'] = header
+        return session


Missing a newline at the end of the file.

brettcannon · 2016-11-29T22:54:04Z

msrestazure/azure_active_directory.py

+
+        try:
+            raw_token = self._adal_method(*self._args, **self._kwargs)
+            scheme, token = raw_token['tokenType'], raw_token['accessToken']


This should be moved outside of the try block as it won't trigger an AdalError.

lmazuel · 2016-11-30T17:29:11Z

@brettcannon I think I made the changes
@annatisch nothing to say?

annatisch · 2016-11-30T17:48:09Z

@lmazuel - not really, it's nice and simple :).
Unless you want to hook in the auto-token-refreshing?

brettcannon · 2016-11-30T18:05:43Z

test/unittest_auth.py

+            raise ConnectionError("Plug the network")
+        credentials = AdalAuthentication(connection_error)
+        with self.assertRaises(AuthenticationError) as cm:
+            session = credentials.signed_session()

 if __name__ == '__main__':
    unittest.main()


Missing a newline at the end of the file.

done this one

brettcannon · 2016-11-30T18:08:01Z

msrestazure/azure_active_directory.py

+
+
+class AdalAuthentication(Authentication):  # pylint: disable=too-few-public-methods
+    """A wrapper to use ADAL for Python easily to authenticate on Azure.


Put the closing """ on the same line and have a newline between it and __init__().

General question: should I PEP8 or/and flake8 or/and PyLint or/and mypy?

Adding ADAL wrapper to msrestazure

1ef292e

lmazuel added the enhancement label Nov 29, 2016

Add args/kwargs syntax feature to AdalAuthentication

81864b9

begoldsm suggested changes Nov 29, 2016

View reviewed changes

Remove az login message in Expired

5e482e8

brettcannon suggested changes Nov 29, 2016

View reviewed changes

lmazuel added 4 commits November 29, 2016 17:16

Add Adal tests

0718a6c

Add Adal in dependencies

fc316fc

Merge branch 'master' into adal_test

b071fa5

Brett review on AdalAuthentication

24d7105

brettcannon reviewed Nov 30, 2016

View reviewed changes

brettcannon approved these changes Nov 30, 2016

View reviewed changes

lmazuel added 2 commits November 30, 2016 10:09

Brett review on test

72cd157

Brett review fixes

82532cf

lmazuel merged commit 335a926 into master Nov 30, 2016

lmazuel mentioned this pull request Nov 30, 2016

Implement authentication for usernames/passwords using adal #3

Closed

lmazuel deleted the adal_test branch November 30, 2016 19:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding ADAL wrapper to msrestazure #8

Adding ADAL wrapper to msrestazure #8

lmazuel commented Nov 29, 2016

lmazuel commented Nov 29, 2016 •

edited

Loading

lmazuel commented Nov 29, 2016

begoldsm Nov 29, 2016

begoldsm commented Nov 29, 2016

brettcannon Nov 29, 2016

brettcannon Nov 29, 2016

brettcannon Nov 29, 2016

brettcannon Nov 29, 2016

brettcannon Nov 29, 2016

brettcannon Nov 29, 2016

brettcannon Nov 29, 2016

lmazuel commented Nov 30, 2016

annatisch commented Nov 30, 2016

brettcannon Nov 30, 2016

lmazuel Nov 30, 2016

brettcannon Nov 30, 2016

lmazuel Nov 30, 2016

lmazuel Nov 30, 2016



		class AdalAuthentication(Authentication): # pylint: disable=too-few-public-methods
		"""A wrapper to use ADAL for Python easily to authenticate on Azure.

Adding ADAL wrapper to msrestazure #8

Adding ADAL wrapper to msrestazure #8

Conversation

lmazuel commented Nov 29, 2016

lmazuel commented Nov 29, 2016 • edited Loading

lmazuel commented Nov 29, 2016

Choose a reason for hiding this comment

begoldsm commented Nov 29, 2016

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

Choose a reason for hiding this comment

lmazuel commented Nov 30, 2016

annatisch commented Nov 30, 2016

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

lmazuel commented Nov 29, 2016 •

edited

Loading