On Mon, Mar 16, 2026 at 10:29 PM Zsolt Parragi <[email protected]> wrote: > > As far as I understand, for a programmer error, Assert should be used. Why > > do we use elog(ERROR) here? > > +1
A couple reasons: - to keep it parallel with the similar elog directly above it - to strengthen the API boundary for an esoteric edge case without requiring assertions to be enabled I don't mind relying on assertions for (not an exhaustive list!) well-exercised code, or when performance is critical, or when the callers and callees are in the same source file, or when it's not a security-critical context... This would seem to fail all of those tests. I imagine some of the existing assertions in OAuth should ideally be strengthened into production checks, too. I've considered adding a "production assert" variant for our security code in general, client- and server-side. > I would also say that for CheckSASLAuth, specifying abandoned is > always required, since the caller can't know when it will result in an > error. That's not really true, because the caller hardcodes the mechanism descriptor. The layers above and below CheckSASLAuth are coupled via injection -- ClientAuthentication knows full well that a uaOAuth HBA entry can't be satisfied by the SCRAM mechanisms, and vice-versa. If that were to change in a meaningful way, then sure, the caller would need to always provide the currently-OAuth-specific-edge-case flag. (But in that case, if the caller somehow doesn't have to know the mechanism in use, we could presumably also centralize a single call to CheckSASLAuth().) > Have you considered adding the error level here instead, the same way > as in auth_failed, explicitly defaulted to normal fatal by the caller, > so existing code don't have to change it? That wouldn't need an > SASL-specific explanation or flag in the generic code. I don't think I can visualize what you're proposing. If you mean that CheckSASLAuth should set the elevel with an output parameter, I'd rather not; that moves the responsibility for a very critical assumption (we're ending the process now) across a bunch of different files. (If more things than OAuth need this eventually, maybe it becomes STATUS_SILENT_ERROR or something, to make it even more generic?) Thanks, --Jacob
