[Core] az logout: Remove service principal access tokens from token cache
#29441
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
️✔️AzureCLI-FullTest
|
|
Hi @jiasli, |
️✔️AzureCLI-BreakingChangeTest
|
|
Core |
microsoft-github-policy-service
bot
added
Account
jiasli
commented
Jul 22, 2024
Comment on lines
+235
to
+236
| # TODO: As MSAL provides no interface to get all service principals in its token cache, this method can't | ||
| # clear all service principals' access tokens from MSAL token cache. |
There was a problem hiding this comment.
I left a comment at AzureAD/microsoft-authentication-library-for-python#666 (comment).
Luckily, when az account clear is called, it internally calls logout_all_users which deletes the whole token cache file, bypassing this issue.
jiasli
commented
Jul 22, 2024
Comment on lines
+222
to
+231
| def logout_service_principal(self, client_id): | ||
| # If client_id is a username, it is ignored | ||
|
|
||
| # Step 1: Remove SP from MSAL token cache | ||
| # Note that removing SP access tokens shouldn't rely on SP store | ||
| cca = ConfidentialClientApplication(client_id, **self._msal_app_kwargs) | ||
| cca.remove_tokens_for_client() | ||
|
|
||
| # Step 2: Remove SP from SP store | ||
| self._service_principal_store.remove_entry(client_id) |
There was a problem hiding this comment.
I originally wrote the code as
def logout_service_principal(self, client_id):
try:
entry = self._service_principal_store.load_entry(client_id, self.tenant_id)
except CLIError as ex:
logger.info('%s %s is possibly a user account.', ex, client_id)
return
# Step 1: Remove service principal access tokens from MSAL token cache.
sp_auth = ServicePrincipalAuth(entry)
cred = ServicePrincipalCredential(sp_auth, **self._msal_app_kwargs)
cred.remove_tokens_for_client()
# Step 2: Remove service principal secrets
self._service_principal_store.remove_entry(client_id)Then I realized removing an SP from token cache doesn't require client_credential, so there is no need to read from self._service_principal_store and create a fully-functioning ServicePrincipalCredential.
|
There is one edge case caused by the "subscription ID as primary key" issue (#15005). If sp1 and sp2 have access to the same subscription, after running To solve that, the user must either
The same happens to user accounts too. |
jiasli
requested review from
evelyn-ys,
calvinhzy,
bebound,
jsntcy,
kairu-ms,
zhoxing-ms and
necusjz
as code owners
bebound
previously approved these changes
Jul 25, 2024
evelyn-ys
previously approved these changes
Jul 25, 2024
bebound
approved these changes
Jul 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related command
az logoutDescription
Based on AzureAD/microsoft-authentication-library-for-python#666 from MSAL 1.27.0 (adopted by #28556).
Testing Guide