Improve query on cors origins.

[Jonatthu]

IdentityServer4/src/EntityFramework.Storage/src/Services/CorsPolicyService.cs

Line 48 in 44651be

var origins = dbContext.Clients.SelectMany(x => x.AllowedCorsOrigins.Select(y => y.Origin)).AsNoTracking().ToList();

Why are we bringing all the client cors origins into memory?

Would not be better just to search if the origin is registered on the AllowedCorsOrigins Table?
I am doing currently an implementation of this cors service and I would like to understand why this is like this.

[brockallen]

It's so we can run this line:

https://github.com/IdentityServer/IdentityServer4/blob/44651bea9b02c992902639b21205f433aad47d03/src/EntityFramework.Storage/src/Services/CorsPolicyService.cs#L52

since we don't know how the DB does it

[Jonatthu]

Would not be better to say something like:
Assuming the origins are normalized on the cors table...

dbContext.AllowedCorsOrigins.Any(x => x.Origin == origin.ToNormalizedOrigin()));

So you do not bring all the cors origins to memory

@brockallen

Because after all you just want to know if the cors origin url is registered on the cors table, right?

[brockallen]

Perhaps, but it wasn't originally designed that way.

[Jonatthu]

@brockallen I am implementing Identity Server for DotNet Graphql usage there is a lot of benefits and I am normalizing the current database implementation repo, I'd like to see a way to show you the benefits without publishing this to public until is ready. Maybe we can make it part of IdentityServer Organization .

[brockallen]

Sure, we're open to suggestions/ideas. My concern is that normalizing an origin is harder than we might think. Maybe it's not, though.

[brockallen]

Any update?

[Jonatthu]

@brockallen is there any way to contact you to share screens and show you the project in action?

[brockallen]

You can paste screen shots here.

[brockallen]

Any update?

[Jonatthu]

Except for the UI Playground everything else was generated and enhancing the strong type of Identity server and docs for a API server, allowing to have a nice UI Admin Panel to manage all the resources and clients.

The tool that I am using to generate services, interfaces and repositories and dbcontext is something that I have been working on for almost 3 years, it is like prisma but for dotnet core generating the whole graphql schema and possibilities to query against the db. Allowing to configure what's public or private.

I am still thinking on what to do with this project, but I would like to open source the Identity Serv er Graphql API and join it as part of Identity server org

[Jonatthu]

@brockallen would be nice to have a chat if you are interested on the details of this

[Jonatthu]

The cool part of this project is that developers can override and extend the project, since all the interfaces to do this got generated, so basically allows to override the db context, services and repositories. This supports batching add, edit and delete out of the box

[Jonatthu]

Only from a class library and using POCO classes can generate the whole api

and extend the generated api by custom fields that the generator does not support

[Jonatthu]

@brockallen this is my update :)

[Jonatthu]

@brockallen Any feedback?

[brockallen]

I've been traveling for the past 10 days. I'll have to look at it late next week.

[Jonatthu]

@brockallen well 14 days have passed, do you need something else to understand the full concept from me? Or any questions?

[brockallen]

So yes, I see the point about querying all rows. This can be improved, but given the EF changes in ASP.NET Core 3 we can't fix this without a breaking change. I will leave this open to address in our 4.0 version. Thanks.

[brockallen]

This will require adding a DbSet<ClientCorsOrigin> on the IConfigurationDbContext.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[brockallen]

PR submitted. Please have a look.

[brockallen]

Any feedback on the PR?

[brockallen]

PR merged

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.