LoadClientAsync(id) on ClientController used to trip an
IndexOutOfRangeException at the InMemory shaper for any
.Include() of one of three Client navs: RedirectUris,
AllowedScopes, AllowedGrantTypes. Five other Client navs (with
the same EF shape and the same application-level config) worked
fine.
Bisection pointed at the InMemory provider; that hypothesis was
wrong. The real cause is in ApplicationDbContext.OnModelCreating:
yavsc was redeclaring the HasOne<Client>().WithMany(...).
HasForeignKey(e => e.ClientId) for all eight Client* navs. The
same relation is already declared (more completely, with
.IsRequired().OnDelete(DeleteBehavior.Cascade)) by
IdentityServer8's ConfigureClientStore via ModelBuilderExtensions.
The redundant mapping on three specific entities — ClientScope,
ClientRedirectUri, ClientGrantType — interacts with the InMemory
provider's shaper in a way that throws IndexOutOfRange. Removing
the redundancy fixes it.
This commit also walks back b12c272d:
- Drops .AsSplitQuery() from LoadClientAsync (no longer needed
for the InMemory shaper, and the Postgres path it was a hedge
against was a false alarm — there is no Postgres production
bug here, only an InMemory shaper quirk surfaced by the
redundant mapping).
- Removes the 9 Bisect_*_alone tests that were the artefact of
the provider-hypothesis phase. They pointed at the right
entities but for the wrong reason.
- Keeps EditRedirectUris_GET_after_add_lists_both_uris as the
end-to-end regression sentinel: with the fix in place, it
loads a Client with two RedirectUris and asserts both are
rendered. Without the fix, it fails with IndexOutOfRange.
LoadClientAsync chains 9 .Include() calls on dbContext.Clients.
On Postgres (and InMemory for some IdentityServer8 nav types), the
resulting cartesian product trips the query shaper with
IndexOutOfRangeException at IncludeCollection materialisation time.
Bug reproduces in production on the Blog admin pages that load a
Client by id.
AsSplitQuery() rewrites the load as 9 separate SELECTs joined by
client id, which sidesteps the cartesian explosion and any shaper
ambiguity between Claims/Properties/ClientSecrets (which share
Type/Value column names across some IdentityServer8 versions).
Tests:
- EditRedirectUris_GET_after_add_lists_both_uris: end-to-end
reproducer that adds a second RedirectUri via POST then re-GETs
the editor. Guards the fix on the integration path.
- Bisect_*_alone: nine unit tests that exercise the same
.SingleOrDefaultAsync(c => c.Id == id).Include(nav) on the
InMemory provider, one nav at a time. Pinpointed three
problematic navs (RedirectUris, AllowedScopes, AllowedGrantTypes)
on InMemory; kept as a regression net for any future shaper
regressions on the InMemory provider (not the Postgres path).
The partial class ClientController had two constructors declared
across ClientController.cs and ClientController.Collections.cs.
ASP.NET Core DI failed to pick one at request time with:
System.InvalidOperationException: Multiple constructors accepting
all given argument types have been found in type
'Yavsc.Controllers.ClientController'.
Move IHtmlLocalizer<ClientController> into the primary constructor
in ClientController.cs and drop the duplicate one in
ClientController.Collections.cs. The Collections partial now keeps
only its readonly field and action methods; the constructor and
field assignment are unified on the main file.
Also add the missing 'using Microsoft.AspNetCore.Mvc.Localization;'
to ClientController.cs so IHtmlLocalizer resolves.
The OAuth2 client editor at /Client/Edit/{id} previously exposed 8
fields out of ~30 scalars and 10 collections on the IdentityServer8
Client entity. Editing the collections (RedirectUris, Scopes, Grant
Types, Cors Origins, IdP Restrictions, Claims, Properties, Secrets)
was either impossible or jammed into a single broken text input that
bound against an IEnumerable<string> property.
Restructure into per-collection subpages, each with its own
list/add/remove flow:
- RedirectUris /Client/EditRedirectUris/{id}
- PostLogoutRedirectUris /Client/EditPostLogoutRedirectUris/{id}
- Scopes /Client/EditScopes/{id}
- GrantTypes /Client/EditGrantTypes/{id}
- CorsOrigins /Client/EditCorsOrigins/{id}
- IdPRestrictions /Client/EditIdPRestrictions/{id}
- Claims /Client/EditClaims/{id}
- Properties /Client/EditProperties/{id}
- Secrets /Client/EditSecrets/{id}
Implementation:
- New partial class ClientController.Collections.cs with one
GET/Add/Remove trio per collection. Add/Remove dispatch through
generic helpers that handle the EF row + ClientId check.
- Shared _EditableStringList.cshtml partial consumed by the six
single-string-field collection pages. Uses reflection to pull
the value field and the row Id off the entity — avoids six
nearly-identical table+form copies.
- Claims / Properties / Secrets each have their own view because
they carry 2+ fields (Type+Value, Key+Value, or
Type+Value+Description+Expiration).
- Main Edit.cshtml enriched: ClientId/Id hidden, all scalar
fields split into fieldsets (Core, Security, Logout, Tokens,
Device flow, Tokens extra), nav links to the 9 subpages with
current row counts as badges.
- ClientController.Edit(int) GET now loads the client with all
navigations via LoadClientAsync so the Edit.cshtml nav badges
render real counts.
Field-correctness notes (verified by disassembling HigginsSoft
IdentityServer8.EntityFramework.Entities.Client 8.0.5-preview-net9):
- The property is PairWiseSubjectSalt, not PairwiseSubjectSalt
(capital W on 'Wise').
- CibaLifetime and PollingInterval do NOT exist on Client in this
IdentityServer8 version — those properties were a guess. The
Device flow fieldset contains DeviceCodeLifetime + UserCodeType
instead.
- AllowedIdentityTokenSigningAlgorithms and AllowAccessTokensViaBrowser
were missing from the original form and are now exposed.
- ConsentLifetime and UserSsoLifetime are int? (nullable); the form
binds them as plain int fields which accept empty strings.
Security:
- All new actions stay under [Authorize('AdministratorOnly')].
- Each Add/Remove takes an explicit id (Client.Id) and the row's
ClientId is checked on the server before any delete; a rowId
from another client returns NotFound.
Docs:
- doc/dev-tracking/client-editor-overhaul.md — inventory, status,
follow-up ideas (confirmation prompts, validation, MVC tests).
The Site:Audience setting was conflating two distinct concepts: an OAuth
JWT audience (a single resource identifier) and a CORS allow-list (an
array of origins). Collapsing them caused several latent bugs:
- OAuth/JWT validation expected a single string while CORS WithOrigins
accepts an array.
- Password-reset callback URLs and OAuth client RedirectUri/Origin were
being built from what was meant to be an audience identifier, not a
base URL.
- Yavsc.Org's main CORS policy was hardcoded to '*', with no way to
restrict it without code changes.
Changes:
- SiteSettings.Audience (string) replaced with CorsAllowedOrigins
(IList<string>).
- OAuth JWT Authority still reads Site:Authority; Audience now reads
Site:ExternalUrl (Org only; Api/Blogs use ValidateAudience=false).
- MailSender and AccountController build reset-callback URLs from
Site:ExternalUrl.
- ClientController uses Site:ExternalUrl for OAuth RedirectUri/Origin
defaults on newly created clients.
- Yavsc.Api and Yavsc.Blogs now read CORS origins from
Site:CorsAllowedOrigins instead of hardcoded URLs.
Add shared AddYavscCors / AddYavscJwtBearer extension methods in
Yavsc.Server/Helpers/ServiceExtensions.cs to enforce a single
configuration contract across all runtime services (Api, Blogs, Org).
Fails closed when CorsAllowedOrigins is empty; fails fast at startup
when Site:Authority is missing.
Remove obsolete ConfigurationHelpers.GetAudience (no remaining callers).
Local appsettings-*.json files (which carry deployment-specific values
and are gitignored) must be updated to add Site:CorsAllowedOrigins.
