Added az account refresh support - Update Subscriptions by listonb · Pull Request #3627 · Azure/azure-cli

listonb · 2017-06-07T00:40:30Z

az account refresh - Get oauth bearer token, refresh if needed, flush token to cache, then pull a list of all subscriptions for each tenant and flush to cache.

#919

msftclas · 2017-06-07T00:40:34Z

@listonb,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

…on list under the active account

codecov-io · 2017-06-07T03:18:11Z

Codecov Report

Merging #3627 into master will increase coverage by 0.4%.
The diff coverage is 27.27%.

@@            Coverage Diff            @@
##           master    #3627     +/-   ##
=========================================
+ Coverage   71.62%   72.03%   +0.4%     
=========================================
  Files         420      422      +2     
  Lines       26209    26319    +110     
  Branches     3968     3982     +14     
=========================================
+ Hits        18773    18959    +186     
+ Misses       6258     6142    -116     
- Partials     1178     1218     +40

Impacted Files	Coverage Δ
...profile/azure/cli/command_modules/profile/_help.py	`100% <100%> (ø)`	⬆️
...ure-cli-core/azure/cli/core/adal_authentication.py	`72.72% <100%> (+2.72%)`	⬆️
...file/azure/cli/command_modules/profile/commands.py	`72.22% <100%> (+1.63%)`	⬆️
...rofile/azure/cli/command_modules/profile/custom.py	`34.17% <33.33%> (-0.04%)`	⬇️
src/azure-cli-core/azure/cli/core/_profile.py	`83.7% <8.33%> (-4.3%)`	⬇️
...c/azure-cli-testsdk/azure/cli/testsdk/utilities.py	`46.34% <0%> (-7.32%)`	⬇️
src/azure-cli-core/azure/cli/core/cloud.py	`70.31% <0%> (-2.29%)`	⬇️
...ice/azure/cli/command_modules/appservice/custom.py	`73.3% <0%> (-0.37%)`	⬇️
...s/azure-cli-interactive/azclishell/az_completer.py	`59.9% <0%> (-0.19%)`	⬇️
...yvault/azure/cli/command_modules/keyvault/_help.py	`100% <0%> (ø)`	⬆️
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14cd4ed...58695a8. Read the comment docs.

yugangw-msft · 2017-06-07T20:17:36Z

src/azure-cli-core/azure/cli/core/_profile.py

+        # Currently requires a default subscription selected
+        existingSubscriptions = self.load_cached_subscriptions()
+        tenants = {}
+        bearer = self.get_raw_token()


Under this flow, we should not manipulate tokens directly. Only az login uses naked token because there might have no established creds, but for refresh, we do have.
Here is the prototype I would do with simpler logics (The TODOs below skip a few trivial logics to focus on the main thing)

from azure.mgmt.resource import SubscriptionClient subscriptions = self.load_cached_subscriptions() client_infos = [] # TODO: eliminate redundant clients from the same tenant for s in subscriptions: client_infos.append((SubscriptionClient(self.get_login_credentials()[0]),s)) refreshed_subs = [] for c in client_infos: result = list(c[0].subscriptions.list()) for r in result: setattr(r, 'tenant_id', c[1][_TENANT_ID]) refreshed_subs += Profile._normalize_properties(c[1][_USER_ENTITY][_USER_NAME], result, c[1][_USER_ENTITY][_USER_TYPE]==_SERVICE_PRINCIPAL) # TODO: # a: extract out following common code with "find_subscriptions_on_login", # b. handle 'tenant only accounts' (no subscriptions) # c: ensure we maintain the active subscription self._set_subscriptions(refreshed_subs) return deepcopy(refreshed_subs)

yugangw-msft · 2017-06-07T20:18:25Z

src/azure-cli-core/azure/cli/core/_profile.py

+                "token": bearer[0][1],
+                "finder": SubscriptionFinder(athctx, self._creds_cache.adal_token_cache)})
+
+        for tenant in tenants:


We should support new or removed tenants. If you decide not to do it, please open a tracking issue

Please add unit tests

Refactored az account refresh to support tenant/subscription addition/removal. Additionally will auth to new tenants. Refactored AdalAuthentication module to no longer expect a method to be passed in as an arg, but expose a in-built method for _token_retriever for backwards compatibility. This allows other portions of the CLI to take advantage of the AdalAuthentication module.

devigned · 2017-06-09T21:10:59Z

Looks good @listonb! @yugangw-msft what do you think?

yugangw-msft

Thanks for contributing. I left a few more comments. Looks like my previous comments were not all addressed, please handle them as well.

yugangw-msft · 2017-06-09T22:11:26Z

src/azure-cli-core/azure/cli/core/_profile.py

+        from azure.cli.core.adal_authentication import AdalAuthentication
+        from azure.mgmt.resource import SubscriptionClient
+        # If no subscription is currently set CLI will throw error asking user to set a subscription or login
+        subClient = SubscriptionClient(self.get_login_credentials()[0])


Are we only refreshing the current azure account or across all of them? Please note, users could login with multiple accounts, and different service principals.

yugangw-msft · 2017-06-09T22:12:59Z

src/azure-cli-core/azure/cli/core/_profile.py

+            # if none exists auto-generates a new oauth token for this tenant
+            self.auth_ctx_factory(tenant, self._creds_cache.adal_token_cache)
+            self._creds_cache.load_adal_token_cache()
+            token = self._creds_cache.retrieve_token_for_user(tokenRetriever[2]['userId'],


Are we skipping the scenario of refreshing for a service principal?

yugangw-msft · 2017-06-09T22:20:41Z

src/azure-cli-core/azure/cli/core/_profile.py

+            # Attempt to pull a cred cache for each tenant
+            # if none exists auto-generates a new oauth token for this tenant
+            self.auth_ctx_factory(tenant, self._creds_cache.adal_token_cache)
+            self._creds_cache.load_adal_token_cache()


I don't believe we should touch the token cache directly.
The only time we need to touch the raw token is when we need to get refreshed tenant list by starting from the common tenant, after that the logic inside _find_using_common_tenant should be reused

yugangw-msft · 2017-06-09T22:27:02Z

src/azure-cli-core/azure/cli/core/adal_authentication.py

-    def __init__(self, token_retriever):
-        self._token_retriever = token_retriever
+    def __init__(self, token):
+        self.token = token


We should not do this. We like to use a token with guaranteed validity, so this is why we acquire one right before sending out the request on the wire. Some commands take a long time to run, say vmss with 100 instances, the change here will break it because the same token used in the subsequent polling might get expired.

yugangw-msft · 2017-07-28T18:30:49Z

#4091 will handle it

msftclas added the cla-not-required label Jun 7, 2017

listonb force-pushed the master branch from f3a3d4f to e647b5f Compare June 7, 2017 01:35

Added az account refresh support - This refreshes existing subscripti…

88742e8

…on list under the active account

listonb force-pushed the master branch from e647b5f to 88742e8 Compare June 7, 2017 03:07

JasonRShaver requested a review from yugangw-msft June 7, 2017 16:43

yugangw-msft suggested changes Jun 7, 2017

View reviewed changes

yugangw-msft suggested changes Jun 9, 2017

View reviewed changes

yugangw-msft added the do-not-merge label Jun 9, 2017

yugangw-msft closed this Jul 28, 2017

haroldrandom added cla-not-required do-not-merge labels Oct 25, 2019

ChaseCrum mentioned this pull request May 4, 2023

AZ CLI calls unsupported lib for Ubuntu 20.04 #26338

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added az account refresh support - Update Subscriptions#3627

Added az account refresh support - Update Subscriptions#3627
listonb wants to merge 2 commits intoAzure:masterfrom
listonb:master

listonb commented Jun 7, 2017 •

edited

Loading

Uh oh!

msftclas commented Jun 7, 2017

Uh oh!

codecov-io commented Jun 7, 2017 •

edited

Loading

Uh oh!

yugangw-msft Jun 7, 2017 •

edited

Loading

Uh oh!

yugangw-msft Jun 7, 2017 •

edited

Loading

Uh oh!

yugangw-msft Jun 7, 2017

Uh oh!

devigned commented Jun 9, 2017

Uh oh!

yugangw-msft left a comment

Uh oh!

yugangw-msft Jun 9, 2017

Uh oh!

yugangw-msft Jun 9, 2017

Uh oh!

yugangw-msft Jun 9, 2017

Uh oh!

yugangw-msft Jun 9, 2017

Uh oh!

yugangw-msft commented Jul 28, 2017 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

listonb commented Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

#919

Uh oh!

msftclas commented Jun 7, 2017

Uh oh!

codecov-io commented Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yugangw-msft Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yugangw-msft Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yugangw-msft Jun 7, 2017

Choose a reason for hiding this comment

Uh oh!

devigned commented Jun 9, 2017

Uh oh!

yugangw-msft left a comment

Choose a reason for hiding this comment

Uh oh!

yugangw-msft Jun 9, 2017

Choose a reason for hiding this comment

Uh oh!

yugangw-msft Jun 9, 2017

Choose a reason for hiding this comment

Uh oh!

yugangw-msft Jun 9, 2017

Choose a reason for hiding this comment

Uh oh!

yugangw-msft Jun 9, 2017

Choose a reason for hiding this comment

Uh oh!

yugangw-msft commented Jul 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

listonb commented Jun 7, 2017 •

edited

Loading

codecov-io commented Jun 7, 2017 •

edited

Loading

yugangw-msft Jun 7, 2017 •

edited

Loading

yugangw-msft Jun 7, 2017 •

edited

Loading

yugangw-msft commented Jul 28, 2017 •

edited

Loading