Add KeyChain Cache, Plain-Text File Cache, and Cache Factory#15
Add KeyChain Cache, Plain-Text File Cache, and Cache Factory#15marstr merged 29 commits intoAzureAD:devfrom
Conversation
This reverts commit f8f61bd.
…mplementation that can be overriden
Previously, implementations lived in their platform specific files. However, this created an import cycle that pylint was complaining about.
marstr
changed the title
This accomplishes a couple of things: - Establishes that _read and _write SHOULD error out if they are unable to do their job. This way different places in code base can choose what is and is not an acceptable place to fail a read or write operation. - Keeps the contract that derived class implementers need to fulfill very simple.
…ne strategically with kwargs
|
Got word from @rayluo offline that there were still some outstanding problems with the Error factory. I've decided to just go ahead and consolidate the different Error Types into one. It's easier to add to public surface area than remove, after all. |
msal_extensions/token_cache.py
Outdated
| def get_protected_token_cache( | ||
| cache_location=None, | ||
| lock_location=None, | ||
| enforce_encryption=False, **kwargs): |
There was a problem hiding this comment.
From a usability standpoint, wouldn't it be somewhat misleading that such constructor's name is get_protected_token_cache(...), yet one of the optional parameter is enforce_encryption=False? It would cause confusion for dev who reads our api signature and even its docs, but not reading its implementation (which means the majority of devs), "am I supposed to always enable enforce_encryption or what?".
Would better make the encryption_thing=True the default behavior. And then of course we would probably want to NOT blow out by RuntimeError. A noticeable warning would be fine. Please see my another comment for a more detailed proposal.
msal_extensions/token_cache.py
Outdated
| from .osx import Keychain | ||
|
|
||
|
|
||
| def get_protected_token_cache( |
There was a problem hiding this comment.
What if we skip this helper completely, and just define the following into the __init__.py? It would be conceptually one less layer of helper, from the perspective of app devs. And it is believed to be the pattern from the python os package.
if sys.platform.startswith('win'):
from .token_cache import WindowsTokenCache as TokenCache
elif sys.platform.startswith('darwin'):
from .token_cache import OSXTokenCache as TokenCache
else:
from .token_cache import UnencryptedTokenCache as TokenCacheAnd then theUnencryptedTokenCache (formerly the inherited from the base FileTokenCache) constructor can have nothing else but that "no encryption" warning behavior built-in, even without a dedicated flag for it.
class UnencryptedTokenCache(FileTokenCache):
def __init__(self, **kwargs):
warnings.warn("You are using an unprotected token cache for platform {}".format(sys.platform)
# Note: since this becomes the default behavior now,
# we do not want to raise RuntimeError(...) which would break an app on a Linux platform
super(UnencryptedTokenCache, self).__init__(**kwargs)There was a problem hiding this comment.
I like this :) Something I wouldn't have thought of, but it definitely simplifies the consumption of this API. I'll have those changes in shortly
rayluo
left a comment
rayluo
left a comment
There was a problem hiding this comment.
Thanks for the tirelessly refining our implementation! Ship it! 
🚢 👍
2 participants
Closes #3
Closes #5
I usually like to keep my PRs small. This time I got tantalized into exploding the size to do some integration test with the Azure CLI.
We are rapidly approaching functional parity with my initial prototype, but with much much cleaner code.