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

Assertion in bytes in python3 #16

Merged
merged 10 commits into from Jan 25, 2019
Merged

Assertion in bytes in python3 #16

merged 10 commits into from Jan 25, 2019

Conversation

rayluo
Copy link
Collaborator

@rayluo rayluo commented Jan 18, 2019

This PR is a superset of existing PR #11.

The major part is equivalent, but here it is done the otherway around, i.e. b"." in data_in_bytes rather than "." in data_in_bytes.decode(). This way we deal with bytes directly, and save an ad-hoc type conversion. (On a side note, another alternative is to normalize the input into string at the very beginning and keep it, that would bring some handy benefit when later we want to log it. But the major drawback is the underlying requests library is technically expecting bytes (per their documentation), and actually converting strings into bytes under the hood. So normalizing-to-string would be a double performance penalty.) This PR also applies the same fix on acquire_token_by_assertion(), which is used in ADFS federated scenario. UPDATE: Based on the latest conversation in this PR, we head to a new direction by avoiding such tricky if b"." in assertion in the first place.

Lastly, we fine tune the relevant logging behavior.

You can test this PR by:

pip install git+https://github.com/AzureAD/microsoft-authentication-library-for-python.git@assertion-in-bytes-in-python3

@@ -405,7 +405,7 @@ def _acquire_token_by_username_password_federated(
raise RuntimeError(
"RSTR returned unknown token type: %s", wstrust_result.get("type"))
return self.client.obtain_token_by_assertion(
b64encode(wstrust_result["token"]),
urlsafe_b64encode(wstrust_result["token"]).strip(b'='),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, i believe rstrip is enough as = is only used as padding at the end. If correct, you can also update similar code below

Copy link
Collaborator Author

@rayluo rayluo Jan 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This turns out becoming an interesting journey.

Yes rstrip(...) would be enough, and now I use it in the underlying implementation. Thanks for the suggestion!

And then it becomes the last straw that I realize we should not have forced the caller to do such tricky ad-hoc encoding in the first place (even though we documented it). We should have wrapped it inside a helper instead, and do the right behavior by default. Which is what we do now in this updated PR. And, as a byproduct, the grant_type parameter is no longer optional, because we removed that hacky if b"." in assertion thing, which also makes the original issue of this PR goes away.

None of the above boring details matters in the MSAL public API acquire_token_by_username_password(), because this refactoring is just implementation details.

I'll merge this PR soon. Happy refactoring!

@rayluo rayluo merged commit 43999a6 into dev Jan 25, 2019
@rayluo rayluo mentioned this pull request Mar 5, 2019
@rayluo rayluo deleted the assertion-in-bytes-in-python3 branch March 18, 2019 22:04
@rayluo rayluo restored the assertion-in-bytes-in-python3 branch March 18, 2019 22:05
@rayluo rayluo deleted the assertion-in-bytes-in-python3 branch March 18, 2019 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants