Making all platform-dependent parameters optional

[Ray Luo (rayluo)]

Changes in this PR

Az CLI uses 2 instances of persistence for different data, stored in different files. Jiashuo Li (@jiasli) found it unobvious that he would need to use different service_name and account_name when running on mac. This PR attempts to hide/handle this detail, by automatically generate those 2 parameters based on the (signal) file location.

Future ideas that are not (yet?) in this PR

1. Historically, before we introduced all those platform-dependent parameters, there was a unified TokenCache interface across all platforms.

2. Later when we added (more and more) platform-dependent parameters, we attempted to keep a similar cross-platform build_persistence() helper but it did not really hide all those platform-dependent details, it merely becomes a union of all parameters.

   def build_persistence(
       ...,
        entropy='',  # For Windows
        service_name=None, account_name=None,  # For OSX
        schema_name="", attributes=None, fallback_to_plaintext=False,  # For Linux
        **kwargs):  # Extra parameters will be relayed to LibsecretPersistence

   Therefore, Charles Lowell (@chlowell) convinced us to remove that helper, and rightfully so:

   I feel the helper doesn't really help me because it doesn't hide the particulars of persistence. To use it, I must learn which options apply on which platforms, and how; which ones are truly optional. Then I either pass all options at once or call the helper conditionally by platform. I prefer to write the conditional code because it would be easier for future generations to understand. At that point I may as well just import and configure persistence classes directly, that's nicely explicit.

3. Now, if this PR would be welcomed and merged, the next step would be reconsidering the introduction of a unified helper. This time, it won't need any platform-dependent parameters. Perhaps even a unified Persistence alias:

   # in __init__.py
   if sys.platform.startswith('win'):
       from .persistence import FilePersistenceWithDataProtection as EncryptedPesistence
   elif sys.platform.startswith('darwin'):
       from .persistence import KeyChainPersistnce as EncryptedPesistence
   else:
       from .persistence import LibsecretPersistence as EncryptedPesistence

   This way, the EncryptedPersistence would still be different classes with different signatures on different platforms, but at least their required parameters would be unified so that you could initialize one as EncryptedPersistence('/path/to/my/file.bin'). And that would help resolve https://github.com/AzureAD/microsoft-authentication-extensions-for-python/issues/87.

Charles Lowell (@chlowell) , Jiashuo Li (@jiasli), thoughts?

UPDATE at 12/7/2021: We decided to reject proposal 3, because that alias would end up with different shapes. We postpone proposal 2 to a future release, after we gain some real-world usage with this PR.

[Jiashuo Li (jiasli)]

Personally, I won't really fancy the idea of unifying everything to EncryptedPesistence if their constructor signatures are different. If I need to use platform-specific features, such as service_name, account_name for MacOS, I will need additional if...then... logic, which defeats the purpose of unified interface.

The original build_persistence solution sounds better to me.

[Jiashuo Li (jiasli)]

Related email discussion: Naming for service_name and account_name for MacOS token encryption

[Ray Luo (rayluo)]

Personally, I won't really fancy the idea of unifying everything to EncryptedPesistence if their constructor signatures are different. If I need to use platform-specific features, such as service_name, account_name for MacOS, I will need additional if...then... logic, which defeats the purpose of unified interface.

The original build_persistence solution sounds better to me.

Jiashuo Li (@jiasli) , did you mean to say that we can now promote this updated sample's helper def build_persistence(location, fallback_to_plaintext=False) into a formal API?

[Jiashuo Li (jiasli)]

Not sure if MSAL should comply with azure-identity which is actually a downstream library of MSAL.

[Ray Luo (rayluo)]

I understand that name "Microsoft.Developer.IndentityService" is defined in downstream "azure-identity". Originally, I thought we could use any name so we might as well just backport the name used by "azure-identity".

But you raised a good point. We should NOT knowingly use any downstream specific service name here, because this open-source library msal-extensions could potentially be used by app developers to store any information, and we/they probably do not want to store those data inside a specific downstream component's namespece. I'll send a new commit to use "msal-extensions" as the default service name.

[Jiashuo Li (jiasli)]

Jiashuo Li (@jiasli) , did you mean to say that we can now promote this updated sample's helper def build_persistence(location, fallback_to_plaintext=False) into a formal API?

I have no strong opinion. Actually, Azure CLI would prefer not to use this build_persistence, as we tend to use azure-cli as the service_name, which is indeed some customization build_persistence can't provide.

[Jiashuo Li (jiasli)]

Tested with Azure CLI in Azure/azure-cli#20636 and it works perfectly.