Skip to content
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.

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

Adjusting token cache format #19

Merged
merged 8 commits into from
Mar 4, 2019
Merged

Adjusting token cache format #19

merged 8 commits into from
Mar 4, 2019

Conversation

rayluo
Copy link
Collaborator

@rayluo rayluo commented Feb 15, 2019

  • Change scope's internal representation from a list to a string
  • Remove client_info from RefreshToken because it was optional and now obsolete
  • Add test case to ensure the token cache representation complies with the Unified Schema
  • The existing token cache implementation is already extendable, we only need to add a test case to assert that.

You can install this branch for your testing:

pip install git+https://github.com/AzureAD/microsoft-authentication-library-for-python.git@target-in-string

@rayluo rayluo force-pushed the target-in-string branch from 1f4b0af to 9110eca Compare March 1, 2019 19:59
Copy link
Contributor

@abhidnya13 abhidnya13 left a comment

Choose a reason for hiding this comment

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

:shipit:

return { # Mimic a real response
"access_token": entry["secret"],
"token_type": "Bearer",
"expires_in": entry["expires_on"] - now,
"expires_in": int(expires_in), # OAuth2 specs defines it as int

Choose a reason for hiding this comment

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

int [](start = 34, length = 3)

it seems like expires_in is already an int? Did -now change it to something else? Should that then have been converted to an int?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question! It is true that the expires_in in the original server-side response is an integer. But here in this code path we hit a token in the cache, and then re-calculate a new expires_in based on its original value and the current time(), which happens to be a float in Python. So we do a Forceful Unobvious Conversion to Keep the Integer Time, at the exact place where it is needed.

@henrik-me
Copy link

        logger.debug(

should this be info or error instead of debug? (not sure how the python logger works though)


Refers to: msal/application.py:322 in 9110eca. [](commit_id = 9110eca, deletion_comment = False)

@@ -38,13 +38,19 @@ def __init__(self):
def find(self, credential_type, target=None, query=None):
target = target or []
assert isinstance(target, list), "Invalid parameter type"
target_set = set(target)

Choose a reason for hiding this comment

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

is target scopes? if yes, do you have a place where you remove dupes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes target is exactly scope in Unified Schema's dialect.

The set(...) method creates a native set object in Python. It automatically deduplicates the scopes.

Copy link

@henrik-me henrik-me left a comment

Choose a reason for hiding this comment

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

:shipit:

@rayluo rayluo merged this pull request into dev Mar 4, 2019
@rayluo rayluo deleted the target-in-string branch March 4, 2019 18:41
@rayluo rayluo mentioned this pull request Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants