Skip to content

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

Merged
bim-msft merged 7 commits intoAzure:devfrom
bim-msft:bim_fix_token
Nov 12, 2020
Merged

{Core} Fix the incorrect token expiration on ADAL tokens#15875
bim-msft merged 7 commits intoAzure:devfrom
bim-msft:bim_fix_token

Conversation

@bim-msft
Copy link
Contributor

@bim-msft bim-msft commented Nov 11, 2020

Currently I'm facing with a token expiration issue, and I think I have found the root cause of this. When using ADAL token retriever, token data will be loaded from the local cache (~/.azure/accessTokens.json), incluing two important properties: expiresIn and expiresOn:

Local cache file:
image

Unfortunatelly, our CLI framework just loads all data directly from the cache without any changes:
image

Based on the observation, our current code is incorrect. It just use expiresIn + now to calculate the expiration time, which will be larger than the real one:

_, token, full_token, _ = self._get_token(_try_scopes_to_resource(scopes))
try:
return AccessToken(token, int(full_token['expiresIn'] + time.time()))


This checklist is used to make sure that common guidelines for a pull request are followed.

@bim-msft bim-msft self-assigned this Nov 11, 2020
@bim-msft bim-msft added this to the S178 milestone Nov 11, 2020
Comment on lines +81 to +82
return AccessToken(token, int(time.mktime(
datetime.datetime.strptime(full_token['expiresOn'], '%Y-%m-%d %H:%M:%S.%f').timetuple())))
Copy link
Member

Choose a reason for hiding this comment

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

datetime.timestamp() is an easier way to acquire epoch time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, updated.

@bim-msft bim-msft added the Core CLI core infrastructure label Nov 11, 2020
datetime.datetime.strptime(full_token['expiresOn'], '%Y-%m-%d %H:%M:%S.%f').timetuple())))

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

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

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

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" (刻舟求剑).

@bim-msft bim-msft merged commit 8604287 into Azure:dev Nov 12, 2020
@bim-msft bim-msft deleted the bim_fix_token branch November 12, 2020 04:49
_, 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

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

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.

@qwordy
Copy link
Member

qwordy commented Nov 12, 2020

I also met this problem!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Core CLI core infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants