Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed {request>user_id} to {user_id} #39

Merged
merged 1 commit into from
Mar 9, 2024

Conversation

steffenbusch
Copy link
Contributor

The {request>user_id} used in the README of this plugin as well as in the definition of commonLogFormat is not returning the value that represents "user_id": "foobar" in the JSON access logs / the value of the placeholder http.auth.user.id.

I am not sure why {request>user_id} was used, but {user_id} works for me in case of basic_auth usage or using the new functionality described here caddyserver/caddy#6108

For me, {user_id} instead of {request>user_id} makes sense because the user_id is on the same level as ts ({ts}), status ( {status}) or size ({size}) as described in https://caddyserver.com/docs/logging#structured-logs

{
	"level": "info",
	"ts": 1646861401.5241024,
	"logger": "http.log.access",
	"msg": "handled request",
	"request": {
		"remote_ip": "127.0.0.1",
		"remote_port": "41342",
		"client_ip": "127.0.0.1",
		"proto": "HTTP/2.0",
		"method": "GET",
		"host": "localhost",
		"uri": "/",
		"headers": {
			"User-Agent": ["curl/7.82.0"],
			"Accept": ["*/*"],
			"Accept-Encoding": ["gzip, deflate, br"],
		},
		"tls": {
			"resumed": false,
			"version": 772,
			"cipher_suite": 4865,
			"proto": "h2",
			"server_name": "example.com"
		}
	},
	"bytes_read": 0,
	"user_id": "",
	"duration": 0.000929675,
	"size": 10900,
	"status": 200,
	"resp_headers": {
		"Server": ["Caddy"],
		"Content-Encoding": ["gzip"],
		"Content-Type": ["text/html; charset=utf-8"],
		"Vary": ["Accept-Encoding"]
	}
}

This PR fixes the README and the definition of commonLogFormat.

Copy link
Member

@mohammed90 mohammed90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha, I'm surprised nobody has noticed for 4 years. Thanks!

@mohammed90 mohammed90 merged commit d5a649a into caddyserver:master Mar 9, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants