-
Notifications
You must be signed in to change notification settings - Fork 142
Scheme Names #979
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
Scheme Names #979
Conversation
|
Also do we want to include the 2FA scheme from ASP.NET Identity? |
wcabus
left a comment
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.
LGTM!
| | Feature | Standalone IdentityServer | With ASP.NET Identity | | ||
| | :----------------------- | :---------------------------------------------------------------------------------------- | :------------------------------------------------------------------- | | ||
| | **Main Auth Cookie** | `"idsrv"`<br/>(`IdentityServerConstants.DefaultCookieAuthenticationScheme`) | `"Identity.Application"`<br/>(`IdentityConstants.ApplicationScheme`) | | ||
| | **External Auth Cookie** | `"idsrv.external"`<br/>(`IdentityServerConstants.ExternalCookieAuthenticationScheme`) | `"Identity.External"`<br/>(`IdentityConstants.ExternalScheme`) | |
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.
See above.
|
@maartenba You're too fast on the merge button... please see review comments. Also I want to bring up another idea I had in the past: In the IdentityServer AddIdentity method I think we should tap into the scheme collection and remove the idsrv and Identity.External cookies. With the current code, those non-used scheme are still configured. So if anyone incorrectly uses those scheme names when setting configuration it will not fail, because the schemes exist. If we removed them, any incorrect code would instead fail. |
|
Updating as we speak! |
|
@AndersAbel fix PR -> #981 |
I know it's been a while since you logged #367, but is this what you were thinking of @AndersAbel?
Not entirely sure about location in the docs hierarchy but this seemed most logical (but not fully).
Fixes #367