[Bug] Updates to msal/application.py may now be out-of-sync with msal_extensions

[kcphila]

Describe the bug

I just upgraded the MSAL Python package and all of our silent authentication routines started breaking - these authenticate manually and cache the access token for future calls. Interactive authentication works and the token cache appears to save the the account and token correctly, but when the repeat call is made with silent it does not find an account and fails.

I have diagnosed the error and traced it back to this commit that changes self.token_cache.find to self.token_cache.search in several places.

It appears that upgraded to a MSAL version at or after this point requires an update of msal_extensions, upgrading from msal_extensions-1.0 .0 to msal_extensions-1.2.0 seems to work correctly, however, it was not obvious that this was the issue because both find and search are valid methods on the msal_extensions 1.0.0 token cache, but find finds the previously cached accounts and search does not.

msal_extensions understandably is not a requirement for the MSAL package, but I would recommend creating some type of link to ensure the versions are in sync when there are breaking changes (especially since the commit history suggests you maintain/contribute to both).

And if that's not doable, perhaps anyone else who runs into this and searches will find this bug report :)

To Reproduce
Steps to reproduce the behavior:

1. pip install msal_extensions==1.0.0
2. pip install --upgrade msal
3. Run a script or test that authenticates interactively and caches the result with an msal_extensions.PersistedTokenCache
4. Run a script or test that authenticates silently and points to the same cache.

Expected behavior
4 should succeed

What you see instead

This fails because {msalapp}.get_accounts() returns nothing due to the search call.

The MSAL Python version you are using

1.31.0

Additional context

Steps above are fixed if (1) is changed to pip install msal_extensions>=1.2.0

[rayluo]

Hi @kcphila , we feel sorry that you run into this known issue.

It appears that upgraded to a MSAL version at or after this point requires an update of msal_extensions, upgrading from msal_extensions-1.0 .0 to msal_extensions-1.2.0 seems to work correctly, however, it was not obvious

You clearly already did your homework and found the solution, which is upgrading msal_extensions to 1.2.0+. Quoted from MSAL Extensions 1.2.0's release note: "Switch to MSAL 1.29+'s TokenCache.search()".

msal_extensions understandably is not a requirement for the MSAL package, but I would recommend creating some type of link to ensure the versions are in sync when there are breaking changes

Yes, that is indeed the tricky part. msal_extensions is not part of msal's dependency, but I suppose we can introduce a new optional dependency such that pip install msal[extensions] will install the proper version of Extensions. I discussed that with @jiasli before, but the decision at that time was to avoid a cyclic reference. But perhaps we shall revisit that decision, @jiasli ? I tried it in my local proof-of-concept and it would work.

[kcphila]

Thanks, @rayluo. I'll leave it to you all. ButI will way in with an opinion :) - the semantics of msal[extensions] does make a lot sense to me and would provide an easy way to keep the versions in sync in the requirements files of tools that use msal. I also build a lot of middle-level infrastructure for our data team, and so I think it would be more conceptually obvious for them than having msal and msal_extensions as listed separate.

[jiasli]

This issue was reported before in #719 and I provided an explanation at #719 (comment).

Personally, I think msal[extensions] is a misuse of the "optional dependencies" mechanism.

According to https://setuptools.pypa.io/en/latest/userguide/dependency_management.html#optional-dependencies, extras_require is used to define optional dependencies in order to provide extra functionalities. In our case, msal-extensions depends on msal, not the otherwise.

The extra functionalities are not provided by msal but injected into msal. That's why applications such as Azure CLI directly use msal-extensions:

https://github.com/Azure/azure-cli/blob/fb805f4bb3167623107c10ec5f34581a838c8a1c/src/azure-cli-core/azure/cli/core/auth/persistence.py#L12-L13

from msal_extensions import (FilePersistenceWithDataProtection, KeychainPersistence, LibsecretPersistence,
                             FilePersistence, PersistedTokenCache, CrossPlatLock)

However, msal[extensions] only means applications can use the extra functionalities provided by msal. It gives no guarantee msal_extensions will be installed.

If we truly want to use msal[extensions], it should have been msal[persisted-cache] and msal should expose something like persist_cache=True so that the underlying caching mechanism is fully covered by msal without exposing msal-extensions.

The real problems come from:

1. msal doesn't adhere to semantic versioning. Renaming a public API find() to seach() deverves a major version bump, that is, to 3.0. This way, pip can detect the incompatiblity.
2. msal-extensions should not override a protected _find() method.

[rayluo]

Thanks for your input, @jiasli ! There are some misunderstandings here that I have to clarify.

1. msal doesn't adhere to semantic versioning. Renaming a public API find() to seach() deverves a major version bump

MSAL takes semantic versioning seriously. The public API TokenCache.find() remains available, even after being deprecated recently. It will remain available until a future MSAL Python 2.0. All downstream applications or SDKs who consume the find() shall remain working. We have received zero regression report from those consumers.

MSAL-Extensions is different. It does not consume MSAL's TokenCache.find(), it injects the persistence behavior into MSAL's TokenCache.find(). Therefore, MSAL-Extensions takes a dependency on "how MSAL Python utilizes the find()", rather than "what output find() produces". Unfortunately, that seems to be a grey area of semantic versioning. We usually do not version a library's implementation detail; we refactor whenever we see fit. It is just that MSAL-Extensions's tight coupling with msal becomes a victim of such a refactoring.

2. msal-extensions should not override a protected _find() method.

msal-extensions did not really override inject a protected _find() method. It was just a temporary workaround shipped as a beta release 1.2.0b1 to buy us more time, and the subsequent stable release 1.2.0 injects MSAL's new public method TokenCache.search() instead.

msal[extensions] only means applications can use the extra functionalities provided by msal. It gives no guarantee msal_extensions will be installed.

In theory, yes, msal[foo] only means applications can use an extra/optional functionality named "foo". But foo can have its own dependencies too, and the msal[foo] is the canonical way to have those dependency installed.

If we truly want to use msal[extensions], ... the underlying caching mechanism is fully covered by msal without exposing msal-extensions.

The consumer just needs to know that the msal[extensions] can provide some new features via this API from msal_extensions import (FilePersistenceWithDataProtection, ...). The consumer does not need to care the implementation detail of whether those new API was implemented inside msal code base or msal-extensions code base (or foo, for that matter).

Last but not least, with regard to our original concern of circular dependency, I stumbled upon this excellent description of "circular dependency of installation is valid".

[jiasli]

Circular dependency will even cause installation issue during releasing.

Consider both msal and msal-extensions are currently at version 1.0 and both need to be bumped to 1.1:

• msal: 1.0 -> 1.1
• msal-extensions: 1.0 -> 1.1

Both require 1.1 of the other library. That is:

• msal 1.1 requires msal-extensions 1.1
• msal-extensions 1.1 requires msal 1.1

If we release msal 1.1 first, msal-extensions 1.1 will not be available, so installing msal[extensions] will fail, and vice versa.

If msal doesn't depend on msal-extensions, at least installing msal won't fail, even though the actual functionality will fail.

---

Another disadvantage of declaring an extensions optional dependency is that it will be difficult to drop it in the future. See the Tips section at https://setuptools.pypa.io/en/latest/userguide/dependency_management.html#optional-dependencies:

Best practice: if a project ends up no longer needing any other packages to support a feature, it should keep an empty requirements list for that feature in its extras_require argument, so that packages depending on that feature don’t break (due to an invalid feature name).

---

Luckily, msal only has one extension (msal-extensions). Azure CLI has hundreds of extensions. It is simply impossible to declare each extension as an optional dependency.