Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ROPC: include for secure auth flow - managed identities - Razor Pages Tutorial #33408
ROPC: include for secure auth flow - managed identities - Razor Pages Tutorial #33408
Changes from 10 commits
65d437c
2d00d71
6e5f0e0
0c4861d
4cb8d16
ed703b8
6474efa
c29f8e1
a4fe031
69c4b8e
f77da82
c37030d
68aa6a5
423d15f
2fde64b
2f64a30
b764cfb
a3acf6e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tdykstra I think that's easier to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tdykstra I think we should put the above in an include file and use it everywhere. Then we only have one place to change it. What do you think?
We'll need this boilerplate in MVC and other place. We could add it to EF tutorials.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wadepickett @tdykstra we need to work in something like this.
For VS/local db:
In this tutorial, localdb is used in dev on the local machine and doesn't use a password, therefore, a secure authentication flow is not required.
Then something similar for SqlLite
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to spook them right away so we need to tell them up front that since this tutorial uses a local db that doesn't use a password, a secure authentication flow isn't required. Maybe tell them that before the big warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Rick-Anderson, sure we can create another include for that case too only for VS/localDB
Clarification/question: For VS/localDB, when Trusted_Connection=True as we are using in the connect here, that results in Windows Authentication used. Wouldn't we consider that a type of Authentication flow? So to say one isn't required might be confusing. It may not be the best one for the situation depending, but is one. Maybe we are keeping to a narrower definition of Authentication flow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resource Owner Password Credentials (ROPC) grant is using a PW. No PW is used with local DB. We're only concerned where a PW is used, like the old deployment method where we put the PW in an env variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and I tweaked it a bit.
Sounds good, but I don't think we should say "don't use environment variables in production" without saying what is OK to use and providing a link to how-to instructions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Rick-Anderson, I think the way you worded it in the last edit already fits both situations nicely by specifying "a local database" rather than more specifically "localDB".
So only the one include would be needed for the note on using a local db. I will just add one include with your text, unless you still want some sort of separate message for SQLite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tdykstra, to address this I added "An environment variable should not be used in a production system. For a production system, use a secure method such as Azure Key Vault configuration provider or Managed Identities for Azure resources to manage sensitive information like connection strings."
See the updated include. If they are not using Azure SQL server, then it would have to be key vault I assume, if they are then it would be Managed Identities for Azure resources.
However, between both includes it feels a little duplicated so maybe I need to address it differently in a combined way. See what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, too long, too much duplication.