search

Azure-Samples / ms-identity-python-on-behalf-of

This sample demonstrates a Python Django web application calling a Python Flask web API that then calls the Azure Management API subscriptions endpoint. The web application and API are secured using Azure Active Directory.
MIT License
27 stars 18 forks source link

Review changes #1

Closed idg-sam closed 3 years ago

idg-sam commented 3 years ago

Purpose

Does this introduce a breaking change?

[ ] Yes
[ ] No

Pull Request Type

What kind of change does this Pull Request introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[ ] Other... Please describe:

How to Test

git clone [repo-address]
cd [repo-name]
git checkout [branch-name]
npm install

What to Check

Verify that the following are valid

Other Information

idg-sam commented 3 years ago

Code works great! This is a great sample @niswitze , very much appreciate the sample and the great work you've put into it!

Some minor feedback:

  1. tweak the README as per the generator (and include script generator - confgure/cleanup scripts). Dogan has provided a copy of the code generator's on behalf of readme which I've uploaded here.

  2. switch to HTTP for local development settings (including removing secure cookie session from local development settings in Django) and run both apps on http://localhost.

  3. do we need the audience environment variable in the flask API side? i.e., we can just use the app's clientID since the values are the same?

niswitze commented 3 years ago

Code works great! This is a great sample @niswitze , very much appreciate the sample and the great work you've put into it!

Some minor feedback:

  1. tweak the README as per the generator (and include script generator - confgure/cleanup scripts). Dogan has provided a copy of the code generator's on behalf of readme which I've uploaded here.
  2. switch to HTTP for local development settings (including removing secure cookie session from local development settings in Django) and run both apps on http://localhost.
  3. do we need the audience environment variable in the flask API side? i.e., we can just use the app's clientID since the values are the same?

@idg-sam thanks for reviewing this. I've updated the sample to reflect changes 2 and 3. For change 1, I've update the sample.json and am currently updating the steps.md. The last sections I need to fill out for it are About the code and Deployment.

During the review, I came up with two questions I hoped you could help me with.

  1. Do I just create a topology overview png using Visio or is there a standard generator?
  2. Does the steps.md file need to be the main readme.md that is displayed when a user visits the GH repo? If so, do you normally just copy the markdown over to the main readme.md or just reference it?
idg-sam commented 3 years ago

Mentioning @rayluo since I can't add him as a reviewer from the menu for some reason.

idg-sam commented 3 years ago

Also mentioning @navyasric as I can't add her from the regular "request reviewer" menu

idg-sam commented 3 years ago

Code works great! This is a great sample @niswitze , very much appreciate the sample and the great work you've put into it! Some minor feedback:

  1. tweak the README as per the generator (and include script generator - confgure/cleanup scripts). Dogan has provided a copy of the code generator's on behalf of readme which I've uploaded here.
  2. switch to HTTP for local development settings (including removing secure cookie session from local development settings in Django) and run both apps on http://localhost.
  3. do we need the audience environment variable in the flask API side? i.e., we can just use the app's clientID since the values are the same?

@idg-sam thanks for reviewing this. I've updated the sample to reflect changes 2 and 3. For change 1, I've update the sample.json and am currently updating the steps.md. The last sections I need to fill out for it are About the code and Deployment.

During the review, I came up with two questions I hoped you could help me with.

  1. Do I just create a topology overview png using Visio or is there a standard generator?
  2. Does the steps.md file need to be the main readme.md that is displayed when a user visits the GH repo? If so, do you normally just copy the markdown over to the main readme.md or just reference it?
  1. There is a visio file I can send to you. Or we get a PNG from an existing sample and re-label it.
  2. Just the readme.md will suffice
kalyankrishna1 commented 3 years ago

For the Flask API application, use the following command:

in the 'FlaskAPI' sub-folder, use the...

Refers to: README.md:81 in e6bb86f. [](commit_id = e6bb86f6901cc77b510d6c901d1d686ce798a544, deletion_comment = False)

kalyankrishna1 commented 3 years ago

For the Django UI application, local execution only, use the following command:

in the 'DjangoUI' sub-folder, use the...

Refers to: README.md:86 in e6bb86f. [](commit_id = e6bb86f6901cc77b510d6c901d1d686ce798a544, deletion_comment = False)

kalyankrishna1 commented 3 years ago

API_SCOPE="api://flaskapi/Subscriptions.Read"

Add AppIdURI= as a parm and this will allow you to remove API_SCOPE and SCOPE as you can construct those URls in code

Refers to: DjangoUI/production.env:24 in e6bb86f. [](commit_id = e6bb86f6901cc77b510d6c901d1d686ce798a544, deletion_comment = False)

idg-sam commented 3 years ago

Todo checklist:

  1. update the sample.json
  2. confirm run instructions from command line
  3. what were the different configurations that lead to different results?
niswitze commented 3 years ago

@kalyankrishna1 @idg-sam Just pushed the following updates based on the comments in the PR. Let me know if I missed or mis-interpreted any updates:

1. Added more links for using virtual environments in Python
2. Added steps for creating both development.env files
3. Updated readme based on PR comments from Kalyan 
4. Removed API from 'Microsoft Graph API' in sample.json
5. Updated in Django UI for SCOPE and readme - should be api and not https
6. Updated JWT access token to JWT Access Token