search

Azure-Samples / ms-identity-python-webapp

A Python web application calling Microsoft graph that is secured using the Microsoft identity platform
MIT License
298 stars 143 forks source link

Style fixes and Python best practices #91

Closed pamelafox closed 1 year ago

pamelafox commented 1 year ago

This PR updates the identity sample according to our review guidelines. The goal is to ensure that the code is Pythonic and also follows best practices.

To look for issues, I ran automated tools like black, ruff, isort, and pyupgrade, and kept the suggestions that were the most objectively Pythonic.

One significant change is that I introduced a .env file for the environment variables. This is what's done in many similar samples (like this one), and is a much safer practice. I nearly checked my secrets in multiple times when they were in app_config.py, so my team agreed that it made sense to move them into a non-checked in file. That change does require updating the article text, which I've done in this PR.

I will now leave inline comments on other changes to explain the reasoning.

pamelafox commented 1 year ago

@rayluo Could you review?

pamelafox commented 1 year ago

@rayluo Good idea, I've updated the readme.

rayluo commented 1 year ago

Thanks, @pamelafox , this PR is getting better and better. :-)

Are we still planning to modify the step 3 of README_B2C accordingly? I think the instruction of CLIENT_ID and CLIENT_SECRET will need to be changed to mention .env stuff, and we probably want to converge the b2c_tenant setup to the new TENANT_ID setup. Let the other user flow things remain as-is.

You can have the "azd-ify" change come in a different PR, so that we can conclude this one sooner.

pamelafox commented 1 year ago

Yep, I still need to go through the B2C flow. Probably tomorrow morning. Also want to figure out if there's an article on Learn.com that links to that B2C readme.

pamelafox commented 1 year ago

The B2C readme is referenced in these two articles:

https://learn.microsoft.com/en-us/azure/active-directory-b2c/integrate-with-app-code-samples https://learn.microsoft.com/en-us/azure/active-directory-b2c/configure-authentication-sample-python-web-app?tabs=windows

I'll go through the second article now to understand the old setup before I make adjustments.

pamelafox commented 1 year ago

I've updated this PR with b2c related changes and sent a PR for the article: https://github.com/MicrosoftDocs/azure-docs-pr/pull/228856

I've now discovered an article in b2c docs that basically builds up the entire code sample: https://review.learn.microsoft.com/en-us/azure/active-directory-b2c/enable-authentication-python-web-app?branch=pr-en-us-228856&tabs=macos So that should ideally get updated too. There's no user feedback for it, so I wonder how many folks go through the app creation process versus just using the existing app code.

rayluo commented 1 year ago

I've now discovered an article in b2c docs that basically builds up the entire code sample: https://review.learn.microsoft.com/en-us/azure/active-directory-b2c/enable-authentication-python-web-app?branch=pr-en-us-228856&tabs=macos So that should ideally get updated too. There's no user feedback for it, so I wonder how many folks go through the app creation process versus just using the existing app code.

It makes total sense to update/simplify that doc.

I believe that doc was written that way (i.e. "basically builds up the entire code sample") as part of the effort to provide step-by-step guidance. And that approach evolved into standalone chapterized tutorials afterwards. But their ongoing maintenance is underfunded.

Now that this universal sample (which covers sign-in/sign-out, calling api, working in both AAD and B2C and another variation coming soon) is further simplified, perhaps we can simply remove all code snippets in that doc, and ask the readers to clone/download this sample instead.

pamelafox commented 1 year ago

@rayluo I actually met with another advocate who is well versed in Powershell, and we ended up upgrading the Powershell scripts to a much newer module, based on the scripts used in the dotnet version of this sample. I've tested that flow and it seems to all work.

The azure-docs-prs were both approved and merged, so if the Powershell changes are good with you, then feel free to merge!

rayluo commented 1 year ago

I do not use that powershell script myself. I can take a look into your new change in a high level within next couple workdays and then likely merge it in as-is.

ended up upgrading the Powershell scripts to a much newer module, based on the scripts used in the dotnet version of this sample

This "fragmentation of helper scripts" sounds concerning. Will your future "azd-ify" PR address it?

pamelafox commented 1 year ago

Yeah, I also don't use Powershell, that's why I asked another advocate to check my work. Here's the dotnet sample which the code is mostly from: https://github.com/Azure-Samples/active-directory-aspnetcore-webapp-openidconnect-v2/blob/master/1-WebApp-OIDC/1-3-AnyOrgOrPersonal/AppCreationScripts/Configure.ps1 I found that sample from the link in this article: https://learn.microsoft.com/en-us/azure/active-directory/develop/scenario-web-app-sign-user-app-registration?tabs=aspnetcore#register-an-app-by-using-powershell

Good question re AZD-ifying. In that case, if the dotnet sample was also going to support azd, they would need similar Bicep code. So I think there'd be redundancy in both cases. With Bicep though, it's possible to store useful modules in a shared registry, so that's what we're hoping to do to avoid redundancy across similar samples.