Skip to content

handle null expireson #22899

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

Closed
xiangyan99 wants to merge 1 commit into from
Closed

handle null expireson #22899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 5 additions & 2 deletions sdk/identity/azure-identity/azure/identity/_credentials/azure_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
# Copyright (c) Microsoft Corporation.
# Licensed under the MIT License.
# ------------------------------------
from datetime import datetime
from datetime import datetime, timedelta
import json
import os
import platform
Expand Down Expand Up @@ -91,7 +91,10 @@ def parse_token(output):
"""
try:
token = json.loads(output)
dt = datetime.strptime(token["expiresOn"], "%Y-%m-%d %H:%M:%S.%f")
expires_on_in_token = token["expiresOn"]
if not expires_on_in_token:
expires_on_in_token = (datetime.now()+timedelta(days=1)).strftime("%Y-%m-%d %H:%M:%S.%f")
Copy link
Member

@chlowell chlowell Feb 3, 2022

Choose a reason for hiding this comment

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

The token's lifetime depends on the tenant's configuration. We don't know anything about that, so we can't safely guess when the token expires. Underestimating degrades performance; overestimating causes the auth policy to send an expired token. Really this is a CLI bug that should be fixed in the CLI. I think we have two options for handling it here:

  1. use the token only once (we can assume it's valid right now)
  2. raise AuthenticationFailedError

Option 1 makes the application terribly slow; option 2 maybe crashes it 😄

dt = datetime.strptime(expires_on_in_token, "%Y-%m-%d %H:%M:%S.%f")
if hasattr(dt, "timestamp"):
# Python >= 3.3
expires_on = dt.timestamp()
Expand Down
24 changes: 23 additions & 1 deletion sdk/identity/azure-identity/tests/test_cli_credential.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
# Copyright (c) Microsoft Corporation.
# Licensed under the MIT License.
# ------------------------------------
from datetime import datetime
from datetime import datetime, timedelta
import json
import re
import sys
Expand Down Expand Up @@ -214,3 +214,25 @@ def fake_check_output(command_line, **_):
):
token = credential.get_token("scope", tenant_id="un" + expected_tenant)
assert token.token == expected_token

def test_no_expireson_token():
"""The credential should parse the CLI's output to an AccessToken even expiresOn is None"""

access_token = "access token"
expected_expires_on = int(datetime.timestamp(datetime.now()+timedelta(days=1)))
no_expireon_output = json.dumps(
{
"expiresOn": None,
"accessToken": access_token,
"subscription": "some-guid",
"tenant": "some-guid",
"tokenType": "Bearer",
}
)

with mock.patch(CHECK_OUTPUT, mock.Mock(return_value=no_expireon_output)):
token = AzureCliCredential().get_token("scope")

assert token.token == access_token
assert type(token.expires_on) == int
assert token.expires_on == expected_expires_on
26 changes: 25 additions & 1 deletion sdk/identity/azure-identity/tests/test_cli_credential_async.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
# Licensed under the MIT License.
# ------------------------------------
import asyncio
from datetime import datetime
from datetime import datetime, timedelta
import json
import re
import sys
Expand Down Expand Up @@ -247,3 +247,27 @@ async def fake_exec(*args, **_):
with mock.patch.dict("os.environ", {EnvironmentVariables.AZURE_IDENTITY_DISABLE_MULTITENANTAUTH: "true"}):
token = await credential.get_token("scope", tenant_id="un" + expected_tenant)
assert token.token == expected_token


async def test_no_expireson_token():
"""The credential should parse the CLI's output to an AccessToken even expiresOn is None"""

access_token = "access token"
expected_expires_on = int(datetime.timestamp(datetime.now()+timedelta(days=1)))
successful_output = json.dumps(
{
"expiresOn": None,
"accessToken": access_token,
"subscription": "some-guid",
"tenant": "some-guid",
"tokenType": "Bearer",
}
)

with mock.patch(SUBPROCESS_EXEC, mock_exec(successful_output)):
credential = AzureCliCredential()
token = await credential.get_token("scope")

assert token.token == access_token
assert type(token.expires_on) == int
assert token.expires_on == expected_expires_on