Fix OAuth validator docs for error_detail on internal errors
Hi,
While testing “[d438a3659] oauth: Let validators provide failure DETAILs”, I noticed a tiny doc issue.
With this feature, when a validator returns false, result->error_detail can carry an error message. However, it seems that the previous paragraph was not updated:
```
<para>
A validator may return <literal>false</literal> to signal an internal error,
in which case any result parameters are ignored and the connection fails.
```
“Any result parameters are ignored” is no longer accurate; it should be something like “any result parameters except result->error_detail are ignored”. This patch just makes that tiny doc fix.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
v1-0001-doc-Clarify-OAuth-validator-error_detail-handling.patchapplication/octet-stream; name=v1-0001-doc-Clarify-OAuth-validator-error_detail-handling.patch; x-unix-mode=0644Download+4-4
On 4 Jun 2026, at 14:33, Chao Li <li.evan.chao@gmail.com> wrote:
“Any result parameters are ignored” is no longer accurate; it should be something like “any result parameters except result->error_detail are ignored”. This patch just makes that tiny doc fix.
That's true, but error_detail is explained in detail in the next paragraph so
I'm not sure this change is needed.
Another thing we don't explicitly document which seems more interesting is that
authn_id is used even in case of failure if log_connections is enabled. Maybe
that deserves a mention?
--
Daniel Gustafsson
On Jun 5, 2026, at 04:19, Daniel Gustafsson <daniel@yesql.se> wrote:
On 4 Jun 2026, at 14:33, Chao Li <li.evan.chao@gmail.com> wrote:
“Any result parameters are ignored” is no longer accurate; it should be something like “any result parameters except result->error_detail are ignored”. This patch just makes that tiny doc fix.
That's true, but error_detail is explained in detail in the next paragraph so
I'm not sure this change is needed.
Agreed. Adding the “exception for result->error_detail” sounds a bit redundant with the next paragraph. But “any result parameters are ignored” also seems to conflict with the next paragraph, so I think we can just delete that part.
ValidatorModuleResult has three fields, so the logic is:
* The first paragraph talks about authorized and authn_id when the validator succeeds.
* The second paragraph talks about the validator’s return values.
* The third paragraph talks about result->error_detail when the validator fails.
Another thing we don't explicitly document which seems more interesting is that
authn_id is used even in case of failure if log_connections is enabled. Maybe
that deserves a mention?
This is a good point. I added that in v2.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
v2-0001-doc-clarify-OAuth-validator-authn_id-logging-on-a.patchapplication/octet-stream; name=v2-0001-doc-clarify-OAuth-validator-authn_id-logging-on-a.patch; x-unix-mode=0644Download+9-5
On 5 Jun 2026, at 00:21, Chao Li <li.evan.chao@gmail.com> wrote:
On Jun 5, 2026, at 04:19, Daniel Gustafsson <daniel@yesql.se> wrote:
On 4 Jun 2026, at 14:33, Chao Li <li.evan.chao@gmail.com> wrote:
“Any result parameters are ignored” is no longer accurate; it should be something like “any result parameters except result->error_detail are ignored”. This patch just makes that tiny doc fix.
That's true, but error_detail is explained in detail in the next paragraph so
I'm not sure this change is needed.Agreed. Adding the “exception for result->error_detail” sounds a bit redundant with the next paragraph. But “any result parameters are ignored” also seems to conflict with the next paragraph, so I think we can just delete that part.
ValidatorModuleResult has three fields, so the logic is:
* The first paragraph talks about authorized and authn_id when the validator succeeds.
* The second paragraph talks about the validator’s return values.
* The third paragraph talks about result->error_detail when the validator fails.Another thing we don't explicitly document which seems more interesting is that
authn_id is used even in case of failure if log_connections is enabled. Maybe
that deserves a mention?This is a good point. I added that in v2.
This version looks good to me, the authn_id sentence is a bit long so I might
do some careful rewording before pushing.
--
Daniel Gustafsson
On Fri, Jun 5, 2026 at 12:32 PM Daniel Gustafsson <daniel@yesql.se> wrote:
This version looks good to me, the authn_id sentence is a bit long so I might
do some careful rewording before pushing.
+1. I'll also think about how to better document the (intentional)
recording of authn_id during failed authorization, but it doesn't need
to block this.
--Jacob
On 5 Jun 2026, at 23:16, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
On Fri, Jun 5, 2026 at 12:32 PM Daniel Gustafsson <daniel@yesql.se> wrote:
This version looks good to me, the authn_id sentence is a bit long so I might
do some careful rewording before pushing.+1. I'll also think about how to better document the (intentional)
recording of authn_id during failed authorization, but it doesn't need
to block this.
Pushed with a little bit of wordsmithing.
--
Daniel Gustafsson
On Jun 6, 2026, at 06:22, Daniel Gustafsson <daniel@yesql.se> wrote:
On 5 Jun 2026, at 23:16, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
On Fri, Jun 5, 2026 at 12:32 PM Daniel Gustafsson <daniel@yesql.se> wrote:
This version looks good to me, the authn_id sentence is a bit long so I might
do some careful rewording before pushing.+1. I'll also think about how to better document the (intentional)
recording of authn_id during failed authorization, but it doesn't need
to block this.Pushed with a little bit of wordsmithing.
--
Daniel Gustafsson
Hi Daniel, thank you very much for taking care of this patch.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/