Skip to content
This repository was archived by the owner on Dec 13, 2018. It is now read-only.

AddXyz() binds to default config section #1180

Closed
wants to merge 3 commits into from
Closed

Conversation

HaoK
Copy link
Member

@HaoK HaoK commented Apr 14, 2017

I didn't add these overloads for Cookies/generic OAuth since those are more likely to have multiple instances, but I can if we want.

cc @glennc @ajcvickers @Tratcher @divega @davidfowl

@dnfclas
Copy link

dnfclas commented Apr 14, 2017

@HaoK,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@HaoK
Copy link
Member Author

HaoK commented Apr 14, 2017

@Tratcher I'm going to merge this into the mega security changes PR

@HaoK
Copy link
Member Author

HaoK commented Apr 14, 2017

I'll keep this PR open to keep it obvious what the changes specifically for the AddXyz overloads were

@kevinchalet
Copy link
Contributor

@HaoK so these overloads require (manually) registering an IConfiguration instance in the DI container, right? Wouldn't passing the IConfiguration as an argument be clearer? Do you know how well the config binder will work with complex types like the token validation parameters class used by the JWT middleware?

Curious: do you plan to adopt this design in other ASP.NET Core components?

@HaoK
Copy link
Member Author

HaoK commented Apr 14, 2017

Yeah so hosting is adding configuration to DI now, so there's the concept of a default configuration. As a result we can add a default AddXyz() overloads to anything where it makes sense to auto bind to a default config section in that configuration.

See aspnet/Hosting@0ab882b

@HaoK
Copy link
Member Author

HaoK commented Apr 14, 2017

RE binding complex arguments, I'm sure it will throw or fail. We likely will need to give the binder lots more love as a result of lighting things up this way...

@Tratcher
Copy link
Member

What would the config need to look like for each provider? How much can realistically be bound?

@Tratcher
Copy link
Member

I see the minimum binding in the tests. Can you add to the tests all properties you can currently bind to?

@HaoK
Copy link
Member Author

HaoK commented Apr 17, 2017

Added the config binding to the core AddXyz, so its always added by default, with the Action overriding.

Added all the properties that can be set to the test. The one we might want to fix is the ability to bind to PathString which currently doesn't work with the binder.

Merging this to the main PR, if there are any other concerns we can deal with them there

@HaoK
Copy link
Member Author

HaoK commented Apr 17, 2017

Rolled into #1170

@HaoK HaoK closed this Apr 17, 2017
@HaoK HaoK deleted the haok/addXyz branch August 7, 2017 16:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants