Updated Azure to allow defining authorization_url_params

[gchq83514]

I hope this is all okay. I have updated the Azure provider to allow someone to define authorization_url_params to pass additional data.

Azure supports extra parameters such as prompt, login_hint and domain_hint

https://docs.microsoft.com/en-us/azure/active-directory/develop/v1-protocols-oauth-code#request-an-authorization-code

[codecov]

Codecov Report

Merging #288 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #288      +/-   ##
==========================================
+ Coverage   96.63%   96.66%   +0.02%     
==========================================
  Files          28       28              
  Lines         922      929       +7     
==========================================
+ Hits          891      898       +7     
  Misses         31       31

Impacted Files	Coverage Δ
flask_dance/contrib/azure.py	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f42014...509fa6c. Read the comment docs.

[singingwolfboy]

This is great! Thank you so much for doing this!

However, it sounds like your goal is being able to set prompt, login_hint and domain_hint. Why not add those to the argument list for make_azure_blueprint(), instead of making users understand where they need to set these values in the authorization_url_params?

For example, the Google blueprint does this for the reprompt_consent, reprompt_select_account, and hosted_domain arguments: https://github.com/singingwolfboy/flask-dance/blob/master/flask_dance/contrib/google.py#L86-L94

Does that seem closer to what you want? Or is there a reason why you want to allow users to set authorization_url_params dynamically?

[singingwolfboy]

Also, thanks for fixing the documentation tests! I've cherry-picked that commit onto the master branch, so you can remove it from this pull request. 😄

[gchq83514]

This is great! Thank you so much for doing this!

However, it sounds like your goal is being able to set prompt, login_hint and domain_hint. Why not add those to the argument list for make_azure_blueprint(), instead of making users understand where they need to set these values in the authorization_url_params?

For example, the Google blueprint does this for the reprompt_consent, reprompt_select_account, and hosted_domain arguments: https://github.com/singingwolfboy/flask-dance/blob/master/flask_dance/contrib/google.py#L86-L94

Does that seem closer to what you want? Or is there a reason why you want to allow users to set authorization_url_params dynamically?

No problem!

The only argument I really need is prompt but thought I would include the others for peoples information.

I will add them to the argument list as you suggest.

Would you like me to do it under a new PR or keep this one open?

Thanks

[singingwolfboy]

You can rebase this pull request and make the changes here, or close this one and open a new one. Whichever you prefer -- it makes no difference to me!

[singingwolfboy]

Here's a handy guide for how to rebase, if you're not familiar with it: https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request

[gchq83514]

Here's a handy guide for how to rebase, if you're not familiar with it: https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request

Thanks for the link, I have rebased and will work on the aforementioned changes

[gchq83514]

All tests pass.

Hopefully this is okay. I couldn't implement exactly like the Google one using bools, as Azure SSO only allows one prompt not multiple.

Thanks

[singingwolfboy]

Fantastic! However, there's one other change you need to make: please move these new function arguments to the end of the list, after the "tenant" argument. If people are calling make_azure_blueprint() with positional arguments, this change in argument ordering could cause backwards incompatibilities. For example, if someone calls make_azure_blueprint("my-id", "my-secret", "scope1,scope2,scope3"), they would expect the third argument to be treated as scopes -- but right now, it would suddenly become domain_hint!

I think that once you make this change (and reorder the documentation for these arguments to match) this will be ready to merge!

[gchq83514]

Hello,

Very valid point, I forgot people might not specify the param name when calling the function!

Hopefully this is now complete. Thanks

[singingwolfboy]

Fantastic! This is now merged, and released with Flask-Dance 3.0! 😄

[gchq83514]

Awesome, thank you! 😊