Say so at startup when anyone can stop a checker without logging in - #42
Merged
Merged
Conversation
AddHealthieController does not require authorization unless it is asked to, and the dashboard's AllowMutations defaults to true. Both are deliberate. The controller ships into someone else's MVC pipeline and cannot demand a policy it knows nothing about; a read-only dashboard would make every first run look broken. Neither default is wrong, and changing either breaks every application that maps them. What was missing was anyone being told. An application that maps one of these and stops there lets whoever can reach it stop a checker, slow its interval, or clear a failing streak -- which hides an incident rather than reporting one, from a tool whose whole job is to report them. That is worth a line in the log at startup. The check reads the endpoints, not the flags that built them. A host may have applied authorization some other way -- RequireAuthorization, an endpoint group, an attribute of its own -- and warning at an application that is already secured is how a warning gets filtered out, after which it is not there for the one that needs it. Only mutating methods are named, because reading health without authenticating is a choice an operator can reasonably make and listing it would drown the part that matters. Both shapes had to be handled, and the tests are why that is known: looking only for IAuthorizeData warned at AddHealthieController(requireAuthorization: true), because the convention adds an AuthorizeFilter, which is an IFilterMetadata and not an IAuthorizeData. The one configuration that had asked for authorization by name was the one being warned about. Healthie.Mcp is left alone: its AllowMutations defaults to false, and when it is off the tools that change anything are not registered at all.
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
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.
Closes the authorization-defaults question by keeping both defaults and making them loud.
AddHealthieControllerdoesn't require authorization unless asked, and the dashboard'sAllowMutationsdefaults totrue. Both are deliberate — the controller ships into someone else's MVC pipeline and can't demand a policy it knows nothing about, and a read-only dashboard would make every first run look broken. Changing either breaks every application that maps them.What was missing was anyone being told. An application that maps one of these and stops there lets whoever can reach it stop a checker, slow its interval, or clear a failing streak — hiding an incident rather than reporting one, from a tool whose whole job is reporting them.
It reads the endpoints, not the flags
A host may have secured things some other way —
RequireAuthorization(), an endpoint group, its own attribute. Warning at an application that is already secured is how a warning gets filtered out, and then it isn't there for the one that needs it. So the check inspects what was actually applied.Only mutating methods are named. Reading health without authenticating is a choice an operator can reasonably make; listing those routes would drown the part that matters.
The tests earned their place
Looking only for
IAuthorizeDatawarned atAddHealthieController(requireAuthorization: true)— because the convention adds anAuthorizeFilter, which isIFilterMetadata, notIAuthorizeData. The one configuration that had explicitly asked for authorization was the one being warned about.AnApiThatRequiresAuthorization_IsNotWarnedAtcaught it.Seven tests, driven through a real host on an OS-assigned port so the endpoints genuinely exist and carry their metadata:
AuthorizeFilterRequireAuthorization473 tests green on net8.0 and net10.0, 0 errors / 0 warnings.
Scope
Healthie.Mcpis left alone — itsAllowMutationsdefaults tofalse, and when off the mutating tools aren't registered at all, so there's nothing to warn about.Logged once at startup, at
Warning. It describes a configuration an operator chose and can change, not a runtime fault, so repeating it per request would bury the logs it's trying to be noticed in.