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.

Sign up for GitHub

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

Jump to bottom

Added ability to specify custom cookie options. Minor comment updates. #257

Merged
merged 1 commit into from
Jun 29, 2020
Merged

Added ability to specify custom cookie options. Minor comment updates. #257

merged 1 commit into from
Jun 29, 2020

Conversation

pmaytak
Copy link
Contributor

@pmaytak pmaytak commented Jun 25, 2020 •
edited
Loading

Issue #237.

  • Added the ability to specify custom cookie options.
  • Added net5.0 and netcoreapp3.1 conditionals to test project.
  • Updated the file path to XML docs in the project files.
  • Minor comment fixes.

jennyf19
jennyf19 approved these changes Jun 25, 2020
View reviewed changes
Copy link
Collaborator

@jennyf19 jennyf19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are there any tests to be modified for this?
is it possible to do separate PRs for different issues (ex. moving the spelling fixes into a different PR in the future, makes it easier to review the current PR)?

@pmaytak pmaytak force-pushed the pmaytak/customCookieOptions branch 2 times, most recently from b432033 to 965ac56 Compare June 25, 2020 07:02
@pmaytak
Copy link
Contributor Author

pmaytak commented Jun 25, 2020

@jennyf19

  • I updated a test here. It only works for Core 3.1 though. In .NET 5, there seems to be no easy way to test. The internals of AddCookies method are changed to add Action<CookieAuthenticationOptions, IServiceProvider> rather than Action<CookieAuthenticationOptions>.
  • Yea, if there are a lot or unrelated comment fixes, I'll move them to a separate PR.

jmprieur
jmprieur reviewed Jun 25, 2020
View reviewed changes
Copy link
Collaborator

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few comments

tests/Microsoft.Identity.Web.Test/Microsoft.Identity.Web.Test.csproj Show resolved Hide resolved
tests/Microsoft.Identity.Web.Test/WebAppExtensionsTests.cs

var provider = services.BuildServiceProvider();

// Assert configure options actions added correctly
var configuredOidcOptions = provider.GetServices<IConfigureOptions<OpenIdConnectOptions>>().Cast<ConfigureNamedOptions<OpenIdConnectOptions>>();
var configuredMsOptions = provider.GetServices<IConfigureOptions<MicrosoftIdentityOptions>>().Cast<ConfigureNamedOptions<MicrosoftIdentityOptions>>();

#if DOTNET_CORE_31
var configuredCookieOptions = provider.GetServices<IConfigureOptions<CookieAuthenticationOptions>>().Cast<ConfigureNamedOptions<CookieAuthenticationOptions>>();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you try to use IConfigureNamedOptions (interface) instead of an implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, but the service is of type IConfigureOptions and interfaces don't have the Action property.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you see this, as an example? I would prefer to not have flags for .net 3.1/net 5 is possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, yea, I could just try to resolve the service for IConfigureOptions<CookieAuthenticationOptions> but that will only tell us that some cookie options were added and not whether our custom cookie action was added.

We might still need flags in general, like for #234 fix.

@henrik-me
Copy link
Contributor 

    private Action<MicrosoftIdentityOptions> _configureMsOptions = (options) =>

unrelated to the change: this one is modified in a number of test cases, not sure how well this works with parallel test execution. Each test which needs something different should probably create a new options var.

this was the only one I spotted, but glancing over the top of this file there might be more... thinking there should be an effort to explore parallel test execution (easy to enable in VS) issues. Not very important if things generally works and test execution is acceptable.

Refers to: tests/Microsoft.Identity.Web.Test/WebAppExtensionsTests.cs:42 in 965ac56. [](commit_id = 965ac56, deletion_comment = False)

@pmaytak
Copy link
Contributor Author

pmaytak commented Jun 25, 2020

@henrik-me
This works for xUnit because it makes a new instance of test class for to run each test method. (NUnit, on the other hand, creates one test class instance and runs all the methods in it.) By default, xUnit tests already run parallel.

@pmaytak pmaytak force-pushed the pmaytak/customCookieOptions branch 2 times, most recently from f2afc9d to de614bd Compare June 29, 2020 23:31
@pmaytak pmaytak force-pushed the pmaytak/customCookieOptions branch from ba57f3c to 0ab9e84 Compare June 29, 2020 23:52
@pmaytak pmaytak merged commit bdbe3f3 into master Jun 29, 2020
@pmaytak pmaytak deleted the pmaytak/customCookieOptions branch June 29, 2020 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jennyf19 jennyf19 jennyf19 approved these changes

@jmprieur jmprieur jmprieur left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pmaytak @henrik-me @jmprieur @jennyf19