[Logging] Add remote IP log context on authn failure #1019
Conversation
riton
force-pushed
the
feature/log_remote_client_ip
branch
from
238ef64 to
6c8a643
Compare
| }, | ||
| description: $localize`Should the backend trust proxies to extract remote Client IP`, | ||
| }) | ||
| trustProxy: string = "false"; |
There was a problem hiding this comment.
is this a boolean? then please use a bollean value (i,e: trustProxy: boolean= false;)
There was a problem hiding this comment.
ahh I see how this works. Reading up on this: https://expressjs.com/en/guide/behind-proxies.html
This value sets the number of hops. Can you improve on that? its not clear form the name and the description what is happening.
Like: changing the config name or separating into a sub config. like: "class trustedProxyConfig {enable:boolean, trustedHop:number}"
There was a problem hiding this comment.
Indeed, the name of the config setting is not easy to choose.
Express trust proxy accepts boolean, string or number values and each have different meaning.
My idea was to just pass through the value to epxress and allow the end user to specify the exact value type he wants.
There was a problem hiding this comment.
ahh true. sorry I misunderstood the code (I read that by def it will convert the value to number).
In this case it looks good to me.
|
generally looks good. I had some confusion about the naming. Can you improve on that? Than you for your contribution! |
riton
force-pushed
the
feature/log_remote_client_ip
branch
from
6c8a643 to
182d5d4
Compare
|
than you for your contribution! |
2 participants
Fixes #1014
Add support for express trust proxy and remote client IP in failed authentication logs.