-
Notifications
You must be signed in to change notification settings - Fork 54
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
Add LoginMode property in OktaMvcOptions #89
Conversation
… for self-hosted login apps.
Okta.AspNet/LoginMode.cs
Outdated
namespace Okta.AspNet | ||
{ | ||
/// <summary> | ||
/// LoginMode controls the behavior of the middleware. |
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.
I would elaborate here, maybe something like "LoginMode controls the login redirect behavior of the middleware"
Okta.AspNet/OktaMvcOptions.cs
Outdated
@@ -44,6 +44,12 @@ public class OktaMvcOptions : Abstractions.OktaWebOptions | |||
/// <value>The scope.</value> | |||
public IList<string> Scope { get; set; } = OktaDefaults.Scope; | |||
|
|||
/// <summary> | |||
/// Gets or sets the <see cref="LoginMode"/>. |
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.
I would elaborate here, maybe something like "Controls the login behavior to redirect to internal or Okta-hosted login"
So can this LoginMode property be set in the normal Web.config way too? Have you tested to make sure the enum is parsed properly? Maybe the docs need to be updated to list this as well? |
@OkGoDoIt Yes, you can set this property in the |
I would personally prefer to update the method summary in the second location I commented on above. And are you absolutely certain the web config is parsed correctly for the enum? The link you posted makes it look like you have to do it manually, but I don’t see any code on our end to handle that. |
Yeah, we don't have any code to handle that. The way to do that in this SDK is via |
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.
Looks good, thanks for the updates
Fixes #77
This property is only available on ASP.NET since ASP.NET Core already has a workaround: okta/samples-aspnetcore#27.