Handling OS errors raised if input is empty and unrecognized for the first usage #65
Conversation
rayluo
left a comment
There was a problem hiding this comment.
Thanks for your productive investigation! With the better understanding, we can now:
-
Update the corresponding issue (#64) to describe how to reproduce the issue. Something along these lines: "Cache file previously used with MSAL already exists before using MSAL EXtensions for the very first time. Exception ... and ... would be raised on Mac and Windows."
-
Is the current PR's description precise?
"... errors raised if input is empty ..." and "Empty cache file already exists before using MSAL extensions ...".
Wouldn't a non-empty cache file still trigger this issue?
-
In terms of the actual adjustment, let's try adjust it at there
|
Hey Ray, Thanks for reviewing !
|
rayluo
left a comment
There was a problem hiding this comment.
Your updated description in the original issue #64 looks good!
Is it possible to prepare some test case to mimic those 3 situations, then? Those test cases won't be automated on Travis CI, but it would probably be valuable to have them for our manual testing.
tests/test_persistence.py
Outdated
| # Make sure key chain entry does not already exist for below service name | ||
| persistence = KeychainPersistence(cache_file, "my_service_name", "my_account_name") |
There was a problem hiding this comment.
| # Make sure key chain entry does not already exist for below service name | |
| persistence = KeychainPersistence(cache_file, "my_service_name", "my_account_name") | |
| persistence = KeychainPersistence( | |
| cache_file, "nonexist_service_name", "nonexist_account_name") |
msal_extensions/persistence.py
Outdated
| with open(self._location, 'rb') as handle: | ||
| return self._dp_agent.unprotect(handle.read()) | ||
| data = handle.read() | ||
| return self._dp_agent.unprotect(data) if data else None |
There was a problem hiding this comment.
If data is None, it means the cache is corrupt. Because even if you log out all the users, the cache file should not be empty (it should be an empty json). So you can handle this here, but I would also handle the more general case of "cache is corrupt" (i.e. MSAL fails to parse the cache) and delete the cache file / keychain item / keyring entry
There was a problem hiding this comment.
Agreed, it will be good to have a general recovery method in the case of cache being corrupt. Created an issue to track this.
| from .osx import KeychainError | ||
| if exp.exit_status != KeychainError.ITEM_NOT_FOUND: | ||
| raise | ||
| else: |
There was a problem hiding this comment.
This implementation does not address similar exceptions from other platforms, does it?
And, I have a feeling that, even after we complete all those platform-dependent logic here, this would not look as good as the #67 approach.
Feel free to explore more, and then we discuss.
There was a problem hiding this comment.
I remember closing the conversation on the fact that we just need to address the Keychain error for now and so for the time being we do it in the token cache layer. Similar errors in other platforms can be addressed by creating a new Exception at the persistence layer later. I remember the reason we do not want to introduce a None return value is because persistence layer is exposed as an API and so introducing a new exception at that layer is also something we possibly need to discuss. So eventually once we have had that conversation we can go ahead and visit #67. @henrik-me Am I missing something ?
Fixes #64
For Keychain:
Scenario:
For Windows:
Added on 8/4/2020: Wiki page summarizing migration scenario from MSAL Python to MSAL Extensions using same file.