Skip to content

{Core} Fix the incorrect token expiration on ADAL tokens #15875

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.

Sign up for GitHub

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

Merged
bim-msft merged 7 commits into from
Nov 12, 2020
Merged

{Core} Fix the incorrect token expiration on ADAL tokens #15875

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e11fea0
init
bim-msft Nov 11, 2020
3df3592
Merge branch 'dev' into bim_fix_token
bim-msft Nov 11, 2020
a7d9cf0
updated
bim-msft Nov 11, 2020
03d466c
Add fallback
bim-msft Nov 12, 2020
0f28a7c
Remove 'if'
bim-msft Nov 12, 2020
2bb621c
updated
bim-msft Nov 12, 2020
175f819
refine format
bim-msft Nov 12, 2020
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
8 changes: 8 additions & 0 deletions src/azure-cli-core/azure/cli/core/adal_authentication.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
# Licensed under the MIT License. See License.txt in the project root for license information.
# --------------------------------------------------------------------------------------------

import datetime
import time
import requests
import adal
Expand Down Expand Up @@ -75,6 +76,13 @@ def get_token(self, *scopes, **kwargs): # pylint:disable=unused-argument
logger.debug("AdalAuthentication.get_token invoked by Track 2 SDK with scopes=%s", scopes)

_, token, full_token, _ = self._get_token(_try_scopes_to_resource(scopes))

try:
expires_on = full_token.get('expiresOn', full_token['expires_on'])
Copy link
Member

@jiasli jiasli Nov 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

For a user token (which has expiresOn), full_token['expires_on'] will be evaluated first and result in a KeyError.

Suggested change
expires_on = full_token.get('expiresOn', full_token['expires_on'])
expires_on = full_token['expiresOn']

Copy link
Member

@jiasli jiasli Nov 12, 2020

Choose a reason for hiding this comment

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

This will also break Cloud Shell because expires_on is an epoch time and should be used directly. Parsing it will result in failure.

return AccessToken(token, int(datetime.datetime.strptime(expires_on, '%Y-%m-%d %H:%M:%S.%f').timestamp()))
except: # pylint: disable=bare-except
pass # To avoid crashes due to some unexpected token formats

try:
return AccessToken(token, int(full_token['expiresIn'] + time.time()))
Copy link
Member

@jiasli jiasli Nov 11, 2020

Choose a reason for hiding this comment

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

Still we need to figure out why expiresIn gets corrupted in the first place.

Copy link
Contributor Author

@bim-msft bim-msft Nov 11, 2020

Choose a reason for hiding this comment

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

@jiasli From my understanding, relative values like expiresIn should not be saved and used. Relative values would become meaningless as time goes by. Or we can recalculate it everytime when we use it, but that will be no difference with using the absolute value expiresOn directly.

Copy link
Member

@jiasli jiasli Nov 12, 2020

Choose a reason for hiding this comment

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

The original code is really "carving the boat to seek the sunk sword" (刻舟求剑).

except KeyError: # needed to deal with differing unserialized MSI token payload
Expand Down