Skip to content
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

Merged
merged 4 commits into from
Jul 19, 2019

Conversation

laura-rodriguez
Copy link
Collaborator

Fixes #77

This property is only available on ASP.NET since ASP.NET Core already has a workaround: okta/samples-aspnetcore#27.

namespace Okta.AspNet
{
/// <summary>
/// LoginMode controls the behavior of the middleware.
Copy link
Contributor

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"

@@ -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"/>.
Copy link
Contributor

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"

@OkGoDoIt
Copy link
Contributor

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?

@laura-rodriguez
Copy link
Collaborator Author

@OkGoDoIt Yes, you can set this property in the Web.Config as in this example:https://stackoverflow.com/questions/8148622/get-enum-values-from-web-config-at-run-time.
I don't think we need to test if this is parsed correctly since that's not the responsibility of the middleware.
Docs definitely need to be updated, but I wanted to hear the team's thoughts first.

@laura-rodriguez laura-rodriguez requested a review from bdemers July 18, 2019 13:45
@laura-rodriguez laura-rodriguez requested a review from OkGoDoIt July 18, 2019 16:10
@OkGoDoIt
Copy link
Contributor

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.

@laura-rodriguez
Copy link
Collaborator Author

laura-rodriguez commented Jul 18, 2019

Yeah, we don't have any code to handle that. The way to do that in this SDK is via Configuration object.

Copy link
Contributor

@OkGoDoIt OkGoDoIt left a 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

@laura-rodriguez laura-rodriguez merged commit 096b337 into master Jul 19, 2019
@laura-rodriguez laura-rodriguez deleted the lr-fix-self-hosted-issue branch July 19, 2019 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Self-hosted samples redirect to Okta instead of the self-hosted login
2 participants