Skip to content
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

[Python] LRO and AAD updates and fixes #1078

Merged
merged 5 commits into from
May 25, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -169,13 +169,11 @@ def test_lro_happy_paths(self):
process = self.client.lr_os.delete_provisioning202_accepted200_succeeded()
self.assertEqual("Succeeded", process.result().provisioning_state)

# TODO: In C# this doesn't raise
self.assertRaisesWithMessage("Long running operation failed with status 'Canceled'",
self.client.lr_os.delete_provisioning202_deletingcanceled200().result)
result = self.client.lr_os.delete_provisioning202_deletingcanceled200().result()
self.assertEqual(result.provisioning_state, 'Canceled')

# TODO: In C# this doesn't raise
self.assertRaisesWithMessage("Long running operation failed with status 'Failed'",
self.client.lr_os.delete_provisioning202_deleting_failed200().result)
result = self.client.lr_os.delete_provisioning202_deleting_failed200().result()
self.assertEqual(result.provisioning_state, 'Failed')

self.assertIsNone(self.client.lr_os.post202_no_retry204(product).result())

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
# --------------------------------------------------------------------------

import ast
import re
import time
try:
from urlparse import urlparse, parse_qs
Expand Down Expand Up @@ -92,7 +93,7 @@ def _https(uri, *extra):
return _build_url(uri, extra, 'https')


class AADMixin(object):
class AADMixin(OAuthTokenAuthentication):
"""Mixin for Authentication object.
Provides some AAD functionality:
- State validation
Expand All @@ -107,6 +108,7 @@ class AADMixin(object):
_resource = 'https://management.core.windows.net/'
_china_resource = "https://management.core.chinacloudapi.cn/"
_keyring = "AzureAAD"
_case = re.compile('([a-z0-9])([A-Z])')

def _configure(self, **kwargs):
"""Configure authentication endpoint.
Expand Down Expand Up @@ -153,7 +155,17 @@ def _check_state(self, response):
raise ValueError(
"State received from server does not match that of request.")

def _convert_token(self, token):
"""Convert token fields from camel case.

:param dict token: An authentication token.
:rtype: dict
"""
return {self._case.sub(r'\1_\2', k).lower(): v
for k, v in token.items()}

def _parse_token(self):
# TODO: We could also check expires_on and use to update expires_in
if self.token.get('expires_at'):
countdown = float(self.token['expires_at']) - time.time()
self.token['expires_in'] = countdown
Expand Down Expand Up @@ -216,7 +228,70 @@ def clear_cached_token(self):
raise_with_traceback(KeyError, "Unable to clear token.")


class UserPassCredentials(OAuthTokenAuthentication, AADMixin):
class AADRefreshMixin(object):
"""
Additional token refresh logic
"""

def refresh_session(self):
"""Return updated session if token has expired, attempts to
refresh using newly acquired token.

:rtype: requests.Session.
"""
if self.token.get('refresh_token'):
try:
return self.signed_session()
except Expired:
pass
self.set_token()
return self.signed_session()


class AADTokenCredentials(AADMixin):
"""
Credentials objects for AAD token retrieved through external process
e.g. Python ADAL lib.

Optional kwargs may include:
- china (bool): Configure auth for China-based service,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather than having a bool for China why not make the Crendentials class accept an AzureEnvironment object (Default is public azure).
From the environment object the credentials object will take the authentication endpoint. This will make it scalable as Azure keeps on adding more environments (USGovernment, BlackForest (German cloud), etc.). This also makes it easy for service teams to add a test environment like DogFood, etc. when they want to record tests against an internal envt. before going GA.
We did the same thing in node.js

We also added wrapper methods (interactive, withUsernamePassword, withServicePrincipalSecret) with defaults to provide a similar experience like CLI.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Later, the python CLI can just call these wrapper methods. So the grunt work remains in one central place and users of python sdk and CLI both benefit from this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plus @lmazuel,
Sure I can look into that.

Copy link
Member

@lmazuel lmazuel May 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amarzavery I like the AzureEnvironment, this is something we clearly need.
One question, how do you spread the various Url into your clients? Example, how do you put managementEndpointUrl as the baseUrl of all Swagger ARM generated client? This is a manual operation?

creds = UserPassCredentials()
client=ResourceClient(crds, sub_id, base_url=creds.env.managementEndpointUrl)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, everybody will have to write this all the time.
If you change the env in creds, this change your client automatically, so it's the best code to be "generic"
Because I don't see a way to relate ResourceClient and managementEndpointUrl automatically.
Could the Swagger have a new annotation or something to tag the host, to be replaced on runtime by the correct cloud Host?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of taking the baseUri as a parameter in the constructor, it would be nice, if the user can provide the name of the environment. Default should be public azure. The generated code should have the base uri like this:

function StorageManagementClient(credentials, options) {
   . . .
   . . . .
  if (!options.environment) {
    options.environment = msrestAzure.AzureEnvironment.Azure;
  }
  this.environment = options.environment;
  this.baseuri = this.environment.resourceManagerEndpointUrl;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amarzavery How Autorest can generate this:

this.baseuri = this.environment.resourceManagerEndpointUrl;

Copy link
Member

@lmazuel lmazuel May 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For instance for Datalake, you want this:

this.baseuri = this.environment.azureDataLakeStoreFileSystemEndpointSuffix;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be an extension. 90% scenario is management clients. 10 % data plane.If the extension is not there then we should default to baseuri.
Anyways, @annatisch would kill me by now ;). We have already digressed from the main point of this PR.

I will catch all the conversation we had over here and file a github issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lmazuel @annatisch - Here is the github issue for the above discussion #1082. We can discuss this in the upcoming bi weekly meeting :).

default is 'False'.
- tenant (str): Alternative tenant, default is 'common'.
- auth_uri (str): Alternative authentication endpoint.
- token_uri (str): Alternative token retrieval endpoint.
- resource (str): Alternative authentication resource, default
is 'https://management.core.windows.net/'.
- verify (bool): Verify secure connection, default is 'True'.
- keyring (str): Name of local token cache, default is 'AzureAAD'.
- cached (bool): If true, will not attempt to collect a token,
which can then be populated later from a cached token.

:param dict token: Authentication token.
:param str client_id: Client ID, if not set, Xplat Client ID
will be used.
"""

def __init__(self, token, client_id=None, **kwargs):
if not client_id:
# Default to Xplat Client ID.
client_id = '04b07795-8ddb-461a-bbee-02f9e1bf7b46'
super(AADTokenCredentials, self).__init__(client_id, None)
self._configure(**kwargs)
if not kwargs.get('cached'):
self.token = self._convert_token(token)
self.signed_session()

@classmethod
def retrieve_session(cls, client_id=None):
"""Create AADTokenCredentials from a cached token if it has not
yet expired.
"""
session = cls(None, None, client_id=client_id, cached=True)
session._retrieve_stored_token()
return session


class UserPassCredentials(AADRefreshMixin, AADMixin):
"""Credentials object for Headless Authentication,
i.e. AAD authentication via username and password.

Expand Down Expand Up @@ -298,7 +373,7 @@ def set_token(self):
self.token = token


class ServicePrincipalCredentials(OAuthTokenAuthentication, AADMixin):
class ServicePrincipalCredentials(AADRefreshMixin, AADMixin):
"""Credentials object for Service Principle Authentication.
Authenticates via a Client ID and Secret.

Expand Down Expand Up @@ -361,7 +436,7 @@ def set_token(self):
self.token = token


class InteractiveCredentials(OAuthTokenAuthentication, AADMixin):
class InteractiveCredentials(AADMixin):
"""Credentials object for Interactive/Web App Authentication.
Requires that an AAD Client be configured with a redirect URL.

Expand Down
Loading