Skip to content
This repository has been archived by the owner on Nov 22, 2018. It is now read-only.

Inject ICorsPolicyProvider instance through Invoke #106

Closed
wants to merge 1 commit into from
Closed

Inject ICorsPolicyProvider instance through Invoke #106

wants to merge 1 commit into from

Conversation

henkmollema
Copy link
Contributor

This allows for scoped instances of ICorsPolicyProvider to be injected in the CORS middleware and prevents turning them into singletons.

Resolves #105

@henkmollema
Copy link
Contributor Author

/cc @brockallen @davidfowl

@henkmollema
Copy link
Contributor Author

@Eilon are we going forward with this PR? And if so, do you want me to update (rebase) it?

{
if (context.Request.Headers.ContainsKey(CorsConstants.Origin))
{
var corsPolicy = _policy ?? await _corsPolicyProvider?.GetPolicyAsync(context, _corsPolicyName);
var corsPolicy = _policy ?? await corsPolicyProvider?.GetPolicyAsync(context, _corsPolicyName);
Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure this can't be null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. Invoking middleware will fail if it can't resolve the parameters, right? I'll remove the ?.

This allows for scoped instances of `ICorsPolicyProvider` to be injected in the CORS middleware and prevents turning them into singletons.

Resolves #105
@henkmollema
Copy link
Contributor Author

@davidfowl @Eilon updated and rebased.

@henkmollema
Copy link
Contributor Author

We need to update baseline.json as well.

@Eilon
Copy link
Member

Eilon commented Sep 6, 2017

BTW per the discussion in #105, this is a breaking change, so it can't be taken as-is in the 2.1 release, so this PR is "on hold" until we figure out how to resolve that.

@aspnet-hello
Copy link

This issue was moved to dotnet/aspnetcore#2376

@aspnet aspnet locked and limited conversation to collaborators Jan 1, 2018
@aspnet-hello
Copy link

Sorry I was a bad bot. Re-opening this PR.

@aspnet-hello aspnet-hello reopened this Jan 2, 2018
@aspnet aspnet unlocked this conversation Jan 2, 2018
@Eilon
Copy link
Member

Eilon commented Apr 9, 2018

@mkArtakMSFT - any further action planned on this PR?

I think that given it's a breaking change, I recommend we close it.

@pranavkm
Copy link
Contributor

@Eilon close?

@brockallen
Copy link

Why close? It needs to be fixed at some point.

@pranavkm
Copy link
Contributor

I was going by the most recent comment here:

I think that given it's a breaking change, I recommend we close it.

It's likely a breaking change would necessitate waiting until 3.0.

@brockallen
Copy link

It's likely a breaking change would necessitate waiting until 3.0.

Sure. Don't you have a way of assigning/labeling the PR for 3.0?

@pranavkm
Copy link
Contributor

It's usually tricky to keep a PR up-to-date (merge conflicts etc) for that long. Much easier to file a tracking issue and fix it in the release.

@natemcmaster natemcmaster changed the base branch from dev to master July 2, 2018 17:57
@mkArtakMSFT
Copy link
Member

Resolving the moved issue: dotnet/aspnetcore#2377

@mkArtakMSFT
Copy link
Member

@henkmollema, we do want to take this change in, and we've just marked the referenced issue accordingly. However, this PR is outdated already and given that we aren't going to act on it yet, you can close it if you want and we'll do the change when the right time will come.

@pranavkm
Copy link
Contributor

pranavkm commented Nov 9, 2018

Tracking merging this in #202

@pranavkm pranavkm closed this Nov 9, 2018
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.

None yet

9 participants