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

Provide a default persistence per OS #87

Closed
mparry opened this issue Apr 14, 2021 · 2 comments · Fixed by #110
Closed

Provide a default persistence per OS #87

mparry opened this issue Apr 14, 2021 · 2 comments · Fixed by #110
Labels
answered question Further information is requested

Comments

@mparry
Copy link
Contributor

mparry commented Apr 14, 2021

In 0.2.0 the simple TokenCache interface was replaced by newer API that now requires the user to write some code along these lines:

def build_persistence(location, fallback_to_plaintext=False):
    """Build a suitable persistence instance based your current OS"""
    if sys.platform.startswith('win'):
        return FilePersistenceWithDataProtection(location)
    if sys.platform.startswith('darwin'):
        return KeychainPersistence(location, "my_service_name", "my_account_name")
    if sys.platform.startswith('linux'):
        try:
            return LibsecretPersistence(
                location,
                schema_name="my_schema_name",
                attributes={"my_attr1": "foo", "my_attr2": "bar"},
                )
        except:  # pylint: disable=bare-except
            if not fallback_to_plaintext:
                raise
            logging.exception("Encryption unavailable. Opting in to plain text.")
    return FilePersistence(location)

cache = PersistedTokenCache(build_persistence(...))

(That's taken from the README, of course.)

Is there a reason why this logic needs to be reproduced in all user code that doesn't have more particular requirements? Or, conversely, are there many situation where this wouldn't be good enough?

If not, couldn't the library provide a get_default_persistence(location) method, that contains something along the lines of the above? I would be happy to raise a PR for this.

@mparry
Copy link
Contributor Author

mparry commented Apr 14, 2021

Side note: we only recently noticed and updated our code to sync with this, since DeprecationWarning doesn't surface by default and you only get something more visible on Linux, where a RuntimeWarning is issued about the unprotected cache you're getting from the legacy TokenCache interface.

@rayluo rayluo added the question Further information is requested label Apr 14, 2021
@rayluo
Copy link
Contributor

rayluo commented Apr 14, 2021

Thanks, @mparry, you raise a good point on the usability. I do not currently have a definite answer. Here is the background behind that change.

  1. Up to MSAL Extensions version 0.1.3, the parameters for macOS's key chain service_name and account_name were indeed predefined with a hardcoded default value. It might not be obvious, but those 2 parameters decide where your encrypted data is actually stored in on macOS. Imagine 2 unrelated apps using MSAL Extensions 0.1.3 on the same macOS machine, their encrypted data would be saved into same logical namespace. That might not feel right, but it was at least functional, because MSAL Extensions 0.1.3 API was TokenCache, which was only used by MSAL's token cache, and all MSAL token cache has compatible data schema.
  2. Since MSAL Extensions 0.2.0, a new generic Persistence layer was introduced, which can be used to store arbitrary data that does not need to have same schema. So, those service_name and account_name become required parameter in the persistence layer, to help preventing unrelated data being stored in same place and overwriting each other.
    At that time we experimented a helper combining those lower level parameters, but that did not actually encapsulate those details, and our friend @chlowell found it difficult to use, so it was changed to the 0.2.0 way you see today.
  3. With the thoughts above, now I suppose it might be possible to derive the service_name, account_name, etc. from the required parameter location?

We are open for PR contribution. Due to the cross-platform nature of this library, all changes need to be tested from all supported platforms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
answered question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants