Introduce a parameter AuthenticationContext(..., api_version='1.0')

[rayluo]

In an attempt to fix #54 yet maintain backward compatibility, this PR introduces a parameter AuthenticationContext(..., api_version='1.0') to turn on/off api-version behavior.

• Legacy ADAL-python customers can safely upgrade to the upcoming new version which contains this adjustment. By default, your current code will still work with the SAME behavior as before.
• However, to ensure a smooth transition, we recommend the current ADAL-python customers to try out the new behavior by explicitly set context = AuthenticationContext(..., api_version=None), to confirm that your app will work fine with this new direction.
• New ADAL-python customers shall use context = AuthenticationContext(..., api_version=None) too. In future, this switch will have a default value as None.

[yugangw-msft]

LGTM. Left one comment for clarification. Feel free to merge after that

[yugangw-msft]

should we verify there is no warning emitted?

[rayluo]

We don't normally verify "no warning emitted" but in this particular case, sure I can do that. New commit is coming soon.

[yugangw-msft]

My understanding is we will just remove this parameter in future, as adal is not a protocol library. If yes, let us change warning words accordingly such as
Future version of ADAL will remove this parameter and use more recent and better version

[rayluo]

Yeah that is the tricky part which is not yet set on stone.

When we choose to maintain the backward compatibility of previous api-version=1.0 behavior by default, from now on the majority of current customers and new customers (those whom got bitten by issue #54 and/or whom be persuaded by the warning message in this PR), would then have to do AuthenticationContext(..., api_version=None) explicitly. And this situation will last for quite a while until the next major version bump on ADAL. So lots of customers will be on the api_version=None side. By the time, we will have 2 options moving forward:

1. We may choose to completely remove the api_version=... parameter with a major version bump. That would be a HARD breaking change, because all customers who follow our advice this time would then have to change their code again to remove the api_version=None.
2. Or we may choose to keep the api_version=... parameter in the API signature, we just will not use it anymore, i.e. it will become a NO-OP. That way it will still be a subtle breaking change (and also shipped with a major version bump), but current AuthenticationContext(..., api_version=None) customers would not need to change their code.

All these sound more complicated than simply remove api_version completely and immediately, but this is the price to pay for the backward compatibility.

[yugangw-msft]

@rayluo, it is your call. I am fine with either way

[RandalliLama]

I don't think that any developers are going to understand that warning. Client developers don't care about the protocol at all, and also don't have visibility in to the claims in the token. They may know nothing about the effect of the api-version QP on the claims in the token and how that might break a service. I don't think that I would put that warning in.

[rayluo]

I have to admit that there is no way to convey the complicated history into this several sentences long warning message. :-( Regardless of the wording, the main purpose of such warning message was trying to say "hey api_version=None is the future, you'd better opt-in (and then test out the new behaviors) now, rather than facing a sudden change in a future major version update".

[yugangw-msft]

My 2 cents
@rayluo, can we consider to downgrading the level to an info, not a warn? My concern is the warning by default will spit out everywhere to end users even for working applications which don't directly uses adal, but get pulled in by its dependencies. I hope such change should have zero end users impacts for most client applications.
At the same time, we probably should update the method's doc-string to describe this parameter, so at least developers coding with adal will know the impact and do the recommended way

[rayluo]

That makes sense. And it seems an easy fix at this point would be changing the warning type to DeprecationWarning, which will then be ignored by default, I've tested it by running python -c "import adal; adal.AuthenticationContext('https://login.windows.net/tenant')" on both Python 2 and Python 3.

I've also add docstring for the new parameter. Please review the latest commit

[rayluo]

Note: we will need a little more time to investigate the current test failure on Travis-CI. It is not a major issue though, it is just that the warning message is not showing up on Travis-CI's test environment which is Python 2.7.9 running on Ubuntu 12.04. FWIW, I've manually tested this PR on Python 2.7.12 & Python 3.5.2 on Windows, and Python 2.7.10 on OSX, they all work. UPDATE: I also test on Python 2.7.9 on Ubuntu 16.04, it still works. So Python 2.7.9 is not the culprit.

[yugangw-msft]

@rayluo, maybe you can just use the built-in logging in adal and wire up your test handler. I am not familiar with warnings package, but handler works for my test added recently

[rayluo]

@yugangw-msft Besides their subtle conceptual difference:

warnings.warn() in library code if the issue is avoidable and the client application should be modified to eliminate the warning

logging.warning() if there is nothing the client application can do about the situation, but the event should still be noted

One noticeable difference is for i in range(10): warnings.warn("foo") will only print out one output by default. And my guess is, this exact smart feature somehow causes the test counter not counting properly (on Travis-CI for unknown reason), hence the test failure.

Using logging instead, or using warnings with its "always warning" mode, would pass that test case, but it results in tons of duplicated warning messages showing up on the console when running our unit tests.

In this particular case, I guess I would simply relax that assertion to make the test cases pass. After all, it is just some optional warning message, for a behavior which will eventually be replaced by next major version update.

[yugangw-msft]

🚢 Thanks @rayluo !