Fix discovery / wiring papercuts surfaced by downstream feedback#127
Merged
antosubash merged 9 commits intomainfrom Apr 30, 2026
Merged
Fix discovery / wiring papercuts surfaced by downstream feedback#127antosubash merged 9 commits intomainfrom
antosubash merged 9 commits intomainfrom
Conversation
AdminModule references SimpleModule.OpenIddict.Contracts but injects IOpenIddictSessionContracts (whose impl ships in SimpleModule.OpenIddict) into its session endpoints. Without OpenIddict installed, ASP.NET's minimal-API parameter classifier falls through to body-binding and the host crashes at MapModuleEndpoints with 'Body was inferred but the method does not allow inferred body parameters' — a message that gives no clue about the actual missing dependency. Override ConfigureMiddleware (which the source generator runs before MapModuleEndpoints) to probe for IOpenIddictSessionContracts and throw a directive error pointing at the missing package.
Deploying simplemodule-website with
|
| Latest commit: |
d42965b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d77a7450.simplemodule-website.pages.dev |
| Branch Preview URL: | https://claude-fix-admin-hidden-runt.simplemodule-website.pages.dev |
Same shape as the AdminModule/OpenIddict fix. EmailModule references SimpleModule.BackgroundJobs.Contracts but injects IBackgroundJobs into EmailService and EmailJobRegistrationHostedService. Without the BackgroundJobs module installed, the host crashes at first resolution with 'Unable to resolve service for type IBackgroundJobs' — useful but still not directive about which package is missing. Probe in ConfigureMiddleware (which runs before MapModuleEndpoints) so the failure points at the missing dependency.
@simplemodule/client/resolve-page hardcoded the assembly name to
SimpleModule.${moduleName}, which only works for framework-published
modules. Downstream apps that ship modules under a bare assembly name
(Customers, Invoices, …) had to roll their own resolver.
Try the bare assembly name first, then fall back to SimpleModule.X so
framework modules continue to resolve. Throw a directive error listing
both candidates if neither bundle loads.
The framework called AddAuthorization() with no FallbackPolicy, so any endpoint without an explicit RequireAuthorization() was silently public. Module groups already RequireAuthorization() via the source generator, but plain app.MapGet(...) calls outside any module group inherited nothing. The wrong default for a business app — and easy to miss on review since the symptom is invisible. Set FallbackPolicy = RequireAuthenticatedUser. Endpoints that genuinely need to be public (login, health probes, error/404 fallbacks) opt out explicitly with .AllowAnonymous(). The framework's existing health, error, and fallback handlers already do this; add a comment so a downstream cleanup pass doesn't accidentally remove them.
Modules that call useTranslation('X') silently render bare keys when
SimpleModule.Localization isn't installed — every label shows as
'Users.Title', 'Roles.ColName', etc. The cause is that
LocaleResolutionMiddleware (Localization-only) is what populates
props.translations on Inertia shared data; without it the property is
undefined and every key falls through to the bare-key fallback.
Detect the missing-bundle case (props.translations undefined, not
empty) and warn once per namespace in dev. Behaviour is unchanged in
production builds.
Modules currently inject IHttpContextAccessor and walk claims by hand to get the user id, role, or permission state — the framework already knows how to do this (ClaimsPrincipalExtensions handles the sub-vs-NameIdentifier difference, HasPermission handles wildcards and the Admin bypass) but never exposed it as an injectable service. Add a thin ICurrentUser interface backed by IHttpContextAccessor, delegating to the existing extensions. Modules can now inject ICurrentUser instead of repeating the boilerplate.
Cookie auth's default OnRedirectToLogin sniffs the Accept header to decide between 302-to-login and bare 401, but the heuristic is unreliable: /api/invoices returns 401 while /api/invoices/1 returns 302 to /Identity/Account/Login. Same authorization, different responses, broken JS clients. Override OnRedirectToLogin / OnRedirectToAccessDenied to short-circuit to 401 / 403 for any path under /api. Browser navigations outside /api keep the redirect.
antosubash
changed the title
The fail-fast checks in Admin and Email modules called GetService<T>() on the root container, which threw "Cannot resolve scoped service from root provider" because the contract types are registered as scoped. The host crashed during ConfigureMiddleware before any endpoint could be mapped. Switch to IServiceProviderIsService.IsService(...), which inspects the descriptor table without instantiating the service.
The new RequireAuthenticatedUser fallback policy applies to every endpoint without an explicit policy — including the static-asset endpoints created by MapStaticAssets. That redirected /_content/<module>/<module>.pages.js to /Identity/Account/Login, so the React Inertia bundle could not load on anonymous pages and the login form never rendered. Also update the FileStorage e2e test that asserted /api/* returns 302; it now returns 401, which is what the cookie OnRedirectToLogin override produces for /api requests.
10 tasks
antosubash
added a commit
that referenced
this pull request
Apr 30, 2026
* fix: address remaining issues from downstream feedback batch Closes the four issues left open after #127 and #139: - #137 — `sm new module` now scaffolds a `<Name>Permissions.cs` so the IModulePermissions pattern is discoverable without source-diving. SimpleModule.Permissions README documents the convention with a worked example, and the constitution clarifies that ConfigurePermissions is no longer required (auto-discovered). - #135 — adds `scripts/smoke-test-nupkg-consumer.mjs` plus a CI step that builds a throwaway PackageReference-based consumer against the freshly packed nupkgs and asserts each module's static web assets propagate through the static-asset manifest. Catches the regression class where the package looks fine on disk but the consumer's wwwroot pipeline doesn't pick the assets up — invisible to the in-repo Host because it uses ProjectReference. - #133 — extracts `framework/SimpleModule.Testing` (TestAuthHandler, TestAuthDefaults, AddTestAuthentication, CreateAuthenticatedClient WebApplicationFactory extensions) so downstream apps can pull the helpers from NuGet instead of copy-pasting. Tests.Shared now consumes it; existing 40+28+20 (Users/Admin/OpenIddict) and 209/195 (Core/Generator) tests stay green. - #130 — adds `ModuleGraphValidator` (opt-in via `SimpleModuleOptions.ValidateModuleGraph`, default on outside Production). Walks loaded SimpleModule.X.Contracts assemblies and logs a warning for every contract interface with no registered implementation — the typical signal that the impl module's package is missing. * refactor: simplify based on /simplify findings - ModuleGraphValidator now uses IServiceProviderIsService.IsService instead of GetService, avoiding eager construction of every contract impl + a redundant scope at startup. - WebApplicationFactoryAuthExtensions uses WellKnownClaims.Permission instead of the bare "permission" string. - Smoke-test consumer puts its NuGet global-packages folder outside the project tree so the SDK's default **/*.cs glob can't reach into it, removing the DefaultItemExcludes workaround. - Drops the now-unused TestAuthScheme const alias on SimpleModuleWebApplicationFactory; remaining call site uses TestAuthDefaults.AuthenticationScheme directly. * ci: allowlist SimpleModule.Testing in framework scope check
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Originally scoped to the Admin/OpenIddict crash named on the branch; expanded to clear the rest of the tractable items from the bug report.
Closes #128
Closes #129
Closes #132
Closes #136
Partially addresses #130 (the two specific peer-dep crashes it lists are now caught with directive errors; the generic boot-time validator it proposes is left for a follow-up).
Commits
AdminModule.ConfigureMiddlewarevalidatesIOpenIddictSessionContractsis registered, throws a directive error beforeMapModuleEndpointscan produce the misleading "Body was inferred…" crash.EmailModule.ConfigureMiddlewareprobes forIBackgroundJobs.resolvePagetries the bare assembly name first, falls back toSimpleModule.X, fails with both candidates listed.AddAuthorizationnow setsFallbackPolicy = RequireAuthenticatedUser. The framework's existing anonymous health/error/fallback handlers got a comment so they don't get swept up in a downstream "remove all.AllowAnonymous()" pass.useTranslationwarns once per namespace in dev whenprops.translationsis undefined, pointing at the missingSimpleModule.Localizationmodule.ICurrentUser(Id / IsAuthenticated / IsInRole / HasPermission / Principal) backed byIHttpContextAccessor, registered inAddSimpleModuleInfrastructure.UsersModuleoverrides cookie auth'sOnRedirectToLogin/OnRedirectToAccessDeniedto short-circuit to 401/403 for any path under/api. Browser routes keep the redirect.Out of scope (deferred — issues remain open)
Pages/Index.tsxcase collision — needs build-time validation in the Vite plugin or a generator diagnostic.SimpleModule.Tests.Sharedpackaging — workflow change, not a code fix.IModulePermissionsfor downstream module authors.Test plan
dotnet buildclean across the full solutiondotnet testclean for affected modules: Admin (28 ✓), Email (30 ✓), Users (40 ✓), Permissions (15 ✓), FeatureFlags (14 ✓), Marketplace (19 ✓), Products (39 ✓), Orders (24 ✓), PageBuilder (43 ✓), AuditLogs (37 ✓), OpenIddict (20 ✓), Settings (27 ✓), Tenants (22 ✓), Map (24 ✓), Core (209 ✓), BackgroundJobs (97 ✓), Localization (26 ✓)/api/anythingunauthenticated → 401 (not 302)